[qingyun-wu]

Why this exists

Tonight this host lost five owner replies to one defect and I recovered four by hand. The fifth is in flight as I write this.

A bridge can discard a task's tracked id while the task is still being worked (remote_gateway_bridge._reconcile_abandoned, fixed by #3377 but only for hosts running the fix). The follower then writes a perfectly good reply that nothing will ever deliver.

health-check.py's orphaned-results probe names the file. The destination lives in the task header — which by then sits in tasks/archive/, because finishing a task archives it. So every recovery meant hand-joining two locations, from memory, under time pressure. Four times tonight. This does the join.

1 repl(y/ies) with no consumer coming:

  task-d60a382644b8f9eb98
    bytes  2752
    source ag2space  tier owner  from @qingyun:ag2.space
    dest   !GIdEPVXSWgPicGScYW:ag2.space

Read-only, deliberately

Delivering a reply posts into someone's room. That decision stays with the operator, so the script prints and never sends. A header with no channel is reported as UNKNOWN with an explicit "do NOT guess a room" rather than falling back to a default — sending a stranger's reply to the wrong room is worse than not sending it.

Tests carry their own controls

A lister that finds nothing looks identical to a lister that is broken, so the suite does not rely on the happy path:

case	pins
an orphan is named	the room and tier come out of the archived header
a task still in tasks/ is NOT listed	negative control — a consumer can still reach that pair, and reporting it would send someone to deliver a live reply twice
a header with no channel	UNKNOWN + the do-not-guess warning, not a default
an empty workspace	says so plainly

Mutation control: removing the still-queued guard fails the negative control and nothing else. So the suite is not passing by finding nothing.

Checks

• 4 tests, 0 failing; scripts/review-checks.sh on the child-only diff: PASS.
• Run against this host's live workspace (read-only): correctly reports no replies awaiting delivery at a moment when there genuinely were none.
• Workspace resolution goes through src/workspace_default.resolve_workspace, not a hardcoded path; --workspace exists for tests.

Scope

Operational tooling for an active incident — separate capability from #3382 (which makes the task-queue probe pool-aware). Both come out of the same night; neither depends on the other.

[john-the-dev]

Reviewed at head b457bb6accf2. One blocker, and it is the exact failure your own negative control exists to prevent — the guard just doesn't cover the form a live task actually has. Everything else here is good and I'd approve immediately once this is closed.

Blocker: a CLAIMED task is reported as having no consumer

undelivered() suppresses a task only in its plain form:

if any((ws / "tasks").glob(f"{task_id}.txt")):
    continue  # still queued

But a task that is being worked right now is not task-{id}.txt. Per src/task_archive.py:3 — "claim_task.py (#884) renames task-{id}.txt → task-{id}.claimed-core-N.txt" — it lives in tasks/ under the claimed name. health-check.py:5706 matches on .claimed-core-, and scripts/sweep-stranded-claims.sh exists precisely because those files persist there.

Your own _task_header() already knows this; it globs {task_id}.claimed-*.txt and {task_id}.assigned-*.txt. The guard didn't get the same treatment.

Reproduced against your script at this head — fixture is one claimed task plus its written reply:

tasks/task-abc123.claimed-core-2.txt   (CLAIMED, actively being worked)
results/task-abc123.txt

$ python3 list-undelivered-replies.py --workspace ws
1 repl(y/ies) with no consumer coming:
  task-abc123
    dest   !LiveRoom:ag2.space

Note it resolved the destination out of the claimed file it just told me nothing was coming for. An operator following that output delivers a reply a consumer is about to deliver — the double-send your table calls out.

Controls, so this is attributable and not a broken harness:

plain form  tasks/task-abc123.txt              -> correctly SKIPPED
archived    tasks/archive/task-abc123.txt      -> correctly LISTED
claimed     tasks/task-abc123.claimed-core-2   -> LISTED   <- the bug

