fix(review): bind capture to canonical repository#1598
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 review integration now emits canonical repository paths in negotiated lens bindings, propagates them through status and capture transitions, and uses them for plugin preflight and artifact capture. Schemas, fixtures, orchestration instructions, and tests were updated. ChangesRepository-aware review execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReviewStart
participant ReviewFacade
participant ReviewResultArtifacts
participant NextTransition
ReviewStart->>ReviewFacade: create lens bindings with repository root
ReviewFacade->>ReviewResultArtifacts: provide repository binding
ReviewResultArtifacts->>ReviewResultArtifacts: validate binding and select capture CWD
ReviewFacade->>NextTransition: pass repository context
NextTransition->>ReviewResultArtifacts: emit repository capture argument
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
Maintainer action required: please add exactly one label, ype:bug. The contributor account cannot apply upstream labels, and the PR validation is currently failing only that label gate. Issue #1484 already has status:approved. |
|
The repository routing works when the binding comes from the negotiated status transition, but I found two blockers before this can fully close #1484:
The good news is that the status-derived path succeeds from process cwd |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/cli/review_start_contract.go (1)
49-55: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPropagate
target_identityfor ordinary and staged STARTs.
ReviewFacadeStartResultalready derives the snapshot identity, but this constructor only copies it inside the overlay branch. Consequently, negotiated workspace/staged START output has repository-bearing bindings without the target identity required to construct the reviewer task.Copy
legacy.TargetIdentityunconditionally, updateValidateto accept/require it for these modes, and update the schema and golden fixture.Proposed fix
result := ReviewIntegrationStartResult{ ... - SelectedLenses: append([]string{}, legacy.SelectedLenses...), LensBindings: append([]ReviewFacadeLensBinding{}, legacy.LensBindings...), + SelectedLenses: append([]string{}, legacy.SelectedLenses...), LensBindings: append([]ReviewFacadeLensBinding{}, legacy.LensBindings...), + TargetIdentity: legacy.TargetIdentity, ... }🤖 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 `@internal/cli/review_start_contract.go` around lines 49 - 55, Propagate legacy.TargetIdentity unconditionally when constructing ReviewIntegrationStartResult, not only in the overlay path, so ordinary and staged STARTs retain the derived snapshot identity. Update ReviewIntegrationStartResult.Validate and its schema to require/accept target_identity for those modes, then refresh the affected golden fixture to include the field.internal/cli/review_start_contract_test.go (1)
53-63: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the repository on the capture argument structurally.
bytes.Containscan pass if the repository appears in another status field. Decode the transition and locate thereviewer_resultargument whose name isrepository, then assert its exact value.🤖 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 `@internal/cli/review_start_contract_test.go` around lines 53 - 63, Replace the bytes.Contains check on statusOutput with structural decoding of the transition returned by RunReview. Locate the reviewer_result argument named repository and assert that its exact value equals repo, failing if the argument is missing or has a different value; keep the existing status command flow unchanged.contracts/review-integration/v1/schemas/start.schema.json (1)
34-34: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep the v1 wire contract additive or version the new required fields.
The START and reviewer-result changes make repository-bearing data mandatory in an existing
/v1contract. Strict consumers may reject the new top-level field, validator behavior, or capture argument. Use additive optional fields with capability negotiation, or publish and negotiate a new contract version.
contracts/review-integration/v1/schemas/start.schema.json#L34-L34: do not requirelens_bindingsin the existing v1 schema unless the contract is versioned.internal/cli/review_start_contract.go#L110-L112: keep validation compatible with legacy v1 responses or gate the requirement by negotiated capability.contracts/review-integration/v1/fixtures/status.fixture.json#L121-L124: version or negotiate the addedrepositorycapture argument.🤖 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 `@contracts/review-integration/v1/schemas/start.schema.json` at line 34, Keep the existing v1 wire contract backward compatible: in contracts/review-integration/v1/schemas/start.schema.json at lines 34-34, make lens_bindings optional unless introducing a new negotiated contract version; in internal/cli/review_start_contract.go at lines 110-112, accept legacy v1 responses or enforce the requirement only after capability negotiation; and in contracts/review-integration/v1/fixtures/status.fixture.json at lines 121-124, version or negotiate the added repository capture argument.
🤖 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.
Outside diff comments:
In `@contracts/review-integration/v1/schemas/start.schema.json`:
- Line 34: Keep the existing v1 wire contract backward compatible: in
contracts/review-integration/v1/schemas/start.schema.json at lines 34-34, make
lens_bindings optional unless introducing a new negotiated contract version; in
internal/cli/review_start_contract.go at lines 110-112, accept legacy v1
responses or enforce the requirement only after capability negotiation; and in
contracts/review-integration/v1/fixtures/status.fixture.json at lines 121-124,
version or negotiate the added repository capture argument.
In `@internal/cli/review_start_contract_test.go`:
- Around line 53-63: Replace the bytes.Contains check on statusOutput with
structural decoding of the transition returned by RunReview. Locate the
reviewer_result argument named repository and assert that its exact value equals
repo, failing if the argument is missing or has a different value; keep the
existing status command flow unchanged.
In `@internal/cli/review_start_contract.go`:
- Around line 49-55: Propagate legacy.TargetIdentity unconditionally when
constructing ReviewIntegrationStartResult, not only in the overlay path, so
ordinary and staged STARTs retain the derived snapshot identity. Update
ReviewIntegrationStartResult.Validate and its schema to require/accept
target_identity for those modes, then refresh the affected golden fixture to
include the field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eb0059a9-ecb6-4e1a-bdc9-6d4b858126a0
📒 Files selected for processing (7)
contracts/review-integration/v1/fixtures/start.fixture.jsoncontracts/review-integration/v1/fixtures/status.fixture.jsoncontracts/review-integration/v1/schemas/start.schema.jsoninternal/cli/review_facade.gointernal/cli/review_start_contract.gointernal/cli/review_start_contract_test.gointernal/reviewtransaction/compact_store.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/review_facade.go (1)
49-60: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftInclude lineage and target in each negotiated START binding.
facadeLensBindingsemits onlylens,order, andrepository; it still lacks immutablelineageandtarget(target_identity). The contract ininternal/assets/skills/_shared/review-ledger-contract.mdLine 9 requires a complete binding, so ordinary/staged START cannot construct reviewer tasks until a later status transition supplies the target. Add those fields to the START binding or emit a complete binding object before launch.🤖 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 `@internal/cli/review_facade.go` around lines 49 - 60, The facadeLensBindings function must emit complete START bindings by including immutable lineage and target_identity fields alongside lens, order, and repository. Update ReviewFacadeLensBinding and its construction so ordinary and staged START outputs carry these values before launch, using the negotiated repository context and existing contract-defined symbols.
🤖 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/assets/skills/_shared/review-ledger-contract.md`:
- Line 9: Update the review-ledger contract to make the repository binding field
additive and optional for v1. Document that legacy four-field START bindings
remain valid and that capture uses the process CWD when repository is absent; do
not require repository universally unless explicit version negotiation is added.
---
Outside diff comments:
In `@internal/cli/review_facade.go`:
- Around line 49-60: The facadeLensBindings function must emit complete START
bindings by including immutable lineage and target_identity fields alongside
lens, order, and repository. Update ReviewFacadeLensBinding and its construction
so ordinary and staged START outputs carry these values before launch, using the
negotiated repository context and existing contract-defined symbols.
🪄 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: 108a14fa-8965-45fe-aed6-c4cf232ced66
⛔ Files ignored due to path filters (13)
testdata/golden/combined-claude-claudemd.goldenis excluded by!testdata/**testdata/golden/combined-windsurf-global-rules.goldenis excluded by!testdata/**testdata/golden/sdd-antigravity-rulesmd.goldenis excluded by!testdata/**testdata/golden/sdd-claude-claudemd.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-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 (3)
internal/assets/skills/_shared/review-ledger-contract.mdinternal/cli/review_facade.gointernal/components/sdd/review_ledger_contract_test.go
3 participants
🔗 Linked Issue
Closes #1484
🏷️ 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
📂 Changes
internal/cli/review_*internal/assets/opencode/plugins/review-result-artifacts.tslens_bindingswith repository context.🧪 Test Plan
R1-001andR3-001post-apply,pre-commit,pre-push, andpre-prgates allow the same content-bound receipt✅ Contributor Checklist
type:*label is applied (pending maintainer permission)Co-Authored-Bytrailers💬 Notes for Reviewers
This is an alternative, complete implementation to the locator-based chain currently starting at #1594. It carries the canonical repository directly in negotiated task bindings while retaining the legacy four-field contract. The two approaches should be reconciled rather than merged independently.
Bounded review lineage:
review-1d9a6aec04e15459.Summary by CodeRabbit
review startnow always returnslens_bindingsincluding a canonical per-lensrepositoryvalue (required in the response).review statusnext-transition capture now carries repository context through to follow-up capture and finalize steps.--cwd <repository>and clarified guidance for version-skew/preflight scenarios.