More than one version of a lesson, and proposals you can read

apps/api/schema.sql

@@ -262,9 +262,30 @@ create table if not exists public.lesson_pull_requests (
   resolved_at   timestamptz,
   -- Recorded so an admin can later ban the address, as for lessons and comments.
   author_ip     text,
-  created_at    timestamptz not null default now()
+  created_at    timestamptz not null default now(),
+  -- The branch of the fork this was proposed from, for display. A proposer can
+  -- work on a variation of their fork (see /web-app/lesson-variations) and offer
+  -- that, so "which one is this?" is a question the queue has to be able to
+  -- answer. Null on a proposal opened before this, and on one from the default
+  -- branch, where naming it would say nothing.
+  head_ref      text,
+  -- A proposal can be updated while it is open, and each upload is a revision.
+  -- `revision` counts them from 1; `previous_head` is the commit the proposal
+  -- pointed at before the most recent one, which is what makes "what changed in
+  -- this update" answerable — both commits are in the stored pack, because an
+  -- update may only move the proposer's branch forward.
+  revision      integer not null default 1,
+  previous_head text,
+  updated_at    timestamptz
 );
 
+-- Columns added after the table shipped. Safe to re-run, and safe on a database
+-- that already has them.
+alter table public.lesson_pull_requests add column if not exists head_ref text;
+alter table public.lesson_pull_requests add column if not exists revision integer not null default 1;
+alter table public.lesson_pull_requests add column if not exists previous_head text;
+alter table public.lesson_pull_requests add column if not exists updated_at timestamptz;
+
 -- The lesson page and the editor both ask "the open proposals on this lesson,
 -- newest first"; index the filter + sort key.
 create index if not exists lesson_pull_requests_lesson_idx

apps/api/src/lib/cors.js

@@ -33,13 +33,18 @@ export function corsHeaders(request, allowed) {
 			headers.set('Access-Control-Allow-Origin', origin);
 			headers.set('Vary', 'Origin');
 			headers.set('Access-Control-Allow-Methods', 'GET, POST, PUT, DELETE, OPTIONS');
-			// X-Git-Head and X-Git-Parent carry a pushed history's tip and the head it
-			// expects to replace (the compare-and-swap in routes/git.js), and the same
-			// tip when a pull request's pack is uploaded. Neither is a safelisted
-			// header, so without naming them here the browser's preflight refuses the
-			// upload — invisibly, whenever the app is served from a different origin
-			// than the API. Same-origin deploys never preflight and so never noticed.
-			headers.set('Access-Control-Allow-Headers', 'Content-Type, Authorization, X-Git-Head, X-Git-Parent');
+			// The X-Git-* headers carry a pushed history's tips and the tips it expects
+			// to replace (the compare-and-swap in routes/git.js) — X-Git-Head and
+			// X-Git-Parent for the lesson itself, and Refs/Expected/Deletes for its
+			// variations. The same tip travels on a pull request's pack upload. None is
+			// a safelisted header, so without naming them here the browser's preflight
+			// refuses the upload — invisibly, whenever the app is served from a
+			// different origin than the API. Same-origin deploys never preflight and so
+			// never noticed.
+			headers.set(
+				'Access-Control-Allow-Headers',
+				'Content-Type, Authorization, X-Git-Head, X-Git-Parent, X-Git-Refs, X-Git-Expected, X-Git-Deletes',
+			);
 			headers.set('Access-Control-Max-Age', '86400');
 		}
 	}

apps/api/src/lib/lessonGit.js

@@ -6,7 +6,14 @@
 // here. Two objects per lesson:
 //
 //   git/<lessonId>/pack        the packfile bytes
-//   git/<lessonId>/refs.json   { head, size, updatedAt }
+//   git/<lessonId>/refs.json   { head, refs, size, updatedAt }
+//
+// `head` is the lesson itself — the default branch — and `refs` maps every branch
+// the repository holds to its tip, one per *variation* its author is trying out
+// (see /web-app/lesson-variations). `head` came first and is kept as its own
+// field rather than being read out of the map: everything that wants "the
+// lesson" wants exactly that one branch, and a reader written before variations
+// existed still gets the right answer from it.
 //
 // A pull request — someone proposing changes to a lesson they don't own — is a
 // packfile too, snapshotted when the request was opened, under its own key:
@@ -28,8 +35,19 @@ import { supabaseHeaders } from './supabase.js';
 // A lesson id is a UUID — pinned down because it's interpolated into an R2 key.
 export const LESSON_ID_RE = /^[0-9a-fA-F-]{36}$/;
 
-// A commit oid is a 40-char lowercase hex SHA-1.
-export const OID_RE = /^[0-9a-f]{40}$/;
+// A commit oid is a 40-char lowercase hex SHA-1, and a branch name is what the
+// editor is allowed to create. Both rules come from the shared module so the
+// Worker validates a push against exactly what the client validated it against —
+// the same arrangement as the pull-request length limits.
+export {
+	DEFAULT_BRANCH,
+	OID_RE,
+	MAX_BRANCHES,
+	MAX_BRANCH_NAME,
+	isBranchName,
+	parseRefMap,
+	serializeRefMap,
+} from '@spelling-creator/core/git/refs';
 
 // Cap one repository's history. Packs hold only JSON (images are referenced by
 // hash and live in the images bucket, not in the repo), so even a long-lived

apps/api/src/routes/git.js

@@ -6,8 +6,9 @@
 // it here the way git itself moves history: as a packfile holding every object
 // reachable from the lesson's branch, plus the commit its branch points at.
 //
-//   GET /git/:lessonId/refs   public* -> { head, size, updatedAt } | 404
-//   GET /git/:lessonId/pack   public* -> the packfile (X-Git-Head names its tip)
+//   GET /git/:lessonId/refs   public* -> { head, refs, size, updatedAt } | 404
+//   GET /git/:lessonId/pack   public* -> the packfile (X-Git-Head names its tip,
+//                                        X-Git-Refs every branch it holds)
 //   PUT /git/:lessonId/pack   Bearer   -> store it (the author, or a trusted collaborator)
 //
 // A pull request's proposed history is a packfile too, but it belongs to the
@@ -24,11 +25,24 @@
 //
 // Two R2 objects per lesson, mirroring the /images routes' use of the bucket:
 //   git/<lessonId>/pack        the packfile bytes
-//   git/<lessonId>/refs.json   { head, size, updatedAt }
+//   git/<lessonId>/refs.json   { head, refs, size, updatedAt }
 //
-// The pack carries its own tip in customMetadata (and in the X-Git-Head response
-// header). A clone therefore reads the head from the *same object* as the bytes,
-// so it can never pair a new refs.json with a stale pack.
+// The pack carries its own tip *and* its branch map in customMetadata (echoed in
+// the X-Git-Head and X-Git-Refs response headers). A clone therefore reads both
+// from the *same object* as the bytes, so it can never pair a new refs.json with
+// a stale pack.
+//
+// ---- More than one branch ---------------------------------------------------
+//
+// `head` is the lesson: the default branch, and the only one a reader, a forker
+// or the lesson's own page ever asks for. `refs` maps every branch the repository
+// holds to its tip — one per *variation* its author is trying out, kept here so a
+// variation follows the lesson between the author's devices rather than living
+// only in the browser it was started in.
+//
+// A variation is exactly as public as the lesson it belongs to: it is in the same
+// pack, and the pack of a published lesson is public so that forking is. See
+// /web-app/lesson-variations, which says so where an author can read it.
 //
 // ---- Who may push, and why it can't lose work -------------------------------
 //
@@ -48,20 +62,113 @@
 // with 409 and the client must fetch, merge, and retry. Since the client only
 // pushes a history that already *contains* the head it merged, an accepted push
 // can only ever move the lesson forward.
+//
+// The same rule runs one level down for the variations, per branch:
+//
+//   X-Git-Refs      the branches to set, `{ "<name>": "<oid>" }`
+//   X-Git-Expected  what the client believes we hold for each name it touches,
+//                   with "" meaning "I believe this one does not exist yet"
+//   X-Git-Deletes   the branches to remove, comma-separated
+//
+// It is all-or-nothing, which costs nothing to arrange: refs.json is a single
+// object and already the commit point, so every branch advances or none does. A
+// branch the client doesn't mention is left exactly as it is — that is what stops
+// a device which has never heard of a new variation from deleting it by omission,
+// and it is why a delete has to be asked for by name rather than inferred.
 
 import { bearerToken } from '../lib/auth.js';
 import { bannedResponse } from '../lib/bans.js';
 import { canReadLesson, fetchLessonRow, isTrustedCollaborator } from '../lib/lesson.js';
-import { LESSON_ID_RE, MAX_PACK_BYTES, OID_RE, isPackfile, packKey, refsKey } from '../lib/lessonGit.js';
+import {
+	DEFAULT_BRANCH,
+	LESSON_ID_RE,
+	MAX_BRANCHES,
+	MAX_PACK_BYTES,
+	OID_RE,
+	isBranchName,
+	isPackfile,
+	packKey,
+	parseRefMap,
+	refsKey,
+	serializeRefMap,
+} from '../lib/lessonGit.js';
 import { supabaseBase, supabaseConfigured, verifySupabaseUser } from '../lib/supabase.js';
 import { textResponse, jsonResponse } from '../lib/http.js';
 
-/** The head we currently hold for a lesson, or null if it has no history yet. */
-async function storedHead(env, lessonId) {
+/** What we currently hold for a lesson: `{ head, refs }`, or null with no history. */
+async function stored(env, lessonId) {
 	const object = await env.LESSON_GIT.get(refsKey(lessonId));
 	if (!object) return null;
-	const refs = await object.json().catch(() => null);
-	return refs && refs.head ? refs.head : null;
+	const value = await object.json().catch(() => null);
+	if (!value || !value.head) return null;
+
+	// A lesson stored before variations existed has no map; the one branch it has
+	// is its head, and saying so here means the rest of this file has one shape to
+	// reason about rather than two.
+	//
+	// A map that is *present but unreadable* is a different thing entirely, and must
+	// not be flattened into the same answer. Both of parseRefMap's limits — the
+	// branch count and the name rules — can be tightened later, and if either is,
+	// every lesson past the new limit would read as single-branch here: the push
+	// path would skip its old-client guard and write the variations away. So say we
+	// don't know, and let the caller refuse.
+	const refs = value.refs === undefined ? { [DEFAULT_BRANCH]: value.head } : parseRefMap(value.refs);
+	if (!refs) return { head: value.head, refs: null, unreadable: true };
+	return { head: value.head, refs };
+}
+
+/**
+ * Apply a push's ref instructions to what we hold, or explain why we won't.
+ *
+ * The rule is the one that has always guarded this endpoint, applied per branch
+ * rather than to the lesson as a whole: a client may only move a branch it has
+ * already seen the current state of. `expected` is what it believes, and a branch
+ * it doesn't mention at all is left untouched — which is what stops a device that
+ * has never heard of somebody's new variation from deleting it by omission.
+ *
+ * @returns {{ refs: object } | { error: string, status: number }}
+ */
+export function applyRefs(current, { refs, deletes, expected }) {
+	const next = { ...current };
+
+	const believes = (name) => (Object.hasOwn(expected, name) ? expected[name] : null);
+	const mismatch = (name) => {
+		const believed = believes(name);
+		// Nothing claimed about this branch: only safe when it is new to us. Moving
+		// one we already hold without saying what we hold is exactly the overwrite
+		// the compare-and-swap exists to refuse.
+		if (believed === null) return Boolean(current[name]);
+		return (current[name] || '') !== believed;
+	};
+
+	// A name in both halves is a request that contradicts itself, and we cannot know
+	// which half was meant. Refusing is the only honest answer — applying them in
+	// order would silently let the delete win, which is how a branch that is alive
+	// on the client disappears from the hub.
+	for (const name of deletes) {
+		if (Object.hasOwn(refs, name)) {
+			return { error: 'That push asks to both keep and remove the same version.', status: 400 };
+		}
+	}
+
+	for (const name of Object.keys(refs)) {
+		if (mismatch(name)) return { error: 'moved', status: 409 };
+		next[name] = refs[name];
+	}
+	for (const name of deletes) {
+		if (!isBranchName(name)) return { error: 'That is not a branch name.', status: 400 };
+		if (mismatch(name)) return { error: 'moved', status: 409 };
+		delete next[name];
+	}
+
+	// The lesson has to still be there afterwards. Deleting the branch that *is*
+	// the lesson would leave a row whose history advertises a tip nothing points
+	// at, and no client asks for that.
+	if (!next[DEFAULT_BRANCH]) return { error: 'The lesson’s own history cannot be removed.', status: 400 };
+	if (Object.keys(next).length > MAX_BRANCHES) {
+		return { error: `A lesson can have at most ${MAX_BRANCHES} versions.`, status: 400 };
+	}
+	return { refs: next };
 }
 
 /**
@@ -128,8 +235,13 @@ export async function handleGit(request, env, lessonId, rest, cors) {
 		headers.set('Cache-Control', 'no-store');
 		const head = object.customMetadata?.head || '';
 		if (head) headers.set('X-Git-Head', head);
-		// The SPA reads X-Git-Head cross-origin, which needs it explicitly exposed.
-		headers.set('Access-Control-Expose-Headers', 'X-Git-Head');
+		// The branch map comes off the *same object* as the bytes, for the reason the
+		// head does: a map read from refs.json a moment later could name a tip this
+		// pack doesn't contain.
+		const refs = object.customMetadata?.refs || '';
+		if (refs) headers.set('X-Git-Refs', refs);
+		// The SPA reads these cross-origin, which needs them explicitly exposed.
+		headers.set('Access-Control-Expose-Headers', 'X-Git-Head, X-Git-Refs');
 		if (object.httpEtag) headers.set('ETag', object.httpEtag);
 		return new Response(object.body, { status: 200, headers });
 	}
@@ -162,24 +274,91 @@ export async function handleGit(request, env, lessonId, rest, cors) {
 			return textResponse('Missing or invalid X-Git-Head.', 400, cors);
 		}
 
+		// What the client wants to happen to the lesson's branches. A client that
+		// sends none of this is one written before a lesson could have more than one
+		// branch: it means "move the lesson, leave everything else alone", and the
+		// two X-Git-Head/X-Git-Parent headers already say that.
+		const requested = parseRefMap(request.headers.get('X-Git-Refs'));
+		const believed = parseRefMap(request.headers.get('X-Git-Expected'));
+		if (request.headers.get('X-Git-Refs') && !requested) {
+			return textResponse('Invalid X-Git-Refs.', 400, cors);
+		}
+		if (request.headers.get('X-Git-Expected') && !believed) {
+			return textResponse('Invalid X-Git-Expected.', 400, cors);
+		}
+		// A ref being *set* must name a real commit; only X-Git-Expected may say ""
+		// (meaning "I believe this branch does not exist yet").
+		if (requested && Object.values(requested).some((oid) => !OID_RE.test(oid))) {
+			return textResponse('Invalid X-Git-Refs.', 400, cors);
+		}
+
+		const deletes = (request.headers.get('X-Git-Deletes') || '')
+			.split(',')
+			.map((name) => name.trim())
+			.filter(Boolean);
+
 		// Compare-and-swap. `parent` is the head the client merged before building
 		// this pack; if the lesson has moved on since (someone else pushed), we
 		// refuse — accepting would drop their commits. 409 tells the client to
 		// fetch, merge and try again.
 		const parent = (request.headers.get('X-Git-Parent') || '').trim();
-		const current = await storedHead(env, lessonId);
+		const held = await stored(env, lessonId);
+		const current = held?.head || null;
+
+		// We hold a branch map we can't read. Every path below decides what to keep
+		// and what to drop by comparing against it, so none of them can run safely.
+		if (held?.unreadable) {
+			return textResponse('This lesson’s stored history could not be read. Please report this rather than saving over it.', 409, cors);
+		}
+
 		if (current && parent !== current) {
 			return textResponse(
 				'This lesson’s history has moved on since you last synced. Merge the latest changes, then save again.',
 				409,
 				cors,
 			);
 		}
-		// Already there — the client is re-pushing a history we hold. Nothing to do.
-		if (current && current === head) {
+
+		// A pack holds every object every branch it advertises needs. An old client
+		// packs only the lesson's own branch, so accepting its pack while keeping a
+		// variation's tip in the map would leave that tip pointing at objects the
+		// stored pack no longer contains — a history that can't be cloned. Rather
+		// than silently drop the variation, refuse and say why.
+		const heldBranches = Object.keys(held?.refs || {});
+		if (!requested && heldBranches.length > 1) {
+			return textResponse(
+				'This lesson has more than one version, which this client cannot save without dropping them. Please update it, or save from the web editor.',
+				409,
+				cors,
+			);
+		}
+
+		const applied = applyRefs(held?.refs || {}, {
+			refs: requested || { [DEFAULT_BRANCH]: head },
+			deletes,
+			expected: believed || (current ? { [DEFAULT_BRANCH]: current } : {}),
+		});
+		if (applied.error) {
+			return textResponse(
+				applied.status === 409
+					? 'One of this lesson’s versions has moved on since you last synced. Merge the latest changes, then save again.'
+					: applied.error,
+				applied.status,
+				cors,
+			);
+		}
+		// The lesson's own tip and the map have to agree — they are two readings of
+		// one fact, and a client that disagrees with itself is one we can't apply.
+		if (applied.refs[DEFAULT_BRANCH] !== head) {
+			return textResponse('X-Git-Head and X-Git-Refs disagree about this lesson.', 400, cors);
+		}
+
+		// Already there — the client is re-pushing exactly what we hold, branches and
+		// all. Nothing to write.
+		if (current && current === head && serializeRefMap(applied.refs) === serializeRefMap(held.refs)) {
 			const object = await env.LESSON_GIT.get(refsKey(lessonId));
-			const refs = object ? await object.json().catch(() => null) : null;
-			if (refs) return jsonResponse(refs, 200, cors);
+			const existing = object ? await object.json().catch(() => null) : null;
+			if (existing) return jsonResponse(existing, 200, cors);
 		}
 
 		const declared = Number(request.headers.get('Content-Length') || 0);
@@ -201,11 +380,12 @@ export async function handleGit(request, env, lessonId, rest, cors) {
 		// Write the pack before the refs. The refs object is the commit point: if the
 		// pack write fails, refs still names the previous (complete) pack, and a
 		// clone of the old history beats a clone of a half-written one.
+		const map = serializeRefMap(applied.refs);
 		await env.LESSON_GIT.put(packKey(lessonId), bytes, {
 			httpMetadata: { contentType: 'application/x-git-packfile' },
-			customMetadata: { head },
+			customMetadata: { head, refs: map },
 		});
-		const refs = { head, size: bytes.byteLength, updatedAt: new Date().toISOString() };
+		const refs = { head, refs: applied.refs, size: bytes.byteLength, updatedAt: new Date().toISOString() };
 		await env.LESSON_GIT.put(refsKey(lessonId), JSON.stringify(refs), {
 			httpMetadata: { contentType: 'application/json' },
 		});