fix(db): validate explicit insert nodeType against anchor's tier (#383) - #656
Conversation
insertParagraphAfter validated an explicit nodeType against the global insertable set, but not against the anchor it would become a sibling of. A non-part anchor with a structurally incompatible explicit nodeType (e.g. pr1 requested after an article anchor, or article after a pr1 anchor) slipped through and landed a malformed row one CSI tier away from where it belongs. resolveInsertableNodeType now also requires the explicit nodeType to be a legal sibling of the anchor: the anchor's own type, continuation (legal at any tier), or any insertable type when the anchor is a note (KNOWN AMBIGUITY — a note carries no tier of its own, documented at the check site and pinned by a regression test). Anything else reuses the existing invalid-type status carrying the rejected type, so the REST/MCP response shape and status set are unchanged. Regression tests added at the DB, REST, and MCP boundaries. The existing "honors an explicit nodeType" DB test asserted pr2-after-pr1 (the same cross-tier bug with a different pair) succeeding; it now asserts an explicit continuation instead, which the rule permits at any tier. Design decisions: - The sibling-compatibility rule was settled by the repository owner in the issue body; no ADR needed per this sprint's override — the rationale lives in isSiblingCompatible's doc comment and here. - openapi.yaml and the insert_paragraph MCP tool description are updated in the same commit to describe the sibling-tier requirement in prose; neither the reachable status set nor any response shape changed, so no contract-shape edits were needed. - Mutation-verified: removing the isSiblingCompatible check from resolveInsertableNodeType fails exactly the 6 new regression tests (54 pass, 6 fail) across the DB/REST/MCP integration suites; with the check restored all 60 pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughParagraph insertion now validates explicit ChangesParagraph sibling compatibility
Sequence Diagram(s)sequenceDiagram
participant Caller
participant REST_or_MCP
participant insertParagraphAfter
participant Database
Caller->>REST_or_MCP: Request insertion with nodeType
REST_or_MCP->>insertParagraphAfter: Insert paragraph after anchor
insertParagraphAfter->>Database: Validate sibling compatibility
Database-->>insertParagraphAfter: Insert sibling or return invalid-type
insertParagraphAfter-->>REST_or_MCP: Return result
REST_or_MCP-->>Caller: Return created paragraph or error
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Comment |
The sibling-compatibility check made `invalid-type` newly reachable for a type that IS on the insertable list but is wrong for the anchor's tier. The existing message ended "pass nodeType (article, pr1–pr7, or continuation)", so a rejected `pr1` was answered by suggesting `pr1` — the caller was told to resend exactly what had just been refused, with no way to correct it. Reword both surfaces in lockstep to state the actual rule (match the anchor's own type, or continuation; parts and notes are never insertable). Message-only: no new status value, no response-shape change, so the REST switch and the MCP handler stay exhaustive and openapi.yaml's schema is untouched. Found by the end-of-draft Codex (gpt-5.6-sol, xhigh) adversarial review. Pinned by an assertion that the 422 no longer offers the rejected type. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
This was written agentically; verify its assertions and edit accordingly: Adversarial cross-review — Codex
|
#383) Owner decision extending the sibling-compatibility rule. Condition 3 now reads "the anchor's node_type is note OR continuation" — the other two conditions are unchanged. The rule's general case works because the anchor DEMONSTRATES a tier that is legal at its parent. A continuation demonstrates nothing: it continues the preceding node's text and inherits that node's tier rather than stating one of its own, so — exactly like a note — it cannot constrain the tier of what follows it. A pr1 after a continuation that itself follows a pr1 is ordinary, well-formed content, and the previous note-only rule refused it. The behaviour was fail-closed, so no row was ever mis-tiered by it. Names the concept as TIERLESS_ANCHOR_NODE_TYPES rather than a second literal, and extends the KNOWN AMBIGUITY comment to say WHY both types qualify. Keeps rejecting the parent-derived alternative: that is the parent->child table this rule avoids on purpose, which mis-rejects ilvl-gapped legacy data (the CPI offset case, where a pr3 legitimately sits under an article). Regression tests added at all three surfaces (DB, REST, MCP): continuation anchor + explicit pr1 now succeeds. Every existing pin re-run and intact — article-after-article accepted, part anchor rejected, pr1-after-article and article-after-pr1 still rejected. Surfaced by the Codex adversarial review as F2(b), escalated, and approved by the repository owner. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Action performedFull review 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 `@src/api/paragraphs.ts`:
- Line 56: Update the insertion error messages in src/api/paragraphs.ts at lines
56-56 and src/mcp/paragraph-handlers.ts at lines 106-106 to state that any
insertable node type is valid after a note or continuation anchor, while
preserving the existing matching-type rule for other anchors and excluding parts
and notes as insertable types.
🪄 Autofix
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 Plus
Run ID: cb23b7e8-11c8-4b8e-ba28-c36805911395
📒 Files selected for processing (9)
openapi.yamlsrc/api/paragraph-insert.integration.test.tssrc/api/paragraphs.tssrc/db/queries/paragraph-insert.integration.test.tssrc/db/queries/paragraph-insert.tssrc/mcp/paragraph-handlers.test.tssrc/mcp/paragraph-handlers.tssrc/mcp/paragraph-tools.integration.test.tssrc/mcp/paragraph-tools.ts
…jection Both the REST route and the MCP handler hand-copied an 'invalid-type' message naming only two of the three ways to pass isSiblingCompatible: "nodeType must match the anchor's own type or be continuation". It omitted the tierless-anchor exception, so an editor inserting a legal pr1 after a `note` anchor read that their request broke a rule it does not break. openapi.yaml and the insert_paragraph tool description already stated the complete rule — only the message a caller actually reads was wrong. Rather than correct two copies and leave the drift class open, the text now lives beside the rule it describes (invalidInsertTypeMessage, next to isSiblingCompatible in paragraph-insert.ts) and is re-exported through db/index.ts. api/ and mcp/ cannot import each other (module-boundaries.md), so a shared owner in db/ is the only single definition available — the same pattern lockedObjectMessage already uses at these exact call sites. Pins the message states the complete rule, matching on the concept (/tierless/i) not the prose. Behaviour after a tierless anchor was already covered at all three layers; the message half was not, which is how it drifted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The db/index.js mock stubbed lockedObjectMessage by hand, so adding invalidInsertTypeMessage to the real barrel left the mock without it — the handler threw a TypeError that its own catch flattened into "Internal error", which is how the suite caught this. Fixed by importing the REAL builder into the mock factory (the pattern the error classes at the top of the same factory already use) rather than writing a second stub. A stub would let the two wordings drift and leave this test asserting against fiction. Verified the dynamic import survives paragraph-insert.ts's own '../index.js' import without recursing. That in turn makes the MCP surface pinnable, so it now asserts the complete rule the same way the REST integration test does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Why
insertParagraphAfter(src/db/queries/paragraph-insert.ts) creates a newnode as a SIBLING of the anchor — same
parent_id. It already rejects a partanchor and rejects a
nodeTypeoutside the globally insertable set, but for anon-part anchor it accepted any explicit
nodeTypethat was insertablesomewhere, without checking it was insertable at this anchor's tier. A
caller could request
pr1after anarticleanchor, orarticleafter apr1anchor, and the row would land one CSI tier away from where it belongs.The default path (no explicit
nodeType) was never affected, because thedefaulted type always equals the anchor's own type — a trivially valid
sibling. The WYSIWYG Enter gesture uses the default, so no current client hit
this; it required an explicit, mismatched
nodeType.What
resolveInsertableNodeTypenow also requires the explicitnodeTypeto be alegal sibling of the anchor via a new pure helper,
isSiblingCompatible:nodeType === anchor.node_type— the general rule: a sibling's onlyproven-legal type is the type the anchor itself already demonstrates as
legal at that
parent_id.nodeType === 'continuation'— a continuation carries no CSI tier of itsown and is legal at any tier.
anchor.node_typeis tierless — anoteor acontinuation—KNOWN AMBIGUITY: rule 1 only constrains anything because the anchor
demonstrates a tier that is legal at its parent. A tierless anchor
demonstrates nothing: a note is an editorial aside, and a continuation
continues the preceding node's text and inherits that node's tier rather
than stating one. Neither can constrain the tier of what follows it, and
both legitimately interleave among body paragraphs of any tier. Deliberately
permissive, documented at the check site and pinned by regression tests at
all three surfaces.
The
continuationhalf of this condition was added by owner decisionafter the Codex adversarial review flagged that the original
note-onlyrule refused a legitimate insert (a
pr1after acontinuationthat itselffollows a
pr1is ordinary content). The original behaviour was fail-closed,so it never mis-tiered a row.
Anything else reuses the existing
invalid-typestatus, carrying therejected
nodeType— no new status value, no response-shape change. Thepre-existing part-anchor guard (
anchor.node_type !== 'part') is kept asdefense-in-depth rather than removed as "logically subsumed," since the issue
explicitly calls out not regressing that case.
The merge added-op apply path (
input.explicitIdset) is unaffected: itsstructural-anchorguard and KNOWN AMBIGUITY (#374) still run first and areuntouched by this change.
Design decisions
owner in the issue body — no ADR, per this sprint's explicit override. The
rationale lives in
isSiblingCompatible's doc comment and here.not an oversight — pinned with a
KNOWN AMBIGUITYcomment in the source andin the DB/REST/MCP regression tests, per repo convention. The two types are
named together as
TIERLESS_ANCHOR_NODE_TYPESso the shared reason (neithercarries a CSI tier of its own) is stated once rather than duplicated as two
string literals.
proposed by the adversarial review and rejected: that is the parent→child
type table this rule avoids on purpose, and it mis-rejects ilvl-gapped legacy
data — the CPI offset case, where a
pr3legitimately sits under anarticle.sibling-compatibility for explicit-type inputs (a part anchor can never
equal an insertable
nodeType, so it always fails the schema check too) —touching a guard the issue explicitly protects was unnecessary risk for no
benefit.
pr2-after-pr1-anchor succeeding — the same cross-tier bug class named inthe issue (
pr2nests aspr1's CHILD, never its sibling). Corrected it toassert an explicit
continuationinstead, which the rule actually permitsat any tier, rather than leaving a test that encoded the bug.
OpenAPI / MCP lockstep
No response shape or reachable-status-set change —
invalid-typeis reusedverbatim, so the REST 422 shape and the MCP tool-error shape are both
unchanged.
openapi.yaml'sPOST /specs/{id}/paragraphsdescription and theinsert_paragraphMCP tool description were both updated in this commit,prose-only, to document the new sibling-tier requirement so the two surfaces
stay in lockstep with the code and with each other.
Testing
pnpm test: 255 files / 3630 tests passed.pnpm test:integration(DATABASE_URL exportedalone, NODE_ENV=test, never sourcing
.env): 158 files passed / 12skipped (170), 1819 tests passed / 141 skipped (1960).
pnpm lintgreen (eslint incl.complexity/cognitive-complexity≤10,tsc --noEmit,prettier --check).malformed examples (
pr1-after-article,article-after-pr1) andconfirmed both are now rejected with
invalid-type, and thatarticle-after-articleand apartanchor's rejection are bothunchanged.
isSiblingCompatible(forcedtrue),reran the DB/REST/MCP integration suites — exactly the 6 new regression
tests failed (54 pass / 6 fail), no other test affected. Reverted the
mutation — all 6 pass again alongside the rest of the suite (confirmed
via the full
pnpm test:integrationrun above).Mutation transcript (independently reproduced during the review loop)
Two mutations, because the rule has two independently load-bearing halves —
the rejection it adds and the tierless exception it carves out. A single
"force
true" run only proves the first.Mutation A — gate fully neutralized (
isSiblingCompatible→return true):Every failure is a cross-tier rejection assertion, one per surface pair.
Mutation B — condition 3 reverted to the pre-owner-decision
note-only rule(
TIERLESS_ANCHOR_NODE_TYPES.has(anchorNodeType)→anchorNodeType === 'note'):Exactly the three newly-permitted cases, so the
continuationhalf of thetierless exception is load-bearing and not decorative.
Restoring the check in both cases:
No unrelated test moved in either run. The gate fails when the fix is removed
and when the exception is narrowed — it is not vacuous.
Adversarial review
Codex
gpt-5.6-sol@xhigh, run once as the final draft gate: 2[P2]findings, both now resolved.
0beeb590) — theinvalid-typemessage suggested the verytype it had just rejected, leaving the caller no way to correct the request.
59921e21) — thecontinuationanchor case, escalated andapproved by the repository owner; see above.
pre-existing behaviour, and the proposed parent-derivation replacement is
rejected on the CPI ilvl-gap grounds above.
Full assessment in a PR comment.
Closes #383.
🤖 Co-authored by Claude Sonnet 5.
Summary by CodeRabbit
Bug Fixes
Documentation