feat(codex): add account namespace foundation - #512
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
eab2c52 to
227e6df
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
227e6df to
c19ba6c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
c19ba6c to
0718812
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
0718812 to
b3aa8c6
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
b3aa8c6 to
9edaf62
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
9edaf62 to
aef5628
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
@Wibias Addressed both blockers from the full security re-review in signed commit
I also fixed CodeRabbit’s valid reauth metadata finding: a failed non-blocking WHAM usage probe no longer erases an existing pool plan. Verification on this exact head:
The PR is ready again. Fresh fork-workflow approval and explicit maintainer authentication/security review are still required for |
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
|
@Ingwannu or @lidge-jun, could you approve the fork workflows for exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afce6d4072
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (provider !== "chatgpt") { | ||
| const config = loadConfig(); | ||
| const namespaceCollision = codexAccountNamespaceProviderCollisionError(config.codexAccountNamespaces, provider); | ||
| if (namespaceCollision) throw new Error(namespaceCollision); | ||
| } |
There was a problem hiding this comment.
This change modifies OAuth authentication flow control, credential persistence ordering, and live activation of authenticated providers, so it falls squarely within the repository's security boundary. The commit description explicitly leaves fresh maintainer security review unchecked; do not merge these changes until an authorized maintainer has performed and recorded that review as required by MAINTAINERS.md.
AGENTS.md reference: AGENTS.md:L83-L89
Useful? React with 👍 / 👎.
|
Fix the remaining security blocker in PR #512 at head
Treat hostname and port as guarded persisted state with a baseline, not a permanent cached override:
Add a regression with this sequence:
Keep the current Claude baseline rebase, OAuth completion ordering, late namespace handling, and legacy Then rerun full cross-platform CI and obtain fresh maintainer security approval on the final head. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
afce6d4 to
8e584f5
Compare
|
Addressed Wibias's binding-baseline blocker in 8e584f5. Live hostname/port remain tied to the open socket; each guarded live save rereads the raw persisted binding, preserves a newer valid disk edit, and advances the persisted baseline only after save succeeds. The regression now covers external runtime bind -> OAuth adopts A -> disk edited to loopback B -> unrelated live save, then verifies runtime still reflects the open socket and a fresh load retains B. The branch is also rebased onto dev 461de39. Full test suite, typecheck, GUI lint, focused GUI test, and the upstream OAuth API-key preservation regressions pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e584f52dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Resolve conflicts pls @chrisae9 then @lidge-jun or @Ingwannu please code owner sec research + merge. |
8e584f5 to
1c24609
Compare
|
Author update on 1c24609:
The exact-head fork workflows may require maintainer approval before they run. @lidge-jun or @Ingwannu, this head is ready for the requested code-owner authentication/security review when convenient. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR metadata ==='
gh pr view 512 --repo lidge-jun/opencodex --json number,headRefOid,baseRefOid,files --jq '{number, headRefOid, baseRefOid, files: [.files[].path]}'
echo '=== Changed files (stat) ==='
git diff --stat origin/dev...HEAD
echo '=== Changed production/test files ==='
git diff --name-only origin/dev...HEAD
echo '=== Structural outlines of primary changed modules ==='
for f in src/config.ts src/oauth/index.ts src/server/management/oauth-account-routes.ts; do
if [ -f "$f" ]; then
echo "--- $f ---"
ast-grep outline "$f" --items all --view expanded || true
fi
done
echo '=== Relevant definitions and call sites ==='
rg -n -C 4 --glob '*.ts' --glob '*.tsx' \
'function (reloadLiveConfigFromDisk|saveConfigPreservingClaudeCode|saveRuntimeConfig|runLogin|upsertOAuthProvider)|\b(reloadLiveConfigFromDisk|saveConfigPreservingClaudeCode|saveRuntimeConfig|runLogin|upsertOAuthProvider)\s*\(' \
src tests gui || true
echo '=== Exact diff for configuration/OAuth paths ==='
git diff --find-renames --unified=80 origin/dev...HEAD -- \
src/config.ts src/oauth/index.ts src/server/management/oauth-account-routes.ts \
tests/config.test.ts tests/oauth-public-surface.test.tsLength of output: 3351 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='65e3fee1f8916ad08f61a0297da91aab05714795'
echo '=== Repository refs / base availability ==='
git status --short
git show -s --format='%H %P %s' HEAD
git cat-file -t "$base"
echo '=== PR diff summary ==='
git diff --stat "$base"...HEAD
git diff --name-only "$base"...HEAD
echo '=== Outlines: primary production and regression files ==='
for f in \
src/config.ts \
src/oauth/index.ts \
src/server/management/oauth-account-routes.ts \
tests/config-user-edits.test.ts \
tests/oauth-public-surface.test.ts \
tests/kiro-review-regressions.test.ts
do
if [ -f "$f" ]; then
echo "--- $f ---"
ast-grep outline "$f" --items all --view expanded || true
fi
done
echo '=== Definitions/call sites with context ==='
rg -n -C 5 --glob '*.ts' --glob '*.tsx' \
'function (reloadLiveConfigFromDisk|saveConfigPreservingClaudeCode|saveRuntimeConfig|reconcile|runLogin|upsertOAuthProvider)|\b(reloadLiveConfigFromDisk|saveConfigPreservingClaudeCode|saveRuntimeConfig|runLogin|upsertOAuthProvider)\s*\(' \
src tests gui || true
echo '=== Exact relevant diff ==='
git diff --find-renames --unified=100 "$base"...HEAD -- \
src/config.ts src/oauth/index.ts src/server/management/oauth-account-routes.ts \
tests/config-user-edits.test.ts tests/config.test.ts tests/oauth-public-surface.test.ts \
tests/kiro-review-regressions.test.tsLength of output: 107094 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Configuration symbols and exact ranges ==='
rg -n -C 12 \
'persistedLiveServerBinding|function (readPersisted|reconcileLiveConfigFromDisk|reloadLiveConfigFromDisk|saveConfigPreservingClaudeCode|armClaudeCodeBaseline)|export function (reconcileLiveConfigFromDisk|reloadLiveConfigFromDisk|saveConfigPreservingClaudeCode|armClaudeCodeBaseline)' \
src/config.ts
echo '=== Read relevant config source slices ==='
for range in 1:260 720:1060 1060:1320; do
start=${range%:*}; end=${range#*:}
echo "--- src/config.ts:$start-$end ---"
sed -n "${start},${end}p" src/config.ts
done
echo '=== All callers and tests of reconciliation/binding-safe saving ==='
rg -n -C 6 --glob '*.ts' --glob '*.tsx' \
'\b(reconcileLiveConfigFromDisk|reloadLiveConfigFromDisk|saveConfigPreservingClaudeCode|armClaudeCodeBaseline)\s*\(' \
src tests gui
echo '=== OAuth-related diff only, compact ==='
base='65e3fee1f8916ad08f61a0297da91aab05714795'
git diff --unified=30 "$base"...HEAD -- src/config.ts src/oauth/index.ts src/server/management/oauth-account-routes.ts
echo '=== Locate tests exercising deletion and nested/array conflicts ==='
rg -n -C 5 --glob '*.test.ts' --glob '*.test.tsx' \
'reconcileLiveConfigFromDisk|three-way|pending live|nested|delet|array|hostname|persisted.*binding' \
tests gui/testsLength of output: 191330 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Exact symbol locations ==='
rg -n \
'persistedLiveServerBinding|function (reconcileLiveConfigFromDisk|reloadLiveConfigFromDisk|saveConfigPreservingClaudeCode|armClaudeCodeBaseline|merge)|export function (reconcileLiveConfigFromDisk|reloadLiveConfigFromDisk|saveConfigPreservingClaudeCode|armClaudeCodeBaseline)' \
src/config.ts
echo '=== Relevant config implementation (line-numbered) ==='
nl -ba src/config.ts | sed -n '600,920p'
nl -ba src/config.ts | sed -n '920,1160p'
echo '=== Relevant config diff only ==='
base='65e3fee1f8916ad08f61a0297da91aab05714795'
git diff --unified=50 "$base"...HEAD -- src/config.ts
echo '=== Direct save/reconcile callers (production only) ==='
rg -n -C 3 --glob '*.ts' \
'\b(reconcileLiveConfigFromDisk|reloadLiveConfigFromDisk|saveConfigPreservingClaudeCode)\s*\(' \
srcLength of output: 1884 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== src/config.ts Lines 1110-1350 ==='
awk 'NR >= 1110 && NR <= 1350 { printf "%6d %s\n", NR, $0 }' src/config.ts
echo '=== src/config.ts changed hunks only ==='
base='65e3fee1f8916ad08f61a0297da91aab05714795'
git diff --unified=35 "$base"...HEAD -- src/config.ts
echo '=== Direct production callers ==='
rg -n -C 3 --glob '*.ts' \
'\b(reconcileLiveConfigFromDisk|reloadLiveConfigFromDisk|saveConfigPreservingClaudeCode)\s*\(' \
srcLength of output: 58404 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Exact head commit ==='
git show -s --format='commit=%H%nparent=%P%nsubject=%s%n%n%b' HEAD
echo
echo '=== Files changed by HEAD only ==='
git diff-tree --no-commit-id --name-status -r HEAD
echo
echo '=== HEAD-only diff stat ==='
git show --stat --oneline HEAD
echo '=== HEAD-only source/test diff ==='
git show --find-renames --unified=80 -- \
src/config.ts src/oauth/index.ts src/server/management/oauth-account-routes.ts \
tests/config-user-edits.test.ts tests/oauth-public-surface.test.ts \
tests/kiro-review-regressions.test.ts
echo '=== OAuth config-save sequencing and direct save implementation ==='
rg -n -C 8 --glob '*.ts' \
'async function runLogin|export async function runLogin|saveLatestConfig\(|loadLatestConfig\(|saveConfigPreservingClaudeCode\(' \
src/oauth/index.ts src/config.ts src/server/management/oauth-account-routes.ts
echo '=== Existing focused regression names ==='
rg -n --glob '*.test.ts' --glob '*.test.tsx' \
'same-provider|pending live|late namespace|reconcile|binding|Kiro CLI recovery' \
tests/oauth-public-surface.test.ts tests/config-user-edits.test.ts tests/kiro-review-regressions.test.tsLength of output: 134055
I found no blocking issues in the updated OAuth/config persistence paths. In particular, the final implementation correctly addresses the prior failure modes:
This was a static review; I did not execute the test suite in the sandbox. |
1c24609 to
ee7b7db
Compare
|
Will get merged after interal code work lands on dev. |
|
Merged into This landed as a rebase rather than a merge commit, so GitHub cannot detect it automatically and the PR has to be closed by hand — the code is in Security review (required by MAINTAINERS.md) — recorded here as the explicit maintainer review this PR was waiting on.
Rebase conflict resolved during merge. Verified on the final rebased tip: typecheck, GUI lint, privacy scan, and the full suite at 5865 pass / 0 fail. Thanks for splitting this out of #426 and for the detailed persistence-ordering work — the follow-up layers (routing, catalog, lifecycle UI) remain tracked in #425. |
Part of #425. This is the namespace-foundation layer split out of #426 after the maintainer review.
What this PR does
codexAccountNamespacesmap from public model-selector namespaces to stored Codex account IDs;__main__for the internal Desktop account and uses@mainonly in the namespace map, leavingmainavailable as a real pool-account ID;The provider-creation checks are deliberately narrow admission guards. The CLI, OAuth, management routes, and Alibaba startup migration do not create or rename namespaces here; they only refuse to overwrite one that is already configured. Management creation endpoints consistently return HTTP
409for these namespace conflicts.Account creation treats the live runtime configuration as authoritative. Manual import and OAuth creation reject a namespace collision before authentication work begins, then recheck after warmup immediately before credential persistence. OAuth reauthentication must still point to a configured pool row at that same boundary, so an account deleted while login is in progress is not recreated.
Generic OAuth provider login performs a namespace preflight before browser or CLI authentication, validates the provider row immediately before credential persistence, then reloads the latest configuration after the credential write and upserts only against that current provider row. This preserves same-provider API-key additions, removals, and switches that occur while credential persistence is pending. Kiro's credential transaction and rollback semantics remain intact. If a user-authored namespace claims the provider during credential persistence, the namespace wins: the credential remains saved, the provider remains absent from both disk and live routing, unrelated concurrent edits are retained, and CLI/GUI polling surfaces the recovery error before treating the credential as a successful login.
The management route no longer activates a provider when the authorization URL first appears:
startLoginFlowcompletes background persistence first, then reconciles the settled disk state into the long-lived server object before status becomes done. Reconciliation is an in-place, recursive three-way merge from the normalized pre-login disk baseline, so unrelated disk edits are adopted while pending live mutations retain precedence and atomic Claude subtree conflicts remain safe. The hostname and port actually bound by the process remain runtime-only admission state, while concurrent disk hostname and port edits remain next-start intent for later saves. A successful login therefore activates the persisted provider row without restart and cannot erase concurrent live or disk configuration changes.This layer is inert unless
codexAccountNamespacesis explicitly configured. It does not add picker entries, route requests to an exact account, pin a conversation, alter Pool or Direct selection, or expose a UI setting. With nocodexAccountNamespacesfield, existing model-selection behavior is unchanged.Privacy and collision rules
Generated public selectors never come from aliases, email addresses, stored account IDs, or deterministic ID fallbacks. Existing random log labels can be reused; legacy rows without one receive a fresh random selector. Explicit maps are preserved rather than silently renamed. Legacy account rows whose IDs cannot pass the namespace-target validator are omitted by the default and append builders rather than producing an invalid map.
Account IDs and selector targets reject JavaScript object-map keys such as
__proto__,prototype, andconstructorcase-insensitively. Route matching and namespace-to-account ownership remain exact, while provider and reserved-namespace admission is case-insensitive so differently cased provider IDs cannot create ambiguous selector prefixes. The field-local schema validates raw own entries before Zod's record parser, and cross-field provider/combo collisions remain object-level validation. The strict account-ID validator protects account creation and OAuth login. Invalid legacy pool rows are excluded from account discovery, selection, quota priming, and credential resolution. Authenticated deletion keeps a narrow compatibility escape for an exact configured non-main row whose ID matched the legacy grammar, including__main__, so unsafe legacy credentials can be removed without touching the Desktopauth.jsonlogin or admitting new reserved IDs. A provider migration also stops without saving if its destination belongs to an account namespace.The implementation footprint is 752 additions and 67 deletions under
src/andgui/src/, plus 50 documentation lines across the five configuration references. Tests add 1,664 lines and remove 8 lines across config-reload, namespace-allocation, persistence-race, provider/account admission, CLI, Kiro, and GUI regressions.Follow-up layers
The remaining work stays separate as requested:
This foundation intentionally does not introduce a partial config-locking abstraction. There is no runtime namespace writer in this layer, and the repository does not yet have a transaction primitive shared by every config writer; locking only these admission paths would imply a cross-process guarantee it cannot provide. The broader config transaction/lifecycle work remains in #425, alongside the already documented general config-write residual in #488.
Verification
bun run prepushgit diff --checkChecklist
devbefore the final push.MAINTAINERS.md.Independent pre-review was used to catch ingress, migration, persistence, and live-runtime edge cases, but it does not replace the required maintainer approval.
Summary by CodeRabbit
codexAccountNamespacesconfiguration (supports@main), including namespace allocation and selector matching.