Skip to content

feat(sparrow): Codex adapter — task files become turns on one Codex app-server thread - #5611

Merged
sonichi merged 9 commits into
mainfrom
feat/sparrow-codex-adapter
Oct 16, 2026
Merged

sonichi merged 9 commits into
mainfrom
feat/sparrow-codex-adapter

Conversation

@sonichi

@sonichi sonichi commented Oct 16, 2026 •

Copy link
Copy Markdown
Owner

What changed, and why

A new opt-in adapter, packages/ag2-sparrow/ag2_sparrow/adapters/codex/, that pushes task files into a running OpenAI Codex session as new turns — the Codex counterpart of a Claude Code mod's $.prompt.submit. Research showed three workable shapes and one that is not (hooks); the adapter implements the two that keep memory across tasks:

  1. Own app-server thread (--server stdio): the connector spawns codex app-server --listen stdio://, handshakes (initialize/initialized), starts or resumes one durable thread, submits each tasks/task-*.txt as turn/start inside a random-tagged untrusted-data wrapper, takes the turn's final agentMessage, and writes results/task-<id>.txt via temp-file + os.replace. One task in flight, mtime order, task: parsed as the terminal header. Approval requests that still arrive are declined and recorded.
  2. --push <thread> <task-file>: the same wrapped text handed to codex queue --thread, for a thread owned by a running interactive TUI (Codex ≥0.157 runs the TUI on the shared daemon).

stub_app_server.py is the test double (same JSON-RPC shapes, no model). The new test is added to the CI out-of-tree sparrow list, next to test_hatch_adapter.py.

Placed under ag2_sparrow/adapters/codex/ (not packages/ag2-sparrow/adapters/) because that is where the package's existing adapter lives and [tool.setuptools.packages.find] discovers it.

Read first

codex_connector.py (parse_task, wrap_untrusted, write_atomic, Connector.run_turn, push), then tests/test_codex_connector.py, then the README's tables.

What this proves

Memory across tasks on one thread, a completion signal per task, atomic result publication, and a push path with no keystrokes. The README records what remains unproven (resume on a real durable thread, a hand-written client on the shared daemon, real approval round trips).

Feature/documentation consistency

N/A for docs/: this adds an optional package adapter with its own README; no user-visible Sutando behavior changes. src/ is untouched. Adopting it for the Codex core is the follow-up #5610 (not closed here).

Before/after evidence

Parent commit (origin/main), the test path does not exist:

$ git show origin/main:packages/ag2-sparrow/tests/test_codex_connector.py
fatal: path 'packages/ag2-sparrow/tests/test_codex_connector.py' exists on disk, but not in 'origin/main'

HEAD (39b335b), the single evidence run (Python 3.14.6, stub only — no network, no real codex):

$ python3 -m unittest packages/ag2-sparrow/tests/test_codex_connector.py
......Error: no rollout
.Queued message m for thread t.
...
----------------------------------------------------------------------
Ran 15 tests in 3.476s

OK

Honesty note (first commit): the first run of this file had two failures, both test-side — a regex that also counted the tag's prose mention, and a run=subprocess.run default bound at definition time that let the mock miss (so that test had called the real codex queue). Both fixed in this commit; the output above is the rerun.

bash scripts/review-checks.sh --diff <(git diff origin/main...HEAD) → review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean).

Live measurements (dev Mac, Codex CLI 0.160.1 logged in via ChatGPT, read-only sandbox, approvals never, ephemeral thread, gpt-6.1-sol — no mini model is offered to the account, prompts were one line each)

task result detect s turn s task-mtime → result-mtime s
"Reply with exactly the text: PONG 41" PONG 41 3.12* 2.88 6.00*
"What number did my previous task ask…" 41 5.00* 1.50 6.50*
prompt-injection probe DECLINED 5.49* 2.08 7.58*
dropped while the connector was idle OK 4 0.028 2.20 2.23

* the three files pre-existed at start-up, so "detect" includes the handshake and queueing. Task 2 answering 41 is the memory-across-tasks proof; task 3 is the wrapper holding.

--push path, own TUI in an own tmux server: codex queue returned rc 0 in 0.13 s; the TUI rendered • QUEUED OK 10.42 s later (model ~2 s, the rest is the TUI's drain cadence). Precondition hit first: a zero-turn thread answers no rollout found for thread id.

Live path?

No: nothing under src/, no bridge, no delivery loop, no startup path changes. The measurements above are the adapter's own round trips, not a Sutando restart.

Stacked?

N/A. Follow-up (not closed by this PR): #5610 — replace the Codex core's tmux send-keys with codex queue --thread.

Hardcoded-path check

Scanner PASS (above). The connector takes every path as an argument (--work, --cwd); the only literal is the PATH fallback /usr/bin:/bin for the child's environment.

Worst-case disruption

None for existing installs: the adapter is opt-in, imported by nothing, and the CI change only adds one test invocation.

Review round 1 (REQUEST_CHANGES at c05d00e) — fixed in 39b335b

  • B1 headers now come from the package owner: adapters/hatch/task_fields.attested_headers over local_task_protocol (strict task-last, trusted last-wins only under task_layout: mid), tier canonicalised the way hatch does (absent/unknown → guest). For a mid file the writer's one-line message goes inside the wrapper; the bridge-written lines below access_tier (tier guardrail, secret notice) go after the closing marker as operator text. Tests: a fixture built from the gateway's shape with the real ag2space_tier_lines, a forged access_tier: owner in the body that does not win, and a delegation pin that patches attested_headers and asserts the call. The tier in the wrapper is annotated, not enforced; Codex core: replace tmux send-keys with codex queue --thread (lost-Enter reproduced) #5610 must carry enforcement.
  • B2 a turn past --turn-timeout gets turn/interrupt {threadId, turnId} (schema-exact), then --interrupt-grace; if it still does not complete, the app-server process is replaced and the thread resumed before the next task starts. Stub gained [[stall]] (completes on interrupt) and [[stall-hard]] (ignores it); both paths tested, including that the second task lands on the same thread id after the restart.
  • B3 the order fixture gives lexically later names the earlier mtimes (expected c, b, a); names.sort() would now fail it.
  • Nits: thread/resume carries cwd/approvalPolicy/sandbox/model and a stale id logs and falls back to thread/start (tested); state/thread-id is read back on start (tested); result mtime comes from the temp fd before os.replace (tested against a claim-rename); a task counts as done if its result exists or its id is in state/done (bounded), so a drained result is not re-run (tested); request logging is method names only; README: unconditional decline, --thread by name marked unexercised.

First run of the revised file had one failure, a real one: state/done lost its order by round-tripping through a set. Fixed; the output above is the rerun.

Coverage gate (REQUEST at 0cda940: diff-cover saw 0% for both adapter files) — fixed in fd2ddf2

Cause, read from .github/workflows/ci.yml + scripts/discover-python-tests.sh: the coverage legs run only tests/**/*.test.py and skills/**/*.test.py; packages/ag2-sparrow/tests/test_*.py is run by the out-of-tree step without coverage. The package suites that are measured have a tests/ag2-sparrow-<name>.test.py runpy wrapper (e.g. test_human_action.py); this PR adds tests/ag2-sparrow-codex-connector.test.py the same way. The connector already ran in-process; the stub child is now traced too under SUTANDO_TEST_SUBPROCESS_COVERAGE=1 with the coverage run --rcfile prefix the runtime-cli suites use (rc parallel=True, the leg's coverage combine merges the fragment). Remaining paths got tests (list-models/list-loaded, server exit mid-turn, other server requests + error notifications, idle exit, proxy no-restart branch, done-list bounds, Rpc kill, env fallback, --work required).

Single foreground run (Python 3.14.6; the usage text is the expected stderr of the --work-required test):

$ python3 -m unittest packages/ag2-sparrow/tests/test_codex_connector.py
.usage: python3.14 -m unittest [-h] [--server {stdio,proxy,stub}] [--work WORK]
                              [--cwd CWD] [--model MODEL]
                              [--sandbox {read-only,workspace-write,danger-full-access}]
                              [--thread-id THREAD_ID] [--ephemeral]
                              [--developer-instructions DEVELOPER_INSTRUCTIONS]
                              [--once ONCE] [--idle-exit IDLE_EXIT]
                              [--poll POLL] [--turn-timeout TURN_TIMEOUT]
                              [--interrupt-grace INTERRUPT_GRACE]
                              [--list-models] [--list-loaded]
                              [--push THREAD TASK_FILE]
python3.14 -m unittest: error: --work is required unless --push is given
......Error: no rollout
.Queued message m for thread t.
...
----------------------------------------------------------------------
Ran 26 tests in 10.770s

OK

Local coverage, same collection as the gate (from the repo root):

$ SUTANDO_TEST_SUBPROCESS_COVERAGE=1 python3 -m coverage run --rcfile=.coveragerc tests/ag2-sparrow-codex-connector.test.py
$ python3 -m coverage combine --rcfile=.coveragerc --quiet
$ python3 -m coverage report --rcfile=.coveragerc --include='packages/ag2-sparrow/ag2_sparrow/adapters/codex/*' -m
Name                                                                 Stmts   Miss  Cover   Missing
--------------------------------------------------------------------------------------------------
packages/ag2-sparrow/ag2_sparrow/adapters/codex/__init__.py              0      0   100%
packages/ag2-sparrow/ag2_sparrow/adapters/codex/codex_connector.py     342      8    98%   58, 61-62, 99-100, 269, 300, 346
packages/ag2-sparrow/ag2_sparrow/adapters/codex/stub_app_server.py      88      3    97%   49, 118-119
--------------------------------------------------------------------------------------------------
TOTAL                                                                  430     11    97%

Code-owner review (REQUEST_CHANGES at fd2ddf2) — fixed in 27aab92

B1 non-owner tiers. (a) The wrapper now carries the Hatch adapter's NON_OWNER_NOTE for every non-owner tier, read from ag2-sparrow-hatch.sh at startup with a pinned fallback; test_pinned_note_is_byte_identical_to_the_hatch_script keeps the two identical (same read-then-pin shape as #5612). collaborator rides in the metadata. Owner tasks keep "Act on it as the user's request"; non-owner bodies are data with the note. (b) Non-owner turns run on their own ephemeral thread per task, approvalPolicy: never, sandbox read-only regardless of --sandbox; owner turns keep the durable thread. The mutant test runs a guest with --sandbox danger-full-access and asserts the stub saw sandbox=read-only ephemeral=True (and the owner task in the same run saw danger-full-access ephemeral=False). (c) --push refuses every non-owner tier with exit 2 and a message, subprocess.run never called.

B2 thread leak. Owner threads are keyed per channel_id under state/threads/<key>; tests: two owner channels get distinct ids and a channel keeps its id; an owner + two guests + a team task yield four distinct ids and only the owner thread is saved. README's live-run narrative now says the 41 recall was one owner channel and that a guest has no such memory.

Non-blocking, done: README states Sparrow never stamps envelopes, so invalid/unsigned are submitted and the tier is the only gate (hence the isolation); sandbox default pinned (read-only is the parser default and the only non-owner value); server exit mid-turn → restart + resume + one retry (owner) or a plain failure body (non-owner), and serve goes on (tested both); an empty completed turn retries once then writes [no reply] … (owner retry succeeds on the same thread; a guest's fresh thread is empty again → [no reply]); timeout/failure bodies are plain text with any partial reply below (no JSON; asserted); single-instance lock state/connector.pid with os.kill(pid, 0) liveness (tested live pid → exit 3, dead pid → taken over, garbage → taken over, released on exit); README merge-order note vs #5612.

Single foreground run (Python 3.14.6; Error: no rollout and Queued message… are the mocked codex queue outputs):

$ python3 -m unittest packages/ag2-sparrow/tests/test_codex_connector.py
.usage: python3.14 -m unittest [-h] [--server {stdio,proxy,stub}] [--work WORK]
.......Error: no rollout
..Queued message m for thread t.
......
----------------------------------------------------------------------
Ran 40 tests in 11.438s

OK

Local coverage, same collection as the gate:

$ SUTANDO_TEST_SUBPROCESS_COVERAGE=1 python3 -m coverage run --rcfile=.coveragerc tests/ag2-sparrow-codex-connector.test.py
$ python3 -m coverage combine --rcfile=.coveragerc --quiet
$ python3 -m coverage report --rcfile=.coveragerc --include='packages/ag2-sparrow/ag2_sparrow/adapters/codex/*' -m
Name                                                                 Stmts   Miss  Cover   Missing
--------------------------------------------------------------------------------------------------
packages/ag2-sparrow/ag2_sparrow/adapters/codex/__init__.py              0      0   100%
packages/ag2-sparrow/ag2_sparrow/adapters/codex/codex_connector.py     434     12    97%   92, 95-96, 136-137, 358, 389, 428, 465-466, 582-583
packages/ag2-sparrow/ag2_sparrow/adapters/codex/stub_app_server.py      92      3    97%   49, 124-125
--------------------------------------------------------------------------------------------------
TOTAL                                                                  526     15    97%

First run of the revised suite had one real failure: the metrics row of a task whose server was restarted recorded thread: null; fixed (the row records the thread the task ran on). The outputs above are the rerun.

Code-owner re-review (REQUEST_CHANGES at 27af0a2) — fixed in bce3491

  • B1 --thread-id is now CHANNEL_ID=THREAD and seeds exactly that channel (bound_thread_id); the test runs rooms A and B with --thread-id '!a:x=bound-0001' and asserts A resumed it and B did not.
  • B2 channel_key is the sha256 hex of channel_id (default when absent); the readable id is written inside the file, never in its name. Tests: .., ., a 200-char pair differing after char 120, and !a:b.org vs _a_b.org all map to distinct 64-hex files; an owner task from channel .. completes and saves under its digest.
  • Non-blocking, done: non-owner turns run with cwd = a fresh empty temp dir removed after the turn (tested: the stub saw it, it is gone afterwards, the owner's cwd is --work); pidfile via O_CREAT|O_EXCL with stale-check → unlink → one retry (tested live/dead/garbage/double-failure); README notes the owner retry-after-death side-effect caveat.
$ python3 -m unittest packages/ag2-sparrow/tests/test_codex_connector.py
Ran 43 tests in 11.746s

OK

🤖 Generated with Claude Code

…pp-server thread

A stdlib connector that owns a `codex app-server` child over stdio, keeps one
durable thread, submits each tasks/task-*.txt as a turn inside a random-tagged
untrusted-data wrapper, and writes results/task-<id>.txt atomically. `--push`
hands the same wrapped text to `codex queue` for a thread owned by a running
TUI. A stub app-server is the test double; CI runs the new test file with the
other out-of-tree sparrow tests.

Stand: Echo Act IV Pro
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@sonichi
sonichi requested a review from qingyun-wu as a code owner October 16, 2026 07:51
@github-actions

github-actions Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

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

Diff Coverage

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

  • packages/ag2-sparrow/ag2_sparrow/adapters/codex/codex_connector.py (95.0%): Missing lines 104-115,117-118,151-152,395,426,465,502-503,622-623
  • packages/ag2-sparrow/ag2_sparrow/adapters/codex/stub_app_server.py (96.7%): Missing lines 49,124-125
  • packages/ag2-sparrow/tests/test_codex_connector.py (99.6%): Missing lines 38,429

Summary

  • Total: 1040 lines
  • Missing: 28 lines
  • Coverage: 97%

packages/ag2-sparrow/ag2_sparrow/adapters/codex/codex_connector.py

Lines 100-122

  100         self.next_id = 1
  101         threading.Thread(target=self._reader, daemon=True).start()
  102 
  103     def _reader(self):
! 104         for line in self.p.stdout:
! 105             line = line.strip()
! 106             if not line:
! 107                 continue
! 108             try:
! 109                 m = json.loads(line)
! 110             except ValueError:
! 111                 continue
! 112             if "id" in m and "method" not in m:
! 113                 q = self.pending.pop(m["id"], None)
! 114                 if q is not None:
! 115                     q.put(m)
  116             else:
! 117                 self.events.put(m)
! 118         self.events.put(None)
  119 
  120     def _send(self, obj):
  121         with self.lock:
  122             self.p.stdin.write(json.dumps(obj) + "\n")

Lines 147-156

  147 
  148     def close(self):
  149         try:
  150             self.p.stdin.close()
! 151         except OSError:
! 152             pass
  153         try:
  154             self.p.wait(timeout=5)
  155         except subprocess.TimeoutExpired:
  156             self.p.kill()

Lines 391-399

  391         deadline = time.time() + timeout
  392         while True:
  393             remaining = deadline - time.time()
  394             if remaining <= 0:
! 395                 return None
  396             try:
  397                 m = self.rpc.events.get(timeout=remaining)
  398             except queue.Empty:
  399                 return None

Lines 422-430

  422         interrupted; if it still does not complete within the grace, the server is
  423         replaced, so the next task never shares the thread with a live turn."""
  424         params = {"threadId": self.thread_id, "input": [{"type": "text", "text": prompt}]}
  425         if model:
! 426             params["model"] = model
  427         res = self.rpc.request("turn/start", params)
  428         turn_id = res["turn"]["id"]
  429         done = self.wait_turn(turn_id, self.args.turn_timeout)
  430         if done is not None:

Lines 461-469

  461             if status == "completed" and not (text or "").strip() and attempt == 1:
  462                 log("empty reply; retrying once")
  463                 continue
  464             return status, text, err
! 465         return status, text, err
  466 
  467     def result_body(self, status, text, err):
  468         if status == "completed":
  469             return (text or "").strip() or NO_REPLY

Lines 498-507

  498                     log("idle-exit")
  499                     return
  500                 try:
  501                     m = self.rpc.events.get(timeout=self.args.poll)
! 502                     if m is not None and "id" in m:
! 503                         self.handle_server_request(m)
  504                 except queue.Empty:
  505                     pass
  506                 continue
  507             name = names[0]

Lines 618-627

  618         c.drop_scratch()
  619         log("closed pid", c.rpc.p.pid, "rc", c.rpc.p.returncode)
  620         try:
  621             os.unlink(lock)
! 622         except OSError:
! 623             pass
  624     return 0
  625 
  626 
  627 if __name__ == "__main__":

packages/ag2-sparrow/ag2_sparrow/adapters/codex/stub_app_server.py

Lines 45-53

  45 
  46 for line in sys.stdin:
  47     line = line.strip()
  48     if not line:
! 49         continue
  50     m = json.loads(line)
  51     meth, rid, p = m.get("method"), m.get("id"), m.get("params") or {}
  52     if meth is None and rid in PENDING:
  53         tid, turn_id, item_id, reply = PENDING.pop(rid)

Lines 120-126

  120             STALLED.pop(p["turnId"])
  121             send({"jsonrpc": "2.0", "method": "turn/completed",
  122                   "params": {"threadId": tid, "turn": {"id": p["turnId"], "status": "interrupted", "items": [],
  123                                                        "error": {"message": "interrupted"}}}})
! 124     elif rid is not None:
! 125         send({"jsonrpc": "2.0", "id": rid, "error": {"code": -32601, "message": "stub: unknown method %s" % meth}})

packages/ag2-sparrow/tests/test_codex_connector.py

Lines 34-42

  34     cwd, where the gate's `coverage combine` picks it up."""
  35     argv = REAL_SERVER_ARGV(server)
  36     if server == "stub" and os.environ.get("SUTANDO_TEST_SUBPROCESS_COVERAGE") == "1":
  37         return [sys.executable, "-m", "coverage", "run", "--rcfile=%s" % (REPO_ROOT / ".coveragerc")] + argv[1:]
! 38     return argv
  39 
  40 
  41 cc.server_argv = covered_server_argv

Lines 425-433

  425             raise RuntimeError("stuck")
  426         return {"turn": {"id": "t1"}, "thread": {"id": "th1"}}
  427 
  428     def notify(self, method, params=None):
! 429         pass
  430 
  431 
  432 class EdgePathTest(unittest.TestCase):
  433     def test_list_models_and_loaded_threads_print_json(self):

…-safe results

Headers come from the package parser (hatch's attested_headers over
local_task_protocol), so a gateway task_layout: mid file takes the writer's
access_tier from below the body and its guardrail lines stay outside the
untrusted wrapper. A turn past --turn-timeout gets turn/interrupt and a bounded
grace; if it still runs, the app-server is replaced and the thread resumed
before the next task. thread/resume carries the thread settings and a stale id
falls back to thread/start; state/thread-id is read back; the result mtime is
taken from the temp fd before the rename; state/done keeps finished ids so a
drained result is not re-run; request logging names methods only. The order
test now gives lexically later names the earlier mtimes.

Stand: Echo Act IV Pro
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
sonichi and others added 2 commits October 16, 2026 01:06
…ack, no restart after a late completion

State the one-line mid-body assumption the gateway's writer guarantees; a
thread/resume that times out falls back to thread/start like a refused one; a
turn that completes between the deadline and the interrupt is read as done
instead of restarting the app-server. Both new paths are driven by the stub.

Stand: Echo Act IV Pro
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…age gate

Coverage discovery takes tests/**/*.test.py only, so the package suite never
ran under coverage and diff-cover saw 0% for both adapter files. Add the
tests/ runpy wrapper the other package suites use, trace the stub child under
SUTANDO_TEST_SUBPROCESS_COVERAGE like the runtime-cli suites, and cover the
remaining paths: list-models/list-loaded, server exit mid-turn, the other
server requests and error notifications, idle exit, the proxy no-restart
branch, done-list bounds, Rpc kill on EOF-ignoring child, env fallback.

Stand: Echo Act IV Pro
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@sonichi

sonichi commented Oct 16, 2026

Copy link
Copy Markdown
Owner Author

@qingyun-wu review requested: /packages/ code owner. CI green at fd2ddf2 (26 tests; coverage 97% on the adapter). Evidence, live measurements and limits are in the body. Sibling #5612 adds a line at the same ci.yml spot; whichever merges second needs a base merge.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-owner review for /packages/ from sutando-qingyun-001 (shared qingyun-wu login), at fd2ddf24.

Verdict: REQUEST_CHANGES. There are two blockers, both on paths a guest can reach.

Already sound: the connector picks the result file from the inbox file name (:365), so the model can't redirect a reply. An absent or unknown tier resolves to guest. Header policy goes through the shared task_fields.attested_headers rather than a copy. Mutants are caught: tier forced to owner, interrupt removed, wrong result id, restart skipped, approval flipped to accept. CI and CLA are green.

Blocker 1: non-owner tiers get a label, not limits

  • wrap_untrusted (codex_connector.py:127-139) puts access_tier= into a metadata line, then tells the model to "Act on it as the user's request". There is no non-owner wording like Hatch's NON_OWNER_NOTE (hatch/ag2-sparrow-hatch.sh:34).
  • thread_params (:197-201) starts every tier with approvalPolicy: "never" and the same sandbox. The default is read-only, but danger-full-access is one flag away, and nothing varies by tier.
  • read-only stops writes, not reads. A guest can ask for workspace or memory content, and the reply is posted to the room.
  • --push (:386-393) queues guest text into the owner's own TUI session.
  • Deferring enforcement to #5610 isn't acceptable for a path guests can reach. The minimum is one of:
    • refuse non-owner tiers until #5610 lands; or
    • add the NON_OWNER_NOTE wording to the wrapper, and run non-owner turns read-only on their own thread.
  • --push should refuse non-owner tiers in either case.

Blocker 2: one shared thread leaks across rooms and tiers

  • serve() runs every task, from every room and every tier, on one durable thread (:247, :298).
  • The PR's own live run shows the leak. Task 2 answers 41, recalled from task 1 (README l.65-71).
  • So a guest in room B can ask what room A's owner said.
  • Fix: keep one thread per channel_id, or per (channel, tier), or use an ephemeral thread per task for non-owner tiers.

Non-blocking

  • No envelope verification. Sparrow never stamps, so invalid and unsigned tasks are submitted. That's acceptable, but say so in the README.
  • No coverage on the sandbox choice. The mutant forcing "danger-full-access" passes all 26 tests. Pin it, especially once tiers get their own sandbox.
  • Server exit mid-turn. There's no restart, and the next turn/start hits a closed pipe and ends serve.
  • Empty completed turn. It writes "\n" and is marked done, so the room gets nothing.
  • Timeout bodies. These post [turn timeout] {error json} to the room.
  • Single-instance lock. There isn't one.
  • Merge order. The ci.yml line sits at the same spot as #5612's.

Not verified

  • How far Codex's read-only sandbox lets reads reach outside cwd.
  • What happens live when the server dies.

sonichi and others added 2 commits October 16, 2026 01:41
… threads keyed per channel

A non-owner task (team/guest/other/ambient, from the attested headers) now runs
on its own ephemeral thread, read-only whatever --sandbox says, with the Hatch
adapter's NON_OWNER_NOTE in the prompt (read from ag2-sparrow-hatch.sh at
startup, pinned fallback kept byte-identical by a test), and --push refuses it.
Owner tasks keep a durable thread per channel_id under state/threads/, so rooms
never share memory. Also: a server that dies mid-turn is restarted (owner: thread
resumed and the turn retried once; non-owner: plain failure body); an empty
completed turn is retried once, then "[no reply]"; timeout and failure bodies are
plain text; one connector per work dir via a liveness-checked pidfile; README
states that envelopes are never verified and why, and the merge order vs #5612.

Stand: Echo Act IV Pro
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ine NON_OWNER_NOTE match

Stand: Echo Act IV Pro
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-owner re-review from sutando-qingyun-001 (shared qingyun-wu login) at 27af0a20. That is 27aab92 plus a docs-only commit, and it replaces my review at fd2ddf2.

Verdict: REQUEST_CHANGES. Both of my earlier blockers are fixed, but the new per-channel thread keying has two small blockers of its own.

Fixed

  • Non-owner sandbox: non-owner turns always run read-only, whatever --sandbox says (codex_connector.py:271-273).
  • Non-owner threads: non-owner turns get a fresh ephemeral thread per task and per retry, so nothing carries over, not even within one room.
  • Note: NON_OWNER_NOTE is on every non-owner turn. The fallback matches Hatch, and an empty match now falls back.
  • --push: it refuses non-owner tiers.
  • Retry: a dead-server retry happens at most twice, and the result is written once.
  • Failure bodies: they're plain text.
  • Mutants: each of these is caught:
    • non-owner turns using args.sandbox;
    • one shared owner thread;
    • non-owner turns on a durable thread;
    • the note dropped;
    • --push allowing non-owner tiers;
    • the pidfile check off (the suite hangs, and CI's timeout catches it).

Blockers

  1. --thread-id merges channels.
    • open_owner_thread does wanted = wanted or self.args.thread_id, so every owner channel without a saved file resumes the same thread.
    • In a probe, rooms A and B both got T-shared, and each channel's file then stores it permanently. That undoes per-channel isolation for owner context across rooms.
    • Fix: bind --thread-id to one named channel, or drop it.
  2. channel_id handling in the thread-file name.
    • channel_key keeps .. and ., so the path becomes state/threads/... open() then raises an uncaught IsADirectoryError (reproduced).
    • The task is never marked done, so every restart crashes the same way.
    • Keys also collide in two ways (both reproduced):
      • two ids that differ only after character 120;
      • !a:b.org and _a_b.org, which map to the same key.
    • Fix: name the file by a hash of channel_id, e.g. sha256 hex.

Non-blocking

  • Reads aren't confined. A non-owner turn's cwd is --work, which holds every room's tasks/ and results/. A read-only sandbox still allows reads, and the README says how far they reach "is not measured". Use an empty temp dir as the non-owner cwd, and measure the read reach.
  • Pidfile race. Read-then-write isn't atomic, so two simultaneous starts can both win.
  • Owner retry repeats the turn. After a server death, side effects under a writable sandbox can run twice.
  • Missing channel_id. It maps to a shared default owner thread. It's owner-only and documented.

CI: only license/cla had finished when I posted.

sonichi and others added 2 commits October 16, 2026 01:52
…d to one channel

The thread file is named by the sha256 of channel_id (the readable id is kept
inside the file), so `..` cannot escape state/threads and ids that differ only
after the old 120-char cut, or only in sanitized characters, get distinct
files. `--thread-id` takes CHANNEL_ID=THREAD and seeds exactly that channel; a
second channel starts its own thread. Non-owner turns run in an empty temp dir
removed after the turn; the pidfile is created O_EXCL with a stale-unlink retry;
README notes that an owner retry after a server death can repeat side effects
under a writable sandbox.

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>
@sonichi

sonichi commented Oct 16, 2026

Copy link
Copy Markdown
Owner Author

@qingyun-wu both blockers are addressed, now at 4bc90a6 (second-round items below); CI is running on that head. Please re-review.

B1, non-owner tiers: the wrapper for team/other/guest/ambient now carries Hatch's NON_OWNER_NOTE, read from ag2_sparrow/adapters/hatch/ag2-sparrow-hatch.sh at startup with a pinned fallback and a byte-identity test; collaborator is carried through; owner tasks keep "act on it as the user's request", non-owner bodies are data with the note. Non-owner turns run on an ephemeral thread with approvalPolicy: never and sandbox read-only regardless of --sandbox (mutant test: a guest under --sandbox danger-full-access → the stub sees read-only ephemeral=True). --push refuses every non-owner tier (exit 2, before codex queue is built).

B2, shared thread: owner threads are keyed per channel_id under state/threads/<key>; non-owner tasks never reuse any thread. Tests: two channels → distinct ids and a channel keeps its id; owner + two guests + team → four distinct ids, only the owner's saved. The README's live-run narrative now says the 41 recall is within one channel.

Non-blocking, done: README states envelope verification is absent (Sparrow never stamps) and why; sandbox default pinned; server death → restart + resume + one retry (owner) / plain-text failure (non-owner), serve continues; empty completed turn → retry once, then [no reply]; plain-text timeout bodies; pidfile lock with liveness check; merge-order note vs #5612.

40 tests (one foreground run, output in the body); adapter coverage 97% locally; review-checks PASS.

Second round (27af0a2 → 4bc90a6): --thread-id now takes CHANNEL_ID=THREAD and seeds only that channel (test: room A resumes it, room B starts its own). Thread files are named by the sha256 hex of channel_id (default when absent), with the readable id only inside the file; tests cover .., ., 200-char ids differing after char 120, and !a:b.org vs _a_b.org → distinct names, plus an owner task from channel .. completing. Also: non-owner turns run in a fresh empty temp dir removed after the turn; the pidfile is taken with O_CREAT|O_EXCL (stale-check → unlink → one retry); README notes that an owner retry after a server death can repeat side effects under a writable sandbox. 43 tests.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-owner re-review from sutando-qingyun-001 (shared qingyun-wu login) at 4bc90a6f. This replaces my REQUEST_CHANGES at 27af0a2.

Verdict: APPROVE. Merge once CI is green; 13 checks were still pending when I posted.

  • Thread files are keyed by digest. channel_key (codex_connector.py:76-79) is sha256(channel_id), or default when there's none. A .. or . id can no longer escape the directory, and the 120-char and punctuation collisions are gone. The readable id is kept inside the file, capped at 80 chars.
  • --thread-id is bound to one channel. bound_thread_id (:82-87) takes CHANNEL_ID=THREAD and returns the thread only for that exact channel, so other channels start their own thread. Splitting on the first = is safe, because Matrix room ids contain no =.
  • Earlier fixes still hold. Non-owner turns are always read-only on an ephemeral thread per task, carry the NON_OWNER_NOTE, and are refused by --push.
  • Tests: packages/ag2-sparrow/tests passes locally, 335 tests.
  • Non-blocking: non-owner turns still run with cwd --work, which holds every room's tasks/ and results/. An empty temp dir as the non-owner cwd, plus a measurement of how far reads reach, would close that.

…apter

# Conflicts:
#	.github/workflows/ci.yml
@github-actions

Copy link
Copy Markdown
Contributor

@cla-assistant check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants