bc4787f857
- Implement V2 execution model in the standard executor, simplifying task execution to a single call without validation loops. - Introduce support for resuming suspended executions, allowing for human input during task processing. - Enhance event handling by pushing task completion and failure events to the event bus for better tracking and integration. - Update tests to reflect changes in execution flow and ensure robust handling of task statuses and results.
263 lines
8.3 KiB
Markdown
263 lines
8.3 KiB
Markdown
# DESIGN-V2 Line-by-Line Review Findings
|
||
|
||
**Review Date:** 2026-02-25
|
||
**Files Reviewed:** `runner.go`, `run.go`
|
||
|
||
---
|
||
|
||
## File 1: runner.go
|
||
|
||
### 1. ExecuteTask: Is it truly single-call? No retry loop? No validation?
|
||
|
||
**✅ PASS** — Lines 65–108
|
||
|
||
- Non-assistant: single call to `executeNonAssistantTask` (L74), no loop
|
||
- Assistant: single call to `executeAssistantTask` (L89), no loop
|
||
- No `validator` import or call anywhere in the file
|
||
- Comment at L62–63: "V2 simplified: single call, no validation loop"
|
||
|
||
---
|
||
|
||
### 2. Does it correctly split assistant vs non-assistant at the top?
|
||
|
||
**✅ PASS** — Lines 72–86 vs 88–107
|
||
|
||
- L72: `if task.ExecutorType != robottypes.ExecutorAssistant` — non-assistant branch first
|
||
- L88+: assistant branch follows
|
||
- Clear split at the top of the function
|
||
|
||
---
|
||
|
||
### 3. For non-assistant: does it call executeNonAssistantTask which handles MCP and Process?
|
||
|
||
**✅ PASS** — Lines 74, 110–119
|
||
|
||
- L74: `output, err := r.executeNonAssistantTask(task, taskCtx)`
|
||
- `executeNonAssistantTask` (L110–119): switch on `ExecutorMCP` and `ExecutorProcess`, delegates to `ExecuteMCPTask` and `ExecuteProcessTask`
|
||
|
||
---
|
||
|
||
### 4. For assistant: does it call executeAssistantTask which returns (output, *CallResult, error)?
|
||
|
||
**✅ PASS** — Lines 89, 123–145
|
||
|
||
- L89: `output, callResult, err := r.executeAssistantTask(task, taskCtx)`
|
||
- L123: `func (r *Runner) executeAssistantTask(...) (interface{}, *CallResult, error)`
|
||
- L144: `return output, turnResult.Result, nil`
|
||
|
||
---
|
||
|
||
### 5. Does executeAssistantTask use conv.Turn() (single turn, not multi-turn)?
|
||
|
||
**✅ PASS** — Lines 126, 138
|
||
|
||
- L126: `conv := NewConversation(task.ExecutorID, chatID, 1)` — maxTurns=1
|
||
- L138: `turnResult, err := conv.Turn(r.ctx, input)` — single `Turn` call, no loop
|
||
|
||
---
|
||
|
||
### 6. Does detectNeedMoreInfo properly check result.Next for map with "status" == "need_input"?
|
||
|
||
**✅ PASS** — Lines 149–166
|
||
|
||
- L150–151: nil checks for `result` and `result.Next`
|
||
- L154: type assertion to `map[string]interface{}`
|
||
- L157–159: `status, _ := m["status"].(string); if status != "need_input" return false`
|
||
- Matches DESIGN §16.5 protocol
|
||
|
||
---
|
||
|
||
### 7. Does it extract "question" from the map? What happens if question is empty?
|
||
|
||
**✅ PASS** — Lines 161–165
|
||
|
||
- L161: `question, _ := m["question"].(string)`
|
||
- L163–164: `if question == "" { question = result.GetText() }` — fallback to `CallResult` text
|
||
- Empty question handled via fallback
|
||
|
||
---
|
||
|
||
### 8. Are result.NeedInput and result.InputQuestion set correctly?
|
||
|
||
**✅ PASS** — Lines 102–105
|
||
|
||
- L102–104: `if needInput, question := detectNeedMoreInfo(callResult); needInput { result.NeedInput = true; result.InputQuestion = question }`
|
||
- Set only when `detectNeedMoreInfo` returns true
|
||
|
||
---
|
||
|
||
### 9. Does result.Duration get set in all paths (success and failure)?
|
||
|
||
**✅ PASS** — Lines 77, 84, 92, 98
|
||
|
||
- Non-assistant error: L77
|
||
- Non-assistant success: L84
|
||
- Assistant error: L92
|
||
- Assistant success: L98
|
||
- All paths set `result.Duration = time.Since(startTime).Milliseconds()`
|
||
|
||
---
|
||
|
||
### 10. Does buildResult helper exist or is result construction inline?
|
||
|
||
**✅ PASS (inline)** — Lines 68–107
|
||
|
||
- No `buildResult` helper; construction is inline
|
||
- DESIGN §16.4 pseudocode uses `buildResult`; inline construction is acceptable and used here
|
||
|
||
---
|
||
|
||
### 11. Any edge cases: what if task.ExecutorType is empty or unknown?
|
||
|
||
**✅ PASS** — Lines 72, 112–118
|
||
|
||
- Empty/unknown: `!= ExecutorAssistant` is true → non-assistant branch
|
||
- L116–117: `default` returns `fmt.Errorf("unsupported executor type: %s (expected mcp or process)", task.ExecutorType)`
|
||
- Error returned and propagated; no silent failure
|
||
|
||
---
|
||
|
||
### Additional Finding (runner.go)
|
||
|
||
**⚠️ Minor:** DESIGN §9.1 shows `event.Push("robot.task.failed", ...)` inside `ExecuteTask` when `err != nil`. Implementation pushes `TaskFailed` from `run.go` (L113–120) when `result.Success` is false. Behavior is equivalent; only location differs.
|
||
|
||
---
|
||
|
||
## File 2: run.go
|
||
|
||
### 1. DefaultRunConfig — ContinueOnFailure defaults to true?
|
||
|
||
**✅ PASS** — Lines 21–26
|
||
|
||
- L23–25: `return &RunConfig{ ContinueOnFailure: true }`
|
||
- Matches DESIGN §6.3
|
||
|
||
---
|
||
|
||
### 2. RunExecution — does it check exec.ResumeContext for startIndex and PreviousResults?
|
||
|
||
**✅ PASS** — Lines 60–66
|
||
|
||
- L61–64: `if exec.ResumeContext != nil { startIndex = exec.ResumeContext.TaskIndex; exec.Results = exec.ResumeContext.PreviousResults }`
|
||
- L72: loop starts at `startIndex`
|
||
- Matches DESIGN §9.2, §16.3
|
||
|
||
---
|
||
|
||
### 3. Does it NOT reset Results when ResumeContext is present?
|
||
|
||
**✅ PASS** — Lines 62–65
|
||
|
||
- When `ResumeContext != nil`: `exec.Results = exec.ResumeContext.PreviousResults` — restores, does not reset
|
||
- When `ResumeContext == nil`: `exec.Results = make(...)` — fresh slice
|
||
|
||
---
|
||
|
||
### 4. Does it set task.Status to TaskRunning before execution?
|
||
|
||
**✅ PASS** — Lines 86–89
|
||
|
||
- L87: `task.Status = robottypes.TaskRunning`
|
||
- L88–89: `task.StartTime = &now`
|
||
- Set before `ExecuteTask` (L98)
|
||
|
||
---
|
||
|
||
### 5. Does it call e.updateTasksState to persist running state?
|
||
|
||
**✅ PASS** — Line 92
|
||
|
||
- L92: `e.updateTasksState(ctx, exec)` immediately after setting task status
|
||
- Persists running state before execution
|
||
|
||
---
|
||
|
||
### 6. Does result.NeedInput trigger e.Suspend(ctx, exec, i, result.InputQuestion)?
|
||
|
||
**✅ PASS** — Lines 100–103
|
||
|
||
- L100–102: `if result.NeedInput { return e.Suspend(ctx, exec, i, result.InputQuestion) }`
|
||
- Correct parameters and early return
|
||
|
||
---
|
||
|
||
### 7. Is the result NOT appended before Suspend (avoiding duplicate results per §16.15)?
|
||
|
||
**✅ PASS** — Lines 100–103, 124
|
||
|
||
- L100–102: `NeedInput` branch returns before any append
|
||
- L124: `exec.Results = append(exec.Results, *result)` is after the `NeedInput` check
|
||
- No append on suspend; matches DESIGN §16.15
|
||
|
||
---
|
||
|
||
### 8. Does it push event.Push for TaskFailed when a task fails?
|
||
|
||
**✅ PASS** — Lines 113–120
|
||
|
||
- L113–120: `event.Push(ctx.Context, robotevents.TaskFailed, robotevents.NeedInputPayload{...})` when `!result.Success`
|
||
- Event is pushed on task failure
|
||
|
||
**⚠️ Minor:** Uses `NeedInputPayload` with `Question: result.Error`. DESIGN §7.2 does not define a TaskFailed payload. `ExecPayload` (with `Error`) might be more appropriate; `NeedInputPayload.Question` is reused for the error message. Functionally acceptable but semantically odd.
|
||
|
||
---
|
||
|
||
### 9. Does it skip remaining tasks when ContinueOnFailure is false?
|
||
|
||
**✅ PASS** — Lines 129–137
|
||
|
||
- L129: `if !result.Success && !config.ContinueOnFailure`
|
||
- L131–134: marks remaining tasks as `TaskSkipped`
|
||
- L136: `return fmt.Errorf(...)` — stops execution
|
||
- Matches DESIGN §9.2
|
||
|
||
---
|
||
|
||
### 10. Does it clear exec.Current and exec.ResumeContext after completion?
|
||
|
||
**✅ PASS** — Lines 141–143
|
||
|
||
- L142–143: `exec.Current = nil; exec.ResumeContext = nil` after loop completes
|
||
- Only on normal completion (no early return from Suspend or failure)
|
||
|
||
---
|
||
|
||
### 11. Is there any event.Push for TaskCompleted?
|
||
|
||
**❌ FINDING** — run.go
|
||
|
||
- No `event.Push(robotevents.TaskCompleted, ...)` when a task succeeds
|
||
- DESIGN §7.2 defines `EventTaskCompleted = "robot.task.completed"`
|
||
- DESIGN-V2 §20.4 (B5) notes missing TaskCompleted event constant; implementation also does not push it
|
||
- **Recommendation:** Add `event.Push(ctx.Context, robotevents.TaskCompleted, payload)` when `result.Success` (e.g. after L110)
|
||
|
||
---
|
||
|
||
### 12. Does getRunConfig properly handle nil data?
|
||
|
||
**✅ PASS** — Lines 50–55
|
||
|
||
- No separate `getRunConfig`; config is obtained inline
|
||
- L51–55: `if cfg, ok := data.(*RunConfig); ok && cfg != nil { config = cfg } else { config = DefaultRunConfig() }`
|
||
- Handles: `data == nil`, wrong type, `cfg == nil` → falls back to `DefaultRunConfig()`
|
||
|
||
---
|
||
|
||
### Additional Finding (run.go)
|
||
|
||
**⚠️ Order of operations:** Task status update (L109–120) and result append (L124) occur *after* the NeedInput check. Flow is correct: NeedInput → Suspend (return) → no append, no status update for that task.
|
||
|
||
---
|
||
|
||
## Summary
|
||
|
||
| Category | runner.go | run.go |
|
||
|----------|-----------|--------|
|
||
| **PASS** | 11/11 | 11/12 |
|
||
| **Minor** | 1 | 1 |
|
||
| **Finding** | 0 | 1 (TaskCompleted not pushed) |
|
||
|
||
### Action Items
|
||
|
||
1. **run.go L109–110:** Add `event.Push(ctx.Context, robotevents.TaskCompleted, payload)` when `result.Success` for per-task completion events.
|
||
2. **run.go L113:** Consider introducing a `TaskFailedPayload` (or using `ExecPayload` with `Error`) instead of `NeedInputPayload` for TaskFailed events.
|