diff --git a/corpus/CLAUDE.learned.md b/corpus/CLAUDE.learned.md index 4d245362..8336b6a4 100644 --- a/corpus/CLAUDE.learned.md +++ b/corpus/CLAUDE.learned.md @@ -19,7 +19,7 @@ Engine-only install drops this file; reflect Accepted global rules land here. - When a fix changes which identity, binary, or database a process uses, verify the one the *service* resolves, not the one your shell resolves. Same session: `which invoker-cli` from an ssh login returned a 0.1.0 system install under `/usr/lib/node_modules`, while the owner process's own `PATH` resolved a different 0.1.1 binary under `~/.local/npm-global/bin`; and a worker's inventory query shelled a checkout script that opened a database holding a week-old snapshot (60 workflows, newest 8 days old) instead of the live owner's (18 workflows, newest that day). - Absence of a field in a projection (CLI, MCP, or API output) is not proof of absent state. Say "not projected" and find the emitter before retrying, resubmitting, or switching agents. Seen once: a task sat in `needs_input` with its reason stored by the owner but omitted by every headless query; the session guessed three causes (usage limit, agent switch, dirty branch) and resubmitted twice before reading the emitter, which then took four minutes. - When delegating a file-finding task to a subagent and two files could plausibly hold the same answer (a duplicate, a moved file, a same-named symbol in two packages), tell the subagent to state whether each file:line claim is "read-confirmed" (it opened the actual reference/import and traced it) or "name-matched" (it assumes the file is the one in use because the name/path looked right). A subagent that reasons by name-proximity instead of tracing the real reference can hand back a confident wrong file — a judgment call about how the subagent qualifies its own confidence, not something a mechanical check can catch. Seen once: a subagent named a dead, unused duplicate file as the source of a bug; the citation was relayed as fact for three turns before a direct Read caught it. -- A `file:line` citation, mine or a subagent's, also names the ref it was read at: working tree, `HEAD`, `origin/`, or the installed bundle. A working-tree read in a checkout with untracked or modified files under the cited path is name-matched, not read-confirmed, until the same line is shown at the ref the change will actually run on. Subagent prompts that ask for read-confirmed vs name-matched must also ask for the ref. Seen repeatedly: a fix was planned against `scripts/e2e-regression-watch.mjs:330` read on a dirty feature branch after `origin/master` had already removed it (#11593); a launch-time freshness gate was misdiagnosed from an untracked copy of `task-specification-preflight.ts`; three Explore subagents reported branch-local lines as read-confirmed. Third instance after the memory `verify-bug-findings-against-master`. +- A `file:line` citation, mine or a subagent's, also names the ref it was read at: working tree, `HEAD`, `origin/`, or the installed bundle. A working-tree read in a checkout with untracked or modified files under the cited path is name-matched, not read-confirmed, until the same line is shown at the ref the change will actually run on. Subagent prompts that ask for read-confirmed vs name-matched must also ask for the ref. Seen repeatedly: a fix was planned against `scripts/e2e-regression-watch.mjs:330` read on a dirty feature branch after `origin/master` had already removed it (Invoker #11593); a launch-time freshness gate was misdiagnosed from an untracked copy of `task-specification-preflight.ts`; three Explore subagents reported branch-local lines as read-confirmed. Third instance after the memory `verify-bug-findings-against-master`. - When a pipeline switches from test/synthetic inputs to my real inputs, re-derive or explicitly re-validate every artifact built under the old conditions — voice clones, cached device lists, presigned URLs, browser sessions that predate a driver install. State which artifacts were rebuilt and which were kept. Seen once: a voice clone built from a bench test-mp3 silently became the live demo voice ("WHY IS MY VOICE ROBOTIC?"), and a Chrome that predated the BlackHole driver install couldn't enumerate it until fully relaunched. # Session hygiene (apply everywhere, every project) diff --git a/engine/hooks/skill-usage-log/tests/test_hooks.py b/engine/hooks/skill-usage-log/tests/test_hooks.py index 980b3053..27cd4c2d 100644 --- a/engine/hooks/skill-usage-log/tests/test_hooks.py +++ b/engine/hooks/skill-usage-log/tests/test_hooks.py @@ -64,7 +64,9 @@ def test_disabled_flag_writes_nothing(self): state_dir=tmp, ) self.assertEqual(read_lines(tmp), []) - self.assertFalse(os.path.exists(claude_pretooluse_log.log_path())) + self.assertFalse( + os.path.exists(os.path.join(tmp, claude_pretooluse_log.LOG_FILE_NAME)) + ) def test_missing_skill_name_writes_nothing(self): with tempfile.TemporaryDirectory() as tmp: diff --git a/product/skills/ship-a-detector/SKILL.md b/product/skills/ship-a-detector/SKILL.md index 51f71217..37792591 100644 --- a/product/skills/ship-a-detector/SKILL.md +++ b/product/skills/ship-a-detector/SKILL.md @@ -22,7 +22,7 @@ Measured on this repo's own history: 65 of 185 merged PRs touch `engine/hooks/`, 13 of 28 hooks needed post-ship repair, and 37 PRs did nothing but repair a shipped detector. `diu-stop` took 8, `pr-schema-gate` 7, `wrong-check-reflect` 6, `scope-lock` 4. `gh-write-verification` hit two -separate known kinds in three days (#322, #323, #324). None of those were +separate known kinds in three days. None of those were new problems; each was a kind already fixed in another hook. ## The playbook diff --git a/product/skills/ship-a-detector/playbooks/detector-lifecycle.md b/product/skills/ship-a-detector/playbooks/detector-lifecycle.md index b5fb13c1..0b02ef66 100644 --- a/product/skills/ship-a-detector/playbooks/detector-lifecycle.md +++ b/product/skills/ship-a-detector/playbooks/detector-lifecycle.md @@ -13,7 +13,7 @@ dropped rather than skipped out loud. `skip: widening an existing hook, no new manifest` is a fine reason. `skip:` with nothing after it is not. Steps 1–3 frame the change. Steps 4–10 are the seven defect kinds, one -kind per step, each with the PRs that paid for it. Steps 11–13 are the +kind per step, each with the failure that paid for it. Steps 11–13 are the evidence. Steps 14–19 are the tail — the install, README, and inventory wiring, which belongs here and not to `make-pr`. Step 20 publishes. @@ -28,15 +28,13 @@ it. Then run your existing patterns against that text and record the hit count. `wrong-check-reflect` got its patterns widened three separate times because -each new shape was described rather than pasted. #220 pasted one real reply -— "My mistake — the skill does have disable-model-invocation: true (I -misread it)" — and it scored zero hits against every pattern the hook had. -#298 went further and counted the shape across the whole transcript corpus: -53 replies opened with that concession, 13 of them straight after a human -correction, and none matched. #322 built four detectors off three sessions -that all hit the same broken `gh pr edit` in one day. - -Cited: #220, #298, #322. +each new shape was described rather than pasted. One real reply, pasted — +"My mistake — the skill does have disable-model-invocation: true (I +misread it)" — scored zero hits against every pattern the hook had. +Counting the concession shape across the whole transcript corpus found 53 +replies that opened with it, 13 of them straight after a human correction, +and none matched. `gh-write-verification` built four detectors +off three sessions that all hit the same broken `gh pr edit` in one day. ## 2. Write down the neighbours you are NOT catching @@ -47,32 +45,30 @@ your pattern is not specific enough yet to know what it will hit. `gh-write-verification` names its silent set explicitly and keeps a fixture per entry: the bracket idiom, a name match without `-f`, a pattern held in a variable or command substitution, `kill -0 "$PID"`, and a log-sentinel -wait (#323). #324 widened the same detector and its Non-goals open with -"Does not change the silent set. Every previously-allowed shape still has a -passing negative fixture." #322's detector 2 is an allowlist of +wait. A widening says up front that the silent set holds, the way a +`gh-write-verification` widening opened its Non-goals: "Does not change the +silent set. Every previously-allowed shape still has a passing negative +fixture." Without that line a reviewer cannot tell a deliberate new block +from a neighbour the widening broke. The hook's detector 2 is an allowlist of state-changing commands rather than a blocklist of safe ones, so every read-only command with discarded output is silent by construction, not by exclusion rule. -Cited: #322, #323, #324. - ## 3. Pick the surface, and know what that surface can see Choose one and say why: - `PreToolUse` — the command text, **before** the command runs. It cannot see the effect, and the payload `cwd` is the session's launch directory, - not where the command will actually execute (#61). + not where the command will actually execute. - `Stop` / `SubagentStop` — the outgoing message plus the transcript file. - `UserPromptSubmit` — the user's turn, including anything they pasted. - `scripts/check_*.py` — the repo at CI time, run from `.github/workflows/ci.yml`. A rule that a CI script can decide from the tree belongs in a script, not a -hook: #296 put "a file-reading detector must have an unreadable-input test" -into `check_hook_test_coverage.py`, where it fails a build, rather than -into prose asking authors to remember. - -Cited: #61, #296, #322. +hook: "a file-reading detector must have an unreadable-input test" lives +in `check_hook_test_coverage.py`, where it fails a build, rather than in +prose asking authors to remember. ## 4. Resolve the target before you match on it @@ -83,91 +79,82 @@ same hook: - a leading `cd &&` / `cd ;` in the command text — `PreToolUse` fires before the shell runs, so the payload `cwd` cannot know a `cd` is - coming (#61). -- a `workdir` field, including Codex's JavaScript-wrapped form (#103). + coming. +- a `workdir` field, including Codex's JavaScript-wrapped form. - `gh`'s own `--repo` / `-R` flag, resolved against the sibling-checkout - convention (#217). + convention. -#217's own Slice Rationale calls itself the "second, independent instance -of the class #61 already fixed once". Do the enumeration in one pass rather -than paying for a third. When the resolver cannot resolve — the flag is -present but no matching checkout exists on disk — do not guess; take the +The `--repo` fix described itself as the second, independent instance of a +class the `cd` fix had already closed once. Do the enumeration in one pass +rather than paying for a third. When the resolver cannot resolve — the flag +is present but no matching checkout exists on disk — do not guess; take the step-11 decision explicitly. -Cited: #61, #103, #217. - ## 5. Strip quoted, fenced, pasted and rehearsed content before matching Text that merely *contains* your trigger is not your trigger. Five separate false-positive PRs, five distinct sources: - **Fenced blocks.** `diu-stop` counted a fenced YAML plan as prose and - blocked a legitimate reply, which lost a staged plan draft (#66 proves - it, #67 fixes it). Require a real closing fence when you strip, or an + blocked a legitimate reply, which lost a staged plan draft. Require a + real closing fence when you strip, or an unterminated ` ``` ` becomes a way to smuggle unlimited text past the gate. - **Pasted transcripts.** `scope-lock` scanned a whole prompt, so a 101,873-character paste quoting *another* session's directives matched at 689 characters from the end. Bound the scan to the last 400 characters, - where a live directive actually sits (#83). + where a live directive actually sits. - **Relayed machine turns.** A `` subagent report arrives shaped like a user turn and can quote your trigger phrases well enough to drive state — including clearing a hard stop by quoting the - escape hatch in prose. Exempt notification-shaped text entirely (#83). + escape hatch in prose. Exempt notification-shaped text entirely. - **Quotes and meta-description.** `wrong-check-reflect` and `restart-risk-check` both fired on text that only quoted or described - their own trigger phrases (#174). + their own trigger phrases. - **Rehearsals.** `pr-schema-gate` armed its pending state on a - `--dry-run`, then blocked the real push that followed (#299). A flag that + `--dry-run`, then blocked the real push that followed. A flag that means "do not actually do it" must not arm state that assumes it was done. -Cited: #66, #67, #83, #174, #299. - ## 6. Enumerate the near-miss shapes of the same meaning Your first pattern catches the phrasing you saw. Write out the other spellings of the same claim and grep the transcript corpus for each one before shipping. Seven PRs, all the same job, all after the fact: -- the hedged form — "I think X happened" alongside the bare claim (#202, - #208) +- the hedged form — "I think X happened" alongside the bare claim - reversed word order, and the follow-on clause that never uses the - trigger word at all (#216) + trigger word at all - the self-caught form — "my mistake", "I misread X" — with no reference - to a prior check (#220) + to a prior check - the concession form — conceding the user's instinct beat your own checks - (#298) - the **unhedged** form, when the gate only knew hedges: a confident wrong - diagnosis passed a bar the tentative one would have failed (#319) -- the one-shot form of a shape you only caught in a loop. #323 exempted a - bare `pgrep -f` as a legitimate "is it running?" check; #324 tested that - assumption three days later and it was false — the harness runs every + diagnosis passed a bar the tentative one would have failed +- the one-shot form of a shape you only caught in a loop. + `gh-write-verification` exempted a bare `pgrep -f` as a legitimate "is it + running?" check, and the exemption was false — the harness runs every tool call as `bash -c ''`, so the pattern is already in a live command line before the search starts, and a bare `pgrep -f` for a token on no process returned a pid and exit 0. For each shape, add a positive fixture. When you exempt a shape, test the -exemption rather than assuming it (that is exactly what #324 cost). - -Cited: #202, #208, #216, #220, #298, #319, #324. +exemption rather than assuming it (that is exactly what the `pgrep -f` +exemption cost). ## 7. Return early on `stop_hook_active`, and name the deficit in the block A `Stop` hook that blocks, gets a rewrite, and blocks the rewrite is a loop. `diu-stop` blocked the same message nine times while the agent shaved -a few words per attempt, until the harness cap overrode it (#124). Return +a few words per attempt, until the harness cap overrode it. Return when `stop_hook_active` is set, like the sibling Stop hooks do: one block, one rewrite, then pass. The block message is half the fix. Say the exact deficit and the structural -change — "cut 40 words: drop a section" — not "shorten it". #124's own -Review Claim pairs the two: the `stop_hook_active` return **and** the -reason naming how many words to cut. #319's `decide()` short-circuits on -`stop_hook_active` before either new path runs, so a widening cannot -reintroduce the loop. - -Cited: #124, #319. +change — "cut 40 words: drop a section" — not "shorten it". The `diu-stop` +repair paired the two: the `stop_hook_active` return **and** the reason +naming how many words to cut. `hedge-runs-prove-it`'s `decide()` +short-circuits on `stop_hook_active` before either of its paths runs, so a +widening cannot reintroduce the loop. ## 8. Run every check; never exit on the first failure @@ -176,23 +163,21 @@ Cited: #124, #319. long *and* made an unverified claim never reached the unverified-claim check — the one that would have caught the wrong root cause. An existing test asserted this was intentional. Three wrong root-cause claims reached -the user that way (#77). +the user that way. -Then pin the interaction. #207 added a fix/re-trigger matrix: for each +Then pin the interaction with a fix/re-trigger matrix: for each trigger, a `broken` message that must block and a `fixed` message — the compliant rewrite — that must not block by *any* check, plus an explicit `KNOWN_DOUBLE_BLOCKS` list for the one case where a fix deliberately still trips a second check. Unit-testing each check alone does not catch this. -Cited: #77, #207. - ## 9. Give the detector three outcomes, not two Found the thing, did not find the thing, **could not read the input at all**. Collapsing the third into the second is how a guard reports an unchecked file as clean. The first `ui-input-guard` skipped any wrapper script over 64 KB and any path holding an unresolved shell variable, and -returned "clean" for both (#296). +returned "clean" for both. `scripts/check_hook_test_coverage.py` now fails any file-reading detector that has no test pinning its unreadable-input behaviour, so this step is a @@ -200,31 +185,26 @@ build failure, not a habit. Name the test with vocabulary that gate recognises: `unreadable`, `malformed`, `corrupt`, `missing`, `too_large`, `fails_open`. -Cited: #296. - ## 10. If the detector remembers anything across turns, write the state machine down first Cross-turn state has its own failure set, and this repo has hit each half: - A state that is set but never consumed. `scope-lock` reached `contract_required` and never transitioned to `locked` on the next - prompt, so the approved continuation could not reach tools (#186). + prompt, so the approved continuation could not reach tools. - A state that is never set. `pr-schema-gate` had no memory that a publication happened, so `mergify stack push` published a bare body and nothing made the `create-pr.mjs` follow-up happen. A published PR then sits with a bare `Depends-On:` body until someone reads it, because no - step downstream of the publish knows the enrichment is still owed - (#194, #196). + step downstream of the publish knows the enrichment is still owed. Write every transition, then a test per transition. Keep the state file -small, one per repo, **outside the worktree**, with a TTL — #196 used two -hours. Every state read that cannot be understood means "nothing owed": -missing, unreadable, malformed, wrong-typed, future-dated and expired all -read as nothing owed, and an unwritable state directory logs to stderr and -allows (#194). Arm state on the real action only, never on a rehearsal -(#299, step 5). - -Cited: #186, #194, #196, #299. +small, one per repo, **outside the worktree**, with a TTL — `pr-schema-gate` +uses two hours. Every state read that cannot be understood means "nothing +owed": missing, unreadable, malformed, wrong-typed, future-dated and +expired all read as nothing owed, and an unwritable state directory logs to +stderr and allows. Arm state on the real action only, never on a rehearsal +(step 5). ## 11. Decide the fail direction and write it in the README @@ -235,26 +215,23 @@ guessing. `scope-lock`'s two reads resolve in opposite directions on purpose: a corrupt state file means no lock is in force, while an unreadable transcript means no scope contract was recorded, so an existing lock is -**not** released by a file that could not be read (#296). +**not** released by a file that could not be read. `gh-write-verification` traps every uncaught detector exception and reports -"allowing", so a detector bug can only under-block (#322). `pr-schema-gate` -fails open when a `--repo` flag names a repo with no matching checkout -(#217). #296's gate deliberately never asserts which direction you must -pick — only that you tested the one you picked. - -Cited: #217, #296, #322. +"allowing", so a detector bug can only under-block. `pr-schema-gate` +fails open when a `--repo` flag names a repo with no matching checkout. +`check_hook_test_coverage.py` deliberately never asserts which direction +you must pick — only that you tested the one you picked. ## 12. Write the fixtures: positive, negative, unreadable In `engine/hooks//tests/`: - one positive test per shape from steps 1 and 6, using the pasted real - payload, not a synthetic approximation (#220, #298); + payload, not a synthetic approximation; - one negative test per neighbour from step 2, and a negative for every - shape you deliberately exempted (#323, #324); -- one unreadable-input test if the detector opens, sizes, or stats a file - (#296); -- the fix/re-trigger matrix if the hook has more than one check (#207). + shape you deliberately exempted; +- one unreadable-input test if the detector opens, sizes, or stats a file; +- the fix/re-trigger matrix if the hook has more than one check. Then run the gate on the hook by name: @@ -265,25 +242,22 @@ python3 scripts/check_hook_test_coverage.py engine/hooks/ It classifies by test-name substring only — it cannot judge whether a test reproduces the right scenario. That judgment is still yours. -Cited: #207, #220, #296, #298, #323, #324. - ## 13. Prove fail-before / pass-after, and keep the prose gates green Run the new test against the **unmodified** detector and show it failing, -then against the change and show it passing, with both outputs pasted. #66 -went as far as landing the failing regression test as its own PR before -#67 landed the fix. #299 ran each suite twice — once with only the detector -reverted, once whole. +then against the change and show it passing, with both outputs pasted. The +`diu-stop` fence fix went as far as landing the failing regression test as +its own PR before the fix landed. Running each suite twice — once with only +the detector reverted, once whole — is the same proof in one PR. Two repo gates apply to everything you just wrote: - **No explanatory comments in code.** `scripts/check_no_new_comments.py` - is the CI twin of the `no-comments` PreToolUse hook (#128). The detector + is the CI twin of the `no-comments` PreToolUse hook. The detector explains itself in the README and the test names. - **No dated provenance lines.** No "as of ", no incident narrative - with a date in the rule text (#125, #198). - -Cited: #66, #67, #125, #128, #198, #299. + with a date in the rule text, and no citation of this repo's own PRs or + issues. ## 14. Wire it into `install.sh` — the symlink AND the settings merge @@ -297,36 +271,31 @@ one that gets forgotten: settings-merge block, plus the `claude.hook.json` fragment and the idempotent marker-based installer it reads. -#114's first line: "Two shipped hooks never ran. install.sh symlinked them -into the hooks dir but no installer merged their commands into -settings.json." `frustration-watchdog` and `demo-freeze` were both live in -the repo and both dead. #301 hit the identical failure again with -`history-claim-check` and logged it as "A hook dir that ships but never -runs". +"Two shipped hooks never ran. install.sh symlinked them into the hooks dir +but no installer merged their commands into settings.json." +`frustration-watchdog` and `demo-freeze` were both live in the repo and +both dead. `history-claim-check` later hit the identical failure, logged as +"A hook dir that ships but never runs". If the hook wires `Stop`, `scripts/mirror_stop_hooks_to_subagent_stop.py` mirrors it to `SubagentStop` automatically from your manifest. To opt out, say so in the manifest with a reason — `"subagent_stop": {"inherit": -false, "reason": "..."}` — never by omission (#260). - -Cited: #114, #260, #301. +false, "reason": "..."}` — never by omission. ## 15. Assert the wiring in `tests/test_install.py` Add the assertion that would have failed yesterday: the symlink resolves to `engine/hooks/`, and each entrypoint appears under its event in the -merged settings. #322's block is the shape to copy — it asserts the link -target, then `PreToolUse`, `Stop`, and `SubagentStop` each contain the -right command. +merged settings. The `gh-write-verification` block is the shape to copy — +it asserts the link target, then `PreToolUse`, `Stop`, and `SubagentStop` +each contain the right command. -#114 added a class-level test that every `claude_*.py` entrypoint under +A class-level test checks that every `claude_*.py` entrypoint under `engine/hooks` is wired after install, which is the check that makes step 14 impossible to forget again. A link is not proof the install took effect: `scripts/check_install_effective.py` runs at the end of `install.sh` for exactly that reason, and `install.sh` exits 3 and names shadowed items -rather than letting a skip read as success (#301). - -Cited: #114, #301, #322. +rather than letting a skip read as success. ## 16. Write `engine/hooks//README.md` @@ -334,14 +303,13 @@ All 31 hooks in this repo have one; yours is not special. Per detector: what it fires on, what it stays silent on, the exact block message, the fail direction from step 11, and the escape hatch if there is one — like `GH_WRITE_VERIFICATION_TRUST_PR_EDIT=1`, which lifts detector 1 once the -CLI stops erroring (#322). +CLI stops erroring. -The README line lands in the **same** PR as the pattern, not after. #220 -shipped "two new admission patterns, two new negative guards, plus their -tests and the README line documenting them" as one slice; #323 and #324 -each carried their own README section in the diff. - -Cited: #220, #322, #323, #324. +The README line lands in the **same** PR as the pattern, not after. A +`wrong-check-reflect` widening shipped "two new admission patterns, two new +negative guards, plus their tests and the README line documenting them" as +one slice; both `gh-write-verification` widenings carried their own README +section in the diff. ## 17. Add the `docs/ecosystem.md` inventory row @@ -350,25 +318,22 @@ qualifier when it needs one — `hook (advisory)`, `hook (advisory; off by default)`, `hook (not always installed)`. This is the measured weak point. 10 of 31 hooks have no row at all, and 8 -got theirs in a later PR: #246, #249, #250, #251 and #252 each shipped a -hook with no row, and #253 came back afterwards to add all of them at once. -#290 did it right — the hook and its row landed in the same commit. - -Cited: #246, #249, #250, #251, #252, #253, #290. +got theirs in a later PR: five hook PRs each shipped with no row, and one +docs PR came back afterwards to add all of them at once. `hook-freshness` +did it right — the hook and its row landed in the same commit. ## 18. Add the pointer from the owning skill A detector that routes to a skill must be named in that skill's own trigger or invoke list, or the written trigger and the detector disagree about when -it fires. #298 added one trigger line to `reflect`'s invoke list alongside -the pattern, and says why: "The reflect skill's invoke list names the same -shapes, so the written trigger and the detector agree." +it fires. `wrong-check-reflect`'s concession widening added one trigger +line to `reflect`'s invoke list alongside the pattern, and says why: "The +reflect skill's invoke list names the same shapes, so the written trigger +and the detector agree." If the detector is a gate rather than a hook, the pointer is the `CONTRIBUTING.md` line under `## Test` that tells a contributor to run it. -Cited: #298. - ## 19. Run every step in `.github/workflows/ci.yml` Not a subset. Locally, in the order CI runs them: @@ -391,12 +356,10 @@ shellcheck install.sh ``` Paste the real output into the Test Plan. A skipped step is not a pass — -say which ran and which did not. #299's Test Plan ran each suite twice and -pasted both; #298 ran "every `engine/hooks/*/tests` directory plus +say which ran and which did not. A good Test Plan runs each suite twice and +pastes both, or runs "every `engine/hooks/*/tests` directory plus `tests/`, the way CI runs them" rather than only the hook it touched. -Cited: #298, #299. - ## 20. Call `make-pr` Now, and not before. Steps 14–18 are already done, so the PR is complete @@ -416,5 +379,3 @@ one PR. `make-pr` owns the PR body schema, the confirmation rules, and the diff-atomicity gate. It does not own steps 14–18 — that is why they are numbered here. - -Cited: #217, #322. diff --git a/product/skills/ship-a-detector/tests/test_playbook_names_resolve.py b/product/skills/ship-a-detector/tests/test_playbook_names_resolve.py new file mode 100644 index 00000000..0e70c8a6 --- /dev/null +++ b/product/skills/ship-a-detector/tests/test_playbook_names_resolve.py @@ -0,0 +1,47 @@ +#!/usr/bin/env python3 +from __future__ import annotations + +import re +import sys +import unittest +from pathlib import Path + +HERE = Path(__file__).resolve().parent +SKILL = HERE.parent +REPO = SKILL.parents[2] +PLAYBOOK = SKILL / "playbooks" / "detector-lifecycle.md" +sys.path.insert(0, str(REPO / "engine" / "skills" / "make-pr" / "scripts")) +import preflight # noqa: E402 + +NAME_RE = re.compile(r"`([a-z][a-z0-9]*(?:-[a-z0-9]+)+)`") +NAME_HOMES = ("engine/hooks", "engine/skills", "corpus/skills", "product/skills") + + +def cited_names(text: str) -> set[str]: + return set(NAME_RE.findall(text)) + + +def unresolved(names: set[str], repo: Path) -> list[str]: + units = {unit for _, unit in preflight.UNIT_RULES} + return sorted( + name for name in names + if name not in units and not any((repo / home / name).is_dir() for home in NAME_HOMES) + ) + + +class TestPlaybookNamesResolve(unittest.TestCase): + def test_every_hook_or_skill_the_playbook_cites_exists(self): + names = cited_names(PLAYBOOK.read_text(encoding="utf-8")) + self.assertIn("gh-write-verification", names) + self.assertEqual(unresolved(names, REPO), []) + + def test_a_renamed_hook_is_reported_as_unresolved(self): + names = cited_names("`diu-stop` blocked it, then `diu-stop-renamed` did too.") + self.assertEqual(unresolved(names, REPO), ["diu-stop-renamed"]) + + def test_review_units_resolve_without_a_directory(self): + self.assertEqual(unresolved({"engine-runtime", "product-skill"}, REPO), []) + + +if __name__ == "__main__": + unittest.main() diff --git a/scripts/check_no_dated_provenance.py b/scripts/check_no_dated_provenance.py index ff0984c3..08f5a797 100755 --- a/scripts/check_no_dated_provenance.py +++ b/scripts/check_no_dated_provenance.py @@ -21,9 +21,14 @@ "owner/repo#12" slug, a foreign github.com URL, or a proper noun sitting in front of the tracker word ("Invoker PRs #10553") is left alone. -An unadorned hash-number carrying no tracker word is deliberately NOT matched: -this repo's own rule text already cites a Cook principle and an Invoker pull -request that way, so the shape alone cannot separate it from prior art. +A bare "#322" with no tracker word is, by that same convention, a link into +this repo, so it is rejected unless something on the line binds the number to +another work: a capitalised title word directly in front ("Tip #164", +"Cook #3", "Invoker #11593"), a URL or a closing title quote before a dash +("...pdf -- #3"), or an earlier hash-number it continues ("#10553-#10558", +"#3, #4"). Code spans are ignored, because GitHub does not link a number +inside one. Punctuation between the word and the number breaks the bind, so +"Cited: #220" and "(#194)" fail. Two modes: python3 scripts/check_no_dated_provenance.py [ROOT] @@ -66,9 +71,15 @@ GITHUB_URL_SLUG_RE = re.compile(r"github\.com/([\w.-]+/[\w.-]+)", re.IGNORECASE) QUALIFIER_RE = re.compile(r"([`\w][\w.`/-]*)[^\w`]*$") SENTENCE_LEADS = frozenset( - """a an and as at but by each every for from in inside it its one on onto our per see - that the their these this those to via when where with""".split() + """a after also an and as at because before but by each every for from if in inside it + its once one on onto our per see since so that the their then these this those to + until via when where while with""".split() ) +CODE_SPAN_RE = re.compile(r"`[^`]*`") +BARE_REF_RE = re.compile(r"(? re.Pattern: @@ -119,13 +130,30 @@ def _names_other_repo(prefix: str) -> bool: return word[:1].isupper() +def _binds_to_title(prefix: str) -> bool: + if CONTINUED_REF_RE.search(prefix) or DASH_AFTER_TITLE_RE.search(prefix): + return True + match = TITLE_WORD_RE.search(prefix) + if not match: + return False + word = match.group(1) + return word[:1].isupper() and word.lower() not in SENTENCE_LEADS + + +def _cites_bare_number(line: str) -> bool: + text = CODE_SPAN_RE.sub(lambda m: " " * len(m.group()), line) + return any(not _binds_to_title(text[: m.start()]) for m in BARE_REF_RE.finditer(text)) + + def _cites_repo_tracker(line: str) -> bool: if OWN_URL_RE.search(line) or OWN_NAME_REF_RE.search(line): return True named = FOREIGN_SLUG_RE.findall(line) + GITHUB_URL_SLUG_RE.findall(line) if any(slug.lower() != REPO_SLUG.lower() for slug in named): return False - return any(not _names_other_repo(line[: m.start()]) for m in TRACKER_REF_RE.finditer(line)) + if any(not _names_other_repo(line[: m.start()]) for m in TRACKER_REF_RE.finditer(line)): + return True + return _cites_bare_number(line) def _line_violates(rel: str, line: str) -> bool: diff --git a/tests/test_no_dated_provenance.py b/tests/test_no_dated_provenance.py index 65758295..6a4e8a2e 100644 --- a/tests/test_no_dated_provenance.py +++ b/tests/test_no_dated_provenance.py @@ -119,7 +119,43 @@ def test_external_numbered_titles_pass(self): "- Richard I. Cook, *How Complex Systems Fail* \u2014 #3, \"Catastrophe requires " "multiple failures\".\n" "- **Battle-tested #2, cumulative drift without any single large payload:** a corpus.\n" - "- a fix was planned against a branch after `origin/master` removed it (#11593).\n", + "- a fix was planned against a branch after `origin/master` removed it (Invoker #11593).\n", + ) + result = _run(root) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + + def test_bare_repo_number_citations_fail(self): + lines = [ + "separate known kinds in three days (#322, #323, #324). None of those were", + "Cited: #220, #298, #322.", + "#217's own Slice Rationale calls itself the \"second, independent instance", + "of the class #61 already fixed once\". Do the enumeration in one pass rather", + "exemption rather than assuming it (that is exactly what #324 cost).", + "Then #324 tested that assumption three days later.", + "- a fix was planned against a branch after `origin/master` removed it (#11593).", + ] + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + _write(root / "product/skills/demo/playbooks/lifecycle.md", "# demo\n\n" + "\n".join(lines) + "\n") + result = _run(root) + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + for lineno in range(3, 3 + len(lines)): + self.assertIn(f"product/skills/demo/playbooks/lifecycle.md:{lineno}:", result.stdout) + + def test_hash_numbers_bound_to_another_work_pass(self): + cook = _live_lines_containing("corpus/CLAUDE.learned.md", "HowComplexSystemsFail.pdf — #3") + self.assertTrue(cook, "corpus/CLAUDE.learned.md no longer cites Cook #3 by URL; fixture cannot run") + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + _write( + root / "corpus/skills/demo/SKILL.md", + "# demo\n\n" + + "\n".join(cook) + + "\n- [Cook #3, #4] both apply.\n" + "- Invoker PRs #11887–#11891 shipped a skill named push-not-poll.\n" + "- A bare `Depends-On: #10736` body is not enough.\n" + "- Jump to [step 1](#1-paste-the-real-payload) first.\n" + "- An em dash is — and a fragment is https://example.com/page.html#3.\n", ) result = _run(root) self.assertEqual(result.returncode, 0, result.stdout + result.stderr) @@ -258,6 +294,17 @@ def test_newly_added_repo_pull_request_ref_fails(self): self.assertEqual(result.returncode, 1, result.stdout + result.stderr) self.assertIn("corpus/skills/demo/SKILL.md", result.stdout) + def test_newly_added_bare_repo_number_fails(self): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + self._init_repo_with_baseline(root, "# demo\n\nAlways check disk first.\n") + _write(root / "corpus/skills/demo/SKILL.md", "# demo\n\nAlways check disk first.\n\nCited: #220, #298.\n") + _git(root, "add", "-A") + _git(root, "commit", "-q", "-m", "add bare repo ref") + result = _run_diff(root, "HEAD~1") + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + self.assertIn("corpus/skills/demo/SKILL.md:5", result.stdout) + def test_preexisting_dated_line_untouched_by_diff_passes(self): with tempfile.TemporaryDirectory() as tmp: root = Path(tmp)