feat(mcp): add create_frame and update_frame tools - #73
Merged
Conversation
Closes #51. The MCP endpoint was read-only. Writes now go through the same RBAC-enforcing service path the Connect API uses rather than a parallel implementation: PublishFrame's body moved into PublishDoc, and a PublishIntent decides create versus update next to the permission checks it interacts with. Creating requires the publisher or admin role; writing to an existing Frame requires edit permission on it. The Connect RPC keeps its upsert behavior. Tool input is typed after frames.Doc so a client is guided by the schema instead of authoring YAML blind, and validation stays with frames.Validate so there is one definition of a valid Frame. update_frame merges onto the Frame's current document. Anything the caller omits keeps its value; an explicitly empty list clears. The merge base is the Frame's own stored document, deliberately not the composed form get_frame returns: merging onto a resolved document would copy every parent's slots into the child and drop its extends edges, destroying the inheritance graph with no error. SourceDoc provides that unresolved read, and the tool description now tells clients not to feed get_frame output back in. Without the merge, an update also erased visibility, scope, and maintainer, so a Frame declared private silently read as internal. The 512KB per-version content cap the design doc promised is now real, enforced in the shared publish path so both front doors are covered. It matters more with an LLM authoring content that lands verbatim in a single-writer database. The design doc had excluded writes for blast-radius reasons; that decision is marked superseded, including the prompt-injection threat that is specific to exposing writes as an AI tool. delete_frame is deliberately not included.
Follow-up to the write-tools work, from a second review pass. update_frame preserved a Frame's extends edges, but nothing gave a client the Frame's own slot values. The only read available returned the composed form, where list slots are a union across the whole ancestry and prose slots are an override. So the obvious instruction - "add a rule to this Frame" - forced a model to send the parents' content back as the child's own. That validated, composed identically, and left no signal, while detaching the child from its parents' future revisions: a later parent edit shows up alongside the stale copy, and overridden prose never tracks the parent again. get_frame now takes source=true, which returns only the Frame's own content via SourceDoc. The default stays composed, which is what a consumer loading context wants. Both tool descriptions say which read to use for which purpose, so the prohibition on resending composed output now comes with an alternative. Test hardening from the same review: - TestApplyToWiresEveryInputField sets every input field to a sentinel and asserts nothing in the resulting document is left zero. The existing guard forced a new slot to gain an input field but not to be wired into applyTo, and the hand-written comparison could not catch that because an unwired field is zero on both sides. - The size cap boundary is pinned exactly, so the comparison cannot drift between > and >=. - The create-over-existing-name test uses a different version, so it exercises the create-intent check rather than version uniqueness. Also: the optional string fields now document that an empty string clears them, description says it is required when creating, and the FailedPrecondition arm notes it is defensive rather than evidence that publish checks acyclicity. Two corrections to the design doc's security section: an admin token is the real worst case for a compromised MCP token, since rbac.Can allows an admin every frame before grants are consulted; and merge semantics are not a prompt-injection mitigation, since an injected call is well-formed and can clear parents deliberately.
|
📄 Docs preview for |
Keycloak does not honour the RFC 8707 `resource` parameter, so without an audience mapper its tokens carry no audience for the MCP resource and the /mcp endpoint rejects every one of them with 401. docs/connect/keycloak-setup.md describes adding this mapper by hand; for the local loop it belongs in the realm the loop imports. The audience is the canonical resource identifier the server derives from FRAMES_PUBLIC_URL, which dev/dev-auth.sh sets to http://localhost:5173. Without this, the write tools added in this branch cannot be exercised against `make dev-auth` at all - only in dev mode, which skips token validation entirely and so proves nothing about the authenticated path.
…st bodies Closes #69, #71. Part of #70. Three ways a write could go wrong that the tools made reachable. **Concurrent updates silently lost one.** update_frame reads the current document as its merge base, so two parallel calls both merged onto the same base, picked different version strings so nothing collided, and both reported success - one caller's change simply vanished. Parallel tool calls are ordinary for an LLM client, so this was not a corner case. PublishDocFrom takes the version the caller believes it is editing, and rejects the publish with FailedPrecondition when the frame has moved on. update_frame passes the version it actually read, so the guard cannot be inert. PublishDoc keeps its signature and means "unchecked", which is what the Connect RPC has always done. **Publishing an older version silently unpublished newer content.** latest_version was assigned unconditionally, so a model that misread the current version could demote a frame: every default read, and the merge base of the next update, then resolved to the older document. A version below the current latest is now rejected. Republishing the current version still reports AlreadyExists, which says more than "does not advance". **Request bodies were unbounded.** The SDK reads a body in full before any tool handler, and therefore before RBAC, so a caller with no write permission could make a single-replica deployment buffer an arbitrary amount. Capped at 8 MiB, well above the content limit so a create at that limit still fits with its framing. The Connect half of the body cap lives on the branch that owns server.go.
The concurrency check added in the previous commit was inert on the path that matters. update_frame derived the base version from its own read at publish time, so the value always matched whatever was current and the check could never fire. Live testing against two MCP sessions showed the lost update happening exactly as before: both published, both reported success, and the first caller's rule was gone from the stored document. The window that loses a change is between the client's read and its write, so only the client knows what it read. base_version is now a required input on update_frame, asserted against the frame's current latest, and the merge still happens onto the current document. A caller that does not read first cannot write. get_frame now reports the frame's version, in both the composed and the source views. Without it a client had no way to obtain the value it is now required to send. The integration test drives this through the tool the way a client does, rather than calling the service directly with a base the test chose - the latter is what let the inert version pass.
The web form marks the version input for a duplicate version but showed a generic form-level error for a version that does not advance, because the latter was a plain InvalidArgument with no details. Both are errors about the same field, so both now carry a FieldViolation on `version` and the CLI names the field too.
jbouder
requested changes
Aug 21, 2026
jbouder
left a comment
jbouder
left a comment
Collaborator
There was a problem hiding this comment.
One blocker inline — the body cap is wired so it only takes effect in dev mode, the inverse of the intent. One-word fix. Everything else I found is minor and can follow up.
From review. The bearer middleware wrapped mcpHandler rather than the capped handler, so the cap was discarded in every deployment that has authentication on - the inverse of what its comment claimed. The test only covered dev mode, which is the single branch where the wrapper survived, so CI was green. It now runs both modes and asserts a control: a small body must succeed before an oversized one is expected to fail, otherwise an unrelated rejection - a 401, say - would satisfy the assertion without the cap existing at all. Measured before the fix: with bearer auth, 8 MiB + 1 KiB returned 200. The accepting stub verifier needs a live expiry, because the middleware treats a zero Expiration as expired and answers 401 - which is what made the first attempt at this test pass vacuously.
The MCP section described a read-only adapter. It is now also a write surface, and two of its rules are the kind that look like details until they are broken - both were broken during this branch's development and caught in review. The merge base for update_frame must be the frame's own document, not the composed form get_frame returns by default, or an edit copies every parent's slots into the child and drops its extends edges. And the base version it checks must come from the caller, not from a fresh server-side read, which always matches and makes the check inert. Also records what publish now enforces in one place, and a testing note: asserting only that something was rejected proves nothing, because an unrelated 401 or a parse failure satisfies it.
jbouder
approved these changes
Aug 21, 2026
This was referenced Aug 21, 2026
jbouder
added a commit
that referenced
this pull request
Aug 24, 2026
…truncation Rebased onto main, which brought #72, #73, #76, and #77. The rebase produced no git conflicts but two packages that did not compile, plus a migration collision that stopped the server booting. Fixes those, the codec data-loss bug, and the smaller items from the same review. Blockers: - Port the MCP write surface to the free-form body. `write.go` declared ten slot-named input fields against a `frames.Doc` that no longer has them. It now carries `body` and `template`: `body` because that is what a Frame's content is, and `template` because the reflective guard in `resources_test.go` asked for a decision and the answer is yes — without it `create_frame` cannot make a template at all, and an omitted-means-keep pointer stops `update_frame` de-listing one by accident. The guard now walks `frames.Doc` alone, since `SlotTable` is gone. - Renumber `005_frame_is_template.sql` to 006. main's `005_canonical_membership_ email.sql` claimed the same version, and goose rejects duplicates at provider construction, so no migration ran at all. Adds the missing `-- +goose Down`. `migrate_legacy_test.go` builds a schema as of 004 and did not carry the frames table, so 006's ALTER failed there as "no such table"; the fixture now carries every table a later migration touches, and asserts a pre-006 frame comes forward as not-a-template. - Match the frontmatter delimiter only at column 0. `TrimSpace(line) == "---"` let an indented `---` inside a YAML block scalar close the frontmatter, which truncated the document and dropped every field after it — including documents the exporter itself produced, where the error named a field the author never touched. Table-driven cases cover the block scalar in both positions, a `---` in the body, and a full round trip through the codec's own output. The unqualified "lossless" claims are replaced with the two normalizations that actually happen. Should-fix: - `frame-yaml.ts` claimed to mirror `legacy.go` and did not: no two-space continuation indent, and `.trim()` where Go trims newlines only. That reaches storage, since restoring a legacy version re-serializes the TypeScript render as canonical content — one rule with a nested list became three flat rules, permanently. The port is now faithful, and both sides are pinned to one shared fixture, `testdata/legacy-slots/`, compared whole rather than by substring. Substring assertions over single-line values are what let the drift through. Also: - Pin the legacy-vs-`body:` precedence, which was silent and untested, and stop `Parse` naming the unexported `frames.docYAML` in unknown-key errors that reach API clients unwrapped. The error now lists the recognized keys. - Cover `is_template` at the sqlite level: four SELECT lists and four Scan calls, none of them exercised by the service tests, which use `store.NewMemory()`. Asserts both the true and false case and the columns either side, so a scan-order mismatch fails. - Guard the empty version in `compose.go`'s `Inherits from:` line, matching `MarshalMarkdown`. - Give the one page-level heading that cannot use `PageHeader` the classes `PageHeader` exists to hold steady. - Rewrite the parts of AGENTS.md and the three design docs that described the deleted slot design as current. The MCP doc's §3.4 gets the struck-through supersede treatment #73 set the precedent for on that same file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Closes #51.
What this does
The MCP endpoint was read-only.
create_frameandupdate_framenow expose writes to any MCP-capable client, gated by the same RBAC the Connect API uses.The issue's central requirement was that writes reuse the existing validation and permission checks rather than reimplement them.
PublishFrame's body moved intoPublishDocplus an internalpublish, so both front doors execute one copy of the logic: creating needs the publisher or admin role, writing to an existing Frame needs edit permission on it. APublishIntentdecides create-vs-update inside the service, next to the checks it interacts with, rather than in the MCP adapter. The Connect RPC passesPublishUpsertand keeps its existing behavior.Tool input is typed after
frames.Doc, so a client is guided by the schema instead of authoring YAML blind. The adapter deliberately does not validate -frames.Validateremains the single definition of a valid Frame.delete_frameis intentionally out of scope; it is #67.The part most worth reviewing
update_framemerges onto the Frame's current document: anything omitted keeps its value, an explicitly empty list clears. The merge base isSourceDoc- the Frame's own stored document - and never the composed form.This matters more than it sounds. List slots are a union across the whole ancestry and prose slots are an override, so feeding composed output back into a write copies the parents' content into the child. It validates, it composes identically afterwards, and it leaves no signal - but the child has silently stopped tracking its parents' future revisions. Two review rounds landed on this from different angles:
visibility/scope/maintainer, so a Frame declaredprivateread asinternal;get_frametherefore takessource=true. The default stays composed, which is what a consumer loading context wants; the source read is what an editor needs, and both tool descriptions say which is which.Also in here
A 512KB per-version content cap (
frames.MaxContentBytes), enforced in the shared publish path so both front doors are covered. The design doc had promised this cap for some time; it was implemented nowhere. It matters more with an LLM authoring content that lands verbatim in a single-writer SQLite database.Reviewers please note: this is a new rejection for existing Connect/CLI/web callers. If any deployed Frame is already over 512KB, its next publish fails. I have not checked whether the CLI and web editor surface that error usefully or as a generic validation failure after the user has composed a long document.
Review history
Two rounds of independent review. Both confirmed by mutation that there is no RBAC bypass and no behavior change for existing Connect callers, and independently re-derived that a stale per-session claims closure cannot execute as another user (the SDK pins the session to
TokenInfo.UserID, and role is re-read from the database on every call).Three defects fixed along the way: the metadata loss and the flattening described above, plus an extraction that re-marshalled the document and would have rewritten every Connect publish's stored bytes - stripping author comments and changing digests for unchanged content.
One test-quality fix worth calling out: a comment claimed a round-trip test enforced slot parity. It did not - it compared against a hand-written literal, so a new slot was zero on both sides and passed. That is precisely how the metadata loss slipped through. There are now two reflective guards: one that fails when
frames.DocorSlotTablegrows a field the tool input lacks, and one that fails when an input field exists but is not wired intoapplyTo. Both were mutation-verified.Testing
go test ./... -racegreen,golangci-lint run ./...clean. The integration tests wire the realframes.Servicewith nothing stubbed on the permission path, and the denial cases assert that nothing was persisted, not merely that an error came back. The size-cap boundary is pinned exactly, so the comparison cannot drift between>and>=.Follow-ups filed
delete_frameover MCP, including whether force-cascade should be reachable from an AI clientupdate_framecalls silently lose onelatest_versionTest evidence
Beyond
go test ./... -raceandgolangci-lint, the write tools were exercised over the real MCP endpoint with real bearer tokens, againstmake dev-auth(not dev mode - dev mode skips token validation entirely and so proves nothing about the authenticated path).Both users obtained tokens through the genuine authorization-code + PKCE flow. Tokens carry the MCP resource audience via the mapper added here; without it every call is rejected with 401.
Two users, opposite permissions.
aliceis a publisher,bobholds only the org-level read grant:Both users see all four tools listed; permission is enforced when a tool is called, which is what the connect docs describe.
The last three assertions are the first review blocker, confirmed fixed on a live server: alice's update supplied only
rules, and the spec metadata survived in the stored document. Before the merge semantics, that update erased it.Inheritance, the second blocker. alice created a parent and an inheriting child over MCP, then updated the child:
The stored child after the update:
The pin is intact and no ancestor content was absorbed, while the composed view still merges the parent in. That is the whole point of
get_frame source=true: without it, a client editing a slot has no way to see the Frame's own values and must send the parents' content back as the child's.Concurrency, version ordering, and body size were added after the first round of evidence above, and verified the same way:
A's rule survived in the stored document, which is the point: before this, B's publish silently discarded it.
Worth flagging for review, because it is the kind of mistake that looks fixed: the first version of the concurrency check derived the base version from a read inside
update_frame, so it always matched and could never fire. Unit tests passed because they called the service directly with a base the test chose. Live testing showed the lost update happening exactly as before. The base now comes from the caller,base_versionis required onupdate_frame, and the integration test drives the tool the way a client does so an inert check fails.Client-side error surfacing was checked rather than assumed. The web form marks the
versionfield for a duplicate version, so a non-advancing version now emits aFieldViolationon the same field instead of a form-level error; the CLI names the field too. The 512KB cap surfaces through the same path as any other invalid document, carrying the byte counts.Review round two
One blocker from @jbouder: the body cap was wired so the bearer middleware re-wrapped the uncapped handler, discarding it in every deployment that has authentication on - the inverse of what its comment claimed. Measured before and after:
The sharper half of that finding was why CI stayed green: the test used
DevMode: true, the one branch where the wrapper survived. It now runs both auth modes and asserts a control - a small body must succeed before an oversized one is expected to fail. Without the control the non-dev case passes on a 401 with no cap present at all, which is exactly what my first attempt at the test did. The accepting stub verifier also needs a live expiry, since the middleware treats a zeroExpirationas expired.Not covered: no cluster deployment.