From c03c4630a68fdbf7bb107c8e1a9e28032b0a12ee Mon Sep 17 00:00:00 2001 From: J0UH <217747670+J0UH@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:52:26 +0200 Subject: [PATCH 1/2] Serialize the orchestrate store's stale-lock takeover Two writers that both found the store lock naming a dead pid could both take it. Each read the dead holder, then ran unlink and an exclusive create. When the second writer's unlink ran after the first writer's create, it deleted the first writer's live lock and created its own, so both held the lock and their read-modify-write of units.tsv or ledger.tsv could drop rows. A takeover now runs behind a claim: a directory, .orch.lock.takeover, holding one file named for the claimant's pid. The claimant stages that directory and renames it into place. rename cannot replace a directory that holds a file, so only one writer claims at a time and the others fail with "being replaced by another writer; retry". Under the claim, the writer reads the lock again and unlinks it only if it still names the holder it judged replaceable. A claim left by a dead writer is removed by its pid-named file, which cannot remove a live claimant's. The forced takeover goes through the same claim. store.ts and orch.test.ts are now declared forks in tools/forks.json. --- CONTRIBUTING.md | 2 +- .../poteto-mode/scripts/orch/orch.test.ts | 56 ++++++++++++++- .../skills/poteto-mode/scripts/orch/store.ts | 68 ++++++++++++++++--- tools/forks.json | 12 ++++ 4 files changed, 125 insertions(+), 13 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 743e261a..dc51ef7d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -9,7 +9,7 @@ Thanks for helping out. This repo is a **port**, not an original work: the skill Upstream owns the skill text. This port owns the rewrite of upstream's Cursor mechanics into their T3 form, and nothing else. The rewrite has two parts: - **Substitutions.** [`tools/substitutions.json`](tools/substitutions.json) rewrites each upstream sentence that names a Cursor mechanism: the `Task` tool, `subagent_type`, model slugs, the `pstack-models.mdc` rule, cloud agents, `/loop`, Cursor paths. Most T3 behavior the port adds, such as the dispatch ledger or the review loop, is an anchored insert: a rule whose pattern is an upstream sentence and whose replacement keeps that sentence and appends the T3 text. A substitution leaves upstream's later edits elsewhere in the file mechanical, so the port prefers it to a fork. Each rule's `rationale` names why it exists, and most cite the section of t3-pstack 1.0.14's `t3-tools.md` it replaces. -- **Forks.** [`tools/forks.json`](tools/forks.json) declares each file the port keeps in its own form, with a `kind`, a one-sentence `why`, the `since` release, and an `upstream` status. Today that is setup-pstack, which writes the T3 model sheet, the hook points of the watch-pr settle overlay, the files that carry the rules and watch-pr fixes restored from Michael Denyer's pstack-claude, and the worktree audit and cleanup playbook, which fix [cursor/plugins#449](https://github.com/cursor/plugins/issues/449). Every upstream change to a forked file needs a person to merge it, so a fork is the last resort. +- **Forks.** [`tools/forks.json`](tools/forks.json) declares each file the port keeps in its own form, with a `kind`, a one-sentence `why`, the `since` release, and an `upstream` status. Today that is setup-pstack, which writes the T3 model sheet, the hook points of the watch-pr settle overlay, the files that carry the rules and watch-pr fixes restored from Michael Denyer's pstack-claude, the worktree audit and cleanup playbook, which fix [cursor/plugins#449](https://github.com/cursor/plugins/issues/449), and the orchestrate store's lock takeover. Every upstream change to a forked file needs a person to merge it, so a fork is the last resort. The port also owns files upstream does not have: the plugin manifests, the session-start hooks and their routing text, `models.json`, the sheet checker `t3-sheet.mjs`, the watch-pr settle overlay (`settle.ts`), and the transcript lookups reflect uses. The sync lists them as port-only. diff --git a/plugins/pstack/skills/poteto-mode/scripts/orch/orch.test.ts b/plugins/pstack/skills/poteto-mode/scripts/orch/orch.test.ts index 2ef435d7..05d6dfb7 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/orch/orch.test.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/orch/orch.test.ts @@ -8,7 +8,7 @@ import { rm, writeFile, } from "node:fs/promises"; -import { realpathSync } from "node:fs"; +import { realpathSync, unlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { @@ -436,6 +436,60 @@ describe("Store", () => { expect(await readdir(directory)).not.toContain(".orch.lock"); }); + it("keeps a lock another writer took over from the same dead holder", async () => { + const { directory } = await initializedStore(); + const lock = join(directory, ".orch.lock"); + const exited = Bun.spawn(["true"]); + await exited.exited; + await writeFile(lock, `${exited.pid}\n`); + const rival = Bun.spawn(["sleep", "30"]); + + try { + const late = useStore(directory, { + onStaleLock: () => { + unlinkSync(lock); + writeFileSync(lock, `${rival.pid}\n`, { flag: "wx" }); + }, + }); + await expect( + late.units.add({ id: "u1", track: "build" }) + ).rejects.toThrow(`store lock held by pid ${rival.pid}`); + expect(await readFile(lock, "utf8")).toBe(`${rival.pid}\n`); + } finally { + rival.kill(); + await rival.exited; + } + }); + + it("serializes stale-lock takeovers behind a claim a dead writer cannot hold", async () => { + const { directory } = await initializedStore(); + const exited = Bun.spawn(["true"]); + await exited.exited; + await writeFile(join(directory, ".orch.lock"), `${exited.pid}\n`); + const claim = join(directory, ".orch.lock.takeover"); + await mkdir(claim); + const claimant = Bun.spawn(["sleep", "30"]); + await writeFile(join(claim, String(claimant.pid)), ""); + + try { + await expect( + useStore(directory).units.add({ id: "u1", track: "build" }) + ).rejects.toThrow( + `store lock held by pid ${exited.pid} is being replaced by another writer; retry` + ); + } finally { + claimant.kill(); + await claimant.exited; + } + + const recovered = useStore(directory); + expect( + await recovered.units.add({ id: "u1", track: "build" }) + ).toMatchObject({ id: "u1" }); + expect(await readdir(directory)).not.toContain(".orch.lock.takeover"); + await recovered.close(); + }); + it("blocks a writer and steals the pid lock only with force", async () => { const { directory, store } = await initializedStore(); await store.close(); diff --git a/plugins/pstack/skills/poteto-mode/scripts/orch/store.ts b/plugins/pstack/skills/poteto-mode/scripts/orch/store.ts index 41736891..24890762 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/orch/store.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/orch/store.ts @@ -17,6 +17,7 @@ import { basename, dirname, join, resolve } from "node:path"; const UNIT_HEADER = "id\ttrack\tstate\tbranch\tpr\tsha\tbrief"; const LEDGER_HEADER = "pr\tsha\tverdict\tevidence\tverifier\tts"; const LOCK_FILE = ".orch.lock"; +const TAKEOVER_DIR = ".orch.lock.takeover"; export type Verdict = | "live-ui-verified" @@ -384,6 +385,7 @@ async function acquireLock( options: OpenStoreOptions ): Promise<() => Promise> { const path = join(store, LOCK_FILE); + const takeover = join(store, TAKEOVER_DIR); const pid = String(process.pid); const create = async (): Promise => { const handle = await open(path, "wx"); @@ -391,17 +393,61 @@ async function acquireLock( await handle.close(); }; - const takeOver = async (): Promise => { - await unlink(path); + // POSIX cannot unlink a file only if it still names the holder judged + // replaceable, so the read and unlink run behind a claim: a directory that + // holds one file named for the claimant's pid. rename cannot replace a + // directory that holds a file, so one writer takes over at a time, and a + // dead claimant's file is removed by name, which cannot remove a live one's. + const takeOver = async (holder: string): Promise => { + const staged = `${takeover}.${pid}.${randomUUID()}`; + await mkdir(staged); try { - await create(); - } catch (retryError) { - if (errorCode(retryError) === "EEXIST") { - const retryHolder = - (await readFile(path, "utf8")).trim() || "unknown"; - throw new UserError(`store lock held by pid ${retryHolder}`); + await writeFile(join(staged, pid), ""); + for (const claimant of await readdir(takeover).catch(() => [])) { + if (holderIsDead(claimant)) { + await rm(join(takeover, claimant), { force: true }); + } + } + try { + await rename(staged, takeover); + } catch (error) { + const code = errorCode(error); + if (code === "ENOTEMPTY" || code === "EEXIST") { + throw new UserError( + `store lock held by pid ${holder} is being replaced by another writer; retry` + ); + } + throw error; + } + try { + const current = await readFile(path, "utf8").then( + (text) => text.trim() || "unknown", + (error: unknown) => { + if (errorCode(error) === "ENOENT") { + return null; + } + throw error; + } + ); + if (current !== null) { + if (current !== holder) { + throw new UserError(`store lock held by pid ${current}`); + } + await unlink(path); + } + await create(); + } catch (retryError) { + if (errorCode(retryError) === "EEXIST") { + const retryHolder = + (await readFile(path, "utf8")).trim() || "unknown"; + throw new UserError(`store lock held by pid ${retryHolder}`); + } + throw retryError; + } finally { + await rename(takeover, staged); } - throw retryError; + } finally { + await rm(staged, { recursive: true, force: true }); } }; @@ -419,10 +465,10 @@ async function acquireLock( } if (holderIsDead(holder)) { options.onStaleLock?.(holder); - await takeOver(); + await takeOver(holder); } else if (options.force) { options.onLockStolen?.(holder); - await takeOver(); + await takeOver(holder); } else { throw new UserError(`store lock held by pid ${holder}`); } diff --git a/tools/forks.json b/tools/forks.json index 086fa801..734f4aec 100644 --- a/tools/forks.json +++ b/tools/forks.json @@ -30,6 +30,18 @@ "since": "2.0.0-rc.7", "upstream": "https://github.com/cursor/plugins/issues/449" }, + "plugins/pstack/skills/poteto-mode/scripts/orch/orch.test.ts": { + "kind": "policy", + "why": "Tests the serialized lock takeover that store.ts adds.", + "since": "2.0.0-rc.16", + "upstream": "not-proposed" + }, + "plugins/pstack/skills/poteto-mode/scripts/orch/store.ts": { + "kind": "policy", + "why": "Runs each stale or forced lock takeover behind a claim directory named for the claimant's pid, so two writers that both read a dead holder cannot both acquire the store lock.", + "since": "2.0.0-rc.16", + "upstream": "not-proposed" + }, "plugins/pstack/skills/poteto-mode/scripts/watch-pr/cli.test.ts": { "kind": "port-feature", "why": "Expects the --settle default that cli.ts adds.", From fa110aacd36ee08329633e87c9a1fad9c3f41814 Mon Sep 17 00:00:00 2001 From: J0UH <217747670+J0UH@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:52:36 +0200 Subject: [PATCH 2/2] Resolve a reused branch to its open pull request in orch frontier set orch frontier set lists each branch's pull requests with --state all, because a merged or closed PR still anchors the chain below the current branch. A branch that had a merged or closed PR and now has a new open one returned all of them, and frontier resolution failed with "has multiple pull requests (#11, #9, #8); close the stale ones first". Closing does not help, since the old PRs are already closed or merged. When a branch has an open PR, that PR is the branch's current one, and its merged and closed PRs are ignored. Two open PRs on one branch still fail, and the error now names only the open ones. A branch with no open PR keeps the old rule: exactly one PR. --- CONTRIBUTING.md | 2 +- .../poteto-mode/scripts/orch/orch.test.ts | 59 +++++++++++++++++++ .../skills/poteto-mode/scripts/orch/store.ts | 10 +++- tools/forks.json | 4 +- 4 files changed, 69 insertions(+), 6 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index dc51ef7d..75cdf762 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -9,7 +9,7 @@ Thanks for helping out. This repo is a **port**, not an original work: the skill Upstream owns the skill text. This port owns the rewrite of upstream's Cursor mechanics into their T3 form, and nothing else. The rewrite has two parts: - **Substitutions.** [`tools/substitutions.json`](tools/substitutions.json) rewrites each upstream sentence that names a Cursor mechanism: the `Task` tool, `subagent_type`, model slugs, the `pstack-models.mdc` rule, cloud agents, `/loop`, Cursor paths. Most T3 behavior the port adds, such as the dispatch ledger or the review loop, is an anchored insert: a rule whose pattern is an upstream sentence and whose replacement keeps that sentence and appends the T3 text. A substitution leaves upstream's later edits elsewhere in the file mechanical, so the port prefers it to a fork. Each rule's `rationale` names why it exists, and most cite the section of t3-pstack 1.0.14's `t3-tools.md` it replaces. -- **Forks.** [`tools/forks.json`](tools/forks.json) declares each file the port keeps in its own form, with a `kind`, a one-sentence `why`, the `since` release, and an `upstream` status. Today that is setup-pstack, which writes the T3 model sheet, the hook points of the watch-pr settle overlay, the files that carry the rules and watch-pr fixes restored from Michael Denyer's pstack-claude, the worktree audit and cleanup playbook, which fix [cursor/plugins#449](https://github.com/cursor/plugins/issues/449), and the orchestrate store's lock takeover. Every upstream change to a forked file needs a person to merge it, so a fork is the last resort. +- **Forks.** [`tools/forks.json`](tools/forks.json) declares each file the port keeps in its own form, with a `kind`, a one-sentence `why`, the `since` release, and an `upstream` status. Today that is setup-pstack, which writes the T3 model sheet, the hook points of the watch-pr settle overlay, the files that carry the rules and watch-pr fixes restored from Michael Denyer's pstack-claude, the worktree audit and cleanup playbook, which fix [cursor/plugins#449](https://github.com/cursor/plugins/issues/449), and the orchestrate store, whose lock takeover and reused-branch lookup carry fixes. Every upstream change to a forked file needs a person to merge it, so a fork is the last resort. The port also owns files upstream does not have: the plugin manifests, the session-start hooks and their routing text, `models.json`, the sheet checker `t3-sheet.mjs`, the watch-pr settle overlay (`settle.ts`), and the transcript lookups reflect uses. The sync lists them as port-only. diff --git a/plugins/pstack/skills/poteto-mode/scripts/orch/orch.test.ts b/plugins/pstack/skills/poteto-mode/scripts/orch/orch.test.ts index 05d6dfb7..8027bc26 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/orch/orch.test.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/orch/orch.test.ts @@ -627,6 +627,65 @@ describe("Store", () => { }); }); + it(`resolves a ${forge} branch reused after a merged and a closed PR to its open PR`, async () => { + const { directory, store } = await initializedStore(); + const stack = await makeGitStack(directory); + + await withFakeForge({ + directory, + forge, + currentRows: forgeRows(forge, [ + { + number: 11, + state: "open", + head: "stack/open", + base: "stack/closed", + }, + { number: 9, state: "closed", head: "stack/open", base: "main" }, + { number: 8, state: "merged", head: "stack/open", base: "main" }, + ]), + operation: async () => { + expect( + (await store.frontier.set({ repo: stack.repo, forge })).prs.map( + (row) => row.pr + ) + ).toEqual([10, 13, 11, 14]); + }, + }); + }); + + it(`rejects a ${forge} branch with more than one open PR`, async () => { + const { directory, store } = await initializedStore(); + const stack = await makeGitStack(directory); + + await withFakeForge({ + directory, + forge, + currentRows: forgeRows(forge, [ + { + number: 12, + state: "draft", + head: "stack/open", + base: "stack/closed", + }, + { + number: 11, + state: "open", + head: "stack/open", + base: "stack/closed", + }, + { number: 9, state: "closed", head: "stack/open", base: "main" }, + ]), + operation: async () => { + await expect( + store.frontier.set({ repo: stack.repo, forge }) + ).rejects.toThrow( + "branch stack/open has multiple pull requests (#12, #11); close the stale ones first" + ); + }, + }); + }); + it(`rejects unparseable ${forge} output loudly`, async () => { const { directory, store } = await initializedStore(); const stack = await makeGitStack(directory); diff --git a/plugins/pstack/skills/poteto-mode/scripts/orch/store.ts b/plugins/pstack/skills/poteto-mode/scripts/orch/store.ts index 24890762..dad6b0e5 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/orch/store.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/orch/store.ts @@ -1165,12 +1165,16 @@ function branchPullRequest({ `branch ${branch} has no pull request; open one with ${forge} pr create, or resolve the frontier from a branch whose chain is fully submitted` ); } - if (rows.length > 1) { + // A reused branch keeps its merged and closed PRs, so an open PR is the + // branch's current one. + const open = rows.filter((item) => item.state === "OPEN"); + const current = open.length > 0 ? open : rows; + if (current.length > 1) { throw new UserError( - `branch ${branch} has multiple pull requests (${rows.map((item) => `#${item.pr}`).join(", ")}); close the stale ones first` + `branch ${branch} has multiple pull requests (${current.map((item) => `#${item.pr}`).join(", ")}); close the stale ones first` ); } - return row; + return current[0]; } function openChildren({ diff --git a/tools/forks.json b/tools/forks.json index 734f4aec..e0a879b3 100644 --- a/tools/forks.json +++ b/tools/forks.json @@ -32,13 +32,13 @@ }, "plugins/pstack/skills/poteto-mode/scripts/orch/orch.test.ts": { "kind": "policy", - "why": "Tests the serialized lock takeover that store.ts adds.", + "why": "Tests the serialized lock takeover and the reused-branch resolution that store.ts adds.", "since": "2.0.0-rc.16", "upstream": "not-proposed" }, "plugins/pstack/skills/poteto-mode/scripts/orch/store.ts": { "kind": "policy", - "why": "Runs each stale or forced lock takeover behind a claim directory named for the claimant's pid, so two writers that both read a dead holder cannot both acquire the store lock.", + "why": "Runs each stale or forced lock takeover behind a claim directory named for the claimant's pid, so two writers that both read a dead holder cannot both acquire the store lock, and resolves a branch with one open pull request to that one when the branch also has merged or closed pull requests from earlier use.", "since": "2.0.0-rc.16", "upstream": "not-proposed" },