feat(acp): pi-ACP model picker and model-aware thinking levels#340
Open
julianromli wants to merge 1 commit into
Open
feat(acp): pi-ACP model picker and model-aware thinking levels#340julianromli wants to merge 1 commit into
julianromli wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds per-session model selection to Termul's ACP agent chat. The backend gains ChangesACP Session Model and Thinking
Sequence Diagram(s)sequenceDiagram
participant UI as Chat UI
participant AcpStore
participant acpApi as ACP API
participant TauriCmd as Tauri Command
participant DriverThread as Driver Thread
UI->>AcpStore: setSessionModel(sessionId, modelId)
AcpStore->>AcpStore: update session.models.currentModelId
AcpStore->>AcpStore: resolveSessionModes(modes, models)
AcpStore->>acpApi: setSessionModel(agentId, sessionId, modelId)
acpApi->>TauriCmd: invoke acp_set_session_model
TauriCmd->>DriverThread: SetSessionModel command
DriverThread-->>TauriCmd: Ok(())
alt effective mode changed after model switch
AcpStore->>acpApi: setMode(newMode)
end
AcpStore-->>UI: session state updated
Note over UI,AcpStore: Incoming thought chunks filtered<br/>by isThinkingVisible(modes, models, configOptions)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
3 tasks
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src-tauri/src/acp/events.rs (1)
253-266: ⚡ Quick winAdd assertion for
modelsfield omission in serialization test.The test
session_created_omits_none_fieldsverifies thatmodesandconfigOptionsare omitted from JSON whenNone, but it does not assert the same for the newmodelsfield. For completeness, add:assert!(value.get("modes").is_none()); + assert!(value.get("models").is_none()); assert!(value.get("configOptions").is_none());🤖 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-tauri/src/acp/events.rs` around lines 253 - 266, The test session_created_omits_none_fields in SessionCreatedEvent serialization is missing an assertion that the new optional models field is omitted when None; update the test by adding an assertion similar to the existing ones (e.g., assert!(value.get("models").is_none())) after the other omission checks so SessionCreatedEvent (constructed with models: None) verifies models is not present in the serialized JSON.src/renderer/components/chat/slash-menu-model.ts (1)
98-105: ⚡ Quick winUpdate the doc comment to reflect the actual section ordering.
The comment describes the order as "Commands first, then each config option..." but the implementation renders sections in this order: Skills → Commands → Models → Config/Modes. The comment should mention both Skills (which come before Commands) and the new Models section (which comes between Commands and Config).
📝 Suggested doc comment update
/** * Build ordered menu sections from the active session's ACP state. * - * Order: Commands first, then each config option as its own section (preserving + * Order: Skills first (if present), then Commands, then Models (unstable + * session model API, if available), then each config option as its own section (preserving * the agent's array order). When `configOptions` is non-empty, the legacy * `modes` section is omitted entirely (precedence). When it is empty, a single * legacy Modes section is emitted if modes exist. */🤖 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/components/chat/slash-menu-model.ts` around lines 98 - 105, Update the doc comment that currently describes section ordering: change it to state the actual order is Skills → Commands → Models → Config (or Modes), noting that Skills appear before Commands and the new Models section sits between Commands and Config; preserve the existing rule that when configOptions is non-empty the legacy Modes section is omitted and when empty a single Modes section is emitted. Ensure the comment above the menu-section builder mentions Skills and Models explicitly and keeps the note about preserving the agent's array order for config options.
🤖 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/assets/agent-icons/acp/agents.json`:
- Around line 650-659: The npx package string for the pi ACP agent ("id":
"pi-acp") is set to an unavailable version (distribution.npx.package =
"pi-acp@0.0.27-termul.1") which acp-registry.ts will invoke as `npx -y ...` and
fail; fix by updating distribution.npx.package to a resolvable npm version
(e.g., "pi-acp@0.0.27") or change the distribution type away from npx (use a
uvx/binary archive URL) or publish the missing "0.0.27-termul.1" tag to npm so
acp-registry.ts can successfully launch the agent.
In `@vendor/pi-acp-termul/README.md`:
- Line 154: Typo in the README: the entry for `/follow-up` currently reads
"`/follow-up` - pats to `pi` Follow-up Mode, get/set"; update that phrase to
"`/follow-up` - maps to `pi` Follow-up Mode, get/set" so the documentation
correctly says "maps to" and references the `/follow-up` command and the `pi`
Follow-up Mode.
- Line 201: Update the README text that currently reads "ACP clients don't yet
suport session history, but ACP sessions from `pi-acp` can be `/resume`d in pi
directly" by correcting the misspelling "suport" to "support" so the sentence
reads "ACP clients don't yet support session history, but ACP sessions from
`pi-acp` can be `/resume`d in pi directly"; locate the exact string in the
README and replace the single word to fix the typo.
---
Nitpick comments:
In `@src-tauri/src/acp/events.rs`:
- Around line 253-266: The test session_created_omits_none_fields in
SessionCreatedEvent serialization is missing an assertion that the new optional
models field is omitted when None; update the test by adding an assertion
similar to the existing ones (e.g., assert!(value.get("models").is_none()))
after the other omission checks so SessionCreatedEvent (constructed with models:
None) verifies models is not present in the serialized JSON.
In `@src/renderer/components/chat/slash-menu-model.ts`:
- Around line 98-105: Update the doc comment that currently describes section
ordering: change it to state the actual order is Skills → Commands → Models →
Config (or Modes), noting that Skills appear before Commands and the new Models
section sits between Commands and Config; preserve the existing rule that when
configOptions is non-empty the legacy Modes section is omitted and when empty a
single Modes section is emitted. Ensure the comment above the menu-section
builder mentions Skills and Models explicitly and keeps the note about
preserving the agent's array order for config options.
🪄 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: b0625827-02ba-4562-a99e-8fa1dec5cd6c
⛔ Files ignored due to path filters (3)
bun.lockis excluded by!**/*.lockvendor/pi-acp-termul/dist/index.jsis excluded by!**/dist/**vendor/pi-acp-termul/dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (27)
.gitignorepackage.jsonplans/ux-recommendation-spec.mdsrc-tauri/Cargo.tomlsrc-tauri/src/acp/commands.rssrc-tauri/src/acp/events.rssrc-tauri/src/acp/manager.rssrc-tauri/src/lib.rssrc/renderer/assets/agent-icons/acp/agents.jsonsrc/renderer/components/chat/AgentChatPanel.tsxsrc/renderer/components/chat/AgentHeader.tsxsrc/renderer/components/chat/ChatInputBar.tsxsrc/renderer/components/chat/SlashCommandMenu.tsxsrc/renderer/components/chat/chat-timeline.test.tssrc/renderer/components/chat/chat-timeline.tssrc/renderer/components/chat/slash-menu-model.test.tssrc/renderer/components/chat/slash-menu-model.tssrc/renderer/lib/acp-api.test.tssrc/renderer/lib/acp-api.tssrc/renderer/lib/acp-thinking.test.tssrc/renderer/lib/acp-thinking.tssrc/renderer/stores/acp-store.test.tssrc/renderer/stores/acp-store.tsvendor/pi-acp-termul/LICENSEvendor/pi-acp-termul/PATCHES.mdvendor/pi-acp-termul/README.mdvendor/pi-acp-termul/package.json
👮 Files not reviewed due to content moderation or server errors (7)
- src-tauri/src/acp/manager.rs
- src/renderer/lib/acp-api.ts
- src/renderer/lib/acp-api.test.ts
- src/renderer/lib/acp-thinking.ts
- src/renderer/lib/acp-thinking.test.ts
- src/renderer/stores/acp-store.ts
- src/renderer/stores/acp-store.test.ts
Owner
|
Hey @julianromli — triage update 🔁 Still a draft. Currently blocked by: merge conflicts with When you're ready to move this forward: rebase onto |
Add unstable session/set_model support, ModelChip UI, and client-side thinking resolution keyed off pi model capabilities. Vendor pi-acp 0.0.27-termul.1 with upstream patches until svkozak/pi-acp merges them. Hide reasoning blocks when thinking is off; auto-clamp level on model change.
julianromli
force-pushed
the
feat/pi-acp-thinking-model-refine
branch
from
d66ffab to
db6433f
Compare
julianromli
commented
Jun 15, 2026
julianromli
left a comment
julianromli
left a comment
Contributor
Author
There was a problem hiding this comment.
Resolved all open threads in db6433f (rebased onto dev, vendored fork removed).
- Critical (agents.json npx): reverted to
pi-acp@0.0.27. The vendored fork is removed entirely; the client-side resolver inacp-thinking.tsnow serves as defense-in-depth and returnsnull(trust agent modes) when model metadata is absent, so it degrades gracefully with stock pi-acp until upstream #51 ships. - README typos (
pats to,suport): the vendoredvendor/pi-acp-termul/README.mdis deleted along with the fork, so these no longer apply. - events.rs test: added
assert!(value.get("models").is_none());tosession_created_omits_none_fields. - slash-menu-model.ts doc comment: updated to reflect actual order (Skills -> Commands -> Models -> Config).
Local checks: bun run ci pass, bun run typecheck pass, bun run test pass (1704 tests), cargo test --lib acp pass (37 passed). Pre-existing git_bash_paths clippy errors on upstream/dev are untouched (outside this diff).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@plans/ux-recommendation-spec.md`:
- Around line 77-82: The fenced code blocks in the file are missing language
tags, which triggers markdownlint rule MD040. Add a language tag to all fenced
code blocks by changing the opening fence from triple backticks to triple
backticks followed by a language identifier. Use `text` as the language tag for
these examples (for example, change ``` to ```text). This fix needs to be
applied to the pi ACP — Setup example shown and to the similar code block
examples at the other locations mentioned in the same file.
🪄 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: 61dccb8e-fc60-45f3-af60-72adab2dc98d
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
plans/ux-recommendation-spec.mdsrc-tauri/Cargo.tomlsrc-tauri/src/acp/commands.rssrc-tauri/src/acp/events.rssrc-tauri/src/acp/manager.rssrc-tauri/src/lib.rssrc/renderer/components/chat/AgentChatPanel.tsxsrc/renderer/components/chat/AgentHeader.tsxsrc/renderer/components/chat/ChatInputBar.tsxsrc/renderer/components/chat/SlashCommandMenu.tsxsrc/renderer/components/chat/chat-timeline.test.tssrc/renderer/components/chat/chat-timeline.tssrc/renderer/components/chat/slash-menu-model.test.tssrc/renderer/components/chat/slash-menu-model.tssrc/renderer/lib/acp-api.test.tssrc/renderer/lib/acp-api.tssrc/renderer/lib/acp-thinking.test.tssrc/renderer/lib/acp-thinking.tssrc/renderer/stores/acp-store.test.tssrc/renderer/stores/acp-store.ts
✅ Files skipped from review due to trivial changes (1)
- src-tauri/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (16)
- src/renderer/components/chat/chat-timeline.test.ts
- src/renderer/components/chat/slash-menu-model.test.ts
- src/renderer/components/chat/SlashCommandMenu.tsx
- src/renderer/lib/acp-api.test.ts
- src-tauri/src/acp/events.rs
- src/renderer/components/chat/AgentChatPanel.tsx
- src-tauri/src/acp/commands.rs
- src/renderer/components/chat/slash-menu-model.ts
- src/renderer/components/chat/AgentHeader.tsx
- src/renderer/lib/acp-thinking.test.ts
- src/renderer/components/chat/ChatInputBar.tsx
- src/renderer/stores/acp-store.ts
- src-tauri/src/acp/manager.rs
- src/renderer/stores/acp-store.test.ts
- src/renderer/lib/acp-thinking.ts
- src/renderer/lib/acp-api.ts
Comment on lines
+77
to
+82
| ``` | ||
| pi ACP — Setup (2/3) | ||
| ☑ Enabled in settings | ||
| ☐ API key / login configured [Open setup guide] | ||
| ☐ Ready to chat [Test connection] | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language tag to these fenced examples.
These bare fences trigger markdownlint MD040. text is sufficient here and keeps the spec lint-clean.
Also applies to: 153-155, 178-180, 199-205
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@plans/ux-recommendation-spec.md` around lines 77 - 82, The fenced code blocks
in the file are missing language tags, which triggers markdownlint rule MD040.
Add a language tag to all fenced code blocks by changing the opening fence from
triple backticks to triple backticks followed by a language identifier. Use
`text` as the language tag for these examples (for example, change ``` to
```text). This fix needs to be applied to the pi ACP — Setup example shown and
to the similar code block examples at the other locations mentioned in the same
file.
Source: Linters/SAST tools
This was referenced Jun 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
session/set_modelend-to-end (Rust command, store, Model chip + slash menu).acp-thinking.ts) that filters levels from modelreasoning/thinkingLevelMap, with safe fallback to the agent's advertised modes when metadata is absent.off; auto-clamp thinking level when the model changes.Related Issue
Relates to #339
Upstream pi-acp refinement tracked in:
Type of Change
What Changed
Backend
unstable_session_modelonagent-client-protocol.acp_set_session_modelcommand; session responses includemodes,models,configOptions.SessionCreatedEventincludesmodels; serialization test assertsmodelsis omitted whenNone.Frontend
ModelChipin agent header;/modelslash commands.acp-thinking.tsfilters levels from modelreasoning/thinkingLevelMap; clamps on model change; returnsnull(trust agent modes) when model metadata is unavailable.How It Was Tested
bun run ci(biome)bun run typecheckbun run test(150 files, 1704 tests)cargo test --lib acp(37 passed, 1 ignored)Checklist
Summary by CodeRabbit
Release Notes