omarluq/sub-agents#205
Open
omarluq wants to merge 5 commits into
Open
Conversation
Add background agent execution, task persistence, lifecycle tools, and terminal task monitoring.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds agent definition discovery, durable asynchronous agent tasks, assistant agent-management tools, execution profiles, database persistence, dependency wiring, and terminal UI support for monitoring, inspecting, canceling, and displaying agent-task completions. ChangesAgent task platform
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AssistantRuntime
participant AgentTaskService
participant RuntimeRunner
participant Database
participant Terminal
User->>AssistantRuntime: invoke agent_start
AssistantRuntime->>AgentTaskService: submit agent task
AgentTaskService->>Database: persist queued task
AgentTaskService->>RuntimeRunner: claim and run task
RuntimeRunner->>AssistantRuntime: execute profiled prompt
AssistantRuntime-->>AgentTaskService: stream task events
AgentTaskService->>Database: persist events and result
AgentTaskService-->>Terminal: publish task updates
Terminal-->>User: render progress and completion
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #205 +/- ##
==========================================
+ Coverage 83.59% 84.30% +0.70%
==========================================
Files 287 296 +9
Lines 23363 25574 +2211
==========================================
+ Hits 19531 21559 +2028
- Misses 2662 2775 +113
- Partials 1170 1240 +70
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/terminal/async_events.go (1)
613-648: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPrevent agent-management tool starts from creating running blocks. At
internal/terminal/async_events.go:613-648,applyStreamedToolStartcan append a block fromfallbackNamewhencallis nil, butapplyStreamedToolEventreturns early for agent-management tools beforeremoveRunningToolBlock. If anagent_*start arrives without a full call payload, the running indicator can stick. Fold the fallback-name case into the agent guard, or mirror the same filter on the result path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminal/async_events.go` around lines 613 - 648, Update applyStreamedToolStart to reject agent-management tools after resolving a nil call with fallbackName, so fallback-created agent_* calls never append to runningToolBlocks. Preserve the existing handling for non-agent tools and ensure the guard covers both populated calls and fallback-name cases.
🧹 Nitpick comments (8)
internal/assistant/agent_tool.go (1)
79-105: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache tool definitions in
Definition()
Definition()is called from registry listing and execution paths, so rebuilding the 5-entry map and re-runningschemaForAgentStart()on every call adds avoidable work. Memoizing the per-executor definition would keep prompt/tool registration cheaper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/assistant/agent_tool.go` around lines 79 - 105, The Definition method currently rebuilds all tool definitions and schemas on every call. Add per-executor memoization around agentToolExecutor.Definition, caching the constructed definitions or returned definition while preserving executor-specific schemaForAgentStart behavior and the existing registry results.internal/agenttask/service.go (3)
475-489: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffSession-slot backpressure retries via a tight sleep loop instead of freeing the worker.
When
acquireSessionSlotfails, the worker blocks in a 10ms sleep → re-enqueue loop rather than returning to service other queued tasks; under sustained per-session backlog this can tie up a large share of the fixeddefaultConcurrencyworker pool on retries for one session while unrelated tasks wait.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agenttask/service.go` around lines 475 - 489, Update the acquireSessionSlot failure path and requeue flow so a worker returns to processing other queued tasks instead of repeatedly sleeping and retrying the same task in a tight loop. Preserve delayed retry behavior by re-enqueuing the task after dispatchRetryInterval without holding a worker slot during the wait, using the existing Service queue and context cancellation mechanisms.
491-502: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winManual JSON string-building for the reject payload is fragile.
`{"error_code":"`+errorCode+`"}`is hand-built rather thanjson.Marshal'd, unlikefinish()(line 620) which marshals properly. Currently safe since only fixed literal error codes are passed here, but iferrorCode/similar ever contains a quote or backslash, the resulting invalid JSON would makevalidateEventreject it, causingFinishto fail — and per the error-swallowing issue above, that failure is silently dropped, leaving the task stuck inqueued.♻️ Suggested fix
+ payload, err := json.Marshal(map[string]string{"error_code": errorCode}) + if err != nil { + return + } + changed, err := service.tasks.Finish( ctx, taskID, []database.TaskState{database.TaskQueued}, database.TaskFailed, - "task_failed", "", errorCode, errorMessage, `{"error_code":"`+errorCode+`"}`, + "task_failed", "", errorCode, errorMessage, string(payload), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agenttask/service.go` around lines 491 - 502, Update rejectQueuedTask to construct the reject payload through json.Marshal, matching the serialization used by finish(), so errorCode is safely escaped and valid JSON is always passed to tasks.Finish. Preserve the existing Finish and publishLatest flow, handling any marshal failure consistently with the surrounding error path.
361-385: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff
Awaitpolls the DB every 50ms instead of using the existingSubscribemechanism.The file already has an event-driven
Subscribe/Eventspath;Awaitcould listen on a subscription (falling back to polling only as a safety net) instead of unconditionally pollingagentTasks.Getfor up to the full 30-minute default timeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agenttask/service.go` around lines 361 - 385, Update Service.Await to use the existing Subscribe/Events mechanism for task completion notifications instead of unconditionally polling agentTasks.Get every 50ms. Subscribe to the requested task, return when a terminal event/state is observed, preserve not-found and context-cancellation errors, and retain polling only as a safety-net fallback if the subscription path cannot provide completion.internal/database/task_repository.go (1)
268-321: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTask-finish call chain exceeds the project's max-parameter threshold in four places. SonarCloud flags all of these for having more than 7 parameters; they share one root cause — the terminal-state fields (
result,errorCode,errorMessage,payloadJSON,usageJSON) are threaded positionally through the whole chain. Bundling them into a small parameter struct (e.g.TaskOutcome{Result, ErrorCode, ErrorMessage, PayloadJSON}, extended withUsageJSONfor the agent variant) would resolve all four warnings and remove the argument-order risk this review itself had to carefully trace.
internal/database/task_repository.go#L268-L321:finishTransaction(10 params) — introduce the outcome struct here first, since bothFinishmethods route through it.internal/database/task_repository.go#L230-L266:Finish(9 params) — accept/forward the same outcome struct.internal/database/agent_task_repository.go#L104-L145:Finish(10 params) — accept the outcome struct plususageJSON.internal/database/agent_task_repository_test.go#L85-L100:assertTaskTransition(8 params) — simplify once the production signatures shrink, or pass a small test-local struct/table row.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/database/task_repository.go` around lines 268 - 321, The task-finish call chain passes terminal-state fields positionally through too many parameters. In internal/database/task_repository.go lines 268-321, introduce a TaskOutcome struct containing Result, ErrorCode, ErrorMessage, and PayloadJSON, and update finishTransaction to accept it; apply the same struct to Finish at lines 230-266. In internal/database/agent_task_repository.go lines 104-145, extend or combine the outcome with UsageJSON and update Finish accordingly. In internal/database/agent_task_repository_test.go lines 85-100, simplify assertTaskTransition using the reduced signatures or a small test-local struct/table row.Source: Linters/SAST tools
internal/assistant/tool_registry.go (1)
58-58: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid allocating a full
Definitions()slice just to check non-emptiness.
Definitions()builds a sorted slice and deep-copiesToolsfor every entry; using it only to testlen(...) > 0allocates unnecessarily on every call topromptToolRegistry. Consider adding a lightweightLen()/HasDefinitions()method onCatalogbacked bylen(catalog.definitions).♻️ Proposed fix
+// in internal/agent/catalog.go +func (catalog *Catalog) Len() int { + if catalog == nil { + return 0 + } + return len(catalog.definitions) +}- if runtime.agents != nil && runtime.agentTasks != nil && len(runtime.agents.Definitions()) > 0 { + if runtime.agents != nil && runtime.agentTasks != nil && runtime.agents.Len() > 0 {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/assistant/tool_registry.go` at line 58, Replace the len(runtime.agents.Definitions()) check in promptToolRegistry with a lightweight Catalog method such as Len() or HasDefinitions() that reads len(catalog.definitions) directly. Add the method to Catalog and preserve the existing nil and non-empty conditions without invoking Definitions().internal/terminal/commands.go (1)
111-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid calling
AgentDiagnostics()twice.
app.runtime.AgentDiagnostics()is called once for the slice-capacity hint and again in the loop. Cache it in a local variable and reuse.♻️ Proposed refactor
- lines := make([]string, 0, len(definitions)+len(app.runtime.AgentDiagnostics())) + diagnostics := app.runtime.AgentDiagnostics() + lines := make([]string, 0, len(definitions)+len(diagnostics)) for index := range definitions { definition := &definitions[index] lines = append(lines, strings.Join([]string{ definition.Name + ": " + definition.Description, " source: " + definition.SourceInfo.Path, " tools: " + agentToolNames(definition.Tools), " permissions: " + string(definition.Permissions), }, "\n")) } - for _, diagnostic := range app.runtime.AgentDiagnostics() { + for _, diagnostic := range diagnostics { lines = append(lines, "warning: "+diagnostic.Path+": "+diagnostic.Message) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminal/commands.go` around lines 111 - 135, Cache the result of app.runtime.AgentDiagnostics() in a local variable within showAgentProfiles, use that variable for the lines capacity calculation, and reuse it in the diagnostics loop instead of calling AgentDiagnostics() twice.internal/agenttask/runtime_runner.go (1)
22-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
oopsfor consistency with the rest of the file.Every other error path in this file (lines 48-49, 53-54, 73, 82-83, 93, 101-102) uses
oops.In("agenttask").Code(...), but this constructor returns a plainerrors.New, losing domain/code context that the rest of the package relies on.♻️ Proposed fix
if runtime == nil || catalog == nil || sessions == nil { - return nil, errors.New("agenttask: runtime, agent catalog, and sessions are required") + return nil, oops.In("agenttask").Code("invalid_dependencies"). + Errorf("runtime, agent catalog, and sessions are required") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agenttask/runtime_runner.go` around lines 22 - 33, Update NewRuntimeRunner’s nil-argument validation to return an oops.In("agenttask").Code(...) error consistent with the other error paths in the file, preserving the existing required-dependencies message while adding the package’s domain/code context.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/agent/catalog.go`:
- Around line 160-172: Update the embedded filesystem path construction in the
catalog loading loop to use slash-based path joining via path.Join rather than
filepath.Join, while retaining filepath.Join in Load for real OS directories
passed to os.DirFS.
In `@internal/agenttask/service_test.go`:
- Line 13: Add an explanatory comment immediately above the blank
modernc.org/sqlite import, documenting that it is imported for SQLite driver
registration or initialization side effects. Keep the import unchanged.
In `@internal/agenttask/service.go`:
- Around line 420-455: add structured error logging for every swallowed failure
in Service.run and rejectQueuedTask, including agentTasks.Get, tasks.Transition,
and finish operations; add or reuse a Service/Options logger and emit at least
slog.Error with task ID and the underlying error before returning. Ensure run
logs transition failures so queued tasks are diagnosable, while preserving
existing recovery and finalization behavior.
In `@internal/assistant/agent_tool.go`:
- Around line 177-222: Update the agent-start flow in the function containing
the `executor.submit` call so a non-nil task returned with an error is still
surfaced to the caller, including its task ID via the tool result details (for
example, using `agentTaskDetails(task)`). Preserve the existing error return
while avoiding the unconditional `tool.TextResult("", nil)` path, and keep the
fully failed submission behavior unchanged.
In `@internal/assistant/runtime_context.go`:
- Around line 86-96: Update the tool guidance construction in the
non-ExecutionTopLevel branch so empty runtime.profile.Tools does not produce
“available tools ()”. Only set toolGuidance when at least one tool name exists,
while preserving the existing guidance text for non-empty tool lists.
In `@internal/assistant/runtime.go`:
- Around line 350-362: Add an explanatory comment directly above the intentional
no-op cancel function returned by SubscribeAgentTask when runtime or
runtime.agentTasks is nil, documenting that the channel is already closed and no
cancellation action is required. Keep the existing behavior unchanged.
In `@internal/terminal/agent_tasks.go`:
- Around line 102-138: The agent-task refresh path performs a per-task database
lookup on the main event loop every second. Update discoverActiveAgentTasks and
refreshActiveAgentTasks to eliminate per-task AgentTask calls by using a batched
AgentTasksByIDs query if available, or rely on the existing
watchAgentTask/postAgentTaskChanged flow with only a cheap list-level fallback;
preserve active-task filtering and watch registration without synchronous N+1
detail fetches.
- Around line 571-621: Update inspectAgentTask and leaveAgentTaskSession to
clear or re-scope app.agentTasks, app.agentTaskWatches, and
app.deliveredAgentTasks whenever app.sessionID changes, preventing prior-session
completions from entering the active transcript. Replace the single
agentTaskParentSession return target with stack semantics: push the current
session on inspect and pop the most recent parent on leave, preserving nested
inspections.
---
Outside diff comments:
In `@internal/terminal/async_events.go`:
- Around line 613-648: Update applyStreamedToolStart to reject agent-management
tools after resolving a nil call with fallbackName, so fallback-created agent_*
calls never append to runningToolBlocks. Preserve the existing handling for
non-agent tools and ensure the guard covers both populated calls and
fallback-name cases.
---
Nitpick comments:
In `@internal/agenttask/runtime_runner.go`:
- Around line 22-33: Update NewRuntimeRunner’s nil-argument validation to return
an oops.In("agenttask").Code(...) error consistent with the other error paths in
the file, preserving the existing required-dependencies message while adding the
package’s domain/code context.
In `@internal/agenttask/service.go`:
- Around line 475-489: Update the acquireSessionSlot failure path and requeue
flow so a worker returns to processing other queued tasks instead of repeatedly
sleeping and retrying the same task in a tight loop. Preserve delayed retry
behavior by re-enqueuing the task after dispatchRetryInterval without holding a
worker slot during the wait, using the existing Service queue and context
cancellation mechanisms.
- Around line 491-502: Update rejectQueuedTask to construct the reject payload
through json.Marshal, matching the serialization used by finish(), so errorCode
is safely escaped and valid JSON is always passed to tasks.Finish. Preserve the
existing Finish and publishLatest flow, handling any marshal failure
consistently with the surrounding error path.
- Around line 361-385: Update Service.Await to use the existing Subscribe/Events
mechanism for task completion notifications instead of unconditionally polling
agentTasks.Get every 50ms. Subscribe to the requested task, return when a
terminal event/state is observed, preserve not-found and context-cancellation
errors, and retain polling only as a safety-net fallback if the subscription
path cannot provide completion.
In `@internal/assistant/agent_tool.go`:
- Around line 79-105: The Definition method currently rebuilds all tool
definitions and schemas on every call. Add per-executor memoization around
agentToolExecutor.Definition, caching the constructed definitions or returned
definition while preserving executor-specific schemaForAgentStart behavior and
the existing registry results.
In `@internal/assistant/tool_registry.go`:
- Line 58: Replace the len(runtime.agents.Definitions()) check in
promptToolRegistry with a lightweight Catalog method such as Len() or
HasDefinitions() that reads len(catalog.definitions) directly. Add the method to
Catalog and preserve the existing nil and non-empty conditions without invoking
Definitions().
In `@internal/database/task_repository.go`:
- Around line 268-321: The task-finish call chain passes terminal-state fields
positionally through too many parameters. In
internal/database/task_repository.go lines 268-321, introduce a TaskOutcome
struct containing Result, ErrorCode, ErrorMessage, and PayloadJSON, and update
finishTransaction to accept it; apply the same struct to Finish at lines
230-266. In internal/database/agent_task_repository.go lines 104-145, extend or
combine the outcome with UsageJSON and update Finish accordingly. In
internal/database/agent_task_repository_test.go lines 85-100, simplify
assertTaskTransition using the reduced signatures or a small test-local
struct/table row.
In `@internal/terminal/commands.go`:
- Around line 111-135: Cache the result of app.runtime.AgentDiagnostics() in a
local variable within showAgentProfiles, use that variable for the lines
capacity calculation, and reuse it in the diagnostics loop instead of calling
AgentDiagnostics() twice.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 58b6dbd1-2670-45c6-a629-0a24d0843c86
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (59)
cmd/librecode/prompt.gointernal/agent/builtin/explore.mdinternal/agent/builtin/general.mdinternal/agent/catalog.gointernal/agent/catalog_test.gointernal/agenttask/runtime_runner.gointernal/agenttask/service.gointernal/agenttask/service_test.gointernal/assistant/agent_tool.gointernal/assistant/context_auto_compaction.gointernal/assistant/context_build.gointernal/assistant/execution_profile.gointernal/assistant/provider_hook_test_helpers_internal_test.gointernal/assistant/runtime.gointernal/assistant/runtime_context.gointernal/assistant/runtime_entries.gointernal/assistant/runtime_model.gointernal/assistant/runtime_test.gointernal/assistant/testing.gointernal/assistant/tool_registry.gointernal/assistant/tool_schema_cache_internal_test.gointernal/database/agent_task_repository.gointernal/database/agent_task_repository_test.gointernal/database/migrations/00006_create_tasks_events.sqlinternal/database/session_message_repository.gointernal/database/session_repository.gointernal/database/session_repository_test.gointernal/database/session_store.gointernal/database/task_repository.gointernal/database/validation.gointernal/di/agent_task_service.gointernal/di/assistant_service.gointernal/di/assistant_service_internal_test.gointernal/di/container.gointernal/di/database_service.gointernal/di/database_service_internal_test.gointernal/di/model_service_internal_test.gointernal/di/register.gointernal/di/service_constructors_internal_test.gointernal/terminal/agent_tasks.gointernal/terminal/agent_tasks_live_internal_test.gointernal/terminal/app.gointernal/terminal/async_events.gointernal/terminal/autocomplete.gointernal/terminal/commands.gointernal/terminal/compact_commands.gointernal/terminal/input.gointernal/terminal/message_layout.gointernal/terminal/panel.gointernal/terminal/panel_actions.gointernal/terminal/prompt_queue.gointernal/terminal/prompt_response.gointernal/terminal/prompt_send.gointernal/terminal/prompt_send_internal_test.gointernal/terminal/render.gointernal/terminal/render_composer.gointernal/terminal/running_tools_internal_test.gointernal/tool/registry.gointernal/tool/registry_test.go
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/librecode/prompt_internal_test.go`:
- Around line 162-180: Make TestRunPromptBuildsRuntimeRequest deterministic by
injecting controlled dependencies into runPrompt instead of accepting arbitrary
success or failure. Assert the constructed runtime request fields or a specific
expected user-visible output/error, and remove the generic non-empty error
assertion so failures before request construction cannot pass.
In `@internal/agent/catalog_behavior_test.go`:
- Around line 85-87: Update the defensive-copy test around catalog.Diagnostics()
to first ensure at least one diagnostic exists, then mutate an element in the
returned diagnostics slice rather than appending to it. Call
catalog.Diagnostics() again and assert the original diagnostic remains
unchanged, using the existing diagnostic setup and assertion style.
In `@internal/agenttask/service.go`:
- Around line 593-598: Update the cancellation-state read in the task execution
flow before service.runner.Run: treat any Get error or missing task (found ==
false) as a failure, return a contextual error without invoking the runner, and
preserve cleanup of the heartbeat and active-task entry on this early exit. Keep
the existing cancel() behavior for TaskCanceling unchanged.
- Around line 241-242: Update the service.done branch in the task submission
flow to durably transition the already-persisted queued task to a rejected or
failed terminal state before returning the service_stopped error. Ensure the
transition is persisted successfully and preserve the existing error return,
preventing restart recovery or caller retries from executing the rejected
submission.
In `@internal/database/task_repository.go`:
- Around line 161-181: Define shared constants for the repeated "event.kind"
validation field and "read affected task rows" error message, then update the
relevant validation and rows-affected error paths in the task repository to
reuse them. Replace all occurrences consistently without changing behavior.
- Around line 292-303: Enforce the lease fence for all terminal task
transitions. Update Transition to reject terminal targets unless the row is
unleased or the caller owns the active lease, and prevent its SQL update from
clearing another worker’s lease. In finishTransaction, make an empty finish
owner match only NULL lease owners rather than authorizing every row, while
preserving valid owner matching.
- Around line 348-350: Update TaskRepository.Finish to validate that finish is
non-nil before accessing finish.From or finish.TargetState, returning the
existing validation error for nil input. Preserve the current validation
behavior for non-nil TaskFinish values, matching the guard style used by
ClaimQueued and RecoverExpired.
In `@internal/terminal/agent_tasks_behavior_internal_test.go`:
- Line 280: Update the agent task tracking test cases around
trackStartedAgentTask to pass valid JSON payloads, quoting the behaviorRunning
task ID as a JSON string. Apply the same correction to both occurrences so the
tests exercise explicit task-ID tracking instead of the discovery fallback.
In `@internal/terminal/panel_session.go`:
- Around line 53-54: Move the app.resetAgentTaskTracking() and
app.agentTaskSessionStack = nil operations to after both loadSessionSettings and
loadInitialMessages succeed in the session-loading flow. Preserve the existing
task tracking and session stack when either load fails, without changing
successful session initialization behavior.
In `@Taskfile.yml`:
- Around line 96-103: Update the lint-suppressions-check command in Taskfile.yml
to scan every tracked Go source file rather than only cmd and internal, while
preserving the existing nolint pattern and failure behavior. Use repository-wide
file selection so root-level and other Go packages are included without scanning
untracked files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e6a8148-d69c-45c4-b973-dead51ae72e6
📒 Files selected for processing (51)
Taskfile.ymlcmd/librecode/prompt_internal_test.gointernal/agent/catalog.gointernal/agent/catalog_behavior_test.gointernal/agent/catalog_testmain_test.gointernal/agenttask/catalog_testmain_test.gointernal/agenttask/runtime_runner.gointernal/agenttask/runtime_runner_internal_test.gointernal/agenttask/service.gointernal/agenttask/service_internal_test.gointernal/agenttask/service_test.gointernal/assistant/agent_runtime_wrappers_internal_test.gointernal/assistant/agent_tool.gointernal/assistant/agent_tool_internal_test.gointernal/assistant/catalog_testmain_test.gointernal/assistant/runtime.gointernal/assistant/runtime_context.gointernal/assistant/tool_registry.gointernal/database/agent_task_repository.gointernal/database/agent_task_repository_test.gointernal/database/migrations/00006_create_tasks_events.sqlinternal/database/migrations/00007_add_task_worker_leases.sqlinternal/database/repository_helpers_internal_test.gointernal/database/session_repository_test.gointernal/database/session_task_behavior_test.gointernal/database/task_lease_test.gointernal/database/task_repository.gointernal/database/task_repository_test.gointernal/database/task_test_helpers_test.gointernal/database/task_validation_internal_test.gointernal/di/agent_task_service.gointernal/di/assistant_service_internal_test.gointernal/di/database_service_internal_test.gointernal/di/service_constructors_internal_test.gointernal/extension/lifecycle_internal_test.gointernal/extension/lua_values_internal_test.gointernal/terminal/agent_tasks.gointernal/terminal/agent_tasks_behavior_internal_test.gointernal/terminal/agent_tasks_live_internal_test.gointernal/terminal/app.gointernal/terminal/async_events.gointernal/terminal/catalog_testmain_test.gointernal/terminal/commands.gointernal/terminal/panel_session.gointernal/terminal/running_tools_internal_test.gointernal/terminal/tool_blocks.gointernal/terminal/tool_display.gointernal/tool/file_tools_internal_test.gointernal/tool/input_validation_internal_test.gointernal/tool/registry_test.gointernal/tool/search_tools_internal_test.go
🚧 Files skipped from review as they are similar to previous changes (16)
- internal/database/migrations/00006_create_tasks_events.sql
- internal/di/agent_task_service.go
- internal/terminal/agent_tasks_live_internal_test.go
- internal/terminal/commands.go
- internal/assistant/runtime_context.go
- internal/tool/registry_test.go
- internal/agenttask/runtime_runner.go
- internal/assistant/tool_registry.go
- internal/agent/catalog.go
- internal/terminal/app.go
- internal/assistant/agent_tool.go
- internal/terminal/async_events.go
- internal/di/assistant_service_internal_test.go
- internal/assistant/runtime.go
- internal/terminal/agent_tasks.go
- internal/terminal/running_tools_internal_test.go
Comment on lines
+593
to
+598
| current, found, err := service.tasks.Get(context.WithoutCancel(ctx), taskID) | ||
| if err == nil && found && current.State == database.TaskCanceling { | ||
| cancel() | ||
| } | ||
|
|
||
| result, runErr := service.runner.Run(runCtx, task, service.eventSink(taskID)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Abort execution when the cancellation-state read fails.
Line 593’s error and !found results are ignored. If cancellation raced with active-run registration and this read fails, Runner.Run can still start model or tool work for a canceled task. Return a contextual error before invoking the runner, while ensuring the heartbeat and active entry are cleaned up.
As per coding guidelines, “Never ignore errors; errcheck with check-blank: true is enabled and must be enforced.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/agenttask/service.go` around lines 593 - 598, Update the
cancellation-state read in the task execution flow before service.runner.Run:
treat any Get error or missing task (found == false) as a failure, return a
contextual error without invoking the runner, and preserve cleanup of the
heartbeat and active-task entry on this early exit. Keep the existing cancel()
behavior for TaskCanceling unchanged.
Source: Coding guidelines
Comment on lines
+292
to
+303
| const update = ` | ||
| UPDATE tasks | ||
| SET state = ?, started_at = ?, finished_at = ?, updated_at = ?, | ||
| lease_owner = CASE WHEN ? THEN NULL ELSE lease_owner END, | ||
| lease_expires_at = CASE WHEN ? THEN NULL ELSE lease_expires_at END | ||
| WHERE id = ? AND state = ?` | ||
|
|
||
| terminal := isTerminalTaskState(targetState) | ||
|
|
||
| result, err := transaction.Exec( | ||
| ctx, update, targetState, nullableTime(startedAt), nullableTime(finishedAt), | ||
| formatTime(now), terminal, terminal, taskID, current.State, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep every terminal transition behind the lease fence.
transition clears an active lease without checking ownership, while finishTransaction treats an empty LeaseOwner as unconditional authorization. Either path can terminate work currently owned by another worker.
Reject terminal targets through Transition (or require an unleased row), and make an empty finish owner match only NULL leases:
Proposed finish predicate
-WHERE id = ? AND state = ? AND (? = '' OR lease_owner = ?)`
+WHERE id = ? AND state = ?
+ AND ((? = '' AND lease_owner IS NULL) OR lease_owner = ?)`Also applies to: 386-395
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/database/task_repository.go` around lines 292 - 303, Enforce the
lease fence for all terminal task transitions. Update Transition to reject
terminal targets unless the row is unleased or the caller owns the active lease,
and prevent its SQL update from clearing another worker’s lease. In
finishTransaction, make an empty finish owner match only NULL lease owners
rather than authorizing every row, while preserving valid owner matching.
Comment on lines
+348
to
+350
| func (repository *TaskRepository) Finish(ctx context.Context, finish *TaskFinish) (bool, error) { | ||
| if len(finish.From) == 0 || !isTerminalTaskState(finish.TargetState) { | ||
| return false, errors.New("database: task finish requires source states and a terminal target") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard a nil TaskFinish before dereferencing it.
Unlike ClaimQueued and RecoverExpired, Finish(nil) panics instead of returning a validation error.
Proposed fix
func (repository *TaskRepository) Finish(ctx context.Context, finish *TaskFinish) (bool, error) {
- if len(finish.From) == 0 || !isTerminalTaskState(finish.TargetState) {
+ if finish == nil || len(finish.From) == 0 || !isTerminalTaskState(finish.TargetState) {
return false, errors.New("database: task finish requires source states and a terminal target")
}📝 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
| func (repository *TaskRepository) Finish(ctx context.Context, finish *TaskFinish) (bool, error) { | |
| if len(finish.From) == 0 || !isTerminalTaskState(finish.TargetState) { | |
| return false, errors.New("database: task finish requires source states and a terminal target") | |
| func (repository *TaskRepository) Finish(ctx context.Context, finish *TaskFinish) (bool, error) { | |
| if finish == nil || len(finish.From) == 0 || !isTerminalTaskState(finish.TargetState) { | |
| return false, errors.New("database: task finish requires source states and a terminal target") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/database/task_repository.go` around lines 348 - 350, Update
TaskRepository.Finish to validate that finish is non-nil before accessing
finish.From or finish.TargetState, returning the existing validation error for
nil input. Preserve the current validation behavior for non-nil TaskFinish
values, matching the guard style used by ClaimQueued and RecoverExpired.
| assert.Equal(t, behaviorRunning, app.agentTasks[0].Task.ID) | ||
| assert.Contains(t, app.agentTaskWatches, behaviorRunning) | ||
|
|
||
| app.trackStartedAgentTask(t.Context(), agentToolEvent("", `{"task_id":behaviorRunning}`, false)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use valid JSON so these tests exercise task-ID tracking.
{"task_id":behaviorRunning} is invalid JSON. Both cases silently take the discovery fallback and can pass even if explicit task-ID tracking is broken.
Proposed fix
-app.trackStartedAgentTask(t.Context(), agentToolEvent("", `{"task_id":behaviorRunning}`, false))
+app.trackStartedAgentTask(t.Context(), agentToolEvent("", `{"task_id":"running"}`, false))-app.applyAgentToolEvent(agentToolEvent(agentStartToolName, `{"task_id":behaviorRunning}`, false))
+app.applyAgentToolEvent(agentToolEvent(agentStartToolName, `{"task_id":"running"}`, false))Also applies to: 354-354
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/terminal/agent_tasks_behavior_internal_test.go` at line 280, Update
the agent task tracking test cases around trackStartedAgentTask to pass valid
JSON payloads, quoting the behaviorRunning task ID as a JSON string. Apply the
same correction to both occurrences so the tests exercise explicit task-ID
tracking instead of the discovery fallback.
Comment on lines
+96
to
+103
| lint-suppressions-check: | ||
| desc: Reject nolint directives in project Go source | ||
| cmds: | ||
| - | | ||
| if grep -RInE --include='*.go' '//[[:space:]]*nolint([[:space:]:]|$)' cmd internal; then | ||
| echo 'nolint directives are not allowed; fix the underlying lint issue instead' >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Scan every tracked Go source file.
Restricting the check to cmd and internal permits //nolint in root-level or other Go packages, bypassing the repository-wide suppression policy.
Proposed fix
- if grep -RInE --include='*.go' '//[[:space:]]*nolint([[:space:]:]|$)' cmd internal; then
+ if git grep -nE '//[[:space:]]*nolint([[:space:]:]|$)' -- '*.go'; thenAs per coding guidelines, “Never ignore errors; errcheck with check-blank: true is enabled and must be enforced.”
📝 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
| lint-suppressions-check: | |
| desc: Reject nolint directives in project Go source | |
| cmds: | |
| - | | |
| if grep -RInE --include='*.go' '//[[:space:]]*nolint([[:space:]:]|$)' cmd internal; then | |
| echo 'nolint directives are not allowed; fix the underlying lint issue instead' >&2 | |
| exit 1 | |
| fi | |
| lint-suppressions-check: | |
| desc: Reject nolint directives in project Go source | |
| cmds: | |
| - | | |
| if git grep -nE '//[[:space:]]*nolint([[:space:]:]|$)' -- '*.go'; then | |
| echo 'nolint directives are not allowed; fix the underlying lint issue instead' >&2 | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Taskfile.yml` around lines 96 - 103, Update the lint-suppressions-check
command in Taskfile.yml to scan every tracked Go source file rather than only
cmd and internal, while preserving the existing nolint pattern and failure
behavior. Use repository-wide file selection so root-level and other Go packages
are included without scanning untracked files.
Source: Coding guidelines
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/agenttask/service.go (1)
629-634: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStill not aborting execution when the cancellation-state read fails or the task can't be found.
if err == nil && found && current.State == database.TaskCancelingonly handles the "found and canceling" case. On aGeterror or!found, the code falls straight through toservice.runner.Run(...), which is exactly the race this comment (and the block above it) claims to guard against. A transient DB error or a task that vanished mid-race should not silently be treated as "safe to run."As per coding guidelines, "Never ignore errors; errcheck with
check-blank: trueis enabled and must be enforced" — the error is assigned but its failure path is effectively unhandled here.🛡️ Suggested fix
current, found, err := service.tasks.Get(context.WithoutCancel(ctx), taskID) - if err == nil && found && current.State == database.TaskCanceling { - return Result{Text: "", UsageJSON: ""}, context.Canceled - } + if err != nil { + return Result{Text: "", UsageJSON: ""}, oops.In("agenttask").Code("execute_task").Wrapf(err, "read task state before execution") + } + if !found { + return Result{Text: "", UsageJSON: ""}, oops.In("agenttask").Code("execute_task").Errorf("task %s not found before execution", taskID) + } + if current.State == database.TaskCanceling { + return Result{Text: "", UsageJSON: ""}, context.Canceled + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agenttask/service.go` around lines 629 - 634, Update the post-registration state check in the task execution flow around service.tasks.Get so any read error or missing task aborts before service.runner.Run, rather than falling through as safe to execute. Preserve the existing cancellation response for TaskCanceling, and return an appropriate error for Get failures or !found cases so the Get result is fully handled.Source: Coding guidelines
🧹 Nitpick comments (2)
internal/agenttask/service.go (1)
238-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStill ignores the
service_stopped/queue-full durability gap? No — this now durably rejects on every early-return path.Confirmed
enqueueCreatedcallsservice.rejectQueuedTask(...)in all four early-return branches (ctx canceled, service stopped x2, ctx.Done during select, queue full) before returning the error — this resolves the previously flagged concern about persisted-but-unrejected tasks surviving a restart.Separately, static analysis flags the
"enqueue task"string literal duplicated 4 times (lines 250, 259, 271, 277);"task service stopped before queue admission"is also duplicated (lines 256, 268). Consider extracting a small helper to reduce duplication.♻️ Suggested refactor
+func (service *Service) enqueueRejectErr(code, message string, cause error) error { + return oops.In("agenttask").Code(code).Wrapf(cause, "enqueue task") +} + func (service *Service) enqueueCreated( ctx context.Context, created *database.AgentTaskEntity, ) (*database.AgentTaskEntity, error) { if err := ctx.Err(); err != nil { service.rejectQueuedTask( created.Task.ID, "enqueue_canceled", "task submission was canceled before queue admission", ) - - return created, oops.In("agenttask").Code("enqueue_canceled").Wrapf(err, "enqueue task") + return created, service.enqueueRejectErr("enqueue_canceled", "task submission was canceled before queue admission", err) } ...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agenttask/service.go` around lines 238 - 283, Refactor enqueueCreated to remove duplicated "enqueue task" and "task service stopped before queue admission" literals, preferably by extracting shared constants or a small local helper. Preserve all existing rejection calls, error codes, wrapping behavior, and return paths unchanged.Source: Linters/SAST tools
internal/agenttask/service_internal_test.go (1)
323-452: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract shared SQLite/session fixture setup.
TestServiceInternalSubmitRejectsBeforeWritableQueueandTestServiceInternalExecuteSkipsDurablyCancelingTaskeach repeat the samesql.Open/Migrate/owner+child session/emptyAgentTaskboilerplate already used elsewhere in this file. A shared fixture helper (similar toserviceWithRepositories) would reduce duplication, though this is purely a maintainability nice-to-have and consistent with existing style in the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agenttask/service_internal_test.go` around lines 323 - 452, Optionally extract the repeated SQLite, migration, owner/child session, and base agent-task setup from TestServiceInternalSubmitRejectsBeforeWritableQueue and TestServiceInternalExecuteSkipsDurablyCancelingTask into a shared fixture helper, following the existing serviceWithRepositories pattern. Update both tests to use the helper while preserving their test-specific state and cleanup behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@internal/agenttask/service.go`:
- Around line 629-634: Update the post-registration state check in the task
execution flow around service.tasks.Get so any read error or missing task aborts
before service.runner.Run, rather than falling through as safe to execute.
Preserve the existing cancellation response for TaskCanceling, and return an
appropriate error for Get failures or !found cases so the Get result is fully
handled.
---
Nitpick comments:
In `@internal/agenttask/service_internal_test.go`:
- Around line 323-452: Optionally extract the repeated SQLite, migration,
owner/child session, and base agent-task setup from
TestServiceInternalSubmitRejectsBeforeWritableQueue and
TestServiceInternalExecuteSkipsDurablyCancelingTask into a shared fixture
helper, following the existing serviceWithRepositories pattern. Update both
tests to use the helper while preserving their test-specific state and cleanup
behavior.
In `@internal/agenttask/service.go`:
- Around line 238-283: Refactor enqueueCreated to remove duplicated "enqueue
task" and "task service stopped before queue admission" literals, preferably by
extracting shared constants or a small local helper. Preserve all existing
rejection calls, error codes, wrapping behavior, and return paths unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 467f6529-4a7a-454d-8240-5935c8a41ef4
📒 Files selected for processing (17)
.github/workflows/bench.ymlcmd/librecode/prompt.gocmd/librecode/prompt_internal_test.gointernal/agent/catalog_behavior_test.gointernal/agenttask/service.gointernal/agenttask/service_internal_test.gointernal/database/agent_task_repository_test.gointernal/database/session_task_behavior_test.gointernal/database/task_lease_test.gointernal/database/task_repository.gointernal/database/task_repository_test.gointernal/database/task_test_helpers_test.gointernal/terminal/app.gointernal/terminal/panel_session.gointernal/terminal/panel_session_internal_test.gointernal/terminal/panel_session_selection_internal_test.gointernal/terminal/session_settings.go
💤 Files with no reviewable changes (1)
- .github/workflows/bench.yml
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/database/session_task_behavior_test.go
- internal/database/task_test_helpers_test.go
- internal/database/agent_task_repository_test.go
- internal/database/task_lease_test.go
- internal/database/task_repository_test.go
- internal/agent/catalog_behavior_test.go
- internal/database/task_repository.go
- internal/terminal/app.go
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.
No description provided.