fix(review): reject results when candidate input is unreadable or denied by filesystem - #1939
Conversation
…ied by filesystem Reviewer sessions executing in constrained or permission-denied sandboxes were reporting read denials in evidence text without failing admission, allowing false-clean zero-finding approvals. - Expand evidenceReportsUnavailableInspection to detect filesystem access denial phrases - Add unit tests verifying read access denials in evidence are rejected as incomplete Closes Gentleman-Programming#1867
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesArtifact admission validation
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/reviewtransaction/artifact_admission_test.go`:
- Around line 80-87: Expand the table-driven cases in the artifact admission
tests around the existing “read denied by filesystem” and “file access denied”
entries to cover every newly supported denial phrase, including “read denied,”
“cannot read manifest,” and “filesystem access denied.” Keep each variant
asserting empty Findings and ArtifactAdmissionIncomplete, or consolidate
production matching to a documented canonical set with tests for each supported
form.
🪄 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: c4b98ea4-d94e-4d4b-ae33-4ae0c2de7d28
📒 Files selected for processing (2)
internal/reviewtransaction/artifact_admission.gointernal/reviewtransaction/artifact_admission_test.go
| {name: "read denied by filesystem", mutate: func(r *ArtifactAdmissionRequest) { | ||
| r.Result.Findings = []Finding{} | ||
| r.Result.Evidence = []string{"The immutable diff and manifest were denied by its filesystem environment."} | ||
| }, decision: ArtifactAdmissionIncomplete}, | ||
| {name: "file access denied", mutate: func(r *ArtifactAdmissionRequest) { | ||
| r.Result.Findings = []Finding{} | ||
| r.Result.Evidence = []string{"Cannot read diff: file access denied."} | ||
| }, decision: ArtifactAdmissionIncomplete}, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Cover the remaining newly supported denial phrases.
These tests validate two variants, but the production change adds several independent phrases (read denied, cannot read manifest, filesystem access denied, and others). Add table entries for each phrase, or consolidate the matcher into a smaller canonical set, so future regressions cannot silently remove coverage for individual variants.
🤖 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 80 - 87,
Expand the table-driven cases in the artifact admission tests around the
existing “read denied by filesystem” and “file access denied” entries to cover
every newly supported denial phrase, including “read denied,” “cannot read
manifest,” and “filesystem access denied.” Keep each variant asserting empty
Findings and ArtifactAdmissionIncomplete, or consolidate production matching to
a documented canonical set with tests for each supported form.
dnlrsls
left a comment
dnlrsls
left a comment
There was a problem hiding this comment.
This patch correctly covers the added unreadable/denied evidence phrases, but it closes #1867 while implementing only part of the approved scope. #1867 also requires atomic truncated-capture handling, typed recovery, and integration coverage. Please either relink this narrow patch to a separately approved issue or implement and test the remaining #1867 acceptance criteria. Also complete the missing required PR-template entries.
|
Please stop implementation churn until #1867 has an approved design. The current patch covers unreadable-input phrases, but the issue also requires atomic truncated capture, typed recovery, and deterministic handling of mutable test and configuration state. Define that complete failure and recovery contract in the issue, regain |
3 participants
🔗 Linked Issue
Closes #1867
🏷️ PR Type
type:bug— Bug fix (non-breaking change that fixes an issue)📝 Summary
Reviewer processes running under permission-restricted sandboxes (such as Windows sandbox environments) could report file/diff read access denials in their evidence strings while returning 0 findings. The admission check allowed these results as valid clean approvals because
evidenceReportsUnavailableInspectiondid not recognize filesystem permission denial phrases.This PR expands
evidenceReportsUnavailableInspectionto recognize filesystem permission denial patterns and ensures unreadable candidate inputs are rejected as incomplete admission failures.📂 Changes
internal/reviewtransaction/artifact_admission.goevidenceReportsUnavailableInspectionphrase detection to cover filesystem access denialsinternal/reviewtransaction/artifact_admission_test.go🧪 Test Plan
go test ./internal/reviewtransaction/...)go run ./internal/gofmtcheck)✅ Contributor Checklist
status:approvedtype:*label to this PRgo test ./...)Co-Authored-BytrailersSummary by CodeRabbit