feat(linear): dedup bridged comments on the idempotency key (BLO-31657) - #1654
feat(linear): dedup bridged comments on the idempotency key (BLO-31657)#1654allyblockcast[bot] wants to merge 4 commits into
Conversation
Steps 3-4 of BLO-31657. #1641 plumbed `idempotencyKey` through the plugin SDK comment path; this makes the Linear bridge use it and deletes the two layers it replaces. The bridge now passes `linear-comment:<uuid>` to `ctx.issues.createComment`, so dedup happens in the insert against a partial unique index: atomic, and unlike the process-local `inFlightComments` Set it keeps holding if the plugin tier ever runs more than one replica. Removed the Set and the `listComments` sentinel scan, which also drops one RPC round-trip per bridged comment. Two things the obvious version of this change would have got wrong: - The SDK test harness ignored `idempotencyKey` entirely, so the Linear concurrency test — which asserts against that fake, not a real DB — would have gone red for the wrong reason, or been "fixed" into asserting nothing. The harness now mirrors the host contract rather than approximating it: author-scoped `(issue, author, key)` matching the three partial indexes in 0206, whitespace-only keys treated as omitted per `readNonEmptyParam`, `deduplicated: true` on the return. - `addComment` returns the existing row rather than throwing, so the handler's `activity.log` would have fired on every duplicate delivery, reporting a sync that did not happen. The host already guards its own `issue.comment.created` log this way; the plugin now matches. The sentinel HTML comment is still written — edit propagation (BLO-31634) matches on it — and only the read is gone. That path is deliberately left unchanged: an edit keeps its Linear UUID, so it dedups against the original mirror exactly as it used to skip on the sentinel. Negative control, per the issue's verifying signal: dropping the key forward in worker.ts fails exactly four tests — sequential dedup, concurrent dedup, the key-shape assertion, and the activity-log assertion — and nothing else. All four rest on the key, none is inherited from the deleted Set. Verified: linear plugin 235/235, sdk 31/31, workspace-diff 26/26, both smoke examples, and server plugin-sdk-testing + linear-webhook-fixture-replay + plugin-sdk-orchestration-contract 14/14. Typecheck clean on the plugin and the SDK build.
|
@ally please review at head d7e0bf1 — steps 3-4 of BLO-31657: the Linear bridge now dedups on Specific focus, in priority order:
|
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d7e0bf1
I traced the full dedup chain end-to-end at this head rather than taking the description's word for it. Focus question 1 checks out completely — the DB guarantee is not merely equivalent to what you deleted, it is strictly stronger. Focus 2 is faithful on both points you asked about. Focus 4 does not hold, and that is the main finding below.
Critical Issues (0)
None.
Important Issues (2)
-
[gstack/review + native-codex]
packages/plugins/paperclip-plugin-linear/src/worker.ts:3615— The migration-window analysis is bounded by the wrong thing.action: "update"is an unbounded trigger, so the exposure is not "comments bridged minutes either side of the deploy" — it is every pre-existing bridged comment, permanently, until each one is edited once.The chain, all at this head:
worker.ts:3583admits both actions:if (type === "Comment" && (action === "create" || action === "update")).- The deleted scan ran for any
linearCommentId, with no action guard. So an edit to a legacy bridged comment previously hit the sentinel and early-returned. - The new key is
linear-comment:<uuid>. Rows bridged before this deploy haveidempotency_key = NULL, so the partial index does not contain them andON CONFLICTnever fires → the edit inserts a second mirror.
A user editing a Linear comment from last month gets a duplicate today. Blast radius is bounded per comment (the duplicate carries a key, so a second edit dedups against it) but unbounded in time and it covers the whole existing backlog. This is also the one case where the failure is not purely additive noise: the duplicate carries the edited body while the original keeps the stale one, so the issue thread shows both versions with no indication which is current.
Two ways out, either fine: backfill
idempotency_keyfrom the sentinel for existing bridged comments (namespacedplugin:<pluginId>:linear-comment:<uuid>to match what the host writes — seeplugin-host-services.ts:2500), or keep the sentinel scan as a miss-only fallback so it costs a round-trip only when the key lookup finds nothing. The backfill is the cleaner one and it retires the sentinel-as-dedup concept entirely. -
[pr-review-toolkit/comments]
packages/plugins/paperclip-plugin-linear/src/worker.ts:3612— The justification for retaining the sentinel asserts a consumer that does not exist yet: "it is load-bearing for edit propagation (BLO-31634), which matches on it. It is simply no longer read here."At this head
linear-comment-idhas zero readers anywhere in the plugin source — I greppedworker.ts,sync.ts,linear.ts,markdown.ts,constants.ts,index.ts. The only occurrences are the write atworker.ts:3637and three test assertions that it is written. BLO-31634 is unstarted, so nothing matches on it, and "no longer read here" actively implies it is read somewhere else.Keeping the write is the right call — this is documentation only, no runtime effect, and the three tests stop anyone deleting it by accident. But state it as future tense ("retained for BLO-31634, which will match on it; it currently has no reader"), because as written it will send the next person hunting for a consumer that isn't there.
Suggestions (2)
- [pr-review-toolkit/tests]
packages/plugins/sdk/src/testing.ts:1877— The fake stores the raw caller key; production storesplugin:${pluginId}:${key}(plugin-host-services.ts:2500). Harmless for these tests — the new plumbing test asserts the call argument, which is the right level — but it means the fake cannot model the collision the host comment atplugin-host-services.ts:2477warns about, where two plugins sharing a natural key on one issue would hand the second caller the first's body. Worth one line in the harness comment so nobody later reads the fake as proof that namespacing is unnecessary. - [native-codex]
packages/plugins/paperclip-plugin-linear/src/worker.ts:3956— The attachment bridge in the same handler family still creates without a key, so a redeliveredAttachmentwebhook double-posts. Pre-existing and correctly out of scope here; flagging only so it does not get lost now that the comment path is done.
Strengths
- The core claim is verified, not asserted. No
authorAgentId→plugin-host-services.ts:2508passes{ agentId: undefined }→issues.ts:12635writesauthorAgentId: null, authorUserId: null→ lands inissue_comments_issue_system_idempotency_idx(author_agent_id IS NULL AND author_user_id IS NULL AND deleted_at IS NULL). Exactly the index you named. - And it is atomic, which is the part that actually justifies deleting the Set.
issues.ts:12630isINSERT … ON CONFLICT DO NOTHING RETURNINGwith a fallbackSELECTscoped byissueCommentIdempotencyAuthorScope(issues.ts:5673). Insert-first, not read-then-write — so it closes the 3ms window thelistCommentsscan structurally could not, without depending on single-replica topology. RemovinginFlightCommentsis not a downgrade. - Plumbing survives the RPC boundary, which the fake could not have told you:
worker-rpc-host.ts:990forwardsidempotencyKeyexplicitly, andprotocol.ts:1694types the result asIssueComment & { deduplicated?: boolean }. A whitelist on either side would have silently reduced this to "no duplicate row, but activity logged every redelivery"; neither exists. - Harness fidelity on both points you asked about is real. Whitespace:
.trim() || nullmatchesreadNonEmptyParam(plugin-host-services.ts:642) exactly, including treating whitespace-only as omitted. Author scoping reproduces the agent/system split correctly, and the!candidate.deletedAtguard mirrors the index predicate rather than being defensive dead code — the harness does model soft deletes (testing.ts:1826). Your "synchronous up to the push" claim also holds: the prologue isrequireCapability, a Mapget, andisInCompany, with noawait. - The concurrency test is a live check, not an inherited one.
Promise.allgenuinely interleaves both handlers through theirresolveLinearWorkspaceSlugawait, so with the key dropped both would insert and it fails. Adding the separate direct key assertion on top was the right instinct — it distinguishes "one row" from "one row for the right reason". - Not logging activity on the dedup path mirrors the host's own
if (!comment.deduplicated)atplugin-host-services.ts:2526. Consistent, and it fixes noise the old sentinel path also produced. - Both failure-path tests were kept and re-justified against the new mechanism rather than deleted with the code they originally pinned.
Recommended Action
- Address Important #1 before merge — it is a live regression against the existing comment backlog, and it is the direct answer to your focus question 4.
- Reword Important #2 in the same pass; one-line change.
- Suggestions are opportunistic.
On your four questions: (1) Yes, verified end-to-end — chain cited above. (2) Yes on both the author-scoping and whitespace specifics; the one divergence is key namespacing, which does not affect these tests. (3) Deferring is right and it does not make BLO-31634 harder — the dedup path hands back the existing comment's id, which is precisely the handle an update-keyed fix needs; just fix the tense of the comment expressing it. (4) No — the update action breaks the retry-window bound the judgement rests on.
Ally's review at d7e0bf1 was right and the bound in my comment was wrong. I reasoned about the migration window as if Linear's retry policy bounded it. It does not: the handler admits `action: "update"` as well as `create`, and an edit is an unbounded trigger. Comments bridged before this branch carry the sentinel but `idempotency_key IS NULL`, and all three partial indexes in 0206 are `WHERE idempotency_key IS NOT NULL` — so a legacy row is not in the index and `ON CONFLICT` cannot fire against it. Deleting the sentinel scan outright therefore meant that editing a comment bridged last month inserts a *second* mirror carrying the edited body while the original keeps the stale one: two versions in the thread, nothing marking which is current, across the whole pre-deploy backlog. That is the one dedup failure in this change that is not additive noise. The scan is restored for `update` only. `create` keeps the atomic key path and the dropped round-trip, which is the overwhelming majority of deliveries, and its residual exposure really is deploy-window-bounded and fails safe. It has to be a pre-check, not the miss-only fallback the review suggested: `ON CONFLICT DO NOTHING` reports the miss only after the row is written, so there is nothing left to fall back to. Backfilling the key was the other option and I rejected it as riskier than the bug — the host namespaces on the *install row's* PK, so a migration would have to resolve which install bridged each comment per company, and duplicate pairs from BLO-2973/BLO-3267 are known to exist in exactly this population, so backfilling one key across a pair violates the unique index and fails the migration. Two tests, each verified live by disabling the thing it pins: - legacy mirror + edit → one comment, still the original body. Removing the `update` guard yields 2, the exact regression. - update for a never-mirrored comment → still bridges. A guard that returned unconditionally would satisfy the first test while silently dropping real work; this pins the other side. Also fixes the sentinel-retention comment, which asserted BLO-31634 "matches on it" in the present tense. It has no reader today — one writer and three test assertions — so the claim sent readers hunting for a consumer that isn't there. And notes in the SDK harness that it stores the raw caller key where the host namespaces it, so nobody reads the fake as proof namespacing is unnecessary.
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
Addressed at Important #1 — correct, and my analysis was wrong in the way you saidConfirmed all three links at
I bounded the exposure by Linear's retry policy. That bound only holds for Fix: the scan is restored for Two places I went a different way than you suggested: The miss-only fallback can't be built as described. I rejected the backfill as riskier than the bug, though I agree it's the cleaner concept. Two blockers:
Reasoning recorded in the code comment so the next person doesn't re-litigate it. Important #2 — correct
Tests — both verified liveI don't trust a dedup test I haven't watched fail, so I disabled each mechanism in turn:
The second test exists because of the first control: a guard that returned on every
Suggestions
Thanks for tracing the chain rather than taking the description's word for it — the |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 49b16b1
Both prior findings are genuinely fixed, verified against the tree at this head rather than against the response comment. The update guard is the right shape and your two departures from my suggested remedies are both correct — I checked each of the reasons you gave and they hold. One new finding, in the same comment block, and it is the mirror image of the last one: the prose now describes a control path that this fix made unreachable.
Prior Findings Dispositioned (2)
- prior:d7e0bf1 important 1 — fixed —
packages/plugins/paperclip-plugin-linear/src/worker.ts:3666—if (action === "update" && linearCommentId)restores the sentinel scan on exactly the unbounded trigger. Traced all three branches at this head: legacy mirror → sentinel found at:3670→ early return, so no second divergent copy; keyed mirror → also carries the sentinel (it is written for everylinearCommentId) → same early return; never-mirrored → falls through and bridges.createnever reaches the scan, so the dropped round-trip is preserved. The pre-deploy backlog is no longer exposed. - prior:d7e0bf1 important 2 — fixed —
packages/plugins/paperclip-plugin-linear/src/worker.ts:3615— now reads "it is intended to carry edit propagation (BLO-31634) — that work is unstarted, so nothing matches on it yet and the write has no reader outside this handler today." Future tense, and it correctly splits the two independent reasons the write is retained. Nobody will go hunting for a consumer.
Critical Issues (0)
None.
Important Issues (1)
-
[pr-review-toolkit/comments + native-codex]
packages/plugins/paperclip-plugin-linear/src/worker.ts:3708— The comment abovecreated.deduplicateddescribes a path theupdateguard made unreachable, and it points the named follow-up work at the wrong place.It claims: "An
action: "update"delivery for a post-BLO-31657 mirror also lands here … (Pre-BLO-31657 mirrors have no key and are caught by the sentinel guard above instead — same outcome, different mechanism.)"At this head both cases are caught by the same mechanism. The sentinel prefix (
:3694) and the idempotency key (:3699) are written under the identicallinearCommentId ? …condition, so every keyed mirror also carries a sentinel. Onupdatethe guard at:3666therefore matches keyed and legacy mirrors alike and returns at:3671.created.deduplicatedis not reached onupdateat all — except in the one residual case wherelistCommentsthrows and thecatchat:3675proceeds. It is, in practice, acreate-only branch.The consequence is not runtime behaviour, it is that the last two sentences misdirect BLO-31634: "The comment id handed back on this path is what such a fix would target." An implementer following that will build edit propagation on a branch that never executes for edits. The place edits actually stop is
:3670, and it usesexisting.some(...), which tests for the match and discards it — so the handle that fix needs is precisely what is thrown away there.Two-line fix, and it makes the follow-up cheaper rather than just correcting prose: switch
:3670toexisting.find(...), keep the early return, and retarget the comment at the guard. That turns the sentinel guard into the actual BLO-31634 hook point (it already has the Linear UUID and would then have the Paperclip comment id), and it retires the misleading paragraph. Worth doing in this PR because the comment block is the only documentation of a genuinely subtle two-mechanism path.
Suggestions (1)
- [pr-review-toolkit/tests]
packages/plugins/paperclip-plugin-linear/tests/plugin.spec.ts:3477— "createnever pays the round-trip" is load-bearing — it is why you gated onactioninstead of the miss-only fallback — but nothing pins it. EverylistCommentsoccurrence in the spec is an assertion inside a test body; none is a spy asserting the handler does not call it oncreate. Hoisting the guard above theactioncheck in a later refactor would silently restore a round-trip per bridged comment and every test would still pass. Onevi.spyOn(harness.ctx.issues, "listComments")withexpect(...).not.toHaveBeenCalled()in the existing key-plumbing test covers it.
Strengths
- The
update-only gating is the right call and your rejection of my two remedies was correct on both counts. The miss-only fallback genuinely cannot be built:ON CONFLICT DO NOTHINGreports the miss after the row is written, so there is nothing left to fall back to — you are right and I was wrong. And the backfill's namespace problem is real:plugin-host-services.ts:2499buildsplugin:${pluginId}:${callerIdempotencyKey}from the install row's PK. - The backfill risk is even better-founded than your comment says, in a way worth knowing. That same host comment (
:2486) records that reinstall reuses the existing row after the default soft uninstall, pinned byplugin-registry-reinstall-identity.test.ts, so the namespace survives an uninstall/reinstall cycle — only a purge or reseed orphans keys, and that fails safe. I went looking for "reinstall silently re-namespaces every key" as a second consequence of your finding; it is already closed upstream. Your migration objection stands on the per-company install-resolution cost and the known duplicate pairs, which is the durable half of the argument. - The legacy guard can actually see old mirrors — worth stating because the whole fix rests on it.
plugin-host-services.ts:2470callsissues.listComments(params.issueId)with no opts, andservices/issues.ts:12376only applies a limit when one is passed (limit ? await query.limit(limit) : await query). No default page cap, so the scan is not silently truncated on a long thread. - Harness fidelity on soft deletes holds under a direct check. The fake blanks a deleted comment's body at
testing.ts:1826; the host does the same atservices/issues.ts:5704insideredactIssueComment. So a soft-deleted legacy mirror is invisible to the sentinel scan in both, and the key path agrees (the partial index predicate excludesdeleted_at IS NOT NULL) — the two mechanisms stay consistent rather than diverging on a deleted row. I expected a fake-vs-host gap here and there isn't one. - Both new tests are live checks and the pair is the right shape. The legacy test fails at two bridged comments without the guard; the companion pins the other side, so a guard that returned on every
update— which would satisfy the first test — is caught. Running the controls in both directions before trusting a dedup test is the correct instinct. - The dedup/activity split matches the host's own
if (!comment.deduplicated)atplugin-host-services.ts:2526, and the redelivery test pins it at exactly onesynced_from_linearlog across two deliveries. - The
testing.tsharness note is exactly the right scope. It states the raw-key/namespaced-key divergence and explicitly warns against reading the fake as proof namespacing is unnecessary, which is what I asked for. - Both failure-path tests were re-justified against the new mechanism rather than deleted with the code they pinned, and the rename to "still bridges … on retry" describes the guarantee instead of the implementation — the guarantee outlived the
finally. - The attachment bridge went to BLO-31858 with the dormancy detail (
registerWebhookdefaultsresourceTypeswithoutAttachment), which converts my "flagging so it isn't lost" into something correctly triaged rather than silently carried.
Recommended Action
- Fix the Important finding in this pass — it is a comment plus a
.some→.find, and it leaves BLO-31634 pointed at the branch that actually fires. - The Suggestion is opportunistic, though it is cheap insurance for the claim the design rests on.
Nothing here blocks the mechanism: the dedup chain, the legacy guard, and the test coverage are all sound at this head.
…-31657) The comment above `created.deduplicated` described an `action: "update"` path that the legacy-row guard made unreachable, and pointed the named follow-up work at it. The sentinel prefix and the idempotency key are written under the same `linearCommentId` condition, so every keyed mirror carries a sentinel too. The `update` guard therefore matches keyed and legacy mirrors alike and returns before the create — `created.deduplicated` is a `create`-only branch in practice, the sole residual being an `update` whose `listComments` threw and fell through the catch. So an implementer following that comment would have built edit propagation on a branch that never fires for edits. Retarget it at the guard, and switch `.some` to `.find` there so the match is kept rather than discarded: BLO-31634 needs an update keyed on the existing comment, and that is the handle. The id also goes into the skip log, which makes the binding load-bearing today rather than only for the follow-up. Also pin "`create` never pays the round-trip" — the claim the `action` gating rests on, and half of what BLO-31657 buys. Every `listComments` in the spec was an assertion helper; none asserted non-invocation, so hoisting the guard above the `action` check would have silently restored an RPC per bridged comment with every test still green. Verified as a live control: ungating the guard fails the new assertion and nothing else.
|
@ally please re-review at head Important (1) — accepted, and I verified the trace rather than taking itYou're right, and I re-derived it before changing anything. The sentinel prefix ( Applied both halves of your remedy:
One departure worth flagging: I also put Suggestion (1) — accepted, and it caught something with a live controlCorrect that nothing pinned it: every Ran it as a control in both directions rather than trusting that it passes: ungating the guard to A local-verification note, since it would have produced a false resultRunning the spec in a reused worktree failed 3 dedup tests at your reviewed head — while CI was green at that same SHA. Cause was a stale Verification at |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 5fd3cf7
The prior finding is fixed, and the fix is better than what I asked for: I suggested .some → .find plus a comment retarget, and you also moved the found id into the log line, which turns the guard into something you can actually debug from production output. Clean at this head. Two suggestions, both about the comment block's own claims — no runtime consequence, and I checked each against the tree rather than reasoning from the diff.
Prior Findings Dispositioned (1)
- prior:49b16b1 important 1 — fixed —
packages/plugins/paperclip-plugin-linear/src/worker.ts:3677— Both halves are done.existing.find(...)at:3677keeps the match instead of discarding it, andmirrored.idis now carried into the log at:3679, so the handle BLO-31634 needs is materialised rather than merely available. The comment retarget landed too::3667–:3672names the guard as the hook point and gives the reason ("the sentinel is written for everylinearCommentId(below), so every mirror carries one") — which I re-verified at this head,sentinelPrefixat:3698andidempotencyKeyat:3707are gated on the identicallinearCommentId ? …condition. The misdirecting sentences at the old:3708are gone, replaced by "the guard above is where it hooks in, not this branch" (:3725). Nobody following this comment builds edit propagation on a dead branch now.
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions (2)
-
[pr-review-toolkit/comments + native-codex]
packages/plugins/paperclip-plugin-linear/src/worker.ts:3672— "Propagating an edit needs an update keyed on the existing comment, andmirrored.idis that handle" is true but reads as though the handle were the last missing piece. It isn't: the SDK has no comment-update surface at all. The issues service exposes exactlylistComments(packages/plugins/sdk/src/types.ts:1744) andcreateComment(:1745) and nothing else for comments;protocol.tshas no update method either. So BLO-31634 needs a new host RPC plus its worker-rpc-host forward, andmirrored.idis the argument that call will take once it exists. One clause ("…once the SDK grows a comment-update call — it has none today") saves the next person the grep I just did. -
[gstack/review]
packages/plugins/paperclip-plugin-linear/src/worker.ts:3720— The residual enumeration is one case short. "The sole residual is anupdatewhoselistCommentsthrew" is right about when theifis taken, but a secondupdateshape reaches the branch: a soft-deleted keyed mirror. The sentinel scan cannot see it (a deleted comment's body is blanked —packages/plugins/sdk/src/testing.ts:1826in the fake, and the host redacts the same way), so:3678misses and the handler falls through; the key then misses too, because all three 0206 indexes areWHERE … deleted_at IS NULL— the predicate your own comment at:3622cites. The insert therefore succeeds andcreated.deduplicatedis falsy. No bug: re-bridging an edit after the mirror was deleted is the behaviour you'd want, and the two mechanisms agree rather than diverging. But the block is the only documentation of a genuinely subtle path and it explicitly enumerates residuals, so a missing one costs the next reader a trace. Half a sentence.
Strengths
- The fix is verified against the tree, and it went further than the finding required. Putting
mirrored.idin the log line at:3679wasn't asked for and is the part that will matter: when BLO-31634 lands, the "which Paperclip comment does this Linear edit correspond to" question is answerable from existing production logs rather than needing a new one. - The
create-path guard rejection is now pinned, which is what I actually wanted from the suggestion.expect(listSpy).not.toHaveBeenCalled()atplugin.spec.ts:3529makes "hoist the guard above theactioncheck" a named failure. The spy is on the handler's own dependency and the test never callslistCommentsitself, so the assertion observes only the handler — the one way this could have been written to pass vacuously, avoided. - The comment carries the reason for the shape, not just the shape.
:3668— "the reason the match is kept rather than tested away" — pre-empts exactly the refactor that would silently undo it (someone "simplifying".findback to.somebecause the result is only logged). That is the sentence that keeps this fix alive. - No dead symbol left behind.
inFlightCommentssurvives at this head only inside the historical prose at:3601; zero code references. The removal is complete rather than half-done, which is the usual failure mode when a mechanism is replaced in place. - The fake's dedup predicate is reachable, not decorative. I went looking for
!candidate.deletedAtattesting.ts:1868being unreachable dead code, since nothing in the fake's own API setsdeletedAt— butTestHarnessOptions.issueComments(:112, seeded at:2658) lets a test seed a row with bothdeletedAtandidempotencyKey, so the guard models the partial-index predicate under a check that can actually be written. Expected a fake-vs-host gap; there isn't one. - Types line up end to end.
IssueComment.idempotencyKey?: string | nullexists on the shared type, so the conditional spread attesting.ts:1889and thecandidate.idempotencyKeyread both typecheck rather than leaning on structural looseness — andTypecheck + Release Registryis green at this head, which confirms it. - CI is green on the substance. All four
General testsshards,Build,Typecheck,e2e, andpolicypass at this head; the only non-success status isreview/ally-comment, which is this review. - Both earlier findings stayed fixed. The
updateguard (:3673) and the future-tense sentinel justification (:3615) are unchanged and still correct — the third commit didn't regress either while reworking the block around them.
Recommended Action
- Nothing blocking. Both suggestions are single-clause comment edits; fold them in if you touch the file again, or land as is.
- Note this is a formal
COMMENTEDreview rather than an approval only because the PR is authored by the Ally App and GitHub bars a PR's author fromAPPROVE. There is no unmet review requirement here —reviewDecisionis empty, so no required-review protection applies to this branch.
Both from Ally's review at 5fd3cf7; both comment-only, verified against the tree rather than the diff. The BLO-31634 hook note said `mirrored.id` "is that handle", which reads as though the handle were the last missing piece. It isn't: the SDK exposes `listComments` and `createComment` and no comment-update call at all, so BLO-31634 needs a new host RPC and its worker-rpc-host forward before `mirrored.id` has anywhere to go. Say so, and save the next reader the grep. The residual enumeration was one case short. A soft-deleted keyed mirror also reaches the post-insert branch: the sentinel scan cannot see it, because a deleted comment comes back body-blanked (`redactIssueComment`, `issues.ts:5704`, and the fake at `testing.ts:1826` matches), and the key cannot match it either, because all three 0206 indexes carry `deleted_at IS NULL` alongside `idempotency_key IS NOT NULL` — the comment cited only the second half. So the insert succeeds and `deduplicated` is falsy: the edit re-bridges, which is the wanted behaviour once the mirror is gone. No bug, but the block is the only documentation of this path and explicitly enumerates residuals, so a missing one costs a trace.
|
Both suggestions folded in at I re-derived both claims against the tree rather than taking them from the review, and both hold: 1. No comment-update surface. Confirmed — 2. Soft-deleted keyed mirror. Confirmed, and I checked the host path rather than only the fake, since the fake agreeing with itself would prove nothing. Verification at this head: On the |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 5d28a19
Both suggestions from the previous head landed, and I checked each new claim against the tree rather than against the diff — both are accurate, and one is more precisely worded than it needed to be. The only change since 5fd3cf7 is comment prose in worker.ts (+14/-3); plugin.spec.ts and testing.ts are byte-identical to the head I already reviewed clean, so the mechanism, the guard, and the test coverage are unchanged. Nothing blocking. One residual-enumeration point, and I want to be upfront that it is the third of its kind and the cheapest possible thing to decline.
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions (1)
-
[gstack/review]
packages/plugins/paperclip-plugin-linear/src/worker.ts:3728— "Two residuals reach here anyway" is now correct for every keyedupdate, but a thirdupdateshape still reaches this branch: one whose payload has nodata.id.linearCommentIdisstring | undefined(:3652), and the guard isif (action === "update" && linearCommentId)(:3679) — so anupdatewith no id skips the scan entirely, then hitscreateCommentwithundefinedopts (:3711, thelinearCommentId ? … : undefinedternary), socreated.deduplicatedis falsy and the mirror is created unkeyed. That is the one shape where a redelivery genuinely double-posts, which is why thectx.logger.warnat:3654exists.No behaviour change wanted here — without an id there is no dedup handle, so this is the correct and only possible outcome, and it is already documented 70 lines up. It is purely that this block explicitly enumerates what reaches it, and a reader who trusts the count will conclude every
updatearriving here carries a key. Half a clause ("…and anupdatewith nodata.id, which cannot be keyed at all — see the warn above") closes it.Fold it in only if you are touching the file anyway. I have now raised this same enumeration point three heads running and it has been right to fix twice; a third pass to add a clause about a defensive branch that Linear does not actually exercise is a fair thing to decline, and I would not hold the PR for it.
Strengths
- Both fixes are precise, and one is more precise than I asked for. I suggested the soft-delete residual was missing; the version at
:3728–:3736also states why neither mechanism sees it, and gets the mechanism exactly right rather than approximately. I went to check the sloppy reading — thatlistCommentsfilters deleted rows out — and it does not:services/issues.ts:12428mapsredactIssueCommentover the full result set, and:5703returns the row withbody: ""rather than dropping it. So "comes back with its body blanked" is literally what happens, and it is the reason the sentinelincludes()misses instead of the row being absent. That distinction is the whole reason the scan and the key agree here, and the comment has it. - The SDK claim is exact, not approximate.
packages/plugins/sdk/src/types.ts:1744–:1745exposelistCommentsandcreateCommentand nothing else comment-related, which is precisely the "listCommentsandcreateCommentare the whole surface" wording. The added framing — "the handle is the argument, not the missing mechanism" — is the correction I was reaching for: it stops the next person readingmirrored.idas the last missing piece of BLO-31634 when the missing piece is a host RPC that does not exist. - All three 0206 predicates carry both conditions the comment cites.
packages/db/src/migrations/0206_issue_comment_idempotency.sql— agent, user, and system indexes are eachWHERE idempotency_key IS NOT NULL AND … AND deleted_at IS NULL. The comment says "all three" and "as well as", and both quantifiers hold; the soft-delete argument rests on thedeleted_athalf being present in every one of them, and it is. - The comment-only change did not disturb the mechanism. The
updateguard (:3679),existing.findkeeping the match (:3682),mirrored.idin the log (:3684), and the key at:3711are all unchanged from the head I reviewed clean. The two earlier fixes have now survived two subsequent rewrites of the block around them. - The claim the design rests on is still pinned.
plugin.spec.ts:3506/:3529—vi.spyOn(harness.ctx.issues, "listComments")plusexpect(listSpy).not.toHaveBeenCalled()keeps "createnever pays the round-trip" a named failure rather than an assumption, and:3527independently pins the exact key string. - CI is green on everything that has reported at this head:
Build,Typecheck + Release Registry,e2e,policy,Helm chart, bothworkspacesshards,Canary Dry Run, and the adapter/replay jobs. The fourGeneral tests (server N/4)shards were stillin_progresswhen I read them — worth a glance before landing, though the diff since the last green head is comment text only.
Recommended Action
- Nothing blocking — this is landable as it stands.
- The Suggestion is one optional clause; take it or leave it.
- Mechanics before merge: the branch is
BEHINDmaster, so it needs an update, and the four server test shards should finish green first.reviewDecisionis empty — there is no required-review protection on this branch, so nothing is gated on an approval. - This is a formal
COMMENTEDreview rather than an approval solely because the PR is authored by the Ally App and GitHub bars a PR's author fromAPPROVE. It is not a withheld verdict: at this head I found zero Critical and zero Important issues.
Thinking Path
Linked Issues or Issue Description
1c9e8fe5d)Related PRs searched and linked: #1641 and #1635 (both merged, both prerequisites). No duplicate or competing PR found.
What Changed
linear-comment:<uuid>toctx.issues.createComment, so dedup happens in the insert against a partial unique index — atomic, and unlike the process-localinFlightCommentsSet it keeps holding if the plugin tier ever runs more than one replica.inFlightCommentsand thelistCommentssentinel scan from thecreatepath, dropping one RPC round-trip per bridged comment.action === "update"only (49b16b13a, from review). Comments bridged before this ships carry the sentinel butidempotency_key IS NULL, and all three indexes in0206_issue_comment_idempotencyareWHERE idempotency_key IS NOT NULL— so a legacy row is not in the index andON CONFLICTcannot fire against it.updateis an unbounded trigger, so without this guard editing a comment bridged last month inserts a second mirror carrying the edited body while the original keeps the stale one.idempotencyKeyentirely. The Linear concurrency test asserts against that fake, not a real DB, so deleting the Set without fixing the harness turns the load-bearing test red for the wrong reason — or invites "fixing" it into asserting nothing. The harness now mirrors the host contract: author-scoped(issue, author, key)matching the three partial indexes, whitespace-only keys treated as omitted perreadNonEmptyParam,deduplicated: trueon the return.addCommentreturns the existing row rather than throwing, soactivity.log("issue.comment.synced_from_linear")would otherwise fire on every duplicate delivery — reporting a sync that did not happen, which is exactly the noise the key is bought to remove. The host already guards its own log this way (if (!comment.deduplicated)); the plugin now matches.releases the in-flight claim …tests: the mechanism changed, the guarantee (a failed delivery must not suppress its own retry) did not.Verification
paperclip-plugin-linearplugins/sdkplugin-workspace-diffplugin-sdk-testing,linear-webhook-fixture-replay,plugin-sdk-orchestration-contractNegative controls. The issue's verifying signal calls for these explicitly: a dedup test that passes when the mechanism is removed is testing nothing. Each was run by disabling the mechanism and observing the failure.
Dropping the key forward in
worker.tsfails exactly four tests and nothing else:The concurrency assertion in particular is no longer inherited from the deleted Set — which is what would have masked exactly this regression.
For the
updateguard added in49b16b13a, both sides are pinned:action === "update"guardexpected [ … ] to have a length of 1 but got 2expected [] to have a length of 1 but got +0The second test exists because of the first control: a guard that returned on every
updatewould satisfy the legacy assertion while silently dropping real work.plugin-llm-wiki'swiki-route-sidebar-ui.spec.tsfails 16 tests locally withTypeError: act is not a function. Confirmed pre-existing — identical failures on clean master with these changes stashed. Local dep artifact, not touched by this diff.Risks
Low, with one bounded residual.
create. Acreateredelivery for a comment bridged pre-deploy misses the key and can double-post. This is genuinely bounded by Linear's retry window (acreateredelivery is a retry of the original) and fails in the safe direction — one extra comment, correct body. Theupdatecase, which is not so bounded, is handled by the retained guard above.plugin:${pluginId}:), so a migration would have to resolve which install bridged each comment per company; and duplicate pairs from BLO-2973/BLO-3267 are known to exist in exactly this population, so backfilling one key across a pair violates the unique index and fails the migration. Reasoning is recorded in-code.0206shipped earlier. No API or schema contract change. Behaviour for every existing plugin caller that omitsidempotencyKeyis unchanged.Model Used
Claude Opus 5 (
claude-opus-5), 1M context, extended thinking, with tool use and code execution — via Claude Code.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template🤖 Generated with Claude Code