fix(FN-branch-group): address fourth-round PR review feedback (#1357)

- needsPrRepair no longer short-circuited by the open-state guard: legacy
  fallback rows (finalized + prState open + prNumber null) now repair by
  creating the real PR on re-promotion; regression test added
- no-PR abandon route test asserts last persisted call + response body
- goal-provenance fallback test clears missionId on its own in-memory store
  so the feature-linkage path is genuinely exercised
This commit is contained in:
gsxdsm
2026-06-03 17:42:43 -07:00
parent 64d02b5108
commit 0be074f8c2
4 changed files with 45 additions and 3 deletions

View File

@@ -2018,7 +2018,12 @@ describe("MissionStore", () => {
const task = await ts.createTask({ title: "Task", description: "Linked task" });
ms.linkFeatureToTask(feature.id, task.id);
db.prepare("UPDATE tasks SET missionId = NULL WHERE id = ?").run(task.id);
// Clear missionId on THIS test's in-memory TaskStore db (the outer `db`
// belongs to a different store) so the lookup genuinely exercises the
// feature-linkage fallback instead of the normal task→mission path.
(ts as unknown as { db: { prepare(sql: string): { run(...args: unknown[]): unknown } } }).db
.prepare("UPDATE tasks SET missionId = NULL WHERE id = ?")
.run(task.id);
expect(ms.listGoalIdsForTask(task.id)).toEqual([goal.id]);
expect(ms.listGoalsForTask(task.id)).toEqual([goal]);

View File

@@ -341,10 +341,11 @@ describe("branch group abandon (U6, R7)", () => {
const res = await REQUEST(app, "POST", "/branch-groups/BG-AB/abandon", JSON.stringify({}), { "content-type": "application/json" });
expect(res.status).toBe(200);
expect(closeGroupPr).not.toHaveBeenCalled();
expect(updateBranchGroup).toHaveBeenCalledWith(
expect(updateBranchGroup).toHaveBeenLastCalledWith(
"BG-AB",
expect.objectContaining({ status: "abandoned", prState: "none" }),
);
expect(res.body.group.prState).toBe("none");
});
});

View File

@@ -866,6 +866,39 @@ describe("promoteBranchGroup finalized-but-PR-less repair (Fix #4 part 2)", () =
expect(mainAfter).toBe(mainBefore);
});
it("repairs the legacy fallback state: finalized + prState 'open' + prNumber null still creates the PR", async () => {
// The old code flipped prState to "open" without creating a PR — re-running
// with createGroupPr wired must not be short-circuited by the open-state guard.
const rootDir = makePrRepo();
let group = makeGroup({ status: "finalized", prState: "open", prNumber: null, prUrl: null });
let createCalls = 0;
const store = {
getBranchGroup: () => group,
getBranchGroupByBranchName: () => null,
listTasksByBranchGroup: async () => [landedMember("FN-A", group.branchName)],
updateBranchGroup: (_id: string, patch: Record<string, unknown>) => {
group = { ...group, ...patch };
return group;
},
} as any;
const result = await promoteBranchGroup({
rootDir,
groupId: group.id,
settings: prSettings,
store,
createGroupPr: async () => {
createCalls += 1;
return { prNumber: 91, prUrl: "https://github.com/x/y/pull/91", prState: "open" as const };
},
});
expect(result.reason).toBe("promoted");
expect(createCalls).toBe(1);
expect(group.prNumber).toBe(91);
expect(group.prState).toBe("open");
});
it("a finalized group that already has a prNumber is still short-circuited (no repair, no PR re-create)", async () => {
const rootDir = makePrRepo();
let group = makeGroup({ status: "finalized", prState: "open", prNumber: 5, prUrl: "https://github.com/x/y/pull/5" });

View File

@@ -287,7 +287,10 @@ async function promoteBranchGroupInner(input: PromoteBranchGroupInput): Promise<
};
}
if (group.prState === "open") {
// Legacy fallback rows are exactly `finalized + prState:"open" + prNumber:null`
// (the old code flipped prState without creating a PR) — the repair path must
// not be short-circuited by the open-state guard for them.
if (!needsPrRepair && group.prState === "open") {
return {
groupId: group.id,
promoted: false,