diff --git a/.changeset/2073-antigravity-reviewer-block.md b/.changeset/2073-antigravity-reviewer-block.md new file mode 100644 index 000000000..009c6cd5e --- /dev/null +++ b/.changeset/2073-antigravity-reviewer-block.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2109 +--- +**`/gsd-review`'s Antigravity CLI reviewer no longer fails silently on large prompts, unavailable pinned models, or pre-session stalls** — the `agy` invocation now uses a file-reference prompt to avoid exec arg-list overflow, is wrapped in an external wall-clock `timeout` paired with `--print-timeout` because `--print-timeout` cannot fire before `agy` creates a session, passes `--model` from `review.models.agy` when set as an escape hatch for a 404'd pinned model, and its empty-output stub now surfaces an `agy` cli.log diagnostic instead of a bare generic message. Supersedes the #687 "no external killer / inline `$(cat)`" contract, which predated `agy` gaining `--model` and predated its own guidance to pair `--print-timeout` with a terminal timeout. (#2073) diff --git a/gsd-core/workflows/review.md b/gsd-core/workflows/review.md index 9b5a2eb9a..4e9543c4c 100644 --- a/gsd-core/workflows/review.md +++ b/gsd-core/workflows/review.md @@ -234,7 +234,8 @@ GEMINI_MODEL=$(gsd_run query config-get review.models.gemini 2>/dev/null | jq -r CLAUDE_MODEL=$(gsd_run query config-get review.models.claude 2>/dev/null | jq -r '.' 2>/dev/null || true) CODEX_MODEL=$(gsd_run query config-get review.models.codex 2>/dev/null | jq -r '.' 2>/dev/null || true) OPENCODE_MODEL=$(gsd_run query config-get review.models.opencode 2>/dev/null | jq -r '.' 2>/dev/null || true) -# review.models.agy is reserved for future model-pinning support; agy selects its model internally +# review.models.agy, when set, is passed to agy as --model (escape hatch for a +# pinned model that 404s server-side); otherwise agy uses its persisted default. AGY_MODEL=$(gsd_run query config-get review.models.agy 2>/dev/null | jq -r '.' 2>/dev/null || true) # #1115: `--dangerously-bypass-hook-trust` only exists on codex-cli >= 0.137.0. @@ -373,7 +374,7 @@ fi **Antigravity CLI:** -**Maintainer note — why this block has three layers (last updated against agy 1.0.2):** +**Maintainer note — why this block has three layers (last updated against agy 1.0.16):** `agy -p` (the `--print` non-interactive flag) works correctly on macOS and Linux: it sends the prompt, receives the model response, and writes it to stdout. On **native Windows** it silently @@ -408,7 +409,9 @@ and Step 3 fires with a clear error message in REVIEWS.md. No silent corruption. Invocation specifics (verified agy 1.0.0, macOS arm64 and Linux amd64): - `-p` takes the prompt as a **flag value** — `echo X | agy -p` errors with "flag needs an argument: -p" - `--print-timeout` defaults to 5m, aligning with this workflow's global timeout -- No `-m` / `--model` flag — agy selects the model internally +- `--model ""` selects the model (available since agy ~1.0.3; `agy models` lists + them). When `review.models.agy` is set it is passed as `--model`; otherwise agy uses + its persisted default (`agy models`). ```bash # Pre-flight: snapshot the transcript watermark before invoking agy. @@ -432,14 +435,41 @@ if [ -f "$_AGY_CACHE" ]; then fi # Step 1 — primary invocation: stdout works on macOS, Linux, and WSL. -# Bound the run with agy's OWN `--print-timeout` (issue #687). On a large, -# file-path-rich prompt agy's agentic Cascade can loop on its code_search/grep -# steps and never converge; `--print-timeout` is agy's native cap for print mode -# (defaults to 5m — see maintainer note above), so we pass it explicitly to let a -# stalled run self-terminate through the tool's own mechanism. A non-zero exit -# (timeout or crash) discards any partial output so the Step 2 transcript fallback -# / Step 3 stub take over. -agy --print-timeout 300s -p "$(cat /tmp/gsd-review-prompt-{phase}.md)" 2>/dev/null > /tmp/gsd-review-antigravity-{phase}.md +# Three hardening invariants (#2073), all mirroring the Cursor block's discipline: +# * FILE-REFERENCE prompt (not inline `$(cat …)`) — a large review prompt (≈197 KB +# for 6 plans + CONTEXT + RESEARCH + REQUIREMENTS) overflows the exec arg list +# (`bash: agy: Argument list too long`, rc 126), indistinguishable from a model +# failure when stderr is suppressed. +# * EXTERNAL `timeout` wrapper when available (GNU `timeout` / `gtimeout`) — +# `--print-timeout` is agy's native cap but it CANNOT fire before agy creates a +# session; under concurrent heavy runs one process can stall pre-session (no +# `brain//` dir, alive at 583 s despite `--print-timeout 300s`). The +# external cap bounds wall-clock regardless. Stock macOS lacks `timeout`, so +# the block probes for it and falls back to --print-timeout alone there. +# * `--model` from `review.models.agy` when set — escape hatch for a pinned model +# that 404s server-side (exits 0 with empty stdout AND empty transcript). +# * stdin tied to /dev/null so agy never blocks on a tty. +# A non-zero exit (external timeout = 124, crash, etc.) discards any partial output +# so the Step 2 transcript fallback / Step 3 diagnostic take over. +if [ -n "$AGY_MODEL" ] && [ "$AGY_MODEL" != "null" ]; then + set -- --model "$AGY_MODEL" +else + set -- +fi +_AGY_PROMPT="Read the file at /tmp/gsd-review-prompt-{phase}.md in full and carry out the review request it contains. Output only the resulting markdown review. Do not edit any files." +# Capability-probe an external wall-clock killer (GNU coreutils `timeout` or the +# macOS Homebrew `gtimeout`). Stock macOS ships NEITHER — a bare `timeout …` would +# fail with rc 127 ("command not found") and silently lose the reviewer, so fall +# back to agy's native --print-timeout alone in that case. The external cap, when +# available, is set HIGHER than --print-timeout so it only backstops a pre-session +# stall (which --print-timeout cannot bound — #2073 mode 3) and never pre-empts a +# healthy run. Mirrors the probe in scripts/base64-scan.sh. +_AGY_KILLER="$(command -v timeout 2>/dev/null || command -v gtimeout 2>/dev/null || true)" +if [ -n "$_AGY_KILLER" ]; then + "$_AGY_KILLER" 600 agy --print-timeout 540s "$@" -p "$_AGY_PROMPT" /dev/null > /tmp/gsd-review-antigravity-{phase}.md +else + agy --print-timeout 540s "$@" -p "$_AGY_PROMPT" /dev/null > /tmp/gsd-review-antigravity-{phase}.md +fi _AGY_RC=$? if [ "$_AGY_RC" -ne 0 ]; then : > /tmp/gsd-review-antigravity-{phase}.md @@ -473,9 +503,25 @@ if [ ! -s /tmp/gsd-review-antigravity-{phase}.md ]; then fi fi -# Step 3 — final guard: both approaches yielded nothing (auth error, first-run setup, path schema changed, etc.) +# Step 3 — final guard: both approaches yielded nothing (auth error, first-run setup, +# path schema changed, 404'd pinned model, pre-session stall, etc.) if [ ! -s /tmp/gsd-review-antigravity-{phase}.md ]; then - echo "Antigravity review failed or returned empty output." > /tmp/gsd-review-antigravity-{phase}.md + { + echo "Antigravity review failed or returned empty output." + # #2073 mode 2: a pinned model that 404s exits 0 with empty stdout AND an empty + # transcript — the only evidence is in agy's own log. Surface it instead of a + # bare generic stub so the failure is diagnosable. + _AGY_LOG="$HOME/.gemini/antigravity-cli/cli.log" + if [ -f "$_AGY_LOG" ]; then + _AGY_ERR=$(grep -iE 'agent executor error|NOT_FOUND|Publisher model' "$_AGY_LOG" | tail -3) + if [ -n "$_AGY_ERR" ]; then + echo "agy log hint (pinned model may be unavailable — run 'agy models' and set review.models.agy):" + echo "$_AGY_ERR" + fi + fi + # #2073 mode 3: pre-session stall tell — no new conversation dir appeared. + echo "If no agy run started, that is the pre-session-stall case: check whether a new ~/.gemini/antigravity-cli/brain// dir appeared within ~30s of launch." + } > /tmp/gsd-review-antigravity-{phase}.md fi ``` diff --git a/tests/antigravity-reviewer.test.cjs b/tests/antigravity-reviewer.test.cjs new file mode 100644 index 000000000..84d347cf5 --- /dev/null +++ b/tests/antigravity-reviewer.test.cjs @@ -0,0 +1,109 @@ +// allow-test-rule: source-text-is-the-product (see #2073) +// gsd-core/workflows/review.md is a workflow document whose bash blocks ARE +// what /gsd-review loads and executes at runtime. Asserting the Antigravity +// invocation shape asserts the deployed contract — this is behavioral coverage +// of the workflow, not a source-grep over application code. + +/** + * Antigravity (agy) reviewer invocation tests (#2073) + * + * The agy block in /gsd-review had three real-world failure modes on agy 1.0.16: + * 1. inline `-p "$(cat )"` overflowed the exec arg list on a large prompt + * 2. a pinned model that 404'd exited 0 with empty stdout AND an empty transcript + * (no --model escape hatch; the generic Step 3 stub gave no diagnostic) + * 3. a pre-session stall hung past --print-timeout (which can't fire before a + * session exists) because there was no external wall-clock `timeout` + * Plus a stale maintainer note claiming agy has no --model flag. + * + * These tests pin the corrected invocation shape in gsd-core/workflows/review.md. + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const ROOT = path.join(__dirname, '..'); +const REVIEW_PATH = path.join(ROOT, 'gsd-core', 'workflows', 'review.md'); + +function agyBashBlock() { + const content = fs.readFileSync(REVIEW_PATH, 'utf-8'); + const fences = content.match(/```bash[\s\S]*?```/g) || []; + // Target the INVOCATION block (the fence that writes the antigravity review + // output), not the `command -v agy` detection one-liner. + const agy = fences.find((f) => /\bagy\b/.test(f) && /gsd-review-antigravity/.test(f)); + assert.ok(agy, 'review.md should contain the agy invocation bash block'); + return agy; +} + +describe('Antigravity (agy) reviewer invocation in /gsd-review (#2073)', () => { + test('review.md exists', () => { + assert.ok(fs.existsSync(REVIEW_PATH), 'review.md should exist'); + }); + + test('#2073 mode 1 — does NOT inline the prompt via "$(cat ...)" (arg-list overflow)', () => { + const block = agyBashBlock(); + assert.ok( + !/"\$\(cat/.test(block), + 'agy must not inline the prompt via "$(cat …)" — a large review prompt overflows the exec arg list', + ); + }); + + test('#2073 mode 1 — uses a file-reference prompt (mirrors the Cursor block)', () => { + const block = agyBashBlock(); + assert.ok( + /Read the file at \/tmp\/gsd-review-prompt-/.test(block), + 'agy should reference the prompt by file path, not inline it', + ); + }); + + test('#2073 mode 3 — pairs agy with an external wall-clock killer when available (timeout/gtimeout probe)', () => { + const block = agyBashBlock(); + // Capability probe for GNU `timeout` and macOS `gtimeout` (stock macOS has neither). + assert.match(block, /command -v timeout/, 'agy block should probe for the `timeout` killer'); + assert.match(block, /command -v gtimeout/, 'agy block should probe for `gtimeout` (macOS Homebrew)'); + // The external cap (600s) is >= agy's native --print-timeout (540s) so it only + // backstops a pre-session stall, never cuts a healthy run. + assert.match(block, /600 agy --print-timeout 540s/, 'external cap (600s) must be >= --print-timeout (540s)'); + // Graceful fallback when no external killer is available (stock macOS). + assert.match(block, /else\n\s*agy --print-timeout 540s/, + 'agy block must fall back to --print-timeout alone when no external killer is available (macOS)'); + }); + + test('#2073 mode 3 — stdin is tied to /dev/null (no tty stall)', () => { + const block = agyBashBlock(); + assert.ok( + /<\/dev\/null/.test(block), + 'agy invocation should redirect stdin from /dev/null so it never blocks on a tty', + ); + }); + + test('#2073 mode 2 — wires review.models.agy via --model when configured', () => { + const block = agyBashBlock(); + assert.match(block, /--model "\$AGY_MODEL"/, 'agy block should pass --model "$AGY_MODEL" when set'); + }); + + test('#2073 mode 2 — Step 3 stub surfaces a diagnostic from agy cli.log (not just a generic stub)', () => { + const block = agyBashBlock(); + assert.ok( + /cli\.log/.test(block), + 'the empty-output stub should inspect agy cli.log for a model-availability diagnostic (NOT_FOUND / agent executor error)', + ); + }); + + test('#2073 — stale "no --model flag" maintainer note is corrected', () => { + const content = fs.readFileSync(REVIEW_PATH, 'utf-8'); + assert.ok( + !/No .{0,4}-m.{0,4}\/.{0,4}--model.{0,4} flag/i.test(content), + 'the stale maintainer note claiming agy has no --model flag must be corrected (--model exists since ~1.0.3)', + ); + }); + + test('#2073 — review.models.agy is documented as supported (not "reserved for future")', () => { + const content = fs.readFileSync(REVIEW_PATH, 'utf-8'); + assert.ok( + !/review\.models\.agy is reserved for future/i.test(content), + 'review.models.agy is now wired (passed as --model); the "reserved for future" comment must be updated', + ); + }); +}); diff --git a/tests/fixtures/golden-install-parity/antigravity.json b/tests/fixtures/golden-install-parity/antigravity.json index 6f495953e..0594525e6 100644 --- a/tests/fixtures/golden-install-parity/antigravity.json +++ b/tests/fixtures/golden-install-parity/antigravity.json @@ -284,7 +284,7 @@ "gsd-core/workflows/remove-phase.md": "23b9eb0858a2535e", "gsd-core/workflows/remove-workspace.md": "d0bd7e0601138798", "gsd-core/workflows/resume-project.md": "98e2cf8908e73a52", - "gsd-core/workflows/review.md": "9bc686206c96748c", + "gsd-core/workflows/review.md": "9ff117dd12ce68ba", "gsd-core/workflows/scan.md": "a7fecd67e5cd655f", "gsd-core/workflows/secure-phase.md": "96b199dfac00e60f", "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 6f7ae32a5..7e232323d 100644 --- a/tests/fixtures/golden-install-parity/augment.json +++ b/tests/fixtures/golden-install-parity/augment.json @@ -355,7 +355,7 @@ "gsd-core/workflows/remove-phase.md": "df9a45f0b1880999", "gsd-core/workflows/remove-workspace.md": "a7ca66db6b7c132c", "gsd-core/workflows/resume-project.md": "f28da1200e4545f4", - "gsd-core/workflows/review.md": "f3c2e894941c9943", + "gsd-core/workflows/review.md": "93fe46bd0b5dd3d1", "gsd-core/workflows/scan.md": "003883d71c37da7d", "gsd-core/workflows/secure-phase.md": "29fc6b62c5c5dc62", "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 95a1dfb72..28a4f3e27 100644 --- a/tests/fixtures/golden-install-parity/claude-local.json +++ b/tests/fixtures/golden-install-parity/claude-local.json @@ -354,7 +354,7 @@ "gsd-core/workflows/remove-phase.md": "8effc8742d58a11a", "gsd-core/workflows/remove-workspace.md": "10882656198d9075", "gsd-core/workflows/resume-project.md": "af9761bcec0f6fe9", - "gsd-core/workflows/review.md": "34cb7ab671eb8684", + "gsd-core/workflows/review.md": "f9b63577ff23c2a9", "gsd-core/workflows/scan.md": "75c670d08cee8680", "gsd-core/workflows/secure-phase.md": "64ec4d06ca85720a", "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 e2009eaa3..0b53e630a 100644 --- a/tests/fixtures/golden-install-parity/claude.json +++ b/tests/fixtures/golden-install-parity/claude.json @@ -283,7 +283,7 @@ "gsd-core/workflows/remove-phase.md": "ada8a0546c686483", "gsd-core/workflows/remove-workspace.md": "f3ab3a88a7e9e1ed", "gsd-core/workflows/resume-project.md": "7f8dc986f0f35d96", - "gsd-core/workflows/review.md": "5cd71e81c4ace401", + "gsd-core/workflows/review.md": "2dbfb1118613be25", "gsd-core/workflows/scan.md": "47371c2073d6c0be", "gsd-core/workflows/secure-phase.md": "59d3c50aba8c9a6c", "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 cc82e4925..1110eff27 100644 --- a/tests/fixtures/golden-install-parity/cline.json +++ b/tests/fixtures/golden-install-parity/cline.json @@ -287,7 +287,7 @@ "gsd-core/workflows/remove-phase.md": "e336350f8113a328", "gsd-core/workflows/remove-workspace.md": "e685dfbd736dfd90", "gsd-core/workflows/resume-project.md": "e23981178fa37b3d", - "gsd-core/workflows/review.md": "c2b3fcc5bfc80038", + "gsd-core/workflows/review.md": "86d03cb58f48f45b", "gsd-core/workflows/scan.md": "dfd92717caea0ce7", "gsd-core/workflows/secure-phase.md": "cf78183f06a02582", "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 c04aa1bf7..acc74ecd4 100644 --- a/tests/fixtures/golden-install-parity/codebuddy.json +++ b/tests/fixtures/golden-install-parity/codebuddy.json @@ -355,7 +355,7 @@ "gsd-core/workflows/remove-phase.md": "df9a45f0b1880999", "gsd-core/workflows/remove-workspace.md": "a7ca66db6b7c132c", "gsd-core/workflows/resume-project.md": "f28da1200e4545f4", - "gsd-core/workflows/review.md": "f3c2e894941c9943", + "gsd-core/workflows/review.md": "93fe46bd0b5dd3d1", "gsd-core/workflows/scan.md": "003883d71c37da7d", "gsd-core/workflows/secure-phase.md": "29fc6b62c5c5dc62", "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 efead535f..3e059529b 100644 --- a/tests/fixtures/golden-install-parity/codex.json +++ b/tests/fixtures/golden-install-parity/codex.json @@ -390,7 +390,7 @@ "gsd-core/workflows/remove-phase.md": "9ee0fddd11a0d9d4", "gsd-core/workflows/remove-workspace.md": "19d7465aaa50cb62", "gsd-core/workflows/resume-project.md": "9965f87eb278f7f8", - "gsd-core/workflows/review.md": "3a086e38f13eab6f", + "gsd-core/workflows/review.md": "63c61eddf20d77f1", "gsd-core/workflows/scan.md": "1a3caa5d724d39e9", "gsd-core/workflows/secure-phase.md": "db91810d16964b1e", "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 23cd30d93..64637655a 100644 --- a/tests/fixtures/golden-install-parity/copilot.json +++ b/tests/fixtures/golden-install-parity/copilot.json @@ -285,7 +285,7 @@ "gsd-core/workflows/remove-phase.md": "e262654e319d1bc4", "gsd-core/workflows/remove-workspace.md": "ceddfeef5f2d6754", "gsd-core/workflows/resume-project.md": "40db7f350f5866d8", - "gsd-core/workflows/review.md": "01b40d7ddd69dc8b", + "gsd-core/workflows/review.md": "5f146ed397bcfe5a", "gsd-core/workflows/scan.md": "dcc2f76d0850e2fb", "gsd-core/workflows/secure-phase.md": "d87bd706f85bcad6", "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 ef6658d4f..9ac2844fc 100644 --- a/tests/fixtures/golden-install-parity/cursor.json +++ b/tests/fixtures/golden-install-parity/cursor.json @@ -355,7 +355,7 @@ "gsd-core/workflows/remove-phase.md": "ada8a0546c686483", "gsd-core/workflows/remove-workspace.md": "433affcd1a200826", "gsd-core/workflows/resume-project.md": "7f8dc986f0f35d96", - "gsd-core/workflows/review.md": "338005f1da182bdc", + "gsd-core/workflows/review.md": "ed6b9f54f74204ff", "gsd-core/workflows/scan.md": "47371c2073d6c0be", "gsd-core/workflows/secure-phase.md": "c55975672c4e1895", "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 50d7c6a7a..80e728340 100644 --- a/tests/fixtures/golden-install-parity/hermes.json +++ b/tests/fixtures/golden-install-parity/hermes.json @@ -284,7 +284,7 @@ "gsd-core/workflows/remove-phase.md": "fce799aae3ab2715", "gsd-core/workflows/remove-workspace.md": "8facde381657dd71", "gsd-core/workflows/resume-project.md": "a0443839f1f83c2d", - "gsd-core/workflows/review.md": "f11ffe5b46b50565", + "gsd-core/workflows/review.md": "c9de3987db14b94f", "gsd-core/workflows/scan.md": "b28f65d88c522767", "gsd-core/workflows/secure-phase.md": "f2957d4b88fb3746", "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 21a1560ca..e2a461d1c 100644 --- a/tests/fixtures/golden-install-parity/kilo.json +++ b/tests/fixtures/golden-install-parity/kilo.json @@ -355,7 +355,7 @@ "gsd-core/workflows/remove-phase.md": "ada8a0546c686483", "gsd-core/workflows/remove-workspace.md": "fc83f362a2d0a1b7", "gsd-core/workflows/resume-project.md": "7f8dc986f0f35d96", - "gsd-core/workflows/review.md": "3af2d510da581502", + "gsd-core/workflows/review.md": "dcea2ffd4f54c845", "gsd-core/workflows/scan.md": "47371c2073d6c0be", "gsd-core/workflows/secure-phase.md": "e8855104c1e0417c", "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 81d6a4c32..d8e0287ca 100644 --- a/tests/fixtures/golden-install-parity/kimi.json +++ b/tests/fixtures/golden-install-parity/kimi.json @@ -320,7 +320,7 @@ "gsd-core/workflows/remove-phase.md": "df9a45f0b1880999", "gsd-core/workflows/remove-workspace.md": "a7ca66db6b7c132c", "gsd-core/workflows/resume-project.md": "f28da1200e4545f4", - "gsd-core/workflows/review.md": "f3c2e894941c9943", + "gsd-core/workflows/review.md": "93fe46bd0b5dd3d1", "gsd-core/workflows/scan.md": "003883d71c37da7d", "gsd-core/workflows/secure-phase.md": "29fc6b62c5c5dc62", "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 6fab45dd5..759b24f55 100644 --- a/tests/fixtures/golden-install-parity/opencode.json +++ b/tests/fixtures/golden-install-parity/opencode.json @@ -355,7 +355,7 @@ "gsd-core/workflows/remove-phase.md": "dea4661e8f89596f", "gsd-core/workflows/remove-workspace.md": "446847e71aa52504", "gsd-core/workflows/resume-project.md": "ad9f06a10bab8cc0", - "gsd-core/workflows/review.md": "94ff56aecbbe2753", + "gsd-core/workflows/review.md": "a039f632e0b89003", "gsd-core/workflows/scan.md": "ad8ebcad4626d4a8", "gsd-core/workflows/secure-phase.md": "e9a488cec3b4efdc", "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 f2feab0c5..b1536043b 100644 --- a/tests/fixtures/golden-install-parity/qwen.json +++ b/tests/fixtures/golden-install-parity/qwen.json @@ -284,7 +284,7 @@ "gsd-core/workflows/remove-phase.md": "e8ae4fbbfac700f0", "gsd-core/workflows/remove-workspace.md": "4ac64de862dc650e", "gsd-core/workflows/resume-project.md": "7f20769f302e5427", - "gsd-core/workflows/review.md": "bb5c36262d5ff951", + "gsd-core/workflows/review.md": "aa4674a4754438f2", "gsd-core/workflows/scan.md": "949692db4834dd27", "gsd-core/workflows/secure-phase.md": "6758f1acf4113e9e", "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 00087a04b..1aa5e03d0 100644 --- a/tests/fixtures/golden-install-parity/trae.json +++ b/tests/fixtures/golden-install-parity/trae.json @@ -284,7 +284,7 @@ "gsd-core/workflows/remove-phase.md": "a46c2fe853bf4e86", "gsd-core/workflows/remove-workspace.md": "ae0e1c6d4438d663", "gsd-core/workflows/resume-project.md": "f242e4c8aba18ea2", - "gsd-core/workflows/review.md": "34a23b9b65fb55f6", + "gsd-core/workflows/review.md": "51eb79b72a09d47a", "gsd-core/workflows/scan.md": "63631467651d9ca8", "gsd-core/workflows/secure-phase.md": "6cc236e53c2e7d56", "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 a4a2eba24..e1f88551e 100644 --- a/tests/fixtures/golden-install-parity/windsurf.json +++ b/tests/fixtures/golden-install-parity/windsurf.json @@ -284,7 +284,7 @@ "gsd-core/workflows/remove-phase.md": "e7a6af429b36e77b", "gsd-core/workflows/remove-workspace.md": "b5e60fbb33b3e33a", "gsd-core/workflows/resume-project.md": "82cfe1b8cb17c085", - "gsd-core/workflows/review.md": "0873ea94ca23383c", + "gsd-core/workflows/review.md": "31b1f08d08b8ccdc", "gsd-core/workflows/scan.md": "12c11b2edc165df9", "gsd-core/workflows/secure-phase.md": "7bf923689bf58288", "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 3d05f6e97..d291c636b 100644 --- a/tests/fixtures/golden-install-parity/zcode.json +++ b/tests/fixtures/golden-install-parity/zcode.json @@ -355,7 +355,7 @@ "gsd-core/workflows/remove-phase.md": "df9a45f0b1880999", "gsd-core/workflows/remove-workspace.md": "a7ca66db6b7c132c", "gsd-core/workflows/resume-project.md": "f28da1200e4545f4", - "gsd-core/workflows/review.md": "f3c2e894941c9943", + "gsd-core/workflows/review.md": "93fe46bd0b5dd3d1", "gsd-core/workflows/scan.md": "003883d71c37da7d", "gsd-core/workflows/secure-phase.md": "29fc6b62c5c5dc62", "gsd-core/workflows/session-report.md": "2e5b1205324ddefa", diff --git a/tests/review-default-reviewers-workflow.test.cjs b/tests/review-default-reviewers-workflow.test.cjs index 4e29db3ac..6104a8f59 100644 --- a/tests/review-default-reviewers-workflow.test.cjs +++ b/tests/review-default-reviewers-workflow.test.cjs @@ -171,36 +171,71 @@ const path = require('node:path'); const reviewPath = path.resolve(__dirname, '..', 'gsd-core', 'workflows', 'review.md'); const read = () => fs.readFileSync(reviewPath, 'utf-8'); -describe('bug #687: agy print mode must be bounded via its native --print-timeout', () => { - test('invokes agy with its own --print-timeout flag (not an external killer)', () => { - assert.match(read(), /agy --print-timeout \d+s? -p "\$\(cat/, - 'review.md must cap agy through `agy --print-timeout -p …` (the tool\'s own mechanism)'); +describe('bug #687 → #2073: agy print mode bounded by --print-timeout PAIRED with an external timeout', () => { + // #687 established that agy print mode must be bounded (its native + // --print-timeout, default 5m). #2073 superseded the "no external killer" + // half of that contract with documentation: + // - agy's own print-mode guidance says to PAIR --print-timeout with an + // external terminal `timeout` ("Pair with the terminal timeout= so the + // outer call doesn't cut the run short"), because --print-timeout cannot + // fire before agy creates a session (a pre-session stall otherwise hangs + // unbounded). The external cap is set HIGHER than --print-timeout so it + // only backstops a stall, never cuts a healthy run. + // - agy gained `--model` in ~1.0.3 (issue #3782's "no --model flag" note + // was correct at the time, stale now); review.models.agy is passed as + // --model so a pinned model that 404s has an escape hatch. + // - the prompt is now a file reference: inline `-p "$(cat …)"` overflows + // the exec arg list on a large review prompt (Linux MAX_ARG_STRLEN + // 128 KB/single-arg → rc 126). + + test('invokes agy with --print-timeout AND a paired external killer when available', () => { + const c = read(); + assert.match(c, /--print-timeout \d+s?/, 'review.md must pass agy its native --print-timeout'); + // Capability probe for GNU `timeout` / macOS `gtimeout` (stock macOS has neither). + assert.match(c, /command -v timeout/, 'review.md must probe for the `timeout` killer'); + assert.match(c, /command -v gtimeout/, 'review.md must probe for `gtimeout` (macOS Homebrew)'); + // The external cap (600s) is applied ahead of agy and is >= --print-timeout (540s). + assert.match(c, /600 agy --print-timeout 540s/, + 'review.md must pair an external cap (600s) >= --print-timeout (540s) with agy (agy guidance)'); }); - test('discards partial output on non-zero exit so the fallback fires', () => { + test('external cap is >= --print-timeout, and falls back to bare agy on macOS', () => { + const c = read(); + const bound = c.match(/(\d+)\s+agy --print-timeout (\d+)s/); + assert.ok(bound, 'review.md must encode the external-cap + --print-timeout pair'); + assert.ok( + Number(bound[1]) >= Number(bound[2]), + 'external cap (seconds) must be >= --print-timeout (seconds) so it only backstops a stall', + ); + // Graceful fallback when no external killer is available (stock macOS). + assert.match(c, /else\n\s*agy --print-timeout/, + 'review.md must fall back to --print-timeout alone when no external killer is available (macOS)'); + }); + + test('uses a file-reference prompt, not inline "$(cat …)" (arg-list overflow, #2073)', () => { + const c = read(); + assert.doesNotMatch(c, /agy[^\n]*-p "\$\(cat/, + 'review.md must not feed agy the prompt inline via "$(cat …)" — a large review prompt overflows the exec arg list (rc 126)'); + assert.match(c, /Read the file at \/tmp\/gsd-review-prompt-/, + 'review.md should pass agy a file-reference prompt (mirrors the Cursor block)'); + }); + + test('wires --model from review.models.agy (#2073 mode 2; agy gained --model in ~1.0.3)', () => { + assert.match(read(), /--model "\$AGY_MODEL"/, + 'review.md must pass --model "$AGY_MODEL" when review.models.agy is set'); + }); + + test('discards partial output on non-zero exit so the fallback fires (#687)', () => { const c = read(); assert.match(c, /_AGY_RC.*-ne 0/, 'review.md must check the agy exit code'); assert.match(c, /: > \/tmp\/gsd-review-antigravity-/, 'review.md must truncate the output file when agy timed out / failed'); }); - test('agy is bounded only by its own --print-timeout, not an external process killer', () => { - const c = read(); - // Print-mode reviewers invoke the tool directly; agy must self-terminate via - // --print-timeout, never via an external SIGKILL/timeout binary wrapped around it. - assert.doesNotMatch(c, /-s KILL/, 'must not SIGKILL agy from the outside'); - // Any external timeout binary wrapping agy — `timeout 300s agy …`, - // `gtimeout 300 agy …`, `timeout -s KILL 300 agy …`. The lookbehind keeps - // agy's own `--print-timeout` flag from tripping it. - assert.doesNotMatch(c, /(? { - // A bare `agy -p "$(cat …)"` with no cap was the original hang. - assert.doesNotMatch(read(), /^agy -p "\$\(cat/m, - 'review.md must not invoke agy -p without --print-timeout'); + // A bare `agy -p …` with no cap was the original #687 hang. + assert.doesNotMatch(read(), /^agy -p/m, + 'review.md must not invoke a bare `agy -p` unbounded at line start'); }); }); }); diff --git a/tests/workflow-size-baseline.json b/tests/workflow-size-baseline.json index 2f8dfb639..732a96b91 100644 --- a/tests/workflow-size-baseline.json +++ b/tests/workflow-size-baseline.json @@ -64,7 +64,7 @@ "remove-phase.md": 8513, "remove-workspace.md": 7551, "resume-project.md": 17270, - "review.md": 43356, + "review.md": 46297, "scan.md": 7732, "secure-phase.md": 13520, "session-report.md": 4044,