diff --git a/docs/skill-routing-and-contracts.md b/docs/skill-routing-and-contracts.md index 5af47cb5a..ea05cd927 100644 --- a/docs/skill-routing-and-contracts.md +++ b/docs/skill-routing-and-contracts.md @@ -73,11 +73,11 @@ 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. `git commit --no-verify` and `cat test/a.test.ts` 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 `&&`, `\|\|`, `;`, `\|`) 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 | | `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:`. A "specialists skipped" line means zero specialists were selected, which passes | | `agent_dispatch` | `match` (regular expression, case-insensitive) | An Agent dispatch's description or prompt matches | -Items are checked against one run of the skill: from its last load to the end of the session, or to its next load when `report` re-checks an earlier run. A load is a Skill tool call, a typed `/name` command, or a Read of its `SKILL.md` under a `skills/` directory (reading the source in a gstack checkout is not a load). A run recorded `met` stays met, so a later unrelated turn in the same session is never blocked by it. +Items are checked against one run of the skill: from its last load to the end of the session, or to its next load when `report` re-checks an earlier run. A load is a Skill tool call, a typed `/name` command, or a Read of its `SKILL.md` under a `skills/` directory (reading the source in a gstack checkout is not a load). A run recorded `met`, `met_waived` or `met_after_block` stays that way, so a later unrelated turn in the same session is never blocked by it. ### Worked example diff --git a/lib/skill-contract.ts b/lib/skill-contract.ts index 5aadf82e4..6cd10c17d 100644 --- a/lib/skill-contract.ts +++ b/lib/skill-contract.ts @@ -253,16 +253,23 @@ 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_PROGRAM = /^(?: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)$|test|spec|repro|check|verify|lint/i; +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 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 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. */ +/** + * 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). + */ 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(); - return CHECK_PROGRAM.test(words[0].split(/[\\/]/).pop() ?? ''); + if (isCheckProgram(words[0])) return true; + return COMMAND_WRAPPER.test(words[0].split(/[\\/]/).pop() ?? '') && words.slice(1, 6).some(isCheckProgram); }); } const DEFAULT_SELECTION_RE = /Dispatching\s+\d+\s+specialists?:\s*(?[^\n]*)/i; @@ -507,10 +514,10 @@ export function writeRecord(file: string, rec: SessionRecord): void { /** * Block at most once per skill run: an unmet contract blocks unless this run was already blocked, which makes it a miss. - * A run recorded met (never blocked) is final: later unrelated turns in the session never reopen it. + * A run recorded met, met_waived or met_after_block is final: later unrelated turns in the session never reopen it. */ export function nextOutcome(prev: RunRecord | undefined, result: CheckResult): { outcome: Outcome; block: boolean } { - if (prev && !prev.blocked_at && (prev.outcome === 'met' || prev.outcome === 'met_waived')) return { outcome: prev.outcome, block: false }; + if (prev && (prev.outcome === 'met' || prev.outcome === 'met_waived' || prev.outcome === 'met_after_block')) return { outcome: prev.outcome, block: false }; if (result.met) return { outcome: prev?.blocked_at ? 'met_after_block' : result.waived.length ? 'met_waived' : 'met', block: false }; return prev?.blocked_at ? { outcome: 'miss', block: false } : { outcome: 'blocked', block: true }; } diff --git a/test/skill-contract-stop-hook.test.ts b/test/skill-contract-stop-hook.test.ts index c50dc885c..80393d79b 100644 --- a/test/skill-contract-stop-hook.test.ts +++ b/test/skill-contract-stop-hook.test.ts @@ -77,6 +77,9 @@ describe('block once, with the pinned text', () => { fs.appendFileSync(file, [tool('b9', 'Bash', { command: 'bun test test/auth.test.ts' }), result('b9', 'pass'), say(DEBUG_REPORT)].join('\n') + '\n'); expect(stop(file, {}, { stop_hook_active: true }).stdout).toBe(''); expect((Object.values(record().runs)[0] as any).outcome).toBe('met_after_block'); + fs.appendFileSync(file, [user('also fix the README typo'), tool('e8', 'Edit', { file_path: 'README.md', old_string: 'teh', new_string: 'the' }), result('e8', 'updated'), say('Fixed.')].join('\n') + '\n'); + expect(stop(file)).toEqual({ code: 0, stdout: '', stderr: '' }); + expect((Object.values(record().runs)[0] as any).outcome).toBe('met_after_block'); }); test('still unmet on the stop_hook_active Stop after our block: a miss, never a second block', () => { diff --git a/test/skill-contract.test.ts b/test/skill-contract.test.ts index 7009ba0f2..01b66f04e 100644 --- a/test/skill-contract.test.ts +++ b/test/skill-contract.test.ts @@ -149,9 +149,10 @@ describe('checkContract is a pure function of the transcript', () => { }); test('check_after_last_edit: a command counts by the program it runs, not by a word inside it', () => { - 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']) + 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']) + 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']) expect([cmd, isCheckCommand(cmd)]).toEqual([cmd, false]); });