fix(inline): Added dedicated inline chat model var

[cSlat3r]

Summary

Inline chat and the other Claude modes use different transports that do not accept the same model-id grammar. handle_inline_chat_request goes to the raw Anthropic SDK via ClaudeChatModel.messages.stream; every other mode hands chat_model to the Claude Code CLI subprocess. Both read the same claude_settings.chat_model, potential for conflict with CLI exclusive conventions. Additionally, they are different workloads - inline chat is one-shot code generation working with raw text, where a cheaper or faster model is often preferable to the one doing agentic work that returns thinking or tool blocks.

There is no config-side workaround, because one string has two consumers with potentially different requirements: rewriting it for the raw API changes what the CLI receives, and leaving it alone risks inline chat breaking.

Solution

Resolution. New optional claude_settings.inline_chat_model, resolved per request as inline_chat_model → chat_model → ClaudeChatModel's own default. Unset, behavior is byte-identical to today. Each candidate is stripped before the fallback so a blank-but-present value is treated as unset rather than shadowing chat_model, matching how '' means unset elsewhere in these settings.

The agentic path is deliberately untouched. _create_client_options still reads chat_model, so the CLI transport receives exactly what it does today. TestAgenticPathIgnoresInlineChatModel is the regression guard for that.

Wired for parity with chat_model: NBI_CLAUDE_INLINE_CHAT_MODEL in STRING_OVERRIDE_SPEC, an entry in CLAUDE_SETTINGS_OVERRIDES, the derived setting lock, the SettingLockName union and settingLocks getter, and the settings-panel round-trip. The two env vars are independent — pinning only NBI_CLAUDE_CHAT_MODEL still governs inline chat through the live fallback, so existing deployments keep their coverage.

Settings UI. A collapsible "Inline chat model" control nested under Claude → Chat model, collapsed by default and auto-expanded when a value is already set. The key has to be in the panel's payload regardless of the UI: that panel POSTs a closed object literal which replaces claude_settings wholesale, so a key absent from it is deleted the first time a user opens the Claude tab.

Testing

• tests/test_claude_client.py (+8): TestInlineChatModelSelection covers the fallback order, empty/absent/whitespace-only handling, and stripping; TestAgenticPathIgnoresInlineChatModel pins that _create_client_options still uses chat_model and that an empty chat_model still yields model=None.
• tests/test_feature_flags.py (+4): each env var writes only its own destination, in both directions.
• tests/test_cell_output_features_response.py (+1): the new lock name is surfaced.
• tsc --noEmit, eslint, prettier, stylelint clean; jest unchanged at 31 suites / 385 tests.
• Full Python suite matches main in the same environment. (Developed on Windows, where a set of pre-existing platform tests — path separators, os.chmod modes, SIGKILL — fail identically on a clean checkout.)
• Live-verified in JupyterLab: with the key absent, inline chat runs on chat_model unchanged; set to a deliberately invalid id, the error names that id, confirming the override won; and with the key set, opening and closing Settings → Claude leaves it intact in config.json — which fails on main.

Risks / follow-ups

• The new locked select omits the sr-only "Locked by your administrator" span that its neighbours have, to keep the added markup minimal.
• Pre-existing and not touched here: the settings panel deleting unknown claude_settings keys generally (this PR closes it only for the new key); _create_client_options raising on a JSON-null chat_model; has_claude_settings_change restarting the CLI subprocess on every settings POST; and inline-chat telemetry emitting NBIAPI.config.chatModel rather than the Claude setting.
• The new locked select omits the sr-only "Locked by your administrator" span that its neighbours have, to keep the added markup minimal. Happy to add it.
• Pre-existing and not touched here: the settings panel deleting unknown claude_settings keys generally (this PR closes it only for the new key); _create_client_options raising on a JSON-null chat_model; has_claude_settings_change restarting the CLI subprocess on every settings POST; and inline-chat telemetry emitting NBIAPI.config.chatModel rather than the Claude setting. Happy to file any of these separately.
• Pinning the chat model by env var and provisioning a different inline_chat_model in config.json is not expressible: the stored value is ignored while the pin is set, and blanked on the next settings save. An administrator needing two models sets both env vars.
• _inline_system_prompt_token_budget still resolves the context window from chat_model, while the request now goes to inline_chat_model. With both keys set to models of differing window sizes the rules budget can be over- or under-generous; unknown ids fall back to 200K, so the common case is unaffected and nothing errors on its own. Flagging it because the divergence is created by this change. Happy to follow up separately.

