Let an assistant propose a lesson change instead of making it - #41
Conversation
An AI assistant connected over MCP could only write a lesson outright, which
left two things impossible. It could not touch a lesson somebody else wrote at
all — nobody may save over another person's lesson — and it could not offer
changes to your own lesson for you to look over first: patch_lesson overwrites
it, and there is nothing to review.
So give it the route a human already has. fork_lesson clones a lesson into a
private draft of its own, the assistant edits that with the ordinary tools, and
propose_changes offers the result back as a proposal, to be read and merged (or
declined) from the web app. The lesson is untouched until a person decides.
list_lesson_proposals is how the assistant finds out what they decided; merging
is deliberately not a tool, because it is theirs.
Two things had to move to make that possible.
The fork-and-propose flow only existed bound to LightningFS, so it ran in a
browser and nowhere else. The git engine already takes its filesystem through
{ fs, gitdir } for exactly this reason, so it needed a filesystem rather than a
rewrite: core/git/memfs.js is an in-memory node:fs, which is what lets this run
on stdio and inside the Worker alike. No repository is kept between calls — a
fork is a real hub lesson with its own stored pack, so each call clones that
pack, does one thing to it, and uploads the result. That survives a restart, a
conversation resumed days later, and a connection moving between instances.
And the Worker refused a proposal from a lesson's own author, on the grounds
that they could simply save. Over MCP the assistant acts as the account it is
signed in with, so that refusal fell on exactly the case worth having. It now
refuses only when there is nothing behind the request: a proposal carrying a
fork you own is allowed, because it means something specific — here is a copy
with changes in it, let me read the diff before it lands. A human gets the same
route via "fork into a new lesson". Since the proposer's name is then your own,
the proposal's body records which client wrote it and the notification reads
"Changes are waiting for your review" rather than naming somebody.
Tested against the real git engine rather than a mock of it: that a proposal's
packfile genuinely shares ancestry with the lesson it targets (without which a
reviewer's three-way merge has no base and the whole thing degrades to
"replace the lesson with mine"), that the target is untouched, that a failed
pack upload withdraws its proposal instead of leaving an empty one in someone's
queue, and that proposing twice stacks rather than colliding.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reviewer's GuideImplements a fork-and-propose workflow for MCP assistants by adding git packfile APIs, an in-memory filesystem-backed git engine, new MCP tools for forking lessons and proposing changes, and tightening pull request rules and notifications so assistants can propose reviewed changes instead of overwriting lessons directly. Sequence diagram for the fork-and-propose MCP workflowsequenceDiagram
actor User
participant Assistant
participant MCPServer as MCP_Server
participant GitEngine as Git_memfs_repo
participant HubAPI as Hub_API_Worker
actor Reviewer as Web_Reviewer
User->>Assistant: Request changes to lesson
%% 1) fork_lesson
Assistant->>MCPServer: fork_lesson(lessonId)
MCPServer->>HubAPI: api.getLesson(lessonId)
HubAPI-->>MCPServer: lesson
MCPServer->>HubAPI: api.fetchLessonPack(lessonId)
HubAPI-->>MCPServer: { packfile, head } | null
MCPServer->>GitEngine: cloneFromPack / commitDoc
GitEngine-->>MCPServer: packed { packfile, head }
MCPServer->>HubAPI: api.createLesson({ forkedFrom })
HubAPI-->>MCPServer: fork lesson
MCPServer->>HubAPI: api.pushLessonPack(fork.id, { packfile, head })
HubAPI-->>MCPServer: ok
MCPServer-->>Assistant: fork { id, head, clonedHistory }
%% 2) Assistant edits fork (patch_lesson etc.)
Assistant->>MCPServer: patch_lesson(id = fork.id, ...)
MCPServer->>HubAPI: api.patchLesson(...)
HubAPI-->>MCPServer: updated fork
%% 3) propose_changes
Assistant->>MCPServer: propose_changes({ forkLessonId })
MCPServer->>HubAPI: api.getLesson(forkLessonId)
HubAPI-->>MCPServer: fork with doc
MCPServer->>HubAPI: api.fetchLessonPack(forkLessonId)
HubAPI-->>MCPServer: { packfile, head }
MCPServer->>GitEngine: pendingOps / commitDoc
GitEngine-->>MCPServer: commit oid, packed { packfile, head }
MCPServer->>HubAPI: api.pushLessonPack(forkLessonId, { packfile, head, parent })
HubAPI-->>MCPServer: ok
MCPServer->>HubAPI: api.fetchLessonHead(targetLessonId)
HubAPI-->>MCPServer: base head | null
MCPServer->>HubAPI: api.createPull(targetLessonId, { title, body, head, base, sourceLessonId })
HubAPI-->>MCPServer: pull
MCPServer->>HubAPI: api.uploadPullPack(targetLessonId, pull.id, { packfile, head })
HubAPI-->>MCPServer: pull.ready
HubAPI-->>Reviewer: pull_request notification (link /hub/:id/proposals/:pullId)
MCPServer-->>Assistant: { proposalId, url, status, changes }
Assistant-->>User: Share proposal url for review
Reviewer->>HubAPI: Open proposal url and merge/decline
HubAPI-->>Reviewer: Result (lesson updated or unchanged)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe MCP server now supports lesson forks, Git-backed proposals, and proposal listing. The API validates fork provenance, supports own-lesson proposals, and sends proposal-specific notifications. Documentation and package manifests describe the new workflow. ChangesLesson proposal workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant forkLesson
participant createApi
participant Hub
participant proposeChanges
MCPClient->>forkLesson: request lesson fork
forkLesson->>createApi: fetch lesson content and history
createApi->>Hub: read lesson pack
Hub-->>MCPClient: return pack or no history
forkLesson->>createApi: create private lesson and push pack
MCPClient->>proposeChanges: submit fork changes
proposeChanges->>createApi: create proposal and upload pack
createApi->>Hub: store proposal pack
Hub-->>MCPClient: return proposal metadata and review URL
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoAdd MCP fork-and-propose workflow for lesson changes
AI Description
Diagram
High-Level Assessment
Files changed (16)
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
api.fetchLessonHead, all non-OK responses are treated asnull, which will silently swallow server-side errors; consider distinguishing 404/no-history from other error statuses so genuine failures are surfaced to callers. - Several user-facing error messages in
forkLesson/proposeChangesinclude rawerr.messagefrom lower layers in parentheses; you might want to standardise these to clearer, high-level messages to avoid leaking internal details into MCP-facing errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `api.fetchLessonHead`, all non-OK responses are treated as `null`, which will silently swallow server-side errors; consider distinguishing 404/no-history from other error statuses so genuine failures are surfaced to callers.
- Several user-facing error messages in `forkLesson`/`proposeChanges` include raw `err.message` from lower layers in parentheses; you might want to standardise these to clearer, high-level messages to avoid leaking internal details into MCP-facing errors.
## Individual Comments
### Comment 1
<location path="apps/mcp/test/fork.test.js" line_range="158-148" />
<code_context>
+ 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, {
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for the failure path when storing a fork's history fails in `forkLesson`.
Since the fake hub’s `pushLessonPack` always succeeds, we never exercise the error branch where history storage fails but the fork lesson row is kept. Please add a test that uses an API where `pushLessonPack` throws (e.g. simulating R2 issues) and asserts that:
- `forkLesson` rejects with an error message containing the fork lesson id and guidance to delete/refork, and
- the fork lesson row remains in `hub.lessons`.
This will lock in the intended UX and guard against regressions in this error handling path.
Suggested implementation:
```javascript
assert.equal(
head,
source.head,
"an unedited fork sits on the original's own commit, so nothing was rewritten",
);
});
test("forking keeps draft when history storage fails", async () => {
const hub = fakeHub();
const source = await seedLesson(hub, {
title: "Volcanoes",
text: "A volcano ERUPTS.",
});
// Create an API that behaves like the hub API but whose pushLessonPack fails.
const failingApi = {
...hub.api,
async pushLessonPack(...args) {
throw new Error("simulated R2 failure while storing fork history");
},
};
let error;
try {
await forkLesson(failingApi, { lessonId: source.id });
} catch (err) {
error = err;
}
// We expect forkLesson to reject with an error containing the fork lesson id
// and guidance for the user to delete/refork.
assert.ok(error instanceof Error, "forkLesson should reject when history storage fails");
assert.match(
error.message,
/fork lesson id/i,
"error message should mention the fork lesson id",
);
assert.match(
error.message,
/delete.*refork/i,
"error message should suggest deleting and reforking the lesson",
);
// Extract the fork lesson id from the error message (as emitted by forkLesson).
const forkLessonIdMatch = error.message.match(/fork lesson id\s*[:=]\s*([0-9a-f-]+)/i);
assert.ok(forkLessonIdMatch, "error message should contain a parseable fork lesson id");
const forkLessonId = forkLessonIdMatch[1];
// The fork lesson row should remain present in hub.lessons.
assert.ok(
hub.lessons.has(forkLessonId),
"the fork lesson row should remain in hub.lessons after a history storage failure",
);
});
```
1. Adjust the regular expressions in the `assert.match` and `error.message.match` calls to match the actual error message format that `forkLesson` produces (for example, if it uses a different phrase than "fork lesson id").
2. Ensure that `hub.lessons` exposes a `has(id)` method; if it is a plain object rather than a `Map`, replace `hub.lessons.has(forkLessonId)` with something like `hub.lessons[forkLessonId]` or `Object.hasOwn(hub.lessons, forkLessonId)` to align with the existing hub implementation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| doc, | ||
| published: true, | ||
| forkedFrom: null, | ||
| }); |
There was a problem hiding this comment.
suggestion (testing): Add a test for the failure path when storing a fork's history fails in forkLesson.
Since the fake hub’s pushLessonPack always succeeds, we never exercise the error branch where history storage fails but the fork lesson row is kept. Please add a test that uses an API where pushLessonPack throws (e.g. simulating R2 issues) and asserts that:
forkLessonrejects with an error message containing the fork lesson id and guidance to delete/refork, and- the fork lesson row remains in
hub.lessons.
This will lock in the intended UX and guard against regressions in this error handling path.
Suggested implementation:
assert.equal(
head,
source.head,
"an unedited fork sits on the original's own commit, so nothing was rewritten",
);
});
test("forking keeps draft when history storage fails", async () => {
const hub = fakeHub();
const source = await seedLesson(hub, {
title: "Volcanoes",
text: "A volcano ERUPTS.",
});
// Create an API that behaves like the hub API but whose pushLessonPack fails.
const failingApi = {
...hub.api,
async pushLessonPack(...args) {
throw new Error("simulated R2 failure while storing fork history");
},
};
let error;
try {
await forkLesson(failingApi, { lessonId: source.id });
} catch (err) {
error = err;
}
// We expect forkLesson to reject with an error containing the fork lesson id
// and guidance for the user to delete/refork.
assert.ok(error instanceof Error, "forkLesson should reject when history storage fails");
assert.match(
error.message,
/fork lesson id/i,
"error message should mention the fork lesson id",
);
assert.match(
error.message,
/delete.*refork/i,
"error message should suggest deleting and reforking the lesson",
);
// Extract the fork lesson id from the error message (as emitted by forkLesson).
const forkLessonIdMatch = error.message.match(/fork lesson id\s*[:=]\s*([0-9a-f-]+)/i);
assert.ok(forkLessonIdMatch, "error message should contain a parseable fork lesson id");
const forkLessonId = forkLessonIdMatch[1];
// The fork lesson row should remain present in hub.lessons.
assert.ok(
hub.lessons.has(forkLessonId),
"the fork lesson row should remain in hub.lessons after a history storage failure",
);
});- Adjust the regular expressions in the
assert.matchanderror.message.matchcalls to match the actual error message format thatforkLessonproduces (for example, if it uses a different phrase than "fork lesson id"). - Ensure that
hub.lessonsexposes ahas(id)method; if it is a plain object rather than aMap, replacehub.lessons.has(forkLessonId)with something likehub.lessons[forkLessonId]orObject.hasOwn(hub.lessons, forkLessonId)to align with the existing hub implementation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
packages/core/src/git/memfs.test.js (2)
30-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtend the contract test to
chmod.The method list omits
chmod, whichmemFsimplements and isomorphic-git binds. Add it so a future removal ofchmodfails this test.♻️ Proposed addition
"readlink", "symlink", + "chmod", ]) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/git/memfs.test.js` around lines 30 - 43, Extend the method list in the memFs contract test to include "chmod", ensuring fs.promises.chmod is validated as a function alongside the existing filesystem methods.
150-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive assertion to the isolation test.
The test asserts only that repo
bhas no head. IfcommitDocon repoafailed silently, the test would still pass. Assert thatahas a head, so the test proves isolation instead of absence of work.💚 Proposed assertion
const a = memRepo(); const b = memRepo(); - await commitDoc({ ...a, doc: doc("A", "a"), author }); + const committed = await commitDoc({ ...a, doc: doc("A", "a"), author }); + expect(await headOid(a)).toBe(committed.oid); expect(await headOid(b)).toBeNull();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/git/memfs.test.js` around lines 150 - 155, Update the “keeps two in-memory repos out of each other’s way” test to assert that headOid(a) is non-null after committing to repo a, while preserving the existing null assertion for repo b.apps/mcp/test/fork.test.js (2)
150-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
packRepostatically with the other pack helpers.
cloneFromPack,contains, andmergeBasecome from a static import at lines 17-21.packRepouses a dynamic import insideseedLesson, which repeats the module resolution on every seed and splits one module's imports across two styles.♻️ Proposed change
@@ imports import { cloneFromPack, contains, mergeBase, + packRepo, } from "`@spelling-creator/core/git/pack`";const ctx = memRepo("seed"); const first = await commitDoc({ ...ctx, doc, author: AUTHOR }); - const { packRepo } = await import("`@spelling-creator/core/git/pack`"); const packed = await packRepo(ctx);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mcp/test/fork.test.js` around lines 150 - 156, Move packRepo into the existing static import alongside cloneFromPack, contains, and mergeBase, then remove the dynamic import from seedLesson while preserving its current usage.
250-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the client name reaches the created proposal.
The test passes
client: "Claude Desktop"but asserts nothing about it. Recording the MCP client name on the proposal is a stated objective of this PR, andproposalBodyis only tested in isolation at lines 403-413. Assert thatresult.pull.bodynames the client, so the wiring fromproposeChangesthroughcreatePullstays covered.💚 Proposed assertion
assert.equal(result.pull.head, result.commit); + assert.match( + result.pull.body, + /Claude Desktop/, + "the proposal records which client opened it", + ); assert.deepEqual(result.changes, ["- edit text block b1 (text)"]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mcp/test/fork.test.js` around lines 250 - 270, Extend the assertions for the result returned by proposeChanges to verify that result.pull.body includes the passed client name, "Claude Desktop". Keep the existing proposal and change assertions unchanged, covering the wiring through createPull rather than only testing proposalBody in isolation.packages/core/src/git/memfs.js (2)
100-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
putassigns a new inode on every write.
putrunsnextIno++for each call, so overwriting an existing file changes itsino.node:fskeeps the inode stable across writes. Bare repositories have no index, so isomorphic-git's stat cache is not consulted here and the behaviour is safe today. Preserving an existing inode would keep the emulation faithful if a caller later usesfswith a working tree.♻️ Proposed inode preservation
function put(path, node) { - nodes.set(path, { ino: nextIno++, mtimeMs: now(), ...node }); + const existing = nodes.get(path); + nodes.set(path, { + ino: existing?.ino ?? nextIno++, + mtimeMs: now(), + ...node, + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/git/memfs.js` around lines 100 - 102, Update put so overwriting an existing path preserves its current ino, while newly inserted paths continue receiving nextIno++. Keep the existing mtimeMs refresh and node merge behavior unchanged.
147-159: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider copying bytes on write and on read.
toBytesreturns the caller'sUint8Arrayunchanged, andreadFilereturns the stored array itself. The volume therefore shares memory with its callers. If any caller reuses or mutates a buffer after a write, or mutates the array it read, the stored object silently changes. LightningFS does not expose this aliasing. A copy on write keeps the volume authoritative for a small allocation cost.♻️ Proposed defensive copy on write
put(full, { type: "file", - data: toBytes(data), + data: new Uint8Array(toBytes(data)), mode: options?.mode ?? existing?.mode ?? FILE_MODE, });Also applies to: 138-145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/git/memfs.js` around lines 147 - 159, Update memfs writeFile and readFile to defensively copy Uint8Array data at both storage and retrieval boundaries. Ensure put stores a new byte array rather than the caller-owned result of toBytes, and readFile returns a separate copy so callers cannot mutate volume state.apps/mcp/src/api.js (1)
222-298: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider one shared helper for the "unwrap a named field" pattern.
listPulls,createPull,uploadPullPack, andclosePulleach repeatdata.X || fallback. A smallpullOf(data)helper would remove four near-identical unwraps. This is optional and does not change behaviour.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mcp/src/api.js` around lines 222 - 298, Optionally add a shared pullOf(data) helper for extracting the pull field with the existing null fallback, then reuse it in listPulls, createPull, uploadPullPack, and closePull without changing their current behavior or validation.apps/mcp/src/git.js (1)
63-69: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake
clampsafe for a non-positive limit.
proposalBodyderives the limit asPULL_BODY_MAX - note.length - 2. If that value is ever non-positive,text.slice(0, limit - 1)uses a negative index and keeps almost the whole string, so the result can exceedPULL_BODY_MAXand the hub rejects the proposal with a 400. A single lower bound removes the case.♻️ Proposed refactor
function clamp(value, limit) { const text = (value || "").trim(); - if (text.length <= limit) return text; + const max = Math.max(1, limit); + if (text.length <= max) return text; - const cut = text.slice(0, limit - 1); + const cut = text.slice(0, max - 1); const space = cut.lastIndexOf(" "); - return `${(space > limit * 0.8 ? cut.slice(0, space) : cut).trimEnd()}…`; + return `${(space > max * 0.8 ? cut.slice(0, space) : cut).trimEnd()}…`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mcp/src/git.js` around lines 63 - 69, Update clamp to handle non-positive limit values before slicing, returning an empty string (or the established minimal result) when the limit is zero or below; preserve the existing truncation and ellipsis behavior for positive limits so proposalBody remains within PULL_BODY_MAX.apps/api/src/routes/pulls.js (1)
303-329: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winValidate
sourceLessonIdprovenance before allowing a self-lesson proposal. An owned, unrelated lesson bypasses the self-proposal guard. Includeforked_frominfetchLessonRowand requiresource.forked_from === lessonIdbefore settingsourceLessonId.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/pulls.js` around lines 303 - 329, Update the source-lesson validation before the self-lesson guard to fetch fork provenance via fetchLessonRow, including forked_from in its selected fields. Only set sourceLessonId when the source is owned by user.id and source.forked_from equals lessonId, while preserving the existing format, non-self, and invalid-value checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/docs/docs/mcp-server/tools.md`:
- Around line 7-22: Add a create_lesson_file row to the tool table alongside the
other lesson creation tools, describing its registered file-based lesson
creation behavior. Ensure the table matches the server registration and the
existing prose reference without changing unrelated entries.
In `@apps/docs/docs/web-app/notifications.md`:
- Around line 31-36: Update the notification documentation around the proposal
link description to accurately distinguish initial pull_request notifications
from merged and closed notifications, which link to /hub/${lessonId};
alternatively, update those notification paths to link to the proposal
consistently. Keep the documented behavior aligned with the implemented
notification link targets.
In `@apps/mcp/src/git.js`:
- Around line 252-293: Handle a null result from commitDoc before calling
packRepo, pushLessonPack, createPull, or uploadPullPack, and return the existing
no-op outcome or throw the appropriate validation error before any remote write.
Keep the normal flow unchanged for a non-null commit, including using commit.oid
in the successful result.
In `@apps/mcp/src/tools.js`:
- Around line 635-639: Update the title schema in proposeChanges to require at
least one character by adding the minimum-length validation to the existing
z.string() definition, while preserving its current description and handling of
non-empty titles.
- Line 1015: Update SERVER_INFO.version from 0.2.0 to 0.3.0 so it matches the
versions declared in apps/mcp/package.json and apps/mcp/manifest.json, keeping
all three version values aligned.
---
Nitpick comments:
In `@apps/api/src/routes/pulls.js`:
- Around line 303-329: Update the source-lesson validation before the
self-lesson guard to fetch fork provenance via fetchLessonRow, including
forked_from in its selected fields. Only set sourceLessonId when the source is
owned by user.id and source.forked_from equals lessonId, while preserving the
existing format, non-self, and invalid-value checks.
In `@apps/mcp/src/api.js`:
- Around line 222-298: Optionally add a shared pullOf(data) helper for
extracting the pull field with the existing null fallback, then reuse it in
listPulls, createPull, uploadPullPack, and closePull without changing their
current behavior or validation.
In `@apps/mcp/src/git.js`:
- Around line 63-69: Update clamp to handle non-positive limit values before
slicing, returning an empty string (or the established minimal result) when the
limit is zero or below; preserve the existing truncation and ellipsis behavior
for positive limits so proposalBody remains within PULL_BODY_MAX.
In `@apps/mcp/test/fork.test.js`:
- Around line 150-156: Move packRepo into the existing static import alongside
cloneFromPack, contains, and mergeBase, then remove the dynamic import from
seedLesson while preserving its current usage.
- Around line 250-270: Extend the assertions for the result returned by
proposeChanges to verify that result.pull.body includes the passed client name,
"Claude Desktop". Keep the existing proposal and change assertions unchanged,
covering the wiring through createPull rather than only testing proposalBody in
isolation.
In `@packages/core/src/git/memfs.js`:
- Around line 100-102: Update put so overwriting an existing path preserves its
current ino, while newly inserted paths continue receiving nextIno++. Keep the
existing mtimeMs refresh and node merge behavior unchanged.
- Around line 147-159: Update memfs writeFile and readFile to defensively copy
Uint8Array data at both storage and retrieval boundaries. Ensure put stores a
new byte array rather than the caller-owned result of toBytes, and readFile
returns a separate copy so callers cannot mutate volume state.
In `@packages/core/src/git/memfs.test.js`:
- Around line 30-43: Extend the method list in the memFs contract test to
include "chmod", ensuring fs.promises.chmod is validated as a function alongside
the existing filesystem methods.
- Around line 150-155: Update the “keeps two in-memory repos out of each other’s
way” test to assert that headOid(a) is non-null after committing to repo a,
while preserving the existing null assertion for repo b.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bc51f9d7-fffc-483a-b0cd-d02f8144f15f
📒 Files selected for processing (16)
apps/api/src/routes/pulls.jsapps/docs/docs/mcp-server/overview.mdapps/docs/docs/mcp-server/tools.mdapps/docs/docs/monorepo/version-history.mdapps/docs/docs/web-app/notifications.mdapps/docs/docs/web-app/pull-requests.mdapps/mcp/manifest.jsonapps/mcp/package.jsonapps/mcp/src/api.jsapps/mcp/src/git.jsapps/mcp/src/tools.jsapps/mcp/test/fork.test.jsapps/mcp/test/smoke.test.jspackages/core/package.jsonpackages/core/src/git/memfs.jspackages/core/src/git/memfs.test.js
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Three bots, ten findings. The four that were real bugs: A proposal pushed the fork's history *before* opening the request, so any failure after that point left the fork's document equal to its own history — the changes safe, but the retry finding nothing pending and refusing it. The push is bookkeeping (a proposal's pack is stored with the proposal, and that is what a reviewer merges), so it now happens last and cannot fail the call; a failed proposal leaves the fork untouched and the retry simply works. commitDoc returns null when the tree is unchanged, and pendingOps answers the looser question of whether the *documents* differ, so the two can disagree. The null was then dereferenced in the return value — after the proposal had gone live, reporting failure for something that had succeeded. It fails before anything is sent instead. Forking read the source document before its history. A lesson being saved in the browser writes those in the other order, so that pairing could put stale content on top of newer commits and quietly revert the save it raced. Reading the history first makes the bad pairing unreachable. And openPull accepted any lesson the caller owned as a proposal's source, while the source is the one thing that unlocks a self-proposal — so citing an unrelated lesson of your own turned the rule off entirely and recorded a fork link to something that wasn't one. It now requires the source to be forked from this lesson, which is what the documentation already claimed. The rest: require a non-empty proposal title, since the hub rejects an empty one only after the whole snapshot has been built and sent; align SERVER_INFO with the package and manifest version, which clients actually display; keep a file's inode when it is rewritten, and state the by-reference contract memfs departs on; bound a client's self-reported name before it goes in a proposal; and let fetchLessonHead throw on a real failure rather than reporting "no history". The tool table was also missing create_lesson_file, and the notification wording claimed every pull_request link opens the proposal when merged and closed ones open the lesson. Tests for the paths that had none: a fork whose history can't be stored, a failed proposal retried without further edits, a proposal surviving a failed history push, the fork read ordering, and that the client name reaches the body a reviewer reads. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — worked through all ten findings in c3f0a21. Four were real bugs. Fixed (bugs)Failed proposals consumed the changes (@qodo-code-review #1). The fork's history was pushed before the request was opened, so any failure after that left the fork's document equal to its own history — the changes safe, but the retry finding nothing pending and refusing. Since a proposal's pack is stored with the proposal, and that's what a reviewer merges, the push is bookkeeping: it now happens last and can't fail the call (reported as
Fork read a stale document over newer history (@qodo-code-review #2). Confirmed against
Self-proposal guard was bypassable (@qodo-code-review #4). Right, and it contradicted the docs I'd written ("a fork you own"): ownership alone was satisfied by any other lesson, which turned the rule off entirely and attached a fork link to a non-fork. Now requires Fixed (the rest)
New testsThe paths that had none: a fork whose history can't be stored (row kept, clear error), a failed proposal retried with no further edit, a proposal surviving a failed history push, the fork read ordering, and that the client name reaches the body a reviewer actually reads. SkippedCopy bytes on read/write in A shared "unwrap a named field" helper in Raw All checks still green: 69 core + 64 MCP + 27 API tests, lint/format clean, docs build, |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline c3f0a21
printf '\n--- changed files ---\n'
git diff-tree --no-commit-id --name-only -r c3f0a21
printf '\n--- relevant symbol outlines ---\n'
ast-grep outline apps/mcp/src/git.js --items all --match 'forkLesson|proposeChanges|commitDoc|clamp|proposalBody' || true
ast-grep outline apps/api/src/routes/pulls.js --items all --match 'source|forked_from|create' || true
ast-grep outline packages/core/src/git/memfs.js --items all --match 'memFs|put|readFile|writeFile' || true
printf '\n--- focused diff ---\n'
git show --format=fuller --find-renames --find-copies c3f0a21 -- \
apps/mcp/src/git.js \
apps/mcp/test/fork.test.js \
apps/api/src/routes/pulls.js \
packages/core/src/git/memfs.js \
packages/core/src/git/memfs.test.js \
apps/mcp/src/api.js \
apps/mcp/src/tools.jsLength of output: 27934
The The proposal flow now uploads the proposal pack before it updates fork history. A proposal creation failure leaves the fork history unchanged. The new retry test covers this case. The fork flow now reads history before it reads the lesson document. The self-proposal check now requires The I also confirmed focused regression tests for the new workflow paths. I did not independently run the reported checks. 🐇 ✅ ✏️ Learnings added
You are interacting with an AI system. |
Thanks for the detailed update. The described fixes address the self-proposal validation and retry-consumption bugs, and the ordering/cleanup rationale is sound. One ordinal correction: in the Qodo index, these are finding 1 (self-proposal guard) and finding 2 (failed proposals consuming changes); finding 4 is the stale server version. The stale document/history race is tracked as finding 3. The reported tests and checks are encouraging. I can’t independently verify the commit or CI results here, and the review index still shows findings 1–4 as active until their statuses are refreshed. |
1 participant
An AI assistant connected over MCP can only write a lesson outright, which leaves two things impossible: it can't touch a lesson somebody else wrote (nobody may save over another person's lesson), and it can't offer changes to your own lesson for you to look over first —
patch_lessonoverwrites it, so there's nothing to review.This gives it the route a human already has.
The lesson is untouched until a person merges it from the Proposals tab. Merging is deliberately not a tool — it happens in the web app under the reviewer's own credentials, because it's their decision. The tool descriptions steer the model to fork-and-propose when the user wants to review the work, and to
patch_lessonfor a correction they asked for outright.Two things had to move
The flow only existed in a browser.
browser/git/sync.jsis bound to LightningFS. The git engine already takes its filesystem through{ fs, gitdir }for exactly this reason, so this needed a filesystem rather than a rewrite:core/git/memfs.jsis an in-memorynode:fs, which is what lets the same code run on stdio and inside the Worker.No repository is kept between calls. A fork is a real hub lesson with its own stored pack, so each call clones that pack, does one thing to it, and uploads the result — which survives a restart, a conversation resumed days later, and a connection moving between Worker instances.
The Worker refused a proposal from a lesson's own author, on the grounds that they could simply save. Over MCP the assistant acts as the account it's signed in with, so that refusal landed on exactly the case worth having. It now refuses only when there's nothing behind the request: a proposal carrying a fork you own is allowed, because it means something specific — here is a copy with changes in it, let me read the diff before it lands. A human gets the same route via "fork into a new lesson" in the editor.
Since the proposer's name is then your own, the proposal's body records which MCP client wrote it (
getClientVersion()— "Claude Desktop", "claude.ai", …) and the notification reads "Changes are waiting for your review" rather than naming somebody.Review notes
patch_lessoncalls aren't separate commits — nothing is watching to record them — so the reviewer gets one clean diff. Documented in the tool description and the docs.forkLessoncommits the fork's document over the cloned head if the two disagree (a lesson edited over MCP is saved without committing, so its stored pack can lag). A no-op in the normal case, and it keeps the later proposal's diff to just the assistant's changes.uploadImageonto the same token-refresh helper as the new binary calls, which removed the duplicated retry logic inapi.js.Verified
pnpm run fmt && pnpm run lintclean; docs site builds.wrangler deploy --dry-runbuilds; the Worker bundle grows 1.42 → 1.50 MB gzipped from pulling in isomorphic-git.AGENTS.md:apps/mcppackage + manifest to 0.3.0,SERVER_INFOto 0.2.0.Not verified: the round trip against the live hub. The flow is tested against a fake that enforces the Worker's pack compare-and-swap and its "pack must match the head it was opened with" rule, but no real request has been made.
🤖 Generated with Claude Code
Summary by Sourcery
Add a fork-and-propose workflow so MCP-connected assistants can offer lesson changes for human review instead of editing lessons directly, including lessons authored by others or by the requesting user.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit