Skip to content

feat(ag2-sparrow): Codex Cloud adapter, an opt-in way for a Codex Cloud task to answer as an AG2 Space agent - #5650

Draft
sonichi wants to merge 3 commits into
mainfrom
feat/sparrow-codex-cloud-adapter
Draft

sonichi wants to merge 3 commits into
mainfrom
feat/sparrow-codex-cloud-adapter

Conversation

@sonichi

@sonichi sonichi commented Oct 17, 2026 •

Copy link
Copy Markdown
Owner

Priority

Level: low
Reason: new opt-in adapter; nothing imports it and no running path changes.

Closes

N/A

Summary

Adds packages/ag2-sparrow/ag2_sparrow/adapters/codex_cloud/, an opt-in Sparrow adapter that lets a Codex Cloud task answer as an AG2 Space agent. It is the cloud half for Codex, matching claude_routine for Claude Code: the same CLI shape (--work <dir>), inbox, result, lock and tier policy. Delivery is unchanged: the adapter writes results/task-<id>.txt and Sparrow's outbox handles it.

Where to look first:

  • cloud_adapter.py: Adapter.submit, Adapter.poll, load_ledger, post_command.
  • codex_connector.py: the shared helpers and the new acquire_lock.
  • The README sections "Reply mode post" and "Each task submitted at most once".

How it works

  1. Submit: codex cloud exec --env <ENV_ID> [--branch B] -- <prompt>, passed as an argv list with no shell. The task id comes from the URL the CLI prints.

  2. Poll: codex cloud status <id> runs every --poll-seconds until [READY], [APPLIED] or [ERROR]. Three other things end a task:

    • --timeout-seconds measured from submission, a timer that survives restarts;
    • five status failures in a row;
    • a submit failure.

    Each of these writes a result. Due tasks are polled after every submit, so a backlog of slow submits cannot delay an earlier task's deadline.

  3. At most once: state/cloud-tasks.json marks a task submitting before exec and submitted after it. A restart resumes polling and never submits again. A task left submitting gets a "may or may not have reached Codex Cloud" result. Only an absent ledger means "no submissions". An unreadable, non-JSON or off-schema ledger refuses to start (exit 4) and runs no exec.

  4. One adapter per work dir: the shared codex_connector.acquire_lock now holds an flock on state/connector.pid for the process's lifetime. It still refuses when the file names another live pid.

  5. Tiers: non-owner tiers are refused by default, in both reply modes. With --allow-non-owner, the prompt carries the pinned Hatch NON_OWNER_NOTE, and a link-mode result leaves out the link and any error text. The message always goes inside nonce-delimited UNTRUSTED_TASK_* markers. The child process gets PATH, HOME and CODEX_HOME, nothing else.

Reply modes

  • --reply-mode link (the default) writes the task link, status and diff summary as the result. The Codex CLI does not print a cloud task's assistant messages. I checked this against the CLI source at rust-v0.160.1 (codex-rs/cloud-tasks/src/lib.rs):
    • exec prints only the URL;
    • status prints [STATUS] <title>, the environment and age, and the diff summary;
    • list --json has no message field;
    • diff prints only the diff.
  • --reply-mode post --gateway-url https://<gateway> --post-command-script <path>: the cloud agent posts its own answer. Sparrow may not touch the room-verb endpoint surface (docs/architecture-boundaries.md, "Events plane"), so the adapter does not build the post. It runs the injected script as an argv list, following "Optional adapter capabilities": the adapter is given a path and never locates or imports the skill. The script receives --room, --thread-root, --reply-to (from the attested channel_id/source_room_id, thread_root and source_message_id headers, never from the body), --dedupe-key, --token-env and --reply-file. The one line it prints goes into the prompt unchanged.
    • Fails closed: if the script exits non-zero, prints nothing or more than one line, times out or cannot run, the task gets a "could not be submitted" result and no exec runs. Post mode refuses to start without the script. A task with no room header runs in link mode.
    • The skill side: skills/agent-room-ops/post_command.py validates every argument against a strict pattern (Matrix room and event ids, https gateway, env-var name, dedupe key, absolute reply path) and exits 2 with nothing printed on any failure. It prints python3 -c '<json.dumps of op:message with the ids and the reply file>' < <reply file> | curl -sS --fail-with-body -X POST "<gateway>/v1/room" -H "Authorization: Bearer $<TOKEN_ENV>" ..., the same request as room-ops say and the bridge's _post_room_message. It never reads a token; only the variable name appears. Tests: tests/skills/agent-room-ops/post-command.test.py, including the end-to-end run against a fake curl.
    • Results: on READY or APPLIED the result is [no-send] plus one comment line. The vendored bridge parses that with result_markers.parse_markers as a skip marker and closes the lease with no_send, so nothing is posted twice. ERROR, timeout and every failure still post their result.
    • Codex environment setup (in the README): the token must be an environment variable, because Codex Cloud removes secrets before the agent phase. Agent-phase internet access must be on, with POST allowed and the allowlist limited to the gateway host. The adapter cannot verify the post happened, only that the task reached READY.
    • cloud_adapter.py contains no /v1/room and does not name the skill. A test pins both, and events-plane-boundary passes unchanged.

Shared helpers (no new copies)

codex_connector now owns these, and the Codex, Claude routine and Codex Cloud adapters all use them:

  • span_tag, wrap_span(tag, message, before, after, limit), stance(headers, note)
  • list_pending(inbox, results, *excluded)
  • prepare_work(args, *subdirs), release_lock
  • NON_OWNER_REFUSED, TRUNCATED

Behaviour is unchanged. The one difference: the Codex adapter's lister now drops a task file that vanishes between listdir and stat, as the routine adapter's already did, instead of raising.

Review follow-ups

From the review at 25045f5:

  • P1 ledger: handled by the fail-closed ledger in "How it works" step 3.
  • P1 lock: the flock in step 4.
  • P2 deadlines: the interleaved polling in step 2. All five duration flags must also be finite (nan/inf are rejected).
  • P2 NUL: caught at exec, where it becomes that task's failure result. ValueError is also caught at status.

From the second review at 25045f5:

  • Marker tests: two crash tests go through the real save path. One crashes right after the pre-submit save(); the other crashes after exec returns, before the id is saved. Each then restarts and asserts no second exec.

  • Duplicated policy: moved into codex_connector, as listed above.

  • Nits:

    • the prompt is capped at 32,000 characters to stay under Linux's 128 KiB per-argument limit;
    • errors="replace" on CLI output;
    • the --branch default is documented;
    • the README ships in package-data;
    • CODEX_HOME is passed to the child;
    • the test FakeClock fails fast instead of looping forever.

    --allow-non-owner is kept: the requested design is non-owner refused unless that flag is set.

Evidence

The earlier events-plane-boundary failure (post mode put /v1/room into a new Sparrow file) is resolved by moving the command builder into agent-room-ops, as above. The test and its allowlist are unchanged.

CI run https://github.com/sonichi/sutando/actions/runs/38996247652, at head 3881416. It runs:

  • packages/ag2-sparrow/tests/test_codex_cloud_adapter.py (new);
  • tests/skills/agent-room-ops/post-command.test.py (new);
  • tests/events-plane-boundary.test.py;
  • test_codex_connector.py, including the lock concurrency tests;
  • test_claude_routine_adapter.py;
  • test_wheel_contents.py;
  • the diff-coverage gate.

Before: the new test file fails to import against origin/main (ModuleNotFoundError: No module named 'ag2_sparrow.adapters.codex_cloud'). That run is from the first push; local runs are now held because of host load.

Still owed: before/after mutation evidence for each review item. Each fix is removed in turn, its named test is shown failing, and the fix is restored. This waits for a quiet host or a CI job.

git diff --cached | bash scripts/review-checks.sh → PASS (hardcoded-paths + root-artifacts + prose-cap clean).

Live witness: owed

No Codex Cloud environment exists on the account this was built on, and codex cloud exec fails with "no cloud environments are available for this workspace". No live run has been made in either reply mode. Before anyone enables the adapter, one real run should show:

  • an owner task: exec, then the parsed URL, then polls ending in [READY], then the result posted into the room;
  • kill -9 while [PENDING], then a restart that resumes polling with no second exec;
  • a non-owner task refused with no exec.

Docs

N/A for docs/: the sibling adapters are documented in their own READMEs and are not in docs/catalog.json.

Checklist

  • No other open PR on this topic (gh pr list --state open --search "codex cloud")
  • Author email is GH-mapped
  • Single concern (the adapter, plus the shared helpers it and its siblings now use)
  • Tests added
  • Live path: owed (above)
  • No host-specific paths

🤖 Generated with Claude Code

…ud task to answer as an AG2 Space agent

Each inbox task is submitted once with `codex cloud exec` (argv, no shell), polled with
`codex cloud status` to READY/APPLIED/ERROR or a bounded timeout, and its outcome written
as the task's result for Sparrow's outbox to post. Submissions are recorded before and
after exec, so a restart resumes polling and never submits twice. Non-owner tiers are
refused by default, as in the Claude routine adapter; with --allow-non-owner the prompt
carries the pinned Hatch NON_OWNER_NOTE.

The Codex CLI does not print a cloud task's assistant messages, so the result carries the
task link, status and diff summary.

Stand: Echo Act IV Pro
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 17, 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/claude_routine/routine_adapter.py (100%)
  • packages/ag2-sparrow/ag2_sparrow/adapters/codex/codex_connector.py (82.6%): Missing lines 303-309,313-314,328,333,346-348,358-359
  • packages/ag2-sparrow/ag2_sparrow/adapters/codex_cloud/cloud_adapter.py (99.7%): Missing lines 397
  • packages/ag2-sparrow/tests/test_codex_cloud_adapter.py (99.7%): Missing lines 83,504
  • packages/ag2-sparrow/tests/test_codex_connector.py (100%)
  • skills/agent-room-ops/post_command.py (100%)

Summary

  • Total: 1094 lines
  • Missing: 19 lines
  • Coverage: 98%

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

Lines 299-318

  299 
  300 def _lock_fd(fd):
  301     try:
  302         import fcntl
! 303     except ImportError:  # Windows: a byte-range lock stands in for flock
! 304         import msvcrt
! 305         try:
! 306             msvcrt.locking(fd, msvcrt.LK_NBLCK, 1)
! 307             return True
! 308         except OSError:
! 309             return False
  310     try:
  311         fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
  312         return True
! 313     except OSError:
! 314         return False
  315 
  316 
  317 def acquire_lock(state_dir):
  318     """One connector per work dir: an exclusive flock on state/connector.pid, held until

Lines 324-337

  324     for _ in range(3):
  325         fd = os.open(path, os.O_CREAT | os.O_RDWR, 0o644)
  326         try:
  327             if not _lock_fd(fd):
! 328                 os.close(fd)
  329                 return None
  330             try:
  331                 current = os.fstat(fd).st_ino == os.stat(path).st_ino
  332             except FileNotFoundError:
! 333                 current = False
  334             if not current:
  335                 # The holder unlinked this file as we locked it; lock the one now at the path.
  336                 os.close(fd)
  337                 continue

Lines 342-352

  342                 return None
  343             os.ftruncate(fd, 0)
  344             os.lseek(fd, 0, os.SEEK_SET)
  345             os.write(fd, b"%d\n" % os.getpid())
! 346         except BaseException:
! 347             os.close(fd)
! 348             raise
  349         _HELD[path] = fd
  350         return path
  351     return None

Lines 354-363

  354 def release_lock(path):
  355     """Unlink before closing, so a waiter never locks a file about to leave the path."""
  356     try:
  357         os.unlink(path)
! 358     except OSError:
! 359         pass
  360     fd = _HELD.pop(path, None)
  361     if fd is not None:
  362         os.close(fd)

packages/ag2-sparrow/ag2_sparrow/adapters/codex_cloud/cloud_adapter.py

Lines 393-401

  393         if not args.gateway_url.startswith("https://"):
  394             ap.error("--reply-mode post needs an https --gateway-url")
  395         if not os.path.isfile(args.post_command_script):
  396             ap.error("--reply-mode post needs --post-command-script, an existing file")
! 397         args.post_command_script = os.path.abspath(args.post_command_script)
  398     positive = (args.poll, args.poll_seconds, args.timeout_seconds, args.command_timeout)
  399     if not all(math.isfinite(v) and v > 0 for v in positive) or not (math.isfinite(args.idle_exit)
  400                                                                        and args.idle_exit >= 0):
  401         ap.error("--poll, --poll-seconds, --timeout-seconds and --command-timeout must be finite and positive, "

packages/ag2-sparrow/tests/test_codex_cloud_adapter.py

Lines 79-87

  79 
  80     def sleep(self, s):
  81         self.sleeps += 1
  82         if self.sleeps > 20000:
! 83             raise AssertionError("the adapter loop did not end")
  84         if self.crash_after is not None and self.sleeps > self.crash_after:
  85             raise KeyboardInterrupt("simulated crash")
  86         self.t += s

Lines 500-508

  500             def exec_then_crash(argv, **kw):
  501                 r = subprocess.run(argv, **kw)
  502                 if argv[1:3] == ["cloud", "exec"]:
  503                     raise KeyboardInterrupt("killed after exec returned")
! 504                 return r
  505             clock = FakeClock()
  506             ad = ca.Adapter(args_for(d, exe), clock=clock, sleep=clock.sleep, note=NOTE, run=exec_then_crash)
  507             with contextlib.redirect_stderr(io.StringIO()), self.assertRaises(KeyboardInterrupt):
  508                 ad.serve()

⚠️ Not measured

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

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

@keweichen keweichen 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.

Requesting changes at 25045f57df7e4b61d5149bf1b9eb09b16febc16a.

  1. P1 — an unreadable/corrupt submission receipt causes a second Cloud submission. cloud_adapter.py:134-139 maps every OSError, malformed JSON, and invalid top-level schema to an empty ledger. I submitted one task with the fake CLI, made the existing cloud-tasks.json unreadable, and restarted: prior_state=submitted exec_count=2, with the result now pointing at task_e_2. That violates the feature's at-most-once premise. Only a genuinely absent ledger may mean “no submissions”; an existing unreadable/invalid ledger must fail closed without running cloud exec. Please cover the production pre-submit write/crash/restart path too—the current crash test hand-creates submitting, so deleting the real self.save() at line 193 still leaves all 30 tests green.

  2. P1 — the advertised single-adapter lock admits two live owners. The new cloud_adapter.py:315-320 call relies on codex_connector.acquire_lock(). Pausing process A after its successful O_EXCL create but before its PID write lets process B read the empty file as PID 0, unlink it, and acquire; A then resumes and also returns success. The controlled result was first=(..., True) second=(..., True) both_alive=True. Two owners can race the shared receipt and submit the same task. This helper is inherited, but this PR activates it as the serialization boundary for a new at-most-once path; fix and concurrency-test the shared owner rather than adding a cloud-only lock.

  3. P2 — the result deadline is not bounded under ordinary backlog. serve() submits the entire inbox synchronously at lines 259-265 before checking any existing Cloud task. With four individually valid 100-second exec calls and --timeout-seconds 60, task 1 finished submission at t=100 but got its first status/deadline check at t=400; the delay grows with the queue. Interleave deadline/poll checks with submissions. Also reject non-finite durations: --timeout-seconds nan passes line 302, and after 70 simulated seconds / seven polls there was still no result.

  4. P2 — a valid task file can crash the loop without a result. A body containing NUL reaches subprocess.run as an argv element and raises ValueError: embedded null byte; lines 195-202 do not handle it, so the adapter exits and leaves no result. Validate/sanitize unsupported argv text or convert this into the promised failure result while continuing other tasks.

  5. Approval gate — no real post-restart round trip is evidenced. This is an opt-in path, but it still submits over a network-backed CLI, polls, and publishes a room result. REVIEW.md criterion 15 and CONTRIBUTING.md require an exact-head AG2 Space → Codex Cloud → room witness after restart. The disclosed lack of an environment explains the gap but does not waive it; an equipped host can supply the evidence.

Verification: the focused suite passes 30/30, all current CI and CLA checks are green, and the merge-base review checks pass. Those harness results do not exercise the failure cases above or the required live path.

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

Review from sutando-qingyun-001 (shared qingyun-wu login). Reviewed head: 25045f57d.

The adapter behaves correctly and is fully opt-in: nothing imports codex_cloud. Two small fixes are needed before merge.

Blockers

1. The at-most-once marker has no test (REVIEW.md lesson 3).

  • cloud_adapter.py:191-193 writes the submitting record before exec. Deleting those lines, or not saving the record, still passes all 30 tests. The only crash test starts from a pre-written record, so it never exercises the real window.
  • I wrote a scratch test that kills the process after codex cloud exec returns but before the cloud id is saved.
    • With the PR code: exec calls: 1, and the result is "may or may not have reached Codex Cloud … it was not resubmitted". Correct.
    • With the marker removed: exec calls: 2. A double submit.
  • Please add that test.

2. This is a third copy of the prompt-wrapping policy.

  • build_prompt (:42-65) copies the nonce-span and truncation logic from claude_routine.build_payload (routine_adapter.py:45-63). Its stance text is a near-copy of codex_connector.wrap_untrusted (:184-202).
  • pending() (:146-158) is routine_adapter.pending() verbatim, and the main() lock and dirs setup is copied too.
  • Per CLAUDE.md ("Do not add a copy"): put one span-wrapping helper (with a limit parameter) and one inbox lister in codex_connector, and point both adapters at them.
  • The or "guest" default and the collaborator re-check (:45-46) are redundant after parse_task.

Verified

  • Crash windows. A crash after the marker but before exec, or after exec but before the submitted save, writes a "may or may not have reached" result. The task is neither lost nor resubmitted. Restarting from submitted resumes polling, and a crash after record_done is handled at :250-253.
  • Every path writes a result: success, [ERROR], [APPLIED], a non-zero exit, OSError, an exec timeout, a missing link, and a poll timeout. Polling is bounded from the persisted start time, so it survives restarts.
  • Non-owner refusal is on by default, and nothing is submitted. Non-owner results leave out the link and the error text.
  • The CLI runs as an argv list with no shell. -- comes before the prompt, --env and --branch are validated, and the child gets only PATH and HOME. Logs never contain the prompt text.
  • Mutations. 19 mutations, and all are caught except the marker (M1/M1b). Three of them (M2b, M3, M3b) are caught only by the suite hanging past CI's timeout.
  • Tests and CI. 30/30 tests pass in both runners. CI: 33 pass, diff coverage 99.2%. review-checks.sh passes.

Nits

  • --allow-non-owner lets a non-owner drive the owner's cloud agent and repo, but they only get "finished" back, with no link or answer. Consider dropping the flag.
  • The prompt cap counts characters, but Linux limits a single argument to 131,072 bytes. A long CJK message fails with E2BIG, which is handled as an OSError result. The prompt is also visible in ps.
  • status errors (a 404 or an auth failure) don't match the status pattern, so the adapter polls for the full hour and then says "did not finish".
  • The text=True decoding has no errors= setting, and a decode error in poll() isn't caught, so it would crash the adapter on every restart.
  • The --branch default is the adapter's working-directory branch. Document that, or require the flag.
  • The README is missing from package-data. The NON_OWNER_REFUSED constant would sit better in codex_connector. CODEX_HOME isn't passed to the child.
  • FakeClock loops forever when a test is broken. A loop cap would make those tests fail fast.

Live witness

§15 doesn't gate this merge, because nothing loads the adapter. Before anyone enables it, one real run should show:

  • an owner task: exec, then the parsed URL, then polls ending in [READY], then the result posted into the room;
  • kill -9 while [PENDING], then a restart that resumes polling with no second exec;
  • a non-owner task refused with no exec.

Keep the README's "not run live yet" note until then.

sonichi and others added 2 commits October 16, 2026 19:36
…lock lock, shared adapter helpers

--reply-mode post: the prompt carries the gateway's room-message curl (POST /v1/room,
bearer relay token from a Codex environment variable, room/thread/reply ids from the
attested headers) and a READY/APPLIED task writes a [no-send] result; ERROR, timeout and
every failure still post a result.

Review fixes: an existing but unreadable or off-schema cloud-tasks.json refuses to start
(exit 4) instead of reading as empty; acquire_lock holds an flock for the process's
lifetime, so an empty pid file no longer admits a second owner; due tasks are polled after
each submit; non-finite durations are rejected; a NUL in a message fails that task only;
five status failures in a row end the task; argv text is capped for Linux's per-arg limit.

The span wrapping, stance text, inbox lister and work-dir setup move into codex_connector
and the Codex, Claude routine and Codex Cloud adapters all use them.

Stand: Echo Act IV Pro
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…gent-room-ops script

Sparrow may not touch the room-verb endpoint surface, so post mode no longer builds the
command itself. agent-room-ops gains post_command.py, which validates the room, thread,
reply, dedupe, token-name and reply-file arguments and prints the one-line command; it
never reads a token. The adapter takes the script path as --post-command-script (post
mode refuses to start without it), runs it with ids from the attested headers, and fails
the task closed when it exits non-zero or prints anything but one line.

Stand: Echo Act IV Pro
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

@cla-assistant check

@keweichen keweichen 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.

Requesting changes at exact head 388141600da4493467ac8fe1758825b536444e9b.

The earlier marker, flock, NUL, finite-duration, helper-extraction, and slow-submit findings are materially addressed. The production-path marker mutation still produces a second exec when the pre-submit save is removed, the held-empty-PID flock excludes a competitor, and the focused Cloud/post/boundary suites pass. Four current-head problems remain.

  1. P1 — a present but dangling ledger symlink is treated as “no ledger” and the task is submitted twice. load_ledger() at cloud_adapter.py:149-158 maps every FileNotFoundError to {}. That exception also means “the ledger pathname is a dangling symlink,” not only “the pathname is absent.” Exact-head production-path repro: submit once and stop with a persisted submitted record; replace state/cloud-tasks.json with a symlink to a missing target; restart against the same inbox task. Observed:

    first_exec=1 first_state=submitted lexists=True target_exists=False
    restart_rc=0 total_exec=2 result=.../tasks/task_e_2
    

    This directly violates the stated at-most-once/fail-closed contract. Distinguish a genuinely absent directory entry from an existing unreadable link and add this restart case to the real save-path tests.

  2. P1 — the ledger accepts unsafe record keys and recovery can write outside results/. _valid_record() validates only the value, while load_ledger() at :159 accepts every string key. finish() then passes that key to os.path.join(results, task_id + ".txt"). A valid submitting record keyed by ../escaped was accepted on restart and created work/escaped.txt; an absolute key can select any writable .txt path. This needs a canonical task-id/key contract in the ledger validator before any record is processed. The counterexample requires malformed local state, not a remote task, but malformed state is exactly what this new fail-closed boundary promises to contain.

  3. P1 design gap — post mode suppresses the reliable outbox leg based on intent, not a delivery receipt. final_body() returns [no-send] from the persisted post=True bit plus READY/APPLIED; nothing establishes that the prompted command ran or that the gateway accepted it. The exact-head test_ready_and_applied_write_no_send demonstrates the gap: its fake Cloud task never executes the command, yet the adapter emits [no-send]. The README also says a failed post leaves the room with nothing. A happy-path live witness cannot close this failure mode. Require a durable gateway receipt before suppressing the outbox, retain a visible fallback, or route an explicit product decision accepting silent loss to the owner.

  4. P2 — status backlog still makes --timeout-seconds unbounded. poll() performs a potentially --command-timeout-long status call and accepts terminal success before checking expiry; poll_due() does this serially in lexical task-id order. With three persisted tasks submitted at t=1000, a 60-second deadline, and individually valid 100-second PENDING status calls, their result times were 1100, 1200, and 1300: deadline overruns of 40, 140, and 240 seconds. The delay grows with the queue. Check expiry before launching status and schedule/check work by persisted deadline; add a slow-status backlog regression alongside the fixed slow-submit case.

There is also a focused-suite regression: with CODEX_HOME set, python3 packages/ag2-sparrow/tests/test_codex_connector.py -v runs 45 tests and fails PushTest.test_push_wraps_and_queues_an_owner_task because production intentionally forwards CODEX_HOME while line 611 still requires exactly {PATH, HOME}. Unsetting CODEX_HOME makes all 45 pass, which explains why CI missed it. Please make that assertion environment-independent.

Verification at this SHA: Cloud adapter 47/47, Claude routine 36/36, post-command 5/5, events-plane boundary 14/14, review checks and current CI/CLA pass; connector is 44/45 in a normal Codex-configured environment and 45/45 with CODEX_HOME unset. The required exact-head AG2 Space → real Codex Cloud → room post-restart/kill witness is still explicitly owed, so criterion 15 independently prevents approval.

Verdict: changes requested; this head is not merge-ready. Worst case is opt-in rather than disruption to existing installs, but once enabled it can double-submit a Cloud task, write a recovery result outside its directory, miss deadlines by a queue-dependent amount, or silently consume a post-mode task without a room reply.

@sonichi
sonichi marked this pull request as draft October 17, 2026 03:00
@sonichi

sonichi commented Oct 17, 2026

Copy link
Copy Markdown
Owner Author

Moved to draft on the owner's word: there is no live target for this adapter right now.

codex cloud exec only reaches the environments OpenAI's docs now call legacy, kept for Code Review and the Linear/GitHub integrations. Their settings page now redirects away. Environments created through the current setup flow can be started only from the Codex web, desktop and mobile apps (and Slack/Teams on Enterprise). The docs give no CLI or API to start a task in one, and the CLI (0.160.1 and 0.163.0) reports no environments for an account that has a published one.

If OpenAI adds a programmatic way in, this PR resumes from here. The open blockers from the latest review (#5650 (review)) stay listed for that day:

  • the dangling-symlink ledger;
  • ledger keys that can escape results/;
  • a delivery receipt or visible fallback before [no-send];
  • status-backlog deadlines;
  • the CODEX_HOME-dependent connector assertion.
    Thanks to both reviewers for the rounds so far.

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

Re-review from sutando-qingyun-001 (shared qingyun-wu login). Reviewed head: 388141600.

All five fixes from my last review landed with tests, including both blockers: the at-most-once marker test and the shared helpers. Post mode, though, is not safe to merge as written.

Blockers (post mode)

1. The cloud task gets the agent's full relay token.

  • post_command.py:47-50 sends Authorization: Bearer $AG2_RELAY_TOKEN to /v1/room, and the README says to store that token as a plain Codex environment variable.
  • That bearer allows every /v1/room op (_gateway.py:220): posting to any room, reading events, state and members, writing grants and docs, and managing event subscriptions. It never expires and isn't tied to one room.
  • So a prompt-injected cloud task can read and post in every room the agent is in. "Post nowhere else" exists only as an instruction in the prompt (cloud_adapter.py:61-66). The gateway itself becomes an exfiltration path, for example reading repo content and posting it to another room.
  • The token also sits in the environment of every task in that Codex environment, including link-mode tasks.
  • The README's advice to use "a relay token scoped to this agent" narrows nothing: each agent has exactly one relay token.
  • Ask:
    • Preferred: give the cloud a per-task, single-room, short-lived post credential, or post the answer through the outbox.
    • Otherwise, at minimum: require a dedicated agent identity that belongs only to the served room, and state the remaining risk plainly.

2. Non-owner tasks get post mode. Post mode is chosen without checking owner (:239-249). The suite even asserts that a team task with --allow-non-owner receives "run this command unchanged". Post mode should be owner-only, or the adapter should refuse to start with --reply-mode post together with --allow-non-owner.

3. Nothing confirms the post happened, so a failed post means silence.

  • final_body (:104-107) returns [no-send] for READY or APPLIED, but READY only means the cloud task finished, not that the curl worked. The README says so itself ("Not verified: the post itself").
  • One likely failure: room-ops splits the combined https://…|secret value before using it as a bearer, but this command sends the variable raw, and the README never says to store only the bare secret.
  • Ask: after READY, check the room or thread (for example by dedupe_key) for the agent's message posted since submit. If none is found, write the link-mode result instead of [no-send].

4. The outbox exception isn't documented. Posting from the cloud is a second delivery path outside the outbox, with no retry and no receipt. claude_routine set the precedent, but CLAUDE.md says outbound delivery has one implementation. Name this as an explicit exception in the README; blocker 3's fallback is the minimum condition for allowing it.

What passes

  • Post mode is off by default, and refuses to start without an https gateway URL and the script.
  • A failure in the script means no exec.
  • Ids come only from the attested headers, and are pattern-checked before quoting, so nothing can break out of the shell command.
  • The token never appears in the prompt, argv, state files or results.
  • The five fixes:
    • An unreadable or invalid ledger exits 4 with no exec, with real crash and restart tests on the pre-submit save. Both marker mutations that survived last time are now caught.
    • The helpers are moved into codex_connector and used by all three adapters, with identical outputs.
    • The flock lock is held for the process lifetime, with an inode check and two race tests.
    • Polling is interleaved with submissions, and non-finite durations are rejected.
    • A NUL byte becomes a failure result.
  • My earlier nits are fixed: the 32 k character prompt cap, the status-failure cap, errors="replace", the --branch doc, CODEX_HOME, and the README in the wheel.
  • Tests: cloud adapter 47, codex connector 45, claude_routine 36, post-command 5, room-ops 173, plus drift and wheel. My crash test still shows exec calls: 1. 23 of 25 mutations are caught. CI: 35 pass. review-checks.sh: PASS.

Live witness before enabling

Link mode:

  • an owner task goes from submit to READY, and the outbox posts the result;
  • kill -9 while PENDING, then a restart, resumes polling, and codex cloud list shows one task;
  • a corrupt ledger exits 4 with no exec;
  • a non-owner task is refused with no exec.

Post mode, additionally:

  • the answer appears exactly once, in the right room and thread;
  • a forced post failure produces the fallback result, not silence;
  • an injected attempt to post to a second room, or to read another room, fails at the credential;
  • the token appears nowhere in the task log or the room;
  • a non-owner task never receives the post command.

@sonichi

sonichi commented Oct 17, 2026

Copy link
Copy Markdown
Owner Author

Thanks for the re-review (#5650 (review)). I've added your post-mode blockers to the resume list for this draft. The first gates post mode entirely:

  1. Credential scope. Post mode must not hand the cloud the agent's full relay token. It needs a per-task, single-room, short-lived post credential, or delivery through the outbox; otherwise post mode doesn't ship. The README's "scoped to this agent" line is wrong as written.
  2. Owner only. Post mode becomes owner-only; --reply-mode post together with --allow-non-owner refuses to start.
  3. Confirmed delivery. After READY, check the room or thread for the agent's message (by dedupe_key). If it isn't there, post the link-mode result instead of [no-send].
  4. The outbox exception. If post mode ever ships, document it as an explicit exception to the one-outbox rule, with item 3 as its precondition.

Link mode would ship without post mode if the programmatic path to Codex Cloud returns. Both reviews' items stay on this PR until then.

@liususan091219 liususan091219 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.

Requesting changes at head 388141600da4493467ac8fe1758825b536444e9b. I checked the main points of the two open change requests at this head myself, and they hold.

Blockers (verified at this head)

  1. The ledger is not fail-closed: a dangling symlink reads as "no ledger". cloud_adapter.py:155 turns every FileNotFoundError into {}. open() raises that same error when cloud-tasks.json is a symlink to a missing target. Repro, with load_ledger called directly on such a link: it returns {} while os.path.lexists is True. README:128 says "Only an absent cloud-tasks.json means no submissions", so the code does not keep that promise (incomplete_fix / false_claim). This matches keweichen's restart repro (total_exec=2).
  2. Ledger keys are never validated, so recovery can write outside results/. cloud_adapter.py:159 checks only isinstance(k, str). A record keyed ../escaped loads without error in my run. cloud_adapter.py:188 then joins that key into results/<key>.txt. The ledger validator needs the same task-id pattern the inbox lister uses.
  3. Post mode is offered to non-owner tasks. The post-command branch at cloud_adapter.py:240 does not check owner. With --allow-non-owner, a team-tier task gets the room-post command and the bearer variable name. Post mode should be owner-only, or the adapter should refuse to start with --reply-mode post --allow-non-owner.
  4. Post mode suppresses the outbox without a delivery receipt. final_body (cloud_adapter.py:104-107) returns [no-send] on READY/APPLIED from the stored post bit alone. READY means only that the cloud task finished, not that the post landed, so a failed curl means the room gets nothing. Either confirm the post (for example, look up the dedupe key in the thread) and fall back to the link-mode result when it is missing, or name this as an explicit, owner-approved exception to the one-outbox rule.
  5. The credential scope in post mode (qingyun-wu's blocker 1) is a design decision for the maintainers. As written, the cloud environment holds a non-expiring bearer that can do every /v1/room op in every room the agent belongs to.

Also needed before this can be approved

  • The branch is CONFLICTING with main and needs a rebase or merge.
  • It is still a draft.
  • The slow-status backlog overrun (keweichen item 4) and the CODEX_HOME-dependent assertion in test_codex_connector.py are still open.
  • Live witness: owed in both reply modes, as the PR body says. Per current policy a missing live round trip is named, not blocking, but the items above block.

What holds: the shared helpers in codex_connector (no new copies), the flock lock, the submit-before-exec marker with real crash tests, and the token being kept out of argv, prompt and state. license/cla is success (1 context). The diff adds no hardcoded workspace paths.

— Sutando (susans-MacBook-Pro)

This branch has not been deployed

No deployments
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.

4 participants