security: harden post-merge service and management boundaries - #707
security: harden post-merge service and management boundaries#707LeoWang331 wants to merge 25 commits into
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:
📝 WalkthroughWalkthroughThis PR separates management and data-plane authentication, adds transactional service-token installation and ownership locking, introduces management model catalog/probe routes, hardens outbound proxy and npm execution behavior, updates GUI routing, and revises multilingual documentation and CI configuration. ChangesSecurity and management
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 25150ee3ed
ℹ️ 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".
| } | ||
| const model = body.model.trim(); | ||
| if (!model || model.length > 512) return jsonResponse({ error: "invalid model" }, 400); | ||
| if (!managementModelTestable(config, model)) { |
There was a problem hiding this comment.
When a configured provider hostname resolves to a loopback, private, link-local, or metadata address—for example after DNS rebinding—this guard still passes because managementModelTestable checks only whether providers exist, are enabled, and are not OpenAI Direct. The probe then dispatches through the ordinary handleResponses path, which does not use the resolved-destination checks applied to provider connection tests, allowing an authenticated management request to send a credentialed POST to an unsafe peer. Validate every possible combo target and the selected physical route with the destination policy before dispatching, and fail without making an upstream request.
AGENTS.md reference: src/AGENTS.md:L17-L20
Useful? React with 👍 / 👎.
| for (const entry of pathEntries) { | ||
| const candidate = posix.join(entry, "npm"); | ||
| if (exists(candidate)) return posix.resolve(candidate); |
There was a problem hiding this comment.
On POSIX systems, if an earlier absolute PATH directory contains a non-executable file or directory named npm, existsSync selects it and the later spawn fails even when a valid executable npm exists later in PATH. Normal command lookup continues past such unusable candidates, so updates that previously worked can now fail; verify that each candidate is an executable regular file (while allowing executable symlinks) and continue searching otherwise.
AGENTS.md reference: AGENTS.md:L109-L115
Useful? React with 👍 / 👎.
| fetch so existing proxy behavior is not silently bypassed. Lowercase `http_proxy`, `https_proxy`, and | ||
| `no_proxy` take precedence over their uppercase forms when both are present, including when the lowercase | ||
| value is empty. The bundled Bun 1.3.14 does not use `ALL_PROXY`; if it is the only effective proxy |
There was a problem hiding this comment.
On POSIX, HTTP_PROXY=http://corp-proxy together with an explicitly empty http_proxy still uses the uppercase proxy: selectedNonBlankProxyEnv skips blank lowercase values and selects the uppercase value, matching the new runtime test's platform-specific expectation. This documentation instead tells operators that the empty lowercase value always wins, so someone following it to suppress an inherited proxy can unexpectedly send provider diagnostics through that proxy. Either make the implementation honor the documented precedence or describe the platform-specific empty-value behavior here and in the translated copies.
AGENTS.md reference: docs-site/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
| const dataToken = validatedServiceApiToken(env); | ||
| const explicitAdmin = validatedServiceAdminToken(env); | ||
| const adminPointer = env.OCX_ADMIN_TOKEN_FILE?.trim(); | ||
| if (explicitAdmin || !adminPointer) { | ||
| return { dataToken, explicitAdmin, preserveAdminPointer: false }; |
There was a problem hiding this comment.
When OPENCODEX_ADMIN_AUTH_TOKEN equals the explicit data token—or an existing config.apiKeys value—this plan writes the conflicting management credential and the service installation reports success. At service startup, initializeManagementAuthState detects that the management secret is also a data-plane admission secret and disables the entire management API, leaving every /api/* request at 503 even though installation appeared successful. Validate the planned admin token against all current data-plane credentials before writing either token file or changing service assets, and return an actionable install error on conflict.
AGENTS.md reference: src/AGENTS.md:L17-L20
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🤖 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 `@docs-site/src/content/docs/ja/reference/configuration.md`:
- Around line 165-169: Update the non-loopback startup requirements consistently
across docs-site/src/content/docs/ja/reference/configuration.md:165-169,
readme/README.ru.md:450-452, readme/README.ko.md:423-425, and
readme/README.ja.md:409-411. Include the protected service-api-token file
alongside OPENCODEX_API_AUTH_TOKEN and config.apiKeys; in the Russian text,
explicitly name both OPENCODEX_API_AUTH_TOKEN and service-api-token. Keep the
existing distinction between data-plane credentials and admin authentication.
- Around line 182-184: Add the http or text language specifier to the repeated
fenced header code blocks at
docs-site/src/content/docs/ja/reference/configuration.md lines 182-184,
readme/README.ru.md lines 455-457, readme/README.ko.md lines 427-429, and
readme/README.ja.md lines 413-415, without changing their contents.
In `@docs-site/src/content/docs/ko/reference/configuration.md`:
- Around line 189-191: Add a consistent text or http language identifier to the
header-example code fences in
docs-site/src/content/docs/ko/reference/configuration.md lines 189-191,
docs-site/src/content/docs/reference/configuration.md lines 271-273, and
docs-site/src/content/docs/zh-cn/reference/configuration.md lines 176-178.
In `@docs-site/src/content/docs/ru/reference/configuration.md`:
- Around line 221-223: Update the fenced code block containing
x-opencodex-api-key to declare the http language, preserving its existing
content and formatting.
In `@src/lib/admin-secrets.ts`:
- Around line 49-51: Update loadServiceAdminTokenFromFile() to reject tokens
containing carriage return, line feed, or NUL characters before returning them,
matching readExistingToken(..., "service") validation. Preserve the existing
file checks, trimming, and null behavior for valid or empty contents.
In `@src/lib/config-ownership.ts`:
- Around line 512-545: Update withOwnershipLock and the removal flow around
removeOwnedConfigStateLocked to refresh the ownership lock lease while a
long-running action is still active. Add a token-validated touchOwnershipLock
helper using the existing lock snapshot and file timestamp APIs, and invoke it
periodically during the manifest/removal walk so the lock mtime remains within
OWNERSHIP_LOCK_STALE_MS; keep refresh best-effort and preserve releaseOwnedLock
cleanup.
- Around line 152-156: Centralize ownership-infrastructure detection by
exporting isOwnershipInfrastructureName from config-ownership.ts, covering the
stable lock, recovery lock, recovery claim prefix, and publish-temp names, then
replace the partial filters at the anchor and config-ownership sites around
lines 183-186, 593-594, and 669-671. In src/service.ts lines 361-372, import and
use this predicate in isLegacyUnownedConfigDir instead of the hardcoded
lock-name check so all infrastructure-only directories are excluded from legacy
classification.
- Around line 406-419: Update recoveryClaimsAreClear, releaseRecoveryLock, and
reclaimStaleOwnershipLock to remove claim files only after re-checking their
filesystem identity (dev/ino) against the verified snapshot, matching
unlinkPublishTempIfOwned. Extend or reuse the existing identity-removal helper
for token-verified claims, preserving ENOENT handling and refusing removal when
the path has been replaced.
In `@src/lib/service-secrets.ts`:
- Around line 32-38: Make loadServiceTokensIntoEnv honor its _configDir
parameter by resolving service-api-token from that directory as a fallback when
loadServiceTokenFromFile(env) finds no token, while preserving the existing
environment-file precedence and token assignment behavior.
In `@src/lib/winsw.ts`:
- Around line 100-108: The WinSW token-definition state and admin-token fallback
duplicate the shared service contract, risking inconsistent OCX_ADMIN_TOKEN_FILE
behavior. Export and reuse the existing ServiceTokenDefinitionState contract and
serviceAdminTokenFileForDefinition resolver from a shared admin-secrets module,
then update the WinSW definition logic around adminTokenFile to use them while
preserving current fallback behavior.
In `@src/service.ts`:
- Around line 566-573: Update the rollback restore flow around writeFileSync,
chmodSync, and renameSync to make chmodSync best-effort, matching the
forward-write paths: catch and ignore chmod failures while preserving the 0o600
mode supplied to writeFileSync, then continue with ACL hardening and rename.
- Around line 361-372: Align isLegacyUnownedConfigDir with config-ownership’s
definition of non-content by exporting an isOwnershipInfrastructureName
predicate from config-ownership that covers the ownership lock, recovery lock,
recovery claim prefix, and publish temporary names. Import and use that
predicate in isLegacyUnownedConfigDir instead of hardcoding
".opencodex-owner.lock", so directories containing only ownership infrastructure
are treated as empty and the existing ownership guard remains fail-closed.
- Line 301: Update the data-plane token ownership-registration flow around
recordOwnedPath to honor its boolean result, matching the management-token
handling near the corresponding registration logic. Refuse or fail the
token-writing operation when registration returns false, while preserving the
existing legacy unowned-directory exception used by the management token path,
so credentials are only written after successful ownership registration.
In `@tests/config-ownership-uninstall.test.ts`:
- Around line 792-810: Make the ownership lock timeout configurable instead of
forcing refusal cases to wait the hard-coded 5 seconds. Thread an optional
timeout through withOwnershipLock and its callers recordOwnedConfigPath and
removeOwnedConfigState, or use the established OCX_OWNERSHIP_LOCK_TIMEOUT_MS
override, while preserving the existing default timeout and refusal semantics.
- Around line 582-639: Add an explicit injection marker to the child process in
the test “revalidates the open fixed lock after the final path identity check,”
writing it when fixedNameChecks === 2. Pass the marker path through the child
environment, then assert existsSync(markerPath) is true alongside the existing
exit and ownership assertions so the targeted tampering branch is confirmed to
execute.
In `@tests/model-test-management-api.test.ts`:
- Around line 624-631: Export a named MODEL_PROBE_TIMEOUT_MS constant from the
model-routes module and use it when creating the probe deadline instead of the
hardcoded value. Update the management model probe test’s setTimeout
interception to compare against this exported constant, keeping the existing
timeout-to-504 assertion behavior unchanged.
In `@tests/provider-outbound.test.ts`:
- Around line 419-431: Increase the timeout governing the nested-spawn routing
matrix in the test around runIsolatedBun to provide Windows and slow-DNS
headroom, using a platform-aware budget with a larger win32 value while
retaining a suitable non-Windows timeout. Keep the routing assertions and
per-case process behavior unchanged.
- Line 440: Update the assertion around result.exitCode to pass only the value
to bun:test’s expect; if stderr diagnostics are needed, log result.stderr
separately before asserting.
In `@tests/proxy-env.test.ts`:
- Around line 73-78: Remove the applyWithEnv cast and invoke applyProxyEnv
directly with the configuration and env arguments, matching the sibling tests
and preserving compile-time checking against its declared signature.
In `@tests/service.test.ts`:
- Around line 646-647: Guard the scriptWriteAt and xmlWriteAt results from
installWindows.search before comparing their order: assert that both values are
not -1 so each expected writeServiceAssetWithRetry call is present, then retain
the ordering assertion.
- Around line 487-493: Update the buildWindowsServiceScript() call in the
escaping test to pass an explicit state with adminTokenFile set to null. Keep
the existing assertions unchanged so the test remains focused on batch escaping
while avoiding serviceAdminTokenFileForDefinition(undefined) and its ambient
filesystem probe.
🪄 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: d9006a14-ec86-4e5f-873e-963465877758
📒 Files selected for processing (79)
.github/workflows/ci.ymlREADME.mddocs-site/src/content/docs/guides/web-dashboard.mddocs-site/src/content/docs/ja/guides/web-dashboard.mddocs-site/src/content/docs/ja/reference/cli.mddocs-site/src/content/docs/ja/reference/configuration.mddocs-site/src/content/docs/ko/guides/web-dashboard.mddocs-site/src/content/docs/ko/reference/cli.mddocs-site/src/content/docs/ko/reference/configuration.mddocs-site/src/content/docs/reference/architecture.mddocs-site/src/content/docs/reference/cli.mddocs-site/src/content/docs/reference/configuration.mddocs-site/src/content/docs/ru/guides/web-dashboard.mddocs-site/src/content/docs/ru/reference/architecture.mddocs-site/src/content/docs/ru/reference/cli.mddocs-site/src/content/docs/ru/reference/configuration.mddocs-site/src/content/docs/zh-cn/guides/web-dashboard.mddocs-site/src/content/docs/zh-cn/reference/architecture.mddocs-site/src/content/docs/zh-cn/reference/cli.mddocs-site/src/content/docs/zh-cn/reference/configuration.mdgui/src/api-access-models.tsgui/src/pages/ApiKeys.tsxgui/src/pages/api-keys-panels.tsxgui/tests/apikeys-layout.test.tsgui/tests/apikeys-refresh-preserve.test.tsxgui/tests/provider-auth-login-copy-link.test.tsxgui/tests/vite-management-proxy.test.tsgui/vite.config.tsreadme/README.ja.mdreadme/README.ko.mdreadme/README.ru.mdreadme/README.zh-CN.mdsrc/cli/help.tssrc/cli/index.tssrc/cli/opencode.tssrc/codex/auth-api.tssrc/config.tssrc/lib/admin-secrets.tssrc/lib/config-ownership.tssrc/lib/destination-policy.tssrc/lib/process-control.tssrc/lib/provider-outbound.tssrc/lib/proxy-env.tssrc/lib/service-secrets.tssrc/lib/winsw.tssrc/oauth/health.tssrc/server/auth-cors.tssrc/server/gui-static.tssrc/server/index.tssrc/server/management-auth.tssrc/server/management/model-routes.tssrc/service.tssrc/update/npm-invocation.d.mtssrc/update/npm-invocation.mjsstructure/04_transports-and-sidecars.mdtests/anthropic-account-pool.test.tstests/ci-workflows.test.tstests/claude-outbound.test.tstests/cli-restore-back.test.tstests/codex-auth-api.test.tstests/config-ownership-uninstall.test.tstests/destination-policy-resolved.test.tstests/forward-admission-separation.test.tstests/helpers/management-auth.tstests/model-test-management-api.test.tstests/oauth-health.test.tstests/opencode-cli.test.tstests/process-control-graceful.test.tstests/provider-outbound.test.tstests/proxy-env.test.tstests/server-auth.test.tstests/server-kiro-oauth-401-replay.test.tstests/server-management-auth.test.tstests/service-admin-token.test.tstests/service.test.tstests/update-npm-invocation.test.tstests/usage-debug.test.tstests/windows-deploy-close-regressions.test.tstests/winsw.test.ts
| 非 loopback バインドではデータプレーン環境 token と `config.apiKeys` の両方がないとプロキシは起動しません。 | ||
| 管理面は独立しており、 | ||
| `OPENCODEX_ADMIN_AUTH_TOKEN` または別途生成され権限が保護された `admin-api-token` ファイルだけが | ||
| `/api/*` を認証します。サービスをインストールすると、明示した管理 token は保護された | ||
| `service-admin-token` 配布ファイルに保存されます。これは管理認証情報の配布方法であり、第四の認証情報種別ではありません。 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the non-loopback startup requirement consistent with the listed credential sources. Each page lists service-api-token as a valid data-plane credential, then describes startup as requiring only an environment variable or config.apiKeys, which can mislead service users with a file-backed token.
docs-site/src/content/docs/ja/reference/configuration.md#L165-L169: Includeservice-api-tokenin the startup condition or explicitly scope it to service startup.readme/README.ru.md#L450-L452: Replace “environment variable andconfig.apiKeys” withOPENCODEX_API_AUTH_TOKEN/service-api-tokenandconfig.apiKeys.readme/README.ko.md#L423-L425: Include the protectedservice-api-tokenfile in the startup requirement.readme/README.ja.md#L409-L411: Include the protectedservice-api-tokenfile in the startup requirement.
📍 Affects 4 files
docs-site/src/content/docs/ja/reference/configuration.md#L165-L169(this comment)readme/README.ru.md#L450-L452readme/README.ko.md#L423-L425readme/README.ja.md#L409-L411
🤖 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 `@docs-site/src/content/docs/ja/reference/configuration.md` around lines 165 -
169, Update the non-loopback startup requirements consistently across
docs-site/src/content/docs/ja/reference/configuration.md:165-169,
readme/README.ru.md:450-452, readme/README.ko.md:423-425, and
readme/README.ja.md:409-411. Include the protected service-api-token file
alongside OPENCODEX_API_AUTH_TOKEN and config.apiKeys; in the Russian text,
explicitly name both OPENCODEX_API_AUTH_TOKEN and service-api-token. Keep the
existing distinction between data-plane credentials and admin authentication.
| ``` | ||
| x-opencodex-api-key: your-secret-token | ||
| x-opencodex-api-key: the-token-for-this-request-plane | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add languages to the repeated header code blocks. These four changed fenced blocks trigger the same Markdown lint warning.
docs-site/src/content/docs/ja/reference/configuration.md#L182-L184: Usehttportext.readme/README.ru.md#L455-L457: Usehttportext.readme/README.ko.md#L427-L429: Usehttportext.readme/README.ja.md#L413-L415: Usehttportext.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 182-182: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
📍 Affects 4 files
docs-site/src/content/docs/ja/reference/configuration.md#L182-L184(this comment)readme/README.ru.md#L455-L457readme/README.ko.md#L427-L429readme/README.ja.md#L413-L415
🤖 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 `@docs-site/src/content/docs/ja/reference/configuration.md` around lines 182 -
184, Add the http or text language specifier to the repeated fenced header code
blocks at docs-site/src/content/docs/ja/reference/configuration.md lines
182-184, readme/README.ru.md lines 455-457, readme/README.ko.md lines 427-429,
and readme/README.ja.md lines 413-415, without changing their contents.
Source: Linters/SAST tools
| ``` | ||
| x-opencodex-api-key: your-secret-token | ||
| x-opencodex-api-key: the-token-for-this-request-plane | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to all header-example fences.
The same untyped code fence appears in three locale pages and triggers MD040. Use text or http consistently:
docs-site/src/content/docs/ko/reference/configuration.md#L189-L191: annotate the Korean header example.docs-site/src/content/docs/reference/configuration.md#L271-L273: annotate the English header example.docs-site/src/content/docs/zh-cn/reference/configuration.md#L176-L178: annotate the Chinese header example.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 189-189: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
📍 Affects 3 files
docs-site/src/content/docs/ko/reference/configuration.md#L189-L191(this comment)docs-site/src/content/docs/reference/configuration.md#L271-L273docs-site/src/content/docs/zh-cn/reference/configuration.md#L176-L178
🤖 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 `@docs-site/src/content/docs/ko/reference/configuration.md` around lines 189 -
191, Add a consistent text or http language identifier to the header-example
code fences in docs-site/src/content/docs/ko/reference/configuration.md lines
189-191, docs-site/src/content/docs/reference/configuration.md lines 271-273,
and docs-site/src/content/docs/zh-cn/reference/configuration.md lines 176-178.
Source: Linters/SAST tools
| ``` | ||
| x-opencodex-api-key: your-secret-token | ||
| x-opencodex-api-key: the-token-for-this-request-plane | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to the fenced block so the docs build lints clean.
markdownlint (MD040) flags the bare fence at Line 221. The neighbouring shell example at Lines 195-199 is tagged, so this is the only untagged block in the rewritten section; http renders the header example with sane highlighting.
📝 Proposed fix
-```
+```http
x-opencodex-api-key: the-token-for-this-request-plane</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 221-221: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs-site/src/content/docs/ru/reference/configuration.md` around lines 221 -
223, Update the fenced code block containing x-opencodex-api-key to declare the
http language, preserving its existing content and formatting.
Source: Linters/SAST tools
| const stat = lstatSync(path); | ||
| if (!stat.isFile() || stat.isSymbolicLink() || stat.size > 512) return null; | ||
| return readFileSync(path, "utf8").trim() || null; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Reject control characters in service management tokens.
loadServiceAdminTokenFromFile() accepts embedded CR, LF, or NUL, while readExistingToken(..., "service") in src/server/management-auth.ts rejects them. Consequently, configuredAdminToken() can return a credential that the server refuses and that may also fail when inserted into a Headers object.
Proposed fix
- return readFileSync(path, "utf8").trim() || null;
+ const token = readFileSync(path, "utf8").trim();
+ return token && !/[\r\n\0]/.test(token) ? token : null;📝 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.
| const stat = lstatSync(path); | |
| if (!stat.isFile() || stat.isSymbolicLink() || stat.size > 512) return null; | |
| return readFileSync(path, "utf8").trim() || null; | |
| const stat = lstatSync(path); | |
| if (!stat.isFile() || stat.isSymbolicLink() || stat.size > 512) return null; | |
| const token = readFileSync(path, "utf8").trim(); | |
| return token && !/[\r\n\0]/.test(token) ? token : null; |
🤖 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/lib/admin-secrets.ts` around lines 49 - 51, Update
loadServiceAdminTokenFromFile() to reject tokens containing carriage return,
line feed, or NUL characters before returning them, matching
readExistingToken(..., "service") validation. Preserve the existing file checks,
trimming, and null behavior for valid or empty contents.
| const child = Bun.spawn([process.execPath, "-e", fetchScript], { | ||
| env: { ...cleanEnv, ...routeEnv }, | ||
| stdout: "ignore", | ||
| stderr: "ignore", | ||
| }); | ||
| const exitCode = await child.exited; | ||
| await Bun.sleep(20); | ||
| routes.push({ | ||
| name, | ||
| route: events.A.length > beforeA ? "A" : events.B.length > beforeB ? "B" : "direct", | ||
| exitCode, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
The nested-spawn routing matrix runs 14 grandchild processes inside one 20s budget — confirm the Windows headroom.
Each case at Lines 419-431 pays a full Bun process start, a fetch with AbortSignal.timeout(2500), and a fixed Bun.sleep(20), all inside the single 20 s timeout declared at Line 461 — and that is on top of the outer runIsolatedBun child's own startup. The direct cases only stay cheap because route-target.invalid fails DNS quickly; on a runner where an unresolvable name is slow (or DNS-hijacked by a captive resolver), each of those five cases can drift toward the 2.5 s abort and push the total past 20 s, producing a timeout failure that looks unrelated to proxy routing. Given this PR already carries commits stabilizing timing-sensitive tests and adjusting platform-specific budgets, consider raising the budget on Windows (e.g. process.platform === "win32" ? 60_000 : 30_000) and/or lowering the per-case AbortSignal.timeout now that the proxies answer immediately.
Also applies to: 461-461
🤖 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/provider-outbound.test.ts` around lines 419 - 431, Increase the timeout
governing the nested-spawn routing matrix in the test around runIsolatedBun to
provide Windows and slow-DNS headroom, using a platform-aware budget with a
larger win32 value while retaining a suitable non-Windows timeout. Keep the
routing assertions and per-case process behavior unchanged.
|
|
||
| const result = await runIsolatedBun(script, childEnv); | ||
|
|
||
| expect(result.exitCode, result.stderr).toBe(0); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Bun test expect second argument label message support
💡 Result:
As of July 29, 2026, the Bun expect() function does not officially support passing a custom message or label as a second argument to the expect() call itself [1][2][3]. While feature requests for this capability exist in the Bun issue tracker [1][4], the current API for expect() in the bun:test module accepts only the actual value to be tested as its single argument [2][3]. To provide custom failure messages in Bun tests, you should instead handle the message within custom matchers defined via expect.extend() [2][5]. When creating custom matchers, the implementation function must return an object containing a message property (a function that returns the error string) [2][6][5]. This allows you to construct dynamic or custom error messages that are displayed when your assertion fails [2][5]. Example of custom message in a custom matcher: expect.extend({ toBeWithinRange(actual, min, max) { const pass = actual >= min && actual <= max; return { pass: pass, message: => expected ${actual} to be within range ${min} .. ${max}, }; }, });
Citations:
- 1: Add custom message to
expectfunction ofbun:testmodule oven-sh/bun#10414 - 2: https://bun.sh/reference/bun/test/Expect
- 3: https://bun.com/reference/bun/test/Expect
- 4: Support for custom test matcher messages to provide failure context oven-sh/bun#5408
- 5: https://bun.com/reference/bun/test/Expect/extend
- 6: https://bun.com/reference/bun/test/Matchers
Drop the second expect() argument here. bun:test’s expect() takes a single value, so result.stderr is ignored and won’t show up in a failing run. If you want child diagnostics, log result.stderr before the assertion or fold it into the asserted value.
🤖 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/provider-outbound.test.ts` at line 440, Update the assertion around
result.exitCode to pass only the value to bun:test’s expect; if stderr
diagnostics are needed, log result.stderr separately before asserting.
| const applyWithEnv = applyProxyEnv as unknown as ( | ||
| config: OcxConfig, | ||
| targetEnv: Record<string, string | undefined>, | ||
| ) => void; | ||
|
|
||
| applyWithEnv(configWithProxy("http://proxy.corp:8080"), env); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Drop the applyWithEnv cast — it now suppresses real type checking.
applyProxyEnv already declares env: ProxyEnvMap = process.env (src/config.ts Line 1622), and the sibling tests at Lines 54 and 106 call it with two arguments directly. The as unknown as (...) cast at Lines 73-76 is leftover from the pre-parameter signature and would happily keep compiling if the second parameter were later removed or retyped, silently reducing this test to a no-op assertion set.
♻️ Proposed simplification
const env: Record<string, string | undefined> = {
http_proxy: "",
https_proxy: "",
};
- const applyWithEnv = applyProxyEnv as unknown as (
- config: OcxConfig,
- targetEnv: Record<string, string | undefined>,
- ) => void;
- applyWithEnv(configWithProxy("http://proxy.corp:8080"), env);
+ applyProxyEnv(configWithProxy("http://proxy.corp:8080"), env);📝 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.
| const applyWithEnv = applyProxyEnv as unknown as ( | |
| config: OcxConfig, | |
| targetEnv: Record<string, string | undefined>, | |
| ) => void; | |
| applyWithEnv(configWithProxy("http://proxy.corp:8080"), env); | |
| const env: Record<string, string | undefined> = { | |
| http_proxy: "", | |
| https_proxy: "", | |
| }; | |
| applyProxyEnv(configWithProxy("http://proxy.corp:8080"), env); |
🤖 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/proxy-env.test.ts` around lines 73 - 78, Remove the applyWithEnv cast
and invoke applyProxyEnv directly with the configuration and env arguments,
matching the sibling tests and preserving compile-time checking against its
declared signature.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d19a645139
ℹ️ 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".
| const { handleResponses } = await import("../responses"); | ||
| const response = await handleResponses( |
There was a problem hiding this comment.
When a public provider hostname rebinds between the preflight at lines 189–204 and this dispatch, or returns a 307/308 redirect to a loopback, private, or metadata URL, the probe enters the ordinary handleResponses transport and can send provider credentials and the request body to an unvalidated peer. Fresh evidence in this revision is that the newly added resolver result is discarded, while fetchWithHeaderTimeout still invokes ordinary fetch without a pinned address or manual redirect policy. Carry the validated destination into a probe-specific transport and reject or revalidate redirects before sending the request.
AGENTS.md reference: AGENTS.md:L154-L160
Useful? React with 👍 / 👎.
| try { | ||
| return install(transaction.definitionState); | ||
| } catch (error) { | ||
| rollbackServiceTokenInstall(transaction.snapshots, options, error); |
There was a problem hiding this comment.
When a reinstall fails after rewriting a plist, systemd unit, or Windows launcher—for example, systemctl --user restart fails—this catch restores only the two token files. If the attempted install added or removed the admin-token pointer, the surviving service definition no longer matches the restored credentials, so a previously working service can reject the old management token on its next start. Snapshot and restore the service definition/assets together with the token files, or publish both only after the platform install succeeds.
Useful? React with 👍 / 👎.
| `admin-api-token` file, authorizes only `/api/*`. A service install persists an explicit management | ||
| token through the protected `service-admin-token` delivery file; this remains a management | ||
| credential, not a fourth credential type. |
There was a problem hiding this comment.
For an existing installation that already has admin-api-token, exporting a new OPENCODEX_ADMIN_AUTH_TOKEN before ocx service install does write service-admin-token, but startup selects the primary file first in initializeManagementAuthState, so the newly documented credential does not authorize /api/*. This is especially misleading for the migration scenario immediately above, where operators are told to set a stable management token; either document that the primary file must be removed or retained as the credential, or change startup precedence so an explicit service-token pointer wins.
AGENTS.md reference: docs-site/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
| } else if (!values.preserveAdminPointer) { | ||
| plan = { | ||
| dataToken: values.dataToken, | ||
| adminAction: { kind: "remove" }, | ||
| definitionState: { adminTokenFile: null }, |
There was a problem hiding this comment.
When a service was installed with an explicit management token and the user later runs ocx update from an ordinary shell, the automatic reinstall in src/update/index.ts lines 300–322 invokes service install without OPENCODEX_ADMIN_AUTH_TOKEN or OCX_ADMIN_TOKEN_FILE. This branch interprets that absence as revocation, deletes service-admin-token, and omits its definition pointer, so the restarted service silently switches credentials and existing remote automation loses management access; dataToken: null similarly removes the stored data token. Preserve existing service credentials when variables are merely absent during repair/update, and require an explicit revocation action to remove them.
AGENTS.md reference: AGENTS.md:L154-L160
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/service.ts (1)
1772-1795: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRestore the prior backend when a backend switch fails. Both switch paths remove the running manager before the replacement is installed and verified. A later asset, registration, UAC, or start failure leaves no background service; the token transaction only restores credentials.
src/service.ts#L1772-L1795: if Scheduler setup fails after WinSW removal, remove any partial Scheduler registration and reinstall/restart WinSW before rethrowing.src/service.ts#L1870-L1901: if WinSW installation fails after Scheduler removal, restore/restart the Scheduler backend before rethrowing.Add failure-injection tests for both directions and assert the previous backend remains installed and the persisted backend state is unchanged.
🤖 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/service.ts` around lines 1772 - 1795, Wrap the Scheduler switch flow in src/service.ts lines 1772-1795 with rollback handling: on any asset, registration, UAC, or start failure after WinSW removal, remove any partial Scheduler registration, reinstall and restart WinSW, then rethrow the original failure while preserving the persisted backend state. Apply the corresponding rollback to the WinSW switch flow in src/service.ts lines 1870-1901: if installation fails after Scheduler removal, restore and restart the Scheduler backend before rethrowing. Add failure-injection tests for both directions asserting the prior backend remains installed and persisted state is unchanged.
🤖 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.
Outside diff comments:
In `@src/service.ts`:
- Around line 1772-1795: Wrap the Scheduler switch flow in src/service.ts lines
1772-1795 with rollback handling: on any asset, registration, UAC, or start
failure after WinSW removal, remove any partial Scheduler registration,
reinstall and restart WinSW, then rethrow the original failure while preserving
the persisted backend state. Apply the corresponding rollback to the WinSW
switch flow in src/service.ts lines 1870-1901: if installation fails after
Scheduler removal, restore and restart the Scheduler backend before rethrowing.
Add failure-injection tests for both directions asserting the prior backend
remains installed and persisted state is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 45727c3d-67f4-482d-9f54-40f228865ddb
📒 Files selected for processing (10)
docs-site/src/content/docs/reference/cli.mdgui/src/pages/ApiKeys.tsxgui/src/pages/api-keys-panels.tsxgui/tests/apikeys-layout.test.tsgui/tests/apikeys-refresh-preserve.test.tsxsrc/cli/index.tssrc/server/management/context.tssrc/service.tstests/ci-workflows.test.tstests/service.test.ts
|
NEEDS-SECURITY-REVIEW, and splitting is a practical prerequisite The work here is good, and that is exactly why I do not want it reviewed as one 7,252-line unit. Requiring management token files to be regular non-symlink files and failing closed when hardening or ownership registration fails ( But those are five different threat models, plus a sixth thing that is not a threat model at all. A reviewer who signs off on the npm PATH hardening has said nothing about the ownership-manifest race, and vice versa. Bundling them does not create atomic safety; it means a single approval implicitly covers work the approver may not have read closely. The diff already splits along clean seams — each has its own implementation and test files:
Number 6 should not be in a security PR at all — it is ordinary maintenance and can go in on its own today. For the record on scope, since "81 files, security" invites worst-case assumptions: this does not touch The conflict is one hunk. What happens next: split along the seams above, land 6 immediately as maintenance, and request explicit security review per remaining piece. If you would rather keep it whole, say so and I will arrange review, but expect it to take considerably longer.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/service.ts (1)
1986-2022: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
installWindowsNativeis the only installer without arestoreServiceDefinitionhook, so a failed--nativeswitch leaves the machine with no service manager at all.At Lines 2019-2021 the transaction is given
serviceAssetPathsbut norestoreServiceDefinition. Meanwhile the install body at Lines 1994-2011 deliberately removes the Task Scheduler backend (uninstallWindows()) and verifies the removal before callinginstallWinswService. IfinstallWinswServicethen throws — missing elevation, WinSW SHA/download mismatch (WINSW_SHA256/WINSW_VERSION), or the post-install verification failing —rollbackServiceInstallrestores both token files andwinswXmlPath()/state files, but nothing re-registers the deleted scheduled task. The operator ends up with neither backend installed and only theconsole.errorat Line 2015 telling them to re-runocx service installmanually.That is precisely the state the transaction exists to prevent, and the other three installers already implement it:
installWindows(Lines 1890-1911) reconstructs the prior task frompreviousTaskXml,installLaunchd(Lines 1808-1811) reloads the prior plist, andinstallSystemd(Lines 2149-2159) restores enable/active state. The native path has the same ingredients available —statusWindowsXml()plus thehadSchedulerquery it already performs — so the asymmetry looks like an oversight rather than a deliberate carve-out.Capture the previous scheduler definition before entering the transaction (the body deletes it, so an in-body capture is too late) and mirror the
installWindowsrestore hook:🛡️ Proposed symmetric restore hook for the native backend switch
async function installWindowsNative(): Promise<void> { + const previousTaskXml = statusWindowsXml(); await withServiceTokenInstallTransactionAsync(async state => { recordOwnedConfigPath(getConfigDir(), serviceStatePath()); @@ writeServiceInstallState("native"); }, { serviceAssetPaths: [winswXmlPath(), ...serviceStatePaths()], + restoreServiceDefinition: () => { + if (!previousTaskXml) return; + const definitionPath = `${windowsTaskXmlPath()}.restore.${process.pid}.${randomUUID()}`; + writeFileSync(definitionPath, `\uFEFF${previousTaskXml}`, "utf16le"); + try { + schtasks(["/create", "/tn", TASK, "/xml", definitionPath, "/f"]); + schtasks(["/run", "/tn", TASK]); + } finally { + try { unlinkSync(definitionPath); } catch { /* rollback error is reported by registration */ } + } + }, }); }Worth a focused regression test next to the existing transaction tests in
tests/service-admin-token.test.ts(a failing async install withrestoreServiceDefinitionsupplied is already covered at Lines 303-314, so the shape exists). As per path instructions, "A behavior change in src/ should come with a focused regression test near the existing tests for that 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 `@src/service.ts` around lines 1986 - 2022, Update installWindowsNative to capture the existing Task Scheduler definition with statusWindowsXml before entering withServiceTokenInstallTransactionAsync, since the transaction body removes it. Supply a restoreServiceDefinition hook alongside serviceAssetPaths that re-registers the captured scheduler definition when the native installation fails, mirroring installWindows. Add a focused regression test in the service-admin transaction tests covering failed native installation and scheduler restoration.Source: Path instructions
🤖 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 `@src/lib/pinned-http.ts`:
- Around line 149-172: Update the request body-writer around the async IIFE and
fail/discardNonSuccessBody paths so request teardown settles any pending drain
wait instead of leaving it blocked. Race drain backpressure with request
close/error or otherwise reuse a teardown-aware wait, and cancel the active body
reader when fail() destroys the request. Add a focused provider-outbound
regression test covering a large POST body with early server response or
destruction.
In `@src/lib/provider-outbound.ts`:
- Around line 203-264: Extract the shared proxy, DNS, NO_PROXY, and
private-network policy flow from providerOutboundGet and
createProviderModelProbeFetch into one internal helper. Have both functions use
it for ALL_PROXY validation, bypass decisions, DNS-failure proxy fallback, and
proxied private-network rejection, while retaining only their respective final
dispatch paths: provider GET versus request/pinnedRequest fetch.
In `@tests/windows-deploy-close-regressions.test.ts`:
- Around line 47-52: Guard the source extraction in the install regression test
by storing the indices for the “function installSystemd(): void” and “function
startSystemd(): void” markers, asserting both are found and that the end index
exceeds the start index, then slice using those validated bounds before the
existing assertions.
---
Outside diff comments:
In `@src/service.ts`:
- Around line 1986-2022: Update installWindowsNative to capture the existing
Task Scheduler definition with statusWindowsXml before entering
withServiceTokenInstallTransactionAsync, since the transaction body removes it.
Supply a restoreServiceDefinition hook alongside serviceAssetPaths that
re-registers the captured scheduler definition when the native installation
fails, mirroring installWindows. Add a focused regression test in the
service-admin transaction tests covering failed native installation and
scheduler restoration.
🪄 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: 9469b26c-1dab-4214-9d5d-038511705468
📒 Files selected for processing (24)
docs-site/src/content/docs/reference/configuration.mdgui/src/pages/Storage.tsxgui/src/pages/api-keys-panels.tsxgui/src/ui.tsxgui/tests/claudecode-fetch-errors.test.tsxgui/tests/codex-account-pool-behaviour.test.tsxgui/tests/codex-account-pool-controller.test.tsgui/tests/codex-account-pool-toast-tone.test.tsxgui/tests/collapse-store.test.tsgui/tests/dashboard-tabs.test.tsgui/tests/grok-switch.test.tsxgui/tests/models-empty-provider.test.tsxscripts/test.tssrc/lib/pinned-http.tssrc/lib/provider-outbound.tssrc/server/management/context.tssrc/server/management/model-routes.tssrc/service.tstests/model-test-management-api.test.tstests/provider-outbound.test.tstests/service-admin-token.test.tstests/service.test.tstests/test-runner.test.tstests/windows-deploy-close-regressions.test.ts
💤 Files with no reviewable changes (1)
- gui/src/ui.tsx
|
|
||
| /** | ||
| * Build the transport used only by management model probes. Each actual request | ||
| * resolves immediately before dispatch and passes that result to the fixed-peer | ||
| * transport. Caller-owned provider fetch overrides are intentionally ignored here. | ||
| */ | ||
| export function createProviderModelProbeFetch( | ||
| name: string, | ||
| provider: Pick<OcxProviderConfig, "baseUrl" | "allowPrivateNetwork">, | ||
| dependencies: ProviderOutboundDependencies = {}, | ||
| ): typeof globalThis.fetch { | ||
| const executor = async (input: RequestInfo | URL, init?: RequestInit): Promise<Response> => { | ||
| const request = input instanceof Request && init === undefined | ||
| ? input | ||
| : new Request(input, init); | ||
| const parsed = new URL(request.url); | ||
| const proxyEnv = dependencies.proxyEnv ?? process.env; | ||
| const configuredProxy = configuredProxyFor(parsed, proxyEnv); | ||
| const proxyConfigured = configuredProxy !== undefined; | ||
| if (!configuredProxy && proxyEnvPresent("ALL_PROXY", proxyEnv)) { | ||
| const supportedKey = parsed.protocol === "https:" ? "HTTPS_PROXY" : "HTTP_PROXY"; | ||
| throw new ProviderOutboundPolicyError( | ||
| `ALL_PROXY is not supported by the bundled Bun provider transport; set ${supportedKey} for this provider URL`, | ||
| ); | ||
| } | ||
| const bypassProxy = proxyConfigured && noProxyMatches(parsed, proxyEnv); | ||
| const resolveAddresses = dependencies.resolveAddresses ?? resolvePublicAddresses; | ||
| let resolved: Awaited<ReturnType<typeof resolvePublicAddresses>>; | ||
| try { | ||
| resolved = await resolveAddresses(request.url, { | ||
| context: "provider URL", | ||
| allowPrivateNetwork: provider.allowPrivateNetwork, | ||
| }); | ||
| } catch (error) { | ||
| const dnsResolutionFailed = error instanceof DestinationDnsResolutionError | ||
| || (error instanceof Error && error.name === "DestinationDnsResolutionError"); | ||
| if (!dnsResolutionFailed) { | ||
| throw new ProviderOutboundPolicyError(error instanceof Error ? error.message : "provider destination was blocked"); | ||
| } | ||
| if (!proxyConfigured || bypassProxy) throw error; | ||
| warnProxyBoundaryOnce(); | ||
| warnProxyDnsDegradationOnce(); | ||
| return requestThroughProxy(request, configuredProxy!); | ||
| } | ||
| if (proxyConfigured && !bypassProxy && !resolved.privateNetwork) { | ||
| warnProxyBoundaryOnce(); | ||
| return requestThroughProxy(request, configuredProxy!); | ||
| } | ||
| if (proxyConfigured && resolved.privateNetwork && !bypassProxy) { | ||
| const hostname = normalizeProxyHostname(parsed.hostname); | ||
| throw new ProviderOutboundPolicyError( | ||
| `provider URL resolves to a private-network destination; add ${hostname} to NO_PROXY before using allowPrivateNetwork with an outbound proxy`, | ||
| ); | ||
| } | ||
| const pinnedRequest = dependencies.pinnedRequest ?? pinnedHttpRequest; | ||
| return pinnedRequest(request, pickPinnedAddress(resolved.addresses), { | ||
| rejectUnauthorized: true, | ||
| context: `${name} model probe response`, | ||
| }); | ||
| }; | ||
| return Object.assign(executor, { preconnect: () => undefined }) as typeof globalThis.fetch; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
createProviderModelProbeFetch duplicates providerOutboundGet's proxy/DNS/private-network policy logic.
Per the diff's own description, this function "applies the same proxy environment selection/ALL_PROXY validation/NO_PROXY bypass logic as providerOutboundGet" and "enforces the proxied private-network NO_PROXY policy via ProviderOutboundPolicyError" — essentially the same control flow as providerOutboundGet (Lines 158-193), just re-implemented against a Request/fetch executor shape instead of a URL/GET shape. Since this is the exact security boundary this PR is hardening (outbound proxy routing + private-network destination gating), having it in two places means a future policy fix applied to one function can silently miss the other. Extract the shared decision logic (ALL_PROXY rejection, NO_PROXY bypass check, DNS-failure-through-proxy retry, private-network-behind-proxy rejection) into one internal helper that both providerOutboundGet and createProviderModelProbeFetch call, leaving only the final dispatch (fetch-through-proxy vs. pinnedRequest) as the divergent piece.
🤖 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/lib/provider-outbound.ts` around lines 203 - 264, Extract the shared
proxy, DNS, NO_PROXY, and private-network policy flow from providerOutboundGet
and createProviderModelProbeFetch into one internal helper. Have both functions
use it for ALL_PROXY validation, bypass decisions, DNS-failure proxy fallback,
and proxied private-network rejection, while retaining only their respective
final dispatch paths: provider GET versus request/pinnedRequest fetch.
| const install = src.slice( | ||
| src.indexOf("function installSystemd(): void"), | ||
| src.indexOf("function startSystemd(): void"), | ||
| ); | ||
| expect(install).toMatch(/^function installSystemd\(\): void \{\s*\n\s*ensureUserBusEnv\(\);/); | ||
| expect(install.indexOf("ensureUserBusEnv();")).toBeLessThan(install.indexOf("systemctl --user")); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
rg -n 'function installSystemd\(\): void|function startSystemd\(\): void' tests/windows-deploy-close-regressions.test.tsRepository: lidge-jun/opencodex
Length of output: 269
🏁 Script executed:
#!/bin/bash
set -eu
printf '\n== test file around the referenced lines ==\n'
cat -n tests/windows-deploy-close-regressions.test.ts | sed -n '35,70p'
printf '\n== search for the source markers in the repo ==\n'
rg -n 'function installSystemd\(\): void|function startSystemd\(\): void' -S .
printf '\n== locate likely source file that contains these functions ==\n'
rg -n 'installSystemd|startSystemd|ensureUserBusEnv|systemctl --user' -S .Repository: lidge-jun/opencodex
Length of output: 7554
🏁 Script executed:
#!/bin/bash
set -eu
cat -n tests/windows-deploy-close-regressions.test.ts | sed -n '1,120p'Repository: lidge-jun/opencodex
Length of output: 4418
Guard the slice bounds in tests/windows-deploy-close-regressions.test.ts:47-52.
src.indexOf("function startSystemd(): void") is the end marker; if it ever returns -1, slice(start, -1) will pull in almost the rest of src/service.ts, and these assertions can still pass against unrelated text. Add explicit checks that both indices are found and that installEnd > installStart before slicing.
🤖 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-deploy-close-regressions.test.ts` around lines 47 - 52, Guard
the source extraction in the install regression test by storing the indices for
the “function installSystemd(): void” and “function startSystemd(): void”
markers, asserting both are found and that the end index exceeds the start
index, then slice using those validated bounds before the existing assertions.
…-merge-hardening # Conflicts: # gui/src/pages/Storage.tsx # gui/tests/apikeys-refresh-preserve.test.tsx # gui/tests/claudecode-fetch-errors.test.tsx # gui/tests/codex-account-pool-behaviour.test.tsx # gui/tests/codex-account-pool-controller.test.ts # gui/tests/codex-account-pool-toast-tone.test.tsx # gui/tests/dashboard-tabs.test.ts # scripts/test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gui/tests/apikeys-refresh-preserve.test.tsx (1)
208-214: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTie the confirmation wait to the production interval.
The hardcoded
310ms is an arbitrary margin over the 300 ms confirmation interval. If that interval changes, this test becomes stale or unnecessarily slow. Use the shared interval constant or fake timers instead, while retaining the explicit enabled-state assertion.🤖 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/apikeys-refresh-preserve.test.tsx` around lines 208 - 214, Update the confirmation wait in the test around confirmBtn to use the production confirmation interval constant, adding only the necessary margin or timer advancement instead of hardcoding 310 ms. Preserve the explicit assertion that confirmBtn exists and is enabled.
🤖 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 `@scripts/test.ts`:
- Around line 115-120: Revert the Windows-specific slow-suite threshold change
in the elapsed-time warning logic near slowSuiteThresholdSeconds, restoring the
existing hang-detection budget; keep this security PR limited to
boundary-hardening changes unless the threshold is explicitly derived from and
documented with the shard-count change.
---
Outside diff comments:
In `@gui/tests/apikeys-refresh-preserve.test.tsx`:
- Around line 208-214: Update the confirmation wait in the test around
confirmBtn to use the production confirmation interval constant, adding only the
necessary margin or timer advancement instead of hardcoding 310 ms. Preserve the
explicit assertion that confirmBtn exists and is enabled.
🪄 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: 6d85969d-24e0-4e95-b271-9c30f99313ce
📒 Files selected for processing (6)
gui/src/pages/Storage.tsxgui/tests/apikeys-refresh-preserve.test.tsxgui/tests/codex-account-pool-controller.test.tsgui/tests/grok-switch.test.tsxgui/tests/models-empty-provider.test.tsxscripts/test.ts
| const elapsedSeconds = Math.round((Date.now() - startedAt) / 1000); | ||
| if (requestedTests.length === 0 && elapsedSeconds > 600) { | ||
| const slowSuiteThresholdSeconds = process.platform === "win32" ? 1_200 : 600; | ||
| if (requestedTests.length === 0 && elapsedSeconds > slowSuiteThresholdSeconds) { | ||
| console.warn( | ||
| `[test] the suite took ${elapsedSeconds}s; it normally runs in about 210s on an idle machine. ` | ||
| `[test] the suite took ${elapsedSeconds}s, beyond the expected idle-machine budget. ` | ||
| + "Check for another test runner, a busy CPU, or a test that started polling something real.", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift
Keep the slow-suite policy change separate from this security PR.
Changing the Windows warning budget to 1,200 seconds weakens hang detection and is test/CI maintenance rather than boundary hardening. Move this threshold change to the separate maintenance PR, or derive and document it from the shard-count change.
🤖 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 `@scripts/test.ts` around lines 115 - 120, Revert the Windows-specific
slow-suite threshold change in the elapsed-time warning logic near
slowSuiteThresholdSeconds, restoring the existing hang-detection budget; keep
this security PR limited to boundary-hardening changes unless the threshold is
explicitly derived from and documented with the shard-count change.
2 participants
Summary
Follow-up hardening for #697 based on review findings and regression testing after that PR merged.
This PR intentionally does not broaden the remaining deferred audit scope.
Why
Post-merge review found several edge cases where service repair, ownership-lock recovery, proxy environment selection, and management model probes could degrade insecurely or fail in ways that were hard to diagnose. The changes keep those boundaries explicit and add regression coverage for the failure paths.
Impact
Verification
Executed on Windows against the final branch state:
bun run test: 6092 passed, 4 skipped, 0 failed across 431 files; exit 0; 1006.9sbun run typecheck: exit 0bun run privacy:scan: exit 0git diff origin/dev...HEAD --check: exit 0Existing non-blocking warnings remain: large production chunks, React test act warnings, and the documentation entry-to-404 notice.
Summary by CodeRabbit
New Features
ocx uninstallcommand.Security & Reliability
Documentation