Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions engine/hooks/gh-write-verification/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 <name>` 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 '<the whole command>'`, 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 <user>` 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
Expand All @@ -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
Expand Down
29 changes: 19 additions & 10 deletions engine/hooks/gh-write-verification/detect.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 <name>` can never exit, and `pkill -f <name>` 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 '<the whole command>'` puts the pattern
in a live process cmdline before the search even starts, which makes a
one-shot `pgrep -f <service>` report present for a service that is not
running anywhere. A wait negated on `pgrep -f <name>` therefore can never
exit, and `pkill -f <name>` 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
Expand Down Expand Up @@ -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 <service>` "
"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."
)


Expand Down
12 changes: 12 additions & 0 deletions engine/hooks/gh-write-verification/tests/test_hooks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 '<the whole command>'`, 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"])

Expand Down
Loading