Repository navigation
fix(claude-local): #5747 follow-ups: bridge state dir recorded once, a lost turn answered at once, publish I/O retries, lost park reply converges - #5769
Conversation
… dir recorded once, a lost turn answered at once, I/O at publish retries, a lost park reply converges Item 3: install records BRIDGE_STATE_DIR (the dir beside the root for the bridge it wires, or --bridge-state-dir for a bridge you run) and derives the HITL store from it; wire_bridge writes the unit or declaration from that same value and verify's bridge row reads the bridge's status there, so a bridge on another AGENT_CONNECT_STATE_DIR passes the row and projects from the store beside it. A config from before the key reads the HITL store's parent, one naming neither the dir beside the root. Item 12: a Mod that lost its offers inside one generation claims a task afresh; a claim it already drove past its call is answered now instead of at the next generation: a TURN_CONFIRMED claim with a descriptor and a verifying blob is published, one with no verifying blob is answered as interrupted (Lifecycle.interrupt_turn, the move recovery already made, now one method), a RESULT_STAGED or PUBLISHED claim is published. CALLING, a turn confirmed with nothing accepted and another generation's claim are left as before. Item 14: the identical park again after one that settled but whose reply was lost (lock release failed, claim unreadable) answers settled, naming the protocol failure only when that is what it delivered, so the Mod retires the offer instead of retaining it until a restart. Item 15: the harness test named for a turn-end publish failure injects two failures, so the turn-end publish is the one that fails; the one-failure form of the test now fails (66/67). Item 16: publish_and_settle(io_raises=True) turns a publication I/O failure into the StateIOError the claims-step boundary answers as the pre-commit retry, so the reply tool says call again with the same text instead of "publication parked". Item 11 is documented in the README's Limits (the forwarder's death is a stop and a resume, not an adoption). Item 10's DECISIONS entries were already pinned. Refs #5747 Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 93%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
packages/ag2-sparrow/ag2_sparrow/adapters/claude_cloud_session/session_adapter.pyLines 197-207 197 try:
198 rec = self.lc.claims.load(task_id)
199 except task_lifecycle.ClaimUnreadable as e:
200 if task_id in self.lc.settled:
! 201 log("already settled, never called again ->", task_id)
! 202 seen.add(task_id)
! 203 return True
204 return self.park(task_id, e)
205 # A local claim is never this adapter's to repair: it only reads the ledger for one.
206 local = bool(rec and rec.get("claim_mode") == task_lifecycle.LOCAL)
207 # Its repair's failures are classified by contained(), never here: a read parks, a write stops.packages/ag2-sparrow/ag2_sparrow/adapters/claude_code/inbox.pyLines 196-205 196 return {"task_id": task_id, "phase": "absent"}
197 try:
198 # A hard reply is final only over a complete settlement; otherwise the caller's retry completes it.
199 request = dict(request or {}, complete=lc.settlement_complete(task_id))
! 200 except Exception:
! 201 request = dict(request or {}, complete=False)
202 return dict(COMMITTED_FIELDS.get(verb, lambda r, q: {})(rec, request), task_id=task_id, phase=rec["phase"])
203
204
205 def _committed_reply(lc, task_id, verb, request, message):Lines 265-273 265 return
266 if rec["phase"] not in (lifecycle.RESULT_STAGED, lifecycle.PUBLISHED):
267 return
268 if lc.publish_and_settle(task_id) != "settled":
! 269 raise Parked("%s: answered, but its claim did not settle now (%s); retried" % (task_id, lc.claims.load(task_id)["phase"]))
270
271
272 def _mirror(root, task_id, phase, out: dict, extra=None, unstuck=False) -> dict:
273 """The plugin record mirrors each claim's phase after the lifecycle step; a failed write marks theLines 379-387 379 else "its accepted answer could not be finished; answered as interrupted now"
380 elif phase == lifecycle.OUTCOME_UNKNOWN:
381 # A committed intermediate state (the rename landed, its directory fsync failed): resumed, never retired.
382 if lc.resume_outcome(task_id) != "staged":
! 383 raise Parked("%s: its recorded outcome could not be resumed now; retried" % task_id)
384 answered = "its recorded outcome, interrupted, is published now"
385 elif phase in (lifecycle.RESULT_STAGED, lifecycle.PUBLISHED):
386 answered = "its staged answer is published now"
387 else:Lines 519-527 519 # `parked` names what the recorded descriptor delivers: this park's failure line, or an accepted answer.
520 failure = rec["staged"].get("sha256") == hashlib.sha256(body).hexdigest()
521 return dict(publish(root, task_id), parked=detail if failure else False)
522 if rec and rec["phase"] == lifecycle.SETTLED:
! 523 return dict(publish(root, task_id), parked=False)
524 staged = stage(root, task_id, turn_id, body)
525 out = publish(root, task_id)
526 out.update({k: v for k, v in staged.items() if k in ("hard", "error")})
527 return dict(out, parked=detail)packages/ag2-sparrow/tests/test_claude_cloud_session_adapter.pyLines 1114-1122 1114
1115 def unlink(path, *a, **k):
1116 if str(path).endswith("/body"):
1117 raise OSError(5, "Input/output error")
! 1118 return real_unlink(path, *a, **k)
1119 patch = mock.patch.object(lc.os, "unlink", unlink) if kind == "unstage" else contextlib.nullcontext()
1120 if kind == "ledger":
1121 ad.lc.settled.durable.discard("task-a") # visible, not yet synced by this process: add() runs
1122 Path(ad.lc.settled.path).write_text("not a task id\ntask-a\n")Lines 1195-1203 1195 real_sleep = clock.sleep
1196
1197 def bounded_sleep(secs):
1198 if clock.t - T0 > 3600:
! 1199 raise RuntimeError("no result within an hour of loop time")
1200 real_sleep(secs)
1201 clock.sleep = bounded_sleep
1202 try:
1203 run_adapter(work, fake, clock=clock, once=1)Lines 1213-1227 1213 failures, stops = [], []
1214 for k in range(1, n + 1):
1215 try:
1216 _, fired, stopped, calls, results = run(k)
! 1217 except Exception as e: # noqa: BLE001 - the property reports any escape
! 1218 failures.append("read %d (%s): serve() raised %s: %s" % (k, clean[k - 1], type(e).__name__, e))
! 1219 continue
1220 if stopped:
1221 stops.append(clean[k - 1])
1222 if stopped != clean[k - 1].endswith("@outcome") or (fired, calls, results) != ([clean[k - 1]], 1, ["task-a.txt"]):
! 1223 failures.append("read %d (%s): fired %r, stopped %r, %d calls, results %r"
1224 % (k, clean[k - 1], fired, stopped, calls, results))
1225 self.assertEqual(failures, [], "N = %d reads: %r" % (n, clean))
1226 self.assertEqual(stops, [r for r in clean if r.endswith("@outcome")], "only the outcome record's reads stop")
1227 self.assertTrue(stops, "the run reaches an outcome record, so the stop path is exercised")packages/ag2-sparrow/tests/test_claude_code_adapter.pyLines 3686-3694 3686 """An unwritable results dir: publish answers retry (the step did not commit), as the Mod's reply tool
3687 turns into 'call ag2space_reply again'; never the soft 'publication parked'. The turn-end park of a
3688 staged answer carries the same retry."""
3689 if os.geteuid() == 0:
! 3690 self.skipTest("root writes an unwritable dir")
3691 lc = inbox._lifecycle(self.root)
3692 t, p = "task-ro-results", "task-ro-park"
3693 self.confirmed(t)
3694 self.confirmed(p)Lines 3867-3875 3867 def add_faults_once(ledger, task_id):
3868 if not hits:
3869 hits.append(task_id)
3870 raise inbox.lifecycle.LedgerIOError("cannot append to settled-ids ([Errno 5] I/O error)")
! 3871 return real_add(ledger, task_id)
3872 with mock.patch.object(inbox.lifecycle.SettledIds, "add", add_faults_once):
3873 rc, out = self.verb("claim", t)
3874 self.assertEqual((rc, out.get("retry"), hits), (1, True, [t]), out)
3875 ledger = lambda: inbox._lifecycle(self.root).settled.ids # noqa: E731Lines 4062-4075 4062 undurable.clear()
4063
4064 def unstage(store, task_id):
4065 if task_id in undurable and (store.dir(task_id) / "body").exists():
! 4066 problems.append("the body removed before a successful ledger sync")
4067 return real_unstage(store, task_id)
4068
4069 def remove(store, task_id):
4070 if task_id in undurable:
! 4071 problems.append("the claim compacted before a successful ledger sync")
4072 return real_remove(store, task_id)
4073 with mock.patch.object(inbox.lifecycle.os, "fsync", fsync), \
4074 mock.patch.object(inbox.lifecycle.ClaimStore, "unstage", unstage), \
4075 mock.patch.object(inbox.lifecycle.ClaimStore, "remove", remove):Lines 4082-4090 4082 def check(t, reply, where, problems, undurable):
4083 """final => complete (and durable), on every reply; True when the reply is final."""
4084 rc, out = reply
4085 if final(rc, out) and (not complete(t) or t in undurable):
! 4086 problems.append("%s, final %r, over an incomplete settlement" % (where, out))
4087 return final(rc, out)
4088
4089 def complete(t):
4090 try:Lines 4088-4097 4088
4089 def complete(t):
4090 try:
4091 ids = inbox.lifecycle.SettledIds(ledger_path) # still opens: no torn or merged record
! 4092 except inbox.lifecycle.LedgerError:
! 4093 return False
4094 return (t in ids.ids and ledger_path.read_text().count(t + "\n") == 1
4095 and not (lc.claims.dir(t) / "body").exists())
4096
4097 def aged(t):Lines 4128-4151 4128 if check(t, again(), "a retry", problems, undurable):
4129 break
4130 else:
4131 if not (done and s_name == "retry"):
! 4132 problems.append("no final report")
4133 if not check(t, self.verb("claim", t), "a later claim", problems, undurable):
! 4134 problems.append("a later claim is not final")
4135 recovery()
4136 if not complete_safe(ledger_path):
! 4137 problems.append("the ledger does not open")
4138 elif not complete(t) or t in undurable:
! 4139 problems.append("settlement incomplete at quiescence")
4140 got = [p.name for p in self.results.iterdir() if p.name == t + ".txt"]
4141 body = (self.results / (t + ".txt")).read_text() if got else None
4142 if body != expected:
! 4143 problems.append("result %r, expected %r" % (body, expected))
4144 if problems:
! 4145 failures.append("%s x %s x %s: %s" % (e_name, f_name, s_name, "; ".join(dict.fromkeys(problems))))
4146 if not complete_safe(ledger_path):
! 4147 ledger_path.write_text("".join(sorted(x + "\n" for x in real_ids(ledger_path))))
4148 self.assertEqual(failures, [])
4149 self.assertEqual(cases, 168)
4150 self.assertEqual(fired_cases, 156, "every fault fired except the mirror write where the first call writes no mirror "
4151 "(the non-admitted claim's refusal, and the two recovery-first entries): 168 - 12")Lines 4165-4173 4165 def add_faults_once(led, task_id):
4166 if not hits:
4167 hits.append(task_id)
4168 raise inbox.lifecycle.LedgerIOError("cannot append to settled-ids ([Errno 5] I/O error)")
! 4169 return real_add(led, task_id)
4170 with mock.patch.object(inbox.lifecycle.SettledIds, "add", add_faults_once):
4171 out = call()
4172 self.assertEqual(len(hits), 1)
4173 return outLines 4268-4276 4268 # One EIO in the claim save that moves the verified blob on; the blob itself is never written again.
4269 if not hits and Path(path).name == "claim.json":
4270 hits.append(path)
4271 raise OSError(errno.EIO, "I/O error")
! 4272 return real_write(path, data)
4273 with mock.patch.object(inbox.lifecycle, "write_durable", claim_save_eio_once), \
4274 mock.patch.object(inbox.lifecycle.Lifecycle, "_commit_blob", side_effect=OSError(errno.EIO, "x")) as blob:
4275 rc, out = self.verb("claim", t)
4276 self.assertEqual((rc, out.get("retry"), len(hits), blob.call_count), (1, True, 1, 0), out)Lines 4420-4432 4420 def fd_path(fd):
4421 """The path an open fd names (macOS F_GETPATH, Linux /proc), or '' when it cannot be read."""
4422 try:
4423 if sys.platform == "darwin":
! 4424 import fcntl
! 4425 return os.path.realpath(fcntl.fcntl(fd, 50, b"\0" * 1024).split(b"\0", 1)[0].decode())
4426 return os.path.realpath(os.readlink("/proc/self/fd/%d" % fd))
! 4427 except OSError:
! 4428 return ""
4429
4430
4431 def complete_safe(path):
4432 """Whether the ledger still opens (a corrupted one is reset so later property cases stay independent)."""Lines 4432-4445 4432 """Whether the ledger still opens (a corrupted one is reset so later property cases stay independent)."""
4433 try:
4434 inbox.lifecycle.SettledIds(path)
4435 return True
! 4436 except inbox.lifecycle.LedgerError:
! 4437 return False
4438
4439
4440 def real_ids(path):
! 4441 return [line for line in Path(path).read_text(errors="replace").splitlines()
4442 if inbox.lifecycle.local_task_protocol.valid_task_id(line)]
4443
4444
4445 class Detector(unittest.TestCase):src/task_lifecycle.pyLines 35-44 35 import errno
36 import hashlib
37 try:
38 import fcntl
! 39 except ImportError: # no flock: one writer per ledger, as before
! 40 fcntl = None
41 import importlib
42 import json
43 import os
44 import reLines 597-606 597 if read and synced:
598 try:
599 fsync_dir(os.path.dirname(self.path))
600 self.durable = read # never what a repair re-read later: no fsync of ours covered it
! 601 except OSError:
! 602 pass # visible, not yet durable: add() syncs before it counts
603
604 def _sync_file_and_dir(self):
605 _fsync_file(self.path)
606 fsync_dir(os.path.dirname(self.path))Lines 888-898 888
889 def mark_calling(self, task_id, attempt):
890 try:
891 rec = self.claims.load(task_id)
! 892 except StateIOError:
! 893 self.open_attempts.discard(task_id) # CALLING was never written: the attempt may be prepared again
! 894 raise
895 self._save(rec, phase=CALLING, attempt=attempt)
896
897 def _outcome_claim(self, task_id):
898 """The claim an outcome is recorded on, after the provider was called: a read that fails here is part ofLines 898-909 898 """The claim an outcome is recorded on, after the provider was called: a read that fails here is part of
899 that durable write (StateWriteError), so a serving loop stops and recovery answers the call at restart."""
900 try:
901 return self.claims.load(task_id)
! 902 except StateWriteError:
! 903 raise
! 904 except StateIOError as e:
! 905 raise StateWriteError(e.message[len("refusing to serve: "):]) from e
906
907 def authorize_call(self, task_id, attempt, *, text_hash="", expected_phase=PREPARED):
908 """One PREPARED -> CALLING transition per attempt: the one-use submit token (a random nonce
909 stored on the claim with `text_hash`), or None when the claim is not `expected_phase` at thisLines 1099-1108 1099 return False
1100 rec = self.claims.load(task_id)
1101 if rec and rec["phase"] == SETTLED:
1102 return self.reconcile_settled(task_id)
! 1103 self.settled.add(task_id) # a compacted claim: its id synced before it counts
! 1104 return True
1105
1106 def reconcile_unbound(self, now=None):
1107 """The SETTLED claims no generation owns (answered without a call): each settlement completed, and
1108 compacted once due, as generation recovery does for its own. Under claims.lock, by the caller."""Lines 1155-1166 1155 the claim save alone: the blob is final and is never written again. finished | parked (no such
1156 claim, or no verifying blob). A save that fails raises, as every claim save does."""
1157 rec = self.claims.load(task_id)
1158 if rec is None or rec.get("claim_mode") != LOCAL or rec["phase"] != TURN_CONFIRMED:
! 1159 return "parked"
1160 descriptor = rec.get("staged")
1161 if descriptor is None or self._verified(task_id, descriptor) is None:
! 1162 return "parked"
1163 # The blob shares claim.json's directory, so this save's directory fsync makes its link durable too.
1164 self._save(rec, phase=RESULT_STAGED)
1165 self.open_attempts.discard(task_id)
1166 return "finished"Lines 1196-1204 1196 published = self._publish(rec)
1197 except OSError as e:
1198 self.log("publication parked ->", task_id, e)
1199 if io_raises and isinstance(e, RenamedNotDurable):
! 1200 raise PublishedNotDurable(str(e))
1201 if io_raises:
1202 raise _state_io("publish", self.results / ("%s.txt" % task_id), e)
1203 return "parked"
1204 if published is None:Lines 1255-1263 1255 fsync_dir(self.results)
1256 seen = self.read_final(final)
1257 state = LIVE_READY if seen is not None else self.classify(task_id)
1258 except OSError as e:
! 1259 raise RenamedNotDurable("%s landed, then %s" % (final, e))
1260 if seen is not None and seen != body:
1261 self.log("the published result differs from the stage ->", task_id)
1262 return {"won": True, "state": state, "at": self.clock()}Lines 1381-1389 1381 "at": now})
1382 rec = self._save(rec, phase=RESULT_STAGED)
1383 elif phase == TURN_CONFIRMED:
1384 if self.close_turn(task_id, "turn confirmed, no result", now) == "parked":
! 1385 continue
1386 elif phase in (OUTCOME_UNKNOWN, RESULT_STAGED):
1387 if phase != RESULT_STAGED:
1388 self.resume_outcome(task_id)
1389 elif phase != PUBLISHED: |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Reviewed at c2511c5.
What I ran (clean clone, no engine checkout touched):
- packages/ag2-sparrow/tests/test_claude_code_adapter.py: 218 OK
- packages/ag2-sparrow/tests/test_task_lifecycle.py: 254 OK
- tests/ag2-sparrow-task-lifecycle.test.py: OK
- packages/ag2-sparrow/tools/test_no_drift.py: PASS (src/ and package copies byte-identical, also identical on main before this PR)
- tests/ag2-sparrow-claude-local-bridge.test.py, tests/sparrowd-agent-bridges.test.py: OK
- scripts/review-checks.sh --diff: PASS (no hardcoded paths in added lines)
- Not run: the Mod harness (claude plugin test), and no live round trip.
On "both copies": the two task_lifecycle.py files are the pre-existing vendored pair guarded by the drift test, so interrupt_turn is not new duplicated policy. Lifting the recovery block into one method is an improvement. The new tests exercise behaviour through the CLI verbs and the Lifecycle API, not source text. Items 3, 14, 15 and 16 look correct to me.
Blocking (one ask):
- The TURN_CONFIRMED decision is still split, and the two callers disagree on a corrupt committed blob.
- src/task_lifecycle.py:1208-1216 (_recover_generation): blob present and verifying -> RESULT_STAGED; blob present but not verifying -> "a committed result does not match its descriptor; parked", left untouched; no blob -> interrupt_turn.
- inbox.py:310-313 (lost_turn_answered): anything other than finish_staged == "staged" -> interrupt_turn. That includes a blob that is present but does not verify.
Probe: confirm a turn, stage "the accepted answer", roll the phase back to TURN_CONFIRMED (as your task-lost-move case does), then overwrite the committed blob so _verified() returns None, and claim again. The claim answers "its accepted answer could not be finished; answered as interrupted now". The result file gets the interrupted body. The claim settles with detail "turn confirmed, the accepted answer's staging was lost", and staged=None drops the descriptor. The same state reached through the next generation's recovery is parked with the descriptor kept. So the outcome for one claim depends on which path reaches it first, and the recorded detail says "lost" when the bytes were present but wrong.
Either policy is defensible (interrupting is arguably kinder than parking forever). What needs fixing is that there are two. Please put the whole decision -- verifying blob -> stage, mismatched committed blob -> X, no blob -> interrupt -- in one Lifecycle method that both _recover_generation and lost_turn_answered call, and pin the mismatched-blob case for both callers. The PR body's "the move _recover_generation already made, now one method both call" holds for the move but not for the decision that leads to it.
Non-blocking:
-
inbox.py:444 -- park's RESULT_STAGED branch returns parked=False even when the staged body is the protocol failure. Item 16 makes this newly reachable: park stages the failure, the publish raises StateIOError -> retry, and the identical park then hits this branch. register.ts:175 then logs "the accepted answer" for what was the protocol failure. The SETTLED branch at :441 already compares sha256 against the failure body; the same comparison here would make the two consistent. It only affects a log line.
-
cli.py:133 with :541 -- for a config from before BRIDGE_STATE_DIR, verify's hitl row derives
wantfrom HITL_STORE itself (store_root(HITL_STORE.parent.parent)). So any /hitl/requirements-claude-local passes. Before this PR, an external-mode config was checked against hitl_store_root(). The bridge row still catches a store with no bridge beside it, so this is a weaker check rather than a hole. Worth a line in the README's old-config note, or a comparison against the derived default when the key is absent.
Live path: items 12, 14 and 16 change CLI/Mod delivery, so REVIEW.md #15 applies. A post-restart round trip is still owed, as the PR body says.
— Sutando (susans-MacBook-Pro)
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES. I agree with @liususan091219's blocker: the TURN_CONFIRMED decision is split between _recover_generation and lost_turn_answered. I'm adding a second repro that makes it a lost-answer case, not only a corrupt-blob one.
Blocking: a transient I/O error discards a verified accepted answer.
- stage_result's
except OSError: return "parked"also covers _commit_blob's temp write, which runs even when the final blob already exists and verifies. - lost_turn_answered (inbox.py ~309-313) treats any result from finish_staged other than "staged" as "no verifying blob", and calls interrupt_turn (staged=None).
- Repro: stage "the accepted answer", roll the phase back to TURN_CONFIRMED (your task-lost-move setup), then claim with Lifecycle._commit_blob raising EIO once:
- claim → 1 {'refused': 'task-probe: its accepted answer could not be finished; answered as interrupted now; ...'}
- results/task-probe.txt → '[AG2 Space] This attempt was interrupted before the session answered; ...'
- From the same state, lc.recover(generation=..., proof=False) publishes 'the accepted answer\n'. At base, the claim was refused and recovery would have delivered the answer, so this window is a regression.
- Ask: in the single Lifecycle method for the TURN_CONFIRMED decision (as requested in the other review), make an I/O failure while finishing a verifying blob a retry (StateIOError/Parked), never an interrupt. Pin it with a transient-failure case.
Non-blocking:
- publish answers "the step did not commit (...); retried" when fsync_dir(results) fails after move_private, even though results/.txt already holds the answer. The retry converges with no duplicate (settled; one result file), so this is wording only.
Verified at this head:
- test_claude_code_adapter.py: 218 OK.
- test_task_lifecycle.py: 254 OK; tests/ag2-sparrow-task-lifecycle.test.py: 254 OK.
- test_no_drift.py and tests/ag2-sparrow-drift.test.py: PASS. The two task_lifecycle.py copies are byte-identical; they are the existing vendored pair, not new duplicated policy.
- claude-local-bridge: 19 OK; sparrowd-agent-bridges: 10 OK.
- review-checks --diff: PASS. gh pr checks: all pass or skip.
- Mutants: disabling the finish_staged branch (item 12), the SETTLED short-circuit in park (item 14), and io_raises in publish (item 16) each fail their pinning test.
Not verified:
- The Mod harness (claude plugin test; 67/67 per the PR body).
- Any live or post-restart round trip. REVIEW.md #15 applies to items 12, 14 and 16.
- Item 3 on a real external-bridge install.
- systemd/Linux.
Reviewed at c2511c5.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Requesting changes on exact head c2511c5d1609b547befe531c6861a4f2e7f3264b.
Blocker — an interruption can commit OUTCOME_UNKNOWN and then be retired forever
Lifecycle.interrupt_turn() makes the interrupted body durable, saves the claim as OUTCOME_UNKNOWN, and only then saves RESULT_STAGED (packages/ag2-sparrow/ag2_sparrow/task_lifecycle.py:1014-1016). If the first claim.json rename lands but its directory fsync fails, ClaimStore.save() correctly reports StateCommitted (:658-667) and _claims_step() returns a hard committed reply (inbox.py:186-206). The durable record is now OUTCOME_UNKNOWN.
On the next same-generation claim, however, lost_turn_answered() handles only TURN_CONFIRMED, RESULT_STAGED, and PUBLISHED (inbox.py:299-320). It therefore returns the final refusal attempt 1 is OUTCOME_UNKNOWN. The Mod treats any non-retry refusal as final and inserts the task file into seen (hooks/register.ts:331-339). No result exists, and the task is not reconsidered until the generation dies/restarts. That contradicts this PR's “lost turn answered at once” contract.
I reproduced this against the real writer by injecting EIO at the directory fsync after the OUTCOME_UNKNOWN rename:
first: hard=true, phase=OUTCOME_UNKNOWN, "claim.json was replaced, then its directory fsync failed"
second: retry absent, refused="task-outcome-unknown-real: attempt 1 is OUTCOME_UNKNOWN"
result_exists=false
Please make the interruption land in a recoverable/publishable phase in one durable transition, or make the shared reclaim/recovery policy explicitly resume every committed intermediate state, including OUTCOME_UNKNOWN. Pin the post-rename-fsync failure through the live claim/Mod path so a result is eventually published without waiting for holder death.
Other exact-head blockers confirmed
I independently reproduced both existing requested-change findings on this same SHA:
- a transient blob-read/write I/O failure during same-generation reclaim can replace a valid accepted answer with the interruption body, while dead-generation recovery later publishes that accepted answer (review evidence);
- a corrupt committed blob is handled inconsistently by live reclaim versus dead-generation recovery, so the full
TURN_CONFIRMEDdecision is still split across adapters (review evidence).
The worst case is therefore either loss/replacement of an accepted answer or a silently stranded owner task. The fix should centralize the complete decision, preserve accepted descriptors/bytes on storage I/O, and give live reclaim and recovery identical outcomes.
Verification and merge readiness
- Exact-head suites passed: adapter 218, both lifecycle copies 254 each, bridge integration 19, sparrowd bridges 10, adapter boundaries 8.
- Drift checks,
git diff --check, andscripts/review-checks.sh --diffpassed; CI and CLA are green. - Selected new tests fail on parent
7f7f56a8and pass on this head. - A separate independent QA pass reproduced the two existing blockers and found no conflicting result.
- The PR changes a live bridge/delivery path, but it explicitly reports no post-restart round trip. The required REVIEW.md §15 witness is still missing.
This head is not merge-ready until the three lifecycle blockers are fixed and a real post-restart witness is attached.
— Sutando (Neo seat)
…ery and the lost claim; I/O while finishing a verifying blob always retries Review round 1 (Susan, Qingyun): the decision for a TURN_CONFIRMED claim nobody will stage again lived in two places that disagreed. Lifecycle.close_turn now takes it for both _recover_generation and inbox.lost_turn_answered: a descriptor whose blob verifies is finished (finish_staged) and published; a blob missing or not matching its descriptor is answered as interrupted, the detail naming the mismatch, never those bytes; an I/O failure while finishing a verifying blob is parked (the claim as it was, the verified body published on the next call), never an interrupt. A mismatched committed blob in recovery was parked with the descriptor kept; it is now interrupted through the same method (the suite's pin updated). Also from the round: park names the protocol failure when that is what the recorded descriptor delivers, in every branch; verify prints an INFO line for a config from before BRIDGE_STATE_DIR instead of checking the HITL store against a value derived from itself; a publication whose rename landed but whose directory fsync or read-back failed answers retry as "the result landed, its durability unknown" (PublishedNotDurable), never "did not commit". Refs #5747 Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…itted OUTCOME_UNKNOWN is resumed and published, never retired Lifecycle.interrupt_turn() saved the claim at OUTCOME_UNKNOWN and then at RESULT_STAGED. When the first claim.json rename landed and its directory fsync failed, ClaimStore.save() raised StateCommitted, the live claim answered hard, and OUTCOME_UNKNOWN stayed durable; the next same-generation claim's lost_turn_answered() handled only TURN_CONFIRMED, RESULT_STAGED and PUBLISHED, so it gave the final refusal "attempt 1 is OUTCOME_UNKNOWN" and the Mod retired the task with no result until the generation died. - interrupt_turn() now lands in RESULT_STAGED in one save (the body is durable before it), so a rename that lands always leaves a publishable claim. - Lifecycle.resume_outcome() is the one move for a local claim committed at OUTCOME_UNKNOWN (left by an older write, or by recovery's own two-step CALLING path): generation recovery calls it, and the live reclaim (lost_turn_answered) resumes and publishes the durable interrupted body instead of refusing. Both lifecycle copies changed together (drift test). - The 9ff7152 rules are unchanged: a verifying blob is never interrupted; I/O while finishing is a retry. Pins: the live claim path with EIO at the directory fsync after the claim.json rename (first claim hard at RESULT_STAGED, the next claim publishes the interrupted body; at c2511c5 and 9ff7152 the first claim leaves OUTCOME_UNKNOWN and the second refuses "attempt 1 is OUTCOME_UNKNOWN" with no result), a committed OUTCOME_UNKNOWN resumed by the next claim and by recovery, and the Mod harness: a hard lost-turn claim keeps the task, intake reopens, the next claim publishes once, nothing submitted. Refs #5747 Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-reviewed at e059763 (incremental from c2511c5 via 9ff7152).
What I ran (clean clone):
- packages/ag2-sparrow/tests/test_claude_code_adapter.py: 219 OK
- packages/ag2-sparrow/tests/test_task_lifecycle.py: 262 OK; tests/ag2-sparrow-task-lifecycle.test.py: OK
- tools/test_no_drift.py and tests/ag2-sparrow-drift.test.py: PASS (both task_lifecycle.py copies byte-identical)
- test_adapter_boundaries, ag2-sparrow-claude-local-bridge, sparrowd-agent-bridges: OK
- claude plugin test ag2space-inbox: 68 pass, 0 fail; claude plugin validate: passed with warnings
- Probes through the real CLI verbs and Lifecycle.recover, below. No live round trip.
Resolved from my review at c2511c5:
- #1: close_turn is now the one TURN_CONFIRMED decision. I re-ran the mismatched-blob probe through both callers. Live claim -> SETTLED, interrupted, detail "... (the committed result does not match its descriptor)". Generation recovery -> the same. Both are pinned (test_close_turn_is_the_one_decision..., test_generation_recovery_answers_a_mismatched_committed_blob...).
- #2: park compares the staged sha in the TURN_CONFIRMED/RESULT_STAGED/SETTLED branch, so a retried protocol failure is no longer logged as the accepted answer.
- #3: the hitl row for a config without BRIDGE_STATE_DIR is an INFO line, not a self-derived PASS.
Also checked:
- interrupt_turn: claims.stage (write_durable) runs before the single RESULT_STAGED save. A crash between them leaves TURN_CONFIRMED with the body on disk, which is safe to redo.
- resume_outcome: both writers of OUTCOME_UNKNOWN (task_lifecycle.py:1171 and :1243) stage the body first, so moving it to RESULT_STAGED is sound.
- The PublishedNotDurable -> Parked retry: the Mod backs it off and marks it STUCK after STUCK_TICKS (register.ts:233-245), so a persistent failure is surfaced, not a silent loop.
- The live transient-I/O case (Qingyun's) is fixed: claim with one EIO in _commit_blob -> retry; next claim -> "the accepted answer" published.
Blocking:
- Restart recovery now strands a verified accepted answer on one transient I/O error (a regression vs main).
close_turn (src/task_lifecycle.py:1046) finishes a verifying blob through finish_staged -> stage_result, which runs _commit_blob's temp write again. At c2511c5 (and main), _recover_generation moved a verifying claim to RESULT_STAGED with a claim save and no blob write.
Probe: stage "the accepted answer", roll the phase back to TURN_CONFIRMED, then call lc.recover(generation=, proof=False) with _commit_blob raising EIO once.- e059763: commit_blob called once -> phase TURN_CONFIRMED, no result.
- c2511c5: commit_blob never called -> SETTLED, "the accepted answer\n" published.
Recovery runs once per hold start (hold.py:159 -> inbox.recover_dead_generation). In between, a Mod claim on that task is another generation's TURN_CONFIRMED claim and gets the final refusal "attempt 1 is TURN_CONFIRMED" (no retry), so the Mod retires it. The answer is not delivered until the next hold start, and the only trace is the "staging parked" log line.
test_generation_recovery_retries_a_verifying_blob_whose_finish_failed_on_io_never_interrupting calls recover twice back to back, which production never does. The second pass it relies on does not exist within a generation.
Ask: when _verified() passes, close_turn should move the claim to RESULT_STAGED without re-committing the blob, as main did. The blob is already final and verified, so there is no I/O to fail. Alternatively, make a parked close_turn in recovery visible and retried within the generation. Pin it through the real recovery cadence: one recover pass, then the live claim.
Non-blocking:
- Lifecycle.recover's docstring (src/task_lifecycle.py:1133) still says "with a mismatched blob parks". It now interrupts.
Verdict: REQUEST_CHANGES on 1. Items 12, 14 and 16 change the CLI/Mod delivery path, so REVIEW.md #15 still applies and a post-restart round trip is owed before merge.
— Sutando (susans-MacBook-Pro)
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
COMMENT: the code is clear; the §15 witness is still pending.
I re-reviewed at e059763 in a read-only clone and pushed nothing.
Prior blockers. I also ran each repro at c2511c5 and 9ff7152.
- My blocker (5588548239): a single EIO in _commit_blob with a verifying blob. FIXED.
- claim#1 → {'refused': '…the accepted answer could not be closed now; retried', 'retry': True}, result None, phase TURN_CONFIRMED.
- claim#2 publishes 'the accepted answer\n', and the phase is SETTLED. This matches lc.recover. (c2511c5 published the interrupted line.)
- The "did not commit" nit is fixed for the publish verb: it now says "the result landed, its durability unknown (…); retried", and the retry settles.
- @liususan091219's blocker (5588475159): the split TURN_CONFIRMED decision. FIXED.
- close_turn is now the one decision both callers use.
- Corrupt blob: the claim ends SETTLED + interrupted, and recovery ends SETTLED + interrupted with the same detail suffix. (At c2511c5, recovery stayed TURN_CONFIRMED and parked.)
- @keweichen's blocker (5588690901): his repro, an EIO at the directory fsync after the claim.json rename. FIXED.
resume_outcome.
- The only writers of OUTCOME_UNKNOWN are record_outcome (cloud-only) and the CALLING branches of recover and _recover_generation. All three stage the body through write_durable before saving the claim.
- A rename whose directory fsync fails raises StateCommitted before the save, and unstage runs only in _settle. So nothing that wasn't durably staged can reach RESULT_STAGED.
- It returns "parked" for SETTLED, PUBLISHED, RESULT_STAGED and TURN_CONFIRMED.
- Forcing a published claim back to OUTCOME_UNKNOWN and then claiming gives "is LIVE_READY", with one result file holding the original answer. There is no republish.
interrupt_turn: write_durable fsyncs the file, renames it, fsyncs the directory, then makes the one claim save.
Ran at e059763:
- adapter: 219 OK
- lifecycle: 262 OK in the package copy and 262 OK in the src copy; the two copies are byte-identical
- drift checks: PASS
- claude-local-bridge 19 OK; sparrowd bridges 10 OK; boundaries 8 OK
git diff --checkclean; review-checks.sh --diff PASS- Mod harness (claude-code@2.1.303): 68 pass, 0 fail
- gh pr checks: all green, CLA passes
Mutations (package copy), each caught:
- resume_outcome never moving the claim
- a close_turn finish I/O failure that interrupts instead of retrying
- interrupt_turn going back to two saves
Non-blocking:
- A. lost_turn_answered doesn't catch PublishedNotDurable. So when a lost claim's publish rename lands and its fsync then fails, the claim answers "the step did not commit (PublishedNotDurable: … landed …)". The answer is delivered, nothing is published twice, and the next claim refuses with "is LIVE_READY". But the claim stays RESULT_STAGED until the next generation's recovery. The same except clause that publish() already has would fix this.
- B. The recover docstring (task_lifecycle.py:1133, both copies) still says "with a mismatched blob parks"; it now answers as interrupted.
This changes the live CLI/Mod delivery path, so REVIEW.md §15 applies. No post-restart round trip is attached, so this is a COMMENT. My c2511c5 REQUEST_CHANGES stays in place until the §15 witness is posted, and then I'll replace it.
Reviewed at e059763.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
A follow-up to my COMMENT at e059763. There's no contradiction with @liususan091219's new blocker: we tested different paths.
- My EIO repro went through the live claim path, which retries and then publishes. My recovery check ran lc.recover with no injected error.
- Susan's blocker is about generation recovery with the EIO injected. close_turn re-commits a blob that has already verified, and recovery runs only once per hold start. So one transient error parks the claim. The next live claim then gets the final refusal "attempt 1 is TURN_CONFIRMED", and the answer waits for the next hold start.
- I didn't test that path, so my "code-clear" doesn't cover it. I agree with her blocker. The cleanest fix is hers: when _verified() passes, move the claim to RESULT_STAGED without writing the blob again. Pin it with one recover pass followed by the live claim.
Reviewed at e059763.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head e05976369520e06e38cf525be6b959d2a5954eaa against both pinned baselines (c2511c5d, 9ff71523) and the current production call graph.
My prior blocker is fixed. Replaying the post-claim.json-rename directory-fsync failure leaves OUTCOME_UNKNOWN at both old heads; at this head it leaves RESULT_STAGED, and the next same-generation claim publishes and settles. interrupt_turn() now stages the body before one claim save, resume_outcome() is limited to local OUTCOME_UNKNOWN, and both the live reclaim and dead-generation recovery use it. The canonical and vendored lifecycle copies are byte-identical.
Blocking:
-
P2 — a post-publication retry leaves a delivered task persistently mirrored as in-flight and raises a false stall. I faulted
read_final()after the result rename landed whilelost_turn_answered()was publishing. The first claim correctly returned a retry, but the durable state was already:phase=RESULT_STAGED result exists progress.inflight={phase: TURN_CONFIRMED, turn_id: turn-1}The next claim returned
... is LIVE_READY; only ABSENT may be submitted. State was stillRESULT_STAGED, theTURN_CONFIRMEDmirror remained, and the production detector then reportedstatic for 601 s with task-lost-publish-rename in flight.Cause:
packages/ag2-sparrow/ag2_sparrow/adapters/claude_code/inbox.py:253-279takes the terminal-result branch beforelost_turn_answered()and clears onlystuck; it neither reconciles the lifecycle throughpublish_and_settle()nor removes the inflight mirror.register.tsthen marks the final-refused task seen.progress()preserves the mirror, and dead-generation recovery does not clear it, so there is no automatic cleanup path.Please reconcile a terminal result with a
RESULT_STAGED/PUBLISHEDclaim and clear the mirror, or provide an equivalent single-owner transition. Pin the actual sequence throughclaim: result rename lands, read-back/fsync path fails, next claim settles,progress.inflightis gone, and no stall card is emitted. -
P1 — one transient I/O error during the single startup recovery pass can strand an accepted dead-generation answer for the lifetime of the new session. I independently reproduced Susan's current-head blocker. With a verified committed blob and one
_commit_blobEIO:recovered_count 1 after_recovery {'finish_failures': 1, 'phase': 'TURN_CONFIRMED', 'result_exists': False} new_generation_claim (1, {'refused': 'task-dead-recovery-one-pass: attempt 1 is TURN_CONFIRMED'}) after_claim {'phase': 'TURN_CONFIRMED', 'result_exists': False}_recover_generation()treatsclose_turn() == "parked"ascontinue; production runs one recovery pass and then starts the new generation. That generation cannot recover the old claim and final-refuses it, so the Mod retires the task. The added unit test's immediate secondrecover()call is not present in production. Please either advance an already-final-and-verified blob without recommitting it, as main did, or retry/surface the parked recovery before admitting the new generation. Pin one production recovery pass followed by the live claim.
Merge gate: this changes the CLI/Mod delivery and startup recovery paths, but the PR still supplies no real post-restart provider → task → result witness required by REVIEW.md §15.
Evidence at this head: adapter 219/219; lifecycle 262/262 in each copy; claude-local bridge 19/19; sparrowd bridges 10/10; adapter boundaries 8/8; drift checks, git diff --check, and review checks pass. All current GitHub checks and license/cla are green. An independent QA replay confirmed blocker 1. No source changes made.
Verdict: REQUEST_CHANGES until both recovery paths converge in the live cadence and the §15 witness is attached.
…the live claim delivers what one recovery pass left; a found result settles the claim and clears in-flight Recovery (review round 3, the recovery path): close_turn finished a verifying blob through finish_staged, which re-committed the blob, so one transient EIO in the single recovery pass a hold start runs left the claim TURN_CONFIRMED with no result, and the new generation's live claim gave the final refusal "attempt 1 is TURN_CONFIRMED". Lifecycle.finish_verified now moves such a claim to RESULT_STAGED with the claim save alone (the blob is final; that save's directory fsync covers its link, same directory), and close_turn uses it (both lifecycle copies, drift test green). The live claim also delivers what one pass left publishable for another generation: a verifying blob, a staged answer whose publication failed, a committed OUTCOME_UNKNOWN; an interruption stays recovery's. In-flight (review round 3, the publication read-back): a lost claim whose result rename landed and whose read-back failed answered retry; the next claim took the terminal-result branch, cleared only the stuck mark, and left the claim RESULT_STAGED with progress.inflight at TURN_CONFIRMED, so the detector reported a false stall. A claim that finds the result now settles a local RESULT_STAGED or PUBLISHED claim on it (the publisher adds none), and drops the in-flight mirror with the stuck mark in one write. lost_turn_answered maps PublishedNotDurable to the publish verb's retry. The e059763 OUTCOME_UNKNOWN fix and the 9ff7152 rules stand (a verifying blob is never interrupted; I/O while finishing is a retry, now the claim save's). The recover docstring no longer says a mismatched blob parks. Pins: one recover_dead_generation pass, then live claims, no second pass (fails at e059763; the blob-EIO part passes at c2511c5); one recovery pass in the lifecycle with the blob writer failing (both copies); the read_final fault through claim (first claim retry, next claim settles, inflight empty, no stall; fails at e059763 and c2511c5). Refs #5747 Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-reviewed at 7e09b8d (incremental from e059763).
What I ran (clean clone):
- packages/ag2-sparrow/tests/test_claude_code_adapter.py: 221 OK
- packages/ag2-sparrow/tests/test_task_lifecycle.py: 262 OK; tests/ag2-sparrow-task-lifecycle.test.py: 262 OK
- tools/test_no_drift.py and tests/ag2-sparrow-drift.test.py: PASS (both task_lifecycle.py copies byte-identical)
- test_adapter_boundaries 8 OK; ag2-sparrow-claude-local-bridge 19 OK; sparrowd-agent-bridges 10 OK
- claude plugin test ag2space-inbox: 68 pass, 0 fail; validate passed with warnings
- scripts/review-checks.sh --diff: PASS
- Probes through the real claim verb and Lifecycle.recover, below. No live round trip.
My blocker from e059763 is resolved:
- close_turn (src/task_lifecycle.py:1061) now moves a verifying blob on through finish_verified (:1035), which is a claim save only. _verified() re-reads the blob and checks length and sha256 against the identity-bound descriptor, so a stale or mismatched blob cannot pass.
- Probe: stage "the accepted answer", roll back to TURN_CONFIRMED, then ONE lc.recover pass with _commit_blob armed to raise EIO. Result: _commit_blob called 0 times; SETTLED; "the accepted answer\n" published.
- A new-generation live claim on a dead generation's TURN_CONFIRMED claim with a verifying blob (inbox.py:329) -> "staged and never published, is published now". With a mismatched blob it returns "" and leaves the interruption to recovery, which close_turn now always performs, so nothing strands.
- Mismatched blob, both callers: live claim -> SETTLED, interrupted, "... (the committed result does not match its descriptor)"; recovery -> the same. Still pinned for both.
- The recover() docstring (:1148) now matches.
- test_one_recovery_pass_then_the_live_claim_delivers_a_dead_generations_verified_answer_once uses the production cadence (one recover_dead_generation pass, then live claims).
Kewei's read-back case is converged:
- Probe: RESULT_STAGED, read_final raising EIO after the rename. Claim #1 -> retry ("the result landed, its durability unknown"), result present, inflight still set. Claim #2 -> settles via _settle_terminal (inbox.py:233, :284), inflight cleared, stuck None.
- _clear_stuck (:214) drops stuck and inflight in one write_progress. The settle happens first under claims.lock and the mirror clear second; a failed clear is a retry, and the next claim converges (settle is a no-op at SETTLED). Pinned by test_a_lost_claims_publication_whose_read_back_fails_after_the_rename_settles_and_clears_in_flight, which includes the 601 s no-stall check.
- Qingyun's note A: lost_turn_answered now catches PublishedNotDurable (:352).
No remaining code findings from me.
Not yet ready to approve:
- The REVIEW.md #15 post-restart round trip (provider -> task -> result) is still not attached. Items 12, 14 and 16 and startup recovery are on the delivery path.
- At this head the commit status endpoint shows license/cla absent (pending, 0 contexts); it was success at e059763. It needs to re-run.
My earlier REQUEST_CHANGES is addressed in code; it stays on record until an approval replaces it.
— Sutando (susans-MacBook-Pro)
|
@cla-assistant check |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
COMMENT: the code is clear; the §15 witness is still pending.
I re-reviewed 7e09b8d in a read-only clone, ran every repro at e059763 as well, and pushed nothing.
@liususan091219's blocker (one EIO in the single recovery pass strands the answer): FIXED.
- Her exact repro: one
lc.recover(generation=G, proof=False)with_commit_blobraising EIO once, then a new generation's claim. - Through hold's
recover_dead_generation: the same at both heads. - A corrupt blob is still interrupted on both paths and is never published.
- My earlier EIO-on-claim repro now publishes on the first claim.
@keweichen's blocker (a delivered task left in flight): FIXED.
- 7e09b8d: claim#2 gets LIVE_READY; SETTLED with
won=True; the in-flight mirror is gone;stall at 601 s: None. - e059763: RESULT_STAGED, the mirror is kept, and the claim is reported static at 601 s.
- With a different body already at
results/<t>.txt, the claim settles withwon=Falseand leaves that result alone. That is the publisher's existing never-replace rule.
My earlier items:
- A: fixed, but not pinned. "the result landed, its durability unknown … retried", and the next claim settles. Removing the new
except PublishedNotDurablesurvives both suites; asserting the message would catch it. - B: fixed in both copies.
New, non-blocking:
- Recovery never clears a dead generation's in-flight mirror. With no later claim on that task, the detector reports
static for 601 s. This reproduces the same at the merge-base, so it predates the PR. Worth a follow-up issue. - A RESULT_STAGED claim whose blob is corrupt, on a task that already has a result, now gets "did not settle now … retried" on every claim, because
_publishchecks the blob before it looks at the existing result. The stuck mark is filtered out as resolved, so nothing surfaces it. It was a final refusal at e059763. It's rare, and settling on the existing result first would close it. finish_verifiedrefusing a blob that doesn't verify is pinned only through the adapter. A lifecycle-level pin would be cheap.
Ran at 7e09b8d:
- adapter 221 OK; lifecycle 262 OK; the other lifecycle, bridge and boundary suites OK
- the two copies are byte-identical; drift checks PASS
- Mod harness (claude-code@2.1.303): 68 pass, 0 fail
- review-checks.sh --diff PASS
- every new pin fails at e059763
- six of seven mutations caught (the exception is A's except, above)
- gh pr checks: 35 pass, 3 skipping
license/cla is absent at this head. The @cla-assistant check comments didn't restore it; close and reopen the PR to retry.
This is a live delivery and startup-recovery path, so REVIEW.md §15 applies. My e059763 REQUEST_CHANGES stays in place until the §15 witness is posted and license/cla is green.
Reviewed at 7e09b8d.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Gate restated, no new findings. My latest COMMENT at this head says the code is clear. A COMMENT review replaces my earlier REQUEST_CHANGES in latestReviews, so I am re-stating the gate explicitly. The only remaining items are license/cla green and the REVIEW.md §15 post-restart witness (CLI/Mod delivery + startup recovery). I will replace this with an APPROVE once those land.
At 7e09b8d
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 7e09b8d647e96d863397a40f063dab594dce8898 (incremental from e0597636). REQUEST_CHANGES: both reported recovery defects are fixed, but the newly activated terminal-settlement path has one durability hole, and the required live witness is not attached.
The two round-3 code blockers now discriminate correctly:
- One production recovery pass with publication failing leaves the verified answer
RESULT_STAGED; the next live claim publishes the exact accepted body and reachesSETTLED. The same probe ate0597636remainsTURN_CONFIRMEDwith no result and the live claim refuses it. - After the result rename lands and
read_finalraises, the next claim settles on the existing result, preserves its inode/bytes, and removesinflightplusstuckin one productionwrite_progressmutation. The detector reports no stall at 601 s. Ate0597636, the claim remainsRESULT_STAGED,inflightremains, and the detector reports the false 601 s stall.
Blocking durability case:
-
Lifecycle._settle()savesphase=SETTLEDbeforeSettledIds.add()(src/task_lifecycle.py:993-996, vendored copy identical). If that append fails during_settle_terminal(), the claim correctly returns a retry, but the durable state is alreadySETTLEDwith no settled-ID record. The next claim skips_settle_terminal()because it accepts onlyRESULT_STAGED/PUBLISHED(inbox.py:233-240), clears the progress mirror, and final-refuses.recover_dead_generation()also excludesSETTLEDclaims (inbox.py:95-97), so a later holder recovery does not repair it.Exact fault probe:
claim #1: retry; phase=SETTLED; settled-id absent claim #2: LIVE_READY final refusal; phase=SETTLED; settled-id still absent dead-generation recovery: 0 generations; settled-id still absentThe result is preserved and I observed no duplicate delivery, but settlement remains permanently incomplete and the claim cannot compact. Please make a retry of an already-
SETTLEDlocal claim reconcile the settled-ID ledger before progress cleanup/final refusal, in the shared lifecycle owner, and pin this exactSettledIds.addfailure sequence through the liveclaimpath.While extending that pin, seed both
stuckandinflight(plus an unrelated neighbor) and assert the single production mutation. The new test's docstring promises both-map cleanup, but its setup/assertions currently cover onlyinflight; an independent production-writer probe confirms the implementation does clear both.
Validation at this SHA: adapter 221/221; both lifecycle copies 262/262; Mod hook suite 68/68; claude-local bridge 19/19; sparrowd bridges 10/10; adapter boundaries 8/8; both drift guards, git diff --check, and review-checks.sh pass. The hardcoded-path scanner positive control fails as expected. Independent QA reproduced both fixes against the parent and the ledger failure above.
license/cla is now green. A replacement CI run was still finishing when I submitted this review (all completed jobs green; two jobs pending).
The REVIEW.md §15 gate also remains open: this changes startup and CLI/Mod delivery, but no real post-restart provider → task → result transcript/output is attached. The PR body still says no such run was performed from the clone. A witness said to exist in an owner's combined session is not reviewable evidence until its actual command/output is posted.
Worst-case disruption from the remaining code defect is durable state debt after one transient ledger-write failure; the shipped task result survives, but the local settlement invariant never converges. Merge readiness: changes requested until the ledger retry is repaired and pinned, the §15 witness is attached, and the current required checks finish green.
…leted before any cleanup, and recovery compacts it Lifecycle._settle() saves SETTLED, then appends the id to settled-ids and removes the staged body. An append that faulted left the claim SETTLED with no ledger entry: that claim answered retry, the next claim's terminal branch handled only RESULT_STAGED/PUBLISHED, cleared the progress mirror and final-refused, and recover_dead_generation skipped every SETTLED claim, so the settlement never completed and the claim never compacted (the result survived; nothing was published twice). - Lifecycle.reconcile_settled() completes a SETTLED claim's settlement: the id appended if missing (SettledIds.add is idempotent), the staged body removed. _settle ends with it, and generation recovery's re-record pass uses it (both lifecycle copies, drift test green). - The claim verb's terminal branch reconciles a SETTLED local claim under claims.lock, before the progress cleanup and the final refusal; a ledger I/O failure stays the pre-commit retry. - recover_dead_generation also brings in the generation of a SETTLED claim still owed its ledger entry or due for compaction, so a settled claim compacts once its time comes. Pins: the settled-ids append faults once, then a second claim: ledger entry present, staged body gone, then one recovery pass past COMPACT_AFTER compacts the claim (fails at 7e09b8d: no ledger entry after the second claim); reconcile_settled in the lifecycle (both copies). The in-flight cleanup test now seeds and asserts both maps (inflight and stuck) with an untouched neighbor, in one production write. Refs #5747 Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-reviewed at 20928ff (incremental from 7e09b8d).
What I ran (clean clone):
- packages/ag2-sparrow/tests/test_claude_code_adapter.py: 222 OK
- packages/ag2-sparrow/tests/test_task_lifecycle.py: 264 OK; tests/ag2-sparrow-task-lifecycle.test.py: 264 OK
- tools/test_no_drift.py and tests/ag2-sparrow-drift.test.py: PASS (both task_lifecycle.py copies byte-identical)
- test_adapter_boundaries 8 OK; ag2-sparrow-claude-local-bridge 19 OK; sparrowd-agent-bridges 10 OK
- claude plugin test ag2space-inbox: 68 pass, 0 fail; validate passed with warnings
- scripts/review-checks.sh --diff: PASS
- Probes through the real verbs, Lifecycle and recover_dead_generation, below. No live round trip.
Kewei's ledger case (src/task_lifecycle.py:993-1008, inbox.py:236, :97):
- Append faults once (SettledIds.add raises on its first call): publish -> retry, claim SETTLED, no ledger entry. The next claim -> reconcile_settled under claims.lock, then the mirror clear, then LIVE_READY. End state: id in the ledger exactly once, staged body gone, one result write (move_private called once), inflight cleared.
- Crash between the append and the body removal (unstage raises): SETTLED, id in the ledger, result intact. reconcile_settled called twice afterwards -> True/True, one ledger line, body gone. Idempotent.
- Scope of the removal: reconcile_settled on a RESULT_STAGED claim -> False, legacy body kept, nothing appended. On a missing claim -> False. Settling one task removes only its own body; a neighbour RESULT_STAGED claim keeps its body and phase.
- Recovery pickup: a dead generation's SETTLED claim missing its ledger entry -> recovered, entry written, 0 result writes, same inode and bytes. With settled_at past COMPACT_AFTER and the entry owed, the same pass writes the entry, then compacts. decide() then answers settle and a later claim refuses LIVE_READY. Nothing re-opened or re-published.
- Lock order: the claim verb and recover_dead_generation both take claims.lock first, and the progress write in claim follows its release. 20 claims raced against 20 recovery passes in two threads completed in 0.23 s with no errors.
- The new pins seed both stuck and inflight plus an untouched neighbour, and assert one write_progress call, as Kewei asked.
No regressions: my earlier probes rerun at this head (one recovery pass with the blob writer armed -> 0 writes and published; new-generation claim on a verifying blob -> published; mismatched blob through the live claim and recovery -> interrupted; read-back failure after the rename -> settles and clears in-flight).
No code findings from me at this head.
Still open before approval:
- The REVIEW.md #15 post-restart round trip (provider -> task -> result, the actual output) is not attached. This is the startup-recovery and CLI/Mod delivery path.
- license/cla is back: success at this head.
— Sutando (susans-MacBook-Pro)
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 20928ff5aafb7f6277325ab9f8a7c01aeb84d0ee. REQUEST_CHANGES: the prior live-claim ledger blocker is fixed, but the same settlement debt still escapes through adjacent production paths, the claimed idempotent append is not idempotent across waiting writers, and the required live witness is not attached.
The requested path now discriminates correctly: after one SettledIds.add fault, claim #1 retries with SETTLED/no ledger entry; claim #2 runs reconcile_settled() under claims.lock, records the ID, removes a real seeded legacy body, clears both progress maps in one write while preserving a neighbor, and later dead-generation recovery compacts without changing the one result's bytes/inode. The parent 7e09b8d6 leaves the ID/body unrepaired and recovers zero generations.
Blocking — already-SETTLED success/final paths do not all reconcile
publish_and_settle() returns "settled" for an already-SETTLED record without calling reconcile_settled() (src/task_lifecycle.py:1079-1103, vendored copy identical). inbox.publish() uses that result directly, and park() routes its SETTLED retries through publish() (packages/ag2-sparrow/ag2_sparrow/adapters/claude_code/inbox.py:463-493). With a one-shot ledger-append fault:
publish #1: retry; phase=SETTLED; ledger=false; body=true
publish #2: rc=0, outcome=settled; ledger=false; body=true
The second call clears the mirror and tells the Mod settlement succeeded without completing either remaining step. A later proven-dead generation repairs this case, but a live generation can retain the debt indefinitely.
The permanent variant is complete_without_call(), whose claims have no claim_mode or generation (src/task_lifecycle.py:981-985). _settle_terminal() rejects them before its new SETTLED branch, while dead-generation recovery selects only generated local claims (inbox.py:84-105,236-250). The real non-owner claim path gives:
claim #1: retry after LedgerIOError; phase=SETTLED; ledger=false; body=true
claim #2: final LIVE_READY refusal; ledger=false; body=true
dead-generation recovery: 0; ledger=false; body=true
interrupt() is worse: after the same append fault, its catch plus resolved() turns the first call into rc=0, resolved=true, answered=false, state=SETTLED; repeated interrupt/claim and recovery still leave the ledger absent and body present (inbox.py:314-324,381-411). These are the same settlement invariant and retry decision this round changes, not unrelated cleanup. Please route every already-SETTLED path that reports success/finality through the shared reconciliation under the claims lock, and pin direct publish/park, non-admitted claim, and interrupt with the append failure.
Blocking — the append is only idempotent in one cached Lifecycle
Lifecycle loads SettledIds.ids before acquiring the adapter's claims lock (src/task_lifecycle.py:729-742; inbox.py:274-277,463-465). SettledIds.add() checks only that cached set before appending (src/task_lifecycle.py:604-627). I created two lifecycle instances while the ledger was empty, then serialized both reconcile_settled() calls through the production records.flocked(claims_lock_path(...)):
prelock_cached=false false
ledger_bytes='task-race\ntask-race\n'
record_count=2
So overlapping CLI retries can append the same ID twice despite the lock and the new “idempotent append” contract. Please make the production writer's idempotence cross-instance and pin two stale readers/waiting writers; repeated races otherwise permit unbounded duplicate ledger growth.
The new body-removal assertions also need a real body: local descriptor/blob staging never creates claims/<id>/body, so replacing ClaimStore.unstage() with a no-op leaves all three new pins green. Seed the legacy body as the failure-mode probe does. The implementation does remove it when reconciliation is actually reached.
Validation at this SHA: adapter 222/222; lifecycle 264/264 (source/vendored bindings); Mod 68/68; bridge 19/19; sparrowd 10/10; boundaries 8/8; both drift guards, git diff --check, and review-checks.sh pass, including a firing hardcoded-path positive control. Current CI and CLA are green.
REVIEW.md §15 remains independently blocking. This changes startup/recovery and CLI/Mod delivery, but the PR still says no post-restart round trip was run and no real provider → task → result command/output is attached. “Will be posted” is not reviewable evidence yet.
Worst-case disruption is a task reported settled/final while its durable settlement remains incomplete: plaintext staging can persist, generation-less claims never compact, and concurrent retries can grow duplicate ledger records. Merge readiness: changes requested until these paths converge through the one writer contract, the failure modes are pinned non-vacuously, and the §15 witness is attached.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES: the code is clear; this holds only the REVIEW.md §15 gate.
I reviewed the delta 7e09b8d → 20928ff in a read-only clone and pushed nothing.
@keweichen's blocker (a SETTLED claim whose ledger append failed never completes): FIXED.
- At 20928ff: claim #1 retries. Claim #2 is final (LIVE_READY), the ledger holds the id once, and the in-flight mirror is gone. The result is delivered exactly once.
- At 7e09b8d: the ledger stays empty, and recovery picks up 0 generations.
- Run against 7e09b8d, the new adapter pin fails and the lifecycle pin errors in both copies.
reconcile_settled is idempotent and only touches SETTLED claims:
- Called twice: the ledger holds the id once.
- An EIO in unstage after the append: the claim retries, then completes once.
- RESULT_STAGED and TURN_CONFIRMED claims are untouched.
- Recovery reconciles a dead generation's claim that is owed its ledger entry, with zero publish calls and the result's inode unchanged. Live-generation claims are untouched.
- It runs under claims.lock in both callers.
Earlier repros, all unchanged: Susan's one-pass EIO, the corrupt blob, the EIO-on-claim, and your read_final case.
Non-blocking:
- My earlier A, 1, 2 and 3 are unchanged.
- The "staged body gone" assertions are vacuous. The pins stage through a blob, so
claims/<t>/bodynever exists, and removingclaims.unstagefrom reconcile_settled still passes. Pinning through an interrupted body would make them real. - The interrupt and non-owner-refusal entry point is in the same ledger class but isn't covered.
interruptwith the append faulting answers resolved with no ledger entry, and nothing repairs it later, becausecomplete_without_callclaims carry no claim_mode or generation. This is pre-existing. Calling reconcile_settled from publish_and_settle's already-SETTLED branch would cover every verb. - One EIO while recovery unstages a leftover body now raises "refusing to serve", so the start is deferred to the next pass. That's consistent with the existing state-I/O policy; a best-effort unlink would avoid it.
Ran at 20928ff:
- adapter: 222 OK; lifecycle: 264 OK in both copies, which are byte-identical; drift: PASS
- Mod harness (claude-code@2.1.303): 68 pass, 0 fail
- review-checks.sh: PASS;
git diff --checkclean - mutations: 5 of 6 caught
- CI green, including license/cla
This is a live delivery and startup-recovery path. My REQUEST_CHANGES stays in place until the §15 post-restart witness is posted.
Reviewed at 20928ff.
…le_settled; the settled-ids append is read-check-append under the ledger's lock The settlement could still stay incomplete after one settled-ids append fault: publish_and_settle answered "settled" for a claim it found SETTLED without completing it (publish, and park through publish); a non-admitted claim and interrupt carry no generation, so the terminal branch and recovery skipped them, and interrupt reported resolved on the SETTLED phase alone. Separately, SettledIds.add checked only the instance's cached ids, so two calls that loaded the ledger before either appended could record an id twice. - Lifecycle (both copies, drift test green): publish_and_settle reconciles a SETTLED claim before it returns settled; settle_without_result ends with reconcile_settled; settlement_complete says whether a settlement is whole (ledger entry, no staged body); reconcile_unbound reconciles and, once due, compacts the SETTLED claims no generation owns. - SettledIds.add takes the ledger's own flock, re-reads it, and appends only an id not there yet. - inbox: resolved() counts a SETTLED claim final only when settlement_complete; the claim verb settles a found result for local and call-less claims before it decides finality; interrupt reconciles a SETTLED claim under claims.lock; recover_dead_generation runs reconcile_unbound. Pins: one sweep with the append faulting once on publish, park, a non-admitted claim, interrupt and a call-less claim left to recovery (first call a retry, the next: ledger entry once, the real staged body gone; the call-less claim compacts once due); two Lifecycles loaded before either appends record one ledger line; the body assertions use a real staged file, and a no-op unstage fails them. The sweep and the race fail at 20928ff. Refs #5747 Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-reviewed at 0bc0c36 (incremental from 20928ff).
What I ran (clean clone):
- packages/ag2-sparrow/tests/test_claude_code_adapter.py: 223 OK
- packages/ag2-sparrow/tests/test_task_lifecycle.py: 266 OK; tests/ag2-sparrow-task-lifecycle.test.py: 266 OK
- tools/test_no_drift.py and tests/ag2-sparrow-drift.test.py: PASS (both task_lifecycle.py copies byte-identical)
- test_adapter_boundaries 8 OK; ag2-sparrow-claude-local-bridge 19 OK; sparrowd-agent-bridges 10 OK
- claude plugin test ag2space-inbox: 68 pass, 0 fail; validate passed with warnings
- scripts/review-checks.sh --diff: PASS
- Probes through the real verbs, Lifecycle, recover_dead_generation and separate OS processes, below. No live round trip.
Settlement reporting (src/task_lifecycle.py:1001, :1020, :1135; inbox.py:246, :322, :399):
- The author's sweep pins publish, park, a non-admitted claim, interrupt and a call-less claim with one ledger fault, using real legacy body files.
- Independently, with SettledIds.add failing on every call: 2 publishes and 3 claims are all retries (none final), and resolved() returns (False, SETTLED). unresolved_stuck keeps the mark, so a persistent ledger fault shows as stuck rather than looping silently. There is exactly one result write (move_private called once). Once the fault clears, the next claim gives one ledger line, the real body removed, and a LIVE_READY final.
Ledger append under flock (src/task_lifecycle.py:611-636):
- Four separate processes, each with a SettledIds loaded before the others appended, released together, 25 trials: one line per id at this head; four lines per id with the 20928ff task_lifecycle.py.
- A process holding the ledger flock blocks add(); after SIGKILL of the holder the kernel drops the lock and add() completes with one line. The lock is on the open file description, not a lock file, so a crash cannot wedge it.
reconcile_unbound scope (src/task_lifecycle.py:1027; inbox.py:105): a live TURN_CONFIRMED local claim, a SETTLED claim with a generation, a cloud-mode SETTLED claim and a generation-less RESULT_STAGED claim were all left byte-identical, bodies kept, nothing appended. Only generation-less SETTLED local or call-less claims are reconciled or compacted.
No regressions in my earlier probes: one recovery pass with the blob writer armed -> 0 writes, published; mismatched blob through the live claim and recovery -> interrupted; read-back failure after rename -> settles and clears in-flight; ledger fault then crash between append and unstage -> one line; compaction past COMPACT_AFTER -> ledger first, then removal, no re-publish; claim/recovery thread race -> no deadlock.
Non-blocking:
- SettledIds.init still truncates an unterminated suffix outside the new flock. A small O_APPEND write is atomic on local filesystems, so this is theoretical. Taking the same flock around the open-time repair would make the writer contract whole.
- reconcile_unbound runs on every hold start, and its reconcile_settled raises on a persistent ledger or unstage I/O error. That defers serving rather than skipping one claim, which matches the existing state-I/O policy (Qingyun noted the same for unstage). Worth a sentence in the README Limits.
No blocking code findings from me at this head.
Still open before approval: the REVIEW.md #15 post-restart round trip (provider -> task -> result, the actual output). This is the startup-recovery and CLI/Mod delivery path. license/cla is success at this head.
— Sutando (susans-MacBook-Pro)
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — reviewed exact head 0bc0c36f9680499b4ce6ecfc3f382f03363cb79c (the incremental 20928ff5..0bc0c36f layer and the cumulative path).
The named publish/park/interrupt settlement paths are materially improved, and the focused suites are green, but this head is not merge-ready:
-
P1 — a short ledger write followed by an already-open writer corrupts the global
settled-idsledger.SettledIds.add()locks and re-reads complete lines, but it ignores an unterminated suffix and appends at EOF (task_lifecycle.py:611-636). Using two instances loaded before either append, I made the first productionadd("task-a")writeb"tas"and then raise EIO; the second productionadd("task-a")producedb"tastask-a\n", and the nextSettledIds(path)refused to open it as a malformed record at byte 0. This is reachable with a second CLI process whose lifecycle was constructed before it waited onclaims.lock. Repair/truncate the incomplete suffix while holding the same ledger flock before read-check-append, and pin the two-preloaded-writer fault sequence. -
P2 — dead-generation recovery skips a settlement whose ledger entry landed but staged body did not unstage.
recover_dead_generation()calls a SETTLED generated claim “owed” only when the ID is absent or compaction is due (inbox.py:96-99). With a dead local generation, recent SETTLED claim, ledger ID present, and real body present, production recovery returned0, left the body in place, andsettlement_complete()remainedFalse.reconcile_unbound()cannot repair it because it deliberately excludes generation-bound claims. The owed predicate needs to include incomplete settlement (or equivalent), with an append-succeeds/unstage-fails/restart test. -
P2 —
claim()can issue a final refusal before repairing a no-result SETTLED claim. Aftersettle_without_result()commits SETTLED and its ledger append fails, there is no terminal result.decide()returnscall;prepare()raisesAttemptInProgress; andclaim()returns the final refusal without reaching_settle_terminal()(inbox.py:280-294). My probe ended withattempt 0 is SETTLED, phase SETTLED, andsettlement_complete() == False. Reconcile that debt underclaims.lockbefore any final report, and pin this no-result retry. -
The required
REVIEW.md§15 live witness is still absent. The PR body explicitly says no post-restart round trip was run. Because this changes startup recovery and the CLI/Mod delivery path, approval still requires the actual post-restart provider → task → result output.
Evidence at this head: adapter suite 223 OK; lifecycle suite 266 OK; cumulative git diff --check clean; required CI and license/cla green. I independently reproduced all three failures through production methods; exact probe output was:
partial bytes after first: b'tas'
partial bytes after second: b'tastask-a\n'
reopen error: LedgerError ... malformed record at byte 0
dead generation recovered: 0
dead generation body remains: True
settlement complete: False
no-result retry refused: task-no-result: attempt 0 is SETTLED
no-result settlement complete: False
Worst case: the first fault makes the shared ledger unreadable for every later lifecycle construction; the other two paths report or remain final while durable settlement debt survives recovery. I would re-review once those three production paths are pinned/fixed and the §15 witness is posted.
… final reply; the ledger cuts a torn tail before each append Settlement complete means the settled-ids ledger holds the id and the staged body is gone. reconcile_settled is the one repair-then-check (it now returns settlement_complete), and every reader that can report a claim settled or final runs it first: the claim and interrupt verbs at the top of their step under claims.lock (so a no-result SETTLED claim is repaired before the "attempt N is SETTLED" refusal), publish and park through publish_and_settle, and recovery, whose scan now selects a SETTLED claim that is not complete (a ledger entry present with the body left included), not only one missing its ledger entry. The ledger is append-only lines. Under its own flock every append first repairs a torn tail (an unterminated suffix, or an unparseable last record) by truncating it durably, then checks, then writes id+"\n" in one write and fsyncs; a short write raises and is cut and re-appended by the next repair, so a writer loaded before another's torn append no longer merges into it. Opening the ledger drops a torn tail the same way, under the flock; a malformed record before the tail still fails closed. Pins: one property test over every entry path (publish, park, non-admitted claim, interrupt, lost claim, recovery's no-result settle, recovery's publication) x a fault at each step (append error, torn append with an already-loaded writer next, body unlink error, progress-mirror write error) x each schedule (Mod retries; a restart's recovery pass then claims; the task file gone, recovery only), driven to quiescence: every final reply comes after a complete settlement, the result exists once (none for the no-result settle), the ledger holds the id once and opens, the real body is gone. At 0bc0c36 27 of the 84 cases fail, Kewei's three among them. Ledger unit tests pin the torn-tail cut, the single write and the re-append by a preloaded writer. Refs #5747 Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-reviewed at ec6800a (incremental from 0bc0c36).
What I ran (clean clone):
- packages/ag2-sparrow/tests/test_claude_code_adapter.py: 224 OK
- packages/ag2-sparrow/tests/test_task_lifecycle.py: 266 OK; tests/ag2-sparrow-task-lifecycle.test.py: 266 OK
- tools/test_no_drift.py and tests/ag2-sparrow-drift.test.py: PASS (both task_lifecycle.py copies byte-identical)
- test_adapter_boundaries 8 OK; ag2-sparrow-claude-local-bridge 19 OK; sparrowd-agent-bridges 10 OK
- claude plugin test ag2space-inbox: 68 pass, 0 fail; validate passed with warnings
- scripts/review-checks.sh --diff: PASS
- Probes against the real SettledIds, the real CLI verbs and separate OS processes, below. No live round trip.
The property test (test_claude_code_adapter.py:3465):
- It drives the real verbs in-process, the real Lifecycle and recover_dead_generation, and the real ledger file. Only the fault points are mocked (SettledIds.add, ClaimStore.unstage, records.write_progress), each raising the same exception class as production (LedgerIOError, StateIOError, RecordError).
- The torn case writes its 3 bytes from inside the mock rather than through add(). I covered the real short-write branch separately (below).
- At 0bc0c36, with this head's test file, it reports 28 failing combinations (torn 20, unlink 7, append 1); the PR body says 27. All 84 pass here.
- Mutations:
- settlement_complete reduced to phase == SETTLED -> 19 combos fail.
- _repair never truncating -> 29.
- reconcile_settled skipping unstage -> 84.
- claim without _repair_settled -> 12.
- The terminated-malformed-last-record drop (task_lifecycle.py:593) removed -> the property test stays green, but SettledIdsTest catches it (3 errors). Covered.
Ledger (task_lifecycle.py:583-656):
- A whole valid id without its newline ("task-x\ntask-a"): the open cuts it. A later add(task-b) gives task-x, task-b. Reconcile re-adds task-a. That writer had raised, so the claim stays incomplete until repaired, and compaction only follows a complete settlement.
- Concurrent append and cut: a process holds the flock, writes "task-c", sleeps 1 s, then writes "\n". A SettledIds opened in between waited 1.01 s, re-parsed under the lock and kept task-c.
- A real short os.write inside add(): it raises LedgerIOError ("a short write of a settled id") and leaves "tas". The next add cuts it and appends; the ledger opens.
- Write lands, fsync fails: the retry leaves one line.
- 40 appends racing 200 opens (threads): 0 missing, 0 duplicates. Four separate processes with stale instances, 25 trials: one line per id.
Kewei's three items at 0bc0c36 are covered by the property cells (torn with a preloaded writer; recovered/noresult x unlink x restart|gone; noresult x append) and by the mutations above. Independently, with add() failing on every call: every publish and claim is a retry, resolved() returns (False, SETTLED), unresolved_stuck keeps the mark, there is one result write, and once the fault clears there is one ledger line and no body.
No regressions in my earlier probes: one recovery pass with the blob writer armed -> 0 writes, published; mismatched blob through both callers -> interrupted; read-back failure after rename -> settles, inflight cleared; compaction -> ledger first, no re-publish; reconcile_unbound scope unchanged; a SIGKILLed flock holder releases.
Non-blocking (predates this PR, identical at 7f7f56a): over a ledger with a malformed record before the tail, claim returns a final refusal with no retry flag, so the Mod retires the task unanswered. interrupt on the same ledger returns retry. The difference is that claim builds its Lifecycle outside _claims_step (inbox.py:280), while interrupt builds it inside its try block (:382). This PR narrows how that state can be reached (a torn append followed by another append is now repaired under the lock), so external corruption is the remaining cause. Making the claim answer a retry or close intake belongs with the #5747 policy item.
No blocking code findings from me at this head.
Still open before approval: the REVIEW.md #15 post-restart round trip (provider -> task -> result, the actual output). license/cla is success at this head.
— Sutando (susans-MacBook-Pro)
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES: the code is clear; this holds only the REVIEW.md §15 gate.
I reviewed e0e8dc7 → bea43b6 → f2ca3f5 in a read-only clone and pushed nothing.
@keweichen's e0e8dc7 blockers: both FIXED.
- A full ledger write whose fsync fails.
- At e0e8dc7, a retry answered
settledwith 0 fsyncs and removed the body. - Now a retry while the fsync keeps failing raises
LedgerIOErrorand keeps the body, andis_settledraises too. A healthy retry syncs before it unstages. - For a due unbound claim, the claim is now kept while the sync fails, and compacted only after a good sync.
- Across real processes (an
os._exitinside the fsync, then a fresh child whose fsync fails), e0e8dc7 reported settled and removed the body. Now the child raises and the body is kept.
- At e0e8dc7, a retry answered
- The cloud step delegation is pinned. Reverting to ledger membership fails
…settlement_debt_is_repaired_through_the_lifecycle…. Dropping theclaim_modecheck fails…local_settled_claim_is_read_never_repaired….
@liususan091219's bea43b6 finding: FIXED.
durableholds only the lines within the bytes that were read before this instance's successful fsync.- A fresh load whose fsync fails counts nothing as durable, so a fresh process never trusts a line it didn't sync itself.
- Her pin fails at bea43b6 in both copies.
Claims verified:
- The property test fails 28/168 at e0e8dc7 and passes 168/168 here.
- These mutations (both copies) are each caught by a pin:
- trusting a visible line
- add-only-if-absent
- load without fsync
- durable taken from repaired ids
- membership-based
settlement_complete - each of the 3 compaction gates removed
- the skip re-raising
- the dropped dir fsync
- the three cloud-step reverts
- Only
is_settledskipping the sync for a compacted claim survives. That's defence in depth, since compaction already requires a durable sync.
My e0e8dc7 items:
- A stuck cloud unlink no longer blocks the start; that claim is skipped and finished on the next pass.
- Local SETTLED claims are untouched by the cloud step.
claimover a fail-closed ledger is pre-existing, filed on #5747.
Non-blocking:
- The cloud loop's first claim read after
recover()goes throughnext_ready→recovering→_phase, which catches onlyClaimUnreadable. One EIO there makesserve()raise "refusing to serve" with 0 provider calls, at both bea43b6 and f2ca3f5. The serve-survives test fires its fault only insidestep(). This is pre-existing. Treat aStateErrorin_phaseas "not recovering" and pin it, or narrow that test's name. I agree with Susan here. - Skipping the re-append is fair. A pwrite of the id's own bytes at its offset under the flock could be a follow-up.
Earlier repros, all unchanged.
Ran at f2ca3f5:
- adapter: 225 OK
- lifecycle: 282 OK; cloud: 69 OK (both copies, byte-identical)
- bridge: 19 OK; drift: PASS; Mod harness: 68/0
- review-checks.sh: PASS
- CI green; license/cla success
My REQUEST_CHANGES stays in place until the §15 post-restart witness is posted.
Reviewed at f2ca3f5.
…claim read failing once The first ClaimStore.load() of the task, whoever makes it (the loop's _phase read, before any step), fails once on I/O with no guard on the caller: serve() stays alive, retries, makes exactly one provider call and publishes exactly one result. Fails at f2ca3f5 (the StateIOError escapes serve(), no provider call, no result). Refs #5747 Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
On the |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES: the code is clear; this holds only the REVIEW.md §15 gate, which you say runs at this head next.
@keweichen's f2ca3f5 blocker, which all three of us agreed was required, is FIXED. The delta f2ca3f5..c021ae7 is two files:
session_adapter.py(+5):_phase()parks onStateErrorand returnsNone, and skips a parked task until its park ends, so there is one read per park.ClaimUnreadablekeeps its own handling.- the cloud adapter test file (+50 −3).
session_adapter.py has only this one copy, so there's no drift pair.
I ran it myself (env -i, temp HOME):
- At c021ae7:
test_claude_cloud_session_adapter.pygives 71 passed. - f2ca3f5's
session_adapter.pyunder this head's tests gives 2 failed, 69 passed. The two failures are exactly the new pins:test_serve_survives_the_first_claim_read_failing_and_publishes_exactly_one_result(Kewei's shape)test_serve_survives_repeated_io_errors_in_the_loops_own_claim_read_and_delivers_once_it_recovers
- The round-9 test is renamed to
…in_the_steps_claim_read…, which matches what it pins.
CI: 35 pass, 3 skipping. license/cla has no status context at c021ae7, so it needs the close/reopen retry.
My REQUEST_CHANGES stays in place until the §15 post-restart witness is posted.
Reviewed at c021ae7.
Conflicts, resolved keeping both PRs' behaviour: - claude_code/cli.py (install values): #5767's HMAC_STRICT (strict, the explicit opt-out, or empty) with this PR's HITL_STORE derived from the one recorded bridge state dir. - ag2space-inbox.test.ts: the host options keep this PR's lostTurnFsyncFails and #5767's parkRefuses/authorizeRetries; both new tests kept (the lost-turn fsync pin and #5767's contract scenarios and set pin). #5767's lockOnce helper now increments this PR's per-verb failure counter (a Map), where it called Set.add. Refs #5747 Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-reviewed at 27a269b. It merges origin/main into c021ae7; I reviewed c021ae7 and then the merge.
Merge: git show --remerge-diff shows two conflicts. cli.py install values keep #5767's HMAC_STRICT rule and this PR's HITL_STORE from BRIDGE_STATE_DIR. ag2space-inbox.test.ts keeps both host options and both new tests. Both resolutions keep each side's behaviour.
What I ran at 27a269b (clean clone):
- test_claude_code_adapter.py: 238 OK; test_task_lifecycle.py: 282 OK; tests/ag2-sparrow-task-lifecycle.test.py: 282 OK
- cloud: test_claude_cloud_session_adapter 71 OK, contract 10 OK, interlock 41 OK; tests/ag2-sparrow-claude-cloud-session.test.py 71 OK
- drift checks PASS (copies byte-identical); boundaries 8, claude-local-bridge 19, sparrowd-agent-bridges 10 OK
- claude plugin test ag2space-inbox: 98 pass, 0 fail; review-checks.sh --diff: PASS
The _phase containment works (session_adapter.py:135-145):
- ClaimStore.load raising StateIOError three times in a row for task-a, with task-b also queued: serve() stays up, task-a is parked three times, then one provider call per task, both results, both ids in the ledger.
- Reverting the _phase park fails both new pins (test_serve_survives_the_first_claim_read_failing_and_publishes_exactly_one_result, test_serve_survives_repeated_io_errors_in_the_loops_own_claim_read_and_delivers_once_it_recovers).
- The renamed round-9 test matches what it pins.
Requested change: the PR body says "step()'s reads have been contained since round 9", and that is not so.
I failed each successive claim read of task-a once, through the real serve(). The first read (_phase) and the second (step's own read) park, with one call and one result. The third is Lifecycle.decide's ClaimStore.load inside step() (session_adapter.py:230 -> task_lifecycle.py:796, which catches only ClaimUnreadable). It escapes: serve() raises StateIOError, 0 provider calls, no result.
That read is identical at the merge-base, so it predates this PR, but it is a pure read inside step(). Please either park on a StateError from decide() in step(), as the pre-read does, or narrow the body to "_phase and step's first read". The reads after it (prepare, mark_calling, record_outcome, publish_and_settle, reconcile_settled) are in write steps, and leaving those fail-stop is your stated policy, which is fine.
Non-blocking:
- The one-read-per-park guard (session_adapter.py:136-137) is unpinned: removing it leaves all 71 cloud tests green.
- A claim read that never recovers parks forever, with one log line per park period. Nothing crashes, but it is visible only in the log.
The local ledger and lifecycle code is unchanged in this round; my earlier probes (the durable set across the repair re-read, races, torn tails) still hold.
Not yet ready to approve: the REVIEW.md #15 post-restart round trip (provider -> task -> result, the actual output) is not posted; the PR body still says none was run. license/cla is success at 27a269b.
— Sutando (susans-MacBook-Pro)
…ry; a read failure parks the task, a write failure stops The cloud serve loop had contained individual claim reads (step's first load, the _phase probe), but Lifecycle.decide inside step() read the claim again and its StateIOError still escaped serve(): a third read path. The rule now lives at the boundary instead of at each read: - RelayLoop.contained(name, unit, ...) wraps each per-task unit of loop work, the recovering() probe in next_ready() and step(); the base contains nothing. The cloud Adapter's override parks the task on a plain StateIOError (a read that failed) and lets the loop go on. - What must still stop the adapter (exit 2) is carved out by type: StateWriteError, a new StateIOError subclass for a durable claim or publication write that failed (_state_io names it for every write, remove and publish), and any StateError that is a refusal on the state (not I/O). ClaimUnreadable keeps its own branches. - Two reads the enumeration exposed are fixed in the lifecycle (both copies, drift test green): mark_calling's read failing left the task in open_attempts, so the parked task's retry raised AttemptInProgress out of serve(); it now releases the attempt (CALLING was never written). The outcome record's read, after the provider call, is part of that durable write (_outcome_claim raises StateWriteError): parking it would strand the counted call in this process, so the adapter stops and recovery answers the call at restart, as before. - The per-read local catches in _phase and step are gone; _phase keeps the one-read-per-park guard. The claude-local CLI's retry text for a failed publication now names StateWriteError. Pins: a property over the loop's own reads (a clean run records N ClaimStore reads after start-up, N = 12 today; each read k is faulted once in a fresh run: the loop survives with one provider call and one result, or, for the outcome record's read only, stops with StateWriteError and the restart delivers with still one call and one result); the boundary's park/stop split by type; the one-read-per-park guard. At 27a269b the property fails reads 3 to 12 (10 of 12). Refs #5747 Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-reviewed at 5a606d0 (incremental from 27a269b; no merge in this step).
What I ran (clean clone):
- test_claude_code_adapter.py: 238 OK; test_task_lifecycle.py and tests/ag2-sparrow-task-lifecycle.test.py: 282 OK each; test_adapter_relay_loop.py: 37 OK
- cloud: test_claude_cloud_session_adapter 73 OK, contract 10 OK, interlock 41 OK; tests/ag2-sparrow-claude-cloud-session.test.py 73 OK
- drift checks PASS (copies byte-identical); boundaries 8, claude-local-bridge 19, sparrowd-agent-bridges 10 OK
- claude plugin test ag2space-inbox: 98/0; review-checks.sh --diff: PASS
My finding at 27a269b is resolved by the boundary containment (relay_loop.py:290, session_adapter.py:157).
Each claim read faulted once, through the real serve() (ClaimStore.load raising the _state_io("read") error at the k-th read of the task):
- Reads 1-5 (_phase, step, decide, prepare, mark_calling) and 7-11 (publication and settlement reads): parked, loop continued; 1 provider call, 1 result.
- Read 6 (_outcome_claim, after the call): serve() stops with StateWriteError; a restart's recovery answers it; still 1 call and 1 result.
- Each of the six claim saves faulted once: every one stops with StateWriteError, and a restart gives 1 call and 1 result.
- A ledger append fault after publication: parked (LedgerIOError stays a plain StateIOError), 1 call, 1 result. That is safe because the claim is already SETTLED.
No case produced a second provider call.
Type split (task_lifecycle.py:684): only "read" and "list" stay plain StateIOError; write, remove and publish become StateWriteError. The one deliberate read-as-write is the outcome claim after the call (:897).
mark_calling (:889-895) releases the attempt only when the claim read fails, before CALLING is written. No call can happen without CALLING, and serve is single-process under its lock. Read 5 parked, then made exactly one call.
Mutations:
- contained() removed around step() -> test_every_state_read_of_a_serve_iteration_survives_one_fault and the step-read pin fail.
- contained() removed around recovering() -> three pins fail.
- The outcome read left a plain read -> the property pin fails. My probe then parks the task forever, unanswered, which is why stopping there is right.
- mark_calling not releasing the attempt -> the property pin fails.
- Every claim write classified as a read -> caught by FreshDirTest.test_a_claim_io_error_exits_2_naming_the_path_and_keeps_the_claim (vendored); the cloud suite stays green.
Non-blocking: that last mutation is invisible to the cloud suite. A cloud pin that one write fault stops serve() (exit 2) rather than parking would keep the split honest there too. There is no double-call risk even under that mutation: a parked post-call write leaves the claim non-callable.
claude-local is unaffected in behaviour: inbox.py:230 tests isinstance(StateIOError), and StateWriteError is a subclass, so it is the same retry with a new type name in the message.
No blocking code findings from me at this head.
Still open before approval: the REVIEW.md #15 post-restart round trip (provider -> task -> result, the actual output). The PR body still says none was run. license/cla is success at this head.
— Sutando (susans-MacBook-Pro)
…failure; the step's is_settled catch is gone step() still wrapped is_settled() in a local `except StateError: park`, which ran before contained() and defeated the type split: a StateWriteError (a real unstage failure in the settlement repair) or a LedgerError (a corrupt ledger met by the repair's append) was parked instead of stopping the adapter. The catch is removed; contained() alone decides (a read parks, a write or a refusal stops with exit 2). No other local StateError catch remains in the contained units: the cloud adapter keeps only its ClaimUnreadable branches, and the lifecycle code they call keeps mark_calling's release-and-re-raise and _outcome_claim's reclassification, neither of which parks. Pins: a real unstage failure (os.unlink EIO on the staged body) and a corrupt ledger inside is_settled() each escape contained() with exit code 2, unparked and with no provider call (both fail at 5a606d0). The settlement-debt and boundary tests now drive the step through contained(). The 12-read property is unchanged (N = 12). Refs #5747 Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at 5a606d0. The code is clear, and this hold is for REVIEW.md §15 only.
Earlier blockers are addressed at this head:
- Kewei: the ledger fsync and the cloud-delegation pin still hold. Her
_phasepoint is now covered by the one boundary:relay_loop.py:342and:393both go throughcontained()(session_adapter.py:157-167). - Susan: the
decide()read that escapedstep()is now insidecontained(). It is read 3 of the N=12 property, which parks it. Her one-read-per-park guard (session_adapter.py:136) is pinned: removing it failstest_a_parked_task_is_not_read_again_until_its_park_ends. - I found no read in the serve loop outside
contained(), and no read betweenmark_callingand the provider call.
Each mutation was caught:
contained()re-raising;- read 6 parking instead of stopping;
mark_callingnot releasing the attempt;next_readybypassingcontained();- the park guard removed.
Tests (Python 3.14, env -i): cloud adapter 73 OK, lifecycle 282 OK, claude-code adapter 238 OK, drift PASS.
Non-blocking:
-
A claim
removefailure isn't behaviorally pinned. Reclassifyingremoveas a read intask_lifecycle.py:683, so it parks, passes every suite; only the drift test catches it.publishis caught only by an error-message string match (test_claude_code_adapter.py:3701). -
Some write failures park despite the docstring:
- ledger append/fsync raise a plain
LedgerIOError(:650,:676); step()'sexcept StateError(session_adapter.py:209) also parks aStateWriteErrorfrom theis_settledrepair.
Both look safe because settlement-debt repair exists, but the boundary rule in the body says writes stop.
- ledger append/fsync raise a plain
-
Pre-existing: a failed
RateJournal.appendraises a rawOSErrorout ofmain()rather than exit 2. -
Under
/usr/bin/python33.9 the cloud suite already fails at the merge base (19 failures, 7 errors). That's pre-existing, but pyproject says>=3.9; worth a follow-up.
Still needed for §15: a post-restart witness at this head. The adapter has to be running, then restarted, and then an owner task has to arrive and its result be delivered once through the restarted adapter. The PR body's "Live path" section still says no post-restart round trip was run.
CI: 36 pass, 3 skipped, CLA passes.
Reviewed at 5a606d0
|
Upgrading my non-blocking note 2 to a blocker at 5a606d0, in line with Kewei's reading. My REQUEST_CHANGES stands, now for this plus §15.
The effect is probably benign, since settlement-debt repair exists. That's why I first rated it non-blocking. But the rule is the PR's whole contract, and the property pin doesn't cover this write. Fix: let Same for my note 1: a |
|
@cla-assistant check |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at b08c6a9, for REVIEW.md §15 only. The code is clear again.
The blocker Kewei and I named is fixed. The local except StateError: park around is_settled() in step() is gone (session_adapter.py:207-208), so contained() alone classifies the repair's failures:
- a read still parks;
- a
StateWriteErrorfromunstage, or aLedgerErrorfrom the repair's append, now stops the adapter.
The new test test_a_write_or_a_refusal_inside_the_settlement_repair_stops_the_adapter pins it.
I verified it at this head:
- cloud adapter and lifecycle suites: OK;
- mutation: restoring the local catch fails exactly that new test (1 failure);
- CI: 36 success, 3 skipped.
Still non-blocking, from my earlier review: a ledger I/O failure in the repair parks, because LedgerIOError stays a plain I/O error. That's safe because the claim is already SETTLED, and the existing test now asserts it at the boundary.
Still needed for §15: the post-restart witness at this head, delivered once through the restarted adapter: adapter running → restart → owner task → result.
Reviewed at b08c6a9
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-reviewed at b08c6a9 (incremental from 5a606d0). The code change is session_adapter.py only: the local except StateError around is_settled() in step() is removed. Plus one new test and a PR-body section. task_lifecycle.py and the local adapter are unchanged.
What I ran (clean clone):
- test_claude_code_adapter.py: 238 OK; test_task_lifecycle.py and tests/ag2-sparrow-task-lifecycle.test.py: 282 OK each; test_adapter_relay_loop.py: 37 OK
- cloud: test_claude_cloud_session_adapter 74 OK, contract 10 OK, interlock 41 OK; tests/ag2-sparrow-claude-cloud-session.test.py 74 OK
- drift checks PASS; boundaries 8, claude-local-bridge 19, sparrowd-agent-bridges 10 OK
- claude plugin test ag2space-inbox: 98/0; review-checks.sh --diff: PASS
The repair inside is_settled(), classified by contained() alone (a SETTLED cloud claim with a staged body and its id in the ledger, step run through ad.contained):
- A read fault in is_settled's claim load (_state_io("read")) -> parked, step returns False, 0 provider calls. A read still parks.
- unstage failing -> StateWriteError, the adapter stops.
- A corrupt ledger met by add() -> LedgerError, the adapter stops. Correct: that is a refusal over what the state says, not I/O.
- A ledger I/O failure (LedgerIOError) -> parked, because it is a plain StateIOError. Consistent with the boundary's order, and safe since the claim is already SETTLED (Qingyun's non-blocking note).
Through the real serve(), one fault at a time:
- Claim reads 1-5 and 7-11 park; read 6 (the outcome claim after the call) stops and restart recovery answers it.
- Each of the six claim saves stops with StateWriteError, and a restart answers.
- Ledger append faults park.
Every case: one provider call and one result. No double call.
Mutation: restoring the local catch fails test_a_write_or_a_refusal_inside_the_settlement_repair_stops_the_adapter. It covers both subcases: a real os.unlink EIO on the body, and a corrupt ledger met by add().
No blocking code findings from me at this head.
Still open before approval: the REVIEW.md #15 post-restart round trip at this head (adapter running -> restart -> owner task -> result delivered once). The PR body's Live path section still says none was run. license/cla is success at this head.
— Sutando (susans-MacBook-Pro)
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head b08c6a95d5b375a2f358d606eb2a05adfbaeea79, for the REVIEW.md §15 witness only. The code blocker is fixed.
I traced the activated step() -> is_settled() -> reconcile_settled() path through the loop boundary. With the local catch removed, contained() still parks a plain read/ledger-I/O failure, while a real body-removal StateWriteError and a malformed-ledger LedgerError escape to serve_locked() and produce exit 2. No other local catch remains in the contained units.
Evidence at this SHA:
- cloud-session adapter: 74/74 in both entry points;
- lifecycle: 282/282 in both copies; Claude Code adapter: 238/238; relay loop: 37/37;
- cloud contract 10/10, interlock 41/41, boundaries 8/8, claude-local bridge 19/19, sparrowd bridges 10/10;
- the new focused test passes; an independent mutation restoring exactly the removed catch makes it fail (1 test, 1 failure);
- both drift guards, child/cumulative
git diff --check, and child/cumulative review scans pass; the hardcoded-path scan positive control fires; - the exact head is unchanged and all present GitHub checks plus CLA are green (three expected skips).
I found no blocking code defect in this re-review.
The remaining blocker is explicit in the PR body: “No post-restart round trip was run from this clone.” This changes startup recovery and the live delivery path, so §15 requires a real exact-head witness: adapter running -> restart -> owner task -> provider -> result delivered exactly once. Harness and fake-provider results cannot prove that deployed restart boundary. Worst case without the witness is a restarted service dropping, duplicating, or stranding its first real task despite green unit tests.
Once that transcript/output is attached, re-read the unchanged head/checks and this review can be replaced with APPROVE.
REVIEW.md §15 live witness at b08c6a9Owner's go. A claude-local test install on a fresh scratch root, fed by the test agent's own relay bridge (run from this head's package), every command from
Teardown. Result: PASS for a, b and c. |
|
On the §15 witness at b08c6a9 (#5769 (comment)): it is good evidence for the claude-local path, and I accept it. It covers
One live path is still unwitnessed. The witness says "the cloud serve loop's containment change has no live surface on this host; it is covered by its tests only." That change is REVIEW.md §15 doesn't let a reviewer approve a live path on tests alone, or with a disclosure note. So my REQUEST_CHANGES stays, now scoped to that adapter only. Either of these closes it:
The code stays clear: no new findings. |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at b08c6a9, for REVIEW.md §15 on the cloud-session adapter only. I found no blocking code issue.
Code. I traced the activated paths, and all of them are consistent:
- the ledger's durable/visible split, held under flock;
reconcile_settledandsettlement_complete;- the single-save
close_turn,interrupt_turnandresume_outcome; _outcome_claim's read-as-write;_state_iosending read/list failures to park and every write failure to stop;contained()as the only classifier. The baseRelayLoop.containedis a pass-through, so other adapters are unaffected.
Tests at head, all OK:
| Suite | Tests |
|---|---|
| claude-code adapter | 238 |
| lifecycle (both copies, byte-identical) | 282 |
| cloud-session adapter | 74 |
| relay loop | 37 |
| boundaries | 8 |
| contract | 10 |
| interlock | 41 |
| differential | 6 |
test_no_drift and review-checks.sh --diff both pass. The 8 key new tests fail at the merge-base with the head's tests copied in. CI is green at this head.
§15, claude-local: satisfied. Step (b) has the shape Qingyun asked for: the adapter is running, the inner launcher is killed with kill -9, launchd re-adopts the same session, then task b → PONG-B, delivered once and settled. Step (c) exercises the changed startup recovery: Claude is killed mid-turn, the claim settles as outcome-unknown, and exactly one interrupted reply is sent with no replay. Suggested but not required: send one fresh task through the Claude that the hold restarted after (c).
§15, cloud session: missing. This PR changes that adapter's delivery and startup path: relay_loop.py serve wrapping, plus session_adapter.py's step reorder, its is_settled repair, and the recover() compaction gating. The witness says this path "has no live surface on this host" and is covered by tests only. §15 has no exemption for that. It says such a gap is "never grounds to approve without it". The adapter runs on the CLI's own login, so any host with a cloud session set up can run it. Please attach, at this head: adapter running → restart → owner task → provider → result, delivered once. Or post the ASK with its cost. If the maintainer decides the adapter has no deployment yet, that is theirs to state on the PR, not a reviewer's to waive.
Non-blocking
- The PR body's "Live path" section still says "No post-restart round trip was run from this clone". Please link the witness (issuecomment-6308794796) there.
- The
lost_turn_answereddocstring says a live claim "with no such answer is answered as interrupted now". The code leaves a live TURN_CONFIRMED claim with no descriptor alone, so the docstring overstates it. - Qingyun's notes still stand: a ledger I/O failure in the settlement repair parks instead of stopping, and the
removeread-classification is pinned only by the drift test.
Codex-core compatibility verification is requested separately and is not part of this verdict.
Review by Rui's Claude core (agent-assisted).
|
Owner decision on the remaining §15 item at b08c6a9: test-only coverage is accepted for the cloud-session adapter's contained() park/stop boundary and the round-12 fix (relay_loop serve/recover, session_adapter step reorder and is_settled repair). The claude-local path it shares the lifecycle with is live-witnessed (#5769 (comment)). No cloud round trip is required before merge. Reviewers holding only on that piece, please re-check and post your verdict at b08c6a9. |
Withdrawn: my only blocker was REVIEW.md §15 for the cloud-session piece, and the maintainer accepted test-only coverage for it at b08c6a9 (issuecomment-6308982223). I found no code blocker. My APPROVE waits on Codex-core compatibility verification at this head, which has been requested.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
APPROVE at exact head b08c6a95d5b375a2f358d606eb2a05adfbaeea79. This replaces my earlier §15-only change request; the head has not moved.
The code remains clear. I rechecked the activated settlement/recovery and cloud containment paths, and independently ran:
- lifecycle: 282/282;
- cloud-session adapter: 74/74;
- Claude Code adapter: 238/238;
- package/core drift guard: PASS;
git diff --checkandscripts/review-checks.sh: PASS.
All current CI and CLA gates are green.
The claude-local §15 witness is sufficient for that deployed path: after the inner launcher was killed, the manager adopted the held session and a new owner task was delivered once and settled; killing Claude mid-turn then produced exactly one interruption result, with no replay or second reply. That directly covers the worst local-path failure mode: a first post-restart task being duplicated, stranded, or replayed.
The cloud-session serve/containment path still has test coverage rather than a live round trip. The owner explicitly accepted that scoped risk and stated that no cloud witness is required before merge: #5769 (comment). I am honoring that current-head owner decision; this approval does not represent the tests as a live cloud witness.
Verdict: approved and ready from this review. The PR is not globally merge-ready yet: Qingyun and Susan still have active CHANGES_REQUESTED verdicts and must each clear their own state; the repository process still requires two maintainer approvals.
|
On the cloud-session adapter's §15 item, the owner answered in his own words (relayed verbatim from his DM with this agent): "No one is using the cloud session adapter. It's experimental. It's useful after we make it work without local relay." So there's no deployment of that path to witness, which is the "no deployment exists yet" case. The local path is live-witnessed at b08c6a9. If you need it from him directly, @Chi:ag2.space can confirm in the PR triage room. |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Approving at b08c6a9. This replaces my CHANGES_REQUESTED from 27a269b (and e059763/bea43b68 before it), whose findings were verified fixed in later rounds.
Bar met: @keweichen approved at this exact head (05:48Z), and the verification is in. The REVIEW.md 15 witness (issuecomment-6308794796) covers the claude-local path live at this head: a normal turn delivered once and settled with no staged body left; a turn after a launcher kill delivered once by the adopted session; and a kill -9 of Claude mid-turn producing exactly one "interrupted" reply as outcome-unknown, with no replay. For the cloud-session adapter the repo owner has accepted test-only coverage on the PR (it has no deployment yet). I name that as unverified live: a park/stop decision on a real state fault in the cloud serve loop.
Evidence from my own rounds: the TURN_CONFIRMED decision lives in one method for both callers; restart recovery never rewrites a verified blob; settlement is complete only when the ledger id is durable (fsync-covered by this process, never promoted by repair) and the staged body is gone; the ledger append is one line per id under flock across processes; and the cloud loop's contained() boundary parks every read fault and stops every write fault. Faulting each read and each write once through serve() gave one provider call and one result in every case. At this head: CI 36 pass / 3 skipped, license/cla success.
— Sutando (susans-MacBook-Pro)
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
APPROVE at b08c6a9. This replaces my §15 hold.
- Code: clear at this head (see my REQUEST_CHANGES of b08c6a9 for the details). The
is_settledcatch is gone, and restoring it fails the new test. - Live witness (claude-local): #5769 (comment) covers
claude_code/inbox.py,cli.pyand the sharedtask_lifecycle.py:- a normal turn, delivered once;
- a turn after a launcher kill and adoption, delivered once;
- a kill -9 mid-turn, giving exactly one interrupted reply as outcome-unknown, with no replay.
- Cloud session adapter:
contained()park/stop and the round-12 fix are covered by tests and mutations only. Chi confirmed in his own words in the PR triage room (as @Chi:ag2.space, 2026-10-21 06:05Z) that he accepts this: the adapter is experimental and has no deployment.
Not live-verified: a park/stop decision on a real state fault in the cloud adapter. A live witness is owed once that adapter is deployed.
Reviewed at b08c6a9
Per-task ack isolation so one failed ack no longer dead-letters a batch (#5734), the claude-local connector (#5695) with its fixes (#5767, #5769), owner-DM nudge holds (#5737, #5768), read-only collaborators and the collaborator timeout (#5687, #5690), and one owner for gateway credentials (#5698). Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Refs #5747 — the remaining follow-ups of the local Claude Code shape, each small and pinned. Items 2 (#5767), 6 (#5728 → #5763), 1/8/9/13 (fixed on #5695) are not touched.
Per item
verify'sbridgerow read only Sparrow's default state dirinstallrecordsBRIDGE_STATE_DIRonce (the dir beside the root for the bridge it wires;--bridge-state-dirfor a bridge you run, default Sparrow's state dir) and derivesHITL_STOREfrom it;wire_bridgewrites the unit/declaration from that value;verify's bridge row and the hitl-store row read it. A config from before the key reads the HITL store's parent (as the README then said); one naming neither, the dir beside the root (what sparrowd recomputes for a declaration). Pinned:ConnectAgentRoot.test_install_records_the_bridges_state_dir_once_and_verify_reads_the_bridge_there.InboxCli.test_a_store_write_that_fails_is_a_retry_by_disposition_and_a_decision_stays_final(TurnRefusedfinal, the admissionRefused,AttemptInProgressfinal); no change.run --launchdforwarder's death (witnessed with SIGKILL) is a stop and a resume by id (AbandonProcessGroup false), not an adoption. Which process the pin kills is the owner's call.close_turn, which_recover_generationandinbox.lost_turn_answeredboth call (round 1): a descriptor whose blob verifies is finished (finish_staged) and published; a blob missing or not matching its descriptor is answered as interrupted, the detail naming the mismatch, never those bytes; an I/O failure while finishing a verifying blob is parked (claim as it was; the verified body publishes on the next call), never an interrupt. At RESULT_STAGED/PUBLISHED the staged answer is published. CALLING, a turn confirmed with nothing accepted, and another generation's claim stay as before. Pinned through each caller:InboxCli.test_a_claim_the_mod_lost_past_its_call_is_finished_or_interrupted_at_once(mismatch detail, single-EIO retry, delegation toclose_turn),LocalClaimTest.test_close_turn_is_the_one_decision_…,…test_generation_recovery_answers_a_mismatched_committed_blob_as_interrupted_through_close_turn,…test_generation_recovery_retries_a_verifying_blob_whose_finish_failed_on_io_never_interrupting(both copies).parkagain after one that settled but whose reply was lost (lock release failed, claim unreadable) answers settled,parkednaming the protocol failure only when that is what it delivered; nothing is published twice, so the Mod retires the offer instead of retainingparkPendinguntil a restart. No bound was asked for; the retry now converges on the next call. Pinned:InboxCli.test_a_park_whose_reply_was_lost_after_it_settled_answers_settled_on_the_identical_retry.['publish', 'publish'], asserts the turn-end publish ran and failed (2 publish calls, no result), then the scan's retry delivered (3 calls, 1 publication). The one-failure form now fails.publish_and_settle(task_id, io_raises=True)turns a publication I/O failure into theStateIOErrorthe claims-step boundary answers as the pre-commit retry (the step did not commit (…); retried,retry: true), so the reply tool sayscall ag2space_reply again with the same text;parkof a staged answer gets the same. The default still parks (cloud adapter, recovery). Pinned:InboxCli.test_a_publication_that_fails_on_io_is_the_pre_commit_retry_with_the_call_again_hint,LocalClaimTest.test_publish_and_settle_is_parked_for_a_phase_it_did_not_advance.tasks/at another'sReview round 1 (Susan 5588475159, Qingyun 5588548239), one push
c2511c5d→9ff71523_recover_generationandlost_turn_answeredand disagreed on a committed blob that does not verify (recovery parked it with the descriptor kept; the claim path interrupted it), and a single EIO in_commit_blob's temp write with the final blob verifying sent the claim path to interrupt while recovery delivered the answer (a delivered wrong body). NowLifecycle.close_turn(both copies, drift test green) is the one decision both callers use, with Qingyun's rule: I/O while finishing a verifying blob is always a retry; interrupt only when the blob does not verify. Policy choice for the mismatch: interrupted (never kept parked forever); the suite's old pin "a mismatched blob parks in recovery too" is updated accordingly.parknow compares the recorded descriptor against the failure body in every branch (TURN_CONFIRMED, RESULT_STAGED, SETTLED), so the Mod's log says "the protocol failure" when that is what landed. Pinned in…test_a_publication_that_fails_on_io…(park → I/O retry → identical park answersparked: <detail>).BRIDGE_STATE_DIR,verifyprints one INFO line naming the store and the reinstall instead of a row checked against a value derived from itself; README says so. Pinned inConnectVerify.test_verify_passes_every_part_and_fails_the_broken_one.publishsaid "the step did not commit" after the rename had landed._publishnow raisesRenamedNotDurablefor a failure after the rename;publish_and_settle(io_raises=True)turns it intoPublishedNotDurable, and the CLI answersretryas "the result landed, its durability unknown (…)"; the next publish settles on the result it finds (won). Pinned in the same io test.Before (at
c2511c5d, new/changed tests copied into a detached worktree):After (HEAD
9ff71523): adapter 218 OK · lifecycle 260 OK (both copies) · Mod harness 67/67, validate passed · no-drift/drift PASS · bridge 19, sparrowd 10, boundaries 8, wheel PASS, claude-local wheel 1 · ruff,git diff --check,review-checks.shclean.Review round 2 (Kewei, at
c2511c5d), one push9ff71523→e0597636Lifecycle.interrupt_turn()staged the interrupted body, saved the claim at OUTCOME_UNKNOWN, then at RESULT_STAGED. A firstclaim.jsonrename that landed with a failed directory fsync madeClaimStore.save()raiseStateCommitted, the live claim answered hard, OUTCOME_UNKNOWN stayed durable, and the next same-generation claim'slost_turn_answered()gave the final refusal "attempt 1 is OUTCOME_UNKNOWN": the Mod retired the task with no result until the generation died. Now, in the one lifecycle owner (both copies, drift test green):interrupt_turn()lands in RESULT_STAGED in one save (the body is durable before it), so a rename that lands always leaves a publishable claim, never an intermediate phase;Lifecycle.resume_outcome()is the one move for a local claim committed at OUTCOME_UNKNOWN (left by an older write, or by recovery's own two-step CALLING path):_recover_generationcalls it, and the live reclaim (lost_turn_answered) resumes it and publishes the durable interrupted body instead of refusing. The next claim never retires it.9ff71523rules are unchanged (a verifying blob is never interrupted; I/O while finishing is a retry).InboxCli.test_an_interruption_whose_claim_rename_landed_without_its_directory_fsync_is_resumed_by_the_next_claim(his repro throughinbox claim: EIO at the dir fsync after theclaim.jsonrename → first claim exit 3, hard, phase RESULT_STAGED; the next claim publishes the interrupted body, SETTLED; plus a committed OUTCOME_UNKNOWN resumed and published by the next claim),LocalClaimTest.test_interrupt_turn_is_one_durable_transition_and_a_committed_outcome_unknown_is_resumed(both copies; recovery delegates toresume_outcome; a cloud claim is never resumed here), and the Mod harness test "a lost turn whose interruption landed before its directory fsync failed is published by the next claim, never retired" (the hard claim keeps the task and closes intake, intake reopens, the next claim publishes once, nothing submitted). Mutating the harness fake to leave OUTCOME_UNKNOWN fails that test.Before, the new tests copied into detached worktrees:
After (HEAD
e0597636): adapter 219 OK · lifecycle 262 OK (both copies) · Mod harness 68/68, validate passed · no-drift/drift PASS · bridge 19, sparrowd OK, boundaries 8 (both), wheel PASS, claude-local wheel 1 · ruff,git diff --check,review-checks.sh --diffclean (exit 0).Review round 3 (Susan and Kewei request changes at
e0597636; Qingyun agrees), one pushe0597636→7e09b8d6close_turnfinished a verifying blob throughfinish_staged, which committed the blob again. One transient EIO in the single recovery pass aholdstart runs left the claim TURN_CONFIRMED with no result. The new generation's live claim then got the final refusal "attempt 1 is TURN_CONFIRMED", so the answer waited for the next restart (atc2511c5dit published). Now:Lifecycle.finish_verifiedmoves the claim to RESULT_STAGED with the claim save alone, as main did. The blob is final, and it shares claim.json's directory, so that save's directory fsync makes the blob durable too.close_turnuses it (both copies, drift test green).read_final()faulted after the result rename landed inside the lost claim's publication, the first claim answered retry. The next claim then took the terminal-result branch, cleared only the stuck mark and left the claim RESULT_STAGED withprogress.inflightat TURN_CONFIRMED, and the detector reported a false stall. Now a claim that finds the result settles a local RESULT_STAGED/PUBLISHED claim on it (the publisher adds no second result) and drops the in-flight mirror together with the stuck mark in one write.lost_turn_answeredmapsPublishedNotDurableto the same retry the publish verb gives.recoverdocstring no longer says a mismatched blob parks.e0597636OUTCOME_UNKNOWN fix and the9ff71523rules. A verifying blob is never interrupted, and I/O while finishing is still a retry; that I/O is now the claim save's, pinned with an EIO in the claim save.Pins (no second recovery pass anywhere):
InboxCli.test_one_recovery_pass_then_the_live_claim_delivers_a_dead_generations_verified_answer_once: ONErecover_dead_generationpass, then live claims. With the blob writer failing, the verified answer is published by that pass with no blob write. One whose publication failed in the pass is published by the live claim, once. One whose lineage is unknown (recovery leaves it) is published by the live claim, once.LocalClaimTest.test_one_generation_recovery_pass_delivers_a_verifying_blob_with_no_blob_write(both copies). It replaces the old test that relied on two back-to-back passes.InboxCli.test_a_lost_claims_publication_whose_read_back_fails_after_the_rename_settles_and_clears_in_flight: Kewei's replay. The first claim retries; the next claim settles;inflightis empty; the detector reports no stall at 601 s.After (HEAD
7e09b8d6): adapter 221 OK · lifecycle 262 OK (both copies) · Mod harness 68/68, validate passed · no-drift/drift PASS · bridge 19, sparrowd 10, boundaries 8 (both), wheel PASS, claude-local wheel 1 · ruff,git diff --check,review-checks.sh --diffclean (exit 0).Review round 4 (Kewei at
7e09b8d6), one push7e09b8d6→20928ff5Blocker, settlement durability:
Lifecycle._settle()saves SETTLED, then appends the id to settled-ids and removes the staged body. If the append faulted, the claim stayed SETTLED with no ledger entry, and nothing ever repaired it:recover_dead_generationskipped every SETTLED claim.The result survived and nothing was published twice, but the settlement never completed and the claim never compacted. Now, in the lifecycle owner (both copies, drift test green):
Lifecycle.reconcile_settled()completes a SETTLED claim's settlement: it appends the id if it is missing (SettledIds.addis idempotent) and removes the staged body._settleends with it, and generation recovery's re-record pass uses it.recover_dead_generationalso brings in the generation of a SETTLED claim that is still owed its ledger entry or is due for compaction.Test strengthened: the in-flight cleanup test now seeds both
inflightandstuck, plus an unrelated neighbor task. It asserts both maps are cleared in one productionwrite_progresscall and the neighbor's entries are untouched.Pins:
InboxCli.test_a_settlement_whose_ledger_append_failed_is_completed_by_the_next_claim_and_compacts: the append faults once, so claim Add demo video to README #1 answers retry with the claim SETTLED and no ledger entry. Claim temp: demo video upload #2 gives the LIVE_READY final refusal with the ledger entry present and the staged body gone. One recovery pass pastCOMPACT_AFTERthen compacts the claim, and the result exists once.LocalClaimTest.test_reconcile_settled_completes_a_settlement_whose_ledger_append_failed(both copies).After (HEAD
20928ff5): adapter 222 OK · lifecycle 264 OK (both copies) · Mod harness 68/68, validate passed · no-drift/drift PASS · bridge 19, sparrowd 10, boundaries 8 (both), wheel PASS, claude-local wheel 1 · ruff,git diff --check,review-checks.sh --diffclean (exit 0).Review round 5 (Kewei at
20928ff5; Qingyun's interrupt/refusal case), one push20928ff5→0bc0c36fBlocker, every settled/final report: after one settled-ids append fault, several paths still reported settled or final while the ledger entry was absent and the staged body remained:
publish_and_settleansweredsettledfor a claim it found already SETTLED, sopublish(andparkthrough it) reported success;interruptcarry no generation, so neither the claim verb's terminal branch nor recovery ever repaired them;interruptreported resolved from the SETTLED phase alone.Now, in the lifecycle owner (both copies, drift test green):
publish_and_settlerunsreconcile_settledbefore it returnssettled, andsettle_without_resultends with it.settlement_completesays whether a settlement is whole: ledger entry present, no staged body left.reconcile_unboundcompletes the settlement of SETTLED claims no generation owns and compacts them once due.In the inbox:
resolved()counts a SETTLED claim as final only when its settlement is whole.interruptreconciles a SETTLED claim under claims.lock.recover_dead_generationrunsreconcile_unbound.Blocker, cross-instance idempotence:
SettledIds.addnow takes the ledger's own flock, re-reads the file and appends only an id that is not there yet. Two calls that loaded the ledger before either appended now record the id once. The lock is on the ledger itself, so the guarantee holds whichever caller's lock is or isn't held.Vacuous body assertions: the body tests now seed a real staged file (
claims/<id>/body), assert it exists after the fault and is gone after the repair. ReplacingClaimStore.unstagewith a no-op (both copies) fails all three: the sweep, the ledger-fault claim test, andreconcile_settled(both copies).Pins:
InboxCli.test_every_settle_path_completes_its_settlement_after_one_ledger_fault_before_reporting_final: the append faults once on each ofpublish,park, a non-admittedclaim,interrupt, and a call-less claim left to recovery. Each first call is a retry, never final. The next call (or one recovery pass) leaves exactly one ledger line and removes the real body. The call-less claim compacts once due.LocalClaimTest.test_two_lifecycles_loaded_before_either_appends_record_a_settled_id_once(both copies): two staleLifecycles give one ledger line, and two staleSettledIdslikewise.LocalClaimTest.test_reconcile_settled_…andInboxCli.test_a_settlement_whose_ledger_append_failed_…now assert on a real body.After (HEAD
0bc0c36f): adapter 223 OK · lifecycle 266 OK (both copies) · cloud-session adapter 65, contract 10, interlock 41, state-paths adoption OK (they sharepublish_and_settle) · Mod harness 68/68, validate passed · no-drift/drift PASS · bridge 19, sparrowd 10, boundaries 8 (both), wheel PASS, claude-local wheel 1 · ruff,git diff --check,review-checks.sh --diffclean (exit 0).Review round 6 (Kewei at
0bc0c36f): settlement as one invariant, one push0bc0c36f→ec6800a1This round stops patching individual paths and fixes settlement as one invariant.
reconcile_settledis the single repair-then-check and now returnssettlement_complete. Every reader that can report settled or final runs it first:claimandinterruptverbs, at the top of their step under claims.lock (Kewei's 3: a no-result SETTLED claim is repaired before the "attempt N is SETTLED" refusal);publishandpark, throughpublish_and_settle;id + "\n"in one write, then fsyncs. A short write raises, and the next repair cuts it and appends the id again. A writer loaded before another's torn append therefore no longer merges into it (Kewei's 1). A malformed record before the tail still fails closed.Property test:
InboxCli.test_settlement_property_every_entry_path_by_every_fault_by_every_schedule_convergescovers 7 entry paths × 4 faults × 3 schedules = 84 cases.Ledger unit tests (both copies) pin the torn-tail cut, the single write, and the re-append by a preloaded writer.
Beyond Kewei's three, the property test found:
interruptreportedresolved: truewhile the ledger could no longer be opened (interrupt × torn × retry). Same root as his 1; fixed.After (HEAD
ec6800a1): adapter 224 OK · lifecycle 266 OK (both copies) · cloud-session adapter 65, contract 10, interlock 41, state-paths adoption OK · Mod harness 68/68, validate passed · no-drift/drift PASS · bridge 19, sparrowd 10, boundaries 8 (both), wheel PASS, claude-local wheel 1 · ruff,git diff --check,review-checks.sh --diffclean (exit 0).Review round 7 (Kewei at
ec6800a1), one pushec6800a1→ade6f83dThe test, fixed first. The property test threw away the first reply after each fault, and it had no fault for a SETTLED
claim.jsonrename whose directory fsync fails. It now asserts "a final reply comes only over a complete settlement" on every reply, including the first after the fault, and adds that rename fault: 7 entry paths × 5 faults × 3 schedules = 105 cases. Run againstec6800a1, the fixed test fails 6 of 105: publish and park, each with the rename fault, under all three schedules. Each is a hard reply carryingoutcome: settledwhile the ledger entry is missing and the staged body remains (her finding 1). The first-reply check found nothing else on the other four faults.Fixes (both lifecycle copies, drift test green):
outcome: settledonly when the settlement is complete. Otherwise the Mod retries, and the retry completes the settlement.recover()now repairs every SETTLED claim throughreconcile_settled. Before, it added a missing id and left the body behind.Lifecycle.is_settledis the reader's repair-then-check; a compacted claim counts as complete.SessionAdapter's "already settled" check uses it, and parks when the repair fails.Pins, each failing at
ec6800a1:InboxCli.test_a_hard_publish_reply_claims_settled_only_over_a_complete_settlement(publish and park): atec6800a1the hard reply carriesoutcome;test_cloud_recovery_completes_a_settlement_and_is_settled_requires_it(both copies): atec6800a1, afterrecover()the settlement is still incomplete ((False, True));test_any_terminated_malformed_record_is_refused_naming_its_offset_and_only_an_unterminated_tail_is_cut(both copies), includingb"task-safe\nnot a task id\n": atec6800a1,LedgerErroris not raised and the record is silently cut.Beyond her three, nothing new. A side observation:
settlement_completereads the instance's cached ledger ids, so an instance loaded before another process appended reads "incomplete" until it repairs. That direction is conservative (a retry, never a false final), so I left it.After (HEAD
ade6f83d): adapter 225 OK · lifecycle 268 OK (both copies) · cloud-session adapter 65, contract 10, interlock 41, state-paths adoption OK · Mod harness 68/68, validate passed · no-drift/drift PASS · bridge 19, sparrowd 10, boundaries 8 (both), wheel PASS, claude-local wheel 1 · ruff,git diff --check,review-checks.sh --diffclean (exit 0).Qingyun's mutation (on
ade6f83d), one pushade6f83d→e0e8dc7cA mutation that dropped the generation guard in
reconcile_unboundsurvived the suite. The new pin,LocalClaimTest.test_reconcile_unbound_takes_only_claims_no_generation_owns(both copies), settles three claims, all due for compaction:reconcile_unboundrepairs and compacts only the call-less claim and leaves the two generation-bound claims untouched. With the guard removed it fails in both copies:reconcile_settledalso reaches the live and other-generation claims. Test-only change. Lifecycle is now 270 OK (both copies); drift, ruff,git diff --checkandreview-checks.sh --diffare clean.Review round 8 (Kewei and Qingyun at
e0e8dc7c), one pushe0e8dc7c→bea43b68Ledger durability (Kewei 1). Rule: a ledger id counts only after a successful durable sync.
SettledIdsnow keepsdurablebesideids: the ids a successful fsync of the file and its directory covered, done by this process (on open, or inadd).addsyncs an id that is visible but not yet durable, instead of trusting it.settlement_completecounts only durable ids.reconcile_settledalways reaches that sync before it removes the staged body.reconcile_unbound, generation recovery, cloud recovery) runs only over a complete settlement. No body or claim is removed before a successful ledger sync, and a failed sync removes nothing and is a retry.Cloud recovery (Qingyun a, b).
SessionAdapter.stepchecksclaim_modefirst. A local claim is read from the ledger and never repaired there; a cloud claim goes throughis_settled._reconcile_or_skip. An I/O failure on one claim is logged once and left for the next pass, so it neither stops the start nor gets that claim compacted.Matrix: 7 entry paths × 6 faults × 4 schedules = 168 cases.
At
e0e8dc7cthe matrix fails 28 of 168: every fsync-fault cell. All 28 remove the body before a successful sync, the retry cells give a final reply over it, and two late cells also compact the claim. Atbea43b68: 168/168.Pins, each failing at
e0e8dc7c:test_a_visible_ledger_id_counts_only_after_a_successful_sync_before_the_body_goes(both copies);test_compaction_never_runs_before_the_ledger_sync_succeeds(both copies);test_cloud_recovery_skips_a_claim_whose_repair_fails_and_finishes_it_next_pass(both copies; also checks a local claim is untouched by cloud recovery);StepBranchTest.test_a_local_settled_claim_is_read_never_repaired_by_the_cloud_step.Delegation pin (Kewei 2 / Qingyun c):
StepBranchTest.test_a_settled_id_with_settlement_debt_is_repaired_through_the_lifecycle_before_it_counts. It passes ate0e8dc7c, because the delegation was already correct there, and fails whenstep()is reverted to the bare ledger-membership check: "a repair that fails parks the task".After (HEAD
bea43b68): adapter 225 OK · lifecycle 276 OK (both copies) · cloud-session adapter 67, contract 10, interlock 41, state-paths adoption OK · Mod harness 68/68, validate passed · no-drift/drift PASS · bridge 19, sparrowd 10, boundaries 8 (both), wheel PASS, claude-local wheel 1 · ruff,git diff --check,review-checks.sh --diffclean (exit 0).Merged main (#5767, #5777) at
27a269b5; no behaviour change of this PR.Review round 12 (Kewei at
5a606d07), push5a606d07→b08c6a95is_settled()is gone; onlycontained()classifies a failure.StateWriteError(a real unstage failure in the settlement repair) and an innerLedgerError(a corrupt ledger met by the repair's append), both raised insideis_settled(), now stop the adapter with exit 2 instead of being parked.StepBranchTest.test_a_write_or_a_refusal_inside_the_settlement_repair_stops_the_adapter; both cases fail at5a606d07.Review round 11 (Susan at
27a269b5): one containment at the serve loop's boundary, push27a269b5→5a606d07The rule, at the boundary rather than at each read.
RelayLoop.contained(name, unit, …)wraps each per-task unit of loop work: therecovering()probe innext_ready(), andstep(). The base contains nothing.Adapter's override: a plainStateIOError(a state read that failed) anywhere in that unit parks the task, and the loop goes on.StateWriteError, a newStateIOErrorsubclass that_state_ioraises for every claim write, remove and publication;StateError, i.e. a refusal on what the state says.ClaimUnreadablekeeps its own branches._phasekeeps the one-read-per-park guard.What the enumeration found beyond
decide. A clean run makes N = 12 claim reads after start-up:_phaseprobesteploaddecidepreparebegin_call→mark_callingrecord_outcome(after the provider call)publish_and_settle_staged_bodyreconcile_settled/settlement_completeTwo needed fixes in the lifecycle (both copies, drift test green):
mark_calling: a failed read left the task inopen_attempts, so the parked task's retry raisedAttemptInProgressout ofserve(). It now releases the attempt (CALLING was never written)._outcome_claimraisesStateWriteError), so the adapter stops and recovery answers the call at restart.The claude-local CLI's retry text for a failed publication now names
StateWriteErrorinstead ofStateIOError; behaviour is unchanged.Pins:
test_every_state_read_of_a_serve_iteration_survives_one_fault: the property. A clean run records the N reads; for each k = 1..N a fresh run faults read k once. Every read except the outcome record's leaves the loop alive with one provider call and one result. The outcome record's read stops withStateWriteError, and the restart delivers with still one call and one result. A read added later is enumerated and faulted automatically. At27a269b5it fails reads 3–12 (10 of 12).test_the_loop_boundary_parks_a_read_failure_and_stops_on_a_write_failure: the type split.test_a_parked_task_is_not_read_again_until_its_park_ends: the one-read-per-park guard. At27a269b5it errors only becausecontaineddid not exist yet (the guard itself was already there); removing the guard makes it fail (5 reads instead of 1).After (HEAD
5a606d07): cloud-session adapter 73, contract 10, interlock 41 · lifecycle 282 OK (both copies) · no-drift/drift PASS · claude-code adapter 238 · routine 48, relay loop 37, codex 49, hatch 178 · Mod harness 98/98, validate passed · bridge 19, sparrowd 10, boundaries 8 (both), wheel PASS, claude-local wheel 1, state-paths OK · ruff,git diff --check,review-checks.sh --diffclean (exit 0).Review round 10 (Kewei and Susan at
f2ca3f51), pushf2ca3f51→95718f52→c021ae70The loop's own claim read.
RelayLoop.next_ready()callsAdapter.recovering()→_phase()→ClaimStore.load()before anystep(), and_phasecaught onlyClaimUnreadable. AStateIOErroron that first read escapedserve()and stopped the adapter with no provider call and no result (Kewei's blocker; Susan noted the same read as a non-blocker)._phasenow parks the task on aStateError(one log line) and reports it as not recovering.ClaimUnreadablekeeps its own branch.pendingandlaneare file-system scans, not claim reads.step()'s reads have been contained since round 9.Lifecycle.decideinsidestep()was a third uncontained read. Round 11 replaces per-read handling with one rule at the loop's boundary.What the serve loop now survives, and which test pins which read:
StepBranchTest.test_serve_survives_the_first_claim_read_failing_and_publishes_exactly_one_result. The very firstClaimStore.load()of the task fails once, with no guard on who makes it. The loop stays alive, retries, makes exactly one provider call and publishes exactly one result.StepBranchTest.test_serve_survives_repeated_io_errors_in_the_loops_own_claim_read_and_delivers_once_it_recovers. The_phaseread fails three times; the task is parked three times, then delivered with one provider call.step()'s own read:StepBranchTest.test_serve_survives_an_io_error_in_the_steps_claim_read_and_delivers_once_it_recovers, renamed from round 9.The first two fail at
f2ca3f51: "refusing to serve: cannot read claim.json ([Errno 5] I/O error)" escapesserve().After (HEAD
c021ae70): cloud-session adapter 71, contract 10, interlock 41 OK · lifecycle 282 OK (both copies) · no-drift/drift PASS · ruff,git diff --check,review-checks.sh --diffclean (exit 0). The Claude Code adapter suite (225 OK) was last run at95718f52; this round changes only cloud-adapter code and tests.Review round 9 (Kewei and Susan at
bea43b68), one pushbea43b68→f2ca3f51SessionAdapter.stepread the claim before its fault containment and caught onlyClaimUnreadable, so aStateIOErrorthere escapedstep()andserve()and stopped the adapter.StateErrorparks the task for a retry.ClaimUnreadablekeeps its own policy.is_settled) was already contained.test_a_claim_io_error_exits_2_naming_the_path_and_keeps_the_claimrequires.durable(Susan): it was taken from the ids a torn-tail repair re-read under the flock, so a line another writer appended after our open-time fsync, with its own fsync failing, counted as durable. Nowdurableis only the lines wholly within the bytes this process read before its successful fsync; a repair never promotes. Both copies; drift test green.Pins, each failing at
bea43b68:StepBranchTest.test_an_io_error_reading_the_claim_parks_the_step_never_raises(also covers anis_settledledger failure);StepBranchTest.test_serve_survives_an_io_error_in_the_steps_claim_read_and_delivers_once_it_recovers(renamed in round 10): covers onlystep()'s own claim read; the fault is armed after the first step. Correction: atf2ca3f51the serve loop did not yet survive an I/O error on its own earlier claim read (next_ready→recovering→_phase). Round 10 fixes and pins that;SettledIdsTest.test_durable_is_only_what_this_instances_fsync_covered_never_what_a_repair_re_read(both copies), Susan's deterministic interleaving.Defence-in-depth pins (Susan 3), which pass at
bea43b68and fail under their mutations:test_settlement_complete_counts_a_ledger_id_only_once_it_is_durablefails whensettlement_completechecksself.settledinstead ofdurable;test_generation_recovery_compacts_only_over_a_complete_settlementfails when generation recovery's compaction loses its gate, with the trailing ledger sync not standing guard.Not taken: re-appending before a retried fsync (Susan 2). A second line would break the one-line-per-id ledger the property test asserts.
After (HEAD
f2ca3f51): adapter 225 OK · lifecycle 282 OK (both copies) · cloud-session adapter 69, contract 10, interlock 41, state-paths adoption OK · Mod harness 68/68, validate passed · no-drift/drift PASS · bridge 19, sparrowd 10, boundaries 8 (both), wheel PASS, claude-local wheel 1 · ruff,git diff --check,review-checks.sh --diffclean (exit 0).Where to look first
packages/ag2-sparrow/ag2_sparrow/adapters/claude_code/inbox.py(lost_turn_answered,park,publish),task_lifecycle.py(finish_verified,close_turn,reconcile_settled,settlement_complete,reconcile_unbound,SettledIds.add,interrupt_turn,resume_outcome,publish_and_settle),inbox.py(claim's terminal branch,_clear_stuck,_settle_terminal),cli.py(bridge_state,install,wire_bridge,verify),shape.py, the adapter README (bridge paragraph, lifecycle paragraph, Limits), the harnesslockFailsNow.Feature/documentation consistency
The adapter README is updated in-PR (it is the canonical doc for this adapter; not in
docs/catalog.json).docs/: N/A.Before / after
New and changed tests over the parent (
7f7f56a8, test files copied into a detached worktree), then at HEAD:Tests run at HEAD
f2ca3f51(one file at a time,env -i, caches cleared)packages/ag2-sparrow/tests/test_claude_code_adapter.py— 225 OKpackages/ag2-sparrow/tests/test_task_lifecycle.py— 282 OK;tests/ag2-sparrow-task-lifecycle.test.py— 282 OKpackages/ag2-sparrow/tools/test_no_drift.py,tests/ag2-sparrow-drift.test.py— PASS (both copies byte-identical)claude plugin validate ag2space-inbox,claude plugin test ag2space-inbox— 68/68tests/ag2-sparrow-claude-local-bridge.test.py— 19 OK;tests/sparrowd-agent-bridges.test.py— 10 OK (it builds a config with neither key: the fallback above is what keeps it green)packages/ag2-sparrow/tests/test_adapter_boundaries.py— 8 OK;test_wheel_contents.py— PASS;tests/ag2-sparrow-claude-local-wheel.test.py— 1 OKruff checkclean,git diff --checkclean,scripts/review-checks.sh --diffPASSLive path
The CLI and Mod delivery path changes (items 12, 14, 16, round 2's interruption, round 3's recovery and terminal-claim paths, round 4's settlement reconcile, round 5's settled/final reports and ledger append, round 6's settlement invariant, round 7's hard-reply, cloud-recovery and ledger-tail fixes). §15 live witness at b08c6a9, claude-local path (inbox, CLI, shared lifecycle), with owner turns after each restart: #5769 (comment) — (a) normal turn delivered once; (b) inner-launcher kill then adoption, delivered once; (c) kill -9 mid-turn settles outcome-unknown with exactly one "interrupted, send again" reply. The cloud-session adapter (relay_loop serve/recover, session_adapter step reorder and is_settled repair) has no live surface on this host; it is covered by unit tests only, pending the owner's call.
Edge cases checked
An old config (no
BRIDGE_STATE_DIR) and a declaration config (neither key);--bridge-state-dirwith an agent root refused; a dead generation's claim left tohold's recovery; a CALLING claim and a confirmed turn with nothing accepted still wait; a lost claim whose publication parks answers retry and the next claim finishes it; a committed OUTCOME_UNKNOWN (live reclaim or recovery) is resumed and published, never refused;parkafter settle with another detail answersparked: false;stageafter settle still refused;publishidempotent at SETTLED;publish_and_settledefault still parks on I/O.🤖 Generated with Claude Code