[sonichi]

The defect

poll_results() iterates pending_replies — a map the bridge fills only when it receives the inbound message itself (discord-bridge.py:3967, plus the persisted-recovery merge). It never scans results/ for ids it does not know.

So a task file written straight into tasks/ by a cron, a script or another host has no entry, and its result is written, never read, and never errors. The channel_id: the producer declared is inert on the delivery side — a documented field with no reader.

Observed live on this host today:

08:10  task-newsradar-1786547446972.txt written to tasks/   channel_id: 1509379092116672602
08:18  results/task-newsradar-1786547446972.txt written
10:0x  BOTH FILES STILL THERE — no delivery, no error, no log line

1509379092116672602 is the owner's DM (type 1, recipient sonichi). The destination was named in the file the entire time. I delivered the body by hand and archived both.

Why the resolution lives in src/, not in the bridge

The same gap exists for every transport keyed on its own in-memory map. Putting the policy in a dependency-light module means the next adapter binds to it instead of copying it — per CLAUDE.md, a copy is the defect whether or not it is currently misbehaving. The adapter injects _is_discord_channel_id because a snowflake, a Telegram chat id and a Matrix room id are not interchangeable.

It reuses the two shared helpers rather than re-deriving them: task_archive.find_task_file (which also matches the .claimed-core-N variant — my first draft missed that) and local_task_protocol.parse_task_headers, the task-last parser, so nothing after task: can forge a header.

Evidence

At the parent (origin/main @ 7de12565), measured — not asserted:

imports resolver ............... False
poll_results calls it .......... False
_is_discord_channel_id exists .. False
src/orphan_result_routes.py .... does not exist

The three WiringTest cases fail at the parent and pass at HEAD. The module's own unit tests cannot fail at a commit where the module is absent, and I am not going to dress that up as red-to-green — the parent-side evidence for the behaviour is the reproduction above.

20/20 pass at HEAD. Every guard mutation-checked, because a guard no test gates is decoration:

drop foreign-source guard  -> CAUGHT      drop known-id guard -> CAUGHT
drop id-shape guard        -> CAUGHT      drop limit          -> CAUGHT
swap in the lenient parser -> CAUGHT (FAILED failures=1)

That last one matters: the lenient parser scans all lines, so a body containing channel_id: <attacker> would become a route. test_the_body_cannot_forge_a_route is what holds the safe parser in place.

Four sibling bridge suites green: bridge-marker-no-leak, bridges-sending-orphan-recovery, bridge-audit-wiring, bridge-timeout-guards. ruff check clean on all three files. No hardcoded host paths in the added lines.

What this deliberately does NOT do

• No guessing. No task file → no route. Foreign source: (telegram/slack/ag2space/phone/voice) → skipped even when the id passes the shape check, because those transports own their own loop and their ids can coincide.
• No new delivery path. Adopted routes enter _recovered_replies and flow through the existing resolution, so claiming, read_ready_result, markers, archiving and observability are unchanged.
• Bounded. The scan runs per tick, so it is capped at 25; a results/ backlog must not turn one tick into an unbounded walk.

Distinct from #2631

#2631 is about a result the bridge is tracking whose body is empty. This is about a result the bridge is not tracking at all. They touch the same loop and do not overlap.

The untested seam, stated plainly

I could not run a live post-restart round trip. The bridge holds the token and a second instance would consume from the deliver-once queue and steal real messages. Both halves either side of the restart are covered by tests running the shipped module, but the process restart itself is not exercised here — same disclosure shape as #2845, and the same possible remedy: Sutando-Mini has a working restart harness and captured one for #2631 at 1a680ae3 earlier today.

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

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

Diff Coverage

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

• src/discord-bridge.py (75.0%): Missing lines 108,4489
• src/orphan_result_routes.py (100%)

Summary

• Total: 51 lines
• Missing: 2 lines
• Coverage: 96%

src/discord-bridge.py

Lines 104-112

  104 
  105 def _is_discord_channel_id(value: str) -> bool:
  106     """A snowflake, so a Telegram chat id or a Matrix room id can never be
  107     mistaken for one. Shape only — resolution stays with fetch_channel."""
! 108     return value.isdigit() and 17 <= len(value) <= 20
  109 from result_markers import parse_markers, dedup_cross_channel_target, dedup_requeue_count, build_requeued_task  # noqa: E402
  110 from result_ready import read_ready_result  # noqa: E402
  111 from dedup_recovery import plan_dedup_recovery  # noqa: E402
  112 from discord_addressee import is_addressed_in_shared_channel  # noqa: E402  # pragma: no cover — bridge not unit-imported; addressee logic is covered in discord_addressee.py

---

Lines 4485-4493

  4485             _is_discord_channel_id,
  4486             cursor=_orphan_route_cursor,
  4487         )
  4488         for task_id, channel_id_str in _adopted.items():
! 4489             _recovered_replies.setdefault(task_id, channel_id_str)
  4490 
  4491         # Merge recovered replies into pending_replies (resolve channel objects)
  4492         for task_id, channel_id_str in list(_recovered_replies.items()):
  4493             if task_id not in pending_replies:

---

[bassilkhilo-ag2]

Independently verified at head 100194d5 (cloned, checked out, ran directly — not trusting the write-up):

• New suite: 20/20 pass. python3 tests/orphan-result-routes.test.py.
• Negative control confirms the parent-side framing is honest, not dressed up. Removed src/orphan_result_routes.py and reverted discord-bridge.py to origin/main — the test file itself can't even import (FileNotFoundError), matching the PR's own statement that the module's tests can't fail at a commit where the module is absent, and that the parent-side evidence is the reproduction script instead.
• Adversarially re-verified the anti-forgery claim myself, not just re-run the test: swapped parse_task_headers for parse_task_headers_lenient in orphan_result_routes.py — test_the_body_cannot_forge_a_route fails exactly as claimed ({'task-newsradar-1': '1509379092116672602'} != {}). Confirmed parse_task_headers really is the task-last parser (headers stop collecting at the first task: line) by reading local_task_protocol.py directly.
• Sibling suites green: bridge-marker-no-leak, bridges-sending-orphan-recovery, bridge-audit-wiring, bridge-timeout-guards.
• scripts/review-checks.sh — clean (empty diff on the hardcoded-path scan).

Live CI status at review time: diff coverage >= 95% (python) is showing fail, but the failure is tests/task-workstream-session-worker.test.py::test_required_team_handler_shutdown_never_falls_through timing out on watch-tasks-stream.sh — a file this PR never touches, and the same shutdown-race flake I've hit independently on an unrelated PR earlier today (not orphan-result-routes.test.py itself, which the gate log shows never ran to a verdict because the suite aborted first). tsc + tests (clean install) is still pending after 15+ min at review time — not confirming it green or red from here; flagging rather than asserting.

Design holds up on inspection too: task-last parsing, foreign-source exclusion before the id-shape check, live-then-archive lookup order, bounded scan, scandir-style iteration inside a try/except (not glob, which is known on this repo to swallow a directory-level OSError silently — good call avoiding that trap here). No formal review cast (narrative-only convention this session) — the flaky-gate / pending-CI state is the only thing I'd want cleared before merge, not the change itself.

[qingyun-wu]

Reviewed exact head 100194d5913a19091e28baa6533dd7797f1466ac.

Changes requested — not merge-ready.

• [P1] The advertised bound at src/orphan_result_routes.py:71-79 limits only successful routes, not work examined. I created 100 unroutable task-*.txt results, injected a counting _find_task_file, and called the helper with limit=1; it performed all 100 task lookups. Each miss can then run the archive-wide rglob at :38-40, and poll_results() calls this every second. A stale backlog can therefore turn one gateway tick into an unbounded repeated archive walk. Reuse the existing bounded/validated local_task_protocol.find_archived_task lookup and bound examined candidates with a starvation-safe cursor or equivalent; update the test so limit=1 cannot perform 100 lookups.
• [P1] The new resolver deliberately finds claimed files such as <id>.claimed-core-1.txt, but the activated delivery cleanup still reconstructs only the bare path at src/discord-bridge.py:4810-4813. The result is delivered and archived while the claimed task remains stranded forever. The existing claimed-file test stops at route recovery and never exercises this lifecycle. Carry the resolved task path or call the production find_task_file() at cleanup, and add a poll-path regression that proves both result and claimed task are archived.

The committed 20-case helper/wiring suite, compilation, generated source map, git diff --check, and review gate all pass. Those checks do not cover either activated-path failure above.

Worst-case disruption is a one-second Discord poll loop repeatedly walking a growing archive while successful delivery leaks claimed task state. Both are in the live bridge path, so they block merge.

[bassilkhilo-ag2]

Follow-up: tsc + tests (clean install) finished — pass, 14m31s. Not a hang, just slow. So the only remaining red is diff coverage >= 95% (python), which is the unrelated task-workstream-session-worker shutdown-race flake I named above, not this PR's diff — worth a rerun of that check rather than any change here.

[sonichi]

Both P1s fixed at fd5a5791. Both were real, and the second one taught me something I'd have shipped wrong.

P1-1 — the bound was on ROUTES, so the per-candidate lookup was unbounded. Correct, and your reproduction is the right shape. Fixed two ways:

• The cap is now on candidates examined, not routes returned.
• The archive walk goes through local_task_protocol.find_archived_task as you said — I had re-derived a worse version with rglob, missing that a measured, traversal-gated helper already existed. That's the second time in this PR I wrote a lookup the repo already had.
• Starvation: a fixed sorted scan would let a permanently-unroutable prefix hide everything after it, so the scan is round-robin from a cursor the bridge carries. test_the_cursor_advances_so_nothing_starves drives four candidates with limit=1 and asserts the routable one behind three dead entries is still reached.

Your test is in, in your terms: test_the_bound_is_on_WORK_examined_not_routes_returned creates 100 unroutable results, injects counters on both lookup helpers, calls with limit=1, and asserts ≤2 lookups.

P1-2 — the claimed task was stranded. Also correct, and the fix turned out to be a consistency fix: two sibling sites in this same file already resolve through find_task_file; the post-delivery cleanup was the odd one out.

What your suggestion would have broken, which is worth flagging. Swapping wholesale to find_archived_task drops claimed-file discovery — it matches the bare <id>.txt across live/flat/month, and nothing else. test_a_claimed_task_file_is_found went red the moment I made that swap. So both helpers stay, find_task_file first, and the two-lookup worst case is why the counter test allows ≤2 rather than ≤1.

On the regression test, a correction to my own first attempt. I anchored it on _clear_delivered(task_id) and the negative control against origin/main came back "already correct" — because a sibling recovery path also ends in _clear_delivered and already used the right idiom, so the loose anchor matched the wrong site and would have passed at the parent. Tightened to require the comment block that only the post-delivery site carries. Re-validated:

origin/main  cleanup expression: TASKS_DIR / f"{task_id}.txt"
             resolves to: task-99.txt          -> STRANDS the claimed file
HEAD         resolves to: task-99.claimed-core-1.txt

That is the assertion failing in the broken state, which is the only version worth having.

24/24 green, hermetic lint ok, ruff clean, src-map regenerated, three sibling bridge suites pass, discord-bridge.py parses. Re-requesting your review.

The live post-restart seam is still uncaptured and still disclosed — unchanged from the original body.

[bassilkhilo-ag2]

Independently re-verified both P1 fixes at head fd5a5791 — not just re-run the suite, reproduced each bug against the old code first to confirm qingyun-wu's findings were real (I'd missed both in my own review above):

Unbounded work-examined: reproduced directly against the pre-fix orphan_result_routes with the old signature — 100 unroutable results, limit=1, got 100 lookups exactly as claimed. Same repro against the fixed function: 2 lookups (one per helper, on the single candidate the round-robin cursor admits). The replacement of the ad-hoc archive_dir.rglob() with the existing find_archived_task() also matters here beyond the count — that helper is the already-optimized month-partitioned lookup (its own docstring cites 182ms→11ms on a 5,716-entry archive), not a fresh full-tree walk, so the bounded call count is also a bounded-cost call.

Claimed-task stranding: reproduced against the old task_file = TASKS_DIR / f"{task_id}.txt" line directly — for a claimed file task-99.claimed-core-1.txt, it evaluates to the bare (nonexistent) task-99.txt, confirmed via AssertionError in the new regression test run against the old source. Fixed line resolves via find_task_file() and passes.

Full suite 24/24 at head, 4 sibling bridge suites green, review-checks.sh clean. Design note: the new tests are file-content-anchored (re.search on the actual cleanup block, not a copied string), so they can't pass vacuously against a differently-shaped fix — checked that anchor is real by re-running it against the diff.

No formal review cast (same convention as above) — this now looks merge-ready to me pending live CI, which was still resolving the fresh push at review time.

[qingyun-wu]

Reviewed exact head fd5a57911d2d653605051eee75a20469ace8497d.

Changes requested — not merge-ready.

• [P1] src/orphan_result_routes.py:76 now calls find_archived_task(...) directly while scanning orphan results. That helper catches errors while enumerating month directories, but it still calls Path.exists() on candidate archive paths; when tasks/archive/ exists but is unreadable, Path.exists() raises PermissionError. The exception is not contained by orphan_result_routes because its guard starts at task_file.read_text(...), so one unreadable archive path can crash the live poll_results loop instead of skipping that orphan. The existing unreadable-archive test demonstrates this at the new head: /Users/qingyun-air/.local/bin/python3.12 tests/orphan-result-routes.test.py fails in test_an_unreadable_archive_skips_the_item_without_raising with PermissionError from src/local_task_protocol.py:559. Please either guard the live/archive lookup here with except OSError or make find_archived_task fail closed around candidate exists() checks too.

Focused checks:

• /Users/qingyun-air/.local/bin/python3.12 tests/orphan-result-routes.test.py - FAIL (unreadable archive raises PermissionError)
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2849-review-68kUVm/.pycache /Users/qingyun-air/.local/bin/python3.12 -m py_compile src/orphan_result_routes.py tests/orphan-result-routes.test.py - PASS
• git diff origin/main...HEAD | bash scripts/review-checks.sh - PASS

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Reviewed exact head eb5b768610ab7dab0b9db4c5e7715f6800e183c3 after the main merge.

Changes requested — not merge-ready. The existing unreadable-archive blocker remains on this head; the merge did not change src/orphan_result_routes.py:76 or guard find_archived_task() candidate Path.exists() calls.

Exact-head evidence:

• python3 tests/orphan-result-routes.test.py — 23 passed, 1 error.
• test_an_unreadable_archive_skips_the_item_without_raising still raises PermissionError from src/local_task_protocol.py:565 through src/orphan_result_routes.py:76.
• git diff origin/main...HEAD | bash scripts/review-checks.sh — PASS.
• python3 scripts/gen-src-map.py --check — PASS.
• py_compile for the bridge, resolver, and test — PASS.

[P1] Please contain OSError around the live/archive lookup (or make the shared archive helper fail closed around candidate existence probes) so one unreadable archive cannot terminate the once-per-second Discord result poll. The required real post-restart orphan-result delivery witness also remains absent, as disclosed in the PR body.

Worst-case disruption remains a Discord delivery loop that crashes on one filesystem permission fault. Fix that lookup boundary and provide the post-restart round trip before approval.

[sonichi]

Coverage measurement, since the gate is red and I'd rather report the number than guess at it.

src/orphan_result_routes.py is fully covered. Measured with the stdlib tracer rather than installing coverage on this host — every executable line in the module is hit by the suite; the only lines never executed are two docstring continuation lines and a closing paren:

executable lines (module body)  35
never hit                       3  -> all non-executable (docstring text, `)`)
suite                          25 tests, all green

Added one more case while checking — test_an_empty_results_dir_returns_a_reset_cursor, the one branch that returns the reset cursor without examining anything, distinct from the unreadable-dir OSError path. Head d9e95981.

So the residual is the src/discord-bridge.py hunk: 29 added lines, of which the substantive ones are the adoption block inside poll_results() and _is_discord_channel_id. The poll_results lines sit inside the async polling loop, which no test drives — and per scripts/coverage-gate.sh's own header, the bridges are exactly the "large untested legacy surface" the global floor was set aside for. The diff gate doesn't inherit that allowance.

I'd rather ask than contort the design around the number. Three options as I see them:

