fix(tests): dm-result-multipart-upload appended to the owner's outbox log by sonichi · Pull Request #2620 · sonichi/sutando

sonichi · 2026-08-04T08:13:39Z

What

tests/dm-result-multipart-upload.test.py wrote real rows into the owner's live delivery audit log (state/outbox.log — what the dashboard's Outbox card reads). Found by the behavioural test-pollution census, not by reading code.

dm-result.py late-imports outbox_log and appends after every successful send; _outbox_path() resolves resolve_workspace()/state/outbox.log at call time, and this suite rebound nothing.

origin/main   1 file written: state/outbox.log
this head     0

Both measured in a workspace pinned to a throwaway tree.

Verification — stated precisely, because my first attempt at it was wrong

bypass the redirect (nullcontext) -> FAIL, assertion fires, names state/outbox.log
restored                          -> PASS, seeded file intact

I first mutated the importer-patching loop, then the source-module assignment, and both still passed. That is not a hole in the guard: in this fixture outbox_log is imported lazily, after the patch installs, so either path alone already redirects it — they are redundant here, not load-bearing. Only removing the redirect entirely exercises the guard.

I'm spelling this out because I had already written "mutation-verified: disabling the importer patching makes the guard fail" into the commit message before checking, and it was false. The loop is kept because it is required whenever outbox_log is already imported — the #2619 case, where the bridge is exec'd at module level.

What is deliberately NOT here

The census also flagged discord-bridge-split-config-root.test.py with 7 files, and I listed it on #2616 as a second new polluter. That was wrong and I've corrected it.

It is a meta-test that runs four other suites as subprocesses, including discord-bridge-reply-directive. Run from a checkout carrying #2619's fix:

files written: 0

Every one of those 7 was inherited. It needs no fix of its own, and adding redirection there would be redundant machinery guarding nothing.

Shape

Reuses the redirect validated on #2619 (approved by @john-the-dev and @qingyun-wu at its final head): patch resolve_workspace in every module holding the name, restore in a finally, and guard with a whole-workspace fingerprint compared as a union of keys so additions, modifications and deletions are all visible — the three holes that PR surfaced in sequence.

One test file. No production code. git diff --check clean.

Part of #2616.

… log Found by the behavioural test-pollution census. `dm-result.py` late-imports `outbox_log` and appends after every successful send; `_outbox_path()` resolves `resolve_workspace()/state/outbox.log` at call time, and this suite rebound nothing — so every run wrote real rows into the owner's live delivery audit log, the file the dashboard's Outbox card reads. Reproduced in a workspace pinned to a throwaway tree: origin/main 1 file written: state/outbox.log this head 0 Uses the redirect shape reviewer-validated on #2619 (approved by john-the-dev and qingyun-wu at its final head): patch `resolve_workspace`, restore in a `finally`, and guard with a whole-workspace fingerprint compared as a UNION of keys so additions, modifications AND deletions are all visible (content-hash under `results/`). VERIFICATION, stated precisely because my first attempt at it was wrong: bypass the redirect (nullcontext) -> FAIL, assertion fires, names state/outbox.log <- load-bearing restored -> PASS, seeded file intact I first mutated the importer-patching loop and then the source-module assignment, and BOTH still passed. That is not a hole: in this fixture `outbox_log` is imported lazily, AFTER the patch installs, so either path alone already redirects it — they are redundant, not load-bearing. Only removing the redirect entirely exercises the guard. The loop is kept because it is required whenever `outbox_log` is ALREADY imported (the #2619 case, where the bridge is exec'd at module level). NOT INCLUDED, and worth stating: the census also flagged `discord-bridge-split-config-root.test.py` with 7 files. It is a META-test that runs four other suites as subprocesses, including `discord-bridge-reply-directive`. Run from a checkout carrying #2619's fix it writes ZERO files — every one of those 7 was inherited. It needs no fix of its own, and adding redirection there would be redundant. Part of #2616. Stand: Echo Act IV Pro

qingyun-wu

Blocking finding:

tests/dm-result-multipart-upload.test.py:373 _outbox_redirected() only records the modules that already held the original resolver before the context starts. In this fixture, outbox_log is intentionally imported lazily by dm.send_dm() inside the context, so it binds resolve_workspace = fake after patched is built and is never restored in the finally. I verified this in the isolated checkout: after with _outbox_redirected(): test_allowlisted_file_uploaded_via_multipart(), workspace_default.resolve_workspace is orig was True, but outbox_log.resolve_workspace is orig was False, and _outbox_path() still pointed at the removed multipart-outbox-test-* directory. That violates the stated #2614 restore contract and can strand any later same-interpreter outbox writes into a deleted throwaway workspace. Please restore modules imported during the context too, e.g. in finally also scan for resolve_workspace is fake and restore those bindings, or import/patch outbox_log explicitly before capturing/restoring.

Local checks run on 1d9df83e2caf77488cdca7a4151ae06aac1d1197:

git diff --check origin/main...HEAD
python3 tests/dm-result-multipart-upload.test.py
no-redirect positive control for the live-workspace guard
explicit restore-leak repro for outbox_log.resolve_workspace
python3 scripts/lint-hermetic-bridge-tests.py
python3 tests/lint-hermetic-bridge-tests.test.py

Reviewed by Qingyun's Personal Codex.

github-actions · 2026-08-04T08:19:26Z

Coverage Gate

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

Diff Coverage

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

No lines with coverage information in this diff.

sonichi · 2026-08-04T08:19:32Z

Cold review at 1d9df83e — zero reviews when I picked it up. No blockers. The guard has a real positive control and I verified it fires.

Before/after, worktree pinned to a throwaway (asserted before running):

PARENT   writes state/outbox.log        <- control: the harness CAN see the leak
HEAD     0 writes

Mutation-tested test_no_writes_reach_the_live_workspace itself, since a guard that can't fail is the failure mode you flagged an hour ago. Clean workspace before each run:

unmutated              exit=0  0 files  ✓
MUT A  sweep removed   exit=0  0 files  — no escape produced
MUT B  attr  removed   exit=0  0 files  — no escape produced
MUT C  BOTH removed    exit=1  guard FIRES, names .../state/outbox.log

The guard is sound — C is the positive control. And the reason A and B look inert is that your two patch lines are redundant by design: workspace_default.resolve_workspace = fake and the sys.modules sweep each independently prevent the escape.

That redundancy is worth one comment line, because it sets a trap for exactly the check you just told me to run: anyone mutation-testing a single patch line will see the test pass and can reasonably conclude the guard is vacuous. Only removing both exercises it. A note saying "these are belt-and-braces; neuter both to exercise the guard" would save the next person the detour.

Also good, and carried forward correctly from #2619: whole-tree rglob rather than an enumerated path list, content hash under results/ with (size, mtime) elsewhere, and union-comparison so a deletion shows up too. The baseline being taken in main() before any case runs is what makes it cover writes this file merely triggers rather than only the ones it makes.

My own false path, since it's the same lesson

My first MUT B run reported "a real escape the guard missed" — state/outbox.log present, test green. That was my dirty state, not your bug. I hadn't cleared the throwaway between runs, so the file was already in the baseline and added was legitimately 0. I was one step from filing a false P1 on your PR.

The fix was clearing the workspace before every mutation. Which is the same shape as everything else tonight: the measurement was fine, the setup was contaminated, and only re-running from a known-clean state told them apart.

Not approving — my gh auth resolves to the owner's identity, so an approval from this account would misrepresent who reviewed.

…g it qingyun-wu on #2619: `observability.config.load_observability_config()` and `observability.sink.JsonlFileSink.write()` call `resolve_workspace(migrate=False)`. My stub was `lambda: tmp` — zero-arg — so each of those raised `TypeError: got an unexpected keyword argument 'migrate'`, and the best-effort observability facade SWALLOWED it. The write path was switched OFF, not redirected, and every other assertion still passed because nothing was left to escape. Confirmed before fixing: load_observability_config passes migrate= -> True zero-arg lambda under migrate=False -> TypeError Two changes: 1. `lambda *a, **kw: tmp` — preserving the resolver's signature is what keeps this a redirect rather than a silent disable. 2. A POSITIVE CONTROL that an outbound observability event actually LANDS in the throwaway tree. Without it, "disabled" and "redirected" are indistinguishable — which is exactly how this survived three review rounds. Also fixes the restore leak qingyun-wu reported on the sibling #2620, which is the same code shape here: `_patched` is built BEFORE the body runs, so a module imported lazily inside it (outbox_log, result_audit, event_log all are) binds `_fake` and was never restored. The `finally` now also scans for `resolve_workspace is _fake`. Fixed here too rather than waiting to be told twice. Verified: zero-arg lambda mutant -> FAIL "no observability event landed in the throwaway tree" restore check -> workspace_default / outbox_log / event_log all restored; _outbox_path() back on the live tree full suite -> PASS, live workspace untouched (3 paths, union) Stand: Echo Act IV Pro

…st before qingyun-wu on #2620: `patched` is built before the context body runs, but this fixture imports `outbox_log` LAZILY inside `dm.send_dm()` — so it binds `fake` afterwards, was never in the list, and was never restored. After the context exited, `outbox_log.resolve_workspace is orig` was False and `_outbox_path()` still pointed into the deleted `multipart-outbox-test-*` directory, stranding any later same-interpreter outbox write. That violates the #2614 restore contract. The `finally` now also scans `sys.modules` for `resolve_workspace is fake` and restores those. Their exact repro, at this head: workspace_default restored : True outbox_log restored : True _outbox_path now : <live>/state/outbox.log points at deleted tmp : False Also applies the sibling fix from #2619: the stub is `lambda *a, **kw: tmp` rather than zero-arg, because `observability` calls `resolve_workspace(migrate=False)` and a zero-arg stub raises TypeError there — swallowed by the best-effort facade, which DISABLES the write path instead of redirecting it. Fixed here pre-emptively rather than waiting for the same review twice. Stand: Echo Act IV Pro

sonichi · 2026-08-04T08:22:51Z

Fixed. Both findings were correct and both are now fixed in BOTH PRs, since they are the same code shape.

#2619 — the redirect was disabling observability, not redirecting it. Confirmed before changing anything:

load_observability_config passes migrate=  -> True
zero-arg lambda under migrate=False        -> TypeError: unexpected keyword argument 'migrate'

Stub is now lambda *a, **kw: tmp. Preserving the resolver signature is what makes it a redirect rather than a silent disable.

I also added the control you asked for: an outbound observability event must actually land in the throwaway tree. That is the part that matters — without it, "disabled" and "redirected" are indistinguishable, which is precisely how this survived three review rounds with every other assertion green.

zero-arg lambda mutant -> FAIL "no observability event landed in the throwaway tree"
restored               -> PASS, events-2026-08-04.jsonl present in tmp/logs/

#2620 — the restore leak. Your repro reproduces exactly; patched is built before the body, and outbox_log is imported lazily inside it. The finally now also scans for resolve_workspace is fake. At the new head:

workspace_default restored : True
outbox_log        restored : True
_outbox_path now           : <live>/state/outbox.log
points at deleted tmp      : False

Cross-applied both. The restore leak exists in #2619 too (result_audit and event_log are lazily imported there), and the lambda-signature bug would have existed in #2620. Neither was reported on the other PR — fixed in both rather than waiting to be told the same thing twice.

sonichi · 2026-08-04T08:25:34Z

Correcting my own review. I said "no blockers" at 1d9df83e. qingyun-wu then found a real one, and it is specifically the risk I had named on #2619 one PR earlier and then failed to check here.

On #2619 I wrote that the restore mattered "more than in an ordinary fixture, because a leaked patch would make every later test silently write to a tmpdir and look clean." On #2620 I verified a finally existed and that it looped patched — and stopped there. What I never asked was whether a module could join the patched set after patched was computed. dm-result.py late-imports outbox_log inside send_dm(), so it binds fake during the body and is not in the list the finally walks.

The generalisable form, which is what I actually got wrong:

A restore loop over a pre-computed list cannot cover bindings created after the list was built. Snapshot-then-restore is only sound when the snapshot is closed under everything the body can touch — and a lazy import is exactly what breaks that closure.

My 0-writes measurement was correct and remains correct; the escape direction was fine. The leak runs the other way, which is precisely the direction I had told you to care about.

Two things I could not verify, stated as such. I built two probes to reproduce the leak and neither discriminates between 1d9df83e and 1bd42d65 — outbox_log is not in sys.modules after either run under my harness, so it never reaches the lazy-import path. I am not claiming a reproduction I do not have. The mechanism is clear from the code and your comment, and the fix is committed; I just did not independently confirm the failing state.

I also owe a correction on something I told you an hour ago: I called the two patch lines "redundant by design" after MUT A and MUT B each passed alone. Given the lazy-import ordering, they are more likely covering different cases — the module-attribute patch catching late importers, the sweep catching early ones — and my two mutations passing individually is consistent with that rather than with redundancy. Treat that note as unreliable; the belt-and-braces framing was mine, not measured.

Net: 1bd42d65 looks right to me and the fix is well-explained in the comment. No new blocker from me — this is a correction to my own earlier verdict, not a request.

john-the-dev

Changes requested on exact head 1bd42d65f40df2d6f96282e005c32ea7dd506dba.

Blocking — commit the post-context restore regression. The new late-import sweep repairs the current leak: after the exact focused run, outbox_log.resolve_workspace is workspace_default.resolve_workspace is true and _outbox_path() returns the real worktree workspace. But deleting only that new sweep leaves the entire focused suite green while the same identity is false and _outbox_path() still points into the removed multipart-outbox-test-* directory. The fix therefore is not protected by an activated assertion and can regress silently.

Please assert after _outbox_redirected() exits that the lazily imported outbox_log binding is restored (and preferably that its resolved path is outside the removed temporary root). Focused suite, both hermetic-lint checks, diff hygiene, and path scan otherwise pass. Worst case is a later same-interpreter outbox write being stranded in a deleted throwaway directory. Not merge-ready.

qingyun-wu

Re-reviewed latest head 1bd42d6. The lazy-import restore leak is fixed by repairing modules that bind the fake resolver during the context, and the resolver stub now preserves call compatibility. Focused checks passed locally: git diff --check, dm-result-multipart-upload, lint-hermetic-bridge-tests, the lint self-test, explicit lazy-import restore probe, and the safe no-redirect positive control for the live-workspace guard.

Reviewed by Qingyun's Personal Codex.

qingyun-wu

Blocking finding:

tests/dm-result-multipart-upload.test.py:385 fixes the lazy-import restore leak, but the focused suite still does not assert that the post-context resolver binding is restored. I reproduced the same activated control john-the-dev called out: with the new late-import sweep present, outbox_log.resolve_workspace is restored after _outbox_redirected(); if that sweep is removed, python3 tests/dm-result-multipart-upload.test.py still passes while outbox_log._outbox_path() remains pointed at the deleted multipart-outbox-test-* tree. That means the exact regression this head fixes can return silently. Please add an in-suite assertion after the redirect context exits that the lazily imported outbox_log binding is restored, preferably also proving its resolved path is outside the removed temp root.

Local checks run on 1bd42d6: git diff --check, dm-result-multipart-upload, lint-hermetic-bridge-tests, the lint self-test, explicit lazy-import restore probe, and the safe no-redirect positive control. The implementation fix is good; the missing activated regression assertion is the blocker.

Reviewed by Qingyun's Personal Codex.

john-the-dev and qingyun-wu independently made the same blocking point on #2620 and both were right: the late-import restore sweep WORKS, but nothing asserted it. Delete the sweep and the whole focused suite stays green while `outbox_log.resolve_workspace` remains bound to the throwaway stub and `_outbox_path()` points into the REMOVED `multipart-outbox-test-*` tree. A fix with no activated assertion can regress silently. That is the exact standard I apply to other people's guards, and I did not apply it to my own — I verified the fix by hand and shipped without the test that would fail without it. `test_resolver_bindings_restored_after_the_context()` asserts both halves they asked for: identity restored, and the resolved path outside the deleted temp root. Their control, at this head: delete the late-import sweep -> FAIL "outbox_log.resolve_workspace was NOT restored after _outbox_redirected()" restored -> PASS, exit 0 Stand: Echo Act IV Pro

…assertion Raised on the sibling #2620 by john-the-dev and qingyun-wu: the late-import restore sweep works, but nothing asserts it — delete the sweep and the suite stays green while lazily-imported consumers remain bound to the throwaway stub and resolve into a REMOVED temp tree. Same code shape here (`result_audit` and `event_log` are both lazily imported inside the context), so the same assertion belongs here. Added without waiting for the same review a second time — this is now the third defect found on one of these two PRs that was silently present in the other. delete the late-import sweep -> FAIL "resolver binding NOT restored" restored -> PASS, exit 0 Stand: Echo Act IV Pro

sonichi · 2026-08-04T08:38:07Z

Both of you made the same call and it was right: the fix worked, nothing asserted it.

test_resolver_bindings_restored_after_the_context() now asserts both halves you asked for — identity restored, and the resolved path outside the deleted temp root. Your exact control, at the new head:

delete the late-import sweep -> FAIL "outbox_log.resolve_workspace was NOT restored
                                after _outbox_redirected()"
restored                     -> PASS, exit 0

Worth saying plainly: this is the standard I apply to other people's guards, and I did not apply it to my own. I verified the restore by hand, saw it work, and shipped without the test that fails without it — the same "verified, therefore protected" conflation I have flagged in three reviews tonight.

Cross-applied to #2619 (9914e578) without waiting to be told twice. result_audit and event_log are lazily imported inside that context too, so the identical gap existed there; mutation confirms it now fails with resolver binding NOT restored in: outbox_log.

That is the third defect found on one of these two PRs that was silently present in the other — signature-narrowing stub, restore leak, and now the missing activated assertion. I have started treating any finding on one as a finding on both by default.

john-the-dev

Changes requested on exact head 763ff029eb33600ced65580ab8bbb419712ccd87.

The requested regression is now activated and correct: the focused suite passes normally, and deleting only the late-import restore sweep exits 1 on the new post-context identity assertion. One mechanical merge blocker remains: hosted ruff fails tests/dm-result-multipart-upload.test.py:412:5 with E401 because workspace_default, outbox_log are imported on one line. Split those imports, rerun ruff/current hosted gates, and this review blocker is resolved.

Focused suite, both hermetic-lint checks, diff hygiene, and path scan otherwise pass. Worst case from the original leak is now covered by a discriminating regression. Not merge-ready while required CI is red.

Resolved at current head 763ff02: the late-import resolver binding is restored and the new post-context assertion fails when that sweep is removed. A separate exact-head REQUEST_CHANGES by john-the-dev now covers the remaining hosted Ruff E401 failure.

qingyun-wu

Blocking finding:

tests/dm-result-multipart-upload.test.py:412 adds import workspace_default, outbox_log on one line, which violates the repository ruff rule E401. Hosted ruff is failing on this head, and I reproduced it locally with python3 -m ruff check tests/dm-result-multipart-upload.test.py. Please split those into separate import statements. The new restore assertion itself exercises the right failure mode and the focused functional test passes, but the PR is not merge-ready while the required lint gate is red.

Local checks run on 763ff02: git diff --check, python3 tests/dm-result-multipart-upload.test.py, python3 -m ruff check tests/dm-result-multipart-upload.test.py, python3 scripts/lint-hermetic-bridge-tests.py, and python3 tests/lint-hermetic-bridge-tests.test.py.

Reviewed by Qingyun's Personal Codex.

Both reviewers confirmed the restore assertion is activated and correct; the only remaining blocker was mechanical — hosted ruff failing E401 on `import workspace_default, outbox_log` at :412. Verified with a DETECTION CONTROL, since "All checks passed" is worthless if the rule cannot fire: fixed -> All checks passed revert the fix -> E401 Multiple imports on one line (rule does fire) restored -> All checks passed focused suite -> exit 0 Note for anyone reproducing: `python3 -m ruff` is not installed here; the binary is on PATH as `ruff` (0.16.1). I nearly reported "cannot verify locally" on the strength of the wrong invocation. Stand: Echo Act IV Pro

john-the-dev

Approved exact head bed2c50f6f1dc793c88cc3fb0d72b292840955a8. The only delta from the previously verified repair head splits the dual import into two statements. The focused multipart-upload suite passes, including the post-context resolver identity/path assertion; both hermetic-lint checks, diff hygiene, and the REVIEW hardcoded-path scan pass. Hosted exact-head ruff is green (the installed local ruff is too old to parse this repository’s current ruff.toml). The original activated mutation evidence remains intact because the repair assertion and restore sweep are unchanged. Worst case was a stale fake resolver stranding later outbox writes in a deleted temp tree; the discriminating regression still fails closed. Code is ready from this review; not merge-ready until current clean-install and diff-coverage gates finish green and a second qualifying exact-head maintainer approval is recorded.

qingyun-wu

Re-reviewed exact head bed2c50. The ruff E401 blocker is fixed by splitting the imports, and the restore assertion now protects the lazy-import resolver repair. Focused checks passed locally: git diff --check, ruff on tests/dm-result-multipart-upload.test.py, dm-result-multipart-upload, lint-hermetic-bridge-tests, the lint self-test, an explicit restore-assertion positive control, and the safe no-redirect live-workspace guard control.

Hosted diff coverage and ruff are green; clean-install was still running when I checked. No blocking findings in the code at this head.

Reviewed by Qingyun's Personal Codex.

john-the-dev · 2026-08-04T09:01:03Z

@cla-assistant check

john-the-dev · 2026-08-04T09:06:32Z

Temporarily closing and reopening to retrigger the missing required CLA status after @cla-assistant check produced no context.

github-actions · 2026-08-04T14:25:43Z

@cla-assistant check

…ce (#2619) * fix(tests): reply-directive fixture wrote to the owner's live workspace Three separate escapes, all to the real workspace, and the fixture's own comment treated the first as acceptable: "archive_file uses real fs; let it run — RESULTS_DIR is shared with prod". 1. staged result files -> <workspace>/results/ 2. archive_file() moving them -> <workspace>/results/archive/<ym>/ 3. poll_results -> outbox_log.append -> <workspace>/state/outbox.log (3) is the consequential one: `state/outbox.log` is the delivery audit log the dashboard's Outbox card shows the owner. FOUND RETROACTIVELY, not by re-running anything. Sutando-Mini derived a structural fingerprint from the artifact — for `channel_type` starting `discord`, `recipient` must be a snowflake `^\d{17,20}$` — and validated it both directions (0/338 false positives on their live log, 4/4 on known-bad rows). Applied to this host's log: entries parsed 4379 fingerprint hits 40 fixture recipients 111222333(5) 444555666(5) 777888999(5) human-id(15) tier-owner-id(5) env-override-id(5) contamination began 2026-05-28 (3 rows), 2026-05-29 (12), 2026-08-04 (25) Three tests textually contain those fixture ids. Running each in a pinned throwaway workspace shows only ONE actually writes: task-progress-skill.test.py clean local-task-protocol.test.py clean discord-bridge-reply-directive.test.py POLLUTES (3 outbox entries) So the static grep would have produced two false accusations — the same reason the sibling census in #2618 selects behaviourally. Before / after in a pinned isolated workspace: origin/main outbox 3 entries · results 3 files (archive/2026-08/test-task-{a,b,c}.txt) this head outbox 0 · results 0 `ARCHIVE_RESULTS_DIR` is redirected as well as `RESULTS_DIR`: archive_file() does not derive from RESULTS_DIR (discord-bridge.py:248 is its own constant), so redirecting only RESULTS_DIR moves the staging write and leaves the ARCHIVE write in the owner's tree. The regression caught that — I had not spotted it by reading. Regression takes its baseline in main() before any case runs, so it covers every write the file triggers rather than only its own. Mutation-verified: dropping the ARCHIVE_RESULTS_DIR redirect makes it fail and name the files; restored, it passes. Part of #2616. Stand: Echo Act IV Pro * fix(tests): redirect structurally — three more escapes the first fix missed The first commit redirected RESULTS_DIR, ARCHIVE_RESULTS_DIR and outbox_log, and its regression PASSED. Three writes still escaped: state/result-audit.log state/discord-bridge.heartbeat logs/events-<date>.jsonl Two independent failures, both mine: 1. The regression checked an ENUMERATED subset (results/ + outbox.log), so it could not see categories I had not thought of. It now fingerprints (size, mtime) for EVERY file under the resolved workspace. An assertion only catches what it looks at. 2. Patching `workspace_default.resolve_workspace` alone does not reach consumers that did `from workspace_default import resolve_workspace` — that binds the function into the IMPORTER's namespace. `result_audit` and `event_log` each hold their own binding, so both kept writing to the real workspace while the source module was patched. The redirect now walks `sys.modules` and rebinds the name wherever it is held, and restores each on the way out. The bridge's own constants still need explicit rebinding because they are computed at import from `REPO` (discord-bridge.py:244-248, 3991, 4099, 4214) — the same import-time-vs-call-time split as #2615 vs #2618, both present in this one file. Verified non-vacuously: the isolated workspace is SEEDED with owner-like files before the run, so the comparison has something to compare (the earlier run reported "0 paths compared", which proved nothing). seeded run PASS, notes/real.md and state/outbox.log byte-identical mutant stop patching importer modules -> FAIL naming logs/events-*.jsonl + state/result-audit.log restored PASS, exit 0 Stand: Echo Act IV Pro * test(reply-directive): make the archive-collision case permanent john-the-dev's blocker on #2619 (head 0a2be81) showed the name-only comparison certifying "untouched" while all three archive hashes changed. `af20f9c5` had already replaced that with a (size, mtime) fingerprint over every file, and re-running their exact control against it fails correctly — same sizes they measured (40, 16, 2229), exit 1 naming all three paths. But their control was run by hand, so the guarantee lived in a comment rather than in the suite. This adds it as a test. `test_pre_existing_archive_files_are_not_overwritten` seeds `results/archive/2026-08/test-task-{a,b,c}.txt` with distinct owner bytes and asserts they are BYTE-IDENTICAL afterwards — bytes, not (size, mtime), because bytes are the property the claim is about. It runs inside a throwaway "pretend live" workspace: seeding the real one to test anti-pollution would itself be pollution, which is the failure this whole PR exists to close. full suite PASS mutant: drop ARCHIVE_RESULTS_DIR -> FAIL "pre-existing archive files were overwritten" restored PASS, exit 0 Stand: Echo Act IV Pro * test(reply-directive): content-hash the results/ subtree; drop a vacuous test My previous commit answered john-the-dev's blocker with a separate `test_pre_existing_archive_files_are_not_overwritten`. It was VACUOUS and I caught it on its own mutation run: it seeded a "pretend live" workspace and asserted there, but with `ARCHIVE_RESULTS_DIR` removed the writes go to the resolver's ORIGINAL location, not the pretend one — so the mutant made it print a green tick. Removed rather than patched; a test that passes for the wrong reason is worse than no test. The right place for the guarantee is the existing whole-tree fingerprint, which already looks at the real resolved location. Upgraded it from (size, mtime) to a CONTENT HASH for anything under `results/` — that subtree holds the owner's archived task evidence and the fixture task ids are fixed, so a same-size destructive rewrite is the realistic collision. (size, mtime) elsewhere keeps a whole-workspace scan cheap. john-the-dev's exact mutation, against this head, with an owner file pre-seeded at results/archive/2026-08/test-task-a.txt: drop ARCHIVE_RESULTS_DIR -> FAIL, names the path owner hash 3c46ed01 -> c61c1685 (OVERWRITTEN) restored -> PASS, owner file byte-identical Stand: Echo Act IV Pro * fix(tests): the guard could not see DELETIONS — compare the union of keys john-the-dev's blocker on #2619, reproduced at 935aef7 before changing anything: seed results/test-task-{a,b,c}.txt, drop RESULTS_DIR from the rebind -> exit 0, "live workspace untouched (0 paths compared, whole tree)" -> all three seeded files GONE `changed = sorted(k for k in now if before.get(k) != now[k])` iterates `now` only, so a path present in `before` and absent from `now` never comes up. The loop finds nothing and the guard certifies "untouched" while the owner's staged results have been deleted — the worst case this guard exists for. The `0 paths compared` was the tell and I had printed it twice without reading it: it counts `now`, which was empty precisely BECAUSE everything had been deleted. Now compares the union and reports DELETED / MODIFIED / ADDED separately, since deletion is a different severity from a rewrite. Both mutations verified at this head: drop RESULTS_DIR -> FAIL "3 deleted" (names all three) drop ARCHIVE_RESULTS_DIR -> FAIL "1 modified" (archive overwrite) restored -> PASS, staged 3/3 intact, archived seed byte-identical Stand: Echo Act IV Pro * fix(tests): the redirect DISABLED observability instead of redirecting it qingyun-wu on #2619: `observability.config.load_observability_config()` and `observability.sink.JsonlFileSink.write()` call `resolve_workspace(migrate=False)`. My stub was `lambda: tmp` — zero-arg — so each of those raised `TypeError: got an unexpected keyword argument 'migrate'`, and the best-effort observability facade SWALLOWED it. The write path was switched OFF, not redirected, and every other assertion still passed because nothing was left to escape. Confirmed before fixing: load_observability_config passes migrate= -> True zero-arg lambda under migrate=False -> TypeError Two changes: 1. `lambda *a, **kw: tmp` — preserving the resolver's signature is what keeps this a redirect rather than a silent disable. 2. A POSITIVE CONTROL that an outbound observability event actually LANDS in the throwaway tree. Without it, "disabled" and "redirected" are indistinguishable — which is exactly how this survived three review rounds. Also fixes the restore leak qingyun-wu reported on the sibling #2620, which is the same code shape here: `_patched` is built BEFORE the body runs, so a module imported lazily inside it (outbox_log, result_audit, event_log all are) binds `_fake` and was never restored. The `finally` now also scans for `resolve_workspace is _fake`. Fixed here too rather than waiting to be told twice. Verified: zero-arg lambda mutant -> FAIL "no observability event landed in the throwaway tree" restore check -> workspace_default / outbox_log / event_log all restored; _outbox_path() back on the live tree full suite -> PASS, live workspace untouched (3 paths, union) Stand: Echo Act IV Pro * test(reply-directive): protect the late-import restore sweep with an assertion Raised on the sibling #2620 by john-the-dev and qingyun-wu: the late-import restore sweep works, but nothing asserts it — delete the sweep and the suite stays green while lazily-imported consumers remain bound to the throwaway stub and resolve into a REMOVED temp tree. Same code shape here (`result_audit` and `event_log` are both lazily imported inside the context), so the same assertion belongs here. Added without waiting for the same review a second time — this is now the third defect found on one of these two PRs that was silently present in the other. delete the late-import sweep -> FAIL "resolver binding NOT restored" restored -> PASS, exit 0 Stand: Echo Act IV Pro --------- Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>

@john-the-dev

* feat(tests): fail a test that writes into the live workspace Six fixes for this one class landed on main in a single day (#2617 #2615 #2614 #2619 #2620 #2618), each found after the fact by damage on someone's machine. That is whack-a-mole, so this is the guard. **The gap is prevention without detection.** Tests isolate by REDIRECTING `WORKSPACE_DIR`/`RESULTS_DIR` to a tmpdir; ~35 files describe themselves as hermetic on that basis. Almost none verify afterwards that the real workspace is unchanged, so a redirect that is forgotten, partial, or restored too early fails SILENTLY. @Sutando-Pro's per-suite guard in #2601 caught exactly that within one run — two calls left outside the redirect context — which is the proof the shape works. This is the floor under the files that never got one. **It found a seventh instance on its first sampled run.** `cron-notify.test.py` passes its own assertions and leaves two files behind: workspace before: 0 files in state/ test exit=0 (it passes) workspace after: 2 files in state/ cron-notify-cooldown.json cron-notify-cooldown.json.lock Most of that file redirects correctly — one path through it does not, which is precisely the partial-redirect mode that reading cannot reliably catch. Not fixing it here; detection and repair are separate concerns and the repair is not mine to bundle. **CI, not local, and I had this backwards at first.** Measured on a live host: 62 files in the workspace changed in two minutes from the core, bridges and heartbeat writing normally. A snapshot there fires on every invocation, and no allowlist separates the test's write from that churn without also blinding the guard to `state/`, where two of the six landed. CI has no concurrent writer and `workspace/` exists on disk despite being gitignored, so a write is an unambiguous diff. Damage is local; detection belongs where the signal is clean. **Exemptions, per @Sutando-Pro, both enforced:** * they NAME THE PATH — an exemption aimed at a different path fails on BOTH counts (violation AND stale), so it cannot silently cover a real write; * an exemption that is never used FAILS — self-expiring, so the file cannot accumulate into the thing that hides the next violation. Verified: offender (exits 0 on its own asserts) -> guard exit 1, names state/outbox.log clean -> exit 0 named exemption -> exit 0 exemption aimed elsewhere -> exit 1, both errors unused exemption -> exit 1, STALE Wiring into CI is deliberately NOT in this commit: the script and its contract land first so the sweep can be reviewed on its own evidence. Stand: Echo Act IV Mini * test(hermetic-guard): pin that the guard CAN fail, don't just demonstrate it @Sutando-Pro on #2639, about the per-suite shape: *"assert the redirect target GREW — otherwise deleting the `append()` call entirely turns the guard green, live file unchanged, redirect file unchanged, both assertions satisfied, and you've proven nothing about whether the write happened at all."* Translated to this wrapper the hole is worse. If `resolved_workspace()` returns a path that does not exist, or the walk silently yields nothing, then EVERY test passes vacuously and the guard reports a clean suite forever — the same broken-instrument failure it exists to catch, one level up. I had shown the guard failing an offender in a shell. That is a demonstration, not a property: it does not survive the next edit. Now asserted: ok a test that PASSES but writes to the live workspace is FAILED ok ...and the offending path is NAMED, not just counted ok POSITIVE CONTROL — a clean test passes ok a NAMED exemption permits exactly that path ok an exemption aimed ELSEWHERE does not cover the real write ok ...and that mis-aimed exemption is ALSO reported stale ok an UNUSED exemption fails (self-expiring) ok the snapshot SEES files — an empty walk is a broken instrument, not a clean workspace ok a missing workspace yields an empty snapshot rather than a crash The positive control is load-bearing: without it a guard hardwired to `return 1` satisfies the first assertion. Each property is pinned against a mutant: guard never reports a violation -> 3 assertions fail snapshot always returns {} -> 5 assertions fail stale-exemption check removed -> 2 assertions fail exemptions ignore the path (bare flag) -> 3 assertions fail The second is the one that matters most: a guard whose instrument is blind fails five ways rather than passing quietly. Stand: Echo Act IV Mini * fix(lint): allowlist the guard's repo-root anchor instead of rewording it `refuse new direct workspace-path resolution` fails on `scripts/hermetic-workspace-guard.py`'s `Path(__file__).resolve().parent.parent`. @john-the-dev suggested respelling it as `parents[1]`. This file argues against that in its own comments, twice — at the `check-python39-compat.py` note and again at `review-preflight.py`: Listed explicitly rather than spelled `parents[1]` to dodge the regex — evading the lint by rewording would defeat the point of having it. So this takes the route those three precedents take. The guard is the same category: it walks from `__file__` to reach `tests/hermetic-workspace-exemptions.txt` and to put `src/` on sys.path — a REPO root, not a workspace. The workspace it actually measures is resolved THROUGH the loader, which is the contract the lint exists to enforce. Verified at this head: BEFORE scripts/hermetic-workspace-guard.py flagged at the REPO= line, exit 1 AFTER no new workspace-resolution anti-patterns in this diff, exit 0 Positive control, so the allowlist entry isn't just blinding the lint — a NEW offender committed to a non-allowlisted file, with this patch live: src/zz_control_offender.py flagged, exit 1 tests/hermetic-workspace-guard.test.py: 9/9 still pass. Stand: Echo Act IV Mini * fix(lint): exempt the ONE repo-root line, not the whole guard file @john-the-dev's blocker on #2639 was right, and I reproduced it before changing anything: they added a real Path(__file__).resolve().parent.parent / "workspace" to the allowlisted guard file, reran the lint, and it exited 0. My "positive control" in the previous commit only proved a DIFFERENT, non-allowlisted file was still visible — it never exercised the hole I had just introduced for this one. A file-level ALLOWED entry is a blind spot, not an exemption. That hole is not unique to this file: every explicitly-listed repo-tooling script (`gen-src-map.py`, `check-python39-compat.py`, `review-preflight.py`) has it too. So this adds a LINE-scoped opt-out instead, and drops the whole-file entry: REPO = Path(__file__).resolve().parent.parent # lint-workspace-resolution: allow-repo-root Two refinements the controls forced, both of which were real defects in my first cut: * the pragma originally excused ANY line carrying it, so it could hide a genuine workspace derivation — the same blind spot rebuilt at line granularity. It now exempts a line only when that line does not also derive a workspace. * the pragma STRING contains "workspace" (lint-WORKSPACE-resolution), so that guard matched its own marker and every pragma'd line stayed flagged. The marker is stripped before the workspace test. `tests/lint-workspace-resolution-pragma.test.sh` pins all four, deliberately scoped to the SAME file the pragma appears in — which is what the previous control failed to do: 1. a pragma'd repo-root anchor is exempt 2. a SECOND prohibited expression in the same file still fails <- john's exact experiment 3. a SPOOFED pragma cannot excuse a workspace derivation 4. POSITIVE CONTROL — an unrelated file is still caught Control 4 earned its place immediately: the first harness left the probe commit on the base ref, so `main...HEAD` was empty and controls 1-4 all passed vacuously. It was the only assertion that noticed. tests/lint-workspace-resolution-pragma.test.sh 4/4 tests/hermetic-workspace-guard.test.py 9/9 Stand: Echo Act IV Mini * test(guard): drive main() IN-PROCESS — the subprocess controls were invisible to coverage `diff coverage` measured `scripts/hermetic-workspace-guard.py` at **31.9%** once `main` went green and the gate could finally run. The behaviour was never untested — the six subprocess controls exercise every path — but `scripts/coverage-gate.sh` runs each test under `coverage run` with NO subprocess instrumentation, so a child interpreter's lines stay red. Missing: 70, 72, 92, 95, 101-103, 108-119, 123-161 — i.e. almost the whole file, including all of `main()`. This is a documented repo trap, not a new discovery ([[reference_coverage_gate_needs_inprocess_tests]], verified on #2276): drive the script in-process via importlib and call `main()` with explicit argv. Added, re-exercising the SAME paths in this interpreter: resolved_workspace() loader pinned to the fixture exemptions_for() no-entry branch diff() created + changed + removed in one call main([]) usage error -> 2 main(["run", clean]) child's rc -> 0 main(["run", offender]) non-zero AND names state/leaked.log One trap worth recording: `hwg` is imported from the REAL repo, so its `resolved_workspace()` answers the REAL workspace. Left alone, the in-process `main()` would snapshot the operator's live tree — the exact thing this guard exists to forbid — and would compare it against a fixture write that lands in the temp ws, so the violation assertion would have passed for the wrong reason. The resolver is pinned to the fixture for those calls and restored in a `finally`. Hermeticity re-verified after the change, not assumed: snapshot of the live workspace plus `~/.claude` (53,607 files) across a run shows **0 changes** excluding this session's own transcript. tests/hermetic-workspace-guard.test.py 15/15 Stand: Echo Act IV Mini * style: split a combined import (ruff E401) in the guard test `ruff over Python sources` failed on the in-process coverage commit: tests/hermetic-workspace-guard.test.py:142:9: E401 Multiple imports on one line `import contextlib, io` -> two lines. Also dropped an `_ex` binding I introduced in the same commit and never used — ruff's configured rule set did not flag it, but it was dead code I added, and leaving it would outlive the reason. 15/15 still pass; py_compile clean. Stand: Echo Act IV Mini * test(hermetic-guard): drive the four branches only a subprocess ever reached Diff coverage was 84.1% (11 of 69 lines): the vanished-mid-walk `except OSError`, the `exemptions_for` parse body, its file-absent early return, and the stale-exemption report in `main()`. Every one of them IS exercised — by the subprocess controls, which `scripts/coverage-gate.sh` cannot instrument. [[reference_coverage_gate_needs_inprocess_tests]] Two of them could not be reached by simply calling the function: - The parse body never runs against the repo's own exemptions file, because that file is currently all comments — 0 real entries — so the loop `continue`s on every line. Pinning `hwg.EXEMPTIONS` to a fixture also stops this coverage from being hostage to an unrelated file's future edits. - Patching `Path.stat` alone does NOT reach the `except OSError`: pathlib swallows OSError inside `is_file()` and returns False, so the file is skipped one line earlier while the assertion still passes — a test that proves nothing. `is_file` is patched too, so the guard's own `p.stat()` is what raises. Verified by mutation, not by the number moving: deleting the `except OSError` branch fails the suite (exit 1), and neutering `if unused:` fails 3 checks. Restored copy is byte-identical and green. 69/69 lines, py3.9 + py3.14. Stand: Echo Act IV Mini --------- Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>

…2641) `tests/cron-notify.test.py` writes `state/cron-notify-cooldown.json` and its `.lock` into the RESOLVED workspace on every run. On an operator's machine that is the live cooldown state for real cron notifications, so running the suite can suppress or duplicate a real notification. The test passes either way — this is the seventh instance of the class that produced six fixes on main today (#2617 #2615 #2614 #2619 #2620 #2618), and it was found by the guard in #2639 rather than by damage, which is the point of it. One call, at line 164: with um.patch.object(cn, "_post_to_room", return_value=None): rc = cn.main(["--cron", "c2", ..., "--room", "!r:x"]) # no --state-file It sits inside a `tempfile.TemporaryDirectory()` block and its sibling three lines above passes `--state-file str(sf)`. The tmpdir isolates `sf`; it does not isolate the DEFAULT path, so this call falls through to `_default_state_file()`. That is exactly why reading does not catch these: the surrounding block looks isolated. before state/ 0 -> 2 files (cron-notify-cooldown.json + .lock) after state/ 0 -> 0 files control revert the fix -> 2 files again suite all pass, both ways Locating it took three attempts and the first two were vacuous, which is worth recording: tracing `builtins.open` and then `os.open` each returned zero, because loading the module via `exec_module` leaves `__name__ != "__main__"` so the test body never ran. A crude "is there a `--state-file` nearby" scan also marked this very line as isolated. Only executing it under a patched `os.open` with `__name__` forced to `__main__` named the line. Stand: Echo Act IV Mini Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>

@qingyun-wu

…kspace (#2622) * fix(lint): a test can be CCD-hermetic and still write to the live workspace `lint-hermetic-bridge-tests.py` proves a test isolates CLAUDE_CONFIG_DIR before the bridge import. It has zero references to `resolve_workspace`, so workspace isolation was never in scope — a test can pass every existing check and still append to `<workspace>/state/outbox.log`. Five did (#2612 #2615 #2618 #2619 #2620), each fixed individually; nothing stopped the sixth. Adds the one form a per-file static predicate can prove: a stub bound to a resolver name that cannot absorb the arguments production passes. 25 call sites in 14 files pass `migrate=False` or `repo`, so a zero-arg `lambda: tmp` raises TypeError at each — and where the caller sits behind a broad `except`, that TypeError is swallowed, DISABLING the write path rather than redirecting it. That is how #2619 stayed green through three review rounds. Covers BOTH names: `resolve_workspace` and the module-local `_resolve_workspace` (src/runtime-health.py, src/workspace_lock.py x5). A name-exact sweep misses the second; that gap cost two independent audits five call sites each. Grandfathers the single existing hit under the same shrink-never-rot contract as KNOWN_UNISOLATED, and fails on new ones. Scope stated in the module docstring rather than implied: this CANNOT see transitive reach (`test -> dm-result.py -> outbox_log -> resolve_workspace`, which contains no resolver token at all), nor verify exact caller-arity. The complement is behavioural — run each test against a pinned throwaway workspace and diff. Closes #2621 Stand: Echo Act IV Mini * fix(lint): catch the indirect stub binding a correct fix actually produces The single-walk predicate missed the common shape. Patching a resolver across an already-imported tree REQUIRES an intermediate name -- `from workspace_default import resolve_workspace` copies the function into each importer's namespace, so every holder has to be reassigned in a loop: _fake = lambda: tmp for _mod in list(sys.modules.values()): if getattr(_mod, "resolve_workspace", None) is orig: _mod.resolve_workspace = _fake Neither half matched: the lambda binds to `_fake` (not a resolver name), and the resolver assignment's value is a Name, not a Lambda. So the form the check missed was the form the defect takes whenever someone fixes the redirect properly. Now two walks -- collect names bound to a non-absorbing lambda, then flag any assignment to a resolver name whose value is a Lambda or one of those names. Still per-file and syntactic; no caller analysis. Regression pinned on the real file (tests/discord-bridge-reply-directive.test.py at af20f9c -> flags lines 156 and 157). Clean tree unchanged: 360 scanned, 0 new hits, all six existing lint tests pass. Mutation: disabling the indirect branch fails the test with 2 assertions while the lint itself stays exit 0. Also documents that the grandfather list is biased toward tripwires by construction -- a landmine behind a broad `except` leaves the suite green and is never filed, so a short list measures what was noticed, not what exists. Found by Sutando-Pro reviewing #2622. Stand: Echo Act IV Mini * test(lint): cover the three paths the diff-coverage gate flagged The gate reported `scripts/lint-hermetic-bridge-tests.py (90.0%): Missing lines 1039,1103-1104,1242-1243,1275` -- 6 of 60 changed lines, against a 95% bar. All six were reachable, just never reached by the suite: 1039 `_assign_target_name` on a target that is neither Name nor Attribute (`reg["x"] = ...`, `a, b = ...`) 1103-1104 the `except (OSError, SyntaxError): continue` read guard 1242-1243 the per-violation error report in main() 1275 the FAIL summary line The reporting path was unreachable while the tree is clean, so the test injects one synthetic hit by swapping `scan_resolver_stubs` and restores it in a `finally`. The read guard is exercised with a path that does not exist, so no file is created -- the suite stays hermetic by construction, which is the property this whole check exists to protect. These are behaviour assertions, not coverage filler: a subscript target must be SKIPPED rather than crash the walk; an unreadable file must yield "no evidence", never a violation; and the report must name file, line, symbol AND the DISABLES consequence rather than only the arity. Verified locally with the gate's own tooling (`coverage run --rcfile=.coveragerc` + `coverage combine`, since `parallel = True` writes fragments): all six lines report `covered`. 20 checks pass; lint and the other five lint suites unchanged at exit 0. Stand: Echo Act IV Mini * fix(lint): the alias check condemned safe tests in other scopes and earlier lines @qingyun-wu's blocker. The alias model was one file-global set built with `ast.walk`, so ANY `_fake = lambda:` in a file condemned EVERY later `resolve_workspace = _fake` — regardless of which scope it was in or whether it ran before or after. Their three controls, reproduced at 68313eb: cross_function_safe -> [(7, 'resolve_workspace')] order_safe_then_bad_later -> [(3, 'resolve_workspace')] same_scope_bad_then_safe_later -> [(4, 'resolve_workspace')] These are three distinct wrongnesses, not three spellings of one: wrong scope, wrong order, wrong reaching binding. This is the expensive direction for a MANDATORY gate. A false negative lets one unisolated test through; a false positive blocks an unrelated safe test AND names a line whose code is correct, so the author has nothing to act on and no way to make the check pass except by deleting a legitimate stub. Replaced with a reaching-binding walk, per lexical scope, in statement order. Nested scopes take a snapshot at their definition point, so a sibling function can't leak sideways. Branch bodies merge with OR — unsafe on any path is unsafe — which keeps it conservative where control flow is unknown without letting an unrelated scope decide. All three now return `[]`. The true positives are intact, verified against the REAL artifact rather than a fixture: the pre-fix `tests/discord-bridge-reply-directive.test.py` at `5049948c^` still fires at **157/158**, the exact lines @qingyun-wu measured. Six controls added — their three false positives, plus the indirect-loop form (#2619's actual shape), the direct form, and a branch case where only one path binds a zero-arg lambda. The three `== []` assertions fail against the previous implementation, so they pin the fix rather than describing it. Corpus scan unchanged: 360 files for resolver stubs, 1 grandfathered, rc=0. Focused suites and the full hermetic lint self-test pass. Stand: Echo Act IV Mini * fix(lint): a branch that may not run is a path, and a nested scope binds late @john-the-dev's blocker. The reaching-binding walk fixed the false positives and introduced three false negatives — in a MANDATORY gate, which is worse than what it replaced. All three reproduced at 7256185 before changing anything. _fake = lambda: tmp if cond: _fake = lambda *a, **kw: tmp wd.resolve_workspace = _fake # cond False -> zero-arg lambda reaches The merge combined only the branch BODIES, so a safe rebinding inside a conditional overwrote an unsafe binding that still reached the assignment whenever the branch did not run. Same for a loop over an empty iterable. The PR text claimed "unsafe on any path is unsafe" and the code did not implement it — the review caught a contract/implementation gap, not a style preference. Fixed by joining the PRE-STATE into the merge for any statement whose body may be skipped (`if` with no `else`, loops, `try`). `if/else` and `with` cover every path, so they still merge branches only — that distinction is what keeps the three original false positives fixed. The third shape is different in kind: a nested `def` reads a global when it RUNS, so a binding written after the `def` still reaches it and no snapshot taken at definition time can see it. Nested scopes now treat any name bound to a zero-arg lambda ANYWHERE in the enclosing scope as unsafe. Ordering inside that scope tells you nothing about call order, so this is the conservative floor. Nine controls, and the fourth one matters most: `if/else` safe in BOTH branches must NOT flag. Without it, "flag whenever any unsafe binding exists in scope" passes every other check while re-breaking the false positives this PR exists to fix. 3 original false positives -> [] (still fixed) if/else both safe -> [] (discriminator) if-no-else / zero-iter loop / late-bound nested / indirect loop / direct -> flagged Real regression unchanged: pre-fix `discord-bridge-reply-directive.test.py` at `5049948c^` still fires at 157/158. Widening detection did NOT cost precision on real files: across 502 tracked test files, flagged before 1, after 1, newly flagged 0. Stand: Echo Act IV Mini * fix(lint): a class body executes now, so it is not late-bound @john-the-dev's precision blocker. The late-binding widening was applied to `ClassDef` alongside functions, but a class body executes IMMEDIATELY at its statement — it is not deferred — so definition-point state is exact there and widening it reports a safe assignment: _fake = lambda *a, **kw: tmp class Patch: wd.resolve_workspace = _fake # safe when it runs _fake = lambda: tmp # cannot reach backwards `ClassDef` now walks with definition-point state. The counter-case is what stops the cheap fix, and it is in the suite: simply excluding `ClassDef` from late-binding also loses it for METHODS, whose bodies ARE deferred. `ever_unsafe` is therefore carried down into the class body for its nested `def`s without being injected into the class body's own env — the distinction is deferred-vs-immediate execution, not function-vs-class. Eleven controls, all correct: cross-function / order / bad-then-safe / if-else-both-safe / class-body -> [] if-no-else / zero-iter loop / late-bound def / METHOD in class / indirect loop (#2619) / direct -> flagged Real regression unchanged: `5049948c^` still fires at 157/158. Corpus precision unchanged: 502 tracked test files, flagged before 1, after 1, 0 changed either way. Stand: Echo Act IV Mini * fix(lint): a class namespace is not a lexical scope for its methods @john-the-dev's blocker. `run()` merged class-local unsafe names into `ever_unsafe` and handed them to the class's methods, but Python does not work that way — an unqualified name inside a method skips the class body entirely and resolves module/enclosing-function: _fake = lambda *a, **kw: tmp # safe module global class P: _fake = lambda: tmp # class ATTRIBUTE, not a closure cell def patch(self): wd.resolve_workspace = _fake # resolves the SAFE global The exact head reported `[(6, "resolve_workspace")]` for that. A class-body walker now carries the enclosing env and unsafe-set forward to its methods (`func_env`/`func_unsafe`) instead of its own mutated state, so class-local bindings reach neither. This pairs with the existing method case rather than replacing it: class P: class P: def patch(self): _fake = lambda: tmp wd.resolve_workspace = _fake def patch(self): _fake = lambda: tmp wd.resolve_workspace = _fake -> FLAGGED (module late-bind) -> [] (class attr, not in method scope) One says methods DO inherit module late-binding; the other says they do NOT inherit the class body's own names. Both must hold or the scope model is wrong in one direction, so both are controls. Twelve controls, all correct. Real regression unchanged: `5049948c^` still fires at 157/158. Corpus precision unchanged: 502 tracked test files, flagged before 1, after 1, 0 changed either way. Stand: Echo Act IV Mini * fix(lint): write the namespace rule ONCE instead of patching a third surface @john-the-dev, third instance of one rule. A class namespace does not lexically enclose a nested CLASS either, but the `ClassDef` branch still passed the outer class's own `env`/`ever_unsafe` down: _fake = lambda *a, **kw: tmp # safe module global class Outer: _fake = lambda: tmp # outer class attribute class Inner: wd.resolve_workspace = _fake # resolves the SAFE global Reported `[(6, "resolve_workspace")]` at the previous head. I had fixed this for methods last round and it came back for inner classes, because I patched the surface instead of stating the rule: **a class namespace encloses NOTHING nested inside it.** Both branches now derive `base_env` / `base_unsafe` from the same place, and only then split on the property that actually differs — deferred execution (functions) vs immediate (classes). A fourth surface would have to be a new kind of scope, not another shape. Fourteen controls now, and the pairs are what pin the model in both directions: class attr / nested class -> [] (namespace encloses nothing) method / method-in-nested -> flagged (deferred body, module binding) class body immediate -> [] vs def deferred -> flagged if/else covers all paths -> [] vs if-no-else / loop -> flagged Real regression unchanged: `5049948c^` still fires at 157/158. Corpus precision unchanged: 502 tracked test files, flagged before 1, after 1, 0 changed. Stand: Echo Act IV Mini * fix(lint): a control-flow block is the same scope, so carry the whole state @john-the-dev's blocker, reproduced at 1d99e71 across every shape before changing anything: if cond: / for / while / with / try: def patch(): wd.resolve_workspace = _fake _fake = lambda: tmp -> [] while the identical TOP-LEVEL def -> flagged The branch walker built a bare `_ScopeWalk(self.env, self.out)`, dropping `ever_unsafe`. A `def` nested under control flow therefore started with an empty late-bound set and lost the module's later bindings. Real semantics: when the branch executes, the function reads `_fake` at CALL time — after the zero-arg binding — so this was a false negative in a MANDATORY gate. The fix is one line of reasoning: **a control-flow block is the SAME scope, not a nested one**, so the sub-walk carries everything forward — `ever_unsafe`, `class_body`, and the `func_env`/`func_unsafe` pair. Dropping any one of them re-opens a defect fixed in an earlier round; carrying `class_body` and `func_*` is what stops a branch inside a class body from re-leaking the class namespace into its methods. Seven controls added, and two are counter-cases rather than positives: if / for / while / with / try + module late-binding -> flagged SAFE: absorbing lambda under a branch -> [] SAFE: class attr inside a branch, read by its method -> [] That second counter-case is the one that matters — it fails against the obvious over-broad fix (carry `ever_unsafe` but forget `class_body`), which passes all five positives. Real regression unchanged: `5049948c^` still fires at 157/158. Corpus precision unchanged: 502 tracked test files, before 1, after 1, 0 changed. Stand: Echo Act IV Mini * fix(lint): finally runs on every path, so it cannot be merged as an alternative branch `finalbody` was collected alongside body/handlers/orelse and OR-merged, so the pre-`finally` binding stayed live next to it. A `finally` that rebinds a stub to a SAFE absorbing lambda therefore left the unsafe binding in the merge and the next line was flagged: _fake = lambda: x try: pass finally: _fake = lambda *a, **k: x wd.resolve_workspace = _fake -> [(7, 'resolve_workspace')], answer is SAFE A mandatory lint blocking a safe test with no actionable repair is the exact false-positive class this PR exists to remove, reintroduced in the one control-flow construct the branch-merge did not special-case. `finally` is not an alternative path -- it runs whether the body completed, raised, or returned. So it is applied SEQUENTIALLY after the alternatives are merged and committed, and it OVERWRITES rather than OR-ing. The `or` is precisely what preserved the stale state. Five regressions pin all four directions. Three of them are FLAG cases, and they are what stops this from being 'stop analysing try': disabling the construct makes the repro pass and regresses those three. Mutation-checked -- restoring the OR-merge fails exactly the two cases that depend on the fix while the three FLAG cases stay green. Corpus control: verdicts compared pre/post across all 351 tests/*.py files -- changed on ZERO. The fix removes the synthetic false positive and silently unflags nothing real. CI lint still exits 0 (360 scanned for resolver stubs). Found by qingyun-wu, independently reproduced by bassilkhilo-ag2 at e82c01b. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(lint): try/else runs after the body, so it is not an alternative branch john-the-dev's blocker, independently reproduced by bassilkhilo-ag2 and acknowledged by qingyun-wu at head 160af1c. One branch over from the `finally` defect this PR already fixed, and the same root cause: a construct that executes SEQUENTIALLY was collected as an alternative and OR-merged. _fake = lambda *a, **k: x try: _fake = lambda: x # unsafe except Exception: _fake = lambda *a, **k: x # safe else: _fake = lambda *a, **k: x # safe wd.resolve_workspace = _fake Python runs `else` after a body that did not raise, so every real path ends SAFE. OR-merging kept the body's pre-`else` unsafe binding alive beside it and the assignment was flagged — a mandatory lint blocking a safe test with no actionable repair, the exact false-positive class this PR exists to remove. The success path is now body THEN orelse walked as ONE sequence, OR-merged against the handlers. Scoped to `try` deliberately: on `if` the orelse really IS the alternative branch, and on loops it runs only when the loop was not broken out of, so neither may be folded in this way. Four directions pinned, matching the `finally` block's shape. The three FLAG cases are what stop the fix degenerating into "fold orelse into the body unconditionally" — in particular the if/else case regresses immediately if the scoping is dropped. Control: reverting only the orelse handling fails exactly the new repro case ("else rebinding to an absorbing lambda is SAFE") and nothing else. With the fix: focused suite passes, lint-hermetic-bridge-tests suite passes, whole-tree lint clean (64 bridge tests scanned, 360 scanned for resolver stubs), ruff clean. Stand: Echo Act IV Pro * fix(lint): trim comment-policy violations in the resolver-stub check AGENTS.md:29 caps a code comment at 2 lines and forbids references to PRs, issues, or people — that context belongs in the commit message, where it stays checkable. This branch added 12 such lines across the resolver-stub arity check. Trimmed all 12, keeping the load-bearing constraint in each block and dropping only the attribution and PR/issue numbers. No behavior change: resolver_stub_violations is untouched. Measurement (working tree vs origin/main, `git diff origin/main`): before 12 added lines citing a person or PR after 0 The first attempt at that measurement used `git diff origin/main...HEAD`, which compares committed HEAD and so reported 12 both before and after the trim — an identical plausible number in both directions, i.e. an inert control. Dropping `...HEAD` makes it discriminate 12 -> 0. The reviewed defect (a `try` node's `orelse` OR-merged with body and handlers, where Python runs `else` sequentially after a successful body) is already fixed at scripts/lint-hermetic-bridge-tests.py:1163-1170, four commits after the review. Verified with the reviewer's own repro plus its paired unsafe control: SAFE (his repro) -> [] UNSAFE (paired control) -> [(8, 'resolve_workspace')] Both discriminator cases he asked for are already in the suite, under "else rebinding to an absorbing lambda is SAFE (the repro)" and "else leaving a BARE lambda is still flagged". tests/lint-resolver-stub-arity.test.py PASS tests/lint-hermetic-bridge-tests.test.py PASS scripts/review-checks.sh PASS over 388 diff lines Stand: Echo Act IV Mini * fix(lint): finish the comment trim — the test file was untouched last round The previous commit trimmed the lint script's citations and left the test file entirely alone, so the review found 12 person/PR references and long explanatory blocks still in `tests/lint-resolver-stub-arity.test.py`. Measured across the WHOLE diff this time rather than the file the review named, which is how the sibling got missed twice: before after added lines citing a person/PR 12 0 added comment runs > 2 lines 25 0 scripts/lint-hermetic-bridge-tests 11 0 tests/lint-resolver-stub-arity 14 0 Each surviving comment states the durable invariant: that patching an imported tree needs an intermediate name; that `finally` runs on every path so OR-merging it keeps the prior state alive; that `else` runs after a successful body; that a class namespace is not a lexical scope for its methods; that a sub-walk dropping `ever_unsafe` loses module late-binding. Repros, reviewer names and PR numbers are in the PR body and the review thread. Two scoping mistakes caught before they shipped: - A first pass collapsed comment blocks across the whole lint script and deleted 236 lines, most of them pre-existing. Reverted; the trim now applies only to blocks whose every line is in this diff. Pre-existing deletions are down to the two the PR genuinely modifies (`if not new_violations` gaining `and not new_stubs`, and the summary f-string gaining the stub counter) — verified both still present, not lost. - Separating blocks with a bare `#` line does not end a comment run, so the first count barely moved (25 -> 23). A real blank line does. tests/lint-resolver-stub-arity.test.py PASS tests/lint-hermetic-bridge-tests.test.py PASS review-checks.sh PASS (hardcoded-paths clean) mutation: neuter the try/orelse sequencing -> suite rejects it Stand: Echo Act IV Mini * fix(lint): rewrite the trimmed comments as complete invariants, not truncated sentences The previous commit satisfied the two-line cap by keeping each block's first two lines, which cut sentences mid-clause. Comments ended `still append to`, `even though`, `OR-merging it`, `A bare` — the cap met, the constraint gone. That is worse than the over-long original: the reader loses the history AND the reason. All 15 blocks of mine rewritten by hand as complete <=2-line invariants: fragment-ending blocks I added 15 -> 0 remaining in the file 2, both PRE-EXISTING (L358, L782) — untouched Scoped by merge-base, so the two pre-existing fragments stay as they are rather than being swept into a comment refactor. The failure generalises past this PR and is recorded rather than just fixed: the same mechanical rule ran over 10 of my PRs tonight and left 42 truncated blocks. My own compliance metric (runs > 2 lines) went 25 -> 0 while the artifact got worse, because a line count cannot see whether the retained line still says anything. A comment cannot be trimmed mechanically; each block has to be rewritten, and one that cannot be stated in two complete lines should be deleted instead. tests/lint-resolver-stub-arity.test.py PASS tests/lint-hermetic-bridge-tests.test.py PASS mutation: neuter the try/orelse sequencing -> suite rejects it Stand: Echo Act IV Mini * fix(lint): trim the five docstrings this PR added to the 2-line cap Bassil and Qingyun both flagged the comment policy at the current head, independently. The five docstrings this PR introduces run past CLAUDE.md's 2-line/no-narration rule: _lambda_absorbs_args, _unsafe_names_in_scope, _ScopeWalk, scan_resolver_stubs, and resolver_stub_violations, whose 28-line body carried a worked repro, an inline code example, and narrative about the function's own history. Each keeps the load-bearing invariant its name and signature cannot express, and nothing else. The worked examples move to the PR description. Scoped to what this PR added. The file has 18 other over-cap docstrings that predate it; trimming those here would bundle an unrelated cleanup into a review that is already two rounds deep. Behaviour unchanged, same numbers the reviewer measured: tests/lint-resolver-stub-arity.test.py all checks passed scripts/lint-hermetic-bridge-tests.py 69 scanned, 47 grandfathered, 423 scanned for resolver stubs Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(lint): an ANNOTATED assignment bypassed the resolver-stub gate `_ScopeWalk.visit()` dispatched only `ast.Assign`, so an annotated zero-arg stub passed the mandatory lint while the identical plain form was caught: wd.resolve_workspace = lambda: tmp -> [(2, 'resolve_workspace')] wd.resolve_workspace: object = lambda: tmp -> [] <- green, unsafe _fake: object = lambda: tmp ; wd.resolve_workspace = _fake -> [] <- same, via alias A gate that reports green on the shape it exists to catch is worse than no gate, and this one guards workspace isolation. `ast.AnnAssign` now routes through the same binding logic, in both places that needed it: `visit()` dispatch and `_unsafe_names_in_scope`, so an annotated ALIAS reaches the gate too. New `_assign_targets()` normalises `.targets` (Assign) against `.target` (AnnAssign) and returns none for a BARE annotation, which binds nothing at runtime and must not flag. Controls, with the source reverted and the tests kept: FAIL zero-arg via an ANNOTATED assignment FAIL zero-arg via an ANNOTATED alias, then a plain rebind ok ANNOTATED absorbing lambda is still SAFE <- passes in BOTH states ok a BARE annotation binds nothing <- passes in BOTH states The two negative controls are what stop "flag every AnnAssign" from satisfying the positives. Mandatory lint unchanged at 69 bridge-importing tests / 423 scanned for resolver stubs. Found by qingyun-wu in review. Stand: Echo Act IV Mini * fix(lint): keep the two cited comments inside the two-line cap Removes the review-history phrase from the counter-case comment and compresses the annotated-assignment note from three lines to two. Both keep the invariant they were stating; only the narration is dropped. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>

sonichi mentioned this pull request Aug 4, 2026

tests write into the live state/ tree, corrupting agent control signals #2616

Open

qingyun-wu previously requested changes Aug 4, 2026

View reviewed changes

sonichi mentioned this pull request Aug 4, 2026

fix(tests): reply-directive fixture wrote to the owner's live workspace #2619

Merged

sonichi requested a review from qingyun-wu August 4, 2026 08:22

john-the-dev requested changes Aug 4, 2026

View reviewed changes

qingyun-wu approved these changes Aug 4, 2026

View reviewed changes

qingyun-wu previously requested changes Aug 4, 2026

View reviewed changes

sonichi requested review from john-the-dev and qingyun-wu August 4, 2026 08:38

john-the-dev requested changes Aug 4, 2026

View reviewed changes

sonichi mentioned this pull request Aug 4, 2026

lint: extend hermetic-tests check to WORKSPACE isolation, not just CLAUDE_CONFIG_DIR #2621

Closed

qingyun-wu requested changes Aug 4, 2026

View reviewed changes

john-the-dev approved these changes Aug 4, 2026

View reviewed changes

qingyun-wu approved these changes Aug 4, 2026

View reviewed changes

sonichi mentioned this pull request Aug 4, 2026

fix(lint): a test can be CCD-hermetic and still write to the live workspace #2622

Merged

john-the-dev closed this Aug 4, 2026

john-the-dev reopened this Aug 4, 2026

sonichi closed this Aug 4, 2026

sonichi reopened this Aug 4, 2026

john-the-dev enabled auto-merge (squash) August 4, 2026 13:55

john-the-dev added 5 commits August 4, 2026 07:11

Merge branch 'main' into fix/two-more-fixtures-write-live-workspace

1c147f0

Merge branch 'main' into fix/two-more-fixtures-write-live-workspace

21c5c30

Merge branch 'main' into fix/two-more-fixtures-write-live-workspace

4cc280f

Merge branch 'main' into fix/two-more-fixtures-write-live-workspace

e1e4e1d

Merge branch 'main' into fix/two-more-fixtures-write-live-workspace

9f2e220

john-the-dev merged commit ca62f0f into main Aug 4, 2026
15 of 16 checks passed

john-the-dev deleted the fix/two-more-fixtures-write-live-workspace branch August 4, 2026 14:25

sonichi mentioned this pull request Aug 4, 2026

fix(tests): cron-notify stamped the operator's real cooldown state #2641

Merged

Conversation

sonichi commented Aug 4, 2026

What

Verification — stated precisely, because my first attempt at it was wrong

What is deliberately NOT here

Shape

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 4, 2026

Coverage Gate

Diff Coverage

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

Uh oh!

sonichi commented Aug 4, 2026

My own false path, since it's the same lesson

Uh oh!

sonichi commented Aug 4, 2026

Uh oh!

sonichi commented Aug 4, 2026

Uh oh!

john-the-dev left a comment

Choose a reason for hiding this comment

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

sonichi commented Aug 4, 2026

Uh oh!

john-the-dev left a comment

Choose a reason for hiding this comment

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

john-the-dev left a comment

Choose a reason for hiding this comment

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

john-the-dev commented Aug 4, 2026

Uh oh!

john-the-dev commented Aug 4, 2026

Uh oh!

github-actions Bot commented Aug 4, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants