Skip to content

feat(cursor): expose native composite model picker - #489

Open
keh4l wants to merge 7 commits into
xintaofei:mainfrom
keh4l:codex/upstream-cursor-model-controls
Open

feat(cursor): expose native composite model picker#489
keh4l wants to merge 7 commits into
xintaofei:mainfrom
keh4l:codex/upstream-cursor-model-controls

Conversation

@keh4l

@keh4l keh4l commented Aug 16, 2026

Copy link
Copy Markdown

Summary

  • opt Cursor ACP into its native parameterized model picker capability
  • present Cursor CLI variants such as High Fast, 1M Thinking, and Extra High Thinking Fast as searchable single-row choices
  • translate each selected row into the base model and parameter values actually advertised by Cursor ACP; CLI aliases never enter the ACP wire
  • restore saved Cursor settings model-first and only persist an acknowledged, authoritative configuration

Why

Cursor exposes model selection through two dynamic surfaces:

  1. cursor-agent models provides the account/version-specific flattened catalog shown by the CLI.
  2. ACP publishes base model IDs and model-dependent reasoning, effort, thinking, context, and fast options.

Without _meta.parameterizedModelPicker: true, Cursor falls back to compatibility tuples, so Codeg cannot expose the complete native controls. Conversely, sending the CLI's flattened alias as the ACP model ID is rejected by Cursor.

This change intersects the live CLI catalog with the live ACP catalog, accepts only globally unique mappings, and keeps the flattened alias as a local UI key. The ACP request contains only Cursor-advertised raw values. Auto (default) keeps Cursor's real default value.

Safety and compatibility

  • Cursor-only capability negotiation; other agents keep their existing behavior.
  • Raw ACP model values continue through the normal wire path even when a composite catalog exists.
  • Unknown/internal __codeg_* values fail closed before any ACP request.
  • Mapping is bounded and ambiguity-safe (shared 8192-state budget; no unbounded Cartesian expansion).
  • Probe execution has null stdin, bounded streaming output (1 MiB stdout / 256 KiB stderr), a 30-second deadline, bounded cache/single-flight behavior, and secret-fingerprinted cache keys without retaining plaintext credentials.
  • Subscription and custom/API-key launches use the same effective Cursor environment policy as the model probe.
  • Unix probe cleanup owns an isolated process group, retains leader identity through waitid(WNOWAIT), retires the lease before final reap, and does not signal stale numeric PID/PGID values after identity loss.
  • Modern config operations are correlated by operation ID. Legacy backends use a fixed 15-second confirmation deadline and cannot let a replaced operation's late error roll back the current selection.
  • Existing host-tools capability gating and disconnect cleanup from current main are preserved.

Validation

  • Cursor frontend focus: 5 files, 175 tests passed.
  • Cursor Rust focus: 53 tests passed.
  • Process/ETXTBSY focus: 7 tests passed.
  • Frontend full suite: 305 files, 4126 tests passed.
  • pnpm eslint .: passed.
  • pnpm build: passed; 32 static routes generated.
  • Server check and clippy with -D warnings: passed.
  • codeg-mcp check and clippy with -D warnings: passed.
  • Server full suite: 2583 passed, 1 ignored. The two remaining failures when run as root are existing chmod-semantics tests; both pass when rerun as the non-privileged www user.
  • Changed frontend files pass Prettier; changed Rust files pass rustfmt. Repository-wide rustfmt drift decreases from 123 baseline files to 117 and adds no new drift.
  • A sanitized real Cursor catalog fixture maps 204/204 CLI rows through the production parser and mapper.

Desktop cargo check is blocked on this host before project-code compilation because the GLib/GIO development packages are not installed. No paid model prompt was issued.

@keh4l

keh4l commented Aug 16, 2026

Copy link
Copy Markdown
Author

CI follow-up is ready in 56a53623 (normal push, no history rewrite).

Fixed the previous run failures:

  • macOS: replaced the non-portable getpgid(zombie) assertion with a second waitid(WNOWAIT) identity-anchor check.
  • Windows: cfg-gated Unix-only lifecycle types/events and removed the unused non-Unix process-group method; restructured PID cleanup to avoid the unnecessary unwrap/unused-variable warnings.
  • Ubuntu desktop: resolved all five -D warnings Clippy findings (is_none_or, constant assertion, and contains).
  • Merged current upstream 3845e9df and preserved both Cursor parameterized-picker and DeepSeek elicitation capabilities. The PR is now mergeable/clean.

Local verification on the merged tree:

  • Cursor probe lifecycle tests: 12/12 passed.
  • cargo clippy --no-default-features --features test-utils --all-targets -- -D warnings: passed.
  • Server Clippy and codeg-mcp check/Clippy: passed.
  • Frontend ESLint: passed.
  • Frontend Vitest: 307 files / 4157 tests passed.
  • Next static build: passed, 32 pages.
  • Server tests: 2600 passed, 1 ignored; the two root/chmod semantic tests fail only under root and both pass when rerun as non-privileged www.

The new upstream run has no jobs and is marked action_required, so a maintainer needs to approve it: https://github.com/xintaofei/codeg/actions/runs/31938768396

@xintaofei

Copy link
Copy Markdown
Owner

Thanks for this — I spent a while on it, and I want to say up front that the protocol work here is genuinely good. I verified your central claims against a live cursor-agent rather than just reading the diff, and they hold up:

  • clientSupportsParameterizedModelPicker() in the shipped bundle really does check _meta.parameterizedModelPicker === true strictly, and getModelPickerMode() really does switch between "parameterized" and "variants".
  • Driving cursor-agent acp over stdio in both modes confirms it. Without the capability you get 35 frozen tuples with raw slugs as labels ('grok-4.6[effort=high,fast=true]' => 'grok-4.6'); with it you get 35 base models with real names plus model-dependent reasoning/fast selectors.
  • The 204/204 fixture mapping is real, the fail-closed guards work (__codeg_* values genuinely cannot reach the wire), and the wire plan writes every parameter explicitly so there's no stale-parameter leakage. I also went looking for a bug where the mapper's reliance on catalog currentValue would make label meanings drift after a user changes a parameter — tested it, and it doesn't happen. Nice.

CI is green on all seven jobs, so nothing below is a "CI is red" complaint. But I don't think it's quite ready to merge yet. Here's what I found.


1. The CURSOR_API_BASE_URL field is an undisclosed revert

This is my main concern. The PR re-adds a user-facing endpoint field to the Cursor settings panel and threads base_url back through the whole probe chain — and deletes the guard test "API-key mode writes the key + model and always scrubs the dead base URL".

Commit 689a55ce on main removed exactly this, deliberately, with the rationale "cursor-agent has no custom-endpoint support". That commit is an ancestor of this PR's merge base, so the revert is intentional in effect — but the PR body doesn't mention it at all, and the deleted test was the thing encoding the earlier decision.

For what it's worth, I grepped the shipped bundle to see what the variable actually does. It's read in exactly one meaningful place — index.js, to build the auth-poll URL (${base}/auth/poll, defaulting to https://api2.cursor.sh). In cursor-agent-svc.js, the service that actually runs models, it's read and discarded. So it isn't a general bring-your-own-endpoint knob. (Minor: the new placeholder says https://api.cursor.com, but the CLI's real default is https://api2.cursor.sh.)

Bringing it back may well be defensible! But it's a separate product decision and should ride in its own PR with its own justification, rather than inside a model-picker change.

2. tsc --noEmit regresses from 0 → 4 errors

Merge base 3845e9df is clean; this branch has 4, all in src/contexts/acp-connections-context.test.tsx:

(2151,28) TS2345 error-event fixture missing required `code`
(2533,9)  TS2322 'string | undefined' not assignable to 'string | null'
(2572,30) TS2345 error-event fixture missing required `code`
(2940,9)  TS2322 'Promise<boolean>' not assignable to 'Promise<void>'

pnpm build and vitest both skip test-file typechecking, so this slips past every gate — including CI. Two of them matter beyond hygiene: those fixtures construct error events without code, a shape the declared AcpEvent type forbids and the backend can't emit, so those cases may be asserting against something that can't happen.

3. Connect-path latency — two serial costs, one paid on every connection

  • cursor_models_for_runtime() is awaited at connection.rs:1272, before build_agent(). On a cache miss every Cursor connection waits for a cursor-agent models subprocess — I measured 1.46s / 2.35s / 4.60s. 300s TTL, so app launch or any >5min idle pays it again.
  • Bigger one: after initialize and before session/new, every connection with a CLI catalog also awaits cursor/list_available_models (connection.rs:4888), uncached. Measured 4.16s cold in a fresh agent process. Since every connection is a fresh process, cold is the normal case.

Neither request is needed to start a session — only to build the picker. Issuing them concurrently with session establishment (or lazily after) would make this disappear.

4. The probe tests are fragile on some hosts

Five new cursor_probe_* tests fail for me under cargo test --features test-utils on macOS arm64. Four only fail under parallel load. But cursor_probe_timeout_kills_and_reaps_descendants fails even run alone in serial (0 passed; 1 failed, 0.27s), reproducibly.

The cause is a 150ms race: the fake #!/bin/sh CLI has to fork a grandchild and write $PID_FILE within the probe's 150ms timeout, and the assertion at src/commands/acp.rs:16156 then reads that file with no retry. Where shell startup + fork + printf exceeds 150ms, cleanup kills the writer first.

To be fair: your CI does run macos-latest and it passes, so this is host-sensitive rather than broken. Worth noting the fix isn't "poll for the file" — by then the writer is already killed and reaped. It needs a realistic timeout or an explicit readiness handshake.

5. Worth an explicit decision: is the 204-row picker worth its machinery?

Not a defect, just something I'd like your view on, because the cost is large.

Some scoping first, in fairness to what main already does: the composer can already live-switch across those 35 tuples, and the settings panel already lists the full cursor-agent models catalog and applies the chosen alias via root-level --model at launch. So all 204 variants are already reachable today — as a global default needing a reconnect. The real delta this PR buys is live in-session access to the full 204 with their parameters, plus readable labels. Genuinely worth having.

The question is the price. The flattened picker costs a label reverse-engineering mapper (tokenize CLI display names, prefix-match ACP names, bounded backtracking over suffix words against ACP option display names, 8192-state budget) that is fail-closed — any wording change on either side silently drops rows behind a tracing::debug!. Plus ~900 lines of probe process-lifecycle machinery (waitid(WNOWAIT), /proc group-quiescence scanning, PID-reuse defense), plus a dual modern/legacy frontend state machine with legacyAllowed / unresolvedPredecessor / deferredOptions and a 15s timer.

And the simpler design already exists in this PR as its own fallback: when the CLI probe fails you show Cursor's native parameterized picker, which is robust to any vendor renaming and is essentially the _meta opt-in alone.

The strongest argument I found for your approach, which I want to credit: the ACP catalog's full Cartesian product is 376 combos versus the CLI's 204 published rows, so the intersection really does prune 172 combinations Cursor doesn't advertise. That's a real benefit. I just think it's worth deciding consciously whether it justifies the mapper + lifecycle + legacy-fallback carrying cost, versus shipping the parameterized picker and revisiting later.

6. Small thing

~145 hunks / ~784 lines (19% of hunks in the changed Rust files) are pure rustfmt reflow of untouched code. CLAUDE.md deliberately leaves cargo fmt out of the required checks and the crate isn't rustfmt-clean, so this mostly just inflates an already 14k-line diff. Dropping those would make re-review much easier.


Where I land

The capability opt-in is correct and I'd like to see it land. Before merging, I'd ask for: (1) splitting out the CURSOR_API_BASE_URL change (or dropping it and restoring the guard test), (2) the 4 tsc errors, and (3) moving the catalog fetches off the critical connect path. The fragile tests and the rustfmt churn I'd call "should fix", and #5 is a design call that's yours to make.

Happy to look again once you've had a chance — and thanks again for the protocol spadework, the parameterizedModelPicker / cursor/list_available_models findings are solid and useful regardless of how the picker shakes out.

Verified locally against 56a53623: pnpm eslint . ✅ · full vitest 307 files / 4157 tests ✅ · desktop clippy -D warnings ✅ · server check + clippy ✅ · tsc --noEmit ❌ 4 (0 at base) · cargo test --features test-utils ❌ 5 cursor_probe_*. Protocol behaviour confirmed by driving cursor-agent acp directly; no prompt was ever sent.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants