fix(review): replace eager diff transport with frozen git trees - #1970
Merged
Conversation
Alan-TheGentleman
added
type:bug
|
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 (3)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughAdds review-integration v2 with native Git tree references, ordered manifests, schema-specific artifact subjects, contract-aware CLI envelopes, provider-owned reviewer capture validation, and updated reviewer tooling, documentation, fixtures, and tests. ChangesNative Git contract and schemas
Immutable review context and artifact identity
CLI negotiation and lifecycle routing
Provider-owned reviewer capture
Validation coverage and Git isolation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the negotiated review integration to use provider-owned frozen Git tree identities (base_tree/candidate_tree) plus an ordered changed-path manifest instead of eagerly transporting full candidate diffs, and introduces review-integration/v2 schemas/fixtures alongside compatibility for legacy v1.
Changes:
- Replace candidate diff transport with frozen Git tree references and extend artifact subjects/admission to bind against those immutable trees (with v1 compatibility paths preserved).
- Add review-integration/v2 schemas + fixtures (capabilities/start/status/operation/repair/failure/consent) and propagate new schema/contract selection through the CLI.
- Update OpenCode/SDD reviewer orchestration prompts and bounded Git inspection recipe to operate on frozen trees with literal pathspecs and tightly-scoped Git commands.
Reviewed changes
Copilot reviewed 74 out of 75 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| testdata/golden/sdd-windsurf-global-rules.golden | Updates generated SDD rules to v2 contract routing and frozen-tree reviewer guidance. |
| testdata/golden/sdd-vscode-instructions.golden | Same as above for VS Code instruction golden. |
| testdata/golden/sdd-opencode-cmd-sdd-apply.golden | Updates OpenCode apply-command golden to v2 status routing. |
| testdata/golden/sdd-kiro-instructions.golden | Updates Kiro instruction golden to v2 routing and frozen-tree guidance. |
| testdata/golden/sdd-gemini-geminimd.golden | Updates Gemini instruction golden to v2 routing and frozen-tree guidance. |
| testdata/golden/sdd-cursor-rules.golden | Updates Cursor rules golden to v2 routing and frozen-tree guidance. |
| testdata/golden/sdd-codex-agentsmd.golden | Updates Codex agents golden to v2 routing and frozen-tree guidance. |
| testdata/golden/sdd-codex-agentsmd-powerful.golden | Same as above for “powerful” variant. |
| testdata/golden/sdd-codex-agentsmd-lowcost.golden | Same as above for “lowcost” variant. |
| testdata/golden/sdd-antigravity-rulesmd.golden | Updates Antigravity rules golden to v2 routing and frozen-tree guidance. |
| testdata/golden/combined-windsurf-global-rules.golden | Updates combined Windsurf rules golden to v2 routing and frozen-tree guidance. |
| testdata/golden/combined-claude-claudemd.golden | Updates combined Claude golden to v2 routing and frozen-tree guidance. |
| testdata/golden/sdd-claude-cmd-sdd-apply.golden | Updates Claude apply-command golden to v2 status routing. |
| testdata/golden/sdd-claude-claudemd.golden | Updates Claude instruction golden to v2 routing and frozen-tree guidance. |
| internal/reviewtransaction/snapshot.go | Adds stdout range support for bounded Git capture; tightens environment sanitization for pathspec-related vars. |
| internal/reviewtransaction/frozen_candidate_context.go | Shifts frozen candidate context from diff-bytes to base/candidate trees; adds legacy diff path for v1 compatibility. |
| internal/reviewtransaction/compact_reviewer_capture.go | Selects artifact subject + admitted-result schema based on subject schema; supports legacy v1 subject derivation. |
| internal/reviewtransaction/compact_result_reopen.go | Reopen/inspection now accounts for v1/v2 admitted-result schema selection and subject derivation. |
| internal/reviewtransaction/artifact_subject.go | Introduces v2 artifact subject schema binding on immutable trees; preserves v1 identity via legacy subject builder. |
| internal/reviewtransaction/artifact_subject_test.go | Updates tests for tree-bound subjects and adds coverage for legacy subject identity separation. |
| internal/reviewtransaction/artifact_admission.go | Updates admission binding checks to tree-based context (but currently has a v1-compatibility binding issue). |
| internal/components/sdd/reviewer_envelope_guard_test.go | Updates guard expectations to frozen-tree and bounded Git inspection recipe language. |
| internal/components/sdd/prompts_test.go | Adjusts shell-disabled subagent set (adds review-refuter). |
| internal/components/sdd/inject.go | Enables bash for OpenCode reviewer subagents and emits per-command bash permission patterns. |
| internal/components/sdd/boundedreview.go | Updates reviewer prompt to reference injected frozen-tree context and provides bounded Git command recipe + OpenCode permission generation. |
| internal/components/sdd/bounded_review_contract_test.go | Updates contract clause assertions for v2 routing and frozen-tree inspection expectations. |
| internal/cli/sync_test.go | Updates synced OpenCode controller expectations to v2 status routing. |
| internal/cli/review_status_contract.go | Adds v2 contract handling and v3 status schema for v2 contract outputs; tightens transport validation. |
| internal/cli/review_status_contract_test.go | Updates tests for v2/v3 schema/contract pairing and strictness behavior. |
| internal/cli/review_start_evidence_test.go | Updates hint text expectations to refer to frozen-tree transport for negotiated start. |
| internal/cli/review_start_contract_test.go | Updates negotiated START expectations and contract validation paths. |
| internal/cli/review_start_context_test.go | Ensures negotiated START does not leak candidate bytes and binds against frozen trees + manifest. |
| internal/cli/review_repair.go | Enables repair for both v1 and v2 contracts with distinct schema identities. |
| internal/cli/review_refusal_wording_test.go | Updates refusal wording expectations for new supported contract set and refresh command. |
| internal/cli/review_process_boundary_test.go | Updates process-boundary test fixture to use frozen-tree context. |
| internal/cli/review_operation_contract.go | Adds v2 operation + failure envelopes and threads contract selection into operation encoding. |
| internal/cli/review_operation_contract_test.go | Updates invalid contract test cases to treat v3 as “future/unsupported”. |
| internal/cli/review_next_transition.go | Extends next-transition inputs with base_tree/candidate_tree and subject-hash argument for v2 capture transitions. |
| internal/cli/review_next_transition_test.go | Updates tests for v2 capture transition payloads (trees + manifest) and subject binding. |
| internal/cli/review_integration_contract_guard_test.go | Updates guard regex to allow the new encodeReviewIntegrationOperation optional contract argument. |
| internal/cli/review_incident.go | Updates capture preflight incident payload shape to use base_tree/candidate_tree. |
| internal/cli/review_incident_test.go | Updates assertions for tree-bound frozen context in capture preflight. |
| internal/cli/review_final_verification_retry.go | Threads contract into negotiated operation envelope encoding. |
| internal/cli/review_failure_contract_test.go | Adds explicit test for v2 failure envelope identity. |
| internal/cli/review_facade_test.go | Updates compatibility assertions about which negotiated-only fields can appear unnegotiated. |
| internal/cli/review_consent_contract.go | Adds v2 consent schema/ID and emits v2 consent envelope under v2 contract. |
| internal/cli/review_capabilities.go | Adds v2 contract surface (protocol 2.0) and advertises v2 schemas/features/bootstrap command. |
| internal/cli/review_capabilities_v12_context_test.go | Adjusts legacy schema expectations for v1-era capability surfaces. |
| internal/cli/review_capabilities_test.go | Adds conformance fixture test for v2 capabilities and updates contract validation expectations. |
| internal/assets/skills/_shared/review-ledger-contract.md | Updates shared bounded review contract text to v2 routing and frozen-tree inspection recipe. |
| internal/assets/assets_test.go | Updates OpenCode plugin contract assertions to require new injected context markers and forbid candidate_diff transport. |
| internal/assets/opencode/plugins/review-result-artifacts.ts | Implements injected binding/context lines (GENTLE_AI_REVIEW_BINDING + GENTLE_AI_REVIEW_CONTEXT) and v2 preflight validation. |
| contracts/review-integration/v2/schemas/status.schema.json | Adds v2 status schema (status/v3) requiring tree-backed next_transition capture inputs. |
| contracts/review-integration/v2/schemas/start.schema.json | Adds v2 start schema (start/v3) with base_tree/candidate_tree and manifest requirements. |
| contracts/review-integration/v2/schemas/repair.schema.json | Adds v2 repair schema envelope. |
| contracts/review-integration/v2/schemas/operation.schema.json | Adds v2 operation schema envelope (including finalize). |
| contracts/review-integration/v2/schemas/failure.schema.json | Adds v2 failure schema envelope. |
| contracts/review-integration/v2/schemas/consent.schema.json | Adds v2 consent schema envelope. |
| contracts/review-integration/v2/schemas/capabilities.schema.json | Adds v2 capabilities schema (protocol 2.0) and enumerates required schemas/features. |
| contracts/review-integration/v2/schemas/artifact-subject.schema.json | Adds v2 artifact subject schema binding to base_tree/candidate_tree. |
| contracts/review-integration/v2/schemas/admitted-result.schema.json | Adds v2 admitted-result schema envelope. |
| contracts/review-integration/v2/fixtures/status.fixture.json | Adds v2 status fixture demonstrating tree-backed capture input and subject-hash argument. |
| contracts/review-integration/v2/fixtures/start.fixture.json | Adds v2 start fixture demonstrating tree-backed frozen context and artifact subjects. |
| contracts/review-integration/v2/fixtures/consent.fixture.json | Adds v2 consent fixture with updated invocation pattern. |
| contracts/review-integration/v2/fixtures/capabilities.fixture.json | Adds v2 capabilities conformance fixture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
369
to
375
| var extra any | ||
| if err := decoder.Decode(&extra); err != io.EOF || envelope.Schema != AdmittedReviewerResultSchema || len(envelope.Result) == 0 { | ||
| if err := decoder.Decode(&extra); err != io.EOF || envelope.Schema != admittedReviewerResultSchemaForSubject(envelope.Subject) || len(envelope.Result) == 0 { | ||
| return slot, false, nil | ||
| } | ||
| expected, err := NewArtifactSubject(state, envelope.Subject.AuthorityRevision, frozen, lens, order, envelope.Subject.CorrectionTargetIdentity) | ||
| frozen, expected, err := artifactSubjectForSchema(context.Background(), SnapshotBuilder{Repo: repository}, state, envelope.Subject.AuthorityRevision, frozen, lens, order, envelope.Subject.CorrectionTargetIdentity, envelope.Subject.Schema) | ||
| if err != nil || envelope.Subject != expected || envelope.Admission.Validate(expected) != nil { | ||
| return slot, false, nil |
Comment on lines
+148
to
151
| if request.FrozenContext.BaseTree != request.ExpectedSubject.BaseTree || | ||
| request.FrozenContext.CandidateTree != request.ExpectedSubject.CandidateTree { | ||
| return fail(ArtifactAdmissionBindingMismatch, "frozen candidate trees do not match the artifact subject") | ||
| } |
Comment on lines
142
to
148
| function captureResult(cwd: string, binding: ReviewBinding, result: string): Promise<string> { | ||
| const subjectArgs = binding.subject_hash ? ["--subject-hash", binding.subject_hash] : [] | ||
| return runNative(cwd, [ | ||
| "review", "capture-result", ...repositoryBindingArgs(cwd, binding), | ||
| "--lineage", binding.lineage, "--target", binding.target, | ||
| "--lens", binding.lens, "--order", String(binding.order), "--input", "-", | ||
| "--lens", binding.lens, "--order", String(binding.order), ...subjectArgs, "--input", "-", | ||
| ], result) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/components/sdd/bounded_review_contract_test.go (2)
13-56: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the completion and admission invariants in the central contract test.
The required-clause list does not assert
completed inspection, ordered manifest paths, severe evidence class/causality, or that access failure is not completion. A future regression could remove these admission-critical rules while this test still passes.Suggested additions
"Native Go owns validation, canonicalization, persistence, hashing, reopening, and binding", + "completed inspection", + "every manifest path in order", + "severe evidence class/causality", + "access failure is not completion",🤖 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/components/sdd/bounded_review_contract_test.go` around lines 13 - 56, Extend boundedReviewRequiredClauses to assert the completion and admission invariants: require completed inspection, ordered manifest paths, severe evidence class and causality, and the rule that access failure does not constitute completion. Add assertions using the exact contract wording and keep the central contract test’s existing coverage unchanged.
93-110: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReject stale v1 negotiated routes explicitly.
This test only requires the v2 command; a generated controller containing both v2 and the old v1 status route would still pass. Add the exact v1 status command to the forbidden list, or assert that v2 is the only negotiated status route.
🤖 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/components/sdd/bounded_review_contract_test.go` around lines 93 - 110, The generated OpenCode controller validation in the bounded review contract test must reject the legacy v1 negotiated status route, not merely require v2. Extend the stale-route checks alongside the existing `stale` entries to forbid the exact old v1 status command, or otherwise assert that only the v2 `next_transition` status route is present.internal/cli/review_start_contract.go (1)
205-271: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract transport/mode invariants into named helpers.
Validate()'s candidate-context checks (Lines 205-217, 260-263) combinelegacyTransport,nativeGitTransport,hasTrees,hasDiff,hasManifest, andTargetModeinto dense mixed&&/||expressions relying on Go operator precedence to be correct. I traced the full truth table and didn't find a bug, but this shape is one accidental parenthesis away from silently admitting or rejecting the wrong envelope shape, and the risk only grows as more transports/target modes are added.Consider extracting named boolean helpers (e.g.
legacyContextComplete(...),nativeContextComplete(...)) so each transport's invariant reads as a single named condition instead of an inlined expression that must be re-derived from precedence rules on every review.🤖 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 205 - 271, Extract the dense transport-specific candidate-context checks in Validate into named helpers, such as legacyContextComplete and nativeContextComplete, covering hasTrees, hasDiff, hasManifest, and TargetMode. Replace the mixed &&/|| condition and the transport-specific ArtifactSubjects contract check with calls to these helpers, preserving the existing truth-table behavior while making each transport invariant explicit.
🤖 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 `@contracts/review-integration/v2/schemas/operation.schema.json`:
- Around line 23-44: Update the finalize definition’s state property to use the
same explicit enum of known review states as the START state definition,
replacing the current unrestricted non-empty string constraint while leaving the
other finalize properties unchanged.
In `@internal/assets/skills/_shared/review-ledger-contract.md`:
- Line 13: Split reviewer echo requirements from capture-layer artifact
handling: in internal/assets/skills/_shared/review-ledger-contract.md:13,
require the result to echo subject_hash and report completed manifest inspection
with reviewer-authored findings/evidence, but remove or reword “every manifest
path in order” so reviewers do not construct provider-owned artifacts; in
internal/components/sdd/bounded_review_contract_test.go:212, update the
corresponding contract assertion to match. Preserve capture-layer ownership of
manifest path/mode retention and result-artifact assembly.
In `@internal/cli/review_artifact.go`:
- Around line 389-394: Export the existing
admittedReviewerResultSchemaForSubject helper as
reviewtransaction.AdmittedReviewerResultSchemaForSubject, then replace the
duplicated schema-selection branches for envelopeSchema and wantSchema in the
relevant review artifact flows with calls to that shared helper, preserving the
current subject-based v1/v2 behavior.
In `@internal/cli/review_facade.go`:
- Line 3577: Update encodeCompactFacadeFinalize to apply WithLegacyCandidateDiff
using the received contract when building capture context for v1 FINALIZE
transitions in the reviewing-state branch. Preserve the existing
FrozenCandidateContext behavior and ensure --next-transition retains the eager
legacy diff supplied by START and STATUS.
In `@internal/components/sdd/boundedreview.go`:
- Around line 148-175: Update the subject_hash copy instruction near the
result-schema guidance to source it from the context’s
artifact_subject.subject_hash, matching the sole-source declaration in ## Input;
remove the reference to GENTLE_AI_REVIEW_BINDING.subject_hash while leaving the
surrounding review and result instructions unchanged.
- Around line 213-224: The openCodeReviewerPermission allowlist currently
replaces all placeholders with unconstrained wildcards. Update the pattern
construction in openCodeReviewerPermission so tree placeholders match only
hexadecimal object IDs and <path> retains its quoted pathspec form, preventing
shell metacharacters or chained commands from matching while preserving the
intended reviewer Git commands.
In `@internal/components/sdd/review_ledger_contract_test.go`:
- Around line 196-204: The ordering check in the permission serialization test
currently inspects json.Marshal output, whose sorted map keys make the failure
condition unreachable. Update the test around permission construction and the
deny/allow assertions to read or render the written opencode.json bytes, then
assert that the broad `"*":"deny"` entry appears before the narrow allow entries
in that file.
In `@internal/components/sdd/reviewer_envelope_guard_test.go`:
- Around line 202-226: The execution-capability assertion is currently disabled
because executionToolVocabulary and declaredLensTools are unused. Update the
relevant guard test flow, including
TestLensAgentPromptsStateWhereTheirInputComesFrom, to invoke these symbols and
enforce the intended tool assertion, or remove both declarations and their
unused scaffolding if that assertion is no longer required.
In `@internal/reviewtransaction/artifact_admission.go`:
- Around line 148-151: Update the artifact admission binding validation around
the frozen candidate tree comparison to branch on the artifact subject schema,
matching ValidateArtifactSubject. For v1 subjects created by
NewLegacyArtifactSubject, compare CandidateDiffSHA256 with
FrozenContext.LegacyCandidateDiff instead of comparing BaseTree/CandidateTree;
retain the existing tree identity checks for v2 subjects.
In `@internal/reviewtransaction/compact_result_reopen.go`:
- Around line 325-333: Thread the caller’s context through the compact-result
reopen classification path: add ctx to classifyCompactResultReopenSlots and
inspectCompactResultReopenSlot, pass it from ReopenCompactReviewerResults and
buildCompactResultReopenPlan, and use it when invoking artifactSubjectForSchema
instead of context.Background().
In `@internal/reviewtransaction/frozen_candidate_context_test.go`:
- Around line 403-432: Update frozenCandidateGitDiff to defer cleanup
immediately after isolatedImmutableTreeGit succeeds, and remove the per-test
t.Cleanup registration so each isolated Git environment is released when that
helper returns.
In `@internal/reviewtransaction/frozen_candidate_context.go`:
- Around line 100-126: Update WithLegacyCandidateDiff to validate the supplied
snapshot evidence using the same validation performed by the native
FrozenCandidateContext constructor before rendering the legacy diff. Keep the
existing frozen-tree consistency check, and return validation errors before
calling isolatedImmutableTreeGit or runGitLimited.
In `@internal/reviewtransaction/snapshot.go`:
- Around line 1627-1643: The isolation branch in the environment filtering logic
redundantly checks for a “GIT_” prefix after confirming membership in
processEssential. Remove that prefix condition so essential variables are
appended directly, or retain it only with a comment explaining its defensive
purpose for future processEssential additions.
---
Outside diff comments:
In `@internal/cli/review_start_contract.go`:
- Around line 205-271: Extract the dense transport-specific candidate-context
checks in Validate into named helpers, such as legacyContextComplete and
nativeContextComplete, covering hasTrees, hasDiff, hasManifest, and TargetMode.
Replace the mixed &&/|| condition and the transport-specific ArtifactSubjects
contract check with calls to these helpers, preserving the existing truth-table
behavior while making each transport invariant explicit.
In `@internal/components/sdd/bounded_review_contract_test.go`:
- Around line 13-56: Extend boundedReviewRequiredClauses to assert the
completion and admission invariants: require completed inspection, ordered
manifest paths, severe evidence class and causality, and the rule that access
failure does not constitute completion. Add assertions using the exact contract
wording and keep the central contract test’s existing coverage unchanged.
- Around line 93-110: The generated OpenCode controller validation in the
bounded review contract test must reject the legacy v1 negotiated status route,
not merely require v2. Extend the stale-route checks alongside the existing
`stale` entries to forbid the exact old v1 status command, or otherwise assert
that only the v2 `next_transition` status route is present.
🪄 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: 2282ddf0-a538-4822-9e97-6e8ddc959bbf
⛔ Files ignored due to path filters (15)
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-claude-cmd-sdd-apply.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 (60)
contracts/review-integration/v2/fixtures/capabilities.fixture.jsoncontracts/review-integration/v2/fixtures/consent.fixture.jsoncontracts/review-integration/v2/fixtures/start.fixture.jsoncontracts/review-integration/v2/fixtures/status.fixture.jsoncontracts/review-integration/v2/schemas/admitted-result.schema.jsoncontracts/review-integration/v2/schemas/artifact-subject.schema.jsoncontracts/review-integration/v2/schemas/capabilities.schema.jsoncontracts/review-integration/v2/schemas/consent.schema.jsoncontracts/review-integration/v2/schemas/failure.schema.jsoncontracts/review-integration/v2/schemas/operation.schema.jsoncontracts/review-integration/v2/schemas/repair.schema.jsoncontracts/review-integration/v2/schemas/start.schema.jsoncontracts/review-integration/v2/schemas/status.schema.jsondocs/review-integration.mdinternal/assets/assets_test.gointernal/assets/opencode/plugins/review-result-artifacts.tsinternal/assets/review_plugin_recovery_test.gointernal/assets/skills/_shared/review-ledger-contract.mdinternal/cli/review_artifact.gointernal/cli/review_capabilities.gointernal/cli/review_capabilities_test.gointernal/cli/review_capabilities_v12_context_test.gointernal/cli/review_consent_contract.gointernal/cli/review_facade.gointernal/cli/review_facade_test.gointernal/cli/review_failure_contract_test.gointernal/cli/review_final_verification_retry.gointernal/cli/review_incident.gointernal/cli/review_incident_test.gointernal/cli/review_integration_contract_guard_test.gointernal/cli/review_next_transition.gointernal/cli/review_next_transition_test.gointernal/cli/review_operation_contract.gointernal/cli/review_operation_contract_test.gointernal/cli/review_process_boundary_test.gointernal/cli/review_provider_artifact_contract_test.gointernal/cli/review_refusal_wording_test.gointernal/cli/review_repair.gointernal/cli/review_start_context_test.gointernal/cli/review_start_contract.gointernal/cli/review_start_contract_test.gointernal/cli/review_start_evidence_test.gointernal/cli/review_start_oversized_candidate_test.gointernal/cli/review_status_contract.gointernal/cli/review_status_contract_test.gointernal/cli/sync_test.gointernal/components/sdd/bounded_review_contract_test.gointernal/components/sdd/boundedreview.gointernal/components/sdd/inject.gointernal/components/sdd/prompts_test.gointernal/components/sdd/review_ledger_contract_test.gointernal/components/sdd/reviewer_envelope_guard_test.gointernal/reviewtransaction/artifact_admission.gointernal/reviewtransaction/artifact_subject.gointernal/reviewtransaction/artifact_subject_test.gointernal/reviewtransaction/compact_result_reopen.gointernal/reviewtransaction/compact_reviewer_capture.gointernal/reviewtransaction/frozen_candidate_context.gointernal/reviewtransaction/frozen_candidate_context_test.gointernal/reviewtransaction/snapshot.go
Comment on lines
+23
to
+44
| "finalize": { | ||
| "type": "object", | ||
| "additionalProperties": false, | ||
| "required": ["operation", "lineage_id", "state", "action", "store_revision"], | ||
| "properties": { | ||
| "operation": {"const": "review/finalize"}, | ||
| "lineage_id": {"type": "string", "minLength": 1}, | ||
| "state": {"type": "string", "minLength": 1}, | ||
| "action": {"type": "string", "minLength": 1}, | ||
| "store_revision": {"$ref": "#/$defs/sha256"}, | ||
| "eligibility": {"$ref": "status.schema.json#/properties/eligibility"}, | ||
| "next_transition": {"$ref": "status.schema.json#/$defs/next_transition"}, | ||
| "validation_request": {"$ref": "../../v1/schemas/targeted-validation-request.schema.json"}, | ||
| "escalation": {"type": "string", "minLength": 1} | ||
| }, | ||
| "allOf": [ | ||
| { | ||
| "if": {"required": ["validation_request"]}, | ||
| "then": {"properties": {"state": {"const": "correction_required"}}} | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Tighten state to an enum of known review states.
state (Line 30) accepts any non-empty string. Elsewhere in this contract (e.g. START's state property) valid states are constrained by an explicit enum; this finalize result def should do the same so schema-only consumers can't accept a state value the Go implementation would never actually produce.
♻️ Proposed fix
- "state": {"type": "string", "minLength": 1},
+ "state": {"enum": ["reviewing", "correction_required", "validating", "approved", "escalated"]},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "finalize": { | |
| "type": "object", | |
| "additionalProperties": false, | |
| "required": ["operation", "lineage_id", "state", "action", "store_revision"], | |
| "properties": { | |
| "operation": {"const": "review/finalize"}, | |
| "lineage_id": {"type": "string", "minLength": 1}, | |
| "state": {"type": "string", "minLength": 1}, | |
| "action": {"type": "string", "minLength": 1}, | |
| "store_revision": {"$ref": "#/$defs/sha256"}, | |
| "eligibility": {"$ref": "status.schema.json#/properties/eligibility"}, | |
| "next_transition": {"$ref": "status.schema.json#/$defs/next_transition"}, | |
| "validation_request": {"$ref": "../../v1/schemas/targeted-validation-request.schema.json"}, | |
| "escalation": {"type": "string", "minLength": 1} | |
| }, | |
| "allOf": [ | |
| { | |
| "if": {"required": ["validation_request"]}, | |
| "then": {"properties": {"state": {"const": "correction_required"}}} | |
| } | |
| ] | |
| } | |
| "finalize": { | |
| "type": "object", | |
| "additionalProperties": false, | |
| "required": ["operation", "lineage_id", "state", "action", "store_revision"], | |
| "properties": { | |
| "operation": {"const": "review/finalize"}, | |
| "lineage_id": {"type": "string", "minLength": 1}, | |
| "state": {"enum": ["reviewing", "correction_required", "validating", "approved", "escalated"]}, | |
| "action": {"type": "string", "minLength": 1}, | |
| "store_revision": {"$ref": "`#/`$defs/sha256"}, | |
| "eligibility": {"$ref": "status.schema.json#/properties/eligibility"}, | |
| "next_transition": {"$ref": "status.schema.json#/$defs/next_transition"}, | |
| "validation_request": {"$ref": "../../v1/schemas/targeted-validation-request.schema.json"}, | |
| "escalation": {"type": "string", "minLength": 1} | |
| }, | |
| "allOf": [ | |
| { | |
| "if": {"required": ["validation_request"]}, | |
| "then": {"properties": {"state": {"const": "correction_required"}}} | |
| } | |
| ] | |
| } |
🤖 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/v2/schemas/operation.schema.json` around lines
23 - 44, Update the finalize definition’s state property to use the same
explicit enum of known review states as the START state definition, replacing
the current unrestricted non-empty string constraint while leaving the other
finalize properties unchanged.
| A canonical four-lens selection is long work: before the first lens runs, give the one cost/side-effect forecast — four reviewer model runs over the frozen candidate, the frozen correction budget, and the at-most-one bounded correction it implies — once per candidate, never per lens. | ||
|
|
||
| Run each exact `review.capture-result` collection input once in the foreground. Begin its reviewer task prompt with the exact literal prefix `GENTLE_AI_REVIEW_BINDING `, including the trailing space and never `=`, immediately followed by one-line bound JSON assembled only from that input's arguments and `artifact_subject`: `lineage`, `target`, `lens`, `order`, `revision` from `expected-revision`, `repository_context` from `repository-context`, and `subject_hash` from `artifact_subject.subject_hash`; omit only fields the provider omitted. The prefix and JSON are the first bytes of the prompt. Return one JSON object echoing `subject_hash`; require `inspection.status: "completed"`, all manifest paths in order as `inspection.paths`, `findings`/`evidence`, and severe `evidence_class`/`causal_disposition`; access failure is not completion. `gentle-ai review capture-result` follows the native transition; handles are cwd-independent and legacy bindings need `--cwd`. Pass manifests in lens order with repeated `--result-artifact-file <path>` arguments, BOM-less UTF-8 on Windows PowerShell 5.1. The POSIX inline `--result-artifact '<manifest-json>'` form remains compatible; so does provider-owned `--captured-results`; never pass raw `--result`. Native Go validates, canonicalizes, persists, hashes, reopens, and binds results; models never construct canonical bytes or hashes. Freeze merged findings. Only `introduced`, `behavior-activated`, or `worsened` with changed-hunk, candidate-created-path, differential-test, or before/after proof may block. Route `pre-existing` and `base-only` to follow-ups; `unknown` escalates. WARNING/SUGGESTION remain `info`. Deterministic blockers need no refuter; inferential blockers share one read-only refuter batch. Judgment Day uses two independent judges. | ||
| Run each exact `review.capture-result` collection input once in the foreground. Begin its reviewer task prompt with the exact literal prefix `GENTLE_AI_REVIEW_BINDING `, including the trailing space and never `=`, followed by one-line JSON assembled only from that input: `lineage`, `target`, `lens`, `order`, `revision` from `expected-revision`, `repository_context`, and `subject_hash` from `artifact_subject.subject_hash`; omit only provider-omitted fields. These are the prompt's first bytes. Never add `candidate_diff` or candidate bytes. Return one JSON object echoing `subject_hash`, with completed inspection, every manifest path in order, findings/evidence, and severe evidence class/causality; access failure is not completion. Capture follows the native transition; opaque handles are cwd-independent and legacy bindings need `--cwd`. Finalize with manifests in lens order via repeated `--result-artifact-file <path>` (BOM-less UTF-8 on Windows PowerShell 5.1); POSIX inline `--result-artifact '<manifest-json>'` and provider-owned `--captured-results` remain compatible; never pass raw `--result`. Native Go owns validation, canonicalization, persistence, hashing, reopening, and binding. Only candidate-caused severe findings block; pre-existing/base-only become follow-ups, unknown escalates, WARNING/SUGGESTION remain info. Deterministic blockers need no refuter; inferential blockers share one read-only refuter batch. Judgment Day uses two judges. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repository files matching relevant names:\n'
fd -a 'review-ledger-contract\.md|bounded_review_contract_test\.go|bounded_review|capture|subject_hash|manifest' . | sed 's#^\./##' | head -200
printf '\nTarget file excerpt:\n'
if [ -f internal/assets/skills/_shared/review-ledger-contract.md ]; then
nl -ba internal/assets/skills/_shared/review-ledger-contract.md | sed -n '1,80p'
fi
printf '\nTest file excerpt around line 212:\n'
if [ -f internal/components/sdd/bounded_review_contract_test.go ]; then
nl -ba internal/components/sdd/bounded_review_contract_test.go | sed -n '180,240p'
fi
printf '\nSearch for subject_hash and GENTLE_AI_REVIEW_BINDING references:\n'
rg -n "subject_hash|GENTLE_AI_REVIEW_BINDING|GENTLE_AI_REVIEW_CONTEXT|--result-artifact|captured-results|review\.capture-result|Return one JSON|changed_path_manifest|base_tree|candidate_tree" internal -SRepository: Gentleman-Programming/gentle-ai
Length of output: 1641
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Target file excerpt:\n'
python3 - <<'PY'
from pathlib import Path
p = Path('internal/assets/skills/_shared/review-ledger-contract.md')
for i, line in enumerate(p.read_text().splitlines(), 1):
if 1 <= i <= 80:
print(f'{i:>5}: {line}')
PY
printf '\nTest file excerpt around line 212:\n'
python3 - <<'PY'
from pathlib import Path
p = Path('internal/components/sdd/bounded_review_contract_test.go')
lines = p.read_text().splitlines()
for i in range(max(1, 180-1), min(len(lines), 240)+1):
print(f'{i+1:>5}: {lines[i]}')
PY
printf '\nSearch for subject_hash, bindings, capture, and provider context terms:\n'
grep -RInE 'subject_hash|GENTLE_AI_REVIEW_BINDING|GENTLE_AI_REVIEW_CONTEXT|--result-artifact|captured-results|review\.capture-result|Return one JSON|changed_path_manifest|base_tree|candidate_tree|provider-omitted|provider-owned|provider_owned|manifest|lens_order|len\(\s*manifests\s*\)|manifests|manifest' internal internal/assets internal/components internal/reviewtransaction internal/sddstatus -SRepository: Gentleman-Programming/gentle-ai
Length of output: 12988
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Bounded review asset path candidates:\n'
git ls-files | grep -E 'internal/assets/.*/agents/review-.*\.md|renderBoundedReview|nativeReviewerResultSchema|Return one JSON|subject_hash|inspection'
printf '\nTest helper definitions:\n'
python3 - <<'PY'
from pathlib import Path
p = Path('internal/components/sdd/bounded_review_contract_test.go')
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if i <= 220 and any(s in line for s in ['func boundedReviewContract', 'func renderBoundedReviewAsset', 'func judgmentDayReviewerContract', 'nativeReviewerResultSchema']):
j = max(1, i-30)
k = min(len(lines), i+60)
print(f'\n--- {line.strip()} around line {i} ---')
for n in range(j-1, k):
print(f'{n+1:>5}: {lines[n]}')
PY
printf '\nReview result schema/usages:\n'
grep -rnE 'nativeReviewerResultSchema|subject_hash|inspection.*status|Return one JSON|RenderedReviewersAreReadOnlyAndSingleResult|judgmentDayReviewerContract|native reviewer|provider-owned|provider-owned' internal/components internal/assets internal/reviewtransaction internal/sddstatus -SRepository: Gentleman-Programming/gentle-ai
Length of output: 1346
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Bounded review asset excerpts:\n'
for f in internal/assets/claude/agents/review-risk.md internal/assets/cursor/agents/review-risk.md internal/assets/kimi/agents/review-risk.md internal/assets/kiro/agents/review-risk.md; do
echo "--- $f ---"
python3 - <<'PY' "$f"
import sys
p = sys.argv[1]
lines = open(p, encoding='utf-8').read().splitlines()
for n, line in enumerate(lines, 1):
if any(x in line for x in ['GENTLE_AI_REVIEW_BINDING', 'GENTLE_AI_REVIEW_CONTEXT', 'subject_hash', 'Return one JSON', 'inspection', 'changed_path_manifest', 'provider-owned', 'provider-owned', 'base_tree', 'candidate_tree']):
print(f'{n:>5}: {line}')
PY
done
printf '\nFind render/import/schema definitions compactly:\n'
grep -rnE 'func boundedReviewContract|func renderBoundedReviewAsset|func judgmentDayReviewerContract|nativeReviewerResultSchema|rendered contract|subject_hash|Return one JSON|inspection|subject_hash|provider-owned|provider-owned|GENTLE_AI_REVIEW_BINDING|GENTLE_AI_REVIEW_CONTEXT' internal/components internal/assets internal/reviewtransaction internal/sddstatus internal/assets/skills/_shared/review-ledger-contract.md --include='*.go' --include='*.md' --include='*.jsx'Repository: Gentleman-Programming/gentle-ai
Length of output: 44118
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant contract tests and renderer sources:\n'
python3 - <<'PY'
from pathlib import Path
for p in [Path('internal/components/sdd/bounded_review_contract_test.go'),
Path('internal/components/sdd/reviewer_envelope_guard_test.go'),
Path('internal/components/sdd/boundedreview.go'),
Path('internal/reviewtransaction/reviewer_envelope_test.go'),
Path('internal/reviewtransaction/artifact_admission.go')]:
print(f'\n--- {p} ---')
lines = p.read_text().splitlines()
seen = []
for i, line in enumerate(lines, 1):
if p.name == 'bounded_review_contract_test.go':
if 15 <= i <= 110 or 125 <= i <= 165 or 190 <= i <= 235 or 300 <= i <= 330:
seen.append((i,line))
elif p.name == 'reviewer_envelope_guard_test.go':
if 1 <= i <= 80 or 125 <= i <= 155 or 200 <= i <= 220 or 255 <= i <= 275:
seen.append((i,line))
elif p.name == 'reviewer_envelope_test.go':
if 1 <= i <= 110:
seen.append((i,line))
elif p.name == 'boundedreview.go':
if 15 <= i <= 210:
seen.append((i,line))
elif p.name == 'artifact_admission.go':
if 85 <= i <= 200:
seen.append((i,line))
for i,line in seen:
print(f'{i:>6}: {line}')
PY
printf '\nCheck manifest/result artifact handling in capture:\n'
python3 - <<'PY'
from pathlib import Path
for p in Path('internal/reviewtransaction').glob('*.go'):
txt = p.read_text(errors='ignore')
if 'manifest' in txt or 'subject_hash' in txt or 'inspection' in txt or 'Captured' in txt or 'result-artifact' in txt:
hits = []
for i,l in enumerate(txt.splitlines(),1):
if any(s in l for s in ['manifest', 'subject_hash', 'inspection', 'Captured', 'result-artifact', 'capture']):
hits.append((i,l))
if hits:
print(f'\n--- {p} ({len(hits)} hits) ---')
for i,l in hits[:40]:
print(f'{i:>6}: {l}')
PYRepository: Gentleman-Programming/gentle-ai
Length of output: 50387
Split echo requirements from capture-layer obligations.
Captured review results must echo the binding’s subject_hash and report a completed manifest inspection, but only the finding/evidence payload is reviewer-authored. Remove or reword every manifest path in order so it cannot imply that the reviewer constructs provider-owned artifacts; let the capture layer preserve path and mode and assemble result artifacts as a separate provider-owned step.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 13-13: Spaces inside code span elements
(MD038, no-space-in-code)
📍 Affects 2 files
internal/assets/skills/_shared/review-ledger-contract.md#L13-L13(this comment)internal/components/sdd/bounded_review_contract_test.go#L212-L212
🤖 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/assets/skills/_shared/review-ledger-contract.md` at line 13, Split
reviewer echo requirements from capture-layer artifact handling: in
internal/assets/skills/_shared/review-ledger-contract.md:13, require the result
to echo subject_hash and report completed manifest inspection with
reviewer-authored findings/evidence, but remove or reword “every manifest path
in order” so reviewers do not construct provider-owned artifacts; in
internal/components/sdd/bounded_review_contract_test.go:212, update the
corresponding contract assertion to match. Preserve capture-layer ownership of
manifest path/mode retention and result-artifact assembly.
Comment on lines
+389
to
+394
| envelopeSchema := reviewAdmittedResultSchema | ||
| if subject.Schema == reviewtransaction.ArtifactSubjectSchemaV1 { | ||
| envelopeSchema = reviewtransaction.AdmittedReviewerResultSchemaV1 | ||
| } | ||
| envelope, err := json.Marshal(admittedReviewerResult{ | ||
| Schema: reviewAdmittedResultSchema, Subject: subject, Admission: admission, Result: result, | ||
| Schema: envelopeSchema, Subject: subject, Admission: admission, Result: result, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate schema→envelope-schema branching; export the shared helper instead.
Both envelopeSchema (389-394) and wantSchema (853-857) reimplement the exact branch already defined as admittedReviewerResultSchemaForSubject in internal/reviewtransaction/compact_result_reopen.go. Exporting that helper (e.g. reviewtransaction.AdmittedReviewerResultSchemaForSubject) and reusing it here removes duplicate logic and one less place for the v1/v2 branch to drift out of sync.
Also applies to: 853-857
🤖 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_artifact.go` around lines 389 - 394, Export the existing
admittedReviewerResultSchemaForSubject helper as
reviewtransaction.AdmittedReviewerResultSchemaForSubject, then replace the
duplicated schema-selection branches for envelopeSchema and wantSchema in the
relevant review artifact flows with calls to that shared helper, preserving the
current subject-based v1/v2 behavior.
| } | ||
|
|
||
| func encodeCompactFacadeFinalize(stdout io.Writer, negotiated, actionEligibility, nextTransition bool, state reviewtransaction.CompactState, revision string, store reviewtransaction.CompactStore, action string, contexts ...reviewFinalizeOutputContext) error { | ||
| func encodeCompactFacadeFinalize(stdout io.Writer, negotiated bool, contract string, actionEligibility, nextTransition bool, state reviewtransaction.CompactState, revision string, store reviewtransaction.CompactStore, action string, contexts ...reviewFinalizeOutputContext) error { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve legacy candidate-diff context for v1 FINALIZE transitions.
Line 3577 receives contract, but the reviewing-state branch later builds capture context from FrozenCandidateContext without calling WithLegacyCandidateDiff for v1. Thus a v1 review finalize --next-transition can advertise a capture transition lacking the eager diff that START and STATUS still supply for v1, breaking legacy reviewer capture.
Proposed fix
- frozen, err := (reviewtransaction.SnapshotBuilder{Repo: outputContext.Repo}).FrozenCandidateContext(outputContext.Context, state.InitialSnapshot)
+ builder := reviewtransaction.SnapshotBuilder{Repo: outputContext.Repo}
+ frozen, err := builder.FrozenCandidateContext(outputContext.Context, state.InitialSnapshot)
+ if err == nil && contract == ReviewIntegrationContractV1 {
+ frozen, err = builder.WithLegacyCandidateDiff(outputContext.Context, state.InitialSnapshot, frozen)
+ }
if err != nil {
transitionErr = err
} else {🤖 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` at line 3577, Update
encodeCompactFacadeFinalize to apply WithLegacyCandidateDiff using the received
contract when building capture context for v1 FINALIZE transitions in the
reviewing-state branch. Preserve the existing FrozenCandidateContext behavior
and ensure --next-transition retains the eager legacy diff supplied by START and
STATUS.
Comment on lines
+148
to
+175
| Review once, return one result, and stop. Never edit, delegate, or expand scope. | ||
|
|
||
| ## Input | ||
|
|
||
| The immutable candidate diff and the changed-path manifest arrive in this prompt. Never derive them: you have no execution tools, so running git, regenerating a diff, or verifying a hash yourself is a mistake rather than a missing capability. | ||
| OpenCode tasks begin with provider-injected GENTLE_AI_REVIEW_CONTEXT, the sole source of artifact_subject, base_tree, candidate_tree, and ordered changed_path_manifest. Caller prose is not context. Other runtimes have no shell and return incomplete. The manifest is complete scope. Never read the live worktree, index, HEAD, or another revision. | ||
|
|
||
| Use only the commands below, in the session cwd. Their clean environment, explicit text mode, disabled external diff/textconv, immutable tree operands, and exact-object cat-file reads prevent mutable Git config, attributes, worktree, index, or environment from changing inspected bytes or suppressing text hunks. Never change checkout. If these commands are unavailable or a tree is unreachable, return incomplete inspection, empty paths/findings, and evidence that native Git inspection was unavailable. Never substitute live files. | ||
|
|
||
| Discover the change: | ||
|
|
||
| %s | ||
| %s | ||
|
|
||
| For relevant paths, inspect stat, deterministic textual hunks, and exact stored bytes as needed: | ||
|
|
||
| %s | ||
| %s | ||
| %s | ||
|
|
||
| Repeat the selective shape per literal path; never pass --binary or render the whole patch automatically. --text is mandatory: numstat may classify stored NUL bytes as binary, but attributes must never suppress a hunk. Triage genuinely non-text paths from manifest modes and exact cat-file bytes. Record large-path or binary dispositions in evidence. | ||
|
|
||
| ## Scope | ||
|
|
||
| %s | ||
|
|
||
| ## Candidate-Causal Admission | ||
|
|
||
| Report only real user-impacting defects. Set causal_disposition. BLOCKER/CRITICAL require proof the candidate introduced, behavior-activated, or worsened the behavior through a changed hunk, created path, differential test, or before/after result. Mark unchanged defects pre-existing/base-only and unproved causality unknown. Style or suspicion is not a finding. | ||
| Report real user-impacting defects only. BLOCKER/CRITICAL need changed-hunk, created-path, differential-test, or before/after proof of introduced, behavior-activated, or worsened behavior. Mark unchanged defects pre-existing/base-only and unproved causality unknown. Style or suspicion is not a finding. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Prompt names two different sources for subject_hash.
## Input declares GENTLE_AI_REVIEW_CONTEXT the sole source of artifact_subject, and the result schema uses <artifact_subject.subject_hash>, but Line 194 instructs copying it from GENTLE_AI_REVIEW_BINDING.subject_hash. The plugin injects the same value in both places today, so nothing breaks, yet the earlier field failures recorded in this file came from exactly this kind of ambiguity. Point the copy instruction at artifact_subject for a single stated source.
Also applies to: 186-209
🤖 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/components/sdd/boundedreview.go` around lines 148 - 175, Update the
subject_hash copy instruction near the result-schema guidance to source it from
the context’s artifact_subject.subject_hash, matching the sole-source
declaration in ## Input; remove the reference to
GENTLE_AI_REVIEW_BINDING.subject_hash while leaving the surrounding review and
result instructions unchanged.
Comment on lines
+148
to
151
| if request.FrozenContext.BaseTree != request.ExpectedSubject.BaseTree || | ||
| request.FrozenContext.CandidateTree != request.ExpectedSubject.CandidateTree { | ||
| return fail(ArtifactAdmissionBindingMismatch, "frozen candidate trees do not match the artifact subject") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Legacy (v1) admission always fails: tree comparison ignores schema.
ExpectedSubject.BaseTree/CandidateTree are always "" for a v1 legacy subject (NewLegacyArtifactSubject clears them), but FrozenContext.BaseTree/CandidateTree are always populated real Git tree hashes — WithLegacyCandidateDiff only adds LegacyCandidateDiff, it never clears the tree fields it received. This means the new equality check unconditionally fails for every legacy subject, so any real (non-preflight) capture-result under contract v1, plus finalize-time re-verification via decodeAdmittedReviewerResult, will always reject with "frozen candidate trees do not match the artifact subject" — breaking v1 end-to-end.
Branch this check on the subject schema, mirroring ValidateArtifactSubject's v1/v2 split: verify CandidateDiffSHA256 against FrozenContext.LegacyCandidateDiff for v1, and tree identity for v2.
🐛 Proposed schema-aware fix
- if request.FrozenContext.BaseTree != request.ExpectedSubject.BaseTree ||
- request.FrozenContext.CandidateTree != request.ExpectedSubject.CandidateTree {
- return fail(ArtifactAdmissionBindingMismatch, "frozen candidate trees do not match the artifact subject")
+ if request.ExpectedSubject.Schema == ArtifactSubjectSchemaV1 {
+ if request.FrozenContext.LegacyCandidateDiff == nil ||
+ request.FrozenContext.LegacyCandidateDiff.SHA256 != request.ExpectedSubject.CandidateDiffSHA256 {
+ return fail(ArtifactAdmissionBindingMismatch, "frozen candidate diff does not match the artifact subject")
+ }
+ } else if request.FrozenContext.BaseTree != request.ExpectedSubject.BaseTree ||
+ request.FrozenContext.CandidateTree != request.ExpectedSubject.CandidateTree {
+ return fail(ArtifactAdmissionBindingMismatch, "frozen candidate trees do not match the artifact subject")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if request.FrozenContext.BaseTree != request.ExpectedSubject.BaseTree || | |
| request.FrozenContext.CandidateTree != request.ExpectedSubject.CandidateTree { | |
| return fail(ArtifactAdmissionBindingMismatch, "frozen candidate trees do not match the artifact subject") | |
| } | |
| if request.ExpectedSubject.Schema == ArtifactSubjectSchemaV1 { | |
| if request.FrozenContext.LegacyCandidateDiff == nil || | |
| request.FrozenContext.LegacyCandidateDiff.SHA256 != request.ExpectedSubject.CandidateDiffSHA256 { | |
| return fail(ArtifactAdmissionBindingMismatch, "frozen candidate diff does not match the artifact subject") | |
| } | |
| } else if request.FrozenContext.BaseTree != request.ExpectedSubject.BaseTree || | |
| request.FrozenContext.CandidateTree != request.ExpectedSubject.CandidateTree { | |
| return fail(ArtifactAdmissionBindingMismatch, "frozen candidate trees do not match the artifact subject") | |
| } |
🤖 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/reviewtransaction/artifact_admission.go` around lines 148 - 151,
Update the artifact admission binding validation around the frozen candidate
tree comparison to branch on the artifact subject schema, matching
ValidateArtifactSubject. For v1 subjects created by NewLegacyArtifactSubject,
compare CandidateDiffSHA256 with FrozenContext.LegacyCandidateDiff instead of
comparing BaseTree/CandidateTree; retain the existing tree identity checks for
v2 subjects.
Comment on lines
+325
to
+333
| func classifyCompactResultReopenSlots(repository, storeDir string, state CompactState, frozen FrozenCandidateContext, authorizedLenses []string) ([]CompactResultReopenSlot, []CompactResultReopenSlot, error) { | ||
| authorized := make(map[string]struct{}, len(authorizedLenses)) | ||
| for _, lens := range authorizedLenses { | ||
| authorized[lens] = struct{}{} | ||
| } | ||
| quarantined := make([]CompactResultReopenSlot, 0) | ||
| retained := make([]CompactResultReopenSlot, 0) | ||
| for order, lens := range state.SelectedLenses { | ||
| slot, trusted, err := inspectCompactResultReopenSlot(storeDir, state, frozen, order, lens) | ||
| slot, trusted, err := inspectCompactResultReopenSlot(repository, storeDir, state, frozen, order, lens) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
classifyCompactResultReopenSlots/inspectCompactResultReopenSlot drop the caller's context.
Both functions take no ctx parameter, so inspectCompactResultReopenSlot falls back to a hardcoded context.Background() when invoking artifactSubjectForSchema (which runs a real Git subprocess for legacy subjects via WithLegacyCandidateDiff). Both call sites (ReopenCompactReviewerResults, buildCompactResultReopenPlan) have a real ctx in scope but never pass it through, unlike the equivalent paths in compact_reviewer_capture.go. This silently defeats caller cancellation/deadlines for this Git-invoking path.
🔧 Proposed fix: thread ctx through
-func classifyCompactResultReopenSlots(repository, storeDir string, state CompactState, frozen FrozenCandidateContext, authorizedLenses []string) ([]CompactResultReopenSlot, []CompactResultReopenSlot, error) {
+func classifyCompactResultReopenSlots(ctx context.Context, repository, storeDir string, state CompactState, frozen FrozenCandidateContext, authorizedLenses []string) ([]CompactResultReopenSlot, []CompactResultReopenSlot, error) {
...
- slot, trusted, err := inspectCompactResultReopenSlot(repository, storeDir, state, frozen, order, lens)
+ slot, trusted, err := inspectCompactResultReopenSlot(ctx, repository, storeDir, state, frozen, order, lens)
...
-func inspectCompactResultReopenSlot(repository, storeDir string, state CompactState, frozen FrozenCandidateContext, order int, lens string) (CompactResultReopenSlot, bool, error) {
+func inspectCompactResultReopenSlot(ctx context.Context, repository, storeDir string, state CompactState, frozen FrozenCandidateContext, order int, lens string) (CompactResultReopenSlot, bool, error) {
...
- frozen, expected, err := artifactSubjectForSchema(context.Background(), SnapshotBuilder{Repo: repository}, state, envelope.Subject.AuthorityRevision, frozen, lens, order, envelope.Subject.CorrectionTargetIdentity, envelope.Subject.Schema)
+ frozen, expected, err := artifactSubjectForSchema(ctx, SnapshotBuilder{Repo: repository}, state, envelope.Subject.AuthorityRevision, frozen, lens, order, envelope.Subject.CorrectionTargetIdentity, envelope.Subject.Schema)Also applies to: 351-374
🤖 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/reviewtransaction/compact_result_reopen.go` around lines 325 - 333,
Thread the caller’s context through the compact-result reopen classification
path: add ctx to classifyCompactResultReopenSlots and
inspectCompactResultReopenSlot, pass it from ReopenCompactReviewerResults and
buildCompactResultReopenPlan, and use it when invoking artifactSubjectForSchema
instead of context.Background().
Comment on lines
+403
to
+432
| func frozenCandidatePathDiff(t *testing.T, repo string, frozen FrozenCandidateContext) []byte { | ||
| t.Helper() | ||
| payload, err := diff.Bytes() | ||
| if err != nil { | ||
| t.Fatalf("candidate diff metadata = %#v: %v", diff, err) | ||
| var payload []byte | ||
| for _, entry := range frozen.ChangedPathManifest { | ||
| payload = append(payload, frozenCandidateGitDiff(t, repo, frozen, entry.Path)...) | ||
| } | ||
| return payload | ||
| } | ||
|
|
||
| func frozenCandidateGitDiff(t *testing.T, repo string, frozen FrozenCandidateContext, logicalPath string) []byte { | ||
| t.Helper() | ||
| isolation, cleanup, err := isolatedImmutableTreeGit(context.Background(), repo) | ||
| if err != nil { | ||
| t.Fatalf("create isolated reviewer Git environment: %v", err) | ||
| } | ||
| t.Cleanup(cleanup) | ||
| args := []string{ | ||
| "--no-pager", "diff-tree", "-p", "--binary", "--full-index", "--no-color", "--no-renames", | ||
| "--no-ext-diff", "--no-textconv", "--diff-algorithm=myers", "--no-indent-heuristic", "--unified=3", | ||
| "--ignore-submodules=none", "--src-prefix=a/", "--dst-prefix=b/", frozen.BaseTree, frozen.CandidateTree, "--", | ||
| } | ||
| if logicalPath != "" { | ||
| args = append(args, ":(literal)"+logicalPath) | ||
| } | ||
| output, err := runGitIsolated(context.Background(), repo, isolation, nil, args...) | ||
| if err != nil { | ||
| t.Fatalf("isolated git path-scoped diff %q: %v\n%s", logicalPath, err, output) | ||
| } | ||
| return output | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
t.Cleanup inside a per-path helper accumulates isolation environments.
frozenCandidateGitDiff is called once per manifest entry (and again via frozenCandidatePathDiff), each registering a cleanup that only runs at test end, so every temporary isolated Git view stays alive for the whole test. Prefer deferring cleanup within the helper after the command completes.
♻️ Release the isolation immediately
- t.Cleanup(cleanup)
+ defer cleanup()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func frozenCandidatePathDiff(t *testing.T, repo string, frozen FrozenCandidateContext) []byte { | |
| t.Helper() | |
| payload, err := diff.Bytes() | |
| if err != nil { | |
| t.Fatalf("candidate diff metadata = %#v: %v", diff, err) | |
| var payload []byte | |
| for _, entry := range frozen.ChangedPathManifest { | |
| payload = append(payload, frozenCandidateGitDiff(t, repo, frozen, entry.Path)...) | |
| } | |
| return payload | |
| } | |
| func frozenCandidateGitDiff(t *testing.T, repo string, frozen FrozenCandidateContext, logicalPath string) []byte { | |
| t.Helper() | |
| isolation, cleanup, err := isolatedImmutableTreeGit(context.Background(), repo) | |
| if err != nil { | |
| t.Fatalf("create isolated reviewer Git environment: %v", err) | |
| } | |
| t.Cleanup(cleanup) | |
| args := []string{ | |
| "--no-pager", "diff-tree", "-p", "--binary", "--full-index", "--no-color", "--no-renames", | |
| "--no-ext-diff", "--no-textconv", "--diff-algorithm=myers", "--no-indent-heuristic", "--unified=3", | |
| "--ignore-submodules=none", "--src-prefix=a/", "--dst-prefix=b/", frozen.BaseTree, frozen.CandidateTree, "--", | |
| } | |
| if logicalPath != "" { | |
| args = append(args, ":(literal)"+logicalPath) | |
| } | |
| output, err := runGitIsolated(context.Background(), repo, isolation, nil, args...) | |
| if err != nil { | |
| t.Fatalf("isolated git path-scoped diff %q: %v\n%s", logicalPath, err, output) | |
| } | |
| return output | |
| } | |
| func frozenCandidatePathDiff(t *testing.T, repo string, frozen FrozenCandidateContext) []byte { | |
| t.Helper() | |
| var payload []byte | |
| for _, entry := range frozen.ChangedPathManifest { | |
| payload = append(payload, frozenCandidateGitDiff(t, repo, frozen, entry.Path)...) | |
| } | |
| return payload | |
| } | |
| func frozenCandidateGitDiff(t *testing.T, repo string, frozen FrozenCandidateContext, logicalPath string) []byte { | |
| t.Helper() | |
| isolation, cleanup, err := isolatedImmutableTreeGit(context.Background(), repo) | |
| if err != nil { | |
| t.Fatalf("create isolated reviewer Git environment: %v", err) | |
| } | |
| defer cleanup() | |
| args := []string{ | |
| "--no-pager", "diff-tree", "-p", "--binary", "--full-index", "--no-color", "--no-renames", | |
| "--no-ext-diff", "--no-textconv", "--diff-algorithm=myers", "--no-indent-heuristic", "--unified=3", | |
| "--ignore-submodules=none", "--src-prefix=a/", "--dst-prefix=b/", frozen.BaseTree, frozen.CandidateTree, "--", | |
| } | |
| if logicalPath != "" { | |
| args = append(args, ":(literal)"+logicalPath) | |
| } | |
| output, err := runGitIsolated(context.Background(), repo, isolation, nil, args...) | |
| if err != nil { | |
| t.Fatalf("isolated git path-scoped diff %q: %v\n%s", logicalPath, err, output) | |
| } | |
| return output | |
| } |
🤖 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/reviewtransaction/frozen_candidate_context_test.go` around lines 403
- 432, Update frozenCandidateGitDiff to defer cleanup immediately after
isolatedImmutableTreeGit succeeds, and remove the per-test t.Cleanup
registration so each isolated Git environment is released when that helper
returns.
Comment on lines
+100
to
+126
| func (builder SnapshotBuilder) WithLegacyCandidateDiff(ctx context.Context, snapshot Snapshot, frozen FrozenCandidateContext) (FrozenCandidateContext, error) { | ||
| if frozen.BaseTree != snapshot.BaseTree || frozen.CandidateTree != snapshot.CandidateTree { | ||
| return FrozenCandidateContext{}, errors.New("legacy candidate transport does not match frozen trees") // refusal:by-design world-action: provider code mixed immutable contexts and must be fixed before retry | ||
| } | ||
| repo, err := builder.repositoryRoot(ctx) | ||
| if err != nil { | ||
| return FrozenCandidateContext{}, err | ||
| } | ||
| isolation, cleanup, err := isolatedImmutableTreeGit(ctx, repo) | ||
| if err != nil { | ||
| return FrozenCandidateContext{}, err | ||
| } | ||
| defer cleanup() | ||
| payload, err := runGitLimited(ctx, repo, isolation, nil, MaxFrozenCandidateDiffBytes, | ||
| "diff", "--binary", "--full-index", "--no-color", "--no-renames", "--no-ext-diff", "--no-textconv", | ||
| "--diff-algorithm=myers", "--no-indent-heuristic", "--unified=3", "--ignore-submodules=none", | ||
| "--src-prefix=a/", "--dst-prefix=b/", snapshot.BaseTree, snapshot.CandidateTree, "--") | ||
| if err != nil { | ||
| return FrozenCandidateContext{}, fmt.Errorf("render legacy frozen candidate diff: %w", err) | ||
| } | ||
| diff, err := NewFrozenCandidateDiff(payload) | ||
| if err != nil { | ||
| return FrozenCandidateContext{}, err | ||
| } | ||
| frozen.LegacyCandidateDiff = &diff | ||
| return frozen, nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Legacy path skips ValidateEvidence, unlike the native constructor.
FrozenCandidateContext validates the snapshot evidence before rendering; WithLegacyCandidateDiff only compares frozen.BaseTree/CandidateTree to the snapshot argument. A caller that hands in a mismatched (or caller-fabricated) snapshot whose trees happen to equal frozen's renders a legacy diff that was never evidence-validated. Consider validating the snapshot here too, or documenting that callers must pass the exact snapshot already used by FrozenCandidateContext.
🤖 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/reviewtransaction/frozen_candidate_context.go` around lines 100 -
126, Update WithLegacyCandidateDiff to validate the supplied snapshot evidence
using the same validation performed by the native FrozenCandidateContext
constructor before rendering the legacy diff. Keep the existing frozen-tree
consistency check, and return validation errors before calling
isolatedImmutableTreeGit or runGitLimited.
Comment on lines
+1627
to
+1643
| processEssential := map[string]struct{}{ | ||
| "COMSPEC": {}, "PATH": {}, "PATHEXT": {}, "SYSTEMDRIVE": {}, | ||
| "SYSTEMROOT": {}, "TEMP": {}, "TMP": {}, "TMPDIR": {}, "WINDIR": {}, | ||
| } | ||
| result := make([]string, 0, len(environment)+len(extra)+1) | ||
| for _, entry := range environment { | ||
| name, _, _ := strings.Cut(entry, "=") | ||
| _, remove := unsafe[name] | ||
| isolatedOverride := isolateConfig && (strings.HasPrefix(name, "GIT_CONFIG_") || strings.HasPrefix(name, "GIT_ATTR_") || name == "GIT_DIFF_OPTS") | ||
| if !remove && name != "LC_ALL" && !isolatedOverride { | ||
| canonicalName := strings.ToUpper(name) | ||
| _, remove := unsafe[canonicalName] | ||
| trace := strings.HasPrefix(canonicalName, "GIT_TRACE") | ||
| _, essential := processEssential[canonicalName] | ||
| if isolateConfig { | ||
| if essential && !strings.HasPrefix(canonicalName, "GIT_") { | ||
| result = append(result, entry) | ||
| } | ||
| continue | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Redundant GIT_ prefix check in the isolation branch.
processEssential contains no GIT_* names, so !strings.HasPrefix(canonicalName, "GIT_") can never be false when essential is true. Dropping it makes the intent (only non-Git process essentials survive isolation) clearer, or keep it with a comment stating it is a defensive guard against future additions to the map.
🤖 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/reviewtransaction/snapshot.go` around lines 1627 - 1643, The
isolation branch in the environment filtering logic redundantly checks for a
“GIT_” prefix after confirming membership in processEssential. Remove that
prefix condition so essential variables are appended directly, or retain it only
with a comment explaining its defensive purpose for future processEssential
additions.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 74 out of 75 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
internal/reviewtransaction/artifact_admission.go:151
- Artifact admission now always compares frozen BaseTree/CandidateTree to the expected subject. For legacy v1 subjects,
NewLegacyArtifactSubjectintentionally clearsBaseTree/CandidateTreeand bindsCandidateDiffSHA256instead, so this check will reject every v1 admission even whenFrozenContext.LegacyCandidateDiffmatches. Make the binding check conditional onExpectedSubject.Schema(diff SHA for v1, trees for v2).
19 tasks
2 tasks
19 tasks
2 tasks
This was referenced Jul 30, 2026
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked Issue
Closes #1689
🏷️ 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
contracts/review-integration/v2/internal/cli/internal/reviewtransaction/internal/assets/opencode/plugins/review-result-artifacts.tsinternal/components/sdd/docs/review-integration.md🧪 Test Plan
Unit Tests
go test ./... -count=1Go Format
Additional Gates
go test ./...)go run ./internal/gofmtcheck)cd e2e && ./docker-test.sh)Focused v1/v2, OpenCode plugin, frozen-tree/pathspec, batching, final-verification, and 5,000-file organic lifecycle scenarios pass. The Docker E2E workflow is left to CI.
✅ Contributor Checklist
status:approvedsize:exceptionwith rationale documentedtype:*label to this PRgo test ./...)go run ./internal/gofmtcheck)cd e2e && ./docker-test.sh)Co-Authored-Bytrailers💬 Notes for Reviewers
The maintainer approved
size:exceptionbecause this is one cross-boundary transport contract: schemas, CLI projection, native admission, managed plugin, prompts, tests, goldens, and docs must move atomically. Splitting those layers would leave intermediate commits unable to execute the negotiated lifecycle safely.Review-driven development was disabled globally during delivery, so this PR is reported as
disabled/unmanaged; no review receipt is claimed. Functional verification completed against commitd1f72d43.For production Go guard review:
Summary by CodeRabbit