🤖 tests: declined review findings of the phase 4 call-record PRs

Read the review findings and mutation survivor lists in this issue, then inspect the referenced tests and source files, including src/node/utils/concurrency/fileLock.ts, src/node/utils/main/crossProcessLock.ts, and src/node/services/artifactVersionStore.ts. Start by checking the PR and test references listed for each finding; mark an item done only when its required test kills the survivor or addresses the finding. The issue assigns unresolved work across later phase 4 PRs and PR 17, so coordinate with that work before starting.

Tracks the review findings that the phase 4 call-record PRs (#7290, #7291) declined. Each one names the PR that must close it.

Refs #7255

• Owner: Phase 4 builder.
• Trigger: before phase 4 PR 16 merges, every item below is done or has a reasoned close.

Findings

1. Lease protocol order (#7290, PRRT_kwDOPxxmWM6vjS7Z, round 6). The record sorts its rows, so it cannot show if WorkspaceUseLeases.hold() inspects the mutation gate before it acquires the use-lease file. That order keeps a mutator and a turn from both going ahead.
   • Closed by: phase 4 PR 4 (SessionLocks service, the first PR that routes hold() through the new layer). Its first new commit adds an explicit order check in the turn scenario, like the removal check: use-lease file acquired before the gate is inspected.
   • Status: in #7345 (first commit e217b89ce5).
2. Digest names follow the production hash builders (#7290, PRRT_kwDOPxxmWM6vjS7d, round 6). The test names digests with historyWriteLockPath, refineApplyLockPath, workspaceMutationLockPath, workspaceUseLockDir and targetMutationLockFilePath. A move that changes one hash scheme changes both the recorded path and its name, so the fixture still matches.
   • Closed by: phase 4 PR 4 (the first move PR, before PRs 9, 12 and 13 touch the files of these builders). Its first new commit pins fixed digests for the fixed test IDs and keys, and checks them against the builders.
   • Status: in #7345 (first commit e217b89ce5).
3. Seed HistoryService in beforeEach (#7290, PRRT_kwDOPxxmWM6vgtmT, P1, round 4). The refine scenario appends its rows inside the scenario, before its try, so a failed append leaks the temp root of createTestHistoryService(). Test hygiene, not a gap in the record: an append failure fails the test at once.
   • Closed by: any later phase 4 PR that edits the scenario harness, or a reasoned close.
   • Status: done in #7493 (merged as e803893928): oneRefineApply seeds inside its try.
4. Windows path separators (#7290, PRRT_kwDOPxxmWM6vehmU, P2, round 2). The temp-root masks expect forward slashes. No CI lane runs this test on Windows (the test-windows job runs a fixed smoke list).
   • Closed by: a reasoned close, unless a Windows lane starts running the file.
   • Status: closed with a reason (#7493 follow-up comment): no CI lane runs this file on Windows. The test-windows job runs a fixed smoke list. Reopen it if a Windows lane starts running the file.

Mutation survivors on lines that PR 4 does not change

Phase 4 PR 4 (#7345) found these survivors in its mutation run (no test-file cap, lane with the call-record and contract tests, head 907f6b054e). Each one sits on a line that PR 4 does not change. Line numbers are on the PR 4 head.

Rule for every later phase 4 PR: before a PR changes or moves a line below, it kills that survivor first. The test goes in a commit at the start of the PR, and it must pass on the code before the change. PR 17 is too late for such a line, because its tests would describe the code after the change. Each PR checks this list, and marks an item done with the PR and the test that kills it. The items that no PR touches go to PR 17.

M1. fileLock.ts:260 in acquireProcessFileLock: 0 -> 1

• File: src/node/utils/concurrency/fileLock.ts:260
• Line: assert(lockPath.length > 0, "acquireProcessFileLock requires a lock path");
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: closed by #7481 (PR 17) without a test: the formal check classes it as a low-value assert (#7255).

M2. fileLock.ts:260 in acquireProcessFileLock: > -> >=

• File: src/node/utils/concurrency/fileLock.ts:260
• Line: assert(lockPath.length > 0, "acquireProcessFileLock requires a lock path");
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: closed by #7481 (PR 17) without a test: the formal check classes it as a low-value assert (#7255).

M3. fileLock.ts:261 in acquireProcessFileLock: > -> >=

• File: src/node/utils/concurrency/fileLock.ts:261
• Line: assert(timeoutMs > 0, "acquireProcessFileLock timeoutMs must be positive");
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: closed by #7481 (PR 17) without a test: the formal check classes it as a low-value assert (#7255).

M4. fileLock.ts:265 in acquireProcessFileLock: false -> true

• File: src/node/utils/concurrency/fileLock.ts:265
• Line: let acquired = false;
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: closed by #7481 (PR 17) without a test: the formal check classes it as equivalent (#7255).

M5. fileLock.ts:278 in acquireProcessFileLock: ?? -> ||

• File: src/node/utils/concurrency/fileLock.ts:278
• Line: const renewIntervalMs = options.renewIntervalMs ?? FILE_LOCK_RENEW_INTERVAL_MS;
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: closed by #7481 (PR 17) without a test: the formal check classes it as a low-value assert (#7255).

M6. fileLock.ts:279 in acquireProcessFileLock: 0 -> 1

• File: src/node/utils/concurrency/fileLock.ts:279
• Line: assert(renewIntervalMs > 0, "renewIntervalMs must be positive");
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: closed by #7481 (PR 17) without a test: the formal check classes it as a low-value assert (#7255).

M7. fileLock.ts:279 in acquireProcessFileLock: > -> >=

• File: src/node/utils/concurrency/fileLock.ts:279
• Line: assert(renewIntervalMs > 0, "renewIntervalMs must be positive");
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: closed by #7481 (PR 17) without a test: the formal check classes it as a low-value assert (#7255).

M8. fileLock.ts:296 in acquireProcessFileLock: !== -> ===

• File: src/node/utils/concurrency/fileLock.ts:296
• Line: if (renewInFlight !== null) await renewInFlight;
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: killed by a test in #7481 (PR 17), merged.

M9. fileLock.ts:306 in acquireProcessFileLock: 0 -> 1

• File: src/node/utils/concurrency/fileLock.ts:306
• Line: const blocker = await reclaimStaleFileLock(lockPath, label, 0, options.testOnlyReclaimSeam);
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: killed by a test in #7481 (PR 17), merged.

M10. fileLock.ts:307 in acquireProcessFileLock: >= -> >

• File: src/node/utils/concurrency/fileLock.ts:307
• Line: if (Date.now() >= deadline) {
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: closed by #7481 (PR 17) without a test: the formal check classes it as equivalent (#7255).

M11. fileLock.ts:311 in acquireProcessFileLock: * -> /

• File: src/node/utils/concurrency/fileLock.ts:311
• Line: setTimeout(resolve, FILE_LOCK_RETRY_MS + Math.random() * FILE_LOCK_RETRY_MS)
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: closed by #7481 (PR 17) without a test: the formal check classes it as equivalent (#7255).

M12. fileLock.ts:311 in acquireProcessFileLock: + -> -

• File: src/node/utils/concurrency/fileLock.ts:311
• Line: setTimeout(resolve, FILE_LOCK_RETRY_MS + Math.random() * FILE_LOCK_RETRY_MS)
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: closed by #7481 (PR 17) without a test: the formal check classes it as equivalent (#7255).

M13. crossProcessLock.ts:130 in parseHolder: || -> && (site 1 of 2 on the line, left to right)

• File: src/node/utils/main/crossProcessLock.ts:130
• Line: if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) {
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: killed by a test in #7481 (PR 17), merged.

M14. crossProcessLock.ts:130 in parseHolder: || -> && (site 2 of 2 on the line, left to right)

• File: src/node/utils/main/crossProcessLock.ts:130
• Line: if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) {
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: closed by #7481 (PR 17) without a test: the formal check classes it as equivalent (#7255).

M15. crossProcessLock.ts:140 in parseHolder: || -> &&

• File: src/node/utils/main/crossProcessLock.ts:140
• Line: typeof pid !== "number" ||
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: killed by a test in #7481 (PR 17), merged.

M16. crossProcessLock.ts:141 in parseHolder: || -> &&

• File: src/node/utils/main/crossProcessLock.ts:141
• Line: !Number.isInteger(pid) ||
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: killed by a test in #7481 (PR 17), merged.

M17. crossProcessLock.ts:142 in parseHolder: 0 -> 1

• File: src/node/utils/main/crossProcessLock.ts:142
• Line: pid <= 0 ||
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: killed by a test in #7481 (PR 17), merged.

M18. crossProcessLock.ts:142 in parseHolder: <= -> <

• File: src/node/utils/main/crossProcessLock.ts:142
• Line: pid <= 0 ||
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: killed by a test in #7481 (PR 17), merged.

M19. crossProcessLock.ts:142 in parseHolder: || -> &&

• File: src/node/utils/main/crossProcessLock.ts:142
• Line: pid <= 0 ||
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: killed by a test in #7481 (PR 17), merged.

M20. crossProcessLock.ts:143 in parseHolder: || -> &&

• File: src/node/utils/main/crossProcessLock.ts:143
• Line: typeof token !== "string" ||
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: killed by a test in #7481 (PR 17), merged.

M21. crossProcessLock.ts:144 in parseHolder: 0 -> 1

• File: src/node/utils/main/crossProcessLock.ts:144
• Line: token.length === 0 ||
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: killed by a test in #7481 (PR 17), merged.

M22. crossProcessLock.ts:144 in parseHolder: || -> &&

• File: src/node/utils/main/crossProcessLock.ts:144
• Line: token.length === 0 ||
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: killed by a test in #7481 (PR 17), merged.

M23. crossProcessLock.ts:145 in parseHolder: || -> &&

• File: src/node/utils/main/crossProcessLock.ts:145
• Line: typeof acquiredAt !== "number" ||
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: killed by a test in #7481 (PR 17), merged.

M24. crossProcessLock.ts:226 in describeHolder: ?? -> ||

• File: src/node/utils/main/crossProcessLock.ts:226
• Line: ``pid ${holder.pid} (started ${holder.identity?.birth ?? "at an unknown time"} +
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: closed by #7481 (PR 17) without a test: the formal check classes it as equivalent (#7255).

M25. crossProcessLock.ts:499 in acquireCrossProcessLock: || -> &&

• File: src/node/utils/main/crossProcessLock.ts:499
• Line: if (stopped || inFlight !== undefined) {
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: killed by a test in #7481 (PR 17), merged.

M26. crossProcessLock.ts:504 in acquireCrossProcessLock: && -> ||

• File: src/node/utils/main/crossProcessLock.ts:504
• Line: if (current.kind === "holder" && current.key === token) {
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: killed by a test in #7481 (PR 17), merged.

M27. crossProcessLock.ts:517 in acquireCrossProcessLock: true -> false

• File: src/node/utils/main/crossProcessLock.ts:517
• Line: stopped = true;
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: killed by a test in #7481 (PR 17), merged.

M28. crossProcessLock.ts:528 in acquireCrossProcessLock: 0 -> 1

• File: src/node/utils/main/crossProcessLock.ts:528
• Line: for (let attempt = 0; attempt < 40; attempt++) {
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: killed by a test in #7481 (PR 17), merged.

M29. crossProcessLock.ts:528 in acquireCrossProcessLock: < -> <=

• File: src/node/utils/main/crossProcessLock.ts:528
• Line: for (let attempt = 0; attempt < 40; attempt++) {
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: killed by a test in #7481 (PR 17), merged.

M30. crossProcessLock.ts:543 in acquireCrossProcessLock: true -> false

• File: src/node/utils/main/crossProcessLock.ts:543
• Line: await fsPromises.rm(lockPath, { force: true });
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: closed by #7481 (PR 17) without a test: the formal check classes it as equivalent (#7255).

M31. crossProcessLock.ts:569 in acquireCrossProcessLock: false -> true

• File: src/node/utils/main/crossProcessLock.ts:569
• Line: let owned = false;
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: killed by a test in #7481 (PR 17), merged.

M32. crossProcessLock.ts:585 in acquireCrossProcessLock: >= -> >

• File: src/node/utils/main/crossProcessLock.ts:585
• Line: if (Date.now() >= deadline) {
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: killed by a test in #7481 (PR 17), merged.

Mutation survivors of PR 8 (#7354) on lines that it does not change

The same rule as above applies. Line numbers are on the #7354 head 8d7daf617a.

M33. artifactVersionStore.ts:152 in recordArtifactVersion: ?? -> ||

• File: src/node/services/artifactVersionStore.ts:152
• Line: const nowMs = params.nowMs ?? Date.now();
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

M34. artifactVersionStore.ts:166 in recordArtifactVersion: 0 -> 1

• File: src/node/services/artifactVersionStore.ts:166
• Line: versions: [...index.versions.slice(0, -1), refreshed],
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

M35. artifactVersionStore.ts:166 in recordArtifactVersion: 1 -> 0

• File: src/node/services/artifactVersionStore.ts:166
• Line: versions: [...index.versions.slice(0, -1), refreshed],
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

M36. artifactVersionStore.ts:185 in recordArtifactVersion: != -> ==

• File: src/node/services/artifactVersionStore.ts:185
• Line: ...(params.kind != null ? { kind: params.kind } : {}),
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

M37. artifactVersionStore.ts:198 in recordArtifactVersion: true -> false

• File: src/node/services/artifactVersionStore.ts:198
• Line: return { artifactId, version: entry, created: true, pin: nextPin };
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

M38. artifactVersionStore.ts:224 in acquireIndexFileLock: ?. -> .

• File: src/node/services/artifactVersionStore.ts:224
• Line: if (abortSignal?.aborted) throw new Error(ARTIFACT_PUBLISH_INTERRUPTED);
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

M39. WorkflowRunStore.ts:228 in constructor: 0 -> 1

• File: src/node/services/workflows/WorkflowRunStore.ts:228
• Line: assert(options.sessionDir.length > 0, "WorkflowRunStore: sessionDir is required");
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

M40. WorkflowRunStore.ts:228 in constructor: > -> >=

• File: src/node/services/workflows/WorkflowRunStore.ts:228
• Line: assert(options.sessionDir.length > 0, "WorkflowRunStore: sessionDir is required");
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

M41. WorkflowRunStore.ts:230 in constructor: ?? -> ||

• File: src/node/services/workflows/WorkflowRunStore.ts:230
• Line: this.staleLeaseMs = options.staleLeaseMs ?? 30_000;
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

M42. WorkflowRunStore.ts:232 in constructor: 0 -> 1

• File: src/node/services/workflows/WorkflowRunStore.ts:232
• Line: options.mutationLockWaitTimeoutMs == null || options.mutationLockWaitTimeoutMs > 0,
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

M43. WorkflowRunStore.ts:232 in constructor: > -> >=

• File: src/node/services/workflows/WorkflowRunStore.ts:232
• Line: options.mutationLockWaitTimeoutMs == null || options.mutationLockWaitTimeoutMs > 0,
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

M44. WorkflowRunStore.ts:1110 in acquireLease: 0 -> 1

• File: src/node/services/workflows/WorkflowRunStore.ts:1110
• Line: assert(ownerId.length > 0, "WorkflowRunStore.acquireLease: ownerId is required");
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

M45. WorkflowRunStore.ts:1110 in acquireLease: > -> >=

• File: src/node/services/workflows/WorkflowRunStore.ts:1110
• Line: assert(ownerId.length > 0, "WorkflowRunStore.acquireLease: ownerId is required");
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

M46. WorkflowRunStore.ts:1119 in acquireLease: <= -> <

• File: src/node/services/workflows/WorkflowRunStore.ts:1119
• Line: if (existing != null && nowMs - existing.acquiredAtMs <= this.staleLeaseMs) {
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

M47. WorkflowRunStore.ts:1778 in withWorkflowFileLock: 0 -> 1

• File: src/node/services/workflows/WorkflowRunStore.ts:1778
• Line: Math.max(0, deadline - Date.now()),
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

M48. journal.ts:92 in constructor: 0 -> 1

• File: src/node/utils/journal/journal.ts:92
• Line: assert(options.filePath.length > 0, "Journal requires a file path");
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

M49. journal.ts:92 in constructor: > -> >=

• File: src/node/utils/journal/journal.ts:92
• Line: assert(options.filePath.length > 0, "Journal requires a file path");
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

M50. journal.ts:98 in constructor: ?? -> ||

• File: src/node/utils/journal/journal.ts:98
• Line: this.appendLockTimeoutMs = options.appendLockTimeoutMs ?? APPEND_LOCK_TIMEOUT_MS;
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

M51. journal.ts:99 in constructor: 0 -> 1

• File: src/node/utils/journal/journal.ts:99
• Line: assert(this.appendLockTimeoutMs > 0, "Journal appendLockTimeoutMs must be positive");
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

M52. journal.ts:99 in constructor: > -> >=

• File: src/node/utils/journal/journal.ts:99
• Line: assert(this.appendLockTimeoutMs > 0, "Journal appendLockTimeoutMs must be positive");
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

M53. journal.ts:152 in append: + -> -

• File: src/node/utils/journal/journal.ts:152
• Line: const postAppendFileSize = fileSize + Buffer.byteLength(payload, "utf-8");
• Closed by: PR 17, or the first phase 4 PR that changes or moves this line.
• Status: moved to #7475 (not lock code, on a line that no phase 4 PR changed).

---

Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high • Cost: $70.87