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"])