fix(hooks): option values are not the check verb, wrapper chains stay linear, agent stalls are timed, report re-checks deferred runs (GST-1088 follow-up review)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

changelog.d/capy-gst-1088-lane-a-followup.md

@@ -1 +1 @@
-Follow-ups to the skill routing and contract hooks (GST-1088). `gstack-ship-queue ready` no longer misreads files a PR adds as deleted when the installed gstack is older than the checkout: the paid-case selector now derives dependencies from the checkout under review. Each of the two hook switches now has one default, `SWITCH_DEFAULT` in `lib/skill-route.ts` (both still off), which setup, both hooks, `gstack-config get|list|defaults` and the new `gstack-skill-route --default <switch>` all read, so changing a default is a one-line edit; `gstack-config list` and `defaults` now show both switches, and an unrecognized value takes the default in the hooks as it already did in setup. The contract checker no longer counts installs, builds or version probes (`bun install`, `go build`, `node --version`, `make clean`) as a test run after an edit, and the Stop hook lets a stop through only once while the same Agent dispatches are still running, recording the run as `deferred`, then checks the contract if they are still running at the next stop.
+Follow-ups to the skill routing and contract hooks (GST-1088). `gstack-ship-queue ready` no longer misreads files a PR adds as deleted when the installed gstack is older than the checkout: the paid-case selector now derives dependencies from the checkout under review. Each of the two hook switches now has one default, `SWITCH_DEFAULT` in `lib/skill-route.ts` (both still off), which setup, both hooks, `gstack-config get|list|defaults` and the new `gstack-skill-route --default <switch>` all read, so changing a default is a one-line edit; `gstack-config list` and `defaults` now show both switches, and an unrecognized value takes the default in the hooks as it already did in setup. The contract checker no longer counts installs, builds or version probes (`bun install`, `go build`, `node --version`, `make clean`) as a test run after an edit, while option values (`make -C web test`, `pnpm --filter app test`) and `node --test` still count, and the Stop hook lets a final stop through while Agent dispatches are still running, recording the run as `deferred`, then checks the contract once the same agents have been running for 10 minutes; `gstack-skill-contract report` re-checks deferred runs.

docs/skill-routing-and-contracts.md

@@ -109,13 +109,13 @@ The hook checks the most recently loaded skill that declares a contract, and onl
 - the session is headless or unattended (`gstack-session-kind`), or
 - the turn does not end on a question to the user (a last paragraph ending in "?", an AskUserQuestion call, or `NEEDS_CONTEXT`).
 
-So an interactive session that pauses to ask a question is never blocked. When Claude Code says a Stop hook already continued this turn (`stop_hook_active`), the hook never blocks: after its own block it records the result (`met_after_block` or `miss`). It also allows the stop once while Agent dispatches are still running (no result yet, or launched in the background with no completion notice naming that agent), because their outputs may still arrive, and records the run as `deferred`. If the same agents are still running at the next stop, they count as stalled and the contract is checked. Because Claude Code can fire Stop before the turn's last message reaches the transcript file, the hook reads the payload's `last_assistant_message` as the turn's final text.
+So an interactive session that pauses to ask a question is never blocked. When Claude Code says a Stop hook already continued this turn (`stop_hook_active`), the hook never blocks: after its own block it records the result (`met_after_block` or `miss`). On a final turn it also allows the stop while Agent dispatches are still running (no result yet, or launched in the background with no completion notice naming that agent), because their outputs may still arrive, and records the run as `deferred`. Once the same agents have been running for 10 minutes since the stop that first saw them, they count as stalled and the contract is checked at the next stop. Because Claude Code can fire Stop before the turn's last message reaches the transcript file, the hook reads the payload's `last_assistant_message` as the turn's final text.
 
 It blocks at most once per skill run. If the model stops again with the contract still unmet, the stop is allowed and the run is recorded as a miss.
 
 ### Records
 
-Each Claude Code session has one record, `$GSTACK_STATE_ROOT/skill-contracts/<session id>.json`, written atomically (mode 0600) before any block. Each skill run in it has an outcome: `met`, `met_waived`, `blocked`, `met_after_block`, `miss` or `deferred` (stopped while its agents were still running, not yet checked). The hook's decisions also go to `$GSTACK_STATE_ROOT/hooks/skill-contract.log`.
+Each Claude Code session has one record, `$GSTACK_STATE_ROOT/skill-contracts/<session id>.json`, written atomically (mode 0600) before any block. Each skill run in it has an outcome: `met`, `met_waived`, `blocked`, `met_after_block`, `miss` or `deferred` (stopped while its agents were still running, not yet checked; `report` re-checks it against its transcript). The hook's decisions also go to `$GSTACK_STATE_ROOT/hooks/skill-contract.log`.
 
 ## Commands
 

hosts/claude/hooks/skill-contract-stop-hook.ts

@@ -7,11 +7,12 @@
  *
  *   stdin -> switch on? -> transcript_path -> parse (+ last_assistant_message)
  *     -> the most recently loaded skill that declares a contract (none: allow)
- *     -> Agent dispatches still running? allow once per set of running agents
- *        (outputs may still arrive); the same set still running at the next
- *        stop is stalled, so the contract is checked
  *     -> final turn? (a STATUS line, a headless or unattended session, or no
- *        pending user question) -> checkContract (lib/skill-contract.ts)
+ *        pending user question; otherwise allow)
+ *     -> Agent dispatches still running? allow (outputs may still arrive),
+ *        until the same set has been running AGENT_STALL_MS since the stop
+ *        that first saw it; then it is stalled and the contract is checked
+ *     -> checkContract (lib/skill-contract.ts)
  *     -> unmet and this run never blocked: record "blocked", then block with
  *        blockMessage(); unmet again later: record "miss" and allow
  *     -> stop_hook_active (a Stop hook already blocked this turn): record the
@@ -30,7 +31,7 @@
 import { runBin } from './spawn-bin';
 import { appendHookTrace, installedSkillsDir, readSkillSwitch, readStdinBounded } from '../../../lib/skill-route';
 import {
-  StateError, UnknownShape, blockMessage, bumpCanary, checkContract, lookupContract, nextOutcome, parseTranscript,
+  AGENT_STALL_MS, StateError, UnknownShape, blockMessage, bumpCanary, checkContract, lookupContract, nextOutcome, parseTranscript,
   readRecord, readTranscriptFile, recordPath, runKey, sameSkill, skillLoads, stateErrorLine, withFinalMessage, writeRecord,
   type CheckResult, type Contract, type Transcript,
 } from '../../../lib/skill-contract';
