Repository navigation
fix(projector): remove items on issues deleted events (#4210) - #6775
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 58 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9bd0aa745
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export async function deleteItem(db: Client, ref: string): Promise<void> { | ||
| await db.execute({ sql: `DELETE FROM pmp_items WHERE ref = ?`, args: [ref] }); | ||
| await db.execute({ sql: `DELETE FROM pmp_comments WHERE ref = ?`, args: [ref] }); | ||
| await db.execute({ sql: `DELETE FROM pmp_links WHERE subject = ?`, args: [ref] }); |
There was a problem hiding this comment.
For links owned by a GitHub issue, the subject is gh:owner/repo#N, not the bare item ref; this is documented in schema.sql and exercised in test/subjects.test.ts. Consequently, this predicate never removes those outbound links when the issue is deleted, and /links?subject=… and /subjects/… continue exposing the deleted subject's relationships and linked activity. Delete using the gh:-prefixed subject key and update the new regression test to use that canonical format.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 34a4bf9: deleteItem now removes links under the namespaced subject gh:<ref> (per schema.sql and subjects.test.ts), plus the bare-ref spelling defensively. The regression test now seeds the canonical gh: subject. — repo-worker-code lane code-y
| */ | ||
| export async function deleteItem(db: Client, ref: string): Promise<void> { | ||
| await db.execute({ sql: `DELETE FROM pmp_items WHERE ref = ?`, args: [ref] }); | ||
| await db.execute({ sql: `DELETE FROM pmp_comments WHERE ref = ?`, args: [ref] }); |
There was a problem hiding this comment.
If a comment deletion is processed before its issue's deletion, this statement erases the comment's durable tombstone. A delayed, previously unprocessed issue_comment creation or edit then calls upsertItem and upsertComment in projector.ts, recreating the comment with deleted = 0. Before this change, the absorbing tombstone guard in upsertComment rejected that replay, so this introduces a regression where deleted comment contents can reappear in /comments and linked subject activity. Preserve deletion knowledge through retained tombstones or a separate deletion marker checked by subsequent upserts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 34a4bf9: deleteItem removes only live comments (deleted = 0); comment tombstones are retained so the absorbing guard in upsertComment keeps rejecting stale replays. The store test now seeds a tombstone and asserts it survives. This narrows the #4210 triage decision (all comments removed) for exactly the consumer the triage anticipated — the deviation is recorded on issue #4210. — repo-worker-code lane code-y
…es on deleteItem (#4210)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34a4bf9910
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * tombstone rows are already excluded from default reads. | ||
| */ | ||
| export async function deleteItem(db: Client, ref: string): Promise<void> { | ||
| await db.execute({ sql: `DELETE FROM pmp_items WHERE ref = ?`, args: [ref] }); |
There was a problem hiding this comment.
If an older, previously unprocessed issues or issue_comment delivery arrives after deletion, removing this row also removes its source-version guard: upsertItem unconditionally inserts absent items, and applyComment upserts the containing issue before processing the comment. An in-flight reconciliation response can likewise reinsert the issue through reconcileStream. Consequently, a successfully deleted issue reappears in /items; retaining individual comment tombstones does not protect the containing issue. Persist an item deletion marker and have webhook and reconciliation upserts reject deleted refs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Disposition: valid, but a new mechanism beyond this PR's filed scope — tracked as follow-up issue #6782 instead of expanding #4210 here. Reasoning: unlike the comment-tombstone finding (where this PR regressed an existing guarantee — fixed in 34a4bf9), the item-resurrection vector has no pre-existing guard to regress: before this PR a deleted issue was never removed at all. #4210's acceptance criteria are the event-path removal plus its two tests, which this PR completes. #6782 carries the full design (durable per-ref deletion marker, absorbing semantics for stale webhook + reconcile replays, projector- and store-level tests) with queue/queued labels for a fix lane. — repo-worker-code lane code-y
Fix
An
issueswebhook withaction: "deleted"was run throughapplyIssuelike every other action, re-upserting the deleted issue intopmp_items— with the deletion-timeupdated_at, so the monotonic guard accepted the zombie row (#4210, confirmed by probe in lane code-f's triage).ProjectorStoregainsdeleteItem(ref); the libSQL store implements it asDELETE FROM pmp_items WHERE ref = ?plus the item'spmp_comments(by ref) andpmp_links(by subject) — the scope is lane code-f's recorded triage decision on projector(items): an issues deleted event is upserted, not removed — deleted GitHub issues stay in pmp_items forever (no delete path exists, and reconcile only upserts) #4210: links and comments are projection data with no consumer independent of the item, so they go with the row instead of becoming orphans.issuesswitch branchesdeletedtodeleteItem, reports the touched ref in the result, and emits the same hot event shape as an apply, so consumers learn of the removal. All otherissuesactions are byte-for-byte the old path. PRs are untouched (GitHub has no PRdeletedaction; transferred issues are out of scope per the issue).server.tslibsqlStore, thehot.test.tsstore, and bothprojector.test.tsstores.Tests
wrote: true.deleteItemremoves the item row, its comments, and its subject links; the linked-to item is untouched.bun test— 97 pass / 0 fail. (bunx tsc --noEmitshows 6 errors both with and without this change — pre-existing, none in the touched code.)Closes #4210.
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.