Guard the threading edge from the replied-to comment, not only the reply (#60)

[mayankguptadotcom]

Closes #60.

The four triggers from #29 all open their WHEN with NEW.parent_id IS NOT NULL, so they only ever see the row that is a reply. Updating a comment that has replies never touches that column, so nothing objected to either half of the mirror image:

-- its replies stay on page A, pointing at a parent that has left
update comments set thread_id = <page B> where id = <root with replies>;
-- everything under it is now three deep, while the replies' own depth still reads 1
update comments set parent_id = <another root>, depth = 1 where id = <root with replies>;

Two more triggers close it from that end, keyed on EXISTS (SELECT 1 FROM comments WHERE parent_id = OLD.id).

The decision, and why the guard is worth it

The cost is a seek on comments_by_parent per UPDATE whose SET clause names the guarded columns. It is affordable because of what does not reach it:

• src/db writes exactly two UPDATEs to comments — moderation (status, moderated_at, on a comment and its replies) and the nightly ip_hash purge (whole table). Neither names parent_id, thread_id or depth, so UPDATE OF does not fire these triggers at all. The one whole-table statement in the codebase pays nothing.
• Nothing writes those three columns after the insert today, so the seek lands only on the feature that first moves a comment between threads.

Measured, not argued. D1's rows_read for a guarded-but-allowed update:

Statement	rows_read
move a childless comment to another page, on a page with 1 reply	3
move a childless comment to another page, on a page with 60 replies	3
the same statement with both new triggers removed from the migration	3

So on the allowed path the guards cost zero extra rows read, and the figure does not move as the page grows.

That measurement needed its own control, because "the guards read nothing" and "rows_read cannot see a trigger" produce identical numbers and only one is good news. test/worker/db/threading-triggers.test.ts now pins the instrument first: an update firing no trigger reads 2 rows, an update whose trigger subquery must read the parent row reads 5. The metric does see trigger work, so the 3-vs-3 above means what it says.

The price, stated plainly

With these in place there is no ordering of UPDATE statements that moves a comment and its replies to another page in one pass — the parent is refused while its replies are behind, and each reply while its parent is. A merge-threads or move-comment feature has to detach the replies (parent_id NULL, depth 0), move every row, then re-attach. That is three steps, 2N+1 statements, and a test holds that route open so the guards cannot quietly become a frozen table.

This is deliberate. The alternative — reading the replies' own thread_id instead of comparing NEW.thread_id with OLD.thread_id, as #60 sketched — would accept or reject a multi-row UPDATE ... WHERE thread_id = ? according to the order SQLite happened to visit rows in. That is a guard that passes in a test and fails in production.

Found during review: a hole neither pair can see

A cold-context adversarial review of the branch constructed this, and I reproduced it:

update comments set parent_id = id, depth = 1 where id = <childless root>;
-- id=4, parent_id=4, depth=1

The row becomes a reply and a replied-to comment atomically. Both depth guards read the pre-update row, where nothing points at it yet; both thread guards are inert; both depth CHECKs pass; the self-FK is satisfied. src/render then files it under its parent and emits no root for it, so the comment and its subtree leave the page in silence.

A CHECK (parent_id IS NULL OR parent_id <> id) answers it, with no trigger and no runtime cost. Slightly beyond the letter of #60, but the migration comment this PR writes claims the edge is closed from both ends, and leaving the hole would have made that comment the exact defect CLAUDE.md warns about.

The same review verified these do not get through, with the new triggers in place: a multi-row set parent_id = case ... end building a 3-deep chain; two roots swapping into a 2-cycle; update comments set thread_id = ? where thread_id = ?; INSERT ... ON CONFLICT(id) DO UPDATE; UPDATE comments SET id = ? (self-FK, NO ACTION); INSERT OR REPLACE (replies cascade-delete rather than strand); UPDATE OR REPLACE ... SET depth = NULL.

SQLite semantics, cited

From https://sqlite.org/lang_createtrigger.html (checked 2026-07-25):

• "the trigger will only fire if column-name appears on the left-hand side of one of the terms in the SET clause of the UPDATE statement"

  Syntactic presence, not a value change. This is why moderation never reaches these triggers, and why a test asserts that writing a comment's existing thread_id back is allowed — the guard is asked, and has to decide it is harmless.

• "Due to an historical oversight, columns named in the 'UPDATE OF' clause do not actually have to exist in the table being updated. Unrecognized column names are silently ignored."

  So a rename disarms a guard with no error. Re-confirmed empirically in workerd below (kill-shot 5): the migration applies clean, and only the tests notice.

Also confirmed on this branch: BEFORE triggers run ahead of constraint checking. A depth-only set depth = 1 on a root with replies is rejected by the trigger's message, not the CHECK's. My first draft of the migration comment asserted the opposite ordering; the review caught it, and it is now corrected with a test that pins which mechanism answers.

EXPLAIN QUERY PLAN cannot be used here — it reports the plan of the UPDATE and says nothing about the trigger subprograms it runs (verified: the plan is unchanged even after dropping comments_by_parent). That is why the cost evidence is rows_read.

Kill-shots (card rule 6)

Each guard disabled in turn, pnpm vitest run --project worker test/worker/db/threading-triggers.test.ts, then restored.

Guard disabled	Result
dropped comments_depth_guard_on_update_with_replies	Tests 3 failed | 18 passed (21) — refuses to turn a comment that has replies into a reply itself, refuses a depth-only update…, leaves the comment a root when that update is refused
dropped comments_parent_thread_guard_on_update_with_replies	Tests 2 failed | 19 passed (21) — refuses to move a comment that has replies onto another page, leaves the comment and its replies on their own page when the move is refused
dropped CHECK (parent_id IS NULL OR parent_id <> id)	Tests 1 failed | 20 passed (21) — refuses to make a comment its own parent
removed depth from the depth guard's OF list	Tests 1 failed | 20 passed (21) — refuses a depth-only update on a comment that has replies, from the trigger
renamed thread_id → thread_id_renamed_by_a_future_migration in the OF list	Tests 2 failed | 19 passed (21) — same two as row 2. The migration applied without error; nothing but the tests noticed.

And the other half of rule 6 — the guards must not have frozen legitimate work. Every one of these passed in all five kill-shot runs and in the final green run:

Legitimate update	Test
moderation on a reply (status, moderated_at)	still lets a moderator change a comment status
moderation on a comment that has replies	still lets a moderator act on a comment that has replies
ordinary body edit	still lets a comment body be edited
re-parenting a reply within its page	still lets a reply move to another root comment on the same page
moving a comment with no replies to another page	still lets a comment with no replies of its own move to another page
writing back the page a comment is already on	still lets a comment with replies be written with the page it is already on
a whole conversation moving pages, detached and re-attached	still lets a whole conversation move pages, detached and re-attached

Amending 0001 rather than adding 0002_

Checked on 2026-07-25, all four still true, so no database anywhere has ever had this schema applied:

• Deploy to Cloudflare button and provisioning flow #16 (Deploy to Cloudflare button and provisioning flow) is open
• wrangler.jsonc still carries the placeholder "database_id": "00000000-0000-0000-0000-000000000000"
• git tag -l — empty
• gh release list — empty

Tests read the migrations directly (readD1Migrations in vitest.config.ts), so the amended file is what the suite ran against.

pnpm check

Test Files  47 passed (47)
     Tests  835 passed (835)
[ok] embed: src/embed/index.ts is 6791 B gzipped of 10240 B allowed
[ok] pnpm is the only lockfile here, its version is pinned to a major the Cloudflare build image runs (10.x), and its format is one that image reads (9.0)
[ok] every package licence is within policy
[ok] every action reference is pinned to a full commit SHA
No known vulnerabilities found
No known vulnerabilities found

Skills used: test-driven-development (every guard here had a failing test first), source-driven-development (the SQLite semantics above), security-and-hardening, performance-optimization (the cost decision), improve and code-review-and-quality over the branch — the latter with a cold-context reviewer, which is what found the self-parent hole and the backwards ordering claim — and superpowers:verification-before-completion.

🤖 Generated with Claude Code

[mayankguptadotcom]

Coordinator verification

The vanishing-subtree hole is the find here, and my kill-shot confirms it

Removing the CHECK (parent_id IS NULL OR parent_id <> id):

× refuses to make a comment its own parent
AssertionError: promise resolved "{ success: true, meta: { …(8) }, …(1) }" instead of rejecting
      Tests  1 failed | 20 passed (21)

The attack is update comments set parent_id = id, depth = 1, and what makes it interesting is how completely it slipped past everything already there: both depth guards read the pre-update row where nothing points at it yet, both thread guards are inert, both CHECKs pass, and the self-referential foreign key is satisfied. src/render then files the comment under its own parent and emits no root — the comment and its entire subtree disappear from the page, silently, with the rows still in the database.

That is beyond the letter of #60. Including it is right: the migration comment now claims the edge is closed from both ends, and a comment asserting a property the code does not enforce is the specific defect CLAUDE.md warns about — it sits exactly where a reader goes to check.

The measurement had a control, which is what makes it a measurement

The cost claim is rows_read of 3 with one reply, 3 with sixty replies, and 3 with both triggers deleted. On its own that third figure is ambiguous: "the guards read nothing" and "rows_read cannot observe a trigger" produce identical numbers, and the naive reading of the first two would have been a false all-clear.

Pinning the instrument first — a no-trigger update reads 2, a trigger that reads a row reads 5 — is what turns 3-vs-3 into evidence. That is the difference between a benchmark and a number.

The departure from the issue's sketch is the right call

#60 proposed reading the replies' own thread_id. Comparing NEW.thread_id with OLD.thread_id instead avoids a guard whose verdict on a multi-row UPDATE ... WHERE thread_id = ? would depend on SQLite's row-visit order — passing in a test with three rows and failing in production with three hundred. Order-dependent security logic is worse than none, because it produces confidence.

The UPDATE OF footgun, demonstrated again

Renaming thread_id in the OF list: the migration applies clean and only the tests notice. That is the same silent-disarm property I verified empirically during #29 — SQLite creates BEFORE UPDATE OF no_such_column without error. Two issues in a row have now depended on it, which is a good argument for the comment that records it.

The constraint worth your eye

The guards make one thing impossible on purpose: no ordering of UPDATEs can move a comment and its replies to another page in a single pass. A future merge-threads feature must detach → move → re-attach, at 2N+1 statements, and there is a test holding that route open.

That is a real cost against the 50-query invocation budget for a bulk operation, and it is the one thing here you might want to overrule. The alternative reintroduces the row-order dependence above, so I would keep it — but it should be a decision rather than a discovery.

Seven "legitimate updates still work" tests passed in every kill-shot run

Including moderation on a comment that has replies. A guard that blocks real work is worse than the gap it closes, and that is asserted rather than assumed.