fix(opencode): extract reviewer JSON from prose-prefixed output - #2099
fix(opencode): extract reviewer JSON from prose-prefixed output#2099umi008 wants to merge 2 commits into
Conversation
review-result-artifacts.ts forwarded reviewer output verbatim to review capture-result, so a reviewer that prefixed explanatory prose (or wrapped the object in markdown fences) before the JSON envelope made the native strict decoder fail with "invalid character ... looking for beginning of value" and stranded the lens result as a preserved incident (regression of Gentleman-Programming#1579, reported as Gentleman-Programming#1789). reviewerResult now runs both the bare output and the extracted task envelope body through extractReviewerJson, which strips markdown fences and locates the single well-formed JSON object by brace-depth scanning that respects string contents, then verifies the slice with JSON.parse. Prose-only output still fails closed and is preserved as an incident. Adds harness scenarios proving the exact extracted JSON reaches capture-result byte for byte for prose-prefixed, fenced, and enveloped-prose output, and that prose-only output stays recoverable.
📝 WalkthroughWalkthroughThe OpenCode review hook now extracts a valid JSON object from reviewer output before capture. It handles prose prefixes, Markdown fences, nested task envelopes, strings, and escapes. Recovery tests verify exact capture payloads and fail-closed behavior. ChangesReviewer JSON recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Reviewer
participant reviewerResult
participant extractReviewerJson
participant CaptureResult
Reviewer->>reviewerResult: Reviewer task output
reviewerResult->>extractReviewerJson: Unwrapped result
extractReviewerJson->>reviewerResult: Valid JSON object
reviewerResult->>CaptureResult: Extracted payload
CaptureResult-->>reviewerResult: Capture status
Possibly related PRs
Suggested labels: 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/assets/opencode/plugins/review-result-artifacts.ts`:
- Around line 123-153: Update the JSON-object scanner around the candidate loop
so each “{” begins an independent scan with depth, inString, and escaped reset,
preventing unmatched quotes in leading prose from affecting later objects.
Preserve balanced-object parsing and continue scanning after invalid candidates,
and add a regression case covering an unmatched quote in the prose prefix before
a valid JSON object.
In `@internal/assets/review_plugin_recovery_test.go`:
- Around line 119-124: Update the recovery test stub and its setup to compare
capture input byte-for-byte: write expectStdin to a temporary expected file in
Go, expose its path through GENTLE_AI_STUB_EXPECT_STDIN_FILE, and replace the
stdin=$(cat) string comparison in the stub with cmp -s
"$GENTLE_AI_STUB_EXPECT_STDIN_FILE" -. Preserve the existing success and
mismatch outputs while ensuring trailing newlines remain significant.
🪄 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: 7921693d-c3ce-4c12-98e2-b40d625bed87
📒 Files selected for processing (2)
internal/assets/opencode/plugins/review-result-artifacts.tsinternal/assets/review_plugin_recovery_test.go
CodeRabbit review of PR Gentleman-Programming#2099 found two correctness gaps: 1. The single-pass scanner carried string state from leading prose into the object scan: an unmatched double quote in the prose set inString and made the scanner ignore the real JSON object that followed. extractReviewerJson now treats every '{' as an independent candidate with fresh depth/string state, and the harness prose is adversarial (unmatched quote plus stray balanced braces) to pin the regression. 2. The stub compared stdin via $(cat), which strips trailing newlines, so the byte-for-byte claim was not honest. The stub now compares with cmp against an expected file, and re-consumes stdin on the failure paths so the plugin never sees an EPIPE instead of the native stderr.
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/assets/opencode/plugins/review-result-artifacts.ts`:
- Around line 121-147: Bound the brace-scanning loop in the reviewer-output
extraction logic before attempting JSON parsing, using the existing reviewer
artifact admission limit or an equivalent scan-work cap. When the limit is
exceeded, preserve the raw response and route it through the existing
extraction-failure path rather than continuing quadratic scanning. Add a
regression case covering a large malformed brace prefix.
🪄 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: 75e37b01-2c94-4a28-a54f-efc9876b64fb
📒 Files selected for processing (2)
internal/assets/opencode/plugins/review-result-artifacts.tsinternal/assets/review_plugin_recovery_test.go
| for (let start = 0; start < candidate.length; start++) { | ||
| if (candidate[start] !== "{") continue | ||
| let depth = 1 | ||
| let inString = false | ||
| let escaped = false | ||
| let i = start + 1 | ||
| for (; i < candidate.length; i++) { | ||
| const ch = candidate[i] | ||
| if (inString) { | ||
| if (escaped) escaped = false | ||
| else if (ch === "\\") escaped = true | ||
| else if (ch === '"') inString = false | ||
| continue | ||
| } | ||
| if (ch === '"') { | ||
| inString = true | ||
| continue | ||
| } | ||
| if (ch === "{") { | ||
| depth++ | ||
| continue | ||
| } | ||
| if (ch === "}") { | ||
| depth-- | ||
| if (depth === 0) break | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Bound the candidate scan before parsing reviewer output.
Line 121 starts a full suffix scan for every {. A response containing N opening braces performs quadratic work before Line 158 throws. This occurs before review capture-result processes the result, so a large malformed reviewer response can block the after hook.
Add a source-size or scan-work limit that matches reviewer artifact admission. Preserve the raw response through the existing extraction-failure path when the limit is exceeded. Add a regression case with a large malformed brace prefix.
🤖 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 121
- 147, Bound the brace-scanning loop in the reviewer-output extraction logic
before attempting JSON parsing, using the existing reviewer artifact admission
limit or an equivalent scan-work cap. When the limit is exceeded, preserve the
raw response and route it through the existing extraction-failure path rather
than continuing quadratic scanning. Add a regression case covering a large
malformed brace prefix.
|
Independent reproductions that strengthen the evidence this PR is the right fix. Environment
Reproduction (12 lens invocations, all rejected pre-fix)For each of the 3 PRs in the sprint, the 4R set was invoked (risk / resilience / readability / reliability). All 12 lens sub-agents returned their JSON wrapped in Sample of one preserved raw output (PR #232 reliability, first lens): $ head -c 220 .git/gentle-ai/review-transactions/incidents/review-8495693e29343476/00-review-risk-acc216b468f1.raw
\`\`\`json
{"findings":[],"evidence":["Inspected .github/workflows/design-system.yml and django-tests.yml..."]}
\`\`\`After manually extracting the JSON, the lens did have substantive findings: $ python3 -c "
import json, re, glob
for f in glob.glob('.git/gentle-ai/review-transactions/incidents/review-*/*.raw'):
raw = open(f).read()
m = re.search(r'\`\`\`(?:json)?\s*(\{.*?\})\s*\`\`\`', raw, re.DOTALL)
if m:
j = json.loads(m.group(1))
n = len(j.get('findings', []))
sev = ','.join(f.get('severity','?') for f in j.get('findings',[]))
print(f'{f.split(\"/\")[-1][:50]}: {n} findings [{sev}]')
"
# ... 1 CRITICAL + 2 WARNING + several SUGGESTION findings across the 12 lensesSo the pre-fix behavior lost real review signal. This PR's Style note that helps both reviewers and PR-#2277Your helper uses brace-balancing with proper string escaping — that's the right primitive. A simpler
Your Suggestion (not blocking)Consider promoting Local workaround statusI shipped a less-robust local patch ( No PR from me in the same area. Keeping the review budget clean for your fix. |
🔗 Linked Issue
Closes #1789
🏷️ PR Type
type:bug— Bug fix (non-breaking change that fixes an issue)📝 Summary
The managed OpenCode capture hook (
review-result-artifacts.ts) forwarded reviewer output verbatim toreview capture-result. When a reviewer prefixed explanatory prose before the JSON envelope (or wrapped it in ```json fences), the native strict decoder failed withinvalid character ... looking for beginning of valueand the lens result was stranded as a preserved `.raw` incident, blocking the review budget. This is the same root cause as #1579 (closed by #1550) regressed on v2.1.11.The fix runs both the bare output and the extracted task-envelope body through a new
extractReviewerJson()that strips markdown fences and locates the single well-formed JSON object via brace-depth scanning (string-aware), verified withJSON.parse. Output with no JSON object at all still fails closed and is preserved as an incident, so recovery behavior is unchanged.📂 Changes
internal/assets/opencode/plugins/review-result-artifacts.tsreviewerResult()now extracts the JSON object from prose-prefixed/fenced output instead of forwarding it verbatiminternal/assets/review_plugin_recovery_test.gocapture-resultbyte-for-byte (prose-prefixed, fenced, enveloped-prose) and that prose-only output stays preserved🧪 Test Plan
Unit Tests
go test ./...internal/assets: 894 passed (5 new tests pin the fix(opencode): review-result-artifacts.ts fails to strip leading prose before JSON again (regression of #1579, v2.1.11) #1789/fix(plugins): review-result-artifacts.ts does not strip markdown code fences from reviewer output #1600 shapes)e2e/organicruntimefailures are pre-existing environment failures (Claude Code installer) reproduced on cleanmainGo Format
E2E Tests (Docker required)
Not run (Docker unavailable locally)
Unit tests pass (
go test ./...)Go format passes (
go run ./internal/gofmtcheck)E2E tests pass (
cd e2e && ./docker-test.sh)Manually tested locally (extraction verified against the preserved
.rawincident from the report)✅ Contributor Checklist
status:approvedsize:exceptionwith rationale documentedtype:*label to this PRgo test ./...)cd e2e && ./docker-test.sh)Co-Authored-BytrailersSummary by CodeRabbit