Code Quality: PR #391 #1136
codeql
on: dynamic
Matrix: analyze
Annotations
1 error and 5 warnings
|
Discovered test total bumped from 52 to 57 without matching item list change:
src/snapshot-tests/__fixtures__/json/device/test--failure.json#L34
The hunk increases `tests.discovered.total` from 52 to 57, but the surrounding context shows the same item list as before (no new test entries added in this hunk). If the items array was not also extended to 57 entries, this fixture update violates the guardrail that fixture updates must map to intentional, consistent behavior changes and that JSON fixtures preserve stable structured output envelopes. This risks the fixture being updated only to make tests pass rather than reflecting a real behavior change.
|
|
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.
|
|
TOCTOU race lets recovery destroy a freshly-acquired lock when prior owner file was missing:
src/utils/fs-lock.ts#L116
In shouldRecoverLockDir, when readLockOwner returns null and the directory is older than the lease (lines 85-88), tryRecoverExpiredLockDir proceeds to quarantine and unconditionally remove the lock dir without re-validating after the rename. Between the readLockOwner/isDirectoryOlderThan checks and the fs.rename in quarantineLockDir, another process can legitimately create owner.json inside that directory (e.g., createLock racing on the same path). Because the recovery.owner === null branch skips the fsLockOwnersEqual post-rename verification used in the non-null branch (lines 116-122), the racing process's valid lock is silently destroyed at line 124, leading to lost-lock and potential double-acquisition by two processes that each believe they own the lock.
|
|
[VLD-QP7] TOCTOU race lets recovery destroy a freshly-acquired lock when prior owner file was missing (additional location):
src/daemon/socket-path.ts#L68
In shouldRecoverLockDir, when readLockOwner returns null and the directory is older than the lease (lines 85-88), tryRecoverExpiredLockDir proceeds to quarantine and unconditionally remove the lock dir without re-validating after the rename. Between the readLockOwner/isDirectoryOlderThan checks and the fs.rename in quarantineLockDir, another process can legitimately create owner.json inside that directory (e.g., createLock racing on the same path). Because the recovery.owner === null branch skips the fsLockOwnersEqual post-rename verification used in the non-null branch (lines 116-122), the racing process's valid lock is silently destroyed at line 124, leading to lost-lock and potential double-acquisition by two processes that each believe they own the lock.
|
|
Scheduled sweep marks scope as running before cooldown check completes, allowing duplicate scheduling on cooldown skip:
src/utils/workspace-filesystem-lifecycle.ts#L401
scheduleWorkspaceFilesystemLifecycleSweep adds scheduleKey to runningScheduledSweeps and sets a timer, but if the timer's sweep returns skippedByCooldown or skippedByLock, lastScheduledAtByScope/lastScheduledAtByPreKey are NOT updated. Subsequent calls will pass the cooldown gate (since lastAt remains stale/undefined) and re-schedule repeatedly. Combined with the unref'd timer, this can cause busy rescheduling under contention without progress, and the pre-key cooldown protection is bypassed.
|
|
Startup registry lock leaks if checkExistingDaemon rejects after lock acquisition:
src/daemon.ts#L201
After `acquireDaemonRegistryMutationLock` succeeds at line 188, the second `await checkExistingDaemon(socketPath)` at line 201 runs outside any try/catch. The protective `try` block does not start until line 210. If `checkExistingDaemon` rejects (e.g., transient socket/permission error), control bypasses both `releaseStartupRegistryLock()` and the catch on line 511, propagating to the top-level `main().catch` handler which only flushes Sentry and exits — leaving the registry mutation lock file/handle orphaned for this workspace. Subsequent daemon start attempts would then fail to acquire the lock until the OS reclaims the handle.
|