[sonichi]

Closes #5297.

Privacy fix. A reply the owner had durably decided to keep private could still be published to the shared room. It happened in three ways:

• a stale or racing "No" on the same review;
• after an upgrade, a publish_pending record that an older bridge left on disk before a restart;
• after an upgrade, a live record that carried any claim id. The same restart could also tell the owner the opposite of their decision.

What changed, and why

The guard owner is src/policy/egress/result.py. It is vendored byte-for-byte as packages/ag2-sparrow/ag2_sparrow/team_result_guard.py via sync_from_src.py.

• withheld_decision_lock(directory) is main's per-directory _ledger_lock, the same lock that issues ids, writes records and archives them. Every function below holds it, and the bridge never takes a lock file itself.

• update_record(path, record, expect=...) is a compare-and-set. The writer passes the version it read (status, decision_claim_id, generation). Under the ledger lock the write is refused if the record moved on since, and every accepted write bumps generation. update_record and claim_withheld_decision share one writer, _write_owned_record, which also refuses a record that does not own its id or would change its body.

• claim_withheld_decision(path, updates) is a compare-and-set. Under the lock it applies a decision only to a record that is still awaiting_owner and has no archived decision. It first records the claim in a ledger: claims/<review>.<claim_id> is created with O_EXCL and fsynced. Then it writes the record, which names that claim id, through the shared writer and only while the record owns its id. Anything else returns None and writes nothing.

• withheld_claim_publishable(path, claim_id) is true only when all of these hold:

  • the id is 32 lowercase hex characters;
  • its ledger entry exists for this review;
  • the live record is publish_pending/false_positive and carries exactly that id;
  • no archived decision exists for the review.

  A field on the record is never proof by itself, because an older writer could have written it.

• archived_withheld_decision(path) returns the archived decision, which is the authority over any live copy. It returns {} for an archive that exists but cannot be read.

• retire_superseded_record(path) moves a proven live copy into archive/superseded/. A copy is proven only when it holds the same body as its archive, its id is reserved to that body, and the archive is a row of the table below. The archive itself is untouched.

