Repository navigation
feat(ag2-sparrow): queue AG2 Space inbox tasks into one persistent Claude Code cloud session - #5660
Conversation
…aude Code cloud session A new opt-in adapter, ag2_sparrow/adapters/claude_cloud_session, turns each tasks/task-*.txt into one `claude -p <text> --cloud <session_id> --output-format json` turn in a session the owner created once and connected to AG2 Space. The session posts the reply itself, so the result is [no-send] plus the session URL. It reuses the routine adapter's payload builder, rate limiter, result lines and serve loop (build_payload gains text_max, result_body gains label), keeps owner-only by default, writes a pre-send marker so a crash mid-send is never resent, and paces turns with --min-gap-seconds and --max-per-hour (state/sends.json). Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Live witness at 42dcabc (REVIEW.md §15)One real task through the shipped adapter into a prepared cloud session, with the agent posting in a test room. Times are UTC from the session's own event log; ids are left out. Preparation (once).
The task. A Sparrow-shaped owner-tier task file in a scratch work dir ( In the session (its event log): In the room: one message from the agent with that text. Queue to post: 11 s. The session kept its memory across turns (it distinguished this task from the earlier probe message in the same session). Not exercised live: failure paths (non-zero exit, timeout, hourly cap), which the unit tests cover with a fake |
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 92%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
packages/ag2-sparrow/ag2_sparrow/task_io.pyLines 158-167 158 except OSError:
159 pass
160 try:
161 if sys.platform == "darwin":
! 162 import fcntl
! 163 path = fcntl.fcntl(fd, fcntl.F_GETPATH, bytes(1024)).split(b"\0", 1)[0].decode()
164 else:
165 path = os.readlink("/proc/self/fd/%d" % fd)
166 st, held = os.stat(path), os.fstat(fd)
167 except OSError as e:packages/ag2-sparrow/ag2_sparrow/task_priority.pyLines 23-33 23 # Relative inside the ag2-sparrow package copy; a flat load (importlib in tests, tools,
24 # bridges) has no parent package, so src/ goes on sys.path instead.
25 try:
26 from . import local_task_protocol as _ltp
! 27 except ImportError:
! 28 sys.path.insert(0, str(Path(__file__).resolve().parent))
! 29 import local_task_protocol as _ltp # noqa: E402
30
31 _ORDER = {"urgent": 0, "normal": 1, "low": 2}
32 _VALID = frozenset(_ORDER.keys())
33 _DEFAULT = "normal"Lines 34-42 34
35
36 def is_valid_priority(value: str) -> bool:
37 """True iff `value` is a recognized priority enum string."""
! 38 return value in _VALID
39
40
41 def default_priority_for_source(source: str, access_tier: str | None = None) -> str:
42 """Recommended priority for a given source. Writers should pass theLines 40-59 40
41 def default_priority_for_source(source: str, access_tier: str | None = None) -> str:
42 """Recommended priority for a given source. Writers should pass the
43 `access_tier` (when known) so non-owner channel tasks demote correctly."""
! 44 s = (source or "").lower().strip()
! 45 if s in ("voice", "phone"):
! 46 return "urgent"
! 47 if s in ("chat", "context-drop"):
! 48 return "normal"
! 49 if s in ("discord", "telegram", "slack"):
50 # Owner-tier traffic stays normal; team/guest is demoted so a channel ping
51 # from a non-owner never preempts owner work.
! 52 return "normal" if (access_tier or "owner").lower() == "owner" else "low"
! 53 if s in ("health-check", "sync-memory", "sync-workspace", "cron"):
! 54 return "low"
! 55 return _DEFAULT
56
57
58 def parse_priority_from_text(content: str) -> str:
59 """Read the `priority:` header from a task-file body. Returns theLines 84-93 84 def parse_priority_from_file(path: Path) -> str:
85 """Read priority from a task file. Missing file -> "normal" (fail-open)."""
86 try:
87 return parse_priority_from_text(path.read_text(errors="replace"))
! 88 except (OSError, UnicodeDecodeError):
! 89 return _DEFAULT
90
91
92 def sort_tasks_by_priority(paths: Iterable[Path]) -> List[Path]:
93 """Sort an iterable of task file paths so the highest-priority comes first.Lines 95-104 95 enriched: List[Tuple[int, float, Path]] = []
96 for p in paths:
97 try:
98 mtime = p.stat().st_mtime
! 99 except OSError:
! 100 mtime = 0.0
101 prio = parse_priority_from_file(p)
102 enriched.append((_ORDER.get(prio, _ORDER[_DEFAULT]), mtime, p))
103 enriched.sort(key=lambda t: (t[0], t[1]))
104 return [p for _, _, p in enriched]packages/ag2-sparrow/tests/adapter_differential_driver.pyLines 44-59 44 """
45
46
47 def write_task(work, tid, tier, channel, body, mtime):
! 48 lines = ["id: " + tid, "timestamp: 2026-10-16T00:00:00Z", "source: ag2space"]
! 49 if channel:
! 50 lines.append("channel_id: " + channel)
! 51 lines += ["access_tier: " + tier, "priority: normal", "task: " + body]
! 52 p = work / "tasks" / (tid + ".txt")
! 53 p.parent.mkdir(parents=True, exist_ok=True)
! 54 p.write_text("\n".join(lines) + "\n", encoding="utf-8")
! 55 os.utime(p, (mtime, mtime))
56
57
58 def norm_text(base, text):
59 text = text.replace(os.path.realpath(base), "<base>").replace(str(base), "<base>")Lines 67-75 67 return {k: ("<n>" if k in TIMING_KEYS else norm(base, v)) for k, v in value.items()}
68 if isinstance(value, list):
69 return [norm(base, v) for v in value]
70 if isinstance(value, float) and value > 1e9:
! 71 return "<t>"
72 if isinstance(value, str):
73 return norm_text(base, value)
74 return valueLines 92-104 92
93
94 def drop_lock(work):
95 """Remove the lock dir a holder_lock revision keeps outside the work root (older ones keep none)."""
! 96 try:
! 97 from ag2_sparrow import holder_lock
! 98 except ImportError:
! 99 return
! 100 shutil.rmtree(holder_lock.lock_dir(str(work)), ignore_errors=True)
101
102
103 def summary(base, work, calls, adapter="codex"):
104 results = {}Lines 114-160 114 class Broker:
115 """The routine's fire endpoint: replies from a script, the last entry repeating."""
116
117 def __init__(self, script):
! 118 broker = self
! 119 self.script, self.calls, self.n = script, [], 0
120
! 121 class Fire(http.server.BaseHTTPRequestHandler):
! 122 def do_POST(self):
! 123 body = self.rfile.read(int(self.headers.get("Content-Length") or 0)).decode("utf-8")
! 124 broker.calls.append({"path": self.path, "anthropic-version": self.headers.get("anthropic-version"),
125 "content-type": self.headers.get("Content-Type"), "body": json.loads(body)})
! 126 status, headers, delay = broker.script[min(broker.n, len(broker.script) - 1)]
! 127 broker.n += 1
! 128 time.sleep(delay)
! 129 raw = json.dumps({"claude_code_session_id": "session_%d" % broker.n,
130 "claude_code_session_url": "https://claude.ai/code/session_%d" % broker.n}
131 if status == 200 else {"type": "error", "error": {"type": "x"}}).encode()
! 132 try:
! 133 self.send_response(status)
! 134 for k, v in headers.items():
! 135 self.send_header(k, v)
! 136 self.send_header("Content-Length", str(len(raw)))
! 137 self.end_headers()
! 138 self.wfile.write(raw)
! 139 except OSError:
! 140 pass
141
! 142 def log_message(self, *a):
! 143 pass
! 144 self.server = http.server.ThreadingHTTPServer(("127.0.0.1", 0), Fire)
! 145 threading.Thread(target=self.server.serve_forever, daemon=True).start()
! 146 self.base = "http://127.0.0.1:%d" % self.server.server_address[1]
147
148 def close(self):
! 149 self.server.shutdown()
! 150 self.server.server_close()
151
152
153 def routine(work, broker, once, extra=()):
! 154 from ag2_sparrow.adapters.claude_routine import routine_adapter as ra
! 155 with contextlib.redirect_stderr(io.StringIO()):
! 156 return ra.main(["--work", str(work), "--trigger-id", "trig_01DIFF", "--allow-insecure-api-base",
157 "--api-base", broker.base, "--once", str(once), "--poll", "0.01", *extra],
158 {"CLAUDE_ROUTINE_TOKEN": "t"})
159 Lines 161-274 161 OK = (200, {}, 0)
162
163
164 def routine_scenarios(base):
! 165 out = {}
166
! 167 def run(name, tasks, script, once, extra=(), before=None, again=None):
! 168 work = base / name
! 169 for t in tasks:
! 170 write_task(work, *t)
! 171 if before:
! 172 before(work)
! 173 broker = Broker(script)
! 174 try:
! 175 rc = [routine(work, broker, once, extra)]
! 176 if again:
! 177 again(work)
! 178 rc.append(routine(work, broker, 1, extra))
179 finally:
! 180 broker.close()
! 181 out[name] = {"rc": rc, **summary(base, work, broker.calls, "routine")}
! 182 drop_lock(work)
183
! 184 run("basic", TASKS, [OK], len(TASKS))
! 185 run("restart", TASKS[:1], [OK], 1, again=lambda w: write_task(w, *TASKS[3]))
186
! 187 def preexisting(work):
! 188 (work / "results").mkdir(parents=True, exist_ok=True)
! 189 (work / "results" / (TASKS[0][0] + ".txt")).write_text("answered elsewhere\n")
! 190 run("pre-existing result", [TASKS[0], TASKS[3]], [OK], 1, before=preexisting)
! 191 run("429 then ok", TASKS[:1], [(429, {"Retry-After": "1"}, 0), OK], 1)
! 192 run("timeout", TASKS[:1], [(200, {}, 3)], 1, extra=("--timeout", "1"))
! 193 run("provider error", TASKS[:1], [(401, {}, 0)], 1)
! 194 work = base / "cap"
! 195 for t in (TASKS[0], TASKS[3]):
! 196 write_task(work, *t)
! 197 (work / "state").mkdir(parents=True)
! 198 (work / "state" / "fires.json").write_text(json.dumps([time.time() - 10]))
! 199 broker = Broker([OK])
! 200 try:
201 # The cap holds the next fire for most of an hour: the run is cut off, and nothing may fire.
! 202 child = subprocess.run([sys.executable, __file__, "routine-cap", str(work), broker.base], timeout=60)
203 finally:
! 204 broker.close()
! 205 out["cap 1 per hour, full"] = {"rc": [child.returncode], **summary(base, work, broker.calls, "routine")}
! 206 drop_lock(work)
! 207 return out
208
209
210 def codex(work, calls, extra=()):
! 211 from ag2_sparrow.adapters.codex import codex_connector as cc
! 212 log, tee = work.parent / (work.name + ".rpc"), work.parent / (work.name + ".tee.py")
! 213 tee.write_text(TEE)
! 214 stub = os.path.join(os.path.dirname(cc.__file__), "stub_app_server.py")
! 215 cc.server_argv = lambda server: [sys.executable, str(tee), str(log), stub]
! 216 with contextlib.redirect_stderr(io.StringIO()):
! 217 rc = cc.main(["--server", "stub", "--work", str(work), "--poll", "0.01", *extra])
! 218 if log.exists():
! 219 calls += [{"method": m["method"], "params": m.get("params")}
220 for m in map(json.loads, log.read_text().splitlines()) if m.get("method")]
! 221 log.unlink()
! 222 return rc
223
224
225 def codex_scenarios(base):
! 226 out = {}
227
! 228 def run(name, tasks, once, extra=(), before=None, again=None):
! 229 work, calls = base / name, []
! 230 for t in tasks:
! 231 write_task(work, *t)
! 232 if before:
! 233 before(work)
! 234 rc = [codex(work, calls, ("--once", str(once), *extra))]
! 235 if again:
! 236 again(work)
! 237 rc.append(codex(work, calls, ("--once", "1", *extra)))
! 238 out[name] = {"rc": rc, **summary(base, work, calls, "codex")}
! 239 drop_lock(work)
240
! 241 run("basic", TASKS, len(TASKS))
! 242 run("restart", TASKS[:1], 1, again=lambda w: write_task(w, "task-1700000000005", "owner", "!room:ag2.space",
243 "a follow-up in the same room", 1005))
244
! 245 def preexisting(work):
! 246 (work / "results").mkdir(parents=True, exist_ok=True)
! 247 (work / "results" / (TASKS[0][0] + ".txt")).write_text("answered elsewhere\n")
! 248 run("pre-existing result", [TASKS[0], TASKS[3]], 1, before=preexisting)
! 249 run("turn timeout", [("task-1700000000006", "owner", "!room:ag2.space", "[[stall]] slow", 1006)], 1,
250 extra=("--turn-timeout", "1", "--interrupt-grace", "2"))
! 251 return out
252
253
254 def main():
! 255 if sys.argv[1] == "routine-cap":
! 256 from ag2_sparrow.adapters.claude_routine import routine_adapter as ra
! 257 threading.Timer(3, lambda: os._exit(0)).start()
! 258 with contextlib.redirect_stderr(io.StringIO()):
! 259 ra.main(["--work", sys.argv[2], "--trigger-id", "trig_01DIFF", "--allow-insecure-api-base",
260 "--api-base", sys.argv[3], "--once", "1", "--poll", "0.01", "--max-per-hour", "1"],
261 {"CLAUDE_ROUTINE_TOKEN": "t"})
! 262 return
! 263 adapter = sys.argv[1]
264 # A base of one fixed length on every host: Codex's `chars` counts its own absolute paths.
! 265 base = Path(tempfile.mkdtemp(prefix="sparrow-diff-", dir="/tmp"))
! 266 try:
! 267 out = {"routine": routine_scenarios, "codex": codex_scenarios}[adapter](base)
268 finally:
! 269 shutil.rmtree(base, ignore_errors=True)
! 270 print(json.dumps(out, indent=1, sort_keys=True))
271
272
273 if __name__ == "__main__":
274 main()packages/ag2-sparrow/tests/test_adapter_boundaries.pyLines 20-28 20 def modules(root):
21 """(dotted module name, path) for every .py under root/ag2_sparrow."""
22 for path in sorted((root / PKG).rglob("*.py")):
23 if "__pycache__" in path.parts:
! 24 continue
25 yield ".".join(path.relative_to(root).with_suffix("").parts), path
26
27
28 def resolve(module, node):packages/ag2-sparrow/tests/test_adapter_relay_loop.pyLines 154-167 154 retired = []
155 stop = threading.Event()
156
157 def drain():
! 158 while not stop.is_set() and not retired:
! 159 if live.exists():
! 160 _, generation = result_ready.identity_of(live)
! 161 retired.append(result_disposal.retire_generation(
162 results, live, generation, lambda m: None, archive, ["task-b-1.txt"]))
! 163 time.sleep(0.001)
164 t = threading.Thread(target=drain, daemon=True)
165 t.start()
166 staged = self.stage(loop, "task-b.txt", "first\n")
167 os.link(staged, live) # a crash between the publish link and the stage's unlinkpackages/ag2-sparrow/tests/test_claude_cloud_session_adapter.pyLines 95-103 95 lines.append("channel_id: " + channel)
96 if thread:
97 lines.append("thread_root: " + thread)
98 if collaborator:
! 99 lines.append("collaborator: true")
100 if kind:
101 lines.append("channel_kind: " + kind)
102 lines += ["access_tier: " + tier, "priority: " + priority, "task: " + body]
103 p = Path(work) / "tasks" / (tid + ".txt")packages/ag2-sparrow/tests/test_cloud_session_contract.pyLines 113-129 113 archive.mkdir(parents=True)
114 stop, retired = threading.Event(), []
115
116 def drain():
! 117 while not stop.is_set():
! 118 if live.exists():
! 119 try:
! 120 _, generation = result_ready.identity_of(live)
! 121 except FileNotFoundError:
! 122 continue
! 123 retired.append(result_disposal.retire_generation(
124 results, live, generation, lambda m: None, archive, ["task-q-%d.txt" % len(retired)]))
! 125 time.sleep(0.001)
126 t = threading.Thread(target=drain, daemon=True)
127 t.start()
128 real_save = lc.ClaimStore.saveLines 267-275 267 while time.monotonic() < deadline:
268 if predicate():
269 return
270 time.sleep(0.1)
! 271 raise AssertionError("timed out after %.0fs waiting for %s" % (timeout, what))
272
273
274 @unittest.skipUnless(CONTRACT, "the task-lifecycle contract is off (TASK_LIFECYCLE_CONTRACT=0 turns it off)")
275 @unittest.skipUnless(BRIDGE_FIXTURE.exists(), "the bridge's own broker fixture lives in the repository's src/")Lines 314-324 314 if bridge.poll() is None:
315 bridge.send_signal(signal.SIGTERM)
316 try:
317 bridge.wait(timeout=10)
! 318 except subprocess.TimeoutExpired:
! 319 bridge.kill()
! 320 bridge.wait(timeout=10)
321 self.addCleanup(stop)
322 task = cloud / "tasks" / (fixture.TASK["id"] + ".txt")
323 bounded_wait(task.exists, "the packaged bridge to write the task")
324 with contextlib.redirect_stderr(io.StringIO()):packages/ag2-sparrow/tests/test_cloud_session_interlock.pyLines 36-45 36
37 try:
38 sys.path.insert(0, str(REPO_TESTS))
39 from holder_lock_contract import FirstActContract # noqa: E402
! 40 except ImportError: # the package outside its repository has no contract mixin
! 41 FirstActContract = None
42
43 # On now that the owner module has landed; TASK_LIFECYCLE_CONTRACT=0 turns the contract off.
44 CONTRACT = os.environ.get("TASK_LIFECYCLE_CONTRACT", "1") == "1"
45 GATE = unittest.skipUnless(CONTRACT, "the task-lifecycle contract is off (TASK_LIFECYCLE_CONTRACT=0)")Lines 170-178 170
171
172 def read_line(stream, timeout=WAIT):
173 if not select.select([stream], [], [], timeout)[0]:
! 174 raise AssertionError("no line within %.0fs" % timeout)
175 return stream.readline()
176
177
178 def wait_for(predicate, what, timeout=WAIT):Lines 180-188 180 while time.monotonic() < deadline:
181 if predicate():
182 return
183 time.sleep(0.05)
! 184 raise AssertionError("timed out after %.0fs waiting for %s" % (timeout, what))
185
186
187 def spied(log):
188 """The SPY log as (words, monotonic time) pairs."""Lines 213-223 213 if child.poll() is None:
214 child.send_signal(signal.SIGTERM)
215 try:
216 child.wait(timeout=WAIT)
! 217 except subprocess.TimeoutExpired:
! 218 child.kill()
! 219 child.wait(timeout=WAIT)
220
221 def spawn(self, code):
222 child = subprocess.Popen([sys.executable, "-c", code], stdout=subprocess.DEVNULL,
223 stderr=subprocess.PIPE, text=True)Lines 234-244 234 legacy = self.work / "state" / "connector.pid"
235 wait_for(lambda: holder(self.work) or child.poll() is not None or legacy.exists(),
236 "the %s adapter to hold the dir" % kind)
237 if child.poll() is not None:
! 238 self.fail("the %s adapter exited early: %s" % (kind, child.stderr.read()))
239 if not holder(self.work):
! 240 self.fail("the %s adapter holds the dir by a pidfile, not by the shared lock" % kind)
241 return child, holder(self.work)
242
243 def run_main(self, fn, argv, environ=None):
244 err = io.StringIO()Lines 244-253 244 err = io.StringIO()
245 with contextlib.redirect_stderr(err):
246 try:
247 rc = fn(argv, environ) if environ is not None else fn(argv)
! 248 except SystemExit as e:
! 249 rc = e.code
250 return rc, err.getvalue()
251
252 def cloud_main(self):
253 return self.run_main(sa.main, ["--work", str(self.work), "--session-id", "session_X", "--claude",Lines 413-423 413 with contextlib.suppress(OSError):
414 child.stdin.close()
415 try:
416 child.wait(timeout=WAIT)
! 417 except subprocess.TimeoutExpired:
! 418 child.kill()
! 419 child.wait(timeout=WAIT)
420
421 def racer(self, pause):
422 child = subprocess.Popen([sys.executable, "-c", RACE % (str(PKG_ROOT), SET_HOME, str(self.work), pause,
423 LOCK_HOME)],Lines 444-452 444
445 def first_act_case(adapter, name):
446 """The parent's FirstActContract against one adapter's real CLI (through first_act_runner.py)."""
447 if FirstActContract is None:
! 448 return unittest.skip("the repository's holder_lock contract mixin is not here")(type(
449 "Skipped", (unittest.TestCase,), {}))
450
451 class Case(FirstActContract, unittest.TestCase):
452 module, module_kind = holder_lock, "vendored"packages/ag2-sparrow/tests/test_task_lifecycle.pyLines 54-62 54
55
56 def src_copy():
57 if not (REPO_SRC / "task_lifecycle.py").is_file():
! 58 return None
59 spec = importlib.util.spec_from_file_location("task_lifecycle_src", REPO_SRC / "task_lifecycle.py")
60 mod = importlib.util.module_from_spec(spec)
61 spec.loader.exec_module(mod)
62 return modLines 76-88 76
77 def fd_path(fd):
78 try:
79 if sys.platform == "darwin":
! 80 import fcntl
! 81 return fcntl.fcntl(fd, fcntl.F_GETPATH, bytes(1024)).split(b"\0", 1)[0].decode()
82 return os.readlink("/proc/self/fd/%d" % fd)
! 83 except OSError:
! 84 return ""
85
86
87 def fd_name(fd):
88 """The name of the file an fd refers to; a dot-temp shows as <temp>."""Lines 96-108 96 st = os.fstat(fd)
97 path = fd_path(fd)
98 try:
99 here = os.stat(path)
! 100 except OSError:
! 101 here = None
102 if here is not None and (here.st_dev, here.st_ino) == (st.st_dev, st.st_ino):
103 return os.path.basename(path)
! 104 return "?%d:%d" % (st.st_dev, st.st_ino)
105
106
107 def durable_events(lc, events):
108 """Patches recording each fsync (a directory by descriptor identity), rename and no-replace rename."""Lines 158-166 158 """Make `path` unreadable; skipped where permissions are not enforced (root)."""
159 os.chmod(path, 0)
160 self.addCleanup(os.chmod, path, 0o700)
161 if os.access(path, os.R_OK):
! 162 self.skipTest("permissions are not enforced for this user")
163
164 def calls(self):
165 p = Path(self.tmp.name + "-bin") / "claude.calls"
166 return len(p.read_text().splitlines()) if p.exists() else 0Lines 280-288 280 def read_line(child, timeout=10.0):
281 """One line from a child's stdout, or AssertionError after `timeout`: a red run never hangs."""
282 ready, _, _ = select.select([child.stdout], [], [], timeout)
283 if not ready:
! 284 raise AssertionError("no line from the child within %.0fs" % timeout)
285 return child.stdout.readline()
286
287
288 class LockTest(Base):Lines 303-311 303 child.kill()
304 child.wait(10)
305 deadline = time.monotonic() + 10
306 while time.monotonic() < deadline and task_io.pid_alive(pid):
! 307 time.sleep(0.02)
308 rc, err = self.main()
309 self.assertEqual(rc, 0, err)
310 self.assertIsInstance(holder_lock.read_holder(str(self.work)), holder_lock.Missing, "released on exit")Lines 427-435 427 writer.send_signal(signal.SIGTERM)
428 writer.wait(10)
429 deadline = time.monotonic() + 10
430 while time.monotonic() < deadline and task_io.pid_alive(int(pid)):
! 431 time.sleep(0.02)
432 pidfile = state / "connector.pid"
433 self.assertEqual(pidfile.read_text(), "%s\n" % pid, "SIGTERMed, its finally never ran")
434 time.sleep(1.1)
435 self.task()Lines 738-746 738 def test_the_modules_read_no_legacy_file(self):
739 for path in (REPO_SRC / "task_lifecycle.py", PKG_ROOT / "ag2_sparrow" / "task_lifecycle.py",
740 ADAPTERS / "claude_cloud_session" / "session_adapter.py"):
741 if not path.exists():
! 742 continue
743 strings = [n.value for n in ast.walk(ast.parse(path.read_text()))
744 if isinstance(n, ast.Constant) and isinstance(n.value, str)]
745 for word in ("sends.json", "fires.json", "sending", ".staged"):
746 self.assertFalse([v for v in strings if word in v], (path.name, word))Lines 1174-1186 1174 busy = []
1175 real = life.read_final
1176
1177 def other_thread_tries():
! 1178 try:
! 1179 with result_disposal.locked(self.results):
! 1180 busy.append(False)
! 1181 except result_disposal.DisposalBusy:
! 1182 busy.append(True)
1183
1184 def check(final):
1185 t = threading.Thread(target=other_thread_tries)
1186 t.start()Lines 1456-1464 1456 def half_then_crash(fd, data):
1457 if data.startswith(b"task-a\n"):
1458 real_write(fd, data[:3])
1459 raise Crash()
! 1460 return real_write(fd, data)
1461 with mock.patch.object(self.lc.os, "write", half_then_crash), self.assertRaises(Crash):
1462 life.publish_and_settle("task-a")
1463 self.assertEqual((self.work / "state" / "settled-ids").read_text(), "tas")
1464 self.life().recover()Lines 1830-1838 1830 hits += [ast.unparse(n) for n in ast.walk(tree) if isinstance(n, (ast.Import, ast.ImportFrom))
1831 and "holder_lock" in ast.unparse(n)]
1832 hits += [n.attr for n in ast.walk(tree) if isinstance(n, ast.Attribute) and n.attr == "holder_lock"]
1833 if hits:
! 1834 found[str(path.relative_to(PKG_ROOT.parents[1]))] = hits
1835 self.assertEqual(found, {})
1836
1837
1838 # A pre-holder_lock revision's own pidfile, under the work root; the shared lock keeps nothing there.Lines 1893-1901 1893 elif isinstance(node, ast.ImportFrom):
1894 found += [(node.module or "") + ":" + a.name for a in node.names
1895 if "task_lifecycle" in (node.module or "") + a.name]
1896 elif isinstance(node, ast.Name) and "task_lifecycle" in node.id:
! 1897 found.append(node.id)
1898 elif isinstance(node, ast.Attribute) and "task_lifecycle" in node.attr:
1899 found.append(node.attr)
1900 elif isinstance(node, ast.Constant) and isinstance(node.value, str) and "task_lifecycle" in node.value:
1901 found.append(node.value[:60])Lines 1937-1945 1937
1938 def test_the_owner_imports_no_adapter(self):
1939 for path in (REPO_SRC / "task_lifecycle.py", PKG_ROOT / "ag2_sparrow" / "task_lifecycle.py"):
1940 if not path.exists():
! 1941 continue
1942 tree = ast.parse(path.read_text())
1943 imported = [ast.unparse(n) for n in ast.walk(tree) if isinstance(n, (ast.Import, ast.ImportFrom))]
1944 self.assertFalse([i for i in imported if "adapters" in i], imported)src/task_lifecycle.pyLines 153-163 153 """The first path under `root` (relative), descending into non-empty directories."""
154 for name in sorted(os.listdir(os.path.join(root, rel) if rel else root)):
155 path = os.path.join(rel, name) if rel else name
156 if os.path.isdir(os.path.join(root, path)) and not os.path.islink(os.path.join(root, path)):
! 157 return _first_entry(root, path) or path
158 return path
! 159 return None
160
161
162 def _init_step(name):
163 """A test seam between the first-init steps; production does nothing here."""Lines 176-198 176 """No marker yet: the work dir may hold the bridge's tasks/ and results/ and an interrupted
177 init's own state/ (an empty ledger, an empty claims/, one marker temp)."""
178 format_temps = 0
179 for name in sorted(os.listdir(work)):
! 180 if name in ("tasks", "results") and _is_type(os.path.join(work, name), "dir"):
! 181 continue
! 182 if name != "state" or not _is_type(os.path.join(work, name), "dir"):
! 183 path = os.path.join(work, name)
! 184 return (_first_entry(work, name) if _is_type(path, "dir") else None) or name
! 185 for inner in sorted(os.listdir(os.path.join(work, "state"))):
! 186 path = os.path.join(work, "state", inner)
! 187 if inner == "settled-ids" and _is_type(path, "file") and not os.lstat(path).st_size:
! 188 continue
! 189 if inner == "claims" and _is_type(path, "dir") and not os.listdir(path):
! 190 continue
! 191 if FORMAT_TEMP.fullmatch(inner) and _is_type(path, "file") and format_temps == 0:
! 192 format_temps += 1
! 193 continue
! 194 return (_first_entry(work, os.path.join("state", inner)) if os.path.isdir(path) else None) \
195 or os.path.join("state", inner)
196 return None
197 Lines 212-220 212 fsync_dir(state)
213 _init_step("claims")
214 for name in os.listdir(state):
215 if FORMAT_TEMP.fullmatch(name):
! 216 os.unlink(os.path.join(state, name))
217 tmp = os.path.join(state, ".format.%d.%s.tmp" % (os.getpid(), uuid.uuid4().hex[:6]))
218 fd = os.open(tmp, os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, "O_CLOEXEC", 0), 0o644)
219 try:
220 os.write(fd, (FORMAT + "\n").encode("utf-8"))Lines 237-245 237 try:
238 if not os.path.lexists(marker):
239 entry = _state_entry_refusal(work)
240 if entry:
! 241 return ("refusing to start: %s is not a fresh work dir for this adapter (it holds %s); "
242 "this adapter serves only a new, dedicated work dir" % (work, entry))
243 _first_init(work)
244 return None
245 with open(marker, encoding="utf-8") as f:Lines 244-271 244 return None
245 with open(marker, encoding="utf-8") as f:
246 found = f.read().strip()
247 if found != FORMAT:
! 248 return "refusing to start: %s says %r, this binary serves %r" % (marker, found, FORMAT)
249 if not _is_type(state, "dir"):
250 return "refusing to start: %s is not a directory" % state
! 251 for name, kind in (("settled-ids", "file"), ("claims", "dir")):
! 252 path = os.path.join(state, name)
! 253 if not os.path.lexists(path):
! 254 return "refusing to start: %s is marked but %s is missing" % (state, name)
! 255 if not _is_type(path, kind):
! 256 return "refusing to start: state/%s in %s is not a plain %s" % (name, state, kind)
! 257 for name in sorted(os.listdir(state)):
! 258 if name in STATE_ALLOWED:
! 259 continue
! 260 if name.startswith(".") and name.endswith(".tmp") and any(name[1:].startswith(a + ".")
261 for a in STATE_ALLOWED - {"format"}):
! 262 os.unlink(os.path.join(state, name))
! 263 continue
! 264 return "refusing to start: state/%s is not this adapter's state (in %s)" % (name, work)
! 265 except OSError as e:
! 266 return "refusing to start: cannot check %s: %s" % (work, e)
! 267 return None
268
269
270 def probe_moves(directory):
271 """Refusal line unless rename, hard link and the no-replace move publication uses all work in `directory`."""Lines 542-551 542 d.mkdir(parents=True, exist_ok=True)
543 if new:
544 fsync_dir(self.root)
545 write_durable(d / "claim.json", json.dumps(rec, sort_keys=True).encode("utf-8"))
! 546 except OSError as e:
! 547 raise _state_io("write", d / "claim.json", e)
548
549 def stage(self, task_id, body):
550 try:
551 write_durable(self.dir(task_id) / "body", body)Lines 579-587 579 def ids(self):
580 try:
581 return sorted(n for n in os.listdir(self.root) if not n.startswith("."))
582 except FileNotFoundError:
! 583 return []
584 except OSError as e:
585 raise _state_io("list", self.root, e)
586
|
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head 42dcabc1c4b8781baadbe4cfce6a321c4d8bae8d. The CI/CLA checks are green, the focused suites pass, and the exact-head live witness is useful. I am requesting changes for the following merge blockers.
-
P1 — a contradictory queue receipt is retried, breaking the advertised at-most-once invariant.
session_adapter.py:65-71only accepts{"ok": true}when the exit code is zero, then lets a matching DNS string classify the same invocation as provably unsent. I reproduced this with stdout containing anokreceipt, exit 1, and stderr containinggetaddrinfo ENOTFOUND: the fake CLI was invoked twice for one task. A positive receipt plus a later error is contradictory/ambiguous, not proof of non-delivery; it must never be retried. Please add this exact conflict case to the retry tests. -
P1 — every task body is exposed in process argv.
session_adapter.py:56-60passes the complete prompt as an argument, and the test attest_claude_cloud_session_adapter.py:210-228pins that behavior. With a long-running stand-in CLI, a synthetic secret marker was visible inps -axo command=(child_exposes_secret=True). These tasks can contain private room or connected-account data. Pass the prompt through stdin and keep argv metadata-only. -
P1 — the persistent session cannot enforce the repository's audience/data-origin policy.
claude_routine/routine_adapter.py:45-63sends room/thread/tier/collaborator/task ID but omits trustedchannel_kind;session-prompt.md:21-28then requires every owner-tier answer to be posted into the source room and forbids DM rerouting. That conflicts withAGENTS.md:243-260, which requires owner-only work and connected-account data to go to the owner's DM even when requested from a shared room. Persistence makes cross-room remembered context an additional exposure. Either restrict a session to an explicitly single owner-DM audience, or carry trusted audience metadata and provide a policy-compliant DM routing path; add a shared-room owner-task test. -
P1 — the child silently drops the configured Claude credential boundary.
session_adapter.py:27,34-36omitsCLAUDE_CONFIG_DIR; the direct check returns no value even when the caller supplies one.src/claude_config_dir.sh:1-3,31-36explicitly refuses this fallback because an unset value selects a different user-level credential store. Preserve the caller's validated config directory and test it, while continuing to exclude unrelated secrets. -
P2 — proven-unsent attempts consume the documented queued-turn limit.
session_adapter.py:139-146records every invocation before inspectingout.kind. With--max-per-hour 1, one DNS failure followed by a successful retry produced two calls but advanced the fake clock by 3,600 seconds, despite the configured retry backoff being 2 seconds and zero turns having been queued before the retry. Record only accepted/ambiguous turns, or rename and document this as an attempt limit; the current implementation contradictsREADME.md:75-76and the flag help. -
P2 — the adapter ignores the task protocol's priority ordering. The inherited
pending()atclaude_routine/routine_adapter.py:202-215sorts only by mtime. An olderlowtask was selected before a newerurgenttask, opposite the production contract insrc/task_priority.py:92-104andlocal_task_protocol.py:112-114. This adapter consumes the sharedtask-*queue, so it must use the shared/vendored priority policy and test urgent-before-low with FIFO inside a tier. -
Architecture blocker — the new adapter imports and subclasses another adapter's private implementation.
session_adapter.py:20-21,82depends directly onclaude_routine.routine_adapter.docs/architecture-boundaries.md:97-106explicitly disallows adapter-to-adapter private implementation dependencies and puts shared provider-neutral behavior in core or an adapter library. Extract the task/result loop, payload/result helpers, and common rate/state policy to a non-private shared owner, then pin both adapters' delegation. This is also the right place to repair priority ordering rather than adding another policy copy.
Verification performed at this head: review preflight and hardcoded-path checks passed; py_compile passed; the new adapter suite passed 25/25; the routine regression suite passed 36/36; wheel/package checks and all remote CI are green. The 0.5-second timeout test also failed once only under concurrent local load and passed alone; please harden that timing while updating the failure-path tests.
…ience, shared relay loop - A stdout receipt with "ok": true means queued whatever the exit code or stderr says; only a receipt-less, provably unsent call is retried. - The turn text goes on stdin; argv is metadata only. - The routing line carries the attested channel_kind and an optional owner_dm (--owner-dm); the standing instructions route owner-only answers from a shared room to the owner's DM, or decline in the room. - CLAUDE_CONFIG_DIR passes through to the CLI when the caller sets it. - The hourly cap counts queued and ambiguous turns only. - The shared task/result loop, payload layout, result lines and rate limiter move to adapters/relay_loop.py; both Claude adapters delegate to it, and neither imports the other. Pending tasks are taken in task-priority order through a vendored copy of src/task_priority.py, which now imports its protocol module relatively inside the package. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Round 2 at ee26fd0 addresses all seven items; the PR body is updated. Witness status at this head, stated plainly: Adapter side ran. With the new standing prompt installed in the prepared session (it replied Session side is blocked, and that is a finding. Before those two turns, a probe turn I sent asked the session to call Consequence for the adapter: a cloud session that raises a permission prompt stalls the whole queue silently, and this adapter cannot detect it (the CLI only reports "queued"). I'm adding to the standing prompt a rule to use only the three tools that ran without prompting, and to the README the limit and the owner-side fix (clear the prompt, or set the session's permission mode in the web UI), with an issue for a liveness check. That lands as one more commit; the final witness follows at that head. |
…rmission prompt A permission prompt inside the cloud session holds every turn queued after it, and nothing on the CLI side can answer it. The standing instructions now allow only ag2_whoami, room_actions_describe and room_action_execute and forbid every other tool; the README names the stall, that the adapter cannot see it, and the owner-side fixes on the session page. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
The follow-up is in: 60a1f25 restricts the standing prompt to the three tools that ran without a permission prompt (whoami, room_actions_describe, room_action_execute), forbids the rest, documents the limit in the README, and pins the allowlist with a test. This is the head to review. The final live witness follows once the owner clears the prompt in the prepared session. |
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed current head 60a1f25a1a1a7a5121109a146069ffb50e6e33b0. Round 2 does resolve the original argv leak, CLAUDE_CONFIG_DIR propagation, the ordinary proven-unsent rate-accounting case, task priority, and the direct cloud-session → routine-adapter dependency. CI and CLA are green. Changes are still requested for the following blockers.
-
P1 — a positive queue receipt can still be retried.
_receipt()atsession_adapter.py:55-65returns the first parseable object while scanning stdout from the end. With stdout{"ok":true,"url":"…"}\n{"error":"diagnostic"}\n, exit 1, and stderrgetaddrinfo ENOTFOUND,queue_turn()returnsretry; the end-to-end fake then runs twice and the result points at the second turn. The positive-receipt-only control returnsok. This contradicts the PR's “any stdout receipt” contract and preserves the duplicate-post risk. Scan all decoded JSON records for anyok is Truebefore considering retry classification, and add the positive-receipt-followed-by-diagnostic case. -
P2 — a crash loses the persisted hourly-cap accounting. At
session_adapter.py:146-153, the marker is durable before the CLI call, butlimiter.record()happens only after the call returns. I injected a crash after the fake CLI queued turn 1 and beforerecord():sends.jsondid not exist and the sending marker did. On restart the marker correctly made turn 1 ambiguous, but with--max-per-hour 1turn 2 queued immediately (calls=2, fake elapsed time0.0).README.md:89-91says queued or possibly queued turns count and survive restart. Persist this transition idempotently so repeated crashes cannot omit or double-count it, and add this crash-window test. -
P1 — the required privacy route contradicts the per-task send instruction.
session_adapter.py:25-26tells the session to “post exactly once,” andsession-prompt.md:9-12says “one message, one reply,” but the private-answer branch atsession-prompt.md:25-35requires two posts: the answer inowner_dmplus “I sent it to you in our DM.” in the source room. This ambiguity sits on the privacy boundary fixed in round 2. Make the reminder and standing prompt agree on the exact allowed message count, pin that invariant, and exercise the two-destination path in the final live witness. -
P2 — the permission-stall mitigation removes the capability the PR advertises.
session-prompt.md:37-40now forbids every tool and command except three AG2 Space calls. That prevents file/code/web/connected-account work, whileREADME.md:3-5and the PR summary still present a persistent agent with the full Claude Code toolset, and the audience section describes connected-account answers. Either establish a permission configuration that retains the intended tools without stalling, or explicitly narrow the adapter's supported task scope and claims. A source-text allowlist test proves only the restriction, not that representative tasks can complete. -
Architecture blocker — the shared owner still depends on a concrete adapter's private implementation. The cloud → routine import is gone, but
relay_loop.py:17imports task/result helpers fromadapters/codex/codex_connector.py; both Claude adapters also import that concrete adapter atsession_adapter.py:20androutine_adapter.py:23. This still violatesdocs/architecture-boundaries.md:97-106. The new structural test only forbids cloud ↔ routine imports, so it cannot catch this dependency. Move the generic task parsing/ledger/atomic-write/lock/note contracts to a non-private shared owner and have all three adapters delegate to it.
Verification at this SHA: 37 cloud-session tests, 8 shared-loop tests, and 36 routine tests passed by name; the wheel/sdist smoke, vendoring check, py_compile, diff scan, and all remote CI/CLA checks passed. The required exact-head live session-side witness is still explicitly owed; approval also remains blocked on that real round trip under REVIEW.md §15.
…ap, one answer, no adapter imports another - Any JSON record with "ok": true anywhere in the CLI's stdout or stderr means queued; a later diagnostic line, exit code or DNS error never turns it into a retry. - The hourly cap entry is written before the CLI call, keyed by task id (idempotent), and removed only when the call proves it was never sent, so a crash after a send keeps the count and a restart cannot double it. - The standing prompt pins the message count: one answer, to the room or to owner_dm, plus only the one DM pointer line in the room. - The session keeps Claude Code's own tools; only the AG2 Space connector is limited to the three tools that did not prompt. - Task-file I/O (attested headers, parse, atomic write, done ledger, pidfile, non-owner note) moves from the Codex adapter to package-level ag2_sparrow/task_io.py, and the relay loop moves to ag2_sparrow/relay_loop.py. Codex, Hatch and both Claude adapters import from there; a structural test fails on any adapter importing another. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Round 3 at ed6fb35, for the round-2 review (#5660 (review)):
Stated openly: The exact-head witness, including the two-destination DM path, follows once the owner clears the permission prompt in the prepared session; the updated standing prompt is already queued there. |
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head ed6fb353677412afc4dba3c62d7ece3b5bd4ee22. The positive-receipt scan is fixed, the one-answer/one-pointer and connector-only tool wording are now internally consistent, and the Python adapter-to-adapter imports are gone. All current CI and CLA checks are green. Changes are still requested for these blockers.
-
P2 — crash recovery waits behind, then renews, its own hourly-cap entry.
RelayLoop.next_ready()applieslimiter.wait_seconds()atrelay_loop.py:190-200before the cloud adapter can inspect the durable sending marker atsession_adapter.py:138-143. With a fixed clock and--max-per-hour 1, I queued one fake turn and crashed after the CLI returned. Restart had the task and marker, butnext_ready()returned(None, 3600.0)and no result existed until an hour elapsed. At that point the expired entry was pruned and the marker branch'srecord(task_id)added it again at the recovery time, so the next real task was held for another 3600 seconds. A crash in the adjacent window betweenrecord()andmark_sending()likewise delays a definitely-unsent task for an hour. The new crash helper restarts under the default cap of 60 (test_claude_cloud_session_adapter.py:511-523), so it does not exercise the full-cap recovery path. Marker disposition must bypass send pacing/rate gates without refreshing an old reservation; please pin both full-cap restart windows. -
P2 — the pre-send reservation no longer anchors the documented minimum gap to the queued outcome. In a fake call that advanced the adapter clock by 10 seconds, the persisted entry stayed at
1800000000.0, the positive queue receipt arrived at1800000010.0, and with--min-gap-seconds 2the next task was immediately ready.RateLimiter.record(key)is idempotent, so the success/ambiguous path never confirms the reservation at the outcome time. The README promises spacing between queued turns, not merely between subprocess starts; retain the crash-safe reservation, then finalize its timestamp once the outcome is known. -
P2 architecture — the shared owner still depends on a concrete adapter. Package-level
task_io.py:14,37-50names, opens, and parsesadapters/hatch/ag2-sparrow-hatch.sh. That is still shared/core → adapter dependency by filesystem lookup, even though it is not a Python import, and it conflicts withdocs/architecture-boundaries.md's dependency direction. The new AST scan cannot see it because it only examines imports underadapters/. Make the non-owner policy canonical in the shared layer/resource and have Hatch consume/render that owner, rather than having shared code locate Hatch.
Approval is independently blocked on the exact-head live witness: the latest author note still says the ed6fb353 two-destination DM path “follows once the owner clears the permission prompt.” The source assertions pin text, but under REVIEW.md §15 this delivery-loop change needs a real post-restart round trip at this SHA, including the DM answer plus the single source-room pointer.
Verification at this SHA: 41 cloud-session, 10 shared-loop, 36 routine, and 43 Codex tests passed locally; independent QA also passed the 109 Hatch tests. Wheel/sdist out-of-tree smoke, vendoring sync, py_compile, diff --check, and the review checks passed. Remote CI and CLA are green. The branch is opt-in, which limits existing-user blast radius, but its recovery path is not merge-ready until the two code blockers above are fixed and the exact-head witness is attached.
… counts from the outcome, one non-owner note owner - A task that already holds a cap reservation or a pre-send marker is settled first on restart and skips the hourly cap and the minimum gap; settling never adds an entry or moves an old one. - Once a turn's outcome is known (queued or ambiguous), its reservation moves to the outcome time, so --min-gap-seconds counts from the queue, not from the call's start. - The non-owner policy text is canonical in ag2_sparrow.task_io (NON_OWNER_NOTE, printed by `python -m ag2_sparrow.task_io --non-owner-note`). The Hatch hook reads it from there at run time, the Claude Code mod pins it against the shared owner, and task_io no longer opens any adapter file. Structural tests fail on a shared module naming an adapters/ path and on any adapter naming another adapter's path. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Round 4 at ea61bcf, for the round-3 review (#5660 (review)):
Suites at this head: cloud-session 44 OK; codex connector 43 OK; claude-code adapter 10 OK; relay_loop 13 OK; Hatch 111 with 1 failure ( The exact-head witness with the DM-plus-pointer path follows once the owner clears the permission prompt in the prepared session. |
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head ea61bcf8676a8daa277a79ac2cce8adfc4c7da4e. The three advertised round-3 cases are fixed in their direct paths: both full-cap recovery tests settle immediately without double-counting, ordinary queued/ambiguous outcomes move the reservation timestamp, and task_io.py now owns the non-owner note while Hatch consumes it. Changes are still requested for the following current-head blockers.
-
P2 — a crash can turn a proven-unsent attempt into a suppressed retry. In
session_adapter.py:158-160, the retry path removes the limiter reservation before clearing the durable sending marker. I made the fake CLI returngetaddrinfo ENOTFOUND, then injected a crash in_clear(). The exact-head output was:after_crash calls=1 marker=True sends=[] result=False after_restart calls=1 result='This message may or may not have reached the cloud agent (interrupted); it was not retried.'The service was provably never reached, but restart sees only the surviving marker at
session_adapter.py:141-145and suppresses the safe retry promised by the failure table. Clear/transition the marker before releasing the keyed reservation (or persist an equivalent atomic state transition), and add a crash-between-cleanup-steps regression test. -
P2 — the crash-before-
touch()window still under-enforces both pacing guarantees. A reservation is written before the call, but if the CLI completes and the process dies beforesession_adapter.py:164, restart settles the marker without finalizing that entry. With a fake call that took 10 seconds, I measured:cap=60 gap=2 first_done=1800000010 next_queued=1800000010 delta=0 cap=1 gap=0 first_done=1800000010 next_queued=1800003600 delta=3590Thus the next turn can queue with zero seconds of the documented two-second gap, or two possibly queued turns can fall inside the documented sliding hour. The new recovery tests stop after settling the marked task, so they miss the following turn. Finalize a possibly-sent interrupted turn conservatively before admitting later work, and pin the next-turn behavior for both gates.
-
P2 architecture guard — the new shared-to-adapter scan does not catch imports.
test_adapter_relay_loop.py:204-211checks string constants only and scans only top-levelag2_sparrow/*.py. The forbidden dependencyfrom .adapters.hatch import clihas no string constants and therefore reports no violation; a shared subpackage is not scanned at all. The implementation is currently pointed in the correct direction, but the claimed structural regression guard does not enforce that boundary. ScanImport/ImportFromnodes recursively across non-adapter package modules as well as path strings, with a positive control for the forbidden import.
Two merge gates also remain open:
- The exact-head post-restart live witness, including the owner-DM answer plus one source-room pointer, is still explicitly owed. Under
REVIEW.md§15, source assertions do not replace that delivery-loop witness. - Required CI is red. The underlying
lints + typecheckfailure is the cumulative review check:task_io.py:13-15forms a three-line comment block over the two-line cap. Thetsc + tests (clean install)aggregate fails only because that dependency failed. CLA is green.
Verification at this SHA: 44 cloud-session, 13 relay-loop, 36 routine, 43 Codex, 10 Claude Code, and 111 Hatch tests pass locally. Independent QA reproduced the direct round-3 fixes and the CI failure; remote diff coverage and the remaining test lanes pass. The checkout stayed clean and the remote head was still exactly ea61bcf8 immediately before this review.
…ze, import-walking boundary guard - The task_io comment block is cut to the two-line cap. - The pre-send marker now carries a phase. A proven-unsent call rewrites it to "unsent" before releasing its reservation, so a crash between the two steps restarts into exactly one retry instead of a suppressed one. - A queued or ambiguous outcome rewrites it to "outcome" after the reservation moves to the outcome time. A restart that finds "sending" (outcome never recorded) moves the one entry to the restart time, so the next turn still waits the full gap and the hour. - The boundary guard moves to test_adapter_boundaries.py. It walks every Import/ImportFrom in every module of the package, subpackages included, resolving relative imports, and keeps the path-string scans. Positive controls plant each forbidden shape in a throwaway tree. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Round 5 at 8d035a4, for the round-4 review (#5660 (review)):
Suites at this head: cloud-session 49 OK (second run, after fixing a new test of mine that read |
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 8d035a4a6703a8df3039389b6148c5483931c7e5. The three round-4 findings are fixed for the committed cases: a retained task recovers from the unsent phase, an interrupted sending phase conservatively protects both later pacing gates, the recursive import guard catches the requested shared-to-adapter shapes, and required CI/CLA are green. Changes are still requested for two adjacent crash/lifecycle blockers.
-
P2 — cancelling a task after the durable
unsentmark strands a proven-unsent cap reservation.session_adapter.py:178-180intentionally persistsunsentbeforeforget(), but recovery is driven only from names returned byRelayLoop.pending()(relay_loop.py:150-158). If the process crashes beforeforget()and the task is then cancelled/removed, no later pass visits its marker. Exact-head fake-CLI repro:marker='unsent' before=[[1800000000.0, 'task-cancelled']] calls_before_restart=1 restart_wait=3590 calls_total=2 after=[[1800003600.0, 'task-next']]The DNS failure proved no turn queued, yet at
--max-per-hour=1unrelated work waited the rest of the hour, and the orphan marker remained. This contradicts the documented “proven-unsent attempts do not count” behavior. The committed test retains the original task, so it cannot exercise this path. Reconcileunsentreservations/markers independently of pending task files and pin cancellation/removal followed by new work. -
P2 — result publication, the done ledger, and marker cleanup are not one recoverable handoff.
RelayLoop.finish()publishes the result beforerecord_done()(relay_loop.py:160-163), and the cloud adapter clears its phase marker only after that returns (session_adapter.py:131-133). A result drain is allowed to claim the file as soon as it appears. I claimed the first result at therecord_done()boundary and injected a crash; restart produced:after_crash claimed='[no-send]\n# claude cloud session: ...turn=1' marker='outcome' live_result=False calls=1 after_restart second='This message may or may not have reached the cloud agent (interrupted); it was not retried.' calls=1Thus one successfully queued turn publishes two contradictory local outcomes. I did not establish a second remote message—the gateway outbox may reject/dedupe the replacement—but the task/result contract is already inconsistent, and
--result-mode echoor another immediate drain can expose it. Make this settlement recoverable across an immediate production-style result claim, and add a concurrent drain/crash regression test against the production writer/consumer boundary.
Approval is also independently blocked by the explicitly owed exact-head live witness under REVIEW.md §15. The earlier witness is from 42dcabc1; 8d035a4a still lacks the real post-restart owner-DM answer plus single source-room pointer.
Verification at this SHA: 49 cloud-session, 8 relay-loop, 8 boundary, 36 routine, 43 Codex, 10 Claude Code, 111 Hatch, wheel/sdist, vendoring sync, diff --check, and cumulative review checks pass locally. Independent QA additionally passed the after-forget/before-clear and after-outcome/before-finish phase probes. Remote required CI and CLA are green. The adapter remains opt-in, limiting existing-user blast radius; when enabled, the first blocker can delay every later turn for up to an hour, while the second can publish a false second disposition.
…, record done before publishing - On start, the cloud session adapter settles markers and reservations of tasks no longer in the inbox: an `unsent` marker and its reservation are dropped, a `sending`/`outcome` marker counts (moved to the start time when its outcome was never recorded) and is recorded done, and a reservation with no task and no marker, never sent, is released. - The shared loop stages a result as a dot file, records the task done, then renames the result into place, so a drain claiming it at once followed by a crash cannot lead to a second, contradicting result. A staged body a crash left behind is published on start. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Round 6 at e5d5058, for the round-5 review (#5660 (review)):
Suites at this head: cloud-session 52 OK, relay_loop 8 OK, codex connector 43 OK, routine 36 OK (one assertion updated for the staged write). The cloud-session suite ran twice (a new test of mine read the fake clock after the idle loop advanced it), and the drain test was re-run once per head after its first version used one claim name for every claim and so could not fail; the body says so. Exact-head witness with the DM-plus-pointer path: still owed, waiting on the owner clearing the permission prompt in the prepared session; the standing prompt is unchanged since ed6fb35 and already queued there. |
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head e5d5058fa3ff76a2f7e9b2a959411deeddd79ade. The two round-5 failures are fixed in the tested single-writer paths: a cancelled task with an unsent marker releases its reservation, and staging the body before done prevents the immediate post-publication claim/crash from creating a second local result. The new tests also fail under the prior implementations. Changes are still requested for two adjacent recovery states.
-
P2 — staged recovery can replace a result that is already published.
RelayLoop.recover()unconditionally callsos.replace(staged, results/task-….txt)(packages/ag2-sparrow/ag2_sparrow/relay_loop.py:234-241). Starting withtask-a.txt = "new-visible"and.task-a.txt.staged = "old-staged", exact-head recovery produced:live_after_recovery= 'old-staged\n' retire_original_generation= replacement-live archive= [] done= ['task-a']A competing result is a supported workspace state: the gateway explicitly handles a concurrent core, and its production disposal is generation-bound (
remote_gateway_bridge.py:4849-4865,6327-6341). If the gateway has already read/sent the visible generation, this replacement is deliberately left live and can be delivered on the next pass, while the first producer's body has been destroyed at the canonical name. The new drain test does not cover this input: it inventstask-q.sending.*names rather than exercising the production result-disposal lifecycle, and it begins only after publication. Resolve the stage with a collision-safe/no-replace lifecycle that preserves an existing published or in-flight generation, and pin both a pre-existing live result and the production disposal race. A check followed byos.replaceremains racy. -
P2 — a reachable legacy marker is settled without charging a possibly queued turn.
session_adapter.py:141-153treats every phase other thanunsentas final, but refreshes the cap only for the literalsending. The original live-tested head42dcabc1wrote a numeric PID marker before invoking the CLI and did not record the send until after the call, so a crash plus upgrade can leave a PID marker with an old keyed entry or no entry. Exact-head startup marked that task done and cleared the marker, but yielded:keyed 50s ago: wait=3550.0 entries=[(1799999950.0, 'task-old')] no keyed entry: wait=0.0 entries=[]Both states may represent a turn queued immediately before the crash; the latter bypasses the documented hourly cap entirely. Treat any non-
unsentleftover as possibly sent and ensure exactly one keyed entry at the conservative restart time, including the old PID format, then cover upgrade state with and without a reservation.
Approval is also independently blocked by the exact-head live-path witness required by REVIEW.md §15. The author says it is still waiting on the owner clearing the permission prompt; the prior real witness was at 42dcabc1, not this head.
Verification at this SHA: 52 cloud-session, 8 shared-loop, 36 routine, 8 boundary, and 43 Codex tests pass; direct injection at each new stage/done/publish crash point recovers one result; wheel/sdist, diff --check, cumulative review checks, all current CI, and CLA pass. The adapter is opt-in, limiting ordinary-user exposure, but concurrent processors can lose or duplicate a result and upgraded crash state can undercount the hourly gate.
…from the package finders, legacy markers count - Publishing a staged result is a hard link, so an existing result at the name is never replaced; on a collision the stage is discarded. - Recovery and the interrupted-task path ask local_task_protocol.find_result and result_disposal.find_claims whether any generation of the result exists (live, mid-disposal, archived); if one does, nothing is written and the task is only recorded done. - Any marker other than "unsent", including an older release's pid marker, is taken as possibly queued: it is settled without a resend and counted with exactly one keyed entry at the restart time (an existing reservation is moved, a missing one is created). Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Round 7 at 3cc5dfe, for the round-6 review (#5660 (review)), and the step back it suggested: Writer contract (now in the PR body). The adapter writes each result once and never replaces another: publishing is a hard link from the staging file to
Suites at this head, each once: cloud-session 55 OK, relay_loop 12 OK, routine 36 OK, codex connector 43 OK; review-checks in CI's form passes. The exact-head witness with the DM-plus-pointer path still waits on the owner clearing the permission prompt in the prepared session; the standing prompt is unchanged since ed6fb35 and already queued there. |
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 3cc5dfe90f7d4bee85d13d40d943603e30e1d267. The two round-6 cases are fixed: a pre-existing live result is not replaced, static live/archive/production-claim recovery discards the stale stage, and legacy PID markers now settle with one keyed cap entry. Changes are still requested for the remaining writer/crash paths below.
-
P2 — normal admission and publication still bypass the any-generation result contract.
pending()checks only the live pathname andfinish()never callsresult_exists(); only the interrupted cloud path reachessettle(). A live task beside an already archived result is a supported transient state while the gateway retires the result before archiving the task, and it is also reachable when a concurrent core produced the result. At this head I got:pending_with_archived_result: ['task-a.txt'] archive: older completed result live: later resultBoth ordinary adapters then use
finish()after their provider call, so the cloud session may already have posted a second room/DM answer before the local collision is noticed. Apply the shared any-generation decision to normal admission/finalization too, and use a shared task claim if concurrent processors are meant to preserve at-most-once provider effects; checking only after the provider call cannot prevent the duplicate post. -
P2 — recovery's existence check and publication are not one transition with disposal.
recover()checksfind_result/find_claims, releases that observation, and later hard-links the stage. In the exact interleaving “check says absent → another producer publishes → productionretire_generationarchives that result → link the stale stage”, I reproduced:other_retired: True archive: other answer live: staged answerThe new “real disposal” test does not exercise this window: it waits for the retirement thread to finish at lines 133–143 and only then invokes recovery. Put the collision decision in the dependency-light shared result-lifecycle owner and serialize the any-generation check plus publication against the production disposal lock; pin the check/publish interleaving, not only a result archived before the check.
-
P2 — the private hard-link implementation leaves supported workspaces permanently unable to recover.
publish()callsos.linkdirectly, although the package declares itself OS-independent and already has the shared private-source no-replace transition inundelivered_quarantine.move_private. Injecting the normalEOPNOTSUPPresult from a filesystem without hard links produced:finish_error: 102 after_finish: done=task-a stage=True live=False recover_error: 102Every restart fails on the same stage before the adapter can serve another task. Reuse the shared no-replace owner (which prefers the platform rename primitive) or document and validate an explicit filesystem prerequisite before recording work done; do not add a second private collision primitive in the adapter loop.
-
P2 — a crash-left atomic marker temp is counted as a separate sent task.
write_atomic()uses.<basename>.<pid>.tmp, but cloud recovery treats every entry instate/sending/as a task id. A SIGKILL after the temp fsync and beforeos.replacetherefore creates a bogus cap entry and done id:entries: [(1000.0, 'task-a'), (1010, '.task-a.4242.tmp')] done: ['.task-a.4242.tmp']At a full cap this delays unrelated work for up to the hour. Filter/recover the writer's temp grammar separately and pin the real crash-left filename; it is not a legacy task marker.
Approval is independently blocked by the required exact-head live-path witness. The author still records it as owed while the prepared cloud session waits on an owner-cleared permission prompt; the only completed real witness is at 42dcabc1, before seven subsequent commits. REVIEW.md §15 requires the post-restart DM-plus-pointer round trip at this head.
Verification at this SHA: relay 12/12, cloud 55/55, routine 36/36, Codex connector 43/43, and adapter-boundary 8/8 tests pass; the new round-6 tests discriminate against e5d5058f; diff --check, cumulative review-checks.sh, source sync, 35 successful CI checks, and SHA-bound CLA all pass. The adapter is opt-in, limiting exposure, but enabled users can receive duplicate posts/results, an unsupported-hard-link workspace cannot recover, and a crash can overcharge the send cap.
Design note before any round 8: one result-lifecycle ownerSeven rounds have each found one more crash or race window in code that stages results, keeps markers and reservations, and recovers after a restart, across One module,
Recovery is one function of that owner: it walks Rate accounting (hourly cap, minimum gap) reads the claims' reservation times; What moves out of the adapters. Relation to core. The claim shape mirrors Ask. Review this design before any more code. If it holds, round 8 is one commit that introduces the owner and makes the adapters delegate, with the round-7 items as its regression tests. Until then #5660 is parked: opt-in, nothing depends on it, and the owner decides in the morning whether to spend that round. |
|
Design review of the proposed shared owner at exact head
With those amendments, yes: one dependency-light |
…, sub-second pidfile order, Codex --once Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head ac2b9427247ca18544027900663c2ff741cef0ee (93cb3d66..ac2b9427 is comment-only). REQUEST_CHANGES — not merge-ready.
I verified that the four previously reported runtime symptoms are fixed at this head: positive short writes complete the journal record; a parked non-owner refusal remains pending and does not satisfy --once; the production pidfile comparison preserves fractional ordering; and all three adapters reject a negative --once. The corrected claim-ID and roomless-parking tests also exercise their stated conditions. Focused suites and the current Actions run are green.
Two blockers remain:
-
A process crash can still leave the first rate-journal directory entry without a durability barrier, and restart never repairs it. In
packages/ag2-sparrow/ag2_sparrow/task_lifecycle.py:126-139,append_durable()recordscreated = not os.path.exists(path), fsyncs the file, closes it, and only then fsyncs the parent whencreatedwas true. If the process dies after the first completerate.jsonlrecord is fsynced but beforefsync_dir(state), the restarted process sees an existing file, loads/deduplicates the reservation, and every later append takescreated == False. No state-directory fsync occurs before the task is settled.Exact-head fault-injection probe (death at that first parent-directory barrier, then a fresh lifecycle instance):
phase_at_crash PREPARED rate_line_complete True settle settled restart_fsynced_state_dir False [] phase_after_work SETTLED charged_in_memory TrueA later host/power loss may therefore remove the never-durable journal directory entry and forget a settled provider charge, undercounting the hourly cap. Re-establish the directory barrier during recovery/startup before provider work or settlement, create the journal durably during initialization, or fsync the parent on every append. Please pin process-death-before-first-parent-fsync → restart/settle → restart with a production-path regression.
-
The stale-pidfile regression does not kill the exact old timestamp-truncation bug. Restoring both old behaviors (
_PROCESS_STARTED_AT = int(time.time())andint(st.st_mtime) < started) leaves the named test—and the full 41-test interlock suite—green:test_the_legacy_pidfile_is_removed_only_when_a_dead_adapter_wrote_it_before_this_start ... ok Ran 1 test ... OK old_bug_mutant_passed Truetest_cloud_session_interlock.py:566-571derives the pidfile timestamp from the mutated_started_at(), so the mutant changes the fixture to the preceding whole second instead of recreating the same-second case. Freeze module/import time independently at a fractional value (for example, start1000.9) and set the file mtime independently on each side (1000.8and1000.901) so the exact former implementation fails.
Independent merge gates also remain: the required exact-head post-restart live bridge witness has not been posted (the only live witness is at 42dcabc1), and the required license/cla context is absent. I did not run the external relay pause/restart because that still requires owner authorization. Actions are otherwise green.
What changes this verdict: close both blockers with discriminating regressions, post the exact-head live witness, obtain green CLA, and satisfy the required current-head maintainer approvals.
…irst record Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Live witness at d2cb273 — partial: the non-owner refusal path, three times; the owner path still pendingSetup (v11 §2.1 topology): a fresh work dir for the adapter with the bridge's What happened: three mentions of the agent were posted in the test room — one from my Stand, two from the owner's personal account. The broker attested all three as team tier for this agent (its owner identity is a different account), so each went down the contract's non-owner path:
What this does and does not show: the post-restart bridge round trip, the lock-first startup, the claim lifecycle through |
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head d2cb2736a5b07770c32f411f7aeff2102331b9d8. REQUEST_CHANGES — not merge-ready.
The rate-journal blocker is closed at this head. I verified the first-create order (file fsync → parent-directory fsync → complete record write → file fsync), the reopen repair, byte-identical core/vendored copies, and the real crash/restart/settle/restart path. The lifecycle suite collected and passed 188 tests (184 at the parent), and independently reverting either the create ordering or reopen repair makes the new tests fail. The production fractional pidfile behavior also works. Current Actions and review scans are green.
Two test blockers remain:
-
The regression still masks the exact old process-start truncation line.
packages/ag2-sparrow/tests/test_cloud_session_interlock.py:566-572setstask_io._PROCESS_STARTED_AT = 1000.9directly. That bypasses the production assignment attask_io.py:258, so restoring only the old_PROCESS_STARTED_AT = int(time.time())line leaves both the named test and the complete interlock suite green:Ran 41 tests in 7.202s OK mutated_process_start_type int old_start_capture_mutant_suite_passed TrueThis is a behaviorally live mutant: with actual start
1000.900stored as1000and pidfile mtime1000.899, production refuses and retains the valid stale pidfile. The separateint(st.st_mtime) < startedmutant is killed, but a combined mutant is not proof that each line is pinned. Freezetime.time()before a fresh module import/reload and let the production assignment capture it; do not overwrite the value produced by that assignment. Then verify each former line independently fails. -
Qingyun's startup-snapshot race remains after the attempted wait.
test_cloud_session_interlock.py:287-291treatsstate/formatplus absence of.format.*as “first init finished,” butsession_adapter.py:305-307still runsprobe_moves(results)andprobe_moves(state)afterward. The snapshot can therefore race the cloud process's own.probe-*creation/removal. Exact-head stress failed 7/20 focused runs, and one full interlock run was 40/41. Observed transitions included:before ['claims', 'format', 'settled-ids'] after ['.probe-92453-7fa85c7e', '.probe-92453-7fa85c7e.moved', 'claims', 'format', 'settled-ids'] before ['.probe-92614-b83b2ce0', 'claims', 'format', 'settled-ids'] after ['claims', 'format', 'settled-ids']This is a test-harness race, not an observed lock failure, but it makes the suite nondeterministic and cannot prove that the refused routine wrote nothing. Synchronize on a real ready signal after both probes (or an end-to-end task), rather than the absence of a transient that may not have started yet.
External gates are also incomplete. The exact-head witness at #5660 (comment) explicitly covers only the non-owner refusal path; design v11 §3.10's owner task → owner DM + one source-room pointer remains pending. The required license/cla context is absent, although all 38 Actions checks completed green/skipped.
Worst-case user impact if the unpinned timestamp bug returns is a valid dead legacy pidfile refusing adapter startup until manual cleanup; the flaky interlock can intermittently fail otherwise-correct changes or obscure attribution. What changes this verdict: kill both timestamp mutants independently, remove the startup-snapshot race, complete the exact-head owner-path witness, restore green CLA, and satisfy the required current-head approvals.
…shot waits for the serve loop Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@cla-assistant check |
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head e259552dfca041dbeee19d57af5dd3ac5574fd70. REQUEST_CHANGES — the code/test blockers are resolved, but this live-path PR is not yet approvable.
I verified both fixes in the tests-only delta:
- The frozen-time alias module exercises the production start assignment without replacing the live
task_ioobject. The canonical module and all adapter exception identities remain unchanged, and the alias is not registered insys.modules. Independently restoring_PROCESS_STARTED_AT = int(time.time())fails at1000 != 1000.9; independently restoringint(st.st_mtime) < startedremoves the1000.901file and fails the retained-file assertion. serving()observes the first relay-loop stop check only after cloud initialization, both move probes, adapter construction, and recovery. The formerly racy routine/cloud cases passed 100/100 each in independent process stress (and another combined 200/200 run); an injected never-ready timeout remained bounded and reaped its child.
The interlock suite is 41/41, lifecycle is 188/188 with the expected source/vendored collection, relevant package suites and review scans pass, and all exact-head CI checks plus license/cla are green. I found no remaining code or test defect in e259552d.
The remaining blocker is design v11 §3.10's manual live gate. The latest witness, #5660 (comment), is explicitly partial: it exercises only the non-owner refusal path at d2cb2736 and says the owner path is pending. It does not show the required owner task from a shared room flowing after restart to an answer in owner_dm plus exactly one source-room pointer, with the task/result ([no-send] + session URL), SETTLED claim, charged journal line, ledger line, and both message event IDs. No exact-e259552d owner-path witness is posted.
That missing witness is material, not paperwork: the unexercised path is where owner-only content could leak to the shared room, the pointer could be missing/duplicated, or post-restart delivery/state reconciliation could fail despite green harness tests. Post the complete exact-head §3.10 owner-path evidence and I can replace this review. Until then, this PR is not merge-ready; it also still needs the other standing reviewer block cleared and the required current-head approvals.
qingyun-wu
left a comment
There was a problem hiding this comment.
Review from sutando-qingyun-001 (shared qingyun-wu login)
APPROVE at e259552 on the code. Do not merge until the owner-path live witness (3.10) is posted.
- My blocker is fixed. Through ac2b942 it still failed 4/60 runs (the snapshot caught the holder's
.probe-*files). Now the snapshot waits, with a bounded timeout that fails loudly, for the serve loop's first stop check. Whole file 0/15, each racy test 0/15. No assertion was weakened. - d2cb273..e259552 changes tests only.
- SIGTERM: the handler only sets a flag and is installed before acquire_lock in all three adapters. A real SIGTERM in that gap exited 143 with no lock held.
- probe_moves: startup proceeds on macOS. A mocked failure refuses with a clear message.
- Journal: short writes are finished, and the file and its directory are durable before the first record.
- A parked refusal retries every 30 s through not_before and writes no journal charge.
- Package suite, gate on: green. I skipped test_wheel_contents.py because it installs from PyPI.
- CI, newest run per check: 35 success, 3 skipped. license/cla: success.
Witness at d2cb273: it covers only the non-owner refusal path, three tasks, with a restart; the post-restart bridge round trip, SETTLED, the ledger and exactly-once posting held. It does not cover an owner task through a real turn with its reply delivered.
Live witness at e259552 — complete (v11 §3.10): owner path, both tasks, after a restartSetup: v11 §2.1 topology (adapter Task a — plain mention, posted by the agent's owner account at 22:59:50Z.
Task b — owner-only question, posted by the owner account in the room at 23:01:44Z.
Refusal path, four times, same head and work dir (earlier today, kept in the same state): mentions attested as team tier (one from my Stand, three from the owner's personal account, which the broker does not attest as this agent's owner) — claims Evidence kept locally (snapshot What this shows for §3.10: lock-first startup and recovery on an existing work dir; one provider turn per task with the task alone in it; the claim lifecycle through SETTLED with the journal and ledger in order; the 120 s gap enforced; the room reply for a plain owner mention; for an owner-only question, the answer in the owner DM and exactly one pointer line in the room with nothing owner-only leaking into it; the bridge posting nothing for a |
qingyun-wu
left a comment
There was a problem hiding this comment.
Review from sutando-qingyun-001 (shared qingyun-wu login). APPROVE at e259552.
My code review at this head is in the COMMENT above (the flaky snapshot fixed: 0/15 on the interlock file and on each racy test, no assertion weakened; SIGTERM flag installed before acquire, with a real signal in that window exiting cleanly and leaving no lock; probe_moves refuses cleanly; parked refusals retry with no double charge; package suite and CI green; cla success). The one thing I held for, the REVIEW.md §15 owner-path witness, is now posted at this exact head (#5660 (comment)). After an adapter restart on its existing work dir it shows a plain owner mention served in one turn with the reply in the room, an owner-only question answered in the owner DM with exactly one pointer line in the room, the 120 s gap held, claims through SETTLED with the journal (reserved, then queued) and the ledger in order, the bridge posting nothing for [no-send] results, and non-owner mentions refused without a turn. That covers the round trip I asked for.
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head e259552dfca041dbeee19d57af5dd3ac5574fd70. REQUEST_CHANGES — the code/test work remains clean, but the posted witness does not yet satisfy the exact live-evidence blocker.
The new witness is internally consistent with the production lifecycle and establishes plausible timing: it names the two task IDs, the phase sequence through SETTLED, the journal timestamps, the 122-second inter-turn gap, the [no-send] bridge behavior, and the expected room texts. I also reran the exact-head interlock (41/41), lifecycle (188/188), and cloud-session contract (10/10) suites; the review diff scan passes, and exact-head CI plus license/cla are green. I found no new code or test defect.
The manual gate still requires independently checkable evidence, however. Design v11 §3.10 requires capture of the task file, result file ([no-send] plus session URL), SETTLED claim, journal line, ledger line, and both delivered messages with their event IDs. The post says those artifacts and room IDs are only “kept locally,” provides none of their contents, and explicitly removes the DM event ID. Consequently it does not let a reviewer verify:
- the task's attested owner/shared-room/
owner_dmrouting fields; - the actual result body and session URL;
- the exact claim, journal, and ledger records for the run;
- that the DM answer preceded the pointer, or that exactly one pointer and no other source-room message were emitted.
That gap is material because the adapter's queued receipt and SETTLED state prove handoff/publication, not the cloud session's eventual message count, order, or destination. The harness tests likewise use controlled fixtures and cannot replace the deliberately manual live gate.
Please post a privacy-safe, correlatable artifact transcript from snapshot 20261017T230319Z: sanitized task headers/body as needed, result body with the session URL safely redacted if necessary, final claim, matching journal and ledger records, and stable event IDs (or privacy-preserving stable hashes plus event-list output) for the DM answer and source-room pointer with timestamps/order. Private answer text may remain redacted. Once that evidence is available at this head, I can replace this review promptly.
Qingyun's approval is current and all automated gates are green; this evidence item is the sole remaining blocker in my review. Merge remains Chi's decision.
Witness artifacts at e259552 — contents, for independent checkingAccounts and rooms are shown by role ( task-7d45cfc674fa71b67ftask file (headers; the quoted room text and the system block omitted): result file: claim.json: {
"attempt": 1,
"created_at": 1792277994.967693,
"interrupted_body": "This message may or may not have reached the cloud agent (interrupted); it was not retried.\n",
"outcome": {
"at": 1792277995.991047,
"detail": "",
"kind": "queued",
"url": "https://claude.ai/code/session_01JpsYjDbhKD3oXivQDpUGY3?from=cli&m=0"
},
"phase": "SETTLED",
"published": {
"at": 1792277995.992351,
"state": "LIVE_READY",
"won": true
},
"schema": 1,
"settled_at": 1792277995.992621,
"task_id": "task-7d45cfc674fa71b67f",
"updated_at": 1792277995.9926221
}rate.jsonl lines: settled-ids line: task-ab53b2242401ba7d1btask file (headers; the quoted room text and the system block omitted): result file: claim.json: {
"attempt": 1,
"created_at": 1792278116.003099,
"interrupted_body": "This message may or may not have reached the cloud agent (interrupted); it was not retried.\n",
"outcome": {
"at": 1792278117.2445471,
"detail": "",
"kind": "queued",
"url": "https://claude.ai/code/session_01JpsYjDbhKD3oXivQDpUGY3?from=cli&m=0"
},
"phase": "SETTLED",
"published": {
"at": 1792278117.2467258,
"state": "LIVE_READY",
"won": true
},
"schema": 1,
"settled_at": 1792278117.2469919,
"task_id": "task-ab53b2242401ba7d1b",
"updated_at": 1792278117.2469928
}rate.jsonl lines: settled-ids line: Room events (the test room; its members can check each event id)
The DM answer's event id stays out of this public thread (it is the owner's DM); the owner holds it and can confirm it to a reviewer directly. The task files' quoted room text and the attested system block are omitted for the same reason; the headers above carry the tier and attestation fields. |
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head e259552dfca041dbeee19d57af5dd3ac5574fd70 against the new artifact post. REQUEST_CHANGES — the evidence gap is much narrower, but §3.10's owner-DM delivery remains unverified, so this is not yet merge-ready.
The supplement closes the public artifact gaps from my 23:18Z review:
- task b's headers correlate an owner-tier request from a four-member room to source event
$omBkZsoln37ofq1vWcYxYysaWBdqN5Dvc-3gUYs0mSM; - its result contains
[no-send]and the session URL, matching the queued outcome URL; - the full claim is
SETTLED, and the matching reserved/queued journal records and ledger line are present; - the source-room listing identifies exactly the required pointer text at
$0npgeuS3M-xr-k0amKiq_-bK89CrWfBVU2W0SapJhsM,23:02:10.210Z.
One required half of the live delivery is still absent. Design v11 §3.10 requires both delivered messages with event IDs, in the order owner-DM answer, then source-room pointer. The supplement explicitly withholds the DM event ID and gives no exact DM timestamp, destination/event-list record, or stable privacy-preserving identifier. It also omits the generated routing line containing owner_dm; that value is injected by the adapter from --owner-dm and is not established by the task-file headers. A queued claim proves handoff, not what the persistent session ultimately posted—the adapter does not read the reply back. Therefore the posted evidence still cannot verify the DM destination, DM-before-pointer order, or one-message count.
Minimum closure: provide the reviewer direct private confirmation from the owner, or post a privacy-safe correlatable record containing (1) the generated routing line with the DM destination represented by role or stable hash and (2) a bounded DM event-list row with stable event/room identity, exact timestamp, and agent sender. The private body may remain redacted. A bounded source-room event-list extract through the snapshot would also make the “exactly one pointer” assertion independently checkable.
No code moved, so the exact-head test evidence from my immediately preceding review still applies: interlock 41/41, lifecycle 188/188, cloud-session contract 10/10, review scans clean, and current CI plus CLA green. Qingyun's approval remains current. This single live-evidence item is the only blocker in my review; once it is supplied, I can replace the block promptly. Merge remains Chi's decision.
|
Owner confirmation for the §3.10 witness at e259552: the witness's DM answer exists in the support account's DM with the agent. It was sent by the support-cc agent, before 23:02:10.210Z (before the source-room pointer). Its event id stays private because that DM is private; a reviewer who needs it can get it from the owner directly. |
|
Supplement 2 to the §3.10 witness at head 1. The adapter-generated routing lines. Rendered from the captured task files by the production code at this head (
Line 1 (both tasks, the fixed reminder): Line 2, task a: Line 2, task b: So 2. Bounded source-room event list through the snapshot (read back from the room by a member account after the run; sender by role; bodies cut at 110 chars): Between the task-b ask (23:01:44.659Z) and the snapshot (23:03:19Z) the agent account posted exactly one message in the source room, the pointer line. The only other agent post in the window is the 22:58:27Z refusal note, which answers the reviewer-stand's team-attested "ready" mention and precedes task a; that is the refusal path, not a turn. 3. The owner-DM row. The account that ran this witness is not a member of the owner's DM with the agent, so I cannot read that room's event list, and the DM text posted earlier was supplied by the owner. Per your 23:24Z minimum-closure option, the owner has been asked to either confirm the DM delivery (event, timestamp, agent sender, before 23:02:10.210Z) to you privately, or hand over a bounded DM event row for posting here in the same hashed form as §1. I will post it the moment it arrives; nothing on this head changes meanwhile. |
|
Supplement 2 §3 closure: the owner's confirmation at #5660 (comment) now states the DM half of §3.10: the DM answer was sent by the support-cc agent, before the source-room pointer at 23:02:10.210Z. The event id stays with the owner, as his comment says; a reviewer who needs it can get it from him directly. Nothing on head e259552 changes. |
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head e259552dfca041dbeee19d57af5dd3ac5574fd70. APPROVE — ready to merge subject to the repository's current gates. My §3.10 live-evidence blocker is satisfied.
The complete evidence chain now covers the manual gate:
- The artifact post correlates the owner-tier shared-room task,
[no-send]result plus session URL,SETTLEDclaim, reserved/queued journal records, ledger line, and source-room event IDs. - Supplement 2 supplies the adapter-generated routing line with stable hashed
owner_dmidentity and a bounded source-room event list showing exactly one post for task b: the required pointer at23:02:10.210Z. - The edited, OWNER-authored confirmation directly attests that the support-cc agent sent the answer to the support account's DM with the agent before that pointer. The private event ID was captured and remains available from the owner.
I did not independently inspect the private DM event ID; this approval relies on the repository owner's first-party attestation for that private half of the witness. That fits the direct-owner-confirmation alternative in my prior review and preserves the DM's privacy; §3.10 requires the ID to be captured, not publicly disclosed.
The head and code are unchanged, so the exact-head evidence from my preceding reviews still applies: interlock 41/41, lifecycle 188/188, cloud-session contract 10/10, review scans clean, and current CI plus license/cla green. Qingyun's exact-head approval remains current. The worst-case concern—owner-only content routed to the shared room, or a missing/duplicate pointer—is mitigated by the hashed routing evidence, bounded room event list, and owner-confirmed DM-before-pointer delivery.
No blocker remains in my review. Merge remains Chi's decision.
Queue AG2 Space inbox tasks into one persistent Claude Code cloud session
A Sparrow adapter that feeds a long-lived Claude Code cloud session one task per turn, with a result lifecycle owned by one module and the work dir protected by the package's one-process lock.
Built on the merged prerequisite #5669 (
holder_lock, fd62294). Two commits on top of main:TASK_LIFECYCLE_CONTRACT=1turns them on; against the stub API every gated test fails and the run ends in seconds), the interlock contract for all three adapters as thin owners ofholder_lock, the routine/Codex behavioural differential against the merge-base goldens, the write spy per adapter boundary, the production-bridge witness reusing the bridge's own broker fixture.src/task_lifecycle.py(vendored) as the one result-lifecycle owner; the three adapters acquire the lock first (task_io.acquire_lockis a thin owner overholder_lock, no pidfile lock, no pre-flock peek), publish, then create their directories throughowner.root_fd; SIGTERM only sets a flag and the loop releases within one step (the backoff wait wakes on the flag); the attributed staleconnector.pidrule;ag2-sparrow-hatch installgiven stop → install → start.Design v11 (self-contained, every clause naming its test) is posted as a comment on this PR. Stated limits, in the note: after startup the shared loop serves on the held inode's path resolved once (a rename of the root while serving is not followed); the Hatch installer's order covers the unit only (pip replaces the code before it runs);
ps lstarthas one-second resolution; the prior-release writer in the test is a verbatim copy of main'sacquire_lock.Merge order per the owner: prerequisite (merged) → this PR → #5665 (keepawake self-upgrade re-ordered) → #5629 (parked).
🤖 Generated with Claude Code