Evidence:

Expanded:

Collapsed:

[pjdoland]

Thank you for this. The problem statement is precise, the transport split is the real cause, and the write-up (including the disclosed risks) made the review considerably easier. I checked out 89106471 against merge-base fa6b175 and ran the full gate set from a clean worktree: 1501 passed in tests/ plus 76 in tests/test_claude_client.py, jest 31 suites / 385 tests, and tsc --noEmit, eslint, stylelint, and prettier --check all clean. The added tests are substantive rather than decorative, and TestAgenticPathIgnoresInlineChatModel pins the thing most worth pinning.

I have one blocker, and it is the item you already flagged under Risks. I think it lands harder than the write-up allows, so I want to lay out why.

NBI_CLAUDE_CHAT_MODEL stops governing inline chat on upgrade

README.md documents the value-presence locks as "setting the env var to a non-empty value pins the control to that value and disables it." Today NBI_CLAUDE_CHAT_MODEL genuinely does that for every Claude surface, because handle_inline_chat_request reads the same chat_model the CLI path reads. After this change it governs one of the two surfaces, and nothing tells the operator that.

Reproduced against the PR head:

from notebook_intelligence.feature_flags import apply_string_overrides, CLAUDE_SETTINGS_OVERRIDES

overrides = {"claude_chat_model": "claude-approved-1", "claude_inline_chat_model": ""}
stored = {"chat_model": "whatever", "inline_chat_model": "claude-opus-4-1-unapproved"}
resolved = apply_string_overrides(stored, overrides, CLAUDE_SETTINGS_OVERRIDES)

# agentic/CLI model : claude-approved-1
# inline chat model : claude-opus-4-1-unapproved

On fa6b175 the second line reads claude-approved-1. The escape needs no API call or hand-edited config.json: with only NBI_CLAUDE_CHAT_MODEL set, settingLocks.claude_inline_chat_model.locked is false, so settings-panel.tsx renders the new select enabled, nested directly beneath the Chat model select that is rendered disabled with the administrator tooltip. The dialog presents the bypass immediately under the lock it bypasses.

What makes this a blocker rather than a documentation gap is that it is silent and retroactive. An operator who pinned a model for cost control or for a gateway with an allowlisted model set gets the hole on upgrade, with no config change on their side and no warning.

On the stated alternative

The write-up rejects clamping on the grounds that it "forces the two transports back onto one value and reintroduces the failure this fixes." I would suggest the clamp does not have to be written that way. Rather than stamping the pinned chat id into inline_chat_model, force inline_chat_model to "" when NBI_CLAUDE_CHAT_MODEL is set and NBI_CLAUDE_INLINE_CHAT_MODEL is not:

• The resolution chain you built is untouched; "" falls through to chat_model, so behavior is byte-identical to fa6b175 for that deployment. No regression to reintroduce.
• The lock keeps meaning what README.md says it means.
• An administrator who needs a distinct raw-API id sets NBI_CLAUDE_INLINE_CHAT_MODEL, which is exactly what this PR adds and documents, and which is the case the feature exists to serve.
• The user-facing case (no pin at all, user wants a cheaper or faster inline model) is completely unaffected, since the clamp only engages when an administrator has pinned something.

The only scenario that loses is "administrator pins the chat model to a CLI-only alias and relies on individual users to repair inline chat themselves," which is the same scenario in which the pin is being escaped. Pairing this with settingLocks.claude_inline_chat_model reporting locked under a chat pin would also close the UI half, so the dialog stops offering an enabled control underneath a disabled one.

tests/test_feature_flags.py::TestInlineChatModelOverride::test_chat_pin_writes_only_the_chat_key currently pins the present behavior as intended, so it would need to move with the decision either way.

Everything else

I looked for other consumers and found no second raw-API construction site: ClaudeChatModel is built in exactly one place, _create_client_options still reads chat_model as claimed, and the seven-place wiring (STRING_OVERRIDE_SPEC, CLAUDE_SETTINGS_OVERRIDES, the derived SETTING_LOCK_NAMES, SettingLockName, the settingLocks getter, the panel payload, and the docs) is complete and consistent with the existing entries. The settingLocks getter is optional-chained, so an older server returning no entry degrades to unlocked rather than throwing. The CSS reuses .expandable-content only for the triangle toggle and .expandable-content-title is already styled for a <button>, so the new disclosure needs no additional reset. Including the key in the panel's object literal is necessary for the reason you give, and I confirmed the panel replaces claude_settings wholesale.

Happy to re-review promptly once the pin question is settled, whichever way you and the maintainers land on it.

[cSlat3r]

Thank you for the fast review and pointing out the blocker! After reconsideration your points make sense, and adding this logic in order to guarantee that existing users using the pre-established pattern do not end up with un expected results or behavior, while also opening up the door for the new pattern to be used wherever relevant. I've taken your alternative essentially verbatim. Verified using the tests and driven through the real NBIConfig.claude_settings property, the real handle_inline_chat_request, and the real _build_setting_locks_response rather than a harness, as well as live testing in my own environment.

On test_chat_pin_writes_only_the_chat_key — you were right that it needed to move, though it turned out to be the comment rather than the assertion. The mapping genuinely does still write only its own destination; what was wrong was the comment claiming that was the guarantee protecting a provisioned inline model. Retargeted, and the behaviour it used to describe is now covered by TestClampInlineChatModel (6 cases, including that an inline pin makes the clamp inert) and TestInlineChatModelLockDisplay (3).

Also added some more observed risks found while implementing but perhaps out of scope for this particular change.

[pjdoland]

Re-reviewed at 593a781a. The blocker is resolved, and I have nothing further.

I re-ran the original reproduction rather than reading the diff, and extended it to the full pin matrix through the real NBIConfig.claude_settings read path and _build_setting_locks_response:

pins set	CLI / agentic model	inline chat model	inline control
neither	user-chat	ESCAPE (user's own)	enabled
NBI_CLAUDE_CHAT_MODEL only	PIN	PIN	disabled
NBI_CLAUDE_INLINE_CHAT_MODEL only	user-chat	IPIN	disabled
both	PIN	IPIN	disabled

Row two is the case that regressed at 89106471, where inline chat resolved to the user's ESCAPE value. It now resolves to the pinned id, matching fa6b175 exactly. Row one confirms the feature itself is intact for unmanaged deployments, and rows three and four confirm the clamp goes inert the moment an administrator provisions the inline var, so the transport-mismatch case this PR exists for still works.

Blanking rather than stamping is the right call and I want to say so explicitly, because the difference is easy to lose in a later refactor. Writing "" leaves the resolution chain doing the work, so the clamp holds no copy of the pinned id that could outlive the pin or drift from it; stamping would have created a second source of truth for the same value. The docstring says this, which should keep it from being "simplified" later.

I also checked the two things a clamp like this usually gets wrong. clamp_inline_chat_model copies rather than mutating its argument, which I verified against the caller's dict. And I looked for read paths that could reach inline_chat_model without passing through the clamped property: the only consumer is claude.py:2396, which reads nbi_config.claude_settings, and the three raw nbi_config.get("claude_settings") sites in extension.py touch only enabled (the one at line 898 deliberately round-trips raw stored state, which is correct there). There is no bypass.

TestClaudeSettingsInlineChatModelPin pinning that the read path calls the clamp, separately from the clamp's own unit tests, is a good instinct. That is exactly the assertion that fails loudly if someone deletes the call while refactoring claude_settings, and it is the failure mode most likely to silently reopen this.

Gates at 593a781a: 1513 passed in tests/ (up 12) plus 76 passed in tests/test_claude_client.py, jest 31 suites / 385 tests, and tsc --noEmit, eslint, stylelint, and prettier --check all clean.

One note offered as context rather than as a finding, since you asked for blockers only: while a chat pin is active, a settings POST persists inline_chat_model: "", so a value a user had stored before the pin arrived does not survive the round trip and will not come back if the pin is later removed. That matches how claude_api_key already behaves under ANTHROPIC_API_KEY in the same handler, it only discards a preference the administrator has legitimately overridden, and the resulting behavior is identical to fa6b175. I would not change it; I mention it only so the choice is on the record.

Thanks for taking the governance point seriously rather than treating it as documentation, and for the thorough test coverage on the way through. This looks good to me.

[pjdoland]

@mbektas Please see if you can get this released with the final 5.4 before it goes out, as this issue is creating some problems for us.