feat: implement interview mode for human nodes (#46)#47
Conversation
…w mode Implements ParseQuestions(markdown string) []Question with support for numbered items (. and ) style), bullet questions, and imperative verb prompts. Extracts inline options from trailing parentheticals and detects yes/no questions by option values or interrogative text patterns. Fenced code blocks are skipped. 20 tests covering all specified cases pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements InterviewAnswer and InterviewResult structs with round-trip JSON serialization and a BuildMarkdownSummary helper for downstream agents. All 9 tests pass covering round-trip, invalid input, skipped answers, canceled flag, and markdown format. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add QuestionsKey, AnswersKey, and Prompt extraction to extractHumanAttrs so interview-mode human nodes can carry their context key config through the IR-to-Graph adapter layer. Also upgrades dippin-lang to v0.15.0-beta.1 which defines these fields on ir.HumanConfig. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rview mode Adds the InterviewInterviewer interface, routes mode=interview nodes to executeInterview(), and implements the full handler: question parsing, last_response fallback, zero-questions freeform degradation, retry pre-fill, custom context keys, and JSON+markdown answer storage. Includes 7 TDD tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements the InterviewInterviewer interface on AutoApproveFreeformInterviewer so --auto-approve mode and tests can drive interview-mode human gates. Picks first option for select questions, "yes" for yes/no, and "auto-approved" for open-ended. Adds compile-time interface assertion and unit test coverage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… forms Implements the TUI component shown when an interview-mode human gate fires. Supports radio select, yes/no toggle, and textarea fields with pagination, pre-fill from previous results, and JSON serialization via replyCh. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add MsgGateInterview message type to tui/messages.go, implement AskInterview on BubbleteaInterviewer (Mode 1 inline program + Mode 2 send-func delegation), and handle MsgGateInterview in app.go handleModalMsg to show the InterviewContent modal. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…implementations Implements InterviewInterviewer on AutopilotInterviewer (native LLM), ClaudeCodeAutopilotInterviewer (claude CLI), and AutopilotTUIInterviewer (TUI wrapper). Adds buildInterviewPrompt and parseInterviewResponse helpers with markdown fence stripping, JSON extraction, and a one-retry-on-parse-fail policy (provider errors hard-fail per CLAUDE.md). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements InterviewInterviewer on ConsoleInterviewer so interview-mode human gates work in non-TUI (piped/testing) contexts. Adds compile-time assertion and three unit tests covering name match, numeric index, and blank-to-skip behavior. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Exercises the full pipeline: context → ParseQuestions → AskInterview → context updates, including zero-questions freeform fallback and retry pre-fill pass-through. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Change "N questions answered" to "N of M questions answered" so skipped questions don't inflate the count. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
12 fixes from four-angle code review: Must-fix: 1. Set Incomplete=true in collectAnswers() when any answer is empty 2. Show "Tab → Add details (optional)" hint on focused select/confirm fields 3. ConsoleInterviewer pre-fill uses ID-based lookup instead of position index 4. Esc in fields navigates back instead of hard-canceling (cancel only at Q1) 5. Cap parsed questions at 100 (maxQuestions) to prevent unbounded allocation 6. Strip markdown emphasis (*bold*, _italic_) from question text labels Should-fix: 7. InterviewContent.Cancel() now closes channel (consistent with all other modals) 8. interviewRunner.Update discards cmd on quit (consistent with other runners) 9. Replace flaky time.Sleep test with blocking channel read 10. Promote default context keys to constants (ContextKeyInterviewQuestions/Answers) 11. Document ClaudeCodeAutopilot silent fallback asymmetry vs native hard-fail 12. Document autopilot prompt injection risk in buildInterviewPrompt Also: replace custom contains/searchString helpers with strings.Contains. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements a new "interview" human-node mode: parse upstream agent markdown/JSON into structured questions, present multi-field interviewer flows (console, TUI, autopilot), collect/serialize answers (JSON + markdown), persist to configurable context keys, and wire structured-output plumbing, retry, cancel, and pre-fill behavior across pipeline and UI layers. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent (upstream)
participant Handler as HumanHandler
participant Parser as Parser
participant Interviewer as Interviewer (TUI/Console/Autopilot)
participant Context as Pipeline Context
Agent->>Handler: emit questions (markdown or JSON)
Handler->>Parser: ParseStructuredQuestions / ParseQuestions
Parser-->>Handler: []Question
Handler->>Interviewer: AskInterview(questions, prev)
Interviewer-->>Handler: InterviewResult (or error / canceled)
Handler->>Context: Serialize and store interview_answers (configurable key)
Handler->>Context: Write markdown summary to human_response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f898549e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
pipeline/handlers/autopilot.go
Outdated
| if len(answers) == 0 { | ||
| return nil, fmt.Errorf("interview response had no matching answers for %d questions", len(questions)) |
There was a problem hiding this comment.
Fail interview parsing when answers are missing
parseInterviewResponse currently treats any non-empty subset of matched IDs as success, so if the LLM omits some questions (or returns unknown IDs) the handler silently drops those questions and returns a seemingly valid InterviewResult. In production this can corrupt downstream context because missing answers are neither represented nor marked incomplete, even though the prompt requires one answer per question.
Useful? React with 👍 / 👎.
pipeline/handlers/human.go
Outdated
| if _, err := fmt.Sscanf(input, "%d", &idx); err == nil && idx >= 1 && idx <= len(q.Options) { | ||
| ans.Answer = q.Options[idx-1] | ||
| } else { | ||
| // Treat as "Other" freeform | ||
| ans.Answer = input |
There was a problem hiding this comment.
Handle numbered "Other" choice explicitly
The console interviewer prints an Other option as len(options)+1, but numeric parsing only accepts 1..len(options). If a user enters the displayed number for Other, execution falls into the fallback branch and stores the literal number string (for example "4") as the answer, which is incorrect data and makes the documented number-based selection path unreliable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pipeline/handlers/autopilot.go (1)
203-207:⚠️ Potential issue | 🟠 MajorUpdate default model names to match current provider offerings.
The hardcoded model names are outdated:
gpt-4.1-minishould begpt-5-4-mini(GPT-5.4 mini)gemini-2.5-flashshould begemini-3-flash(Gemini 3 Flash)Verify the exact model identifiers from each provider's API documentation and update the map accordingly. The
claude-sonnet-4-6name requires confirmation as well against current Anthropic offerings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pipeline/handlers/autopilot.go` around lines 203 - 207, Update the autopilotModelDefaults map to use current provider model identifiers: replace "openai":"gpt-4.1-mini" with "openai":"gpt-5-4-mini" and replace "gemini":"gemini-2.5-flash" with "gemini":"gemini-3-flash"; also verify the "anthropic":"claude-sonnet-4-6" entry against Anthropic’s current API/docs and update that value if their official model id differs, then run any existing unit or integration tests that rely on autopilotModelDefaults to ensure compatibility.
🧹 Nitpick comments (4)
docs/superpowers/plans/2026-03-30-interview-mode.md (1)
828-836: Add language identifier to fenced code block.The dependency graph diagram uses a fenced code block without a language identifier, triggering markdownlint MD040. Use
```textor```plaintextfor ASCII diagrams.📝 Suggested fix
-``` +```text Task 1 (parser) ────────────┐🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-03-30-interview-mode.md` around lines 828 - 836, The fenced ASCII diagram block (the one starting with "Task 1 (parser) ────────────┐" and containing Task 2..Task 10) lacks a language identifier which triggers markdownlint MD040; fix by changing the opening fence from ``` to a fenced code block with a plaintext/text specifier (e.g., use ```text or ```plaintext) so the diagram remains intact and linting passes.pipeline/handlers/interview_parse_test.go (1)
340-348: Dead code: self-comparison is always true.Line 341
if opt != optcompares a variable to itself, which is always false (theifbody never executes). This appears to be leftover placeholder code. The actual whitespace check on line 345 is correct.🧹 Proposed cleanup
for i, opt := range questions[0].Options { - if opt != opt { - _ = i // just ensure no leading/trailing spaces would matter - } // Check no leading or trailing spaces if len(opt) > 0 && (opt[0] == ' ' || opt[len(opt)-1] == ' ') { t.Errorf("Option[%d] has leading/trailing space: %q", i, opt) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pipeline/handlers/interview_parse_test.go` around lines 340 - 348, Remove the dead self-comparison in the test loop: delete the `if opt != opt { _ = i }` block inside the loop that iterates over `questions[0].Options` (the `opt` and `i` variables). Keep the existing whitespace assertion that checks leading/trailing spaces; no other logic changes are needed in the test function(s) that reference `questions[0].Options`.pipeline/handlers/autopilot_claudecode.go (1)
135-188: LGTM with one observation —prevparameter is unused.The implementation correctly:
- Hard-fails on CLI execution errors (line 156) and empty responses (line 161) per coding guidelines
- Uses fallback answers on parse failure with warning logged (lines 165-186)
- Documents the intentional asymmetry with
AutopilotInterviewer(lines 128-134)However, the
prev *InterviewResultparameter is accepted but never used. If retry pre-fill should work for autopilot, this would need to be incorporated into the prompt. If intentionally ignored for autopilot mode, consider documenting why with a_ = prevassignment or comment.💡 Option: Document or use the prev parameter
func (a *ClaudeCodeAutopilotInterviewer) AskInterview(questions []Question, prev *InterviewResult) (*InterviewResult, error) { + // Note: prev is intentionally ignored for autopilot — each interview + // starts fresh since the LLM doesn't need retry context. + _ = prev + prompt := buildInterviewPrompt(questions)Or alternatively, incorporate previous answers into the prompt for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pipeline/handlers/autopilot_claudecode.go` around lines 135 - 188, The AskInterview method of ClaudeCodeAutopilotInterviewer currently accepts prev *InterviewResult but never uses it; either explicitly acknowledge and ignore it or incorporate it into the prompt. Fix by adding a deliberate no-op to document intent (e.g., _ = prev with a short comment) if autopilot intentionally ignores previous answers, or merge previous answers into the prompt generation (modify buildInterviewPrompt call or append a serialized prev answers block to fullPrompt) so retry pre-fill is applied; reference AskInterview, the prev parameter, buildInterviewPrompt and fullPrompt when making the change.tui/interviewer.go (1)
266-284: Consider detecting terminal dimensions for Mode 1.The hardcoded
80, 24dimensions may not match the actual terminal size, potentially causing display issues. Other Mode 1 implementations might benefit from terminal size detection.♻️ Optional: Detect terminal size
import "golang.org/x/term" func (b *BubbleteaInterviewer) askMode1Interview(questions []handlers.Question, prev *handlers.InterviewResult) (*handlers.InterviewResult, error) { ch := make(chan string, 1) width, height := 80, 24 if w, h, err := term.GetSize(int(os.Stdout.Fd())); err == nil { width, height = w, h } content := NewInterviewContent(questions, prev, ch, width, height) // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/interviewer.go` around lines 266 - 284, The askMode1Interview method currently constructs NewInterviewContent with hardcoded 80x24 dimensions; update askMode1Interview (in BubbleteaInterviewer) to detect the real terminal size using term.GetSize(int(os.Stdout.Fd())) and fall back to width=80,height=24 on error, then pass those width and height variables into NewInterviewContent; add required imports (golang.org/x/term and os) to the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pipeline/context.go`:
- Around line 23-24: Fix the formatting for the two constant declarations
ContextKeyInterviewQuestions and ContextKeyInterviewAnswers so spacing is
consistent (make the alignment match other constant lines) or simply run gofmt
to auto-format the file; ensure the declaration for ContextKeyInterviewAnswers
uses the same spacing style as ContextKeyInterviewQuestions to satisfy gofmt/CI.
---
Outside diff comments:
In `@pipeline/handlers/autopilot.go`:
- Around line 203-207: Update the autopilotModelDefaults map to use current
provider model identifiers: replace "openai":"gpt-4.1-mini" with
"openai":"gpt-5-4-mini" and replace "gemini":"gemini-2.5-flash" with
"gemini":"gemini-3-flash"; also verify the "anthropic":"claude-sonnet-4-6" entry
against Anthropic’s current API/docs and update that value if their official
model id differs, then run any existing unit or integration tests that rely on
autopilotModelDefaults to ensure compatibility.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-03-30-interview-mode.md`:
- Around line 828-836: The fenced ASCII diagram block (the one starting with
"Task 1 (parser) ────────────┐" and containing Task 2..Task 10) lacks a language
identifier which triggers markdownlint MD040; fix by changing the opening fence
from ``` to a fenced code block with a plaintext/text specifier (e.g., use
```text or ```plaintext) so the diagram remains intact and linting passes.
In `@pipeline/handlers/autopilot_claudecode.go`:
- Around line 135-188: The AskInterview method of ClaudeCodeAutopilotInterviewer
currently accepts prev *InterviewResult but never uses it; either explicitly
acknowledge and ignore it or incorporate it into the prompt. Fix by adding a
deliberate no-op to document intent (e.g., _ = prev with a short comment) if
autopilot intentionally ignores previous answers, or merge previous answers into
the prompt generation (modify buildInterviewPrompt call or append a serialized
prev answers block to fullPrompt) so retry pre-fill is applied; reference
AskInterview, the prev parameter, buildInterviewPrompt and fullPrompt when
making the change.
In `@pipeline/handlers/interview_parse_test.go`:
- Around line 340-348: Remove the dead self-comparison in the test loop: delete
the `if opt != opt { _ = i }` block inside the loop that iterates over
`questions[0].Options` (the `opt` and `i` variables). Keep the existing
whitespace assertion that checks leading/trailing spaces; no other logic changes
are needed in the test function(s) that reference `questions[0].Options`.
In `@tui/interviewer.go`:
- Around line 266-284: The askMode1Interview method currently constructs
NewInterviewContent with hardcoded 80x24 dimensions; update askMode1Interview
(in BubbleteaInterviewer) to detect the real terminal size using
term.GetSize(int(os.Stdout.Fd())) and fall back to width=80,height=24 on error,
then pass those width and height variables into NewInterviewContent; add
required imports (golang.org/x/term and os) to the file.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d07da88-df82-41af-9d77-a17d57cb2440
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
docs/superpowers/plans/2026-03-30-interview-mode.mdgo.modpipeline/context.gopipeline/dippin_adapter.gopipeline/dippin_adapter_test.gopipeline/handlers/autopilot.gopipeline/handlers/autopilot_claudecode.gopipeline/handlers/autopilot_test.gopipeline/handlers/human.gopipeline/handlers/human_test.gopipeline/handlers/interview_integration_test.gopipeline/handlers/interview_parse.gopipeline/handlers/interview_parse_test.gopipeline/handlers/interview_result.gopipeline/handlers/interview_result_test.gotui/app.gotui/autopilot_interviewer.gotui/interview_content.gotui/interview_content_test.gotui/interviewer.gotui/interviewer_test.gotui/messages.gotui/modal.go
…eview workflow Interview mode UX: - One question at a time with progress bar, answered summary, selection feedback - Enter confirms and advances for select and yes/no fields - Text wraps to terminal width (questions, context, options, summaries) - Untouched select fields report empty answer (not first option) - "Other" variants auto-filtered from options (UI always provides its own) - Cancel returns OutcomeFail so pipelines can route on cancellation Structured output: - ParseStructuredQuestions validates JSON questions from agent output - extractJSON uses bracket-depth counting (not greedy first/last brace) - response_format plumbing: .dip → IR → adapter → codergen → session → LLM API - All three providers supported (Anthropic, OpenAI, Gemini) - dippin-lang v0.16.0 integration (ResponseFormat, ResponseSchema, Params) Reliability: - Empty API response detection with capped retry (2 attempts) and diagnostics - Anthropic adapter logs raw response on 0-token replies - Codergen empty-response guard fails node instead of silent success - AskFreeform now hard-fails on provider errors (was silently swallowing) - ClaudeCode autopilot retry-then-hardfail on parse failure - Goroutine leak fix in flashDecision with done channel - Context leak fix (explicit cancel() in retry loop) - Mode 1 tea.Cmd propagation in all three runners New content: - deep_review built-in workflow (3 interview gates, parallel analysis) - interview-loop.dip reusable subgraph - dippin-lang structured output request doc (resolved) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CI caught formatting issues that the pre-commit hook missed because the hook was not installed (symlink to .git/hooks/pre-commit was missing). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Implements interview mode for human gate nodes, enabling structured multi-question Q&A driven by upstream agent output (markdown heuristics or structured JSON), with TUI/console/autopilot backends and context persistence.
Changes:
- Add interview question parsing + interview result serialization/markdown summary, and wire
mode: interviewthrough the human handler and TUI message bus. - Add fullscreen TUI interview modal and interviewer integrations (Bubbletea Mode 1/2, console, autopilot variants).
- Add
response_format/response_schemaplumbing from.dip→ IR → adapter → agent session → provider requests, plus a newdeep_reviewbuilt-in workflow and related docs.
Reviewed changes
Copilot reviewed 42 out of 43 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tui/modal.go | Propagate terminal size into the new Interview modal content. |
| tui/messages.go | Add MsgGateInterview to route interview gates through the TUI. |
| tui/interviewer.go | Implement AskInterview for Bubbletea interviewer (Mode 1 + Mode 2), and propagate tea.Cmd from content updates. |
| tui/interviewer_test.go | Add Mode 2 interview gate message test coverage. |
| tui/interview_content.go | Add fullscreen interview form UI (select/yes-no/textarea, pagination/navigation, submit/cancel). |
| tui/interview_content_test.go | Add unit tests for InterviewContent behavior (collect/submit/cancel/prefill/view). |
| tui/autopilot_interviewer.go | Add TUI “flash” support for autopilot interview results + improve goroutine lifetime handling. |
| tui/autopilot_interviewer_test.go | Add test for inner-autopilot lacking interview support. |
| tui/app.go | Show InterviewContent modal when receiving MsgGateInterview. |
| README.md | Document interview mode and new deep_review workflow; update counts (3→4 workflows / 3→4 gate modes). |
| pipeline/handlers/tool_test.go | Fix status artifact test to use env.WorkingDir() instead of overriding internals. |
| pipeline/handlers/parallel.go | Remove unused branchOverride type. |
| pipeline/handlers/interview_result.go | Add interview result/answer types, JSON serialization, and markdown summary builder. |
| pipeline/handlers/interview_result_test.go | Add tests for serialization + markdown summary formatting. |
| pipeline/handlers/interview_parse.go | Add markdown + structured JSON interview question parsing utilities (caps, filtering, yes/no detection). |
| pipeline/handlers/interview_parse_test.go | Add extensive test coverage for parsing and filtering behaviors. |
| pipeline/handlers/interview_integration_test.go | Add end-to-end tests for interview handler flow (markdown, structured JSON, retry prefill, zero-question fallback). |
| pipeline/handlers/human.go | Add InterviewInterviewer interface and executeInterview(); add console and auto-approve interview implementations. |
| pipeline/handlers/human_test.go | Add handler/interviewer tests covering interview mode behaviors and error paths. |
| pipeline/handlers/codergen.go | Add response-format wiring and strengthen empty-response guard conditions. |
| pipeline/handlers/autopilot.go | Make freeform hard-fail on provider errors; add autopilot interview prompting + JSON parsing. |
| pipeline/handlers/autopilot_test.go | Add tests for ambiguous choice matching and interview prompt/response parsing. |
| pipeline/handlers/autopilot_claudecode.go | Make freeform hard-fail on CLI errors; add claude-code interview support with retry-on-parse-failure. |
| pipeline/handlers/autopilot_claudecode_test.go | Update tests to assert hard-fail behavior on freeform CLI errors. |
| pipeline/events.go | Remove interview-specific event types. |
| pipeline/events_test.go | Update uniqueness test to match event type list after removal. |
| pipeline/dippin_adapter.go | Extract new IR fields (human interview keys/prompt; agent response_format/schema + params passthrough). |
| pipeline/dippin_adapter_test.go | Add tests for interview human-attr extraction. |
| pipeline/context.go | Add default context keys for interview questions/answers. |
| llm/anthropic/adapter.go | Add logging for completely empty Anthropic responses to aid diagnostics. |
| go.mod | Bump github.com/2389-research/dippin-lang to v0.16.0. |
| go.sum | Add checksums for updated dippin-lang versions. |
| examples/subgraphs/interview-loop.dip | Add reusable interview-loop subgraph workflow example. |
| docs/superpowers/plans/2026-03-30-interview-mode.md | Add a detailed implementation plan document for interview mode. |
| docs/site/workflows.html | Document the new deep_review built-in workflow and update workflow count messaging. |
| docs/requests/dippin-lang-structured-output.md | Add/mark-resolved request doc for IR structured output support. |
| cmd/tracker/workflows/deep_review.dip | Add new embedded deep_review workflow using interview gates. |
| cmd/tracker/embed_test.go | Update embedded workflow tests for the new deep_review workflow and count. |
| CLAUDE.md | Document interview mode + structured output/params passthrough behavior. |
| CHANGELOG.md | Add release notes covering interview mode, structured output support, and deep_review. |
| agent/session.go | Retry truly empty API responses instead of stopping silently. |
| agent/session_run.go | Wire ResponseFormat into LLM requests and implement buildResponseFormat(). |
| agent/config.go | Add ResponseFormat and ResponseSchema to session config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tui/interview_content.go
Outdated
| if len(f.question.Options) > 0 { | ||
| // Select field: find matching option or set as "Other". | ||
| matched := false | ||
| for j, opt := range f.question.Options { | ||
| if strings.EqualFold(opt, ans.Answer) { | ||
| ic.fields[i].selectCursor = j | ||
| matched = true | ||
| break | ||
| } | ||
| } | ||
| if !matched && ans.Answer != "" { | ||
| // Set as "Other" | ||
| ic.fields[i].selectCursor = len(f.question.Options) | ||
| ic.fields[i].isOther = true | ||
| ic.fields[i].otherInput.SetValue(ans.Answer) | ||
| } |
There was a problem hiding this comment.
Interview pre-fill populates selectCursor/otherInput but never sets selected=true for option questions. As a result, previously answered select questions render as unanswered and collectAnswers() will serialize an empty answer unless the user re-confirms the selection. Set selected when a previous answer exists (matched option or non-empty Other) so retry pre-fill actually preserves answers on submit.
| // fieldAnswered returns true if the field has a non-empty answer. | ||
| func (ic *InterviewContent) fieldAnswered(f *interviewField) bool { | ||
| if len(f.question.Options) > 0 { | ||
| return f.selected | ||
| } | ||
| if f.question.IsYesNo { | ||
| return f.confirmed != nil | ||
| } | ||
| return strings.TrimSpace(f.textInput.Value()) != "" |
There was a problem hiding this comment.
fieldAnswered counts option questions as answered when selected is true, but for the built-in "Other" path selected can be true even if the otherInput is empty (e.g., user hits Enter immediately). This inflates the answered count/progress and disagrees with collectAnswers() (which will record an empty answer and mark the result incomplete). Consider treating "Other" as answered only when the trimmed otherInput is non-empty.
| // Cancel implements Cancellable. Closes the reply channel to signal cancellation, | ||
| // consistent with all other ModalContent types (ChoiceContent, FreeformContent, etc.). | ||
| func (ic *InterviewContent) Cancel() { ic.cancelForm() } | ||
|
|
||
| // cancelForm closes the reply channel to signal cancellation. | ||
| // The receiver (askMode2Interview) detects the closed channel via the ok flag | ||
| // and returns InterviewResult{Canceled: true}. | ||
| func (ic *InterviewContent) cancelForm() tea.Cmd { | ||
| if ic.done { | ||
| return nil | ||
| } | ||
| ic.done = true | ||
| if ic.replyCh != nil { | ||
| close(ic.replyCh) | ||
| ic.replyCh = nil | ||
| } | ||
| return func() tea.Msg { return MsgModalDismiss{} } |
There was a problem hiding this comment.
Cancel currently closes replyCh without sending any serialized result, so the caller only receives InterviewResult{Canceled:true} and loses any partial answers entered in the form. If cancellation is intended to persist partial answers (as described in the PR metadata/changelog), send a serialized result with Canceled=true (and current answers) before closing the channel (or return it via a separate cancellation message).
| if len(q.Options) > 0 { | ||
| // Print numbered options | ||
| for j, opt := range q.Options { | ||
| fmt.Fprintf(c.Writer, " %d) %s\n", j+1, opt) | ||
| } | ||
| fmt.Fprintf(c.Writer, " %d) Other\n", len(q.Options)+1) | ||
|
|
||
| // Pre-fill hint | ||
| if prevAns.Answer != "" { | ||
| fmt.Fprintf(c.Writer, "Previous: %s\n", prevAns.Answer) | ||
| } | ||
| fmt.Fprintf(c.Writer, "Enter choice (name or number, blank to skip): ") | ||
|
|
||
| line, err := c.readLine() | ||
| if err != nil { | ||
| canceled = true | ||
| answers[i] = ans | ||
| // Fill remaining questions with empty answers. | ||
| for j := i + 1; j < len(questions); j++ { | ||
| answers[j] = InterviewAnswer{ | ||
| ID: fmt.Sprintf("q%d", questions[j].Index), | ||
| Text: questions[j].Text, | ||
| } | ||
| } | ||
| break | ||
| } | ||
| input := strings.TrimSpace(line) | ||
| if input != "" { | ||
| // Match by name (case-insensitive) or number | ||
| matched := false | ||
| for _, opt := range q.Options { | ||
| if strings.EqualFold(input, opt) { | ||
| ans.Answer = opt | ||
| matched = true | ||
| break | ||
| } | ||
| } | ||
| if !matched { | ||
| var idx int | ||
| if _, err := fmt.Sscanf(input, "%d", &idx); err == nil && idx >= 1 && idx <= len(q.Options) { | ||
| ans.Answer = q.Options[idx-1] | ||
| } else { | ||
| // Treat as "Other" freeform | ||
| ans.Answer = input | ||
| } |
There was a problem hiding this comment.
The console UI prints an "Other" option as len(options)+1) Other, but numeric input handling only accepts indices up to len(q.Options). If the user enters the displayed "Other" number, it falls through and records the literal number (e.g., "3") as the answer. Handle idx == len(q.Options)+1 explicitly (e.g., prompt for a freeform value, or treat it as selecting Other with an empty answer).
pipeline/handlers/autopilot.go
Outdated
| // Build a lookup map from question ID (q1, q2, ...) to Question | ||
| qByID := make(map[string]Question, len(questions)) | ||
| for _, q := range questions { | ||
| qByID[fmt.Sprintf("q%d", q.Index)] = q | ||
| } | ||
|
|
||
| answers := make([]InterviewAnswer, 0, len(envelope.Answers)) | ||
| for _, a := range envelope.Answers { | ||
| q, ok := qByID[a.ID] | ||
| if !ok { | ||
| // Skip answers with unrecognized IDs rather than failing | ||
| continue | ||
| } | ||
| answers = append(answers, InterviewAnswer{ | ||
| ID: a.ID, | ||
| Text: q.Text, | ||
| Options: q.Options, | ||
| Answer: a.Answer, | ||
| Elaboration: a.Elaboration, | ||
| }) | ||
| } | ||
|
|
||
| if len(answers) == 0 { | ||
| return nil, fmt.Errorf("interview response had no matching answers for %d questions", len(questions)) | ||
| } | ||
|
|
||
| return &InterviewResult{Questions: answers}, nil |
There was a problem hiding this comment.
parseInterviewResponse doesn’t validate option-constrained answers against the provided options, despite the prompt comment saying risk is mitigated by matchChoice validation. Also, it returns answers only for IDs present in the LLM response and in that response order, which can drop unanswered questions, misorder results, and leaves Incomplete unset. Consider building a result with one entry per question (in question order), normalizing/validating answers (use matchChoice when options exist, normalize yes/no), and setting Incomplete when any answers are missing/empty.
| // Build a lookup map from question ID (q1, q2, ...) to Question | |
| qByID := make(map[string]Question, len(questions)) | |
| for _, q := range questions { | |
| qByID[fmt.Sprintf("q%d", q.Index)] = q | |
| } | |
| answers := make([]InterviewAnswer, 0, len(envelope.Answers)) | |
| for _, a := range envelope.Answers { | |
| q, ok := qByID[a.ID] | |
| if !ok { | |
| // Skip answers with unrecognized IDs rather than failing | |
| continue | |
| } | |
| answers = append(answers, InterviewAnswer{ | |
| ID: a.ID, | |
| Text: q.Text, | |
| Options: q.Options, | |
| Answer: a.Answer, | |
| Elaboration: a.Elaboration, | |
| }) | |
| } | |
| if len(answers) == 0 { | |
| return nil, fmt.Errorf("interview response had no matching answers for %d questions", len(questions)) | |
| } | |
| return &InterviewResult{Questions: answers}, nil | |
| // Build a lookup map from answer ID (q1, q2, ...) to the raw response object. | |
| ansByID := make(map[string]interviewResponseAnswer, len(envelope.Answers)) | |
| for _, a := range envelope.Answers { | |
| if a.ID == "" { | |
| continue | |
| } | |
| ansByID[a.ID] = a | |
| } | |
| answers := make([]InterviewAnswer, 0, len(questions)) | |
| incomplete := false | |
| recognizedCount := 0 | |
| // Build one InterviewAnswer per question, in question order. | |
| for _, q := range questions { | |
| id := fmt.Sprintf("q%d", q.Index) | |
| raw, ok := ansByID[id] | |
| answerText := "" | |
| elaboration := "" | |
| if ok { | |
| recognizedCount++ | |
| answerText = strings.TrimSpace(raw.Answer) | |
| elaboration = raw.Elaboration | |
| // Normalize yes/no style answers to canonical values. | |
| lower := strings.ToLower(answerText) | |
| switch lower { | |
| case "yes", "y", "true": | |
| answerText = "yes" | |
| case "no", "n", "false": | |
| answerText = "no" | |
| } | |
| // If options are provided, ensure the answer matches one of them. | |
| if len(q.Options) > 0 && answerText != "" { | |
| valid := false | |
| for _, opt := range q.Options { | |
| if strings.EqualFold(strings.TrimSpace(opt), answerText) { | |
| // Use the canonical option string from the question. | |
| answerText = opt | |
| valid = true | |
| break | |
| } | |
| } | |
| if !valid { | |
| // Treat answers that don't match any option as unanswered. | |
| answerText = "" | |
| } | |
| } | |
| } | |
| if strings.TrimSpace(answerText) == "" { | |
| incomplete = true | |
| } | |
| answers = append(answers, InterviewAnswer{ | |
| ID: id, | |
| Text: q.Text, | |
| Options: q.Options, | |
| Answer: answerText, | |
| Elaboration: elaboration, | |
| }) | |
| } | |
| // Preserve existing behavior: fail if no answers matched any known question. | |
| if recognizedCount == 0 { | |
| return nil, fmt.Errorf("interview response had no matching answers for %d questions", len(questions)) | |
| } | |
| return &InterviewResult{Questions: answers, Incomplete: incomplete}, nil |
| // Log diagnostic info for empty responses — helps debug silent API failures. | ||
| if resp.Usage.OutputTokens == 0 && resp.Text() == "" && len(resp.ToolCalls()) == 0 { | ||
| log.Printf("[anthropic] WARNING: empty response (0 output tokens, no text, no tool calls) — status=%d stop_reason=%s model=%s request_id=%s raw_length=%d", | ||
| httpResp.StatusCode, resp.FinishReason.Raw, resp.Model, httpResp.Header.Get("Request-Id"), len(respBody)) | ||
| if len(respBody) < 2000 { | ||
| log.Printf("[anthropic] raw response body: %s", string(respBody)) | ||
| } |
There was a problem hiding this comment.
This diagnostic path logs the raw provider response body when an empty response is detected. Provider bodies can include error details and may contain user-supplied text depending on upstream behavior; logging them unconditionally risks leaking sensitive data into logs. Consider gating raw-body logging behind a debug flag/env var, redacting, or logging only metadata (request id, status, stop_reason, length).
| - Questions with "other" variants in options are filtered — the UI always provides its own "Other" escape hatch | ||
| - One question shown at a time with progress bar, answered summary above, and selection feedback (filled dot + checkmark) | ||
| - Canceled interviews return `OutcomeFail`, not `OutcomeSuccess` — pipeline edges can route on cancellation | ||
| - `questions_key` defaults to `last_response`; `answers_key` defaults to `interview_answers` |
There was a problem hiding this comment.
Docs say questions_key defaults to last_response, but the handler’s default is interview_questions (with a fallback to last_response only if that context key is empty). Please align this bullet with the actual runtime behavior to avoid confusion when authoring .dip files.
| - `questions_key` defaults to `last_response`; `answers_key` defaults to `interview_answers` | |
| - `questions_key` defaults to `interview_questions` (falling back to `last_response` when empty); `answers_key` defaults to `interview_answers` |
- parseInterviewResponse fills missing answers with empty entries and sets Incomplete when LLM omits questions (Codex P1) - Console "Other" number (len+1) now prompts for freeform text instead of storing the literal number string (Codex P2) - Document unused prev param in ClaudeCodeAutopilotInterviewer.AskInterview - Detect real terminal size in Mode 1 interview via golang.org/x/term - Add language identifier to markdown code fence in plan doc Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent/config.go`:
- Around line 33-34: Validate ResponseFormat and ResponseSchema early in
Validate(): ensure ResponseFormat is either empty, "json_object", or
"json_schema"; if it's "json_schema" require ResponseSchema to be non-empty and
parseable as valid JSON (e.g., json.Unmarshal into interface{}) and return a
descriptive error if parsing fails; add corresponding error returns from
Validate() so invalid structured-output settings fail at config load instead of
later during provider calls.
In `@agent/session.go`:
- Around line 192-205: The retry loop currently treats repeated empty API
responses as a natural stop once emptyResponseRetries exceeds
maxEmptyResponseRetries (setting stoppedNaturally = true and breaking), so
Session.Run returns nil; change the logic in the block that checks
result.TotalToolCalls(), resp.Message.Content, resp.Usage.OutputTokens and
emptyResponseRetries to, when emptyResponseRetries >= maxEmptyResponseRetries,
emit an error event (using s.emit with a diagnostic like the existing diag) and
return/propagate a non-nil error from Session.Run (instead of setting
stoppedNaturally = true), so callers see a failure; keep the existing retry
increment behavior while under the budget (emptyResponseRetries,
maxEmptyResponseRetries, diag, s.messages).
In `@cmd/tracker/workflows/deep_review.dip`:
- Around line 286-289: The revise path currently restarts WriteRemediationPlan
with the original inputs but never passes the ReviewPlan feedback into the
prompt, causing identical regeneration; modify the revise handler so that when
ReviewPlan returns feedback you persist that feedback under a dedicated key
(e.g., review_plan_feedback or plan_review_feedback) and include that key/value
in the inputs passed into WriteRemediationPlan (and in any prompt assembly
function used by WriteRemediationPlan) so the new run sees reviewer comments;
apply the same change to the other revise-like invocations you saw (the other
two occurrences referenced) to ensure all WriteRemediationPlan restarts include
the persisted ReviewPlan feedback.
- Around line 239-260: The prompt enforces 3-5 prioritization questions even
when no findings were triaged as actionable; change the logic around the JSON
output (the block that currently mandates "Rules: - 3-5 questions" and produces
{"questions":[...]} ) to first inspect ctx.findings_answers for any entries
marked "fix now" or "fix later" and only require 3-5 questions in that case;
otherwise emit a valid empty fallback JSON object {"questions":[]} as the final
output. Ensure the condition references ctx.findings_answers and the JSON-output
requirement text so the template produces the empty-question set when all
findings are "skip" or "need more context".
- Around line 70-76: The interview node ScopeInterview (mode: interview)
overwrites ctx.human_response with a markdown summary, so snapshot the original
developer goal (e.g., the DescribeGoal input) into a dedicated context key (for
example ctx.original_goal or ctx.describe_goal_snapshot) before ScopeInterview
runs; update subsequent analysis prompts to reference that snapshot key instead
of ctx.human_response; apply the same change for the other interview nodes noted
(lines ~90-92, 119-121, 152-154) so all analysis prompts read the preserved
original goal rather than the overwritten human_response.
In `@docs/site/workflows.html`:
- Around line 243-265: The Mermaid diagram uses raw '>' characters in arrow
tokens (e.g., the edges like Ask --> Explore, Explore --> GenScope, J --> Synth,
etc.), which triggers HTMLHint spec-char-escape; update every occurrence of the
arrow token "-->" to the escaped form "-->" throughout the block (including
all edges such as Ask --> Explore, P --> Correct, Correct --> J, Review
-->|approve| Final, etc.) so the HTML file passes the linter.
In `@examples/subgraphs/interview-loop.dip`:
- Around line 25-31: The prompts for the Interviewer and Assess nodes are
missing the live interview state, so inject the conversation/context variables
into their prompt templates: update the Interviewer node's prompt to include
${ctx.interview_answers} (and any previous Assess output held on ctx, e.g.
${ctx.assess_output} or the actual key used) so it can see prior answers and
restart state, and update the Assess node's prompt to include
${ctx.interview_answers} so it evaluates the collected answers; apply the same
change to the second prompt block around lines 67-80 to ensure both rounds use
ctx state.
In `@llm/anthropic/adapter.go`:
- Around line 112-119: The empty-response sentinel (resp.Usage.OutputTokens == 0
&& resp.Text() == "" && len(resp.ToolCalls()) == 0) currently only logs a
warning but still returns resp,nil from Complete(); change this to return a
non-nil error so callers treat it as a failure. Inside Complete(), when that
condition matches, construct and return an error (e.g., with fmt.Errorf or
errors.Wrap) that includes diagnostic fields: httpResp.StatusCode,
resp.FinishReason.Raw, resp.Model, httpResp.Header.Get("Request-Id"), and
len(respBody) (and include respBody text when small) instead of continuing; keep
the existing log lines if desired but ensure the function returns nil,err so
callers don’t treat the empty resp as success.
In `@pipeline/handlers/autopilot_test.go`:
- Around line 230-238: The test fails because buildInterviewPrompt(nil) only
renders explicit Question.Options and doesn't convert Question.IsYesNo into
actual options; update buildInterviewPrompt to detect when a Question has
IsYesNo == true and, if Options is empty, set Options to []string{"yes", "no"}
(or otherwise ensure the rendered prompt includes "yes, no") before
formatting/rendering; modify the logic in buildInterviewPrompt (and any helper
that maps Question -> rendered text) so it prefers explicit Options but falls
back to the IsYesNo-derived ["yes","no"] for questions with Question.IsYesNo.
In `@pipeline/handlers/autopilot.go`:
- Around line 326-389: AskInterview ignores the provided prev *InterviewResult
so the model isn't prefilled with prior answers; update buildInterviewPrompt to
accept prev *InterviewResult (or add a new helper) and have AskInterview call it
with prev, then include a "Previously answered" JSON block listing each prior
answer's id/index, answer, and elaboration (matched to Question via
Question.Index or id) before the Questions list so the LLM sees and can continue
from earlier selections; ensure both the initial req and the retry path use the
same prompt-with-prev when constructing req.Messages.
- Around line 445-457: The loop that builds InterviewAnswer from
envelope.Answers must validate constrained question types before appending: for
each answer (range envelope.Answers) look up q := qByID[a.ID] and if q.Type is a
constrained type (e.g., yes/no or fixed-option) verify a.Answer exactly matches
an allowed option (normalize casing/whitespace if desired) or for yes/no accept
only "yes"/"no"; if validation fails return a parse error from
parseInterviewResponse (do not continue/append) so the retry path can run;
otherwise append the validated InterviewAnswer as before (refer to qByID,
InterviewAnswer, envelope.Answers, and parseInterviewResponse).
In `@pipeline/handlers/human.go`:
- Around line 248-293: The menu's "Other" choice isn't handled: when the user
types the displayed Other number or "Other" the code currently saves the
index/string instead of collecting a custom response. Update the input handling
in the block that reads q.Options (around q.Options, ans.Answer, c.readLine) to
(1) treat a match on strings.EqualFold(input, "other") or an parsed idx ==
len(q.Options)+1 as selection of the "Other" branch, (2) when "Other" is
selected prompt again (use c.readLine) for the freeform value and assign that to
ans.Answer, and (3) keep existing behavior for name matches and numeric
selections 1..len(q.Options); ensure the blank-skip and error/cancel logic
remain unchanged.
In `@tui/interview_content.go`:
- Around line 316-323: The branch selection currently treats questions with
Options before IsYesNo, causing yes/no questions (which also have Options) to be
handled as a generic select; change the logic to give IsYesNo precedence by
computing a shared isSelect := len(q.Options) > 0 && !q.IsYesNo (use the
question variable/name used in this file, e.g., f.question or q) and replace the
repeated len(Options) > 0 checks in interview_content.go (the field update
dispatch that calls updateSelectField, updateConfirmField, updateTextField) as
well as the prefill, serialization, rendering, and progress helper locations to
use isSelect so yes/no questions route to updateConfirmField instead of
updateSelectField.
- Around line 592-601: The summary loop currently iterates over all prior fields
using ic.cursor and ic.fields and ignores the visible page/height, causing the
active prompt to be pushed out of view; limit the rendered answered-summary to
what fits in the current viewport by computing a rowsAvailable value from the
page/height variables and only iterating over the last rowsAvailable entries
(e.g., compute start = max(0, ic.cursor - rowsAvailable) and loop from start to
ic.cursor), calling fieldAnswerText(f) and using the existing
summaryStyle/skippedStyle rendering as before so the summary is clipped to the
current page.
- Around line 144-162: prefill() restores selectCursor and otherInput but
doesn't mark the field as selected, so collectAnswers() ignores restored select
answers; update the prefill logic (in the block handling f.question.Options) to
set ic.fields[i].selected = true whenever you set selectCursor (both when a
matching option is found and when marking as "Other"/isOther) so restored
selects are serialized on submit; also ensure isOther and
otherInput.SetValue(ans.Answer) remain set as before and preserve setting
ic.fields[i].elaboration when ans.Elaboration is present.
- Around line 519-535: The cancel path currently just closes replyCh so callers
treat it as a generic cancel with no answers, discarding partial input; change
cancelForm (and thereby InterviewContent.Cancel) to send an InterviewResult
containing Canceled:true plus the current partial answers/state (e.g., capture
ic.answers or the struct fields used by askMode2Interview) into ic.replyCh
before closing it, then set ic.done and nil out ic.replyCh and return the
MsgModalDismiss cmd; to avoid blocking if the receiver might not be ready,
perform the send in a goroutine or ensure the channel is buffered, and still
close the channel afterwards to keep the existing receiver logic in
askMode2Interview compatible.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c0522db2-d455-4157-b600-497809ea2480
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
CHANGELOG.mdCLAUDE.mdREADME.mdagent/config.goagent/session.goagent/session_run.gocmd/tracker/embed_test.gocmd/tracker/workflows/deep_review.dipdocs/requests/dippin-lang-structured-output.mddocs/site/workflows.htmlexamples/subgraphs/interview-loop.dipgo.modllm/anthropic/adapter.gopipeline/dippin_adapter.gopipeline/events.gopipeline/events_test.gopipeline/handlers/autopilot.gopipeline/handlers/autopilot_claudecode.gopipeline/handlers/autopilot_claudecode_test.gopipeline/handlers/autopilot_test.gopipeline/handlers/codergen.gopipeline/handlers/human.gopipeline/handlers/human_test.gopipeline/handlers/interview_integration_test.gopipeline/handlers/interview_parse.gopipeline/handlers/interview_parse_test.gopipeline/handlers/interview_result.gopipeline/handlers/parallel.gopipeline/handlers/tool_test.gotui/autopilot_interviewer.gotui/autopilot_interviewer_test.gotui/interview_content.gotui/interview_content_test.gotui/interviewer.go
💤 Files with no reviewable changes (2)
- pipeline/events.go
- pipeline/handlers/parallel.go
✅ Files skipped from review due to trivial changes (4)
- pipeline/events_test.go
- go.mod
- pipeline/handlers/tool_test.go
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- pipeline/handlers/interview_parse.go
- pipeline/handlers/interview_result.go
| if len(q.Options) > 0 { | ||
| // Print numbered options | ||
| for j, opt := range q.Options { | ||
| fmt.Fprintf(c.Writer, " %d) %s\n", j+1, opt) | ||
| } | ||
| fmt.Fprintf(c.Writer, " %d) Other\n", len(q.Options)+1) | ||
|
|
||
| // Pre-fill hint | ||
| if prevAns.Answer != "" { | ||
| fmt.Fprintf(c.Writer, "Previous: %s\n", prevAns.Answer) | ||
| } | ||
| fmt.Fprintf(c.Writer, "Enter choice (name or number, blank to skip): ") | ||
|
|
||
| line, err := c.readLine() | ||
| if err != nil { | ||
| canceled = true | ||
| answers[i] = ans | ||
| // Fill remaining questions with empty answers. | ||
| for j := i + 1; j < len(questions); j++ { | ||
| answers[j] = InterviewAnswer{ | ||
| ID: fmt.Sprintf("q%d", questions[j].Index), | ||
| Text: questions[j].Text, | ||
| } | ||
| } | ||
| break | ||
| } | ||
| input := strings.TrimSpace(line) | ||
| if input != "" { | ||
| // Match by name (case-insensitive) or number | ||
| matched := false | ||
| for _, opt := range q.Options { | ||
| if strings.EqualFold(input, opt) { | ||
| ans.Answer = opt | ||
| matched = true | ||
| break | ||
| } | ||
| } | ||
| if !matched { | ||
| var idx int | ||
| if _, err := fmt.Sscanf(input, "%d", &idx); err == nil && idx >= 1 && idx <= len(q.Options) { | ||
| ans.Answer = q.Options[idx-1] | ||
| } else { | ||
| // Treat as "Other" freeform | ||
| ans.Answer = input | ||
| } | ||
| } |
There was a problem hiding this comment.
The console Other option is currently broken.
This branch prints N+1) Other, but the parser only recognizes indices up to len(q.Options). If the user follows the UI and enters the displayed Other number, the saved answer becomes the literal menu index ("3", "4", etc.) instead of a custom response. Typing Other has the same problem. Handle the extra menu item explicitly and collect the custom value there.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pipeline/handlers/human.go` around lines 248 - 293, The menu's "Other" choice
isn't handled: when the user types the displayed Other number or "Other" the
code currently saves the index/string instead of collecting a custom response.
Update the input handling in the block that reads q.Options (around q.Options,
ans.Answer, c.readLine) to (1) treat a match on strings.EqualFold(input,
"other") or an parsed idx == len(q.Options)+1 as selection of the "Other"
branch, (2) when "Other" is selected prompt again (use c.readLine) for the
freeform value and assign that to ans.Answer, and (3) keep existing behavior for
name matches and numeric selections 1..len(q.Options); ensure the blank-skip and
error/cancel logic remain unchanged.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
pipeline/handlers/human.go (1)
287-298:⚠️ Potential issue | 🟠 Major
Otherstill has a broken literal path and swallows follow-up errors.Typing the literal
Otherfalls through toans.Answer = input, so the saved answer becomes"Other"instead of prompting for the custom value. Also, if the follow-upreadLine()fails, that error is ignored and the interview keeps going instead of being marked canceled with partial writes. Route both paths through the sameOtherhandler and propagate the follow-up read failure intocanceled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pipeline/handlers/human.go` around lines 287 - 298, The literal "Other" path currently falls through and readLine() errors are ignored; update the option-selection logic (the block using fmt.Sscanf, q.Options and idx) to route both the numeric "Other" choice (idx == len(q.Options)+1) and the literal input "Other" into the same handler: prompt via fmt.Fprintf(c.Writer, "Enter your answer: "), call c.readLine() to get otherLine, set ans.Answer = strings.TrimSpace(otherLine) on success, and if c.readLine() returns an error then set the interview canceled flag (canceled) and propagate/return the error so the interview is aborted with no partial writes; ensure you reference c.readLine(), ans.Answer, q.Options and canceled when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-03-30-interview-mode.md`:
- Line 11: Replace the hardcoded absolute worktree path shown after the
"Worktree:" label (the string
"/home/clint/code/tracker/.worktrees/feat-interview-mode") with a
repository-relative path or placeholder; update the line to use
".worktrees/feat-interview-mode" or a variable like
"$REPO_ROOT/.worktrees/feat-interview-mode" so the planning doc is portable for
all contributors and does not contain a machine-specific absolute path.
- Around line 632-644: The askMode2Interview function currently blocks
indefinitely on reading from the reply channel; update askMode2Interview to use
a timed/select read (e.g., select between receiving from ch and a timeout case
or context cancellation) when waiting for MsgGateInterview replies so the
function returns a canceled/error result instead of hanging; ensure the timeout
path returns a sensible value (e.g., &handlers.InterviewResult{Canceled: true}
or a wrapped error) and still uses handlers.DeserializeInterviewResult on the
successful reply path, and keep MsgGateInterview, ch, and
DeserializeInterviewResult references intact so the change is localized to the
receive logic.
- Around line 226-229: The SerializeInterviewResult function currently ignores
the error from json.Marshal(r); update it to handle the error by returning or
bubbling it: either check the error from json.Marshal in
SerializeInterviewResult and log/return a fallback plus the error, or
(preferred) change SerializeInterviewResult's signature to return (string,
error) and propagate the marshal error to callers so callers can handle it;
reference the SerializeInterviewResult function and the InterviewResult type
when making this change and update all call sites to handle the new error
return.
In `@pipeline/handlers/human.go`:
- Around line 255-260: The current prompt handling prints a pre-fill hint
(prevAns.Answer) but then treats an empty user input as an intentional empty
answer and overwrites prevAns.Answer; change the input handling in the human
prompt/retry code paths (the blocks that print "Previous: %s" and "Enter
choice..." and the other similar branches around the noted ranges) so that when
the user submits a blank line you preserve the existing prevAns.Answer (i.e., do
not serialize an empty string); introduce an explicit token (for example "clear"
or "-") that, when entered, will intentionally clear the previous answer if
clearing must be supported, and ensure all branches that parse
choice/name/number follow this same rule (look for uses of prevAns, the code
that reads c.Reader/scan input, and any serialization of the chosen answer).
- Around line 498-510: The freeform fallback currently returns
h.executeFreeform(...) and skips writing the structured answers/answersKey and
the normal interview serialization; update the zero-questions branch to create
and persist an empty InterviewResult (e.g., zero answers, appropriate metadata)
using the same serialization/path used elsewhere (the answersKey handling)
before proceeding to freeform. Keep the prompt construction
(node.Attrs["prompt"], node.Label, agentOutput) but ensure you write the empty
InterviewResult to the same storage/context that other interview flows use (so
answersKey is set and stale data is replaced) and then call h.executeFreeform;
also ensure the persisted InterviewResult represents an empty/optional note case
so AskFreeform's non-empty-input semantics do not incorrectly mark it as an
error.
- Around line 75-90: Auto-approve logic in
AutoApproveFreeformInterviewer.AskInterview checks q.Options before q.IsYesNo,
causing questions that are both optioned and yes/no to be treated as generic
selects; change the branching to test q.IsYesNo first (or explicitly detect a
two-option yes/no pair) and set InterviewAnswer.Answer to "yes" for those cases,
and apply the same reorder/fix to the other AskInterview-like block around lines
referenced (the similar branch at the 248-325 region) so yes/no questions are
consistently auto-approved as "yes".
---
Duplicate comments:
In `@pipeline/handlers/human.go`:
- Around line 287-298: The literal "Other" path currently falls through and
readLine() errors are ignored; update the option-selection logic (the block
using fmt.Sscanf, q.Options and idx) to route both the numeric "Other" choice
(idx == len(q.Options)+1) and the literal input "Other" into the same handler:
prompt via fmt.Fprintf(c.Writer, "Enter your answer: "), call c.readLine() to
get otherLine, set ans.Answer = strings.TrimSpace(otherLine) on success, and if
c.readLine() returns an error then set the interview canceled flag (canceled)
and propagate/return the error so the interview is aborted with no partial
writes; ensure you reference c.readLine(), ans.Answer, q.Options and canceled
when making the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 918a4526-2605-4226-8ec3-e3b6de0c0e65
📒 Files selected for processing (10)
docs/superpowers/plans/2026-03-30-interview-mode.mdpipeline/context.gopipeline/dippin_adapter.gopipeline/events.gopipeline/handlers/autopilot.gopipeline/handlers/autopilot_claudecode.gopipeline/handlers/human.gopipeline/handlers/tool_test.gotui/interview_content.gotui/interviewer.go
✅ Files skipped from review due to trivial changes (3)
- pipeline/handlers/tool_test.go
- pipeline/context.go
- tui/interview_content.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pipeline/handlers/autopilot_claudecode.go
- tui/interviewer.go
- pipeline/handlers/autopilot.go
|
|
||
| **Tech Stack:** Go, charmbracelet/bubbles (textarea, viewport), lipgloss. No new dependencies (NOT huh — the codebase uses bubbles throughout). | ||
|
|
||
| **Worktree:** `/home/clint/code/tracker/.worktrees/feat-interview-mode` (branch `feat/interview-mode` from `fix/p0-critical-safety-fixes`) |
There was a problem hiding this comment.
Replace hardcoded absolute path with a relative path or placeholder.
The worktree path /home/clint/code/tracker/.worktrees/feat-interview-mode is specific to one developer's local machine and will not work for other contributors. Planning documents should use relative paths (e.g., .worktrees/feat-interview-mode) or placeholders (e.g., $REPO_ROOT/.worktrees/feat-interview-mode).
📝 Suggested fix
-**Worktree:** `/home/clint/code/tracker/.worktrees/feat-interview-mode` (branch `feat/interview-mode` from `fix/p0-critical-safety-fixes`)
+**Worktree:** `.worktrees/feat-interview-mode` (branch `feat/interview-mode` from `fix/p0-critical-safety-fixes`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **Worktree:** `/home/clint/code/tracker/.worktrees/feat-interview-mode` (branch `feat/interview-mode` from `fix/p0-critical-safety-fixes`) | |
| **Worktree:** `.worktrees/feat-interview-mode` (branch `feat/interview-mode` from `fix/p0-critical-safety-fixes`) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-03-30-interview-mode.md` at line 11, Replace the
hardcoded absolute worktree path shown after the "Worktree:" label (the string
"/home/clint/code/tracker/.worktrees/feat-interview-mode") with a
repository-relative path or placeholder; update the line to use
".worktrees/feat-interview-mode" or a variable like
"$REPO_ROOT/.worktrees/feat-interview-mode" so the planning doc is portable for
all contributors and does not contain a machine-specific absolute path.
| func SerializeInterviewResult(r InterviewResult) string { | ||
| b, _ := json.Marshal(r) | ||
| return string(b) | ||
| } |
There was a problem hiding this comment.
Handle the error from json.Marshal.
The planned implementation of SerializeInterviewResult ignores the error from json.Marshal(r). While InterviewResult contains only serializable types, silently ignoring errors is a risky pattern that could mask future issues (e.g., if the struct is modified to include non-serializable fields).
🛡️ Proposed fix
func SerializeInterviewResult(r InterviewResult) string {
- b, _ := json.Marshal(r)
- return string(b)
+ b, err := json.Marshal(r)
+ if err != nil {
+ // Log or handle the error appropriately
+ return ""
+ }
+ return string(b)
}Or better yet, change the signature to return an error:
-func SerializeInterviewResult(r InterviewResult) string {
+func SerializeInterviewResult(r InterviewResult) (string, error) {
b, err := json.Marshal(r)
- return string(b)
+ if err != nil {
+ return "", fmt.Errorf("failed to serialize interview result: %w", err)
+ }
+ return string(b), nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func SerializeInterviewResult(r InterviewResult) string { | |
| b, _ := json.Marshal(r) | |
| return string(b) | |
| } | |
| func SerializeInterviewResult(r InterviewResult) string { | |
| b, err := json.Marshal(r) | |
| if err != nil { | |
| // Log or handle the error appropriately | |
| return "" | |
| } | |
| return string(b) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-03-30-interview-mode.md` around lines 226 - 229,
The SerializeInterviewResult function currently ignores the error from
json.Marshal(r); update it to handle the error by returning or bubbling it:
either check the error from json.Marshal in SerializeInterviewResult and
log/return a fallback plus the error, or (preferred) change
SerializeInterviewResult's signature to return (string, error) and propagate the
marshal error to callers so callers can handle it; reference the
SerializeInterviewResult function and the InterviewResult type when making this
change and update all call sites to handle the new error return.
| func (b *BubbleteaInterviewer) askMode2Interview(questions []handlers.Question, prev *handlers.InterviewResult) (*handlers.InterviewResult, error) { | ||
| ch := make(chan string, 1) | ||
| b.send(MsgGateInterview{Questions: questions, Previous: prev, ReplyCh: ch}) | ||
| reply, ok := <-ch | ||
| if !ok { | ||
| return &handlers.InterviewResult{Canceled: true}, nil | ||
| } | ||
| result, err := handlers.DeserializeInterviewResult(reply) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to deserialize interview reply: %w", err) | ||
| } | ||
| return &result, nil | ||
| } |
There was a problem hiding this comment.
Add timeout to channel read to prevent indefinite blocking.
The askMode2Interview implementation reads from a channel without a timeout. If the reply never arrives (e.g., due to a bug in the message handler, race condition, or UI crash), this will block indefinitely, potentially hanging the entire pipeline execution.
⏱️ Proposed fix with context timeout
func (b *BubbleteaInterviewer) askMode2Interview(questions []handlers.Question, prev *handlers.InterviewResult) (*handlers.InterviewResult, error) {
ch := make(chan string, 1)
b.send(MsgGateInterview{Questions: questions, Previous: prev, ReplyCh: ch})
- reply, ok := <-ch
+
+ select {
+ case reply, ok := <-ch:
- if !ok {
- return &handlers.InterviewResult{Canceled: true}, nil
- }
- result, err := handlers.DeserializeInterviewResult(reply)
- if err != nil {
- return nil, fmt.Errorf("failed to deserialize interview reply: %w", err)
- }
- return &result, nil
+ if !ok {
+ return &handlers.InterviewResult{Canceled: true}, nil
+ }
+ result, err := handlers.DeserializeInterviewResult(reply)
+ if err != nil {
+ return nil, fmt.Errorf("failed to deserialize interview reply: %w", err)
+ }
+ return &result, nil
+ case <-time.After(5 * time.Minute): // Adjust timeout as appropriate
+ return nil, fmt.Errorf("interview timed out waiting for user response")
+ }
}Alternatively, if a context is available, use ctx.Done() in the select statement for cancellation support.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (b *BubbleteaInterviewer) askMode2Interview(questions []handlers.Question, prev *handlers.InterviewResult) (*handlers.InterviewResult, error) { | |
| ch := make(chan string, 1) | |
| b.send(MsgGateInterview{Questions: questions, Previous: prev, ReplyCh: ch}) | |
| reply, ok := <-ch | |
| if !ok { | |
| return &handlers.InterviewResult{Canceled: true}, nil | |
| } | |
| result, err := handlers.DeserializeInterviewResult(reply) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to deserialize interview reply: %w", err) | |
| } | |
| return &result, nil | |
| } | |
| func (b *BubbleteaInterviewer) askMode2Interview(questions []handlers.Question, prev *handlers.InterviewResult) (*handlers.InterviewResult, error) { | |
| ch := make(chan string, 1) | |
| b.send(MsgGateInterview{Questions: questions, Previous: prev, ReplyCh: ch}) | |
| select { | |
| case reply, ok := <-ch: | |
| if !ok { | |
| return &handlers.InterviewResult{Canceled: true}, nil | |
| } | |
| result, err := handlers.DeserializeInterviewResult(reply) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to deserialize interview reply: %w", err) | |
| } | |
| return &result, nil | |
| case <-time.After(5 * time.Minute): // Adjust timeout as appropriate | |
| return nil, fmt.Errorf("interview timed out waiting for user response") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-03-30-interview-mode.md` around lines 632 - 644,
The askMode2Interview function currently blocks indefinitely on reading from the
reply channel; update askMode2Interview to use a timed/select read (e.g., select
between receiving from ch and a timeout case or context cancellation) when
waiting for MsgGateInterview replies so the function returns a canceled/error
result instead of hanging; ensure the timeout path returns a sensible value
(e.g., &handlers.InterviewResult{Canceled: true} or a wrapped error) and still
uses handlers.DeserializeInterviewResult on the successful reply path, and keep
MsgGateInterview, ch, and DeserializeInterviewResult references intact so the
change is localized to the receive logic.
pipeline/handlers/human.go
Outdated
| // 0 questions or malformed → fall back to freeform with prompt | ||
| if len(questions) == 0 { | ||
| prompt := node.Attrs["prompt"] | ||
| if prompt == "" { | ||
| prompt = node.Label | ||
| } | ||
| if prompt == "" { | ||
| prompt = "No questions were generated. Please provide any input." | ||
| } | ||
| if agentOutput != "" { | ||
| prompt = prompt + "\n\n---\n" + agentOutput | ||
| } | ||
| return h.executeFreeform(node, prompt) |
There was a problem hiding this comment.
Freeform fallback bypasses the interview persistence contract.
This branch returns executeFreeform() before writing answersKey or going through the normal interview serialization path, so zero-question / malformed runs leave missing or stale structured answers in context. It also inherits AskFreeform()'s non-empty-input behavior, which turns the “optional note” case into an error. Persist an empty InterviewResult here instead of delegating straight to freeform.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pipeline/handlers/human.go` around lines 498 - 510, The freeform fallback
currently returns h.executeFreeform(...) and skips writing the structured
answers/answersKey and the normal interview serialization; update the
zero-questions branch to create and persist an empty InterviewResult (e.g., zero
answers, appropriate metadata) using the same serialization/path used elsewhere
(the answersKey handling) before proceeding to freeform. Keep the prompt
construction (node.Attrs["prompt"], node.Label, agentOutput) but ensure you
write the empty InterviewResult to the same storage/context that other interview
flows use (so answersKey is set and stale data is replaced) and then call
h.executeFreeform; also ensure the persisted InterviewResult represents an
empty/optional note case so AskFreeform's non-empty-input semantics do not
incorrectly mark it as an error.
…ug logging - Pre-fill sets selected=true so retry answers survive collectAnswers - fieldAnswered checks otherInput is non-empty for Other selections - Cancel sends partial answers (Canceled=true + current state) before closing - parseInterviewResponse builds results in question order, validates options via matchChoice, normalizes yes/no, sets Incomplete for missing answers - Raw response body logging gated behind TRACKER_DEBUG env var Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Config validation:
- ResponseFormat must be json_object or json_schema
- json_schema requires non-empty valid JSON ResponseSchema
Session reliability:
- Exhausted empty response retries now hard-fail instead of stoppedNaturally
Workflow fixes (deep_review.dip):
- SaveGoal tool node preserves original developer goal before interviews
- Analysis agents read goal from .ai/review/goal.txt
- Empty priority questions allowed when no actionable findings
- Revise loop includes ReviewPlan feedback via ${ctx.human_response}
Interview UX:
- IsYesNo takes precedence over Options in all code paths
- Answered summary capped to viewport height
- Blank retry input preserves previous answer (console)
- Freeform fallback sets answersKey in ContextUpdates
- Autopilot includes previous answers in retry prompt
Other:
- HTML-escape Mermaid arrows in workflows.html
- interview-loop.dip prompts include ${ctx.interview_answers}
- Added ResponseFormat validation and blank-preserve tests
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…re-commit CI failed because examples/ had stale copies of the 3 existing workflows and deep_review.dip was missing from examples/ entirely. - Copied embedded workflows to examples/ (source of truth for CI) - Added deep_review.dip to Makefile sync-workflows and check-workflows targets - Added workflow sync check to .pre-commit hook (gate #8) - Added deep_review.dip to dippin lint list in .pre-commit Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
mode: interviewfor human gate nodes per feat: implement interview mode for human nodes (huh forms) #46ctx[answers_key]and markdown summary toctx["human_response"]Changes
New files (8):
pipeline/handlers/interview_parse.go— pure functionParseQuestions(markdown) []Questionwith emphasis stripping and 100-question cappipeline/handlers/interview_result.go—InterviewResult/InterviewAnswertypes, JSON round-trip, markdown summarytui/interview_content.go— fullscreen multi-field form modal (bubbles-based, not huh)Modified files (10):
pipeline/handlers/human.go—InterviewInterviewerinterface,executeInterview(), auto-approve + consoleAskInterviewpipeline/dippin_adapter.go— extractquestions_key,answers_key,promptfrom IRpipeline/context.go—ContextKeyInterviewQuestions/ContextKeyInterviewAnswersconstantstui/interviewer.go—BubbleteaInterviewer.AskInterview(Mode 1 + Mode 2)tui/messages.go—MsgGateInterviewmessage typetui/app.go— handleMsgGateInterviewin modal switchpipeline/handlers/autopilot.go—AutopilotInterviewer.AskInterviewwith prompt injection documentationpipeline/handlers/autopilot_claudecode.go—ClaudeCodeAutopilotInterviewer.AskInterviewwith documented fallback behaviortui/autopilot_interviewer.go—AutopilotTUIInterviewer.AskInterviewgo.mod— bump dippin-lang to v0.15.0-beta.1Dependencies
HumanConfig.QuestionsKey/AnswersKey/Promptfields. Will update to stable v0.15.0 before release.Test plan
go build ./...passesgo test ./... -short— all packages pass (1 pre-existing failure inTestToolHandlerWritesStatusArtifactunrelated).dippipeline usingmode: interviewdippin doctoron example pipelines after dippin-lang stable releaseCloses #46
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores
Tests
Bug Fixes