golem15/msd-core
4
0
Fork 0
Code Issues 1 Pull Requests Actions 3 Packages Projects Releases Wiki Activity

fix(#2605): make dropped local-server reviewer lanes loud instead of silent (#2689)

Browse Source 
* fix(#2605): make dropped lm_studio / llama_cpp reviewer lanes loud, not silent

The `lm_studio` and `llama_cpp` reviewer legs in `gsd-core/workflows/review.md`
carried the same empty-output defect as the claude/gemini legs (#2494, fixed in
#2592) in a worse variant: when the local endpoint was unreachable or returned
empty content, they wrote NOTHING to `{run_dir}/gsd-review-<leg>.md`. There was
no `[ ! -s … ]` stub at all, so the file never existed, `write_reviews` omitted
that reviewer's section, and the outcome was indistinguishable from the reviewer
never having been selected.

Two diagnostic holes are closed, because an OpenAI-compatible server fails in two
ways that leave evidence in different places:

- Transport failure (endpoint unreachable): curl writes to stderr and exits
  non-zero. Both legs used `curl -s`, which suppresses curl's ERROR text as well
  as the progress meter, and then discarded stderr to `/dev/null` — so there was
  nothing to capture even in principle. Now `-sS` with stderr to a `.err`
  sidecar, matching the claude/gemini/codex legs.
- 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 evidence. The stub
  appends the raw response. The `llama_cpp` leg additionally piped curl straight
  into `jq`, throwing the body away before anything could inspect it; the
  response is now captured to a variable first, as the `lm_studio` leg already
  did.

The existing `>&2` warning is kept and now points at the stub file.

Failing-first verified by extracting both shipped blocks and running them under a
real bash against a stubbed curl: pre-fix, all three failure modes produce NO
review file and no `.err` sidecar for both legs; post-fix, each produces a
diagnosable stub, and a successful review still passes through untouched.

Closes #2605

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf

* fix(#2605): close the remaining silent-drop paths the review surfaced

Four defects found by the orthogonal review of the first commit, all fixed here
rather than deferred (they are the same defect class the issue is about, and
three of them sit inside the code that commit touched).

1. CodeRabbit leg was still unguarded. `coderabbit review --prompt-only
   2>/dev/null > file` with no `[ ! -s … ]` stub — the last leg still shaped like
   pre-#2494 code. A missing or unauthenticated binary left a zero-byte file that
   write_reviews rendered as "ran cleanly, nothing to report". It now captures
   stderr to a `.err` sidecar and emits the same diagnosable stub as every other
   leg. This is the leg the next issue in the #2494 -> #2592 -> #2605 series
   would have been about.

2. The budget-skip path dropped the lane just as silently. When
   `prepare_trimmed_prompt_for_reviewer` fails, `*_SKIP=1` bypasses the entire
   block — guard included — so no file was written and the only trace was a
   stderr warning nothing persists. All three local-server legs now write a
   "review skipped: prompt budget too small" stub on that path.

3. Whitespace-only replies evaded the guard. `[ ! -s … ]` counts BYTES, and
   command substitution strips trailing newlines but not spaces, so a reply of
   `"   "` was written out and passed as a "successful" but vacuous review — the
   same indistinguishable-from-success outcome the guard exists to prevent. A
   `case` glob now normalizes whitespace-only content to empty.

4. `echo "$VAR"` swallowed option-like content. bash's builtin `echo` treats a
   value of exactly `-n`/`-e`/`-E` as a flag and writes 0 bytes, which would trip
   the empty guard and DISCARD a genuine reply. Switched to `printf '%s\n'`, the
   idiom the OpenCode leg in this same file already uses for this reason.

Also brings the Ollama leg to parity while it is in hand: it always emitted a
stub so it never silently vanished, but it was the least diagnosable of the three
local-server legs — bare `-s`, stderr to /dev/null, and the response piped
straight into jq so the error body was discarded unread.

Verified by extracting all four shipped blocks and running them under a real bash
against stubbed CLIs: 22 cases (7 per local-server leg x 3, plus CodeRabbit) all
produce the contracted output, and a successful review still passes through
untouched on every leg.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf

* test(#2605): regenerate golden install-parity fixtures for the review.md edit

The remote test run on the prior commit failed with 19 "golden parity —
<runtime>" mismatches. review.md is installed into every runtime's tree, so
editing it changes its content hash in all 19 golden fixtures.

Regenerated with `npm run gen:golden`; the diff is exactly one line per fixture
— the gsd-core/workflows/review.md hash — and nothing else.

This is the second ripple of a workflow edit, alongside
tests/workflow-size-baseline.json which the first commit already updated.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf

* chore(#2605): backfill changeset PR number (#2689)

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-07-27 00:31:17 -04:00
committed by GitHub
parent 3eb1cede26
commit 2c44241a0b
23 changed files with 519 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files

115
gsd-core/workflows/review.md
View File

@@ -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
```