Skip to content

fix(cli): apply client selector to registry integrations (#1798) - #1802

Merged
DeusData merged 1 commit into
DeusData:mainfrom
rarepops:fix/install-clients-registry-1798
Sep 2, 2026
Merged

fix(cli): apply client selector to registry integrations (#1798)#1802
DeusData merged 1 commit into
DeusData:mainfrom
rarepops:fix/install-clients-registry-1798

Conversation

@rarepops

@rarepops rarepops commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #1798.

--clients previously filtered only the legacy client table, so registry-backed clients such as Qoder and Rovo Dev were still detected and configured. This change makes registry stable IDs part of the selector vocabulary and applies the same restriction to registry detection output and installation.

The selector remains restrictive: it configures only selected clients that are detected, and does not force-enable undetected clients.

Validation covered the sanitizer build and both selector regressions. The full CLI suite passes with 283 tests. clang-format 20.1.8 and git diff --check pass; focused cppcheck reports only pre-existing style findings in test helpers and older test code.

Checklist

  • Every commit is signed off (git commit -s) - required, CI rejects unsigned commits (DCO, see CONTRIBUTING.md)
  • Tests pass locally (cli: 283 passed)
  • Lint passes (make -f Makefile.cbm lint-ci) - upstream lint / lint and lint / lint-mem checks pass
  • New behavior is covered by a test (reproduce-first for bug fixes)

Signed-off-by: Rares Popa <2606875+rarepops@users.noreply.github.com>
@rarepops
rarepops requested a review from DeusData as a code owner August 22, 2026 18:42
@github-actions

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

@DeusData DeusData added bug Something isn't working editor/integration Editor compatibility and CLI integration priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Aug 24, 2026
@DeusData

Copy link
Copy Markdown
Owner

Thank you for the focused selector fix and regression coverage. I checked current main: --clients is applied to the legacy cbm_detected_agents_t set, after which install_agent_client_registry() is called separately without that selected-client restriction. That grounds the registry-backed client leak described here.

I have labeled this as a high-priority editor-integration bug and queued it for review. CI is green. Our review queue is full, so detailed review may take a little time, but the PR is now classified and routed. Thank you for preserving the selector's restrictive semantics rather than force-enabling undetected clients.

@DeusData

DeusData commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Approved on merit. I verified the gap on main rather than taking it: cli_clients_apply_selection is applied to the legacy probe at cli.c:10280, while print_detected_registry_agents (:8283, called from :7510) iterates registry profiles and calls cbm_agent_client_detect(profile->id, …) with no reference to any selection at all. So registry-backed clients genuinely bypassed --clients entirely.

That matters more than a missing filter usually would, because it fails in the configuring direction. A user writing --clients=claude is stating what they want touched; silently configuring Qoder and Rovo Dev anyway is the same class of harm as #1753, where a flag was accepted and had no effect. A selector that under-selects is a bug; one that over-configures is a surprise on someone's machine.

Two things I checked and want to credit:

The polarity is right. Both new guards read !g_client_selection || cli_clients_selects_registry_client(...) — no selection means no filtering, so the default path is byte-identical to today and only an explicit --clients restricts. The inverse form in the skip path is consistent with it. That is the shape that makes this safe to take without auditing every caller.

You kept the restrictive semantics explicit: selected and detected. Not force-enabling an undetected client is the right call, and stating it in the description saved me from having to derive it from the guards.

And a nice interaction you may not have noticed: adding registry stable_ids to cli_clients_print_list makes install --clients list the registry clients too. #1800 is currently open and adds --clients=<tokens> to the CLI help, pointing users at exactly that listing to discover valid tokens. Without your change that listing would have been incomplete for precisely the clients this PR fixes. The two are complementary; I have approved both.

Before merge

You are 205 commits behind main, so the 35/35 green here is stale. I am deliberately not rebasing: our Actions pool is servicing roughly one job at a time against 21 queued PR runs, and every rebase adds a full matrix to that. I will refresh it once the pool recovers.

Nothing to change in the code. Thank you — this is the third careful PR of yours I have read today, and closing the --clients gap on the registry side completes work that #1753 and #1800 only half-covered.

@DeusData

DeusData commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Merged as 4d8ac9e1. That is your fourth landing today — #1779, #1799, this, and the delta on #1773. Thank you.

The verification route is worth a note, since your branch was 205 commits behind and the green was stale. Our Actions pool has been servicing roughly one job at a time against a 38-deep queue, so rebasing would have cost you hours and everyone else a slot. Instead I built the actual merge result locally — current main plus this branch plus #1800, which is file-disjoint and complementary to it — and ran the cli and agent_clients suites: clean build, 329 passed. Verifying the combined tree meant one check covered the state after both merges rather than two.

On the fix: what made it easy to accept was the polarity. !g_client_selection || cli_clients_selects_registry_client(...) leaves the no-selector path byte-identical, so only an explicit --clients can restrict anything — that is what let me take it without auditing every caller. And the fixing SHA is recorded on #1798.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working editor/integration Editor compatibility and CLI integration priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

install --clients=<token> accepted but inert in the Windows binary: all detected clients configured, none force-included

2 participants