[codex] Isolate mutation test runs - #1682
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:
📝 WalkthroughWalkthroughAdds isolated mutation-run execution in a copied working checkout with persisted run records and CLI controls, updates the mutation entrypoints to use it, and expands tests and docs. Also adds a shared filesystem existence helper and adjusts stripe-mock test timing. ChangesMutation isolation runner
Test utility and stripe-mock test adjustments
Sequence Diagram(s)sequenceDiagram
participant DenoTask as deno task mutation
participant IsolationCLI as scripts/mutation.ts
participant Supervisor as scripts/mutation/isolation.ts
participant SnapshotRun as scripts/mutation.ts child
participant RunState as scripts/mutation/isolation-state.ts
DenoTask->>IsolationCLI: start with mutation args
IsolationCLI->>Supervisor: runIsolatedMutationCommand(Deno.args)
Supervisor->>RunState: createRunId, newRunRecord, copyMutationSnapshot
Supervisor->>SnapshotRun: spawn with MUTATION_* env vars and rewritten args
SnapshotRun->>RunState: withMutationRunLock / read and write run.json
SnapshotRun->>Supervisor: exit code and status
Supervisor->>RunState: markFinished / markInterrupted
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 27bc9a0552
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
32c67ae to
ac81824
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/mutation/isolation.ts (1)
167-175: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRecord interrupted runs as
interruptedinscripts/mutation/isolation.ts:167-175. In the post-spawn path,child.status.codewill usually be the signal exit code (typically 143 for SIGTERM), somarkFinished(record, status.code)storesfailed; thecatchbranch already maps130tointerrupted.🤖 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/mutation/isolation.ts` around lines 167 - 175, The post-spawn completion path in the isolation runner is treating signal-based exits as failed instead of interrupted. Update the `child.status` handling in `scripts/mutation/isolation.ts` so `markFinished` uses the same interruption mapping as the `catch` branch, and ensure `writeRunRecord` persists `interrupted` for signal exits such as SIGTERM rather than storing the raw status code. Refer to the `child.status`, `markFinished`, and `writeRunRecord` flow when making the fix.
🤖 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 `@test/scripts/mutation-isolation.test.ts`:
- Around line 234-262: The test combines multiple concerns in one body, making
failures hard to attribute to a specific unit. Split the assertions in
mutation-isolation.test.ts into separate tests: keep one focused on selectedRuns
filtering using the minimal record fixtures needed, and move
visibleStatus/formatRunList coverage into its own test with only the fixtures
required. Use the existing helpers newRunRecord, markRunning, markFinished, and
markInterrupted only where they are necessary for each specific behavior.
---
Outside diff comments:
In `@scripts/mutation/isolation.ts`:
- Around line 167-175: The post-spawn completion path in the isolation runner is
treating signal-based exits as failed instead of interrupted. Update the
`child.status` handling in `scripts/mutation/isolation.ts` so `markFinished`
uses the same interruption mapping as the `catch` branch, and ensure
`writeRunRecord` persists `interrupted` for signal exits such as SIGTERM rather
than storing the raw status code. Refer to the `child.status`, `markFinished`,
and `writeRunRecord` flow when making the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7bb82122-c119-4cfb-a15a-5b308056b931
📒 Files selected for processing (3)
scripts/mutation/isolation-state.tsscripts/mutation/isolation.tstest/scripts/mutation-isolation.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac81824e93
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/mutation/isolation.ts`:
- Line 32: The re-export of MUTATION_SNAPSHOT_CHILD_ENV in isolation.ts is
unnecessary because it only forwards the constant unchanged. Remove that alias
export and update scripts/mutation.ts to import MUTATION_SNAPSHOT_CHILD_ENV
directly from isolation-state.ts, keeping runIsolatedMutationCommand unchanged
and avoiding the extra forwarding layer.
- Around line 43-62: The liveness check in processBelongsToRun relies on
processEnv reading /proc/<pid>/environ, which is Linux-only and breaks non-Linux
behavior. Update the processBelongsToRun/processEnv flow to avoid
unconditionally reading /proc: either gate the /proc-based logic behind a
Linux-only check or replace it with a cross-platform way to verify the mutation
run identity, so --list, --kill, and cleanup paths can still recognize running
workers on macOS.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9c04aade-234f-4a36-873c-a751940deb0f
📒 Files selected for processing (11)
.gitignoreAGENTS.mdscripts/mutation.tsscripts/mutation/isolation-state.tsscripts/mutation/isolation.tsscripts/precommit-mutation.tstest/lib/stripe-mock/install.test.tstest/lib/stripe-mock/ports.test.tstest/scripts/mutation-isolation.test.tstest/scripts/test-coverage.test.tstest/test-utils/files.ts
ac81824 to
dbc2cde
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbc2cde694
ℹ️ 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".
dbc2cde to
f6acf04
Compare
f6acf04 to
442cf62
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/scripts/mutation-isolation-supervisor.test.ts`:
- Line 245: `readRunRecords(root)` can return an empty array, so the repeated
`const [record] = await readRunRecords(root);` destructures in
`mutation-isolation-supervisor.test.ts` leave `record` possibly undefined and
fail typechecking when accessing its fields. Add a shared narrowing helper in
`mutation-isolation-helpers.ts` (for example, a first-record accessor that calls
`readRunRecords` and throws if no record exists), then replace each destructure
with that helper in the affected test cases so `record.status`,
`record.exitCode`, and similar accesses are always on a defined value.
- Around line 172-175: The `calls` and `killCalls` arrays in the `Deno.kill`
stubs are using an optional `signal?: Deno.Signal` shape, but the stub records
`signal` as `Deno.Signal | undefined`, which breaks under
`exactOptionalPropertyTypes`. Update the recorded type in the relevant test
helpers (`stub(Deno, "kill")` and the `killCalls` collector) so `signal`
explicitly allows `undefined`, or otherwise avoid pushing an explicit
`undefined` value for that property; keep the shape consistent wherever the
kill-call objects are created.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7e274c6e-db9a-42d2-8a7e-d0986d66359f
📒 Files selected for processing (6)
scripts/mutation.tsscripts/mutation/isolation-state.tsscripts/mutation/isolation.tstest/scripts/mutation-isolation-helpers.tstest/scripts/mutation-isolation-supervisor.test.tstest/scripts/mutation-isolation.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 454102c22e
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01dce176c4
ℹ️ 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".
|
Re: Stop the child when recording its PID fails Addressed in 4023986. If |
|
Re: Allow stale copying runs to be cleaned ( Addressed in 4023986. The |
|
Re: Use a portable liveness probe Addressed in 4023986. The |
|
Re: Do not report failed cleanup as removed Addressed in 4023986. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 402398666f
ℹ️ 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".
runningProcessStillExists now requires the run lock to be held OR the record to be within a 30s startup grace, so a stale running record whose PID was reused by an unrelated process becomes cleanable once the grace expires. Fixes the last outstanding Codex review comment.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14c856fd49
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e313e9af04
ℹ️ 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".
Restructure the stopProcess call in the catch block to avoid an unreachable defensive catch (stopProcess already handles kill errors internally). Fix the test stub to target the running-status write specifically rather than counting global writeTextFile calls, which was misfiring because writeFakeMutationScript also uses writeTextFile.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d74f54cc1
ℹ️ 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".
1 participant
What changed
Mutation tests now run inside a copied checkout at
.mutation-runs/<id>/work, so the live working tree is not mutated while the existing mutation engine still works in place inside the copy.Each isolated run records its status and child PID in
.mutation-runs/<id>/run.json. The mutation command can now list runs, kill live runs, and clean finished or stale runs.The review follow-up tightened the run management paths: live checks no longer depend on
/proc, reused PIDs are treated as stale unless the per-run lock is held, cleanup uses the current.mutation-runs/<id>directory instead of trusting stale JSON paths, and active copying/running snapshots are skipped by cleanup.The precommit mutation gate uses the same isolated runner as manual mutation commands.
Why
Mutation testing used to write directly into live files and could leave hard-to-identify subprocesses running. This makes mutation runs visible, removable, and much less likely to interfere with normal work.
Checks
deno task typecheckdeno task test:files test/scripts/mutation-isolation.test.ts test/scripts/mutation-isolation-supervisor.test.tsdeno task lint:cideno task cpddeno task test:coveragetest,CodeRabbit, and both deploy checks passedFull
deno task precommitwas not rerun, per request.