Add spec mode review flow - #145
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds a spec-draft workflow: ChangesSpec Draft Feature: CLI and TUI
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI_TUI as "CLI/TUI"
participant SpecAgent as "SpecDraftAgent"
participant Store as "SessionStore"
participant ImplAgent as "ImplementationAgent"
User->>CLI_TUI: `zero exec --use-spec` or `/spec <task>`
CLI_TUI->>Store: create spec-draft session
CLI_TUI->>SpecAgent: run with PermissionModeSpecDraft + submit_spec tool
SpecAgent->>CLI_TUI: emit spec_review_required with metadata
CLI_TUI->>Store: persist draft spec (status=draft)
CLI_TUI->>User: render review prompt (approve/reject/edit/cancel)
User->>CLI_TUI: approve
CLI_TUI->>Store: load spec file, create implementation session, link draft→implementation, set status=Approved
CLI_TUI->>ImplAgent: run with ImplementationPrompt (spec + context)
ImplAgent->>User: produce implementation output
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 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: 2
🧹 Nitpick comments (1)
internal/cli/exec_spec_test.go (1)
100-122: ⚡ Quick winAdd parser tests for the remaining new
--use-specvalidation branches.Please add cases for
--spec-model/--spec-reasoning-effortwithout--use-spec, and for--use-specwith specialist session tag. Those branches were added but aren’t exercised here.🤖 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_spec_test.go` around lines 100 - 122, Add tests exercising the remaining --use-spec validation branches: call parseExecArgs with "--spec-model" and with "--spec-reasoning-effort" but without "--use-spec" and assert an error is returned (err != nil and error text indicates these flags require --use-spec), and add a runExecWithEcho test that runs the command with "--use-spec" plus a specialist session tag (e.g. "--session-tag", "specialist") and assert it exits with exitUsage and stderr contains a validation message referencing "specialist" (use existing helpers parseExecArgs, runExecWithEcho and constant exitUsage to locate where to add these new assertions).
🤖 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_spec.go`:
- Around line 193-195: The code calls writer.runEnd(...) unconditionally right
after writer.specReviewRequired(draftInfo), causing duplicate terminal JSON
`done` events because specReviewRequired already emits a `done` event; change
the flow so runEnd is only called when specReviewRequired did not emit the
terminal event. Update writer.specReviewRequired(...) to return a boolean (or
set a flag on writer) indicating whether it emitted the terminal JSON, then call
writer.runEnd(...) only when that return/flag is false; keep the existing
writer.err check but ensure runEnd is invoked conditionally to avoid duplicate
terminal markers.
In `@internal/tui/spec_mode.go`:
- Around line 170-213: The current flow creates an implementation session via
m.sessionStore.Create and appends an event before calling
m.sessionStore.RecordSpec, which can leave an orphaned impl if RecordSpec fails;
make the operation idempotent/atomic by first checking for an existing
implementation session for the given SpecID + review.DraftSessionID (e.g., add
or use a method like sessionStore.FindImplSession(specID, draftSessionID) or
sessionStore.GetSessionByMeta) and reuse its SessionID instead of
unconditionally calling Create, or implement a single store-side operation
(e.g., sessionStore.ApproveSpecAndCreateImpl) that performs the RecordSpec and
Create/AppendEvent transactionally; update the code paths around
m.sessionStore.Create, AppendEvent and RecordSpec to use that check/atomic
operation and only create/append when you have ensured no existing impl session
will be orphaned.
---
Nitpick comments:
In `@internal/cli/exec_spec_test.go`:
- Around line 100-122: Add tests exercising the remaining --use-spec validation
branches: call parseExecArgs with "--spec-model" and with
"--spec-reasoning-effort" but without "--use-spec" and assert an error is
returned (err != nil and error text indicates these flags require --use-spec),
and add a runExecWithEcho test that runs the command with "--use-spec" plus a
specialist session tag (e.g. "--session-tag", "specialist") and assert it exits
with exitUsage and stderr contains a validation message referencing "specialist"
(use existing helpers parseExecArgs, runExecWithEcho and constant exitUsage to
locate where to add these new 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: b5b18693-e218-489f-8469-07e38d9e8e27
📒 Files selected for processing (15)
internal/cli/app.gointernal/cli/exec.gointernal/cli/exec_parse.gointernal/cli/exec_spec.gointernal/cli/exec_spec_test.gointernal/cli/exec_tools.gointernal/cli/spec.gointernal/cli/spec_test.gointernal/specmode/review.gointernal/specmode/specmode_test.gointernal/tui/commands.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/spec_mode.gointernal/tui/spec_mode_test.go
41ad2dd to
6a1c355
Compare
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Review: Add spec mode review flow — looks good, minor suggestions
Strong PR — and it resolves both change-requests from my #144 review:
ExitSpecMode→submit_specrename landed (noExitSpecModeleft;submit_specthroughout). ✅- Path-traversal boundary locked:
specmode.ResolveSpecFilePathcleans the path and callsensureSpecPathContained(filepath.Rel-based, rejects./../absolute/..-prefix) at both write (SaveDraft) and read. Andzero spec show/approve/rejectresolve a draft by session id (store lookup) then load the draft's recorded contained path — not a raw user path. So the live surface is well-guarded. ✅
Other things I verified:
- Enforcement is genuinely wired now (the #144 foundation was unreachable):
runExecSpecDraftruns withDraftSystemPrompt+ a spec permission mode, andvalidateExecToolFiltersrefuses to disable/filter outsubmit_spec— so the agent can draft + submit but the spec-draft tool restriction (deny write/shell/network) actually applies. - Approve/reject state machine is correct: approve requires a spec-draft session, refuses a rejected spec, is idempotent on re-approve (no duplicate impl session), and the rejected path is guarded too.
- Flag validation is thorough:
--use-specrejected with--resume/--fork, inside a specialist child, and--spec-model/--spec-reasoning-effortrequire--use-spec. - Gates:
go test ./internal/cli/... ./internal/specmode/...andgo vetclean.
🟡 Minor / non-blocking
LoadSpecFilereads withos.ReadFile— no non-regular-file guard. A FIFO/named-pipe planted at.zero/specs/<file>.mdwould blockzero spec show/approveindefinitely (os.Openon a writerless FIFO) — the same class you flagged on my #143 (--image <fifo>). Lower risk here (the path is contained to.zero/specs, files are tool-created, and planting a FIFO needs workspace write), but for symmetry consider anos.Stat().Mode().IsRegular()check inResolveSpecFilePath/LoadSpecFile, or a one-line note that it's acceptable per the threat model.- Approve partial-failure leaves an orphan impl session. In
runSpecApprove,store.Create(impl session) runs beforeAppendEventandRecordSpec. IfAppendEvent/RecordSpecfails afterCreate, the draft isn't marked approved, so a retry creates a second impl session (the first is orphaned, never referenced). Harmless but leaks sessions. Optional: create the impl session last, or clean up on the failure path. - Depends on #144's
RecordSpecordering fix. This PR drivesRecordSpecon the draft (exec_spec.go:183) and approve (spec.go:251) paths — the metadata-before-event ordering issue I flagged on #144 now has live callers, so please make sure that fix lands with the foundation. - CodeRabbit's 2 actionable items (parser tests for the remaining
--spec-*flags) are reasonable quick wins.
Since it builds on #144, this should merge after the foundation. Nice work — the lifecycle (draft → review → approve → implementation handoff) is clean and the security-sensitive paths are handled well.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Approving — see my detailed review comment above.
This resolves both change-requests from my #144 review (the ExitSpecMode → submit_spec rename, and the .zero/specs/<slug> path-traversal lock via the filepath.Rel-based ensureSpecPathContained, applied at write + read). I verified the spec-draft tool restriction is now genuinely wired (DraftSystemPrompt + spec permission mode + submit_spec non-filterable), the approve/reject state machine is correct (refuses rejected, idempotent re-approve), flag validation is thorough, and tests/vet are clean.
The remaining items are non-blocking suggestions: the LoadSpecFile non-regular-file (FIFO) guard for symmetry with #143, the orphan-impl-session-on-partial-failure in runSpecApprove, and the dependency on #144's RecordSpec ordering fix. Since this builds on the foundation, it should merge after #144.
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/spec.go`:
- Around line 121-126: The validation currently checks
options.comment/options.reason string values, which misses cases where a flag
was provided with an empty value; update flag parsing to record presence (e.g.,
add boolean fields like CommentProvided and ReasonProvided on the options struct
and set them when the flags are parsed) and change the guards in spec.go to use
those booleans: replace checks of options.comment != "" / options.reason != ""
with checks of options.CommentProvided / options.ReasonProvided and return the
same execUsageError when a provided flag is not valid for the current command
(use the same command and error text).
- Around line 222-263: The approval flow can leave an orphaned spec-impl session
if store.Create succeeds but store.AppendEvent or store.RecordSpec fails; update
the logic around store.Create (impl) so that on any subsequent error you clean
up the created impl session (e.g., call store.DeleteSession or an equivalent
rollback) using impl.SessionID, and ensure you still return the original error
to the caller; reference the symbols store.Create, impl.SessionID,
store.AppendEvent, and store.RecordSpec so you perform the delete/rollback in
both the AppendEvent and RecordSpec error branches and, if the deletion itself
fails, log that secondary failure but do not mask the primary error.
- Around line 165-171: Update the error messages in the spec resolution code so
they comply with ST1005: make the strings start with a lowercase letter and
remove trailing periods. Specifically, change the three fmt.Errorf calls that
return sessions.Metadata{} (the ones using redact(target) and the branch
conditions checking len(matches) == 0, len(matches) > 1, and the ambiguous-zero
check) to use lowercase initial words and no final punctuation (e.g., "zero spec
id is ambiguous: %s" and "zero spec not found: %s") so the messages conform to
staticcheck ST1005.
🪄 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: 41d9b912-a427-4f57-8738-50fd11bf57f7
📒 Files selected for processing (15)
internal/cli/app.gointernal/cli/exec.gointernal/cli/exec_parse.gointernal/cli/exec_spec.gointernal/cli/exec_spec_test.gointernal/cli/exec_tools.gointernal/cli/spec.gointernal/cli/spec_test.gointernal/specmode/review.gointernal/specmode/specmode_test.gointernal/tui/commands.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/spec_mode.gointernal/tui/spec_mode_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/tui/model_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- internal/tui/commands.go
- internal/specmode/specmode_test.go
- internal/cli/spec_test.go
- internal/cli/exec_parse.go
- internal/cli/exec.go
- internal/cli/exec_spec_test.go
- internal/specmode/review.go
- internal/cli/exec_tools.go
- internal/tui/spec_mode_test.go
- internal/tui/model.go
- internal/cli/app.go
- internal/tui/spec_mode.go
- internal/cli/exec_spec.go
6a1c355 to
0667c82
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/cli/spec.go (2)
165-171:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStaticcheck ST1005 errors are still present in error strings.
Line 165, Line 168, and Line 171 still use capitalized/punctuated error text; this will keep ST1005 failing.
Suggested fix
- return sessions.Metadata{}, fmt.Errorf("Zero spec id is ambiguous: %s. Use the draft session id.", redact(target)) + return sessions.Metadata{}, fmt.Errorf("zero spec id is ambiguous: %s; use the draft session id", redact(target)) @@ - return sessions.Metadata{}, fmt.Errorf("Zero spec not found: %s", redact(target)) + return sessions.Metadata{}, fmt.Errorf("zero spec not found: %s", redact(target)) @@ - return sessions.Metadata{}, fmt.Errorf("Zero spec id is ambiguous: %s. Use the draft session id.", redact(target)) + return sessions.Metadata{}, fmt.Errorf("zero spec id is ambiguous: %s; use the draft session id", redact(target))🤖 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/spec.go` around lines 165 - 171, Update the three error literals returned around the matches checks so they follow ST1005 (start with a lowercase letter and avoid terminal punctuation): change "Zero spec id is ambiguous: %s. Use the draft session id." and "Zero spec not found: %s" to lowercase forms like "zero spec id is ambiguous: %s; use the draft session id" and "zero spec not found: %s" (remove the leading capital and trailing period), in the function returning sessions.Metadata{} that uses redact(target) and matches.
121-126:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFlag-scoped validation still misses explicitly-empty values.
Line 121 and Line 124 validate by value (
!= "") instead of flag presence, so--comment ""/--reason ""can bypass command scoping checks.Suggested fix
type specCommandOptions struct { json bool comment string reason string + commentSet bool + reasonSet bool } @@ case arg == "--comment": @@ options.comment = value + options.commentSet = true @@ case strings.HasPrefix(arg, "--comment="): @@ options.comment = value + options.commentSet = true @@ case arg == "--reason": @@ options.reason = value + options.reasonSet = true @@ case strings.HasPrefix(arg, "--reason="): @@ options.reason = value + options.reasonSet = true @@ - if options.comment != "" && command != "approve" { + if options.commentSet && command != "approve" { return command, target, options, false, execUsageError{"--comment is only valid for zero spec approve"} } - if options.reason != "" && command != "reject" { + if options.reasonSet && command != "reject" { return command, target, options, false, execUsageError{"--reason is only valid for zero spec reject"} }🤖 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/spec.go` around lines 121 - 126, The validation uses value checks (options.comment != "" / options.reason != "") so explicitly-empty flags like --comment "" or --reason "" bypass scoping; update the checks to test flag presence instead (e.g. use the flag set's Changed("comment") and Changed("reason") or the codepath that records if a flag was provided) and return the same execUsageError when a provided flag is out-of-scope relative to command (keep the same error messages and use the same symbols: options.comment, options.reason, command, execUsageError).
🤖 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.
Duplicate comments:
In `@internal/cli/spec.go`:
- Around line 165-171: Update the three error literals returned around the
matches checks so they follow ST1005 (start with a lowercase letter and avoid
terminal punctuation): change "Zero spec id is ambiguous: %s. Use the draft
session id." and "Zero spec not found: %s" to lowercase forms like "zero spec id
is ambiguous: %s; use the draft session id" and "zero spec not found: %s"
(remove the leading capital and trailing period), in the function returning
sessions.Metadata{} that uses redact(target) and matches.
- Around line 121-126: The validation uses value checks (options.comment != "" /
options.reason != "") so explicitly-empty flags like --comment "" or --reason ""
bypass scoping; update the checks to test flag presence instead (e.g. use the
flag set's Changed("comment") and Changed("reason") or the codepath that records
if a flag was provided) and return the same execUsageError when a provided flag
is out-of-scope relative to command (keep the same error messages and use the
same symbols: options.comment, options.reason, command, execUsageError).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3dcc1946-b138-42b2-89ab-e202d5509e72
📒 Files selected for processing (17)
internal/cli/app.gointernal/cli/exec.gointernal/cli/exec_parse.gointernal/cli/exec_spec.gointernal/cli/exec_spec_test.gointernal/cli/exec_tools.gointernal/cli/spec.gointernal/cli/spec_test.gointernal/sessions/spec_impl.gointernal/sessions/store_test.gointernal/specmode/review.gointernal/specmode/specmode_test.gointernal/tui/commands.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/spec_mode.gointernal/tui/spec_mode_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/tui/model_test.go
- internal/cli/exec_tools.go
- internal/tui/commands.go
- internal/cli/exec_parse.go
- internal/cli/spec_test.go
- internal/cli/app.go
- internal/specmode/review.go
- internal/tui/model.go
- internal/cli/exec_spec.go
- internal/tui/spec_mode_test.go
- internal/tui/spec_mode.go
0667c82 to
6377ffb
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
# Conflicts: # internal/cli/exec.go
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Review — spec mode review flow
Verified locally on this branch (current with main):
go build ./...✅go vet ./internal/{cli,tui,sessions,specmode}/...✅go test ./internal/{specmode,sessions,cli,tui}/...✅go test -race ./internal/{sessions,specmode,cli}/...✅
Overview
Builds the user-facing flow on top of the spec-mode foundation (#144): zero exec --use-spec (restricted spec-draft mode + submit_spec, structured spec_review_required terminal event), zero spec show|approve|reject, and the TUI /spec draft→review→implement flow. Well-structured. Permission gating is sound: --use-spec forces PermissionModeSpecDraft, disables specialist tools (app.go:398), is blocked inside specialist children and against --resume/--fork, and validates tool filters can't hide submit_spec (exec_tools.go:51). The approve-path locking/idempotency is genuinely good.
CodeRabbit items — status
- Duplicate terminal
done(major) → fixed:specReviewRequiredreturnsemittedTerminal,runEndconditional (exec_spec.go:193). One terminal marker per format. ✅ - "provided but empty" flag bypass (minor) → fixed via
commentProvided/reasonProvided(spec.go:127-132). ✅ - Missing parser tests (nitpick) → covered by
TestParseExecSpecOverridesRequireUseSpec+TestRunExecUseSpecRejectsSpecialistTag. ✅ - Non-atomic approval handoff (major) → largely mitigated: both surfaces use
EnsureSpecImplementation, which locks the source session (spec_impl.go:70), reuses an existing impl session, and dedups the prompt event. A failedRecordSpecafter creation is now recoverable on retry. ✅
Findings
1. Major — CLI approve→resume handoff loses the spec body (500-char truncation).
runSpecApprove stores the full implementation prompt (embedding the entire spec body) as the impl session's user message, then directs the user to zero exec --resume <impl> "Start implementation". But agent.Run is stateless — it seeds only system + prompt + images, never replaying stored events (agent/loop.go:59). On --resume, prior context is rebuilt by FormatExecPrompt → summarizePayload, which truncates each event to 500 chars (sessions/exec_session.go:193). So the implementation model receives only ~500 chars of the spec inline — and since ImplementationPrompt places the Spec file: <path> pointer at the end (specmode/review.go:67), that line is cut off too, so the model isn't reliably told where to re-read the full spec.
This diverges from the TUI approve path, which passes the full implementation prompt straight to agent.Run (tui/spec_mode.go:207) and is unaffected. Same approval → full-fidelity run in the TUI, truncated run from the CLI (the path the Next: hint steers users to).
Suggested directions (any one):
- Have
runSpecApprovestart the implementation itself (mirror the TUI) instead of deferring to a lossy resume, or - Put the
Spec file:path line first inImplementationPromptso it survives truncation, or - Skip digest-summarization for spec-impl resume so the impl prompt passes through intact.
No test asserts the resumed implementation actually receives the spec content — worth adding.
2. Minor — runSpecReject can reject an approved spec when impl id is empty.
spec.go:274 only blocks reject when SpecStatus == Approved && SpecImplSessionID != "". Shouldn't occur via the normal flow, but the guard is asymmetric with the rest of the lifecycle. Consider blocking reject on Approved regardless of impl id.
3. Nit — unused store parameter in runSpecShow (spec.go:182).
Test coverage
Good lifecycle coverage (approve/idempotent/reject/show, parser rejections, TUI draft→review→approve, shift+tab block, EnsureSpecImplementation reuse). Gap: nothing verifies the content the implementation run receives after approval — exactly where Finding 1 hides.
Recommendation
Approve-with-comments. No correctness/security blocker in the changed code; gating and the idempotent/locked handoff are solid. I'd want Finding 1 addressed (or acknowledged as intended) before merge, since it silently weakens the CLI flow the PR markets.
3 participants
Summary:
Tests:
Summary by CodeRabbit
New Features
zero specCLI (show/approve/reject),zero exec --use-specwith spec-specific model/reasoning flags, and interactive/specTUI flow with spec-review modal and spec-draft runs that emit structured review-required outputs.Validation
Tests