Conversation
The Claude settings tab posts a fixed set of ten keys when it mounts, and ConfigHandler.post stored that object in place of the old one. Any other claude_settings key was erased the first time the tab opened, including jupyter_ui_tools_external (plmbr#398), which has no UI control and is set by hand in config.json. Merge the posted dict onto the stored value before the admin policies, string overrides, inline-model clamp, and ANTHROPIC_API_KEY scrub run, so those still apply to every key that ends up saved.
ConfigHandler.post saves the whole in-memory config, and unlike the GET handlers it never reloaded from disk. A key added by hand to config.json while JupyterLab runs was overwritten by the next settings POST, including the General tab's mount POST, which sends no claude_settings at all. The tests now write the stored state to disk the way a user would, assert that the handler finished and that posted values landed, and pin that the merge runs before apply_claude_policies (which always rewrites tools and setting_sources), that a posted list replaces the stored one, and that a key from the environment-wide config is carried into the user config. The admin guide now says where to set jupyter_ui_tools_external and that it only takes effect while the Jupyter UI tools are enabled.
Reloading config.json at the start of the POST also moved the baseline for the handler's change checks from the state the server runs with to whatever was on disk. When the two differ (a hand edit, or a second server sharing the NBI user directory): - the ACP tab's mount POST restarted the agent for a change it never made, and missed a real change that happened to match the disk; - ACP enabled on disk counted as already on, so the user's own ACP toggle lost the tie to Claude; - a Claude disable found on disk left no conflict to resolve, so enabling ACP never disconnected the live Claude client. Capture the prior Claude and ACP state before the reload, compare the ACP change against it, and disconnect the Claude client whenever Claude was on before the POST and is off after it. Also add a 5.4.1 changelog entry, and tell admins not to create a claude_settings object holding only this key (a user-level object replaces the environment-wide one as a whole) and to restart after the edit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
jupyter_ui_tools_external(#398) has no control in the Settings dialog, so it is set by hand underclaude_settingsin~/.jupyter/nbi/config.json. Opening the Claude settings tab erased it. The tab saves the ten keys it renders when it mounts, andConfigHandler.poststored that object in place of the old one. A deployment that set the key to serve the Jupyter UI tools through the relay lost it the first time a user opened Settings, and Claude mode went back to the in-process server without any notice. The same applied to any otherclaude_settingskey the tab does not show.Solution
ConfigHandler.postmerges a postedclaude_settingsdict onto the stored value, then runs the admin policies, string overrides, inline-model clamp, andANTHROPIC_API_KEYscrub on the merged result. The order matters:apply_claude_policiesalways writestoolsandsetting_sources, so merging after it would empty both on a partial POST. The merge is shallow, so a posted list still replaces the stored list and unchecking a tool sticks.claude_settings. It now callsnbi_config.load()before applying the POST. This also keeps settings saves from rewriting a hand-editedmcp.jsonfrom memory.acp_settingsare captured before the reload. The exclusivity check and the ACP restart decision therefore compare against the state the server is running with, as they did before, not against the disk. The handler also disconnects the Claude client whenever Claude was on before the POST and is off after it. Without that, a Claude disable picked up by the reload would leave the client connected while ACP took over.claude_settingskeys in the user config and not to create an object holding only this key. It also says the key needs the Jupyter UI tools enabled and takes effect after a restart. A5.4.1entry is added under Fixed.Testing
tests/test_config_handler_claude_settings.pyhas 18 tests. They driveConfigHandler.postagainst a config whose files all live intmp_pathand read the results back from disk.continue_conversationpolicy, and the API-key scrub all apply to stored keys.claude_settings, including an unrelated top-level key andmcp.json; a malformedconfig.jsonfails without being overwritten.claude_settings;pytest tests/ --ignore=tests/test_claude_client.py(1771 passed),jlpm tsc --noEmit, stylelint, eslint, prettier on the changed docs, andjlpm jest(423 passed).jupyter_ui_tools_external: true, against this branch and againstmainas a negative control. No console errors and no server tracebacks, and every config POST returned 200.maininline_completion_debouncer_delayRisks / follow-ups
config.jsonormcp.jsondoes not parse, a settings POST returns 500 and leaves the file alone. Before, the POST rewrote the broken file from memory, which discarded the edit.GET /capabilitiesalready fails the same way. The frontend only logs a failed POST to the console, so a clearer error could be a follow-up.claude_settingsof their own, the first save copies the environment-wide object into the user config, merged keys included, as it already did for the keys the tab sends. A later change to the environment-wide value does not reach that user. Layering user settings over environment settings key by key on read would address this more generally.NBIConfigwould cover every writer.tools), made while the page is open, is reverted when that tab opens. Now that the server merges, posting only changed fields from the panel would cover that and would let the ACP tab stop relying on change detection to avoid restarts. The Claude client also still reconnects on every Claude tab mount.acp_settingsis not merged. The ACP tab replaces its object the same way. It has no hand-set keys today.claude_settingsPOST still replaces the stored object. Only a scripted client can send one.disabled_toolsfor servers that have not connected yet. That is separate, and this PR does not change it.5.4.1section above5.4.0, whose heading still reads "unreleased". Happy to adjust if you would rather manage the release headings.No issue was filed for this. It came up while checking the 5.4 release notes against the code, and the details are above.