# 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.