You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "2BAB (via GitHub)" <gi...@apache.org> on 2026/10/20 07:26:12 UTC
[PR] feat(acp): expose model selection as a session config option [maka]
2BAB opened a new pull request, #6189:
URL: https://github.com/apache/maka/pull/6189
ACP sessions can currently change permission mode, thinking level, collaboration mode and orchestration mode, but not the model — even though the runtime already accepts an explicit model target.
This adds the missing knob as a standard session config option:
- `configOptions` gains a `model` select, projected from the session's own connection catalogue (ids and display names come from the Host, not re-derived by the CLI).
- `session/set_config_option { configId: "model", value }` applies it as `{ modelTarget: { kind: "explicit", connectionId, connectionSlug, model } }`.
Two deliberate constraints:
1. **A locked connection projects only the model the session already has.** Listing the rest and then refusing every one of them would advertise choices the client cannot take.
2. **Validation is session-scoped.** A value the session's connection does not offer is rejected, and a request with no resolvable catalogue is refused rather than guessed — the same failure-surfacing rule the other options follow.
Tests cover the projection order and catalogue mapping, the locked projection, the patch shape, rejection of unoffered values and locked changes, and rejection of a model request with no catalogue context. `tsc --noEmit` is clean and the CLI test suite for this file passes.
No protocol changes: the option rides on the existing `session/set_config_option` surface.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(acp): expose model selection as a session config option [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #6189:
URL: https://github.com/apache/maka/pull/6189#discussion_r4323335250
##########
packages/cli/src/acp/session-configuration.ts:
##########
@@ -160,8 +246,21 @@ export function validateAcpSessionConfigOptionRequest(
export function createAcpSessionConfigPatch(
request: SetSessionConfigOptionRequest,
+ modelContext?: AcpSessionModelConfigContext,
): SessionConfigurationPatch {
- validateAcpSessionConfigOptionRequest(request);
+ validateAcpSessionConfigOptionRequest(request, modelContext);
+ if (request.configId === MODEL_CONFIG_ID) {
+ // Non-null: validation above rejects a model request without a context.
+ const context = modelContext as AcpSessionModelConfigContext;
+ return {
+ modelTarget: {
Review Comment:
P2: the patch keeps the session's current `thinkingLevel`. `#resolveModel` rejects a target model that doesn't support it, so listed models can be refused. The TUI's `setModel` sends `thinkingLevel: null` together with the target.
##########
packages/cli/src/acp/session-configuration.ts:
##########
@@ -35,6 +35,57 @@ interface AcpSessionConfigSpec {
readonly options: readonly (readonly [string, string])[];
}
+/** One model the session's connection offers, as the Host resolved it. */
+export interface AcpModelChoice {
+ readonly id: string;
+ readonly name?: string;
+}
+
+/** Everything a model-valued request needs, from the session and its catalogue. */
+export interface AcpSessionModelConfigContext {
+ readonly connectionId: string;
+ readonly connectionSlug: string;
+ readonly currentModel: string;
+ readonly connectionLocked: boolean;
+ readonly models: readonly AcpModelChoice[];
+}
+
+/**
+ * `model` is a category the ACP spec reserves for the model selector, alongside
+ * `mode`, `model_config` and `thought_level`
+ * (https://agentclientprotocol.com/protocol/v1/session-config-options#option-categories).
+ * Categories without a leading `_` belong to the spec, so this one is
+ * deliberately not namespaced like the `_maka/*` extensions below.
+ */
+const MODEL_CONFIG_ID = 'model';
+const MODEL_SPEC_NAME = 'Model';
+const MODEL_CATEGORY = 'model';
+
+/**
+ * The values a session may select. A locked connection projects exactly the
+ * model it already has: offering the rest and refusing every one of them would
+ * advertise choices the client cannot take.
+ */
+function modelValues(
+ models: readonly AcpModelChoice[],
+ currentModel: string,
+ locked: boolean,
+): string[] {
+ if (locked) return [currentModel];
Review Comment:
P2: `connectionLocked` becomes true at the first durable user message and on any `modelTarget` patch (session-catalog-coordinator.ts:1490), and the Host does not refuse a same-connection switch on a locked session. This branch leaves ACP clients with a one-entry picker after the first prompt or the first switch.
##########
packages/cli/src/acp/session-registry.ts:
##########
@@ -2504,7 +2515,45 @@ export class AcpSessionRegistry {
({ connectionId }) => connectionId === session.llmConnectionId,
);
const selectedModel = selectedConnection?.catalogEntries.find(({ id }) => id === session.model);
- return projectAcpSessionConfigOptions(session, selectedModel?.thinkingLevels ?? []);
+ // Only models the connection has enabled are offerable: an entry that is in
+ // the resolved catalogue but not in `enabledModelIds` is rejected by the
+ // Host when selected, so listing it would advertise a dead choice.
+ const enabled = new Set(selectedConnection?.enabledModelIds ?? []);
+ const offerable = (selectedConnection?.catalogEntries ?? []).filter(
+ ({ id }) => enabled.has(id) || id === session.model,
+ );
+ return projectAcpSessionConfigOptions(
+ session,
+ selectedModel?.thinkingLevels ?? [],
+ offerable.map(({ id, displayName }) => ({ id, name: displayName })),
+ );
+ }
+
+ /**
+ * The session-scoped facts a model selection needs: which connection it runs
+ * on, which models that connection offers, and whether the connection is
+ * locked to the current model. Undefined means "cannot validate" — the
+ * request is rejected rather than guessed.
+ */
+ async #modelConfigContext(
+ connection: AcpSessionRegistryConnection,
+ session: SessionCatalogProjection,
+ ): Promise<AcpSessionModelConfigContext | undefined> {
+ if (session.llmConnectionId === null) return undefined;
+ const catalog = await readRuntimeHostConnectionCatalog(connection);
+ const entry = catalog.connections.find(
+ ({ connectionId }) => connectionId === session.llmConnectionId,
+ );
+ return {
+ connectionId: session.llmConnectionId,
+ connectionSlug: session.llmConnectionSlug,
+ currentModel: session.model,
+ connectionLocked: session.connectionLocked,
+ models: (entry?.catalogEntries ?? []).map(({ id, displayName }) => ({
Review Comment:
P3: the projection filters by `enabledModelIds` but this validation context doesn't, so a disabled model passes here and is refused by the Host.
##########
packages/cli/src/acp/session-registry.ts:
##########
@@ -2430,12 +2436,17 @@ export class AcpSessionRegistry {
committed = await updateRuntimeHostSession(
connection,
params.sessionId,
- (current) =>
- connection.request('session.configuration.update', {
+ async (current) => {
+ const modelContext =
+ params.configId === 'model'
+ ? await this.#modelConfigContext(connection, current)
+ : undefined;
+ return connection.request('session.configuration.update', {
sessionId: params.sessionId,
expectedRevision: current.revision,
- patch: createAcpSessionConfigPatch(params),
- }),
+ patch: createAcpSessionConfigPatch(params, modelContext),
Review Comment:
P2: an `AcpSessionConfigInputError` thrown here reaches `requestErrorFromSessionUpdate` and is mapped to `internalError` (`internal_failure`, source runtime_host), not `invalid_params`. Map it with `requestErrorFromConfigInput`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(acp): expose model selection as a session config option [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #6189:
URL: https://github.com/apache/maka/pull/6189#discussion_r4326678306
##########
packages/cli/src/acp/session-configuration.ts:
##########
@@ -160,8 +259,25 @@ export function validateAcpSessionConfigOptionRequest(
export function createAcpSessionConfigPatch(
request: SetSessionConfigOptionRequest,
+ modelContext?: AcpSessionModelConfigContext,
): SessionConfigurationPatch {
- validateAcpSessionConfigOptionRequest(request);
+ validateAcpSessionConfigOptionRequest(request, modelContext);
+ if (request.configId === MODEL_CONFIG_ID) {
+ // Non-null: validation above rejects a model request without a context.
+ const context = modelContext as AcpSessionModelConfigContext;
+ return {
+ modelTarget: {
+ kind: 'explicit',
+ connectionId: context.connectionId,
+ connectionSlug: context.connectionSlug,
+ model: request.value,
+ },
+ // A thinking level belongs to the old model: the TUI's `setModel` sends
+ // the same `null` (runtime-host-session-driver.ts), and keeping it here
+ // makes the Host refuse any target model that does not support it.
+ thinkingLevel: null,
Review Comment:
P3: this also clears an explicit thinking level when the requested model is the current one (a client re-sending `currentValue`), so the session drops from e.g. `high` to default. Consider sending `thinkingLevel: null` only when `request.value !== context.currentModel`, or keeping the level when the target model supports it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org