From afd766600baca7f7a45d4a8b69f2547be56070ed Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 06:43:26 +0000 Subject: [PATCH 1/4] fix(ship-queue, hooks): selector reads the checkout under review; config lists the hook switches; installs are not checks; one deferral per running agent set (GST-1088 follow-up) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-config | 8 +++- changelog.d/capy-gst-1088-lane-a-followup.md | 1 + docs/skill-routing-and-contracts.md | 4 +- .../claude/hooks/skill-contract-stop-hook.ts | 18 ++++++-- lib/skill-contract.ts | 44 +++++++++++++++---- scripts/lib/paid-select.ts | 7 ++- test/gstack-config-defaults.test.ts | 25 ++++++++--- test/pr-dependencies.test.ts | 14 ++++++ test/skill-contract-stop-hook.test.ts | 16 ++++--- test/skill-contract.test.ts | 5 ++- 10 files changed, 111 insertions(+), 31 deletions(-) create mode 100644 changelog.d/capy-gst-1088-lane-a-followup.md diff --git a/bin/gstack-config b/bin/gstack-config index c9e4f72fc..fde1fbe5c 100755 --- a/bin/gstack-config +++ b/bin/gstack-config @@ -344,6 +344,8 @@ lookup_default() { artifacts_sync_removals) echo "off" ;; # on | off — publish removals of synced files gone from disk plan_tune_hooks) echo "prompt" ;; # prompt | yes | no — controls ./setup plan-tune hook install timeline_stop_hook) echo "yes" ;; # yes | no — controls ./setup timeline Stop hook registration (#2677) + skill_routing) echo "off" ;; # on | off — ./setup registers the skill-routing UserPromptSubmit hook (docs/skill-routing-and-contracts.md) + skill_contracts) echo "off" ;; # on | off — ./setup registers the skill-contract Stop hook (docs/skill-routing-and-contracts.md) redact_repo_visibility) echo "" ;; # empty → fall through to gh/glab detection redact_prepush_hook) echo "false" ;; @@ -1139,7 +1141,8 @@ $_gstack_legacy_root" ship_measure_judge_outputs ship_measure_ask_per_trial_usd ship_measure_budget_usd \ ship_measure_max_rounds ship_measure_sweep_cases ship_measure_sweep_budget_usd ship_rerun_backend test_backend \ plan_review_tier implementation_tier model_frontier_claude model_frontier_openai \ - model_smart_claude model_smart_openai codex_effort review_effort_tiers agent_model harness_start_hook; do + model_smart_claude model_smart_openai codex_effort review_effort_tiers agent_model harness_start_hook \ + skill_routing skill_contracts; do gstack_config_select "$KEY" VALUE="$_gstack_cfg_value" SOURCE="default" @@ -1164,7 +1167,8 @@ $_gstack_legacy_root" ship_measure_judge_outputs ship_measure_ask_per_trial_usd ship_measure_budget_usd \ ship_measure_max_rounds ship_measure_sweep_cases ship_measure_sweep_budget_usd ship_rerun_backend test_backend \ plan_review_tier implementation_tier model_frontier_claude model_frontier_openai \ - model_smart_claude model_smart_openai codex_effort review_effort_tiers agent_model harness_start_hook; do + model_smart_claude model_smart_openai codex_effort review_effort_tiers agent_model harness_start_hook \ + skill_routing skill_contracts; do printf ' %-24s %s\n' "$KEY:" "$(lookup_default "$KEY")" done ;; diff --git a/changelog.d/capy-gst-1088-lane-a-followup.md b/changelog.d/capy-gst-1088-lane-a-followup.md new file mode 100644 index 000000000..04327a872 --- /dev/null +++ b/changelog.d/capy-gst-1088-lane-a-followup.md @@ -0,0 +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. `gstack-config list` and `defaults` show `skill_routing` and `skill_contracts` (both off by default). 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. diff --git a/docs/skill-routing-and-contracts.md b/docs/skill-routing-and-contracts.md index f0eb59b84..ecb34374f 100644 --- a/docs/skill-routing-and-contracts.md +++ b/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 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. 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`). 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. 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/.json`, written atomically (mode 0600) before any block. Each skill run in it has an outcome: `met`, `met_waived`, `blocked`, `met_after_block` or `miss`. 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/.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`. ## Commands diff --git a/hosts/claude/hooks/skill-contract-stop-hook.ts b/hosts/claude/hooks/skill-contract-stop-hook.ts index f46da16a2..125cd6474 100755 --- a/hosts/claude/hooks/skill-contract-stop-hook.ts +++ b/hosts/claude/hooks/skill-contract-stop-hook.ts @@ -7,7 +7,9 @@ * * 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 (outputs may still arrive) + * -> 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) * -> unmet and this run never blocked: record "blocked", then block with @@ -104,12 +106,22 @@ async function main(): Promise { if (!t) return trace({ event: 'no-contract-skill' }); const result = checkContract(transcript, t.skill, t.contract); if (!result.load) return trace({ event: 'no-contract-skill' }); - if (result.pendingAgents.length) return trace({ event: 'agents-running', skill: t.skill, agents: result.pendingAgents.length }); - if (!isFinalTurn(result)) return trace({ event: 'not-final-turn', skill: t.skill }); const file = recordPath(sessionId); 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 { outcome, block } = nextOutcome(prev, result); if (block && stopActive) return trace({ event: 'stop-hook-active', skill: t.skill }); const missing = result.missing.join(',') || '-'; diff --git a/lib/skill-contract.ts b/lib/skill-contract.ts index bd93e34cb..301eb9f54 100644 --- a/lib/skill-contract.ts +++ b/lib/skill-contract.ts @@ -267,23 +267,48 @@ export function skillLoads(t: Transcript): SkillLoad[] { export const STATUS_RE = /^[ \t>*_-]*(?:\*\*|__)?(?:STATUS|Status)(?:\*\*|__)?[ \t]*(?:\*\*|__)?:[ \t]*(?:\*\*|__)?[ \t]*(DONE_WITH_CONCERNS|DONE|BLOCKED|NEEDS_CONTEXT)\b/m; const EDIT_TOOLS = /^(?:Edit|Write|MultiEdit|NotebookEdit)$/; const AGENT_TOOLS = /^(?:Agent|Task)$/; -const CHECK_RUNNER = /^(?:pytest|jest|vitest|mocha|rspec|phpunit|ctest|tox|nox|tsc|make|cargo|go|bun|bunx|npm|npx|pnpm|yarn|node|deno|python3?|ruby|rake|mvn|gradlew?|dotnet|swift|xcodebuild|curl|mix|bazel|just|dart|flutter|rails)$/i; -const CHECK_SCRIPT = /(?:^|[-_.])(?:tests?|spec|repro|check|verify|lint)(?:[-_.]|$)/i; +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_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; +const PACKAGE_EXEC = /^(?:npx|bunx|pnpx)$/i; +const INTERPRETER = /^(?:node|python3?|ruby|php|perl|tsx|ts-node)$/i; +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 isCheckProgram = (word: string) => { const name = word.split(/[\\/]/).pop() ?? ''; return CHECK_RUNNER.test(name) || (name !== 'test' && CHECK_SCRIPT.test(name)); }; +const SCRIPT_ARG = /[\\/]|\.(?:[cm]?[jt]sx?|py|rb|php|pl|sh)$/; +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 { + const program = baseName(words[0] ?? ''); + const args = words.slice(1).filter(w => !w.startsWith('-')); + 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 (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 (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); +} const SAFE_SPECIALIST = /^[A-Za-z0-9][A-Za-z0-9 _.-]{0,39}$/; /** - * A test or repro run: some segment of the command (split on && || ; | and newlines) starts with a test runner or a - * test, check or repro script, directly or behind a wrapper (bundle exec, uv run, docker compose run, xvfb-run, bash). + * A test or repro run: some segment of the command (split on && || ; | and newlines) runs a test runner, a test verb + * (`npm test`, `go test`, `make check`), a test, check or repro script, or code through an interpreter, directly or + * behind a wrapper (bundle exec, uv run, docker compose run, xvfb-run, bash). Installs, builds and version probes do not count. */ export function isCheckCommand(cmd: string): boolean { return cmd.split(/&&|\|\||[;|\n]/).some(segment => { const words = segment.trim().replace(/^[({]\s*/, '').split(/\s+/); while (words.length > 1 && COMMAND_PREFIX.test(words[0])) words.shift(); - if (isCheckProgram(words[0])) return true; - return COMMAND_WRAPPER.test(words[0].split(/[\\/]/).pop() ?? '') && words.slice(1, 6).some(isCheckProgram); + return runsCheck(words); }); } const WAIVE_RE = /gstack-skill-contract["']?\s+waive\s+["']?([A-Za-z0-9][A-Za-z0-9_.-]*)["']?/; @@ -495,8 +520,9 @@ export function lookupContract(skill: string, skillsDir: string, skillFile?: str return 'error' in parsed ? { skill, file, error: `${file}: ${parsed.error}`, reason: 'invalid' } : { skill, file, contract: parsed.contract }; } -export type Outcome = 'met' | 'met_waived' | 'blocked' | 'met_after_block' | 'miss'; -export interface RunRecord { skill: string; load_line: number; outcome: Outcome; missing: string[]; waived: string[]; blocked_at?: string; updated_at: string } +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 } export interface SessionRecord { version: 1; session_id: string; transcript_path: string; runs: Record } export function recordsDir(env: NodeJS.ProcessEnv = process.env): string { diff --git a/scripts/lib/paid-select.ts b/scripts/lib/paid-select.ts index 192a4363a..2b2f75bae 100644 --- a/scripts/lib/paid-select.ts +++ b/scripts/lib/paid-select.ts @@ -10,7 +10,8 @@ import { isPaidTestFile } from '../../test/helpers/paid-test-set'; import { PERIODIC_CI_EXCLUDE } from '../../test/helpers/periodic-exclude-data'; import { FILE_RETRY_BUDGETS } from '../../test/helpers/eval-budgets'; import { OVERLAY_MIN_FILE_WALL_MS } from '../../test/helpers/overlay-case-policy'; -import { packageChangeOnlyVersion, prProfileFileCases, selectPrProfile, type PrProfileSelection } from '../test-pr-profile'; +import { PR_PROFILE_MAPS, packageChangeOnlyVersion, prProfileFileCases, selectPrProfile, type PrProfileSelection } from '../test-pr-profile'; +import { derivedDependencies } from '../pr-dependencies'; import { detectBaseBranch, getChangedFiles, selectTests, E2E_TOUCHFILES, E2E_TIERS, LLM_JUDGE_TOUCHFILES, GLOBAL_TOUCHFILES } from '../../test/helpers/touchfiles'; import { caseTestNamePattern, fileCaseRegistration, shardCaseId, shardFile, shardTrial } from './paid-cases'; import { DEFAULT_MAX_FILES_PER_SHARD, DEFAULT_SHARD_TIMEOUT_MS, DEFAULT_WITHIN_SHARD_CONCURRENCY, PAID_TIERS, ROOT, isOverlayTestFile, @@ -249,8 +250,10 @@ export function computePaidCaseSelection(options: { : selectTests(selectionFiles, table, GLOBAL_TOUCHFILES, { baseRef, cwd: rootDir }).selected; const selection = { e2e: select(E2E_TOUCHFILES), judges: select(LLM_JUDGE_TOUCHFILES) }; if (options.profile === 'full') return { selection, reason: all ? 'run-all' : 'diff' }; + // The changed files are paths in the checkout under review, which can be newer than the tree this selector runs from. + const readSource = (file: string) => { try { return fs.readFileSync(path.join(rootDir, file), 'utf8'); } catch { return ''; } }; const coverage = selectPrProfile({ selectedE2E: selection.e2e, selectedJudges: selection.judges, changedFiles: effectiveFiles, sourceAliases, - screenPolicy: env.EVALS_SCREEN_POLICY === 'narrow' ? 'narrow' : 'broad' }); + derived: derivedDependencies(PR_PROFILE_MAPS, rootDir), readSource, screenPolicy: env.EVALS_SCREEN_POLICY === 'narrow' ? 'narrow' : 'broad' }); if (coverage.needsFullValidation) { throw new Error(`PR profile requires full validation: ${coverage.missingCoverage.join(', ')}. Use --profile full and the relevant periodic cases.`); } diff --git a/test/gstack-config-defaults.test.ts b/test/gstack-config-defaults.test.ts index 0e7fd4269..57d7bad4b 100644 --- a/test/gstack-config-defaults.test.ts +++ b/test/gstack-config-defaults.test.ts @@ -28,7 +28,7 @@ * depend on receiving "" successfully. * * The reverse direction is pinned too: every key in the DEFAULTS table must - * be read by code (bin/, lib/, scripts/, browse/src/, setup, or a .tmpl), or be listed in + * be read by code (bin/, lib/, scripts/, browse/src/, hosts/, setup, or a .tmpl), or be listed in * PROSE_ONLY_KEYS with a reason. A documented setting no code reads is a * switch that does nothing, which is how a consent choice once went unread. */ @@ -120,7 +120,7 @@ function keysReadInTree(root = ROOT): string[] { */ const PROSE_ONLY_KEYS: Record = {}; -const READER_DIRS = ['bin', 'lib', 'scripts', 'browse/src']; +const READER_DIRS = ['bin', 'lib', 'scripts', 'browse/src', 'hosts']; const READER_FILES = ['setup']; /** @@ -176,7 +176,7 @@ function isReadInCorpus(arm: string, corpus: string): boolean { const re = new RegExp([ `(?:gstack-config|\\$\\{?[A-Z_]*CONF[A-Z_]*\\}?)["']?[ \\t]+(?:get|has)[ \\t]+["']?${k}`, `['"](?:get|has)['"]\\s*,\\s*['"]${k}`, - `(?:readConfigKey|readConfigKeyWithRoot|readGstackConfigYamlKey|configValue|configGet|gstack_read_config_key|gstack_config_select)\\(?[ \\t]*['"\`]?${k}`, + `(?:readConfigKey|readConfigKeyWithRoot|readGstackConfigYamlKey|readSkillSwitch|configValue|configGet|gstack_read_config_key|gstack_config_select)\\(?[ \\t]*['"\`]?${k}`, `\\^${k}${wildcard ? '' : ':'}`, ].join('|')); return re.test(corpus); @@ -226,7 +226,7 @@ describe('gstack-config defaults (gate, free)', () => { if (unread.length > 0) { throw new Error( `DEFAULTS keys with no reader: ${unread.join(', ')}. Fix one of two ways: ` + - `add the code that reads the key (gstack-config get/has in bin/, lib/, scripts/, setup or a .tmpl), ` + + `add the code that reads the key (gstack-config get/has in bin/, lib/, scripts/, hosts/, setup or a .tmpl), ` + `or add it to PROSE_ONLY_KEYS in ${SELF} with the reason it has no reader.`, ); } @@ -250,14 +250,15 @@ describe('gstack-config defaults (gate, free)', () => { "spawnSync(bin, ['get', 'key_three'], { timeout: 2000 })", "readConfigKey('key_four')", "readGstackConfigYamlKey('key_eight')", + "readSkillSwitch('key_nine')", 'grep -E "^key_five_${hash}:" "$F"', ].join('\n')); fs.writeFileSync(path.join(root, 'docs', 'prose.md'), 'gstack-config get key_six\n'); fs.writeFileSync(path.join(root, 'skill.md.tmpl'), 'gstack-config get key_seven\n'); const corpus = readerCorpus(root); - const read = ['key_one', 'key_two', 'key_three', 'key_four', 'key_five_*', 'key_six', 'key_seven', 'key_eight', 'key_one_more'] + const read = ['key_one', 'key_two', 'key_three', 'key_four', 'key_five_*', 'key_six', 'key_seven', 'key_eight', 'key_nine', 'key_one_more'] .filter((k) => isReadInCorpus(k, corpus)); - expect(read).toEqual(['key_one', 'key_two', 'key_three', 'key_four', 'key_five_*', 'key_seven', 'key_eight']); + expect(read).toEqual(['key_one', 'key_two', 'key_three', 'key_four', 'key_five_*', 'key_seven', 'key_eight', 'key_nine']); } finally { fs.rmSync(root, { recursive: true, force: true }); } @@ -320,6 +321,18 @@ describe('design_detector (auto|off, rejecting validator)', () => { }); }); +describe('skill_routing and skill_contracts (on|off, default off)', () => { + test('both default to off, and list and defaults enumerate them', () => { + for (const key of ['skill_routing', 'skill_contracts']) expect(get(key)).toEqual({ out: 'off', code: 0 }); + for (const verb of ['list', 'defaults']) { + const r = spawnSync('bash', [CONFIG_BIN, verb], { encoding: 'utf-8', timeout: 30_000, env: { ...process.env, GSTACK_STATE_ROOT: STATE } }); + expect(r.status).toBe(0); + expect(r.stdout).toMatch(/skill_routing:\s+off/); + expect(r.stdout).toMatch(/skill_contracts:\s+off/); + } + }); +}); + describe('design_detector_install_prompted (true|false, rejecting validator)', () => { const env = { ...process.env, GSTACK_STATE_ROOT: STATE }; test('defaults to false, rejects a typo with the file unchanged, round-trips true/false, and is enumerated', () => { diff --git a/test/pr-dependencies.test.ts b/test/pr-dependencies.test.ts index 95abcfb08..11098c14b 100644 --- a/test/pr-dependencies.test.ts +++ b/test/pr-dependencies.test.ts @@ -304,6 +304,20 @@ describe('deleted files (paths absent from the head tree)', () => { expect(real.referencers('bin/gstack-next-version')).toContain('review/SKILL.md.tmpl'); }); + test('a file the PR adds is read from the checkout under review, not from the selector\'s own tree (an older install)', () => { + const added = 'hosts/claude/hooks/zz-added-hook.ts'; + expect(fs.existsSync(path.join(ROOT, added))).toBe(false); + expect(real.referencers(added)).toContain('setup'); + const checkout = fixtureRepo({ setup: 'SETUP_HOOK=hosts/claude/hooks/zz-added-hook\n', [added]: 'export {};\n' }); + try { + const result = computePaidCaseSelection({ profile: 'pr', rootDir: checkout, env: { EVALS_BASE: 'HEAD' }, changedFiles: [added], packageVersionOnly: true }); + expect(result.coverage!.unknownFileLabels.filter(l => l.file === added)).toEqual([]); + expect(result.reason).not.toContain('deleted but still referenced'); + } finally { + fs.rmSync(checkout, { recursive: true, force: true }); + } + }); + test('a deleted file a full-gate input still references, or a deleted full-gate entry, restores the full gate', () => { expect(real.referencers('bin/gstack-install-registry.sh')).toContain('setup'); const referenced = pick('bin/gstack-install-registry.sh'); diff --git a/test/skill-contract-stop-hook.test.ts b/test/skill-contract-stop-hook.test.ts index 065fd0cec..c5a10b68f 100644 --- a/test/skill-contract-stop-hook.test.ts +++ b/test/skill-contract-stop-hook.test.ts @@ -135,16 +135,20 @@ 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, nothing recorded', () => { + test('Agent readers still running when Stop fires: allowed once and recorded deferred; the same set still running next stop is 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.')]; - const unanswered = [...base, tool('a2', 'Agent', { description: 'Security specialist', prompt: 'review' })]; - for (const lines of [background, unanswered]) { - expect(stop(transcript(lines))).toEqual({ code: 0, stdout: '', stderr: '' }); - } - expect(fs.existsSync(path.join(state, 'skill-contracts'))).toBe(false); + const file = transcript(background); + 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`); + 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'); + const unanswered = [...base, tool('a2', 'Agent', { description: 'Security specialist', prompt: 'review' })]; + expect(stop(transcript(unanswered, 'unanswered.jsonl'), {}, { session_id: 'sess-2' })).toEqual({ code: 0, stdout: '', stderr: '' }); }); test('no contract skill loaded: allowed', () => { diff --git a/test/skill-contract.test.ts b/test/skill-contract.test.ts index 79b6d8960..103c34fb3 100644 --- a/test/skill-contract.test.ts +++ b/test/skill-contract.test.ts @@ -176,7 +176,10 @@ describe('checkContract is a pure function of the transcript', () => { for (const cmd of ['bun test', 'cd repo && npm test', 'FOO=1 timeout 60 pytest -x', './scripts/run-tests.sh', 'node repro.js | tail -5', 'bin/gstack-verify', 'bundle exec rspec', 'uv run pytest', 'poetry run pytest', 'docker compose run app pytest', 'xvfb-run -a npm test', 'bash scripts/test.sh', 'mix test', 'bazel test //...']) expect([cmd, isCheckCommand(cmd)]).toEqual([cmd, true]); - for (const cmd of ['git commit -m wip --no-verify', 'git add test/a.test.ts', 'cat test/a.test.ts', 'git diff --check', 'ls go', 'test -f README.md', './scripts/fetch-latest.sh', 'bash scripts/deploy.sh']) + for (const cmd of ['npm test', 'npm run test:unit', 'yarn jest', 'go test ./...', 'cargo test', 'make check', 'python -m pytest', 'bun repro.ts', 'npx vitest run', 'curl -s http://localhost:3000/api']) + expect([cmd, isCheckCommand(cmd)]).toEqual([cmd, true]); + for (const cmd of ['git commit -m wip --no-verify', 'git add test/a.test.ts', 'cat test/a.test.ts', 'git diff --check', 'ls go', 'test -f README.md', './scripts/fetch-latest.sh', 'bash scripts/deploy.sh', + 'bun install', 'npm install lodash', 'bun add zod', 'go build ./...', 'go mod tidy', 'node --version', 'make clean', 'make', 'npm run build', 'curl --version']) expect([cmd, isCheckCommand(cmd)]).toEqual([cmd, false]); }); From 365dd613d266e8c1ed16154cc59a3ee3f10b8050 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 07:00:18 +0000 Subject: [PATCH 2/4] feat(hooks): one SWITCH_DEFAULT per hook switch, read by setup, the hooks, gstack-config and gstack-skill-route (GST-1088 follow-up) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-config | 5 +- bin/gstack-skill-route | 17 ++- changelog.d/capy-gst-1088-lane-a-followup.md | 2 +- docs/skill-routing-and-contracts.md | 8 +- .../claude/hooks/skill-contract-stop-hook.ts | 2 +- hosts/claude/hooks/skill-route-hook.ts | 2 +- lib/skill-route.ts | 15 ++- setup | 27 ++-- test/setup-skill-hooks-gate.test.ts | 124 ++++++++++++++++-- 9 files changed, 167 insertions(+), 35 deletions(-) diff --git a/bin/gstack-config b/bin/gstack-config index fde1fbe5c..c27f155a9 100755 --- a/bin/gstack-config +++ b/bin/gstack-config @@ -26,6 +26,7 @@ # reports which root received the value and which root still overrides it. # Docs: https://github.com/garrytan/gstack/blob/main/docs/state-root.md set -euo pipefail +GSTACK_BUN_CONFIG=/dev/null; case "${OSTYPE:-}" in msys*|cygwin*) if command -v cygpath >/dev/null 2>&1; then GSTACK_BUN_CONFIG=NUL; fi ;; esac [ -r "$(dirname "$0")/gstack-state-root.sh" ] && . "$(dirname "$0")/gstack-state-root.sh" 2>/dev/null || { echo "$0: cannot resolve the gstack state root: $(dirname "$0")/gstack-state-root.sh is missing. fix: reinstall with ./setup or /gstack-upgrade (docs/state-root.md)" >&2; exit 1; } gstack_state_root_select @@ -344,8 +345,8 @@ lookup_default() { artifacts_sync_removals) echo "off" ;; # on | off — publish removals of synced files gone from disk plan_tune_hooks) echo "prompt" ;; # prompt | yes | no — controls ./setup plan-tune hook install timeline_stop_hook) echo "yes" ;; # yes | no — controls ./setup timeline Stop hook registration (#2677) - skill_routing) echo "off" ;; # on | off — ./setup registers the skill-routing UserPromptSubmit hook (docs/skill-routing-and-contracts.md) - skill_contracts) echo "off" ;; # on | off — ./setup registers the skill-contract Stop hook (docs/skill-routing-and-contracts.md) + skill_routing) bun --no-env-file --config="${GSTACK_BUN_CONFIG:-/dev/null}" "$(dirname "$0")/gstack-skill-route" --default skill_routing ;; # on | off — the skill-routing UserPromptSubmit hook; default: SWITCH_DEFAULT in lib/skill-route.ts (docs/skill-routing-and-contracts.md) + skill_contracts) bun --no-env-file --config="${GSTACK_BUN_CONFIG:-/dev/null}" "$(dirname "$0")/gstack-skill-route" --default skill_contracts ;; # on | off — the skill-contract Stop hook; default: SWITCH_DEFAULT in lib/skill-route.ts (docs/skill-routing-and-contracts.md) redact_repo_visibility) echo "" ;; # empty → fall through to gh/glab detection redact_prepush_hook) echo "false" ;; diff --git a/bin/gstack-skill-route b/bin/gstack-skill-route index 5af371c41..c0273bb4d 100755 --- a/bin/gstack-skill-route +++ b/bin/gstack-skill-route @@ -4,19 +4,30 @@ * * Usage: * gstack-skill-route --explain "" [--json] [--threshold ] + * gstack-skill-route --default * * Prints the switch state, the threshold, every candidate skill with its * score and best trigger, the route the hook would take and the exact line it * would inject. Works with the switch off (a dry run): it never injects - * anything. Exit 0, or 2 on a usage error. Docs: docs/skill-routing-and-contracts.md. + * anything. --default prints a switch's SWITCH_DEFAULT (on or off), the value + * setup and gstack-config use when the switch is unset. Exit 0, or 2 on a usage error. Docs: docs/skill-routing-and-contracts.md. */ import { - ROUTE_THRESHOLD, hintLine, installedSkillsDir, readSkillSwitch, routePrompt, scanInstalledSkills, + ROUTE_THRESHOLD, SWITCH_DEFAULT, hintLine, installedSkillsDir, readSkillSwitch, routePrompt, scanInstalledSkills, } from '../lib/skill-route'; -const USAGE = 'Usage: gstack-skill-route --explain "" [--json] [--threshold ]'; +const USAGE = 'Usage: gstack-skill-route --explain "" [--json] [--threshold ]\n gstack-skill-route --default '; const args = process.argv.slice(2); if (args[0] === '--help' || args[0] === '-h') { process.stdout.write(`${USAGE}\n`); process.exit(0); } +if (args[0] === '--default') { + const name = args[1] ?? ''; + if (!Object.hasOwn(SWITCH_DEFAULT, name)) { + process.stderr.write(`SKILL_ROUTE_USAGE: --default needs skill_routing or skill_contracts\n${USAGE}\n`); + process.exit(2); + } + process.stdout.write(`${SWITCH_DEFAULT[name as keyof typeof SWITCH_DEFAULT]}\n`); + process.exit(0); +} const at = args.indexOf('--explain'); const prompt = at >= 0 ? args[at + 1] : undefined; const tAt = args.indexOf('--threshold'); diff --git a/changelog.d/capy-gst-1088-lane-a-followup.md b/changelog.d/capy-gst-1088-lane-a-followup.md index 04327a872..c42a42658 100644 --- a/changelog.d/capy-gst-1088-lane-a-followup.md +++ b/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. `gstack-config list` and `defaults` show `skill_routing` and `skill_contracts` (both off by default). 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 ` 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. diff --git a/docs/skill-routing-and-contracts.md b/docs/skill-routing-and-contracts.md index ecb34374f..0e7c34493 100644 --- a/docs/skill-routing-and-contracts.md +++ b/docs/skill-routing-and-contracts.md @@ -1,6 +1,6 @@ # Skill routing and skill contracts -Two opt-in Claude Code hooks that help a model actually run gstack's skills (GST-1088). Both are **off by default**, and with both off nothing changes. +Two Claude Code hooks that help a model actually run gstack's skills (GST-1088). Each has one default, `SWITCH_DEFAULT` in `lib/skill-route.ts`; `gstack-config defaults` prints it. With both off nothing changes. - **Skill routing** (`skill_routing`, a UserPromptSubmit hook). When a prompt matches an installed skill's `triggers:`, the hook adds one line of context naming that skill, for example "this request fits /investigate. Run /investigate with the Skill tool before you start". - **Skill contracts** (`skill_contracts`, a Stop hook). A skill can declare the outputs that must exist before it is done. When the model tries to stop without them, the hook blocks once and names what is missing. @@ -12,9 +12,9 @@ Each switch resolves in this order, and the first one set wins: 1. The `./setup` flag: `--skill-routing` / `--no-skill-routing` and `--skill-contracts` / `--no-skill-contracts` (also `--skill-routing=on|off`). A flag is saved to config, so it survives upgrades. 2. The environment: `GSTACK_SKILL_ROUTING` and `GSTACK_SKILL_CONTRACTS` (`on` or `off`). Not saved. 3. Config: `gstack-config set skill_routing on|off` and `gstack-config set skill_contracts on|off`. -4. The default: off. +4. The default: `SWITCH_DEFAULT` in `lib/skill-route.ts`, the only place it is written. Setup, the hooks, `gstack-config get|list|defaults` and `gstack-skill-route --default ` all read it. -`on` also accepts yes, true and 1; `off` accepts no, false and 0. Setup warns about any other value, uses off for that run, and does not save it. +`on` also accepts yes, true and 1; `off` accepts no, false and 0. Setup warns about any other value, uses the default for that run, and does not save it; the hooks also use the default for such a value. Setup registers a hook only when its switch is on, prints one line the first time it registers it, and removes the registration when the switch is off (or with `--no-team`). The hooks also read the switch every time they run, so this takes effect on the next prompt without re-running setup: @@ -73,7 +73,7 @@ Every item has an `id` (lowercase letters, digits, `.`, `_`, `-`) and a `kind`, | kind | Fields | Met when | |---|---|---| | `report_labels` | `heading`, `labels[]` | The last assistant message containing the heading has a `Label: value` line for every label, and no value is empty or a placeholder (`[...]`, `<...>`, TBD, TODO, `...`, or a template like `DONE \| BLOCKED`) | -| `check_after_last_edit` | optional `match` (regular expression) | No Edit, Write, MultiEdit or NotebookEdit happened, or a Bash test or repro command ran after the last one. By default a command counts when, with quoted text removed, one of its parts (split on `&&`, `\|\|`, `;`, `\|`) starts with a test runner, build tool or interpreter (bun, npm, pytest, go, cargo, make, node, python, curl, ...) or a script whose name says test, spec, repro, check, verify or lint, directly or behind a wrapper (`bundle exec rspec`, `uv run pytest`, `docker compose run app pytest`, `xvfb-run npm test`, `bash scripts/test.sh`). `git commit --no-verify`, `cat test/a.test.ts` and the shell's `test -f` do not count | +| `check_after_last_edit` | optional `match` (regular expression) | No Edit, Write, MultiEdit or NotebookEdit happened, or a Bash test or repro command ran after the last one. By default a command counts when, with quoted text removed, one of its parts (split on `&&`, `\|\|`, `;`, `\|`) runs a test runner (pytest, jest, vitest, rspec, tsc, ...), a test or check verb (`npm test`, `go test`, `cargo run`, `make check`, `bun run lint`), code through an interpreter (`node repro.js`, `python -c ...`, `bun scripts/repro.ts`), `curl` against a URL, or a script whose name says test, spec, repro, check, verify or lint, directly or behind a wrapper (`bundle exec rspec`, `uv run pytest`, `docker compose run app pytest`, `xvfb-run npm test`, `bash scripts/test.sh`). Installs, builds, cleans and version probes (`bun install`, `npm install`, `go build`, `make clean`, `node --version`), `git commit --no-verify`, `cat test/a.test.ts` and the shell's `test -f` do not count | | `selected_specialists_dispatched` | optional `marker`, `empty_marker` | The selection line was printed in an assistant message or a Bash result (never a fetched page or file), its names are plain (letters, digits, spaces, `_`, `.`, `-`, at most 40 characters), and each specialist it names has an Agent dispatch that mentions it. The marker is literal text in which a whole-word N stands for a number; the default is `Dispatching N specialists:`. N = 0 (`Dispatching 0 specialists: (small diff)`) or a "specialists skipped" line means zero specialists were selected, which passes. Text in parentheses is not a name (`Dispatching 1 specialists: State Machines (small diff)` selects State Machines) | | `agent_dispatch` | `match` (regular expression, case-insensitive), optional `marker` | The `marker` line (literal text, like /review's `Dispatching adversarial pass: native`) was printed and an Agent dispatch follows it; otherwise, as the fallback, an Agent dispatch's description or prompt matches `match` | diff --git a/hosts/claude/hooks/skill-contract-stop-hook.ts b/hosts/claude/hooks/skill-contract-stop-hook.ts index 125cd6474..752876b1f 100755 --- a/hosts/claude/hooks/skill-contract-stop-hook.ts +++ b/hosts/claude/hooks/skill-contract-stop-hook.ts @@ -3,7 +3,7 @@ * skill-contract-stop-hook — Stop hook that blocks once when a contract skill * ends without its required outputs (GST-1088, lane A). Off unless * skill_contracts is on (env GSTACK_SKILL_CONTRACTS, then config - * skill_contracts; default off). + * skill_contracts, then SWITCH_DEFAULT). * * stdin -> switch on? -> transcript_path -> parse (+ last_assistant_message) * -> the most recently loaded skill that declares a contract (none: allow) diff --git a/hosts/claude/hooks/skill-route-hook.ts b/hosts/claude/hooks/skill-route-hook.ts index 320513d42..61ae7abfd 100755 --- a/hosts/claude/hooks/skill-route-hook.ts +++ b/hosts/claude/hooks/skill-route-hook.ts @@ -2,7 +2,7 @@ /** * skill-route-hook — UserPromptSubmit hook that names the gstack skill a prompt * fits (GST-1088, lane A). Off unless skill_routing is on (env - * GSTACK_SKILL_ROUTING, then config skill_routing; default off). + * GSTACK_SKILL_ROUTING, then config skill_routing, then SWITCH_DEFAULT). * * stdin (<= 256 KiB, 500 ms) -> switch on? -> JSON object with a prompt? * -> not a /command, not over 8000 chars -> score against installed triggers diff --git a/lib/skill-route.ts b/lib/skill-route.ts index 458140b6b..e5f967cec 100644 --- a/lib/skill-route.ts +++ b/lib/skill-route.ts @@ -34,6 +34,12 @@ export const SWITCH_ENV: Record = { skill_contracts: 'GSTACK_SKILL_CONTRACTS', }; +/** Each switch's one default. setup, gstack-config and gstack-skill-route read it through `gstack-skill-route --default `; the hooks read it here. */ +export const SWITCH_DEFAULT: Record = { + skill_routing: 'off', + skill_contracts: 'off', +}; + export function normalizeSwitch(raw: string | null | undefined): 'on' | 'off' | null { const v = String(raw ?? '').trim().toLowerCase(); if (['on', 'yes', 'y', 'true', '1'].includes(v)) return 'on'; @@ -43,18 +49,19 @@ export function normalizeSwitch(raw: string | null | undefined): 'on' | 'off' | export interface SwitchReading { on: boolean; source: string; invalid?: string } -/** env GSTACK_SKILL_ROUTING / GSTACK_SKILL_CONTRACTS > config key > default off. An unrecognized value is off. */ +/** env GSTACK_SKILL_ROUTING / GSTACK_SKILL_CONTRACTS > config key > SWITCH_DEFAULT. An unrecognized value takes the default, as setup does. */ export function readSkillSwitch(name: SwitchName, env: NodeJS.ProcessEnv = process.env): SwitchReading { + const fallback = SWITCH_DEFAULT[name] === 'on'; const envName = SWITCH_ENV[name]; const raw = env[envName]; if (raw !== undefined && raw !== '') { const v = normalizeSwitch(raw); - return v ? { on: v === 'on', source: `env ${envName}` } : { on: false, source: `env ${envName}`, invalid: raw }; + return v ? { on: v === 'on', source: `env ${envName}` } : { on: fallback, source: `env ${envName}`, invalid: raw }; } const cfg = readConfigKey(name, env); - if (cfg === null) return { on: false, source: 'default' }; + if (cfg === null) return { on: fallback, source: 'default' }; const v = normalizeSwitch(cfg); - return v ? { on: v === 'on', source: `config ${name}` } : { on: false, source: `config ${name}`, invalid: cfg }; + return v ? { on: v === 'on', source: `config ${name}` } : { on: fallback, source: `config ${name}`, invalid: cfg }; } /** Where Claude Code reads skills: GSTACK_INSTALLED_SKILLS_DIR, else ${CLAUDE_CONFIG_DIR:-~/.claude}/skills. */ diff --git a/setup b/setup index 470b8cccf..14425f958 100755 --- a/setup +++ b/setup @@ -53,7 +53,7 @@ Options: -q, --quiet Suppress progress output. -h, --help Show this help and exit. -Claude Code hooks (off by default; docs/skill-routing-and-contracts.md): +Claude Code hooks (docs/skill-routing-and-contracts.md): --skill-routing / --no-skill-routing Name the gstack skill a prompt fits, in one line of context (UserPromptSubmit). Saved as skill_routing. @@ -4882,32 +4882,38 @@ if [ "$NO_TEAM_MODE" -ne 1 ] && [ "$TL_DECISION" != "no" ] && [ -x "$SETTINGS_HO fi # ─── Skill routing + contract hooks (GST-1088) ─────────────────────────────── -# Two opt-in Claude Code hooks, both OFF by default (docs/skill-routing-and-contracts.md): +# Two Claude Code hooks (docs/skill-routing-and-contracts.md): # skill_routing — UserPromptSubmit: one line naming the gstack skill a prompt fits. # skill_contracts — Stop: blocks once when a contract skill ends without its outputs. # Gate per hook, mirroring the timeline_stop_hook block above: flag > env # (GSTACK_SKILL_ROUTING / GSTACK_SKILL_CONTRACTS) > saved config (skill_routing / -# skill_contracts) > default off. An explicit flag persists to config; a typo -# warns and is not persisted; off (or --no-team) removes a live registration. +# skill_contracts) > the switch's default, SWITCH_DEFAULT in lib/skill-route.ts, +# read through gstack-skill-route --default (off if that cannot run). An +# explicit flag persists to config; a typo warns, takes the default and is not +# persisted; off (or --no-team) removes a live registration. # The hooks read the same switch at run time, so `gstack-config set off` # takes effect on the next prompt without re-running setup. _skill_hook_gate() { local key="$1" mode="$2" env_name="$3" event="$4" cmd="$5" source="$6" timeout="$7" what="$8" - local decision src out unrecognized=0 + local decision src out def unrecognized=0 + def="$(bun --no-env-file --config="${GSTACK_BUN_CONFIG:-/dev/null}" "$SKILL_ROUTE_BIN" --default "$key" 2>/dev/null || true)" + [ "$def" = "on" ] || def="off" if [ -n "$mode" ]; then decision="$mode"; src="flag" elif [ -n "${!env_name:-}" ]; then decision="${!env_name}"; src="env $env_name" else - decision="$("$GSTACK_CONFIG" get "$key" 2>/dev/null || true)"; src="config $key" + decision="$("$GSTACK_CONFIG" get --stored "$key" 2>/dev/null || true)"; src="config $key" + if [ "$decision" = "unset" ]; then decision=""; src="default"; fi fi decision=$(printf '%s' "$decision" | tr '[:upper:]' '[:lower:]' | tr -d '[:space:]') case "$decision" in - on|yes|y|true|1) decision="on" ;; - off|no|n|false|skip|0|"") decision="off" ;; + on|yes|y|true|1) decision="on" ;; + off|no|n|false|skip|0) decision="off" ;; + "") decision="$def" ;; *) - log " WARNING: unrecognized $key value '$decision' (from $src) — using default 'off' for this run, not persisting" - decision="off"; unrecognized=1 ;; + log " WARNING: unrecognized $key value '$decision' (from $src) — using default '$def' for this run, not persisting" + decision="$def"; unrecognized=1 ;; esac if [ -n "$mode" ] && [ "$unrecognized" -eq 0 ]; then "$GSTACK_CONFIG" set "$key" "$decision" >/dev/null 2>&1 || true @@ -4929,6 +4935,7 @@ _skill_hook_gate() { log " warning: settings hook update failed: $(printf '%s\n' "$out" | head -1) — run $SETTINGS_HOOK manually" fi } +SKILL_ROUTE_BIN="$SOURCE_GSTACK_DIR/bin/gstack-skill-route" SKILL_ROUTE_HOOK="$(_hook_command_path hosts/claude/hooks/skill-route-hook || true)" SKILL_CONTRACT_HOOK="$(_hook_command_path hosts/claude/hooks/skill-contract-stop-hook || true)" _skill_hook_gate skill_routing "$SKILL_ROUTING_MODE" GSTACK_SKILL_ROUTING UserPromptSubmit \ diff --git a/test/setup-skill-hooks-gate.test.ts b/test/setup-skill-hooks-gate.test.ts index 8468c1ce5..d0138f314 100644 --- a/test/setup-skill-hooks-gate.test.ts +++ b/test/setup-skill-hooks-gate.test.ts @@ -5,12 +5,17 @@ * settings.json: default off registers nothing, a flag persists and registers * once with a one-time line, a typo warns and is not persisted, and off * removes the registration. Static pins cover the flags and the parse gate. + * Each switch's default lives only in SWITCH_DEFAULT (lib/skill-route.ts): a + * copy with one table line flipped changes what setup registers, what + * gstack-config reports and what the hook does. */ -import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, test } from 'bun:test'; import { spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; import * as path from 'node:path'; -import { ROOT, tmpDir } from './helpers/skill-contract-fixture'; +import { + J1_TRANSCRIPT, ROOT, hookEnv, runHook, skillsDir, tmpDir, +} from './helpers/skill-contract-fixture'; const setupSrc = fs.readFileSync(path.join(ROOT, 'setup'), 'utf8'); const settingsHookSrc = fs.readFileSync(path.join(ROOT, 'bin', 'gstack-settings-hook'), 'utf8'); @@ -23,26 +28,31 @@ afterEach(() => { for (const d of [state, home]) fs.rmSync(d, { recursive: true, const settingsFile = () => path.join(home, 'settings.json'); const settings = () => (fs.existsSync(settingsFile()) ? JSON.parse(fs.readFileSync(settingsFile(), 'utf8')) : {}); -/** Run the routing gate once: mode is the flag value ("" = none). */ -function gate(mode: string, env: Record = {}, noTeam = 0): { code: number | null; out: string } { - const hook = path.join(ROOT, 'hosts', 'claude', 'hooks', 'skill-route-hook'); +const GATES = { + skill_routing: 'GSTACK_SKILL_ROUTING UserPromptSubmit "$2/hosts/claude/hooks/skill-route-hook" gstack-skill-route 5 "routing hint"', + skill_contracts: 'GSTACK_SKILL_CONTRACTS Stop "$2/hosts/claude/hooks/skill-contract-stop-hook" gstack-skill-contract 10 "contract check"', +}; + +/** Run one gate once from the checkout at root: mode is the flag value ("" = none). */ +function gate(mode: string, env: Record = {}, noTeam = 0, root = ROOT, key: keyof typeof GATES = 'skill_routing'): { code: number | null; out: string } { const script = [ 'set -e', 'log() { printf "%s\\n" "$*"; }', - `GSTACK_CONFIG="${path.join(ROOT, 'bin', 'gstack-config')}"`, + 'GSTACK_CONFIG="$2/bin/gstack-config"', `SETTINGS_HOOK="${path.join(ROOT, 'bin', 'gstack-settings-hook')}"`, + 'SKILL_ROUTE_BIN="$2/bin/gstack-skill-route"', `IS_WINDOWS=0; NO_TEAM_MODE=${noTeam}`, gateFn, - `_skill_hook_gate skill_routing "$1" GSTACK_SKILL_ROUTING UserPromptSubmit "${hook}" gstack-skill-route 5 "routing hint"`, + `_skill_hook_gate ${key} "$1" ${GATES[key]}`, ].join('\n'); - const r = spawnSync('bash', ['-c', script, 'gate', mode], { + const r = spawnSync('bash', ['-c', script, 'gate', mode, root], { env: { PATH: process.env.PATH ?? '', HOME: home, GSTACK_STATE_ROOT: state, GSTACK_SETTINGS_FILE: settingsFile(), ...env }, encoding: 'utf8', timeout: 30_000, }); return { code: r.status, out: (r.stdout ?? '') + (r.stderr ?? '') }; } const config = () => (fs.existsSync(path.join(state, 'config.yaml')) ? fs.readFileSync(path.join(state, 'config.yaml'), 'utf8') : ''); -const registered = () => (settings().hooks?.UserPromptSubmit ?? []) as Array<{ hooks: Array<{ type: string; command: string; timeout?: number }> }>; +const registered = (event = 'UserPromptSubmit') => (settings().hooks?.[event] ?? []) as Array<{ hooks: Array<{ type: string; command: string; timeout?: number }> }>; describe('setup: skill hook gates', () => { test('flags parse for both switches, and both appear in --help', () => { @@ -102,3 +112,99 @@ describe('setup: skill hook gates', () => { expect(registered()).toEqual([]); }); }); + +describe('SWITCH_DEFAULT is the one default (a flip PR changes one line)', () => { + const TABLE = path.join('lib', 'skill-route.ts'); + const original = fs.readFileSync(path.join(ROOT, TABLE), 'utf8'); + let copy = '', skills = '', work = ''; + beforeAll(() => { + copy = tmpDir('gstack-switch-default-'); + fs.cpSync(path.join(ROOT, 'lib'), path.join(copy, 'lib'), { recursive: true }); + fs.cpSync(path.join(ROOT, 'hosts', 'claude', 'hooks'), path.join(copy, 'hosts', 'claude', 'hooks'), { recursive: true }); + for (const f of ['gstack-config', 'gstack-state-root.sh', 'gstack-skill-route']) fs.cpSync(path.join(ROOT, 'bin', f), path.join(copy, 'bin', f)); + fs.symlinkSync(path.join(ROOT, 'node_modules'), path.join(copy, 'node_modules')); + skills = skillsDir(['investigate']); + work = tmpDir('gstack-switch-default-work-'); + }); + afterAll(() => { for (const d of [copy, skills, work]) fs.rmSync(d, { recursive: true, force: true }); }); + + /** Write the copy's table with only this switch's line flipped to on, and prove the diff is that one line. */ + function flip(key: keyof typeof GATES): void { + const line = ` ${key}: 'off',`; + expect(original.split('\n').filter(l => l === line)).toHaveLength(1); + const flipped = original.replace(line, ` ${key}: 'on',`); + const changed = flipped.split('\n').filter((l, i) => l !== original.split('\n')[i]); + expect(changed).toEqual([` ${key}: 'on',`]); + fs.writeFileSync(path.join(copy, TABLE), flipped); + } + const configAt = (root: string, ...args: string[]) => spawnSync(path.join(root, 'bin', 'gstack-config'), args, { + env: { PATH: process.env.PATH ?? '', HOME: home, GSTACK_STATE_ROOT: state }, encoding: 'utf8', timeout: 30_000, + }); + const routeAt = (root: string, env: Record = {}) => runHook(path.join(root, 'hosts', 'claude', 'hooks', 'skill-route-hook.ts'), + JSON.stringify({ session_id: 'sess-d', transcript_path: '/tmp/t.jsonl', cwd: work, hook_event_name: 'UserPromptSubmit', prompt: 'please fix this bug in login' }), + hookEnv(state, skills, env)); + const stopAt = (root: string) => { + const file = path.join(work, 'transcript.jsonl'); + fs.writeFileSync(file, `${J1_TRANSCRIPT.join('\n')}\n`); + return runHook(path.join(root, 'hosts', 'claude', 'hooks', 'skill-contract-stop-hook.ts'), + JSON.stringify({ session_id: 'sess-d', transcript_path: file, cwd: work, hook_event_name: 'Stop', stop_hook_active: false }), + hookEnv(state, skills)); + }; + + test('both defaults are off today, and every reader agrees', () => { + expect(original).toContain("export const SWITCH_DEFAULT: Record = {\n skill_routing: 'off',\n skill_contracts: 'off',\n};"); + for (const key of ['skill_routing', 'skill_contracts']) { + const get = configAt(ROOT, 'get', key); + expect({ code: get.status, out: get.stdout }).toEqual({ code: 0, out: 'off' }); + expect(configAt(ROOT, 'get', '--stored', key).stdout).toBe('unset'); + } + expect(configAt(ROOT, 'defaults').stdout).toMatch(/^ {2}skill_routing: +off$/m); + expect(gate('').out).toBe(''); + expect(routeAt(ROOT)).toEqual({ code: 0, stdout: '', stderr: '' }); + expect(stopAt(ROOT)).toEqual({ code: 0, stdout: '', stderr: '' }); + }); + + test('an absent key reads empty from gstack-config get, never "false"', () => { + const r = configAt(ROOT, 'get', 'no_such_key_gst1088'); + expect({ code: r.status, out: r.stdout }).toEqual({ code: 1, out: '' }); + }); + + test('flipping skill_routing alone: setup registers, gstack-config reports on, the hook routes; config off still wins', () => { + flip('skill_routing'); + expect(configAt(copy, 'get', 'skill_routing').stdout).toBe('on'); + expect(configAt(copy, 'get', 'skill_contracts').stdout).toBe('off'); + expect(configAt(copy, 'defaults').stdout).toMatch(/^ {2}skill_routing: +on$/m); + const g = gate('', {}, 0, copy); + expect(g.out).toContain('registered UserPromptSubmit hook: routing hint (on via default;'); + expect(registered()).toHaveLength(1); + expect(config()).not.toContain('skill_routing'); + const hint = routeAt(copy); + expect(hint.code).toBe(0); + expect(JSON.parse(hint.stdout).hookSpecificOutput.additionalContext).toContain('/investigate'); + expect(stopAt(copy).stdout).toBe(''); + + expect(configAt(copy, 'set', 'skill_routing', 'off').status).toBe(0); + gate('', {}, 0, copy); + expect(registered()).toEqual([]); + expect(routeAt(copy).stdout).toBe(''); + }); + + test('flipping skill_contracts alone: setup registers the Stop hook and the hook blocks', () => { + flip('skill_contracts'); + const g = gate('', {}, 0, copy, 'skill_contracts'); + expect(g.out).toContain('registered Stop hook: contract check (on via default;'); + expect(registered('Stop')).toHaveLength(1); + expect(registered()).toEqual([]); + expect(JSON.parse(stopAt(copy).stdout).decision).toBe('block'); + expect(routeAt(copy).stdout).toBe(''); + }); + + test('a typo takes the flipped default in setup and in the hook', () => { + flip('skill_routing'); + const typo = gate('onn', {}, 0, copy); + expect(typo.out).toContain("WARNING: unrecognized skill_routing value 'onn' (from flag) — using default 'on' for this run, not persisting"); + expect(registered()).toHaveLength(1); + expect(JSON.parse(routeAt(copy, { GSTACK_SKILL_ROUTING: 'onn' }).stdout).hookSpecificOutput.additionalContext).toContain('/investigate'); + expect(routeAt(ROOT, { GSTACK_SKILL_ROUTING: 'onn' }).stdout).toBe(''); + }); +}); From 7595189caf0d40042e952de827396d7e08c31c8e Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 07:12:44 +0000 Subject: [PATCH 3/4] 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 | 2 +- docs/skill-routing-and-contracts.md | 4 +- .../claude/hooks/skill-contract-stop-hook.ts | 39 ++++++++------- lib/skill-contract.ts | 48 +++++++++++++------ test/skill-contract-stop-hook.test.ts | 21 ++++++-- test/skill-contract.test.ts | 23 ++++++++- 6 files changed, 98 insertions(+), 39 deletions(-) diff --git a/changelog.d/capy-gst-1088-lane-a-followup.md b/changelog.d/capy-gst-1088-lane-a-followup.md index c42a42658..a3b7ba3ab 100644 --- a/changelog.d/capy-gst-1088-lane-a-followup.md +++ b/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 ` 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 ` 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. diff --git a/docs/skill-routing-and-contracts.md b/docs/skill-routing-and-contracts.md index 0e7c34493..89960dbf3 100644 --- a/docs/skill-routing-and-contracts.md +++ b/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/.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/.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 diff --git a/hosts/claude/hooks/skill-contract-stop-hook.ts b/hosts/claude/hooks/skill-contract-stop-hook.ts index 752876b1f..27d95111b 100755 --- a/hosts/claude/hooks/skill-contract-stop-hook.ts +++ b/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 { 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(',') || '-'; diff --git a/lib/skill-contract.ts b/lib/skill-contract.ts index 301eb9f54..23eb49d97 100644 --- a/lib/skill-contract.ts +++ b/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 } 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 }; })); } diff --git a/test/skill-contract-stop-hook.test.ts b/test/skill-contract-stop-hook.test.ts index c5a10b68f..0045d35cd 100644 --- a/test/skill-contract-stop-hook.test.ts +++ b/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('bash b9 completed')}\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: '' }); diff --git a/test/skill-contract.test.ts b/test/skill-contract.test.ts index 103c34fb3..7cfbfe86f 100644 --- a/test/skill-contract.test.ts +++ b/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'); From 457195be85e485cc526227f562f88ad1376903df Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 08:50:22 +0000 Subject: [PATCH 4/4] fix(hooks): tests read SWITCH_DEFAULT instead of assuming off; an unreadable config turns both hooks off whatever the default (GST-1088 follow-up) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/capy-gst-1088-lane-a-followup.md | 2 +- docs/skill-routing-and-contracts.md | 2 +- lib/skill-route.ts | 18 ++- setup | 4 + test/gstack-config-defaults.test.ts | 11 +- test/setup-hook-syntax-gate.test.ts | 2 + test/setup-skill-hooks-gate.test.ts | 129 +++++++++++-------- test/skill-contract-stop-hook.test.ts | 9 +- test/skill-route.test.ts | 29 +++-- 9 files changed, 133 insertions(+), 73 deletions(-) diff --git a/changelog.d/capy-gst-1088-lane-a-followup.md b/changelog.d/capy-gst-1088-lane-a-followup.md index a3b7ba3ab..b95e87c27 100644 --- a/changelog.d/capy-gst-1088-lane-a-followup.md +++ b/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 ` 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. +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 ` all read, so changing a default is a one-line edit; `gstack-config list` and `defaults` now show both switches, an unrecognized value takes the default in the hooks as it already did in setup, and a config file that exists but cannot be read turns both hooks off in setup and in the hooks whatever the default. 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. diff --git a/docs/skill-routing-and-contracts.md b/docs/skill-routing-and-contracts.md index 89960dbf3..026f23a73 100644 --- a/docs/skill-routing-and-contracts.md +++ b/docs/skill-routing-and-contracts.md @@ -14,7 +14,7 @@ Each switch resolves in this order, and the first one set wins: 3. Config: `gstack-config set skill_routing on|off` and `gstack-config set skill_contracts on|off`. 4. The default: `SWITCH_DEFAULT` in `lib/skill-route.ts`, the only place it is written. Setup, the hooks, `gstack-config get|list|defaults` and `gstack-skill-route --default ` all read it. -`on` also accepts yes, true and 1; `off` accepts no, false and 0. Setup warns about any other value, uses the default for that run, and does not save it; the hooks also use the default for such a value. +`on` also accepts yes, true and 1; `off` accepts no, false and 0. Setup warns about any other value, uses the default for that run, and does not save it; the hooks also use the default for such a value. A config file that exists but cannot be read turns both hooks off in setup and in the hooks, whatever the default, because it may hold an `off` you set. Setup registers a hook only when its switch is on, prints one line the first time it registers it, and removes the registration when the switch is off (or with `--no-team`). The hooks also read the switch every time they run, so this takes effect on the next prompt without re-running setup: diff --git a/lib/skill-route.ts b/lib/skill-route.ts index e5f967cec..69ad89c43 100644 --- a/lib/skill-route.ts +++ b/lib/skill-route.ts @@ -49,7 +49,21 @@ export function normalizeSwitch(raw: string | null | undefined): 'on' | 'off' | export interface SwitchReading { on: boolean; source: string; invalid?: string } -/** env GSTACK_SKILL_ROUTING / GSTACK_SKILL_CONTRACTS > config key > SWITCH_DEFAULT. An unrecognized value takes the default, as setup does. */ +/** A config.yaml that exists but cannot be read as a file: the person's choice is unknown. */ +function configUnreadable(env: NodeJS.ProcessEnv): boolean { + const file = path.join(resolveStateRoot(env), 'config.yaml'); + try { + fs.accessSync(file, fs.constants.R_OK); + return !fs.statSync(file).isFile(); + } catch (e) { + return (e as NodeJS.ErrnoException)?.code !== 'ENOENT'; + } +} + +/** + * env GSTACK_SKILL_ROUTING / GSTACK_SKILL_CONTRACTS > config key > SWITCH_DEFAULT. An unrecognized value takes the + * default, as setup does. An unreadable config is off whatever the default: it may hold an off the person set. + */ export function readSkillSwitch(name: SwitchName, env: NodeJS.ProcessEnv = process.env): SwitchReading { const fallback = SWITCH_DEFAULT[name] === 'on'; const envName = SWITCH_ENV[name]; @@ -59,7 +73,7 @@ export function readSkillSwitch(name: SwitchName, env: NodeJS.ProcessEnv = proce return v ? { on: v === 'on', source: `env ${envName}` } : { on: fallback, source: `env ${envName}`, invalid: raw }; } const cfg = readConfigKey(name, env); - if (cfg === null) return { on: fallback, source: 'default' }; + if (cfg === null) return configUnreadable(env) ? { on: false, source: 'config unreadable' } : { on: fallback, source: 'default' }; const v = normalizeSwitch(cfg); return v ? { on: v === 'on', source: `config ${name}` } : { on: fallback, source: `config ${name}`, invalid: cfg }; } diff --git a/setup b/setup index 14425f958..0fa1026c2 100755 --- a/setup +++ b/setup @@ -4905,6 +4905,10 @@ _skill_hook_gate() { else decision="$("$GSTACK_CONFIG" get --stored "$key" 2>/dev/null || true)"; src="config $key" if [ "$decision" = "unset" ]; then decision=""; src="default"; fi + if [ "$decision" = "unreadable" ]; then + log " WARNING: cannot read $key from config — using 'off' for this run whatever the default; fix config.yaml, then re-run setup" + decision="off" + fi fi decision=$(printf '%s' "$decision" | tr '[:upper:]' '[:lower:]' | tr -d '[:space:]') case "$decision" in diff --git a/test/gstack-config-defaults.test.ts b/test/gstack-config-defaults.test.ts index 57d7bad4b..172b532bd 100644 --- a/test/gstack-config-defaults.test.ts +++ b/test/gstack-config-defaults.test.ts @@ -38,6 +38,7 @@ import { spawnSync } from 'child_process'; import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; +import { SWITCH_DEFAULT } from '../lib/skill-route'; const ROOT = path.resolve(import.meta.dir, '..'); const CONFIG_BIN = path.join(ROOT, 'bin', 'gstack-config'); @@ -321,14 +322,14 @@ describe('design_detector (auto|off, rejecting validator)', () => { }); }); -describe('skill_routing and skill_contracts (on|off, default off)', () => { - test('both default to off, and list and defaults enumerate them', () => { - for (const key of ['skill_routing', 'skill_contracts']) expect(get(key)).toEqual({ out: 'off', code: 0 }); +describe('skill_routing and skill_contracts (on|off, default SWITCH_DEFAULT)', () => { + test('get, list and defaults report SWITCH_DEFAULT for both', () => { + for (const key of ['skill_routing', 'skill_contracts'] as const) expect(get(key)).toEqual({ out: SWITCH_DEFAULT[key], code: 0 }); for (const verb of ['list', 'defaults']) { const r = spawnSync('bash', [CONFIG_BIN, verb], { encoding: 'utf-8', timeout: 30_000, env: { ...process.env, GSTACK_STATE_ROOT: STATE } }); expect(r.status).toBe(0); - expect(r.stdout).toMatch(/skill_routing:\s+off/); - expect(r.stdout).toMatch(/skill_contracts:\s+off/); + expect(r.stdout).toMatch(new RegExp(`skill_routing:\\s+${SWITCH_DEFAULT.skill_routing}`)); + expect(r.stdout).toMatch(new RegExp(`skill_contracts:\\s+${SWITCH_DEFAULT.skill_contracts}`)); } }); }); diff --git a/test/setup-hook-syntax-gate.test.ts b/test/setup-hook-syntax-gate.test.ts index c86c7126a..4db29ae41 100644 --- a/test/setup-hook-syntax-gate.test.ts +++ b/test/setup-hook-syntax-gate.test.ts @@ -293,6 +293,8 @@ describe('setup: registers the hooks that parse, refuses the rest, exits non-zer GSTACK_STATE_ROOT: path.join(fx.home, '.gstack'), GSTACK_SETTINGS_FILE: fx.settings, GSTACK_PLAN_TUNE_HOOKS: 'yes', + GSTACK_SKILL_ROUTING: 'off', + GSTACK_SKILL_CONTRACTS: 'off', TMPDIR: fx.base, }; // A file, not `bash -c`: the script is longer than Windows' 32,767-character command line. diff --git a/test/setup-skill-hooks-gate.test.ts b/test/setup-skill-hooks-gate.test.ts index d0138f314..3af9dd5e1 100644 --- a/test/setup-skill-hooks-gate.test.ts +++ b/test/setup-skill-hooks-gate.test.ts @@ -13,6 +13,7 @@ import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, test } fr import { spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; import * as path from 'node:path'; +import { SWITCH_DEFAULT } from '../lib/skill-route'; import { J1_TRANSCRIPT, ROOT, hookEnv, runHook, skillsDir, tmpDir, } from './helpers/skill-contract-fixture'; @@ -72,12 +73,28 @@ describe('setup: skill hook gates', () => { expect(setupSrc).toMatch(/_skill_hook_gate skill_contracts "\$SKILL_CONTRACTS_MODE" GSTACK_SKILL_CONTRACTS Stop \\\n "\$SKILL_CONTRACT_HOOK" gstack-skill-contract 10/); }); - test('default off: nothing registered, nothing persisted', () => { - expect(gate('')).toEqual({ code: 0, out: '' }); - expect(registered()).toEqual([]); + test('no flag, env or config: SWITCH_DEFAULT decides, and nothing is persisted', () => { + const g = gate(''); + expect(g.code).toBe(0); + if (SWITCH_DEFAULT.skill_routing === 'on') { + expect(g.out).toContain('registered UserPromptSubmit hook: routing hint (on via default;'); + expect(registered()).toHaveLength(1); + } else { + expect(g.out).toBe(''); + expect(registered()).toEqual([]); + } expect(config()).not.toContain('skill_routing'); }); + test('an unreadable config: warns, registers nothing and removes a live registration, whatever the default', () => { + gate('on'); + fs.rmSync(path.join(state, 'config.yaml')); + fs.mkdirSync(path.join(state, 'config.yaml')); + const g = gate(''); + expect(g.out).toContain("WARNING: cannot read skill_routing from config — using 'off' for this run whatever the default"); + expect(registered()).toEqual([]); + }); + test('flag on: persists, registers a schema-valid entry with a one-time line; a re-run is quiet', () => { const first = gate('on'); expect(first.code).toBe(0); @@ -92,13 +109,13 @@ describe('setup: skill hook gates', () => { expect(registered()).toHaveLength(1); }); - test('env on registers without persisting; a typo warns, applies off and is not persisted', () => { + test('env on registers without persisting; a typo warns, takes SWITCH_DEFAULT and is not persisted', () => { expect(gate('', { GSTACK_SKILL_ROUTING: 'yes' }).out).toContain('on via env GSTACK_SKILL_ROUTING'); expect(config()).not.toContain('skill_routing'); const typo = gate('onn'); - expect(typo.out).toContain("WARNING: unrecognized skill_routing value 'onn' (from flag) — using default 'off' for this run, not persisting"); + expect(typo.out).toContain(`WARNING: unrecognized skill_routing value 'onn' (from flag) — using default '${SWITCH_DEFAULT.skill_routing}' for this run, not persisting`); expect(config()).not.toContain('skill_routing'); - expect(registered()).toEqual([]); + expect(registered()).toHaveLength(SWITCH_DEFAULT.skill_routing === 'on' ? 1 : 0); }); test('off removes a live registration, and so does --no-team', () => { @@ -116,6 +133,7 @@ describe('setup: skill hook gates', () => { describe('SWITCH_DEFAULT is the one default (a flip PR changes one line)', () => { const TABLE = path.join('lib', 'skill-route.ts'); const original = fs.readFileSync(path.join(ROOT, TABLE), 'utf8'); + const other = (v: 'on' | 'off') => (v === 'on' ? 'off' : 'on'); let copy = '', skills = '', work = ''; beforeAll(() => { copy = tmpDir('gstack-switch-default-'); @@ -128,14 +146,15 @@ describe('SWITCH_DEFAULT is the one default (a flip PR changes one line)', () => }); afterAll(() => { for (const d of [copy, skills, work]) fs.rmSync(d, { recursive: true, force: true }); }); - /** Write the copy's table with only this switch's line flipped to on, and prove the diff is that one line. */ - function flip(key: keyof typeof GATES): void { - const line = ` ${key}: 'off',`; + /** Write the copy's table with only this switch's line flipped, prove the diff is that one line, and return the new default. */ + function flip(key: keyof typeof GATES): 'on' | 'off' { + const line = ` ${key}: '${SWITCH_DEFAULT[key]}',`; + const next = other(SWITCH_DEFAULT[key]); expect(original.split('\n').filter(l => l === line)).toHaveLength(1); - const flipped = original.replace(line, ` ${key}: 'on',`); - const changed = flipped.split('\n').filter((l, i) => l !== original.split('\n')[i]); - expect(changed).toEqual([` ${key}: 'on',`]); + const flipped = original.replace(line, ` ${key}: '${next}',`); + expect(flipped.split('\n').filter((l, i) => l !== original.split('\n')[i])).toEqual([` ${key}: '${next}',`]); fs.writeFileSync(path.join(copy, TABLE), flipped); + return next; } const configAt = (root: string, ...args: string[]) => spawnSync(path.join(root, 'bin', 'gstack-config'), args, { env: { PATH: process.env.PATH ?? '', HOME: home, GSTACK_STATE_ROOT: state }, encoding: 'utf8', timeout: 30_000, @@ -146,22 +165,42 @@ describe('SWITCH_DEFAULT is the one default (a flip PR changes one line)', () => const stopAt = (root: string) => { const file = path.join(work, 'transcript.jsonl'); fs.writeFileSync(file, `${J1_TRANSCRIPT.join('\n')}\n`); + fs.rmSync(path.join(state, 'skill-contracts'), { recursive: true, force: true }); return runHook(path.join(root, 'hosts', 'claude', 'hooks', 'skill-contract-stop-hook.ts'), JSON.stringify({ session_id: 'sess-d', transcript_path: file, cwd: work, hook_event_name: 'Stop', stop_hook_active: false }), hookEnv(state, skills)); }; - - test('both defaults are off today, and every reader agrees', () => { - expect(original).toContain("export const SWITCH_DEFAULT: Record = {\n skill_routing: 'off',\n skill_contracts: 'off',\n};"); - for (const key of ['skill_routing', 'skill_contracts']) { - const get = configAt(ROOT, 'get', key); - expect({ code: get.status, out: get.stdout }).toEqual({ code: 0, out: 'off' }); - expect(configAt(ROOT, 'get', '--stored', key).stdout).toBe('unset'); + const routes = (r: { stdout: string }) => r.stdout !== '' && JSON.parse(r.stdout).hookSpecificOutput.additionalContext.includes('/investigate'); + const blocks = (r: { stdout: string }) => r.stdout !== '' && JSON.parse(r.stdout).decision === 'block'; + /** Every reader at root agrees that the switches default to want. */ + function expectReaders(root: string, want: Record): void { + for (const key of ['skill_routing', 'skill_contracts'] as const) { + const get = configAt(root, 'get', key); + expect({ key, code: get.status, out: get.stdout }).toEqual({ key, code: 0, out: want[key] }); + expect(configAt(root, 'get', '--stored', key).stdout).toBe('unset'); + expect(configAt(root, 'defaults').stdout).toMatch(new RegExp(`^ {2}${key}: +${want[key]}$`, 'm')); + const g = gate('', {}, 0, root, key); + expect(registered(key === 'skill_routing' ? 'UserPromptSubmit' : 'Stop')).toHaveLength(want[key] === 'on' ? 1 : 0); + if (want[key] === 'on') expect(g.out).toContain('(on via default;'); + else expect(g.out).toBe(''); } - expect(configAt(ROOT, 'defaults').stdout).toMatch(/^ {2}skill_routing: +off$/m); - expect(gate('').out).toBe(''); - expect(routeAt(ROOT)).toEqual({ code: 0, stdout: '', stderr: '' }); - expect(stopAt(ROOT)).toEqual({ code: 0, stdout: '', stderr: '' }); + expect(routes(routeAt(root))).toBe(want.skill_routing === 'on'); + expect(blocks(stopAt(root))).toBe(want.skill_contracts === 'on'); + expect(config()).toBe(''); + } + + test('pin: skill_routing defaults to off today (a flip PR changes lib/skill-route.ts line 39 and this pin)', () => { + expect(original.split('\n')[38]).toBe(" skill_routing: 'off',"); + expect(SWITCH_DEFAULT.skill_routing).toBe('off'); + }); + + test('pin: skill_contracts defaults to off today (a flip PR changes lib/skill-route.ts line 40 and this pin)', () => { + expect(original.split('\n')[39]).toBe(" skill_contracts: 'off',"); + expect(SWITCH_DEFAULT.skill_contracts).toBe('off'); + }); + + test('every reader agrees with SWITCH_DEFAULT: gstack-config, setup, the routing hook and the Stop hook', () => { + expectReaders(ROOT, SWITCH_DEFAULT); }); test('an absent key reads empty from gstack-config get, never "false"', () => { @@ -169,42 +208,26 @@ describe('SWITCH_DEFAULT is the one default (a flip PR changes one line)', () => expect({ code: r.status, out: r.stdout }).toEqual({ code: 1, out: '' }); }); - test('flipping skill_routing alone: setup registers, gstack-config reports on, the hook routes; config off still wins', () => { - flip('skill_routing'); - expect(configAt(copy, 'get', 'skill_routing').stdout).toBe('on'); - expect(configAt(copy, 'get', 'skill_contracts').stdout).toBe('off'); - expect(configAt(copy, 'defaults').stdout).toMatch(/^ {2}skill_routing: +on$/m); - const g = gate('', {}, 0, copy); - expect(g.out).toContain('registered UserPromptSubmit hook: routing hint (on via default;'); - expect(registered()).toHaveLength(1); - expect(config()).not.toContain('skill_routing'); - const hint = routeAt(copy); - expect(hint.code).toBe(0); - expect(JSON.parse(hint.stdout).hookSpecificOutput.additionalContext).toContain('/investigate'); - expect(stopAt(copy).stdout).toBe(''); - - expect(configAt(copy, 'set', 'skill_routing', 'off').status).toBe(0); + test('flipping skill_routing alone moves every reader; an explicit config value still wins', () => { + const next = flip('skill_routing'); + expectReaders(copy, { ...SWITCH_DEFAULT, skill_routing: next }); + expect(configAt(copy, 'set', 'skill_routing', SWITCH_DEFAULT.skill_routing).status).toBe(0); gate('', {}, 0, copy); - expect(registered()).toEqual([]); - expect(routeAt(copy).stdout).toBe(''); + expect(registered()).toHaveLength(SWITCH_DEFAULT.skill_routing === 'on' ? 1 : 0); + expect(routes(routeAt(copy))).toBe(SWITCH_DEFAULT.skill_routing === 'on'); }); - test('flipping skill_contracts alone: setup registers the Stop hook and the hook blocks', () => { - flip('skill_contracts'); - const g = gate('', {}, 0, copy, 'skill_contracts'); - expect(g.out).toContain('registered Stop hook: contract check (on via default;'); - expect(registered('Stop')).toHaveLength(1); - expect(registered()).toEqual([]); - expect(JSON.parse(stopAt(copy).stdout).decision).toBe('block'); - expect(routeAt(copy).stdout).toBe(''); + test('flipping skill_contracts alone moves every reader', () => { + const next = flip('skill_contracts'); + expectReaders(copy, { ...SWITCH_DEFAULT, skill_contracts: next }); }); test('a typo takes the flipped default in setup and in the hook', () => { - flip('skill_routing'); + const next = flip('skill_routing'); const typo = gate('onn', {}, 0, copy); - expect(typo.out).toContain("WARNING: unrecognized skill_routing value 'onn' (from flag) — using default 'on' for this run, not persisting"); - expect(registered()).toHaveLength(1); - expect(JSON.parse(routeAt(copy, { GSTACK_SKILL_ROUTING: 'onn' }).stdout).hookSpecificOutput.additionalContext).toContain('/investigate'); - expect(routeAt(ROOT, { GSTACK_SKILL_ROUTING: 'onn' }).stdout).toBe(''); + expect(typo.out).toContain(`WARNING: unrecognized skill_routing value 'onn' (from flag) — using default '${next}' for this run, not persisting`); + expect(registered()).toHaveLength(next === 'on' ? 1 : 0); + expect(routes(routeAt(copy, { GSTACK_SKILL_ROUTING: 'onn' }))).toBe(next === 'on'); + expect(routes(routeAt(ROOT, { GSTACK_SKILL_ROUTING: 'onn' }))).toBe(SWITCH_DEFAULT.skill_routing === 'on'); }); }); diff --git a/test/skill-contract-stop-hook.test.ts b/test/skill-contract-stop-hook.test.ts index 0045d35cd..1da9b72f3 100644 --- a/test/skill-contract-stop-hook.test.ts +++ b/test/skill-contract-stop-hook.test.ts @@ -8,6 +8,7 @@ import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; import * as fs from 'node:fs'; import * as path from 'node:path'; import { AGENT_STALL_MS, STATE_ERROR_DOC, stateErrorLine } from '../lib/skill-contract'; +import { SWITCH_DEFAULT } from '../lib/skill-route'; import { CONTRACT_HOOK, DEBUG_REPORT, INTERACTIVE_TURNS, J1_TRANSCRIPT, ROOT, hookEnv, meta, result, runHook, say, skillsDir, slash, tmpDir, tool, user, } from './helpers/skill-contract-fixture'; @@ -218,9 +219,15 @@ describe('fails open on state errors with one stderr line', () => { }); describe('off switch', () => { - test('default off, config off, and env off over config on: no output at all', () => { + test('with nothing set SWITCH_DEFAULT decides; config off, env off over config on, and an unreadable config: no output at all', () => { const file = transcript(J1_TRANSCRIPT); + fs.mkdirSync(path.join(state, 'config.yaml')); expect(runHook(CONTRACT_HOOK, payload(file), hookEnv(state, skills))).toEqual({ code: 0, stdout: '', stderr: '' }); + fs.rmSync(path.join(state, 'config.yaml'), { recursive: true }); + const unset = runHook(CONTRACT_HOOK, payload(file), hookEnv(state, skills)); + if (SWITCH_DEFAULT.skill_contracts === 'on') expect(JSON.parse(unset.stdout).decision).toBe('block'); + else expect(unset).toEqual({ code: 0, stdout: '', stderr: '' }); + fs.rmSync(path.join(state, 'skill-contracts'), { recursive: true, force: true }); fs.writeFileSync(path.join(state, 'config.yaml'), 'skill_contracts: off\n'); expect(runHook(CONTRACT_HOOK, payload(file), hookEnv(state, skills))).toEqual({ code: 0, stdout: '', stderr: '' }); fs.writeFileSync(path.join(state, 'config.yaml'), 'skill_contracts: on\n'); diff --git a/test/skill-route.test.ts b/test/skill-route.test.ts index 246032c21..1d0e00e12 100644 --- a/test/skill-route.test.ts +++ b/test/skill-route.test.ts @@ -2,13 +2,15 @@ * lib/skill-route.ts, the skill-route hook and gstack-skill-route --explain * (GST-1088, lane A): deterministic scoring against installed triggers, one * additionalContext line when it fires, nothing at all (exit 0, no stderr) on - * every error, and off by default. + * every error. Nothing here assumes a default: tests set the switch, or read + * the expected result from SWITCH_DEFAULT (the pins live in + * test/setup-skill-hooks-gate.test.ts). */ import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; import { spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; import * as path from 'node:path'; -import { PROMPT_MAX_CHARS, ROUTED_SKILLS, ROUTE_THRESHOLD, firstStep, hintLine, readSkillSwitch, routePrompt, scanInstalledSkills, scorePrompt, stem } from '../lib/skill-route'; +import { PROMPT_MAX_CHARS, ROUTED_SKILLS, ROUTE_THRESHOLD, SWITCH_DEFAULT, firstStep, hintLine, readSkillSwitch, routePrompt, scanInstalledSkills, scorePrompt, stem } from '../lib/skill-route'; import { ROOT, ROUTE_HOOK, SKILLS, hookEnv, runHook, skillsDir, tmpDir } from './helpers/skill-contract-fixture'; let state = '', skills = ''; @@ -75,12 +77,17 @@ describe('scorer', () => { expect(firstStep(SKILLS.investigate)).toBe('Reproduce the failure before you change code, then fix it and prove the fix.'); }); - test('switch precedence: env over config over default off; typos read as off', () => { - expect(readSkillSwitch('skill_routing', { GSTACK_STATE_ROOT: state })).toEqual({ on: false, source: 'default' }); + test('switch precedence: env over config over SWITCH_DEFAULT; a typo takes the default; an unreadable config is off', () => { + expect(readSkillSwitch('skill_routing', { GSTACK_STATE_ROOT: state })).toEqual({ on: SWITCH_DEFAULT.skill_routing === 'on', source: 'default' }); fs.writeFileSync(path.join(state, 'config.yaml'), 'skill_routing: on\nskill_contracts: maybe\n'); expect(readSkillSwitch('skill_routing', { GSTACK_STATE_ROOT: state })).toEqual({ on: true, source: 'config skill_routing' }); expect(readSkillSwitch('skill_routing', { GSTACK_STATE_ROOT: state, GSTACK_SKILL_ROUTING: 'off' })).toEqual({ on: false, source: 'env GSTACK_SKILL_ROUTING' }); - expect(readSkillSwitch('skill_contracts', { GSTACK_STATE_ROOT: state })).toEqual({ on: false, source: 'config skill_contracts', invalid: 'maybe' }); + expect(readSkillSwitch('skill_contracts', { GSTACK_STATE_ROOT: state })).toEqual({ on: SWITCH_DEFAULT.skill_contracts === 'on', source: 'config skill_contracts', invalid: 'maybe' }); + fs.rmSync(path.join(state, 'config.yaml')); + fs.mkdirSync(path.join(state, 'config.yaml')); + for (const name of ['skill_routing', 'skill_contracts'] as const) { + expect(readSkillSwitch(name, { GSTACK_STATE_ROOT: state })).toEqual({ on: false, source: 'config unreadable' }); + } }); }); @@ -101,8 +108,10 @@ describe('skill-route hook', () => { .toContain('Done means: a DEBUG REPORT with Symptom, Root cause, Fix, Evidence; a test or repro run after your last edit.'); }); - test('off by default, off in config, and off in env over config on', () => { - expect(route(prompt('fix this bug'), {})).toEqual(silent); + test('with nothing set SWITCH_DEFAULT decides; off in config, and off in env over config on', () => { + const unset = route(prompt('fix this bug'), {}); + if (SWITCH_DEFAULT.skill_routing === 'on') expect(expectHookSchema(unset.stdout)).toContain('/investigate'); + else expect(unset).toEqual(silent); fs.writeFileSync(path.join(state, 'config.yaml'), 'skill_routing: off\n'); expect(route(prompt('fix this bug'), {})).toEqual(silent); fs.writeFileSync(path.join(state, 'config.yaml'), 'skill_routing: on\n'); @@ -136,7 +145,7 @@ describe('skill-route hook', () => { expect(traceLog()).toContain('event=skill-problems problems="broken: malformed triggers" failures=1'); }); - test('unreadable config and an unwritable state root: still exit 0 and silent', () => { + test('an unreadable config is off whatever the default; an unwritable state root still routes when on', () => { fs.mkdirSync(path.join(state, 'config.yaml')); expect(route(prompt('fix this bug'), {})).toEqual(silent); const file = path.join(state, 'root-is-a-file'); @@ -158,10 +167,10 @@ describe('skill-route hook', () => { describe('gstack-skill-route --explain', () => { const explain = (...args: string[]) => spawnSync(path.join(ROOT, 'bin', 'gstack-skill-route'), args, { env: hookEnv(state, skills), encoding: 'utf8', timeout: 20_000 }); - test('prints the switch, threshold, candidates with scores, the route and the hint (a dry run with the switch off)', () => { + test('prints the switch, threshold, candidates with scores, the route and the hint (a dry run whatever the switch)', () => { const r = explain('--explain', 'this test fails, fix it'); expect(r.status).toBe(0); - expect(r.stdout).toContain('switch: skill_routing off (default)'); + expect(r.stdout).toContain(`switch: skill_routing ${SWITCH_DEFAULT.skill_routing} (default)`); expect(r.stdout).toContain('threshold: 0.66'); expect(r.stdout).toContain('1.00 /investigate "this test fails"'); expect(r.stdout).toContain('route: /investigate');