Task tracking system with kanban UI and spec-driven delegation#227
Merged
Task tracking system with kanban UI and spec-driven delegation#227
Conversation
- Added a new SQL migration to create the `tasks` table with relevant fields and indexes. - Introduced task management tools for creating, listing, and updating tasks within the system. - Enhanced the API to support task operations, including endpoints for task creation, retrieval, updating, and deletion. - Updated documentation to include new task tools and their usage. - Integrated task management into the agent's workflow, allowing for better task tracking and execution.
Remove all link-conversation code in preparation for task-based inter-agent delegation. Link channels will become audit logs of delegated tasks instead of LLM conversation threads. Removed (~830 lines): - conclude_link tool, prompt, and template - link_context prompt fragment and LinkContext type - channel.rs: link_concluded, link_turn_count, originating_channel, originating_source fields; coalesce bypass; source=internal handling; turn safety cap; conclusion routing; build_link_context method - main.rs: outbound reply relay for internal link channels - tools.rs: conclude_link registration, link_counterparty helper, delegation target complexity (add_channel_tools 12->8 params) - send_agent_message.rs: stripped to stub (validates link, ends turn) Kept: - Link topology infrastructure (AgentLink, config, store) - Org hierarchy prompt context (build_org_context, org_context.md.j2) - send_agent_message tool shell (for future task delegation) - Task system (store, API, tools, cortex ready-task loop)
…c-driven prompts Wire task state changes through SSE (TaskUpdated event in ProcessEvent, ApiEvent, and SSE serializer). Emit from all API handlers and cortex pickup/complete/requeue. Add kanban board UI (AgentTasks.tsx) with 5-column layout, task cards, create dialog, detail dialog, and real-time SSE updates via taskEventVersion in useLiveContext. Integrate active tasks into the cortex memory bulletin via gather_active_tasks(). Log cortex-spawned task workers to worker_runs with direct ProcessRunLogger calls so they appear in the Workers tab. Redesign /execute endpoint to move tasks to ready for cortex pickup instead of orphaning at in_progress. Fix claim_next_ready to respect priority ordering. Restrict worker scope in task_update to subtasks and metadata only. Rewrite all task prompts for spec-driven philosophy: task descriptions are full markdown specs with pre-filled subtask execution plans, refined iteratively through conversation. Update channel, branch, cortex_chat prompts and tool descriptions. Add tasks.mdx feature doc. Fix clippy warnings: auto-deref in cortex, clamp in store, collapsible ifs.
Contributor
WalkthroughAdds a task management system (DB, TaskStore, API endpoints, frontend Kanban, tools) and removes link-based inter-agent conversation logic, replacing delegation with task-based flows and wiring per-agent TaskStore through agent, tool-server, and cortex lifecycles and prompts. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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)
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 |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
src/tasks/store.rs
Outdated
| .flatten() | ||
| .and_then(|value| if value.is_empty() { None } else { Some(value) }), | ||
| created_by: row.try_get("created_by").unwrap_or_default(), | ||
| approved_at: row.try_get("approved_at").ok(), |
Contributor
There was a problem hiding this comment.
approved_at is the only timestamp not normalized to RFC3339 here (vs created_at/updated_at/completed_at). Keeping formats consistent avoids flaky JS date parsing.
Suggested change
| approved_at: row.try_get("approved_at").ok(), | |
| approved_at: row | |
| .try_get::<Option<chrono::NaiveDateTime>, _>("approved_at") | |
| .ok() | |
| .flatten() | |
| .map(|v| v.and_utc().to_rfc3339()), |
src/tasks/store.rs
Outdated
| return Ok(None); | ||
| }; | ||
|
|
||
| let task_number: i64 = row.try_get("task_number").unwrap_or(0); |
Contributor
There was a problem hiding this comment.
Minor: unwrap_or(0) can hide unexpected row shape issues and potentially run the claim update against task_number = 0. Prefer propagating the decode error.
Suggested change
| let task_number: i64 = row.try_get("task_number").unwrap_or(0); | |
| let task_number: i64 = row | |
| .try_get("task_number") | |
| .context("failed to read task_number")?; |
src/api/tasks.rs
Outdated
| let priority = query | ||
| .priority | ||
| .as_deref() | ||
| .and_then(crate::tasks::TaskPriority::parse); |
Contributor
There was a problem hiding this comment.
Right now invalid status / priority query params are silently treated as “no filter”. That can make debugging clients harder; I’d rather return 400 for invalid values.
Suggested change
| .and_then(crate::tasks::TaskPriority::parse); | |
| let status = match query.status.as_deref() { | |
| None => None, | |
| Some(value) => Some(crate::tasks::TaskStatus::parse(value).ok_or(StatusCode::BAD_REQUEST)?), | |
| }; | |
| let priority = match query.priority.as_deref() { | |
| None => None, | |
| Some(value) => Some(crate::tasks::TaskPriority::parse(value).ok_or(StatusCode::BAD_REQUEST)?), | |
| }; |
src/tools/task_update.rs
Outdated
| } | ||
|
|
||
| let status = args.status.as_deref().and_then(TaskStatus::parse); | ||
| let priority = args.priority.as_deref().and_then(TaskPriority::parse); |
Contributor
There was a problem hiding this comment.
Similar to the HTTP handlers: invalid status / priority strings currently become None and get treated like “no change”. If an LLM passes a bad value, it’ll look like the tool succeeded but did nothing.
Suggested change
| let priority = args.priority.as_deref().and_then(TaskPriority::parse); | |
| let status = match args.status.as_deref() { | |
| None => None, | |
| Some(value) => Some( | |
| TaskStatus::parse(value) | |
| .ok_or_else(|| TaskUpdateError(format!("invalid status: {value}")))?, | |
| ), | |
| }; | |
| let priority = match args.priority.as_deref() { | |
| None => None, | |
| Some(value) => Some( | |
| TaskPriority::parse(value) | |
| .ok_or_else(|| TaskUpdateError(format!("invalid priority: {value}")))?, | |
| ), | |
| }; |
src/agent/cortex.rs
Outdated
| &deps.runtime_config.instance_dir.display().to_string(), | ||
| &deps.runtime_config.workspace_dir.display().to_string(), | ||
| ) | ||
| .expect("failed to render worker prompt"); |
Contributor
There was a problem hiding this comment.
This is running in a long-lived background loop; a panic here kills task pickup entirely. Probably better to bubble up an error (and potentially re-queue the task) instead of expect.
Suggested change
| .expect("failed to render worker prompt"); | |
| .map_err(|error| anyhow::anyhow!("failed to render worker prompt: {error}"))?; |
src/agent/cortex.rs
Outdated
| .join(".spacebot") | ||
| .join("logs"); | ||
| let _ = std::fs::create_dir_all(&screenshot_dir); | ||
| let _ = std::fs::create_dir_all(&logs_dir); |
Contributor
There was a problem hiding this comment.
Swallowing create_dir_all errors makes later worker failures harder to diagnose (and you lose logs/screenshots silently). At least emit a warning with the path.
Suggested change
| let _ = std::fs::create_dir_all(&logs_dir); | |
| if let Err(error) = std::fs::create_dir_all(&screenshot_dir) { | |
| tracing::warn!(%error, path = %screenshot_dir.display(), "failed to create screenshot directory"); | |
| } | |
| if let Err(error) = std::fs::create_dir_all(&logs_dir) { | |
| tracing::warn!(%error, path = %logs_dir.display(), "failed to create logs directory"); | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
interface/src/routes/AgentConfig.tsx (1)
108-138:⚠️ Potential issue | 🟠 MajorAdd rollback for optimistic config updates on mutation failure.
The
onMutatecallback (line 108) writes optimistic updates to the cache, butonError(line 137) only clears thesavingflag without rolling back. If the request fails, the UI displays unsaved values until the next refetch, creating a data consistency issue.Suggested fix
const configMutation = useMutation({ mutationFn: (update: AgentConfigUpdateRequest) => api.updateAgentConfig(update), - onMutate: (update) => { + onMutate: async (update) => { + await queryClient.cancelQueries({ queryKey: ["agent-config", agentId] }); setSaving(true); // Optimistically merge the sent values into the cache so the UI // reflects the change immediately (covers fields the backend // doesn't yet return in its response, like sandbox). const previous = queryClient.getQueryData<AgentConfigResponse>(["agent-config", agentId]); if (previous) { const { agent_id: _, ...sections } = update; const merged = { ...previous } as unknown as Record<string, unknown>; const prev = previous as unknown as Record<string, unknown>; for (const [key, value] of Object.entries(sections)) { if (value !== undefined) { merged[key] = { ...(prev[key] as Record<string, unknown> | undefined), ...value, }; } } queryClient.setQueryData(["agent-config", agentId], merged as unknown as AgentConfigResponse); } + return { previous }; }, onSuccess: (result) => { // Merge server response with cache to preserve fields the backend // doesn't yet return (e.g. sandbox). const previous = queryClient.getQueryData<AgentConfigResponse>(["agent-config", agentId]); queryClient.setQueryData(["agent-config", agentId], { ...previous, ...result }); setDirty(false); setSaving(false); }, - onError: () => setSaving(false), + onError: (_error, _vars, ctx) => { + if (ctx?.previous) { + queryClient.setQueryData(["agent-config", agentId], ctx.previous); + } else { + queryClient.invalidateQueries({ queryKey: ["agent-config", agentId] }); + } + setSaving(false); + }, + onSettled: () => { + queryClient.invalidateQueries({ queryKey: ["agent-config", agentId] }); + }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentConfig.tsx` around lines 108 - 138, Capture and return the previous cache snapshot from onMutate so you can rollback on failure: inside onMutate (for the mutation that updates agent config) save the previous queryClient.getQueryData(["agent-config", agentId]) into a context object and return it; then update the cache optimistically as you already do. Update onError to accept the mutation context (error, variables, context) and, if context?.previous exists, call queryClient.setQueryData(["agent-config", agentId], context.previous) to restore the prior state and also call setSaving(false) there. Ensure the unique symbols referenced are onMutate, onError, queryClient.getQueryData, and queryClient.setQueryData.src/tools/send_agent_message.rs (1)
174-200:⚠️ Potential issue | 🔴 CriticalDo not report success for a no-op delegation path.
The tool ends the turn and returns
success: true, but nothing is delivered or queued. This can silently drop user work. Until task delegation is wired, return a structured error and avoid setting skip.As per coding guidelines "Implement tool errors as structured results returned to the LLM, not panics. The LLM sees the error and can recover".Proposed fix
- // End the current turn immediately after delegation. - if let Some(ref flag) = self.skip_flag { - flag.store(true, Ordering::Relaxed); - } + // Do not end the turn for a no-op path. @@ - Ok(SendAgentMessageOutput { - success: true, - target_agent: target_display, - message: "Message validated. Cross-agent task delegation not yet implemented." - .to_string(), - }) + Err(SendAgentMessageError( + "cross-agent task delegation is not implemented yet; no task/message was delivered" + .to_string(), + ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/send_agent_message.rs` around lines 174 - 200, The current no-op delegation path sets self.skip_flag.store(true, ...) and returns SendAgentMessageOutput { success: true, ... } even though nothing is delivered; remove the skip_flag mutation and instead return a structured error result to the caller so the LLM can recover (do not panic). Concretely, in the function handling delegation, do not call self.skip_flag.store(...) in the stub branch; return an Err containing a descriptive tool error (or Ok(SendAgentMessageOutput { success: false, target_agent: target_display, message: "...delegation not implemented..." })) using the project's standard tool-error type, or populate SendAgentMessageOutput.success = false with an explanatory message, referencing the SendAgentMessageOutput struct and the skip_flag field so the change is applied to the no-op delegation branch.
🟠 Major comments (20)
interface/src/hooks/useLiveContext.tsx-231-233 (1)
231-233:⚠️ Potential issue | 🟠 MajorTask data can remain stale after reconnect without a forced bump.
Line 231 only updates the version when a
task_updatedevent arrives. After SSE reconnect, if no new task event comes in, consumers keyed bytaskEventVersionmay keep stale task data.🔧 Suggested fix
const onReconnect = useCallback(() => { syncStatusSnapshot(); queryClient.invalidateQueries({ queryKey: ["channels"] }); queryClient.invalidateQueries({ queryKey: ["status"] }); queryClient.invalidateQueries({ queryKey: ["agents"] }); + bumpTaskVersion(); -}, [syncStatusSnapshot, queryClient]); +}, [syncStatusSnapshot, queryClient, bumpTaskVersion]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/hooks/useLiveContext.tsx` around lines 231 - 233, When the SSE connection is re-established the code currently only updates consumers when a real "task_updated" arrives, so add a forced version bump on reconnect: in useLiveContext.tsx locate the SSE/connect/reconnect handling (the code that registers channelHandlers / opens the EventSource or handles onopen/onreconnect) and invoke the existing bumpTaskVersion function there (or emit a synthetic "task_updated" through the same channelHandlers path) so consumers keyed by taskEventVersion are refreshed after reconnect even if no new task event arrives; reference bumpTaskVersion, task_updated, and channelHandlers when making the change.src/api/config.rs-203-207 (1)
203-207:⚠️ Potential issue | 🟠 MajorValidate sandbox mode before persisting config.
modeis currently free-form text and is written directly toconfig.toml. Invalid values can be saved and break reload/runtime consistency.Proposed fix
#[derive(Deserialize, Debug)] pub(super) struct SandboxUpdate { mode: Option<String>, writable_paths: Option<Vec<String>>, } @@ fn update_sandbox_table( doc: &mut toml_edit::DocumentMut, agent_idx: usize, sandbox: &SandboxUpdate, ) -> Result<(), StatusCode> { let agent = get_agent_table_mut(doc, agent_idx)?; let table = get_or_create_subtable(agent, "sandbox")?; if let Some(ref mode) = sandbox.mode { + match mode.as_str() { + "enabled" | "disabled" => {} + _ => return Err(StatusCode::BAD_REQUEST), + } table["mode"] = toml_edit::value(mode.as_str()); } if let Some(ref paths) = sandbox.writable_paths { let mut array = toml_edit::Array::new(); for path in paths {Also applies to: 678-680
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/config.rs` around lines 203 - 207, The SandboxUpdate struct allows any string for mode which can be written to config.toml and cause invalid runtime state; add strict validation by mapping mode to a concrete enum of allowed modes (e.g., SandboxMode) or by validating the incoming string against the known set of modes before persisting. Change the handling for SandboxUpdate::mode so you either (a) deserialize into Option<SandboxMode> (implement TryFrom/Deserialize for the enum) or (b) validate the Option<String> at the point where config is saved (the code paths that write the config referenced around SandboxUpdate and the other occurrence at lines ~678-680), returning an error or rejecting the update if the value is not one of the allowed variants; ensure only validated/normalized values are written to config.toml.migrations/20260219000001_tasks.sql-7-8 (1)
7-8:⚠️ Potential issue | 🟠 MajorAdd schema hardening for task status and priority in a new migration, not in-place edits.
The tasks table currently allows any text value for
statusandpriorityfields, and defaultsstatusto'backlog'rather than'pending_approval'to align with the approval-first lifecycle. These are valid concerns.However, the migration file
migrations/20260219000001_tasks.sqlhas already been committed and applied (there are newer migrations that depend on it). Per coding guidelines, this file cannot be edited in place. Instead, create a new migration file to add CHECK constraints and update the default:-- migrations/20260225000001_add_task_constraints.sql ALTER TABLE tasks ADD CONSTRAINT check_status CHECK (status IN ('pending_approval', 'backlog', 'ready', 'in_progress', 'done')), ADD CONSTRAINT check_priority CHECK (priority IN ('low', 'medium', 'high', 'urgent')); -- Update existing backlog tasks to pending_approval to align with approval-first lifecycle UPDATE tasks SET status = 'pending_approval' WHERE status = 'backlog'; -- Change the default for new tasks -- Note: Some databases require column recreation; verify SQLite syntaxVerify task creation code in
src/tools/task_create.rsandsrc/tasks/store.rsto ensure callers always setstatusexplicitly or accept the new'pending_approval'default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migrations/20260219000001_tasks.sql` around lines 7 - 8, Create a new migration (do not edit 20260219000001_tasks.sql) that adds CHECK constraints for tasks.status and tasks.priority, updates existing rows with status='backlog' to 'pending_approval', and changes the column default to 'pending_approval' (ensure SQL syntax is correct for our DB engine/SQLite quirks); reference the new migration name like migrations/20260225000001_add_task_constraints.sql and add constraints named e.g. check_status and check_priority. After adding the migration, audit and adjust code paths that create tasks so they either supply an explicit status or accept the new default: check src/tools/task_create.rs (task creation CLI/handler) and src/tasks/store.rs (store.create or insert functions) to ensure callers set status appropriately and tests cover creation with and without status provided.interface/src/routes/AgentTasks.tsx-75-77 (1)
75-77:⚠️ Potential issue | 🟠 MajorDetail dialog can show stale task state after mutations.
selectedTaskstores a snapshot object. After invalidation/refetch, the dialog can still render old status/actions.🧭 Suggested state model fix
- const [selectedTask, setSelectedTask] = useState<TaskItem | null>(null); + const [selectedTaskId, setSelectedTaskId] = useState<string | null>(null); + const selectedTask = tasks.find((task) => task.id === selectedTaskId) ?? null; @@ - onSelect={setSelectedTask} + onSelect={(task) => setSelectedTaskId(task.id)} @@ - onClose={() => setSelectedTask(null)} + onClose={() => setSelectedTaskId(null)}Also applies to: 191-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentTasks.tsx` around lines 75 - 77, selectedTask currently stores a snapshot object which can go stale after refetch; change the state to store only the selected task id (e.g. selectedTaskId via setSelectedTaskId) inside the AgentTasks component and update all setters that call setSelectedTask to set the id instead of the object; in the detail dialog component (where you previously used selectedTask) derive the live task by finding it in the current tasks list (e.g. tasks.find(t => t.id === selectedTaskId)) so the rendered status/actions reflect the latest data, and ensure you clear selectedTaskId when closing the dialog; update any usages of setSelectedTask and selectedTask to the new id-based API (including the other occurrences referenced around the component).src/agent/cortex.rs-1049-1060 (1)
1049-1060:⚠️ Potential issue | 🟠 MajorDon’t silently ignore filesystem setup errors.
Lines 1059-1060 discard
create_dir_allfailures. If these fail, downstream worker behavior becomes inconsistent and hard to diagnose.As per coding guidelines, "Don't silently discard errors; no let _ = on Results. Handle, log, or propagate errors. Only exception is .ok() on channel sends where the receiver may be dropped".🔧 Suggested fix
- let _ = std::fs::create_dir_all(&screenshot_dir); - let _ = std::fs::create_dir_all(&logs_dir); + tokio::fs::create_dir_all(&screenshot_dir).await?; + tokio::fs::create_dir_all(&logs_dir).await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/cortex.rs` around lines 1049 - 1060, The code currently discards errors from std::fs::create_dir_all for screenshot_dir and logs_dir; change this to handle failures by propagating or logging the error instead of using let _ =. Locate the screenshot_dir and logs_dir setup (references: screenshot_dir, logs_dir, deps.runtime_config.workspace_dir) and replace the ignored Results with proper error handling: call create_dir_all and on Err either return Err from the enclosing function (propagate a contextual io::Error) or log the failure with the actual error and then return/abort so downstream workers cannot proceed with inconsistent state. Ensure error messages include the target path (screenshot_dir/logs_dir) for easier debugging.interface/src/routes/AgentTasks.tsx-279-287 (1)
279-287:⚠️ Potential issue | 🟠 MajorTask cards are not keyboard accessible.
The card is clickable via mouse only. Keyboard users cannot open task details from the board.
♿ Suggested accessibility fix
<motion.div layout @@ className="cursor-pointer rounded-md border border-app-line/30 bg-app p-3 transition-colors hover:border-app-line" onClick={onSelect} + role="button" + tabIndex={0} + onKeyDown={(event) => { + if (event.key === "Enter" || event.key === " ") { + event.preventDefault(); + onSelect(); + } + }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentTasks.tsx` around lines 279 - 287, The task card (motion.div with props layout, initial, animate, exit and onClick={onSelect}) is not keyboard accessible; make it focusable and operable via keyboard by adding tabIndex={0}, role="button" and an onKeyDown handler that invokes onSelect when Enter or Space is pressed (prevent default for Space), and ensure an accessible name via existing text or aria-label if needed; update the element rendering in AgentTasks.tsx where the motion.div with className="cursor-pointer ..." is defined.src/agent/cortex.rs-1004-1011 (1)
1004-1011:⚠️ Potential issue | 🟠 MajorAdd a circuit breaker to the ready-task cortex loop.
This loop currently retries indefinitely on repeated failures with only warning logs.
As per coding guidelines, "Implement circuit breaker: auto-disable recurring tasks after 3 consecutive failures. Apply to cron jobs, maintenance workers, and cortex routines".🧯 Suggested circuit-breaker pattern
async fn run_ready_task_loop(deps: &AgentDeps, logger: &CortexLogger) -> anyhow::Result<()> { tracing::info!("cortex ready-task loop started"); + let mut consecutive_failures = 0_u8; @@ - if let Err(error) = pickup_one_ready_task(deps, logger).await { + if let Err(error) = pickup_one_ready_task(deps, logger).await { + consecutive_failures += 1; tracing::warn!(%error, "ready-task pickup pass failed"); + if consecutive_failures >= 3 { + tracing::error!("disabling ready-task loop after 3 consecutive failures"); + break; + } + } else { + consecutive_failures = 0; } } + + Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/cortex.rs` around lines 1004 - 1011, The ready-task loop must stop auto-retrying after 3 consecutive failures: add a consecutive_failures counter outside the loop, increment it when pickup_one_ready_task(deps, logger).await returns Err and reset it to 0 on success; when the counter reaches 3 log an error with the final error and disable further runs by updating the runtime config cortex enable flag (use the existing runtime_config/cortex API—e.g., call the cortex enabled setter or store a disabled state) or break out of the loop if no setter is available; ensure the tick interval logic (deps.runtime_config.cortex.load().tick_interval_secs) remains and include a clear log message when the circuit opens.src/agent/cortex.rs-1116-1181 (1)
1116-1181:⚠️ Potential issue | 🟠 MajorOnly emit
TaskUpdatedafter the task state update succeeds.The code emits
"done"/"ready"task events even when DB status updates fail, which can desync UI/SSE from persisted state.✅ Suggested event gating fix
- if let Err(error) = task_store - .update(...) - .await - { - tracing::warn!(...); - } - - let _ = event_tx.send(ProcessEvent::TaskUpdated { ... "done" ... }); + match task_store.update(...).await { + Ok(()) => { + event_tx + .send(ProcessEvent::TaskUpdated { ... "done" ... }) + .ok(); + } + Err(error) => { + tracing::warn!(...); + return; + } + } @@ - if let Err(update_error) = task_store - .update(...) - .await - { - tracing::warn!(...); - } - - let _ = event_tx.send(ProcessEvent::TaskUpdated { ... "ready" ... }); + match task_store.update(...).await { + Ok(()) => { + event_tx + .send(ProcessEvent::TaskUpdated { ... "ready" ... }) + .ok(); + } + Err(update_error) => { + tracing::warn!(...); + return; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/cortex.rs` around lines 1116 - 1181, The TaskUpdated SSE/event is emitted regardless of whether the DB update succeeded; modify the success and error branches so that the send(ProcessEvent::TaskUpdated { ... }) only occurs after task_store.update(...) returns Ok. Specifically, in the Ok(result) branch, move the ProcessEvent::TaskUpdated send to follow the awaited task_store.update(...) success (the update call that sets TaskStatus::Done), and in the Err(error) branch only send ProcessEvent::TaskUpdated after the awaited update that resets status to TaskStatus::Ready completes without Err; if the update returns Err, log the warning (tracing::warn!) but do not emit TaskUpdated for that task_number/agent_id. Ensure you reference the existing task_store.update, task.task_number, agent_id, and ProcessEvent::TaskUpdated symbols when making the change.src/api/tasks.rs-16-17 (1)
16-17:⚠️ Potential issue | 🟠 MajorValidate and cap
limitbefore querying storage.
limitcurrently accepts negative and very large values; in SQLite, negativeLIMITcan behave like unbounded reads. Enforce a safe range and reject invalid values.Proposed fix
fn default_task_limit() -> i64 { 20 } @@ - let tasks = store - .list(&query.agent_id, status, priority, query.limit) + if query.limit < 1 || query.limit > 200 { + return Err(StatusCode::BAD_REQUEST); + } + + let tasks = store + .list(&query.agent_id, status, priority, query.limit)Also applies to: 109-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/tasks.rs` around lines 16 - 17, Validate and clamp the request's limit field before any database query: reject non-positive limits with a BadRequest error and cap excessively large limits to a safe maximum (e.g., 1000). Locate uses of the limit field in this file (the struct field named limit and the two places referenced around lines ~16 and ~109) and add logic to (1) validate that limit > 0, returning a client error if not, and (2) clamp limit = limit.min(MAX_LIMIT) before building the SQL query or calling storage so SQLite never receives negative or unbounded values.src/tools/task_list.rs-33-35 (1)
33-35:⚠️ Potential issue | 🟠 MajorValidate
limitrange before querying tasks.Negative or very large limits are currently accepted and passed to storage; this can lead to surprising or expensive queries.
Proposed fix
async fn call(&self, args: Self::Args) -> Result<Self::Output, Self::Error> { + if args.limit < 1 || args.limit > 200 { + return Err(TaskListError("limit must be between 1 and 200".to_string())); + } + let status = args.status.as_deref().and_then(TaskStatus::parse); let priority = args.priority.as_deref().and_then(TaskPriority::parse); let tasks = self .task_storeAlso applies to: 86-86
src/api/tasks.rs-100-107 (1)
100-107:⚠️ Potential issue | 🟠 MajorInvalid
status/priorityvalues are silently accepted.Unknown enum strings currently become
None(or default), which broadens list results or ignores client intent on create/update. These should return400 BAD_REQUEST.Proposed fix pattern
+fn parse_status_opt(raw: Option<&str>) -> Result<Option<crate::tasks::TaskStatus>, StatusCode> { + match raw { + Some(value) => crate::tasks::TaskStatus::parse(value) + .map(Some) + .ok_or(StatusCode::BAD_REQUEST), + None => Ok(None), + } +} + +fn parse_priority_opt(raw: Option<&str>) -> Result<Option<crate::tasks::TaskPriority>, StatusCode> { + match raw { + Some(value) => crate::tasks::TaskPriority::parse(value) + .map(Some) + .ok_or(StatusCode::BAD_REQUEST), + None => Ok(None), + } +}Then use these helpers in list/create/update instead of silent fallback.
Also applies to: 147-156, 197-204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/tasks.rs` around lines 100 - 107, The code currently calls crate::tasks::TaskStatus::parse and TaskPriority::parse and silently treats unknown strings as None; change this to validate parse results and return a 400 BAD_REQUEST when parsing fails: introduce small helper(s) (e.g., parse_status_or_bad_request / parse_priority_or_bad_request) that take the incoming &str/Option<&str> from query or request body and return Result<StatusEnum, HttpResponse/ApiError>, then use those helpers in the list, create, and update handlers (replace the current uses around query.status/query.priority and the other occurrences at the referenced ranges) so that invalid enum strings produce an immediate 400 response instead of falling back to None/default.src/tools/task_list.rs-82-83 (1)
82-83:⚠️ Potential issue | 🟠 MajorInvalid filter enums should produce a tool error.
Unknown
status/prioritycurrently becomeNone, which broadens results unexpectedly while still reporting success.Proposed fix
- let status = args.status.as_deref().and_then(TaskStatus::parse); - let priority = args.priority.as_deref().and_then(TaskPriority::parse); + let status = match args.status.as_deref() { + Some(value) => Some( + TaskStatus::parse(value) + .ok_or_else(|| TaskListError(format!("invalid status '{}'", value)))?, + ), + None => None, + }; + let priority = match args.priority.as_deref() { + Some(value) => Some( + TaskPriority::parse(value) + .ok_or_else(|| TaskListError(format!("invalid priority '{}'", value)))?, + ), + None => None, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/task_list.rs` around lines 82 - 83, The current code silently treats unknown filters as None by using args.status.as_deref().and_then(TaskStatus::parse) and args.priority.as_deref().and_then(TaskPriority::parse); change this to validate the input and return a tool error when parsing fails instead of falling back to None. Replace the optional chaining with explicit parsing logic that checks args.status and args.priority (use TaskStatus::parse and TaskPriority::parse) and, on parse failure, returns an Err (tool error) with a clear message about the invalid status/priority filter so the command exits with an error instead of returning broadened results.src/tools/task_create.rs-103-108 (1)
103-108:⚠️ Potential issue | 🟠 MajorReject invalid
priority/statusinstead of silently defaulting.Unknown enum values currently fall back to
medium/backlog, which can create the wrong task while reporting success. Return a tool error for invalid values so callers can recover explicitly.Proposed fix
- let priority = TaskPriority::parse(&args.priority).unwrap_or(TaskPriority::Medium); - let status = args - .status - .as_deref() - .and_then(TaskStatus::parse) - .unwrap_or(TaskStatus::Backlog); + let priority = TaskPriority::parse(&args.priority) + .ok_or_else(|| TaskCreateError(format!("invalid priority '{}'", args.priority)))?; + + let status = match args.status.as_deref() { + Some(raw) => TaskStatus::parse(raw) + .ok_or_else(|| TaskCreateError(format!("invalid status '{}'", raw)))?, + None => TaskStatus::Backlog, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/task_create.rs` around lines 103 - 108, Replace the silent defaults for invalid enums by returning a tool error: instead of using unwrap_or on TaskPriority::parse(&args.priority) and on args.status.as_deref().and_then(TaskStatus::parse), check for None and return an Err with a clear message (including the invalid input string) so callers can handle it; e.g. use TaskPriority::parse(&args.priority).ok_or_else(|| /* Tool error: invalid priority with args.priority */) and similarly for args.status -> args.status.as_deref().and_then(TaskStatus::parse).ok_or_else(|| /* Tool error: invalid status with args.status */), making sure the function signature returns Result so the error is propagated.src/tools/task_update.rs-184-189 (1)
184-189:⚠️ Potential issue | 🟠 MajorDo not silently ignore invalid
status/priorityor negativecomplete_subtask.Current conversion paths collapse invalid inputs to
None, then return success. That makes failed updates look successful.Proposed fix
- let status = args.status.as_deref().and_then(TaskStatus::parse); - let priority = args.priority.as_deref().and_then(TaskPriority::parse); - let complete_subtask = args - .complete_subtask - .and_then(|value| usize::try_from(value).ok()); + let status = match args.status.as_deref() { + Some(value) => Some( + TaskStatus::parse(value) + .ok_or_else(|| TaskUpdateError(format!("invalid status '{}'", value)))?, + ), + None => None, + }; + let priority = match args.priority.as_deref() { + Some(value) => Some( + TaskPriority::parse(value) + .ok_or_else(|| TaskUpdateError(format!("invalid priority '{}'", value)))?, + ), + None => None, + }; + let complete_subtask = match args.complete_subtask { + Some(value) if value >= 0 => Some(value as usize), + Some(value) => { + return Err(TaskUpdateError(format!( + "complete_subtask must be >= 0, got {}", + value + ))) + } + None => None, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/task_update.rs` around lines 184 - 189, The current conversions for status, priority and complete_subtask silently turn invalid inputs into None; instead validate and return an error when parsing fails: replace the and_then chains that set status = args.status.as_deref().and_then(TaskStatus::parse) and priority = args.priority.as_deref().and_then(TaskPriority::parse) with explicit parsing that maps parse failures into an Err (with a clear message including the invalid string) and propagate it from the update function, and change complete_subtask from args.complete_subtask.and_then(|value| usize::try_from(value).ok()) to explicit validation that returns an Err when the value is negative or cannot convert to usize (e.g., by using try_from and mapping the failure to an error), so invalid inputs produce a failing Result instead of silently becoming None.src/tools/send_agent_message.rs-69-75 (1)
69-75:⚠️ Potential issue | 🟠 MajorDetect ambiguous display-name matches during target resolution.
Case-insensitive name lookup returns the first
HashMapmatch. If multiple agents share a display name, routing becomes non-deterministic and can target the wrong agent.Proposed fix
- fn resolve_agent_id(&self, target: &str) -> Option<String> { + fn resolve_agent_id(&self, target: &str) -> Result<String, SendAgentMessageError> { // Direct ID match if self.agent_names.contains_key(target) { - return Some(target.to_string()); + return Ok(target.to_string()); } - // Name match (case-insensitive) - let target_lower = target.to_lowercase(); - for (agent_id, name) in self.agent_names.iter() { - if name.to_lowercase() == target_lower { - return Some(agent_id.clone()); - } - } - - None + let matches = self + .agent_names + .iter() + .filter(|(_, name)| name.eq_ignore_ascii_case(target)) + .map(|(agent_id, _)| agent_id.clone()) + .collect::<Vec<_>>(); + + match matches.as_slice() { + [only] => Ok(only.clone()), + [] => Err(SendAgentMessageError(format!("unknown agent '{}'", target))), + _ => Err(SendAgentMessageError(format!( + "agent name '{}' is ambiguous; use explicit agent ID", + target + ))), + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/send_agent_message.rs` around lines 69 - 75, The current case-insensitive lookup using target_lower over self.agent_names stops at the first match and returns agent_id.clone(), which masks ambiguous display names; change the logic in the display-name resolution (the loop over self.agent_names comparing name.to_lowercase() == target_lower) to collect all matching agent_id values instead of returning immediately, then if the match list is empty return None, if it has exactly one return that agent_id, and if it has more than one treat it as an ambiguous target (return an error/None or a distinct AmbiguousTarget result and surface a clear message) so callers can detect and handle duplicate display names. Ensure you update the caller behavior to handle the new ambiguous result if needed.src/tasks/store.rs-171-177 (1)
171-177:⚠️ Potential issue | 🟠 Major
MAX(task_number)+1allocation can race under concurrent creates.The number allocation query and insert are separate steps, so parallel creates for the same agent can collide on
UNIQUE(agent_id, task_number)and fail intermittently.Also applies to: 184-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tasks/store.rs` around lines 171 - 177, The current allocation that runs SELECT COALESCE(MAX(task_number),0)+1 then inserts can race under concurrent creates and violate UNIQUE(agent_id, task_number); change it so allocation and insert are done atomically within the same transaction by taking a row-level lock first (e.g., lock the agent row or relevant tasks rows via FOR UPDATE in the same tx) before computing MAX(task_number) and inserting, or alternatively switch to a per-agent sequence/monotonic counter and use that in the INSERT; also add a retry on UNIQUE constraint violations as a fallback. Make these changes around the task number allocation code (the query that computes task_number, the tx variable/transaction block, and the subsequent insert into tasks) so concurrent callers cannot compute the same task_number.src/tasks/store.rs-401-401 (1)
401-401:⚠️ Potential issue | 🟠 MajorDecode failures are being masked with defaults.
Line 401 falls back to
0fortask_number, and row/JSON parsing helpers default to empty values on decode failures. That can hide corruption and surface invalid tasks instead of failing fast.🧯 Proposed hardening
-fn parse_subtasks(value: &str) -> Vec<TaskSubtask> { - serde_json::from_str(value).unwrap_or_default() +fn parse_subtasks(value: &str) -> Result<Vec<TaskSubtask>> { + serde_json::from_str(value).context("invalid subtasks JSON").map_err(Into::into) } -fn parse_metadata(value: &str) -> Value { - serde_json::from_str(value).unwrap_or_else(|_| Value::Object(serde_json::Map::new())) +fn parse_metadata(value: &str) -> Result<Value> { + serde_json::from_str(value).context("invalid metadata JSON").map_err(Into::into) }- let task_number: i64 = row.try_get("task_number").unwrap_or(0); + let task_number: i64 = row + .try_get("task_number") + .context("invalid tasks.task_number")?; ... - subtasks: parse_subtasks(&subtasks_value), - metadata: parse_metadata(&metadata_value), + subtasks: parse_subtasks(&subtasks_value)?, + metadata: parse_metadata(&metadata_value)?,As per coding guidelines: Don't silently discard errors; no let _ = on Results. Handle, log, or propagate errors.
Also applies to: 597-603, 605-652
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tasks/store.rs` at line 401, Replace silent defaulting on decode errors: do not use unwrap_or(0) on row.try_get("task_number") or the parsing helpers that return empty values; instead return or propagate the error (use ? or map_err with a contextual message) from the surrounding function so failures surface, or explicitly log and return an Err with context. Update the occurrence at row.try_get("task_number") and the similar decode sites referenced around lines 597-603 and 605-652 to stop swallowing errors, ensuring the JSON/row parsing helpers return Results and their callers handle non-Ok cases (propagate, map_err with context, or log+return Err) rather than substituting zero/empty defaults. Ensure any function signatures that need Result propagation are adjusted accordingly.src/tools.rs-307-310 (1)
307-310: 🛠️ Refactor suggestion | 🟠 MajorDo not silently ignore optional tool removal failures.
Line 308–310 drops
Results entirely, so failures become invisible during teardown and debugging.♻️ Proposed fix
- let _ = handle.remove_tool(CronTool::NAME).await; - let _ = handle.remove_tool(SendMessageTool::NAME).await; - let _ = handle.remove_tool(SendAgentMessageTool::NAME).await; + if let Err(error) = handle.remove_tool(CronTool::NAME).await { + tracing::debug!(%error, "failed to remove optional cron tool"); + } + if let Err(error) = handle.remove_tool(SendMessageTool::NAME).await { + tracing::debug!(%error, "failed to remove optional send_message tool"); + } + if let Err(error) = handle.remove_tool(SendAgentMessageTool::NAME).await { + tracing::debug!(%error, "failed to remove optional send_agent_message tool"); + }As per coding guidelines: Don't silently discard errors; no let _ = on Results. Handle, log, or propagate errors. Only exception is .ok() on channel sends where the receiver may be dropped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools.rs` around lines 307 - 310, The three calls to handle.remove_tool(CronTool::NAME), handle.remove_tool(SendMessageTool::NAME), and handle.remove_tool(SendAgentMessageTool::NAME) currently drop their Results with `let _ =`, hiding any teardown errors; change them to handle the Result instead—either propagate the error (use `?` if in a fallible async fn) or log failures (e.g., with processLogger/tracing using error/warn) so removal failures are visible during teardown; locate the calls to remove_tool and replace the `let _ =` patterns with explicit error handling for each tool name.src/agent/channel.rs-211-224 (1)
211-224:⚠️ Potential issue | 🟠 Major
send_agent_messageavailability is frozen at channel creation.This checks links once when the channel is created. If links are added later, existing channels never expose the tool. Consider evaluating link presence per turn (right before
add_channel_tools) or keeping the tool available and letting it consult current links at execution time.🛠️ Suggested direction
- let send_agent_message_tool = { - let has_links = - !crate::links::links_for_agent(&deps.links.load(), &deps.agent_id).is_empty(); - if has_links { - Some(crate::tools::SendAgentMessageTool::new( - deps.agent_id.clone(), - deps.links.clone(), - deps.agent_names.clone(), - )) - } else { - None - } - }; + let send_agent_message_tool = Some(crate::tools::SendAgentMessageTool::new( + deps.agent_id.clone(), + deps.links.clone(), + deps.agent_names.clone(), + ));Then gate tool registration at turn time using the latest
deps.links.load().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 211 - 224, The current code sets send_agent_message_tool once at channel creation using crate::links::links_for_agent(&deps.links.load(), &deps.agent_id), so channels never gain the tool when links are added later; move the link-presence check and tool registration into the per-turn path right before add_channel_tools (or alternatively always construct a SendAgentMessageTool that consults deps.links.load() at execution time) so that crate::tools::SendAgentMessageTool::new isn't frozen at creation and add_channel_tools sees the latest links each turn.src/tasks/store.rs-436-447 (1)
436-447:⚠️ Potential issue | 🟠 MajorTransition matrix bypasses the declared lifecycle.
Current rules allow direct
pending_approval -> readyand effectively* -> backlog. That contradicts the stated sequence (pending_approval -> backlog -> ready -> in_progress -> done) and can skip intended gates.🔒 Proposed stricter transition table
-fn can_transition(current: TaskStatus, next: TaskStatus) -> bool { - if current == next { - return true; - } - - if next == TaskStatus::Backlog { - return true; - } - - matches!( - (current, next), - (TaskStatus::PendingApproval, TaskStatus::Ready) - | (TaskStatus::Ready, TaskStatus::InProgress) - | (TaskStatus::InProgress, TaskStatus::Done) - | (TaskStatus::InProgress, TaskStatus::Ready) - | (TaskStatus::Backlog, TaskStatus::Ready) - ) -} +fn can_transition(current: TaskStatus, next: TaskStatus) -> bool { + if current == next { + return true; + } + + matches!( + (current, next), + (TaskStatus::PendingApproval, TaskStatus::Backlog) + | (TaskStatus::Backlog, TaskStatus::Ready) + | (TaskStatus::Ready, TaskStatus::InProgress) + | (TaskStatus::InProgress, TaskStatus::Done) + | (TaskStatus::InProgress, TaskStatus::Ready) + ) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tasks/store.rs` around lines 436 - 447, The transition check currently lets any state go to Backlog and allows PendingApproval -> Ready, bypassing the intended lifecycle; update the logic in the function that evaluates TaskStatus transitions (the match using TaskStatus::PendingApproval, TaskStatus::Backlog, etc.) to remove the special-case `if next == TaskStatus::Backlog { return true; }`, remove the (TaskStatus::PendingApproval, TaskStatus::Ready) arm, and add (TaskStatus::PendingApproval, TaskStatus::Backlog) instead; keep the valid transitions as (TaskStatus::PendingApproval, TaskStatus::Backlog) | (TaskStatus::Backlog, TaskStatus::Ready) | (TaskStatus::Ready, TaskStatus::InProgress) | (TaskStatus::InProgress, TaskStatus::Done) and optionally (TaskStatus::InProgress, TaskStatus::Ready) if rollbacks are allowed.
🟡 Minor comments (6)
docs/content/docs/(features)/tasks.mdx-148-148 (1)
148-148:⚠️ Potential issue | 🟡 MinorUse “who” instead of “that” for workers.
Line 148 reads more naturally as “a worker who completes without errors…”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/`(features)/tasks.mdx at line 148, Change the wording in the sentence "a worker that completes without errors is considered successful" to use "who" for people-focused language; update the sentence in docs/content/docs/(features)/tasks.mdx (the sentence containing "worker that completes without errors") to read "a worker who completes without errors is considered successful."src/api/agents.rs-800-803 (1)
800-803:⚠️ Potential issue | 🟡 MinorAdd symmetric cleanup for
task_storeson agent deletion.Line 800 adds the per-agent task store into API state, but
delete_agentdoes not remove it. This leaves stale entries after agent removal.🧹 Suggested delete-path cleanup
+ let mut task_stores = (**state.task_stores.load()).clone(); + task_stores.remove(&agent_id); + state.task_stores.store(std::sync::Arc::new(task_stores));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 800 - 803, delete_agent currently omits removing the per-agent entry from state.task_stores, leaving stale task stores; update the delete_agent implementation to symmetrically remove the agent_id key from the task_stores map by loading the Arc map from state.task_stores, cloning into a mutable map, calling .remove(&agent_id) (or equivalent) and then storing back an Arc of the updated map via state.task_stores.store(...); reference the task_stores variable usage and state.task_stores.load()/store() in the existing add-path to mirror its behavior.docs/content/docs/(core)/architecture.mdx-300-313 (1)
300-313:⚠️ Potential issue | 🟡 MinorAPI endpoint prefixes in the table appear outdated/inaccurate.
The table lists top-level prefixes like
/api/tasksand/api/cron, but current task/cron routes are agent-scoped (/api/agents/...). Aligning this table with the current API surface will prevent integration confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/`(core)/architecture.mdx around lines 300 - 313, The table rows for "Tasks" and "Cron" are outdated: update their Prefix cells to the current agent-scoped routes (e.g. change `/api/tasks` to `/api/agents/{agentId}/tasks` and `/api/cron` to `/api/agents/{agentId}/cron`). While updating, verify and, if needed, correct any other rows that reference top-level routes (e.g. "Channels", "Workers") to their current prefixes to match the live API surface so the table entries like "Tasks" and "Cron" accurately reflect agent-scoped endpoints.docs/content/docs/(core)/architecture.mdx-25-56 (1)
25-56:⚠️ Potential issue | 🟡 MinorProcess-model intro contradicts the compactor definition.
The intro says all five process types are Rig agents, while the compactor section states it is not an LLM process. Please make this consistent (e.g., four Rig-agent processes plus one programmatic compactor).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/`(core)/architecture.mdx around lines 25 - 56, Update the opening sentence to state that four of the process types are Rig agents (Agent<SpacebotModel, SpacebotHook>) and that the Compactor is a programmatic monitor (not an LLM/Agent), so the doc is consistent; specifically, change the line that currently claims "Five process types, each implemented as a Rig `Agent<SpacebotModel, SpacebotHook>`" to something like "Five process types — four are Rig agents (Channel, Branch, Worker, ...), and the Compactor is a programmatic monitor (not an LLM process)" and ensure the subsequent descriptions for Channel, Branch, Worker, and Compactor reflect that distinction.docs/design-docs/link-channels-task-delegation.md-21-21 (1)
21-21:⚠️ Potential issue | 🟡 MinorAdd language identifiers to fenced code blocks (MD040).
The unlabeled fences trigger markdownlint warnings. Please annotate them with explicit languages (
text,rust, etc.) for lint compliance and clearer rendering.Also applies to: 58-58, 99-99, 109-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design-docs/link-channels-task-delegation.md` at line 21, The markdown file contains unlabeled fenced code blocks that trigger MD040; add explicit language identifiers to each fenced block mentioned (the blocks at the current location and the other occurrences noted) — e.g., convert ``` to ```text or ```rust as appropriate for the snippet content — so each fenced code block (the unlabeled fences referenced and the ones at the other mentioned spots) is annotated with the proper language identifier for lint compliance and clearer rendering.src/tasks/store.rs-293-297 (1)
293-297:⚠️ Potential issue | 🟡 MinorOut-of-range
complete_subtaskindexes are silently ignored.If the index is invalid, the update returns success with no change, which can hide caller bugs.
✅ Proposed fix
- if let Some(index) = input.complete_subtask - && let Some(subtask) = subtasks.get_mut(index) - { - subtask.completed = true; - } + if let Some(index) = input.complete_subtask { + let Some(subtask) = subtasks.get_mut(index) else { + return Err(crate::error::Error::Other(anyhow::anyhow!( + "subtask index out of range: {index}" + ))); + }; + subtask.completed = true; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tasks/store.rs` around lines 293 - 297, The code silently ignores invalid input.complete_subtask indexes because subtasks.get_mut(index) returning None is treated the same as success; change the logic so when input.complete_subtask is Some(index) and subtasks.get_mut(index) returns None you return an error (or propagate a Result) instead of doing nothing. Locate the block using input.complete_subtask and subtasks.get_mut and replace the silent-ignore with an explicit failure (e.g., return Err(StoreError::InvalidSubtaskIndex(index)) or map to a Result::Err) so callers receive a clear error when the index is out of range; update any function signature and callers to propagate/handle that Result accordingly.
🧹 Nitpick comments (3)
interface/src/routes/AgentConfig.tsx (1)
67-68: Derive valid tabs fromSECTIONSinstead of duplicating literals.Line 67 duplicates section ids already defined in
SECTIONS; this can drift when new tabs are added.♻️ Suggested refactor
-const validSections: SectionId[] = ["soul", "identity", "user", "routing", "tuning", "compaction", "cortex", "coalesce", "memory", "browser", "sandbox"]; -if (validSections.includes(search.tab as SectionId)) { +const validSections = new Set(SECTIONS.map((s) => s.id)); +if (validSections.has(search.tab as SectionId)) { setActiveSection(search.tab as SectionId); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentConfig.tsx` around lines 67 - 68, Replace the hardcoded validSections array with a derived list from the existing SECTIONS constant: compute validSections from SECTIONS (e.g. Object.keys(SECTIONS) as SectionId[] or SECTIONS.map(s => s.id) depending on SECTIONS shape) and then use that to check if search.tab is valid (keep the includes check against search.tab). Update references to validSections and retain the SectionId typing to avoid drifting when tabs are added.prompts/en/tools/task_update_description.md.j2 (1)
1-1: Define “metadata” explicitly to reduce invalid update attempts.Please enumerate the allowed metadata fields (for example:
status,priority) so worker-scoped updates are unambiguous.Proposed wording refinement
-Update an existing task by task number. Use this to refine the spec as scope evolves — append sections, rewrite requirements, adjust subtasks, change priority. The description is a living document; update it when the user clarifies intent or when you discover new context. Move to `ready` when the spec is complete and the cortex will pick it up for execution. For worker processes, only subtask and metadata updates are allowed. +Update an existing task by task number. Use this to refine the spec as scope evolves — append sections, rewrite requirements, adjust subtasks, change priority. The description is a living document; update it when the user clarifies intent or when you discover new context. Move to `ready` when the spec is complete and the cortex will pick it up for execution. For worker processes, only subtask and metadata updates are allowed (metadata = `status` and `priority` only).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prompts/en/tools/task_update_description.md.j2` at line 1, Update the task update template (task_update_description.md.j2) to explicitly define "metadata" and list allowed metadata fields for worker-scoped updates; replace the vague phrase "metadata" with a short enumerated list such as "Allowed metadata fields: status, priority, assignee, due_date, labels" and add a clarifying sentence that workers may only update those listed fields (and subtasks), and move the spec-to-ready guidance unchanged.src/prompts/engine.rs (1)
441-454: Renamerender_channel_prompt_with_linksto match current behavior.This helper no longer handles link context, so the name is now misleading.
♻️ Suggested rename
- pub fn render_channel_prompt_with_links( + pub fn render_channel_prompt_with_org_context(Also update the
render_channel_prompt(...)call site accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/engine.rs` around lines 441 - 454, The function render_channel_prompt_with_links no longer handles link context and should be renamed to render_channel_prompt to reflect current behavior; rename the method declaration (render_channel_prompt_with_links -> render_channel_prompt) and update all call sites that reference render_channel_prompt_with_links (including the internal call to render_channel_prompt(...) noted in the diff) to use the new name, ensuring any associated documentation/comments and the pub fn signature, plus any references in tests or modules, are updated to the new identifier.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/content/docs/(core)/meta.jsonis excluded by!**/*.jsondocs/content/docs/(features)/meta.jsonis excluded by!**/*.json
📒 Files selected for processing (50)
TODOdocs/.node-versiondocs/content/docs/(configuration)/config.mdxdocs/content/docs/(core)/architecture.mdxdocs/content/docs/(core)/cortex.mdxdocs/content/docs/(features)/tasks.mdxdocs/content/docs/index.mdxdocs/design-docs/link-channels-task-delegation.mdinterface/src/api/client.tsinterface/src/components/AgentTabs.tsxinterface/src/hooks/useLiveContext.tsxinterface/src/router.tsxinterface/src/routes/AgentConfig.tsxinterface/src/routes/AgentTasks.tsxmigrations/20260219000001_tasks.sqlprompts/en/branch.md.j2prompts/en/channel.md.j2prompts/en/cortex_chat.md.j2prompts/en/fragments/link_context.md.j2prompts/en/fragments/org_context.md.j2prompts/en/tools/branch_description.md.j2prompts/en/tools/conclude_link_description.md.j2prompts/en/tools/task_create_description.md.j2prompts/en/tools/task_list_description.md.j2prompts/en/tools/task_update_description.md.j2src/agent/channel.rssrc/agent/cortex.rssrc/agent/ingestion.rssrc/agent/worker.rssrc/api.rssrc/api/agents.rssrc/api/config.rssrc/api/server.rssrc/api/state.rssrc/api/system.rssrc/api/tasks.rssrc/lib.rssrc/main.rssrc/prompts/engine.rssrc/prompts/text.rssrc/tasks.rssrc/tasks/store.rssrc/tools.rssrc/tools/conclude_link.rssrc/tools/send_agent_message.rssrc/tools/task_create.rssrc/tools/task_list.rssrc/tools/task_update.rstests/bulletin.rstests/context_dump.rs
💤 Files with no reviewable changes (3)
- prompts/en/fragments/link_context.md.j2
- prompts/en/tools/conclude_link_description.md.j2
- src/tools/conclude_link.rs
…idate enums, fix task number race - Replace expect() with error propagation in cortex ready-task loop - Return 400/tool errors for invalid status/priority values in API and LLM tools - Retry task creation on UNIQUE constraint collision (concurrent creates) - Emit SSE task events only after DB write confirms - Normalize approved_at to RFC3339, propagate decode errors in task_from_row - Log create_dir_all failures instead of silently swallowing - Derive selectedTask from live query data to prevent stale detail dialog - Invalidate task queries and bump version on SSE reconnect
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/tools/task_update.rs (1)
48-48: Aligntask_numbertype with store/API (i64).Using
i32here introduces an avoidable range cap and conversion step while every downstream path isi64.Proposed fix
pub struct TaskUpdateArgs { - pub task_number: i32, + pub task_number: i64, @@ - let task_number = i64::from(args.task_number); + let task_number = args.task_number;Also applies to: 147-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/task_update.rs` at line 48, The struct field pub task_number: i32 is using the wrong integer width; change its type to i64 to match the store/API and remove unnecessary conversions—update both occurrences of task_number (the declaration at the shown pub task_number entry and the other occurrence referenced) to pub task_number: i64 and fix any affected usages/signatures that rely on the old i32 type (e.g., constructors, deserializers, and callers) so all paths consistently use i64.
🤖 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/agent/cortex.rs`:
- Around line 1015-1035: The code currently claims a task via
task_store.claim_next_ready(&deps.agent_id) and then proceeds with setup steps
like prompt_engine.render_worker_prompt which may return early with ?, leaving
the task stuck as in_progress; modify the flow so that after a successful claim
you perform setup in a fallible block and on any error explicitly release or
requeue the claimed task (call the task store's release/requeue method such as
task_store.requeue or a similarly named method with task.task_number/agent_id)
before returning the error; apply the same pattern for the later setup area
(around render_worker_prompt and the section noted at 1080-1089) to ensure any
failure restores task state.
- Around line 1004-1011: The ready-task loop should trip a simple circuit
breaker after 3 consecutive errors: around the existing loop calling
pickup_one_ready_task(deps, logger).await, add a persistent failure counter
(AtomicUsize) and a breaker flag (AtomicBool) on the runtime state/config (e.g.,
add ready_task_enabled: AtomicBool and ready_task_failures: AtomicUsize to the
cortex/runtime config struct). Before sleeping, check ready_task_enabled and
skip/exit the loop if false; when pickup_one_ready_task returns Err, increment
ready_task_failures and if it reaches 3 set ready_task_enabled to false and emit
an error-level log indicating the breaker tripped; on Ok, reset
ready_task_failures to 0. Ensure you reference pickup_one_ready_task and
deps.runtime_config.cortex (or the new ready_task_enabled/ready_task_failures
fields) so callers can find and toggle the breaker state.
- Around line 1091-1097: Replace the silent Result discard pattern used when
sending events on the channel with explicit .ok() handling: locate occurrences
of deps.event_tx.send(ProcessEvent::TaskUpdated { ... }) (and the other similar
sends around the same block) and change them to call .ok() on the returned
Result so the send is explicitly ignored only in the allowed channel-drop case;
update every occurrence (e.g., the TaskUpdated sends and other ProcessEvent
variants at the mentioned locations) to use deps.event_tx.send(...).ok() rather
than using let _ =.
In `@src/api/tasks.rs`:
- Around line 226-229: The map_err closures wrapping store.update(...) currently
convert all errors to INTERNAL_SERVER_ERROR; change them to inspect the returned
error (e.g., match on the error's enum/variant or use an
is_validation/is_domain_error helper) and map domain/validation failures
(illegal status transition, invalid input) to StatusCode::BAD_REQUEST while
still mapping unexpected/storage errors to StatusCode::INTERNAL_SERVER_ERROR;
update the three occurrences around the current diff and the other spots you
noted (the closures at ~lines 226-229, 300-303, and 358-361) and keep the
existing tracing::warn! calls but include the chosen StatusCode mapping logic.
In `@src/tasks/store.rs`:
- Around line 633-639: parse_subtasks and parse_metadata currently swallow JSON
decode failures by returning empty defaults; change them to propagate errors
instead of hiding them: update parse_subtasks to return Result<Vec<TaskSubtask>,
serde_json::Error> and parse_metadata to return Result<Value,
serde_json::Error>, replace unwrap_or_default / unwrap_or_else with
serde_json::from_str(...)?, and propagate the Result up to callers (or log and
convert to an Err at the callsite) so callers can handle or log corrupt data;
apply the same change for the other similar parsing helpers referenced around
the 648-650 area.
- Around line 327-331: The code silently no-ops when input.complete_subtask
contains an out-of-range index; change the logic so that when
input.complete_subtask is Some(index) you attempt to get_mut on subtasks and if
get_mut returns None you return a proper error (e.g. Err(...) or an appropriate
StoreError) instead of continuing, otherwise set subtask.completed = true; use
input.complete_subtask and subtasks.get_mut(index) to locate the place to return
the error so callers receive a failure for invalid indices.
---
Nitpick comments:
In `@src/tools/task_update.rs`:
- Line 48: The struct field pub task_number: i32 is using the wrong integer
width; change its type to i64 to match the store/API and remove unnecessary
conversions—update both occurrences of task_number (the declaration at the shown
pub task_number entry and the other occurrence referenced) to pub task_number:
i64 and fix any affected usages/signatures that rely on the old i32 type (e.g.,
constructors, deserializers, and callers) so all paths consistently use i64.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
interface/src/hooks/useLiveContext.tsxinterface/src/routes/AgentTasks.tsxsrc/agent/cortex.rssrc/api/tasks.rssrc/tasks/store.rssrc/tools/task_create.rssrc/tools/task_list.rssrc/tools/task_update.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/tools/task_list.rs
- src/tools/task_create.rs
- interface/src/routes/AgentTasks.tsx
Comment on lines
+1004
to
+1011
| loop { | ||
| let interval = deps.runtime_config.cortex.load().tick_interval_secs; | ||
| tokio::time::sleep(Duration::from_secs(interval.max(5))).await; | ||
|
|
||
| if let Err(error) = pickup_one_ready_task(deps, logger).await { | ||
| tracing::warn!(%error, "ready-task pickup pass failed"); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add a failure circuit breaker to the ready-task recurring loop.
The loop currently warns forever on repeated failures. This routine should auto-disable (or trip a breaker flag) after 3 consecutive failures.
As per coding guidelines "Implement circuit breaker: auto-disable recurring tasks after 3 consecutive failures. Apply to cron jobs, maintenance workers, and cortex routines".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/cortex.rs` around lines 1004 - 1011, The ready-task loop should
trip a simple circuit breaker after 3 consecutive errors: around the existing
loop calling pickup_one_ready_task(deps, logger).await, add a persistent failure
counter (AtomicUsize) and a breaker flag (AtomicBool) on the runtime
state/config (e.g., add ready_task_enabled: AtomicBool and ready_task_failures:
AtomicUsize to the cortex/runtime config struct). Before sleeping, check
ready_task_enabled and skip/exit the loop if false; when pickup_one_ready_task
returns Err, increment ready_task_failures and if it reaches 3 set
ready_task_enabled to false and emit an error-level log indicating the breaker
tripped; on Ok, reset ready_task_failures to 0. Ensure you reference
pickup_one_ready_task and deps.runtime_config.cortex (or the new
ready_task_enabled/ready_task_failures fields) so callers can find and toggle
the breaker state.
Comment on lines
+1015
to
+1035
| let Some(task) = deps.task_store.claim_next_ready(&deps.agent_id).await? else { | ||
| return Ok(()); | ||
| }; | ||
|
|
||
| logger.log( | ||
| "task_pickup_started", | ||
| &format!("Picked up ready task #{}", task.task_number), | ||
| Some(serde_json::json!({ | ||
| "task_number": task.task_number, | ||
| "title": task.title, | ||
| })), | ||
| ); | ||
|
|
||
| let prompt_engine = deps.runtime_config.prompts.load(); | ||
| let worker_system_prompt = prompt_engine | ||
| .render_worker_prompt( | ||
| &deps.runtime_config.instance_dir.display().to_string(), | ||
| &deps.runtime_config.workspace_dir.display().to_string(), | ||
| ) | ||
| .map_err(|error| anyhow::anyhow!("failed to render worker prompt: {error}"))?; | ||
|
|
Contributor
There was a problem hiding this comment.
Requeue the task if pickup setup fails after claim.
After claim_next_ready, any early ? (prompt render/update failure) exits without restoring the task, leaving it stuck in in_progress with no worker.
Proposed fix
async fn pickup_one_ready_task(deps: &AgentDeps, logger: &CortexLogger) -> anyhow::Result<()> {
let Some(task) = deps.task_store.claim_next_ready(&deps.agent_id).await? else {
return Ok(());
};
+ let task_number = task.task_number;
- let prompt_engine = deps.runtime_config.prompts.load();
- let worker_system_prompt = prompt_engine
- .render_worker_prompt(
- &deps.runtime_config.instance_dir.display().to_string(),
- &deps.runtime_config.workspace_dir.display().to_string(),
- )
- .map_err(|error| anyhow::anyhow!("failed to render worker prompt: {error}"))?;
+ let setup_result: anyhow::Result<()> = async {
+ let prompt_engine = deps.runtime_config.prompts.load();
+ let worker_system_prompt = prompt_engine
+ .render_worker_prompt(
+ &deps.runtime_config.instance_dir.display().to_string(),
+ &deps.runtime_config.workspace_dir.display().to_string(),
+ )
+ .map_err(|error| anyhow::anyhow!("failed to render worker prompt: {error}"))?;
- // ... build prompt / create worker ...
+ // ... build prompt / create worker ...
- deps.task_store
- .update(
- &deps.agent_id,
- task.task_number,
- UpdateTaskInput {
- worker_id: Some(worker_id.to_string()),
- ..Default::default()
- },
- )
- .await?;
+ deps.task_store
+ .update(
+ &deps.agent_id,
+ task.task_number,
+ UpdateTaskInput {
+ worker_id: Some(worker_id.to_string()),
+ ..Default::default()
+ },
+ )
+ .await?;
- tokio::spawn(async move {
- // existing worker run logic
- });
+ tokio::spawn(async move {
+ // existing worker run logic
+ });
+ Ok(())
+ }
+ .await;
+
+ if let Err(error) = setup_result {
+ if let Err(requeue_error) = deps
+ .task_store
+ .update(
+ &deps.agent_id,
+ task_number,
+ UpdateTaskInput {
+ status: Some(TaskStatus::Ready),
+ clear_worker_id: true,
+ ..Default::default()
+ },
+ )
+ .await
+ {
+ tracing::error!(%requeue_error, task_number, "failed to requeue task after pickup setup failure");
+ }
+ return Err(error);
+ }
Ok(())
}Also applies to: 1080-1089
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/cortex.rs` around lines 1015 - 1035, The code currently claims a
task via task_store.claim_next_ready(&deps.agent_id) and then proceeds with
setup steps like prompt_engine.render_worker_prompt which may return early with
?, leaving the task stuck as in_progress; modify the flow so that after a
successful claim you perform setup in a fallible block and on any error
explicitly release or requeue the claimed task (call the task store's
release/requeue method such as task_store.requeue or a similarly named method
with task.task_number/agent_id) before returning the error; apply the same
pattern for the later setup area (around render_worker_prompt and the section
noted at 1080-1089) to ensure any failure restores task state.
Comment on lines
+1091
to
+1097
| let _ = deps.event_tx.send(ProcessEvent::TaskUpdated { | ||
| agent_id: deps.agent_id.clone(), | ||
| task_number: task.task_number, | ||
| status: "in_progress".to_string(), | ||
| action: "updated".to_string(), | ||
| }); | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use .ok() (or explicit handling) instead of let _ = for channel sends.
These sends are exactly the allowed exception case; using .ok() keeps this compliant and explicit.
Proposed fix
- let _ = deps.event_tx.send(ProcessEvent::TaskUpdated { ... });
+ deps.event_tx.send(ProcessEvent::TaskUpdated { ... }).ok();
- let _ = deps.event_tx.send(ProcessEvent::WorkerStarted { ... });
+ deps.event_tx.send(ProcessEvent::WorkerStarted { ... }).ok();
- let _ = event_tx.send(ProcessEvent::TaskUpdated { ... });
+ event_tx.send(ProcessEvent::TaskUpdated { ... }).ok();
- let _ = event_tx.send(ProcessEvent::WorkerComplete { ... });
+ event_tx.send(ProcessEvent::WorkerComplete { ... }).ok();
- let _ = event_tx.send(ProcessEvent::TaskUpdated { ... });
+ event_tx.send(ProcessEvent::TaskUpdated { ... }).ok();
- let _ = event_tx.send(ProcessEvent::WorkerComplete { ... });
+ event_tx.send(ProcessEvent::WorkerComplete { ... }).ok();As per coding guidelines "Don't silently discard errors; no let _ = on Results. Handle, log, or propagate errors. Only exception is .ok() on channel sends where the receiver may be dropped".
Also applies to: 1100-1107, 1139-1145, 1156-1163, 1187-1193, 1205-1212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/cortex.rs` around lines 1091 - 1097, Replace the silent Result
discard pattern used when sending events on the channel with explicit .ok()
handling: locate occurrences of deps.event_tx.send(ProcessEvent::TaskUpdated {
... }) (and the other similar sends around the same block) and change them to
call .ok() on the returned Result so the send is explicitly ignored only in the
allowed channel-drop case; update every occurrence (e.g., the TaskUpdated sends
and other ProcessEvent variants at the mentioned locations) to use
deps.event_tx.send(...).ok() rather than using let _ =.
Comment on lines
+226
to
+229
| .map_err(|error| { | ||
| tracing::warn!(%error, agent_id = %request.agent_id, task_number = number, "failed to update task"); | ||
| StatusCode::INTERNAL_SERVER_ERROR | ||
| })? |
Contributor
There was a problem hiding this comment.
Map task transition/domain errors to 4xx instead of 500.
store.update(...) can return domain validation failures (e.g., illegal status transition), but these branches currently convert everything to INTERNAL_SERVER_ERROR. That makes client errors look like server outages and encourages bad retries.
Proposed fix
+fn map_task_update_error(error: crate::error::Error) -> StatusCode {
+ let message = error.to_string();
+ if message.contains("invalid task status transition") {
+ StatusCode::BAD_REQUEST
+ } else {
+ tracing::warn!(%error, "task update failed");
+ StatusCode::INTERNAL_SERVER_ERROR
+ }
+}
...
- .map_err(|error| {
- tracing::warn!(%error, agent_id = %request.agent_id, task_number = number, "failed to update task");
- StatusCode::INTERNAL_SERVER_ERROR
- })?
+ .map_err(map_task_update_error)?Also applies to: 300-303, 358-361
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/tasks.rs` around lines 226 - 229, The map_err closures wrapping
store.update(...) currently convert all errors to INTERNAL_SERVER_ERROR; change
them to inspect the returned error (e.g., match on the error's enum/variant or
use an is_validation/is_domain_error helper) and map domain/validation failures
(illegal status transition, invalid input) to StatusCode::BAD_REQUEST while
still mapping unexpected/storage errors to StatusCode::INTERNAL_SERVER_ERROR;
update the three occurrences around the current diff and the other spots you
noted (the closures at ~lines 226-229, 300-303, and 358-361) and keep the
existing tracing::warn! calls but include the chosen StatusCode mapping logic.
Comment on lines
+327
to
+331
| if let Some(index) = input.complete_subtask | ||
| && let Some(subtask) = subtasks.get_mut(index) | ||
| { | ||
| subtask.completed = true; | ||
| } |
Contributor
There was a problem hiding this comment.
Return an error for out-of-range complete_subtask indices.
An invalid index currently no-ops and still reports success, which is misleading for callers and tools.
Proposed fix
let mut subtasks = input.subtasks.unwrap_or(current.subtasks);
- if let Some(index) = input.complete_subtask
- && let Some(subtask) = subtasks.get_mut(index)
- {
- subtask.completed = true;
+ if let Some(index) = input.complete_subtask {
+ let Some(subtask) = subtasks.get_mut(index) else {
+ return Err(crate::error::Error::Other(anyhow::anyhow!(
+ "complete_subtask index {} out of range (len={})",
+ index,
+ subtasks.len()
+ )));
+ };
+ subtask.completed = true;
}📝 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 let Some(index) = input.complete_subtask | |
| && let Some(subtask) = subtasks.get_mut(index) | |
| { | |
| subtask.completed = true; | |
| } | |
| if let Some(index) = input.complete_subtask { | |
| let Some(subtask) = subtasks.get_mut(index) else { | |
| return Err(crate::error::Error::Other(anyhow::anyhow!( | |
| "complete_subtask index {} out of range (len={})", | |
| index, | |
| subtasks.len() | |
| ))); | |
| }; | |
| subtask.completed = true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tasks/store.rs` around lines 327 - 331, The code silently no-ops when
input.complete_subtask contains an out-of-range index; change the logic so that
when input.complete_subtask is Some(index) you attempt to get_mut on subtasks
and if get_mut returns None you return a proper error (e.g. Err(...) or an
appropriate StoreError) instead of continuing, otherwise set subtask.completed =
true; use input.complete_subtask and subtasks.get_mut(index) to locate the place
to return the error so callers receive a failure for invalid indices.
src/tasks/store.rs
Outdated
Comment on lines
633
to
639
| fn parse_subtasks(value: &str) -> Vec<TaskSubtask> { | ||
| serde_json::from_str(value).unwrap_or_default() | ||
| } | ||
|
|
||
| fn parse_metadata(value: &str) -> Value { | ||
| serde_json::from_str(value).unwrap_or_else(|_| Value::Object(serde_json::Map::new())) | ||
| } |
Contributor
There was a problem hiding this comment.
Stop swallowing subtasks/metadata decode failures.
Falling back to [] / {} on decode errors hides data corruption and can lead to silent data loss in follow-up updates.
Proposed fix
-fn parse_subtasks(value: &str) -> Vec<TaskSubtask> {
- serde_json::from_str(value).unwrap_or_default()
+fn parse_subtasks(value: &str) -> Result<Vec<TaskSubtask>> {
+ serde_json::from_str(value).context("failed to parse task subtasks JSON").map_err(Into::into)
}
-fn parse_metadata(value: &str) -> Value {
- serde_json::from_str(value).unwrap_or_else(|_| Value::Object(serde_json::Map::new()))
+fn parse_metadata(value: &str) -> Result<Value> {
+ serde_json::from_str(value).context("failed to parse task metadata JSON").map_err(Into::into)
}
@@
- let subtasks_value: String = row.try_get("subtasks").unwrap_or_else(|_| "[]".to_string());
- let metadata_value: String = row.try_get("metadata").unwrap_or_else(|_| "{}".to_string());
+ let subtasks_value: Option<String> = row.try_get("subtasks").context("failed to read task subtasks")?;
+ let metadata_value: Option<String> = row.try_get("metadata").context("failed to read task metadata")?;
@@
- subtasks: parse_subtasks(&subtasks_value),
- metadata: parse_metadata(&metadata_value),
+ subtasks: parse_subtasks(subtasks_value.as_deref().unwrap_or("[]"))?,
+ metadata: parse_metadata(metadata_value.as_deref().unwrap_or("{}"))?,As per coding guidelines "Don't silently discard errors; no let _ = on Results. Handle, log, or propagate errors. Only exception is .ok() on channel sends where the receiver may be dropped".
Also applies to: 648-650
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tasks/store.rs` around lines 633 - 639, parse_subtasks and parse_metadata
currently swallow JSON decode failures by returning empty defaults; change them
to propagate errors instead of hiding them: update parse_subtasks to return
Result<Vec<TaskSubtask>, serde_json::Error> and parse_metadata to return
Result<Value, serde_json::Error>, replace unwrap_or_default / unwrap_or_else
with serde_json::from_str(...)?, and propagate the Result up to callers (or log
and convert to an Err at the callsite) so callers can handle or log corrupt
data; apply the same change for the other similar parsing helpers referenced
around the 648-650 area.
…_after_test_module
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/tasks/store.rs (2)
327-331:⚠️ Potential issue | 🟡 MinorReturn an error when
complete_subtaskindex is out of range.At Line 327, an invalid index currently no-ops and still returns success, which is misleading to callers.
Proposed fix
- if let Some(index) = input.complete_subtask - && let Some(subtask) = subtasks.get_mut(index) - { - subtask.completed = true; - } + if let Some(index) = input.complete_subtask { + let Some(subtask) = subtasks.get_mut(index) else { + return Err(crate::error::Error::Other(anyhow::anyhow!( + "complete_subtask index {} out of range (len={})", + index, + subtasks.len() + ))); + }; + subtask.completed = true; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tasks/store.rs` around lines 327 - 331, The current branch that handles input.complete_subtask silently no-ops when the provided index is out of range; change it so that when input.complete_subtask is Some(index) you explicitly validate index against subtasks.len() and return an error (eg. Err(StoreError::InvalidSubtaskIndex) or a suitable Result::Err) if index >= subtasks.len(), otherwise use subtasks.get_mut(index) and set subtask.completed = true; update the surrounding function signature/return path to propagate this Result error instead of always returning success so callers receive a clear out-of-range error.
500-506:⚠️ Potential issue | 🟠 MajorStop swallowing JSON/row decode errors in task hydration.
Line 500, Line 505, and several
task_from_rowfields currently default on decode/read failures (unwrap_or_default,unwrap_or_else,.ok()), which can hide corruption and silently drop persisted data.Proposed fix
-fn parse_subtasks(value: &str) -> Vec<TaskSubtask> { - serde_json::from_str(value).unwrap_or_default() +fn parse_subtasks(value: &str) -> Result<Vec<TaskSubtask>> { + serde_json::from_str(value) + .context("failed to parse task subtasks JSON") + .map_err(Into::into) } -fn parse_metadata(value: &str) -> Value { - serde_json::from_str(value).unwrap_or_else(|_| Value::Object(serde_json::Map::new())) +fn parse_metadata(value: &str) -> Result<Value> { + serde_json::from_str(value) + .context("failed to parse task metadata JSON") + .map_err(Into::into) } @@ - let subtasks_value: String = row.try_get("subtasks").unwrap_or_else(|_| "[]".to_string()); - let metadata_value: String = row.try_get("metadata").unwrap_or_else(|_| "{}".to_string()); + let subtasks_value: Option<String> = row + .try_get("subtasks") + .context("failed to read task subtasks")?; + let metadata_value: Option<String> = row + .try_get("metadata") + .context("failed to read task metadata")?; @@ - description: row.try_get("description").ok(), + description: row + .try_get::<Option<String>, _>("description") + .context("failed to read task description")?, @@ - subtasks: parse_subtasks(&subtasks_value), - metadata: parse_metadata(&metadata_value), - source_memory_id: row.try_get("source_memory_id").ok(), + subtasks: parse_subtasks(subtasks_value.as_deref().unwrap_or("[]"))?, + metadata: parse_metadata(metadata_value.as_deref().unwrap_or("{}"))?, + source_memory_id: row + .try_get::<Option<String>, _>("source_memory_id") + .context("failed to read task source_memory_id")?, @@ - approved_by: row.try_get("approved_by").ok(), + approved_by: row + .try_get::<Option<String>, _>("approved_by") + .context("failed to read task approved_by")?, @@ - completed_at: row - .try_get::<chrono::NaiveDateTime, _>("completed_at") - .ok() - .map(|v| v.and_utc().to_rfc3339()), + completed_at: row + .try_get::<Option<chrono::NaiveDateTime>, _>("completed_at") + .context("failed to read task completed_at")? + .map(|v| v.and_utc().to_rfc3339()),As per coding guidelines "Don't silently discard errors; no let _ = on Results. Handle, log, or propagate errors. Only exception is .ok() on channel sends where the receiver may be dropped".
Also applies to: 515-517, 532-537, 551-563
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tasks/store.rs` around lines 500 - 506, parse_subtasks and parse_metadata currently swallow JSON decode errors (using unwrap_or_default/unwrap_or_else) and task_from_row uses .ok()/defaults for several fields (lines referenced) which hides data corruption; change these helpers to return Result (e.g., parse_subtasks -> Result<Vec<TaskSubtask>, serde_json::Error>, parse_metadata -> Result<Value, serde_json::Error>) and update task_from_row to propagate or return a descriptive error (rather than .ok() or defaults) when deserialization fails, including contextual info (field name and raw string) in the error or log; ensure all occurrences that currently use unwrap_or_default/unwrap_or_else/.ok() are replaced to handle, return, or log the error with context instead of silently defaulting.
🤖 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/tasks/store.rs`:
- Around line 415-451: claim_next_ready currently does a separate SELECT then
UPDATE, leaving a race between claimers; replace that two-step pattern in
claim_next_ready with a single atomic UPDATE that selects the target row inside
the WHERE clause (for example: UPDATE tasks SET status='in_progress', updated_at
= datetime('now') WHERE rowid = (SELECT rowid FROM tasks WHERE agent_id = ? AND
status = 'ready' ORDER BY CASE priority WHEN 'critical' THEN 0 WHEN 'high' THEN
1 WHEN 'medium' THEN 2 WHEN 'low' THEN 3 ELSE 4 END ASC, task_number ASC LIMIT
1)); bind agent_id, execute, check rows_affected() and only call
get_by_number(agent_id, task_number) when the UPDATE affected a row (you can
SELECT task_number from that same subquery or read it after successful UPDATE);
make this change inside the claim_next_ready function to remove the race window.
---
Duplicate comments:
In `@src/tasks/store.rs`:
- Around line 327-331: The current branch that handles input.complete_subtask
silently no-ops when the provided index is out of range; change it so that when
input.complete_subtask is Some(index) you explicitly validate index against
subtasks.len() and return an error (eg. Err(StoreError::InvalidSubtaskIndex) or
a suitable Result::Err) if index >= subtasks.len(), otherwise use
subtasks.get_mut(index) and set subtask.completed = true; update the surrounding
function signature/return path to propagate this Result error instead of always
returning success so callers receive a clear out-of-range error.
- Around line 500-506: parse_subtasks and parse_metadata currently swallow JSON
decode errors (using unwrap_or_default/unwrap_or_else) and task_from_row uses
.ok()/defaults for several fields (lines referenced) which hides data
corruption; change these helpers to return Result (e.g., parse_subtasks ->
Result<Vec<TaskSubtask>, serde_json::Error>, parse_metadata -> Result<Value,
serde_json::Error>) and update task_from_row to propagate or return a
descriptive error (rather than .ok() or defaults) when deserialization fails,
including contextual info (field name and raw string) in the error or log;
ensure all occurrences that currently use unwrap_or_default/unwrap_or_else/.ok()
are replaced to handle, return, or log the error with context instead of
silently defaulting.
Comment on lines
+415
to
+451
| let row = sqlx::query( | ||
| "SELECT task_number FROM tasks WHERE agent_id = ? AND status = 'ready' \ | ||
| ORDER BY CASE priority \ | ||
| WHEN 'critical' THEN 0 \ | ||
| WHEN 'high' THEN 1 \ | ||
| WHEN 'medium' THEN 2 \ | ||
| WHEN 'low' THEN 3 \ | ||
| ELSE 4 END ASC, \ | ||
| task_number ASC \ | ||
| LIMIT 1", | ||
| ) | ||
| .bind(agent_id) | ||
| .fetch_optional(&self.pool) | ||
| .await | ||
| .context("failed to find ready task")?; | ||
|
|
||
| let Some(row) = row else { | ||
| return Ok(None); | ||
| }; | ||
|
|
||
| let task_number: i64 = row | ||
| .try_get("task_number") | ||
| .context("failed to read task_number from ready task row")?; | ||
| let result = sqlx::query( | ||
| "UPDATE tasks SET status = 'in_progress', updated_at = datetime('now') WHERE agent_id = ? AND task_number = ? AND status = 'ready'", | ||
| ) | ||
| .bind(agent_id) | ||
| .bind(task_number) | ||
| .execute(&self.pool) | ||
| .await | ||
| .context("failed to claim ready task")?; | ||
|
|
||
| if result.rows_affected() == 0 { | ||
| return Ok(None); | ||
| } | ||
|
|
||
| self.get_by_number(agent_id, task_number).await |
Contributor
There was a problem hiding this comment.
claim_next_ready has a race window between SELECT and UPDATE.
Line 415 and Line 438 run as separate statements, so concurrent claimers can read the same ready task; one then returns None even if other ready tasks exist. Claim should be a single atomic DB operation.
Proposed atomic claim pattern
- let row = sqlx::query(
- "SELECT task_number FROM tasks WHERE agent_id = ? AND status = 'ready' \
- ORDER BY CASE priority \
- WHEN 'critical' THEN 0 \
- WHEN 'high' THEN 1 \
- WHEN 'medium' THEN 2 \
- WHEN 'low' THEN 3 \
- ELSE 4 END ASC, \
- task_number ASC \
- LIMIT 1",
- )
- .bind(agent_id)
- .fetch_optional(&self.pool)
- .await
- .context("failed to find ready task")?;
-
- let Some(row) = row else {
- return Ok(None);
- };
-
- let task_number: i64 = row
- .try_get("task_number")
- .context("failed to read task_number from ready task row")?;
- let result = sqlx::query(
- "UPDATE tasks SET status = 'in_progress', updated_at = datetime('now') WHERE agent_id = ? AND task_number = ? AND status = 'ready'",
- )
- .bind(agent_id)
- .bind(task_number)
- .execute(&self.pool)
- .await
- .context("failed to claim ready task")?;
-
- if result.rows_affected() == 0 {
- return Ok(None);
- }
+ let row = sqlx::query(
+ "WITH candidate AS ( \
+ SELECT task_number FROM tasks \
+ WHERE agent_id = ? AND status = 'ready' \
+ ORDER BY CASE priority \
+ WHEN 'critical' THEN 0 \
+ WHEN 'high' THEN 1 \
+ WHEN 'medium' THEN 2 \
+ WHEN 'low' THEN 3 \
+ ELSE 4 END ASC, \
+ task_number ASC \
+ LIMIT 1 \
+ ) \
+ UPDATE tasks \
+ SET status = 'in_progress', updated_at = datetime('now') \
+ WHERE agent_id = ? \
+ AND task_number = (SELECT task_number FROM candidate) \
+ AND status = 'ready' \
+ RETURNING task_number",
+ )
+ .bind(agent_id)
+ .bind(agent_id)
+ .fetch_optional(&self.pool)
+ .await
+ .context("failed to claim ready task")?;
+
+ let Some(row) = row else {
+ return Ok(None);
+ };
+
+ let task_number: i64 = row
+ .try_get("task_number")
+ .context("failed to read claimed task_number")?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tasks/store.rs` around lines 415 - 451, claim_next_ready currently does a
separate SELECT then UPDATE, leaving a race between claimers; replace that
two-step pattern in claim_next_ready with a single atomic UPDATE that selects
the target row inside the WHERE clause (for example: UPDATE tasks SET
status='in_progress', updated_at = datetime('now') WHERE rowid = (SELECT rowid
FROM tasks WHERE agent_id = ? AND status = 'ready' ORDER BY CASE priority WHEN
'critical' THEN 0 WHEN 'high' THEN 1 WHEN 'medium' THEN 2 WHEN 'low' THEN 3 ELSE
4 END ASC, task_number ASC LIMIT 1)); bind agent_id, execute, check
rows_affected() and only call get_by_number(agent_id, task_number) when the
UPDATE affected a row (you can SELECT task_number from that same subquery or
read it after successful UPDATE); make this change inside the claim_next_ready
function to remove the race window.
This was referenced Feb 26, 2026
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
1 participant
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.
Summary
Task system backend —
TaskStorewith CRUD, status transitions (pending_approval→backlog→ready→in_progress→done), priority-awareclaim_next_ready, and per-agent SQLite storage. Three LLM tools (task_create,task_list,task_update) available to branches and cortex chat, with scoped-downtask_updatefor workers (subtasks/metadata only).Cortex auto-execution — background loop picks up
readytasks everytick_interval_secs, spawns workers, and handles completion/failure (re-queue on failure). Task workers are logged directly toworker_runsso they appear in the Workers tab. The/executeAPI endpoint moves tasks toreadyfor cortex pickup rather than spawning workers directly.SSE real-time updates —
TaskUpdatedevent flows throughProcessEvent→ApiEvent→ SSE serializer. Emitted from all 7 API handlers and cortex pickup/complete/requeue paths. The kanban board updates live viataskEventVersioninuseLiveContext.Kanban board UI — 5-column board (
AgentTasks.tsx) with task cards, priority badges, subtask progress bars, quick action buttons (Approve, Execute, Mark Done), create dialog, and detail dialog. Wired as a "Tasks" tab in the agent page.Bulletin integration —
gather_active_tasks()includes non-done tasks in the cortex memory bulletin, giving all channels/branches awareness of the current task board.Spec-driven prompts — Task descriptions are full markdown specs (requirements, constraints, file paths, acceptance criteria) with pre-filled subtask execution plans. All prompts (channel, branch, cortex_chat) and tool descriptions updated to reinforce this philosophy. Tasks are living documents refined through conversation.
Link-conversation teardown — Stripped ~830 lines of LLM-to-LLM conversation machinery from link channels.
send_agent_messagekept as a stub. Link topology infrastructure (AgentLink, config parsing, org hierarchy) preserved for future task-based delegation.Changed files
tasks.rs,tasks/store.rs,api/tasks.rs,tools/task_create.rs,tools/task_list.rs,tools/task_update.rs,AgentTasks.tsx,tasks.mdxconclude_link.rs,link_context.md.j2,conclude_link_description.md.j220260219000001_tasks.sqlTesting
cargo fmtcleantests/bulletin.rs) fail due to redb lock conflict with running instance — not related to this PRNote
Task tracking and delegation system complete. New
TaskStorebackend manages full task lifecycle with state machine (pending_approval → backlog → ready → in_progress → done). Cortex auto-execution picks up ready tasks on configurable intervals and spawns workers. Real-time kanban UI (5-column board with task cards, priorities, subtask progress, and action buttons) updates via SSE. Task descriptions are spec-driven markdown documents with requirements, constraints, and acceptance criteria. Removed ~830 lines of legacy LLM-to-LLM link conversation machinery; link infrastructure preserved for future task-based delegation. All tests pass except 2 integration tests with unrelated redb lock conflicts.Written by Tembo for commit b5675b7.