fix(review): allow proofRefs and evidence to cite repository paths outside candidate diff - #1926
Conversation
|
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 (2)
📝 WalkthroughWalkthroughArtifact admission now rejects malformed or unknown repository-path references while allowing structurally valid evidence and proof references to known repository files outside the changed-path manifest. Tests cover malformed references, unmodified-file references, and absent repository paths. ChangesProof reference scope
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/reviewtransaction/artifact_admission_test.go`:
- Around line 231-245: The test coverage only exercises ProofRefs, not the
canonical.Evidence admission path for unmodified files. Add an analogous test
near TestAdmitArtifactAllowsProofRefsToUnmodifiedRepositoryFiles that sets
request.Result.Evidence to internal/secret.go:42, asserts completed admission,
and verifies canonical.Findings[0].Evidence preserves that reference.
In `@internal/reviewtransaction/artifact_admission.go`:
- Around line 219-220: ProofRefs currently allow unknown bare paths because they
reuse the permissive evidence tokenizer. In
internal/reviewtransaction/artifact_admission.go:219-220, update the
proof-reference validation around referenceOutsideScope to use a proof-specific
validator that requires a known repository path; in
internal/reviewtransaction/artifact_admission_test.go:224-225, add a negative
admission test covering a bare unknown proof path without relying on free-form
evidence expectations.
🪄 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: 2bd5ecac-bb0c-43bc-9538-411daa7b7599
📒 Files selected for processing (2)
internal/reviewtransaction/artifact_admission.gointernal/reviewtransaction/artifact_admission_test.go
| func TestAdmitArtifactAllowsProofRefsToUnmodifiedRepositoryFiles(t *testing.T) { | ||
| _, _, request := admittedArtifactFixture(t) | ||
| // Finding location remains inside changed-path manifest | ||
| request.Result.Findings[0].Location = "internal/a.go:10" | ||
| // ProofRef references an unmodified repository file outside the changed-path manifest | ||
| request.Result.Findings[0].ProofRefs = []string{"internal/secret.go:42"} | ||
|
|
||
| canonical, admission, err := AdmitArtifact(request) | ||
| if err != nil || admission.Decision != ArtifactAdmissionCompleted { | ||
| t.Fatalf("AdmitArtifact() = %q, %v; want completed when proof_ref references valid repository path", admission.Decision, err) | ||
| } | ||
| if len(canonical.Findings[0].ProofRefs) != 1 || canonical.Findings[0].ProofRefs[0] != "internal/secret.go:42" { | ||
| t.Fatalf("AdmitArtifact() proofRefs = %v, want [internal/secret.go:42]", canonical.Findings[0].ProofRefs) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add end-to-end coverage for unmodified-file evidence.
The new regression covers ProofRefs, but the separate canonical.Evidence admission path is not tested with a valid unmodified repository file. Add an analogous case using internal/secret.go:42 in request.Result.Evidence and assert completed admission plus canonical evidence preservation.
🤖 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_test.go` around lines 231 -
245, The test coverage only exercises ProofRefs, not the canonical.Evidence
admission path for unmodified files. Add an analogous test near
TestAdmitArtifactAllowsProofRefsToUnmodifiedRepositoryFiles that sets
request.Result.Evidence to internal/secret.go:42, asserts completed admission,
and verifies canonical.Findings[0].Evidence preserves that reference.
e93d801 to
7b9c04a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/reviewtransaction/artifact_admission_test.go (1)
207-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the
allowed-versus-repositorydistinction in this test.Passing
repositoryas both arguments makes every known repository path appear in scope, so the renamed “outside scope” cases no longer exercisereferenceOutsideScope’s changed-path filtering. Restore a distinctallowedsubset, or remove this helper test if the function is intentionally retired.🤖 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_test.go` around lines 207 - 225, Update the referenceOutsideScope test to pass a distinct allowed-path subset as its first argument and the full repository as its second, so the outside cases exercise changed-path filtering; preserve the existing expectations. If referenceOutsideScope is intentionally retired, remove this helper test instead.
🤖 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/reviewtransaction/artifact_admission.go`:
- Around line 200-201: Update the failure diagnostics in the proof-reference
validation branches, including the branch guarded by proofReferenceIsMalformed,
to describe malformed repository references rather than paths outside the frozen
candidate. Preserve the existing ArtifactAdmissionOutOfScope result and fail
flow while changing both affected messages consistently.
- Around line 219-220: Require explicit proof references in the admission flow
to match a canonical path present in the frozen repository manifest, not merely
pass malformed-path checks. Update proofReferenceIsMalformed or split the
validators so path-like tokens with known == false are rejected, and add a
negative admission test covering not-in-repository.go:42. Apply these changes at
internal/reviewtransaction/artifact_admission.go:219-220 and 440-449, and
internal/reviewtransaction/artifact_admission_test.go:94-96.
---
Outside diff comments:
In `@internal/reviewtransaction/artifact_admission_test.go`:
- Around line 207-225: Update the referenceOutsideScope test to pass a distinct
allowed-path subset as its first argument and the full repository as its second,
so the outside cases exercise changed-path filtering; preserve the existing
expectations. If referenceOutsideScope is intentionally retired, remove this
helper test instead.
🪄 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: b7e7e179-3c9b-4f1d-9c43-beffe5232cc8
📒 Files selected for processing (2)
internal/reviewtransaction/artifact_admission.gointernal/reviewtransaction/artifact_admission_test.go
| if proofReferenceIsMalformed(evidence, repository) { | ||
| return fail(ArtifactAdmissionOutOfScope, "reviewer evidence references a path outside the frozen candidate") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the diagnostics to match the new validation.
These branches now reject malformed repository references, not references outside the changed-path manifest. Messages such as “outside the frozen candidate” are misleading for inputs like ./internal/a.go:3; report a malformed repository reference instead.
Also applies to: 219-220
🤖 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 200 - 201,
Update the failure diagnostics in the proof-reference validation branches,
including the branch guarded by proofReferenceIsMalformed, to describe malformed
repository references rather than paths outside the frozen candidate. Preserve
the existing ArtifactAdmissionOutOfScope result and fail flow while changing
both affected messages consistently.
…tside candidate diff (Gentleman-Programming#1920)
7b9c04a to
b2bf8c0
Compare
dnlrsls
left a comment
dnlrsls
left a comment
There was a problem hiding this comment.
Two changes are needed before merge:
-
internal/reviewtransaction/artifact_admission.go:329—referenceOutsideScopeis no longer called by production code after this change. The dead-code ratchet reports it as a new unreachable function, which is why the Unit Tests job is failing. Please remove the obsolete helper and its dedicated test, or restore a live production use. -
internal/reviewtransaction/artifact_admission.go:201,220— rejected malformed or repository-absent references are still described as being “outside the frozen candidate.” Under this PR, repository paths outside the candidate are valid. Please update both diagnostics to identify malformed references or paths absent from the frozen repository manifest, and assert the diagnostic in tests.
Targeted admission tests pass, and the prior unknown-path and evidence-coverage concerns are resolved at the current head.
dnlrsls
left a comment
dnlrsls
left a comment
There was a problem hiding this comment.
Required Unit Tests fail because the patch makes referenceOutsideScope unreachable under the dead-code ratchet. Please remove or legitimately reuse that helper, restore the missing Linked Issue, PR Type, Test Plan, and Contributor Checklist sections from the required template, and rerun CI. The new repository-scope admission tests otherwise match #1920.
|
This candidate is superseded by main commit |
3 participants
Summary
Closes #1920
Reviewer evidence and
proof_refsentries were being validated against the changed-path manifest (allowed) instead of the full frozen repository path manifest (repository). A reviewer is permitted to cite any immutable repository file as contextual proof for a finding; only the finding location itself must reside inside the changed-path manifest. Validating proof references againstallowedcaused valid results — where a reliability or resilience finding referenced an unmodified contract or interface file — to be rejected without_of_scope, quarantined byreview abandon, and then irrecoverable viareview repair --preflight(eligible_candidates: 0), forcing a new budget spend in violation of the exactly-once budget rule.Changes
internal/reviewtransaction/artifact_admission.go: inAdmitArtifact, evidence andProofRefsloops now callreferenceOutsideScope(value, repository, repository)instead ofreferenceOutsideScope(value, allowed, repository). Finding location validation againstwantPaths(changed-path manifest) is unchanged.internal/reviewtransaction/artifact_admission_test.go:TestAdmitArtifactRequiresCompletedBoundInScopeInspectionproof/evidence out-of-scope cases to use paths absent from the repository manifest (the actual admission boundary).TestReferenceOutsideScopeRecognizesOnlyStructuredRepositoryPathsto callreferenceOutsideScopewith repository-as-allowed, matching the new semantics.TestAdmitArtifactAllowsProofRefsToUnmodifiedRepositoryFiles— RED-first regression: a finding whose location is inside the candidate diff but whose proof_ref cites a valid unmodified repository file must be admitted ascompleted.Verification
Summary by CodeRabbit