Feat/cli auth autodetect -- restore claude cli integration and auto cli detection#399
Feat/cli auth autodetect -- restore claude cli integration and auto cli detection#399jasonkneen wants to merge 7 commits into
Conversation
Introduces a new `agentcli` layer to catalog installed agent CLIs, detect login state, and extract reusable local credentials (notably Codex and Claude). Provider config now supports `authCLI`, and the provider factory can build CLI-authenticated providers, including Claude token refresh/login flows, new `zero auth claude`, and richer `auth status` output with detected CLI logins. Specialist/swarm execution gains harness-backed members with stream decoders for harness output formats plus manifest fields for `harness`/`provider`. The TUI onboarding/provider wizard now supports “use detected CLI login” paths, and runtime adds an `onlybash` mode (`/onlybash`) that enforces bash+skill-only tooling.
jasonkneen
changed the title
WalkthroughThis PR adds a new ChangesAgent CLI Detection and Credential Reuse
Onlybash Tool Restriction Mode
Specialist and Swarm External Harness Execution
Estimated code review effort: 5 (Critical) | ~150 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TUI as TUI Onboarding
participant agentcli
participant Factory as Provider Factory
participant Anthropic
User->>TUI: Select CLI connect method
TUI->>agentcli: Detect() / DetectOne()
agentcli-->>TUI: Detection (LoggedIn/LoggedOut)
TUI->>TUI: Skip provider/credential steps, set AuthCLI
User->>Factory: Save profile with AuthCLI
Factory->>agentcli: ExtractCredentials(harness)
Factory->>Anthropic: Build provider with bearer resolver
Anthropic-->>Factory: Authorization header on request
sequenceDiagram
participant Orchestrator
participant Executor
participant agentcli
participant HarnessProcess as External Harness CLI
participant Decoder
Orchestrator->>Executor: runFresh(manifest with Harness)
Executor->>agentcli: DetectOne(harness)
Executor->>HarnessProcess: runChildWithDecoder(args, env)
HarnessProcess-->>Decoder: stdout NDJSON lines
Decoder-->>Executor: streamjson.Event(s)
Executor-->>Orchestrator: ExecResult with final text
Possibly related PRs
Suggested reviewers: Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR introduces a unified “agent CLI” integration layer and wires it through the TUI, CLI, provider factory, and specialist/swarm execution paths so Zero can (a) detect third‑party agent CLIs, (b) reuse their local logins as provider credentials, and (c) run specialists via external harness CLIs when requested. It also adds an /onlybash mode preset + command to hard-restrict the tool surface and extends configuration to persist CLI-authenticated provider profiles (AuthCLI).
Changes:
- Added
internal/agentclifor detecting installed agent CLIs, probing login state, and extracting reusable credentials (Claude/Codex supported), plus tests. - Added CLI-authenticated provider profiles (
AuthCLI) and updated TUI onboarding/provider wizard/model picker + provider factory to build providers using live-extracted CLI credentials (including Claude Code identity requirements). - Added harness execution support for specialists (and swarm per-member harness/provider overrides) and introduced
/onlybashmode + invariants tests.
Reviewed changes
Copilot reviewed 57 out of 57 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tui/session_controls.go | Adds /onlybash command handling with stash/restore of tool filters. |
| internal/tui/provider_wizard.go | Adds CLI-based connect methods and wizard plumbing for CLI-auth profiles. |
| internal/tui/provider_wizard_test.go | Updates wizard tests for new method-option plumbing. |
| internal/tui/provider_wizard_oauth_test.go | Updates OAuth-path tests for new method-option plumbing. |
| internal/tui/provider_wizard_cli_test.go | New tests covering CLI method rows, selection, profile shaping, and persistence. |
| internal/tui/picker.go | Makes CLI-auth profiles distinct in picker grouping; avoids env key autofill for AuthCLI. |
| internal/tui/options.go | Extends setup selection with AuthCLI. |
| internal/tui/onlybash_command_test.go | New tests for /onlybash state transitions and invariants. |
| internal/tui/onboarding.go | Adds CLI connect method to first-run setup and threads AuthCLI through save. |
| internal/tui/onboarding_test.go | Updates setup tests for method-kind enum. |
| internal/tui/onboarding_cli_test.go | New tests for CLI setup path, filtering, and AuthCLI propagation. |
| internal/tui/model.go | Adds onlybash state fields and command routing for /onlybash. |
| internal/tui/model_switch_cli_test.go | New tests ensuring AuthCLI profiles can switch models and aren’t normalized incorrectly. |
| internal/tui/commands.go | Registers /onlybash command. |
| internal/tui/commands_test.go | Adds parsing coverage for /onlybash. |
| internal/tui/command_center.go | Exempts AuthCLI profiles from “no usable credential” gate for /model switching. |
| internal/tools/onlybash_mode_test.go | Ensures onlybash mode tool-name literals match registered tool names. |
| internal/swarm/team.go | Threads Definition harness/provider overrides into member spec. |
| internal/swarm/member.go | Adds Harness and Provider fields to member spec. |
| internal/swarm/launcher_specialist.go | Threads harness/provider from member spec into specialist manifest metadata. |
| internal/swarm/harness_test.go | New tests asserting harness/provider plumbing through swarm + specialist manifest. |
| internal/swarm/definition.go | Adds Harness and Provider fields to swarm definitions. |
| internal/specialist/manifest.go | Adds harness/provider/harnessArgs metadata, parsing, validation, and summaries. |
| internal/specialist/harness.go | Adds execution path for running specialists via external harness CLIs. |
| internal/specialist/harness_manifest_test.go | New tests for harness/provider/harnessArgs parsing + extends merging + validation. |
| internal/specialist/harness_decode.go | Adds harness stdout decoders (Claude stream-json, Codex NDJSON) with fallback. |
| internal/specialist/harness_decode_test.go | New tests for harness decoder selection and decoding behavior. |
| internal/specialist/harness_child_test.go | New tests for provider env pinning, cwd wiring, stderr/exit capture, progress callback. |
| internal/specialist/format.go | Includes harness/provider fields in specialist show output. |
| internal/specialist/exec.go | Adds harness dispatch, provider env pinning for self-exec children, and decoder abstraction. |
| internal/providers/factory.go | Adds AuthCLI branch to build providers using agentcli-sourced credentials. |
| internal/providers/anthropic/provider.go | Adds Claude Code identity support (beta flag + required system block). |
| internal/providers/anthropic/provider_test.go | Tests Claude Code identity injection and beta behavior. |
| internal/providers/agentclicreds.go | Implements provider construction + token resolution for CLI-authenticated profiles. |
| internal/providers/agentclicreds_test.go | Extensive tests for CLI-auth provider behavior, refresh logic, and error shaping. |
| internal/provideronboarding/advice.go | Threads agentcli detections into “missing credential” advice and treats AuthCLI as credentialed. |
| internal/provideronboarding/advice_test.go | Tests AuthCLI credential recognition and detection-based hints. |
| internal/provideroauth/claudecode.go | Adds Claude Code login + refresh PKCE/token-refresh flows (used for zero-owned Claude login). |
| internal/provideroauth/claudecode_test.go | Tests Claude Code refresh + login flows and error handling. |
| internal/modelregistry/modes.go | Adds onlybash preset mode (tool surface restriction only). |
| internal/modelregistry/modes_test.go | Tests onlybash preset behavior and model-resolution guard for empty model. |
| internal/config/types.go | Adds AuthCLI to provider profiles and JSON decoding/aliases. |
| internal/config/types_test.go | Tests AuthCLI JSON round-trip and omitempty behavior. |
| internal/config/credentials.go | Treats AuthCLI as a configured credential (affects OAuth resolver attachment). |
| internal/config/credentials_test.go | Tests HasConfiguredCredential and OAuth candidates gating for AuthCLI. |
| internal/cli/setup.go | Ensures CLI-auth selections clear APIKey/APIKeyEnv and persist AuthCLI. |
| internal/cli/provider_detect.go | Adds agent-CLI detections into provider-detect reporting/advice surface. |
| internal/cli/auth.go | Adds zero auth claude and extends auth status to list detected agent CLIs. |
| internal/cli/auth_test.go | Tests agent-CLI section rendering and JSON output in auth status. |
| internal/agentcli/credentials.go | Implements credential extraction for codex + claude stores (file/keychain). |
| internal/agentcli/credentials_test.go | Tests credential extraction behavior and shadowing edge cases. |
| internal/agentcli/agentcli.go | Adds agent CLI catalog + Detect/DetectOne probing and macOS keychain read support. |
| internal/agentcli/agentcli_test.go | Tests catalog invariants, detection behavior, login probing, and LoginCommand. |
| internal/agent/loop_test.go | Adds invariant test ensuring onlybash allowlist blocks delegation and blocks tool_search via denylist. |
| cascade/10-findings.cf | Adds an audit/findings record file. |
| cascade/00-task.txt | Adds a task file describing review scope/goals. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (m model) newProviderWizard() *providerWizardState { | ||
| detections := agentcli.Detect(agentcli.Deps{}) | ||
| providers := providerWizardProviders() |
| detections := agentcli.Detect(agentcli.Deps{}) | ||
| state := setupState{ | ||
| visible: options.Visible, | ||
| required: options.Required, | ||
| configPath: strings.TrimSpace(options.ConfigPath), |
| out, err := exec.Command("/usr/bin/security", "find-generic-password", "-s", service, "-w").Output() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("agentcli: keychain lookup for %q: %w", service, err) | ||
| } | ||
| return bytes.TrimRight(out, "\n"), nil |
|
hello @jasonkneen kindly rebase to main and fix conflicts |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
internal/tui/onboarding.go (1)
718-748: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate model-stage bootstrap logic between the CLI fast path and the generic advance path.
The CLI branch (lines 740-748) repeats the same
resetSetupModels/modelErr/modelGen++/setupModelDiscoveryCmd/modelLoadsequence as the default post-advance flow (lines 797-803). Extracting a small helper (e.g.m.enterModelStage()) would prevent the two copies drifting apart.♻️ Suggested helper extraction
+func (m *model) enterModelStage() tea.Cmd { + m.setup.stage = setupStageModel + m.resetSetupModels() + m.setup.modelErr = "" + m.setup.modelGen++ + cmd := m.setupModelDiscoveryCmd(m.setup.modelGen) + m.setup.modelLoad = cmd != nil + return cmd +}Then both call sites become
cmd := m.enterModelStage(); return m, cmd.Also applies to: 795-804
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/onboarding.go` around lines 718 - 748, The CLI fast path in providerWizardMethodCLI duplicates the same model-stage setup sequence used in the generic advance path, which risks the two flows drifting apart. Extract the repeated reset-and-bootstrap steps into a shared helper on the onboarding model, such as m.enterModelStage(), and have both the CLI branch and the default post-advance flow call it. Keep the helper responsible for resetSetupModels, clearing modelErr, incrementing modelGen, creating the setupModelDiscoveryCmd, and setting modelLoad consistently.internal/tui/provider_wizard.go (1)
1779-1787: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPersist the provider default BaseURL for CLI profiles too.
providerWizardProfile()stores the descriptor defaultBaseURL, but CLI profiles currently persist it empty. Keep the profile shape consistent so provider construction and next-launch reloads do not depend on downstream default hydration.Proposed fix
func providerWizardCLIProfile(harness agentcli.Harness, provider providercatalog.Descriptor, model string) config.ProviderProfile { return config.ProviderProfile{ Name: harness.ID, ProviderKind: providerWizardProviderKind(provider), CatalogID: provider.ID, + BaseURL: provider.DefaultBaseURL, APIFormat: providerWizardAPIFormat(provider), Model: firstProviderDisplayValue(model, provider.DefaultModel), AuthCLI: harness.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 `@internal/tui/provider_wizard.go` around lines 1779 - 1787, The CLI profile builder in providerWizardCLIProfile is omitting the provider default BaseURL, unlike providerWizardProfile, so update it to persist the descriptor’s default BaseURL when constructing config.ProviderProfile. Keep the profile fields aligned with the non-CLI path by using the provider value from providercatalog.Descriptor so provider construction and reloads don’t rely on later default hydration.internal/provideronboarding/advice.go (1)
184-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
profile.HasConfiguredCredential()instead of a second hand-rolled credential check.
internal/cli/app.go'sprofileHasCredentialwas just updated to delegate toprofile.HasConfiguredCredential()(added in the config layer specifically to be the canonical "does this profile have configured auth" check, AuthCLI included). This function checks the same three fields by hand instead. Any future credential type added toHasConfiguredCredentialwon't automatically flow into onboarding advice unless someone remembers to update both places.♻️ Proposed consolidation
func providerProfileHasCredential(profile config.ProviderProfile) bool { - return strings.TrimSpace(profile.APIKey) != "" || - strings.TrimSpace(profile.AuthHeaderValue) != "" || - strings.TrimSpace(profile.AuthCLI) != "" + return profile.HasConfiguredCredential() }Please confirm
HasConfiguredCredential()'s semantics match exactly (i.e., it doesn't also fold in APIKeyEnv resolution, which would change onboarding-advice behavior) before applying.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/provideronboarding/advice.go` around lines 184 - 188, `providerProfileHasCredential` is duplicating the auth check instead of using the canonical profile method. Update this helper in `internal/provideronboarding/advice.go` to delegate to `profile.HasConfiguredCredential()` so onboarding advice stays in sync with the config layer. Before changing it, verify the `HasConfiguredCredential()` semantics in the `config.ProviderProfile` type match the current behavior exactly and do not include any environment-resolution logic.internal/provideroauth/claudecode.go (2)
144-151: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDefault 30s client timeout may be too short for this endpoint under load.
Third-party reports of this exact endpoint describe 40-60s response times during platform degradation (and Anthropic's own CLI has a known bug from a hardcoded 15s timeout being too aggressive). The default
http.Client{Timeout: 30 * time.Second}used when no client is injected risks the same false-timeout failures;runAuthClaudealready overrides with 60s for login, butRefreshClaudeCode's default (used wherever a caller doesn't inject a client) stays at 30s.Also applies to: 283-290
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/provideroauth/claudecode.go` around lines 144 - 151, The default HTTP timeout in `RefreshClaudeCode`/`runAuthClaude` is too aggressive for this endpoint and can cause false timeouts under load. Update the fallback client setup in `claudecode.go` so the default `http.Client` timeout is increased to match the longer auth/login behavior already used elsewhere, and ensure both the `RefreshClaudeCode` path and any other default-client path in this file use the same timeout policy when `opts.HTTPClient` is nil.
71-79: 🔒 Security & Privacy | 🔵 TrivialReusing Claude subscription OAuth tokens outside Claude Code may violate Anthropic's ToS — worth a compliance sanity-check before shipping.
The design here explicitly mints/refreshes Claude Code's own OAuth client tokens for use inside a third-party CLI (
zero). Recent third-party reporting describes Anthropic's consumer terms as restricting this:"Using OAuth tokens obtained through a Free, Pro, or Max account in any other product, tool, or service constitutes a violation"
...though other reports describe mixed/relaxed enforcement for CLI-reuse patterns. This is unofficial secondary reporting, not Anthropic's own published terms, so treat it as a flag to verify rather than a settled fact. Given the blast radius (user account bans/API access revocation), it's worth having someone confirm current ToS language for this specific "sign in with Claude" flow before this ships broadly.
Also applies to: 265-283
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/provideroauth/claudecode.go` around lines 71 - 79, Add a compliance check around ClaudeCodeLogin and the refresh path used by RefreshClaudeCode/Claude Code token persistence so we are not reusing Claude subscription OAuth tokens in zero without explicit validation. Review the current token-minting flow in ClaudeCodeLogin and the token-store usage it feeds, then either gate it behind a verified Anthropic ToS allowance or replace it with a compliant auth approach; keep the fix localized to the Claude Code login/token-refresh symbols so the behavior is easy to audit.internal/specialist/harness.go (1)
17-61: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo direct test coverage for
runHarness.
harness_child_test.goandharness_decode_test.gocover the shared plumbing (childProviderEnv,runChildWithDecoder, decoders) but nothing exercisesExecutor.runHarnessitself — e.g. the "not installed" error path, session-id/accounting wiring, orrunFresh's background-rejection gate in exec.go (lines 341-343). SincerunHarnesscallsrunChildWithDecoderdirectly instead of going through the injectableRunChildseam, testing it end-to-end would currently require a realclaude/codexbinary on PATH.Consider adding a small seam (mirroring
executor.RunChild) so the accounting/error-handling logic inrunHarnesscan be unit tested without a real external 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 `@internal/specialist/harness.go` around lines 17 - 61, `Executor.runHarness` has no direct unit-test coverage because it calls `runChildWithDecoder` directly, bypassing the injectable `RunChild` seam. Add a small indirection in `runHarness` (or a similar injectable helper on `Executor`) so tests can stub the child process and cover the harness-not-installed error path, session/accounting wiring, and the background-rejection behavior without requiring a real CLI binary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/agentcli/agentcli.go`:
- Around line 370-379: The readKeychain helper currently shells out to
/usr/bin/security without any context, so a stalled keychain lookup can block
outbound requests indefinitely. Update readKeychain to use a context with
timeout when invoking exec.Command, and make sure the timeout is applied through
the command path used in agentcli.go so the security call can be canceled
cleanly. Keep the existing error wrapping in readKeychain, but ensure the
timeout/cancellation behavior is wired into the Deps.Keychain implementation
path described by resolveDeps and agentclicreds.go.
In `@internal/providers/agentclicreds.go`:
- Around line 155-196: `claudeRefreshingBearerResolver` can race when multiple
concurrent `StreamCompletion` calls invoke the same shared resolver, causing
duplicate `refresh(ctx, refreshToken)` attempts with the same token. Serialize
the resolver’s refresh path by adding a `sync`-based lock inside
`claudeRefreshingBearerResolver` so only one goroutine can evaluate
cache/extract/refresh/save at a time, and keep the existing fast-path token
reuse in place. Update the import block to include `sync`, and use the resolver
closure plus `store`, `refresh`, and `newestClaudeRefreshToken` as the key
points for the locking change.
In `@internal/specialist/exec.go`:
- Around line 37-40: Add a small sync test in internal/specialist that compares
zeroProviderEnvVar in exec.go against config.ActiveProviderEnv. The issue is
that zeroProviderEnvVar is a duplicated literal source of truth, so a future
rename of config.ActiveProviderEnv could silently break provider pinning; fix it
by adding a test that imports both symbols and asserts they stay equal.
In `@internal/specialist/harness_decode.go`:
- Around line 97-152: Update codexJSONDecoder to parse the current codex exec
--json event schema instead of the legacy nested msg format, since decodeLine
currently only handles agent_message/task_complete/token_count and will ignore
modern top-level events. Adjust codexLine and decodeLine to recognize
item.completed and turn.completed (including usage on turn.completed.usage), and
make sure the final assistant text is still preserved and emitted via
lastMessage/finish. Keep the fix localized to codexJSONDecoder, codexLine, and
decodeLine so current Codex JSONL output is surfaced correctly.
In `@internal/tui/onboarding.go`:
- Around line 124-138: `newSetupState` eagerly calls
`agentcli.Detect(agentcli.Deps{})` during TUI startup, causing blocking
PATH/file/keychain probes before setup is actually needed. Move the detection
behind the setup flow so it only runs when the onboarding/setup UI is opened,
and keep `setupState` initialization in `newSetupState` limited to cheap fields
like `visible`, `required`, `configPath`, and `selectedMethod`. Use
`agentcli.Detect` and `setupState` as the main points to refactor so the initial
render stays responsive.
In `@internal/tui/provider_wizard_cli_test.go`:
- Around line 112-113: The provider wizard tests are constructing the wizard
through m.newProviderWizard(), which still triggers agentcli.Detect before the
test overrides are applied. Update the setup to build the wizard state with
injected detections directly, or add a test-only deps hook so newModel and
providerWizard initialization do not touch real CLI detection. Keep the fix
centered around newModel, m.newProviderWizard, and the agentCLIDetections
override used in these tests.
In `@internal/tui/provider_wizard.go`:
- Around line 543-567: The CLI method selection in providerWizard.go is using
stale login state from methodOptions(), so the wizard can keep rejecting a
harness after the user logs in. Before setting the logged-out error in the
providerWizardMethodCLI branch, refresh the underlying CLI detection state (or
re-run the harness probe used by agentCLIDetections) and rebuild the selected
option so selected.loggedIn reflects the current status. Keep the fix localized
around the switch on selected.kind and the selected.harness/login check.
In `@internal/tui/session_controls.go`:
- Around line 300-319: The /onlybash off path in handleOnlyBashCommand can wipe
existing tool restrictions when onlybash was never enabled, because
disableOnlyBash restores from empty stash fields unconditionally. Update
disableOnlyBash (and any callers like handleOnlyBashCommand) to no-op unless
onlyBashActive is true, mirroring the guard already used by enableOnlyBash, so
pre-existing agentOptions.EnabledTools and DisabledTools are preserved. Add or
adjust coverage for the explicit off/status flow where onlybash starts inactive
and an operator-configured tool restriction already exists.
---
Nitpick comments:
In `@internal/provideroauth/claudecode.go`:
- Around line 144-151: The default HTTP timeout in
`RefreshClaudeCode`/`runAuthClaude` is too aggressive for this endpoint and can
cause false timeouts under load. Update the fallback client setup in
`claudecode.go` so the default `http.Client` timeout is increased to match the
longer auth/login behavior already used elsewhere, and ensure both the
`RefreshClaudeCode` path and any other default-client path in this file use the
same timeout policy when `opts.HTTPClient` is nil.
- Around line 71-79: Add a compliance check around ClaudeCodeLogin and the
refresh path used by RefreshClaudeCode/Claude Code token persistence so we are
not reusing Claude subscription OAuth tokens in zero without explicit
validation. Review the current token-minting flow in ClaudeCodeLogin and the
token-store usage it feeds, then either gate it behind a verified Anthropic ToS
allowance or replace it with a compliant auth approach; keep the fix localized
to the Claude Code login/token-refresh symbols so the behavior is easy to audit.
In `@internal/provideronboarding/advice.go`:
- Around line 184-188: `providerProfileHasCredential` is duplicating the auth
check instead of using the canonical profile method. Update this helper in
`internal/provideronboarding/advice.go` to delegate to
`profile.HasConfiguredCredential()` so onboarding advice stays in sync with the
config layer. Before changing it, verify the `HasConfiguredCredential()`
semantics in the `config.ProviderProfile` type match the current behavior
exactly and do not include any environment-resolution logic.
In `@internal/specialist/harness.go`:
- Around line 17-61: `Executor.runHarness` has no direct unit-test coverage
because it calls `runChildWithDecoder` directly, bypassing the injectable
`RunChild` seam. Add a small indirection in `runHarness` (or a similar
injectable helper on `Executor`) so tests can stub the child process and cover
the harness-not-installed error path, session/accounting wiring, and the
background-rejection behavior without requiring a real CLI binary.
In `@internal/tui/onboarding.go`:
- Around line 718-748: The CLI fast path in providerWizardMethodCLI duplicates
the same model-stage setup sequence used in the generic advance path, which
risks the two flows drifting apart. Extract the repeated reset-and-bootstrap
steps into a shared helper on the onboarding model, such as m.enterModelStage(),
and have both the CLI branch and the default post-advance flow call it. Keep the
helper responsible for resetSetupModels, clearing modelErr, incrementing
modelGen, creating the setupModelDiscoveryCmd, and setting modelLoad
consistently.
In `@internal/tui/provider_wizard.go`:
- Around line 1779-1787: The CLI profile builder in providerWizardCLIProfile is
omitting the provider default BaseURL, unlike providerWizardProfile, so update
it to persist the descriptor’s default BaseURL when constructing
config.ProviderProfile. Keep the profile fields aligned with the non-CLI path by
using the provider value from providercatalog.Descriptor so provider
construction and reloads don’t rely on later default hydration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1c19886c-8e97-4334-b9ba-46d159598bcc
📒 Files selected for processing (57)
cascade/00-task.txtcascade/10-findings.cfinternal/agent/loop_test.gointernal/agentcli/agentcli.gointernal/agentcli/agentcli_test.gointernal/agentcli/credentials.gointernal/agentcli/credentials_test.gointernal/cli/app.gointernal/cli/auth.gointernal/cli/auth_test.gointernal/cli/provider_detect.gointernal/cli/setup.gointernal/config/credentials.gointernal/config/credentials_test.gointernal/config/types.gointernal/config/types_test.gointernal/modelregistry/modes.gointernal/modelregistry/modes_test.gointernal/provideroauth/claudecode.gointernal/provideroauth/claudecode_test.gointernal/provideronboarding/advice.gointernal/provideronboarding/advice_test.gointernal/providers/agentclicreds.gointernal/providers/agentclicreds_test.gointernal/providers/anthropic/provider.gointernal/providers/anthropic/provider_test.gointernal/providers/factory.gointernal/specialist/exec.gointernal/specialist/format.gointernal/specialist/harness.gointernal/specialist/harness_child_test.gointernal/specialist/harness_decode.gointernal/specialist/harness_decode_test.gointernal/specialist/harness_manifest_test.gointernal/specialist/manifest.gointernal/swarm/definition.gointernal/swarm/harness_test.gointernal/swarm/launcher_specialist.gointernal/swarm/member.gointernal/swarm/team.gointernal/tools/onlybash_mode_test.gointernal/tui/command_center.gointernal/tui/commands.gointernal/tui/commands_test.gointernal/tui/model.gointernal/tui/model_switch_cli_test.gointernal/tui/onboarding.gointernal/tui/onboarding_cli_test.gointernal/tui/onboarding_test.gointernal/tui/onlybash_command_test.gointernal/tui/options.gointernal/tui/picker.gointernal/tui/provider_wizard.gointernal/tui/provider_wizard_cli_test.gointernal/tui/provider_wizard_oauth_test.gointernal/tui/provider_wizard_test.gointernal/tui/session_controls.go
| // readKeychain is the real Deps.Keychain implementation, wired up only on | ||
| // darwin (see resolveDeps): it shells out to the `security` CLI rather than | ||
| // linking a CGo/keychain-access library, keeping this package stdlib-only. | ||
| func readKeychain(service string) ([]byte, error) { | ||
| out, err := exec.Command("/usr/bin/security", "find-generic-password", "-s", service, "-w").Output() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("agentcli: keychain lookup for %q: %w", service, err) | ||
| } | ||
| return bytes.TrimRight(out, "\n"), nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a timeout to the keychain shell-out.
readKeychain is the real Deps.Keychain implementation, and per the package's own design (see agentclicreds.go's doc comment), credentials are re-read — including this keychain probe — on every outbound provider request, with no caching. exec.Command here has no context/timeout, so a hung or blocked security process (e.g. a stalled keychain-unlock prompt) will block the request goroutine indefinitely. golangci-lint's noctx hint is valid here, not a false positive, since this sits on a real request path.
🔧 Proposed fix
+import (
+ "context"
+)
+
func readKeychain(service string) ([]byte, error) {
- out, err := exec.Command("/usr/bin/security", "find-generic-password", "-s", service, "-w").Output()
+ ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ out, err := exec.CommandContext(ctx, "/usr/bin/security", "find-generic-password", "-s", service, "-w").Output()
if err != nil {
return nil, fmt.Errorf("agentcli: keychain lookup for %q: %w", service, err)
}
return bytes.TrimRight(out, "\n"), 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.
| // readKeychain is the real Deps.Keychain implementation, wired up only on | |
| // darwin (see resolveDeps): it shells out to the `security` CLI rather than | |
| // linking a CGo/keychain-access library, keeping this package stdlib-only. | |
| func readKeychain(service string) ([]byte, error) { | |
| out, err := exec.Command("/usr/bin/security", "find-generic-password", "-s", service, "-w").Output() | |
| if err != nil { | |
| return nil, fmt.Errorf("agentcli: keychain lookup for %q: %w", service, err) | |
| } | |
| return bytes.TrimRight(out, "\n"), nil | |
| } | |
| // readKeychain is the real Deps.Keychain implementation, wired up only on | |
| // darwin (see resolveDeps): it shells out to the `security` CLI rather than | |
| // linking a CGo/keychain-access library, keeping this package stdlib-only. | |
| func readKeychain(service string) ([]byte, error) { | |
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| defer cancel() | |
| out, err := exec.CommandContext(ctx, "/usr/bin/security", "find-generic-password", "-s", service, "-w").Output() | |
| if err != nil { | |
| return nil, fmt.Errorf("agentcli: keychain lookup for %q: %w", service, err) | |
| } | |
| return bytes.TrimRight(out, "\n"), nil | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 374-374: os/exec.Command must not be called. use os/exec.CommandContext
(noctx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/agentcli/agentcli.go` around lines 370 - 379, The readKeychain
helper currently shells out to /usr/bin/security without any context, so a
stalled keychain lookup can block outbound requests indefinitely. Update
readKeychain to use a context with timeout when invoking exec.Command, and make
sure the timeout is applied through the command path used in agentcli.go so the
security call can be canceled cleanly. Keep the existing error wrapping in
readKeychain, but ensure the timeout/cancellation behavior is wired into the
Deps.Keychain implementation path described by resolveDeps and agentclicreds.go.
Source: Linters/SAST tools
| func claudeRefreshingBearerResolver(harness agentcli.Harness, deps agentcli.Deps, store cliAuthTokenStore, refresh claudeRefreshFunc) providerio.TokenResolver { | ||
| return func(ctx context.Context, forceRefresh bool) (string, string, bool, error) { | ||
| now := time.Now() | ||
| var cached oauth.Token | ||
| if store != nil { | ||
| if token, ok, err := store.Load(cliAuthTokenStoreKey); err == nil && ok { | ||
| cached = token | ||
| } | ||
| } | ||
| if !forceRefresh && cached.AccessToken != "" && !cached.NeedsRefresh(now, claudeRefreshBuffer) { | ||
| return "Authorization", "Bearer " + cached.AccessToken, true, nil | ||
| } | ||
|
|
||
| creds, ok, err := agentcli.ExtractCredentials(harness, deps) | ||
| if err != nil { | ||
| return "", "", false, fmt.Errorf("%s credentials: %w", harness.DisplayName, err) | ||
| } | ||
| if !ok { | ||
| creds = agentcli.Credentials{} | ||
| } | ||
| extractedFresh := strings.TrimSpace(creds.AccessToken) != "" && | ||
| !creds.ExpiresAt.IsZero() && creds.ExpiresAt.After(now.Add(claudeRefreshBuffer)) | ||
| if !forceRefresh && extractedFresh { | ||
| return "Authorization", "Bearer " + creds.AccessToken, true, nil | ||
| } | ||
|
|
||
| refreshToken := newestClaudeRefreshToken(cached, creds) | ||
| if refreshToken == "" { | ||
| return "", "", false, cliLoginExpiredError(harness) | ||
| } | ||
| token, refreshErr := refresh(ctx, refreshToken) | ||
| if refreshErr != nil { | ||
| return "", "", false, fmt.Errorf("%w (auto-refresh failed: %v)", cliLoginExpiredError(harness), refreshErr) | ||
| } | ||
| if store != nil { | ||
| // A failed save only costs a redundant refresh next call; the token | ||
| // in hand is still good. | ||
| _ = store.Save(cliAuthTokenStoreKey, token) | ||
| } | ||
| return "Authorization", "Bearer " + token.AccessToken, true, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Race on concurrent refreshes: no locking around the Claude refresh chain.
claudeRefreshingBearerResolver's closure is shared across concurrent StreamCompletion calls on the same provider (e.g. parallel tool calls). If two calls simultaneously see a stale cached/extracted token, both will call refresh(ctx, refreshToken) with the same refresh token. OAuth refresh tokens are commonly single-use/rotated server-side, so the losing call can fail with a spurious auth error instead of transparently reusing the winner's freshly-rotated token.
🔒 Proposed fix: serialize the resolver body
func claudeRefreshingBearerResolver(harness agentcli.Harness, deps agentcli.Deps, store cliAuthTokenStore, refresh claudeRefreshFunc) providerio.TokenResolver {
+ var mu sync.Mutex
return func(ctx context.Context, forceRefresh bool) (string, string, bool, error) {
+ mu.Lock()
+ defer mu.Unlock()
now := time.Now()
var cached oauth.Token(Also add "sync" to the import block.)
📝 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 claudeRefreshingBearerResolver(harness agentcli.Harness, deps agentcli.Deps, store cliAuthTokenStore, refresh claudeRefreshFunc) providerio.TokenResolver { | |
| return func(ctx context.Context, forceRefresh bool) (string, string, bool, error) { | |
| now := time.Now() | |
| var cached oauth.Token | |
| if store != nil { | |
| if token, ok, err := store.Load(cliAuthTokenStoreKey); err == nil && ok { | |
| cached = token | |
| } | |
| } | |
| if !forceRefresh && cached.AccessToken != "" && !cached.NeedsRefresh(now, claudeRefreshBuffer) { | |
| return "Authorization", "Bearer " + cached.AccessToken, true, nil | |
| } | |
| creds, ok, err := agentcli.ExtractCredentials(harness, deps) | |
| if err != nil { | |
| return "", "", false, fmt.Errorf("%s credentials: %w", harness.DisplayName, err) | |
| } | |
| if !ok { | |
| creds = agentcli.Credentials{} | |
| } | |
| extractedFresh := strings.TrimSpace(creds.AccessToken) != "" && | |
| !creds.ExpiresAt.IsZero() && creds.ExpiresAt.After(now.Add(claudeRefreshBuffer)) | |
| if !forceRefresh && extractedFresh { | |
| return "Authorization", "Bearer " + creds.AccessToken, true, nil | |
| } | |
| refreshToken := newestClaudeRefreshToken(cached, creds) | |
| if refreshToken == "" { | |
| return "", "", false, cliLoginExpiredError(harness) | |
| } | |
| token, refreshErr := refresh(ctx, refreshToken) | |
| if refreshErr != nil { | |
| return "", "", false, fmt.Errorf("%w (auto-refresh failed: %v)", cliLoginExpiredError(harness), refreshErr) | |
| } | |
| if store != nil { | |
| // A failed save only costs a redundant refresh next call; the token | |
| // in hand is still good. | |
| _ = store.Save(cliAuthTokenStoreKey, token) | |
| } | |
| return "Authorization", "Bearer " + token.AccessToken, true, nil | |
| } | |
| } | |
| func claudeRefreshingBearerResolver(harness agentcli.Harness, deps agentcli.Deps, store cliAuthTokenStore, refresh claudeRefreshFunc) providerio.TokenResolver { | |
| var mu sync.Mutex | |
| return func(ctx context.Context, forceRefresh bool) (string, string, bool, error) { | |
| mu.Lock() | |
| defer mu.Unlock() | |
| now := time.Now() | |
| var cached oauth.Token | |
| if store != nil { | |
| if token, ok, err := store.Load(cliAuthTokenStoreKey); err == nil && ok { | |
| cached = token | |
| } | |
| } | |
| if !forceRefresh && cached.AccessToken != "" && !cached.NeedsRefresh(now, claudeRefreshBuffer) { | |
| return "Authorization", "Bearer " + cached.AccessToken, true, nil | |
| } | |
| creds, ok, err := agentcli.ExtractCredentials(harness, deps) | |
| if err != nil { | |
| return "", "", false, fmt.Errorf("%s credentials: %w", harness.DisplayName, err) | |
| } | |
| if !ok { | |
| creds = agentcli.Credentials{} | |
| } | |
| extractedFresh := strings.TrimSpace(creds.AccessToken) != "" && | |
| !creds.ExpiresAt.IsZero() && creds.ExpiresAt.After(now.Add(claudeRefreshBuffer)) | |
| if !forceRefresh && extractedFresh { | |
| return "Authorization", "Bearer " + creds.AccessToken, true, nil | |
| } | |
| refreshToken := newestClaudeRefreshToken(cached, creds) | |
| if refreshToken == "" { | |
| return "", "", false, cliLoginExpiredError(harness) | |
| } | |
| token, refreshErr := refresh(ctx, refreshToken) | |
| if refreshErr != nil { | |
| return "", "", false, fmt.Errorf("%w (auto-refresh failed: %v)", cliLoginExpiredError(harness), refreshErr) | |
| } | |
| if store != nil { | |
| // A failed save only costs a redundant refresh next call; the token | |
| // in hand is still good. | |
| _ = store.Save(cliAuthTokenStoreKey, token) | |
| } | |
| return "Authorization", "Bearer " + token.AccessToken, true, 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 `@internal/providers/agentclicreds.go` around lines 155 - 196,
`claudeRefreshingBearerResolver` can race when multiple concurrent
`StreamCompletion` calls invoke the same shared resolver, causing duplicate
`refresh(ctx, refreshToken)` attempts with the same token. Serialize the
resolver’s refresh path by adding a `sync`-based lock inside
`claudeRefreshingBearerResolver` so only one goroutine can evaluate
cache/extract/refresh/save at a time, and keep the existing fast-path token
reuse in place. Update the import block to include `sync`, and use the resolver
closure plus `store`, `refresh`, and `newestClaudeRefreshToken` as the key
points for the locking change.
| // zeroProviderEnvVar mirrors config.ActiveProviderEnv ("ZERO_PROVIDER") without | ||
| // importing internal/config: a per-child provider override just needs the env | ||
| // var name a self-exec zero child reads at startup to resolve its profile. | ||
| const zeroProviderEnvVar = "ZERO_PROVIDER" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the actual value of config.ActiveProviderEnv matches "ZERO_PROVIDER"
rg -n 'ActiveProviderEnv' internal/config --type=go -A2 -B2Repository: Gitlawb/zero
Length of output: 4457
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant source and nearby tests/usages.
sed -n '1,140p' internal/specialist/exec.go
printf '\n---\n'
rg -n 'zeroProviderEnvVar|SetActiveProviderEnv|ActiveProviderEnv' internal --type=go -A3 -B3Repository: Gitlawb/zero
Length of output: 24117
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether there's already a test guarding the cross-package env var name.
rg -n 'zeroProviderEnvVar|ActiveProviderEnv|ZERO_PROVIDER' internal --type=go -A3 -B3Repository: Gitlawb/zero
Length of output: 29869
Add a test that keeps the specialist env var in sync with config.ActiveProviderEnv.
This literal is a separate source of truth; if the config constant is renamed later, provider pinning will break silently. A tiny test in internal/specialist comparing zeroProviderEnvVar to config.ActiveProviderEnv would catch that immediately.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/specialist/exec.go` around lines 37 - 40, Add a small sync test in
internal/specialist that compares zeroProviderEnvVar in exec.go against
config.ActiveProviderEnv. The issue is that zeroProviderEnvVar is a duplicated
literal source of truth, so a future rename of config.ActiveProviderEnv could
silently break provider pinning; fix it by adding a test that imports both
symbols and asserts they stay equal.
| // codexJSONDecoder decodes the NDJSON stdout produced by `codex exec --json`: | ||
| // each line wraps a "msg" whose "type" distinguishes an assistant text chunk | ||
| // ("agent_message"), the run's completion marker ("task_complete"), and a | ||
| // best-effort token usage report ("token_count"). Codex's final answer is | ||
| // "the last agent_message seen", not a dedicated field, so the decoder must | ||
| // remember it across lines. | ||
| type codexJSONDecoder struct { | ||
| lastMessage string | ||
| done bool | ||
| } | ||
|
|
||
| type codexLine struct { | ||
| Msg struct { | ||
| Type string `json:"type"` | ||
| Message string `json:"message"` | ||
| InputTokens *int `json:"input_tokens"` | ||
| OutputTokens *int `json:"output_tokens"` | ||
| TotalTokens *int `json:"total_tokens"` | ||
| } `json:"msg"` | ||
| } | ||
|
|
||
| func (d *codexJSONDecoder) decodeLine(line string) []streamjson.Event { | ||
| var parsed codexLine | ||
| if err := json.Unmarshal([]byte(line), &parsed); err != nil { | ||
| return nil | ||
| } | ||
| switch parsed.Msg.Type { | ||
| case "agent_message": | ||
| if parsed.Msg.Message == "" { | ||
| return nil | ||
| } | ||
| d.lastMessage = parsed.Msg.Message | ||
| return []streamjson.Event{{Type: streamjson.EventText, Delta: parsed.Msg.Message}} | ||
| case "task_complete": | ||
| d.done = true | ||
| return []streamjson.Event{{Type: streamjson.EventFinal, Text: d.lastMessage}} | ||
| case "token_count": | ||
| event := streamjson.Event{Type: streamjson.EventUsage} | ||
| event.PromptTokens = parsed.Msg.InputTokens | ||
| event.CompletionTokens = parsed.Msg.OutputTokens | ||
| event.TotalTokens = parsed.Msg.TotalTokens | ||
| return []streamjson.Event{event} | ||
| default: | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| func (d *codexJSONDecoder) finish(int) []streamjson.Event { | ||
| // task_complete never arrived (e.g. the CLI was killed mid-run) but there | ||
| // was at least one agent_message — still surface it as the final answer | ||
| // rather than losing it. | ||
| if d.done || d.lastMessage == "" { | ||
| return nil | ||
| } | ||
| return []streamjson.Event{{Type: streamjson.EventFinal, Text: d.lastMessage}} | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
rg -n "codexJSONDecoder|codex exec --json|agent_message|task_complete|token_count|item.completed|turn.completed|streamjson.Event" -S .Repository: Gitlawb/zero
Length of output: 21939
🏁 Script executed:
sed -n '1,220p' internal/specialist/harness_decode.go && printf '\n--- TESTS ---\n' && sed -n '1,240p' internal/specialist/harness_decode_test.goRepository: Gitlawb/zero
Length of output: 11992
🌐 Web query:
Codex CLI "exec --json" schema msg agent_message task_complete token_count item.completed turn.completed usage
💡 Result:
The Codex CLI exec --json flag switches standard output to a newline-delimited JSON Lines (JSONL) stream, where each line represents a structured event during the agent's execution lifecycle [1][2][3]. Key event types and structures include: - item.completed: Signals that an item (such as an agent_message, file_change, or command_execution) has reached a terminal state [4]. - turn.completed: Emitted when a turn is finished. It contains the usage object, which details token consumption [4][5]. - usage: This field within turn.completed events includes: - input_tokens: Total input tokens used [4][5]. - cached_input_tokens: Number of cached input tokens used [5][4]. - output_tokens: Number of output tokens generated [4][5]. - agent_message: Typically emitted as part of an item.completed event when the agent provides a final or intermediate text response [6][5]. - task_complete: Often used synonymously or in conjunction with turn.completed to signal that a specific task or turn has concluded [7][8][4]. To enforce a specific structure on the agent's final response, you can use the --output-schema <FILE> flag, which ensures the output conforms to a provided JSON Schema [1][2][9]. For automation and CI/CD pipelines, piping the output to tools like jq is standard practice to extract specific fields or filter events [1][2][3]. Example usage: codex exec --json "analyze project structure" | jq -c 'select(.type=="turn.completed")' [9] When using the --output-schema flag: codex exec "Extract metadata" --output-schema./schema.json -o./result.json [1]
Citations:
- 1: https://developers.openai.com/codex/noninteractive
- 2: https://openai-codex.mintlify.app/cli/exec
- 3: https://codex.danielvaughan.com/2026/06/11/codex-exec-structured-output-pipelines-output-schema-json-resume-ci-automation/
- 4: https://github.com/openai/codex/blob/d807d44a/codex-rs/exec/src/exec_events.rs
- 5: https://takopi.dev/reference/runners/codex/exec-json-cheatsheet/
- 6: https://openai-codex.mintlify.app/advanced/exec-mode
- 7: https://github.com/neochoon/agenthud/blob/main/docs/schemas/codex-session.md
- 8: app-server: silent turn/completed(last_agent_message=null) when run_turn early-returns after compaction failure openai/codex#25619
- 9: https://openai-codex.mintlify.app/concepts/non-interactive-mode
Update codexJSONDecoder to the current codex exec --json schema
internal/specialist/harness_decode.go still expects legacy {"msg": ...} lines, but current Codex JSONL uses top-level events like item.completed and turn.completed (with usage on turn.completed.usage). As written, codex-json will drop current output and never surface the assistant’s final answer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/specialist/harness_decode.go` around lines 97 - 152, Update
codexJSONDecoder to parse the current codex exec --json event schema instead of
the legacy nested msg format, since decodeLine currently only handles
agent_message/task_complete/token_count and will ignore modern top-level events.
Adjust codexLine and decodeLine to recognize item.completed and turn.completed
(including usage on turn.completed.usage), and make sure the final assistant
text is still preserved and emitted via lastMessage/finish. Keep the fix
localized to codexJSONDecoder, codexLine, and decodeLine so current Codex JSONL
output is surfaced correctly.
| detections := agentcli.Detect(agentcli.Deps{}) | ||
| state := setupState{ | ||
| visible: options.Visible, | ||
| required: options.Required, | ||
| configPath: strings.TrimSpace(options.ConfigPath), | ||
| providers: providers, | ||
| allProviders: providers, | ||
| apiKey: apiKey, | ||
| agentCLIDetections: detections, | ||
| } | ||
| // Don't open the method chooser with the cursor parked on an informational | ||
| // (unselectable) CLI row — mirrors providerWizardState's identical guard. | ||
| state.selectedMethod = firstSelectableMethodIndex(filterSetupMethodOptions(providerWizardMethodOptions(detections), providers)) | ||
| return state | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm callers of newSetupState and inspect the keychain Deps default for a timeout.
rg -n "newSetupState\(" --type=go
rg -n -A10 "func resolveDeps" internal/agentcli/agentcli.go
rg -n -A15 "security.*find-generic-password" internal/agentcliRepository: Gitlawb/zero
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## newSetupState call sites\n'
rg -n "newSetupState\(" --type=go .
printf '\n## agentcli files\n'
git ls-files | rg '^internal/agentcli/.*\.go$'
printf '\n## inspect agentcli implementation\n'
for f in $(git ls-files | rg '^internal/agentcli/.*\.go$'); do
echo "### $f"
ast-grep outline "$f" --view expanded || true
doneRepository: Gitlawb/zero
Length of output: 3793
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## locate setup state and setup flow\n'
rg -n "type setupState|func newSetupState|setupStages|setupReturnToMethod|providerWizardMethodCLI|providerWizardMethodOptions" internal/tui -g '*.go'
printf '\n## inspect onboarding file sections around newSetupState\n'
wc -l internal/tui/onboarding.go
sed -n '1,220p' internal/tui/onboarding.go
printf '\n## inspect agentcli detection code\n'
sed -n '1,260p' internal/agentcli/*.goRepository: Gitlawb/zero
Length of output: 19279
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## exact detect-related references\n'
rg -n "Detect\(|type Deps|LookPath|find-generic-password|security" internal/agentcli internal/tui -g '*.go'
printf '\n## find startup entry points using onboarding\n'
rg -n "onboarding|setupState|newSetupState\(" . -g '*.go'Repository: Gitlawb/zero
Length of output: 8093
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## inspect model initialization around newSetupState\n'
sed -n '720,790p' internal/tui/model.go
printf '\n## inspect CLI/app setup launch path and onboarding gating\n'
sed -n '1,220p' internal/cli/setup.go
printf '\n---\n'
sed -n '520,620p' internal/cli/app.goRepository: Gitlawb/zero
Length of output: 15102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## inspect agentcli Detect and resolveDeps\n'
sed -n '280,390p' internal/agentcli/agentcli.goRepository: Gitlawb/zero
Length of output: 3125
Defer agentcli.Detect until setup is needed
newSetupState runs agentcli.Detect(agentcli.Deps{}) synchronously during interactive TUI startup, so the PATH/file/keychain probes happen before the UI renders even when setup isn’t shown. The macOS security call has no timeout, so a stalled keychain prompt can freeze the TUI.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/onboarding.go` around lines 124 - 138, `newSetupState` eagerly
calls `agentcli.Detect(agentcli.Deps{})` during TUI startup, causing blocking
PATH/file/keychain probes before setup is actually needed. Move the detection
behind the setup flow so it only runs when the onboarding/setup UI is opened,
and keep `setupState` initialization in `newSetupState` limited to cheap fields
like `visible`, `required`, `configPath`, and `selectedMethod`. Use
`agentcli.Detect` and `setupState` as the main points to refactor so the initial
render stays responsive.
| m := newModel(context.Background(), Options{}) | ||
| m.providerWizard = m.newProviderWizard() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Avoid real CLI detection before overriding test detections.
m.newProviderWizard() runs agentcli.Detect(agentcli.Deps{}) before these tests replace agentCLIDetections, so they can still touch the developer’s PATH/keychain. Build the wizard state with injected detections directly, or add a test-only deps hook.
Also applies to: 166-167
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/provider_wizard_cli_test.go` around lines 112 - 113, The
provider wizard tests are constructing the wizard through m.newProviderWizard(),
which still triggers agentcli.Detect before the test overrides are applied.
Update the setup to build the wizard state with injected detections directly, or
add a test-only deps hook so newModel and providerWizard initialization do not
touch real CLI detection. Keep the fix centered around newModel,
m.newProviderWizard, and the agentCLIDetections override used in these tests.
| options := wizard.methodOptions() | ||
| wizard.selectedMethod = clampInt(wizard.selectedMethod, 0, maxInt(0, len(options)-1)) | ||
| wizard.err = "" | ||
| if len(options) > 0 && options[wizard.selectedMethod].oauth { | ||
| if len(options) == 0 { | ||
| return | ||
| } | ||
| selected := options[wizard.selectedMethod] | ||
| wizard.oauthMode = false | ||
| wizard.cliHarness = nil | ||
| switch selected.kind { | ||
| case providerWizardMethodOAuth: | ||
| wizard.oauthMode = true | ||
| wizard.providers = providerWizardOAuthDescriptors() | ||
| } else { | ||
| wizard.oauthMode = false | ||
| wizard.selectedProvider = 0 | ||
| wizard.refreshModels() | ||
| wizard.step = providerWizardStepProvider | ||
| case providerWizardMethodCLI: | ||
| if selected.disabled { | ||
| // Informational row only — nothing to advance into. | ||
| return | ||
| } | ||
| if !selected.loggedIn { | ||
| wizard.err = selected.harness.DisplayName + " is not logged in — run `" + | ||
| selected.harness.LoginCommand() + "` to log in, then try again." | ||
| return |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Refresh CLI login state before rejecting the harness.
methodOptions() uses login state captured when the wizard opened. After this error tells users to run codex login/claude login and try again, the same wizard still sees selected.loggedIn == false until it is reopened. Reprobe this harness or refresh agentCLIDetections before returning the logged-out error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/provider_wizard.go` around lines 543 - 567, The CLI method
selection in providerWizard.go is using stale login state from methodOptions(),
so the wizard can keep rejecting a harness after the user logs in. Before
setting the logged-out error in the providerWizardMethodCLI branch, refresh the
underlying CLI detection state (or re-run the harness probe used by
agentCLIDetections) and rebuild the selected option so selected.loggedIn
reflects the current status. Keep the fix localized around the switch on
selected.kind and the selected.harness/login check.
| func (m model) handleOnlyBashCommand(args string) (model, string) { | ||
| args = strings.TrimSpace(strings.ToLower(args)) | ||
| switch args { | ||
| case "", "toggle": | ||
| if m.onlyBashActive { | ||
| m = m.disableOnlyBash() | ||
| } else { | ||
| m = m.enableOnlyBash() | ||
| } | ||
| case "on": | ||
| m = m.enableOnlyBash() | ||
| case "off": | ||
| m = m.disableOnlyBash() | ||
| case "status": | ||
| // report only — fall through to onlyBashText() below | ||
| default: | ||
| return m, "Onlybash\nUsage: /onlybash [on|off|status]" | ||
| } | ||
| return m, m.onlyBashText() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Bug: /onlybash off clobbers pre-existing filters when never turned on.
enableOnlyBash guards stashing with if !m.onlyBashActive, but disableOnlyBash has no symmetric guard. Calling /onlybash off (or explicit "off") while onlyBashActive is already false unconditionally overwrites agentOptions.EnabledTools/DisabledTools with the stash fields — which are nil if onlybash was never enabled — silently wiping any operator-configured tool restrictions (e.g. a deliberately disabled "bash" tool). None of the provided tests cover this path (all "off" tests first call "on").
🐛 Proposed fix
func (m model) disableOnlyBash() model {
+ if !m.onlyBashActive {
+ return m
+ }
m.agentOptions.EnabledTools = m.onlyBashStashedEnabledTools
m.agentOptions.DisabledTools = m.onlyBashStashedDisabledTools
m.onlyBashStashedEnabledTools = nil
m.onlyBashStashedDisabledTools = nil
m.onlyBashActive = false
return m
}Also applies to: 346-353
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/session_controls.go` around lines 300 - 319, The /onlybash off
path in handleOnlyBashCommand can wipe existing tool restrictions when onlybash
was never enabled, because disableOnlyBash restores from empty stash fields
unconditionally. Update disableOnlyBash (and any callers like
handleOnlyBashCommand) to no-op unless onlyBashActive is true, mirroring the
guard already used by enableOnlyBash, so pre-existing agentOptions.EnabledTools
and DisabledTools are preserved. Add or adjust coverage for the explicit
off/status flow where onlybash starts inactive and an operator-configured tool
restriction already exists.
Brings in main's independent work since divergence: `zero providers models` CLI discovery, config resolver/paths changes, gemini provider updates, sandbox test fixes, and tools/edit_file + web_search changes, alongside this branch's AuthCLI auto-detection and Claude Code OAuth work. Resolved conflicts (keep-both) in: - internal/cli/app.go: appDeps struct/wiring union of discoverProviderModels (main) and detectAgentCLIs (this branch) - internal/specialist/manifest.go: import union of agentcli and config Also wires AuthCLI-detected credentials (e.g. Claude Code) into main's new discoverProviderModels so `zero providers models` authenticates AuthCLI profiles the same way a real request would: a stored API key is used verbatim, an OAuth access token becomes an Authorization: Bearer header, and the anthropic catalog additionally gets the oauth-2025-04-20 beta header. Adds internal/cli/provider_models.go:applyAuthCLIDiscoveryCredential plus unit tests covering anthropic/chatgpt x APIKey/AccessToken.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/cli/provider_models.go (2)
110-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider wiring AuthCLI credential extraction through
appDepsfor testability/consistency.
discoveryCredentialProfilecallsagentcli.Lookup/agentcli.ExtractCredentialsas bare package functions rather than through an injectable dependency, unlikeprovider_detect.go'sdeps.detectAgentCLIs(defaulting toagentcli.Detect). As a result,provider_models_test.gocan only unit-test the pureapplyAuthCLIDiscoveryCredentialhelper — there's no coverage for the actualAuthCLIdispatch branch (harness lookup + credential extraction feeding into the discovery profile), and this path can't be mocked without touching real files/keychain.♻️ Illustrative sketch (adjust to match appDeps' actual shape)
-func discoveryCredentialProfile(profile config.ProviderProfile) config.ProviderProfile { +func discoveryCredentialProfile(profile config.ProviderProfile, extractCreds func(agentcli.Harness, agentcli.Deps) (agentcli.Credentials, bool, error)) config.ProviderProfile { authed := profile if strings.TrimSpace(authed.AuthCLI) != "" { if harness, ok := agentcli.Lookup(authed.AuthCLI); ok && harness.CatalogID != "" { - if creds, ok, err := agentcli.ExtractCredentials(harness, agentcli.Deps{}); err == nil && ok { + if creds, ok, err := extractCreds(harness, agentcli.Deps{}); err == nil && ok { authed = applyAuthCLIDiscoveryCredential(authed, harness.CatalogID, creds) } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/provider_models.go` around lines 110 - 124, `discoveryCredentialProfile` currently hardcodes `agentcli.Lookup` and `agentcli.ExtractCredentials`, which makes the AuthCLI discovery branch impossible to mock in tests. Route this logic through `appDeps` (following the pattern used by `provider_detect.go` and `deps.detectAgentCLIs`) so the harness lookup and credential extraction can be injected, then update the `discoveryCredentialProfile` call sites and tests in `provider_models_test.go` to cover the full AuthCLI path, not just `applyAuthCLIDiscoveryCredential`.
143-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the Anthropic OAuth beta string into a shared constant. The same
"oauth-2025-04-20"literal is duplicated in the CLI discovery path and the Claude CLI auth provider; centralizing it in a shared package will keep those call sites from drifting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/provider_models.go` around lines 143 - 163, The Anthropic OAuth beta value is hardcoded in applyAuthCLIDiscoveryCredential and duplicated elsewhere, so hoist the "oauth-2025-04-20" string into a shared constant in a common package and update applyAuthCLIDiscoveryCredential to use that constant when setting the anthropic-beta header.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/cli/provider_models.go`:
- Around line 110-124: `discoveryCredentialProfile` currently hardcodes
`agentcli.Lookup` and `agentcli.ExtractCredentials`, which makes the AuthCLI
discovery branch impossible to mock in tests. Route this logic through `appDeps`
(following the pattern used by `provider_detect.go` and `deps.detectAgentCLIs`)
so the harness lookup and credential extraction can be injected, then update the
`discoveryCredentialProfile` call sites and tests in `provider_models_test.go`
to cover the full AuthCLI path, not just `applyAuthCLIDiscoveryCredential`.
- Around line 143-163: The Anthropic OAuth beta value is hardcoded in
applyAuthCLIDiscoveryCredential and duplicated elsewhere, so hoist the
"oauth-2025-04-20" string into a shared constant in a common package and update
applyAuthCLIDiscoveryCredential to use that constant when setting the
anthropic-beta header.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5eb3c0ab-f4d3-4739-a917-610a270fd32f
📒 Files selected for processing (11)
.gitignoreinternal/agent/loop_test.gointernal/cli/app.gointernal/cli/provider_detect_test.gointernal/cli/provider_models.gointernal/cli/provider_models_test.gointernal/cli/setup_test.gointernal/specialist/harness_dispatch_test.gointernal/specialist/manifest.gointernal/tui/model.gointernal/tui/picker.go
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/tui/picker.go
- internal/cli/app.go
- internal/specialist/manifest.go
Summary
Linked issue
This pull request introduces a new package,
internal/agentcli, which provides a unified API for detecting, probing, and extracting credentials from third-party agent CLI tools (such as Claude, Codex, Gemini CLI, etc.). It also adds comprehensive tests for this package, and strengthens invariants in the agent tool filtering logic. Additionally, it documents a full code review and records that no defects were found in a recent audit.The most important changes are:
New agent CLI detection and credential extraction API
agentclipackage, which catalogs known agent CLI harnesses, provides functions to detect their presence onPATH, probes their login state via credential files or keychain entries, and exposes a stable, testable API for other packages to build on. This centralizes all harness-specific logic in one place, making maintenance and updates easier. (internal/agentcli/agentcli.go)internal/agentcli/agentcli.go)Comprehensive tests for agentcli
agentclipackage, covering catalog invariants, detection logic, credential probing (including file and keychain scenarios), and API behaviors. (internal/agentcli/agentcli_test.go)Strengthened tool filtering invariants
/onlybashmode strictly allowlists onlybashandskilltools, ensuring that delegation (e.g., to "Task") is blocked and cannot escape the restricted surface. (internal/agent/loop_test.go)Documentation and audit records
cascade/00-task.txt)cascade/10-findings.cf)Fixes #
Checklist
issue-approvedlabel.go build ./...,go vet ./..., andgo test ./...pass locally.gofmtclean.-racewhere relevant).Summary by CodeRabbit
New Features
/onlybashmode to quickly restrict the toolset to bash and skill tools.Bug Fixes