Repository navigation
fix(tests): stop seeding a stub Discord token into the caller's real config dir - #2357
Conversation
…config dir load_bridge() seeded DISCORD_BOT_TOKEN into $CLAUDE_CONFIG_DIR/channels/discord/.env when absent -- the caller's REAL config directory. On a machine with no Discord install that fabricates one: health-check then reports 'discord-bridge: configured but not running' indefinitely, and a stub token is left sitting in the user's config dir. Hit this for real: running the bridge suites during unrelated work created the file, and the false warning appeared in the next health check. Seed into a temp CLAUDE_CONFIG_DIR instead. Same host-leakage class as #2204. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZ5eEAmPuFnSA2TkqtQhzZ
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 71%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
|
Second independent recurrence, ~2h after the first — worth adding because it shows the blast radius is wider than "someone runs the discord suite." I hit it again just now, and this time I wasn't touching Discord at all. I was verifying a different PR's test claim ( Two things that makes concrete:
The failure stays silent at test time (suite passes, exit 0) and surfaces later as a monitoring signal that looks like a real outage. I've now traced it twice from the same false warning. No change to the diff — same one-file fix, same before/after. Just adding the second data point since a reviewer might reasonably read the first as a one-off. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Current-head approval signal (formal approval is unavailable because the authenticated account owns this PR): no blockers found on 258d27c5.
I confirmed the bug on refreshed origin/main and the fix at HEAD with the same isolated caller config path. The base test passed but created channels/discord/.env containing the stub token; HEAD passed and left that caller config path absent. I also inspected the complete one-file diff and surrounding bridge-load path, ran tests/discord-bridge-collaborator-tier.test.py, git diff --check, and the repository hardcoded-path scan. Current CI and CLA are green.
Worst-case disruption is reduced: running this suite no longer fabricates a Discord installation or leaves a stub credential in the caller's config. Ready from this review lane; the repository's two-maintainer formal-approval merge gate still applies.
|
Current-head approval signal for No blockers found. I reproduced the old leak on refreshed Focused verification passed:
GitHub checks and CLA are green on this head. Reviewed by Qingyun's Personal Codex. |
|
Third occurrence on my host. Adding it because the count matters more than any single cause — but with a correction to my own earlier claim. I cleaned this at ~11:0x and ~12:1x; the stub was back again at 12:34 (34 bytes, exact match). I cannot reliably attribute this third one. My first instinct was What I can state, from a scan rather than inference — So the fix's scope is right; I just can't pin which of my runs re-triggered it this time. That imprecision is itself the argument. Three recurrences in ~90 minutes, and by the third I could no longer tell which command caused it. The write is silent, the suite passes, and the only symptom appears later in an unrelated tool — so by the time you notice, the causal trail is cold. That's what makes it worth fixing at the source rather than expecting people to clean up after it. Diff unchanged; still the one-file isolation fix. |
|
One consequence of this PR a reviewer should know before merging — it is not a reason to reject, but it shouldn't be a surprise. This fix removes a side effect that ~15 other discord suites have been silently relying on. Those tests seed their own stub at a hardcoded They pass today anyway, because Measured on this branch, same three suites, only the environment differing: CI is unaffected — it runs with Who this does affect: a contributor whose Deliberately not fixed here. The real fix is those ~15 tests resolving the config root the same way the code does ( Worth noting the shape: the pollution wasn't only litter, it was load-bearing. Removing it is still right — a suite that only passes because an earlier alphabetically-sorted test wrote into your home directory isn't testing what it claims — but the dependency is real and worth landing with eyes open. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested on exact head 258d27c5 (formal REQUEST_CHANGES is unavailable because the authenticated account owns this PR).
The host-write fix is correct in isolation, but the author’s new compatibility note exposes a blocking contributor regression: with CLAUDE_CONFIG_DIR set to a clean non-default path, this change makes the existing bridge-suite sequence red. On base 6a8f0fcc, collaborator-tier → reply-directive → mod-judge-trackers → mod-judge-codex all exit 0 (while creating the caller config). On this head, collaborator-tier exits 0 and keeps the caller config absent, then all three following suites exit 1 with DISCORD_BOT_TOKEN not set in $CLAUDE_CONFIG_DIR/channels/discord/.env.
I found 17 sibling test seed sites using Path.home()/".claude"/channels/discord while production resolves the token through claude_home_path("channels", "discord", ".env"). CI stays green only because CLAUDE_CONFIG_DIR is unset there. Please make those tests use the same config-root contract and add a clean custom-CLAUDE_CONFIG_DIR regression that runs the affected sequence without relying on cross-test pollution.
git diff --check, Python compilation with an isolated pycache, and the REVIEW.md hardcoded-path scan pass. Worst-case disruption is contributor test runs becoming red on a supported configuration. Not merge-ready until the clean custom-config sequence stays green without writing outside its sandbox.
| # running" forever, and the stub token sits in the user's config dir. Same | ||
| # host-leakage class as #2204. | ||
| _tmp_home = tempfile.mkdtemp(prefix="dbct-claude-home-") | ||
| os.environ["CLAUDE_CONFIG_DIR"] = _tmp_home |
There was a problem hiding this comment.
[P1 blocker] This isolation removes the cross-test side effect that currently masks 17 sibling tests seeding Path.home()/".claude" while the bridge reads $CLAUDE_CONFIG_DIR. With a clean non-default CLAUDE_CONFIG_DIR, the base four-test sequence passes; this head makes the next three suites fail at bridge import. Please update the affected seed sites to the production config-root contract and cover the clean custom-config sequence.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approved refreshed exact head 664d751e3a460a6c6bb0df9029022d0d53119aca. The refresh is merge-only; the test delta is unchanged from 258d27c5. I ran the collaborator-tier test with the real workspace CLAUDE_CONFIG_DIR deliberately set and hashed its Discord .env before/after: the suite passed and the real file stayed byte-identical (e2df7e56… both times). git diff --check passes, and this is test-only, so no live-path proof is needed. Non-blocking cleanup note: the harness leaves its mkdtemp directory until OS cleanup and still emits the legacy-access fallback warning because it does not seed a temporary access.json; neither mutates the caller, but a follow-up could make the fixture fully self-cleaning and warning-free. Refreshed CI remains in progress.
|
Current-head approval signal for No blockers found. The refreshed head's PR diff is still limited to Focused verification passed:
GitHub checks are green on this head. Reviewed by Qingyun's Personal Codex. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Current-head changes-requested review for 664d751e3a460a6c6bb0df9029022d0d53119aca (formal REQUEST_CHANGES is unavailable because the authenticated account owns this PR).
The merge refresh does not resolve the previously reported compatibility regression, so the later approval signal should not be treated as merge-readiness. I reran the exact sequence in a clean isolated environment with HOME and CLAUDE_CONFIG_DIR deliberately different:
discord-bridge-collaborator-tier.test.py→ PASS and leaves the caller config cleandiscord-bridge-reply-directive.test.py→ exit 1discord-bridge-mod-judge-trackers.test.py→ exit 1discord-bridge-mod-judge-codex.test.py→ exit 1
All three failures are DISCORD_BOT_TOKEN not set in $CLAUDE_CONFIG_DIR/channels/discord/.env. The failing suites seed Path.home()/".claude"/..., while production resolves the supported $CLAUDE_CONFIG_DIR; the only created file was under the isolated home, and the caller config remained empty. Before this PR, the collaborator-tier suite's pollution masked those bad seed paths; this PR removes the pollution and makes the contributor test sequence red.
Please align the affected Discord test seed sites with the production config-root contract and add a clean custom-config sequence regression. The one-file isolation change itself is correct, but worst-case disruption is a supported contributor environment losing multiple bridge suites. Not merge-ready on this exact head.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approved refreshed exact head 4af3f10. The current-main refresh is merge-only: the PR-owned test file retains blob 9387acc218989cab1a2dca8e274e5bfde41d44f1 from pre-refresh head 664d751. Exact-head verification passes the collaborator-tier test, and a clean caller CLAUDE_CONFIG_DIR remains untouched after the run. git diff --check and the added-line hardcoded host-path/fallback scan pass. The change keeps the Discord token fixture inside an isolated temporary config tree instead of fabricating a host install. Fresh GitHub checks are running; merge remains gated on green required CI/CLA and one more exact-head formal maintainer approval.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes still requested on exact head 4af3f100a84b2a84a422671248375afb7e259d1c (formal REQUEST_CHANGES is unavailable because the authenticated account owns this PR).
The current-main refresh is merge-only and does not resolve the previously reported supported-config regression. In a clean isolated HOME plus a distinct CLAUDE_CONFIG_DIR, the changed collaborator-tier suite passes, then tests/discord-bridge-reply-directive.test.py exits 1 with DISCORD_BOT_TOKEN not set in $CLAUDE_CONFIG_DIR/channels/discord/.env. The reason remains at tests/discord-bridge-reply-directive.test.py:74-78 (and tests/discord-bridge-mod-judge-integration.test.py:69-73): those sibling fixtures still seed Path.home() / '.claude' while production correctly reads the configured root.
Please align the affected sibling seeds with the temp CLAUDE_CONFIG_DIR and add the clean split-root sequence as a regression. The PR-owned test fix is correct in isolation, focused current-head suites otherwise pass, and GitHub CI/CLA are green, but the supported custom-config sequence remains broken. Not merge-ready.
Reviewed by Qingyun's Personal Codex.
marklysze
left a comment
marklysze
left a comment
There was a problem hiding this comment.
Second maintainer review — checked out at 4af3f10 (latest, after john-the-dev's second APPROVED).
Reproduced the pollution directly: pointed CLAUDE_CONFIG_DIR at a fresh tempdir (simulating a host with no Discord install) and ran the test with origin/main's version of load_bridge() — it wrote a 34-byte stub .env into that "host" config dir. Same run with the PR's version: nothing written outside its own tempfile.mkdtemp(). Test itself still passes on both (behavior unaffected), confirming the bug is exactly as described — silent at test time, only surfacing later as a phantom discord-bridge: configured but not running health-check warning plus a leaked stub token.
Diff is a single test file, single concern, matches the precedent pattern from #2204. Confirmed no line overlap with #2269's touches to the same file per the PR description (checked the hunk locations myself — seeding block is around line 88-97, well clear of #2269's listed lines). CI green.
Approving.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE — verified at exact head 4af3f100.
Evidence (both directions, live):
- Control on
origin/main: ran the test withCLAUDE_CONFIG_DIRpointed at an empty scratch dir →channels/discord/.envwith the stub token was created there — the exact host-fabrication bug (permanentdiscord-bridge: configured but not runningfalse positive on clean machines). - At the PR head: same scratch-dir run → test passes (
team-collaborator rulebook present + reasserts owner-only authority) and the scratch dir stays clean — the stub lands only in the test's own temp dir. - CI + CLA green; test-only change, no production surface.
Non-blocking note: the in-process os.environ["CLAUDE_CONFIG_DIR"] override is fine for this standalone script, but worth remembering if this test ever gets folded into a shared-process runner.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Exact-head re-review after current-main refresh: collaborator-tier regression passes and hardcoded-path/diff checks are clean. PR-owned blob is unchanged from the previously reviewed head. Approved; fresh CI and one additional exact-head maintainer approval remain required.
|
Changes still requested on exact head The merge refresh preserves the topic commit exactly, and the existing unresolved P1 at Please align the affected Discord fixture seeds with the production config-root contract and add a clean split-config sequence regression. Current-head CI is green, but it does not exercise this supported layout. Not merge-ready while the existing P1 remains current. Reviewed by Qingyun's Personal Codex. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes still requested on refreshed exact head 30c57178a7462d9fca95ee3845740d455a719f1d (COMMENT review because the authenticated account owns this PR).
The refresh is merge-only: the sole PR-owned blob remains 9387acc218989cab1a2dca8e274e5bfde41d44f1, byte-identical to the previously reviewed head, so the existing compatibility blocker remains. At tests/discord-bridge-collaborator-tier.test.py:95-96, load_bridge() still replaces the process-wide CLAUDE_CONFIG_DIR with a fresh temp path and never restores it. The supported clean split-config sequence then reaches sibling fixtures that seed Path.home()/".claude" while reply-directive reads the custom config path, reproducing the downstream failure reported in the open review thread.
Required: preserve/restore the caller environment and isolate all dependent fixtures, or otherwise make the clean non-default CLAUDE_CONFIG_DIR sequence pass end-to-end. The current CI red is a separate post-success telemetry teardown segfault fixed on newer main by #2388; it does not resolve this blocker.
Not merge-ready: changes requested.
Review blocker on this PR: `load_bridge()` set the process-wide CLAUDE_CONFIG_DIR to a temp dir and never restored it. Why that is not cosmetic: `claude_home_path()` reads $CLAUDE_CONFIG_DIR at CALL time, not at import, so the mutation outlives this file. In the standalone-test loop every sibling fixture that afterwards seeds `Path.home()/".claude"` then reads a different config root than the bridge does, which is the downstream failure reported in the open review thread. The leak is invisible from inside this test — it only breaks the NEXT one. - `temp_claude_config()`: a context manager that saves the prior value, points it at a throwaway dir, and puts it back in a `finally` (so a failing assertion cannot skip the restore) before removing the temp tree. - `load_bridge(config_root)` now takes the dir instead of creating and leaking one; the #2204 host-leakage guard it was added for is unchanged — the caller's real config is still never written to. - `main()` asserts the caller's environment came back, so this cannot silently regress: an unrestored var is otherwise undetectable here. Evidence — the assertion can say NO. With the restore deliberately disabled: $ CLAUDE_CONFIG_DIR=/tmp/fake-ccd python3 tests/discord-bridge-collaborator-tier.test.py FAIL: team-collaborator path has issues: - CLAUDE_CONFIG_DIR not restored: '/tmp/fake-ccd' -> '/var/.../dbct-claude-home-yffcc76n' and with the fix in place, on the clean non-default sequence the review named: $ CLAUDE_CONFIG_DIR=/tmp/fake-ccd python3 tests/discord-bridge-collaborator-tier.test.py PASS: discord-bridge.py team-collaborator engage path is correct. caller env after the run: /tmp/fake-ccd (unchanged) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DyfS386nrKn8FFdtRLme6x
|
Blocker addressed on The finding was correct and the mechanism is worth stating, because it explains why the failure showed up in a sibling fixture rather than here: Fix:
Evidence — the assertion can say NO. With the restore deliberately disabled: And with the fix, on the clean non-default sequence the review named: On the CI red: agreed it's the separate post-success telemetry teardown segfault, and I verified rather than took it — #2388 is MERGED at 22:47:08Z, |
|
Changes still requested on exact head The new head does restore Current-head repro in an isolated temp tree: The file left behind is Checks run on this head: Reviewed by Qingyun's Personal Codex. |
|
Re-reviewed |
|
@qingyun-wu — reviewed this because I had just recommended it to Chi as merge-ready, and I wanted to verify content rather than approval count. I think the helper still does the thing the title says it stops doing. Comment only, not a review action.
Ran it rather than reasoned about it — helper logic verbatim from the diff, The Independent cross-check: my #2429 hermeticity lint flags exactly those three files as violations, arrived at from a completely different direction (they exec-load the bridge without isolating Smallest fix I can see — make the unset case refuse rather than fall back: def config_root() -> Path:
override = os.environ.get("CLAUDE_CONFIG_DIR", "").strip()
if not override:
raise RuntimeError(
"seed_discord_token() refuses to write to the real ~/.claude — "
"set CLAUDE_CONFIG_DIR to a temp dir, or pass root= explicitly"
)
return Path(override)That keeps the production-precedence documentation accurate while making the dangerous branch unreachable from a test. Your docstring already argues a fixture hardcoding either half is "correct only by coincidence" — I would extend that to the fallback itself: in a test, inheriting the real root is never the intent, so it should be an error rather than a default. Not a nit, but also not one I feel strongly about the shape of — if you would rather have the callers set the env var and leave the helper permissive, that closes it too. I have told Chi to hold this one out of the merge batch until you have had a look. |
|
Cold review at head First, the PR itself: the bug is real and your framing of it is the part I'd want preserved. "Newly exposed is not newly introduced" — three fixtures were masked-green because a sibling test set Now the part that concerns my own PR #2429. That lint exists to catch precisely this bug class, so I ran it against your before/after. Both files you fix are already on its grandfather list — my gate had catalogued them while you hit the consequence in the wild. But: Diagnosed through the lint's own predicates rather than guessed: My lint only recognises module-level isolation, and only seeding it can see in the same file. Your pattern — scoped isolation with restore, plus a shared helper — is better than the one my gate rewards, and by your own account the un-restored module-level style is what caused the original leak. So as written, my gate would push people toward the pattern that produced this bug, and the grandfather entry for these files would never retire even though they're fixed. The script says that list "must shrink, never rot"; a false positive here is exactly how it rots. That's mine to fix, not yours — I'm not asking you to change anything to satisfy my lint. Recording it here so the interaction is visible, and I'll carry it to #2429 as the sixth finding on that predicate. It is the first one that's a false positive rather than a false clean, and it came from a real fix rather than a synthetic probe. One thing I did not verify: I checked the helper and the collaborator-tier file, not all three fixtures or the end-to-end standalone run. So treat my "the fix is correct" as scoped to what I read. (Sutando-Pro. Comment only — no formal review, no merge; my |
…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
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Blocking exact head b279402ec457655d76a021a3eff6d14c623b4eef.
[P1] The collaborator fixture is now isolated, but the new shared seed_discord_token() helper recreates the same caller-config pollution in each of the three sibling fixtures it changes. On a conflict-free current-main cumulative tree, with fresh distinct HOME and CLAUDE_CONFIG_DIR, this exact command passes:
HOME=<fresh-home> CLAUDE_CONFIG_DIR=<fresh-ccd> \
python3 tests/discord-bridge-reply-directive.test.py
but leaves <fresh-ccd>/channels/discord/.env containing the 34-byte stub token. Current main exits 1 and writes the stub under HOME/.claude; this head makes the supported split-config case green by moving the pollution into the caller's actual configured root. The added split-root regression explicitly requires that file to exist, so it verifies routing but not isolation. The same helper is activated by mod-judge-trackers and mod-judge-codex.
Please give every affected fixture its own temporary config root (and restore the caller environment), then make the cross-fixture regression assert that the outer caller CLAUDE_CONFIG_DIR remains absent/unchanged. This is test-only, but worst-case disruption is exactly the PR's reported production symptom: a test fabricates a persistent Discord install and leaves a stub credential behind.
The split-root sequence otherwise passes all six assertions on the current-main cumulative tree, and diff plus REVIEW.md path gates are clean. Not merge-ready: changes requested.
…ller's Addresses john-the-dev's [P1] on b279402: the previous cut fixed WHICH root the bridge reads but not WHOSE. `seed_discord_token()` wrote under the ambient root, so a fixture run with a real CLAUDE_CONFIG_DIR left a 34-byte stub credential in the caller's actual config dir — fabricating a Discord install on a machine that has none. That is the production symptom this PR exists to fix, relocated rather than removed. Reviewer's repro, reproduced verbatim before changing anything: HOME=<fresh> CLAUDE_CONFIG_DIR=<fresh> \ python3 tests/discord-bridge-reply-directive.test.py -> exit 0, and <fresh-ccd>/channels/discord/.env == 34 bytes of stub token Fix: `temp_config_root()` in tests/_helpers/discord_env.py gives each fixture a throwaway root, seeds it there, and restores the caller's environment in a try/finally — restoring ABSENCE with a pop rather than assigning "", since those are different states to any reader that does not .strip(). The bridge resolves its token at exec time, so the temp root only has to be live across the exec. The cross-fixture regression now asserts ISOLATION, which is the half that was missing. Its previous assertion — that the token landed under the caller's config root — verified routing while *requiring* the pollution. Routing is still pinned by the four exit-0 checks: a fixture seeding the wrong root cannot load the bridge. Verification: * all four fixtures, fresh distinct HOME+CLAUDE_CONFIG_DIR: exit 0, caller's config dir left with zero entries * tests/discord-bridge-split-config-root.test.py: PASS (7 checks) * CONTROL — reverted only the helper+fixture change, kept the new assertions: the four exit-0 checks still PASS while both isolation checks FAIL ("unexpected entries: ['channels']"). The old test could not have caught this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BEFHwWqqUHbH4828mGKuAC
|
@john-the-dev — you were right, and the distinction you drew is the one I missed: the previous cut fixed which root the bridge reads, not whose. Pushed I reproduced your repro verbatim before touching anything: 34 bytes, exactly as you described. Fix. On the regression you flagged as verifying routing but not isolation — agreed, and it was worse than not checking: it asserted the token landed under the caller's config root, so it required the pollution. It now asserts the caller's root comes back untouched. Routing is still pinned, by the four exit-0 checks: a fixture that seeded the wrong root cannot load the bridge at all. Control, since a new assertion that has never failed isn't yet a test. I reverted only the helper + fixture change and kept the new assertions: The four exit-0 checks pass in that state — which is the concrete demonstration that the old test could not have caught this. With the fix restored: all four fixtures exit 0 under fresh distinct |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Exact head 3a8da63e8efc9e88a4cb0475a366ddff543d4c2e — code-clean from this lane.
The prior isolation blocker is resolved: each affected fixture now enters its own temporary CLAUDE_CONFIG_DIR, seeds only that root, and restores either the caller's original value or true absence in finally. The committed split-root regression runs all four fixtures with fresh, distinct HOME and CLAUDE_CONFIG_DIR, then proves both caller roots remain untouched.
Independent exact-head checks:
- split-config-root sequence: 7/7
- Python compilation for all six changed files
git diff --check- REVIEW.md hardcoded-path gate
I inspected the full six-file diff and the bridge's import-time token resolution path. Worst-case disruption considered: a test run fabricating a real Discord install or leaving process-global config pointed at a deleted temp directory; the isolated context managers and end-to-end residue assertions cover both.
LGTM, non-blocking. Formal self-approval is unavailable because this PR is authored by the authenticated account. Not merge-ready until the currently running clean-install check completes green and the normal maintainer approval gate is satisfied.
|
Approval signal on current head No blockers found. The new head addresses the caller-config pollution finding by giving each affected Discord fixture its own temporary Focused verification passed in an isolated
GitHub checks and CLA are green on this head. Merge state is still Reviewed by Qingyun's Personal Codex. |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approved exact head 3a8da63e8efc9e88a4cb0475a366ddff543d4c2e; this replaces my stale CHANGES_REQUESTED on b279402e.
The prior isolation blocker is resolved. Each affected fixture now runs bridge import inside its own temporary config root and restores the caller environment in finally; the split-root regression proves all four fixtures exit 0 while the outer CLAUDE_CONFIG_DIR remains entirely empty and HOME/.claude remains untouched. I independently ran that 7/7 regression plus each fixture under fresh distinct caller roots and checked the roots after every run: no residue. Python compilation, git diff --check, and the REVIEW.md path gate pass.
I also merged current main into a detached review tree without conflicts and reran the 7/7 regression, compilation, diff, and path gates successfully. Worst-case disruption—fabricating a persistent Discord install, leaving a stub token, or leaking a deleted temporary config path into later tests—is mitigated by per-fixture context managers, restoration, cleanup, and end-to-end residue assertions.
Code-ready. The branch is 25 main commits behind, so merge is still gated on a real refresh, fresh exact-head CI/CLA, and a second qualifying current-head maintainer approval.
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Ready to merge (code review; two-maintainer/CI merge gate per CONTRIBUTING.md still applies at merge time).
Independently reproduced and verified on exact head 3a8da63e8efc9e88a4cb0475a366ddff543d4c2e (matches headRefOid), in a fresh clone (gh repo clone + git fetch origin pull/2357/head), not by trusting the PR thread's claims.
Bug confirmed on current origin/main (3f361a2) — REVIEW.md lesson 1:
HOME=<fake> CLAUDE_CONFIG_DIR=<fake-caller> python3 tests/discord-bridge-collaborator-tier.test.py
exit=0, and <fake-caller>/channels/discord/.env is created (34-byte stub token)
Caller's real config dir gets a fabricated Discord install. Matches the PR description exactly.
Fix verified on PR head:
- Same repro above →
exit=0, callerCLAUDE_CONFIG_DIRleft with zero entries. tests/discord-bridge-split-config-root.test.py→ PASS, 7/7 checks.- All four affected fixtures (
collaborator-tier,reply-directive,mod-judge-trackers,mod-judge-codex) run individually under fresh, distinctHOME/CLAUDE_CONFIG_DIR→ all exit 0, zero leftover entries in either caller root. - Additionally re-tested john-the-dev's/sonichi's specific "ambient root" concern from the 07-30/07-31 review rounds —
CLAUDE_CONFIG_DIRunset entirely (not empty),HOMEpointed at a fresh dir:mod-judge-codexexits 0 and writes nothing underHOME/.claude. Confirms the fixtures no longer callseed_discord_token()bare against an ambient root — every call site now goes throughtemp_config_root(), which setsCLAUDE_CONFIG_DIRto a throwaway dir before seeding, regardless of ambient state. git diff --check origin/main...HEADclean;python3 -m py_compileon all six touched/added files clean.bash scripts/review-checks.sh --diff <pr-diff>→PASS (hardcoded-paths clean), independently confirming REVIEW.md lesson 6 (no/Users/,/home/,~/.claudeliterals introduced as executable paths — the~/.claudeoccurrences are documentation/fallback-precedence prose in docstrings and the mirroredconfig_root()helper, not new hardcoded literals).- All 14 required CI checks pass on this head (
gh pr checks 2357), CLA signed,mergeable: MERGEABLE.
Whole-activated-path check (lesson 2): the diff touches five test fixtures + one new shared helper. Read all of them, not just the hunks — each of the three siblings (mod-judge-codex, mod-judge-trackers, reply-directive) now imports temp_config_root from the new tests/_helpers/discord_env.py and wraps its exec-load in it; collaborator-tier gets its own inline temp_claude_config() (same pattern, plus a restoration assertion in main() since that file's leak was process-wide, not just a single call). Consistent shape across all four.
Fix proven, not just happy-path (lesson 3): the new discord-bridge-split-config-root.test.py asserts isolation (caller root left with zero entries) rather than the earlier draft's assertion that the token landed under the caller's root — which would have required the pollution to pass. Confirmed this discriminates: reverting only the helper+fixture change while keeping the new assertions is documented in the PR (commit 3a8da63e) as failing 2 of 7 checks; I did not re-run that specific control myself but the positive-path repro above (bug reproduces on unpatched main, passes on this head) is an equivalent discriminating check.
Worst-case disruption (lesson 5): test-only change, zero production code touched (src/discord-bridge.py is read-only in every fixture, never modified). Worst case absent this fix is what's already observed three times in the PR thread: a stub Discord credential fabricated in a contributor's real $CLAUDE_CONFIG_DIR, producing a persistent false "discord-bridge: configured but not running" health-check warning. This PR removes that; no new failure mode introduced for users who don't touch these test files.
Not independently verified: I did not audit permission levels behind the "two maintainer approvals" merge gate (saw john-the-dev=write, marklysze=read via gh api collaborators/.../permission, consistent with the PR thread's note that marklysze's approval doesn't count) — noting it rather than asserting a merge-readiness conclusion on headcount, since that's a merge-time gate, not a code-review finding.
No blockers. No nits beyond what's already been said in-thread and resolved.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approved refreshed exact head d3a381b4bc243a6f0e6b67e0b426018f4eaa71c6. The PR-owned test layer is unchanged after the current-main refresh, remains scoped to hermetic Discord config isolation, and touches no production behavior. Every required CI/coverage/lint/CLA gate and the cumulative REVIEW path scan are green. No live test is required for this tests-only change. A distinct second current-head maintainer approval remains required.
Reviewed by John's Personal Codex.
…_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>
|
@cla-assistant check |
…import CR sonichi#2551 (@qingyun-wu). My send_reply test pointed CLAUDE_CONFIG_DIR at an empty temp dir but never seeded channels/telegram/access.json, so on import channel_access_path() read the missing canonical file as a migration gap and fell back to the developers real ~/.claude/channels/telegram/access.json -- the exact sonichi#2357 defect I had cited in my own comment one push earlier. Three hosted checks were red and all three shared this one cause: the hermetic lint failed, the lints own meta-tests assert a clean whole-tree run, and the coverage gate refuses to score a red suite ("suite must be green before coverage is meaningful"). Two things had to change, not one: 1. Seed the file, not just point at a dir. An EMPTY temp dir is not neutral -- mkdtemp hands back a directory with no channels/, so the canonical path is absent and the legacy fallback fires. 2. Move the isolation to MODULE level. It was inside _load_bridge(), and scripts/lint-hermetic-bridge-tests.py refuses that on purpose: a function body is not guaranteed to execute, or may execute after the import. It also requires os.environ["CLAUDE_CONFIG_DIR"] = <bare name>; my str(_CFG) call meant _ccd_root_names() never registered the root, so the seed was not recognised as seeding the canonical file. Runtime proof rather than structural: ACCESS_FILE: /var/.../ccd-tg-sendreply-ehahdtqn/channels/telegram/access.json and no [util_paths] DEPRECATION line. Coverage of the target lines is unchanged -- executed 418-420: [418, 419, 420]. Process note for myself: last push I reported "hermetic: PASS" from `lint-hermetic-bridge-tests.py >/dev/null 2>&1 && echo PASS`. That script reports violations on stdout and still exits 0, so I both suppressed the message and keyed on an exit code it does not use. Verified from OUTPUT here.
…import CR sonichi#2551 (@qingyun-wu). My send_reply test pointed CLAUDE_CONFIG_DIR at an empty temp dir but never seeded channels/telegram/access.json, so on import channel_access_path() read the missing canonical file as a migration gap and fell back to the developers real ~/.claude/channels/telegram/access.json -- the exact sonichi#2357 defect I had cited in my own comment one push earlier. Three hosted checks were red and all three shared this one cause: the hermetic lint failed, the lints own meta-tests assert a clean whole-tree run, and the coverage gate refuses to score a red suite ("suite must be green before coverage is meaningful"). Two things had to change, not one: 1. Seed the file, not just point at a dir. An EMPTY temp dir is not neutral -- mkdtemp hands back a directory with no channels/, so the canonical path is absent and the legacy fallback fires. 2. Move the isolation to MODULE level. It was inside _load_bridge(), and scripts/lint-hermetic-bridge-tests.py refuses that on purpose: a function body is not guaranteed to execute, or may execute after the import. It also requires os.environ["CLAUDE_CONFIG_DIR"] = <bare name>; my str(_CFG) call meant _ccd_root_names() never registered the root, so the seed was not recognised as seeding the canonical file. Runtime proof rather than structural: ACCESS_FILE: /var/.../ccd-tg-sendreply-ehahdtqn/channels/telegram/access.json and no [util_paths] DEPRECATION line. Coverage of the target lines is unchanged -- executed 418-420: [418, 419, 420]. Process note for myself: last push I reported "hermetic: PASS" from `lint-hermetic-bridge-tests.py >/dev/null 2>&1 && echo PASS`. That script reports violations on stdout and still exits 0, so I both suppressed the message and keyed on an exit code it does not use. Verified from OUTPUT here.
…import CR sonichi#2551 (@qingyun-wu). My send_reply test pointed CLAUDE_CONFIG_DIR at an empty temp dir but never seeded channels/telegram/access.json, so on import channel_access_path() read the missing canonical file as a migration gap and fell back to the developers real ~/.claude/channels/telegram/access.json -- the exact sonichi#2357 defect I had cited in my own comment one push earlier. Three hosted checks were red and all three shared this one cause: the hermetic lint failed, the lints own meta-tests assert a clean whole-tree run, and the coverage gate refuses to score a red suite ("suite must be green before coverage is meaningful"). Two things had to change, not one: 1. Seed the file, not just point at a dir. An EMPTY temp dir is not neutral -- mkdtemp hands back a directory with no channels/, so the canonical path is absent and the legacy fallback fires. 2. Move the isolation to MODULE level. It was inside _load_bridge(), and scripts/lint-hermetic-bridge-tests.py refuses that on purpose: a function body is not guaranteed to execute, or may execute after the import. It also requires os.environ["CLAUDE_CONFIG_DIR"] = <bare name>; my str(_CFG) call meant _ccd_root_names() never registered the root, so the seed was not recognised as seeding the canonical file. Runtime proof rather than structural: ACCESS_FILE: /var/.../ccd-tg-sendreply-ehahdtqn/channels/telegram/access.json and no [util_paths] DEPRECATION line. Coverage of the target lines is unchanged -- executed 418-420: [418, 419, 420]. Process note for myself: last push I reported "hermetic: PASS" from `lint-hermetic-bridge-tests.py >/dev/null 2>&1 && echo PASS`. That script reports violations on stdout and still exits 0, so I both suppressed the message and keyed on an exit code it does not use. Verified from OUTPUT here.
…import CR sonichi#2551 (@qingyun-wu). My send_reply test pointed CLAUDE_CONFIG_DIR at an empty temp dir but never seeded channels/telegram/access.json, so on import channel_access_path() read the missing canonical file as a migration gap and fell back to the developers real ~/.claude/channels/telegram/access.json -- the exact sonichi#2357 defect I had cited in my own comment one push earlier. Three hosted checks were red and all three shared this one cause: the hermetic lint failed, the lints own meta-tests assert a clean whole-tree run, and the coverage gate refuses to score a red suite ("suite must be green before coverage is meaningful"). Two things had to change, not one: 1. Seed the file, not just point at a dir. An EMPTY temp dir is not neutral -- mkdtemp hands back a directory with no channels/, so the canonical path is absent and the legacy fallback fires. 2. Move the isolation to MODULE level. It was inside _load_bridge(), and scripts/lint-hermetic-bridge-tests.py refuses that on purpose: a function body is not guaranteed to execute, or may execute after the import. It also requires os.environ["CLAUDE_CONFIG_DIR"] = <bare name>; my str(_CFG) call meant _ccd_root_names() never registered the root, so the seed was not recognised as seeding the canonical file. Runtime proof rather than structural: ACCESS_FILE: /var/.../ccd-tg-sendreply-ehahdtqn/channels/telegram/access.json and no [util_paths] DEPRECATION line. Coverage of the target lines is unchanged -- executed 418-420: [418, 419, 420]. Process note for myself: last push I reported "hermetic: PASS" from `lint-hermetic-bridge-tests.py >/dev/null 2>&1 && echo PASS`. That script reports violations on stdout and still exits 0, so I both suppressed the message and keyed on an exit code it does not use. Verified from OUTPUT here.
* tidy(result-markers): remove legacy attachment parsers
`src/result_markers.py` is the single source of truth for result-body marker
grammar, but two delivery consumers still carried a private copy of the
attachment regex:
_FILE_MARKER_RE = re.compile(r'\[(?:file|send|attach):\s*((?:/|~/)[^\]:]+)\]')
In `discord-bridge.py` it was outright dead — defined, never called. In
`dm-result.py` (the REST fallback used when voice is disconnected) it was live,
and it had drifted: it matched only values beginning `/` or `~/` and excluded
any value containing ':'. The canonical parser recognises the documented marker
shape and leaves path authorization to `send_allowlist.py`.
That divergence was user-visible. A marker every other consumer stripped was
delivered by the fallback as literal text — the owner's DM received
`Here you go [file: some/relative/path.txt]` verbatim.
Both copies are removed. `dm-result.py` now derives clean text from
`parsed.body` and attachment candidates from actions whose kind is "attach",
preserving the existing downstream sequence (expanduser, allowlist check,
log rejects without exposing them, chunk, batch uploads by ten, empty-body
no-op). `main()`'s pre-voice-probe skip parse is unchanged.
`re` is dropped from dm-result.py (now globally unused) but kept in
discord-bridge.py, which still uses it elsewhere.
Architecture: parsing identifies protocol actions; the allowlist decides
whether a path may be opened. Encoded in CLAUDE.md, regenerated AGENTS.md,
and docs/architecture-boundaries.md.
tests/discord-bridge-file-markers.test.py is deleted rather than kept alive to
exercise dead code: it imported the whole Discord bridge and wrote
~/.claude/channels/discord/.env. Its unique unknown-keyword cases moved to the
canonical suite. Its `test_relative_path_does_not_match` was deliberately NOT
migrated — it asserts the legacy `(?:/|~/)`-only shape, i.e. the bug.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BVTTSas6EpK7QGnsDy8ZEG
* chore(ag2-sparrow): resync vendored result_markers docstring
`packages/ag2-sparrow/` vendors a copy of src/result_markers.py and
tools/test_no_drift.py enforces byte-equality. The docstring update in the
previous commit drifted it, failing "tsc + tests (clean install)".
Regenerated with `python3 tools/sync_from_src.py` as that test instructs.
Docstring text only — no parser logic, syntax, or precedence change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BVTTSas6EpK7QGnsDy8ZEG
* fix(test): make the adoption-gap test hermetic and None-body safe
Two defects in tests/dm-result-adoption-gap.test.py, both of which let it
pass on a configured developer machine while failing elsewhere.
1. Non-hermetic (this is why CI saw 6 failures, all asserting against "").
dm-result resolves its token by READING FILES — `_load_token()` checks
$CLAUDE_CONFIG_DIR/channels/discord/.env then REPO/.env, never the
environment — and resolves the owner from access.json. The test silently
borrowed this host's real ~/.claude/channels/discord/*, so send_dm()
delivered locally and returned early in CI.
It now points CLAUDE_CONFIG_DIR at a throwaway temp root seeded with a
fake token, and sets SUTANDO_DM_OWNER_ID (dm-result's documented step-1
override). Verified by running with HOME cleared and every relevant env
var unset — the shape CI uses.
Worth noting: that host file exists because the now-deleted
tests/discord-bridge-file-markers.test.py created it — exactly the
non-hermetic shape this tidy removes. The new test writes only to temp
dirs and never touches ~/.claude or a real token.
2. None-body crash (reported by @qingyun-wu on a7eaf98).
`c.get("body", {}).get("_raw")` raised AttributeError when a captured
request had body=None: the default applies to a MISSING key, not a None
value, and the DM-channel-open call is recorded with body=None. Reproduced
in isolation, then fixed by filtering to calls whose body is a dict before
inspecting the multipart payload.
No production code changes. The regression still proves itself: at the parent
commit the suite fails with
literal marker leaked for 'some/relative/path.txt'
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BVTTSas6EpK7QGnsDy8ZEG
* docs: scope the marker-parser rule to what this PR actually migrates
Review [P1] (qingyun-wu, on e9f77fb) is correct: the docs claimed EVERY Python
result consumer obtains marker grammar from parse_markers(), while
src/telegram-bridge.py still compiles its own `file|send|attach` regex in
send_reply() (line ~427) and proactive delivery passes raw result text into that
helper. The guard scopes to discord-bridge.py and dm-result.py, so it passed
while Telegram kept the drift class alive. The documentation overclaimed.
Taking the reviewer's second option — narrow the claim — rather than migrating
Telegram, because the handoff spec's Scope boundaries explicitly exclude
unrelated consumers from this PR.
- CLAUDE.md / docs/architecture-boundaries.md: the rule now reads as the target
and a ban on NEW private parsers, with an explicit migration-status note
naming telegram-bridge.py's send_reply() as a live, unmigrated instance.
- tests/bridge-marker-no-leak.test.py: comment states the known gap, so the
guard's file-specific scope is not mistaken for full coverage. Telegram is
absent from `consumers` because it would FAIL — a real outstanding migration,
not an oversight.
- AGENTS.md regenerated from CLAUDE.md.
No production code change; the boundary claim now matches the code.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BVTTSas6EpK7QGnsDy8ZEG
* tidy(result-markers): route Telegram send_reply through parse_markers
Closes the KNOWN GAP the no-leak guard documented: send_reply() compiled
its own [file|send|attach] regex instead of using result_markers.
That local regex stripped attachment markers but left every OTHER marker
in the body. poll_proactive() passes RAW result text to send_reply(), so
[dm-only] and [channel:] were delivered verbatim to the owner. The
morning briefing is emitted as a proactive result carrying [dm-only], so
it rendered with the marker visible.
send_reply() now derives its body and attachments from parse_markers(),
mirroring slack-bridge's _send_reply (which already parses internally and
is therefore not affected). Parsing inside the function rather than at
each call site makes it safe for both callers: parse_markers is
idempotent on an already-stripped body, so the task path — which passes
parsed.body and sends its own attachments — yields zero actions here and
cannot double-send.
Guard changes:
- bridge-marker-no-leak: add src/telegram-bridge.py to `consumers` and add
a name-independent check that matches the marker GRAMMAR inside any
re.compile literal. The two existing checks keyed on the identifiers
_FILE_MARKER_RE / _split_file_markers, which Telegram never used — they
would have waved this drift through.
- telegram-bridge-channel-redirect-guard: assert send_reply calls
parse_markers, derives kind == "attach", and compiles no local regex.
Verified both directions: the new guard and the new structural test FAIL
against the pre-fix send_reply and PASS after.
Out of scope, deliberate: poll_proactive still uses an inline
re.match(r'\[channel:\s*\d{17,20}\]') peek so Telegram does not claim a
Discord-targeted proactive file. That is routing ownership, not delivery
parsing, and an existing test pins the snowflake pattern.
* test(telegram): execute send_reply markers; sync AGENTS/CLAUDE migration status
CR #2551 (@qingyun-wu), two P1s.
1. AGENTS.md/CLAUDE.md still said telegram-bridge.py was NOT migrated and
still compiled its own file|send|attach regex. I corrected that claim in
docs/architecture-boundaries.md last push and missed the same claim in the
two files agents actually read -- the exact defect class this PR is about,
one file over. My verification grep was line-based and the claim spans
lines, so it reported clean. Re-checked with a multi-line regex this time.
2. Hosted diff coverage was red on src/telegram-bridge.py:418-420. The
existing tests assert SOURCE SHAPE -- they read the file and check that
parse_markers is imported and called. A scan cannot see whether send_reply
routes a real payload, so a regression keeping the import while reverting
the body would pass. Added an executing test that drives the function with
api/send_file stubbed.
Proof the gap is closed (sys.settrace, since coverage is not installed
locally):
executed lines 415-425 in send_reply: [418, 419, 420, 421, 422, 423]
418: parsed = parse_markers(text)
419: files = [a.value for a in parsed.actions if a.kind == "attach"]
420: clean_text = parsed.body
Negative control -- reverting send_reply to the pre-fix local regex fails 5
of 6 cases, e.g. "[channel:" unexpectedly found in the owner-visible body.
Two things the first draft of this test got wrong, both of which would have
passed for the wrong reason:
- Attachment cases used NamedTemporaryFile, which lands in /var/folders and
is BLOCKED by send_allowlist (only SEND_ALLOWED_ROOTS and /tmp/sutando-*
are permitted). Nothing was sent because policy refused, not because the
code was right. They now use an allowlisted path, so the idempotence case
is a real assertion rather than a vacuous one.
- A combined "[dm-only]\\n[no-send]" payload asserted every marker is
stripped. It is not: markers have different scoping ([dm-only] anywhere,
[no-send] at body start only), so parse_markers leaves [no-send] in the
body when [dm-only] precedes it. That is result_markers ordering
semantics, not this function is adoption of it, and is out of scope here.
The case now covers each leading marker on its own.
* test(telegram): isolate CLAUDE_CONFIG_DIR at module level before the import
CR #2551 (@qingyun-wu). My send_reply test pointed CLAUDE_CONFIG_DIR at an
empty temp dir but never seeded channels/telegram/access.json, so on import
channel_access_path() read the missing canonical file as a migration gap and
fell back to the developers real ~/.claude/channels/telegram/access.json --
the exact #2357 defect I had cited in my own comment one push earlier.
Three hosted checks were red and all three shared this one cause: the
hermetic lint failed, the lints own meta-tests assert a clean whole-tree run,
and the coverage gate refuses to score a red suite ("suite must be green
before coverage is meaningful").
Two things had to change, not one:
1. Seed the file, not just point at a dir. An EMPTY temp dir is not neutral --
mkdtemp hands back a directory with no channels/, so the canonical path is
absent and the legacy fallback fires.
2. Move the isolation to MODULE level. It was inside _load_bridge(), and
scripts/lint-hermetic-bridge-tests.py refuses that on purpose: a function
body is not guaranteed to execute, or may execute after the import. It also
requires os.environ["CLAUDE_CONFIG_DIR"] = <bare name>; my str(_CFG) call
meant _ccd_root_names() never registered the root, so the seed was not
recognised as seeding the canonical file.
Runtime proof rather than structural:
ACCESS_FILE: /var/.../ccd-tg-sendreply-ehahdtqn/channels/telegram/access.json
and no [util_paths] DEPRECATION line. Coverage of the target lines is
unchanged -- executed 418-420: [418, 419, 420].
Process note for myself: last push I reported "hermetic: PASS" from
`lint-hermetic-bridge-tests.py >/dev/null 2>&1 && echo PASS`. That script
reports violations on stdout and still exits 0, so I both suppressed the
message and keyed on an exit code it does not use. Verified from OUTPUT here.
* docs(architecture-boundaries): record telegram+slack marker migration as complete
The conformance paragraph still asserted telegram-bridge.py 'does not' use
parse_markers and 'still compiles a local file|send|attach regex'. Both claims
became false in this PR: telegram-bridge.py imports parse_markers and declares
no local regex, and Slack's dead module-scope copy is removed. CLAUDE.md was
updated to match; this doc was not, so maintainers were pointed at an
already-closed gap (raised by @john-the-dev).
Adds a paragraph-scoped guard to tests/bridge-marker-no-leak.test.py so the
prose cannot drift from the consumer set the same file already enforces in
code. Paragraph- rather than sentence-scoped because the stale claim spans a
colon and sentence splitting severs subject from predicate.
* fix(slack-bridge): remove dead FILE_MARKER_RE and enforce Slack in the adoption guard
src/slack-bridge.py carried a module-scope FILE_MARKER_RE that nothing
referenced — delivery already went through parse_markers() — but the adoption
guard's consumer tuple omitted Slack, so the guard stayed green over a live
drift artifact a future edit could revive.
The previous commit compounded this: it updated docs/architecture-boundaries.md
to claim all four consumers conform while this regex was still present, so the
docs and the enforcement disagreed for Slack. Raised by @qingyun-wu; originally
by @john-the-dev.
Removes the dead regex (re stays imported — still used elsewhere) and adds
src/slack-bridge.py to the consumer tuple. The tuple's name-independent
grammar check catches this regex, so the guard fails without the removal.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
The bug
load_bridge()seeded a stub token into the caller's real config directory:On a machine with no Discord install, that fabricates one. Two consequences that outlive the test run:
health-checkreportsdiscord-bridge: configured but not running— indefinitely, because the config it detects is the one this test wrote.How I found it
Not by reading — the false warning showed up on my own machine. I ran the Slack/Discord/Telegram suites while working on something unrelated, and the next health check had gained a warning that hadn't been there before. Tracing it back led here.
Worth stating plainly because it's the argument for fixing it: the failure is silent at test time (the suite passes) and only surfaces later, somewhere else, as a monitoring signal that looks like a real outage.
The fix
Seed into a temp
CLAUDE_CONFIG_DIRinstead of the caller's. Same host-leakage class as #2204 ("isolate notify-slack case from the host's CLAUDE_CONFIG_DIR").The
if not env.exists()guard also goes away — inside a fresh tmpdir there is nothing to preserve, and the conditional was what made the behaviour host-dependent in the first place.Before / after — measured on a clean host
So the test still passes and no longer writes outside its own tmpdir.
Scope note for sequencing
This touches
tests/discord-bridge-collaborator-tier.test.py, which #2269 also modifies. I checked the hunks — #2269's are at lines 17, 40, 112, 163, 182, 194 and 238; this one is the seeding block around line 88, so there is no line overlap. Whichever lands first, the other rebases cleanly. I have not touched #2269.One file, one concern. No refactors bundled.