ci: add clippy, fmt, and cargo-deny gates behind a CI Green aggregator - #158
Conversation
Adopts the CI check pattern from lean-ctx's PR #721: cargo clippy (-D warnings, exempting the repo's existing warn-only unsafe_code and clippy::pedantic policy), cargo fmt --check, and cargo-deny (license and dependency policy, deny.toml adopted verbatim from lean-ctx since its allow-list/bans/sources policy applies unchanged here). A CI Green job aggregates build/clippy/fmt/deny so branch protection can point at one stable check instead of every individual leg. Also runs cargo fmt across the whole workspace (first pass, no prior formatting enforcement existed) and fixes every clippy finding the new gate surfaces: real bugs (redundant match guard, needless range loop, unnecessary sort_by, lines().flatten() looping forever on stream errors, duplicated if/else branches, nested format! args), plus targeted #[allow] suppressions for pre-existing dead code and too-many-arguments functions that are out of scope for this change.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds Clippy, formatting, cargo-deny, and aggregate CI checks. It introduces cargo-deny policy configuration and applies broad Rust formatting, lint annotations, idiomatic refactors, and unchanged-behavior test updates across workspace crates and application modules. ChangesRepository-wide CI and Rust cleanup
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- The dtolnay/rust-toolchain and EmbarkStudios/cargo-deny-action SHAs
copied from lean-ctx's workflow do not exist in those repos (zizmor's
impostor-commit audit, confirmed independently via the GitHub API).
Repinned both to their actual, verified commits.
- crates/agentflare-artifacts/src/store.rs: CI's clippy (newer toolchain
than local) flagged a redundant `&` in format!("/{}", &id) —
useless_borrows_in_formatting.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
93-94: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRun Clippy with the committed lockfile.
Add
--locked; otherwise Cargo may updateCargo.lockduring CI and lint a dependency graph different from the one reviewed. Apply the same reproducibility policy to the existing build/test commands.[details]
- run: cargo clippy --workspace --all-targets --all-features -- -D warnings -A unsafe_code -A clippy::pedantic + run: cargo clippy --locked --workspace --all-targets --all-features -- -D warnings -A unsafe_code -A clippy::pedantic[/details]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 93 - 94, Update the Clippy command in the workflow to include Cargo’s --locked flag, then apply the same flag to the existing Cargo build and test commands so CI always uses the committed Cargo.lock without modifying or resolving dependencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 72-154: Add an explicit least-privilege permissions declaration
for the workflow, granting only contents: read to the checkout and CI jobs; if
permissions are scoped per job, set ci-green permissions to {} since it does not
access repository contents. Place the declaration at the workflow level or apply
equivalent job-level settings without changing the existing gate behavior.
In `@deny.toml`:
- Line 10: Update the cargo-deny policy settings for yanked crates, unknown
registries, and unknown Git sources to use deny-level enforcement instead of
warnings, while preserving any intentional explicit exceptions. Ensure the
dependency source policy, including allow-git, causes CI to fail for unapproved
sources.
In `@src/agent_launch.rs`:
- Around line 322-324: Update the test around run_launch to use an explicit
match that asserts the NotFound message contains “not found on PATH” and panics
for Launched, UnknownAgent, or Extension outcomes, ensuring unexpected launch
results fail the test.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 93-94: Update the Clippy command in the workflow to include
Cargo’s --locked flag, then apply the same flag to the existing Cargo build and
test commands so CI always uses the committed Cargo.lock without modifying or
resolving dependencies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 72d51444-8acb-4492-99b9-12487de94ecc
📒 Files selected for processing (108)
.github/workflows/ci.ymlcrates/agent-registry/src/detect.rscrates/agent-registry/src/lib.rscrates/agent-registry/src/registry.rscrates/agentflare-artifacts/src/lib.rscrates/agentflare-artifacts/src/server.rscrates/agentflare-artifacts/src/store.rscrates/caveman/src/compress.rscrates/caveman/src/lib.rscrates/caveman/src/llm.rscrates/caveman/src/prompt.rscrates/caveman/src/sensitive.rscrates/caveman/src/validate.rscrates/gateway-registry/src/audit.rscrates/gateway-registry/src/circuit.rscrates/gateway-registry/src/config.rscrates/gateway-registry/src/db.rscrates/gateway-registry/src/error.rscrates/gateway-registry/src/lib.rscrates/gateway-registry/src/mcp_http.rscrates/gateway-registry/src/mcp_stdio.rscrates/gateway-registry/src/redact.rscrates/gateway-registry/src/registry.rscrates/gateway-registry/src/sanitize.rscrates/gateway-registry/src/search.rscrates/gateway-registry/src/truncate.rscrates/gateway-registry/tests/fixtures/fixture_server.rscrates/gateway-registry/tests/gateway_audit_log.rscrates/gateway-registry/tests/mcp_http_call.rscrates/gateway-registry/tests/mcp_http_circuit_breaker.rscrates/gateway-registry/tests/mcp_http_discover.rscrates/gateway-registry/tests/mcp_stdio_call.rscrates/gateway-registry/tests/mcp_stdio_circuit_breaker.rscrates/gateway-registry/tests/mcp_stdio_discover.rscrates/gateway-registry/tests/mcp_stdio_reconnect.rscrates/gateway-registry/tests/mcp_stdio_timeout.rscrates/gateway-registry/tests/registry.rscrates/gateway-registry/tests/support/mod.rscrates/ponytail/src/config.rscrates/ponytail/src/detect.rscrates/ponytail/src/instructions.rscrates/ponytail/src/lib.rscrates/ponytail/src/platform.rscrates/ponytail/src/state.rscrates/ponytail/src/sub_skills.rscrates/ponytail/src/switcher.rscrates/skill-registry/src/db.rscrates/skill-registry/src/frontmatter.rscrates/skill-registry/src/lib.rscrates/skill-registry/src/load.rscrates/skill-registry/src/search.rscrates/skill-registry/src/sources.rscrates/skill-registry/tests/golden_queries.rsdeny.tomlsrc/agent_install.rssrc/agent_launch.rssrc/agents.rssrc/alias.rssrc/auth.rssrc/auth_crypt.rssrc/auth_db.rssrc/auth_runner.rssrc/build_time.rssrc/channels.rssrc/claims.rssrc/cli/agents.rssrc/cli/auth.rssrc/cli/caveman.rssrc/cli/channel.rssrc/cli/claim.rssrc/cli/coaching.rssrc/cli/gateway.rssrc/cli/handoff.rssrc/cli/memory.rssrc/cli/ponytail.rssrc/cli/review.rssrc/coaching.rssrc/components.rssrc/cost.rssrc/db.rssrc/dev_vars.rssrc/errors.rssrc/gateway_integrations.rssrc/gateway_secrets.rssrc/hook.rssrc/init.rssrc/main.rssrc/mcp_prompts.rssrc/mcp_server.rssrc/memory/mcp.rssrc/memory/observations.rssrc/memory/prompts.rssrc/memory/relations.rssrc/memory/search.rssrc/memory/sessions.rssrc/memory/store.rssrc/memory/summaries.rssrc/mise_install.rssrc/optimize.rssrc/pricing.rssrc/review.rssrc/rollup.rssrc/shell.rssrc/state.rssrc/tool_install.rssrc/uninstall.rssrc/update.rstests/caveman_cli.rs
| # all vulnerability/notice advisories now emit errors unless explicitly ignored. | ||
| # `unmaintained`/`unsound` now control which dependencies can fail the check. | ||
| unmaintained = "workspace" | ||
| yanked = "warn" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the policy file and any cargo-deny references
git ls-files | rg '(^|/)deny\.toml$|cargo-deny|deny\.(toml|ya?ml)$|\.cargo-deny'
# Show the relevant section with line numbers
if [ -f deny.toml ]; then
cat -n deny.toml | sed -n '1,140p'
fi
# Search for any documented intent around yanked / git / registry policy
rg -n --hidden --no-ignore-vcs 'yanked|unknown-registry|unknown-git|allow-git|cargo-deny' .Repository: getappz/agentflare
Length of output: 2453
🌐 Web query:
cargo-deny configuration warn deny yanked unknown-registry unknown-git allow-git warnings fail the check semantics
💡 Result:
In cargo-deny, configuration fields such as yanked, unknown-registry, and unknown-git control how the tool evaluates dependency sources and advisories [1][2][3]. These fields accept a lint level, which determines the outcome of the check [2][4][5]. The standard lint levels are: - deny: The tool will emit an error and the check will fail (returning a non-zero exit code) [2][5][6]. - warn: The tool will emit a warning diagnostic, but the check will not fail [2][5][6]. - allow: No warning or error is produced [2][5][6]. Specific behaviors for your listed fields include: - yanked: Found under the [advisories] section, this controls the response when a crate version has been yanked from its registry [3]. Setting this to deny will fail the check if any dependency is yanked [3]. - unknown-registry and unknown-git: Found under the [sources] section, these define how to handle crates sourced from registries or git repositories not explicitly listed in allow-registry or allow-git [2][7][4]. Setting these to deny will fail the check if a dependency is found from an unapproved source [2][4]. - allow-git: This is a configuration list rather than a lint level; it specifies the allowed git repository URLs [2][7]. If a crate's source is not in this list, the behavior is determined by the unknown-git lint level [7]. By default, many of these fields are set to warn [2][3][5]. To enforce strict policies, you must explicitly set these values to deny in your deny.toml file [2][8].
Citations:
- 1: https://embarkstudios.github.io/cargo-deny/checks/cfg.html
- 2: https://docs.rs/crate/cargo-deny/latest/source/deny.template.toml
- 3: https://embarkstudios.github.io/cargo-deny/checks/advisories/cfg.html
- 4: https://github.com/rust-lang/crates.io/blob/main/deny.toml
- 5: https://docs.rs/cargo-deny/latest/src/cargo_deny/lib.rs.html
- 6: https://github.com/EmbarkStudios/cargo-deny/blob/master/src/lib.rs
- 7: https://embarkstudios.github.io/cargo-deny/checks/sources/cfg.html
- 8: https://github.com/EmbarkStudios/cargo-deny/blob/main/deny.template.toml
🌐 Web query:
cargo-deny config severity warn deny yanked unknown-registry unknown-git action behavior
💡 Result:
In cargo-deny, several configuration options control the severity of checks, which determines the action taken when a violation occurs [1]. These severity levels are defined as lint levels [1]: - deny: Produces an error and causes the check to fail [1]. - warn: Produces a warning, but the check will not fail [1]. - allow: Produces no warning or error [1]. Specific configuration behaviors for the fields mentioned are as follows: yanked The [advisories] section includes a setting for yanked crates [2]. It determines the action taken when a crate version that has been yanked from its registry is encountered [2]. You can set this to deny to emit an error, or configure other levels depending on your project needs [2]. Additionally, specific yanked versions can be ignored by adding them to the ignore list in the [advisories] section [1]. unknown-registry This setting, found in the [sources] section, controls what happens when a crate is sourced from a registry not included in the allow-registry list [1][3]. The default behavior is typically set to warn, but it can be changed to deny to enforce stricter registry compliance [1][3]. unknown-git Similar to unknown-registry, this setting in the [sources] section controls the behavior when a crate is sourced from a Git repository not listed in the allow-git configuration [1][4]. It also supports the standard lint levels, with the default often set to warn [1][5]. In summary, for these settings, you define the desired severity (allow, warn, or deny) to dictate whether cargo-deny should permit the dependency, warn the developer, or fail the build [1].
Citations:
- 1: https://docs.rs/crate/cargo-deny/latest/source/deny.template.toml
- 2: https://embarkstudios.github.io/cargo-deny/checks/advisories/cfg.html
- 3: https://embarkstudios.github.io/cargo-deny/checks/cfg.html
- 4: https://embarkstudios.github.io/cargo-deny/checks/sources/cfg.html
- 5: https://github.com/EmbarkStudios/cargo-deny/blob/main/deny.template.toml
Enforce the dependency source and yanked-crate policy.
yanked, unknown-registry, and unknown-git are set to warn, so cargo-deny will only emit warnings and CI can still pass. allow-git = [] is not enforced unless unknown-git is deny; switch these to deny or add explicit exceptions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deny.toml` at line 10, Update the cargo-deny policy settings for yanked
crates, unknown registries, and unknown Git sources to use deny-level
enforcement instead of warnings, while preserving any intentional explicit
exceptions. Ensure the dependency source policy, including allow-git, causes CI
to fail for unapproved sources.
…ed, stricter test
- Add explicit `permissions: contents: read` to the build/clippy/fmt/deny
jobs (only `changes` had it) and `permissions: {}` to ci-green, which
never checks out the repo.
- Run the new clippy job with --locked so CI lints the committed
Cargo.lock instead of silently letting cargo re-resolve it.
- src/agent_launch.rs: launch_not_on_path_errors used `if let` so it
silently passed on any outcome other than NotFound; switched to an
exhaustive match that panics on Launched/UnknownAgent/Extension.
Needed LaunchOutcome to derive Debug for the panic message.
Skipped CodeRabbit's deny.toml suggestion (escalate yanked/unknown-registry/
unknown-git from warn to deny) — deny.toml was deliberately adopted verbatim
from lean-ctx's already-tuned policy per an explicit ask; changing its
semantics needs a separate decision, not folding into this CI-gates PR.
|
Addressed in 59ca9f8:
Intentionally not applying the deny.toml suggestion (escalating yanked/unknown-registry/unknown-git from warn to deny): deny.toml was adopted verbatim from lean-ctx's own tuned cargo-deny policy at the repo owner's explicit request. Changing its enforcement semantics is a separate policy decision, not something to fold silently into a CI-gates PR. |
#573) Same architectural gap as skill-registry (#519/PR #572), which gateway-registry's own doc comments say it mirrors: hand-rolled apply_schema() with no user_version tracking. Audited the git history (#104 -> #158 -> #347) -- the tools table's columns have never changed since creation, so there's no live "no such column" bug today, but the next column addition would hit the identical class of bug. Migrates to agentflare-db-kit's open_file/open_memory with a real migration list: 0001_initial replays the original (#104) narrow schema, 0002_fts_triggers unconditionally drops and recreates tools_fts as the external-content shape with sync triggers plus a backfill. No ALTER TABLE/migration hook needed here (unlike #519) since tools's columns are stable -- DROP ... IF EXISTS before a fresh CREATE is correct regardless of which pre-migration shape existed. Added GatewayError::DbInit for db_kit::open::Error and its error_kind match arm. All 60 gateway-registry unit tests pass, including the existing legacy-standalone-FTS conversion test; clippy (with the CI gate flags) and fmt are clean; the full agentflare binary compiles. Agentflare-Agent: claude-code_2-1-237_agent Agentflare-Branch: task/520-fix-gateway-registry-adopt-agentflare-db Agentflare-Item: 520
2 participants
Summary
clippy,fmt, andcargo-denyjobs to CI, plus aCI Greenaggregator job so branch protection can point at one stable required check (pattern adopted from lean-ctx's PR #721: feat: integrate lean-md as an external lean-ctx addon (+ LSP formatter routing) yvgude/lean-ctx#721)clippyruns-D warningsbut exemptsunsafe_codeandclippy::pedantic, preserving this repo's existing deliberate warn-only[lints]policy (rootCargo.toml+ 3 sub-crates) instead of silently escalating itdeny.tomlis adopted verbatim from lean-ctx (its license allow-list / bans / sources policy applies unchanged to agentflare's dependency tree — confirmed via a localcargo deny checkrun:advisories ok, bans ok, licenses ok, sources ok)cargo fmtacross the whole workspace (no formatting enforcement existed before this) and fixes every clippy finding the new gate surfacesDetails
Clippy fixes were all either genuine bugs or pre-existing dead code:
crates/caveman/src/compress.rs: removed an unread placeholder initializercrates/agentflare-artifacts/src/server.rs: redundant match guardcrates/agentflare-artifacts/src/store.rs:sort_by→sort_by_key(Reverse(...))crates/gateway-registry/src/error.rs: needless range loop inlevenshteinsrc/auth_runner.rs:reader.lines().flatten()→.map_while(Result::ok)(the old form loops forever if the stream errors without EOF)src/auth_db.rs: collapsed twoif/else ifbranches that returned the same valuesrc/cost.rs: nestedformat!informat!args;get().is_none()→contains_keysrc/shell.rs: suppressedenum_variant_names(renaming risks breaking match sites)#[allow(dead_code)]/#[allow(clippy::too_many_arguments)]on pre-existing unused items and wide constructors acrosssrc/errors.rs,src/auth_db.rs,src/memory/{prompts,relations,sessions,observations}.rs,src/optimize.rs,src/mise_install.rs— none are behavior changes, all are out of scope to delete/refactor hereTest plan
cargo build --workspacecargo test --workspace(323 passed, 0 failed)cargo fmt --checkcleancargo clippy --workspace --all-targets --all-features -- -D warnings -A unsafe_code -A clippy::pedanticcleancargo deny checkclean (only warn-level duplicate-version notices, no errors)Summary by CodeRabbit