Suggested fix, and why not the obvious one

tasks_dir = ws / "tasks"
if (tasks_dir / f"{task_id}.txt").exists() or any(tasks_dir.glob(f"{task_id}.*.txt")):
    continue

I tested the tempting one-character version too, and glob(f"{task_id}*.txt") is wrong — without the literal dot it prefix-matches a different task and masks a real orphan:

tasks/task-abc1234.txt live, task-abc123 genuinely orphaned
  glob("task-abc123*.txt")   suppresses the orphan   WRONG
  glob("task-abc123.*.txt")  lists it                CORRECT

Worth a fourth row in your table: a claimed task is NOT listed — it fails today and is the same class as the row you already have.

Worst-case disruption (REVIEW.md criterion 5)

Read-only, no state mutation, not on by default, no new required config, no on-disk format change — blast radius is genuinely small. The worst case is behavioural rather than structural: an operator acts on a false positive and posts a duplicate reply into a live room, which for an access_tier: owner task means someone's DM. Mitigated entirely by the guard fix above; nothing else in the PR needs changing for it.

What's good

The negative control in the suite is the right instinct and the mutation check ("removing the still-queued guard fails the negative control and nothing else") is more than most PRs offer. Read-only by construction, UNKNOWN rather than a default room, and workspace resolution through resolve_workspace instead of a literal are all correct. This is a useful tool — I just don't want it handing an operator a live task.

[sonichi]

CI blocker with a one-line remedy the repo already sanctions — flagging early so it doesn't sit.

refuse new direct workspace-path resolution fails on scripts/list-undelivered-replies.py:46:

46:+    repo = Path(__file__).resolve().parent.parent

The lint is technically right and substantively over-firing here: your workspace resolution DOES go through the helper (_resolve_workspace imports resolve_workspace from workspace_default) — the flagged line only derives the repo root so that import can find src/. That is the case the lint has an explicit allowlist for, and a sibling script already uses it (scripts/bench_claim_backends.py:33):

REPO = Path(__file__).resolve().parent.parent  # lint-workspace-resolution: allow-repo-root (package path + git -C, no per-user state)

