Skip to content

fix(codex): migrate stale desktop runtimes - #1027

Merged
NicholaiVogel merged 3 commits into
mainfrom
aaf2tbz/fix-codex-desktop-runtime
Jul 28, 2026
Merged

fix(codex): migrate stale desktop runtimes#1027
NicholaiVogel merged 3 commits into
mainfrom
aaf2tbz/fix-codex-desktop-runtime

Conversation

@aaf2tbz

@aaf2tbz aaf2tbz commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes #1024. Codex Desktop packaging updates can move its bundled Node executable, leaving Signet-owned lifecycle hooks and MCP stdio configured with a dead absolute path. The connector now discovers and validates the current bundled runtime before rewriting those Signet-owned commands.

Changes

  • Scan supported macOS Codex.app resource layouts for an executable Node runtime and validate it with --version.
  • Use the validated runtime for Signet hook commands and local MCP stdio when the installed Signet entry points are available.
  • Detect a missing prior Signet runtime and emit a focused migration warning while preserving unrelated hooks.
  • Add the Codex Desktop packaging-update regression test and document refresh behavior.

Type

  • feat — new user-facing feature (bumps minor)
  • fix — bug fix
  • refactor — restructure without behavior change
  • chore — build, deps, config, docs
  • perf — performance improvement
  • test — test coverage

Packages affected

  • @signet/core
  • @signet/daemon
  • @signet/cli / dashboard
  • @signet/sdk
  • @signet/connector-*
  • @signet/web
  • predictor
  • Other:

Screenshots

N/A — no UI changes.

PR Readiness (MANDATORY)

  • Spec alignment validated (INDEX.md + dependencies.yaml) — connector-install behavior only; no product spec dependency changed.
  • Agent scoping verified on all new/changed data queries — N/A; no data queries.
  • Input/config validation and bounds checks added — runtime must be a regular executable file and pass node --version; scanning is depth bounded.
  • Error handling and fallback paths tested (no silent swallow) — unusable candidates are skipped; existing Signet command resolution remains the fallback.
  • Security checks applied to admin/mutation endpoints — N/A; no endpoint changes.
  • Docs updated for API/spec/status changes — Codex integration guide documents runtime discovery and migration.
  • Regression tests added for each bug fix — macOS packaging-update fixture covers hooks, MCP, and the migration diagnostic.
  • Lint/typecheck/tests pass locally — affected-file Biome check, connector typecheck, and 59/59 connector tests pass. Repository-wide lint/typecheck/test failures reproduce on untouched main (existing format/type drift, stale generated route/content guards, and local SQLite extension loading).

Migration Notes (if applicable)

  • Migration is idempotent — reconnect rewrites only the Signet hook and MCP registrations.
  • Daemon Rust parity reviewed or explicitly N/A — N/A; this is an install-time TypeScript connector change.
  • Rollback / compatibility note included in PR description — reinstalling older Signet restores its previous command strategy; non-Signet hooks are preserved.

Testing

  • bun test passes — bun test integrations/codex/connector/src/config-toml.test.ts: 59 pass, 0 fail. Full bun test has only failures reproduced on untouched main: three Rust route-parity guard failures, one generated web-content-index guard failure, and this macOS hosts SQLite build cannot load vec_version().
  • bun run typecheck passes — connector package passes. Root typecheck failures are pre-existing daemon type errors reproduced on untouched main.
  • bun run lint passes — touched files pass Biome. Root lint has 726 pre-existing diagnostics reproduced on untouched main.
  • Tested against running daemon — N/A; no daemon runtime behavior changed.
  • N/A

AI disclosure

  • No AI tools were used in this PR
  • AI tools were used (see Assisted-by tag in commit)

Notes

The connector uses the current Codex Desktop runtime only when the installed Signet JS entry points can be identified. Otherwise it retains the existing portable Signet command fallback.

@aaf2tbz
aaf2tbz marked this pull request as ready for review July 25, 2026 21:18
@aaf2tbz aaf2tbz self-assigned this Jul 25, 2026

@NicholaiVogel NicholaiVogel left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NicholaiVogel

Copy link
Copy Markdown
Collaborator

Request changes — the fix is dead code in production + two real regressions

Reviewed the full diff, the connector source, the codex test suite, and the production launch path. The design is right (discover → validate → apply to Signet-owned paths only, preserve third-party hooks). But the "apply" wiring never fires in production, and two of the new code paths introduce regressions. All three verified empirically, not just by reading.

🔴 Critical: resolveSignetEntry() never matches in production — the discovered runtime is thrown away

resolveSignetEntry() gates on basename(process.argv[1]) === "signet.js" (index.ts:59). Production Signet is a Bun-compiled native binary: dist/signetai/bin/signet.jslaunch.js spawns the binary with process.argv.slice(2), and inside that binary argv[1] is the virtual bunfs path, not signet.js.

Empirical proof — I built a minimal bun build --compile probe:

$ ./probe-bin one two
argv: ["bun","/$bunfs/root/probe-bin","one","two"]
basename(argv[1]): probe-bin
basename check (the PR's gate): false

And dist/signetai/bin/launch.js confirms there's no node-mode fallback — if the binary is missing it process.exit(1)s; the connector code only ever runs inside the compiled binary where argv[1] = /$bunfs/root/signet.

Consequence: resolveSignetEntry() always returns nullresolveSignetArgs(runtime) never takes the runtime && entry branch → the discovered Codex Desktop node is computed by resolveCodexDesktopNode() and then discarded. Hooks/MCP fall back to bare PATH signet/signet-mcp. The entire fix for #1024 is dead code in production. The new test passes only because it mutates process.argv[1] = signetEntry (a .js path) at config-toml.test.ts:~919 — a value that never occurs at runtime.

Suggested fix: don't gate on argv[1]. Derive the Signet entry from process.execPath (the native binary's install root, e.g. join(dirname(process.execPath), "..", "bin", "signet.js")), or reuse the existing resolveSignetCliCommand() layout logic in libs/connector-base — then existsSync-validate. Add a production-realistic test that does not mutate argv[1] (or simulates the bunfs argv) so this dead branch is caught.

🟡 Warning: hasMissingSignetRuntime crashes on malformed-but-valid hooks.json (regression)

hasMissingSignetRuntime (index.ts:483) traverses the hooks file unguarded: groups.flatMap(...)group.hooks.map(...)handler.command. I confirmed it throws on three malformed-but-valid-JSON inputs:

  • {hooks: {SessionStart: "x"}}groups.flatMap is not a function
  • {hooks: {SessionStart: [{hooks: "x"}]}}group.hooks.map is not a function
  • {hooks: {SessionStart: [{hooks: [null]}]}}null is not an object (evaluating 'handler.command')

This is a regression: pre-PR install() called resolveSignetArgs() directly and let the guarded installCompatibilityHooks handle such files. Post-PR, install() calls hasMissingSignetRuntime(readHooksFile(...), configPath) first (line 1002), so a hand-edited or partial hooks.json that previously installed fine now aborts the whole install with an uncaught TypeError. Every sibling reader guards this way — isSignetMatcherGroup (index.ts:471 if (!Array.isArray(hooksArr))), removeSignetEntries (if (!Array.isArray(groups)) continue;).

Suggested fix: mirror the sibling guards: if (!Array.isArray(groups)) continue; per event, if (!Array.isArray(group?.hooks)) continue; per group, if (typeof handler?.command !== "string") continue; per handler.

🟡 Warning: codexDesktopNodeCandidates is symlink-blind

readdirSync(root, { withFileTypes: true }) then entry.isFile() / entry.isDirectory(). Dirent methods use lstat semantics — I confirmed a symlinked node reports isFile: false, isDirectory: false, isSymbolicLink: true. So:

  • If Codex.app ships node as a symlink to a versioned binary (e.g. bin/node -> node-v22.14.0, a common bundler/version-manager pattern), the candidate is silently dropped.
  • Symlinked runtime directories under Contents/Resources aren't recursed into.

In both cases resolveCodexDesktopNode returns null and the feature silently no-ops. The test fixture uses a real file, so this isn't covered.

Suggested fix: accept symlinks — (entry.isFile() || entry.isSymbolicLink()) && entry.name === "node" for the file match, and use statSync(path) (follows symlinks) for the directory recursion check. Wrap the statSync in try/catch to stay graceful on broken links.

What's sound (verified)

  • Third-party hooks are preserved correctly — installCompatibilityHooks calls removeSignetEntries then re-adds Signet's own, so only Signet-owned commands get the new runtime path.
  • Null-runtime fallback is clean — when Desktop isn't installed, resolveSignetArgs(null)resolveSignetCliCommand() (pre-existing behavior, no regression in the common non-Desktop case).
  • Scan can't escape Codex.app (rooted at Contents/Resources, depth-bounded at 8).
  • Stale-detection regex is safe — no ReDoS, correct node-path extraction across quoted/Windows/node_modules cases.
  • Validation (spawnSync --version, 5s timeout, strict vN.N.N regex) is sound; no shell injection (path passed directly).

Minor (not blocking)

  • The stale-runtime warning fires based purely on hasMissingSignetRuntime, independent of whether resolveCodexDesktopNode found a replacement. If Desktop was uninstalled (runtime=null) but stale paths remain, the warning asserts a "refresh" that actually fell back to PATH signet. Consider splitting into "replaced" vs "no replacement found, used PATH fallback."
  • entry.isDirectory() && entry.name !== "app.asar" contains a dead predicate — app.asar is a file, so isDirectory() is already false. If the intent was to skip app.asar.unpacked (a real directory), that isn't excluded.

How I validated

  • Built core+connectors in an isolated worktree on the PR head; ran integrations/codex/connector/src/config-toml.test.ts — 59/59 pass (including the new migration test, which passes only because of the argv[1] mutation).
  • Built a bun build --compile probe confirming argv[1] inside a compiled binary is /$bunfs/root/<name>.
  • Reproduced all three hasMissingSignetRuntime crashes and the symlink blindness directly.

The approach is right; the production wiring just needs to not depend on argv[1].

@aaf2tbz

aaf2tbz commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the review findings and rebased this PR onto the latest main.

  • Resolve the packaged Signet entry from process.execPath, so Bun-compiled production binaries no longer depend on the virtual argv[1] path.
  • Guard malformed-but-valid hooks.json structures during stale-runtime detection.
  • Follow symlinked Codex runtime directories and node binaries; broken links are ignored safely.
  • Clarify the stale-runtime warning when no replacement runtime is found.

Added regression coverage for Bun-style argv, malformed hooks files, and symlinked runtimes. Verified with bun test integrations/codex/connector/src/config-toml.test.ts (61 passing), Biome, and the connector TypeScript check.

@aaf2tbz
aaf2tbz force-pushed the aaf2tbz/fix-codex-desktop-runtime branch from c19ad0b to bdc1082 Compare July 28, 2026 03:10
@NicholaiVogel
NicholaiVogel force-pushed the aaf2tbz/fix-codex-desktop-runtime branch from 0084767 to f067a77 Compare July 28, 2026 11:47
@NicholaiVogel
NicholaiVogel merged commit 1eb8c3c into main Jul 28, 2026
15 checks passed
@NicholaiVogel
NicholaiVogel deleted the aaf2tbz/fix-codex-desktop-runtime branch July 28, 2026 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(codex): migrate stale desktop runtime paths used by hooks and MCP

2 participants