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 f965181..212a79a 100644 --- a/apps/api/src/routes/pulls.js +++ b/apps/api/src/routes/pulls.js @@ -300,7 +300,41 @@ 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; + // 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; + 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 *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); } @@ -326,17 +360,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 +454,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..78c94da 100644 --- a/apps/docs/docs/mcp-server/tools.md +++ b/apps/docs/docs/mcp-server/tools.md @@ -4,19 +4,70 @@ 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). | +| `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. | +| `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..9f5c9c2 100644 --- a/apps/docs/docs/web-app/notifications.md +++ b/apps/docs/docs/web-app/notifications.md @@ -28,9 +28,15 @@ 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, 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 + 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..9e2ee4c 100644 --- a/apps/docs/docs/web-app/pull-requests.md +++ b/apps/docs/docs/web-app/pull-requests.md @@ -79,12 +79,33 @@ 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 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 + 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 +187,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 +209,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..0d0f2d5 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,101 @@ 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. + * + * 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.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; + }, + + /** + * 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 +316,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..bf296f9 --- /dev/null +++ b/apps/mcp/src/git.js @@ -0,0 +1,342 @@ +// 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 (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. + * + * 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 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; + // 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 }) { + // 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."); + } + + // 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. + * + * 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, + { 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`, + }); + // `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); + + // 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, + }); + + let ready; + try { + ready = await api.uploadPullPack(target, pull.id, { + packfile: packed.packfile, + head: packed.head, + }); + } 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 7862b65..688697e 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,162 @@ 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.", + ), + // 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'.", + ), + 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, + historyPushed, + } = 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." + + // 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.)"), + }); + }), + ); + + 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", { @@ -849,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.1.3", + version: "0.3.0", }; diff --git a/apps/mcp/test/fork.test.js b/apps/mcp/test/fork.test.js new file mode 100644 index 0000000..6ced3c9 --- /dev/null +++ b/apps/mcp/test/fork.test.js @@ -0,0 +1,537 @@ +// 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, + packRepo, +} 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 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("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", { + 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)"]); + + // 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( + 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("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." }); + + 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..a907d6a --- /dev/null +++ b/packages/core/src/git/memfs.js @@ -0,0 +1,290 @@ +// 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). +// +// 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; +/** 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(); + + // 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) { + const existing = nodes.get(path); + nodes.set(path, { + ino: existing ? existing.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..7fee06e --- /dev/null +++ b/packages/core/src/git/memfs.test.js @@ -0,0 +1,171 @@ +// 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", + "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({ + 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(); + // 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(); + }); +});