fix(agent-memory-sync): reject cron step<=0 to stop scheduler infinite-loop hang (+ coverage)#61
Merged
Conversation
parsePart validated a step token against the field range via parseInteger(stepToken, min, max). For fields with min=0 (minute/hour/dayOfWeek) step=0 was accepted, and fillRange(result, min, max, 0) then looped forever (cursor += 0) — a typo'd operator cron string like '*/0 * * * *' hung the sync daemon unbounded. Validate the step independently as an integer >= 1 (new parseStep), with no upper bound: a step larger than the range simply collapses to the start value in a single terminating fillRange iteration. Adds tests for the rejected '*/0', the now-valid range-independent '*/90', and pins the existing throw for the unsupported lone-value 'a/n' step form. Refs: agent-memory task 124c89e0 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The embed-provider suite proved an abort throws but never that the DEFAULT_TIMEOUT_MS (5000) is the value handed to AbortSignal.timeout and that its signal reaches fetch. Spy over the real AbortSignal.timeout (call-through) to pin the 5000ms default and the explicit-override branch of `opts.timeoutMs ?? DEFAULT_TIMEOUT_MS`. Refs: agent-memory task 124c89e0 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The generate suite replaced the action with a spy, so the real body (scan → extract → generate → format → write/print + catch→exit) had ~0% coverage. Add integration tests driving the real registerGenerateCommand action against today-dated temp-dir fixtures: markdown --output write, --json --output write (also parsing non-default --days/--max), stdout print when --output is omitted, and the catch/process.exit(1) branch via an unwritable --output path. Stubs console.log/error and process.exit with save/restore. Tightens the tautological description assertion to the exact string. Refs: agent-memory task 124c89e0 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
LanNguyenSi
added
review:tests-pass
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes a pre-existing infinite-loop hang in the agent-memory-sync cron scheduler, and closes two residual test-coverage gaps. Follow-up to the coverage PR #60.
The bug
parsePartvalidated a cron step token against the field range viaparseInteger(stepToken, min, max). For min=0 fields (minute, hour, dayOfWeek),step=0was accepted, andfillRange(result, min, max, 0)then looped forever (cursor += 0). A typo'd cron string like*/0 * * * *hung the sync daemon unbounded. Confirmed:validateCronExpression('*/0 * * * *')timed out at 3s (exit 124).The fix
A new
parseStepvalidates the step as an integer >= 1, independent of the field's min/max (per the task spec). There is no upper bound: a step larger than the range collapses to the start value in a single terminatingfillRangeiteration.Tests
*/0now throws instead of hanging, the range-independent*/90is valid, and the unsupported lone-value5/2form is pinned as throwing. 49 tests pass.AbortSignal.timeoutand that its signal reaches fetch, plus the explicit-override branch. 233 tests pass.All three package coverage gates pass. Reviewed adversarially: every mutation was caught.
Follow-up
The lone-value step form
a/n(e.g.5/2) throws with a confusing 'undefined ... outside range' message, where standard cron reads it asa-max/n. Tracked as a separate task.Refs: agent-memory task 124c89e0