Conversation
…inyhumansai#2394) The group was rendered permanently empty and unselectable, which reads as "ACP is switched off on this host" — while the backend already reports claude and codex as detected and runnable. It is retired rather than finished because there is nothing a company-level row could ever hold: a CLI login has no key and no endpoint, and a stored "connected" flag would be a second source of truth that could disagree with the CLI actually being installed. Binding is per teammate and already works from an agent's Model tab, so the dialog links there instead.
…nyhumansai#2394) Both lost their only consumer when the add-provider dialog stopped rendering the category. A `false` flag nothing reads is the same second-source-of-truth shape the category was retired for, so the reasoning moves onto the `cli` field that survives — still populated, because the self-managed setup step resolves an already-connected CLI row's label through it.
tinyhumansai#2394) The comment implied the handler would start accepting these once a delegated credential existed. It will not: there is no key and no endpoint to store, so the refusal is the design rather than a gap waiting to be filled.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe add-provider dialog now exposes cloud and local providers only. It directs Claude Code and Codex binding to the teammate roster. CLI connection constants and related empty-state behavior were removed, and tests verify the updated dialog. ChangesCLI login retirement
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The dialog now removes an unavailable provider path and directs users to the teammate roster. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
I’m a rabbit with a tidy tray, Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Tiny Sweeper reviewReviewed 4 files; 1 finding. _The code index is behind this pull request (indexed at `93d1768791d8`), so retrieved context may be out of date._ _Memory was unavailable (model: cortex: v1/recall: timed out after 10s), so this review ran without it._ State: Ready for maintainer review Review snapshot
Completeness: Complete What changed- Removed the CLI-logins category from the add-provider dialog, including the `CLI_LOGINS_REACHABLE` export and `CLI_LOGINS_UNAVAILABLE` constant. Features
Tests
Findings
Before mergeNone. How this fits togetherflowchart LR
n0["plan_add<br/>changed"]:::changed
n1["AddProviderDialog<br/>changed<br/>1 finding"]:::flagged
n2["Category<br/>changed<br/>1 finding"]:::flagged
n3["addOptions<br/>changed"]:::changed
n4["cloud_provider"]:::impacted
n5["options"]:::impacted
n6["endpointHost"]:::impacted
n7["isConnected"]:::impacted
n8["Provider"]:::impacted
n9["format"]:::impacted
n0 -->|calls| n4
n0 -->|calls| n9
n1 -->|uses| n2
n1 -->|uses| n5
n1 -->|uses| n8
n2 -->|uses| n5
n3 -->|calls| n6
n3 -->|calls| n7
n3 -->|uses| n8
n5 -->|calls| n3
n7 -->|uses| n8
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Agent review detailscritique
security
tests
commits
description
e2e
Evidence and run details
|
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0126 · 210,863 in / 11,550 out · 8,031 cached (4%) · ladder/vectors, gpt-5.6-luna, deepseek/deepseek-v4-flash · 595 embedded
critique: $0.0050 · 91,939 in / 3,436 out · 6,169 cached (7%) · gpt-5.6-luna, deepseek/deepseek-v4-flash
security: $0.0038 · 71,338 in / 1,638 out · 1,862 cached (3%) · gpt-5.6-luna
tests: $0.0018 · 15,562 in / 2,278 out · 0 cached (0%) · deepseek/deepseek-v4-flash
description: $0.0004 · 7,235 in / 83 out · 0 cached (0%) · deepseek/deepseek-v4-flash
e2e: $0.0012 · 19,754 in / 2,640 out · 0 cached (0%) · deepseek/deepseek-v4-flash
| emptyNote={CLI_LOGINS_UNAVAILABLE} | ||
| onChoose={onChoose} | ||
| /> | ||
|
|
There was a problem hiding this comment.
Cover the add-provider dialog removal with an end-to-end test
The PR removes the CLI logins group from the AddProviderDialog, a change visible to every user who opens the dialog. No end-to-end test exercises this dialog to verify the absence of the group or the presence of the new harness hint. A unit test was added, but the external surface (the rendered dialog) is not driven by any e2e suite. Without e2e coverage, a regression that re-introduces the group or breaks the hint would go unnoticed in CI.
[RULE] e2e-uncovered ·
There was a problem hiding this comment.
Acknowledged: the AddProviderDialog e2e coverage gap is fair to flag. However, this PR is scoped to retire the CLI-logins group; adding a new e2e test for the dialog itself expands scope beyond the card's boundaries and belongs as a follow-up feature request to the console's e2e suite, not in this PR. The change is validated by the unit test added here (the absence of the group in the rendered state) and by manual verification during development. Coverage of the full dialog surface (including harness hint rendering) is reasonable follow-up work for the e2e suite.
tinysweeper
Bot
added
the
priority: p2
1 participant
Summary
The LLM/Providers page's Add a provider dialog rendered a "CLI logins" group that was permanently empty and unselectable —
CLI_LOGINS_REACHABLEwas pinnedfalse, so the category was handed[]and showed "Not available on this host." A group named after exactly the thing an operator wants, greyed out, reads as "ACP is switched off here" — whileGET {scope}/harnessesalready reportsclaudeandcodexasdetected: true, runsHere: trueon a desktop build.This retires the group rather than finishing it. There is nothing a company-level row could ever hold for a CLI login: no key, no endpoint, and — per the External Harnesses design — deliberately no stored "connected" flag, since that would be a second source of truth that could disagree with the CLI actually being installed. Binding is per teammate and already works today from an agent's Model tab, so the dialog links there instead.
First of four PRs implementing #2394's rollout. Independent of the rest, in both directions.
API Or Behavior Changes
#/company/agents, which closes the dialog on the way out.CLI_LOGINS_REACHABLEandCLI_LOGINS_UNAVAILABLEare deleted. Both lost their only consumer with the group; afalseflag nothing reads is the same second-source-of-truth shape the category was retired for. The reasoning moves ontoaddOptions' survivingclifield, which stays populated because the self-managed step resolves an already-connected CLI row's label through it.Category's now-unusedemptyNoteprop removed.cli_loginrefusal stays exactly as it was; only its comment changes, from implying a future delegated-credential fix to stating the refusal is permanent by design.Category::Cli,CLI_LOGINS,ProviderRef::ClaudeCodeand thehas_categoryrouting guard are all untouched — still load-bearing.catalogue_tests_mirror.rspasses unmodified.Tests
New:
frontend/test/unit/inference-add-cli-group-retired.test.ts— asserts no#inference-add-clicontrol, no "CLI logins" or "Not available on this host." text in the dialog, and that the roster link carries#/company/agentsand closes the dialog when taken. Verified this fails (2/2) against the pre-change sources before passing, rather than passing vacuously.npm run typechecknpm run typecheck:unitnpm run typecheck:e2enpx vitest runover the new test plusinference-add-dialog-close,inference-connect,setup-wizard-self-managed-step1— 60 passedscripts/ci/assert-setup-inference-imports.shscripts/ci/assert-design-tokens.shcargo fmt -p opencompany-core --checkcargo check -p opencompany-corecargo clippy -p opencompany-core --no-deps --all-targets -- -D warningscargo test— N/A, no Rust behavior changed (comment only)Manual verification in the running console (dev server against a live host): the dialog shows Cloud and Local runtimes only, with the pointer line beneath; clicking Open the roster navigates to the Agents roster and closes the dialog.
Documentation
No spec change needed here.
docs/spec/runtime/external-harnesses-ui.mddescribes the External Harnesses settings surface, which this PR does not touch — it is updated in the PR that folds that page into a per-harness detail view.🤖 Generated with Claude Code
Summary by CodeRabbit