Repository navigation
feat(chat): skill-defined model lock (P1b-2, precedence layer 1) - #17
Conversation
A Persona can LINK skills (Persona.skills). If a linked skill declares a requiredModel (model:/provider: frontmatter, or authored in the editor), selecting that persona PINS the composer model/provider and DISABLES the picker — precedence layer 1, above the soft default (layer 2) and user pick (layer 3). - types: SkillDefinition.requiredModel/requiredProvider; Persona.skills; PersonaModelSeed.locked/reason (fills the reserved P1b-2 seam). - resolveSkillModelLock() runs above resolvePersonaModelSeed; first linked skill (by id OR name) with a requiredModel wins. - ChatTile: computes modelLock from active persona + workspace skills, short-circuits the layer-2 seed in onSelectAgent, pins live state via a lock-only effect, threads modelLocked/lockReason to the composer pills. - SkillEditor: Required Model/Provider fields; AgentEditor: linked-skills picker (stores names; flags skills that lock a model). - Discovery parses model:/provider: frontmatter into requiredModel/Provider. Model is NOT a security boundary: renderer model-resolution + composer disablement only. resolveAuthoritativeAgentMode and the daemon tools/ permission path are untouched. Built-ins carry NO skills, so the shared<->daemon DEFAULT_PERSONAS drift guard stays green.
|
Warning Review limit reached
More reviews will be available in 27 minutes and 23 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIntroduces a hard-lock precedence layer for persona-based model/provider selection. ChangesSkill-Defined Model Lock
Sequence Diagram(s)sequenceDiagram
actor User
participant ChatTile
participant resolveSkillModelLock
participant resolvePersonaModelSeed
participant ChatTileComposer
rect rgba(100, 149, 237, 0.5)
note over ChatTile: Agent selection
User->>ChatTile: onSelectAgent(nextPersona)
ChatTile->>resolveSkillModelLock: resolve(nextPersona, workspaceSkills)
alt skill hard lock found
resolveSkillModelLock-->>ChatTile: PersonaModelSeed { locked: true, model, provider, reason }
ChatTile->>ChatTile: set provider/model from lock
else no lock
resolveSkillModelLock-->>ChatTile: null
ChatTile->>resolvePersonaModelSeed: resolve(nextPersona)
resolvePersonaModelSeed-->>ChatTile: soft seed
ChatTile->>ChatTile: set provider/model from soft seed
end
end
rect rgba(144, 238, 144, 0.5)
note over ChatTile,ChatTileComposer: Render
ChatTile->>ChatTileComposer: modelLocked=true, lockReason="Locked by skill X"
ChatTileComposer->>ChatTileComposer: disable provider pill, tooltip=lockReason
ChatTileComposer->>ChatTileComposer: disable model pill, tooltip=lockReason
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/renderer/src/hooks/personaModelBinding.ts (1)
9-12: ⚡ Quick winUpdate the stale precedence note to reflect that layer 1 is now implemented.
The header still says skill-lock resolution is not implemented, but this file now exports
resolveSkillModelLock. Keeping this stale note will mislead future edits.Suggested doc-only patch
-// 1) [P1b-2, NOT IMPLEMENTED] skill-required model = HARD lock. SEAM: a future -// resolver runs ABOVE this, short-circuits, and returns a locked result the -// composer disables. `locked` is reserved on PersonaModelSeed for that. +// 1) [P1b-2] skill-required model = HARD lock via resolveSkillModelLock(). +// It runs ABOVE this soft resolver, short-circuits, and returns a locked +// result that disables the composer picker.🤖 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 `@src/renderer/src/hooks/personaModelBinding.ts` around lines 9 - 12, Update the stale comment block in the personaModelBinding.ts file (around lines 9-12) that marks skill-required model lock resolution (P1b-2) as "NOT IMPLEMENTED". Since this file now exports the resolveSkillModelLock function, remove the "NOT IMPLEMENTED" designation from the comment and update it to accurately reflect that this layer is now implemented. Preserve the technical description of what the resolver does but ensure the status reflects the current state of the codebase.
🤖 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 `@src/renderer/src/components/CustomisationTile.tsx`:
- Around line 801-803: The Input field for requiredProvider (around line 802)
allows saving a provider value even when requiredModel is empty, which creates
an inert configuration since lock resolution is model-gated. Modify the onChange
handler for the requiredProvider Input field to only update the requiredProvider
value if requiredModel has a non-empty value, preventing users from persisting a
provider-only setting that won't actually function.
---
Nitpick comments:
In `@src/renderer/src/hooks/personaModelBinding.ts`:
- Around line 9-12: Update the stale comment block in the personaModelBinding.ts
file (around lines 9-12) that marks skill-required model lock resolution (P1b-2)
as "NOT IMPLEMENTED". Since this file now exports the resolveSkillModelLock
function, remove the "NOT IMPLEMENTED" designation from the comment and update
it to accurately reflect that this layer is now implemented. Preserve the
technical description of what the resolver does but ensure the status reflects
the current state of the codebase.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc097517-5e9c-4204-a494-1c6ee085399b
📒 Files selected for processing (7)
src/renderer/src/components/ChatTile.tsxsrc/renderer/src/components/CustomisationTile.tsxsrc/renderer/src/components/chat/ChatTileComposer.tsxsrc/renderer/src/hooks/personaModelBinding.tssrc/renderer/src/hooks/useChatTileWorkspaceSkills.tssrc/shared/types.tstest/daemon/persona-model-binding.test.mjs
| <Field label="Required Provider"> | ||
| <Input value={draft.requiredProvider ?? ''} onChange={v => up({ requiredProvider: v.trim() || undefined })} placeholder="optional — e.g. claude, codex (pins alongside the model)" /> | ||
| </Field> |
There was a problem hiding this comment.
Prevent provider-only lock settings that silently do nothing.
Line [802] currently allows saving requiredProvider when requiredModel is empty. Since lock resolution is model-gated, this persists an inert config and can mislead users.
Suggested fix
<Field label="Required Model">
- <Input value={draft.requiredModel ?? ''} onChange={v => up({ requiredModel: v.trim() || undefined })} placeholder="e.g. claude-opus-4-8 — pins the composer model when a persona links this skill" />
+ <Input
+ value={draft.requiredModel ?? ''}
+ onChange={v => {
+ const nextModel = v.trim() || undefined
+ up({
+ requiredModel: nextModel,
+ requiredProvider: nextModel ? draft.requiredProvider : undefined,
+ })
+ }}
+ placeholder="e.g. claude-opus-4-8 — pins the composer model when a persona links this skill"
+ />
</Field>
<Field label="Required Provider">
- <Input value={draft.requiredProvider ?? ''} onChange={v => up({ requiredProvider: v.trim() || undefined })} placeholder="optional — e.g. claude, codex (pins alongside the model)" />
+ <Input
+ value={draft.requiredProvider ?? ''}
+ onChange={v => up({ requiredProvider: draft.requiredModel ? (v.trim() || undefined) : undefined })}
+ placeholder="optional — e.g. claude, codex (pins alongside the model)"
+ />
</Field>🤖 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 `@src/renderer/src/components/CustomisationTile.tsx` around lines 801 - 803,
The Input field for requiredProvider (around line 802) allows saving a provider
value even when requiredModel is empty, which creates an inert configuration
since lock resolution is model-gated. Modify the onChange handler for the
requiredProvider Input field to only update the requiredProvider value if
requiredModel has a non-empty value, preventing users from persisting a
provider-only setting that won't actually function.
There was a problem hiding this comment.
Pull request overview
This PR adds precedence layer 1 “skill-defined model lock” to the chat composer: when a Persona links a workspace skill that declares a requiredModel (and optionally requiredProvider), selecting that Persona pins the composer’s model/provider and disables the model/provider pickers, taking precedence over the existing persona soft default (layer 2) and user selection (layer 3).
Changes:
- Extends shared types to support
SkillDefinition.requiredModel/requiredProviderandPersona.skills, and addslocked/reasonto the seed shape used by composer binding. - Implements
resolveSkillModelLock(...)and wires it intoChatTileselection + a lock-only effect to keep live state pinned across async skill loading / restored personas. - Adds UI for authoring required model/provider on skills and linking skills to personas, plus discovery parsing of
model:/provider:from bounded leading frontmatter.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/daemon/persona-model-binding.test.mjs | Adds test coverage for layer-1 lock precedence, discovery parsing behavior, UI wiring checks, and altitude assertions. |
| src/shared/types.ts | Introduces requiredModel/requiredProvider on skills and skills on personas with detailed docstrings. |
| src/renderer/src/hooks/useChatTileWorkspaceSkills.ts | Parses model: / provider: from bounded frontmatter and attaches lock metadata to discovered skills. |
| src/renderer/src/hooks/personaModelBinding.ts | Adds resolveSkillModelLock above the existing soft seed resolver and expands PersonaModelSeed with lock metadata. |
| src/renderer/src/components/CustomisationTile.tsx | Adds Required Model/Provider fields to SkillEditor and a linked-skills picker to AgentEditor. |
| src/renderer/src/components/ChatTile.tsx | Computes and applies the model lock (including short-circuiting the soft seed) and threads lock state to the composer. |
| src/renderer/src/components/chat/ChatTileComposer.tsx | Disables provider/model pills when locked and surfaces lock reason via tooltip. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
What
Implements P1b-2: skill-defined model lock (precedence layer 1). A Persona can now LINK skills; if a linked skill declares a required model, selecting that Persona pins the composer model/provider and disables the picker — sitting above the P1b-1 soft default (layer 2) and the user pick (layer 3).
How (the precedence ladder)
resolveSkillModelLock(persona, workspaceSkills)runs ABOVEresolvePersonaModelSeed. The first linked skill (matched byidORname) that declares arequiredModelwins; its model (+requiredProvider, if set) pins the composer.defaultBindingseeds the composer (P1b-1).Where layer 1 short-circuits layer 2:
ChatTile.tsxonSelectAgent— whenresolveSkillModelLock(...)returns a lock it pins provider/model and takes theifbranch; the soft seed (resolvePersonaModelSeed) only runs in theelse. A lock-onlyuseEffectre-pins the live state for async skill-load / restored-agentIdpaths where the click handler never fires.Changes
SkillDefinition.requiredModel/requiredProvider;Persona.skills?: string[];PersonaModelSeed.locked?/reason?(fills the reserved P1b-2 seam).resolveSkillModelLockinpersonaModelBinding.ts, above the soft resolver.model:/provider:parsed from the bounded leading frontmatter block (line-anchored — body prose like "the best model: fast" can't trip a spurious lock).modelLock, threadsmodelLocked/lockReasonto the composer pills.ToolbarPills disable (disabled+title, Lock glyph) when locked.SkillEditorRequired Model/Provider fields;AgentEditorlinked-skills picker (stores names, flags skills that lock a model).Altitude / security
Model is not a security boundary — renderer model-resolution + composer disablement only.
resolveAuthoritativeAgentModeand the daemon tools/permission path are untouched (git diff --statshows nothing undersrc/main/chat/orpackages/codesurf-daemon/). An altitude test asserts the authoritative resolver references none ofrequiredModel|requiredProvider|resolveSkillModelLock|.skills. Built-ins carry no skills, so the shared↔daemonDEFAULT_PERSONASdrift guard stays green.Tests
npm run test:daemon→ 221 pass / 0 fail. New tests inpersona-model-binding.test.mjs: lock overrides soft default; id/name match; first-with-requiredModel wins; no-linked-skill → null; locked ⇒ pills disabled (composer wiring); onSelect short-circuits layer 2; overlay/extends carriesPersona.skills; built-ins carry no skills (drift guard); altitude; bounded-frontmatter parse (real + prose-false-positive cases). Touched renderer tests green (pre-existingchat-convention-promptsfailures confirmed on base, unrelated).npm run buildnot run, per instructions.Summary by CodeRabbit
requiredModeland/orrequiredProvidervia leading frontmatter (model:/provider:).