feat(tool): add durable background tool execution - #260
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change adds durable background tool tasks and detachable foreground tools. It adds task persistence, bounded scheduling, lease recovery, ownership controls, workspace coordination, assistant and terminal controls, provider dispatch validation, and dependency-injection wiring. ChangesDurable tool task execution
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Assistant
participant ToolTaskService
participant TaskRuntime
participant Tool
participant Database
Assistant->>ToolTaskService: start durable tool task
ToolTaskService->>Database: persist invocation and queued event
ToolTaskService->>TaskRuntime: notify queued task
TaskRuntime->>Tool: claim and execute task
Tool-->>TaskRuntime: return result or error
TaskRuntime->>Database: settle terminal outcome
Assistant->>ToolTaskService: retrieve, wait, or cancel task
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #260 +/- ##
==========================================
- Coverage 84.49% 84.29% -0.21%
==========================================
Files 323 333 +10
Lines 31487 33486 +1999
==========================================
+ Hits 26606 28228 +1622
- Misses 3319 3593 +274
- Partials 1562 1665 +103
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: 11
🧹 Nitpick comments (19)
internal/assistant/task_tool.go (2)
114-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant tool-name case in
Execute.Lines 122-124 list ordinary tool names and return the same error as the fallback at line 127. The branch adds maintenance cost because every new built-in tool name must be added here. Use only the default fallback.
♻️ Proposed simplification
case taskListToolName: return executor.list(ctx, input) - case tool.NameRead, tool.NameBash, tool.NameEdit, tool.NameWrite, - tool.NameGrep, tool.NameFind, tool.NameLS, tool.NameAST, tool.NameFetch: - return tool.Result{}, errors.New("unknown task management tool") }🤖 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/task_tool.go` around lines 114 - 128, Remove the explicit ordinary-tool-name case from taskToolExecutor.Execute, leaving the task-specific cases and the existing default unknown-task-management-tool error fallback.
149-160: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueReject non-positive
timeout_secondsexplicitly.The upper-bound guard is present, but a negative
TimeoutSecondsproduces a negativeTimeout.tooltask.Service.Startthen substitutes the default timeout, so the caller receives a silently different timeout than requested. The JSON schema declares"minimum":1, so this only occurs if schema validation is bypassed. Add an explicit check for defense in depth.🛡️ Proposed guard
+ if args.TimeoutSeconds < 0 { + return tool.Result{}, errors.New("timeout_seconds must be positive") + } + maxTimeoutSeconds := int64((time.Duration(1<<63 - 1)) / time.Second)🤖 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/task_tool.go` around lines 149 - 160, Update the timeout validation around maxTimeoutSeconds to reject args.TimeoutSeconds values less than 1 before constructing tooltask.StartRequest, returning the existing invalid-input error style. Preserve the upper-bound check and only allow positive timeout values to reach the Timeout conversion.internal/assistant/tool_detach.go (1)
75-89: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider an in-process fallback when durable start fails.
Lines 75-76 release the prepared invocation before
toolTasks.Startruns. IfStartreturns an error, the tool call fails even though the prepared in-process execution was available. A transient durable-store error therefore converts an ordinary read into a tool error.An alternative is to keep the prepared invocation until
Startsucceeds and fall back toexecutePreparedToolCallon failure. Confirm the current fail-fast behavior is intentional.🤖 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_detach.go` around lines 75 - 89, The detach flow around prepared.invocation.Release and runtime.toolTasks.Start currently fails the tool when durable startup errors; preserve the prepared invocation through the start attempt and, on Start failure, invoke executePreparedToolCall using it as the in-process fallback. Release the invocation only after durable start succeeds or fallback execution has completed, while preserving the existing error behavior for fallback failures.internal/assistant/tool_executor.go (1)
18-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the variadic
attachmentparameter with explicit parameters.
executeProviderToolCallsacceptsattachment ...stringand only reads it whenlen(attachment) > 1. A caller that passes a single value gets bothownerandcwdsilently discarded, and the compiler cannot detect the mistake. Both current callers pass exactly two values, so explicit parameters cost nothing and make the contract checkable.♻️ Proposed signature change
func (runtime *Runtime) executeProviderToolCalls( registry *tool.Registry, - attachment ...string, + owner, cwd string, ) ToolExecutor { - owner, cwd := "", "" - if len(attachment) > 1 { - owner, cwd = attachment[0], attachment[1] - } -Update any caller that omits the values to pass
"", "".🤖 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_executor.go` around lines 18 - 26, Change Runtime.executeProviderToolCalls to accept explicit owner and cwd string parameters instead of the variadic attachment slice, removing the length check and direct parameter assignment. Update every caller to provide both values, passing empty strings where either value is unavailable.internal/assistant/testing.go (1)
62-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winForward the new runtime dependencies through the test options.
NewRuntimeForTestalways passes nil forToolTasks,GenericTasks, andToolCoordinator. This prevents callers from using the common helper to test the new task orchestration paths.Add matching fields to
RuntimeTestOptionsand pass those values through. Keep nil defaults for existing callers.🤖 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/testing.go` around lines 62 - 64, Add ToolTasks, GenericTasks, and ToolCoordinator fields to RuntimeTestOptions, then update NewRuntimeForTest to forward those option values into the runtime configuration. Preserve nil defaults so existing callers remain unchanged.internal/di/assistant_service_internal_test.go (1)
94-95: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse the production repository graph in this fixture.
DatabaseService.Taskscomes fromworkflows.Tasks(), butNewToolTaskRepository(connection)creates a separate provider andTaskRepository. BuildToolTaskswith the same provider andTaskRepositoryso tests cover the shared graph and clock contract.🤖 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/di/assistant_service_internal_test.go` around lines 94 - 95, Update the fixture around DatabaseService.Tasks to build ToolTasks from the same workflows.Tasks() provider and TaskRepository used by the production repository graph, rather than calling NewToolTaskRepository(connection). Preserve the existing error assertion while ensuring both services share the production graph and clock contract.internal/taskruntime/service_internal_test.go (2)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExplain the blank import.
SonarCloud reports this blank import on the pull request. Add a short comment that states the import registers the
sqlitedriver forsql.Open.♻️ Proposed change
- _ "modernc.org/sqlite" + // Registers the "sqlite" driver used by sql.Open in these tests. + _ "modernc.org/sqlite"🤖 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/taskruntime/service_internal_test.go` at line 13, Add a concise comment immediately above the blank sqlite import explaining that it registers the sqlite driver for sql.Open, while leaving the import unchanged.Source: Linters/SAST tools
85-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
newRuntimeTestDatabasefor the duplicated SQLite setup.The helper already configures
BusyTimeout,SetMaxOpenConns(1), and migrations. Use it in the three tests with inline SQLite setup.🤖 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/taskruntime/service_internal_test.go` around lines 85 - 93, Replace the duplicated SQLite initialization in the affected tests with the existing newRuntimeTestDatabase helper. Remove the inline SQLiteOptions, sql.Open, connection limit, ConfigureSQLite, and Migrate calls while preserving each test’s database connection and cleanup behavior.internal/database/tool_task_repository_test.go (1)
197-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd table-driven coverage for
validateToolTask.The tests exercise the happy paths and the lease and cancellation races. No test covers the rejection rules: empty
TargetName,CWD,OwnerSessionID,InvocationID, orWrapperCallID; a non-positiveTimeoutSeconds;ArgumentsJSONabove 256 KiB; and a non-objectPolicyJSONorDefinitionJSON. A table-driven test overnewToolTaskmutations covers all of them in one function.As per coding guidelines: "Prefer table-driven tests for core behavior and regression tests for terminal rendering bugs."
🤖 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/tool_task_repository_test.go` around lines 197 - 204, Add a table-driven test for validateToolTask using newToolTask as the baseline, with cases mutating each rejected field: empty TargetName, CWD, OwnerSessionID, InvocationID, and WrapperCallID; non-positive TimeoutSeconds; ArgumentsJSON exceeding 256 KiB; and non-object PolicyJSON or DefinitionJSON. Assert each case is rejected while retaining the existing valid fixture behavior.Source: Coding guidelines
internal/database/task_repository.go (2)
499-513: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTerminate the recovery loop on the selected row count, not the recovered count.
recoverExpiredBatchreturns only the IDs that transitioned. If the batch query selects 100 expired tasks and one of them fails to transition, the returned slice has 99 entries and the loop stops. The remaining expired tasks stay running until the next recovery interval.ToolTaskRepository.RecoverExpiredininternal/database/tool_task_repository.goalready paginates on the processed row count, so the two recovery paths behave differently.Return the number of selected rows alongside the recovered IDs and compare that value against
recoveryBatchSize.♻️ Proposed change
for { - batch, err := repository.recoverExpiredBatch(ctx, recovery, recoveryBatchSize) + batch, selected, err := repository.recoverExpiredBatch(ctx, recovery, recoveryBatchSize) if err != nil { return nil, oops.In("database").Code("recover_tasks").Wrapf(err, "recover expired tasks") } recovered = append(recovered, batch...) - if len(batch) < recoveryBatchSize { + if selected < recoveryBatchSize { return recovered, nil } } }Update
recoverExpiredBatchto also returnlen(rows).🤖 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 499 - 513, Update recoverExpiredBatch to return both the transitioned IDs and the number of selected rows, using len(rows) for the latter. In the recovery loop of RecoverExpired, append the returned IDs as before but use the selected-row count—not len(batch)—to decide when to terminate, continuing whenever a full recoveryBatchSize was selected.
693-722: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap the scan error from
collectSQLRows.Line 720 returns the
collectSQLRowserror unwrapped.ListOwnedandListByStateswrap the same error withoops.In("database").Code("scan_task"). Keep the error contract consistent across the repository.♻️ Proposed change
- return collectSQLRows(rows, taskFromRow) + tasks, err := collectSQLRows(rows, taskFromRow) + if err != nil { + return nil, oops.In("database").Code("scan_task").Wrapf(err, "scan task") + } + + return tasks, nil }As per coding guidelines: "Use
oops.In("domain").Code("code").Wrapf(err, "message")for contextual errors where the package already usessamber/oops."🤖 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 693 - 722, Update ListQueuedExcluding to wrap errors returned by collectSQLRows with oops.In("database").Code("scan_task").Wrapf(err, "scan task"), matching the error handling used by ListOwned and ListByStates; preserve the existing successful return behavior.Source: Coding guidelines
internal/database/tool_task_repository.go (1)
356-367: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated validation label.
The literal
"tool_task.owner_session_id"appears at Lines 362, 379, and 413. SonarCloud reports it on the pull request. Define a package-level constant and use it at the three call sites.♻️ Proposed change
const ( toolTaskPersistenceTimeout = 2 * time.Second // TaskKindTool identifies a durable background tool invocation. TaskKindTool = "tool" maxToolTaskArgumentsBytes = 256 * 1024 + toolTaskOwnerField = "tool_task.owner_session_id" )Replace the three literal occurrences with
toolTaskOwnerField.🤖 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/tool_task_repository.go` around lines 356 - 367, Define a package-level constant named toolTaskOwnerField for the "tool_task.owner_session_id" validation label, then replace all three matching literals in the ToolTaskRepository validation call sites, including GetByInvocation and the other occurrences near the referenced lines.Source: Linters/SAST tools
internal/tool/bash_ingestion_internal_test.go (1)
12-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a table-driven test for buffer capacity behavior.
The two tests differ only by input size and expected truncation state. Combine them into table cases. Keep the retained-output assertion in the overflow case.
As per coding guidelines, prefer table-driven tests for core behavior.
🤖 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/tool/bash_ingestion_internal_test.go` around lines 12 - 47, Combine TestSynchronizedBufferBoundsOutputAtIngestion and TestSynchronizedBufferExactCapacityIsNotTruncated into one table-driven test covering exact-capacity and overflow inputs. Keep each case’s expected captured output, total, and truncated values, including the retained-output assertion for the overflow case, and run the existing synchronizedBuffer write/snapshot assertions for every case.Source: Coding guidelines
internal/tooltask/service_internal_test.go (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the SQLite driver blank imports. SonarCloud flags both blank imports of
modernc.org/sqliteas undocumented. Add a short trailing comment at each site that states the driver registration purpose.
internal/tooltask/service_internal_test.go#L16-L16: append// Register the SQLite driver used by sql.Open.to the blank import.internal/taskruntime/dispatch_benchmark_test.go#L10-L10: append the same comment to the blank import.🤖 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/tooltask/service_internal_test.go` at line 16, The blank SQLite driver imports need documentation for their registration purpose. In internal/tooltask/service_internal_test.go lines 16-16 and internal/taskruntime/dispatch_benchmark_test.go lines 10-10, append the specified trailing comment to each modernc.org/sqlite blank import, stating that it registers the SQLite driver used by sql.Open.Source: Linters/SAST tools
internal/tool/bash_windows.go (1)
80-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExplain the empty function.
SonarCloud fails the build on this empty function.
configureShellCommandis the Windows no-op counterpart of the Unix implementation. State that in a nested comment.♻️ Proposed fix
-func configureShellCommand(_ *exec.Cmd) {} +func configureShellCommand(_ *exec.Cmd) { + // Windows has no process-group attribute to set; taskkill /T terminates the tree. +}🤖 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/tool/bash_windows.go` at line 80, Add a nested explanatory comment inside the Windows no-op configureShellCommand function, stating that it intentionally does nothing as the Windows counterpart to the Unix shell-command configuration implementation. Keep the function behavior unchanged.Source: Linters/SAST tools
internal/taskruntime/manager_internal_test.go (1)
74-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the SQLite driver in this file.
newRuntimeTestDatabasecallssql.Open("sqlite", ...), but this file does not importmodernc.org/sqlite. The driver is registered only becauseinternal/taskruntime/dispatch_benchmark_test.goblank-imports it, and both test packages link into the same test binary. If that benchmark file is removed or moved, this test fails at run time with an unknown-driver error.♻️ Proposed fix
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + _ "modernc.org/sqlite" // Register the SQLite driver used by sql.Open. "github.com/omarluq/librecode/internal/database"🤖 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/taskruntime/manager_internal_test.go` around lines 74 - 86, Add a blank import for modernc.org/sqlite in internal/taskruntime/manager_internal_test.go so the sqlite driver is registered within the file’s own test package. Keep newRuntimeTestDatabase unchanged and ensure sql.Open("sqlite", ...) works independently of dispatch_benchmark_test.go.internal/taskruntime/service.go (2)
365-407: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider removing the duplicate task load.
runClaimedcallsTasks.Getat Line 366 and again at Line 391 for the same task ID. The second call only re-reads the state to detectTaskCanceling. Both calls run after the claim, with only local setup between them. One load with a cancel-state check would remove a database round trip per task start.🤖 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/taskruntime/service.go` around lines 365 - 407, Remove the second Tasks.Get call in runClaimed and reuse the task loaded at the start, checking its State for TaskCanceling before safeRun. Preserve the existing handling for load errors, missing tasks, cancellation, and normal execution without adding another database read.
207-212: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider running startup recovery outside
service.mu.
Startholdsservice.mufor its whole body.recover()performs database I/O for every handler kind. That blocks any concurrentCancelActive,finishWithHandler, andeventSinkcaller for the duration of recovery. Startup contention is low today, so this is optional. Releasing the lock afterstarted = trueand then callingrecover()keeps the same ordering guarantee.🤖 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/taskruntime/service.go` around lines 207 - 212, Update Start so it marks the service started while holding service.mu, then releases the mutex before invoking service.recover(). Preserve the existing ordering by completing recovery before launching service.poll() and calling service.Notify(), while allowing CancelActive, finishWithHandler, and eventSink to proceed during database recovery.internal/tooltask/service.go (1)
544-548: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the discarded completion-hook error.
applyCompletionHookdrops the error fromservice.completionHookand returns the original result. Assistant lifecycle failures then leave no trace. Keeping the tool result is the correct control flow, but the error should still reach a log or a task event.Servicehas no logger today, so add one or surface the failure through the returnedtaskruntime.Outcome.🤖 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/tooltask/service.go` around lines 544 - 548, Update applyCompletionHook around service.completionHook so a non-nil completion-hook error is not discarded: preserve returning the original completion result, but record the error through an available logger or surface it via the returned taskruntime.Outcome. Add the necessary Service logging dependency if choosing the logger path, and ensure successful hooks retain the existing 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.
Inline comments:
In `@internal/assistant/tool_detach_internal_test.go`:
- Around line 65-74: Update detachableTaskController.Wait to avoid mutating the
shared controller.entity: create and return a copy with Task.State set to
database.TaskSucceeded when controller.done closes. Preserve the existing
context-cancellation error path and ensure the shared entity remains unchanged
for concurrent DetachForegroundTool access.
In `@internal/assistant/tool_detach.go`:
- Around line 107-125: Set prepared.lifecycleCompleted only in the
attachmentCompleted and attachmentDetached cases, where the durable worker
already applies the result lifecycle; leave it false for attachmentCanceled and
attachmentUnresolved so finalizePreparedToolCall invokes
dispatchToolResultLifecycle for those paths.
In `@internal/database/task_repository.go`:
- Around line 499-513: The duplicated recovery loops use incorrect termination
counters. In internal/database/task_repository.go:499-513, update
recoverExpiredBatch and its caller to return and compare the selected row count
against recoveryBatchSize rather than len(recovered). In
internal/database/tool_task_repository.go:244-281, track transitioned rows
separately from processed rows in the recoverExpiredTransaction closure and stop
when the transitioned count is zero or processed is below batchSize; consider
sharing a helper that returns both counts.
- Around line 436-445: Update the AgentTaskRepository.Finish handling in the
service flow around recoverInterrupted so a changed == false result is not
silently discarded. Retry or invoke the existing task-recovery path for the
affected task, preserving the original finish result and ensuring expired-lease
tasks are recovered instead of remaining in TaskRunning.
In `@internal/database/tool_task_repository.go`:
- Around line 179-213: Update finishTransaction so non-cancel settlements
preserve the caller-provided finish.From states instead of always replacing them
with []TaskState{TaskRunning}. Keep the cancellation-race override to
TaskCanceling unchanged, and ensure valid callers such as TaskQueued can
transition and persist their outcome.
In `@internal/di/assistant_service_internal_test.go`:
- Around line 66-73: Update the conditional around NewTaskRuntimeService to
check both databaseService.Tasks and databaseService.ToolTasks for nil. Provide
the unavailable TaskRuntimeService fixture when either repository is missing;
only construct the runtime and require a successful result when both
repositories are available.
In `@internal/provider/openai_chat.go`:
- Around line 71-73: Update openAIChatFinishReason to return
llm.FinishReasonLength when the provider finish reason is "length", before
applying the hasToolCalls fallback to llm.FinishReasonToolCalls. Add a
chat-stream regression case covering a length-terminated response with parsed
tool calls and verify partial tool arguments are rejected by
validateToolDispatch.
In `@internal/taskruntime/service.go`:
- Around line 582-584: Update the settlement logging condition around the
changed result so a false changed value with a nil err still records a
diagnostic event; preserve logging of non-nil errors and ensure the message does
not depend on service.log ignoring nil errors.
In `@internal/terminal/tool_tasks.go`:
- Around line 25-28: Update refreshToolTasks to return the error from
app.runtime.ToolTasks instead of discarding it, while preserving successful task
updates. Change listToolTasks to propagate the refreshToolTasks error to /tasks,
and update periodic callers to handle the returned error separately so no call
site ignores it.
In `@internal/tool/bash_windows.go`:
- Line 92: Update the taskkill invocation in the killTree command construction
to use an absolute executable path derived from the SystemRoot environment value
and the Windows system directory, rather than resolving taskkill through PATH.
Adjust imports and PID string conversion as needed while preserving the existing
CommandContext arguments and behavior.
In `@internal/tooltask/service.go`:
- Around line 468-469: Before creating the timeout context in the task execution
flow, validate persisted.TimeoutSeconds and use service.defaultTimeout when it
is not positive. Ensure the resulting timeout remains positive before passing it
to context.WithTimeout, while preserving persisted.TimeoutSeconds for valid
positive values.
---
Nitpick comments:
In `@internal/assistant/task_tool.go`:
- Around line 114-128: Remove the explicit ordinary-tool-name case from
taskToolExecutor.Execute, leaving the task-specific cases and the existing
default unknown-task-management-tool error fallback.
- Around line 149-160: Update the timeout validation around maxTimeoutSeconds to
reject args.TimeoutSeconds values less than 1 before constructing
tooltask.StartRequest, returning the existing invalid-input error style.
Preserve the upper-bound check and only allow positive timeout values to reach
the Timeout conversion.
In `@internal/assistant/testing.go`:
- Around line 62-64: Add ToolTasks, GenericTasks, and ToolCoordinator fields to
RuntimeTestOptions, then update NewRuntimeForTest to forward those option values
into the runtime configuration. Preserve nil defaults so existing callers remain
unchanged.
In `@internal/assistant/tool_detach.go`:
- Around line 75-89: The detach flow around prepared.invocation.Release and
runtime.toolTasks.Start currently fails the tool when durable startup errors;
preserve the prepared invocation through the start attempt and, on Start
failure, invoke executePreparedToolCall using it as the in-process fallback.
Release the invocation only after durable start succeeds or fallback execution
has completed, while preserving the existing error behavior for fallback
failures.
In `@internal/assistant/tool_executor.go`:
- Around line 18-26: Change Runtime.executeProviderToolCalls to accept explicit
owner and cwd string parameters instead of the variadic attachment slice,
removing the length check and direct parameter assignment. Update every caller
to provide both values, passing empty strings where either value is unavailable.
In `@internal/database/task_repository.go`:
- Around line 499-513: Update recoverExpiredBatch to return both the
transitioned IDs and the number of selected rows, using len(rows) for the
latter. In the recovery loop of RecoverExpired, append the returned IDs as
before but use the selected-row count—not len(batch)—to decide when to
terminate, continuing whenever a full recoveryBatchSize was selected.
- Around line 693-722: Update ListQueuedExcluding to wrap errors returned by
collectSQLRows with oops.In("database").Code("scan_task").Wrapf(err, "scan
task"), matching the error handling used by ListOwned and ListByStates; preserve
the existing successful return behavior.
In `@internal/database/tool_task_repository_test.go`:
- Around line 197-204: Add a table-driven test for validateToolTask using
newToolTask as the baseline, with cases mutating each rejected field: empty
TargetName, CWD, OwnerSessionID, InvocationID, and WrapperCallID; non-positive
TimeoutSeconds; ArgumentsJSON exceeding 256 KiB; and non-object PolicyJSON or
DefinitionJSON. Assert each case is rejected while retaining the existing valid
fixture behavior.
In `@internal/database/tool_task_repository.go`:
- Around line 356-367: Define a package-level constant named toolTaskOwnerField
for the "tool_task.owner_session_id" validation label, then replace all three
matching literals in the ToolTaskRepository validation call sites, including
GetByInvocation and the other occurrences near the referenced lines.
In `@internal/di/assistant_service_internal_test.go`:
- Around line 94-95: Update the fixture around DatabaseService.Tasks to build
ToolTasks from the same workflows.Tasks() provider and TaskRepository used by
the production repository graph, rather than calling
NewToolTaskRepository(connection). Preserve the existing error assertion while
ensuring both services share the production graph and clock contract.
In `@internal/taskruntime/manager_internal_test.go`:
- Around line 74-86: Add a blank import for modernc.org/sqlite in
internal/taskruntime/manager_internal_test.go so the sqlite driver is registered
within the file’s own test package. Keep newRuntimeTestDatabase unchanged and
ensure sql.Open("sqlite", ...) works independently of
dispatch_benchmark_test.go.
In `@internal/taskruntime/service_internal_test.go`:
- Line 13: Add a concise comment immediately above the blank sqlite import
explaining that it registers the sqlite driver for sql.Open, while leaving the
import unchanged.
- Around line 85-93: Replace the duplicated SQLite initialization in the
affected tests with the existing newRuntimeTestDatabase helper. Remove the
inline SQLiteOptions, sql.Open, connection limit, ConfigureSQLite, and Migrate
calls while preserving each test’s database connection and cleanup behavior.
In `@internal/taskruntime/service.go`:
- Around line 365-407: Remove the second Tasks.Get call in runClaimed and reuse
the task loaded at the start, checking its State for TaskCanceling before
safeRun. Preserve the existing handling for load errors, missing tasks,
cancellation, and normal execution without adding another database read.
- Around line 207-212: Update Start so it marks the service started while
holding service.mu, then releases the mutex before invoking service.recover().
Preserve the existing ordering by completing recovery before launching
service.poll() and calling service.Notify(), while allowing CancelActive,
finishWithHandler, and eventSink to proceed during database recovery.
In `@internal/tool/bash_ingestion_internal_test.go`:
- Around line 12-47: Combine TestSynchronizedBufferBoundsOutputAtIngestion and
TestSynchronizedBufferExactCapacityIsNotTruncated into one table-driven test
covering exact-capacity and overflow inputs. Keep each case’s expected captured
output, total, and truncated values, including the retained-output assertion for
the overflow case, and run the existing synchronizedBuffer write/snapshot
assertions for every case.
In `@internal/tool/bash_windows.go`:
- Line 80: Add a nested explanatory comment inside the Windows no-op
configureShellCommand function, stating that it intentionally does nothing as
the Windows counterpart to the Unix shell-command configuration implementation.
Keep the function behavior unchanged.
In `@internal/tooltask/service_internal_test.go`:
- Line 16: The blank SQLite driver imports need documentation for their
registration purpose. In internal/tooltask/service_internal_test.go lines 16-16
and internal/taskruntime/dispatch_benchmark_test.go lines 10-10, append the
specified trailing comment to each modernc.org/sqlite blank import, stating that
it registers the SQLite driver used by sql.Open.
In `@internal/tooltask/service.go`:
- Around line 544-548: Update applyCompletionHook around service.completionHook
so a non-nil completion-hook error is not discarded: preserve returning the
original completion result, but record the error through an available logger or
surface it via the returned taskruntime.Outcome. Add the necessary Service
logging dependency if choosing the logger path, and ensure successful hooks
retain the existing behavior.
🪄 Autofix
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: 54fea1dc-9f9f-4026-ad7e-2f96e7464c26
📒 Files selected for processing (68)
cmd/librecode/cli_helpers_internal_test.gocmd/librecode/tool_internal_test.gointernal/assistant/agent_tool.gointernal/assistant/background_lifecycle.gointernal/assistant/execute_tool.gointernal/assistant/provider_hook_test_helpers_internal_test.gointernal/assistant/runtime.gointernal/assistant/runtime_model.gointernal/assistant/runtime_test.gointernal/assistant/task_tool.gointernal/assistant/testing.gointernal/assistant/tool_detach.gointernal/assistant/tool_detach_internal_test.gointernal/assistant/tool_executor.gointernal/assistant/tool_executor_internal_test.gointernal/assistant/tool_registry.gointernal/assistant/tool_schema_cache_internal_test.gointernal/config/config.gointernal/config/defaults.gointernal/config/loader.gointernal/database/agent_task_repository_test.gointernal/database/migrations/00015_create_tool_tasks.sqlinternal/database/migrations_test.gointernal/database/task_lease_test.gointernal/database/task_repository.gointernal/database/task_repository_test.gointernal/database/task_test_helpers_test.gointernal/database/tool_task_repository.gointernal/database/tool_task_repository_test.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/di/task_runtime_service.gointernal/di/tool_service.gointernal/executeworker/client.gointernal/provider/anthropic.gointernal/provider/client_internal_test.gointernal/provider/openai_chat.gointernal/provider/openai_responses.gointernal/provider/tool_loop.gointernal/provider/tool_loop_internal_test.gointernal/taskruntime/dispatch_benchmark_test.gointernal/taskruntime/manager.gointernal/taskruntime/manager_internal_test.gointernal/taskruntime/service.gointernal/taskruntime/service_internal_test.gointernal/terminal/app.gointernal/terminal/async_events.gointernal/terminal/autocomplete.gointernal/terminal/commands.gointernal/terminal/prompt_send_internal_test.gointernal/terminal/render_parity_internal_test.gointernal/terminal/running_tools_internal_test.gointernal/terminal/tool_tasks.gointernal/tool/bash.gointernal/tool/bash_ingestion_internal_test.gointernal/tool/bash_windows.gointernal/tool/coordinator.gointernal/tool/mutation_queue.gointernal/tool/mutation_queue_internal_test.gointernal/tool/registry.gointernal/tooltask/service.gointernal/tooltask/service_internal_test.go
da1084d to
48e9f58
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (19)
internal/database/tool_task_repository_test.go (1)
64-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
t.Context()for consistency.Every other call in this file passes
t.Context(). Line 64 passescontext.Background(), so this call is not canceled when the test ends. If the plain context is deliberate, add a short comment.🤖 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/tool_task_repository_test.go` at line 64, Update the Finish call in the test to pass t.Context() instead of context.Background(), matching the other repository calls and ensuring test-scoped cancellation.internal/database/tool_task_repository.go (2)
538-568: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
validateToolTaskmutates its argument.Line 556 assigns a default to
task.PolicyJSON. The name states validation only, andCreatecopies the candidate after this call, so the caller's entity is changed as a side effect. Apply the default inCreateon the localcreatedcopy, and keep the validator free of side effects.🤖 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/tool_task_repository.go` around lines 538 - 568, The validateToolTask function must remain side-effect free: remove its assignment to task.PolicyJSON while preserving validation. In Create, apply the "{}" default to the local created copy before or during validation so the caller’s candidate is not mutated and the persisted entity retains the default.
363-363: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated field name into a constant.
"tool_task.task_id"appears at lines 363, 389, and 460. The file already definestoolTaskOwnerFieldfor the owner equivalent. AddtoolTaskIDFieldnext to it and use it in all three places. SonarCloud reports this as a failure.♻️ Proposed fix
toolTaskOwnerField = "tool_task.owner_session_id" + toolTaskIDField = "tool_task.task_id"- if err := validateUUIDv7("tool_task.task_id", taskID); err != nil { + if err := validateUUIDv7(toolTaskIDField, taskID); err != nil {🤖 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/tool_task_repository.go` at line 363, Define a toolTaskIDField constant alongside toolTaskOwnerField with the value "tool_task.task_id", then replace the duplicated string at all three validateUUIDv7 call sites, including the locations around lines 363, 389, and 460.Source: Linters/SAST tools
internal/di/service_constructors_internal_test.go (1)
146-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRegister the shutdown with
t.Cleanup.Line 146 starts real runtime workers. If a later assertion fails, line 147 never runs and the workers stay active for the rest of the package run. Register the shutdown immediately after
Startsucceeds.♻️ Proposed fix
require.NoError(t, service.Start(t.Context())) - require.NoError(t, service.Shutdown(context.Background())) + t.Cleanup(func() { + require.NoError(t, service.Shutdown(context.Background())) + })🤖 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/di/service_constructors_internal_test.go` around lines 146 - 147, Update the test setup around service.Start so service.Shutdown is registered with t.Cleanup immediately after Start succeeds, ensuring cleanup runs even when later assertions fail; remove the direct shutdown call from the assertion sequence.internal/assistant/lifecycle.go (1)
221-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the argument decoding to a single unmarshal.
tool.ArgumentsFromRawunmarshals intomap[string]json.RawMessage, re-marshals, andFields()unmarshals the same bytes again. The result is then decoded a third time per field. A single decode intomap[string]anyproduces the same payload with less work and less nesting.The current code also drops a malformed field silently.
payloadstill carriesArgumentsJSONthroughlifecycleToolResult, so consumers keep a raw fallback. Confirm that this fallback is the intended behavior for extensions.♻️ Proposed refactor
- if arguments, err := tool.ArgumentsFromRaw([]byte(event.ArgumentsJSON)); err == nil { - fields, fieldsErr := arguments.Fields() - if fieldsErr == nil { - structured := make(map[string]any, len(fields)) - for key, raw := range fields { - var value any - if json.Unmarshal(raw, &value) == nil { - structured[key] = value - } - } - - payload["arguments"] = structured - } - } + structured := map[string]any{} + if err := json.Unmarshal([]byte(event.ArgumentsJSON), &structured); err == nil { + payload["arguments"] = structured + }This removes the
toolimport if it is not used elsewhere 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/assistant/lifecycle.go` around lines 221 - 234, Replace the nested ArgumentsFromRaw, Fields, and per-field json.Unmarshal flow with one json.Unmarshal of event.ArgumentsJSON into map[string]any, assigning the result to payload["arguments"] on successful decoding. Preserve the existing raw ArgumentsJSON fallback through lifecycleToolResult for malformed input and remove the tool import if it is no longer used elsewhere in the file.internal/taskruntime/service_internal_test.go (1)
244-252: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the duplicated database setup with
newRuntimeTestDatabase.Three tests inline the same eight-line SQLite setup: open with
database.SQLiteDSN, register a close cleanup, setSetMaxOpenConns(1), callConfigureSQLite, then callMigrate.newRuntimeTestDatabaseininternal/taskruntime/manager_internal_test.goat lines 129-141 already performs exactly this and takes the filename as a parameter. Other tests in this same file use it at lines 121, 160, 200, 222, 336, 379, 412, 456, and 505.Replace each inline block with a helper call.
♻️ Proposed refactor
- sqliteOptions := database.SQLiteOptions{BusyTimeout: time.Second} - connection, err := sql.Open( - "sqlite", database.SQLiteDSN(filepath.Join(t.TempDir(), "shutdown.db"), sqliteOptions), - ) - require.NoError(t, err) - t.Cleanup(func() { require.NoError(t, connection.Close()) }) - connection.SetMaxOpenConns(1) - require.NoError(t, database.ConfigureSQLite(t.Context(), connection, sqliteOptions)) - require.NoError(t, database.Migrate(t.Context(), connection)) - sessions, err := database.NewSessionRepository(connection) + connection := newRuntimeTestDatabase(t, "shutdown.db") + sessions, err := database.NewSessionRepository(connection)Apply the same change at lines 298-304 with
"unknown.db"and at lines 519-527 with"runtime.db". This also removes thedatabase/sql,path/filepath, and possiblytimeimport usages from those sites.As per coding guidelines: "Follow existing package patterns and keep changes small and focused."
Also applies to: 298-304, 519-527
🤖 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/taskruntime/service_internal_test.go` around lines 244 - 252, Replace each duplicated SQLite setup block in the affected tests with calls to the existing newRuntimeTestDatabase helper, passing the corresponding filenames: shutdown.db, unknown.db, and runtime.db. Remove imports that become unused after this change, while preserving each test’s existing database behavior.Source: Coding guidelines
internal/taskruntime/service.go (2)
421-425: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
oopsfor the panic error to match the package convention.Line 423 builds the panic error with
fmt.Errorf. Every other error in this file carries anoopsdomain and code. A panic is the highest-signal failure in this package, so it benefits most from a queryable code.The message text stays identical, so the assertion in
internal/taskruntime/service_internal_test.goat line 216 continues to pass.♻️ Proposed refactor
defer func() { if recovered := recover(); recovered != nil { - err = fmt.Errorf("task handler panicked: %v", recovered) + err = oops.In("taskruntime").Code("handler_panicked"). + Errorf("task handler panicked: %v", recovered) } }()As per coding guidelines: "Use
oops.In("domain").Code("code").Wrapf(err, "message")for contextual errors where the package already usessamber/oops."🤖 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/taskruntime/service.go` around lines 421 - 425, Update the panic recovery defer in the task handler to construct the error with the package’s oops domain and code convention instead of fmt.Errorf, preserving the exact “task handler panicked: %v” message and the existing recovered value. Keep the surrounding recovery behavior unchanged.Source: Coding guidelines
284-287: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueMake the externally-owned task kinds explicit.
Line 285 hardcodes
database.TaskKindAgentanddatabase.TaskKindWorkflowas kinds that this runtime must not reject. A kind that is added later and handled outside this runtime will have its queued tasks durably transitioned tofailedwithunknown_kind. The failure is silent from the perspective of the owning subsystem.Extract the list into a named package-level variable with a comment that states the invariant, so the coupling is discoverable from the
databasekind definitions.♻️ Proposed refactor
+// externallyHandledKinds are task kinds dispatched outside this runtime. They +// must never be rejected as unknown. Update this list when a new kind is owned +// by another scheduler. +var externallyHandledKinds = []string{database.TaskKindAgent, database.TaskKindWorkflow} + func (service *Service) rejectUnknownKinds(ctx context.Context) { - knownKinds := append([]string{database.TaskKindAgent, database.TaskKindWorkflow}, service.handlerOrder...) + knownKinds := append(append([]string{}, externallyHandledKinds...), service.handlerOrder...)🤖 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/taskruntime/service.go` around lines 284 - 287, Update rejectUnknownKinds to use a named package-level variable instead of hardcoding database.TaskKindAgent and database.TaskKindWorkflow in the append call. Define the variable near the package-level declarations, include a comment stating that these externally owned task kinds must not be rejected by this runtime, and preserve combining it with service.handlerOrder when listing queued tasks.internal/tool/registry_test.go (1)
208-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the test-name comparison with an explicit expectation field.
The assertion path branches on
testCase.name != "decorates registered executor"at Line 220. If someone renames that case, the test takes the error branch and fails in a confusing way. Worse, a case that should succeed but starts failing would still be reported as a pass by the error branch.Add a
wantErrText stringfield to the table and drive the branch from data.♻️ Proposed refactor
tests := []struct { wantError error registry func(*testing.T) *tool.Registry decorate func(tool.Executor) tool.Executor name string + wantErrText string toolName tool.Name }{err := registry.Wrap(testCase.toolName, testCase.decorate) if testCase.wantError != nil { require.ErrorIs(t, err, testCase.wantError) return } - if testCase.name != "decorates registered executor" { + if testCase.wantErrText != "" { require.Error(t, err) - assert.Contains(t, err.Error(), "preserve its name") + assert.Contains(t, err.Error(), testCase.wantErrText) return }🤖 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/tool/registry_test.go` around lines 208 - 232, Add a wantErrText string field to the test-case table and replace the testCase.name comparison in the registry.Wrap test with a data-driven check. For cases with wantErrText, assert the error and matching text; otherwise require no error and preserve the existing execution/result assertions.internal/tooltask/service_internal_test.go (1)
149-206: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the POSIX shell dependency in this test.
The command at Lines 161-163 uses
touch, awhileloop,[ -f ... ], andsleep 0.01. Those require a POSIX shell. The repository shipsinternal/tool/bash_windows.go, so Bash discovery on Windows is a supported path, but a Windows runner without Git Bash, MSYS2, Cygwin, or WSL cannot run this command. The test then blocks for the full 10-secondrequire.Eventuallywindow and fails with a marker-file error that does not name the real cause.Skip the test when a Bash executable is not available, so the failure mode is explicit.
🛡️ Proposed guard
func TestForegroundAndBackgroundMutationsShareCoordinator(t *testing.T) { t.Parallel() + if _, err := exec.LookPath("bash"); err != nil { + t.Skip("bash is not available on this platform") + } + coordinator := tool.NewCoordinator()Add
"os/exec"to the import block.As per coding guidelines: "On Windows, the public
bashtool must use a configured or compatible Bash shell such as Git Bash, MSYS2, Cygwin, or WSL, and must not silently fall back tocmd.exefor Bash semantics."🤖 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/tooltask/service_internal_test.go` around lines 149 - 206, Update TestForegroundAndBackgroundMutationsShareCoordinator to detect whether the configured Bash executable is available before starting the POSIX-dependent command, using os/exec and the repository’s Bash resolution path; call t.Skip with an explicit message when no compatible Bash is found, while preserving the existing test behavior when Bash is available.Source: Coding guidelines
internal/tool/registry.go (1)
37-58: 🚀 Performance & Scalability | 🔵 TrivialBash now takes a shared workspace mutation reservation.
NameBashjoinsNameEditandNameWriteon the coordinator's mutation locks. The testTestForegroundAndBackgroundMutationsShareCoordinatorininternal/tooltask/service_internal_test.goat Lines 149-206 confirms that two Bash calls can no longer overlap, and that a Bash call blocks any concurrent write.This is correct for durable-task safety. It also changes foreground throughput: read-only Bash commands such as
lsorrgnow serialize behind any other Bash or write call. Consider tracking a follow-up that scopes Bash reservations more narrowly, or measuring the added latency on prompts that issue several Bash calls in parallel.🤖 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/tool/registry.go` around lines 37 - 58, Keep NameEdit and NameWrite on the shared coordinator mutation locks, but narrow NameBash reservation to commands that can mutate the workspace; read-only Bash commands such as ls and rg must not serialize with other mutations. Update the newBashTool integration while preserving serialization for mutating Bash operations.internal/tooltask/service.go (1)
456-484: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDocument the
TryAdmiterror contractThe scheduler logs the error but dispatches the task when
admittedistrue. Document this behavior onAdmitter.TryAdmitto prevent future changes from treating a non-nil error as admission failure.🤖 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/tooltask/service.go` around lines 456 - 484, Document the Admitter.TryAdmit contract to state that a true admitted result permits dispatch even when the returned error is non-nil, while false indicates admission failure. Ensure the documentation matches the behavior in Service.TryAdmit, including deferred definition-drift errors.internal/assistant/execute_tool.go (1)
235-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the
oneOfordering contract.
backgroundToolExecutor.Definition()currently places the ordinary schema at index0, and eligible built-in schemas do not currently define nativeoneOfvariants. Add a focused test or shared schema contract so a future ordering change cannot silently alter the schema exposed byexecute.🤖 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/execute_tool.go` around lines 235 - 241, Add a focused test or shared schema contract around backgroundToolExecutor.Definition and the execute schema-selection logic, asserting that the ordinary schema remains the first oneOf variant for eligible built-in tools. Ensure future changes to oneOf ordering fail validation rather than changing the schema exposed by execute.internal/agenttask/service.go (1)
41-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConstant extraction is correct, but one literal remains.
recoverInterruptedat Line 1089 still passes the string literal"task_interrupted"asEventKind. UsetaskInterruptedEventthere so the constant is the single source for this event kind.Also applies to: 978-978
🤖 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` at line 41, Replace the remaining "task_interrupted" string literals passed as EventKind in recoverInterrupted and the additional occurrence with the existing taskInterruptedEvent constant, ensuring it is the single source for this event kind.internal/assistant/background_tool_internal_test.go (2)
229-257: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass the subtest
*testing.Tintocallinstead of capturing the outert.The
callclosures uset.Context()from the parent test. The subtests run in parallel, so the context lifetime is bound to the parent, not to each subtest. Add a*testing.Tparameter tocalland use the subtest instance. This keeps each context scoped to its own subtest and avoidsparalleltest/tparallellint findings.♻️ Proposed signature change
tests := []struct { configure func(*backgroundTaskController) - call func(*Runtime, *backgroundTaskController) error + call func(*testing.T, *Runtime, *backgroundTaskController) error name string code string }{ { name: "list", code: "list_tool_tasks", configure: func(controller *backgroundTaskController) { controller.listErr = errors.New("list failed") }, - call: func(runtime *Runtime, _ *backgroundTaskController) error { + call: func(t *testing.T, runtime *Runtime, _ *backgroundTaskController) error { + t.Helper() _, err := runtime.ToolTasks(t.Context(), "owner", nil, 1) return err }, },Update the
getandcancelentries the same way, then calltestCase.call(t, runtime, controller)at Line 268.🤖 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/background_tool_internal_test.go` around lines 229 - 257, Update the test-case call function signatures for the “list”, “get”, and “cancel” entries to accept a *testing.T parameter, and replace captured outer t.Context() calls with the provided subtest instance. Update the invocation near the test loop to pass the subtest t as the first argument.
461-531: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a case for the
timeout_secondsoverflow guard.
backgroundToolExecutor.startrejects an oversizedtimeout_secondsatinternal/assistant/task_tool.goLines 162-165. No test covers that branch. Add a table case with a largetimeout_secondsvalue andwantError: "timeout_seconds is too large".🤖 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/background_tool_internal_test.go` around lines 461 - 531, Add a table case to TestBackgroundToolExecutorErrorContracts covering start with an oversized timeout_seconds value, using valid assistant invocation metadata and setting wantError to "timeout_seconds is too large" so the overflow guard in backgroundToolExecutor.start is exercised.internal/assistant/task_tool.go (3)
153-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
oopsfor the errors this file creates.The file wraps repository and target errors with
oops.In("assistant").Code(...), but returns plainerrors.Newandfmt.Errorfvalues for its own validation failures. The result is an inconsistent error surface: some assistant errors carry a domain and a code, and others carry neither.Use
oops.In("assistant").Code(...).Errorf(...)for these cases. The tests assert on message substrings only, so the change does not break them.As per coding guidelines: "Use `oops.In("domain").Code("code").Wrapf(err, "message")` for contextual errors where the package already uses `samber/oops`."♻️ Proposed change for the validation errors
- return tool.Result{}, errors.New("background execution requires assistant invocation metadata") + return tool.Result{}, oops.In("assistant").Code("missing_task_invocation"). + Errorf("background execution requires assistant invocation metadata")- return tool.Result{}, errors.New("task_id must be a canonical UUIDv7") + return tool.Result{}, oops.In("assistant").Code("invalid_task_id"). + Errorf("task_id must be a canonical UUIDv7")- return nil, fmt.Errorf("task %q not found", taskID) + return nil, oops.In("assistant").Code("tool_task_not_found"). + Errorf("task %q not found", taskID)Apply the same treatment at Lines 164, 202, 207, and 226.
Also applies to: 164-164, 184-184, 202-202, 207-207, 221-221, 226-226
🤖 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/task_tool.go` at line 153, Replace the plain errors.New and fmt.Errorf validation errors in the task-tool flow, including the cases around assistant invocation metadata and the other referenced validation branches, with oops.In("assistant").Code(...).Errorf(...) errors. Assign appropriate consistent error codes while preserving the existing message content and repository/target error wrapping behavior.Source: Coding guidelines
63-74: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the augmented background tool definition.
tool_schema_cache.gocaches token estimates, notbackgroundToolExecutor.Definition().Registry.Definitions()repeatedly unmarshals, rebuilds, marshals, and validates the same schema. Cache the augmentedtool.Definitionper wrapper and return it on later calls.🤖 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/task_tool.go` around lines 63 - 74, Add per-instance caching to backgroundToolExecutor.Definition so the augmented tool.Definition is built only once and returned on subsequent calls. Store the completed definition on the executor, guarding initialization for concurrent callers as appropriate, while preserving the existing schema transformation and validation behavior.
28-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName parameters and remove the unused generic controller
Name the parameters on both interfaces, including
ownerandtaskID. No code callsGenericTaskControllermethods; remove the interface and its wiring frominternal/assistant/runtime.goandinternal/di/assistant_service.go.🤖 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/task_tool.go` around lines 28 - 42, Name all parameters in ToolTaskController and GenericTaskController methods, using clear names such as owner and taskID. Remove the unused GenericTaskController interface, then remove its related wiring from runtime setup and assistant service dependency injection in runtime.go and assistant_service.go.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/agenttask/service.go`:
- Around line 1035-1053: Update the recovery flow around
service.tasks.RecoverExpired and the unchanged-finish handling so every task ID
returned in recovered is published, rather than only taskID. Alternatively,
replace RecoverExpired with a task-scoped recovery API that guarantees only
taskID can transition; preserve the existing error handling and warning
behavior.
In `@internal/database/migrations/00015_create_tool_tasks.sql`:
- Line 5: Update the task_id column definition in the tool_tasks table migration
to explicitly declare it NOT NULL while preserving its TEXT PRIMARY KEY
constraint, ensuring composite foreign-key rows cannot use NULL task
identifiers.
In `@internal/taskruntime/service.go`:
- Around line 444-452: Update the lease-renewal failure branch in the ticker
handler around Tasks.RenewLease to log the renewal error or unsuccessful result
before calling cancel(). Record both failure cases with enough context to
distinguish a renewal error from a false return, while preserving the existing
cancellation and return behavior.
- Around line 399-415: Update the canceling branch in the task execution switch
to use a distinct sentinel outcome that settle can recognize as an intentional
user cancellation, rather than assigning context.Canceled as a generic run
error. Extend settle to map that sentinel to database.TaskCanceled and preserve
the existing teardown flow through service.settle.
In `@internal/tool/bash_windows_test.go`:
- Around line 56-84: Add a CI Windows test job that runs the Go test suite on a
Windows runner, ensuring *_windows_test.go—including
TestFindWindowsBashRejectsRelativeConfiguredPath and
TestFindWindowsBashDoesNotUseDirectoryCandidate—is executed. Keep the existing
Ubuntu test job and Windows cross-compilation matrix unchanged.
In `@internal/tool/bash_windows.go`:
- Around line 64-68: Update windowsBashCandidates and its discovery error
handling to detect a non-empty relative LIBRECODE_BASH_PATH, retain the security
behavior of excluding it from candidates, and report the configured value with a
clear reason that the path must be absolute. Preserve the existing generic error
for cases where the variable is unset.
In `@internal/tooltask/service.go`:
- Around line 508-545: Ensure PreparedCall reservation ownership is released
consistently: in Service.Run, after resolving call, admit fallback calls before
Execute and defer call.Release for every executing call; update the recovery
flow around RecoverExpired to return recovered task IDs and invoke
ReleaseAdmission for each task no longer owned by this process, covering the
admission-handling code at internal/tooltask/service.go lines 486-506 and Run at
lines 508-545.
- Around line 675-682: Update the truncation logic around the summary limit so
the limit is reduced to a valid UTF-8 rune boundary before slicing summary. Add
the unicode/utf8 dependency and use its boundary-checking functionality before
constructing bounded, ensuring persisted text never ends with a partial rune.
---
Nitpick comments:
In `@internal/agenttask/service.go`:
- Line 41: Replace the remaining "task_interrupted" string literals passed as
EventKind in recoverInterrupted and the additional occurrence with the existing
taskInterruptedEvent constant, ensuring it is the single source for this event
kind.
In `@internal/assistant/background_tool_internal_test.go`:
- Around line 229-257: Update the test-case call function signatures for the
“list”, “get”, and “cancel” entries to accept a *testing.T parameter, and
replace captured outer t.Context() calls with the provided subtest instance.
Update the invocation near the test loop to pass the subtest t as the first
argument.
- Around line 461-531: Add a table case to
TestBackgroundToolExecutorErrorContracts covering start with an oversized
timeout_seconds value, using valid assistant invocation metadata and setting
wantError to "timeout_seconds is too large" so the overflow guard in
backgroundToolExecutor.start is exercised.
In `@internal/assistant/execute_tool.go`:
- Around line 235-241: Add a focused test or shared schema contract around
backgroundToolExecutor.Definition and the execute schema-selection logic,
asserting that the ordinary schema remains the first oneOf variant for eligible
built-in tools. Ensure future changes to oneOf ordering fail validation rather
than changing the schema exposed by execute.
In `@internal/assistant/lifecycle.go`:
- Around line 221-234: Replace the nested ArgumentsFromRaw, Fields, and
per-field json.Unmarshal flow with one json.Unmarshal of event.ArgumentsJSON
into map[string]any, assigning the result to payload["arguments"] on successful
decoding. Preserve the existing raw ArgumentsJSON fallback through
lifecycleToolResult for malformed input and remove the tool import if it is no
longer used elsewhere in the file.
In `@internal/assistant/task_tool.go`:
- Line 153: Replace the plain errors.New and fmt.Errorf validation errors in the
task-tool flow, including the cases around assistant invocation metadata and the
other referenced validation branches, with
oops.In("assistant").Code(...).Errorf(...) errors. Assign appropriate consistent
error codes while preserving the existing message content and repository/target
error wrapping behavior.
- Around line 63-74: Add per-instance caching to
backgroundToolExecutor.Definition so the augmented tool.Definition is built only
once and returned on subsequent calls. Store the completed definition on the
executor, guarding initialization for concurrent callers as appropriate, while
preserving the existing schema transformation and validation behavior.
- Around line 28-42: Name all parameters in ToolTaskController and
GenericTaskController methods, using clear names such as owner and taskID.
Remove the unused GenericTaskController interface, then remove its related
wiring from runtime setup and assistant service dependency injection in
runtime.go and assistant_service.go.
In `@internal/database/tool_task_repository_test.go`:
- Line 64: Update the Finish call in the test to pass t.Context() instead of
context.Background(), matching the other repository calls and ensuring
test-scoped cancellation.
In `@internal/database/tool_task_repository.go`:
- Around line 538-568: The validateToolTask function must remain side-effect
free: remove its assignment to task.PolicyJSON while preserving validation. In
Create, apply the "{}" default to the local created copy before or during
validation so the caller’s candidate is not mutated and the persisted entity
retains the default.
- Line 363: Define a toolTaskIDField constant alongside toolTaskOwnerField with
the value "tool_task.task_id", then replace the duplicated string at all three
validateUUIDv7 call sites, including the locations around lines 363, 389, and
460.
In `@internal/di/service_constructors_internal_test.go`:
- Around line 146-147: Update the test setup around service.Start so
service.Shutdown is registered with t.Cleanup immediately after Start succeeds,
ensuring cleanup runs even when later assertions fail; remove the direct
shutdown call from the assertion sequence.
In `@internal/taskruntime/service_internal_test.go`:
- Around line 244-252: Replace each duplicated SQLite setup block in the
affected tests with calls to the existing newRuntimeTestDatabase helper, passing
the corresponding filenames: shutdown.db, unknown.db, and runtime.db. Remove
imports that become unused after this change, while preserving each test’s
existing database behavior.
In `@internal/taskruntime/service.go`:
- Around line 421-425: Update the panic recovery defer in the task handler to
construct the error with the package’s oops domain and code convention instead
of fmt.Errorf, preserving the exact “task handler panicked: %v” message and the
existing recovered value. Keep the surrounding recovery behavior unchanged.
- Around line 284-287: Update rejectUnknownKinds to use a named package-level
variable instead of hardcoding database.TaskKindAgent and
database.TaskKindWorkflow in the append call. Define the variable near the
package-level declarations, include a comment stating that these externally
owned task kinds must not be rejected by this runtime, and preserve combining it
with service.handlerOrder when listing queued tasks.
In `@internal/tool/registry_test.go`:
- Around line 208-232: Add a wantErrText string field to the test-case table and
replace the testCase.name comparison in the registry.Wrap test with a
data-driven check. For cases with wantErrText, assert the error and matching
text; otherwise require no error and preserve the existing execution/result
assertions.
In `@internal/tool/registry.go`:
- Around line 37-58: Keep NameEdit and NameWrite on the shared coordinator
mutation locks, but narrow NameBash reservation to commands that can mutate the
workspace; read-only Bash commands such as ls and rg must not serialize with
other mutations. Update the newBashTool integration while preserving
serialization for mutating Bash operations.
In `@internal/tooltask/service_internal_test.go`:
- Around line 149-206: Update
TestForegroundAndBackgroundMutationsShareCoordinator to detect whether the
configured Bash executable is available before starting the POSIX-dependent
command, using os/exec and the repository’s Bash resolution path; call t.Skip
with an explicit message when no compatible Bash is found, while preserving the
existing test behavior when Bash is available.
In `@internal/tooltask/service.go`:
- Around line 456-484: Document the Admitter.TryAdmit contract to state that a
true admitted result permits dispatch even when the returned error is non-nil,
while false indicates admission failure. Ensure the documentation matches the
behavior in Service.TryAdmit, including deferred definition-drift errors.
🪄 Autofix
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: 8a6f2c19-dd46-4645-8480-375630d7468f
📒 Files selected for processing (39)
go.modinternal/agenttask/service.gointernal/agenttask/service_internal_test.gointernal/assistant/background_lifecycle.gointernal/assistant/background_tool_internal_test.gointernal/assistant/execute_tool.gointernal/assistant/export_test.gointernal/assistant/lifecycle.gointernal/assistant/task_tool.gointernal/assistant/tool_detach.gointernal/assistant/tool_detach_internal_test.gointernal/assistant/tool_executor.gointernal/assistant/tool_lifecycle_test.gointernal/assistant/tool_registry.gointernal/config/config_internal_test.gointernal/database/migrations/00015_create_tool_tasks.sqlinternal/database/task_repository.gointernal/database/task_repository_branches_test.gointernal/database/tool_task_repository.gointernal/database/tool_task_repository_test.gointernal/di/assistant_service_internal_test.gointernal/di/service_constructors_internal_test.gointernal/provider/openai_chat.gointernal/provider/openai_chat_stream_internal_test.gointernal/provider/tool_loop_internal_test.gointernal/taskruntime/dispatch_benchmark_test.gointernal/taskruntime/manager_internal_test.gointernal/taskruntime/service.gointernal/taskruntime/service_internal_test.gointernal/terminal/app.gointernal/terminal/async_events.gointernal/terminal/tool_tasks.gointernal/terminal/tool_tasks_internal_test.gointernal/tool/bash_windows.gointernal/tool/bash_windows_test.gointernal/tool/registry.gointernal/tool/registry_test.gointernal/tooltask/service.gointernal/tooltask/service_internal_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- internal/assistant/tool_detach.go
- internal/provider/tool_loop_internal_test.go
- internal/terminal/async_events.go
- internal/assistant/background_lifecycle.go
- internal/assistant/tool_registry.go
- internal/terminal/tool_tasks.go
- internal/terminal/app.go
- internal/taskruntime/dispatch_benchmark_test.go
- internal/assistant/tool_executor.go
- internal/assistant/tool_detach_internal_test.go
- internal/di/assistant_service_internal_test.go
- internal/database/task_repository.go
48e9f58 to
a41b098
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/agenttask/service.go (1)
1035-1053: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPublish every task that this sweep recovers.
RecoverExpiredhere is not scoped totaskID. It transitions every expired agent task. This path then publishes onlytaskID(Line 1056), so live subscribers of the other recovered tasks receive no terminal event from this call.recoverInterrupted(Lines 1097-1099) already publishes each recovered ID; apply the same rule here.♻️ Proposed fix
if !changed { recovered, recoverErr := service.tasks.RecoverExpired(ctx, &database.TaskRecovery{ Kind: database.TaskKindAgent, TargetState: database.TaskInterrupted, EventKind: taskInterruptedEvent, ErrorCode: "process_restart", ErrorMessage: "task interrupted after its worker lease expired", PayloadJSON: `{"error_code":"process_restart"}`, ExpiresBefore: time.Now(), }) if recoverErr != nil { service.logError( ctx, "recover expired agent task after unchanged finish", "task_id", taskID, "error", recoverErr, ) return } + for _, recoveredID := range recovered { + if recoveredID != taskID { + service.publishLatest(ctx, recoveredID) + } + } + if !slices.Contains(recovered, taskID) { service.logWarn(ctx, "agent task finish was unchanged", "task_id", taskID) return } }🤖 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 1035 - 1053, Update the recovery handling around RecoverExpired to publish a terminal event for every task ID returned in recovered, rather than only taskID. Mirror the per-ID publishing behavior used by recoverInterrupted, while preserving the existing recoverErr handling and unchanged-task warning behavior.internal/tooltask/service.go (1)
549-588: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftThe fallback path still executes without a mutation reservation.
Line 569 now releases the call on every path. That closes half of the earlier ownership gap. The other half remains: when
admittedCallreturns nil,Runprepares a fresh call and executes it at Line 579 without callingTryAdmitorAdmit. This path runs whenever a worker claims a task that this process did not admit, for example after lease recovery. Confirm whethertool.PreparedCall.Executerequires a held reservation for mutating tools; if it does, admit the fallback call before execution.#!/bin/bash # Description: Inspect PreparedCall reservation semantics. set -euo pipefail fd -t f 'prepared_call|registry|mutation_queue|coordinator' internal/tool ast-grep run --pattern $'func ($_ *PreparedCall) $NAME($$$) { $$$ }' --lang go internal/tool rg -nP --type=go -C4 '\bfunc \(.*PreparedCall\) (Execute|Admit|TryAdmit|Release)\s*\('🤖 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/tooltask/service.go` around lines 549 - 588, Update the fallback branch in Service.Run around admittedCall and preparePersistedCall to verify PreparedCall.Execute’s reservation requirement, then acquire a mutation reservation for the newly prepared call before execution using the established TryAdmit or Admit flow. Handle admission failure without executing, and preserve the existing deferred call.Release ownership for successfully admitted fallback calls.
🧹 Nitpick comments (4)
internal/taskruntime/service_internal_test.go (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the package error convention.
Replace
fmt.Errorfwith the existing structuredoopspattern for this contextual handler error. Remove the unusedfmtimport.Proposed fix
import ( "bytes" "context" - "fmt" "log/slog" @@ - return Outcome{}, fmt.Errorf("wait for cancellation: %w", ctx.Err()) + return Outcome{}, oops.In("taskruntime").Code("handler_canceled").Wrapf(ctx.Err(), "wait for cancellation") }As per coding guidelines, "
**/*.go: Useoops.In("domain").Code("code").Wrapf(err, "message")for contextual errors where the package already usessamber/oops."Also applies to: 45-53
🤖 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/taskruntime/service_internal_test.go` at line 6, Replace the contextual handler error’s fmt.Errorf usage with the package’s established oops.In(...).Code(...).Wrapf(...) convention, preserving the existing error context and message; then remove the now-unused fmt import from the test file.Source: Coding guidelines
internal/terminal/async_events.go (1)
721-723: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the inline
"task_"prefix with a named predicate.The neighboring checks use named helpers:
workflowToolNameat Line 711 andisAgentManagementToolat Line 715. The raw"task_"literal here breaks that pattern. A named predicate also documents which tool family triggers the refresh, which matters because the background tool wrapper removestask_-prefixed names from the prompt registry.♻️ Proposed refactor
- if strings.HasPrefix(event.Name, "task_") { + if isTaskManagementTool(event.Name) { app.logToolTaskRefreshError(ctx, app.refreshToolTasks(ctx)) }Define the predicate next to
isAgentManagementTool:const taskToolPrefix = "task_" func isTaskManagementTool(name string) bool { return strings.HasPrefix(name, taskToolPrefix) }🤖 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 721 - 723, Replace the inline "task_" prefix check in the event handling flow with a named isTaskManagementTool predicate. Define taskToolPrefix and isTaskManagementTool alongside isAgentManagementTool, then use the predicate in the refresh condition while preserving the existing behavior.internal/assistant/background_tool_internal_test.go (1)
339-365: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a table-driven form for
TestForegroundOutcomeContracts.The test checks five separate outcome contracts in one sequential body. If one assertion fails, the later contracts are not exercised, and the failure name does not identify the case. A table with named subtests reports each contract independently.
The coding guidelines ask for table-driven tests for core behavior.
♻️ Proposed table-driven form
func TestForegroundOutcomeContracts(t *testing.T) { t.Parallel() - result, err := foregroundOutcome(nil, errors.New("wait failed")) - require.ErrorContains(t, err, "wait failed") - assert.Empty(t, result.Text()) - - _, err = foregroundOutcome(nil, nil) - require.ErrorContains(t, err, "without an outcome") - - entity := new(database.ToolTaskEntity) - invalid := "not-json" - entity.OutcomeJSON = &invalid - _, err = foregroundOutcome(entity, nil) - require.ErrorContains(t, err, "decode foreground outcome") - - failure := `{"result":{"content":[]},"error":"tool failed","is_error":true}` - entity.OutcomeJSON = &failure - _, err = foregroundOutcome(entity, nil) - require.ErrorContains(t, err, "tool failed") - - success := `{"result":{"content":[{"type":"text","text":"done"}]},"is_error":false}` - entity.OutcomeJSON = &success - result, err = foregroundOutcome(entity, nil) - require.NoError(t, err) - assert.Equal(t, "done", result.Text()) + tests := []struct { + name string + outcome *string + waitErr error + wantError string + wantText string + }{ + {name: "wait error", outcome: nil, waitErr: errors.New("wait failed"), wantError: "wait failed"}, + {name: "missing outcome", outcome: nil, waitErr: nil, wantError: "without an outcome"}, + {name: "invalid outcome", outcome: ptr("not-json"), wantError: "decode foreground outcome"}, + { + name: "tool failure", + outcome: ptr(`{"result":{"content":[]},"error":"tool failed","is_error":true}`), + wantError: "tool failed", + }, + { + name: "success", + outcome: ptr(`{"result":{"content":[{"type":"text","text":"done"}]},"is_error":false}`), + wantText: "done", + }, + } + + for _, testCase := range tests { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + var entity *database.ToolTaskEntity + if testCase.outcome != nil { + entity = new(database.ToolTaskEntity) + entity.OutcomeJSON = testCase.outcome + } + + result, err := foregroundOutcome(entity, testCase.waitErr) + if testCase.wantError != "" { + require.ErrorContains(t, err, testCase.wantError) + + return + } + + require.NoError(t, err) + assert.Equal(t, testCase.wantText, result.Text()) + }) + } }
ptris a small generic helper:func ptr[T any](value T) *T { return &value }.🤖 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/background_tool_internal_test.go` around lines 339 - 365, Refactor TestForegroundOutcomeContracts into a named table-driven test with one subtest per outcome contract, including wait failure, missing outcome, invalid JSON, tool failure, and success. Move each case’s setup and assertions into its subtest so failures identify the specific contract and all cases run independently; use the existing outcome symbols and a small pointer helper if needed for OutcomeJSON.Source: Coding guidelines
internal/terminal/app.go (1)
457-464: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe periodic tool-task refresh does not run when only durable tool tasks are active.
workTickat Line 572 returns the ticker channel only when the app is busy, agent tasks are running, or the agent-task or workflow panel is selected. A session that runs only durable background tool tasks matches none of these conditions. The refresh at Line 461 then never fires, andapp.toolTasksstays stale until the user runs/tasksagain.Completions still render through the completion watcher, so this affects only the cached list snapshot. Consider adding a running-tool-task condition to
workTick.♻️ Proposed condition
func (app *App) workTick(ticker *time.Ticker) <-chan time.Time { - if app.busy() || app.hasRunningAgentTasks() || + if app.busy() || app.hasRunningAgentTasks() || app.hasRunningToolTasks() || app.selectedPanelKind == panelAgentTasks || app.selectedPanelKind == panelWorkflows { return ticker.C } return nil }Add the predicate next to the existing tool-task helpers:
func (app *App) hasRunningToolTasks() bool { for index := range app.toolTasks { switch app.toolTasks[index].Task.State { case database.TaskQueued, database.TaskRunning: return true } } return false }🤖 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/app.go` around lines 457 - 464, Update App.workTick to return the ticker channel when durable tool tasks are queued or running, so periodic refreshes continue without other activity. Add or reuse a hasRunningToolTasks helper that checks app.toolTasks states against database.TaskQueued and database.TaskRunning, and include it in the existing workTick condition while preserving current 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.
Inline comments:
In `@internal/terminal/tool_tasks.go`:
- Around line 31-34: Update the error branch after SubscribeToolTaskCompletions
in the tool-task subscription flow to call the existing logToolTaskRefreshError
with the subscription error before returning, preserving the current early-exit
behavior.
---
Duplicate comments:
In `@internal/agenttask/service.go`:
- Around line 1035-1053: Update the recovery handling around RecoverExpired to
publish a terminal event for every task ID returned in recovered, rather than
only taskID. Mirror the per-ID publishing behavior used by recoverInterrupted,
while preserving the existing recoverErr handling and unchanged-task warning
behavior.
In `@internal/tooltask/service.go`:
- Around line 549-588: Update the fallback branch in Service.Run around
admittedCall and preparePersistedCall to verify PreparedCall.Execute’s
reservation requirement, then acquire a mutation reservation for the newly
prepared call before execution using the established TryAdmit or Admit flow.
Handle admission failure without executing, and preserve the existing deferred
call.Release ownership for successfully admitted fallback calls.
---
Nitpick comments:
In `@internal/assistant/background_tool_internal_test.go`:
- Around line 339-365: Refactor TestForegroundOutcomeContracts into a named
table-driven test with one subtest per outcome contract, including wait failure,
missing outcome, invalid JSON, tool failure, and success. Move each case’s setup
and assertions into its subtest so failures identify the specific contract and
all cases run independently; use the existing outcome symbols and a small
pointer helper if needed for OutcomeJSON.
In `@internal/taskruntime/service_internal_test.go`:
- Line 6: Replace the contextual handler error’s fmt.Errorf usage with the
package’s established oops.In(...).Code(...).Wrapf(...) convention, preserving
the existing error context and message; then remove the now-unused fmt import
from the test file.
In `@internal/terminal/app.go`:
- Around line 457-464: Update App.workTick to return the ticker channel when
durable tool tasks are queued or running, so periodic refreshes continue without
other activity. Add or reuse a hasRunningToolTasks helper that checks
app.toolTasks states against database.TaskQueued and database.TaskRunning, and
include it in the existing workTick condition while preserving current behavior.
In `@internal/terminal/async_events.go`:
- Around line 721-723: Replace the inline "task_" prefix check in the event
handling flow with a named isTaskManagementTool predicate. Define taskToolPrefix
and isTaskManagementTool alongside isAgentManagementTool, then use the predicate
in the refresh condition while preserving the existing behavior.
🪄 Autofix
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: d8d2c1b2-85a7-4b8b-b1db-18b89264f1c9
📒 Files selected for processing (38)
internal/agenttask/service.gointernal/agenttask/service_internal_test.gointernal/assistant/background_lifecycle.gointernal/assistant/background_tool_internal_test.gointernal/assistant/execute_tool_internal_test.gointernal/assistant/lifecycle.gointernal/assistant/provider_hook_test_helpers_internal_test.gointernal/assistant/runtime.gointernal/assistant/task_tool.gointernal/assistant/testing.gointernal/assistant/tool_detach.gointernal/assistant/tool_detach_internal_test.gointernal/assistant/tool_executor.gointernal/assistant/tool_executor_internal_test.gointernal/assistant/tool_lifecycle_test.gointernal/assistant/tool_registry.gointernal/assistant/tool_schema_cache_internal_test.gointernal/database/migrations/00015_create_tool_tasks.sqlinternal/database/migrations_test.gointernal/database/task_repository.gointernal/database/task_repository_branches_test.gointernal/database/tool_task_repository.gointernal/database/tool_task_repository_test.gointernal/di/assistant_service.gointernal/di/service_constructors_internal_test.gointernal/taskruntime/manager_internal_test.gointernal/taskruntime/service.gointernal/taskruntime/service_internal_test.gointernal/terminal/app.gointernal/terminal/async_events.gointernal/terminal/tool_tasks.gointernal/terminal/tool_tasks_internal_test.gointernal/tool/bash_ingestion_internal_test.gointernal/tool/bash_windows.gointernal/tool/bash_windows_test.gointernal/tool/registry_test.gointernal/tooltask/service.gointernal/tooltask/service_internal_test.go
🚧 Files skipped from review as they are similar to previous changes (20)
- internal/assistant/lifecycle.go
- internal/assistant/tool_registry.go
- internal/assistant/provider_hook_test_helpers_internal_test.go
- internal/tool/bash_windows.go
- internal/assistant/tool_schema_cache_internal_test.go
- internal/assistant/tool_lifecycle_test.go
- internal/assistant/tool_executor_internal_test.go
- internal/tool/bash_ingestion_internal_test.go
- internal/database/task_repository_branches_test.go
- internal/di/service_constructors_internal_test.go
- internal/assistant/tool_detach_internal_test.go
- internal/tool/bash_windows_test.go
- internal/di/assistant_service.go
- internal/assistant/tool_detach.go
- internal/tool/registry_test.go
- internal/database/task_repository.go
- internal/assistant/tool_executor.go
- internal/tooltask/service_internal_test.go
- internal/taskruntime/service.go
- internal/database/tool_task_repository.go
a41b098 to
69804ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/ci.yml:
- Around line 76-77: Update the test-windows job to add the existing mise setup
step before its Test step, then change the Test command to run go test through
mise exec while preserving the race flag and ./... target.
- Around line 67-68: Update the Checkout step in the CI workflow to set
persist-credentials to false, ensuring the Windows test job does not retain Git
credentials after actions/checkout completes.
🪄 Autofix
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: 58df5a9f-ba24-4ea5-86e1-3a852ed9e864
📒 Files selected for processing (10)
.github/workflows/ci.ymlinternal/agenttask/service.gointernal/assistant/background_tool_internal_test.gointernal/assistant/extension_tool_internal_test.gointernal/database/task_repository.gointernal/taskruntime/service_internal_test.gointernal/terminal/agent_tasks.gointernal/terminal/app.gointernal/terminal/async_events.gointernal/terminal/tool_tasks.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/terminal/async_events.go
- internal/terminal/tool_tasks.go
- internal/agenttask/service.go
- internal/taskruntime/service_internal_test.go
- internal/database/task_repository.go
- internal/terminal/app.go
69804ba to
a1e2b53
Compare
|
1 participant
No description provided.