fix(sandbox): add network-restricted sandbox hints#71
Conversation
Sandboxed network failures were surfacing as generic request errors, which made it unclear that the user should retry with elevated permissions. Detect socket-open and connection-refused sandbox errors in the shared request path, append localized guidance, and update affected auth, connector, file, package, and skills flows with coverage. Signed-off-by: Kevin Cui <bh@bugs.cc>
Summary by CodeRabbitRelease Notes
WalkthroughThis PR adds i18n translator support and network-error detection throughout the CLI. It threads a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/application/commands/auth/index.cli.test.ts (1)
6-17: 🛠️ Refactor suggestion | 🟠 MajorReuse
writeAuthFilefor these auth-status sandbox tests and remove unnecessaryasyncfrom throwing fetchers.Both new tests duplicate the same auth TOML setup. The shared helper
writeAuthFilealready creates this default account (user-1 / Alice / secret-1), so using it avoids drift and makes tests focus on network behavior. The throwing fetchers can also be synchronous since they contain noawait.Proposed changes
import { createCliSandbox, createCliSnapshot, createConnectionRefusedError, createFailedToOpenSocketError, defaultAuthEndpoint, findLoginUrl, readAuthLoginUrlPrefix, readLatestLogContent, runPrintedAuthLogin, toRequest, + writeAuthFile, } from "../../../../__tests__/helpers.ts";In the first test (around line 376):
- const authFilePath = join( - sandbox.env.XDG_CONFIG_HOME!, - APP_NAME, - "auth.toml", - ); - - await Bun.write( - authFilePath, - [ - "id = \"user-1\"", - "", - "[[auth]]", - "id = \"user-1\"", - "name = \"Alice\"", - "api_key = \"secret-1\"", - "endpoint = \"oomol.com\"", - "", - ].join("\n"), - ); + await writeAuthFile(sandbox); const result = await sandbox.run( ["auth", "status"], { - fetcher: async () => { + fetcher: () => { throw createFailedToOpenSocketError("network down"); }, },Apply the same replacement in the connection-refused test.
Per coding guidelines: "Extract repeated setup (mocks, stubs, setup objects) into local factory functions in test files" and "Never mark a function
asyncif it contains noawait".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/auth/index.cli.test.ts` around lines 6 - 17, Replace the duplicated auth TOML setup in the auth-status sandbox tests by calling the existing helper writeAuthFile (which creates the default account user-1/Alice/secret-1) instead of re-writing the file in-test; locate the two tests that currently use createCliSandbox/createCliSnapshot with inline auth content and swap that setup to invoke writeAuthFile before creating the sandbox, and also remove the unnecessary async keyword from the throwing fetcher functions (they contain no await) so the throwing fetchers used in the connection-refused and other network-behavior tests are plain synchronous functions.
🧹 Nitpick comments (4)
src/application/commands/shared/request.test.ts (1)
182-262: Consolidate the duplicated sandbox-hint tests using a table-driven approach. Both tests repeat the same request context and error constructors with only the error factory and expected message differing. Extract this common setup and use a loop to eliminate duplication, removing the unnecessaryasynckeywords from the fetchers since they only throw and never await.Example refactor
- test("adds a sandbox network hint when the fetcher cannot open a socket", async () => { - const logCapture = createLogCapture(); - - try { - await expect(executeCliRequest({ - context: { - fetcher: async () => { - throw createFailedToOpenSocketError("network down"); - }, - logger: logCapture.logger, - translator: createTranslator("zh"), - }, - createRequestError: error => new CliUserError( - "errors.shared.requestError", - 1, - { - message: error instanceof Error ? error.message : String(error), - }, - ), - createRequestFailedError: status => new CliUserError( - "errors.shared.requestFailed", - 1, - { - status, - }, - ), - label: "Shared", - requestUrl: new URL("https://example.com/items/1"), - })).rejects.toMatchObject({ - key: "errors.shared.requestError", - params: { - message: - "network down\n当前环境可能在网络受限的沙箱中，请尝试提权。", - }, - }); - } - finally { - logCapture.close(); - } - }); - - test("adds a sandbox network hint when the fetcher connection is refused", async () => { + for (const testCase of [ + { + title: "adds a sandbox network hint when the fetcher cannot open a socket", + createError: () => createFailedToOpenSocketError("network down"), + expectedMessage: "network down\n当前环境可能在网络受限的沙箱中，请尝试提权。", + }, + { + title: "adds a sandbox network hint when the fetcher connection is refused", + createError: () => createConnectionRefusedError("connection refused"), + expectedMessage: "connection refused\n当前环境可能在网络受限的沙箱中，请尝试提权。", + }, + ] as const) { + test(testCase.title, async () => { const logCapture = createLogCapture(); try { await expect(executeCliRequest({ context: { - fetcher: async () => { - throw createConnectionRefusedError("connection refused"); + fetcher: () => { + throw testCase.createError(); }, logger: logCapture.logger, translator: createTranslator("zh"), @@ key: "errors.shared.requestError", params: { - message: - "connection refused\n当前环境可能在网络受限的沙箱中，请尝试提权。", + message: testCase.expectedMessage, }, }); } finally { logCapture.close(); } - }); + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/shared/request.test.ts` around lines 182 - 262, Tests duplicate nearly identical cases for sandbox hint behavior; consolidate them into a table-driven loop that iterates over error factories and expected messages. Replace the two separate tests with a single test that builds a cases array (e.g. entries pairing createFailedToOpenSocketError and "network down\n当前环境可能在网络受限的沙箱中，请尝试提权。" and createConnectionRefusedError and "connection refused\n当前环境可能在网络受限的沙箱中，请尝试提权。"), and for each case call executeCliRequest with the same context (use createLogCapture(), createTranslator("zh"), same CliUserError factories, requestUrl) but set fetcher to a non-async throwing function created from the case's error factory; assert rejects.toMatchObject as before and ensure logCapture.close() runs after the loop.src/application/auth/login-flow.test.ts (1)
174-196: Hardcoded localized string couples this test to the i18n catalog.The assertion embeds the full zh translation (
当前环境可能在网络受限的沙箱中，请尝试提权。). Any wording tweak in the catalog will break this unrelated auth test. Consider asserting via the same translator instance (e.g., build the expected suffix fromtranslator.t("…sandboxHint…")) or usingexpect.stringContaining("network down")plus a dedicated test in the shared request module that owns the hint text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/auth/login-flow.test.ts` around lines 174 - 196, The test is hardcoding the full Chinese sandbox hint which couples it to the i18n catalog; change the assertion to derive the expected localized suffix from the same translator instance (created via createTranslator("zh")) or use partial assertions: either build the expected params.message by concatenating the error message with translator.t("sandboxHint") and compare that object from startAuthLoginSession, or replace the strict message check with expect.stringContaining("network down") plus expect.stringContaining(translator.t("sandboxHint")). Update references in the failing test that call startAuthLoginSession, createTranslator, and createFailedToOpenSocketError accordingly.src/application/commands/connector/shared.test.ts (1)
208-229: Duplicated hardcoded hint string across tests.The en sandbox hint (
"Current environment may be running in a network-restricted sandbox. Try requesting elevated permissions.") is embedded here and the zh variant is embedded insrc/application/auth/login-flow.test.ts. Each consumer ofgetUnexpectedRequestErrorMessagere-asserts the same copy, so a catalog wording change cascades across unrelated test files. Prefer asserting the behavior via the translator (resolve the key at test time) and keep one exhaustive text-level test in the shared request module tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/connector/shared.test.ts` around lines 208 - 229, The test hardcodes the English sandbox hint; instead resolve the translation at test time so wording changes don’t break unrelated tests—update the assertion in the test using runConnectorAction and createRequestContext to call the app translator (the same translation key used by getUnexpectedRequestErrorMessage) and compare error.params.message to the concatenation of the original error message and the translator-resolved sandbox hint, rather than embedding the literal English string.src/application/commands/connector/shared.ts (1)
97-103: Optional: consider usinggetUnexpectedRequestErrorMessageconsistently.The
createUnexpectedErrorcallbacks insearchConnectorActions,listAuthenticatedConnectorServices, andgetConnectorActionMetadatastill inlineerror instanceof Error ? error.message : String(error). This currently works only becauseperformLoggedRequestpre-enhances the error with the sandbox hint. Routing all three throughgetUnexpectedRequestErrorMessage(error, context.translator)(asrunConnectorActiondoes) would make the hint behavior explicit at each call site and remove a silent dependency on upstream wrapping — matching the intent called out in the coding guideline to extract shared utilities.Also applies to: 158-164, 214-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/connector/shared.ts` around lines 97 - 103, The createUnexpectedError lambda currently constructs the error message inline; update the three sites (searchConnectorActions, listAuthenticatedConnectorServices, getConnectorActionMetadata) to call getUnexpectedRequestErrorMessage(error, context.translator) when building the CliUserError so the sandbox hint behavior is explicit and consistent with runConnectorAction; leave the CliUserError construction (message key and metadata shape) intact but replace the message expression with the shared helper, and confirm performLoggedRequest still supplies the original error to preserve existing context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/application/commands/skills/search.cli.test.ts`:
- Around line 251-275: The fetcher passed into sandbox.run in the test "adds a
sandbox hint when the skills search request cannot open a socket" is marked
async but only performs a synchronous throw (createFailedToOpenSocketError), so
remove the unnecessary async keyword from the fetcher callback (change fetcher:
async () => { throw ... } to fetcher: () => { throw ... }) in that test to
satisfy the "no async without await" guideline while keeping its behavior
unchanged.
---
Outside diff comments:
In `@src/application/commands/auth/index.cli.test.ts`:
- Around line 6-17: Replace the duplicated auth TOML setup in the auth-status
sandbox tests by calling the existing helper writeAuthFile (which creates the
default account user-1/Alice/secret-1) instead of re-writing the file in-test;
locate the two tests that currently use createCliSandbox/createCliSnapshot with
inline auth content and swap that setup to invoke writeAuthFile before creating
the sandbox, and also remove the unnecessary async keyword from the throwing
fetcher functions (they contain no await) so the throwing fetchers used in the
connection-refused and other network-behavior tests are plain synchronous
functions.
---
Nitpick comments:
In `@src/application/auth/login-flow.test.ts`:
- Around line 174-196: The test is hardcoding the full Chinese sandbox hint
which couples it to the i18n catalog; change the assertion to derive the
expected localized suffix from the same translator instance (created via
createTranslator("zh")) or use partial assertions: either build the expected
params.message by concatenating the error message with
translator.t("sandboxHint") and compare that object from startAuthLoginSession,
or replace the strict message check with expect.stringContaining("network down")
plus expect.stringContaining(translator.t("sandboxHint")). Update references in
the failing test that call startAuthLoginSession, createTranslator, and
createFailedToOpenSocketError accordingly.
In `@src/application/commands/connector/shared.test.ts`:
- Around line 208-229: The test hardcodes the English sandbox hint; instead
resolve the translation at test time so wording changes don’t break unrelated
tests—update the assertion in the test using runConnectorAction and
createRequestContext to call the app translator (the same translation key used
by getUnexpectedRequestErrorMessage) and compare error.params.message to the
concatenation of the original error message and the translator-resolved sandbox
hint, rather than embedding the literal English string.
In `@src/application/commands/connector/shared.ts`:
- Around line 97-103: The createUnexpectedError lambda currently constructs the
error message inline; update the three sites (searchConnectorActions,
listAuthenticatedConnectorServices, getConnectorActionMetadata) to call
getUnexpectedRequestErrorMessage(error, context.translator) when building the
CliUserError so the sandbox hint behavior is explicit and consistent with
runConnectorAction; leave the CliUserError construction (message key and
metadata shape) intact but replace the message expression with the shared
helper, and confirm performLoggedRequest still supplies the original error to
preserve existing context.
In `@src/application/commands/shared/request.test.ts`:
- Around line 182-262: Tests duplicate nearly identical cases for sandbox hint
behavior; consolidate them into a table-driven loop that iterates over error
factories and expected messages. Replace the two separate tests with a single
test that builds a cases array (e.g. entries pairing
createFailedToOpenSocketError and "network down\n当前环境可能在网络受限的沙箱中，请尝试提权。" and
createConnectionRefusedError and "connection refused\n当前环境可能在网络受限的沙箱中，请尝试提权。"),
and for each case call executeCliRequest with the same context (use
createLogCapture(), createTranslator("zh"), same CliUserError factories,
requestUrl) but set fetcher to a non-async throwing function created from the
case's error factory; assert rejects.toMatchObject as before and ensure
logCapture.close() runs after the loop.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e690cd2-7867-4d18-9982-e4d5cffed3a8
📒 Files selected for processing (28)
__tests__/helpers.tscontrib/skills/codex/oo/SKILL.mdsrc/application/auth/login-flow.test.tssrc/application/auth/login-flow.tssrc/application/commands/auth/index.cli.test.tssrc/application/commands/auth/login.tssrc/application/commands/auth/status.tssrc/application/commands/cloud-task/shared.tssrc/application/commands/connector/schema-cache.test.tssrc/application/commands/connector/schema-cache.tssrc/application/commands/connector/search-provider.tssrc/application/commands/connector/shared.test.tssrc/application/commands/connector/shared.tssrc/application/commands/file/download/plan.tssrc/application/commands/file/download/request.test.tssrc/application/commands/file/download/request.tssrc/application/commands/file/shared.test.tssrc/application/commands/file/shared.tssrc/application/commands/package/search-provider.tssrc/application/commands/package/shared.test.tssrc/application/commands/package/shared.tssrc/application/commands/shared/request.test.tssrc/application/commands/shared/request.tssrc/application/commands/skills/registry-skill-source.test.tssrc/application/commands/skills/registry-skill-source.tssrc/application/commands/skills/search.cli.test.tssrc/application/commands/skills/search.tssrc/i18n/catalog.ts
| test("adds a sandbox hint when the skills search request cannot open a socket", async () => { | ||
| const sandbox = await createCliSandbox(); | ||
|
|
||
| try { | ||
| await writeAuthFile(sandbox); | ||
|
|
||
| const result = await sandbox.run( | ||
| ["skills", "search", "text generation"], | ||
| { | ||
| fetcher: async () => { | ||
| throw createFailedToOpenSocketError("network down"); | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| expect(result.exitCode).toBe(1); | ||
| expect(result.stdout).toBe(""); | ||
| expect(result.stderr).toBe( | ||
| "The skills search request failed: network down\nCurrent environment may be running in a network-restricted sandbox. Try requesting elevated permissions.\n", | ||
| ); | ||
| } | ||
| finally { | ||
| await sandbox.cleanup(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the changed skills-search sandbox test no longer contains an async callback with no await.
awk 'NR>=251 && NR<=275 { print NR ":" $0 }' src/application/commands/skills/search.cli.test.ts | sed -n '/fetcher: async/,+4p'Repository: oomol-lab/oo-cli
Length of output: 256
Remove async from the throwing test fetcher—it has no await.
Line 260 marks the fetcher callback as async but contains only a synchronous throw with no await, violating the coding guideline that states: "Never mark a function async if it contains no await; remove async when all code paths are synchronous."
Change
- fetcher: async () => {
+ fetcher: () => {
throw createFailedToOpenSocketError("network down");
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/application/commands/skills/search.cli.test.ts` around lines 251 - 275,
The fetcher passed into sandbox.run in the test "adds a sandbox hint when the
skills search request cannot open a socket" is marked async but only performs a
synchronous throw (createFailedToOpenSocketError), so remove the unnecessary
async keyword from the fetcher callback (change fetcher: async () => { throw ...
} to fetcher: () => { throw ... }) in that test to satisfy the "no async without
await" guideline while keeping its behavior unchanged.
1 participant
Sandboxed network failures were surfacing as generic request errors, which made it unclear that the user should retry with elevated permissions.
Detect socket-open and connection-refused sandbox errors in the shared request path, append localized guidance, and update affected auth, connector, file, package, and skills flows with coverage.