hooks: fire the self-match guard on a one-shot check too - #324
Merged
Conversation
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 '<the whole command>'`, 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F43CBUnsDEs6J2zEC1r8a8
Bugbot couldn't run - usage limit reachedBugbot 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_f08fda23-bc43-440f-9ea3-c22cdf946389) |
Contributor
|
Queued — the merge queue status continues in this comment ↓. |
Owner
Author
|
@Mergifyio queue |
Contributor
Merge Queue Status
This pull request spent 4 minutes 22 seconds in the queue, including 3 minutes 20 seconds running CI. Required conditions to merge
|
5 tasks
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #323. That PR's detector stayed silent on a one-shot
pgrep -f postgres, on the assumption it is a legitimate "is it running?" check.Tested the assumption and it is wrong. 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.
A bare
pgrep -ffor a token present on no process returned a pid and exit 0.There is no correct plain
-fspelling under such a harness. The one-shot answer is wrong too — just less loudly than a loop that never exits.This makes the detector fire on the standalone and
if-guarded forms as well, and rewrites the block message to explain the wrapper rather than only the loop.Review Claim
Widening detector 3 from loop-shaped self-matches to every plain-literal
-fpattern is correct rather than noisy, because under this harness the one-shot form is also a wrong answer — a true positive, not a tolerated false one.Review Lane
behavior
Review Unit
engine-runtime
Safety Invariant
Behaviour change is strictly additive to what
self_matching_process_waitsalready reported: no case that was silent for a good reason becomes noisy. The silent set is unchanged and still covered by fixtures — 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. The detector remains pure text analysis with no process lookup, no subprocess, and no file read, so widening it cannot make the hook itself slower or fallible at runtime. It stays insidepretooluse_problems(), whose only effect is stderr plus exit 2, and a detector exception is still trapped and reported as "allowing".Assumptions:this Safety Invariant is unconfirmed — drafted under a non-interactive delegation with no user available to confirm it, perdraft-pr's headless mode.Slice Rationale
Separate PR because #323 was merged (squashed onto
mainas6c7112b) while this commit was still local, so it could not ride along. Same three files, no new ones; the diff is the widening plus the fixture and README paragraph that justify it.Non-goals
wait-needs-wakeup, which is what pushes agents into these loops.Test Plan
Test Plan
Repro before:
pgrep -f zzz_unique_token_ghwv_9931, a token on no process anywhere → printed pid1766632,exit=0pgrep -afon the same token → the single match was/bin/bash -c source …— the harness's own wrapper carrying the command textmain's detector (from hooks: refuse a process wait that matches its own command line #323) onpgrep -f postgres→ silent, which is the gap this closesRepro after — the five-probe set:
pkillself-match → firespgrep -f postgres→ fires (was silent; this is the change)Also fires:
if pgrep -f nginx; then echo up; fi. Also still silent:pgrep run_all_tests.sh,pgrep -x bash,pkill -x node,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, OKbash scripts/run_all_tests.sh— 37 suites,EXIT6=0, noFAILED/ERROR:linespython3 scripts/check_hook_test_coverage.py engine/hooks/gh-write-verification— OKpython3 engine/skills/make-pr/scripts/preflight.py --base origin/main—unit engine-runtime: 3 file(s),ok preflight passedpython3 scripts/check_no_new_comments.py --base origin/main—ok no new commentspython3 scripts/check_no_dated_provenance.py --base origin/main—ok no dated provenanceruff check . --select E9,F— All checks passed;shellcheck install.sh— cleannode engine/skills/draft-pr/scripts/lint-diff-atomicity.mjs --base origin/main --review-lane behavior— passed, no warningsRevert Plan
Revert Plan
git revert <sha>settings.jsonentries are unchanged.🤖 Generated with Claude Code
https://claude.ai/code/session_01F43CBUnsDEs6J2zEC1r8a8
Note
Low Risk
Additive PreToolUse guidance and tests for gh-write-verification; no change to silent allowances and no runtime subprocess or I/O in the detector.
Overview
Clarifies and locks in the
self_matching_process_waitsguard so standalone andif-guardedpgrep -f/pkill -fwith plain literals are treated as violations, not only loop-shaped waits.The block message and README now explain the agent harness (
bash -c '<command>'): the pattern is already on the wrapper’s cmdline, so a one-shot “is postgres running?” check can exit 0 even when the service is absent. Docs also note a sibling false-positive risk when detectors scan raw payload text (e.g. heredocs in tests).New unit tests assert
pgrep -f postgresandif pgrep -f nginx; then …are flagged. The silent set (bracket idiom, no-f, variables,kill -0, log sentinels) is unchanged per existing fixtures.Reviewed by Cursor Bugbot for commit 45f2e49. Bugbot is set up for automated code reviews on this repo. Configure here.