[sonichi]

Priority

Level: high
Reason: duplicate-delivery risk: a live owner's outbox claim can be reclaimed.

Closes

Fixes #5829

Summary

_read_claim_at coerced pid, start_usec and claimed_at with int()/float(). An impossible birth token (start_usec -1 or 10**400) next to a live pid made _pid_was_reused() report reuse, so may_reclaim_delivery(..., 0) returned True for a claim whose owner is alive. Huge claimed_at and 5001-digit pid values raised instead of reading UNKNOWN.

What changed (src/outbox.py, byte-identical in packages/ag2-sparrow/ag2_sparrow/outbox.py):

• Claim fields are parsed without coercion. Accepted: pid a non-bool int with 0 < pid <= PID_MAX (231-1) via a new valid_pid; start_usec null or a non-bool int with 0 < start_usec < 2**63 (no writer emits 0); claimed_at a finite non-bool int or float within ±253.
• Torn vs malformed. A claim is torn only when its text is empty or fails JSON decoding (what a crashed writer leaves, since the writer emits one complete object). Torn claims keep the existing SWAP_GRACE_S (30 s) sweep. Anything that decodes but has invalid fields, is not an object, or holds an impossible number reads UNKNOWN with malformed=True. Integers are decoded through _bounded_int: one with more than 20 digits becomes a private sentinel that every field validator rejects, so a single oversized value spoils only its own field and the record keeps its owner. Any other ValueError/OverflowError from json.loads still reads malformed, keeping the caller's item_id.
• A malformed claim is never reclaimed sooner than a valid one. It keeps its parseable item_id, drainer_id, strictly valid pid and valid start_usec.
• Evidence is asymmetric. Blocking a takeover is lenient: _candidate_owner_pid reads a pid from every form the pre-fix reader's int() accepted: any non-bool int, any finite float (truncated, as int() does), or any string in int()'s grammar at any length (so "0123", "+123", " 123 ", "1_23" and a pid padded to 5000 characters all count). _int_string_pid reads exactly that grammar in linear time, without calling int() on unbounded input. It strips only the characters int() strips (_INT_WHITESPACE, every c with int(c + "1" + c) == 1, which excludes U+001C..U+001F that str.strip() would remove). It allows one + and requires decimal digit groups joined by single _. It maps every digit of any script to its value with unicodedata.decimal, drops leading zeros, and parses only when at most 10 significant digits remain. More digits are past PID_MAX at any length, so non-blocking, within 0 < pid <= PID_MAX. The blocking set is therefore a superset of what main recognised. For every JSON value type the candidate is exactly valid_pid(int(value)), or None where int() raises: true is pid 1 (a real drainer in a container or PID namespace), false is 0 (non-blocking), and null, lists, objects and the oversize sentinel give None. If that candidate is ALIVE, or its state is UNKNOWN, the claim is held, whatever the TTL. Authorizing is strict: pid reuse is proven only with a strict valid_pid and a valid birth token, and release and identity use the strict pid.
• A malformed claim is reclaimable only when (1) the liveness rules above allow it, and (2) the claim file's own mtime is at least max(TTL, SWAP_GRACE_S) old. claimed_at is never trusted for it. This applies on every path: may_reclaim_delivery, reclaim_delivery_claim, and DesignAClaimBackend.claim()/recover().
• Its owner (matching drainer_id) may release it, so startup recover() retires a terminal item's malformed claim once the rules above allow.
• _pid_was_reused treats an invalid token on either side as "cannot tell", so an impossible token is never evidence of reuse.

Consequence to note: a malformed claim whose pid is readable and alive, with no valid birth token, is held for as long as that process lives, the same rule a valid claim with start_usec: null already follows. Before this PR, start_usec=-1 on a live pid was read as reuse, so the claim was reclaimed.

Trade-off: a corrupt record naming a live pid never ages out. Blocking errs toward never displacing a possibly-live owner. Any pid a malformed claim plausibly names blocks a takeover while that pid reads ALIVE or UNKNOWN, whatever the claim's age. The cost is that such a record stays held. For example, a garbage "pid": true names pid 1, which reads ALIVE or UNKNOWN on most hosts, so it never ages out.

No safe manual remedy exists today. Deleting the claim file is not one. A raw unlink bypasses the item lock and the claim-instance checks. Against a live owner it causes a duplicate delivery (1 → 2 provider calls), and acting on a stale observation it can remove a newer successor's claim. Clearing such a claim safely needs four steps, and no CLI provides them:

1. quiesce every drainer on the outbox;
2. confirm no provider call is in flight;
3. reconcile any indeterminate outcome;
4. release the claim through a locked administrative transition.

This is documented in src/outbox.py's module docstring (docstring-only commit 4ef93d5; the code is identical to 90bf1d1, AST-compared).

Follow-up (not in this PR): two pieces of tooling for this case.

• A health row that surfaces any claim held beyond N×TTL.
• A locked administrative release command that performs steps 1 to 4 above.

Review first: _read_claim_at and _pid_was_reused in src/outbox.py, then contracts 24-25 in tests/sparrow-outbox-claim-protocol.test.py and tests/outbox-malformed-claim-backend.test.py.

Overlap with #5828: #5828 also rewrites _read_claim_at and adds PID_MAX and valid_pid. This PR uses the same names and the same valid_pid signature and semantics (returns the pid or None) so the two reconcile cleanly. Whichever merges second rebases. One difference to settle at that rebase: in #5828 an int pid outside valid_pid stays HELD and process_identity reads it as DEAD, so the claim is reclaimable. Here it reads UNKNOWN (malformed): no owner can be read, so it is reclaimable only once the claim file's mtime is at least max(TTL, SWAP_GRACE_S) old.

Docs: N/A. Internal claim-parsing hardening, no user-visible feature.

Before / after evidence

Production read_delivery_claim / may_reclaim_delivery on a fresh claim with one field rewritten (the pid is the live test process):

Before (origin/main 7940f25):

claimed_at=10**400             read: OverflowError | may_reclaim(0): OverflowError
claimed_at=-10**400            read: OverflowError | may_reclaim(0): OverflowError
pid=5001 digits                read: ValueError | may_reclaim(0): ValueError
start_usec=-1, live pid        read: 'HELD' | may_reclaim(0): True
start_usec=10**400, live pid   read: 'HELD' | may_reclaim(0): True

After (this branch):

claimed_at=10**400             read: 'UNKNOWN' | may_reclaim(0): False
claimed_at=-10**400            read: 'UNKNOWN' | may_reclaim(0): False
pid=5001 digits                read: 'UNKNOWN' | may_reclaim(0): False
start_usec=-1, live pid        read: 'UNKNOWN' | may_reclaim(0): False
start_usec=10**400, live pid   read: 'UNKNOWN' | may_reclaim(0): False

New contract 24, run against origin/main's src/outbox.py (each case reports separately, so every repro is shown failing):

  FAIL an out-of-range pid, birth token or claimed_at reads UNKNOWN and is never reclaimable
         claimed_at=10**400: raised OverflowError; claimed_at=-10**400: raised OverflowError; claimed_at=1e400: state=HELD may_reclaim(0)=False; claimed_at=true: state=HELD may_reclaim(0)=False; pid=5001 digits: raised ValueError; pid=0: state=HELD may_reclaim(0)=False; pid=2**31: state=HELD may_reclaim(0)=True; pid=true: state=HELD may_reclaim(0)=False; pid=1.0: state=HELD may_reclaim(0)=False; start_usec=-1, live pid: state=HELD may_reclaim(0)=True; start_usec=10**400, live pid: state=HELD may_reclaim(0)=True; start_usec=2**63, live pid: state=HELD may_reclaim(0)=True; start_usec=1.5, live pid: state=HELD may_reclaim(0)=True
  target: src/outbox.py  (repo wt)
--
  23 contracts: 22 pass, 1 FAIL, 0 ERROR, 0 not-implemented

Same contract on this branch:

  ok   an out-of-range pid, birth token or claimed_at reads UNKNOWN, not pid reuse

Contract 24 also pins start_usec=0 as invalid (UNKNOWN, may_reclaim(0)=False), and the edge values start_usec=1 and claimed_at=2**53 as HELD.

Malformed claims after review (head 251c826)

Contract 25 (tests/sparrow-outbox-claim-protocol.test.py) ages the claim file's mtime and calls the public reclaim_delivery_claim with TTL 300:

• A live owner whose pid is unreadable (pid a string): refused at 31 s and at 299 s, taken at 300 s.
• A live readable pid with start_usec=-1: refused at 31 s, at 299 s and at 30000 s.
• A dead pid with start_usec=-1: refused at 31 s and at 299 s, taken at 300 s.
• A torn write: refused at 29 s, taken at 31 s (the grace sweep still applies).

tests/outbox-malformed-claim-backend.test.py runs through DesignAClaimBackend:

• The review's scenario (a live owner, start_usec=-1, mtime aged SWAP_GRACE_S + 1, TTL 1e9): claim(B) returns None and A still holds the claim.
• A torn claim under the same conditions is still taken.
• A terminal item's malformed claim (dead pid, start_usec=-1): recover() retires it at TTL age and the item can be republished. At TTL-1 s it is kept.

Controls:

--- previous head edcb838f, sparrow-outbox-claim-protocol
  FAIL an out-of-range pid, birth token or claimed_at reads UNKNOWN, not pid reuse
         start_usec=0, live pid: state=HELD may_reclaim(0)=True
  FAIL a malformed claim ages by file mtime to the full TTL, under the liveness rules; torn still sweeps
         unreadable pid at 31.0s: want taken=False; unreadable pid at 300.0s: want taken=True; live owner, start_usec=-1, taken at 31.0s; dead owner, start_usec=-1 at 31.0s: want taken=False; dead owner, start_usec=-1 at 300.0s: want taken=True
  24 contracts: 22 pass, 2 FAIL, 0 ERROR, 0 not-implemented
--- origin/main 7940f25f, sparrow-outbox-claim-protocol
  FAIL an out-of-range pid, birth token or claimed_at reads UNKNOWN, not pid reuse   (all 14 cases listed)
  ERR  a malformed claim ages by file mtime ...   OverflowError: int too large to convert to float
  24 contracts: 22 pass, 1 FAIL, 1 ERROR, 0 not-implemented
--- this head 251c8261
  24 contracts: 24 pass, 0 FAIL, 0 ERROR, 0 not-implemented

--- previous head edcb838f, outbox-malformed-claim-backend
FAIL: test_live_owner_bad_token_not_taken_after_grace
FAIL: test_terminal_malformed_claim_retired_at_ttl
FAILED (failures=2)
--- origin/main 7940f25f, outbox-malformed-claim-backend
FAIL: test_terminal_malformed_claim_retired_at_ttl     (main ages it by the untrusted claimed_at)
FAILED (failures=1)
--- this head 251c8261
Ran 4 tests ... OK

Oversized integers after the second review (head 4127343)

Three new tests in tests/outbox-malformed-claim-backend.test.py, each using a 5001-digit integer:

• Public path: a live owner's claim with a valid pid and birth token and only claimed_at oversized is not taken by reclaim_delivery_claim at 301 s or at 30000 s (TTL 300).
• DeliveryCore path: drainer A is blocked inside the provider. B runs deliver_one with the claim aged 301 s and then 30000 s. Both return NOT_CLAIMED, and the provider is called exactly once.
• Terminal retirement: a terminal claim with an oversized claimed_at and an oversized extra field keeps item_id, drainer_id, pid and birth token, and recover() retires it once the pid is dead and the mtime reaches the TTL.

Control at the previous head 251c826 (all three fail):

FAIL: test_live_owner_oversize_claimed_at_never_reclaimed
AssertionError: True is not false : a live owner's claim was reclaimed at 301.0s
FAIL: test_live_owner_oversize_claimed_at_one_provider_call
AssertionError: 2 != 1 : provider called 2 times
FAIL: test_terminal_claim_with_oversize_int_keeps_identity_and_retires
AssertionError: Tuples differ: ('UNKNOWN', True, '', '', -1, None) != ('UNKNOWN', True, 'room-evt-malformed', 'A', 3874, 123456789)
Ran 7 tests ... FAILED (failures=3)

At 4127343: Ran 7 tests ... OK.

Pid type after the third review (head 2623173)

Contract 26 (tests/sparrow-outbox-claim-protocol.test.py, public may_reclaim_delivery and reclaim_delivery_claim) and test_pid_forms_block_or_release_by_plausibility (tests/outbox-malformed-claim-backend.test.py, through DeliveryCore and recover()) run one table. Nine pid forms (int, "<pid>", <pid>.0, "0<pid>", "-1", <pid>.5, true, 5001 digits, 2**31) are crossed with a live or dead owner. claimed_at is spoiled so every row is malformed.

• Plausible form and live owner: never reclaimed at 301 s or 30000 s, and exactly 1 provider call while A is blocked inside the provider. recover() does not retire it.
• Dead owner or implausible form: not reclaimed at 299 s, reclaimed at 300 s (2 provider calls through DeliveryCore), and retired by recover() at 300 s but not at 299 s.

Control at the previous head 4127343:

FAIL a plausible live owner pid in any form blocks a takeover; only a strict one authorizes
         digit string alive=True at 301.0s: may/reclaim=(True, True), want False; digit string alive=True at 30000.0s: may/reclaim=(True, True), want False; integral float alive=True at 301.0s: may/reclaim=(True, True), want False; integral float alive=True at 30000.0s: may/reclaim=(True, True), want False
--
  25 contracts: 24 pass, 1 FAIL, 0 ERROR, 0 not-implemented

FAIL: test_pid_forms_block_or_release_by_plausibility (__main__.MalformedClaimBackend.test_pid_forms_block_or_release_by_plausibility) (form='digit string', alive=True)
AssertionError: 2 != 1
FAIL: test_pid_forms_block_or_release_by_plausibility (__main__.MalformedClaimBackend.test_pid_forms_block_or_release_by_plausibility) (form='integral float', alive=True)
AssertionError: 2 != 1
Ran 8 tests in 0.161s
FAILED (failures=2)

At 2623173: 26 contracts: 26 pass; Ran 8 tests ... OK.

Contract 27 covers the remaining new branches: a decode error reads torn; an oversized pid spoils only the pid; other ValueError/OverflowError read malformed and keep the caller's id; a malformed claim whose file has vanished has no mtime and is never reclaimable.

Diff coverage (coverage 7.16 with .coveragerc, over every outbox test that imports src/; diff-cover is not installed here): 56 of 56 changed statements in src/outbox.py covered (100%), up from 51/56 (91.1%) before contract 27. The package copy is omitted by .coveragerc, as for every vendored module.

int()-accepted pid forms after the fourth review (head 4c666d7)

The pid-form table now has 13 forms, each crossed with a live or dead owner.

• Plausible (blocking): int, "<pid>", <pid>.0, "0<pid>", "+<pid>", " <pid> ", "<p1>_<p2>", <pid>.5.
• Implausible: "-1", "abc", true, 5001 digits, 2**31.

Contract 26 also checks the property directly: every form that int() turns into a valid pid is a blocking candidate. Contract 25's implausible row is now "abc".

Control at the previous head 2623173:

FAIL a plausible live owner pid in any form blocks a takeover; only a strict one authorizes
         leading-zero string alive=True at 301.0s: may/reclaim=(True, True), want False; leading-zero string alive=True at 30000.0s: may/reclaim=(True, True), want False; plus-sign string alive=True at 301.0s: may/reclaim=(True, True), want False; plus-sign string alive=True at 30000.0s: may/reclaim=(True, True), want False; padded string alive=True at 301.0s: may/reclaim=(True, True), want False; padded string alive=True at 30000.0s: may/reclaim=(True, True), want False; underscore string alive=True at 301.0s: may/reclaim=(True, True), want False; underscore string alive=True at 30000.0s: may/reclaim=(True, True), want False; fractional float alive=True at 301.0s: may/reclaim=(True, True), want False; fractional float alive=True at 30000.0s: may/reclaim=(True, True), want False
--
  26 contracts: 25 pass, 1 FAIL, 0 ERROR, 0 not-implemented

FAIL: test_pid_forms_block_or_release_by_plausibility (__main__.MalformedClaimBackend.test_pid_forms_block_or_release_by_plausibility) (form='leading-zero string', alive=True)
AssertionError: 2 != 1
FAIL: test_pid_forms_block_or_release_by_plausibility (__main__.MalformedClaimBackend.test_pid_forms_block_or_release_by_plausibility) (form='plus-sign string', alive=True)
AssertionError: 2 != 1
FAIL: test_pid_forms_block_or_release_by_plausibility (__main__.MalformedClaimBackend.test_pid_forms_block_or_release_by_plausibility) (form='padded string', alive=True)
AssertionError: 2 != 1
FAIL: test_pid_forms_block_or_release_by_plausibility (__main__.MalformedClaimBackend.test_pid_forms_block_or_release_by_plausibility) (form='underscore string', alive=True)
AssertionError: 2 != 1
FAIL: test_pid_forms_block_or_release_by_plausibility (__main__.MalformedClaimBackend.test_pid_forms_block_or_release_by_plausibility) (form='fractional float', alive=True)
AssertionError: 2 != 1
Ran 8 tests in 0.131s
FAILED (failures=5)

At 4c666d7: 26 contracts: 26 pass; Ran 8 tests ... OK.

Diff coverage at 4c666d7, measured the same way: 59 of 59 changed statements in src/outbox.py covered (100%).

Padding length after the fifth review (head 046b626)

The length cap is gone. Contract 26's table property is replaced by a boundary sweep. A live pid is zero-padded and whitespace-padded to lengths 1, 10, 32, 33, 64 and 5000 (lengths 1 and 10 use the pid's own width, since a pid cannot be shorter than its digits). Each must read as the pid, and neither may_reclaim_delivery nor reclaim_delivery_claim may take it past the TTL. Grammar edge cases are pinned as well: -5, +-5, 1__2 and _12 are non-blocking; " +0_0012 " reads as 12; 40 zeros followed by 7 reads as 7. test_padded_live_pid_blocks_at_every_length sweeps the same lengths through DeliveryCore (exactly 1 provider call) and recover() (no retirement while the owner lives).

Control at the previous head 4c666d7:

FAIL a plausible live owner pid in any form blocks a takeover; only a strict one authorizes
         zero-padded len=33: candidate None; zero-padded len=33: live owner reclaimed past the TTL; space-padded len=33: candidate None; space-padded len=33: live owner reclaimed past the TTL; zero-padded len=64: candidate None; zero-padded len=64: live owner reclaimed past the TTL; space-padded len=64: candidate None; space-padded len=64: live owner reclaimed past the TTL; zero-padded len=5000: candidate None; zero-padded len=5000: live owner reclaimed past the TTL; space-padded len=5000: candidate None; space-padded len=5000: live owner reclaimed past the TTL; '00000000000000000000000000000000000000007': candidate None, want 7
--
  26 contracts: 25 pass, 1 FAIL, 0 ERROR, 0 not-implemented

FAIL: test_padded_live_pid_blocks_at_every_length (__main__.MalformedClaimBackend.test_padded_live_pid_blocks_at_every_length) (kind='zero', length=33)
AssertionError: 2 != 1
FAIL: test_padded_live_pid_blocks_at_every_length (__main__.MalformedClaimBackend.test_padded_live_pid_blocks_at_every_length) (kind='space', length=33)
AssertionError: 2 != 1
FAIL: test_padded_live_pid_blocks_at_every_length (__main__.MalformedClaimBackend.test_padded_live_pid_blocks_at_every_length) (kind='zero', length=64)
AssertionError: 2 != 1
FAIL: test_padded_live_pid_blocks_at_every_length (__main__.MalformedClaimBackend.test_padded_live_pid_blocks_at_every_length) (kind='space', length=64)
AssertionError: 2 != 1
FAIL: test_padded_live_pid_blocks_at_every_length (__main__.MalformedClaimBackend.test_padded_live_pid_blocks_at_every_length) (kind='zero', length=5000)
AssertionError: 2 != 1
FAIL: test_padded_live_pid_blocks_at_every_length (__main__.MalformedClaimBackend.test_padded_live_pid_blocks_at_every_length) (kind='space', length=5000)
AssertionError: 2 != 1
Ran 9 tests in 0.189s
FAILED (failures=6)

At 046b626: 26 contracts: 26 pass; Ran 9 tests ... OK. Correction: an earlier version of this body said a 300,000-string local fuzz agreed with valid_pid(int(s)) on every input. That claim was wrong. Its generator never produced runs of non-ASCII decimal zeros, so it missed the Unicode-zero false negatives and the U+001C..U+001F false positives fixed at deae1ac (see below). Both were found in review by @keweichen.

Diff coverage at 046b626: 65 of 65 changed statements in src/outbox.py covered (100%).

Exact int() grammar after the sixth review (head deae1ac)

Contract 28: exhaustive single-character oracle. For every code point c from U+0000 to U+10FFFF (surrogates included), it builds five strings around a live pid P:

• c+P+c (padding)
• P[0]+c+P[1:] (separator)
• c+P (leading digit)
• P+c (trailing digit)
• c*10+P (ten-character pad)

It asserts _int_string_pid(s) == valid_pid(int(s)), or None where int() raises. That is 5,570,560 cases. It then rescans all code points and asserts that _INT_WHITESPACE equals {c : int(c+"1"+c) == 1}, which is 25 characters on CPython 3.14.

• Scope: one non-pid character per string, in those five positions, around a single live pid. It does not enumerate multi-character combinations. Those are covered by the seeded fuzz and the fixed rows below.

Measured: at 046b626 the oracle found 75 false negatives (another script's zero, alone or padded ten deep) and 16 false positives (U+001C..U+001F as padding, leading and trailing digit, and ten-character pad). At deae1ac it finds 0 and 0.

Contract 26 additions:

• A live pid padded by ten U+0660, and by mixed-script zeros to lengths 33 and 64.

• A seeded fuzz of 20,000 strings checked against int(). Its alphabet:

  • optional leading space, U+3000 or U+001C;
  • an optional + or -;
  • 0 to 40 zeros drawn from all 76 decimal zeros (every c with unicodedata.decimal(c) == 0);
  • 0 to 12 digits, each from a random script among all 760 decimal digit characters;
  • noise spliced in at 8%: space, tab, U+3000, U+001C, U+001F, +, -, _, a, U+00B2;
  • an optional trailing space or newline.

  A separate local run of the same generator over 1,000,000 strings (seed 1; 376,333 of them name a valid pid) found 0 disagreements. That was measured before the whitespace fix, against a generator that had no U+001C/U+001F noise. Contract 28 is the check that covers those characters.

Backend: test_unicode_zero_padded_live_pid_blocks drives U+0660 and mixed-script zero padding at lengths 33 and 64 through DeliveryCore (exactly 1 provider call) and recover() (no retirement while the owner lives).

Control at the previous head 046b626 (contract 28 stops listing after 10 disagreements):

FAIL a plausible live owner pid in any form blocks a takeover; only a strict one authorizes
         ten U+0660: candidate None, int() reads 34654; mixed-script zeros len=33: candidate None, int() reads 34654; mixed-script zeros len=64: candidate None, int() reads 34654; fuzz '\u3000𖩠०𞗱𐵀߀𞋰𝟶০𞓰᮰０๐꤀꧐𑙐꧰০𑛐߀᱀୦𐴰꤀𞋰𑵐𑽐꩐𑃰𑇐𞅀𑛚𝟎௦𖭐2 ': candidate None, int() gives 2
--
  FAIL the pid string reader agrees with int() for every code point as padding, separator or digit
         U+001C as padding: reader 34654, int() None; U+001C as leading digit: reader 34654, int() None; U+001C as trailing digit: reader 34654, int() None; U+001C as ten as padding: reader 34654, int() None; U+001D as padding: reader 34654, int() None; U+001D as leading digit: reader 34654, int() None; U+001D as trailing digit: reader 34654, int() None; U+001D as ten as padding: reader 34654, int() None; U+001E as padding: reader 34654, int() None; U+001E as leading digit: reader 34654, int() None; U+001E as trailing digit: reader 34654, int() None; U+001E as ten as padding: reader 34654, int
--
  27 contracts: 25 pass, 2 FAIL, 0 ERROR, 0 not-implemented

FAIL: test_unicode_zero_padded_live_pid_blocks (__main__.MalformedClaimBackend.test_unicode_zero_padded_live_pid_blocks) (kind='U+0660', length=33)
AssertionError: 2 != 1
FAIL: test_unicode_zero_padded_live_pid_blocks (__main__.MalformedClaimBackend.test_unicode_zero_padded_live_pid_blocks) (kind='mixed-script', length=33)
AssertionError: 2 != 1
FAIL: test_unicode_zero_padded_live_pid_blocks (__main__.MalformedClaimBackend.test_unicode_zero_padded_live_pid_blocks) (kind='U+0660', length=64)
AssertionError: 2 != 1
FAIL: test_unicode_zero_padded_live_pid_blocks (__main__.MalformedClaimBackend.test_unicode_zero_padded_live_pid_blocks) (kind='mixed-script', length=64)
AssertionError: 2 != 1
Ran 10 tests in 0.556s
FAILED (failures=4)

At deae1ac: 27 contracts: 27 pass; Ran 10 tests ... OK.

Diff coverage: 68 of 68 changed statements in src/outbox.py covered (100%). This was measured just before _INT_WHITESPACE was re-laid out as range() spans (same characters). That re-layout keeps outbox-adapter-contract's transport-leak scan from matching a substring of the literal 0x2000. The constant is a single statement executed at import.

Every JSON value type, the owner axis, and the asymmetry pinned (head 90bf1d1)

Fix: _candidate_owner_pid passes bool through int(). Before this, "pid": true gave no candidate, so a live pid 1 was displaced at the TTL.

Contract 28, domain oracle. For representative values of every JSON type, it asserts _candidate_owner_pid(v) == valid_pid(int(v)), or None where int() raises TypeError, ValueError or OverflowError. The values:

• true, false, null, [], {}, [pid], {"pid": pid}
• ints: 0, 1, own pid, PID_MAX, PID_MAX+1, -1, 10**400, and a dead pid
• floats: 0.0, -0.0, 1.0, 1.5, own pid, pid+0.9, PID_MAX, PID_MAX+1, ±1e308, ±inf, NaN, and the dead pid
• strings: "", "0", "1", own and dead pid, " +pid ", "abc", "-5", U+0660-padded pid, "PID_MAX", "0PID_MAX"
• the oversize sentinel

The owner varies across those values (own pid, pid 1, PID_MAX, a dead pid), not only how the pid is written.

Contract 29, asymmetry pin. A claim from the live owner has its pid rewritten as a plausible but non-strict string ("0<pid>" or " <pid> "). Its birth token is either a mismatched valid one (reuse-looking) or 1 (dead-looking), and it is aged 100× the TTL. It must:

• be held, with may_reclaim and reclaim both refused;
• carry authorizing pid -1;
• refuse release by another drainer.

The control is the same token on a strict int pid, which must still be proven reuse and be reclaimable.

Mutation result. I deleted the strict guard valid_pid(rec.pid) is not None and from the ALIVE branch of _record_is_reclaimable, ran the suites, then restored it. Contract 29 fails, so it pins the asymmetry. The backend test stays green under that mutation; contract 29 is the check that catches it.

FAIL a plausible but non-strict pid blocks a takeover and never proves pid reuse
         leading-zero string, reused-looking token: taken as proven reuse of a live owner; leading-zero string, dead-looking token: taken as proven reuse of a live owner; padded string, reused-looking token: taken as proven reuse of a live owner; padded string, dead-looking token: taken as proven reuse of a live owner
--
  28 contracts: 27 pass, 1 FAIL, 0 ERROR, 0 not-implemented

Ran 11 tests in 0.367s
OK

Backend. The pid-form tables gain true (blocking) and false (not blocking). test_owner_identity_axis crosses five pid forms with five owners:

• own pid (alive)
• pid 1, alive and dead
• PID_MAX, alive and dead
• a dead pid

pid 1 and PID_MAX cannot be owned in a test, so their liveness probe is patched to the row's state, and the test says so. Each case runs through DeliveryCore (1 provider call when alive, 2 when dead) and through recover() at the TTL (no retirement when alive, retired when dead).

Control at the previous head deae1ac:

FAIL a plausible live owner pid in any form blocks a takeover; only a strict one authorizes
         true alive=True at 301.0s: may/reclaim=(True, True), want False; true alive=True at 30000.0s: may/reclaim=(True, True), want False
--
  FAIL the pid reader agrees with int() for every JSON value type, and every code point as padding, separator or digit
         domain True: candidate None, int() oracle 1
--
  28 contracts: 26 pass, 2 FAIL, 0 ERROR, 0 not-implemented

FAIL: test_pid_forms_block_or_release_by_plausibility (__main__.MalformedClaimBackend.test_pid_forms_block_or_release_by_plausibility) (form='true', alive=True)
AssertionError: 2 != 1
Ran 11 tests in 0.562s
FAILED (failures=1)

Contract 29 and the owner-axis test pass at deae1ac as well. They pin behaviour that was already right; the mutation above is their failing control. At 90bf1d1: 28 contracts: 28 pass; Ran 11 tests ... OK.

Diff coverage at 90bf1d1: 68 of 68 changed statements in src/outbox.py covered (100%).

Tests run

Each suite ran alone with nice -n 19, started only when the 1-minute load was under 4, at head 90bf1d1 (157285f and 4ef93d5 change only the module docstring; ag2-sparrow-drift and outbox-adapter-contract were rerun on 4ef93d5 and pass). All exited 0:
sparrow-outbox-claim-protocol, outbox-malformed-claim-backend, ag2-sparrow-drift, proactive-recovery-process-probe, outbox-adapter-contract, outbox-claim-fault-injection, outbox-claim-regressions, outbox-disk-format, outbox-error-paths, outbox-log, outbox-operator-recovery, outbox-race, outbox-receipt-destination, outbox-recovery-paths, outbox-windows-policy, outbox-windows (18 Windows-only skips), proactive-outbox-classification, health-check-outbox-parked, discord-result-outbox-delegation, slack-bridge-reply-outbox, slack-result-outbox-delegation, delivery-core-contract, delivery-core-crash-between-terminal-and-release, delivery-core-enforcement, delivery-core-terminal-not-contended, delivery-primitives-single-owner, proactive-fence-terminal-claim-retired-on-restart, proactive-claim-fence, gateway-terminal-result-moved-once, and the ag2-sparrow package tests test_hatch_adapter and test_wheel_contents.

CI at deae1ac: red only on tests/skills/process-hygiene/reap-orphans.test.py, in test_a_process_that_changes_between_scan_and_signal_is_left_alone (change='cwd') ("the orphan was not a candidate"). This PR does not touch that code. The same assertion failed on main at 47ed231 and 5bcf615, and other main runs pass, so it is an intermittent flake on main.

outbox-claim-machine exited 0 with SKIP: hypothesis not installed on this host, so it did not exercise the change here. CI runs it.

Live path: still owed. This changes delivery-loop recovery behaviour, so the post-restart live witness the reviewer asked for has not been run yet. It needs the owner's go.

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

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

Diff Coverage

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

• src/outbox.py (100%)

Summary

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

[keweichen]

Blocker — an invalid live-owner claim is still reclaimed after 30 seconds, and this bypasses the configured TTL.

Reviewed exact head edcb838fb0972428c6f276d2bb00018c9d1d309b.

_read_claim_at() maps invalid numeric fields to UNKNOWN, the same state used for a torn write. The actual reclaim path then treats every UNKNOWN as an abandoned torn claim and removes it solely once its file mtime passes SWAP_GRACE_S; DesignAClaimBackend.claim() routes UNKNOWN directly into that branch.

I created a real claim owned by the current ALIVE process, rewrote start_usec=-1, aged its mtime by SWAP_GRACE_S + 1, and used a deliberately huge reclaim TTL. HEAD produced:

owner_state ALIVE
read_state UNKNOWN
may_ttl_1e9 False
reclaim_ttl_1e9 True
holder B

A blocked-provider two-drainer control exercises the live DeliveryCore path: at HEAD both A and B reached the provider and returned ATTEMPTED; at merge base 7940f25f, only A reached the provider and B returned NOT_CLAIMED. With the normal 300-second backend TTL, this patch therefore turns the invalid-token case into a takeover after 30 seconds and can duplicate the external side effect sooner than the pre-fix path.

The added contract does not catch this because it stops at may_reclaim_delivery() (lines 798-805); its “never reclaimable” title is not true for reclaim_delivery_claim() or the backend claim path.

Please distinguish a semantically invalid record from a recoverable torn write, or otherwise prevent the torn-write sweep from taking this case. Pin both sides: an invalid live-owner claim remains held before and after grace through the public/backend reclaim path, while a genuinely torn write still ages out. Since this is active delivery-loop recovery behavior, the repository-required post-restart live-path witness is also still owed after the fix.

[keweichen]

Second recovery regression: this UNKNOWN sentinel discards the parseable on-disk item_id, so startup cannot retire a terminal claim left by a crash between terminal write and release.

Exact-head control: publish and claim as A, call record_delivered, rewrite start_usec=-1, then run DesignAClaimBackend.recover(). HEAD returns retired=[]; a B claim is refused, republish is false, and the claim remains forever. The merge base reads the same record as HELD/A, returns retired=["terminal"], removes the claim, and permits republish. The skip occurs because recover() calls _read_claim_at(p, "") and ignores records whose sentinel has an empty item_id.

Please preserve enough routing identity for safe terminal retirement (without making an invalid READY claim reclaimable), and cover the crash-recovery path alongside the READY-item control.

[keweichen]

Boundary blocker: 0 is accepted here and explicitly pinned as HELD by the new contract, but no production identity writer emits a zero birth token: unavailable is None, and Linux, Darwin, and Windows produce positive epoch microseconds. For the current live PID, rewriting only start_usec=0 produced state=HELD, may_reclaim(..., 0)=True, reclaim(..., 0)=True, and holder B. That is the same impossible-token-as-PID-reuse failure this PR is meant to close.

Please make the non-null lower bound strict (0 < start_usec) unless there is a real writer that can emit zero, and replace the positive edge control with a two-way zero-token regression.

[keweichen]

Reviewed exact head 251c82616a1eb2370c1df41be1ac9b2d05436050.

Requesting changes: the prior malformed-claim recovery blocker remains for the issue's 5,001-digit integer case.

P1 — a parse-limit error discards valid sibling fields, so a live claim can still be stolen and a terminal claim can still wedge. In src/outbox.py:485-494 (and the vendored copy), json.loads() raises ValueError when any integer exceeds CPython's digit limit; the handler sets d = None and calls _malformed_claim(..., {}). That loses an otherwise valid PID/birth token and also loses item_id/drainer_id.

Focused local reproduction with a valid current PID/birth token, a raw 5,001-digit claimed_at (same result for start_usec), claim-file age 301s, and TTL 300s:

actual_owner=ALIVE parsed_pid=-1 may_reclaim=True reclaim=True
second drainer=ATTEMPTED provider_calls=2 while first provider call remained blocked

The terminal form also reproduces the earlier recovery wedge when only pid is a raw 5,001-digit integer:

scan item_id='' state=UNKNOWN malformed=True
recover recovered=[] retired=[]
claim_exists=True republish=False

So the new full-TTL behavior delays the duplicate but does not prevent it, and DesignAClaimBackend.recover() still skips the terminal record because the scanner lost its item ID. The new tests cover a huge invalid PID and separately cover terminal malformed fields that still decode to a dict; they do not cover one parse-limit field alongside usable owner/item identity.

Please retain unaffected fields across this parse-error class and add public/backend tests for (1) a huge claimed_at or start_usec with a live valid owner and (2) terminal recovery with a huge numeric field. Preserve the current normal/torn positive controls. After the correctness fix, the required post-restart live-path witness is still needed before approval.

Verification at this head: the focused protocol suite passed 24/24, malformed-backend tests passed 4/4, delivery-core contract suites passed, package parity passed, git diff --check passed, and the repository review checks passed. Current CI has one red standalone-Python lane whose observed failure appears unrelated to this diff; it is not the basis of this review.

[keweichen]

Reviewed exact head 4127343b27bc5dfbcf7263f65e008b0cc71b3a89.

REQUEST_CHANGES — the 5,001-digit sibling-field blocker from review 5630062187 is fixed, but the malformed-PID policy can now steal a live owner's claim.

The new _bounded_int path does preserve a valid PID/birth token and terminal identity when only claimed_at or an extra field is oversized. The focused protocol suite passes 24/24, the backend suite passes 7/7, and the three new oversized-integer cases fail at 251c8261 and pass here.

Blocker: an invalid-but-recognizable PID bypasses liveness and duplicates delivery

At _malformed_claim(), a decimal-string or integral-float PID becomes -1. _record_is_reclaimable() then skips process_identity() entirely and authorizes takeover from file mtime at TTL. The added contract actually codifies this for a string PID at lines 852–856.

I exercised the real DeliveryCore path with drainer A blocked inside the provider, rewrote only its live PID from 3621 to "3621", aged the claim, then ran drainer B:

7940f25f age=300   actual_owner=ALIVE B=not_claimed provider_calls=1
7940f25f age=30000 actual_owner=ALIVE B=not_claimed provider_calls=1
4127343b age=300   actual_owner=ALIVE B=attempted   provider_calls=2
4127343b age=30000 actual_owner=ALIVE B=attempted   provider_calls=2

The integral-float form has the same base→HEAD regression. The normal writer emits integer PIDs, but this PR's concern is precisely malformed persisted claims; a five-minute provider call plus this one-field corruption now permits the duplicate external side effect the outbox exclusion contract forbids. Strict schema validation can remain strict, but an unusable PID must not be treated as proof that no live owner exists. Either retain a tightly bounded recognizable PID solely for the liveness check or fail closed on an unreadable owner, and pin the public/backend two-drainer path at TTL and well beyond it.

Remaining readiness gates

• Required CI genuinely ran and is red: diff coverage >= 95% (python) reports src/outbox.py at 89.6%, missing lines 489-490, 521-522, 577. These are the parse fallback and missing-mtime fail-closed branches introduced by this PR.
• The PR body still explicitly says the required post-restart live delivery round trip is owed. This delivery-loop change is not approvable on harness evidence alone.
• license/cla is currently absent at this SHA, so the complete merge gate is not green.

Other verified evidence: both outbox copies are byte-identical; adapter ownership/delegation checks, git diff --check, and repository review checks pass. A future-but-in-range claimed_at can strand a dead claim, but that behavior is identical on the merge base and is not the basis of this verdict.

[keweichen]

Reviewed exact head 2623173fc0d9942186ef000a15727ee4af7f0d67.

REQUEST_CHANGES — the canonical decimal-string/integral-float blocker from review 5630516794 is fixed, but adjacent PID strings that main safely recognized can still steal a live owner's claim.

The asymmetric shape is otherwise sound: candidate_pid is used only to refuse takeover; UNKNOWN/ALIVE candidates hold; PID-reuse authorization still requires the strict integer PID plus a valid birth token. Independent exact-head QA passed 225 ALIVE/DEAD/UNKNOWN PID/age combinations, eight concurrent DeliveryCore contenders at TTL and 100×TTL, protocol 26/26, backend 8/8, package parity, and the repository scanners. The new tests genuinely fail on 4127343b. Exact-head CI and 56/56 diff coverage are green.

Blocker: the safety-only parser is still narrower than the prior live-owner proof

_candidate_owner_pid() accepts a string only when it is digit-only and has no leading zero. main's prior int(...) read also recognized unambiguous positive strings with leading zeros, a leading +, or surrounding ASCII whitespace. At this head those forms become pid=-1, candidate_pid=None; once file mtime reaches TTL, _record_is_reclaimable() never probes the live owner and authorizes takeover.

I exercised the actual DeliveryCore path with A blocked inside the provider, changed only its live PID field, aged the claim to TTL+1 (301 s for TTL 300), and ran B:

form             7940f25f base                 2623173f head
canonical        B=not_claimed calls=1         B=not_claimed calls=1
leading-zero     B=not_claimed calls=1         B=attempted   calls=2
leading-plus     B=not_claimed calls=1         B=attempted   calls=2
ASCII-whitespace B=not_claimed calls=1         B=attempted   calls=2

All other claim fields remained valid and the real owner stayed ALIVE with the same birth token. The new table currently codifies the leading-zero case as “implausible” and expects the second provider call at tests/outbox-malformed-claim-backend.test.py:34-44 and :199-217. That is a base→HEAD duplicate-delivery regression in the persisted-malformed-record class this PR owns.

Please let the lenient, blocking-only parser retain tightly bounded, unambiguous positive textual PID forms that the prior reader recognized, while keeping valid_pid and the birth token strict for every authorizing decision. Pin the one-field forms through both public reclaim and blocked-provider DeliveryCore paths at TTL and well beyond it.

Remaining readiness gates

• The PR body still explicitly says the required real post-restart delivery round trip is owed. This delivery-loop change remains unapprovable on harness evidence alone under REVIEW.md §15.
• license/cla is absent at this SHA. The CLA recheck workflow only posted the bot command; no required status appeared.

Non-blocking portability question: PID_MAX = 2**31 - 1 says it bounds every supported OS, while the Windows probe passes a DWORD and Win32 exposes process IDs as DWORD. I did not observe a high-bit PID, so this is not part of the blocker above, but the bound should be justified or covered by a Windows edge test.

[keweichen]

Reviewed exact head 4c666d7bf6d45dd0b0631cb12434c7f622cde1a8.

REQUEST_CHANGES — the five newly added forms fix the prior review, but the string-length boundary still lets a live claim be stolen and delivered twice.

The declared bounded contract is otherwise sound. Independent QA passed 324 public/recovery cases, 324 DeliveryCore cases, 20 eight-contender scenarios, protocol 26/26, malformed backend 8/8, and strict reuse/incarnation/completion/release checks. Current CI is green (35 success, 3 intentional skips), package parity is green, and license/cla is present and successful.

P1 blocker: a 33-character int()-accepted PID bypasses the live-owner check

_candidate_owner_pid() calls int() for a string only when len(raw) <= 32. A live PID padded with leading zeros or surrounding whitespace to exactly 33 characters is still accepted by the former main reader and resolves to that live PID, but this head assigns candidate_pid=None. The reclaim rule then skips process liveness and authorizes takeover at the file-mtime TTL.

Independent production-path control with a real live owner, TTL 300, at both 300 s and 30,000 s:

7940f25f zeros33: state=HELD may=False reclaim=False
7940f25f DeliveryCore: B=not_claimed provider_calls=1
7940f25f terminal recovery: retired=[]

4c666d7b zeros33: state=UNKNOWN candidate=None may=True reclaim=True
4c666d7b DeliveryCore: B=attempted provider_calls=2
4c666d7b terminal recovery: retired=[qa-long-pid]

The 33-character whitespace-padded form produces the same result. Thus the cap is a base-to-HEAD regression in exactly the malformed persisted-record class this PR owns: B reaches the provider while A is still blocked inside its provider call.

The purported direct property check at contract 26 only loops over the finite 13-row table, so it never tests the 32/33 boundary. The backend table has the same omission.

Please preserve the old blocking evidence for every safely recognizable valid PID string, or fail closed when a bounded parser declines one; keep the current strict authorization path unchanged. Pin lengths 32 and 33 through public reclaim, concurrent DeliveryCore, and terminal recovery. The exact worst case is duplicate external delivery by two drainers.

The required real post-restart delivery witness is also still explicitly owed under REVIEW.md lesson 15. Fixing this code blocker, then attaching that witness on the final head, is what changes the verdict.

[keweichen]

Reviewed exact head 046b62603260d2d1cc5ef9449a7dacd591de8e2a.

REQUEST_CHANGES — the ASCII 32/33-length blocker is fixed, but the replacement parser drops valid Unicode-zero-padded live PIDs and still permits duplicate delivery.

P1 blocker: non-ASCII decimal zeros are counted as significant digits

_int_string_pid() accepts every Unicode decimal digit through isdecimal(), but line 551 removes only ASCII "0". A string such as ten Arabic-Indic zeros followed by the current live PID is valid Python integer syntax and resolves to that PID, yet its code-point length remains above ten, so this head returns candidate_pid=None.

Exact production-path control (the PID was alive throughout each run):

4c666d7b  Arabic-zero padded: int(text)=<live pid> candidate=<live pid>
          may=False reclaim=False; B=not_claimed provider_calls=1; retired=[]

046b6260  Arabic-zero padded: int(text)=<live pid> candidate=None
          may=True  reclaim=True;  B=attempted     provider_calls=2; retired=['qa-long-pid']

The same failure occurs with fullwidth zero and at both 300 s and 30,000 s claim ages. Base 7940f25f and prior head 4c666d7b both preserve the live claim. An exhaustive Unicode scan found this class for all 67 non-ASCII decimal-zero code points; an independent 39,273-case parser oracle recorded 1,107 false negatives. This does not depend on CPython's long-integer digit limit: the shortest reproducer here is only about 14 characters.

The added length sweep misses the defect because str.zfill() inserts only ASCII zeros. Through the real backend, B therefore enters the provider while A remains blocked in its provider call. The worst case is duplicate external delivery; the same miss also lets terminal recovery retire a claim still owned by a live process.

Please count zero-valued Unicode decimal digits as leading zeros (without passing unbounded significant input to int()), or otherwise fail closed when the blocking-only parser cannot resolve a candidate. Pin at least Arabic-Indic/fullwidth zero padding across the direct parser, public reclaim, blocked-provider DeliveryCore, and terminal recovery paths. Keep the current strict authorization/reuse path unchanged.

Verification at this head: protocol 26/26 and malformed-backend 9/9 pass but miss this case; package parity, git diff --check, and repository review checks pass. CLA is green; several CI lanes are still running. The required REVIEW.md §15 post-restart live delivery witness is also still explicitly owed after the correctness fix.

[keweichen]

Secondary parser finding from the same exact-head sweep (not the basis of the P1 duplicate-delivery verdict): str.strip() at src/outbox.py:545 removes U+001C–U+001F, but Python int() rejects those four characters as surrounding whitespace. For example, _candidate_owner_pid("\x1c" + str(live_pid) + "\x1c") returns the live PID while int(...) raises ValueError.

That direction is refusal-only, so it cannot authorize a duplicate. It can, however, retain a malformed delivery/terminal claim indefinitely whenever those digits name an ALIVE or UNKNOWN process, where the pre-fix reader did not recognize the string. Please make the whitespace grammar match the intended int() contract while touching this helper, or document and test the deliberate conservative superset if indefinite retention is accepted.

[keweichen]

Re-reviewed exact head deae1ac588f71f28117e410c837884f6d101259c. The Unicode-decimal and exact-whitespace fixes are sound: the focused protocol suite is 27/27, the backend suite is 10/10, and independent oracle testing found no parser mismatches. One cumulative safety blocker remains.

P1 — a live PID-1 drainer is now reclaimed when its malformed JSON pid is true.

The PR's compatibility rule is intentionally asymmetric: every legacy int()-coercible valid PID blocks takeover, while only a strict integer can authorize reuse/release. The implementation now exempts true because the PR says PID 1 is “never a drainer.” That premise does not hold for the shipped library:

• write_delivery_claim() writes os.getpid() without excluding 1, and valid_pid() accepts integer 1.
• Sparrow documents the outbox as a library contract and explicitly supports container startup, where the drainer can be PID 1.
• _candidate_owner_pid(True) falls through to strict valid_pid() and becomes None. The reclaim path then skips liveness and authorizes takeover from mtime alone after TTL (lines 598–615).

I ran the unmodified production modules at base and HEAD with controlled OS seams modeling a PID namespace (getpid() == 1, PID 1 ALIVE with birth token 111). Drainer A was held inside the provider; its valid claim's JSON pid was changed from 1 to true and aged past TTL; then drainer B and terminal recovery ran:

base 7940f25f
direct: may=False reclaim=False holder=A
blocked DeliveryCore: B=not_claimed provider_calls=1
recovery: retired=[] claim_exists=True

head deae1ac5
direct: may=True reclaim=True holder=B
blocked DeliveryCore: B=attempted provider_calls=2
recovery: retired=['terminal'] claim_exists=False

That is duplicate external delivery while the original drainer is still live, plus premature retirement of its terminal claim.

Please recognize raw True as candidate PID 1 only on the refusal/liveness path; keep valid_pid, reuse authorization, release, and False strict/invalid. Add public-path, blocked-DeliveryCore, and recovery coverage with a controlled live PID 1. Please also pin the authorization asymmetry with a malformed lenient PID plus a different live birth token (must still refuse) and a strict integer positive control (may prove reuse); deleting the strict guard at src/outbox.py:608 currently leaves both focused suites green.

Other exact-head gates: CI is currently red because python standalone tests (6/9) hit the unrelated-looking reap-orphans cwd race (the same 74-test file passed locally, 74/74); the aggregate job consequently failed and diff coverage skipped. license/cla is absent on this SHA. The required §15 real post-restart delivery witness is also still pending. These gates need to be green/current after the P1 fix.

[keweichen]

Re-review began on requested head 90bf1d1252c80b92560ddb16ea55050dceef58a2; the PR moved during verification, so this verdict covers current head 157285fb3e0b5585daf4908dfbb6e299acbbd4e6 and the intervening delta.

The executable PID fix is now sound. Exact-head protocol is 28/28, malformed-backend is 11/11, DeliveryCore ran 46 tests with 45 passing and one existing skip, and independent JSON-domain/string oracles and owner matrices passed. Restoring the deae1ac5 bool behavior fails contracts 26/28 plus the blocked-provider PID-1 case; deleting the strict reuse guard fails contract 29. Canonical and vendored copies are byte-identical.

P1 blocker — the new recovery instruction defeats the exclusion guarantee.

src/outbox.py:18-22 now tells an operator to delete a malformed live-owner claim file by hand. That raw unlink bypasses the item lock and claim-instance validation. The same module explicitly says even the locked force=True path is “ADMINISTRATIVE DESTRUCTION” that may remove a live peer, and names stale verify-then-unlink as a defect class.

Two exact-head production-path controls:

live-owner documented delete:
  before delete: B=not_claimed, provider_calls=1
  after delete:  B=attempted,     provider_calls=2

stale-observation documented delete:
  observed=A; A released; successor B acquired
  blind unlink removed B; C_acquired=true

In the first control, A remained blocked inside the provider, its malformed record read pid=-1, candidate_pid=1, and following the new instruction allowed B to call the provider concurrently. In the second, the operator's stale path deleted a newer live claim. Worst-case disruption is duplicate external delivery—including duplicate messages or other non-idempotent provider effects—precisely what this claim policy exists to prevent.

Please remove the raw-deletion remedy. The minimum safe documentation is that an ALIVE/UNKNOWN candidate must never be deleted or released and requires operator reconciliation. Any actionable procedure must first quiesce every drainer sharing the root, establish that no provider call remains in flight, and reconcile or park an indeterminate provider outcome before clearing the claim through a locked administrative transition. Quiescing alone is insufficient if the first call may already have crossed the provider boundary. No current CLI exposes that transaction, so omitting the remedy is safer than directing an unlocked unlink.

Non-blocking test hardening: raw true itself is not pinned as strict-invalid under a mismatched live birth token. A narrow mutation retaining True as strict PID 1 leaves both focused suites green (28/28 and 11/11) while changing may_reclaim from False to True. Adding raw true to contract 29 would directly preserve the “candidate-only, never authorization” boundary.

Current gates: exact-head CI is green, including diff coverage. license/cla is absent on this SHA, and the required §15 real post-restart delivery witness remains explicitly owed. Changes requested: fix the unsafe recovery guidance; approval also remains gated on the live witness and normal CLA/approval requirements.

[github-actions]

@cla-assistant check

[keweichen]

Re-reviewed exact head 4ef93d56e4fcc562dd151856414be057ac24dde5.

The prior unsafe-remedy P1 is fixed. The 157285fb..4ef93d56 delta changes only the two outbox module docstrings: both now say no safe manual remedy exists, reject raw unlinking because it can duplicate a live delivery or remove a successor's claim, and name the prerequisites for a future locked administrative transition. The current PR body says the same and labels the health row/admin command as follow-up work, not a current capability.

Fresh exact-head evidence:

• Removing the leading module docstring yields identical ASTs at 90bf1d12 and this head for both copies; canonical and vendored files are byte-identical.
• Claim protocol: 28/28; malformed backend: 11/11; adapter: 10/10; operator recovery: 50/50; DeliveryCore: 45 pass, 1 expected skip.
• Merge-base review checks and the hardcoded-path scan pass.
• The original failure still discriminates: on 7940f25f, a live claim with start_usec=-1 returns may_reclaim(0)=True; this head reads it malformed/UNKNOWN and returns False.

REQUEST_CHANGES remains, now solely for the required live-path witness. This changes delivery-loop recovery behavior, and the PR body still explicitly says the real post-restart delivery round trip is owed. Under REVIEW.md §15, harness proof cannot support approval here. Please run one real delivery through the restarted service on this exact code and attach the end-to-end evidence; that is the condition that changes this review to approval, assuming the head and normal gates remain current.

Current gate context, not additional code findings: python standalone tests (6/9) is red because untouched reap-orphans.test.py failed its own setup (change='exec': “the orphan was not a candidate; the case proves nothing”), so it appears unrelated but required CI still needs a green rerun. license/cla is absent on this SHA.

Non-blocking documentation precision: the module/PR prose says any plausibly named ALIVE PID blocks indefinitely, while _record_is_reclaimable() permits takeover when a strict PID plus valid birth tokens prove PID reuse. Adding “unless strict identity proves PID reuse” would make the trade-off exact. This overstatement is conservative and does not reintroduce the safety issue.

Worst-case disruption remains duplicate external delivery if exclusion fails; the fail-closed implementation and focused controls mitigate the code risk, while the restart witness is still needed to verify the shipped path. Verdict: changes requested; documentation blocker resolved, live-path evidence outstanding.

[liususan091219]

Reviewed at 4ef93d56e4fcc562dd151856414be057ac24dde5. Verdict: comment. The code is right; the approval bar is not met yet.

What I checked myself:

• src/outbox.py and packages/ag2-sparrow/ag2_sparrow/outbox.py are byte-identical at this head.
• I reproduced the bug and the fix with the production functions. A fresh claim held by a live pid, with one field rewritten:
  • main (7940f25): start_usec=-1 reads HELD and may_reclaim_delivery(..., 0) returns True; start_usec=10**400 the same; claimed_at=10**400 raises OverflowError.
  • This head: all of those read UNKNOWN and may_reclaim(0) returns False. A pid of 2**40 reads UNKNOWN / False on both.
• tests/outbox-malformed-claim-backend.test.py: 11/11. tests/sparrow-outbox-claim-protocol.test.py: 28/28 contracts pass. Both drive the real module; no source-grep or in-file copy.
• No hardcoded workspace path in the added lines.
• CI is now green at this head (35 success, 3 skipped), so the earlier red shard has cleared.

What is still missing before this can be approved:

1. No approval yet. keweichen's latest review at this head blocks only on the post-restart delivery round trip. Under this account's review policy, a missing live round trip is named, not blocking, so it does not stop my approval. But the bar here also needs at least one expert approval, and there is none yet.
2. license/cla is absent on this SHA. The commit status endpoint shows zero contexts, which means the check has not run, not that it passed.

Still owed, not blocking from my side: one real delivery through the restarted service on this code.

Non-blocking: #5828 rewrites _read_claim_at too. Settle the out-of-range-int pid difference described in the PR body when the second of the two rebases.

— Sutando (susans-MacBook-Pro)

[sonichi]

REVIEW.md §15 live witness at 4ef93d5

I checked the head with git ls-remote against refs/pull/5830/head at start, before each step and before posting. It stayed at 4ef93d56e4fcc562dd151856414be057ac24dde5 throughout.

Setup. The head was staged by git archive into a scratch directory with a scratch venv. HOME, the task/result/state dirs and the workspace were all in scratch. The bridge ran as python3 -m ag2_sparrow.remote_gateway_bridge from the head's package, with env -i and nice -n 19, in a private tmux server. It used a test agent identity whose usual relay was paused for the run. REMOTE_PROACTIVE_ROOM was empty. The live core and live gateway were not touched.

PR tests (scratch, stdlib only): at the head, sparrow-outbox-claim-protocol.test.py passes 28/28 and outbox-malformed-claim-backend.test.py passes 11/11. The same protocol test run against the merge-base outbox.py (7940f25) fails 3, including:

start_usec=-1, live pid: state=HELD may_reclaim(0)=True

(A) Real item after a restart — PASS

I started the bridge, sent TERM to its verified PID, and started it again from the head. Then the owner posted one message in a test room ("reply PONG"). I wrote its result, and the restarted bridge's outbox claimed, sent and recorded it:

claim (captured in flight, <outbox>/.claims/<item>.claim):
{"claimed_at": 1792796974.869621, "drainer_id": "gateway-result-drain", "item_id": "task-e0d16f8f0779ffa7e0", "pid": <restarted bridge pid>, "start_usec": 1792796930051419}
log:
result task-e0d16f8f0779ffa7e0 delivered via DeliveryCore (provider=AG2SpaceResultProvider, backend=DesignAClaimBackend, worker=gateway-result-drain)
archived result for task-e0d16f8f0779ffa7e0
receipt (<outbox>/.items/<item>.json): "status": "DELIVERED", "provider": "AG2SpaceResultProvider"

The log has exactly one delivered line, and the claim was released afterwards. Checked in the room: exactly one reply.

(B) Malformed claim with a live owner, held across a restart — PASS

The owner posted a second real message. Before writing its result, I planted a claim on that item. The claim has an impossible birth token (start_usec: -1), is 1 hour old by both claimed_at and mtime, and is owned by a live sleep I started (PID verified):

{"claimed_at": <now-3600>, "drainer_id": "witness-live-owner", "item_id": "task-8edd409f6123c61728", "pid": <live sleep pid>, "start_usec": -1}
head:  state=UNKNOWN malformed=True candidate_pid=<sleep pid>  may_reclaim(ttl=300)=False
base (7940f25f, read-only on a copy, no send):  state=HELD start_usec=-1  may_reclaim(ttl=300)=True  (owner ALIVE)

On base, this live owner's claim counts as pid reuse and is stolen. Then I restarted the bridge (TERM by verified PID, start from head) and wrote the result:

result task-8edd409f6123c61728: pending retry or delivery claim (attempts=0, next=None, deadline=None)   x40 passes
delivered/accepted lines for this item: 0
item: status READY, no provider recorded; claim file unchanged

The test room has no copy of that result: the owner read the room, and the task-2 post is the newest message.

Cleanup: I stopped the bridge by verified PID. I deleted the held item's result, outbox item and claim without sending them, then killed the sleep owner. The broker lease for task 2 was left open on purpose, and the owner retires it without sending (a [no-send] close if it is redelivered). Nothing was left running, the test identity's relay was restored, and the scratch dir was removed.

No approve, no merge.

[sonichi]

@cla-assistant check

[keweichen]

Re-reviewed exact head 4ef93d56e4fcc562dd151856414be057ac24dde5. APPROVE — the code and §15 live-path evidence are ready.

The sole condition in my latest review is now met by the later same-SHA live witness:

• After a verified bridge TERM/restart, one real result went claim → send → DELIVERED, produced exactly one DeliveryCore delivered line and one room reply, then released its claim.
• A one-hour-old start_usec=-1 claim owned by a verified-live process survived a second bridge restart and 40 drain passes unchanged, with zero accepted/delivered lines and zero room copies. The merge-base control reads the same live-owner claim as reclaimable.

Independent exact-head QA corroborated both branches without live credentials: the delivery case made one provider call, wrote DELIVERED, released the claim, and three later fresh-process passes stayed terminal; the live-owner case ran 40 fresh-process passes, all NOT_CLAIMED, with zero provider calls and byte/inode/mtime-identical claim state. The same scenario is actually reclaimed at merge-base 7940f25f.

Current exact-head checks:

• claim protocol 28/28 and malformed-backend 11/11;
• broader adapter, DeliveryCore, recovery, fault/error, lifecycle, bridge, and race suites passed (native Windows-only cases were explicitly skipped on macOS);
• canonical/vendored parity, git diff --check, and repository review/hardcoded-path checks pass;
• all current CI lanes are green (35 success, 3 intentional skips), and the branch is not behind main.

The previously noted raw-true/mismatched-birth-token test-hardening gap remains non-blocking: the implementation keeps candidate PID 1 separate from strict authorization, and the current PID-1 backend coverage passes.

This approval replaces my now-resolved CHANGES_REQUESTED; it is not merge authorization yet. license/cla is still absent on this SHA, and this will be only one formal approval where the repo requires two. Merge only after CLA and the second maintainer approval are present and the head/checks remain current.

[sonichi]

@cla-assistant check

[liususan091219]

Approving at 4ef93d56e4fcc562dd151856414be057ac24dde5. The approval bar is now met.

Why it is met now

• Expert approval: keweichen (collaborator; author of 18 merged PRs among the repo's last 100 closed; ten formal reviews on this PR) approved at this exact head, replacing the earlier change requests.
• Verification: the same-SHA live witness (issue comment 6376595932) covers both paths on a restarted bridge: one real result went claim, send, DELIVERED, one room reply, claim released; and a one-hour-old start_usec=-1 claim with a live owner survived a second restart and 40 drain passes with zero sends, while the merge-base reads the same claim as reclaimable. That was the only thing missing in my comment review at this head.
• Code (my own check at this head, unchanged): the two outbox.py copies are byte-identical; tests/outbox-malformed-claim-backend.test.py passes and tests/sparrow-outbox-claim-protocol.test.py reports "the claim protocol holds" on this machine today; both drive the real module, no source-grep or in-file copy; no hardcoded workspace path in the added lines. The two unresolved threads are outdated and were superseded by keweichen's later approval.

Gates that still stand before merge (not review blockers)

1. CI is red on the newest run only, and not because of this diff. Run 40048431645 (01:08Z) failed python standalone tests (1/9) on tests/sparrowd-agent-bridge-rescan.test.py (test_a_removed_worker_stays_owned_until_it_has_exited, a process-exit timing assert). This PR does not touch that file or sparrowd, and the same head passed CI twice before (runs 40013263230 and 40040782784) on the same main. Locally the file passes 5/5 at this head; this Mac's timing differs from the CI runner, so that only shows it is not deterministically broken. A re-run of the job should clear it.
2. license/cla is absent on this SHA: the commit status endpoint returns zero contexts. That is "has not run", not green.

Still owed, not blocking: nothing beyond the witness above.

— Sutando (susans-MacBook-Pro)

[qingyun-wu]

Reviewed at 4ef93d5: src/outbox.py and the vendored packages/ag2-sparrow/ag2_sparrow/outbox.py are identical, no second claim implementation appears, nothing kills. "Impossible" = a JSON record with an invalid field (pid outside 1..2^31-1, start_usec not null/positive int < 2^63, non-finite claimed_at, non-object, >20-digit int) → malformed; empty/undecodable stays torn. The bug on main is real (live pid + start_usec=-1 reads as pid reuse, reclaimable at TTL 0). A 200k-string differential of _candidate_owner_pid against valid_pid(int(s)) found 0 mismatches. Suites: claim-protocol 28/28, malformed-claim-backend 11 OK, adapter-contract 10/10, bench smoke OK, and the new tests fail against main's outbox (22 pass/3 FAIL/1 ERROR; 73 failures).

Approving, with should-fixes:

1. This loosens main knowingly: a claim whose pid cannot be read was held forever; now it is taken after max(TTL, 30 s) by mtime (outbox.py:608-624). The body states it; the risk is a claim file restored with its old mtime ageing out at once.
2. backend_a.py:107 still names the flag recovered_torn though it now covers malformed too, and outbox_cli inspect prints (UNKNOWN) for both, so an operator cannot tell them apart.
3. #5828 edits both outbox copies and reads an out-of-range int pid as DEAD where this reads malformed; whichever merges second must settle that on purpose, not by a mechanical conflict resolution.