Skip to content

Helper-lifecycle test hygiene: PID sentinels, process.ppid, an unexercised selector branch, and a shipped test hook #668

Description

@possibilities

Follow-up from CodeRabbit review of #665. All of it is #664 surface, so it is filed rather than folded into a PR whose scope is one behavior change.

1. Dead-helper fixtures rely on PIDs above the platform ceiling

test/app-bind.test.ts:1143-1144 (and :1187) use 2_147_483_646/-645/-644; test/codex-manager-rotation-command.test.ts:484-494 uses 99999999; test/runtime-current-account.test.ts:573-598 uses 99999998/99999999. These exceed pid_max on Linux and PID_MAX on macOS, so process.kill(pid, 0) may raise EINVAL rather than ESRCH.

Worth noting what saves it today: every liveness check in the tree returns true only for EPERM and treats every other errno as dead (lib/runtime/app-bind.ts:417-427, lib/runtime/runtime-current-account.ts:114-124, lib/codex-manager/commands/rotation.ts:593-603, test/codex-bin-wrapper.test.ts:722-729), so EINVAL classifies as dead either way. The fixtures are correct by a property they never state. Owning a child and killing it — as reaps a stranded helper whose owner PID is genuinely dead in #665 does — makes "dead" a fact instead.

2. process.ppid makes the "+1 more running" count environment-dependent

test/codex-manager-rotation-command.test.ts:503 depends on the vitest worker's parent staying alive. That parent is the pool process, which the test does not control; reparenting differs across threads, forks, and --no-file-parallelism, and a parent exiting mid-run flips the assertion to (+0 more running). A sleeper the test owns and kills in finally makes the second live PID deterministic.

3. readAppRuntimeHelperStatus never exercises its recency sort

lib/runtime/runtime-current-account.ts:168-188 sorts live candidates by recency before returning the first, but test/runtime-current-account.test.ts:571 supplies exactly one live candidate, so live.sort(byRecency) is dead weight in the test. The equivalent case exists for the rotation command (test/codex-manager-rotation-command.test.ts:449), but that is a different selector, and this one drives runtime account resolution.

4. A metadata-cleanup fault injector ships in the published wrapper

scripts/codex.js:4088 reads its environment variable unconditionally and package.json publishes the file. The default is 0, but Number.parseInt also accepts "2abc" and "1e3". Either remove the hook from shipped code or put it behind an explicit test-only gate, with a regression case asserting production invocations ignore it.

5. Minor: re-encoded owner-path literal in tests

test/app-bind.test.ts:1199 hand-builds runtime-rotation-app-helper-owner.<pid>.json although resolveRuntimeHelperOwnerPath already exists at test/app-bind.test.ts:95-108.

Credit: CodeRabbit, on #665.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions