feat(agent): add native plugin management - #42
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughAdds ChangesAgent integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant CommandRouter
participant AgentManager
participant Codex
participant Claude
CLI->>CommandRouter: parse govctl agent install or update
CommandRouter->>AgentManager: dispatch runtime operation
AgentManager->>Codex: project and install Codex assets
AgentManager->>Claude: project and install Claude assets
AgentManager-->>CLI: return diagnostics and status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
src/config/runtime.rs (1)
33-46: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueHandle relative
startpaths ingoverned_root_from.The loop calls
current.pop()to walk ancestors. Ifstartis relative,pop()reduces the path to an emptyPathBufand the loop stops after re-checking the process working directory. True ancestors are never inspected. Current callers pass absolute paths:find_configusesstd::env::current_dir(), andread_hook_inputinsrc/cmd/agent_hooks.rsnormalizescwdto an absolute path. The method ispub(crate), so a future caller can pass a relative path and get silentOk(None).Canonicalize or absolutize
startat entry to make the contract explicit.♻️ Suggested hardening
pub(crate) fn governed_root_from(start: &Path) -> DiagnosticResult<Option<PathBuf>> { - let mut current = start.to_path_buf(); + let mut current = if start.is_absolute() { + start.to_path_buf() + } else { + std::env::current_dir() + .map_err(|err| Diagnostic::io_error("resolve current directory", err, "."))? + .join(start) + }; loop {🤖 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 `@src/config/runtime.rs` around lines 33 - 46, Update governed_root_from to canonicalize or otherwise convert start to an absolute path before initializing current, preserving the existing ancestor traversal and governance checks while ensuring relative inputs inspect their true parent directories.src/cmd/agent_hooks.rs (4)
167-183: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider shell redirect patterns in
commandpayloads.
managed_edit_targetinspectscommandonly throughpatch_path, which matches Codexapply_patchheaders. A ClaudeBashcall that writes a governance artifact directly, for examplecat > gov/work/x.toml <<'EOF'orprintf ... > gov/releases.toml, produces no advisory. The advisory is best-effort, so this is not a defect. If broader coverage is a requirement of RFC-0002:C-AGENT-INTEGRATION, add redirect-target extraction.🤖 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 `@src/cmd/agent_hooks.rs` around lines 167 - 183, Extend managed_edit_target’s command inspection to recognize shell output redirection targets in addition to patch_path results, including commands such as cat or printf writing governance artifacts. Extract each redirect destination and pass it through is_lifecycle_managed_path using the existing project_root and input.cwd context, while preserving the current apply_patch handling and best-effort behavior.
227-243: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGuard
ParentDiragainst popping the root.
normalize_pathpops unconditionally forComponent::ParentDir. For a path such as/../gov/work/a.toml,pop()removes theRootDircomponent. The result is the relative pathgov/work/a.toml. Inis_lifecycle_managed_path,strip_prefixagainst the absoluteproject_rootthen fails and the advisory is skipped. The outcome is fail-safe, so this is not a correctness break for the advisory. Skip the pop when the accumulated path is a root or a prefix to keep the helper reusable.♻️ Suggested change
Component::ParentDir => { - normalized.pop(); + if !matches!( + normalized.components().next_back(), + Some(Component::RootDir | Component::Prefix(_)) | None + ) { + normalized.pop(); + } }🤖 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 `@src/cmd/agent_hooks.rs` around lines 227 - 243, Update normalize_path’s Component::ParentDir handling to avoid popping when the accumulated normalized path is already a filesystem root or prefix; only pop ordinary path components. Preserve normal parent-directory normalization while ensuring paths such as `/../gov/work/a.toml` remain absolute.
274-298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative-path tests for the advisory scope.
The two tests cover the character limit and the recovery case. Three behaviors carry the advisory contract and have no test:
is_lifecycle_managed_pathmust returnfalsefor paths outsidegov/, forgov/config.toml, and forgov/releases.toml/extra.managed_edit_targetmust detect a path from acommandpayload throughpatch_path.bound_contextmust return the input unchanged when the length equalsHOOK_CONTEXT_CHAR_LIMIT.Add these cases to lock the boundaries before the advisory logic changes.
🤖 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 `@src/cmd/agent_hooks.rs` around lines 274 - 298, Add negative-path tests near the existing hook scope tests: verify is_lifecycle_managed_path returns false for paths outside gov/, gov/config.toml, and gov/releases.toml/extra; verify managed_edit_target recognizes a command payload containing patch_path; and verify bound_context returns input unchanged when its length equals HOOK_CONTEXT_CHAR_LIMIT.
211-215: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrevent drift between this artifact list and
Config.The match hard-codes
rfc,adr,work,guard,conformance, andreleases.toml.Configinsrc/config/runtime.rsalready exposesrfc_dir,adr_dir,work_dir,guard_dir,conformance_dir, andreleases_path. Duplicating the list here is justified, because the pre-tool-use path must work without a loadable config. Add a shared constant slice for the directory names so a new artifact kind updates both places at once.🤖 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 `@src/cmd/agent_hooks.rs` around lines 211 - 215, Define a shared constant slice for the artifact directory names and use it in the artifact check around the shown components match instead of hard-coding the `rfc`, `adr`, `work`, `guard`, and `conformance` alternatives. Keep `releases.toml` handling separate, and update `Config`’s corresponding directory-name usage to reference the same constant so additions remain synchronized.src/cli/commands.rs (1)
90-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd RFC traceability comments to the new agent surface. The surrounding code annotates command definitions and lock classifications with
[[RFC-...]]citations, but the new agent code paths carry none. The shared root cause is missing clause references for the agent integration contract.
src/cli/commands.rs#L90-L96: cite[[RFC-0002:C-AGENT-INTEGRATION]]and[[RFC-0002:C-GLOBAL-COMMANDS]]on theAgentcommand variant.src/command_router/plan.rs#L174-L175: cite the clause that keeps user-scoped agent operations outside the gov-root write-lock class, in the same style as theSearcharm.As per coding guidelines: "Implement behavior in Rust only according to normative RFC requirements; cite applicable RFC clauses when implementing invariants."
🤖 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 `@src/cli/commands.rs` around lines 90 - 96, Add RFC traceability comments to the Agent variant in src/cli/commands.rs at lines 90-96, citing [[RFC-0002:C-AGENT-INTEGRATION]] and [[RFC-0002:C-GLOBAL-COMMANDS]]. Also annotate the corresponding agent arm in src/command_router/plan.rs at lines 174-175 with the clause establishing that user-scoped agent operations remain outside the gov-root write-lock class, matching the Search arm’s citation style.Source: Coding guidelines
src/cmd/agent.rs (3)
44-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why
doctor()probes withUpdateinstead ofDoctor.
doctor()callscheck_runtime(*runtime, AgentPluginOperation::Update).check_runtimehas no match arm forAgentPluginOperation::Doctor; onlyInstallandUpdateare handled. UsingUpdategives the broadest probe set, since Claude withUpdatechecks bothInstallandUpdatereadiness. This is not obvious from reading the code alone.Add a short comment at the
doctor()call site explaining thatUpdateis used deliberately to get the most comprehensive readiness check, not becauseDoctormaps toUpdatesemantically.Also applies to: 133-151
🤖 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 `@src/cmd/agent.rs` around lines 44 - 58, Document the deliberate AgentPluginOperation::Update argument at the check_runtime call inside doctor(), noting that Update provides the broadest readiness probe because it checks both Install and Update capabilities, rather than representing Doctor semantically. Add only a short explanatory comment and leave the existing probing behavior unchanged.
233-260: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the hardcoded manifest count with a named constant.
validate_bundled_versionsrequireschecked == 2bundledplugin.jsonmanifests. This number is not tied to any symbolic definition of the supported runtime count.Introduce a named constant (for example,
const EXPECTED_PLUGIN_MANIFEST_COUNT: usize = 2;) with a comment linking it to the two supported runtimes (Codex and Claude). This makes the invariant self-documenting for future maintainers adding a runtime.🤖 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 `@src/cmd/agent.rs` around lines 233 - 260, In validate_bundled_versions, replace the hardcoded checked != 2 comparison with a named EXPECTED_PLUGIN_MANIFEST_COUNT constant set to 2. Define the constant near the function and document that it represents the two supported runtimes, Codex and Claude; use it in the error message as well.
184-200: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOld versioned plugin bundles are never cleaned up.
materialize_pluginwrites plugin assets undermarketplace_root/versions/{CARGO_PKG_VERSION}/.claudeon every install or update, but nothing removes assets from priorgovctlversions. Over repeated upgrades, the user-scopedmarketplace_rootdirectory accumulates stale versioned bundles indefinitely.Consider pruning
versions/*directories that do not match the currentCARGO_PKG_VERSIONafter materializing the current one, or documenting that this is left to manual/OS-level cleanup.🤖 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 `@src/cmd/agent.rs` around lines 184 - 200, Update materialize_plugin to prune stale directories under marketplace_root/versions after successfully materializing the current version, preserving the directory matching CARGO_PKG_VERSION and removing prior versioned bundles. Keep the current manifest and asset-writing behavior unchanged, and propagate filesystem cleanup errors through DiagnosticResult.
🤖 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 `@gov/rfc/RFC-0002/clauses/C-AGENT-INTEGRATION.toml`:
- Around line 16-20: Add a language identifier such as text or sh to the
canonical command fence in gov/rfc/RFC-0002/clauses/C-AGENT-INTEGRATION.toml at
lines 16-20, then regenerate the rendered projection at docs/rfc/RFC-0002.md
lines 1048-1052; do not edit the generated Markdown directly.
- Around line 16-22: Define the hook adapter subcommands, invocation contract,
and runtime-selector exception in
gov/rfc/RFC-0002/clauses/C-AGENT-INTEGRATION.toml (lines 16-22), then
synchronize gov/rfc/RFC-0002/clauses/C-GLOBAL-COMMANDS.toml (lines 207-213) and
regenerate docs/rfc/RFC-0002.md (lines 1046-1054); keep
.claude/hooks/claude.json (lines 8-20) aligned with the corrected grammar.
In `@src/cmd/agent_hooks.rs`:
- Around line 68-71: Update handle_session_start so warnings from
load_work_items_with_warnings are appended to the existing lines and included in
the final bound_context output instead of being returned as Err. Preserve the
project summary and active work context, adding each warning as an extra line;
only escalate warnings if required by RFC-0002 and document that requirement in
a comment.
- Around line 185-194: Update patch_path to trim trailing whitespace from the
path extracted after matching the patch header prefixes, including CRLF carriage
returns and trailing spaces, while preserving the existing prefix detection and
return behavior.
In `@tests/test_agent_plugin.rs`:
- Around line 133-135: Strengthen the dry-run log assertions by first verifying
that the log read from log_path is non-empty, then retain the existing all-lines
--help assertion. Update the test around the log variable so a skipped preflight
cannot satisfy the test through vacuous all() behavior.
---
Nitpick comments:
In `@src/cli/commands.rs`:
- Around line 90-96: Add RFC traceability comments to the Agent variant in
src/cli/commands.rs at lines 90-96, citing [[RFC-0002:C-AGENT-INTEGRATION]] and
[[RFC-0002:C-GLOBAL-COMMANDS]]. Also annotate the corresponding agent arm in
src/command_router/plan.rs at lines 174-175 with the clause establishing that
user-scoped agent operations remain outside the gov-root write-lock class,
matching the Search arm’s citation style.
In `@src/cmd/agent_hooks.rs`:
- Around line 167-183: Extend managed_edit_target’s command inspection to
recognize shell output redirection targets in addition to patch_path results,
including commands such as cat or printf writing governance artifacts. Extract
each redirect destination and pass it through is_lifecycle_managed_path using
the existing project_root and input.cwd context, while preserving the current
apply_patch handling and best-effort behavior.
- Around line 227-243: Update normalize_path’s Component::ParentDir handling to
avoid popping when the accumulated normalized path is already a filesystem root
or prefix; only pop ordinary path components. Preserve normal parent-directory
normalization while ensuring paths such as `/../gov/work/a.toml` remain
absolute.
- Around line 274-298: Add negative-path tests near the existing hook scope
tests: verify is_lifecycle_managed_path returns false for paths outside gov/,
gov/config.toml, and gov/releases.toml/extra; verify managed_edit_target
recognizes a command payload containing patch_path; and verify bound_context
returns input unchanged when its length equals HOOK_CONTEXT_CHAR_LIMIT.
- Around line 211-215: Define a shared constant slice for the artifact directory
names and use it in the artifact check around the shown components match instead
of hard-coding the `rfc`, `adr`, `work`, `guard`, and `conformance`
alternatives. Keep `releases.toml` handling separate, and update `Config`’s
corresponding directory-name usage to reference the same constant so additions
remain synchronized.
In `@src/cmd/agent.rs`:
- Around line 44-58: Document the deliberate AgentPluginOperation::Update
argument at the check_runtime call inside doctor(), noting that Update provides
the broadest readiness probe because it checks both Install and Update
capabilities, rather than representing Doctor semantically. Add only a short
explanatory comment and leave the existing probing behavior unchanged.
- Around line 233-260: In validate_bundled_versions, replace the hardcoded
checked != 2 comparison with a named EXPECTED_PLUGIN_MANIFEST_COUNT constant set
to 2. Define the constant near the function and document that it represents the
two supported runtimes, Codex and Claude; use it in the error message as well.
- Around line 184-200: Update materialize_plugin to prune stale directories
under marketplace_root/versions after successfully materializing the current
version, preserving the directory matching CARGO_PKG_VERSION and removing prior
versioned bundles. Keep the current manifest and asset-writing behavior
unchanged, and propagate filesystem cleanup errors through DiagnosticResult.
In `@src/config/runtime.rs`:
- Around line 33-46: Update governed_root_from to canonicalize or otherwise
convert start to an absolute path before initializing current, preserving the
existing ancestor traversal and governance checks while ensuring relative inputs
inspect their true parent directories.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 822346ae-234f-4f4c-9d2e-47cae423428d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
.claude/.claude-plugin/plugin.json.claude/.codex-plugin/plugin.json.claude/hooks/claude.json.claude/hooks/codex.json.claude/skills/discuss/SKILL.md.claude/skills/gov/SKILL.md.claude/skills/init/SKILL.md.claude/skills/spec/SKILL.mdAGENTS.mdCHANGELOG.mdCargo.tomlJustfileREADME.mdbuild.rsbuild_support/agent_templates.rsdocs/guide/validation.mddocs/rfc/RFC-0002.mdgov/adr/ADR-0033-distribute-govctl-agent-integration-as-claude-code-plugin.tomlgov/adr/ADR-0061-use-one-agent-integration-command-with-client-specific-projections.tomlgov/rfc/RFC-0002/clauses/C-AGENT-INTEGRATION.tomlgov/rfc/RFC-0002/clauses/C-GLOBAL-COMMANDS.tomlgov/rfc/RFC-0002/rfc.tomlgov/work/2026-07-31-harden-agent-plugin-installation-across-codex-and-claude.tomlsrc/cli/commands.rssrc/cli/help.rssrc/cli/mod.rssrc/cmd/agent.rssrc/cmd/agent_hooks.rssrc/cmd/describe/catalog.rssrc/cmd/mod.rssrc/cmd/new/skills.rssrc/command_router/execute/builtin.rssrc/command_router/parsed.rssrc/command_router/plan.rssrc/config/mod.rssrc/config/runtime.rssrc/diagnostic/code/metadata.rssrc/diagnostic/code/mod.rssrc/main.rstests/test_agent_dir.rstests/test_agent_plugin.rstests/test_describe.rs
| ``` | ||
| govctl agent doctor <codex|claude|all> | ||
| govctl agent install <codex|claude|all> | ||
| govctl agent update <codex|claude|all> | ||
| ``` | ||
|
|
||
| The runtime selector is required. `all` selects Codex and Claude. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define the hook adapter command in the normative command grammar.
The Claude manifest invokes govctl agent hook session-start and govctl agent hook pre-tool-use. The normative grammar lists only doctor, install, and update, and requires a runtime selector. The installed hook invocation is therefore outside the governing command contract.
gov/rfc/RFC-0002/clauses/C-AGENT-INTEGRATION.toml#L16-L22: Define the hook adapter subcommands, their invocation contract, and their runtime-selector exception, or replace the hook invocation with a listed command.gov/rfc/RFC-0002/clauses/C-GLOBAL-COMMANDS.toml#L207-L213: Synchronize the global command grammar with the canonical clause.docs/rfc/RFC-0002.md#L1046-L1054: Regenerate this rendered projection from the corrected canonical clause..claude/hooks/claude.json#L8-L20: Keep these invocations aligned with the corrected normative grammar.
As per coding guidelines, gov/rfc/**/*.toml RFCs are normative authority and code must not conflict with normative RFC content.
📍 Affects 4 files
gov/rfc/RFC-0002/clauses/C-AGENT-INTEGRATION.toml#L16-L22(this comment)gov/rfc/RFC-0002/clauses/C-GLOBAL-COMMANDS.toml#L207-L213docs/rfc/RFC-0002.md#L1046-L1054.claude/hooks/claude.json#L8-L20
🤖 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 `@gov/rfc/RFC-0002/clauses/C-AGENT-INTEGRATION.toml` around lines 16 - 22,
Define the hook adapter subcommands, invocation contract, and runtime-selector
exception in gov/rfc/RFC-0002/clauses/C-AGENT-INTEGRATION.toml (lines 16-22),
then synchronize gov/rfc/RFC-0002/clauses/C-GLOBAL-COMMANDS.toml (lines 207-213)
and regenerate docs/rfc/RFC-0002.md (lines 1046-1054); keep
.claude/hooks/claude.json (lines 8-20) aligned with the corrected grammar.
Source: Coding guidelines
1 participant
Summary
govctl agent doctor|install|updateworkflows for Codex and Claude native pluginsGovernance
Validation
GUARD-GOVCTL-CHECKGUARD-CARGO-TESTcargo clippy --all-targets --all-features -- -D warningsSummary by CodeRabbit
New Features
govctl agent doctor,install, andupdatecommands for Claude and Codex.Documentation
Bug Fixes