feat(adapters/cursor): populate models / model_dominant from the store's model label (closes #115)#118
Conversation
|
Reviewed this and the content is great. You did the one thing #115 actually needed: verified against a real install instead of trusting the web-research framing, and your findings correct it nicely (model name lives on One thing blocking the merge, and it's purely mechanical, not your code: the PR shows up as conflicting. That's because this is the same The good news is the only genuinely new work here is the model commit ( That drops the two already-merged commits and leaves only the model change, which should flip this green. Adjust the remote name if yours differs (it might be |
…model label (firstbatchxyz#113 follow-up) Verified against a real Cursor install where the model NAME lives and how Auto mode looks, then wired it into the adapter: - composerData.modelConfig.modelName is the authoritative source (present on every composer); each user bubble (type 1) also stamps modelInfo.modelName (the field CodeBurn reads), used as a fallback when the composer row is gone. - Auto mode records the literal "default" (not blank, not "auto"); normalize it to "auto" for a clean corpus signal. - No per-model tokens in the store, so model_dominant is the MOST FREQUENT model, not token-weighted like pi.py. Token/cost columns stay 0 — this adds the model NAME only; the firstbatchxyz#113 token/cost limitations are unchanged. Synthetic-fixture tests cover the composer-config path, the "default"->"auto" normalization, the bubble-modelInfo fallback, frequency-based dominance, and the no-model case (fields stay empty). Full suite green (510 passed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
de32114 to
a04d135
Compare
|
Fixed the branch — thanks for the clear diagnosis. Did exactly what you suggested (remote is The branch now sits on current main with a single new commit ( Ready for re-review whenever you are. |
The modern-shape test pinned project_dir to a literal "/home/u/proj/src", but the adapter derives it via os.path.commonpath, which uses the host separator — "\" on Windows — so the test failed on windows-latest CI while passing on macOS/Linux. Compare against os.path.normpath(...) so the expected value picks up the same separator the adapter produces. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
CI update — pushed one small follow-up commit (1814286):
Ready for re-review. |
…(F821) write_changelog's body calls `_maybe_auto_install(..., force=args.auto_install)` but the function had no `args` parameter, so ruff flagged F821 (undefined name) and the call would raise NameError at runtime (swallowed by the surrounding try/except, which is why it went unnoticed). Thread `args` through the single call site (main, where it's in scope) so the --auto-install flag actually takes effect and lint passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
CI is fully green now — went ahead and fixed the ruff failure too (commit 5dd1d89). Root cause: It's a pre-existing bug from the auto-install commit, not from this PR's model work — flagging it clearly so you can decide whether you'd rather pull it into a separate PR. Happy to split it out if you prefer to keep this PR scoped to the Cursor model change; just say the word and I'll revert it here and open it on its own. All 7 checks pass (lint + tests across macOS/ubuntu/windows × py3.11/3.12). Ready for re-review. |
aktasbatuhan
left a comment
There was a problem hiding this comment.
Perfect! 🎉
Thanks for the clean rebase and fixing that pre-existing ruff issue too. The undefined args in write_changelog() was indeed a bug from the auto-install commit — good catch threading it through properly so --auto-install actually works.
Happy to include the bug fix here since it's closely related (auto-install functionality) and you've already done the work. The scoped approach (model extraction + fixing the flag it depends on) makes sense.
Excellent work on the real-install verification and staying in scope. This closes #115 cleanly.
…120) The auto-install feature shipped in #113-era curate work was silently broken: write_changelog() called `_maybe_auto_install(..., force=args.auto_install)` but had no `args` parameter, so the NameError was swallowed by the surrounding bare `except Exception` and auto-install never ran. Fixed in #118 by threading `args` through; this adds the regression coverage that was missing. Three tests exercise the real write_changelog → _maybe_auto_install path (heavy git/state/index helpers stubbed so the test is hermetic): - force=args.auto_install reaches the installer and symlinks skills even with the project opt-in off (the --auto-install path that broke); - the force value is read from args, not a constant; - flag off installs nothing. Verified these fail (swallowed NameError → no install) against the buggy signature and pass on the fix. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Closes #115.
Verified the model-name question in #115 against a real Cursor install (
globalStorage/state.vscdb, tablecursorDiskKV, 15 composers), then wired the signal into the adapter. Scope stays exactly where #115 asked: model NAME only — token/cost stay 0, the #113 limitations are unchanged.Verification findings (the acceptance criteria)
Where the model name lives — confirmed:
composerData:<id>→modelConfig.modelNameis present on every composer (it's the picker's selection). This is the authoritative/primary source.bubbleId:<composer>:<bubble>→modelInfo.modelNameexists too — the field CodeBurn reads — but ontype:1(user) bubbles, not assistant bubbles. (Worth flagging: the issue/web-research framing implied per-assistant-message; it's actually stamped on the user turn.) We use it as a fallback when the composerData row is gone (orphan-bubble recovery).Auto mode — confirmed: the store records the literal string
"default"(not blank, not"auto"). 13/15 composers were"default"; the 2 with real messages were"composer-2.5". We normalize"default"→"auto"so the corpus carries an explicit, readable Auto signal.Tokens/cost — confirmed absent:
tokenCountis 0 as #113 documented, somodel_dominantis computed by frequency (composer pick = 1 vote, each modelInfo bubble = +1), not token-weighted likepi.py. No token/cost is introduced.End-to-end against the real DB: 14/15 composers now carry a model signal; the two real conversations report
composer-2.5; tokens/cost remain0/0.Changes
cursor.py:_normalize_model(default→auto),_composer_model(primary),_bubble_model(fallback); frequency-basedmodel_dominant; docstring updated to spell out the verified schema + that this is name-only.default→auto, bubble-modelInfo fallback, frequency dominance, and the no-model case (fields stay[]/None). Full suite green (510 passed).🤖 Generated with Claude Code