fix(comment): dedupe duplicate managed comments in the gh fallback path#489
Conversation
Mirrors the bot path's #470/#484 self-healing dedupe: findManagedComment now collects every comment carrying the exact namespaced marker instead of stopping at the first hit, and upsertAttachmentsComment patches the oldest then best-effort `gh api -X DELETE`s the rest, swallowing failures. This path has no id cache, so unlike the bot path a duplicate here never healed on its own (issue #486). Legacy unnamespaced marker comments remain adopt-only and are never deleted.
The dedupe filters on the exact namespaced marker, so a neighbouring workspace's comment is safe only because of the trailing " -->" in the marker text. That is load-bearing for cross-tenant safety and the bot path tests it, so mirror the coverage here: `acme-2` is deliberately a superstring of `acme`, and only our own duplicate may be deleted. Mutation-checked: loosening the matcher to drop the trailing " -->" fails this test and only this test.
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (2)
🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe local ChangesManaged comment deduplication
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
uploads-auth | 44849d7 | Commit Preview URL Branch Preview URL |
Jul 23 2026, 09:51 PM |
Considered making dedupe an App-only feature and decided against it: the gh fallback is a supported path, not a stopgap. This file already reimplements the hunt, the legacy adoption and the create-vs-patch gate against a different transport, and #486 existed because those drifted -- so the parity is deliberate and future changes to upsertBotComment owe a matching change here. Comment-only; also notes the credentials posture at the point of use.
|
CodeRabbit (@coderabbitai) review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/uploads/src/github-gh.ts`:
- Around line 248-250: Update the matching logic in the uploads flow at
packages/uploads/src/github-gh.ts lines 248-250 so ATTACHMENTS_MARKER returns
only the oldest matching comment without an extras collection, while preserving
extras behavior for other markers. Add a default-marker test at
packages/uploads/test/github-gh.test.ts lines 332-353 with multiple legacy
matches and assert that no DELETE request is made.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1eb96f8-7a07-4fd7-ab35-91f1d916e212
📒 Files selected for processing (3)
.changeset/dedupe-gh-fallback-comment.mdpackages/uploads/src/github-gh.tspackages/uploads/test/github-gh.test.ts
In legacy mode (no workspace to namespace with) the "exact" marker IS the shared legacy one, so hits.slice(1) handed other workspaces' comments to the delete loop -- contradicting the adopt-only contract the same doc comment states. The ambiguity that makes a legacy comment undeletable applies just as much when it is the marker being hunted on. Fixed in the gh path AND the bot path: the bot path has the identical shape, and its marker is only slug-safe by construction rather than by guarantee. Fixing one and not the other would break the parity contract this PR just documented, on its first exercise. Both guards mutation-checked: dropping the legacy branch fails the new test on each side and nothing else. Reported by CodeRabbit on #489.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
uploads-api | 44849d7 | Commit Preview URL Branch Preview URL |
Jul 23 2026, 09:50 PM |
1 participant
Closes #486.
The gap
The duplicate-managed-comment dedupe from #470/#480 lived entirely in the bot path. The local-
ghfallback resolved its comment with.find(...)— first hit only — and had zeroDELETEcalls, so it patched the oldest marker comment and left any duplicate stale forever.Worse than #480 in one respect: the gh path has no id cache, so there is no 404-drop or TTL to force a re-hunt. A duplicate there never heals on its own.
The fix
Mirror the bot path in
findManagedComment/upsertAttachmentsComment(packages/uploads/src/github-gh.ts): collect every comment carrying the workspace's exact namespaced marker, patch the oldest, then best-effortgh api -X DELETEthe extras with each delete wrapped so a failure never fails the caller's command.Preserved unchanged: legacy unnamespaced markers stay adopt-only (they can belong to another workspace, so they are never deleted), the
createIfMissingpatch-only-when-empty gate, and the existing return shape. A create can't have extras by construction — no marker hit is what led to the create.Reviewer note: this deletes under your credentials
Worth a deliberate look rather than a skim. This is the first place the CLI deletes a GitHub resource under the invoking human's own
ghcredentials — every otherrun("gh", …)inpackages/uploadsis a read or a create/PATCH, and every other DELETE in the repo runs under the App's bot token.Assessment, for the record:
uploads-sh[bot]'s.Deliberately implemented as the straightforward mirror: no prompt, flag, or config toggle to soften this. If the attribution posture is judged wrong, the right fix is a design change, not a knob.
Tests
Four new cases in
packages/uploads/test/github-gh.test.ts:updatedoverallThat last one guards a property that rests entirely on the trailing
" -->"in the marker — the only reasonws=acmedoesn't matchws=acme-2. It is mutation-checked: removing that" -->"from the matcher fails this test and only this test.Golden fixtures deliberately untouched — only comment-hunting/deletion changed, not rendering.
Full suite: 2905 passed.
pnpm typecheckandpnpm lintclean.Considered: making dedupe App-only
Whether to skip this entirely and declare dedupe a GitHub-App-only feature was raised and rejected. Recorded here and in the source doc comment so it isn't re-litigated:
Consequence: treat any behavior change to
upsertBotCommentas owing a matching change here.Summary by CodeRabbit