fix(mcp): scope Lunora by-id writes to their table - #331
Conversation
Every MCP write whose plan contained a patch or delete failed with `-32603`; insert-only writes succeeded. Reads were unaffected. `ctx.db.asId(table, id)` is a compile-time parse boundary — it returns a phantom-branded `Id<T>` whose table name erases before the call reaches the store, and the typed by-id `patch`/`delete` have no `expectedTable` parameter to carry it. The id then resolved via `UNION ALL` across all five shard tables, tripping Workerd SQLite's compound-SELECT limit. The prior comment named that constraint correctly and then concluded `asId` compensated for it, which it does not. Route the three by-id writes through the per-table accessor instead (`ctx.db.nodes` / `ctx.db.dailyIndex`), which `bindTableFacade` binds with its table name forwarded as `expectedTable` — scoped by construction, so it cannot drift back. Inserts already named their table and were never affected. The existing unit coverage passed throughout because its fake store does not model shard id resolution, so add a regression test that models the by-id path as the throw it is. Fixes #330 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMCP outline mutations now use runtime table-scoped node accessors, and daily-note mapping updates use the scoped daily-index accessor. Tests verify routing, operation order, nullable patches, and both mapping paths. Workspace guidance and a patch changeset document the correction. ChangesMCP shard-scoped mutations
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
![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 `@worker/lunora-mcp-bridge.test.ts`:
- Around line 61-63: Update the patch mock in the test to accept and record its
fields payload alongside id, operation, and table. Extend the assertions around
the `"n1"` patch call to verify the recorded fields equal `{ parentId: null }`,
ensuring the payload is propagated rather than only routing being tested.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f254f66-87ab-4253-8bb0-bb576d391f0b
📒 Files selected for processing (4)
.changeset/scoped-mcp-shard-writes.mdAGENTS.mdlunora/mcp.tsworker/lunora-mcp-bridge.test.ts
The recording fake dropped `patch`'s second argument, so the null-field test proved routing only — a cast that silently discarded `parentId: null` would still have passed the very case it exists to cover. Record the fields and assert them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Reviewed changes
lunora/mcp.ts— three by-id writes (commitPlandeletes,commitPlanpatches,claimDailyMapping) moved to the per-table accessor;commitPlanexported as a test seam.worker/lunora-mcp-bridge.test.ts— newrecordingCtx()fake plus two regression tests.AGENTS.md— the learned-fact bullet rewritten to name both correct spellings and state thatasIdscopes nothing..changeset/scoped-mcp-shard-writes.md— patch bump.
I verified the diagnosis rather than taking it on faith: bindTableFacade really does forward its bound name as expectedTable (node_modules/@lunora/server/dist/packem_shared/bindOrm-Bp9hsM2q.mjs), and the nominal DatabaseWriter really does declare delete: <T>(id: Id<T>) => Promise<void> with no expectedTable parameter while the runtime writer has one — so the asymmetry the fix is built on is real, and asId is compile-time only. I also checked for residual unscoped writes: lunora/mcp.ts and lunora/mutators.ts are the only files with by-id patch/delete/get calls, and every mutators call site already passes the explicit table. bun test worker/lunora-mcp-bridge.test.ts → 4 pass.
ℹ️ Two copies of commitPlan, and nothing keeps them in lockstep
lunora/mutators.ts:122-142 is a near-identical twin of the commitPlan this PR fixes — same deletes → patches → inserts order, same nodeToInsertFields, same OutlinePlan. It was always correct, because MutatorCtx is loose enough to accept the third expectedTable argument, so it uses ctx.db.delete(id, "nodes") while the MCP copy needed the facade. That divergence in spelling is exactly why the MCP copy could drift and ship broken while the browser path stayed healthy, and the fix doesn't change that: the two are still independent bodies that must be edited together.
Worth a human call on scope. Extracting a shared planner-applier is awkward given the two ctx types, but a comment on each pointing at the other, or a note in AGENTS.md that outline-plan application exists twice, would at least make the coupling discoverable. Out of scope for a fix PR if you'd rather not widen it.
ℹ️ First runtime use of the table facade in this repo — the live-shard check is the only thing that can confirm it
Before this PR nothing in the repo called ctx.db.<table>.*. I chased the static evidence and it holds up: the facade is attached in lunora/_generated/shard.ts:887-893 inside the single buildCtx, which every dispatch path goes through — including the internal-mutation system RPC that worker/lunora-mcp-store.ts uses — and the instrumentation wrapper is a Proxy with only a get trap, so the property assignment lands and reads pass through. So ctx.db.nodes should exist at runtime.
But the unit test can't corroborate any of that: its fake supplies the accessor by construction, so an absent one would still pass. Your own test-plan box "not verified against a live shard" is still unchecked, and it's the only check that closes this. One update_node and one delete_node over MCP on a Lunora account after deploy would do it.
ℹ️ Nitpicks
See the two inline comments on worker/lunora-mcp-bridge.test.ts.
Claude Opus ｜ 𝕏
|
Run failed. View the logs →
|
Two review nitpicks from pullfrog on #331. The fake threw for ANY by-id patch/delete, including the correct 3-arg `ctx.db.patch(id, fields, "nodes")` form that `lunora/mutators.ts` uses and that this branch's own AGENTS.md bullet blesses. That asserted a spelling, not the invariant — a refactor to the explicit-arg form would have failed a test while being correct. It now throws only when the table argument is absent, which is the actual constraint, and records the table for either correct form. `claimDailyMapping` had no test at all, so the dailyIndex half of the fix could regress silently. Two more cover it (patch on an existing mapping, insert on a missing one), plus one guarding the guard. Cross-reference comments on both `commitPlan` twins: same plan, same bucket order, two bodies, and nothing keeps them in lockstep — which is how the MCP copy shipped unscoped while the browser path stayed healthy. Reverting `commitPlan` + `claimDailyMapping` to the unscoped by-id calls fails 3 of 7; restored, 7 pass. Full suite 1018 pass, all gates green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Run failed. View the logs →
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lunora/mutators.ts (1)
122-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider collapsing the twin
commitPlanimplementations.The comment correctly documents that this file's
commitPlanandlunora/mcp.ts'scommitPlanare hand-synced twins, and that their divergence is exactly what shipped the#330bug. Documentation reduces but doesn't eliminate the recurrence risk — a future edit to one body without the other reintroduces the same class of bug. If feasible, consider factoring the shared bucket logic (deletes → patches → inserts) into one function parameterized by a write-adapter (delete/patch/insert primitives), soMutatorCtxandMutationCtxeach supply their own adapter but the ordering/logic lives in one place.🤖 Prompt for 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. In `@lunora/mutators.ts` around lines 122 - 150, Consolidate the duplicate commitPlan implementations in lunora/mutators.ts and lunora/mcp.ts by extracting one shared helper for the deletes → patches → inserts bucket logic, parameterized by delete, patch, and insert write operations. Update each commitPlan to provide its context-specific adapter while preserving the existing node/table handling and operation order.
🤖 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 `@worker/lunora-mcp-bridge.test.ts`:
- Around line 144-155: Await the rejected-promise assertion in the test “an
unscoped by-id write is what the fake rejects — the bug, not a spelling” by
adding await before expect(byId.patch("n1", {})).rejects.toThrow(...). Keep the
subsequent awaited byId.patch call and call-record assertion unchanged.
---
Nitpick comments:
In `@lunora/mutators.ts`:
- Around line 122-150: Consolidate the duplicate commitPlan implementations in
lunora/mutators.ts and lunora/mcp.ts by extracting one shared helper for the
deletes → patches → inserts bucket logic, parameterized by delete, patch, and
insert write operations. Update each commitPlan to provide its context-specific
adapter while preserving the existing node/table handling and operation order.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 42ef3d35-e7e5-4d09-8cd6-86997f6be4f3
📒 Files selected for processing (3)
lunora/mcp.tslunora/mutators.tsworker/lunora-mcp-bridge.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lunora/mcp.ts
CodeRabbit's nit on #331. Every other `.rejects` in the repo is awaited (9 call sites); this one wasn't. Its stated failure mode does NOT reproduce here: bun 1.3.14 surfaces the unawaited rejection either way — checked by flipping the expected message to one that never appears, with and without the await, and with a minimal repro that has nothing after it to yield. Both fail. So this is consistency and cross-runner portability, not a vacuous test. Full suite 1018 pass, all gates green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Three commits since the prior pullfrog review (3cdceaa), all responding to that review's feedback. The production delta is comment-only; the substance is the test file.
- The fake now models the invariant, not a spelling —
record()throws only when the table is absent, andwriter(bound?)accepts either correct form: the bound accessor or the explicit third argumentlunora/mutators.tsuses. A future refactor to the explicit-arg form no longer fails a test for being correct. claimDailyMappingis covered — two tests through.handler: an existing mapping asserts apatchondailyIndexplus its payload, a missing one asserts the insert path carries no by-id write to scope.- Payload assertions added —
patchedrecords the fields alongside the route, so a cast that quietly droppedparentId: nullwould now fail instead of routing correctly. - A guard-the-guard test — asserts the fake still rejects an unscoped by-id write, so
record()can't go inert and silently green the two tests that depend on it.39dd12fawaits that.rejectsassertion. - Cross-reference comments on the two
commitPlantwins —lunora/mcp.ts:66-67andlunora/mutators.ts:122-129each point at the other and say why the two bodies differ.
I checked the regression coverage empirically rather than taking the test plan on faith: reverting all three lunora/mcp.ts call sites to the unscoped by-id form fails exactly 3 of 7 (commitPlan ×2, claimDailyMapping ×1), and restoring them returns 7 pass — so these tests bite on the bug they exist to catch. I also confirmed .handler is the real registered handler rather than a shim (RegisteredFunction.handler: (context, args) => …, @lunora/server/dist/types.d.ts:417; the revert's stack trace runs through Lunora's own initLunora wrapper), and that lunora/mutators.ts genuinely uses the explicit third-arg form the new comment blesses (ctx.db.delete(id, "nodes") at :134, mctx.db.patch(id, fields, "savedQueries") at :695) — so the "two correct spellings" framing is accurate, not aspirational. typecheck:test and lint are green.
The one item still open is unchanged and not something these commits could close: the live-shard test-plan box. The fake supplies ctx.db.nodes/ctx.db.dailyIndex by construction, so no unit test here can confirm the accessor exists at runtime — one update_node and one delete_node over MCP on a Lunora account after deploy stays the only check that does.
Claude Opus ｜ 𝕏
1 participant
Summary
Every MCP write whose plan contained a patch or delete failed with
-32603; insert-only writes succeeded and reads were fine. An agent could add bullets but could never check one off, edit it, move it, or delete it. Lunora sync only.Root cause:
ctx.db.asId(table, id)is a compile-time parse boundary. It returns a phantom-brandedId<T>whose table name erases before the call reaches the store, and the typed by-idpatch/deletehave noexpectedTableparameter to carry it — so the id resolved viaUNION ALLacross all five shard tables and tripped Workerd SQLite's compound-SELECT limit.The old comment named that exact constraint and then concluded
asIdcompensated for it. It doesn't.Changes
lunora/mcp.ts— route the three by-id writes through the per-table accessor (ctx.db.nodes,ctx.db.dailyIndex).bindTableFacadebinds each accessor with its table name forwarded asexpectedTable, so they're scoped by construction rather than by remembering an argument. Comment rewritten to say whatasIdactually does.worker/lunora-mcp-bridge.test.ts— regression tests that model the unscoped by-id path as the throw it is. The fake asserts the invariant ("an id-addressed write names its table at runtime"), not one spelling of it: it accepts both the bound accessor and the explicit third argumentlunora/mutators.tsuses, and throws only when neither carries a table. CoverscommitPlan(nodes) andclaimDailyMapping(dailyIndex).lunora/mutators.ts+lunora/mcp.ts— cross-reference comments on the twocommitPlantwins. Same plan, same bucket order, two bodies, nothing keeping them in lockstep — which is how the MCP copy shipped unscoped while the browser path stayed healthy. Comment only; no shared-planner extraction (the two ctx types make it awkward, and that's a wider change than a fix PR).AGENTS.md— the learned fact said "mutators must passexpectedTable", which read as mutator-specific and leftasIdlooking sufficient. Now covers both correct spellings and states plainly thatasIdscopes nothing.Inserts already named their table (
ctx.db.insert("nodes", …)) and were never affected.Flow
Breaking
None. Restores intended behavior.
Test plan
commitPlanANDclaimDailyMappingto the unscoped by-id calls and re-running: 3 of 7 fail. With the fix: 7 pass.bun run test— 1018 pass, 0 failtypecheck,typecheck:worker,typecheck:test,lint,fmt:check— all greenUNION ALLfailure can only be exercised on a Lunora account. Worth oneupdate_node+ onedelete_nodethrough MCP after deploy.Notes for review
Two judgment calls worth a look:
1.
commitPlanis now exported. The file header says "full shard RPC stays integration-only," and I kept that — the fake is a call recorder, not a store, and the test asserts routing, which is where the bug lived. But it does widen the module surface for a test seam. Say the word if you'd rather it stayed private.2. A cast on the patch payload. The generated
Insert_nodesdrops.nullable()— the schema declaresparentId: v.string().nullable()but codegen emitsparentId: string, so a legitimate null patch (moving a node to the top level) doesn't satisfyPartial<Insert_nodes>. The write is fine; the generated type is too narrow. There's a test covering the null case so the cast can't quietly hide a real break. This looks like a lunorash codegen defect and may be worth reporting upstream alongside theasIdergonomics.3. Upstream ergonomics.
bindTableFacade's own doc says the forwarded scope also prevents an IDOR — a branded id from another table resolving cross-table. Here the blast radius is bounded (rows are shard-scoped per user andassertOwnergates the call), so this was a crash, not a data-leak. But the general shape is worth raising: the typed by-id API silently drops information the looser one carries, so the disciplined-looking call is the broken one.Fixes #330
🤖 Generated with Claude Code
Summary by CodeRabbit