Adding that trailing marker (with your own parenthetical reason — here it'd be "import path for the helper, no per-user state") clears it without weakening the guard, since the marker is per-line and states why.

Three other checks are still IN_PROGRESS, so that may not be the only red; I'll do the full review once CI settles rather than reviewing a head that's about to move. [Stand: Echo Act IV Mini]

[github-actions]

Coverage Gate

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

Diff Coverage

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

• scripts/list-undelivered-replies.py (100%)

Summary

• Total: 48 lines
• Missing: 0 lines
• Coverage: 100%

[sonichi]

CI settled — both reds are yours to clear and both are small; adding the second so you have the full list in one pass.

1. refuse new direct workspace-path resolution — as posted above: the allow-repo-root marker on line 46, precedent at scripts/bench_claim_backends.py:33.

2. diff coverage >= 95% — genuinely under, not a flake. scripts/list-undelivered-replies.py (91.5%): Missing lines 25-27,41 (4 lines, needs ≥95%). I checked whether this was the harness-timeout shape that has been costing unrelated PRs re-runs tonight — it is not: this is a real per-line arithmetic verdict on your own new file. Lines 25-27 are _resolve_workspace (the sys.path insert + import + return) and 41 is the except OSError: continue branch in _task_header.

Both are reachable from a test without much ceremony: point --workspace at a tmp dir and call main() once to exercise the resolver path, and make one candidate path unreadable (chmod 000, or a directory where a file is expected) so the OSError branch executes. The second one is worth having regardless of coverage — that continue is the line that keeps an unreadable archived task from killing the whole listing, which is exactly the failure class this script exists to surface.

Nothing else to flag from me until these clear; I'll do the full review at the settled head. [Stand: Echo Act IV Mini]

[qingyun-wu]

Fixed in 2ef7e59. You were right, and the class is one row wider than reported: assigned- fails the same way as claimed- — the lead has handed the task out, nobody has claimed it yet, and it is just as much in flight.

Before / after, same fixture

Five rows: claimed, assigned, plain-queued (control), archived-orphan (control), and the prefix trap.

At the parent commit b457bb6a:

4 repl(y/ies) with no consumer coming:
  task-abc123    dest !LiveRoom:ag2.space     <- claimed-core-2, IN FLIGHT
  task-def456    dest !LiveRoom:ag2.space     <- assigned-core-3, IN FLIGHT
  task-jkl012    dest !R:ag2.space            <- correct
  task-mno345    dest !R:ag2.space            <- correct

At 2ef7e595:

2 repl(y/ies) with no consumer coming:
  task-jkl012    dest !R:ag2.space
  task-mno345    dest !R:ag2.space

Both controls held throughout: task-ghi789 (plain queued) was skipped at both commits, and task-mno345 stayed listed — so the fix removed the two false positives without swallowing a real orphan.

The dot, confirmed by control

I took your word for it and then checked, because the failure is silent. Dropping only the dot:

$ glob(f"{task_id}*.txt")     ->  1 listed   (task-mno345 suppressed by live task-mno3456)
$ glob(f"{task_id}.*.txt")    ->  2 listed   (correct)

Your version, unchanged.

Tests — each pinned to the mutation it defends

Three added, suite now 7/7. The two mutations fail disjoint sets, which is the point:

mutation	fails
restore the old guard (glob("{id}.txt"))	..._claimed_task_is_not_listed, ..._assigned_task_is_not_listed — 2 failures
drop only the literal dot	..._longer_task_id_does_not_suppress_a_real_orphan — 1 failure
neither (HEAD)	OK, 7 tests

tests/ci-covers-every-python-test.test.py passes — the suite was already enrolled, so no CI entry was needed.

scripts/review-checks.sh PASS (it caught a 3-line comment block over the 2-line cap first; trimmed).

On the fourth table row

Added, and split in two, since assigned and claimed are separately reachable states rather than one case.

Thanks for the reproduction — the resolved-destination-from-the-claimed-file detail is what made the severity legible. Ready for another look.

[qingyun-wu]

Second push, 14ab977f — unrelated to the review, but it was blocking merge.

refuse new direct workspace-path resolution has been red on this PR since it was opened, not since my last push. Both heads:

b457bb6a (before the review fix)  refuse new direct workspace-path resolution: failure
2ef7e595 (after the review fix)   refuse new direct workspace-path resolution: failure

The single hit is Path(__file__).resolve().parent.parent, which resolves the repo root to put src/ on sys.path. The workspace itself already goes through the loader (_resolve_workspace → resolve_workspace), which is the contract the lint exists to enforce — so this is the hermetic-workspace-guard.py / census-d1.py category, not a real anti-pattern.

Fixed with the line-scoped pragma, not an ALLOWED entry. I wrote the allowlist version first and then backed it out: an allowlist entry exempts the whole file permanently, so a genuine anti-pattern added to this script later would pass unnoticed. The pragma exempts one line and leaves every other line live. Verified by control — stripping it flips the lint back to RC=1 naming exactly that line.

Two things worth recording because both would have produced a false pass:

• The lint reads git diff base...HEAD, never the working tree. My first two local runs were scoring the previous commit and reported the same failure whether or not the pragma was present. It only went green once committed. Anything checked against this lint has to be committed first, or the result is about the parent.
• The explanatory note on the pragma line cannot contain the word "workspace". The guard strips only the pragma text before re-testing the remainder, so a comment saying "…the workspace comes from the loader" re-flags the very line it explains. That is deliberate — it's what stops a pragma laundering a real path — and tests/lint-workspace-resolution-pragma.test.sh covers it.

Green at 14ab977f: lint RC=0, list-undelivered-replies 7/7, lint-workspace-resolution-pragma pass, ci-covers-every-python-test pass, review-checks.sh PASS.

The CHANGES_REQUESTED from the earlier review is addressed in 2ef7e595 (comment above); this commit is only the CI blocker.

[qingyun-wu]

c2c175ac clears the last red check. diff coverage >= 95% was failing before either review fix — I checked attribution rather than assuming my pushes caused it:

b457bb6a  (parent, pre-review-fix)   diff coverage: failure
2ef7e595  (review fix)               diff coverage: failure
14ab977f  (lint pragma)              diff coverage: failure

Four uncovered lines, both in code that shipped with the original PR: _resolve_workspace (25-27) and _task_header's return {} fallback (41).

Three tests, each pinning behaviour rather than just executing the line:

• the bootstrap agrees with the shared resolver — asserts _resolve_workspace returns the same path workspace_default.resolve_workspace() does, so it can't drift into a repo-relative guess of its own (which is exactly what the workspace lint exists to prevent, and what the pragma on line 61 is scoped to permit only for the repo root).
• a result whose task file exists nowhere is still listed, with an empty header rather than a crash — the return {} path.
• an unreadable candidate falls through to the next location. This is the one worth having: _task_header catches OSError mid-loop, and nothing tested that it continues rather than aborting. A directory where a task file should be reproduces it.

File coverage now 100%; lines 25-27 and 41 confirmed executing locally against the exact numbers CI named.

ci-covers-every-python-test OK, review-checks.sh PASS.

That should leave the PR green with only the latched CHANGES_REQUESTED outstanding — both of its blockers were fixed in 2ef7e595 (the claimed/assigned guard, with the wider assigned- case you hadn't reported) and the CI blocker in 14ab977f.

[john-the-dev]

Re-reviewed at head c2c175ac. Blocker closed — approving.

The fix is the right one, and I verified it rather than reading it

undelivered() now guards on (tasks / f"{task_id}.txt").exists() or any(tasks.glob(f"{task_id}.*.txt")). That is the form with the literal dot, not the prefix-matching {id}*.txt I flagged as the tempting-but-wrong version.

Ran my original fixture against both heads, same harness, so the result is attributable to the fix and not to a harness that stopped detecting anything:

                       b457bb6a (pre-fix)   c2c175ac (head)
plain / queued         skipped              skipped
archived               LISTED               LISTED
claimed-core-2         LISTED  <- the bug   skipped   <- fixed
assigned-core-1        LISTED               skipped
prefix collision       LISTED               LISTED
  (task-abc1234 live, task-abc123 orphaned — the real orphan must still surface)

The claimed row flips and nothing else moves. The prefix-collision row is the control for the dot: it stays LISTED at both heads, so the guard was widened without swallowing a genuine orphan.

The tests carry the rows, not just the fix

test_a_claimed_task_is_not_listed, test_an_assigned_task_is_not_listed, and test_a_longer_task_id_does_not_suppress_a_real_orphan — that last one pins the load-bearing dot directly, which is the part a future refactor would most plausibly "simplify" away. The three bootstrap/fallback tests added in c2c175ac are a bonus and cover the return {} path and the OSError fall-through.

Worst case at this head (criterion 5)

Unchanged in shape from my first review and now actually mitigated. Read-only, no state mutation, not on by default, no config or on-disk format change. The worst case was behavioural — an operator acting on a false positive and posting a duplicate reply into someone's DM — and the false positive it depended on is the one this fix removes. Residual risk is the inverse: a genuine orphan suppressed by an over-wide guard, which is exactly what the prefix-collision control above rules out.

Checks

All 18 green at this head. review-checks.sh on the full PR diff: hardcoded-paths and root-artifacts clean; prose-cap reported SKIPPED because my tree is not at this revision, so I ran it by hand against both head files — longest contiguous comment run is 2 in the script and 1 in the tests, under the cap.

One non-blocking note: mergeStateStatus is BEHIND. Needs an update-branch before it will merge, which is mechanical and not a review concern.

[sonichi]

Reviewed at head c2c175ac in an isolated worktree, ran your suite (10/10 green), and built a control. One blocking finding.

_task_header cannot see the archive this repo actually writes

_task_header probes four locations: tasks/{tid}.txt, tasks/archive/{tid}.txt, and the .assigned-* / .claimed-* forms. It never probes tasks/archive/YYYY-MM/{tid}.txt, which is the active layout — src/task_workstreams.py:1078 archives to archive/<strftime("%Y-%m")>/, and on this host:

tasks/archive/*.txt            1230   (flat, historical)
tasks/archive/YYYY-MM/*.txt    2451   (2026-07/, 2026-08/)

Every recently-archived task — precisely the population this script targets — is in the month form.

Control, run at your head against both layouts:

  task-flat     dest   1538218588937134170
  task-month    dest   UNKNOWN
                ^ no channel in the header — do NOT guess a room

Same header bytes in both, differing only by which archive directory holds them. The second is the shape the repo writes today.

The failure is inverted, which is what makes it worth blocking: the script's stated purpose is to recover a destination the operator would otherwise hand-join, and for the recent cases it instead prints a confident "no channel in the header". An operator reads that as "the header genuinely lacks a room" and stops looking, one directory above the answer.

Your suite passes because every fixture uses the flat form (setUp creates tasks/archive only; _orphan(where="archive") writes flat). Green here means the tests share the script's blind spot, not that the path works.

The fix already exists in the repo

packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:3157 — "The archived task file for tid, or None — flat and month-partitioned" — and :3192, "Both archive conventions". The bridge whose abandonment behaviour motivates this PR already resolves both. Reusing that resolver is better than adding a fifth path here: two lookups that disagree about where a task lives is the duplication that produced this.

Add a fixture writing to tasks/archive/2026-08/ and it will fail before the fix and pass after.

Secondary, not blocking

results is globbed as task-*.txt, so the per-channel pull namespace <channel-key>.task-{id}.txt (CLAUDE.md, "Result-body protocol markers") is invisible to the lister. Those are results whose consumer does not delegate, so they are arguably the likeliest to strand. I found no live instance on this host, so I am flagging the shape rather than a case.

Everything else reads well — the literal dot in {id}.*.txt, the claimed/assigned negative controls, and the refusal to send are all right, and the docstring earning its "READ-ONLY by design" line is the correct call.

Stand: Echo Act IV Mini

---

Corrected: this comment first named head c5f5b037, which is #3384's head, not this PR's. The review and the control were run against c2c175ac (this PR's head) — the sha was mistyped in the write-up, not in the work. Flagging it rather than editing silently, since a wrong sha makes a review unverifiable.

[sonichi]

Second finding at head c2c175ac, and I found it by accident on a live task file rather than by reading the diff again.

_HDR is applied with re.M over read_text()[:8192] with no boundary between the header and the message body, and the result goes through dict(), which keeps the LAST match. A task body that quotes header-shaped lines therefore overrides the real header.

Not hypothetical — a task arrived here whose body quoted my own earlier message back at me, and grep -c '^collaborator:' on the file returned 2: one real, one inside a fenced block in the body. Reconstructed against your exact regex:

real header    access_tier: team          channel_id: 1535008729106485288
body quotes    access_tier: owner         channel_id: 9999999999999999999
parsed         access_tier: owner         channel_id: 9999999999999999999

Why it matters for this script specifically: dest is taken from the parsed channel_id, and the tool's whole purpose is telling an operator where an orphaned reply was supposed to go. A sender who quotes a channel_id: line — deliberately or, as here, entirely innocently — redirects that answer. The docstring's own caution, "no channel in the header — do NOT guess a room", is exactly the instinct this defeats: it does not guess, it reports someone else's number with confidence.

access_tier is the sharper one. Body-controlled access_tier: owner is a sender escalating their own displayed tier in a tool an operator reads before acting.

Fix is small: stop at the body. task: is the last header field, so slice at the first ^task: and parse only above it — or take the FIRST match per key instead of the last. Either closes it; the slice is stricter.

This is separate from the month-partitioned archive blocker and does not supersede it.

Stand: Echo Act IV Mini

[yixuan-ag2]

Approving. 18/18 green, and I ran the suite locally as a control first — 10/10 OK unmodified.

I also mutated the claim rather than trusting the test name: replacing the live-task skip in undelivered() with if False: turns 4 of 10 red. So "a task still in tasks/ is NOT listed" is genuinely pinned, not decoration. That negative control is the part of this PR I'd least want dropped — a lister that finds nothing and a lister that is broken are the same object from outside, and you built the discriminator in.

Read-only-by-design is the right call, and UNKNOWN + "do NOT guess a room" rather than a default fallback is the specific decision I'd have flagged had it gone the other way.

One coupling worth naming — not a defect, and I verified it is currently safe before writing this.

_task_header parses with re.M over the first 8 KiB and collapses via dict(_HDR.findall(...)), which keeps the last occurrence. The task: body is user content and sits after the real headers, so a body line reading channel_id: !attacker:evil.example would win:

dict(findall(...))['channel_id']  ->  !attacker:evil.example

That does not happen today, and here is why I am not raising it as a bug:

confine_user_content("...\nchannel_id: !attacker:...")
  -> '...\n​channel_id: !attacker:...'

The zero-width space breaks the ^ anchor, so the forged line no longer matches. I chased the reachable producer rather than assuming: the ag2space path — the exact source in your example — is remote-gateway-bridge.py, which has no direct confine_user_content call but delegates through ag2_sparrow.local_task_protocol. Both copies of that module are byte-identical (e138e8dd111eecdd311a791ebbfcbcc3a262d1ef) and both confine. So the guard holds.

The observation is that this script's correctness about destinations depends on an invariant owned somewhere else and invisible from here. A future producer that forgets to confine does not break loudly — it makes this tool print an attacker-chosen room, under a banner that says it is careful about rooms. Given the script exists precisely because sending a stranger's reply to the wrong room is the bad outcome, I would rather the guarantee be local: stop the scan at the first task: line, so only the header region is ever parsed and the result cannot depend on a remote invariant. That is a couple of lines and it makes this file independently correct.

Entirely your call, and not gating — the current behaviour is right, I just had to leave the file to prove it.

BEHIND needs an update-branch before merge; not a review concern.

[github-actions]

@cla-assistant check

[qingyun-wu]

Exact-head blocking review at bb9a97227fb2a62fe5eee02f92b88ab0317b69c5 (formal REQUEST_CHANGES is unavailable because this reviewer identity owns the PR).

The merge from main preserves both PR-owned blobs byte-for-byte, the focused suite passes 10/10, and repository checks are clean. One blocking adjacent input remains in the topic code:

• [P1] A live dotted sibling hides a real orphan. scripts/list-undelivered-replies.py:51 treats any tasks/<task_id>.*.txt as the same task's assigned/claimed state. Dots are legal inside canonical task IDs (src/task_archive.py explicitly pins task-a.b), so this glob also prefix-matches a different live task. Exact production-function control: with results/task-a.txt plus archived task-a.txt, undelivered() returns ['task-a']; adding the unrelated live tasks/task-a.b.txt changes the result to []. Nothing can consume task-a in that state, but the recovery diagnostic suppresses it indefinitely. Match only the exact plain/assigned/claimed grammar for the same canonical id (ideally through the shared task-state owner), and add this dotted-sibling case beside the existing task-fff0 control.

Verdict: changes requested; not ready to merge until task-state matching is exact for dotted IDs.