Skip to content

fix(auth): point Keychain-only legacy Codex OAuth users at doctor instead of auto-prompting#86220

Merged
RomneyDa merged 1 commit into
mainfrom
codex-oauth-keychain-fail-fast
May 25, 2026
Merged

fix(auth): point Keychain-only legacy Codex OAuth users at doctor instead of auto-prompting#86220
RomneyDa merged 1 commit into
mainfrom
codex-oauth-keychain-fail-fast

Conversation

@RomneyDa

@RomneyDa RomneyDa commented May 24, 2026

Copy link
Copy Markdown
Member

Summary

Alternative to #85163 closing the same bucket from #85083 (macOS users whose legacy oauthRef sidecar seed lives only in the Keychain), but without an auto-prompt startup hook.

#85163 reuses the existing maybeRepairLegacyOAuthSidecarProfiles doctor flow as a pre-command hook in runCli, so every non-skipped interactive openclaw invocation on darwin can fire a Keychain migration prompt. That works, but trades a wide and growing skip surface (five ClawSweeper rounds, --non-interactive/--yes/--no-input/--force/--json/channels remove --delete and counting) for the auto-heal property — and it violates the AGENTS.md rule:

Legacy config repair belongs in openclaw doctor --fix, not startup/load-time core migrations.

This PR takes the inverse design: keep doctor as the only place that migrates, and make sure affected headless users see a single, actionable error pointing at it instead of a silent fall-through to No API key found for provider "openai-codex".

Approach

Three concrete pieces, ~40 lines net:

  1. One-shot headless warn in src/agents/auth-profiles/legacy-oauth-sidecar.ts — when loadLegacyOAuthSidecarMaterial returns null on darwin with allowKeychainPrompt: false (i.e., env/file seeds were tried and Keychain was blocked), emit a single log.warn per process naming openclaw doctor --fix and macOS Keychain. Rate-limited so embedded turns log it once per gateway lifetime, not per attempt. Guarded by the same VITEST env signals as the keychain read so unrelated test runs don't surface it.
  2. Wording cleanup in src/commands/doctor-auth-oauth-sidecar.ts — drop the user-confusing sidecar-backed phrasing from the doctor prompt, notes, and changes (users don't know what a sidecar is); internal identifier names are left intact.
  3. Docs/changelog — update docs/gateway/doctor.md accordion 5 to mention the new headless warning alongside the existing doctor-driven migration; add a single CHANGELOG bullet.

What this deliberately doesn't do

  • No runCli pre-command hook. No new file in src/cli/.
  • No interactive prompt outside openclaw doctor --fix. Users who upgrade into the Keychain-only state see one warning telling them what to run; doctor remains the single place that prompts.
  • No decline marker, no env opt-out (OPENCLAW_AUTO_MIGRATE_LEGACY_OAUTH_SIDECAR), no skip ladder. Nothing to bypass because nothing prompts.
  • No change to maybeRepairLegacyOAuthSidecarProfiles semantics. Doctor still migrates exactly the same way it has since Keep legacy Codex OAuth sidecar profiles usable #83312.

Trade vs. #85163

#85163 (auto-prompt hook) This PR (fail-fast pointer)
User has to know about openclaw doctor --fix No — prompt fires on next interactive command Yes — one log line tells them
Surface where prompts can appear Any non-skipped CLI command on darwin None (only openclaw doctor --fix)
Skip-rule maintenance Growing list (channels remove --delete was the latest) None
openclaw status can surprise-prompt the user Yes No
Violates AGENTS.md "Legacy config repair belongs in doctor" Yes (intentionally) No
LOC added +860 / -10 / 10 files +183 / -10 / 7 files

The cost of this design is honesty: we tell affected users they have one command to run instead of trying to auto-heal them silently. That cost is bounded — the headless warn is loud, actionable, and survives across restarts; openclaw doctor --fix is the one-line remediation. The benefit is the entire ClawSweeper skip-rule treadmill plus the AGENTS.md tension go away.

Regression tests

  • src/agents/auth-profiles/legacy-oauth-sidecar.test.ts (new) — exercises the headless warn path: emit once when blocked on darwin with no env/file seeds; never emit on non-darwin; rate-limited per process.

Verification

  • node scripts/run-vitest.mjs src/agents/auth-profiles/legacy-oauth-sidecar.test.ts src/commands/doctor-auth-oauth-sidecar.test.ts src/commands/doctor/repair-sequencing.test.ts — 21/21 green (4 files; one is a co-resident sibling).
  • node scripts/run-oxlint.mjs clean on all touched files; oxfmt --check clean.

Related

@RomneyDa RomneyDa requested a review from a team as a code owner May 24, 2026 22:43
@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation gateway Gateway runtime commands Command implementations agents Agent runtime and tooling size: S maintainer Maintainer-authored PR labels May 24, 2026
@clawsweeper

clawsweeper Bot commented May 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed May 24, 2026, 6:57 PM ET / 22:57 UTC.

Summary
The branch adds a one-shot Darwin headless warning in the legacy Codex OAuth sidecar loader, updates doctor wording/docs/changelog, and adds focused Vitest coverage for the warning path.

PR surface: Source +30, Tests +142, Docs +1. Total +173 across 7 files.

Reproducibility: yes. from source inspection: current main skips Keychain reads when allowKeychainPrompt is false and then returns null for encrypted legacy sidecars that cannot decrypt from env/file seeds. I did not run a live macOS Keychain scenario in this read-only review.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Maintainer should decide whether warning-only doctor guidance is enough to resolve the linked Keychain-only issue.

Risk before merge

  • The PR intentionally leaves affected Keychain-only users needing to run openclaw doctor --fix; maintainers should decide whether that warning-only remediation is enough to close the linked issue's original self-heal acceptance criteria.
  • The warning infers the Keychain-only bucket from failed env/file seed decryption plus a blocked Keychain read, so the wording may be over-specific for other undecryptable Darwin legacy sidecars.

Maintainer options:

  1. Decide the mitigation before merge
    Keep migration owned by doctor, and merge this warning path only if maintainers accept warning-only remediation as the right resolution for the remaining Keychain-only bucket; otherwise keep self-heal tracked separately.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge
Protected maintainer-labeled auth work needs a maintainer decision on the warning-only resolution; there is no narrow automated repair to queue.

Security
Cleared: The diff adds warning text, docs/changelog wording, and tests; it does not add dependencies, scripts, token logging, permissions, or credential writes.

Review details

Best possible solution:

Keep migration owned by doctor, and merge this warning path only if maintainers accept warning-only remediation as the right resolution for the remaining Keychain-only bucket; otherwise keep self-heal tracked separately.

Do we have a high-confidence way to reproduce the issue?

Yes from source inspection: current main skips Keychain reads when allowKeychainPrompt is false and then returns null for encrypted legacy sidecars that cannot decrypt from env/file seeds. I did not run a live macOS Keychain scenario in this read-only review.

Is this the best way to solve the issue?

Mostly yes if the maintainer decision is warning-only remediation: the patch keeps migration in doctor and avoids startup prompts. It is not the original self-heal acceptance path, so the product decision should be explicit before merge.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 8ae997749d3a.

Label changes

Label justifications:

  • P2: The PR addresses a narrow but real auth-provider diagnostic gap for legacy macOS Codex OAuth users without broad runtime churn.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This is a member-authored maintainer-labeled PR, so the external-contributor real behavior proof gate does not apply; the PR body reports targeted Vitest, oxlint, and oxfmt verification.
Evidence reviewed

PR surface:

Source +30, Tests +142, Docs +1. Total +173 across 7 files.

View PR surface stats
Area Files Added Removed Net
Source 2 33 3 +30
Tests 3 148 6 +142
Docs 2 2 1 +1
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 7 183 10 +173

What I checked:

Likely related people:

  • joshavant: Merged history for the central auth surface shows 06f4c971... introduced the legacy Codex OAuth sidecar compatibility code this PR extends. (role: feature introducer; confidence: high; commits: 06f4c97130c6; files: src/agents/auth-profiles/legacy-oauth-sidecar.ts, src/agents/auth-profiles/persisted.ts, src/agents/auth-profiles/store.ts)
  • RomneyDa: Recent merged auth-provider work on the same legacy sidecar and fail-fast surfaces landed through 4399eee... and 205c595..., and this PR is a focused follow-up on that path. (role: recent area contributor; confidence: high; commits: 4399eee6e0e4, 205c595b134c, 3ab58dcfcf10; files: src/agents/auth-profiles/store.ts, src/agents/auth-profiles/oauth-manager.ts, src/agents/auth-profiles/legacy-oauth-sidecar.ts)
  • Totalsolutionsync: The related embedded Codex OAuth repair was credited as originating from their live gateway work and was co-authored into the single-purpose loader fix. (role: adjacent contributor; confidence: medium; commits: 4399eee6e0e4; files: src/agents/auth-profiles/store.ts, docs/gateway/doctor.md)
  • Vincent Koc: Current blame in this shallow checkout points the central auth-sidecar files at the latest grafted main snapshot, so this is a weak routing signal for recent maintenance rather than original design ownership. (role: recent area contributor; confidence: low; commits: ce48e4c197e5; files: src/agents/auth-profiles/legacy-oauth-sidecar.ts, src/commands/doctor-auth-oauth-sidecar.ts, docs/gateway/doctor.md)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. labels May 24, 2026
@clawsweeper

clawsweeper Bot commented May 24, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg

✨ Hatched: 🥚 common Clockwork Signal Puff

Hatch command

Comment @clawsweeper hatch when this PR is hatchable.

Hatchability rules:

  • Merged PRs are hatchable.
  • Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
  • Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.

Rarity: 🥚 common.
Trait: sparkles near resolved comments.
Image traits: location release reef; accessory shell-shaped keyboard; palette moonlit blue and soft silver; mood sparkly; pose standing beside its cracked shell; shell matte ceramic shell; lighting soft underwater shimmer; background smooth stones and checkmarks.
Share on X: post this hatch
Copy: My PR egg hatched a 🥚 common Clockwork Signal Puff in ClawSweeper.

What is this egg doing here?
  • Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
  • The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
  • Hatchability usually comes from sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness. A merged PR is already final, so merge makes the egg hatchable independently.
  • The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
  • Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

@RomneyDa RomneyDa mentioned this pull request May 24, 2026
Closed
@RomneyDa RomneyDa merged commit a61d530 into main May 25, 2026
163 of 170 checks passed
@RomneyDa RomneyDa deleted the codex-oauth-keychain-fail-fast branch May 25, 2026 18:39
@github-actions github-actions Bot mentioned this pull request May 25, 2026
Open
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request May 26, 2026
@RomneyDa
fix(auth): emit one-shot doctor-pointer warning for Keychain-only leg…
eb12f65 
…acy Codex OAuth profiles (openclaw#86220)
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 26, 2026
@RomneyDa @SebTardif
fix(auth): emit one-shot doctor-pointer warning for Keychain-only leg…
b4e98a7 
…acy Codex OAuth profiles (openclaw#86220)
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 26, 2026
@RomneyDa @SebTardif
fix(auth): emit one-shot doctor-pointer warning for Keychain-only leg…
a44c35c 
…acy Codex OAuth profiles (openclaw#86220)
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 26, 2026
@RomneyDa @SebTardif
fix(auth): emit one-shot doctor-pointer warning for Keychain-only leg…
23feaa4 
…acy Codex OAuth profiles (openclaw#86220)
jameslcowan pushed a commit to jameslcowan/openclaw that referenced this pull request Jun 2, 2026
@RomneyDa
fix(auth): emit one-shot doctor-pointer warning for Keychain-only leg…
ef9f048 
…acy Codex OAuth profiles (openclaw#86220)
SYU8384 pushed a commit to SYU8384/openclaw that referenced this pull request Jun 3, 2026
@RomneyDa
fix(auth): emit one-shot doctor-pointer warning for Keychain-only leg…
e54e248 
…acy Codex OAuth profiles (openclaw#86220)
sablehead pushed a commit to sablehead/openclaw that referenced this pull request Jun 10, 2026
@RomneyDa
fix(auth): emit one-shot doctor-pointer warning for Keychain-only leg…
9f509ae 
…acy Codex OAuth profiles (openclaw#86220)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling commands Command implementations docs Improvements or additions to documentation gateway Gateway runtime maintainer Maintainer-authored PR P2 Normal backlog priority with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Legacy Codex OAuth sidecars stored only in macOS Keychain still require doctor for embedded runtime path

1 participant

@RomneyDa