Skip to content

Commit 34029ef

Browse files
juanmaguitarclaude
andauthored
[Fix] Carry the applied-patch record onto the ticket with the work, Fixes #236 (#473)
## Why Apply a patch on trunk, link a ticket, and choose to take the loose edits along. The files move onto the ticket branch, because a carry is `branch` plus `checkout` and the worktree is left alone. The record describing them does not: it stays at site level, where a patch applied on trunk is written. So the two halves end in different places. Return to trunk and the panel names the patch, counts its files and offers to revert it, over a tree that holds none of it. Accept, and the revert fails blaming a trunk update or a discard that never happened. The claim is stored, so it greets the contributor every time they come back. Fixes #236. Reproduced on v1.1.0-beta.2 before writing anything; the steps and the store evidence are [on the issue](#236 (comment)). The detail that shaped this change is that the per-branch model is fine: a patch applied while already on a ticket is recorded against that branch. Only the carry from trunk fails to move the record, so this is one missing read-change-write, not a fault in the layer model. ## What changes `carryAppliedPatch` moves the record onto the branch the carry just took the files to, in a single `changeSiteMeta`, called from `sites:set-ticket` when the carry happened. The clear and the write are indivisible on purpose: split in two, a concurrent write leaves the record in both places or in neither, and neither is worse — it takes the patch text with it, which is what Revert reverses with. It runs after the branch's own meta is written, so the entry exists with its branch point rather than being created here holding one field (#172). A branch with no entry is left alone: `workMetaScope` makes such a ref read from the site anyway, so the record stays visible over the files that are there. Two things fall out that nobody had filed. A trunk update run from a carried ticket used to reach `mergeSiteMeta({ appliedPatch: null })` and destroy the only copy of the patch text while the patch was still in the branch's WIP commit; it now clears an already-null field and leaves the branch record alive. And `appliedPatchSubmissionRefusal` did not fire on the branch, so a carried patch's hunks could be submitted as the contributor's own work — the refusal #328 exists for now happens. ## How to test this Platforms: either macOS or Windows for the manual pass; any for the suites. The journey walks the reported path, so run it rather than clicking through it: ```sh npx playwright test --project=journeys tests/e2e/journeys/patch-apply.spec.js ``` `work carried into a ticket takes the applied-patch record with it (#236)` applies a patch on trunk, links a ticket taking the edits along, checks both halves arrived, then returns to trunk with **Unlink** and asserts nothing is offered over the clean tree. It was run red against `trunk`'s `src/main.js` and green with this change. The unit regressions, in `tests/unit/ipc-wiring.test.cjs`: ```sh node --test tests/unit/ipc-wiring.test.cjs ``` Three cases. The carry moves the whole record, patch text included. Declining the carry leaves it on trunk. And a patch recorded against the branch while the link was running is not overwritten by trunk's older one. **What must not have happened:** no record may exist in both places after a carry; the patch `text` must survive the move, or Revert loses what it reverses with; declining the carry must move nothing; and a patch applied on a ticket directly must still be recorded against that branch as before. ## Risks and limitations **Sites already in the split state are not repaired.** Trunk keeps its stale record on those until someone presses Revert. Deliberately out of scope: dropping a site-level record when trunk is clean is a behaviour change on existing installs and deserves its own decision, not a quiet ride-along with a bug fix. The move makes a carried patch revertible from the ticket, which is what the issue asks for and is a visible behaviour change: submitting that ticket is now refused while the layer is applied. ## Related Fixes #236. #356, which would have dissolved this bug by retiring the app-held layer, is closed as not planned, so the direct fix is the only one coming. --- <details> <summary>Review outcome (required — see AGENTS.md)</summary> **0 [fix here] · 3 [follow-up].** Fresh-context judgement pass against `origin/trunk`, dispatched to a subagent with the diff and `.github/instructions/code-review.instructions.md` and nothing else from the session that wrote the code. Deterministic layer: `npm run lint` clean; `npm test` 1454 pass, 0 fail, 2 expected skips; electron suite 1455 pass; all 30 journeys pass. The reviewer traced the carry path end to end and confirmed `branchRef` is the ref HEAD is on, so the record lands where `readWorkMeta`/`workMetaScope` look for it, and checked every consumer of the record for a regression: submission refusal, revert, both discards, trunk update, unlink, ticket switch, branch delete and the branch list. None regressed; two improved, as described above. Acted on here: - The race where a patch applied during the link is overwritten by the carry. Pre-fix it produced a phantom trunk record; post-fix it would have destroyed revert data, so it is guarded in this PR with its own regression test rather than deferred. - A cosmetic indentation inconsistency in the new helper. Deferred, as issues rather than as comments: - The pair `mergeBranchMeta` + `carryAppliedPatch` is two writes, so a quit landing between them persists the #236 state. The window is two microtasks with no I/O. Closing it means folding both into one `changeSiteMeta`, which reshapes the handler. - `updateIncomplete` is the same class of bug and does not travel with a carry either, leaving a false "Update incomplete" banner on trunk. Pre-existing and untouched by this change. </details> <details> <summary>Screenshots or recording</summary> The change removes something from the screen rather than adding one: after a carry, trunk no longer shows the applied-patch card. The journey asserts it by count. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_0121dia1RzfskutrzA8iSK1K --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 1c03422 commit 34029ef

