Feat/captain subagent history#13
Conversation
Promote google/uuid to a direct dependency to support the new spec frontmatter parsing feature. Add decodeSpecFrontmatter function that re-encodes the raw dotprompt frontmatter map as YAML and unmarshals it into ai.Request, allowing spec-native keys (permissions, memory, budget, context, maxTurns) to be declared directly in .prompt files. The dotprompt config block remains canonical for the three knobs it owns (maxOutputTokens, temperature, reasoning). Extend test coverage with new fixtures for all supported backends (Anthropic, ClaudeAgent, ClaudeCLI, CodexCLI, Gemini, GeminiCLI, OpenAI) and a dedicated spec frontmatter test.
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (36)
WalkthroughThis PR adds canonical ChangesCaptain: api types, cmux provider, CLI commands, and web UI
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| function git(args: string, fallback: string) { | ||
| try { | ||
| return execSync(`git -C ${JSON.stringify(clickyPackageRoot)} ${args}`, { |
![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
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/ai/agent.go (1)
89-97: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the same model fallback in the success response.
accrue()now falls back toa.cfg.Model.Namewhen providers leaveresp.Modelempty, butExecutePrompt()still returnsPromptResponse.Model = resp.Modelon success. That leaves successful responses with a blank model while the associated cost record uses the configured one.Proposed fix
cost := a.accrue(resp) + model := resp.Model + if model == "" { + model = a.cfg.Model.Name + } return &PromptResponse{ Request: req, Result: resp.Text, StructuredData: resp.StructuredData, Costs: Costs{cost}, - Model: resp.Model, + Model: model, Duration: time.Since(start), CacheHit: resp.CacheHit, }, nil }Also applies to: 160-164
🤖 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 `@pkg/ai/agent.go` around lines 89 - 97, ExecutePrompt() is returning a blank PromptResponse.Model when providers omit resp.Model, while accrue() already applies the configured fallback. Update the success response construction in ExecutePrompt() to use the same fallback logic as accrue(), so PromptResponse.Model is set to resp.Model when present or a.cfg.Model.Name otherwise. Make the same change in the other return path referenced by the review so both success responses stay consistent with the cost record.pkg/ai/models_remote_test.go (1)
138-140: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClear
GOOGLE_API_KEYin the missing-key test.
GetAPIKeyFromEnv(BackendGemini)falls back toGOOGLE_API_KEY, so this test can become host-environment dependent unless that variable is cleared too.Suggested fix
t.Setenv("OPENAI_API_KEY", "") t.Setenv("ANTHROPIC_API_KEY", "") t.Setenv("GEMINI_API_KEY", "") + t.Setenv("GOOGLE_API_KEY", "")🤖 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 `@pkg/ai/models_remote_test.go` around lines 138 - 140, The missing-key test is still dependent on host environment because GetAPIKeyFromEnv(BackendGemini) can read GOOGLE_API_KEY as a fallback even when GEMINI_API_KEY is empty. Update the test setup in the missing-key case to clear GOOGLE_API_KEY alongside OPENAI_API_KEY, ANTHROPIC_API_KEY, and GEMINI_API_KEY so the behavior is fully isolated and deterministic.pkg/cli/ai.go (1)
53-80: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--budget 0cannot disable a saved budget cap.
parseFloatFlagreturns0for both “flag omitted” and--budget 0, and Line 71 then always falls back tosaved.BudgetUSD. After a user saves a budget incaptain configure, commands that go throughToConfig()can no longer request the documented “0 = unlimited” behavior from the CLI.🤖 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 `@pkg/cli/ai.go` around lines 53 - 80, The AIProviderOptions.ToConfig flow is treating an explicit --budget 0 the same as an omitted flag, so it incorrectly falls back to saved.BudgetUSD. Update ToConfig to distinguish “flag not provided” from an explicit zero value when using parseFloatFlag, and only inherit the saved budget when the budget flag was actually unset. Keep the existing validation and config assembly in AIProviderOptions.ToConfig, but ensure api.Budget{Cost: budget} can remain 0 to mean unlimited.
🟠 Major comments (23)
pkg/ai/provider/genkit/options.go-125-128 (1)
125-128: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve
Prompt.AppendSystemin Genkit requests.
AppendSystemis now ignored for Genkit providers, so callers using appended system instructions lose them.Proposed fix
- if req.Prompt.System != "" { - opts = append(opts, gkai.WithSystem(req.Prompt.System)) + system := req.Prompt.System + if req.Prompt.AppendSystem != "" { + if system != "" { + system += "\n\n" + } + system += req.Prompt.AppendSystem + } + if system != "" { + opts = append(opts, gkai.WithSystem(system)) } opts = append(opts, gkai.WithPrompt(req.Prompt.User))🤖 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 `@pkg/ai/provider/genkit/options.go` around lines 125 - 128, `genkit/options.go` is dropping appended system instructions because `req.Prompt.System` is the only system text passed into `gkai.WithSystem`. Update the request-building logic in `options.go` to preserve `Prompt.AppendSystem` for Genkit providers by combining it with the existing system prompt before calling `gkai.WithSystem`, while still passing `req.Prompt.User` through `gkai.WithPrompt`.pkg/ai/provider/genkit/genkit.go-65-65 (1)
65-65: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor
cfg.Model.IDwhen constructing the Genkit model ref.
pkg/api/model.godefinesIDas the provider-specific override for cases where the backend id differs from the catalog name. Buildingreffromcfg.Model.Namealone throws that away, so Genkit-specific ids can resolve the wrong model or fail outright.Proposed fix
- ref, err := modelRef(backend, cfg.Model.Name) + modelID := cfg.Model.ID + if modelID == "" { + modelID = cfg.Model.Name + } + ref, err := modelRef(backend, modelID)🤖 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 `@pkg/ai/provider/genkit/genkit.go` at line 65, The Genkit model reference is being built from cfg.Model.Name only, which ignores the provider-specific override in cfg.Model.ID. Update genkit.go so the ref construction in the code path using modelRef and cfg.Model includes cfg.Model.ID when present, falling back to cfg.Model.Name only when ID is empty. Make the change in the logic around modelRef(backend, ...) so Genkit resolves the intended backend model identifier.pkg/ai/provider/gemini_cli.go-48-50 (1)
48-50: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the full prompt when calling Gemini CLI.
This now drops
Prompt.SystemandPrompt.AppendSystementirely. Any prompt template that relies on system instructions will behave differently on Gemini CLI than on the other provider paths.Proposed fix
cliReq := map[string]string{ - "prompt": req.Prompt.User, + "prompt": composePrompt(req), "model": g.model, }🤖 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 `@pkg/ai/provider/gemini_cli.go` around lines 48 - 50, The Gemini CLI request building in GeminiCli should preserve the complete prompt instead of using only req.Prompt.User. Update the prompt assembly logic in the GeminiCLI provider so Prompt.System and Prompt.AppendSystem are included alongside the user content before populating the cliReq payload, matching the behavior of the other provider paths and keeping system instructions intact.pkg/ai/history/codex_parser.go-79-80 (1)
79-80: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not stop at
thread.startedbefore reading later session metadata.
ReadCodexSessionInfonow explicitly handlesthread.startedas something that can precede richer metadata. Returning here meansReadCodexSessionMetanever sees a followingsession_meta, so cheap history filtering loses CWD/git/provider fields and can exclude valid sessions.Proposed fix
func ReadCodexSessionMeta(sessionFile string) (*CodexSessionInfo, error) { file, err := os.Open(sessionFile) if err != nil { return nil, err } defer func() { _ = file.Close() }() scanner := bufio.NewScanner(file) scanner.Buffer(make([]byte, 0, 64*1024), 10*1024*1024) + var info *CodexSessionInfo for scanner.Scan() { line := strings.TrimSpace(scanner.Text()) if line == "" { continue } @@ switch event.Type { case "session_meta": - info := &CodexSessionInfo{ - ID: event.Payload.ID, - CWD: event.Payload.CWD, - CLIVersion: event.Payload.CLIVersion, - ModelProvider: event.Payload.ModelProvider, - Originator: event.Payload.Originator, - StartedAt: event.Time(), - } + if info == nil { + info = &CodexSessionInfo{} + } + if info.ID == "" { + info.ID = event.Payload.ID + } + info.CWD = event.Payload.CWD + info.CLIVersion = event.Payload.CLIVersion + info.ModelProvider = event.Payload.ModelProvider + info.Originator = event.Payload.Originator + if info.StartedAt == nil { + info.StartedAt = event.Time() + } if event.Payload.Git != nil { info.GitBranch = event.Payload.Git.Branch info.GitCommit = event.Payload.Git.CommitHash } - return info, nil case "thread.started": - return &CodexSessionInfo{ID: event.ThreadID, StartedAt: event.Time()}, nil + if info == nil { + info = &CodexSessionInfo{StartedAt: event.Time()} + } + if info.ID == "" { + info.ID = event.ThreadID + } case "turn_context", "response_item", "event_msg", "item.completed", "turn.failed", "error": - return nil, nil + return info, nil } } if err := scanner.Err(); err != nil { return nil, err } - return nil, nil + return info, nil }🤖 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 `@pkg/ai/history/codex_parser.go` around lines 79 - 80, The `ReadCodexSessionInfo` handling for `thread.started` returns too early and prevents later `session_meta` data from being read. Update the parser flow in `codex_parser.go` so `thread.started` only initializes `CodexSessionInfo` and then continues scanning for richer metadata, allowing `ReadCodexSessionMeta` to pick up CWD/git/provider fields. Use the `ReadCodexSessionInfo` and `ReadCodexSessionMeta` paths as the key places to adjust control flow without breaking existing event parsing.pkg/ai/provider/cmux/stall.go-33-36 (1)
33-36: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTighten approval-dialog detection before holding the stall clock.
The current regex matches any visible “do you want to” text, including normal prompts or model output. Since
awaitingHumanthen skips stall checks, a non-dialog phrase can prevent stalled runs from ever failing. Require Claude approval-dialog structure/choices before treating the screen as awaiting approval.Also applies to: 185-187, 209-219
🤖 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 `@pkg/ai/provider/cmux/stall.go` around lines 33 - 36, The approval-prompt detection in approvalPromptRe is too broad and can misclassify ordinary text as a human approval dialog, which causes awaitingHuman to bypass stall checks incorrectly. Tighten the matching in stall.go so it only returns true for Claude-style approval dialogs with the expected structure and explicit choices/buttons, and update the awaitingHuman path to rely on that stricter signal before suspending stall detection.pkg/ai/provider/cmux/provider.go-118-139 (1)
118-139: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not return success when the stream closes without
EventResult.On cancellation,
drivecan skip emittingEventError/EventResultbecauseemithonors the canceled context.Executethen falls through and returns a successful empty response. Treat missingEventResultas cancellation or execution failure.🐛 Proposed fix
- if !sawResult && lastErr != "" { - return nil, fmt.Errorf("%w: %s", ai.ErrCLIExecutionFailed, lastErr) + if !sawResult { + if lastErr != "" { + return nil, fmt.Errorf("%w: %s", ai.ErrCLIExecutionFailed, lastErr) + } + if err := ctx.Err(); err != nil { + return nil, err + } + return nil, fmt.Errorf("%w: cmux stream closed without a result", ai.ErrCLIExecutionFailed) }🤖 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 `@pkg/ai/provider/cmux/provider.go` around lines 118 - 139, The success path in `cmux/provider.go`’s `Execute` currently returns an empty `ai.Response` even when `drive` finishes without ever producing `EventResult`; update the post-stream handling to detect that case and return `ai.ErrCLIExecutionFailed` instead of success. Use the existing `sawResult`, `success`, and `lastErr` checks near the end of `Execute` to treat a missing result as cancellation or execution failure, and keep the existing failure-return behavior consistent with the `EventError`/`EventResult` flow.pkg/ai/provider/cmux/sessionlog.go-30-42 (1)
30-42: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate
sessionIDbefore joining it into a filesystem path.
filepath.Join(..., sessionID+".jsonl")allows../or path separators from resume/configured session IDs to escape the expected Claude project directory. Reject non-basename session IDs before constructing the path.🛡️ Proposed fix
import ( "bytes" "context" "errors" "fmt" "io" "os" "path/filepath" + "strings" "time" @@ func SessionLogPath(workDir, sessionID string) (string, error) { + sessionID = strings.TrimSpace(sessionID) if sessionID == "" { return "", fmt.Errorf("session id is required") } + if strings.ContainsAny(sessionID, `/\`) || sessionID == "." || sessionID == ".." { + return "", fmt.Errorf("invalid session id %q", sessionID) + } abs, err := filepath.Abs(workDir)🤖 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 `@pkg/ai/provider/cmux/sessionlog.go` around lines 30 - 42, `SessionLogPath` currently joins `sessionID` directly into the session log path, which allows path traversal via separators or `..` values. Add a validation step in `SessionLogPath` to reject any `sessionID` that is not a plain basename before calling `filepath.Join`, and return an error for invalid IDs so only safe session log filenames are constructed.pkg/ai/provider/cmux/submit.go-121-125 (1)
121-125: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDon’t log the full prompt payload.
At Line 124,
textis dumped verbatim into debug logs. That payload can include source code, credentials, and user input, so this turns routine troubleshooting into a log exfiltration path.Safer logging
log.Infof("cmux: sending %s to workspace %s surface %s (attempt %d/%d)", label, workspaceRef, surfaceRef, attempt, attempts) log.Debugf("cmux command: cmux send --workspace %q --surface %q -- <%s>", workspaceRef, surfaceRef, label) log.Debugf("cmux command: cmux send-key --workspace %q --surface %q Enter", workspaceRef, surfaceRef) - log.Debugf("cmux send payload:\n%s", text) + log.Debugf("cmux: %s payload bytes=%d", label, len(text))🤖 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 `@pkg/ai/provider/cmux/submit.go` around lines 121 - 125, The debug logging in SendSurface is leaking the full prompt payload by printing text verbatim, which can expose sensitive content. Update the cmux submit flow in submit.go so the log statement around the SendSurface call no longer includes raw text; keep only metadata like workspaceRef, surfaceRef, label, and attempt information, or replace the payload with a redacted/length-only summary. Use the existing SendSurface and log.Debugf call sites to locate the change.pkg/ai/provider/cmux/sessionstats.go-500-507 (1)
500-507: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winStop serving finished sessions from the live map.
Finish()only clearsInProgress, butGet()still prefers the accumulator as long as it remains inc.live. That means any lines flushed after the tailer stops — or a resumed session writing to the same log — stay invisible until this entry happens to be evicted, so callers can see permanently stale stats.Proposed fix
func (c *SessionStatsCache) Get(sessionID, path string) (SessionStats, error) { c.mu.Lock() acc := c.live[sessionID] c.mu.Unlock() if acc != nil { - return acc.snapshot(), nil + s := acc.snapshot() + if s.InProgress { + return s, nil + } + c.mu.Lock() + delete(c.live, sessionID) + c.mu.Unlock() } return c.coldStats(sessionID, path) }🤖 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 `@pkg/ai/provider/cmux/sessionstats.go` around lines 500 - 507, The SessionStatsCache.Get path is still returning a live accumulator for finished sessions, because it prefers c.live entries even after Finish() has cleared InProgress. Update Get() (and/or the session lifecycle around Finish() and c.live) so it only serves live stats while a session is actually in progress; once finished, fall back to coldStats() or remove the accumulator from c.live so session snapshots stay current for SessionStatsCache.Get and SessionStatsCache.Finish.pkg/ai/middleware/caching.go-25-25 (1)
25-25: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCache key now ignores the non-user prompt state.
After the
api.Promptrefactor, response-shaping fields live outsideUser(System,AppendSystem, andSchema). Keying and storing the cache entry with onlyreq.Prompt.Usermeans two requests with identical user text but different instructions/output contracts can now collide and return the wrong cached response. Please derive the cache key from the full response-shaping prompt state instead of only the user text.Also applies to: 53-53
🤖 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 `@pkg/ai/middleware/caching.go` at line 25, The cache key in the caching middleware only uses req.Prompt.User, which can cause collisions when System, AppendSystem, or Schema differ. Update the cache lookup and storage in the caching middleware (the Get/Set path around the cache entry handling) to derive the key from the full response-shaping prompt state, including the non-user fields from api.Prompt, while keeping the model-specific part via c.provider.GetModel().pkg/api/model.go-29-35 (1)
29-35: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExplicit cmux models are rejected by the canonical
Modelhelpers.
BackendClaudeCmuxandBackendCodexCmuxare declared inpkg/api/enums.go, butBackend.Valid()excludes them becauseAllBackends()only lists user-facing backends. That meansModel{Backend: BackendClaudeCmux}fails bothResolveBackend()andValidate(), which blocks the new cmux provider path from using the canonicalapi.Model. Please split user-facing backend validation from internal backend support, or allow the internal cmux backends through this path.Also applies to: 53-55
🤖 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 `@pkg/api/model.go` around lines 29 - 35, The canonical Model helpers are rejecting internal cmux backends because ResolveBackend() and Validate() rely on Backend.Valid()/AllBackends(), which only cover user-facing values. Update the backend validation path in Model so BackendClaudeCmux and BackendCodexCmux are accepted by canonical api.Model while keeping any user-facing list separate; use the existing ResolveBackend and Validate methods (and any backend list helpers they call) to split internal backend support from public validation.pkg/ai/prompt/prompt.go-164-165 (1)
164-165: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
cfg.Model.Effortaligned with theconfig.reasoningoverride.Line 164 only updates
req.Effort. If a prompt mixes spec-nativeeffortwithconfig.reasoning,Renderreturns a request and config that disagree, even thoughRunAIPromptpassescfgintoai.NewProvider. That drops the documented “config wins” behavior for any provider that readscfg.Model.Effort.Proposed fix
if s, ok := c["reasoning"].(string); ok { - req.Effort = api.Effort(s) + effort := api.Effort(s) + req.Effort = effort + cfg.Model.Effort = effort }🤖 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 `@pkg/ai/prompt/prompt.go` around lines 164 - 165, The `config.reasoning` override in `Render` currently updates only `req.Effort`, leaving `cfg.Model.Effort` out of sync and causing `RunAIPrompt`/`ai.NewProvider` to see conflicting values. Update the `Render` path in `prompt.go` so the same `reasoning` branch also applies to `cfg.Model.Effort`, keeping the returned request and config aligned; use the existing `req.Effort`, `cfg.Model.Effort`, and `config.reasoning` handling in `Render` to make the override consistent.pkg/cli/sessions.go-150-164 (1)
150-164: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t return the first prefix match.
This loop treats key matches, exact session-id matches, and prefix matches as the same first-hit condition. If two sessions share a prefix—or an exact match appears later in the slice—
sessions getcan open the wrong transcript based purely on discovery order.Suggested fix
func RunSessionGet(opts SessionGetOptions) (SessionRecord, error) { @@ - for _, candidate := range candidates { - if candidate.record.Key != id && candidate.record.ID != id && !strings.HasPrefix(candidate.record.ID, id) { - continue - } - record, err := loadSessionDetail(candidate) - if err != nil { - return SessionRecord{}, err - } - return record, nil - } - return SessionRecord{}, fmt.Errorf("session %q not found", id) + var exact []sessionCandidate + var prefix []sessionCandidate + for _, candidate := range candidates { + switch { + case candidate.record.Key == id || candidate.record.ID == id: + exact = append(exact, candidate) + case strings.HasPrefix(candidate.record.ID, id): + prefix = append(prefix, candidate) + } + } + matches := exact + if len(matches) == 0 { + matches = prefix + } + switch len(matches) { + case 0: + return SessionRecord{}, fmt.Errorf("session %q not found", id) + case 1: + record, err := loadSessionDetail(matches[0]) + if err != nil { + return SessionRecord{}, err + } + return record, nil + default: + return SessionRecord{}, fmt.Errorf("session %q is ambiguous; use the full key or --source", id) + } }🤖 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 `@pkg/cli/sessions.go` around lines 150 - 164, The session lookup in discover/load flow is returning the first prefix match too early, which can select the wrong transcript when multiple candidates share an ID prefix. Update the matching logic in the session retrieval loop in sessions.go so exact matches on candidate.record.Key or candidate.record.ID are preferred and returned immediately, while prefix matches are only accepted if no exact match exists; use discoverSessionCandidates and loadSessionDetail as the key symbols to locate the flow.pkg/cli/whoami.go-213-235 (1)
213-235: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the live model probes with a timeout.
Each goroutine calls
ai.ListModels(context.Background(), backend)without cancellation, so one slow provider can hangcaptain whoami --modelsindefinitely. Please use a per-backend timeout or plumb the command context through this helper.🤖 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 `@pkg/cli/whoami.go` around lines 213 - 235, The live model probe in fetchAPIModels currently calls ai.ListModels with context.Background(), so a slow backend can block captain whoami --models forever. Update fetchAPIModels to accept and use the command context, or create a per-backend timeout context inside the goroutine before calling ai.ListModels. Make sure the timeout is applied for each backend probe and that the context is canceled after the call.pkg/cli/serve.go-153-170 (1)
153-170: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBuild listener and browser URLs with proper host/port helpers.
These paths all concatenate
host:portmanually. That produces invalid addresses for IPv6 hosts (for example::1:8080) and--openwill try to launch unroutable URLs likehttp://0.0.0.0:8080/. Usenet.JoinHostPortfor bind addresses and construct the displayed/opened URLs from aurl.URL.Also applies to: 191-195, 315-320
🤖 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 `@pkg/cli/serve.go` around lines 153 - 170, The server address and displayed/opened URLs are still being built by concatenating host and port, which breaks IPv6 and produces bad browser links for non-routable bind hosts. Update the address construction around the http.Server setup in serve.go to use net.JoinHostPort for the bind addr, and build the printed/opened endpoints from a url.URL derived from the actual listening host rather than the raw opts.Host. Apply the same fix in the later URL-printing/open logic at the other listed spots so all references use the same helper-based host/port handling.pkg/cli/serve.go-239-248 (1)
239-248: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAvoid persisting orphan threads on partial failure.
pkg/cli/chat_thread_store.go:32-55persistsCreate()immediately, andpkg/cli/chat_thread_store.go:124-139persistsSetProviderSession()in a second write. If the second write fails, this handler returns500after already creating a thread with noproviderSessionId, so client retries can accumulate duplicates. Roll the create back on error, or persist the session ID in the initial write.🤖 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 `@pkg/cli/serve.go` around lines 239 - 248, The thread creation flow in the serve handler leaves an orphan record when `store.SetProviderSession` fails after `store.Create` has already persisted the thread. Update the `serve.go` handler to either include `ProviderSessionID` in the initial `store.Create` write, or explicitly roll back/delete the created thread if the second write fails, using the `store.Create`, `store.SetProviderSession`, and `store.Get` path to keep the operation atomic from the client’s perspective.pkg/cli/webapp/src/session.ts-43-57 (1)
43-57: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't silently remap non-Claude agents to Claude.
For anything outside the codex/claude special cases, Line 57 falls back to
claude-agent-sonnet.pkg/cli/webapp/src/AgentLauncher.tsx:30-73uses that value when it creates the follow-up thread, so gemini/openai launches end up resuming the returned provider session under the wrong chat model. That will misroute follow-up turns or fail thread resume. Please derive the thread model from every supported backend, or surface an explicit “follow-up chat unsupported” error instead of defaulting 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 `@pkg/cli/webapp/src/session.ts` around lines 43 - 57, The fallback in agentModelFor is incorrectly remapping non-Claude backends to the Claude default, which then causes AgentLauncher to resume follow-up threads under the wrong model. Update agentModelFor so it returns a provider-specific thread model for every supported backend or explicitly rejects unsupported follow-up chat in AgentLauncher, rather than falling through to DEFAULT_AGENT_MODEL; keep the codex and Claude special cases intact while handling gemini/openai separately.pkg/cli/webapp/vite.config.ts-13-15 (1)
13-15: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't default
vite serveto a siblingclicky-uicheckout.Line 14 enables source aliases for every dev-server run, but Lines 45-90 then redirect imports into
../../../../clicky-ui/packages/ui/src. On a normal clone without that sibling repo,captain serve --devfails before the UI starts. Please gate source mode behind an existence check, or make it opt-in instead of the default.Suggested fix
+import { existsSync, readFileSync } from "node:fs"; -import { readFileSync } from "node:fs"; @@ export default defineConfig(({ command }) => { const useClickySource = - command === "serve" && process.env.CAPTAIN_UI_CLICKY_SOURCE !== "0"; + command === "serve" && + process.env.CAPTAIN_UI_CLICKY_SOURCE !== "0" && + existsSync(clickyPackageRoot);🤖 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 `@pkg/cli/webapp/vite.config.ts` around lines 13 - 15, The dev-server source aliasing in vite.config.ts is defaulting to a sibling clicky-ui checkout, which breaks normal captain serve --dev runs when that repo is absent. Update the useClickySource logic so source mode is only enabled when the target checkout actually exists, or make it explicitly opt-in via CAPTAIN_UI_CLICKY_SOURCE; keep the aliasing block that points to ../../../../clicky-ui/packages/ui/src gated behind that check.pkg/cli/webapp/vite.config.ts-115-120 (1)
115-120: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid shell interpolation when running
git.execSync()still invokes a shell here, andJSON.stringify(clickyPackageRoot)does not neutralize$()/backticks inside the path. UseexecFileSync()/spawnSync()with an argument array instead.🤖 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 `@pkg/cli/webapp/vite.config.ts` around lines 115 - 120, The git helper in vite.config.ts is still using a shell via execSync, and JSON.stringify(clickyPackageRoot) does not prevent shell injection from the path. Update the git(args, fallback) helper to run git with execFileSync() or spawnSync() using an argument array instead of string interpolation, while keeping the same fallback behavior and preserving the existing git helper call sites.Source: Linters/SAST tools
pkg/cli/ai_prompt_file.go-54-78 (1)
54-78: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftExplicit zero CLI overrides are lost when a prompt file already sets temperature or budget.
overlayCLItreats0as “unset” for both fields: Line 69 skips--temperature 0, and Line 77 ignores--budget 0. That means a.promptfile withtemperature: 0.7orbudget.cost: 5cannot be cleared from the CLI, even though0is a valid documented value for both flags.🤖 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 `@pkg/cli/ai_prompt_file.go` around lines 54 - 78, The overlayCLI logic is dropping explicit zero values for temperature and budget because it treats 0 as “not provided.” Update the merge behavior in overlayCLI so that CLI flags can distinguish unset from an explicit 0 for o.Temperature and o.Budget, and then apply 0 as a valid override to req.Model.Temperature and req.Budget.Cost instead of falling back to saved or base values.pkg/cli/ai_models.go-104-115 (1)
104-115: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--backend geminiis documented here but rejected at runtime.Line 70 advertises Gemini, and
pkg/cli/configure.goalready treats Gemini as a live API-backed model source, but this switch only allows OpenAI, Anthropic, or CLI backends. The result is a broken documented command path forcaptain ai models --backend gemini.🤖 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 `@pkg/cli/ai_models.go` around lines 104 - 115, The backend validation in runLiveModels is too restrictive and rejects the documented Gemini path. Update the switch on backendFilter in runLiveModels so `--backend gemini` is accepted alongside the existing live API-backed backends, and keep the error branch only for truly unsupported values. Make sure the allowed-backend check stays consistent with the CLI documentation and the backend handling used by configure.go, while preserving the existing CLI/backend catalog shortcut.pkg/cli/webapp/src/App.tsx-165-172 (1)
165-172: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard route decoding against malformed URLs.
Lines 167 and 171 can throw
URIErroron a bad%escape, which takes down the whole SPA instead of falling back to an invalid-route state.💡 Suggested change
+function safeDecodeURIComponent(value: string) { + try { + return decodeURIComponent(value); + } catch { + return value; + } +} + function parseRoute(pathname: string, search: string): Route { if (pathname.startsWith("/operations")) return { kind: "operations" }; if (pathname.startsWith("/sessions")) { const raw = pathname.slice("/sessions".length).replace(/^\/+/, ""); - const sessionId = raw ? decodeURIComponent(raw.split("/")[0] ?? "") : undefined; + const sessionId = raw ? safeDecodeURIComponent(raw.split("/")[0] ?? "") : undefined; return sessionId ? { kind: "sessions", sessionId } : { kind: "sessions" }; } if (pathname.startsWith("/chat/")) { - const threadId = decodeURIComponent(pathname.slice("/chat/".length).split("/")[0] ?? ""); + const threadId = safeDecodeURIComponent( + pathname.slice("/chat/".length).split("/")[0] ?? "", + ); const model = new URLSearchParams(search).get("model") || undefined; return threadId ? { kind: "chat", threadId, model } : { kind: "launcher" }; }🤖 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 `@pkg/cli/webapp/src/App.tsx` around lines 165 - 172, Guard the route parsing in App.tsx so malformed percent-escapes do not throw during navigation. In the route handling logic that builds the sessions and chat route objects, wrap the decodeURIComponent calls used for sessionId and threadId in a safe helper or try/catch and fall back to an invalid-route state when decoding fails. Keep the fix localized to the pathname parsing branch in App’s route resolver so bad URLs no longer crash the SPA..gitignore-14-16 (1)
14-16: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReorder the
dist/ignore and exception.Line 16 re-ignores
pkg/cli/webapp/dist/after Lines 14-15 unignore it, so the exception never takes effect.💡 Suggested change
pkg/cli/webapp/node_modules/ pkg/cli/webapp/*.tsbuildinfo -!pkg/cli/webapp/dist/ -!pkg/cli/webapp/dist/** dist/ +!pkg/cli/webapp/dist/ +!pkg/cli/webapp/dist/** .grite/🤖 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 @.gitignore around lines 14 - 16, The .gitignore rules for pkg/cli/webapp/dist are ordered incorrectly, so the later dist/ ignore overrides the earlier exception. Update the ignore entries so the generic dist/ rule is placed before the pkg/cli/webapp/dist/ unignore patterns, and keep the related exception entries together in the correct order to ensure the webapp dist directory is actually tracked.
🧹 Nitpick comments (3)
pkg/ai/models_remote_test.go (1)
150-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the new cmux backends in the rejection matrix.
ListModelsnow rejects all non-API backends, but this test only pins the older CLI/agent set. AddingBackendClaudeCmuxandBackendCodexCmuxwould keep the contract aligned with the expanded backend enum.Suggested fix
-// TestListModels_RejectsCLIBackends pins that CLI/agent backends have no live +// TestListModels_RejectsNonAPIBackends pins that non-API backends have no live // listing path: they authenticate internally and enumerate from the static // catalog (pkg/cli), so ListModels must fail loud rather than require an API key. -func TestListModels_RejectsCLIBackends(t *testing.T) { +func TestListModels_RejectsNonAPIBackends(t *testing.T) { t.Setenv("OPENAI_API_KEY", "sk-test") t.Setenv("ANTHROPIC_API_KEY", "ant-test") t.Setenv("GEMINI_API_KEY", "g-test") - for _, b := range []Backend{BackendClaudeCLI, BackendClaudeAgent, BackendCodexCLI, BackendGeminiCLI} { + for _, b := range []Backend{ + BackendClaudeCLI, + BackendClaudeAgent, + BackendCodexCLI, + BackendGeminiCLI, + BackendClaudeCmux, + BackendCodexCmux, + } { if _, err := ListModels(context.Background(), b); err == nil { - t.Errorf("backend=%s: expected error (no live listing for CLI/agent backends)", b) + t.Errorf("backend=%s: expected error (no live listing for non-API backends)", b) } } }🤖 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 `@pkg/ai/models_remote_test.go` around lines 150 - 163, The ListModels rejection test only covers the older CLI/agent backends, so extend the backend set in TestListModels_RejectsCLIBackends to also include BackendClaudeCmux and BackendCodexCmux. Update the loop over Backend values in this test so it asserts ListModels fails for every non-API backend now supported by the enum, keeping the contract in sync with ListModels and the Backend constants.pkg/cli/ai_filters_test.go (1)
68-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the model-completion test deterministic.
This assertion is pinned to whatever default
aichatcatalog ships today, so a dependency bump can fail the test even if the prefix-stripping logic is still correct.pkg/cli/ai_catalog_test.go:15-28already has the safer pattern: install a tiny synthetic catalog here and assert against that.🤖 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 `@pkg/cli/ai_filters_test.go` around lines 68 - 77, The model suggestion test is currently dependent on the external default aichat catalog, so it can break on dependency changes even when the prefix-stripping behavior is correct. Update TestAIFilters_ModelSuggestsBareNames in ai_filters_test.go to use a small synthetic catalog like the pattern already used in ai_catalog_test.go, then assert the bare-name output against that fixed data via optionKeys and contains instead of relying on whatever catalog ships by default.pkg/cli/webapp/src/SessionBrowser.tsx (1)
58-65: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDebounce the session search before refetching.
Line 63 ties the request directly to the raw input value, so each keystroke kicks off a new session scan. A short debounce will make the filter feel much cheaper.
💡 Suggested change
const [source, setSource] = useState<SourceFilter>("all"); const [allProjects, setAllProjects] = useState(false); const [query, setQuery] = useState(""); + const [debouncedQuery, setDebouncedQuery] = useState(""); + + useEffect(() => { + const timer = window.setTimeout(() => setDebouncedQuery(query), 250); + return () => window.clearTimeout(timer); + }, [query]); const listQuery = useQuery({ - queryKey: ["sessions", source, allProjects, query], - queryFn: () => fetchSessions({ source, allProjects, query }), + queryKey: ["sessions", source, allProjects, debouncedQuery], + queryFn: () => fetchSessions({ source, allProjects, query: debouncedQuery }), });🤖 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 `@pkg/cli/webapp/src/SessionBrowser.tsx` around lines 58 - 65, The session search is refetching on every keystroke because the raw query state is used directly in SessionBrowser’s useQuery and fetchSessions call. Introduce a debounced version of the query input in SessionBrowser, and use that debounced value in the queryKey and queryFn so fetchSessions only runs after typing pauses. Keep the existing source and allProjects filters unchanged, and update the query state handling so the UI still reflects the immediate input while the network request uses the delayed value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad47f855-3c7d-47ac-a338-01017fc8519f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (127)
.gitignoreREADME.mdcmd/captain/main.gogo.modpkg/ai/agent.gopkg/ai/agent/runner.gopkg/ai/agent/runner_test.gopkg/ai/agent/scope_test.gopkg/ai/agent_test.gopkg/ai/client.gopkg/ai/history/codex_parser.gopkg/ai/history/codex_parser_test.gopkg/ai/loop_test.gopkg/ai/middleware/caching.gopkg/ai/middleware/cost.gopkg/ai/middleware/logging.gopkg/ai/middleware/logging_test.gopkg/ai/models_remote.gopkg/ai/models_remote_test.gopkg/ai/pricing/registry.gopkg/ai/pricing/registry_test.gopkg/ai/prompt/prompt.gopkg/ai/prompt/prompt_test.gopkg/ai/prompt/testdata/fixtures/anthropic-claude-opus.promptpkg/ai/prompt/testdata/fixtures/anthropic-claude-sonnet.promptpkg/ai/prompt/testdata/fixtures/claude-agent-opus.promptpkg/ai/prompt/testdata/fixtures/claude-agent-sonnet.promptpkg/ai/prompt/testdata/fixtures/claude-cli-opus.promptpkg/ai/prompt/testdata/fixtures/claude-cli-sonnet.promptpkg/ai/prompt/testdata/fixtures/codex-cli.promptpkg/ai/prompt/testdata/fixtures/gemini-api.promptpkg/ai/prompt/testdata/fixtures/gemini-cli.promptpkg/ai/prompt/testdata/fixtures/openai-gpt.promptpkg/ai/prompt/testdata/options.promptpkg/ai/provider.gopkg/ai/provider/claudeagent/provider.gopkg/ai/provider/claudeagent/provider_test.gopkg/ai/provider/claudeagent/turn.gopkg/ai/provider/cli.gopkg/ai/provider/cli_test.gopkg/ai/provider/cmux/client.gopkg/ai/provider/cmux/client_test.gopkg/ai/provider/cmux/executor.gopkg/ai/provider/cmux/executor_test.gopkg/ai/provider/cmux/focus.gopkg/ai/provider/cmux/provider.gopkg/ai/provider/cmux/provider_test.gopkg/ai/provider/cmux/sessionlog.gopkg/ai/provider/cmux/sessionlog_test.gopkg/ai/provider/cmux/sessionstats.gopkg/ai/provider/cmux/sessionstats_test.gopkg/ai/provider/cmux/stall.gopkg/ai/provider/cmux/stall_test.gopkg/ai/provider/cmux/submit.gopkg/ai/provider/cmux/submit_test.gopkg/ai/provider/cmux/support_test.gopkg/ai/provider/codex_appserver.gopkg/ai/provider/codex_appserver_protocol.gopkg/ai/provider/codex_appserver_test.gopkg/ai/provider/gemini_cli.gopkg/ai/provider/genkit/genkit.gopkg/ai/provider/genkit/genkit_test.gopkg/ai/provider/genkit/options.gopkg/ai/provider/init.gopkg/ai/provider_test.gopkg/ai/typed.gopkg/ai/types.gopkg/api/budget.gopkg/api/context.gopkg/api/cost.gopkg/api/cost_test.gopkg/api/enums.gopkg/api/enums_test.gopkg/api/memory.gopkg/api/model.gopkg/api/permissions.gopkg/api/pretty.gopkg/api/pretty_test.gopkg/api/prompt.gopkg/api/schema.gopkg/api/schema_test.gopkg/api/spec.gopkg/api/spec_test.gopkg/claude/session.gopkg/claude/session_test.gopkg/claude/tooluse.gopkg/claude/tooluse_test.gopkg/cli/ai.gopkg/cli/ai_agent.gopkg/cli/ai_agent_test.gopkg/cli/ai_catalog.gopkg/cli/ai_catalog_test.gopkg/cli/ai_filters.gopkg/cli/ai_filters_test.gopkg/cli/ai_models.gopkg/cli/ai_prompt_file.gopkg/cli/ai_prompt_file_test.gopkg/cli/ai_test.gopkg/cli/chat_thread_store.gopkg/cli/chat_thread_store_test.gopkg/cli/configure.gopkg/cli/configure_test.gopkg/cli/cost.gopkg/cli/history.gopkg/cli/info.gopkg/cli/serve.gopkg/cli/serve_test.gopkg/cli/session_filter.gopkg/cli/session_filter_test.gopkg/cli/sessions.gopkg/cli/sessions_test.gopkg/cli/stdin.gopkg/cli/webapp/index.htmlpkg/cli/webapp/src/AgentLauncher.tsxpkg/cli/webapp/src/App.tsxpkg/cli/webapp/src/ChatLayer.tsxpkg/cli/webapp/src/ChatRoute.tsxpkg/cli/webapp/src/SessionBrowser.tsxpkg/cli/webapp/src/api.tspkg/cli/webapp/src/index.csspkg/cli/webapp/src/main.tsxpkg/cli/webapp/src/session.tspkg/cli/webapp/tsconfig.jsonpkg/cli/webapp/vite.config.tspkg/cli/whoami.gopkg/cli/whoami_render.gopkg/cli/whoami_test.go
| func AgentCommand(opts AgentCommandOpts) string { | ||
| switch opts.Agent { | ||
| case "codex": | ||
| if opts.Model != "" { | ||
| return "codex -m " + opts.Model | ||
| } | ||
| return "codex" | ||
| default: | ||
| cmd := "claude" | ||
| if opts.SessionID != "" { | ||
| if opts.Resume { | ||
| cmd += " --resume " + opts.SessionID | ||
| } else { | ||
| cmd += " --session-id " + opts.SessionID | ||
| } | ||
| } | ||
| mode := opts.PermissionMode | ||
| if opts.Plan { | ||
| mode = api.PermissionPlan | ||
| } | ||
| if opts.Plan || opts.PermissionMode != "" { | ||
| cmd += " --permission-mode " + cliPermissionMode(mode) | ||
| } | ||
| if len(opts.AllowedTools) > 0 { | ||
| cmd += " --allowedTools " + strings.Join(opts.AllowedTools, ",") | ||
| } | ||
| if len(opts.DisallowedTools) > 0 { | ||
| cmd += " --disallowedTools " + strings.Join(opts.DisallowedTools, ",") | ||
| } | ||
| if opts.Model != "" { | ||
| cmd += " --model " + opts.Model | ||
| } | ||
| return cmd | ||
| } | ||
| } | ||
|
|
||
| // withEnv prepends KEY='value' assignments (sorted by key for deterministic | ||
| // output) to the agent launch command so the terminal shell exports them to the | ||
| // agent process, whose tool children inherit them. The host supplies the env via | ||
| // req.Context.Env. | ||
| func withEnv(command string, env map[string]string) string { | ||
| if len(env) == 0 { | ||
| return command | ||
| } | ||
| keys := make([]string, 0, len(env)) | ||
| for k := range env { | ||
| keys = append(keys, k) | ||
| } | ||
| sort.Strings(keys) | ||
| assigns := make([]string, 0, len(keys)) | ||
| for _, k := range keys { | ||
| assigns = append(assigns, k+"="+shellSingleQuote(env[k])) | ||
| } | ||
| return strings.Join(assigns, " ") + " " + command |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Escape or validate every dynamic shell token before pasting this command.
AgentCommand() concatenates SessionID, Model, and tool lists directly into a shell command, and withEnv() only quotes values while leaving env keys unvalidated. This string is sent to an interactive shell, so a repo-controlled model/frontmatter value or caller-supplied env key like A=1; touch /tmp/pwned turns into local command injection before the agent even starts.
Possible fix
+var safeEnvKey = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`)
+
+func shellArg(v string) string {
+ return shellSingleQuote(v)
+}
+
func AgentCommand(opts AgentCommandOpts) string {
switch opts.Agent {
case "codex":
- if opts.Model != "" {
- return "codex -m " + opts.Model
+ parts := []string{"codex"}
+ if opts.Model != "" {
+ parts = append(parts, "-m", shellArg(opts.Model))
}
- return "codex"
+ return strings.Join(parts, " ")
default:
- cmd := "claude"
+ parts := []string{"claude"}
if opts.SessionID != "" {
if opts.Resume {
- cmd += " --resume " + opts.SessionID
+ parts = append(parts, "--resume", shellArg(opts.SessionID))
} else {
- cmd += " --session-id " + opts.SessionID
+ parts = append(parts, "--session-id", shellArg(opts.SessionID))
}
}
- …
- return cmd
+ …
+ return strings.Join(parts, " ")
}
}
func withEnv(command string, env map[string]string) string {
…
for _, k := range keys {
+ if !safeEnvKey.MatchString(k) {
+ continue
+ }
assigns = append(assigns, k+"="+shellSingleQuote(env[k]))
}
return strings.Join(assigns, " ") + " " + command
}🤖 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 `@pkg/ai/provider/cmux/executor.go` around lines 236 - 289, The shell command
builder in AgentCommand and withEnv is concatenating untrusted values directly
into a command string, which can allow command injection. Update AgentCommand to
escape or validate every dynamic token it appends, including SessionID, Model,
AllowedTools, DisallowedTools, and any permission-related values, and ensure
withEnv also rejects or sanitizes unsafe env keys before prefixing assignments.
Use the existing AgentCommand and withEnv helpers as the place to centralize
safe shell quoting/validation so the final launch command cannot be influenced
by raw shell metacharacters.
…built webapp The //go:embed webapp/dist directive failed to compile in CI (build, test, and lint jobs) because dist is gitignored and the webapp cannot be built in CI (it depends on a local clicky-ui link). Commit a tracked dist/.gitkeep placeholder and switch to //go:embed all:webapp/dist so the directive always matches. The real webapp overlays dist when built locally or in the release workflow.
…of inline truncation Gavel-Issue-Id: aac4545f57ee05bb834447a3dad121aa Claude-Session-Id: 327b3a33-9f27-4db0-97f2-675dbcd5c55c
…plan-mode plans Gavel-Issue-Id: 55471644bcb5b00631cb9c4d70d0fe29 Claude-Session-Id: 0355fe0b-55fe-4391-b44d-c5897c93e931
Move the provider runtime contract (Config, Provider/StreamingProvider, Response, Event/EventKind/Event*, PermissionFunc/Request/Decision, the provider registry and NewProvider) from pkg/ai into pkg/api so external consumers (clicky/aichat) can depend on the stable api surface without importing the heavier pkg/ai runtime. pkg/ai re-exports every moved symbol as a type alias / const / wrapper func, so all existing call sites and pkg/ai/provider registration compile unchanged. Additive to pkg/api (backwards compatible); the registry map stays unexported.
Bump clicky/aichat to v1.21.34-0.20260630075958-cd55f2f4fce3, the commit where aichat depends on captain/pkg/api (the relocated runtime contract) instead of pkg/ai. Resolves the CI build failure: captain (the main module) satisfies aichat's captain imports from its own pkg/api source.
This implements a comprehensive model catalog system with support for both API and agent backends. The catalog manages model registration, lookup, and resolution with live model fetching, pricing integration, and caching. Key features: - Model registry with in-place updates and safe concurrent access - Resolution pipeline that merges catalog entries with live API listings - Pricing integration supporting both OpenRouter and static Claude pricing tables - Model cache with 24-hour TTL and fingerprint-based invalidation - JSON API for client-driven model selection with configurability detection - Comprehensive test coverage for all catalog operations
runCLI read StdoutPipe/StderrPipe in goroutines that raced cmd.Wait(); Wait closes those pipes on process exit, so a Wait-first ordering failed an in-flight read with 'file already closed' (deterministic on Linux CI, passing on macOS). Use bytes.Buffer for cmd.Stdout/Stderr so Wait blocks until the output copiers finish, and feed stdin from a goroutine. Fixes TestRunCLIUsesContextDir on Linux.
Three defects in the model-catalog work surfaced once CI could compile: - catalog_resolve_test.go: the three live-fetch tests set ANTHROPIC_API_KEY before stubLiveFetcher, which clears all keys, so the live fetch was skipped. Set the key after the stub (matching TestResolveModels_TokensUnionDedup). - catalog_resolve.go: use the promoted ResolvedModel.ContextWindow (staticcheck QF1008). - cmux/sessionstats.go: modelInfo stopped at the first candidate, but the bare hyphenated id classifies to the static Claude table (prices only, zero context), shadowing the dotted OpenRouter id that carries the context window. Prefer a candidate with a non-zero context window.
2 participants
Summary by CodeRabbit
servecommand to run the HTTP API and embedded web app.whoamito inspect backend availability, authentication status, and (optionally) model access.