feat(config): explicit rebase provenance for deletion vs unseen keys (#1478) - #2613
Conversation
Replace the pinned grokExcludedModels raw-delete setup with the provenance helper so its deletion assertion now proves explicit intent. The unrelated claudeCode hand-edit assertion remains unchanged because provenance supersedes only disk-only top-level key inference.
|
✅ Deterministic PR hygiene checks passed. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThe change adds version-1 config deletion provenance, applies it during config save and rebase operations, migrates top-level deletion writers to a shared helper, and adds tests for deletion, legacy, future-version, round-trip, and reassignment behavior. ChangesConfig provenance and deletion tracking
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ConfigWriter
participant deleteConfigTopLevelKey
participant saveConfigPreservingClaudeCode
participant DiskConfig
participant LiveConfig
ConfigWriter->>deleteConfigTopLevelKey: delete a top-level config key
deleteConfigTopLevelKey->>LiveConfig: remove key and record pending deletion
ConfigWriter->>saveConfigPreservingClaudeCode: save live config
saveConfigPreservingClaudeCode->>DiskConfig: read persisted config
saveConfigPreservingClaudeCode->>LiveConfig: reconcile disk keys and tracked deletions
saveConfigPreservingClaudeCode->>DiskConfig: persist config and provenance
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a3a682aa4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (provenanceProjection.configRebaseProvenance === undefined) delete projectedConfig.configRebaseProvenance; | ||
| else projectedConfig.configRebaseProvenance = provenanceProjection.configRebaseProvenance; |
There was a problem hiding this comment.
When a long-lived server loaded an unknown/future provenance record and a newer process subsequently updates that record on disk, reconciliation deliberately skips this field, and these lines unconditionally restore the stale in-memory projection. The next unrelated saveConfigPreservingClaudeCode therefore erases metadata the current binary cannot interpret, defeating the promised forward-compatible passthrough. Derive opaque provenance from the authoritative onDisk value while combining only understood version-1 candidate tombstones.
AGENTS.md reference: src/AGENTS.md:L7-L11
Useful? React with 👍 / 👎.
| } catch (error) { | ||
| if (previousSettings.hasCodexAutoStart) config.codexAutoStart = previousSettings.codexAutoStart; | ||
| else delete config.codexAutoStart; | ||
| else deleteConfigTopLevelKey(config, "codexAutoStart"); |
There was a problem hiding this comment.
When /api/settings attempts to set a previously absent option and the injected or real save throws (for example, a full disk), this rollback calls the provenance helper for every setting that was previously absent. Although the visible values are restored, the WeakMap now contains tombstones for actions the user never requested; a later successful save can persist those tombstones and delete concurrent values for these keys. Restore the provenance bookkeeping captured before the attempt, or use non-intent-recording deletes for rollback.
AGENTS.md reference: src/AGENTS.md:L7-L11
Useful? React with 👍 / 👎.
| // Drop the key entirely once nothing is left in it, so enabling twice does | ||
| // not leave `"clientIntegrations": {}` behind in the user's file. | ||
| if (Object.keys(integrations).length === 0) delete config.clientIntegrations; | ||
| if (Object.keys(integrations).length === 0) deleteConfigTopLevelKey(config, "clientIntegrations"); |
There was a problem hiding this comment.
When enabling the final disabled integration removes clientIntegrations, this helper stores the tombstone against confirmedConfig, but mutatePersistedConfig then passes a new object returned by projectCustomModelCatalogMigration to persistConfigUnlocked. Because pending provenance is identity-keyed in a WeakMap, the projected object has no tombstone and the resulting file contains no configRebaseProvenance; after restart, the legacy rebase path can still discard an unrelated disk-only key. Project or transfer the provenance before changing object identity.
AGENTS.md reference: src/AGENTS.md:L7-L11
Useful? React with 👍 / 👎.
Replace the pinned grokExcludedModels raw-delete setup with the provenance helper so its deletion assertion now proves explicit intent. The unrelated claudeCode hand-edit assertion remains unchanged because provenance supersedes only disk-only top-level key inference.
Replace the pinned grokExcludedModels raw-delete setup with the provenance helper so its deletion assertion now proves explicit intent. The unrelated claudeCode hand-edit assertion remains unchanged because provenance supersedes only disk-only top-level key inference.
1 participant
Summary
Closes #1478.
src/config.tsstored snapshot baselines only and inferred intent from key presence, so "the live writer deleted this key" and "this key was never in the live config" were the same observation. The rebase path had to guess, and both wrong guesses were separately pinned by existing tests.Deletion intent is now explicit.
deleteConfigTopLevelKey()records the writer's intent alongside the delete, and a versionedconfigRebaseProvenance: { version: 1, deletedTopLevelKeys: [...] }carries it across a stale whole-config rebase. Every top-level deletion writer routes through it — 10 modules, 27 named deletion paths plus one generic keyed path, enumerated in the design note rather than trusted to the issue's estimate.Compatibility is lossless in both directions, and each direction has a test:
One subtlety worth naming: provider preservation reads symbol-keyed live-owner state that
structuredClonedrops, so ownership is resolved before the JSON provenance projection rather than after. Getting that order wrong silently loses disk-only provider rows.Verification
Falsified independently on the merge with current
dev: dropping the provenance-aware key set from the rebase turns "provenance distinguishes an unseen disk key from an explicit deletion" red while the other 42 stay green — which is what pins that the change discriminates rather than just widening the rebase. Restored, all green. The subagent additionally falsified the account-pause writer by reverting it to a rawdelete.One pinned test changed: the Grok deletion case now expresses deletion intent explicitly instead of relying on a bare
delete. That assertion was pinning the ambiguity this issue exists to remove. The pinned Claude hand-edit assertion is untouched.Merge note
Two conflicts with
dev, both the same shape: #2463 and #2464 added top-level config fields at the position this branch also extends. Both sides kept — they are independent additions, not competing ones.Checklist
devdevlog/_plan/260826_config_rebase_provenance/010_design.mdSummary by CodeRabbit