Skip to content

hooks: refuse a process wait that matches its own command line - #323

Merged
EdbertChan merged 1 commit into
mainfrom
automate/gh-write-selfmatch-20260909
Sep 9, 2026
Merged

hooks: refuse a process wait that matches its own command line#323
EdbertChan merged 1 commit into
mainfrom
automate/gh-write-selfmatch-20260909

Conversation

@EdbertChan

@EdbertChan EdbertChan commented Sep 9, 2026

Copy link
Copy Markdown
Owner

Summary

Fourth detector for gh-write-verification, following #322. Same class as the other three: a command that silently reports the wrong thing.

pgrep -f and pkill -f compare full command lines. The pattern sits in the argv of the shell that runs them, so the match is never empty.

A wait negated on pgrep -f <name> can therefore never exit. pkill -f <name> kills its own wrapper mid-command.

One agent armed three such waits, ended its turn expecting to be woken, and was never resumed. Two were still spinning 45 minutes later.

The trap is baited by this repo: wait-needs-wakeup blocks a plain foreground wait and tells you to arm a watcher, without saying how to write one that terminates.

It fires on a one-shot check too. The harness runs every tool call as bash -c '<the whole command>'.

So the pattern is already in a live cmdline before the search starts.

pgrep -f postgres therefore exits 0 whether or not postgres runs anywhere. There is no correct plain -f spelling here.

Review Claim

A PreToolUse detector that decides, from the command text alone, whether a pgrep/pkill full-command-line match will also match the shell issuing it — firing on every plain-literal -f form, looped or one-shot, and on a bracket class undone by a plain mention elsewhere, and staying silent on the bracket idiom, a name-only match, a variable pattern, and a log-sentinel wait.

Review Lane

behavior

Review Unit

engine-runtime

Safety Invariant

The detector is pure text analysis: it runs no process lookup, spawns nothing, and reads no file. It reaches a verdict only from the command string already in the hook payload, so it cannot itself be fooled by the live process table it is reasoning about. It is wired into the existing pretooluse_problems() list, which already writes to stderr and exits 2 and has no other write path; a detector exception is trapped by claude_pretooluse.py and reported as "allowing", so a bug can only under-block. A pattern held in a variable or command substitution is deliberately passed through rather than guessed at.

Assumptions: this Safety Invariant is unconfirmed — drafted under a non-interactive delegation with no user available to confirm it, per draft-pr's headless mode.

Slice Rationale

Separate PR because #322 was merged (squashed onto main as e8fbb71) while this commit was still local, so it could not ride along. It is additive to the same hook: one new detector function, its message, its fixtures, and a README section. No existing detector changes.

Non-goals

  • Does not change wait-needs-wakeup, which is what pushes agents into these loops. This only makes a loop terminate; whether the loop should exist stays that hook's call. Expect both to fire on the same command.
  • Does not detect a non-terminating wait in general — only the self-match. A loop waiting on a condition that never becomes true for some other reason is out of scope.
  • Does not exempt a one-shot pgrep -f <service>. Reviewed and kept firing: under a harness that wraps each call in bash -c '<the whole command>', that check is contaminated too — it returns the wrapper and exits 0 for a service running nowhere — so it is a true positive, not a tolerated false one. Narrowing to until/while loops would also miss a one-shot pkill -f and an if pgrep -f X; then guard. The fixture carries this reasoning in its docstring so it is not later relaxed to silent.
  • Does not resolve a pattern held in a variable or command substitution. Undecidable from the text, so it passes through; this under-blocks by design.
  • Does not kill or inspect any running process.
  • Does not rename the hook directory, though gh-write-verification is now narrower than its scope — this detector is not a gh write. Renaming would rewrite every file plus the install.sh wiring and the settings marker; recommended as a follow-up and noted in the README.

Test Plan

Test Plan

Repro before:

  • pgrep -f zzz_selfmatch_demo_ghwv.sh for a token no process on this box uses → RUNNING; pgrep -af on the same token showed it matched the argv of the shell asking the question
  • a token appearing exactly once, as the pgrep argument only → still RUNNING. So a check for "the pattern appears elsewhere in the command" would miss the canonical loop, which names the process once — the detector fires on the plain-literal form instead
  • bare one-shot pgrep -f zzz_unique_token_ghwv_9931, a token on no process anywhere → printed a pid and exit=0; pgrep -af showed the match was the harness's own bash -c wrapper. This is why the one-shot case fires
  • pkill -f vvvv_selfkill_ghwv inside bash -c → the following echo SURVIVED never ran; the wrapper was killed, harness reported exit 144
  • live stuck watcher on this box, PID 1429941: its log sentinel had already matched 35 times, so only the ! pgrep half kept it alive. Killed; ps -p confirmed gone
  • bracket form in isolation → absent; the same bracket form in a command that also spells the token plainly → RUNNING, which is why the detector re-checks for a plain mention

Repro after — fires:

  • the incident wait loop, pkill -f run_all_tests.sh, pgrep -af my_worker.py, pgrep --full …, pgrep -f -u edbert …, a bracket pattern undone by a plain mention in the same command, plus the one-shot pgrep -f postgres and if pgrep -f nginx; then …
  • entrypoint returns exit 2 with the message naming kill -0 and the log-sentinel shape

Repro after — stays silent:

  • bracket idiom alone; a log-sentinel wait; pgrep run_all_tests.sh / pgrep -x bash / pkill -x node (no -f); kill -0 "$PID"; pgrep -f "$PATTERN"; a backtick-substituted pattern; ps aux | head -5

Gates:

  • python3 -m unittest discover -s engine/hooks/gh-write-verification/tests -v — 35 tests, OK
  • bash scripts/run_all_tests.sh — 37 suites, EXIT5=0, no FAILED/ERROR: lines
  • python3 scripts/check_hook_test_coverage.py engine/hooks/gh-write-verification — OK
  • python3 engine/skills/make-pr/scripts/preflight.py --base origin/mainunit engine-runtime: 3 file(s), ok preflight passed
  • python3 scripts/check_no_new_comments.py --base origin/mainok no new comments
  • python3 scripts/check_no_dated_provenance.py --base origin/mainok no dated provenance
  • ruff check . --select E9,F — All checks passed; shellcheck install.sh — clean
  • node engine/skills/draft-pr/scripts/lint-diff-atomicity.mjs --base origin/main --review-lane behavior — passed, no warnings

Revert Plan

Revert Plan
  • Safe to revert? Yes
  • Revert command: git revert <sha>
  • Post-revert steps: None. The detector is additive; reverting leaves the three detectors from hooks: refuse a write whose effect you cannot see #322 wired and working, and no reinstall is needed because the hook's settings.json entries are unchanged.
  • Data migration? No.

Sibling defect worth naming

Three hooks in this repo, including this one, share a failure shape: a scanner that reads data as code.

wait-needs-wakeup blocks on until/sleep appearing anywhere in a payload — it fired twice today on test strings being handed to a detector, not on commands being run, and blocked authoring work on this very PR. no-comments had the same shape until triple-quoted strings were excluded from its scan (#303), and it still misreads a ## markdown heading inside a Python string. This detector is exposed too: it matches patterns in raw payload text, so documenting it in an inline heredoc trips it, which the README records.

Not fixed here — named so the class is visible rather than rediscovered a fourth time.

🤖 Generated with Claude Code

https://claude.ai/code/session_01F43CBUnsDEs6J2zEC1r8a8

Fourth detector for gh-write-verification, same class as the other three: a
command that silently reports the wrong thing.

`pgrep -f` and `pkill -f` compare full command lines, and the pattern sits in
the argv of the shell that runs them, so the match is never empty. A wait
negated on `pgrep -f <name>` can never exit; `pkill -f <name>` kills its own
wrapper mid-command. Verified against a token no process on the box uses:
plain form reports RUNNING, `-af` shows it matched the asking shell, and a
`pkill -f` never reached the next statement in its own script.

The pattern occurring exactly once is enough -- being the pgrep argument is
the occurrence -- so a check for "the pattern appears elsewhere in the
command" would miss the canonical loop, which names the process once. The
detector therefore fires on any plain-literal `-f`/`--full` pattern, and
separately on a bracket-class pattern whose plain spelling still appears
somewhere else in the same command, which is the one case where the standard
bracket workaround stops working.

Silent on a name match without `-f` (a shell is named `bash`, so it cannot
self-match), on the bracket idiom alone, on a pattern held in a variable or
command substitution, and on a wait guarded by a log sentinel the watched
process writes.

catstack's own wait-needs-wakeup hook is what pushes agents into these loops:
it blocks a plain foreground wait and tells you to arm a watcher, without
saying how to write one that terminates. Both hooks fire on the same command
shape, so they will be seen together.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F43CBUnsDEs6J2zEC1r8a8
@cursor

cursor Bot commented Sep 9, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_b3051198-9102-465d-982c-569413fdc815)

@EdbertChan
EdbertChan merged commit 6c7112b into main Sep 9, 2026
4 checks passed
mergify Bot pushed a commit that referenced this pull request Sep 10, 2026
…331)

* Add ship-a-detector: the playbook for authoring a hook or gate detector

65 of 185 merged PRs in this repo touch engine/hooks/, 13 of 28 hooks
needed post-ship repair, and 37 PRs did nothing but repair a shipped
detector. gh-write-verification hit two already-known kinds in three days
(#322, #323, #324). create-skill never mentions hooks, CONTRIBUTING has no
hook section, and 10 of 31 hooks have no docs/ecosystem.md row.

The playbook is a verbatim 20-step ordered list, copied into a todolist
before task-specific work; a step that does not apply stays in the list
marked `skip: <reason>`. Steps 4-10 are the seven recurring defect kinds,
one per step. Steps 14-18 are the install, README, and inventory tail that
this repo measurably drops. Step 20 hands off to make-pr.

Every numbered step cites the PRs that motivated it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* invoker: wf-1788992325476-4/ship-a-detector-playbook — Review claim: authoring a detector follows one written, ordered sequence whose steps come from the repair history rather than from memory.
Review lane: behavior
Safety invariant: the playbook adds no new mechanical gate and changes no existing hook, so a detector that passes CI today still passes; it can only change what a future author does before opening a PR.
Slice rationale: the sequence has to exist before anything can route to it, and its steps are derived from thirty-seven specific PRs, so writing it is a separate reviewable claim from wiring the routing.
Architectural effect: adds the repo's second playbook package; its final step calls the existing make-pr skill rather than duplicating publication.
Goal: add product/skills/ship-a-detector with a playbook carrying the ordered steps that the thirty-seven repair PRs imply.
Effectiveness measurement: each of the seven recurring defect kinds maps to a numbered step, and each step cites the PR numbers that motivated it, so a reader can check the step against the incident rather than trusting the author.
Motivation: gh-write-verification shipped this month and hit two of the seven known kinds within three days, authored by an agent with the whole corpus available; the knowledge existed and was not reachable.
Alternative considerations: adding the steps to create-skill was rejected because that skill covers skills rather than hooks and mixing them would bury both; a new hook was rejected because the gap is a missing order, not a missing check.
Implementation details: the seven kinds are target and scope resolution, false positive on quoted or fenced content, missed near-miss shape, retry-loop behaviour under stop_hook_active, first-failure exit hiding a second check, unreadable input reported as clean, and lifecycle state across turns.
Non-goals: does not modify any existing hook, does not add a CI gate, does not change create-skill, does not reimplement what make-pr does.
Layer: domain
Feature state: active
Files: product/skills/ship-a-detector/** (new)
Change types: new skill, new playbook
Acceptance criteria: check_skill_test_coverage.py passes for the new skill, and every numbered step cites at least one PR number.

Exit code: 0

---------

Co-authored-by: Edbert Chan <chanedbert@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants