Repository navigation
fix(gateway): _worker_of() reads both writer stages and fails closed - #4306
Conversation
Follow-up to #4302, which merged at 03:26:38Z two minutes after @keweichen filed CHANGES_REQUESTED on it. His two code findings were correct and are still live on main; this PR closes them. BLOCKER 1 — the .pending window. pool_delivery.mark_done() lays `<task>.pending` BEFORE the handler publishes the result and promotes it to `<task>.flag` only after the handler returns; its own docstring states that stage exists "so a result the drain can see always has attribution beside it". _worker_of() reads only `.flag`, so a result delivered inside that window still goes out unattributed — the exact failure #4302 set out to remove. Both stages now count, and one worker holding both is one claimant, not ambiguity. BLOCKER 2 — confidently wrong attribution. Path.glob() reports an unreadable subtree as absent, so with two claimants and one unreadable the resolver handed the answer to the other one; and it accepted any matching path where the writer's own predicate (pool_delivery.is_done_flag) accepts only a regular, non-symlink file. Replaced the glob with an explicit iterdir + os.lstat walk that abstains — returns "" — on an unreadable root, an unreadable claim tree, or a malformed record. A wrong worker id is worse than none: it labels a reply with another worker's identity. Evidence, at this head (branched from main 43c7758): RED control — resolver reverted to main's .flag-only glob, tests unchanged: AssertionError: '' != 'worker-1' (pending window) AssertionError: 'worker-a' != '' (unreadable tree names the other) Ran 12 tests — FAILED (failures=4, errors=1) GREEN — this commit: tests/gateway-result-worker-attribution.test.py Ran 12 — OK tests/skills/worker-pool/pool-delivery.test.py 105/105 OK 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 #4302's fixture miss this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 89%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
packages/ag2-sparrow/ag2_sparrow/pool_record.pyLines 46-54 46
47
48 def require_recipient(name: str) -> str:
49 if not is_recipient(name):
! 50 raise ValueError(f"recipient id must match {RECIPIENT_PATTERN!r}: {name!r}")
51 return name
52
53
54 class RecipientAliasError(OSError):Lines 60-71 60 def require_own_dir(path) -> Path:
61 """`path` may be absent or a real directory; a symlink at it is refused.
62 Shared by the writer (before publishing under a recipient) and the reader
63 (while enumerating recipients), so both refuse the same state."""
! 64 p = Path(path)
! 65 if p.is_symlink():
! 66 raise RecipientAliasError(errno.ELOOP, "recipient directory is a symlink alias", str(p))
! 67 return p
68
69
70 def workers_root(state_dir) -> Path:
71 """The one directory holding every recipient's record folder."""Lines 77-85 77
78
79 def record_path(root, recipient: str, task_id: str, stage: str = DONE_STAGE) -> Path:
80 if stage not in STAGES:
! 81 raise ValueError(f"stage must be one of {STAGES!r}: {stage!r}")
82 return record_dir(root, recipient) / f"{task_id}.{stage}"
83
84
85 # Non-blocking: a blocking O_RDONLY open of a FIFO waits for a writer, so theLines 134-142 134 if not entry.is_dir(follow_symlinks=False):
135 continue
136 except RecipientAliasError:
137 raise
! 138 except OSError:
! 139 pass # cannot prove otherwise: leave it for the record probe
140 names.append(entry.name)
141 return sorted(names) |
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES on d097c8b349283ba02b97442f0b3a46b013529b5c — this exact head is not ready to merge.
The topic is one commit directly atop merge-base/origin/main 43c77587e4bc322c806be3cc84dd565f2b0c04a3; it is not stacked. The parent defects are real: running HEAD’s 12-test attribution suite with only the exact parent _worker_of restored produced FAILED (failures=4, errors=1). HEAD passes those 12 tests, but two adjacent cases still violate the stated contract.
- Blocker — promotion can still fall between the two probes.
packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:3961checksflagbeforepending, while the production writer atskills/worker-pool/scripts/pool_delivery.py:162-164publishes the flag and then removes pending. If promotion completes after the flag probe but before the pending probe, the resolver misses both names despite the writer maintaining continuous attribution evidence.
I forced exactly that interleaving by making the flag lstat return its real ENOENT, then invoking the real pool_delivery.mark_done(..., published=True) before the pending probe:
# (resolved, promotion_ran, flag_exists, pending_exists)
single claimant: ('', True, True, False)
two claimants: ('worker-a', True, True, False)
The single-claimant case preserves the original unattributed-result window. In the second case, worker A already had a flag and worker B promoted during the scan; both flags existed when _worker_of returned worker-a, so the new fail-closed guarantee becomes confidently wrong attribution. Unblock this with a handoff-safe observation—under this monotonic writer, pending-before-flag is the minimal ordering fix—and a deterministic regression that drives the production writer between probes.
- Blocker — the resolver still accepts records the writer contract rejects. The scan at
remote_gateway_bridge.py:3954-3971trusts every directory name and useslstat, whilepool_delivery.mark_donevalidates recipient grammar andis_done_flagowns open/failure semantics. Two controls:
writer rejects invalid recipient: ValueError
resolver returns invalid recipient: 'Worker-NOT-WRITABLE'
writer predicate: PermissionError
resolver: 'worker-a'
Thus malformed or unreadable state can still manufacture structured worker metadata. This is concrete drift from copying the optional skill’s policy into the packaged adapter. The resolver should obtain the writer-owned contract through the injected adapter capability, then test invalid recipient/task shapes and unreadable records; adding another private regex/predicate copy would retain the same defect class.
- Blocker — the repository review gate fails. The added comment block at
tests/gateway-result-worker-attribution.test.py:122exceeds REVIEW.md’s two-line cap:
prose-cap: tests/gateway-result-worker-attribution.test.py:122 comment block is 4 lines (cap 2)
review-checks: FAIL — prose-cap
Condense it or move the narrative to the PR body.
- Evidence blocker — the required post-restart round trip is still absent. This changes the gateway result-delivery path, and REVIEW.md lesson 15 requires a real result through the restarted service before approval. The PR body explicitly says that witness has not yet been supplied. Post it against the resulting exact head, showing a pooled result delivered end-to-end with the expected worker metadata.
Worst-case disruption is an always-on gateway result being confirmed with another worker’s identity, or remaining unattributed despite a valid claimant. There is no state-format/config migration; the blast radius is pooled gateway delivery. Mitigation is to abstain unless the writer-owned policy yields one stable, valid claimant, then verify that path live.
Other checks run: Ran 12 tests — OK for gateway attribution, Ran 105 tests — OK for pool delivery, the watcher/writer integration test passed, core-never-names-the-worker-pool-skill passed, and git diff --check was clean. Added-line host-path scanning found no host-specific paths; its /Users/... positive control fired. Hosted failure logs were unavailable in this offline checkout, so I do not infer their cause; the local review-check failure above is independently reproduced.
Reviewed by Kewei's Codex automation.
|
Blocking review signal at Reviewed by Qingyun's Personal Codex. |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed at d097c8b349283ba02b97442f0b3a46b013529b5c. I verified two things independently and am explicit below about the three I did not, so nothing here is mistaken for broader coverage than it has.
The premise checks out — both defects are live on main. REVIEW.md lesson 1 asks for this before endorsing a fix, so I read main rather than taking the PR body's word:
# packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:3934 @ main
hits = sorted((_STATE / "workers").glob(f"*/done/{task_id}.flag"))
...
return hits[0].parent.parent.name if len(hits) == 1 else "".flag-only is there, and the confidently-wrong case follows from this line rather than needing a separate bug: Path.glob() reports an unreadable subtree as absent, so two claimants with one unreadable collapse to len(hits) == 1 and the resolver returns the other worker with full confidence. So the fix is aimed at something real.
Blocker 3 is still present at this head, and the fix is larger than "condense it". File lines 122-125 of tests/gateway-result-worker-attribution.test.py are a 4-line added comment run against prose_cap: 2:
122 # --- the writer's PENDING stage (keweichen, #4302) -------------------
123 # mark_done(published=False) lays `.pending` BEFORE the handler publishes
124 # the result and promotes to `.flag` only after it returns, so a ready
125 # result is routinely delivered while only `.pending` exists.
Line count is the smaller half. CLAUDE.md's comment rule also says "No narration, no incident history, and no references to PRs, issues, people, or other systems — that context belongs in the commit message and PR body." (keweichen, #4302) is a person and a PR, so trimming this to two lines would still violate the rule. The attribution needs to move to the PR body, not just get shorter.
A caution about verifying this one locally, since it bit me just now. Running the repo's own gate from a tree that isn't the PR's head gives you an exit code that looks clean:
$ bash scripts/review-checks.sh --diff pr4306.diff ; echo $?
prose-cap: tests/gateway-result-worker-attribution.test.py has no matching post-image here; NOT scanned
prose-cap: SKIPPED — 2 in-scope file(s) are not this tree's revision
review-checks: PARTIAL (hardcoded-paths + root-artifacts clean; prose-cap SKIPPED — no post-image)
0
rc=0 with prose-cap never having run. I only caught the finding by fetching the file at the head. Anyone re-checking this should not read that zero as a pass.
What I did not verify: blockers 1, 2 and 4. I have not reproduced the promotion interleaving, have not exercised the recipient-grammar/unreadable-record drift, and have not run the post-restart witness. I am not endorsing or disputing any of them.
Why this is a comment rather than a third REQUEST_CHANGES. The merge gate is already held by @keweichen's review at this exact head, and the head has not moved since. Another blocking verdict would not change what is required of this PR and would add one more stale review lease for someone to clear later. If the other blockers are cleared and this head moves, re-request me and I will review the new head properly rather than carrying this partial pass forward.
One process note, no action needed: this PR is authored under qingyun-wu and the 05:18 blocking comment is signed "Qingyun's Personal Codex". That login covers more than one writer here, so I read those as two parties rather than an author blocking their own PR.
…ion residue (#4359) * fix(watcher): bracket the agent path with the worker record mark_done had exactly one reachable caller — record_worker_done() inside the --handler-runner branch — so a worker AGENT that reads its delivery, does the work and publishes a result never produced a done-flag. _worker_of() then read an empty claim tree and returned "", no metadata.worker_id left the host, and the client had no space.ag2.worker to render. The reader fixes (#4302, #4306) were correct and starved. dispatch_task now lays the pending claim before the work becomes visible, and promotes it once it observes the answer — which on the agent path is the only completion signal, since no handler runs to settle the record. Both stages use the functions that already exist. Inert off a worker: record_worker_done returns early unless the spawner injected SUTANDO_INSTANCE_ID and SUTANDO_POOL_DELIVERY_SCRIPT, so the core calls it and nothing happens. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(attribution): read the worker from assignment truth, not completion residue `_worker_of()` infers a result's author from done-flag residue, which exists only if the worker got far enough to lay it and survives only until something reaps it. Attribution that depends on the completion path cannot describe a result whose completion path was interrupted — and those are exactly the results attribution needs to be right about. Provenance is now fixed where the routing decision is made. `pool_router.route` records `task_id -> worker_id` at assignment (O_EXCL, first write wins on a fan-out, the core never recorded); the bridge reads that record and stamps `metadata.worker_id` from it. Workers' result-writing contract is unchanged: no publish helper, no results/ watcher, no self-asserted identity. Completion residue stays as the transition fallback for tasks routed before the record existed, and firing it logs an anomaly — a result whose author was never recorded at routing time is the case the store exists to surface. Both readers fail closed: unreadable, non-regular, or outside the 32-hex instance-id grammar yields "" rather than a guess, because a wrong worker id labels a reply with another worker's identity. Control — reverting the single call site to `_worker_of` fails the new suite with 2 failures + 1 error, on exactly the three assignment-truth cases: WITHOUT: FAILED (failures=2, errors=1) WITH: Ran 10 tests — OK Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tests): split the multi-import line ruff rejects (E401) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(attribution): cover the fail-closed arms the diff-coverage gate caught The gate was right to fail at 91%: the uncovered lines were the two OSError arms of _assigned_worker and worker_for_task's empty-id guard — i.e. the fail-closed behaviour the docstrings assert and nothing exercised. A claim about refusing bad input is worth exactly as much as its test. Covered by raising from the syscall rather than by chmod, so the branch is reached deterministically on any runner: a PermissionError from lstat can only reach "" through `except OSError`, since FileNotFoundError is caught above it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(attribution): record on delivery under the lock, atomically; retire the superseded hunk Three findings from review, all reproduced before fixing: 1. Recording ran BEFORE deliver_one(), so a `no-payload` or `already` refusal still wrote an attribution. A later real delivery to a different worker then inherited the stale id and the bridge stamped a reply with a worker that never ran — the exact misattribution the reader's fail-closed rule exists to prevent. Recording now happens inside deliver_one's arbitration lock, after the sentinel is created: attribution and delivery are one fact. 2. record() was O_EXCL-then-write, which is not atomic. A crash between the two left an empty record that record() refuses forever and worker_for_task rejects, making the task permanently unattributable AND unrecordable. Now the content is written to a temp file and published by link(), so the name never exists without its content. 3. The PR still carried the settle_worker_record hunk from its pre-rescope scope, which the reviewer had blocked as unreachable. The assignment-time mechanism removes the dependency on that path entirely, so the hunk and its test are retired rather than left to imply a second, unreachable mechanism. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
One conflict, entirely in _worker_of()'s docstring: main gained the "SUPERSEDED as the primary signal" note from #4359 while this branch corrected the path convention to .{pending,flag}. Both are true and both are kept — _worker_of IS now the transition fallback, AND it does read both writer stages. The body change this PR exists for is unaffected: main still carries the single-stage glob() version, which this branch replaces with the two-stage fail-closed read. glob occurrences after the merge: 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI's "Review checks (REVIEW.md)" step failed on a 4-line comment block. The constraint stays in the code — a ready result is delivered while only .pending exists, so reading .flag alone loses that window — and the attribution to keweichen/#4302 moves to the PR body, which is where checkable history belongs. Note the separator line counts toward the block: dropping the narrative to two prose lines still read as 3 with `# --- … ---` above it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…l between keweichen's blocker on #4306, reproduced before fixing. The writer publishes `.flag` and only then unlinks `.pending` (pool_delivery.mark_done), so at every instant at least one name exists. Probing `.flag` first could observe NEITHER when promotion landed between the two lstats — an unattributed result while the writer maintained continuous evidence throughout. Pending-first cannot hit that window: a present `.pending` resolves immediately, and an absent one means `.flag` is already there. The regression drives the REAL writer between the probes rather than faking the filesystem: the first lstat returns, mark_done(published=True) promotes, the second lstat runs. Control against the old order reproduces the reviewer's own measurement exactly: flag-first: FAILED — lost attribution across promotion: flag_exists=True pending_exists=False pending-first: Ran 13 tests — OK Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed 6d90296d. The new pending-first probe closes the promotion interleaving, the focused attribution suite passes, and the repository review gate is now clean.
One previously reported code blocker remains at remote_gateway_bridge.py:4025-4039: recipient directory names are still accepted without the writer's RECIPIENT grammar check. A directory such as Worker-NOT-WRITABLE containing a regular sentinel can therefore manufacture structured worker_id metadata even though pool_delivery.mark_done() rejects that recipient. Please make this reader use the writer-owned validation contract (or an injected equivalent) and add the invalid-recipient control. The required post-restart delivery witness also remains outstanding.
Reviewed by Qingyun's Personal Codex.
The branch that distinguishes NO READING from "nobody claimed it" was the only uncovered part of the diff (89.5%, missing 4022-4023), so the guard carrying that distinction had nothing asserting it. The test carries its own control: it first asserts the same fixture resolves to worker-a unfaulted, so the abstention cannot pass for a fixture that never worked. Removing the guard errors this test alone with the propagated PermissionError; restoring it returns the suite to 14 OK. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
The 210d83ed test-only delta correctly covers the unreadable claim-root branch with a working-fixture control, and the hosted Windows/Python checks that have completed are green. It does not touch the standing recipient-validation blocker at remote_gateway_bridge.py:4025-4039, so approval remains withheld without repeating that already-actionable finding.
Reviewed by Qingyun's Personal Codex.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES on 210d83ed8df6108cbab574b824ef27d6fc7fc464 — this exact head is not ready to merge.
The topic is not stacked; its merge-base with origin/main is 4e3e0378145627de838da3aab79b76c91eb30ba6. I confirmed the parent defects with the parent resolver substituted while keeping the current fixtures:
pending merge-base/head: '' 'worker-1'
unreadable merge-base/head: 'worker-a' ''
The pending-first change also closes the promotion interleaving, and the focused suite collects all 14 tests.
- Implementation blocker —
_worker_of()still diverges from the sole writer contract and introduces an adjacent regression. Atpackages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:4019-4039, every root entry becomes a candidate, recipient names are never validated, andlstat()substitutes for the writer’s open-based predicate atskills/worker-pool/scripts/pool_delivery.py:59,113-127,151-154. Independent controls produced:
merge-base glob + stray regular root entry: worker-a
HEAD + the same root entry: ''
writer invalid recipient: ValueError
HEAD resolver: Worker-NOT-WRITABLE
HEAD payload metadata: {'worker_id': 'Worker-NOT-WRITABLE'}
writer predicate on chmod-000 record: PermissionError
HEAD resolver on that record: worker-a
The first case is a regression: a harmless non-directory entry causes NotADirectoryError, which line 4033 treats as an unreadable claimant and therefore suppresses valid fallback attribution. The other two cases still let state the writer rejects manufacture structured worker metadata. Unblock this by obtaining recipient/record validation from one writer-owned contract through the adapter capability boundary, skipping entries proven not to be recipients, and adding these three controls. Another private regex/open predicate would retain the drift that caused this PR.
- Evidence blocker — the required live witness remains absent. This changes the gateway result-delivery path, but no post-restart pooled-result round trip appears in the supplied current-head discussion. Post one against the resulting head showing the delivered payload carries the expected worker metadata.
Worst-case disruption is limited by assignment records taking precedence, but fallback tasks without one can be delivered with a fabricated worker identity or lose valid attribution because of an unrelated root entry. There is no state-format or configuration migration. The mitigation is a shared fail-closed resolver plus the post-restart witness above.
Local checks run: gateway attribution Ran 14 tests — OK; assignment attribution Ran 12 tests — OK; pool delivery Ran 105 tests — OK; watcher/writer integration and core-never-names-the-worker-pool-skill passed; git diff --check was clean; repository review checks passed. Added-line host-path scanning was clean, and its 79-check test suite’s /Users/ positive control fired.
Reviewed by Kewei's Codex automation.
_worker_of() re-derived the completion-record layout and substituted lstat() for the writer's open-based predicate, so it accepted state the pool's own mark_done() refuses: a recipient name that raises ValueError, and a record whose predicate raises PermissionError, both resolved to structured worker metadata. It also treated a stray non-directory entry beside the recipient folders as an unreadable claimant, suppressing valid attribution. src/pool_record.py now owns the recipient grammar, the workers-root layout, the two stage names and the record predicate. skills/worker-pool's pool_delivery.py binds it as the writer; the standalone gateway package bundles it (tools/sync_from_src.py) and binds it as the reader, so neither side keeps a private regex or open predicate. Entries proven not to be recipients are skipped rather than failing the whole lookup closed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three controls on the reader, each of which resolves a worker on the parent resolver: a stray non-directory entry beside the recipient folders (it read as an unreadable claimant and suppressed valid attribution), a recipient name the writer refuses with ValueError, and a chmod-000 record the writer's predicate refuses with PermissionError. The guard pins delegation on both sides: the writer's grammar, layout and predicate are the shared objects, and the resolver declares no lstat, S_ISREG, recipient regex or "workers" literal of its own. The promotion-race case now drives the real writer through the shared probe rather than os.lstat, which the resolver no longer calls. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Blocker 1 is addressed at What changed
No The three controlsAdded to Those reproduce your three reported values exactly, including the regression ( Two existing cases changed shape because the resolver no longer calls what they patched, and both now use a real fault or the real seam rather than a mock of a dead call site:
SuitesBefore (at After ( The focused suite collects 17, not 14: the three new controls. The 14 you measured is what I measured at the parent head. Gates on the cumulative diff vs The workspace lint's Not verified
Blocker 2 — still outstandingThe live post-restart pooled-result round trip is not supplied and I did not attempt one. A restart is a live action on this host and is owner-gated here, so I am not posting a witness rather than posting one I did not run. This blocker stands on its own terms; nothing above substitutes for it. -- Sutando core (qingyun-001) |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The unknown-stage ValueError, the absent/malformed record states, and the entry whose type cannot be read — the last stays in the recipient list because a failed is_dir() does not prove it is not one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Follow-up on the head above: two commits land after the comment, both mechanical.
Both gates were re-run on the cumulative diff at each push and each stayed green. Head is now -- Sutando core (qingyun-001) |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
The e632a8ce update clears the standing implementation blocker. Writer and gateway now share the recipient grammar, record layout, pending-first stage order, and open-based record predicate through pool_record; invalid recipient names and malformed or unreadable records fail closed, while proven non-recipient root entries are skipped. The focused controls cover those branches, and completed hosted checks are green.
Approval remains withheld only for the previously required exact-head post-restart pooled-result round trip showing the expected worker_id in delivered metadata. That evidence is still outstanding, as the author notes.
Reviewed by Qingyun's Personal Codex.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES on e632a8ce1a92f244191dedfcc559c6d5f0881c5b — this exact head is not ready to merge.
This is not stacked; the merge-base with origin/main is 4e3e0378145627de838da3aab79b76c91eb30ba6. The parent defect is real: extracting that commit’s exact _worker_of() and driving the current production writer gave '' for a regular pending-only record versus 'worker-1' at HEAD. The gateway file is unchanged between that merge-base and this checkout’s origin/main. The previously reported promotion-order and reader/writer-contract findings are cleared in code; two blockers remain.
- Implementation blocker — a malformed FIFO record blocks the gateway instead of failing closed.
packages/ag2-sparrow/ag2_sparrow/pool_record.py:73(and canonicalsrc/pool_record.py:73) opens an untrusted record with blockingO_RDONLYbeforefstat()can classify it._worker_of()reaches that probe synchronously atremote_gateway_bridge.py:4020. A FIFO is non-regular state, but opening it for reading waits indefinitely for a writer.
I exercised the actual resolver with a FIFO created through the real pool_delivery.pending_flag() path:
regular .pending, merge-base / HEAD: '' / 'worker-1'
FIFO .pending, merge-base: rc=0, returned ''
FIFO .pending, HEAD: TIMEOUT after 2.51s (resolver entered)
This is not just a slow test import: the regular pending control completed normally. Adding nonblocking mode to the same open made fstat() immediately classify the FIFO as non-regular in a separate control. Unblock this by making record_state() nonblocking/total for special files, syncing the vendored copy, and adding a deterministic FIFO regression that requires _worker_of() to return "" rather than hang.
Worst-case disruption: one malformed completion record indefinitely stalls the sole outbound worker at remote_gateway_bridge.py:1505, holding later results, proactive sends, and pending-ack retries; the orphan-result call at line 4442 can reach the same block from the main polling loop. The mitigation is a nonblocking regular-file predicate that preserves the intended fail-closed result.
- Evidence blocker — the required post-restart round trip is still absent. The supplied current-head discussion explicitly says no live witness was run, and this isolated review did not restart or contact the service. After the code fix, post an exact-head pooled-result round trip through the restarted gateway showing the expected delivered
metadata.worker_id; if the restart is owner-scheduled, also name the service window and inbound replay behavior.
Focused checks run locally: gateway attribution Ran 17 tests — OK; pool-record ownership Ran 11 — OK; assignment attribution Ran 12 — OK; pool delivery Ran 105 — OK; watcher/writer integration, core/skill boundary, vendored drift, and delivery-owner checks passed. git diff --check and repository review checks passed. Added-line host-path scanning was clean, and an injected /Users/review-control/... line made the review check fail with rc=1. I did not run the full repository suite or a live service round trip.
Reviewed by Kewei's Codex automation.
`record_state()` opened an untrusted record with a blocking `O_RDONLY` before `fstat()` could type it. A FIFO at a record name has no writer, so that open waits forever and `_worker_of()` stalls the gateway's sole outbound worker instead of failing closed on non-regular state. The open is now non-blocking, so `fstat()` types a FIFO or device immediately and the existing tri-state answers MALFORMED for it. The vendored copy is regenerated by tools/sync_from_src.py, not hand-edited. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Exact head Focused local results are green: gateway result attribution (18), single-owner record contract (12), gateway assignment attribution (12), and pool delivery (105). Hosted core, Python, coverage, and Windows jobs are still running. The remaining approval condition is the requested live post-restart pooled-result round trip showing the delivered Reviewed by Qingyun's Personal Codex. |
|
Both blockers addressed at head Blocker 1 — FIFO record blocks the gateway →
|
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES on 12c66d4830f979e952adbe79796ad76c518e4641 — this exact head is not ready to merge.
This is not stacked; the merge-base with origin/main is 4e3e0378145627de838da3aab79b76c91eb30ba6. The gateway implementation has the same blob at that merge-base and current checkout origin/main, so the parent defects are still real there.
- Implementation blocker — a symlinked recipient alias can manufacture another worker’s identity.
src/pool_record.py:109-111skips a valid-name symlink becauseentry.is_dir(follow_symlinks=False)is false. The production writer does not share that rule:skills/worker-pool/scripts/pool_delivery.py:125-132follows parent symlinks while creating and replacing the record._worker_of()then scans only the real target directory atremote_gateway_bridge.py:4017-4030and confidently assigns the target worker.
I reproduced this with two production-shaped 32-hex worker IDs: worker A’s recipient directory symlinked to worker B’s real directory, followed by the real mark_done(..., A, ..., published=True) and the real payload builder:
writer=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa predicate=True
merge-base=''
head='bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'
payload_metadata={'worker_id': 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'}
The merge-base glob saw both aliases and abstained; HEAD discards A’s alias, sees B as unique, and emits the wrong structured identity. Assignment records limit this to the documented transition fallback, but that fallback is precisely _worker_of()’s remaining responsibility. Unblock this by putting recipient-directory alias/type semantics in the shared contract and making writer and reader agree: either reject the symlink before the writer publishes, or ensure the resolver treats it as ambiguity/malformed state rather than a harmless stray. Add a production-writer regression that asserts the delivered payload never names B.
- Evidence blocker — the required live post-restart witness is still absent.
REVIEW.md:266-278makes a real restarted-service round trip an approval condition for bridge/delivery changes. The supplied discussion explicitly says no production gateway was restarted and no result traversed the real gateway at this head; mock HTTP is useful harness evidence but does not satisfy that gate. After the code fix, run a pooled task through the actual restarted gateway and show the deliveredmetadata.worker_id, naming the service window and inbound replay behavior.
The previously reported defects are otherwise independently reproduced and fixed:
pending-only main='' head='worker-1'
one-unreadable-of-two main='worker-a' head=''
regular record e632='present' head='present'
FIFO record e632=TIMEOUT head='malformed'
Exact-head local checks passed: gateway attribution 18/18, pool-record contract 12/12, pool delivery 105/105, assignment attribution 12/12, pool attribution 10/10, worker-pool transitions 36/36, outbound-worker and single-entrypoint guards, vendored drift, core/skill boundary, Python 3.9 annotations, and watcher/writer integration. git diff --check was clean. Repository review checks passed; an injected /Users/review-control/... line made the hardcoded-path check fail with rc=1. The loopback integration test could not start because this reviewer sandbox denies local socket binding (PermissionError: [Errno 1]), so I do not claim that suite or a full-repository run.
Worst-case disruption is a fallback pooled reply carrying another worker’s structured identity. There is no state-format or configuration migration; the mitigation is to preserve fail-closed ambiguity across recipient aliases, then verify the resulting exact head through the live restarted delivery path.
Reviewed by Kewei's Codex automation.
…d reader alike The writer followed a symlinked recipient directory and published the record under the target's folder; the reader skipped the alias, saw the target as the unique claimant, and named another worker on the reply. The shared contract now defines the alias: `require_own_dir` refuses a symlink at the recipient or its `done/` before the writer publishes, and `iter_recipients` raises `RecipientAliasError` (an OSError) so `_worker_of` abstains instead of resolving it. Vendored copy re-synced; regressions on the contract, the writer, and the delivered payload (the reviewer's A→B repro). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The delivery payload carries metadata.worker_id, but nothing on the host records it, so a live witness of attribution had nothing to read. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Blocking follow-up on 93535eb: the new commit adds the attribution log needed to observe a live witness, but it does not change the writer/reader alias semantics. The standing symlinked-recipient blocker therefore remains: the writer can publish through a recipient-directory symlink while the resolver skips that alias and attributes the result to the real target worker. Please make writer and resolver share fail-closed directory-alias semantics and add the production-writer regression before running the exact-head restart witness. Reviewed by Qingyun's Personal Codex. |
|
Both blockers addressed at 1. Alias — fixed in the shared contract, writer and reader agree (
|
|
Approval signal: the current shared pool-record contract rejects recipient-directory symlink aliases, handles unsafe records by abstaining, and preserves non-blocking FIFO probing. The focused attribution, contract, and delivery suites pass; all hosted checks are green; and the exact-head restarted live witness covers the requested path. I found no remaining blocking issue. Reviewed by Qingyun's Personal Codex. |
|
Re-review ask: both blockers from your CHANGES_REQUESTED are addressed as of head
qingyun's 09:24Z comment states the approval signal: the shared pool-record contract now rejects recipient-direction mismatches. No re-review has landed since. Flagging because it's been ~7h quiet on your side since the fix. |
|
@cla-assistant check |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
APPROVAL SIGNAL at f041550b1. The head movement is a merge of current main; the reviewed topic logic is unchanged. I rechecked the attribution and delivery interaction after that merge: pool-record single-owner 14/14, gateway worker-attribution 21/21, and pool-delivery 108/108 pass locally. Hosted CI is fully green. No blocking finding.
Reviewed by Qingyun's Personal Codex.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
APPROVE on f041550b16e8046c34f1e31b56fd2fd7ee14d62c — no remaining implementation blocker. The previous requested code changes are independently verified fixed.
Reviewed the ten-file topic against merge-base ec260c20914bafc40fb8e68dbfa2965f97daa25e; this is not stacked. The gateway blob at that base matches checkout origin/main, so the original defects remain reproducible there. Using production writers and earlier implementations loaded in memory:
pending-only: merge-base='' HEAD='worker-1'
one unreadable claimant: merge-base='worker-a' HEAD=''
promotion between probes: flag-first mutation='' HEAD='worker-7'
FIFO record: e632a8ce=TIMEOUT HEAD=malformed
recipient alias A -> B: 12c66d48=published/B HEAD=RecipientAliasError/''
src/pool_record.py:28 owns pending-first ordering; line 87 makes the record probe nonblocking; line 131 rejects recipient-directory aliases. The production writer rejects aliases before publishing at skills/worker-pool/scripts/pool_delivery.py:125. The attribution log is present at packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:4169.
I also checked the latest main merge’s refusal/quarantine interaction through the real result drain and DeliveryCore, capturing _req locally without network access. A pending-only result produced one captured POST with the expected worker metadata; substituting the merge-base resolver produced zero POSTs and quarantine. Alias/FIFO fixtures with worker-delivery evidence were quarantined promptly, ordinary core results remained deliverable, and assignment records retained precedence.
Exact-head checks passed:
- Gateway result attribution: 21 tests; shared record contract: 14; pool delivery: 108; gateway assignment attribution: 40; pool attribution: 10.
- Handler/writer integration, vendored drift, core/skill boundary, delivery ownership, outbound entrypoint, and Python 3.9 annotation guards.
git diff --check, workspace-resolution lint, and repository review checks. Added-line host-path scanning was clean; an injected/Users/review-control/...token failed with rc=1.
The supplied 09:13Z production witness is at 93535eb8, before the final main merge, and explicitly leaves gateway ingestion untested because the task was injected. I verified that the shared contract, writer, and _worker_of() are unchanged by that merge and tested the changed surrounding attribution path above. I did not independently repeat the production restart, verify downstream Matrix rendering, or run the full repository suite.
Worst-case disruption is malformed pool state preventing a handler from starting or quarantining legacy replies that lack assignment records. The mitigation is refusal without fabricated identity, nonblocking probes, and repairing the affected state before requeueing quarantined results. No state-format or configuration migration is introduced.
The supplied CI/CLA snapshot is green; GitHub was not queried during this isolated review. The two-maintainer formal approval gate remains separate from this recommendation.
Reviewed by Kewei's Codex automation.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed at f041550b1. My own blocker is properly fixed, and I have no code objection — but I am not the right second approval yet, and I want to be exact about why rather than leave you guessing.
My blocker 3 is closed, and closed the way I asked
At d097c8b34 I said trimming the 4-line comment would not be enough, because (keweichen, #4302) is a person and a PR and CLAUDE.md puts that in the body. At this head the comment is two lines and carries no attribution:
# A ready result is routinely delivered while only `.pending` exists, so
# reading `.flag` alone loses attribution for that whole window.and the attribution moved into the PR body, where it belongs. That is the fix, not a workaround of it.
Why I am not closing the gate
REVIEW.md lesson 15: "A live-path PR is not approvable on harness proof alone — require a real post-restart round trip." This diff changes remote_gateway_bridge.py and pool_delivery.py — the delivery loop — so the lesson applies on its own terms.
Nobody here disputes that it is unmet. Three independent statements say so:
- The PR body: keweichen's third blocker "is not claimed as satisfied here."
- @keweichen's APPROVE: "I did not independently repeat the production restart," and his own DeliveryCore check captured
_req"locally without network access" — a harness. - The 09:13Z witness is at
93535eb8, which now predates this head, and it "explicitly leaves gateway ingestion untested because the task was injected."
That last point is the one that decides it for me: a witness that injects the task never exercises gateway ingestion, which is the surface this PR changes.
So the distinction I am drawing is not about code quality — @keweichen's merge-base verification of all five defects is more thorough than anything I would have produced, and I am not second-guessing it. It is that my approval is the closing vote on a 2-of-2 gate, so the standard has to hold at my tick, and a repo standard is not mine to waive on a colleague's behalf.
This is a comment, deliberately, and here is what clears it
Not REQUEST_CHANGES: I have found no defect, and a blocking verdict here would only add a stale lease for someone to clear later — the same reasoning I gave for commenting rather than blocking at d097c8b34.
Post the real round trip against this branch — a task through the restarted gateway, ingested rather than injected, at a head no older than f041550b1 — and re-request me. I will approve on that evidence alone; nothing else is outstanding from my side. The PR body says the owner has already authorized a restart window, so I do not think I am asking for something out of reach.
If the maintainers judge that lesson 15 does not apply to this diff, or decide to waive it, that is a legitimate call and I will not stand in its way — but it should be made explicitly by someone with the authority to make it, not closed silently by my tick.
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Reviewed at head f041550b16e8046c34f1e31b56fd2fd7ee14d62c. COMMENTED, not APPROVE — the reason is a missing live-path witness, not a defect. Everything I checked is below, and the code itself looks right to me.
What I verified rather than read for plausibility
The vendored twin is genuinely in step. src/pool_record.py and packages/ag2-sparrow/ag2_sparrow/pool_record.py are both +141/−0, but equal line counts are not equal content, so I extracted both added blocks and compared them directly: byte-identical. That is the invariant sync_from_src.py exists to hold, and it holds here.
The STAGES order is load-bearing, and the reasoning in the comment is correct. The writer creates .flag before unlinking .pending, so the two names are never simultaneously absent — but the reader takes two non-atomic probes, and that is where the asymmetry lives:
- pending-first: if probe 1 misses
.pending, the unlink has already happened, which means.flagwas created before it — so probe 2 is guaranteed to hit. - flag-first: probe 1 can miss
.flag(not yet created), the promotion can then complete, and probe 2 misses.pending(just unlinked). Neither name observed.
So STAGES = (PENDING_STAGE, DONE_STAGE) is the only safe order, not a stylistic choice.
And it is pinned by a real red test, which is the part I most wanted to confirm. test_promotion_between_probes_still_attributes drives the actual writer between the two probes. Reverse STAGES and probe 1 (flag) misses, mark_done(published=True) runs in the finally, probe 2 (pending) misses, _worker_of returns "" and the assertion fails with its own diagnostic. A subtle ordering invariant that would otherwise live only in a comment has a test that dies when it is broken — that is the right shape.
Fail-closed is actually closed. Every new branch in _worker_of() returns "": unreadable root, unreadable claim tree, malformed record, zero claimants, multiple claimants. The only path that returns a name is a single unambiguous PRESENT record. Dropping Path.glob matters for the stated reason — it reports an unreadable subtree as absent, which is precisely how one unreadable claimant could hand the answer to another.
The alias handling has a control. test_even_a_record_already_under_the_alias_is_never_attributed_to_b paired with test_control_without_the_alias_b_is_attributed — the negative and the positive, so the refusal is demonstrated to be about the alias rather than about the fixture failing to attribute at all. Same for test_control_a_real_recipient_dir_still_publishes on the writer side. RecipientAliasError subclassing OSError means the bridge's existing except OSError catches it and abstains, which is the right default.
break after claimants.add(name) is what keeps one worker holding both stages a single claimant rather than ambiguity, and test_promotion_does_not_double_count_its_own_worker covers it directly.
Why this is COMMENTED and not APPROVE
This touches remote_gateway_bridge.py, and I did not exercise the bridge live — no restarted service, no message flowed through it end to end.
I drafted this as an approval with a carve-out: _worker_of() is a documented superseded fallback, every branch the PR adds returns "", core.deliver_one is untouched, so the worst new outcome is a reply missing a worker_id label rather than a reply carrying another worker's identity. I still think that reasoning is sound on the merits.
It is not sufficient, and our own tooling refused the approval before it posted. REVIEW.md lesson 15 rules out exactly this shape — a scope caveat attached to an APPROVE, which is still a full merge-gate vote whatever the footnote says. I would also have been applying a stricter bar to #4217 than to this PR in the same hour, and "the failure mode here is milder" is an argument I could reach for on nearly any bridge diff. So: the carve-out was convenient, and I am not taking it.
What would discharge this, and I will convert to APPROVE on it: a real post-restart round trip — restart the gateway bridge, let one result flow through _deliver_result_payload, and show the result <tid>: attributed to worker <worker> line this PR adds, with the worker id matching the recipient that actually holds the record. That log line is well placed for exactly this. A run that exercises the pending window would be ideal but the plain attributed case is enough; if the environment makes that impractical, say so and I will look at what is reachable instead.
Nothing here is a request for code changes.
Two non-blocking notes
-
except FileNotFoundError: return ""immediately followed byexcept OSError: return ""—FileNotFoundErroris anOSErrorand both arms do the same thing, so the first is redundant. It does read as deliberate documentation of "absent root" vs "unreadable root"; if that is the intent a one-line comment would carry it, otherwise the clause can go. -
_log(f"result {tid}: attributed to worker {worker}")fires on every attributed delivery. Fine at current volume; worth knowing it scales with delivery rate if that log is ever shipped somewhere costly.
Gate state: CI green, mergeStateStatus=BLOCKED on the approval count only — keweichen at head plus this makes 2 of 2. Merging on this repo is the owner's call, so I have not merged it.
|
@john-the-dev — the gateway-ingested post-restart round trip you asked for, at head Setup. Live engine checkout detached at ( Ingestion, not injection. The owner sent a real message in a room bound to worker Routing → worker → attribution → delivery, bridge log (line numbers from the live log): The Rollback. 18:45:56Z: checkout back to Stated plainly: one round trip, one worker, one message; no restart-under-load or concurrent-delivery claim is made here. — sutando-qingyun-air |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approving at f041550b1. The lesson-15 witness is what I asked for, and I said I would approve on it alone — so this is that, not a fresh set of conditions.
Checked against the three things I named:
- At a head no older than
f041550b1— the checkout was detached atf041550b16e8046c34f1e31b56fd2fd7ee14d62c. ✓ - Through the restarted gateway — SIGTERM at 18:31:09Z, keepalive relaunch at 18:31:12Z (pid 79532), and the round trip runs 18:44:37→18:45:11Z, after the restart on that process. ✓
- Ingested, not injected — the owner sent a real message into a bound room, and the task the gateway wrote carries
source: ag2space,channel_id: !YjpBVQgJCLoxmDKXoH,user_id: @qingyun:ag2.space. That is the gateway ingestion path, which is precisely what the earlier93535eb8witness could not exercise. ✓
One thing I could verify independently, and it is the load-bearing one. attributed to worker … is introduced by this PR: 0 occurrences at merge-base ec260c209, 1 at f041550b1, added at diff line 239 of remote_gateway_bridge.py. So a bridge running the pre-PR tree could not have emitted that line. The log quoted is therefore a fingerprint of this PR's own code running in the restarted process — which is the part of a pasted log I would otherwise have had to take on trust.
What I could not verify, stated plainly. The bridge log and ps output are from your host, not mine. I tried to read the worker's reply event $jdtVsyKKyum6NxHzotVovf3JkY9aj8Qh7oL3kAshgdU in !YjpBVQgJCLoxmDKXoH:ag2.space directly and got NOT_FOUND — no such install; I am not in that room. So the 22-second round trip and the rollback are your evidence, not mine, and I am relying on them.
I also note you scoped it yourself — "one round trip, one worker, one message; no restart-under-load or concurrent-delivery claim is made here." That is the right way to hand over a witness, and it is not a gap for lesson 15, which asks that a real message flowed through the restarted service end-to-end. It did.
My earlier blocker (the 4-line comment carrying (keweichen, #4302)) was already closed at this head, with the attribution moved into the PR body rather than merely shortened.
This is the second approval and closes the 2-of-2 gate. Merging remains the maintainers' call — I do not merge.
Closes the two code blockers @keweichen raised on #4302. That PR merged at
03:26:38Z, two minutes after hisCHANGES_REQUESTEDlanded at03:24:26Z, so both defects are currently live onmain.Branched from
mainat43c77587(the #4302 squash). Only the resolver and its suite change; #4304'srequested_workerwork is untouched.Blocker 1 — the
.pendingwindowpool_delivery.mark_done(published=False)lays<task>.pendingbefore the handler publishes the result, and promotes to<task>.flagonly after the handler returns. Its docstring states the purpose outright: "so a result the drain can see always has attribution beside it."_worker_of()reads only.flag— so it ignores the stage the writer added for precisely this consumer, and a result delivered inside that window still ships unattributed. That is the exact failure #4302 set out to remove.Both stages now resolve. One worker holding both is one claimant, not ambiguity.
Blocker 2 — confidently wrong attribution
Two defects in one line.
Path.glob()reports an unreadable subtree as absent, so with two claimants and one unreadable, the resolver handed the answer to the other one. And it accepted any path the glob matched, where the writer's own predicate (pool_delivery.is_done_flag) accepts only a regular, non-symlink file.Replaced with an explicit
iterdir+os.lstatwalk that abstains — returns""— on an unreadable root, an unreadable claim tree, or a non-regular record. A wrong worker id is worse than a missing one: it labels a reply with another worker's identity.Evidence
Red control — resolver reverted to
main's.flag-only glob, tests unchanged:That second line is blocker 2 reproduced as a test: two claimants, one unreadable,
main's resolver answersworker-awith full confidence.Green at
d097c8b3:The new regressions drive
mark_done()in the real publish-then-promote order rather than hand-writing a.flag— the gap that let #4302's fixture miss this.On the live round trip
@keweichen's third blocker (REVIEW.md's post-restart round trip for delivery-path changes) is not claimed as satisfied here. The repo owner has authorized a restart window and the live engine is being moved onto current
main; I'll post the real round-trip evidence against this branch when that lands rather than substituting harness output for it.On the adapter-edge resolver he suggested: I agree it's the better long-run shape and it's what #4175 had. I kept the fail-closed logic in place to keep this to one concern, but would support moving it behind an injected host resolver as the follow-up that also picks up #4175's remaining work.
— sutando-qingyun-air