3 files changed

Lines changed: 168 additions & 1 deletion

File tree

‎src/main.js‎

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1573,6 +1573,49 @@ async function changeWorkMetaOn(sitePath, ref, change) {
15731573
});
15741574
}
15751575

1576+
/**
1577+
* Moves the applied-patch record onto the branch a carry just took the files
1578+
* to (#236).
1579+
*
1580+
* A carry is `branch` plus `checkout`: HEAD moves and the worktree is left
1581+
* alone, so the edits ride along. The record describing them did not, and the
1582+
* two ended in different places — the changes on the ticket, the claim about
1583+
* them on trunk. Returning to trunk then met a panel naming a patch, counting
1584+
* its files and offering to revert it over a clean tree, and the revert failed
1585+
* blaming a trunk update that never happened.
1586+
*
1587+
* One `changeSiteMeta`, because the clear and the write have to be indivisible:
1588+
* split in two, a concurrent write leaves the record in both places or in
1589+
* neither, and neither is the worse of the two — it takes the patch text with
1590+
* it, which is what Revert reverses with.
1591+
*
1592+
* Called after the branch's own meta is written, so the entry exists with its
1593+
* branch point rather than being created here holding this one field (#172).
1594+
*
1595+
* @param {string} sitePath
1596+
* @param {string} ref The branch the work was carried onto.
1597+
*/
1598+
async function carryAppliedPatch(sitePath, ref) {
1599+
await changeSiteMeta(sitePath, (m) => {
1600+
const carried = m.appliedPatch;
1601+
if (!carried) return m;
1602+
const branches = { ...(m.branches || {}) };
1603+
// No entry means nowhere per-branch for it to live, and the reader
1604+
// would look at the site for it anyway (workMetaScope). Leaving it on
1605+
// trunk is wrong but visible; moving it somewhere unread is not.
1606+
if (!branches[ref]) return m;
1607+
// A patch applied while this link was still running belongs to the
1608+
// branch and is newer. Overwriting it with trunk's would drop the text
1609+
// Revert reverses with, leaving a patch on disk the app cannot undo —
1610+
// the same loss this function exists to prevent, in the other
1611+
// direction. Trunk's record is left where it is, visibly wrong, rather
1612+
// than taking a newer one down with it.
1613+
if (branches[ref].appliedPatch) return m;
1614+
branches[ref] = { ...branches[ref], appliedPatch: carried };
1615+
return { ...m, appliedPatch: null, branches };
1616+
});
1617+
}
1618+
15761619
async function writeWorkMeta(sitePath, patch) {
15771620
const { ref } = await activeBranch(sitePath);
15781621
return writeWorkMetaOn(sitePath, ref, patch);
@@ -2882,7 +2925,12 @@ ipcMain.handle('sites:set-ticket', async (event, sitePath, ref, options) => with
28822925
lastUsedAt: new Date().toISOString()
28832926
});
28842927
await mergeSiteMeta(sitePath, { tracTicket: parsed.id, currentBranch: checkoutRef });
2885-
if (carriedFrom === TRUNK) reportCarriedWork(event, sitePath, parsed.id);
2928+
if (carriedFrom === TRUNK) {
2929+
// The record travels with the files (#236), before the notice that
2930+
// says they arrived.
2931+
await carryAppliedPatch(sitePath, branchRef);
2932+
reportCarriedWork(event, sitePath, parsed.id);
2933+
}
28862934
return { ok: true, ticket: parsed.id, branch: checkoutRef, ...impact };
28872935
}));
28882936

‎tests/e2e/journeys/patch-apply.spec.js‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,3 +232,49 @@ new file mode 100644
232232
const meta = session.readSettings().siteMeta[ site.dir ];
233233
expect( meta.branches[ 'ticket/60001' ].appliedPatch ).toBeFalsy();
234234
} );
235+
236+
test( 'work carried into a ticket takes the applied-patch record with it (#236)', async ( { session } ) => {
237+
const site = await makeSite( session );
238+
const { page } = await session.start( site.settings );
239+
240+
// Applied on trunk, before any ticket is known — the order that produced
241+
// the bug. The edits are loose on the site, and so is the record.
242+
const patch = makePatchFile( session, 'ticket-60001.patch', [
243+
{ file: 'wp-login.php', from: TRUNK_LOGIN, to: PATCHED_LOGIN },
244+
] );
245+
await applyPatchFile( session, patch );
246+
await expect( page.getByRole( 'button', { name: 'Revert this patch', exact: true } ) ).toBeVisible( {
247+
timeout: 60_000,
248+
} );
249+
250+
// Linking a ticket now asks what happens to the loose work (#234). Taking
251+
// it along is the path under test.
252+
await page.getByLabel( 'Trac ticket number or URL' ).first().fill( '60001' );
253+
await page.getByRole( 'button', { name: 'Link ticket', exact: true } ).first().click();
254+
await page.getByRole( 'button', { name: 'Take these edits into #60001', exact: true } ).click();
255+
await expect( page.getByText( '#60001', { exact: true } ).first() ).toBeVisible( { timeout: 30_000 } );
256+
257+
// INVARIANT — the files came along.
258+
expect( read( site.dir, LOGIN ) ).toBe( `${ PATCHED_LOGIN }\n` );
259+
260+
// INVARIANT — and so did the claim about them. Split, this is the bug: the
261+
// changes on the ticket, the record on trunk.
262+
const carried = session.readSettings().siteMeta[ site.dir ];
263+
expect( carried.branches[ 'ticket/60001' ].appliedPatch.label ).toBe( 'ticket-60001.patch' );
264+
expect( carried.appliedPatch ).toBeFalsy();
265+
266+
// INVARIANT — and the offer to undo is still on screen, against the branch
267+
// that now holds both halves.
268+
await expect( page.getByRole( 'button', { name: 'Revert this patch', exact: true } ) ).toBeVisible( {
269+
timeout: 60_000,
270+
} );
271+
272+
// The moment the report was about: back on trunk, over a tree that holds
273+
// none of it, the app used to name the patch, count its files and offer to
274+
// revert it.
275+
await page.getByRole( 'button', { name: 'Unlink', exact: true } ).click();
276+
await expect( page.getByRole( 'button', { name: 'Revert this patch', exact: true } ) ).toHaveCount( 0, {
277+
timeout: 30_000,
278+
} );
279+
expect( gitOk( [ 'status', '--porcelain' ], site.dir ).trim() ).toBe( '' );
280+
} );

‎tests/unit/ipc-wiring.test.cjs‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4484,6 +4484,79 @@ test('sites:set-ticket says how much loose work the chosen carry took into a new
44844484
assert.match(fs.readFileSync(path.join(dir, 'wp-login.php'), 'utf8'), /before the ticket was known/);
44854485
});
44864486

4487+
// The record belongs with the work (#236). A carry moves the files onto the
4488+
// branch; leaving the applied-patch record behind on trunk is what made the
4489+
// panel greet a returning contributor with a patch it named, counted and
4490+
// offered to revert, over a tree that no longer held any of it — and the
4491+
// revert then blamed a trunk update that never happened.
4492+
test('the carry takes the applied-patch record onto the ticket with the files (issue #236)', async (t) => {
4493+
const dir = dirtyTrunkFixture(t, 'ipc-wiring-carry-patch-');
4494+
const applied = {
4495+
label: '62281.diff',
4496+
appliedAt: '2026-09-15T08:22:44.741Z',
4497+
files: ['wp-login.php'],
4498+
text: 'diff --git a/wp-login.php b/wp-login.php\n'
4499+
};
4500+
const settings = fakeSettingsStore({ sites: [dir], siteMeta: { [dir]: { appliedPatch: applied } } });
4501+
const main = loadMain({ stubs: { ...silentLogging(), ...settings.stubs } });
4502+
4503+
const event = createIpcEvent();
4504+
assert.equal((await main.invokeWith('sites:set-ticket', event, dir, '62281', { carryTrunkWork: true })).ok, true);
4505+
// The count runs after the handler answers, on a Git child of its own.
4506+
// Waiting for its notice is what says that child is done — on Windows an
4507+
// unfinished one holds the fixture directory open and the teardown cannot
4508+
// delete it.
4509+
assert.ok(await carriedWork(event), 'the carry finished and said so');
4510+
4511+
const meta = settings.values.siteMeta[dir];
4512+
assert.deepEqual(
4513+
meta.branches['ticket/62281'].appliedPatch,
4514+
applied,
4515+
'the whole record travels, the patch text included — it is what Revert reverses with'
4516+
);
4517+
assert.ok(!meta.appliedPatch, 'and trunk stops claiming a patch whose files it no longer holds');
4518+
});
4519+
4520+
// The window between creating the branch and moving the record: a patch
4521+
// applied in it is recorded against the branch and is the newer of the two.
4522+
// Overwriting it would drop the text Revert reverses with, leaving a patch on
4523+
// disk the app cannot undo — the loss the carry exists to prevent, arriving
4524+
// from the other side.
4525+
test('the carry does not overwrite a patch recorded against the branch while it ran (issue #236)', async (t) => {
4526+
const dir = dirtyTrunkFixture(t, 'ipc-wiring-carry-patch-race-');
4527+
const fromTrunk = { label: 'older.diff', appliedAt: '2026-09-15T08:00:00.000Z', files: ['wp-login.php'], text: 'older\n' };
4528+
const onBranch = { label: 'newer.diff', appliedAt: '2026-09-15T08:30:00.000Z', files: ['wp-login.php'], text: 'newer\n' };
4529+
const settings = fakeSettingsStore({
4530+
sites: [dir],
4531+
siteMeta: { [dir]: { appliedPatch: fromTrunk, branches: { 'ticket/62281': { appliedPatch: onBranch } } } }
4532+
});
4533+
const main = loadMain({ stubs: { ...silentLogging(), ...settings.stubs } });
4534+
4535+
const event = createIpcEvent();
4536+
assert.equal((await main.invokeWith('sites:set-ticket', event, dir, '62281', { carryTrunkWork: true })).ok, true);
4537+
assert.ok(await carriedWork(event), 'the carry finished and said so — see the note in the test above');
4538+
4539+
const meta = settings.values.siteMeta[dir];
4540+
assert.deepEqual(meta.branches['ticket/62281'].appliedPatch, onBranch, 'the newer record survives');
4541+
assert.deepEqual(meta.appliedPatch, fromTrunk, 'and the older one is left where it is rather than taken down with it');
4542+
});
4543+
4544+
// The other half of the same rule: a carry the contributor declined moves
4545+
// nothing, so a patch applied on trunk stays trunk's. Without this the fix
4546+
// above could satisfy its test by clearing the record unconditionally.
4547+
test('declining the carry leaves the applied-patch record on trunk (issue #236)', async (t) => {
4548+
const dir = dirtyTrunkFixture(t, 'ipc-wiring-carry-patch-declined-');
4549+
const applied = { label: '62281.diff', appliedAt: '2026-09-15T08:22:44.741Z', files: ['wp-login.php'], text: 'diff\n' };
4550+
const settings = fakeSettingsStore({ sites: [dir], siteMeta: { [dir]: { appliedPatch: applied } } });
4551+
const main = loadMain({ stubs: { ...silentLogging(), ...settings.stubs } });
4552+
4553+
const event = createIpcEvent();
4554+
const result = await main.invokeWith('sites:set-ticket', event, dir, '62281');
4555+
4556+
assert.equal(result.code, 'dirty-trunk', 'the gesture asks rather than moving anything');
4557+
assert.deepEqual(settings.values.siteMeta[dir].appliedPatch, applied, 'nothing moved, so the record did not either');
4558+
});
4559+
44874560
test('nothing loose means nothing is claimed (issue #108)', async (t) => {
44884561
const dir = adoptedRepo(t, 'ipc-wiring-carry-clean-');
44894562
fs.writeFileSync(path.join(dir, 'wp-login.php'), '<?php // trunk\n');

0 commit comments

Comments
 (0)