Skip to content

feat(cli): warn when the agent's run config differs from the request - #324

Closed
zxch3n wants to merge 1 commit into
fix/acp-per-model-config-option-validationfrom
feat/acp-run-config-divergence-warning
Closed

feat(cli): warn when the agent's run config differs from the request#324
zxch3n wants to merge 1 commit into
fix/acp-per-model-config-option-validationfrom
feat/acp-run-config-divergence-warning

Conversation

@zxch3n

@zxch3n zxch3n commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Stacked on #316 — review that one first; the base flips to main once it merges.

Problem

applyAcpSessionRunConfig decided a user-visible agent_warning from a REJECTION, then suppressed it for exactly the four controls users most need told about — model, reasoning effort, Fast, Plan — on Codex and Claude (suppressKnownRunConfigWarnings && isKnownRunConfigOption(...)). The suppression existed because those calls fail routinely when the turn config is re-sent every turn.

A rejection is the wrong signal, in both directions:

  • False negative. Codex applyFastModeChange sets fastModeEnabled unconditionally — it ACCEPTS fast-mode on a model with no fast speed tier. Nothing throws; createSessionConfigOptions then omits the option and resolveFastServiceTier returns null, so the turn runs at normal speed and the user is told nothing. Even without the suppression, no rejection ever existed to report.
  • False positive. A rejected value that was already effective changed nothing, and was reported.

Fix

Key the notice off divergence: after applying the turn's config, compare each requested selection against the state the agent itself publishes (runtimeConfigPatch, already computed from agentClient.getConfigOptions()), and warn only where they disagree.

  • An on/off select and a boolean toggle express the same choice, so neither shape alone counts as a divergence.
  • An explicit config.modeId/modelId outranks a duplicate config-option entry; losing that precedence contest is not the agent disagreeing, so only the winner is judged.
  • Where the agent published no config options at all — or for a sensitive id, which the runtime state deliberately omits — the failed call stays the only signal and is still used.
  • rejectedSelections (debug diagnostics) is unchanged.

The notice text now says the agent did not apply part of the config rather than rejected it, which is what the new signal actually means.

Noise is bounded: the composer re-sends the state the agent reported, so an ordinary turn diverges nowhere, and recordAgentWarning already deduplicates by message per session — a stuck divergence costs one notice, not one per turn.

Invariant change

apps/cli/AGENTS.md documented the old behavior ("Codex/Claude mismatches for model, reasoning effort, Fast, or Plan are not promoted to visible agent_warning notices"). That paragraph is rewritten to state the divergence rule, why a rejection is not it, and to say plainly that a per-agent suppression list must not come back. The Claude Fable fast=false no-op exception is preserved — it is skipped before dispatch, so it is judged for neither.

Tests

apps/cli/src/session/acp-session-config-applier.test.ts — the two tests that encoded the suppression are replaced by ones for the new rule: a selection accepted but dropped from the agent's state (the Codex Fast shape, with empty rejectedSelections), a rejected selection that was already effective (silent), and the select/boolean equivalence. The four tests covering redaction, non-publishing agents, and other agent types are unchanged and still pass.

pnpm --filter lody test (2467), typecheck, lint all pass.

🤖 Generated with Claude Code

The applier suppressed user-visible warnings for exactly the four controls
users most need told about — model, reasoning effort, Fast, Plan — on Codex
and Claude, because it keyed the notice off a REJECTION and those calls fail
routinely when re-sent per turn. But a rejection is the wrong signal in both
directions. Codex ACCEPTS `fast-mode` on a model with no fast speed tier and
then omits the option from the state it publishes: nothing throws, the turn
runs at normal speed, and the user is told nothing. Meanwhile a rejected
value that was already effective changed nothing and was reported.

Key the notice off divergence instead: after applying the turn's config,
compare each requested selection against the state the agent itself
publishes, and warn only where they disagree. An `on`/`off` select and a
boolean toggle are the same choice, so neither shape alone is a divergence.
Where the agent published no config options at all — or for a sensitive id,
which the runtime state deliberately omits — the failed call remains the only
signal, so it is still used there.

Rejections keep going to debug diagnostics unchanged, and `agent_warning`
notices are already deduplicated by message per session, so a stuck
divergence costs one notice rather than one per turn.

Model: claude-opus-5[1m]

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 484d31ea8e

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +299 to +303
selection.source.kind === 'mode'
? runtimeConfigPatch.modeId
: selection.source.kind === 'model'
? runtimeConfigPatch.modelId
: effectiveConfigOptionValues[selection.source.configId];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Compare mode and model against the agent-published state

When a config-option-backed request resolves but the response retains a different mode or omits the requested model, AgentClient has already stored that authoritative snapshot, but confirmedLegacyModeId overwrites its mode and confirmedLegacyModelId fills its missing model before this comparison. The comparison therefore sees the requested values rather than the agent's answer and emits no agent_warning, silently defeating the new divergence warning for permission modes and models; apply the fallback only for actual legacy protocol paths or compare against the unmodified snapshot.

AGENTS.md reference: apps/cli/AGENTS.md:L277-L285

Useful? React with 👍 / 👎.

@zxch3n

zxch3n commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #333, which contains these commits and supersedes their mechanism: the capability snapshot no longer rejects a run config at all, so the per-model exemption set this PR introduced has been deleted rather than maintained. Consolidating review on #333.

@zxch3n zxch3n closed this Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant