feat: add Go headless protocol sessions#60
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 (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis PR adds a stream-json protocol for structured agent execution, session management for resuming/forking prior executions, comprehensive CLI argument parsing with tool allow/deny filters, and agent-level tool filtering enforced at both advertisement and execution time. The CLI exec command is refactored to support multiple input/output formats, sessions hooks, and event-driven output. ChangesStructured exec runtime and persistence flow
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as zero exec
participant Session as sessions.Store
participant AgentRun as agent.Run
participant Writer as execEventWriter
participant stdout as stdout/stream-json
User->>CLI: zero exec --resume --enabled-tools read_file
CLI->>Session: Load(resumeId)
Session-->>CLI: Prior session + events
CLI->>CLI: FormatExecPrompt (with context)
CLI->>AgentRun: Run(formatted prompt, options)
AgentRun->>Writer: runStart(metadata)
Writer-->>stdout: run_start event
AgentRun->>Writer: text("Processing...")
Writer-->>stdout: text event
AgentRun->>Writer: toolCall(read_file)
Writer-->>stdout: tool_call event
AgentRun->>Writer: toolResult(file contents)
Writer-->>stdout: tool_result event
AgentRun->>Writer: usage(tokens)
Writer-->>stdout: usage event
AgentRun-->>CLI: final answer
CLI->>Session: AppendEvent(user_message, tool_calls, final)
Session-->>CLI: events persisted
CLI->>Writer: final(answer)
Writer-->>stdout: final event
CLI->>Writer: runEnd(success)
Writer-->>stdout: run_end event
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 6
🧹 Nitpick comments (2)
internal/agent/loop_test.go (1)
193-228: ⚡ Quick winAdd explicit allowlist rejection coverage.
Line 193 currently validates the denylist rejection path; add a companion test where
EnabledToolsexcludes the called tool (without relying onDisabledTools) to lock in allowlist semantics too.Proposed test addition
+func TestRunRejectsToolCallsOutsideEnabledList(t *testing.T) { + root := t.TempDir() + registry := tools.NewRegistry() + registry.Register(tools.NewReadFileTool(root)) + provider := &mockProvider{ + turns: [][]zeroruntime.StreamEvent{ + { + {Type: zeroruntime.StreamEventToolCallStart, ToolCallID: "call_1", ToolName: "read_file"}, + {Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "call_1", ArgumentsFragment: `{"path":"README.md"}`}, + {Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: "call_1"}, + {Type: zeroruntime.StreamEventDone}, + }, + { + {Type: zeroruntime.StreamEventText, Content: "done"}, + {Type: zeroruntime.StreamEventDone}, + }, + }, + } + + result, err := Run(context.Background(), "read", provider, Options{ + Registry: registry, + EnabledTools: []string{"grep"}, + MaxTurns: 2, + }) + if err != nil { + t.Fatal(err) + } + if result.FinalAnswer != "done" { + t.Fatalf("expected final answer after filtered tool result, got %q", result.FinalAnswer) + } + lastMessage := result.Messages[len(result.Messages)-2] + if !strings.Contains(lastMessage.Content, "not enabled") { + t.Fatalf("expected filtered tool error message, got %#v", result.Messages) + } +}🤖 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/agent/loop_test.go` around lines 193 - 228, Add a companion test to cover allowlist (EnabledTools) rejection: create a new test (e.g., TestRunRejectsFilteredToolCalls_Allowlist) mirroring TestRunRejectsFilteredToolCalls but call Run with Options that set EnabledTools to a list that does NOT include "read_file" (instead of using DisabledTools), keep the same provider/registry fixture and assertions (expect FinalAnswer "done" and that the second-to-last message contains "not enabled"); ensure the test references Run, Options.EnabledTools, and the registered ReadFile tool so the allowlist rejection path is exercised.internal/cli/exec_tools.go (1)
81-114: 🏗️ Heavy liftConsolidate tool-visibility policy with agent runtime logic.
Lines 99-114 duplicate filtering/advertisement policy that also exists in
internal/agent/loop.go; extracting shared policy helpers would prevent future drift between--list-toolsvisibility and actual runtime enforcement.🤖 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/exec_tools.go` around lines 81 - 114, Refactor visibleExecTools to reuse the agent runtime policy helpers instead of duplicating logic: remove the local implementations of execToolAllowedByFilters and execToolAdvertised and call the shared policy functions (the ones used by internal/agent/loop.go) from visibleExecTools while preserving the same checks on tool.Name(), tool.Safety().Permission, agent.PermissionMode (including agent.PermissionModeAuto) and the enabledTools/disabledTools filters; ensure references to tools.PermissionDeny and tools.PermissionAllow and the tool.Safety().Permission check are delegated to the shared helpers so list-tools visibility and runtime enforcement remain consistent.
🤖 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/exec_parse.go`:
- Around line 195-199: The nextFlagValue function currently treats any non-empty
next token as a value, so flag-shaped tokens (e.g., "--other") get consumed;
update nextFlagValue to consider tokens starting with "-" as missing values:
after computing next := strings.TrimSpace(args[index+1]) check if next == "" or
strings.HasPrefix(next, "-") and in that case return
execUsageError(fmt.Sprintf("%s requires a value", flag)) without advancing the
index; otherwise return next and index+1 as before. Ensure you still trim the
value and keep using execUsageError for the error path.
In `@internal/cli/exec_writer.go`:
- Around line 97-121: In execEventWriter.toolResult, the stream-json path always
sets truncated := false so large result.Outputs are sent whole; change this to
detect output size (e.g., >10KB), set truncated=true when truncating, and send a
truncated string instead of the full result.Output in the streamjson.Event; use
the existing truncateForStatus (or a new helper) to produce the truncated
payload and assign it to Event.Output while setting Event.Truncated to true so
consumers know the payload was shortened.
In `@internal/sessions/exec_session.go`:
- Around line 79-90: Trim/normalize the Resume value before deciding resume
behavior so whitespace-only values don't trigger latest-session resume: call
strings.TrimSpace on options.Resume into a local variable (or assign back to
options.Resume) and then use that trimmed value in the branch that checks
options.Resume != "" || options.ResumeLatest; if the trimmed resume ID is empty
and options.ResumeLatest is true, call store.Latest() and handle nil/error as
now (return PreparedExec{} / ExecError), otherwise use the trimmed sessionID for
resuming; update references to sessionID and the existing ExecError/PreparedExec
handling accordingly.
In `@internal/sessions/store.go`:
- Around line 290-327: AppendEvent is racy: it reads metadata
(Store.readMetadata), computes sequence, writes event file (Store.eventsPath)
and then updates metadata (Store.writeMetadata) without synchronization,
allowing concurrent AppendEvent calls to assign duplicate sequence/IDs and
corrupt event counts; fix by introducing a synchronization guard around the
critical region (e.g. a per-session mutex map in Store or a global Store mutex)
and acquire the lock at the start of AppendEvent before readMetadata, sequence
assignment, event file write and metadata update, releasing it after
writeMetadata completes; if cross-process concurrency is possible also add an
OS-level file lock on eventsPath while appending to ensure inter-process
serialization.
- Line 130: Replace capitalized error messages in the fmt.Errorf calls with
lowercase strings to satisfy ST1005; e.g., change fmt.Errorf("Invalid Zero
session id") to fmt.Errorf("invalid zero session id") and similarly update "Zero
session already exists: %s", "Zero session not found: %s", and "Invalid JSON in
Zero session %s %s at line %d" to "zero session already exists: %s", "zero
session not found: %s", and "invalid json in zero session %s %s at line %d"
respectively—preserve all formatting verbs and the surrounding return of
Metadata{} or other error returns so only the string literals are modified.
In `@internal/streamjson/streamjson.go`:
- Around line 79-85: CreateRunID currently falls back to predictable time bytes
when crypto/rand.Read fails; remove that weak fallback and fail fast instead by
changing CreateRunID to return (string, error), call rand.Read(random) and if it
returns an error return "", error (no time-based bytes), then build the run ID
as before using now.UTC().Format and hex.EncodeToString(random); update callers
to handle the error accordingly so unpredictable IDs are not synthesized from
predictable time values.
---
Nitpick comments:
In `@internal/agent/loop_test.go`:
- Around line 193-228: Add a companion test to cover allowlist (EnabledTools)
rejection: create a new test (e.g., TestRunRejectsFilteredToolCalls_Allowlist)
mirroring TestRunRejectsFilteredToolCalls but call Run with Options that set
EnabledTools to a list that does NOT include "read_file" (instead of using
DisabledTools), keep the same provider/registry fixture and assertions (expect
FinalAnswer "done" and that the second-to-last message contains "not enabled");
ensure the test references Run, Options.EnabledTools, and the registered
ReadFile tool so the allowlist rejection path is exercised.
In `@internal/cli/exec_tools.go`:
- Around line 81-114: Refactor visibleExecTools to reuse the agent runtime
policy helpers instead of duplicating logic: remove the local implementations of
execToolAllowedByFilters and execToolAdvertised and call the shared policy
functions (the ones used by internal/agent/loop.go) from visibleExecTools while
preserving the same checks on tool.Name(), tool.Safety().Permission,
agent.PermissionMode (including agent.PermissionModeAuto) and the
enabledTools/disabledTools filters; ensure references to tools.PermissionDeny
and tools.PermissionAllow and the tool.Safety().Permission check are delegated
to the shared helpers so list-tools visibility and runtime enforcement remain
consistent.
🪄 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: ce22fed1-b5be-4062-bbfb-f5fd3b9545b8
📒 Files selected for processing (15)
internal/agent/loop.gointernal/agent/loop_test.gointernal/agent/types.gointernal/cli/app.gointernal/cli/exec.gointernal/cli/exec_parse.gointernal/cli/exec_protocol_test.gointernal/cli/exec_sessions.gointernal/cli/exec_tools.gointernal/cli/exec_writer.gointernal/sessions/exec_session.gointernal/sessions/store.gointernal/sessions/store_test.gointernal/streamjson/streamjson.gointernal/streamjson/streamjson_test.go
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Findings
-
[P1]
--output-format stream-jsonis not honored for pre-runtime responses. Ininternal/cli/exec.go:87-99, the early--list-tools, session preflight, and prompt/input validation exits bypass the stream-json writer and either call the text tool-list formatter orwriteExecUsageError(stderr, ...). That means a headless client that requested JSONL can receive plain text instead. I reproduced this locally:printf '{bad\n' | ./zero exec --input-format stream-json --output-format stream-jsonexits 2 with empty stdout and[zero] Invalid stream-json input...on stderr, and./zero exec --list-tools --output-format stream-jsonprintsTools visible to model:text to stdout. The protocol docs say errors are part of the protocol and followed byrun_end, so these paths should emit stream-jsonerror+run_endfor usage failures and JSONL output for list-tools when that output format is selected. -
[P2]
run_start.apiModelandproviderare using profile fields instead of resolved runtime metadata. Ininternal/cli/exec_writer.go:35-44, the stream-json event sets bothModelandAPIModelfromprovider.Model, and setsProviderfromprovider.Name. For registry aliases and profile names this can diverge from what actually gets sent byproviders.New: for example an alias likesonnet-4.5resolves to an Anthropic API model, and an OpenAI-compatible profile namedlocalis not the runtime provider kind. Stream-json clients userun_startfor diagnostics/routing, so please plumb the resolved provider kind and API model into the writer instead of echoing the config profile fields.
I also checked CodeRabbit's comments; the nextFlagValue flag-shaped-token issue and the sessions.Store.AppendEvent read/append/metadata race are still valid and worth addressing in the same pass.
Validation:
go test ./...passed locally on rerun.- Direct repros above confirm the stream-json pre-runtime output issue.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cli/exec_protocol_test.go (1)
103-105: ⚡ Quick winStrengthen stream-json tool-list filtering assertion.
This test only verifies inclusion. Add an exclusion check so it fails if non-enabled tools leak into the stream-json output.
Proposed test hardening
if text, _ := events[1]["text"].(string); !strings.Contains(text, "Tools visible to model") || !strings.Contains(text, "read_file") { t.Fatalf("expected final event to contain tool list, got %#v", events[1]) } + if text, _ := events[1]["text"].(string); strings.Contains(text, "bash") { + t.Fatalf("expected filtered tool list to hide bash, got %#v", events[1]) + }🤖 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/exec_protocol_test.go` around lines 103 - 105, The test currently only asserts that events[1]["text"] includes "Tools visible to model" and "read_file"; strengthen it by also asserting that any non-enabled tool names do NOT appear in that same text. After extracting text from events[1] (as done now), create a slice of disabled/non-enabled tool names (e.g., disabled := []string{"write_file", "delete_file"} or the actual non-enabled tools for this test) and loop over it, using if strings.Contains(text, name) { t.Fatalf("unexpected disabled tool %q leaked into stream-json output: %v", name, events[1]) } so the test fails if any disabled tool is present.
🤖 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/cli/exec_protocol_test.go`:
- Around line 103-105: The test currently only asserts that events[1]["text"]
includes "Tools visible to model" and "read_file"; strengthen it by also
asserting that any non-enabled tool names do NOT appear in that same text. After
extracting text from events[1] (as done now), create a slice of
disabled/non-enabled tool names (e.g., disabled := []string{"write_file",
"delete_file"} or the actual non-enabled tools for this test) and loop over it,
using if strings.Contains(text, name) { t.Fatalf("unexpected disabled tool %q
leaked into stream-json output: %v", name, events[1]) } so the test fails if any
disabled tool is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f1e1e2a7-4142-470c-96a8-4260435f65fe
📒 Files selected for processing (14)
internal/agent/loop.gointernal/agent/loop_test.gointernal/cli/app.gointernal/cli/exec.gointernal/cli/exec_parse.gointernal/cli/exec_protocol_test.gointernal/cli/exec_tools.gointernal/cli/exec_writer.gointernal/providers/factory.gointernal/sessions/exec_session.gointernal/sessions/store.gointernal/sessions/store_test.gointernal/streamjson/streamjson.gointernal/streamjson/streamjson_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/sessions/store.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/agent/loop_test.go
- internal/cli/app.go
- internal/cli/exec_writer.go
- internal/streamjson/streamjson_test.go
- internal/sessions/exec_session.go
- internal/agent/loop.go
- internal/cli/exec.go
|
@coderabbitai review Pushed @gnanam1990 the earlier requested stream-json/runtime-metadata fixes were addressed in |
|
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Approved after rereviewing current head ed7b998.
Verified the previous requested-change items are addressed:
- stream-json pre-runtime errors now stay in protocol with error + run_end and empty stderr
- stream-json tool listing emits JSONL and preserves enabled-tool filtering
- run_start provider/apiModel now use resolved runtime metadata
- CodeRabbit follow-ups around flag-shaped values, append serialization, truncation, run IDs, and test coverage are addressed
Local validation passed:
- git diff --check origin/main...HEAD
- go test -count=1 ./internal/cli ./internal/sessions ./internal/streamjson ./internal/agent
- go test -count=1 ./...
- bun install --frozen-lockfile
- bun test ./tests --timeout 15000
- bunx tsc --noEmit
- bun run build
- bun run smoke:build
- bun run smoke:go
- direct stream-json repro checks
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/exec_tools.go`:
- Around line 100-107: The helper function toolListContains is dead code and
should be removed; delete the entire toolListContains function declaration (the
function that iterates over values and compares to want) from the codebase to
eliminate unused code, then run the build/test linter (go vet/go test or your
project's lint step) to ensure nothing else depends on it and commit the
removal.
In `@internal/cli/exec_writer.go`:
- Around line 116-125: The stream-json branch builds a streamjson.Event for tool
results but omits the tool name, making events non-self-describing; update the
code in the execOutputStreamJSON branch where
truncateForStreamJSONOutput(result.Output) is used and
writer.writeStreamJSON(...) is called to include Name: result.Name in the
streamjson.Event (the event of Type streamjson.EventToolResult with fields
RunID, ID, Status, Output, Truncated) so consumers can identify the tool from a
single tool_result event.
In `@internal/streamjson/streamjson.go`:
- Around line 228-261: redactValue currently only examines string contents and
misses secrets stored under sensitive object keys; update the map branch inside
redactValue (the case map[string]any) to detect sensitive keys (e.g., "apikey",
"api_key", "api-key", "password", "secret", "token", "access_token", "bearer",
etc.) in a case-insensitive/normalized way and, when a key matches, replace the
corresponding value with a redacted placeholder (e.g., "[REDACTED]") instead of
recursing; keep existing behavior for non-sensitive keys (recursing into
redactValue) and continue using redactString and secretPatterns for string
content redaction.
🪄 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: 48ca4735-a7da-4c23-ac3c-0a7afe04e1b1
📒 Files selected for processing (16)
internal/agent/loop.gointernal/agent/loop_test.gointernal/agent/types.gointernal/cli/app.gointernal/cli/exec.gointernal/cli/exec_parse.gointernal/cli/exec_protocol_test.gointernal/cli/exec_sessions.gointernal/cli/exec_tools.gointernal/cli/exec_writer.gointernal/providers/factory.gointernal/sessions/exec_session.gointernal/sessions/store.gointernal/sessions/store_test.gointernal/streamjson/streamjson.gointernal/streamjson/streamjson_test.go
|
@coderabbitai review Addressed the current actionable comments at head:
Validation passed locally:
|
|
✅ Action performedReview finished.
|
|
@coderabbitai approve |
✅ Action performedComments resolved. Approval is disabled; enable |
|
@coderabbitai approve |
…rotocol # Conflicts: # .coderabbit.yaml
✅ Action performedComments resolved and changes approved. |
2 participants
Summary
zero execwith stream-json input/output,--auto, tool filters,--list-tools,--resume, and--forkTests
git diff --checkbun run typecheckbun test ./tests --timeout 15000bun run test:gobun run buildbun run smoke:buildbun run build:gobun run smoke:go./zero.exe exec --list-tools --enabled-tools read_file,grepSummary by CodeRabbit
Release Notes
--enabled-toolsand--disabled-toolsflags to control agent access to specific tools.--list-toolsflag to display available tools with descriptions.