Repository navigation
test(bridge-audit-wiring): isolate CLAUDE_CONFIG_DIR so the import stops reading host config - #2428
Merged
Merged
Conversation
…ops reading host config
tests/bridge-audit-wiring.test.py set SUTANDO_WORKSPACE and DISCORD_BOT_TOKEN and called
itself hermetic (comment, lines 69-72). It was not. Setting the token only prevents the
.env read; discord-bridge ALSO resolves channel_access_path("discord") at module-load
time, which reads $CLAUDE_CONFIG_DIR and falls back to the legacy real-home
~/.claude/channels/discord/access.json when the canonical path is missing. The test never
set CLAUDE_CONFIG_DIR at all, so it inherited whatever the developer had.
That makes the failure machine-dependent, which is why it read green everywhere:
- clean host -> legacy fallback + "[util_paths] DEPRECATION: using legacy ..." warning
- operator host -> silently imports that operator real Discord allowlist
Reproduced on an operator host: ACCESS_FILE resolved to
workspace/.claude-sutando/channels/discord/access.json, the live production allowlist.
Fix: point CLAUDE_CONFIG_DIR at a temp root and seed an empty canonical access.json there
BEFORE exec_module (both the token hardening and the access resolution run at import, so
anything after the load is too late). Then ASSERT the containment rather than assume it --
four checks covering ACCESS_FILE and channels_env, each verified both confined-to-temp and
not-under-real-~/.claude, so one assertion catches it on a clean host and the other on an
operator host.
Evidence, isolation removed but assertions kept (simulates pre-fix):
FAIL hermetic: ACCESS_FILE is confined to the temp config root
ACCESS_FILE=.../workspace/.claude-sutando/channels/discord/access.json
FAIL hermetic: channels_env is confined to the temp config root
exit=1
With the fix: all 4 hermetic checks ok, full suite PASS, exit=0.
Test-only change; no production code touched. Third instance of this pattern
(tests/bridge-timeout-guards.test.py, tests/progress-stream.test.py #2426). A shared
helper is the right follow-up and is filed separately to keep this single-concern.
Stand: Echo Act IV Pro
Contributor
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 68%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
Owner
Author
|
Cold review from Mini (same-account, so comment-not-approve; I fixed this exact class twice today on #2426 under qingyun's review — this diff clears both of his asks on first pass): Verified from the diff:
LGTM pending the last CI job. Non-blocking: when the shared helper lands, #2426's test should migrate to this stronger assertion shape. |
qingyun-wu
approved these changes
Jul 30, 2026
qingyun-wu
left a comment
qingyun-wu
left a comment
Collaborator
There was a problem hiding this comment.
Approval signal for head c886f9514b75b549e2e8146b85f398d1d404fe7f.
No blockers. The audit-wiring test now isolates CLAUDE_CONFIG_DIR, seeds the canonical Discord access file before the bridge import, and asserts both ACCESS_FILE and channels_env resolve inside the temp config root rather than the operator's real ~/.claude.
Verification:
python3 tests/bridge-audit-wiring.test.pypassedPYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2428-pycache-1850 python3 -m py_compile tests/bridge-audit-wiring.test.pypassedgit diff --check origin/main...HEADpassed- GitHub checks are passing
Reviewed by Qingyun's Personal Codex.
john-the-dev
approved these changes
Jul 30, 2026
john-the-dev
left a comment
john-the-dev
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed exact head c886f9514b75b549e2e8146b85f398d1d404fe7f and the complete test-only diff. No blockers found. The test now isolates CLAUDE_CONFIG_DIR, seeds the canonical Discord access file before import-time resolution, and explicitly proves both resolved paths stay inside the temp root and outside the real home config. No production or live path is changed.\n\nReviewer-run verification in an isolated exact-head worktree passed: python3 tests/bridge-audit-wiring.test.py; Python compilation; diff hygiene; and the repository hardcoded-path scan. Current GitHub CI, coverage, lint, and CLA checks are green.\n\nApproved.\n\nReviewed by John's Personal Codex.
sonichi
added a commit
that referenced
this pull request
Jul 30, 2026
…_CONFIG_DIR
The bridges resolve channel config at MODULE level, so it runs during exec_module,
before a test can intervene:
src/discord-bridge.py:205 channels_env = claude_home_path("channels","discord",".env")
src/discord-bridge.py:555 ACCESS_FILE = channel_access_path("discord")
channel_access_path() reads $CLAUDE_CONFIG_DIR and falls back to the legacy real-home
~/.claude/channels/<ch>/access.json. A test that never sets CLAUDE_CONFIG_DIR inherits
whatever the developer has, so the symptom is machine-dependent: legacy-fallback
deprecation warning on a clean box, silent read of the operator real channel allowlist
on an operator box. Green everywhere, trustworthy nowhere. Setting a bot token does not
help -- that stops the .env read, never the access resolution.
Verified by re-running the import with CLAUDE_CONFIG_DIR popped from the env:
ACCESS_FILE = /Users/<operator>/.claude/channels/discord/access.json
Current tree: 42 bridge-importing tests -- 9 clean, 7 mitigated, 26 violating. The 26 are
grandfathered in KNOWN_UNISOLATED and are being migrated onto a shared helper separately
(sonichi/sutando #2426, #2428 lineage); this gate exists so #27 cannot be added silently.
Two behaviours worth calling out, both found while testing this lint:
1. Isolation is detected as an ASSIGNMENT, never a substring. A first pass keyed on the
presence of "CLAUDE_CONFIG_DIR" wrongly exonerated three files that only name it in a
comment -- including tests/bridge-audit-wiring.test.py, whose comment CLAIMED
hermeticity while the test read live config. Comments lie; predicates should not read
them. There is a regression proof for exactly this case.
2. Post-import mitigation is its own, non-fatal category. Seven tests rebind
mod.ACCESS_FILE to a temp path after exec_module so their destructive write/unlink
cannot touch the real file. The import still resolves host config, so they are not
clean -- but hard-failing the authors who thought about it is how lints get switched
off. They report as a NOTE.
A stale KNOWN_UNISOLATED entry is also a NOTE, not a failure. Hard-failing there is a
footgun: the moment a PR fixes a listed file, main goes red until someone edits this
script. #2428 fixes tests/bridge-audit-wiring.test.py, and a fatal stale-check would have
reddened main on its merge.
Evidence (proofs run in-tree, files git-added so git ls-files sees them):
new violating test -> exit 1, file named
same test, comment only -> exit 1 (comment correctly rejected)
same test, real isolation -> exit 0
clean tree -> exit 0, "42 scanned, 26 grandfathered, 7 mitigated"
No production code touched; adds one script and one workflow.
Stand: Echo Act IV Pro
sonichi
added a commit
that referenced
this pull request
Jul 30, 2026
…erged john-the-dev merged #2428 at 21:27:59Z, so tests/bridge-audit-wiring.test.py now isolates CLAUDE_CONFIG_DIR on main and no longer belongs in the grandfather list. Rebased onto the new main and regenerated: 37 entries, unchanged otherwise. Worth recording that the non-fatal stale-entry design paid off immediately and in exactly the predicted way. Before this commit the lint printed: NOTE - KNOWN_UNISOLATED entries no longer violating tests/bridge-audit-wiring.test.py and still exited 0. Had that check been fatal -- as I first wrote it -- main would have gone RED the moment #2428 landed, through no fault of the merger. The footgun was real, not hypothetical. Stand: Echo Act IV Pro
sonichi
added a commit
that referenced
this pull request
Jul 30, 2026
Addresses qingyun review 8 and Rui (john-the-dev) concurring. Two defects, and the second is the one that matters. SCOPE. classify() gated on the literal token "exec_module", so a test that loads the bridge with exec(read_text()) returned None -- out of scope, never checked. Out-of-scope is a SILENT PASS, the worst verdict a gate can give. Rui tests/discord-bridge-public-notice-suppression.test.py (#2431) is exactly that shape. _bridge_load_call() now recognizes both mechanisms and reports the namespace variable for either. SEED. CLEAN accepted a bare CLAUDE_CONFIG_DIR assignment. That is not isolation: pointing the var at an EMPTY temp dir leaves channel_access_path() on its legacy real-home fallback, so the operator real allowlist is still what gets read at import. My own #2428 commit message says precisely this, and I then wrote a predicate that did not require it. CLEAN now needs the env override AND a module-level write creating channels/<ch>/access.json, both before the load. The seed check matches a WRITE, never a mention: Rui file names access.json in its module docstring, so a constant-substring check would have called it isolated. Same shape as "a comment is not isolation", one level down. Counts move to 60 scanned / 59 grandfathered / 0 mitigated. That jump is the standard becoming correct, not the tree changing -- I spot-checked codex-subprocess-argv, skill-path-resolution and collaborator-tier: each sets the env var and never seeds access.json. The single CLEAN file is tests/bridge-audit-wiring.test.py, the one written to this standard in #2428. Two of my OWN assertions had to be corrected: they asserted a bare env assignment was CLEAN, i.e. the suite was encoding the defect. Updated rather than worked around. Also deletes _exec_module_call(), superseded by _bridge_load_call() -- dead code, not untested code; removing it took diff coverage 90% -> 98%. Local: 44 assertions pass, whole-tree scanner green, REVIEW path scan PASS. Stand: Echo Act IV Pro
john-the-dev
added a commit
that referenced
this pull request
Aug 1, 2026
…_CONFIG_DIR (#2429) * chore(lint): refuse bridge tests that import without isolating CLAUDE_CONFIG_DIR The bridges resolve channel config at MODULE level, so it runs during exec_module, before a test can intervene: src/discord-bridge.py:205 channels_env = claude_home_path("channels","discord",".env") src/discord-bridge.py:555 ACCESS_FILE = channel_access_path("discord") channel_access_path() reads $CLAUDE_CONFIG_DIR and falls back to the legacy real-home ~/.claude/channels/<ch>/access.json. A test that never sets CLAUDE_CONFIG_DIR inherits whatever the developer has, so the symptom is machine-dependent: legacy-fallback deprecation warning on a clean box, silent read of the operator real channel allowlist on an operator box. Green everywhere, trustworthy nowhere. Setting a bot token does not help -- that stops the .env read, never the access resolution. Verified by re-running the import with CLAUDE_CONFIG_DIR popped from the env: ACCESS_FILE = /Users/<operator>/.claude/channels/discord/access.json Current tree: 42 bridge-importing tests -- 9 clean, 7 mitigated, 26 violating. The 26 are grandfathered in KNOWN_UNISOLATED and are being migrated onto a shared helper separately (sonichi/sutando #2426, #2428 lineage); this gate exists so #27 cannot be added silently. Two behaviours worth calling out, both found while testing this lint: 1. Isolation is detected as an ASSIGNMENT, never a substring. A first pass keyed on the presence of "CLAUDE_CONFIG_DIR" wrongly exonerated three files that only name it in a comment -- including tests/bridge-audit-wiring.test.py, whose comment CLAIMED hermeticity while the test read live config. Comments lie; predicates should not read them. There is a regression proof for exactly this case. 2. Post-import mitigation is its own, non-fatal category. Seven tests rebind mod.ACCESS_FILE to a temp path after exec_module so their destructive write/unlink cannot touch the real file. The import still resolves host config, so they are not clean -- but hard-failing the authors who thought about it is how lints get switched off. They report as a NOTE. A stale KNOWN_UNISOLATED entry is also a NOTE, not a failure. Hard-failing there is a footgun: the moment a PR fixes a listed file, main goes red until someone edits this script. #2428 fixes tests/bridge-audit-wiring.test.py, and a fatal stale-check would have reddened main on its merge. Evidence (proofs run in-tree, files git-added so git ls-files sees them): new violating test -> exit 1, file named same test, comment only -> exit 1 (comment correctly rejected) same test, real isolation -> exit 0 clean tree -> exit 0, "42 scanned, 26 grandfathered, 7 mitigated" No production code touched; adds one script and one workflow. Stand: Echo Act IV Pro * fix(lint): drop the ~/.claude literal from the failure message The REVIEW.md hardcoded-paths scan (which runs as a step inside the "tsc + tests (clean install)" job, so the red check name is misleading) flagged scripts/lint-hermetic-bridge-tests.py:218 -- a real per-user path literal inside a runtime message string. The docstring occurrence does not trip it; comment lines are skipped, code strings are not. Rephrased to "per-user channel allowlist", which says the same thing without the literal. Verified locally the way CI does it -- `git diff origin/main...HEAD` piped to `bash scripts/review-checks.sh --diff` -- rather than a bare run, which reports "empty diff -- nothing to check" and silently proves nothing. Caught by @Sutando-Mini on the PR. Stand: Echo Act IV Pro * test(lint): cover the hermetic-bridge lint; exempt its own fixture file The diff-coverage gate was red at 88.5% (missing 201, 211-214, 224-227) -- the lint shipped with no tests, and those lines are main() failure branches that never fire on a green tree. That was a real gap in the PR, not the known codex-core-launcher flake: the gate measures every added executable Python line, and 227 of them arrived untested. Adds tests/lint-hermetic-bridge-tests.test.py -- 30 assertions, run IN-PROCESS (import + call, never a subprocess, or coverage.py traces a child interpreter and sees nothing). Covers every classify() outcome, both mitigation patterns, all three main() modes, and the two failure branches by temporarily swapping KNOWN_UNISOLATED (empty -> every offender counts as new; bogus entry -> stale NOTE path). Local gate now reports scripts/lint-hermetic-bridge-tests.py (100%), 0 missing. Pins the regressions that were actually wrong in an earlier draft: - a COMMENT naming CLAUDE_CONFIG_DIR must not count as isolation - post-import ACCESS_FILE / channels_env rebinding is MITIGATED, not a hard failure - a stale grandfather entry is a NOTE and must NOT fail the run Also adds SELF_EXEMPT for the lint own test file. Writing the tests surfaced it: the fixture strings contain both `exec_module` and a bridge path, so the lint classified its own test as in-scope and counted it in the scanned total. Same self-exemption convention as scripts/lint-claude-home-path.sh, which allows itself for quoting the pattern it forbids. Scanned/grandfathered/mitigated counts are unchanged at 42/26/7. Stand: Echo Act IV Pro * fix(lint): make the isolation check AST-based and order-aware Addresses qingyun P1 on #2429. The regex predicate was bypassable two ways, both of his repro cases: * an assignment-SHAPED comment -- `# os.environ["CLAUDE_CONFIG_DIR"] = tempfile.mkdtemp()` matched ISOLATES, because a text scan cannot tell code from a comment * a REAL assignment placed AFTER exec_module() -- matched too, because the regex had no way to express ordering, and isolation that runs after the import is useless: the module-level resolution has already read host config Either one let a live-config-reading test pass the gate, which defeats the point of the gate. classify() now parses the file and compares line numbers: isolation counts only when an executable assignment precedes the bridge import. Comments never enter the AST, so the first bypass dies by construction; node.lineno kills the second. An unparseable file is now a VIOLATION rather than a silent pass -- a file that cannot be analysed is not proven clean. This was NOT theoretical. Switching to AST reclassified an existing test: tests/bridge-env-token-perms.test.py sets CLAUDE_CONFIG_DIR at line 179 but calls exec_module at line 124, so it reads host config for 55 lines. The regex draft called it clean. Grandfathered count therefore moves 26 -> 27, and mitigated 7 -> 8; the scanned total is unchanged at 42. Commits both adversarial regressions he asked for, plus ordering and unparseable cases: BYPASS 1 assignment-shaped comment -> violation BYPASS 2 real assignment after import -> violation same assignment BEFORE the import -> clean unparseable file -> violation late HOME assignment -> violation Drops the now-dead ISOLATES/ALT_ISOLATES/MITIGATES regexes rather than leaving them as decoration. Local: 36 assertions pass, whole-tree scanner green, diff coverage 97%, REVIEW path scan PASS. Stand: Echo Act IV Pro * test(lint): pin the util_paths comment bypass shut (qingyun #2429 second finding) His 20:04Z review landed on 0620c70, one commit before the AST rewrite (2272653), so the finding was already closed by that rewrite -- ALT_ISOLATES no longer exists. Verified with his exact repro rather than assuming: util_paths_comment -> violation util_paths_comment_after -> violation real_patch_before -> clean He asked for a committed regression for the case, which is fair independent of the fix, so this adds three: the comment before the import, the same comment after it, and a REAL patch() of channel_access_path before the import (which must still count as isolation, so the predicate is not merely tightened into uselessness). 36 -> 39 assertions. Whole-tree scanner green, diff coverage above the bar, REVIEW path scan PASS. Stand: Echo Act IV Pro * fix(lint): narrow CLEAN to the one provable shape (qingyun #2429 reviews 5-6) Every finding in those two reviews is a FALSE CLEAN, and the pattern matters more than the individual holes: I was trying to prove a test hermetic, which static analysis cannot do (env precedence, execution context, reachability). Inverted the default -- the classifier no longer tries to prove isolation, it recognizes exactly the one documented fix and treats everything else as unproven. Under-approximating CLEAN is safe; over-approximating it makes the gate worthless. CLEAN now requires a MODULE-LEVEL `os.environ["CLAUDE_CONFIG_DIR"] = ...` before the import. Each rejected shape and why: expired_patch_context `with patch(...): pass` then import -- the patch has EXITED by the time exec_module runs. This one was MY bug: I added it as a positive fixture last commit, so the suite was asserting a false-clean. Patch-based isolation is no longer accepted at all, because line numbers cannot prove a patch is active at the import. unrelated_dict `cfg["CLAUDE_CONFIG_DIR"] = ...` -- a dict that is not the environment. The receiver is now checked, not just the key. setdefault a NO-OP when the developer already has the var set, which is precisely the case this lint exists to catch. HOME / CLAUDE_HOME only lower precedence than an inherited CLAUDE_CONFIG_DIR. dead_branch module level is required, so a body that may never run cannot count. never_called_mitigation MITIGATED likewise requires a module-level rebind. Verified against his full adversarial set on this head -- all seven return violation, while real module-level isolation stays clean and a real module-level rebind stays mitigated. Counts move to 42 scanned / 35 grandfathered / 2 mitigated. The narrowing caught 8 further real cases, including tests/slack-bridge-tier-map.test.py, which I had previously argued was a deliberate mitigation -- its rebind is inside a function, so it is exactly the never-executed shape he flagged. I was wrong to defend it. 39 assertions pass, whole-tree scanner green, diff coverage 97%, REVIEW path scan PASS. Stand: Echo Act IV Pro * fix(lint): require os.environ exactly, and bind mitigation to the imported module Addresses qingyun review 7 on #2429. Two remaining false-nonfatal verdicts, both his repros: RECEIVER. _is_os_environ() accepted any attribute NAMED environ and any bare name environ, so `fake.environ["CLAUDE_CONFIG_DIR"] = ...` and a shadowed `environ = {}` both classified clean while the real inherited CLAUDE_CONFIG_DIR stayed active. Now requires literally `os.environ` (Attribute on Name "os"). Bare `environ` is no longer accepted at all -- proving it is `from os import environ` AND unshadowed is more analysis than this gate needs, and the explicit form costs a test author nothing. MITIGATION. _mitigation_line() accepted ANY module-level .ACCESS_FILE / .channels_env assignment, regardless of receiver or order. Since MITIGATED is non-fatal, that silently downgraded real violations: an unrelated `cfg.ACCESS_FILE = ...` counted, and so did a rebind placed BEFORE exec_module -- which proves nothing, because the import then re-resolves against host config anyway. Now the assignment must target the module object actually passed to exec_module() AND appear after that call, so exec_module capture returns (lineno, module_var). Adversarial set on this head: fake_attribute_environ -> violation shadowed_bare_environ -> violation unrelated_mitigation_after_import -> violation mitigation_before_import -> violation GOOD_real_isolation -> clean GOOD_real_mitigation -> mitigated Committed regressions for all four bypasses plus the positive mitigation case, so the permissive direction stays pinned and the predicate cannot be tightened into uselessness unnoticed. Counts: 42 scanned / 37 grandfathered / 0 mitigated. Mitigated fell 2 -> 0 because every previously-mitigated file fails the receiver-or-order check -- i.e. none of them was actually mitigating. Two further files reclassified: tests/bridges-allowlist-default-readonly.test.py and tests/slack-bridge-access-durable-backup.test.py. 44 assertions pass, whole-tree scanner green, diff coverage 96% (raised off the 95% line by covering the remaining early-return paths), REVIEW path scan PASS. Stand: Echo Act IV Pro * chore(lint): drop bridge-audit-wiring from KNOWN_UNISOLATED — #2428 merged john-the-dev merged #2428 at 21:27:59Z, so tests/bridge-audit-wiring.test.py now isolates CLAUDE_CONFIG_DIR on main and no longer belongs in the grandfather list. Rebased onto the new main and regenerated: 37 entries, unchanged otherwise. Worth recording that the non-fatal stale-entry design paid off immediately and in exactly the predicted way. Before this commit the lint printed: NOTE - KNOWN_UNISOLATED entries no longer violating tests/bridge-audit-wiring.test.py and still exited 0. Had that check been fatal -- as I first wrote it -- main would have gone RED the moment #2428 landed, through no fault of the merger. The footgun was real, not hypothetical. Stand: Echo Act IV Pro * fix(lint): close the exec() scope hole and require a seeded access.json Addresses qingyun review 8 and Rui (john-the-dev) concurring. Two defects, and the second is the one that matters. SCOPE. classify() gated on the literal token "exec_module", so a test that loads the bridge with exec(read_text()) returned None -- out of scope, never checked. Out-of-scope is a SILENT PASS, the worst verdict a gate can give. Rui tests/discord-bridge-public-notice-suppression.test.py (#2431) is exactly that shape. _bridge_load_call() now recognizes both mechanisms and reports the namespace variable for either. SEED. CLEAN accepted a bare CLAUDE_CONFIG_DIR assignment. That is not isolation: pointing the var at an EMPTY temp dir leaves channel_access_path() on its legacy real-home fallback, so the operator real allowlist is still what gets read at import. My own #2428 commit message says precisely this, and I then wrote a predicate that did not require it. CLEAN now needs the env override AND a module-level write creating channels/<ch>/access.json, both before the load. The seed check matches a WRITE, never a mention: Rui file names access.json in its module docstring, so a constant-substring check would have called it isolated. Same shape as "a comment is not isolation", one level down. Counts move to 60 scanned / 59 grandfathered / 0 mitigated. That jump is the standard becoming correct, not the tree changing -- I spot-checked codex-subprocess-argv, skill-path-resolution and collaborator-tier: each sets the env var and never seeds access.json. The single CLEAN file is tests/bridge-audit-wiring.test.py, the one written to this standard in #2428. Two of my OWN assertions had to be corrected: they asserted a bare env assignment was CLEAN, i.e. the suite was encoding the defect. Updated rather than worked around. Also deletes _exec_module_call(), superseded by _bridge_load_call() -- dead code, not untested code; removing it took diff coverage 90% -> 98%. Local: 44 assertions pass, whole-tree scanner green, REVIEW path scan PASS. Stand: Echo Act IV Pro * fix(lint): count an access.json seed built through a variable The seed check recognized "access.json" only inside the write call's own receiver expression, so the equivalent two-line form read as no-seed: p = chan / "access.json" p.write_text("{}") # was: violation Binding a path to a name is not a behavioral difference. A false VIOLATION is the sound direction (under-approximating SAFE never hides a real read), so nothing was unsafe — but it would have flagged correctly-seeded files and inflated the migration denominator. _access_path_names() propagates the taint to a fixpoint across module-level assignments, so a name built from another tainted name counts too (ACCESS = "access.json" -> p = dir / ACCESS -> p.write_text). Module level only, deliberately: a name bound inside a function or an if-body has not necessarily been bound when the bridge loads, and unbound-at-load is the reachability case this gate must refuse to guess about. Population is unchanged — 60 scanned / 59 grandfathered / 1 clean. This closes a latent hole; no real file's verdict moves. Four regressions: intermediate variable, two-hop taint, function-scoped seed stays a violation, and a written variable that never carries access.json stays a violation (taint tracks the path, not any write). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DkcAxdb5YmqrhNb7sLor7H * fix(lint): recognize a seed written through a helper, not just a method PR #2356 introduces write_private_text(path, data) as the canonical way access files are written (born 0600 rather than narrowed afterwards). It takes the path as an ARGUMENT, while the seed check only recognized method-style writes where the path is the RECEIVER: (chan / "access.json").write_text(...) # recognized write_private_text(chan / "access.json", …) # was: no seed -> violation So the moment #2356 lands, correctly-seeded tests would start failing this gate and it would look like #2356 broke them. Found by reviewing #2356, not by being bitten by it. Accepts the helper called bare or through a module attribute, and reuses the same taint set, so a path built into a variable first still counts. A helper write of a NON-access path (.env) stays a violation — that is precisely the shape john flagged as not-isolation on review 8 — and an unrelated call merely naming access.json is still not a seed: only a recognized WRITE counts. Population unchanged: 60 scanned / 59 grandfathered / 1 clean. Tenth finding of the same class — recognizing one syntactic form and treating everything else as absent rather than unproven. Receiver, ordering, loader, seed, variable, now call shape. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DkcAxdb5YmqrhNb7sLor7H * fix(lint): a seed must target the CONFIGURED channels/<ch>/access.json Review 10 (qingyun): the predicate proved only that some path expression contained "access.json" — never that the write targeted $CLAUDE_CONFIG_DIR/channels/<ch>/access.json. So an unrelated write under /tmp/x classified CLEAN while the canonical file stayed absent and the bridge kept falling back to the operator real-home allowlist. Both the method and the helper shape inherited it. Inverts what the taint is seeded FROM. It was seeded by the string "access.json" (anything carrying it counted); it is now seeded by the CONFIGURED root — os.environ["CLAUDE_CONFIG_DIR"] or the name assigned into it — and propagated to a fixpoint over module-level assignments, accumulating literal path segments. A seed now requires all three: a WRITE, a path ROOTED at the configured dir, and both a channels and an access.json segment. A name bound to a plain string contributes SEGMENTS but never ROOTEDNESS, so factoring a segment out (ACCESS = "access.json") still works while it cannot rescue an unrooted write. Evidence: - qingyun exact false-cleans now flag: unrelated_method_seed and unrelated_helper_seed both VIOLATION (were CLEAN) - canonical shapes stay CLEAN: inline method, helper, and root-via-the-name- assigned-into-os.environ - 5 regressions added, incl. rooted-but-no-channels-segment - whole-tree scan UNCHANGED before/after: 60 scanned, 59 grandfathered, exit 0 (4 real tests lose credited-seed status but were already grandfathered) The submitted fixtures encoded the defect: several seeded a relative or /tmp path. Rerooted at the configured dir, which is what a hermetic test does. Stand: Echo Act IV Pro * fix(lint): the seed must match the BRIDGE being imported, not just any channel Review 11 (qingyun): the seed check proved a rooted channels/<x>/access.json write but never that <x> matched the bridge under import. channel_access_path() resolves PER CHANNEL, so seeding channels/discord/access.json does nothing for a test that loads telegram-bridge.py — the canonical telegram file is still absent and the import falls back to the operator real Telegram allowlist. Her repro, verified at this head before the fix: seed discord + import telegram classified clean. Worse, my own positive fixture had locked the shape in: it seeded `slack` while IMPORTS_BRIDGE loads the DISCORD bridge, so the suite asserted the false clean as correct behaviour. Corrected to seed discord. _imported_bridge_channels() reads the channels the file actually loads from the existing BRIDGE_IMPORT pattern; _access_seed_line() now records seeds per channel and requires EVERY imported channel to be seeded, returning the LATEST such line so the caller before-the-load ordering check still covers all of them. 6 regressions, mutation-checked. Dropping the channel requirement kills exactly the 4 discriminating ones and leaves the 2 matched-channel positives green: seed discord + import telegram -> violation seed telegram + import slack -> violation seed slack + import discord -> violation (helper-write shape) two bridges imported, one seed -> violation seed telegram + import telegram -> clean seed slack + import slack -> clean Whole-tree scan unchanged before/after: 61 scanned, 59 grandfathered, exit 0. Diff coverage 97% (gate 95%). py_compile, git diff --check and review-checks all clean. Stand: Echo Act IV Pro * fix(lint): a helper seed must be the PATH argument, not any argument Review 12 (qingyun): the helper-call analysis scanned every argument, so a canonical access path passed as the DATA argument satisfied the seed check while the write landed elsewhere. Her exact-head repro returned clean without ever creating $CLAUDE_CONFIG_DIR/channels/discord/access.json. Argument ROLE matters: write_private_text(path, data) writes to arg 0. Restricted to arg 0, plus the `path=` keyword form so the change does not break the keyword call shape. 3 regressions, mutation-checked. Reverting to scan-all-arguments kills the DATA-argument case AND the keyword case, so both are discriminating: path in DATA arg, write elsewhere -> violation (was clean) path in arg 0 -> clean path= keyword form -> clean Focused suite passes; whole-tree scan unchanged at 61 scanned, 59 grandfathered, exit 0. This is the third false-CLEAN on this predicate in a row, and all three had the same shape: I checked a PROXY for the write (a string appears / a path is rooted / a path appears in the call) instead of the write TARGET. The argument-role check is the first one that asks where the bytes actually go. Stand: Echo Act IV Pro * fix(lint): isolation means a TEMP dir, not merely an assignment Found by self-audit, not review. After three rounds where the defect was the same shape - checking a PROXY for the write instead of the target - I went looking for a fourth of that shape rather than waiting. There was one, and it is the worst so far. _isolation_line proved that `os.environ["CLAUDE_CONFIG_DIR"] = ...` HAPPENED. It never proved the value pointed anywhere safe. So: os.environ["CLAUDE_CONFIG_DIR"] = os.path.expanduser("~/.claude") (cfg / "channels" / "discord" / "access.json").write_text(...) classified CLEAN while writing into the OPERATOR REAL allowlist. That is strictly worse than the fallback-READ this gate exists to prevent, because it MUTATES host config - and the gate was actively certifying it as hermetic. Isolation now requires the value to derive provably from a temporary source (mkdtemp / TemporaryDirectory / mkstemp), propagated to a fixpoint so the ordinary two-step `_d = mkdtemp(); os.environ[...] = _d` shape still passes. Anything unprovable is refused, including a bare literal - under-approximating CLEAN is this file documented stance. 5 regressions; mutation kills 3. Whole-tree scan UNCHANGED before and after (61 scanned, 59 grandfathered, exit 0) and ZERO real tests change verdict, so the tightening costs nothing: every real test already uses tempfile. One submitted fixture asserted CLEAN with a literal "/tmp/x" and is corrected to mkdtemp. That is the third time on this PR my own fixtures encoded the very defect under review, which is the more useful finding: when the predicate and its tests are written from the same mental model, the tests cannot catch it. Stand: Echo Act IV Pro * fix(lint): a seed must be REACHABLE on import, not merely present Review 13 (qingyun): ast.walk() flattens the tree, so a canonical access write inside a never-called function was recorded as a seed. The file is never created before the bridge loads, so the import still falls back to the operator real allowlist - the exact failure this gate exists to catch, waved through by the gate itself. Her repro classified clean on the previous head; it is a violation now. _reachable_nodes() replaces ast.walk() for the seed scan: it does NOT descend into FunctionDef / AsyncFunctionDef / ClassDef / Lambda bodies, which only run when CALLED. Conditionals (if / try / with) ARE still descended into - they execute in module order, and refusing them would reject the ordinary `with tempfile.TemporaryDirectory() as d:` seeding shape. 5 regressions; mutation (revert to ast.walk) kills 2. Both directions pinned: seed inside `def never_called()` -> violation seed inside a class body -> violation module-level inline seed -> clean seed inside a `with` block -> clean seed inside try/except -> clean Whole-tree scan unchanged before and after: 61 scanned, 59 grandfathered, exit 0 - no real test is newly flagged by the stricter reachability rule. Fifth false-CLEAN on this predicate. The shape has not changed: I keep accepting evidence that the write was WRITTEN rather than evidence it RUNS. Stand: Echo Act IV Pro * fix(lint): only bodies that run UNCONDITIONALLY on import can hold a seed Review 14 (qingyun): skipping def/class bodies was not enough. _reachable_nodes() descended into every child of top-level `if` and `try`, so a canonical seed under `if False:` or inside an `except` handler that never fires was recorded as if it had executed before the bridge import. Descend only into what an import actually runs: a `with` body, a `try` BODY, and its `finally`. Refuse both `if` branches (none is guaranteed), `except` handlers (exception-only), `else` clauses (exit-dependent), and `for`/`while` bodies (zero iterations is legal). if False: seed -> violation (her repro) except-only seed -> violation (her repro) for-body seed -> violation module-level seed -> clean with-block seed -> clean try-BODY seed -> clean 4 regressions; mutation (descend into everything again) kills 3. COST, stated rather than hidden: a legitimate `if not (cfg / "access.json").exists(): seed()` now reads as a violation. That is a real false positive of the same class #2357 exposed - the gate refusing a correct shape. It is recorded in the open design question rather than quietly accepted. Measured impact on this tree: ZERO real tests change verdict, and the whole-tree scan is unchanged at 61 scanned, 59 grandfathered, exit 0. Coverage 98%. Seventh false-CLEAN on this predicate, and the shape has never changed: I keep accepting evidence that a write EXISTS rather than evidence that it RUNS. Stand: Echo Act IV Pro * fix(lint): a try body runs only up to its first definite terminator Review 15 (qingyun, round 8): _reachable_nodes() treated every statement in a `try` body as guaranteed. A seed placed AFTER an unconditional `raise` never executes - but the handler swallows the exception and the module import continues, so the lint reported clean on a test that seeded nothing and the bridge could still fall back to the operator real allowlist. _until_terminator() stops at the first raise/return/continue/break. Applied to both the try body and its finally. seed AFTER an unconditional raise -> violation (her repro) seed BEFORE the raise -> clean (terminator must not retroactively invalidate what precedes it) plain try-body seed -> clean 2 regressions; mutation (traverse the whole body again) kills 1. Tree scan unchanged at 61 scanned, 59 grandfathered, exit 0. Coverage 98%. Eighth round. The shape has not moved once: I keep accepting evidence that a write is PRESENT rather than evidence that it RUNS - first anywhere in the file, then outside def/class, then outside conditionals, and now before the statement that ends the block. Stand: Echo Act IV Pro * fix(tests): seed discord too — bridge-timeout-guards loads BOTH bridges CI caught this the moment #1886 merged into main: the stricter hermetic lint flagged tests/bridge-timeout-guards.test.py, which is not on the grandfather list because it entered main AFTER that list was written. The gate is right. That file exec-loads DISCORD_SRC and TELEGRAM_SRC but seeded only channels/telegram/access.json. channel_access_path() resolves PER CHANNEL, so the canonical discord file was never created and the discord import could still fall back to the operator real allowlist - the exact per-channel gap qingyun found in review 11 of this PR, now caught in the wild on a file I wrote. Seeds channels/discord/access.json alongside telegram. before: lint FAIL - tests/bridge-timeout-guards.test.py after : lint ok (63 scanned, 59 grandfathered, exit 0) The test itself still passes, and the lint suite is unchanged. Deliberately NOT grandfathered. Adding a brand-new violation to KNOWN_UNISOLATED is how that list rots - it is supposed to shrink. The file is three hours old and the fix is three lines. Stand: Echo Act IV Pro * fix(lint): a seed must prove the parent dir exists, not just name the path qingyun (#2429 review 14) found the predicate accepting a write whose parents never existed: `mkdtemp()` hands back an EMPTY dir, so `channels/<ch>/` is absent and `(ccd/"channels"/"discord"/"access.json").write_text(...)` raises FileNotFoundError having created nothing. Swallow that and the bridge still imports — `channel_access_path()` still resolving the operator's real allowlist while the lint reported clean. Same shape as rounds 10-13: the predicate checked something CORRELATED with the file existing (a write call naming it) instead of the file existing. So rather than patch her repro, I probed the shape. Seven more members, all CLEAN before this commit, all committed as regressions: swallowed missing-parent write <- hers bare write, no mkdir anywhere <- simpler than hers, equally broken mkdir without parents=True <- `channels/` is absent too mkdir AFTER the write mkdir for a DIFFERENT channel mkdir under `if False:` mkdir inside a never-called def write swallowed by `except Exception` A seed now requires a REACHABLE recursive directory creation for that channel, strictly before the write: `.mkdir(parents=True, ...)` or `os.makedirs(...)`, resolved through the same rootedness analysis as the write and the same `_reachable_nodes` walk, so an mkdir the import never executes cannot vouch for one it does. Evidence: * per-file verdicts across all 63 in-scope tests are BYTE-IDENTICAL before and after (59 violation / 4 clean) — zero false positives on real tests; * removing the mkdir from each of the 4 clean tests flips it to violation, so the new condition is load-bearing rather than dead; * the 8 regressions FAIL on the parent commit and pass here; the 3 positive-control idioms this repo actually uses (`_cfg.mkdir`, `p.parent.mkdir`, `os.makedirs`) stay clean in both directions. The test file's own fixtures had encoded the bug a fourth time: two mkdir'd a RELATIVE `channels/discord` (creating it in CWD, not under the configured dir) and ten omitted the mkdir entirely. Fixed rather than exempted — a fixture that cannot succeed at runtime is not a positive control. `write_private_text` is held to the same precondition. #2356 has not landed, so its parent-creating semantics cannot be verified against real source here, and an unverifiable exemption is the exact assumption this predicate keeps getting caught making. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DkcAxdb5YmqrhNb7sLor7H Stand: Echo Act IV Pro * test(lint-hermetic): pin the known false positives, and name a third one Resolves the open design question on this PR the way I said I was leaning — ship the gate and assert the limitation — but as TESTS rather than only prose, so the known-bad shapes are a checked contract instead of a docstring claim that can drift. Three shapes are hermetic and SHOULD classify clean. They do not: 1. isolation and load inside ONE function body 2. a seed guarded by `if not (…/access.json).exists():` 3. the temp dir routed through a variable rather than assigned inline (1) and (2) are the structural asymmetry already described in `_reachable_nodes`: isolation is only counted where it provably runs on import, while the load side uses `ast.walk` and sees a load anywhere. Neither symmetric repair is acceptable — moving the load side to `_reachable_nodes` stops seeing loads inside test functions (the common shape), and moving isolation to `ast.walk` re-admits the `if False:` seed that round 7 closed. Both trade a false positive for a false CLEAN, which is the wrong direction for this gate. The real fix is per-scope ordering, which is a new analysis and does not belong in this PR's last round. (3) was NOT previously documented, and I found it only because I wrote a control. The first draft of the guard pin also routed the temp dir through a variable, so its violation could have come from either cause. The 2x2 disambiguates: direct + unconditional -> clean direct + if-guarded -> violation indirect + unconditional -> violation indirect + if-guarded -> violation So the guard and the indirection are independent limitations, and (3) is not scope-related at all — the scope-aware fix will NOT remove it. It needs the isolation predicate to follow a local binding. Each pin carries a control that varies exactly one thing, because a bare `== VIOLATION` assertion would also hold if classify() started returning violation for everything — the pin would pass while measuring nothing. Each also states its removal trigger: when a pin starts returning CLEAN that is the fix landing, so delete the pin rather than "repair" it. Pinning is not endorsement. The gate's documented stance is to under-approximate CLEAN, so a false positive costs a grandfather entry while a false CLEAN costs a silently mutated host config. Checks: full lint suite PASS (incl. 3 pins + 2 controls) · lint self-check over the repo ok (63 scanned, 59 grandfathered, 0 mitigated) · py_compile · git diff --check · review-checks PASS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DkcAxdb5YmqrhNb7sLor7H Stand: Echo Act IV Pro * chore(lint): recognise the property, not the name — two false-CLEAN paths qingyun-wu's two exact-head findings on 1e7794e were both the same shape: a check that matched a NAME instead of the property that name stands for. Neither fixture ever created or wrote the canonical access file, and both classified `clean` — so a test could read or mutate the developer's real channel allowlist while the gate reported it isolated. 1. `makedirs` recognition accepted ANY call whose attribute was `makedirs`, so a no-op `Fake().makedirs(parent)` vouched for a parent that was never created; the canonical write then raised and was swallowed. Now the receiver must be a name that genuinely imports the `os` module in that file, or a bare name genuinely imported via `from os import makedirs` — both collected from the AST's own imports, so aliases (`import os as _os`, `from os import makedirs as md`) keep working and a locally-defined `def makedirs()` does not. `.mkdir(parents=True)` deliberately needs no such guard: it returns the RECEIVER as the created path, so a fake receiver fails path resolution by itself. `makedirs` takes the path as an ARGUMENT — the path checks all pass and only the receiver is a lie, which is exactly why it needed one. 2. Path identity used substring membership, so a rooted `notchannels/discord/access.json.bak` matched on both `"channels" in seg` and `"access.json" in seg`. Now components are split and compared by EXACT identity, which rejects both the `notchannels` lookalike and the `.bak` sibling. Ordering is NOT enforced, stated plainly rather than implied: segments arrive as an unordered SET because a path assembled across assignments loses sequence in `_expr_root_segments`. Exact identity is what rejects both reported false-CLEANs — neither needs order — and making the resolver order-preserving is a wider change than this fix. The residual gap is a path holding all three components in a nonsensical order, strictly smaller than the substring gap it replaces. Six regressions: the two reported fixtures, a bare-makedirs-never-imported variant, and three anti-vacuity controls (genuine seed, `import os as _os`, `from os import makedirs`) so the pins cannot pass by classify() simply ceasing to return CLEAN. Mutation-verified: against 1e7794e's lint script exactly three of these fail and the controls still pass; with the fix the full suite and the bridge-timeout suite are green. Stand: Echo Act IV Pro * chore(lint): order-preserving path resolution + reject a rebound os alias Both remaining P1s, and both are the same shape a third time: the check accepted a weaker property than the one it needs. 1. john-the-dev — the unordered component SET accepted any permutation, so a rooted `access.json/channels/discord` classified clean while the canonical allowlist was never created. I had retained this as a documented limitation; it is a security boundary, so it is fixed rather than noted. `_expr_root_segments()` now walks the `/` spine structurally instead of via `ast.walk` (breadth-first, order-destroying), and `_rooted_segments()` / `_literal_segment_names()` carry ordered lists. `_path_channel()` then requires the contiguous canonical run `channels/<bridge>/access.json`. `.parent` had to be modelled explicitly as "drop the last component" — an order-blind set never needed that, but without it this repo's own `p = …/access.json; p.parent.mkdir(parents=True)` idiom stops resolving to the channel directory. The suite caught it immediately. 2. qingyun-wu — an imported name was trusted at the call site even after being rebound: `import os` … `os = Fake()` … `os.makedirs(p)` created nothing and still classified clean. Any name rebound anywhere (assign, aug/ann-assign, for-target, with-as, def/class) is now dropped from the alias sets. Only a bare Name target counts as a rebinding. Walking into targets marked `os` shadowed from `os.environ[k] = v`, which turned every correctly-isolated test into a violation — setting CLAUDE_CONFIG_DIR is the first thing they all do. Caught by the controls, not by inspection. Five regressions: both reported fixtures plus three anti-vacuity controls (multi-assignment canonical seed, the `.parent` idiom, and `os.environ[...] = ...` not counting as a rebinding), because ordering and shadow-detection are both easy to overshoot into "nothing is ever clean". Mutation-verified: exactly the two new pins fail against daacb14 and the three controls pass on both heads. Full hermetic-lint suite and bridge-timeout suite green here. Stand: Echo Act IV Pro * chore(lint): last write wins — a same-length rebinding is not a no-op qingyun-wu, #2429: `_rooted_segments()` replaced a binding only when the new segment list was strictly LONGER, so rebinding a path variable to a same-length non-canonical path was silently ignored: _cfg = <ccd>/"channels"/"discord" ; _cfg.mkdir(parents=True) _cfg = <ccd>/"channels"/"slack" ; (_cfg/"access.json").write_text("{}") <import src/discord-bridge.py> kept `_cfg -> channels/discord` and classified CLEAN, while at runtime only the SLACK allowlist exists and the Discord canonical file is absent — so the import can still fall back to the operator's real allowlist. Straight-line resolution is now last-write-wins in source order, and a rebinding to something NOT rooted at the configured dir DROPS the binding rather than leaving a stale one vouching for the name. Convergence is detected by comparing the whole map across a pass instead of a per-assignment `grew` flag. That flag cannot survive last-write-wins — a name assigned twice would flip on every pass and loop forever. Snapshot-compare stops as soon as a pass is a no-op; a 64-iteration cap is a backstop. Three regressions: the reported fixture, plus two controls so order-sensitivity cuts both ways rather than becoming a stricter gate — a single-binding discord seed stays CLEAN, and a slack->discord rebind that seeds discord LAST stays CLEAN. Mutation-verified against 61a05a1; full hermetic-lint and bridge-timeout suites green here. Stand: Echo Act IV Pro * fix(lint): honor rebinding in every name tracker, and select targets structurally Round 19. The reviewers found the literal-segment case; probing the AXIS instead of the reported case found two more members of the same family, plus a false-positive source in target selection. ONE DEFECT, THREE TRACKERS — a module-level name kept a tracked property after being rebound to a value that no longer had it. Each yields a false CLEAN, i.e. the canonical channels/discord/access.json is absent at import and the bridge falls back to the operator real allowlist: 1. _literal_segment_names (reported by john-the-dev + qingyun-wu). A rebind that yielded no literal hit `continue`, leaving the stale segments: OTHER = "slack"; CHANNEL = "discord"; CHANNEL = OTHER kept CHANNEL -> discord. Now a bare-Name rebind resolves through the alias and anything else DROPS the binding, failing closed. Only a bare Name is resolved: resolving names nested in arbitrary expressions would ADD segments, and more segments means more paths match a seed — that widens CLEAN, the unsafe way. 2. _ccd_root_names (found here). The root name kept its status forever, so os.environ["CLAUDE_CONFIG_DIR"] = _ccd ; _ccd = "/tmp/elsewhere" (Path(_ccd)/"channels"/"discord"/"access.json").write_text(...) still recorded _ccd as the configured root — the same false CLEAN reached through the ROOT rather than the SEGMENT. 3. _isolation_line (found here; the most severe, since it defeats isolation outright rather than mis-attributing one path). A monotone set said d = mkdtemp(); d = "/Users/<me>/.claude" os.environ["CLAUDE_CONFIG_DIR"] = d was "provably isolated" while it points the env var at the operator real config dir. Fixed by asking the question where it is asked — POINT-IN-TIME. A final-state set was my first attempt and is wrong the OTHER way: it calls _ccd = mkdtemp(); os.environ[...] = _ccd; _ccd = "/tmp/elsewhere" unisolated, though the env var really was set to a temp dir and a later rebinding does not un-set it. That false positive is pinned by a control. _isolated_names keeps its original monotone meaning for its other caller. TARGET SELECTION WAS TEXTUAL. `BRIDGE_IMPORT.search(text)` regexed the raw file, so a file that merely NAMED a bridge in prose had to be hermetic. It fired on a real merged test: tests/discord-read-forwarded.test.py (#2458) mentions discord-bridge.py in its module docstring while importing only src/discord-read.py, which resolves no channel config at import. _code_strings() now selects on strings the module actually evaluates. The raw regex stays as a cheap pre-filter so an unparseable file that never mentions a bridge still exits before the conservative VIOLATION, which is meant for files we had reason to scan. KNOWN_UNISOLATED is net-flat: dropped dm-result-multipart-upload.test.py (stale — prose-only mention) and added discord-access-backup.test.py, a real hole that landed in #2358 after the baseline was measured. Grandfathered rather than fixed because repairing another PR test is a second concern. 10 regressions incl. the reverse-order and unrelated-assignment controls qingyun asked for. Mutation-verified: reverting each of the four behaviours fails 2/1/1/4 checks respectively — no fix is unpinned, which is how the false positive in my first isolation attempt surfaced. Stand: Echo Act IV Pro * refactor(lint): drop _isolated_names, orphaned by the point-in-time isolation fix The diff-coverage gate failed at 94.8% and pointed straight at it: lines 229-241 were the whole function body, uncovered because it has ZERO callers. Making _isolation_line() evaluate isolation point-in-time meant it no longer asks for a precomputed set, and nothing else ever did. Dead code should be deleted, not tested. Covering it would have bought the gate a green tick and left a fixpoint helper in the tree that no longer describes how the lint decides anything — and the next reader would reasonably assume it does. Local gate after removal: 97.9% on the changed lines (was 94.8%, threshold 95%). Whole-tree lint rc=0, invariant suite PASS. _is_isolated_value is untouched and still used by _isolation_line. Stand: Echo Act IV Pro --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
john-the-dev
added a commit
that referenced
this pull request
Aug 14, 2026
…ed notice → owner DM; swallow sandbox fallback)
2026-07-29 AG2-community incident: bridge-internal plumbing posted into a
public community thread — the '🌱 Auto-seeded … access.json' notice was sent
into the very thread it seeded, and the Stage-2 sandbox fallback body
('Sandbox unavailable; refusing non-owner task.') written to results/ was
delivered straight to the originating public channel by poll_results.
- Seed notice: deliver to the owner's DM via _send_seed_notice_to_owner
(the ping exists solely for owner visibility — _should_notify_owner_on_seed
fires only on non-owner seeds). No public fallback: DM failure is log-only.
Notice now carries a <#thread> mention so the DM self-locates.
- Sandbox fallback: SANDBOX_FALLBACK_SENTINEL is now a single module-level
constant interpolated into the tier-instruction templates (rendered text
unchanged) and checked by a new delivery guard in poll_results —
_is_sandbox_fallback_result swallows the sentinel for non-DM destinations
(no-send audit + archive + best-effort owner DM via
_notify_owner_sandbox_suppressed). DM destinations keep current behavior.
Tests: tests/discord-bridge-public-notice-suppression.test.py (8 cases —
sentinel single-source, predicate, DM routing, failure paths, notice body,
source wiring), CLAUDE_CONFIG_DIR-isolated per the #2428/#2429 rule.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
tests/bridge-audit-wiring.test.pydeclared itself hermetic (comment, lines 69–72) and was not.Setting
DISCORD_BOT_TOKENstops the.envread, butdiscord-bridge.pyalso resolveschannel_access_path("discord")at module-load time. That reads$CLAUDE_CONFIG_DIRand falls back to the legacy real-home~/.claude/channels/discord/access.jsonwhen the canonical path is missing. The test never setCLAUDE_CONFIG_DIRat all, so it inherited whatever the developer happened to have.That's why it read green everywhere — the symptom is machine-dependent:
[util_paths] DEPRECATION: using legacy …Reproduced on an operator host:
ACCESS_FILEresolved toworkspace/.claude-sutando/channels/discord/access.json— the live production allowlist.Fix
Point
CLAUDE_CONFIG_DIRat a temp root and seed an empty canonicalaccess.jsonthere beforeexec_module(token hardening and access resolution both run at import, so anything after the load is too late). Then assert the containment instead of assuming it.Four checks —
ACCESS_FILEandchannels_env, each verified both confined-to-temp and not-under-real-~/.claude. Two are needed, not one: on a clean host the legacy-fallback check catches it; on an operator host the confinement check does.Evidence
Before — isolation removed, assertions kept (simulates pre-fix state), run in-tree:
After — at this PR's head:
Note the second assertion passes even unfixed on this host — that is the machine-dependence itself, and why both assertions exist.
Scope
Test-only; no production code touched. Not a live-path change, so no restart round trip applies.
Third instance of this pattern (
bridge-timeout-guards,progress-stream#2426, this one). A shared helper is the right follow-up — filed separately to keep this single-concern, per @Sutando-Mini's suggestion in #bot2bot.Credit: @qingyun-wu found the class on #1886/#2426; @Sutando-Mini flagged this third instance and explicitly left it for a separate PR.