test(server): [YW-269] cover real createDashframeServer vault seam — anti-shadow + injection#133
Conversation
… injection
Extract buildDashframeApp from createDashframeServer so the vault-injection
seam (the anti-shadow merge and the short-circuit path) can be driven by direct
unit tests without a live socket. Three contracts exercised:
1. Anti-shadow — injected vault wins over a bogus vault key in request context;
a merge-order regression would cause the test to fail with a registry error.
2. No-injection short-circuit — buildDashframeApp({db}) returns the raw unwrapped
app when vault and onWrite are both absent.
3. Vault visible to handlers — credential-bearing addDataSource succeeds only
when the injected vault is in context.
The app.ts change is a pure refactor-for-testability: the merge logic (the
anti-shadow { ...(context ?? {}), ...staticContext } spread and the onWrite hook
wiring) is extracted verbatim into buildDashframeApp; createDashframeServer now
delegates to it. No behavior change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 53 minutes and 9 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // --------------------------------------------------------------------------- | ||
| // AC3 — runHandler threads the injected vault | ||
| // --------------------------------------------------------------------------- |
There was a problem hiding this comment.
The comment at the section boundary labels this block "AC3 — runHandler threads the injected vault", but the single test inside only exercises app.call — runHandler is never invoked. A developer who later wants to add a direct runHandler test might skip this section assuming it's already covered, or write a duplicate. The body comment explains the limitation correctly, but the banner label should reflect what's actually proven (e.g. "AC3 — vault threads into handlers via app.call") so the gap between intended and exercised coverage is unambiguous at a glance.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/app.test.ts
Line: 596-598
Comment:
**Section banner doesn't match the test it guards**
The comment at the section boundary labels this block "AC3 — runHandler threads the injected vault", but the single test inside only exercises `app.call` — `runHandler` is never invoked. A developer who later wants to add a direct `runHandler` test might skip this section assuming it's already covered, or write a duplicate. The body comment explains the limitation correctly, but the banner label should reflect what's actually proven (e.g. "AC3 — vault threads into handlers via `app.call`") so the gap between intended and exercised coverage is unambiguous at a glance.
How can I resolve this? If you propose a fix, please make it concise.
| /** | ||
| * Build the WyStack app with vault injection and onWrite hook wiring — without | ||
| * starting an HTTP server. | ||
| * | ||
| * Extracted from `createDashframeServer` so the vault-injection seam (the | ||
| * anti-shadow merge and the short-circuit path) can be driven by direct unit | ||
| * tests without a live socket. `createDashframeServer` calls this internally; | ||
| * tests import and exercise it directly. | ||
| * | ||
| * Security invariant: `vault` is injected into every handler context via a | ||
| * static spread that wins over per-call context. The merge order | ||
| * `{ ...(context ?? {}), ...staticContext }` means the vault key cannot be | ||
| * shadowed by a caller-supplied context — the vault identity is fixed for the | ||
| * lifetime of the returned app. | ||
| */ |
There was a problem hiding this comment.
onWrite–runHandler asymmetry is undocumented on the now-public API
buildDashframeApp is newly exported, so its JSDoc is now the authoritative contract. The onWrite hook fires only from the call wrapper; the runHandler wrapper applies the vault-merge but does not call onWrite (preserved from the original inlined code). A caller supplying onWrite to buildDashframeApp directly — rather than via createDashframeServer — will see silent non-firing for writes that go through runHandler. Worth a one-line note such as "onWrite fires only from call; runHandler performs context injection but does not invoke the hook".
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/app.ts
Line: 220-234
Comment:
**`onWrite`–`runHandler` asymmetry is undocumented on the now-public API**
`buildDashframeApp` is newly exported, so its JSDoc is now the authoritative contract. The `onWrite` hook fires only from the `call` wrapper; the `runHandler` wrapper applies the vault-merge but does not call `onWrite` (preserved from the original inlined code). A caller supplying `onWrite` to `buildDashframeApp` directly — rather than via `createDashframeServer` — will see silent non-firing for writes that go through `runHandler`. Worth a one-line note such as "`onWrite` fires only from `call`; `runHandler` performs context injection but does not invoke the hook".
How can I resolve this? If you propose a fix, please make it concise.1 participant
What this PR does
Adds direct unit tests for the security-critical vault-injection seam in
createDashframeServer(apps/server/src/app.ts). The prior state had no test driving the real production code —vault-control-plane.test.tshad re-implemented an equivalent wrapper inline, meaning a merge-order regression inapp.tswould go undetected.Test cases (in
apps/server/src/app.test.ts)Anti-shadow — injected vault wins over a bogus vault key in call context. Injects a real vault (with
connector-keyclass registered), then passes a bogus vault (noconnector-keybackend) in the request context. The bogus vault causes a registry throw if it wins; the test succeeds only when the injected vault is used. A reversed merge order would fail this test with a "no default backend for class connector-key" error.Anti-shadow — identity verification via
hasCallCount. Stronger form: afteraddDataSource+getDataSourcewith a bogus vault in context, assertsrealBackend.hasCallCount > 0— confirming the injected backend'shas()path was exercised on the read side.No-injection short-circuit.
buildDashframeApp({ db })with no vault or onWrite returns the raw unwrapped app. A CSV source (no credential) round-trips without needing a vault.Vault visible to handlers via
app.call. Credential-bearingaddDataSourcesucceeds only when the injected vault is in context. ThebuildDashframeAppwrapper applies the same{ ...(context ?? {}), ...staticContext }merge to bothcallandrunHandler.app.tschange — classification: pure refactor-for-testability (KEPT)The prior agent extracted the vault-injection/anti-shadow merge logic from
createDashframeServerinto a new exportedbuildDashframeAppfunction, then replaced the inline code with a call tobuildDashframeApp. The logic is byte-for-byte identical — samestaticContextconstruction, samehasStaticContextflag, same spread order{ ...(context ?? {}), ...staticContext }, sameonWritenull-check, same short-circuit whenvault == null && onWrite == null. No behavior change. This is the standard "hoist-to-export for testability" pattern (seeassertBindAuthorizedin the same file).No production behavior was changed. The tests exercise the real seam — a regression in app.ts would fail them.
Local gate
turbo typecheck(whole graph): PASS — 44 tasks, 26 cached, 0 errorslint: PASS — clean (pre-existing@dashframe/uiwarning unrelated to this diff)format: PASS — no violationsbun test(apps/server): 197/197 PASS — including all 4 new seam testscode-review --effort high(clawpatch): 0 MUSTs — 2 comment-accuracy SUGGESTs resolved inline before commitTracked internally as YW-269
🤖 Generated with Claude Code
Greptile Summary
This PR extracts the vault-injection and
onWritewiring from the inlined body ofcreateDashframeServerinto a separately-exportedbuildDashframeAppfunction, then adds four direct unit tests that drive the real seam. The production logic is byte-for-byte identical to what was removed — same merge order, same short-circuit, sameonWriteswallow-on-throw.app.ts:buildDashframeAppis exported with the same vault anti-shadow merge ({ ...(context ?? {}), ...staticContext }) and the identicalonWritenull-check;createDashframeServernow delegates to it.app.test.ts: Four new tests verify the anti-shadow invariant (two forms: success path and backend call-count identity), the no-injection short-circuit, and vault visibility viaapp.call.Confidence Score: 4/5
Safe to merge — the production refactor is a mechanical extraction with no behavior change, and the new tests tighten coverage of the security-critical vault seam.
The vault anti-shadow logic is unchanged and the tests confirm correct merge order. The only open items are documentation gaps: the AC3 section banner names
runHandlerbut the test only exercisesapp.call, and the new exportedbuildDashframeAppJSDoc doesn't note thatonWriteis silent for writes going throughrunHandler.The
buildDashframeAppJSDoc inapp.ts(lines 220–234) and the AC3 section banner inapp.test.ts(line 596–598) are the two spots worth a quick pass before merge.Important Files Changed
buildDashframeApp;createDashframeServerdelegates to it. Logic and merge-order are byte-for-byte identical. Minor doc gap:onWrite–runHandlerasymmetry not mentioned in the new JSDoc.app.call. AC3 section banner says "runHandler" but the test only exercisesapp.call; acknowledged in-body but the label is misleading.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Caller participant buildDashframeApp participant WrappedApp participant rawApp Caller->>buildDashframeApp: "buildDashframeApp({ db, vault?, onWrite? })" buildDashframeApp->>rawApp: "createWyStack({ db, functions })" rawApp-->>buildDashframeApp: rawApp alt "vault == null && onWrite == null" buildDashframeApp-->>Caller: rawApp (short-circuit) else vault or onWrite supplied buildDashframeApp-->>Caller: "WrappedApp { ...rawApp, call, runHandler }" end Caller->>WrappedApp: call(path, args, context?) WrappedApp->>WrappedApp: "merged = { ...(context ?? {}), ...staticContext }" Note over WrappedApp: staticContext (vault) wins — cannot be shadowed WrappedApp->>rawApp: call(path, args, merged) rawApp-->>WrappedApp: result opt "onWrite != null && tablesWritten.size > 0" WrappedApp->>WrappedApp: onWrite() [errors swallowed] end WrappedApp-->>Caller: result Caller->>WrappedApp: runHandler(path, args, tracked, context?) WrappedApp->>WrappedApp: "merged = { ...(context ?? {}), ...staticContext }" WrappedApp->>rawApp: runHandler(path, args, tracked, merged) rawApp-->>WrappedApp: result Note over WrappedApp: onWrite NOT fired from runHandler path WrappedApp-->>Caller: result%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Caller participant buildDashframeApp participant WrappedApp participant rawApp Caller->>buildDashframeApp: "buildDashframeApp({ db, vault?, onWrite? })" buildDashframeApp->>rawApp: "createWyStack({ db, functions })" rawApp-->>buildDashframeApp: rawApp alt "vault == null && onWrite == null" buildDashframeApp-->>Caller: rawApp (short-circuit) else vault or onWrite supplied buildDashframeApp-->>Caller: "WrappedApp { ...rawApp, call, runHandler }" end Caller->>WrappedApp: call(path, args, context?) WrappedApp->>WrappedApp: "merged = { ...(context ?? {}), ...staticContext }" Note over WrappedApp: staticContext (vault) wins — cannot be shadowed WrappedApp->>rawApp: call(path, args, merged) rawApp-->>WrappedApp: result opt "onWrite != null && tablesWritten.size > 0" WrappedApp->>WrappedApp: onWrite() [errors swallowed] end WrappedApp-->>Caller: result Caller->>WrappedApp: runHandler(path, args, tracked, context?) WrappedApp->>WrappedApp: "merged = { ...(context ?? {}), ...staticContext }" WrappedApp->>rawApp: runHandler(path, args, tracked, merged) rawApp-->>WrappedApp: result Note over WrappedApp: onWrite NOT fired from runHandler path WrappedApp-->>Caller: resultPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "test(server): cover real buildDashframeA..." | Re-trigger Greptile