Skip to content

Anchor the MCP allocation to the database, and nothing else - #40

Merged
takashi-matsuyama merged 1 commit into
mainfrom
fix/mcp-allocation-anchor
Aug 10, 2026
Merged

takashi-matsuyama merged 1 commit into
mainfrom
fix/mcp-allocation-anchor

Conversation

@takashi-matsuyama

Copy link
Copy Markdown
Member

The MCP allocation and the row-access grant have shared an anchor since #37, and they should not: they answer different questions.

  • The grant is an authorization. The approval dialog named a database, and it named it with a schema list and a credential state — widen the schemas and the same grant reaches rows nobody approved; change which credentials it is exercised with and it is a different act. It keeps all three.
  • The allocation is a destination — a port, a capability path and a locator id, which is what an AI client's pasted config points at. Only a change of database can make a pasted entry answer for the wrong one. Sharing the grant's anchor charged a repaste for edits that moved nothing.

What "the same database" is

dbIdentityKey — host, port, database name — the definition this codebase already owns and already uses for the duplicate warning, so there is one answer to that question instead of two that disagree.

A raw string compare (which this branch first used) is strictly finer and calls :5432 and the default port, postgres:// and postgresql://, localhost and 127.0.0.1 different databases. That matters more than it looks: there is no edit form in this app. The only form is "+ Add database", never prefilled, so changing a schema list means retyping the whole URL — and each of those spellings would then charge exactly the repaste this change exists to avoid. The username is not part of the key either, so a role change keeps the allocation and the grant is what re-asks.

Widening the schema list keeps the destination but clears autoStart. Without that, an operator who adds a schema gets the wider set served to an already-pasted config on the next launch with no action of their own; the explicit start is what says yes to it. Narrowing, reordering, a password change and a role change all keep autoStart.

Safety: "no worse", both sides stated in the code

The motive is ergonomic, not safety.

  • Closed: a profiles.json carrying a password in url by hand looked identical to main, so no stop — while the store's credential state flipped and the allocation went out from under a running server.
  • Left open (narrower, and commented): if that hand-written password also holds a broken percent-escape, splitDbUrl throws here while joinDbUrl replaces it, so main can see no change while this discards. Either way the cost is an orphaned locator file, which can only fail a bridge or reach the server it already named.

The ordering invariant ("allocation discarded implies main stopped") holds because a discard requires both the identity keys and the normalised strings to differ, and equal raw strings cannot produce different normalised ones — so main, which compares raw strings, has already stopped. An earlier draft justified this by claiming main's own read throws on an unparseable URL; it does not (joinDbUrl returns early when no password is stored), and that sentence is gone.

Surfaces corrected

Four places stated the old rule: README twice, mcpAllocation.ts's header, src/shared/types.ts (the allocation, autoStart and bridgeId docs — autoStart's is the one that matters, since its survival is the point), and the test comment sitting three lines above the tests this inverts.

Coverage

The only repoint previously tested changed the database name, so a pathname-only comparison would have passed while a prod-to-staging edit kept the port, path, bridge id and autoStart. There are now tests for host and for port, for a role change, for three spellings of one database, for the query-param forms the identity key declines (where the normalised string is the fallback), and for autoStart — which is false in a fresh allocation, so nothing before this pinned it. Broken four ways while writing it, each failing a different test.

Local: typecheck / lint / test:unit (344) green. test:e2e green on 6 consecutive runs after pnpm build. One flake to declare: on the first full-suite run on this branch, browse.spec.ts:335 failed and the run took 44s against a normal 15s, which points at that test's own expect(grid).toBeVisible({ timeout: 30_000 }). It has not reproduced in 6 subsequent runs (1 isolated, 5 full-suite, one of them immediately after a fresh build, i.e. the same conditions), and the exact assertion text was lost when the next run cleared test-results/. Not diagnosed, not claimed green in that one run.

Deferred / not covered

  • Residual hole above (percent-escape), stated in the code, cost is a stray locator file.
  • index.ts's connectionChanged — the other half of the ordering invariant — is touched by neither a unit test nor an e2e.

Design-challenge candidates raised by review and blocked (operator's call)

Reported rather than applied:

  1. Use dbIdentityKey for the grant too, not just the allocation.
  2. Build the edit form first — review argued this is the true defect, and that anchoring rules are a workaround for its absence.

The allocation and the row-access grant have shared an anchor since #37, and
they should not: they answer different questions.

The grant is an authorization. The approval dialog named a database, and it
named it with a schema list and a credential state — widen the schemas and the
same grant reaches rows nobody approved; change which credentials it is
exercised with and it is a different act. It keeps all three.

The allocation is a destination: a port, a capability path and a locator id,
which is what an AI client's pasted config points AT. Only a change of database
can make a pasted entry answer for the wrong one. Sharing the grant's anchor
charged a repaste for edits that moved nothing.

"The same database" is `dbIdentityKey` — host, port and database name, the
definition this codebase already owns and already uses for the duplicate
warning, so there is one answer to that question rather than two that disagree.
A raw string compare (which this commit first used) is strictly finer and calls
`:5432` and the default port, `postgres://` and `postgresql://`, `localhost` and
`127.0.0.1` different databases. That matters more than it looks, because there
is no edit form in this app: the only form is "+ Add database", never prefilled,
so changing a schema list means retyping the whole URL — and each of those
spellings would then charge the repaste this change exists to avoid. The
username is not part of the key either, so a role change keeps the allocation
and the grant is what re-asks.

Widening the schema list keeps the destination but clears autoStart. Without
that, an operator who adds a schema gets the wider set served to an
already-pasted config on the next launch with no action of their own; the
explicit start is what says yes to it. Narrowing, reordering, a password change
and a role change all keep autoStart.

The motive is ergonomic, not safety. The safety story is "no worse": one hole
closed, one narrower one left, both stated in the code. Closed — a profiles.json
carrying a password in `url` by hand looked identical to main, so no stop, while
flipping the credential state in the store and taking the allocation out from
under a running server. Left — if that hand-written password also holds a broken
percent-escape, `splitDbUrl` throws here while `joinDbUrl` replaces it, so main
can still see no change while this discards. Either way the cost is an orphaned
locator file, which can only fail a bridge or reach the server it already named.

The ordering invariant ("allocation discarded implies main stopped") now holds
because a discard requires both the identity keys and the normalised strings to
differ, and equal raw strings cannot produce different normalised ones — so main,
which compares raw strings, has already stopped. An earlier draft justified this
by claiming main's own read throws on an unparseable URL; it does not,
`joinDbUrl` returns early when no password is stored, and that sentence is gone.

Four surfaces stated the old rule and are corrected: README twice,
`mcpAllocation.ts`'s header, `types.ts` (the allocation, autoStart and bridgeId
docs — autoStart's is the one that matters, since its survival is the point), and
the test comment sitting three lines above the tests this inverts.

Coverage: the only repoint previously tested changed the database NAME, so a
pathname-only comparison would have passed while a prod-to-staging edit kept the
port, path, bridge id and autoStart. There are now tests for host and port, for a
role change, for three spellings of one database, for the query-param forms the
identity key declines (where the normalised string is the fallback), and for
autoStart — which is false in a fresh allocation, so nothing before this pinned
it. Broken four ways, each failing a different test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@takashi-matsuyama
takashi-matsuyama force-pushed the fix/mcp-allocation-anchor branch from b38850b to c481660 Compare August 10, 2026 15:47
@takashi-matsuyama
takashi-matsuyama merged commit 56fe5f1 into main Aug 10, 2026
2 checks passed
@takashi-matsuyama
takashi-matsuyama deleted the fix/mcp-allocation-anchor branch August 11, 2026 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant