From 23254ca5a7fc665147c306676f629bab881b6a17 Mon Sep 17 00:00:00 2001 From: Behruz Nassre Esfahani Date: Sun, 5 Jul 2026 11:07:07 -0700 Subject: [PATCH] fix(#1936): reconstruct OpenCode review from JSON events (#1992) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#1936): reconstruct OpenCode review from JSON events; diagnosable empty-output stub On a large review prompt, OpenCode's default `build` agent runs a few read tool calls then ends its turn with zero output tokens (reason:"stop", output:0), so `opencode run --format default` emits empty stdout. The reviewer block redirected stderr to /dev/null and wrote a generic "failed or returned empty output" stub — so the phase silently lost its second independent reviewer with no diagnostic and no timeout. Rewrite the OpenCode reviewer block to invoke `--format json` as the primary call and reconstruct the review from the assistant `text` parts (jq). Capture stderr to a `.err` sidecar (mirrors the Codex block). When the agent emits no text, surface the stop reason, output-token count, and stderr so the failure is diagnosable. Gate the stub on the extracted CONTENT, not the output file size — an empty jq extraction still prints a lone newline that a `[ -s file ]` check would treat as populated. Document the wall-clock timeout as a Bash-tool param (macOS lacks GNU timeout; opencode has no native timeout flag). review.md was already at the DEFAULT size-tier ceiling (40956/40960), so the fix cannot fit without reclassifying it into the LARGE tier (it is a multi-reviewer orchestration file that outgrew "focused single-purpose"; 43.4 KB sits well under the LARGE high-water mark). Recapture the 16 golden-install fixtures — the diff is exactly one review.md hash per runtime. Regression block folded into review-default-reviewers-workflow.test.cjs (new bug-NNNN test files are not accepted). Co-Authored-By: Claude Opus 4.8 (1M context) * chore(#1936): add changeset * test(#1936): property-test the OpenCode review jq reconstruction Address the re-review's one actionable finding: the jq JSON-event → text reconstruction had no fast-check property test. Add tests/opencode-review-reconstruction.property.test.cjs. It extracts the two shipped jq programs (OPENCODE_REVIEW, OPENCODE_DIAG) verbatim from gsd-core/workflows/review.md and runs the real jq — not a reimplementation — so the shipped logic is what gets tested. Properties: the reconstructed review equals the newline-join of every assistant text part (order preserved); a stream with no text part reconstructs to empty (drives the #1936 stub); null/absent text parts are dropped, never rendered as "null". Plus example-based coverage of the diagnostic edges the reviewer cited: missing .tokens.output and no step_finish degrade to "?"; non-JSON stdout makes jq fail rather than masquerade as a review. Verified the invariant has teeth (a comma-join jq fails the property). Co-Authored-By: Claude Opus 4.8 (1M context) * test(#1936): skip jq reconstruction property test when jq is absent The property test shells out to `jq`, which GitHub's windows-latest runners do not ship (macOS/Linux runners do). `execFileSync('jq')` therefore ENOENT-failed the whole file on `test (windows-latest, *)`. Probe `jq --version` at load and skip the suite when jq is not on PATH — the reconstruction logic is platform-independent, so the assertions still run in full on every jq-present runner (mirrors how golden-install-parity skips on win32). Verified: jq present → 7 pass; jq removed from PATH → 7 skipped, 0 fail. Co-Authored-By: Claude Opus 4.8 (1M context) * test(#1936): skip jq reconstruction property test on Windows, not just when jq is absent The prior guard skipped only when `jq` was absent from PATH — but the windows-latest runners DO ship jq, so the suite still ran there and failed with `jq: parse error: Invalid numeric literal` (confirmed from the CI job log). Root cause is Node's child_process argument quoting mangling the jq program (it embeds double quotes) on Windows, not the shipped review.md logic — the macOS/Linux legs pass. Gate the suite on `process.platform === 'win32'` (still also skipping when jq is absent), mirroring golden-install-parity's win32 skip. Logic is platform-independent and fully asserted on every macOS/Linux CI leg. Verified: macOS → 7 pass; simulated win32 → skips. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .changeset/bold-deer-zip.md | 5 + gsd-core/workflows/review.md | 44 ++++- .../golden-install-parity/antigravity.json | 2 +- .../golden-install-parity/augment.json | 2 +- .../golden-install-parity/claude.json | 2 +- .../fixtures/golden-install-parity/cline.json | 2 +- .../golden-install-parity/codebuddy.json | 2 +- .../fixtures/golden-install-parity/codex.json | 2 +- .../golden-install-parity/copilot.json | 2 +- .../golden-install-parity/cursor.json | 2 +- .../golden-install-parity/hermes.json | 2 +- .../fixtures/golden-install-parity/kilo.json | 2 +- .../fixtures/golden-install-parity/kimi.json | 2 +- .../golden-install-parity/opencode.json | 2 +- .../fixtures/golden-install-parity/qwen.json | 2 +- .../fixtures/golden-install-parity/trae.json | 2 +- .../golden-install-parity/windsurf.json | 2 +- ...de-review-reconstruction.property.test.cjs | 167 ++++++++++++++++++ ...review-default-reviewers-workflow.test.cjs | 83 +++++++++ tests/workflow-size-baseline.json | 2 +- tests/workflow-size-budget.test.cjs | 1 + 21 files changed, 312 insertions(+), 20 deletions(-) create mode 100644 .changeset/bold-deer-zip.md create mode 100644 tests/opencode-review-reconstruction.property.test.cjs diff --git a/.changeset/bold-deer-zip.md b/.changeset/bold-deer-zip.md new file mode 100644 index 000000000..065cc968b --- /dev/null +++ b/.changeset/bold-deer-zip.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 1992 +--- +**OpenCode reviewer no longer silently yields an empty review on large prompts** — `/gsd-review --opencode` now invokes `opencode run --format json` and reconstructs the review from the assistant text parts, so a large-prompt run where the default `build` agent ends its turn with zero output tokens no longer produces an empty stub. When the agent genuinely emits no text, the stub now reports the stop reason, output-token count, and captured stderr instead of a generic message. (#1936) diff --git a/gsd-core/workflows/review.md b/gsd-core/workflows/review.md index 324440c70..c5dbaff19 100644 --- a/gsd-core/workflows/review.md +++ b/gsd-core/workflows/review.md @@ -302,14 +302,50 @@ coderabbit review --prompt-only 2>/dev/null > /tmp/gsd-review-coderabbit-{phase} ``` **OpenCode (via GitHub Copilot):** + +OpenCode's default `build` agent is an agentic coder, not a prompt→completion API. +On a large review prompt it may run a few `read` tool calls and then end its turn +with **zero output tokens** (`reason:"stop"`, `output:0`), so `--format default` +yields empty stdout and the second reviewer is silently lost (#1936). Invoke with +`--format json` and reconstruct the review from the assistant `text` parts; if the +agent emitted none, surface the stop `reason`, output-token count, and captured +stderr so the failure is diagnosable instead of a generic empty stub. Runs are also +nondeterministic in length, so bound this Bash tool call with a wall-clock timeout — +set `timeout: 660000` on the call (same mechanism the CodeRabbit block documents). +That bound is hard: if it fires mid-`opencode run` the tool kills the command and +the jq reconstruction below never runs, so the reviewing agent simply proceeds +without an OpenCode result. The completing zero-output case — the actual #1936 bug — +is fully handled below; the timeout only backstops the rarer nondeterministic hang. A +reviewer instance with `"agent": "review"` (see +`gsd-core/references/reviewer-instances.md`) sidesteps the default `build` agent and +is the durable fix when this recurs. + ```bash +# stderr → sidecar (never /dev/null) so a real error is diagnosable — mirrors the +# Codex block. --format json is the primary invocation (not a fallback): the review +# text lives in assistant `text` parts, which the default formatter drops when the +# agent stops with no final message (#1936). if [ -n "$OPENCODE_MODEL" ] && [ "$OPENCODE_MODEL" != "null" ]; then - cat /tmp/gsd-review-prompt-{phase}.md | opencode run --model "$OPENCODE_MODEL" - 2>/dev/null > /tmp/gsd-review-opencode-{phase}.md + set -- --model "$OPENCODE_MODEL" else - cat /tmp/gsd-review-prompt-{phase}.md | opencode run - 2>/dev/null > /tmp/gsd-review-opencode-{phase}.md + set -- fi -if [ ! -s /tmp/gsd-review-opencode-{phase}.md ]; then - echo "OpenCode review failed or returned empty output." > /tmp/gsd-review-opencode-{phase}.md +cat /tmp/gsd-review-prompt-{phase}.md | opencode run "$@" --format json - 2>/tmp/gsd-review-opencode-{phase}.err > /tmp/gsd-review-opencode-{phase}.json +# Reconstruct the review from the assistant text parts. Capture into a variable and +# test its CONTENT (not the output file's size): an empty extraction still prints a +# trailing newline, which would fool a `[ -s file ]` check into skipping the stub. +OPENCODE_REVIEW=$(jq -rs '[.[] | select(.type=="text") | .part.text // empty] | join("\n")' /tmp/gsd-review-opencode-{phase}.json 2>/dev/null) +if [ -n "$OPENCODE_REVIEW" ]; then + printf '%s\n' "$OPENCODE_REVIEW" > /tmp/gsd-review-opencode-{phase}.md +else + # No assistant text (agent emitted no final message, or stdout was not valid JSON events). + { + echo "OpenCode review returned no assistant text (#1936: agent ended its turn with no final message)." + OPENCODE_DIAG=$(jq -rs '[.[] | select(.type=="step_finish")] | last | "stop reason=\(.part.reason // "?"), output tokens=\(.part.tokens.output // "?")"' /tmp/gsd-review-opencode-{phase}.json 2>/dev/null) + [ -n "$OPENCODE_DIAG" ] && echo "Diagnostic: $OPENCODE_DIAG" + echo "stderr:" + cat /tmp/gsd-review-opencode-{phase}.err + } > /tmp/gsd-review-opencode-{phase}.md fi ``` diff --git a/tests/fixtures/golden-install-parity/antigravity.json b/tests/fixtures/golden-install-parity/antigravity.json index 478d209d6..6333e8f25 100644 --- a/tests/fixtures/golden-install-parity/antigravity.json +++ b/tests/fixtures/golden-install-parity/antigravity.json @@ -278,7 +278,7 @@ "gsd-core/workflows/remove-phase.md": "6b9947fba1a97f46", "gsd-core/workflows/remove-workspace.md": "95f05defffe6754e", "gsd-core/workflows/resume-project.md": "cadf390bae95fc76", - "gsd-core/workflows/review.md": "aa08449a6a180944", + "gsd-core/workflows/review.md": "42efd80befc112de", "gsd-core/workflows/scan.md": "f2754f3e3ea528ff", "gsd-core/workflows/secure-phase.md": "78705b7f09612464", "gsd-core/workflows/session-report.md": "2e5b1205324ddefa", diff --git a/tests/fixtures/golden-install-parity/augment.json b/tests/fixtures/golden-install-parity/augment.json index c4ab8c706..ade70527e 100644 --- a/tests/fixtures/golden-install-parity/augment.json +++ b/tests/fixtures/golden-install-parity/augment.json @@ -348,7 +348,7 @@ "gsd-core/workflows/remove-phase.md": "d030f80ca0df4fa9", "gsd-core/workflows/remove-workspace.md": "b8817a3a5907f5bc", "gsd-core/workflows/resume-project.md": "b18b51fd15cbce95", - "gsd-core/workflows/review.md": "fbfa6eebcad41aa0", + "gsd-core/workflows/review.md": "b59974ca892b7404", "gsd-core/workflows/scan.md": "54ff1ff60041d065", "gsd-core/workflows/secure-phase.md": "1d1c66ad9ea01bd2", "gsd-core/workflows/session-report.md": "2e5b1205324ddefa", diff --git a/tests/fixtures/golden-install-parity/claude.json b/tests/fixtures/golden-install-parity/claude.json index 440cb4fbf..eef09403a 100644 --- a/tests/fixtures/golden-install-parity/claude.json +++ b/tests/fixtures/golden-install-parity/claude.json @@ -277,7 +277,7 @@ "gsd-core/workflows/remove-phase.md": "f76e1c2a4dd31a09", "gsd-core/workflows/remove-workspace.md": "0e73844f0f41cbc3", "gsd-core/workflows/resume-project.md": "3dcaa7abe1800d35", - "gsd-core/workflows/review.md": "a1d3ebce6222ff72", + "gsd-core/workflows/review.md": "fec86657cbd9e92f", "gsd-core/workflows/scan.md": "8e1bbf2eed1752ca", "gsd-core/workflows/secure-phase.md": "b2b9100ff79d6017", "gsd-core/workflows/session-report.md": "2e5b1205324ddefa", diff --git a/tests/fixtures/golden-install-parity/cline.json b/tests/fixtures/golden-install-parity/cline.json index 7a8644def..c2282a5f8 100644 --- a/tests/fixtures/golden-install-parity/cline.json +++ b/tests/fixtures/golden-install-parity/cline.json @@ -281,7 +281,7 @@ "gsd-core/workflows/remove-phase.md": "61b68a4af414e3c2", "gsd-core/workflows/remove-workspace.md": "411bfff942216185", "gsd-core/workflows/resume-project.md": "f8f71b5a98374b85", - "gsd-core/workflows/review.md": "f7d5b0f64631adea", + "gsd-core/workflows/review.md": "a09b4d6ece11ddb6", "gsd-core/workflows/scan.md": "db0c48c3963f9a8b", "gsd-core/workflows/secure-phase.md": "c51b86e369574195", "gsd-core/workflows/session-report.md": "2e5b1205324ddefa", diff --git a/tests/fixtures/golden-install-parity/codebuddy.json b/tests/fixtures/golden-install-parity/codebuddy.json index 84fd4f82a..700530540 100644 --- a/tests/fixtures/golden-install-parity/codebuddy.json +++ b/tests/fixtures/golden-install-parity/codebuddy.json @@ -348,7 +348,7 @@ "gsd-core/workflows/remove-phase.md": "d030f80ca0df4fa9", "gsd-core/workflows/remove-workspace.md": "b8817a3a5907f5bc", "gsd-core/workflows/resume-project.md": "b18b51fd15cbce95", - "gsd-core/workflows/review.md": "fbfa6eebcad41aa0", + "gsd-core/workflows/review.md": "b59974ca892b7404", "gsd-core/workflows/scan.md": "54ff1ff60041d065", "gsd-core/workflows/secure-phase.md": "1d1c66ad9ea01bd2", "gsd-core/workflows/session-report.md": "2e5b1205324ddefa", diff --git a/tests/fixtures/golden-install-parity/codex.json b/tests/fixtures/golden-install-parity/codex.json index 268d8d358..ff82e369a 100644 --- a/tests/fixtures/golden-install-parity/codex.json +++ b/tests/fixtures/golden-install-parity/codex.json @@ -313,7 +313,7 @@ "gsd-core/workflows/remove-phase.md": "75c8ca0dbd1ce404", "gsd-core/workflows/remove-workspace.md": "a9d1dcda7755b6c9", "gsd-core/workflows/resume-project.md": "94ac2cac4c562557", - "gsd-core/workflows/review.md": "bd3873bea54d6143", + "gsd-core/workflows/review.md": "0ed332a3ab1426e6", "gsd-core/workflows/scan.md": "29f3b65f5d9de885", "gsd-core/workflows/secure-phase.md": "858b5e1152b569ed", "gsd-core/workflows/session-report.md": "dd8fa011c9394075", diff --git a/tests/fixtures/golden-install-parity/copilot.json b/tests/fixtures/golden-install-parity/copilot.json index 14fa8a7c1..c58a4ac7f 100644 --- a/tests/fixtures/golden-install-parity/copilot.json +++ b/tests/fixtures/golden-install-parity/copilot.json @@ -279,7 +279,7 @@ "gsd-core/workflows/remove-phase.md": "e94ddfe4eabc0e08", "gsd-core/workflows/remove-workspace.md": "f28be7f32249f0d4", "gsd-core/workflows/resume-project.md": "4ebcb4acd3c29302", - "gsd-core/workflows/review.md": "60b8ca1c543ccdfa", + "gsd-core/workflows/review.md": "2b673bcc3a00e2f5", "gsd-core/workflows/scan.md": "28a2847b8d04156e", "gsd-core/workflows/secure-phase.md": "bae509fa254fe435", "gsd-core/workflows/session-report.md": "2e5b1205324ddefa", diff --git a/tests/fixtures/golden-install-parity/cursor.json b/tests/fixtures/golden-install-parity/cursor.json index 093f7ae1e..ac8301bad 100644 --- a/tests/fixtures/golden-install-parity/cursor.json +++ b/tests/fixtures/golden-install-parity/cursor.json @@ -348,7 +348,7 @@ "gsd-core/workflows/remove-phase.md": "f76e1c2a4dd31a09", "gsd-core/workflows/remove-workspace.md": "dadbb14d9033ea3f", "gsd-core/workflows/resume-project.md": "3dcaa7abe1800d35", - "gsd-core/workflows/review.md": "141589a39b8c7e18", + "gsd-core/workflows/review.md": "3002073a6f3b5f83", "gsd-core/workflows/scan.md": "8e1bbf2eed1752ca", "gsd-core/workflows/secure-phase.md": "7bd4c335484fd121", "gsd-core/workflows/session-report.md": "2e5b1205324ddefa", diff --git a/tests/fixtures/golden-install-parity/hermes.json b/tests/fixtures/golden-install-parity/hermes.json index 9a3096f10..d2c1247a6 100644 --- a/tests/fixtures/golden-install-parity/hermes.json +++ b/tests/fixtures/golden-install-parity/hermes.json @@ -278,7 +278,7 @@ "gsd-core/workflows/remove-phase.md": "f27cecd8c78008ae", "gsd-core/workflows/remove-workspace.md": "977155e18b9df623", "gsd-core/workflows/resume-project.md": "849f3d49d366ff13", - "gsd-core/workflows/review.md": "06cd9992e289b241", + "gsd-core/workflows/review.md": "aeedade386e820f5", "gsd-core/workflows/scan.md": "cbde2b8b2b5fa5dd", "gsd-core/workflows/secure-phase.md": "5fc8fbd5e217e48d", "gsd-core/workflows/session-report.md": "2e5b1205324ddefa", diff --git a/tests/fixtures/golden-install-parity/kilo.json b/tests/fixtures/golden-install-parity/kilo.json index 6df66038c..aed05a584 100644 --- a/tests/fixtures/golden-install-parity/kilo.json +++ b/tests/fixtures/golden-install-parity/kilo.json @@ -348,7 +348,7 @@ "gsd-core/workflows/remove-phase.md": "f76e1c2a4dd31a09", "gsd-core/workflows/remove-workspace.md": "7bd5d1b63ef091a8", "gsd-core/workflows/resume-project.md": "3dcaa7abe1800d35", - "gsd-core/workflows/review.md": "9ed1fda4d8b657f1", + "gsd-core/workflows/review.md": "2af34d173c11f74e", "gsd-core/workflows/scan.md": "8e1bbf2eed1752ca", "gsd-core/workflows/secure-phase.md": "87fdcb37a8610e58", "gsd-core/workflows/session-report.md": "2e5b1205324ddefa", diff --git a/tests/fixtures/golden-install-parity/kimi.json b/tests/fixtures/golden-install-parity/kimi.json index 43ea7c136..03ea5d7e5 100644 --- a/tests/fixtures/golden-install-parity/kimi.json +++ b/tests/fixtures/golden-install-parity/kimi.json @@ -314,7 +314,7 @@ "gsd-core/workflows/remove-phase.md": "d030f80ca0df4fa9", "gsd-core/workflows/remove-workspace.md": "b8817a3a5907f5bc", "gsd-core/workflows/resume-project.md": "b18b51fd15cbce95", - "gsd-core/workflows/review.md": "fbfa6eebcad41aa0", + "gsd-core/workflows/review.md": "b59974ca892b7404", "gsd-core/workflows/scan.md": "54ff1ff60041d065", "gsd-core/workflows/secure-phase.md": "1d1c66ad9ea01bd2", "gsd-core/workflows/session-report.md": "2e5b1205324ddefa", diff --git a/tests/fixtures/golden-install-parity/opencode.json b/tests/fixtures/golden-install-parity/opencode.json index ce361ce90..58f5786ec 100644 --- a/tests/fixtures/golden-install-parity/opencode.json +++ b/tests/fixtures/golden-install-parity/opencode.json @@ -348,7 +348,7 @@ "gsd-core/workflows/remove-phase.md": "863c58e99e9d630d", "gsd-core/workflows/remove-workspace.md": "4ed03c52e5c7bd4d", "gsd-core/workflows/resume-project.md": "15153ce5a4c38674", - "gsd-core/workflows/review.md": "4076abd5ff01195d", + "gsd-core/workflows/review.md": "7cd3922bff5f4a48", "gsd-core/workflows/scan.md": "4634caefa32a7307", "gsd-core/workflows/secure-phase.md": "9616682fe23cf042", "gsd-core/workflows/session-report.md": "2e5b1205324ddefa", diff --git a/tests/fixtures/golden-install-parity/qwen.json b/tests/fixtures/golden-install-parity/qwen.json index b1b07ced4..706708c55 100644 --- a/tests/fixtures/golden-install-parity/qwen.json +++ b/tests/fixtures/golden-install-parity/qwen.json @@ -278,7 +278,7 @@ "gsd-core/workflows/remove-phase.md": "17c46fdcef27df2d", "gsd-core/workflows/remove-workspace.md": "1d34788a9b2272d2", "gsd-core/workflows/resume-project.md": "bba3250afbea8f09", - "gsd-core/workflows/review.md": "8ed9a9e7d150978d", + "gsd-core/workflows/review.md": "75d82be469799e0f", "gsd-core/workflows/scan.md": "514d3eba81565193", "gsd-core/workflows/secure-phase.md": "a7e8edb0e5f48256", "gsd-core/workflows/session-report.md": "2e5b1205324ddefa", diff --git a/tests/fixtures/golden-install-parity/trae.json b/tests/fixtures/golden-install-parity/trae.json index 8b4c7054a..dc15663b0 100644 --- a/tests/fixtures/golden-install-parity/trae.json +++ b/tests/fixtures/golden-install-parity/trae.json @@ -278,7 +278,7 @@ "gsd-core/workflows/remove-phase.md": "5a2521695edd486b", "gsd-core/workflows/remove-workspace.md": "ed8e5e30c33f1bfd", "gsd-core/workflows/resume-project.md": "ee9dcc4e3dd3e32c", - "gsd-core/workflows/review.md": "a2dab55fa3acfca0", + "gsd-core/workflows/review.md": "c9fd33007ace85d3", "gsd-core/workflows/scan.md": "afc48f3339293b30", "gsd-core/workflows/secure-phase.md": "882886f04d3b7e82", "gsd-core/workflows/session-report.md": "2e5b1205324ddefa", diff --git a/tests/fixtures/golden-install-parity/windsurf.json b/tests/fixtures/golden-install-parity/windsurf.json index f750157b7..5a1a869b0 100644 --- a/tests/fixtures/golden-install-parity/windsurf.json +++ b/tests/fixtures/golden-install-parity/windsurf.json @@ -278,7 +278,7 @@ "gsd-core/workflows/remove-phase.md": "24559d23e008c9c3", "gsd-core/workflows/remove-workspace.md": "e958c1e932aef492", "gsd-core/workflows/resume-project.md": "c5731b8aabed70f9", - "gsd-core/workflows/review.md": "dc8366ef168183db", + "gsd-core/workflows/review.md": "7ace79b5e67c9be3", "gsd-core/workflows/scan.md": "8aa95448b1ba4a4a", "gsd-core/workflows/secure-phase.md": "550152cd82c25c00", "gsd-core/workflows/session-report.md": "2e5b1205324ddefa", diff --git a/tests/opencode-review-reconstruction.property.test.cjs b/tests/opencode-review-reconstruction.property.test.cjs new file mode 100644 index 000000000..6416d34af --- /dev/null +++ b/tests/opencode-review-reconstruction.property.test.cjs @@ -0,0 +1,167 @@ +// allow-test-rule: source-text-is-the-product (see #1936) +// The OpenCode reviewer reconstructs its review from opencode's --format json +// event stream using two embedded jq programs in gsd-core/workflows/review.md. +// Those programs ARE the runtime contract; this test extracts them verbatim from +// the workflow and exercises the real jq (not a reimplementation) so the shipped +// reconstruction logic is what gets property-tested. +'use strict'; + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const { execFileSync } = require('node:child_process'); +const fs = require('node:fs'); +const path = require('node:path'); +const fc = require('./helpers/fast-check-setup.cjs'); + +const reviewPath = path.resolve(__dirname, '..', 'gsd-core', 'workflows', 'review.md'); +const workflow = fs.readFileSync(reviewPath, 'utf-8'); + +// Extract the two shipped jq programs verbatim. If review.md changes their shape, +// these throw and the test fails loudly (intended coupling — #1936). +function extractJqProgram(varName) { + const re = new RegExp(`${varName}=\\$\\(jq -rs '([^']*)'`); + const m = workflow.match(re); + assert.ok(m, `review.md must define ${varName} via jq -rs '' (#1936)`); + return m[1]; +} +const TEXT_PROGRAM = extractJqProgram('OPENCODE_REVIEW'); // review reconstruction +const DIAG_PROGRAM = extractJqProgram('OPENCODE_DIAG'); // empty-output diagnostic + +// This suite shells out to `jq`. On Windows, Node's child_process argument quoting +// mangles the jq program (it embeds quotes) — jq then raises a parse error — and +// jq isn't guaranteed on the host regardless. The reconstruction logic is +// platform-independent (the review workflow runs jq in its Unix-y runtime), so gate +// the suite to jq-present non-Windows hosts, mirroring golden-install-parity's win32 +// skip. The assertions run in full on every macOS/Linux CI leg. +let jqAvailable = false; +try { execFileSync('jq', ['--version'], { stdio: 'ignore' }); jqAvailable = true; } catch { /* no jq on PATH */ } +const skipReason = process.platform === 'win32' + ? 'jq invocation is not portable under Node child_process arg-quoting on Windows; logic is platform-independent and asserted on macOS/Linux' + : (jqAvailable ? false : 'jq not on PATH'); +const opts = { skip: skipReason }; + +// Run a shipped jq program against a stream of events serialized exactly as +// opencode emits them: one JSON value per line (jq -s slurps them into an array). +// jq -r appends a single trailing newline to the (single) string result; strip it +// to recover the value the workflow's `$(…)` capture would see. +function runJq(program, events) { + const jsonl = events.map((e) => JSON.stringify(e)).join('\n'); + const out = execFileSync('jq', ['-rs', program], { input: jsonl, encoding: 'utf8' }); + return out.endsWith('\n') ? out.slice(0, -1) : out; +} + +// Text values safe to round-trip through JSON → jq (utf8) → string. Excludes lone +// surrogates (which don't survive utf8) but keeps the interesting cases: newlines, +// quotes, backslashes, braces, unicode. +const safeText = fc + .string({ minLength: 0, maxLength: 40 }) + .filter((s) => Buffer.from(s, 'utf8').toString('utf8') === s); + +// A `text` event whose `.part.text` is a string, or null/absent (dropped by `// empty`). +const textEvent = fc.record({ + type: fc.constant('text'), + part: fc.oneof( + fc.record({ text: safeText }), + fc.record({ text: fc.constant(null) }), // null → jq `// empty` drops it + fc.record({}), // absent → jq `// empty` drops it + ), +}); +const stepFinishEvent = fc.record({ + type: fc.constant('step_finish'), + part: fc.record({ + reason: fc.constantFrom('stop', 'length', 'tool_calls'), + tokens: fc.record({ output: fc.integer({ min: 0, max: 100000 }) }), + }), +}); +const nonTextEvent = fc.oneof( + stepFinishEvent, + fc.record({ type: fc.constant('tool_use'), part: fc.record({ tool: safeText }) }), + fc.record({ type: fc.constant('step_start'), part: fc.record({}) }), +); +// Weight text events higher so streams routinely mix real review text with noise, +// but also generate text-free streams (the #1936 zero-output case). +const eventStream = fc.array(fc.oneof(textEvent, textEvent, nonTextEvent), { + minLength: 1, + maxLength: 30, +}); + +describe('#1936 OpenCode review reconstruction — jq properties', () => { + test('review == the newline-join of every assistant text part (order preserved)', opts, () => { + fc.assert( + fc.property(eventStream, (events) => { + const expected = events + .filter((e) => e.type === 'text' && e.part && typeof e.part.text === 'string') + .map((e) => e.part.text) + .join('\n'); + assert.equal(runJq(TEXT_PROGRAM, events), expected); + }), + ); + }); + + test('a stream with no assistant text part reconstructs to empty (drives the #1936 stub)', opts, () => { + fc.assert( + fc.property(fc.array(nonTextEvent, { minLength: 1, maxLength: 20 }), (events) => { + // This is the exact failure the bug describes: the agent runs tool calls + // and ends with step_finish, emitting no text. Reconstruction must be empty + // so the content-gate (`[ -n "$OPENCODE_REVIEW" ]`) falls through to the stub. + assert.equal(runJq(TEXT_PROGRAM, events), ''); + }), + ); + }); + + test('text parts that are null/absent are dropped, never rendered as "null"', opts, () => { + fc.assert( + fc.property( + fc.array( + fc.oneof( + fc.record({ type: fc.constant('text'), part: fc.record({ text: fc.constant(null) }) }), + fc.record({ type: fc.constant('text'), part: fc.record({}) }), + ), + { minLength: 1, maxLength: 10 }, + ), + (events) => { + const out = runJq(TEXT_PROGRAM, events); + assert.equal(out, ''); + assert.doesNotMatch(out, /null/); + }, + ), + ); + }); + + // Diagnostic path (empty-output stub). The finding calls out `missing .tokens.output` + // and no-step_finish as real edges — pin them with examples against the shipped jq. + describe('diagnostic reconstruction (stop reason + output tokens)', () => { + test('reports reason and output tokens from the LAST step_finish', opts, () => { + const events = [ + { type: 'step_finish', part: { reason: 'tool_calls', tokens: { output: 5 } } }, + { type: 'tool_use', part: {} }, + { type: 'step_finish', part: { reason: 'stop', tokens: { output: 0 } } }, + ]; + assert.equal(runJq(DIAG_PROGRAM, events), 'stop reason=stop, output tokens=0'); + }); + + test('missing .tokens.output degrades to "?" rather than null/garbage', opts, () => { + const events = [{ type: 'step_finish', part: { reason: 'stop', tokens: {} } }]; + assert.equal(runJq(DIAG_PROGRAM, events), 'stop reason=stop, output tokens=?'); + }); + + test('no step_finish at all degrades both fields to "?"', opts, () => { + const events = [{ type: 'tool_use', part: { tool: 'read' } }]; + assert.equal(runJq(DIAG_PROGRAM, events), 'stop reason=?, output tokens=?'); + }); + }); + + // The primary reconstruction runs before any content gate; on non-JSON stdout + // (e.g. an opencode crash that printed a plain-text error) jq must fail rather + // than emit that text as a "review" — the workflow's `2>/dev/null` + empty + // capture then routes to the diagnostic stub. + test('non-JSON stdout does not masquerade as a reconstructed review', opts, () => { + let threw = false; + try { + execFileSync('jq', ['-rs', TEXT_PROGRAM], { input: 'auth token expired\n', encoding: 'utf8' }); + } catch { + threw = true; + } + assert.ok(threw, 'jq must reject non-JSON input so it cannot be captured as a review'); + }); +}); diff --git a/tests/review-default-reviewers-workflow.test.cjs b/tests/review-default-reviewers-workflow.test.cjs index daf8097d9..4e29db3ac 100644 --- a/tests/review-default-reviewers-workflow.test.cjs +++ b/tests/review-default-reviewers-workflow.test.cjs @@ -355,3 +355,86 @@ describe('#1698 regression: codex review is captured via --output-last-message, }); }); } + + +// ──────────────────────────────────────────────────────────────────────── +// #1936: OpenCode reviewer must not silently yield an empty review +// ──────────────────────────────────────────────────────────────────────── +{ + const { describe: __foldDescribe } = require('node:test'); + __foldDescribe('#1936: OpenCode reviewer empty-output hardening', () => { +'use strict'; + +// allow-test-rule: source-text-is-the-product (see #1936) +// review.md is a workflow file whose embedded bash IS the runtime contract; the +// `opencode run` invocation on a large agentic prompt cannot be run in CI, so we +// assert on its content. + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const reviewPath = path.resolve(__dirname, '..', 'gsd-core', 'workflows', 'review.md'); +const read = () => fs.readFileSync(reviewPath, 'utf-8'); + +// Isolate the base OpenCode reviewer block (heading -> next reviewer heading) so +// assertions about its stderr handling don't accidentally match sibling reviewers +// (gemini/claude/coderabbit/qwen legitimately use /dev/null). +function openCodeBlock() { + const c = read(); + const start = c.indexOf('**OpenCode (via GitHub Copilot):**'); + assert.notStrictEqual(start, -1, 'review.md must contain the base OpenCode reviewer block'); + const rest = c.slice(start + 1); + const nextHeading = rest.search(/\n\*\*[A-Z][^\n]*:\*\*/); + return nextHeading === -1 ? c.slice(start) : c.slice(start, start + 1 + nextHeading); +} + +describe('bug #1936: OpenCode reviewer must not silently yield an empty review', () => { + test('captures opencode stderr to a sidecar, never /dev/null', () => { + const block = openCodeBlock(); + assert.match(block, /opencode run [^\n]*2>\/tmp\/gsd-review-opencode-\{phase\}\.err/, + 'the opencode invocation must send stderr to a .err sidecar so failures are diagnosable'); + assert.doesNotMatch(block, /opencode run [^\n]*2>\/dev\/null/, + 'the opencode invocation must not discard stderr to /dev/null (#1936)'); + }); + + test('requests structured JSON output and reconstructs review from assistant text parts', () => { + const block = openCodeBlock(); + assert.match(block, /opencode run [^\n]*--format json/, + 'must invoke opencode with --format json so assistant text parts are recoverable'); + assert.match(block, /select\(\.type=="text"\)\s*\|\s*\.part\.text/, + 'must extract the assistant text parts via `.part.text` from the JSON event stream'); + }); + + test('gates the empty-review stub on extracted CONTENT, not output-file size', () => { + // An empty jq extraction still writes a trailing newline, so a `[ -s file ]` + // check would treat a content-less review as populated and skip the stub. The + // block must test the captured text variable instead. + const block = openCodeBlock(); + assert.match(block, /OPENCODE_REVIEW=\$\(jq/, 'must capture the extraction into a variable'); + assert.match(block, /\[ -n "\$OPENCODE_REVIEW" \]/, + 'must branch on the content of $OPENCODE_REVIEW, not on the size of the .md file'); + assert.doesNotMatch(block, /\[ ! -s \/tmp\/gsd-review-opencode-\{phase\}\.md \]/, + 'must not gate the stub on `[ ! -s ...opencode...md ]` (a lone newline defeats it)'); + }); + + test('empty-output stub is diagnosable: references #1936, stop reason/tokens, and stderr', () => { + const block = openCodeBlock(); + assert.match(block, /#1936/, 'the empty-output stub must reference the issue'); + assert.match(block, /step_finish[\s\S]*\.part\.reason[\s\S]*\.part\.tokens\.output/, + 'the stub must surface the stop reason and output-token count from the final step_finish'); + assert.match(block, /cat \/tmp\/gsd-review-opencode-\{phase\}\.err/, + 'the stub must append the captured stderr'); + }); + + test('does not regress the Codex reviewer block (still captures stderr to .err)', () => { + // #1936 changes only the OpenCode block; the Codex block's existing + // stderr-to-sidecar contract must remain intact. + assert.match(read(), /codex exec [^\n]*2>\/tmp\/gsd-review-codex-\{phase\}\.err/, + 'the Codex reviewer block must be left unchanged'); + }); +}); + + }); +} diff --git a/tests/workflow-size-baseline.json b/tests/workflow-size-baseline.json index 424db6752..2268b79aa 100644 --- a/tests/workflow-size-baseline.json +++ b/tests/workflow-size-baseline.json @@ -63,7 +63,7 @@ "remove-phase.md": 8469, "remove-workspace.md": 7507, "resume-project.md": 17226, - "review.md": 40956, + "review.md": 43406, "scan.md": 7688, "secure-phase.md": 13476, "session-report.md": 4044, diff --git a/tests/workflow-size-budget.test.cjs b/tests/workflow-size-budget.test.cjs index 46a80f75e..d93ca07fa 100644 --- a/tests/workflow-size-budget.test.cjs +++ b/tests/workflow-size-budget.test.cjs @@ -124,6 +124,7 @@ const LARGE_WORKFLOWS = new Set([ 'update', // 20766 'quick', // 45710 'code-review', // 28726 + 'review', // multi-reviewer orchestration; outgrew DEFAULT (was at the 40960 ceiling) when the OpenCode reviewer gained JSON reconstruction + a diagnosable empty-output stub (#1936) ]); // Single source of truth for BOTH enumeration and measurement (#1074; finishes