fix(review): declare each runtime's own identity in the negotiated route - #2244
fix(review): declare each runtime's own identity in the negotiated route#2244aparragithub wants to merge 4 commits into
Conversation
The shared review-ledger contract hardcoded `--agent claude-code` in the
negotiated STATUS route. That contract text is spliced into every adapter's
generated orchestrator and sdd-apply assets, so a non-Claude runtime was
instructed to declare Claude Code's runtime identity and would pass the
preflight transport gate it must fail closed against.
Render the existing {{GENTLE_AI_RUNTIME_AGENT_ID}} placeholder for every
generated asset rather than only the orchestrator: commands and embedded
subagents now render with their adapter's agent. Substitution runs after the
contract sections are spliced in, because those sections carry the
placeholder themselves.
Refs Gentleman-Programming#2242
A negotiated route carrying zero or more than one `--agent` token was reported as `immutable_review_transport_unsupported`, which named a capability the facade never evaluated and left the caller a terminal stop with nothing to run. An absent or duplicated token is a malformed or stale route, not a claim about any runtime's transport. Return `runtime_identity_unresolved` with `next_action: correct_request` instead. The refusal stays retry-safe and its additive `cause` names `gentle-ai sync` as the way to regenerate the route. Refs Gentleman-Programming#2242
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change replaces hard-coded Claude Code identities with runtime-specific agent IDs in bounded-review assets. Negotiated STATUS and START now distinguish unresolved route identities from unsupported transport and provide ChangesRuntime identity handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ReviewStatusOrStart
participant RoutePreflight
participant gentleAiSync
ReviewStatusOrStart->>RoutePreflight: validate generated --agent identity
RoutePreflight-->>ReviewStatusOrStart: return runtime_identity_unresolved
ReviewStatusOrStart->>gentleAiSync: regenerate route
gentleAiSync-->>ReviewStatusOrStart: return adapter-specific route
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/cli/review_facade.go`:
- Around line 763-766: Classify empty or whitespace-only --agent values as
unresolved runtime identities before transport evaluation. In
internal/cli/review_facade.go lines 763-766 and 1480-1482, validate the trimmed
runtimeAgent and return reviewPreflightRefusal with
reviewRuntimeIdentityUnresolvedReason when it is empty; in
internal/cli/review_transport_capability_test.go lines 114-160, add STATUS and
START coverage for both --agent "" and --agent= inputs.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 12d44f26-e6a1-49bf-ac33-7a9c3a640cdc
⛔ Files ignored due to path filters (12)
testdata/golden/combined-windsurf-global-rules.goldenis excluded by!testdata/**testdata/golden/sdd-antigravity-rulesmd.goldenis excluded by!testdata/**testdata/golden/sdd-codex-agentsmd-lowcost.goldenis excluded by!testdata/**testdata/golden/sdd-codex-agentsmd-powerful.goldenis excluded by!testdata/**testdata/golden/sdd-codex-agentsmd.goldenis excluded by!testdata/**testdata/golden/sdd-cursor-rules.goldenis excluded by!testdata/**testdata/golden/sdd-gemini-geminimd.goldenis excluded by!testdata/**testdata/golden/sdd-kiro-instructions.goldenis excluded by!testdata/**testdata/golden/sdd-opencode-cmd-sdd-apply.goldenis excluded by!testdata/**testdata/golden/sdd-opencode-multi-settings.goldenis excluded by!testdata/**testdata/golden/sdd-vscode-instructions.goldenis excluded by!testdata/**testdata/golden/sdd-windsurf-global-rules.goldenis excluded by!testdata/**
📒 Files selected for processing (13)
docs/review-integration.mdinternal/assets/skills/_shared/review-ledger-contract.mdinternal/cli/review_facade.gointernal/cli/review_transport_capability.gointernal/cli/review_transport_capability_test.gointernal/cli/sync_test.gointernal/components/sdd/bounded_review_contract_test.gointernal/components/sdd/boundedreview.gointernal/components/sdd/inject.gointernal/components/sdd/inject_test.gointernal/components/sdd/prompts_test.gointernal/components/sdd/review_ledger_contract_test.gointernal/components/sdd/reviewer_envelope_guard_test.go
`--agent ""` and `--agent=` satisfy the cardinality check with a single token but declare no identity. Both flows fell through to the transport lookup and were reported as `immutable_review_transport_unsupported`, which asserts something about a runtime that was never named. Reject an empty trimmed identity alongside the cardinality check in negotiated STATUS and START, so the caller gets `runtime_identity_unresolved` and its runnable exit. Well-formed tokens naming an unknown runtime keep the unsupported-transport classification. Refs Gentleman-Programming#2242
|
Thanks — applied in
Both negotiated STATUS and START now reject an empty trimmed identity alongside the cardinality check, and coverage was added for Verified the new cases fail without the change and pass with it; the three pre-existing cases were unaffected. Deliberately unchanged: well-formed tokens naming an unknown runtime ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/cli/review_transport_capability_test.go`:
- Around line 141-162: Add table-driven START cases for duplicate runtime
identities and whitespace-only runtime identities in the review transport
capability tests, alongside the existing status cases. Route them through
runReviewFacadeStart and assert the established unresolved-identity failure
envelope, covering both duplicate and whitespace-only validation paths without
changing status coverage.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 53ff9ebc-bc1c-4961-9f33-10766b389b9a
📒 Files selected for processing (2)
internal/cli/review_facade.gointernal/cli/review_transport_capability_test.go
START resolves the runtime identity in its own preflight branch, but the duplicate and whitespace-only shapes were exercised only through STATUS. Refs Gentleman-Programming#2242
|
Closing this in favour of a root-level fix. This PR is correct as far as it goes: each runtime's generated route now declares its own identity instead of borrowing The underlying problem is that immutable receipt-review transport exists for exactly one runtime. Per #2076 / #2221 the resolution for OpenCode was to fail closed, because OpenCode 1.18.10 cannot bind candidate-specific shell permissions to a child session. That left OpenCode's lens agents with no shell — which is the hard requirement already satisfied — but they were never given the prompt-carried evidence path Claude Code has. Since the Nothing here is lost: the branch remains at |
1 participant
🔗 Linked Issue
Closes #2242
🏷️ PR Type
type:bug— Bug fix (non-breaking change that fixes an issue)type:feature— New feature (non-breaking change that adds functionality)type:docs— Documentation onlytype:refactor— Code refactoring (no functional changes)type:chore— Build, CI, or tooling changestype:breaking-change— Breaking change (fix or feature that changes existing behavior)📝 Summary
--agent claude-codein the negotiated STATUS route. That text is spliced into every adapter's generated orchestrator andsdd-applyassets, so a non-Claude runtime was instructed to declare Claude Code's runtime identity — and would pass the preflight transport gate it must fail closed against. Each runtime's generated route now declares its own identity.--agenttoken was reported asimmutable_review_transport_unsupported, naming a capability the facade never evaluated and leaving the caller a terminal stop with nothing to run. That is the envelope reported in fix(opencode): negotiated review status misclassifies supported runtime transport #2242, produced by installed assets predating the gating. It is now a distinctruntime_identity_unresolvedoutcome with a runnable exit.📂 Changes
internal/assets/skills/_shared/review-ledger-contract.md{{GENTLE_AI_RUNTIME_AGENT_ID}}placeholder instead of a literalclaude-code, in both the Route section and the authority-first procedureinternal/components/sdd/boundedreview.gorenderBoundedReviewAssettakes the target agent and resolves the runtime placeholder for every generated asset, not only the orchestrator; substitution runs after the contract sections are spliced in, because those sections carry the placeholderinternal/components/sdd/inject.goadapter.Agent()internal/cli/review_transport_capability.goreviewRuntimeIdentityUnresolvedReason(runtime_identity_unresolved,next_action: correct_request)internal/cli/review_facade.go--agentwith the new reason; the cause namesgentle-ai syncas the recoverydocs/review-integration.mdtestdata/golden/*(12 files)Test baselines that moved
TestKilocodeReviewSettingsMatchCurrentMainBaselineSHATestOpenCodeRenderedReviewProtocolCostpins (+38 chars each tier)claude-code. Both tiers keep their >15% headroom below the ceilings🧪 Test Plan
Unit Tests — ✅ pass, 63 packages, 0 failures
go test ./...Go Format — ✅ pass
E2E Tests —⚠️ not run (Docker not available in the authoring environment). Left to CI.
Benchmark Validation —⚠️ not run. This change touches the review lifecycle, so benchmark validation does apply and should not be waived. Flagging it explicitly rather than claiming coverage that was not produced.
go test ./...)go run ./internal/gofmtcheck)cd e2e && ./docker-test.sh)Manual verification: built the binary and confirmed the generated OpenCode assets now emit
--agent opencode, and that negotiated STATUS without an--agenttoken returnsruntime_identity_unresolvedwithretry_safe: trueand a cause naminggentle-ai sync.✅ Contributor Checklist
status:approved— fix(opencode): negotiated review status misclassifies supported runtime transport #2242 is not yet approved; see the note abovetype:*label to this PRgo test ./...)go run ./internal/gofmtcheck)cd e2e && ./docker-test.sh) — not run locallyCo-Authored-Bytrailers💬 Notes for Reviewers
The security-relevant direction is the inverse of what #2242's title suggests. #2242 reads as "supported OpenCode is misclassified as unsupported". It is not: OpenCode's transport is deliberately disabled (
ee31bc2e, pending #2076). The real defect is that the product handed unsupported runtimes Claude Code's identity, which would let them through the gate #2207 introduced. This PR closes that, and deliberately does not re-enable any transport.A second surface is still open and is tracked separately in #2243:
gentle-ai review capabilitiesadvertisesbootstrap.commandwith a compile-time--agent claude-codefor every caller, andcapabilitiesis exempt from the negotiated-contract argument requirement. It is left out here becausebootstrap.commandis pinned as a JSONconstin thecapabilities-v2.1andv2.2schemas with byte-pinned fixtures, so resolving it is a contract decision rather than a string edit. It is a prerequisite for #2076 and #2208.Guard population: this change adds no security/integrity/admission/repair/governance guard and does not alter any guard's legitimate input population;
.guard-population-baseline.txtis unchanged. Tworefusal:by-design world-actionmarkers were removed from the two reclassified sites, because those refusals now name a runnable exit — the refusal-resolution ratchet flags the combination as contradictory, and it is right to.Summary by CodeRabbit
New Features
gentle-ai sync.Bug Fixes
Documentation