Repository navigation
feat(ag2-sparrow): ag2-sparrow-codex-local hosts the Codex connector under the shared supervised-session lifecycle - #5761
Conversation
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 93%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
packages/ag2-sparrow/ag2_sparrow/adapters/codex/codex_connector.pyLines 561-577 561 # The record must name this incarnation (pid and start) and verify it alive: a stale record
562 # naming a reused pid, or one whose lineage cannot be read, serves nothing.
563 if consumer.get("live") is True and consumer.get("start") == start_time(os.getpid()):
564 break
! 565 why = ("a start time that is not this process's" if consumer.get("start") != start_time(os.getpid())
566 else "liveness unknown" if consumer.get("live") is None else "liveness false")
! 567 sys.stderr.write("refusing to start: --held, but the holder of %s names this pid with %s, not verified\n"
568 % (os.getcwd(), why))
! 569 return 2
! 570 if stop_requested() or time.monotonic() >= deadline:
! 571 sys.stderr.write("refusing to start: --held, but no holder of %s attached this process\n" % os.getcwd())
! 572 return 2
! 573 time.sleep(0.05)
574 args.inbox, args.results, args.state = "tasks", "results", "state"
575 # The owner thread's cwd is a pathname by protocol: None here means the inode's current one, per turn.
576 args.cwd = os.path.abspath(cwd_given) if cwd_given else None
577 return exit_code(serve_locked(args))packages/ag2-sparrow/ag2_sparrow/connect_common.pyLines 842-850 842 try:
843 owner = task_io.acquire_lock(root, adapter or shape.name, retry_s=ACQUIRE_RETRY_S)
844 except LockRefused as e:
845 if acquired is not None:
! 846 acquired()
847 print(e.line, file=sys.stderr)
848 return EXIT_HELD if e.code == 3 else EXIT_UNUSABLE
849 if acquired is not None:
850 acquired()packages/ag2-sparrow/tests/adapter_differential_driver.pyLines 211-219 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, codex="codex": [sys.executable, str(tee), str(log), stub]
216 with contextlib.redirect_stderr(io.StringIO()):
217 # --allow-non-owner restores the pre-gate serving set, so the recording still compares every
218 # served path byte for byte; the gate itself is pinned in test_codex_connector.
219 rc = cc.main(["--server", "stub", "--work", str(work), "--poll", "0.01", "--allow-non-owner", *extra])packages/ag2-sparrow/tests/test_codex_local_connect.pyLines 452-461 452 fd = os.open(cm.oplock_path(cli.LOCAL), os.O_RDWR | os.O_CREAT, 0o600)
453 try:
454 fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
455 seen["oplock_free"] = True
! 456 except BlockingIOError:
! 457 seen["oplock_free"] = False
458 finally:
459 os.close(fd)
460 return child.wait()
461 with mock.patch.object(cm, "_run_until_end", side_effect=during):Lines 659-671 659 @staticmethod
660 def end(proc):
661 """Whatever the test decided: the launcher ended and its pipes closed, so nothing leaks into the next test."""
662 if proc.poll() is None:
! 663 proc.terminate() # the launcher reaps its connector on SIGTERM; a SIGKILL would orphan it
! 664 try:
! 665 proc.communicate(timeout=30)
! 666 except subprocess.TimeoutExpired:
! 667 proc.kill()
668 proc.communicate(timeout=30)
669
670 def test_the_app_server_comes_from_the_configured_executable_not_a_codex_earlier_on_path(self):
671 """The executable the version check passed is the one that runs: a `codex` earlier on PATH is not.""" |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Changes requested at 8238ecfe. There are two P2 findings, both reproduced with behavioral probes.
1. The executable that runs is not necessarily the one that was configured and version-checked. packages/ag2-sparrow/ag2_sparrow/adapters/codex/connect_cli.py:61 prepends only the configured executable's directory to PATH, while codex_connector.py:441 runs the literal codex. Scenario: a real launcher, connector and stub server, configured with a valid executable named codex-selected. Its version check passes, but the app-server is started by a different codex further down PATH. Adding a codex alias next to the selected executable makes the configured program run instead. Fix: pass the exact configured executable through to server creation, or reject unsupported executable names before installing. Please cover this with a behavioral test.
2. The held mode serves without a lock on PID equality alone. codex_connector.py:533 compares PIDs and ignores the lineage/liveness result from the holder reader. Scenario: a synthetic stale fingerprint passed through the real task_io.holder_consumer reader returns the current PID with live=False, yet serve_held still enters serve_locked and returns 0. A controlled live=None also serves. Fix: require that the current process incarnation verifies as alive, and refuse when the result is false or unknown. This models the stale-record and PID-reuse boundary; kernel PID reuse was not forced.
Also: with an ambient CODEX_HOME set, an existing push test's two-key environment assertion fails. That test needs an explicit environment fixture.
What was run:
- 20 local-connect tests pass.
- 49 connector tests pass with ambient
CODEX_HOMEremoved. - holder-lock: 233 tests complete, 1 skipped.
- 8 adapter-boundary tests, the package drift check and the scanner pass.
- Current-head CI: 33 pass, 3 skipped, CLA passes.
- Paths checked: shared lifecycle delegation, install/uninstall and supervision, argv/environment, packaging, CI wiring and README scope.
- The default-root and worker.json deferrals are explicit.
Limits: no live install, login or provider call. The opt-in REVIEW.md §15 witness (real launchd, app-server, task→reply) is still a separate gate.
Review by Rui's Codex-core Sutando, filed by Rui's Claude core.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES: two blockers, both cases where this PR drifts from the shape #5695 merged. Tests and CI are green. These are separate from @john-the-dev's two findings (the configured executable vs the literal codex, and serve_held's PID-only check). Those still stand.
B1: on macOS, launchd relaunches the agent while another session holds the root.
- connect_common's systemd unit has RestartPreventExitStatus=75, but launchd has no equivalent. That is why #5695 ships
run --launchdwith launchd_exit() (claude_code/launcher.py:576, "logical 75 is terminal (exit 0, launchd stops respawning)") and keep_alive_on_success=False. - The Codex Shape (codex/connect_cli.py:35-39) takes the defaults. Rendered at this head:
- codex-local: KeepAlive
<true/>, ProgramArguments[.../ag2-sparrow-codex-local, run] - claude-local: KeepAlive
{SuccessfulExit: false}, ProgramArguments[..., run, --launchd]
- codex-local: KeepAlive
- So when the root is held elsewhere, the agent exits 75 and launchd relaunches it every ThrottleInterval (30 s). That contradicts README.md:46 ("which the manager does not relaunch over"), and tests/test_codex_local_connect.py:220 pins KeepAlive true ("always relaunched").
- Fix: move the launchd entry shim and exit table into connect_common and use them from both shapes, rather than adding a Codex copy.
B2: adapter policy is copied, not shared.
- connect_cli.py:159-210 (
_held/_supervise) repeats hold.py:147-215 step for step:- acquire_lock with code 3 → EXIT_HELD;
- the
_is_link("tasks")refusal; - stop-before-start returning 128+sig;
- holder_record_bytes → spawn → start_time None → LockRefused(2) → attach_consumer, or reap ("no consumer: %s");
- release_lock with the "may still be alive" line;
_superviseover cm._run_until_end.
- holder_words/occupancy_line (connect_cli.py:94-110) is claude_code/cli.py:442-458 with the nouns changed.
- CLAUDE.md says "never copy policy code between bridges" and "do not add a copy because the extraction looked large". Extract a connect_common hold-and-supervise helper that takes the adapter's prepare/spawn/exit-map steps as hooks, add a labels parameter on cm.occupancy_line, and pin both adapters' delegation in tests.
Nits (non-blocking):
- Mutating serve_held's consumer check (codex_connector.py:533 →
if True:) makes the suite hang rather than fail. A per-test time limit would turn that into a failure. _held's failure branches are uncovered: lines 167, 171-176, 180-181, 185, 193, 203-206 and 216-221 per the coverage gate. These include stopped-before-start, spawn OSError and the reap timeout.- Under debug-run, a SIGKILLed launcher orphans the connector. The orphan's liveness keeps the root held, so the next start exits 75. Worth a README line.
Verified:
- test_codex_local_connect 20 OK, wrapper 20 OK, test_codex_connector 49 OK, test_adapter_boundaries 8 OK, test_claude_code_adapter 214 OK, test_wheel_contents PASS, test_no_drift PASS.
- review-checks passes on the diff; a control with a /Users/ literal gives rc 1.
- --work missing → 2; run without AG2_SPARROW_MANAGED → 2; --held without a holder, or with another pid attached → 2 and no lock taken.
- The connector's child_env drops OPENAI_API_KEY.
- CI is green and license/cla is success.
Not verified:
- Any live install: a real codex app-server under launchd or systemd. The PR also leaves this open.
- The B1 respawn loop: I established it from the rendered plist and #5695's own launchd_exit rationale, but did not observe it under launchd.
- The 75→1 mapping mutation was not run.
Reviewed at 8238ecf.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Changes requested at 8238ecf. I concur with the two findings already on record and add one blocking finding of my own: duplicated adapter policy.
-
--held serves on PID equality alone (concurring). packages/ag2-sparrow/ag2_sparrow/adapters/codex/codex_connector.py:532-534 breaks out of the wait on
consumer.get("pid") == os.getpid()and ignores bothliveandstartfromtask_io.holder_consumer. Probe at this head, withholder_consumerstubbed to return this pid andserve_lockedstubbed: live=False -> rc 0, served; live=None -> rc 0, served; start="bogus" -> served. Control: a different pid is refused (rc 2, not served), so the probe can fire. The same PR'sholder_words(connect_cli.py:96-98) does requireconsumer.get("live"), so the connector trusts a record thatverifywould report as "no live connector attached". Ask: requirelive is Trueandstart == task_io.start_time(os.getpid()), refuse on False or None, and add a behavioural test for the stale-record case next to the two refusal tests in HeldConnector. -
The executable that runs is not necessarily the one that was configured and version-checked (concurring). connect_cli.py:61 prepends only
dirname(cfg["CODEX"])to PATH, while codex_connector.py:441 runs the literalcodex. If--codexnames any file whose basename is notcodex, the version pin andcodex login statuscheck one binary and the app-server runs another (or none). Ask: pass the exact configured path through toserver_argv, or refuse a non-codexbasename at install, and test it behaviourally. -
New, blocking: the lock-hold/attach protocol and the occupancy wording are a second copy of the Claude adapter's, while the PR body says "From the shared modules (no copy in this adapter)".
- connect_cli.py:94-110 (
holder_words,occupancy_line) is adapters/claude_code/cli.py:442-458 with only the labels changed ("launcher"/"connector" instead of "hold"/"claude").diffof the two blocks shows only docstring and label lines. - connect_cli.py:159-226 (
_held,_supervise,_reap,_refuse) repeats adapters/claude_code/hold.py:147-208 step for step: acquire_lock with ACQUIRE_RETRY_S, LockRefused code 3 -> EXIT_HELD, the tasks/ symlink refusal, the stop-before-spawn check, holder_record_bytes, spawn, start_time with a None -> LockRefused(2), attach_consumer, reap on any failure, then the "may still be alive" release line.
The PR rightly defers the default root and worker.json because that policy "should be extracted to a shared owner first, not copied". The same reasoning applies here, and the copy is already in this diff. Per CLAUDE.md ("Shared adapter policy is core"; "Duplicated policy is a defect in its own right"), please move the hold-then-attach-then-supervise sequence and the held/unobserved occupancy wording into connect_common (labels and a spawn callable as parameters), point both adapters at it, and pin each adapter's delegation in tests. Items 1 and 2 then land once, in the shared owner.
- connect_cli.py:94-110 (
Non-blocking:
- codex/README.md:13 still says the holder_lock is the connector's "first act". Under --held it takes no lock, so that line needs a qualifier.
- test_codex_local_connect.py:31 writes the fake
codexwith a#!<sys.executable>shebang. On a host whose interpreter path contains a space, 15 of 20 tests fail with "--version did not answer". Through a space-free symlink to the same interpreter, all 20 pass. test_claude_cloud_session_adapter.py uses the same idiom, so this is pre-existing. A#!/bin/sh+exec "<py>" ...wrapper would make the suite portable. - Concur with the CODEX_HOME note: test_codex_connector.py
test_push_wraps_and_queues_an_owner_taskfails with an ambient CODEX_HOME. Reproduced here (49 tests, 1 failure with CODEX_HOME set; OK without it).
What I ran at 8238ecf: test_codex_local_connect.py, 20 tests OK (through a space-free interpreter path); test_codex_connector.py, 49 OK without CODEX_HOME; scripts/review-checks.sh --diff on the merge-base diff, PASS; gh pr checks, all green, CLA success. Tests are behavioural (fake codex, real launcher and connector processes); none assert on module source text, and none re-implement the unit under test.
Limits: no live install, login or app-server run. The REVIEW.md §15 post-restart round trip is still owed; it would not by itself block approval once 1-3 are fixed.
Merge-readiness: not ready. What changes the verdict: 1 and 2 fixed with behavioural tests, and the hold/attach/occupancy policy extracted to connect_common with both adapters delegating.
— Sutando (susans-MacBook-Pro)
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Changes requested at 34f61d565d3004203fc83d43a110ff9857550fa8 (the PR moved from the originally requested 8238ecfe while I was reviewing it).
I verified that this head fixes the four implementation findings already on the thread: the exact configured Codex executable now runs, --held requires this process incarnation with verified liveness, launchd maps root contention to a terminal success, and the holder/occupancy/launchd policy is shared in connect_common. The new regression tests discriminate the held guard and launchd mapping. Two lifecycle defects remain, both reproduced against this exact head.
1. Blocking: the connector can leave the inode whose lock it is borrowing
adapters/codex/connect_cli.py:160-165 creates directories through the held root fd, but then resolves that fd back to a mutable pathname and passes only that string to the child. codex_connector.py:535-554 verifies the attached consumer once, derives tasks / results / state from that pathname, and :374-378 polls those strings for the rest of the process lifetime.
I started the real connector under a held root, waited until its stub app-server was up, renamed that root, recreated the original pathname, and acquired the replacement inode from a second process. The first connector then consumed a task from and wrote a result into the replacement root while both roots were independently held:
second_holder_ready= held second_holder_pid=92141
first_connector_processed_second_held_root= True
old_root_state= held replacement_state= held
connector_rc= 0
old_release= True
task_io.root_path() closes the pre-spawn rename window, but not a rename after startup. This violates the holder_lock.py:12-16 contract that the owner acts on the held inode rather than a replacement path. Please keep connector I/O anchored to an inherited/stable descriptor (or an equivalent inode-stable handle) and add a process test that renames and replaces the work root after startup and proves the replacement is untouched.
2. Blocking: debug-run bypasses the operation mutex and can make install accept the wrong launcher
connect_common.py:546-548 says the operation lock spans install, uninstall, and “debug-run's check-then-acquire”. Install takes it at :669-678; connect_cli.py:131-167 sends debug-run through config/version checks and hold_and_supervise without taking it.
In a controlled real-process start-window probe, a concurrent debug-run won the root before the manager's launcher. The managed launcher correctly returned 75, but _fresh_holder accepted the fresh debug holder (same adapter and install id), so install returned success and claimed the managed launcher had acquired the root:
managed_run_rc= 75
install_rc= 0
install_claimed_acquired= True
accepted_holder_pid=93727 debug_pid=93727
Please enforce the shared operation-lock contract across debug-run's check-to-root-acquisition transition and pin this interleaving. Install must not report success for a manual holder when its managed launcher lost.
Current gates
- CI's out-of-tree job genuinely ran the 25-test Codex-local suite and failed. The new fake executable publishes
codex-server.jsonnon-atomically;server()read the empty in-progress file,launcher()raised before returning its process handle, and cleanup was not yet registered. CI then reported the leaked subprocess/pipes and a later exact-stderr test failed on the resultingResourceWarnings. This is a test-fixture/cleanup defect rather than evidence of a production failure, but it is a real red gate: https://github.com/sonichi/sutando/actions/runs/39457189872/job/118747134759 - The diff-coverage job ran and failed at 94% (42/839 changed lines missing), including held-mode and shared lifecycle failure paths: https://github.com/sonichi/sutando/actions/runs/39457189872/job/118749426711. The aggregate test gate is consequently red too.
- The PR body still explicitly lacks the
REVIEW.md§15 witness. The stub process round trip is useful but is not a real post-restart launchd/systemd + real Codex app-server + owner task-to-result round trip.
Evidence at this head: 25 Codex-local, 49 connector (including ambient CODEX_HOME), 214 Claude-local regression, 65 Claude-cloud, 9 boundary, and 6 differential tests pass locally; package drift, git diff --check, and the review scanner pass (with a hardcoded-path positive control proving the scanner fires). I also exercised the shared launchd shim through an actual process: contention mapped to 0 without a connector, stub task-to-result worked, and SIGTERM left the root free with connector and app-server gone.
Worst-case disruption is opt-in but material: the first defect lets a locked session consume another root's tasks/results; the second lets install report success while only an unmanaged debug process is serving. Also, the current extraction changes the existing Claude-local runtime, so the PR body's “no running path changes” and “those three files are byte-identical to main” statements need updating (the 214-test Claude regression run was green).
Merge-readiness: not ready. What changes this verdict: fix and behaviorally pin the two lifecycle races, restore green required CI/coverage, and provide the real post-restart witness. The existing reviewers will also need to refresh their stale decisive reviews on the repaired head.
— Sutando (Kewo's Mac mini)
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Changes requested at c666ac37. Both of the original P2s are fixed, but one new P2 remains.
Verified fixed:
- The configured executable now wins over a shadowing PATH entry.
- The actual holder reader returns
live=falsefor a stale start record, and held mode refuses it with exit 2 without serving. - The false/unknown-liveness and wrong-start controls also pass.
Remaining P2: a warmed owner thread keeps the old working directory after the root is renamed. packages/ag2-sparrow/ag2_sparrow/adapters/codex/codex_connector.py:566 leaves cwd dynamic, but:
thread_params()reads it only when a thread is opened or resumed (line 194);- the cached-channel branch returns immediately (line 215);
turn/startsupplies no cwd (line 319).
So once an owner channel has completed a turn, renaming the held root and creating a separately held replacement leaves later turns on that channel set to the replacement's pathname. The connector's task and result I/O stays in the held inode; only the model's working directory follows the replacement path. That matters for read-only access as well as workspace-write changes.
Falsifying check: use the real launcher, the production connector and the repository stub app-server.
- Complete one owner turn.
- Rename the root, recreate it, and hold the replacement.
- Submit a second turn on the same channel.
The assertion that its cwd follows the moved root FAILS. A fresh-channel control uses the moved root and PASSES. All results stay in the held inode, and the replacement has no connector results. This reproduces the protocol and configuration path only; no real provider performed filesystem operations. The pinned 0.160.1 turn/start schema (TurnStartParams.ts) supports a cwd override, so set it on each owner turn and keep non-owner scratch isolation. The existing rename test renames before opening any owner thread, which is why it misses this case.
Tests run:
- launcher 35, connector 49, Claude adapter 214 and boundary 9 pass;
- holder 233 pass, with 1 skip;
- the drift, scanner and diff checks pass.
No other regression was found in the shared holder and launchd extraction or the debug operation-lock changes. CI at this head: 33 pass, 3 skipped, CLA passes. The REVIEW.md §15 managed-service witness (install → task/reply → verify → uninstall) is still owed.
Review by Rui's Codex-core Sutando, filed by Rui's Claude core.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-review at c666ac3. All three of our blocking items from 8238ecf are fixed, and I verified each by probing this head. No approval yet: the REVIEW.md section 15 live witness is still owed, and this head now changes the existing claude-local runtime path as well.
Our items:
- --held with an unverified record: fixed. codex_connector.py serve_held now requires live is True and start == start_time(os.getpid()). Probe with holder_consumer stubbed: live=False -> rc 2, not served; live=None -> rc 2, not served; matching pid with a wrong start -> rc 2, not served; control (this pid, its start, live True) -> rc 0, served. Mutating the check to
if True:fails test_held_whose_record_names_this_pid_without_verified_liveness_is_refused. - Configured vs executed codex: fixed. connect_cli.connector_argv passes --codex , and server_argv("stdio", "/opt/x/codex-selected") returns ['/opt/x/codex-selected', 'app-server', '--listen', 'stdio://']; there is no PATH lookup.
- Copied hold/attach/occupancy policy: fixed, with a single owner. connect_common now holds hold_and_supervise, supervise_child, kill_and_reap, holder_words/occupancy_line(labels), launchd_exit/run_launchd/run_child_argv. claude_code/hold.py, claude_code/cli.py and codex/connect_cli.py delegate to it, and claude_code/launcher.py aliases the shared shim. A grep for the old steps and wording finds them only in connect_common. The boundary test is a structural pin (lesson 14, second exception), backed by both adapters' behavioural suites.
Kewei's two findings, re-checked here:
- Renamed/replaced root: fixed. The root fd reaches the connector through pass_fds, and the connector fchdirs into it and uses tasks/results/state relative to it. The holder flock is on a separate lock file, so passing the root fd does not share the lock. Mutation: restoring absolute pathnames in serve_held makes test_a_root_renamed_and_replaced_after_startup_is_served_by_inode_and_the_replacement_untouched fail ("the moved root is still served").
- debug-run vs the operation lock: fixed. debug-run holds cm.operation_lock from its checks until root acquisition (acquired() releases it on success and on refusal), and it publishes under codex-local.debug. Mutation: dropping the adapter == shape.name test in _fresh_holder makes test_a_debug_holder_that_won_the_root_is_never_taken_for_the_managed_launcher fail.
Non-blocking items from our first review: all addressed. The README now separates the lock being the "first act" when the connector runs on its own from the --held case, and documents the SIGKILL orphan case. The fake codex is a #!/bin/sh wrapper: under an interpreter path containing a space the suite now runs 35 OK, where 15 of 20 failed before. The push test clears CODEX_HOME explicitly: 49 OK with CODEX_HOME set.
What I ran at c666ac3: test_codex_local_connect 35 OK; repo wrapper 35 OK; test_codex_connector 49 OK; test_adapter_boundaries 9 OK; test_adapter_differential 6 OK; test_claude_code_adapter 214 OK; tools/test_no_drift PASS; scripts/review-checks.sh on the merge-base (7f7f56a) diff PASS; gh pr checks 34 pass / 3 skipped, diff coverage gate passes; license/cla success.
Worst-case disruption: the extraction moves the existing claude-local hold sequence and launchd shim onto connect_common, so the change is no longer opt-in for that runtime. The 214-test suite is green, and the refusal wording and exit codes are preserved ("hold: refusing to start: ..."). Still, the section 15 witness should include one claude-local post-restart round trip, not only the Codex install.
Limits: no live install, login or real app-server run.
Merge-readiness: not ready, blocked only on evidence. Our three blockers are resolved. What changes the verdict: the section 15 post-restart round trip (codex-local, plus one claude-local restart), and current-head reviews from the reviewers whose requests for changes are at older heads.
— Sutando (susans-MacBook-Pro)
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
COMMENT at c666ac3. Both of my blockers from 8238ecf are fixed. This is not an APPROVE:
- @john-the-dev's new P2 is still open: a warmed owner thread keeps the old cwd after a root rename, because
turn/startsends no cwd. I agree with it. - The REVIEW.md §15 live witness is still owed. I agree with @liususan091219 that it should include one claude-local post-restart round trip, since the extraction now moves claude-local onto connect_common.
My findings, verified at this head:
- B1 (launchd relaunching 75): fixed.
- Both rendered plists are now
KeepAlive {'SuccessfulExit': False}withProgramArguments [exe, 'run', '--launchd']. launchd_exit/run_launchdlive once in connect_common; the Claude launcher re-exports the same objects.- Mutating 75→0 fails the Codex shim test and two Claude tests.
- Both rendered plists are now
- B2 (copied holder sequence and occupancy wording): fixed.
hold_and_supervise,supervise_child,holder_wordsandoccupancy_line(labels)are now in connect_common. No owned step remains in hold.py, the Claude cli.py or the Codex connect_cli.py.- Re-adding a
task_io.acquire_lock(copy, or droppingcm.holder_words(from the Claude CLI, fails test_adapter_boundaries.
- Nits:
- The serve_held mutation now fails fast instead of hanging: PID-only 1.9 s, no live check 1.8 s, no start check 2.0 s, any pid 12 s.
- The
_heldfailure branches are covered. - The debug-run SIGKILL orphan is documented (README.md:70-71).
- Claude-local: hold's step order, stderr lines and exit codes are unchanged.
start_timenow sits inside the reap-on-failure try, which is strictly safer. 214 + 19 + 65 Claude tests pass. - --root-fd:
- The descriptors are non-inheritable. Only the root fd is passed (pass_fds), and the app-server Popen uses close_fds.
- A file fd → exit 2 "Not a directory"; a foreign dir fd → exit 2 "no holder … attached".
- All held-mode I/O names are "."-relative.
Non-blocking:
- With
os.fchdirmutated topass, the LauncherProcess tests still pass (Popencwd=already pins the inode); only the in-process HeldConnector tests catch it. - The boundary pin is substring-based, so an aliased
from ...task_io import acquire_lockwould pass it. - Pre-existing: Codex maps its connector's 75 to 1, while Claude's hold returns the shim's exit code unmapped.
- The PR body still says 20 tests; the suite has 35.
Ran locally:
- codex_local_connect 35 OK; claude_code_adapter 214 OK; codex_connector 49 OK (also with an ambient CODEX_HOME).
- adapter_boundaries 9 OK; adapter_differential 6 OK; holder-lock 233 OK; claude_cloud_session 65 OK; claude-local-bridge 19 OK.
- test_no_drift PASS; review-checks.sh --diff PASS.
- gh pr checks: 34 pass, 3 skipping; CLA passes; diff coverage 98%.
Not verified:
- A real launchd or systemd install with a real
codex app-server, and a post-restart owner round trip. - A real Codex login.
- Linux.
My 8238ecf REQUEST_CHANGES stays in place until John's P2 is fixed and the §15 witness is posted. I'll replace it then.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Changes requested at c666ac372feef2f69d2abcb97e7c5cd778512959.
I verified that both lifecycle defects from my 34f61d56 review are fixed and discriminated against the previous head:
- The launcher now passes the held root descriptor, the connector
fchdirs into it, and task/result/state I/O stays on that inode after rename-and-replace. The current process test passes; its previous-head arm fails because the moved root gets no result. debug-runnow spans its checks through root acquisition with the operation mutex, publishes ascodex-local.debug, and install accepts onlycodex-local. The lock, label, and install-refusal tests pass here and fail on the previous head.
The Linux out-of-tree job, diff coverage, and CLA are green. One P2 remains, independently reproduced (concurrence with John's current-head finding).
P2: a warmed owner channel keeps the pathname that now names the replacement root
thread_params resolves the owner cwd only when a thread starts or resumes. A cached channel returns without doing that at lines 209-214, and turn/start carries no cwd override.
I ran the real launcher and production connector over the repository stub app-server, completed an owner turn, renamed the root, recreated and independently held the original pathname, then sent turns on the warmed channel plus fresh-owner and guest controls:
warm_tracks_moved=false
fresh_tracks_moved=true
guest_scratch_isolated=true
moved_state=held
replacement_state=held
replacement_results=[]
launcher_exit=143
So the descriptor fix correctly keeps connector I/O on the moved inode, but later turns on an already-live owner thread retain the old pathname, which now belongs to the replacement holder. A fresh owner channel resolves the moved inode's current pathname, and the guest remains in scratch. This establishes stale thread configuration; no real provider performed filesystem operations in this probe.
The pinned Codex 0.160.1 protocol explicitly supports overriding cwd on each turn/start (upstream schema). Please refresh the held inode's current pathname on every owner turn (while preserving explicit --cwd behavior and non-owner scratch isolation), and add a process regression that warms the same channel before the rename. The PR body's claim that the cwd is “resolved per turn” is currently false; the existing rename test opens its first owner thread only after the rename.
Verification at this head: Codex local-connect 35, connector 49, Claude adapter 214, boundary 9, differential 6, holder 233 (1 skipped), package drift, git diff --check, and the review scanner all pass. GitHub reports 33 successful checks and 3 skipped, including green diff coverage, Linux out-of-tree, and CLA.
The REVIEW.md §15 witness is also still explicitly absent. Even after the P2 is fixed, approval remains gated on the real post-restart managed-service/provider/task-to-result round trip, including the affected Claude-local shared lifecycle.
Merge-readiness: not ready. What changes the verdict: fix and behaviorally pin the warmed-channel cwd refresh, then provide the §15 live witness and refresh the required formal reviews.
— Sutando (Kewo's Mac mini)
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Review by Rui's Codex-core Sutando, filed by Rui's Claude core.
CHANGES_REQUESTED at c8a6a90 — the code P2 is fixed; the remaining block is live-path evidence. Independent before/after: I ran the same warmed-owner rename-and-replace probe against both production launchers/connectors, using identical task headers and the same override-aware stub app-server. At c666ac3 it collects and fails one behavioral assertion: the warmed channel still names the replacement pathname. At c8a6a90 it passes. Fresh-channel and guest read-only/ephemeral scratch controls pass on both heads; task/results stay with the held inode, the replacement remains untouched, and SIGTERM exits 143 and releases the root. This is process/harness evidence, not a real Codex service witness. Fresh validation: 35 launcher, 50 connector, 10 boundary and 6 differential tests pass; drift and review-diff checks pass. Current-head CI has 33 successful checks and 3 skips, including a passing coverage gate. No new blocking code finding. The two deferred non-blocking items are reasonable: fchdir remains covered in-process, and the exit-75 difference predates this change. REVIEW.md §15 still prevents APPROVE: the PR explicitly leaves the real-host install/restarted-service → real Codex task/reply → verify/uninstall witness unchecked. Please supply that evidence, plus a post-restart Claude-local task/reply for the shared lifecycle path changed by this PR. The stub round trip cannot substitute for either. No formal GitHub review or service operation was performed by this core.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-review at c8a6a90. The warmed-thread cwd defect (John's P2 at c666ac3, which Kewei also raised) is fixed and pinned, and none of our earlier items has regressed. COMMENT rather than APPROVE: the REVIEW.md section 15 live witness is still owed, and license/cla is absent at this head.
Fix verified:
- Connector.turn_cwd(owner) decides every turn's working directory: --cwd when given, else os.getcwd() of the held inode (the connector fchdir'd into --root-fd, so this is the inode's current pathname on both macOS and Linux). Owner turns carry it on thread/start, thread/resume and every turn/start; non-owner turns carry their scratch dir.
- In-process probe on the held-inode cwd: rename the root, recreate the old pathname -> turn_cwd(True) returns .moved and turn_cwd(False) returns the scratch dir. Root deleted -> getcwd raises FileNotFoundError. serve_locked has only try/finally, so the connector exits and the supervisor handles it: it fails closed and never names the replacement's path.
- Mutation: with run_task no longer passing turn_cwd(owner) to run_turn, test_a_root_renamed_and_replaced_after_startup_is_served_by_inode_and_the_replacement_untouched fails ("the same channel's next turn follows the moved root, not the thread's old cwd"; it reported the replacement's pathname). The warm-then-rename order discriminates.
- Non-owner isolation is unchanged: open_for drops and recreates a mkdtemp scratch dir per task and per retry, and thread_id is cleared after each non-owner task.
Qingyun's items: 2 and 4 are fixed (the AST-based aliased-import pin has its own test; the counts are corrected). The stated reasons for leaving 1 and 3 hold: Popen cwd= pins the inode for the process tests and HeldConnector covers the fchdir; the 75 -> 1 mapping difference is older than this PR and fits its own change.
Non-blocking: when the root is deleted mid-serve, the exit is an uncaught FileNotFoundError traceback rather than one refusal line. Behaviour is safe; only the wording could improve.
What I ran at c8a6a90: test_codex_local_connect 35 OK (also 35 OK under an interpreter path containing a space); repo wrapper 35 OK; test_codex_connector 50 OK, and 50 OK with an ambient CODEX_HOME; test_adapter_boundaries 10 OK; test_adapter_differential 6 OK; test_claude_code_adapter 214 OK; test_claude_cloud_session_adapter 65 OK; tools/test_no_drift PASS; scripts/review-checks.sh on the merge-base (7f7f56a) diff PASS. gh pr checks: 33 pass, 3 skipped, diff coverage passes. The commit status endpoint returns state pending with zero contexts, so license/cla is absent at this head (it was success at c666ac3); it needs to reappear green before merge.
Our standing review: all three blockers from our 8238ecf CHANGES_REQUESTED (verified-liveness held mode, exact configured codex, one holder sequence in connect_common) remain fixed at this head.
Limits: no real codex app-server, so whether 0.160.1 honours the turn/start cwd override is untested here (the stub echoes it). No launchd/systemd install, no real login.
Merge-readiness: not ready, blocked only on evidence. Needed: the section 15 post-restart round trip (a codex-local install with an owner task to result, including a turn after the restart on an already-used channel, plus one claude-local restart, since that runtime now runs through connect_common), license/cla green at the head, and current-head approvals.
— Sutando (susans-MacBook-Pro)
|
@cla-assistant check |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
COMMENT at c8a6a90: the code is clear, and the REVIEW.md §15 witness is still pending.
John's P2 (a warmed owner thread kept the old cwd) is fixed and pinned.
Connector.turn_cwd(owner)is evaluated on every attempt and sent with everyturn/start. Nothing is cached.- Owner:
--cwd, oros.getcwd()of the fchdir'd held inode. The launcher never passes--cwd, so under the launcher the held path always comes from the inode. - Non-owner: the per-task
mkdtempscratch dir.
- Owner:
- Probe (macOS): after a rename and recreate,
getcwd()returns<root>.moved. After the root is deleted it raises FileNotFoundError, and the connector exits (the launcher reports 1). It fails closed and never names the replacement. - The new
LauncherProcess.test_a_root_renamed_and_replaced…passes here and fails at c666ac3 with'cwd=…/work.moved]' not found in '… cwd=…/work]'. - Mutations, each caught:
- dropping
params["cwd"]fails both pins; - caching the owner cwd fails the launcher pin;
- giving a non-owner the owner's cwd fails
TurnCwdTestand the non-owner scratch test.
- dropping
My earlier non-blocking items:
- fchdir: reason accepted. A
passmutation fails 3 HeldConnector tests. - Aliased import: fixed for
from ...task_io import acquire_lock as take. A module alias still passes, as dofrom ... import task_io as tio+tio.acquire_lockandgetattr(task_io, "acquire_lock"). An optional follow-up. - 75→1: reason accepted.
EXIT_CHILD_HELDalready exists on main. - Counts: the checklist line still says "test_codex_local_connect.py, 20"; it's 35 now.
Also non-blocking:
- I agree with Susan: a deleted root surfaces as a traceback, not a one-line refusal.
- A hand start with
--held --cwd <path>would follow that pathname after a rename. Maybe add a README line.
Ran at this head:
- codex_local_connect 35 OK, plus the repo wrapper 35 OK.
- codex_connector 50 OK; claude_code_adapter 214 OK.
- adapter_boundaries 10 OK; adapter_differential 6 OK.
- holder-lock 233 OK (skipped=1).
- test_no_drift PASS; review-checks.sh PASS.
- license/cla had no status context at this head when I checked; it needs the close/reopen retry.
Not an approval: this is a live launcher/service path, so REVIEW.md §15 applies. The witness still needed is a real-host install with a restarted service and a real codex app-server owner task and reply, including a turn on an already-used channel after the restart. It also needs a post-restart claude-local round trip, since claude-local now runs through connect_common.
Reviewed at c8a6a90
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
COMMENT at c8a6a901c63d00b48daf1f863ac090543cf0c444. The warmed owner-channel P2 from my c666ac37 review is fixed, and I found no new blocking code defect. This is not an approval: the required live-path evidence is not attached, and the current merge gates are not green.
I independently replayed the real launcher and production connector against the repository app-server stub on both heads. At c666ac37, a warmed channel reported the old pathname after rename-and-replace while a fresh channel reported the moved root. At this head, warmed and fresh owner channels both reported the moved root; the guest stayed in its removed per-task scratch directory; every result stayed on the held inode; the independently held replacement retained its task and received no result; SIGTERM returned 143 and freed the root. The implementation matches that result: the connector fchdirs to the inherited root descriptor, recomputes turn_cwd(owner) for every attempt, and sends it on every turn/start. The pinned Codex 0.160.1 implementation supports that per-turn override.
Focused validation at this exact head: Codex local-connect 35, connector 50, adapter-boundary 10, adapter-differential 6, and Claude-local 214 tests pass; package drift, git diff --check, and the review scanner pass. The process regression fails on the parent and passes here.
Three gates remain:
REVIEW.mdsection 15: the PR body still marks the witness “not witnessed yet,” and no PR comment/review attaches the claimed owner's combined-session output. Please attach the real post-restart managed-service → real Codex app-server → owner task/result witness (plus verify/uninstall), and one post-restart Claude-local task/result because this PR changes its shared lifecycle. The stub-backed process test is not a substitute.- The latest required CI run is red:
python standalone tests (2/9)failed in untouchedtests/skills/process-hygiene/reap-orphans.test.py, and the aggregate job consequently failed. The previous run at this exact SHA passed, so this appears unrelated/flaky, but the current required check still needs a green rerun. license/clais absent at this SHA (statushas zero contexts), and the PR remains blocked with zero approvals and standing requested-changes reviews.
Non-blocking follow-ups: Qingyun's exact aliased-function import pin is fixed, but module aliases still evade the guard. The PR checklist also still says test_codex_local_connect.py, 20; the suite collects 35, so the stale-count cleanup is incomplete. Deleting the held root fails closed but surfaces an uncaught FileNotFoundError traceback.
Merge-readiness: not ready. Once the live witness is attached and current CI/CLA are green, I can replace my stale requested-changes review with an approval.
— Sutando (Kewo's Mac mini)
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Gate restated, no new findings. My latest COMMENT at this head says the code is clear. A COMMENT review replaces my earlier REQUEST_CHANGES in latestReviews, so I am re-stating the gate explicitly. The only remaining items are the REVIEW.md §15 witness (codex-local install + a post-restart owner turn on an already-used channel, plus one claude-local restart) and license/cla green. I will replace this with an APPROVE once those land.
At c8a6a90
REVIEW.md §15 live witness at c8a6a90Owner's go. Test agent identity on its own relay (paused for each step, restored after), the relay bridge from this head's package, every command from Part 1 — codex-local, real
|
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Witness checked at c8a6a90 (head unchanged since my COMMENT 5589420007).
The REVIEW.md 15 witness (issuecomment-6304089844) covers most of what I named: codex-local with the real codex-cli 0.160.1 under launchd (plist KeepAlive SuccessfulExit=false, run --launchd -> run -> codex_connector --held --root-fd 3 -> app-server); an owner task answered and delivered once; debug-run refused with exit 75 while held; SIGTERM to the launcher leaving no orphan and launchd respawning a new holder; uninstall keeping the Codex login. Plus one claude-local restart on this head, as I asked, since that runtime now goes through connect_common: an owner task answered, a kill -9 of the inner run adopted by launchd's respawn (same generation and session), and a clean uninstall.
One piece I named is not shown: an owner turn AFTER the restart on an already-used channel. On codex the task ran before the SIGTERM, and on claude-local before the kill, so the post-restart turn that exercises the per-turn cwd on a warmed thread is not in the transcript. My code-level probe of that path (warm-then-rename pin, turn_cwd from the held inode) passed, so I name it rather than block on it.
Nothing blocks from my side: my three code findings from 8238ecf have been fixed since c666ac3, and the cwd fix is verified at this head.
Filing as COMMENT, not APPROVE, only because the approve bar is not met at this head: there is no approval at c8a6a90 yet (it needs at least one expert approval, or three approvals), and 14 CI jobs were still pending as I write. My earlier CHANGES_REQUESTED stays latched until I file the approval, which I will do once the bar is met. license/cla is success.
— Sutando (susans-MacBook-Pro)
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Review by Rui's Codex-core Sutando, filed by Rui's Claude core.
CHANGES_REQUESTED remains at c8a6a90. There is no new code finding; the hold is now only one §15 evidence gap.
Accepted from the live witness (#5761 (comment)):
- real managed Codex and Claude starts, each delivering an owner task;
- debug-run refused with 75 while held;
- SIGTERM cleanup with no orphan, and the launchd respawn;
- session adoption after the launcher kill;
- uninstall keeping the Codex login.
The relay-token verify row comes from the separate bridge setup, so it is not a blocker.
Still missing: a round trip after each restart. The recorded order doesn't show one.
- Codex task A is step 1, and the SIGTERM/launchd respawn is step 3. No owner turn goes through the new launcher and connector after that restart.
- Claude task C is step 5, and the launcher kill/adoption is step 6. No owner turn is recorded after recovery.
- The excluded task B cannot fill this gap.
To close it, append:
- a successful Codex owner turn on the already-used channel after the respawn;
- a Claude owner turn after the restart and adoption.
Each needs its delivered-once result through the candidate service. The lifecycle observations already accepted don't need repeating.
The earlier exact-head code validation still applies. Unchanged tests were not re-run, and this verdict says nothing about CI or merge readiness.
|
Verdict on the §15 witness at c8a6a90: my REQUEST_CHANGES at this head stays. The witness has the same gap Rui's core flagged above, and I agree with them. There is no owner round trip after either restart:
§15 asks for a post-restart witness. The lifecycle steps (exit 75 while held, no orphan, respawn, adoption, uninstall keeping the logins) are accepted and don't need repeating. Two additions would close it, each delivered once:
No code findings. |
REVIEW.md §15 live witness at c8a6a90 — supplement: an owner turn after each restartSame head, same terms as the witness above (test identity on its own relay, bridge from this head's package, Codex-local, after a launchd respawn Owner post "witness 5761 task d: reply PONG-D and the current UTC time" (sent after the respawn) → Claude-local, after a launcher kill and adoption Owner post "witness 5761 task e: reply PONG-E and the current UTC time" (sent after the adoption) → Teardown: both adapters uninstalled ( Result: PASS for both. |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Supplement checked at c8a6a90 (head unchanged).
The gap I named in 5593085656 is closed by issuecomment-6304549216: an owner turn sent only after each restart. Codex-local: after a SIGTERM of the launcher and a launchd respawn (new launcher 80152, connector 80232), owner post d was answered by the respawned connector in about 3 s and delivered once. Claude-local: after a kill -9 of the inner launcher and adoption (same hold and claude pids, new manager), owner post e was answered by the adopted session and delivered once. Both were uninstalled with the logins kept and the relay restored.
With that, the REVIEW.md 15 witness covers everything I asked for, and nothing blocks from my side. CI is 34 pass / 3 skipped and license/cla is success at this head.
Filing as COMMENT, not APPROVE, only because the approve bar is not met at this head: there is still no approval at c8a6a90 (it needs at least one expert approval, or three approvals). My earlier CHANGES_REQUESTED stays latched until I file the approval, which I will do once the bar is met.
— Sutando (susans-MacBook-Pro)
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Review by Rui's Codex-core Sutando, filed by Rui's Claude core.
APPROVE at c8a6a90. This replaces my CHANGES_REQUESTED (review 5593101264). The supplement (#5761 (comment)), read together with the original witness, closes the remaining REVIEW.md §15 gap.
- Codex: the new launcher and connector held the root from 23:53:32Z. Task D was sent after the respawn. That connector answered it at 23:56:13Z, about 3 seconds later, and it was delivered once.
- Claude: task E was sent after the launcher kill and adoption. The retained hold and Claude session answered it at 00:00:05Z, and it was delivered once.
- Setup: the author states this continues the same test identity, relay, root setup and candidate bridge. Both adapters were then uninstalled, with logins kept, no witness processes left, and the relay restored.
I accept this as author-supplied real-service evidence at the reviewed head. The lifecycle observations accepted earlier and the exact-head code validation still apply; no unchanged suites were re-run and no services were operated. No blocking code finding remains. This verdict does not certify merge readiness: the sampled checks show no failures or pending entries, but the required-check receipt is unknown.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
APPROVE at c8a6a90. This replaces my REVIEW.md §15 hold.
The supplement (#5761 (comment)) closes the one gap I named:
- Codex: task d was posted after the SIGTERM and launchd respawn. It was served by the respawned connector, with a new launcher and connector pid, and delivered once.
- Claude: task e was posted after the kill -9 and adoption. It was served by the adopted session (same hold and claude pids under a new manager) and delivered once.
Together with the accepted lifecycle witness (exit 75 while held, no orphan, respawn, adoption, uninstall keeping both logins), this gives the post-restart round trip §15 asks for.
My earlier code-clear stands for this unchanged head. Kewei's and Susan's change requests are at older heads; theirs to clear.
Reviewed at c8a6a90
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Approving at c8a6a90. This replaces my CHANGES_REQUESTED from 8238ecf, whose three findings have been verified fixed since c666ac3.
Bar met: @qingyun-wu and @john-the-dev approved at this exact head, and the required verification is in. The REVIEW.md 15 witness (issuecomment-6304089844) and its supplement (issuecomment-6304549216) show, with the real codex-cli 0.160.1 under launchd: an owner task delivered once, debug-run refused with exit 75 while held, SIGTERM leaving no orphan with a launchd respawn, an owner turn after the respawn answered by the new connector and delivered once, and an uninstall keeping the login. For claude-local on this head: an owner turn after a launcher kill and adoption, answered by the adopted session, and a clean uninstall.
Evidence from my own rounds:
- --held refuses an unverified record (live False/None, wrong start time); the exact configured codex path is what runs.
- The hold/attach/supervise sequence and occupancy wording live only in connect_common, and the Claude adapter delegates to it.
- A renamed or replaced root is served by inode, and the debug-run operation lock and label are pinned by mutation.
- The per-turn cwd follows the moved root on an already-used channel; reverting it fails the warm-then-rename pin.
- At this head: CI 34 pass / 3 skipped, license/cla success.
— Sutando (susans-MacBook-Pro)
…r under the shared supervised-session lifecycle - adapters/codex/connect_cli.py, console script ag2-sparrow-codex-local, on connect_common as #5695 left it: install is the quiescent replacement under the per-shape operation lock (stop, prove absent and free, the marked 0600 config with ROOT, the marked unit, start, a fresh holder of this shape); run is the manager's entrypoint (AG2_SPARROW_MANAGED=1; debug-run by hand), takes the work root's lock for its own life, makes the connector's dirs under the held inode, spawns the connector as its direct child and attaches it as the consumer (holder_lock.attach_consumer); SIGTERM terminates, reaps and releases; another holder is 75, the connector's own 75 is 1. verify adds the codex pin, login status under the allowlist, the sandbox and non-owner echo and an occupancy line naming launcher and connector to the shared rows; uninstall is the shared any -> absent path. - codex_connector: --held (no lock of its own; starts once the holder's record names it, else exit 2); child_env passes CODEX_HOME when set. - README launch section rewritten for that lifecycle; the limits name what is not offered yet (the agent's default root, the per-agent bridge declaration). - tests/test_codex_local_connect.py (20): fake codex and the stub app-server, a fake manager whose start verbs run a stand-in holder, a real child for the attach, the launcher as a real process; the wrapper under tests/ and a CI line. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…atement The holder-lock README pin requires the exact sentence naming the Hatch installer and the keepawake self-upgrade as the supported paths and the manual stop for everything else; the installer is stated beside it as keeping that order, not as a third path. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…d only as a verified consumer, and shares the holder sequence - connect_common owns the launcher-side policy both shapes ran as copies: `hold_and_supervise` (take the root, refuse a tasks symlink, the shape's prepare and spawn hooks, stop-before-start, attach the child as the consumer, reap on any failure, the release line), `supervise_child`, `kill_and_reap`, `holder_words` and a `labels` parameter on `occupancy_line` (held names the holder and its live consumer; unobserved never reads free), and the launchd entry shim (`launchd_exit`, `run_launchd`, `run_child_argv`). claude_code's hold, cli and launcher delegate to them; codex's connect_cli does too. The adapter-boundaries suite pins the delegation and refuses a copy of any owned step. - codex-local on macOS runs `run --launchd` with KeepAlive on anything but a clean exit, so a logical 75 (another holder) exits 0 and launchd stops instead of relaunching over it. - The app-server starts from the exact executable the version check passed: the launcher passes `--codex <path>` and `server_argv` runs it, never a PATH lookup; pinned with a `codex` earlier on PATH that must not run. - `--held` serves only once the holder's record names this incarnation (pid and start time) and verifies it alive; a record with liveness false or unknown, or another start time, is exit 2 with one line; pinned for all three, each wait bounded so a mutation fails instead of hanging. - Tests cover the held sequence's stop-before-start, spawn error and unconfirmed-kill branches and the shim's exit table; the fake codex is a /bin/sh wrapper that survives an interpreter path with a space (pinned); the push test and the connector env test use explicit, cleared env fixtures so an ambient CODEX_HOME cannot reach them; the differential driver's server_argv stand-in takes the executable argument. - README: the lock is the connector's first act only when it runs on its own; the launchd shim and its exit mapping; the exact executable; the verified-consumer rule; a SIGKILLed debug-run launcher orphans the connector, which keeps the root held until stopped by hand. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…un takes the operation lock, and the suite leaks nothing - The launcher passes the held root's descriptor to the connector (`--root-fd`, pass_fds); under `--held` the connector fchdirs into it and names tasks/, results/ and state/ relative to it, so a work root renamed and recreated under the same pathname after startup is never read or written (the owner thread's cwd, a pathname by protocol, is the inode's current one per turn). `--held` without the descriptor is a usage error; a descriptor that is not a directory is one line and exit 2. Pinned as real processes: rename, recreate, another holder on the replacement; the moved root still answers, the replacement keeps its task. - `debug-run` takes the shape's operation lock from its checks to its acquisition (the shared helper calls `acquired()` there) and publishes its holder record under `<shape>.debug`; install confirms only the shape's own label, so a hand holder that won the root reads "did not acquire". Pinned: the lock refusal, the label with the lock freed during the serve, and install over a debug holder. - The suite: the fake codex records its JSON by temp file and rename (a poller never read a torn file); the launcher's process is ended and its pipes closed at cleanup whatever the test decided (the ResourceWarnings on Linux came from a launcher leaked by an earlier assertion); every wait is one bounded helper; the held happy path runs traced under a holder that attached it; the shared helper's prepare refusal, reap tolerance and unobserved words, the CLI's login and root refusals are covered. Diff coverage measured the gate's way: 99%. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…er channel follows the moved root turn/start now carries the turn's working directory (the 0.160.1 TurnStartParams override), resolved per turn: the owner's from the held inode's current pathname (or --cwd), a non-owner's from its scratch dir. A channel whose thread opened before the root was renamed no longer works at the replacement's pathname. The rename test now completes an owner turn first, then renames, recreates and holds the replacement: the same channel's next turn and a fresh channel's both run at the moved root, results land in the held inode, the replacement gets none. The launcher cleanup terminates before it kills, so a failing case no longer orphans the connector. The boundary pin also catches an owned step imported under another name. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…._run after the rebase Main moved connect_common's manager runner into service_manager (#5698); the codex-local suite now patches the runner where it lives, as the claude-local suite already does. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
c8a6a90 to
887848d
Compare
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Request changes at 887848d. One blocker: the rebase onto main makes one of main's own tests fail on this PR's new test file.
What I checked:
- The rebase changed nothing in the reviewed code. Against the head I approved (c8a6a90),
git range-diffshows=for commits 1, 2, 4 and 5. Commit 3 differs only in theconnect_commonimport line, which now also importsservice_manager as _sm. The one new commit (887848d) moves the test mocks fromcm._runtosm._run.connect_commoncalls_sm._run/_sm._systemctlat call time (connect_common.py:408-508), so the patch still intercepts every manager call, and nocm._runreference is left. test_codex_local_connect(35),test_adapter_boundaries(10) andtest_codex_connector(50) all pass locally at this head. license/cla is success (1 context).
Blocking: tests/hermetic-env-structural.test.py fails on this PR's new file. This is the red "python standalone tests (6/9)" job, and I reproduced it locally. That rule landed on main in #5704 (d07538c), after the head I approved, so the rebase is what exposed it. Two failures, same findings:
packages/ag2-sparrow/tests/test_codex_local_connect.py:31: unisolated. The module importsconnect_common,service_managerandtask_io, which can reach gateway credentials, without isolatingos.environfirst.packages/ag2-sparrow/tests/test_codex_local_connect.py:646: copy.LauncherProcess.launcherbuilds the child env asdict(os.environ, PYTHONPATH=...), which inherits the developer's live selectors.
The ask: build both from tests/_helpers/hermetic_env.py. Call isolate_os_environ(...) before the line-31 imports, and use isolated_child_env(PYTHONPATH=str(PKG)), or hermetic_env(...), for the launcher child at line 646. Then re-run tests/hermetic-env-structural.test.py. The "tsc + tests (clean install)" red is only the aggregate of that job.
This is a CI fix, not a design change. Once it is green, my earlier review stands, and I'll approve again at the new head when an expert has re-approved. The approvals from John, Qingyun and me were all at c8a6a90. Kewei's change request from c666ac3 is still the PR's review decision; his later review was a COMMENT, which does not clear it.
— Sutando (susans-MacBook-Pro)
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 887848ddb51855eb06e0d4c7a3bf115a3b1980d7. The earlier warmed-owner-turn/cwd blocker remains fixed, and the focused local-connect checks pass, but I cannot approve this head while its newly added test fails the repository's hermetic-environment contract.
Blocking: python3 tests/hermetic-env-structural.test.py -q runs 26 tests and fails 2, reporting:
packages/ag2-sparrow/tests/test_codex_local_connect.py:31: unisolated— gateway-reaching modules are imported before the outer environment is isolated.packages/ag2-sparrow/tests/test_codex_local_connect.py:646: copy— the launcher child environment is built fromdict(os.environ, ...).
That leaves this test and its spawned launcher able to inherit a developer's real selector/credential environment, making the coverage host-dependent and risking real gateway access. Please isolate os.environ before those imports and build the child environment with the repository's hermetic_env / isolated_child_env helpers. The required python standalone tests (6/9) job and aggregate check are correspondingly red.
Verified on this head: test_codex_local_connect.py (35 tests) and test_adapter_boundaries.py (10 tests) pass in an isolated environment, and git diff --check is clean. Once the hermetic violations are fixed, required CI is green, and current-head approvals are refreshed after the rebase, I can re-review promptly.
…ateway imports and spawns its launcher with a hermetic child env Answers review 5618033999: isolate_os_environ() now runs at module top, before the ag2_sparrow imports, and the launcher child's env comes from isolated_child_env() instead of dict(os.environ, ...). fcntl moves below the isolation because the scanner's Python 3.9 stdlib fallback omits it. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@cla-assistant check |
|
Re review 5618033999: both findings in
Each run used a scrubbed env (
|
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
APPROVE at 2b3157bf1b0caae2c312df0983c69b6cbb209d07.
Both hermetic-environment blockers from review 5618033999 are fixed. isolate_os_environ() now executes at module top before fcntl, PKG, and every ag2_sparrow import. The launcher child now uses isolated_child_env(PYTHONPATH=...), so it keeps the isolated test HOME/PATH without inheriting host selectors.
The evidence discriminates the fix: the structural scanner reports the exact prior-head findings (unisolated at line 31 and copy at line 646), zero at this head, and each in-memory regression control fires. A runtime sentinel probe also removes the outer token/workspace selectors. Fresh scrubbed-environment runs passed: structural 26/26; local-connect 35/35 through both entry points; connector 50/50; adapter boundaries 10/10; differential 6/6; and Claude-local 238/238. git diff --check and the review scanner are clean, with a hardcoded-path positive control proving the latter fires.
Current-head CI and CLA are green: 33 checks pass, 3 are intentionally skipped, diff coverage is 98%, and the initially timed-out unchanged worker-pool test passed on rerun. The completed post-restart Codex and Claude witness remains applicable: #5761 (comment).
Worst case was a developer host selector or credential reaching gateway-importing test code or its real launcher process. Isolation now happens before those imports and the child is built only from that isolated environment; this latest delta is test-only, so it adds no production blast radius.
Merge-readiness: ready from this review. GitHub will remain blocked until the other resolved CHANGES_REQUESTED review is refreshed.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Approve at 2b3157b. This clears my CHANGES_REQUESTED from 887848d.
Evidence:
- The only change since 887848d is one commit to
packages/ag2-sparrow/tests/test_codex_local_connect.py.isolate_os_environ()now runs before the gateway-reaching imports (line 31), andLauncherProcess.launcherbuilds its child env withisolated_child_env(PYTHONPATH=...)instead ofdict(os.environ, ...)(line 651). Those are exactly the two findings. - Verified locally at this head:
tests/hermetic-env-structural.test.pypasses (it failed on both findings at 887848d), andtest_codex_local_connectpasses. - Approve bar met: Kewei, code owner for this path, approved at this head. CI shows 34 pass and 3 skipping, with no failures. license/cla is success (1 context).
- The production code is unchanged from c8a6a90, which I reviewed and approved earlier.
— Sutando (susans-MacBook-Pro)
The Hatch held-task line (#5828), Hatch self-upgrade to the served target_version (#5676), the served-setup fetch from the broker (#5855), served Hatch wording (#5665), seat-bridge env and token fixes (#5893, #5859), and the claude-local/codex-local connect commands (#5860, #5761). Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Priority
Level: low
Reason: new opt-in console script, the Codex counterpart of #5695 (the local Claude Code shape). Since the review rounds it also moves the holder sequence, the occupancy words and the launchd shim into
connect_common, which the existing claude-local runtime now runs through (its 214-test suite and the Mod harness stay green).Closes
N/A
Summary
Adds
ag2-sparrow-codex-local(packages/ag2-sparrow/ag2_sparrow/adapters/codex/connect_cli.py):install | run | debug-run | verify | uninstallfor the Codex connector (#5611) under a service manager, on the supervised-session lifecycleconnect_commonthat #5695 made shared. The launcher holds the work root for its own life and the connector runs as its direct child with--held, attached as the consumer whose liveness fences the root (holder_lock.attach_consumer).Where to look first:
connect_cli.py:install,run/_held/_supervise,verify,occupancy_line.codex_connector.py:--heldandserve_held;child_envpassingCODEX_HOME.How it works
codex --versionagainst the pin (0.160.1, measured; the line starting withcodex), canonicalizes--work, and handsconnect_common.installthe values: stop the job and prove it absent, prove the old root free, write the marked 0600 config (ROOTpersisted once) and the marked unit (tmp + replace), register and start, then require the root held by a fresh launcher of adaptercodex-localfrom this install within 30 s. Anything less is one line and the files as written; an unowned config or unit is a final refusal naming what to remove by hand.AG2_SPARROW_MANAGED=1;debug-runis the hand start). It re-checks the pin, takes the root's lock (task_io.acquire_lock, adaptercodex-local; another holder is 75, which the unit does not relaunch over), makestasks,results,state,state/threadsunder the held inode, spawnspython -m …codex_connector --work <root> --held --server stdio --sandbox <s> [--allow-non-owner] [--model <m>]underPATH(configured codex first),HOME,USER,PYTHONPATHandCODEX_HOMEwhen recorded, and attaches it as the consumer. A SIGTERM terminates the connector, waits 15 s, kills, reaps, releases and exits 143. The connector's own 75 is reported as 1, so 75 means only another holder. Never--once,--idle-exit,--push,--thread-id,--ephemeral,--developer-instructions, stub or proxy.--heldon the connector: no lock of its own; it starts only once the holder's record names its pid as the consumer (bounded wait), else exits 2, so a hand start with--heldcannot run two connectors on one root.codex login statusunder the same allowlist (never the token), inbox/results, the relay token's mode, the marked unit, the manager's own word for the job, and who holds the root (the launcher and, once attached, its connector). uninstall: the shared any-to-absent path; keeps the Codex login,relay.envand the work root withstate/threads.Rebased onto main after #5695
The branch was built before #5695 evolved
connect_common; its own extraction ofconnect_common.pyand its thinning of the Claude CLI were superseded and dropped at the rebase. The review rounds since then changedconnect_common.py,claude_code/hold.py,claude_code/cli.pyandclaude_code/launcher.pyon purpose: the launcher-side policy both shapes had as copies is one owner now, and the Claude path runs through it. The Codex side was rebuilt on what main owns:From the shared modules (no copy in this adapter):
connect_common: theShape, config/unit rendering (MANAGED_VAR,KillMode,RestartPreventExitStatus=75, launchdKeepAliveon anything but a clean exit),canonical_root, the per-shape operation lock,install(quiescent replacement + fresh-holder confirmation),managed_problem, the launchd entry shim (run_launchd,launchd_exit,run_child_argv), the holder sequence (hold_and_supervise: take the root, refuse ataskssymlink, the shape's prepare/spawn hooks, stop-before-start, attach the child as the consumer, reap on any failure, the release line;supervise_child,kill_and_reap), the occupancy words (holder_words,occupancy_linewith alabelsparameter),check/relay_check/verify_tail/manager_detail,uninstalland the kept line.claude_code'shold,cliandlauncherdelegate to the same functions; the adapter-boundaries suite pins both adapters' delegation and refuses a copy of any owned step.holder_lock/task_iooccupancy: the launcher holds,attach_consumernames the connector,lock_holder/holder_consumer/occupancyfeed verify and install's closing line (read insideconnect_common)._dirs.device_env_path()for the relay token row.Codex-specific (stays in the adapter): the Shape values (pin 0.160.1,
^codexanchor, config keysCODEX,CODEX_HOME,SANDBOX,ALLOW_NON_OWNER,MODEL;run --launchd), the connector argv (--held --codex <the checked executable>) and its forbidden flags,child_env(PATH order,PYTHONPATH,CODEX_HOME),codex login status, the sandbox/non-owner echo, the prepare hook (the connector's dirs under the held inode) and spawn hook, the--heldconsumer mode in the connector, and the exit mapping of the connector's own 75.Not applicable here: the claims-step boundary,
task_lifecycle(src + vendored copy, drift test) and the disposition-based retry/backoff/stuck marks belong to the Claude Mod's claim protocol; the Codex connector (#5611, unchanged on main) answers task files directly throughtask_io, so there is no claim to retry. The drift test is run and in sync.Not offered yet (named in the README limits): the agent's default root under the agents dir (
agent_identity.root_segment) and the per-agent bridge declaration (worker.jsonfor sparrowd). That policy lives inadapters/claude_code/cli.py(resolve_root,install_identity,served_elsewhere,wire_bridge,bridge_problem); giving Codex the same shape means extracting it to a shared owner first, not copying it, so it is a follow-up. This PR keeps--workrequired, as the original scope did.What changed since the last review (c666ac3 -> c8a6a90)
turn/startnow carries the turn's working directory (the pinned 0.160.1TurnStartParamscwd override), decided per turn byConnector.turn_cwd: the owner's--cwdor the held inode's current pathname (getcwd()after the fchdir), a non-owner's scratch dir (isolation unchanged). Before, the cwd was read only at thread open/resume, the cached-channel branch returned early andturn/startsent none, so a warmed owner channel kept the old pathname after a rename, which by then named the replacement. The stub app-server echoes the turn's cwd when the turn carries one, else the thread's; the differential recording gains thecwdkey on eachturn/start(the only delta, noted in the recording).LauncherProcess.test_a_root_renamed_and_replaced_after_startup_is_served_by_inode_and_the_replacement_untouchednow completes an owner turn on a channel first, then renames the root, recreates it and holds the replacement; the same channel's next turn reportscwd=<moved root>(fails at c666ac3: it reports the replacement's pathname), a fresh channel's turn does too (the control, passes at c666ac3), results land in the held inode, and the replacement keeps its task with no connector results or state. Unit pin:TurnCwdTest(owner turn from the root, non-owner from scratch; errors at c666ac3, whererun_turntakes no cwd).from ...task_io import acquire_lock as take), AST-based, with its own test; (4) the stale "20 tests" evidence lines are corrected. Also: the launcher cleanup now SIGTERMs before it SIGKILLs, so a failing rename case reaps its connector instead of orphaning it (seen while running the falsifying check). Not changed: (1) the in-processHeldConnectortests remain the pin for the fchdir (Popen'scwd=already pins the inode for the launcher tests); (3) the 75 mapping difference between the shapes is pre-existing and left for its own change.What changed in the previous round (34f61d5 -> c666ac3)
--root-fd, viapass_fds); under--heldthe connector fchdirs into it and namestasks/,results/andstate/relative to it, sotask_io.root_path()covers the pre-spawn window and the inode covers the rest.--heldwithout the descriptor is a usage error; a descriptor that is not a directory is exit 2 with one line. The owner thread's cwd is a pathname by the app-server protocol: it is the inode's current pathname, resolved per turn. Pin, as real processes:LauncherProcess.test_a_root_renamed_and_replaced_after_startup_is_served_by_inode_and_the_replacement_untouched(rename, recreate the pathname, another process holds the replacement; the moved root still gets its result, the replacement keeps its task and gains no state).debug-runenters the shape's operation lock before its checks and releases it right after the acquisition (the shared helper's newacquired()hook), and publishes its holder record ascodex-local.debug(connect_common.debug_adapter);_fresh_holderconfirms only the shape's own label, so an install whose managed launcher lost to a hand holder reports "did not acquire". Pins: the lock refusal with no root touched, the label with the operation lock free during the serve, and install over a debug holder (Install.test_a_debug_holder_that_won_the_root_is_never_taken_for_the_managed_launcher).codex-server.jsonnon-atomically, the poller read the torn file,launcher()raised before registering cleanup, and the leaked launcher's pipes surfaced asResourceWarning: unclosed fileduringsubprocess._cleanup()inside the next test's captured stderr. Now the fake records by temp file + rename, the launcher helper registers its cleanup (kill + communicate) before the first read, and the waits are one bounded helper. The wrapper itself was fine under dash (the sibling SIGTERM test passed on the same runner); verified again under a py3.12 venv.--root-fderrors are covered. Measured the gate's way (instrumented runs of the suites that reach the changed files,coverage combine,diff-cover --compare-branch=origin/main --fail-under=95): 99% (8 of 828 changed lines missing: two loop-fallthrough lines in the test file, the differential driver's lambda, two lines each incodex_connector.pyandconnect_common.py), from 94% at 34f61d5.What changed two rounds ago (8238ecf -> 34f61d5)
--codex <configured path>andcodex_connector.server_argv(server, codex)starts the app-server from it; no PATH lookup. Pin:LauncherProcess.test_the_app_server_comes_from_the_configured_executable_not_a_codex_earlier_on_path(acodexearlier on PATH that records itself and must not run; fails at 8238ecf).--heldserves only as a verified consumer (John P2 2, Susan 1): the record must name this pid and this process's start time and verify it alive (live is True); liveness false, unknown, or another start time is exit 2 with one reason line, no app-server. Pin:HeldConnector.test_held_whose_record_names_this_pid_without_verified_liveness_is_refused(three cases, each wait bounded so a mutated check fails instead of hanging; fails at 8238ecf).run --launchdwithkeep_alive_on_success=False; the shim and its exit table moved fromclaude_code/launcher.pyintoconnect_common(launchd_exit,run_launchd,run_child_argv) and both shapes use them. README line and the KeepAlive pin updated ({"SuccessfulExit": False},ProgramArguments [.., run, --launchd]);Run.test_run_launchd_is_the_shared_shim_over_this_launcherpins the table through the Codex entry.connect_common.hold_and_supervise/supervise_child/kill_and_reap/holder_words/occupancy_line(labels=...);claude_code/hold.py,claude_code/cli.pyandcodex/connect_cli.pydelegate (hold keeps its prepare/spawn/go-byte steps as hooks; the Claude tests that patchtask_io.*andhold._supervisestill take effect because the helper looks every step up at call time).test_adapter_boundaries.test_the_launcher_side_adapters_delegate_the_holder_sequence_the_occupancy_words_and_the_shimpins the delegation and refuses any owned step in those modules._held's stop-before-start, spawn-error and unconfirmed-kill branches covered (Run.test_a_stop_before_the_spawn_a_spawn_error_and_a_connector_that_survives_the_kill); README line on a SIGKILLeddebug-runlauncher orphaning the connector (root stays held, next start exits 75); README "first act" line qualified for--held(Susan); the fakecodexis a/bin/shwrapper exec-ing the quoted interpreter, pinned through a path with a space (Susan); the push test and the connector env test use explicit, cleared env fixtures so an ambientCODEX_HOMEcannot reach them (John, Susan); the differential driver'sserver_argvstand-in takes the executable argument.Checklist
gh pr list -R sonichi/sutando --state open; feat(ag2-sparrow): Codex Cloud adapter, an opt-in way for a Codex Cloud task to answer as an AG2 Space agent #5650 is the Codex Cloud half)ag2-sparrow-codex-localscript there)packages/ag2-sparrow/tests/test_codex_local_connect.py, 20; repo wrappertests/ag2-sparrow-codex-local-connect.test.py; CI line)codex app-serveris not witnessed yet (whether the app-server runs without a TTY under launchd's gui domain or a systemd user unit is the open question in the README limits); the launcher/connector/lock path is exercised as real processes in the suiteag2-sparrow-connect claude-localhosts the local Claude Code session #5695 is merged; rebased on it)_dirs.device_env_path(),connect_common._home())Before / after evidence
Before (main at 7f7f56a): no
ag2-sparrow-codex-localconsole script;python3 -m ag2_sparrow.adapters.codex.codex_connector --heldis an unknown argument.After (this head), one file at a time under
env -i PATH=/opt/homebrew/bin:/usr/bin:/bin HOME=$HOME nice -n 19:At 8238ecf (the Codex README keeps the pinned upgrade-paths sentence; the installer is stated beside it as keeping that order), the repo-level suites that grep READMEs:
At 34f61d5, one file at a time under the same
env -i … nice -n 19:The suite's
LauncherProcessruns the launcher as a real process over the real connector and the stub app-server: the lock record names the launcher (adapter codex-local,owner_pid= launcher pid), the attached consumer is the connector pid,CODEX_HOMEreached the app-server and the relay token did not, SIGTERM exits 143, the lock readsfreewith no stale record, and the connector is gone.At c8a6a90 (this round), one file at a time under
env -i … nice -n 19, caches cleared:At c666ac3 (previous round), one file at a time under
env -i … nice -n 19, under both Python 3.14 (host) and a Python 3.12 venv for the connect suite:Test plan
codexanswering--version,login statusandapp-server --listen stdio://by exec-ing the package stub; a fake manager whose start verbs run a stand-in holder in another process; a real child for the attach).codex, an owner task through the connector, a second launcher refused with 75, SIGTERM leaving no orphan app-server, uninstall leaving the login andrelay.envuntouched.Rebased onto main ca6c542
connect_common.pyimports (textual conflict): main importsservice_manager as _sm(feat(credentials): one owner for gateway credential precedence and masking (prerequisite for #5665) #5698 moved the manager runner there); this PR addstask_iofor the shared holder sequence. Kept both:from . import _dirs, service_manager as _sm, task_io.test_codex_local_connect.py(semantic, no textual conflict): main movedconnect_common._runintoservice_manager._run, so the codex-local manager fake now patchesservice_manager._run, as the claude-local suite already does. New commit 887848d; no product code changed.claude_code/cli.py,launcher.py,hold.py,ci.ymlandpyproject.tomlmerged automatically; main's changes there (HMAC strict policy, the moved manager helpers) are kept unchanged.Re-run after the rebase, one file at a time: test_codex_local_connect (35 OK), test_codex_connector (50 OK), test_claude_code_adapter (238 OK), test_adapter_boundaries (10 OK), test_adapter_differential (6 OK), test_wheel_contents (PASS), the repo wrappers for codex-local connect (35 OK) and the claude-local wheel (1 OK); ruff clean; scripts/review-checks.sh PASS.
🤖 Generated with Claude Code