Repository navigation
Phase 3: vault MCP server with execution-time evidence revalidation - #4
Merged
Merged
Conversation
Streamable HTTP transport in stateless mode over node:http — all durable state will live in the vault store, so there is no MCP session to manage. The public surface is exactly five tools (no raw-row query tool exists), release_result alone is marked destructive, and unimplemented handlers fail closed. Verified end-to-end with the SDK client over a real socket. skipLibCheck added because the SDK's own declaration files do not compile under exactOptionalPropertyTypes; our sources stay fully checked. AI-assisted (Claude Code); reviewed and understood by the author.
Returns column names with sensitivity labels, the fixed mission constants, safe dimensions, allowed metrics, and one aggregate row count — no code path touches a row value, and a test scans the full payload for canary and fixture values to prove it. AI-assisted (Claude Code); reviewed and understood by the author.
Authorization strictly precedes data access: a denied mission or a bad plan leaves with zero SQLite queries executed, proven by an instrumented database in tests. The aggregate query accepts only allowlisted identifiers (never caller values), small cells are suppressed inside the vault before any candidate exists, and the prepared candidate is stored keyed by a vault-issued queryId — the evidence release-time checks will load instead of trusting a caller body. The vault surface-lock test grows by exactly the aggregate accessor. AI-assisted (Claude Code); reviewed and understood by the author.
The verdict, findings, and hashes always come from what prepare_analysis stored under the queryId — there is no path that validates a caller-supplied body. A test smuggles a contract-violating candidate straight into the store to prove storage is not trust: the contract still denies it. AI-assisted (Claude Code); reviewed and understood by the author.
Release loads the vault-stored candidate by queryId — the caller supplies only the id and the two approved hashes. Before any release write, the full contract re-verifies and the vault recomputes the aggregation from its own data: a candidate whose rows differ from what the vault can reproduce right now (regrouped cells, fabricated group sizes, smuggled rows) is denied as evidence_mismatch. Every call leaves an audit record; only success creates a receipt, at most one per queryId, enforced again at the write. The flagship test smuggles the real size-2 cell claimed as group_size 3 — it passes every contract rule and is caught only by the evidence recomputation. AI-assisted (Claude Code); reviewed and understood by the author.
Charts render released data only: without a receipt the aggregate is still a candidate and the tool refuses. The end-to-end test drives the real handlers through the MCP client over a live socket — denied mission, prepare, validate, release, denied replay, chart — and scans every payload that crossed the wire for raw values. AI-assisted (Claude Code); reviewed and understood by the author.
Six findings from the internal quality gate, all fixed: request bodies are capped at 1 MB and chunked bodies refused (SDK 1.30 dropped its own size guard); audited query ids are clipped at the write so a hostile id cannot grow the enforcement record; a thrown release error now audits as denied and returns a generic internal_error instead of echoing the exception; the server binds to loopback only; stored candidates are deep-frozen copies rather than aliases of caller arrays; and the duplicate-receipt guard plus the needs_review audit branch gained the tests they were missing. AI-assisted (Claude Code); reviewed and understood by the author.
PR Summary by QodoAdd vault MCP server with execution-time evidence revalidation
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a security-critical trust boundary with dynamically constructed SQL and a new network-facing surface handling sensitive data, which warrants final human review despite the thorough tests.
Review details
- Files reviewed: 9/11 changed files
- Comments generated: 1
- Review effort level: Balanced
…ction DNS rebinding reaches a loopback-bound server with same-origin requests, so the endpoint now requires a loopback Host and, when an Origin header is present, a loopback Origin (per the MCP Streamable HTTP spec). Audit records keep frozen copies of each finding rather than references the producer can mutate afterwards. The released-row projection reached its third use and is now one shared helper for release and chart; the contract library's copy stays independent on purpose — the hash definition must not depend on server code. AI-assisted (Claude Code); reviewed and understood by the author.
prepare_analysis persists a candidate under a new queryId, so it is not read-only; the annotation test now pins the full hint matrix for all five tools. Receipt hashes carry the branded Sha256Hex type end to end, so only values that came out of the contract's hasher can be stored. AI-assisted (Claude Code); reviewed and understood by the author.
Suppressing per request at the requested granularity was reversible: the vault published the same totals at several granularities, and a coarse total still contained the rows a finer query hid, so subtracting one from the other reconstructed the suppressed cell. A sweep over parent/child dimension pairs recovered 11 suppressed cells, four of them a single person, including individual resolution times. Suppression now runs once at the finest granularity and every coarser aggregate is rolled up from the survivors, so all granularities stay mutually consistent; the same sweep now yields zero residuals. Coarse totals are correspondingly lower because they no longer include withheld rows. Also from the same review: the prepared-candidate cache is bounded at 500 with FIFO eviction (the audit log stays uncapped on purpose — dropping enforcement records to save memory would be fail-open); the sensitive-dimension and pre-release-chart denials now write audit records; every tool call is wrapped so a thrown error becomes a generic internal_error instead of the SDK echoing the exception message; and the database header comment no longer claims something groupSize contradicts. AI-assisted (Claude Code); reviewed and understood by the author.
The grouping-key separator was a literal NUL byte, which made handlers.ts a binary blob to git: the whole release and aggregation path showed as "Binary files differ" and could not be diff-reviewed, and the rendered source looked like an ordinary space so the control character was invisible to a reader. A pipe cannot occur in the closed week/region/category value domains either, so grouping is unchanged; the key is internal and never hashed or released. AI-assisted (Claude Code); reviewed and understood by the author.
GET /mcp reached the SDK's standalone SSE branch, and because a fresh transport is built per request the SDK's one-stream-per-session guard never fired: 400 GETs held 400 open streams and 65 MB, and a single one kept close() pending forever. The vault never pushes server-initiated messages, so non-POST is now refused with 405 (the SDK client treats that as "no SSE offered" and continues). Probed after the fix: 0 of 200 GETs held a stream, close() returns in 0 ms. An unauthorized mission left no audit record while a mistyped dimension did — the more significant enforcement event was the invisible one. Both now audit. db.aggregate no longer takes a caller-chosen granularity: it always groups by SAFE_DIMENSIONS, with both statements prepared once at open. Suppression irreversibility rested on the single caller happening to pass the finest granularity, and the layer holding the raw rows now enforces it structurally instead. Deletes the per-shape statement cache and its allowlist loop. Tests: the any-granularity claim now iterates all eight dimension subsets, the differencing check covers the average metric as well as counts, and the same-origin assertion pins the response body rather than a status code that a 500 would also satisfy. AI-assisted (Claude Code); reviewed and understood by the author.
Publishing the raw total alongside suppressedCells lets a caller solve how many withheld cells are size 1 and how many size 2. It does not say which cells, so no individual is isolated, and that a missing cell is under k is inherent to any suppression scheme. Documented rather than removed: the approver needs the real dataset size. AI-assisted (Claude Code); reviewed and understood by the author.
close() waits for every open connection, so a socket that connected and never completed a request kept the vault running indefinitely — probed with both a silent socket and a slowloris partial-header write, each left close() pending past 8 seconds. closeAllConnections() brings both to 1ms and 0ms. The next phase needs a deterministic shutdown to wire TrueForge, and a caller must not be able to deny it with one idle TCP connection. AI-assisted (Claude Code); reviewed and understood by the author.
Grouping by a sensitive column audited; naming the same column as the metric did not, so a caller could enumerate email, phone, customer_id and free_text through the adjacent field and leave the enforcement record untouched. Both reaches now audit. The malformed-plan denials next to it — too many dimensions, duplicate dimension — stay unaudited deliberately: they are client bugs rather than reaches for a sensitive column, and auditing them would grow the deliberately uncapped log for nothing. AI-assisted (Claude Code); reviewed and understood by the author.
The sensitive-column audits ran after the length and duplicate checks,
so one padding element swallowed them: {dimensions:["week","week"],
metric:"email"} denied as duplicate_dimension and wrote no record, and
twelve probes naming every sensitive column in either slot left the
audit log empty. Allowlists now run first, so a malformed-plan denial
can only be reached once every named column is allowlisted and cannot
carry a sensitive column name — which is what the comment beside it
already claimed.
The shutdown test was vacuous: it closed in the same tick as the socket
write, before the server tracked the connection, so it passed with
closeAllConnections removed. A 10ms settle makes it fail without the
guard, verified by counterfactual.
Also corrects the prepare_analysis annotation comment, which said the
tool never transitions state while savePrepared evicts the oldest
candidate past the store cap.
AI-assisted (Claude Code); reviewed and understood by the author.
The VaultDatabase comment said groupSize() "returns a count the aggregate surface already exposes." It does not: it is a raw COUNT(*) with no suppression filter, while the public surface only ever publishes rollups of survivors at k>=3. Where every category under a (week, region) pair is suppressed, groupSize returns that withheld cell's exact size — vault.test.ts already asserts precisely that. Nothing calls it from a tool today, so this is not a live disclosure, but that sentence was the written justification for keeping the method on the exported type and pointed the next author at the wrong conclusion. Scopes the prepare_analysis audit claim to what the handler can see. A request the tool schema rejects (metric: 7 after a sensitive dimensions array) never reaches the handler and leaves no audit record. Not a disclosure — no data is touched and the schema error is identical whichever column was named, so there is no oracle — but the comment read as an absolute guarantee. Widening the schema so the handler could audit these was considered and declined: dimensions and metric would become unknown across the handler boundary and every downstream string assumption would need guarding, for an event that carries no signal. Pins the third of the three deep-frozen copies. The columns freeze was the one claim in that comment no test covered; verified by counterfactual that the new assertion fails without it. AI-assisted (Claude Code); reviewed and understood by the author.
The previous commit corrected this comment but named the wrong condition, and got it wrong in both directions. It said groupSize() returns a withheld cell's precise size where every category under a (week, region) pair is suppressed. Measured against the seed: pair raw published residual suppressed cells 2026-W33/APAC 2 0 2 [1, 1] <- all suppressed 2026-W33/NA 4 3 1 [1] 2026-W32/EU 9 7 2 [2] The one pair matching the stated condition returns a sum across two suppressed cells, and no cell there has size 2. The pairs where the residual really is one cell's precise size — including a cell of size 1, a single individual — both fail that condition, so the comment steered away from the sharpest case. The rule is the number of suppressed cells under the pair, not whether all of them are suppressed. Also drops the previous commit's claim that vault.test.ts asserts a cell's precise size: line 110 asserts a (week, region) group size, which is a different quantity. Pins the audit log's copy-on-read. audits() freezing its return value is cosmetic; returning a copy is the guard, and without it a consumer holds the live array and can splice enforcement records away. It was the one immutability guard in the store whose removal left the whole suite green. AI-assisted (Claude Code); reviewed and understood by the author.
render_safe_chart re-serves entry.candidate.rows after release with no
recompute and no revalidation — it only checks that a receipt exists.
The freeze on the candidate object is the only thing stopping a
post-release in-process swap of that array, which store.ts:69-71
already claimed. No test asserted it. Verified end to end: with the
freeze removed, a legitimate prepare/validate/release followed by
entry.candidate.rows = [{region:"NA",ticket_count:1,group_size:1}]
makes the chart serve [{"region":"NA","ticket_count":1}] — a cell
covering one person, out through a public tool, which is the exact
claim this PR exists to make. The test whose name promises deep-frozen
candidates now asserts the candidate and entry objects too.
Cuts the safety line the groupSize comment drew in the wrong place.
The previous wording was arithmetically true but told a reader that
pairs with more than one suppressed cell are safe. From the caller's
actual position — published rollups plus groupSize probes, cell sizes
known to be 1 or 2 — the residual and the set of missing categories
usually solve the exact size of every suppressed cell under a pair,
including at pairs with two of them; one of the two pairs the old rule
named is in fact the one case that stays ambiguous. The comment now
says what groupSize really adds, which is localization: that size-1
cells exist is already public from rowCount and suppressedCells, but
this narrows one to a single week, region, and two categories.
Corrects two over-claims of my own. The audits() comment called
splicing "the one fail-open the audit trail cannot survive" — the copy
is shallow, so rewriting an outcome on a shared record is a second
route, guarded by the per-record freeze. And 859cc12's message said
the audits() copy was the only store immutability guard whose removal
left the suite green; six others do too, five of which stay
deliberately unpinned as defence in depth with no reachable defect.
AI-assisted (Claude Code); reviewed and understood by the author.
Owner
Author
|
/review |
|
Code review by qodo was updated up to the latest commit 81ed3fd |
The prepared-entry cap evicted the oldest entry unconditionally, so a successfully released query lost its candidate after 500 later preparations and render_safe_chart answered unknown_query_id while the receipt still existed — an orphan receipt promising rows the vault had discarded. Eviction now skips entries that hold a receipt: released entries are pinned for the process lifetime, the same deliberate trade already made for the receipts themselves, and the cap keeps bounding what an unreleased prepare loop can retain. Regression test releases a query, prepares 500 more, and asserts the chart still serves it; verified failing before the fix. Found by Qodo review on 81ed3fd. AI-assisted (Claude Code); reviewed and understood by the author.
Owner
Author
|
Dispositions for the three new findings (review of 81ed3fd): 2. Released chart data gets evicted — confirmed and fixed in 01f1866. Reproduced exactly as described: release a query, prepare 500 more, and 1. Audit log enables memory exhaustion — declined. The uncapped audit log is a deliberate, documented fail-closed decision, not an oversight: dropping enforcement records to reclaim memory is precisely the fail-open a release-clearance vault cannot make. The exposure is bounded in practice — the surface is loopback-only, every caller-controlled field in a record is clipped at the write (queryId to 64 chars, details to 120), and a record is a few hundred bytes, so degrading the process takes millions of completed HTTP round trips from the same local machine that could more easily just kill the process. A durable append-only sink is the right production answer and the wrong scope for this phase; the in-memory log is the phase's audit evidence. 4. Preparation falsely marked non-destructive — declined. With 01f1866 in place the premise no longer holds: eviction can never remove a released entry, so |
AI-assisted (Claude Code); reviewed and understood by the author.
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a network-facing, security- and privacy-critical vault boundary (PII suppression, evidence revalidation, DNS-rebinding defenses) whose correctness warrants final human verification despite no defects being found.
Review details
- Files reviewed: 9/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This was referenced Aug 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 3 — Vault MCP server
The vault's public surface over Streamable HTTP: exactly five tools, no raw-row query tool exists, and only
release_resultis destructive.What this adds
src/server/mcp.ts): stateless Streamable HTTP overnode:http, loopback-only bind, 1 MB body cap (SDK 1.30 dropped its own guard), chunked bodies refused. Verified end-to-end with the SDK client over a real socket.describe_dataset: schema, sensitivity labels, policy constants, one aggregate count — no code path touches a row value (payload scanned for canary/fixture values in tests).prepare_analysis: mission authorization strictly precedes data access (denied mission → zero SQLite queries, proven with an instrumented database); aggregation accepts only allowlisted identifiers; small cells are suppressed inside the vault; the candidate is stored under a vault-issuedqueryId.validate_release: contract verdict + hashes over the vault-stored candidate only. A test smuggles a contract-violating candidate straight into the store to prove storage is not trust.release_result: loads only vault-stored evidence, re-verifies the full contract and both hashes, then recomputes the aggregation from the live database — a candidate the vault cannot reproduce (fabricated group sizes, regrouped cells) is denied asevidence_mismatch. Every call writes an audit record; only success writes a receipt, at most one perqueryId, enforced again at the write. This closes the PR Phase 2: deterministic ReleaseContract library #3 trust-boundary finding as planned.render_safe_chart: renders released aggregates only (no receipt → refused), nevergroup_size.Fail-closed guarantees under test (116 tests total)
group_size: 3passes every contract rule and is caught only by execution-time evidence recomputation.AI usage
Built with AI assistance (Claude Code); every change reviewed and understood by the author. Internal quality-gate review ran before this PR; all six of its findings are fixed in the final commit.