[sonichi]

Summary

skills/proactive-loop/SKILL.md step 1 stages a grouped [deduped: X] reply under
state/dedup-staging/<file> and is only supposed to promote it via:

S="$WORKSPACE/state/dedup-staging/<file>"
python3 skills/proactive-loop/scripts/check-dedup-targets.py "$S" && mv -f "$S" "$WORKSPACE/results/<file>"

check-dedup-targets.py refuses (exit 1) a staged result whose [deduped: X] target
resolves to nothing — X's own result is [no-send] or absent — because that would
otherwise have the bridge tell the room "see task X" for an X that delivers nothing
(the exact incident measured 2026-09-01, three collaborator notices, a 519-task sweep
hunting an id that never existed on the peer's side).

That protection only holds if the && chain is actually typed. A bare mv moving a
dedup-staged file into results/, run from any skill or any live session with the
chain dropped, bypasses it completely. This is the same architectural gap
gh-policy-gate.py (#4268) and memory-index-guard.py (#4271, merged as 59c54d2)
already closed for their own chained checks, and the same fix: move enforcement from
"a step I remember to chain" to the action itself.

hooks/dedup-staging-guard.py gates any Bash mv (any caller) that lands a
dedup-staged file into results/ on check-dedup-targets.py actually having been
invoked somewhere in the same command.

Design tradeoff: not tracking shell variables

A PreToolUse hook sees tool_input.command as the RAW, unexpanded shell text — $S is
a literal variable reference, not the path it resolves to. This hook does not
attempt real shell-variable-substitution tracking (out of scope: fragile, and unlike a
gh/Edit/Write call, a Bash command carries no resolved value the hook can read
directly).

Instead it matches literal substrings, the way the skill's own prose documents the
pattern (state/dedup-staging/ → results/):

1. an mv invocation (tokenized via the shared hooks/_shell_scan.py, same scanner
   gh-policy-gate.py uses) whose own segment's arguments mention results — the
   destination is always written out literally in the real pattern, even when the
   source is hidden behind $S;
2. dedup-staging mentioned anywhere in the whole command — usually the earlier
   S=... assignment, ;-separated from the mv;
3. no segment anywhere in the same command invokes check-dedup-targets.py.

I verified directly (not assumed) that _shell_scan.segments() splits && into a
separate segment from what precedes it:

>>> segments('S="..."; python3 check-dedup-targets.py "$S" && mv -f "$S" "$WORKSPACE/results/x.txt"')
[['S=...'], ['python3', 'check-dedup-targets.py', '$S'], ['mv', '-f', '$S', '$WORKSPACE/results/x.txt']]

So the checker call and the mv it gates land in different segments in the
canonical invocation — unlike gh-policy-gate.py, where the flag it checks and the gh
subcommand share one segment. Because of that, checks 2 and 3 above are scanned across
the whole command rather than per-segment; only the results match (1) stays
segment-scoped to the mv itself, so an unrelated mv elsewhere in the command can't
combine with an unrelated dedup-staging mention to false-positive.

Fails open on anything ambiguous: a command mentioning only one of dedup-staging /
results, or no mv at all, is allowed. SUTANDO_ALLOW_UNGATED_DEDUP_STAGING=1 is the
one-shot override, same shape as the other two hooks.

Not covered: a staging→promotion sequence split across multiple Bash tool calls
(S=... in one call, the check+mv in a following one) — this hook only sees one
command string per invocation, the same scope limit gh-policy-gate.py and
memory-index-guard.py accept implicitly.

Test plan

tests/dedup-staging-guard.test.py — 21/21 green:

• Unit tests for _mv_segments (including the direct proof that && splits the
  checker call from the mv into separate segments) and check_dedup_staging_bypass
  (safe chained command allows, variable-hidden bypass denies, fully-literal bypass
  denies, unrelated mv / partial-mention cases fail open, an unrelated mv in one
  segment doesn't mask detection of the real bypass in another).
• EndToEnd subprocess tests: non-Bash tool ignored, non-matching command ignored,
  safe chained command allowed, bypass denied with the hook's own message content,
  override env var bypasses, malformed stdin doesn't crash.

$ python3 tests/dedup-staging-guard.test.py -v
...
Ran 21 tests in 0.148s
OK

Regression test proved, not just asserted: I disabled the gate logic
(check_dedup_staging_bypass short-circuited to always return None, simulating a
revert) and reran the suite — 4 tests genuinely failed (both bypass-detection unit
tests, the cross-segment-non-masking test, and the end-to-end deny test), then
restored the real file and confirmed 21/21 green again with a clean diff against the
backup. Also ran python3 tests/proactive-loop-check-dedup-targets.test.py (the
underlying checker's own suite, 21/21) to confirm no interference.

bash scripts/review-checks.sh --diff <this diff> (run from inside this worktree, off
origin/main): review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean).

Not in this PR

• Deployment (copying into $CLAUDE_CONFIG_DIR/hooks/ and registering under
  settings.json's PreToolUse → Bash matcher) — source under review only, same as
  feat(hooks): gate gh issue create / gh pr comment for any Bash caller #4268 and feat(hooks): gate Edit/Write to MEMORY.md for any caller (step 7.5) #4271 before it merged.
• Real shell-variable-substitution tracking — explicitly out of scope, see tradeoff
  section above.

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 90%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

⚠️ Not measured

Changed, but absent from coverage.xml — outside [run] source in
.coveragerc, so diff-cover never examined these lines:

• hooks/dedup-staging-guard.py

[yixuan-ag2]

Approving. I verified the guard by running the shipped hook as a subprocess against real payloads rather than reading it — both halves, since a deny-hook that never denies and one that denies everything look identical from the code.

case                                                      expect   got
POSITIVE: bare bypass (S=...; mv -f "$S" .../results/…)   deny     deny    OK
POSITIVE: bypass, literal paths, no variables             deny     deny    OK
NEGATIVE: the prescribed check-dedup-targets && mv chain  allow    allow   OK
NEGATIVE: ordinary result publish, no dedup-staging       allow    allow   OK
NEGATIVE: no mv at all                                    allow    allow   OK

Run with SUTANDO_ALLOW_UNGATED_DEDUP_STAGING cleared from the environment, so the allows are real allows and not the override failing open. tests/dedup-staging-guard.test.py is 21/21 green, and pr-checks.py reports rc 0 — 16 distinct checks from 16 entries, 0 superseded, nothing failing or running.

One thing I flagged and then withdrew, since another reviewer may raise it: the PR adds no .claude/settings.json registration, and an unregistered hook is inert while looking exactly like a working one. That is not actionable here — .claude/ is gitignored and untracked on main (the path 404s), and the merged sibling #4271 (59c54d291) has the identical three-file shape. Registration is a host-side install step and cannot live in this PR. Worth stating so the next reviewer does not spend the same probe.

Two non-blocking observations, neither gating:

1. cp reaches the same end state and is ungated. _mv_segments keys on basename_is("mv"), so cp state/dedup-staging/x.txt results/x.txt is allowed and publishes the same unchecked body — the staging file merely survives. I ran it: allowed. The stated aim is moving enforcement "to the action itself", and copy-then-claim is that action too. A one-word change to the basename test would cover it. I would not hold the PR for this — the skill prescribes mv -f and the sibling hooks accept the same boundary — but it is the shape a future bypass takes.

2. A narrow false positive. Because dedup-staging is matched across the whole command while only the mv segment needs to mention results, a command that references the staging dir for an unrelated reason and separately moves a file within results/ is denied:

ls workspace/state/dedup-staging/; mv -f workspace/results/old.txt workspace/results/new.txt   -> denied

Low-frequency, and the deny message names the override, so the cost is one retry rather than a wedge. Mentioning it only so the widening in (1) is not made without noticing this direction gets wider too.

The docstring is unusually good about its own limits — the raw-unexpanded-$S constraint, fail-open-on-uncertainty, and the single-command scope are all stated rather than discovered by the reviewer. That is what made this cheap to check.

Bar is 0 of 2; this takes it to 1. It needs a second counting approval from one of bassilkhilo-ag2, john-the-dev, jsun-m, liususan091219, qingyun-wu, sonichi.

[bassilkhilo-ag2]

Approving. Independently verified rather than trusting Yixuan's already-thorough review — reran the same ground, plus my own mutation test.

Ran the real 21/21 suite at this head, genuinely green.

Ran the hook as a subprocess myself against 6 cases (not reading the code and assuming): the bare-variable bypass and the fully-literal bypass both DENY; the prescribed check-dedup-targets.py && mv chain, an ordinary results publish, Yixuan's cp-instead-of-mv finding, and her false-positive case all behaved exactly as she reported — cp is genuinely ungated, and the false positive (dedup-staging mentioned for an unrelated reason + an unrelated results/-to-results/ mv) genuinely denies. Both her non-blocking findings check out.

Mutation-tested the gate myself: short-circuited check_dedup_staging_bypass to return None unconditionally (not the same mutation as the PR's own described one, but the most direct kill of the whole function). 4 tests failed — same count the PR's own regression-test section claims. Restored, git diff --stat clean.

Design tradeoffs (no real shell-variable tracking, single-command scope, fail-open-on-ambiguity) are explicitly documented and match #4268/#4271's precedent, and the .claude/settings.json non-registration is correctly out of scope for this PR (source-only, same as those two).

No blocking findings. This is my genuine second approval, not a formality — checked the collaborator-permission endpoint directly (gh api repos/sonichi/sutando/collaborators/bassilkhilo-ag2/permission returns write, and my prior review on #3317 shows author_association: COLLABORATOR), so this should clear the 2-approval bar.

[john-the-dev]

Approving at 849a774f30b3. I ran the hook rather than reading it, in both directions, and it does what the body claims. Two findings below, neither blocking — one is a real weakening of the stated guarantee with a cheap fix, the other is narrower than I first thought and I am reporting the check that shrank it.

What I verified by execution

Loaded dedup-staging-guard.py + _shell_scan.py at this head and called check_dedup_staging_bypass() directly. Six cases, both polarities:

DENY   the bug, variable source: S=".../state/dedup-staging/t.txt"; mv -f "$S" ".../results/t.txt"
DENY   the bug, fully literal paths
ALLOW  the canonical chain (check-dedup-targets.py "$S" && mv -f "$S" ".../results/t.txt")
ALLOW  an ordinary atomic result write: tmp=$(mktemp); ...; mv -f "$tmp" ".../results/task-cron-x.txt"
ALLOW  no mv at all
ALLOW  dedup-staging mentioned, mv unrelated to results/

The fourth matters to me specifically: that is the atomic result write this loop performs on every pass, and a false deny there would have been a live-path regression. It allows.

Repo checks at this head: hardcoded-paths clean, root-artifacts clean (positive control — a planted /Users/ruiwang/... line makes the scanner exit 1, so the clean result is real, not an inert scan). prose-cap SKIPPED in my checkout ("no post-image"), so I ran its logic myself against the fetched post-image: dedup-staging-guard.py longest # block = 1 line, dedup-staging-guard.test.py = 1 line, cap 2 — within cap. I specifically checked whether the ~50-line module docstring counts, since the same gate blocked #4306 for a 4-line comment: it does not, because review-checks-prose-cap.py classifies via tokenize and only counts COMMENT tokens, and a docstring is a STRING token. No inconsistency between the two PRs.

Finding 1 (non-blocking, cheap): the docstring says invokes, the code says mentions

Condition (c) is documented as "NO segment anywhere in the same command invokes check-dedup-targets.py". The implementation is a substring test over every word:

if any(_CHECKER_MARKER in w for w in all_words):
    return None

So merely naming the checker satisfies it. Measured:

S=".../state/dedup-staging/t.txt"; echo "TODO: check-dedup-targets.py"; mv -f "$S" ".../results/t.txt"
-> ALLOW        (the mv runs; the checker never does)

A # check-dedup-targets.py note, a TODO, or a log line naming the script in the same command silently disables the guard for that command. I do not think this is an evasion concern — the threat model is a forgotten chain, and a forgetful caller will not echo the script name — but the guarantee as written is stronger than the guarantee as implemented, and the machinery to close it is already in this file: _mv_segments() uses w.basename_is("mv"), and the same test against the checker's own segment would mean invoked rather than mentioned. Worth either tightening, or softening the docstring to say "mentions".

Finding 2 (nit, and smaller than it first looked): heredoc bodies tokenize as words

_shell_scan.words() yields heredoc body text as ordinary words — control:

words('cat <<EOF\nHEREDOC_SENTINEL_WORD\nEOF')
-> ['cat', '<<EOF', 'HEREDOC_SENTINEL_WORD', 'EOF']

so documentation text can be read as an invocation. A pure cat >> notes.md that writes the bypass pattern is denied even though the only command word is cat:

cat >> notes.md <<EOF
step 1 stages under state/dedup-staging/ then: mv -f "$S" "$WORKSPACE/results/<file>"
EOF
-> DENY

I went looking for a real in-repo instance and did not find one, which is why this is a nit rather than a blocker. I tried this PR's own hooks/README.md opening, expecting the hook to block its own documentation. It allows, for two independent reasons: the markdown backticks around `mv` stop basename_is("mv") matching, and the full README names check-dedup-targets.py anyway. So the false positive needs doc text that writes the mv pattern unbacktick-quoted (a fenced block or plain notes), mentions dedup-staging, and never names the checker. Narrow. The SUTANDO_ALLOW_UNGATED_DEDUP_STAGING=1 override covers it if anyone hits it.

Worst-case disruption

Always-on, every Bash mv, so false positives are the disruptive direction. The realistic false-positive set is the Finding 2 shape above, it has a documented one-shot override, and the hook fails open on every ambiguity I could construct. The two genuine bypass shapes are both denied. Net: strictly better than the status quo, which is no enforcement at all.

Scope of this approval: the hook's behavior, the scanner interaction, and the repo checks above. I did not exercise the PreToolUse wiring end-to-end inside a live session — I called the decision function directly.