Fix: honor remote deletions in metadata.json merge so unpinning sticks (#1105) - #1120
Fix: honor remote deletions in metadata.json merge so unpinning sticks (#1105)#1120KaladinX wants to merge 5 commits into
Conversation
Problem: when an admin unpinned a project (deleting
meta.pinnedExtensions from metadata.json) and a teammate synced with any
unsynced local work, the teammate's merge silently restored the pin and
pushed it back to the server. mergeObjects() read "field absent from
theirs" as "theirs has no opinion" and kept the local value; it could
not tell a deliberate deletion from a field that never existed. Any
ordinary metadata.json field deletion arriving from the server was
undone the same way.
Change: in mergeObjects(), a field present in base, absent from theirs,
and unchanged locally since base is now left out of the merged result.
The guard sits before the object recursion so a deleted object is
dropped whole instead of being rebuilt {} or entry by entry. All other
rules stay as they were: local changes still win over remote deletions
(conflict), locally-added fields are kept, the local-side-deletes case
still works, and the specialized merges (edits,
initiateRemoteUpdatingFor, projectSwap, users, originalFilesHashes) are
untouched because they run before or override the generic merge.
Also exports resolveMetadataJsonConflict so the new unit tests can run
the real end-to-end resolver, per the ticket's test checklist.
Tests: 10 new cases in resolveMetadataJsonConflict.test.ts covering
every automated item in the #1105 checklist. Full suite: 1286 passing,
7 pending, 0 failing.
|
Ran an adversarial pass over this fix, tracing the pin lifecycle across frontier-authentication, the Codex shell's Conductor, and frontier-server. One thing worth a small guard before merge, one rollout caveat, and two all-clears. 1. An empty
The conflict contents come from frontier-authentication's Before this fix an empty Suggested guard: disarm the deletion rule when 2. Rollout: one stale client re-pins everyone A teammate on an older extension still resurrects the pin and pushes it. Clients with the fix then see the pin as a remote addition (absent from base), which the merge correctly takes. So unpins only stick once every collaborator has updated, and a single stale client re-pins the project for everyone. Nothing to change in this PR; worth a line in the release notes so "the bug is back" reports during the rollout window get read correctly. 3. Conductor cache: verified, no change needed I checked whether the shell's cached remote pins could keep a project pinned after metadata.json converges. They cannot: frontier-authentication clears the cache twice per sync. Pre-merge it reads pins fresh from the fetched remote head and calls 4. Server side: all clear frontier-server never writes metadata.json. Its only touches are read-only GitLab raw-file fetches ( |
Problem: the #1105 fix treats "field in base, absent from theirs, ours unchanged" as a deliberate remote deletion. But the conflict builder in frontier-authentication (GitService.ts) initializes remoteContent to "" and swallows readBlobAtRef failures, so an incomplete fetch can deliver a conflict with theirs: "" and isDeleted: false. The deletion rule would then read every locally-unchanged field as remotely deleted, strip metadata.json down to its specialized-merge fields, and push the result to the whole team. Before #1105 the same input was harmless (every absence fell back to keeping ours). Change: compute theirsIsTrustworthy once per merge — theirs must be non-blank and parse to an object with at least one key — and require it before honoring any remote deletion. A real metadata.json always has keys, so an empty theirs means "the remote blob could not be read", not "the remote deleted everything". Also log each honored deletion via debugLog, since the original bug shipped with no trace anywhere. Why this shape: the guard restores the old keep-ours behavior in exactly the cases where the old behavior was the safe one, and changes nothing when theirs is a real file. Tests cover both the empty-string and empty-object forms.
2 participants
Fixes #1105.
Problem
Unpinning a project deletes
meta.pinnedExtensionsfrommetadata.json, and the deletion reaches the server. But when a teammate with any unsynced local work then syncs, the 3-way merge inmergeObjects(src/projectManager/utils/merge/resolvers.ts) reads "field absent from theirs" as "theirs has no opinion" and keeps the local copy of the pin, which gets pushed back to the server. The project ends up pinned again for everyone, admin included, with no error anywhere. The same applies to deleting any ordinarymetadata.jsonfield.Fix
One guard in
mergeObjects, before both the leaf merge and the object recursion: a field present in base, absent from theirs, and unchanged locally since base is a deliberate remote deletion, so the key is left out of the merged result. Placing it before the recursion matters — fixing only the leaf line would still rebuild a deleted object as{}(or entry by entry) through the recursive path, which the ticket's AC explicitly forbids.Everything else keeps its existing behavior:
edits,initiateRemoteUpdatingFor,projectSwap,users,originalFilesHashes) are skipped or overridden after the generic merge, exactly as before.resolveMetadataJsonConflictis now exported so tests can run the real resolver end to end, per the ticket's checklist.Test checklist (from #1105)
All automated items are covered by 10 new cases in
src/test/suite/resolvers/resolveMetadataJsonConflict.test.ts:meta.pinnedExtensions, local unchanged → nopinnedExtensionsin resultpinnedExtensions→ only that entry gone, sibling survivesversion/url, local unchanged → server values winusers; the others are skipped before the guard can see them)resolveMetadataJsonConflictrun of the unpin scenario → nopinnedExtensions, unrelated local change still mergesRiskiest part
The guard changes merge behavior for every field of
metadata.jsonthat lacks specialized handling, not just pins: remote deletions now stick when the local side hasn't touched the field. That is the ticket's intent, but it's the place to stare at in review. The comparison uses the sameJSON.stringifyequality the leaf merge already used, so "unchanged locally" is judged exactly as before.Gates
npm run lintclean; full VS Code suite: 1286 passing, 7 pending, 0 failing.