fix(ci): measure a stacked unit against its parent pull request - #1924

Summary by CodeRabbit

• Chores
  • Mutation-testing checks can recognize a non-draft parent pull request in a stack and compare the current pull request against it.
  • For recognized stacked pull requests, checks use the parent pull request’s head and include stacked-pull-request context and a reproduction command in the summary.
  • Otherwise, checks retain the standard event-based comparison. If pull request details are unavailable or invalid, checks continue without a stacked pull request match; if the pull request head cannot be resolved, checks fall back to the merge commit.
  • Checks reinstall dependencies when relevant manifests differ after aligning the comparison. Failures are reported and stop the check.

The workflow passes open pull request data to the mutation diff script. When a PR head’s sole parent matches a confirmed non-draft open PR head targeting the same base ref, the script uses that parent as the diff base and the PR head as the diff head. It aligns the execution tree and records the matched parent PR in the summary.

Changes: Stacked-unit mutation diff

Layer / File(s)	Summary
Fetch and pass open pull request data .github/workflows/mutation-testing.yml, scripts/stryker-diff.test.mjs	The workflow fetches open pull request numbers, head SHAs, draft status, and base refs, then passes the map and merge commit details to the diff command. Tests check workflow permissions, token scope, workflow inputs, artifact transfer, and failure handling.
Parse pull request map and resolve base scripts/stryker-diff.mjs, scripts/stryker-diff.test.mjs	The script accepts valid map entries and uses a sole parent as the base only when it matches a confirmed non-draft open PR head targeting the same base ref. Otherwise, it retains the event base. Tests cover parsing, matching, and fallback cases.
Align and select the unit diff scripts/stryker-diff.mjs, scripts/stryker-diff.test.mjs	For a matched stacked unit, the script uses the PR head as the diff head, aligns the execution tree, checks dependency-manifest drift, and preserves the resolved base during selection. It records the parent PR and adds summary and reproduction details. Tests cover selection, alignment, drift, reinstall failures, and output.

Estimated code review effort: 4 (Complex) | ~50 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant MutationTestingWorkflow
  participant GitHubAPI
  participant StrykerDiff
  participant GitCommitGraph
  MutationTestingWorkflow->>GitHubAPI: Fetch open pull request data
  GitHubAPI-->>MutationTestingWorkflow: Return pull request map
  MutationTestingWorkflow->>StrykerDiff: Pass map and merge commit details
  StrykerDiff->>GitCommitGraph: Read PR head parent
  GitCommitGraph-->>StrykerDiff: Return parent SHA
  StrykerDiff->>StrykerDiff: Align tree and select unit diff

Loading

Possibly related issues

• [split-1066] U7 - fix(write-to-file): clean partial state on missing-param and rooignore denial #1938: The issue records a mutation-gate measurement for a stacked unit. This change affects the measurement scope used by the mutation diff.

Possibly related PRs

• Zoo-Code-Org/Zoo-Code#2149: The PR summarizes and publishes reports from the mutation-diff job whose measured scope this change affects.

Merge Risk: Moderate · up to fc17b

The gate can still run with a missing map, but its regression check is weak and pull-request code may receive unnecessary write permissions. Remove that exposure before merging.

Security architecture risk: Moderate · up to d4442

The new read token is appropriately isolated, but stacked measurement can omit inherited changes without a cumulative queue recheck and can apply child-revision line numbers to merged code. These changes weaken CI assurance; they do not directly grant production privileges.

Retained concerns

• Medium · security · inferred: An open parent PR is sufficient to exempt its ancestor changes from the child's executable-line cap, without checking that the parent passed or will merge separately. A contributor controlling two PR heads could therefore obtain a passing child measurement while its actual merge includes an oversized parent delta. Previously the child gate measured that cumulative delta. The unchanged merge-group path only writes a summary, so it does not compensate for the newly narrowed scope. Actual admission remains dependent on external required-check and approval configuration.
• Medium · reliability · inferred: Recognized stacks now produce changed-line coordinates from the parent-to-child diff, but source parsing and mutation execution still use the checked-out merge result. Upstream insertions or deletions in the same file can therefore target unrelated lines or omit changed executable lines, weakening both scope enforcement and mutation coverage. The baseline production invocation kept diff coordinates aligned with the merge-result source.

