chore: sync core lib and CLAUDE.md from agent-core#29
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces security hardening for binary downloads, including SHA-256 checksum verification and strict archive entry validation to prevent path traversal. It also adds typed wrappers for repository intelligence queries and enhances task state management with optimistic locking to handle concurrent updates. Feedback suggests addressing potential filename escaping issues in query arguments and replacing a busy-wait loop with a non-blocking sleep mechanism to improve performance during update retries.
| if (!files.every(f => typeof f === 'string')) { | ||
| throw new TypeError('diffRisk: all entries in files must be strings'); | ||
| } | ||
| const joined = files.join(','); |
There was a problem hiding this comment.
Joining filenames with a comma without escaping can lead to incorrect behavior if any filename contains a comma. If the agent-analyzer binary expects a comma-separated list, consider validating that the input filenames do not contain commas, or check if the binary supports an alternative way to pass file lists (e.g., via multiple flags or a file).
| const jitter = Math.floor(Math.random() * 20); | ||
| const start = Date.now(); | ||
| while (Date.now() - start < jitter) { /* busy-wait for short jitter */ } |
There was a problem hiding this comment.
This busy-wait loop blocks the Node.js event loop, which can degrade performance and responsiveness, even for short durations. Since sleepForRetry is already imported and used elsewhere in this file (e.g., in updateFlow), it should be used here as well for a more efficient wait mechanism.
| const jitter = Math.floor(Math.random() * 20); | |
| const start = Date.now(); | |
| while (Date.now() - start < jitter) { /* busy-wait for short jitter */ } | |
| const jitter = Math.floor(Math.random() * 20); | |
| sleepForRetry(jitter); |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 79ca49a. Configure here.
| // Another writer won — retry with jitter | ||
| const jitter = Math.floor(Math.random() * 20); | ||
| const start = Date.now(); | ||
| while (Date.now() - start < jitter) { /* busy-wait for short jitter */ } |
There was a problem hiding this comment.
Busy-wait loop blocks event loop instead of using sleepForRetry
Low Severity
The updateTasks function uses a synchronous busy-wait loop for retry jitter, which blocks the event loop and consumes CPU for up to 20ms per retry. The sleepForRetry utility, which uses Atomics.wait for non-blocking sleep, is already imported and used by updateFlow in the same file.
Reviewed by Cursor Bugbot for commit 79ca49a. Configure here.
| return updateTasks(tasks => { | ||
| tasks.tasks = tasks.tasks.filter(t => t.id !== taskId); | ||
| return tasks; | ||
| }, projectPath); |
There was a problem hiding this comment.
Concurrent task writes can silently lose claimed tasks
Medium Severity
setActiveTask and clearActiveTask perform a full read-modify-write of tasks.json without using the new updateTasks optimistic locking, while the newly introduced claimTask/releaseTask do use it. If setActiveTask reads the file, then claimTask writes a new entry to tasks[], then setActiveTask writes back its stale snapshot, the claimed task entry is silently lost. Since setActiveTask has no retry logic, this data loss is permanent.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 79ca49a. Configure here.
| } catch (e) { | ||
| console.error(`[CRITICAL] Corrupted tasks.json at ${tasksPath}: ${e.message}`); | ||
| return { active: null }; | ||
| throw new Error(`Corrupted tasks.json at ${tasksPath}: ${e.message}`); |
There was a problem hiding this comment.
Throwing readTasks breaks clearActiveTask in workflow completions
Medium Severity
readTasks changed from catching JSON parse errors (returning a default) to throwing. setActiveTask, clearActiveTask, and hasActiveTask call readTasks without try-catch. In completeWorkflow and abortWorkflow, the flow is already updated via updateFlow before clearActiveTask is called — so a corrupted tasks.json now causes an unhandled throw after the flow state has been committed, leaving flow.json and tasks.json in an inconsistent state.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 79ca49a. Configure here.
| return { active: null }; | ||
| throw new Error(`Corrupted tasks.json at ${tasksPath}: ${e.message}`); | ||
| } | ||
| return normalizeTasksData(data); |
There was a problem hiding this comment.
normalizeTasksData crashes on null from valid JSON parse
Low Severity
normalizeTasksData is called outside the try-catch in readTasks. If tasks.json contains the valid JSON literal null, JSON.parse succeeds and returns null, then Object.prototype.hasOwnProperty.call(null, 'active') throws an unhandled TypeError. The old code's broader try-catch caught this same crash and returned a safe default; the new narrower try-catch only covers JSON.parse, so the TypeError escapes with an unhelpful message.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 79ca49a. Configure here.
1 participant
Automated sync of lib/ and CLAUDE.md from agent-core.
Note
Medium Risk
Touches security-sensitive runtime binary download/extraction and changes
tasks.jsonread/write semantics (now throws on corruption and adds optimistic locking), which could affect install reliability and workflow state under concurrency.Overview
Hardens runtime installation of the
agent-analyzerbinary. Downloads are now SHA-256 verified via a required<asset>.sha256sidecar (with an explicitskipChecksumescape hatch), and archives are extracted into an isolated scratch dir with path-traversal/absolute-path/UNC checks; only the expected binary is then copied into place (Windows zip extraction is replaced with a PowerShell-Filehelper that validates entries and is safe with spaces/brackets).Adds coverage and new repo-intel/query API surface. Introduces
lib/binary/index.test.jsto validate checksum parsing, entry validation, scratch cleanup, and Windows script structure, and addslib/repo-intel/queries.jsas JSON-parsing wrappers aroundagent-analyzer repo-intel query ...with a dedicatedRepoIntelMissingError.Updates workflow tasks state format and concurrency behavior.
tasks.jsonnow uses a canonical schema ({ active, tasks, _version }), throws on corrupted JSON (instead of silently defaulting), and adds_writerId-stamped writes plus newupdateTasks/claimTask/releaseTaskhelpers with optimistic retry.Reviewed by Cursor Bugbot for commit 79ca49a. Configure here.