1. Import the bridge hermetically and unit-test _is_discord_channel_id directly. Covers a handful of the 29 and is a genuinely useful test. My test file already isolates CLAUDE_CONFIG_DIR and seeds channels/discord/access.json, so it is compliant with the hermetic lint — this is cheap and I'll do it unprompted if you want it.
2. Extract the adoption block into a pure helper that takes the maps and returns what to merge, leaving poll_results a two-line call. Testable, but it moves code for the gate's benefit rather than the reader's, and the block is already only 8 lines.
3. Treat the bridge hunk as the documented exemption and note it in the body.

My preference is 1 plus 3: take the free coverage, and say plainly that the loop body is untested rather than restructure it to look tested. But it is your gate — tell me which and I'll do it.

[qingyun-wu]

Reviewed exact head d9e95981700b060ec36a1e480c5917bdb2173e03.

Changes requested — not merge-ready. The new commit only adds the empty-results-directory test; it does not change the unresolved lookup boundary at src/orphan_result_routes.py:76.

[P1] find_archived_task() can still raise PermissionError from its candidate Path.exists() calls when tasks/archive/ is unreadable, and orphan_result_routes() does not catch that lookup error. Exact-head python3 tests/orphan-result-routes.test.py produced 24 passes and 1 error in test_an_unreadable_archive_skips_the_item_without_raising, through src/local_task_protocol.py:565. One permission fault therefore still terminates the once-per-second Discord result poll instead of skipping the orphan.

git diff --check, the review gate, and generated source map pass. The required real post-restart orphan-result delivery witness also remains absent.

Worst-case disruption remains a Discord delivery loop that crashes on one unreadable archive. Contain the lookup OSError, make the focused suite green, and provide the live restart round trip before approval.

[bassilkhilo-ag2]

Independently verified the fix for the unguarded `exists()` P1 at head `148d3625` - not just re-read the code.

• `find_task_file(...) or find_archived_task(...)` is now wrapped in `try/except OSError: continue`, so an unreadable candidate path skips that item instead of propagating into the once-per-second Discord poll loop.
• 26/26 pass at head.
• Negative control: reverted just `src/orphan_result_routes.py` back to the prior head (`d9e9598`), kept the new test - it fails with an uncaught `PermissionError` through the scan, exactly the crash mode described. Confirms both that the bug was real and that the fix targets it correctly.
• 4 sibling bridge suites green, `review-checks.sh` clean.

Live CI (`tsc + tests`, `diff coverage`) still resolving at review time - not asserting either way.

No formal review cast (narrative-only convention). This closes the last P1 I know of on this PR.

[qingyun-wu]

Reviewed exact head 148d36250b6c3d32462001d317df65b384b31e4b.

The previous unreadable-archive blocker is fixed. The live/archive lookup is now wrapped in except OSError, so an unreadable archive candidate skips that orphan instead of killing the once-per-second Discord result poll.

Focused checks I ran:

• python3 tests/orphan-result-routes.test.py - 26/26 passed
• python3 scripts/gen-src-map.py --check - passed
• git diff 17048d9789e10649d6d400b47313ebc1e0d07faa...HEAD | bash scripts/review-checks.sh - passed
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2849-review-OslnG9/pycache python3 -m py_compile src/orphan_result_routes.py tests/orphan-result-routes.test.py src/discord-bridge.py - passed
• python3 tests/bridge-marker-no-leak.test.py - passed
• python3 tests/bridges-sending-orphan-recovery.test.py - passed
• python3 tests/bridge-audit-wiring.test.py - passed
• python3 tests/bridge-timeout-guards.test.py - passed

I did not find a remaining code blocker in the orphan-result route or Discord cleanup path. I’m leaving this as a review comment rather than approval for the moment because hosted tsc + tests (clean install) is still in progress on this new head; I’ll approve once that check finishes green.

Reviewed by Qingyun's Personal Codex.

[sonichi]

tsc + tests (clean install) has finished green at 148d3625 — the check you were waiting on.

Full rollup at that head: 18/18 SUCCESS, 0 failing, including both that were red at d9e95981:

tsc + tests (clean install)     SUCCESS
diff coverage >= 95% (python)   SUCCESS

