Bundle native packaged runtime#287
Merged
Merged
Conversation
Preserve Codex SDK reasoning output tokens when accumulating usage, update adapter mocks, and make the screen-studio default background satisfy the tuple type. Verification: bun run typecheck; bun run typecheck:agent-server.
Add the Bun-compiled deus-runtime entrypoint, native runtime manifest validation, locked darwin codex/claude/rg staging, GitHub CLI manifesting, and deterministic bundled CLI path helpers. Verification: bun run build:runtime; bun run validate:runtime; bun run prepare:agent-clis; bun run prepare:gh-cli; bun run typecheck.
Make packaged Electron start backend with Resources/bin/deus-runtime, make the backend start agent-server via the same runtime, strip leaked Electron-as-Node/dev env, and use runtime resource paths outside Electron. Verification: bun run build:runtime; bun run validate:runtime; bun run typecheck; bun run typecheck:backend; bun run typecheck:agent-server; source runtime self-test/agent-server/backend smokes.
Remove packaged shell/global CLI discovery, require explicit executable overrides or bundled codex/claude binaries, skip login-shell env in runtime mode, and keep packaged desktop CLI probes on Resources/bin plus system paths. Verification: bun run typecheck; bun run typecheck:agent-server; bun run typecheck:backend; source runtime backend smoke confirmed agents initialize without global CLI fallback.
Copy deus-runtime and native CLIs into mac Resources/bin, fail packaging on stale Electron/runtime outputs, and verify packaged executables, signatures, entitlements, manifest hashes, and dylib dependencies. Verification: bun run validate:runtime; node --check scripts/prune-pencil-cli-binaries.cjs; node --check scripts/runtime/electron-builder-before-pack.cjs; fake packaged Resources/bin verifier smoke.
Record the shortened runtime goal prompt plus Conductor, OpenCode, and T3Code references used for this branch. Verification: documentation-only.
Packaged Electron now starts the backend through Resources/bin/deus-runtime, so it should not export NODE_PATH for the old Electron-as-Node backend module path. The packaged spawn test now asserts NODE_PATH is absent along with ELECTRON_RUN_AS_NODE and agent-server CJS entry env vars.\n\nVerification:\n- bun run typecheck\n- bun run typecheck:backend\n- bun run typecheck:agent-server\n- git diff --check
Bun-compiled deus-runtime cannot rely on Electron's app.asar module resolver for externalized native packages. Unpack @napi-rs/canvas alongside better-sqlite3 and node-pty, and make the packaged resource verifier fail if those external module package roots are missing from app.asar.unpacked.\n\nVerification:\n- node --check scripts/prune-pencil-cli-binaries.cjs\n- bun run typecheck\n- bun run typecheck:backend\n- bun run typecheck:agent-server\n- direct node fixture for verifyPackagedRuntimeExternalModules\n- git diff --check
The Bun-compiled deus-runtime is not an Electron process, so it should not search Resources/app.asar/node_modules for externalized native modules. Keep NODE_PATH pointed at app.asar.unpacked/node_modules and staged dev node_modules instead, and expose the resolved NODE_PATH in self-test output for inspection.\n\nVerification:\n- bun run typecheck\n- bun run typecheck:backend\n- bun run typecheck:agent-server\n- bun run build:runtime\n- bun run validate:runtime\n- bun apps/runtime/index.ts self-test\n- git diff --check
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@apps/backend/test/unit/runtime/agent-process.test.ts`:
- Around line 61-79: The env fixture contains an extra blank line because
envFormat both includes a trailing empty string in the array and then appends "+
\"\\n\""; remove the redundant newline by either deleting the trailing empty
array element or removing the final "+ \"\\n\"" concatenation so that envFormat
ends with a single newline; update the construction of envFormat (the constant
named envFormat in the test) so the generated string exactly matches the
expected fixture.
In `@scripts/prune-pencil-cli-binaries.cjs`:
- Around line 717-779: The promise currently resolves/rejects on
child.on("exit") while stdout/stderr may still be draining, causing flaky
validateVersionOutput; change logic to wait for the child's "close" event (which
guarantees stdio has finished) before resolving/rejecting: keep the same
timeout, fail helper, and diagnostics (packagedExecutableDiagnostics,
macExecutionPolicyHint, PACKAGED_VERSION_TIMEOUT_MS, stdout/stderr) but move the
success/failure resolution to child.on("close") or track exit code and only call
resolve()/fail() after receiving "close" (ensuring you still handle "error" as
before); ensure you don't double-settle settled and clearTimeout(timeout) on
close.
- Around line 230-247: The three synchronous spawnSync calls (the
prebuild-install probe that invokes process.execPath with prebuildInstall, and
the two codesign probes later in this file) must be made timeout-bounded and
must check result.error like runDiagnostic() does: add a timeout (e.g. 20_000)
to each spawnSync options, build the combined output from
result.stdout/result.stderr, then if (result.error) return or throw a
descriptive error using result.error.code || result.error.message plus the
output, and finally check result.status !== 0 and handle non-zero exits by
returning the output or a message like `${command} exited with status
${result.status}`; update the three call sites (the spawnSync for
prebuildInstall, and the two codesign spawnSync calls) to follow this exact
pattern.
- Around line 698-705: The current denylist (PACKAGED_VERSION_ENV_DENYLIST)
leaves many sensitive env vars available to spawned CLIs; replace it with an
explicit allowlist when building env: create a fresh env object that only
includes the minimal safe keys (e.g., NODE_ENV, TZ, LANG if needed) plus
DEUS_BUNDLED_BIN_DIR (set to binDir), PATH (set to [binDir,
...PACKAGED_SYSTEM_PATHS].join(...)), and optionally a temporary HOME instead of
inheriting the real one; when including any allowed key copy it from process.env
if present, do not spread process.env or delete keys from it, and remove use of
PACKAGED_VERSION_ENV_DENYLIST in this block so only allowlisted variables are
passed to the spawned --version checks.
In `@test/unit/runtime/validate-runtime.test.ts`:
- Around line 150-152: The test currently only uses toHaveBeenCalledWith on
validateDeusRuntimeMock which can hide multiple calls; add an explicit
call-count assertion (e.g.,
expect(validateDeusRuntimeMock).toHaveBeenCalledTimes(1)) immediately before or
after the existing toHaveBeenCalledWith in both tests that reference
validateDeusRuntimeMock (the assertions around projectRoot/verifyRunnable flags
at the blocks corresponding to lines with expect.objectContaining({ projectRoot,
verifyRunnable: false }) and the similar block at lines 164-166) so the
validator is asserted to be invoked exactly once and prevent false-positive
passes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4ca2a86-2b3a-446d-a909-4be5c0c45ad4
📒 Files selected for processing (8)
apps/agent-server/build.tsapps/agent-server/test/agent-browser-client.test.tsapps/backend/src/lib/sqlite.tsapps/backend/test/unit/runtime/agent-process.test.tsapps/backend/test/unit/runtime/child-env.test.tsapps/runtime/index.tsscripts/prune-pencil-cli-binaries.cjstest/unit/runtime/validate-runtime.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/backend/test/unit/runtime/child-env.test.ts
- apps/agent-server/test/agent-browser-client.test.ts
- apps/backend/src/lib/sqlite.ts
- apps/runtime/index.ts
Owner
Author
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/unit/runtime/validate-runtime.test.ts (1)
142-154: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMissing assertion for
validateStagedAgentClisarguments.The test asserts
validateStagedAgentClisMockwas called once but doesn't verify the arguments passed. If the implementation accidentally passes the wrongprojectRootor omits required options, the test won't catch it.🔍 Proposed assertion
expect(validateDeusRuntimeMock).toHaveBeenCalledWith( expect.objectContaining({ projectRoot, verifyRunnable: false }) ); + expect(validateDeusRuntimeMock).toHaveBeenCalledTimes(1); expect(validateStagedAgentClisMock).toHaveBeenCalledOnce(); + expect(validateStagedAgentClisMock).toHaveBeenCalledWith( + expect.objectContaining({ projectRoot }) + ); });🤖 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 `@test/unit/runtime/validate-runtime.test.ts` around lines 142 - 154, The test currently only checks that validateStagedAgentClisMock was called once but not what it was called with; update the test to assert the call received the correct arguments by adding an expectation that validateStagedAgentClisMock was called with an object containing the same projectRoot (and any expected options like log or other flags) — e.g., use expect(validateStagedAgentClisMock).toHaveBeenCalledWith(expect.objectContaining({ projectRoot })) to ensure validateRuntimeStage passes the correct parameters to validateStagedAgentClis.
🟡 Minor comments (14)
test/unit/desktop/cli-tools.test.ts-17-51 (1)
17-51:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore
DEUS_RESOURCES_PATHin test cleanup to prevent cross-test env leakage.
configurePackagedMainRuntimeEnv(...)can setDEUS_RESOURCES_PATH, butafterEachdoes not restore/delete it. This can make later tests order-dependent.Suggested patch
const originalBundledBinDir = process.env.DEUS_BUNDLED_BIN_DIR; +const originalDeusResourcesPath = process.env.DEUS_RESOURCES_PATH; const originalDeusPackaged = process.env.DEUS_PACKAGED; const originalDeusRuntime = process.env.DEUS_RUNTIME; const originalDeusRuntimeExecutable = process.env.DEUS_RUNTIME_EXECUTABLE; @@ afterEach(() => { mockSyncShellEnvironment.mockClear(); if (originalBundledBinDir === undefined) delete process.env.DEUS_BUNDLED_BIN_DIR; else process.env.DEUS_BUNDLED_BIN_DIR = originalBundledBinDir; + if (originalDeusResourcesPath === undefined) delete process.env.DEUS_RESOURCES_PATH; + else process.env.DEUS_RESOURCES_PATH = originalDeusResourcesPath; if (originalDeusPackaged === undefined) delete process.env.DEUS_PACKAGED; else process.env.DEUS_PACKAGED = originalDeusPackaged;🤖 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 `@test/unit/desktop/cli-tools.test.ts` around lines 17 - 51, The afterEach cleanup in the test file fails to restore or delete DEUS_RESOURCES_PATH which configurePackagedMainRuntimeEnv(...) may set, causing cross-test environment leakage; update the afterEach block (the cleanup function containing mockSyncShellEnvironment.mockClear and the environment restores) to mirror the pattern used for other env vars by checking originalDeusResourcesPath (capture its initial value at top like originalBundledBinDir) and then either delete process.env.DEUS_RESOURCES_PATH if it was undefined or restore it to originalDeusResourcesPath, ensuring DEUS_RESOURCES_PATH is properly reset between tests.docs/deus-runtime-verification.md-21-23 (1)
21-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the packaged app path example to match both architectures.
Line 22 hardcodes
mac-arm64while Line 21 documents<arm64|x64>, which can mislead x64 verification runs.📘 Proposed doc fix
-bun run package:mac:dir -- --arch <arm64|x64> -node scripts/runtime/smoke-packaged-app.cjs --app dist-electron/mac-arm64/Deus.app +bun run package:mac:dir -- --arch <arm64|x64> +node scripts/runtime/smoke-packaged-app.cjs --app <path-to-Deus.app>🤖 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/deus-runtime-verification.md` around lines 21 - 23, The example hardcodes the packaged app path to dist-electron/mac-arm64/Deus.app while the packaging command documents <arm64|x64>; update the second command (the node smoke-packaged-app.cjs invocation) to accept the same architecture placeholder or show both architectures (e.g., dist-electron/mac-<arch>/Deus.app or provide two example lines for mac-arm64 and mac-x64) so the docs match the bun packaging command; change the path in the example that currently contains "dist-electron/mac-arm64/Deus.app" accordingly.scripts/runtime/package-mac-dir.cjs-89-101 (1)
89-101:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIcon resolution could fail silently with unclear error message.
If no candidates exist, the error message "Unable to resolve electron-builder icon input from: [args]" doesn't indicate whether the problem is missing
--root, missing--input, or missing files at the resolved paths.📋 Proposed improvement
+ const triedPaths = []; for (const candidate of candidates) { const absoluteCandidates = path.isAbsolute(candidate) ? [candidate] : roots.map((root) => path.resolve(PROJECT_ROOT, root, candidate)); for (const absolutePath of absoluteCandidates) { + triedPaths.push(absolutePath); if (fs.existsSync(absolutePath)) return absolutePath; } } - throw new Error(`Unable to resolve electron-builder icon input from: ${args.join(" ")}`); + throw new Error( + `Unable to resolve electron-builder icon input. Tried paths:\n${triedPaths.join("\n")}\nArgs: ${args.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/package-mac-dir.cjs` around lines 89 - 101, resolveIconInput currently throws a vague error when resolution fails; update resolveIconInput to detect and report which step failed: if roots is empty, include a message that no "--root" values were provided; if candidates is empty, say no "--input" or "--fallback-input" values were provided; otherwise, when files are not found, include the attempted absolute paths (from absoluteCandidates) and the original args in the thrown Error so it’s clear which resolved paths were checked; reference the resolveIconInput function and the roots, candidates, and absoluteCandidates variables when implementing the improved error messages.scripts/runtime/smoke-packaged-app.cjs-44-67 (1)
44-67:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winArgument parsing lacks protection against index overflow.
When
--appor--archflags appear at the end of argv without a following value,argv[++index]returnsundefined, which is silently assigned. This can lead to confusing error messages downstream instead of a clear "missing value" error at parse time.🛡️ Proposed fix to validate flag values
const arg = argv[index]; if (arg === "--app") { + if (index + 1 >= argv.length) throw new Error("--app requires a value"); options.appPath = argv[++index]; } else if (arg === "--arch") { + if (index + 1 >= argv.length) throw new Error("--arch requires a value"); options.arch = argv[++index];🤖 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-app.cjs` around lines 44 - 67, The argument parser loop that consumes argv (the for loop using argv and index) doesn't check for missing values when handling the "--app" and "--arch" flags; update the logic so before doing argv[++index] you validate that index + 1 < argv.length and throw a clear error like "Missing value for --app" or "Missing value for --arch" (and do the same pattern for any future flags that consume a value), only increment index after the check and assignment so undefined is never stored into options.appPath or options.arch.test/unit/runtime/validate-runtime.test.ts-127-131 (1)
127-131:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent mock reset strategy could hide test pollution.
validateDeusRuntimeMockandvalidateStagedAgentClisMockusemockReset(), which clears implementation and calls, whileexecFileSyncMockusesmockClear(), which only clears calls. If a test accidentally sets a different implementation onexecFileSyncMock, it will leak into subsequent tests.🔧 Proposed fix for consistency
beforeEach(() => { validateDeusRuntimeMock.mockReset(); validateStagedAgentClisMock.mockReset(); - execFileSyncMock.mockClear(); + execFileSyncMock.mockReset(); });🤖 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 `@test/unit/runtime/validate-runtime.test.ts` around lines 127 - 131, The beforeEach uses mockReset() for validateDeusRuntimeMock and validateStagedAgentClisMock but mockClear() for execFileSyncMock, risking leaked implementations between tests; change execFileSyncMock.mockClear() to execFileSyncMock.mockReset() inside the same beforeEach so all three mocks are consistently reset (clear calls and implementations) before each test, referring to validateDeusRuntimeMock, validateStagedAgentClisMock, execFileSyncMock and the beforeEach hook.apps/runtime/index.ts-202-207 (1)
202-207:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Module._initPaths()call may silently fail in Bun without error handling.Line 206 calls
moduleWithInitPaths._initPaths?.(), which is a private Node.js API. While the optional chaining prevents crashes if Bun doesn't support this API, the function will silently do nothing, potentially leaving the module cache out of sync with the updatedNODE_PATHset on line 319. This could cause modules to fail resolving from the updated paths without explicit error feedback.Either:
- Add explicit handling or logging if
_initPathsis unavailable- Document the expected behavior when running under Bun
- Add a test to verify module resolution works correctly after this call
🤖 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 `@apps/runtime/index.ts` around lines 202 - 207, The call in refreshNodePathResolution uses the private NodeModule._initPaths() which may be absent under Bun and currently fails silently; modify refreshNodePathResolution to detect whether NodeModule._initPaths exists and if not log a clear warning via the existing logger (or throw in CI), and add a fallback path: after checking moduleWithInitPaths._initPaths, if it's undefined call require('module').Module._resolveLookupPaths or re-initialize require.cache/resolution in a supported way, and add a unit/integration test that updates NODE_PATH (the change referenced at line ~319) and asserts that module resolution succeeds; reference the function refreshNodePathResolution, the NodeModule/_initPaths symbol, and the NODE_PATH update to locate where to add logging, fallback behavior, and the new test.test/unit/desktop/terminal-command.test.ts-11-41 (1)
11-41:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore
PATHandNODE_ENVin teardown.
configurePackagedMainRuntimeEnv()mutates both in the packaged-env test, but this suite only restores theDEUS_*variables. That leaks a packagedPATHandNODE_ENV="production"into later tests.🔧 Minimal fix
const originalBundledBinDir = process.env.DEUS_BUNDLED_BIN_DIR; const originalResourcesPath = process.env.DEUS_RESOURCES_PATH; const originalDeusPackaged = process.env.DEUS_PACKAGED; const originalDeusRuntime = process.env.DEUS_RUNTIME; +const originalNodeEnv = process.env.NODE_ENV; +const originalPath = process.env.PATH; const tempRoots: string[] = []; afterEach(() => { if (originalBundledBinDir === undefined) delete process.env.DEUS_BUNDLED_BIN_DIR; else process.env.DEUS_BUNDLED_BIN_DIR = originalBundledBinDir; @@ if (originalDeusPackaged === undefined) delete process.env.DEUS_PACKAGED; else process.env.DEUS_PACKAGED = originalDeusPackaged; if (originalDeusRuntime === undefined) delete process.env.DEUS_RUNTIME; else process.env.DEUS_RUNTIME = originalDeusRuntime; + if (originalNodeEnv === undefined) delete process.env.NODE_ENV; + else process.env.NODE_ENV = originalNodeEnv; + if (originalPath === undefined) delete process.env.PATH; + else process.env.PATH = originalPath; for (const root of tempRoots.splice(0)) { rmSync(root, { recursive: true, force: true }); } });🤖 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 `@test/unit/desktop/terminal-command.test.ts` around lines 11 - 41, The teardown currently restores only DEUS_* env vars but not PATH or NODE_ENV, causing leaks from tests that call configurePackagedMainRuntimeEnv(); capture the originals (e.g., const originalPath = process.env.PATH and const originalNodeEnv = process.env.NODE_ENV near the top alongside originalBundledBinDir) and in the afterEach block restore them (delete if undefined or reassign otherwise) so PATH and NODE_ENV are reset after each test; update references inside the same afterEach that handles originalBundledBinDir/originalDeus* to include originalPath and originalNodeEnv and ensure cleanup still removes tempRoots created by createBundledTool.apps/backend/test/unit/services/gh.service.test.ts-250-271 (1)
250-271:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore
DEUS_PACKAGEDto its previous value here.This
finallyblock always deletes the variable. If the suite starts in packaged mode, laterrunGhcases in this file will run under the wrong environment.Proposed fix
} finally { if (originalPath === undefined) delete process.env.PATH; else process.env.PATH = originalPath; - delete process.env.DEUS_PACKAGED; + if (originalDeusPackaged === undefined) delete process.env.DEUS_PACKAGED; + else process.env.DEUS_PACKAGED = originalDeusPackaged; } });🤖 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 `@apps/backend/test/unit/services/gh.service.test.ts` around lines 250 - 271, The finally block currently always deletes DEUS_PACKAGED; save its original value at the start of the test (e.g., const originalDeusPackaged = process.env.DEUS_PACKAGED) and in the finally restore it (if originalDeusPackaged is undefined delete process.env.DEUS_PACKAGED else set process.env.DEUS_PACKAGED = originalDeusPackaged) so the test that calls runGh(...) leaves DEUS_PACKAGED unchanged; update the test case "uses only bundled and system PATH entries in packaged runtime mode" to use that saved variable name and restore it in the finally block.scripts/runtime/run-version-check.cjs-55-75 (1)
55-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSkip stop-waiting when the child never got a PID.
If
spawn()fails before the process starts,child.pidis unset. This path still arms thestopTimeoutMstimer, so immediate failures get stretched into a 5-second delay.Proposed fix
function stopChild(child) { - if (child.exitCode !== null || child.signalCode !== null) return Promise.resolve(); + if (child.exitCode !== null || child.signalCode !== null || !child.pid) { + return Promise.resolve(); + } return new Promise((resolve) => {🤖 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/run-version-check.cjs` around lines 55 - 75, The stopChild function can hang on the stopTimeoutMs timer when spawn failed and child.pid is unset; modify stopChild to immediately resolve (skip setting the forceTimer and sending signals) when child.pid is null/undefined. Concretely, at the start of stopChild (before creating the Promise or arming the timeout) check if child.pid == null and if so return Promise.resolve(); keep the existing checks for exitCode/signalCode and the rest of the logic (finish, forceTimer, killChildTree) unchanged so normal children are still terminated.apps/backend/src/lib/sqlite.ts-56-65 (1)
56-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNarrow the function signature to accepted options or document the limitation.
The Bun adapter ignores most
BetterSqlite3.Options(line 64 only checksreadonly), so callers passingfileMustExist,timeout, or other options will see them silently dropped. Currently no callers pass those options, but the function signature claims to accept the fullBetterSqlite3.Optionstype.Either narrow the signature to
{ readonly?: boolean }or explicitly document which options are unsupported in Bun mode.🤖 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 `@apps/backend/src/lib/sqlite.ts` around lines 56 - 65, openSqliteDatabase currently accepts BetterSqlite3.Options but the Bun path (loadBunSqlite -> BunDatabase) only respects readonly, silently dropping other fields; narrow the function signature to accept a limited options type (e.g., { readonly?: boolean }) or add a clear JSDoc above openSqliteDatabase stating which BetterSqlite3.Options are unsupported in Bun mode (fileMustExist, timeout, verbose, etc.), and update references to the options parameter in the Bun branch (the bunOptions construction using options?.readonly) so callers and IDEs see the correct accepted shape; use the function name openSqliteDatabase and references to loadBunSqlite / BunDatabase / withBetterSqlitePragmaShape to locate and modify the code.scripts/runtime/agent-clis.ts-449-469 (1)
449-469:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPromise resolves on
exitinstead ofclose, risking truncated output.Same issue as in the CJS file: the
stdout/stderrstreams may still be draining whenexitfires. Consider waiting for thecloseevent before resolving to ensure all output is captured.🤖 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 449 - 469, The handler currently uses child.on("exit", ...) which can fire before stdout/stderr finish flushing; replace or supplement this with child.on("close", ...) to wait until streams are drained before resolving/rejecting. Update the logic around the existing child.on("exit", ...)/settled/timeout block so that you only call resolve() or fail(new Error(...)) inside a child.on("close", ...) callback (keeping the same status/code/signal checks and using stagedExecutableDiagnostics(executablePath), macExecutionPolicyHint(diagnostics), stdout/stderr, args, resolve, fail, timeout, and settled) and ensure you still clearTimeout(timeout) and set settled to true when finished.scripts/runtime/agent-clis.ts-534-537 (1)
534-537:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRedundant ternary always evaluates to
"--version".The conditional
tool === "claude" ? "--version" : "--version"produces the same value regardless of the condition.Suggested fix
const output = verifyVersion( executablePath, - [tool === "claude" ? "--version" : "--version"], + ["--version"], 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 `@scripts/runtime/agent-clis.ts` around lines 534 - 537, The array argument passed to verifyVersion uses a redundant ternary (tool === "claude" ? "--version" : "--version") that always yields "--version"; simplify by replacing that ternary with a single literal "--version" (update the call to verifyVersion(executablePath, ["--version"], env)), keeping the same variables executablePath, env and the verifyVersion function call.scripts/runtime/agent-clis.ts-263-263 (1)
263-263:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
tarextraction lacks timeout.A malformed or malicious tarball could cause
execFileSyncto hang indefinitely. Add a timeout consistent with other subprocess calls.Suggested fix
- execFileSync("tar", ["-xzf", tarballPath, "-C", tempRoot], { stdio: "pipe" }); + execFileSync("tar", ["-xzf", tarballPath, "-C", tempRoot], { + stdio: "pipe", + timeout: VERIFY_TIMEOUT_MS, + });🤖 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` at line 263, The tar extraction call using execFileSync("tar", ["-xzf", tarballPath, "-C", tempRoot], { stdio: "pipe" }) can hang indefinitely; update that call to include a timeout option consistent with other subprocess invocations in this file (e.g., add timeout: <sameTimeoutMs> to the options object) and propagate/handle the thrown error as the other execFileSync usages do; locate the execFileSync invocation (references: execFileSync, tarballPath, tempRoot) and add the timeout option to the options literal so the extraction will abort after the configured period.scripts/runtime/agent-clis.ts-224-249 (1)
224-249:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHTTP fetch lacks timeout and response size limit.
fetchBufferwill wait indefinitely if the server stalls or sends a slow-drip response. Additionally, there's no limit on response size, which could exhaust memory if the registry returns an unexpectedly large payload.Suggested fix with timeout
function fetchBuffer(url: string): Promise<Buffer> { - return new Promise((resolve, reject) => { - get(url, (response) => { + return new Promise((resolve, reject) => { + const request = get(url, (response) => { if ( response.statusCode && response.statusCode >= 300 && @@ ... const chunks: Buffer[] = []; + let totalSize = 0; + const maxSize = 100 * 1024 * 1024; // 100 MB limit response.on("data", (chunk: Buffer) => { + totalSize += chunk.length; + if (totalSize > maxSize) { + response.destroy(); + reject(new Error(`Response from ${url} exceeded ${maxSize} bytes`)); + return; + } chunks.push(chunk); }); response.on("end", () => resolve(Buffer.concat(chunks))); }).on("error", reject); + + request.setTimeout(60_000, () => { + request.destroy(); + reject(new Error(`Request to ${url} timed out`)); + }); }); }🤖 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 224 - 249, The fetchBuffer function lacks a request timeout and response size limit; update fetchBuffer to enforce a configurable timeout (e.g., default 10s) and a max response byte limit (e.g., default 50MB), canceling and rejecting the promise if either is exceeded; implement this by tracking received bytes in the "data" handler and rejecting+destroying the response when maxBytes is exceeded, and start a timer when the request is made that aborts the request and rejects when it fires (clearing the timer on "end" or error); ensure to remove listeners and call request.destroy() (or response.destroy()) on timeout or overflow so no handles leak and errors propagate correctly.
🧹 Nitpick comments (6)
scripts/runtime/smoke-packaged-app.cjs (1)
230-234: 💤 Low valueUse function composition for readability.
The chained callback in line 232-234 can be extracted to a named predicate for better clarity.
♻️ Optional refactor
const isForbiddenAsarEntry = (entry) => FORBIDDEN_RUNTIME_PACKAGE_PREFIXES.some((prefix) => entry.startsWith(prefix)) || - FORBIDDEN_RUNTIME_PACKAGE_ROOTS.some( - (root) => entry === root || entry.startsWith(`${root}/`) - ); + FORBIDDEN_RUNTIME_PACKAGE_ROOTS.some((root) => + entry === root || entry.startsWith(`${root}/`) + );🤖 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-app.cjs` around lines 230 - 234, Extract the inline predicate used in isForbiddenAsarEntry into a named helper for clarity: create a function (e.g., matchesForbiddenRoot) that takes an entry and returns FORBIDDEN_RUNTIME_PACKAGE_ROOTS.some(root => entry === root || entry.startsWith(`${root}/`)), then rewrite isForbiddenAsarEntry to call FORBIDDEN_RUNTIME_PACKAGE_PREFIXES.some(...) || matchesForbiddenRoot(entry); keep the same constants FORBIDDEN_RUNTIME_PACKAGE_PREFIXES and FORBIDDEN_RUNTIME_PACKAGE_ROOTS and preserve behavior.apps/backend/src/runtime/agent-process.ts (1)
6-23: ⚖️ Poor tradeoffConsider extracting the denylist to shared runtime constants.
RUNTIME_AGENT_SERVER_ENV_DENYLISTis defined here, but similar denylists likely exist in other runtime-related files (e.g., the smoke test scripts). Centralizing this inshared/runtime.tswould ensure consistency and reduce duplication.🤖 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 `@apps/backend/src/runtime/agent-process.ts` around lines 6 - 23, Extract the RUNTIME_AGENT_SERVER_ENV_DENYLIST constant into a shared runtime constants module (e.g., add it to shared/runtime.ts), export it (preserve as readonly tuple or equivalent), then replace the local RUNTIME_AGENT_SERVER_ENV_DENYLIST definition with an import from that shared module; update any other files (such as smoke test scripts or other runtime-related files) that duplicate the list to import the shared constant instead so all consumers reference the single source of truth.electron-builder.yml (1)
98-100: 💤 Low valueDocument why
--pagesize 4096is required.The
additionalArgumentsfor macOS signing includes--pagesize 4096, but there's no inline comment explaining why. This is likely for Apple Silicon compatibility, but future maintainers won't know whether this can be removed or if it addresses a specific issue.📝 Suggested comment
additionalArguments: + # Required for Apple Silicon code signing compatibility - "--pagesize" - "4096"🤖 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 `@electron-builder.yml` around lines 98 - 100, Add an inline comment next to the additionalArguments entry for "--pagesize" explaining why pagesize 4096 is required (e.g., to ensure Apple Silicon / ARM64 code signing compatibility or to work around a specific macOS linker/code-signing bug), reference the specific rationale or a link to the Apple/packaging issue that motivated this choice, and include a note about when it can be revisited; update the additionalArguments block (the "--pagesize" / "4096" entries) so future maintainers understand the why and when it might be removed.apps/runtime/index.ts (1)
130-156: 💤 Low valueDocument the fallback behavior when runtime executable cannot be resolved.
Line 155 falls back to
process.execPatheven whenbasename(process.execPath) !== RUNTIME_NAME. This means the returned path might bebunornodeinstead ofdeus-runtime. Downstream code should handle this case, or the function should fail explicitly.Consider adding a comment explaining when the fallback path is not a native runtime:
+ // Fallback: may return bun/node if running in development mode return process.execPath;🤖 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 `@apps/runtime/index.ts` around lines 130 - 156, resolveRuntimeExecutablePath currently returns process.execPath as a silent fallback which may be a different binary (e.g., node or bun) when basename(process.execPath) !== RUNTIME_NAME; update resolveRuntimeExecutablePath to either throw a clear Error (e.g., "deus-runtime not found: resolved execPath is not deus-runtime") when no matching candidate is found and basename(process.execPath) !== RUNTIME_NAME, or, if you prefer to keep the fallback, add a concise comment above the final return explaining that process.execPath may not be the native deus-runtime and downstream callers must handle that case; modify the function body around the final return to implement one of these behaviors and ensure messages reference RUNTIME_NAME and process.execPath so callers can locate the change.scripts/runtime/native-runtime.ts (1)
433-437: 💤 Low valueConsider adding an explicit
otoolavailability check or clearer error handling.The
execOutput("otool", ...)call will throw if Xcode Command Line Tools are missing, but the error may be cryptic. Since this is build-time verification code, a pre-flight check (e.g.,which otool) or a wrapper with a descriptive error message would improve developer experience when the dependency is not installed.🤖 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/native-runtime.ts` around lines 433 - 437, The execOutput("otool", ["-L", manifestCommandPath], projectRoot) call can throw a cryptic error if Xcode Command Line Tools (otool) are missing; update the code around manifestCommandPath/execOutput/verifyMacSystemDylibs to first check otool availability (e.g., run a lightweight execOutput("which" or "command -v", ["otool"]) or wrap the otool exec in a try/catch), and if missing or the exec fails, throw or log a clear, actionable error message that mentions otool/Xcode Command Line Tools and suggests installation steps before calling verifyMacSystemDylibs. Ensure the check references the same manifestCommandPath and preserves existing behavior when otool is present.scripts/prune-pencil-cli-binaries.cjs (1)
298-330: 💤 Low value
verifyMachOArchandverifyMachO64Archare near-duplicates.These two functions differ only in whether they check for
"Mach-O 64-bit executable"vs"Mach-O 64-bit". Consider consolidating into a single function with a parameter to control the expected pattern.Suggested consolidation
-function verifyMachOArch(filePath, label, expectedFileArch) { +function verifyMachOArch(filePath, label, expectedFileArch, requireExecutable = true) { const fileOutput = require("node:child_process") .execFileSync("file", [filePath], { encoding: "utf8", timeout: 20_000, stdio: ["ignore", "pipe", "pipe"], }) .trim(); + const pattern = requireExecutable ? "Mach-O 64-bit executable" : "Mach-O 64-bit"; if ( - !fileOutput.includes("Mach-O 64-bit executable") || + !fileOutput.includes(pattern) || (expectedFileArch && !fileOutput.includes(expectedFileArch)) ) { throw new Error(`Packaged ${label} has unexpected architecture: ${fileOutput}`); } console.log(`[runtime] packaged ${label}: ${fileOutput}`); } - -function verifyMachO64Arch(filePath, label, expectedFileArch) { - const fileOutput = require("node:child_process") - .execFileSync("file", [filePath], { - encoding: "utf8", - timeout: 20_000, - stdio: ["ignore", "pipe", "pipe"], - }) - .trim(); - if ( - !fileOutput.includes("Mach-O 64-bit") || - (expectedFileArch && !fileOutput.includes(expectedFileArch)) - ) { - throw new Error(`Packaged ${label} has unexpected architecture: ${fileOutput}`); - } - console.log(`[runtime] packaged ${label}: ${fileOutput}`); -}Then update the call site at line 595:
- verifyMachO64Arch(filePath, label, expectedFileArch); + verifyMachOArch(filePath, label, expectedFileArch, false);🤖 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/prune-pencil-cli-binaries.cjs` around lines 298 - 330, verifyMachOArch and verifyMachO64Arch are duplicated; replace both with a single function (e.g., verifyMachO(filePath, label, expectedFileArch, expectedPattern)) that accepts the expected pattern string (or a boolean flag like requireExecutable) and runs the shared execFileSync/check logic once, then update all callers that used verifyMachOArch and verifyMachO64Arch to call the new verifyMachO with the appropriate expectedPattern (e.g., "Mach-O 64-bit executable" or "Mach-O 64-bit") so behavior is preserved.
🤖 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 `@apps/agent-server/agents/environment/cli-discovery.ts`:
- Around line 103-121: The bundled-candidate branch currently only checks
existsSync and can mark discovery successful for directories or non-executables;
modify the bundled branch (the block checking candidate.source === "bundled" and
using candidatePath, state.executablePath, state.result) to first verify
candidatePath is a regular file (fs.statSync(...).isFile()) and is executable
(fs.accessSync(..., fs.constants.X_OK) on POSIX, or at minimum ensure isFile on
Windows), and if either check fails push a descriptive entry to triedCandidates
(e.g., `${candidatePath} (not a regular executable)`) and continue the loop
instead of setting state.result and returning success. Ensure the logic still
sets state.executablePath and state.result only after these checks pass.
In `@apps/backend/src/runtime/agent-process.ts`:
- Around line 46-52: The Windows branch in isExecutableFile currently returns
true for any regular file; change it to validate the file extension instead:
after confirming existsSync and stat.isFile(), if process.platform === "win32"
check path.extname(filePath).toLowerCase() against an allowed set (e.g. ['.exe',
'.com', '.cmd', '.bat', '.ps1']) and only return true when the extension is in
that set; keep the existing Unix permission check ((stat.mode & 0o111) !== 0)
for non-Windows. Use the existing function name isExecutableFile and the same
stat checks, and import/require path.extname where needed.
In `@apps/desktop/main/runtime-env.ts`:
- Around line 4-21: The PACKAGED_RUNTIME_ENV_DENYLIST constant is missing
BUN_OPTIONS, which allows parent Bun flags to leak into packaged runtimes;
update the PACKAGED_RUNTIME_ENV_DENYLIST array (exported const
PACKAGED_RUNTIME_ENV_DENYLIST) to include "BUN_OPTIONS" so that the sanitization
of child process env removes that variable when spawning deus-runtime; ensure
the new entry matches the existing style (string literal in the tuple) and run
tests or a quick spawn to verify deterministic runtime behavior.
In `@resources/entitlements.runtime.plist`:
- Around line 6-13: The entitlement
com.apple.security.cs.allow-unsigned-executable-memory looks unnecessary for
JavaScriptCore JIT; remove the
com.apple.security.cs.allow-unsigned-executable-memory key from
resources/entitlements.runtime.plist and keep com.apple.security.cs.allow-jit
only, unless you have a specific unsigned-executable-memory use-case. For
com.apple.security.cs.disable-library-validation, verify that bundled native
modules (referenced as codex, claude, gh, rg and any third-party .dylib) are
code-signed with your Team ID; if they are, remove the
com.apple.security.cs.disable-library-validation key and instead sign those
dylibs/modules with the app’s Team ID; if any module cannot be signed, document
the exception and retain the disable-library-validation entitlement only for
that case. Ensure entitlements remain minimal and update packaging/signing steps
to sign the listed native modules if you remove disable-library-validation.
In `@scripts/runtime/package-mac-dir.cjs`:
- Around line 103-122: The installIconResolver patch currently assumes the
internal API appBuilder.executeAppBuilderAsJson exists; add a runtime check at
the start of installIconResolver to verify that
require("app-builder-lib/out/util/appBuilder") returns an object and that
executeAppBuilderAsJson is a function, and if not throw a clear Error stating
the missing internal API and the expected electron-builder version/range (e.g.,
"executeAppBuilderAsJson not found; this smoke test requires electron-builder
^26.0.0"), then only proceed to capture realExecuteAppBuilderAsJson and patch
it; also update project docs or package.json to pin or document the compatible
electron-builder version range.
In `@scripts/runtime/run-version-check.cjs`:
- Around line 38-40: The writeResult function currently calls
process.stdout.write(...) then immediately process.exit(...), which can truncate
output when stdout is piped; change writeResult to exit only after the write has
flushed by using the write callback or checking its return value: call
process.stdout.write(JSON.stringify(result) + '\n', () =>
process.exit(exitCode)) or if you prefer to check the boolean return of
process.stdout.write, call process.exit(exitCode) immediately when it returns
true, otherwise attach a 'drain' listener to process.stdout to exit after drain;
reference function writeResult, process.stdout.write and process.exit.
In `@scripts/runtime/validate.ts`:
- Around line 116-154: assertStagedGhCli currently only verifies the staged
binaries against the staged gh-cli.json; modify it to also validate the manifest
against the canonical GH contract (GH_VERSION and target metadata) used when
preparing gh (extract that contract into shared code and import it here instead
of relying on the file alone). Specifically, create or import a shared constant
(e.g. GH_CONTRACT or exported GH_VERSION and DARWIN_NATIVE_CLI_TARGETS metadata
from the prepare-gh-cli module), then in assertStagedGhCli verify
manifest.ghVersion matches GH_VERSION and that manifest.targets contains entries
that match the canonical targets (runtimeKey, tool, fileArch, expected path
pattern) before doing the per-target binary checks; throw
createBuildRuntimeError on any mismatch to ensure stale artifacts fail
validation.
---
Outside diff comments:
In `@test/unit/runtime/validate-runtime.test.ts`:
- Around line 142-154: The test currently only checks that
validateStagedAgentClisMock was called once but not what it was called with;
update the test to assert the call received the correct arguments by adding an
expectation that validateStagedAgentClisMock was called with an object
containing the same projectRoot (and any expected options like log or other
flags) — e.g., use
expect(validateStagedAgentClisMock).toHaveBeenCalledWith(expect.objectContaining({
projectRoot })) to ensure validateRuntimeStage passes the correct parameters to
validateStagedAgentClis.
---
Minor comments:
In `@apps/backend/src/lib/sqlite.ts`:
- Around line 56-65: openSqliteDatabase currently accepts BetterSqlite3.Options
but the Bun path (loadBunSqlite -> BunDatabase) only respects readonly, silently
dropping other fields; narrow the function signature to accept a limited options
type (e.g., { readonly?: boolean }) or add a clear JSDoc above
openSqliteDatabase stating which BetterSqlite3.Options are unsupported in Bun
mode (fileMustExist, timeout, verbose, etc.), and update references to the
options parameter in the Bun branch (the bunOptions construction using
options?.readonly) so callers and IDEs see the correct accepted shape; use the
function name openSqliteDatabase and references to loadBunSqlite / BunDatabase /
withBetterSqlitePragmaShape to locate and modify the code.
In `@apps/backend/test/unit/services/gh.service.test.ts`:
- Around line 250-271: The finally block currently always deletes DEUS_PACKAGED;
save its original value at the start of the test (e.g., const
originalDeusPackaged = process.env.DEUS_PACKAGED) and in the finally restore it
(if originalDeusPackaged is undefined delete process.env.DEUS_PACKAGED else set
process.env.DEUS_PACKAGED = originalDeusPackaged) so the test that calls
runGh(...) leaves DEUS_PACKAGED unchanged; update the test case "uses only
bundled and system PATH entries in packaged runtime mode" to use that saved
variable name and restore it in the finally block.
In `@apps/runtime/index.ts`:
- Around line 202-207: The call in refreshNodePathResolution uses the private
NodeModule._initPaths() which may be absent under Bun and currently fails
silently; modify refreshNodePathResolution to detect whether
NodeModule._initPaths exists and if not log a clear warning via the existing
logger (or throw in CI), and add a fallback path: after checking
moduleWithInitPaths._initPaths, if it's undefined call
require('module').Module._resolveLookupPaths or re-initialize
require.cache/resolution in a supported way, and add a unit/integration test
that updates NODE_PATH (the change referenced at line ~319) and asserts that
module resolution succeeds; reference the function refreshNodePathResolution,
the NodeModule/_initPaths symbol, and the NODE_PATH update to locate where to
add logging, fallback behavior, and the new test.
In `@docs/deus-runtime-verification.md`:
- Around line 21-23: The example hardcodes the packaged app path to
dist-electron/mac-arm64/Deus.app while the packaging command documents
<arm64|x64>; update the second command (the node smoke-packaged-app.cjs
invocation) to accept the same architecture placeholder or show both
architectures (e.g., dist-electron/mac-<arch>/Deus.app or provide two example
lines for mac-arm64 and mac-x64) so the docs match the bun packaging command;
change the path in the example that currently contains
"dist-electron/mac-arm64/Deus.app" accordingly.
In `@scripts/runtime/agent-clis.ts`:
- Around line 449-469: The handler currently uses child.on("exit", ...) which
can fire before stdout/stderr finish flushing; replace or supplement this with
child.on("close", ...) to wait until streams are drained before
resolving/rejecting. Update the logic around the existing child.on("exit",
...)/settled/timeout block so that you only call resolve() or fail(new
Error(...)) inside a child.on("close", ...) callback (keeping the same
status/code/signal checks and using stagedExecutableDiagnostics(executablePath),
macExecutionPolicyHint(diagnostics), stdout/stderr, args, resolve, fail,
timeout, and settled) and ensure you still clearTimeout(timeout) and set settled
to true when finished.
- Around line 534-537: The array argument passed to verifyVersion uses a
redundant ternary (tool === "claude" ? "--version" : "--version") that always
yields "--version"; simplify by replacing that ternary with a single literal
"--version" (update the call to verifyVersion(executablePath, ["--version"],
env)), keeping the same variables executablePath, env and the verifyVersion
function call.
- Line 263: The tar extraction call using execFileSync("tar", ["-xzf",
tarballPath, "-C", tempRoot], { stdio: "pipe" }) can hang indefinitely; update
that call to include a timeout option consistent with other subprocess
invocations in this file (e.g., add timeout: <sameTimeoutMs> to the options
object) and propagate/handle the thrown error as the other execFileSync usages
do; locate the execFileSync invocation (references: execFileSync, tarballPath,
tempRoot) and add the timeout option to the options literal so the extraction
will abort after the configured period.
- Around line 224-249: The fetchBuffer function lacks a request timeout and
response size limit; update fetchBuffer to enforce a configurable timeout (e.g.,
default 10s) and a max response byte limit (e.g., default 50MB), canceling and
rejecting the promise if either is exceeded; implement this by tracking received
bytes in the "data" handler and rejecting+destroying the response when maxBytes
is exceeded, and start a timer when the request is made that aborts the request
and rejects when it fires (clearing the timer on "end" or error); ensure to
remove listeners and call request.destroy() (or response.destroy()) on timeout
or overflow so no handles leak and errors propagate correctly.
In `@scripts/runtime/package-mac-dir.cjs`:
- Around line 89-101: resolveIconInput currently throws a vague error when
resolution fails; update resolveIconInput to detect and report which step
failed: if roots is empty, include a message that no "--root" values were
provided; if candidates is empty, say no "--input" or "--fallback-input" values
were provided; otherwise, when files are not found, include the attempted
absolute paths (from absoluteCandidates) and the original args in the thrown
Error so it’s clear which resolved paths were checked; reference the
resolveIconInput function and the roots, candidates, and absoluteCandidates
variables when implementing the improved error messages.
In `@scripts/runtime/run-version-check.cjs`:
- Around line 55-75: The stopChild function can hang on the stopTimeoutMs timer
when spawn failed and child.pid is unset; modify stopChild to immediately
resolve (skip setting the forceTimer and sending signals) when child.pid is
null/undefined. Concretely, at the start of stopChild (before creating the
Promise or arming the timeout) check if child.pid == null and if so return
Promise.resolve(); keep the existing checks for exitCode/signalCode and the rest
of the logic (finish, forceTimer, killChildTree) unchanged so normal children
are still terminated.
In `@scripts/runtime/smoke-packaged-app.cjs`:
- Around line 44-67: The argument parser loop that consumes argv (the for loop
using argv and index) doesn't check for missing values when handling the "--app"
and "--arch" flags; update the logic so before doing argv[++index] you validate
that index + 1 < argv.length and throw a clear error like "Missing value for
--app" or "Missing value for --arch" (and do the same pattern for any future
flags that consume a value), only increment index after the check and assignment
so undefined is never stored into options.appPath or options.arch.
In `@test/unit/desktop/cli-tools.test.ts`:
- Around line 17-51: The afterEach cleanup in the test file fails to restore or
delete DEUS_RESOURCES_PATH which configurePackagedMainRuntimeEnv(...) may set,
causing cross-test environment leakage; update the afterEach block (the cleanup
function containing mockSyncShellEnvironment.mockClear and the environment
restores) to mirror the pattern used for other env vars by checking
originalDeusResourcesPath (capture its initial value at top like
originalBundledBinDir) and then either delete process.env.DEUS_RESOURCES_PATH if
it was undefined or restore it to originalDeusResourcesPath, ensuring
DEUS_RESOURCES_PATH is properly reset between tests.
In `@test/unit/desktop/terminal-command.test.ts`:
- Around line 11-41: The teardown currently restores only DEUS_* env vars but
not PATH or NODE_ENV, causing leaks from tests that call
configurePackagedMainRuntimeEnv(); capture the originals (e.g., const
originalPath = process.env.PATH and const originalNodeEnv = process.env.NODE_ENV
near the top alongside originalBundledBinDir) and in the afterEach block restore
them (delete if undefined or reassign otherwise) so PATH and NODE_ENV are reset
after each test; update references inside the same afterEach that handles
originalBundledBinDir/originalDeus* to include originalPath and originalNodeEnv
and ensure cleanup still removes tempRoots created by createBundledTool.
In `@test/unit/runtime/validate-runtime.test.ts`:
- Around line 127-131: The beforeEach uses mockReset() for
validateDeusRuntimeMock and validateStagedAgentClisMock but mockClear() for
execFileSyncMock, risking leaked implementations between tests; change
execFileSyncMock.mockClear() to execFileSyncMock.mockReset() inside the same
beforeEach so all three mocks are consistently reset (clear calls and
implementations) before each test, referring to validateDeusRuntimeMock,
validateStagedAgentClisMock, execFileSyncMock and the beforeEach hook.
---
Nitpick comments:
In `@apps/backend/src/runtime/agent-process.ts`:
- Around line 6-23: Extract the RUNTIME_AGENT_SERVER_ENV_DENYLIST constant into
a shared runtime constants module (e.g., add it to shared/runtime.ts), export it
(preserve as readonly tuple or equivalent), then replace the local
RUNTIME_AGENT_SERVER_ENV_DENYLIST definition with an import from that shared
module; update any other files (such as smoke test scripts or other
runtime-related files) that duplicate the list to import the shared constant
instead so all consumers reference the single source of truth.
In `@apps/runtime/index.ts`:
- Around line 130-156: resolveRuntimeExecutablePath currently returns
process.execPath as a silent fallback which may be a different binary (e.g.,
node or bun) when basename(process.execPath) !== RUNTIME_NAME; update
resolveRuntimeExecutablePath to either throw a clear Error (e.g., "deus-runtime
not found: resolved execPath is not deus-runtime") when no matching candidate is
found and basename(process.execPath) !== RUNTIME_NAME, or, if you prefer to keep
the fallback, add a concise comment above the final return explaining that
process.execPath may not be the native deus-runtime and downstream callers must
handle that case; modify the function body around the final return to implement
one of these behaviors and ensure messages reference RUNTIME_NAME and
process.execPath so callers can locate the change.
In `@electron-builder.yml`:
- Around line 98-100: Add an inline comment next to the additionalArguments
entry for "--pagesize" explaining why pagesize 4096 is required (e.g., to ensure
Apple Silicon / ARM64 code signing compatibility or to work around a specific
macOS linker/code-signing bug), reference the specific rationale or a link to
the Apple/packaging issue that motivated this choice, and include a note about
when it can be revisited; update the additionalArguments block (the "--pagesize"
/ "4096" entries) so future maintainers understand the why and when it might be
removed.
In `@scripts/prune-pencil-cli-binaries.cjs`:
- Around line 298-330: verifyMachOArch and verifyMachO64Arch are duplicated;
replace both with a single function (e.g., verifyMachO(filePath, label,
expectedFileArch, expectedPattern)) that accepts the expected pattern string (or
a boolean flag like requireExecutable) and runs the shared execFileSync/check
logic once, then update all callers that used verifyMachOArch and
verifyMachO64Arch to call the new verifyMachO with the appropriate
expectedPattern (e.g., "Mach-O 64-bit executable" or "Mach-O 64-bit") so
behavior is preserved.
In `@scripts/runtime/native-runtime.ts`:
- Around line 433-437: The execOutput("otool", ["-L", manifestCommandPath],
projectRoot) call can throw a cryptic error if Xcode Command Line Tools (otool)
are missing; update the code around
manifestCommandPath/execOutput/verifyMacSystemDylibs to first check otool
availability (e.g., run a lightweight execOutput("which" or "command -v",
["otool"]) or wrap the otool exec in a try/catch), and if missing or the exec
fails, throw or log a clear, actionable error message that mentions otool/Xcode
Command Line Tools and suggests installation steps before calling
verifyMacSystemDylibs. Ensure the check references the same manifestCommandPath
and preserves existing behavior when otool is present.
In `@scripts/runtime/smoke-packaged-app.cjs`:
- Around line 230-234: Extract the inline predicate used in isForbiddenAsarEntry
into a named helper for clarity: create a function (e.g., matchesForbiddenRoot)
that takes an entry and returns FORBIDDEN_RUNTIME_PACKAGE_ROOTS.some(root =>
entry === root || entry.startsWith(`${root}/`)), then rewrite
isForbiddenAsarEntry to call FORBIDDEN_RUNTIME_PACKAGE_PREFIXES.some(...) ||
matchesForbiddenRoot(entry); keep the same constants
FORBIDDEN_RUNTIME_PACKAGE_PREFIXES and FORBIDDEN_RUNTIME_PACKAGE_ROOTS and
preserve behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc38936c-29ef-4d6d-a4c3-a11d0a1fe792
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (99)
.github/workflows/release.yml.github/workflows/test.ymlREADME.mdapps/agent-server/agents/claude/claude-discovery.tsapps/agent-server/agents/codex-server/codex-server-client.tsapps/agent-server/agents/codex-server/codex-server-discovery.tsapps/agent-server/agents/codex-server/codex-server-handler.tsapps/agent-server/agents/codex/codex-discovery.tsapps/agent-server/agents/codex/codex-handler.tsapps/agent-server/agents/deus-tools/agent-browser-client.tsapps/agent-server/agents/deus-tools/sim-ops.tsapps/agent-server/agents/environment/cli-discovery.tsapps/agent-server/agents/environment/env-builder.tsapps/agent-server/agents/environment/packaged-cli-paths.tsapps/agent-server/build.tsapps/agent-server/messages/codex-sdk-adapter.tsapps/agent-server/test/agent-browser-client.test.tsapps/agent-server/test/claude-handler.test.tsapps/agent-server/test/cli-discovery.test.tsapps/agent-server/test/codex-sdk-adapter.test.tsapps/agent-server/test/codex-server-handler.test.tsapps/agent-server/test/e2e.test.tsapps/agent-server/test/env-builder.test.tsapps/backend/build.tsapps/backend/src/config/installed-apps.tsapps/backend/src/lib/database.tsapps/backend/src/lib/sqlite.tsapps/backend/src/routes/workspaces.tsapps/backend/src/runtime/agent-process.tsapps/backend/src/runtime/child-env.tsapps/backend/src/services/aap/apps.service.tsapps/backend/src/services/aap/lifecycle.tsapps/backend/src/services/gh.service.tsapps/backend/src/services/manifest.service.tsapps/backend/src/services/pty.service.tsapps/backend/src/services/recent-projects.service.tsapps/backend/src/services/workspace-init.service.tsapps/backend/test/unit/config/installed-apps.test.tsapps/backend/test/unit/runtime/agent-process.test.tsapps/backend/test/unit/runtime/child-env.test.tsapps/backend/test/unit/services/gh.service.test.tsapps/backend/test/unit/services/recent-projects.service.test.tsapps/cli/package.jsonapps/desktop/main/backend-process.tsapps/desktop/main/cli-tools.tsapps/desktop/main/index.tsapps/desktop/main/install-preflight.tsapps/desktop/main/native-handlers.tsapps/desktop/main/runtime-env.tsapps/desktop/main/shell-env.tsapps/desktop/main/terminal-command.tsapps/runtime/index.tsapps/web/src/platform/native/cli.tsdocs/deus-runtime-completion-audit.mddocs/deus-runtime-goal.mddocs/deus-runtime-verification.mdelectron-builder.ymlpackage.jsonpackages/pencil/build.tspackages/pencil/package.jsonpackages/screen-studio/src/renderer/frame-renderer.tsresources/entitlements.runtime.plistscripts/prepare-gh-cli.mjsscripts/prune-pencil-cli-binaries.cjsscripts/runtime/agent-clis.tsscripts/runtime/build.tsscripts/runtime/dev.tsscripts/runtime/electron-builder-before-pack.cjsscripts/runtime/native-runtime.tsscripts/runtime/package-mac-dir.cjsscripts/runtime/prepare-agent-clis.tsscripts/runtime/run-version-check.cjsscripts/runtime/runtime-smoke-rpc.cjsscripts/runtime/smoke-desktop-main-runtime.cjsscripts/runtime/smoke-native-runtime.cjsscripts/runtime/smoke-packaged-app.cjsscripts/runtime/smoke-packaged-desktop.cjsscripts/runtime/smoke-packaged-dmgs.cjsscripts/runtime/smoke-packaged-resources.cjsscripts/runtime/smoke-packaged-runtime.cjsscripts/runtime/smoke-source-runtime.cjsscripts/runtime/stage.tsscripts/runtime/unsupported-packaged-platform.cjsscripts/runtime/validate.tsscripts/verify-packaged-agent-clis.cjsshared/lib/cli-path.tsshared/runtime.tstest/unit/desktop/backend-process.test.tstest/unit/desktop/cli-tools.test.tstest/unit/desktop/github-cli-auth.test.tstest/unit/desktop/install-preflight.test.tstest/unit/desktop/runtime-env.test.tstest/unit/desktop/shell-env.test.tstest/unit/desktop/terminal-command.test.tstest/unit/runtime/electron-builder-before-pack.test.tstest/unit/runtime/prune-pencil-cli-binaries.test.tstest/unit/runtime/validate-runtime.test.tstest/unit/shared/cli-path.test.tstest/unit/shared/runtime.test.ts
💤 Files with no reviewable changes (1)
- apps/agent-server/agents/environment/packaged-cli-paths.ts
Comment on lines
+6
to
+13
| <key>com.apple.security.cs.allow-jit</key> | ||
| <true/> | ||
| <key>com.apple.security.cs.allow-unsigned-executable-memory</key> | ||
| <true/> | ||
|
|
||
| <!-- Native Node-compatible modules are loaded by the bundled runtime. --> | ||
| <key>com.apple.security.cs.disable-library-validation</key> | ||
| <true/> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
`Apple hardened runtime entitlement guidance for:
- com.apple.security.cs.allow-jit
- com.apple.security.cs.allow-unsigned-executable-memory
- com.apple.security.cs.disable-library-validation
For Bun/JavaScriptCore macOS apps, which are strictly required, and what safer alternatives are recommended to avoid broad exceptions?`
💡 Result:
For Bun/JavaScriptCore macOS apps with Hardened Runtime: - com.apple.security.cs.allow-jit: Strictly required for JavaScriptCore's JIT compilation fast-path, as it enables writable/executable memory via MAP_JIT. Without it, JSC falls back to interpreter or crashes [1][2]. Official JSC testapi uses it [3]. Bun's entitlements include it [4]. Safer alternative: None; it's the most secure option for JIT [5][1]. - com.apple.security.cs.allow-unsigned-executable-memory: Not strictly required if using MAP_JIT; broader and less secure, allowing W^X without JIT restrictions. Bun includes it [4], but Apple recommends avoiding in favor of allow-jit for DRM/legacy [1][6]. Safer alternative: Use only com.apple.security.cs.allow-jit [5]. - com.apple.security.cs.disable-library-validation: Likely required for Bun if loading unsigned/third-party dylibs (Bun uses it [4]), but not for pure JSC which uses special entitlements like com.apple.private.verified-jit [7]. Adds Gatekeeper scrutiny [8]. Safer alternative: Ensure all libraries are signed with same Team ID; avoid if possible [9][8].
Citations:
- 1: https://developer.apple.com/documentation/BundleResources/Entitlements/com.apple.security.cs.allow-jit
- 2: https://developer.apple.com/documentation/bundleresources/entitlements/com%5Fapple%5Fsecurity%5Fcs%5Fallow-jit
- 3: https://github.com/apple-opensource/JavaScriptCore/blob/master/testapi.entitlements
- 4: https://github.com/oven-sh/bun/blob/main/entitlements.plist
- 5: https://developer.apple.com/forums/thread/776290
- 6: https://developer.apple.com/documentation/bundleresources/entitlements/com.apple.security.cs.allow-unsigned-executable-memory
- 7: https://github.com/WebKit/WebKit/blob/main/Source/JavaScriptCore/entitlements.plist
- 8: https://developer.apple.com/documentation/BundleResources/Entitlements/com.apple.security.cs.disable-library-validation
- 9: https://developer.apple.com/documentation/security/hardened-runtime
🏁 Script executed:
# First, understand the structure and context of the entitlements file
find . -name "*.plist" -o -name "*entitlements*" | head -20Repository: zvadaadam/deus-machine
Length of output: 197
🏁 Script executed:
# Check if there's documentation in the repo explaining why these entitlements were added
rg -i "allow-jit|allow-unsigned-executable-memory|disable-library-validation" --type-list allRepository: zvadaadam/deus-machine
Length of output: 456
🏁 Script executed:
# Look for comments or documentation in the entitlements file itself and nearby files
cat -n resources/entitlements.runtime.plistRepository: zvadaadam/deus-machine
Length of output: 733
🏁 Script executed:
# Search for any build configuration or documentation about these specific entitlements
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.js" -o -name "*.ts" \) | xargs rg -l "entitlements|hardened-runtime" 2>/dev/null | head -10Repository: zvadaadam/deus-machine
Length of output: 178
🏁 Script executed:
# Check the runtime documentation for context on these entitlements
head -100 docs/deus-runtime-verification.mdRepository: zvadaadam/deus-machine
Length of output: 6786
🏁 Script executed:
# Search for references to these specific entitlements or native modules
rg "allow-unsigned-executable-memory|allow-jit|disable-library-validation|native.*module" docs/Repository: zvadaadam/deus-machine
Length of output: 169
🏁 Script executed:
# Check the native-runtime script for insights about module loading
head -100 scripts/runtime/native-runtime.tsRepository: zvadaadam/deus-machine
Length of output: 2356
Review the necessity of allow-unsigned-executable-memory and verify library signatures.
The bundled runtime currently includes three hardened-runtime entitlements, but allow-unsigned-executable-memory appears redundant: Apple's guidance indicates that allow-jit alone is the secure choice for JavaScriptCore, while allow-unsigned-executable-memory is a broader exception intended for DRM/legacy compatibility. Removing it would strengthen the security posture without breaking JIT functionality.
For disable-library-validation, verify whether the bundled native modules (codex, claude, gh, rg) and third-party dylibs are code-signed with your Team ID. If they are, this entitlement can be removed in favor of proper library signing, which is the recommended safer alternative per Apple's hardened-runtime guidance.
🤖 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 `@resources/entitlements.runtime.plist` around lines 6 - 13, The entitlement
com.apple.security.cs.allow-unsigned-executable-memory looks unnecessary for
JavaScriptCore JIT; remove the
com.apple.security.cs.allow-unsigned-executable-memory key from
resources/entitlements.runtime.plist and keep com.apple.security.cs.allow-jit
only, unless you have a specific unsigned-executable-memory use-case. For
com.apple.security.cs.disable-library-validation, verify that bundled native
modules (referenced as codex, claude, gh, rg and any third-party .dylib) are
code-signed with your Team ID; if they are, remove the
com.apple.security.cs.disable-library-validation key and instead sign those
dylibs/modules with the app’s Team ID; if any module cannot be signed, document
the exception and retain the disable-library-validation entitlement only for
that case. Ensure entitlements remain minimal and update packaging/signing steps
to sign the listed native modules if you remove disable-library-validation.
Owner
Author
There was a problem hiding this comment.
Deferred — I am keeping these entitlements for this PR because removing them requires a notarized release/Gatekeeper verification pass and a Team-ID signing plan for all native modules. The packaged runtime smoke still verifies the current entitlements explicitly.
There was a problem hiding this comment.
@zvadaadam, understood — that's a reasonable deferral. Tightening to allow-jit-only and removing disable-library-validation does require a full notarization + Gatekeeper smoke pass and a Team-ID signing plan for all bundled native modules, so keeping them as-is for this PR is the pragmatic call.
Would you like me to open a follow-up GitHub issue to track the entitlement hardening work (removing allow-unsigned-executable-memory, signing bundled modules with your Team ID, and removing disable-library-validation)?
Owner
Author
Greenlight — Round 4Fixed (19): executable validation for Claude/E2E and bundled CLI discovery; packaged NODE_ENV/env scrubbing including BUN_OPTIONS; better-sqlite3 required binding validation; timeout parsing and flush/close handling in runtime version checks; subprocess timeout/error handling; GH CLI contract validation; packaged app path detection; test cleanup/call-count assertions; electron-builder internal API guard. Deferred (1): hardened-runtime entitlement minimization. Keeping current Bun/runtime entitlements in this PR because removing them needs notarized release/Gatekeeper verification and a native-module Team-ID signing plan. CI and CodeRabbit are green on 6a5ca63. |
This was referenced May 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Local blocker
Direct native/package desktop execution is still blocked on this workstation before user code at _dyld_start for generated/copied Mach-O binaries. The PR macOS runtime CI is expected to run the packaged deus-runtime direct smokes on GitHub-hosted macOS.
Summary by CodeRabbit
New Features
Platform Changes
Reliability
Tests
Documentation