Keep the config keys this build has no setting for - #1118
Draft
notluquis wants to merge 19 commits into
Draft
Conversation
`save` rebuilds `settings.json` and `desktop-settings.json` from the values it
holds, and `read` only ever looked at the keys in `SettingType`. So a key the
file had and this build does not know was not ignored, it was deleted at the
next save.
Measured on master, no other change applied:
before: {"futureSetting":42,"theme":"dark"}
after a read and a save: {"theme":"dark"}
Which means a `settings.json` written by a newer build loses whatever an older
one does not recognise, and `troubleshoot.md` sends people to edit this file by
hand, so a line somebody added on purpose goes the same way.
Neither reference does this. VS Code splices its edits into the original text by
offset (`applyEdits` in `vs/base/common/jsonEdit.ts`), so nothing outside the
edit can be lost. `conf`, which `electron-store` wraps, reads the whole file into
an object, changes one key and writes the object back (`const {store} = this`
through `this.store = store`), so unknown keys survive by construction. This
takes conf's shape, which is the one that fits a writer that already rebuilds
from an object.
`read` now walks the file rather than the enum, and what it has no setting for is
kept and written back. Two sets, not one: `super.read` fills the base class from
the global file while `WorkspaceSettings.save` writes the project one, so sharing
a field would put the global file's leftovers into every project. There is a test
for that crossing specifically.
A key that is not overridable by a project is kept too. It does nothing there,
but deleting a line somebody wrote is worse than leaving one that has no effect.
Mutation checked, five branches one at a time, each turning exactly one test red:
either save dropping the leftovers, either read not collecting them, and the
workspace unset not clearing them.
Not here: `ApplicationData.save` rebuilds the same way, but `app-data.json` is
app-managed rather than documented for hand editing, and the loss reported
against it is dropped array entries rather than unknown keys, which needs a
different fix. jupyterlab#1116.
Worked through this with Claude Code. The two reference implementations above I
read rather than recalled, and the before-and-after is a run against master.
notluquis
marked this pull request as draft
August 20, 2026 16:38
Collaborator
Author
|
Draft. This is wrong in a way a follow-up commit does not fix, and the approach has to change.
Confirmed rather than reasoned about: a probe left such a file behind on my machine, and the next unrelated test run failed while registering its suites, before reaching any test of mine. Deleting the file restored the suite. Anything importing the module inherits it. The enum-first loop this replaced was immune, because a name out of the file never indexed anything. Adjacent, from the same review and each verified:
The description also mis-states the reference it cites. Not pasting the payload. Reworking it as read-modify-write rather than patching these one at a time, and not tonight: this is the fourth time today that a fix of mine in this area produced something worse than what it replaced, and that pattern is the reason to stop rather than push on. Worked through this with Claude Code, which also produced the review that caught it. |
The first attempt at keeping unknown keys walked the file in read() and asked `key in SettingType` for each key. `in` climbs the prototype chain, so `__proto__` answered true, `this._settings['__proto__']` resolved to Object.prototype, and the assignment landed there. At module import, from a file troubleshoot.md tells people to edit by hand. A payload left behind by a probe broke the next test run while it was registering its suites, before any test executed. It also recreated a workspace file the user had deleted, wrote a stale value while getValue returned a new one, kept dropping an overridable key whose value matched the global, and turned a file whose top level is an array into an object with numeric keys. All of that came from the same choice: caching a snapshot at read time and replaying it at save. The description cited conf as the model and did not match it; conf re-reads inside the write. So read() goes back to walking SettingType, which is what master did and is immune, because nothing out of the file ever indexes _settings. save() reads the file and merges its own settings over it, which is conf's shape. Both extra fields are gone, and with them the "one bag each" invariant that rested on a comment and a test. Deleting a key needs an explicit unsetValue, tracked in a set. A value equal to its default is indistinguishable from one nobody ever set, so save cannot use that to decide a removal without deleting what somebody typed. A file whose top level is not an object yields nothing rather than being spread, and object spread defines rather than assigns, so a __proto__ key from the file stays an own property and round-trips instead of reaching the prototype. Mutation checked, four branches, each turning exactly one test red: read walking the file, save not merging, the unset set ignored, the reader accepting non-objects. The fourth passed at first because prettier had reformatted the ternary and the mutation never applied; the test was fine and my check was the thing that could not fail. The fs mock in settings.test.ts had no readFileSync and no beforeEach, so the new tests would have leaked their stubs into whatever ran next. It throws an ENOENT-coded error now and resets per test; verified with four shuffled runs. Worked through this with Claude Code, which also produced the review that found the pollution. The behaviour above is measured against the real classes and a real file, not read off the diff.
Collaborator
Author
|
Reworked rather than patched, since the findings all came from one choice. The first attempt cached what the file held at read time and replayed it at save. Doing the merge in
Both extra fields are gone, and with them the one-set-per-class invariant that was defended by a comment and a test. Deleting a key now needs an explicit The description also cited Four mutations, one red test each: Separately, the Worked through this with Claude Code. Every row in that table is a run against the real classes and a real file. |
The merge answered write-or-not, which quietly meant leave-whatever-is-there for everything else. Two different situations were sharing that answer: a key this build owns and no longer sets, which has to come out of the file, and a key it has no setting for, which is not its to remove. So the value on disk outlived the change that replaced it. `showNewsFeed` back to its default left `false` in the file while getValue returned `true`, and a project override that stopped differing from the global value stayed in desktop-settings.json, which is the "Use default Python environment" path. Setting a key after unsetting it wrote nothing at all, because WorkspaceSettings overrides setValue and never cleared the pending removal. The decision has three answers now. A key of SettingType that matches its default is deleted, because it does not belong in the file whatever the file says. A key a project cannot override is left alone. Everything else is written. That makes the separate set of unset keys redundant: setToDefault and the delete out of _wsSettings already carry it, so it is gone. Written test-first this time, which is the only reason the shape came out right: the three failing cases went in before the fix, and two of them turned out to be testing something else. One passed because the fixture had no serverArgs, so it asserted on the key's absence rather than its removal. Another drove unsetValue with theme, which no project can override and the CLI refuses, so it was an unreachable input; it uses uiMode now. Mutation checked: dropping the delete branch turns three tests red, dropping the leave branch one. Worked through this with Claude Code, which also produced the review that caught all three. Third rework of this function today, and the first where the tests that would catch the next mistake existed before the code did.
A sentence broken across several // lines is a wrapping decision made for the editor and the diff viewer, which both wrap it anyway. Master already reads this way: of its 222 // blocks in src/main, 55% are a single line and 34% are two. Prettier leaves comments alone and there is no max-len rule, so nothing forced the break. Scoped to comments this branch adds, and directives are skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Htfy7XC6ZG2kyr1M3tyWcn
`__proto__` was tested and the other two were not. It is the one that pollutes; `constructor` and `toString` are the two that shadow, and all three have to come back out of the merge as own properties of a plain object, or a key somebody put in the file by hand is lost exactly the way an unknown one used to be.
Walked from the tests skill, which says the negative cases are the test for anything security-relevant and names those three.
The spread is what makes it safe and the mutation shows it: swapping `{ ...onDisk }` for `Object.assign({}, onDisk)` turns the `__proto__` test red, because assign assigns where spread defines. That is what the comment above the line claims, now measured.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016CxXVgkyGjZcrtvCa8WudF
The bare catch could not tell an absent file from an unreadable one, and the next statement in both callers is a write. Probed on this branch: readable at construction, EBUSY at save time, and `save()` wrote `{"theme":"dark"}` with `futureSetting` gone — the exact loss this branch exists to prevent, silently, with the write reporting success. The reachable triggers are all post-startup, since a malformed file throws out of `read()` at module import before a save is ever reached: EBUSY or EPERM while an antivirus or backup pass holds it on Windows, EACCES after a permission change, EMFILE under descriptor pressure.
Absent stays silent, because merging over nothing is right for it. Anything else is logged. Refusing the write outright is better and is jupyterlab#1115's, whose shared reader already does it; this branch is not the place to add a second mechanism for it.
Two things about the tests rather than the code.
`takes nothing from a file whose top level is not an object` used `[1,2,3]`, which is the only non-object shape that survives `read()`: `null`, a number and a string all throw out of `key in jsonData`. The name claimed the whole class and the fixture covered one member of it, so it is named for the array now.
The two `Object.prototype` assertions in the `__proto__` test are for the merge, not for the read, and the test did not say so. Measured by deleting them: with `read()` mutated to walk the file instead of the enum, the test still goes red with both gone, and the round-trip assertion below is what catches that one. They fire for the other mutation, swapping the spread for `Object.assign`. Both now say which.
Found by a code review over this branch. The new guard is mutation-checked in both directions: never logging and always logging each turn exactly one test red.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016CxXVgkyGjZcrtvCa8WudF
Self-review against the rule this repository just wrote down: before adding a side effect to a function that already exists, grep its callers. `readJsonFileOrEmpty` is reached from both `save()` methods, and eighteen call sites in `src/main` reach those, so the log line added in the last commit would have repeated on every settings change for as long as the condition lasted — an antivirus pass holding the file, a permission that stayed wrong. That is the same reason this repository already gives for leaving the directory flush at debug level: raising it would put a line in the log on every single save. Reported once per path per run instead. The set is module state, so it outlives a test the way the unreadable set in jupyterlab#1115 did, and the second unreadable case here read as silent because the first had already reported. `resetUnreadableReports` exists for that and runs in `beforeEach`, which is what `settings.test.ts` and `appdata.test.ts` already do for the sibling set. Mutation-checked: dropping the "not reported yet" half of the condition turns the new test red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016CxXVgkyGjZcrtvCa8WudF
… reaches the write
Both of these are corrections to comments written in the previous round, and both said something measured that was not.
The `__proto__` test claimed its two prototype assertions catch the merge mutation. They do not. Measured one assertion at a time against both mutations this time, rather than seeing the test go red and inferring which line did it:
| assertion | `read` walking the file | spread swapped for `Object.assign` |
| --- | --- | --- |
| `({}).value` | catches | no |
| `({}).pwned` | catches | no |
| `written` contains `__proto__` | catches | catches |
`Object.assign` invokes the `__proto__` setter on `merged`, which retargets that object's own prototype and never writes to `Object.prototype`, so neither probe can see it. The round-trip assertion is the only merge guard, and it is the one that reads as redundant sitting next to two prototype checks, so it now says so. The two probes stay as what they are, the guard against a future change that writes onto the prototype.
The `readJsonFileOrEmpty` comment named EBUSY, EACCES and EMFILE as the triggers. Those fail the `writeFileSync` below as well, so they throw rather than lose quietly. The case that actually reaches the write is the parse failure: `userSettings` is constructed once at import, so a user who follows `troubleshoot.md` and hand-edits `settings.json` while the app is running, leaving a trailing comma, gets the SyntaxError caught here and `will-quit` rewrites the file without their edit.
Found by a second review pass over this branch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016CxXVgkyGjZcrtvCa8WudF
…hape
Three from a third review pass, two of them about claims in my own comments.
The comment on the `__proto__` test said all three assertions fire against a mutated `read()`. None of them do: the test dies first with `TypeError: Invalid property descriptor` out of the assignment itself. The mutation is caught, by the throw, and the per-assertion measurement that produced the earlier claim was measuring nothing — whichever assertion was left, the throw got there first. Written down as what it is now: the two probes catch neither named mutation and stay as the pollution invariant, the round-trip is the merge guard.
`expect(({} as any).c).toBeUndefined()` could not go red. The fixture puts `"c"` as the value of the `constructor` key, and no implementation of `_merged` can define `Object.prototype.c`.
`reportedUnreadable` was never cleared on a later successful read, so a file repaired at noon and broken again at three said nothing the second time, and a support log collected that evening showed the first breakage rather than the current state.
A top-level array or primitive was rejected to `{}` with no log at all, while a parse failure logged. Both wipe the file the same way, so the one that says nothing was the odd one out.
Both new branches mutation-checked; the suite stayed at 583 green until each had a test, which is the tell.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016CxXVgkyGjZcrtvCa8WudF
The dedup added last round only covered the throw path. `reportedUnreadable.delete` ran on every successful parse, before the shape check re-added through `reportRejected`, so a file that parses and is not an object logged on every save while a parse failure logged once. Measured: three saves over a top-level array gave three log lines. The delete belongs in the accepted branch, and a test now pins it at one.
The workspace fixtures threw `new Error('ENOENT')` with no `code`, so the reader took them as read failures rather than as an absent file, and the ENOENT discrimination had no workspace-side coverage at all. They carry the code now. That file also never cleared `reportedUnreadable`, which is module state, so a later test inherited a path already reported and would have read as silent.
Three em dashes had gone into the comments and one into a describe name, which is not how anything else here is written.
Found by a fourth review pass. The delete's new position is mutation-checked: removing it turns the repaired-and-broke-again test red.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016CxXVgkyGjZcrtvCa8WudF
The mark remembered that a path had been reported and not what was wrong with it, so a file that failed to parse and then came back as an array said nothing the second time, and the first failure's wording stood in the log describing a different problem. Both orders are affected, and so is a file that disappears between two breakages, since the ENOENT arm returned without clearing a mark that no longer described anything. The branch's own test for this passed because its fixture puts a valid object between the two breakages, which clears the mark for a reason a real run does not supply. Three tests now cover the transitions with nothing in between, and the ENOENT case clears explicitly. Found by a fifth review pass and confirmed by reading the two call sites rather than by running its probes. Mutation-checked in both halves: matching on the path rather than the kind turns two red, and dropping the ENOENT clear turns one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016CxXVgkyGjZcrtvCa8WudF
The central behaviour of this branch had no test in the `UserSettings` suite. Mutating `_merged` to write a key only when it is absent from the file left all forty of them green; only the workspace suite's `uiMode` test noticed. The two that looked like they covered it could not: one runs against an empty file, and the other has the same value in memory and on disk, so neither can tell "in memory wins" from "only fill the gaps". There is a test for it now, and it pins the other half in the same breath, that a key this build does not own survives. The `null` and non-object arms of the shape guard were both deletable with every test still green. The test comment said `read()` throws on those shapes first, which is true through the constructor and not through `new UserSettings(false)`, which skips the read and reaches `save()` with the file untouched. Both arms are reachable and are covered now. `_merged`'s doc said the merge preserves "every value the read declined to take". It does not: a key this build owns is written or deleted by the decision, and `UserSettings.save` deletes any that equals its default, which is exactly what a declining read leaves behind. That question is jupyterlab#1116's. All three found by a sixth review pass, each verified by applying the mutation it describes rather than by reading. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016CxXVgkyGjZcrtvCa8WudF
… read A `SyntaxError` has no `.code`, so it fell past the ENOENT branch into the permission wording. That inverted the specificity on exactly the case the function's own comment names as the one that reaches the write: somebody follows `troubleshoot.md`, hand-edits `settings.json` while the app is running, leaves a trailing comma, and at quit reads "Could not read" and goes looking at file permissions rather than at what they just typed. The rarer top-level array got the specific message. It carries its own kind rather than reusing `unreadable`, because the dedup is keyed by kind and a file that goes EACCES and then malformed would otherwise stay silent on the second break. That case has a test. Found by a seventh review pass. Both branches mutation-checked: never taking the SyntaxError path turns one test red, and reusing the `unreadable` kind for it turns another. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016CxXVgkyGjZcrtvCa8WudF
The comment said an EBUSY or EACCES would fail the `writeFileSync` as well, so writing after a failed read was safe. That is false, and a review falsified it with real files: a `settings.json` at mode 0200 gives EACCES on the read and OK on the write, because `writeFileSync` opens O_WRONLY and never reads. Reproduced here before changing anything. The file ended up as `{}` with one log line, which is exactly the loss this branch exists to prevent.
The reader now returns `undefined` for a file that is there and unusable, and `{}` only for one that is absent, since merging over nothing is right for that. Both saves return without writing on `undefined`. Refusing was already the stated better answer and was being left to jupyterlab#1115's shared reader; it turns out this branch cannot wait for it, because the case it was deferring is the case that loses the data.
The messages say the file is left alone until it is repaired rather than that keys may be dropped, which is what they now describe.
Found by an eighth review pass. Mutation-checked: removing the refusal in `UserSettings.save` turns one test red.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016CxXVgkyGjZcrtvCa8WudF
The refusal added last round was invisible: `save()` returned `void`, so `jlab config set theme dark` printed "set successfully" over a write that never happened. That silent path is one this branch created, so it is this branch's. Both saves return a boolean now and `config set` asks before it claims anything. The other sixteen call sites belong to jupyterlab#1115, which changes the same signature. Two comments still described the behaviour from before that round. The catch block claimed EBUSY and EACCES "throw rather than lose quietly", which the measurement that motivated the change disproves: mode 0200 gives EACCES on the read and OK on the write. And `reportRejected` was typed as returning an object while returning `undefined`, which only `strict: false` let through. Found by a ninth review pass, which noted these are one reconciliation rather than three defects. Mutation-checked: returning true from the refusal turns the new test red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016CxXVgkyGjZcrtvCa8WudF
`UserSettings.save()` and `WorkspaceSettings.save()` both refuse when the file is there and could not be read, and only the first was covered. Mutating the second refusal away left all 596 tests green, so the guard read exactly like one that works. Found by mutating every decision this branch adds rather than by reading: the other five all turned tests red, and this one did not. A project's `desktop-settings.json` that becomes unreadable between the read and the save would have been rebuilt from whatever survived, which is the loss the merge exists to prevent. The test distinguishes the two files by path, because `WorkspaceSettings.read()` calls `super.read()` and the global `settings.json` is read first; a mock that throws for every read never reaches the branch under test. 597 tests, and mutating the refusal away now turns exactly this one red. Worked through this with Claude Code, and the mutation is what found it.
A review pass found `config unset` announcing success over a write that did not happen. `config set` had grown that guard three rounds earlier and `unset` never did, which is the shape this branch already carries a note about: a guard written on one side of a file only. Checking it turned up the larger half. Neither refusal had a test. Both handlers were unexported, so mutating either guard away left the whole suite green, including `set`'s, which shipped as a fix and was never exercised. Both are exported now and both are covered, the two cases sharing one body so a third command cannot quietly skip it. Mutating either condition turns exactly one red. Two smaller things from the same pass. `reportRejected`'s JSDoc said it "give[s] back the empty object the caller merges over" while the function returns the sentinel that means leave the file alone, which is the opposite instruction to the caller; the signature was already right. And the note about `uiMode` being saved even when it matches the global default had been left above `readJsonFileOrEmpty`, fifteen lines from the check it explains, so it now sits on that check. Two of the five findings were already fixed on this head and one was half fixed, because the pass ran against 595 tests and the branch is at 599. Worked through this with Claude Code. The uncovered `set` guard came out of mutating rather than reading, and it is the second guard on this branch that looked right and could not fail.
A review pass read the new reader as protecting a corrupt settings.json in general. It does not: `read()` still calls `JSON.parse` with no try/catch, and `userSettings` is constructed at module import, so a file edited into invalid JSON while the app is closed throws before `app.whenReady` and the app does not start. Only the mid-run edit reaches the catch and gets the file left alone. The comment on the reader was already scoped to "while the app runs" and stayed accurate; what was missing was anything at the parse itself, which is where a reader forms the wrong impression. Guarding it is jupyterlab#1115, which replaces both call sites with a shared reader, and that is now said where the gap is rather than only in the pull request body. Two of the pass's three findings described a head two commits behind: `save()` does return a boolean, `reportRejected` is declared `: undefined`, and both `config set` and `config unset` check the result. Verified against the pushed tree rather than assumed. Worked through this with Claude Code, and I checked each claim against `git show` on the pushed head before acting on it.
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
1 participant
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.
References
masterwith nothing else appliedOrdering against the other config pull requests
Measured with
git merge-tree --write-tree --name-only, not assumed, and confirmed with a realgit merge --no-committhat was then aborted. The three-argumentgit merge-tree <base> <a> <b>reports no conflicts for the first row, so it is the wrong form to ask:src/main/config/settings.tsandtest/unit/settings.test.tsmasterread()andsave()are what both this and #1115 rewrite, so whichever lands second needs a rebase. #1115 first is the cheaper order: the localreadJsonFileOrEmptyhere exists only because the shared reader lands there, so rebasing onto it is deleting this helper and callingreadJsonConfigFile, plussavereturning the writer's boolean. The reverse order works too and is no worse in substance, just more to redo.#1112 is independent of all of these and can land whenever.
What this does not fix
A file that is there and could not be read is now left alone rather than merged over. That started as #1115's job and turned out to be this branch's: the reason for deferring it was that an
EBUSYorEACCESwould fail the write too, and it does not. Measured on darwin, asettings.jsonat mode0200givesEACCESon the read andOKon the write, becausewriteFileSyncopensO_WRONLYand never reads, so the file ended as{}with one log line. The same holds for the hand-edit with a trailing comma thattroubleshoot.mdtells people to make.save()returns a boolean so a caller can tell, andjlab config setandjlab config unsetboth ask before printing success. Counted on the current head: four of the twenty-three call sites use the result and nineteen ignore it; #1115 changes the same signature and is where the rest get wired.With the app open,
jlab config set logLevel debugin a separate process is still reverted at quit:will-quitcallsuserSettings.save(), which writes everySettingTypekey from in-memory state that predates the other process's write. Unknown keys now survive that; known ones do not. Same asmaster, so not a regression, and not something this claims.Code changes
saverebuildssettings.jsonanddesktop-settings.jsonfrom the values it holds, andreadonly ever looked at the keys inSettingType. A key the file had and this build has no setting for was not ignored. It was deleted at the next save.Measured on
master, nothing else applied:So a
settings.jsonwritten by a newer build loses whatever an older one does not recognise, andtroubleshoot.mdsends readers to edit this file by hand, so a line somebody added on purpose goes the same way.What the references do
applyEditsinvs/base/common/jsonEdit.tsconf, whichelectron-storewrapsconst {store} = thisthroughthis.store = storekey in SettingTypeThis takes conf's shape: it re-reads the file inside the write and merges there. An earlier commit on this branch cached at read time and replayed the snapshot at save, which is not the same thing and is where its defects came from; that is reworked.
VS Code's approach keeps comments and key order too, but it means a JSON-with-offsets editor, which is a larger
change than the problem needs.
The change
savemerges over what the file holds rather than rebuilding it, so what this build has no setting for is kept and written back.readis unchanged: it still walks the enum.readwalksSettingTyperather than the file, which is whatmasterdid and is whymasterwas immune to a__proto__key: nothing out of the file ever indexes_settings.savedecides three things per key, not two. A key ofSettingTypematching its default is deleted, because it does not belong in the file whatever the file says. A key a project cannot override is left alone. Everything else is written. Collapsing the last two into one boolean is what let a stale on-disk value outlive the change that replaced it, and the commits on this branch have that mistake in them.A key that is not overridable by a project is kept too. It does nothing in a project file, but deleting a line somebody wrote is worse than leaving one that has no effect.
User-facing changes
A key in
settings.jsonor.jupyter/desktop-settings.jsonthat this build does not recognise stays there.jlab config listdoes not show it, because it is not a setting; nothing else changes.Backwards-incompatible changes
None. Every key that used to round-trip still does; the ones that used to vanish do not.
Manual testing
The before-and-after above is a run against
masterrather than a description of one, driving the realUserSettingsagainst a real file.Five new branches, mutation-checked one at a time, each turning exactly one test red: either
savedropping the leftovers, eitherreadnot collecting them, and the workspaceunsetValuenot clearing them.The three cases above were written as failing tests before the fix, and two of them turned out to be testing something else: one passed because its fixture had no
serverArgs, so it asserted on the key's absence rather than its removal, and another droveunsetValuewiththeme, which no project can override and the CLI refuses. Both are fixed; that is the only reason the shape came out right this time.Not done: none of the Review guidance checks were run on Windows or Linux.
Remaining
ApplicationData.saverebuilds the same way.app-data.jsonis app-managed rather than documented for hand editing, and the loss reported against it is dropped array entries rather than unknown top-level keys, which needs a different fix. #1116.AI usage