From 9aba7d9bd78d0197117c612baf67bfcc51acd2d1 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 3 Oct 2026 20:46:53 +0000 Subject: [PATCH] INV-1: gate outcome contract for outside reviews classifyOutsideReview() returns separate execution, findings and verdict fields from text, stderr and exit status; reason codes, anchors and fix actions live in lib/gate-outcomes.ts. validateOutsideReview() stays as the text-only wrapper and the two-argument CLI keeps exit 0/1/2 for importers and installed skills that predate a re-render. The --verdict form (implied by --stderr/--exit) prints VERDICT/FINDINGS/REASON and exits 0/3/4/1; the generated outside-voice caller moves to it and branches on each status (unverified prints OUTSIDE_STATUS: unverified, never completed). Adds docs/troubleshooting.md with one anchored section per reason code, the anchor test, and the gate-tail lint (query scripts may not swallow failures, gate calls may not end in || true, other best-effort tails say why). --- bin/gstack-codex-probe | 2 +- docs/troubleshooting.md | 191 +++++++++++++++++++ lib/gate-outcomes.ts | 92 +++++++++ lib/outside-review-result.ts | 170 ++++++++++++++--- scripts/resolvers/outside-voice.ts | 15 +- test/autoplan-dual-voice-evidence.test.ts | 2 + test/gate-tail-lint.test.ts | 71 +++++++ test/helpers/autoplan-dual-voice-evidence.ts | 5 +- test/outside-review-result.test.ts | 128 +++++++++++++ test/outside-voice-invocation.test.ts | 15 ++ test/troubleshooting-anchors.test.ts | 36 ++++ 11 files changed, 686 insertions(+), 41 deletions(-) create mode 100644 docs/troubleshooting.md create mode 100644 lib/gate-outcomes.ts create mode 100644 test/gate-tail-lint.test.ts create mode 100644 test/outside-review-result.test.ts create mode 100644 test/troubleshooting-anchors.test.ts diff --git a/bin/gstack-codex-probe b/bin/gstack-codex-probe index d840c20c3..8fa33ea0e 100755 --- a/bin/gstack-codex-probe +++ b/bin/gstack-codex-probe @@ -260,7 +260,7 @@ _gstack_codex_timeout_wrapper() { "$@" & local _cmd_pid=$! ( - sleep "$_duration" && trap '' TERM && kill -TERM "$_cmd_pid" 2>/dev/null || exit 0 + sleep "$_duration" && trap '' TERM && kill -TERM "$_cmd_pid" 2>/dev/null || exit 0 # best-effort: the command already exited, nothing to stop _GSTACK_CODEX_WAITED=0 while [ "$_GSTACK_CODEX_WAITED" -lt "$_grace" ] && kill -0 "$_cmd_pid" 2>/dev/null; do sleep 1 diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md new file mode 100644 index 000000000..a96f9fdf8 --- /dev/null +++ b/docs/troubleshooting.md @@ -0,0 +1,191 @@ +# Troubleshooting gstack messages + +Find the message you saw, then follow its fix. Every check gstack runs ends in +one of three states: + +- **ran**: the check executed. It may still have found problems. +- **not run**: gstack chose not to run it (you turned it off, or nothing applied). +- **unavailable**: gstack tried and could not run it. This is missing coverage, + never a pass. + +A message that is not `ran` has this shape: + +``` +: not run (). Fix: + unavailable: (). No review ran; this is missing coverage, not a pass. Fix: +: ran, verdict unverified (). Fix: read the output above +``` + +Search this page for the words after `unavailable:` or `not run (`. Each +section has a stable link anchor; the reason codes and anchors come from +`lib/gate-outcomes.ts`, and a free test fails if a code has no section here. + +--- + +## Outside reviews (Codex and Claude Code) + +Outside reviews send your diff, plan or question to a second AI provider. Their +verdict comes from `lib/outside-review-result.ts`, which prints +`VERDICT: clean|findings|unverified|unavailable`. A `findings` verdict with a +P0 or P1 finding blocks exactly like a native P0/P1. `unverified` and +`unavailable` are missing coverage: /ship and /review continue, show the gap in +the readiness dashboard and the PR body, and never count it as a pass. + + +### `Codex outside review unavailable: Codex's sandbox could not start here (...)` + +**Meaning.** Codex runs every command inside a Linux sandbox (bubblewrap). In +many containers and devcontainers the kernel does not allow unprivileged user +namespaces, so the sandbox cannot start and every command Codex tries fails. +gstack reports this instead of trusting a review that read nothing. +`CODEX_MODE: sandbox_unavailable` is the same condition found by the free +preflight before any paid call. + +**What is kept.** Nothing was sent for review, or the review was discarded. Your +code and files are unchanged. + +**Fix.** Enable unprivileged user namespaces for the container (for Docker, a +seccomp profile that allows them), or, inside a container you trust, run +Codex without its sandbox for this shell: + +```bash +export GSTACK_CODEX_NO_SANDBOX=1 +``` + +**Expected result.** With namespaces enabled, the next review prints +`OUTSIDE_STATUS: completed`. With `GSTACK_CODEX_NO_SANDBOX=1`, every review +prints `WARNING: GSTACK_CODEX_NO_SANDBOX=1: ...` because Codex can then read +and write anything your user can. Only the exact value `1` works, and only from +your shell environment. + + +### `... outside review unavailable: the reviewer could not run commands or read the diff (...)` + +**Meaning.** The reviewer tried to run commands but none succeeded, or its +answer says it could not run commands or read the diff. A "no issues found" +written after that is not a review. + +**What is kept.** Nothing; the answer is shown above but not counted. + +**Fix.** Read the reviewer's stderr above, repair what it names, and re-run. + +**Expected result.** `OUTSIDE_STATUS: completed` with a `VERDICT:` line. + + +### `... outside review unavailable: the reviewer process failed (exit N: ...)` + +**Meaning.** The provider CLI exited non-zero. The first stderr line is in the +parentheses. + +**What is kept.** Partial output is shown above; it is not counted. + +**Fix.** Repair the cause in the provider diagnosis (log in again, fix the +model, check the network), then re-run. + +**Expected result.** `OUTSIDE_STATUS: completed`. + + +### `... outside review unavailable: the reviewer hit its time limit and was stopped (exit 124)` + +**Meaning.** The provider did not finish within its deadline and was stopped. + +**What is kept.** Partial output is shown above; it is not counted. + +**Fix.** Re-run with a smaller scope, or check the provider's status and your +network. + +**Expected result.** `OUTSIDE_STATUS: completed`. + + +### `... outside review unavailable: the reviewer returned no response` + +**Meaning.** The provider exited successfully but returned nothing. + +**Fix.** Read the stderr above, then re-run. + +**Expected result.** `OUTSIDE_STATUS: completed`. + + +### `... outside review unavailable: the reviewer declined to review` + +**Meaning.** The provider answered with a refusal instead of a review. + +**Fix.** Re-run; if it declines again, rely on the native review. + +**Expected result.** `OUTSIDE_STATUS: completed`. + + +### `... outside review unavailable: the response lacks the markers this gate requires (...)` + +**Meaning.** The answer did not contain what the gate checks: a +`Recommendation: ... because ...` line, a `SCORE:`/`AMBIGUITIES:` pair, or +severity tags. + +**Fix.** Re-run the review. + +**Expected result.** `OUTSIDE_STATUS: completed`. + + +### `... outside review: ran, verdict unverified (...)` / `OUTSIDE_STATUS: unverified` / `GATE: UNVERIFIED` + +**Meaning.** The review completed but tagged nothing and gave no explicit +no-findings conclusion, so no pass or fail can be read from it. + +**What is kept.** The full answer is shown above. + +**Fix.** Read the output above and decide. It is not a pass, and /ship and +/review list it as missing coverage. + + +### `CODEX_MODE: model_unusable` / `MODEL_UNUSABLE` + +**Meaning.** Codex rejected the selected model: the account cannot use it +(HTTP 400), the model is retired, or a custom provider's `base_url` is wrong +(HTTP 404). The `CODEX_MODEL:` line names the model and where it came from. + +**Fix.** Choose a model your account can use, or correct the provider: + +```bash +export GSTACK_CODEX_MODEL= +# or edit model / base_url in ${CODEX_HOME:-~/.codex}/config.toml +``` + +**Expected result.** `CODEX_MODE: ready`. + + +### `CODEX_MODE: unverified` / `MODEL_PROBE_INCONCLUSIVE` + +**Meaning.** The short model check timed out or hit a network error, so gstack +could not confirm the model works. The review still runs and its own result is +checked. + +**Fix.** Nothing now. If the review then fails, its message says why. + + +### `CODEX_MODE: not_authed` / `AUTH_FAILED` + +**Meaning.** No Codex credentials were found: no `CODEX_API_KEY`, no +`OPENAI_API_KEY`, no `auth.json`, and no set environment variable named by a +custom provider's `env_key` in `config.toml`. + +**Fix.** + +```bash +codex login +``` + +or export the variable your provider's `env_key` names. + +**Expected result.** `CODEX_MODE: ready`. + + +### `Codex review skipped (codex_reviews disabled)` / `CODEX_MODE: disabled` + +**Meaning.** You turned outside reviews off. This is `not run`, never a pass and +never an outage. + +**Fix.** To turn them back on: + +```bash +gstack-config set codex_reviews enabled +``` diff --git a/lib/gate-outcomes.ts b/lib/gate-outcomes.ts new file mode 100644 index 000000000..ff1687b8b --- /dev/null +++ b/lib/gate-outcomes.ts @@ -0,0 +1,92 @@ +/** + * INV-1 gate outcome contract: every gate or outside review ends as + * `ran` (with findings or a verdict), `not_run` (gstack chose not to run it) or + * `unavailable` (it tried and could not). Each reason code maps to one state, + * one stable anchor in docs/troubleshooting.md and one fix action; the + * validator prints these lines and test/troubleshooting-anchors.test.ts checks + * every anchor exists. Add a row here before printing a new reason anywhere. + */ +export type GateState = 'ran' | 'not_run' | 'unavailable'; + +export interface GateOutcome { + state: GateState; + /** Stable `` in docs/troubleshooting.md. */ + anchor: string; + /** What happened, in plain words; `` adds the real value (exit code, stderr line). */ + summary: string; + fix: string; +} + +export const GATE_OUTCOMES = { + sandbox_unavailable: { + state: 'unavailable', anchor: 'codex-sandbox-unavailable', + summary: "Codex's sandbox could not start here", + fix: 'enable unprivileged user namespaces for this container, or set GSTACK_CODEX_NO_SANDBOX=1', + }, + commands_failed: { + state: 'unavailable', anchor: 'outside-review-commands-failed', + summary: 'the reviewer could not run commands or read the diff', + fix: 'read the reviewer stderr above, repair the environment it names, then re-run the review', + }, + execution_failed: { + state: 'unavailable', anchor: 'outside-review-execution-failed', + summary: 'the reviewer process failed', + fix: 'read the provider diagnosis above (auth, model, network), repair it, then re-run the review', + }, + timeout: { + state: 'unavailable', anchor: 'outside-review-timeout', + summary: 'the reviewer hit its time limit and was stopped', + fix: 're-run the review with a smaller scope, or check the provider status and network', + }, + empty_response: { + state: 'unavailable', anchor: 'outside-review-empty-response', + summary: 'the reviewer returned no response', + fix: 'read the reviewer stderr above, then re-run the review', + }, + review_refused: { + state: 'unavailable', anchor: 'outside-review-refused', + summary: 'the reviewer declined to review', + fix: 're-run the review; if it declines again, review the change natively', + }, + missing_markers: { + state: 'unavailable', anchor: 'outside-review-missing-markers', + summary: 'the response lacks the markers this gate requires', + fix: 're-run the review; a response without its required markers never counts as a pass', + }, + untagged_review: { + state: 'ran', anchor: 'outside-review-unverified', + summary: 'the review completed without severity tags or an explicit no-findings line', + fix: 'read the output above and decide; it is not a pass', + }, + model_unusable: { + state: 'unavailable', anchor: 'codex-model-unusable', + summary: 'Codex could not use the selected model', + fix: 'set GSTACK_CODEX_MODEL= or fix model (and base_url for a custom provider) in your Codex config.toml', + }, + probe_inconclusive: { + state: 'ran', anchor: 'codex-mode-unverified', + summary: 'the Codex model check timed out or hit a network error, so readiness is unverified', + fix: 'nothing to do now; the review itself is still checked, and a failure there is reported', + }, + auth_failed: { + state: 'unavailable', anchor: 'codex-auth-failed', + summary: 'no Codex credentials were found', + fix: 'run codex login, or set the API key your Codex config.toml provider names in env_key', + }, + disabled: { + state: 'not_run', anchor: 'outside-review-disabled', + summary: 'outside reviews are turned off (codex_reviews=disabled)', + fix: 'gstack-config set codex_reviews enabled', + }, +} as const satisfies Record; + +export type GateReason = keyof typeof GATE_OUTCOMES; + +/** One user-facing line for an outcome that is not a clean `ran`. */ +export function gateOutcomeLine(gate: string, reason: GateReason, detail?: string): string { + const outcome: GateOutcome = GATE_OUTCOMES[reason]; + const what = detail ? `${outcome.summary} (${detail})` : outcome.summary; + if (outcome.state === 'not_run') return `${gate}: not run (${what}). Fix: ${outcome.fix}.`; + if (outcome.state === 'ran') return `${gate}: ran, verdict unverified (${what}). Fix: ${outcome.fix}.`; + return `${gate} unavailable: ${what}. No review ran; this is missing coverage, not a pass. Fix: ${outcome.fix}.`; +} diff --git a/lib/outside-review-result.ts b/lib/outside-review-result.ts index 09dca0b72..8e10fe822 100644 --- a/lib/outside-review-result.ts +++ b/lib/outside-review-result.ts @@ -1,38 +1,146 @@ -/** Review-specific completion evidence, separate from provider transport success. */ -export type OutsideGate = 'review' | 'structured' | 'spec'; -export function validateOutsideReview(text: string, gate: OutsideGate): { completed: boolean; reason?: string; score?: number; gate?: 'pass' | 'fail' } { - if (!text.trim()) return { completed: false, reason: 'empty response' }; - // "I cannot find any issues" is a legitimate clean conclusion. Match a - // refused/unavailable review, not every use of a negative auxiliary verb. - if (/\b(?:(?:I (?:cannot|can't|won't|will not|am unable to)|I'm unable to)\s+(?:review|analy[sz]e|evaluate|assess|inspect|access|complete|perform|provide|assist|help|proceed)|unable to (?:review|analy[sz]e)|I must (?:decline|refuse))\b/i.test(text)) return { completed: false, reason: 'review refused' }; - if (gate === 'spec') { - const scores = [...text.matchAll(/^SCORE:[\t ]*(10|[0-9])[\t ]*\r?$/gm)]; - const ambiguities = [...text.matchAll(/^AMBIGUITIES:[\t ]*(\S[^\r\n]*)\r?$/gm)]; - if (scores.length !== 1 || ambiguities.length !== 1) return { completed: false, reason: 'missing or invalid SCORE/AMBIGUITIES markers' }; - const score = Number(scores[0][1]); - return { completed: true, score, gate: score >= 7 ? 'pass' : 'fail' }; - } - if (gate === 'structured') { - const plain = plainReview(text); - const severity = /\[P[0-3]\]|^P[0-3]:/m.test(plain); - const clear = /\bNO_FINDINGS\b|\bno (?:actionable |significant |new |concrete )?(?:bugs|issues|findings|problems)\b|\b(?:did not|didn't) (?:find|identify) any (?:actionable |new |concrete )?(?:bugs|issues|findings|problems)\b/i.test(text); - if (!severity && !clear) return { completed: false, reason: 'missing severity or explicit no-findings conclusion' }; - return { completed: true, gate: /\[P1\]|^P1:/m.test(plain) ? 'fail' : 'pass' }; - } - // Formatting the requested marker in bold, inline code, or a list does not - // invalidate a completed review. Preserve the explicit action + reason gate. - const plain = plainReview(text); - if (!/^Recommendation:[\t ]*[^\r\n]+\bbecause\b[\t ]*\S[^\r\n]+$/im.test(plain)) return { completed: false, reason: 'missing review completion recommendation' }; - return { completed: true }; +/** + * Review-specific completion evidence, separate from provider transport success. + * + * classifyOutsideReview() is the INV-1 contract: it takes the review text, the + * provider's stderr and exit status and returns three separate answers: did + * the review execute, what is the highest finding severity, and the verdict. validateOutsideReview() is the + * older text-only shape, kept for direct importers and stale rendered skills. + */ +import { GATE_OUTCOMES, gateOutcomeLine, type GateReason } from './gate-outcomes'; + +export type OutsideGate = 'review' | 'structured' | 'spec' | 'execution'; +export type OutsideVerdict = 'clean' | 'findings' | 'unverified' | 'unavailable'; +export type Severity = 'P0' | 'P1' | 'P2' | 'P3'; + +export interface OutsideReviewInput { + text: string; + gate: OutsideGate; + stderr?: string; + exit?: number; } + +export interface OutsideReviewClassification { + execution: { state: 'ran' } | { state: 'unavailable'; reason: GateReason; detail?: string }; + findings: { highest: Severity | null }; + verdict: OutsideVerdict; + /** Set for every verdict except clean/findings. */ + reason?: GateReason; + detail?: string; + score?: number; +} + +const REFUSAL = /\b(?:(?:I (?:cannot|can't|won't|will not|am unable to)|I'm unable to)\s+(?:review|analy[sz]e|evaluate|assess|inspect|access|complete|perform|provide|assist|help|proceed)|unable to (?:review|analy[sz]e)|I must (?:decline|refuse))\b/i; + +function stderrHead(stderr: string): string | undefined { + return stderr.split(/\r?\n/).map(line => line.trim()).find(Boolean)?.slice(0, 240); +} + +function execution(input: OutsideReviewInput): OutsideReviewClassification['execution'] { + const exit = input.exit ?? 0; + if (exit !== 0) { + const head = stderrHead(input.stderr ?? ''); + const detail = head ? `exit ${exit}: ${head}` : `exit ${exit}`; + return { state: 'unavailable', reason: exit === 124 ? 'timeout' : 'execution_failed', detail }; + } + if (!input.text.trim()) return { state: 'unavailable', reason: 'empty_response' }; + if (REFUSAL.test(input.text)) return { state: 'unavailable', reason: 'review_refused' }; + return { state: 'ran' }; +} + +/** Formatting the requested marker in bold, inline code, or a list does not invalidate a completed review. */ function plainReview(text: string): string { return text.split(/\r?\n/).map(line => line.replace(/^[\t ]*(?:#{1,6}[\t ]+|[-+*][\t ]+)?/, '').replace(/[*_`]/g, '')).join('\n'); } + +export function classifyOutsideReview(input: OutsideReviewInput): OutsideReviewClassification { + const plain = plainReview(input.text); + const levels = [...plain.matchAll(/\[(P[0-3])\]|^(P[0-3]):/gm)].map(m => (m[1] ?? m[2]) as Severity); + const findings = { highest: levels.length ? levels.sort()[0]! : null }; + const ran = execution(input); + if (ran.state === 'unavailable') return { execution: ran, findings, verdict: 'unavailable', reason: ran.reason, detail: ran.detail }; + const blocking = findings.highest === 'P0' || findings.highest === 'P1'; + const result = (verdict: OutsideVerdict, reason?: GateReason, detail?: string): OutsideReviewClassification => + ({ execution: ran, findings, verdict, ...(reason ? { reason } : {}), ...(detail ? { detail } : {}) }); + if (input.gate === 'spec') { + const scores = [...input.text.matchAll(/^SCORE:[\t ]*(10|[0-9])[\t ]*\r?$/gm)]; + const ambiguities = [...input.text.matchAll(/^AMBIGUITIES:[\t ]*(\S[^\r\n]*)\r?$/gm)]; + if (scores.length !== 1 || ambiguities.length !== 1) return result('unavailable', 'missing_markers', 'missing or invalid SCORE/AMBIGUITIES markers'); + const score = Number(scores[0]![1]); + return { ...result(score >= 7 ? 'clean' : 'findings'), score }; + } + if (input.gate === 'structured') { + const clear = /\bNO_FINDINGS\b|\bno (?:actionable |significant |new |concrete )?(?:bugs|issues|findings|problems)\b|\b(?:did not|didn't) (?:find|identify) any (?:actionable |new |concrete )?(?:bugs|issues|findings|problems)\b/i.test(input.text); + if (!findings.highest && !clear) return result('unverified', 'untagged_review', 'missing severity or explicit no-findings conclusion'); + return result(blocking ? 'findings' : 'clean'); + } + if (input.gate === 'review' && !/^Recommendation:[\t ]*[^\r\n]+\bbecause\b[\t ]*\S[^\r\n]+$/im.test(plain)) { + return result('unavailable', 'missing_markers', 'missing review completion recommendation'); + } + return result(blocking ? 'findings' : 'clean'); +} + +const LEGACY_REASONS: Partial> = { + empty_response: 'empty response', + review_refused: 'review refused', +}; + +/** Text-only compatibility shape for direct importers (claude-code/SKILL.md.tmpl) and the two-argument CLI. */ +export function validateOutsideReview(text: string, gate: OutsideGate): { completed: boolean; reason?: string; score?: number; gate?: 'pass' | 'fail' } { + const checked = classifyOutsideReview({ text, gate }); + if (checked.verdict === 'unavailable' || checked.verdict === 'unverified') { + const reason = LEGACY_REASONS[checked.reason!] ?? checked.detail ?? GATE_OUTCOMES[checked.reason!].summary; + return { completed: false, reason }; + } + if (gate === 'spec') return { completed: true, score: checked.score, gate: checked.verdict === 'clean' ? 'pass' : 'fail' }; + if (gate === 'structured') return { completed: true, gate: /\[P1\]|^P1:/m.test(plainReview(text)) ? 'fail' : 'pass' }; + return { completed: true }; +} + +const VERDICT_EXIT: Record = { clean: 0, findings: 3, unverified: 4, unavailable: 1 }; +const GATES = ['review', 'structured', 'spec', 'execution']; +const USAGE = `Usage: outside-review-result.ts + outside-review-result.ts --verdict [--stderr ] [--exit ] [--label ] +Gates: ${GATES.join('|')}. Two-argument form exits 0 completed, 1 unavailable, 2 usage. +Verdict form prints VERDICT: clean|findings|unverified|unavailable and exits 0, 3, 4 or 1.`; + +async function readOptional(file: string | undefined): Promise { + if (!file) return ''; + try { return await Bun.file(file).text(); } catch { return ''; } +} + if (import.meta.main) { - const [gate, path] = process.argv.slice(2); - if (!['review', 'structured', 'spec'].includes(gate) || !path) { console.error('Usage: outside-review-result.ts review|structured|spec '); process.exit(2); } - try { - const result = validateOutsideReview(await Bun.file(path).text(), gate as OutsideGate); + const args = process.argv.slice(2); + const flags: Record = {}; + const positional: string[] = []; + let verdictMode = false; + for (let i = 0; i < args.length; i++) { + const arg = args[i]!; + if (arg === '--verdict') verdictMode = true; + else if (['--stderr', '--exit', '--label'].includes(arg) && i + 1 < args.length) { flags[arg] = args[++i]!; verdictMode ||= arg !== '--label'; } + else if (arg.startsWith('--')) { console.error(USAGE); process.exit(2); } + else positional.push(arg); + } + const [gate, path] = positional; + const exit = flags['--exit'] === undefined ? 0 : Number(flags['--exit']); + if (!gate || !GATES.includes(gate) || !path || positional.length !== 2 || !Number.isInteger(exit) + || (!verdictMode && gate === 'execution')) { console.error(USAGE); process.exit(2); } + let text: string; + try { text = await Bun.file(path).text(); } catch (error) { + if (!verdictMode) { console.error(`Outside review unavailable: ${error}`); process.exit(1); } + text = ''; + } + if (!verdictMode) { + const result = validateOutsideReview(text, gate as OutsideGate); if (!result.completed) { console.error(`Outside review unavailable: ${result.reason}; missing coverage.`); process.exit(1); } - } catch (error) { console.error(`Outside review unavailable: ${error}`); process.exit(1); } + process.exit(0); + } + const checked = classifyOutsideReview({ text, gate: gate as OutsideGate, exit, + stderr: await readOptional(flags['--stderr']) }); + console.log(`VERDICT: ${checked.verdict}`); + console.log(`FINDINGS: ${checked.findings.highest ?? 'none'}`); + if (checked.reason) { + console.log(`REASON: ${checked.reason}`); + console.error(gateOutcomeLine(flags['--label'] ?? 'Outside review', checked.reason, checked.detail)); + } + process.exit(VERDICT_EXIT[checked.verdict]); } diff --git a/scripts/resolvers/outside-voice.ts b/scripts/resolvers/outside-voice.ts index a2f3e4934..43a41dbea 100644 --- a/scripts/resolvers/outside-voice.ts +++ b/scripts/resolvers/outside-voice.ts @@ -158,12 +158,13 @@ ${v.id === 'codex' && ctx.skillName === 'autoplan' ? `if [ "$_OUTSIDE_EXIT" -eq _gstack_codex_log_hang "autoplan" "0" || true fi` : ''} cat "$_OUTSIDE_TMP/stderr" >&2 || { [ "$_OUTSIDE_EXIT" -ne 0 ] || _OUTSIDE_EXIT=1; } -if [ "$_OUTSIDE_EXIT" -ne 0 ]; then - echo '${v.label} outside review unavailable: execution failed; missing coverage. Check the provider diagnosis above.' >&2 - exit "$_OUTSIDE_EXIT" -fi -bun "${root}/lib/outside-review-result.ts" ${opts.gate ?? 'review'} "$_OUTSIDE_TMP/text" || exit 1 -${v.id === 'claude-code' ? 'cat "$_OUTSIDE_TMP/text" || exit 1' : ''} +_OUTSIDE_RC=0 +bun "${root}/lib/outside-review-result.ts" --label '${v.label} outside review' --exit "$_OUTSIDE_EXIT" --stderr "$_OUTSIDE_TMP/stderr" ${opts.gate ?? 'review'} "$_OUTSIDE_TMP/text" || _OUTSIDE_RC=$? +${v.id === 'claude-code' ? '[ "$_OUTSIDE_RC" -eq 1 ] || cat "$_OUTSIDE_TMP/text" || exit 1\n' : ''}case "$_OUTSIDE_RC" in + 0|3) ;; + 4) echo 'OUTSIDE_STATUS: unverified provider=${v.id} host=${ctx.host}'; exit 4 ;; + *) [ "$_OUTSIDE_EXIT" -ne 0 ] && exit "$_OUTSIDE_EXIT"; exit 1 ;; +esac echo 'OUTSIDE_STATUS: completed provider=${v.id} host=${ctx.host}'`; } @@ -197,7 +198,7 @@ export function outsideVoiceInvocation(ctx: TemplateContext, requested: OutsideC ${outsideVoiceCommand(ctx, opts)} \`\`\` -Use Bash \`timeout: ${gateMs}\`; show the full response in a \`tool-output\` fence. Require successful execution and valid markers. Refusal, empty/malformed output, missing ${planRecommendation ? 'Recommendation: because ' : opts.purpose === 'design-direction' ? 'Recommendation' : 'score/severity/completion'} markers, timeout or CLI failure means \`outside_status: unavailable\`. ${opts.purpose === 'design-direction' ? 'Continue completed proposals; native completion does not count as outside coverage.' : opts.nativeAlreadyRequired ? 'Retain the required native pass without duplicating it; it cannot complete outside coverage.' : "Use the caller's fallback; missing coverage is never clean/PASS."} ${nativeStructured ? 'Scratch cleanup is automatic.' : 'After either outcome, delete only your private prompt; scratch cleanup is automatic.'}`; +Use Bash \`timeout: ${gateMs}\`; show the full response in a \`tool-output\` fence. Require successful execution and valid markers. Refusal, empty/malformed output, missing ${planRecommendation ? 'Recommendation: because ' : opts.purpose === 'design-direction' ? 'Recommendation' : 'score/severity/completion'} markers, timeout or CLI failure means \`outside_status: unavailable\`. P0/P1 findings block like native ones; \`OUTSIDE_STATUS: unverified\` is missing coverage. ${opts.purpose === 'design-direction' ? 'Continue completed proposals; native completion does not count as outside coverage.' : opts.nativeAlreadyRequired ? 'Retain the required native pass without duplicating it; it cannot complete outside coverage.' : "Use the caller's fallback; missing coverage is never clean/PASS."} ${nativeStructured ? 'Scratch cleanup is automatic.' : 'After either outcome, delete only your private prompt; scratch cleanup is automatic.'}`; } /** diff --git a/test/autoplan-dual-voice-evidence.test.ts b/test/autoplan-dual-voice-evidence.test.ts index 26bd99114..0b6695a0e 100644 --- a/test/autoplan-dual-voice-evidence.test.ts +++ b/test/autoplan-dual-voice-evidence.test.ts @@ -169,6 +169,8 @@ test.each([ 'Outside review unavailable: empty response; missing coverage.', 'Outside review unavailable: review refused; missing coverage.', 'Outside review unavailable: missing review completion recommendation; missing coverage.', + 'Codex outside review unavailable: the reviewer process failed (exit 1: error: 401 Unauthorized). No review ran; this is missing coverage, not a pass. Fix: read the provider diagnosis above (auth, model, network), repair it, then re-run the review.', + 'Codex outside review unavailable: the response lacks the markers this gate requires (missing review completion recommendation). No review ran; this is missing coverage, not a pass. Fix: re-run the review; a response without its required markers never counts as a pass.', ])('actual owned post-execution error proves attempted outside voice, not completion: %s',diagnostic=>{ const f=fixture();f.events[7]=ack('outside','Exit code 1\n'+diagnostic,true); expect(f.read()).toMatchObject({claudeVoiceFired:true,codexAttempted:true,codexVoiceFired:false,codexUnavailable:true,failedOutsideToolUseId:'outside'}); diff --git a/test/gate-tail-lint.test.ts b/test/gate-tail-lint.test.ts new file mode 100644 index 000000000..3a966a206 --- /dev/null +++ b/test/gate-tail-lint.test.ts @@ -0,0 +1,71 @@ +/** + * INV-1 lint: a check that could not run must never look like "nothing found". + * 1. The learnings/timeline query scripts may not end in `2>/dev/null || exit 0` + * (a missing bun read as "no learnings recorded"). + * 2. No template or resolver may wrap a gate binary in `|| true`, which turns + * "could not run" into success. + * 3. Any other `2>/dev/null || exit 0` tail in bin/ is a deliberate best-effort + * exit and says why on the same line (`# best-effort: `). + */ +import { describe, expect, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const QUERY_SCRIPTS = ['bin/gstack-learnings-search', 'bin/gstack-timeline-read']; +const GATE_CALLS = ['outside-review-result.ts', 'gstack-learnings-search', 'gstack-timeline-read', + '_gstack_codex_auth_probe', '_gstack_codex_model_probe', '_gstack_codex_sandbox_preflight']; +const SILENT_EXIT = /2>\/dev\/null \|\| exit 0\b/; + +function walk(dir: string, keep: (file: string) => boolean, out: string[] = []): string[] { + for (const entry of fs.readdirSync(path.join(ROOT, dir), { withFileTypes: true })) { + if (entry.name === 'node_modules' || entry.name.startsWith('.')) continue; + const rel = path.join(dir, entry.name); + if (entry.isDirectory()) walk(rel, keep, out); + else if (keep(rel)) out.push(rel); + } + return out; +} + +function sources(): string[] { + const templates = fs.readdirSync(ROOT, { withFileTypes: true }) + .filter(d => d.isDirectory() && !d.name.startsWith('.') && !['node_modules', 'test'].includes(d.name)) + .flatMap(d => walk(d.name, f => f.endsWith('.tmpl'))); + return [...templates, ...walk('scripts/resolvers', f => f.endsWith('.ts')), ...walk('bin', () => true)]; +} + +describe('gate tails never hide a check that did not run', () => { + test('query scripts do not swallow a failed run as an empty result', () => { + const offenders = QUERY_SCRIPTS.filter(f => SILENT_EXIT.test(fs.readFileSync(path.join(ROOT, f), 'utf8'))); + expect(offenders).toEqual([]); + }); + + test('no gate binary call is wrapped in || true', () => { + const offenders: string[] = []; + for (const rel of sources()) { + fs.readFileSync(path.join(ROOT, rel), 'utf8').split('\n').forEach((line, i) => { + if (/^\s*(?:#|\/\/|\*)/.test(line)) return; + if (GATE_CALLS.some(g => line.includes(g)) && /\|\|\s*true\b/.test(line)) offenders.push(`${rel}:${i + 1}: ${line.trim()}`); + }); + } + expect(offenders).toEqual([]); + }); + + test('other best-effort exits in bin/ carry a same-line reason', () => { + const offenders: string[] = []; + for (const rel of walk('bin', () => true)) { + if (QUERY_SCRIPTS.includes(rel)) continue; + fs.readFileSync(path.join(ROOT, rel), 'utf8').split('\n').forEach((line, i) => { + if (SILENT_EXIT.test(line) && !/#\s*best-effort:\s*\S/.test(line)) offenders.push(`${rel}:${i + 1}: ${line.trim()}`); + }); + } + expect(offenders).toEqual([]); + }); + + test('negative controls: each rule flags a planted offender', () => { + expect(SILENT_EXIT.test('" 2>/dev/null || exit 0')).toBe(true); + expect(/#\s*best-effort:\s*\S/.test('mkdir -p "$X" 2>/dev/null || exit 0 # best-effort: spool is optional')).toBe(true); + const line = '$GSTACK_BIN/gstack-learnings-search --limit 10 2>/dev/null || true'; + expect(GATE_CALLS.some(g => line.includes(g)) && /\|\|\s*true\b/.test(line)).toBe(true); + }); +}); diff --git a/test/helpers/autoplan-dual-voice-evidence.ts b/test/helpers/autoplan-dual-voice-evidence.ts index 541e7614a..63c8c73b0 100644 --- a/test/helpers/autoplan-dual-voice-evidence.ts +++ b/test/helpers/autoplan-dual-voice-evidence.ts @@ -247,9 +247,10 @@ export function autoplanDualVoiceEvidence(transcript: unknown[], options: Autopl if (execution.succeeded && /^OUTSIDE_STATUS: completed provider=codex host=claude$/m.test(execution.output)) { result.codexAttempted = result.codexVoiceFired = true; result.outsideToolUseId = outside.id; } else if (!execution.background && !execution.succeeded && outside.result.error && - /^(?:Codex outside review unavailable: execution failed; missing coverage\. Check the provider diagnosis above\.|Outside review unavailable: (?:empty response|review refused|missing review completion recommendation); missing coverage\.)$/m.test(execution.output)) { + /^(?:Codex outside review unavailable: execution failed; missing coverage\. Check the provider diagnosis above\.|Outside review unavailable: (?:empty response|review refused|missing review completion recommendation); missing coverage\.|Codex outside review unavailable: [^\n]+\. No review ran; this is missing coverage, not a pass\. Fix: [^\n]+)$/m.test(execution.output)) { // These finite diagnostics occur only after the exact generated - // command reaches Codex or its review validator. They prove an + // command reaches Codex or its review validator (lib/gate-outcomes.ts + // lines from the verdict form; the first two are pre-verdict renders). They prove an // attempted voice, never a completed review or comparison. result.codexAttempted = result.codexUnavailable = true; result.failedOutsideToolUseId = outside.id; diff --git a/test/outside-review-result.test.ts b/test/outside-review-result.test.ts new file mode 100644 index 000000000..cb987c471 --- /dev/null +++ b/test/outside-review-result.test.ts @@ -0,0 +1,128 @@ +/** + * INV-1 outside-review contract: classifyOutsideReview() separates execution, + * findings and verdict; validateOutsideReview() and the two-argument CLI keep + * their old shape for importers and installed skills that predate a re-render. + */ +import { afterAll, describe, expect, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { spawnSync } from 'node:child_process'; +import { classifyOutsideReview, validateOutsideReview, type OutsideGate } from '../lib/outside-review-result'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const LIB = path.join(ROOT, 'lib', 'outside-review-result.ts'); +const TMP = fs.mkdtempSync(path.join(os.tmpdir(), 'outside-review-result-')); +afterAll(() => fs.rmSync(TMP, { recursive: true, force: true })); + +let n = 0; +function file(content: string): string { + const p = path.join(TMP, `f${n++}.txt`); + fs.writeFileSync(p, content); + return p; +} +function cli(args: string[]) { + const r = spawnSync(process.execPath, [LIB, ...args], { encoding: 'utf8', timeout: 10000 }); + return { status: r.status, stdout: r.stdout, stderr: r.stderr }; +} + +const RECOMMEND = 'Recommendation: fix the guard because changed.ts loses data.'; + +describe('classifyOutsideReview: separate execution, findings and verdict', () => { + const cases: Array<[string, { text: string; gate: OutsideGate; stderr?: string; exit?: number }, string, string | null, string | undefined]> = [ + ['clean review', { text: RECOMMEND, gate: 'review' }, 'clean', null, undefined], + ['review with P1', { text: `[P1] data loss\n${RECOMMEND}`, gate: 'review' }, 'findings', 'P1', undefined], + ['review with only P2', { text: `[P2] naming\n${RECOMMEND}`, gate: 'review' }, 'clean', 'P2', undefined], + ['review missing recommendation', { text: 'A few observations.', gate: 'review' }, 'unavailable', null, 'missing_markers'], + ['structured untagged prose', { text: 'The change looks reasonable overall.', gate: 'structured' }, 'unverified', null, 'untagged_review'], + ['structured explicit clear', { text: 'NO_FINDINGS', gate: 'structured' }, 'clean', null, undefined], + ['structured native P1 label', { text: 'P1: race in cleanup', gate: 'structured' }, 'findings', 'P1', undefined], + ['spec passing score', { text: 'SCORE: 8\nAMBIGUITIES: NONE', gate: 'spec' }, 'clean', null, undefined], + ['spec failing score', { text: 'SCORE: 4\nAMBIGUITIES: scope', gate: 'spec' }, 'findings', null, undefined], + ['nonzero exit', { text: RECOMMEND, gate: 'review', exit: 1, stderr: '\nerror: 401 Unauthorized\n' }, 'unavailable', null, 'execution_failed'], + ['timeout', { text: 'Partial', gate: 'review', exit: 124 }, 'unavailable', null, 'timeout'], + ['empty', { text: ' \n', gate: 'structured' }, 'unavailable', null, 'empty_response'], + ['refusal', { text: `I cannot review this request. ${RECOMMEND}`, gate: 'review' }, 'unavailable', null, 'review_refused'], + ['consult answer without review markers', { text: 'Use a queue here; the writer pool blocks.', gate: 'execution', stderr: '' }, 'clean', null, undefined], + ]; + for (const [label, input, verdict, highest, reason] of cases) { + test(label, () => { + const result = classifyOutsideReview(input); + expect(result.verdict).toBe(verdict as any); + expect(result.findings.highest).toBe(highest as any); + expect(result.reason).toBe(reason as any); + expect(result.execution.state).toBe(['execution_failed', 'timeout', 'empty_response', 'review_refused'].includes(reason ?? '') ? 'unavailable' : 'ran'); + }); + } + + test('a non-zero exit names the exit code and the first stderr line', () => { + const result = classifyOutsideReview({ text: '', gate: 'review', exit: 1, stderr: '\nerror: 401 Unauthorized\nmore' }); + expect(result.detail).toBe('exit 1: error: 401 Unauthorized'); + }); +}); + +describe('validateOutsideReview keeps the text-only shape for direct importers', () => { + test('existing shapes are unchanged', () => { + expect(validateOutsideReview('', 'review')).toEqual({ completed: false, reason: 'empty response' }); + expect(validateOutsideReview('I must refuse.', 'review')).toEqual({ completed: false, reason: 'review refused' }); + expect(validateOutsideReview('notes', 'review')).toEqual({ completed: false, reason: 'missing review completion recommendation' }); + expect(validateOutsideReview('notes', 'structured')).toEqual({ completed: false, reason: 'missing severity or explicit no-findings conclusion' }); + expect(validateOutsideReview('SCORE: 8', 'spec')).toEqual({ completed: false, reason: 'missing or invalid SCORE/AMBIGUITIES markers' }); + expect(validateOutsideReview('SCORE: 6\nAMBIGUITIES: NONE', 'spec')).toEqual({ completed: true, score: 6, gate: 'fail' }); + expect(validateOutsideReview(RECOMMEND, 'review')).toEqual({ completed: true }); + expect(validateOutsideReview('[P2] nit', 'structured')).toEqual({ completed: true, gate: 'pass' }); + expect(validateOutsideReview('[P1] bug', 'structured')).toEqual({ completed: true, gate: 'fail' }); + }); +}); + +describe('outside-review-result CLI', () => { + test('two-argument form keeps exit 0 completed / 1 unavailable / 2 usage, findings included', () => { + expect(cli(['structured', file('[P1] data loss')]).status).toBe(0); + expect(cli(['review', file(`[P0] data loss\n${RECOMMEND}`)]).status).toBe(0); + const untagged = cli(['structured', file('looks fine')]); + expect(untagged.status).toBe(1); + expect(untagged.stderr).toBe('Outside review unavailable: missing severity or explicit no-findings conclusion; missing coverage.\n'); + expect(cli(['structured', path.join(TMP, 'missing-file')]).status).toBe(1); + expect(cli(['bogus', file('x')]).status).toBe(2); + expect(cli(['execution', file('x')]).status).toBe(2); + expect(cli(['review']).status).toBe(2); + // No VERDICT line: stale callers only read the status. + expect(cli(['structured', file('[P1] data loss')]).stdout).toBe(''); + }); + + test('verdict form prints VERDICT and exits 0 clean / 3 findings / 4 unverified / 1 unavailable', () => { + const clean = cli(['--verdict', 'review', file(RECOMMEND)]); + expect([clean.status, clean.stdout]).toEqual([0, 'VERDICT: clean\nFINDINGS: none\n']); + const findings = cli(['--verdict', 'structured', file('[P0] corrupts data')]); + expect([findings.status, findings.stdout]).toEqual([3, 'VERDICT: findings\nFINDINGS: P0\n']); + const unverified = cli(['--verdict', 'structured', file('looks fine')]); + expect(unverified.status).toBe(4); + expect(unverified.stdout).toContain('VERDICT: unverified\n'); + expect(unverified.stderr).toContain('Outside review: ran, verdict unverified'); + const unavailable = cli(['--verdict', '--label', 'Codex outside review', 'review', file('')]); + expect(unavailable.status).toBe(1); + expect(unavailable.stdout).toContain('VERDICT: unavailable\nFINDINGS: none\nREASON: empty_response\n'); + expect(unavailable.stderr).toMatch(/^Codex outside review unavailable: the reviewer returned no response\. .*Fix: /); + }); + + test('--stderr and --exit imply the verdict form; a missing stderr file reads as empty', () => { + const failed = cli(['--exit', '7', '--stderr', file('fatal: provider exploded\n'), 'review', file(RECOMMEND)]); + expect(failed.status).toBe(1); + expect(failed.stdout).toContain('REASON: execution_failed'); + expect(failed.stderr).toContain('(exit 7: fatal: provider exploded)'); + const missing = cli(['--stderr', path.join(TMP, 'never-written'), 'review', file(RECOMMEND)]); + expect([missing.status, missing.stdout]).toEqual([0, 'VERDICT: clean\nFINDINGS: none\n']); + expect(cli(['--exit', 'x', 'review', file(RECOMMEND)]).status).toBe(2); + }); + + test('an old rendered caller (pre-verdict skill text) still records findings as completed', () => { + // The tail every generated outside-review fence carried before the verdict + // form; bin/ and lib/ run live from the checkout, rendered text may lag. + const response = file('[P1] Seeded data-loss bug\nREVIEW_COMPLETE'); + const old = `bun "$LIB" structured "$RESPONSE" || exit 1\necho 'OUTSIDE_STATUS: completed provider=codex host=claude'`; + const r = spawnSync('bash', ['-c', old], { encoding: 'utf8', timeout: 10000, + env: { ...process.env, LIB, RESPONSE: response } }); + expect(r.status).toBe(0); + expect(r.stdout).toBe('OUTSIDE_STATUS: completed provider=codex host=claude\n'); + }); +}); diff --git a/test/outside-voice-invocation.test.ts b/test/outside-voice-invocation.test.ts index 90eae99e2..adbce5dcb 100644 --- a/test/outside-voice-invocation.test.ts +++ b/test/outside-voice-invocation.test.ts @@ -196,6 +196,21 @@ describe('generated outside-review dispatch', () => { } }); + test('INV-1: the generated caller branches on VERDICT: findings complete, untagged is unverified', () => { + for (const host of ['claude', 'codex'] as const) { + const findings = invoke(host, { structuredBase: 'main', gate: 'structured' }, { FAKE_RESPONSE: '[P0] Seeded corruption' }); + expect(findings.status).toBe(0); + expect(findings.stdout).toContain('VERDICT: findings\nFINDINGS: P0\n'); + expect(findings.stdout).toContain('OUTSIDE_STATUS: completed'); + const untagged = invoke(host, { structuredBase: 'main', gate: 'structured' }, { FAKE_RESPONSE: 'The change reads fine to me.' }); + expect(untagged.status).toBe(4); + expect(untagged.stdout).toContain('VERDICT: unverified'); + expect(untagged.stdout).toContain('OUTSIDE_STATUS: unverified'); + expect(untagged.stdout).not.toContain('OUTSIDE_STATUS: completed'); + expect(untagged.stderr).toContain('ran, verdict unverified'); + } + }); + test('malformed Claude JSON cannot reach completion evaluation', () => { const result = invoke('codex',{}, {FAKE_MODE:'malformed'}); expect(result.status).toBe(1); diff --git a/test/troubleshooting-anchors.test.ts b/test/troubleshooting-anchors.test.ts new file mode 100644 index 000000000..d36da4ef4 --- /dev/null +++ b/test/troubleshooting-anchors.test.ts @@ -0,0 +1,36 @@ +/** INV-1: every gate reason code users can see has a section in docs/troubleshooting.md. */ +import { describe, expect, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { GATE_OUTCOMES, gateOutcomeLine, type GateReason } from '../lib/gate-outcomes'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const doc = fs.readFileSync(path.join(ROOT, 'docs/troubleshooting.md'), 'utf8'); +const anchors = new Set([...doc.matchAll(/<\/a>/g)].map(m => m[1])); + +describe('docs/troubleshooting.md anchors', () => { + for (const [code, outcome] of Object.entries(GATE_OUTCOMES)) { + test(`${code} → #${outcome.anchor}`, () => { + expect(anchors.has(outcome.anchor)).toBe(true); + // The anchor introduces a section: the next line is its heading. + const after = doc.slice(doc.indexOf(``)).split('\n')[1] ?? ''; + expect(after).toMatch(/^#{2,4} /); + }); + } + + test('anchors are unique and every outcome has a fix action', () => { + const all = [...doc.matchAll(/<\/a>/g)].map(m => m[1]); + expect(new Set(all).size).toBe(all.length); + const codes = Object.values(GATE_OUTCOMES).map(o => o.anchor); + expect(new Set(codes).size).toBe(codes.length); + for (const outcome of Object.values(GATE_OUTCOMES)) expect(outcome.fix.trim().length).toBeGreaterThan(0); + }); + + test('outcome lines follow the not run / unavailable / unverified shapes', () => { + const line = (code: GateReason, detail?: string) => gateOutcomeLine('Codex outside review', code, detail); + expect(line('disabled')).toBe('Codex outside review: not run (outside reviews are turned off (codex_reviews=disabled)). Fix: gstack-config set codex_reviews enabled.'); + expect(line('sandbox_unavailable', 'bwrap: No permissions to create new namespace')) + .toBe("Codex outside review unavailable: Codex's sandbox could not start here (bwrap: No permissions to create new namespace). No review ran; this is missing coverage, not a pass. Fix: enable unprivileged user namespaces for this container, or set GSTACK_CODEX_NO_SANDBOX=1."); + expect(line('untagged_review')).toMatch(/^Codex outside review: ran, verdict unverified \(.+\)\. Fix: read the output above/); + }); +});