feat/mvm-execute#210
Merged
Merged
Conversation
docs(mvm): mark workflow roadmap complete
📝 WalkthroughWalkthroughThis PR adds durable workflow execution, isolated MVM evaluation through an execute worker, nested tool-call identity tracking, prompt metrics and strategy selection, workflow-aware dependency injection, and terminal UI support for active and failed workflows. ChangesWorkflow execution and persistence
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #210 +/- ##
==========================================
+ Coverage 84.33% 84.51% +0.18%
==========================================
Files 296 311 +15
Lines 25579 28086 +2507
==========================================
+ Hits 21572 23737 +2165
- Misses 2767 3001 +234
- Partials 1240 1348 +108
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: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/terminal/running_tools.go (1)
10-24: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTreat a non-empty CallID as authoritative.
If
event.CallIDis present but no matching block exists, the code falls back to arguments/name matching. A stale event can therefore remove another nested or concurrent block with identical arguments or name. Only use the legacy fallbacks whenevent.CallIDis empty.Proposed fix
- if index, ok := app.runningToolBlockIndexByCallID(event.CallID); ok { - app.deleteRunningToolBlock(index) - - return + if event.CallID != "" { + if index, ok := app.runningToolBlockIndexByCallID(event.CallID); ok { + app.deleteRunningToolBlock(index) + } + + 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/terminal/running_tools.go` around lines 10 - 24, Update the running-tool removal flow so a non-empty event.CallID is authoritative: after runningToolBlockIndexByCallID fails, return without attempting argument or name matching. Keep runningToolBlockIndexByArguments and runningToolBlockIndexByName as fallbacks only when event.CallID is empty.internal/assistant/runtime_persist.go (1)
455-484: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not fall back to name matching when
CallIDis present.An unknown nonempty
CallIDcurrently falls through and can remove an unrelated pending call with the same name or arguments, corrupting later failure persistence.Proposed fix
func (progress *partialPromptProgress) pendingToolIndex(callID, name, argumentsJSON string) (int, bool) { if callID != "" { for index, pending := range progress.pendingTools { if pending.CallID == callID { return index, true } } + + return 0, 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/assistant/runtime_persist.go` around lines 455 - 484, Update pendingToolIndex so any nonempty CallID is matched exclusively by CallID: return not found immediately when no pending entry has that ID, without attempting name or arguments matching. Retain the existing name-and-arguments and name-only fallback matching only when CallID is empty.
🟡 Minor comments (4)
internal/workflow/service.go-77-80 (1)
77-80: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle
uuid.NewV7errors inNewService.uuid.Must(...)can still panic here even though the constructor already returns an error; return the UUID error instead and wrap it with the existingoops.In("workflow")pattern.🤖 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/workflow/service.go` around lines 77 - 80, Update NewService to call uuid.NewV7 without uuid.Must, capture and return any UUID generation error wrapped with the existing oops.In("workflow") pattern, and only construct the Service after successful UUID creation.Source: Coding guidelines
cmd/librecode/prompt.go-122-127 (1)
122-127: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the prompt failure when metrics output also fails.
Lines 122-127 return
metricsErrfirst, maskingpromptErr. If both operations fail, report both errors while keeping the prompt failure primary.🤖 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 `@cmd/librecode/prompt.go` around lines 122 - 127, Update the error handling after writePromptMetrics in the prompt execution flow to preserve promptErr as the primary failure when both prompt execution and metrics writing fail, while including metricsErr as additional context. Keep returning metricsErr alone when promptErr is nil.internal/terminal/agent_tasks.go-266-269 (1)
266-269: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
/workflowsin the failure hint.internal/terminal/agent_tasks.go:267currently points users to/workflow, but the terminal command is registered asworkflows, so the recovery hint should match the actual command.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminal/agent_tasks.go` around lines 266 - 269, Update the failure hint in the completion message within the agent task flow to reference the registered /workflows command instead of /workflow. Keep the existing formatting and all other message content unchanged.internal/executeworker/client.go-174-177 (1)
174-177: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve cancellation and protocol-read causes during worker shutdown.
Cancellation after receiving the result frame currently surfaces as a raw
Waiterror. Separately, a non-EOF read error is discarded whenwaitErr == nil, producing%!w(<nil>). PassctxintofinishResultand centralize termination-error normalization.Proposed fix
- return finishResult(worker, stdin, &message) + return finishResult(ctx, worker, stdin, &message) -func finishResult(worker *workerProcess, stdin io.Closer, message *Message) (mvmhost.Result, error) { +func finishResult(ctx context.Context, worker *workerProcess, stdin io.Closer, message *Message) (mvmhost.Result, error) { if err := stdin.Close(); err != nil { return mvmhost.Result{}, worker.abort(fmt.Errorf("close execute worker stdin: %w", err)) } if err := worker.cmd.Wait(); err != nil { + if ctx.Err() != nil { + return mvmhost.Result{}, canceledError(ctx.Err()) + } return mvmhost.Result{}, fmt.Errorf("wait for execute worker: %w", err) }- if waitErr != nil && !errors.Is(readErr, io.EOF) { - return fmt.Errorf("read execute worker: %w", readErr) + if !errors.Is(readErr, io.EOF) { + return errors.Join(fmt.Errorf("read execute worker: %w", readErr), waitErr) } - return fmt.Errorf("execute worker exited without result: %w", waitErr) + if waitErr != nil { + return fmt.Errorf("execute worker exited without result: %w", waitErr) + } + + return errors.New("execute worker exited without result")Also applies to: 215-222, 269-281
🤖 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/executeworker/client.go` around lines 174 - 177, Update the worker result shutdown flow around callbacks.Wait and finishResult to pass ctx into finishResult and centralize termination-error normalization. Preserve cancellation errors after receiving a result, and retain non-EOF protocol-read errors when waitErr is nil instead of wrapping a nil error. Apply the same normalization to the related termination paths identified in finishResult and worker read handling.
🧹 Nitpick comments (11)
internal/assistant/execute_tool_internal_test.go (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the package’s contextual error wrapper in the test executor.
Proposed fix
- "fmt" ... - return tool.Result{}, fmt.Errorf("decode echo input: %w", err) + return tool.Result{}, oops.In("assistant").Code("execute_test_echo_input"). + Wrapf(err, "decode echo input")As per coding guidelines, “Use
oops.In('domain').Code('code').Wrapf(err, 'message')for contextual errors where the package already uses samber/oops.” <coding_guidelines>Also applies to: 40-42
🤖 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_internal_test.go` at line 6, Replace the test executor’s fmt-based contextual error construction with the package’s existing oops pattern, using In, Code, and Wrapf with the appropriate domain and error code; update imports accordingly and preserve the original message and wrapped cause.Source: Coding guidelines
internal/database/agent_task_repository_test.go (1)
91-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise a failure after the child session is inserted.
Neither error case tests rollback: one fails on the child INSERT, and the other fails before the transaction. Add a case with a valid owner but a nonexistent UUIDv7
ParentTaskID, causinginsertTaskto fail afterinsertSession; then assert the child count remains unchanged. Prefer table-driven subtests for these failure stages.As per coding guidelines,
**/*_test.goshould 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/database/agent_task_repository_test.go` around lines 91 - 105, The CreateWithChildSession tests currently cover failures before child-session insertion but not rollback after insertion. Refactor the failure cases around CreateWithChildSession into table-driven subtests, adding a valid owner with a nonexistent UUIDv7 ParentTaskID so insertTask fails after insertSession; assert each case returns the expected error and that ListChildSessions still reports the original count.Source: Coding guidelines
internal/workflow/workflow.go (2)
294-300: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse structured
oopserrors for workflow validation.These raw errors lose the package domain and stable error code used elsewhere.
Proposed fix
- return "", errors.New("workflow: agent prompt is required") + return "", oops.In("workflow").Code("invalid_agent_prompt"). + Errorf("agent prompt is required") - return AgentOptions{}, errors.New("workflow: agent accepts at most one options value") + return AgentOptions{}, oops.In("workflow").Code("invalid_agent_options"). + Errorf("agent accepts at most one options value")As per coding guidelines, “Use
oops.In('domain').Code('code').Wrapf(err, 'message')for contextual errors where the package already uses samber/oops.”Also applies to: 445-455
🤖 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/workflow/workflow.go` around lines 294 - 300, Update the workflow validation errors in the function containing the prompt check and the additional validation at the referenced later block to use the package’s established samber/oops pattern, including the workflow domain, stable error code, and contextual message via Wrapf; replace the raw errors.New returns while preserving the existing validation behavior.Source: Coding guidelines
361-363: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnblock Sonar by extracting the ownership message.
The supplied Sonar check marks this four-times duplicated literal as a failure. Define one package constant and reuse it.
Also applies to: 371-373, 396-398, 418-420
🤖 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/workflow/workflow.go` around lines 361 - 363, Extract the duplicated “task is not owned by this workflow session” text into a single package-level constant in the workflow package, then update the ownership error paths in the surrounding workflow logic—including the branches at the referenced locations—to reuse that constant while preserving their existing error codes and behavior.Source: Linters/SAST tools
cmd/librecode/workflow_internal_test.go (1)
13-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover every workflow subcommand’s argument contract.
run,list,get,events, andcancellack the focused validation coverage required for this new CLI surface. Add them to a table-driven command/argument test alongside the existing cases.As per coding guidelines, CLI commands should add tests for argument validation when practical, and Go tests should 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 `@cmd/librecode/workflow_internal_test.go` around lines 13 - 64, Expand the workflow command argument-validation tests around newWorkflowCmd to use a table-driven test covering submit, worker, metrics, resume, run, list, get, events, and cancel. For each subcommand, assert its Use string and validate both accepted and rejected argument counts according to its contract, preserving the existing coverage while consolidating it into the table.Source: Coding guidelines
cmd/librecode/workflow.go (1)
390-413: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository’s structured
oopswrapping convention.Lines 392, 401, 413, and 473 use
oops.Wrapfdirectly. Wrap these withoops.In("workflow").Code("...").Wrapf(...)and stable per-failure codes instead.As per coding guidelines, use
oops.In('domain').Code('code').Wrapf(err, 'message')for contextual errors where the package already uses samber/oops.Also applies to: 472-473
🤖 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 `@cmd/librecode/workflow.go` around lines 390 - 413, Update the workflow metrics error paths around service.Get, service.AgentTasks, AgentTask, and the additional failure at the referenced later location to use oops.In("workflow").Code("...").Wrapf(...) instead of direct oops.Wrapf calls. Assign a distinct stable code to each failure while preserving the existing contextual messages and return behavior.Source: Coding guidelines
internal/terminal/workflow_commands_internal_test.go (1)
22-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover ownership and inspector-error branches.
Add table-driven cases for a foreign-session run and failures from
Get,Events, andAgentTasks; the current stub cannot exercise these core inspection paths.As per coding guidelines, "
**/*_test.go: Prefer table-driven tests for core behavior and regression tests for terminal rendering bugs in Go."Also applies to: 146-179
🤖 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/workflow_commands_internal_test.go` around lines 22 - 68, Add table-driven coverage for the workflow inspection behavior using workflowInspectorStub, including a foreign-session run and injected errors from Get, Events, and AgentTasks. Extend the stub with configurable errors for those methods, return them when set, and assert each command path reports the expected failure or ownership result while preserving the existing List validation.Source: Coding guidelines
internal/mvmhost/host.go (1)
168-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd error codes to these contextual oops wrappers.
Use stable codes such as
mvm_evalandoutput_capture.As per coding guidelines, use
oops.In('domain').Code('code').Wrapf(err, 'message')for contextual errors where the package already uses samber/oops.Also applies to: 423-442
🤖 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/mvmhost/host.go` around lines 168 - 170, The contextual oops wrappers in the MVM evaluation and output-capture error paths need stable error codes. Update the wrappers around machine.Eval and the output-capture handling to use oops.In("mvmhost").Code("mvm_eval") and oops.In("mvmhost").Code("output_capture") respectively before Wrapf, preserving their existing messages and error flow.Source: Coding guidelines
internal/assistant/tool_lifecycle_test.go (1)
58-60: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the lifecycle tests depend on identity propagation.
Zero-valued metadata cannot detect regressions in the newly added payload mapping. Use nonempty parent/call IDs and a positive sequence, then verify the Lua handler receives them.
Also applies to: 94-97
🤖 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_lifecycle_test.go` around lines 58 - 60, Update the lifecycle tests around the metadata setup and Lua handler assertions to use nonempty ParentCallID and CallID values plus a positive Sequence, then verify the Lua handler receives those exact propagated values. Apply the same identity assertions to the additional test case referenced by the comment.internal/assistant/llm_conversion_internal_test.go (1)
122-143: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the new identity fields with nonzero values.
These fixtures still pass if conversion drops
CallID,ParentCallID, orSequence. Populate them and assert the resulting tool-result ID and metadata.🤖 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/llm_conversion_internal_test.go` around lines 122 - 143, Update the ToolEvents fixtures in the relevant conversion test to use nonzero CallID, ParentCallID, and Sequence values, then assert that conversion preserves them in the resulting tool-result ID and metadata. Cover both tool events as needed so the test fails if any new identity field is dropped.internal/assistant/lifecyclepayload/lifecyclepayload_test.go (1)
158-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the newly added identity fields with non-default values.
The test does not verify that
parent_call_id,sequence, or the resultcall_idare actually propagated, so regressions could still pass.Proposed test coverage
- ParentCallID: "", + ParentCallID: "parent-1", ... - Sequence: 0, + Sequence: 2, }) assert.Equal(t, "call-1", toolCall["call_id"]) + assert.Equal(t, "parent-1", toolCall["parent_call_id"]) + assert.Equal(t, 2, toolCall["sequence"]) ... - CallID: "", - ParentCallID: "", + CallID: "call-1", + ParentCallID: "parent-1", ... - Sequence: 0, + Sequence: 2, }) + assert.Equal(t, "call-1", toolResult["call_id"]) + assert.Equal(t, "parent-1", toolResult["parent_call_id"]) + assert.Equal(t, 2, toolResult["sequence"])🤖 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/lifecyclepayload/lifecyclepayload_test.go` around lines 158 - 181, Update the lifecycle payload test around ToolCallPayload and ToolResultPayload to use non-default ParentCallID and Sequence values, and provide a non-empty ToolResult CallID. Add assertions verifying parent_call_id, sequence, and the result call_id are propagated in the generated payloads, while preserving the existing name, error, and is_error checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/librecode/prompt.go`:
- Around line 172-179: Update both error paths in the metrics-writing flow
around json.MarshalIndent and os.WriteFile to use the established
oops.In(...).Code(...).Wrapf pattern. Assign the appropriate repository domain
and stable codes encode_prompt_metrics and write_prompt_metrics while preserving
the existing contextual messages and error propagation.
In `@internal/agenttask/runtime_runner.go`:
- Around line 77-80: Update the error-return path in the Prompt flow after
agentUsageJSON to return the captured usageJSON instead of the hardcoded "{}".
Preserve the existing error wrapping and ensure usage from metrics.Snapshot()
remains available when Prompt fails.
In `@internal/agenttask/service.go`:
- Around line 120-123: Update the dependency validation at the start of New to
check options == nil before accessing options.Tasks, options.AgentTasks, or
options.Runner. Preserve the existing required-dependency error behavior for
both nil options and incomplete option values.
In `@internal/assistant/execute_tool.go`:
- Around line 111-116: Update the nested tool argument handling in the executor
flow around executor.call to preserve JSON numeric precision: avoid
unmarshalling message.Input into any, and pass the raw message.Input directly to
tool.ArgumentsFromRaw, or configure decoding with UseNumber. Apply the same
change to the corresponding handling at the additional referenced location.
In `@internal/assistant/tool_executor.go`:
- Around line 72-78: Update the ToolCallEvent initialization in
assistantToolCallsFromLLM to populate ParentCallID and Sequence from the
corresponding ToolCall.Metadata values, preserving parent-child hierarchy and
ordering for lifecycle, persistence, and terminal consumers.
- Around line 229-232: Update the details handling in the event-result
construction to clear result.Details before attempting to unmarshal
event.DetailsJSON, so malformed lifecycle-mutated JSON cannot retain the
original details. Preserve assigning decoded details on successful
unmarshalling, and add a regression test covering malformed DetailsJSON and
verifying details are empty or cleared.
In `@internal/assistant/workflow_tool.go`:
- Around line 60-102: Update Execute in workflowToolExecutor to wrap every
listed failure path—the nil submitter, input Decode failure, workflow
name/source and UTF-8 validation failures, argument JSON encoding failure, and
submitter.Submit failure—with assistant-scoped oops errors. Preserve each
existing error message and validation behavior while replacing plain
errors.New/fmt.Errorf or directly propagated errors with the established oops
construction and appropriate assistant error codes.
In `@internal/database/agent_task_repository.go`:
- Line 104: Update prepareCreate to validate a nil agentTask before
dereferencing it, returning a contextual samber/oops error via the package’s
oops.In(...).Code(...) pattern; ensure both Create APIs propagate this error
without panicking.
In `@internal/database/validation.go`:
- Around line 131-133: Update the validation around argumentsJSON in the visible
validation flow to require a valid JSON object, not merely syntactically valid
JSON. Ensure arrays, scalars, and other non-object values are rejected before
persistence, while preserving acceptance of JSON objects for decodeArguments.
In `@internal/database/workflow_repository.go`:
- Around line 281-304: The LinkAgentTask transaction still performs a race-prone
read followed by insert, while insertWorkflowAgentTask separately derives the
next sequence. Make workflow-link creation atomic so concurrent calls for the
same workflowTaskID, nodeKey, and invocationIndex return the existing link
rather than surfacing duplicate-link or sequence errors; alternatively retry
conflicts and re-read the row. Add a concurrent regression test covering these
calls.
In `@internal/executeworker/protocol.go`:
- Around line 16-20: The protocol limits in internal/executeworker/protocol.go
around MaxFrameSize and MaxResultSize must be composable so the combined encoded
output and result always fit within one response frame; enforce an aggregate
response budget or provide equivalent worst-case frame headroom. In
internal/mvmhost/host.go, derive the default output cap from that aggregate
protocol budget, and add coverage for combined output/value boundary cases.
In `@internal/executeworker/worker.go`:
- Around line 16-23: Update rpcCaller’s reader loop and exchange/write paths to
retain the terminal read error, deliver failure results to every pending RPC
channel, and reject exchanges started after reader termination. Handle the
reader error explicitly, remove each pending entry when Write fails, and ensure
all error-returning operations in this flow are checked rather than discarded.
In `@internal/workflow/dispatcher_cross_process_test.go`:
- Around line 20-49: The cross-process test currently shares one *sql.DB with a
single connection, so it does not validate independent database access. Update
the setup around submitter, worker, and dispatcher to use a file-backed SQLite
database opened through separate sql.Open handles, with the worker/dispatcher
accessing their own handle; preserve migration and cleanup while exercising
cross-process visibility, locking, and atomic task claims.
In `@internal/workflow/dispatcher.go`:
- Around line 88-125: Synchronize Dispatcher.Submit with the shutdown lifecycle
so new submissions are rejected once Shutdown begins, before calling
service.Submit or enqueue. Add a closed-dispatcher state/coordination mechanism
shared by Submit and Shutdown, and return the established structured oops error
for that condition while preserving normal submission and enqueue behavior
before shutdown.
In `@internal/workflow/service.go`:
- Around line 415-424: Update the WorkflowRunEntity construction in the run
creation flow to persist the submitted SourceLimit and OutputLimit values
alongside the existing run metadata. Ensure ExecuteQueued and Resume can read
these stored limits, preserving the limits from both initial submission and
resumed executions.
- Around line 399-403: Update Service.createRun to validate request before
accessing request.SourceVersion, returning the established invalid-request error
for nil input. Preserve the existing behavior for non-nil requests and ensure
Submit(nil) no longer panics.
In `@internal/workflow/workflow.go`:
- Around line 155-158: Replace the source-text check in the workflow result
handling around normalizeWorkflowValue and normalizePipelineResults with an
explicit tagged result kind propagated across the worker boundary. Set the tag
at the actual pipeline result producer, then normalize only when the received
kind identifies a pipeline result, preserving standard normalization for all
other results.
- Around line 474-480: Update runHost.taskResults to propagate host.list errors
instead of converting them to an empty result, then thread that error through
Run so the workflow persists a failed outcome. Adjust the taskResults signature
and all callers to handle the returned error, preserving normal results on
success and ensuring failures are not silently ignored.
- Around line 303-309: Update the invocation identity logic around
normalizeNodeKey and invocationKey so persisted-task lookup no longer depends on
RPC arrival order; derive the key from a stable execution position or propagate
the pipeline index through the workflow. Ensure resumed concurrent callbacks
reuse the persisted task for the matching input or prompt, and add a regression
test covering replay with reordered concurrent callbacks.
---
Outside diff comments:
In `@internal/assistant/runtime_persist.go`:
- Around line 455-484: Update pendingToolIndex so any nonempty CallID is matched
exclusively by CallID: return not found immediately when no pending entry has
that ID, without attempting name or arguments matching. Retain the existing
name-and-arguments and name-only fallback matching only when CallID is empty.
In `@internal/terminal/running_tools.go`:
- Around line 10-24: Update the running-tool removal flow so a non-empty
event.CallID is authoritative: after runningToolBlockIndexByCallID fails, return
without attempting argument or name matching. Keep
runningToolBlockIndexByArguments and runningToolBlockIndexByName as fallbacks
only when event.CallID is empty.
---
Minor comments:
In `@cmd/librecode/prompt.go`:
- Around line 122-127: Update the error handling after writePromptMetrics in the
prompt execution flow to preserve promptErr as the primary failure when both
prompt execution and metrics writing fail, while including metricsErr as
additional context. Keep returning metricsErr alone when promptErr is nil.
In `@internal/executeworker/client.go`:
- Around line 174-177: Update the worker result shutdown flow around
callbacks.Wait and finishResult to pass ctx into finishResult and centralize
termination-error normalization. Preserve cancellation errors after receiving a
result, and retain non-EOF protocol-read errors when waitErr is nil instead of
wrapping a nil error. Apply the same normalization to the related termination
paths identified in finishResult and worker read handling.
In `@internal/terminal/agent_tasks.go`:
- Around line 266-269: Update the failure hint in the completion message within
the agent task flow to reference the registered /workflows command instead of
/workflow. Keep the existing formatting and all other message content unchanged.
In `@internal/workflow/service.go`:
- Around line 77-80: Update NewService to call uuid.NewV7 without uuid.Must,
capture and return any UUID generation error wrapped with the existing
oops.In("workflow") pattern, and only construct the Service after successful
UUID creation.
---
Nitpick comments:
In `@cmd/librecode/workflow_internal_test.go`:
- Around line 13-64: Expand the workflow command argument-validation tests
around newWorkflowCmd to use a table-driven test covering submit, worker,
metrics, resume, run, list, get, events, and cancel. For each subcommand, assert
its Use string and validate both accepted and rejected argument counts according
to its contract, preserving the existing coverage while consolidating it into
the table.
In `@cmd/librecode/workflow.go`:
- Around line 390-413: Update the workflow metrics error paths around
service.Get, service.AgentTasks, AgentTask, and the additional failure at the
referenced later location to use oops.In("workflow").Code("...").Wrapf(...)
instead of direct oops.Wrapf calls. Assign a distinct stable code to each
failure while preserving the existing contextual messages and return behavior.
In `@internal/assistant/execute_tool_internal_test.go`:
- Line 6: Replace the test executor’s fmt-based contextual error construction
with the package’s existing oops pattern, using In, Code, and Wrapf with the
appropriate domain and error code; update imports accordingly and preserve the
original message and wrapped cause.
In `@internal/assistant/lifecyclepayload/lifecyclepayload_test.go`:
- Around line 158-181: Update the lifecycle payload test around ToolCallPayload
and ToolResultPayload to use non-default ParentCallID and Sequence values, and
provide a non-empty ToolResult CallID. Add assertions verifying parent_call_id,
sequence, and the result call_id are propagated in the generated payloads, while
preserving the existing name, error, and is_error checks.
In `@internal/assistant/llm_conversion_internal_test.go`:
- Around line 122-143: Update the ToolEvents fixtures in the relevant conversion
test to use nonzero CallID, ParentCallID, and Sequence values, then assert that
conversion preserves them in the resulting tool-result ID and metadata. Cover
both tool events as needed so the test fails if any new identity field is
dropped.
In `@internal/assistant/tool_lifecycle_test.go`:
- Around line 58-60: Update the lifecycle tests around the metadata setup and
Lua handler assertions to use nonempty ParentCallID and CallID values plus a
positive Sequence, then verify the Lua handler receives those exact propagated
values. Apply the same identity assertions to the additional test case
referenced by the comment.
In `@internal/database/agent_task_repository_test.go`:
- Around line 91-105: The CreateWithChildSession tests currently cover failures
before child-session insertion but not rollback after insertion. Refactor the
failure cases around CreateWithChildSession into table-driven subtests, adding a
valid owner with a nonexistent UUIDv7 ParentTaskID so insertTask fails after
insertSession; assert each case returns the expected error and that
ListChildSessions still reports the original count.
In `@internal/mvmhost/host.go`:
- Around line 168-170: The contextual oops wrappers in the MVM evaluation and
output-capture error paths need stable error codes. Update the wrappers around
machine.Eval and the output-capture handling to use
oops.In("mvmhost").Code("mvm_eval") and
oops.In("mvmhost").Code("output_capture") respectively before Wrapf, preserving
their existing messages and error flow.
In `@internal/terminal/workflow_commands_internal_test.go`:
- Around line 22-68: Add table-driven coverage for the workflow inspection
behavior using workflowInspectorStub, including a foreign-session run and
injected errors from Get, Events, and AgentTasks. Extend the stub with
configurable errors for those methods, return them when set, and assert each
command path reports the expected failure or ownership result while preserving
the existing List validation.
In `@internal/workflow/workflow.go`:
- Around line 294-300: Update the workflow validation errors in the function
containing the prompt check and the additional validation at the referenced
later block to use the package’s established samber/oops pattern, including the
workflow domain, stable error code, and contextual message via Wrapf; replace
the raw errors.New returns while preserving the existing validation behavior.
- Around line 361-363: Extract the duplicated “task is not owned by this
workflow session” text into a single package-level constant in the workflow
package, then update the ownership error paths in the surrounding workflow
logic—including the branches at the referenced locations—to reuse that constant
while preserving their existing error codes and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d9c4a14-9698-4a53-bbc1-695382cc8c7b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (119)
cmd/librecode/chat.gocmd/librecode/main.gocmd/librecode/prompt.gocmd/librecode/prompt_internal_test.gocmd/librecode/root.gocmd/librecode/workflow.gocmd/librecode/workflow_internal_test.gogo.modinternal/agenttask/runtime_runner.gointernal/agenttask/service.gointernal/agenttask/service_internal_test.gointernal/agenttask/service_test.gointernal/assistant/agent_runtime_wrappers_internal_test.gointernal/assistant/agent_submitter.gointernal/assistant/agent_submitter_internal_test.gointernal/assistant/agent_tool.gointernal/assistant/agent_tool_internal_test.gointernal/assistant/catalog_testmain_test.gointernal/assistant/client.gointernal/assistant/client_adapter.gointernal/assistant/client_adapter_internal_test.gointernal/assistant/context_auto_compaction.gointernal/assistant/execute_tool.gointernal/assistant/execute_tool_internal_test.gointernal/assistant/lifecycle.gointernal/assistant/lifecyclepayload/lifecyclepayload.gointernal/assistant/lifecyclepayload/lifecyclepayload_test.gointernal/assistant/llm_conversion_internal_test.gointernal/assistant/provider_hook_test_helpers_internal_test.gointernal/assistant/run_metrics.gointernal/assistant/run_metrics_internal_test.gointernal/assistant/runtime.gointernal/assistant/runtime_events.gointernal/assistant/runtime_lifecycle_test.gointernal/assistant/runtime_persist.gointernal/assistant/runtime_persist_internal_test.gointernal/assistant/runtime_slash.gointernal/assistant/runtime_test.gointernal/assistant/tool_arguments_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/assistant/workflow_controller.gointernal/assistant/workflow_tool.gointernal/assistant/workflow_tool_internal_test.gointernal/database/agent_task_repository.gointernal/database/agent_task_repository_test.gointernal/database/migrations/00008_create_workflow_runs.sqlinternal/database/migrations/00009_add_workflow_invocation_index.sqlinternal/database/migrations/00010_add_workflow_name.sqlinternal/database/migrations_test.gointernal/database/repository_helpers.gointernal/database/session_repository.gointernal/database/task_repository.gointernal/database/task_test_helpers_test.gointernal/database/validation.gointernal/database/workflow_repository.gointernal/database/workflow_repository_test.gointernal/di/agent_task_service.gointernal/di/assistant_service_internal_test.gointernal/di/chat_workflow_service.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/workflow_service.gointernal/executeworker/client.gointernal/executeworker/client_test.gointernal/executeworker/protocol.gointernal/executeworker/protocol_test.gointernal/executeworker/worker.gointernal/mvmhost/host.gointernal/mvmhost/host_test.gointernal/provider/anthropic.gointernal/provider/openai_chat.gointernal/provider/openai_responses.gointernal/provider/usage.gointernal/provider/usage_merge_internal_test.gointernal/terminal/agent_tasks.gointernal/terminal/agent_tasks_behavior_internal_test.gointernal/terminal/app.gointernal/terminal/async_events.gointernal/terminal/async_events_internal_test.gointernal/terminal/autocomplete.gointernal/terminal/autocomplete_internal_test.gointernal/terminal/bench_terminal_internal_test.gointernal/terminal/commands.gointernal/terminal/commands_internal_test.gointernal/terminal/message_layout.gointernal/terminal/prompt_response_internal_test.gointernal/terminal/render_internal_test.gointernal/terminal/render_parity_internal_test.gointernal/terminal/running_tools.gointernal/terminal/running_tools_internal_test.gointernal/terminal/token_usage_behavior_internal_test.gointernal/terminal/token_usage_export_test.gointernal/terminal/tool_blocks.gointernal/terminal/tool_display.gointernal/terminal/tool_display_internal_test.gointernal/terminal/workflow_commands.gointernal/terminal/workflow_commands_internal_test.gointernal/terminal/workflow_summary_internal_test.gointernal/tool/registry.gointernal/tool/registry_test.gointernal/transcript/tool_event.gointernal/transcript/tool_event_test.gointernal/workflow/dispatcher.gointernal/workflow/dispatcher_cross_process_test.gointernal/workflow/dispatcher_test.gointernal/workflow/pipeline.gointernal/workflow/service.gointernal/workflow/service_test.gointernal/workflow/worker_testmain_test.gointernal/workflow/workflow.gointernal/workflow/workflow_test.go
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/workflow/service.go (1)
237-239: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist the terminal outcome with a non-canceled context.
CancelcancelsrunCtx, thenfinishuses that same context. Database writes can therefore fail immediately, leaving durable runs stuck incancelingor otherwise non-terminal. Lease-monitor cancellation has the same failure mode.Proposed fix
- if finishErr := service.finish(runCtx, run.Task.ID, &result, runErr); finishErr != nil { + finishCtx := context.WithoutCancel(runCtx) + if finishErr := service.finish(finishCtx, run.Task.ID, &result, runErr); finishErr != nil { return run, &result, errors.Join(runErr, finishErr) }🤖 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/workflow/service.go` around lines 237 - 239, Update the terminal persistence call in the workflow execution path to invoke service.finish with a non-canceled context rather than runCtx. Preserve the existing run.Task.ID, result, runErr, error-joining, and return behavior while ensuring cancellation from Cancel or lease monitoring cannot prevent finish from recording the terminal outcome.
🤖 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/executeworker/coverage_internal_test.go`:
- Line 60: Handle the returned errors from both buffer writes in the test setup
around truncated: check and propagate the errors from WriteByte and WriteString
so errcheck passes, while preserving the existing data construction behavior.
In `@internal/workflow/dispatcher_test.go`:
- Around line 97-110: Replace the fixed time.Sleep in the concurrency test with
observable synchronization: capture the initial connection.Stats().WaitCount
before starting Submit, then use require.Eventually to wait until WaitCount
increases, confirming the submission is blocked waiting for the database
connection before calling Shutdown.
---
Outside diff comments:
In `@internal/workflow/service.go`:
- Around line 237-239: Update the terminal persistence call in the workflow
execution path to invoke service.finish with a non-canceled context rather than
runCtx. Preserve the existing run.Task.ID, result, runErr, error-joining, and
return behavior while ensuring cancellation from Cancel or lease monitoring
cannot prevent finish from recording the terminal outcome.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 67a0d51f-3342-49da-b12b-d05b133044e4
📒 Files selected for processing (52)
cmd/librecode/prompt.gointernal/agenttask/runtime_runner.gointernal/agenttask/runtime_runner_internal_test.gointernal/agenttask/service.gointernal/agenttask/service_test.gointernal/assistant/execute_tool.gointernal/assistant/execute_tool_internal_test.gointernal/assistant/lifecyclepayload/lifecyclepayload_test.gointernal/assistant/llm_conversion.gointernal/assistant/llm_conversion_internal_test.gointernal/assistant/run_metrics_internal_test.gointernal/assistant/runtime_persist.gointernal/assistant/runtime_persist_internal_test.gointernal/assistant/tool_executor.gointernal/assistant/tool_executor_internal_test.gointernal/assistant/tool_lifecycle_test.gointernal/assistant/workflow_controller_internal_test.gointernal/assistant/workflow_tool.gointernal/assistant/workflow_tool_internal_test.gointernal/database/agent_task_repository.gointernal/database/agent_task_repository_test.gointernal/database/validation.gointernal/database/workflow_repository.gointernal/database/workflow_repository_test.gointernal/di/service_constructors_internal_test.gointernal/executeworker/client.gointernal/executeworker/coverage_internal_test.gointernal/executeworker/protocol.gointernal/executeworker/protocol_test.gointernal/executeworker/worker.gointernal/executionlimits/limits.gointernal/mvmhost/host.gointernal/mvmhost/host_test.gointernal/terminal/agent_tasks.gointernal/terminal/agent_tasks_behavior_internal_test.gointernal/terminal/app.gointernal/terminal/autocomplete.gointernal/terminal/autocomplete_internal_test.gointernal/terminal/prompt_cancel_internal_test.gointernal/terminal/running_tools.gointernal/terminal/running_tools_internal_test.gointernal/terminal/workflow_summary_internal_test.gointernal/terminal/workflow_test_helpers_internal_test.gointernal/workflow/dispatcher.gointernal/workflow/dispatcher_cross_process_test.gointernal/workflow/dispatcher_test.gointernal/workflow/service.gointernal/workflow/service_internal_test.gointernal/workflow/service_test.gointernal/workflow/workflow.gointernal/workflow/workflow_internal_test.gointernal/workflow/workflow_test.go
💤 Files with no reviewable changes (1)
- internal/terminal/autocomplete.go
🚧 Files skipped from review as they are similar to previous changes (28)
- internal/assistant/llm_conversion_internal_test.go
- internal/workflow/dispatcher_cross_process_test.go
- internal/di/service_constructors_internal_test.go
- internal/terminal/running_tools.go
- internal/assistant/lifecyclepayload/lifecyclepayload_test.go
- internal/executeworker/protocol_test.go
- internal/database/validation.go
- internal/assistant/workflow_tool.go
- internal/terminal/app.go
- internal/terminal/running_tools_internal_test.go
- internal/workflow/dispatcher.go
- cmd/librecode/prompt.go
- internal/database/agent_task_repository.go
- internal/assistant/tool_executor_internal_test.go
- internal/agenttask/service_test.go
- internal/assistant/runtime_persist.go
- internal/assistant/tool_executor.go
- internal/assistant/runtime_persist_internal_test.go
- internal/database/workflow_repository.go
- internal/executeworker/worker.go
- internal/agenttask/runtime_runner.go
- internal/workflow/workflow_test.go
- internal/assistant/tool_lifecycle_test.go
- internal/assistant/execute_tool.go
- internal/executeworker/protocol.go
- internal/workflow/workflow.go
- internal/agenttask/service.go
- internal/terminal/agent_tasks.go
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/database/workflow_repository.go (1)
216-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGroup persistence inputs into a request struct.
persistWorkflowAgentTaskhas eight parameters and exceeds the configured SonarCloud threshold. Bundle the workflow/task-specific inputs into a private request struct.🤖 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/workflow_repository.go` around lines 216 - 225, Introduce a private request struct for the workflow/task-specific inputs and update persistWorkflowAgentTask to accept that struct alongside the existing context and transaction dependencies, reducing its parameter count below the SonarCloud threshold. Update all call sites to construct and pass the request while preserving the current values and persistence behavior.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@internal/database/workflow_repository.go`:
- Around line 216-225: Introduce a private request struct for the
workflow/task-specific inputs and update persistWorkflowAgentTask to accept that
struct alongside the existing context and transaction dependencies, reducing its
parameter count below the SonarCloud threshold. Update all call sites to
construct and pass the request while preserving the current values and
persistence behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: adf7935e-f84f-470b-8169-a5d7e9dc4ab1
📒 Files selected for processing (16)
internal/assistant/workflow_tool.gointernal/assistant/workflow_tool_internal_test.gointernal/database/workflow_repository.gointernal/executeworker/client.gointernal/executeworker/coverage_internal_test.gointernal/executeworker/protocol.gointernal/terminal/tool_blocks.gointernal/terminal/tool_display_internal_test.gointernal/workflow/dispatcher_cross_process_test.gointernal/workflow/dispatcher_test.gointernal/workflow/service.gointernal/workflow/service_internal_test.gointernal/workflow/service_test.gointernal/workflow/workflow.gointernal/workflow/workflow_internal_test.gointernal/workflow/workflow_test.go
🚧 Files skipped from review as they are similar to previous changes (14)
- internal/workflow/dispatcher_cross_process_test.go
- internal/workflow/dispatcher_test.go
- internal/terminal/tool_display_internal_test.go
- internal/assistant/workflow_tool.go
- internal/workflow/service_internal_test.go
- internal/assistant/workflow_tool_internal_test.go
- internal/executeworker/coverage_internal_test.go
- internal/executeworker/protocol.go
- internal/workflow/service_test.go
- internal/workflow/workflow.go
- internal/executeworker/client.go
- internal/workflow/workflow_internal_test.go
- internal/workflow/workflow_test.go
- internal/workflow/service.go
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.