From e6b3f7a239d8d032288504cbb1ee73179c4b6ba6 Mon Sep 17 00:00:00 2001 From: Christoph Date: Fri, 24 Jul 2026 16:09:53 +0200 Subject: [PATCH 1/3] fix(review): fall back when a provider rejects response_format Some OpenAI-compatible providers return HTTP 500 on response_format: {type: json_object} (seen with a GLM reasoning model on Runware), so the model review lenses produced no output and the review failed closed even though the connection was fine. The adapter now sends response_format on the first attempt and drops it on retry, leaning on the system prompt and the extractor; it also logs the raw error body on a persistent failure so the cause is named. Test: a provider that 500s on response_format still yields a review via the retry. Signed-off-by: Christoph --- .github/asdd/runtime/openai-compat.sh | 35 +++++++++++++++++----- .github/asdd/runtime/openai-compat.test.sh | 12 ++++++++ CHANGELOG.md | 6 ++++ 3 files changed, 46 insertions(+), 7 deletions(-) diff --git a/.github/asdd/runtime/openai-compat.sh b/.github/asdd/runtime/openai-compat.sh index de85723..99a1a24 100755 --- a/.github/asdd/runtime/openai-compat.sh +++ b/.github/asdd/runtime/openai-compat.sh @@ -62,23 +62,34 @@ extract_json() { return 1 } -payload="$(jq -n --arg m "$ASDD_MODEL" --arg s "$sys" --arg p "$prompt" \ - '{model:$m, temperature:0, - messages:[{role:"system", content:$s},{role:"user", content:$p}], - response_format:{type:"json_object"}}')" +# Request body, with a chooseable response_format. Some OpenAI-compatible providers (seen with a GLM +# reasoning model on Runware) reject response_format with a 5xx, which would kill the review even though +# the system prompt already demands JSON only and the extractor tolerates prose. So the first attempt asks +# for a json_object; if it fails, the remaining attempts drop response_format and lean on the prompt plus +# the extractor. Every mainstream provider still gets response_format on the first, cheapest try. +build_payload() { # $1: 1 = with response_format, 0 = without + if [ "$1" = 1 ]; then + jq -n --arg m "$ASDD_MODEL" --arg s "$sys" --arg p "$prompt" \ + '{model:$m, temperature:0, messages:[{role:"system",content:$s},{role:"user",content:$p}], + response_format:{type:"json_object"}}' + else + jq -n --arg m "$ASDD_MODEL" --arg s "$sys" --arg p "$prompt" \ + '{model:$m, temperature:0, messages:[{role:"system",content:$s},{role:"user",content:$p}]}' + fi +} attempts="${ASDD_MODEL_RETRIES:-3}" case "$attempts" in ''|*[!0-9]*) attempts=3 ;; esac [ "$attempts" -ge 1 ] || attempts=1 nap="${ASDD_MODEL_RETRY_SLEEP:-1}" -result="" +result=""; with_rf=1 for i in $(seq 1 "$attempts"); do http="" resp="$(curl -sS --max-time 180 -w '\n%{http_code}' -X POST "$endpoint" \ -H "Authorization: Bearer $ASDD_RUNTIME_TOKEN" \ -H "Content-Type: application/json" \ - -d "$payload" || true)" + -d "$(build_payload "$with_rf")" || true)" http="${resp##*$'\n'}"; resp="${resp%$'\n'*}" # split the trailing status line off the body # Pull the answer across the shapes providers actually use: content as a string, content as an array of # parts (concatenate .text), or a legacy .text field; THEN append message.reasoning_content. A reasoning @@ -95,7 +106,13 @@ for i in $(seq 1 "$attempts"); do if cand="$(extract_json "$content")"; then result="$cand"; break fi - if [ "$i" -lt "$attempts" ]; then + # This attempt failed. If response_format was still on, drop it for the remaining attempts: a provider + # that rejects it (a 5xx or an error body) then gets a clean try the prompt and extractor can satisfy. + if [ "$with_rf" = 1 ]; then + with_rf=0 + echo "openai-compat: attempt $i failed (HTTP ${http:-n/a}); retrying without response_format" >&2 + sleep "$nap" 2>/dev/null || true + elif [ "$i" -lt "$attempts" ]; then echo "openai-compat: attempt $i returned unparseable output; retrying" >&2 sleep "$nap" 2>/dev/null || true fi @@ -115,6 +132,10 @@ if [ -z "$result" ]; then echo " last HTTP status: ${http:-unknown}; response bytes: ${rlen}; extracted-content chars: ${clen}" echo " content head: ${head_snip}" echo " content tail: ${tail_snip}" + # The raw response body on an error names the cause (an unsupported parameter, a bad model name, a + # quota message). It holds no key (the key is only in the request header) and reviews already-public + # PR content, so a truncated echo is key-safe and turns a blind failure into a named one. + [ "$clen" -eq 0 ] && echo " response body (truncated): $(printf '%s' "$resp" | tr '\n' ' ' | cut -c1-240)" [ "$clen" -eq 0 ] && echo " (content was empty across message.content, content parts, .text, and reasoning_content: check the endpoint returns OpenAI-shaped choices, the model name, and quota.)" } >&2 fi diff --git a/.github/asdd/runtime/openai-compat.test.sh b/.github/asdd/runtime/openai-compat.test.sh index 4cef541..3fe311a 100755 --- a/.github/asdd/runtime/openai-compat.test.sh +++ b/.github/asdd/runtime/openai-compat.test.sh @@ -26,6 +26,11 @@ case "${MODE:-url}" in flaky) # garbage on the first call, a valid object afterwards (proves retry) n=$(cat "$COUNTER" 2>/dev/null || echo 0); n=$((n+1)); echo "$n" > "$COUNTER" if [ "$n" -le 1 ]; then envelope 'nope'; else envelope '{"schema":"asdd/review/v0.1","lenses":[]}'; fi ;; + rf) # a provider that rejects response_format: the call carrying it yields no usable content, the + # retry without it succeeds (proves the response_format fallback) + has_rf=0; for a in "$@"; do case "$a" in *response_format*) has_rf=1;; esac; done + if [ "$has_rf" = 1 ]; then printf '%s\n500' "$(envelope 'provider rejected response_format')" + else printf '%s\n200' "$(envelope '{"schema":"asdd/review/v0.1","lenses":[]}')"; fi ;; esac EOF chmod +x "$STUB/curl" @@ -68,4 +73,11 @@ got="$(MODE=bad ASDD_MODEL_RETRIES=2 PATH="$STUB:$PATH" ASDD_RUNTIME_TOKEN=x ASD ASDD_MODEL_RETRY_SLEEP=0 ASDD_MODEL_URL='https://x.test/v1/chat/completions' bash "$SCRIPT" /dev/null)" [ -z "$got" ] || fail "all-bad responses should yield empty output, got '$got'" +# 6. response_format fallback: a provider that 500s on response_format still gets a review, because the +# retry drops the parameter and the prompt + extractor carry it. +got="$(MODE=rf ASDD_MODEL_RETRIES=3 PATH="$STUB:$PATH" ASDD_RUNTIME_TOKEN=x ASDD_MODEL=m \ + ASDD_MODEL_RETRY_SLEEP=0 ASDD_MODEL_URL='https://x.test/v1/chat/completions' bash "$SCRIPT" "$STUB/err")" +[ "$(printf '%s' "$got" | jq -r .schema)" = "asdd/review/v0.1" ] || fail "response_format fallback did not recover a review (got '$got')" +grep -q 'retrying without response_format' "$STUB/err" || fail "fallback did not drop response_format on the retry" + echo "openai-compat.test.sh: PASS" diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b34667..672f87b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,12 @@ draft, so pin a conformance claim to a commit or date. "update this branch with main" (or the GitHub "Update branch" button) added an unsigned merge commit that failed DCO with no clean fix, because a merge commit cannot be signed without rewriting history. The commit list is now built with `--no-merges`, matching the DCO convention. +- **The review adapter falls back when a provider rejects `response_format`.** Some OpenAI-compatible + providers (seen with a GLM reasoning model on Runware) return HTTP 500 on `response_format: + {type: json_object}`, so the model lenses produced no output and the review failed closed even though + the connection was fine. The adapter now asks for `response_format` on the first attempt and drops it on + retry, leaning on the system prompt and the extractor, and logs the raw error body on a persistent + failure so the cause is named. - **Model calls fall back to `max_completion_tokens`.** A newer OpenAI reasoning model rejects `max_tokens` with a 400 asking for `max_completion_tokens`, so `connect-check` reported it dead and the developer council could not call it. Both now send `max_tokens` first and retry once with the renamed parameter on From adb8cdd2d0a2a25686703dac18ab1daa8adc922a Mon Sep 17 00:00:00 2001 From: Christoph Date: Fri, 24 Jul 2026 17:25:43 +0200 Subject: [PATCH 2/3] feat(review): warn when the reviewer is a heavy reasoning model A reasoning model reasons at length and can exceed a hosted inference window on a real code diff, so the review times out and posts no lenses while trivial diffs pass and look fine (observed live: a GLM reviewer 500s on a real diff). doctor and setup now flag a reasoning reviewer by name and point to a faster one. It is a property of the model, not the host, so it warns regardless of provider. WARN, never a block. Signed-off-by: Christoph --- CHANGELOG.md | 6 ++++++ cli/doctor.py | 43 +++++++++++++++++++++++++++++++++++++++++++ cli/doctor.test.sh | 10 ++++++++++ cli/setup-goose.py | 11 +++++++++++ 4 files changed, 70 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 672f87b..54284a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ draft, so pin a conformance claim to a commit or date. ## [Unreleased] +### Added +- **`doctor` and `setup` warn when the reviewer is a heavy reasoning model.** A reasoning model reasons at + length and can exceed a hosted inference window on a real code diff, so the review times out and posts no + lenses while trivial or docs-only diffs still pass and look fine. The preflight and the setup wizard now + flag a reasoning reviewer (a property of the model, not the host) and point to a faster one. + ### Fixed - **Review runtime recovers the model's JSON.** A reasoning model wraps its review object in analysis prose (with its own braces), code fences, or trailing commentary, or emits it in a separate diff --git a/cli/doctor.py b/cli/doctor.py index 93fcc02..c079a68 100644 --- a/cli/doctor.py +++ b/cli/doctor.py @@ -200,6 +200,48 @@ def check_recipes(rep, config): "re-run `asdd init --goose` to scaffold them") +# Model-name fragments for families that reason at length. A reasoning reviewer can exceed a hosted +# inference window on a substantive diff (observed live: a GLM reviewer returns HTTP 500 "inference timed +# out" on a real code diff, while trivial or docs-only diffs pass and look fine), so the review silently +# produces no lenses. Heuristic name match, WARN not FAIL, and it is a property of the MODEL, not the host. +_REASONING_HINTS = ("glm", "deepseek-r1", "deepseek_r1", "deepseek-reasoner", "reasoner", "qwq", + "-thinking", "thinking-", "magistral", "o1-", "o1@", "o3-", "o3@", "o4-", "minimax-m1") + + +def _model_of(config, role): + """The model for a roster role (models.), read without a YAML dependency (kit text-scan).""" + inblk = False + try: + with open(config) as fh: + for line in fh: + s = line.split("#", 1)[0].rstrip() + if s.strip() == "models:": + inblk = True + continue + if inblk: + if s and not s[0].isspace(): + break + t = s.strip() + if t.startswith(f"{role}:"): + return t[len(role) + 1:].strip().strip('"').strip("'") + except OSError: + return None + return None + + +def check_reviewer_reasoning(rep, config): + """WARN when the reviewer is a heavy reasoning model, which times out reviewing a real diff.""" + model = _model_of(config, "reviewer") + if not model: + return + if any(h in model.lower() for h in _REASONING_HINTS): + rep.add(WARN, f"the reviewer looks like a reasoning model ({model})", + "a reasoning model reasons at length and can exceed a hosted inference window on a real " + "code diff, so the review times out and posts no lenses while trivial diffs still pass", + "set models.reviewer to a faster non-reasoning model, or split the review per lens; the " + "per-call timeout will otherwise fail fast and name this cause") + + def main(): ap = argparse.ArgumentParser(description="Preflight the ASDD Goose operate path.") ap.add_argument("config", nargs="?", default=".asdd.yml", @@ -218,6 +260,7 @@ def main(): check_goose(rep) check_spec_tool(rep, a.config) check_roster(rep, a.config) + check_reviewer_reasoning(rep, a.config) check_conventions(rep, a.config) check_runtime_key(rep) check_recipes(rep, a.config) diff --git a/cli/doctor.test.sh b/cli/doctor.test.sh index 8695c38..06ca292 100644 --- a/cli/doctor.test.sh +++ b/cli/doctor.test.sh @@ -55,4 +55,14 @@ out="$(python3 "$DOC" "$TMP/nope.yml" 2>&1)"; rc=$? [ "$rc" = "1" ] && grep -q 'config not found' <<<"$out" \ && ok "missing config fails cleanly (exit 1)" || bad "missing-config case (exit $rc)" +# A reasoning-model reviewer must WARN (it times out on a real diff) but stay READY; a fast reviewer must not. +printf 'lanes:\n - feature\nmodels:\n developer: ""\n reviewer: "zai:glm@5.2"\n test_author: "moonshotai:kimi@k2.6"\n test_runner: "moonshotai:kimi@k2.6"\n' > "$TMP/glm.yml" +out="$(python3 "$DOC" "$TMP/glm.yml" 2>&1)" +grep -qi 'reasoning model' <<<"$out" && grep -q 'RESULT: READY' <<<"$out" \ + && ok "reasoning reviewer WARNs but stays READY" || bad "reasoning-reviewer WARN missing" +printf 'lanes:\n - feature\nmodels:\n developer: ""\n reviewer: "openai:gpt-oss-120b"\n test_author: "moonshotai:kimi@k2.6"\n test_runner: "moonshotai:kimi@k2.6"\n' > "$TMP/fast.yml" +out="$(python3 "$DOC" "$TMP/fast.yml" 2>&1)" +grep -qi 'reasoning model' <<<"$out" && bad "fast reviewer wrongly warned" \ + || ok "fast reviewer does not trigger the reasoning warn" + [ "$fail" = "0" ] && { echo "doctor self-test: PASS"; exit 0; } || { echo "doctor self-test: FAIL"; exit 1; } diff --git a/cli/setup-goose.py b/cli/setup-goose.py index eb13ea0..eea4155 100755 --- a/cli/setup-goose.py +++ b/cli/setup-goose.py @@ -374,6 +374,17 @@ def main(): print_steps(roles, recipe_dir) + # WARN if the reviewer got a heavy reasoning model: it can exceed a hosted inference window on a real + # code diff and time out with no lenses, while trivial diffs pass. Canonical check: cli/doctor.py + # check_reviewer_reasoning. Advisory; setup still succeeds. + reasoning_hints = ("glm", "deepseek-r1", "deepseek_r1", "deepseek-reasoner", "reasoner", "qwq", + "-thinking", "thinking-", "magistral", "o1-", "o1@", "o3-", "o3@", "o4-", "minimax-m1") + rev = next((r["value"] for r in roles if r["key"] == "reviewer"), "") + if rev and any(h in rev.lower() for h in reasoning_hints): + print(f"\nWARNING: models.reviewer ({rev}) looks like a reasoning model. A reasoning model can " + "exceed a hosted inference window on a real code diff and time out with no lenses, while " + "trivial diffs still pass. Prefer a faster non-reasoning reviewer.") + rc = 0 if not no_validate: code, out = run_check(config, strict=changed) From 6c46372735953cba2634096c6c1dbb7b6b8c0e02 Mon Sep 17 00:00:00 2001 From: Christoph Date: Fri, 24 Jul 2026 17:29:59 +0200 Subject: [PATCH 3/3] feat(review): fail fast on a slow model with a configurable timeout The model call had a hardcoded 180s timeout, so a reasoning reviewer that hangs burned the full server timeout on every retry before failing closed. Add a configurable per-call timeout (review.timeout_seconds / ASDD_MODEL_TIMEOUT, default 45s) above a fast reviewer's real-diff time and below a reasoning model's server-side hang, and on a timeout emit an actionable message naming the cause. run-review maps review.timeout_seconds to the adapter. Signed-off-by: Christoph --- .asdd.example.yml | 3 +++ .github/asdd/run-review.sh | 6 ++++++ .github/asdd/runtime/openai-compat.sh | 18 ++++++++++++++---- .github/asdd/runtime/openai-compat.test.sh | 7 +++++++ CHANGELOG.md | 4 ++++ 5 files changed, 34 insertions(+), 4 deletions(-) diff --git a/.asdd.example.yml b/.asdd.example.yml index 09eba7e..28ee6df 100644 --- a/.asdd.example.yml +++ b/.asdd.example.yml @@ -117,6 +117,9 @@ agents: [] # the cap (only sensible on a private repo). review: max_diff_lines: 2000 + # Per-call model timeout (seconds). A slow reasoning reviewer times out around 60s on a real diff; the + # default fails fast below that with an actionable message rather than hanging. Re-tune per provider. + timeout_seconds: 45 # Spec tool: which validator decides a change's spec is READY (the operate agents' readiness gate). # builtin -> ASDD's definition-of-ready (cli/spec-check.py): outcomes/scope/constraints/verification diff --git a/.github/asdd/run-review.sh b/.github/asdd/run-review.sh index 09d7907..f49c393 100755 --- a/.github/asdd/run-review.sh +++ b/.github/asdd/run-review.sh @@ -69,6 +69,12 @@ fi # enormous PR drains the budget. Above the cap we refuse the model and say so, rather than pay. # The deterministic security layer below still runs, so the gate keeps biting either way. CAP="$(yaml_scalar_in max_diff_lines)"; CAP="${CAP:-2000}" +# Per-call model timeout (seconds). review.timeout_seconds in .asdd.yml maps to ASDD_MODEL_TIMEOUT, which +# the OpenAI-compatible adapter enforces, so a slow reasoning reviewer fails fast with a named cause +# instead of hanging. Unset here leaves the adapter default. A caller's env ASDD_MODEL_TIMEOUT still wins. +if [ -z "${ASDD_MODEL_TIMEOUT:-}" ]; then + _to="$(yaml_scalar_in timeout_seconds)"; [ -n "$_to" ] && export ASDD_MODEL_TIMEOUT="$_to" +fi DIFF_LINES=$(wc -l < "$WORKDIR/changes.diff" 2>/dev/null | tr -d ' '); DIFF_LINES="${DIFF_LINES:-0}" too_big=false if [ "$CAP" -gt 0 ] 2>/dev/null && [ "$DIFF_LINES" -gt "$CAP" ]; then too_big=true; fi diff --git a/.github/asdd/runtime/openai-compat.sh b/.github/asdd/runtime/openai-compat.sh index 99a1a24..22b1d1a 100755 --- a/.github/asdd/runtime/openai-compat.sh +++ b/.github/asdd/runtime/openai-compat.sh @@ -83,13 +83,22 @@ case "$attempts" in ''|*[!0-9]*) attempts=3 ;; esac [ "$attempts" -ge 1 ] || attempts=1 nap="${ASDD_MODEL_RETRY_SLEEP:-1}" -result=""; with_rf=1 +# Per-call timeout. Default 45s: above a fast reviewer's full-review time (~24s measured) and below a heavy +# reasoning model's server-side hang (a reasoning reviewer times out around 60s on a real diff), so a slow +# reasoning reviewer fails fast with a named cause instead of burning the full server timeout on every +# retry. Configurable via ASDD_MODEL_TIMEOUT (run-review.sh maps review.timeout_seconds to it); re-tune per +# provider. +timeout_s="${ASDD_MODEL_TIMEOUT:-45}" +case "$timeout_s" in ''|*[!0-9]*) timeout_s=45 ;; esac + +result=""; with_rf=1; timed_out=0 for i in $(seq 1 "$attempts"); do - http="" - resp="$(curl -sS --max-time 180 -w '\n%{http_code}' -X POST "$endpoint" \ + http=""; curl_rc=0 + resp="$(curl -sS --max-time "$timeout_s" -w '\n%{http_code}' -X POST "$endpoint" \ -H "Authorization: Bearer $ASDD_RUNTIME_TOKEN" \ -H "Content-Type: application/json" \ - -d "$(build_payload "$with_rf")" || true)" + -d "$(build_payload "$with_rf")")" || curl_rc=$? + [ "$curl_rc" = 28 ] && timed_out=1 # 28 = curl's own timeout exit code http="${resp##*$'\n'}"; resp="${resp%$'\n'*}" # split the trailing status line off the body # Pull the answer across the shapes providers actually use: content as a string, content as an array of # parts (concatenate .text), or a legacy .text field; THEN append message.reasoning_content. A reasoning @@ -137,6 +146,7 @@ if [ -z "$result" ]; then # PR content, so a truncated echo is key-safe and turns a blind failure into a named one. [ "$clen" -eq 0 ] && echo " response body (truncated): $(printf '%s' "$resp" | tr '\n' ' ' | cut -c1-240)" [ "$clen" -eq 0 ] && echo " (content was empty across message.content, content parts, .text, and reasoning_content: check the endpoint returns OpenAI-shaped choices, the model name, and quota.)" + [ "$timed_out" = 1 ] && echo " the model did not respond within ${timeout_s}s; a heavy reasoning model can be too slow to review a real diff (see 'asdd doctor'). Use a faster reviewer, or raise ASDD_MODEL_TIMEOUT / review.timeout_seconds if the model is legitimately slow." } >&2 fi printf '%s' "$result" diff --git a/.github/asdd/runtime/openai-compat.test.sh b/.github/asdd/runtime/openai-compat.test.sh index 3fe311a..0339848 100755 --- a/.github/asdd/runtime/openai-compat.test.sh +++ b/.github/asdd/runtime/openai-compat.test.sh @@ -26,6 +26,7 @@ case "${MODE:-url}" in flaky) # garbage on the first call, a valid object afterwards (proves retry) n=$(cat "$COUNTER" 2>/dev/null || echo 0); n=$((n+1)); echo "$n" > "$COUNTER" if [ "$n" -le 1 ]; then envelope 'nope'; else envelope '{"schema":"asdd/review/v0.1","lenses":[]}'; fi ;; + timeout) exit 28 ;; # simulate curl's --max-time timeout rf) # a provider that rejects response_format: the call carrying it yields no usable content, the # retry without it succeeds (proves the response_format fallback) has_rf=0; for a in "$@"; do case "$a" in *response_format*) has_rf=1;; esac; done @@ -80,4 +81,10 @@ got="$(MODE=rf ASDD_MODEL_RETRIES=3 PATH="$STUB:$PATH" ASDD_RUNTIME_TOKEN=x ASDD [ "$(printf '%s' "$got" | jq -r .schema)" = "asdd/review/v0.1" ] || fail "response_format fallback did not recover a review (got '$got')" grep -q 'retrying without response_format' "$STUB/err" || fail "fallback did not drop response_format on the retry" +# 7. per-call timeout (curl exit 28): empty output + a named, actionable timeout message (not a blind fail). +got="$(MODE=timeout ASDD_MODEL_RETRIES=2 ASDD_MODEL_TIMEOUT=1 PATH="$STUB:$PATH" ASDD_RUNTIME_TOKEN=x ASDD_MODEL=m \ + ASDD_MODEL_RETRY_SLEEP=0 ASDD_MODEL_URL='https://x.test/v1/chat/completions' bash "$SCRIPT" "$STUB/err")" +[ -z "$got" ] || fail "a timeout should yield empty output, got '$got'" +grep -q 'did not respond within' "$STUB/err" || fail "timeout did not produce the actionable message" + echo "openai-compat.test.sh: PASS" diff --git a/CHANGELOG.md b/CHANGELOG.md index 54284a2..f2a92ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ draft, so pin a conformance claim to a commit or date. length and can exceed a hosted inference window on a real code diff, so the review times out and posts no lenses while trivial or docs-only diffs still pass and look fine. The preflight and the setup wizard now flag a reasoning reviewer (a property of the model, not the host) and point to a faster one. +- **A configurable per-call review timeout that fails fast.** The model call now has a default 45s timeout + (`review.timeout_seconds` / `ASDD_MODEL_TIMEOUT`), above a fast reviewer's real-diff time and below a + reasoning model's server-side hang, so a slow reviewer fails fast with an actionable message naming the + cause instead of burning the full server timeout on every retry. ### Fixed - **Review runtime recovers the model's JSON.** A reasoning model wraps its review object in analysis