Repository navigation
Conversation
…eprecated no-op `ag2-sparrow-codex-local connect` no longer refuses without the hidden --experimental-connect flag. The gate was added in #5871 while three ownership windows on the shared start/undo path were open; #6151 and #6204 closed them, #6143 moved every refusal before the code is spent, and #6145 and #6240 made one agent one bridge and one job. The old flag is still accepted, prints one deprecation warning and does nothing else; it is never carried into a printed rerun command. --agent stays required and every safety check is unchanged. README and help text drop the "experimental, off" wording. Tests: connect runs flagless under systemd and launchd; the deprecated flag warns exactly once and connects the same; a failed connect's rerun line never carries the flag. The bridge-exclusive and seam suites now run codex connect without the flag. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 92%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed at 516e04d. Approving: the change is correct and removes only the gate.
What I checked
connect_cli.py:211-218: the only behavior change is the gate block. The oldif not args.experimental_connect: return EXIT_UNUSABLEis now a stderr warning when the flag is given. Everything after it is unchanged at this head: the--basecheck,cb.fenced(args.agent, ...),redeem_into_seatwithconnect_preflightrunning before the code is spent, andfinish_connect. No safety check was moved past redemption.- The deprecated flag is still accepted.
connect_cli.py:389keeps--experimental-connect(store_true, help suppressed), so existing scripts don't hit an argparse error.connect_bridge.py:309still listsexperimental_connectin_NOT_SESSION, sosession_flags()never puts it into the reruninstallhint.test_the_install_a_failed_connect_names_never_carries_the_deprecated_flagpins that. --agentis still required before the code is read.test_bridge_exclusive.py:286(test_a_connect_without_agent_is_refused_before_the_code_is_read) now runs without the flag for both adapters, so it exercises the real default path rather than the flagged one. The renamedconnect_switchtests (:240,:243) also run flagless.- The tests cover both modes:
test_connect_runs_without_any_flag_on_both_managers(new default) andtest_the_old_experimental_flag_is_a_deprecated_no_op_that_warns_once(old flag). Each runs under both managers throughMANAGERS, without forcingis_macos. Each asserts one redemption,rc == 0, and the unit's suffix. The fail-first evidence in the body matches what the gate change would produce. - The docs match. The README usage block and the "Connect with a code" paragraph, the module docstring (
:16), the subparser help (:387) and the CHANGELOG-PENDING entry all describe the new default and the deprecated no-op the same way. Nothing else in the tree still tells users to pass--experimental-connect. - CI: all blocking checks pass at this head (46 pass, 3 skipped release/Windows jobs). review-checks: hardcoded-paths and root-artifacts are clean.
Notes (non-blocking)
- A bare
connectwith no arguments used to fail with the gate'sEXIT_UNUSABLEbefore any argument check. It now fails on argparse's--baseerror (exit 2). That's expected now that the gate is gone. I mention it only in case a caller keyed on the old exit code. _connectstill passesexperimental_connectthroughsessioninto theinstallNamespace. That's harmless because install never reads it, and it was already the case before this PR.- The body's own merge gate is still open: a live Codex
connectround trip at the final SHA (redeem, then bridge, then task and result). This approval covers the code and the tests, not that witness. Please record it on the PR before merging.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head 516e04d43282d729ca40c622eecb250f4769945c.
Requesting changes — this is not merge-ready.
-
The newly default-on path can remove the bridge from a live foreground Codex session. I reproduced flagless Codex
connectat this exact head in all four systemd/launchd × unit/sparrowd combinations. The supported rotation sequence was: initial connect succeeds; stop the managed launcher while its bridge remains; reconnect's manager accepts the new start but no managed launcher acquires the root; a queued foreground Codex session acquires it before pending-start failure cleanup. All four runs returned nonzero with the holder alive, but the unit/plist was deleted or the sparrowd declaration withdrawn.The path is direct:
connect_cli.py:218-233reachesfinish_connect;connect_bridge.py:761-776releases the redemption lock and callscm.settle;connect_common.py:1884-1892runs the pending failure callbacks registered atconnect_bridge.py:539-577._unstarted/_withdrawremoves the artifact withoutorphan_bridge's live-holder check. The earlier nested settle is deferred while redemption still holds the lock, andfinish_connecthas no final holder-aware reconciliation. Worst case: credential rotation leaves an apparently live agent unable to receive tasks or return results. Route destructive pending cleanup through the single final reconciliation decision and add this connect-specific interleaving regression across both managers and bridge hosts before removing the gate. -
The reason-6 closure claim is false for an agent root not yet recorded by Codex. Required
--agentalready determines the canonical root, butknown_root_problem()checks root/seat state only when this adapter's config already recordsROOT(connect_bridge.py:683-695). The exact-head test attest_codex_local_connect.py:1413-1426proves the consequence: when the other adapter holds that agent root, Codex connect makes one backend redemption call and then reports that the code is spent. I reran the test at this head. Preflight the root derived from required--agentbefore reading/redeeming the code, while retaining the post-redemption race check. -
The mandatory final-SHA live witness is still absent. The PR body says the real redemption → bridge → task/result round trip remains open.
REVIEW.mdmakes that a review-time gate. Run it after the failure seams above are fixed; a happy-path witness alone cannot cover them. -
Both commits lack the repository-required
Sutando-Task:trailer.
Positive evidence: the exact-head 124-test Codex suite, 22 bridge-exclusive tests, and 13 adapter-boundary tests pass; the targeted flag/no-flag tests, fail-first control, all five claimed mutations, review checks, CLA, coverage, and visible CI are green. Those validate the intended deprecation behavior, but not the activated lifecycle path.
A fixed head with failure-mode regressions, the required trailer, and the post-fix final-head witness would change this verdict.
…gent's root is checked before the code is spent 1. A pending start that fails withdrew what waited on its session (the bridge unit or the sparrowd declaration) unconditionally. With connect on by default, a reconnect whose managed launcher never took the root while a foreground session did deleted that session's bridge. The withdrawal now runs under the agent lock only while no session serves the root (connect_common.serving_session, the same predicate orphan_bridge uses); otherwise the bridge is kept and a bridge unit this attempt stopped is started again. connect's install no longer runs its bridge reconciliation inside the redemption's lock, before the start settled: finish_connect runs it after the settle. 2. known_root_problem also checks the root the required --agent names (its agent lock and who holds it) before the code is read, not only a root the adapter's config recorded. The post-redemption check stays. Tests: a reconnect whose launcher never takes the root, on systemd and launchd, unit and sparrowd: a foreground holder keeps its unit/plist or declaration (and its stopped bridge runs again); with no holder the bridge is withdrawn. A root another adapter holds is refused with zero backend calls, in both adapters' suites; a root taken after the preflight is still refused after the redemption. Sutando-Task: task-010a8e322d193403e0 Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@keweichen thank you. Both blockers reproduced exactly as you described. Round 2 is at 1. A failed reconnect removed a live foreground session's bridge: fixed.
2. The root named by
3. Live witness: it runs at the final head after these fixes, and its result will be posted here before approval. The PR body says so. 4. Suites (local, focused):
All OK; ruff and review-checks are clean. I'll report CI, coverage and CLA once they finish. @qingyun-wu: this round changes the shared start-cleanup and preflight code since your approval at (agent-assisted, Claude Code for Chi Wang) |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 998dae67d4831da9df0a678e9340a35551ffcb0e (round 2 on top of 516e04d4). Read the round-2 delta to the shared connect code end to end, plus the activated paths it reaches (under_agent_lock, settle/await_started, _reconciled, orphan_bridge, redeem_into_seat).
Verdict: the code looks correct and I see no blocker. Not approvable yet: REVIEW.md lesson 15 makes the live round trip a review-time gate for a bridge/connect path, and the PR body itself says approval waits for it. Posting the real connect witness at this head (redeem → bridge → task → result) is what would turn this into an APPROVE. The earlier APPROVED review at 516e04d4 is stale: this round changes shared cleanup and preflight code, so it should not count as approval of this head.
Round 2, item 1 (a failed reconnect removed a live session's bridge): fixed as described
connect_common.py:1889: the failed-start cleanup is now_undo_start(p) + _unwaited(p)under the agent lock. The order matters and is correct: the undo runs first, soserving_sessionsees the post-undo manager state.connect_common.py:1905-1924:serving_sessiontreats an active job, a live holder, and an unreadable holder as "served". All three keep the bridge, so an ambiguous read fails toward keeping it, not tearing it down.orphan_bridge(connect_bridge.py:453) now uses the same predicate, so both teardown decisions use one rule and are not two copies that could drift.connect_bridge.py:407-409+finish_connect:782-799: inside connect, the install's reconciliation is deferred through_FINALand runs aftercm.settle, outside the redemption's lock.finish_connectclears the per-thread entry on entry, so a leftover entry from an earlier call cannot leak in.wire_bridge:569-595:restoreis registered only when the bridge unit wasactivebefore this attempt stopped it. A bridge that was not running is not started by the kept path.- The
lost_reconnectmatrix (2 managers × unit/sparrowd × with/without a foreground holder) exercises the reported interleaving, with the no-holder controls. The fail-first and mutation results in the round-2 comment match what the diff would produce.
Round 2, item 2 (preflight the root named by --agent): fixed
known_root_problem(local, agent_id)(connect_bridge.py:701-716) checks the lock and holder for both the recordedROOTandnamed_root(agent_id), beforeread_code. The seat check stays scoped to the recorded root.test_a_root_another_adapter_holds_is_refused_before_the_code_is_readasserts zero backend calls.test_a_root_taken_after_the_preflight_is_refused_before_the_seat_is_writtenkeeps the post-redemption race covered.
Non-blocking notes
connect_common.py:1907-1909: when_undo_startdeclines to stop a job it cannot prove it owns, and that job isactivebut never took the root,serving_sessionreports "served", and the bridge is kept and possibly restarted with no session on the root. This fails toward keeping the bridge, which I think is the right side. A one-line mention in the residual-risk list would make it explicit.connect_bridge.py:577restorerestarts the bridge from the unit file this attempt just rewrote. In the tested case the bytes are identical (the test asserts it). If a future change makes the rewritten unit differ, for example a new seat path, the kept path would start the new unit, not the old one. Consider a comment, or assert this in the helper._FINALis a module-level dict keyed by thread id. It is fine for the CLI's single thread. It would need a lock if connect ever ran concurrently in-process.
Evidence
- CI at this head: 46 pass, 3 skipped (release and Windows jobs), none cancelled or timed out. Coverage gate passes.
- Locally at this head: the codex-local connect, claude-local connect and bridge-exclusive suites: 293 passed, 215 subtests passed.
- Worst-case disruption (lesson 5): connect is now on by default for Codex. The new worst case is a failed connect leaving a bridge running while a foreground session holds the root. That is the intended state, and no worse than before.
--experimental-connectis still accepted as a no-op, so existing scripts do not break.
Outstanding before approval: the final-head live witness. keweichen's CHANGES_REQUESTED also needs that reviewer to re-check.
|
@cla-assistant check |
Live witness at 859f74a (codex-local, token path): PASSThe owner approved this run. The token path is the primary one, since the app issues no
On Not exercised: a real Cleanup: the venv, worktree, token file and agent work root were removed, and a scratch vault entry from an earlier attempt was deleted. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 859f74a7bff4dd39697e6d3e7898d783ea41bafe, including round 2 and the main merge. No new code blocker found; the relay-token lifecycle now has the real post-restart witness required by REVIEW.md lesson 15.
The exact-head witness records install, verify, a collaborator task/reply, a session kickstart, a second task/reply, and uninstall. That satisfies the lesson's real-message-through-the-restarted-service requirement for the relay-token path. It is author-provided evidence, not a live run I performed. It does NOT prove an acp. redemption: the final flagless command stops at missing-code validation. I am not counting that refusal as a successful connect. The API-only mint route creates a new agent and is not exposed by the checked app sources; redemption remains harness-verified.
Code checks: connect_common.py:1889,1905-1924 re-reads serving state after undo under the agent lock, preserving a live or unreadable holder's bridge. connect_bridge.py:407-409,782-799 defers connect reconciliation until after settlement; :701-734 checks the agent-named root before reading/spending the code. The eight reconnect interleavings and the preflight/post-redemption race cases ran in the local Codex suite. Worst-case disruption is a failed rotation interrupting delivery; the holder-aware keep/restore and no-holder teardown controls address that. The old flag remains accepted and there is no state migration.
The main merge has an empty remerge diff and no changes to the three connect implementation files relative to round 2. It imports main's trigger-header support, not additional connect edits; the imported agent-trigger tests pass locally, and the live witness is after that merge.
Validation: review checks pass. The local Codex suite collected 133 tests, with three startup/seam failures; all three pass unchanged in a focused rerun (3/3). Two failures explicitly report the fake launcher's two-second acquisition timeout; I do not report the initial full suite as clean. Exact-head CI and CLA are green (46 successful checks, three conditional skips).
Verdict: code and relay-token runtime evidence support approval at this head. This updates the earlier review's evidence assessment; it does not assert live acp. redemption or clear another reviewer's outstanding change request. Normal approval/merge gates still apply.
|
Closing for now, on the repository owner's decision. Turning on the one-time-code 🤖 Generated with Claude Code |
What changed, and why
ag2-sparrow-codex-local connectworks without--experimental-connect. #5871 gated it off while ownership windows on the shared start/undo path were still open. Those fixes have since landed on main (checklist below), so the gate comes off.connectno longer refuses without the flag.--experimental-connectis still accepted, so existing scripts keep working. It is a deprecated no-op: it prints one line,warning: --experimental-connect is deprecated and does nothing: connect is on; drop the flag, and changes nothing else. It is never carried into a rerun command that a failed connect prints.--agentis still required. No safety check is removed or weakened.516e04d4), shared connect code:connect_common.serving_session, the same predicateorphan_bridgenow uses). Otherwise the bridge is kept, and a bridge unit this attempt stopped is started again. connect's install also stopped running its bridge reconciliation inside the redemption's lock, before the start settled;finish_connectnow runs it once, after the settle.--agentnames is preflighted before the code is read.known_root_problemnow checks that root's agent lock and holder too, not only a root the adapter's config recorded. The post-redemption check stays, for a root taken in between.Why the gate existed, and what closed each reason
start_supervisor()was claimed as this attempt's own, and a rollback could stop it. On launchd this was a regression against the earlier main.InvocationIDplus a start inside our call._undo_startrefuses unless it holds the agent lock._retire_runsignals only the run its own start began, by exact pid and start time. #6204: no step names the job to its manager, and the bridge is never stopped by name on the root-replaced branch.is_macos=False, so the "launchd" cases never ran the launchd branch.install --token-env-file.install --token-env-filereaches the same start/undo windows.--agentis required, and the owner check runs before the code is spent. #6240: one job per agent, so several agents can share a host.The closing PRs also list what they could not fix, because the managers have no atomic primitive for it: systemd start attribution inside one call window, the macOS pid-reuse window without a pidfd, and a retire that depends on the launcher cooperating. Those are all in #6151's residual-risk section and are not specific to Codex.
Turning connect on by default also exposed a ninth issue, which keweichen reproduced on all four manager × bridge-host combinations: a failed reconnect removed a live foreground session's bridge. This PR fixes it (round 2, item 1).
Live witness (REVIEW.md gate): the audit asked for a live witness at the final SHA: a real one-time-code redemption, then the bridge, then a task and its result. The witnesses on #6151 and #6240 used
install --token-env-file, not a redemption. A live Codexconnectround trip (redeem → bridge → task/result) will run at this PR's final head, and approval will be requested only after it is posted here.Where to look first
packages/ag2-sparrow/ag2_sparrow/connect_common.py:serving_session,_unwaitedand their call inawait_started. Thenconnect_bridge.py:wire_bridge'srestore,known_root_problem,under_agent_lockandfinish_connect(_FINAL).packages/ag2-sparrow/ag2_sparrow/adapters/codex/connect_cli.py:connect()and theconnectsubparser.packages/ag2-sparrow/tests/test_codex_local_connect.py:lost_reconnectand its 8 generated tests,connected_on, the two newConnectCodetests, andtest_the_install_a_failed_connect_names_never_carries_the_deprecated_flag.packages/ag2-sparrow/ag2_sparrow/adapters/codex/README.md: the "Connect with a code" paragraph and the usage block.User behavior this proves
ag2-sparrow-codex-local connect --base <url> --agent <id>redeems the code once, writes the seat, installs, and the launcher acquires the agent's root. This holds under systemd and under launchd.connect --experimental-connect …behaves the same and warns exactly once.connectwithout--agentis still refused before the code is read (test_a_connect_without_agent_is_refused_before_the_code_is_read, now run without the flag, for both adapters).A reconnect whose managed launcher never takes the root, while a foreground session does, fails (rc ≠ 0) but leaves that session its bridge: the unit/plist is kept and the bridge it stopped runs again, or the sparrowd declaration is kept byte for byte. With no session on the root, the bridge is withdrawn as before. Both hold on systemd and launchd.
A root the other adapter holds is refused with zero backend calls, and the code stays unused, in both adapters' suites.
Feature/documentation consistency
The adapter's own README is updated.
docs/catalog.jsonhas no entry for the codex-local adapter, so nodocs/page changes. CHANGELOG-PENDING has an entry under Changed.Before/after evidence
Fail-first: the new tests run against
origin/main'sconnect_cli.py, with only that file reverted:At the head:
Ran 2 tests … OK.Mutations. Each was applied to the head and restored afterwards:
Round 2 evidence
Fail-first for item 1: the new interleaving tests run against
516e04d4'sconnect_common.pyandconnect_bridge.py. This reproduces the review's finding in all four combinations; the no-session controls pass on both sides:At the head:
Ran 8 tests … OK.Fail-first for item 2: the changed test fails in both suites at
516e04d4:(test_codex_local_connect and test_claude_code_connect each show 1 failure). Both pass at the head.
Mutations (each applied at the head, then restored):
Three existing tests changed expectation because the refusal now comes earlier.
test_a_job_active_for_another_root_does_not_vouch_for_a_held_rootandtest_an_independent_holder_of_this_shape_on_the_root_is_refused_while_the_job_serves_anotherare now refused before the code is spent. In both suites,test_a_holder_that_takes_the_root_after_the_clean_check_keeps_the_seatnow takes the root after the post-redemption check, which is the check it targets.Tests run
These are focused suites only, on Python 3.14 under
nice -n 19andenv -i. Full discovery and the version matrix are left to CI.Test changes: the old flag-off refusal test is replaced by the two new tests. The
ConnectCodeand bridge-exclusive helpers drop the flag, so every existing codex connect test and seam test now runs flagless. Two bridge-exclusive tests are renamed (test_codex_connect_is_refused_before_the_code_is_spent_{systemd,launchd}).Edge cases
--agentor a missing--baserefuses as before. The deprecation warning, when the flag is given, comes first.Risks and rollback
There is no migration, config change or new env var. To roll back, revert this PR:
connectthen refuses again unless the flag is given.Known gaps
Commit trailers
The round-2 commit carries
Sutando-Task:. The first two commits (a10f562c,516e04d4) predate the trailer and are not rewritten, since that would need a force-push.Written by Claude Code (Claude Opus 5.5) for Chi Wang, who is accountable for it.
🤖 Generated with Claude Code