CLI wiring: skills/zeroline commands, --mode presets, exec checkpoints, sandbox --effective, sessions rewind - #128
Merged
Merged
Conversation
…s, sandbox --effective, sessions rewind Final module of the runtime-core split. Wires the new runtime features into the cli, PRESERVING main's merged specialist cli (the source branch predates specialist and drops it; this keeps it via a 3-way merge from the common base, adding the reskin features as ours/theirs-only changes). - skills command (internal/cli/skills.go) listing internal/skills; zeroline-skin TUI launcher (internal/cli/zeroline.go) via runInteractiveTUIWithSkin. - exec: --mode presets (smart/deep/fast/large/precise) via applyExecMode; model-registry alias resolution + deprecation/effort notices; ContextWindow sizing (modelContextWindow); before-mutation checkpoint recording in OnToolCall; enhanced tool-result output (ChangedFiles/Redacted/Display); permission-mode validation fix. - sandbox policy --effective (resolved guards); sessions rewind (executes the rewind plan); interactive TUI defaults to Ask mode (advertises write/edit/bash + gates via prompts). - internal/streamjson: EventCheckpoint/EventRestore + CheckpointInfo for structured checkpoint output. Specialist cli preserved (specialist.go + flags + runtime + tests all intact). build/vet/-race/full-suite + GOOS=windows build green; no new deps.
Contributor
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. |
Vasanthdev2004
requested changes
Jun 7, 2026
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed current head 5e28f0e against main.
Validation passed locally:
go test ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smokegit diff --check origin/main...origin/pr-128
GitHub Actions are green. I found one blocker: the new sessions rewind command is implemented and documented, but the parser rejects it before dispatch. Manual repro at the PR head: go run ./cmd/zero sessions rewind demo --sequence 1 prints unknown sessions command "rewind".
Requesting changes for the parser/test fix below. CodeRabbit was still pending while I reviewed.
| return writeExecUsageError(stderr, "sessions rewind-plan requires a session id") | ||
| } | ||
| return runSessionsRewindPlan(store, remaining[0], options, stdout, stderr) | ||
| case "rewind": |
Collaborator
There was a problem hiding this comment.
[P1] Make sessions rewind reachable. This dispatch case is added, and help advertises zero sessions rewind, but parseSessionsArgs only accepts the first positional token if isSessionsCommand returns true. That whitelist currently includes rewind-plan and compact-plan but not rewind, so zero sessions rewind <id> --sequence <n> fails early with unknown sessions command "rewind" and never reaches this case. Please add rewind to the command whitelist and add a regression test that invokes runWithDeps([]string{"sessions", "rewind", ...}) successfully.
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds skills and zeroline CLI commands, refactors interactive TUI (default permission Ask, skin support), implements exec --mode presets with model-registry resolution and reasoning/autonomy validation, captures exec tool-call checkpoints, enriches event/StreamJSON payloads, and adds sandbox policy --effective output. ChangesCLI infrastructure and new commands
Exec --mode preset system and model resolution
Exec session checkpoint capture and event enrichment
Sandbox policy --effective output
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant CLI as runExec
participant Registry as modelRegistry
participant Agent as agent.Run
participant Recorder as execSessionRecorder
participant Writer as execEventWriter
participant Store as SessionStore
User->>CLI: exec --mode <preset> [--model] [--list-tools]
CLI->>CLI: applyExecMode (seed overrides)
CLI->>Registry: resolve selected model (alias/deprecation)
Registry-->>CLI: resolved model + notice
alt --list-tools
CLI->>Agent: request tool list (filters applied)
Agent-->>CLI: tools
CLI->>Writer: format & print tool list
else execute
CLI->>Agent: Run with resolved model, ContextWindow, permissions
Agent->>Recorder: OnToolCall -> captureCheckpoint (if session)
Recorder-->>Store: CaptureToolCheckpoint
Store-->>Recorder: checkpoint event
Recorder-->>Writer: checkpoint(event)
Agent->>Agent: execute tool (may mutate files)
Agent-->>CLI: tool result (redacted/changedFiles/display)
CLI->>Writer: toolResult(event with metadata)
Writer-->>User: StreamJSON/JSON output
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 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: 5
🧹 Nitpick comments (4)
internal/streamjson/streamjson_test.go (1)
196-214: ⚡ Quick winAdd round-trip coverage for restore-specific checkpoint fields.
Line 196 currently validates checkpoint serialization only. Please add a companion test for
EventRestorethat assertsfilesRestored,filesDeleted, andskippedsurvive marshal/unmarshal and remain omitted on bare events.🤖 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/streamjson/streamjson_test.go` around lines 196 - 214, Add a companion unit test to TestEventRoundTripsCheckpointInfo that creates an Event with Type EventRestore and a Checkpoint containing the restore-specific fields filesRestored, filesDeleted, and skipped, marshal/unmarshal it and assert those fields survive (e.g., check lengths/values), then also marshal a bare Event{SchemaVersion:1, Type:EventText} and assert the bare JSON does not contain the "checkpoint" field; put this logic in a new test function (e.g., TestEventRoundTripsRestoreCheckpointInfo) and reference Event, EventRestore, Checkpoint (or the same Checkpoint struct used in the existing test), and the filesRestored/filesDeleted/skipped field names when making the assertions.internal/cli/sandbox_test.go (1)
258-263: ⚡ Quick winCover all effective guard fields in JSON assertions.
The JSON test validates only
interactiveCommandanddestructiveShell. Please also assertnetworkandworkspaceso regressions in effective-guard resolution are caught.♻️ Proposed test update
- if !payload.Guards.InteractiveCommand || !payload.Guards.DestructiveShell { + if !payload.Guards.InteractiveCommand || !payload.Guards.DestructiveShell || !payload.Guards.Network || !payload.Guards.Workspace { t.Fatalf("expected guards reported: %#v", payload.Guards) }🤖 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/sandbox_test.go` around lines 258 - 263, The test currently only asserts payload.Guards.InteractiveCommand and payload.Guards.DestructiveShell; extend the assertion to also check payload.Guards.Network and payload.Guards.Workspace so all effective guard fields are validated. Update the conditional that fails the test to include Network and Workspace (e.g., ensure payload.Guards.Network && payload.Guards.Workspace are true/expected) and adjust the failure message to print the full payload.Guards for debugging; keep the other Plan/GrantsPath assertions unchanged.internal/cli/app_test.go (1)
556-576: ⚡ Quick winAssert
skillsappears in top-level help output.This PR adds
skillsas a top-level command, but the help expectations only addzeroline. Addingskillshere protects help-surface regressions.Proposed assertion update
for _, want := range []string{ "ZERO terminal coding agent", "Usage:", "zero [command]", "exec", "config", "models", "providers", "doctor", "search", "plugins", + "skills", "hooks", "mcp", "sandbox", "update", "worktrees", "verify", "serve", "zeroline", "--version", } {🤖 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 556 - 576, The top-level help test in internal/cli/app_test.go iterates over an expected string slice of commands but misses the newly added "skills" command; update the test by adding "skills" to the slice (the same list that currently contains "zeroline", "exec", "config", etc.) so the loop checks for "skills" in the help output and prevents regressions to the help surface.internal/cli/skills_test.go (1)
61-69: ⚡ Quick winAdd coverage for implicit
skills --jsonrouting.Line 66 verifies
skills list --json, but Line 29-31 behavior ininternal/cli/skills.goalso promisesskills --json. A dedicated test would lock that UX contract.Proposed test addition
+func TestRunSkillsImplicitListJSON(t *testing.T) { + dir := t.TempDir() + writeSkillFixture(t, dir, "demo", "---\nname: demo\ndescription: a demo\n---\nbody") + + var stdout, stderr bytes.Buffer + exit := runWithDeps([]string{"skills", "--json"}, &stdout, &stderr, appDeps{ + skillsDir: func() string { return dir }, + }) + if exit != 0 { + t.Fatalf("exit = %d, stderr = %s", exit, stderr.String()) + } + var payload struct { + Skills []struct { + Name string `json:"name"` + } `json:"skills"` + } + if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil { + t.Fatalf("invalid JSON: %v\n%s", err, stdout.String()) + } + if len(payload.Skills) != 1 || payload.Skills[0].Name != "demo" { + t.Fatalf("unexpected payload: %#v", payload) + } +}🤖 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/skills_test.go` around lines 61 - 69, Add a new unit test that mirrors TestRunSkillsListJSON but invokes the CLI with the implicit routing form to ensure "skills --json" routes to the list handler: create TestRunSkillsJSONImplicit (or similar) that sets up a skill fixture via writeSkillFixture, calls runWithDeps with args ["skills","--json"] and the same appDeps (skillsDir lambda), asserts exit == 0, and validates stdout contains the expected JSON output (same assertions used in TestRunSkillsListJSON); this locks the UX contract implemented in the routing logic in skills.go that maps the bare "skills --json" form to the list command.
🤖 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 69-70: The inline --mode= handling currently accepts empty values;
change the case for strings.HasPrefix(arg, "--mode=") to call the helper
requiredInlineFlagValue(arg, "--mode") (or equivalent) and assign its non-empty
result to options.mode so an empty `--mode=` returns the same usage error as
`--mode` without a value; update the branch that sets options.mode to use
requiredInlineFlagValue and trim/assign the returned value.
In `@internal/cli/exec.go`:
- Around line 177-182: The fmt.Fprintln calls that write notices to stderr
(after resolveSelectedModel when options.model != "" and the similar block at
lines 198-200) currently ignore write errors; update both places to capture the
error returned by fmt.Fprintln(stderr, notice) and, if non-nil, call exitCrash
with that error (or wrap it with context) so write failures are treated the same
as other fatal errors; keep the same logic for setting overrides.Provider.Model
using resolvedModel and only print/handle notice when notice != "".
In `@internal/cli/sessions.go`:
- Around line 64-68: The parser rejects the new "rewind" command because
isSessionsCommand (used by parseSessionsArgs) does not include "rewind", so add
"rewind" to the set of valid session commands checked by isSessionsCommand (and
any helper that builds the command list/usage) so parseSessionsArgs will accept
it; ensure parseSessionsArgs and any usage/help output include "rewind" (repeat
the same change for the analogous check around the other occurrence noted at
197-200) so runSessions can receive the token and dispatch to
runSessionsRewind(…).
- Around line 311-331: The destructive rewind path ignores --exclude-target
because runSessionsRewind calls store.ApplyRewind(sessionID, workspaceRoot,
plan.TargetSequence) without passing plan.KeepTarget; update the apply path to
accept the resolved last-kept sequence (or a keepTarget flag) and call it with
the correct value (e.g. if plan.KeepTarget is true use plan.TargetSequence,
otherwise use plan.TargetSequence-1 or plan.LastKeptSequence exposed from
PlanRewind); change the ApplyRewind signature and all implementations to accept
the extra parameter (or last-kept sequence) and ensure the change is applied
while holding the session lock used by runSessionsRewind so the destructive
rewind honors sessions.RewindOptions.KeepTarget consistently.
- Around line 341-342: The success message prints the raw sessionID; update the
fmt.Fprintf call that writes to stdout to pass redact(sessionID) instead of
sessionID so text-mode output matches the JSON path's redaction. Locate the
fmt.Fprintf(...) invocation that includes sessionID, plan.TargetSequence,
report.FilesRestored, report.FilesDeleted and len(report.Skipped) and replace
the sessionID argument with redact(sessionID) while leaving the rest of the
arguments unchanged.
---
Nitpick comments:
In `@internal/cli/app_test.go`:
- Around line 556-576: The top-level help test in internal/cli/app_test.go
iterates over an expected string slice of commands but misses the newly added
"skills" command; update the test by adding "skills" to the slice (the same list
that currently contains "zeroline", "exec", "config", etc.) so the loop checks
for "skills" in the help output and prevents regressions to the help surface.
In `@internal/cli/sandbox_test.go`:
- Around line 258-263: The test currently only asserts
payload.Guards.InteractiveCommand and payload.Guards.DestructiveShell; extend
the assertion to also check payload.Guards.Network and payload.Guards.Workspace
so all effective guard fields are validated. Update the conditional that fails
the test to include Network and Workspace (e.g., ensure payload.Guards.Network
&& payload.Guards.Workspace are true/expected) and adjust the failure message to
print the full payload.Guards for debugging; keep the other Plan/GrantsPath
assertions unchanged.
In `@internal/cli/skills_test.go`:
- Around line 61-69: Add a new unit test that mirrors TestRunSkillsListJSON but
invokes the CLI with the implicit routing form to ensure "skills --json" routes
to the list handler: create TestRunSkillsJSONImplicit (or similar) that sets up
a skill fixture via writeSkillFixture, calls runWithDeps with args
["skills","--json"] and the same appDeps (skillsDir lambda), asserts exit == 0,
and validates stdout contains the expected JSON output (same assertions used in
TestRunSkillsListJSON); this locks the UX contract implemented in the routing
logic in skills.go that maps the bare "skills --json" form to the list command.
In `@internal/streamjson/streamjson_test.go`:
- Around line 196-214: Add a companion unit test to
TestEventRoundTripsCheckpointInfo that creates an Event with Type EventRestore
and a Checkpoint containing the restore-specific fields filesRestored,
filesDeleted, and skipped, marshal/unmarshal it and assert those fields survive
(e.g., check lengths/values), then also marshal a bare Event{SchemaVersion:1,
Type:EventText} and assert the bare JSON does not contain the "checkpoint"
field; put this logic in a new test function (e.g.,
TestEventRoundTripsRestoreCheckpointInfo) and reference Event, EventRestore,
Checkpoint (or the same Checkpoint struct used in the existing test), and the
filesRestored/filesDeleted/skipped field names when making the assertions.
🪄 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: fe1093c9-d83f-4b69-a85f-e15419d53e7e
📒 Files selected for processing (16)
internal/cli/app.gointernal/cli/app_test.gointernal/cli/exec.gointernal/cli/exec_parse.gointernal/cli/exec_sessions.gointernal/cli/exec_test.gointernal/cli/exec_tools.gointernal/cli/exec_writer.gointernal/cli/sandbox.gointernal/cli/sandbox_test.gointernal/cli/sessions.gointernal/cli/skills.gointernal/cli/skills_test.gointernal/cli/zeroline.gointernal/streamjson/streamjson.gointernal/streamjson/streamjson_test.go
…-target, mode/notice/redaction fixes - sessions: add "rewind" to the parser whitelist (isSessionsCommand) — it was wired in dispatch + help but parseSessionsArgs rejected it as 'unknown sessions command', so it was unreachable. Regression test TestRunSessionsRewindIsReachableAndHonorsExcludeTarget. - sessions rewind: honor --exclude-target — ApplyRewind keeps THROUGH a sequence, so when KeepTarget is false apply through TargetSequence-1, matching rewind-plan (previously rewind always applied TO the target, disagreeing with the plan). The test asserts --exclude-target keeps fewer events than the default. - sessions rewind: redact the session id in the success + not-found messages (consistent with every other human-readable path). - exec_parse: reject empty inline --mode= via requiredInlineFlagValue (was a silent no-op running defaults). - exec: notice writes (model-deprecation + reasoning-effort advisories) now treat write failures as exitCrash, consistent with the rest of the command. build/vet/-race/full-suite + GOOS=windows build green.
Collaborator
Author
|
Addressed all 6 (thanks both):
build/vet/-race/full-suite + windows green. @coderabbitai review |
|
(っ◔◡◔)っ ♥ 🧠 Learnings used✅ Action performedReview finished.
|
Vasanthdev2004
approved these changes
Jun 7, 2026
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
Collaborator
There was a problem hiding this comment.
Rereviewed latest head effd09f. The earlier blocker is fixed: sessions rewind is now reachable through the parser, and --exclude-target now applies through the prior sequence instead of always keeping the target. I also checked the command surface for skills/zeroline, --mode= validation, and help output.\n\nValidation run locally on Windows:\n- go test ./... ✅\n- go run ./cmd/zero-release build ✅\n- go run ./cmd/zero-release smoke ✅\n- git diff --check origin/main...HEAD ✅\n\nManual checks:\n- go run ./cmd/zero sessions rewind demo --sequence 1 now reaches session rewind logic instead of unknown-command parsing.\n- go run ./cmd/zero exec --mode= hello rejects the empty mode value.\n- go run ./cmd/zero skills --json works.\n\nVerdict: Approved. Remaining CodeRabbit-style items I saw are test coverage polish, not merge blockers.
This was referenced Jun 8, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CLI wiring — skills/zeroline commands, --mode presets, exec checkpoints, sandbox --effective, sessions rewind
The final module of the runtime-core decomposition. Wires the new runtime features into the cli while preserving main's merged specialist cli.
The source branch predates the specialist work and drops it; this PR keeps it by 3-way merging each drifted cli file from the common base (
2d41e24) — specialist is a main-only (ours) addition, the reskin features are theirs-only, so both survive (onlyapp.go/exec.gohad real conflicts, resolved to include both).What's in it
zero skills(listsinternal/skills);zero zeroline(interactive TUI with the zeroline skin, viarunInteractiveTUIWithSkin).--modepresets (smart/deep/fast/large/precise) viaapplyExecMode; model-registry alias resolution + deprecation/effort notices;ContextWindowsizing; before-mutation checkpoint recording inOnToolCall; enhanced tool-result output (ChangedFiles/Redacted/Display); permission-mode validation fix.policy --effective(resolved guards).rewind(executes the rewind plan).internal/streamjson—EventCheckpoint/EventRestore+CheckpointInfo.Specialist preserved
specialist.go, the specialist flags (--calling-session-id/--tag/--depth/--init-session-id/…), the specialist runtime registration, and all specialist tests are intact and passing.Testing
build / vet /
go test ./.../go test -race ./internal/cli//GOOS=windows go build ./...all green. No new deps.Completes the decomposition of #101.
Summary by CodeRabbit
New Features
skillscommand (text and JSON output)zerolineinteractive TUI skin with snapshot renderingsessions rewindcommand to restore files from checkpoints--modepresets toexecsandbox policy --effectivefor resolved policy viewsChanges