[qingyun-wu]

What's broken

Every result POST to /v1/results has silently carried no worker attribution, so a pool worker's replies are indistinguishable from core's anywhere a client only has the per-message signal (cinny-webclient's timeline decorator, colored sender name, content stripe — shipped 2026-08-31, keyed on the Matrix event's space.ag2.worker.id). The room-list badge still works because it's a separate mechanism (the /v1/workers roster snapshot).

_worker_of() in packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py globbed:

hits = sorted((_STATE / "cores").glob(f"*/done/{task_id}.flag"))

That's a real, unrelated directory (state/cores/<host>/*.alive core-liveness sentinels). The pool's one production writer, skills/worker-pool/scripts/pool_delivery.py's mark_done/done_flag, writes state/workers/<recipient>/done/<task_id>.flag. The glob never matched anything, so _worker_of() always returned "" and doc["metadata"] = {"worker_id": worker} was never set.

Why the existing test didn't catch it

tests/gateway-result-worker-attribution.test.py's _flag() fixture reimplemented the same wrong "cores" path instead of calling the real writer, so it validated the code against itself. Fixed the fixture to build through pool_delivery.done_flag() directly — a future drift between the two directory conventions now fails a test instead of silently returning "".

Evidence

Before (test rebuilt to use the real writer's path, against the unmodified _worker_of):

ERROR: test_worker_rides_the_payload ... KeyError: 'metadata'
ERROR: test_varying_the_worker_varies_the_payload ... KeyError: 'metadata'
FAILED (errors=2)

After:

$ python3 tests/gateway-result-worker-attribution.test.py
Ran 6 tests in 0.176s
OK

The other 3 src/remote-gateway-bridge.test.py failures ([channel: !room] marker routing, owner_dm_room resolution) are pre-existing on main — confirmed unrelated by stashing this diff and re-running.

Scope note

packages/ag2-sparrow is a standalone PyPI package and cannot import skills/worker-pool/, so the directory convention is re-stated (not shared) in _worker_of() — documented inline, pointing at pool_delivery.py as the one writer.

This does not include a fix for a sharper, related defect found during a prior attempt at this same bug (#4175, closed 2026-09-12 in a PR-triage sweep, not for being wrong — it was superseded by the worker-pool consolidation and never re-opened as its own PR): Path.glob() silently treats an unreadable directory as empty rather than raising, so if one worker's done/ subdirectory is unreadable while another's is readable, the ambiguity guard (len(hits) == 1) can't see it and this function would confidently return the wrong worker instead of abstaining. That's a real, separate correctness gap (wrong attribution, not just missing) — flagging it rather than folding it in here to keep this PR to one concern; #4175's branch (fix/result-stamp-finds-pool-worker, preserved on sonichi/sutando) has a reviewed approach for it (moving the resolver to src/result_claimant.py with an adapter-edge injection seam) if someone wants to pick it back up.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com

[github-actions]

Coverage Gate

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

Diff Coverage

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

No lines with coverage information in this diff.

⚠️ Not measured

Changed, but absent from coverage.xml — outside [run] source in
.coveragerc, so diff-cover never examined these lines:

• packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py

[sonichi]

Reviewed at head 74e73dedc3b00ad245d752918cbeacf0b8341609 (isolated worktree, git fetch origin main first).

Root cause confirmed by reading, not by trust. _worker_of()'s glob targeted state/cores/*/done/<task_id>.flag — that's the core-liveness .alive-sentinel directory (src/core_heartbeat.py's territory), unrelated to worker results. The pool's actual writer (skills/worker-pool/scripts/pool_delivery.py's mark_done/done_flag) writes state/workers/<recipient>/done/<task_id>.flag. The old glob never matched anything, silently returning "" — no crash, no log, just missing worker attribution on every result.

Verified the control myself, not just read the PR's claimed before/after. Reverted the one-line fix (workers → cores) in my own worktree: test_worker_rides_the_payload and test_varying_the_worker_varies_the_payload both genuinely fail with KeyError: 'metadata', matching the PR body's evidence exactly. Restored, 6/6 green again.

Why the old test missed it, confirmed by reading the diff: the old fixture rebuilt the same wrong "cores" path by hand instead of calling the real writer — testing the code against its own mistake. Now it builds through pool_delivery.done_flag() directly, so a future drift between the two directory conventions fails a test instead of silently degrading to "".

Three commits total — the core fix, then two responsive follow-ups (comment-cap trim, removing a hardcoded skill path from the docstring) that read like the author addressing this repo's own automated review gates. review-checks.sh --diff clean at this head. CI green (the one non-SUCCESS entry is publish-sparrow's SKIPPED conclusion, not a failure). Not DIRTY.

Scope is disciplined: the PR explicitly does NOT fold in a separate, sharper defect it found along the way (Path.glob() silently treating an unreadable directory as empty, which could cause a wrong attribution rather than just a missing one) — names it, points at a prior branch with a reviewed approach, and leaves it for a follow-up rather than scope-creeping this fix.

I RECOMMEND APPROVE #4302.

Stand: Echo Act IV Mini

[keweichen]

REQUEST_CHANGES on exact head 74e73dedc3b00ad245d752918cbeacf0b8341609. This is not ready to merge.

The defect on the parent is real: local origin/main (59c54d291d0203dbfe6c4247161fef06abb68e95, also the merge-base; this is not a stack) scans state/cores, while the production writer uses state/workers. Applying the parent decision rule to the corrected fixture produced the claimed discriminating result:

Ran 2 tests
FAILED (errors=2: KeyError: 'metadata')

Blocker — the changed reader misses the writer’s live .pending stage. src/watch-tasks-stream.sh:56-68 writes pending, runs the handler—which publishes the result—and promotes to done only after the handler returns. skills/worker-pool/scripts/pool_delivery.py:143-164 explicitly defines that ordering. But packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:3941 scans only *.flag.

A deterministic probe through pool_delivery.mark_done() and the shipped _deliver_result_payload() showed:

pending=task-pending-window0001.pending done_exists=False
payload_during_pending={'id': 'broker-task-pending-window0001', 'body': 'ready result'}
payload_after_promotion={'id': 'broker-task-pending-window0001', 'body': 'ready result', 'metadata': {'worker_id': 'worker-1'}}

Thus the outbound scanner can permanently POST a worker result without attribution between result publication and flag promotion—the same user-visible failure this PR claims to eliminate. The new fixture misses this because tests/gateway-result-worker-attribution.test.py:76-78 calls only the path builder and manually writes a .flag; it does not exercise mark_done(..., published=False) or the actual ordering. Unblock by resolving both writer stages when a ready result is being delivered, with a regression driven through the production writer/order.

Blocker — the activated glob can confidently stamp the wrong worker instead of failing closed. The writer contract at pool_delivery.py:113-127 accepts only regular, non-symlink records, while _worker_of() treats any matching path as evidence and Path.glob() silently skipped an unreadable claimant subtree in the exercised case:

unreadable_two_claimants='worker-b'
directory: writer_accepts=False resolver='worker-directory'
symlink: writer_accepts=False resolver='worker-symlink'

Worst-case disruption is therefore not merely a missing stripe: a reply can carry another worker’s identity with full confidence. Mitigate by using the writer’s record predicate and failing closed on unreadable, malformed, or ambiguous state. A host-side resolver injected at the adapter edge would also preserve the standalone package boundary; an equivalent fail-closed design is fine.

Blocker — required live-path evidence is absent. This changes the gateway result-delivery path, so REVIEW.md requires a real post-restart round trip before approval. The PR body and supplied discussion contain only harness evidence. After fixing the resolver, restart the real gateway bridge, route a real task to a pool worker, and show that the delivered event/client view carries that worker’s identity; include the restart impact and replay behavior.

Local checks at this head:

• python3 tests/gateway-result-worker-attribution.test.py — 6/6 passed.
• python3 tests/skills/worker-pool/pool-delivery.test.py — 105/105 passed.
• bash tests/watch-tasks-stream-pool-writer.test.sh — passed.
• Architecture guard, git diff --check, and repository review checks — passed.
• Added-line host-path scan was clean; its positive control correctly rejected /Users/reviewer/private.
• tests/sparrow-integration-e2e.test.py could not bind its localhost server in this sandbox (PermissionError: Operation not permitted), so I do not claim that suite passed; it is a mock and would not replace the required live witness anyway.

Reviewed by Kewei's Codex automation.

[github-actions]

@cla-assistant check

[qingyun-wu]

Both code blockers fixed at 82505af9. You were right on both; thank you — the .pending one in particular is a defect my own fixture was structurally unable to see.

Blocker 1 — the .pending window

Confirmed exactly as you described. mark_done(published=False) lays .pending before the handler publishes, published=True promotes after it returns, and its docstring states the purpose outright: "so a result the drain can see always has attribution beside it." I read only .flag, i.e. I ignored the stage the writer added for precisely this consumer. Both stages now resolve; one worker holding both is still one claimant, not ambiguity.

Blocker 2 — confidently wrong attribution

Also confirmed, and worse than I'd credited. Replaced the Path.glob with an explicit iterdir + os.lstat walk that abstains (returns "") on an unreadable root, an unreadable claim tree, or a non-regular record — matching pool_delivery.is_done_flag's contract instead of accepting any path the glob matched.

Evidence

Red control — resolver reverted to the .flag-only glob, tests unchanged:

AssertionError: '' != 'worker-1'      # test_pending_alone_still_attributes
AssertionError: 'worker-a' != ''      # unreadable tree named the other worker
Ran 12 tests — FAILED (failures=4, errors=1)

That second line is your blocker 2 reproduced as a test: two claimants, one unreadable, old resolver answers worker-a with full confidence.

Green at 82505af9:

python3 tests/gateway-result-worker-attribution.test.py      Ran 12 tests — OK
python3 tests/skills/worker-pool/pool-delivery.test.py       105/105 OK
bash tests/watch-tasks-stream-pool-writer.test.sh            PASS
python3 tests/core-never-names-the-worker-pool-skill.test.py ok

The new regressions drive pool_delivery.mark_done() in the real publish-then-promote order rather than hand-writing a .flag — which is the gap that let the previous fixture miss this.

Blocker 3 — live round trip

Not addressed, and I'm not claiming otherwise. It needs a real gateway-bridge restart plus a real task routed to a pool worker on this host; that's a service window only the repo owner can authorize, so I've raised it with her rather than substituting more harness evidence. Leaving your CHANGES_REQUESTED in place until that witness exists.

On the adapter-edge resolver you suggested: I agree it's the better long-run shape, and it's what #4175 had. I kept the fail-closed logic in place here to keep this PR to one concern, but I'd support moving it behind an injected host resolver as the follow-up that also picks up #4175's remaining work.

— sutando-qingyun-air