Skip to content

fix(review): bind capture evidence to reviewed target#1796

Open
doblas wants to merge 1 commit into
Gentleman-Programming:mainfrom
doblas:fix/review-evidence-contract
Open

fix(review): bind capture evidence to reviewed target#1796
doblas wants to merge 1 commit into
Gentleman-Programming:mainfrom
doblas:fix/review-evidence-contract

Conversation

@doblas

@doblas doblas commented Jul 23, 2026

Copy link
Copy Markdown

Linked Issue

Closes #1689
Closes #1699

Related to #1775 and #1788. Those issues are not yet marked status:approved, so this PR does not auto-close them.


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 only
  • type:refactor - Code refactoring (no functional changes)
  • type:chore - Build, CI, or tooling changes
  • type:breaking-change - Breaking change (fix or feature that changes existing behavior)

Summary

  • Bind provider-owned subject context and frozen candidate paths to OpenCode reviewer capture.
  • Publish and validate the verification-evidence contract while accepting optional finding IDs at the native ownership boundary.
  • Reject captured-result shortcuts after reviewer execution and preserve replayable failure evidence.

Changes

File / Area What Changed
contracts/review-integration/v1/ Added public verification-evidence schema and updated negotiated contract fixtures.
internal/assets/opencode/plugins/review-result-artifacts.ts Injected provider-owned frozen context and enriched captured reviewer results.
internal/cli/ Bound review transitions, capture evidence, and schema validation to native authority.
internal/reviewtransaction/ Aligned optional finding IDs with native candidate-causal ownership.
docs/ and shared review contract Documented the updated trust and evidence boundaries.

Test Plan

  • Causally affected 13-test Go suite passed across internal/assets, internal/cli, internal/reviewtransaction, and internal/components/sdd with -count=1.
  • shellcheck scripts/test-review-contract-package.sh passed through koalaman/shellcheck:stable.
  • Native 4R review completed with no findings.
  • Native pre-commit, pre-push, and pre-pr receipt gates allowed the exact reviewed candidate.
  • Full repository unit tests: CI pending. Local broad package runs were host-blocked by an unrelated durability timeout and Windows symlink privileges.
  • Go format: CI pending.
  • E2E tests: CI pending. No E2E runtime surface changed.

Automated Checks

All repository checks remain authoritative and are expected to run in CI.


Contributor Checklist


Notes for Reviewers

Native review lineage: review-27dc64136fee676e.

The content-bound receipt was preserved through delivery. origin/main advanced after review, and native pre-pr validation proved a compatible, path-disjoint base advance with identical delivered patch identity.

Summary by CodeRabbit

  • New Features

    • Added final verification evidence support, including passed/failed outcomes and named checks.
    • Added reviewer task bindings to preserve review context across transitions and restarts.
    • Improved result capture with provider-derived inspection paths and subject verification.
    • Added canonical finding ID validation and lens consistency checks.
  • Bug Fixes

    • Prevented stale captured results from being reused outside the active review window.
    • Improved finalization behavior by automatically discovering captured verification evidence.
  • Documentation

    • Documented evidence capture, finalization workflows, and updated contract inventory counts.

Ensure provider-owned subject context and strict evidence schemas survive capture while rejecting post-review substitutions.
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds a versioned verification-evidence contract, propagates frozen reviewer task bindings through START and STATUS, enriches managed reviewer results with provider-owned metadata, and canonicalizes lens results before artifact admission and replay.

Changes

Review capture contracts

Layer / File(s) Summary
Verification evidence contracts and validation
contracts/review-integration/v1/..., internal/cli/review_schema.go, internal/cli/review_artifact.go, docs/..., scripts/test-review-contract-package.sh
Adds the gentle-ai.review-verification-evidence/v1 schema, fixture, strict parser, capability advertisement, packaging checks, and related workflow documentation.
Reviewer task binding propagation
internal/cli/review_start_contract.go, internal/cli/review_next_transition.go, internal/cli/review_status_contract.go, contracts/review-integration/v1/...
Adds per-lens reviewer task bindings to negotiated START and capture-result inputs, with rendering, validation, fixture coverage, and restart compatibility checks.
Evidence-aware finalize handling
internal/cli/review_facade.go, internal/cli/review_operation_contract.go, internal/cli/review_next_transition_test.go
Discovers captured verification evidence during finalize, derives verification failure from its outcome, rejects stale captured-result selectors, and verifies terminal receipt replay.
Managed reviewer result enrichment
internal/assets/opencode/plugins/review-result-artifacts.ts, internal/assets/review_result_artifacts_behavior_test.go, internal/assets/skills/_shared/review-ledger-contract.md
Retains bound preflight context, validates changed-path manifests, enriches reviewer-only output with provider-owned capture metadata, and tests recovery after capture failures.
Canonical lens-result admission
internal/reviewtransaction/artifact_admission.go, internal/cli/review_artifact.go, internal/cli/review_artifact_test.go
Introduces validated canonical lens results, enforces lens-bound finding IDs, and uses canonical results for artifact admission and replay.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related issues

