fix(windows): async ACL harden for response-state writes (#612) - #645
Merged
Conversation
Stop blocking the event loop on sync icacls during debounced response-state persistence. Add an async ACL runner with explicit timeout provenance, async atomic writes keyed by destination path (not parent dir), single-flight serialization, and awaited flush on shutdown.
📝 WalkthroughWalkthroughThis PR adds Codex account namespace and pause controls, quota-scoped routing, xAI video bridging, asynchronous response-state persistence and Windows ACL hardening, OAuth/config reconciliation, workflow validation, GUI updates, and corresponding tests and documentation. ChangesRuntime and account controls
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: fdf0236214
ℹ️ 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".
| * atomicWriteFile, but Windows ACL harden yields the event loop. Timeout memo is keyed | ||
| * by the final destination path (not the unique temp, not the parent directory). | ||
| */ | ||
| export async function atomicWriteFileAsync( |
There was a problem hiding this comment.
The reviewed commit is an ordinary TypeScript runtime fix whose direct parent is ca7b104 (main), rather than a maintainer-controlled promotion from dev; retarget or rebase the change onto dev before merging, since neither the async writer nor any other changed file belongs to the scoped dev2-go line.
AGENTS.md reference: AGENTS.md:L77-L82
Useful? React with 👍 / 👎.
| harden: async target => { | ||
| try { chmodSync(target, 0o600); } catch { /* platform may ignore chmod */ } | ||
| if (process.platform === "win32") { | ||
| await hardenSecretPathAsync(target, { required: true, timeoutMemoKey: path }); |
There was a problem hiding this comment.
This changes the Windows ACL enforcement path that protects persisted conversation/request content, including a destination-keyed timeout memo that can cause later temporary files to skip icacls, while the commit's test plan explicitly leaves security review unchecked; obtain the required explicit security review before merging this confidentiality-boundary change.
AGENTS.md reference: AGENTS.md:L83-L89
Useful? React with 👍 / 👎.
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Collaborator
Author
Follow-up on Codex notesRetarget to Security review: completed locally against this diff. Verdict: no new medium+ regressions vs the existing sync/#160 timeout soft-fail policy. Confirmed: owner-first grant before Still not merging — awaiting maintainer security sign-off per the #612 routing label. |
2 tasks
The project is moving its primary runtime to the Go native port, so dev2-go has to keep receiving everything that lands on dev. Nothing changes for contributors: pull requests still target dev. The extra step belongs to the maintainer who merges one — rebase the work onto dev2-go, port whatever needs a Go counterpart under go/, and merge that. The item is finished only when both lines carry the change. An unported dev merge is the failure mode this rule exists to prevent: dev2-go silently falls behind and the divergence surfaces later as a conflict nobody has context for. So the port happens in the same session as the merge; a change with no Go counterpart gets that decision recorded; a port that has to wait gets a tracking issue against dev2-go naming the source commits. MAINTAINERS.md carries the authoritative wording and the other seven files summarise it. The maintainer table widens both maintainers' integration duty to dev and dev2-go, since that is what the rule actually asks of them. Also correct the 2026-07-27 change-log entry. It claimed requirement 2 (review by another current maintainer) would be satisfied "when this change is reviewed and merged". It never was: a2693c0, dc3a4ad and 02bbd47 landed on dev as direct owner pushes with no associated pull request. The addition is in effect regardless — @Wibias holds write access and has been merging since 2026-07-26 — so the entry now records the gap instead of asserting a procedure that did not happen.
Adds nested AGENTS.md files and CODEOWNERS coverage for agent guidance. Keeps the owner Transition intake rule already on dev (merge to dev, carry onto dev2-go, port go/ only when needed).
) * feat(videos): add Grok video bridge for non-OpenAI models Extends the image bridge to support asynchronous video generation via xAI Grok Imagine Video. Video generation uses a submit→poll→download pattern (POST /v1/videos/generations → GET /v1/videos/{id}) with heartbeat forwarding to keep the SSE stream alive during the 30-180s generation window. New files: - src/images/xai-video-client.ts: submitVideoJob + pollVideoJob - src/images/fulfill-video.ts: arg parsing, async polling generator, result builder - tests/videos/: 32 tests (xai-video-client, fulfill-video, plan-video) - docs-site/.../video-bridge.md: user guide Modified files: - src/images/loop.ts: unified image+video fulfillment, per-turn call caps - src/images/types.ts: VideoBridgePlan/VideoCallResult shared types - src/images/synthetic-tool.ts: buildVideoTool, VIDEO_GEN_TOOL_NAME - src/images/plan.ts: planVideoBridge (opt-in via videoBridgeEnabled) - src/images/artifacts.ts: downloadVideoToArtifact (200MB cap, SSRF-protected) - src/server/responses/core.ts: video bridge wiring + tool dedup - src/types.ts: videoGeneration flag, video config fields Video bridge is opt-in (videoBridgeEnabled defaults to false). All upstream image-bridge hardening preserved (SSRF, size caps, abort linking, pinned HTTPS). * fix(videos): address Codex P1/P2 + CodeRabbit review findings P1 fixes: - Non-streaming requests no longer 400 when video bridge is enabled (only image bridge requires stream=true; video-only skips bridge for non-stream requests) - Restore globalThis.fetch after video client tests (afterEach cleanup) P2 fixes: - Use clamped vidPlan.timeoutMs instead of raw config value - Honor videoMaxRounds when both image+video bridges active (tighter wins) - guessVideoExtFromMagic throws on unrecognized magic (was defaulting to mp4) - Poll 4xx permanent failures (except 429) fail fast instead of retrying - buildVideoResult uses pathToFileURL for cross-platform markdown links - Initial poll interval restored to 5s (was 200ms) - done-without-videoUrl returns error instead of spinning until timeout - VideoBudget charges every streamed chunk (was only first chunk) - downloadVideoToArtifact: reader cleanup on file-open failure - readBoundedText: cancel reader body on size cap breach - Namespaced video_gen tools preserved in dedup filter - Fix JSON missing closing brace in docs - Clarify API key auth requirement in docs prerequisites * fix(videos): encode requestId, defer paidVideoCalls, guard plan! assertion - Encode requestId in poll URL to prevent path injection (CodeRabbit) - Move paidVideoCalls++ past arg validation so malformed calls don't burn budget - Add early guard for undefined plan in image branch, removing non-null assertions * fix(videos): batch-aware artifact pruning + clarify provider key docs - Move pruneArtifacts from per-download to post-batch so multi-video turns don't delete earlier videos before tool results are injected (Codex P2) - Document that video bridge requires providers.xai with apiKey authMode, not OAuth from 'ocx login xai' (Codex P2) * fix(videos): web search coexistence, unified timeout budget, VideoBudget ceiling Address Wibias CHANGES_REQUESTED review: 1. Web search coexistence (P1): - When wsPlan is active, media bridge is skipped (existing behavior) - Now emits console.warn so the user sees the skip instead of silent loss - Documented priority rule in video-bridge.md 2. Timeout budget (P2): - Start deadline BEFORE submitVideoJob, not after - Poll receives remaining budget (deadline - now), min 5s floor - Submit (60s) + poll now share one videoTimeoutMs deadline 3. VideoBudget aggregate ceiling (optional, addressed): - VideoBudget now has a cap (600 MiB = 3 × single-download max) - chargeVideoBudget() enforces the ceiling per-chunk during streaming - Exceeding the budget throws mid-download (partial file is cleaned up) 4. New tests: done-without-videoUrl, permanent 4xx poll stop, requestId encoding * fix(videos): gate tool_choice on imgPlan, shared deadline signal, buffer magic sniff Address Wibias second CHANGES_REQUESTED review: Required: B. Gate image tool_choice rewriting on imgPlan — video-only turns no longer rewrite image_generation/image_gen aliases to an undeclared tool (core.ts) C. Shared timeout complete — deadline-bound AbortSignal passed into submitVideoJob; if budget expired after submit, poll is skipped with timeout error (no 5s floor) Preferred: A. Buffer ≥12 bytes before magic-byte sniff — accumulate chunks until sniff minimum so short first reads don't crash guessVideoExtFromMagic (artifacts.ts) Optional: D. Docs: scope web-search priority to runnable sidecar / non-runTurn path * fix(videos): pass deadline-bound signal into poll generator Pass linkedDeadline (not raw client signal) into pollVideoWithHeartbeats so in-flight pollVideoJob fetches and sleep() calls abort when the wall-clock budget expires. Abort error messages now distinguish deadline-expiry from client-cancel based on elapsed time vs timeoutMs threshold. * fix(videos): deadline abort classification, pruned-path guard, video aliases, DNS noun Address Wibias fourth CHANGES_REQUESTED review: 1. Deadline abort classification (required #1): - Replace 0.95 wall-clock heuristic with signal.reason.name === 'TimeoutError' - deadlineSignal declared outside try so catch can check deadlineSignal.aborted - Submit catch returns timeout error when deadlineSignal fires (not generic abort) 2. Don't deliver pruned artifact paths (required #2): - After batch prune, scan fulfilled results and rewrite any whose path no longer exists to ok:false with explanatory error 3. Wire video aliases (required #3): - planVideoBridge seeds toolNames with VIDEO_GEN_TOOL_NAME + any request function tools matching isVideoGenName (image-bridge parity) - core.ts filter only strips unnamespaced video_gen aliases (namespaced MCP tools are left alone) 4. Fix test fixtures (required lidge-jun#4): - authMode 'api_key' → 'key' (valid OcxProviderConfig value) 5. DNS error noun (small): - resolvePublicAddresses accepts optional noun param (default 'image') - downloadVideoToArtifact passes 'video' * fix(videos): complete pruned-path guard, skip namespaced aliases, mock sleep in tests Address Wibias fifth CHANGES_REQUESTED review: 1. Finish pruned-path guard: - Filter files[] through existsSync, not just path - If all pruned: ok:false with no markdown/path - If some survive: refresh path/files/count/markdown from survivors 2. Skip namespaced tools when seeding videoPlan.toolNames: - if (t.namespace) continue — matches core.ts filter rule 3. Fix ubuntu CI timeout: - Mock setTimeout in heartbeat test so 5s poll interval resolves instantly - Suite now runs in ~156ms (was ~5s) * fix(videos): stop hanging ubuntu CI with global setTimeout mock cf76fb1 mocked globalThis.setTimeout to skip the 5s poll sleep. That races parallel Bun workers and cancelled ubuntu-latest at the 12m job cap (938f25e was green; cf76fb1 hung). Inject sleep/poll seams instead and drop mock.module so video tests no longer poison each other. --------- Co-authored-by: Your Name <your.email@example.com> Co-authored-by: Wibias <37517432+Wibias@users.noreply.github.com>
* Merge remote-tracking branch 'origin/dev' into fix/codex-spark-quota-scope-dev * fix: retain affinity per Codex quota scope * fix: address scoped affinity review feedback * fix: restore dev version and harden compact test
* feat(gui): show reset credit expiration time * fix(gui): respect selected locale for credit dates
…#638) * fix(adapters): neutralize Codex CLI 0.145 identity wording Closes lidge-jun#622 * fix(adapters): consume full GPT-5.x.y identity versions
Maintainer takeover of lidge-jun#565. Persisted pause exclusion, bulk pause-exhausted, GUI controls, docs. Integrated with account namespaces and Spark quota scopes on current dev.
Name the tracking-issue label in MAINTAINERS; mirror it in the AGENTS Transition summary.
…lidge-jun#627) * fix(cursor): steer Windows bridge shell away from PS 5.1 syntax loops Native-shell rejection told the model to replay the same bash/CMD command through the Codex bridge, which on Windows PowerShell 5.1 fails and loops (lidge-jun#604). Adapt for the host shell and inject PS-safe guidance instead. * fix(cursor): address lidge-jun#604 review — host-shell-neutral retry-stop guidance Drop proxy-OS platform branching and `;`-as-`&&` advice; use if ($?) and one corrected bridge attempt for Codex/CodeRabbit feedback on lidge-jun#627. * fix(cursor): state PowerShell 5.1 has no &&/|| in lidge-jun#604 guidance CodeRabbit follow-up: make unsupported-parser-error wording explicit and assert it in rejection/guidance tests.
…e-jun#651) Fixes lidge-jun#647. Single shared 401 token refresh gate; no prompt storm on concurrent /api/* 401s.
…gates (lidge-jun#631) Hardens wrong-base PR drafting so the required check fails even when draft GraphQL soft-fails, and tightens bug-form Version/OS/reproduction gates. Go port: none — CI/workflows and issue-quality scripts only; no Go counterpart.
* fix(catalog): accept Together top-level /models arrays Closes lidge-jun#617 * fix(catalog): keep models[] for probe only; array+data for discovery
Fixes lidge-jun#618. Prefer findLiveProxy over ocx.pid alone; keep status/doctor read-only on malformed config; preserve authoritative null live pids and runtime vs config listen source.
…jun#643) Fixes lidge-jun#636. Skip bare gpt-*/native OpenAI catalog rows when no enabled canonical openai provider exists; make NoEnabledOpenAiProviderError actionable.
Keep dig namespace imports and PR lidge-jun#645 async hardenSecretPathAsync on the same config import surface.
Collaborator
Author
|
Resolved conflict with current \dev\ (account namespaces): kept both namespace imports and \hardenSecretPathAsync. Focused ACL/response-state tests + typecheck green. |
This was referenced Jul 29, 2026
Wibias
added a commit
that referenced
this pull request
Jul 29, 2026
Collaborator
Author
|
Transition complete:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 39
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/responses/state.ts (1)
459-468: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
clearResponseStateMemoryForTestsclears the timer andpendingPersistPathbut leavespersistGateholding an in-flight write.The new gate is module state just like
persistTimer, and it is the one piece this reset does not account for. If a debounced write was already dispatched (void persistNow(path)at Line 298) when a test calls this helper, that write is still queued onpersistGate; it will later resolvesnapshotPath()-independent — it already capturedpath— and write into the previousOPENCODEX_HOME, while the next test'sflushResponseState()awaits a chain that includes the stale writer. That is exactly the shutdown/test race the gate was added to remove.Note you cannot simply assign
persistGate = Promise.resolve()here, because that abandons serialization for the still-running write. Draining is the correct reset:💚 Drain before clearing, or document the requirement
-export function clearResponseStateMemoryForTests(): void { - if (persistTimer) { - clearTimeout(persistTimer); - persistTimer = null; - } +/** Drains any queued snapshot write so a later test cannot observe a foreign-home write (`#612`). */ +export async function clearResponseStateMemoryForTests(): Promise<void> { + if (persistTimer) { + clearTimeout(persistTimer); + persistTimer = null; + } + await persistGate; pendingPersistPath = null; states.clear(); storedResponseBytes = 0; loaded = false; }If making the helper async is too disruptive for its existing call sites (e.g.
tests/responses-state.test.ts:57), keep it sync and add a comment stating that callers mustawait flushResponseState()first.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/responses/state.ts` around lines 459 - 468, Update clearResponseStateMemoryForTests to account for the module-level persistGate alongside persistTimer and pendingPersistPath. Preserve serialization by draining or awaiting the existing gate before resetting test state; do not replace it with Promise.resolve while a write may still be running. If the helper must remain synchronous, document that callers must await flushResponseState() before invoking it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/issue-quality.cjs:
- Around line 716-727: Add and export a neutral helper such as isUnknownStandIn
for shared unknown-value matching, use it in the OS validation branch alongside
the existing version validation, and retain isUnusableVersion as a thin
compatibility wrapper so current imports and behavior remain unchanged.
Consolidate the two OS branches’ identical reason and guidance handling without
changing validation outcomes.
- Around line 694-697: Remove the redundant !softPass condition from the terse
reproduction check in the bug-report branch, changing the guard around
isTooTerseBugReproduction(repro) to rely only on that predicate. Preserve the
existing reasons and guidance behavior, and leave the later !softPass guards
unchanged.
In `@gui/src/hooks/useCodexAccountPool.ts`:
- Around line 242-307: Extract the duplicated defensive response parsing and
active-account reconciliation from setAccountPaused and pauseExhaustedAccounts
into shared helpers, such as readJsonObject and applyServerActiveId. Preserve
the existing behavior of treating invalid JSON as an empty object and updating
pendingActiveIdRef/setActiveId only when activeCodexAccountId is present, then
have both callbacks reuse those helpers.
In `@gui/tests/active-account-reauth-aggregate.test.ts`:
- Around line 24-25: Replace the source-text-only assertions in the active
account reauthentication test with focused behavioral coverage that exercises
the controller or extracted predicate using an active account with paused: true
and needsReauth: true. Assert that activeNeedsReauth is false, while preserving
coverage of the relevant active-account selection behavior.
In `@gui/tests/intl-formatters.test.ts`:
- Around line 21-23: Add a test alongside “handles invalid dates consistently”
that passes an invalid date string to formatCreditDate and asserts the "—"
fallback. Keep the existing formatCreditDateTime assertion unchanged and target
the shared invalid-date behavior in the intl formatter tests.
In `@src/adapters/cursor/native-exec-shell.ts`:
- Around line 26-39: The PowerShell/POSIX shell-policy guidance is duplicated
and can drift. In src/adapters/cursor/native-exec-shell.ts lines 26-39, extract
the shared guidance from nativeShellDisabledMessage() into one exported constant
or helper; in src/adapters/cursor/tool-definitions.ts lines 420-424, import and
reuse that shared symbol in hostShellNote instead of maintaining a separate
string literal.
In `@src/codex/auth-api.ts`:
- Around line 449-469: Update the !resp.ok early-return branch in the WHAM usage
fetch flow to await cancellation of resp.body, swallowing any cancellation
rejection before returning the existing quota and reauthentication result. Keep
the current status handling unchanged and align with the response cleanup
pattern used by other early-return paths in auth-api.ts.
In `@src/codex/routing.ts`:
- Around line 489-514: Normalize an absent model scope to "shared" consistently
rather than treating undefined as a separate partition: update
getCodexQuotaHealthSnapshot and threadAffinityScope in src/codex/routing.ts so
scope-less resolutions consult shared cooldowns and affinity bindings while
preserving "legacy" only where explicitly required; update
src/codex/auth-context.ts (ResolveCodexAuthContextOptions) and native/pool
callers only if choosing the alternative of making modelId mandatory, otherwise
no direct change is required there. Add a focused regression test in
tests/codex-routing.test.ts covering a reset-derived 429 recorded with a native
modelId followed by resolution without modelId, which must not select that
account.
- Around line 1187-1188: Thread quotaScope through applyFailureFailover and its
callers, including the invocation after applyQuotaAutoSwitch. Ensure its
alternate-account selection and promotion use the scoped pool, and apply the
same promotion gating as applyQuotaAutoSwitch so scoped failover does not read
or update shared active-account cursors.
In `@src/config.ts`:
- Around line 138-221: Extract the shared residual-temp cleanup and
error-classification policy from atomicWriteFile and atomicWriteFileAsync into
one audited helper that supports both synchronous and asynchronous I/O. Update
both wrappers to use it while preserving truncate, empty rewrite, double unlink,
re-hardening, and the existing
AtomicWriteSecretResidualError/AtomicWriteResidualTempError outcomes; keep
atomicWriteFile synchronous for current callers.
- Around line 1395-1411: Define a shared default-port constant near the
configuration schema, then replace the hard-coded 10100 in both the schema
default and readPersistedServerBinding with that constant. Ensure
saveConfigPreservingClaudeCode and normal schema parsing use the same value.
- Around line 713-731: Normalize account IDs consistently before collision
validation. Update configuredCodexPoolAccountIds to store
codexProviderNamespaceKey(id), and use the same normalized key when building
namespaceTargets and checking configuredAccountIds in the account namespace
validation loop; preserve the existing provider-namespace check and collision
errors.
In `@src/images/artifacts.ts`:
- Around line 528-531: In the video data-URI and streamed download paths, update
the validation order so the MAX_VIDEO_DOWNLOAD_BYTES cap is checked before
calling chargeVideoBudget. Ensure oversized payloads throw immediately without
mutating budget.spent, while valid payloads retain the existing per-turn budget
enforcement.
In `@src/images/fulfill-video.ts`:
- Around line 62-65: Update the duration validation in the fulfillment logic
around result.duration to require Number.isFinite(obj.duration) before flooring
and clamping it, so NaN and Infinity are not emitted to the xAI request.
Preserve the existing 1–15 clamp for finite numeric durations.
In `@src/images/loop.ts`:
- Around line 615-621: In the deadline setup within the video processing flow,
update the linkedDeadline assignment to always call AbortSignal.any with signal
and deadlineSignal. Remove the unnecessary signal truthiness branch, since
signal is guaranteed to be provided by internalAbort.signal in this scope.
- Around line 599-606: Extract the repeated defensive JSON parsing into a single
helper, such as parseToolCallArgs, near the enclosing function or existing
utility symbols. Invoke it once per call before the video/image branch, then
replace the parsing blocks at the budget-exhausted path and the other referenced
branches with the resulting pArgs value, preserving the {} fallback and
rejection of null or arrays.
- Around line 610-614: Export DEFAULT_VIDEO_TIMEOUT_MS from fulfill-video.ts and
update the video timeout assignment in the loop flow to use that shared constant
instead of the hardcoded 300_000 value. Import and reuse the exported symbol
without changing the existing nullish-coalescing behavior.
- Around line 703-730: Carry the media kind from the existing isVideoCall value
into each fulfilled entry, then use that explicit flag in the survivor-rewrite
logic around the markdown update instead of inferring the kind from
markdown.startsWith("!["). Preserve the current image and video markdown URL
formats while making them independent of prompt-visible markdown text.
In `@src/images/plan.ts`:
- Around line 92-96: Add a concise comment above or within planVideoBridge
explaining that it intentionally remains async for parity with planImageBridge
and its awaited call sites, despite containing no await or I/O. Do not change
the function signature or behavior.
In `@src/images/synthetic-tool.ts`:
- Around line 117-130: Define shared exported constants for the resolution and
aspect-ratio allowlists, then update the schema in the synthetic video tool and
validation in parseVideoCallArgs within fulfill-video.ts to reference them.
Remove the duplicated inline arrays so model-advertised values and parser
validation always remain synchronized.
- Around line 114-116: Update the video tool description near the duration and
resolution defaults to avoid claiming 6 seconds and 720p when xAI may apply
different defaults. Either document the actual provider defaults from xAI’s
video client behavior or state that duration and resolution use the provider
default, keeping the description aligned with the fields forwarded by the xAI
client.
In `@src/images/types.ts`:
- Around line 27-41: No change is required; the reviewer considers the
duplicated VideoBridgePlan shape acceptable and the suggested shared
MediaBridgePlanBase refactor optional. Preserve the existing ImageBridgePlan and
VideoBridgePlan definitions.
In `@src/images/xai-video-client.ts`:
- Around line 91-104: Wrap JSON.parse in both submitVideoJob and pollVideoJob
with error handling that throws a non-retryable error carrying the HTTP response
status when the successful response body is malformed or non-JSON. Preserve the
existing bounded-body read and request ID validation, and do not include
response bodies, credentials, or account identifiers in the error message.
- Around line 143-162: Update the status normalization logic near rawStatus in
the xAI poll response handling to emit a warning once when an unrecognized
status/state falls through to processing, including the raw value. Ensure
repeated polls for the same operation do not duplicate the warning, while
preserving existing mappings and retry behavior for unknown states.
In `@src/lib/windows-secret-acl.ts`:
- Around line 363-367: Add a development-mode validation around timeoutMemoKey
usage in timeoutMemoKey and its timedOutPaths read/write paths, rejecting or
warning when the memo key is a parent directory rather than the destination
file. Preserve targetPath-based hardenedPaths caching and existing fallback
behavior, while ensuring invalid directory keys cannot poison timeout
memoization across files.
- Around line 111-139: Update defaultAsyncIcaclsRunner to begin draining
proc.stdout immediately after spawning the process, before awaiting proc.exited,
while preserving the existing empty-string fallback when stdout is unavailable
or unreadable. Await the captured stdout result after process completion and
retain the current timeout, exitCode, and success semantics.
In `@src/oauth/index.ts`:
- Around line 712-717: Remove the throwaway preCommitConfig block around
upsertOAuthProvider, since it only repeats the earlier preflight validation
without persisting anything. Rely on the existing validation before credential
persistence and the authoritative post-credential upsert in the surrounding
OAuth flow.
In `@src/router.ts`:
- Around line 296-299: Update the remediation message in the constructor’s Model
error near the visible super call to list “ocx provider add openai”, “ocx sync”,
and “ocx restart” as separate commands instead of chaining them with &&.
Preserve the existing command order and guidance while making the text
copy-pasteable across supported shells.
In `@src/server/management/oauth-account-routes.ts`:
- Around line 103-105: The merge baseline in the OAuth account flow must only be
captured when readConfigDiagnostics() reports a trustworthy source. Update the
code around persistedBaseline to inspect the returned diagnostics.source, refuse
to start the flow when it is "fallback" (using the route’s existing
error/response handling), and retain the config value only for valid persisted
reads.
In `@src/server/responses/core.ts`:
- Around line 1808-1822: Extend the toolChoice alias remapping near the existing
imgPlan logic to also handle vidPlan.toolNames and video aliases, mapping
matching names to VIDEO_GEN_TOOL_NAME in both allowedTools arrays and single
name selections. Preserve the existing image mappings and ensure the video
rewrite is gated on vidPlan so undeclared video tools are not introduced.
In `@structure/08_openai-provider-tiers.md`:
- Around line 30-31: Update the bulk pause behavior description to say accounts
are mutated when the plan-relevant quota window meets or exceeds the exhaustion
threshold, rather than only at exactly 100%. Preserve the existing qualification
that the window must be freshly confirmed, and that unknown or failed refreshes
are skipped.
In `@tests/codex-auth-api.test.ts`:
- Around line 117-126: Export the login poll interval constant used by the auth
polling logic in auth-api.ts, then import and reuse that symbol in the test’s
setTimeout stub instead of declaring the duplicate 2,000 ms value. Keep the
existing timer-gating behavior unchanged.
In `@tests/codex-auth-context.test.ts`:
- Around line 622-623: Update the negative assertion in the cooldown message
test to check that the message does not contain the scope-free
account-cooling-down wording, rather than the fully masked label
“account-…3c21”. Keep the existing positive assertion and ensure the negative
check remains independent of cooldownAccountLabel’s masking format.
In `@tests/codex-routing.test.ts`:
- Around line 903-922: Update the test setup in “thread affinity LRU cap
includes legacy and native quota scopes” to create
CODEX_THREAD_AFFINITY_MAX_ENTRIES + 1 total affinity entries directly, rather
than deriving threads with division by three. Preserve the existing legacy,
shared, and spark assertions so only the oldest legacy entry is evicted while
the later scoped entries remain affined to account “a”.
In `@tests/oauth-public-surface.test.ts`:
- Around line 54-61: Increase the polling budget in waitForOAuthDone so OAuth
settlement has enough time for disk and platform-specific work, while preserving
the existing polling behavior. Track the elapsed wait duration and include it in
the timeout error to make failures diagnosable; keep all callers and successful
status handling unchanged.
In `@tests/rate-limit-reset-credits.test.ts`:
- Line 266: Replace the source-text assertions with runtime behavior tests: in
tests/rate-limit-reset-credits.test.ts:266, render the relevant account-pool
cards behavior and verify the expected UI state; in
tests/provider-workspace-auth.test.ts:197-199, exercise useCodexAccountPool
through its public hook behavior instead of matching implementation fragments.
Preserve the existing scenarios while removing assertions tied to exact
formatting or source structure.
In `@tests/responses-compaction-routing.test.ts`:
- Around line 307-312: Harden the fetch stub in the compact-request test to
handle an already-aborted signal by settling the promise immediately with its
abort reason, while retaining the abort event listener for signals that abort
later. Replace the executor throw for a missing signal with an explicit
assertion using a tracked flag such as missingSignal, and assert that flag
alongside the existing 499 response expectation.
In `@tests/router.test.ts`:
- Around line 144-147: Update the two routeModel error assertions for disabled
or unavailable OpenAI providers to verify both the NoEnabledOpenAiProviderError
class and the existing message patterns. Preserve the current scenarios and
message checks while adding explicit error-type assertions so instanceof-based
handling remains covered.
In `@tests/windows-secret-acl.test.ts`:
- Around line 515-521: Add a regression test in the async hardening tests using
a deferred promise in setAsyncIcaclsRunnerForTests, so the simulated child
settles only after an explicit release. Start hardenSecretPathAsync, yield once,
assert it has not resolved before release, then release the child and verify the
result is unsuccessful with ETIMEDOUT diagnostics.
---
Outside diff comments:
In `@src/responses/state.ts`:
- Around line 459-468: Update clearResponseStateMemoryForTests to account for
the module-level persistGate alongside persistTimer and pendingPersistPath.
Preserve serialization by draining or awaiting the existing gate before
resetting test state; do not replace it with Promise.resolve while a write may
still be running. If the helper must remain synchronous, document that callers
must await flushResponseState() before invoking it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 794f0776-f7bd-4a71-b89e-44a7f89baf6c
📒 Files selected for processing (131)
.github/AGENTS.md.github/CODEOWNERS.github/scripts/enforce-pr-target.test.cjs.github/scripts/issue-quality.cjs.github/scripts/issue-quality.test.cjs.github/workflows/enforce-pr-target.yml.github/workflows/issue-quality-tests.ymlAGENTS.mdCONTRIBUTING.mdMAINTAINERS.mdREADME.mddocs-site/AGENTS.mddocs-site/src/content/docs/contributing.mddocs-site/src/content/docs/guides/providers.mddocs-site/src/content/docs/guides/video-bridge.mddocs-site/src/content/docs/ja/contributing.mddocs-site/src/content/docs/ja/reference/configuration.mddocs-site/src/content/docs/ko/contributing.mddocs-site/src/content/docs/ko/reference/configuration.mddocs-site/src/content/docs/reference/configuration.mddocs-site/src/content/docs/ru/contributing.mddocs-site/src/content/docs/ru/reference/configuration.mddocs-site/src/content/docs/zh-cn/contributing.mddocs-site/src/content/docs/zh-cn/reference/configuration.mdgui/AGENTS.mdgui/src/api.tsgui/src/components/CodexAccountPool.tsxgui/src/components/codex-account-pool-cards.tsxgui/src/components/codex-account-pool-helpers.tsxgui/src/components/codex-account-pool-main-card.tsxgui/src/components/codex-account-pool-utils.tsgui/src/components/codex-account-reset-modal.tsxgui/src/components/use-add-provider-oauth.tsgui/src/hooks/useCodexAccountPool.tsgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/icons.tsxgui/src/intl-formatters.tsgui/tests/active-account-reauth-aggregate.test.tsgui/tests/add-provider-oauth-url-leak.test.tsxgui/tests/api-auth-memory.test.tsgui/tests/codex-account-pool-behaviour.test.tsxgui/tests/codex-account-pool-controller.test.tsgui/tests/codex-account-pool-toast-tone.test.tsxgui/tests/intl-formatters.test.tsscripts/AGENTS.mdsrc/AGENTS.mdsrc/adapters/cursor/native-exec-shell.tssrc/adapters/cursor/tool-definitions.tssrc/adapters/identity.tssrc/cli/account-auth.tssrc/cli/doctor.tssrc/cli/provider.tssrc/cli/status.tssrc/codex/account-id.tssrc/codex/account-lifecycle.tssrc/codex/account-namespace-match.tssrc/codex/account-namespaces.tssrc/codex/account-pause.tssrc/codex/account-usability.tssrc/codex/auth-api.tssrc/codex/auth-collision.tssrc/codex/auth-context.tssrc/codex/catalog/provider-fetch.tssrc/codex/catalog/sync.tssrc/codex/main-account.tssrc/codex/quota.tssrc/codex/routing.tssrc/codex/subagent-model-fallback.tssrc/config.tssrc/images/artifacts.tssrc/images/fulfill-video.tssrc/images/index.tssrc/images/loop.tssrc/images/plan.tssrc/images/synthetic-tool.tssrc/images/types.tssrc/images/xai-video-client.tssrc/lib/destination-policy.tssrc/lib/windows-secret-acl.tssrc/oauth/index.tssrc/oauth/login-cli.tssrc/providers/alibaba-region-migration.tssrc/responses/state.tssrc/router.tssrc/server/lifecycle.tssrc/server/management/combo-routes.tssrc/server/management/oauth-account-routes.tssrc/server/management/provider-routes.tssrc/server/proxy-liveness.tssrc/server/responses/compact.tssrc/server/responses/core.tssrc/types.tsstructure/08_openai-provider-tiers.mdtests/alibaba-region-migration.test.tstests/alibaba-region-startup.test.tstests/ci-workflows.test.tstests/cli-account.test.tstests/cli-provider.test.tstests/cli-status-json.test.tstests/codex-account-namespaces.test.tstests/codex-auth-api.test.tstests/codex-auth-context.test.tstests/codex-pool-rotation.test.tstests/codex-routing.test.tstests/combo-management-api.test.tstests/config-user-edits.test.tstests/config.test.tstests/cursor-native-exec-policy.test.tstests/cursor-tool-definitions.test.tstests/identity-neutralize.test.tstests/kiro-review-regressions.test.tstests/management-provider-validation.test.tstests/oauth-public-surface.test.tstests/provider-connection-test.test.tstests/provider-workspace-auth.test.tstests/proxy-liveness.test.tstests/rate-limit-reset-credits.test.tstests/responses-compaction-routing.test.tstests/responses-state.test.tstests/router.test.tstests/server-auth.test.tstests/server-combo-failover-e2e.test.tstests/videos/fulfill-video.test.tstests/videos/plan-video.test.tstests/videos/xai-video-client.test.tstests/windows-secret-acl.test.ts
Comment on lines
+694
to
697
| } else if (!softPass && isTooTerseBugReproduction(repro)) { | ||
| reasons.push("Reproduction is too vague to act on."); | ||
| guidance.push("List exact steps, commands, and the observed failure — not only a short phrase."); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Dead guard: !softPass can never be false in this else branch.
softPass is only ever set inside the sibling if (isEmpty(summary) && isEmpty(repro)) branch at Line 668, and no kind === "bug" code before Line 694 can set it. Inside this else, softPass is therefore always false, so the extra condition is inert and misleads readers into thinking a soft-passing report can reach the terse-reproduction check. Drop it (the later !softPass guards at Lines 701/716/734 are meaningful because they run outside the branch).
♻️ Proposed simplification
- } else if (!softPass && isTooTerseBugReproduction(repro)) {
+ } else if (isTooTerseBugReproduction(repro)) {
reasons.push("Reproduction is too vague to act on.");
guidance.push("List exact steps, commands, and the observed failure — not only a short phrase.");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else if (!softPass && isTooTerseBugReproduction(repro)) { | |
| reasons.push("Reproduction is too vague to act on."); | |
| guidance.push("List exact steps, commands, and the observed failure — not only a short phrase."); | |
| } | |
| } else if (isTooTerseBugReproduction(repro)) { | |
| reasons.push("Reproduction is too vague to act on."); | |
| guidance.push("List exact steps, commands, and the observed failure — not only a short phrase."); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/issue-quality.cjs around lines 694 - 697, Remove the
redundant !softPass condition from the terse reproduction check in the
bug-report branch, changing the guard around isTooTerseBugReproduction(repro) to
rely only on that predicate. Preserve the existing reasons and guidance
behavior, and leave the later !softPass guards unchanged.
Comment on lines
+716
to
+727
| if (!softPass && isNewBugForm && os !== null && isUnusableVersion(os)) { | ||
| reasons.push("Operating system is missing or unknown."); | ||
| guidance.push("Add your OS name and version (for example Windows 11 24H2)."); | ||
| } else if ( | ||
| !softPass && | ||
| isNewBugForm && | ||
| os !== null && | ||
| (isEmpty(os) || isRawPlaceholder(os)) | ||
| ) { | ||
| reasons.push("Operating system is missing."); | ||
| guidance.push("Add your OS name and version (for example Windows 11 24H2)."); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
isUnusableVersion(os) is a correct-but-misnamed reuse; also the two OS branches emit identical guidance.
Line 716 classifies the Operating-system field with a matcher named for Version. Behaviour is right (Unknown, 모름, ? are unusable for any environment field, and tests/.../issue-quality.test.cjs Line 827 covers it), but the name will invite a future edit to UNUSABLE_VERSION_RE that silently changes OS validation. Consider exporting a neutrally named alias (for example isUnknownStandIn) and using it for both fields, keeping isUnusableVersion as a thin wrapper so the existing test import at .github/scripts/issue-quality.test.cjs Line 19 stays valid.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/issue-quality.cjs around lines 716 - 727, Add and export a
neutral helper such as isUnknownStandIn for shared unknown-value matching, use
it in the OS validation branch alongside the existing version validation, and
retain isUnusableVersion as a thin compatibility wrapper so current imports and
behavior remain unchanged. Consolidate the two OS branches’ identical reason and
guidance handling without changing validation outcomes.
Comment on lines
+242
to
+307
| const setAccountPaused = useCallback(async (id: string, paused: boolean) => { | ||
| if (pauseMutationRef.current) return { ok: false, reason: "busy" } as const; | ||
| pauseMutationRef.current = { accountId: id }; | ||
| setPauseUpdatingId(id); | ||
| try { | ||
| const response = await fetch(`${apiBase}/api/codex-auth/accounts/pause`, { | ||
| method: "PUT", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ id, paused }), | ||
| }); | ||
| if (!response.ok) return { ok: false, reason: "request" } as const; | ||
| const raw = await response.json().catch(() => ({})); | ||
| const result = (raw && typeof raw === "object" ? raw : {}) as { activeCodexAccountId?: string | null }; | ||
| setAccounts(current => current.map(account => ( | ||
| account.id === id || (id === "__main__" && account.isMain) | ||
| ? { ...account, paused } | ||
| : account | ||
| ))); | ||
| if (Object.prototype.hasOwnProperty.call(result, "activeCodexAccountId")) { | ||
| const nextActiveId = result.activeCodexAccountId ?? null; | ||
| pendingActiveIdRef.current = { id: nextActiveId }; | ||
| setActiveId(nextActiveId); | ||
| } | ||
| void load(); | ||
| return { ok: true } as const; | ||
| } catch { | ||
| return { ok: false, reason: "request" } as const; | ||
| } finally { | ||
| pauseMutationRef.current = null; | ||
| setPauseUpdatingId(null); | ||
| } | ||
| }, [apiBase, load]); | ||
|
|
||
| const pauseExhaustedAccounts = useCallback(async () => { | ||
| if (pauseMutationRef.current) return { ok: false, reason: "busy" } as const; | ||
| pauseMutationRef.current = "bulk"; | ||
| setPausingExhausted(true); | ||
| try { | ||
| const response = await fetch(`${apiBase}/api/codex-auth/accounts/pause-exhausted`, { method: "PUT" }); | ||
| if (!response.ok) return { ok: false, reason: "request" } as const; | ||
| const raw = await response.json().catch(() => ({})); | ||
| const result = (raw && typeof raw === "object" ? raw : {}) as { | ||
| pausedAccountIds?: string[]; | ||
| pausedCount?: number; | ||
| activeCodexAccountId?: string | null; | ||
| }; | ||
| const pausedIds = new Set(result.pausedAccountIds ?? []); | ||
| setAccounts(current => current.map(account => ( | ||
| pausedIds.has(account.id) || (pausedIds.has("__main__") && account.isMain) | ||
| ? { ...account, paused: true } | ||
| : account | ||
| ))); | ||
| if (Object.prototype.hasOwnProperty.call(result, "activeCodexAccountId")) { | ||
| const nextActiveId = result.activeCodexAccountId ?? null; | ||
| pendingActiveIdRef.current = { id: nextActiveId }; | ||
| setActiveId(nextActiveId); | ||
| } | ||
| void load(); | ||
| return { ok: true, pausedCount: result.pausedCount ?? pausedIds.size } as const; | ||
| } catch { | ||
| return { ok: false, reason: "request" } as const; | ||
| } finally { | ||
| pauseMutationRef.current = null; | ||
| setPausingExhausted(false); | ||
| } | ||
| }, [apiBase, load]); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Both pause mutations duplicate the same JSON-parse + activeCodexAccountId reconciliation block.
Lines 253-264 and Lines 282-298 are byte-for-byte equivalent apart from the payload fields: defensive json().catch, object-shape narrowing, hasOwnProperty probe, pendingActiveIdRef write, setActiveId. Extracting one helper keeps the "only reconcile when the server actually reported an active id" invariant in a single place, so a future change to the reconciliation contract cannot drift between the single and bulk paths.
♻️ Suggested extraction
+ const applyServerActiveId = useCallback((result: { activeCodexAccountId?: string | null }) => {
+ if (!Object.prototype.hasOwnProperty.call(result, "activeCodexAccountId")) return;
+ const nextActiveId = result.activeCodexAccountId ?? null;
+ pendingActiveIdRef.current = { id: nextActiveId };
+ setActiveId(nextActiveId);
+ }, []);
+
+ const readJsonObject = async (response: Response) => {
+ const raw = await response.json().catch(() => ({}));
+ return (raw && typeof raw === "object" ? raw : {}) as Record<string, unknown>;
+ };Then both callbacks reduce to const result = await readJsonObject(response); … applyServerActiveId(result);.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/src/hooks/useCodexAccountPool.ts` around lines 242 - 307, Extract the
duplicated defensive response parsing and active-account reconciliation from
setAccountPaused and pauseExhaustedAccounts into shared helpers, such as
readJsonObject and applyServerActiveId. Preserve the existing behavior of
treating invalid JSON as an empty object and updating
pendingActiveIdRef/setActiveId only when activeCodexAccountId is present, then
have both callbacks reuse those helpers.
Comment on lines
+24
to
+25
| expect(hook).toContain("const activeAccount = activePoolAccount ?? mainAccount"); | ||
| expect(hook).toContain("!activeAccount?.paused && accountNeedsReauth(activeAccount)"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test the paused-account behavior, not only its source text.
Lines 24-25 only search the hook source for the new expression; they never run the controller with a paused, reauthentication-required active account. The test can therefore pass while activeNeedsReauth still propagates true. Add focused behavioral coverage asserting that { paused: true, needsReauth: true } produces activeNeedsReauth === false (or extract and test the pure predicate).
As per path instructions, tests must add focused regression coverage for behavior changes in the corresponding subsystem.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/tests/active-account-reauth-aggregate.test.ts` around lines 24 - 25,
Replace the source-text-only assertions in the active account reauthentication
test with focused behavioral coverage that exercises the controller or extracted
predicate using an active account with paused: true and needsReauth: true.
Assert that activeNeedsReauth is false, while preserving coverage of the
relevant active-account selection behavior.
Source: Path instructions
Comment on lines
+21
to
+23
| test("handles invalid dates consistently", () => { | ||
| expect(formatCreditDateTime("invalid")).toBe("—"); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a matching invalid-date test for formatCreditDate.
Only formatCreditDateTime's "—" fallback is exercised here; formatCreditDate shares the same Number.isNaN(date.getTime()) fallback path (gui/src/intl-formatters.ts:48-52) but has no invalid-input regression test in this suite.
✅ Suggested addition
test("handles invalid dates consistently", () => {
expect(formatCreditDateTime("invalid")).toBe("—");
+ expect(formatCreditDate("invalid")).toBe("—");
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("handles invalid dates consistently", () => { | |
| expect(formatCreditDateTime("invalid")).toBe("—"); | |
| }); | |
| test("handles invalid dates consistently", () => { | |
| expect(formatCreditDateTime("invalid")).toBe("—"); | |
| expect(formatCreditDate("invalid")).toBe("—"); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/tests/intl-formatters.test.ts` around lines 21 - 23, Add a test alongside
“handles invalid dates consistently” that passes an invalid date string to
formatCreditDate and asserts the "—" fallback. Keep the existing
formatCreditDateTime assertion unchanged and target the shared invalid-date
behavior in the intl formatter tests.
Comment on lines
+54
to
+61
| async function waitForOAuthDone(provider: string): Promise<ReturnType<typeof getLoginStatus>> { | ||
| for (let attempt = 0; attempt < 200; attempt += 1) { | ||
| const status = getLoginStatus(provider); | ||
| if (status.done) return status; | ||
| await Bun.sleep(5); | ||
| } | ||
| throw new Error(`OAuth login for ${provider} did not settle`); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
waitForOAuthDone gives settlement only ~1s, which is thin for flows doing real disk IO.
200 attempts × Bun.sleep(5) caps the wait at roughly 1 second, and five tests (lines 314, 356, 387, 416, 474) rely on it. Settlement in those paths performs real work under the temp OPENCODEX_HOME: saveCredential writes the credential store, runLogin then re-reads config and calls saveConfig/saveConfigPreservingClaudeCode (atomic write + chmod, plus hardenSecretDir via icacls on Windows runners). On a loaded CI runner that can exceed 1s and the throw at line 60 turns into a spurious failure that looks like a settlement bug.
🕒 Proposed fix: widen the budget and report elapsed time
async function waitForOAuthDone(provider: string): Promise<ReturnType<typeof getLoginStatus>> {
- for (let attempt = 0; attempt < 200; attempt += 1) {
+ const deadline = Date.now() + 10_000;
+ while (Date.now() < deadline) {
const status = getLoginStatus(provider);
if (status.done) return status;
await Bun.sleep(5);
}
- throw new Error(`OAuth login for ${provider} did not settle`);
+ throw new Error(`OAuth login for ${provider} did not settle within 10s`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function waitForOAuthDone(provider: string): Promise<ReturnType<typeof getLoginStatus>> { | |
| for (let attempt = 0; attempt < 200; attempt += 1) { | |
| const status = getLoginStatus(provider); | |
| if (status.done) return status; | |
| await Bun.sleep(5); | |
| } | |
| throw new Error(`OAuth login for ${provider} did not settle`); | |
| } | |
| async function waitForOAuthDone(provider: string): Promise<ReturnType<typeof getLoginStatus>> { | |
| const deadline = Date.now() + 10_000; | |
| while (Date.now() < deadline) { | |
| const status = getLoginStatus(provider); | |
| if (status.done) return status; | |
| await Bun.sleep(5); | |
| } | |
| throw new Error(`OAuth login for ${provider} did not settle within 10s`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/oauth-public-surface.test.ts` around lines 54 - 61, Increase the
polling budget in waitForOAuthDone so OAuth settlement has enough time for disk
and platform-specific work, while preserving the existing polling behavior.
Track the elapsed wait duration and include it in the timeout error to make
failures diagnosable; keep all callers and successful status handling unchanged.
| // Next-session still renders BESIDE the ticket; health projection also suppresses | ||
| // it for projected reauth/cooldown (not only the legacy needsReauth flag). | ||
| expect(source).toContain("{isNext(a.id) && !showReauth && !inCooldown && ("); | ||
| expect(source).toContain("{isNext(a) && !showReauth && !inCooldown && ("); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -i 'codex-account-pool-cards.tsx' gui/src
fd -i 'useCodexAccountPool.ts' gui/src
echo '--- cards ---'
rg -nF 'isNext(a) && !showReauth && !inCooldown && (' gui/src/components/codex-account-pool-cards.tsx
echo '--- hook ---'
rg -nF -e 'accountNeedsReauth(activeAccount)' -e '!activeAccount?.paused &&' -e 'activePoolAccount ?? mainAccount' gui/src/hooks/useCodexAccountPool.tsRepository: lidge-jun/opencodex
Length of output: 562
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tests/rate-limit-reset-credits.test.ts excerpt ---'
sed -n '250,275p' tests/rate-limit-reset-credits.test.ts
echo '--- tests/provider-workspace-auth.test.ts excerpt ---'
sed -n '185,205p' tests/provider-workspace-auth.test.ts
echo '--- gui/src/components/codex-account-pool-cards.tsx excerpt ---'
sed -n '65,90p' gui/src/components/codex-account-pool-cards.tsx
echo '--- gui/src/hooks/useCodexAccountPool.ts excerpt ---'
sed -n '325,340p' gui/src/hooks/useCodexAccountPool.tsRepository: lidge-jun/opencodex
Length of output: 5328
Replace the source-text assertions with behavior checks.
tests/rate-limit-reset-credits.test.ts:266pinsgui/src/components/codex-account-pool-cards.tsxto the exact fragment{isNext(a) && !showReauth && !inCooldown && (.tests/provider-workspace-auth.test.ts:197-199pinsgui/src/hooks/useCodexAccountPool.tsto exact implementation fragments.
These assertions will fail on harmless formatting or refactoring changes even when runtime behavior stays the same; prefer exercising the rendered UI/hook behavior instead.
📍 Affects 2 files
tests/rate-limit-reset-credits.test.ts#L266-L266(this comment)tests/provider-workspace-auth.test.ts#L197-L199
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/rate-limit-reset-credits.test.ts` at line 266, Replace the source-text
assertions with runtime behavior tests: in
tests/rate-limit-reset-credits.test.ts:266, render the relevant account-pool
cards behavior and verify the expected UI state; in
tests/provider-workspace-auth.test.ts:197-199, exercise useCodexAccountPool
through its public hook behavior instead of matching implementation fragments.
Preserve the existing scenarios while removing assertions tied to exact
formatting or source structure.
Comment on lines
+307
to
+312
| globalThis.fetch = ((_url: string | URL | Request, init?: RequestInit) => new Promise<Response>((_resolve, reject) => { | ||
| const signal = init?.signal; | ||
| if (!signal) throw new Error("expected compact request abort signal"); | ||
| signal.addEventListener("abort", () => reject(signal.reason), { once: true }); | ||
| markFetchStarted(); | ||
| })) as typeof fetch; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Guard the fetch stub against an already-aborted signal, otherwise it can hang the suite.
Line 310 only registers an abort listener. addEventListener("abort") never fires for a signal that is already aborted, so if the abort ever lands before the stub is entered (e.g. a scheduling change in handleResponsesCompact, or a retry that reuses the aborted signal), this promise never settles and the test hangs until Bun's default timeout instead of failing with a useful message. The current ordering (await fetchStarted then abort.abort()) happens to be safe, but the stub should not depend on that.
Also note line 309: throwing inside the new Promise executor turns into a rejection rather than a test-visible assertion failure, so the "expected compact request abort signal" case shows up as an opaque fetch error.
🛡️ Proposed hardening of the stub
- globalThis.fetch = ((_url: string | URL | Request, init?: RequestInit) => new Promise<Response>((_resolve, reject) => {
- const signal = init?.signal;
- if (!signal) throw new Error("expected compact request abort signal");
- signal.addEventListener("abort", () => reject(signal.reason), { once: true });
- markFetchStarted();
- })) as typeof fetch;
+ let missingSignal = false;
+ globalThis.fetch = ((_url: string | URL | Request, init?: RequestInit) => new Promise<Response>((_resolve, reject) => {
+ const signal = init?.signal;
+ if (!signal) {
+ missingSignal = true;
+ reject(new Error("expected compact request abort signal"));
+ markFetchStarted();
+ return;
+ }
+ if (signal.aborted) reject(signal.reason);
+ else signal.addEventListener("abort", () => reject(signal.reason), { once: true });
+ markFetchStarted();
+ })) as typeof fetch;…then assert expect(missingSignal).toBe(false) next to the 499 expectation on line 321.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| globalThis.fetch = ((_url: string | URL | Request, init?: RequestInit) => new Promise<Response>((_resolve, reject) => { | |
| const signal = init?.signal; | |
| if (!signal) throw new Error("expected compact request abort signal"); | |
| signal.addEventListener("abort", () => reject(signal.reason), { once: true }); | |
| markFetchStarted(); | |
| })) as typeof fetch; | |
| let missingSignal = false; | |
| globalThis.fetch = ((_url: string | URL | Request, init?: RequestInit) => new Promise<Response>((_resolve, reject) => { | |
| const signal = init?.signal; | |
| if (!signal) { | |
| missingSignal = true; | |
| reject(new Error("expected compact request abort signal")); | |
| markFetchStarted(); | |
| return; | |
| } | |
| if (signal.aborted) reject(signal.reason); | |
| else signal.addEventListener("abort", () => reject(signal.reason), { once: true }); | |
| markFetchStarted(); | |
| })) as typeof fetch; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/responses-compaction-routing.test.ts` around lines 307 - 312, Harden
the fetch stub in the compact-request test to handle an already-aborted signal
by settling the promise immediately with its abort reason, while retaining the
abort event listener for signals that abort later. Replace the executor throw
for a missing signal with an explicit assertion using a tracked flag such as
missingSignal, and assert that flag alongside the existing 499 response
expectation.
Comment on lines
144
to
+147
| expect(() => routeModel({ ...base, providers: { ...base.providers, openai: { ...forward, disabled: true } } }, "gpt-5.5")) | ||
| .toThrow(NoEnabledOpenAiProviderError); | ||
| .toThrow(/requires the canonical openai provider/); | ||
| const unavailable = { ...base, providers: { "openai-proxy": base.providers["openai-proxy"] } }; | ||
| expect(() => routeModel(unavailable, "gpt-5.5")).toThrow(NoEnabledOpenAiProviderError); | ||
| expect(() => routeModel(unavailable, "gpt-5.5")).toThrow(/ocx provider add openai/); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Assert the error class alongside the message so the typed contract stays covered.
Lines 145 and 147 now only match message text, but NoEnabledOpenAiProviderError is still imported on line 3 and is presumably what error-mapping code branches on. As written, this file no longer pins the class identity anywhere: someone could throw a plain Error with the same wording and the suite would stay green, while downstream instanceof handling silently breaks. toThrow accepts a class or a regex, so assert both.
♻️ Keep both the class and the message assertions
expect(() => routeModel({ ...base, providers: { ...base.providers, openai: { ...forward, disabled: true } } }, "gpt-5.5"))
- .toThrow(/requires the canonical openai provider/);
+ .toThrow(NoEnabledOpenAiProviderError);
+ expect(() => routeModel({ ...base, providers: { ...base.providers, openai: { ...forward, disabled: true } } }, "gpt-5.5"))
+ .toThrow(/requires the canonical openai provider/);
const unavailable = { ...base, providers: { "openai-proxy": base.providers["openai-proxy"] } };
- expect(() => routeModel(unavailable, "gpt-5.5")).toThrow(/ocx provider add openai/);
+ expect(() => routeModel(unavailable, "gpt-5.5")).toThrow(NoEnabledOpenAiProviderError);
+ expect(() => routeModel(unavailable, "gpt-5.5")).toThrow(/ocx provider add openai/);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect(() => routeModel({ ...base, providers: { ...base.providers, openai: { ...forward, disabled: true } } }, "gpt-5.5")) | |
| .toThrow(NoEnabledOpenAiProviderError); | |
| .toThrow(/requires the canonical openai provider/); | |
| const unavailable = { ...base, providers: { "openai-proxy": base.providers["openai-proxy"] } }; | |
| expect(() => routeModel(unavailable, "gpt-5.5")).toThrow(NoEnabledOpenAiProviderError); | |
| expect(() => routeModel(unavailable, "gpt-5.5")).toThrow(/ocx provider add openai/); | |
| expect(() => routeModel({ ...base, providers: { ...base.providers, openai: { ...forward, disabled: true } } }, "gpt-5.5")) | |
| .toThrow(NoEnabledOpenAiProviderError); | |
| expect(() => routeModel({ ...base, providers: { ...base.providers, openai: { ...forward, disabled: true } } }, "gpt-5.5")) | |
| .toThrow(/requires the canonical openai provider/); | |
| const unavailable = { ...base, providers: { "openai-proxy": base.providers["openai-proxy"] } }; | |
| expect(() => routeModel(unavailable, "gpt-5.5")).toThrow(NoEnabledOpenAiProviderError); | |
| expect(() => routeModel(unavailable, "gpt-5.5")).toThrow(/ocx provider add openai/); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/router.test.ts` around lines 144 - 147, Update the two routeModel error
assertions for disabled or unavailable OpenAI providers to verify both the
NoEnabledOpenAiProviderError class and the existing message patterns. Preserve
the current scenarios and message checks while adding explicit error-type
assertions so instanceof-based handling remains covered.
Comment on lines
+515
to
+521
| test("async timeout soft-fails with the same policy as sync", async () => { | ||
| setAsyncIcaclsRunnerForTests(async () => timeout); | ||
| const result = await hardenSecretPathAsync(secretFile(), { required: true }); | ||
| expect(result.ok).toBe(false); | ||
| expect(result.diagnostics).toContain("ETIMEDOUT"); | ||
| expect(warnings.some(w => w.includes("continuing without NTFS ACL harden"))).toBe(true); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
The central fix — awaiting child settlement after the timer fires — is not actually exercised.
setAsyncIcaclsRunnerForTests(async () => timeout) resolves on the first microtask, so this test proves the classification of a timedOut result but never proves the runner awaits a child process that is still alive when the timeout deadline passes. That is precisely the regression #612 targets ("awaits child-process settlement before classification"): a runner that classified on timer expiry while leaving the child unreaped would pass all four tests here.
Suggested addition in this describe block — a runner that only resolves after a deferred release, asserting hardenSecretPathAsync does not settle early:
💚 Proposed late-settlement regression test
test("async harden waits for a late-settling child before classifying", async () => {
let release!: () => void;
const settled = new Promise<void>(resolve => { release = resolve; });
let resolvedEarly = false;
setAsyncIcaclsRunnerForTests(async () => {
await settled;
return timeout;
});
const pending = hardenSecretPathAsync(secretFile(), { required: true })
.then(result => { resolvedEarly = true; return result; });
await Bun.sleep(0);
expect(resolvedEarly).toBe(false); // no classification before the child settles
release();
const result = await pending;
expect(result.ok).toBe(false);
expect(result.diagnostics).toContain("ETIMEDOUT");
});Want me to open a follow-up issue tracking this coverage gap?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/windows-secret-acl.test.ts` around lines 515 - 521, Add a regression
test in the async hardening tests using a deferred promise in
setAsyncIcaclsRunnerForTests, so the simulated child settles only after an
explicit release. Start hardenSecretPathAsync, yield once, assert it has not
resolved before release, then release the child and verify the result is
unsuccessful with ETIMEDOUT diagnostics.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
icaclswaits on the response-state write path with an async ACL runner that records timeout provenance and awaits child settlement before classifying ([Bug]: synchronous Windows ACL hardening stalls the proxy on response-state writes #612).atomicWriteFileAsync+ destination-path timeout memo (not parent-directory), single-flight response-state persistence, and awaited flush on shutdown; keep owner-first grant before/inheritance:r(fix(windows): grant owner ACE before ACL inheritance removal #601).flushResponseStatein existing snapshot tests.Test plan
bun run typecheckbun test tests/windows-secret-acl.test.ts tests/responses-state.test.ts tests/openai-provider-option-startup.test.tsCloses #612
Summary by CodeRabbit
New Features
Bug Fixes
Documentation