Add Go M0 agent runtime#51
Conversation
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds an agent runtime (streaming collection, ordered tool-call extraction and execution with permission modes, message-copying) with unit tests, and integrates it into the CLI as a new ChangesAgent system and CLI exec integration
Sequence Diagram(s)sequenceDiagram
participant Client
participant Run
participant collectTurn
participant Provider
participant executeToolCall
participant Registry
Client->>Run: Run(ctx,prompt,provider,options)
Run->>Provider: StreamCompletion(CompletionRequest with Messages+Tools)
Provider->>collectTurn: StreamEvent stream
collectTurn-->>Run: assistant text + ordered ToolCalls
Run->>Run: append assistant Message (ToolCalls)
alt Has tool calls
Run->>executeToolCall: execute ToolCall
executeToolCall->>Registry: RunWithOptions(permission)
Registry-->>executeToolCall: ToolResult(status,output)
executeToolCall-->>Run: ToolResult
Run->>Run: append RoleTool message with ToolCallID
Run->>Provider: next turn (stream)
else No tool calls
Run-->>Client: Result{FinalAnswer}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/cli/app_test.go (1)
95-110: ⚡ Quick winAdd exec tests for quoted-empty and whitespace-only prompts.
Current coverage only checks missing args; it should also assert rejection for
Run([]string{"exec", ""}, ...)and whitespace-only prompt input.🧪 Suggested test extension
func TestRunExecRequiresPrompt(t *testing.T) { var stdout bytes.Buffer var stderr bytes.Buffer exitCode := Run([]string{"exec"}, &stdout, &stderr) if exitCode != 2 { t.Fatalf("expected exit code 2, got %d", exitCode) } if stdout.Len() != 0 { t.Fatalf("expected empty stdout, got %q", stdout.String()) } if !strings.Contains(stderr.String(), "Prompt required") { t.Fatalf("expected prompt error, got %q", stderr.String()) } } + +func TestRunExecRejectsEmptyOrWhitespacePrompt(t *testing.T) { + for _, args := range [][]string{ + {"exec", ""}, + {"exec", " "}, + } { + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := Run(args, &stdout, &stderr) + if exitCode != 2 { + t.Fatalf("expected exit code 2 for args %#v, got %d", args, exitCode) + } + if stdout.Len() != 0 { + t.Fatalf("expected empty stdout for args %#v, got %q", args, stdout.String()) + } + if !strings.Contains(stderr.String(), "Prompt required") { + t.Fatalf("expected prompt error for args %#v, got %q", args, stderr.String()) + } + } +}🤖 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/cli/app_test.go` around lines 95 - 110, Extend TestRunExecRequiresPrompt to also call Run with Run([]string{"exec", ""}, &stdout, &stderr) and Run([]string{"exec", " "}, &stdout, &stderr), verifying each returns exit code 2, produces empty stdout, and writes a "Prompt required" error to stderr; you can add them as separate subtests or sequential assertions within TestRunExecRequiresPrompt and reuse the stdout/stderr buffers (resetting them between calls) to locate the behavior around the Run function.
🤖 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/cli/app.go`:
- Around line 69-74: The current arg-check only verifies len(args) and allows
empty or whitespace-only prompts (e.g., `zero exec ""`) to proceed; update the
validation where args is inspected (the block using fmt.Fprintln(stderr, "Prompt
required...") and the similar block at the later check) to trim whitespace from
the joined or first argument (strings.TrimSpace) and treat an empty result as
missing input: write the same error to stderr and return the same exit code(s)
as present now so whitespace-only or empty prompts are rejected before runtime
invocation.
---
Nitpick comments:
In `@internal/cli/app_test.go`:
- Around line 95-110: Extend TestRunExecRequiresPrompt to also call Run with
Run([]string{"exec", ""}, &stdout, &stderr) and Run([]string{"exec", " "},
&stdout, &stderr), verifying each returns exit code 2, produces empty stdout,
and writes a "Prompt required" error to stderr; you can add them as separate
subtests or sequential assertions within TestRunExecRequiresPrompt and reuse the
stdout/stderr buffers (resetting them between calls) to locate the behavior
around the Run function.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c81e4df-ab26-40af-924f-c67dcb470fa6
📒 Files selected for processing (5)
internal/agent/loop.gointernal/agent/loop_test.gointernal/agent/types.gointernal/cli/app.gointernal/cli/app_test.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
Vasanthdev2004
dismissed
coderabbitai[bot]’s stale review
Fixed in 7d91a7d: exec now trims prompts and rejects empty or whitespace-only input with tests.
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
What's good
- Clean ReAct spine:
Run(internal/agent/loop.go:23-99) drives amaxTurns-bounded loop, copies the message log defensively on everyCompletionRequest(copyMessages,internal/agent/loop.go:214-224), and returns aResultwithFinalAnswer,Turns, and the full message log. Provider errors and max-turns termination both return the partial message log so a TUI can render what happened. collectTurn(internal/agent/loop.go:103-145) handles every event type correctly:EventTextaccumulates and fans out viaOnText;EventToolCallStart/EventToolCallDeltaregister tool calls in arrival order via theorder []string+pending map[string]*pendingToolCallpair, and a missing Start doesn't break Delta accumulation.EventToolCallEndis consumed but intentionally a no-op (the loop terminates onEventDoneorctx.Done).- Permission model is the right shape:
PermissionModeAutoadvertises onlyPermissionAllowtools viaisAdvertised(internal/agent/loop.go:202-209);PermissionModeAskadvertises everything non-Deny (so the model can call write tools, but the registry'sRunWithOptionswill refuse without a grant);PermissionModeUnsafegrants every tool.executeToolCall(internal/agent/loop.go:172-200) layers an unconditional grant on top ofPermissionAllowtools, so reads always run regardless of mode. - Tool call results are appended to the message log as
RoleToolwith theToolCallID, and the provider sees them on the next turn'sCompletionRequest. JSON parse errors are returned as tool results (not Go errors), which is the right signal for the model — the loop continues and the provider can self-correct. - The
offlineProvider(internal/cli/app.go:118-131) is a clean stub: it picks the most recentRoleUsermessage, echoes it back asEventText, and emitsEventDone. Good for proving the CLI ↔ runtime path end-to-end without pulling in a real provider in this PR. - CLI:
zero execjoins args with a single space, trims, rejects empty / whitespace-only / missing prompts with exit code 2 and aPrompt required.message (internal/cli/app.go:69-83). Tests iterate over the three failure cases viat.Runsubtests, addressing CodeRabbit's nitpick directly. - Test coverage: text deltas, usage events, tool execution through the registry, permission denial in
Askmode, grant inUnsafemode, max-turns termination, and the fullzero exechappy path with the offline provider. CI green on all four matrices; CodeRabbit reports SUCCESS on the latest SHA (7d91a7d).
Observations (non-blocking)
-
System prompt is a hard-coded package constant.
defaultSystemPrompt(internal/agent/loop.go:11) is the only prompt the agent ever sees, andRunbuilds the seed message list with it inline. A real provider will want to inject the workspace path, the available tool list, repo conventions, etc. The minimal fix is aSystemPromptfield onOptions(with a fallback todefaultSystemPrompt); a slightly bigger fix is to let the caller pass a fully-formed seed[]Messageand haveRunonly append the user message. The latter is more flexible and matches how OpenAI-style agents usually do it. -
PermissionModeAutoandPermissionModeAskare currently identical at the runtime level.isAdvertiseddistinguishes them (Auto advertises onlyPermissionAllow; Ask advertises everything non-Deny) andexecuteToolCallgrantsPermissionAllowtools unconditionally, but neither mode grants aPermissionPrompttool — both requirePermissionModeUnsafefor that. The intent is presumably "Ask = interactively prompt the user, Auto = silently deny", with the interactive prompt living in the TUI layer that consumesOnToolCalland chooses to abort vs. re-run withPermissionGranted: true. Worth a one-line comment on thePermissionModeconstants ininternal/agent/types.go:69-75so a future reader doesn't assumeAskshould auto-grant anything, and a follow-up test that assertsPermissionModeAuto+ aPermissionPrompttool returns the samePermission required for ...message we already test forAsk. -
runExecruns the agent oncontext.Background(). No timeout, no signal handling, no--max-durationflag (internal/cli/app.go:97-101). A runaway agent loop or a slow model would tie up the terminal indefinitely;Ctrl+Cwould kill the process but not surface the partialResult. Pairingcontext.WithCancelwith a SIGINT handler that printsresult.FinalAnswerandresult.Turnson interrupt would make the CLI usable for real prompts. -
copyMessagesis called per turn and at every error/return path. For a 12-turn run with a 50-message log, that's 12+ deep copies, each allocating a fresh[]ToolCallper message. Not a perf concern at the volumes involved today, but a single ownedmessagesslice plus a shallowResult.Messagescapture at the end would be cheaper. If you ever streamMessagesto a TUI mid-turn, you'll need a clean ownership story anyway. -
No test for
EventToolCallEndwithout a matchingEventToolCallStartorEventToolCallDelta. The currentcollectTurnis resilient to that (it auto-creates the pending entry on the first event for an id), but the test mock always sends the full Start → Delta → End sequence. A one-line case where onlyEndis sent (and the resulting tool call has empty args) would document the resilience. -
The
OnToolCallcallback is fired beforeexecuteToolCall, butOnToolResultonly fires after the synchronous execution completes. A tool that takes 30 seconds (e.g.bashwith a long timeout) would block the UI from updating for the full duration. If the TUI wants a "running" indicator, the runtime would need to either expose anOnToolStartchannel that the tool's goroutine signals, or move tool execution into a goroutine. For v1 with the TUI still ahead, fine; flag it now so the design choice is conscious. -
runExecjoins args with a single space.prompt := strings.TrimSpace(strings.Join(args, " "))(internal/cli/app.go:85) loses the user's whitespace, sozero exec "hello world"becomeshello world(after the shell strips the quotes) which happens to be the same in this case, butzero exec 'hello\nworld'would arrive ashello\nworld(literal backslash-n) and any multi-line prompt construction via argv will not survive. A future--prompt-file -or a-stdin convention would round this out. -
offlineProviderignoresrequest.Toolsentirely. That's fine for a stub, but the stub doesn't exercise the tool-advertising path on the provider side — a real provider that mis-handles theToolsslice (e.g. drops a tool, renames a parameter) wouldn't be caught. ThemockProviderinloop_test.godoes receive the fullCompletionRequestand stores it inrequests, so a tool-advertising regression would be caught there; worth a follow-up assertion inTestRunExecutesToolCallThroughRegistrythat the second-turn request includes theread_fileToolDefinition.
No blockers. The runtime is a clean foundation for the real provider; the only change I'd want to see before this lands in front of users is making the system prompt configurable (observation #1). The rest are good follow-ups for when the TUI / real provider land.
2 participants
Summary
Adds the first Go agent runtime spine for M0:
tools.Registry.RunWithOptions, feeds tool results back into provider messages, and stops atMaxTurns.zero exec <prompt>Go CLI command through the new runtime with a deterministic offline provider until the real Go provider lands in the next PR.Tests
git diff --checkgo test ./...bun run build:gobun run smoke:gobun run typecheckbun test ./tests --timeout 15000bun run buildbun run smoke:buildNotes
The Go
zero execprovider is intentionally offline in this PR. It proves the CLI -> agent runtime path without adding the OpenAI-compatible provider in the same review. The next PR should replace that provider stub with real provider/config wiring.Summary by CodeRabbit
New Features
Chores / Tests