fix(comment): dedupe duplicate managed comments in the gh fallback path

.changeset/dedupe-gh-fallback-comment.md

@@ -0,0 +1,5 @@
+---
+"@buildinternet/uploads": patch
+---
+
+The local-`gh` fallback for the managed attachments comment (used when the GitHub App bot path is unavailable or unauthorized) now collapses duplicate marker comments the same way the bot path does: it collects every comment carrying the workspace's exact namespaced marker, patches the oldest, and best-effort deletes the rest, swallowing delete failures. Previously this path only ever patched the first match it found, so a duplicate left by a concurrent-create race (two `uploads attach` runs racing before either found an existing comment) never healed. Legacy unnamespaced marker comments are still adopt-only and are never deleted.

apps/api/src/github-comment.test.ts

@@ -958,6 +958,55 @@ describe("upsertBotComment", () => {
     });
   });
 
+  it("in legacy mode (no workspace) never deletes a second legacy comment", async () => {
+    // Mirrors the CLI's gh-path guard: with the shared legacy marker a second
+    // hit may belong to a different workspace, so it is adopted, never deleted.
+    const { env } = makeTestEnv();
+    const cfg = { appId: "1", privateKey: await testPem(), homeInstallationId: "9" };
+    const deleted: string[] = [];
+    const fetchImpl = (async (url: string, init: RequestInit = {}) => {
+      if (url.includes("/access_tokens")) {
+        return new Response(JSON.stringify({ token: "t" }), { status: 201 });
+      }
+      if (url.includes("/issues/12/comments")) {
+        return new Response(
+          JSON.stringify([
+            { id: 20, body: `${ATTACHMENTS_MARKER}\nfirst` },
+            { id: 21, body: `${ATTACHMENTS_MARKER}\ncould be another workspace` },
+          ]),
+          { status: 200 },
+        );
+      }
+      if (init.method === "DELETE") {
+        deleted.push(url);
+        return new Response(null, { status: 204 });
+      }
+      if (url.includes("/issues/comments/20") && init.method === "PATCH") {
+        return new Response(
+          JSON.stringify({ id: 20, html_url: "https://github.com/acme/web/pull/12#c20" }),
+          { status: 200 },
+        );
+      }
+      return new Response("nf", { status: 404 });
+    }) as unknown as typeof fetch;
+    // An empty workspace name degrades attachmentsMarker() to the shared
+    // legacy marker — the only way this path is reachable server-side.
+    const res = await upsertBotComment(
+      env,
+      cfg,
+      42,
+      { repo: "acme/web", num: 12 },
+      "BODY",
+      "",
+      fetchImpl,
+    );
+    expect(res).toEqual({
+      action: "updated",
+      commentUrl: "https://github.com/acme/web/pull/12#c20",
+    });
+    expect(deleted).toEqual([]);
+  });
+
   it("forceHunt ignores a warm cached id so a duplicate is collapsed (issue #480)", async () => {
     const { env, kv } = makeTestEnv();
     const cfg = { appId: "1", privateKey: await testPem(), homeInstallationId: "9" };

apps/api/src/github-comment.ts

@@ -376,6 +376,14 @@ async function findMarkerComment(
     if (comments.length < 100) break; // last page reached.
   }
   // Comments list oldest-first, so hits[0] is the oldest marker comment.
-  if (hits.length > 0) return { comment: hits[0], extras: hits.slice(1) };
+  if (hits.length > 0) {
+    // In legacy mode (no workspace to namespace with) our "exact" marker IS
+    // the shared one, so a second hit is not our own duplicate — it may be
+    // another workspace's comment. Adopt the oldest and never delete: the
+    // adopt-only contract is about the marker being ambiguous, which is just
+    // as true when it is the marker we are hunting on.
+    const extras = marker === ATTACHMENTS_MARKER ? undefined : hits.slice(1);
+    return { comment: hits[0], extras };
+  }
   return { comment: legacyHit }; // best effort — adopt a legacy hit if seen.
 }

packages/uploads/src/github-gh.ts

@@ -217,42 +217,83 @@ interface GhComment {
 /**
  * PR comments live on the issues endpoint, so one path covers PRs and issues.
  * `--paginate` follows Link headers and merges every page into one array, so the
- * marker comment is found even on threads past 100 comments.
+ * marker comment is found even on threads past 100 comments. GitHub returns
+ * comments oldest-first, so `hits[0]` (after merging paginated pages, which
+ * preserve that order) is the oldest exact-`marker` hit.
  *
  * Hunts for `marker` (the namespaced, per-workspace marker) first; when none
  * is found, falls back to a comment carrying the shared legacy
  * `ATTACHMENTS_MARKER` (pre-4b, unnamespaced) so it can be adopted and
  * migrated in place. When `marker` IS the legacy marker (no workspace to
  * namespace with) this collapses to a single hunt, unchanged from pre-4b
  * behavior.
+ *
+ * Collects EVERY comment carrying `marker` (a create race can leave more
+ * than one — issue #486, mirroring the bot path's #470 fix): the oldest is
+ * `comment`, the rest come back as `extras` for the caller to delete. Only
+ * exact-`marker` hits are ever extras — a legacy (unnamespaced) comment may
+ * belong to a different workspace, so it is adopted at most, never deleted.
  */
 function findManagedComment(
   target: GhTarget,
   run: CommandRunner,
   marker: string,
-): GhComment | undefined {
+): { comment?: GhComment; extras?: GhComment[] } {
   const raw = run("gh", [
     "api",
     `repos/${target.repo}/issues/${target.num}/comments?per_page=100`,
     "--paginate",
   ]);
   const comments = JSON.parse(raw) as GhComment[];
-  const namespacedHit = comments.find((c) => typeof c.body === "string" && c.body.includes(marker));
-  if (namespacedHit) return namespacedHit;
-  if (marker === ATTACHMENTS_MARKER) return undefined;
-  return comments.find((c) => typeof c.body === "string" && c.body.includes(ATTACHMENTS_MARKER));
+  const hits = comments.filter((c) => typeof c.body === "string" && c.body.includes(marker));
+  if (hits.length > 0) {
+    // In legacy mode (no workspace to namespace with) our "exact" marker IS
+    // the shared one, so a second hit is not our own duplicate — it may be
+    // another workspace's comment. Adopt the oldest and never delete: the
+    // adopt-only contract is about the marker being ambiguous, which is just
+    // as true when it is the marker we are hunting on.
+    const extras = marker === ATTACHMENTS_MARKER ? undefined : hits.slice(1);
+    return { comment: hits[0], extras };
+  }
+  if (marker === ATTACHMENTS_MARKER) return {};
+  const legacyHit = comments.find(
+    (c) => typeof c.body === "string" && c.body.includes(ATTACHMENTS_MARKER),
+  );
+  return { comment: legacyHit };
 }
 
 /**
  * Create the managed attachments comment, or edit it in place if it already
- * exists. Never touches any other comment. Body is passed via stdin
+ * exists. Never touches any other comment except best-effort deletes of
+ * duplicate marker comments (see below). Body is passed via stdin
  * (`-F body=@-`) so it is never shell-interpolated.
  *
  * `marker` identifies which comment to hunt for (see `findManagedComment`);
  * `body` is expected to already carry that same marker as its first line
  * (built via `attachmentsCommentBody(items, galleries, marker)`), so patching
  * an adopted legacy comment migrates it to the namespaced marker in place.
  * Defaults to the shared legacy marker for backward compatibility.
+ *
+ * Self-healing dedupe (issue #486, mirroring the bot path's #470/#484 fix):
+ * a create race (two concurrent `uploads attach` runs, neither finding an
+ * existing comment) can leave more than one marker comment on the thread.
+ * This path has no id cache, so unlike the bot path a duplicate here never
+ * heals on its own — every sync just patches the oldest and leaves the rest
+ * stale. After patching (or creating), any extra exact-`marker` hits are
+ * deleted best-effort via `gh api -X DELETE`; a failed delete is swallowed
+ * and never fails the caller's command, and the next sync retries anyway.
+ *
+ * On why this duplicates the bot path rather than deferring to it: the gh
+ * fallback is a supported path, not a stopgap, so it is held at behavioral
+ * parity deliberately. This file already reimplements the hunt, the legacy
+ * adoption and the create-vs-patch gate against a different transport (the
+ * `gh` subprocess, not the App's token), and #486 existed precisely because
+ * the two drifted. Treat any behavior change to `upsertBotComment`
+ * (apps/api/src/github-comment.ts) as owing a matching change here. Note
+ * this is the one place the CLI deletes a GitHub resource under the
+ * invoking human's own credentials — bounded to comments carrying this
+ * workspace's exact namespaced marker, whose content is always
+ * regenerable.
  */
 export function upsertAttachmentsComment(
   target: GhTarget,
@@ -262,7 +303,18 @@ export function upsertAttachmentsComment(
   opts: { createIfMissing?: boolean } = {},
 ): { action: "created" | "updated" | "skipped" } {
   const createIfMissing = opts.createIfMissing ?? true;
-  const existing = findManagedComment(target, run, marker);
+  const { comment: existing, extras } = findManagedComment(target, run, marker);
+
+  const deleteExtras = () => {
+    for (const extra of extras ?? []) {
+      try {
+        run("gh", ["api", `repos/${target.repo}/issues/comments/${extra.id}`, "-X", "DELETE"]);
+      } catch {
+        // Best effort only — a failed delete must never fail the caller's command.
+      }
+    }
+  };
+
   if (existing) {
     run(
       "gh",
@@ -276,11 +328,14 @@ export function upsertAttachmentsComment(
       ],
       body,
     );
+    deleteExtras();
     return { action: "updated" };
   }
   // Patch-only (createIfMissing false, i.e. an empty body) with no existing
   // comment: nothing to do — never create one just to say it's empty.
   if (!createIfMissing) return { action: "skipped" };
+  // No existing marker hit means `extras` is necessarily empty here (see
+  // `findManagedComment`) — nothing to delete after a create.
   run("gh", ["api", `repos/${target.repo}/issues/${target.num}/comments`, "-F", "body=@-"], body);
   return { action: "created" };
 }

packages/uploads/test/github-gh.test.ts

@@ -247,6 +247,131 @@ describe("upsertAttachmentsComment", () => {
     expect(patch.args).toContain("repos/o/r/issues/comments/42");
     expect(patch.args).toContain("PATCH");
   });
+
+  it("collapses duplicate marker comments: patches the oldest, deletes the extras (issue #486)", () => {
+    const marker = attachmentsMarker("acme");
+    const { run, calls } = fakeRunner({
+      gh: (args) => {
+        if (args[1]?.includes("/comments?per_page=100")) {
+          // Oldest-first, as GitHub returns them.
+          return JSON.stringify([
+            { id: 1, body: "unrelated" },
+            { id: 10, body: `${marker}\nold body` },
+            { id: 11, body: `${marker}\nduplicate from a create race` },
+            { id: 12, body: `${marker}\nanother duplicate` },
+          ]);
+        }
+        return JSON.stringify({ id: 10 });
+      },
+    });
+    const result = upsertAttachmentsComment(target, `${marker}\nnew body`, run, marker);
+    expect(result).toEqual({ action: "updated" });
+
+    // Patches the oldest hit.
+    const patch = calls[1];
+    expect(patch.args).toEqual(
+      expect.arrayContaining(["repos/o/r/issues/comments/10", "-X", "PATCH", "-F", "body=@-"]),
+    );
+
+    // Best-effort deletes the extras, oldest-hit excluded.
+    const deletes = calls.slice(2);
+    expect(deletes).toHaveLength(2);
+    expect(deletes[0].args).toEqual(["api", "repos/o/r/issues/comments/11", "-X", "DELETE"]);
+    expect(deletes[1].args).toEqual(["api", "repos/o/r/issues/comments/12", "-X", "DELETE"]);
+  });
+
+  it("still reports success when a duplicate delete fails (best-effort, swallowed)", () => {
+    const marker = attachmentsMarker("acme");
+    const { run, calls } = fakeRunner({
+      gh: (args) => {
+        if (args[1]?.includes("/comments?per_page=100")) {
+          return JSON.stringify([
+            { id: 10, body: `${marker}\nold body` },
+            { id: 11, body: `${marker}\nduplicate` },
+          ]);
+        }
+        if (args.includes("DELETE")) {
+          throw new Error("gh: 403 Forbidden");
+        }
+        return JSON.stringify({ id: 10 });
+      },
+    });
+    const result = upsertAttachmentsComment(target, `${marker}\nnew body`, run, marker);
+    expect(result).toEqual({ action: "updated" });
+    expect(calls.some((c) => c.args.includes("DELETE"))).toBe(true);
+  });
+
+  it("in legacy mode (no workspace) never deletes a second legacy comment", () => {
+    // With the shared legacy marker there is no workspace dimension, so a
+    // second hit may belong to a different workspace rather than being our
+    // own duplicate. Adopt the oldest; delete nothing.
+    const { run, calls } = fakeRunner({
+      gh: (args) => {
+        if (args[1]?.includes("/comments?per_page=100")) {
+          return JSON.stringify([
+            { id: 20, body: `${ATTACHMENTS_MARKER}\nfirst` },
+            { id: 21, body: `${ATTACHMENTS_MARKER}\ncould be another workspace` },
+          ]);
+        }
+        return JSON.stringify({ id: 20 });
+      },
+    });
+    const result = upsertAttachmentsComment(target, `${ATTACHMENTS_MARKER}\nnew body`, run);
+    expect(result).toEqual({ action: "updated" });
+    expect(calls.some((c) => c.args.includes("DELETE"))).toBe(false);
+    expect(calls[1].args).toContain("repos/o/r/issues/comments/20");
+  });
+
+  it("never deletes another workspace's namespaced marker comment while deduping", () => {
+    const marker = attachmentsMarker("acme");
+    const otherMarker = attachmentsMarker("acme-2");
+    const { run, calls } = fakeRunner({
+      gh: (args) => {
+        if (args[1]?.includes("/comments?per_page=100")) {
+          // `acme-2` is deliberately a superstring of `acme`: the trailing
+          // ` -->` in the marker is what keeps `includes()` from matching a
+          // neighbouring workspace's comment. Guards the cross-tenant
+          // property that the bot path also tests.
+          return JSON.stringify([
+            { id: 10, body: `${marker}\nours` },
+            { id: 11, body: `${otherMarker}\nanother workspace's comment` },
+            { id: 12, body: `${marker}\nour duplicate` },
+          ]);
+        }
+        return JSON.stringify({ id: 10 });
+      },
+    });
+    const result = upsertAttachmentsComment(target, `${marker}\nnew body`, run, marker);
+    expect(result).toEqual({ action: "updated" });
+
+    // Only our own duplicate is deleted — never the other workspace's.
+    const deletes = calls.filter((c) => c.args.includes("DELETE"));
+    expect(deletes).toHaveLength(1);
+    expect(deletes[0].args).toEqual(["api", "repos/o/r/issues/comments/12", "-X", "DELETE"]);
+  });
+
+  it("never deletes a legacy (unnamespaced) comment even when adopting it", () => {
+    const marker = attachmentsMarker("acme");
+    const { run, calls } = fakeRunner({
+      gh: (args) => {
+        if (args[1]?.includes("/comments?per_page=100")) {
+          // Only a legacy hit exists — no namespaced marker at all, so this
+          // must be adopted (patched), never deleted, and no other comment
+          // should be touched.
+          return JSON.stringify([
+            { id: 7, body: `${ATTACHMENTS_MARKER}\nold body` },
+            { id: 8, body: "unrelated comment, also unnamespaced but no marker at all" },
+          ]);
+        }
+        return JSON.stringify({ id: 7 });
+      },
+    });
+    const result = upsertAttachmentsComment(target, `${marker}\nnew body`, run, marker);
+    expect(result).toEqual({ action: "updated" });
+    expect(calls.some((c) => c.args.includes("DELETE"))).toBe(false);
+    const patch = calls[1];
+    expect(patch.args).toContain("repos/o/r/issues/comments/7");
+  });
 });
 
 describe("resolveGhTitle", () => {