[Fix] the false Update incomplete banner on trunk after an update run from a ticket - #446
Merged
Merged
Conversation
|
Warning Review limit reachedNext included review available in 3 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
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 |
juanmaguitar
force-pushed
the
juanmaguitar/419-false-update-incomplete
branch
from
September 11, 2026 10:33
a6fda17 to
a8d1c73
Compare
Fixes #419 `git:update-trunk` parks the linked ticket before it runs, so by the time it wrote `updateIncomplete` HEAD said trunk and the write landed at site level. The build that follows ends after the handler has returned to the ticket, so `sites:mark-update-complete` cleared the branch-level copy instead. Nothing cleared the site-level one, and it surfaced as a false "Update incomplete" banner the next time the contributor was on trunk. `writeWorkMetaOn` takes the ref rather than reading HEAD, and the update writes on the branch it is about to return to. The copy earlier versions left behind is cleared once in the same run, so an affected site is not stuck with the banner. The failure path is unchanged: it leaves the contributor on trunk, so HEAD is already the right scope there. The layer-3 test the issue proposes is red without this. The neighbouring #108 test stubbed `currentBranchName` to a constant, which hid the park from every read between it and the return; it now follows the checkouts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015pjnEjcbyVZHWmhkoeCiaB
…ate, Fixes #419 Self-review findings on the previous commit, all in the same write. The applied patch must not follow the flag onto the ticket branch. The park commits the worktree, applied hunks included, onto the branch's WIP commit and the return checkout puts them back, so the patch is still in the work when the update ends; `branches:rebase` keeps its record for exactly that reason, and #328 reads it to refuse publishing another author's hunks as your own. Only trunk's tree was reset, so only trunk's record goes. The clear of the stale site-level copy was unconditional on where the live flag actually landed. A branch with no entry of its own writes its flag at site level, and an up-to-date run writes none at all; clearing in either case dropped a genuine incomplete state. `workMetaScope` answers where a write will land without writing, so the clear happens only when the live flag is somewhere else. Four more tests, three of them red on the previous commit: the stale copy being cleared, the branch with no meta of its own, the up-to-date run, and the ticket's applied-patch record surviving. Also corrects the failure path's comment, which claimed site level was the wrong place for the flag when that path is precisely where it is right. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015pjnEjcbyVZHWmhkoeCiaB
…ixes #419 Re-review finding on 046154d. The flag's write learned that a branch with no entry of its own reads and writes at site level, so the site record is that branch's rather than trunk's — but the applied-patch clear beside it did not, and stayed unconditional. In exactly that case the update parked the ticket, deleted the only record of the patch the park had just committed into the WIP commit, then checked the branch back out and put the hunks back on disk: applied code with nothing recording it, so no Revert and no refusal to stop it being submitted as the contributor's own work (#328). Both site-level corrections now run only when the site record really is trunk's: no ticket to return to, or a returning branch that has an entry of its own. `writeWorkMetaOn` no longer reports where it wrote. The handler asks `workMetaScope` once, before the writes, because both of them need the answer. The new test is red on 046154d. Also drops a sentence from the failure path's comment that defended a case it cannot reach: only `updateToLatestTrunk` sets `stage`, and it runs after the park, so a park failure never reaches that write. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015pjnEjcbyVZHWmhkoeCiaB
The sentence dropped in the previous commit was correct and the reason given for dropping it was not: `switchToBranch` tags its own checkout failures `stage: 'checkout'` (src/ticket-branches.js:345) and `withSwitchMarker` rethrows them intact, so a park that dies reaches this write with HEAD still on the ticket. Reading HEAD is right for that case as well as for the update's own checkout failure, and the comment now says why for both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015pjnEjcbyVZHWmhkoeCiaB
The layer-3 test proves the handlers write the flag where the build will clear it. This proves what the contributor sees, which is the thing #419 was about: a red "Update incomplete" banner offering to retry a build that had already succeeded. Link a ticket, update to latest trunk, let the chain finish, Unlink, and assert that trunk shows neither the banner nor "Retry install & build". The last step is the one that reveals it: everything is quiet until the site is back on trunk, which is where the stale flag was read from. Red against trunk's src/main.js on exactly that assertion, in 17 s. Green here in under 2 s, because the fixture's build script is `node -e ""`. This is what replaces the manual pass for this change. What a journey still cannot reach — a real wordpress-develop build, an OS dialog, a Windows path — none of it is involved in a metadata scope bug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015pjnEjcbyVZHWmhkoeCiaB
Two rules, each in the file that owns it rather than restated in both. TESTING.md gains the bar for the default suite: a journey earns its place by being a path a contributor walks, not a state someone had to fabricate. The question is not whether the bug that prompted it was specific — it is whether the flow is. Every journey runs on macOS and Windows for every non-draft pull request, which is the reason the bar exists and also the reason a one-second journey over a real flow is cheap. What needs an app launch but not on every pull request has a home already: the opt-in real-setup lane from #444. AGENTS.md gains the consequence for the pull request template: "a green test suite does not replace it" is about the unit suite, which never opens a window. A journey that drives the reported path is not that, so name it, say it is red without the change, and spend the section on what a journey cannot reach. This PR is the first instance of both, which is why they ship with it: without them its own How to test this section contradicts AGENTS.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015pjnEjcbyVZHWmhkoeCiaB
juanmaguitar
force-pushed
the
juanmaguitar/419-false-update-incomplete
branch
from
September 11, 2026 10:39
a8d1c73 to
ef5d6de
Compare
juanmaguitar
added a commit
that referenced
this pull request
Sep 11, 2026
…base survives an overlapping write, Fixes #172 (#454) ## Why Every write to a site's stored metadata read the whole record, changed one field and wrote it back. Two writes that overlap lose one of the two changes. Harmless while the fields were labels and flags; since #108 one of them is a branch's recorded trunk base, written once when the branch starts and read for the rest of its life as the base every patch for that ticket is measured against. Lose it and the patch is generated against the wrong trunk: not empty, not refused, wrong. The overlap the issue names is a ticket started while a trunk update is finishing. It is reproduced here, and so are five more windows of the same shape that the self-review turned up. ## What changes The root cause is narrower than the issue's framing. The store's `get` and `set` are synchronous, so the event loop already serialises writers, and `mergeSiteMeta` on its own was atomic: nothing yields between its read and its write. What was not atomic was every writer that computed what it stored from a read taken an `await` earlier. `changeSiteMeta(sitePath, change)` is now the helper for those: it awaits the store, then reads, applies the caller's function to the record as it is at that moment, and writes, with no yield in between. `changeWorkMetaOn` is the same idea one layer up, for the per-branch work meta whose scope has to be resolved before anything can be written. Six writers were losing changes, each a real lost write rather than a theoretical one: - **`mergeBranchMeta`** read the `branches` map, awaited the store, and wrote that map back. A branch another flow started inside that await was written out of existence, base and all. This is the one the issue describes. - **`wordpress:setup`** held the whole site map, not one record, across a Git spawn that reads the new clone's trunk info. A write to *any other site* during it was undone. The read now happens before the record is touched. - **`sites:set-ticket`** read the branch's recorded base on the existing-branch path and wrote it straight back, which rolled back a `branches:rebase` that moved the base while the switch was running. The base is now written only by the flow that created the branch; the other path leaves it alone rather than rewriting it to the value it just read. - **`branches:rebase`** keeps the applied-patch record and drops only its text, and it read that record, resolved the write scope with a Git spawn, then wrote back what it had read. A discard or an apply landing in that window was replaced by the record from before it, which is a revert banner for a patch that is not on disk and a Revert with no hunks to find. This is the one that costs a contributor a patch rather than a base. - **`branches:delete`** rebuilt the map from an earlier read, the same way `mergeBranchMeta` did. - **`migrateSiteToBranches`** is the one write that cannot be a single read-change-write, because its Git work sits between the read and the write. It now yields to a migration that finished in the meantime, and its catch re-reads instead of handing back the record it opened with. That last part was a bug of its own: a migration that loses the race throws `branch-exists` before the guard, so the loser reported an unmigrated site and `branches:rebase` refused a ticket whose base was on record. AGENTS.md carries the rule under the persistence bullet, stated as the rule itself rather than as a claim about which helper gets called: nothing may yield between reading the store and writing it back. Nothing about what is stored changes. Same keys, same shapes, no migration needed. ## How to test this **Platforms: any.** Nothing here touches spawning, paths, or line endings. Every command below runs in the repository directory. Six new layer-3 tests in `tests/unit/ipc-wiring.test.cjs`, each verified red against trunk `f31e5df` and green at head: ``` node --test --test-name-pattern="issue #172" tests/unit/ipc-wiring.test.cjs npm test npm run test:electron ``` The instrument is the stubbed `getStore`. Three of the tests run one handler and land a second handler inside *each* store access the first one makes, one subtest per access, holding the first until the second has run to completion; `AsyncLocalStorage` is what lets a single stub tell the two flows apart. Iterating over every access rather than picking one matters: the count is an implementation detail, and these fixes changed it, so a test pinned to the access that fails today would pass vacuously tomorrow. | Test | What trunk loses | | --- | --- | | a ticket started while a trunk update is finishing keeps its recorded base | the new branch's base, at access 5 of 8 | | linking a ticket does not roll back a rebase that lands while it switches | the rebase's new base, at accesses 6 and 7 | | a rebase does not restore an applied-patch record a discard cleared while it ran | the discard's clear, at five of eleven accesses | | registering a finished clone does not undo another site's write while it reads the trunk info | the other site's whole record | | a migration that finished first is not overwritten by one still doing its git work | the winner's map, replaced by the loser's | | a migration that lost the race still reports the record the winner wrote | the answer, not the record: `branches:rebase` says "no recorded starting point" | One thing to know when reading the staged tests, because it is not obvious: the hold sits inside the stubbed `getStore`, never between a `get` and its `set`, so the nested flow never sees a half-written store, which is a state the real one cannot be in. For the two pairs that live in one process the real window between read and write is microtask-only, so the staging opens a wider window than the app can. The lost-write shape is identical and reachable by two overlapping microtask chains; the clone and migration tests sit across genuine Git spawns, where the window is as wide as it looks. By hand, on either platform, on a packaged build of this branch's head: link a ticket, run **Update to latest trunk**, and as soon as `Updated to the latest trunk` appears link a second ticket. Open the second ticket's card, and **Create patch** must produce a diff of that ticket's changes only. The window is a few milliseconds, so a manual pass is not evidence either way. It was not run for that reason, and the tests are what covers this. **What must not have happened:** - No branch may have lost its recorded base. That is the whole issue: a base silently replaced produces a patch that is wrong rather than missing, and nobody finds out until someone tries to apply it. - A site registered while another site's record was being written must not have blanked it. The clone path held every site's record at once, so the blast radius was the whole registry, not one site. - A rebase must still keep the applied-patch record and still drop its text. The record is what #328 reads to refuse publishing another author's hunks as your own, and a record without a text is exactly "applied, not revertable" to the card. - The update's own writes must still land: the trunk oid, the incomplete flag on the returning branch, and the return to the ticket. The existing #108 and #419 tests cover these and are green. - A branch whose entry is created by a switch rather than by a start must still be readable. It now has no `baseOid` key at all rather than an explicit `null`; every reader treats the two the same, which is why `branches:list` and the patch refusal are unchanged. ## Risks and limitations Review outcome: 5 `[fix here]` · 6 `[follow-up]` over three passes, all 5 fixed and 4 of the follow-ups taken. Details in the collapsed section. - **Two flows can still move HEAD at once.** This fixes the store half of the concurrency. A rebase running while a link is inside its checkout is two checkouts on one worktree, and the mid-switch marker only refuses after a checkout has already failed. Out of scope for #172, which is about what is stored, and deliberately not filed as an issue. - **Two flows still fight over `currentBranch` and `tracTicket`.** With a link landing inside the update's return, whichever writes last names the active ticket, and HEAD may say the other. That is field-level last-writer-wins, not a lost write: both values are recomputable from HEAD, and `activeBranch` reads HEAD first. Recorded here rather than opened. - **`workMetaScope` still decides a scope by reading, then writes after a yield.** If a branch entry appears in that window the flag is filed at site level while the reader looks on the branch. The result is a flag written where nobody reads it, which is the #419 shape, not a dropped base, and no other writer's data is destroyed. Left as is rather than folded into the change callback, because it would rework the work-meta path #446 has just settled. - **A lock was not added.** See the collapsed section for why. If a future writer must do Git work between its read and its write, the migration's pattern is the shape to copy, not a lock. - **Two concurrent migrations can still record the safe answer instead of the true one.** If the loser gets far enough to see the branch on disk it records `baseOid: null` for it, and if it writes first the winner yields to that. Null is the deliberate answer for a branch this app did not create (#308): the ticket refuses patch operations rather than guessing. So the failure mode is a refusal a re-link clears, not a wrong patch, and closing it would mean ordering two migrations rather than just stopping them clobbering each other. - **Not driven by hand.** The windows are milliseconds wide and no manual sequence hits them reliably, which is why the tests iterate over every store access instead. ## Related Fixes #172. Follows #446 (Fixes #419), which moved the incomplete flag onto the returning branch and named this mechanism on the way past. #108 introduced the recorded base, #308 is why a branch without one refuses rather than guesses, and #328 is why a rebase keeps the applied-patch record it strips. --- <details> <summary>Design decisions and alternatives considered</summary> The issue offered three shapes: serialise writes to the record, make the write take the field rather than the record, or guard the one value that cannot be lost. **A per-site lock** was rejected as more machinery than the bug needs. The store's own calls are already synchronous, so a lock adds nothing a synchronous read-change-write does not, and it brings the usual costs: every writer has to remember to take it, one that awaits inside it holds everyone else, and a forgotten one is invisible until it races. It would also have deadlocked the test that reproduces the bug, since the held update would own the lock the link waits for. That the test hangs under that fix rather than failing was itself a review finding, and the wait is now bounded so a future attempt gets a readable failure instead of a silent CI hang. **Per-field writes** do not fit the value at stake. `baseOid` lives inside `branches[ref]`, a nested map that is replaced whole, so a field-level write of `branches` is exactly the whole-map write that lost the base. A path-level write would work but is a second write API beside the merge, and the merge is already a per-field write at the top level. **A guard for `baseOid` alone**, refusing to write a map that drops an entry the store still has, would have protected the one value and left the pattern in place for whatever is stored next, which the issue names as the reason not to. The applied-patch record the rebase used to restore is that next value, and it was already there. `changeSiteMeta` is the smallest of the four: it changes where the read happens, not what is written, and it removes the pattern rather than guarding one instance of it. One thing considered and declined: a runtime guard rejecting an `async` change function, which would otherwise store a Promise that serialises to `{}` and wipe the record. Every call site is in the same file and synchronous, the JSDoc says so, and a defensive check for a programmer error the surrounding code makes obvious is the speculative kind this repo's complexity rule asks not to add. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> 5 `[fix here]` · 6 `[follow-up]` over three passes. All 5 fixed; 4 of the 6 follow-ups taken as well because each was a few lines; the 2 deferred are in **Risks and limitations** with their reasons. Fixed: 1. Architecture (pass 1, `b5f24ff`) `[fix here]`: `wordpress:setup` held the whole site map across the Git spawn that reads the clone's trunk info, so a write to any other site during it was undone. Read hoisted, write through `changeSiteMeta`. `a651d4f`, with a test red on `b5f24ff`. 2. Architecture (pass 1) `[fix here]`: `sites:set-ticket` read a branch's recorded base and wrote it back a yield later, rolling back a `branches:rebase` landing in between. The read is gone and the base is written only by the flow that created the branch. `a651d4f`, test red on trunk. 3. Architecture (pass 1) `[fix here]`: the migration's catch returned the record from before its git work, so a migration that lost the race reported an unmigrated site and `branches:rebase` refused a ticket whose base was on record. Re-reads now. `a651d4f`, test red on `b5f24ff`. 4. Architecture (pass 1) `[fix here]`: the AGENTS.md sentence claimed every write goes through `changeSiteMeta`; eight handlers write the store directly and correctly. Restated as the rule itself. `a651d4f`. 5. Architecture (pass 2, `a651d4f`) `[fix here]`: the migration's race-loser branch swallowed the error it returned past. Logged. `b7303ee`. 6. Tests (pass 1) `[follow-up]`: the staged test hangs rather than fails under a mutex-shaped fix, and `node --test` sets no timeout. The held wait is bounded with a message naming the deadlock. `a651d4f`. 7. Architecture (pass 2) `[follow-up]`: `branches:rebase` read the applied-patch record, resolved the scope with a Git spawn, then wrote back what it had read, so a discard or apply landing in the window was replaced by the old record. Taken as a fix rather than deferred: it is the same class, it was the one live counter-example to the rule this PR writes into AGENTS.md, and it costs a patch rather than a base. `changeWorkMetaOn`. `b7303ee`, test red on trunk and on `a651d4f`. 8. Tests (pass 2) `[follow-up]`: two guards pinned the number of store accesses a flow makes, which the tests' own comments call an implementation detail. Replaced by guards that the overlap happened. `b7303ee`. 9. Architecture (pass 3, `b7303ee`) `[follow-up]`: the work-meta change re-created a branch entry that had been deleted between the scope decision and the write. The write is conditional on the entry still being there. `6ec7e7e`. 10. Two pass-3 observations, both taken in `6ec7e7e`: the helper re-derived the branch from HEAD with a second Git spawn when its one caller had the ref in hand, so it takes the ref now like `writeWorkMetaOn`; and the rebase/discard test asserts the rebase's own write landed, the guard its sibling already had. Deferred: 11. Architecture (pass 1) `[follow-up]`: `workMetaScope` decides a scope by reading, then the write follows a yield. The worst case is a flag written where nobody reads it, not a lost base; reworking the path #446 just settled is not worth it here. 12. Architecture (pass 2) `[follow-up]`: two flows can still move HEAD at once; this PR fixes the store half of that concurrency, and the mid-switch marker only refuses once a checkout has already failed. - **CodeRabbit:** not run on `6ec7e7e`. The check reads pass; its comment reads "Review limit reached, next included review available in 55 minutes". Merged inside that window on the author's decision, so no retry was requested; the three fresh-context passes above are the review coverage for this change. - **Review:** completed. Separate agent context (Claude, fresh session per pass with the diff and `.github/instructions/code-review.instructions.md` only), three passes per step 3 of that file. Pass 1 head `b5f24ff`, pass 2 head `a651d4f`, pass 3 head `b7303ee`; base `origin/trunk` `f31e5df` throughout. Working tree clean at each pass; no uncommitted or untracked files in scope. Deterministic layer run by the author and reported to each pass: `npm run lint` clean, `npm test` and `npm run test:electron` green at every head (1312 tests at `6ec7e7e`). - **Since review:** `b5f24ff` → `a651d4f` → `b7303ee`, each re-reviewed in a fresh context rather than carried forward; pass 2 reconciled all four pass-1 fixes as resolved, pass 3 reconciled all five earlier fixes as still in place. `b7303ee` → `6ec7e7e` is pass 3's own follow-up and observations: one function renamed to take a ref and drop a spawn, one guard line, one test assertion. No fourth pass was run; the six #172 tests are red on trunk and green at `6ec7e7e`, and the full suite passes on both runtimes. </details> <details> <summary>Implementation notes</summary> `src/main.js` - `changeSiteMeta(sitePath, change)` sits above `mergeSiteMeta`, which is now that function with a shallow merge. It returns the record it wrote, which is what `migrateSiteToBranches` hands back. - `changeWorkMetaOn(sitePath, ref, change)` resolves the scope first, because that is the only part that cannot be answered without yielding, then does one read-change-write against the ref its caller already has. Returning null from the change writes nothing, and so does an entry deleted between the scope decision and the write. - `mergeBranchMeta` and `branches:delete` build the `branches` map inside the change function. `wasActive` in the delete is still decided from the HEAD read before it, because that is a question about the checkout, not about the record. - `sites:set-ticket` leaves `baseOid` undefined on the existing-branch path, and the patch spreads it only when it is set. An entry created there has no `baseOid` key rather than an explicit null; `patchBaseOid`, `branches:list`, `branches:rebase` and `site:status` all read the two the same way. - `migrateSiteToBranches` guards its write with `now.branches ? now : …` and its catch with a re-read, which are the same question asked at the two places the race is observable. The catch logs the error it returns past, so a genuine failure that coincides with another flow finishing the migration is not swallowed. `tests/unit/ipc-wiring.test.cjs` - The staging counts a flow's store accesses from the moment its Git work is done, then replays the whole flow once per access. - The held wait is bounded at four seconds with a message naming the deadlock it expects, because `node --test` sets no timeout of its own. - Each looping test carries a guard that the overlap actually happened, rather than one on how many times a flow reaches the store: some hold has to let the second flow through, and the first flow has to still have a write of its own to make afterwards. - The migration tests hold the loser on its first call into `ticket-branches`, not on a flag the loser itself sets, or the loser wins and there is no race left to test. </details> <details> <summary>Screenshots or recording</summary> Nothing on screen changes. The fix is in what is stored, and the values stored are the same ones as before. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01LEj32Dunw3y321YzBXNiWS --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
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.
Why
Update to latest trunk, run while a ticket is linked, finishes cleanly and the build succeeds. Unlink the ticket, and the site card on trunk shows Update incomplete: the code is new but the built assets are old, with Retry install & build and a red dot on Start build watch. Nothing is incomplete: the build ran minutes earlier and exited 0. Reproduced twice during the #408 Windows acceptance, on two sites; not platform-specific.
What changes
The root cause is that the flag is written and cleared in two different metadata scopes.
git:update-trunkparks the ticket onto trunk before it can update, so by the time it wroteupdateIncomplete: truethroughwriteWorkMeta— which resolves its scope from HEAD — HEAD said trunk and the write landed at site level. It then returns to the ticket, the renderer's build chain finishes there, andsites:mark-update-completeclears the branch's copy. Nothing ever cleared the site-level one, andreadWorkMetareads site level on trunk, so it surfaced the moment the contributor came back. The flag predates the per-branch work meta of #108 and the two were never reconciled.The correction stays inside the metadata scope, as the issue asks:
writeWorkMetaOn(sitePath, ref, patch)takes the branch rather than reading HEAD, and the update writes the flag onbranchBefore, the branch it is about to return to and the one the build will end on.writeWorkMetais now that function called with HEAD, so there is still one rule in one place.workMetaScope(sitePath, ref)answers where such a write will land, without writing. The update asks it once, before its writes, because everything else it does to the site record depends on the answer: a returning branch with no entry of its own reads and writes its work meta at site level, so for that branch the site record is the branch's, not trunk's, and nothing there is trunk's to correct.appliedPatch: nulldeliberately does not move with the flag. The park commits the worktree, applied hunks included, into the ticket's WIP commit, and the return checkout puts them back, so that patch is still in the work when the update ends. Only trunk's tree was reset, so only trunk's record goes.branches:rebasekeeps its record for the same reason, and "Save" puts my username on a patch when I just applied someone else's PR #328 reads it to refuse publishing another author's hunks as your own.The failure path is unchanged and still writes at HEAD: it does not return anyone to a ticket, it unlinks and leaves the contributor on trunk, which is where the flag will be read from. Its comment claimed the opposite and is corrected.
How to test this
A journey does it, on both platforms, and there is nothing left here worth doing by hand.
an update run from a linked ticket leaves no incomplete marker behind on trunkintests/e2e/journeys/trunk-update.spec.jsdrives the real app through exactly the sequence the issue reports: link a ticket, Update to latest trunk, let the chain finish, Unlink, and assert that trunk shows neither the Update incomplete banner nor Retry install & build. The last step is the one that reveals it — everything is quiet until the site is back on trunk, which is where the stale flag was read from.It is red against trunk's
src/main.js, on that assertion, and the journeys job already runs onwindows-latestas well asmacos-latestfor every non-draft PR, so the platform where this was found is covered by CI rather than by a person.That leaves the usual reason a green suite is not enough — a real
wordpress-developbuild, an OS dialog, a Windows path with a space — and none of it is involved in a metadata scope bug. If you want to see it with your own eyes anyway: link a ticket, update, wait forUpdated to the latest trunk, click Unlink, and the card on trunk should be clean.What must not have happened:
branches:rebase, and only when asked.Test evidence, each verified red against the commit it corrects rather than written after the fact: the layer-3 test the issue proposes (
git:update-trunk leaves no incomplete flag on trunk after the build finishes) red ontrunk; three of the four tests in046154dred onfe5b335; the patch-record test in04a8bbbred on046154d; the journey red ontrunk.Risks and limitations
Review outcome: 5
[fix here]· 3[follow-up]over three passes — all 5 fixed. Worth knowing when reading the commits separately: two of the four were the same mistake in two places, an applied-patch record being dropped for a patch that was still on disk, and neither was caught by the first pass's tests.migrateSiteToBranchescopiesupdateIncompleteonto the new branch entry but never removes the site-level copy, becausemergeSiteMetamerges rather than deletes. A pre-Working on a second ticket means rebuilding the world: tickets should be branches, not sites #108 site migrated mid-update carries a permanent site-leveltruethat shows the same false banner on trunk. The one-time clear added here reaches it only throughgit:update-trunk, which migrates on its way in; every other entry point into the migration still leaves the duplicate. Pre-existing and a different code path, so left as a follow-up rather than widened into this PR.wordpress-developsite. The journey drives the real app but against a fixture repository whose build script is a no-op, which is the right trade for a metadata bug and no trade at all for one that needs a real build. Buildkite has signed artifacts for the branch if anyone wants the real thing.#172(overlapping metadata writes) and#188are deliberately untouched: the electron-store race is a separate item on the v1.1.0 list.Related
Fixes #419. Found during the Windows acceptance of #408. #111 introduced the flag, #108 the per-branch work meta. The opt-in lane the docs change points at is #444's.
The last commit is a docs change that does not belong to the bug: TESTING.md gains the bar for what earns a place in the default journey suite, and AGENTS.md gains the consequence — a journey that drives the reported path can carry the How to test this section instead of a list of clicks. It ships here because this PR is the first instance of both, and without it this PR's own testing section contradicts AGENTS.md. Say so if you would rather see it on its own.
Design decisions and alternatives considered
The issue left the shape open: "either clear the flag where it was written, or write it on the branch the contributor will be on when the build ends."
Writing it on the destination branch was chosen because it leaves one scope holding the truth. Clearing at both scopes from
sites:mark-update-completewould have worked and would have self-healed stale flags on any successful build, but it keeps the flag being written to a place no reader trusts and reads as a workaround for a scope decision rather than a correction of it.The one-time site-level clear is the small concession to that alternative: it exists only so machines that already met the bug are not stuck, and it is gated on the live flag having gone elsewhere, so it cannot mask a real state.
writeWorkMetaOnreturns the scope it wrote to rather than the caller re-deriving it. The alternative was a secondworkMetaScopecall in the handler, which reads the store twice to answer the same question and can drift from what was actually written.Review outcome (required — see AGENTS.md)
5
[fix here]· 3[follow-up]over three passes — all 5 fixed, the follow-ups deferred with their reasons in Risks and limitations.Fixed:
fe5b335) —appliedPatch: nullmoved onto the ticket branch with the flag, deleting the record of a patch the park had carried through and the return checkout had restored. Split: the flag goes tobranchBefore, the patch record stays with trunk's reset tree.upToDaterun, could have a genuine incomplete state erased. Gated on the scope the write actually used.fe5b335.046154d) — the same blind spot the flag had just learned to guard was still open on the patch record beside it: for a returning branch with no entry of its own, site level is that branch's work meta, so the unconditional clear deleted the record of a patch that was about to be restored to disk. Both site-level corrections now run only when the site record really is trunk's. One test, red on046154d.04a8bbb) — a comment, but a load-bearing one on a failure path. Pass 2 had called a sentence there unreachable and it was removed on that basis; pass 3 showed the claim was wrong, sinceswitchToBranchtags its own checkout failures the same way andwithSwitchMarkerrethrows them intact, so a park that dies does reach that write with HEAD still on the ticket. Restored, and now says why reading HEAD is right for both failures.6da73ca.Deferred:
[follow-up]—migrateSiteToBranchesleaves the site-level copy behind. Pre-existing, a different path, the same class of symptom.[follow-up]— a real site-level flag from the failure path can be cleared with the stale ones. Narrow, self-correcting, and separating the two would need a second field; answered with a comment at the clear.[follow-up]— the branch-followingcurrentBranchNamestub is now repeated verbatim across seven tests. A local helper would not weaken any of them, but the repetition keeps each test readable on its own, which is the trade this file already makes elsewhere.Review rate limitedon this PR rather than producing a review. Recorded as not run, not as zero findings..github/instructions/code-review.instructions.mdstep 3, each with no knowledge of this session's reasoning. Pass 1 headfe5b335, pass 2 head046154d, pass 3 head04a8bbb; baseorigin/trunkdd8bc22throughout. Working tree clean at each pass; no uncommitted or untracked files in scope. Deterministic layer run by the author and reported to each pass:npm run lintclean,npm test1252 passing at head.fe5b335→046154d→04a8bbb, each re-reviewed in a fresh context rather than carried forward. Pass 3 verified the fix for pass 2's finding against an explicit six-row matrix of (returning branch is trunk or a ticket) x (branch entry present or absent) x (up to date or not), and raised finding 5 above.04a8bbb→6da73cais that finding's fix and changes one comment and nothing else: no fourth pass was run, and the reviewed code is unchanged between the two.6da73ca→c0e960eadds the journey and touches no source; unreviewed by a pass, and its own evidence is that it is red against trunk.Implementation notes
src/main.jsworkMetaScope(sitePath, ref)— the scope rule, extracted so it can be asked without writing. Returns the branch, or null for the site. A ref with no entry of its own resolves to the site: a pre-Working on a second ticket means rebuilding the world: tickets should be branches, not sites #108 site, one whose migration could not run, or a branch the contributor's own Git client checked out. That is the same rulereadWorkMetareads by, which is what makes the answer usable.writeWorkMetaOn(sitePath, ref, patch)— the write, against a named branch.writeWorkMetais now this called with HEAD.git:update-trunksuccess path —workMetaScopeonce, the flag throughwriteWorkMetaOn(sitePath, branchBefore, …)unless the run was already up to date, and then the site-level corrections only underbranchBefore === TRUNK || workScope, which is the condition that says the site record is trunk's own.tests/unit/ipc-wiring.test.cjsThe instrument that matters is the
currentBranchNamestub.git:update-trunk parks the ticket, updates, and returns to it (issue #108)stubbed it to a constant'ticket/59234', soactiveBranchnever observed the park and the site-level write was invisible to it — which is why that test asserted the right thing and passed anyway. It now follows the checkouts, as the new tests do.Screenshots or recording
Nothing on screen changed by design. What changes is that an existing banner stops appearing when it should not; the banner itself is untouched (
src/renderer/index.jsx:4389).🤖 Generated with Claude Code
https://claude.ai/code/session_015pjnEjcbyVZHWmhkoeCiaB