@@ -110,18 +111,24 @@ async function main(): Promise<void> {
   const record = readRecord(file) ?? { version: 1 as const, session_id: sessionId, transcript_path: transcriptPath, runs: {} };
   const key = runKey(t.skill, result.load);
   const prev = record.runs[key];
-  const pending = [...result.pendingAgents].sort();
-  if (pending.length && pending.join(',') !== (prev?.deferred_agents ?? []).join(',')) {
-    record.transcript_path = transcriptPath;
-    record.runs[key] = {
-      skill: t.skill, load_line: result.load.line, outcome: prev?.outcome ?? 'deferred', missing: prev?.missing ?? [], waived: prev?.waived ?? [],
-      ...(prev?.blocked_at ? { blocked_at: prev.blocked_at } : {}), deferred_agents: pending, updated_at: new Date().toISOString(),
-    };
-    writeRecord(file, record);
-    return trace({ event: 'agents-running', skill: t.skill, agents: pending.length });
-  }
-  if (pending.length) trace({ event: 'agents-stalled', skill: t.skill, agents: pending.length });
   if (!isFinalTurn(result)) return trace({ event: 'not-final-turn', skill: t.skill });
+  const pending = [...result.pendingAgents].sort();
+  if (pending.length) {
+    const sameSet = pending.join(',') === (prev?.deferred_agents ?? []).join(',') && !!prev?.deferred_at;
+    if (!sameSet) {
+      const now = new Date().toISOString();
+      record.transcript_path = transcriptPath;
+      record.runs[key] = {
+        skill: t.skill, load_line: result.load.line, outcome: prev?.outcome ?? 'deferred', missing: prev?.missing ?? [], waived: prev?.waived ?? [],
+        ...(prev?.blocked_at ? { blocked_at: prev.blocked_at } : {}), deferred_agents: pending, deferred_at: now, updated_at: now,
+      };
+      writeRecord(file, record);
+    }
+    if (!sameSet || !(Date.now() - Date.parse(prev!.deferred_at!) >= AGENT_STALL_MS)) {
+      return trace({ event: 'agents-running', skill: t.skill, agents: pending.length });
+    }
+    trace({ event: 'agents-stalled', skill: t.skill, agents: pending.length });
+  }
   const { outcome, block } = nextOutcome(prev, result);
   if (block && stopActive) return trace({ event: 'stop-hook-active', skill: t.skill });
   const missing = result.missing.join(',') || '-';

lib/skill-contract.ts

@@ -268,7 +268,7 @@ export const STATUS_RE = /^[ \t>*_-]*(?:\*\*|__)?(?:STATUS|Status)(?:\*\*|__)?[
 const EDIT_TOOLS = /^(?:Edit|Write|MultiEdit|NotebookEdit)$/;
 const AGENT_TOOLS = /^(?:Agent|Task)$/;
 const TEST_RUNNER = /^(?:pytest|py\.test|jest|vitest|mocha|rspec|phpunit|ctest|tox|nox|tsc|rake|playwright|cypress)$/i;
-const CHECK_SCRIPT = /(?:^|[-_.:])(?:tests?|spec|repro|check|verify|lint)(?:[-_.:]|$)/i;
+const CHECK_SCRIPT = /(?:^|[-_.:])(?:tests?|spec|repro|check|typecheck|verify|lint)(?:[-_.:]|$)/i;
 const CHECK_VERB = /^(?:tests?|t|check|verify|spec|vet|clippy|nextest|lint|e2e)$/i;
 const SUBCOMMAND_TOOL = /^(?:make|cargo|go|dotnet|swift|mix|bazel|just|dart|flutter|rails|mvn|gradlew?|xcodebuild|bun|npm|pnpm|yarn|deno)$/i;
 const RUNS_PROGRAM = /^(?:cargo|go|dotnet|swift)$/i;
@@ -278,22 +278,39 @@ const FILE_INTERPRETER = /^(?:bun|deno)$/i;
 const COMMAND_WRAPPER = /^(?:bundle|uv|poetry|pipenv|docker|podman|xvfb-run|bash|sh|zsh)$/;
 const COMMAND_PREFIX = /^(?:env|sudo|time|nice|timeout|command|exec|\w+=\S*|-\S*|\d+[smh]?)$/;
 const SCRIPT_ARG = /[\\/]|\.(?:[cm]?[jt]sx?|py|rb|php|pl|sh)$/;
+/** Options whose next word is their value (`make -C web test`, `pnpm --filter app test`, `xcodebuild -scheme App test`). */
+const VALUE_FLAG = /^(?:-[CfpwF]|--(?:prefix|filter|cwd|dir|directory|workspace|package|manifest-path|project|file)|-(?:scheme|project|workspace|destination|configuration|target|sdk))$/;
+const WRAPPER_DEPTH = 3;
 const baseName = (word: string) => word.split(/[\\/]/).pop() ?? '';
 const isCheckScript = (word: string) => { const name = baseName(word); return name !== 'test' && CHECK_SCRIPT.test(name); };
 
-/** One command's words (prefixes removed): does it run tests, a check, or a repro? Installs, builds and version probes do not. */
-function runsCheck(words: string[]): boolean {
+/** The words after the program that are not options or option values. */
+function positionals(words: string[]): string[] {
+  const out: string[] = [];
+  for (let i = 1; i < words.length; i++) {
+    if (words[i] === '--') break;
+    if (!words[i].startsWith('-')) out.push(words[i]);
+    else if (VALUE_FLAG.test(words[i])) i++;
+  }
+  return out;
+}
+
+/**
+ * One command's words (prefixes removed): does it run tests, a check, or a repro? Installs, builds and version probes
+ * do not. Wrappers nest at most WRAPPER_DEPTH deep, so a long chain of wrapper words costs linear time, not exponential.
+ */
+function runsCheck(words: string[], depth = 0): boolean {
   const program = baseName(words[0] ?? '');
-  const args = words.slice(1).filter(w => !w.startsWith('-'));
+  const args = positionals(words);
   if (TEST_RUNNER.test(program) || isCheckScript(words[0] ?? '')) return true;
-  if (PACKAGE_EXEC.test(program)) return runsCheck(words.slice(1).filter(w => !w.startsWith('-')));
-  if (COMMAND_WRAPPER.test(program)) return [1, 2, 3, 4, 5].some(i => i < words.length && runsCheck(words.slice(i)));
-  if (INTERPRETER.test(program)) return args.length > 0 || words.slice(1).some(w => /^-[ecm]$/.test(w));
+  if (depth < WRAPPER_DEPTH && PACKAGE_EXEC.test(program)) return runsCheck(words.slice(1).filter(w => !w.startsWith('-')), depth + 1);
+  if (COMMAND_WRAPPER.test(program)) return depth < WRAPPER_DEPTH && [1, 2, 3, 4, 5].some(i => i < words.length && runsCheck(words.slice(i), depth + 1));
+  if (INTERPRETER.test(program)) return args.length > 0 || words.slice(1).some(w => /^-[ecm]$|^--test$/.test(w));
   if (program === 'curl') return args.some(a => /^https?:\/\/|^localhost|^127\./.test(a));
   if (FILE_INTERPRETER.test(program) && args[0] && SCRIPT_ARG.test(args[0])) return true;
   if (!SUBCOMMAND_TOOL.test(program) || !args[0]) return false;
   const [verb, next] = args;
-  if (CHECK_VERB.test(verb) || isCheckScript(verb)) return true;
+  if (CHECK_VERB.test(verb) || isCheckScript(verb) || args.some(a => CHECK_VERB.test(a))) return true;
   if (verb === 'run' || verb === 'exec' || verb === 'x') return RUNS_PROGRAM.test(program) || (!!next && (CHECK_VERB.test(next) || isCheckScript(next) || TEST_RUNNER.test(baseName(next)) || SCRIPT_ARG.test(next)));
   return /^(?:yarn|pnpm)$/i.test(program) && TEST_RUNNER.test(verb);
 }
@@ -521,8 +538,10 @@ export function lookupContract(skill: string, skillsDir: string, skillFile?: str
 }
 
 export type Outcome = 'met' | 'met_waived' | 'blocked' | 'met_after_block' | 'miss' | 'deferred';
-/** deferred_agents: the Agent dispatches still running at the last stop the hook let through unchecked. */
-export interface RunRecord { skill: string; load_line: number; outcome: Outcome; missing: string[]; waived: string[]; blocked_at?: string; deferred_agents?: string[]; updated_at: string }
+/** deferred_agents: the Agent dispatches still running at the last stop the hook let through unchecked; deferred_at: when that set was first seen. */
+export interface RunRecord { skill: string; load_line: number; outcome: Outcome; missing: string[]; waived: string[]; blocked_at?: string; deferred_agents?: string[]; deferred_at?: string; updated_at: string }
+/** The same Agent dispatches still running this long after the stop that first saw them count as stalled. */
+export const AGENT_STALL_MS = 10 * 60 * 1000;
 export interface SessionRecord { version: 1; session_id: string; transcript_path: string; runs: Record<string, RunRecord> }
 
 export function recordsDir(env: NodeJS.ProcessEnv = process.env): string {
@@ -588,16 +607,15 @@ export function bumpCanary(why: string, env: NodeJS.ProcessEnv = process.env): n
 
 export interface ReportRow { session: string; skill: string; outcome: Outcome | 'fixed_after_block'; missing: string[]; waived: string[]; transcript: string }
 
-/** Every recorded run; a run still marked blocked is re-checked against its transcript when a recheck is given. */
+/** Every recorded run; a run still marked blocked or deferred is re-checked against its transcript when a recheck is given. */
 export function reportRows(records: SessionRecord[], recheck?: (rec: SessionRecord, run: RunRecord) => CheckResult | null): ReportRow[] {
   return records.flatMap(rec => Object.values(rec.runs).map(run => {
     const row: ReportRow = { session: rec.session_id, skill: run.skill, outcome: run.outcome, missing: run.missing, waived: run.waived, transcript: rec.transcript_path };
-    if (run.outcome !== 'blocked' || !recheck) return row;
+    if ((run.outcome !== 'blocked' && run.outcome !== 'deferred') || !recheck) return row;
     const again = recheck(rec, run);
     if (!again) return row;
-    return again.met
-      ? { ...row, outcome: 'fixed_after_block' as const, missing: [], waived: again.waived }
-      : { ...row, outcome: 'miss' as const, missing: again.missing, waived: again.waived };
+    if (!again.met) return { ...row, outcome: 'miss' as const, missing: again.missing, waived: again.waived };
+    return { ...row, outcome: run.outcome === 'blocked' ? 'fixed_after_block' as const : 'met' as const, missing: [], waived: again.waived };
   }));
 }
 

test/skill-contract-stop-hook.test.ts

@@ -7,9 +7,9 @@
 import { afterEach, beforeEach, describe, expect, test } from 'bun:test';
 import * as fs from 'node:fs';
 import * as path from 'node:path';
-import { STATE_ERROR_DOC, stateErrorLine } from '../lib/skill-contract';
+import { AGENT_STALL_MS, STATE_ERROR_DOC, stateErrorLine } from '../lib/skill-contract';
 import {
-  CONTRACT_HOOK, DEBUG_REPORT, INTERACTIVE_TURNS, J1_TRANSCRIPT, ROOT, hookEnv, result, runHook, say, skillsDir, slash, tmpDir, tool, user,
+  CONTRACT_HOOK, DEBUG_REPORT, INTERACTIVE_TURNS, J1_TRANSCRIPT, ROOT, hookEnv, meta, result, runHook, say, skillsDir, slash, tmpDir, tool, user,
 } from './helpers/skill-contract-fixture';
 
 const PINNED = fs.readFileSync(path.join(ROOT, 'test', 'fixtures', 'skill-contract', 'block-message.txt'), 'utf8').trimEnd();
@@ -135,7 +135,7 @@ describe('never blocks the wrong turn', () => {
     expect(JSON.parse(stop(file, { GSTACK_HEADLESS: '1' }).stdout).decision).toBe('block');
   });
 
-  test('Agent readers still running when Stop fires: allowed once and recorded deferred; the same set still running next stop is checked', () => {
+  test('Agent readers still running when Stop fires: allowed and recorded deferred until the same set has run AGENT_STALL_MS, then checked', () => {
     const base = [slash('review'), say('Dispatching 2 specialists: Testing, Security. Skipped: Performance (scope not detected).')];
     const background = [...base, tool('a1', 'Agent', { description: 'Testing specialist', prompt: 'review', run_in_background: true }),
       result('a1', 'Async agent launched in the background.'), say('Waiting for the specialists to finish.')];
@@ -143,7 +143,13 @@ describe('never blocks the wrong turn', () => {
     expect(stop(file)).toEqual({ code: 0, stdout: '', stderr: '' });
     expect(Object.values(record().runs)[0]).toMatchObject({ outcome: 'deferred', deferred_agents: ['a1'] });
     expect(fs.readFileSync(path.join(state, 'hooks', 'skill-contract.log'), 'utf8')).toContain('event=agents-running');
-    fs.appendFileSync(file, `${say('Still waiting on the specialists.')}\n`);
+    fs.appendFileSync(file, `${user('how is it going?')}\n${meta('<task-notification>bash b9 completed</task-notification>')}\n${say('Still waiting on the specialists.')}\n`);
+    expect(stop(file)).toEqual({ code: 0, stdout: '', stderr: '' });
+    expect(Object.values(record().runs)[0]).toMatchObject({ outcome: 'deferred', deferred_agents: ['a1'] });
+    const rec = record();
+    const key = Object.keys(rec.runs)[0];
+    rec.runs[key].deferred_at = new Date(Date.now() - AGENT_STALL_MS - 1000).toISOString();
+    fs.writeFileSync(path.join(state, 'skill-contracts', 'sess-1.json'), JSON.stringify(rec));
     expect(JSON.parse(stop(file).stdout).decision).toBe('block');
     expect(Object.values(record().runs)[0]).toMatchObject({ outcome: 'blocked' });
     expect(fs.readFileSync(path.join(state, 'hooks', 'skill-contract.log'), 'utf8')).toContain('event=agents-stalled');
@@ -151,6 +157,13 @@ describe('never blocks the wrong turn', () => {
     expect(stop(transcript(unanswered, 'unanswered.jsonl'), {}, { session_id: 'sess-2' })).toEqual({ code: 0, stdout: '', stderr: '' });
   });
 
+  test('a stop on a turn that ends on a question never starts the agent clock', () => {
+    const asking = [slash('review'), tool('a1', 'Agent', { description: 'Testing specialist', prompt: 'review', run_in_background: true }),
+      result('a1', 'Async agent launched in the background.'), say('Should I also review the migrations?')];
+    expect(stop(transcript(asking))).toEqual({ code: 0, stdout: '', stderr: '' });
+    expect(fs.existsSync(path.join(state, 'skill-contracts', 'sess-1.json'))).toBe(false);
+  });
+
   test('no contract skill loaded: allowed', () => {
     const r = stop(transcript([slash('qa'), say('Done. STATUS: DONE')]));
     expect(r).toEqual({ code: 0, stdout: '', stderr: '' });

test/skill-contract.test.ts

@@ -8,7 +8,8 @@ import { spawnSync } from 'node:child_process';
 import * as fs from 'node:fs';
 import * as path from 'node:path';
 import {
-  DEFAULT_MATCHERS, EXIT, STATUS_RE, blockMessage, checkContract, isCheckCommand, isPlaceholder, parseContract, parseTranscript, skillLoads, UnknownShape,
+  DEFAULT_MATCHERS, EXIT, STATUS_RE, blockMessage, checkContract, isCheckCommand, isMiss, isPlaceholder, parseContract, parseTranscript, reportRows, skillLoads, UnknownShape,
+  type CheckResult,
   type Contract,
 } from '../lib/skill-contract';
 import { readFrontmatter } from '../lib/skill-route';
@@ -183,6 +184,26 @@ describe('checkContract is a pure function of the transcript', () => {
       expect([cmd, isCheckCommand(cmd)]).toEqual([cmd, false]);
   });
 
+  test('check_after_last_edit: an option value is never the verb, and a long chain of wrapper words stays fast', () => {
+    for (const cmd of ['make -C web test', 'npm --prefix web test', 'pnpm --filter app test', 'bun --cwd app test', 'yarn workspace app test',
+      'xcodebuild -scheme App test', 'cargo +nightly test', 'mvn clean test', 'bun run typecheck', 'node --test'])
+      expect([cmd, isCheckCommand(cmd)]).toEqual([cmd, true]);
+    for (const cmd of ['make -C web build', 'npm --prefix web install', 'pnpm --filter app build', 'xcodebuild -scheme App build'])
+      expect([cmd, isCheckCommand(cmd)]).toEqual([cmd, false]);
+    const started = performance.now();
+    expect(isCheckCommand(`${Array(40).fill('sh').join(' ')} echo`)).toBe(false);
+    expect(performance.now() - started).toBeLessThan(500);
+  });
+
+  test('report re-checks a deferred run: unmet is a miss, met is met', () => {
+    const run = { skill: 'review', load_line: 1, outcome: 'deferred' as const, missing: [], waived: [], deferred_agents: ['a1'], updated_at: '' };
+    const rec = { version: 1 as const, session_id: 's', transcript_path: 't', runs: { k: run } };
+    const unmet = reportRows([rec], () => ({ met: false, missing: ['specialists'], waived: [] }) as unknown as CheckResult);
+    expect(unmet.map(r => [r.outcome, isMiss(r)])).toEqual([['miss', true]]);
+    const met = reportRows([rec], () => ({ met: true, missing: [], waived: [] }) as unknown as CheckResult);
+    expect(met.map(r => [r.outcome, isMiss(r)])).toEqual([['met', false]]);
+  });
+
   test('selected_specialists_dispatched: a selection line only counts from assistant text or a Bash result, with plain names', () => {
     const red = [tool('c', 'Agent', { description: 'Red team', prompt: 'Think like an attacker and a chaos engineer.' }), result('c', '[]')];
     const report = say('Pre-Landing Review: 0 blocking\nGate edits: none\nSTATUS: DONE');