Harden packaged runtime startup#299
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 adds device-use runtime command support, centralizes PACKAGED_SYSTEM_PATHS, switches backend dynamic imports to createRequire for ESM compatibility, adds AAP prefetch entrypoint validation and launch command resolution for device-use, makes device-use build/staging idempotent with native helper handling, creates shared device-use payload helpers, and extends packaged/source runtime smoke tests. ChangesDevice-Use Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…link-poll-cancel # Conflicts: # apps/backend/src/lib/sqlite.ts # apps/backend/src/services/agent/commands.ts # apps/backend/test/unit/runtime/agent-process.test.ts # package.json
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/runtime/smoke-packaged-runtime.cjs (1)
99-105: 💤 Low valueShell injection vector in
isCliAvailable.While
JSON.stringifyescapes quotes, it doesn't prevent all shell metacharacters. Ifcommandcontains characters like$(...)or backticks, they could still be interpreted. Since this is an internal smoke test and the only caller passes the literal string"xcrun", this is low risk but worth tightening.🛡️ Safer alternative using direct exec
function isCliAvailable(command) { - const result = spawnSync("sh", ["-c", `command -v ${JSON.stringify(command)}`], { + const result = spawnSync("command", ["-v", command], { + shell: true, stdio: "ignore", timeout: 2_000, }); return result.status === 0; }Or use
whichdirectly:function isCliAvailable(command) { - const result = spawnSync("sh", ["-c", `command -v ${JSON.stringify(command)}`], { + const result = spawnSync("which", [command], { stdio: "ignore", timeout: 2_000, }); return result.status === 0; }🤖 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/runtime/smoke-packaged-runtime.cjs` around lines 99 - 105, The isCliAvailable function uses spawnSync with a shell string that can interpret metacharacters, creating a shell injection risk; fix it by avoiding the shell: call the executable directly (e.g., use spawnSync("which", [command], {...}) or spawnSync with an exec-file style argument array) or, if you must use a shellless builtin, validate/sanitize command against a strict whitelist/regex (e.g., only allow [A-Za-z0-9._-]) before invoking; update isCliAvailable to use the direct exec call or whitelist check so no user-controlled data is interpolated into a shell command.scripts/runtime/agent-clis.ts (1)
632-636: ⚡ Quick winValidate
runtimeKeysinput to avoid silent no-op staging.If callers pass an unknown key, this currently skips all targets and still reports success. Add an upfront validation against
AGENT_CLI_TARGETSand throw on unsupported keys.Proposed patch
const manifestTargets: StagedAgentCli[] = []; const runtimeKeys = options.runtimeKeys ? new Set(options.runtimeKeys) : null; + if (runtimeKeys) { + const supported = new Set(AGENT_CLI_TARGETS.map((t) => t.runtimeKey)); + const invalid = [...runtimeKeys].filter((key) => !supported.has(key)); + if (invalid.length > 0) { + throw new Error(`Unsupported agent CLI runtime key(s): ${invalid.join(", ")}`); + } + }🤖 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/runtime/agent-clis.ts` around lines 632 - 636, Validate the provided runtime keys before the loop: check options.runtimeKeys (and the derived runtimeKeys Set) against the known AGENT_CLI_TARGETS list and throw an error if any unknown key is present so we don't silently skip all targets; implement this by computing the set of allowed runtime keys from AGENT_CLI_TARGETS, finding any difference with options.runtimeKeys, and throwing a descriptive exception that names the unsupported keys (referencing options.runtimeKeys, runtimeKeys, and AGENT_CLI_TARGETS to locate the change).
🤖 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.
Nitpick comments:
In `@scripts/runtime/agent-clis.ts`:
- Around line 632-636: Validate the provided runtime keys before the loop: check
options.runtimeKeys (and the derived runtimeKeys Set) against the known
AGENT_CLI_TARGETS list and throw an error if any unknown key is present so we
don't silently skip all targets; implement this by computing the set of allowed
runtime keys from AGENT_CLI_TARGETS, finding any difference with
options.runtimeKeys, and throwing a descriptive exception that names the
unsupported keys (referencing options.runtimeKeys, runtimeKeys, and
AGENT_CLI_TARGETS to locate the change).
In `@scripts/runtime/smoke-packaged-runtime.cjs`:
- Around line 99-105: The isCliAvailable function uses spawnSync with a shell
string that can interpret metacharacters, creating a shell injection risk; fix
it by avoiding the shell: call the executable directly (e.g., use
spawnSync("which", [command], {...}) or spawnSync with an exec-file style
argument array) or, if you must use a shellless builtin, validate/sanitize
command against a strict whitelist/regex (e.g., only allow [A-Za-z0-9._-])
before invoking; update isCliAvailable to use the direct exec call or whitelist
check so no user-controlled data is interpolated into a shell command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91b9c539-9353-433f-a8c6-b370f736fdb9
📒 Files selected for processing (30)
apps/backend/src/lib/sqlite.tsapps/backend/src/services/aap/apps.service.tsapps/backend/src/services/aap/lifecycle.tsapps/backend/src/services/agent/commands.tsapps/backend/src/services/pty.service.tsapps/backend/test/integration/aap.test.tsapps/backend/test/unit/runtime/agent-process.test.tsapps/backend/test/unit/services/aap/lifecycle.test.tsapps/desktop/main/backend-process.tsapps/desktop/main/cli-tools.tsapps/desktop/main/runtime-env.tsapps/runtime/index.tsdocs/runtime-agent-cli-ownership.mdelectron-builder.ymlpackage.jsonpackages/device-use/native/Sources/SimInspector/build.shpackages/device-use/scripts/build-ts.tspackages/device-use/src/cli/commands/serve.tsscripts/prepare-device-use.mjsscripts/runtime/agent-clis.tsscripts/runtime/dev.tsscripts/runtime/electron-builder-before-pack.cjsscripts/runtime/lib/device-use-payloads.cjsscripts/runtime/prepare-dev-agent-clis.tsscripts/runtime/smoke-packaged-app.cjsscripts/runtime/smoke-packaged-runtime.cjsscripts/runtime/smoke-source-runtime.cjsshared/lib/cli-path.tsshared/runtime.tstest/unit/runtime/smoke-packaged-runtime.test.ts
![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: b2724d695c
ℹ️ 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 rawCwd = prefetch.cwd ? substituteTemplate(prefetch.cwd, vars) : packageRoot; | ||
| const cwd = isAbsolute(rawCwd) ? rawCwd : resolvePath(packageRoot, rawCwd); | ||
| const command = resolveCommand(prefetch.command, packageRoot); | ||
| const args = substituteArgs(prefetch.args, vars); |
There was a problem hiding this comment.
runPrefetch now calls substituteArgs before checking canSpawnResolvedCommand, so a manifest with templated prefetch args (for example {workspace}) will throw immediately when vars is empty even in the “command unavailable” path that is supposed to be skipped. Because prefetchInstalledAppAssets() fire-and-forgets runPrefetch, this rejection is unhandled and can crash startup under Node’s default unhandled-rejection behavior instead of logging a benign skip.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d744949: prefetch now checks whether the resolved command is spawnable before expanding cwd/args/env templates, so unavailable optional prefetch commands skip cleanly instead of rejecting on empty boot-time vars. Added integration coverage for a missing prefetch command with templated cwd/args.
| if (!firstArg.includes("/") && !firstArg.includes("\\")) return null; | ||
|
|
||
| const entrypoint = isAbsolute(firstArg) ? firstArg : resolvePath(cwd, firstArg); | ||
| return existsSync(entrypoint) ? null : entrypoint; |
There was a problem hiding this comment.
The new missing-entrypoint guard treats any first arg containing / or \ as a filesystem path, so valid commands like curl https://... (or any tool whose first operand is a URI) are incorrectly resolved against cwd and skipped as “entrypoint unavailable.” This silently disables legitimate prefetch commands for manifests that use URL-style operands.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d744949: the prefetch entrypoint guard now ignores URI-like first operands such as https://... and file:..., so valid URL operands are not treated as missing filesystem paths. Added an integration case that runs a prefetch command with an https:// first arg.
| "postinstall": "bun run prepare:device-use", | ||
| "native:electron": "electron-builder install-app-deps", | ||
| "native:node": "cd node_modules/better-sqlite3 && node ../node-gyp/bin/node-gyp.js rebuild", | ||
| "native:node": "cd node_modules/better-sqlite3 && PYTHON=${PYTHON:-/usr/bin/python3} node ../node-gyp/bin/node-gyp.js rebuild", |
There was a problem hiding this comment.
Setting PYTHON=${PYTHON:-/usr/bin/python3} hardcodes a fallback path that does not exist on some supported developer/CI setups where Python is installed elsewhere (for example via non-system toolchains). In those environments node-gyp fails immediately, breaking bun run native:node (and commands that depend on it, like dev:web and backend tests) even though a usable python3 is on PATH.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d744949: native:node now uses scripts/runtime/rebuild-node-native.cjs, which validates available Python candidates instead of forcing PYTHON=/usr/bin/python3. It still handles local broken Python installs by selecting a working candidate, and falls back to node-gyp discovery if none is validated.
Greenlight — Round 2Fixed 3 review comments in d744949:
Validation: bun run native:node, focused AAP tests, bun run typecheck, bun run test:backend. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d744949f56
ℹ️ 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".
| if (!firstArg.includes("/") && !firstArg.includes("\\")) return null; | ||
|
|
||
| const entrypoint = isAbsolute(firstArg) ? firstArg : resolvePath(cwd, firstArg); | ||
| return existsSync(entrypoint) ? null : entrypoint; |
There was a problem hiding this comment.
Treating any first arg containing / or \\ as a filesystem entrypoint now breaks valid prefetch commands like bunx @scope/tool or npx @scope/tool, because scoped package names include / but are not paths. In those cases this guard resolves @scope/tool against cwd, marks it missing, and skips prefetch with entrypoint unavailable, so legitimate third-party prefetch flows never run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e70ab8f. The prefetch missing-entrypoint guard now only treats explicit filesystem entrypoints as paths (absolute paths, ./, ../, Windows drive paths), so scoped package arguments like @scope/tool are passed through. Added integration coverage for a scoped package-style prefetch argument.
Greenlight updateFixed the latest review round in e70ab8f:
Validated locally with typecheck, targeted backend AAP tests, runtime rebuild, |
* Harden packaged runtime startup * Fix device-use runtime CLI import * Address device-use packaging review * Address AAP prefetch review * Tighten AAP runtime packaging guards * chore: organize runtime smoke scripts * chore: document runtime package flow
1 participant
Harden packaged runtime startup by routing packaged backend, AAP, and device-use children through bundled runtime env, fixing ESM native module loading via createRequire, and keeping dev agent CLI staging from rewriting the full packaged manifest.
Bundle and verify device-use simulator helpers plus Mobile Use payloads in macOS artifacts, including universal helper checks and packaged runtime smokes that exercise backend commands, agent discovery, and device-use serve.
Document runtime/agent CLI ownership boundaries and add focused tests for AAP launch routing and packaged runtime smoke invariants.
Verification: git diff --check; pre-commit ran bun install, eslint --fix, and prettier --write.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores