feat(pages): partial page editing without resending whole pages - #22
Conversation
The BookStack API supports only full replacement of page content: `PUT
/api/pages/{id}` takes a complete `html` or `markdown` body and there is no
PATCH. So changing one paragraph of a long page means reading all of it, having
the model reproduce it verbatim with the change applied, and sending it all back.
The content crosses the model twice, and the entire page rides on it being copied
byte-perfectly. `bookstack_pages_update`'s own description says as much today:
"to append you must resend the existing content plus the addition".
This adds three tools that run the read-modify-write cycle inside the server, so
a caller sends only the fragment it wants changed. Existing behaviour is
untouched: `bookstack_pages_update` still replaces the whole content field, and
`bookstack_pages_read` called without any of the new options returns exactly the
object it always did.
## Tools
- `bookstack_pages_edit` - literal find-and-replace. Each edit replaces an
`old_string` with a `new_string`, applied in order. Guards below.
- `bookstack_pages_append` - insert at the end of the page, the end of a named
section, or directly after a section heading (`position: "start"`).
- `bookstack_pages_outline` - heading structure with level, exact text, offset
and section size, plus which editor the page uses. Transfers no content.
`bookstack_pages_read` gains `grep`, `case_sensitive`, `context`, `max_matches`,
`offset`, `length` and `metadata_only`, so a large page can be searched or
windowed instead of read whole. `grep` searches the STORED source, which is what
makes a returned excerpt usable verbatim as an `old_string` anchor.
## Two invariants
Both are load-bearing and both are in src/utils/page-content.ts:
- Markdown pages (`editor === 'markdown'`) are patched and written through
`markdown`. Writing `html` to one switches the page's editor type, and the API
offers no way back.
- Every other page is patched against `raw_html`, the stored source - never
against `html`, the rendered output. Patching the rendered output would write
back expanded page-include tags and destroy the includes permanently.
## Guards
Aimed at the failure modes an LLM caller actually hits:
- An `old_string` must match exactly once, or the edit is refused rather than
applied to the wrong place. The error carries the first few matches with
context. `replace_all` is the explicit opt-in for a rename that repeats.
- When an anchor is not found, the error reports the same text found with
DIFFERENT WHITESPACE. That is by far the most common near-miss - a model
reproduces the sentence with collapsed whitespace - and reporting only "not
found" leaves it with no way forward.
- `dry_run` applies the edits in memory and reports what would change without
writing anything.
- `expected_updated_at` is an optimistic lock against the page's `updated_at`.
Without it a concurrent edit is silently overwritten.
- A result smaller than half the original is refused unless `allow_shrink` is
set, so an anchor whose closing text appears earlier than intended cannot
swallow the document.
- After a write the page is re-read and the written fragments are looked for in
NORMALISED text. BookStack re-generates heading anchors and injects `id`
attributes on save, so the bytes that come back are not the bytes that were
sent and a byte comparison would report every success as a failure. A fragment
that cannot be found yields `verified: false` rather than an error - the write
did happen, and the caller needs both facts.
No response from these tools contains page content; they exist so that it does
not travel through the model. Every write creates a BookStack revision, so an
applied edit remains reversible in the UI.
## Error mapping
`PageContentError` and `PageStaleError` are mapped in `ErrorHandler.handleError()`
rather than at the tool, so McpError stays confined to src/utils/errors.ts as it
is today. `PageContentError` becomes InvalidParams: every case is a fact about
what the caller sent, and the `details` bag - available section names, ambiguous
matches, the whitespace-tolerant match - is what lets a model fix its own call.
Falling through to the generic branch would report it as InternalError and drop
all of that. `PageStaleError` is separate and becomes InvalidRequest: the
arguments were right, the page moved, and the client should re-read rather than
rewrite its anchor.
## Contract and tests
- `MCPSchemaNode` gains `minItems`/`maxItems`, so the published lower bound on
the `edits` array agrees with the zod rule, the way `minLength` already pairs
with `z.string().min(n)`.
- New schemas use `z.strictObject` and the shared `entityId`; `id` lives in the
schema and the handler destructures the validated object.
- tests/unit/id-schema-contract.test.ts: 7 new integer classifications and 3 tool
bases. `context` carries `minimum: 1` rather than a practical floor of 20,
because the exact-minimum rule would otherwise need a fifth category for one
property - and the load-bearing bound there is the maximum, which is what keeps
a grep from returning the whole page.
- tests/unit/page-content.test.ts (33 tests) covers the helpers, both invariants
and every diagnostic.
- tests/unit/pages.test.ts (19 tests) drives the handlers against a recording
client with the REAL validator, asserting which field is written, that nothing
is written when a guard fires, and that responses carry no content.
- Tool count 56 -> 59 in tests/transport/tools.test.ts, tests/transport/
stdio.test.ts, README, docs/ and docker-compose.yml.
`bunx tsc --noEmit` clean, `bunx biome check .` clean, `bun test` 297 passing.
a14cc12 to
402782f
Compare
pnocera
left a comment
There was a problem hiding this comment.
Requesting changes before merge. I reviewed the current head (402782f) and found three blockers:
-
BLOCKER [contract-drift] — unbounded grep result (
src/utils/page-content.ts:325-356;src/tools/pages.ts:375-400): an allowed regex such as[\s\S]*puts the complete stored page inmatches[0].match.contextandmax_matchesdo not bound a single match, contradicting the narrowed-read/large-page contract. Use a safe bounded query design: cap each match and aggregate output, return offset/length/truncation metadata for oversize matches, and either constrain regex syntax or use a safe regex engine. Add regression coverage for whole-document and pathological patterns. -
BLOCKER [correctness] — expected_updated_at is a preflight, not an optimistic lock (
src/tools/pages.ts:761-766,904-909,1038-1047, followed by an unconditional PUT at1066): another writer can update aftergetPage()but before the PUT, and this request overwrites it. Implement an atomic upstream conditional update if BookStack supports one and integration-test the race; otherwise rename/re-document this as best-effort stale detection and remove the no-overwrite guarantee. -
BLOCKER [correctness] — deletions falsely report verified (
src/tools/pages.ts:793-800;src/utils/page-content.ts:275-285): edit verification passesnew_string; for deletion it is empty, andcontainsNormalized(..., "")always returns true. An unchanged reread after a deletion can therefore returnverified: true. Model required-present and required-absent expectations separately, then add the unchanged-reread deletion regression.
I ran bun run typecheck plus the two new unit suites (52 passing); their current coverage does not exercise these cases. Please push a focused fix set to this PR, then request re-review.
pnocera
left a comment
There was a problem hiding this comment.
Resolved the requested correctness issues on the PR branch and gated the final head.
Validation:
bun test: 538 passed, 5 configured Docker smoke skips, 0 failedbun run typecheck- Claude implementation review (round 4): GO; durable artifact records its non-blocking residual risks.
The problem
BookStack's API only supports full replacement of page content.
PUT /api/pages/{id}takes a completehtmlormarkdownbody, and there is no PATCH endpoint.For an LLM client this is expensive. Changing one paragraph of a long page means reading the whole page, reproducing it verbatim with the change applied, and sending all of it back. The content passes through the model twice, and correctness depends on the model copying thousands of tokens without a single slip.
The repo already documents the workaround, in
bookstack_pages_update's own description:This PR moves that read-modify-write cycle into the server, so a client sends only the fragment it wants changed.
Existing behaviour is unchanged.
bookstack_pages_updatestill replaces the whole content field.bookstack_pages_readwithout any of the new options returns the same object it always did, and there is a test asserting that by object identity.What's added
bookstack_pages_editold_stringfor anew_string, applied in order.bookstack_pages_appendbookstack_pages_outlinebookstack_pages_readgainsgrep,case_sensitive,context,max_matches,offset,lengthandmetadata_only, so a large page can be searched or read in windows.grepsearches the stored source, not the rendered output, which is what makes a returned excerpt usable as anold_stringanchor without modification.A typical sequence on a large page:
Two rules about which field to patch
Both live in
src/utils/page-content.tsand both have direct test coverage.Markdown pages (
editor === 'markdown') are patched and written throughmarkdown. Writinghtmlto a markdown page changes its editor type, and the API provides no way to change it back.Every other page is patched against
raw_html, the stored source, and never againsthtml, the rendered output.htmlhas page includes already resolved, so writing a patched copy of it back would replace every{{@42}}include with a frozen snapshot of its target. An anchor taken from the rendered output would also fail to match on write.Guards
These target the mistakes an LLM client makes in practice, not malicious input.
An
old_stringmust match exactly once. If it matches more often the edit is refused instead of applied to the first hit, and the error includes the first few matches with surrounding context.replace_allis available for a rename that legitimately repeats.If an anchor is not found, the error reports whether the same text exists with different whitespace, and includes the actual bytes. This is the most frequent failure we saw in practice: a model reproduces a sentence with collapsed whitespace because that is how it reads on screen. Reporting only "not found" gives it nothing to act on.
dry_runapplies the edits in memory and reports what would change. No request reaches BookStack.expected_updated_atis an optimistic lock against the page'supdated_at. Without it, a concurrent edit by someone else is silently overwritten.A result smaller than half the original is refused unless
allow_shrinkis set. This catches an anchor whose closing text occurs earlier than the author expected, which would otherwise delete most of the document.After a write, the page is read again and the written fragments are looked for in normalised text. BookStack regenerates heading anchors and injects
idattributes on save, so the bytes returned differ from the bytes sent and a byte comparison would report every successful write as a failure. A fragment that cannot be found yieldsverified: falseinstead of an exception, because the write did happen and the client needs both facts.None of these tools return page content in their responses. Each write creates a BookStack revision, so an applied edit can still be rolled back from the UI.
Error mapping
PageContentErrorandPageStaleErrorare mapped inErrorHandler.handleError()rather than at the tool, soMcpErrorstays confined tosrc/utils/errors.tsand the tool classes keep throwing plain errors.PageContentErrormaps toInvalidParams. Each case describes something about the caller's input, and thedetailsobject carries what the client needs to correct itself: available section names, ambiguous matches, or the whitespace-tolerant match. The generic branch would report these asInternalErrorand discarddetails.PageStaleErrormaps toInvalidRequestand is a separate class for that reason. The arguments were valid and the page moved, so the correct client response is to re-read and retry, not to change the anchor.Contract and tests
MCPSchemaNodegainsminItemsandmaxItems, so the published lower bound on theeditsarray matches the zod rule, the wayminLengthalready pairs withz.string().min(n).New schemas use
z.strictObjectand the sharedentityId.idis part of the schema and the handler destructures the validated object.tests/unit/id-schema-contract.test.tsgains 7 integer classifications and 3 tool bases. One judgement call worth flagging:contextpublishesminimum: 1. I first wrote a practical floor of about 20, but the exact-minimum rule in that test would then need a fifth category for a single property. The bound that matters there is the maximum, which is what stops a grep from returning the whole page.tests/unit/page-content.test.ts(33 tests) covers the pure helpers, both field-selection rules, and every diagnostic.tests/unit/pages.test.ts(19 tests) drives the handlers against a recording client with the realValidationHandler. These tools depend on schema defaults and onstrictObjectrejecting unknown keys, so a stubbed validator would test a contract that does not ship. The assertions cover which field is written, that nothing is written when a guard fires, and that responses contain no page content.Tool count 56 to 59 in
tests/transport/tools.test.ts,tests/transport/stdio.test.ts, README,docs/anddocker-compose.yml.bunx tsc --noEmitclean,bunx biome check .clean,bun test297 passing (integration suites skip without a live BookStack).Notes for review
This has been running against a live BookStack 26.05.2 instance in a fork for a while. Every guard above exists because the corresponding mistake happened first.
src/utils/page-content.tshas no client and no I/O, so the field-selection rules and the diagnostics are testable in isolation.Happy to split this if you prefer smaller pieces: the helpers plus the
pages_readoptions are independently useful, with the three tools as a second PR. Also happy to change naming, parameter shapes or descriptions.🤖 Generated with Claude Code