test: guard agent surface parity - #361
Conversation
Require a documented agent-surface decision for every catalog feature. Verify the CLI and MCP resource registries and document the contributor wiring step.
|
Warning Review limit reachedNext included review available in 4 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📓 Path-based instructions (5)Check that tests cover API errors, mapping edge cases, and navigation state transitions, not only happy paths.⚙️ CodeRabbit configuration file Files:
Documentation must match implemented behavior.⚙️ CodeRabbit configuration file Files:
For Go reviews, look beyond compilation and prioritize nil pointer risks, context propagation, AWS SDK pagination, error wrapping, deterministic sorting, and stable table/detail rendering.⚙️ CodeRabbit configuration file Files:
For every feature added to `domain.Catalog()`, update `internal/mcp/agent_surface_test.go`: map it to both a `unic resources` command and MCP tool, or add a one-line `agentSurfaceExempt` reason.📄 CodeRabbit inference engine (AGENTS.md) Files:
For every feature added to `domain.Catalog()`, update `internal/mcp/agent_surface_test.go`: map it to both a `unic resources` command and MCP tool, or add a one-line `agentSurfaceExempt` reason📄 CodeRabbit inference engine (CLAUDE.md) Files:
🔇 Additional comments (5)
WalkthroughThe pull request documents an Agent Surface Parity requirement and adds a test that validates catalog features against resource commands and MCP tools, with explicit exemptions for unsupported features. ChangesAgent surface parity
Priority: ⚪ Pending latest changes Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Other Merge Risk: ⚪ Minimal · up to The new guard and guidance consistently require each catalog feature to have both agent surfaces or a documented exemption, with no concrete regression identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (4 skipped: 4 unsupported.) ✨ 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 |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR successfully establishes agent surface parity guardrails by adding documentation and a test that enforces explicit decisions for every catalog feature. The implementation is well-structured: the test validates bidirectional mappings between features, CLI commands, and MCP tools, ensuring no catalog feature is accidentally excluded from agent surfaces without a documented reason. The documentation updates in contributor guides and architecture docs properly communicate the new workflow step. No defects found that would block merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
YoungJinJung
left a comment
YoungJinJung
left a comment
There was a problem hiding this comment.
Reviewed head: 9eb2cde
Findings
-
Medium — Validate the CLI contract, not only the command name (
internal/mcp/agent_surface_test.go:65-68; current counterexample atinternal/cli/resources.go:65-116)The new guard treats every direct child of
unic resourcesas a valid agent-facing command based only on its name. At this head, that makesbackup-vaultspass even thoughunic capabilities --jsonandunic schema resources backup-vaults --jsonadvertise it asread_only: falseandoutput_version: "unversioned"because the command has no discovery annotations. Agents relying on the advertised contract can therefore reject or mishandle a surface that this parity test declares present. Please require mapped commands to carry the read-only and v1 output annotations (and retain a JSON-output flag), and annotatebackup-vaultsaccordingly.
Require mapped resource commands to advertise read-only v1 JSON output. Annotate the existing backup-vaults command with its documented contract.
|
Addressed in 5f047aa: the parity guard now requires every mapped CLI resource command to advertise read-only and v1 output and to provide a JSON flag. |
|
/q review |
There was a problem hiding this comment.
This PR successfully implements automated enforcement of agent surface parity through test-driven governance. The implementation correctly validates that every catalog feature maps to both CLI commands and MCP tools, or carries an explicit exemption. All documentation updates accurately reflect the new requirement. The test logic is sound and will prevent drift between the catalog and agent surfaces. No blocking issues identified.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
YoungJinJung
left a comment
YoungJinJung
left a comment
There was a problem hiding this comment.
Reviewed head: 5f047aa
Findings
-
Medium — Verify that each MCP tool corresponds to its mapped CLI resource (
internal/mcp/agent_surface_test.go:80-85,122-126)The guard records resource tools only by name, so the command and tool in an
agentSurfaceare validated independently. For example, mappingFeatureRDSBrowsertords-instancesandlist_ec2_instanceswould still pass: both names are registered, but the MCP side exposes EC2 rather than RDS. That false positive defeats the parity guarantee for the exact copy/paste or wiring mistake this test is intended to catch. Keep the registered tool metadata by name and require its output contract to match the paired command (for example,unic.resources.<command>.v1); also retain the existing registration check.
|
Addressed the latest review finding in 68c002b: the parity test now retains each MCP resource tool output contract and requires it to match unic.resources..v1. Validation passed: focused MCP parity test, make test, and make build. |
YoungJinJung
left a comment
YoungJinJung
left a comment
There was a problem hiding this comment.
Reviewed head: 68c002b
Findings
-
Medium — Reject duplicate feature-to-surface mappings (
internal/mcp/agent_surface_test.go:128-129)mappedCommandsandmappedToolsare used as sets, so assigning an already-mapped command or tool silently succeeds. For example, a new catalog feature can copy the existingrds-instances/list_rds_instancesmapping; all registration and contract checks pass, and the reverse-coverage loops still see those surfaces as mapped, even though the new feature added no agent surface. That permits the exact silent coverage gap this guard is intended to prevent. When recording each mapping, fail if either command or tool already belongs to another feature (tracking the owning feature will also make the diagnostic actionable), unless shared surfaces are explicitly supported and documented.
- Track the catalog feature owning each CLI command and MCP tool.\n- Fail parity validation when a surface is reused by another feature.
|
Addressed in 8e330c0: the parity guard now tracks the catalog feature owning each mapped CLI command and MCP tool, and reports duplicate reuse with both feature names. Validation passed: focused MCP parity test, make test, and make build. |
YoungJinJung
left a comment
YoungJinJung
left a comment
There was a problem hiding this comment.
Reviewed head: 8e330c0
Findings
-
Medium — Reject duplicate names in the registered surfaces (
internal/mcp/agent_surface_test.go:67-79)Both registry scans assign directly into maps keyed by command/tool name, so a second registration with the same name silently overwrites the first. Cobra's
AddCommandaccepts duplicate child names and dispatches the first match, whiletools/listpublishes thetoolsslice verbatim; therefore a copy/paste registration can make the CLI ambiguous or expose duplicate MCP definitions while this parity guard and both reverse-coverage loops still pass. Please fail while buildingregisteredCommandswhen a command name is already present, and check every MCP tool name for uniqueness before filtering resource output contracts.
YoungJinJung
left a comment
YoungJinJung
left a comment
There was a problem hiding this comment.
Reviewed head: 8e330c0
Findings
- Medium — Validate the MCP read-only contract too (
internal/mcp/agent_surface_test.go:80-84,119-126)
The guard now verifies that each mapped CLI command advertises read-only v1 JSON output, but registeredResourceTools retains only OutputContract and ignores registered.Annotations.ReadOnlyHint. A resource tool with the correct name and unic.resources.<command>.v1 contract would therefore pass even if MCP advertises it as potentially mutating, which can cause safety-aware clients to reject or mishandle the surface. Retain the mapped tool annotation and require ReadOnlyHint to be true alongside the existing output-contract check.
Reject duplicate CLI and MCP registrations and require mapped MCP resource tools to advertise read-only.
|
Addressed both current-head review findings in 8c9cff9: the parity guard now rejects duplicate direct resource-command names and duplicate MCP tool names before filtering, and mapped MCP resource tools retain their metadata and must advertise read-only alongside the paired output contract. Validation passed: focused MCP parity test, make test, and make build. /q review |
YoungJinJung
left a comment
YoungJinJung
left a comment
There was a problem hiding this comment.
Reviewed head: 8c9cff9
Findings
-
Medium — Verify that mapped MCP tools are actually callable and dispatch to the paired command (
internal/mcp/agent_surface_test.go:132-141; runtime dispatch atinternal/mcp/server.go:290-297,348-460)The parity check now proves that a mapped name exists in
toolsand that its metadata names the expected output contract, buttools/calluses the separatetoolArgsswitch. A new resource can therefore add a correctly named tool entry, mapping, read-only hint, and output contract while omitting itstoolArgscase (or routing that case to a differentresourcessubcommand); this test still passes, yet clients receive-32602 unknown toolor data from the wrong resource. Extend the mapping/test with minimal valid arguments for each mapped tool, calltoolArgs, and assert that it succeeds and returns a command beginning withresources,surface.command.
- Exercise every mapped MCP tool through the runtime dispatcher. - Require dispatch to target the paired resources command.
|
Addressed in beeeb98: the parity guard now calls every mapped MCP tool through the runtime dispatcher with minimal valid arguments and requires it to target the paired resources command. Validation passed: focused MCP parity test, make test, and make build. /q review |
YoungJinJung
left a comment
YoungJinJung
left a comment
There was a problem hiding this comment.
Reviewed head: beeeb98
Findings
-
Medium — Validate the complete generated CLI invocation (
internal/mcp/agent_surface_test.go:155-159; runtime path atinternal/mcp/server.go:290-308)The new dispatch check accepts any argument vector whose first two entries are
resourcesand the mapped command. AtoolArgscase can therefore keep that prefix while emitting a renamed or unknown flag, adding an invalid positional argument, or omitting--json; this parity test remains green, but the realExecuteAutomationpath either fails Cobra parsing or returns human output thatcallToolrejects as invalid JSON. This is a different failure mode from the earlier missing/wrong-command finding. Please parse and validate the full returned argument vector against a fresh CLI command tree (without running the AWS loader), including required flags/positional arguments, and explicitly require JSON output before considering the MCP surface callable.
|
Reviewed and verified rather than read — this goes further than what I sketched in #360, in the right directions. Verified the guard can actually fail. A parity test that cannot fail is decoration, so I added a Accounting is exact: 38 catalog features, 38 entries across the two maps (7 mapped + 31 exempt). The parts I did not propose and think are the best of this PR:
Two non-blocking observations for the follow-up work, not this PR: 1. Inspector is outside the guard entirely. 2. Four exemptions lean on "mutations exist", but
Both are notes for later. This is good as-is — merging. |
|
Addressed the current-head review finding in follow-up PR #362 because #361 merged while the fix was being validated. The guard now resolves each generated invocation through a fresh CLI tree, parses all flags, validates positional/required/group constraints, and requires --json without running AWS loaders. Validation passed: focused MCP parity test, make test, and make build. |
Adds `unic inspect --json` and the `run_security_inspector` MCP tool, the first fill from #360's coverage list. Ten rule packs collapse into one call, which is the curated view a raw AWS API bridge cannot reproduce. Returns the shared v1 envelope; `data` carries scanned_at, scanner_count, finding_count, severity_counts, and findings. Per-scanner failures land in `warnings` instead of being dropped, so a partially denied scan is never reported as a clean one. Findings serialize as an empty array rather than null so agents need not branch on both. `--checklist` is a root persistent flag, so it is inherited here and advertised by `unic schema inspect`. Running the security packs while silently ignoring it would hand back the wrong report, so it is rejected explicitly until the checklist output contract is defined. Inspector is a workflow rather than a catalog feature, so it sits outside `unic resources` and outside the catalog parity test added in #361; the docs now say so. Refs #360 Claude-Session: https://claude.ai/code/session_01P2bi5Xga4wU5hDPgNPd4mv Co-authored-by: YoungJinJung <18644538+YoungJinJung@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Summary
domain.Catalog()feature to map to both a registeredunic resourcescommand and MCP tool, or carry a one-line exemptioninspectexposure as follow-up work in feat: stop TUI browsers from silently skipping the agent-facing surface #360Related Issues
Part of #360
Validation
make testmake buildgo test ./internal/mcp -run TestCatalogFeaturesHaveAgentSurfaceDecision -count=1Checklist
docs/branch-naming-harness.mdSummary by CodeRabbit
Documentation
Tests