Skip to content

fix(auth): reconcile Codex credential transitions - #895

Open
ynadge wants to merge 6 commits into
mainfrom
ynadge/sap-3116-codex-credential-restart
Open

fix(auth): reconcile Codex credential transitions#895
ynadge wants to merge 6 commits into
mainfrom
ynadge/sap-3116-codex-credential-restart

Conversation

@ynadge

@ynadge ynadge commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Primary change type

  • Bug fix
  • Documentation
  • Feature
  • Tests
  • Dependency update
  • Maintenance or refactor

Problem and motivation

The stale-MCP-credential recovery added in #872 only tracked Claude Code launches. Codex now receives the same generated, API-key-bearing Sapiom MCP configuration, so its live sessions can retain stale credentials after login or key rotation and can survive disconnect with a removed credential.

This addresses the unresolved review question on #872: #872 (comment).

Summary and scope

  • stamp Codex launches with the same private credential generation used for Claude Code
  • allow the existing exact-session, resumability-checked restart path to replace stale Codex runtimes
  • make restart diagnostics and internal API comments provider-neutral
  • exercise login, key rotation/restart, and disconnect cleanup for both embedded providers

OAuth cancellation and unrelated authentication changes remain out of scope.

Related work

Related issue or discussion: SAP-3116, #872

Validation

pnpm build — passed
pnpm typecheck — passed
pnpm lint — passed (pre-existing warnings only in unrelated SDK packages)
pnpm --filter @sapiom/harness test — 4,049 standard and 10 performance tests passed
pnpm --filter @sapiom/harness-desktop build — passed
pnpm --filter @sapiom/harness-desktop typecheck — passed
pnpm --filter @sapiom/harness-desktop test — 205 tests passed
pnpm provider-copy:check — passed
pnpm terminology:check — passed
pnpm changeset status --since=origin/main — minor changeset recognized
git diff --check — passed
pnpm test — all reached suites passed except one unchanged @sapiom/agent-core permission test; this cloud sandbox grants CAP_DAC_OVERRIDE to uid 1000, invalidating the test's unreadable-directory premise. The same base passed GitHub's Node 20/22 matrix on #869.

Tests and documentation

Lifecycle and SessionManager regressions now run against Claude Code and Codex. Provider-specific internal comments were updated; no README change is needed because this restores intended provider parity rather than adding a new workflow.

Compatibility and release impact

  • Breaking or externally visible changes: No breaking change. Codex sessions now surface the existing restart-required state after a connection change and are included in disconnect cleanup.
  • Changeset: Added a minor changeset for @sapiom/harness, including Codex restart state, explicit restart, and disconnect termination.

Security

  • I have not included secrets, credentials, private data, or unsanitized logs.
  • This pull request does not publicly disclose a suspected vulnerability. I will follow the Security Policy for private reporting.

AI assistance

  • I did not use AI assistance for this change.
  • I used AI assistance and have described it below.

Codex traced the provider-specific launch/restart path and generated the focused implementation and regression-test edits. I reviewed the complete diff and validated it with the commands above.

Checklist

  • I read CONTRIBUTING.md, and this contribution follows the direct-PR or issue-first policy.
  • This pull request addresses one focused problem and contains no unrelated cleanup.
  • I added or updated tests, or explained above why tests are not applicable.
  • I ran the relevant build, typecheck, lint, and test commands, or explained any N/A checks above.
  • I updated documentation for user-facing changes, or marked it N/A above.
  • I added a Changeset for a published-package change, or explained why it is not applicable.
  • I can explain and maintain every submitted change, including any AI-assisted work.

Refs: SAP-3116

Summary by CodeRabbit

  • New Features

    • Codex sessions now receive the information needed for MCP authentication.
    • MCP credential recovery now supports both Claude Code and Codex sessions.
    • Resumable sessions can restart using the latest credentials after authentication changes.
  • Bug Fixes

    • Stale credentials are reported more clearly.
    • Disconnecting stops credential-bearing sessions and related background tasks.
    • Recovery messages now refer broadly to coding-agent sessions.

@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 46465fd9-dae8-4f58-996c-979f45486506

📥 Commits

Reviewing files that changed from the base of the PR and between 0f28c4e and becd225.

📒 Files selected for processing (8)
  • .changeset/reconcile-codex-connection-restarts.md
  • packages/harness/src/core/session-manager.test.ts
  • packages/harness/src/core/session-manager.ts
  • packages/harness/src/server/auth-mcp-wiring.test.ts
  • packages/harness/src/server/index.ts
  • packages/harness/web/src/components/McpAuthRestartNotice.tsx
  • packages/harness/web/src/lib/api.ts
  • packages/harness/web/src/lib/use-harness-state.ts

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Codex sessions now support MCP credential-triggered restarts, authentication lifecycle handling, disconnect cleanup, and credential launch metadata. Tests cover equivalent Claude Code and Codex behavior. Related documentation uses coding-agent terminology.

Changes

Codex MCP lifecycle

Layer / File(s) Summary
Credential restart support
packages/harness/src/core/session-manager.ts, packages/harness/src/core/session-manager.test.ts
Credential restarts now apply to all harness kinds. Resume errors use the registered harness label. Tests cover Claude Code and Codex runtimes.
Authentication wiring and launch metadata
packages/harness/src/server/auth-mcp-wiring.test.ts, packages/harness/src/server/index.ts, .changeset/reconcile-codex-connection-restarts.md
Authentication tests cover restart-required state, explicit restart, and disconnect behavior for both harnesses. Codex receives MCP credential launch metadata.
Coding-agent recovery documentation
packages/harness/web/src/components/McpAuthRestartNotice.tsx, packages/harness/web/src/lib/api.ts, packages/harness/web/src/lib/use-harness-state.ts
Recovery documentation now refers to coding-agent runtimes instead of Claude runtimes.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to becd2

Codex credential lifecycle handling now aligns with the existing Claude Code flow, with coverage for restart and disconnect behavior. No current merge-blocking risk was identified.

Sequence Diagram(s)

sequenceDiagram
  participant AuthAPI
  participant SessionManager
  participant HarnessAdapter
  participant Runtime
  AuthAPI->>SessionManager: Request MCP credential restart
  SessionManager->>Runtime: Kill stale runtime
  SessionManager->>HarnessAdapter: Resume with current credentials
  HarnessAdapter->>Runtime: Create resumed runtime
Loading

Suggested reviewers: bywallace, gwitwer, evtran0209

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 7 files. (1 skipped: 1 u…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The description follows the required template. It identifies the bug fix, motivation, scope, related work, validation results, compatibility impact, changeset, security status, AI assistance, and chec…
Title check ✅ Passed The title clearly identifies the main change: fixing authentication credential transitions for Codex. It is concise and related to the pull request scope.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ynadge/sap-3116-codex-credential-restart

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

A rabbit watched the runtimes restart
Codex and Claude now share the art
Fresh credentials guide the way
Stale sessions hop away
MCP paths grow clear and bright

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Review — PR #895 (fix(auth): reconcile Codex credential transitions)

The core change is right: Codex genuinely receives the API key (via buildCodexMcpConfig's
env_http_headersSAPIOM_CODEX_MCP_* env binding, packages/harness/src/core/adapters/codex-mcp.ts:120),
its canResume is a real rollout-file check (codex.ts:388), and dropping the
session.harness !== "claude-code" gate in restartForMcpCredentials is sound. Two findings, both
on the published surface.


1. The changeset understates the change, and patch is the wrong level

.changeset/reconcile-codex-connection-restarts.md says only:

Reconcile and restart Codex sessions when their launch-time Sapiom MCP credential becomes stale.

That covers the restart path and nothing else. Removing the req.harness === "claude-code" gate in
packages/harness/src/server/index.ts:666 stamps mcpCredentialLaunch on every launch, which
pulls Codex into two more behaviors the changelog never mentions:

  • Termination on disconnect. terminateCredentialBearingSessions
    (session-manager.ts:904) and terminateCredentialBearingTasks (task-manager.ts:495) now match
    Codex PTYs and Codex background tasks. A live Codex session is killed on sign-out where it
    previously survived. The PR's own tests assert this
    (auth-mcp-wiring.test.ts, "waits for a credential-bearing codex session to exit").
  • mcpAuthState flips for Codex. HarnessSession.mcpAuthState is an exported public type. Codex
    sessions previously reported "not-applicable" always; they now report
    "current" / "restart-required" / "restarting". Any embedder branching on
    "not-applicable" to mean "no Sapiom credential in this process" silently changes meaning.

This matters more than usual because the sibling entry already in .changeset/
reconcile-studio-connection-removal.md — tells consumers the opposite in writing:

…stop affected Studio-managed Claude sessions and background tasks when the current Sapiom
connection is removed.

…and it shipped that behavior as minor. Once both compile into CHANGELOG.md, a consumer reads
a minor entry scoping termination to Claude, then a patch entry that mentions only "restart" — and
concludes their Codex sessions still survive sign-out. That is wrong and unretractable.

Fix: rewrite the changeset body to name all three effects (restart-required state, explicit
restart, and termination on connection removal) for Codex, and raise it to minor to match the
level the identical Claude behavior shipped at. A running session being killed on sign-out is
something a pinned consumer can break on; that belongs in a called-out body, not a patch one-liner.


2. The launch stamp is now unconditional rather than scoped to harnesses that carry the key

server/index.ts:666 went from an explicit claude-code gate to no gate at all. Today
SPAWNABLE_HARNESS_KINDS is exactly ["claude-code", "codex"], so there is no behavioral
difference — but the stamp is now decoupled from the fact it asserts. credentialBearing: apiKey !== null
now means "a key existed at launch", not "this runtime received the key". shared/types.ts:145-147
documents pi and opencode as scaffold adapters waiting on an e2e suite; the day either is added to
SPAWNABLE_HARNESS_KINDS, its sessions are killed on sign-out and shown a "Restart session" banner
whether or not its launch() ever reads opts.mcpConfigFile, with no diff touching this file to
signal it.

Fix: either restore an explicit list (req.harness === "claude-code" || req.harness === "codex")
so adding a harness is a deliberate opt-in, or move the decision onto the adapter (a
consumesMcpConfig flag on HarnessAdapterInfo) so the stamp follows the credential rather than the
launch. A one-line comment explaining that every spawnable harness consumes the generated MCP config
would be an acceptable minimum.


Verdict: logic is correct and the test parameterization is the right shape; block only on the
changeset — it is the one surface here that cannot be fixed after publish.

@BYWallace

Copy link
Copy Markdown
Contributor

@coderabbitai rate limit

@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown

Your plan includes PR reviews subject to rate limits. More reviews will be available in 21 minutes.

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Follow-up review — PR #895 (delta since b3e54af)

Both findings from round 1 are fixed by 735d04a1.

  • Changeset level and body.changeset/reconcile-codex-connection-restarts.md is now minor and its body names all three effects (stale-credential reporting, explicit restart of resumable sessions, termination of credential-bearing sessions and background tasks on disconnect). It no longer reads as restart-only alongside the sibling reconcile-studio-connection-removal.md entry, and the copy stays on the generic role — no confidentiality issue.
  • Unconditional launch stamppackages/harness/src/server/index.ts:666 restores an explicit req.harness === "claude-code" || req.harness === "codex" gate, so mcpCredentialLaunch is again opt-in per harness. A future spawnable adapter now defaults to not stamped, which is the safe direction: it will not be killed on sign-out or shown a restart banner until someone deliberately adds it here.

No new findings in the delta.

Verdict: approve — the two blocking items are resolved and nothing regressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants