feat(agentloop): add Callbacks struct with OnToolStart to agentloop.Run

[birdmanmandbir]

Summary

• Replaces the bare onDelta func(string) parameter in agentloop.Run with a Callbacks struct
• Callbacks has two fields: OnDelta func(string) and OnToolStart func(toolName string)
• OnToolStart fires inside OnToolCall so callers can emit tool status events (e.g. WebSocket tool_start events to iOS clients)
• Struct is nil-safe by default — unset fields are no-ops
• All callers updated: cmd/subagent.go, cmd/explore.go, and all tests in pkg/agentloop/loop_test.go

Why Callbacks struct vs individual params

Run already has 5 params. More callbacks are coming (onToolEnd, onError, onStepComplete) for WebSocket streaming. The struct scales without signature churn and is zero-value safe — callers that don't need new callbacks don't change.

Test plan

• All existing tests pass (make test)
• Build passes (go build ./...)
• Pre-commit hooks pass (fmt, vet, lint)

[birdmanmandbir]

PR Review: feat(agentloop) — Callbacks struct + OnToolStart

Summary

Clean, principled refactor. The Callbacks struct is a correct Open/Closed improvement — adding future hooks requires no signature changes. All nil-safety guards are in place, zero-value Callbacks{} is safe throughout, and all callers were updated consistently.

Important Issues

Missing test for OnToolStart [pkg/agentloop/loop_test.go]

OnToolStart is the headline feature but has no dedicated test. TestRun_ToolCallAndResultCallbacks already has a mock that emits a bash tool call — it just passes Callbacks{}. A regression that passes the wrong field (tc.ToolCallID instead of tc.ToolName), removes the nil guard, or moves the callback into OnToolResult would pass the entire test suite silently.

Suggested addition to the existing tool call test:

var toolsStarted []string
result, err := Run(context.Background(), cfg, nil, "run bash", Callbacks{
    OnToolStart: func(toolName string) {
        toolsStarted = append(toolsStarted, toolName)
    },
})
require.NoError(t, err)
assert.Equal(t, []string{"bash"}, toolsStarted)

Suggestions

• TestRun_ToolCallAndResultCallbacks naming — the test name implies callback behavior is covered, but it uses Callbacks{}. Either rename it to reflect what it actually tests (step accumulation), or populate OnToolStart in it to match the name.

• OnToolStart godoc ordering — the comment says "called when the agent invokes a tool" but doesn't clarify it fires before execution (at tool selection, not tool result). Worth noting for callers building progress UIs: OnToolStart → tool runs → OnToolResult.

Strengths

• Struct-based callbacks is the right API shape — additive, backward-compatible, zero-value safe
• Nil checks on each field are correctly placed and consistent
• Both cmd/explore.go and cmd/subagent.go updated consistently
• Doc comment on Callbacks ("All fields are nil-safe") accurately describes the implementation

VERDICT: LGTM (with suggested test addition)

Blocking on nothing. The OnToolStart test is strongly recommended before merge to lock in the callback contract.

[birdmanmandbir]

Triage Update

Fixed

• Missing OnToolStart test — added TestRun_OnToolStart in dee650f. Uses the existing toolCallThenDoneStream + makeCaptureTool mocks; asserts []string{"capture"} is collected. Catches wrong-field regressions (ToolCallID vs ToolName) and callback placement errors.
• OnToolStart godoc timing — updated in dee650f to clarify it fires at tool selection (before execution), with the ordering: OnToolStart → tool executes → OnToolResult.

Verdict

LGTM — no remaining issues.