Repository navigation
feat(cron): add scheduled task execution system with AI-callable tools - #867
Closed
swaroopvarma1 wants to merge 1 commit into
Closed
swaroopvarma1 wants to merge 1 commit into
swaroopvarma1 wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces a comprehensive cron/scheduling system for NeuroLink, including task lifecycle management, multiple task stores (in-memory and Redis-backed), scheduling backends, tool integration, and configuration support. It adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Agent
participant NL as NeuroLink
participant CronMgr as CronManager
participant Scheduler as SchedulerBackend
participant Store as TaskStore
participant Executor as TaskExecutor
Client->>NL: initialize with cron config
NL->>CronMgr: new CronManager(config)
CronMgr->>Scheduler: initialize NodeTimeoutScheduler
CronMgr->>Store: initialize TaskStore (memory/redis)
NL->>CronMgr: setExecutor(generate)
Client->>NL: call createScheduledTask tool
NL->>CronMgr: createTask(options)
CronMgr->>Scheduler: schedule(task, callback)
Scheduler->>Scheduler: compute nextRunAt
CronMgr->>Store: save(task)
CronMgr-->>Client: return ScheduledTask
Scheduler->>Scheduler: timeout/interval/cron triggers
Scheduler->>CronMgr: executeTask callback
CronMgr->>Executor: call executor(task, sessionId)
Executor->>NL: generate(prompt)
NL-->>Executor: responseText, tokenUsage
CronMgr->>Store: addRunResult(run)
CronMgr->>Scheduler: getNextRunTime(task)
Scheduler-->>CronMgr: next run timestamp
Client->>NL: shutdown
NL->>CronMgr: shutdown()
CronMgr->>Scheduler: shutdown()
CronMgr->>Store: shutdown()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (5)
src/lib/types/configTypes.ts (1)
132-133: Consider adding explicit default fordisableCronToolsinDEFAULT_CONFIG.The
disableCronToolsproperty is added toToolConfigbut not included inDEFAULT_CONFIG.tools. While the implicit default (undefined→falsevia the utility function) works, adding an explicit default would improve consistency with other tool flags likedisableBuiltinTools: false.🔧 Suggested addition to DEFAULT_CONFIG
tools: { disableBuiltinTools: false, allowCustomTools: true, maxToolsPerProvider: 100, enableMCPTools: true, + disableCronTools: false, },Also applies to: 217-222
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/types/configTypes.ts` around lines 132 - 133, Add an explicit default for the new ToolConfig property disableCronTools by setting it to false in the DEFAULT_CONFIG.tools object (the same place other flags like disableBuiltinTools are defined) and ensure any other DEFAULT_CONFIG occurrences that define tool defaults (the other DEFAULT_CONFIG.tools block referenced) also include disableCronTools: false so the default is explicit and consistent with other tool flags.src/lib/cron/cronTools.ts (1)
23-23: Consider extracting individual tools to reduce function length.Static analysis flagged the function as having too many lines (316 > 300). While the current implementation is readable, extracting each tool into a separate factory function could improve maintainability.
🔧 Example refactoring approach
// Extract each tool definition function createCreateScheduledTaskTool(getCronManager: () => CronManager | undefined) { return tool({ description: "Create a new scheduled task...", // ... }); } function createListScheduledTasksTool(getCronManager: () => CronManager | undefined) { return tool({ description: "List all scheduled tasks...", // ... }); } // Main factory composes them export function createCronTools(getCronManager: () => CronManager | undefined) { return { createScheduledTask: createCreateScheduledTaskTool(getCronManager), listScheduledTasks: createListScheduledTasksTool(getCronManager), cancelScheduledTask: createCancelScheduledTaskTool(getCronManager), getScheduledTaskStatus: createGetScheduledTaskStatusTool(getCronManager), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/cron/cronTools.ts` at line 23, The createCronTools function exceeds the allowed length; extract each tool object into its own small factory function (e.g., createCreateScheduledTaskTool, createListScheduledTasksTool, createCancelScheduledTaskTool, createGetScheduledTaskStatusTool) that accept the same getCronManager: () => CronManager | undefined and return the corresponding tool(...) definition, then have createCronTools simply compose and return those factories' results (referencing createCronTools and CronManager to locate the code). Ensure each new factory mirrors the original tool description/handler signatures and imports so behavior remains identical.src/lib/cron/taskStore.ts (2)
94-111: Consider adding connection error handling for Redis.The
getClient()method doesn't handle connection failures gracefully. If Redis is unavailable, the error will propagate to the caller without a clear error message indicating Redis connectivity issues.🔧 Suggested improvement
private async getClient(): Promise<RedisLikeClient> { if (this.redisClient) { return this.redisClient; } - // Dynamic import to avoid requiring redis as a dependency - const { createClient } = await import("redis"); + let createClient; + try { + const redis = await import("redis"); + createClient = redis.createClient; + } catch { + throw new Error( + "Redis package not installed. Install 'redis' to use RedisTaskStore.", + ); + } const url = this.config.url || `redis://${this.config.host || "localhost"}:${this.config.port || 6379}`; this.redisClient = createClient({ url, password: this.config.password, }) as unknown as RedisLikeClient; - await this.redisClient.connect(); + try { + await this.redisClient.connect(); + } catch (error) { + this.redisClient = null; + throw new Error( + `Failed to connect to Redis at ${url}: ${error instanceof Error ? error.message : String(error)}`, + ); + } return this.redisClient; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/cron/taskStore.ts` around lines 94 - 111, The getClient method currently calls createClient(...) and await this.redisClient.connect() without handling connection failures; wrap the connect call in a try/catch inside getClient (and consider also catching errors from createClient import) so connection errors are caught, log or rethrow a clearer error (e.g., include "Redis connection failed" and this.config host/port/url) and ensure this.redisClient is cleaned up on failure (set to undefined or call disconnect if partially connected); modify getClient, redisClient usage and error handling accordingly so callers receive a descriptive Redis connectivity error instead of an opaque exception.
134-154: Consider performance implications oflist()with many tasks.The current implementation fetches all task IDs from the Redis set, then issues individual
GETcommands for each task. For production workloads with many scheduled tasks, this could be slow.Consider using Redis pipelines or
MGETfor batch retrieval in a future optimization pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/cron/taskStore.ts` around lines 134 - 154, The list method currently calls getClient(), sMembers(indexKey()) and then issues individual GETs with taskKey(id) for each id; replace the per-id GET loop with a batched retrieval (e.g., build an array of task keys via this.taskKey(id) and call a single MGET or a Redis pipeline/transaction on the client to fetch all task values at once), then parse the returned array into ScheduledTask objects, apply the existing filter checks (status, sessionMode) and limit, and return the filtered slice; use the existing symbols list, getClient, indexKey, taskKey and ensure you handle possible null/undefined entries from the batched response.src/lib/neurolink.ts (1)
1374-1383: Build a typedGenerateOptionshere instead of casting throughRecord.This double cast bypasses strict TS and depends on the later top-level
sessionIdcompatibility shim. Passingcontext: { sessionId }in a realGenerateOptionskeeps the cron adapter on the supported API surface.As per coding guidelines "Maintain strict TypeScript across all modules with no circular dependencies between type definition files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 1374 - 1383, Replace the ad-hoc Record cast and build a properly typed GenerateOptions object for the call to this.generate: construct an object matching import("./types/generateTypes.js").GenerateOptions with fields input: { text: task.prompt }, provider: task.provider || config.defaultProvider, model: task.model || config.defaultModel, and put sessionId inside context: { sessionId } (not a top-level sessionId), then pass that object directly to this.generate (avoid any "as Record<string, unknown>" or double-casting); update the variable name generateOptions to that typed object so GenerateOptions is satisfied without bypassing TypeScript.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/agent/directTools.ts`:
- Around line 905-924: The module-level mutable cronManagerRef makes the last
NeuroLink instance win; instead expose instance-scoped cron tools by adding a
factory that binds a specific CronManager to createCronTools and avoid global
state: implement export function createCronToolsFor(manager: CronManager):
Record<string, any> { if (shouldDisableCronTools()) return {}; return
createCronTools(() => manager); } and update getCronTools to either accept an
optional manager argument or be deprecated in favor of createCronToolsFor; keep
setCronManagerRef as a compatibility shim that returns
createCronToolsFor(manager) (or internally delegates to it) so existing callers
still work while new code uses the instance-scoped factory (referencing
cronManagerRef, setCronManagerRef, getCronTools, createCronTools,
shouldDisableCronTools, NeuroLink).
In `@src/lib/cron/cronManager.ts`:
- Around line 193-203: The duplicate maxRuns check in executeTask (currently
before execution and again after) can race; replace the pre-execution check with
a single atomic check-and-increment in the store so only one run ever claims a
slot. Implement a new store method (e.g., incrementRunCountIfBelowMax / claimRun
or an optimistic-locking update in the store used by executeTask) that
atomically increments task.runCount and returns whether the run was allowed and
whether maxRuns was reached; use that result to decide whether to proceed, and
keep the post-execution logic (scheduler.cancel and
store.updateStatus("completed")) only when the atomic call indicates max
reached. Update executeTask to call the new store method instead of reading
task.maxRuns/task.runCount directly and remove the initial non-atomic check to
avoid race conditions.
- Line 186: The if statement checking task state in cronManager.ts (the line
with if (!task || task.status !== "active") return;) must use curly braces to
satisfy linting rules; replace the single-line form with a braced block (e.g.,
if (!task || task.status !== "active") { return; }) so the condition and early
return are enclosed in braces around the return statement.
- Line 136: The single-line conditional "if (!task) return false;" in
cronManager.ts (inside the function handling task lookup/validation where the
variable task is used) must be expanded to use curly braces to satisfy ESLint;
change it to a block-style if (i.e., wrap the return false in { ... }) so the
statement becomes an explicit block, then run the linter/CI to confirm the
ESLint error is resolved.
In `@src/lib/cron/schedulerBackend.ts`:
- Line 155: Add curly braces around the single-line if statement that calls
timer.unref to satisfy lint rules: change "if (timer.unref) timer.unref();" to
use a block form "if (timer.unref) { timer.unref(); }" so the conditional body
is wrapped in braces (referencing the timer.unref call in schedulerBackend.ts).
- Line 185: The single-line if statement using "if (!isNaN(rawNum) && rawNum >
0) return rawNum;" needs braces to satisfy lint rules; update the conditional in
the scheduler backend (the check using rawNum) to use a block form (e.g., if
(condition) { return rawNum; }) so the return is wrapped in curly braces and
formatting/linting errors are resolved.
- Line 134: Add curly braces around the single-line if statement that calls
timer.unref to satisfy linting: replace the bare if (timer.unref) timer.unref();
with a block form if (timer.unref) { timer.unref(); } in the scheduler code (the
conditional invoking timer.unref) so the linter no longer flags a missing block.
In `@src/lib/cron/taskStore.ts`:
- Line 95: The single-line if lacks curly braces and fails linting; update the
conditional that checks this.redisClient to use a braced block (e.g., if
(this.redisClient) { return this.redisClient; }) so the early-return is enclosed
in braces; locate the occurrence that references this.redisClient in taskStore
(the method where the client is returned) and wrap the return statement in {} to
satisfy the linter.
- Around line 143-145: The linter flags missing curly braces on the multiline if
statements in the task filtering logic; update the two conditions comparing
filter?.status and filter?.sessionMode against task.status and task.sessionMode
by wrapping each consequent continue in braces (e.g., change `if (filter?.status
&& task.status !== filter.status) continue;` to `if (filter?.status &&
task.status !== filter.status) { continue; }` and similarly for the sessionMode
check) inside the function that iterates tasks so the statements comply with
lint rules.
In `@src/lib/neurolink.ts`:
- Line 681: The constructor currently calls initializeCronManager(config?.cron)
which omits constructor-level cron settings because shouldDisableCronTools() is
later invoked with no config; update the call to forward the full config (e.g.,
initializeCronManager(config)) or otherwise ensure initializeCronManager
receives the same config object so that it can call
shouldDisableCronTools(config) and respect constructor-provided disable flags;
locate initializeCronManager and shouldDisableCronTools in neurolink.ts and make
them accept/consume the config parameter consistently so cron manager booting is
disabled based on the passed config, not only env vars.
- Around line 1397-1398: The code is storing an instance-scoped cron manager
into module-global state via setCronManagerRef(this.cronManager), which causes
the last NeuroLink() instance to override the shared scheduler; stop using
module-global state: remove the setCronManagerRef(this.cronManager) call and
refactor cron consumers to accept the cron manager from the NeuroLink instance
(inject cronManager into functions/classes that previously read module state),
e.g., change any cron helper APIs that relied on setCronManagerRef to take a
cronManager parameter or access it from the NeuroLink instance, keep
setCronManagerRef only as a deprecated shim that forwards when a singleton is in
use to preserve backward compatibility with the exported default new
NeuroLink(), and update callers to use instance.cronManager where possible
(references: setCronManagerRef, cronManager, NeuroLink, exported default new
NeuroLink).
- Around line 2248-2255: The dispose() path must mirror the cron cleanup: inside
the dispose() method, check if this.cronManager exists, call await
this.cronManager.shutdown() in a try/catch (log debug on success and warn on
failure), then clear the local reference (this.cronManager = undefined) and also
clear any module/shared reference that holds the manager so no stale cron
manager remains reachable; use the same cronManager and shutdown() symbols so
the shutdown logic matches the existing block.
- Around line 1354-1368: The current CronManagerConfig construction lets
...cronConfig overwrite environment overrides and accepts invalid env values;
fix by applying ...cronConfig first and then explicit overrides (so store and
maxConcurrentRuns take precedence), validate envStore against allowed values
("memory" | "redis") before assigning to store, parse envMaxConcurrent with
parseInt and fallback if Number.isNaN (or use Number.isInteger) to avoid NaN,
and ensure types align with CronManagerConfig; update the object construction
around CronManagerConfig, envStore, and envMaxConcurrent to implement these
changes.
---
Nitpick comments:
In `@src/lib/cron/cronTools.ts`:
- Line 23: The createCronTools function exceeds the allowed length; extract each
tool object into its own small factory function (e.g.,
createCreateScheduledTaskTool, createListScheduledTasksTool,
createCancelScheduledTaskTool, createGetScheduledTaskStatusTool) that accept the
same getCronManager: () => CronManager | undefined and return the corresponding
tool(...) definition, then have createCronTools simply compose and return those
factories' results (referencing createCronTools and CronManager to locate the
code). Ensure each new factory mirrors the original tool description/handler
signatures and imports so behavior remains identical.
In `@src/lib/cron/taskStore.ts`:
- Around line 94-111: The getClient method currently calls createClient(...) and
await this.redisClient.connect() without handling connection failures; wrap the
connect call in a try/catch inside getClient (and consider also catching errors
from createClient import) so connection errors are caught, log or rethrow a
clearer error (e.g., include "Redis connection failed" and this.config
host/port/url) and ensure this.redisClient is cleaned up on failure (set to
undefined or call disconnect if partially connected); modify getClient,
redisClient usage and error handling accordingly so callers receive a
descriptive Redis connectivity error instead of an opaque exception.
- Around line 134-154: The list method currently calls getClient(),
sMembers(indexKey()) and then issues individual GETs with taskKey(id) for each
id; replace the per-id GET loop with a batched retrieval (e.g., build an array
of task keys via this.taskKey(id) and call a single MGET or a Redis
pipeline/transaction on the client to fetch all task values at once), then parse
the returned array into ScheduledTask objects, apply the existing filter checks
(status, sessionMode) and limit, and return the filtered slice; use the existing
symbols list, getClient, indexKey, taskKey and ensure you handle possible
null/undefined entries from the batched response.
In `@src/lib/neurolink.ts`:
- Around line 1374-1383: Replace the ad-hoc Record cast and build a properly
typed GenerateOptions object for the call to this.generate: construct an object
matching import("./types/generateTypes.js").GenerateOptions with fields input: {
text: task.prompt }, provider: task.provider || config.defaultProvider, model:
task.model || config.defaultModel, and put sessionId inside context: { sessionId
} (not a top-level sessionId), then pass that object directly to this.generate
(avoid any "as Record<string, unknown>" or double-casting); update the variable
name generateOptions to that typed object so GenerateOptions is satisfied
without bypassing TypeScript.
In `@src/lib/types/configTypes.ts`:
- Around line 132-133: Add an explicit default for the new ToolConfig property
disableCronTools by setting it to false in the DEFAULT_CONFIG.tools object (the
same place other flags like disableBuiltinTools are defined) and ensure any
other DEFAULT_CONFIG occurrences that define tool defaults (the other
DEFAULT_CONFIG.tools block referenced) also include disableCronTools: false so
the default is explicit and consistent with other tool flags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2176feb9-440b-4344-a885-4ce17179700c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
package.jsonsrc/lib/agent/directTools.tssrc/lib/core/baseProvider.tssrc/lib/cron/cronManager.tssrc/lib/cron/cronTools.tssrc/lib/cron/index.tssrc/lib/cron/schedulerBackend.tssrc/lib/cron/taskStore.tssrc/lib/cron/types.tssrc/lib/neurolink.tssrc/lib/types/configTypes.tssrc/lib/types/index.tssrc/lib/utils/toolUtils.ts
Comment on lines
+905
to
+924
| /** Singleton reference to the CronManager, set by NeuroLink during initialization */ | ||
| let cronManagerRef: CronManager | undefined; | ||
|
|
||
| /** | ||
| * Set the CronManager reference for cron tools. | ||
| * Called by NeuroLink constructor after CronManager initialization. | ||
| */ | ||
| export function setCronManagerRef(manager: CronManager): void { | ||
| cronManagerRef = manager; | ||
| } | ||
|
|
||
| /** | ||
| * Get cron tools if enabled. Returns empty object if disabled via env var. | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| export function getCronTools(): Record<string, any> { | ||
| if (shouldDisableCronTools()) { | ||
| return {}; | ||
| } | ||
| return createCronTools(() => cronManagerRef); |
There was a problem hiding this comment.
Avoid process-global cron manager state.
cronManagerRef is shared across the entire module, so the last NeuroLink instance to call setCronManagerRef() wins. If an app constructs multiple SDK instances, cron tool calls can end up bound to the wrong scheduler/store. Please bind cron tools to an instance-scoped manager instead of a mutable singleton.
Possible direction
-let cronManagerRef: CronManager | undefined;
-
-export function setCronManagerRef(manager: CronManager): void {
- cronManagerRef = manager;
-}
-
-// eslint-disable-next-line `@typescript-eslint/no-explicit-any`
-export function getCronTools(): Record<string, any> {
- if (shouldDisableCronTools()) {
- return {};
- }
- return createCronTools(() => cronManagerRef);
+export function getCronTools(manager?: CronManager) {
+ if (shouldDisableCronTools() || !manager) {
+ return {};
+ }
+ return createCronTools(() => manager);
}As per coding guidelines, "Maintain backward compatibility with existing SDK APIs when making changes. All SDK modifications must not break existing code using the library."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/agent/directTools.ts` around lines 905 - 924, The module-level
mutable cronManagerRef makes the last NeuroLink instance win; instead expose
instance-scoped cron tools by adding a factory that binds a specific CronManager
to createCronTools and avoid global state: implement export function
createCronToolsFor(manager: CronManager): Record<string, any> { if
(shouldDisableCronTools()) return {}; return createCronTools(() => manager); }
and update getCronTools to either accept an optional manager argument or be
deprecated in favor of createCronToolsFor; keep setCronManagerRef as a
compatibility shim that returns createCronToolsFor(manager) (or internally
delegates to it) so existing callers still work while new code uses the
instance-scoped factory (referencing cronManagerRef, setCronManagerRef,
getCronTools, createCronTools, shouldDisableCronTools, NeuroLink).
| */ | ||
| async cancelTask(taskId: string): Promise<boolean> { | ||
| const task = await this.store.get(taskId); | ||
| if (!task) return false; |
There was a problem hiding this comment.
Add curly braces to satisfy linting rules.
This is causing CI pipeline failure (ESLint error).
🔧 Proposed fix
- if (!task) return false;
+ if (!task) {
+ return false;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!task) return false; | |
| if (!task) { | |
| return false; | |
| } |
🧰 Tools
🪛 GitHub Actions: CI
[error] 136-136: ESLint: Expected { after 'if' condition. (curly)
🪛 GitHub Check: 🛡️ Code Quality & Security Gate
[failure] 136-136:
Expected { after 'if' condition
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/cron/cronManager.ts` at line 136, The single-line conditional "if
(!task) return false;" in cronManager.ts (inside the function handling task
lookup/validation where the variable task is used) must be expanded to use curly
braces to satisfy ESLint; change it to a block-style if (i.e., wrap the return
false in { ... }) so the statement becomes an explicit block, then run the
linter/CI to confirm the ESLint error is resolved.
| private async executeTask(taskId: string): Promise<void> { | ||
| await this.concurrencyLimit(async () => { | ||
| const task = await this.store.get(taskId); | ||
| if (!task || task.status !== "active") return; |
There was a problem hiding this comment.
Add curly braces to satisfy linting rules.
Static analysis flagged missing curly braces after if condition.
🔧 Proposed fix
- if (!task || task.status !== "active") return;
+ if (!task || task.status !== "active") {
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!task || task.status !== "active") return; | |
| if (!task || task.status !== "active") { | |
| return; | |
| } |
🧰 Tools
🪛 GitHub Check: 🛡️ Code Quality & Security Gate
[failure] 186-186:
Expected { after 'if' condition
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/cron/cronManager.ts` at line 186, The if statement checking task
state in cronManager.ts (the line with if (!task || task.status !== "active")
return;) must use curly braces to satisfy linting rules; replace the single-line
form with a braced block (e.g., if (!task || task.status !== "active") { return;
}) so the condition and early return are enclosed in braces around the return
statement.
Comment on lines
+193
to
+203
| // Check maxRuns limit | ||
| if (task.maxRuns !== undefined && task.runCount >= task.maxRuns) { | ||
| this.scheduler.cancel(taskId); | ||
| await this.store.updateStatus(taskId, "completed"); | ||
| logger.info("[CronManager] Task completed (maxRuns reached)", { | ||
| taskId, | ||
| runCount: task.runCount, | ||
| maxRuns: task.maxRuns, | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Duplicate maxRuns check may cause race conditions.
The maxRuns limit is checked both at the start of executeTask (lines 194-203) and after execution (lines 270-276). If two runs complete concurrently, both might pass the initial check but both execute, potentially exceeding maxRuns.
Consider moving the check to a single atomic location or using optimistic locking in the store.
🔧 Suggested consolidation
private async executeTask(taskId: string): Promise<void> {
await this.concurrencyLimit(async () => {
const task = await this.store.get(taskId);
if (!task || task.status !== "active") {
return;
}
if (!this.executor) {
logger.warn("[CronManager] No executor set, skipping task", { taskId });
return;
}
- // Check maxRuns limit
- if (task.maxRuns !== undefined && task.runCount >= task.maxRuns) {
- this.scheduler.cancel(taskId);
- await this.store.updateStatus(taskId, "completed");
- logger.info("[CronManager] Task completed (maxRuns reached)", {
- taskId,
- runCount: task.runCount,
- maxRuns: task.maxRuns,
- });
- return;
- }
// ... execution logic ...
// Store run result
await this.store.addRunResult(taskId, run);
- // Update next run time
- const updatedTask = await this.store.get(taskId);
- if (updatedTask) {
- updatedTask.nextRunAt = this.scheduler.getNextRunTime(updatedTask);
- await this.store.save(updatedTask);
- }
+ // Re-fetch task to get updated runCount and check maxRuns atomically
+ const updatedTask = await this.store.get(taskId);
+ if (!updatedTask) {
+ return;
+ }
+
+ updatedTask.nextRunAt = this.scheduler.getNextRunTime(updatedTask);
+ await this.store.save(updatedTask);
// For "at" (one-shot) tasks, mark as completed after execution
if (task.schedule.type === "at") {
await this.store.updateStatus(taskId, "completed");
+ this.scheduler.cancel(taskId);
+ return;
}
// Check if maxRuns reached after this run
- if (
- task.maxRuns !== undefined &&
- task.runCount + 1 >= task.maxRuns
- ) {
+ if (updatedTask.maxRuns !== undefined && updatedTask.runCount >= updatedTask.maxRuns) {
this.scheduler.cancel(taskId);
await this.store.updateStatus(taskId, "completed");
}
});
}Also applies to: 270-276
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/cron/cronManager.ts` around lines 193 - 203, The duplicate maxRuns
check in executeTask (currently before execution and again after) can race;
replace the pre-execution check with a single atomic check-and-increment in the
store so only one run ever claims a slot. Implement a new store method (e.g.,
incrementRunCountIfBelowMax / claimRun or an optimistic-locking update in the
store used by executeTask) that atomically increments task.runCount and returns
whether the run was allowed and whether maxRuns was reached; use that result to
decide whether to proceed, and keep the post-execution logic (scheduler.cancel
and store.updateStatus("completed")) only when the atomic call indicates max
reached. Update executeTask to call the new store method instead of reading
task.maxRuns/task.runCount directly and remove the initial non-atomic check to
avoid race conditions.
| }, delayMs); | ||
|
|
||
| // Prevent the timer from keeping the process alive | ||
| if (timer.unref) timer.unref(); |
There was a problem hiding this comment.
Add curly braces to satisfy linting rules.
Static analysis flagged missing curly braces after if condition. This is causing CI pipeline failures.
🔧 Proposed fix
- if (timer.unref) timer.unref();
+ if (timer.unref) {
+ timer.unref();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (timer.unref) timer.unref(); | |
| if (timer.unref) { | |
| timer.unref(); | |
| } |
🧰 Tools
🪛 GitHub Check: 🛡️ Code Quality & Security Gate
[failure] 134-134:
Expected { after 'if' condition
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/cron/schedulerBackend.ts` at line 134, Add curly braces around the
single-line if statement that calls timer.unref to satisfy linting: replace the
bare if (timer.unref) timer.unref(); with a block form if (timer.unref) {
timer.unref(); } in the scheduler code (the conditional invoking timer.unref) so
the linter no longer flags a missing block.
Comment on lines
+143
to
+145
| if (filter?.status && task.status !== filter.status) continue; | ||
| if (filter?.sessionMode && task.sessionMode !== filter.sessionMode) | ||
| continue; |
There was a problem hiding this comment.
Add curly braces to satisfy linting rules.
Static analysis flagged missing curly braces for multiline if statements.
🔧 Proposed fix
- if (filter?.status && task.status !== filter.status) continue;
- if (filter?.sessionMode && task.sessionMode !== filter.sessionMode)
- continue;
+ if (filter?.status && task.status !== filter.status) {
+ continue;
+ }
+ if (filter?.sessionMode && task.sessionMode !== filter.sessionMode) {
+ continue;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (filter?.status && task.status !== filter.status) continue; | |
| if (filter?.sessionMode && task.sessionMode !== filter.sessionMode) | |
| continue; | |
| if (filter?.status && task.status !== filter.status) { | |
| continue; | |
| } | |
| if (filter?.sessionMode && task.sessionMode !== filter.sessionMode) { | |
| continue; | |
| } |
🧰 Tools
🪛 GitHub Check: 🛡️ Code Quality & Security Gate
[failure] 145-145:
Expected { after 'if' condition
[failure] 143-143:
Expected { after 'if' condition
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/cron/taskStore.ts` around lines 143 - 145, The linter flags missing
curly braces on the multiline if statements in the task filtering logic; update
the two conditions comparing filter?.status and filter?.sessionMode against
task.status and task.sessionMode by wrapping each consequent continue in braces
(e.g., change `if (filter?.status && task.status !== filter.status) continue;`
to `if (filter?.status && task.status !== filter.status) { continue; }` and
similarly for the sessionMode check) inside the function that iterates tasks so
the statements comply with lint rules.
| ); | ||
| this.registerFileTools(); | ||
| this.registerMemoryRetrievalTools(); | ||
| this.initializeCronManager(config?.cron); |
There was a problem hiding this comment.
Cron disablement is env-only from this path.
Only config.cron is threaded into initializeCronManager(), and Line 1349 calls shouldDisableCronTools() with no config. That means any constructor-level tool settings meant to disable cron are ignored here, so the manager still boots unless NEUROLINK_DISABLE_CRON_TOOLS is set.
Also applies to: 1348-1350
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/neurolink.ts` at line 681, The constructor currently calls
initializeCronManager(config?.cron) which omits constructor-level cron settings
because shouldDisableCronTools() is later invoked with no config; update the
call to forward the full config (e.g., initializeCronManager(config)) or
otherwise ensure initializeCronManager receives the same config object so that
it can call shouldDisableCronTools(config) and respect constructor-provided
disable flags; locate initializeCronManager and shouldDisableCronTools in
neurolink.ts and make them accept/consume the config parameter consistently so
cron manager booting is disabled based on the passed config, not only env vars.
Comment on lines
+1354
to
+1368
| const envStore = process.env.NEUROLINK_CRON_STORE; | ||
| const envMaxConcurrent = process.env.NEUROLINK_CRON_MAX_CONCURRENT; | ||
|
|
||
| const config: CronManagerConfig = { | ||
| enabled: true, | ||
| store: (envStore as "memory" | "redis") || cronConfig?.store || "memory", | ||
| maxConcurrentRuns: envMaxConcurrent | ||
| ? parseInt(envMaxConcurrent, 10) | ||
| : cronConfig?.maxConcurrentRuns, | ||
| maxRunHistory: cronConfig?.maxRunHistory, | ||
| redisConfig: cronConfig?.redisConfig, | ||
| defaultProvider: cronConfig?.defaultProvider, | ||
| defaultModel: cronConfig?.defaultModel, | ||
| ...cronConfig, | ||
| }; |
There was a problem hiding this comment.
Don't let ...cronConfig erase the env overrides.
...cronConfig is applied after store and maxConcurrentRuns, so explicit config wins over NEUROLINK_CRON_STORE / NEUROLINK_CRON_MAX_CONCURRENT. The unchecked cast and raw parseInt() also allow invalid values through as arbitrary strings or NaN.
Suggested fix
const envStore = process.env.NEUROLINK_CRON_STORE;
const envMaxConcurrent = process.env.NEUROLINK_CRON_MAX_CONCURRENT;
+ const resolvedStore =
+ envStore === "memory" || envStore === "redis"
+ ? envStore
+ : (cronConfig?.store ?? "memory");
+
+ const parsedMaxConcurrent =
+ envMaxConcurrent !== undefined
+ ? Number.parseInt(envMaxConcurrent, 10)
+ : cronConfig?.maxConcurrentRuns;
+
const config: CronManagerConfig = {
- enabled: true,
- store: (envStore as "memory" | "redis") || cronConfig?.store || "memory",
- maxConcurrentRuns: envMaxConcurrent
- ? parseInt(envMaxConcurrent, 10)
- : cronConfig?.maxConcurrentRuns,
- maxRunHistory: cronConfig?.maxRunHistory,
- redisConfig: cronConfig?.redisConfig,
- defaultProvider: cronConfig?.defaultProvider,
- defaultModel: cronConfig?.defaultModel,
...cronConfig,
+ enabled: cronConfig?.enabled ?? true,
+ store: resolvedStore,
+ maxConcurrentRuns:
+ parsedMaxConcurrent !== undefined &&
+ Number.isInteger(parsedMaxConcurrent) &&
+ parsedMaxConcurrent > 0
+ ? parsedMaxConcurrent
+ : undefined,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const envStore = process.env.NEUROLINK_CRON_STORE; | |
| const envMaxConcurrent = process.env.NEUROLINK_CRON_MAX_CONCURRENT; | |
| const config: CronManagerConfig = { | |
| enabled: true, | |
| store: (envStore as "memory" | "redis") || cronConfig?.store || "memory", | |
| maxConcurrentRuns: envMaxConcurrent | |
| ? parseInt(envMaxConcurrent, 10) | |
| : cronConfig?.maxConcurrentRuns, | |
| maxRunHistory: cronConfig?.maxRunHistory, | |
| redisConfig: cronConfig?.redisConfig, | |
| defaultProvider: cronConfig?.defaultProvider, | |
| defaultModel: cronConfig?.defaultModel, | |
| ...cronConfig, | |
| }; | |
| const envStore = process.env.NEUROLINK_CRON_STORE; | |
| const envMaxConcurrent = process.env.NEUROLINK_CRON_MAX_CONCURRENT; | |
| const resolvedStore = | |
| envStore === "memory" || envStore === "redis" | |
| ? envStore | |
| : (cronConfig?.store ?? "memory"); | |
| const parsedMaxConcurrent = | |
| envMaxConcurrent !== undefined | |
| ? Number.parseInt(envMaxConcurrent, 10) | |
| : cronConfig?.maxConcurrentRuns; | |
| const config: CronManagerConfig = { | |
| ...cronConfig, | |
| enabled: cronConfig?.enabled ?? true, | |
| store: resolvedStore, | |
| maxConcurrentRuns: | |
| parsedMaxConcurrent !== undefined && | |
| Number.isInteger(parsedMaxConcurrent) && | |
| parsedMaxConcurrent > 0 | |
| ? parsedMaxConcurrent | |
| : undefined, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/neurolink.ts` around lines 1354 - 1368, The current CronManagerConfig
construction lets ...cronConfig overwrite environment overrides and accepts
invalid env values; fix by applying ...cronConfig first and then explicit
overrides (so store and maxConcurrentRuns take precedence), validate envStore
against allowed values ("memory" | "redis") before assigning to store, parse
envMaxConcurrent with parseInt and fallback if Number.isNaN (or use
Number.isInteger) to avoid NaN, and ensure types align with CronManagerConfig;
update the object construction around CronManagerConfig, envStore, and
envMaxConcurrent to implement these changes.
Comment on lines
+1397
to
+1398
| // Set the reference so cron tools can access the manager | ||
| setCronManagerRef(this.cronManager); |
There was a problem hiding this comment.
Avoid a module-global cron manager for instance-scoped tools.
setCronManagerRef() stores this manager in shared module state. Because this file also exports a default new NeuroLink() singleton at Lines 9206-9208, the last constructed instance wins, so multi-instance apps can end up creating/listing/canceling tasks against the wrong scheduler or store.
As per coding guidelines "Maintain backward compatibility with existing SDK APIs when making changes. All SDK modifications must not break existing code using the library."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/neurolink.ts` around lines 1397 - 1398, The code is storing an
instance-scoped cron manager into module-global state via
setCronManagerRef(this.cronManager), which causes the last NeuroLink() instance
to override the shared scheduler; stop using module-global state: remove the
setCronManagerRef(this.cronManager) call and refactor cron consumers to accept
the cron manager from the NeuroLink instance (inject cronManager into
functions/classes that previously read module state), e.g., change any cron
helper APIs that relied on setCronManagerRef to take a cronManager parameter or
access it from the NeuroLink instance, keep setCronManagerRef only as a
deprecated shim that forwards when a singleton is in use to preserve backward
compatibility with the exported default new NeuroLink(), and update callers to
use instance.cronManager where possible (references: setCronManagerRef,
cronManager, NeuroLink, exported default new NeuroLink).
Comment on lines
+2248
to
+2255
| if (this.cronManager) { | ||
| try { | ||
| await this.cronManager.shutdown(); | ||
| logger.debug("[NeuroLink] CronManager shutdown completed"); | ||
| } catch (error) { | ||
| logger.warn("[NeuroLink] CronManager shutdown failed:", error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Mirror this cron cleanup in dispose() and clear the shared ref.
shutdown() now stops the instance-local manager, but dispose() later in this file still never shuts it down. That leaves timer/Redis resources alive on the documented cleanup path, and direct cron tools can keep holding a stale manager reference after shutdown.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/neurolink.ts` around lines 2248 - 2255, The dispose() path must
mirror the cron cleanup: inside the dispose() method, check if this.cronManager
exists, call await this.cronManager.shutdown() in a try/catch (log debug on
success and warn on failure), then clear the local reference (this.cronManager =
undefined) and also clear any module/shared reference that holds the manager so
no stale cron manager remains reachable; use the same cronManager and shutdown()
symbols so the shutdown logic matches the existing block.
Implement a cron/scheduling system for NeuroLink that allows AI models to create, manage, and execute scheduled tasks via generate() calls. New files (src/lib/cron/): - types.ts: Type definitions for tasks, schedules, stores, backends - taskStore.ts: InMemoryTaskStore + RedisTaskStore with atomic operations - schedulerBackend.ts: NodeTimeoutScheduler using setTimeout/setInterval/croner - cronManager.ts: Core orchestrator for task lifecycle and execution - cronTools.ts: 4 AI-callable tools (create, list, cancel, getStatus) - index.ts: Public exports Features: - Three schedule types: "at" (one-shot), "every" (interval), "cron" (expression) - Two session modes: "isolated" (fresh context) and "same-session" (shared) - Instance-scoped CronManager (no module-global state, multi-instance safe) - Atomic incrementRunCountIfUnderLimit() to prevent maxRuns race conditions - Cryptographically secure task IDs via crypto.randomBytes() - Pluggable scheduler backend interface (Node.js default, extensible) - Pluggable persistence (in-memory default, optional Redis) - Concurrency control via p-limit - Env-based control: NEUROLINK_DISABLE_CRON_TOOLS, NEUROLINK_CRON_STORE, NEUROLINK_CRON_MAX_CONCURRENT (validated before use) - Registered as built-in tools via BaseProvider with NeuroLink instance binding - Graceful shutdown in both shutdown() and dispose() paths https://claude.ai/code/session_01LGQEq8JtRqf6QnrrrkvXfo
swaroopvarma1
force-pushed
the
claude/neurolink-cron-exploration-8055o
branch
from
March 12, 2026 16:46
e52d3c2 to
3b9b2dc
Compare
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Collaborator
Author
|
Changes requested This PR introduces critical runtime defects and non-functional components that make the scheduler unsafe for production. A ReferenceError masks exceptions, RedisTaskStore is unusable, and cron expressions are silently replaced with hardcoded intervals.
|
Contributor
This branch was successfully deployed
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
4 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.
Implement a cron/scheduling system for NeuroLink that allows AI models
to create, manage, and execute scheduled tasks via generate() calls.
New files (src/lib/cron/):
Features:
NEUROLINK_CRON_MAX_CONCURRENT
https://claude.ai/code/session_01LGQEq8JtRqf6QnrrrkvXfo
Summary by CodeRabbit
Release Notes