diff --git a/.changeset/sharp-herons-click.md b/.changeset/sharp-herons-click.md new file mode 100644 index 000000000..60a0e0c6c --- /dev/null +++ b/.changeset/sharp-herons-click.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2689 +--- +**A failed LM Studio or llama.cpp reviewer leg is now visible instead of silently dropped** — when a local OpenAI-compatible endpoint was unreachable or returned empty content, `/gsd-review` wrote no review file at all, so the reviewer's section was omitted from the final review and the result was indistinguishable from that reviewer never having been selected. Both legs now emit a diagnosable stub carrying curl's stderr and the raw response body, matching the guard the claude/gemini/codex legs already had. (#2605) diff --git a/gsd-core/workflows/review.md b/gsd-core/workflows/review.md index 410a0e892..22fda7a34 100644 --- a/gsd-core/workflows/review.md +++ b/gsd-core/workflows/review.md @@ -367,7 +367,15 @@ fi Note: CodeRabbit reviews the current git diff/working tree — it does not accept a prompt or model flag. It may take up to 5 minutes. Use `timeout: 360000` on the Bash tool call. The source-grounding requirement in the build_prompt Review Instructions applies only to the prompt-fed reviewers above; CodeRabbit is a diff-only reviewer and never receives it. Treat its output as a diff observation, not a grounded plan-level verdict. ```bash -coderabbit review --prompt-only 2>/dev/null > {run_dir}/gsd-review-coderabbit.md +# #2605: same guard as every other leg (#2494/#2592). `2>/dev/null` with no +# `[ ! -s … ]` stub left a zero-byte file when coderabbit was missing, +# unauthenticated, or exited without stdout — write_reviews then rendered a +# reviewer that "ran cleanly with nothing to report", silently dropping the lane. +coderabbit review --prompt-only 2>{run_dir}/gsd-review-coderabbit.err > {run_dir}/gsd-review-coderabbit.md +if [ ! -s {run_dir}/gsd-review-coderabbit.md ]; then + echo "CodeRabbit review failed or returned empty output. stderr:" > {run_dir}/gsd-review-coderabbit.md + cat {run_dir}/gsd-review-coderabbit.err >> {run_dir}/gsd-review-coderabbit.md 2>/dev/null +fi ``` **OpenCode (via GitHub Copilot):** @@ -702,16 +710,34 @@ OLLAMA_MODEL=$(gsd_run query config-get review.models.ollama --raw 2>/dev/null | if [ -z "$OLLAMA_MODEL" ] || [ "$OLLAMA_MODEL" = "null" ]; then OLLAMA_MODEL=$(curl -s --max-time 2 "${OLLAMA_HOST}/v1/models" 2>/dev/null | jq -r '.data[0].id // "llama3"' 2>/dev/null || echo "llama3") fi -jq -n --rawfile content "$OLLAMA_PROMPT_FILE" \ +# #2605: brought to parity with the LM Studio / llama.cpp legs below. Ollama +# already emitted a non-empty stub, so it never silently vanished — but it was +# the LEAST diagnosable leg: bare `-s` (which suppresses curl's error text as +# well as the progress meter), stderr to /dev/null, and the response piped +# straight into jq so the body — where an OpenAI-compatible server puts its error +# JSON on an HTTP 4xx/5xx, with curl still exiting 0 — was discarded unread. +OLLAMA_RESPONSE=$(jq -n --rawfile content "$OLLAMA_PROMPT_FILE" \ --arg model "$OLLAMA_MODEL" \ '{model: $model, messages: [{role: "user", content: $content}]}' | \ - curl -s --max-time 120 -X POST "${OLLAMA_HOST}/v1/chat/completions" \ - -H "Content-Type: application/json" -d @- 2>/dev/null | \ - jq -r '.choices[0].message.content // "Ollama review failed or returned empty output."' \ - > {run_dir}/gsd-review-ollama.md -if [ ! -s {run_dir}/gsd-review-ollama.md ]; then - echo "Ollama review failed or returned empty output." > {run_dir}/gsd-review-ollama.md + curl -sS --max-time 120 -X POST "${OLLAMA_HOST}/v1/chat/completions" \ + -H "Content-Type: application/json" -d @- 2>{run_dir}/gsd-review-ollama.err) +OLLAMA_CONTENT=$(echo "$OLLAMA_RESPONSE" | jq -r '.choices[0].message.content // ""' 2>/dev/null || echo "") +case "$OLLAMA_CONTENT" in + *[![:space:]]*) : ;; + *) OLLAMA_CONTENT="" ;; +esac +if [ -n "$OLLAMA_CONTENT" ]; then + printf '%s\n' "$OLLAMA_CONTENT" > {run_dir}/gsd-review-ollama.md fi +if [ ! -s {run_dir}/gsd-review-ollama.md ]; then + echo "Warning: Ollama returned empty content — see {run_dir}/gsd-review-ollama.md" >&2 + echo "Ollama review failed or returned empty output. stderr:" > {run_dir}/gsd-review-ollama.md + cat {run_dir}/gsd-review-ollama.err >> {run_dir}/gsd-review-ollama.md 2>/dev/null + echo "Raw response body:" >> {run_dir}/gsd-review-ollama.md + printf '%s\n' "$OLLAMA_RESPONSE" >> {run_dir}/gsd-review-ollama.md +fi +else +echo "Ollama review skipped: prompt budget (${OLLAMA_REVIEWER_BUDGET} tokens) too small for the minimum review set." > {run_dir}/gsd-review-ollama.md fi ``` @@ -750,21 +776,53 @@ LM_STUDIO_MODEL=$(gsd_run query config-get review.models.lm_studio --raw 2>/dev/ if [ -z "$LM_STUDIO_MODEL" ] || [ "$LM_STUDIO_MODEL" = "null" ]; then LM_STUDIO_MODEL=$(curl -s --max-time 2 "${LM_STUDIO_HOST}/v1/models" 2>/dev/null | jq -r '.data[0].id // "local-model"' 2>/dev/null || echo "local-model") fi +# #2605: same guard as the claude/gemini/codex legs above (#2494/#2592). Two +# changes make a dropped lane diagnosable rather than silently omitted: +# 1. `-sS` instead of `-s`. Plain `-s` silences curl's ERROR text too, so an +# unreachable endpoint produced no message anywhere. `-S` restores errors +# while keeping the progress meter off; they land in the .err sidecar. +# 2. An `[ ! -s … ]` stub. Previously nothing was written when content was +# empty, so the file never existed, write_reviews omitted the section, and +# the result was indistinguishable from the reviewer never being selected. +# The raw response body is appended too: an HTTP 4xx/5xx from an OpenAI-compatible +# server exits 0 with the error JSON in the BODY, so stderr alone would be empty. LM_STUDIO_RESPONSE=$(jq -n --rawfile content "$LM_STUDIO_PROMPT_FILE" \ --arg model "$LM_STUDIO_MODEL" \ '{model: $model, messages: [{role: "user", content: $content}]}' | \ - curl -s --max-time 120 -X POST "${LM_STUDIO_HOST}/v1/chat/completions" \ - -H "Content-Type: application/json" -d @- 2>/dev/null) + curl -sS --max-time 120 -X POST "${LM_STUDIO_HOST}/v1/chat/completions" \ + -H "Content-Type: application/json" -d @- 2>{run_dir}/gsd-review-lm_studio.err) LM_STUDIO_ACTUAL_MODEL=$(echo "$LM_STUDIO_RESPONSE" | jq -r '.model // ""' 2>/dev/null || echo "") if [ -n "$LM_STUDIO_ACTUAL_MODEL" ] && [ "$LM_STUDIO_ACTUAL_MODEL" != "null" ] && [ "$LM_STUDIO_ACTUAL_MODEL" != "$LM_STUDIO_MODEL" ]; then echo "Warning: LM Studio served model '$LM_STUDIO_ACTUAL_MODEL' but '$LM_STUDIO_MODEL' was requested. Review may be from a different model." >&2 fi LM_STUDIO_CONTENT=$(echo "$LM_STUDIO_RESPONSE" | jq -r '.choices[0].message.content // ""' 2>/dev/null || echo "") +# A whitespace-only reply must count as empty. `[ ! -s … ]` counts BYTES, so a +# response of " " would be written out and pass the guard as a "successful" +# but vacuous review — the same indistinguishable-from-success outcome the guard +# exists to prevent. Command substitution strips trailing newlines but not +# spaces, so this case-glob is what actually closes it. +case "$LM_STUDIO_CONTENT" in + *[![:space:]]*) : ;; + *) LM_STUDIO_CONTENT="" ;; +esac +# printf, not echo: `echo "$VAR"` swallows a value that is exactly `-n`/`-e`/`-E` +# and would write 0 bytes, misclassifying a real reply as empty. Same idiom the +# OpenCode leg already uses above. if [ -n "$LM_STUDIO_CONTENT" ]; then - echo "$LM_STUDIO_CONTENT" > {run_dir}/gsd-review-lm_studio.md -else - echo "Warning: LM Studio returned empty content — skipping review." >&2 + printf '%s\n' "$LM_STUDIO_CONTENT" > {run_dir}/gsd-review-lm_studio.md fi +if [ ! -s {run_dir}/gsd-review-lm_studio.md ]; then + echo "Warning: LM Studio returned empty content — see {run_dir}/gsd-review-lm_studio.md" >&2 + echo "LM Studio review failed or returned empty output. stderr:" > {run_dir}/gsd-review-lm_studio.md + cat {run_dir}/gsd-review-lm_studio.err >> {run_dir}/gsd-review-lm_studio.md 2>/dev/null + echo "Raw response body:" >> {run_dir}/gsd-review-lm_studio.md + printf '%s\n' "$LM_STUDIO_RESPONSE" >> {run_dir}/gsd-review-lm_studio.md +fi +else +# A budget skip drops the lane just as silently as an empty response did: no +# file, so write_reviews omits the section entirely. Leave the same diagnosable +# stub so the skip is visible in the review output, not only on stderr (#2605). +echo "LM Studio review skipped: prompt budget (${LM_STUDIO_REVIEWER_BUDGET} tokens) too small for the minimum review set." > {run_dir}/gsd-review-lm_studio.md fi ``` @@ -803,17 +861,34 @@ LLAMA_CPP_MODEL=$(gsd_run query config-get review.models.llama_cpp --raw 2>/dev/ if [ -z "$LLAMA_CPP_MODEL" ] || [ "$LLAMA_CPP_MODEL" = "null" ]; then LLAMA_CPP_MODEL=$(curl -s --max-time 2 "${LLAMA_CPP_HOST}/v1/models" 2>/dev/null | jq -r '.data[0].id // "local-model"' 2>/dev/null || echo "local-model") fi -LLAMA_CPP_CONTENT=$(jq -n --rawfile content "$LLAMA_CPP_PROMPT_FILE" \ +# #2605: same guard as the LM Studio leg above. The response is captured to a +# variable FIRST rather than piped straight into jq — piping discarded the raw +# body, which is exactly where an OpenAI-compatible server puts its error JSON on +# an HTTP 4xx/5xx (curl still exits 0), leaving nothing to diagnose. +LLAMA_CPP_RESPONSE=$(jq -n --rawfile content "$LLAMA_CPP_PROMPT_FILE" \ --arg model "$LLAMA_CPP_MODEL" \ '{model: $model, messages: [{role: "user", content: $content}]}' | \ - curl -s --max-time 120 -X POST "${LLAMA_CPP_HOST}/v1/chat/completions" \ - -H "Content-Type: application/json" -d @- 2>/dev/null | \ - jq -r '.choices[0].message.content // ""' 2>/dev/null || echo "") + curl -sS --max-time 120 -X POST "${LLAMA_CPP_HOST}/v1/chat/completions" \ + -H "Content-Type: application/json" -d @- 2>{run_dir}/gsd-review-llama_cpp.err) +LLAMA_CPP_CONTENT=$(echo "$LLAMA_CPP_RESPONSE" | jq -r '.choices[0].message.content // ""' 2>/dev/null || echo "") +# Whitespace-only reply counts as empty; printf not echo. See the LM Studio leg +# above for why both are required. +case "$LLAMA_CPP_CONTENT" in + *[![:space:]]*) : ;; + *) LLAMA_CPP_CONTENT="" ;; +esac if [ -n "$LLAMA_CPP_CONTENT" ]; then - echo "$LLAMA_CPP_CONTENT" > {run_dir}/gsd-review-llama_cpp.md -else - echo "Warning: llama.cpp returned empty content — skipping review." >&2 + printf '%s\n' "$LLAMA_CPP_CONTENT" > {run_dir}/gsd-review-llama_cpp.md fi +if [ ! -s {run_dir}/gsd-review-llama_cpp.md ]; then + echo "Warning: llama.cpp returned empty content — see {run_dir}/gsd-review-llama_cpp.md" >&2 + echo "llama.cpp review failed or returned empty output. stderr:" > {run_dir}/gsd-review-llama_cpp.md + cat {run_dir}/gsd-review-llama_cpp.err >> {run_dir}/gsd-review-llama_cpp.md 2>/dev/null + echo "Raw response body:" >> {run_dir}/gsd-review-llama_cpp.md + printf '%s\n' "$LLAMA_CPP_RESPONSE" >> {run_dir}/gsd-review-llama_cpp.md +fi +else +echo "llama.cpp review skipped: prompt budget (${LLAMA_CPP_REVIEWER_BUDGET} tokens) too small for the minimum review set." > {run_dir}/gsd-review-llama_cpp.md fi ``` diff --git a/tests/fix-2605-review-local-server-empty-guard.test.cjs b/tests/fix-2605-review-local-server-empty-guard.test.cjs new file mode 100644 index 000000000..dff5144e7 --- /dev/null +++ b/tests/fix-2605-review-local-server-empty-guard.test.cjs @@ -0,0 +1,399 @@ +// allow-test-rule: source-text-is-the-product (see #2605) +// The lm_studio and llama_cpp reviewer dispatch blocks in gsd-core/workflows/review.md +// ARE the runtime contract — the workflow's text is what the reviewing agent executes. +// This suite extracts those two shell blocks verbatim from the workflow and runs them +// under a real bash against a stubbed curl, so the shipped guard is what gets exercised +// rather than a reimplementation of it. The assertions on the produced review file are +// assertions on that guard's documented output contract (the stub line the consensus +// step must be able to tell apart from a clean empty review), not incidental string +// matching. + +/** + * Regression tests for #2605 — the lm_studio and llama_cpp reviewer legs dropped + * empty output with no stub file, the same defect class as #2494 (claude/gemini) + * but a worse variant. + * + * Before the fix both blocks ran `curl -s ... 2>/dev/null` and, when the model + * returned empty content, wrote NOTHING to + * `{run_dir}/gsd-review-.md` — there was no `[ ! -s … ]` stub at all. The + * review file therefore never existed, `write_reviews` silently omitted that + * reviewer's section, and the outcome was indistinguishable from the reviewer + * never having been selected. + * + * Two distinct diagnostic holes are covered here, because an OpenAI-compatible + * server can fail in two ways that leave evidence in different places: + * - transport failure (endpoint unreachable): curl writes to STDERR and exits + * non-zero. `-s` suppressed that error text entirely, so `-sS` is required. + * - application failure (HTTP 4xx/5xx): curl exits 0 and the error JSON is in + * the response BODY, so stderr is empty and only the body is diagnosable. + * The llama_cpp leg additionally piped curl straight into jq, discarding the + * body before anything could inspect it. + * + * These tests fail against pre-fix review.md: no review file is produced at all, + * so the existence assertion trips first. + */ + +'use strict'; + +const { describe, test, before, after } = require('node:test'); +const assert = require('node:assert/strict'); +const { spawnSync, execFileSync } = require('node:child_process'); +const fs = require('node:fs'); +const path = require('node:path'); +const { createTempDir, cleanup } = require('./helpers.cjs'); + +const ROOT = path.join(__dirname, '..'); +const REVIEW_PATH = path.join(ROOT, 'gsd-core', 'workflows', 'review.md'); + +// Normalize CRLF: on a Windows git-autocrlf checkout every line carries a +// trailing \r, which would leave the extracted block's redirect tokens mangled +// and defeat the fence regexes below. +const WORKFLOW = fs.readFileSync(REVIEW_PATH, 'utf-8').replace(/\r\n/g, '\n'); + +// These blocks shell out to a real `jq` (the request body is built with it). +// Gate to jq-present non-Windows hosts, mirroring the opencode reconstruction +// suite — the guard logic is platform-independent and is asserted in full on +// every macOS/Linux CI leg. +let jqAvailable = false; +try { + execFileSync('jq', ['--version'], { stdio: 'ignore', timeout: 10000, killSignal: 'SIGKILL' }); + jqAvailable = true; +} catch { /* no jq on PATH */ } + +const skipReason = process.platform === 'win32' + ? 'extracted block is POSIX shell; guard logic is platform-independent and asserted on macOS/Linux' + : (jqAvailable ? false : 'jq not on PATH'); + +/** + * Extract a reviewer dispatch block verbatim from the workflow. If review.md + * changes the block's shape these throw and the test fails loudly — intended + * coupling, the same contract the #2494 suite pins for the claude/gemini legs. + */ +function extractBlock(headingRe, label) { + // Some legs (Ollama, CodeRabbit) put an explanatory paragraph between the + // heading and the fence, so allow non-fence content in between. The lazy + // quantifiers take the FIRST ```bash fence after the heading, which is that + // leg's own block. + const re = new RegExp(`${headingRe}\\n[\\s\\S]*?\`\`\`bash\\n([\\s\\S]*?)\\n\`\`\``); + const m = WORKFLOW.match(re); + assert.ok(m, `review.md must define the ${label} reviewer dispatch as a bash block (#2605)`); + return m[1]; +} + +const LEGS = [ + { + key: 'lm_studio', + label: 'LM Studio', + budgetVar: 'LM_STUDIO_REVIEWER_BUDGET', + block: extractBlock('\\*\\*LM Studio \\(local, OpenAI-compatible\\):\\*\\*', 'LM Studio'), + }, + { + key: 'llama_cpp', + label: 'llama.cpp', + budgetVar: 'LLAMA_CPP_REVIEWER_BUDGET', + block: extractBlock('\\*\\*llama\\.cpp \\(local, OpenAI-compatible\\):\\*\\*', 'llama.cpp'), + }, + // Ollama was the least diagnosable of the three local-server legs (bare `-s`, + // stderr to /dev/null, response piped straight into jq). It emitted a stub, so + // it never silently vanished — but it is the same family and is held to the + // same contract here. + { + key: 'ollama', + label: 'Ollama', + budgetVar: 'OLLAMA_REVIEWER_BUDGET', + block: extractBlock('\\*\\*Ollama \\(local, OpenAI-compatible\\):\\*\\*', 'Ollama'), + }, +]; + +const STUB_STDERR = 'gsd-2605-stub: curl: (7) Failed to connect to localhost'; +const STUB_ERROR_BODY = '{"error":{"message":"gsd-2605-stub: model not loaded","code":503}}'; + +let sandbox; + +before(() => { sandbox = createTempDir('gsd-2605-'); }); +after(() => { cleanup(sandbox); }); + +/** + * Run one extracted block with `{run_dir}` pointed at a fresh run directory and + * `curlBody` installed on PATH as `curl`. + * + * `gsd_run` is deliberately NOT stubbed: every call site is + * `$(gsd_run … || echo "")`, so an absent binary takes the documented + * default path (no prompt budget, default host, model probed from the server) — + * which is the configuration the issue reproduces against. + * + * Single exec site so the Windows guard lives in one place: Git Bash (msys2) + * ignores Node's chmod exec bit for PATH-executed extension-less scripts + * (DEFECT.WINDOWS-TEST-PORTABILITY), and every suite below is skipped on win32 — + * this early return keeps the exec unreachable there rather than relying on the + * skip alone. + */ +function runLeg({ leg, curlBody, preamble = '' }) { + if (process.platform === 'win32') return null; + if (!jqAvailable) return null; + + const caseDir = fs.mkdtempSync(path.join(sandbox, 'run-')); + const runDir = path.join(caseDir, 'run'); + const binDir = path.join(caseDir, 'bin'); + fs.mkdirSync(runDir); + fs.mkdirSync(binDir); + + const stub = path.join(binDir, 'curl'); + fs.writeFileSync(stub, curlBody); + fs.chmodSync(stub, 0o755); + + fs.writeFileSync(path.join(runDir, 'gsd-review-prompt.md'), '# review prompt\n'); + + const script = preamble + leg.block.split('{run_dir}').join(runDir); + const result = spawnSync('bash', ['-c', script], { + encoding: 'utf8', + timeout: 30000, + killSignal: 'SIGKILL', + env: { ...process.env, PATH: `${binDir}${path.delimiter}${process.env.PATH}` }, + }); + + const reviewPath = path.join(runDir, `gsd-review-${leg.key}.md`); + return { + result, + reviewPath, + errPath: path.join(runDir, `gsd-review-${leg.key}.err`), + review: fs.existsSync(reviewPath) ? fs.readFileSync(reviewPath, 'utf-8') : null, + }; +} + +/** + * A curl stub. The block may call curl twice — once to probe `/v1/models` for a + * model id, once to POST `/v1/chat/completions` — so the stub branches on the URL + * and only applies the failure mode to the completion call. + */ +function curlStub({ completionStdout = '', completionStderr = '', exitCode = 0 }) { + return `#!/bin/sh +for a in "$@"; do + case "$a" in + */v1/models) echo '{"data":[{"id":"stub-model"}]}'; exit 0 ;; + esac +done +${completionStderr ? `echo "${completionStderr}" >&2` : ':'} +${completionStdout ? `cat <<'GSD_EOF'\n${completionStdout}\nGSD_EOF` : ':'} +exit ${exitCode} +`; +} + +/** + * The guard's contract: the review file EXISTS (the #2605 core defect — it did + * not), is not empty, and names the leg as failed-or-empty so consensus + * synthesis can tell it apart from "ran cleanly, nothing to report". + */ +function assertDiagnosable(out, legLabel) { + assert.ok(out.review !== null, `${legLabel}: review file must exist after a failed lane — its absence is what write_reviews silently omitted (#2605)`); + assert.notStrictEqual(out.review.trim(), '', `${legLabel}: review file must not be empty after a failed lane (#2605)`); + assert.match( + out.review, + new RegExp(`${legLabel.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')} review failed or returned empty output`, 'i'), + `${legLabel}: review file must carry a diagnosable failure line (#2605)`, + ); +} + +for (const leg of LEGS) { + describe(`#2605 — ${leg.label} reviewer leg fails loudly`, { skip: skipReason }, () => { + test('an unreachable endpoint produces a diagnosable stub carrying curl stderr', () => { + // Transport failure: curl writes to stderr and exits non-zero. `-s` alone + // would have suppressed this text, which is why the fix uses `-sS`. + const out = runLeg({ + leg, + curlBody: curlStub({ completionStderr: STUB_STDERR, exitCode: 7 }), + }); + + assertDiagnosable(out, leg.label); + assert.ok( + out.review.includes(STUB_STDERR), + `${leg.label}: captured stderr must be appended to the review file, not discarded to /dev/null (#2605)`, + ); + assert.ok( + fs.existsSync(out.errPath), + `${leg.label}: stderr must be captured to a .err sidecar (#2605)`, + ); + }); + + test('an HTTP error body produces a diagnosable stub carrying the raw response', () => { + // Application failure: an OpenAI-compatible server returns its error JSON in + // the BODY and curl exits 0, so stderr is empty and only the body is + // evidence. The llama_cpp leg used to pipe curl straight into jq, throwing + // the body away before anything could inspect it. + const out = runLeg({ + leg, + curlBody: curlStub({ completionStdout: STUB_ERROR_BODY, exitCode: 0 }), + }); + + assertDiagnosable(out, leg.label); + assert.ok( + out.review.includes('gsd-2605-stub: model not loaded'), + `${leg.label}: the raw response body must be preserved — it is the only evidence when curl exits 0 (#2605)`, + ); + }); + + test('an empty 200 response still produces a stub rather than no file', () => { + // Boundary: a well-formed response whose content is the empty string. This + // is the exact case the issue reproduces — the old code took the `else` + // branch and wrote nothing at all. + const out = runLeg({ + leg, + curlBody: curlStub({ + completionStdout: '{"choices":[{"message":{"content":""}}]}', + exitCode: 0, + }), + }); + + assertDiagnosable(out, leg.label); + }); + + test('a whitespace-only response is treated as empty, not as a successful review', () => { + // `[ ! -s … ]` counts BYTES, so a reply of " " was written out and passed + // the guard as a "successful" but vacuous review — the same + // indistinguishable-from-success outcome the guard exists to prevent. + // Command substitution strips trailing newlines but NOT spaces, so this + // case is not covered by the empty-string case above. + const out = runLeg({ + leg, + curlBody: curlStub({ + completionStdout: '{"choices":[{"message":{"content":" "}}]}', + exitCode: 0, + }), + }); + + assertDiagnosable(out, leg.label); + }); + + test('a reply that is exactly an echo option is not misclassified as empty', () => { + // `echo "$VAR" > file` writes 0 bytes when VAR is exactly `-n`, which would + // trip the empty guard and DISCARD a genuine reply. printf is required. + const out = runLeg({ + leg, + curlBody: curlStub({ + completionStdout: '{"choices":[{"message":{"content":"-n"}}]}', + exitCode: 0, + }), + }); + + assert.ok(out.review !== null, `${leg.label}: a reply of "-n" must still produce a review file (#2605)`); + assert.ok( + out.review.includes('-n'), + `${leg.label}: a reply of "-n" must be written verbatim, not swallowed by echo's option parsing (#2605)`, + ); + assert.ok( + !/failed or returned empty output/i.test(out.review), + `${leg.label}: a genuine "-n" reply must not be misclassified as empty (#2605)`, + ); + }); + + test('a budget skip leaves a visible stub rather than no file', () => { + // The skip path sits one `if` away from the guard and dropped the lane just + // as silently: no file, so write_reviews omitted the section entirely and + // the only trace was a stderr warning nothing persists. + const out = runLeg({ + leg, + curlBody: curlStub({ completionStdout: '{"choices":[{"message":{"content":"unused"}}]}' }), + // `gsd_run` supplies the budget, so stubbing the variable directly is + // useless — the block's first line overwrites it. Drive the skip through + // `gsd_run` itself: `config-get` yields a non-null budget so the trim + // branch is entered, and `prompt-budget` returns 2 — the documented + // "budget too small for the minimum review set" code. Stubbing only the + // helper would not work for the Ollama leg, whose fence also carries the + // shared `prepare_trimmed_prompt_for_reviewer` definition and so + // overrides any stub of it. + preamble: 'gsd_run() { case "$2" in prompt-budget) return 2 ;; esac; echo 1; }\n' + + 'prepare_trimmed_prompt_for_reviewer() { return 2; }\n', + }); + + assert.ok(out.review !== null, `${leg.label}: a budget-skipped lane must still produce a review file (#2605)`); + assert.match( + out.review, + /review skipped: prompt budget/i, + `${leg.label}: a budget-skipped lane must say so in the review file (#2605)`, + ); + }); + + test('a successful review passes through untouched', () => { + // The guard must not fire on real content, and must not wrap or annotate it. + const out = runLeg({ + leg, + curlBody: curlStub({ + completionStdout: '{"choices":[{"message":{"content":"## Real Review\\nLooks good."}}]}', + exitCode: 0, + }), + }); + + assert.ok(out.review !== null, `${leg.label}: a successful review must produce a review file (#2605)`); + assert.ok( + out.review.includes('Looks good.'), + `${leg.label}: a successful review must pass through untouched (#2605)`, + ); + assert.ok( + !/failed or returned empty output/i.test(out.review), + `${leg.label}: the stub must not fire on a non-empty review (#2605)`, + ); + }); + }); +} + +describe('#2605 — the CodeRabbit leg fails loudly', { skip: skipReason }, () => { + // CodeRabbit was the last CLI leg still shaped like pre-#2494 code: + // `2>/dev/null > file` with no stub. A missing or unauthenticated binary left a + // zero-byte file that write_reviews rendered as "ran cleanly, nothing to report". + const CODERABBIT_BLOCK = extractBlock('\\*\\*CodeRabbit:\\*\\*', 'CodeRabbit'); + + test('a failing coderabbit CLI produces a diagnosable stub, not a zero-byte file', () => { + if (process.platform === 'win32') return; + + const caseDir = fs.mkdtempSync(path.join(sandbox, 'cr-')); + const runDir = path.join(caseDir, 'run'); + const binDir = path.join(caseDir, 'bin'); + fs.mkdirSync(runDir); + fs.mkdirSync(binDir); + + const stub = path.join(binDir, 'coderabbit'); + fs.writeFileSync(stub, `#!/bin/sh\necho "${STUB_STDERR}" >&2\nexit 127\n`); + fs.chmodSync(stub, 0o755); + + const script = CODERABBIT_BLOCK.split('{run_dir}').join(runDir); + spawnSync('bash', ['-c', script], { + encoding: 'utf8', + timeout: 30000, + killSignal: 'SIGKILL', + env: { ...process.env, PATH: `${binDir}${path.delimiter}${process.env.PATH}` }, + }); + + const reviewPath = path.join(runDir, 'gsd-review-coderabbit.md'); + assert.ok(fs.existsSync(reviewPath), 'CodeRabbit: review file must exist after a failed lane (#2605)'); + const review = fs.readFileSync(reviewPath, 'utf-8'); + assert.notStrictEqual(review.trim(), '', 'CodeRabbit: review file must not be zero-byte after a failed lane (#2605)'); + assert.match( + review, + /CodeRabbit review failed or returned empty output/i, + 'CodeRabbit: review file must carry a diagnosable failure line (#2605)', + ); + assert.ok( + review.includes(STUB_STDERR), + 'CodeRabbit: captured stderr must be appended, not discarded to /dev/null (#2605)', + ); + }); +}); + +describe('#2605 — every local-server leg uses -sS so curl errors are not suppressed', { skip: skipReason }, () => { + test('the chat/completions call captures stderr to a sidecar instead of /dev/null', () => { + // Pins the two changes that make the stub's evidence real rather than empty. + // Asserted on the extracted block text because the redirect target is the + // contract; the behavioural consequence is covered by the suites above. + for (const leg of LEGS) { + assert.match( + leg.block, + new RegExp(`curl -sS[^\\n]*\\n[^\\n]*-d @- 2>[^\\n]*gsd-review-${leg.key}\\.err`), + `${leg.label}: the completion call must use -sS and redirect stderr to its .err sidecar (#2605)`, + ); + assert.ok( + !new RegExp(`curl -s --max-time 120`).test(leg.block), + `${leg.label}: the completion call must not use bare -s, which suppresses curl's error text (#2605)`, + ); + } + }); +}); diff --git a/tests/fixtures/golden-install-parity/antigravity.json b/tests/fixtures/golden-install-parity/antigravity.json index c64193e13..609401ac5 100644 --- a/tests/fixtures/golden-install-parity/antigravity.json +++ b/tests/fixtures/golden-install-parity/antigravity.json @@ -301,7 +301,7 @@ "gsd-core/workflows/remove-phase.md": "23b9eb0858a2535e", "gsd-core/workflows/remove-workspace.md": "1058d1d3160eb120", "gsd-core/workflows/resume-project.md": "98e2cf8908e73a52", - "gsd-core/workflows/review.md": "8ca65b75fb2aa8e2", + "gsd-core/workflows/review.md": "0c3c483764110a1c", "gsd-core/workflows/scan.md": "46c5a73f6f682023", "gsd-core/workflows/secure-phase.md": "9564c529052d1d36", "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 71ebb9dd9..8b2bf199e 100644 --- a/tests/fixtures/golden-install-parity/augment.json +++ b/tests/fixtures/golden-install-parity/augment.json @@ -372,7 +372,7 @@ "gsd-core/workflows/remove-phase.md": "df9a45f0b1880999", "gsd-core/workflows/remove-workspace.md": "f67860c795fb4bfe", "gsd-core/workflows/resume-project.md": "f28da1200e4545f4", - "gsd-core/workflows/review.md": "0e440626c82bd1d9", + "gsd-core/workflows/review.md": "8724993cbc91bcc5", "gsd-core/workflows/scan.md": "e1c12d542e61720d", "gsd-core/workflows/secure-phase.md": "fda2361739e511ee", "gsd-core/workflows/session-report.md": "2e5b1205324ddefa", diff --git a/tests/fixtures/golden-install-parity/claude-local.json b/tests/fixtures/golden-install-parity/claude-local.json index b668f8989..87605b5e9 100644 --- a/tests/fixtures/golden-install-parity/claude-local.json +++ b/tests/fixtures/golden-install-parity/claude-local.json @@ -371,7 +371,7 @@ "gsd-core/workflows/remove-phase.md": "8effc8742d58a11a", "gsd-core/workflows/remove-workspace.md": "64b73daff58b9aec", "gsd-core/workflows/resume-project.md": "af9761bcec0f6fe9", - "gsd-core/workflows/review.md": "9c3ca42e01b7eb72", + "gsd-core/workflows/review.md": "73f78512f2c33aed", "gsd-core/workflows/scan.md": "686e787d3704db90", "gsd-core/workflows/secure-phase.md": "a7272ff8163a61ac", "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 76b031bef..61c0fcd53 100644 --- a/tests/fixtures/golden-install-parity/claude.json +++ b/tests/fixtures/golden-install-parity/claude.json @@ -300,7 +300,7 @@ "gsd-core/workflows/remove-phase.md": "ada8a0546c686483", "gsd-core/workflows/remove-workspace.md": "015cde237c75be39", "gsd-core/workflows/resume-project.md": "7f8dc986f0f35d96", - "gsd-core/workflows/review.md": "4e1ee8ab952db2dc", + "gsd-core/workflows/review.md": "9f271477cf84c34b", "gsd-core/workflows/scan.md": "a71e3009998cfc57", "gsd-core/workflows/secure-phase.md": "ba807b4862d7f3c9", "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 74a6d866e..56fb268d8 100644 --- a/tests/fixtures/golden-install-parity/cline.json +++ b/tests/fixtures/golden-install-parity/cline.json @@ -304,7 +304,7 @@ "gsd-core/workflows/remove-phase.md": "e336350f8113a328", "gsd-core/workflows/remove-workspace.md": "79d3669cc9eb0b10", "gsd-core/workflows/resume-project.md": "e23981178fa37b3d", - "gsd-core/workflows/review.md": "5f9ffdb708203c27", + "gsd-core/workflows/review.md": "9f7d0f99ed40205b", "gsd-core/workflows/scan.md": "f0bd2f64f5530598", "gsd-core/workflows/secure-phase.md": "1a2e389991fc6263", "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 63e7a3f4b..405e082cf 100644 --- a/tests/fixtures/golden-install-parity/codebuddy.json +++ b/tests/fixtures/golden-install-parity/codebuddy.json @@ -372,7 +372,7 @@ "gsd-core/workflows/remove-phase.md": "df9a45f0b1880999", "gsd-core/workflows/remove-workspace.md": "f67860c795fb4bfe", "gsd-core/workflows/resume-project.md": "f28da1200e4545f4", - "gsd-core/workflows/review.md": "0e440626c82bd1d9", + "gsd-core/workflows/review.md": "8724993cbc91bcc5", "gsd-core/workflows/scan.md": "e1c12d542e61720d", "gsd-core/workflows/secure-phase.md": "fda2361739e511ee", "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 10ece515d..3c8c42755 100644 --- a/tests/fixtures/golden-install-parity/codex.json +++ b/tests/fixtures/golden-install-parity/codex.json @@ -407,7 +407,7 @@ "gsd-core/workflows/remove-phase.md": "9ee0fddd11a0d9d4", "gsd-core/workflows/remove-workspace.md": "d0160c5d05bcb2bb", "gsd-core/workflows/resume-project.md": "9965f87eb278f7f8", - "gsd-core/workflows/review.md": "b16cb18b657562ae", + "gsd-core/workflows/review.md": "f2d0cf16167cb107", "gsd-core/workflows/scan.md": "dcd6aac25ef39251", "gsd-core/workflows/secure-phase.md": "3ba89719288dd3f3", "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 9d19fe004..f1516381e 100644 --- a/tests/fixtures/golden-install-parity/copilot.json +++ b/tests/fixtures/golden-install-parity/copilot.json @@ -302,7 +302,7 @@ "gsd-core/workflows/remove-phase.md": "e262654e319d1bc4", "gsd-core/workflows/remove-workspace.md": "e7e5b5b1e0cc9d81", "gsd-core/workflows/resume-project.md": "40db7f350f5866d8", - "gsd-core/workflows/review.md": "eaafaf8ba23f356e", + "gsd-core/workflows/review.md": "09f390a2f11b40d3", "gsd-core/workflows/scan.md": "76aad4e70281c364", "gsd-core/workflows/secure-phase.md": "d60aa4053154e52f", "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 a1edbea77..158f8c7fa 100644 --- a/tests/fixtures/golden-install-parity/cursor.json +++ b/tests/fixtures/golden-install-parity/cursor.json @@ -372,7 +372,7 @@ "gsd-core/workflows/remove-phase.md": "ada8a0546c686483", "gsd-core/workflows/remove-workspace.md": "0572a83d71650937", "gsd-core/workflows/resume-project.md": "7f8dc986f0f35d96", - "gsd-core/workflows/review.md": "f2f9f47f6b9a7c88", + "gsd-core/workflows/review.md": "907cf2747dfb833d", "gsd-core/workflows/scan.md": "a71e3009998cfc57", "gsd-core/workflows/secure-phase.md": "b008216148d2afac", "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 8c3d6b3b5..bb826cc3c 100644 --- a/tests/fixtures/golden-install-parity/hermes.json +++ b/tests/fixtures/golden-install-parity/hermes.json @@ -301,7 +301,7 @@ "gsd-core/workflows/remove-phase.md": "fce799aae3ab2715", "gsd-core/workflows/remove-workspace.md": "42029a7559f1d8fb", "gsd-core/workflows/resume-project.md": "a0443839f1f83c2d", - "gsd-core/workflows/review.md": "5d8751246d8c9dc2", + "gsd-core/workflows/review.md": "272f7d8befd51657", "gsd-core/workflows/scan.md": "5c2370d6a8118b6c", "gsd-core/workflows/secure-phase.md": "c087131f1dd12901", "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 201441977..cd13ab426 100644 --- a/tests/fixtures/golden-install-parity/kilo.json +++ b/tests/fixtures/golden-install-parity/kilo.json @@ -372,7 +372,7 @@ "gsd-core/workflows/remove-phase.md": "ada8a0546c686483", "gsd-core/workflows/remove-workspace.md": "f5dc82bb63e2efa4", "gsd-core/workflows/resume-project.md": "7f8dc986f0f35d96", - "gsd-core/workflows/review.md": "c1152ff4aa9e52a4", + "gsd-core/workflows/review.md": "cb218a57c772a041", "gsd-core/workflows/scan.md": "c039d3e40d26b606", "gsd-core/workflows/secure-phase.md": "5a8fbf603d218100", "gsd-core/workflows/session-report.md": "2e5b1205324ddefa", diff --git a/tests/fixtures/golden-install-parity/kimi-code.json b/tests/fixtures/golden-install-parity/kimi-code.json index e79431089..0c6483a57 100644 --- a/tests/fixtures/golden-install-parity/kimi-code.json +++ b/tests/fixtures/golden-install-parity/kimi-code.json @@ -330,7 +330,7 @@ "gsd-core/workflows/remove-phase.md": "df9a45f0b1880999", "gsd-core/workflows/remove-workspace.md": "f67860c795fb4bfe", "gsd-core/workflows/resume-project.md": "f28da1200e4545f4", - "gsd-core/workflows/review.md": "0e440626c82bd1d9", + "gsd-core/workflows/review.md": "8724993cbc91bcc5", "gsd-core/workflows/scan.md": "e1c12d542e61720d", "gsd-core/workflows/secure-phase.md": "fda2361739e511ee", "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 d04b3f703..64d3872fc 100644 --- a/tests/fixtures/golden-install-parity/kimi.json +++ b/tests/fixtures/golden-install-parity/kimi.json @@ -366,7 +366,7 @@ "gsd-core/workflows/remove-phase.md": "df9a45f0b1880999", "gsd-core/workflows/remove-workspace.md": "f67860c795fb4bfe", "gsd-core/workflows/resume-project.md": "f28da1200e4545f4", - "gsd-core/workflows/review.md": "0e440626c82bd1d9", + "gsd-core/workflows/review.md": "8724993cbc91bcc5", "gsd-core/workflows/scan.md": "e1c12d542e61720d", "gsd-core/workflows/secure-phase.md": "fda2361739e511ee", "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 cbef8fd61..5e17044ce 100644 --- a/tests/fixtures/golden-install-parity/opencode.json +++ b/tests/fixtures/golden-install-parity/opencode.json @@ -372,7 +372,7 @@ "gsd-core/workflows/remove-phase.md": "dea4661e8f89596f", "gsd-core/workflows/remove-workspace.md": "21a8581add5f31ae", "gsd-core/workflows/resume-project.md": "ad9f06a10bab8cc0", - "gsd-core/workflows/review.md": "1b3bf694dcace23e", + "gsd-core/workflows/review.md": "c775d08988c05be6", "gsd-core/workflows/scan.md": "cbfb79df855e5e61", "gsd-core/workflows/secure-phase.md": "ef09f40dfd4d2424", "gsd-core/workflows/session-report.md": "2e5b1205324ddefa", diff --git a/tests/fixtures/golden-install-parity/pi.json b/tests/fixtures/golden-install-parity/pi.json index 85f741d81..75fac746a 100644 --- a/tests/fixtures/golden-install-parity/pi.json +++ b/tests/fixtures/golden-install-parity/pi.json @@ -268,7 +268,7 @@ "gsd-core/workflows/remove-phase.md": "df9a45f0b1880999", "gsd-core/workflows/remove-workspace.md": "f67860c795fb4bfe", "gsd-core/workflows/resume-project.md": "f28da1200e4545f4", - "gsd-core/workflows/review.md": "0e440626c82bd1d9", + "gsd-core/workflows/review.md": "8724993cbc91bcc5", "gsd-core/workflows/scan.md": "e1c12d542e61720d", "gsd-core/workflows/secure-phase.md": "fda2361739e511ee", "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 b86b9531f..e06b16f02 100644 --- a/tests/fixtures/golden-install-parity/qwen.json +++ b/tests/fixtures/golden-install-parity/qwen.json @@ -301,7 +301,7 @@ "gsd-core/workflows/remove-phase.md": "e8ae4fbbfac700f0", "gsd-core/workflows/remove-workspace.md": "ae520235f4d1f4a5", "gsd-core/workflows/resume-project.md": "7f20769f302e5427", - "gsd-core/workflows/review.md": "7412a3b9ff1b0291", + "gsd-core/workflows/review.md": "c6d7f9e26af9fbdc", "gsd-core/workflows/scan.md": "b7efd0d381a3b8a5", "gsd-core/workflows/secure-phase.md": "f7bfa7175102af31", "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 bd4c06439..bee521ddc 100644 --- a/tests/fixtures/golden-install-parity/trae.json +++ b/tests/fixtures/golden-install-parity/trae.json @@ -301,7 +301,7 @@ "gsd-core/workflows/remove-phase.md": "a46c2fe853bf4e86", "gsd-core/workflows/remove-workspace.md": "8ddc5f04f7c48d6c", "gsd-core/workflows/resume-project.md": "f242e4c8aba18ea2", - "gsd-core/workflows/review.md": "96deafef432bef80", + "gsd-core/workflows/review.md": "663197fee51897e4", "gsd-core/workflows/scan.md": "3b14bcb51d3a4de8", "gsd-core/workflows/secure-phase.md": "267393d5b02b5334", "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 f4a8fc664..ca66b6254 100644 --- a/tests/fixtures/golden-install-parity/windsurf.json +++ b/tests/fixtures/golden-install-parity/windsurf.json @@ -301,7 +301,7 @@ "gsd-core/workflows/remove-phase.md": "e7a6af429b36e77b", "gsd-core/workflows/remove-workspace.md": "30ca05fe4a3efc25", "gsd-core/workflows/resume-project.md": "82cfe1b8cb17c085", - "gsd-core/workflows/review.md": "2d067a516e323db9", + "gsd-core/workflows/review.md": "8a9380fa5a5a3b12", "gsd-core/workflows/scan.md": "4a910da5e34f2685", "gsd-core/workflows/secure-phase.md": "d5d811bc468ce7f2", "gsd-core/workflows/session-report.md": "2e5b1205324ddefa", diff --git a/tests/fixtures/golden-install-parity/zcode.json b/tests/fixtures/golden-install-parity/zcode.json index dee90b414..245775a94 100644 --- a/tests/fixtures/golden-install-parity/zcode.json +++ b/tests/fixtures/golden-install-parity/zcode.json @@ -372,7 +372,7 @@ "gsd-core/workflows/remove-phase.md": "df9a45f0b1880999", "gsd-core/workflows/remove-workspace.md": "f67860c795fb4bfe", "gsd-core/workflows/resume-project.md": "f28da1200e4545f4", - "gsd-core/workflows/review.md": "0e440626c82bd1d9", + "gsd-core/workflows/review.md": "8724993cbc91bcc5", "gsd-core/workflows/scan.md": "e1c12d542e61720d", "gsd-core/workflows/secure-phase.md": "fda2361739e511ee", "gsd-core/workflows/session-report.md": "2e5b1205324ddefa", diff --git a/tests/workflow-size-baseline.json b/tests/workflow-size-baseline.json index b78cff11f..94f8b76c0 100644 --- a/tests/workflow-size-baseline.json +++ b/tests/workflow-size-baseline.json @@ -64,7 +64,7 @@ "remove-phase.md": 8513, "remove-workspace.md": 7916, "resume-project.md": 17270, - "review.md": 54036, + "review.md": 59213, "scan.md": 8314, "secure-phase.md": 14627, "session-report.md": 4044,