fix(scm-github,session-manager): tighten fork filter; add closed-PR restore test
Address PR review:
1. Strict-by-default fork filter in `detectPR` — require the head owner
to be a non-empty string AND match the project repo owner. A null,
undefined, or empty `headRepositoryOwner.login` (e.g. deleted account
or future gh API change) no longer silently passes the filter.
2. Update existing same-repo cache/draft tests to include
`headRepositoryOwner: { login: "acme" }` since the strict filter
would otherwise reject them as fork PRs.
3. Add a regression test that null/missing `headRepositoryOwner` is
rejected.
4. Add an independent restore test for the `pr.state === "closed"`
branch of session-manager's step 3a (previously only `merged` was
covered).
Refs #1724.
This commit is contained in:
parent
2bfad4a138
commit
4043fa4a47
|
|
@ -226,6 +226,36 @@ describe("restore", () => {
|
|||
await expect(sm.restore("app-1")).rejects.toThrow(/is merged/);
|
||||
});
|
||||
|
||||
it("refuses to restore sessions whose PR is closed (unmerged terminal state)", async () => {
|
||||
const ws = "/tmp/mock-ws/app-1-closed";
|
||||
writeMetadata(sessionsDir, "app-1", {
|
||||
worktree: ws,
|
||||
branch: "feat/abandoned",
|
||||
status: "killed",
|
||||
pr: "https://github.com/acme/repo/pull/99",
|
||||
project: "my-app",
|
||||
runtimeHandle: makeHandle("rt-old"),
|
||||
});
|
||||
// No legacy `status` maps to lifecycle.pr.state="closed", so write the
|
||||
// canonical lifecycle directly to exercise the closed branch of step 3a.
|
||||
updateMetadata(sessionsDir, "app-1", {
|
||||
lifecycle: JSON.stringify({
|
||||
version: 2,
|
||||
session: { state: "terminated", terminatedAt: new Date().toISOString() },
|
||||
pr: {
|
||||
state: "closed",
|
||||
reason: "closed_unmerged",
|
||||
number: 99,
|
||||
url: "https://github.com/acme/repo/pull/99",
|
||||
lastObservedAt: new Date().toISOString(),
|
||||
},
|
||||
}),
|
||||
});
|
||||
|
||||
const sm = createSessionManager({ config, registry: mockRegistry });
|
||||
await expect(sm.restore("app-1")).rejects.toThrow(/is closed/);
|
||||
});
|
||||
|
||||
it("throws SessionNotRestorableError for working sessions", async () => {
|
||||
writeMetadata(sessionsDir, "app-1", {
|
||||
worktree: "/tmp",
|
||||
|
|
|
|||
|
|
@ -650,10 +650,13 @@ function createGitHubSCM(): SCM {
|
|||
}> = JSON.parse(raw);
|
||||
|
||||
// Only same-repo PRs auto-associate. Fork PRs (different head owner)
|
||||
// are someone else's work — never the session's.
|
||||
// are someone else's work — never the session's. Strict-by-default:
|
||||
// a missing/null/empty headOwner is rejected so a future gh API
|
||||
// change or deleted-account edge case can't quietly let a fork PR
|
||||
// through.
|
||||
const sameRepoPrs = prs.filter((pr) => {
|
||||
const headOwner = pr.headRepositoryOwner?.login;
|
||||
return !headOwner || headOwner === owner;
|
||||
return typeof headOwner === "string" && headOwner === owner;
|
||||
});
|
||||
|
||||
if (sameRepoPrs.length === 0) return null;
|
||||
|
|
|
|||
|
|
@ -400,6 +400,7 @@ describe("scm-github plugin", () => {
|
|||
headRefName: "feat/my-feature",
|
||||
baseRefName: "main",
|
||||
isDraft: false,
|
||||
headRepositoryOwner: { login: "acme" },
|
||||
},
|
||||
]);
|
||||
|
||||
|
|
@ -459,12 +460,37 @@ describe("scm-github plugin", () => {
|
|||
headRefName: "feat/my-feature",
|
||||
baseRefName: "main",
|
||||
isDraft: true,
|
||||
headRepositoryOwner: { login: "acme" },
|
||||
},
|
||||
]);
|
||||
const result = await scm.detectPR(makeSession(), project);
|
||||
expect(result?.isDraft).toBe(true);
|
||||
});
|
||||
|
||||
it("rejects PRs with missing/null headRepositoryOwner (strict-by-default)", async () => {
|
||||
mockGh([
|
||||
{
|
||||
number: 50,
|
||||
url: "https://github.com/acme/repo/pull/50",
|
||||
title: "owner missing",
|
||||
headRefName: "feat/my-feature",
|
||||
baseRefName: "main",
|
||||
isDraft: false,
|
||||
headRepositoryOwner: null,
|
||||
},
|
||||
{
|
||||
number: 51,
|
||||
url: "https://github.com/acme/repo/pull/51",
|
||||
title: "owner field absent",
|
||||
headRefName: "feat/my-feature",
|
||||
baseRefName: "main",
|
||||
isDraft: false,
|
||||
},
|
||||
]);
|
||||
const result = await scm.detectPR(makeSession(), project);
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
|
||||
it("returns null when session.branch === project.defaultBranch", async () => {
|
||||
const result = await scm.detectPR(
|
||||
makeSession({ branch: "main" }),
|
||||
|
|
@ -1334,6 +1360,7 @@ describe("scm-github plugin", () => {
|
|||
headRefName: "feat/my-feature",
|
||||
baseRefName: "main",
|
||||
isDraft: false,
|
||||
headRepositoryOwner: { login: "acme" },
|
||||
},
|
||||
]);
|
||||
const a = await scm.detectPR(makeSession(), project);
|
||||
|
|
@ -1366,6 +1393,7 @@ describe("scm-github plugin", () => {
|
|||
headRefName: "feat/my-feature",
|
||||
baseRefName: "main",
|
||||
isDraft: false,
|
||||
headRepositoryOwner: { login: "acme" },
|
||||
},
|
||||
]);
|
||||
const after = await scm.detectPR(makeSession(), project);
|
||||
|
|
@ -1382,6 +1410,7 @@ describe("scm-github plugin", () => {
|
|||
headRefName: "feat/a",
|
||||
baseRefName: "main",
|
||||
isDraft: false,
|
||||
headRepositoryOwner: { login: "acme" },
|
||||
},
|
||||
]);
|
||||
mockGh([
|
||||
|
|
@ -1392,6 +1421,7 @@ describe("scm-github plugin", () => {
|
|||
headRefName: "feat/b",
|
||||
baseRefName: "main",
|
||||
isDraft: false,
|
||||
headRepositoryOwner: { login: "acme" },
|
||||
},
|
||||
]);
|
||||
const a = await scm.detectPR(makeSession({ branch: "feat/a" }), project);
|
||||
|
|
@ -1410,6 +1440,7 @@ describe("scm-github plugin", () => {
|
|||
headRefName: pr.branch,
|
||||
baseRefName: "main",
|
||||
isDraft: false,
|
||||
headRepositoryOwner: { login: "acme" },
|
||||
},
|
||||
]);
|
||||
await scm.detectPR(makeSession({ branch: pr.branch }), project);
|
||||
|
|
|
|||
Loading…
Reference in New Issue