feat(cli): expose ACP session configuration - #4051
Conversation
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
0bca0ed to
dda8f4b
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
The +1606 is misleading in a good way: production is about 306 lines across three files, the rest is tests. And the important structural choices are right. It uses the SDK's own session.setConfigOption rather than inventing an extension method, declines session/set_mode explicitly with a test asserting -32601, and lands on the Host's session.configuration.update with expectedRevision CAS instead of mirroring session state in the adapter. acp-child-process.test.ts is a real end-to-end path, actual subprocess, actual Host, session/new then set_config_option then read back. That is better evidence than most PRs bring.
Four things pull away from the existing seam, and they are the same fix repeated.
[P2] The thinking menu is a static list of eight, ignoring what the model declares
projectAcpSessionConfigOptions takes only the session, so THINKING_SPEC ships all eight levels regardless of model. The Host is fail-closed: session-catalog-coordinator.ts:881 rejects any level not in thinkingVariantsForConnection(...) with invalid_request, which this PR maps to invalidParams.
So the menu offers options that must fail. claude-sonnet-5 declares ['low','medium','high','xhigh','max'], so off and minimal always fail. deepseek-v4-flash declares ['low','high','max'], so four of eight fail. A non-reasoning model returns [], where the UI is supposed to hide the switch entirely and here seven of eight fail.
model-thinking.ts:335 says it is "the single place that decides which models expose the knob", and the same package already does this correctly: pi-tui-runner.ts:1270 reads currentModelChoice()?.thinkingLevels ?? [] and refuses rather than offering a menu when it is empty. The per-model list is already in the connection catalog projection, so no protocol change is needed. If deriving it is out of scope for this slice, not shipping thinking_level yet beats shipping a menu where most entries error.
[P2] explore is offered as a user-selectable permission mode
PERMISSION_SPEC.options includes ['explore', 'Explore']. settings.ts:486 is unusually direct about this:
exploreis excluded, it's reserved for Deep Research sessions and Bot-incoming guards and is never a mode the user picks, in the composer dropdown or here. Derived from the canonical PERMISSION_MODES (not a hand-copied literal) so adding a future mode updates every consumer in one place.
That comment names both of the things this PR does. CHAT_DEFAULT_PERMISSION_MODES is the list every existing picker uses via PERMISSION_MODE_ORDER. Note currentValue still needs to be able to say explore, since a Deep Research session projects as that.
[P2] The four enums are hand-copied, so the adapter becomes a stricter second gate
PERMISSION_SPEC, THINKING_SPEC, COLLABORATION_SPEC and ORCHESTRATION_SPEC each restate values that live in @maka/core (permission.ts:26, orchestration.ts:20, collaboration.ts:20, model-thinking.ts:49). Add a value to any of them later and ACP will not only miss the new option, validateAcpSessionConfigOptionRequest will reject it as unsupported even though the Host accepts it. Deriving the options and keeping only a Record<Value, DisplayName> makes a missed value a compile error, and is the same edit as the two findings above.
[P2] Third copy of the CAS retry loop in one package
#setConfigOption's retry loop, #getConfigurableSession, requireConfigurableSession and ACP_SESSION_CONFIGURATION_MAX_ATTEMPTS = 3 are line-for-line siblings of updateRuntimeHostSession, getRuntimeHostSession, requireSession and MAX_CATALOG_ATTEMPTS = 3 in runtime-host-session-driver.ts:1767, same package. Exporting those and calling them leaves #setConfigOption with only the ACP-specific parts, and is a net deletion.
Smaller notes: session_busy and operation_conflict map to internalError where invalidParams fits, both are client-correctable. And #ownedSessionIds looks redundant against the Host authority, but it actually blocks something real, session-manager.ts:1117 silently strips the Deep Research label when a Deep Research session's permission mode moves off explore. One comment saying what it guards would stop it being deleted as redundant state later.
Next step
The four P2s converge into one edit and production comes out smaller. Two test cases that assert eight thinking levels and three permission modes will need to change with them.
Then manual acceptance with a real ACP client against maka --acp, since this is user-visible and acp-child-process.test.ts only exercises collaboration_mode: after session/new, check that each dropdown's options match what the session's model and the product actually allow, and set each one once. That is exactly where the first two findings live.
One scheduling note, not a defect: session/prompt is not registered yet, so a client can create and configure a session but cannot drive a turn. Fine as a slice of #3132, worth being deliberate about in merge order.
Evidence boundary: read the full production diff, all four acp/ files at head, the Host coordinator's configuration path, the core enum modules, and the SDK 1.4.0 types. I did not read the 881-line registry test body beyond its case titles, and ran no build, test or typecheck.
AI-assisted review: drafted with Maka.
|
Thanks for the review. I pushed
I kept three boundaries intentionally:
Verification after the change: full CLI suite 772 tests / 769 passed / 3 skipped / 0 failed, plus repository build, typecheck, lint, and format checks all passing. |
Summary
configOptionsfrom the Runtime Host's committed Session projection for every successfulsession/new: permission mode, collaboration mode, orchestration mode, and thinking level only when the selected model advertises supported thinking levels.permission_modeadvertisesaskandbypass; a pre-existingexploreSession may still reportexploreas its current value, but ACP cannot select it.thinking_level; reasoning models advertisedefaultplus exactly their declared levels.session/listnever grants ownership.session/set_config_optionthrough the official SDK route, validating ownership, id, type, and value before Host I/O.SessionConfigurationPatchCAS update, preserving model selection and unrelated fields, with at most three attempts on revision conflicts.RequestErrormapping at the ACP boundary.session/set_modeunsupported and unadvertised.Refs #3132
Depends on #3744 (merged)
Review dependency
mainat1581dc1c4after feat(cli): add ACP session creation and listing #3744 merged, so this PR contains only the PR3 delta.aee2b5725: canonical/model-aware options, non-selectableexplore, neutral shared CAS logic, and Host-backed subprocess coverage.Error and acceptance boundaries
session_busyandoperation_conflictremain internal ACP errors with the concrete Host code preserved inerror.data.code; they are not classified as JSON-RPCinvalidParamsbecause unchanged parameters may succeed after the conflicting state clears.Verification
npm run buildTMPDIR=/private/tmp npm --workspace maka-agent run test:dist— 772 tests: 769 passed, 3 skipped, 0 failed.npm run typechecknpm run lintnpm run format:checknpm run check:asf-headers— submitted source files pass; the local checkout also contains unrelated untracked planning documents outside this PR.git diff --checkAI use
Select exactly one:
Tool(s) and scope: Codex assisted with design, implementation, tests, verification, checklist alignment, review follow-up, and documentation updates.
Checklist
Does this PR entail a change in behavior?