fix(review): surface reviewer admission diagnostics - #2082
Conversation
📝 WalkthroughWalkthroughThe change adds typed finding-location validation and structured artifact admission diagnostics. CLI and OpenCode recovery handling now preserve validated finding details while filtering unsafe or unparseable native error content. ChangesAdmission diagnostics
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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/assets/opencode/plugins/review-result-artifacts.ts`:
- Around line 289-322: Update the safeLocation validation in admissionRejection
to scan the entire parsed.location for ".." path segments across both slash and
colon separators, rather than only the substring before the first colon.
Preserve the existing location safety checks and add a regression test alongside
TestReviewPluginSurfacesStructuredLocationRecoveryDiagnostic covering a
colon-embedded traversal location such as "good:../../secret:1".
In `@internal/reviewtransaction/artifact_admission.go`:
- Around line 282-289: In the location error handling of the artifact admission
flow, check the boolean result from errors.As before accessing
typedLocationErr.Reason. Update the parseFindingLocation error branch to safely
handle a failed type match, following the established pattern in
NewArtifactLocationAdmissionError, while preserving the existing diagnostic and
return behavior for *FindingLocationError values.
🪄 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: b7ab24f9-7800-46a3-bc3c-5b16690dbfd1
📒 Files selected for processing (9)
internal/assets/opencode/plugins/review-result-artifacts.tsinternal/assets/review_plugin_recovery_test.gointernal/cli/review_artifact.gointernal/cli/review_artifact_test.gointernal/reviewtransaction/artifact_admission.gointernal/reviewtransaction/artifact_admission_test.gointernal/reviewtransaction/compact.gointernal/reviewtransaction/compact_store_test.gointernal/reviewtransaction/snapshot.go
| const ADMISSION_DIAGNOSTIC = /; admission_diagnostic=(\{[^\r\n]{1,1024}\})$/ | ||
| const ADMISSION_DIAGNOSTIC_REASONS = new Set([ | ||
| "expected_path_and_line", "line_suffix_not_integer", "line_must_be_positive", | ||
| "path_must_be_repository_relative", "path_must_be_canonical", "line_not_changed_by_candidate", | ||
| ]) | ||
|
|
||
| type AdmissionDiagnostic = { | ||
| code: "invalid_finding_location" | "candidate_causality_unproven" | ||
| finding_id: string | ||
| location: string | ||
| reason: string | ||
| } | ||
|
|
||
| function admissionRejection(cause: unknown): string | undefined { | ||
| const match = ADMISSION_REJECTION.exec(errorMessage(cause)) | ||
| return match ? match[1] : undefined | ||
| function admissionRejection(cause: unknown): { decision: string, diagnostic?: AdmissionDiagnostic } | undefined { | ||
| const message = errorMessage(cause) | ||
| const match = ADMISSION_REJECTION.exec(message) | ||
| if (!match) return undefined | ||
| const detail = ADMISSION_DIAGNOSTIC.exec(message) | ||
| if (!detail) return { decision: match[1] } | ||
| try { | ||
| const parsed = JSON.parse(detail[1]) as Partial<AdmissionDiagnostic> | ||
| const safeLocation = typeof parsed.location === "string" && parsed.location.length <= 256 && | ||
| !/[\u0000-\u001f\u007f\\]/.test(parsed.location) && !/^(?:[A-Za-z]:[\\/]|[\\/])/.test(parsed.location) && | ||
| !parsed.location.split(":", 1)[0].split("/").includes("..") | ||
| const safeID = typeof parsed.finding_id === "string" && /^R[1-4]-[A-Za-z0-9][A-Za-z0-9._-]{0,127}$/.test(parsed.finding_id) | ||
| const safeCode = parsed.code === "invalid_finding_location" || parsed.code === "candidate_causality_unproven" | ||
| const safeReason = typeof parsed.reason === "string" && ADMISSION_DIAGNOSTIC_REASONS.has(parsed.reason) | ||
| return safeLocation && safeID && safeCode && safeReason | ||
| ? { decision: match[1], diagnostic: parsed as AdmissionDiagnostic } | ||
| : { decision: match[1] } | ||
| } catch { | ||
| return { decision: match[1] } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fix the path-traversal filter: it only checks the segment before the first colon.
parsed.location is fully attacker/reviewer-controlled text: it is exactly the raw finding.Location that failed canonical validation, so it can contain any number of colons. The traversal check at Line 312 is !parsed.location.split(":", 1)[0].split("/").includes(".."), which only inspects the substring before the first colon.
A location such as "good:../../secret:1" bypasses this check: split(":", 1)[0] yields "good", which contains no ".." segment, so safeLocation evaluates to true even though the full string (displayed verbatim via JSON.stringify on Line 330) still carries the ../../secret traversal component. This defeats the "opaque path filtering" goal for this diagnostic path.
Scan the whole string across both separators, not just the pre-first-colon segment.
🛡️ Proposed fix to scan the full location for traversal segments
const parsed = JSON.parse(detail[1]) as Partial<AdmissionDiagnostic>
const safeLocation = typeof parsed.location === "string" && parsed.location.length <= 256 &&
!/[\u0000-\u001f\u007f\\]/.test(parsed.location) && !/^(?:[A-Za-z]:[\\/]|[\\/])/.test(parsed.location) &&
- !parsed.location.split(":", 1)[0].split("/").includes("..")
+ !parsed.location.split(/[/:]/).includes("..")Consider adding a regression test in internal/assets/review_plugin_recovery_test.go alongside TestReviewPluginSurfacesStructuredLocationRecoveryDiagnostic for a location containing a colon-embedded .. segment (for example "good:../../secret:1"), to lock in the fix.
📝 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.
| const ADMISSION_DIAGNOSTIC = /; admission_diagnostic=(\{[^\r\n]{1,1024}\})$/ | |
| const ADMISSION_DIAGNOSTIC_REASONS = new Set([ | |
| "expected_path_and_line", "line_suffix_not_integer", "line_must_be_positive", | |
| "path_must_be_repository_relative", "path_must_be_canonical", "line_not_changed_by_candidate", | |
| ]) | |
| type AdmissionDiagnostic = { | |
| code: "invalid_finding_location" | "candidate_causality_unproven" | |
| finding_id: string | |
| location: string | |
| reason: string | |
| } | |
| function admissionRejection(cause: unknown): string | undefined { | |
| const match = ADMISSION_REJECTION.exec(errorMessage(cause)) | |
| return match ? match[1] : undefined | |
| function admissionRejection(cause: unknown): { decision: string, diagnostic?: AdmissionDiagnostic } | undefined { | |
| const message = errorMessage(cause) | |
| const match = ADMISSION_REJECTION.exec(message) | |
| if (!match) return undefined | |
| const detail = ADMISSION_DIAGNOSTIC.exec(message) | |
| if (!detail) return { decision: match[1] } | |
| try { | |
| const parsed = JSON.parse(detail[1]) as Partial<AdmissionDiagnostic> | |
| const safeLocation = typeof parsed.location === "string" && parsed.location.length <= 256 && | |
| !/[\u0000-\u001f\u007f\\]/.test(parsed.location) && !/^(?:[A-Za-z]:[\\/]|[\\/])/.test(parsed.location) && | |
| !parsed.location.split(":", 1)[0].split("/").includes("..") | |
| const safeID = typeof parsed.finding_id === "string" && /^R[1-4]-[A-Za-z0-9][A-Za-z0-9._-]{0,127}$/.test(parsed.finding_id) | |
| const safeCode = parsed.code === "invalid_finding_location" || parsed.code === "candidate_causality_unproven" | |
| const safeReason = typeof parsed.reason === "string" && ADMISSION_DIAGNOSTIC_REASONS.has(parsed.reason) | |
| return safeLocation && safeID && safeCode && safeReason | |
| ? { decision: match[1], diagnostic: parsed as AdmissionDiagnostic } | |
| : { decision: match[1] } | |
| } catch { | |
| return { decision: match[1] } | |
| } | |
| } | |
| const ADMISSION_DIAGNOSTIC = /; admission_diagnostic=(\{[^\r\n]{1,1024}\})$/ | |
| const ADMISSION_DIAGNOSTIC_REASONS = new Set([ | |
| "expected_path_and_line", "line_suffix_not_integer", "line_must_be_positive", | |
| "path_must_be_repository_relative", "path_must_be_canonical", "line_not_changed_by_candidate", | |
| ]) | |
| type AdmissionDiagnostic = { | |
| code: "invalid_finding_location" | "candidate_causality_unproven" | |
| finding_id: string | |
| location: string | |
| reason: string | |
| } | |
| function admissionRejection(cause: unknown): { decision: string, diagnostic?: AdmissionDiagnostic } | undefined { | |
| const message = errorMessage(cause) | |
| const match = ADMISSION_REJECTION.exec(message) | |
| if (!match) return undefined | |
| const detail = ADMISSION_DIAGNOSTIC.exec(message) | |
| if (!detail) return { decision: match[1] } | |
| try { | |
| const parsed = JSON.parse(detail[1]) as Partial<AdmissionDiagnostic> | |
| const safeLocation = typeof parsed.location === "string" && parsed.location.length <= 256 && | |
| !/[\u0000-\u001f\u007f\\]/.test(parsed.location) && !/^(?:[A-Za-z]:[\\/]|[\\/])/.test(parsed.location) && | |
| !parsed.location.split(/[/:]/).includes("..") | |
| const safeID = typeof parsed.finding_id === "string" && /^R[1-4]-[A-Za-z0-9][A-Za-z0-9._-]{0,127}$/.test(parsed.finding_id) | |
| const safeCode = parsed.code === "invalid_finding_location" || parsed.code === "candidate_causality_unproven" | |
| const safeReason = typeof parsed.reason === "string" && ADMISSION_DIAGNOSTIC_REASONS.has(parsed.reason) | |
| return safeLocation && safeID && safeCode && safeReason | |
| ? { decision: match[1], diagnostic: parsed as AdmissionDiagnostic } | |
| : { decision: match[1] } | |
| } catch { | |
| return { decision: match[1] } | |
| } | |
| } |
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 304-304: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 306-306: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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/opencode/plugins/review-result-artifacts.ts` around lines 289
- 322, Update the safeLocation validation in admissionRejection to scan the
entire parsed.location for ".." path segments across both slash and colon
separators, rather than only the substring before the first colon. Preserve the
existing location safety checks and add a regression test alongside
TestReviewPluginSurfacesStructuredLocationRecoveryDiagnostic covering a
colon-embedded traversal location such as "good:../../secret:1".
| logicalPath, _, locationErr := parseFindingLocation(finding.Location) | ||
| if locationErr != nil { | ||
| var typedLocationErr *FindingLocationError | ||
| errors.As(locationErr, &typedLocationErr) | ||
| return failFinding(ArtifactAdmissionOutOfScope, "reviewer finding location is invalid", | ||
| findingAdmissionDiagnostic("invalid_finding_location", finding.ID, finding.Location, string(typedLocationErr.Reason)), locationErr) | ||
| } | ||
| if stringIndex(wantPaths, logicalPath) < 0 { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Check the errors.As result before dereferencing typedLocationErr.
errors.As(locationErr, &typedLocationErr) on Line 285 discards its boolean result. If errors.As returns false, typedLocationErr stays nil, and typedLocationErr.Reason on Line 287 panics with a nil-pointer dereference.
Today parseFindingLocation only ever returns *FindingLocationError, so this path is not currently reachable. But this call site does not enforce that invariant, and it diverges from NewArtifactLocationAdmissionError at Line 114, which checks the same pattern correctly. Guard the dereference the same way there.
🛡️ Proposed fix to guard the type assertion
logicalPath, _, locationErr := parseFindingLocation(finding.Location)
if locationErr != nil {
var typedLocationErr *FindingLocationError
- errors.As(locationErr, &typedLocationErr)
- return failFinding(ArtifactAdmissionOutOfScope, "reviewer finding location is invalid",
- findingAdmissionDiagnostic("invalid_finding_location", finding.ID, finding.Location, string(typedLocationErr.Reason)), locationErr)
+ reason := "invalid_location"
+ if errors.As(locationErr, &typedLocationErr) {
+ reason = string(typedLocationErr.Reason)
+ }
+ return failFinding(ArtifactAdmissionOutOfScope, "reviewer finding location is invalid",
+ findingAdmissionDiagnostic("invalid_finding_location", finding.ID, finding.Location, reason), locationErr)
}📝 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.
| logicalPath, _, locationErr := parseFindingLocation(finding.Location) | |
| if locationErr != nil { | |
| var typedLocationErr *FindingLocationError | |
| errors.As(locationErr, &typedLocationErr) | |
| return failFinding(ArtifactAdmissionOutOfScope, "reviewer finding location is invalid", | |
| findingAdmissionDiagnostic("invalid_finding_location", finding.ID, finding.Location, string(typedLocationErr.Reason)), locationErr) | |
| } | |
| if stringIndex(wantPaths, logicalPath) < 0 { | |
| logicalPath, _, locationErr := parseFindingLocation(finding.Location) | |
| if locationErr != nil { | |
| var typedLocationErr *FindingLocationError | |
| reason := "invalid_location" | |
| if errors.As(locationErr, &typedLocationErr) { | |
| reason = string(typedLocationErr.Reason) | |
| } | |
| return failFinding(ArtifactAdmissionOutOfScope, "reviewer finding location is invalid", | |
| findingAdmissionDiagnostic("invalid_finding_location", finding.ID, finding.Location, reason), locationErr) | |
| } | |
| if stringIndex(wantPaths, logicalPath) < 0 { |
🤖 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 282 - 289, In
the location error handling of the artifact admission flow, check the boolean
result from errors.As before accessing typedLocationErr.Reason. Update the
parseFindingLocation error branch to safely handle a failed type match,
following the established pattern in NewArtifactLocationAdmissionError, while
preserving the existing diagnostic and return behavior for *FindingLocationError
values.
|
The diagnostics work here provided an important early view of how reviewer recovery should behave. I am closing this PR only because commits |
2 participants
Linked Issue
Closes #2028
PR Type
type:bug- Bug fixtype:feature- New featuretype:docs- Documentation onlytype:refactor- Code refactoringtype:chore- Build, CI, or tooling changestype:breaking-change- Breaking changeMaintainer action required: apply exactly the
type:buglabel. GitHub does not grant fork authors permission to label upstream pull requests.Summary
Changes
internal/reviewtransactioninternal/cliinternal/assets/opencodeTest Plan
Focused tests
internal/reviewtransactionissue fix(review): provide recovery path for rejected reviewer results #2028 tests pass.internal/clicapture and refusal-policy tests pass.node --experimental-strip-types --check internal/assets/opencode/plugins/review-result-artifacts.tsgo run ./internal/gofmtcheckgit diff --checkBroad validation
go test ./...did not complete on Windows.e2e/organicruntimeand a laterinternal/clitest timed out. The candidate-caused refusal-policy failure found before the timeout was fixed, and its exact test plus affected focused packages pass.cd bench && go build ./...cd bench && go vet ./...cd bench && go test ./...fails three Windows-only shell fixture tests that create extensionless#!/bin/shexecutables.Automated Checks
Closes #2028status:approvedtype:*Labeltype:bugContributor Checklist
status:approvedtype:*label is applied, maintainer permission is requiredCo-Authored-BytrailersNotes for Reviewers
Please challenge the admission guard with real input populations: canonical repository-relative single-line locations remain accepted, malformed ranges, text, zero, and negative lines are rejected with typed reasons, and candidate-causal findings on unchanged lines remain
out_of_scope.guard:populationdirection or baseline change was required; the existing fail-closed direction remains unchanged..guard-population-baseline.txtis unchanged.Receipt-driven development was disabled globally, so the native lifecycle did not start and delivery is recorded as
disabled/unmanaged. No review PASS is claimed.Summary by CodeRabbit