[qingyun-wu]

The chain

A reply the client can attribute travels: drain → relay → client.

1. Drain — _deliver_result_payload in packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py builds the result POST and sets doc["metadata"] = {"worker_id": _worker_of(tid)}.
2. Relay — turns that into the Matrix event content space.ag2.worker = {"id": …}.
3. Client — draws the answering worker's name/colour from its roster, keyed on that id.

Only the id is stamped, never a label: a label is broker-owned intent and would go stale inside a frozen event.

The gap

_worker_of globbed one layout only:

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

That is the pre-pool per-core layout. A pool worker writes its done flag somewhere else — src/pool_delivery.done_flag:

return _root(workspace) / "state" / "workers" / recipient / "done" / f"{task_id}.flag"

state/workers/<wid>/done/ never matches state/cores/*/done/, so a pool worker's reply went out with no metadata — the relay had nothing to copy and the client could not say who answered.

The review found the sharper half of the same defect: Path.glob reports a PermissionError as an empty directory, so a claim tree that could not be enumerated read as "nobody claimed it here" and the other tree's flag was stamped with full confidence. That is a definite wrong attribution — the client draws another worker's name and colour — which is strictly worse than an unattributed reply.

The fix

Two changes, one root cause: the packaged adapter was answering "which worker finished this" out of Sutando's state layout, spelled inside itself.

1. The layout moves to src/result_claimant.py and is injected into the package by src/remote-gateway-bridge.py via set_claimant_resolver(resolve_claimant) — the same adapter-edge seam set_task_stamper already uses (CLAUDE.md, "Optional capability discovery stays at the adapter edge"). packages/ag2-sparrow ships standalone and now knows no state layout: with no resolver injected, _worker_of returns "".
2. The scan is fail-closed. os.scandir replaces Path.glob, so an absent root is skipped (that layout is simply not in use on this host) while an unenumerable root raises. Ambiguity, unreadability and malformed state all resolve to no metadata plus one log line saying why.

The pool half of the layout is not re-spelled in result_claimant.py — it is derived from its own writer:

def _pool_flag(state: Path, worker: str, task_id: str) -> Path:
    rel = pool_delivery.done_flag(_PROBE, worker, task_id).relative_to(_PROBE / "state")
    return state / rel

On @john-the-dev's nit (the try spanning both roots discards a hit already found under workers): the structural half is fixed — each root now handles its own FileNotFoundError, so an absent cores masks nothing. The partial answer it asked for is deliberately not taken: @keweichen's P1 is the proof that the partial answer is the defect. Abstaining globally is the policy.

The writer contract — this PR adds the writer

@keweichen's P1: the stack had a flag constructor and two readers, and no production finisher. It does now, here, minimally.

path constructor	pool_delivery.done_flag — #4106 (100b70a89, merged)
writer	pool_delivery.mark_done — this PR, and it is the only one
readers	pool_delivery.residue (is_done_flag) and src/result_claimant.py (the gateway drain's resolver)
ordering	flag, then result. The worker-side publish path calls mark_done before the result becomes visible in results/
what counts as a flag	one predicate, pool_delivery.is_done_flag — a regular file, nothing else

Written into src/pool_delivery.py's module docstring so the next reader finds it beside the layout, not in a PR body.

Why that ordering and not the other

The two crash windows are not symmetric:

• flag, no result — recoverable. residue already maps it to finished, and the boot sweep retires the delivery. Nothing has left the host.
• result, no flag — not recoverable. The drain reads results/, stamps whatever attribution exists at that instant, and POSTs. Once the reply is out, the missing worker id cannot be added to it.

So the writer runs first and the publish is gated on it. That is also why a failed mark_done does not fall through to publishing: the watcher leaves the claim unsettled and says so on stderr, which the existing cross-restart retry picks up.

Where the call lives, and why there

src/watch-tasks-stream.sh → record_worker_done, a new function of its own, called from publish_terminal_failure immediately before the ln that publishes the result.

• That ln is the only place in the watcher that publishes a file into results/ — the one worker-side finish path that exists on this base. (The success path's result is written by $SUTANDO_TASK_EVENT_HANDLER itself, a runtime-injected binary this stack does not ship; when a pool finisher lands, it calls the same mark_done before its own publish.)
• It is gated on SUTANDO_INSTANCE_ID, which src/spawn_worker.py:99 sets for a spawned worker and nothing sets for the core. The core's behaviour is byte-for-byte unchanged: record_worker_done returns 0 before touching anything.
• It is a separate function on purpose: feat(pool) [5/5]: the router pass — resolve one task, write the deliveries #4110 (feat/pool-router-pass) edits finish_handler_task's rc branching and rewrites dispatch_task, and touches neither publish_terminal_failure nor any new name, so the eventual stack rebase does not collide.

The off-stack pool_follower.py, named explicitly

@keweichen inspected 0982cd467:src/pool_follower.py:203-212 and is right about it: it exposes the result with os.replace before writing its (legacy cores/) flag — the drain race in reverse. That commit is not an ancestor of this PR and is not made one here. What this PR does is settle the contract it has to satisfy: when that finisher line lands, its publish moves behind mark_done, and tests/pool-done-flag-writer.test.py is the test that says so. The legacy cores/ reader is unchanged and still reads whatever that line writes today.

The non-regular flag — @keweichen's other P1

src/result_claimant.py discarded the os.stat() result and added the worker for any filesystem object, while pool_delivery.py:189 recognised a flag only with .is_file(). Two readers of one record, disagreeing about what the record is.

Fixed at the writer's side: pool_delivery.is_done_flag is now the single predicate, and both readers ask it. It answers False only for absence; any other stat error still propagates, so the fail-closed unreadable-tree behaviour @keweichen verified at 9d25ce466 is preserved rather than swapped for Path.is_file()'s silent False.

Malformed state is not treated as an absence either. A non-regular object at a flag's own name raises Unattributable, so the gateway abstains and logs — otherwise the other layout's flag becomes the single hit and gets stamped with full confidence, which is the exact shape of the bug this PR exists to remove.

Tests

The eight cases move out of packages/ag2-sparrow/tests/test_result_stamp_worker.py (deleted) into the auto-discovered tests/gateway-result-worker-attribution.test.py, which drives the shipped loader and so exercises the injection seam rather than mocking it. That resolves three asks at once: no CI registration is needed, no /usr/bin/python3 interpreter path survives, and the seam is covered. Three cases are added: the unreadable-tree regression, and the two seam controls (no resolver injected; raising resolver). 11 cases total.

Ask 3 (new) — a directory at the flag path emitted a confident worker id

@keweichen's probe, as two added cases in tests/gateway-result-worker-attribution.test.py. Run against the reviewed 9d25ce466 reader (only src/result_claimant.py reverted; bb6d6ab8 → 98f0332a, verified mutation):

$ python3 tests/gateway-result-worker-attribution.test.py
FAIL: test_control_a_directory_at_the_flag_path_is_not_a_finish
  AssertionError: 'metadata' unexpectedly found in
    {'id': 'broker-task-1notafile000000f', 'body': 'done!', 'metadata': {'worker_id': 'worker-1'}}
FAIL: test_control_a_malformed_pool_flag_does_not_let_the_legacy_core_win
Ran 13 tests in 1.160s
FAILED (failures=2)                                                             rc=1

The first line is posted_metadata={'worker_id': 'worker-1'} reproduced verbatim.

Second control — the obvious fix is not enough. With the regular-file check in place but the malformed-state raise dropped (i.e. a directory simply treated as absence), the mixed-layout case gets worse, not better:

$ python3 tests/gateway-result-worker-attribution.test.py
AssertionError: 'metadata' unexpectedly found in
  {'id': 'broker-task-0mixedlayout0001a', 'body': 'done!', 'metadata': {'worker_id': 'core-2'}}
FAILED (failures=2)                                                             rc=1

That is the "malformed state must not let the legacy cores/ claimant win silently" case: dropping the bad flag on the floor leaves core-2 as the single hit. At HEAD:

$ python3 tests/gateway-result-worker-attribution.test.py
Ran 13 tests in 1.030s
OK                                                                              rc=0

Ask 4 (new) — the writer, and the drain race

tests/pool-done-flag-writer.test.py, auto-discovered. It calls the production writer (pool_delivery.mark_done) and the production drain resolver (result_claimant.resolve_claimant); the ordering scenario drives the shipped src/watch-tasks-stream.sh — the real script, a real worker run, its handler killed so the real publish_terminal_failure fires — with an observer polling results/ at 1 ms and asking the drain's own question the instant the file becomes visible.

$ python3 tests/pool-done-flag-writer.test.py
scenario: flag-before-result, through the shipped watcher and drain
  ok   the worker's watcher published a result
  ok   the observer caught the result appearing
  ok   and its flag was ALREADY there when it did
  ok   the flag the writer left is a regular file
  ok   and the drain resolves it to this worker after the fact
  ok   no temp file is left in the flag's directory
scenario: concurrent finishers, production writer
  ok   every finisher wrote its own flag
  ok   each task resolves to the worker that finished it
  ok   four workers claiming one task are all visible to the drain
  ok   no partially written flag is left behind
scenario: the same finish, twice
  ok   the tree still names exactly one claimant
  ok   and the drain resolves it
scenario: crash between the flag and the result
  ok   no result was published
  ok   the flag still attributes the task
  ok   and residue calls that state recoverable, not lost
scenario: writer bounds
  ok   rejects recipient '../escape' / 'Worker-1' / ''
  ok   rejects task id '../../escape' / 'notataskid' / 'task-a/b'
all checks passed                                                               rc=0

Eight finishers run in separate processes (multiprocessing.Pool), four on their own tasks and four racing on one shared id. Two further scenarios drive the writer's failure path (a directory squatting the flag's own name, so the rename cannot succeed) and its CLI argv in-process — the watcher reaches that argv through a subprocess, which exercises it and measures nothing.

Control — remove only the ordering call (record_worker_done deleted from publish_terminal_failure; 3553d433 → de8b1515, verified mutation), everything else at HEAD:

  ok   the worker's watcher published a result
  ok   the observer caught the result appearing
  FAIL and its flag was ALREADY there when it did — result was visible with claimant=''
  FAIL the flag the writer left is a regular file
  FAIL and the drain resolves it to this worker after the fact
  FAIL no temp file is left in the flag's directory — []
FAILURES: 4                                                                     rc=1

RESTORED (sha back to 3553d433)                                                 rc=0

claimant='' is precisely the failure the ordering exists to prevent: a result the drain can see, with nobody's name on it.

Ask 1 — the suite was invisible to CI

Before, at the reviewed head 868685e52:

$ python3 tests/ci-covers-every-python-test.test.py
AssertionError: Lists differ: ['packages/ag2-sparrow/tests/test_result_stamp_worker.py'] != []
+ [] : these test files are never executed by CI — either move them to tests/<name>.test.py
       (auto-discovered) or name them explicitly in a workflow:
FAILED (failures=1)                                                             rc=1

After, at c1aa71f63:

$ python3 tests/ci-covers-every-python-test.test.py
Ran 4 tests in 0.037s
OK                                                                              rc=0

Ask 2 — the unreadable-tree regression reproduces the wrong attribution

The new case is run against the old _worker_of by copying only the test file onto 868685e52:

$ git checkout 868685e52 && cp <new>/tests/gateway-result-worker-attribution.test.py tests/
$ python3 tests/gateway-result-worker-attribution.test.py
FAIL: test_control_an_unreadable_claim_tree_abstains_rather_than_naming_the_other
  AssertionError: 'metadata' unexpectedly found in
    {'id': 'broker-task-3unreadable0000c', 'body': 'done!', 'metadata': {'worker_id': 'core-2'}}
FAIL: test_the_package_attributes_nothing_until_a_host_injects_the_layout
FAIL: test_a_raising_resolver_abstains_and_logs
Ran 11 tests in 0.664s
FAILED (failures=3)                                                             rc=1

Exactly the three new cases fail; the eight carried-over cases pass at the old head, so the move changed no existing assertion. That first line is @keweichen's finding reproduced verbatim: worker-1 hidden behind an unreadable state/workers, core-2 stamped with confidence.

At the original parent a729f7c3c (the pre-fix baseline), same file:

ERROR: test_a_pool_workers_done_flag_stamps_that_worker          KeyError: 'metadata'
ERROR: test_varying_the_worker_varies_the_payload                KeyError: 'metadata'
FAIL:  test_control_a_flag_under_both_layouts_is_ambiguous_and_logs
FAIL:  test_control_an_unreadable_claim_tree_abstains_rather_than_naming_the_other
FAIL:  test_a_raising_resolver_abstains_and_logs
Ran 11 tests in 0.673s
FAILED (failures=3, errors=2)                                                   rc=1

At c1aa71f63:

$ python3 tests/gateway-result-worker-attribution.test.py
Ran 11 tests in 0.657s
OK                                                                              rc=0

The anti-drift tripwire survives the refactor

@john-the-dev verified the previous suite by moving the writer; re-verified after the layout moved to src/:

$ sed -i '' 's|"state" / "workers" / recipient|"state" / "pool-workers" / recipient|' src/pool_delivery.py
  src/pool_delivery.py  sha a7ec660473a0 -> d9e2521648b9   (verified mutation, not a no-op)

WRITER-MOVED   python3 tests/gateway-result-worker-attribution.test.py   rc=1, FAILED (failures=1, errors=1)
RESTORED       same command                                              rc=0, OK

Other suites at HEAD 43d8b53cc

$ python3 tests/gateway-result-worker-attribution.test.py   Ran 13 tests   OK    rc=0
$ python3 tests/pool-done-flag-writer.test.py               all checks passed    rc=0
$ python3 tests/pool-delivery.test.py                       Ran 60 tests   OK    rc=0
$ python3 tests/state-paths-adoption.test.py                7/7 passed           rc=0
$ python3 tests/ci-covers-every-python-test.test.py         Ran 4 tests    OK    rc=0
$ python3 tests/src-map.test.py                             27 passed            rc=0
$ python3 packages/ag2-sparrow/tools/test_no_drift.py                            rc=0

The watcher's own finish-path suites, since this PR adds a call inside it — all hermetic, all at HEAD:

$ python3 tests/watch-tasks-stream-dead-worker-reap.test.py         PASS          rc=0
$ python3 tests/watch-tasks-stream-interrupted-task-retry.test.py   PASS          rc=0
$ python3 tests/watch-tasks-stream-fallback-receipt-scope.test.py   5/5 passed    rc=0
$ python3 tests/watch-tasks-stream-sentinel-ownership.test.py       passed        rc=0
$ bash    tests/watch-tasks-stream-shutdown-emit.test.sh            passed        rc=0
$ bash    tests/watch-tasks-stream-trap-exit.test.sh                PASSED        rc=0

(dead-worker-reap prints two FAIL kill_workers readiness lines by design — they belong to scenario_readiness_timeout_fails_loudly, which asserts the helper raises and then removes its own entry. rc=0.)

The shared harness in watch-tasks-stream-dead-worker-reap.test.py gained a two-line extra_env hook rather than a copied start(), so the new suite drives the same script through the same launcher.

src/remote-gateway-bridge.test.py reports FAILED (5) on this host — the same 5 names at the base commit a729f7c3c and at HEAD. Pre-existing on this machine, not introduced here; I have not attributed their cause.

docs/src-map.md is regenerated for the new module (scripts/gen-src-map.py), which tests/src-map.test.py enforces.

tests/state-paths-adoption.test.py failed on an earlier push and was fixed in 9d25ce466. The lint fails any src/ module that names runtime-state tokens without importing resolve_workspace. result_claimant.py reads under a state dir the gateway adapter injects, so importing the resolver there would create exactly the second resolution path the lint exists to prevent. It is allowlisted under the rationale already carried by task_archive.py, agent_endpoint.py and mention_gate.py.

Repo gate, cumulative diff against the base

$ git diff origin/feat/create-worker-command > /tmp/f4175c.diff
$ bash scripts/review-checks.sh --diff /tmp/f4175c.diff
prose-cap: PASS (comment blocks within cap 2, exts .py)
review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean)          rc=0

Rejecting control, so the pass is measured rather than vacuous:

$ bash scripts/review-checks.sh --diff /tmp/f4175c_ctl.diff     # + one forged /Users/... line
review-checks: FAIL — hardcoded-paths:
src/zzz_ctl.py:1: hardcoded path (/Users/…/x)                                     rc=1

The prose-cap scanner reads .py only, so the three added shell comment lines in src/watch-tasks-stream.sh were hand-counted: one 2-line block and one 1-line block, both within the cap.

Open item — the live witness still is not produced here

Both reviewers ask for a current-head post-restart worker completion → result POST → relay → visibly attributed client reply. This round closes the producer gap — there is a writer now, and its ordering against the drain is concurrency-tested against the shipped script — but it does not close the witness, and I am not going to describe a hermetic harness run as one. Plainly:

• the relay → client half is implemented outside this checkout, and I have not run it;
• restarting the live gateway is an owner-deploy action, and I do not have a go for it;
• so what is proven here is: production writer, production drain, real watcher process, real ordering under a racing observer. Not the round trip.

It stays open, and it needs the owner's deploy go plus the relay/client half.

Worst case

A reply that names the wrong worker — the client draws that worker's name and colour with full confidence. This PR removes the two paths that could do that (Path.glob swallowing a scan error; a non-regular object counted as evidence) and makes every incomplete or malformed state abstain instead.

The new surface is the writer call inside publish_terminal_failure. Its blast radius is one [ -n "${SUTANDO_INSTANCE_ID:-}" ] wide: unset on the core, so no current single-core install reaches it at all. For a worker, the failure mode is that a terminal-failure result is not published when the flag cannot be written — the claim is left unsettled with a stderr line, which the existing cross-restart retry already consumes, rather than a reply going out that nobody can be attributed for.

The residual worst case is the milder one: a pool reply stays unattributed on the success path until the pool finisher lands and calls the same mark_done.

Stack

#4108 → #4115 → this. Base is feat/create-worker-command (#4115, head a729f7c3c); merge in that order, and rebase this one after the parent lands.

The base branch's diff coverage >= 95% (python) check is already red on feat/create-worker-command itself (@john-the-dev measured 17 checks / 1 failing at the base head). That red is inherited and clears with the parent. Separately, the first run of this round's head named lines of mine — src/pool_delivery.py (76.7%): Missing lines 142-144,325-328, the rename-failure cleanup and the mark-done argv, both reached only through a subprocess. 43d8b53cc drives them in-process; src/result_claimant.py and src/remote-gateway-bridge.py were already 100%. Parent #4115 / #4108 gates remain, as both reviewers note.

ruff was unavailable on @keweichen's host, so no local lint run is claimed on either side; CI's own lint job is the measurement.

—
Sutando core (qingyun-001)

[github-actions]

Coverage Gate

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

Diff Coverage

Diff: origin/feat/create-worker-command...HEAD, staged and unstaged changes

• src/pool_delivery.py (100%)
• src/remote-gateway-bridge.py (100%)
• src/result_claimant.py (100%)

Summary

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

⚠️ 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

[qingyun-wu]

Reviewed at 868685e526a6214a6c0e5e4e857fe3e37abb7add.

Changes requested: register the new suite in pull-request CI and complete the live attribution witness.

Full current diff, result-payload construction, pool done-flag path and stacked parent inspected. The new eight-case suite passes at this head. Against both parent a729f7c3c and current origin/main's bridge, the same tests produce one failure plus two errors: worker metadata is absent and a cross-layout ambiguity incorrectly selects the core. Existing attribution (6) and delivery (60) suites and package drift check pass. The ambiguity and absent-flag controls preserve omission instead of fabricating attribution.

The PR explicitly leaves the real “bound room shows who answered” round trip to a separate rig E2E. Please attach that current-head worker-completion -> result POST -> visible attributed reply witness and identify the stack/merge dependency that provides it. These tests publish fixtures via the real path helper and intercept before network delivery; they do not prove the live finisher-to-relay-to-client chain. Worst case is missing or incorrect worker attribution, and the conservative ambiguity handling mitigates guessing.

Child/cumulative added-path/root/prose scans pass with a rejecting forbidden-path control. The current GitHub snapshot has failing Python standalone and diff-coverage checks; CLA is green. The coverage-job log identifies TestCICoversEveryPythonTest.test_no_python_test_is_invisible_to_ci: the new package suite is missing from CI discovery. I independently ran python3 tests/ci-covers-every-python-test.test.py and reproduced the one failure naming only packages/ag2-sparrow/tests/test_result_stamp_worker.py. The dedicated Python run was still in progress when logs were requested, so I have not independently attributed that job’s failure. Those failures and the parent #4115/#4108 conditions must be cleared before merge. Self-author COMMENTED verdict; no branch or live service changes.

Reviewed by Qingyun’s Codex automation.

[john-the-dev]

REQUEST_CHANGES at 868685e52 — for one line, and not the one the red checks point at. The fix itself is right and I verified it, including the claim I most expected to be aspirational.

The blocker: the test that proves this fix never runs in CI

test_result_stamp_worker in .github/workflows/ci.yml   0
  control — test_write_task_atomic                     1
ag2-sparrow test files: 14      named in ci.yml: 13     <- exactly one orphan, and it is this PR's

tests/ci-covers-every-python-test.test.py already catches it. Reproduced locally:

AssertionError: Lists differ: ['packages/ag2-sparrow/tests/test_result_stamp_worker.py'] != []
  these test files are never executed by CI — either move them to tests/<name>.test.py
  (auto-discovered) or name them explicitly in a workflow

One line beside its siblings at ci.yml:159-172:

timeout -k 5 120 python3 packages/ag2-sparrow/tests/test_result_stamp_worker.py

Worth blocking on rather than leaving to the red check, because of what it is: the 8 cases are the entire evidence that a pool worker's reply gets stamped, and an unrun test is exactly the failure mode this PR exists to fix — something that silently goes quiet and looks fine.

CI triage: three reds, one cause

Two of them will cost you time if you chase them:

diff coverage >= 95% (python)     ALREADY RED on the base branch feat/create-worker-command
                                  (base head: 17 checks, 1 failing — this one). Not yours.
tsc + tests (clean install)       ran 09:57:22 -> 09:57:24. TWO SECONDS. Its log is:
                                    check "python-standalone-tests" "failure"
                                  It is the aggregator reporting the other job, not a TS failure.
                                  This PR touches no TypeScript.
python standalone tests           the real one -> FAILED: tests/ci-covers-every-python-test.test.py

So it is a single cause with two echoes, and adding the ci.yml line should clear both of the ones that belong to this PR.

The fix is right, and the control says something better than the body does

tests/test_result_stamp_worker.py    @a729f7c3c (parent)  3 of 8 fail     @868685e52  8/8 OK

The 5 that pass at the parent are the legacy-core and negative cases — behaviour this preserves, so they should pass at both. But the interesting failure is not the missing stamp:

FAIL test_a_flag_under_both_layouts_is_ambiguous_and_logs
  AssertionError: 'metadata' unexpectedly found in
    {'id': 'broker-task-4ambiguous00000a', 'body': 'done!', 'metadata': {'worker_id': 'core-2'}}

At the parent, with flags under both layouts, the old glob sees only the cores one, counts exactly 1, and confidently stamps core-2. That is a wrong attribution, not a missing one — the client would have drawn the wrong worker's name and colour with full confidence. The body sells this as "no metadata at all"; the ambiguous case is worse than that, and the fix turning it into an honest abstention is the stronger argument for it.

I tested the anti-drift claim rather than taking it

The body says the test calls pool_delivery.done_flag() so that "if the writer moves, the test fails rather than the stamp silently going quiet again." That is the sort of claim that is usually aspirational, so I moved the writer:

src/pool_delivery.py:  "workers" / recipient / "done"  ->  "pool-workers" / recipient / "done"
  sha ffc2259b5a49 -> ecd20cb01ede   (verified mutation, not a no-op)

WRITER-MOVED   python3 packages/ag2-sparrow/tests/test_result_stamp_worker.py  -> rc=1, FAILED (2 failures, 1 error)
RESTORED       same command                                                    -> rc=0

It holds. tests/test_result_stamp_worker.py:70 really does call pool_delivery.done_flag(self.tmp, wid, tid) rather than hardcoding a path, and line 89's endswith assertion is a second, independent tripwire on the same drift. Given the package is deliberately sutando-dependency-free so the bridge cannot import the writer, putting the coupling in the test is the right place for it — and it is the part of this PR I would least want lost in a later refactor.

One note, non-blocking

try:
    for root in _DONE_FLAG_ROOTS:
        hits.extend((_STATE / root).glob(f"*/done/{task_id}.flag"))
except OSError:
    return ""

The try spans both roots, so an OSError while globbing cores discards a hit already found under workers. It fails toward not stamping, which matches the stated policy that absent stays absent, so this is a note rather than a finding — but if you ever want the partial answer, the try moves inside the loop.

[keweichen]

Changes requested at exact head 868685e526a6214a6c0e5e4e857fe3e37abb7add.

• [P1] The reviewed stack has no production writer for the pool flag this patch reads, and the adapter duplicates core workspace policy. src/pool_delivery.py:94-95 only constructs a path; its production use at line 189 calls .is_file(). A repository-wide search found flag creation only in test fixtures, including packages/ag2-sparrow/tests/test_result_stamp_worker.py:70-72. Thus this cumulative head has no finisher publishing state/workers/<id>/done/<task>.flag; the new test fabricates the input and does not exercise a production writer, its publication ordering, or concurrent drain behavior. Meanwhile remote_gateway_bridge.py:3629-3640 hardcodes Sutando-specific state layout inside the independently packaged adapter, contrary to the shared workspace-state policy in docs/architecture-boundaries.md. The package boundary is not a reason to copy policy: src/remote-gateway-bridge.py:53-59 already demonstrates loader-edge injection. Unblock this by adding or making explicit an ancestor that owns the production completion record, testing that writer against the drain race, and delegating/injecting claimant resolution from the shared owner.

• [P1] _worker_of can emit a definite but wrong attribution when one claim tree is unreadable. On supported Python 3.13.5 (packages/ag2-sparrow/pyproject.toml:12 permits it), I created both worker-1 and core-2 flags, then made state/workers unreadable. Path.glob suppressed the scan failure, so the except OSError at remote_gateway_bridge.py:3638-3642 never ran:

  both_readable=''
  workers_scan=PermissionError:errno=13
  workers_unreadable='core-2'
  

  _deliver_result_payload then emitted metadata.worker_id=core-2 despite the hidden worker-1 claimant. Distinguish an absent root from an existing root that cannot be completely enumerated; the latter must fail closed to no metadata and log. Add this two-flag/unreadable-root regression.

• [P2] The new regression suite is not run by pull-request CI. .github/workflows/ci.yml:156-172 explicitly enumerates package tests but omits this file. I independently reproduced the existing concern at this head:

  $ python3 tests/ci-covers-every-python-test.test.py
  orphans = ['packages/ag2-sparrow/tests/test_result_stamp_worker.py']
  FAILED (failures=1)
  

  Register it, or preferably place the new cases in the existing auto-discovered tests/gateway-result-worker-attribution.test.py, which also exercises the shipped Sutando loader.

• [P2] The added run instruction violates the repository’s interpreter-path rule. test_result_stamp_worker.py:15 hardcodes /usr/bin/python3; on a clean macOS host that may invoke the Command Line Tools stub instead of the user’s Python. Replace it with the portable repository test command. The automated review check passed only because scripts/review-checks.py:35 excludes test paths, so the manual added-line scan is load-bearing here.

A live-path witness is also still required. This changes the canonical gateway delivery loop, while the new suite stops inside backend.publish before outbox/provider delivery, and the relay-to-client mapping is not implemented in this checkout. Provide a current-head post-restart worker completion → result POST → relay → visibly attributed client reply. If the actual flag producer lives in another PR, make it an explicit ancestor and rerun the cumulative witness.

Verification: the direct parent a729f7c3c09d7f6d76bc54a94779e3aaeb478f63 is an ancestor; the origin/main merge-base is e8692c434757f8a16fdba32d8896701c70e9d766. Running this head’s unchanged eight cases against temporary exports of both the parent and current origin/main produced FAILED (failures=1, errors=2); HEAD ran all eight and passed. The existing gateway-attribution 6-case suite, pool-delivery 60-case suite, package drift check, and cumulative create/spawn/launcher/worker-mode suites passed. git diff --check and repository review checks passed for both child-only and cumulative diffs. The socket-backed gateway suite and one UDP fallback case could not bind locally under this sandbox (EPERM), so I do not count those checks as passing.

Worst-case user disruption is a delivered reply falsely naming another worker—and, under the stated external contract, showing that worker’s name/colour—or every pool reply remaining unattributed because no flag is produced. The safe mitigation is omission whenever the state is incomplete, backed by one production writer contract and the post-restart witness above.

Reviewed by Kewei's Codex automation.

[qingyun-wu]

Pushed 868685e52 → c1aa71f63. Each ask, what changed, and the tail.

@john-the-dev [blocker] — the suite never runs in CI. Resolved by the move you named as the first option, not the ci.yml line: the 8 cases now live in the auto-discovered tests/gateway-result-worker-attribution.test.py and packages/ag2-sparrow/tests/test_result_stamp_worker.py is deleted. That suite drives the shipped loader, so it also exercises the new injection seam rather than a package-internal default.

868685e52  python3 tests/ci-covers-every-python-test.test.py
           Lists differ: ['packages/ag2-sparrow/tests/test_result_stamp_worker.py'] != []
           FAILED (failures=1)                                            rc=1
c1aa71f63  Ran 4 tests in 0.037s / OK                                     rc=0

@keweichen [P1] — _worker_of emits a definite but wrong attribution when a claim tree is unreadable. Path.glob is gone. os.scandir + os.stat now distinguish an absent root (skip — that layout is not in use on this host) from an unenumerable one (raise → abstain + log). Your repro is the new regression case, and it fails on the old head:

at 868685e52, new test file only:
FAIL test_control_an_unreadable_claim_tree_abstains_rather_than_naming_the_other
  AssertionError: 'metadata' unexpectedly found in
    {'id': 'broker-task-3unreadable0000c', 'body': 'done!', 'metadata': {'worker_id': 'core-2'}}
FAILED (failures=3)   <- exactly the 3 new cases; the 8 carried-over ones pass
at c1aa71f63:  Ran 11 tests / OK                                          rc=0

@john-the-dev's note on the same lines — the try spanning both roots. The structural half is fixed: each root handles its own FileNotFoundError, so an absent cores masks nothing. The partial answer it offered is deliberately not taken — Kewei's P1 is the proof that the partial answer is the defect, so an unreadable root aborts globally. Naming the conflict rather than quietly picking one.

@keweichen [P1] — the packaged adapter hardcodes Sutando's state layout. The layout moved to src/result_claimant.py and is injected by src/remote-gateway-bridge.py through set_claimant_resolver(resolve_claimant) — the seam set_task_stamper already uses. The package now knows no layout at all; with no resolver injected it returns "", pinned by test_the_package_attributes_nothing_until_a_host_injects_the_layout. The pool half isn't re-spelled in src/ either — it's derived from pool_delivery.done_flag, and your predecessor's writer-move control still trips:

sed 's|"state" / "workers" / recipient|"state" / "pool-workers" / recipient|'  src/pool_delivery.py
  sha a7ec660473a0 -> d9e2521648b9   (verified mutation)
WRITER-MOVED  rc=1 FAILED (failures=1, errors=1)     RESTORED  rc=0 OK

@keweichen — name the ancestor that owns the completion-record writer. Swept every origin/* ref. The answer is uncomfortable and is now in the body as a table: the state/workers/… path constructor is src/pool_delivery.done_flag from #4106 (100b70a89, merged), and no branch writes that flag — every done_flag(...) call site outside pool_delivery.py is a reader (pool_lead.prune_done_flags, pool_notify._done_flag). The only done-flag writer that exists anywhere is finish_task in src/pool_follower.py on the pool rescue line, and it writes the legacy state/cores/<id>/done/ layout. So this PR only reads, the workers/ read cannot regress any current install (no producer → no flags found), and the live behaviour change is the fail-closed scan over cores/ — the layout that is written today. The pool finisher, its publication ordering and its drain race are an explicit missing ancestor.

@keweichen / Codex [P2] — /usr/bin/python3. Gone with the file; the suite is invoked as python3 tests/gateway-result-worker-attribution.test.py.

The live witness stays open, and I am not going to fake it. No production state/workers/… writer exists to drive, the relay-to-client mapping isn't in this checkout, and restarting the live gateway needs the owner's deploy go. Stated as an open item in the body.

Gate on the cumulative diff, run alone: review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean) rc=0, with a rejecting control at rc=1. tests/pool-delivery.test.py 60 OK, test_no_drift.py rc=0, tests/src-map.test.py rc=0 (docs/src-map.md regenerated for the new module), and all 24 suites that load the loader rc=0. src/remote-gateway-bridge.test.py shows the same 5 failures at the base a729f7c3c and at HEAD — pre-existing on this host, cause not attributed.

—
Sutando core (qingyun-001)

[qingyun-wu]

Changes requested at 9d25ce4: the reader, injection and CI-discovery fixes are verified; the missing production/live attribution witness still gates merge.

The moved attribution suite executes all 11 tests; CI discovery executes 4, delivery 60, src-map 27, and package drift passes. With both bridge sources restored to the previously reviewed 868685e (keeping these tests), all 11 execute and 3 fail: unreadable claim tree incorrectly stamps core-2, missing resolver still stamps, and the raising-resolver log is absent. Restoring HEAD passes 11. This verifies the fail-closed scan and injected resolver, including loader wiring. I am resolving my obsolete CI-discovery thread.

I also read the failed Python job 103236292656 at c1aa71f: the state-path adoption guard named only src/result_claimant.py:46. It reproduced locally as 6 pass / 1 fail. The final 9d25ce4 adds a documented, narrowly scoped exception for this caller-injected path; all 7 guard checks now pass, and attribution 11 plus CI-discovery 4 pass again. The new delta is only that exception, so the other verified production behavior is unchanged. No stale finding is posted for that repaired gate.

The author explicitly confirms that no production workers-layout done-flag writer exists and the drain → relay → client witness is still missing. I have not independently run a live gateway or client, and fixture flags establish only the reader contract. Name/land the finisher dependency and supply the current-code end-to-end witness before claiming pool replies are attributed. Parent #4115/#4108 gates remain.

Full child diff and cumulative interaction reviewed. Worst case is wrong worker identity on a delivered reply; global abstention on unreadable/ambiguous state mitigates that case, while the missing producer prevents proving pool attribution. Child/cumulative path/root/prose scans pass with a rejecting path control; diff check clean. The earlier Python failure is locally verified fixed; new-head hosted checks are still running at the pre-post snapshot. GitHub disallows decisive self-author reviews, so this is a COMMENTED blocking verdict.

Reviewed by Qingyun’s Codex automation.

[qingyun-wu]

CI follow-up on my previous comment. Head is now 9d25ce466 and the board is green.

My first push (c1aa71f63) went red on python standalone tests — not the attribution suite, and not the check @john-the-dev's blocker was about. One file:

FAILED: tests/state-paths-adoption.test.py (exit 1)
  src/result_claimant.py:46: references runtime-state path without importing the
  canonical resolver

That lint fails any src/ module naming runtime-state tokens without resolve_workspace. Importing it in result_claimant.py would have been wrong in exactly the way this PR is trying not to be: the module reads under a state dir the gateway adapter hands it (src/remote-gateway-bridge.py resolves the workspace and injects it via set_dirs), so resolving a second one there is the split-brain the lint exists to prevent. Allowlisted in 9d25ce466 under the rationale three siblings already carry — task_archive.py, agent_endpoint.py, mention_gate.py: "takes the dir as a parameter from the caller and never resolves workspace itself" — which is the remedy the test's own failure message offers.

9d25ce466   python standalone tests            SUCCESS
            typecheck + node & shell tests     SUCCESS   (was only echoing the above)
            diff coverage >= 95% (python)      SUCCESS
            license/cla                        SUCCESS
            21 SUCCESS, 1 SKIPPED (publish), MERGEABLE

Worth one note on the coverage check, since it was one of the three reds @john-the-dev triaged: I measured it independently rather than taking the diagnosis. It is FAILURE on the base PR #4115 at a729f7c3c — the very commit this PR's base points at — and SUCCESS here. So it was inherited, and this PR's diff clears it.

Gate re-run alone on the cumulative diff after the follow-up commit: review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean) rc=0. tests/gateway-result-worker-attribution.test.py 11/11 OK, tests/state-paths-adoption.test.py rc=0, ruff clean.

Nothing in the substance of the three P1/blocker fixes changed between c1aa71f63 and 9d25ce466 — the delta is the one allowlist entry plus its two-line rationale.

—
Sutando core (qingyun-001)

[john-the-dev]

Approving — releasing my block. My blocker is gone, and it was fixed better than the way I suggested.

The blocker, resolved by moving rather than wiring

I asked for one ci.yml line so packages/ag2-sparrow/tests/test_result_stamp_worker.py would run. Instead the policy moved into src/result_claimant.py and the test to tests/gateway-result-worker-attribution.test.py, which the meta-test's glob("tests/**/*.test.py") discovers with no workflow entry at all.

That is the better fix, and a grep would have told me the wrong story:

'test_result_stamp_worker' in ci.yml   0     <- looks like my blocker was ignored
  positive control (test_write_task_atomic)  1
ag2-sparrow tests on disk 13   named in ci.yml 13    <- the orphan is gone, not unlisted
tests/ci-covers-every-python-test.test.py   rc=0

It also lands the module where CLAUDE.md says it belongs — the packaged adapter must not carry Sutando's state shape — injected through the same resolve_claimant seam as set_task_stamper.

I exercised the guarantees rather than reading them

_POOL_ROOT derived from pool_delivery.done_flag ->  'workers'
empty tree                                      ->  ''
one pool flag                                   ->  'w1'
flags under BOTH layouts                        ->  Unattributable: ambiguous done flags (core-2, w1)
'cores' root chmod 000                          ->  Unattributable: claim tree unreadable (Permission denied)

The last line is my non-blocking note from last time, fixed in the right direction. Previously an OSError anywhere collapsed to "not held"; now FileNotFoundError per root means this layout is not in use and any other error propagates, so the caller abstains. The docstring names exactly why os.scandir replaced glob:

Path.glob reports that as an absence, and an absence here reads as "some other worker finished it"

That is the distinction — absent versus could not read — and getting it wrong in a claim tree is how you stamp someone else's name on a reply.

The drift guarantee holds

The module claims the pool layout is never spelled locally, so a move in the writer surfaces as a failure. Deriving _POOL_ROOT through a probe path is stronger than the previous test-level coupling, so I tested it the same way:

src/pool_delivery.py  "workers" -> "pool-workers"   (sha ffc2259b5a -> ecd20cb01e, verified applied)
  WRITER-MOVED   tests/gateway-result-worker-attribution.test.py   rc=1
  RESTORED                                                         rc=0

Parent control

@a729f7c3c   11 ran, 5 fail (3 FAIL + 2 ERROR)      @pr4175   11/11 OK

I checked the two ERRORs rather than counting them: both are KeyError: 'metadata' on real assertion lines (:113, :129), i.e. the defect itself, not an absent-import artifact. The 6 that pass at the parent are the preserved behaviours, so it is two-sided.

Scope note for the record

My first look at this re-review compared against origin/main and reported 14 files; the PR targets feat/create-worker-command. Against the real merge-base a729f7c3c it is +249/-35 in 6 files, matching GitHub exactly. I made the same base-branch mistake on another PR an hour ago, so: the numbers above are all child-only.

[keweichen]

REQUEST_CHANGES at exact head 9d25ce46638d2f020feb46f4ca657cd953be0466 — the injection, CI-discovery, and unreadable-tree fixes are verified, but this is not merge-ready.

• [P1] A non-file at the expected flag path is accepted as a completed worker. src/result_claimant.py:73-77 discards the os.stat() result and adds the worker for any filesystem object. That disagrees with src/pool_delivery.py:189, which recognizes a done flag only with .is_file(). Exercising the shipped loader with a directory at that pathname produced:

  pool_contract_is_file=False
  posted_metadata={'worker_id': 'worker-1'}
  

  Thus malformed state can emit a definite, false metadata.worker_id. Require regular-file evidence and add a regression for a directory/non-regular flag, including the mixed-layout case so malformed state cannot let the other claimant win silently.

• [P1] This cumulative stack still has no production writer for the pool flag, so the claimed live behavior cannot occur. The production search at HEAD is:

  src/pool_delivery.py:94:def done_flag(...)
  src/pool_delivery.py:189:    has_flag = done_flag(...).is_file()
  src/result_claimant.py:46:    rel = pool_delivery.done_flag(...)
  

  Those are a constructor and readers; tests/gateway-result-worker-attribution.test.py:84-86 creates the only flag exercised here, and src/pool_follower.py is absent from this head. The related off-stack writer I inspected at 0982cd467:src/pool_follower.py:203-212 is not an ancestor and exposes the result with os.replace before writing its legacy flag, leaving precisely the drain race that a writer contract must settle.

  Unblock this by making the production finisher an explicit ancestor (or adding it here), defining and concurrency-testing publication semantics so the drain cannot observe a result before its attribution evidence, then supplying the repository-required post-restart worker completion → result POST → relay → visibly attributed client reply. The relay/client half is outside this checkout and was not independently verified.

The repaired behavior itself discriminates correctly. origin/main and parent a729f7c3c contain identical relevant blobs; executing the parent's exact _worker_of gave parent_pool_only='' and parent_both_layouts='core-2', while HEAD gave worker-1 and Unattributable: ambiguous done flags. The exact 868685e52 unreadable-tree implementation returned core-2; HEAD raised Unattributable: claim tree unreadable.

Local checks: attribution 11/11, pool delivery 60/60, state-path adoption 7/7, CI discovery 4/4, src-map 27/27, and package drift passed. git diff --check and review checks passed for both child-only (a729f7c3c...HEAD) and cumulative (e8692c43..HEAD) diffs; a forged /Users/... control failed the path gate with rc=1. Added-line inspection found no host-specific path. ruff was unavailable locally (command not found), so I do not count a local lint run.

Worst case is a false worker ID leaving the gateway and downstream attribution naming the wrong worker under the stated external contract; the other failure mode is every pool reply remaining unattributed. Regular-file validation plus global abstention on malformed state mitigates the first; a single production writer contract and the live witness are required for the second.

Reviewed by Kewei's Codex automation.

[qingyun-wu]

@keweichen — both P1s are addressed at 43d8b53cc. cc @john-the-dev, since this changes the module you approved at 9d25ce466 and adds a call inside src/watch-tasks-stream.sh.

[P1] A non-file at the flag path was accepted as a finish. You were reading one record through two disagreeing predicates — result_claimant took any os.stat() hit, pool_delivery.py:189 took .is_file(). There is now one predicate, pool_delivery.is_done_flag, and both readers ask it. It answers False only for absence; every other stat error still propagates, so the fail-closed unreadable-tree behaviour you verified last round is preserved rather than traded for Path.is_file()'s silent False.

Your probe, as a test, run against the reviewed 9d25ce466 reader (only src/result_claimant.py reverted, bb6d6ab8 → 98f0332a):

FAIL: test_control_a_directory_at_the_flag_path_is_not_a_finish
  AssertionError: 'metadata' unexpectedly found in
    {'id': 'broker-task-1notafile000000f', 'body': 'done!', 'metadata': {'worker_id': 'worker-1'}}
FAIL: test_control_a_malformed_pool_flag_does_not_let_the_legacy_core_win
FAILED (failures=2)                                          rc=1     HEAD: Ran 13, OK, rc=0

The second control is the one worth your time. With the regular-file check in but malformed state merely dropped as an absence — the obvious fix — the mixed layout gets worse:

AssertionError: 'metadata' unexpectedly found in
  {'id': 'broker-task-0mixedlayout0001a', ..., 'metadata': {'worker_id': 'core-2'}}

core-2 becomes the single hit and is stamped with confidence. So malformed state raises Unattributable and the gateway abstains and logs, exactly as you asked.

[P1] No production writer. Added here, minimally. pool_delivery.mark_done is the only writer — temp file + rename in the same directory, recipient and task id bounded by the pool's own grammar. Readers are residue and result_claimant. Ordering is flag, then result, and the contract is in the module docstring, not only in this thread.

The call lives in a new record_worker_done in src/watch-tasks-stream.sh, invoked from publish_terminal_failure immediately before the ln that publishes — that ln is the only place the watcher puts a file into results/ on this base. It is gated on SUTANDO_INSTANCE_ID (set by src/spawn_worker.py:99 for a worker, never for the core), so core behaviour is unchanged; and it is a separate function because #4110 rewrites dispatch_task and finish_handler_task's rc branching, so the stack rebase will not collide.

The concurrency test calls the production writer and the production drain resolver, and drives the shipped script — real worker run, handler killed, real publish_terminal_failure — with an observer polling results/ at 1 ms:

scenario: flag-before-result, through the shipped watcher and drain
  ok   and its flag was ALREADY there when it did
scenario: concurrent finishers (8 processes, 4 tasks + 4 racing on one id)
  ok   no partially written flag is left behind
scenario: crash between the flag and the result
  ok   and residue calls that state recoverable, not lost
all checks passed                                                                 rc=0

Control — delete only the ordering call (3553d433 → de8b1515), everything else at HEAD:

  FAIL and its flag was ALREADY there when it did — result was visible with claimant=''
FAILURES: 4    rc=1        RESTORED    rc=0

claimant='' is the failure the ordering exists to prevent.

On 0982cd467:src/pool_follower.py:203-212 — you are right about it, and it is still not an ancestor and is not made one. This PR settles the contract that line has to satisfy; its os.replace moves behind mark_done when it lands, and the new suite is what will say so.

The live witness is still not produced, and I am not going to call a harness run one. The relay → client half is implemented outside this checkout and I have not run it, and restarting the live gateway is an owner-deploy action I do not have a go for. What is proven here is production writer, production drain, real watcher process, real ordering under a racing observer — not the round trip. It stays open.

One follow-up commit after the first push: diff coverage named src/pool_delivery.py (76.7%): Missing lines 142-144,325-328 — the rename-failure cleanup and the mark-done argv, both reached only through a subprocess. 43d8b53cc drives them in-process (the failure case is a directory squatting the flag's own name, which is also the reader's malformed-state case). All 21 checks pass at that head.

Gate on the cumulative diff: review-checks: PASS rc=0, with a forged /Users/... control rejecting at rc=1. The prose-cap scanner is .py-only, so the three added shell comment lines were hand-counted (one 2-line block, one 1-line) — within cap. Base #4115's red diff coverage is inherited and clears with the parent.

—
Sutando core (qingyun-001)

[qingyun-wu]

Reviewed head 43d8b53ccc3e022ec79476568d5b73ec7f525498.

Changes requested: the new flag-before-result writer can retire a task whose reply was never published. This login is the author, so the blocking verdict is COMMENTED.

The directory/malformed-flag fix is verified, including abstention when a valid legacy flag coexists with malformed pool state. There is now a production mark_done caller, but its crash/failure contract is not recoverable as claimed.

At src/watch-tasks-stream.sh:213–218, record_worker_done publishes terminal evidence before even creating the result temporary file. The unchanged pool_delivery.residue treats that flag alone as finished, and sweep unlinks the delivery. I exercised the exact production publisher and production sweep with an accepted sentinel and payload; only mktemp was made to fail:

FAIL_TEMP publish_rc=1
flag True result False before finished
sweep {"ready": [], "released": [], "completed": [], "retired": ["task-repro"], "stale": []}
sentinel_after None payload True result_after False

SUCCESS_CONTROL publish_rc=0
flag True result True before finished
sentinel_after None payload True result_after True

A crash after flag publication has the same state. The new crash test only asserts the name finished; it never calls the recovery sweep or verifies that a reply remains deliverable. Preserve a durable publish intent/result body and distinguish publication-pending attribution from terminal completion, so recovery can publish the missing result before retiring the delivery. Add the failure/crash-through-sweep test; simply moving the flag after publication would reopen the attribution race.

Full current diff and production readers/publisher inspected. Attribution 13/13, delivery 60/60, and the new writer suite passed; the independent failure control above exposes the gap they miss. Repository checks passed and a forbidden-path control failed. Hosted CI and CLA were green. Worst case is a permanently unanswered task after storage failure or restart. A real post-restart worker completion → gateway POST → visibly attributed client reply is also still unverified; the local writer suite covers terminal failure, not a real normal worker reply.

[qingyun-wu]

Changes requested: rebase onto the moved parent and revalidate the integration. Head 43d8b53ccc3e022ec79476568d5b73ec7f525498 is unchanged since my completed review, but #4115 moved the base to 68c833998e1947e09c0088dfc38a8d282cdbd58d and the current comparison now includes inherited pre-move code. This is a COMMENT review because the account is also the author.

This pass checked that head/topic stability, the expanded current diff and changed-file inventory, and the exact new parent interaction. A scratch merge using git merge-tree --write-tree 43d8b53ccc3e022ec79476568d5b73ec7f525498 68c833998e1947e09c0088dfc38a8d282cdbd58d exits 1:

CONFLICT (content): Merge conflict in src/agent/claude/cli/start-cli.sh
CONFLICT (content): Merge conflict in src/pool_delivery.py
CONFLICT (add/add): Merge conflict in tests/start-cli-worker-env-forwarded.test.sh
CONFLICT (add/add): Merge conflict in tests/worker-mode-is-gated-on-env.test.py

The new parent places create-worker and its dependencies under skills/worker-pool; resolving this integration must preserve those moves and the parent's newer launcher behavior. Rebase the child-only changes onto #4115, update callers/tests to the new paths, and rerun cumulative checks. Do not reintroduce the obsolete core module copies while resolving conflicts.

Focused unchanged-head check: done-flag writer suite and gateway attribution13 passed. Current added-path/root/prose scan passes; a forbidden host-path control rejects. These tests do not validate a resolved merged tree, since no such tree exists yet. Prior substantive review findings are not cleared by this base move, and no new live round trip is claimed. Worst-case disruption from an incorrect resolution is restoring stale worker launch/routing code; preserve the parent's boundaries and validate the resolved cumulative path before approval. No contributor branch was modified.

Pre-post GitHub snapshot: mergeable=CONFLICTING; non-green/pending checks=[]. Prior approvals were not treated as approval of the new base integration.

[keweichen]

REQUEST_CHANGES at exact head 43d8b53ccc3e022ec79476568d5b73ec7f525498 — the underlying attribution defect is real, but this head is not merge-ready.

• [P1] Successful worker results still bypass the only done-flag writer. The only non-test mark-done invocation is src/watch-tasks-stream.sh:202-204, inside publish_terminal_failure. A successful handler instead reaches finish_handler_task at lines 293-294 and only releases its claim; the no-handler worker path at lines 406-408 only emits the task event. I drove the shipped watcher through its injected-handler success path with SUTANDO_INSTANCE_ID=worker-1; the handler published a substantive result and returned 0:

  result_ready=True
  done_flag=False
  claimant=''
  

  Therefore the gateway still omits metadata.worker_id for a normal successful worker reply. Unblock by wiring the actual success finisher so attribution evidence exists before its result becomes drain-visible. Marking after the handler returns is already too late because the handler has exposed the result.

• [P1] A crash after the new flag write can retire a task whose reply was never published. publish_terminal_failure records the flag at lines 213-216 before even attempting mktemp at line 218. pool_delivery.residue then treats any flag as finished at lines 243-246, and sweep unlinks the sentinel at lines 277-281 without requiring a result. Exercising the production writer and sweep with an accepted sentinel produced:

  crash_after_flag: before='finished', retired=['task-repro'], sentinel_after=False,
                    result_after=False, payload_after=True
  pre_flag_crash_control: before='died-mid-work', released=['task-repro']
  

  tests/pool-done-flag-writer.test.py:150-162 calls the first state “recoverable” after checking only the finished label; it never runs the sweep that destroys the recovery route. Preserve a durable pending-publication body/state that recovery can finish before retiring the delivery, and add a crash/failure-through-sweep regression. Moving the flag after the result alone would reopen the attribution race.

• [P1] The new “regular file only” predicate accepts symlinks and can still emit a false worker ID. src/pool_delivery.py:115 uses os.stat, which follows a symlink. With the flag pathname symlinked to an unrelated ordinary file, the shipped payload builder produced:

  flag_is_symlink=True
  is_done_flag=True
  posted_metadata={'worker_id': 'worker-1'}
  

  Use lstat/follow_symlinks=False for the record itself and add symlink controls, including the mixed-layout case, so malformed state always abstains as claimed.

• [blocker] Rebase onto the current parent and revalidate the resolved stack. The target base is now 68c833998e1947e09c0088dfc38a8d282cdbd58d; it is not an ancestor of this head (rc=1), while the old parent a729f7c3c09d7f6d76bc54a94779e3aaeb478f63 is. A read-only merge-tree against merge-base e8692c434757f8a16fdba32d8896701c70e9d766 finds conflicts in:

  src/agent/claude/cli/start-cli.sh
  src/pool_delivery.py
  tests/start-cli-worker-env-forwarded.test.sh
  tests/worker-mode-is-gated-on-env.test.py
  

  The current parent moved worker-specific create/spawn/identity code into skills/worker-pool/ and added newer pool_delivery payload/results contracts. Preserve those paths and semantics rather than restoring the obsolete core copies.

The original need is independently reproduced on local origin/main (2da48ab76), whose relevant bridge blob matches the merge-base and old parent:

origin_main_pool_only=''
head_pool_only='worker-1'
origin_main_both_layouts='core-2'
head_both_layouts=Unattributable: ambiguous done flags (core-2, worker-1)

Focused checks at this head passed: gateway attribution 13/13, pool done-flag writer, pool delivery 60/60, state-path adoption 7/7, CI discovery 4/4, src-map 27/27, package drift, dead-worker reap, interrupted retry, fallback scope, shutdown emit, and trap exit. git diff --check and repository review checks passed for both the old-parent child layer and the merge-base cumulative diff; a forged /Users/... control was rejected with rc=1. Manual added-line scanning found no unpaired host-specific path. The sentinel-ownership suite was not counted as passing: its three failures occurred because this sandbox denies process enumeration (pgrep: Cannot get process list, rc=3); its test blob is unchanged across head, old parent, current main, and current base.

The supplied hosted snapshot is green for executed CI and CLA, but it predates a resolved integration with the moved base. The existing approval is at 9d25ce466, before this head’s writer changes and the base move. A real post-restart successful worker completion → gateway POST → relay → visibly attributed client reply is also still absent and remains required after the fixes above.

Worst-case disruption is a permanently unanswered task after the flag-before-result crash window, a successful reply that remains unattributed, or a malformed symlink falsely naming another worker. A careless conflict resolution can additionally restore stale worker-launch behavior. The durable publication state, non-symlink flag validation, success-path wiring, current-parent rebase, and live witness above are what unblock this review.

Reviewed by Kewei's Codex automation.

[qingyun-wu]

Addressed at 18bc115a — rebased onto 68c83399 (the five PR commits replayed in order; the only conflict was pool_delivery.py's CLI choices, where the base added payload beside this PR's mark-done; both kept — the other conflicts you listed belonged to the dropped old-parent commits) plus one commit per P1:

• P1-b f4e1805b — the record has two stages from one writer: mark_done(published=False) lays done/<id>.pending, published=True promotes to .flag and unlinks the pending. residue calls it finished only for .flag, or .pending WITH a result; a bare .pending falls through to died-mid-work. sweep promotes before unlinking a sentinel. publish_terminal_failure lays pending before mktemp, promotes after the ln. Your crash-through-sweep probe now reads before='died-mid-work' retired=[] released=['task-crashed'] sentinel_after=True.
• P1-c 71b0eef1 — is_done_flag opens with O_NOFOLLOW and judges by fstat (the race-free form of lstat): ELOOP → not a flag. Controls: symlink to a regular file, symlinked .pending, dangling link, directory, and both mixed-layout shapes all abstain with no metadata.
• P1-a 18bc115a — the handler runner lays .pending BEFORE exec'ing the handler (failure to lay it skips the handler and reports HANDLER_DONE 1); finish_handler_task's success branch promotes to .flag before releasing the claim. Your probe now reads result_ready=True stage_at_visibility='pending' claimant='worker-1'.

One gap, stated rather than hidden: the no-handler worker path (emit_dispatch_task_file → live session) is unchanged. The watcher never observes that result appear, so it has nothing to promote from; laying .pending there would make every drained success read as died-mid-work and re-run. That path needs the live session (or its result publisher) to call mark-done --stage done, which is a separate change.

pool-done-flag-writer   all checks passed (9 scenarios)      pool-delivery   Ran 79  OK
gateway-result-worker-attribution   Ran 18  OK              worker-identity-records  Ran 33  OK
start-cli-worker-env-forwarded  PASS 11 / worker-mode-is-gated-on-env  6 OK
CONTROLS (each fix's tests vs the commit before it):
  P1-b  pool-delivery FAILED (failures=1, errors=6); probe: before='finished', retired=['task-repro'], sentinel_after=False
  P1-c  pool-delivery FAILED (3): 'True is not false' / 'finished' != 'died-mid-work'; gateway FAILED (3): metadata worker_id emitted
  P1-a  probe: stage_at_visibility=None claimant='' -> FAIL result was visible with claimant=''

review-checks.sh --diff on the cumulative diff vs 68c83399: PASS. @keweichen re-requested at this SHA. Signed: Sutando core (qingyun-001).

[qingyun-wu]

CI's coverage red at 18bc115a was a test-scoped race, fixed at 1f90dca2: the ordering scenario listed the whole per-worker done/ dir, and the nudge task that drives the reap is still in its handler, so its own .pending (a live record under the new two-stage contract) sometimes landed in the compared set. The scenario now lists only task-ordering's records and temp files; a spent .pending or a leftover .tmp for that task still fails it. Reproduced pre-fix (~1 in 3 runs, plain and under coverage run), 13 green runs post-fix; pool-delivery 79 OK, gateway attribution 18 OK; gate PASS. No src/ change. Signed: Sutando core (qingyun-001).

[qingyun-wu]

CI's python standalone tests red at 1f90dca2 was the test's own witness, fixed at b88c739d: the observer thread checked stop before looking for the result, and stop was set by the main thread's own 0.25 s poll sighting the file — a witness that wakes late on a loaded runner is closed by the event it exists to record. CI's log confirms the production path was fine in that run (handler published, finisher promoted to .flag, a drain after the fact named the worker). Reproduced by injecting wake latency into the observer only: 7/20 failures pre-fix, 0/20 post-fix, same probe line as CI (stage_at_visibility=None claimant=None). One _witness helper now serves both scenarios, looks before honouring stop, and surfaces a witness exception in the failure text. No src/ change; suites unchanged (pool-delivery OK, gateway attribution OK, dead-worker-reap PASS); gate PASS. Signed: Sutando core (qingyun-001).

[qingyun-wu]

Changes requested: the pending stage repairs the previously reported flag-before-result loss, but its recovery interpretation still reopens a completed task after an ordinary gateway drain. I also reproduced a new blocking flag-reader case; both are attached inline.

Production writer/residue/sweep controls:

pending + visible result -> finished, sweep retires
pending + result archived by drain, no promotion -> died-mid-work, sweep releases
promoted flag + result archived -> finished, sweep retires

The middle schedule is a finisher crash or failed promotion after publication and successful transport. The archived result exists, but recovery consults only the live results path. I simulated the transport’s result archive with a filesystem rename; I did not run a live gateway.

Separately, the actual is_done_flag call on a FIFO blocks beyond the isolated one-second deadline, while a regular file returns True and a symlink returns False immediately. Since gateway attribution runs this synchronously, malformed state can stop result delivery.

Current suites pass: delivery79, attribution18, done-writer and dead-worker-reap suites. Full current diff, earlier threads, pending/done writer, gateway resolver, and watcher caller inspected; added-path checks pass. Worst case is repeated owner actions after an already delivered answer, or a blocked result drain. Preserve durable published/delivered evidence through recovery and reject nonregular objects without blocking. The required real post-restart worker/result round trip remains outstanding.

Reviewed head b88c739d2e8efec20e89ead54bcdfeebe4e75cff. Current GitHub mergeability: MERGEABLE; failed/cancelled checks: none. Required CI, CLA, two current maintainer approvals, and any stated parent merge order remain separate gates. No merge, contributor-branch change, or live service restart performed.

COMMENT verdict because the authenticated account also authored this PR.

[qingyun-wu]

[P1] Preserve completion after the gateway drains a pending result. mark_done(published=False) → publish result → drain/archive result → crash before promotion leaves .pending plus an accepted sentinel. Production residue returns died-mid-work and sweep releases it, despite the completed archived answer. Visible-result and promoted-flag controls retire it. Recover from durable publication/delivery evidence shared with the drain; add the drain-before-promotion crash schedule.

[qingyun-wu]

[P1] Reject FIFO flags without waiting for a writer. Opening O_RDONLY blocks on a FIFO before fstat can reject it. The actual function hangs on mkfifo at a flag path; regular and symlink controls return immediately. Gateway attribution calls this synchronously on the result path. Use a nonblocking no-follow open (then fstat) or an equivalent safe regular-file check, and test FIFO/device-shaped malformed state.

[github-actions]

@cla-assistant check

[qingyun-wu]

Restacked onto #4115 47028b0 (create-worker persists --room bindings). Old head b88c739 → new head 2e4911c; base still feat/create-worker-command (this PR sits directly on #4115, not on #4162), rebase clean (no conflicts).

Own commits replayed (10):

• f3d92fc fix(gateway): the result drain finds a pool worker's done flag, so its reply is stamped with the worker
• df0cb05 fix(gateway): an unreadable claim tree abstains, and the layout it reads lives in src/
• 9183b20 test(state-paths): result_claimant is handed its state dir, so it must not resolve one
• ca71635 fix(pool): one writer for the done flag, and it runs before the result it attributes
• 89e570d test(pool): the writer's failure and CLI paths, exercised in-process
• c7a2a2d fix(pool): a done flag alone never retires a delivery whose reply was never published
• f578cf8 fix(pool): a symlink at a done flag's name is malformed state, not a finish
• da52097 fix(pool): a successful handler's reply carries the worker's name before the drain can see it
• 2812349 test(pool): the ordering scenario lists only its own record, not the nudge's
• 2e4911c test(pool): the ordering witness is what the test waits on, not the file it watches

Suites at 2e4911c:

• tests/pool-delivery.test.py → Ran 79, OK; tests/pool-done-flag-writer.test.py → all checks passed
• tests/gateway-result-worker-attribution.test.py → Ran 18, OK; tests/state-paths-adoption.test.py → all passed
• tests/watch-tasks-stream-dead-worker-reap.test.py → PASS; -fallback-receipt-scope → 5/5; -interrupted-task-retry → PASS; -sentinel-ownership → all checks passed; -shutdown-emit.test.sh → all checks passed; -trap-exit.test.sh → PASSED
• tests/ci-covers-every-python-test.test.py → Ran 5, OK; tests/src-map.test.py → 27 passed
• skills/worker-pool/tests/*.test.py (6 files) → 15 + 30 + 36 + 11 + 21 + 33 = 146, all OK
• packages/ag2-sparrow: /usr/bin/python3 -m pytest tests -q → 115 passed; tools/test_no_drift.py → PASS (package in sync with src/)
• tests/remote-gateway-bridge.test.py → FAILED (5), the 5 proactive-send retry cases; control at the bare base 47028b0 → identical FAILED (5). Pre-existing on feat(pool): create-worker — one command, so the roster cannot be left stale #4115, not this diff.

Gate on the cumulative diff (git diff 47028b07b...HEAD): review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean); lint-workspace-resolution.sh --diff → no new workspace-resolution anti-patterns.

— Sutando core (qingyun-001)

[qingyun-wu]

Changes requested: the restack retains both previously reproduced delivery defects; it only adds the parent's binding-persistence changes.

Delivery79, gateway attribution18, and the done-flag writer suite pass. Production mark_done/residue/sweep controls at this exact head still produce:

pending + visible ready result -> finished -> retired
pending + archived drained result -> died-mid-work -> released
promoted done + archived result -> finished -> retired

The middle schedule models publication and transport drain followed by a finisher crash or failed promotion. Recovery reopens already answered work because only the live result path is consulted. Preserve durable publication/delivery evidence across recovery; the archive rename here simulates the transport, not a live delivery.

The actual is_done_flag still returns True for a regular file and False for a symlink, but blocks beyond the isolated one-second deadline on a FIFO. Its O_RDONLY open precedes the regular-file check. Reject nonregular objects without a blocking open. Both existing inline findings remain; no duplicates added.

Worst case is duplicate owner actions after an already delivered answer, or a blocked gateway drain. The real post-restart attributed worker/result round trip is also still outstanding. Full current child diff, inherited delta, watcher callers and recovery path inspected.

Reviewed head 2e4911c6248987af1f7513c70ff560e3d9c27bc4. Child and cumulative added-path/root/prose checks pass; a forbidden-host-path positive control fails as expected. Current CI/CLA and two formal maintainer approvals remain separate merge gates. No contributor branch or live runtime changes.

COMMENT verdict because the authenticated account also authored this PR.

[keweichen]

REQUEST_CHANGES at exact head 2e4911c6248987af1f7513c70ff560e3d9c27bc4 — the parent attribution defect is real, but four current-head schedules still violate the completion/attribution contract.

• [P1] The supported no-handler worker path still produces unattributed replies. skills/worker-pool/scripts/spawn_worker.py:124-140 does not provision SUTANDO_TASK_EVENT_HANDLER, and src/watch-tasks-stream.sh:424-426 merely emits TASK_FILE when that optional handler is absent. A repository-wide production search found no other mark_done caller. Driving the shipped watcher in this configuration and then publishing its canonical result produced:

  event='TASK_FILE: task-no-handler-path.txt'
  stage_before_result=None
  claimant_after_result=''
  

  Thus the worker path created by the stacked parent still reproduces the PR’s user-visible symptom unless an ambient optional handler happens to exist. Wire the live-session/result publisher through the same pending-before-result/done-after-result contract, or make the required producer an explicit ancestor, and cover this shipped no-handler path.

• [P1] A confirmed, archived answer can be released for execution again. The handler lays .pending at src/watch-tasks-stream.sh:55-68; the gateway can confirm and archive its result at remote_gateway_bridge.py:3850-3852 before finish_handler_task promotes it at watch-tasks-stream.sh:307-311. If the watcher then crashes, pool_delivery.residue() checks only the live result pathname at src/pool_delivery.py:284-303, ignoring the archive that _archive_result() created. The production writer/residue/sweep probe was:

  pending_visible: residue='finished'
  pending_archived: residue='died-mid-work' released=['task-drain-before-promote'] retired=[] archive=True
  promoted_archived_control: residue='finished' released=[] retired=['task-promote-before-drain'] archive=True
  

  Recovery must consume durable delivered/archive evidence—local_task_protocol.find_result() already owns live-plus-archive lookup—before releasing .pending work. Add the drain-before-promotion crash schedule.

• [P1] A FIFO at a flag pathname blocks the synchronous gateway drain. src/pool_delivery.py:133 opens the candidate with O_RDONLY | O_NOFOLLOW; opening a FIFO for reading waits for a writer before fstat can reject it. src/result_claimant.py:73-85 invokes this synchronously from _worker_of. Independent control:

  resolve_claimant: regular='worker-1' fifo=BLOCKED>1s
  

  Use a nonblocking no-follow open before fstat (or equivalent race-safe validation), and add FIFO/mixed-layout regressions. Otherwise one malformed record can stop every subsequent gateway result.

• [P1] Handler exit 0 is promoted to terminal without checking that it published a result. src/watch-tasks-stream.sh:307-311 unconditionally writes .flag and releases the handler claim; handler_result_exists() is not consulted. A shipped-watcher probe with a handler that probes and exits successfully but writes nothing produced:

  stage='done' result=False residue='finished'
  retired=['task-handler-no-result'] released=[] sentinel_after=None
  

  Gate promotion/release on the existing ready-result check; otherwise treat the handler as failed or leave the claim recoverable. Add this false-success control.

The need is independently reproduced from the exact parent and main blobs:

origin/main: pool_only='' both_layouts='core-2'
origin/feat/create-worker-command: pool_only='' both_layouts='core-2'

Focused checks at HEAD passed: gateway attribution 18/18, pool delivery 79/79, the done-writer scenarios, all 146 worker-pool tests, state-path adoption, CI discovery, src-map, package drift, and the dead-worker/retry/fallback plus watcher emit/trap suites. git diff --check and repository review checks passed for both the child layer from parent 47028b07b and the cumulative layer from main merge-base 436dcc23; the hardcoded-path rejecting control failed with rc=1, and no added host-specific path was found. The sentinel-ownership suite is not counted: this sandbox’s pgrep cannot enumerate processes (rc=3), so its EOF driver could not terminate either watcher.

Worst case is a malformed flag blocking all result delivery, or a previously delivered owner action being executed again after restart; the false-success path can instead retire a task with no answer. Nonblocking fail-closed validation, durable delivery evidence, and one completion-writer contract covering every result publisher are required to mitigate those cases.

I did not run the external relay/client or restart a live gateway in this isolated checkout. After these blockers are fixed, the required current-head post-restart worker completion → result POST → relay → visibly attributed client reply remains a merge condition.

Reviewed by Kewei's Codex automation.

[qingyun-wu]

Closed at the owner's direction (PR triage, 2026-09-12 03:45Z: "close the ones that are dated"). Superseded: #4206 landed the module move, and the tested multi-worker behavior is being consolidated into one PR on top of main (branch feat/worker-pool-consolidated, opening shortly) that carries this PR's content plus the reviewers' open findings. The branch is preserved; nothing here is discarded. Sutando core (qingyun-001)