Possibly related PRs

Suggested labels: type:bug

Suggested reviewers: alan-thegentleman

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 2.04% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR implements provider-owned capture enrichment and deterministic lens-bound finding IDs with matching validation and tests for #1689 and #1699.
Out of Scope Changes check ✅ Passed The added schemas, fixtures, docs, and CLI plumbing all support the review-capture and admission changes, with no clear unrelated scope.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: binding capture evidence to the reviewed target.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@doblas

doblas commented Jul 23, 2026

Copy link
Copy Markdown
Author

Maintainer action requested: please apply type:bug and size:exception. The PR is one reviewed cross-boundary contract unit across public schemas, native admission, provider capture, tests, and documentation; splitting it would leave intermediate commits with mismatched contracts.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_schema.go`:
- Around line 74-79: Align the schema and native parsing in the final
verification check parser around the check structure and validation logic: allow
non-empty padded name and command values consistently with the schema, while
still rejecting whitespace-only values. Track whether command is omitted versus
explicitly set to an empty string (for example via a pointer) and reject the
latter, then add boundary tests covering padded and empty/omitted command cases.
🪄 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: 25052b03-993d-401d-a33c-0063f952b984

📥 Commits

Reviewing files that changed from the base of the PR and between 0d216f2 and 1e485a8.

📒 Files selected for processing (30)
  • contracts/review-integration/v1/fixtures/capabilities-v1.4.fixture.json
  • contracts/review-integration/v1/fixtures/start-v2.fixture.json
  • contracts/review-integration/v1/fixtures/status-v2.fixture.json
  • contracts/review-integration/v1/fixtures/verification-evidence.fixture.json
  • contracts/review-integration/v1/schemas/start-v2.schema.json
  • contracts/review-integration/v1/schemas/status-v2.schema.json
  • contracts/review-integration/v1/schemas/verification-evidence.schema.json
  • docs/review-authority-threat-model.md
  • docs/review-integration.md
  • internal/assets/assets_test.go
  • internal/assets/opencode/plugins/review-result-artifacts.ts
  • internal/assets/review_result_artifacts_behavior_test.go
  • internal/assets/skills/_shared/review-ledger-contract.md
  • internal/cli/review_artifact.go
  • internal/cli/review_artifact_test.go
  • internal/cli/review_capabilities.go
  • internal/cli/review_capabilities_test.go
  • internal/cli/review_facade.go
  • internal/cli/review_next_transition.go
  • internal/cli/review_next_transition_test.go
  • internal/cli/review_operation_contract.go
  • internal/cli/review_schema.go
  • internal/cli/review_schema_test.go
  • internal/cli/review_start_context_test.go
  • internal/cli/review_start_contract.go
  • internal/cli/review_status_contract.go
  • internal/components/sdd/review_ledger_contract_test.go
  • internal/reviewtransaction/artifact_admission.go
  • internal/reviewtransaction/artifact_admission_test.go
  • scripts/test-review-contract-package.sh

Comment on lines +74 to +79
if strings.TrimSpace(check.Name) == "" || strings.TrimSpace(check.Name) != check.Name ||
(check.Status != reviewVerificationPassed && check.Status != reviewVerificationFailed) || len(check.Evidence) == 0 {
return evidence, nil, fmt.Errorf("invalid final verification check %d", index+1)
}
if check.Command != "" && (strings.TrimSpace(check.Command) == "" || strings.TrimSpace(check.Command) != check.Command) {
return evidence, nil, fmt.Errorf("invalid final verification check %d command", index+1)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Align the advertised schema and native parser.

The schema accepts padded name/command values (\S only requires one non-whitespace character), but these lines reject them. Conversely, command: "" violates the schema yet is accepted because an omitted command and an empty command decode identically. Track command presence (for example, with *string) and make the schema/parser whitespace rules identical; add boundary tests for both cases.

🤖 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_schema.go` around lines 74 - 79, Align the schema and
native parsing in the final verification check parser around the check structure
and validation logic: allow non-empty padded name and command values
consistently with the schema, while still rejecting whitespace-only values.
Track whether command is omitted versus explicitly set to an empty string (for
example via a pointer) and reject the latter, then add boundary tests covering
padded and empty/omitted command cases.

@ftorga

ftorga commented Jul 24, 2026

Copy link
Copy Markdown

Thanks for the substantial work here. PR #1797 is the smaller, already-clean carrier for #1699, at 4b2b559e053ba6b83727ab8fbedbed4d42fb9fda with 203 additions and 24 deletions.

