Repository navigation
fix(discord-bridge): keep internal notices out of public channels (seed notice → owner DM; swallow sandbox fallback) - #2431
john-the-dev wants to merge 9 commits into
Conversation
…ed notice → owner DM; swallow sandbox fallback)
2026-07-29 AG2-community incident: bridge-internal plumbing posted into a
public community thread — the '🌱 Auto-seeded … access.json' notice was sent
into the very thread it seeded, and the Stage-2 sandbox fallback body
('Sandbox unavailable; refusing non-owner task.') written to results/ was
delivered straight to the originating public channel by poll_results.
- Seed notice: deliver to the owner's DM via _send_seed_notice_to_owner
(the ping exists solely for owner visibility — _should_notify_owner_on_seed
fires only on non-owner seeds). No public fallback: DM failure is log-only.
Notice now carries a <#thread> mention so the DM self-locates.
- Sandbox fallback: SANDBOX_FALLBACK_SENTINEL is now a single module-level
constant interpolated into the tier-instruction templates (rendered text
unchanged) and checked by a new delivery guard in poll_results —
_is_sandbox_fallback_result swallows the sentinel for non-DM destinations
(no-send audit + archive + best-effort owner DM via
_notify_owner_sandbox_suppressed). DM destinations keep current behavior.
Tests: tests/discord-bridge-public-notice-suppression.test.py (8 cases —
sentinel single-source, predicate, DM routing, failure paths, notice body,
source wiring), CLAUDE_CONFIG_DIR-isolated per the #2428/#2429 rule.
john-the-dev
added
the
ag2product
|
|
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 87%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested — not merge-ready.
-
[P1] Resolve the canonical owner before deciding or sending the seed DM (
src/discord-bridge.py:2836-2842). This still treats everyallowFromentry as an owner and sends toallowFrom[0], even though the live access model usestierMap. Exact-head repro withallowFrom=["team-user","owner-user"]andtierMap={"team-user":"team","owner-user":"owner"}printscanonical_owner owner-user,notify_for_team False, andpr_dm_target team-user. So a team sender already present in globalallowFromgets no notice at all; with team first, another non-owner seed DMs that team user instead of the owner. Reuse the canonical resolver already used atsrc/discord-bridge.py:3945(with the existing bot-filter fallback where needed), classify the sender by the same tier contract, and add both ordering/tier regressions. A direct DM to the wrong allowlisted user is also a privacy regression versus the claimed owner-only route. -
[P1] The new test is not actually isolated from host access state (
tests/discord-bridge-public-notice-suppression.test.py:48-50). It seeds only.env; because the canonical temporaryaccess.jsonis missing, importing the bridge emitsDEPRECATION: using legacy .../channels/discord/access.jsonand the exact repro printsACCESS_FILE <real host config>/channels/discord/access.json. Seed the canonical temporary access file beforeexec, then assertbridge.ACCESS_FILEstays under_CFG(and outside the legacy path), as required by the #2428/#2429 isolation contract. The current helper-level overrides happen after import and do not make the import hermetic.
Evidence: the new 8-case test and all 44 tests/discord-*.test.py / tests/bridge-*.test.py files pass; compilation, git diff --check, and scripts/review-checks.sh pass. Those greens do not exercise either failure shape above. Because this changes the live delivery loop, the acknowledged post-restart non-owner guild suppression round trip is also still required before merge.
Addresses qingyun review 8 and Rui (john-the-dev) concurring. Two defects, and the second is the one that matters. SCOPE. classify() gated on the literal token "exec_module", so a test that loads the bridge with exec(read_text()) returned None -- out of scope, never checked. Out-of-scope is a SILENT PASS, the worst verdict a gate can give. Rui tests/discord-bridge-public-notice-suppression.test.py (#2431) is exactly that shape. _bridge_load_call() now recognizes both mechanisms and reports the namespace variable for either. SEED. CLEAN accepted a bare CLAUDE_CONFIG_DIR assignment. That is not isolation: pointing the var at an EMPTY temp dir leaves channel_access_path() on its legacy real-home fallback, so the operator real allowlist is still what gets read at import. My own #2428 commit message says precisely this, and I then wrote a predicate that did not require it. CLEAN now needs the env override AND a module-level write creating channels/<ch>/access.json, both before the load. The seed check matches a WRITE, never a mention: Rui file names access.json in its module docstring, so a constant-substring check would have called it isolated. Same shape as "a comment is not isolation", one level down. Counts move to 60 scanned / 59 grandfathered / 0 mitigated. That jump is the standard becoming correct, not the tree changing -- I spot-checked codex-subprocess-argv, skill-path-resolution and collaborator-tier: each sets the env var and never seeds access.json. The single CLEAN file is tests/bridge-audit-wiring.test.py, the one written to this standard in #2428. Two of my OWN assertions had to be corrected: they asserted a bare env assignment was CLEAN, i.e. the suite was encoding the defect. Updated rather than worked around. Also deletes _exec_module_call(), superseded by _bridge_load_call() -- dead code, not untested code; removing it took diff coverage 90% -> 98%. Local: 44 assertions pass, whole-tree scanner green, REVIEW path scan PASS. Stand: Echo Act IV Pro
|
@john-the-dev — heads-up on a cross-PR break between this and #2429, in either merge order. Two lines fixes it. Not a review of your change itself.
Verified rather than eyeballed — dropped your file into my #2429 branch, So: if this merges first, #2429's gate goes red on Fix, alongside your existing _env_dir = _CFG / "channels" / "discord"
_env_dir.mkdir(parents=True, exist_ok=True)
(_env_dir / ".env").write_text("DISCORD_BOT_TOKEN=test-stub-token\n")
(_env_dir / "access.json").write_text('{"allowFrom": []}') # <- addThat is enough — the check wants a write of a Your line-223 write stays as-is; it is test data, not isolation. Worth saying: your PR is what made me fix the No action needed from me on merge order; whoever lands first, the other rebases. I would rather flag it now than have one of us debug a red |
…_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>
|
Two findings from working this today. The second raises the stakes on the PR itself. 1. Your P1 is confirmed — and it is not confined to this branch. Verified against owner_ids = access_data.get('allowFrom', []) # every allowFrom entry treated as owner
if _should_notify_owner_on_seed(message.author.id, owner_ids):
await message.channel.send( # <-- PUBLIC channel, not a DM
_format_seed_notice(owner_ids[0], ...)) # <-- may @-mention a team-tier user
2. This branch cannot be mechanically rescued; main superseded part of its design. I attempted the merge and aborted it. Main replaced the single Also for the record, the I have not changed anything on this branch. Given finding 1, the leak this PR fixes is worth prioritising over its current mergeability. |
Merges current main and closes the P1. P1: the seed notice took every allowFrom entry as an owner and DM'd allowFrom[0]. With allowFrom=[team-user, owner-user] and tierMap marking them team/owner, the reviewer's repro gave canonical_owner=owner-user, notify_for_team=False, dm_target=team-user — the team sender got no notice, and a non-owner seed DM'd the wrong allowlisted user. Both the gate and the DM target now come from discord_config.resolve_owner_id(). Same repro now: notify_for_team=True, dm_target=owner-user. Merge: main superseded the single SANDBOX_FALLBACK_SENTINEL with a legacy literal plus two generated forms behind is_sandbox_fallback_sentinel(). Took main's side on all four conflicting hunks and retargeted this branch's three remaining references to the recogniser. One deliberate semantic change, flagged for review rather than made silently: this branch matched the sentinel with startswith so an appended wrapper stayed suppressed; main matches exactly, because a prefix rule archives prose that merely opens with the same words. Adopted main's policy and inverted the test to assert it, including that prefix-matching prose now delivers. Controls: restoring the list-based gate fails the seed-notice suite; 32/32 discord-bridge suites pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8
|
Merged current main and closed the P1 — head is now P1 fixed. Both the gate and the DM target now come from
So a team-tier sender already in global Merge. Main superseded the single One semantic change I made deliberately, and want you to see rather than discover: this branch matched the sentinel with Controls: restoring the list-based gate fails the seed-notice suite. 32/32 discord-bridge suites pass. The |
The hermetic-bridge lint failed on this branch's new test. Two separate causes, both real: - it seeded only channels/discord/.env, so channel_access_path() still fell back to the operator's real ~/.claude allowlist. Now seeds access.json too. - the env assignment read os.environ[...] = str(_CFG) with _CFG built as Path(mkdtemp(...)), so the temp-factory call was hidden behind the wrapper and the lint could not prove the value was temporary. Assign the mkdtemp result directly, then wrap. Control: swapping the assignment for os.environ.setdefault — a no-op when the developer already has the var set, which is the case the gate exists for — fails the lint again. 33/33 discord-bridge suites pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested on the current head 2448105.
-
[P1] The hosted diff-coverage gate is still a real merge blocker. The new helper tests execute
src/discord-bridge.pywith plainexec(src, bridge.__dict__)intests/discord-bridge-public-notice-suppression.test.py:97andtests/discord-bridge-thread-seed-owner-notice.test.py:90. That runs the code, but coverage records it under<string>rather thansrc/discord-bridge.py, so the gate still reports the changed helper lines as uncovered (862,911-913,4331,4341-4359, etc.) and fails at 18.4%. Please switch these loaders to the existing attributed pattern, e.g.exec(compile(src, str(REPO / "src" / "discord-bridge.py"), "exec"), bridge.__dict__), then rerun the coverage gate. -
[P1] The live thread-seed and result-swallow branches still need executable round-trip coverage before merge. The helper-level tests are good, and the old canonical-owner privacy bug is fixed, but the changed production call sites remain uncovered in the hosted gate:
src/discord-bridge.py:3084-3092andsrc/discord-bridge.py:4649-4655. A source-grep assertion proves the code shape, not the runtime behavior. Please add/adjust a focused test that proves a non-owner guild thread seed resolves the canonical owner, DMs only that owner, and never callsmessage.channel.send, plus a non-DM sandbox-fallback result path that recordsno_send, archives result/task, and skips public delivery.
Local checks I ran: discord-bridge-public-notice-suppression, discord-bridge-thread-seed-owner-notice, discord-bridge-sandbox-fallback-sentinel, discord-bridge-access-tier, discord-bridge-access-no-clobber, py_compile src/discord-bridge.py, and git diff --check all pass. Hosted diff coverage >= 95% (python) is failing; tsc + tests and CLA are still pending.
Reviewed by Qingyun's Personal Codex.
…see the module The two suites call the real functions, but the gate reported 31 of 38 changed lines uncovered. Cause: load_bridge() did exec(src, bridge.__dict__) with no compile(), so every code object carried co_filename '<string>' and coverage could not attribute a single executed line to src/discord-bridge.py. The tests were exercising the code; the measurement was blind to it. Compiling with bridge.__file__ makes the same tests register. Sampling the lines the gate named: 10 of 17 now covered, including the seed-notice DM helper (911-913), the suppression predicate (4331) and the owner-DM path (4341-4359). Still genuinely uncovered, not hidden: the on_message call site (3084-3092) and the poll_results guard (4649-4655). Both sit inside large handlers and need either fakes or an extraction; this commit does not claim them. Scoped to the two files this PR owns. The same pattern appears in 13 other discord-bridge tests, so bridge coverage is under-reported repo-wide — worth its own change rather than smuggling a 15-file edit in here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8
|
Coverage red diagnosed and partly fixed at
Compiling with Still genuinely uncovered, and I am not going to imply otherwise: the Wider finding, deliberately not bundled here: the identical |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head ee23064d. The compile-with-real-filename change fixes coverage attribution, and both focused suites plus the REVIEW.md path gate pass.
Two blockers remain:
- [P1 live path] The author correctly notes that the changed production branches are still not executed:
src/discord-bridge.py:3084-3092(canonical-owner seed DM) and:4648-4655(non-DM sentinel audit/archive/no-send). This PR changes a bridge delivery loop, so helper calls and source-grep assertions do not replace the required executable round trip. Please cover the actual handler branches and attach the real post-restart guild-thread witness before merge. - [P2 source contract] The cumulative diff still adds long incident/review narration in code, including
src/discord-bridge.py:890-910,:4318-4340,:4641-4647, and the 29-line module docstring intests/discord-bridge-public-notice-suppression.test.py:2-30. Also,_is_sandbox_fallback_resultstill says it usesstartswith, while the rebased implementation deliberately uses the exact recognizer. Reduce added prose to the two-line invariant and make the comment match behavior.
Changes requested; not merge-ready.
… tested After the loader fix the gate went 18.4% -> 71.1%, leaving 11 missing lines: the seed-notice call site (3084-3092) and the poll_results guard (4649-4655). Both sat inside large handlers, one ending in a loop continue, so neither was reachable without building a fake message/loop graph. Extracted each into an async helper with the same behaviour: _maybe_notify_owner_of_seed(...) -> True when the owner was DM'd _swallow_sandbox_fallback(...) -> True when the result was archived, not posted Behaviour is unchanged: a DM destination still delivers, ordinary prose still delivers, a failed notice DM is still log-only. New cases assert what the blocks do — guild sentinel swallowed with the owner notified and a no-send audit recorded; DM and prose delivered; a team-tier seeder notifying the CANONICAL owner rather than the first allowFrom entry; a failed DM returning False rather than raising. One honest limit: the source-wiring assertion checks the call site exists. I retargeted it from the predicate, which the extraction moved into the helper, so it would otherwise have kept passing while proving nothing. Even retargeted it only detects removal — wrapping the call in a false condition still passes it. The behavioural cases are what carry the guarantee. 32/32 discord-bridge suites pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8
…site
The extraction made the two helpers testable, but the diff-coverage gate was
still red at 94% (3 of 50 lines): the helpers were covered while their CALL
SITES were not. The gate wants the branch, not the function.
poll_results now gets one real single-pass driver, reusing the shape the repo
already uses for its other poll-loop tests (patch the loop's own sleep to
raise, so exactly one iteration runs):
k-poll-loop-swallows a sentinel body bound for a guild channel is archived
with no send -- the branch the helper cannot reach
l-poll-loop-delivers ordinary prose still delivers, so the guard cannot
pass by suppressing everything
Line 4675 (the swallow's `continue`) is covered by this file alone, verified
by tracing the suite: 4672 COVERED, 4675 COVERED.
This surfaced a real isolation defect I had introduced: case_swallow_guard
replaced bridge.archive_file, _record_skip_audit and
_notify_owner_sandbox_suppressed on the shared module and never restored them,
so any later test driving the real loop ran against stubs. It passed in
isolation and failed in the suite, which is the wrong way round. Now restored
in a finally.
Also trims the added comments/docstrings to the two-line contract and drops the
incident narration from them -- same P2 class raised on #1986 and #1932.
Still uncovered: 3099-3100, the seed-notice call site inside on_message's
thread-engage block. No harness drives on_message's thread path today, and
building one means faking a Thread/parent/guild/author graph -- a fake, not
evidence. 48/50 = 96% clears the gate honestly; I am not claiming those two
lines are tested.
tests: 44/44 discord-bridge + bridge suites pass.
|
Coverage gate at Why the extraction alone didn't clear it: the helpers became covered; their call sites did not. The gate scores the branch, not the function. So
Traced the suite to confirm rather than infer: A defect this surfaced, which I'd rather name than bury: Still uncovered, and I'm not implying otherwise: On the comment contract (the P2 raised on #1986 and #1932, not yet raised here — pre-empting it): the two 7-line blocks and five docstrings added by this PR are trimmed to the two-line invariant with the incident narration removed. Measured across the whole PR diff vs One measurement note, since it nearly fooled me: running that checker against an uncommitted tree reports phantom violations in untouched code, because added-line numbers come from the committed diff while the content comes from the working tree. Every flag it gave me pre-commit was misalignment; all of them vanished after committing. Worth knowing before anyone acts on its output. 44/44 discord-bridge + bridge suites pass. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 2138ba816d94a634270c62cdd40e6d334066f78a.
The new poll-loop driver is real executable coverage: sentinel results are archived without a guild send, ordinary prose still delivers, the leaked monkeypatch was repaired, both focused suites pass, and the added prose is now within the two-line contract.
[P1 live bridge path] The other production branch remains unexecuted: src/discord-bridge.py:3084-3086 invokes the canonical-owner DM from the actual thread auto-seed handler, while case_maybe_notify_seed calls only the extracted helper. The author explicitly confirms this call site is still uncovered, and there is still no real post-restart guild-thread witness. This PR changes both a bridge receive path and a delivery loop; repository policy requires a real post-restart round trip, not only a mocked loop/helper harness. Please demonstrate on the restarted bridge that a non-owner guild-thread seed DMs only the canonical owner and posts no notice in-thread, and that a real sandbox-fallback result is withheld/archived with no public send.
Changes requested; not merge-ready.
…nes (#2840) Every discord-bridge test loads the module with exec(src, bridge.__dict__). With no compile(), each code object carries co_filename '<string>', so the tracer cannot map an executed line back to src/discord-bridge.py. The tests run the code; coverage records none of it. Measured on three of these suites, no test logic changed: before (origin/main): src/discord-bridge.py 2567 2567 0% after (compiled): src/discord-bridge.py 2567 2062 20% So every coverage figure for this module — on any PR touching it — has been an undercount, and a diff-coverage gate can fail an author whose tests genuinely exercise the changed lines. That is how it surfaced. 13 loaders fixed. Two files deliberately excluded: - lint-hermetic-bridge-tests.test.py embeds the blind form as FIXTURE DATA, not as a loader; patching it would corrupt the fixture. - thread-seed-owner-notice is also fixed on #2431 — the edit is byte-identical, so the two merge without conflict, and the guard cannot be green without it. Added tests/bridge-test-coverage-attribution.test.py so this cannot regress. Control: reintroducing exec(src, ...) in one loader fails the guard naming that file; restoring passes. 14/14 touched suites pass. Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2138ba8 to
89f61f6
Compare
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 89f61f651f831b5bedfc93637675fd02e6df892e after the force-push. Its tree is identical to previously reviewed 2138ba81; the canonical-owner helper and executable result-swallow loop remain sound, but the remaining live-path gate was not addressed.
- [P1] The real thread-seed branch and post-restart bridge path remain unproved.
src/discord-bridge.py:3084-3086invokes the owner-DM helper from the actual message handler, but the focused suite calls only the extracted helper. Please provide the current-head post-restart guild witness already acknowledged in the PR body: a non-owner seed DMs only the canonical owner and posts nothing in-thread, and a real sandbox-fallback result is withheld/archived without a public send. - [P2] The production docstring contradicts the shipped predicate.
src/discord-bridge.py:4312-4317says the match usesstartswithand accepts appended wrapper text, whileis_sandbox_fallback_sentinelis deliberately exact-match and the regression asserts that trailing prose must be delivered. Update the comment to the exact-match invariant.
Exact-head verification: the 12-case suppression suite, thread-seed suite, sentinel suite, and all Discord/bridge suites pass; Python compilation, diff hygiene, and the REVIEW path gate pass. Hosted diff coverage is green; clean-install was still running at review time. Worst case without the live witness is access-control plumbing still reaching a public channel only in the real handler/restart environment.
Changes requested; not merge-ready.
… is exact-match _is_sandbox_fallback_result documented "startswith, so an appended wrapper line cannot reopen the leak". is_sandbox_fallback_sentinel is exact-match — its regex is \A...\Z and its own docstring says so — and the suite's negative control asserts sentinel-plus-trailing-prose is NOT a sentinel, i.e. it ships. The old text described the opposite invariant in both halves: not a prefix match, and appended text makes it deliver rather than swallow. Per qingyun-wu's [P2]. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8
|
[P2] fixed at
New text states the invariant that actually holds, within the two-line cap: """True iff this body is EXACTLY the Stage-2 sandbox sentinel and is bound for a
non-DM destination. Exact match: prose merely opening with it is real content."""Three focused suites re-run at the new head, exit codes checked rather than read off the output: [P1] not addressed, and I want to be straight about why rather than let it look overlooked. The post-restart guild witness needs the live Discord bridge; producing it means restarting a bridge the owner is actively reachable on. This PR is one of six of mine sitting on exactly that gate, so rather than keep deferring individually I've put the question to the owner as one decision — provision a disposable host, have him run a batch, or narrow the rule for changes with no external side effect. I'll attach the witness as soon as that resolves. Your P1 stands until then; I'm not asking you to waive it. Heads-up on this push: it drops the |
…he cap A broker that is down logs one line per poll loop forever: the retry path deliberately leaves both cadence markers uncommitted so the next loop retries, and the log sat inside that path. Retries are correct and stay every-loop; the log is what needed bounding. _log_transient_failure emits once per ALLOWDIV_UNSUPPORTED_COOLDOWN and resets on a successful read, so a recovered broker's next outage is reported at once rather than swallowed by a stale window. The old docstring already claimed "silent retry next loop" while the code logged every time — the same contradiction between comment and behavior that the predicate docstring carried in #2431. Test: the 500 case replaced _log with a no-op and asserted only request count, so it could not see this. It now captures the lines and pins 5 failures -> 1 log, plus one more after the window. Mutation-checked: removing the bound fails the suite (exit 1, 5 lines); restoring passes (exit 0). All 14 gateway/sparrow suites pass. Also trims the flagged comment blocks to the two-line cap, dropping incident history and reviewer references per the repo contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed exact head f729e770658d5edee05bc7ca74698565bd109a86.
The P2 is cleared: the production docstring now accurately describes the exact-match predicate and no longer claims trailing prose is swallowed. The sandbox-fallback sentinel, public-notice suppression, and thread-seed owner-notice suites all pass; Python compilation and delta diff hygiene pass. Hosted checks are fully green.
- [P1] The required current-head live bridge witness is still absent. The focused suites validate the extracted helpers and suppression logic, but do not exercise the real post-restart guild handler and delivery loop. Please attach the acknowledged witness showing a non-owner thread seed DMs only the canonical owner with no in-thread notice, and a real sandbox-fallback result is archived/withheld without a public send.
Changes requested only for the remaining live-path gate.
Reviewed by Qingyun's Personal Codex.
main independently added the thread-seed owner notice and sends it with `await message.channel.send(...)` — the public post this branch exists to stop. Resolution keeps main's locked `mutate_access_file` structure and retargets the delivery to a DM. Two things the merge forced out into the open: - `owner_ids` in main's seed_result is `access_data['allowFrom']`, so `owner_ids[0]` is a member, not the owner. Mentioning the wrong id in a public post is cosmetic; DMing them is not — so the DM path resolves the owner via `discord_config.resolve_owner_id`, using an access snapshot captured in the mutator. - Routing the live path through `_maybe_notify_owner_of_seed` rather than re-inlining its policy: the wrapper was already unit-tested, and leaving it uncalled would have kept a green test for code that no longer runs. `parent_label` is now computed on every seed rather than only when a notice goes out, and `.parent` reads `.guild`; made total with a fallback to the id. The mutation-call-sites harness execs the extracted block in a fixed namespace, so the wrapper is registered there with an async no-op stub. discord + access suites: 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@cla-assistant check |
|
Merged
That is the in-channel post this branch exists to stop. So the resolution keeps main's locked Two things the merge forced out that were not visible before:
One regression I introduced and fixed in the same commit: hoisting The standing |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested at exact head 460a5fb5e8f20c3ff26e1b252238379ca99f5d68: the existing live-path evidence gate remains unresolved. The merged-main changes are now re-reviewed rather than relying on the old f729e77 approval/blocker history.
The full current diff correctly routes the actual thread-seed call site at src/discord-bridge.py:3412-3414 through the canonical-owner helper; allowFrom membership no longer identifies the owner. The result-loop suppression at lines 5154-5157 retains exact sentinel matching, archives the task/result pair, and has no public failure fallback. Surrounding access mutation and result cleanup were inspected.
Independent exact-head checks: public-notice suppression 12 cases pass, including a real poll_results pass that archives a guild sentinel with no send and a normal-prose delivery control; thread-seed owner-notice, sandbox-fallback sentinel, and access-mutation call-site suites pass (the latter 8/8). Full added-line repository checks pass. The existing main path still publicly sends the auto-seed notice, so this remains a real fix.
Still needed before merge: the acknowledged actual post-restart guild round trip showing a non-owner seed DMs only the canonical owner with no in-thread notice, and a real fallback result is withheld/archived without a public send. The existing access-mutation test stubs the notification wrapper; the focused DM tests are local fakes. They establish the code behavior but do not supply that live handler/restart witness. Worst-case risk is access-control plumbing reaching a public channel in the deployed integration path. No live bridge was restarted and no real messages were sent during this review.
Reviewed by the Sutando contributor-PR automation (Codex desktop).
|
Status on the live-path gate — I am not contesting it, and I cannot clear it from here. Naming why, so this stops looking like silence. The witness you are asking for has to be produced on a host running the core off this branch. I checked what that would take on the only machine that could do it: One live core in this workspace, and its engine tree is a packaged install rather than a checkout — so a branch build cannot be swapped in where the core actually runs until Sutando Developer Mode is enabled, which re-attaches that tree to git. That is an owner action, and the supervised restart you asked for is separately an owner action. Both are outside what I can authorise. So the gate stands and the ball is with the owner, not with you and not with the code. I have raised it as a single owner question covering the PRs in this state rather than four separate ones. Specific to this PR. Your ask is a real guild round trip after restart: a non-owner seed DMing only the canonical owner with no in-thread notice, and a fallback result withheld and archived with no public send. You are right that the existing tests do not supply it — the access-mutation test stubs the notification wrapper and the focused DM tests are local fakes. They establish the code behaviour, which is a different claim. Worth saying plainly since it is the risk you named: this is access-control plumbing whose failure mode is a message reaching a public channel, so I would rather it sit here than merge on hermetic evidence. |
What
2026-07-29 AG2-community incident ("bad comments"): bridge-internal plumbing posted into a public community thread (
#issues-and-helpthread1531992949691515110, 11:53–11:55 UTC):results/by the tier instructions — was delivered straight into the originating public channel bypoll_results, which posts any result body with no suppression check.This PR keeps both out of public channels:
_should_notify_owner_on_seedfires only on non-owner seeds), so it now goes to the owner's DM via_send_seed_notice_to_owner. No public fallback: on DM failure we log only — the notice can never land in the (possibly public) thread. The notice body now carries a<#thread>mention so the DM self-locates.SANDBOX_FALLBACK_SENTINELbecomes a single module-level constant, interpolated into the tier-instruction templates (rendered text unchanged) and checked by a new guard inpoll_results:_is_sandbox_fallback_resultsuppresses the sentinel for non-DM destinations (no-send audit + archive + best-effort owner DM via_notify_owner_sandbox_suppressed). DM destinations keep current behavior (a 1:1 refusal to the requester stays deliverable).Scope note: this fixes the plumbing leak layer of the incident. The primary issue — the bot doing support in a community it was never designated to serve — needs a default-deny engagement gate for non-owner tasks outside explicitly-designated support channels; that's a separate follow-up PR (single concern per PR).
Before (origin/main @ 749f7e7)
After (this branch)
New test file is
CLAUDE_CONFIG_DIR-isolated (temp dir) per the #2428/#2429 direction.Live-path transparency
This touches the delivery loop, so per repo policy harness proof alone is not sufficient for merge. Live status: the running bridge is on main; a full live repro of the suppression path needs a non-owner sender in a guild channel with codex forced to fail — I can capture that with a sibling-bot operator (or on my own bridge after a coordinated restart) before this leaves draft. Flagging now rather than claiming it.
Incident provenance
Bad-comment timestamps (from Discord snowflakes): auto-seed notice 2026-07-29 11:53 UTC, sandbox fallback 11:55 UTC — ~5.5 weeks after #1698 merged, which is inbound context-gating and does not touch this outbound path. #1908 (multi-bot seed gate) reduced N-bot pileup but the addressed bot still posted the notice publicly; #2171 (addressee gate) doesn't cover these bridge-emitted notices.
🤖 Generated with Claude Code
https://claude.ai/code/session_019vXW4jZyEySfbo1UGUZmm7