Repository navigation
Description
Measured while clearing a review thread on PR #1562 (head 82f0a9af7), read from the commit blobs rather than a working tree.
A profile record has an id but no provenance. saveConfig resolves the id as config.id || existingId || this.generateId() (line 468) and then replaces the whole entry under its name: providerProfiles.apiConfigs[name] = { ...filteredConfig, id } (line 480). The map is keyed by name, so a name has exactly one id at a time and a rewrite that does not carry the old id drops it out of the section. getProfile reads by name (lines 498-505); looking a profile up by id means scanning entries for a matching apiConfig.id (lines 507-513). No timestamp, revision, writer or origin field exists on the record, so nothing in the section says who wrote it or when.
Consequence for error handling: after a failed read-back, the guard in src/extension/api.ts (line 810) branches on exactly one identity signal, currentRecord.id !== id, and the reasoning is already written down at lines 811-814: identical settings under a different id is a replacement rather than this call own write, because saveConfig keeps an existing id when it overwrites, so a new id can only come from a writer that replaced the record. Skipping the restore there is the data-preserving side of the trade.
What the id signal does and does not separate, case by case:
- A concurrent save that does not carry an id re-derives it from
existingId, so the id is unchanged, the guard does not fire, and content drift is caught separately by themovedKeysbranch (lines 820-824). That case is distinguishable. - A rename writes the record at the new name while carrying the id forward (
webviewMessageHandler.ts:2603destructuresidfrom the previous record and passes it in), so a rename does not look like a replacement either. - An import replaces the whole section at once (
importExport.ts:240callsproviderSettingsManager.import(providerProfiles)), so an import that carries a different id for the same name produces exactly the observation the guard is written for - it catches it, but cannot say whether the writer was an import or any other write that supplied its own id. - An import that carries the same id as the record it replaces produces the same observation as this call own write: identity matches and nothing else in the record says who wrote it. Only the
movedKeyscomparison notices, and only if the content differs. That is the real hole.
So the gap is narrower than "identity cannot help": identity already routes the replacement case, and the unresolvable residue is a same-id rewrite by another writer plus the inability to tell an import from any other id-carrying write. Both trace to the same missing fact - the record does not say which write produced it.
If the imported replacement should be protected, the change belongs in the store rather than in the guard: saveConfig needs to record which write replaced a record (provenance on the entry, or a per-name id history) so a caller can branch on a fact instead of guessing. Recorded here so the guard choice is reviewed as a design decision. Related to issue #2354 (originating view takes the sanitised payload while siblings take the persisted record); both came out of review threads on PR #1562.
Activity