fix(pm-dispatch): verify issue_number attach target before enqueue (#3007)

pm-stack/dispatch/src/github.ts

@@ -33,6 +33,24 @@ function b64url(data: Buffer | string): string {
   return Buffer.from(data).toString("base64").replace(/\+/g, "-").replace(/\//g, "_").replace(/=+$/, "");
 }
 
+/**
+ * True only when a gh CLI error reports an actual HTTP 404. Matches the
+ * status text ("HTTP 404" / "(HTTP 404)"), never the bare digits: the gh
+ * error prefix echoes the request path, so an issue literally numbered
+ * 404 (".../issues/404") must not turn an auth/rate-limit/transient
+ * failure into a definitive not-found (Codex P2 on PR #3149).
+ */
+export function isNotFoundError(message: string): boolean {
+  return /HTTP 404\b/.test(message);
+}
+
+export interface IssueInfo {
+  number: number;
+  state: string;
+  isPullRequest: boolean;
+  url: string;
+}
+
 export class GitHubClient {
   private installationIdCache = new Map<string, number>();
   private tokenCache = new Map<string, { token: string; expiresAt: number }>();
@@ -183,6 +201,64 @@ export class GitHubClient {
     return { number: Number(m[1]), url };
   }
 
+  /**
+   * Look up an issue by number (issue #3007). Returns null on a
+   * definitive 404; throws GitHubError for any other failure so callers
+   * can distinguish an invalid target from a lookup failure. The GitHub
+   * issues API also returns pull requests — `isPullRequest` exposes the
+   * `pull_request` marker so callers can reject PR numbers.
+   */
+  async getIssue(repo: string, issueNumber: number): Promise<IssueInfo | null> {
+    let appError: unknown = null;
+    if (this.appConfigured) {
+      try {
+        const token = await this.installationToken(repo);
+        const res = await this.appApi("GET", `/repos/${repo}/issues/${issueNumber}`, token);
+        if (res.status === 404) return null;
+        if (!res.ok) throw new GitHubError(`github issue lookup failed (HTTP ${res.status})`);
+        const data = (await res.json()) as {
+          number: number;
+          state: string;
+          html_url: string;
+          pull_request?: unknown;
+        };
+        return {
+          number: data.number,
+          state: data.state,
+          isPullRequest: Boolean(data.pull_request),
+          url: data.html_url,
+        };
+      } catch (e) {
+        appError = e;
+        /* fall through to gh, mirroring createIssue */
+      }
+    }
+    try {
+      const out = await this.gh(["api", `repos/${repo}/issues/${issueNumber}`]);
+      const data = JSON.parse(out) as {
+        number: number;
+        state: string;
+        html_url: string;
+        pull_request?: unknown;
+      };
+      return {
+        number: data.number,
+        state: data.state,
+        isPullRequest: Boolean(data.pull_request),
+        url: data.html_url,
+      };
+    } catch (e) {
+      const msg = e instanceof Error ? e.message : String(e);
+      if (isNotFoundError(msg)) return null;
+      if (appError) {
+        throw new GitHubError(
+          `issue lookup failed via app (${appError instanceof Error ? appError.message : String(appError)}) and via gh (${msg})`,
+        );
+      }
+      throw e;
+    }
+  }
+
   async addLabels(repo: string, issueNumber: number, labels: string[]): Promise<void> {
     for (const label of labels) await this.ensureLabel(repo, label);
     if (this.appConfigured) {

pm-stack/dispatch/src/server.ts

@@ -289,6 +289,31 @@ async function handleTasks(req: Request, caller: CallerIdentity): Promise<Respon
     issue = await maybeCreateIssue(body.repo, body.title, body.prompt, ["queue/queued", "source/dispatch"], warnings);
   }
 
+  // Verify the attach target before enqueueing (#3007): a nonexistent
+  // number, a PR number, or a closed issue must not silently attach.
+  let attachVerified: boolean | null = null;
+  if (body.issueNumber !== undefined && issue) {
+    const verification = await verifyAttachTarget({
+      appInstalled: APP_INSTALLED_REPOS.has(body.repo),
+      repo: body.repo,
+      issueNumber: body.issueNumber,
+      lookup: async () => {
+        const info = await github.getIssue(body.repo, body.issueNumber!);
+        return info ? { state: info.state, isPullRequest: info.isPullRequest } : null;
+      },
+    });
+    if (verification.outcome === "reject" || verification.outcome === "lookup_failed") {
+      return json(verification.status, {
+        error: verification.error,
+        issue_number: issue.number,
+        issue_url: issue.url,
+        ...(warnings.length ? { warnings } : {}),
+      });
+    }
+    if (verification.outcome === "unverified") warnings.push(verification.warning);
+    attachVerified = verification.outcome === "verified";
+  }
+
   const dedupeKey =
     body.dedupeKey ??
     (issue ? canonicalDedupeKey(body.kind, body.repo, issue.number) : null) ??
@@ -351,12 +376,82 @@ async function handleTasks(req: Request, caller: CallerIdentity): Promise<Respon
   return json(201, {
     task_id: task.id,
     issue_number: issue?.number ?? null,
-    ...(body.issueNumber !== undefined && issue ? { issue_url: issue.url } : {}),
+    ...(body.issueNumber !== undefined && issue ? { issue_url: issue.url, attach_verified: attachVerified } : {}),
     queue: REPO_QUEUES[body.repo],
     ...(warnings.length ? { warnings } : {}),
   });
 }
 
+// ---------------------------------------------------------------------------
+// Attach-target verification (issue #3007)
+// ---------------------------------------------------------------------------
+
+export interface AttachTargetInfo {
+  state: string;
+  isPullRequest: boolean;
+}
+
+export type AttachVerification =
+  | { outcome: "verified" }
+  | { outcome: "unverified"; warning: string }
+  | { outcome: "reject"; status: number; error: string }
+  | { outcome: "lookup_failed"; status: 502; error: string };
+
+/**
+ * Verify an issue_number attach target BEFORE anything is enqueued:
+ * the number must exist in the repo, be an issue (not a PR), and be
+ * open. Invalid targets reject with a 4xx and enqueue nothing; GitHub
+ * lookup failures are a retryable 502, never conflated with invalid
+ * targets. Where the GitHub App is not installed the target cannot be
+ * verified — the caller proceeds (the task is the tracking record) but
+ * the unverified state is explicit in the response, never silent.
+ */
+export async function verifyAttachTarget(args: {
+  appInstalled: boolean;
+  repo: string;
+  issueNumber: number;
+  lookup: () => Promise<AttachTargetInfo | null>;
+}): Promise<AttachVerification> {
+  if (!args.appInstalled) {
+    return {
+      outcome: "unverified",
+      warning: `github app is not installed on ${args.repo}; attach target #${args.issueNumber} was NOT verified (it may not exist, may be a pull request, or may be closed)`,
+    };
+  }
+  let info: AttachTargetInfo | null;
+  try {
+    info = await args.lookup();
+  } catch (e) {
+    return {
+      outcome: "lookup_failed",
+      status: 502,
+      error: `attach target verification failed for ${args.repo}#${args.issueNumber}: ${e instanceof Error ? e.message : String(e)}; nothing was enqueued — retry later`,
+    };
+  }
+  if (!info) {
+    return {
+      outcome: "reject",
+      status: 404,
+      error: `issue #${args.issueNumber} not found in ${args.repo}; nothing was enqueued`,
+    };
+  }
+  if (info.isPullRequest) {
+    return {
+      outcome: "reject",
+      status: 422,
+      error: `#${args.issueNumber} in ${args.repo} is a pull request, not an issue; nothing was enqueued`,
+    };
+  }
+  if (info.state !== "open") {
+    return {
+      outcome: "reject",
+      status: 422,
+      error: `issue #${args.issueNumber} in ${args.repo} is ${info.state}, not open; nothing was enqueued`,
+    };
+  }
+  return { outcome: "verified" };
+}
+
 // ---------------------------------------------------------------------------
 // Routing
 // ---------------------------------------------------------------------------

pm-stack/dispatch/test/attach_verify.test.ts

@@ -0,0 +1,187 @@
+// Issue #3007: pm-dispatch must verify the issue_number attach target
+// before enqueueing. A nonexistent number, a PR number, or a closed
+// issue used to attach "successfully" (201 + task enqueued against a
+// bogus target). These tests stub the GitHub lookup/client — no network.
+import { describe, expect, test } from "bun:test";
+import { generateKeyPairSync } from "node:crypto";
+import { verifyAttachTarget, type AttachTargetInfo } from "../src/server.ts";
+import { GitHubClient, isNotFoundError } from "../src/github.ts";
+
+const REPO = "developing-today/code";
+const found = (partial: Partial<AttachTargetInfo>): (() => Promise<AttachTargetInfo | null>) =>
+  async () => ({ state: "open", isPullRequest: false, ...partial });
+
+describe("verifyAttachTarget (#3007)", () => {
+  test("nonexistent number rejects 404 (invalid target, not a lookup failure)", async () => {
+    const out = await verifyAttachTarget({
+      appInstalled: true,
+      repo: REPO,
+      issueNumber: 999999,
+      lookup: async () => null,
+    });
+    expect(out.outcome).toBe("reject");
+    if (out.outcome === "reject") {
+      expect(out.status).toBe(404);
+      expect(out.error).toContain("nothing was enqueued");
+    }
+  });
+
+  test("PR number rejects 422", async () => {
+    const out = await verifyAttachTarget({
+      appInstalled: true,
+      repo: REPO,
+      issueNumber: 3143,
+      lookup: found({ isPullRequest: true }),
+    });
+    expect(out.outcome).toBe("reject");
+    if (out.outcome === "reject") {
+      expect(out.status).toBe(422);
+      expect(out.error).toContain("pull request");
+    }
+  });
+
+  test("closed issue rejects 422", async () => {
+    const out = await verifyAttachTarget({
+      appInstalled: true,
+      repo: REPO,
+      issueNumber: 123,
+      lookup: found({ state: "closed" }),
+    });
+    expect(out.outcome).toBe("reject");
+    if (out.outcome === "reject") {
+      expect(out.status).toBe(422);
+      expect(out.error).toContain("closed");
+    }
+  });
+
+  test("open issue is the happy path: verified", async () => {
+    const out = await verifyAttachTarget({
+      appInstalled: true,
+      repo: REPO,
+      issueNumber: 3007,
+      lookup: found({}),
+    });
+    expect(out.outcome).toBe("verified");
+  });
+
+  test("GitHub lookup failure is a retryable 502, distinct from an invalid target", async () => {
+    const out = await verifyAttachTarget({
+      appInstalled: true,
+      repo: REPO,
+      issueNumber: 3007,
+      lookup: async () => {
+        throw new Error("github issue lookup failed (HTTP 503)");
+      },
+    });
+    expect(out.outcome).toBe("lookup_failed");
+    if (out.outcome === "lookup_failed") {
+      expect(out.status).toBe(502);
+      expect(out.error).toContain("nothing was enqueued");
+    }
+  });
+
+  test("app-not-installed repo: unverified is explicit and the lookup is never attempted", async () => {
+    let lookups = 0;
+    const out = await verifyAttachTarget({
+      appInstalled: false,
+      repo: "developing-today/zettel",
+      issueNumber: 5,
+      lookup: async () => {
+        lookups++;
+        return { state: "open", isPullRequest: false };
+      },
+    });
+    expect(out.outcome).toBe("unverified");
+    if (out.outcome === "unverified") expect(out.warning).toContain("NOT verified");
+    expect(lookups).toBe(0);
+  });
+});
+
+// --- GitHubClient.getIssue with a stubbed fetch (app route) -----------
+
+const { privateKey } = generateKeyPairSync("rsa", { modulusLength: 2048 });
+const pem = privateKey.export({ format: "pem", type: "pkcs8" }) as string;
+
+const realFetch = globalThis.fetch;
+
+function stubGitHub(issueResponse: () => Response) {
+  globalThis.fetch = (async (input: unknown) => {
+    const url = String(input);
+    if (url.endsWith(`/repos/${REPO}/installation`)) {
+      return new Response(JSON.stringify({ id: 42 }), { status: 200 });
+    }
+    if (url.includes("/app/installations/42/access_tokens")) {
+      return new Response(
+        JSON.stringify({ token: "test-token", expires_at: new Date(Date.now() + 3_600_000).toISOString() }),
+        { status: 201 },
+      );
+    }
+    if (url.includes(`/repos/${REPO}/issues/`)) return issueResponse();
+    throw new Error(`unexpected fetch in test: ${url}`);
+  }) as typeof fetch;
+}
+
+describe("GitHubClient.getIssue with stubbed GitHub (#3007)", () => {
+  test("open issue parses; pull_request marker absent", async () => {
+    stubGitHub(
+      () =>
+        new Response(JSON.stringify({ number: 3007, state: "open", html_url: `https://github.com/${REPO}/issues/3007` }), {
+          status: 200,
+        }),
+    );
+    try {
+      const client = new GitHubClient("12345", pem);
+      const info = await client.getIssue(REPO, 3007);
+      expect(info).not.toBeNull();
+      expect(info!.state).toBe("open");
+      expect(info!.isPullRequest).toBe(false);
+    } finally {
+      globalThis.fetch = realFetch;
+    }
+  });
+
+  test("PR number parses with isPullRequest true", async () => {
+    stubGitHub(
+      () =>
+        new Response(
+          JSON.stringify({
+            number: 3143,
+            state: "open",
+            html_url: `https://github.com/${REPO}/pull/3143`,
+            pull_request: { url: "https://api.github.com/x" },
+          }),
+          { status: 200 },
+        ),
+    );
+    try {
+      const client = new GitHubClient("12345", pem);
+      const info = await client.getIssue(REPO, 3143);
+      expect(info).not.toBeNull();
+      expect(info!.isPullRequest).toBe(true);
+    } finally {
+      globalThis.fetch = realFetch;
+    }
+  });
+
+  test("404 returns null (definitive not-found, no throw)", async () => {
+    stubGitHub(() => new Response(JSON.stringify({ message: "Not Found" }), { status: 404 }));
+    try {
+      const client = new GitHubClient("12345", pem);
+      expect(await client.getIssue(REPO, 999999)).toBeNull();
+    } finally {
+      globalThis.fetch = realFetch;
+    }
+  });
+});
+
+describe("isNotFoundError (#3007, Codex P2 on PR #3149)", () => {
+  test("an HTTP 404 status in the gh error is a not-found", () => {
+    expect(isNotFoundError("gh api repos/x/issues/9 failed: gh: Not Found (HTTP 404)")).toBe(true);
+  });
+
+  test("issue NUMBER 404 in the echoed path is not a not-found", () => {
+    expect(isNotFoundError("gh api repos/x/issues/404 failed: gh: Forbidden (HTTP 403)")).toBe(false);
+    expect(isNotFoundError("gh api repos/x/issues/404 failed: gh: rate limit (HTTP 429)")).toBe(false);
+    expect(isNotFoundError("gh api repos/x/issues/404 failed: gh: server error (HTTP 500)")).toBe(false);
+  });
+});