Repository navigation
fix(startup): use selected core for auth preflight - #2640
Conversation
f59acd4 to
91eb305
Compare
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
|
Cold review, cloned and verified rather than just reading:
One real gap, not inherited noise: this PR adds Nothing else found. Small, mechanical fix once the src-map is regenerated - not blocking on substance, just flagging the gate that's currently red for a PR-caused reason. |
sonichi
left a comment
There was a problem hiding this comment.
Reviewed at head 91eb3051, on top of 80b7d492.
Premise confirmed on main before endorsing anything. At 80b7d492 the ordering bug is exactly as described:
src/startup.sh:207-261— Claude credential carry (.credentials.json,.claude.jsonseeds, env-token persist)src/startup.sh:292—auth-preflight-gate.sh … || exit $?, the Claude-only gate that aborts startupsrc/startup.sh:531— wherecore_runtimewas actually resolved, ~220 lines later
So a Codex host does reach a Claude /login abort before anything consults core.runtime. Real bug.
Control / treatment. I ran your new suite against main's src/ (test file only, checked out over the old source) and then at your head:
main's src/ + new test : EXIT=1 3 passed, 12 failed
FAIL runtime selection precedes credential carry: select= carry=
FAIL selected auth gate precedes service bootstrap: gate= init=479
PR head 91eb3051 : EXIT=0 15 passed, 0 failed
It genuinely reproduces the failure rather than asserting the new arrangement. All 8 existing tests/startup-*.test.sh suites also pass at head. scripts/review-checks.sh --diff PASS; no hardcoded host paths in the 239 added lines; no absolute /usr/bin/ tool invocations (REVIEW.md lesson 7) — codex is called by bare name.
Three things I checked because they looked wrong and aren't — recording them so the next reviewer doesn't re-derive:
- The
claudearm skips the gate when_claude_config_diris empty, which reads like a new fail-open. It isn't:main:292is already wrapped inif [ -n "${CLAUDE_CONFIG_DIR:-}" ]. Byte-for-byte the same reachability. export CLAUDE_CONFIG_DIR="$_ccd"sits outside the newclaude_auth_carry_enabledblock. That's correct and load-bearing — Codex hosts still needCLAUDE_CONFIG_DIRforchannels/*/access.json, so gating the export would have broken every bridge on a Codex core.- The new
casehard-codesclaude|codex. I checked_SUPPORTED_CORE_RUNTIMESinsrc/sutando_config.py— it is exactly{claude, codex}, so the domain is complete and*)is defensive rather than a live gap.
One non-blocking observation, on blast radius rather than correctness.
resolve_startup_core_runtime() ends in || true, and the call site does [ -n "$core_runtime" ] || core_runtime="claude". resolve_core_runtime() raises on an unsupported value, so an operator who writes core.runtime: "gemini" gets an empty result, silently falls back to claude, and startup proceeds as a Claude core ignoring their stated intent.
That is identical to main's old line 531, so it is not a regression from this PR and I would not hold the merge for it. What changed is the consequence: on main that fallback only chose which binary command -v probed; here it decides whether Claude credential carry runs and which auth gate gates the boot. Same door, larger room behind it. Worth a follow-up that distinguishes "resolver failed" from "resolver returned nothing", rather than folding both into the Claude default.
Nit, non-blocking: the if claude_auth_carry_enabled …; then wrapper leaves the ~70 enclosed lines at their original indentation. Re-indenting would have made the diff much harder to audit, so I think you made the right call — noting it only so it isn't read as an accident.
No blockers from me. Comment-only review — I have no approval authority here; merges are Chi's call.
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Verified independently on a fresh clone of fix/codex-startup-auth-preflight (not the frozen local app-bundled checkout):
$ bash tests/startup-core-auth-routing.test.sh
ok .env runtime override is visible to early selection
ok early selection does not leak unrelated .env values
ok authenticated Codex preflight passes
ok Codex status uses configured CODEX_HOME
ok Codex does not call Claude auth gate
ok logged-out Codex fails closed
ok logged-out Codex names its own remedy
ok Codex skip hatch passes
ok Codex skip hatch executes no CLI
ok Claude preflight still passes through
ok Claude preflight still receives CLAUDE_CONFIG_DIR
ok Codex disables Claude credential carry
ok Claude keeps credential carry
ok runtime selection precedes credential carry
ok selected auth gate precedes service bootstrap
Results: 15 passed, 0 failed
Mutation check on the same checkout — disabled the codex login status branch (if false; then) in preflight_selected_core_auth, reran: 3 of the 15 assertions correctly flip to FAIL (authenticated Codex preflight passes, logged-out Codex fails closed, and its remedy-message check). Confirms the suite is exercising the real gate, not passing vacuously. Restored the file after.
Also traced the "activated wiring" claim past the helper itself: tests/startup-core-auth-routing.test.sh greps the real src/startup.sh for core_runtime="$(resolve_startup_core_runtime)", if claude_auth_carry_enabled "$core_runtime", and preflight_selected_core_auth "$core_runtime" and asserts their line-number order relative to the Claude credential-carry block and init.sh --auto. That's checking the call site is reached in the right place, not just that the extracted functions are individually correct.
Diff is internally consistent: the old direct bash "$REPO/src/auth-preflight-gate.sh" "$CLAUDE_CONFIG_DIR" call and the old duplicate core_runtime=$(...) resolution near the L527 CLI-presence check are both removed in favor of the single early resolution + dispatch — no leftover duplicate logic. Docs (README.md, docs/codex-core.md, docs/runtime-dependencies.md, docs/catalog.json last_verified bump) match the new behavior.
The two failing CI checks (refuse docs/src-map.md stale vs src/, tsc + tests (clean install)) are the known inherited main-red src-map count-line staleness affecting every open PR right now (tracked by #2635/#2638) — this PR adds no new src/ module and isn't the cause; confirmed via gh pr diff --name-only (only startup-runtime.sh, startup.sh modified, no new src files) and the failing job's own log showing docs/src-map.md is stale as the sole failure.
Approving.
# Conflicts: # docs/src-map.md
|
Addressed the source-map comment and refreshed onto current
The branch is current; hosted checks and fresh exact-head approvals are now required. |
|
Refreshed this release-gate branch onto current Independent post-refresh verification from a detached worktree: The focused Codex route still proves early runtime selection, authenticated/logged-out fail-closed behavior, Claude-gate exclusion, configured Hosted checks are rerunning. Prior approvals predate this merge-only head, so please review exact |
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 206724a after its current-main refresh.
The full current diff and startup/auth paths are sound. Runtime selection now precedes Claude credential carry, Codex exports the same configured CODEX_HOME as its launcher and checks codex login status, Claude retains its existing gate, and the shared skip hatch does not execute the Codex CLI. The focused 15-case routing suite, shell syntax, diff hygiene, and REVIEW.md hardcoded-path checks pass; completed hosted gates and CLA are green.
Worst case is a Codex host being blocked by Claude credentials or authenticating against the wrong home; the selected-runtime ordering, no-Claude-call, configured-home, logged-out, and skip controls cover those paths. Code approved. Clean-install is still running and merge state remains blocked, so this is not yet ready to merge under the complete repository gate.
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head fc9bd1e17462aab849ec6643b9d5cb957f2b539d.
No blocking findings. This is a merge-only refresh of the previously approved startup auth-preflight topic, now incorporating #2674. I compared the complete effective topic patch before and after both refreshes; the normalized file/patch fingerprint is identical. The prior focused 15-case startup/auth suite and mutation check therefore still exercise the exact production changes. The worst-case user impact remains a startup refusal for an unauthenticated selected core, which is intentional and includes the core-specific remedy plus an explicit skip hatch.
APPROVED for code. Merge-ready only after hosted checks are green and the repository's second current-head maintainer approval requirement is satisfied.
Reviewed by Qingyun's Personal Codex.
|
@cla-assistant check |
What changed, and why?
startup.shselectedcore.runtimeonly after it had copied Claude credentials and run the Claude-only boot gate. A host configured for Codex could therefore abort with a Claude/loginremedy even thoughcodex login statuspassed.This change resolves the runtime before credential handling, keeps Claude credential carry Claude-only, and dispatches the early auth gate by runtime. Codex now exports the configured
CODEX_HOMEand checkscodex login statusbefore any service bootstrap. Claude keeps the existing rich preflight unchanged.Review first
src/startup-runtime.sh: early runtime selection and selected-core preflightsrc/startup.sh: activated ordering and Claude-only credential carrytests/startup-core-auth-routing.test.sh: runtime isolation, fail-closed, escape-hatch, and activated-wiring coverageUser behavior proved
A Codex-configured startup no longer reads/copies Claude login state or asks the owner to authenticate Claude. It fails with a Codex-specific remedy only when Codex itself is unavailable or logged out.
Documentation consistency
Updated the canonical runtime dependency and Codex setup docs, their
docs/catalog.jsonverification dates, and the README setup guidance.Before / after evidence
Parent
0604188b, real isolated startup withSUTANDO_CORE_RUNTIME=codex:HEAD
91eb3051, same real startup path with an isolated workspace/socket/session (GUI commands suppressed so the test did not launch Sutando.app):Real post-startup task/result round trip through the isolated Codex core and notifier:
The activated regression test run against the parent source fails (
3 passed, 12 failed); the same test at HEAD passes:Tests
Additional startup regression scripts passed: re-migrate loop, self-log temp path, shell setup wiring, stand-identity self-heal, and voice optionality.
python3 scripts/gen-src-map.py --checkremains red on both parent and HEAD because the checked-in map says 193 modules while current main generates 194; this PR does not add asrc/module and does not bundle that pre-existing documentation drift.Edge cases
.envruntime override participates in early selection without leaking unrelated.envvalues into the parent shell.codex login.SUTANDO_SKIP_AUTH_PREFLIGHT=1skips the early check without invoking a CLI.Risk / rollback
No migration or permission change. Roll back this commit to restore the old ordering. The shared one-run recovery escape hatch remains available.
Stacking / known gaps
Not stacked. This is intentionally one concern. The separate headless-startup issue (startup currently tries to launch the macOS app) is not bundled here and should be handled independently.