Skip to content

Commit 2d94ccd

Browse files
committed
One live-PR predicate, a pinned test store, and a spec correction
- repo_service: extract `isLivePr` (a type guard, so it narrows) — two sites encoded "exists && OPEN && not stale" independently, and a change to one would silently diverge from the other. - target_rules fixture: pin MAKIT_WORKTREE_TARGETS_FILE. `worktreeTargetsFile()` prefers it over MAKIT_HOME, so a value inherited from another suite would point these tests at a shared store. - SPEC-51: the wire aliases read `||`, not `??` (an empty string must fall through), and the wrapUp fallback is `defaultBranchFor()` post-merge.
1 parent c6a702a commit 2d94ccd

3 files changed

Lines changed: 27 additions & 4 deletions

File tree

docs/specs/2026-08-11-SPEC-51-target-branch.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,10 +67,10 @@ A winner equal to the worktree's own branch is discarded and resolution continue
6767

6868
**Rule 1 — one vocabulary.** `base``target` across server and app. Two documented exceptions:
6969
`baseRefName` (GitHub's own field, mirrored from their API) and two **one-release** wire aliases —
70-
`worktree.create` and `worktree.wrapUp` read `env.targetBranch ?? env.baseBranch`, the app sends
70+
`worktree.create` and `worktree.wrapUp` read `env.targetBranch || env.baseBranch`, the app sends
7171
both keys, and `WrapUpReport.fromJson` reads both sets. The wrapUp alias is load-bearing, not
7272
cosmetic: a client that predates the rename would otherwise send a key the server ignores, the
73-
manager's `?? detectDefaultBranch()` fallback would fast-forward the **wrong branch**, and the ack
73+
manager's `?? defaultBranchFor()` fallback would fast-forward the **wrong branch**, and the ack
7474
would report success. Guarded by a test that fails the day someone deletes the alias without
7575
shipping the app first.
7676

server/src/repo_service.ts

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,22 @@ const PR_CONCURRENCY = 6;
7676
* result to add PR info without redoing the git work — so the numbers never
7777
* wait on the network.
7878
*/
79+
/**
80+
* Whether a cached pull request is **authoritative** about where its branch
81+
* lands: it exists, it is OPEN, and it is fresh.
82+
*
83+
* `stale` matters as much as the state. `enrichPrs` deliberately retains the
84+
* last-known PR when a lookup could not complete, so during a GitHub outage a
85+
* stale record would otherwise let an unverified — possibly closed or
86+
* since-retargeted — base overwrite a target the user just chose.
87+
*
88+
* One predicate, because two sites encoded it independently and a change to one
89+
* silently diverged from the other.
90+
*/
91+
function isLivePr(pr: PullRequestDTO | null): pr is PullRequestDTO {
92+
return pr !== null && !pr.stale && pr.state?.toUpperCase() === "OPEN";
93+
}
94+
7995
/**
8096
* Persist the base of any **live** pull request that disagrees with what we have
8197
* stored, and return the effective map.
@@ -112,7 +128,7 @@ function adoptLivePrTargets(
112128
// retained by `enrichPrs` when a lookup could not complete; adopting its base
113129
// during a GitHub outage could overwrite a freshly-chosen user target with an
114130
// unverified — possibly closed or since-retargeted — base.
115-
if (!pr || pr.stale || pr.state?.toUpperCase() !== "OPEN") continue;
131+
if (!isLivePr(pr)) continue;
116132
const base = pr.baseRefName;
117133
// A PR cannot land in its own head branch; treat that as bad data rather than
118134
// persisting a self-target we would then have to discard on every read.
@@ -217,7 +233,7 @@ async function repairVanishedTargets(
217233
for (const e of entries) {
218234
if (!e.branch) continue;
219235
const pr = lastKnown(repoPath, e.branch);
220-
if (pr && !pr.stale && pr.state?.toUpperCase() === "OPEN" && pr.baseRefName) {
236+
if (isLivePr(pr) && pr.baseRefName) {
221237
live.add(pr.baseRefName);
222238
}
223239
}

server/src/target_rules.test.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,11 @@ async function fixture(): Promise<Fixture> {
3838
const prevWt = process.env.MAKIT_WORKTREE_DIR;
3939
process.env.MAKIT_HOME = home;
4040
process.env.MAKIT_WORKTREE_DIR = wtDir;
41+
// Pin the targets file explicitly. `worktreeTargetsFile()` prefers
42+
// MAKIT_WORKTREE_TARGETS_FILE over MAKIT_HOME, so an inherited value from
43+
// another suite would silently point these tests at a shared store.
44+
const prevTargets = process.env.MAKIT_WORKTREE_TARGETS_FILE;
45+
process.env.MAKIT_WORKTREE_TARGETS_FILE = join(home, "worktree-targets.json");
4146

4247
const g = (cwd: string, ...args: string[]) => execFileSync("git", args, { cwd });
4348
g(repo, "init", "-q", "-b", "main");
@@ -58,6 +63,8 @@ async function fixture(): Promise<Fixture> {
5863
else process.env.MAKIT_HOME = prevHome;
5964
if (prevWt === undefined) delete process.env.MAKIT_WORKTREE_DIR;
6065
else process.env.MAKIT_WORKTREE_DIR = prevWt;
66+
if (prevTargets === undefined) delete process.env.MAKIT_WORKTREE_TARGETS_FILE;
67+
else process.env.MAKIT_WORKTREE_TARGETS_FILE = prevTargets;
6168
rmSync(home, { recursive: true, force: true });
6269
rmSync(repo, { recursive: true, force: true });
6370
rmSync(wtDir, { recursive: true, force: true });

0 commit comments

Comments
 (0)