Independent publish: staging and unstaging patches via patch groups - #464
Draft
freekh wants to merge 14 commits into
Draft
Independent publish: staging and unstaging patches via patch groups#464freekh wants to merge 14 commits into
freekh wants to merge 14 commits into
Conversation
RFC only, no implementation. Covers: - what a patch set is today, and two bugs in PatchSets.insertPath that become correctness bugs once patch sets decide what gets published (segment-unaware prefix match; insertedPatches skipped on the no-schema path) - the patch group model, and the prefix invariant that decides when a patch must join a group, including the case where patch sets coalesce and retroactively invalidate an existing group - API surface between this repo and content.val.build, annotating applicable/patches rather than filtering it so parentRef stays correct - a test rig that executes staged subsets and asserts applicability, fidelity, non-interference, convergence and minimality - naming options for the patch group concept Also adds the self-contained prompt for the content.val.build repo. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbT8HyyW8rkero7eZPLfjS
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbT8HyyW8rkero7eZPLfjS
🦋 Changeset detectedLatest commit: 7839e59 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Adds an implementation checklist (blocks A-D) so the closure and its test rig can land before content.val.build has any patch-group support. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbT8HyyW8rkero7eZPLfjS
- Naming settled on patch group; drops the alternatives section, which is replaced by the principle it made room for. - New section 2: staged is the truth. base + your group is what the studio shows, what the preview shows, and what publish writes; the compare view is the only exception. Works through the consequences: the preview needs no extra plumbing since draft data is browser-side; a group is only valid relative to a base commit, so patch_group gains base_commit and publish must check it; an array edit cannot leave another author's edit to the same array unstaged, and unstage is not durable against your own later edits in the same patch set; stat cannot detect a membership change since it compares patch ids, so it needs a patchGroupsSha. - markApplied moves from open question to required work in block C, with the reason it is required rather than nice to have. - Test rig gains a base-moved harness step and invariants 9 and 10. - Home repo prompt: base_commit column, a distinguishable stale-base rejection on commit, marking committed patches applied, and an explicit 'do not rebase server-side' since revalidation needs the schema. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbT8HyyW8rkero7eZPLfjS
Corrects section 2.2. There is no base-relative validity condition on a patch group: patches survive commits, and after one user publishes the others keep working on patches that still apply. The prefix invariant is what guarantees it — two patches that could interfere are in the same patch set, so the later one's author already had the earlier one staged, and committing the earlier one into the base cannot move the later one's paths. Two patches in different patch sets are independent by definition. Removes patch_group.base_commit, the stale-base rejection on commit and the revalidation step, from both the plan and the home repo prompt. markApplied stays required, for the plain reason that published patches must not be re-applied. The rig's base-moved coverage becomes a publish-then-continue scenario that tests the argument instead of a revalidation path that no longer exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbT8HyyW8rkero7eZPLfjS
The closure (packages/ui/spa/utils/patchGroups.ts) implements the one rule the design rests on: for every patch group G and patch set PS, G n PS must be a prefix of PS in chain order. stageClosure grows a group forwards from the start of each patch set, unstageClosure shrinks it backwards from the end, validateGroup reports holes and repairGroup fixes them. The scenario harness (patchGroupScenario.ts) does not assert on patch ids. It replays a sequence of edits by several authors, rebuilding patch sets one patch at a time so retroactive merges actually happen, applies each author's group for real, then publishes each author's group in turn from the same pre-publish state and checks everybody else can carry on. Each patch carries its author's intent as a predicate over that author's own view, which must still hold after anyone publishes - a replace landing on a different array element is not detectable from group membership. Every scenario emits a readable trace (chain, patch sets, groups and why each pulled-in patch was pulled in, views, then each publish order) as a snapshot for review, plus a hard assertion that the problem list is empty so a regression fails rather than rewriting a snapshot. The suite caught a real bug on its first run: PatchSets.insertPath used a raw startsWith on the patch set path, which has no segment terminator, so '?foobar/title' matched '?foo'. Removing record key foo and retitling record key foobar were merged into one patch set, meaning staging foobar would silently publish the deletion of foo. Fixed with a boundary-checking isInsidePatchSetPath. Also corrects the plan's account of the second insertedPatches bug: since insert is called once per op, a patch whose first op is a file op has its source ops silently dropped. The fix changes insert's signature, so it is flagged for a decision rather than done here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbT8HyyW8rkero7eZPLfjS
Rewrites the scenario harness around an explicit step list - edit, stage, unstage, publish - so semantics that only appear in a sequence can be scripted. After every step the trace shows each author's group and what each author sees, and when a scenario has no publish step it ends by showing what each author would commit from the same state, which is what makes the ordering asymmetry legible side by side. 14 scenarios, grouped by the question each one answers, three of them marked as open decisions: DECISION 1 - array co-editing. Once two authors have both edited the same array and the first returns to it, both groups contain both authors' work and neither can publish alone. Arrays are therefore effectively single-writer for independent publishing, unless patch sets get finer than the whole array. DECISION 2 - repair policy, run over the same scenario under both extend and truncate so the traces can be compared. Two findings: extend is safe in the merge case, because a merge is caused by a broader path and the patches it pulls in had narrower ones that cannot shift indices; and truncate's cost is invisible to assertions, since dropping the user's own patch still leaves a valid group. DECISION 3 - unstaging. Tightening the intent predicates surfaced a real correctness hole. Unstage a patch, then edit the same patch set, and the closure re-stages it and shifts the indices under the path the author just picked. Base [A,B,C], Bob inserts New at the top, Alice unstages it, Alice renames index 1 meaning B, and gets [New, B*, B, C] - she renamed A. It applies cleanly and the prefix invariant holds; only the content is wrong, which is why the harness checks author intent rather than group membership. The defect is asserted rather than hidden, so the expectation changes when it is fixed. The plan recommends refusing unstage for a patch set the author has pending edits in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbT8HyyW8rkero7eZPLfjS
Corrects the model. The earlier sketch had a group start empty and collect its owner's edits, with the prefix closure pulling in whatever else was required. Making the scenario harness resolve op paths against the author's actual view - rather than letting a test hand-write an array index - showed that does not work. The closure runs when a patch is created, which is after its author has already picked a path. If Alice has inserted at items/0 and that insert is not in Bob's group, Bob sees [A, B, C], picks index 1 for "B", and creating his patch closes his group over her insert: index 1 becomes "A" and he has silently renamed the wrong element. It applies cleanly and the prefix invariant holds - only the content is wrong. Staging later cannot fix a path chosen earlier. So a group holds every pending patch by default. That makes every author's view complete at pick time, and it makes the default behaviour identical to today's all-or-nothing publish, so staging is opt-in rather than a new risk. Independence comes from unstaging instead: carve a patch set out of your group and it leaves your view and your publish. The same hole is still reachable deliberately, by unstaging and then editing the region you just carved out. editWouldRestage is the guard: that edit is refused, with the author asked to re-stage first so their view is complete before they pick a path. Its candidate keys mirror PatchSets.insert - the op path for a replace, plus the parent for ops that may widen to it - because being coarser would make holding one region block edits everywhere, and being finer would miss unsafe edits. The harness gains a step script (edit / stage / unstage / publish), a "picked in" line showing the view each path was chosen against, and a blocked list so a refusal is a reportable outcome rather than a failure. 15 scenarios. The headline pair is Alice shipping a one-line title fix while Bob's half-finished list stays behind, and Bob then finishing and shipping that list against the new base. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbT8HyyW8rkero7eZPLfjS
Types and routes only - no client uses them yet. Existing routes gain fields, all optional so an old client or a server that predates patch groups keeps working unchanged: - PUT /patches takes patchGroupId, alsoAddPatchIds and closureVersion, so a patch and its group membership arrive in one request. There is then no window in which a patch exists but belongs to no group, which is what makes "created means staged" true rather than aspirational. Its response returns the group actually used. - GET /patches annotates each patch with patchGroupIds and adds a patchGroups list. Annotating rather than filtering is a correctness requirement, not a preference: getParentRef takes the chain head from the last patch in that response, so filtering it to one group would make every client compute a stale parent and 409 forever. - /stat gains patchGroupsSha. Patch ids alone cannot detect a stage or unstage - the pending set is unchanged, only who holds it - so without this, unstaging in one tab never reaches another. New: PUT and DELETE /patch-groups/~/patches for staging and unstaging. Both take an already-closed set, because deriving the closure needs the content schema and only the client has it. Both are idempotent. 403 means not your group, 409 means the group is already published. ValOpsHttp forwards these to content.val.build and passes 403 and 409 through rather than flattening them to 500. ValOpsFS acknowledges without storing anything: FS mode has one author and no shared store, and the client already sends an explicit patch id list to /save, so a group held in the client is enough to publish a subset correctly there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbT8HyyW8rkero7eZPLfjS
PatchStagingProvider owns the group and the closure rules; the rows stay presentational and ask "is this staged" and "what else moves if I toggle it". One place maintains the prefix invariant, and it is the same code the scenario suite already tests. StagingToggle gives each row three states. The middle one is the feature: "Held" means the change exists, is not in your preview, and will not go out when you publish - though someone else may still publish it. A held row stays visible and re-stageable rather than disappearing, because if unstaging hid the change there would be no way to find it again and put it back. Held rows are desaturated so the state reads without hovering. Toggling can move more than the row clicked, since a patch set is the unit that must move together. When it does, the tooltip says so and names the other author. Silently enlarging or shrinking somebody's publish is the failure this control exists to prevent, so the explanation is not optional. HeldSummary puts the count in the review header, along with the constraint that held sections are read-only until staged again. Four Storybook stories: nothing held (identical in effect to the old all-or-nothing screen, since staging is opt-in), one change held, two entangled array changes that cannot be separated, and staging disabled - which is what FS mode and any content API without patch group support render. One component, not two review screens. patchGroupsStaging.test.ts unit-tests each primitive so a failure points at one function rather than a whole scenario. Includes a regression test that a top-level replace is not widened to the module root: if it were, holding one region would block every edit everywhere and unstaging would be useless. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbT8HyyW8rkero7eZPLfjS
The plan's sections 2.2 and 2.3 argued that patch set membership made itself consistent, so no guard was needed. That argument assumed the closure ran before an author picked a path, and it does not - it runs when the patch is created. Both sections are rewritten around the counterexample that showed it, and around what was actually built: a group holds everything by default, independence comes from unstaging, and a held region is read-only until staged again. Sections 9 and 10 are rewritten too: seven things move from open to settled, and the checklist now separates what is on this branch from what is blocked on content.val.build. The home repo prompt needed the same correction. It described a group as holding only its owner's patches, which is the model that corrupts content. It now specifies that a new patch joins every open group except those holding its region back, with holdBackForGroupIds carrying that decision - the server cannot derive it, since "its region" needs the schema. Also adds the changeset. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbT8HyyW8rkero7eZPLfjS
A new patch joins every open group except those holding its region back, and the server cannot derive which those are - "its region" is a patch set and that needs the schema. The client names them. The home repo prompt justified "patches outlive commits" via the prefix rule alone, which was the reasoning that turned out to be wrong. The guarantee is real but comes from elsewhere: a group holds everything by default, so an author's view already contained every other pending patch when they picked their path, and committing some of those into the base moves nothing. The only way a group lacks a pending patch is that its owner unstaged it, and that region is then read-only for them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbT8HyyW8rkero7eZPLfjS
PatchSets.insert now takes a whole patch instead of one op. The de-duplication in it is per-patch, but callers looped over ops, so every call after the first hit the insertedPatches guard and returned - only a patch's first op ever reached a patch set. A patch touching two places (a move, or a file op sitting before its source op) was therefore mis-grouped, which was invisible in the compare view but decides what a patch group must contain, and left editWouldRestage and holdsRegionOf blind to the later ops. Six call sites updated. editWouldRestage now takes the op's `from` as well, so a move or copy out of a held array is actually checked. Its parameter type could not carry `from` at all, so the guard returned nothing for that case and only the test harness compensated - externally, and with nothing saying it had to. The extend/truncate scenario never triggered repairGroup: under "a group holds everything by default" Alice's group had no member inside the merged patch set, so there was no hole to repair and the two snapshots were identical apart from the header. Rebuilt so she holds item 0, edits item 1 (a different patch set, so allowed), and Carol's append merges both. Both traces now differ and each test asserts the repair line it expects, so the policy has coverage rather than two names over one outcome. patchGroupId is body-supplied and was interpolated into the content API path unencoded, so "../../commit" would have reached a different endpoint carrying the project's auth headers. Encoded. Three UI fixes: the held count deduplicates, since a patch in two patch sets (any move) was counted twice; a partly-staged row no longer gets the fully-staged tooltip claiming its held half will publish; and indexPatchSets is caught rather than thrown during render, where a patchSets/chainOrder skew mid-sync would have taken down the whole review screen instead of just disabling staging. HeldSummary no longer claims held sections are read-only. The rule is real and guarded, but enforcement at the point of editing lands with the sync engine wiring, so shipping the copy now would promise something the build does not do. The changeset says the same. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbT8HyyW8rkero7eZPLfjS
Restores the schema retry in PatchSets.insert. Moving insertedPatches.add above the !schema branch marked a patch inserted even when its module schema was unknown, and ValSyncEngine only re-inserts while isInserted is false - so the whole-module fallback patch set became permanent, making every pending change in that file one inseparable staging unit. The patch is now only marked once there was a schema to place it with. /patches PUT declared the patch group fields but no handler read them, so a version-skewed client got 200 and believed membership had been recorded when it was dropped. Recording it needs the endpoints on content.val.build that do not exist yet, so the handler now rejects a request carrying those fields with a message saying so. A clear failure beats a silent one. A 401 from the content API on a patch group call became an opaque 500. It now gets the same "verify the api keys" wording as every other call in ValOpsHttp, since it is the app's credentials failing rather than the user's session. PatchStagingProvider now re-validates and repairs the group whenever the index changes, which patchGroups.ts documents as required and nothing was doing. Patch sets coalesce as patches arrive, so a third party's array insert can swallow two leaf patch sets and leave a hole in a group whose owner did nothing; validating only on stage/unstage misses it entirely. Also fixes prettier violations that would have failed the CI format job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbT8HyyW8rkero7eZPLfjS
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.
Lets one person publish a small fix without shipping somebody else's unfinished work.
A patch group is the set of patches one user has chosen to publish. Not a patch set — a patch set is computed from the schema and says which patches must move together; a patch group is curated and says which ones you want live.
Nothing changes for existing projects. A group holds every pending patch by default, so with staging untouched Publish is byte-identical to Publish today. Staging is opt-in.
Eight commits, each reviewable on its own. Design and reasoning:
docs/independent-publish/PLAN.md. The content.val.build side:docs/independent-publish/HOME_REPO_PROMPT.md.Start here: the traces
packages/ui/spa/utils/__snapshots__/patchGroups.test.ts.snapis the deliverable. Every scenario replays edits by several authors and prints, after each step, what each author picked their path in, what their group holds, what they see, and what each would commit. The headline story:Alice ships her one-line title fix. Bob's half-finished list stays behind, survives the commit, and he ships it later. That is the whole feature.
The model, and the evidence for it
Two earlier versions of this design were wrong, and the tests are what showed it.
A group holds everything by default. The tempting model — your group starts empty and collects your own edits — corrupts content. Making the harness resolve op paths against each author's actual view, rather than letting a test hand-write an array index, exposed it: the closure runs when a patch is created, which is after its author picked a path. If Alice has inserted at
items/0and Bob's group doesn't hold it, Bob sees[A, B, C], picks index 1 meaning "B", and creating his patch closes his group over her insert — index 1 is now "A" and he has silently renamed the wrong element. It applies cleanly, every invariant holds, only the content is wrong. Staging later cannot fix a path chosen earlier. Thepicked inline in every trace is that guarantee made visible.Patches outlive commits, but not for the reason I first wrote. An earlier revision claimed a
base_commitcheck and a rebase step were needed; that was wrong, as you pointed out. A later revision justified the absence via the prefix rule alone, which was the reasoning that turned out to be unsound. The guarantee is real and comes from the default: an author's view already contained every other pending patch when they picked their path, so committing some of them into the base moves nothing.The rule is that for every group and every patch set, the group's members within that patch set form a prefix in chain order. So staging pulls in what preceded it in the same patch set; unstaging drops what was built on top of it. The compare view names what a toggle moves and whose it is.
A held region is read-only until re-staged. The hole above is still reachable deliberately — unstage, then edit what you carved out.
editWouldRestageis the guard, and it mirrorsPatchSets.insertprecisely: areplacekeys on its own path, and only ops that can shift positions get the parent. Widening areplacewould make a top-level field the whole module, so holding one region would block every edit — there's a unit test for exactly that.Two pre-existing bugs fixed
?foobar/titlematched?foo— deleting record keyfooand retitlingfoobarwere one inseparable change. The suite caught this on its first run. Over-grouping in the review screen before; publishing a deletion nobody asked for after.PatchSets.inserttook one op at a time while its de-duplication was per-patch, so every call after the first returned early and only a patch's first op ever reached a patch set. Amove, or afileop sitting before its source op, was mis-grouped. It now takes the whole patch (six call sites).Tests
patchGroups.test.ts— 17 scenarios: why the default is everything, independence via unstaging, the guard and its narrowness, patch set integrity (sibling record keys,moveacross two arrays, last-write-wins), and the repair policy under bothextendandtruncate.patchGroupsStaging.test.ts— 20 unit tests, one contract per primitive, so a failure points at one function.problemsasserts no group breaks the invariant, no group fails to apply, no author's stated intent stops being true in their own view, and nothing declared independent is entangled.blockedasserts refusals separately, since refusing is a correct outcome.CI green:
lint,format,typecheck,pnpm test(1102),pnpm run build,examples/nextbuild, plusstorybook build.Two review passes
Both found real problems, all fixed in
083fabaand7839e59. Worth knowing about two:insertrefactor — marking a patch inserted before the!schemabranch made the whole-module fallback patch set permanent, sinceValSyncEngineonly retries whileisInsertedis false. That would have made every change in a file one staging unit.repairGroup; the two snapshots were identical apart from the header. Rebuilt so a merge genuinely invalidates a group, and each test now asserts the repair line it expects.patchGroupIdwas interpolated into the content API path unencoded, so"../../commit"would have reached a different endpoint with the project's auth headers.Not done — blocked on content.val.build
ValSyncEnginedoes not yet hold a real group, so nothing reads one and behaviour is unchanged.PUT /patchesrejects the group fields rather than accepting them and returning 200 for membership it cannot record. Remaining: sync engine wiring (group state, group-scoped optimistic source,patchGroupsSha, enforcing the guard at the point of editing),patch_group_idonapply_patches, andmarkApplied. §10 of the plan has the list.Still open (§9)
Chiefly array co-editing: once two authors have both edited the same array and the first returns to it, both groups contain both authors' work and neither can publish alone — arrays are effectively single-writer for independent publishing unless patch sets get finer than "the whole array". Nothing is broken; it's a question of how much independence arrays get. Also whether a publisher is warned they're carrying someone else's work, and what an emptied group does on Publish.