2 Commits

Author	SHA1	Message	Date
Sergei Selivanov	3d39cb491a	fix(hooks): harden the read-only classifier; re-arm the nudge for long sessions (#161) ... * fix(hooks): close 19 classifier escapes found by adversarial review Tool-call layer: - Task/Agent no longer blanket-neutral: a spawned subagent can edit files or call write APIs, so only the read-only agent types (Explore, Plan) keep the exemption; every other subagent_type is suspicious - EnterWorktree/ExitWorktree removed from the neutral set (they create and remove git branches/worktrees) - read-verb-named SQL tools scan their serialized input: EXPLAIN ANALYZE executes the statement, so DML keywords or "analyze": true in the payload disable the exemption - "prepare" added to write verbs (prepare_query_tuning provisions a branch); "resolve" dropped from read verbs (resolving a review thread mutates it) Shell scanner: - double-quote escaping now uses backslash parity: "a\\" closes the quote (the backslashes escape each other), so a following command is seen Command classifiers: - python -c: .execute()/.executemany(), Path.open('w'), aliased os imports (import os as x / from os import ...), case-insensitive SQL and pattern matching (lowercase "delete from" caught) - curl: --config/-K (config can carry any request), -Q/--quote (FTP protocol commands like DELE), Q/K in short-flag clusters - git: --upload-pack/--receive-pack/--exec inject remotely-executed commands even under read-only subcommands like ls-remote - date: BSD/macOS positional operand sets the clock (+format and -j forms stay read-only) - awk: redirects/pipes to variables (print > f, print | c), close(), getline; numeric comparisons ($3 > 100) stay clean - sed: w-detection works with any substitution delimiter (s#a#b#w out) - xxd/uniq: a bare "-" stdin marker counts as positional, so the output file argument is no longer missed - less/more: -o/-O/--log-file copy input into a file Twenty new transcript tests pin each escape and the read-only forms. * feat(hooks): re-arm the capture nudge for long-running sessions "Once per session" starves multi-day sessions: the single nudge fires early, and everything after it is never captured automatically. The sentinel now expires when BOTH hold: it is older than WIKI_STOP_REARM_SECONDS (default 6h) and at least WIKI_STOP_REARM_EDITS (default 10) new file edits happened since the nudge that claimed it. Short sessions keep exactly one nudge; long busy sessions get roughly one per work stretch; long idle sessions stay silent. The edit count at nudge time is stored inside the sentinel directory; sentinels predating the feature have no count and re-arm from zero. The young-sentinel path stays the cheap pre-parse short-circuit, and an expired sentinel is retired with an atomic mv so concurrent invocations (duplicate hook registration) still produce exactly one re-nudge. * fix(hooks): close a re-arm double-nudge race under duplicate registration With the hook registered twice, both invocations can pass the sentinel age check; the winner retires the expired sentinel, nudges, and claims a fresh one — and the loser's mv then grabbed that FRESH sentinel and nudged again. After winning the mv, the invocation now verifies the retired sentinel really is expired: a young one means a concurrent re-arm already nudged, so it is moved back intact (count preserved) and the invocation stands down. If the restore itself loses to yet another claim, the grabbed copy is discarded and the newest claim wins. A stress test runs 10 rounds of two concurrent invocations against an expired sentinel and asserts exactly one nudge and a surviving sentinel every round. * fix(hooks): repair edit-count state when restoring a stolen fresh sentinel The double-nudge guard restores a fresh sentinel it grabbed by mistake — but if it grabbed it in the gap between the winner's mkdir and its count write, the restored sentinel has no edits file, and the next re-arm would compute its delta from zero and fire early. Both invocations count the same stop's transcript, so the restorer's own count is the correct stand-in: after a successful restore, write it if the edits file is missing. The stress test now also asserts the surviving sentinel holds the correct count every round, in every interleaving. * test(hooks): make the re-arm race test actually concurrent The stress test launched two hook processes but fed stdin through sequential communicate() calls — and the hook's first statement is INPUT=$(cat), so the second process blocked at that read until the first had fully exited. The bodies ran one after another and the race path was never exercised; the test only re-proved the sequential young-sentinel path. Both processes now get stdin from a pre-written payload file and execute the full body simultaneously, with a 200-entry transcript so the parse phase gives a wide overlap window between the age check and the claim. Verified with bash -x tracing: both processes reach the claim phase in every round (one wins the mv or mkdir, the other loses it or restores), with exactly one nudge and intact edit-count state throughout. * test(hooks): assert the race path actually executes, not just its outcome The outcome invariants (one nudge, surviving sentinel, intact count) hold in a serialized interleaving too, so the concurrent test could pass without ever exercising the race. The hook now runs under bash -x and the test inspects each process's xtrace: a process that reached the claim phase shows an mv or mkdir on the sentinel path, while a serialized loser exits at the top age check without touching it. A round counts as raced only when BOTH processes reached the claim phase; at least 3 of 10 rounds must race (unloaded machines hit 10/10, the slack tolerates loaded-CI serialization) or the test fails loudly, flagging that the harness degraded back to sequential execution. * fix(hooks): fix BSD-first stat fallback returning wrong mtime on Linux stat -f %m FILE || stat -c %Y FILE assumed the BSD form fails clean on GNU/Linux. It doesn't: GNU stat's -f takes no format argument, so %m is parsed as a second FILE operand. The command errors on that bogus operand but still prints real filesystem info for the sentinel to stdout, and exits nonzero overall — so `||` also runs stat -c, and $(...) concatenates both outputs into one non-numeric string. The ^[0-9]+$ guard rejects that and silently falls back to $NOW, so the sentinel always looks freshly-created and the age check never passes. Flip the order: GNU stat -c is an invalid option on BSD/macOS and fails with no stdout, so trying it first is safe on both platforms. Same pattern existed twice (CLAIMED_AT, RET_AT). Reproduced on a real Linux box: 4 of the PR's own re-arm tests (test_rearm_after_age_and_new_edits, test_rearm_updates_stored_count_and_rests_again, test_pre_feature_sentinel_rearms_from_zero, test_concurrent_rearm_produces_exactly_one_nudge) fail there and pass after this fix; all 74 tests pass on macOS both before and after. --------- Co-authored-by: ar9av <ar9avg@gmail.com>	2026-08-04 12:25:08 -07:00
Sergei Selivanov	34277d3c2c	fix(hooks): exempt read-only sessions from the Stop-hook capture nudge (#158) ... * fix(hooks): exempt read-only sessions from the Stop-hook capture nudge The Stop hook fired after pure research sessions (0 file edits, N shell commands that only read state — gh/git log/grep/cat), forcing a full wiki-capture skill invocation that predictably ends in SKIP. Shell-command count alone is a poor signal of capture-worthy work. The transcript scan now classifies each Bash command read-only vs mutating (quote-aware pipeline splitting, redirect detection outside quotes, git/gh/sed/python/curl subcommand and flag awareness; unknown commands count as mutating). The nudge still fires on any Write/Edit/NotebookEdit, and on >= 4 shell calls when at least one mutated state — but an edit-free session whose shell activity is entirely read-only exits silently, acting as a cheap pre-gate so a SKIP-destined session never costs a skill turn. Behavioral tests run the hook against synthetic transcripts and pin the exit-code contract, the classifier edges, and the existing sentinel and stop_hook_active guards. * fix(hooks): close read-only classifier gaps that hid mutating commands Review of the exemption surfaced commands the classifier wrongly proved read-only. Close them: - command/process substitution: $(...), backticks, <(...) run arbitrary inner commands (also inside double quotes) — now always mutating; single-quoted occurrences stay literal - find with -delete/-exec/-execdir/-ok/-fls/-fprint* mutates - curl: glued -XPOST, --request, --json, --form-string now detected - sort -o / --output writes a file; uniq with two positional args too - env/nice/stdbuf are wrappers that run their argument command — unwrap them (bare env stays read-only); date -s and sysctl -w mutate - python -c payloads are scanned for write/exec patterns (os.remove, shutil, subprocess, open(...,w), .write(), writing HTTP verbs) - awk programs with internal redirection or system() mutate Tokenization now uses shlex (with a naive fallback), and the classifier moved from an inline $(python3 <<heredoc) into a temp file: bash 3.2 (macOS /bin/bash) scans a $() body naively and chokes on backticks or unbalanced quotes in the embedded program text. Eleven new transcript tests pin both directions of each case. * fix(hooks): harden the classifier against remaining read-only escapes A second adversarial pass found more commands the classifier wrongly proved read-only, plus one common false positive: - python -c payloads using exec()/eval()/__import__, open(mode="w"), pandas/PIL-style .save()/.to_csv(), sqlite writes (INSERT/DROP/commit), and urllib Request(data=...) now count as mutating - the sed `w` script command writes files without -i: `sed -n 'w out'`, `s/a/b/w out` now detected - `git diff/log --output=file` writes without a shell redirect - curl short-flag clusters (-sd, -sLo, -sT, -sD, glued -X) hide data/output flags from exact-match checks — any cluster containing d/D/o/O/T/F/X now counts as mutating - shell-session state commands (cd, pushd, export, set, read, man, ...) are read-only: they mutate nothing durable, so research chains like `cd repo && git log` no longer lose the exemption Eight new transcript tests pin both directions of each case. * fix(hooks): exemption must consider non-Bash tools and remaining allowlist holes The read-only exemption only inspected Bash commands, so a session that mutated a real system through an MCP tool (a CRM update, an SQL INSERT) while its shell activity stayed read-only was wrongly exempted — a nudge the pre-exemption hook would have delivered. Non-Bash tool calls now feed the decision: core harness read tools (Read/Grep/WebFetch/planning/task bookkeeping) stay neutral; MCP tools are classified by name verbs (write verbs beat read verbs, unknown counts as suspicious), and any unknown harness tool counts as suspicious. Suspicious calls never trigger a nudge on their own — they only disable the exemption, so the hook still never fires where the original threshold (edits >= 1 or bash >= 4) wouldn't have. Also closes remaining bash allowlist holes: hostname with an argument sets the hostname; fd -x/--exec runs commands; tree -o and a second xxd positional write files; rg --pre executes a preprocessor. Nine new transcript tests pin the MCP/unknown-tool matrix and each flag. * fix(hooks): correct single-quote scanning and remaining mutating flag forms The quote scanner treated a backslash before a closing single quote as an escape, but the shell allows no escaping inside single quotes — so constructs like: echo 'a\' ; rm x' kept the rm inside a "quoted" region and classified read-only. Single quotes now always close; only double quotes honor a backslash. More mutating forms detected: - git reflog expire / reflog delete (bare reflog and reflog show read) - curl cookie/header/trace writers: -c clusters, --cookie-jar, --dump-header, --trace*, --etag-save - awk programs piping to a command or reading via getline pipe - sed w-script glued into -e's/a/b/w out' (w-scan covers all tokens) - history -c/-d/-w/-a/-r mutate history state (bare history reads) - python -c payloads touching smtplib/ftplib/paramiko/os.kill Eight new transcript tests pin each case in both directions.	2026-08-03 09:01:18 -07:00