To avoid overlapping ownership, I recommend keeping #1797 as the canonical carrier for #1699 and narrowing #1796 to its unique #1689 work, including removing #1699 from its closing references and duplicated #1699 scope. Could maintainers please confirm the canonical carrier before further changes proceed?

@ftorga

ftorga commented Jul 25, 2026

Copy link
Copy Markdown

Behavior-first validation for #1689

I revalidated v2.2.0-rc.1 at tag 45e53bda23663abdbe2392214213c1283c28f78b against the current #1801 head b37aa8e45d24cbc0abae17569ba1116e3e0bfe7e. Running the OpenCode plugin with Bun observed this stdin at capture-result: {"findings":[],"evidence":["reviewed candidate.go"]}.

The plugin passes the raw result without adding top-level subject_hash or the required inspection envelope. Admission therefore fails closed with reviewer artifact admission incomplete: reviewer result omitted the provider-owned artifact subject. An already enriched artifact completes capture, persistence, reopen, and finalize successfully.

Would the maintainer prefer to keep #1796 as the focused adapter/plugin fix, or integrate that change into the #1801 branch?

@edwinsaavedran

Copy link
Copy Markdown

Proposed safe rebase boundary against Refresh 7

This PR is partially superseded, but its current #1689 enrichment must not merge unchanged.

The corresponding acceptance correction is recorded in #1689.

Already equivalent in 1b1a8676

Trust-boundary defect in the proposed enrichment

enrichedReviewerResult derives:

{"inspection":{"status":"completed","paths":["<provider manifest>"]}}

whenever the hook received a strict {findings,evidence} response and retained preflight. This proves that the adapter presented the frozen scope; it does not prove that the reviewer inspected it. The adapter therefore manufactures the exact completion fact that native admission uses to prevent the false-clean class reported in #1555/#1867.

The ownership split should be:

  • provider/native CLI: subject hash, slot, target, ordered manifest and scope identity;
  • adapter/runtime: scope_presented and provider-context digest;
  • reviewer or an observable read-runtime: structured inspection_outcome=completed|scope_unavailable;
  • native admission: combine and validate those facts, reject missing/unavailable inspection, canonicalize and persist.

The adapter may add provider paths after a valid completed attestation. It must not synthesize completed.

A reviewer attestation is not cryptographic proof of cognitive attention. The enforceable goal is narrower: no layer fabricates completion, explicit unavailability cannot canonicalize, and accepted evidence is candidate-specific and scope-consistent.

Still required

  1. fix(opencode): enrich reviewer output with provider-owned capture subject #1689: binding retention, provider-fact enrichment, same-lineage replay, restart/version coherence, and a non-forgeable inspection outcome.
  2. fix(review): reject captured results after reviewing #1788: reject capturedResults outside reviewing; current guard covers paths/artifacts but omits that boolean.

Different design, not equivalence

This PR also publishes gentle-ai.review-verification-evidence/v1; Refresh 7 serves bounded raw evidence. That contract decision should be split rather than hidden inside the adapter fix.

Minimal rebase

  • remove fix(review): align optional finding IDs with candidate-causal admission #1699 scope;
  • split/drop the verification-evidence schema change;
  • retain provider binding/context and preserved-result replay;
  • replace synthetic completion with reviewer/runtime-owned attestation;
  • add negative tests for missing attestation, scope_unavailable, model-supplied hash/path overrides, stale OpenCode process, capture failure and exact replay;
  • run one real OpenCode oracle proving no second lens/budget is consumed.

This keeps native admission fail-closed and produces a reviewable #1689/#1788 carrier.

@aparragithub

Copy link
Copy Markdown
Contributor

I reproduced the remaining post-enrichment deadlock with this PR active: all preserved payloads already contained subject_hash and completed ordered inspection, but the emitted rinc1_* handles had no consumable native replay path.

A tested follow-up is available at https://github.com/aparragithub/gentle-ai/tree/fix/1796-preserved-result-replay (f5de122f). It lets review capture-result --input rinc1_<digest> resolve and re-admit the unchanged provider-bound payload safely. The patch was proven on the real blocked lineage without rerunning reviewers, and includes bounded cross-platform path/TOCTOU protections.

I also requested status:approved on #1689 so a policy-compliant linked PR can be opened.

@aparragithub

Copy link
Copy Markdown
Contributor

@doblas, I opened a focused draft PR against your #1796 branch:

doblas#1

It contains the clean preserved-result replay commit 5e93b914. The isolated rinc1_* -> capture-result -> finalize path passes with the locally installed binary.

Please review whether you prefer to merge the draft or cherry-pick the commit into #1796.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(review): align optional finding IDs with candidate-causal admission fix(opencode): enrich reviewer output with provider-owned capture subject

4 participants