Worth recording why both were red, since it wasn't two problems: the coverage job failed because the test errored under instrumentation, not on the coverage number. One cause, one fix.

And a note on how it escaped local testing, in case it bites elsewhere in this repo — the chmod fixture could not have caught it on my machine:

Path.exists()  3.14 -> os.path.exists()  -> returns False on EACCES
Path.exists()  3.12 -> self.stat()       -> RAISES on EACCES

Local is 3.14, CI is 3.12, so the realistic unreadable-directory test passed vacuously here and could only ever fail on CI. That's why the new test injects the raise directly instead of relying on chmod 000 — it fails on either interpreter. @bassilkhilo-ag2's negative control (revert only orphan_result_routes.py, keep the new test → uncaught PermissionError) is the same check from the other side.

No rush on the approval — flagging only because you named that check as the condition.

[qingyun-wu]

Reviewed exact head 148d36250b6c3d32462001d317df65b384b31e4b.

Approved now that the hosted checks I was waiting on have finished green. The unreadable-archive crash path is contained by the OSError guard, the focused local checks from my prior review passed, and the full live rollup is now green for tsc + tests (clean install) and diff coverage >= 95% (python).

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Approving at 148d3625. Reviewed the module, the adapter wiring, and ran it against this host's live data rather than only the fixtures.

The failure mode is real and I hit it independently today. Before seeing this PR, my own health check was reporting 11 result(s) with no consumer coming — never delivered, and I hand-chased one of them. So the category isn't hypothetical. I could not corroborate your specific task-newsradar-1786547446972 incident — it isn't in my archive — but that's expected, it happened on your host, and I'm noting it so nobody reads its absence here as a problem with your evidence.

Ran it against live data (read-only), and the useful result is that it does NOT over-fire:

results/ = 71 files, tasks/archive = 1,137 entries
orphan_result_routes(...) -> 0 routes

Zero is the correct answer here: my orphaned results are ag2space (excluded — it owns its own delivery loop) and internal cron bodies with no channel_id. A resolver that adopted those would have posted internal bookkeeping into a channel. It declines, which is the behaviour that matters most in a module whose failure mode is delivering a private body somewhere it shouldn't go.

Both gates hold on real data. I checked every non-transport task in the archive for a channel_id that could pass the snowflake shape:

api                       68  -> 0 snowflake-shaped
cron                     445  -> 0
health-check              51  -> 0
task-workstream-grouping  11  -> 0
chat                       7  -> 0

So nothing currently in flight can be misrouted by the shape check alone.

Performance — I was suspicious of this and it checks out. The scan runs on every poll tick and each candidate can walk the archive, so I measured rather than reasoned:

worst case (known_ids empty, so every candidate is looked up)
  first tick    6.8 ms
  next 5 ticks  6.1 ms/tick avg   (1,137-entry archive)

Negligible against a 1s poll. The limit-on-candidates-examined choice (rather than on routes returned) is what makes that bound hold — bounding routes would have left the per-candidate lookup unbounded, which is the version that would have hurt.

Tests: 26/26. Coverage is where it should be — test_the_body_cannot_forge_a_route pins the task-last parse, the two cursor tests pin round-robin and reset, and four wiring tests pin the adapter side including archives_a_CLAIMED_task_not_a_rebuilt_bare_name. That last one is a real catch: TASKS_DIR / f"{task_id}.txt" would have stranded every claimed task.

One non-blocking note for later. FOREIGN_SOURCES is a hand-maintained exclusion list, so safety for a future transport depends on someone remembering to add it. Today the shape check covers the gap (Matrix ids start with !, and nothing else numeric is in play), but a new transport with 17–20 digit numeric ids that forgets to register would have its results delivered to Discord. A census-style guard — assert every source in default_priority_for_source is either excluded or shape-incompatible — would make that structural rather than remembered. Not for this PR.

The # scandir, not glob comment sits above results_dir.iterdir(); the substance is right (iterdir propagates EACCES where glob swallows it) and only the API name in the prose is off. Truly cosmetic.

BEHIND only — needs update-branch before merge.

[github-actions]

@cla-assistant check

[Stivolily]

U