• ARCHIVED_OUTCOMES / archived_outcome(archived) is the one reconcile table, keyed on the archived (status, decision):

  archived status	archived decision	action
  kept_private	sensitive	restore the card to "Kept private", then retire the live copy
  published	false_positive	restore the card to "Published to the original room", then retire the live copy
  publish_failed	false_positive	restore the card to "publication failed and requires attention", then retire the live copy
  any other pair, a non-string field, a non-dict archive or an unreadable archive	—	freeze the live copy in conflicts/ (main's freeze) with its bytes intact; it never acts and the card is untouched

  No row ever publishes. The owner chose to reconcile only proven outcomes; everything else keeps main's freeze.

The bridge (packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py):

• Both Yes and No go through the claim. A reply that loses is closed with its [no-send] control result, logged, and changes nothing.
• _publish_review asks withheld_claim_publishable before anything else. On refusal it does not touch the record and does not POST, and it logs once.
• _defers_to_archive(path) is called by the card-resolution retry and by _resolve_review_card. When an archived decision exists, it applies the reconcile table: only a proven outcome restores the owner's card, and only after that edit is accepted is the live copy retired. Otherwise the copy is frozen in conflicts/ and the archive and card are untouched. Nothing ever writes a state that contradicts the archive.
• Every fault inside _defers_to_archive is contained and logged for that record, including a retirement that raises, such as archive/superseded being a file, a permission error or a full disk. One bad record can no longer abort the poll beat before task intake.
• _route_withheld_review, _publish_review and _resolve_review_card each write with the version they read. A refused write is logged, the record's newer state stands, and the next beat retries from it.
• Publication goes through the outbox. An owner-released result is an outbox item keyed <review>.<claim>. It is built at the existing pinned outbox construction site, _delivery_core, now taking a lane: a DesignAClaimBackend under results/.outbox-withheld-review, with AG2SpaceRoomMessageProvider. The /v1/room POST stays in the bridge (_post_room_message) and is injected into the provider. Both architecture pins (delivery-primitives-single-owner, events-plane-boundary) are unchanged.
  • An explicit refusal (ok: false, error, errcode) wins over any id in the same response and is a definite refusal (publish_failed). Only a non-empty $-prefixed Matrix event id, read through outbox_adapter.classify_response, confirms; an integer or any other string is unconfirmed.
  • The outbox keeps no receipt per item, so the confirmed event id is written to the review record (release_event_id, through the guard's single writer) before the outbox is told DELIVERED. If that write fails, the send is unconfirmed and parks. A DELIVERED item whose record has no event id is logged and left pending, never marked published.
  • An accepted response without a Matrix event id, a 5xx, or a lost reply may have landed. The gateway protocol does not document dedupe_key dedup for /v1/room, so the provider declares idempotent_send=False and the outbox parks the item outcome-unknown. It is never re-sent automatically, and the record stays publish_pending. Auth and rate refusals did not apply and are retried on the outbox's schedule.
  • A parked release is visible. The owner's card shows "publication could not be confirmed and requires attention". The outbox root sits under results/.outbox*, which check_outbox_parked scans. That check now keeps each park's reason: an outcome-unknown park is reported as "may have been delivered — do not requeue without checking the room", and only definite parks as "never delivered". The log names the shell-quoted recovery command: python3 src/outbox_cli.py --root <ws>/results/.outbox-withheld-review requeue <review>.<claim>.
  • Known limit: that requeue re-sends under a new dedupe key (#1), so a release that had in fact landed may post twice.
  • Bound: a crash after the POST lands but before the receipt is written is re-sent once the dead owner's claim is reclaimed. That is the outbox's own dead-owner guarantee, so the release can post twice; the tests pin it at two POSTs. A crash after the receipt write posts once. Every other crash stage posts once.
• A later owner reply to an archived review is answered with the archived outcome and acts on nothing, instead of falling through to chat. If the archive is not a recognised outcome (archived_outcome returns none), the reply is still consumed but answered without stating any outcome.
• Durable moves. Archiving, freezing to conflicts/ and retiring a superseded copy link the new name, sync its directory, and only then unlink the original. A failure leaves the original in place, and it is reconciled or frozen on the next beat.
• Card syncing is moved to a follow-up PR: every beat, re-edit any recent card that doesn't match the authoritative outcome. Until then, cards behave as on main: the card_resolution_pending retry for live records, and the archived-decision restore for a live copy beside its archive. A stale card edit that lands after a newer one can leave a card showing an older outcome; the durable decision itself is unaffected.
• Durable decisions. A decision acts (POST, card edit) only once it is durably on disk.
  • The claim writes the decision with decision_durable: false and flips it to true only after that write's directory sync succeeds.
  • Each beat runs a durability barrier (establish_decision) that re-syncs and marks any undurable decision. withheld_claim_publishable and _resolve_review_card refuse undurable records.
  • A record rename reports success only after its directory is synced, both for decisions and for new review records. The claim ledger is synced before the record names it.
  • A decision whose record write fails before the rename (any exception) gives up its claim, and the owner is told to answer again.
  • A decision whose claim survived a power loss but whose record did not leaves an orphan claim. That claim blocks any later opposite decision. The owner is told whether a publication may already have been sent (only the outbox can prove it was not) and gets the shell-quoted rm command that unlocks the review.
• A recorded event id is proof the release landed. If the record holds release_event_id for the active claim, the beat commits published and never re-POSTs. This covers a crash after the receipt write but before DELIVERED, and a receipt whose directory sync failed after the POST landed.
  • The outbox item is still driven terminal. Each beat, _settle_landed_releases takes any withheld-outbox item not yet DELIVERED whose review, live or archived, holds the receipt for its claim, and drives it to DELIVERED through the outbox's own requeue_item_with_epoch (if PARKED), claim and complete(CONFIRMED).
  • A claim still held, by a live owner or by a dead one inside the 300 s reclaim window, is retried on later beats until it can be taken.
• A claim failure never escapes the beat. Any exception on the claim path, a shared ledger-lock failure included, is answered with a control result asking the owner to reply again.
• A refused card edit is a refusal. _edit_review_card shares the room provider's room_response_refused helper: an explicit refusal wins over any event id, so a refused restore never retires a live copy.
• Known limit. A later reply that matches no live review reads the archive, a cost that grows with the archive.
• Settlement reads the archive first. The archived decision owns its review, so a stale live copy never hides its release receipt. An unreadable record fails closed: the item is not settled until the record can be read.
• Known limit. The settle scan reads every item of the withheld outbox each beat. That is one small JSON read per owner-approved release; the outbox has no terminal index or retention to bound it by.
• A reply that cannot be checked is answered. If the reviews cannot be inspected (ledger lock, issued/, or the archive unreadable), the reply is consumed and answered "please reply again"; it is never written as ordinary work. The guard no longer reads an unreadable archive as absent. An answer that cannot be saved is logged, and the beat goes on.
• An unreadable decision path is unknown, never absent. On Python 3.14, exists(), is_file() and is_dir() return False on EACCES, and glob() reads an unreadable directory as empty, so an unreadable archive let a kept-private body pass the publication gate. main's _owns_its_id has the same shape. Every existence check on the record, archive, issued and claim paths in the guard, and in the bridge's withheld paths, now goes through one strict walk (present(), entries(), read_strict()/read_json_strict()). It lstats each step from the review namespace down, and a symlink or a file anywhere on the way is unknown. Only a genuinely missing entry below a readable directory is absent; links are followed only above the namespace. Every guard and bridge read of a review, archive, claim or reservation file uses the same walk, so the retry beat and the reply path apply one rule. So does every write: a folder is created only when genuinely absent and refused on any link (_ensure_dir), and the shared move verifies the source and the target folder first, so a linked folder never takes a private record outside the namespace. Reads and moves open each directory no-follow relative to the last. A move links and unlinks only through its held directory fds, syncs them by fd, and re-checks that each is still the folder at its path in the namespace, undoing on a rebind. Reads open non-blocking and refuse any non-regular file. Its rollbacks are durable (each undo is fsynced through the held fds before it reports). Without directory fds a move is refused, never made by path. The walk refuses Windows junctions like links. Other writes check the path and then write by path, a stated known limit against a concurrent swapper with state-dir write access. A claims/ or issued/ entry that is not a regular file is unknown. The bridge's reply check and the health check share one read-only preflight, namespace_problems(). Unknown fails closed.
• A review record that is not a record is unknown. An archived, live or frozen file that does not parse, or is not an object, makes the owner's reply unknown: it is answered "please reply again" and never treated as ordinary work. While such a file exists, every owner Yes/No in the DM gets that answer, which names the file; the withheld-reviews-readable health check warns about it.
• Not fixed here. A crash between complete()'s terminal write and its claim release strands that claim. That happens in every gateway outbox, because none calls DeliveryCore.recover(), so it is filed as Gateway outboxes never call DeliveryCore.recover(): a crash between complete()'s terminal write and its claim release strands the claim #5396.
• A reconcile that keeps failing (for example, archive/superseded occupied by a file) stops re-editing the card after three tries per process, and logs the give-up once.
• The poll beat's three retries are now one named function, _retry_withheld_reviews(), and the poll loop calls it. That way the regression drives the production sequence, not a copy of it.
• packages/ag2-sparrow/tests/test_pool_advertisement_push.py: the structural contract now pins that main() calls _retry_withheld_reviews before the advertisement push, and that the wrapper runs the three retries in their original order.

Behaviour changes, all fail-closed:

• A second reply to a decided review is ignored. publish_failed is never claimed or posted.
• A publish_pending written by a pre-fix bridge never publishes, even if the owner really said No before the upgrade. It has no ledger claim. When its review has an archived decision, the card is restored to that decision and the copy is retired.

Rebased onto main. The branch is one commit on main (f85a1d5fb); the reviewed history is at 8acd5e080. #5269 per-body record ledger is kept as is, and this PR's functions run on that ledger's lock and writer. _delivery_core() uses main's gateway_instance.outbox_root() for the result lane.

Look first

1. update_record, _write_owned_record, record_version, _proven_superseded, withheld_decision_lock, claim_withheld_decision, withheld_claim_publishable, archived_withheld_decision and retire_superseded_record in src/policy/egress/result.py
2. _defers_to_archive, _resolve_review_card, _publish_review, _retry_withheld_reviews and the two retry loops in packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py
3. tests/withheld-review-decision-final.test.py

Behaviour this proves

tests/withheld-review-decision-final.test.py runs the canonical src/ writer, injected into the bridge, and asserts that the vendored copy is byte-identical to it. It imports the bridge hermetically: isolated HOME, CLAUDE_CONFIG_DIR and workspace dirs, a dummy token, and AG2_DEVICE_ENV removed. It asserts the bridge took the dummy token, and an open spy with a positive-control sentinel asserts that no host path was opened.

1. The issue's stale-reply schedule, in both directions, with the record live and archived.
2. Publication revalidates the claim on disk, not the caller's snapshot.
3. The full retry beat on the legacy schedule (section 6). A live publish_pending with no claim, as main's writer leaves it, sits beside an archived kept_private, with card_resolution_pending both true and false.
   • When the card cannot be restored, by timeout or by a rejected edit, the record, archive and card are untouched, nothing is posted and the deferral is logged.
   • Once the card can be restored, two beats make exactly one card edit, saying "Kept private". The live copy is retired with its bytes intact, the archive's bytes and mtime are unchanged, and nothing is posted.
4. Each retry defers on its own (section 6b). The card retry reconciles a stale live published record. A direct _resolve_review_card call next to an archived decision never shows "False positive".
5. The ledger matrix (section 7). Each state refuses a Yes claim, a No claim, _publish_review and the full retry beat, and keeps its bytes and mtime. The states are:
   • legacy (no claim id);
   • list-shaped, short, non-hex 32-character, unissued hex32 and issued-for-another-review claim ids;
   • wrong decision, publish_failed, kept_private and published;
   • claimed or awaiting next to an archived kept_private, and next to an unreadable or non-dict archive;
   • malformed JSON, non-dict JSON and missing.
6. The reconcile table, every row (section 6c). The test covers 9 statuses (including missing, int and list) × 4 decisions (including missing and list), plus a non-dict and an unreadable archive. In each case a live legacy copy sits beside the archive and two full retry beats run.
   • For the 3 proven pairs: exactly one card edit with the expected text, the copy is retired with its bytes intact, and the archive is untouched.
   • For every other row: no card edit, the copy is frozen in conflicts/ with its bytes intact, and the archive is untouched.
   • Section 6d pins the split: the same proven archive beside a copy of a different body is frozen, not reconciled.
   • Section 11: a stale router (paused mid-DM while a second router routes and the owner's Yes commits) cannot write awaiting_owner back, so a later No neither claims nor publishes.
   • Section 11b: a stale card resolver's write after the record was published is refused; the published record stands, and the live card retry then shows Published.
   • Section 12: a proven archive beside a same-body copy whose id is reserved to another body gives zero card edits and freezes the copy in conflicts/.
   • Section 13: claim, publication check, archived read, proven check, retire and update each run under the ledger lock, and the writer never runs without it.
   • Nothing posts in any row. The expected outcomes are hard-coded in the test, not read from the module.
7. Liveness through the real main() (section 10). archive/superseded is a regular file, so retirement raises FileExistsError. One real poll iteration still takes in the new task in the same beat. The fault is logged for its record, nothing is logged as unexpected, and the copy and the archive are untouched.
8. Exact claim (section 7b). Both ids are issued for this review, but the caller's is not the one the live record carries, so it is refused.
9. Positive control (section 8). This writer's own claim, interrupted before its POST, is in the ledger and is published exactly once by the retry beat.
10. Delegation (section 9). Each injected guard function returns an answer the real guard would not give for that state. The bridge must act on the injected answer:

• a refused claim on an awaiting record leaves it byte-identical;
• a sentinel claim posts;
• a sentinel archive drives the card edit and the retirement.

A bridge that bypasses the injected owner therefore changes the outcome.
11. A race: 25 rounds of two threads released together. Exactly one decision wins.

Before/after evidence

These blocks were captured on earlier heads of this PR, before main was merged in; the last of them, round 4, is at 60b68ccea. They show the original bug and the round-4 fixes and were not re-run at the current head. Current-head evidence is under Tests run and in the latest round comment.

Every run is under env -i HOME=$HOME PATH=/usr/bin:/bin:/usr/sbin:/sbin:/opt/homebrew/bin with python3 -B, on temp state only.

Full restart beat, Kewei's schedule. The base writer (origin/main 90244ef02, from a temp worktree) archives Yes as kept_private. The stale No then durably leaves publish_pending and stops before its POST:

• edit-ok: its card edit succeeded, which already showed the owner "False positive" from main's own code;
• edit-fails: its card edit failed.

A fresh process then runs one restart beat from each head:

$ seed with origin/main 90244ef02 (card edit during the stale No: edit-ok), then one restart beat with previous head 448b1cac5
{"phase": "seed-main", "checkout": "wt-5297-base", "live": "publish_pending", "live.card_resolution_pending": false, "archive": "kept_private", "posts": [], "owner_card_edits": []}
{"phase": "retry-head", "checkout": "wt-5297-r2", "live": "publish_pending", "live.card_resolution_pending": false, "archive": "kept_private", "posts": [], "owner_card_edits": []}
$ seed with origin/main 90244ef02 (card edit during the stale No: edit-fails), then one restart beat with previous head 448b1cac5
{"phase": "seed-main-no-crash", "checkout": "wt-5297-base", "live": "publish_pending", "live.card_resolution_pending": true, "archive": "kept_private", "posts": [], "owner_card_edits": []}
{"phase": "retry-head", "checkout": "wt-5297-r2", "live": "publish_pending", "live.card_resolution_pending": false, "archive": "kept_private", "posts": [], "owner_card_edits": ["\u2713 False positive recorded; publication to the original room is pending."]}
$ seed with origin/main 90244ef02 (card edit during the stale No: edit-ok), then one restart beat with this head
{"phase": "seed-main", "checkout": "wt-5297-base", "live": "publish_pending", "live.card_resolution_pending": false, "archive": "kept_private", "posts": [], "owner_card_edits": []}
{"phase": "retry-head", "checkout": "wt-5297", "live": null, "live.card_resolution_pending": null, "archive": "kept_private", "posts": [], "owner_card_edits": ["\u2713 Kept private \u2014 the owner confirmed it contains sensitive information."]}
$ seed with origin/main 90244ef02 (card edit during the stale No: edit-fails), then one restart beat with this head
{"phase": "seed-main-no-crash", "checkout": "wt-5297-base", "live": "publish_pending", "live.card_resolution_pending": true, "archive": "kept_private", "posts": [], "owner_card_edits": []}
{"phase": "retry-head", "checkout": "wt-5297", "live": null, "live.card_resolution_pending": null, "archive": "kept_private", "posts": [], "owner_card_edits": ["\u2713 Kept private \u2014 the owner confirmed it contains sensitive information."]}

Round 4: archived-decision variants and a retirement fault, one restart beat. The variants are an archived publish_failed beside a contradictory live kept_private, two inconsistent archived pairs, and archive/superseded occupied by a file:

$ archived-decision variants beside a live copy, then one restart beat — at 5ecc583f8
archive publish_failed/false_positive  card_edits=[] live_present=True live_unchanged=True posts=0
archive kept_private/false_positive    card_edits=['✓ False positive recorded; publication to the original room is pending.'] live_present=False live_unchanged=False posts=0
archive published/sensitive            card_edits=['✓ Kept private — the owner confirmed it contains sensitive information.'] live_present=False live_unchanged=False posts=0
retire-error card_edits=['✓ Kept private — the owner confirmed it contains sensitive information.'] live_present=True raised=FileExistsError

$ same, at this head
archive publish_failed/false_positive  card_edits=['✓ False positive recorded, but publication failed and requires attention.'] live_present=False live_unchanged=False posts=0
archive kept_private/false_positive    card_edits=[] live_present=True live_unchanged=True posts=0
archive published/sensitive            card_edits=[] live_present=True live_unchanged=True posts=0
retire-error card_edits=['✓ Kept private — the owner confirmed it contains sensitive information.'] live_present=True raised=None

Round 4: the exact-head CI failure. The node + out-of-tree python tests job (run 38005986805) failed with AssertionError: _retry_pending_publications is not called in main(). Under env -i:

$ python3 packages/ag2-sparrow/tests/test_pool_advertisement_push.py   # at 5ecc583f8
    assert name in lines, f"{name} is not called in main()"
AssertionError: _retry_pending_publications is not called in main()
exit=1

$ python3 packages/ag2-sparrow/tests/test_pool_advertisement_push.py   # at this head
PASS test_shutdown_holds_the_lock_until_no_publication_can_complete
ALL PASS test_pool_advertisement_push
exit=0

Persisted claim-id matrix. Each case is a live publish_pending/false_positive record with the given claim id, followed by the restart publication retry:

$ persisted publish_pending claim-id matrix, restart retry at 448b1cac5
missing          posts=0
list             posts=0
short            posts=0
nonhex32         posts=1
arbitrary_hex32  posts=1
$ persisted publish_pending claim-id matrix, restart retry at this head
missing          posts=0
list             posts=0
short            posts=0
nonhex32         posts=0
arbitrary_hex32  posts=0

The regression at 60b68ccea (exit 0; 5 more runs passed in a row):

yes-then-stale-no outcome {'Yes': True, 'No': True} | live False | status kept_private | shared-room posts []
yes-live-then-stale-no outcome {'Yes': True, 'No': True} | live True | status kept_private | shared-room posts []
no-then-stale-yes outcome {'No': True, 'Yes': True} | live False | status published | shared-room posts [('!shared:example', 'SECRET-BODY')]
PASS: a durable owner decision is final; stale or racing replies change nothing.

At 448b1cac5 the new test cannot complete, because the beat function it drives does not exist there:

  File "<wt>/tests/withheld-review-decision-final.test.py", line 231, in <module>
    bridge._retry_withheld_reviews()
AttributeError: module 'ag2_sparrow.remote_gateway_bridge' has no attribute '_retry_withheld_reviews'

Mutants at 60b68ccea. Each was applied to src/ or to the bridge and re-vendored with sync_from_src.py, so the copy is in sync and the byte-identity check cannot mask it. Each was run with python3 -B, then restored. All 18 are killed:

### mutant publish_failed_claimable: exit=1 vendored=in-sync FAIL-lines=2
FAIL: publish_failed: claims [{'review_id': 'wr_0000000000000000', 'status': 'kept_private', 'dm_room_id': '!o
### mutant no_status_check: exit=1 vendored=in-sync FAIL-lines=48
FAIL: a stale No on a live kept-private record must not post or flip it
### mutant no_archive_check_in_guard: exit=1 vendored=in-sync FAIL-lines=8
FAIL: claimed-but-archived-kept_private: claims [None, None], publish True, 1 posts
### mutant any_string_claim_id: exit=1 vendored=in-sync FAIL-lines=3
FAIL: a different issued claim id is not the live record's release claim
### mutant no_claim_ledger_check: exit=1 vendored=in-sync FAIL-lines=4
FAIL: unissued-hex32-claim-publish_pending: claims [None, None], publish True, 1 posts
### mutant ledger_not_written: exit=1 vendored=in-sync FAIL-lines=16
FAIL: a stale Yes after publication must not rewrite the published decision
### mutant table_drops_publish_failed: exit=1 vendored=in-sync FAIL-lines=2
FAIL: table publish_failed/false_positive: outcome publish_failed
### mutant table_trusts_status_only: exit=1 vendored=in-sync FAIL-lines=18
FAIL: table kept_private/false_positive: outcome None
### mutant table_accepts_any_dict: exit=1 vendored=in-sync FAIL-lines=2
FAIL: table non-dict: outcome None
### mutant bridge_reconciles_any_archive: exit=1 vendored=in-sync FAIL-lines=33
FAIL: table kept_private/false_positive: unproven archive leaves card and copy untouched: ['**Private result r
### mutant retire_error_escapes: exit=1 vendored=in-sync FAIL-lines=2
FAIL: the beat whose retirement raised still took in the new task: []
### mutant bridge_claim_bypasses_injected_owner: exit=1 vendored=in-sync FAIL-lines=1
FAIL: the bridge's decision is the injected guard's claim answer (refused: nothing written)
### mutant bridge_publish_gate_bypasses_injected_owner: exit=1 vendored=in-sync FAIL-lines=1
FAIL: the bridge's publication gate is the injected guard's answer
### mutant bridge_archive_bypasses_injected_owner: exit=1 vendored=in-sync FAIL-lines=1
FAIL: the bridge's archive authority and retirement are the injected guard's
### mutant card_retry_ignores_archive: exit=1 vendored=in-sync FAIL-lines=4
FAIL: legacy pending=False: the deferral is logged for the owner
### mutant resolve_card_ignores_archive: exit=1 vendored=in-sync FAIL-lines=2
FAIL: a card resolution beside an archived decision reports nothing resolved
### mutant retire_without_card_restore: exit=1 vendored=in-sync FAIL-lines=14
FAIL: legacy pending=True, card unreachable: record, archive and card untouched
### mutant beat_skips_card_retry: exit=1 vendored=in-sync FAIL-lines=11
FAIL: legacy pending=True: the deferral is logged for the owner

• table_drops_publish_failed, table_trusts_status_only, table_accepts_any_dict, bridge_reconciles_any_archive and retire_error_escapes are new this round.
• retire_error_escapes catches only TimeoutError, so a filesystem error escapes. It is killed by the main() liveness section.

Tests run

Committed suites (run them at 8acd5e080, on Python 3.9, 3.12 and 3.14):

• tests/withheld-review-decision-final.test.py drives the production functions through every schedule raised in review, including:
  • the three receipt-crash schedules (claim reclaimable, held by a live owner, owner dead inside the reclaim window), each ending DELIVERED with one POST after the review was archived;
  • the receipt-sync PARKED case, settled DELIVERED;
  • a real ledger-lock failure, made by revoking the lock file after the reply is matched;
  • a card edit refused with an event id;
  • the unlock command's shell quoting under an Application Support path;
  • an archived receipt settling despite a restored stale live copy, with the card edit refused (§25m), and an unreadable archive failing closed (§25n);
  • settlement calling only the outbox's claim and complete, with no POST (§25o);
  • a reply arriving while the reviews cannot be read: the lock, issued/, the archive/ directory, or an archived record, each made unreadable before the match. Each reply is consumed and answered, and the next one applies (§25p);
  • an answer that cannot be saved is logged, never raised (§25q);
  • an unreadable archive beside a kept-private decision, emulating 3.14's reading on any Python and with a real mode 000: no POST, no card edit, nothing raised (§25r);
  • a per-site contract for every converted existence check (§25s);
  • a broken namespace, through the production paths: archive/ a file, a dangling archive record, archive/ a dangling link, the record a directory, and issued/ a file. Each makes zero POSTs and never marks the review published (§25u). Also conflicts/ as a file at the match, a dangling archive record at settlement (§25n2), and the absence rule itself (§25v);
  • linked claims/, linked issued/ and a linked live record, through publishing, the retry beat and a reply; a read below a linked folder; a leaf swapped for a link before the open; a linked state root with no reviews (a plain Yes/No stays ordinary chat) versus a linked namespace (unknown);
  • a linked archive/superseded/ or conflicts/ through the real retry beat (the record stays, nothing is written outside), a final .issued link (the reply is answered), a move refusing a linked source, a first review under a linked namespace, and a directory or the namespace root swapped for a link between the walk and the open;
  • a directory or FIFO entry in claims/ or issued/ (through the bridge, the guard and the health check); superseded/ swapped for a link right after it is verified, during a real retry beat; and the suppression and quarantine journals refusing a linked suppressed-team-results/;
  • the withheld-reviews-readable health check, which reports the guard's shared preflight (tests/health-check-outbox-parked.test.py);
• Committed mutation controls: python3 tests/mutation/5300/run.py runs the 30 committed controls (controls.json) in a git archive export of HEAD. It needs a passing baseline, disables bytecode, counts syntax or import failures as INVALID, and exits 0 only when all are killed. At 37ddc73a4: 30/30 killed on Python 3.14.6 (local runs are 3.14 only now; CI covers the other versions).
  • {not json, [], "str" and an empty file, each as an archived, live and frozen record (§25t).
• tests/health-check-outbox-parked.test.py, withheld-review-dm, gateway-disposal-delegation, and both architecture gates also pass.
• The vendored team_result_guard.py is byte-identical to src/policy/egress/result.py.
• Changed-line coverage: see the CI check diff coverage >= 95% (python) at the current head.

Author-local evidence, not reproducible from committed artifacts:

• Earlier rounds' mutation controls: author-local only, not committed. 106 were killed at 178acbe7f. They are listed in the round comments and are not claimed as evidence here. each applied to src/ and the vendored copy together. Each was run against withheld-review-decision-final, gateway-disposal-delegation, withheld-review-dm and health-check-outbox-parked, under env -i with PYTHONDONTWRITEBYTECODE=1 and every __pycache__ cleared before each run.
  • An unmutated baseline passed first.
  • A mutant counted as killed only on a non-zero exit that was not a syntax, indentation or import error.
• Full discovery (at 8acd5e080): scripts/discover-python-tests.sh, one file at a time under env -i. 4 of the 1159 files compile Swift probes and were left out (no local builds on that host; CI skips them without swiftc). Of the other 1155, 1149 passed, and the 6 failures fail identically at the merge base c5bae26c8 on that host.
• Treat both as a local cross-check, not evidence; CI is the authority.

Local runs from here on: only the focused suites on Python 3.14, plus mutation controls for new or changed code. Full discovery and the Python 3.9/3.12 matrix are left to CI at each pushed head.

Live path

This touches the gateway bridge's owner-review and retry path. The real post-restart round trip has not been run at any head of this PR, including the current one, and is required before approval. It will be posted separately.

Docs

N/A. This is not a user-visible feature change: a reply to an already-decided review was always meant to change nothing, and the card is always meant to show the decision.

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

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

Diff Coverage

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

• packages/ag2-sparrow/ag2_sparrow/delivery_core/provider_ag2space.py (100%)
• packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py (96.4%): Missing lines 939-942,1152-1153,1180,1198-1199,1458-1459
• src/health-check.py (93.1%): Missing lines 9766-9767
• src/policy/egress/result.py (97.3%): Missing lines 549,567,635,643,1033-1034,1036,1038,1064

Summary

• Total: 707 lines
• Missing: 22 lines
• Coverage: 96%

packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py

Lines 935-946

  935 
  936 def _review_paths(directory: Path, strict: bool) -> list[Path]:
  937     try:
  938         return team_result_guard.entries(directory, "wr_*.json")
! 939     except OSError as exc:
! 940         if strict:
! 941             raise _ReviewsUnreadable(exc) from exc
! 942         return []
  943 
  944 
  945 def _check_reviews_readable(directory: Path) -> None:
  946     # The guard's one read-only preflight, under the ledger lock so no move races it: any entry

---

Lines 1148-1157

  1148                     _REFUSED_REVIEW_PUBLICATIONS:
  1149                 _REFUSED_REVIEW_PUBLICATIONS.add(f"settle:{item_id}")
  1150                 _log(f"withheld review {review_id}: published; its outbox item is still claimed "
  1151                      "and is settled once that claim can be taken")
! 1152         except Exception as exc:  # noqa: BLE001 — next beat retries
! 1153             _log(f"withheld review {review_id}: outbox settlement deferred ({exc})")
  1154 
  1155 
  1156 def _settlement_record(directory: Path, review_id: str) -> "dict | None":
  1157     """The archived decision owns its review, so it is read first and a live copy never hides

---

Lines 1176-1184

  1176     for _attempt in range(5):
  1177         current = _read_review_json(path)
  1178         if current is not None or not team_result_guard.present(path):
  1179             return current or {}
! 1180     return {}
  1181 
  1182 
  1183 def _record_publish_failure(path: Path, record: dict, version: dict, reason) -> bool:
  1184     record.update({"status": "publish_failed", "publish_error": str(reason),

---

Lines 1194-1203

  1194     record.update({"status": "published", "published_at": time.time(),
  1195                    "published_event_id": event_id, "card_resolution_pending": True})
  1196     if team_result_guard.update_record(path, record, expect=version):
  1197         return True
! 1198     _stale_review_write(path, "publication")
! 1199     return False
  1200 
  1201 
  1202 def _resolved_review_body(record: dict) -> str:
  1203     rid = str(record.get("review_id") or "")

---

Lines 1454-1463

  1454         except Exception as exc:  # noqa: BLE001 — next poll retries
  1455             _log(f"withheld review {record.get('review_id')} publish retry failed: {exc}")
  1456     try:
  1457         _settle_landed_releases()
! 1458     except Exception as exc:  # noqa: BLE001 — next poll retries
! 1459         _log(f"withheld release settlement deferred: {exc}")
  1460 
  1461 
  1462 def _retry_review_card_resolutions() -> None:
  1463     for path, record in _pending_review_records():

---

src/health-check.py

Lines 9762-9771

  9762     name = "withheld-reviews-readable"
  9763     workspace = Path(workspace_dir or WORKSPACE_DIR)
  9764     try:
  9765         from policy.egress import result as guard  # noqa: PLC0415 - the bridge's own strict rule
! 9766     except ImportError as exc:
! 9767         return {"name": name, "status": "warn", "detail": f"cannot load the review guard ({exc})"}
  9768     base = workspace / "state" / guard.WITHHELD_RESULT_DIR
  9769     bad = []
  9770     for path, reason in guard.namespace_problems(base):
  9771         shown = path.relative_to(workspace) if path.is_relative_to(workspace) else path.name

---

src/policy/egress/result.py

Lines 545-553

  545     """The held directory is still the one at its path in the namespace: no link on the way,
  546     and the same device and inode as the fd. A rename or rebind after the open fails this."""
  547     try:
  548         if not present(directory, stat.S_IFDIR):
! 549             return False
  550         held, there = os.fstat(fd), os.lstat(directory)
  551     except OSError:
  552         return False  # unknown is not "still there"
  553     return (held.st_dev, held.st_ino) == (there.st_dev, there.st_ino)

---

Lines 563-571

  563             continue
  564         if not os.access(ancestor, os.R_OK | os.X_OK):
  565             raise OSError(f"{path}: {ancestor} is not a readable directory")
  566         return
! 567     raise OSError(f"{path}: no existing ancestor")
  568 
  569 
  570 _VERSION_KEYS = ("status", "decision_claim_id", "generation")

---

Lines 631-639

  631     path first, so a concurrent swapper with state-dir write access could redirect those."""
  632     if not present(directory, stat.S_IFDIR):
  633         directory.mkdir(mode=0o700, parents=True, exist_ok=True)
  634         if not present(directory, stat.S_IFDIR):
! 635             raise FileNotFoundError(2, "No such file or directory", str(directory))
  636     return directory
  637 
  638 
  639 def _durable_move(source: Path, target: Path) -> None:

---

Lines 639-647

  639 def _durable_move(source: Path, target: Path) -> None:
  640     """Move a record so its only copy is never deleted before the new one is durable:
  641     link, sync the target directory, then unlink and sync the source directory."""
  642     if not present(source, stat.S_IFREG):                # a real file, reached through no link
! 643         raise FileNotFoundError(2, "No such file or directory", str(source))
  644     _ensure_dir(target.parent)
  645     if not hasattr(os, "O_DIRECTORY"):                   # no fd confinement here: refuse, never by path
  646         raise OSError(f"{source}: a confined move needs directory fds; the record stays")
  647     source_fd = _open_dir(source.parent)

---

Lines 1029-1042

  1029     path = Path(path)
  1030     with _ledger_lock(_record_directory(path)):
  1031         try:
  1032             current = read_json_strict(path)
! 1033         except (OSError, ValueError):
! 1034             return False
  1035         if not isinstance(current, dict):
! 1036             return False
  1037         if decision_is_durable(current):
! 1038             return True
  1039         try:
  1040             _fsync_directory(path.parent)
  1041         except OSError:
  1042             return False

---

Lines 1060-1068

  1060             current = read_json_strict(path)
  1061         except (OSError, ValueError):
  1062             return False
  1063         if not isinstance(current, dict) or current.get("decision_claim_id") != claim_id:
! 1064             return False
  1065         if current.get("status") == "published":
  1066             return True  # already settled by this claim's release
  1067         if current.get("status") != "publish_pending":
  1068             return False

---

⚠️ Not measured

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

• packages/ag2-sparrow/tests/test_pool_advertisement_push.py

[keweichen]

Reviewed the current open, non-draft head 95820ef20e1ae126d6892aeee02a4e5d38f6b005. Requesting changes because the privacy invariant is still breakable across a crash/upgrade boundary.

Blocker — a legacy publish_pending can still disclose the kept-private reply after restart

The new CAS protects decisions made by the new writer, but it does not leave any claim identity/generation that the publisher can validate. _retry_pending_publications feeds every live publish_pending record into _publish_review, which revalidates only that status before POSTing the body.

I reproduced this with the production functions at the exact base and head: base archived a Yes decision as kept_private, a stale No writer durably left the live record as publish_pending, and the process was stopped before POST. Starting the head's retry path then produced:

{"phase": "seed-main", "live": "publish_pending", "archive": "kept_private", "posts": []}
{"phase": "retry-head", "live": "published", "archive": "kept_private", "posts": ["SECRET-BODY"]}

Please bind publication to the exact claim produced by the new CAS (or an equivalent unambiguous generation/provenance), and fail closed for legacy/ambiguous pending state—especially when the archive says kept_private. The regression needs a base-writer → crash → head-retry case, not only two writers running the new code.

Blocking test/CI contract gaps

• The required diff-coverage check is red at 42%. The bridge file is covered, but the canonical src/policy/egress/result.py writer is only 8.3% covered (missing lines 406–422 and 424–428) because the regression executes only the vendored copy. Exercise the canonical writer, inject it into the bridge test, retain the vendoring-drift check, and pin adapter delegation with a spy/sentinel.
• The new test imports the bridge before creating its temp root. Under the PR body's advertised env -i HOME=... PATH=... command, that import reached the host token-vault fallback. Set an isolated CLAUDE_CONFIG_DIR and a dummy REMOTE_TASK_TOKEN before import. The current hermetic-test lint reports zero scanned tests for this bridge name, so its green result does not cover this case.
• The stated adjacent-state invariant is under-tested: a mutant that allowed publish_failed to be claimed passed the entire new regression. Enumerate publish_pending, publish_failed, kept_private, published, and malformed/missing state and assert both refusal and byte/mtime preservation.

Remaining live-path gate

The PR body correctly says the required real post-restart bridge round trip has not yet been run. After the implementation and regression above are fixed, that live witness is still required before approval; the fake-relay harness is useful but does not exercise the affected delivery loop.

Positive evidence: the new regression fails on the parent (31 failed checks) and passes at this head; 20 repeated runs covered 500 race rounds; focused guard/DM/vendoring suites, review-checks.sh, and git diff --check passed. The overlap with #5269 is correctly disclosed; after this focused fix is sound, #5269 should be rebased deliberately so its ledger lock remains the single lock owner.

[john-the-dev]

Reviewed head 95820ef20e1ae126d6892aeee02a4e5d38f6b005. REQUEST_CHANGES. The core claim is correct, and I verified it independently below. Two of @keweichen's blockers (review 5476224486) reproduce for me as well. I'm adding evidence on those rather than restating them.

Blockers (independently reproduced)

1. A legacy publish_pending still publishes after the upgrade. I seeded the state a base-version stale "No" leaves behind: the archive holds kept_private and a live publish_pending record has been rewritten at the original path. That writer is _atomic_private_json(path, record) on main, which recreates the file after the archive move. Then I ran head's _retry_pending_publications() with a fake relay:

legacy publish_pending + archived kept_private -> head retry posts: ['SECRET']

_publish_review revalidates only status == "publish_pending". Nothing ties that status to a claim made by the new CAS, so state written before the upgrade is trusted. At minimum, fail closed when archive/<same name> exists with kept_private. Better, stamp the claim (for example a claim_id/generation written by claim_withheld_decision) and post only when it is present. Add a regression for "base writer -> crash -> head retry".

2. The required diff coverage >= 95% (python) check is red at this head. Run 38002270669, job 114063724299, attempt 1, headSha 95820ef2:

packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py (100%)
src/policy/egress/result.py (8.3%): Missing lines 406-422,424-428
Coverage: 42%

The regression imports only the vendored ag2_sparrow.team_result_guard, so the canonical src/ writer never runs. Unit-test claim_withheld_decision from src/policy/egress/result.py directly. That should cover refusal on each non-awaiting_owner status, refusal on missing or malformed records, no write on refusal, and the winning write.

Test gap (non-blocking alone, but fold it into the fix)

A mutant that also lets publish_failed be claimed (status not in ("awaiting_owner", "publish_failed")) passes the full regression (rc=0). The new behaviour, "a second reply on publish_pending/publish_failed is ignored", is stated in the PR body but not pinned. On the host-config point: under the PR's env -i command the bridge import did not load a token on my host. In an ordinary shell, though, importing remote_gateway_bridge printed token not in env; loaded from <host>/channels/ag2space/.env. The test therefore depends on the caller's environment for isolation. Setting a dummy REMOTE_TASK_TOKEN and an isolated CLAUDE_CONFIG_DIR before the import fixes that.

What I verified holds

• Regression: fails on 90244ef0 (rc=1, 31 FAIL) and passes at head (rc=0, twice). sync_from_src.py --check reports in sync. review-checks.sh --diff on gh pr diff 5300 passes. The team-result-guard, sparrow-result-guard, withheld-review-dm, gateway-owner-mention, team-guard-suppress-journaled-silent-close, collaborator-attachment-gate, ag2-sparrow-drift and restructure-compat-shims suites all pass at head.
• My mutants, each killed by the regression:
  • lock replaced with nullcontext: 25 FAIL (all race rounds, 3/3 runs);
  • awaiting_owner check removed: 26 FAIL;
  • _publish_review on-disk re-check removed: 1 FAIL.
• Lock scope across processes: locked_file is fcntl.flock on .decision.lock, so it serialises separate processes as well as threads. I tested two spawned processes claiming Yes/No on the same record with a synchronised start. Over 40 rounds, 0 bad and exactly one winner each time (23 Yes, 17 No). With the lock mutated out, 40/40 rounds had both winning. This matters because the poll loop is single-threaded (_handle_review_decision runs inline at L5195), so in production the race needs two pollers, for example a stale-singleton takeover. A thread lock would not have covered that.
• Crash windows (separate processes):
  • claim(No) then process exit: the restart retry posts once, then never again;
  • claim(Yes) then exit: retries never post, and the card retry archives;
  • stale "No" against a live kept_private and against an archived one: no post, no flip, and the file is not recreated at the live path.
• TOCTOU between the publish re-check and the POST: no writer can turn publish_pending back toward private. The CAS transitions only out of awaiting_owner. The remaining unlocked writers (_resolve_review_card, _publish_review) re-write their own kept_private/publish_pending/published snapshot and cannot manufacture publish_pending over kept_private. So within head-written state the re-check cannot be invalidated. The only hole I found is #1, which is state written outside the CAS.
• Other publish paths: withheld_body is read only in remote_gateway_bridge.py (_publish_review via _released_review_body) and written by the guard. The only callers of _publish_review are _handle_review_decision and _retry_pending_publications. No path bypasses the claim except the legacy-state case above.

Not run by me: the live post-restart round trip, which the PR body already says is outstanding.

[sonichi]

@keweichen @john-the-dev, thank you both. Your two reviews (Kewei, Rui) found the same blocker independently. This comment answers both at head 448b1cac5efec96b5041b3ebd0293b66a185c5d5.

1. Legacy publish_pending published after restart (P1, both reviews). Fixed.

• claim_withheld_decision now writes a random 128-bit decision_claim_id atomically with the decision.
• _publish_review first asks the guard's withheld_claim_publishable(path, claim_id). Under the same lock, it allows the POST only when the live record is publish_pending/false_positive and carries that exact claim id. It never allows it when an archived decision exists for the review.
• Legacy, ambiguous or malformed pending state is refused. So are publish_failed, kept_private, published and missing records. Nothing is written and the refusal is logged once per review.
• The trade-off: a pre-fix publish_pending from a real "No" also stays private. That is fail-closed by design, and it is stated in the body.

Your schedule, run as you ran it: the base writer from a temp worktree at 90244ef02 writes a durable publish_pending and stops before the POST, then a fresh process runs the head's retry path.

$ seed with origin/main (90244ef02), then restart retry with round-1 head 95820ef20
{"phase": "seed-main", "checkout": "wt-5297-base", "live": "publish_pending", "archive": "kept_private", "posts": []}
{"phase": "retry-head", "checkout": "wt-5297-r1", "live": null, "archive": "published", "posts": ["SECRET-BODY"]}
$ seed with origin/main (90244ef02), then restart retry with this head
{"phase": "seed-main", "checkout": "wt-5297-base", "live": "publish_pending", "archive": "kept_private", "posts": []}
{"phase": "retry-head", "checkout": "wt-5297", "live": "publish_pending", "archive": "kept_private", "posts": []}

The committed regression (section 6) pins the same state through the head's retry loop. It asserts no POST, byte/mtime preservation, the archive still kept_private, and the refusal logged.

2. Diff coverage at 42% (both). Fixed.

• The test now imports the canonical src/policy/egress/result.py and injects it into the bridge as team_result_guard. Spies on claim_withheld_decision and withheld_claim_publishable assert the bridge delegates to it.
• A byte-identity assertion against the vendored copy keeps the drift check.
• Locally, with coverage run --rcfile=.coveragerc, no changed line in result.py (403–450) or in the bridge hunks is missing. CI's diff-coverage job is the authority, and its run at this head is pending.

3. Hermetic import (Kewei; Rui's channel .env observation). Fixed.

• Before the import, the test sets an isolated HOME, CLAUDE_CONFIG_DIR and AGENT_CONNECT_{TASK,RESULT,STATE}_DIR, sets a dummy REMOTE_TASK_TOKEN, and pops AG2_DEVICE_ENV/AG2_REMOTE_TOKEN. That leaves the channel-.env lookup and the vault tier unreached.
• It asserts bridge.TOKEN is the dummy.
• A spy on open/io.open/os.open asserts nothing under the real home (outside repo and sandbox) was opened. A sentinel open checks first that the spy fires.
• scripts/lint-hermetic-bridge-tests.py reports ok.

4. Adjacent states / the publish_failed mutant (both). Pinned.

• Section 7 enumerates these states: legacy publish_pending, malformed claim id, wrong decision, publish_failed, kept_private, published, claimed-but-archived kept_private, awaiting-but-archived, malformed JSON, non-dict JSON and missing.
• Each must refuse a Yes claim, a No claim, _publish_review and the retry loop, and keep its bytes and mtime.
• Section 8 is the positive control: this writer's own interrupted claim still posts exactly once.

Mutants, each re-vendored so the byte-identity check cannot mask them, then restored:

### mutant publish_failed_claimable: exit=1 vendored=in-sync FAIL-lines=2
FAIL: publish_failed: claims [{'review_id': 'wr_0000000000000000', 'status': 'kept_private', 'dm_room_id': '!owner-dm:example', 'owner': '@owner:examp
FAIL: publish_failed: record bytes or mtime changed
### mutant no_status_check: exit=1 vendored=in-sync FAIL-lines=39
FAIL: a stale No on a live kept-private record must not post or flip it
FAIL: a legacy publish_pending beside kept_private must not post
FAIL: legacy-publish_pending: claims [{'review_id': 'wr_0000000000000000', 'status': 'kept_private', 'dm_room_id': '!owner-dm:example', 'owner': '@own
### mutant no_archive_check: exit=1 vendored=in-sync FAIL-lines=4
FAIL: claimed-but-archived-kept_private: claims [None, None], publish True, 1 posts
FAIL: claimed-but-archived-kept_private: record bytes or mtime changed
FAIL: awaiting-but-archived-kept_private: claims [{'review_id': 'wr_0000000000000000', 'status': 'kept_private', 'dm_room_id': '!owner-dm:example', 'o
### mutant no_claim_id_match: exit=1 vendored=in-sync FAIL-lines=4
FAIL: legacy-publish_pending: claims [None, None], publish True, 1 posts
FAIL: legacy-publish_pending: record bytes or mtime changed
FAIL: malformed-claim-publish_pending: claims [None, None], publish True, 1 posts
### mutant bridge_status_only: exit=1 vendored=in-sync FAIL-lines=12
FAIL: a legacy publish_pending beside kept_private must not post
FAIL: the refused legacy record is left byte-for-byte
FAIL: the refused publication is logged for the owner

bridge_status_only reinstates round 1's status-only re-check, so it is the exact hole you found.

Lock ownership. The lock is one named function in the guard owner, withheld_decision_lock(directory). The bridge never opens a lock file itself. As Kewei asked, this PR lands first, then #5269 rebases. On that rebase the ledger lock subsumes withheld_decision_lock, so there is a single lock owner for guard records. Rui, your cross-process result (flock, 40/40 with exactly one winner) is unchanged by this round, because claim and publish check share that one lock.

Tests at this head, under env -i:

• The new test passes, and passed 5 more runs in a row.
• 105 of the 109 related files pass. The other 4 fail identically at 90244ef02 for host reasons, listed in the body.
• The sync check, review-checks.sh and git diff --check are all clean.

The live post-restart witness is still outstanding. It will run once at the approved head.

[john-the-dev]

Re-reviewed head 448b1cac5efec96b5041b3ebd0293b66a185c5d5. Both of my blockers from review 5476234834 (at 95820ef2) are fixed, and I've verified each one below. I'm dismissing that CR. I'm not approving yet: the next step is Rui's Codex-core verification at this exact head. @keweichen's remaining gate (the live post-restart bridge round trip) is also still outstanding, as the PR says.

Blocker 1 (legacy publish_pending publishes) — fixed

I re-ran my own probe. It seeds the state a base-version stale "No" leaves behind: archive kept_private, live publish_pending with no claim id. Then it runs the head's _retry_pending_publications() against a fake relay:

95820ef2: legacy publish_pending + archived kept_private -> head retry posts: ['SECRET']
448b1cac: legacy publish_pending + archived kept_private -> head retry posts: []

Variations at 448b1cac, each followed by two retry passes of publish and card resolution:

A archive kp + live pp, no claim id                 posts=[] live=publish_pending
B archive kp + live pp, forged 32-hex claim         posts=[] live=publish_pending
C live pp, claim id wrong length                    posts=[] live=publish_pending
D live pp, claim id non-str                         posts=[] live=publish_pending
E live publish_failed + claim id                    posts=[] (archived by card retry)
F archived awaiting_owner, then a No claim          claim=None, posts=[]
G archived awaiting_owner + live pp with claim id   posts=[]
H genuine No claimed, crash before POST             posts=['SECRET'] once (positive control)

In the real bridge, every writer of publish_pending goes through claim_withheld_decision. The only other unlocked writers are _resolve_review_card and _publish_review. They write back their own snapshot, and they never produce publish_pending over a decided record. When a delayed card retry re-creates a live file after the record was archived, the archive check in _live_undecided_record refuses it. The check-then-POST window is safe for the same reason as last round: nothing transitions a record out of publish_pending toward private.

Blocker 2 (required diff-coverage red) — fixed

The diff coverage >= 95% (python) check passes at this head. That is run 38004012129, job 114069477794, attempt 1, headSha 448b1cac:

packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py (100%)
src/policy/egress/result.py (100%)
Coverage: 100%

Regression and mutants

The new test passes at head: rc=0 on 4 runs under env -i, plus 1 run in an ordinary shell. Against 95820ef2 and 90244ef0 it fails at import. Its delegation spies need withheld_claim_publishable and claim_withheld_decision, which do not exist there. So the before/after evidence for the legacy case is the probe output above, not the test's own FAIL lines.

I applied each mutant to both the canonical src copy and the vendored copy, cleared __pycache__ between runs, and ran the full regression:

no_claim_id_match          rc=1  FAIL-lines=5   (legacy-publish_pending posts)
publish_failed_claimable   rc=1  FAIL-lines=3
no_archive_check           rc=1  FAIL-lines=5
no_decision_check          rc=1  FAIL-lines=3
no_claim_stamp             rc=1  FAIL-lines=1
bridge_skip_guard          rc=1  FAIL-lines=28
bridge_status_only         rc=1  FAIL-lines=13  (round 1's hole)
claim_id_len_only          rc=0  SURVIVES (see nit 1)

Other checks

• src/policy/egress/result.py is byte-identical to packages/ag2-sparrow/ag2_sparrow/team_result_guard.py, and sync_from_src.py --check reports in sync.
• review-checks.sh --diff on the full gh pr diff 5300 passes, run from the repo at this head.
• team-result-guard, withheld-review-dm, sparrow-result-guard and ag2-sparrow-drift all pass at head.
• gh pr checks shows every check pass, including refuse bridge tests that read host config.
• The hermetic import holds. In an ordinary shell, which reproduced the leak last round, the test now asserts bridge.TOKEN == "dummy-test-token" and no host-path opens, and it passes.

Nits (non-blocking)

1. The claim-id equality is not what any test pins. Mutant claim_id_len_only changes the last clause to "the record has any string decision_claim_id" and passes the whole suite. That's because the bridge always passes the claim id from its own fresh read of the same record. Under this state machine, the binding is in effect a provenance stamp ("written by the new CAS"), not a capability the caller has to present. I found no reachable path where the two differ, so the fix holds either way. Either state that in the docstring, or add a case that passes a stale snapshot whose claim id differs from the one on disk.
2. decision_claim_id is also stamped on Yes (kept_private) claims. So a hypothetical writer that copied a post-Yes record and changed status and decision would carry a valid-looking id. My probe "I" shows that publishes. No head writer does this, and base's stale-No writer snapshots before the claim, so it has no id (case A). Stamping the id only on release claims would close the class anyway.
3. github-evidence.py --expect-head 448b1cac reported "Required-check evidence malformed; readiness is unknown". That's a tooling limitation on my side, not a finding against the PR. I relied on gh pr checks and the check-run head_sha instead.

Not run by me: the live post-restart round trip.

Agent-assisted review by Rui's Sutando (Claude); no personal human inspection claimed.

Blockers verified fixed at 448b1ca (evidence in my COMMENTED review on that head); approval pending Codex-core verification.

[keweichen]

REQUEST_CHANGES — this is not merge-ready at 448b1cac5efec96b5041b3ebd0293b66a185c5d5.

I verified that this head fixes the originally reported stale Yes/No disclosure: the base writer reproduces the leak, the current publication retry refuses that legacy state, the isolated regression passes, the canonical/vendored guards are in sync, and current CI plus CLA are green. The full activated path still has two fail-closed defects, though.

1. Blocking: the next step in the real poll loop rewrites the refused record and tells the owner the opposite decision. The committed legacy regression stops after _retry_pending_publications() (test lines 226–242), but production immediately runs _retry_review_card_resolutions() next (bridge lines 5174–5175). That retry has no archive/claim guard (lines 1095–1104).

   I seeded the exact base writer's kept_private archive + crash-left live legacy publish_pending, then ran both production retries from this head in a fresh process. The result was:

   publication_retry_unchanged=true
   full_poll_unchanged=false
   archive=kept_private
   live=publish_pending
   live.card_resolution_pending=false
   posts=[]
   

   The second retry issued an owner-card edit saying False positive recorded; publication to the original room is pending, then _resolve_review_card rewrote the live record (lines 964–995). The archive remains authoritative, so every later owner claim is refused (guard lines 409–438); the owner is shown the wrong resolution and cannot act, while the contradictory live record remains indefinitely. Please exercise the complete poll sequence and keep the archived decision authoritative in storage and presentation.

2. Blocking: a matching 32-character field is treated as proof that this CAS issued the claim. Restart retry reads decision_claim_id from the live record and passes it back to _publish_review (lines 1084–1092); the guard then checks only length and equality with that same record (guard lines 420–440). My persisted-state retry matrix produced:

   missing          posts=0 status=publish_pending
   list             posts=0 status=publish_pending
   short            posts=0 status=publish_pending
   nonhex32         posts=1 status=published
   arbitrary_hex32  posts=1 status=published
   canonical_claim  posts=1 status=published
   

   Thus a malformed non-hex value and an arbitrary 128-bit-looking value are indistinguishable from a claim minted by claim_withheld_decision, and both publish the withheld body. The adjacent-state table omits this matching publish_pending/false_positive case (test lines 244–286). If the contract is “only a claim this CAS wrote,” it needs independently verifiable durable provenance; equality to the same field is tautological. At minimum, malformed values must fail closed, and the test should distinguish an issued claim from a well-shaped foreign one.

3. Approval gate: the real post-restart gateway round trip is still absent. The PR body explicitly leaves it outstanding. This changes a bridge delivery/retry path, so REVIEW.md lesson 15 and CONTRIBUTING.md require a real input → restarted service → delivered reply witness at this exact head before approval; the fake-relay fresh-process probe is not that witness.

One additional blocking test-contract gap: the claimed claim_withheld_decision bridge-delegation spy is not discriminating. Direct guard calls at lines 280 and 292 increment the same counter that is only checked as > 0 at line 332. I mutated _claim_review_decision to bypass the injected canonical owner and call the bundled module directly; the full regression still exited 0/PASS. Pin a bridge-specific call/delta (or add the structural delegation check the architecture contract requires).

Worst-case disruption at this head: an upgraded host can overwrite the owner's visible keep-private resolution with a false-positive/pending result that the owner can no longer resolve; a malformed or foreign-looking persisted claim can publish the withheld body. Please fix those cases, add a full-loop regression, and attach the exact-head live witness; I will re-review promptly.

[sonichi]

@keweichen @john-the-dev, round 3 is at 5ecc583f883c2c767510a6ccac2d1c24fb5f2f35. It answers Kewei's review and Rui's two nits. I reproduced each blocker at 448b1cac5 first, through the full restart beat.

1. The card retry contradicted the archive (Kewei, blocking). Fixed.

• The archive is now the authority for every retry that touches a withheld record or its card.
• _defers_to_archive runs in the card-resolution retry and in _resolve_review_card. When archived_withheld_decision finds a decision, it first restores the owner's card to the archived outcome. Only after that edit is accepted does it retire the live copy to archive/superseded/ under the decision lock.
• If the card cannot be restored, the record, the archive and the card are all left untouched and the next beat retries.
• The publication gate already refuses anything beside an archived decision.
• The beat's three retries are now one named function, _retry_withheld_reviews(), called by the poll loop, so the regression drives the production sequence.

Kewei's schedule, with the base writer from a temp worktree at 90244ef02, then one restart beat from each head:

$ seed with origin/main 90244ef02 (card edit during the stale No: edit-ok), then one restart beat with previous head 448b1cac5
{"phase": "seed-main", "checkout": "wt-5297-base", "live": "publish_pending", "live.card_resolution_pending": false, "archive": "kept_private", "posts": [], "owner_card_edits": []}
{"phase": "retry-head", "checkout": "wt-5297-r2", "live": "publish_pending", "live.card_resolution_pending": false, "archive": "kept_private", "posts": [], "owner_card_edits": []}
$ seed with origin/main 90244ef02 (card edit during the stale No: edit-fails), then one restart beat with previous head 448b1cac5
{"phase": "seed-main-no-crash", "checkout": "wt-5297-base", "live": "publish_pending", "live.card_resolution_pending": true, "archive": "kept_private", "posts": [], "owner_card_edits": []}
{"phase": "retry-head", "checkout": "wt-5297-r2", "live": "publish_pending", "live.card_resolution_pending": false, "archive": "kept_private", "posts": [], "owner_card_edits": ["\u2713 False positive recorded; publication to the original room is pending."]}
$ seed with origin/main 90244ef02 (card edit during the stale No: edit-ok), then one restart beat with this head
{"phase": "seed-main", "checkout": "wt-5297-base", "live": "publish_pending", "live.card_resolution_pending": false, "archive": "kept_private", "posts": [], "owner_card_edits": []}
{"phase": "retry-head", "checkout": "wt-5297", "live": null, "live.card_resolution_pending": null, "archive": "kept_private", "posts": [], "owner_card_edits": ["\u2713 Kept private \u2014 the owner confirmed it contains sensitive information."]}
$ seed with origin/main 90244ef02 (card edit during the stale No: edit-fails), then one restart beat with this head
{"phase": "seed-main-no-crash", "checkout": "wt-5297-base", "live": "publish_pending", "live.card_resolution_pending": true, "archive": "kept_private", "posts": [], "owner_card_edits": []}
{"phase": "retry-head", "checkout": "wt-5297", "live": null, "live.card_resolution_pending": null, "archive": "kept_private", "posts": [], "owner_card_edits": ["\u2713 Kept private \u2014 the owner confirmed it contains sensitive information."]}

In edit-ok, main's own stale No had already shown the owner "False positive" before the crash. This head restores the card to "Kept private". The committed regression (section 6) runs that schedule through _retry_withheld_reviews() with card_resolution_pending true and false:

• while the card cannot be restored (timeout or rejected edit), everything stays untouched;
• once it can, exactly one "Kept private" edit is made, the copy is retired with its bytes intact, and the archive's bytes and mtime are unchanged;
• nothing is ever posted.

Section 6b checks that each retry defers on its own.

2. A claim id on the record was treated as proof (Kewei, blocking). Fixed with a separate ledger.

• claim_withheld_decision creates claims/<review>.<claim_id> with O_EXCL and fsyncs it under the lock, before the record names the claim.
• withheld_claim_publishable requires a 32-hex id, its ledger entry for this review, and a live publish_pending/false_positive record carrying exactly that id, never beside an archived decision.

$ persisted publish_pending claim-id matrix, restart retry at 448b1cac5
missing          posts=0
list             posts=0
short            posts=0
nonhex32         posts=1
arbitrary_hex32  posts=1
$ persisted publish_pending claim-id matrix, restart retry at this head
missing          posts=0
list             posts=0
short            posts=0
nonhex32         posts=0
arbitrary_hex32  posts=0

Section 7 covers these ids: list, short, non-hex 32, unissued hex32 and issued-for-another-review. Section 7b covers the exact-claim case: two ids issued for this review, and the caller holds the one the record does not carry. Section 8 is the positive control: an issued claim interrupted before its POST still posts exactly once.

3. The delegation spy did not discriminate (Kewei). Fixed. The counters are gone. Each injected guard function now returns an answer the real guard would not give for that state, and the test asserts the bridge acted on it:

• a refused claim leaves an awaiting record byte-identical;
• a sentinel claim posts;
• a sentinel archive drives the card edit and the retirement.

Your bypass mutant, applied to each of the three calls, is killed. All mutants were applied to src/, re-vendored so the byte-identity check cannot mask them, and run with python3 -B (I found that an mtime-plus-size .pyc cache can silently reuse an earlier mutant's bytecode):

### mutant publish_failed_claimable: exit=1 vendored=in-sync FAIL-lines=2
FAIL: publish_failed: claims [{'review_id': 'wr_0000000000000000', 'status': 'kept_private', 'dm_room_id': '!owner-d
### mutant no_status_check: exit=1 vendored=in-sync FAIL-lines=48
FAIL: a stale No on a live kept-private record must not post or flip it
### mutant no_archive_check_in_guard: exit=1 vendored=in-sync FAIL-lines=8
FAIL: claimed-but-archived-kept_private: claims [None, None], publish True, 1 posts
### mutant any_string_claim_id: exit=1 vendored=in-sync FAIL-lines=3
FAIL: a different issued claim id is not the live record's release claim
### mutant no_claim_ledger_check: exit=1 vendored=in-sync FAIL-lines=4
FAIL: unissued-hex32-claim-publish_pending: claims [None, None], publish True, 1 posts
### mutant ledger_not_written: exit=1 vendored=in-sync FAIL-lines=18
FAIL: a stale Yes after publication must not rewrite the published decision
### mutant bridge_claim_bypasses_injected_owner: exit=1 vendored=in-sync FAIL-lines=1
FAIL: the bridge's decision is the injected guard's claim answer (refused: nothing written)
### mutant bridge_publish_gate_bypasses_injected_owner: exit=1 vendored=in-sync FAIL-lines=1
FAIL: the bridge's publication gate is the injected guard's answer
### mutant bridge_archive_bypasses_injected_owner: exit=1 vendored=in-sync FAIL-lines=1
FAIL: the bridge's archive authority and retirement are the injected guard's
### mutant card_retry_ignores_archive: exit=1 vendored=in-sync FAIL-lines=4
FAIL: legacy pending=False: the deferral is logged for the owner
### mutant resolve_card_ignores_archive: exit=1 vendored=in-sync FAIL-lines=2
FAIL: a card resolution beside an archived decision reports nothing resolved
### mutant retire_without_card_restore: exit=1 vendored=in-sync FAIL-lines=10
FAIL: legacy pending=True, card unreachable: record, archive and card untouched
### mutant beat_skips_card_retry: exit=1 vendored=in-sync FAIL-lines=7
FAIL: legacy pending=True: the deferral is logged for the owner

Rui's nits.

1. The any-string-claim-id mutant is now killed (any_string_claim_id above), via the exact-claim case in 7b. Against the previous test it passed, with exit 0.
2. The claim id stays on kept-private records, and it is harmless. The publication gate requires publish_pending and false_positive, and the ledger proves only issuance, so a kept_private record's id is never used to publish. Section 7 pins this: a kept_private record whose id is issued and matches the caller's is refused everywhere, with bytes and mtime preserved.

Coverage. Locally, against git diff -U0 origin/main, changed-line coverage is 99.2%. The one missing line is the poll loop's call to _retry_withheld_reviews() inside main().

Checks at this head:

• 105 of the 109 related files pass. The 4 that fail also fail at 90244ef02, for host reasons.
• The sync check, review-checks.sh, lint-hermetic-bridge-tests.py, ruff and git diff --check are all clean.

Rebase plan: whichever of this PR and #5269 lands second rebases. withheld_decision_lock is the single named lock function, so #5269's ledger lock subsumes it. The claim ledger and decision_claim_id are additive alongside #5269's body-bound records.

The exact-head live post-restart witness is still outstanding. It will be posted separately.

[keweichen]

REQUEST_CHANGES — this is not approval-ready at 5ecc583f883c2c767510a6ccac2d1c24fb5f2f35.

The round-3 fixes are real. I independently reproduced the exact base-writer → crash-left publish_pending → current full retry beat: this head refused publication, restored the archived kept_private card, and retired the stale live copy. Malformed, well-shaped-but-unissued, and other-review claim IDs all remained private; the focused regression, canonical/vendored sync check, repository review checks, and git diff --check passed. I also independently ran the 13 advertised mutants; all were killed, including the three injected-owner delegation mutants.

There are still three implementation/test blockers, plus the outstanding live-path gate:

1. Archive authority omits a valid terminal status and does not validate the archived decision pair. _archive_resolved_review archives publish_failed, but _defers_to_archive restores/retires only kept_private and published. A full retry beat with an archived publish_failed/false_positive plus a contradictory live kept_private/sensitive record produced:

   archive_status=publish_failed card_edits=[] live_still_present=true live_unchanged=true
   

   That state is reachable when a failed No publication has already been archived and a stale Yes handler crashes after its live write/card edit but before its archive move. The contradictory owner card and hot record then persist indefinitely. The same path accepts any archived JSON object and _resolved_review_body trusts decision before status; my adjacent-state probes produced:

   archive kept_private/false_positive -> card: "False positive ... pending"; live retired
   archive published/sensitive         -> card: "Kept private"; live retired
   

   The second case falsely assures the owner that an archived publication stayed private. Please reconcile all terminal archive states and fail closed (leave both records/card untouched) for inconsistent status/decision pairs; the committed regression currently covers only the consistent archived kept_private case.

2. A retirement filesystem error escapes the per-record retry and wedges inbound polling. retire_superseded_record can raise from mkdir/os.replace; _defers_to_archive catches only the card edit, and _retry_review_card_resolutions calls it outside its per-record try. With archive/superseded occupied by a regular file, the production retry produced:

   card_edit_count=1 card_says_kept_private=true live_still_present=true raised=FileExistsError
   

   main() runs this retry before the task GET (lines 5213–5219), so one persistent permission/disk/path-shape failure aborts every beat before inbound tasks are polled. Isolate the record failure and add a regression proving later retries/polling continue.

3. Exact-head CI is genuinely red. The hosted node + out-of-tree python tests job fails, and I reproduced it locally with python3 packages/ag2-sparrow/tests/test_pool_advertisement_push.py:

   AssertionError: _retry_pending_publications is not called in main()
   

   The existing structural contract requires the three retries before the advertisement push (test lines 469–492), while main() now calls the wrapper. The wrapper behavior is correct, but the shipped delegation contract was not updated to pin that wrapper and its order. The failing job ran normally; this is not an infrastructure/zero-step failure.

4. The required real post-restart witness is still absent. The author's exact-head evidence explicitly leaves the real gateway round trip outstanding. Because this changes bridge retry/delivery behavior, REVIEW.md lesson 15 requires real input → restarted service → delivered result evidence at this SHA before approval; a fake-relay fresh-process probe is not that witness.

Worst case at this head: an archived outcome can leave or create a false owner-facing resolution, while a single retirement I/O fault prevents the gateway from polling any new tasks. Please cover the terminal/malformed archive matrix, contain retirement failures, restore green CI, and attach the exact-head live witness; I will re-review promptly.

[sonichi]

@keweichen, round 4 is at 60b68ccea1e44bb737801acc7bcaa2f18c01ac1e and answers your review. @john-the-dev, this round is relevant to your earlier review too. I reproduced each item at 5ecc583f8 first, using the production functions in a fresh process.

1. The archive reconcile was not a table (blocking). Fixed with one reconcile table in the guard. ARCHIVED_OUTCOMES / archived_outcome() is keyed on the archived (status, decision):

archived status	archived decision	action
kept_private	sensitive	restore the card to "Kept private", then retire the live copy
published	false_positive	restore the card to "Published to the original room", then retire the live copy
publish_failed	false_positive	restore the card to "publication failed and requires attention", then retire the live copy
anything else, incl. non-string fields, non-dict or unreadable	—	leave record, archive and card untouched; log once for the owner

No row publishes. The bridge reconciles only a proven outcome. Your three probes, before and after:

$ archived-decision variants beside a live copy, then one restart beat — at 5ecc583f8
archive publish_failed/false_positive  card_edits=[] live_present=True live_unchanged=True posts=0
archive kept_private/false_positive    card_edits=['✓ False positive recorded; publication to the original room is pending.'] live_present=False live_unchanged=False posts=0
archive published/sensitive            card_edits=['✓ Kept private — the owner confirmed it contains sensitive information.'] live_present=False live_unchanged=False posts=0
retire-error card_edits=['✓ Kept private — the owner confirmed it contains sensitive information.'] live_present=True raised=FileExistsError

$ same, at this head
archive publish_failed/false_positive  card_edits=['✓ False positive recorded, but publication failed and requires attention.'] live_present=False live_unchanged=False posts=0
archive kept_private/false_positive    card_edits=[] live_present=True live_unchanged=True posts=0
archive published/sensitive            card_edits=[] live_present=True live_unchanged=True posts=0
retire-error card_edits=['✓ Kept private — the owner confirmed it contains sensitive information.'] live_present=True raised=None

Section 6c tests every row: 9 statuses (including missing, int and list) × 4 decisions, plus non-dict and unreadable archives. Each row runs two full retry beats with a live legacy copy beside the archive.

• For the 3 proven pairs: exactly one card edit with the expected text, the copy retired with its bytes intact, and the archive untouched.
• For every other row: no edit, and the copy's bytes and mtime are unchanged.
• Nothing posts in any row, and the expected outcomes are hard-coded in the test rather than read from the module.

2. A retirement fault wedged intake (blocking). Fixed. _defers_to_archive now contains every fault for its record, including a retirement that raises, and logs it. Section 10 drives one real main() iteration with archive/superseded occupied by a regular file:

• the new task is still taken in during the same beat;
• the fault is logged for its record, and nothing is logged as unexpected;
• the copy and the archive are untouched.

The retire_error_escapes mutant (catch only TimeoutError) is killed by exactly that section.

3. Exact-head CI was red. Fixed. The node + out-of-tree python tests job in run 38005986805 failed on the structural contract. It now pins that main() calls _retry_withheld_reviews before the advertisement push, and that the wrapper runs the three retries in their original order:

$ python3 packages/ag2-sparrow/tests/test_pool_advertisement_push.py   # at 5ecc583f8
    assert name in lines, f"{name} is not called in main()"
AssertionError: _retry_pending_publications is not called in main()
exit=1

$ python3 packages/ag2-sparrow/tests/test_pool_advertisement_push.py   # at this head
PASS test_shutdown_holds_the_lock_until_no_publication_can_complete
ALL PASS test_pool_advertisement_push
exit=0

My related-suite run had missed it because it only listed tests/. It now also includes every out-of-tree Python test the CI job runs: 129 files under env -i.

• 124 pass at this head.
• 5 fail identically on origin/main for host reasons: the vault token in both remote-gateway-bridge suites, the credential golden test, a live Slack bridge and the room-ops redaction test.

Mutants. All 18 are killed. Each was re-vendored and run with python3 -B, then restored:

### mutant publish_failed_claimable: exit=1 vendored=in-sync FAIL-lines=2
### mutant no_status_check: exit=1 vendored=in-sync FAIL-lines=48
### mutant no_archive_check_in_guard: exit=1 vendored=in-sync FAIL-lines=8
### mutant any_string_claim_id: exit=1 vendored=in-sync FAIL-lines=3
### mutant no_claim_ledger_check: exit=1 vendored=in-sync FAIL-lines=4
### mutant ledger_not_written: exit=1 vendored=in-sync FAIL-lines=16
### mutant table_drops_publish_failed: exit=1 vendored=in-sync FAIL-lines=2
### mutant table_trusts_status_only: exit=1 vendored=in-sync FAIL-lines=18
### mutant table_accepts_any_dict: exit=1 vendored=in-sync FAIL-lines=2
### mutant bridge_reconciles_any_archive: exit=1 vendored=in-sync FAIL-lines=33
### mutant retire_error_escapes: exit=1 vendored=in-sync FAIL-lines=2
### mutant bridge_claim_bypasses_injected_owner: exit=1 vendored=in-sync FAIL-lines=1
### mutant bridge_publish_gate_bypasses_injected_owner: exit=1 vendored=in-sync FAIL-lines=1
### mutant bridge_archive_bypasses_injected_owner: exit=1 vendored=in-sync FAIL-lines=1
### mutant card_retry_ignores_archive: exit=1 vendored=in-sync FAIL-lines=4
### mutant resolve_card_ignores_archive: exit=1 vendored=in-sync FAIL-lines=2
### mutant retire_without_card_restore: exit=1 vendored=in-sync FAIL-lines=14
### mutant beat_skips_card_retry: exit=1 vendored=in-sync FAIL-lines=11

Checks at this head:

• Changed-line coverage is 100% locally.
• The sync check, review-checks.sh, lint-hermetic-bridge-tests.py, ruff and git diff --check are clean.
• The new test passed 5 more runs in a row.

4. Live witness. The exact-head post-restart round trip is still outstanding and will be posted separately.

[john-the-dev]

Re-review at 60b68ccea1e44bb737801acc7bcaa2f18c01ac1e: no code blockers found. COMMENTED, not APPROVE. This repo needs Rui's Codex-core PASS at this exact head before I approve. The exact-head live post-restart witness (REVIEW.md lesson 15, keweichen's item 4) is also still outstanding.

I tested in a fresh checkout at the head, with merge-base 90244ef0, python3 -B, and __pycache__ cleared before every run. My probes drive the production _retry_withheld_reviews() with a fake relay.

1. Legacy state and claim-id variants. Nothing posts.

A archive kept_private + live publish_pending, no claim id   posts=[] live retired
B same + forged 32-hex claim                                 posts=[] live retired
C live publish_pending, claim id wrong length                posts=[]
D live publish_pending, claim id non-str                     posts=[]
E live publish_failed + claim                                posts=[]
F archived awaiting_owner + live awaiting, then No           claim=None posts=[]
G archived awaiting_owner + live pp w/ claim                 posts=[]
mixed-version race (main's stale-read pp overwrites a head-claimed Yes)  posts=[]
H genuine No claimed, crash before POST, 3 beats             posts=['SECRET']   (exactly once)

2. The reconcile table: 9 archived statuses x 4 decisions, beside a live legacy publish_pending, two beats each. The statuses include missing and int; the decisions include missing.

pairs that acted: [('kept_private','sensitive'), ('published','false_positive'), ('publish_failed','false_positive')]
  each: posts=0, one card edit with the matching text, live copy retired
every other pair (incl. un-tabled kept_private/false_positive, published/sensitive): posts=0 edits=[] live bytes unchanged
non-dict archive / unreadable archive / status given as a list: posts=0 edits=[] live untouched

3. Trying to make the O_EXCL ledger accept a forged id. All refused. Each case shows withheld_claim_publishable=False and posts=[]:

• review B's genuinely issued claim id placed on review A's record;
• the same id uppercased;
• a ../-prefixed id;
• a directory planted at claims/<A>.<id>.

4. Mutation testing. Each mutant was applied to both src/policy/egress/result.py and the vendored team_result_guard.py, with sync_from_src.py --check in sync, then run against tests/withheld-review-decision-final.test.py. 17 of 21 were killed:

• Ledger: ledger_check_removed, ledger_any_review, claim_id_any_string, no_ledger_write.
• Table: table_adds_kp_fp, table_adds_pub_sens, table_drops_kp, table_drops_published, table_status_only, table_no_str_check, table_nondict_ok.
• Bridge: bridge_any_outcome, bridge_retire_without_edit, bridge_unproven_falls_through, bridge_except_narrow (catch only TimeoutError; killed by the main() intake section), bridge_publish_gate_off, card_retry_no_defer.

4 survived. None is a reachable hole:

• no_hex_check and no_len_check: a malformed id never has a ledger entry, so the ledger lookup refuses it anyway, and a / cannot traverse because claims/<stem>.<x> is not a directory. The format check is defense-in-depth that no test pins on its own. Nit, optional.
• ledger_exists_not_file and no_excl: equivalent for any id the code can issue (os.urandom(16)).

5. My two earlier nits.

• Nit 1 (any-string claim id) is addressed: claim_id_any_string is killed by section 7b.
• Nit 2 (claim id stamped on kept-private records) is unchanged, and I accept the rationale. A synthetic writer that flips a Yes-claimed live record to publish_pending while keeping its id would publish. My probe I shows this: posts=['SECRET']. But no shipped writer produces that state. Main's handler treats a live kept_private record as a delivery retry, and the racing write comes from a pre-claim read, so it carries no id. The mixed-version race case above refuses. Not stamping the id when status != publish_pending would remove even the synthetic case. Optional.

6. Observation, not a blocker. In the mixed-version race (main still running concurrently on the same state dir), the record stays publish_pending. The card retry then edits the owner card to "False positive recorded; publication pending" while the body stays private. Main itself would post in that state anyway, so this PR cannot close it. Mentioning it only for the owner-facing text.

7. Checks.

• sync_from_src.py --check: in sync.
• bash scripts/review-checks.sh --diff <(gh pr diff 5300), 1028 lines, run from the repo at head: PASS.
• git diff --check 90244ef0 HEAD: clean.
• test_pool_advertisement_push.py: ALL PASS.
• The PR test passes under env -i.
• Exact-head check-runs: 36 total, all success or skipped. That includes diff coverage >= 95% (python), node + out-of-tree python tests (red at 5ecc583f) and all 9 python shards. license/cla is success.

Ready for Codex-core verification at 60b68cce. Approval also waits on the exact-head live post-restart round trip.

Agent-assisted review by Rui's Sutando (Claude); no personal human inspection claimed.

[john-the-dev]

Approve at 60b68ccea1e44bb737801acc7bcaa2f18c01ac1e.

My evidence is in COMMENTED review 5476765048 at this head:

• No withheld body is posted. Every one of these posts nothing: legacy archived kept_private with a live publish_pending (with or without a forged claim), malformed claims, publish_failed, an archived awaiting_owner, and a live race against old main code. A genuine No followed by a crash publishes exactly once.
• Reconcile table: none of the 9×4 archived (status, decision) pairs, nor the non-dict, unreadable or list archives, posts anything. Only the three listed pairs edit the card and retire the live copy.
• The O_EXCL ledger refuses forgeries: another review's id, an uppercase id, a ../ id, and a planted directory.
• Mutants: 17 of 21 killed. The survivors are the redundant claim-format check and two equivalents.
• CI and housekeeping: all 36 checks at this head are green, the vendored copy is in sync, and review-checks passes on the full diff.

Rui's Codex-core verification: PASS at this exact head. Its independent probe shows an unissued 32-hex claim in a live publish_pending posts once at 448b1ca and 0 times here. It also confirms all 36 archived combinations, 22 Sparrow cases, and the ledger and kept_private/false_positive mutants.

Before merge: keweichen's standing CHANGES_REQUESTED (latest at 5ecc583) needs clearing by its author, and the PR still owes the real post-restart gateway/owner-review round trip, which the author is posting separately.

Agent-assisted review by Rui's Sutando (Claude); no personal human inspection claimed.

[liususan091219]

Ready to approve.

Reviewed head 60b68ccea1e44bb737801acc7bcaa2f18c01ac1e.

Evidence I checked myself, in a fresh worktree at this head:

• python3 -B tests/withheld-review-decision-final.test.py: PASS. python3 -B packages/ag2-sparrow/tests/test_pool_advertisement_push.py: ALL PASS (this was the red CI job at 5ecc583f).
• I reverted one fix line (the withheld_claim_publishable gate at the top of _publish_review) and re-ran the new test. It failed with 121 checks, including "a stale publish_pending snapshot must not post" and "legacy pending=True: nothing posts". Restored afterwards. So the test exercises the real bridge and guard, not a copy.
• sync_from_src.py --check: in sync. scripts/review-checks.sh on the full diff: PASS, and no added hardcoded workspace paths.
• CI: every check at this head is SUCCESS. license/cla: success (one context). No unresolved review threads.

keweichen's blockers at 5ecc583f, as I read them at this head:

1. Archive authority: ARCHIVED_OUTCOMES in src/policy/egress/result.py now covers publish_failed/false_positive, and any other or malformed (status, decision) pair leaves record, archive and card untouched. Addressed.
2. A retirement error wedging the beat: _defers_to_archive catches everything and returns True, so the per-record fault no longer escapes _retry_review_card_resolutions. The test has a "failed retirement" case. Addressed.
3. Red CI: green now. Addressed.

This matches john-the-dev's approval at this head, which includes Rui's Codex-core PASS and a 21-mutant run.

Still needed before merge (not blockers to this verdict):

• keweichen's CHANGES_REQUESTED at 5ecc583f stands until keweichen re-reviews this head.
• The real post-restart round trip at this head (REVIEW.md lesson 15) is still owed; the author says it will be posted separately.

Nits:

• If retire_superseded_record keeps failing (for example archive/superseded is a file), _defers_to_archive re-sends the same card edit every beat, because the edit runs before the retirement and nothing remembers it succeeded. The log is rate-limited; the network edit is not.
• _REFUSED_REVIEW_PUBLICATIONS grows for the life of the process. Small in practice.

Not verified by me: the live gateway round trip, and the mixed-version race (I relied on john-the-dev's probe for that).

— Sutando (susans-MacBook-Pro)

[qingyun-wu]

Code-owner approval at 60b68cc, on Qingyun's Approve in the [PA] Pending actions app. All checks green on this head. Kewei's changes request at 5ecc583 is still open and needs his re-review at this head; John approved this head. Merge stays with Chi.

[qingyun-wu]

Review at 60b68ccea1e44bb737801acc7bcaa2f18c01ac1e (unchanged since keweichen's round 3 answer). COMMENT only: this login is shared, and it already recorded an APPROVED at this head (16:49Z).

Verdict: no code blockers. Not merge-ready until the exact-head post-restart gateway round trip is posted (REVIEW.md lesson 15; the PR body, line 239, still says it "has not been run yet"). This is a bridge delivery/retry path, so harness proof is not enough. Note for the owner: the shared login's APPROVED at this head predates that witness.

What I ran (fresh clone, merge-base 90244ef0, python3 -B, env -i HOME=<tmp> PATH=...):

• tests/withheld-review-decision-final.test.py: exit 0, ends PASS: a durable owner decision is final; stale or racing replies change nothing.
• Same test against base source (90244ef0 with only the new test copied in): exit 1. Five behavioral FAILs fire before it stops on the missing _retry_withheld_reviews, e.g. FAIL: a stale No after a durable keep-private must not post the body and FAIL: the archived keep-private decision must not flip. So the test reproduces the original leak.
• packages/ag2-sparrow/tests/test_pool_advertisement_push.py: ALL PASS (the job that was red at 5ecc583f).
• packages/ag2-sparrow/tools/sync_from_src.py --check: in sync. scripts/review-checks.sh --diff on the full PR diff: PASS. git diff --check 90244ef0 HEAD: clean.
• Three mutants of my own on remote_gateway_bridge.py, each restored after:
  • _publish_review gate replaced by if False: -> exit 1, 122 FAIL lines;
  • _defers_to_archive skip removed from _retry_review_card_resolutions -> exit 1, 5 FAIL lines;
  • _defers_to_archive's except Exception narrowed to TimeoutError -> exit 1, 3 FAIL lines.
• gh pr checks 5300: everything pass; only PowerShell + Windows runtime and publish skipped.

keweichen's CHANGES_REQUESTED at 5ecc583f, checked at this head:

1. Archive authority / pair validation: addressed. ARCHIVED_OUTCOMES (src/policy/egress/result.py:456-460) has exactly the three proven pairs, including publish_failed/false_positive; the test's expected table is hard-coded (tests/withheld-review-decision-final.test.py:282), not read from the module, and my card_retry_no_defer mutant is killed.
2. Retirement fault wedging intake: addressed. _defers_to_archive contains every fault (remote_gateway_bridge.py around the new except Exception), and narrowing it is killed by the test.
3. Red CI: addressed; green at this head and locally.
4. Live witness: still open.
   His block stands until he re-reviews; nothing here clears it on his behalf.

Nits (non-blocking):

• A genuine No written by a pre-fix bridge (publish_pending, no claim id, no archive) is now stranded: _publish_review refuses it forever, claim_withheld_decision will not re-claim it (status is not awaiting_owner), and the card retry resolves the owner card to "publication to the original room is pending", which will never become true. The body (line 44) discloses the "never publishes" half; the owner-facing card text is the part worth a one-line follow-up. Reasoned from the code, not probed.
• As liususan091219 noted, a persistent retirement failure re-sends the same card edit every beat.

Not verified by me: the live gateway round trip; the mixed-version race (main and this head on one state dir), for which I rely on john-the-dev's probe.

Agent-assisted review by Qingyun's Sutando (Claude) on the shared qingyun-wu login; no personal human inspection claimed.

[keweichen]

Readiness/evidence check at 60b68ccea1e44bb737801acc7bcaa2f18c01ac1e — comment only, not a completed independent code review.

• CI: GitHub reports 34 successful check runs and 2 skipped (PowerShell + Windows runtime and publish); license/cla is also successful. Current checks.
• Earlier code findings: the existing exact-head reviews report that archive reconciliation, retirement-error isolation, and the earlier CI failure are addressed. I read those reports; this is not independent confirmation of their code findings. See John's review and Qingyun's latest review.
• Outstanding evidence: the PR description and the latest review still list the real post-restart gateway/owner-review round trip at this exact head as missing. That remains the documented pre-merge evidence requirement.

My check was limited to CI status and review evidence; I did not rerun the security probes or independently validate the implementation fixes. This comment is neither an approval nor a new request for changes, and does not clear or modify any existing review decision.

Posted by Codex at the owner's request.

[liususan091219]

Re-check at head 60b68cc (unchanged since my review 5476928594). COMMENT: the code is still verified, but this head can no longer merge as reviewed.

What changed since 5476928594:

• #5269 merged at 2026-10-10T16:56:21Z (merge commit 0d2db0e). gh pr view now reports mergeable=CONFLICTING, mergeStateStatus=DIRTY; compare main...head is 35 behind, 5 ahead, merge-base 90244ef.
• A trial merge of origin/main into this head in a scratch clone conflicts in exactly the three guard/bridge files: src/policy/egress/result.py, packages/ag2-sparrow/ag2_sparrow/team_result_guard.py, packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py.
• main now has its own _ledger_lock (origin/main src/policy/egress/result.py:386). The PR body says withheld_decision_lock should become, or delegate to, that lock on rebase. That is the concurrency core of this fix, so the rebased head needs fresh verification, not a carried-over approval.

Re-verified at this head (env -i, python3 -B): tests/withheld-review-decision-final.test.py exits 0, "PASS: a durable owner decision is final...". Added-line scan for ~/.sutando, $HOME/.sutando, /Users//.sutando, SUTANDO_WORKSPACE: no hits. CI all pass; license/cla success (1 context).

Needed before this can go to merge:

1. Rebase onto main. Show withheld_decision_lock delegating to main's _ledger_lock (one lock, not two), and re-run the new test, the 18-mutant set and sync_from_src.py --check at the rebased head.
2. keweichen's CHANGES_REQUESTED (5476452800, at 5ecc583) still stands; his 17:05Z comment explicitly does not clear it.
3. The exact-head post-restart gateway round trip (REVIEW.md lesson 15) is still owed, per the PR body's "Live path" section. Under our policy that is named, not blocking, but it must be at the rebased head.

Approvals at 60b68cc (john-the-dev 5476781324, qingyun-wu 5479931549) predate the conflict and go stale at the rebase.

Not verified by me: the conflict resolution itself (it does not exist yet), and the live round trip.

— Sutando (susans-MacBook-Pro)

[sonichi]

Merged current main into this branch at e4226f1a2, as a merge commit. Main had gained its own per-body record ledger (_ledger_lock, update_record, archive_record, and actionable_records, which freezes conflicts into conflicts/), so the guard needed more than a textual resolution:

• One lock. Every decision claim, publication check, archived read and retire now runs under main's _ledger_lock. The separate .decision.lock is gone, so a claim can no longer interleave with an id issue, write or archive.
• One writer. update_record and claim_withheld_decision now share one owned-record writer, _write_owned_record, and a claim writes only while the record owns its id. The claim ledger and the awaiting_owner check are unchanged. Main has neither, and without them a second owner reply could rewrite a decision.
• Live copy beside an archived decision. The owner chose to reconcile only proven outcomes:
  • When the live copy holds the same body as its archive, and the archive is one of the ARCHIVED_OUTCOMES pairs, the copy is reconciled (card restored, copy retired to archive/superseded/).
  • Every other case keeps main's freeze to conflicts/ and never acts.
  • actionable_records leaves only those proven copies in place, and the bridge reconciles them before the freeze runs.
• Per-body ids. The decision functions work on whichever per-body record the path names. "The first durable decision is final" now holds per record.

Tests:

• tests/withheld-review-decision-final.test.py: PASS. The reconcile table now expects unproven rows frozen in conflicts/, byte-identical. A new split case pins that a proven pair is reconciled while the same proven archive beside a different body is frozen.
• Control: removing the proven-copy skip from actionable_records fails the proven reconcile checks.
• tests/gateway-disposal-delegation.test.py: OK, 75 tests. Its one-writer contract now also accepts the claim path, and asserts that update_record and claim_withheld_decision are the only callers of the shared writer.
• 67 gateway, guard, marker, drift and withheld suites, each run under env -i HOME PATH: 66 pass. The other, remote-gateway-bridge, fails one check (env-fallback: no env token and no file yields empty token), which fails identically on plain main on the same host.
• src/policy/egress/result.py and the vendored team_result_guard.py are byte-identical.

[john-the-dev]

Not review-clean at dbea3fdee (full sha dbea3fdee18cfa825bbfbe1e5997e85b3175d1a6, verified unmoved): one blocker. It is the still-open half of Kewei's blocker 2. I'm filing this as a COMMENT because approval on this repo waits on Sutando Release verification, but this head is not ready for that verification yet.

Blocker: a .issued reservation of the wrong kind still turns an owner reply into ordinary work.

• namespace_problems() checks claims/ and issued/ leaves with present(path) and no kind (result.py, the if not records: present(path) branch). So a directory or FIFO named <stem>.issued passes the preflight.
• The reply path then calls read_strict(reservation), which raises on the kind mismatch. _pending_review_records() turns that OSError into [], and _handle_review_decision() returns False.
• I drove this through the real bridge, guard and health functions with temp fixtures and the fake relay from this PR's test:

issued-leaf-dir   5701610d0: handled=True  reply-again=True  posts=0
                  178acbe7f: handled=False reply-again=False posts=0  health=ok
                  dbea3fdee: handled=False reply-again=False posts=0  health=ok   (3.9.6 and 3.14.6)
issued-leaf-fifo  dbea3fdee: handled=False reply-again=False posts=0  health=ok

• Nothing publishes. But the owner's Yes/No falls through to the core as ordinary chat, which is the regression Kewei named ("a final .issued symlink or odd entry"). The health check also reports ok for it.
• Only the symlink leaf was fixed, and the test pins only that case.
• Suggested fix: check leaves as present(path, stat.S_IFREG) (.lock is regular too). Better, keep UNKNOWN distinct all the way to the handler: on the reply path, _pending_review_records should raise _ReviewsUnreadable rather than return [], so the reply never depends on the preflight's list being complete. Please add a test with a dir or FIFO leaf.

Verified fixed (each driven through the production functions on 3.9.6 and 3.14.6):

• Codex-core P2 (my account's Sutando Release run at 178acbe7f): linked claims/ → health warn, the reply is answered "reply again", 0 POSTs. Same for linked issued/. At 178acbe7f both read health=ok.
• Linked .issued leaf: at 5701610d0 it posted once. At 178acbe7f the reply fell through (handled=False). Now handled=True, reply-again, 0 POSTs, health warn.
• Write paths: with archive/superseded/ and conflicts/ linked, the in-suite real-retry cases keep the live record byte-identical, write nothing outside, post nothing and answer the reply. The suite passes on both versions. _ensure_dir, _durable_move (real source, verified target) and the O_NOFOLLOW creates read correctly.
• The normal non-link path is unaffected: a plain review + "No" publishes exactly once, and health is ok.
• My nit 1 (mid-read directory swap): reads now go through dir_fd with no-follow at every step from the namespace root. The directory-swap-before-open and root-swap-before-open tests pin it, and the harness kills both mutants.

Mutation harness (tests/mutation/5300/run.py): I ran it myself.

• 15/15 KILLED on 3.9.6 and on 3.14.6.
• It can report a survivor. I added two probes to a local controls.json:
  • dropping O_NOFOLLOW from _reserve's O_EXCL create → SURVIVED. It is equivalent, because O_EXCL already refuses an existing link.
  • swapping the preflight leaf's present(path) for os.lstat(path) → KILLED (by the linked-reservation test).
  • So the runner does distinguish the two. A third control, swapping present(path) for present(path, S_IFREG), would be the regression test for the blocker above.

Suites: withheld-review-decision-final, health-check-outbox-parked, team-result-guard, gateway-disposal-delegation, gateway-terminal-result-moved-once and withheld-review-dm all pass on 3.9.6 and 3.14.6. The guard and its vendored copy are byte-identical. review-checks.sh --diff on the full PR diff passes. CI: 36 pass, 3 skipped.

Nits (non-blocking):

1. Writes (mkdir, os.link, the O_EXCL creates) are still path-based after the walk, not anchored to a dir_fd the way reads are. A directory swapped mid-write would be followed. That needs write access inside the state dir, so it is the same trust domain.
2. claim-leaf-dir (a directory named like a claim) reads health=ok. The reply is consumed through the decision-lost path, so no harm, but the preflight's leaf check applies to claims/ too.

Still owed: the blocker above; then Sutando Release verification and the live post-restart round trip at the fixed head.

[qingyun-wu]

Approve at dbea3fd. Kewei's three blockers from 178acbe are fixed; I reproduced each before and after.

Layout	178acbe	dbea3fd (3.14.6, 3.12.8, 3.9.6)
linked archive/superseded/	live record gone; written outside; health ok	0 POSTs; record bytes unchanged; target untouched; reply-again names the path; health warns
linked conflicts/	live record gone; written outside	same as above
linked .issued leaf	handled=False, fell through as ordinary work	handled=True; reply-again names the .issued path; health warns

(Hermetic real-bridge probes: 3 beats, then No, then Yes, then 3 more beats; link targets snapshotted before and after.)

Also verified on all three Pythons

• Fail closed (0 POSTs, record stays, nothing written outside, health names a workspace-relative path): linked claims/ and entries in it, linked issued/, and archive/ as chmod 000, a file, a link or a dangling link.
• Linked state root with no withheld dir: a plain Yes is ordinary chat. Linked root with a real pending review: "No" publishes exactly once across 6 beats.
• Every mkdir, write, move and reservation on a review-tree path goes through _ensure_dir/present first, and the bridge and health check share namespace_problems. The vendored copy is byte-identical.
• Tests: the PR's 5 suites plus 2 adjacent ones, 21/21 green under env -i. tests/mutation/5300/run.py kills 15/15 with no hardcoded paths. review-checks.sh passes; CI and CLA are green.

Should-fix (non-blocking)

1. Check-then-write is path-based; only reads use dir fds. _durable_move (result.py:604-611) calls os.link by path after _ensure_dir, as do mkstemp + os.replace and os.chmod(_ensure_dir(...)). Swapping superseded/ for a link right after _ensure_dir returns moves the record outside, on all three Pythons. The same swap against read_strict is refused. This needs a concurrent same-uid writer, which could read the 0600 records anyway, so it isn't blocking. Either use src_dir_fd/dst_dir_fd, or state that the dir-swap guarantee is read-side only.
2. Two write guards are unpinned. Reverting the strict check in journal_suppressed_result or journal_quarantined_attachment to plain mkdir + chmod survives every test. The code at head is correct; add a linked-namespace case for each.

Nits

• Reverting archive_record to mkdir + chmod survives; it would chmod a link target before _durable_move refuses.
• _archived_review_candidates now reads _read_review_json(...) or {} after the lock is released, so a record that turns unreadable in that window is skipped rather than refused. That needs a race and can't publish, but it is weaker than 178acbe.
• A linked archive/ is counted twice in the health warning.
• Windows has no O_NOFOLLOW/O_DIRECTORY, and directory junctions may not show as links under lstat (not verified; that CI job is skipped).
• The run.py docstring cites the PR number.

Still open, not mine to clear: Kewei's CHANGES_REQUESTED, and the live post-restart round trip the body says hasn't been run yet.

[sonichi]

Round for Rui's review at dbea3fdee (5495093372) and @qingyun-wu's should-fixes at the same head. New head: 4d15bf97f.

A claims/ or issued/ entry that is not a regular file is unknown (Rui's blocker). The shared preflight now checks every entry there with present(path, S_IFREG). So a directory or FIFO entry, like a link, makes the owner's reply unknown: it is consumed with "please reply again" and the health check warns. This restores the 5701610d0 behaviour for the odd-entry half of Kewei's blocker 2.

New tests use a real directory and a real FIFO (os.mkfifo) in both issued/ and claims/:

• through the bridge (_handle_review_decision: answered, still awaiting, no POST);
• through the guard (namespace_problems reports the entry);
• through the health check (warns and names it).

Moves go through verified directory fds (Qingyun's 1).

• _durable_move() now links and unlinks relative to directory fds. They are opened step by step from the namespace root with O_DIRECTORY | O_NOFOLLOW, the same _open_dir walk the reads use: os.link(..., src_dir_fd=, dst_dir_fd=, follow_symlinks=False) and os.unlink(..., dir_fd=).
• So archive/superseded/ (or conflicts/) swapped for a link right after it was verified is refused, and the record stays. A test drives that swap during a real retry beat.
• Known limit, stated in _ensure_dir's docstring: the other writes (in-place record rewrite, new-record creation, reservation/claim creation, mkdir) still verify the path and then write by path. A concurrent swapper with write access to the state directory could redirect those. Moving them to dir_fd means reworking the guard tests that spy on os.link/os.rename by path, so it is left for a follow-up.

The journal guards are tested (Qingyun's 2). journal_suppressed_result() and journal_quarantined_attachment() each refuse a linked suppressed-team-results/: the first returns the fail-closed suppression verdict, the second returns False. Nothing is written or re-moded outside.

Rui's write nit is covered by the move change and the stated limit above.

Committed mutation controls (tests/mutation/5300/) now hold 19 controls. The 4 added this round:

• a non-regular claims//issued/ entry read as fine;
• a move linking by path instead of through the verified fds;
• the suppression journal, and the quarantine journal, each creating its folder through a link.

The runner's test list adds the two journal suites. Results at 4d15bf97f: BASELINE pass and 19/19 killed, exit 0, on Python 3.9.6, 3.12.13 and 3.14.6.

Full Python discovery at 4d15bf97f, one file at a time under env -i (the 4 Swift-compiling files left out): 1149 of 1155 pass. The 6 failures are the same host-only files that fail identically at the merge base c5bae26c8. review-checks.sh passes on the full PR diff, and the vendored guard is byte-identical to src/.

The exact-head post-restart round trip is still not run; it will be scheduled after code review.

[john-the-dev]

Review-clean at 4d15bf97f (full sha 4d15bf97f6412c2059a29bdd2b1c7b5782ab7b91, verified unmoved), pending Sutando Release verification. My dbea3fdee blocker is fixed. No blockers. This is a COMMENT, not an approval: approval on this repo waits on Sutando Release (Codex) verification of this head.

My blocker is fixed. I re-ran my own repro: the real bridge, guard and health functions, temp fixtures, and this PR's fake relay. Results were identical on 3.9.6 and 3.14.6:

case	dbea3fdee	4d15bf97f
.issued leaf is a directory	handled=False, no reply, health ok	handled=True, reply-again, 0 POSTs, health warn
.issued leaf is a FIFO	handled=False, no reply, health ok	handled=True, reply-again, 0 POSTs, health warn
claims/ entry is a directory (my nit 2)	health ok	reply-again, 0 POSTs, health warn
claims/ entry is a FIFO	–	reply-again, 0 POSTs, health warn
unrelated directory inside issued/	–	reply-again, health warn
linked claims/ / issued/ / archive/ / .issued leaf	(fixed before)	still reply-again, 0 POSTs, health warn
normal review + "No"	1 POST	exactly 1 POST, health ok

The fix is present(path, stat.S_IFREG) in namespace_problems.

Move through verified directory fds: I tested a swap after the check. I wrapped _ensure_dir so that, right after it verifies archive/superseded/, the folder is renamed away and replaced by a link to an outside directory. Then I called the real _durable_move:

• At dbea3fdee, the record moved outside (outside: ['wr_1.x.json'], source gone).
• At this head, it is refused with NotADirectoryError from the no-follow _open_dir. The source stays and nothing is written outside.
• Same result on 3.9.6 and 3.14.6.
• I read _open_dir / _durable_move. It opens the namespace root with O_NOFOLLOW, takes each step relative to the last with no-follow, and then calls os.link(..., src_dir_fd, dst_dir_fd, follow_symlinks=False) and os.unlink(dir_fd=). A non-O_DIRECTORY platform falls back to path-based calls.

Mutation harness (committed). I ran it myself: 19/19 KILLED on 3.9.6 and on 3.14.6, with a clean baseline on both.

• To check that it reports survivors, I ran two extra probes against a local controls.json:
  • Removing _ensure_dir(target.parent) from _durable_move → KILLED (team-result-guard).
  • Flipping the new move's follow_symlinks=False → True → SURVIVED.
• So the runner does report a survivor. See nit 1 for what this one means.

Suites, on 3.9.6 and 3.14.6, in a /private/tmp clone with a fresh HOME — all pass:

• withheld-review-decision-final
• health-check-outbox-parked
• team-result-guard
• gateway-disposal-delegation
• gateway-terminal-result-moved-once
• withheld-review-dm
• team-guard-suppress-journaled-silent-close
• collaborator-attachment-gate

Other checks. The guard and its vendored copy are byte-identical. review-checks.sh --diff on the full PR diff passes. CI at this head: 36 pass, 3 skipped.

Nits (non-blocking):

1. follow_symlinks=False on the move is not pinned by any test (the survivor above). The source leaf is verified as a regular file just before, so it only matters if that leaf is swapped for a link in the gap. With True, the link target would be pulled into the archive and the link removed. Nothing leaks outward, but a source-leaf swap test would pin it.
2. Non-move writes (mkdir, the O_EXCL creates, the record rewrite) are still path-based after the walk. The _ensure_dir docstring now states this limit, and it needs write access to the state dir.

Still owed before merge: Sutando Release verification of this head, and the live post-restart owner-review → gateway → shared-room round trip.

Ready for Sutando Release (Codex) verification: yes.

[qingyun-wu]

Approve at 4d15bf9. Both of my should-fixes from dbea3fd and Rui's directory/FIFO blocker are fixed. No new blockers.

My earlier points

• Dir swap after _ensure_dir: fixed. _durable_move (~613-637) opens both dir fds with _open_dir, walking with O_DIRECTORY|O_NOFOLLOW at every step, then calls os.link(src_dir_fd=, dst_dir_fd=, follow_symlinks=False) and os.unlink(dir_fd=). My swap probe on 3.14.6, 3.12.8 and 3.9.6 is refused with ENOTDIR: nothing outside, record still live. At dbea3fd it moved the record out.
• Journal guards: fixed. My M3 and M4 mutants are now killed.
• archive_record mkdir+chmod revert (M1) still survives, but its impact is smaller now: the move is refused, so only the chmod lands outside.
• The run.py docstring still cites the PR number (line 2).

Rui's blocker. Verified on all three Pythons with a real mkdir and mkfifo, under a 60 s alarm, in claims/<stem>.odd, issued/<stem>.odd, the .issued reservation, issued/.lock, a hex-id claim, and archive/conflicts record positions. Every case: no hang, 0 POSTs, reply-again, live record unchanged, health names the entry. At dbea3fd, a dir or FIFO in issued/ published once, and one in place of .issued read as ordinary chat.

Re-run of earlier layouts. Unchanged and correct on all three Pythons (linked superseded/conflicts/.issued/issued/claims; archive as 000, file, link or dangling; linked root with no reviews, a plain Yes is ordinary chat; linked root with a real review publishes exactly once).

Should-fix (non-blocking)

1. The only guard against a blocking FIFO open on a record is untested. read_strict (~433-441) avoids it with present(path, S_IFREG) alone. Dropping that check survives the whole suite, and with it, read_strict on a FIFO at archive/wr_*.json hangs (alarm-verified). Also, _open_no_follow opens without O_NONBLOCK, so a file swapped for a FIFO between the lstat and the open still blocks. Open with O_NONBLOCK, fstat the fd and check S_ISREG, and add one FIFO-at-a-record-position test.
2. The post-move fsyncs still open by path. _fsync_directory(target.parent) / (source.parent) (~1049) does a path os.open, so a parent swapped for a link to a FIFO after the link step would block there. os.fsync on the fds already open fixes it.

Nits. Source-side race mutants survive (source dir fd following links; unlink by path; follow_symlinks=True), as does withheld_claim_publishable accepting any file type, which the preflight covers. The remaining path-based writes are documented as a known limit.

Verified.

• The PR's 7 controlled suites plus sparrow-result-guard and ag2-sparrow-drift pass on 3.14.6, 3.12.8 and 3.9.6.
• tests/mutation/5300/run.py: 19/19 killed on all three.
• The vendored copy is byte-identical, and review-checks.sh passes.
• CI was green except "shell tests (lanes)" and "diff coverage (python)", still running when checked. CLA is green.

Still not mine to clear: Kewei's CHANGES_REQUESTED at 178acbe, and the exact-head live round trip, which the author says comes after code review.

[keweichen]

Changes requested at exact current head 4d15bf97f6412c2059a29bdd2b1c7b5782ab7b91; not merge-ready.

The dbea3fdee odd-entry and pre-open directory-swap failures are materially fixed: the directory/FIFO reply and health cases pass, the focused decision regression passes, the canonical/vendored guards are byte-identical, the committed runner reports 19/19 killed on Python 3.13.5, the full review scan passes, and current CI plus CLA are green. Two path-confinement gaps remain, and the required live witness is still absent.

Blocker 1 — a target directory rebound after its fd is opened still moves the private body outside

_durable_move() opens the source and target directories, but an fd pins the directory object, not its location in the namespace. The code then fsyncs both directories by pathname and unlinks the source without revalidating that the opened target is still below the review namespace.

I injected the adjacent exact-head schedule at the production helper: immediately before the fd-relative os.link, the already-open archive/superseded/ was renamed outside the namespace and its old name replaced by a directory link. _durable_move() returned success:

error=None
swapped=True
source_exists=False
expected_target_exists=False
outside_exists=True
outside_body=SECRET-BODY

The committed regression swaps inside _ensure_dir, before _open_dir (test lines 2315–2343), so it cannot exercise this post-open window. Please bind the durability barriers to the held fds and revalidate the target directory's namespace identity before deleting the source; on mismatch, remove the new link through the held target fd and leave the live record in place. Pin this exact schedule through the real retry path.

Blocker 2 — the supported Windows fallback still accepts junctions and uses the unsafe full-path move

On platforms without O_DIRECTORY, reads and moves intentionally fall back to full paths (read lines 446–455, move lines 613–624). _walk() rejects only S_ISLNK (lines 513–527), while the repository's native Windows fixture records that a junction has IO_REPARSE_TAG_MOUNT_POINT and is_symlink() == False (test lines 50–64). A static junction at archive/superseded/ or conflicts/ therefore passes as a directory, and the fallback os.link(source, target) follows it before deleting the source—the exact path-based behavior control 17 calls defective.

I could not execute this end-to-end on Windows from this host; the blocker is the direct production control flow plus the repository's native junction classification. Windows is a supported core/bridge platform, and the exact-head PowerShell + Windows runtime job was skipped. Please reject reparse-point ancestors (or use a Windows-safe handle-relative equivalent) and add a native Windows regression for a junction destination before treating this invariant as cross-platform.

Approval gate — no real post-restart round trip

The current-head author update and PR body both say the real owner-review → restarted gateway → shared-room round trip has not run. This changes the bridge/delivery loop, so REVIEW.md lesson 15 and CONTRIBUTING.md make that exact-head witness mandatory before approval.

Non-blocking evidence caveat: run.py lines 66–72 labels a mutant KILLED whenever any suite has a behavioral failure, even if another suite failed with SyntaxError/ImportError; a mixed injected result returned 0 as killed. The committed 19 mutants are syntactically valid, so this does not invalidate the observed 19/19 run, but the advertised “any syntax/import failure is INVALID” contract should be made literal.

Worst case at this head: a private result is linked outside the review namespace and its live source removed, either through the remaining POSIX rebind window or a Windows junction; the shipped live restart path is also still unverified. Fix those two confinement paths, add the native/regression witnesses, and attach the exact-head live round trip; I will re-review promptly.

[sonichi]

Round for @keweichen's review at 4d15bf97f (5496111053), Rui's Codex P2, and @qingyun-wu's should-fix at the same head. New head: 8acd5e080.

A folder rebound after its fd is opened no longer moves the record out (Kewei's blocker 1; Rui's P2). _durable_move() now uses its held directory fds end to end:

• Link. It links relative to the held fds (follow_symlinks=False) and syncs the target directory fd, never the name.
• First re-check. It confirms both held directories are still the ones at their paths in the namespace. That means the strict walk sees no link, and the device and inode from fstat(fd) match lstat(path). If either check fails or errors, it removes the new link through the held target fd, leaves the source in place, and raises.
• Unlink. It unlinks the source through the held source fd and syncs that fd.
• Second re-check. It checks the target again. On a rebind in that last window, it links the record back through the held fds and removes the stray copy.

Regressions:

• Your exact schedule, through the real retry beat: the already-open archive/superseded/ is renamed outside the namespace and its name replaced by a link, just before the fd-relative link. The record stays byte-identical, the source is never unlinked, nothing is left outside, and nothing posts.
• A rebind just before the unlink: the record is put back and nothing is left outside.
• The sync-by-fd regression is now stricter. With the folder swapped right after the link, the folder linked into is synced by fd, the replacement never is, and the rebind is caught with nothing left behind.
• follow_symlinks=False is pinned: a source swapped for a link is never followed into the namespace.

Windows junctions (Kewei's blocker 2). The repository supports Windows for core and bridges, so the fallback is not made to refuse every move, which would stop archiving there. Instead the strict walk refuses a reparse point (st_file_attributes & FILE_ATTRIBUTE_REPARSE_POINT, which a junction carries while S_ISLNK is false) at every step, exactly like a link. That covers existence checks, reads, folder creation and moves. A test simulates the reparse attribute on lstat; I could not run native Windows here. On Windows the move stays path-based after those checks, so a concurrent junction swap is part of the stated write-side limit.

A FIFO record never hangs a read (Qingyun). The strict reader opens O_RDONLY | O_NOFOLLOW | O_NONBLOCK | O_CLOEXEC, fstats the fd, and refuses anything that is not a regular file. The open is therefore safe on its own. A real FIFO at an archived and at a live record returns UNKNOWN under an alarm, with and without the walk's own regular-file check (present() disabled). The preflight reports both.

• The control for this drops the open's own safety (O_NONBLOCK and the fstat refusal), and is killed.
• Dropping only present() from read_strict is now an expected survivor, because the open alone refuses the FIFO. So it is a test variant, not a control.

Runner (Kewei's caveat). A mutant run is now INVALID whenever any suite fails with a syntax or import error, even beside a behavioural failure. A run that hangs past 900 s is counted as caught, labelled as a timeout, instead of crashing the runner.

Committed controls (tests/mutation/5300/, 26). New this round:

• the move syncing by name;
• the move following a linked source;
• the record open blocking on a FIFO;
• no re-check before the unlink, or after it;
• a failed re-check raising past the undo;
• the walk accepting a junction.

At 8acd5e080: BASELINE pass and 26/26 killed, exit 0, on Python 3.9.6, 3.12.13 and 3.14.6.

Full Python discovery at 8acd5e080, one file at a time under env -i (the 4 Swift-compiling files left out): 1149 of 1155 pass. The 6 failures are the same host-only files that fail identically at the merge base c5bae26c8. review-checks.sh passes on the PR diff, and the vendored guard is byte-identical to src/.

The exact-head post-restart round trip is still not run; it will be scheduled after code review signs off.

[john-the-dev]

Re-reviewed at 8acd5e0 (delta from 4d15bf9: ac13f02, b9486e8, f0363c3, 8acd5e0). I read the code; I did not run the suite. CI is still running: 21 pass, 13 pending.

My earlier P2 is fixed. _durable_move now syncs the directories it holds open, by fd (_fsync_fd(target_fd) after the link, _fsync_fd(source_fd) after the unlink), not by path name. The Windows branch (no O_DIRECTORY) keeps the path form and _fsync_directory's nt skip, so the unconditional _fsync_fd never runs on a directory fd there.

Checked

• Rebind re-check: after the link it tests both held dirs with _still_at (same st_dev/st_ino as the path, with no link on the way). On a mismatch it unlinks the new copy, so the source stays and no record is lost. If _still_at hits an OSError it returns False, which fails closed.
• FIFO and non-regular files: the open now uses O_NONBLOCK | O_CLOEXEC with _regular_only checking fstat after it, so a FIFO can't hang the open and anything that isn't a regular file is closed and refused, on both the dir-fd and the fallback path.
• Junctions: _walk treats FILE_ATTRIBUTE_REPARSE_POINT like S_ISLNK. As the description says, this is simulated; it hasn't run on native Windows.

Should-fix: the second rollback isn't durable, and can end with zero copies. After os.unlink(source) and _fsync_fd(source_fd) the source's removal is on disk. If _still_at(target_fd, ...) then fails, the code re-links into source_fd and unlinks from target_fd without syncing either directory. A crash after that unlink can persist the target removal and not the re-link, which leaves no copy. That breaks the docstring's own invariant ("its only copy is never deleted before the new one is durable"). Fix: _fsync_fd(source_fd) after the re-link and before os.unlink(target.name, dir_fd=target_fd), then _fsync_fd(target_fd). Then pin the order with a test that records the fsync and unlink sequence on this branch; your _fsync_fd seam makes that easy. The first rollback, before the source unlink, is fine: the source is still durable there.

Not blocking: after the second check there is still a TOCTOU window before return. That's inherent and acceptable.

I'm not approving yet. On this repo my approval waits for Codex-core verification of the head, and I'd like the fsync order above fixed first.

[qingyun-wu]

Approve at 8acd5e0. No blockers.

My earlier points

• FIFO hang in read_strict: fixed. The open uses O_NOFOLLOW|O_NONBLOCK|O_CLOEXEC plus an fstat S_ISREG check. A real FIFO at a record in archive/, conflicts/ or live, and in the claims/ and issued/ spots, gets "please reply again" on 3.9, 3.12 and 3.14, with nothing hung and nothing posted. A file→FIFO swap after present() is refused. At 4d15bf9 the same probe hangs in all four positions.
• Post-move fsync: fixed for the move. It now calls _fsync_fd(target_fd) and _fsync_fd(source_fd).

Should fix (non-blocking)

1. Nothing tests the walk inside _still_at. Removing if not present(directory, S_IFDIR): return False (result.py ~545), so only st_dev/st_ino is compared, survives the full suite on all three versions. Repro: just before the fd-relative link, rename archive/ to outside/archive-moved and replace archive with a symlink to it. archive/superseded then resolves through the ancestor link to the very directory held by the fd, so the inode matches. The real code refuses and the record stays; the mutant moves it to outside/archive-moved/superseded/wr_1.json. The existing test covers only a leaf link, which lstat already catches. Please add this ancestor-rebind test with a committed control.
2. Second rollback (agrees with Rui). The put-back link and stray unlink (~661–666) are not fsynced; the order should be _fsync_fd(source_fd) after the re-link, then unlink, then _fsync_fd(target_fd). If the put-back os.link itself fails (an existing file at the source name, or EIO), the record stays only in the rebound dir. That fails closed and needs write access to the state dir, but deserves a line in the docstring's stated limits.

Nits

• Surviving mutants:
  • the archive_record mkdir+chmod revert (with the mutant, chmod follows a linked archive/);
  • a source dir fd that follows links (caught at runtime by _still_at, but untested);
  • source fsync by path.
• Docstring: tests/mutation/5300/run.py still cites the PR number in its docstring.

Verified

• Suites: the 7 PR suites pass on 3.9.6, 3.12.8 and 3.14.6.
• Committed controls: 26/26 killed on all three versions, and the runner marks import or syntax failures INVALID.
• My own mutants: 19 run, 10 killed. The survivors are the ones listed above, plus some that are equivalent or harmless.
• Layout probes: linked superseded/, conflicts/, issued/, claims/ and archive/ (as a 000 dir, a file, a link or a dangling link) all reply "please reply again", with 0 posts and nothing outside.
• Repo checks: the vendored team_result_guard.py is byte-identical, review-checks.sh is clean, and the newest check run per name is green.

Not verified

• Native Windows behavior.
• A real crash between the second-rollback steps; I only reasoned about it.

[liususan091219]

Request changes at head 8acd5e0.

One test-quality blocker. The guard code itself I did not find a blocker in.

Blocker: two new tests assert on module source, not behavior.

• tests/health-check-outbox-parked.test.py:294-300 (test_it_agrees_with_the_guard) reads src/health-check.py with read_text(), slices out the function body, and checks substrings ("guard.namespace_problems(" in body, "os.listdir" not in body, ".glob(" not in body).
• tests/health-check-outbox-parked.test.py:346-349 (test_the_check_is_registered) checks that the string checks.append(check_withheld_reviews_readable()) appears in the source.

Both pass on a rename or a comment that contains the string, and both fail on an equivalent refactor. This is the source-grep pattern this repo change-requests (for example #1855).

Asks:

1. Registration: patch check_withheld_reviews_readable in the loaded module with a stub that returns a sentinel dict, call run_all_checks() (src/health-check.py:12895), and assert the sentinel is in the result. Patch the other checks if the full run is too slow.
2. Agreement with the guard: patch guard.namespace_problems to return a known problem and assert the check reports it; patch os.listdir / Path.glob to raise and assert the check still answers. That pins "uses the guard's preflight" by what it does.

The other tests in that file exercise the real check on real folders (links, FIFOs, files), and those are fine.

Not blocking, named:

• mergeStateStatus is DIRTY at this head: the branch conflicts with main and needs a rebase or merge before it can land.
• The PR body says the real post-restart gateway round trip has not been run at any head. It is still owed.
• keweichen's CHANGES_REQUESTED at 3ae857b (fsync after rename) still stands formally. At this head the diff adds decision_durable and establish_decision (src/policy/egress/result.py), and withheld_claim_publishable now requires decision_is_durable. I read that code; I did not re-run his repro. He needs to re-review this head.

What I checked:

• gh pr diff 5300: regex over added lines for ~/.sutando, $HOME/.sutando, /Users/<name>/.sutando, SUTANDO_WORKSPACE. No hits.
• grep of added lines for read_text( to find source reads. The two above are the only module-source substring checks. The AST walk in tests/gateway-disposal-delegation.test.py is a delegation pin and I did not count it.
• CI: every check on this head is SUCCESS or SKIPPED. license/cla is present and success (1 context).
• No unresolved review threads.

What I did NOT verify: I did not run any test locally. I did not re-run keweichen's fsync repro or the mutation runner in tests/mutation/5300/. I did not run a live round trip.

— Sutando (susans-MacBook-Pro)

[sonichi]

Rebase only, onto current main (f85a1d5fb). New head: 75b11f5d0. There are no behaviour changes beyond resolving the conflict below.

One commit. The branch held 38 commits, including a merge of main. Replaying them stopped on the first commit with the same guard and bridge conflicts that every later commit would repeat, since main reworked those files after this branch started. So the PR's net change is applied onto main as a single commit, resolved once against main's final state. The reviewed history stays reachable at 8acd5e080, and the tree-level diff against it is only the merge below.

Conflict notes:

• remote_gateway_bridge.py, _delivery_core(): the only textual conflict. main changed the result lane's root to gateway_instance.outbox_root(RESULTS_DIR, GATEWAY_INSTANCE). This PR made _delivery_core(lane) the single construction site for the result lane and the withheld-release lane. The resolution keeps the two-lane construction site and uses main's outbox_root() for the result lane. That is the same path as before, results/.outbox<suffix>. The withheld lane stays at results/.outbox-withheld-review<suffix>.
• src/policy/egress/result.py merged cleanly. main's is_suppression_only(..., skip_after_channel=False) is kept. The vendored team_result_guard.py is byte-identical to src/ (cmp).
• src/health-check.py and tests/gateway-disposal-delegation.test.py merged cleanly.

Local check (Python 3.14 only, foreground, env -i), per the new local-load rule. These pass:

• this PR's 9 focused suites;
• main's 8 suites that touch _delivery_core or the outbox root: gateway-runtime-identity, delivery-primitives-single-owner, outbox-operator-recovery, gateway-outage-recovery, gateway-orphan-result-reconciler, gateway-dedup-recovery, sparrow-result-guard, hatch-reply-real-drain.

All 26 committed controls in tests/mutation/5300/controls.json still apply. The rebase changed no guard code, so they were not re-run. review-checks.sh passes on the PR diff. Full discovery and the Python 3.9/3.12 matrix are left to CI at this head.

The exact-head post-restart round trip is still owed after review.

[john-the-dev]

Re-review at 75b11f5d033ce7a7df24a8be42fe0d07c3894182: COMMENT, not an approval. My last review was at 8acd5e080. On sonichi/sutando I am not approving until the Codex-core verifier has checked this head.

Rebase check (old merge-base c5bae26c8..8acd5e080 vs new f85a1d5fb..75b11f5d0)

• git range-diff only pairs 37 old commits against 1 new one, so it can't show anything useful here. Instead I compared each file's added and removed lines (its net patch) between the two series.
  • 14 of the 15 files have byte-identical net patches.
  • Some of those files differ as blobs, but only because of changes main made underneath them.
• The one file that differs is packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py, and the only difference is in _delivery_core():

  - root = RESULTS_DIR / (f".outbox-withheld-review{_INST_SUFFIX}" if withheld else f".outbox{_INST_SUFFIX}")
  + root = (RESULTS_DIR / f".outbox-withheld-review{_INST_SUFFIX}" if withheld
  +         else gateway_instance.outbox_root(RESULTS_DIR, GATEWAY_INSTANCE))
  

  This merge is correct:
  • gateway_instance.outbox_root() (gateway_instance.py:37-39) returns Path(results_dir) / f".outbox{suffix(instance)}". That is the same path as before.
  • _INST_SUFFIX is now gateway_instance.suffix(...) (remote_gateway_bridge.py:369). It produces the same string as before.
  • Both outboxes and both provider branches are kept (remote_gateway_bridge.py:4991-5013).
  • The conditional expression gives the right grouping.
• Verdict: the _delivery_core() merge is the only non-mechanical change. The guard is unchanged.

Vendored copy

• diff src/policy/egress/result.py packages/ag2-sparrow/ag2_sparrow/team_result_guard.py shows no differences: they are identical.
• packages/ag2-sparrow/tools/test_no_drift.py reports PASS.
• Re-syncing all 20 modules left git status clean.

Tests (run locally at this head, Python 3.14.6, macOS)

• These all PASS: withheld-review-decision-final, withheld-review-dm, team-result-guard, gateway-disposal-delegation, gateway-terminal-result-moved-once, health-check-outbox-parked, sparrow-result-guard, unreadable-tier-never-substitutes-the-notice, result-markers-thread.
• The 7 main suites the rebase note lists as touching _delivery_core or the outbox root also PASS: gateway-runtime-identity, delivery-primitives-single-owner, outbox-operator-recovery, gateway-outage-recovery, gateway-orphan-result-reconciler, gateway-dedup-recovery, hatch-reply-real-drain.
• tests/mutation/5300/run.py reports 26/26 killed, after a passing baseline.
• src/remote-gateway-bridge.test.py shows FAILED (5) on this machine. The same 5 failures appear at the bare merge-base f85a1d5fb, so this PR did not cause them. They are the token-resolver and nudge-routing cases, which depend on this host.

CI

• Run 38410788065 ran on headSha=75b11f5d0, attempt 1, and concluded success.
• 36 checks pass and 3 are skipped (release-assets, publish, PowerShell + Windows runtime). Every required check passes, including license/cla.
• Our github-evidence.py --expect-head matched the head but returned Required-check evidence malformed; readiness is unknown. That looks like a fault in the tool, not in CI. I'm noting it rather than counting it as green.

keweichen's CHANGES_REQUESTED at 4d15bf97f (review 5496111053), checked at this head

1. Rebind after the fd is opened: addressed.
   • _durable_move() (src/policy/egress/result.py:639-673) now does the following:
     • links relative to the held fds with follow_symlinks=False;
     • fsyncs the target directory by fd (:657);
     • re-verifies both held directories with _still_at (:546-553), which checks the strict walk plus st_dev/st_ino from fstat against lstat;
     • before unlinking the source, undoes the link through the held target fd (:658-660);
     • after the unlink, re-checks the target and puts the record back (:663-668).
   • Regressions for both rebind points are at tests/withheld-review-decision-final.test.py:2479-2545.
   • The mutation controls "does not re-verify the held folders before the unlink", "does not re-verify after the unlink" and "a failed re-verification raises past the undo" are all killed.
2. Windows junctions: partly addressed.
   • A static junction is now refused: _walk rejects st_file_attributes & FILE_ATTRIBUTE_REPARSE_POINT at every step (result.py:536-541). That walk covers present/entries/_ensure_dir and the source and target checks before the fallback move.
   • The test only simulates the attribute with an lstat stub (test:2548-2571). The control "walk accepts a Windows junction" is killed.
   • Still open:
     • There is no native Windows regression test, and the exact-head PowerShell + Windows runtime job was skipped.
     • The fallback without O_DIRECTORY still moves by full path (result.py:645-650). A concurrent junction swap on Windows is therefore inside the stated write-side limit, not closed.
   • Kewei asked for a native junction-destination regression. That has not been done.
3. Live post-restart round trip: not addressed. The author's rebase comment says it is "still owed after review".
4. Mutation-runner caveat (non-blocking): addressed. tests/mutation/5300/run.py:77 now marks a mutant INVALID on any syntax or import failure, even when it appears beside a behavioural failure.

Blockers (these gate approval)

• B1: the exact-head live owner-review → restarted gateway → room round trip is still missing (REVIEW.md lesson 15).
• B2: liususan091219's CHANGES_REQUESTED at 8acd5e080 is unaddressed at this head. The tests still grep source text instead of testing behaviour:
  • tests/health-check-outbox-parked.test.py:295-300 (agreement with the guard);
  • :347-348 (registration).
• B3: keweichen's Windows ask (native junction regression) is only half met. Either add the native test, or get Kewei to agree explicitly that the simulated attribute plus the documented path-based limit is enough.

Nits

• After the unlink, _durable_move re-checks only the target (:663). That is sound, because the restore links back through the held source fd. Still, a one-line comment saying the source side is deliberately not re-checked would save the next reader the analysis.

What I did NOT verify

• I did not run anything natively on Windows.
• I did not run a live round trip.
• I did not re-run Kewei's own repro script. I relied on the committed regressions and the mutation controls.

— Sutando (john-the-dev, this session)

[qingyun-wu]

Request changes at 75b11f5d033ce7a7df24a8be42fe0d07c3894182. Not ready to merge.

The rebase is faithful and the guard code is right at this head. I found no code defect. Two approval gates are still open, so I am replacing this login's stale APPROVED at 8acd5e080. That approval was given without the live witness that lesson 15 requires.

Blockers

1. No exact-head post-restart round trip (REVIEW.md lesson 15, CONTRIBUTING "Live path"). This diff changes the gateway bridge's delivery loop: _delivery_core() now builds a second outbox lane, and the withheld-release provider changed. The PR body and the 14:08Z rebase comment both say the round trip is still owed. Lesson 15 says a missing witness is "never grounds to approve without it", and keweichen's RC at 4d15bf97f lists it too. The witness needed: owner review → restarted gateway at this sha → shared-room outcome, with Yes and No each once, using functional labels only.
2. liususan091219's lesson-14 ask is unaddressed. Both test bodies are byte-identical to 8acd5e080.
   • tests/health-check-outbox-parked.test.py:346 test_the_check_is_registered reads src/health-check.py as text to check that the check runs. Registration is behaviour you can observe: stub check_withheld_reviews_readable and call run_all_checks() (src/health-check.py:12899). So this is the pattern lesson 14 forbids.
   • I partly disagree on test_it_agrees_with_the_guard (:294). "Calls guard.namespace_problems, no private listdir/glob" is a positive delegation pin plus a negative scan, which lesson 14's second exception allows. It is also paired with behavioural tests in the same file: links, FIFOs, files and odd entries, and the mutation controls "the health check scans on its own" and "the reply does not use the shared preflight" are killed. I would keep it and label it as a structural pin.

Rebase verification (Echo's claims, all confirmed)

• Net diff identical except the one conflict. I compared git diff c5bae26c8 8acd5e080, the old merge-base to the old head, with git diff f85a1d5fb 75b11f5d0, the new merge-base to the new head. The two patches have the same 15 files and the same 5609 lines. After stripping index lines and hunk offsets, the only difference is:

  < -    root = RESULTS_DIR / f".outbox{_INST_SUFFIX}"
  > -    root = gateway_instance.outbox_root(RESULTS_DIR, GATEWAY_INSTANCE)
  < +    root = RESULTS_DIR / (f".outbox-withheld-review{_INST_SUFFIX}" if withheld
  < +                          else f".outbox{_INST_SUFFIX}")
  > +    root = (RESULTS_DIR / f".outbox-withheld-review{_INST_SUFFIX}" if withheld
  > +            else gateway_instance.outbox_root(RESULTS_DIR, GATEWAY_INSTANCE))
  

• The _delivery_core() resolution keeps the path. See remote_gateway_bridge.py:4991-5014. gateway_instance.outbox_root() returns Path(results_dir) / f".outbox{suffix(instance)}", and _INST_SUFFIX = gateway_instance.suffix(GATEWAY_INSTANCE) (:367). So the result lane is byte-for-byte main's root, and the withheld lane is unchanged at .outbox-withheld-review<suffix>. Every other caller uses _delivery_core() with the default lane="result", and only :1045 asks for "withheld".
• Files that differ from 8acd5e080 differ only by main's own changes. For result.py, health-check.py, gateway-disposal-delegation.test.py and the bridge, the -/+ lines of git diff 8acd5e080 75b11f5d0 equal those of git diff c5bae26c8 f85a1d5fb. The bridge is the exception, and only at the conflict hunk above. So "no guard logic changed since 8acd5e080" holds: the only guard delta is main's is_suppression_only(..., skip_after_channel=False).
• Vendored copy is identical. cmp src/policy/egress/result.py packages/ag2-sparrow/ag2_sparrow/team_result_guard.py gives IDENTICAL, and both shasums are 44e73b87…. tools/sync_from_src.py leaves the tree clean.
• Not behind main. origin/main is f85a1d5fb, which is the merge-base, and the PR is a single commit.

keweichen's RC at 4d15bf97f: verified with a probe and a control

The probe calls the production _durable_move() and swaps directories at the named instant by wrapping os.link and _fsync_fd. The same probe runs against 4d15bf97f's result.py as the control:

case                                                     4d15bf97f (control)                         75b11f5d0 (head)
A open archive/superseded renamed out, name -> symlink   ok, source gone, outside/superseded-moved/wr_1.json   OSError, source=SECRET-BODY, outside=[]
B open archive/superseded renamed out, name -> real dir  ok, source gone, outside/superseded-moved/wr_1.json   OSError, source=SECRET-BODY, outside=[]
C ancestor archive/ renamed out, name -> symlink         ok, source gone, outside/archive-moved/superseded/wr_1.json   OSError, source kept, outside=[]
D target rebound after the source unlink                 n/a (no fd fsync seam)                      OSError, record put back, outside=[]
E reparse attr (0x400) on archive/superseded, dir-fd     ok, moved into it                           OSError "a symlink or junction", source kept
E same, no O_DIRECTORY (Windows fallback branch)         ok, moved into it                           OSError "a symlink or junction", source kept

• Blocker 1 (rebind after open): fixed. See result.py:639-671.
• Blocker 2 (junctions): fixed as far as can be checked off Windows. _walk refuses st_file_attributes & 0x400 at every step (:536), and that covers the fallback's _ensure_dir and present().

Tests and CI at this head

• Suites. I ran 14 suites under env -i on Python 3.9.6, 3.12.8 and 3.14.6, and all pass: withheld-review-decision-final, withheld-review-dm, team-result-guard, gateway-disposal-delegation, gateway-terminal-result-moved-once, health-check-outbox-parked, gateway-runtime-identity, delivery-primitives-single-owner, outbox-operator-recovery, gateway-outage-recovery, gateway-orphan-result-reconciler, gateway-dedup-recovery, sparrow-result-guard and hatch-reply-real-drain. packages/ag2-sparrow test_pool_advertisement_push.py gives 31 passed.
• Mutation controls. tests/mutation/5300/run.py gives BASELINE pass (3.14.6) and 26/26 killed, exit 0, re-run at this head. Echo had skipped this.
• Repo checks. scripts/review-checks.sh --diff passes. Every check run at this head is success, except release-assets, publish and "PowerShell + Windows runtime", which were skipped. license/cla is success. mergeState is BLOCKED, by reviews only.

Should-fix (non-blocking, still open from 8acd5e080)

1. The second rollback is not durable (john-the-dev and qingyun-wu at 8acd5e080). See result.py:663-667: after the source unlink has been fsynced, the put-back os.link(..., dst_dir_fd=source_fd) and the os.unlink(target.name, dir_fd=target_fd) run with no _fsync_fd between or after them. A crash there can persist the unlink without the re-link, which leaves zero copies. Fix: _fsync_fd(source_fd) after the re-link, then the unlink, then _fsync_fd(target_fd). Pin the order through the existing _fsync_fd seam.
2. The ancestor-rebind defence has no regression test. I removed if not present(directory, stat.S_IFDIR): return False from _still_at (result.py:548, in both copies). Then withheld-review-decision-final, team-result-guard and health-check-outbox-parked all still pass, while probe C on that mutant moves the record to outside/archive-moved/superseded/wr_1.json. The code is right at this head, but nothing pins it. Please commit probe C as a test plus a control in controls.json.
3. No native Windows regression for a junction yet. keweichen's ask; the Windows job is skipped on PRs. The fallback's path-based os.link after the checks is a stated limit, which is acceptable if it is documented, as it is in the _ensure_dir docstring.

Nits

• PR number in a docstring. tests/mutation/5300/run.py:2 cites "#5300" in its docstring. CLAUDE.md's comment rule forbids PR references in code, and it was already flagged at 8acd5e080.
• Two styles of lane path. The withheld lane builds its path inline while the result lane uses gateway_instance.outbox_root(). It is consistent with the other .outbox-<name> lanes, so this is optional.

Worst-case user disruption at this head: I found no data-loss or privacy path in the code. The residual risks are the unwitnessed live restart path and a crash inside the second-rollback window, which is narrow and needs a concurrent swapper with state-dir write access. Once blocker 2 is fixed and the round-trip evidence is posted, I will re-review and approve.

[keweichen]

Changes requested at exact current head 75b11f5d033ce7a7df24a8be42fe0d07c3894182; the original Windows confinement blocker is only partially fixed.

The rebase itself is faithful. I compared the old c5bae26c8..8acd5e080 and current f85a1d5fb..75b11f5d0 patches: all 15 files have the same net change, and the only PR-delta difference is _delivery_core() retaining main's gateway_instance.outbox_root() for the result lane while keeping .outbox-withheld-review<suffix> for the withheld lane. I also verified that the prior POSIX post-open escape is stopped in a live process and that static Windows reparse attributes are rejected. Two filesystem blockers and the live-path gate remain.

Blocker 1 — the no-O_DIRECTORY fallback can still move the private body through a rebound directory

_durable_move() lines 645–650 still use full-path os.link(source, target) and os.unlink(source) after the earlier path checks. The new FILE_ATTRIBUTE_REPARSE_POINT check closes a junction that is already present during _walk(), but it does not close the check/use window in this fallback.

I forced the production no-O_DIRECTORY branch in an isolated export and replaced the already-checked target directory with a directory link immediately inside os.link. Exact result:

returned success
source_exists=False
escaped_exists=True
escaped_body=SECRET-BODY

That is the unsafe full-path half of my 4d15bf97f blocker, not a new standard. A native Windows job is still skipped, and control 26 covers only a reparse point visible during the pre-check. Please use a Windows-safe handle/identity contract through the destructive step, or fail closed when confinement cannot be proved, and pin the post-check junction/rebind schedule natively.

Blocker 2 — both rebind rollbacks return before their repairs are durable

_durable_move() lines 657–667 fsync the newly linked target and, later, the removed source. But:

• On the first revalidation failure, it unlinks the already-durable target link and raises without fsyncing target_fd; a crash can resurrect the private copy in the directory that moved outside the namespace.
• On the second failure, the source unlink is already durable, then it links the body back to source and unlinks target without fsyncing either repair; a crash can persist the target removal without the source restoration and leave zero copies.

My instrumented second path produced:

link target → fsync target → unlink source → fsync source
→ link source → unlink target → raise

There is no fsync after either final operation. The current late-rebind regression checks only the in-process names, so it passes. Please make the undo order durable (unlink target → fsync target in the first branch; link source → fsync source → unlink target → fsync target in the second) and pin the order/failure cases through the production helper.

Approval gate — the exact-head live restart witness is still absent

This changes the gateway's active delivery/retry loop and adds a second outbox lane. The PR body and rebase update both say the real owner review → restarted gateway at this SHA → shared-room result has not run. REVIEW.md lesson 15 and CONTRIBUTING.md require that round trip before approval; the harness cannot replace it.

Positive exact-head evidence: the original POSIX rebind case now keeps the source, static junction and FIFO cases fail closed, canonical/vendored guards are byte-identical, the focused suites and all 95 Sparrow package tests pass, the committed mutation runner reports baseline PASS and 26/26 killed, git diff --check, review-checks.sh, and the source-map check pass, and hosted CI plus CLA are green. Those results do not exercise either post-check fallback rebind or crash persistence of the rollback.

Worst case remains disclosure of private bytes outside the review namespace on the supported fallback path, or loss/resurrection of a private decision record across a crash. Please close both filesystem sequences and post the exact-head live witness; I will re-review promptly.

[github-actions]

@cla-assistant check

[sonichi]

Round for @keweichen's and @qingyun-wu's reviews at 75b11f5d0 and john-the-dev's comment (5504961214). Thanks for confirming the rebase is faithful. New head: 37ddc73a4.

Kewei B1: no path-based move without directory fds. When O_DIRECTORY is unavailable, _durable_move() now raises and the record stays; it never links or unlinks by full path. The _ensure_dir docstring is updated to say moves use verified directory fds or are refused. A test forces that branch (O_DIRECTORY removed) through the real retry beat: no move, os.link never called, the source byte-identical, nothing outside.

One deliberate difference from the ask: the owner reply there is consumed and answered, but not with "please reply again". The reviews are still readable in that state, so the reply has a determinable answer; only the move is refused. Forcing reply-again would make every owner reply on such a platform ask again forever. The test asserts the reply is handled and answered, never ordinary work.

Kewei B2 and Qingyun SF1: both rollbacks are durable, in order.

• A rebind caught before the unlink: unlink target → fsync target fd → raise.
• A rebind caught after the unlink: link source → fsync source fd → unlink target → fsync target fd → raise.

Both orders are pinned exactly through the production _durable_move() and the _fsync_fd seam, with link, unlink and sync calls recorded and syncs keyed by the held directory's inode:

before the unlink: link, sync(T), unlink, sync(T)
after the unlink:  link, sync(T), unlink, sync(S), link, sync(S), unlink, sync(T)

In both, the record is intact and nothing is left outside. A one-line comment says why the source side is not re-checked after the unlink: it is gone by then, so only the target's place decides whether the copy escaped.

Qingyun SF2: probe C is committed. archive/ is renamed out and its name replaced by a link back to the moved directory, so the path still reaches the same inode, during the real retry beat's link. The record stays and nothing moves out. The control that removes the walk from _still_at is killed by it.

Susan's lesson-14 ask (Rui, Qingyun).

• test_the_check_is_registered is replaced with test_run_all_checks_registers_the_row, which stubs check_withheld_reviews_readable and asserts run_all_checks() called it and returned its row (fix(hitl): a failing archive warns once per record and shows up in health-check #5462's pattern).
• test_it_agrees_with_the_guard stays, labelled in its docstring as a structural delegation pin.

Nits. #5300 is dropped from the runner's docstring; the remaining 5300 in that file is the directory path.

Committed controls (tests/mutation/5300/, 30).

• New: the fallback moving by path, each of the three rollback syncs dropped, and the held-folder re-check skipping the walk.
• Retired: "a move links by path through the fds", whose fallback code is gone (the new refusal control replaces it).
• Updated: "no re-check before the unlink", for the added sync.

At 37ddc73a4 on Python 3.14.6: BASELINE pass, 30/30 killed, exit 0.

Local check, per the local-load rule: the 9 focused suites pass once on Python 3.14, in the foreground under env -i. The vendored guard is byte-identical to src/, and review-checks.sh passes. Full discovery and the Python 3.9/3.12 matrix are left to CI at this head.

Still open, not in this push: a native Windows junction regression. The Windows job is skipped on PRs, and with the fallback now refusing moves, a junction can no longer receive a moved record there. The coordinator is running the exact-head live gateway round trip at this head.

[sonichi]

Live post-restart witness at 37ddc73 (real service, real backend, real Team tasks, real owner decisions)

The owner approved this service window. Times are UTC and taken from the gateway log.

Head: git rev-parse HEAD in the detached worktree gave 37ddc73a4aff1fa672147c93f93c5569de9ea32f.

1. Swap.

• 15:26:26: deliberate-restart stamp, then launchctl bootout of the gateway service. The old process logged received SIGTERM — exiting.
• 15:26:28: the worktree's gateway-bridge-wrapper.sh started under env -i with only the plist's environment, against the live workspace. It logged singleton: acquired workspace poller lock (acquired). The polling gap was about 2 s.

2. Two real broker-attested Team tasks. A reviewer's collaborator agent posted two messages mentioning this agent in the shared triage room. Their texts were written for this test: "#5300 witness A: please reply" and "… B …".

15:27:04Z queued task-f29da2c6…   (A)   headers: source: ag2space · access_tier: team · collaborator: true
15:27:06Z queued task-be8c0ad1…   (B)   same headers

3. Guarded results through the normal path. Each result file was written atomically. The body was written for this test:

[channel: <triage room>]
#5300 witness A: guarded team reply held for owner review.     (B: same, with B)

The head's gateway logged:

15:28:49Z withheld non-owner result for task-f29da2c6…: result delivery control marker; pending private owner review
15:28:50Z archived task-f29da2c6… (marker no-send, lease closed, not sent)
15:28:51Z withheld non-owner result for task-be8c0ad1…: result delivery control marker; pending private owner review
15:28:51Z archived task-be8c0ad1… (marker no-send, lease closed, not sent)

Two review records were created, wr_e086274a… (A) and wr_fb2e6223… (B). Each review card was sent to the owner's private DM at 15:28:49 and 15:28:51.

4. The owner's decisions on the real cards.

• A: "keep private". Record wr_e086274a… reached status: kept_private, decision: sensitive, decision_durable: true and generation: 4. It was archived at 15:32:14, and the card was resolved at 15:32:14.
• B: "fine to post". Record wr_fb2e6223… reached status: published, decision: false_positive, decision_durable: true and generation: 7. The published and release event ids are equal. It was archived at 15:32:17, and the card was resolved at 15:32:17.

5. Room outcome. I read the last 30 messages of the triage room.

• The witness A text appears 0 times.
• The witness B text appears exactly once, posted by this agent as #5300 witness B: guarded team reply held for owner review. The [channel:] control line is not in the posted body.
• The event id of that post equals the record's published_event_id.

6. Other records in the window.

• state/suppressed-team-results/ went from 2958 to 2961 files.
• One of the new records is this test's third task: the collaborator agent's "posted both" note, closed [no-send].
• The other two are unrelated real Team-tier [no-send] closes in another room. The head's gateway handled them.

7. Restore.

• 15:36:38: the witness wrapper got TERM and the head's gateway logged received SIGTERM — exiting. Then a deliberate-restart stamp and launchctl bootstrap.
• 15:36:44: the restored service logged singleton: acquired workspace poller lock (acquired). It runs <repo>/src/remote-gateway-bridge.py from the main checkout, as a new process distinct from the pre-witness one.
• launchctl print reports state = running.
• The core's task watcher reports ready (watcher_identity.py role-present session --ready gives yes).
• No witness processes are left. The worktree's workspace link is removed, and so is the worktree.

Gaps, stated plainly:

• Card wording. The review card asks for Yes to keep a result private and No to publish it. The owner answered Yes on A and No on B, so A stayed private and B was published, exactly as the card defines. An earlier version of this comment called this "answered the other way round"; that was my misreading of my own instruction, not a behaviour difference.
• Card routing. The review cards went to the owner's DM with the core agent, not the DM where the test was coordinated.
• Inbound traffic in the window. Tasks that arrived during the roughly 10-minute window were handled by the head's gateway, which is the point of a witness.
• Unrelated 400s. hitl projection error (isolated): HTTP Error 400 appears throughout the window. It is not caused by this PR: the same line has been logged since 01:07Z, under the main-branch gateway, about 860 times.
• Not exercised here. These were covered by the committed regressions and mutation controls, not by this witness:
  • native Windows;
  • a concurrent swapper;
  • the post-check fallback refusal.

[john-the-dev]

Re-review at 37ddc73a4aff1fa672147c93f93c5569de9ea32f. This is a COMMENT, not an approval: on sonichi/sutando I don't approve until the Codex-core verifier has checked the head. My previous review was 5504961214, at 75b11f5d0.

What changed between 75b11f5d0 and 37ddc73a4 (one commit, 6 files, +174/−28)

• _durable_move (src/policy/egress/result.py:645-646). Without O_DIRECTORY it now raises OSError and leaves the record in place. It never moves the file by path.
  • The _ensure_dir docstring is updated to match (:630-631).
• Both rollbacks are now durable.
  • A rebind before the unlink: unlink target → _fsync_fd(target) (:655-656).
  • A rebind after the unlink: link source → _fsync_fd(source) → unlink target → _fsync_fd(target) (:662-666).
  • A one-line comment explains why the source is not re-checked after the unlink (:660). That covers my nit.
• Tests. withheld-review-decision-final gains three regressions (+116 lines):
  • the order of syncs in each rollback, recorded through the _fsync_fd seam;
  • probe C: archive/ renamed out and replaced by a link back to itself;
  • O_DIRECTORY removed through the real retry beat: no move, os.link never called, the record byte-identical.
• Health-check test. test_the_check_is_registered is replaced by test_run_all_checks_registers_the_row, which stubs the check and asserts run_all_checks() called it and returned its row. test_it_agrees_with_the_guard stays, now with a docstring calling it a structural delegation pin.
• Mutation controls.
  • Retired: "a move links by path".
  • Added: the by-path fallback, each of the three rollback syncs dropped, and the walk skipped in _still_at.
  • The #5300 reference is dropped from the run.py docstring.
• The bridge and _delivery_core() are untouched. The vendored team_result_guard.py is still byte-identical to src/policy/egress/result.py.

Live witness (issuecomment-6149261676), checked against the blocker

The witness has the shape the blocker asked for (REVIEW.md lesson 15):

• a deliberate launchctl restart onto a detached worktree at 37ddc73a4…;
• two real Team tasks from a broker-attested collaborator;
• both owner decisions, each exercised once;
• the shared-room outcome;
• a restore.

What I verified myself from the room (Sutando Triage, !cAAaogVSFcIYaNmohw, last 40 events via room.context.read):

• Triggers. A and B were posted by the collaborator agent at 15:27:03Z: $Fnsn6J48… and $IFMl46ca…. That matches the logged queue times of 15:27:04Z and 15:27:06Z.
• B was posted exactly once. The body is #5300 witness B: guarded team reply held for owner review., event $BUZjxGiOeRi-krvidy23dcTwipdjwlJ-1y50UNbp3Go, at 15:32:16Z. That matches the archive time of 15:32:17Z, and the [channel:] control line is not in the posted body.
• A was never posted. No "witness A: guarded…" reply appears.

What I could not verify:

• The record side: published_event_id/release_event_id, kept_private, and decision_durable.
• That the restarted process ran 37ddc73a4.

Both live only on the host that ran the witness, so on those two points I am relying on the author. One cheap way to let a reviewer close that gap: post the B record's published_event_id value (an event id is not secret) next to the room event above.

Verdict: I consider blocker 1 from my last review met for this head.

Other blockers from my last review

• Susan's request about source-text tests: resolved in substance.
  • Registration is now tested behaviourally at tests/health-check-outbox-parked.test.py:348-355, which was her ask 1.
  • The agreement test (:294-301) is still a source scan. It is now labelled as a structural delegation pin, which lesson 14's exception for a positive delegation pin plus a negative scan allows. qingyun-wu reached the same reading in review 5505006898.
  • It does not do her ask 2, which was to patch guard.namespace_problems and assert the check reports it. Whether that is enough is her call. Her CHANGES_REQUESTED at 8acd5e080 still stands formally until she re-reviews.
• Kewei's Windows ask: the risk is closed by failing safe, and his acceptance is still pending.
  • The path-based fallback he reproduced at 75b11f5d0 (review 5505102838, blocker 1) no longer exists: without directory fds a move refuses (result.py:645-646).
  • His own wording offered "fail closed when confinement cannot be proved" as an alternative, and this matches it.
  • The refusal is pinned by forcing the branch on POSIX, not natively. The PowerShell + Windows runtime job was skipped again at this head.
  • With no move code left to swap into, a native junction test has little left to exercise. Kewei has to confirm that is acceptable, and his CHANGES_REQUESTED at 75b11f5d0 still stands.
  • His blocker 2 (rollback durability) is addressed: see the order checks above and the killed controls below.

Tests (local, at this head, Python 3.14.6, macOS)

• All PASS:
  • withheld-review-decision-final
  • withheld-review-dm
  • team-result-guard
  • gateway-disposal-delegation
  • gateway-terminal-result-moved-once
  • health-check-outbox-parked
  • sparrow-result-guard
  • unreadable-tier-never-substitutes-the-notice
  • result-markers-thread
  • delivery-primitives-single-owner
  • gateway-runtime-identity
• Mutation runner: tests/mutation/5300/run.py passes its baseline and reports 30/30 killed, exit 0. The new rollback-sync and walk-skip controls are each killed by the new regressions.

CI at 37ddc73a4

• All 17 workflow runs on this commit completed success, except Windows runtime, which was skipped. The main CI run is 38417899259.
• gh pr checks shows 36 pass and 3 skipped (PowerShell + Windows runtime, publish, release-assets).
• license/cla passes.
• Our github-evidence.py --expect-head matched the head but again returned "Required-check evidence malformed". It looks like a fault in the tool, so I'm not counting it as either green or red.

What still blocks merge

1. keweichen's CHANGES_REQUESTED at 75b11f5d0. He needs to re-review this head and say whether failing closed without a native Windows test satisfies his Windows ask.
2. Re-reviews from liususan091219 (CR at 8acd5e080) and qingyun-wu (CR at 75b11f5d0). Both CRs named things this head addresses: the live witness and the registration test.
3. Repo gates: two formal approvals at this head, and the Codex-core verification before any approval from me.

Non-blocking

• Failing closed means records on a Windows host never archive or retire. The author says the owner's reply is still answered. A health-check line, or at least a log entry, for "move refused: no directory fds" would make that state visible to an operator, so it does not quietly pile up.

— Sutando (john-the-dev, this session)

[keweichen]

Approve at exact head 37ddc73a4aff1fa672147c93f93c5569de9ea32f. I re-reviewed the delta from 75b11f5d0, the prior cumulative findings, and the posted real-service witness. My two filesystem blockers and the live-path gate are closed. I found no remaining code blocker at this head.

My 75b11f5d0 blockers

• No-directory-fd fallback: fixed fail-closed. src/policy/egress/result.py:645-646 now raises before any link or unlink when O_DIRECTORY is unavailable. The production retry regression removes that capability, runs two beats plus the owner reply, and observes a byte-identical live record, zero os.link calls, no outside file, and an answered reply. The committed counter-mutation restores the former full-path move and is killed. I accept this fail-closed replacement: there is no destructive Windows fallback left for a post-check junction swap to redirect, so the absent native junction run is no longer a privacy blocker.
• Rollback durability: fixed. The pre-unlink rollback is now unlink target → fsync target fd; the post-unlink rollback is link source → fsync source fd → unlink target → fsync target fd. The production helper test records those exact operations against the held directory inodes, preserves the record, and leaves nothing outside. Each omitted-sync mutation is killed. The ancestor-rebind probe and its walk-skip control are also committed.

Live-path gate

The post-restart witness has the required shape at this exact SHA: a detached-head gateway started against the live workspace after the managed service was stopped; two real broker-attested Team tasks traversed the normal guard; the owner used both real review-card decisions; keep-private produced zero shared-room posts and remained durable; release produced exactly one room post, stripped the [channel:] control line, and recorded the same event id as the delivered event. The head then remained up for later retry beats before the normal service was restored and its watcher reported ready. The planned A/B answers were swapped, but each functional branch still ran once, so coverage is unchanged. John independently verified the two triggers and the zero/one room outcomes from room history.

That closes REVIEW lesson 15 and the live-path section of CONTRIBUTING for me.

Other standing findings

• Susan's registration-test blocker is resolved behaviorally: test_run_all_checks_registers_the_row patches check_withheld_reviews_readable, calls run_all_checks(), and asserts both the call and returned row. The remaining source scan is now accurately labelled as the structural delegation/no-private-copy pin; that is REVIEW lesson 14's explicit structural exception, and the concrete namespace behavior is covered separately.
• The canonical guard and vendored Sparrow copy are byte-identical. Publication continues through the shared outbox/provider boundary; the bridge-specific room POST remains injected at the edge.
• Current upstream/main still carries the vulnerable unlocked-snapshot / POST-before-authoritative-CAS path described in #5297, so this remains a real fix rather than a stale patch.

Exact-head verification

• PASS: withheld-review-decision-final, withheld-review-dm, health-check-outbox-parked (28), and gateway-disposal-delegation (76).
• PASS: committed mutation runner baseline and 30/30 killed on Python 3.13.5, including the four new fallback/rollback/walk controls.
• PASS: cumulative git diff --check, review-checks.sh --diff, gen-src-map.py --check, package no-drift, and canonical/vendored comparison. The hardcoded-path scanner rejected its positive control.
• Exact-head hosted gates: 35 check runs succeeded, the three expected jobs were skipped (PowerShell + Windows runtime, publish, release-assets), and license/cla is successful. GitHub reports the head mergeable. Current main has one newer unrelated commit and no changed-file overlap with this PR.

Worst case and remaining process items

On a platform without directory fds, resolved/frozen review records now remain live and are retried/scanned instead of being archived or retired. That can accumulate records and deserves operator visibility or a Windows-safe follow-up, but it fails closed: the unsafe move is not attempted, the durable decision remains authoritative, and no private body is disclosed or lost through this branch. I consider that non-blocking for this privacy fix.

Please update the PR body before merge: it still says the live witness never ran and calls the branch one commit, while the evidence is now in the linked comment and the branch has two commits. This needs no code push and does not block this approval.

Review verdict: APPROVE / code-and-evidence clean at 37ddc73a4. The PR itself is not yet merge-gate-ready: the other standing CHANGES_REQUESTED reviews still need exact-head replacement/dismissal and the repository requires two current maintainer approvals. Do not substitute this approval for those gates.

[qingyun-wu]

Approve at 37ddc73a4aff1fa672147c93f93c5569de9ea32f. No blockers remain from my side. This review replaces this login's CHANGES_REQUESTED at 75b11f5d0. Merge still needs keweichen's and liususan091219's CHANGES_REQUESTED to be cleared or dismissed, plus a second maintainer approval.

What changed since 75b11f5d0

There is one commit, 37ddc73a4, with its parent 75b11f5d0 and base still f85a1d5fb. It touches six files: the guard (result.py plus the vendored copy, ±16), health-check-outbox-parked.test.py, controls.json, run.py and withheld-review-decision-final.test.py (+116). The bridge is unchanged. cmp shows the vendored team_result_guard.py is byte-identical to src/policy/egress/result.py.

My two blockers at 75b11f5d0: both cleared

1. The live witness (comment 6149261676) is at this exact sha and goes through the real service.
   • It reports git rev-parse HEAD as 37ddc73a4aff….
   • The launchd gateway was booted out (old process received SIGTERM), then the head's wrapper was started with the plist environment and took the workspace poller lock. The service was restored afterwards with launchctl bootstrap.
   • The two tasks were real broker tasks with access_tier: team and collaborator: true.
   • The logged lines come from the production drain loop, not a harness:
     • withheld non-owner result for … is remote_gateway_bridge.py:5623;
     • archived … (marker no-send, lease closed, not sent) is :5701;
     • … pending private owner review is team_result_guard.py:882.
   • The owner decided both outcomes on the real cards. Keep-private ended kept_private with decision_durable: true, and the room has 0 posts. Fine-to-post ended published, posted once with the [channel:] line stripped, and the post's event id equals published_event_id (written at remote_gateway_bridge.py:1195).
   • Labels are functional and the bodies are written for the test, so it meets lesson 15's publication rule.
   • Not independently re-observed: the witness ran on the author's host, and this host's gateway log has none of those task ids. I checked it for consistency against the code at this head; I did not replay it.
2. Lesson 14 (liususan091219's ask) is fixed.
   • test_the_check_is_registered is gone. test_run_all_checks_registers_the_row stubs check_withheld_reviews_readable, calls the real run_all_checks(), and asserts the call happened and the sentinel row came back.
   • test_it_agrees_with_the_guard now carries a docstring that labels it a structural delegation pin, which lesson 14's second exception allows. It is backed by the behavioural folder tests in the same file.

The should-fixes, and keweichen's blockers at 75b11f5d0: fixed and pinned

• Both rollbacks are durable. See result.py:653-667.
  • First rollback: unlink target → _fsync_fd(target_fd) → raise.
  • Second rollback: link source → _fsync_fd(source_fd) → unlink target → _fsync_fd(target_fd) → raise.
  • The new test records the exact call order through the production _durable_move and the _fsync_fd seam.
• The ancestor-walk mutant is now killed. At 75b11f5d0 it survived every suite. I re-applied the same mutant (remove if not present(directory, S_IFDIR): return False from _still_at) to both copies, and withheld-review-decision-final now exits 1. I also dropped each of the three new rollback fsyncs one at a time: each is killed (exit 1).
• The no-O_DIRECTORY fallback fails closed (keweichen's B1). See result.py:645-646: it raises before any link. My probe with O_DIRECTORY deleted printed refused: … a confined move needs directory fds; the record stays, source kept: True, links: []. That is the "fail closed when confinement cannot be proved" option keweichen offered.
• My earlier probe, re-run at this head. The six schedules were: the open target swapped for a link, the open target swapped for a real dir, the ancestor swapped for a link, a rebind after the unlink, and a junction attribute on the dir-fd path and on the fallback. Every one raises OSError with source_body='SECRET-BODY' and outside_files=[].

Tests and CI at this head

• Suites. I ran 14 suites under env -i on Python 3.9.6, 3.12.8 and 3.14.6, and all exit 0: withheld-review-decision-final, withheld-review-dm, team-result-guard, gateway-disposal-delegation, gateway-terminal-result-moved-once, health-check-outbox-parked, gateway-runtime-identity, delivery-primitives-single-owner, outbox-operator-recovery, gateway-outage-recovery, gateway-orphan-result-reconciler, gateway-dedup-recovery, sparrow-result-guard and hatch-reply-real-drain. The packages/ag2-sparrow suite gives 256 passed.
• Mutation controls. tests/mutation/5300/run.py gives 30/30 killed, rc=0, re-run here.
• Repo checks.
  • review-checks.sh --diff gives PASS.
  • Check runs: 35 succeeded. Release-assets, publish and "PowerShell + Windows runtime" were skipped.
  • license/cla is success.
  • main moved one commit (8811ceeb5, #5457). It touches none of this PR's files, and git merge-tree reports a clean merge.

Non-blocking

• Silent archive refusal on Windows. With the fallback refusing, a Windows host never archives or freezes resolved review records; they stay live. Decisions stay final, because a claim requires awaiting_owner and the stale-reply tests cover that. But _archive_resolved_review drops the False without a word, so live records pile up with no operator signal. A health-check row or a once-only log line would make this visible. A native Windows regression is also still absent; with moves refused there, the remaining risk is that pile-up, not disclosure.
• Witness gaps the author already disclosed. The owner gave the answers in the reverse order from what was asked, but each was still exercised once. The hitl projection 400s predate the PR.

Worst-case user disruption: on POSIX I found no disclosure or loss path. On Windows the cost is unarchived review records, not leaked bodies. Ready to merge from my side once the other change requests are cleared and a second maintainer approves.

[liususan091219]

Ready to approve.

Re-reviewed head 37ddc73a4aff1fa672147c93f93c5569de9ea32f, following up on my CHANGES_REQUESTED at 8acd5e080.

• Registration: test_run_all_checks_registers_the_row now stubs the check and runs run_all_checks(). I removed the checks.append(check_withheld_reviews_readable()) line, and separately wrapped it in if False:. Both make the test fail ("Called 0 times"); restored, all 28 pass. Fixed.
• test_it_agrees_with_the_guard: it is now labelled as a structural delegation pin. That fits REVIEW.md lesson 14's second exception (a positive delegation pin plus a check that no private scanner exists), and it sits beside the behaviour tests that run the real check through the real namespace_problems on links, FIFOs, files and directories. Accepted.
• Since 8acd5e080: with no directory fds, a move is now refused rather than done by path, and both rollbacks flush to disk in a pinned order. The vendored guard is byte-identical to src/. Locally, the 7 touched suites pass and tests/mutation/5300/run.py kills 30 of 30. review-checks.sh passes on the full diff. CI is green and license/cla passes.
• Live round trip: the exact-head post-restart witness (comment at 15:37Z) covers both owner decisions: the kept-private reply posted 0 times, the released one exactly once with a matching event id.

Nit, not blocking: there is still no native Windows junction regression test, as the author notes.

Not verified by me: the live round trip itself (I read the witness comment), and native Windows.