Skip to content

gh fallback never dedupes duplicate managed comments (and can't self-heal) #486

Description

@zachdunn

Follow-up to #470 / #480, spotted while fixing #480 (PR #484).

Behavior

The duplicate-comment dedupe added in PR #471 and extended in PR #484 lives entirely in the bot path (upsertBotComment, apps/api/src/github-comment.ts). The local-gh fallback has no equivalent.

findManagedComment (packages/uploads/src/github-gh.ts:229) resolves the managed comment with .find(...) — first hit only:

const namespacedHit = comments.find((c) => typeof c.body === "string" && c.body.includes(marker));

and upsertAttachmentsComment (packages/uploads/src/github-gh.ts:257) issues exactly one PATCH or POST. There are zero DELETE calls in the file. So where the bot path collects every marker hit, patches the oldest and deletes the extras, the gh path patches the oldest and leaves the rest untouched forever — no cache expiry to eventually heal it, because this path has no cache at all.

How a duplicate gets there

Same create race as #470, just on the other path: two concurrent uploads attach runs against a repo where the App isn't installed (or isn't authorized) both hunt, both find nothing, and both POST. From then on every sync patches the older comment while the newer one sits stale on the thread.

Consequence

Narrower blast radius than #480 — this only affects threads where the bot path declined and the CLI fell through to gh (syncAttachmentsComment, packages/uploads/src/commands.ts:717). But unlike #480 it is permanent, not time-bounded: there is no 404-drop or TTL to force a re-hunt, so the duplicate never heals on its own.

Proposed

Mirror the bot path: have findManagedComment collect every exact-marker hit rather than the first, keep the oldest, return the rest, and best-effort gh api -X DELETE them — failures swallowed, same as the bot path's dedupe loop.

Two things worth deciding first, which is why this is filed rather than just fixed:

  1. Whose credentials do the deleting. The bot path deletes as uploads-sh[bot], which only ever authored those comments. The gh path would delete as the invoking human, so a stray delete is attributable to a real person and shows in their audit trail. Probably still fine — it only ever targets comments carrying this workspace's own namespaced marker — but it is a different risk posture than the bot path and should be a deliberate call.
  2. Legacy markers stay adopt-only. As in the bot path, only exact-marker hits may be deleted; an unnamespaced pre-4b comment can belong to another workspace and must never be removed.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions