Repository navigation
fix(codex): carry uploaded-file retention through compact and keep the denial lookup behind the read fence - #4806
Conversation
#4778) A conversation carrying live uploaded-file references must keep the account that issued them: the files are readable only by that account, the reference stays in conversation history, and every later turn is refused with 409 account_change_file_scope until the user re-uploads or starts over. #4797 passed the retention bit at regular Responses final auth, but two paths that bind the same conversation did not: - Native compact resolved its initial account without it. The refusal it does carry runs only after a 429, so a voluntary quota rebind had already moved the conversation by the time that guard declined to move it. - Encrypted-agent-task recovery rebuilds the preview selection options from scratch, against the decrypted body -- the first point at which a ciphertext-only file reference becomes readable. Preview could report one account while final auth bound another. Both now answer from the same predicate and the same body the existing guards use, so no two sites can disagree about which conversations are in scope. The post-429 compact guard is unchanged: refusing an alternate after a rejection is a different question from declining a voluntary move. Retention is inert unless a move was about to happen, so a request with no uploaded file resolves exactly as before.
…ad fence (#4768) cachedDeniedCodexAccountIdsForModel validates every cached roster against the account's current credential, and for native main that validation resolves the physical stored token -- a synchronous read of account storage, once per cached client version, on the request path. Auth context establishes nativeMainReadsForbidden and excludes main from the entitlement snapshot immediately above, then called the denial reader with no exclusions at all, so a request served by its own forwarded credential, or one arriving while a profile switch drains main, reread the credential it is forbidden to touch. The two subagent-fallback previews in request-prepare did the same, which also let preview score main differently than the resolution it exists to predict. The reader now takes an exclusion set and honours it before the identity read, so an excluded account is skipped rather than validated. Dropping it leaves the account unknown rather than denied, and the denied set is an ordering preference that selection discards whenever honouring it would leave no candidate -- so an account can only become more selectable, never refused. Excluding main is therefore a no-op for every request that was allowed to read it.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe pull request updates account selection fences, preserves account ownership for uploaded files, and adds structural coverage for fallback preview options. It also moves fallback preview tests into the routing test layout. ChangesResponses account selection
Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
✅ Deterministic PR hygiene checks passed. |
) The #2509 source-oracle matched each preview call's selection options with `[^}]*` on either side of `modelEligibleAccountIds`, which silently assumed the options object contained no nested literal. Giving `deniedModelAccountIds` an options argument of its own added one, so the match count went from two to zero and the test reported both call sites as having dropped the eligible set. That is the failure the surrounding comment promises not to have: it names #4546 and #4768 as growth it tolerates, then fails on the next instance of exactly that growth -- and fails in the direction that looks like the real defect rather than like a broken matcher. The options literal is now read by brace balance, and the call-site check is kept by asserting the literal is the argument immediately before `modelId`. The assertion is unchanged in substance: it still requires both sites to forward the set, still passes on the pre-change tree, and still fails when either site drops it.
…n file (#2509) The brace-balance rewrite pushed subagent-fallback-handle-responses.test.ts to 2313 lines, past its 2289 ratchet cap. The cap cannot be raised and the file cannot be exempted, so the cases move instead. They are the right ones to move. Both read request-prepare.ts as text and assert on it: they open no server, install no credential, and touch no account state, so they were the part of that file paying for an end-to-end pool harness they never used. Splitting the single #2509 case into two also lets each assertion name what it checks -- that both sites accept the eligible set, and that both forward it into the preview call -- instead of reporting one failure for two different contracts. The moved file is registered in scripts/test-layout/layout.json and tests/fixtures/test-layout-expected.json with the same key and target, so the two copies of that table stay equal. Its name also resolves to routing/ through the existing subagent- seed, so the seed and the explicit table agree. The source file drops to 2249 lines and its now-unused node:url import; tests/fixtures/file-size-baseline.json is untouched.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bada1686ce
ℹ️ 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".
| deniedModelAccountIds: cachedDeniedCodexAccountIdsForModel(modelId, previewNow, { | ||
| excludeAccountIds: nativeMainReadsForbidden ? new Set([MAIN_CODEX_ACCOUNT_ID]) : undefined, | ||
| }), |
There was a problem hiding this comment.
For a thread_spawn request carrying its own forwardable ChatGPT bearer, codexRouteCredentialOwnership(...).requestScopedMainCredential is true while nativeMainReadsForbidden remains false unless startup recovery or profile draining is active. Consequently this lookup still calls currentCredentialIdentity(__main__), reading the physical stored token and potentially scoring main differently from final auth, whose fence explicitly includes requestScopedMainCredential. Fold that ownership result into this fence and recompute it for the recovery preview as well.
AGENTS.md reference: src/AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
| const requestPrepareSource = async (): Promise<string> => Bun.file( | ||
| fileURLToPath(new URL("../../src/server/responses/request-prepare.ts", import.meta.url)), | ||
| ).text(); |
There was a problem hiding this comment.
This new source-oracle test resolves the runtime file relative to import.meta.url rather than through tests/helpers/repo-root.ts. Repository tests may execute from relocated or differently resolved trees, so the mandated helper is the stable repository authority; import repoPath and open repoPath("src", "server", "responses", "request-prepare.ts") instead.
AGENTS.md reference: AGENTS.md:L21-L25
Useful? React with 👍 / 👎.
…e denial lookup behind the read fence (lidge-jun#4806) Release-blocker fix for 2.57.0, found by the final cross-change regression audit. Exact head has a green aggregate ci check with no failing job. Uploaded-file retention did not reach native compact initial account resolution or the encrypted-recovery preview, so a file-carrying conversation could still be moved before the post-429 guard ran, or preview and final authentication could pick different accounts — either one reproduces the failure lidge-jun#4778 removed. Both paths now carry the bit, answered from the same object and predicate the post-429 guard uses. Separately the flagship denial lookup crossed the native-main read fence by validating cached rosters through the physical main token; it now honours the fence and answers unknown there, which changes nothing because unknown never excludes an account. Host-owned merge decision; no local suite, typecheck, build, or install was run.
1 participant
Summary
Two blockers the final 2.57.0 regression audit found in the account-selection work that landed in #4797.
Uploaded-file retention did not reach two paths. Native compact resolved its initial account without the retention bit, so quota-based rebinding could move a file-carrying conversation before the post-429 guard ever ran, and the encrypted-recovery preview rebuilt selection options without it, so preview and final authentication could pick different accounts. Either one reproduces the exact failure #4778 removed: the file references are orphaned and every later turn is refused until the user re-uploads or restarts the conversation. Both paths now carry the bit, answered from the same object with the same predicate the post-429 guard uses so the two cannot disagree about scope.
The flagship denial lookup crossed the native-main read fence. Auth establishes that native main reads are forbidden and then called the denial lookup without exclusions; that lookup validates each cached roster through the physical main token, which is the read the fence exists to prevent and which can resynchronously reread account storage once per cached version. The lookup now honours the fence, and the outcome under it is unknown rather than denied, which changes nothing because unknown never excludes an account.
Verification
Checklist
Summary by CodeRabbit
Bug Fixes
Tests