Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCodex managed-home synchronization now enforces 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
45bd811 to
7a0bfda
Compare
|
Final stale-overlay cleanup follow-up is now on the rebased current head The implementation uses a versioned body marker plus same-directory stage/quarantine and no-clobber hard-link publication. It removes only unmodified Orca-managed stale overlays; unmarked/user-modified files, non-regular type changes, and concurrent targets are preserved or quarantined with a warning. The earlier manifest/fd/inode/copy/symlink-reconstruction design was removed. Three independent hard-gate rounds closed the stale-restore, quarantined-symlink, and active-publish races; final verdict is Spec PASS / Quality APPROVED. Post-rebase verification: 11 changed test files / 253 tests, focused 3 files / 76 tests, node/CLI/web typecheck, lint/format/max-lines/diff, and exact two-commit range-diff all pass under Node 24.18.0. Electron/E2E and physical Windows filesystem exercise were not run. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/rate-limits/codex-backend-base-url.ts (2)
29-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate top-level TOML line-scanning loop.
The
isTomlStructuralLine→getTomlTableHeader/break → match →updateTomlLineScanStateloop inparseTopLevelTomlString(Lines 69-81) mirrors the same scan pattern incodex-config-auth-store.ts(Lines 23-39). Both files already share the low-level helpers fromconfig-toml-line-scan.ts; extracting a generic "find/replace top-level key line" iterator there would remove the duplicated control flow and centralize future fixes (e.g. the CRLF edge case flagged in the sibling file).// e.g. in config-toml-line-scan.ts export function findTopLevelTomlLine( lines: string[], predicate: (line: string) => boolean ): number | null { let scanState = createTomlLineScanState() for (let index = 0; index < lines.length; index += 1) { const line = lines[index] ?? '' if (isTomlStructuralLine(scanState)) { if (getTomlTableHeader(line)) return null if (predicate(line)) return index } scanState = updateTomlLineScanState(scanState, line) } return null }Also applies to: 63-83
37-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnreachable
|| url.originfallback.
${url.origin}${...}is always a non-empty string (url.origin is never falsy for a successfully parsed URL), so|| url.originnever triggers.🧹 Proposed cleanup
- return `${url.origin}${path === '/' ? '' : path}` || url.origin + return `${url.origin}${path === '/' ? '' : path}`
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c47d8768-98e0-4e56-bce6-7f535ef8553c
📥 Commits
Reviewing files that changed from the base of the PR and between 45bd8110af66a71ff1cf643dae07c8912bbd9dac and 7a0bfda53496fcdb18d3c316ceb0fd624c268fcb.
📒 Files selected for processing (24)
config/tsconfig.cli.jsonsrc/main/codex-accounts/runtime-home-service.test.tssrc/main/codex-accounts/service.test.tssrc/main/codex-accounts/service.tssrc/main/codex/codex-config-auth-store.tssrc/main/codex/codex-config-mirror.test.tssrc/main/codex/codex-config-mirror.tssrc/main/codex/codex-config-path-reference-rewrite.tssrc/main/codex/codex-profile-config-overlay-active-publish.tssrc/main/codex/codex-profile-config-overlay-mirror.test.tssrc/main/codex/codex-profile-config-overlay-mirror.tssrc/main/codex/config-settings-promotion.test.tssrc/main/rate-limits/codex-backend-base-url.test.tssrc/main/rate-limits/codex-backend-base-url.tssrc/main/rate-limits/codex-fetcher-backend.test.tssrc/main/rate-limits/codex-fetcher-buckets.test.tssrc/main/rate-limits/codex-fetcher.test.tssrc/main/rate-limits/codex-fetcher.tssrc/main/rate-limits/codex-rpc-rate-limit-mapping.tssrc/renderer/src/components/status-bar/StatusBar.tsxsrc/renderer/src/components/status-bar/status-bar-provider-menu-focus.test.tsxsrc/renderer/src/components/status-bar/tooltip.test.tssrc/renderer/src/components/status-bar/tooltip.tsxsrc/shared/rate-limit-types.ts
✅ Files skipped from review due to trivial changes (1)
- src/shared/rate-limit-types.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- src/main/codex-accounts/service.ts
- src/main/codex/config-settings-promotion.test.ts
- src/renderer/src/components/status-bar/StatusBar.tsx
- src/main/rate-limits/codex-fetcher.test.ts
- src/renderer/src/components/status-bar/tooltip.test.ts
- src/main/rate-limits/codex-fetcher-backend.test.ts
- src/main/codex/codex-config-path-reference-rewrite.ts
- src/renderer/src/components/status-bar/tooltip.tsx
- src/renderer/src/components/status-bar/status-bar-provider-menu-focus.test.tsx
- src/main/rate-limits/codex-rpc-rate-limit-mapping.ts
- src/main/rate-limits/codex-fetcher-buckets.test.ts
- src/main/codex-accounts/runtime-home-service.test.ts
- src/main/codex/codex-config-mirror.test.ts
- src/main/codex/codex-config-mirror.ts
- src/main/rate-limits/codex-backend-base-url.test.ts
- src/main/rate-limits/codex-fetcher.ts
- src/main/codex-accounts/service.test.ts
7a0bfda to
880ad9e
Compare
|
Current-head review-body nitpick dispositions:
The branch was rebased again onto current main |
|
@coderabbitai review |
✅ Action performedReview finished.
|
fb5dcc4 to
29d041e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/codex/codex-config-auth-store.ts (1)
42-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePreserve Windows line endings when prepending the configuration.
When prepending the setting to an existing configuration that uses Windows
\r\nline endings, the newly added line uses\n, resulting in mixed line endings. While TOML parsers typically accept this, detecting the line ending from the existing content ensures strict formatting consistency.🛠️ Proposed refactor
- return restoreBom( - content.length === 0 - ? `${FILE_AUTH_CREDENTIALS_STORE_LINE}\n` - : `${FILE_AUTH_CREDENTIALS_STORE_LINE}\n${content}` - ) + const fallbackLineEnding = content.includes('\r\n') ? '\r\n' : '\n' + return restoreBom( + content.length === 0 + ? `${FILE_AUTH_CREDENTIALS_STORE_LINE}${fallbackLineEnding}` + : `${FILE_AUTH_CREDENTIALS_STORE_LINE}${fallbackLineEnding}${content}` + )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: af035d94-08ab-44c6-aa68-34dc9337848e
📥 Commits
Reviewing files that changed from the base of the PR and between fb5dcc4 and 29d041eb25f48b4ea29de5524b41819a1e8f8f57.
📒 Files selected for processing (24)
config/tsconfig.cli.jsonsrc/main/codex-accounts/runtime-home-service.test.tssrc/main/codex-accounts/service.test.tssrc/main/codex-accounts/service.tssrc/main/codex/codex-config-auth-store.tssrc/main/codex/codex-config-mirror.test.tssrc/main/codex/codex-config-mirror.tssrc/main/codex/codex-config-path-reference-rewrite.tssrc/main/codex/codex-profile-config-overlay-active-publish.tssrc/main/codex/codex-profile-config-overlay-mirror.test.tssrc/main/codex/codex-profile-config-overlay-mirror.tssrc/main/codex/config-settings-promotion.test.tssrc/main/rate-limits/codex-backend-base-url.test.tssrc/main/rate-limits/codex-backend-base-url.tssrc/main/rate-limits/codex-fetcher-backend.test.tssrc/main/rate-limits/codex-fetcher-buckets.test.tssrc/main/rate-limits/codex-fetcher.test.tssrc/main/rate-limits/codex-fetcher.tssrc/main/rate-limits/codex-rpc-rate-limit-mapping.tssrc/renderer/src/components/status-bar/StatusBar.tsxsrc/renderer/src/components/status-bar/status-bar-provider-menu-focus.test.tsxsrc/renderer/src/components/status-bar/tooltip.test.tssrc/renderer/src/components/status-bar/tooltip.tsxsrc/shared/rate-limit-types.ts
🚧 Files skipped from review as they are similar to previous changes (21)
- src/main/codex/config-settings-promotion.test.ts
- src/main/rate-limits/codex-fetcher.test.ts
- src/shared/rate-limit-types.ts
- src/renderer/src/components/status-bar/StatusBar.tsx
- src/main/rate-limits/codex-backend-base-url.test.ts
- src/main/codex-accounts/service.ts
- src/main/codex/codex-profile-config-overlay-mirror.ts
- src/renderer/src/components/status-bar/status-bar-provider-menu-focus.test.tsx
- src/renderer/src/components/status-bar/tooltip.test.ts
- src/renderer/src/components/status-bar/tooltip.tsx
- src/main/codex/codex-config-mirror.ts
- src/main/codex/codex-config-path-reference-rewrite.ts
- src/main/rate-limits/codex-rpc-rate-limit-mapping.ts
- src/main/codex-accounts/service.test.ts
- src/main/rate-limits/codex-fetcher-buckets.test.ts
- src/main/codex-accounts/runtime-home-service.test.ts
- src/main/rate-limits/codex-fetcher-backend.test.ts
- src/main/codex/codex-profile-config-overlay-mirror.test.ts
- src/main/codex/codex-profile-config-overlay-active-publish.ts
- src/main/codex/codex-config-mirror.test.ts
- src/main/rate-limits/codex-fetcher.ts
29d041e to
7e78a50
Compare
|
Rebased onto latest Conflict resolution notes:
|
45ff821 to
9a2ffd5
Compare
|
Rebased onto current Upstream #10136 ( What this PR still contributes on top of upstream: the multi-meter One additional commit was needed to adapt to upstream: null-tolerant mapper types plus the buckets test now expecting normalized windows. Happy to restore the original 299/10079 behavior if that was intentional on upstream's side rather than incidental. |
9a2ffd5 to
d5d5a84
Compare
a0f3e22 to
2df682b
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes three interconnected areas: managed Codex configs are now forced to
Confidence Score: 5/5Safe to merge; the auth-forcing, overlay mirroring, and rate-limit changes are well-bounded and backed by comprehensive RED-to-GREEN tests. All three areas of change (auth config hardening, overlay lifecycle, rate-limit alignment) have matching test coverage for the edge cases the PR describes: BOM handling, no-clobber races, hard-link preflight failure, stale-overlay ownership verification, backend URL parsing, and Codex bucket rendering. The logic is internally consistent and the failure modes fail closed with warnings rather than data loss. Files Needing Attention: tooltip.tsx — the
|
| Filename | Overview |
|---|---|
| src/main/codex/codex-profile-config-overlay-active-publish.ts | New module implementing stage/quarantine/hard-link atomic publish for profile overlay files, with deterministic probe path (capped at one per target), pre-flight hard-link capability check, and no-clobber restore on failure. |
| src/main/codex/codex-profile-config-overlay-mirror.ts | New module syncing profile overlay files into the managed runtime home, writing a versioned SHA-256 ownership marker (BOM-aware), and safely removing only marker-matching stale copies. |
| src/main/codex/codex-config-auth-store.ts | New module that forces cli_auth_credentials_store = "file" at the top level of managed TOML configs, correctly handling BOM, existing-correct values (no-op), existing-wrong values (inline replace preserving indent/CRLF), and missing values (prepend). |
| src/main/rate-limits/codex-backend-base-url.ts | New module parsing chatgpt_base_url from CODEX_HOME's top-level TOML (with BOM handling and scan-state-aware line scanning), normalizing it, and building WHAM vs custom-backend URL suffixes correctly. |
| src/main/rate-limits/codex-rpc-rate-limit-mapping.ts | New module extracting extra RPC rate-limit buckets from rateLimitsByLimitId, excluding the preferred meter, using reported windowDurationMins for each bucket's window length. |
| src/renderer/src/components/status-bar/tooltip.tsx | Adds getWindowSections that prepends the preferred Codex session meter and appends the weekly meter around extra buckets; Codex with buckets now shows all meters in the tooltip instead of only buckets. |
| src/renderer/src/components/status-bar/StatusBar.tsx | Single-line fix: Codex provider no longer enters the bucket-only compact/verbose path in VerboseProviderUsage, preserving session/weekly visibility in the status bar when dynamic buckets are present. |
| src/main/rate-limits/codex-fetcher.ts | Integrates mapCodexRpcRateLimitBuckets into the RPC response path and wires resolveCodexBackendBaseUrl into backend reset-credit fetch and consume paths; pre-existing WSL usage-endpoint hardcode unchanged. |
| src/main/codex/codex-config-mirror.ts | Now calls forceFileAuthCredentialsStore as part of prepareSystemConfigForRuntimeMirror and invokes syncCodexProfileConfigOverlaysIntoManagedHome before the main config mirror, even when the primary config is blank. |
| src/main/codex-accounts/service.ts | Applies forceFileAuthCredentialsStore inside writeManagedConfig, ensuring per-account managed configs also enforce file-backed auth before the equality check and atomic write. |
| src/shared/rate-limit-types.ts | Adds buckets?: RateLimitBucket[] to ProviderRateLimits and the RateLimitBucket type; minimal, additive change. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[syncSystemConfigIntoManagedCodexHome] --> B[promoteCodexRuntimeSettingsToSystem]
B --> C{Promotion OK?}
C -- No --> D[Report stall, return]
C -- Yes --> E[syncCodexProfileConfigOverlaysIntoManagedHome]
E --> F{system overlays?}
F -- Yes --> G[mirrorCodexProfileConfigOverlay]
G --> G1[readFileSync source overlay]
G1 --> G2[rewriteRelativePathConfigValues]
G2 --> G3[forceFileAuthCredentialsStore]
G3 --> G4[addProfileOverlayOwnershipMarker SHA-256]
G4 --> G5{target exists?}
G5 -- No --> G6[publishActiveManagedOverlay replaceExisting=false]
G5 -- Yes & match --> G7[skip no-op]
G5 -- Yes & differ --> G8[canPublishProfileOverlayByHardLink probe preflight]
G8 -- OK --> G9[quarantineProfileOverlayTarget]
G9 --> G10[linkSync stage to target]
G10 -- EEXIST --> G11[warn concurrent target]
G10 -- other error --> G12[restoreRegularProfileOverlayQuarantine]
G10 -- OK --> G13[removeProfileOverlayQuarantine]
F -- Done --> H[removeStaleManagedOverlays]
H --> I{overlay in runtime not in system?}
I -- Yes --> J[quarantine read verify SHA-256 marker]
J -- not managed --> K[restoreRegularProfileOverlayQuarantine]
J -- managed --> L[removeProfileOverlayQuarantine]
E --> M[Mirror main config.toml]
M --> N[forceFileAuthCredentialsStore + path rewrite]
N --> O[writeFileAtomically]
Reviews (2): Last reviewed commit: "fix(codex): bound retained overlay probe..." | Re-trigger Greptile
…malize Accept null preferred rateLimits from app-server, keep classification-normalized session/weekly windows, and leave extra meters on buckets.
Managed homes inject cli_auth_credentials_store=file so account switching keeps credentials inside CODEX_HOME; update assertions.
Main's rate-limit probe now waits for child exit; the buckets fixture must emit exit on stdin end like the other fetcher harnesses.
5158405 to
b50557f
Compare
|
Closing as part of fork PR backlog cleanup. This branch is long-conflicted (DIRTY) against current |
3 participants
Summary
chatgpt_base_urlfrom top-level TOML, including BOM-prefixed configs, for custom-backend rate-limit requestswindowDurationMinsand retain additional by-ID bucketsThis is the focused auth/config/rate-limit replacement extracted from #7950. It intentionally excludes hooks/status, startup delivery, and session/native-chat changes.
Refs #7950
Screenshots
No visual redesign; Codex status-bar usage can now retain session/weekly while exposing additional meters in the tooltip.
Testing
pnpm typecheck(node, CLI, web)pnpm lint(passes with repository-existing warnings)oxlintoxfmt --checkpnpm check:max-lines-ratchetgit diff --check origin/main...HEADgit range-diffafter rebase ontoe5efbc9ffAI Review Report
The first independent gate found four blockers: profile overlays could override file auth, rate-limit durations were rounded as though they were remaining time, Codex dynamic buckets hid the weekly meter, and a BOM could hide the first-line backend URL. Each was reproduced and fixed before submission.
A separate three-round hard gate reviewed stale-overlay ownership and concurrent filesystem behavior. It found and drove tests for no-clobber stale restore, post-quarantine symlink type changes, and active-publish replacement races. The final implementation removed the earlier manifest/fd/inode/copy/symlink-reconstruction design and split active publishing into one concrete module.
A current-head CodeRabbit review then identified that a final hard-link failure could strand the previous overlay in quarantine. The finding was independently confirmed and severity-calibrated, then fixed with a pre-quarantine hard-link capability probe plus best-effort no-clobber restore for non-
EEXISTfinal failures. Two new RED-to-GREEN tests cover permanent preflight failure and a one-shot final publish failure. Final independent verdict: Spec PASS / Quality APPROVED, with no confirmed blocker.Cross-platform review covered macOS, Linux, and Windows path/type behavior: paths use Node utilities, rename/link operations stay within one directory, Windows overlay names compare case-insensitively, the existing writer retains Windows ACL repair, and no UI shortcut/label/Electron-specific behavior changes.
Security Audit
CODEX_HOME; no credential value crosses a new channelEEXISTpreserves the concurrent targetCompatibility
TOML handling preserves BOM placement and rewrites platform paths through existing path utilities. Status rendering changes only the Codex bucket branch; Gemini's bucket-only compact behavior remains unchanged.
The no-clobber protocol preflights same-directory hard links before moving an existing target. A filesystem or share without hard-link support therefore fails closed with a warning and leaves the existing overlay at its original path. If a transient final publish and subsequent no-clobber restore both fail, the previous regular file remains under an explicit quarantine name for manual recovery.
Notes
Validation used the repository-declared Node 24.18.0. The three branch commits were range-diff equivalent when rebased onto
origin/main@e5efbc9ff; the branch has since been rebased again.ELI5
Managed Codex auth config and overlays could drift (keyring vs file auth, stale overlays, path issues), which broke rate limits and account switching. Auth is forced file-backed in managed homes, overlays are versioned and cleaned carefully, and rate-limit handling stays aligned.