fix(mcp): re-sync the offline AUTHORING_INSTRUCTIONS fallback with the server primer [SAP-2959] - #734
Conversation
…e server primer (SAP-2959) The fallback is served only when the startup fetch of GET /v1/mcp/instructions fails — the one path with no other source of truth — and it had drifted two content releases behind the canonical primer (2.6 → 2.8). Offline sessions were therefore told App Links do not exist at all: no durable-sharing paragraph, no sapiom_dev_app_publish, no hosted or REST publish surface. The copy also still carried the pre-2.7 single-alias framing of the hosted capability MCP. The body is now byte-identical to the server's copy. Because the doc comment demanding that was the only thing holding the two together — and it failed silently across two releases — instructions.test.ts now pins sha-256 of the string to the same frozen digest the server-side spec pins for this content release. A one-sided edit reddens one repo or the other, with no network call from either suite; the `contain` assertions it joins all still passed against the stale text, which is why they could not catch this. Refs: SAP-2959
Review — PR #734 (
|
Review on the paired server-side PR caught the claim overreaching. Both pins are self-consistency checks against their own body, so the direction that caused this incident — a server-side content release — moved one copy while both suites stayed green. Saying "a one-sided edit reddens one repo or the other" was true only for an in-place edit of this file. The server-side spec now pins this same digest against its own current primer, which is what makes a release redden something. Say that, and say the limit out loud: neither pin can block a merge in the other repository. What the pair removes is the silent path, not the possibility. Refs: SAP-2959
Follow-up review — PR #734 (delta since
|
…the sync drops
Review caught that this PR's test rewrite deleted coverage whose subject matter is
still present. It was right, and the cause was mine: I read the base file from a
stale local branch rather than origin/main, then wrote the replacement wholesale
over a version 52 lines longer than the one I had looked at.
Restored, all verified passing against the new text: tool_use, "it forces a tool",
"treat missing as unknown", "reports both as `null` today", and the negative
not.toContain("If you must pin") that exists to stop a known-wrong line coming back.
The digest does not substitute for these — it is re-pointed on every content release
by design, so it catches accidental drift but never a deliberate regression made
during a sync, which is exactly when that line would return.
The ctx.shared quota test could not be restored: its subject really is gone. That
paragraph was in this fallback and not in the served primer, so online sessions never
saw it, and syncing removes it here. That is the direction the rule requires — one
canonical text, and the digest cannot hold if the copies differ by a paragraph — but
it was undisclosed. It now has a test naming the removal and its reason, and the
changeset says so instead of describing this as purely additive.
Also cut the published surface down to what a consumer can act on. tsconfig.build
emits instructions.ts's JSDoc into dist/instructions.d.ts, so CI mechanics and repo
layout were shipping in the npm tarball, as was the changeset's second paragraph in
a CHANGELOG that can never be edited. The sync rule and the digest's limits moved to
instructions.test.ts, which is excluded from the build.
Refs: SAP-2959
|
Four of five actioned; one declined with reasoning, and one correction to the review. Pushed in 499e2bf (and 8e5b5c4 for #2). 3. Deleted regression assertions — you're right, and the cause was worse than sloppinessRestored all five. The cause is worth stating plainly: I read the base test file from a stale local branch instead of Restored and verified passing against the new text: Your reasoning for why the digest doesn't cover them is the part I'd gotten wrong: it is re-pointed on every content release by design, so it catches accidental drift but never a deliberate regression made during a sync — which is precisely when "If you must pin" would come back. Negative assertions and the digest are orthogonal. They're back. 4. The
|
|
For the record, since the last review on this page reads "Request changes" against findings that were fixed in the commit that merged (
Verified against merged Still open, deliberately not in this PR — worth a ticket: the alias contradiction from #1. I also have a two-line follow-up trim to the published JSDoc (dropping "and what that digest can and cannot catch" from the maintainer pointer) that missed the merge. Not worth its own PR on its own; happy to fold it into whoever takes the docs ticket. |
What
Re-syncs
packages/mcp/src/instructions.ts(AUTHORING_INSTRUCTIONS) with the canonical server-side primer, moving the offline fallback from 2.6-era text to the current 2.8 release, and adds a frozen sha-256 so the two copies cannot drift silently again.Why
AUTHORING_INSTRUCTIONSis the copy the MCP server serves when its startup fetch ofGET /v1/mcp/instructionsfails. It is a verbatim duplicate of the server's canonical primer by design — the package must work offline, so it cannot import it — and the only thing enforcing that was a doc comment saying KEEP IT IDENTICAL.It stopped being true two content releases ago. The fallback was missing both App Links releases:
https://apps.sapiom.ai/{org}/{slug}that outlives the sandbox)sapiom_dev_app_publish, version-gated at@sapiom/mcp >= 0.13So the one path with no other source of truth — an offline session, which is precisely the case where the live fetch failed — was told App Links do not exist at all: no durable-sharing paragraph, no local publish tool, no hosted or REST surface. The copy also still carried the pre-2.7 single-alias framing of the hosted capability MCP, which now lives under its own
sapiom-directalias.The drift ran both ways, which the ticket's "2.6-era text" framing (and my first description of this PR) obscured. Diffed against
origin/main, this copy was behind on App Links and the alias framing, level on the LLM call-surface block, and ahead onctx.shared— see Removed below. Blast radius is low either way, since online sessions live-fetch the server text and it wins.What the sync removes
The fallback carried an 11-line
ctx.sharedquota contract — the inclusive 256 KiB / 262,144-byte limit, compact-JSON.stringifymeasurement, setter-time validation, nodelete(), structural guards overinstanceof— that is not in the served primer. Online sessions never saw it; only offline ones did.Syncing removes it here, and I'm keeping that: the two are one canonical text, the ticket's scope is "copy verbatim," and the digest below cannot hold if the copies differ by a paragraph. It is disclosed rather than silent — there's a test named for the removal, and a Removed section in the changeset. The contract still reaches authors via
packages/agent/README.mdand the scaffold-shippedsapiom-agent-authoringskill. Putting it back into the primer is a server-side content release; flagged for the epic rather than reintroducing divergence here.Changes
packages/mcp/src/instructions.ts— body replaced verbatim with the 2.8 primer; byte-identical to the server copy. Doc comment updated to describe the guard rather than restate a rule that had already failed.packages/mcp/src/instructions.test.ts— newis byte-identical to the backend primer (frozen sha-256, SAP-2959)pinssha256(AUTHORING_INSTRUCTIONS)to7f518d9c…4494c0df, the same digest the server-side spec pins for its current primer. Updates the assertions the old text happened to satisfy (the previous hosted-MCP framing), adds App Links coverage, and adds a test recording thectx.sharedremoval above..changeset/— patch release of@sapiom/mcp.The guard, and why the existing tests missed this
Every
containassertion in this file passed against the stale text — they assert what must be present, and nothing noticed what had stopped being copied. So the copy fell two releases behind with a green suite.The digest is what binds the two copies, but only because both ends pin it, which review on the paired server-side PR was right to press on. A pin checked only against its own body is a self-consistency check: it catches an accidental in-place edit here and nothing else. In particular it would not catch the direction that caused this incident — a server-side content release, which touches that repo only.
So the paired PR pins this same digest against the server's current primer. Cutting the next release changes that hash and reddens a spec that names this pin, which is the moment its author discovers this body has to move too.
Stated plainly, in the code as well as here: neither pin can block a merge in the other repository, and an author can still move one side alone. What the pair removes is the silent path — drifting now takes a deliberate edit to a line that says what it is for. Re-pointing this digest alone just re-blesses the drift.
This is the fourth reactive heal of the same class of drift (SAP-1367 → SAP-1467 → SAP-2227 → this); the guard is the point of the ticket, not the copy.
Known-adjacent, deliberately not in this PR
The repo ships three answers on MCP aliases:
docs/mcp-servers.md:7(Remotesapiom/ Localsapiom-dev),README.md:141(claude mcp add sapiom-dev), andpackages/mcp/README.md:35(claude mcp add sapiom, agreeing with the primer). Review flagged it here.It's real, but it isn't this PR's: the alias vocabulary is the canonical primer's, decided server-side in 2.7 and already live to every online session. The contradiction exists on
maintoday and this diff doesn't change it — it only moves the offline copy onto the vocabulary the online path already uses. A partial fix would leavedocs/mcp-servers.mdbuilt end-to-end around the inverse mapping, so the repo would still ship three answers; doing it properly is a docs-page rewrite that belongs with SAP-2923. Flagged for the epic.Testing
vitest run src/instructions.test.ts src/instructions-fetch.test.ts— 14 passed, including the five regression assertions restored after review (tool_use,"it forces a tool","treat missing as unknown","reports both as `null` today",not.toContain("If you must pin")), all verified passing against the new texttsc --noEmit— cleaneslint src --ext .ts— cleanprettier --checkon the touched files — cleanRelated
sapiom_dev_app_publish, feat(mcp,skills): sapiom_dev_app_publish + teach App Links in the preview skill (SAP-2922) #722), SAP-2923 (the 2.7/2.8 server content releases)🤖 Generated with Claude Code
https://claude.ai/code/session_01M2AUF348cx5Mh4N9bbnkSF