Security review details

Security Blast Radius

• inferred — The supported exposure is repository CI assurance for mutation-tested packages. A contributor can influence parent recognition through PR commit history and another open PR head. The evidence does not establish additional production credentials, tenant access, or an unauthorized merge.

Security Findings and Attack Paths

• inferred — A contributor can leave an oversized parent PR open and submit a small child on its exact head. Recognition excludes the parent's content from the child's cap even though that content remains in the child's merge ancestry. Parent approval or gate success is not checked by the resolver, and the queue path does not repeat measurement. External admission controls could constrain exploitation but were not available for verification.

Trust Boundaries and Controls

• observed — Full-SHA validation and exact sole-parent matching constrain revision selection; Git commands receive argument arrays rather than interpolated shell commands. API failure produces an empty map, preserving the broader measurement rather than selecting an arbitrary base. These controls authenticate commit relationships, not the parent's approval or successful enforcement.

Resilience and Maintainability Implications

• observed — Superseded runs are cancelled, both jobs have timeouts, and top-level gate errors set a failing exit status. These failure controls do not detect a successfully constructed but incomplete manifest, which can return normally without mutation execution.

Passed checks: 6 passed

Check name	Status	Explanation
Title check		The title "fix(ci): measure a stacked unit against its parent pull request" matches the main change. The PR adds stacked-unit detection to mutation-diff, so a stacked unit is measured against its open…
Description check		The description follows the repository template. It links the issue (Fixes #1923) and explains the approach: stacked-unit detection from the commit graph, the draft-parent refusal, and the fallback to…
Linked Issues check		[#1923] Whole-PR basis: merge base a15ba8c to reviewed head fc17bc4. The head moved one commit past the previous review (f133c58). The incremental diff was unavailable, so the workflow was read …
Out of Scope Changes check		[#1923] Each change beyond the base resolver supports stacked measurement or the summary that the issue requires: - Execution-tree alignment (alignExecutionTree). It moves the working tree to the u…
Persistence Integrity		The check fails only for a changed persistence path that can lose or corrupt state through an unawaited operation, a non-atomic write, missing rollback or partial-failure handling, or a persisted defa…
Lifecycle Resource Cleanup		The check fails only for a changed lifecycle path that leaks a listener, watcher, provider, timer, task, or other resource, or that duplicates work after cancellation, disposal, or restart. The change…

Full details: Title check

Explanation

The title "fix(ci): measure a stacked unit against its parent pull request" matches the main change. The PR adds stacked-unit detection to mutation-diff, so a stacked unit is measured against its open, non-draft parent PR head instead of main. The title is specific and follows a conventional commit style. It is 63 characters, within a short-title range, and uses plain wording.

Full details: Description check

Explanation

The description follows the repository template. It links the issue (Fixes #1923) and explains the approach: stacked-unit detection from the commit graph, the draft-parent refusal, and the fallback to the event base. It gives a test procedure with the command and test counts (52 passing) and states the new test cases. It also notes the maintainer review changes and the main refresh. Several template items are missing or unchecked: the Pre-Submission Checklist is not ticked, the Documentation Updates choice is not marked, and the Get in Touch section (Discord username) is absent. The description also refers to a 'Fixes' link while noting the issue should close only after both halves land, which conflicts with the template's closing keyword use. The core required content is present and clear.

Resolution

Optional completions: tick the Pre-Submission Checklist items that apply, mark the Documentation Updates choice, add a Discord username under Get in Touch, and confirm whether 'Fixes #1923' should be changed to a non-closing reference since the issue must stay open until #1925 lands.

Full details: Linked Issues check

Explanation

[#1923] Whole-PR basis: merge base a15ba8c to reviewed head fc17bc4. The head moved one commit past the previous review (f133c58). The incremental diff was unavailable, so the workflow was read at the head.

Coding requirements and code evidence:

1. Graph detection (#1923). resolveStackedUnitBase reads the PR head's parent with git rev-list --parents. It accepts the parent only when an open-PR map entry has that head SHA. No label or mapping file decides the result. Met.
2. Stacked unit measured against parent head (#1923). On a match, resolveCiInvocation returns baseSha = parent head and diffHead = PR head. alignExecutionTree moves the working tree to that head. Met, subject to item 6.
3. Non-stacked and multi-commit PRs keep the event base (#1923). A head with parents.length !== 1 returns the event base. A single parent that matches no open PR head also returns the event base. Met.
4. Unparsable map falls back (#1923). parseStackedMap returns [] on non-array parts, trailing garbage, or an unbalanced bracket. readStackedMap returns "" on read failure and emits a warning. Met.
5. Job summary records the base (#1923). formatSummary adds 'Stacked unit: measured against the head of parent PR #N'. The reproduction block adds git checkout <head> for a stacked unit. Met.
6. Narrowing beyond the issue text. The parent must be draft === false and must target the same base ref as the child. The issue does not state these limits. The PR description and code comments give the reason: a draft is not gated, and a feature-branch parent's delta would not reach main through this gate. The narrowing serves the issue's intent and is accepted.
7. Merge-queue enforcement (#1923 design). merge_group runs only the 'Record merge-queue enforcement' step. The PR corrects the issue's claim that the queue checks the cap. The requirement that enforcement stay unchanged holds. This is a factual correction, not a coding requirement.
8. Reviewer filtering (#1945, #1925) is excluded by the issue. No label code appears in the reviewed changes.

Workflow at head: the stacked_map job reads open PR heads with gh api --paginate. mutation-diff reads the map from ${RUNNER_TEMP} (commit fc17bc4). PR_HEAD_SHA resolves from the event or HEAD^2. The gate receives --pr-head, --pr-base-ref, and --stacked-map-file.

Unresolved uncertainty: the tail of main() was truncated in the diff output. The call that passes preserveBase to selectFromGit was not read. Its option and the resolveCiInvocation wiring support the claim. The test count (52 passing) is author-reported. The test file was not re-run.

Full details: Out of Scope Changes check

Explanation

[#1923] Each change beyond the base resolver supports stacked measurement or the summary that the issue requires:

• Execution-tree alignment (alignExecutionTree). It moves the working tree to the unit head for a stacked unit. Selection, source reads, and mutation then use one tree. This supports the unit-delta measurement.
• Dependency reinstall (dependencyManifestDrift, reinstallAlignedDependencies, summarizeReinstallFailure, reinstallFailureDetail). It runs only when the aligned tree changes a lockfile, workspace file, or manifest. This supports the aligned tree using its own toolchain.
• Map job and artifact (stacked_map, download-artifact, needs: stacked_map). The map carries only the fields the stacked check needs. The gate reads it as a file under RUNNER_TEMP. This is the map mechanism the issue specifies. The file path also avoids the argument size cap.
• Draft and base-ref fields in map entries. These are part of stacked detection, as described under the linked-issue assessment.
• Summary and reproduction changes (formatSummary, summarizeReinstallFailure). These report the base used, as the issue requires.
• Workflow permissions. mutation-diff sets contents: read and actions: write, which the artifact download and report upload need. The stacked_map job sets pull-requests: read and actions: write. The PR description says that job 'only reads open pull request heads'. It also has actions: write for artifact upload. The description understates that permission. This is a documentation point, not a scope failure.

No unrelated product code, webview code, or reviewer-label code appears in the reviewed changes. The author-reported red webview-visual job is a CI status. It is not a change in this diff, so it is not assessed here.

Full details: Persistence Integrity

Explanation

The check fails only for a changed persistence path that can lose or corrupt state through an unawaited operation, a non-atomic write, missing rollback or partial-failure handling, or a persisted default not reaching a consumer. The changed diff (git diff of base..head) touches three files: .github/workflows/mutation-testing.yml, scripts/stryker-diff.mjs, and scripts/stryker-diff.test.mjs. The only write-like paths added are CI scratch data: - printf '%s\n' "$STACKED_MAP" > stacked-map.json in the stacked_map job. This writes a transient file in a runner workspace. Nothing durable depends on it. - actions/upload-artifact (mutation-stacked-map, continue-on-error: true) and actions/download-artifact (continue-on-error: true, path ${{ runner.temp }}/mutation-stacked-map). The artifact is a per-run gate input, not stored state. - readStackedMap reads the map with fs.readFileSync. Failure returns empty input, and the gate uses the event base. Each failure path has an explicit fallback to the event base, which is stricter, not looser: - Upload failure does not block mutation-diff, because the upload step is continue-on-error: true. - A missing download leaves no file at the runner.temp path, so the gate uses the event base. - parseStackedMap rejects the whole input on any invalid part. The download targets runner.temp, outside the checkout, so a committed .mutation/stacked-map.json cannot supply the map. The .github review comments that flagged this were addressed in the current workflow. No changed path matches a listed failure condition. The stacked map is not persisted state, so loss or corruption of it does not corrupt durable data. Review threads on the workflow and script are not evidence of a persistence defect and were not relied on as such.

Full details: Lifecycle Resource Cleanup

Explanation

The check fails only for a changed lifecycle path that leaks a listener, watcher, provider, timer, task, or other resource, or that duplicates work after cancellation, disposal, or restart. The changed files are .github/workflows/mutation-testing.yml, scripts/stryker-diff.mjs, and scripts/stryker-diff.test.mjs. I found no such path. - No long-lived resources. The diff adds no setTimeout, setInterval, event listeners, watchers, child-process streams, or background tasks. The only process call is spawnSync("pnpm", ["install", "--frozen-lockfile"], ...) in reinstallAlignedDependencies (scripts/stryker-diff.mjs, around line 411). It is synchronous and returns its result, so it leaks no handle. - Temporary directories are cleaned up. Each new test that calls fs.mkdtempSync removes its directory in a finally block with fs.rmSync(..., { recursive: true, force: true }). Setup-failure paths remove the directory in a catch before rethrowing. The stacked-map- and reinstall-summary- cases follow the same pattern. - Job lifetimes are bounded. The new stacked_map job sets timeout-minutes: 10. The existing mutation-diff job sets timeout-minutes: 30. The needs: stacked_map dependency runs once per workflow run. The diff adds no retry loop or repeated work after cancellation. - Artifact download is one step. The artifact download step has continue-on-error: true. It runs once and leaves no resource open. - Code-level findings are not lifecycle leaks. The open review comments about pagination, the reinstall summary, and the spawnSync error detail concern data correctness and reporting. They do not concern resource retention. The spawnSync call has no explicit timeout. Its duration is bounded by the 30-minute job timeout, and it does not leak a resource. No changed path matches the stated failure condition.

Explanation

Summary: The prior security FAIL still applies to the current head. This PR adds an actions: write grant to the mutation-diff job, and that job runs pull-request-controlled code. Changed path (causality established): - The base workflow has only a top-level permissions: contents: read, and mutation-diff has no job-level permissions block. - At head, .github/workflows/mutation-testing.yml lines 77-79 add a job-level permissions: block with contents: read and actions: write to mutation-diff. This PR introduces that grant. - Lines 89-105 of the same job check out github.sha (the PR merge commit), run ./.github/actions/setup-node-pnpm from that checkout, and run pnpm test:mutation-ci. Lines 120-134 then run the gate, which can move the tree to the PR head (alignExecutionTree) and reinstall dependencies from that tree. - A same-repository PR can change .github/actions/setup-node-pnpm/action.yml or package scripts. Those files then execute with a write-capable GITHUB_TOKEN. Plausible scenario: A same-repository pull request edits a setup action or a package script. The modified code runs in mutation-diff with actions: write. It can then use that token against workflow runs or artifacts, for example to delete artifacts or cancel runs. Fork pull requests receive a read-only token by default, so the exposure applies mainly to same-repository branches. Why the grant is not needed: I found no gh api, curl, or other token-using API call in the mutation-diff steps. The job's artifact operations (upload and download) do not depend on the actions: write scope. The grant is therefore unused, and it only widens what PR-controlled code can do. Other paths checked: The stacked map is built in the stacked_map job, which checks out nothing and runs no PR code. parseStackedMap validates SHAs and rejects malformed input, and the map is downloaded under RUNNER_TEMP, outside the checkout. These paths do not introduce a new bypass. They do not clear the actions: write grant. Prior failure disposition: Not addressed. The workflow still grants actions: write at lines 77-79, and the job still runs PR code at lines 89-105. The prior failure's factual claims are true of the current head. Assessment: The changed grant gives PR-executed code a write-capable token. This matches the check's "executes unvalidated input" failure condition for this changed path.

Resolution

1. In .github/workflows/mutation-testing.yml, remove actions: write from the mutation-diff job (lines 77-79). Keep contents: read there. Confirm that no step in that job needs a GITHUB_TOKEN API scope. 2. If artifact download needs a token scope, use actions: read at most. Note that this still exposes that scope to PR-controlled steps. 3. If write access is truly required, move artifact publication or retrieval into a trusted job that does not check out or execute pull-request code. Do not give write scope to a job that runs setup-node-pnpm, pnpm test:mutation-ci, or the gate. 4. Update the permission comments at lines 75-76 so they match the final permissions.

Explanation

The aligned-tree reinstall path still lacks focused regression coverage. The prior FAIL stands. Changed behavior: - reinstallAlignedDependencies(repoRoot, driftedManifests) runs spawnSync("pnpm", ["install", "--frozen-lockfile"], ...). It throws when result.error is set or result.status !== 0 (scripts/stryker-diff.mjs:407-417). - main() calls it only when alignExecutionTree changed the tree and dependencyManifestDrift returned entries. On failure it calls summarizeReinstallFailure(invocation, driftedManifests, error) and rethrows (:1102-1113). Existing tests do not cover this behavior: - scripts/stryker-diff.test.mjs:580-611 calls summarizeReinstallFailure directly with fabricated input. It checks summary text only. - :613-626 calls reinstallFailureDetail directly with fabricated results. It checks detail formatting only. - :807-877 covers dependencyManifestDrift only. - The test file never imports or invokes reinstallAlignedDependencies. It spawns no pnpm process and uses no injected runner or fake executable. It does not run the ci path through main(). Missing evidence: - The success path (no throw; pnpm install --frozen-lockfile invoked with cwd: repoRoot). - The process-start error branch (result.error). - The non-zero exit branch (result.status !== 0). - The caller behavior: the failure summary is written, then the error propagates. These are affected error branches of a changed behavior. The helper tests do not exercise the branch condition or the throw or catch flow. The gap is not mechanical configuration. The reinstall decides whether the gate measures on a consistent toolchain. Prior-failure status: the test file has no new reinstall-path test since the prior run. The concern is not addressed.

Resolution

Add focused tests for the aligned reinstall path. Make the process runner injectable, or place a controlled fake pnpm executable on PATH and run the ci path through main(). Cover: 1. The exact pnpm install --frozen-lockfile invocation and its cwd. 2. Successful completion, with no throw. 3. A process-start error (result.error), with the error message in the thrown text. 4. A non-zero exit status, with the exit status in the thrown text. 5. Caller propagation: main() writes the aligned-tree failure summary, includes the stacked base, then rethrows.

Generate unit tests (beta)

• Create a new PR

• X
• Mastodon
• Reddit
• LinkedIn

Full review finished.

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Review rate limited.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 6 minutes.

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.