Repository navigation
feat(tasks): add TaskManager — scheduled and self-running AI tasks - #913
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 TaskManager system for NeuroLink, enabling scheduled and on-demand task execution with two pluggable backends (BullMQ for Redis-based distributed scheduling and NodeTimeout for in-process timers), persistent storage via file or Redis, continuation-mode multi-turn execution, CLI commands for full lifecycle management, and agent-facing tools for programmatic task control. Changes
Sequence DiagramsequenceDiagram
participant Client
participant TaskManager
participant TaskBackend as TaskBackend<br/>(BullMQ/Timeout)
participant TaskExecutor
participant TaskStore
participant NeuroLink
participant Callback as Callback<br/>Handler
Client->>TaskManager: create(taskDefinition)
TaskManager->>TaskStore: save(task)
TaskManager->>TaskBackend: schedule(task, executor)
TaskBackend->>TaskBackend: setup timer/cron
TaskManager-->>Client: task created
Note over TaskBackend: time elapsed or<br/>manual trigger
TaskBackend->>TaskManager: onTaskTick()
TaskManager->>TaskStore: get(taskId)
TaskManager->>TaskExecutor: execute(task)
TaskExecutor->>NeuroLink: generate(prompt)
NeuroLink-->>TaskExecutor: output + toolCalls
TaskExecutor->>TaskStore: appendRun(result)
TaskManager->>Callback: invoke(result)
Callback-->>Callback: success/error handler
TaskManager->>Client: emit(task:completed/failed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes The PR spans 20+ new files totaling 3,500+ lines with heterogeneous changes: dense task orchestration logic (lifecycle, state transitions, retries), two pluggable backend implementations with Redis/file I/O, CLI command infrastructure, agent tool definitions, and interconnected type/storage layers. While individual files follow clear patterns, the interdependencies and operational semantics (retry backoff, TTL enforcement, continuation history, pause/resume guarantees) require careful reasoning across multiple subsystems. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
✅ 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 |
Documentation Validation Results🚀 Documentation validation passed!
📦 Build artifact uploaded successfully. Ready for deployment preview. Commit: |
There was a problem hiding this comment.
Pull request overview
Introduces a TaskManager subsystem to NeuroLink for scheduled/self-running AI tasks, including SDK API integration, agent tools, CLI commands, and supporting storage/backends (BullMQ+Redis for production, NodeTimeout+files for dev). It also extends the generate pipeline to support proper multi-turn continuation via conversationMessages.
Changes:
- Add TaskManager core (types, executor, stores, backends) and integrate into
NeuroLinkplus direct agent tools. - Add
neurolink task ...CLI command group and a unified test runner + multiple task test suites. - Extend generation types/pipeline to pass
conversationMessagesand deprecateconversationHistory.
Reviewed changes
Copilot reviewed 30 out of 32 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test-task-tools.mjs | Adds an AI tool discovery test for task tools. |
| test/test-task-production.mjs | Adds production-readiness behavior tests for NodeTimeout backend. |
| test/test-task-manager.mjs | Adds basic unit-style CRUD/lifecycle tests for TaskManager. |
| test/test-task-execute.mjs | Adds real-AI integration test for task execution. |
| test/test-task-edge-cases.mjs | Adds edge-case tests (retry behavior, limits, history bounds, cron validation). |
| test/test-task-continuation.mjs | Adds continuation-mode behavior test using conversation history. |
| test/test-task-bullmq.mjs | Adds BullMQ+Redis integration tests (requires local Redis). |
| test/test-task-all.mjs | Adds unified runner to execute all task suites sequentially. |
| src/lib/types/index.ts | Re-exports TaskManager-related public types from tasks module. |
| src/lib/types/generateTypes.ts | Adds conversationMessages to GenerateOptions and deprecates conversationHistory. |
| src/lib/types/configTypes.ts | Adds tasks?: TaskManagerConfig to NeurolinkConstructorConfig. |
| src/lib/tasks/types.ts | Defines TaskManager types and defaults (schedules, config, run results, store/backend interfaces). |
| src/lib/tasks/tools/taskTools.ts | Implements built-in agent tools (create/list/runs/delete/run-now) for tasks. |
| src/lib/tasks/taskManager.ts | Implements TaskManager lifecycle orchestration (CRUD, scheduling, execution, pause/resume, events). |
| src/lib/tasks/taskExecutor.ts | Implements single-run execution with retries and continuation history plumbing. |
| src/lib/tasks/store/taskStore.ts | Re-exports TaskStore-related types for convenience. |
| src/lib/tasks/store/redisTaskStore.ts | Implements Redis-backed task/run/history persistence and trimming/TTL hooks. |
| src/lib/tasks/store/fileTaskStore.ts | Implements file-backed task persistence and JSONL run logs (NodeTimeout backend). |
| src/lib/tasks/index.ts | Exports TaskManager module public API (core, stores, backends, tools, types). |
| src/lib/tasks/backends/taskBackendRegistry.ts | Adds registry/factory for built-in and custom task backends. |
| src/lib/tasks/backends/taskBackend.ts | Re-exports TaskBackend-related types for convenience. |
| src/lib/tasks/backends/nodeTimeoutBackend.ts | Implements in-process scheduling via Croner + timers. |
| src/lib/tasks/backends/bullmqBackend.ts | Implements BullMQ scheduling/worker execution using Redis. |
| src/lib/neurolink.ts | Integrates TaskManager (lazy getter + shutdown) and wires conversationMessages through generate/workflows/stream fallback. |
| src/lib/agent/directTools.ts | Registers TaskManager agent tools into direct agent tools set. |
| src/cli/parser.ts | Registers task command group in CLI parser. |
| src/cli/commands/task.ts | Adds neurolink task command suite (create/list/get/run/pause/resume/update/delete/logs/start worker). |
| scripts/build-browser.mjs | Adds stubs/exports for new dependencies and fs rename to support browser build. |
| pnpm-lock.yaml | Locks new dependencies (BullMQ, Croner, transitive deps). |
| package.json | Adds BullMQ and Croner dependencies; pins packageManager version. |
| docs/features/task-manager.md | Adds extensive TaskManager documentation (architecture, API, CLI, tools). |
| .gitignore | Adjusts ignore rules to include test/test-*.mjs files in version control. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export class NodeTimeoutBackend implements TaskBackend { | ||
| readonly name = "node-timeout"; | ||
| private scheduled = new Map<string, ScheduledEntry>(); | ||
| private paused = new Map<string, ScheduledEntry>(); | ||
|
|
||
| constructor(_config: TaskManagerConfig) { | ||
| // No config needed for in-process timers | ||
| } | ||
|
|
||
| async initialize(): Promise<void> { | ||
| logger.info("[NodeTimeout] Backend initialized"); | ||
| } | ||
|
|
||
| async shutdown(): Promise<void> { | ||
| for (const entry of this.scheduled.values()) { | ||
| this.clearEntry(entry); | ||
| } | ||
| this.scheduled.clear(); | ||
| this.paused.clear(); | ||
| logger.info("[NodeTimeout] Backend shut down"); | ||
| } | ||
|
|
||
| async schedule(task: Task, executor: TaskExecutorFn): Promise<void> { | ||
| // Cancel existing schedule for this task if any | ||
| await this.cancel(task.id); | ||
|
|
||
| const entry: ScheduledEntry = { taskId: task.id, executor, task }; | ||
| const schedule = task.schedule; | ||
|
|
||
| if (schedule.type === "cron") { | ||
| entry.cronJob = new Cron( | ||
| schedule.expression, | ||
| { | ||
| timezone: schedule.timezone, | ||
| catch: (err) => { | ||
| logger.error("[NodeTimeout] Cron execution error", { | ||
| taskId: task.id, | ||
| error: String(err), | ||
| }); | ||
| }, | ||
| }, | ||
| () => { | ||
| this.executeTask(entry); | ||
| }, | ||
| ); | ||
| } else if (schedule.type === "interval") { | ||
| // Wait for the first interval tick before executing | ||
| entry.intervalId = setInterval(() => { | ||
| this.executeTask(entry); | ||
| }, schedule.every); | ||
| } else if (schedule.type === "once") { |
There was a problem hiding this comment.
TaskManagerConfig.maxConcurrentRuns is used to configure BullMQ worker concurrency, but the NodeTimeout backend ignores it entirely. In a single process, this can allow unbounded concurrent executions (especially if tasks take longer than their schedule interval). Consider enforcing maxConcurrentRuns in NodeTimeoutBackend (or in TaskExecutor/TaskManager) to prevent overload.
There was a problem hiding this comment.
⏺ executeTask() tracks activeRuns and skips when >= maxConcurrentRuns. The constructor reads it from config on line 39-40. Copilot didn't read the file.
| /** | ||
| * TaskManager instance reference — set at initialization. | ||
| * Tools are no-ops until this is set. | ||
| */ | ||
| let taskManagerRef: TaskManager | null = null; | ||
|
|
||
| export function setTaskManagerRef(manager: TaskManager): void { | ||
| taskManagerRef = manager; | ||
| } | ||
|
|
||
| export function clearTaskManagerRef(): void { | ||
| taskManagerRef = null; | ||
| } | ||
|
|
||
| function getManager(): TaskManager { | ||
| if (!taskManagerRef) { | ||
| throw new Error("TaskManager not initialized. Tasks are not available."); | ||
| } | ||
| return taskManagerRef; | ||
| } |
There was a problem hiding this comment.
Task tools rely on a module-level singleton (taskManagerRef). This becomes problematic when multiple NeuroLink instances exist in the same process (tools may operate on the wrong instance), and it can also keep a shutdown TaskManager reachable. Prefer passing the TaskManager via tool execution context (or registering tools per-instance) rather than storing a global reference.
| // Instead, run logs and history keys get TTL. Task cleanup is handled | ||
| // by a periodic sweep in TaskManager or by BullMQ's built-in job cleanup. |
There was a problem hiding this comment.
applyRetentionTTL() comments claim task cleanup is handled “by a periodic sweep in TaskManager”, but no such sweep exists in TaskManager (and Redis hashes can’t TTL individual fields). As written, terminal tasks stored in neurolink:tasks will never expire automatically. Either implement the sweep/cleanup mechanism or update the comment to avoid implying behavior that isn’t present.
| // Instead, run logs and history keys get TTL. Task cleanup is handled | |
| // by a periodic sweep in TaskManager or by BullMQ's built-in job cleanup. | |
| // Instead, only the associated run logs and history keys receive TTL here. | |
| // Task hash entries in neurolink:tasks are not automatically expired by this store. |
| ### Environment Variable Overrides | ||
|
|
||
| | Variable | Purpose | Default | | ||
| | -------------------------------- | ---------------------------------- | ----------------------------- | | ||
| | `NEUROLINK_TASKS_ENABLED` | Enable/disable TaskManager | `true` | | ||
| | `NEUROLINK_TASKS_BACKEND` | Backend selection | `bullmq` | | ||
| | `NEUROLINK_TASKS_REDIS_URL` | Redis connection URL (BullMQ) | `redis://localhost:6379` | | ||
| | `NEUROLINK_TASKS_STORE_PATH` | File store path (NodeTimeout only) | `.neurolink/tasks/tasks.json` | | ||
| | `NEUROLINK_TASKS_MAX_CONCURRENT` | Max concurrent task runs | `5` | | ||
|
|
There was a problem hiding this comment.
The “Environment Variable Overrides” section lists NEUROLINK_TASKS_* variables, but there is no code reading these env vars in the current implementation. Either implement the overrides or remove/flag this section to avoid advertising unsupported configuration.
| taskUpdates.schedule = updates.schedule; | ||
| } | ||
| if (updates.mode !== undefined) { | ||
| taskUpdates.mode = updates.mode; |
There was a problem hiding this comment.
Updating a task’s mode to "continuation" doesn’t create a sessionId, so continuation history will never be loaded/appended (TaskExecutor requires task.sessionId). Similarly, switching away from continuation doesn’t clear sessionId/history. Handle mode transitions explicitly (create sessionId + initialize history on switch to continuation; clear history/sessionId when switching to isolated).
| taskUpdates.mode = updates.mode; | |
| // Handle mode transitions explicitly to manage continuation session state | |
| const newMode = updates.mode; | |
| const oldMode = existing.mode; | |
| // Only perform transition logic if the mode is actually changing | |
| if (newMode !== oldMode) { | |
| taskUpdates.mode = newMode; | |
| if (newMode === "continuation") { | |
| // Entering continuation mode: ensure we have a sessionId and initialized history | |
| (taskUpdates as any).sessionId = (existing as any).sessionId ?? nanoid(); | |
| // Initialize history if it does not exist; preserve existing history if present | |
| if ((existing as any).history === undefined) { | |
| (taskUpdates as any).history = []; | |
| } | |
| } else if (oldMode === "continuation") { | |
| // Leaving continuation mode: clear continuation-specific state | |
| (taskUpdates as any).sessionId = undefined; | |
| (taskUpdates as any).history = []; | |
| } | |
| } else { | |
| // Mode unchanged: propagate the value if the caller explicitly set it | |
| taskUpdates.mode = newMode; | |
| } |
| 3. The conversation history is stored via NeuroLink's existing memory system (Redis or in-memory) | ||
| 4. On subsequent runs, the full history is loaded and passed as `conversationHistory` to `generate()` | ||
| 5. Context compaction kicks in automatically when history exceeds budget |
There was a problem hiding this comment.
The continuation-mode description says history is passed as conversationHistory to generate() and that NeuroLink’s context compaction kicks in automatically. In the implementation, TaskExecutor uses conversationMessages, and history bounding is handled via maxHistoryEntries trimming in the store (no summarization/compaction step here). Please update this section to match the actual behavior.
| 3. The conversation history is stored via NeuroLink's existing memory system (Redis or in-memory) | |
| 4. On subsequent runs, the full history is loaded and passed as `conversationHistory` to `generate()` | |
| 5. Context compaction kicks in automatically when history exceeds budget | |
| 3. The conversation history is stored via NeuroLink's existing memory system (Redis or in-memory), keyed by the task's `sessionId` | |
| 4. On subsequent runs, the stored messages are loaded and passed as `conversationMessages` to `generate()` | |
| 5. History is bounded by `maxHistoryEntries` trimming in the task store (older messages are dropped); TaskManager itself does not perform additional summarization/compaction |
| // Check if task should complete | ||
| if (current.maxRuns && current.runCount + 1 >= current.maxRuns) { | ||
| updates.status = "completed"; | ||
| await this.backend!.cancel(task.id); |
There was a problem hiding this comment.
once-scheduled tasks are never marked completed after their first successful run (completion is only tied to maxRuns). This leaves one-shot tasks stuck as active, and rescheduleActiveTasks() will re-schedule them on restart (often with delay=0), causing unintended re-execution. Consider automatically setting status="completed" (and cancelling any remaining scheduled work) after the first successful execution for schedule.type === "once" when maxRuns isn’t explicitly set.
| await this.backend!.cancel(task.id); | |
| await this.backend!.cancel(task.id); | |
| } else if ( | |
| current.schedule.type === "once" && | |
| !current.maxRuns && | |
| result.status === "success" | |
| ) { | |
| // Auto-complete one-shot tasks after first successful run | |
| updates.status = "completed"; | |
| await this.backend!.cancel(task.id); |
There was a problem hiding this comment.
This is also invalid. Look at the code right below that block:
Read 1 file (ctrl+o to expand)
⏺ Lines 411-415 already handle exactly what Copilot suggests — once-schedule tasks are marked "completed" after a successful run. And lines 417-420 mark them
"failed" on error.
Copilot didn't read 3 lines further. Invalid comment.
| await cbs.onError({ | ||
| taskId: task.id, | ||
| runId: result.runId, | ||
| error: result.error ?? "Unknown error", | ||
| attempt: current.retry.maxAttempts, // Already retried in executor | ||
| maxAttempts: current.retry.maxAttempts, | ||
| willRetry: false, | ||
| timestamp: result.timestamp, | ||
| }); |
There was a problem hiding this comment.
The onError callback payload sets attempt to current.retry.maxAttempts, which doesn’t reflect the actual attempt count (e.g., permanent errors fail on attempt 1). Either track attempts in TaskExecutor and return it, or set attempt based on real execution attempts so consumers can reason about retry behavior accurately.
There was a problem hiding this comment.
Half-valid observation, but low priority. The attempt field is misleading (always 1) but it doesn't matter in practice because willRetry: false already tells
the consumer "this is final". To truly fix it, TaskExecutor.execute() would need to return the attempt count in TaskRunResult. Not worth doing in this PR —
it's cosmetic.
| } | ||
|
|
||
| // Schedule | ||
| await this.backend!.schedule(task, (t) => this.onTaskTick(t)); |
There was a problem hiding this comment.
Task creation saves the task to the store before scheduling it. If backend.schedule() throws (e.g., invalid cron expression, Redis/BullMQ error), the task will remain persisted but unscheduled, and create() will throw, leaving inconsistent state. Consider scheduling first (if possible) or rolling back (delete task) on schedule failure.
| await this.backend!.schedule(task, (t) => this.onTaskTick(t)); | |
| try { | |
| await this.backend!.schedule(task, (t) => this.onTaskTick(t)); | |
| } catch (err) { | |
| // Roll back persisted task and in-memory callbacks on scheduling failure | |
| try { | |
| await this.store!.delete(task.id); | |
| } catch (deleteErr) { | |
| logger.error("[TaskManager] Failed to roll back task after schedule error", { | |
| taskId: task.id, | |
| scheduleError: err, | |
| deleteError: deleteErr, | |
| }); | |
| } | |
| this.callbacks.delete(task.id); | |
| throw err; | |
| } |
| - Uses `bullmq` `Queue` + `Worker` + `QueueScheduler` | ||
| - Cron tasks → BullMQ repeatable jobs | ||
| - Interval tasks → BullMQ repeatable jobs with `every` option | ||
| - One-shot tasks → BullMQ delayed jobs | ||
| - Pause/Resume → BullMQ job state management |
There was a problem hiding this comment.
BullMQ backend docs mention QueueScheduler and “Pause/Resume → BullMQ job state management”, but the implementation doesn’t use QueueScheduler and pause is implemented via cancel() + re-schedule() on resume. Please adjust this section so operational expectations match the code.
| - Uses `bullmq` `Queue` + `Worker` + `QueueScheduler` | |
| - Cron tasks → BullMQ repeatable jobs | |
| - Interval tasks → BullMQ repeatable jobs with `every` option | |
| - One-shot tasks → BullMQ delayed jobs | |
| - Pause/Resume → BullMQ job state management | |
| - Uses `bullmq` `Queue` + `Worker` | |
| - Cron tasks → BullMQ repeatable jobs | |
| - Interval tasks → BullMQ repeatable jobs with `every` option | |
| - One-shot tasks → BullMQ delayed jobs | |
| - Pause/Resume → implemented by cancelling scheduled executions and re-scheduling them on resume (does not affect in-flight jobs) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (20)
docs/features/task-manager.md-646-655 (1)
646-655:⚠️ Potential issue | 🟠 MajorRemove outdated
QueueSchedulerreference from BullMQ documentation.The documentation lists
QueueScheduleras part of BullMQ's architecture, but this component was deprecated in BullMQ v2.0.0 (released September 2022) and removed in subsequent versions. The project uses BullMQ v5.52.2, whereQueueSchedulerfunctionality has been merged entirely into theWorkerclass. Remove this reference from the BullMQ Backend Details section.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/task-manager.md` around lines 646 - 655, Remove the outdated reference to QueueScheduler in the BullMQ Backend Details: update the line that currently reads "Uses `bullmq` `Queue` + `Worker` + `QueueScheduler`" to reflect the current BullMQ v5 behavior (e.g., "Uses `bullmq` `Queue` + `Worker`") and, if helpful, add a brief note that `Worker` now includes scheduling functionality; ensure mentions of `QueueScheduler` elsewhere in this section are deleted or replaced with `Worker`.src/lib/neurolink.ts-2626-2635 (1)
2626-2635:⚠️ Potential issue | 🟠 MajorPut TaskManager teardown behind
withTimeout()and always clear the ref.Both cleanup paths only clear
clearTaskManagerRef()/_taskManagerafter a successfulawait this._taskManager.shutdown(). If backend teardown hangs or rejects, shutdown can stall and the global task-tool hook keeps pointing at a half-closed manager. Move the ref reset intofinallyand bound the await withwithTimeout(...). As per coding guidelines "Wrap async operations with withTimeout utility".Also applies to: 11694-11709
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 2626 - 2635, The TaskManager teardown should be made timeout-safe and must always clear the global ref: wrap the shutdown call in withTimeout(this._taskManager.shutdown(), ...) to bound waits and move clearTaskManagerRef() plus setting this._taskManager = undefined into a finally block so the ref is cleared regardless of success or failure; update the block that currently references this._taskManager, clearTaskManagerRef(), and logger.debug/logger.warn to use withTimeout(...) and perform the ref reset in finally (apply same change to the other occurrence around lines 11694-11709).src/lib/neurolink.ts-845-855 (1)
845-855:⚠️ Potential issue | 🟠 MajorEagerly publish the TaskManager ref when tasks are enabled.
initializeMCP()registersdirectToolsServerduring normalgenerate()/stream()flows, but the only place that callssetTaskManagerRef()is this getter. A caller can configuretasksand go straight to generation without ever touchingneurolink.tasks, which leaves the task tools unbound on first use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 845 - 855, The TaskManager reference is only published inside the tasks getter, so callers that enable tasks via configuration and then call generate()/stream() never trigger setTaskManagerRef(), leaving directToolsServer unbound; modify initializeMCP() (the MCP initialization path used by generate()/stream()) to detect when tasks are enabled and, if this._taskManager exists or when creating a TaskManager from this._taskManagerConfig, call setTaskManagerRef(this._taskManager) (or create and assign this._taskManager then setTaskManagerRef) so the TaskManager ref is eagerly published; ensure you use the existing TaskManager constructor and setEmitter logic (TaskManager, this._taskManagerConfig, setEmitter, emitter) when creating/publishing the ref.src/lib/neurolink.ts-3418-3419 (1)
3418-3419:⚠️ Potential issue | 🟠 Major
conversationHistoryis dropped on the main generate path.The workflow branches below still fall back to
options.conversationHistory, butbaseOptionsonly forwardsconversationMessages. Sogenerate({ conversationHistory: [...] })silently loses its prior turns everywhere except the workflow path.Possible fix
- // Pass through conversation messages for task continuation and external callers - conversationMessages: options.conversationMessages, + // Pass through conversation messages for task continuation and + // keep the deprecated conversationHistory path working. + conversationMessages: + options.conversationMessages ?? + (options.conversationHistory as ChatMessage[] | 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 3418 - 3419, baseOptions currently forwards conversationMessages but drops conversationHistory, so calls like generate({ conversationHistory: [...] }) lose prior turns; update where baseOptions is constructed (the code that sets conversationMessages: options.conversationMessages) to also include conversationHistory: options.conversationHistory (or merge conversationHistory into conversationMessages if the code expects a unified field) so both the main generate path and the workflow branches receive the same conversation history; ensure references to conversationHistory in downstream functions (e.g., generate, any workflow branching logic) consume the populated field consistently.src/lib/neurolink.ts-7113-7113 (1)
7113-7113:⚠️ Potential issue | 🟠 MajorFallback streaming retries with the wrong conversation history.
createMCPStream()resolves and compacts history onto the enhanced options object, but this fallback call still readsoptions.conversationMessages. When the caller did not pass history explicitly, the retry drops the prepared continuation context and replays as a fresh prompt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` at line 7113, The fallback retry uses the original options.conversationMessages instead of the compacted/updated history produced by createMCPStream; update the fallback call to use the enhanced/returned options (the variable created/returned by createMCPStream, e.g., enhancedOptions or the resolved result) so that conversationMessages is taken from that enhanced object (reference createMCPStream and options.conversationMessages) rather than the original options, ensuring the prepared continuation context is preserved on retries.src/lib/neurolink.ts-3751-3761 (1)
3751-3761:⚠️ Potential issue | 🟠 MajorDon't downcast arbitrary roles into workflow history.
m.role as "user" | "assistant"does not normalize the runtime value. If continuation-mode history containssystem/toolmessages—the same file preserves them elsewhere withrepairToolPairs()—the workflow runner still receives unsupported roles or a JSON-stringified blob instead of structured context.Also applies to: 3869-3879
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 3751 - 3761, The mapping that forces m.role to "user" | "assistant" and JSON-stringifies non-string m.content incorrectly downcasts and loses structured messages (e.g., "system"/"tool"); update the conversationHistory mapping to preserve the original m.role (do not cast to "user" | "assistant") and preserve structured content (return m.content unchanged when it's not a string instead of JSON.stringify), and widen the TypeScript type of conversationHistory to accept the actual role strings (or use a union/unknown role) so runtime roles like "system" or "tool" are passed through intact; apply the same fix to the analogous block around the repairToolPairs usage noted (lines 3869-3879).src/lib/tasks/store/fileTaskStore.ts-188-200 (1)
188-200:⚠️ Potential issue | 🟠 MajorSerialize
flush()calls before writingtasks.json.tmp.Every mutation writes the full snapshot through the same temp file path. Concurrent
save()/update()/delete()calls can clobber each other or race onrename(), losing updates or throwingENOENT. Putflush()behind a write mutex/queue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tasks/store/fileTaskStore.ts` around lines 188 - 200, flush() currently writes to a single temp file (this.storePath + ".tmp") and can race when save()/update()/delete() call it concurrently; serialize flush() calls by introducing a write mutex or queue (e.g., a simple Promise-based lock or an async FIFO) around flush() so only one flush runs at a time, acquire the lock at the start of flush() and release it after rename succeeds (or on error), ensuring operations that call flush() (save, update, delete, or any caller referencing this.tasks and this.storePath) await the lock to prevent concurrent writes and ENOENT/race conditions.test/test-task-bullmq.mjs-385-389 (1)
385-389:⚠️ Potential issue | 🟠 MajorPoll for the delayed run instead of sleeping 5 seconds.
A hard
sleep(5000)is brittle here: queue startup, Redis jitter, or model latency can push the run past five seconds and make this test intermittently fail. Pollneurolink.tasks.runs(task.id)until a deadline instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-task-bullmq.mjs` around lines 385 - 389, Replace the hard sleep with a polling loop that repeatedly calls neurolink.tasks.runs(task.id) until either runs.length >= 1 or a deadline is reached; implement a short interval (e.g., 100–500ms) between attempts, set a sensible timeout (e.g., 10–30s), and fail the test if the deadline elapses while still calling report("once: auto-executed", runs.length >= 1, `runs: ${runs.length}`) with the final runs count; update the block around sleep(5000) to use this poll strategy so startup/Redis/model jitter won't cause intermittent failures.test/test-task-edge-cases.mjs-28-46 (1)
28-46:⚠️ Potential issue | 🟠 MajorDon't delete the default task storage from tests.
resetState()hard-removes.neurolink/tasks, which is the default real storage location. Running this script locally can destroy a developer's existing task data. Use a per-test tempstorePath/logsPathinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-task-edge-cases.mjs` around lines 28 - 46, The resetState() function currently force-deletes the default ".neurolink/tasks" directory; change tests to use per-test temporary storage and stop deleting the global default: update freshNeuroLink(opts) to set and return a NeuroLink instance with unique temp paths (e.g., storePath/logsPath) when not supplied, and modify resetState() to only rmSync those temp paths (or track and remove instances created via freshNeuroLink) instead of hard-removing ".neurolink/tasks"; ensure all tests call freshNeuroLink with the temp paths so cleanup is safe and the global default storage is never touched.src/lib/tasks/backends/nodeTimeoutBackend.ts-38-49 (1)
38-49:⚠️ Potential issue | 🟠 Major
isHealthy()should reflect shutdown state.After
shutdown(), this backend still reports healthy forever. That makes lifecycle checks meaningless and diverges fromBullMQBackend, which becomes unhealthy after close. Track initialized/shutdown state and returnfalseonce disposed.Also applies to: 131-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tasks/backends/nodeTimeoutBackend.ts` around lines 38 - 49, The backend currently never flips health after shutdown; update the lifecycle to track an "initialized/closed" boolean: set a private flag (e.g. this.initialized = true) in initialize() and clear/set a disposed flag (e.g. this.initialized = false or this.closed = true) at the end of shutdown() after clearing scheduled/paused entries and calling clearEntry; then make isHealthy() return false when disposed/closed (and true only when initialized and not closed). Apply the same pattern to the other similar class/methods referenced around lines 131-133 so it mirrors BullMQBackend's behavior.test/test-task-bullmq.mjs-40-53 (1)
40-53:⚠️ Potential issue | 🟠 MajorIsolate Redis cleanup from developer data.
cleanRedis()runs against DB0and deletes everyneurolink:*/bull:neurolink*key it finds. On a shared local Redis, that can wipe unrelated tasks and queues. Point the test at a dedicated Redis DB or a randomized key prefix instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-task-bullmq.mjs` around lines 40 - 53, The cleanRedis() helper currently connects to DB 0 and calls client.keys("neurolink:*") / client.keys("bull:neurolink*") which can delete unrelated data; update cleanRedis() to target an isolated namespace by either connecting to a dedicated Redis DB (change createClient URL to e.g. redis://localhost:6379/<dedicatedDb>) or accept and use a randomized/test-only key prefix parameter (replace "neurolink:" and "bull:neurolink" with that prefix) so only test-owned keys are deleted; ensure the function signature (cleanRedis) and any callers pass/select the dedicated DB or prefix and that client.connect() still occurs before key deletions.test/test-task-edge-cases.mjs-378-413 (1)
378-413:⚠️ Potential issue | 🟠 MajorThe history-bounds test never checks the bound.
A broken trim in
appendHistory()would still pass here, because the only assertion is that run 5 succeeds. If this is meant to covermaxHistoryEntries, assert the stored history length via the store or an exposed diagnostic hook.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-task-edge-cases.mjs` around lines 378 - 413, The testHistoryBounds test currently only verifies run success but never asserts that trimming occurred; update testHistoryBounds to fetch the stored history after the 4 runs and assert its length equals the configured maxHistoryEntries (4) and that it contains the last exchanges (i.e. last 4 entries), by reading the internal history via whatever diagnostic/store accessor exists on neurolink (e.g. neurolink.tasks.get(task.id).history or a debug hook like neurolink._debug.getHistory(task.id)); if no accessor exists, add a temporary test-only diagnostic method to expose history or adjust appendHistory to expose trimmed length for verification, then assert the bound so the test actually covers appendHistory/maxHistoryEntries trimming.src/lib/tasks/store/fileTaskStore.ts-120-123 (1)
120-123:⚠️ Potential issue | 🟠 MajorDelete the JSONL run log with the task.
delete()removes the task record and in-memory history, but leaves${logsPath}/${taskId}.jsonlbehind. That meansdeleteTaskis not actually permanent and prior run outputs can remain on disk and still be returned bygetRuns().Also applies to: 135-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tasks/store/fileTaskStore.ts` around lines 120 - 123, The delete(taskId: string) method currently removes in-memory state but leaves the on-disk run log `${logsPath}/${taskId}.jsonl`; update delete(taskId) to also remove that file (use fs.promises.unlink or equivalent) and swallow/ignore ENOENT so missing files aren’t treated as failures, then await flush() as before; apply the same fix to the other task-deleting method(s) in this file (the second delete/clear routine that handles multiple/older tasks around the same area) so getRuns() no longer returns leftover JSONL files.src/lib/tasks/tools/taskTools.ts-39-60 (1)
39-60:⚠️ Potential issue | 🟠 MajorValidate
schedulebytypebefore creating the task.The schema currently accepts payloads like
{ type: "interval" }or an invalidonce.at, andparseSchedule()just casts those values through. That pushes bad input into the backend, where it fails later or can run immediately on an invalid date. Make each schedule variant require its own fields and bounds up front.Also applies to: 67-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tasks/tools/taskTools.ts` around lines 39 - 60, parseSchedule currently casts fields through and allows incomplete/invalid schedules; update parseSchedule to validate required fields per type: for "cron" require a non-empty expression (optionally validate cron syntax) and an optional timezone string; for "interval" require every to be a finite number > 0; for "once" require at to be a valid ISO timestamp (and reject past/invalid dates). Throw clear errors identifying the missing/invalid field (e.g., "interval.every must be a positive number"), and make the same strict validations for the analogous schedule-parsing logic elsewhere in this file (the other schedule parser block).src/lib/tasks/backends/bullmqBackend.ts-226-230 (1)
226-230: 🛠️ Refactor suggestion | 🟠 MajorUse
ErrorFactoryfor backend-state errors.This raw
Errorbypasses the repository's typed-error convention for source code.
As per coding guidelines,src/**/*.ts: Use ErrorFactory for creating typed errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tasks/backends/bullmqBackend.ts` around lines 226 - 230, Replace the raw throw in ensureInitialized with the repository's typed error by using ErrorFactory: import ErrorFactory (or the appropriate factory export) and throw ErrorFactory.create(...) (or the specific factory method for backend/state errors) with the same descriptive message; update the ensureInitialized method in the BullMQ backend (ensureInitialized in the BullMQBackend class in bullmqBackend.ts) to construct and throw the typed ErrorFactory error instead of new Error, and add the necessary import for ErrorFactory at the top of the file.src/lib/tasks/store/fileTaskStore.ts-102-106 (1)
102-106: 🛠️ Refactor suggestion | 🟠 MajorUse
ErrorFactoryfor missing-task errors.A raw
Errorhere makes it harder for callers to distinguish "task not found" from storage failures. Keep store errors typed.
As per coding guidelines,src/**/*.ts: Use ErrorFactory for creating typed errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tasks/store/fileTaskStore.ts` around lines 102 - 106, The update method currently throws a raw Error when a task is missing (inside update, after this.tasks.get(taskId)); replace that with a typed error from ErrorFactory: import ErrorFactory and throw an appropriate not-found error (e.g., ErrorFactory.createNotFoundError or ErrorFactory.notFound(...) depending on your ErrorFactory API) with context ("Task" and taskId) so callers can distinguish missing-task from storage failures; keep the rest of update logic unchanged.test/test-task-bullmq.mjs-420-430 (1)
420-430:⚠️ Potential issue | 🟠 MajorGate provider-dependent cases too.
The preflight only checks Redis, but multiple tests below call
tasks.run()withprovider: "vertex". On any machine without that provider configured, the suite fails for environment reasons instead of BullMQ regressions. Add a provider preflight or swap these cases to a deterministic fake provider.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-task-bullmq.mjs` around lines 420 - 430, The tests currently only preflight Redis but later call tasks.run() with provider: "vertex", which causes environment-dependent failures; add a provider preflight that checks the Vertex (or other configured) provider is available before running those cases (and skip with a clear message if not), or change the affected test cases to use a deterministic stub/fake provider instead of "vertex" (modify the test cases that pass provider: "vertex" to either guard with a isProviderConfigured('vertex') check or replace the provider value with a test/fake provider and corresponding mocked behavior so the suite is deterministic).test/test-task-edge-cases.mjs-121-156 (1)
121-156:⚠️ Potential issue | 🟠 MajorThis doesn't reliably exercise the retry path.
The task uses the default Ollama endpoint, so it may succeed on machines where Ollama is running, and none of the assertions prove that a second attempt actually happened. Point the provider at a guaranteed-dead endpoint and assert on elapsed time or attempt count.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-task-edge-cases.mjs` around lines 121 - 156, testRetryOnTransientError currently may hit a real Ollama server and doesn't prove retries; update the task creation in testRetryOnTransientError to point the provider at a guaranteed-dead endpoint (e.g., explicit providerOptions/baseUrl or host: non-routable IP/unused port) so the first call will fail, keep retry: { maxAttempts: 2, backoffMs: [100] }, then after neurolink.tasks.run(task.id) assert that a retry actually occurred by checking elapsed time (expect elapsed >= first backoff, e.g., >100ms) and/or verifying an attempt count property returned by the run result (e.g., run.attempts or similar task execution metadata) to ensure the retry path was exercised.src/lib/tasks/types.ts-12-319 (1)
12-319: 🛠️ Refactor suggestion | 🟠 MajorMove exported task types under
src/lib/types/.This file introduces a new shared/public type surface outside the repository's canonical types directory. Keeping task types here fragments the type system and bypasses the standard re-export path.
Based on learnings, "Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files." and "all new type definitions must be placed in src/lib/types/."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tasks/types.ts` around lines 12 - 319, Move all exported task-related type and interface declarations (e.g., TaskScheduleType, CronSchedule, IntervalSchedule, OnceSchedule, TaskSchedule, TaskExecutionMode, TaskStatus, TaskDefinition, Task, TaskRunResult, TaskRunError, TaskStore, ConversationEntry, TaskExecutorFn, TaskBackend, TaskBackendName, TaskBackendFactoryFn, TaskRetentionConfig, TaskManagerConfig) into the shared types module used by the project; then update this file to import those types from the new shared types module (or re-export them) instead of declaring them locally, remove duplicated type declarations here, and ensure any code referencing TASK_DEFAULTS, TASK_DEFAULTS members, or the moved types now import the types from the centralized types module so the public type surface is only under the canonical types area.src/lib/tasks/backends/bullmqBackend.ts-208-215 (1)
208-215:⚠️ Potential issue | 🟠 MajorPreserve full Redis URL semantics.
When
redis.urlis provided, this parser throws away ACL usernames,rediss://TLS, and any other URL options by reducing it to{ host, port, password, db }. Managed Redis URLs that rely on those fields will not behave the same as the advertisedurloption.For BullMQ / ioredis connection configs, what connection properties must be preserved from a Redis URL (for example `rediss://` TLS and ACL usernames), and is decomposing the URL into only `{ host, port, password, db }` sufficient?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tasks/backends/bullmqBackend.ts` around lines 208 - 215, The current branch in the redis.url handling (variable redis.url, parsed = new URL(redis.url)) strips ACL usernames, the scheme (e.g., rediss://) and any query options by returning only { host, port, password, db }; instead, preserve full Redis URL semantics by returning the original connection string (e.g., { url: redis.url }) or include all URL-derived fields (username from parsed.username, password from parsed.password, protocol/scheme to infer TLS for rediss, and any query params) so BullMQ/ioredis receives the complete connection info; update the code path that currently returns { host, port, password, db } to either return { url: redis.url } or construct an options object containing parsed.username, parsed.password, protocol/secure flag for rediss, and any parsed.searchParams in addition to host/port/db.
🟡 Minor comments (7)
test/test-task-production.mjs-29-43 (1)
29-43:⚠️ Potential issue | 🟡 MinorAccessing private property
_taskManageris fragile.Line 31 accesses
neurolink._taskManagerwhich appears to be an internal implementation detail. If the internal structure changes, this will break.Consider using the public API to check initialization
async function resetState() { try { - if (neurolink?._taskManager) { + if (neurolink) { await neurolink.tasks.shutdown(); } } catch { /* expected */ }Alternatively, the
NeuroLinkclass could expose anisTaskManagerInitialized()method if this check is commonly needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-task-production.mjs` around lines 29 - 43, In resetState(), stop checking the private field `_taskManager`; instead use the public API to detect and shutdown the task manager—e.g., check that `neurolink?.tasks` exists and exposes a shutdown function (`typeof neurolink.tasks.shutdown === 'function'`) before calling `await neurolink.tasks.shutdown()`, or add and call a public `isTaskManagerInitialized()` method on the NeuroLink class and use that (replace references to `_taskManager` with the public check) so the test no longer depends on internal implementation.test/test-task-continuation.mjs-93-96 (1)
93-96:⚠️ Potential issue | 🟡 MinorSame
finallycontrol flow issue as other test files.The unconditional
process.exit(0)infinallycan mask failures from thecatchblock.Proposed fix
if (runs.length < 2) { console.error("FAIL: Expected at least 2 run logs, got", runs.length); process.exit(1); } + process.exit(0); } catch (e) { console.error("FAIL:", e.message); process.exit(1); } finally { await neurolink.tasks.shutdown(); - process.exit(0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-task-continuation.mjs` around lines 93 - 96, The finally block currently unconditionally calls process.exit(0) after awaiting neurolink.tasks.shutdown(), which masks failures; change the control flow so the shutdown call remains guaranteed but process.exit(0) is only invoked on success and non-zero exit (or rethrow) happens on error: capture any thrown error in the surrounding try/catch, move the process.exit(0) into the successful path (or call process.exit(1) in the catch), and ensure neurolink.tasks.shutdown() is still awaited in a finally-like cleanup step (keep the shutdown call but remove the unconditional process.exit(0) from the finally block).test/test-task-execute.mjs-52-55 (1)
52-55:⚠️ Potential issue | 🟡 MinorUnconditional
process.exit(0)infinallycan mask test failures.If an exception is thrown, the
catchblock callsprocess.exit(1), but thefinallyblock then executesprocess.exit(0). Whileprocess.exit()is typically synchronous, this pattern is confusing and fragile.Proposed fix: remove exit from finally, add explicit success exit
if (runs.length !== 1) { console.error("FAIL: Expected 1 run log, got", runs.length); process.exit(1); } console.info("\n✅ Level 4: Integration test passed!"); + process.exit(0); } catch (e) { console.error("FAIL:", e.message); process.exit(1); } finally { await neurolink.tasks.shutdown(); - process.exit(0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-task-execute.mjs` around lines 52 - 55, The finally block unconditionally calls process.exit(0) which can override the catch's process.exit(1) and mask failures; remove the process.exit(0) from the finally block and instead call process.exit(0) explicitly only on successful completion (for example, after the main try completes or after awaiting neurolink.tasks.shutdown() when no error was thrown). Update the code around neurolink.tasks.shutdown() so shutdown always runs in finally but exit codes are set outside of finally (use process.exit(1) in catch and process.exit(0) after successful completion).test/test-task-tools.mjs-60-63 (1)
60-63:⚠️ Potential issue | 🟡 MinorSame
finallycontrol flow issue.Proposed fix
} else { console.info( "Available tools:", result.availableTools?.map((t) => t.name).join(", "), ); console.error("FAIL: Task tools not found in available tools."); process.exit(1); } } + process.exit(0); } catch (e) { console.error("FAIL:", e.message); process.exit(1); } finally { await neurolink.tasks.shutdown(); - process.exit(0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-task-tools.mjs` around lines 60 - 63, The finally block unconditionally calls process.exit(0) after awaiting neurolink.tasks.shutdown(), which masks errors and prevents proper test harness handling; replace this pattern by removing process.exit(0) from the finally, ensure neurolink.tasks.shutdown() is awaited in either a catch or finally, and allow errors to propagate (or explicitly rethrow) so test failure is visible—locate the finally containing neurolink.tasks.shutdown() and process.exit(0) and change control flow to await neurolink.tasks.shutdown() then either rethrow the caught error or return normally, leaving any process exit decision to the caller or test runner.src/cli/commands/task.ts-598-608 (1)
598-608:⚠️ Potential issue | 🟡 MinorMissing mutual exclusivity check for schedule flags in update.
Unlike
create(lines 126-140), theupdatecommand doesn't validate that only one of--cron,--every, or--atis specified. If a user provides multiple schedule flags, only the first matching one (cron → every → at) is applied silently.🛡️ Suggested fix
+ // Validate schedule mutual exclusivity + const scheduleFlags = [argv.cron, argv.every, argv.at].filter(Boolean); + if (scheduleFlags.length > 1) { + console.info(chalk.red("Only one of --cron, --every, or --at can be used")); + await manager.shutdown(); + process.exit(1); + } + // Build schedule if any schedule flag is provided if (argv.cron) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/task.ts` around lines 598 - 608, The update schedule block (handling argv.cron, argv.every, argv.at and setting updates.schedule) must enforce mutual exclusivity like the create command does: detect if more than one of argv.cron, argv.every, argv.at is provided and surface a clear error (throw or exit with message) instead of silently choosing the first; add this check before the existing if/else and reuse the same validation behavior/pattern used in the create flow so callers get a clear error when multiple schedule flags are supplied.docs/features/task-manager.md-674-676 (1)
674-676:⚠️ Potential issue | 🟡 MinorMinor inconsistency:
conversationHistoryis deprecated but referenced.The documentation mentions passing
conversationHistorytogenerate(), but based on the PR summary,conversationHistoryis deprecated in favor ofconversationMessages. Consider updating this reference.-4. On subsequent runs, the full history is loaded and passed as `conversationHistory` to `generate()` +4. On subsequent runs, the full history is loaded and passed as `conversationMessages` to `generate()`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/task-manager.md` around lines 674 - 676, Update the docs to stop referencing the deprecated conversationHistory and instead reference conversationMessages when describing what is passed to generate(); change the wording in the steps (lines mentioning "passed as `conversationHistory` to `generate()`") to "`conversationMessages`" and, if helpful, add a brief parenthetical noting that conversationHistory is deprecated in favor of conversationMessages so readers know to migrate.src/lib/tasks/store/redisTaskStore.ts-152-168 (1)
152-168:⚠️ Potential issue | 🟡 MinorPost-fetch filtering may return fewer results than
limit.The method fetches
limititems from Redis, then applies the status filter in-memory. If many runs have different statuses, the result set could be much smaller than requested. Consider documenting this behavior or fetching more items when a filter is applied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tasks/store/redisTaskStore.ts` around lines 152 - 168, getRuns currently reads a fixed lRange window then applies an in-memory status filter, which can return fewer than the requested limit when options.status is set; update getRuns (referencing getRuns, taskRunsKey, this.client.lRange and this.client.lLen) so that when options.status is provided it requests additional items from Redis in batches (e.g., increase the lRange end or fetch until lLen exhausted) and accumulates matching TaskRunResult entries until either you have `limit` matches or no more items remain; ensure you still parse JSON (JSON.parse) and preserve the existing behavior when options.status is not provided.
🧹 Nitpick comments (10)
test/test-task-all.mjs (1)
101-106: Redundant environment variable setting.
NEUROLINK_SKIP_MCP=trueis set both as a command prefix and in theenvobject. Theenvobject alone is sufficient.Proposed fix
- const output = execSync(`NEUROLINK_SKIP_MCP=true node test/${suite.file}`, { + const output = execSync(`node test/${suite.file}`, { cwd: process.cwd(), timeout: 600_000, // 10 min per suite stdio: ["pipe", "pipe", "pipe"], env: { ...process.env, NEUROLINK_SKIP_MCP: "true" }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-task-all.mjs` around lines 101 - 106, The execSync invocation in test/test-task-all.mjs redundantly sets NEUROLINK_SKIP_MCP twice (as a command prefix and inside the env object); remove the shell prefix ("NEUROLINK_SKIP_MCP=true " before node) and rely solely on the env parameter in the execSync call so the environment is passed via env: { ...process.env, NEUROLINK_SKIP_MCP: "true" }; update the execSync call that assigns output accordingly (the invocation around execSync(...) that runs node test/${suite.file}) to avoid the duplicated setting.test/test-task-production.mjs (1)
326-328: Magic delay for async callbacks could be flaky.The 100ms
sleepassumes callbacks complete within that time, which may not hold under load or on slower systems.Consider polling with a timeout instead:
// Wait up to 500ms for callbacks for (let i = 0; i < 50 && !successCalled; i++) await sleep(10);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-task-production.mjs` around lines 326 - 328, Replace the single fixed await sleep(100) with a polling loop that waits until the callback flag (e.g., successCalled) is true or a timeout elapses; specifically, poll in short increments (e.g., 10ms) up to a max (e.g., 500ms) to avoid flaky tests. Locate the current await sleep(100) and the callback flag variables (successCalled / any other result flags) in the test/test-task-production.mjs test and implement the for-loop that checks the flag each iteration and awaits small sleeps, breaking early when the flag becomes true.test/test-task-manager.mjs (1)
185-190: Late import after code execution is unusual but valid.The
import { rmSync } from "fs"appears after test execution code. While valid in ESM (imports are hoisted), placing all imports at the top improves readability.Move import to top of file
+import { rmSync } from "fs"; import { NeuroLink } from "../dist/index.js"; // ... rest of file ... -// Clean up file store so next run starts fresh -import { rmSync } from "fs"; +// Clean up file store so next run starts fresh try { rmSync(".neurolink/tasks", { recursive: true, force: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-task-manager.mjs` around lines 185 - 190, Move the late import of rmSync to the top of the module to improve readability and follow convention: relocate the line "import { rmSync } from 'fs'" so it appears with the other top-level imports in test-task-manager.mjs, leaving the try { rmSync(...); } catch { } block unchanged (refer to the rmSync usage in the cleanup block).src/cli/commands/task.ts (2)
323-371: Consider type-safe access to the emitter.The current implementation uses unsafe type casting to access the internal
emitter. Consider exposing a typed method on NeuroLink (e.g.,neurolink.on(event, handler)) to avoid internal coupling.// Current (unsafe cast) const emitter = (neurolink as unknown as { emitter: ... }).emitter; // Suggested: If NeuroLink already exposes event subscription neurolink.on("task:completed", (result) => { ... });This would be cleaner if NeuroLink already provides a public event subscription API, which the documentation suggests it does (line 852-858 in docs).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/task.ts` around lines 323 - 371, The enterWorkerMode function currently casts neurolink to access a private emitter; replace this unsafe cast by using a typed public event API on NeuroLink (e.g., call neurolink.on("task:completed", handler) and neurolink.on("task:failed", handler)) or, if the API doesn't exist, add a small typed wrapper method on the NeuroLink class (e.g., on(event: string, handler: (payload: unknown) => void)) and call that from enterWorkerMode so you no longer reference emitter directly; update the handler signatures to use the proper typed result shapes (taskId, output, durationMs, error) and remove the (neurolink as unknown as { emitter: ... }).emitter cast.
420-422: Document the blocking behavior in CLI help.The
createcommand enters worker mode and blocks forever. This is mentioned in the command description at line 298 forstart, but not explicitly forcreate. Users might expectcreateto just create and exit.Consider either:
- Adding clarification to the
createcommand description- Or adding a
--detachflag for non-blocking creation🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/task.ts` around lines 420 - 422, The create command currently calls TaskCommandFactory.enterWorkerMode(neurolink, manager) and then awaits a never-resolving Promise, causing the CLI to block forever; update the create command help/description to explicitly state this blocking worker mode behavior (similar to the start command description) and/or implement an optional --detach (or --no-wait) flag on the create command that, when provided, skips calling TaskCommandFactory.enterWorkerMode and the await new Promise(() => {}) so the command returns immediately; ensure the help text, flag parsing, and any related usage strings reference the new flag and behavior so users understand the default blocking behavior and how to run non-blocking creation.docs/features/task-manager.md (1)
863-895: Optional: Consider resetting list numbering within each phase.Static analysis flagged the ordered list prefixes (MD029). The current continuous numbering (1-16 across phases) is intentional for tracking overall progress, but some markdown processors and renderers may auto-number based on list structure. If strict markdown compliance is desired:
♻️ Suggested format (optional)
Reset numbering within each phase, e.g.:
- Phase 1: 1, 2, 3, 4, 5, 6
- Phase 2: 1, 2
- Phase 3: 1, 2, 3
- etc.
Or use a different format like task IDs (e.g.,
P1.1,P2.1) to preserve overall sequencing while satisfying linting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/task-manager.md` around lines 863 - 895, Reset the ordered lists inside each phase header (e.g., "Phase 1: Core Infrastructure", "Phase 2: Backends", "Phase 3: Orchestration", etc.) so numbering restarts at 1 for each phase to satisfy MD029; alternatively convert item prefixes to phase-scoped IDs (e.g., P1.1, P2.1) to preserve global sequencing while avoiding continuous 1..16 numbering across the whole section.src/lib/tasks/taskManager.ts (1)
214-252: Consider consolidating field updates to reduce repetition.The explicit if-checks work correctly but are repetitive. A whitelist-based approach would be more maintainable.
♻️ Optional consolidation
const ALLOWED_UPDATE_FIELDS = [ "prompt", "schedule", "mode", "provider", "model", "thinkingLevel", "systemPrompt", "tools", "maxTokens", "temperature", "maxRuns", "timeout", "metadata" ] as const; const taskUpdates: Partial<Task> = {}; for (const field of ALLOWED_UPDATE_FIELDS) { if (updates[field] !== undefined) { taskUpdates[field] = updates[field]; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tasks/taskManager.ts` around lines 214 - 252, Replace the repetitive per-field if-checks with a whitelist-driven loop: define a constant array (e.g., ALLOWED_UPDATE_FIELDS) containing the allowed keys ("prompt","schedule","mode","provider","model","thinkingLevel","systemPrompt","tools","maxTokens","temperature","maxRuns","timeout","metadata"), initialize taskUpdates as Partial<Task>, then iterate the whitelist and assign taskUpdates[field] = updates[field] only when updates[field] !== undefined; update the function that currently constructs taskUpdates (referenced by the variable name taskUpdates and the incoming updates parameter) to use this loop to reduce repetition and ensure only allowed fields are copied.src/lib/tasks/taskExecutor.ts (2)
164-165: Consider wrappinggenerate()call withwithTimeoututility.The timeout value is passed as an option within
generateOptions, but per coding guidelines, async operations should be wrapped with thewithTimeoututility for consistent timeout handling.♻️ Example refactor
+import { withTimeout } from "../utils/withTimeout.js"; // Execute -const result = await this.neurolink.generate(generateOptions); +const result = await withTimeout( + this.neurolink.generate(generateOptions), + task.timeout ?? TASK_DEFAULTS.timeout, + `Task execution timed out: ${task.id}` +);As per coding guidelines: "Wrap async operations with withTimeout utility".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tasks/taskExecutor.ts` around lines 164 - 165, The call to this.neurolink.generate(generateOptions) should be wrapped with the withTimeout utility to enforce consistent timeout behavior; replace the direct await this.neurolink.generate(generateOptions) with an await to withTimeout(this.neurolink.generate(generateOptions), timeout) (using the timeout value from generateOptions or a derived value), and ensure errors/timeouts are handled the same way as other withTimeout usages in the codebase so the result assignment (const result = ...) remains correct and exceptions propagate consistently.
178-183: Minor: Fallback toinputTokens/outputTokensis unnecessary.Per
src/lib/types/analytics.ts:11-19, the actualTokenUsagetype only hasinputandoutputfields —inputTokens/outputTokensdon't exist. The fallback works correctly but is dead code.🧹 Simplified version
tokensUsed: result.usage ? { - input: result.usage.input ?? result.usage.inputTokens ?? 0, - output: result.usage.output ?? result.usage.outputTokens ?? 0, + input: result.usage.input ?? 0, + output: result.usage.output ?? 0, } : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tasks/taskExecutor.ts` around lines 178 - 183, The current tokensUsed construction references non-existent fields inputTokens/outputTokens on result.usage; remove that dead fallback and simplify the logic in the tokensUsed assignment (inside taskExecutor.ts) to only read TokenUsage's real fields (result.usage.input and result.usage.output) with a safe default (e.g. ?? 0) when result.usage is present; update the expression that builds tokensUsed accordingly (referencing tokensUsed and result.usage) so it no longer checks inputTokens/outputTokens.src/lib/tasks/store/redisTaskStore.ts (1)
226-231: Silent error swallowing hinders debugging.The catch blocks swallow errors without any logging. Even for best-effort operations, a debug-level log would help diagnose TTL failures in production.
♻️ Proposed fix to add debug logging
this.client!.expire(taskRunsKey(task.id), ttlSeconds).catch((_err) => { - // Best-effort TTL — Redis may be temporarily unavailable + logger.debug("[TaskStore:Redis] Failed to set TTL on runs key", { + taskId: task.id, + error: String(_err), + }); }); this.client!.expire(taskHistoryKey(task.id), ttlSeconds).catch((_err) => { - // Best-effort TTL — Redis may be temporarily unavailable + logger.debug("[TaskStore:Redis] Failed to set TTL on history key", { + taskId: task.id, + error: String(_err), + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tasks/store/redisTaskStore.ts` around lines 226 - 231, The catch blocks for the best-effort TTL calls on this.client!.expire (for taskRunsKey(task.id) and taskHistoryKey(task.id)) currently swallow errors; update each .catch to log the failure at debug level (e.g., this.logger.debug or the existing store logger) including the key name (taskRunsKey/taskHistoryKey), ttlSeconds and the caught error object, but do not rethrow — preserve best-effort behavior while surfacing diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a27d4ef8-ab6f-4400-bf6f-f3f66e785a6a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
.gitignoredocs/features/task-manager.mdpackage.jsonscripts/build-browser.mjssrc/cli/commands/task.tssrc/cli/parser.tssrc/lib/agent/directTools.tssrc/lib/neurolink.tssrc/lib/tasks/backends/bullmqBackend.tssrc/lib/tasks/backends/nodeTimeoutBackend.tssrc/lib/tasks/backends/taskBackend.tssrc/lib/tasks/backends/taskBackendRegistry.tssrc/lib/tasks/index.tssrc/lib/tasks/store/fileTaskStore.tssrc/lib/tasks/store/redisTaskStore.tssrc/lib/tasks/store/taskStore.tssrc/lib/tasks/taskExecutor.tssrc/lib/tasks/taskManager.tssrc/lib/tasks/tools/taskTools.tssrc/lib/tasks/types.tssrc/lib/types/configTypes.tssrc/lib/types/generateTypes.tssrc/lib/types/index.tstest/test-task-all.mjstest/test-task-bullmq.mjstest/test-task-continuation.mjstest/test-task-edge-cases.mjstest/test-task-execute.mjstest/test-task-manager.mjstest/test-task-production.mjstest/test-task-tools.mjs
| export type NeuroLinkExecutable = { | ||
| generate(optionsOrPrompt: unknown): Promise<{ | ||
| content: string; | ||
| toolExecutions?: Array<{ name: string; input: unknown; output: unknown }>; | ||
| usage?: { | ||
| input?: number; | ||
| output?: number; | ||
| inputTokens?: number; | ||
| outputTokens?: number; | ||
| }; | ||
| }>; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the actual return type structure of generate()
ast-grep --pattern $'toolExecutions?: Array<{
$$$
}>'Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Read the NeuroLinkExecutable interface from taskExecutor.ts
cat -n src/lib/tasks/taskExecutor.ts | head -40Repository: juspay/neurolink
Length of output: 1379
🏁 Script executed:
# Look for the mapping code around lines 173-177
cat -n src/lib/tasks/taskExecutor.ts | sed -n '170,180p'Repository: juspay/neurolink
Length of output: 460
🏁 Script executed:
# Search for the actual generate() return type definition in the codebase
rg "toolExecutions" -A 3 -B 1 src/lib/types/Repository: juspay/neurolink
Length of output: 2232
🏁 Script executed:
# Check for TokenUsage type definition
rg "type TokenUsage" -A 10 src/lib/types/Repository: juspay/neurolink
Length of output: 557
🏁 Script executed:
# Find where generate() is implemented or where NeuroLinkExecutable is used
rg "generate\(" src/lib/ -B 2 -A 5 | head -100Repository: juspay/neurolink
Length of output: 6081
🏁 Script executed:
# Check if NeuroLinkExecutable is implemented somewhere
rg "NeuroLinkExecutable" src/lib/ -B 2 -A 10Repository: juspay/neurolink
Length of output: 3072
🏁 Script executed:
# Look at the full generateTypes.ts to understand which toolExecutions structure is the actual return type
cat -n src/lib/types/generateTypes.ts | head -80Repository: juspay/neurolink
Length of output: 3331
🏁 Script executed:
# Check what the client/generate function signature looks like
rg "type.*Result|interface.*Result" src/lib/types/generateTypes.ts -A 10Repository: juspay/neurolink
Length of output: 1951
🏁 Script executed:
# Look at TextGenerationResult more completely
rg "type TextGenerationResult" -A 20 src/lib/types/generateTypes.tsRepository: juspay/neurolink
Length of output: 527
🏁 Script executed:
# Look at the cli.ts version mentioned in the review (around lines 402-427)
cat -n src/lib/types/cli.ts | sed -n '395,430p'Repository: juspay/neurolink
Length of output: 1238
🏁 Script executed:
# Check what GenerateResult actually returns
rg "type GenerateResult" -A 30 src/lib/types/generateTypes.tsRepository: juspay/neurolink
Length of output: 1093
🏁 Script executed:
# Find where neurolink.generate is actually called in the tasks context
rg "neurolink\.generate|provider\.generate" src/lib/tasks/ -B 3 -A 10Repository: juspay/neurolink
Length of output: 826
Type definition for toolExecutions does not match actual generate() return type.
The NeuroLinkExecutable interface expects toolExecutions with structure { name, input, output }, but actual implementations return { toolName, args, result, executionTime } (as defined in cli.ts:413-418 and generateTypes.ts). The mapping at lines 173-177 will produce undefined values when accessing te.name, te.input, and te.output.
Additionally, the usage fallback at line 180-181 references inputTokens and outputTokens which do not exist in the TokenUsage type (which only has input, output, total).
🐛 Proposed fix to align with actual return type
/** Minimal interface for the NeuroLink SDK methods we need */
export type NeuroLinkExecutable = {
generate(optionsOrPrompt: unknown): Promise<{
content: string;
- toolExecutions?: Array<{ name: string; input: unknown; output: unknown }>;
+ toolExecutions?: Array<{
+ toolName: string;
+ args: unknown;
+ result: unknown;
+ executionTime?: number;
+ }>;
usage?: {
input?: number;
output?: number;
- inputTokens?: number;
- outputTokens?: number;
};
}>;
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/tasks/taskExecutor.ts` around lines 21 - 32, The
NeuroLinkExecutable.generate type is incorrect: update the toolExecutions and
usage shapes to match actual implementations (use fields toolName, args, result,
executionTime for each tool execution and TokenUsage with input, output, total)
and then update any mapping that reads te.name/te.input/te.output to use
te.toolName/te.args/te.result; also fix the usage fallback to reference
usage.input, usage.output, and usage.total (or convert to the expected shape) so
callers and the mapping at generate -> toolExecutions and TokenUsage are
consistent with generate() implementations.
There was a problem hiding this comment.
- toolExecutions shape: generate() returns through transformToolExecutions() (line 30) which outputs { name, input, output, duration }. Our
NeuroLinkExecutable interface expects { name, input, output } — this matches correctly. The { toolName, executionTime, success } shape is only used by
transformToolExecutionsForMCP() which is a separate code path. - usage fields: We already fixed this earlier — the code uses result.usage.input and result.usage.output (lines 178-179), not inputTokens/outputTokens. The
reviewer was looking at an older version.
Both concerns are already addressed. The comment is stale/invalid.
There was a problem hiding this comment.
🧩 Analysis chain
Let me verify both claims against the actual code before responding.
🏁 Script executed:
#!/bin/bash
# Check transformToolExecutions output shape
rg "transformToolExecutions" src/lib/ -B 2 -A 15 --type ts | grep -v "MCP"
# Check lines 170-185 of taskExecutor.ts for actual usage field access
cat -n src/lib/tasks/taskExecutor.ts | sed -n '165,190p'
Length of output: 6864
@swaroopvarma1, you're correct on both points — I've verified against the actual code:
-
toolExecutionsshape:transformToolExecutions()intransformationUtils.tsindeed returns{ name, input, output, duration }, which aligns with theNeuroLinkExecutableinterface expecting{ name, input, output }. The{ toolName, executionTime, success }shape belongs to the separatetransformToolExecutionsForMCP()function. My original analysis conflated the two. -
usagefields: Lines 178–179 confirmresult.usage.input ?? 0andresult.usage.output ?? 0are used — noinputTokens/outputTokenspresent. My comment was based on a stale view of the code.
Both concerns in my original review are invalid. Apologies for the noise!
🧠 Learnings used
Learnt from: CR
Repo: juspay/neurolink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-25T04:03:05.933Z
Learning: Applies to src/lib/workflow/**/*.ts : Use workflow engine with fluent API for sequential task execution and checkpointing
Learnt from: RajuSudhar
Repo: juspay/neurolink PR: 0
File: :0-0
Timestamp: 2025-11-04T22:14:18.719Z
Learning: In the juspay/neurolink repository, new type definitions should use the `type` keyword instead of `interface`, unless there is a valid and justified exception. Flag new interface declarations in code reviews.
Learnt from: RajuSudhar
Repo: juspay/neurolink PR: 0
File: :0-0
Timestamp: 2025-11-04T22:14:18.719Z
Learning: In the juspay/neurolink repository, do not flag existing type or interface definitions located outside src/lib/types/ - these are part of a phased migration plan and will be addressed in upcoming PRs. Only enforce type centralization rules on new code going forward.
| toolCalls: result.toolExecutions?.map((te) => ({ | ||
| name: te.name, | ||
| input: te.input, | ||
| output: te.output, | ||
| })), |
There was a problem hiding this comment.
Tool call mapping uses incorrect property names — will produce undefined values.
Given the type mismatch identified above, this mapping accesses te.name, te.input, te.output but the actual properties are te.toolName, te.args, te.result. All mapped values will be undefined.
🐛 Proposed fix to use correct property names
toolCalls: result.toolExecutions?.map((te) => ({
- name: te.name,
- input: te.input,
- output: te.output,
+ name: te.toolName,
+ input: te.args,
+ output: te.result,
})),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/tasks/taskExecutor.ts` around lines 173 - 177, The toolCalls mapping
is using wrong property names and yields undefined; update the mapping of
result.toolExecutions in taskExecutor.ts so each entry uses the actual
properties: replace te.name → te.toolName, te.input → te.args, and te.output →
te.result (i.e., change the mapping inside the toolCalls:
result.toolExecutions?.map((te) => ({ ... })) to return { name: te.toolName,
input: te.args, output: te.result }).
There was a problem hiding this comment.
Same thing — invalid. As we just confirmed, generate() returns toolExecutions through transformToolExecutions() which normalizes everything to { name, input,
output, duration }. So te.name, te.input, te.output are correct.
The reviewer confused the two different transform functions:
- transformToolExecutions() → { name, input, output } (what generate() returns, what we use)
- transformToolExecutionsForMCP() → { toolName, executionTime, success } (different code path, not used here)
No fix needed.
There was a problem hiding this comment.
🧩 Analysis chain
Let me verify this by checking the actual transformToolExecutions function in the codebase.
🏁 Script executed:
#!/bin/bash
# Find transformToolExecutions function
rg -n "transformToolExecutions" --type=ts -C 5
Length of output: 4189
@swaroopvarma1, you're absolutely right — the code confirms your explanation. transformToolExecutions() normalizes toolExecutions to { name, input, output, duration }, and that's what generate() returns (line 3577 in neurolink.ts). transformToolExecutionsForMCP() (which returns { toolName, executionTime, success }) is a separate function used only in the MCP code path. My original comment was incorrect. No fix is needed here.
✏️ Learnings added
Learnt from: swaroopvarma1
Repo: juspay/neurolink PR: 913
File: src/lib/tasks/taskExecutor.ts:171-175
Timestamp: 2026-03-30T13:39:38.512Z
Learning: In `src/lib/neurolink.ts`, `generate()` returns `toolExecutions` normalized via `transformToolExecutions()` (from `src/lib/utils/transformationUtils.ts`), which produces objects with `{ name: string, input: StandardRecord, output: unknown, duration: number }`. A separate `transformToolExecutionsForMCP()` function returns `{ toolName, executionTime, success }` and is only used in the MCP code path (`tryMCPGeneration`). Do not confuse the two.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: juspay/neurolink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-25T04:03:05.933Z
Learning: Applies to src/lib/workflow/**/*.ts : Use workflow engine with fluent API for sequential task execution and checkpointing
| let taskManagerRef: TaskManager | null = null; | ||
|
|
||
| export function setTaskManagerRef(manager: TaskManager): void { | ||
| taskManagerRef = manager; | ||
| } | ||
|
|
||
| export function clearTaskManagerRef(): void { | ||
| taskManagerRef = null; | ||
| } | ||
|
|
||
| function getManager(): TaskManager { | ||
| if (!taskManagerRef) { | ||
| throw new Error("TaskManager not initialized. Tasks are not available."); | ||
| } | ||
| return taskManagerRef; | ||
| } |
There was a problem hiding this comment.
Don't route task tools through a process-global manager.
taskManagerRef is shared across all NeuroLink instances. Because the tools are registered globally and the ref is only set when some caller touches neurolink.tasks, tool calls can either fail as "not initialized" or hit the wrong manager in multi-instance processes. Bind the manager per instance/tool execution context instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/tasks/tools/taskTools.ts` around lines 18 - 33, The current module
uses a process-global taskManagerRef (setTaskManagerRef, clearTaskManagerRef,
getManager) which causes cross-instance interference for
NeuroLink/neurolink.tasks; refactor the API so task functions are bound to a
TaskManager per NeuroLink instance rather than a shared global: remove reliance
on taskManagerRef and change consumers of getManager (and the exported setters)
to accept or be initialized with a TaskManager (e.g., pass a TaskManager into
the taskTools factory/constructor or into each function that previously called
getManager) so each NeuroLink instance registers/uses its own manager; update
all call sites to construct or bind task tools from that per-instance manager
instead of using the global setters.
🤖 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 |
b29bdad to
7961859
Compare
🤖 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 |
There was a problem hiding this comment.
follow contnius test case patter for test case and remove these tests
| * enabling AI agents to execute prompts on cron, interval, or one-shot schedules. | ||
| */ | ||
|
|
||
| import type { ThinkingLevel } from "../types/configTypes.js"; |
There was a problem hiding this comment.
Types in Types folder and should be logically grouped with the correct Types files.
| @@ -0,0 +1,50 @@ | |||
| /** | |||
There was a problem hiding this comment.
Don't re-export types. Types should be only imported from barrel types.
| * - RedisTaskStore: used with BullMQ backend (production) | ||
| * - FileTaskStore: used with NodeTimeout backend (development) | ||
| */ | ||
| export type { |
There was a problem hiding this comment.
Don't re-export types. Types should be only imported from barrel types. Check this in all the files. Understand how this project follows by turns. Let's make sure we follow the same.
| TASK_DEFAULTS, | ||
| } from "../types.js"; | ||
|
|
||
| type TasksFile = { |
There was a problem hiding this comment.
No inline types like this only type should be available in the types common folder
7961859 to
04145e7
Compare
🤖 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 |
04145e7 to
0d127c6
Compare
🤖 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 |
0d127c6 to
645b4ee
Compare
645b4ee to
3c20ab4
Compare
🤖 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 |
3c20ab4 to
f176909
Compare
🤖 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 |
f176909 to
0b9dc5c
Compare
🤖 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 |
0b9dc5c to
e2f0bf0
Compare
🤖 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 |
|
needs_changes This PR introduces a guaranteed runtime crash on task retries and high-severity path traversal vulnerabilities. Merge is blocked pending resolution.
Also worth fixing:
|
| TASK_DEFAULTS, | ||
| } from "../../types/taskTypes.js"; | ||
|
|
||
| type ScheduledEntry = { |
|
needs_changes Critical path traversal vulnerabilities allow arbitrary file read/write/delete via unsanitized
|
e2f0bf0 to
9f32a2b
Compare
🤖 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 |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
9f32a2b to
0919687
Compare
🤖 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 |
|
🎉 This PR is included in version 9.41.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
conversationMessages)task create(stays alive as worker) andtask start(reconnects after restart),logs --fullconversationMessagesbeing dropped in generate() baseOptions, stream fallback, and workflow paths; added toGenerateOptionstype; deprecatedconversationHistoryTest plan
neurolink task create --every 1m --mode continuationwith live outputNEUROLINK_SKIP_MCP=true node test/test-task-all.mjs🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes