Files
kandev/apps/backend/internal/agentctl
José Almeida 7115ee08d5 fix(acp): stop inventing currentModelId from AvailableModels[0] (#947)
* fix(acp): stop inventing currentModelId from AvailableModels[0]

When session/new returned an empty currentModelId, the adapter fell back
to AvailableModels[0]. Auggie returns an alphabetically-sorted list whose
[0] is a pseudo-agent ("Build Analyzer"), so the UI showed the wrong
model on every fresh load and session resume.

Drop that fallback (the configOption-based fallback stays — that one is
legitimate). When neither CurrentModelId nor a model-shaped configOption
provides a value, emit empty and let the frontend resolve to the profile
or snapshot model.

Also emit a session_models convergence event after SetModel succeeds,
so the UI updates after the lifecycle manager applies the profile model
at session init and after user-driven /session/set_model calls. Cache
configOptions alongside availableModels for inclusion in that event.

* fix(acp): tighten SetModel convergence event semantics

Address PR #947 review feedback:

- Bind the convergence event to the session captured under the same
  RLock used to read the connection. `SetModel` now snapshots
  `sessionID`, `available`, and `cachedConfig` together and passes
  them into `emitSetModelEvent`, so a concurrent session switch can
  no longer route the event to the wrong target.

- Rewrite the model-shaped configOption's `CurrentValue` to the new
  `modelID` before emitting. Without this the event carried
  `CurrentModelID = <new>` alongside `ConfigOptions[model].CurrentValue
  = <old>` — a latent inconsistency for codex-style consumers that
  read the option value. The cached slice is copied first so the
  caller's snapshot is not mutated.

- Refresh `availableConfigOptions` under `Lock` from inbound
  `ConfigOptionUpdate` notifications, so a subsequent `SetModel`
  convergence event uses the latest options instead of the
  session/new snapshot.

- Log the convergence event with `session_id` and `model_id`, matching
  the logging already on `emitSessionModels`.

- Extract a `configOptionIDModel` constant for the well-known "model"
  ID (silences goconst on the new helper).

Tests: extend `TestEmitSetModelEvent_EmitsSessionModelsWithCachedState`
to assert the `CurrentValue` rewrite and the caller-slice
no-mutation guarantee; add `TestConfigOptionUpdate_RefreshesCachedConfig`
to pin the cache refresh.

* test(acp): tighten ConfigOptionUpdate cache-refresh assertions

Address two minor follow-up suggestions from Claude on PR #947:

- Add a comment to emitSetModelEvent noting that the cachedConfig
  copy is shallow (Options shares the caller's backing array) — safe
  today because only CurrentValue is mutated, but worth flagging in
  case a future caller mutates Options in place.

- Have TestConfigOptionUpdate_RefreshesCachedConfig assert that the
  returned event's CurrentModelID resolves to the fresh value from
  the refreshed configOption, so the test pins both the cache update
  and the event payload in one place.

* test(acp): cover Category-based model option rewrite path

Address Claude's follow-up suggestion on PR #947: the previous test
only exercised the ID == "model" branch of emitSetModelEvent's
CurrentValue rewrite, so an accidental drop of the
Category == "model" branch would have passed. Add a third entry
with Category: "model" (and a non-matching ID) and assert both
matched options are rewritten while the unmatched one is untouched.
2026-05-19 14:18:51 +01:00
..