From 45f2e496a731a11ea27cf6eb8ca1367a57b711a9 Mon Sep 17 00:00:00 2001 From: Edbert Chan Date: Wed, 9 Sep 2026 14:24:26 -0700 Subject: [PATCH] hooks: fire the self-match guard on a one-shot check too Reviewed the one-shot `pgrep -f postgres` case, which reads like a legitimate "is it running?" check, and kept it firing. An agent harness runs each tool call as `bash -c ''`, so the pattern is already in a live process cmdline before the search starts. Verified: a bare `pgrep -f` for a token present on no process anywhere printed a pid and exited 0, and `pgrep -af` showed the match was that wrapper. There is no correct plain `-f` spelling under such a harness, so the one-shot answer is wrong too -- just less loudly than a loop that never exits. Narrowing to loop constructs would also miss a one-shot `pkill -f`, which is destructive, and an `if pgrep -f X; then` guard, which silently takes the wrong branch. The block message now explains the wrapper rather than only the loop, so the redirect teaches the actual rule. The fixture carries the reasoning in its docstring so a later reader does not relax it back to silent. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01F43CBUnsDEs6J2zEC1r8a8 --- engine/hooks/gh-write-verification/README.md | 20 +++++++++++-- engine/hooks/gh-write-verification/detect.py | 29 ++++++++++++------- .../gh-write-verification/tests/test_hooks.py | 12 ++++++++ 3 files changed, 49 insertions(+), 12 deletions(-) diff --git a/engine/hooks/gh-write-verification/README.md b/engine/hooks/gh-write-verification/README.md index bf6024b..7e3bc49 100644 --- a/engine/hooks/gh-write-verification/README.md +++ b/engine/hooks/gh-write-verification/README.md @@ -82,14 +82,23 @@ prints `RUNNING` even when nothing by that name exists — it matched the shell asking the question. A wait negated on that (`! pgrep …` as a loop condition) can never exit, and `pkill -f ` kills its own wrapper mid-command. +**This fires on a one-shot check too, deliberately.** An agent harness runs each +tool call as `bash -c ''`, so the pattern is sitting in a live +process cmdline before the search even starts. A plain `pgrep -f postgres` then +returns that wrapper and exits 0 whether or not postgres is running anywhere — +verified against a token present on no process on the box. There is no correct +plain `-f` spelling under such a harness, so a one-shot status check is a true +positive, not a tolerated false one; narrowing the detector to `until`/`while` +loops would also miss a one-shot `pkill -f` and an `if pgrep -f X; then` guard. + The pattern occurring **exactly once** is enough — being the `pgrep` argument *is* the occurrence. A test for "the pattern appears elsewhere in the command" therefore misses the canonical loop, which mentions the name only once. **Fires on:** any `pgrep`/`pkill` with `-f`/`--full` (including `-af`, and with value-taking flags such as `-u ` in front) whose pattern is a plain -literal; and a bracket-class pattern whose plain spelling still appears -somewhere else in the same command. +literal — in a loop, in an `if` guard, or standalone; and a bracket-class +pattern whose plain spelling still appears somewhere else in the same command. **Stays silent on:** the bracket idiom on its own (`pgrep -f '[r]un_all_tests'`); a name match with no `-f` (`pgrep run_all_tests.sh`, `pgrep -x bash`) — the @@ -110,6 +119,13 @@ Prior art: no formal citation found. The named folk pattern is the classic `ps aux | grep foo` self-match and its `[f]oo` bracket idiom; the repro above is the evidence of record. +**Sibling defect, in this repo's own hooks:** a scanner that reads data as code. +`wait-needs-wakeup` blocks on `until`/`sleep` appearing anywhere in a payload, +including inside test *strings* handed to a detector rather than commands being +run; `no-comments` had the same shape until triple-quoted strings were excluded +from its scan. This detector is exposed to it as well — its patterns are matched +in raw payload text, so writing about it in an inline heredoc trips it. + ## 4. A merge cannot end the turn unverified (Stop) `gh pr merge` reporting `MERGED` only means the PR closed against **its own diff --git a/engine/hooks/gh-write-verification/detect.py b/engine/hooks/gh-write-verification/detect.py index 7b1b475..b8730a5 100644 --- a/engine/hooks/gh-write-verification/detect.py +++ b/engine/hooks/gh-write-verification/detect.py @@ -26,9 +26,14 @@ 3. SELF-MATCHING PROCESS WAIT (`self_matching_process_waits`). `pgrep -f` and `pkill -f` match full command lines, and the pattern sits in the argv of the - very shell that runs them, so the match is never empty: a wait negated on - `pgrep -f ` can never exit, and `pkill -f ` kills its own - wrapper. The pattern occurring exactly once is enough -- being the pgrep + very shell that runs them, so the match is never empty. An agent harness + that runs each tool call as `bash -c ''` puts the pattern + in a live process cmdline before the search even starts, which makes a + one-shot `pgrep -f ` report present for a service that is not + running anywhere. A wait negated on `pgrep -f ` therefore can never + exit, and `pkill -f ` kills its own wrapper. There is no correct plain + `-f` spelling under such a harness, so the detector does not wait for a loop + before objecting. The pattern occurring exactly once is enough -- being the pgrep argument *is* the occurrence -- so a test for "the pattern appears elsewhere in the command" misses the canonical loop. A bracket character class is the standard workaround, and it holds only while the plain spelling appears @@ -131,13 +136,17 @@ SELF_MATCH_MESSAGE = ( "gh-write-verification: this matches on a process pattern that also matches " "the shell asking the question:\n{hits}\n" - "`pgrep -f` / `pkill -f` compare full command lines, and the pattern sits in " - "this command's own argv, so the match is never empty -- a wait negated on it " - "never exits, and `pkill -f` kills its own wrapper. Wait on something the " - "watched process writes instead (`grep -q '^EXIT=' out.log` in the loop " - "condition), or on a pid you captured (`kill -0 \"$PID\" 2>/dev/null`). A " - "bracket class such as `[r]un_all_tests` works only while the plain spelling " - "appears nowhere else in the same command." + "`pgrep -f` / `pkill -f` compare full command lines, and this command runs " + "inside a wrapper shell whose cmdline carries the whole command text -- so " + "the pattern is already in a live process before the search starts. The " + "match is never empty even outside a loop: a one-shot `pgrep -f ` " + "reports present for a service that is running nowhere, a wait negated on it " + "never exits, and `pkill -f` kills its own wrapper. Match on a pid you " + "captured (`kill -0 \"$PID\" 2>/dev/null`), wait on something the watched " + "process writes (`grep -q '^EXIT=' out.log` in the loop condition), or hide " + "the pattern from the cmdline with a bracket class such as `[p]ostgres` -- " + "which holds only while the plain spelling appears nowhere else in the " + "command." ) diff --git a/engine/hooks/gh-write-verification/tests/test_hooks.py b/engine/hooks/gh-write-verification/tests/test_hooks.py index 75d2b0b..3ffbc42 100644 --- a/engine/hooks/gh-write-verification/tests/test_hooks.py +++ b/engine/hooks/gh-write-verification/tests/test_hooks.py @@ -174,6 +174,18 @@ def test_the_incident_wait_loop_is_flagged(self): def test_a_pattern_occurring_only_once_is_still_flagged(self): self.assertTrue(self_matching_process_waits("pgrep -f wwww_once_only_ghwv")) + def test_a_one_shot_status_check_is_flagged_because_the_wrapper_carries_the_pattern(self): + """`pgrep -f postgres` looks like a legitimate "is it running?" check and is not. + + The harness runs each tool call as `bash -c ''`, so + the pattern is already in a live process cmdline: the search returns the + wrapper and exits 0 for a service running nowhere. Verified against a + token present on no process. Do not relax this to silent -- the one-shot + answer is wrong too, just less loudly than a loop that never exits. + """ + self.assertEqual(self_matching_process_waits("pgrep -f postgres"), ["pgrep -f postgres"]) + self.assertTrue(self_matching_process_waits("if pgrep -f nginx; then echo up; fi")) + def test_destructive_pkill_is_flagged(self): self.assertEqual(self_matching_process_waits("pkill -f run_all_tests.sh"), ["pkill -f run_all_tests.sh"])