Skip to content

Code Quality: PR #391 #1110

Code Quality: PR #391

Code Quality: PR #391 #1110

Triggered via dynamic May 4, 2026 15:29
Status Success
Total duration 1m 22s
Artifacts

codeql

on: dynamic
Matrix: analyze
Fit to window
Zoom out
Zoom in

Annotations

12 warnings
Analyze (python)
Starting April 2026, the CodeQL Action will skip computing file coverage information on pull requests to improve analysis performance. File coverage information will still be computed on non-PR analyses. To opt out of this change, create a custom repository property with the name `github-codeql-file-coverage-on-prs` and the type "True/false", then set this property to `true` in the repository's settings.
Analyze (javascript-typescript)
Starting April 2026, the CodeQL Action will skip computing file coverage information on pull requests to improve analysis performance. File coverage information will still be computed on non-PR analyses. To opt out of this change, create a custom repository property with the name `github-codeql-file-coverage-on-prs` and the type "True/false", then set this property to `true` in the repository's settings.
Volatile test progress lines added to fixture without normalization: src/snapshot-tests/__fixtures__/cli/swift-package/test--failure.txt#L8
The fixture now includes seven 'Running tests (N completed, M failure, ...)' progress lines. These lines depend on test execution timing/ordering and are not normalized like other volatile values (timestamps, PIDs, hashes). Per the skill guardrails, volatile values should be normalized in code rather than baked into fixtures, otherwise the snapshot will be flaky across runs. The Derived Data path change is expected (workspace-scoped path), but these progress lines are a separate concern.
Test failure count jumps from 2 to 4 alongside path-only refactor: src/snapshot-tests/__fixtures__/mcp/simulator/test--failure.txt#L9
This fixture update mixes two unrelated changes: the workspace-scoped Derived Data path rename, and a substantive change in test discovery/failure counts (52→57 discovered, 2→4 failures, with two new failing test entries). The PR description scopes fixture churn to 'new workspace-scoped paths', but new failing tests and discovered counts indicate a behavior change in the underlying test suite or runner output that is not explained. Per the skill guardrail 'Fixture updates map to intentional behavior changes' and 'Do not update fixtures only to make tests pass', the reviewer cannot verify whether these added failures are intentional or accidental snapshot drift.
forceStopDaemon silently leaves stale socket and registry when SIGTERM target hasn't exited within 500ms: src/cli/daemon-control.ts#L53
After sending SIGTERM and waiting 500ms, forceStopDaemon calls cleanupWorkspaceDaemonFiles with pid set but without allowLiveOwner=true. In canRemoveRegistryEntry, when allowLiveOwner is not true the function returns !isPidAlive(entry.pid); if the daemon hasn't yet exited (500ms is short for a graceful SIGTERM shutdown), removeRegistryAtPathIfOwned returns null and neither the registry entry nor the socket is removed. The previous implementation unconditionally called removeStaleSocket(socketPath), so this is a behavioral regression: ensureDaemonRunning will then proceed to startDaemonBackground while the old socket file still exists, which can cause the new daemon's bind() to fail with EADDRINUSE and leave the workspace unable to start a daemon.
[LX2-SSG] forceStopDaemon silently leaves stale socket and registry when SIGTERM target hasn't exited within 500ms (additional location): src/daemon/daemon-registry.ts#L332
After sending SIGTERM and waiting 500ms, forceStopDaemon calls cleanupWorkspaceDaemonFiles with pid set but without allowLiveOwner=true. In canRemoveRegistryEntry, when allowLiveOwner is not true the function returns !isPidAlive(entry.pid); if the daemon hasn't yet exited (500ms is short for a graceful SIGTERM shutdown), removeRegistryAtPathIfOwned returns null and neither the registry entry nor the socket is removed. The previous implementation unconditionally called removeStaleSocket(socketPath), so this is a behavioral regression: ensureDaemonRunning will then proceed to startDaemonBackground while the old socket file still exists, which can cause the new daemon's bind() to fail with EADDRINUSE and leave the workspace unable to start a daemon.
Forced-shutdown timer can hang indefinitely waiting on async cleanup: src/daemon.ts#L344
The 5s forced-shutdown timer previously called the synchronous cleanupWorkspaceDaemonFiles and then exited. It now awaits the async cleanupOwnedWorkspaceFilesystemArtifacts via .finally() with no timeout. If that promise hangs (e.g., a stuck filesystem lock acquired in fs-lock-shared / workspace-filesystem-lifecycle), the daemon will never exit, defeating the purpose of the forced-shutdown path and causing process leaks across daemon restarts.
[FX3-GFT] Forced-shutdown timer can hang indefinitely waiting on async cleanup (additional location): src/utils/workspace-filesystem-lifecycle.ts#L403
The 5s forced-shutdown timer previously called the synchronous cleanupWorkspaceDaemonFiles and then exited. It now awaits the async cleanupOwnedWorkspaceFilesystemArtifacts via .finally() with no timeout. If that promise hangs (e.g., a stuck filesystem lock acquired in fs-lock-shared / workspace-filesystem-lifecycle), the daemon will never exit, defeating the purpose of the forced-shutdown path and causing process leaks across daemon restarts.
Daemon socket directory moved to shared tmpdir without per-user isolation enables symlink/TOCTOU attacks: src/daemon/socket-path.ts#L21
`daemonRunDir()` now returns `os.tmpdir()` (typically `/tmp` on Linux, a world-writable shared directory), and `daemonDirForWorkspaceKey` builds a predictable path `xcodebuildmcp-<12-hex>` under it. Because `workspaceKeyForRoot` is a deterministic SHA-256 of the workspace root path, any local user can predict the directory name and pre-create it (or create it as a symlink) before the daemon starts. The directory creator in `ensureSocketDir` uses `existsSync`+`mkdirSync` (TOCTOU) and will not enforce ownership/mode if the directory already exists, which can lead to the daemon binding its UNIX socket inside an attacker-controlled directory and `removeStaleSocket` unlinking attacker-chosen files. Previously the directory lived under `~/.xcodebuildmcp` which was not shared between users.
[V8F-EGD] Daemon socket directory moved to shared tmpdir without per-user isolation enables symlink/TOCTOU attacks (additional location): src/daemon/socket-path.ts#L16
`daemonRunDir()` now returns `os.tmpdir()` (typically `/tmp` on Linux, a world-writable shared directory), and `daemonDirForWorkspaceKey` builds a predictable path `xcodebuildmcp-<12-hex>` under it. Because `workspaceKeyForRoot` is a deterministic SHA-256 of the workspace root path, any local user can predict the directory name and pre-create it (or create it as a symlink) before the daemon starts. The directory creator in `ensureSocketDir` uses `existsSync`+`mkdirSync` (TOCTOU) and will not enforce ownership/mode if the directory already exists, which can lead to the daemon binding its UNIX socket inside an attacker-controlled directory and `removeStaleSocket` unlinking attacker-chosen files. Previously the directory lived under `~/.xcodebuildmcp` which was not shared between users.
PID reuse can prevent recovery of expired locks indefinitely: src/utils/fs-lock.ts#L74
shouldRecoverLockDir refuses to recover an expired lock if `isPidAlive(staleOwner.pid)` returns true. On Linux/macOS, PIDs are recycled, so an unrelated long-running process inheriting the recorded PID will keep the expired lock un-recoverable forever. Every future acquirer for that workspace will fail to acquire the lock, producing a denial-of-service for cleanup/daemon operations. Consider also comparing process start time or the owner token before honoring `isPidAlive`.
[67B-Y65] PID reuse can prevent recovery of expired locks indefinitely (additional location): src/utils/fs-lock.ts#L195
shouldRecoverLockDir refuses to recover an expired lock if `isPidAlive(staleOwner.pid)` returns true. On Linux/macOS, PIDs are recycled, so an unrelated long-running process inheriting the recorded PID will keep the expired lock un-recoverable forever. Every future acquirer for that workspace will fail to acquire the lock, producing a denial-of-service for cleanup/daemon operations. Consider also comparing process start time or the owner token before honoring `isPidAlive`.