From 7e1c3e087abde4cf428a2851102b746b223c1535 Mon Sep 17 00:00:00 2001 From: playforge-coding <251060555+playforge-coding@users.noreply.github.com> Date: Mon, 10 Aug 2026 17:24:04 -0500 Subject: [PATCH 1/2] Let an assistant propose a lesson change instead of making it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An AI assistant connected over MCP could only write a lesson outright, which left two things impossible. It could not touch a lesson somebody else wrote at all — nobody may save over another person's lesson — and it could not offer changes to your own lesson for you to look over first: patch_lesson overwrites it, and there is nothing to review. So give it the route a human already has. fork_lesson clones a lesson into a private draft of its own, the assistant edits that with the ordinary tools, and propose_changes offers the result back as a proposal, to be read and merged (or declined) from the web app. The lesson is untouched until a person decides. list_lesson_proposals is how the assistant finds out what they decided; merging is deliberately not a tool, because it is theirs. Two things had to move to make that possible. The fork-and-propose flow only existed bound to LightningFS, so it ran in a browser and nowhere else. The git engine already takes its filesystem through { fs, gitdir } for exactly this reason, so it needed a filesystem rather than a rewrite: core/git/memfs.js is an in-memory node:fs, which is what lets this run on stdio and inside the Worker alike. No repository is kept between calls — a fork is a real hub lesson with its own stored pack, so each call clones that pack, does one thing to it, and uploads the result. That survives a restart, a conversation resumed days later, and a connection moving between instances. And the Worker refused a proposal from a lesson's own author, on the grounds that they could simply save. Over MCP the assistant acts as the account it is signed in with, so that refusal fell on exactly the case worth having. It now refuses only when there is nothing behind the request: a proposal carrying a fork you own is allowed, because it means something specific — here is a copy with changes in it, let me read the diff before it lands. A human gets the same route via "fork into a new lesson". Since the proposer's name is then your own, the proposal's body records which client wrote it and the notification reads "Changes are waiting for your review" rather than naming somebody. Tested against the real git engine rather than a mock of it: that a proposal's packfile genuinely shares ancestry with the lesson it targets (without which a reviewer's three-way merge has no base and the whole thing degrades to "replace the lesson with mine"), that the target is untouched, that a failed pack upload withdraws its proposal instead of leaving an empty one in someone's queue, and that proposing twice stacks rather than colliding. Co-Authored-By: Claude Opus 5 (1M context) --- apps/api/src/routes/pulls.js | 51 ++- apps/docs/docs/mcp-server/overview.md | 6 + apps/docs/docs/mcp-server/tools.md | 76 +++- apps/docs/docs/monorepo/version-history.md | 10 +- apps/docs/docs/web-app/notifications.md | 5 +- apps/docs/docs/web-app/pull-requests.md | 29 +- apps/mcp/manifest.json | 14 +- apps/mcp/package.json | 2 +- apps/mcp/src/api.js | 233 +++++++++--- apps/mcp/src/git.js | 298 +++++++++++++++ apps/mcp/src/tools.js | 163 +++++++- apps/mcp/test/fork.test.js | 413 +++++++++++++++++++++ apps/mcp/test/smoke.test.js | 3 + packages/core/package.json | 1 + packages/core/src/git/memfs.js | 274 ++++++++++++++ packages/core/src/git/memfs.test.js | 156 ++++++++ 16 files changed, 1645 insertions(+), 89 deletions(-) create mode 100644 apps/mcp/src/git.js create mode 100644 apps/mcp/test/fork.test.js create mode 100644 packages/core/src/git/memfs.js create mode 100644 packages/core/src/git/memfs.test.js diff --git a/apps/api/src/routes/pulls.js b/apps/api/src/routes/pulls.js index f965181..c2e3a35 100644 --- a/apps/api/src/routes/pulls.js +++ b/apps/api/src/routes/pulls.js @@ -300,7 +300,31 @@ async function openPull(request, env, base, lessonId, cors) { if (!(await canReadLesson(env, base, request, lesson))) { return textResponse('Lesson not found.', 404, cors); } - if (lesson.author_id === user.id) { + // The fork this came from, when it is itself a saved lesson. Only recorded if + // it really is the caller's: it becomes a link shown next to their name, and + // nobody should be able to point that at a lesson they don't own. A bad value + // is dropped rather than rejected — the pack is what carries the changes. + // + // Resolved before the own-lesson check below, which turns on it. + let sourceLessonId = null; + const claimed = typeof body.sourceLessonId === 'string' ? body.sourceLessonId.trim() : ''; + if (LESSON_ID_RE.test(claimed) && claimed !== lessonId) { + const source = await fetchLessonRow(env, base, claimed); + if (source && source.author_id === user.id) sourceLessonId = claimed; + } + + // Proposing to your own lesson is refused when there is nothing behind it: you + // can simply save, and a request to yourself out of nowhere is a mistake. + // + // It is allowed when it carries a fork you own, because then it means something + // specific and useful — "here is a copy with changes in it, let me read the diff + // before it lands". That is the shape of an AI assistant's work: over MCP the + // assistant acts as the account it is signed in with, so changes it proposes to + // the user's own lesson arrive from the user's own id (see apps/mcp/src/git.js). + // Holding them in the review queue is the whole point — the lesson is untouched + // until a person reads the diff and merges it. A human gets the same route via + // "fork into a new lesson" in the editor. + if (lesson.author_id === user.id && !sourceLessonId) { return textResponse('This is your own lesson — save your changes to it directly instead.', 400, cors); } @@ -326,17 +350,6 @@ async function openPull(request, env, base, lessonId, cors) { ); } - // The fork this came from, when it is itself a saved lesson. Only recorded if - // it really is the caller's: it becomes a link shown next to their name, and - // nobody should be able to point that at a lesson they don't own. A bad value - // is dropped rather than rejected — the pack is what carries the changes. - let sourceLessonId = null; - const claimed = typeof body.sourceLessonId === 'string' ? body.sourceLessonId.trim() : ''; - if (LESSON_ID_RE.test(claimed) && claimed !== lessonId) { - const source = await fetchLessonRow(env, base, claimed); - if (source && source.author_id === user.id) sourceLessonId = claimed; - } - const insert = { lesson_id: lessonId, source_lesson_id: sourceLessonId, @@ -431,14 +444,22 @@ async function putPullPack(request, env, base, lessonId, pullId, cors) { // Now there is something to look at, tell the lesson's author. Best-effort — // never fail a proposal that has landed over a notification that hasn't. + // + // A proposal from the author's own account is notified too, and is the case + // that needs it most: it is how an AI assistant working over MCP offers changes + // (see openPull above), and the notification is the only thing that tells the + // author there is something waiting. The wording doesn't claim someone else + // wrote it, because the account says otherwise; the proposal's own body records + // what opened it. const lesson = await fetchLessonRow(env, base, lessonId); - if (lesson && lesson.author_id !== user.id) { + if (lesson) { + const own = lesson.author_id === user.id; await createNotification(env, base, { userId: lesson.author_id, type: 'pull_request', - title: `${authorFromUser(user)} proposed changes to your lesson`, + title: own ? 'Changes are waiting for your review' : `${authorFromUser(user)} proposed changes to your lesson`, body: pull.title, - link: `/hub/${lessonId}`, + link: `/hub/${lessonId}/proposals/${pullId}`, }).catch(() => {}); } diff --git a/apps/docs/docs/mcp-server/overview.md b/apps/docs/docs/mcp-server/overview.md index 6415353..e3e6aac 100644 --- a/apps/docs/docs/mcp-server/overview.md +++ b/apps/docs/docs/mcp-server/overview.md @@ -14,6 +14,12 @@ app uses** (`/lessons`), authenticating as you with a Supabase token — so ever lesson goes through the existing validation, ban checks, and author attribution. Nothing here bypasses the normal API. +It can also **fork** a lesson and open a **proposal** against it, rather than +writing to it — the assistant edits a copy, and you read the diff and decide. +That's the only route into a lesson somebody else wrote, and the one to use when +you'd rather check the assistant's work before it goes live. See +[Proposing changes instead of making them](./tools.md#proposing-changes-instead-of-making-them). + Two ways to connect: - **[Remote (hosted) mode](./remote-mode.md)** (recommended) — point your diff --git a/apps/docs/docs/mcp-server/tools.md b/apps/docs/docs/mcp-server/tools.md index c4ea5a1..70569b9 100644 --- a/apps/docs/docs/mcp-server/tools.md +++ b/apps/docs/docs/mcp-server/tools.md @@ -4,19 +4,69 @@ title: Tools # Tools -| Tool | What it does | -| ---------------------- | ----------------------------------------------------------------------------- | -| `whoami` | Confirm the session is valid and show the publishing display name. | -| `create_lesson` | Build and save a new lesson (draft by default; `published: true` to share). | -| `patch_lesson` | Edit a lesson with a small diff (id-addressed ops) instead of a full replace. | -| `update_lesson` | Replace a lesson's whole title/content (author only). | -| `get_lesson` | Fetch one lesson with its full content (read before editing / as a template). | -| `list_my_lessons` | List your own lessons (drafts + published). | -| `list_hub_lessons` | Browse published lessons for inspiration / de-duplication. | -| `set_lesson_published` | Toggle a lesson between public and private draft. | -| `delete_lesson` | Permanently delete one of your lessons. | -| `search_images` | Search Wikimedia Commons for freely-licensed images to illustrate a lesson. | -| `add_image` | Download a searched image and insert it as an image block in a lesson. | +| Tool | What it does | +| ----------------------- | ----------------------------------------------------------------------------- | +| `whoami` | Confirm the session is valid and show the publishing display name. | +| `create_lesson` | Build and save a new lesson (draft by default; `published: true` to share). | +| `patch_lesson` | Edit a lesson with a small diff (id-addressed ops) instead of a full replace. | +| `update_lesson` | Replace a lesson's whole title/content (author only). | +| `fork_lesson` | Copy a lesson into a private draft of your own, keeping its version history. | +| `propose_changes` | Offer a fork's changes back to the original, for a human to review and merge. | +| `list_lesson_proposals` | List the proposals against a lesson, and whether yours have been resolved. | +| `get_lesson` | Fetch one lesson with its full content (read before editing / as a template). | +| `list_my_lessons` | List your own lessons (drafts + published). | +| `list_hub_lessons` | Browse published lessons for inspiration / de-duplication. | +| `set_lesson_published` | Toggle a lesson between public and private draft. | +| `delete_lesson` | Permanently delete one of your lessons. | +| `search_images` | Search Wikimedia Commons for freely-licensed images to illustrate a lesson. | +| `add_image` | Download a searched image and insert it as an image block in a lesson. | + +## Proposing changes instead of making them + +An assistant can change a lesson two ways, and which one it should use is a question +about **who decides**, not about the size of the edit. + +`patch_lesson` writes straight to the lesson. It's right for a correction the user has +asked for outright — a typo, a wrong answer — where a review step is only friction. + +**`fork_lesson` + `propose_changes`** leaves the lesson untouched and puts the changes in +its [Proposals](/web-app/pull-requests) tab instead, where a person reads the diff and +merges or declines it: + +```text +fork_lesson({ lessonId }) -> a private draft fork you own +patch_lesson({ id: fork.id, … }) -> edit THE FORK +propose_changes({ forkLessonId }) -> a proposal, with a URL to review it +``` + +That's the only available route for a lesson somebody else wrote — nobody can save over +another person's lesson — and it's the better route whenever the user wants to look over +the assistant's work before it goes live. `propose_changes` returns the proposal's `url`; +the assistant is expected to hand that over and stop, rather than report the change as +done. + +Some mechanics worth knowing: + +- **A fork is a real clone.** It carries the original's git history, so the reviewer's + merge is a true three-way merge against the commit the two diverged from, block by + block. A lesson with no stored history can still be forked, but the fork shares no + ancestor with it, so the whole document reads as the change. `fork_lesson` says which + happened. +- **A proposal is one commit**, made when it's opened, holding the fork as it then + stands. Intermediate `patch_lesson` calls aren't separate commits, so make all the + edits first. Proposing again after further edits opens a second, separate proposal + (at most 5 open against one lesson). +- **Images aren't copied.** Blocks reference them by content hash and the bytes are + already stored, so forking is cheap. +- **Forks are private drafts** and count against the draft cap, so `delete_lesson` the + fork once its proposal has been resolved. +- **Merging is not an MCP tool.** It happens in the web app, under the reviewer's own + credentials, because it is theirs to decide. `list_lesson_proposals` is how the + assistant finds out what they decided. + +Because the assistant acts as the account it's signed in with, a proposal against your +_own_ lesson is opened by _you_ — so its body carries a note saying an assistant wrote +it, and the notification you get reads "Changes are waiting for your review". ## Editing a lesson: patch vs. replace diff --git a/apps/docs/docs/monorepo/version-history.md b/apps/docs/docs/monorepo/version-history.md index eb6acff..974ccaa 100644 --- a/apps/docs/docs/monorepo/version-history.md +++ b/apps/docs/docs/monorepo/version-history.md @@ -296,6 +296,7 @@ in the browser, in Node and inside the Worker: | `repo` | Commit, history, diff two commits, restore. | | `pack` | Pack for upload; clone/fetch from a pack; merge base; ancestry. | | `remote` | The `/git/:lessonId` Worker calls (incl. the 409 on a stale push). | +| `memfs` | An in-memory filesystem, for the hosts with no other. | `remote` reads the API's base URL through `@spelling-creator/core/config` rather than the bundler's env, which is what lets it sit on this side of the line. @@ -308,6 +309,13 @@ needs a real browser: | `fs` | LightningFS — the IndexedDB filesystem the repos live on. | | `sync` | Fork (clone), merge, push, and both sides of a pull request. | +Server-side (`apps/mcp/src/git.js`) — the fork-and-propose flow for an AI +assistant, which is `browser/git/sync`'s two outbound steps built on `memfs` +instead of LightningFS. It keeps no repository between calls: a fork is a real +hub lesson with its own stored pack, so each call clones that pack, does one +thing to it and uploads the result. See +[Pull requests](/web-app/pull-requests) and [MCP tools](/mcp-server/tools). + App-bound (`apps/web/src/lib/git/`) — what cannot leave the bundle: | File | Purpose | @@ -317,7 +325,7 @@ App-bound (`apps/web/src/lib/git/`) — what cannot leave the bundle: `repo` and friends take their filesystem through `repoCtx` rather than opening one, which is exactly what lets the same commit/merge/restore logic run against -LightningFS in the browser and `node:fs` in tests. +LightningFS in the browser, and `memfs` in Node, in the Worker and in tests. A repo tracks remotes in git's own vocabulary: `origin` (this lesson's own published history, which a trusted collaborator may have moved on without us), diff --git a/apps/docs/docs/web-app/notifications.md b/apps/docs/docs/web-app/notifications.md index 6c43688..efb9db5 100644 --- a/apps/docs/docs/web-app/notifications.md +++ b/apps/docs/docs/web-app/notifications.md @@ -28,9 +28,12 @@ queries the table directly — everything goes through the Worker's profile. See [Following](./profiles-and-display-names.md#following). - **`pull_request`** — someone proposed changes to a lesson you published, or the proposal _you_ made was merged or closed. The three are one type because they're - one conversation; the title says which happened and the link opens the lesson. + one conversation; the title says which happened and the link opens the proposal. Nothing is sent until a proposal actually has changes in it (an upload that never finished notifies nobody), and withdrawing your own never notifies you. + A proposal opened from your own account — which is how an AI assistant working + over MCP offers changes — reads _"Changes are waiting for your review"_, because + it's the only thing telling you there's something in the queue. See [Pull requests](./pull-requests.md). - **`lesson_update`** — a trusted collaborator saved a lesson you published. It changed under you and you didn't do it, so you're told. (Merging a proposal into diff --git a/apps/docs/docs/web-app/pull-requests.md b/apps/docs/docs/web-app/pull-requests.md index 4548b66..9391573 100644 --- a/apps/docs/docs/web-app/pull-requests.md +++ b/apps/docs/docs/web-app/pull-requests.md @@ -79,12 +79,32 @@ author's queue. | -------------------------- | ---- | ----- | -------------------------- | | Anyone signed in | ✅ | ❌ | ❌ | | The person who opened it | ✅ | ❌ | ✅ (withdraw) | -| The lesson's author | ❌\* | ✅ | ✅ (decline) | +| The lesson's author | ✅\* | ✅ | ✅ (decline) | | A **trusted collaborator** | ✅ | ✅ | ✅ | | A moderator/admin | ✅ | ❌ | ✅ (as with any user text) | -\* The author has nothing to propose to themselves — they can just save. The -Worker refuses that case rather than creating a request nobody needs. +\* Only from a fork they own — see below. + +### Proposing to your own lesson + +Out of nowhere, this is a mistake: you can just save, so the Worker refuses a +proposal against your own lesson rather than creating a request nobody needs. + +It's allowed when it **carries a fork you own** (`sourceLessonId` resolves to +another of your lessons), because then it means something specific: _here is a +copy with changes in it, let me read the diff before it lands._ Two things use +that: + +- An **AI assistant over MCP** acts as the account it's signed in with, so + changes it proposes to your lesson arrive from your own id. Holding them in the + review queue is the entire point — the lesson is untouched until you read the + diff and merge it. See [MCP tools](/mcp-server/tools). +- **"Fork into a new lesson"** in the editor gives a human the same route for + work they want to look over before committing to it. + +You can then merge it yourself, since you're the author. The notification you get +reads "Changes are waiting for your review" rather than naming a proposer, because +the account is yours; the proposal's body says what opened it. "Trusted collaborator" is not a new concept: it's the email list the author already manages in the collaboration dialog (`doc.trustedCollaborators`, the same @@ -166,7 +186,7 @@ which is why the submission dialog says so plainly before you send it. | Method & path | Auth | What it does | | ------------------------------------- | ----------------------- | ---------------------------------------------------------------------------------------------- | | `GET /lessons/:id/pulls` | none, unless a draft\* | `{ "pulls": [...], "canReview": bool }` — newest first; unready rows only for their own author | -| `POST /lessons/:id/pulls` | `Bearer ` | Opens a proposal (`{ title, body, head, base, sourceLessonId }`); anyone but the author | +| `POST /lessons/:id/pulls` | `Bearer ` | Opens a proposal (`{ title, body, head, base, sourceLessonId }`); the author only from a fork | | `PUT /lessons/:id/pulls/:prId/pack` | `Bearer ` | Uploads its packfile (`X-Git-Head` must match). The proposer's, once | | `GET /lessons/:id/pulls/:prId/pack` | none, unless a draft\* | The packfile; `X-Git-Head` names its tip | | `POST /lessons/:id/pulls/:prId/merge` | `Bearer ` | Records the merge (`{ mergeCommit }`); author or trusted collaborator only | @@ -188,6 +208,7 @@ frontend can surface `res.text()` directly. | `apps/api/src/lib/lessonGit.js` | R2 key layout (`git/pulls//pack`) and the sweeps that delete it | | `@spelling-creator/core/pulls` | The browser client, and the shared length limits | | `@spelling-creator/core/browser/git/sync` | `submitPullRequest` (propose) and `preparePullMerge` (review) | +| `apps/mcp/src/git.js` | The same two steps for an AI assistant — fork, then propose | | `apps/web/src/components/ProposeChangesDialog.jsx` | The submission form | | `apps/web/src/pages/lesson/LessonProposals.jsx` | The Proposals tab | | `apps/web/src/pages/lesson/LessonProposal.jsx` | One proposal, read-only, with the hand-off into the editor | diff --git a/apps/mcp/manifest.json b/apps/mcp/manifest.json index ae32ef2..f806079 100644 --- a/apps/mcp/manifest.json +++ b/apps/mcp/manifest.json @@ -2,7 +2,7 @@ "manifest_version": "0.3", "name": "spelling-creator-hub", "display_name": "Spelling Creator Hub", - "version": "0.2.0", + "version": "0.3.0", "description": "Author and publish spelling lessons to the Spelling Creator hub from your AI assistant.", "long_description": "Connects your AI assistant to the Spelling Creator hub so it can compose and publish spelling lessons for you. You describe the lesson; the assistant builds it (reading passages with ALL-CAPS spelling words, explicit word lists, and quiz questions) and saves it to the hub through the same API the web app uses — with all the usual validation and author attribution. Lessons default to private drafts; publish them when you're ready. Requires a Supabase refresh token (run the bundled `login` helper, or copy one from the web app) and a display name set on your account.", "author": { @@ -72,6 +72,18 @@ "name": "patch_lesson", "description": "Edit a lesson with a small list of id-addressed operations." }, + { + "name": "fork_lesson", + "description": "Copy a lesson into a private draft of your own, keeping its version history." + }, + { + "name": "propose_changes", + "description": "Offer a fork's changes back to the original lesson, for a human to review and merge." + }, + { + "name": "list_lesson_proposals", + "description": "List the proposals against a lesson, and whether yours have been merged." + }, { "name": "get_lesson", "description": "Fetch one lesson including its full content document." diff --git a/apps/mcp/package.json b/apps/mcp/package.json index 671243c..2011206 100644 --- a/apps/mcp/package.json +++ b/apps/mcp/package.json @@ -1,6 +1,6 @@ { "name": "@spelling-creator/mcp", - "version": "0.2.0", + "version": "0.3.0", "private": true, "description": "MCP server for the Spelling Creator hub — lets any MCP-capable AI assistant author and publish spelling lessons.", "license": "AGPL-3.0-only", diff --git a/apps/mcp/src/api.js b/apps/mcp/src/api.js index 672cd77..eab8d58 100644 --- a/apps/mcp/src/api.js +++ b/apps/mcp/src/api.js @@ -15,22 +15,32 @@ import { sha256Hex, extFromMime } from "./images.js"; */ export function createApi(config, auth) { const lessonsUrl = (path = "") => `${config.apiUrl}/lessons${path}`; + const gitUrl = (lessonId, path) => + `${config.apiUrl}/git/${encodeURIComponent(lessonId)}${path}`; + const pullsUrl = (lessonId, path = "") => + `${lessonsUrl(`/${encodeURIComponent(lessonId)}`)}/pulls${path}`; - // Fetch a Worker endpoint with a Bearer token, refreshing + retrying once if - // the token is rejected. `needsAuth: false` is for the public reads. - async function call(url, { method = "GET", body, needsAuth = true } = {}) { + /** + * One request to the Worker with a Bearer token, refreshed and retried once if + * the token is rejected — so a long-lived server survives its access token + * expiring between calls. Returns the raw Response; the callers below decide + * what a non-ok status means, because for some of them a 404 is an answer + * ("this lesson has no history") rather than a failure. + * + * `body` is passed through untouched, so this carries JSON and packfile bytes + * alike; JSON callers go through `call` below, which serialises for them. + */ + async function request( + url, + { method = "GET", headers, body, needsAuth = true } = {}, + ) { const send = async (token) => { - const headers = {}; - if (body !== undefined) headers["Content-Type"] = "application/json"; - if (token) headers.Authorization = `Bearer ${token}`; - return fetch(url, { - method, - headers, - body: body === undefined ? undefined : JSON.stringify(body), - }); + const sent = { ...headers }; + if (token) sent.Authorization = `Bearer ${token}`; + return fetch(url, { method, headers: sent, body }); }; - let token = needsAuth ? await auth.getAccessToken() : ""; + const token = needsAuth ? await auth.getAccessToken() : ""; let res; try { res = await send(token); @@ -50,16 +60,65 @@ export function createApi(config, auth) { } } } + return res; + } - if (!res.ok) { - // The Worker returns a short plain-text reason for 4xx/5xx; surface it. - const detail = await res.text().catch(() => ""); - throw new Error(detail || `Request failed (${res.status}).`); - } + /** The Worker returns a short plain-text reason for 4xx/5xx; surface it. */ + async function readError(res, fallback) { + const detail = await res.text().catch(() => ""); + return new Error(detail || fallback || `Request failed (${res.status}).`); + } + + // A JSON call: serialise the body, throw on a bad status, return the parsed + // response. `needsAuth: false` is for the public reads. + async function call(url, { method = "GET", body, needsAuth = true } = {}) { + const res = await request(url, { + method, + needsAuth, + headers: body === undefined ? {} : { "Content-Type": "application/json" }, + body: body === undefined ? undefined : JSON.stringify(body), + }); + if (!res.ok) throw await readError(res); // DELETE returns a tiny JSON; everything else returns JSON too. return res.json().catch(() => ({})); } + /** + * Upload a packfile — a lesson's history, or a proposal's snapshot of one. The + * tip travels in X-Git-Head so the bytes and the ref they belong to can never + * be paired from two different moments (see core/git/remote.js). + */ + async function putPack(url, { packfile, head, parent }, badStatusMessage) { + const headers = { + "Content-Type": "application/x-git-packfile", + "X-Git-Head": head, + }; + // The compare-and-swap: the head we believe the lesson points at. Omitted + // when it has no history yet. + if (parent) headers["X-Git-Parent"] = parent; + + const res = await request(url, { method: "PUT", headers, body: packfile }); + if (!res.ok) throw await readError(res, badStatusMessage); + return res.json().catch(() => ({})); + } + + /** + * Download a packfile, with its tip from the same response. Returns null when + * there is no history stored (404) rather than throwing: for a lesson that + * predates version history, "no repo" is a normal answer a fork has to handle. + */ + async function getPack(url) { + const res = await request(url); + if (res.status === 404) return null; + if (!res.ok) throw await readError(res, "Could not download the history."); + + const head = res.headers.get("X-Git-Head"); + if (!head) return null; // a pack with no tip is unusable + const packfile = new Uint8Array(await res.arrayBuffer()); + if (packfile.byteLength === 0) return null; + return { packfile, head }; + } + return { /** Verify the session and return the Supabase user (id, email, display name). */ async whoami() { @@ -113,11 +172,21 @@ export function createApi(config, auth) { return data.lesson; }, - /** Create a lesson. `doc` is the canonical editor document. */ - async createLesson({ title, doc, published }) { + /** + * Create a lesson. `doc` is the canonical editor document. `forkedFrom` is + * the lesson this one was forked from, which the hub records as the fork's + * pointer home — it's what lets the fork later pull the original's changes + * in, and what a proposal links back to. + */ + async createLesson({ title, doc, published, forkedFrom }) { const data = await call(lessonsUrl(), { method: "POST", - body: { title, doc, published }, + body: { + title, + doc, + published, + ...(forkedFrom ? { forkedFrom } : {}), + }, }); return data.lesson || {}; }, @@ -141,6 +210,92 @@ export function createApi(config, auth) { return { ok: true }; }, + // ---- Version history and proposals ------------------------------------- + // + // A lesson's history is a git repository, and it travels as a packfile (see + // packages/core/src/git/). These four are what forking and proposing need: + // read a lesson's history, write a fork's, and open a proposal carrying it. + // Unlike the browser's equivalents (core/git/remote.js, core/pulls.js) these + // always send the caller's token — a fork starts life as a private draft, so + // its own history is not a public read. + + /** + * Download a lesson's packed history, or null when it has none (an older + * lesson from before version history, or one never pushed). + */ + async fetchLessonPack(lessonId) { + return getPack(gitUrl(lessonId, "/pack")); + }, + + /** The tip of a lesson's published history, or null when it has none. */ + async fetchLessonHead(lessonId) { + const res = await request(gitUrl(lessonId, "/refs")); + if (!res.ok) return null; + const data = await res.json().catch(() => null); + return data?.head || null; + }, + + /** + * Upload a lesson's history. `parent` is the head we believe it points at — + * the Worker refuses the push if the lesson has moved on since, which is + * what stops two writers erasing each other. + */ + async pushLessonPack(lessonId, { packfile, head, parent }) { + return putPack( + gitUrl(lessonId, "/pack"), + { packfile, head, parent }, + "Could not save the lesson history.", + ); + }, + + /** The proposals open against a lesson, and whether we may review them. */ + async listPulls(lessonId) { + const data = await call(pullsUrl(lessonId)); + return { + pulls: Array.isArray(data.pulls) ? data.pulls : [], + canReview: Boolean(data.canReview), + }; + }, + + /** + * Open a proposal against a lesson. This creates the request but not its + * contents — the changes follow as a packfile (uploadPullPack), and until + * that lands the request is unready and nobody but its author sees it. + */ + async createPull(lessonId, { title, body, head, base, sourceLessonId }) { + const data = await call(pullsUrl(lessonId), { + method: "POST", + body: { + title, + body, + head, + ...(base ? { base } : {}), + ...(sourceLessonId ? { sourceLessonId } : {}), + }, + }); + if (!data.pull) throw new Error("Could not open the proposal."); + return data.pull; + }, + + /** Upload a proposal's packfile — the changes themselves. */ + async uploadPullPack(lessonId, pullId, { packfile, head }) { + const data = await putPack( + pullsUrl(lessonId, `/${encodeURIComponent(pullId)}/pack`), + { packfile, head }, + "Could not upload the proposed changes.", + ); + return data.pull || null; + }, + + /** Withdraw a proposal (used to clean up one whose pack never landed). */ + async closePull(lessonId, pullId) { + const data = await call( + pullsUrl(lessonId, `/${encodeURIComponent(pullId)}/close`), + { method: "POST" }, + ); + return data.pull || null; + }, + /** * Upload raw image bytes to R2 by their content hash and return the image * ref to put on an image block ({ hash, mime, ext }). PUT /images/:hash is @@ -152,39 +307,13 @@ export function createApi(config, auth) { */ async uploadImage(bytes, mime) { const hash = await sha256Hex(bytes); - const url = `${config.apiUrl}/images/${hash}`; - const send = (token) => - fetch(url, { - method: "PUT", - headers: { - "Content-Type": mime || "application/octet-stream", - Authorization: `Bearer ${token}`, - }, - body: bytes, - }); - - let token = await auth.getAccessToken(); - let res; - try { - res = await send(token); - } catch { - throw new Error(`Could not reach the lesson hub at ${config.apiUrl}.`); - } - if (res.status === 401) { - const refreshed = await auth.forceRefresh(); - if (refreshed) { - try { - res = await send(refreshed); - } catch { - throw new Error( - `Could not reach the lesson hub at ${config.apiUrl}.`, - ); - } - } - } + const res = await request(`${config.apiUrl}/images/${hash}`, { + method: "PUT", + headers: { "Content-Type": mime || "application/octet-stream" }, + body: bytes, + }); if (!res.ok) { - const detail = await res.text().catch(() => ""); - throw new Error(detail || `Image upload failed (${res.status}).`); + throw await readError(res, `Image upload failed (${res.status}).`); } return { hash, diff --git a/apps/mcp/src/git.js b/apps/mcp/src/git.js new file mode 100644 index 0000000..a902af3 --- /dev/null +++ b/apps/mcp/src/git.js @@ -0,0 +1,298 @@ +// Forking a lesson and proposing changes back, for an AI assistant. +// +// This is the assistant's version of what the editor does in the browser (see +// packages/core/src/browser/git/sync.js, which is the same flow bound to +// LightningFS). The rule it exists to respect is the hub's, not ours: nobody +// writes a lesson from a fork. Work travels back through a proposal, which a +// human reads and merges. So an assistant asked to change a lesson does not save +// over it — it forks, edits its own copy, and opens a proposal. +// +// ---- Why there is no state between calls ------------------------------------ +// +// A repository here lives in memory (core/git/memfs.js) and is thrown away when +// the tool call returns. It doesn't need to survive, because the fork is a real +// hub lesson with its own stored history: the durable state is the fork's row +// (its document) and its packfile in R2. Each call rebuilds exactly what it +// needs by cloning that pack, which means a fork survives the server restarting, +// a conversation being resumed days later, and the remote transport moving a +// connection between Worker instances. +// +// It also means the assistant edits its fork with the ordinary tools — +// patch_lesson, add_image, update_lesson — and only pays for git at the two +// moments that need it: forking, and proposing. +// +// ---- What a proposal contains ----------------------------------------------- +// +// One commit, made when the proposal is opened, holding the fork's document as +// it then stands. The intermediate patches aren't separate commits — nothing was +// watching to record them — so the reviewer sees a single change against the +// commit the fork and the lesson diverged from. That diff is the thing being +// reviewed, and it's exact; the fork's own history is what makes it a true +// three-way merge rather than a guess. + +import { stripLocalFields } from "@spelling-creator/core/git/doc"; +import { memRepo } from "@spelling-creator/core/git/memfs"; +import { describeOp } from "@spelling-creator/core/git/ops"; +import { + cloneFromPack, + fetchRemotePack, + packRepo, +} from "@spelling-creator/core/git/pack"; +import { + UPSTREAM_REF, + authorFrom, + commitDoc, + pendingOps, +} from "@spelling-creator/core/git/repo"; +import { PULL_BODY_MAX, PULL_TITLE_MAX } from "@spelling-creator/core/pulls"; + +/** + * The signature to stamp the assistant's commits with. + * + * The hub attributes everything to the account whose token this is — the + * assistant acts as the signed-in user, and there is no separate identity to + * claim. So the commit carries that user's name, and `proposalBody` below is + * where the fact that an assistant wrote it is recorded. + */ +async function commitAuthor(api) { + const me = await api.whoami().catch(() => null); + return authorFrom({ name: me?.displayName, email: me?.email }); +} + +/** Trim to a limit on a word boundary where there is one nearby. */ +function clamp(value, limit) { + const text = (value || "").trim(); + if (text.length <= limit) return text; + const cut = text.slice(0, limit - 1); + const space = cut.lastIndexOf(" "); + return `${(space > limit * 0.8 ? cut.slice(0, space) : cut).trimEnd()}…`; +} + +/** + * The proposal's body, with a note saying which assistant wrote it. + * + * Worth the line: the hub records the proposal against the account it was opened + * with, so on a self-proposal the reviewer would otherwise see their own name + * against changes they didn't write. `client` is the MCP client's own reported + * name (Claude Desktop, claude.ai, Cursor, …), which is the closest thing to an + * honest answer available — we know what connected, not what model it drove. + */ +export function proposalBody(body, client) { + const note = client + ? `Proposed by an AI assistant via ${client} (Spelling Creator MCP).` + : "Proposed by an AI assistant via the Spelling Creator MCP server."; + const text = (body || "").trim(); + if (!text) return note; + // Keep the whole note: it's the provenance, and it's what tells a reviewer to + // read the diff rather than assume they wrote it. + return `${clamp(text, PULL_BODY_MAX - note.length - 2)}\n\n${note}`; +} + +/** + * Build an in-memory repository holding a lesson's history, cloned from its + * stored pack. The clone carries the original's commit oids, so the fork shares + * ancestry with it — which is what a reviewer's three-way merge needs. + */ +async function cloneRepo(pack) { + const ctx = memRepo(); + await cloneFromPack({ ...ctx, ...pack }); + return ctx; +} + +/** + * Fork a lesson into a new private draft owned by the caller. + * + * The fork is a genuine clone wherever it can be: its repository is the source + * lesson's, downloaded and re-uploaded under the new id, with the original's tip + * recorded at refs/remotes/upstream/main so the fork knows where it came from. + * + * A lesson with no stored history (one written before version history, or only + * ever written over MCP) can't be cloned, so the fork's history is seeded from + * its document instead. That fork still works — it just shares no commit with + * the original, so a later merge compares two sides rather than three. The + * result says which happened, because it changes what a reviewer will see. + * + * @returns {Promise<{ lesson: object, head: string, clonedHistory: boolean }>} + */ +export async function forkLesson(api, { lessonId, title }) { + const source = await api.getLesson(lessonId); + if (!source.doc?.sections?.length) { + throw new Error("That lesson has no content to fork."); + } + + const pack = await api.fetchLessonPack(lessonId); + + // The fork's document. Local-only fields never travel (see core/git/doc.js): + // the trusted-collaborator list belongs to the lesson it was named on, not to + // a copy of it, and it must not be carried into a new lesson's document. + const doc = stripLocalFields(source.doc); + const forkTitle = (title || "").trim() || source.title; + + // Create the row first: the history is pushed under the new lesson's id, so + // there has to be a new lesson to push it to. `forkedFrom` is the pointer home. + const lesson = await api.createLesson({ + title: forkTitle, + doc: { ...doc, title: forkTitle }, + published: false, + forkedFrom: lessonId, + }); + + const author = await commitAuthor(api); + let ctx; + if (pack) { + ctx = await cloneRepo(pack); + // Record where we came from, so a later sync has a base before it fetches + // anything new. + await fetchRemotePack({ ...ctx, ...pack, ref: UPSTREAM_REF }); + + // The row's document and the history's tip can disagree — a lesson edited + // over MCP is saved without committing, so its stored pack lags. Commit the + // difference now, under the fork, so the fork is self-consistent from the + // start and the proposal's diff later shows only what the assistant changed. + // A no-op when they already agree, which is the normal case. + await commitDoc({ + ...ctx, + doc: { ...doc, title: forkTitle }, + author, + message: `Fork "${source.title}"\n\nBrings the fork up to the lesson's saved document.\n`, + }); + } else { + ctx = memRepo(); + await commitDoc({ + ...ctx, + doc: { ...doc, title: forkTitle }, + author, + message: `Fork "${source.title}"\n\nThe original has no stored history, so this fork starts from its current document.\n`, + }); + } + + const packed = await packRepo(ctx); + try { + // A brand-new lesson has no history, so there is nothing to compare and swap + // against. + await api.pushLessonPack(lesson.id, { + packfile: packed.packfile, + head: packed.head, + parent: null, + }); + } catch (err) { + // The row exists but has no history behind it, which is the one state the + // rest of this file can't work from. Say so rather than leaving the assistant + // to rediscover it at propose time — and don't delete the lesson, which is + // a real copy of the document and the user's to keep or remove. + throw new Error( + `The fork was created (${lesson.id}) but its history could not be stored, so changes to it can't be ` + + `proposed yet. Delete it and fork again. (${err.message})`, + ); + } + + return { lesson, head: packed.head, clonedHistory: Boolean(pack) }; +} + +/** + * Offer a fork's work back to the lesson it came from, as a proposal. + * + * Nothing is written to that lesson — not its document, not its history. What + * travels is a snapshot of the fork's repository, which its author (or a trusted + * collaborator) reviews and merges from the web app. Snapshotting is what makes + * the request stable: the fork can carry on changing and what the reviewer read + * won't move under them. + * + * Opening it is two steps — the request, then its pack — because the pack is + * uploaded against the request's id. If the upload fails the empty request is + * withdrawn, rather than left in a review queue with nothing in it. + * + * @returns {Promise<{ pull: object, lessonId: string, commit: string, ops: object[] }>} + */ +export async function proposeChanges( + api, + { forkLessonId, lessonId, title, body, client }, +) { + const fork = await api.getLesson(forkLessonId); + const target = (lessonId || fork.forkedFrom || "").trim(); + if (!target) { + throw new Error( + `Lesson ${forkLessonId} is not a fork of anything, so there is nobody to propose to. ` + + "Pass lessonId to say which lesson the changes are for, or fork_lesson first.", + ); + } + if (target === forkLessonId) { + throw new Error("A lesson cannot propose changes to itself."); + } + if (!fork.doc?.sections?.length) { + throw new Error("That fork has no content to propose."); + } + + const forkPack = await api.fetchLessonPack(forkLessonId); + if (!forkPack) { + throw new Error( + `Lesson ${forkLessonId} has no stored history, so there is no fork to propose from. ` + + "Create the fork with fork_lesson, edit that, then propose.", + ); + } + + // Commit the fork's document as it now stands. This is the change being + // proposed: everything the assistant did to the fork since it was created, + // as one commit against the shared history. + const ctx = await cloneRepo(forkPack); + const doc = stripLocalFields(fork.doc); + + // Read the operations before committing so the commit message can itemise them + // — the proposal's title, then a line per change, which is what the reviewer's + // history view renders. + const ops = await pendingOps({ ...ctx, doc }); + if (!ops.length) { + throw new Error( + "This fork is identical to what has already been proposed — there is nothing to propose. " + + "Edit the fork first (patch_lesson on the fork's id), then try again.", + ); + } + + const author = await commitAuthor(api); + const commit = await commitDoc({ + ...ctx, + doc, + author, + message: `${clamp(title, PULL_TITLE_MAX)}\n\n${ops.map(describeOp).join("\n")}\n`, + }); + + const packed = await packRepo(ctx); + + // Push the fork's own history before proposing, so the fork's History tab and + // the proposal agree, and so re-proposing later builds on this commit rather + // than re-making it. + await api.pushLessonPack(forkLessonId, { + packfile: packed.packfile, + head: packed.head, + parent: forkPack.head, + }); + + // The target's tip as it stands, recorded on the request so a reviewer can see + // what it was built against. Informational: the merge finds its own base from + // the shared ancestry. + const base = await api.fetchLessonHead(target).catch(() => null); + + const pull = await api.createPull(target, { + title: clamp(title, PULL_TITLE_MAX), + body: proposalBody(body, client), + head: packed.head, + base, + sourceLessonId: forkLessonId, + }); + + try { + const ready = await api.uploadPullPack(target, pull.id, { + packfile: packed.packfile, + head: packed.head, + }); + return { + pull: ready || pull, + lessonId: target, + commit: commit.oid, + changes: ops.map(describeOp), + }; + } catch (err) { + await api.closePull(target, pull.id).catch(() => {}); + throw err; + } +} diff --git a/apps/mcp/src/tools.js b/apps/mcp/src/tools.js index 7862b65..be988d4 100644 --- a/apps/mcp/src/tools.js +++ b/apps/mcp/src/tools.js @@ -12,6 +12,7 @@ import { z } from "zod"; import { buildDoc, buildLessonFile, QUESTION_TYPES } from "./doc.js"; +import { forkLesson, proposeChanges } from "./git.js"; import { applyPatch, findBlock } from "./patch.js"; import { searchWikimediaImages, resolveWikimediaImage } from "./wikimedia.js"; import { LESSON_STANDARDS } from "./standards.js"; @@ -302,6 +303,20 @@ function checkStandard({ export function registerTools(server, ctx) { const { api, config } = ctx; const hubUrl = (id) => `${config.apiUrl}/hub/${id}`; + const proposalUrl = (lessonId, pullId) => + `${hubUrl(lessonId)}/proposals/${pullId}`; + + // Which MCP client is connected, by its own account of itself — recorded on a + // proposal so a reviewer can see the changes came from an assistant rather + // than from them (see proposalBody in git.js). Only known once the client has + // initialised, and not every client sends it, so this is best-effort. + const clientName = () => { + try { + return server.server.getClientVersion()?.name || ""; + } catch { + return ""; + } + }; // Wrap a handler so thrown errors become a clean isError result the assistant // can read and recover from, rather than a transport-level failure. @@ -541,6 +556,152 @@ export function registerTools(server, ctx) { }), ); + server.registerTool( + "fork_lesson", + { + title: "Fork a lesson", + description: + "Copy a lesson into a new private draft of your own, keeping its version history and a link back to the " + + "original. This is the first step of the review flow:\n\n" + + " 1. fork_lesson(lessonId) -> a draft fork you own\n" + + " 2. patch_lesson(fork.id, ...) -> edit THE FORK, not the original\n" + + " 3. propose_changes(...) -> open a proposal for a human to read and merge\n\n" + + "USE THIS INSTEAD OF EDITING DIRECTLY when either applies:\n" + + "• The lesson was written by someone else. You cannot save over it at all — a proposal is the only route.\n" + + "• The user wants to look over your changes before they go live. Editing their lesson with patch_lesson " + + " overwrites it immediately and there is nothing to review; forking leaves the lesson untouched until " + + " they merge, and they can decline.\n\n" + + "Prefer editing directly (patch_lesson) for a small correction to the user's own lesson that they have " + + "asked for outright — a typo, a wrong answer — where a review step is just friction.\n\n" + + "Forks count against your private-draft limit; delete_lesson the fork once its proposal is merged or " + + "declined. Images are shared with the original rather than copied, so forking is cheap.", + inputSchema: { + lessonId: z.string().describe("The id of the lesson to fork."), + title: z + .string() + .optional() + .describe( + "Title for the fork. Defaults to the original's — usually right, since a proposal is a change to " + + "that lesson rather than a new one.", + ), + }, + }, + tool(async ({ lessonId, title }) => { + const { lesson, head, clonedHistory } = await forkLesson(api, { + lessonId, + title, + }); + return text({ + ...lesson, + url: hubUrl(lesson.id), + head, + note: + `Forked into a private draft (${lesson.id}). Edit THIS id, not ${lessonId}, then call propose_changes ` + + `with forkLessonId: "${lesson.id}".` + + (clonedHistory + ? "" + : " The original has no stored version history, so this fork shares no common ancestor with it — a " + + "reviewer will see the whole document as the change rather than a tidy diff."), + }); + }), + ); + + server.registerTool( + "propose_changes", + { + title: "Propose a fork's changes", + description: + "Offer the changes you made to a fork back to the lesson it came from, as a proposal a human reviews. " + + "Call this after fork_lesson and after editing the fork.\n\n" + + "Nothing is written to the target lesson: the proposal is a snapshot of your fork, and the lesson's author " + + "(or a trusted collaborator) merges it from the web app, block by block, or declines it. Tell the user the " + + "returned `url` — that is the page where they read the diff and decide. Their answer is theirs to give: " + + "don't tell them it is done, and don't try to merge it yourself.\n\n" + + "The proposal carries ONE commit holding the fork as it now stands, so make all your edits before calling " + + "this. You may propose again after further edits; each proposal is a separate request (at most 5 open " + + "against one lesson).\n\n" + + "Write the title and body for the reviewer, not for the log: say what changed and why it is an improvement, " + + "so someone who has not read the diff can judge it.", + inputSchema: { + forkLessonId: z + .string() + .describe("The id of your fork — the lesson holding the changes."), + lessonId: z + .string() + .optional() + .describe( + "The lesson to propose to. Defaults to the one the fork was forked from, which is nearly always right.", + ), + title: z + .string() + .describe( + "One line naming the change, e.g. 'Fix three ungrounded answers in section 4'.", + ), + body: z + .string() + .optional() + .describe( + "The case for the change, in plain text: what you altered, and why. A note recording that an AI " + + "assistant wrote it is appended automatically.", + ), + }, + }, + tool(async ({ forkLessonId, lessonId, title, body }) => { + const { + pull, + lessonId: target, + commit, + changes, + } = await proposeChanges(api, { + forkLessonId, + lessonId, + title, + body, + client: clientName(), + }); + return text({ + proposalId: pull.id, + lessonId: target, + forkLessonId, + title: pull.title, + status: pull.status, + ready: pull.ready, + commit, + changes, + url: proposalUrl(target, pull.id), + note: + "Proposal opened. Nothing has changed in the lesson itself — give the user the `url` so they can read " + + "the diff and merge or decline it. Poll list_lesson_proposals if you need to know what they decided.", + }); + }), + ); + + server.registerTool( + "list_lesson_proposals", + { + title: "List a lesson's proposals", + description: + "List the proposals against a lesson, newest first — use this to check whether one you opened has been " + + "merged, declined (status 'closed'), or is still waiting. `canReview` says whether you may merge them " + + "yourself; merging is done in the web app, not over MCP, because it is the human's decision.", + inputSchema: { + lessonId: z + .string() + .describe("The lesson whose proposals you want to see."), + }, + }, + tool(async ({ lessonId }) => { + const { pulls, canReview } = await api.listPulls(lessonId); + return text({ + canReview, + proposals: pulls.map((pull) => ({ + ...pull, + url: proposalUrl(lessonId, pull.id), + })), + }); + }), + ); + server.registerTool( "get_lesson", { @@ -851,5 +1012,5 @@ export function registerTools(server, ctx) { // The server's identifying metadata, shared by both transports. export const SERVER_INFO = { name: "spelling-creator-hub", - version: "0.1.3", + version: "0.2.0", }; diff --git a/apps/mcp/test/fork.test.js b/apps/mcp/test/fork.test.js new file mode 100644 index 0000000..bbbb85b --- /dev/null +++ b/apps/mcp/test/fork.test.js @@ -0,0 +1,413 @@ +// Forking a lesson and proposing changes back (src/git.js), against a fake hub. +// +// The point of these is the git, not the HTTP: the fake hub stores packfiles the +// way the Worker's R2 bucket does (bytes plus a head, with the same +// compare-and-swap on write), so what's under test is whether the repository the +// assistant builds is a real clone — whether the proposal's pack shares ancestry +// with the lesson it targets. Without that a reviewer's three-way merge has no +// base and the whole flow degrades to "replace the lesson with mine". +// +// The in-memory filesystem those repositories are built on is tested in +// packages/core/src/git/memfs.test.js. + +import assert from "node:assert/strict"; +import test from "node:test"; + +import { memRepo } from "@spelling-creator/core/git/memfs"; +import { + cloneFromPack, + contains, + mergeBase, +} from "@spelling-creator/core/git/pack"; +import { commitDoc, headOid, readDocAt } from "@spelling-creator/core/git/repo"; + +import { forkLesson, proposalBody, proposeChanges } from "../src/git.js"; + +const AUTHOR = { name: "Teacher", email: "teacher@example.com" }; + +function lessonDoc(title, text) { + return { + title, + sections: [ + { + id: "s1", + name: "Reading", + blocks: [ + { id: "b1", type: "text", text }, + { id: "b2", type: "spelling", words: ["BECAUSE", "FRIEND"] }, + ], + }, + ], + }; +} + +/** + * A stand-in for the hub: lesson rows, packfiles keyed by lesson id, and + * proposals. Records every call so a test can assert on what was sent, and + * enforces the two rules the real Worker enforces — the pack compare-and-swap, + * and a proposal's pack matching the head it was opened with. + */ +function fakeHub() { + const lessons = new Map(); + const packs = new Map(); + const pullPacks = new Map(); + const pulls = []; + let nextId = 1; + + const clone = (value) => JSON.parse(JSON.stringify(value)); + + const api = { + async whoami() { + return { id: "u1", email: AUTHOR.email, displayName: AUTHOR.name }; + }, + + async getLesson(id) { + const lesson = lessons.get(id); + if (!lesson) throw new Error("Lesson not found."); + return clone(lesson); + }, + + async createLesson({ title, doc, published, forkedFrom }) { + const id = `lesson-${nextId++}`; + const lesson = { + id, + title, + doc: clone(doc), + published: Boolean(published), + forkedFrom: forkedFrom || null, + }; + lessons.set(id, lesson); + // The real POST /lessons returns the row without its document. + const { doc: _omitted, ...row } = lesson; + return row; + }, + + async fetchLessonPack(id) { + const pack = packs.get(id); + return pack ? { packfile: pack.packfile, head: pack.head } : null; + }, + + async fetchLessonHead(id) { + return packs.get(id)?.head || null; + }, + + async pushLessonPack(id, { packfile, head, parent }) { + const current = packs.get(id); + // The Worker's compare-and-swap: refuse a push built on a head that is no + // longer the lesson's, because accepting it would drop someone's commits. + if ((current?.head || null) !== (parent || null)) { + throw new Error( + "This lesson’s history has moved on since you last synced.", + ); + } + packs.set(id, { packfile, head }); + return { head }; + }, + + async createPull(lessonId, fields) { + const pull = { + id: `pull-${nextId++}`, + lessonId, + status: "open", + ready: false, + ...fields, + }; + pulls.push(pull); + return clone(pull); + }, + + async uploadPullPack(lessonId, pullId, { packfile, head }) { + const pull = pulls.find((p) => p.id === pullId); + if (!pull) throw new Error("Proposal not found."); + assert.equal(head, pull.head, "the pack must match the head opened with"); + pullPacks.set(pullId, { packfile, head }); + pull.ready = true; + return clone(pull); + }, + + async closePull(lessonId, pullId) { + const pull = pulls.find((p) => p.id === pullId); + if (pull) pull.status = "closed"; + pullPacks.delete(pullId); + return pull ? clone(pull) : null; + }, + }; + + return { api, lessons, packs, pullPacks, pulls }; +} + +/** Seed a lesson that has a real published history, as the editor leaves it. */ +async function seedLesson(hub, { id = "original", title, text }) { + const doc = lessonDoc(title, text); + hub.lessons.set(id, { + id, + title, + doc, + published: true, + forkedFrom: null, + }); + + const ctx = memRepo("seed"); + const first = await commitDoc({ ...ctx, doc, author: AUTHOR }); + const { packRepo } = await import("@spelling-creator/core/git/pack"); + const packed = await packRepo(ctx); + hub.packs.set(id, { packfile: packed.packfile, head: packed.head }); + return { id, doc, head: first.oid }; +} + +test("forking clones the lesson's history under a new private draft", async () => { + const hub = fakeHub(); + const source = await seedLesson(hub, { + title: "Volcanoes", + text: "A volcano ERUPTS.", + }); + + const { lesson, head, clonedHistory } = await forkLesson(hub.api, { + lessonId: source.id, + }); + + assert.equal(clonedHistory, true); + assert.equal(lesson.published, false, "a fork starts as a private draft"); + assert.equal(lesson.forkedFrom, source.id, "a fork keeps its pointer home"); + assert.equal( + head, + source.head, + "an unedited fork sits on the original's own commit, so nothing was rewritten", + ); + + // The fork's history is stored under its own id, and is a genuine clone. + const stored = hub.packs.get(lesson.id); + assert.ok(stored, "the fork's history was pushed"); + const ctx = memRepo("check"); + await cloneFromPack({ ...ctx, ...stored }); + assert.equal(await headOid(ctx), source.head); + assert.deepEqual( + (await readDocAt({ ...ctx, oid: source.head })).sections[0].blocks[0].text, + "A volcano ERUPTS.", + ); +}); + +test("forking a lesson with no stored history seeds one from its document", async () => { + const hub = fakeHub(); + hub.lessons.set("plain", { + id: "plain", + title: "Rivers", + doc: lessonDoc("Rivers", "A river FLOWS."), + published: true, + forkedFrom: null, + }); + + const { lesson, head, clonedHistory } = await forkLesson(hub.api, { + lessonId: "plain", + }); + + assert.equal(clonedHistory, false, "there was nothing to clone"); + assert.ok(head, "the fork still has a history of its own"); + assert.ok(hub.packs.get(lesson.id)); +}); + +test("a fork's title is renamed in both its document and its history", async () => { + const hub = fakeHub(); + const source = await seedLesson(hub, { + title: "Volcanoes", + text: "A volcano ERUPTS.", + }); + + const { lesson, head } = await forkLesson(hub.api, { + lessonId: source.id, + title: "Volcanoes (revised)", + }); + + assert.notEqual(head, source.head, "renaming is a commit of its own"); + assert.equal(hub.lessons.get(lesson.id).doc.title, "Volcanoes (revised)"); + + const ctx = memRepo("check"); + await cloneFromPack({ ...ctx, ...hub.packs.get(lesson.id) }); + assert.equal( + (await readDocAt({ ...ctx, oid: head })).title, + "Volcanoes (revised)", + ); + // Still a descendant of the original, so it can still be merged back. + assert.equal( + await contains({ ...ctx, oid: head, ancestor: source.head }), + true, + ); +}); + +test("proposing sends a pack that shares ancestry with the target lesson", async () => { + const hub = fakeHub(); + const source = await seedLesson(hub, { + title: "Volcanoes", + text: "A volcano ERUPTS.", + }); + const { lesson: fork } = await forkLesson(hub.api, { lessonId: source.id }); + + // The assistant edits its fork — the ordinary patch_lesson path, which saves + // the document without committing anything. + hub.lessons.get(fork.id).doc.sections[0].blocks[0].text = + "A volcano ERUPTS when MAGMA reaches the surface."; + + const result = await proposeChanges(hub.api, { + forkLessonId: fork.id, + title: "Explain what makes a volcano erupt", + body: "The first paragraph asserted the eruption without giving its cause.", + client: "Claude Desktop", + }); + + assert.equal( + result.lessonId, + source.id, + "proposed to the lesson forked from", + ); + assert.equal(result.pull.ready, true, "the pack landed"); + assert.equal(result.pull.sourceLessonId, fork.id); + assert.equal( + result.pull.base, + source.head, + "records the tip it was built on", + ); + assert.equal(result.pull.head, result.commit); + assert.deepEqual(result.changes, ["- edit text block b1 (text)"]); + + // Nothing was written to the lesson itself — the whole guarantee of the flow. + assert.equal(hub.packs.get(source.id).head, source.head); + assert.equal( + hub.lessons.get(source.id).doc.sections[0].blocks[0].text, + "A volcano ERUPTS.", + ); + + // The proposal's pack is a real clone plus one commit, so a reviewer merging it + // gets a three-way merge against the commit the two histories diverged from. + const uploaded = hub.pullPacks.get(result.pull.id); + assert.ok(uploaded, "the proposal carries its changes"); + const ctx = memRepo("review"); + await cloneFromPack({ ...ctx, ...uploaded }); + assert.equal( + await contains({ ...ctx, oid: uploaded.head, ancestor: source.head }), + true, + ); + assert.equal( + await mergeBase({ ...ctx, ours: uploaded.head, theirs: source.head }), + source.head, + ); + assert.match( + (await readDocAt({ ...ctx, oid: uploaded.head })).sections[0].blocks[0] + .text, + /MAGMA/, + ); + + // And the fork's own history moved forward with it, so proposing again builds + // on this commit rather than remaking it. + assert.equal(hub.packs.get(fork.id).head, uploaded.head); +}); + +test("proposing twice stacks on the first proposal instead of colliding", async () => { + const hub = fakeHub(); + const source = await seedLesson(hub, { + title: "Volcanoes", + text: "A volcano ERUPTS.", + }); + const { lesson: fork } = await forkLesson(hub.api, { lessonId: source.id }); + + hub.lessons.get(fork.id).doc.sections[0].blocks[0].text = "First revision."; + const first = await proposeChanges(hub.api, { + forkLessonId: fork.id, + title: "First pass", + }); + + hub.lessons.get(fork.id).doc.sections[0].blocks[0].text = "Second revision."; + const second = await proposeChanges(hub.api, { + forkLessonId: fork.id, + title: "Second pass", + }); + + assert.notEqual(second.commit, first.commit); + assert.equal(hub.pulls.length, 2, "each proposal is its own request"); + + // The second builds on the first, which is what the compare-and-swap in + // pushLessonPack would have refused otherwise. + const ctx = memRepo("review"); + await cloneFromPack({ ...ctx, ...hub.pullPacks.get(second.pull.id) }); + assert.equal( + await contains({ ...ctx, oid: second.commit, ancestor: first.commit }), + true, + ); +}); + +test("proposing an unchanged fork is refused, and opens nothing", async () => { + const hub = fakeHub(); + const source = await seedLesson(hub, { + title: "Volcanoes", + text: "A volcano ERUPTS.", + }); + const { lesson: fork } = await forkLesson(hub.api, { lessonId: source.id }); + + await assert.rejects( + proposeChanges(hub.api, { forkLessonId: fork.id, title: "Nothing" }), + /nothing to propose/, + ); + assert.equal(hub.pulls.length, 0, "no empty request was left behind"); +}); + +test("a proposal whose pack fails to upload is withdrawn, not left empty", async () => { + const hub = fakeHub(); + const source = await seedLesson(hub, { + title: "Volcanoes", + text: "A volcano ERUPTS.", + }); + const { lesson: fork } = await forkLesson(hub.api, { lessonId: source.id }); + hub.lessons.get(fork.id).doc.sections[0].blocks[0].text = "Revised."; + + hub.api.uploadPullPack = async () => { + throw new Error("R2 is having a moment."); + }; + + await assert.rejects( + proposeChanges(hub.api, { forkLessonId: fork.id, title: "Revise" }), + /R2 is having a moment/, + ); + assert.equal(hub.pulls.length, 1); + assert.equal( + hub.pulls[0].status, + "closed", + "an unreviewable request must not sit in someone's queue", + ); +}); + +test("proposing from a lesson that is not a fork says so", async () => { + const hub = fakeHub(); + await seedLesson(hub, { id: "solo", title: "Solo", text: "Alone." }); + + await assert.rejects( + proposeChanges(hub.api, { forkLessonId: "solo", title: "Change" }), + /not a fork of anything/, + ); +}); + +test("proposing from a lesson with no history points back at fork_lesson", async () => { + const hub = fakeHub(); + hub.lessons.set("draft", { + id: "draft", + title: "Draft", + doc: lessonDoc("Draft", "Text."), + published: false, + forkedFrom: "original", + }); + + await assert.rejects( + proposeChanges(hub.api, { forkLessonId: "draft", title: "Change" }), + /no stored history/, + ); +}); + +test("a proposal's body records that an assistant wrote it", () => { + assert.match( + proposalBody("Because the answer was wrong.", "claude.ai"), + /claude\.ai/, + ); + assert.match(proposalBody("", ""), /AI assistant/); + // The note survives a body long enough to need trimming — it's the provenance. + const long = proposalBody("x".repeat(6000), "Cursor"); + assert.ok(long.length <= 4000); + assert.match(long, /Cursor/); +}); diff --git a/apps/mcp/test/smoke.test.js b/apps/mcp/test/smoke.test.js index 1c75e3a..07cf786 100644 --- a/apps/mcp/test/smoke.test.js +++ b/apps/mcp/test/smoke.test.js @@ -367,10 +367,13 @@ test("the MCP server exposes the full tool set", async () => { "create_lesson", "create_lesson_file", "delete_lesson", + "fork_lesson", "get_lesson", "list_hub_lessons", + "list_lesson_proposals", "list_my_lessons", "patch_lesson", + "propose_changes", "search_images", "set_lesson_published", "update_lesson", diff --git a/packages/core/package.json b/packages/core/package.json index 641162a..c4da43f 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -31,6 +31,7 @@ "./config": "./src/config.js", "./git/doc": "./src/git/doc.js", "./git/layout": "./src/git/layout.js", + "./git/memfs": "./src/git/memfs.js", "./git/merge": "./src/git/merge.js", "./git/ops": "./src/git/ops.js", "./git/pack": "./src/git/pack.js", diff --git a/packages/core/src/git/memfs.js b/packages/core/src/git/memfs.js new file mode 100644 index 0000000..6aa8665 --- /dev/null +++ b/packages/core/src/git/memfs.js @@ -0,0 +1,274 @@ +// An in-memory filesystem for the git engine, for the hosts that have no other. +// +// repo.js and pack.js take `{ fs, gitdir }` precisely so they don't care where +// the objects live: the browser hands them LightningFS over IndexedDB (see +// browser/git/fs.js), and everywhere else hands them this. "Everywhere else" is +// the MCP server (apps/mcp), which forks a lesson and opens a pull request on an +// assistant's behalf — in Node when it runs over stdio, and inside the Cloudflare +// Worker when it runs remotely. Neither has a disk, and the Worker can't have +// one, so the repository is built, used and thrown away in memory. +// +// That fits what a non-browser caller actually does. It never keeps a working +// copy: it clones a lesson's packed history, commits once on top, packs the +// result back up and uploads it. The repository's durable home is R2 (the +// lesson's stored pack), so nothing is lost when this is garbage collected — +// which is why there is no persistence here and no cache to invalidate. +// +// This implements the subset of node:fs's promise API that isomorphic-git binds +// on construction, with POSIX error codes, because isomorphic-git reads `.code` +// to tell "not there" from "broken" (a missing file must be ENOENT, or a first +// commit looks like a failure rather than an empty repo). + +/** File mode for a regular file, as git and node:fs report it. */ +const FILE_MODE = 0o100644; +/** File mode for a directory. */ +const DIR_MODE = 0o40755; +/** File mode for a symbolic link. */ +const SYMLINK_MODE = 0o120777; + +/** A POSIX-shaped error, so isomorphic-git's `err.code` checks work. */ +function fsError(code, path, syscall) { + const err = new Error(`${code}: ${syscall} '${path}'`); + err.code = code; + err.errno = -1; + err.path = path; + err.syscall = syscall; + return err; +} + +/** + * Resolve a path to a canonical absolute one: no empty segments, no "." and no + * "..". Everything is keyed by the result, so "/a/b", "/a//b" and "/a/./b" are + * the same node. + */ +function normalize(path) { + const parts = []; + for (const part of String(path).split("/")) { + if (!part || part === ".") continue; + if (part === "..") { + parts.pop(); + continue; + } + parts.push(part); + } + return `/${parts.join("/")}`; +} + +function dirname(path) { + const at = path.lastIndexOf("/"); + return at <= 0 ? "/" : path.slice(0, at); +} + +/** Coerce whatever a caller wrote to bytes. isomorphic-git writes both. */ +function toBytes(data) { + if (typeof data === "string") return new TextEncoder().encode(data); + if (data instanceof Uint8Array) return data; + if (data instanceof ArrayBuffer) return new Uint8Array(data); + if (ArrayBuffer.isView(data)) { + return new Uint8Array(data.buffer, data.byteOffset, data.byteLength); + } + throw new TypeError("Unsupported data passed to writeFile."); +} + +/** The encoding out of node:fs's `options`, which may be a bare string. */ +function encodingOf(options) { + if (typeof options === "string") return options; + return options?.encoding || null; +} + +/** + * A fresh in-memory filesystem, shaped like `node:fs` — pass it straight to any + * `{ fs, gitdir }` function in repo.js / pack.js. + * + * Nothing is shared between calls: each one is its own empty volume, with its + * own root. Two repositories built in the same process therefore cannot see each + * other, which is what we want when one request forks a lesson while another is + * merging one. + * + * @returns {{ promises: object }} An fs whose `promises` is an own data property + * — isomorphic-git detects the promise API with + * `Object.getOwnPropertyDescriptor(fs, 'promises')`, so a getter or an + * inherited property would silently drop it back to callback style. + */ +export function memFs() { + // path -> { type, data?, target?, mode, mtimeMs, ino } + const nodes = new Map(); + let nextIno = 1; + + const now = () => Date.now(); + + function put(path, node) { + nodes.set(path, { ino: nextIno++, mtimeMs: now(), ...node }); + } + + put("/", { type: "dir", mode: DIR_MODE }); + + function lookup(path, syscall) { + const node = nodes.get(path); + if (!node) throw fsError("ENOENT", path, syscall); + return node; + } + + /** The parent must exist and be a directory before anything can be made in it. */ + function requireParentDir(path, syscall) { + const parent = nodes.get(dirname(path)); + if (!parent) throw fsError("ENOENT", path, syscall); + if (parent.type !== "dir") throw fsError("ENOTDIR", path, syscall); + } + + function statsOf(node) { + const size = node.type === "file" ? node.data.byteLength : 0; + return { + type: node.type === "dir" ? "dir" : "file", + mode: node.mode, + size, + ino: node.ino, + dev: 1, + uid: 1, + gid: 1, + mtimeMs: node.mtimeMs, + ctimeMs: node.mtimeMs, + isFile: () => node.type === "file", + isDirectory: () => node.type === "dir", + isSymbolicLink: () => node.type === "symlink", + }; + } + + const promises = { + async readFile(path, options) { + const full = normalize(path); + const node = lookup(full, "open"); + if (node.type === "dir") throw fsError("EISDIR", full, "read"); + const encoding = encodingOf(options); + if (encoding) return new TextDecoder().decode(node.data); + return node.data; + }, + + async writeFile(path, data, options) { + const full = normalize(path); + requireParentDir(full, "open"); + const existing = nodes.get(full); + if (existing && existing.type === "dir") { + throw fsError("EISDIR", full, "open"); + } + put(full, { + type: "file", + data: toBytes(data), + mode: options?.mode ?? existing?.mode ?? FILE_MODE, + }); + }, + + async unlink(path) { + const full = normalize(path); + const node = lookup(full, "unlink"); + if (node.type === "dir") throw fsError("EISDIR", full, "unlink"); + nodes.delete(full); + }, + + async readdir(path) { + const full = normalize(path); + const node = lookup(full, "scandir"); + if (node.type !== "dir") throw fsError("ENOTDIR", full, "scandir"); + + // Direct children only: every descendant shares the prefix, so a name with + // a "/" left in it belongs to a deeper directory. + const prefix = full === "/" ? "/" : `${full}/`; + const names = []; + for (const key of nodes.keys()) { + if (key === full || !key.startsWith(prefix)) continue; + const rest = key.slice(prefix.length); + if (rest && !rest.includes("/")) names.push(rest); + } + return names.sort(); + }, + + async mkdir(path, options) { + const full = normalize(path); + if (options?.recursive) { + // node:fs makes every missing ancestor and treats an existing directory + // as success, rather than EEXIST. + let current = ""; + for (const part of full.split("/").filter(Boolean)) { + current += `/${part}`; + const existing = nodes.get(current); + if (existing) { + if (existing.type !== "dir") + throw fsError("ENOTDIR", full, "mkdir"); + continue; + } + put(current, { type: "dir", mode: DIR_MODE }); + } + return; + } + if (nodes.has(full)) throw fsError("EEXIST", full, "mkdir"); + requireParentDir(full, "mkdir"); + put(full, { type: "dir", mode: DIR_MODE }); + }, + + async rmdir(path) { + const full = normalize(path); + const node = lookup(full, "rmdir"); + if (node.type !== "dir") throw fsError("ENOTDIR", full, "rmdir"); + const prefix = `${full}/`; + for (const key of nodes.keys()) { + if (key.startsWith(prefix)) throw fsError("ENOTEMPTY", full, "rmdir"); + } + if (full === "/") throw fsError("EBUSY", full, "rmdir"); + nodes.delete(full); + }, + + async stat(path) { + const full = normalize(path); + // Resolve the link, which is the whole difference between stat and lstat. + const node = lookup(full, "stat"); + if (node.type === "symlink") { + return statsOf(lookup(normalize(node.target), "stat")); + } + return statsOf(node); + }, + + async lstat(path) { + return statsOf(lookup(normalize(path), "lstat")); + }, + + async readlink(path) { + const full = normalize(path); + const node = lookup(full, "readlink"); + if (node.type !== "symlink") throw fsError("EINVAL", full, "readlink"); + return node.target; + }, + + async symlink(target, path) { + const full = normalize(path); + requireParentDir(full, "symlink"); + if (nodes.has(full)) throw fsError("EEXIST", full, "symlink"); + put(full, { + type: "symlink", + target: String(target), + mode: SYMLINK_MODE, + }); + }, + + async chmod(path, mode) { + const full = normalize(path); + const node = lookup(full, "chmod"); + node.mode = mode; + }, + }; + + // A plain own data property, not a getter — see the return doc above. + return { promises }; +} + +/** + * A brand-new empty repository context, ready for `cloneFromPack` or `commitDoc`. + * + * The gitdir path is arbitrary (nothing else lives on this volume) but named for + * what it holds, so a stack trace from isomorphic-git reads sensibly. + * + * @returns {{ fs: object, gitdir: string }} The context every function in + * repo.js / pack.js takes. + */ +export function memRepo(name = "lesson") { + return { fs: memFs(), gitdir: `/${name}/.git` }; +} diff --git a/packages/core/src/git/memfs.test.js b/packages/core/src/git/memfs.test.js new file mode 100644 index 0000000..8130fd0 --- /dev/null +++ b/packages/core/src/git/memfs.test.js @@ -0,0 +1,156 @@ +// The in-memory filesystem is only worth anything if isomorphic-git accepts it, +// so these tests drive the real engine over it — commit, pack, clone, merge base +// — rather than checking readFile round-trips. That is the whole contract: the +// MCP server's fork-and-propose flow (apps/mcp/src/git.js) is exactly this +// sequence, with the packs travelling over HTTP in between. + +import { describe, expect, it } from "vitest"; +import { memFs, memRepo } from "./memfs.js"; +import { commitDoc, headOid, readDocAt } from "./repo.js"; +import { cloneFromPack, contains, mergeBase, packRepo } from "./pack.js"; + +const author = { name: "Test", email: "test@example.com" }; + +function doc(title, text) { + return { + title, + sections: [ + { id: "s1", name: "One", blocks: [{ id: "b1", type: "text", text }] }, + ], + }; +} + +describe("memFs", () => { + it("looks like node:fs to isomorphic-git", () => { + const fs = memFs(); + // isomorphic-git detects the promise API with getOwnPropertyDescriptor and + // falls back to callback style when it isn't an own data property. + const descriptor = Object.getOwnPropertyDescriptor(fs, "promises"); + expect(descriptor?.value).toBeTruthy(); + for (const method of [ + "readFile", + "writeFile", + "unlink", + "readdir", + "mkdir", + "rmdir", + "stat", + "lstat", + "readlink", + "symlink", + ]) { + expect(typeof fs.promises[method]).toBe("function"); + } + }); + + it("reports a missing file as ENOENT", async () => { + const fs = memFs(); + await expect(fs.promises.readFile("/nope")).rejects.toMatchObject({ + code: "ENOENT", + }); + await expect(fs.promises.stat("/nope")).rejects.toMatchObject({ + code: "ENOENT", + }); + }); + + it("lists only a directory's direct children", async () => { + const fs = memFs(); + await fs.promises.mkdir("/a", { recursive: true }); + await fs.promises.mkdir("/a/b", { recursive: true }); + await fs.promises.writeFile("/a/one.txt", "1"); + await fs.promises.writeFile("/a/b/two.txt", "2"); + + expect(await fs.promises.readdir("/a")).toEqual(["b", "one.txt"]); + expect(await fs.promises.readdir("/a/b")).toEqual(["two.txt"]); + }); + + it("round-trips bytes and text", async () => { + const fs = memFs(); + await fs.promises.writeFile("/hello", "héllo"); + expect(await fs.promises.readFile("/hello", "utf8")).toBe("héllo"); + expect(await fs.promises.readFile("/hello")).toBeInstanceOf(Uint8Array); + }); + + it("refuses to remove a directory that still has something in it", async () => { + const fs = memFs(); + await fs.promises.mkdir("/a", { recursive: true }); + await fs.promises.writeFile("/a/one.txt", "1"); + await expect(fs.promises.rmdir("/a")).rejects.toMatchObject({ + code: "ENOTEMPTY", + }); + await fs.promises.unlink("/a/one.txt"); + await fs.promises.rmdir("/a"); + expect(await fs.promises.readdir("/")).toEqual([]); + }); +}); + +describe("the git engine on an in-memory repo", () => { + it("commits a document and reads it back", async () => { + const ctx = memRepo(); + const first = await commitDoc({ + ...ctx, + doc: doc("Volcanoes", "Lava"), + author, + }); + + expect(first?.oid).toMatch(/^[0-9a-f]{40}$/); + expect(await headOid(ctx)).toBe(first.oid); + expect(await readDocAt({ ...ctx, oid: first.oid })).toMatchObject({ + title: "Volcanoes", + }); + }); + + it("does not commit an unchanged document twice", async () => { + const ctx = memRepo(); + await commitDoc({ ...ctx, doc: doc("Volcanoes", "Lava"), author }); + const again = await commitDoc({ + ...ctx, + doc: doc("Volcanoes", "Lava"), + author, + }); + expect(again).toBeNull(); + }); + + it("packs a history and clones it into a fork that shares ancestry", async () => { + // The original: two commits, packed for upload exactly as the Worker stores it. + const origin = memRepo("origin"); + await commitDoc({ ...origin, doc: doc("Rivers", "Source"), author }); + const forkPoint = await commitDoc({ + ...origin, + doc: doc("Rivers", "Source to sea"), + author, + }); + const packed = await packRepo(origin); + expect(packed.head).toBe(forkPoint.oid); + + // The fork: a genuine clone, so the same commit oids and the same content. + const fork = memRepo("fork"); + await cloneFromPack({ ...fork, ...packed }); + expect(await headOid(fork)).toBe(packed.head); + expect(await readDocAt({ ...fork, oid: packed.head })).toMatchObject({ + title: "Rivers", + }); + + // What the assistant proposes: one commit on top of the clone. + const proposal = await commitDoc({ + ...fork, + doc: doc("Rivers", "Source to sea, and the delta"), + author, + }); + + // And the ancestry that makes a reviewer's three-way merge real. + expect( + await contains({ ...fork, oid: proposal.oid, ancestor: packed.head }), + ).toBe(true); + expect( + await mergeBase({ ...fork, ours: proposal.oid, theirs: packed.head }), + ).toBe(packed.head); + }); + + it("keeps two in-memory repos out of each other's way", async () => { + const a = memRepo(); + const b = memRepo(); + await commitDoc({ ...a, doc: doc("A", "a"), author }); + expect(await headOid(b)).toBeNull(); + }); +}); From c3f0a2111adf199b64b1e1101f90025d3d04f8c8 Mon Sep 17 00:00:00 2001 From: playforge-coding <251060555+playforge-coding@users.noreply.github.com> Date: Mon, 10 Aug 2026 17:48:03 -0500 Subject: [PATCH 2/2] Address PR review: five ways a proposal could go wrong MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three bots, ten findings. The four that were real bugs: A proposal pushed the fork's history *before* opening the request, so any failure after that point left the fork's document equal to its own history — the changes safe, but the retry finding nothing pending and refusing it. The push is bookkeeping (a proposal's pack is stored with the proposal, and that is what a reviewer merges), so it now happens last and cannot fail the call; a failed proposal leaves the fork untouched and the retry simply works. commitDoc returns null when the tree is unchanged, and pendingOps answers the looser question of whether the *documents* differ, so the two can disagree. The null was then dereferenced in the return value — after the proposal had gone live, reporting failure for something that had succeeded. It fails before anything is sent instead. Forking read the source document before its history. A lesson being saved in the browser writes those in the other order, so that pairing could put stale content on top of newer commits and quietly revert the save it raced. Reading the history first makes the bad pairing unreachable. And openPull accepted any lesson the caller owned as a proposal's source, while the source is the one thing that unlocks a self-proposal — so citing an unrelated lesson of your own turned the rule off entirely and recorded a fork link to something that wasn't one. It now requires the source to be forked from this lesson, which is what the documentation already claimed. The rest: require a non-empty proposal title, since the hub rejects an empty one only after the whole snapshot has been built and sent; align SERVER_INFO with the package and manifest version, which clients actually display; keep a file's inode when it is rewritten, and state the by-reference contract memfs departs on; bound a client's self-reported name before it goes in a proposal; and let fetchLessonHead throw on a real failure rather than reporting "no history". The tool table was also missing create_lesson_file, and the notification wording claimed every pull_request link opens the proposal when merged and closed ones open the lesson. Tests for the paths that had none: a fork whose history can't be stored, a failed proposal retried without further edits, a proposal surviving a failed history push, the fork read ordering, and that the client name reaches the body a reviewer reads. Co-Authored-By: Claude Opus 5 (1M context) --- apps/api/src/lib/lesson.js | 5 +- apps/api/src/routes/pulls.js | 30 ++++-- apps/docs/docs/mcp-server/tools.md | 1 + apps/docs/docs/web-app/notifications.md | 5 +- apps/docs/docs/web-app/pull-requests.md | 9 +- apps/mcp/src/api.js | 13 ++- apps/mcp/src/git.js | 86 ++++++++++++---- apps/mcp/src/tools.js | 18 +++- apps/mcp/test/fork.test.js | 126 +++++++++++++++++++++++- packages/core/src/git/memfs.js | 18 +++- packages/core/src/git/memfs.test.js | 17 +++- 11 files changed, 284 insertions(+), 44 deletions(-) diff --git a/apps/api/src/lib/lesson.js b/apps/api/src/lib/lesson.js index dcddedf..66997cb 100644 --- a/apps/api/src/lib/lesson.js +++ b/apps/api/src/lib/lesson.js @@ -52,7 +52,10 @@ export function isTrustedCollaborator(row, user) { * callers treat both as "no". */ export async function fetchLessonRow(env, base, lessonId, { withDoc = false } = {}) { - const columns = withDoc ? 'id,author_id,published,shadowbanned,doc' : 'id,author_id,published,shadowbanned'; + // `forked_from` rides along because it is a permission input, not just display: + // opening a proposal against your own lesson is allowed only from a fork *of + // that lesson* (see openPull in routes/pulls.js). + const columns = withDoc ? 'id,author_id,published,shadowbanned,forked_from,doc' : 'id,author_id,published,shadowbanned,forked_from'; const query = `id=eq.${encodeURIComponent(lessonId)}&select=${columns}&limit=1`; let res; try { diff --git a/apps/api/src/routes/pulls.js b/apps/api/src/routes/pulls.js index c2e3a35..212a79a 100644 --- a/apps/api/src/routes/pulls.js +++ b/apps/api/src/routes/pulls.js @@ -307,24 +307,34 @@ async function openPull(request, env, base, lessonId, cors) { // // Resolved before the own-lesson check below, which turns on it. let sourceLessonId = null; + // Whether that source is a fork *of this lesson*, which is a stricter thing and + // the only one that may unlock a self-proposal below. + let sourceForkedFromThis = false; const claimed = typeof body.sourceLessonId === 'string' ? body.sourceLessonId.trim() : ''; if (LESSON_ID_RE.test(claimed) && claimed !== lessonId) { const source = await fetchLessonRow(env, base, claimed); - if (source && source.author_id === user.id) sourceLessonId = claimed; + if (source && source.author_id === user.id) { + sourceLessonId = claimed; + sourceForkedFromThis = source.forked_from === lessonId; + } } // Proposing to your own lesson is refused when there is nothing behind it: you // can simply save, and a request to yourself out of nowhere is a mistake. // - // It is allowed when it carries a fork you own, because then it means something - // specific and useful — "here is a copy with changes in it, let me read the diff - // before it lands". That is the shape of an AI assistant's work: over MCP the - // assistant acts as the account it is signed in with, so changes it proposes to - // the user's own lesson arrive from the user's own id (see apps/mcp/src/git.js). - // Holding them in the review queue is the whole point — the lesson is untouched - // until a person reads the diff and merges it. A human gets the same route via - // "fork into a new lesson" in the editor. - if (lesson.author_id === user.id && !sourceLessonId) { + // It is allowed when it carries a fork *of this lesson* that you own, because + // then it means something specific and useful — "here is a copy with changes in + // it, let me read the diff before it lands". That is the shape of an AI + // assistant's work: over MCP the assistant acts as the account it is signed in + // with, so changes it proposes to the user's own lesson arrive from the user's + // own id (see apps/mcp/src/git.js). Holding them in the review queue is the + // whole point — the lesson is untouched until a person reads the diff and merges + // it. A human gets the same route via "fork into a new lesson" in the editor. + // + // Ownership of the source alone is deliberately not enough: any other lesson of + // theirs would satisfy that, which would turn the rule off entirely and attach a + // fork link to something that isn't one. + if (lesson.author_id === user.id && !sourceForkedFromThis) { return textResponse('This is your own lesson — save your changes to it directly instead.', 400, cors); } diff --git a/apps/docs/docs/mcp-server/tools.md b/apps/docs/docs/mcp-server/tools.md index 70569b9..78c94da 100644 --- a/apps/docs/docs/mcp-server/tools.md +++ b/apps/docs/docs/mcp-server/tools.md @@ -8,6 +8,7 @@ title: Tools | ----------------------- | ----------------------------------------------------------------------------- | | `whoami` | Confirm the session is valid and show the publishing display name. | | `create_lesson` | Build and save a new lesson (draft by default; `published: true` to share). | +| `create_lesson_file` | Build an importable lesson file offline, with no account or network. | | `patch_lesson` | Edit a lesson with a small diff (id-addressed ops) instead of a full replace. | | `update_lesson` | Replace a lesson's whole title/content (author only). | | `fork_lesson` | Copy a lesson into a private draft of your own, keeping its version history. | diff --git a/apps/docs/docs/web-app/notifications.md b/apps/docs/docs/web-app/notifications.md index efb9db5..9f5c9c2 100644 --- a/apps/docs/docs/web-app/notifications.md +++ b/apps/docs/docs/web-app/notifications.md @@ -28,7 +28,10 @@ queries the table directly — everything goes through the Worker's profile. See [Following](./profiles-and-display-names.md#following). - **`pull_request`** — someone proposed changes to a lesson you published, or the proposal _you_ made was merged or closed. The three are one type because they're - one conversation; the title says which happened and the link opens the proposal. + one conversation, and the title says which happened. The link follows what you'd + want next: a proposal to review opens that proposal, while a merged or closed one + opens the lesson, since the decision has already been made and what matters is + where your changes ended up. Nothing is sent until a proposal actually has changes in it (an upload that never finished notifies nobody), and withdrawing your own never notifies you. A proposal opened from your own account — which is how an AI assistant working diff --git a/apps/docs/docs/web-app/pull-requests.md b/apps/docs/docs/web-app/pull-requests.md index 9391573..9e2ee4c 100644 --- a/apps/docs/docs/web-app/pull-requests.md +++ b/apps/docs/docs/web-app/pull-requests.md @@ -90,10 +90,11 @@ author's queue. Out of nowhere, this is a mistake: you can just save, so the Worker refuses a proposal against your own lesson rather than creating a request nobody needs. -It's allowed when it **carries a fork you own** (`sourceLessonId` resolves to -another of your lessons), because then it means something specific: _here is a -copy with changes in it, let me read the diff before it lands._ Two things use -that: +It's allowed when it **carries a fork of that lesson which you own** — that is, +`sourceLessonId` names one of your lessons whose `forked_from` is this one. +Ownership alone isn't enough, since any other lesson of yours would satisfy it and +turn the rule off entirely. Then it means something specific: _here is a copy with +changes in it, let me read the diff before it lands._ Two things use that: - An **AI assistant over MCP** acts as the account it's signed in with, so changes it proposes to your lesson arrive from your own id. Holding them in the diff --git a/apps/mcp/src/api.js b/apps/mcp/src/api.js index eab8d58..0d0f2d5 100644 --- a/apps/mcp/src/api.js +++ b/apps/mcp/src/api.js @@ -227,10 +227,19 @@ export function createApi(config, auth) { return getPack(gitUrl(lessonId, "/pack")); }, - /** The tip of a lesson's published history, or null when it has none. */ + /** + * The tip of a lesson's published history, or null when it has none. + * + * Only a 404 means "none" — every other bad status is a genuine failure and + * throws, rather than being flattened into the same answer. A caller that + * would rather not know (the proposal's informational `base`) can catch it; + * one that needs the real head must not be told there isn't one. + */ async fetchLessonHead(lessonId) { const res = await request(gitUrl(lessonId, "/refs")); - if (!res.ok) return null; + if (res.status === 404) return null; + if (!res.ok) + throw await readError(res, "Could not read the lesson history."); const data = await res.json().catch(() => null); return data?.head || null; }, diff --git a/apps/mcp/src/git.js b/apps/mcp/src/git.js index a902af3..bf296f9 100644 --- a/apps/mcp/src/git.js +++ b/apps/mcp/src/git.js @@ -62,12 +62,18 @@ async function commitAuthor(api) { /** Trim to a limit on a word boundary where there is one nearby. */ function clamp(value, limit) { const text = (value || "").trim(); + if (limit <= 0) return ""; // no room at all; slicing by a negative would cut from the end if (text.length <= limit) return text; const cut = text.slice(0, limit - 1); const space = cut.lastIndexOf(" "); return `${(space > limit * 0.8 ? cut.slice(0, space) : cut).trimEnd()}…`; } +// A client's self-reported name is used in the proposal's provenance note, so it +// is bounded before it gets there: it is arbitrary text from the connecting +// client, and an absurd one must not crowd out the body it is annotating. +const CLIENT_NAME_MAX = 80; + /** * The proposal's body, with a note saying which assistant wrote it. * @@ -78,8 +84,9 @@ function clamp(value, limit) { * honest answer available — we know what connected, not what model it drove. */ export function proposalBody(body, client) { - const note = client - ? `Proposed by an AI assistant via ${client} (Spelling Creator MCP).` + const named = clamp(client, CLIENT_NAME_MAX); + const note = named + ? `Proposed by an AI assistant via ${named} (Spelling Creator MCP).` : "Proposed by an AI assistant via the Spelling Creator MCP server."; const text = (body || "").trim(); if (!text) return note; @@ -115,13 +122,22 @@ async function cloneRepo(pack) { * @returns {Promise<{ lesson: object, head: string, clonedHistory: boolean }>} */ export async function forkLesson(api, { lessonId, title }) { + // Read the history *before* the document, which is the safe order rather than + // the obvious one. A lesson being saved in the browser at this moment pushes + // its history first and its document row second (see the ordering note in + // apps/web/src/pages/EditorPage.jsx), so a document read after a pack is never + // older than that pack. Read the other way round and we could pair yesterday's + // document with today's history — and the reconciliation commit below would + // then commit stale content on top of newer commits, quietly reverting the save + // it raced. This way that pairing can't arise: at worst the document is newer, + // which the reconciliation commit simply carries forward. + const pack = await api.fetchLessonPack(lessonId); + const source = await api.getLesson(lessonId); if (!source.doc?.sections?.length) { throw new Error("That lesson has no content to fork."); } - const pack = await api.fetchLessonPack(lessonId); - // The fork's document. Local-only fields never travel (see core/git/doc.js): // the trusted-collaborator list belongs to the lesson it was named on, not to // a copy of it, and it must not be carried into a new lesson's document. @@ -202,7 +218,10 @@ export async function forkLesson(api, { lessonId, title }) { * uploaded against the request's id. If the upload fails the empty request is * withdrawn, rather than left in a review queue with nothing in it. * - * @returns {Promise<{ pull: object, lessonId: string, commit: string, ops: object[] }>} + * The fork's own history is pushed *last*, deliberately: see below. + * + * @returns {Promise<{ pull: object, lessonId: string, commit: string, + * changes: string[], historyPushed: boolean }>} */ export async function proposeChanges( api, @@ -255,18 +274,19 @@ export async function proposeChanges( author, message: `${clamp(title, PULL_TITLE_MAX)}\n\n${ops.map(describeOp).join("\n")}\n`, }); + // `ops` says the documents differ; commitDoc says the *trees* do, which is the + // stricter question (a field git doesn't store can differ without changing the + // tree). Nothing has been sent yet, so bail here rather than dereferencing a + // null commit further down, once the proposal is already live. + if (!commit) { + throw new Error( + "This fork's document is already committed, so there is nothing to propose. " + + "Edit the fork first (patch_lesson on the fork's id), then try again.", + ); + } const packed = await packRepo(ctx); - // Push the fork's own history before proposing, so the fork's History tab and - // the proposal agree, and so re-proposing later builds on this commit rather - // than re-making it. - await api.pushLessonPack(forkLessonId, { - packfile: packed.packfile, - head: packed.head, - parent: forkPack.head, - }); - // The target's tip as it stands, recorded on the request so a reviewer can see // what it was built against. Informational: the merge finds its own base from // the shared ancestry. @@ -280,19 +300,43 @@ export async function proposeChanges( sourceLessonId: forkLessonId, }); + let ready; try { - const ready = await api.uploadPullPack(target, pull.id, { + ready = await api.uploadPullPack(target, pull.id, { packfile: packed.packfile, head: packed.head, }); - return { - pull: ready || pull, - lessonId: target, - commit: commit.oid, - changes: ops.map(describeOp), - }; } catch (err) { await api.closePull(target, pull.id).catch(() => {}); throw err; } + + // Only now advance the fork's own stored history, and don't let it fail the + // call. The proposal does not depend on it — its pack is stored separately, and + // that is what a reviewer merges — so this is bookkeeping: it keeps the fork's + // History tab honest and gives the next proposal this commit to build on. + // + // The order matters. Pushing first would mean a failure anywhere below left the + // fork's document equal to its own history, so the retry would find no pending + // operations and refuse — the changes safe but unproposable without making a + // further edit. Pushing last, a failed proposal leaves the fork exactly as it + // was and the retry simply works. + let historyPushed = true; + try { + await api.pushLessonPack(forkLessonId, { + packfile: packed.packfile, + head: packed.head, + parent: forkPack.head, + }); + } catch { + historyPushed = false; + } + + return { + pull: ready || pull, + lessonId: target, + commit: commit.oid, + changes: ops.map(describeOp), + historyPushed, + }; } diff --git a/apps/mcp/src/tools.js b/apps/mcp/src/tools.js index be988d4..688697e 100644 --- a/apps/mcp/src/tools.js +++ b/apps/mcp/src/tools.js @@ -632,8 +632,11 @@ export function registerTools(server, ctx) { .describe( "The lesson to propose to. Defaults to the one the fork was forked from, which is nearly always right.", ), + // Non-empty: the hub requires a title, and it would otherwise reject the + // proposal only after the whole snapshot had been built and sent. title: z .string() + .min(1) .describe( "One line naming the change, e.g. 'Fix three ungrounded answers in section 4'.", ), @@ -652,6 +655,7 @@ export function registerTools(server, ctx) { lessonId: target, commit, changes, + historyPushed, } = await proposeChanges(api, { forkLessonId, lessonId, @@ -671,7 +675,13 @@ export function registerTools(server, ctx) { url: proposalUrl(target, pull.id), note: "Proposal opened. Nothing has changed in the lesson itself — give the user the `url` so they can read " + - "the diff and merge or decline it. Poll list_lesson_proposals if you need to know what they decided.", + "the diff and merge or decline it. Poll list_lesson_proposals if you need to know what they decided." + + // The proposal is complete either way — its changes are stored with it. + // This only means the fork's own history didn't catch up. + (historyPushed + ? "" + : " (The proposal is complete, but the fork's own version history could not be updated, so the fork's " + + "History tab won't show this change and a further proposal from it will re-send the same edits.)"), }); }), ); @@ -1010,7 +1020,11 @@ export function registerTools(server, ctx) { } // The server's identifying metadata, shared by both transports. +// +// Keep `version` in step with apps/mcp/package.json and apps/mcp/manifest.json: +// this is the one clients actually see, so a stale value misnames the server in +// every client UI and bug report. export const SERVER_INFO = { name: "spelling-creator-hub", - version: "0.2.0", + version: "0.3.0", }; diff --git a/apps/mcp/test/fork.test.js b/apps/mcp/test/fork.test.js index bbbb85b..6ced3c9 100644 --- a/apps/mcp/test/fork.test.js +++ b/apps/mcp/test/fork.test.js @@ -18,6 +18,7 @@ import { cloneFromPack, contains, mergeBase, + packRepo, } from "@spelling-creator/core/git/pack"; import { commitDoc, headOid, readDocAt } from "@spelling-creator/core/git/repo"; @@ -149,7 +150,6 @@ async function seedLesson(hub, { id = "original", title, text }) { const ctx = memRepo("seed"); const first = await commitDoc({ ...ctx, doc, author: AUTHOR }); - const { packRepo } = await import("@spelling-creator/core/git/pack"); const packed = await packRepo(ctx); hub.packs.set(id, { packfile: packed.packfile, head: packed.head }); return { id, doc, head: first.oid }; @@ -187,6 +187,57 @@ test("forking clones the lesson's history under a new private draft", async () = ); }); +test("a fork whose history can't be stored says so, and keeps the draft", async () => { + const hub = fakeHub(); + const source = await seedLesson(hub, { + title: "Volcanoes", + text: "A volcano ERUPTS.", + }); + hub.api.pushLessonPack = async () => { + throw new Error("R2 is having a moment."); + }; + + // The row is created before the history is pushed, so a failed push leaves a + // fork that nothing can be proposed from. Say that, rather than letting it be + // rediscovered at propose time. + await assert.rejects( + forkLesson(hub.api, { lessonId: source.id }), + /could not be stored.*Delete it and fork again/s, + ); + + // The draft itself is a real copy of the document and the user's to keep or + // remove, so it is not deleted behind their back. + assert.equal(hub.lessons.size, 2, "the fork's row is still there"); + const fork = [...hub.lessons.values()].find((l) => l.id !== source.id); + assert.equal(fork.forkedFrom, source.id); + assert.equal(hub.packs.has(fork.id), false, "and it has no history"); +}); + +test("forking reads the history before the document", async () => { + // A lesson being saved in the browser pushes its history first and its document + // second, so reading the document first could pair an old document with a new + // pack — and the reconciliation commit would then revert the save it raced. + const hub = fakeHub(); + const source = await seedLesson(hub, { + title: "Volcanoes", + text: "A volcano ERUPTS.", + }); + + const calls = []; + const { fetchLessonPack, getLesson } = hub.api; + hub.api.fetchLessonPack = async (id) => { + calls.push("pack"); + return fetchLessonPack(id); + }; + hub.api.getLesson = async (id) => { + calls.push("doc"); + return getLesson(id); + }; + + await forkLesson(hub.api, { lessonId: source.id }); + assert.deepEqual(calls.slice(0, 2), ["pack", "doc"]); +}); + test("forking a lesson with no stored history seeds one from its document", async () => { const hub = fakeHub(); hub.lessons.set("plain", { @@ -269,6 +320,12 @@ test("proposing sends a pack that shares ancestry with the target lesson", async assert.equal(result.pull.head, result.commit); assert.deepEqual(result.changes, ["- edit text block b1 (text)"]); + // The provenance reaches the proposal a reviewer actually reads: on a + // self-proposal it is the only thing saying they didn't write this. + assert.match(result.pull.body, /Claude Desktop/); + assert.match(result.pull.body, /AI assistant/); + assert.match(result.pull.body, /without giving its cause/); + // Nothing was written to the lesson itself — the whole guarantee of the flow. assert.equal(hub.packs.get(source.id).head, source.head); assert.equal( @@ -374,6 +431,73 @@ test("a proposal whose pack fails to upload is withdrawn, not left empty", async ); }); +test("a failed proposal can be retried without editing the fork again", async () => { + // The fork's history is pushed only after the proposal has landed. Pushing it + // first would leave the fork's document equal to its own history, so the retry + // would find nothing pending and refuse — the changes safe but unproposable. + const hub = fakeHub(); + const source = await seedLesson(hub, { + title: "Volcanoes", + text: "A volcano ERUPTS.", + }); + const { lesson: fork } = await forkLesson(hub.api, { lessonId: source.id }); + const forkHead = hub.packs.get(fork.id).head; + hub.lessons.get(fork.id).doc.sections[0].blocks[0].text = "Revised."; + + // First attempt: the proposal itself fails. + const { createPull } = hub.api; + hub.api.createPull = async () => { + throw new Error("The hub is having a moment."); + }; + await assert.rejects( + proposeChanges(hub.api, { forkLessonId: fork.id, title: "Revise" }), + /The hub is having a moment/, + ); + assert.equal( + hub.packs.get(fork.id).head, + forkHead, + "the fork's history did not move, so the edit is still pending", + ); + + // Second attempt: it works, with no further edit to the fork. + hub.api.createPull = createPull; + const result = await proposeChanges(hub.api, { + forkLessonId: fork.id, + title: "Revise", + }); + assert.equal(result.pull.ready, true); + assert.equal(result.historyPushed, true); + + const ctx = memRepo("review"); + await cloneFromPack({ ...ctx, ...hub.pullPacks.get(result.pull.id) }); + const proposed = await readDocAt({ ...ctx, oid: result.commit }); + assert.match(proposed.sections[0].blocks[0].text, /Revised/); +}); + +test("a proposal still stands when the fork's own history can't be updated", async () => { + const hub = fakeHub(); + const source = await seedLesson(hub, { + title: "Volcanoes", + text: "A volcano ERUPTS.", + }); + const { lesson: fork } = await forkLesson(hub.api, { lessonId: source.id }); + hub.lessons.get(fork.id).doc.sections[0].blocks[0].text = "Revised."; + + hub.api.pushLessonPack = async () => { + throw new Error("R2 is having a moment."); + }; + + // The proposal's changes are stored with the proposal, so it is complete and + // reviewable regardless — this is bookkeeping, and must not fail the call. + const result = await proposeChanges(hub.api, { + forkLessonId: fork.id, + title: "Revise", + }); + assert.equal(result.pull.ready, true); + assert.equal(result.historyPushed, false, "reported, not thrown"); + assert.ok(hub.pullPacks.get(result.pull.id)); +}); + test("proposing from a lesson that is not a fork says so", async () => { const hub = fakeHub(); await seedLesson(hub, { id: "solo", title: "Solo", text: "Alone." }); diff --git a/packages/core/src/git/memfs.js b/packages/core/src/git/memfs.js index 6aa8665..a907d6a 100644 --- a/packages/core/src/git/memfs.js +++ b/packages/core/src/git/memfs.js @@ -18,6 +18,14 @@ // on construction, with POSIX error codes, because isomorphic-git reads `.code` // to tell "not there" from "broken" (a missing file must be ENOENT, or a first // commit looks like a failure rather than an empty repo). +// +// One departure from a real filesystem: file contents are stored and returned by +// reference, not copied. A real fs copies through the kernel, so a caller may +// safely mutate a buffer it wrote or one it read back. Here that would alias. +// Nothing does it — the git engine treats both as immutable — and a packfile runs +// to megabytes, so copying every blob and pack twice per operation would be a +// real cost against a hypothetical caller. Anything else sharing this module must +// keep to the same rule. /** File mode for a regular file, as git and node:fs report it. */ const FILE_MODE = 0o100644; @@ -97,8 +105,16 @@ export function memFs() { const now = () => Date.now(); + // Overwriting keeps the inode: on a real filesystem rewriting a file in place + // doesn't make it a different file, and isomorphic-git's index caching reads + // stat data to decide what it can trust. function put(path, node) { - nodes.set(path, { ino: nextIno++, mtimeMs: now(), ...node }); + const existing = nodes.get(path); + nodes.set(path, { + ino: existing ? existing.ino : nextIno++, + mtimeMs: now(), + ...node, + }); } put("/", { type: "dir", mode: DIR_MODE }); diff --git a/packages/core/src/git/memfs.test.js b/packages/core/src/git/memfs.test.js index 8130fd0..7fee06e 100644 --- a/packages/core/src/git/memfs.test.js +++ b/packages/core/src/git/memfs.test.js @@ -38,11 +38,23 @@ describe("memFs", () => { "lstat", "readlink", "symlink", + "chmod", ]) { expect(typeof fs.promises[method]).toBe("function"); } }); + it("keeps a file's inode when it is rewritten", async () => { + const fs = memFs(); + await fs.promises.writeFile("/a", "one"); + const before = await fs.promises.stat("/a"); + await fs.promises.writeFile("/a", "two"); + const after = await fs.promises.stat("/a"); + + expect(after.ino).toBe(before.ino); + expect(after.size).toBe(3); + }); + it("reports a missing file as ENOENT", async () => { const fs = memFs(); await expect(fs.promises.readFile("/nope")).rejects.toMatchObject({ @@ -150,7 +162,10 @@ describe("the git engine on an in-memory repo", () => { it("keeps two in-memory repos out of each other's way", async () => { const a = memRepo(); const b = memRepo(); - await commitDoc({ ...a, doc: doc("A", "a"), author }); + // Assert both halves: that a's commit landed, so this proves isolation + // rather than that nothing happened anywhere. + const committed = await commitDoc({ ...a, doc: doc("A", "a"), author }); + expect(await headOid(a)).toBe(committed.oid); expect(await headOid(b)).toBeNull(); }); });