Enforce the lesson authoring standard on write - #34
Merged
Conversation
The standard has only ever been prose: server `instructions` (which the MCP spec makes optional, and which claude.ai's connector UI drops) plus a tool description the model may or may not act on. Every rule it states was one an author had to remember, and the ones that got broken in practice were the mechanical ones — a green answer that isn't in its own passage, a spelling word hiding inside an answer, the same word answering two questions. Split the standard in two. standards.js keeps the half that needs judgement: tone, difficulty, what makes a tight open easy. validate.js takes the half a script can decide and enforces it on every write path, so it holds even when the model never read a word of the standard. Errors reject the write; warnings ride along with a successful one. The line between them is whether a legitimate lesson could ever trip the check — an ungrounded answer is always a defect, a five-section lesson is occasionally exactly what the user asked for. `skipValidation: true` turns the errors off for the case where the user genuinely wants what the standard forbids. Two decisions worth calling out: patch_lesson validates before and after the edit and holds the caller only to the difference. Without that, a one-line tweak to a lesson written in the web editor would be blocked by defects the patch never touched and the assistant has no mandate to fix. Findings are keyed on the defect's identity rather than its message, so moving a section doesn't make every later finding look new. update_lesson gets no such exemption: it replaces the document, so it owns it. blockSchema is now .passthrough(). zod strips unknown keys, so `exampleAnswer` on an open question was being dropped before any handler saw it — the model was told nothing and kept sending it. It now reaches validation and comes back as E_OPEN_HAS_ANSWER. Also tightens the standard's own text: the orange-question traps (general knowledge belongs in a background question; don't paraphrase what the passage says) and the answer-reuse rule, which was "no 6+ letter word repeats across sections" and let GAS, ASH and ROCK through. It is now one answer word, one question, anywhere, at any length. validate.test.js is built around a six-section lesson written exactly to the standard that must produce nothing at all; the rest mutate it a rule at a time. A false positive here blocks an author who did nothing wrong, so that clean-lesson case is the one that matters most. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The icon was produced by a script that drew it in code. It is now a real image file checked into the repo, so the generator has no job left: make-icon.mjs is deleted along with the `icon` package script that ran it and the docs line telling packagers to regenerate before building. pack.mjs's missing-icon error no longer points at that script, since there is nothing to point at. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reviewer's GuideIntroduces a new validate.js module that enforces the mechanically checkable half of the lesson authoring standard on every write path (create, update, patch), wires it into MCP tools, and documents/testing to ensure validations return actionable errors/warnings while allowing deliberate bypass via skipValidation; also replaces the generated MCP bundle icon pipeline with a static icon and simplifies packaging scripts/docs. Sequence diagram for patch_lesson validation with baseline comparisonsequenceDiagram
actor Model
participant MCPServer
participant tools_patch_lesson as patch_lesson_handler
participant api as api
participant checkStandard
participant validateLesson
participant validateInput
participant newFindings
Model->>MCPServer: call patch_lesson { id, operations, skipValidation? }
MCPServer->>api: getLesson(id)
api-->>MCPServer: current_lesson
MCPServer->>tools_patch_lesson: applyPatch(current.doc, operations)
tools_patch_lesson->>checkStandard: { doc, rawBlocks: inputBlocksFromOperations(operations), skipValidation, baselineDoc: current.doc }
alt [skipValidation === true]
checkStandard-->>tools_patch_lesson: [] (no warnings, no errors)
else [skipValidation === false]
checkStandard->>validateLesson: doc
validateLesson-->>checkStandard: { errors, warnings }
checkStandard->>validateInput: rawBlocks
validateInput-->>checkStandard: input_errors
checkStandard->>newFindings: before.errors, [...input_errors, ...errors]
newFindings-->>checkStandard: failures
checkStandard->>newFindings: before.warnings, warnings
newFindings-->>checkStandard: flags
alt [failures.length > 0]
checkStandard->>checkStandard: validationErrorMessage(failures)
checkStandard-->>tools_patch_lesson: throw Error
tools_patch_lesson-->>MCPServer: isError result (no save)
else [no failures]
checkStandard-->>tools_patch_lesson: toWireWarnings(flags)
tools_patch_lesson->>api: updateLesson(id, { title, doc, published })
api-->>tools_patch_lesson: saved_lesson
tools_patch_lesson-->>MCPServer: { id, url, warnings }
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThe PR adds lesson validation for MCP lesson writes, integrates structured errors and warnings into write tools, documents validation behavior, vendors required core modules during packaging, and removes icon-generation support. ChangesLesson validation
MCP packaging
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant LessonWriteTool
participant validateLesson
participant LessonAPI
MCPClient->>LessonWriteTool: create, update, or patch lesson
LessonWriteTool->>validateLesson: validate raw input and document
validateLesson-->>LessonWriteTool: errors, warnings, or new patch findings
LessonWriteTool->>LessonAPI: save valid lesson
LessonAPI-->>MCPClient: saved payload and warnings
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![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 2 issues, and left some high level feedback:
- The validation module has grown quite large and mixes core rules, text normalisation helpers, wiring helpers, and formatting; consider splitting it into smaller focused files (e.g. grounding/shape checks vs. input/wiring helpers) to keep each piece easier to reason about and evolve.
validationErrorMessagecurrently throws a plainErrorwith a long formatted message string; introducing a dedicated error type carrying structured findings (codes, sections, messages) would make it easier for callers or future transports to render or process validation results differently without re-parsing text.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The validation module has grown quite large and mixes core rules, text normalisation helpers, wiring helpers, and formatting; consider splitting it into smaller focused files (e.g. grounding/shape checks vs. input/wiring helpers) to keep each piece easier to reason about and evolve.
- `validationErrorMessage` currently throws a plain `Error` with a long formatted message string; introducing a dedicated error type carrying structured findings (codes, sections, messages) would make it easier for callers or future transports to render or process validation results differently without re-parsing text.
## Individual Comments
### Comment 1
<location path="apps/mcp/test/validate.test.js" line_range="475-250" />
<code_context>
+ );
+});
+
+test("an open question carrying an answer is rejected from the raw input", () => {
+ const input = lessonInput();
+ question(input, 0, 8).exampleAnswer = "blue";
+ question(input, 1, 9).answer = "red";
+ const findings = validateInput(inputBlocksFromSections(input.sections));
+ assert.deepEqual(codes(findings), ["E_OPEN_HAS_ANSWER", "E_OPEN_HAS_ANSWER"]);
+ assert.match(findings[0].message, /`exampleAnswer`/);
+ assert.match(findings[1].message, /`answer`/);
+});
+
+test("shape deviations warn rather than block", () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding coverage for validateInput when the source is patch operations, not section-based input
Currently validateInput is only exercised via inputBlocksFromSections, covering section-based writes. Because patch_lesson uses inputBlocksFromOperations, please add a test that passes a patch operations array with an open question containing `answer`/`exampleAnswer` through inputBlocksFromOperations + validateInput, and verifies that E_OPEN_HAS_ANSWER is raised there as well. This will ensure all write paths consistently enforce the rule.
Suggested implementation:
```javascript
const {
validateInput,
inputBlocksFromSections,
inputBlocksFromOperations,
} = require("../src/validate");
```
```javascript
test("an open question carrying an answer is rejected from the raw input", () => {
const input = lessonInput();
question(input, 0, 8).exampleAnswer = "blue";
question(input, 1, 9).answer = "red";
const findings = validateInput(inputBlocksFromSections(input.sections));
assert.deepEqual(codes(findings), ["E_OPEN_HAS_ANSWER", "E_OPEN_HAS_ANSWER"]);
assert.match(findings[0].message, /`exampleAnswer`/);
assert.match(findings[1].message, /`answer`/);
});
test("an open question carrying an answer is rejected when sourced from patch operations", () => {
const input = lessonInput();
// Mirror the raw-input test by constructing patch operations that introduce
// exampleAnswer/answer on otherwise-open questions.
const operations = [
{
op: "update_question",
sectionIndex: 0,
questionIndex: 8,
question: {
...question(input, 0, 8),
exampleAnswer: "blue",
},
},
{
op: "update_question",
sectionIndex: 1,
questionIndex: 9,
question: {
...question(input, 1, 9),
answer: "red",
},
},
];
const findings = validateInput(inputBlocksFromOperations(operations));
assert.deepEqual(codes(findings), ["E_OPEN_HAS_ANSWER", "E_OPEN_HAS_ANSWER"]);
assert.match(findings[0].message, /`exampleAnswer`/);
assert.match(findings[1].message, /`answer`/);
});
test("shape deviations warn rather than block", () => {
```
The new test assumes the following:
1. `inputBlocksFromOperations` is exported from `../src/validate` alongside `validateInput` and `inputBlocksFromSections`. If the actual module path or export shape differs, adjust the import SEARCH/REPLACE block accordingly.
2. Patch operations for questions follow an `{ op: "update_question", sectionIndex, questionIndex, question }` shape. If your actual patch operation schema uses different field names (e.g. `sectionId`, `questionId`, `payload`), update the `operations` objects in the new test to match.
3. The spread `...question(input, 0, 8)` / `...question(input, 1, 9)` assumes `question()` returns a plain object representing the question. If `question()` returns something else (e.g. a wrapper or mutable reference), you may need to construct the question payload differently (for example, cloning or using the same helper you use in existing patch_lesson tests).
Once these adjustments are made to fit your actual operation format, the test will ensure that `validateInput` raises `E_OPEN_HAS_ANSWER` for the patch-based write path as well as the section-based one.
</issue_to_address>
### Comment 2
<location path="apps/mcp/test/validate.test.js" line_range="15-20" />
<code_context>
// checks (we never set the author; the Worker derives it from the token).
import { z } from "zod";
-import {
- buildDoc,
- buildLessonFile,
- lessonWarnings,
- QUESTION_TYPES,
-} from "./doc.js";
+import { buildDoc, buildLessonFile, QUESTION_TYPES } from "./doc.js";
import { applyPatch, findBlock } from "./patch.js";
import { searchWikimediaImages, resolveWikimediaImage } from "./wikimedia.js";
import { LESSON_STANDARDS } from "./standards.js";
+import {
+ inputBlocksFromOperations,
+ inputBlocksFromSections,
+ newFindings,
+ validateInput,
+ validateLesson,
+ validationErrorMessage,
+} from "./validate.js";
</code_context>
<issue_to_address>
**suggestion (testing):** formatFindings and validationErrorMessage are not directly tested and could benefit from targeted assertions
The helpers in validate.js (`formatFindings`, `validationErrorMessage`) are only exercised indirectly via error message smoke tests. Since they enforce the findings cap and compose the user-facing rejection text, it would be valuable to add direct tests that:
- Use a synthetic `Finding[]` exceeding the limit to verify truncation and the "…and N more" summary.
- Verify `validationErrorMessage` reports the total count, embeds the formatted findings, and preserves the skipValidation guidance.
This would better lock down the error messaging contract and make future changes safer.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| warnings.map((w) => w.message), | ||
| [], | ||
| ); | ||
| }); |
There was a problem hiding this comment.
suggestion (testing): Consider adding coverage for validateInput when the source is patch operations, not section-based input
Currently validateInput is only exercised via inputBlocksFromSections, covering section-based writes. Because patch_lesson uses inputBlocksFromOperations, please add a test that passes a patch operations array with an open question containing answer/exampleAnswer through inputBlocksFromOperations + validateInput, and verifies that E_OPEN_HAS_ANSWER is raised there as well. This will ensure all write paths consistently enforce the rule.
Suggested implementation:
const {
validateInput,
inputBlocksFromSections,
inputBlocksFromOperations,
} = require("../src/validate");test("an open question carrying an answer is rejected from the raw input", () => {
const input = lessonInput();
question(input, 0, 8).exampleAnswer = "blue";
question(input, 1, 9).answer = "red";
const findings = validateInput(inputBlocksFromSections(input.sections));
assert.deepEqual(codes(findings), ["E_OPEN_HAS_ANSWER", "E_OPEN_HAS_ANSWER"]);
assert.match(findings[0].message, /`exampleAnswer`/);
assert.match(findings[1].message, /`answer`/);
});
test("an open question carrying an answer is rejected when sourced from patch operations", () => {
const input = lessonInput();
// Mirror the raw-input test by constructing patch operations that introduce
// exampleAnswer/answer on otherwise-open questions.
const operations = [
{
op: "update_question",
sectionIndex: 0,
questionIndex: 8,
question: {
...question(input, 0, 8),
exampleAnswer: "blue",
},
},
{
op: "update_question",
sectionIndex: 1,
questionIndex: 9,
question: {
...question(input, 1, 9),
answer: "red",
},
},
];
const findings = validateInput(inputBlocksFromOperations(operations));
assert.deepEqual(codes(findings), ["E_OPEN_HAS_ANSWER", "E_OPEN_HAS_ANSWER"]);
assert.match(findings[0].message, /`exampleAnswer`/);
assert.match(findings[1].message, /`answer`/);
});
test("shape deviations warn rather than block", () => {The new test assumes the following:
inputBlocksFromOperationsis exported from../src/validatealongsidevalidateInputandinputBlocksFromSections. If the actual module path or export shape differs, adjust the import SEARCH/REPLACE block accordingly.- Patch operations for questions follow an
{ op: "update_question", sectionIndex, questionIndex, question }shape. If your actual patch operation schema uses different field names (e.g.sectionId,questionId,payload), update theoperationsobjects in the new test to match. - The spread
...question(input, 0, 8)/...question(input, 1, 9)assumesquestion()returns a plain object representing the question. Ifquestion()returns something else (e.g. a wrapper or mutable reference), you may need to construct the question payload differently (for example, cloning or using the same helper you use in existing patch_lesson tests).
Once these adjustments are made to fit your actual operation format, the test will ensure that validateInput raises E_OPEN_HAS_ANSWER for the patch-based write path as well as the section-based one.
Comment on lines
+15
to
+20
| import { | ||
| inputBlocksFromSections, | ||
| newFindings, | ||
| normalizeText, | ||
| validateInput, | ||
| validateLesson, |
There was a problem hiding this comment.
suggestion (testing): formatFindings and validationErrorMessage are not directly tested and could benefit from targeted assertions
The helpers in validate.js (formatFindings, validationErrorMessage) are only exercised indirectly via error message smoke tests. Since they enforce the findings cap and compose the user-facing rejection text, it would be valuable to add direct tests that:
- Use a synthetic
Finding[]exceeding the limit to verify truncation and the "…and N more" summary. - Verify
validationErrorMessagereports the total count, embeds the formatted findings, and preserves the skipValidation guidance.
This would better lock down the error messaging contract and make future changes safer.
`pnpm --filter @spelling-creator/mcp pack` has failed since 0c1a336 moved the Wikimedia plumbing into @spelling-creator/core. Staging copies package.json verbatim, so npm met `"@spelling-creator/core": "workspace:*"` and refused the whole install with EUNSUPPORTEDPROTOCOL. No bundle could be built, which also means the mcpb-release workflow would have failed on its next run. npm has never understood the `workspace:` protocol, and pointing it at a `file:` dependency wouldn't help either — npm symlinks those, and a symlink is the exact thing that doesn't survive the zip. So pack now takes the workspace deps out of the staged manifest before npm sees it, and copies the core modules the server actually imports into the staged node_modules as real files afterwards, with a manifest carrying just the exports it uses. Vendoring is cheap only because those modules are dependency-free: core's own list (yjs, docx, isomorphic-git, supabase) serves its browser modules and has no place in this bundle. That is an assumption, so pack checks it rather than trusting it — it walks the imports out from each module the server uses and fails the build, naming the package and the file, if any of them reaches a real dependency. The check runs before npm install, so a broken assumption costs a second instead of a full vendor cycle. Verified by copying the staged bundle outside the workspace, where no symlink can rescue it, and importing the server: it loads, and only richText.js and wikimedia.js are vendored. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/mcp/test/validate.test.js (1)
496-501: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the question mutation.
Line 499 assigns
undefinedtoanswer, and line 500 then deletes the property. The assignment has no effect on the result.♻️ Proposed simplification
- [questions[0].questionType, questions[0].answer] = ["open", undefined]; - delete questions[0].answer; + questions[0].questionType = "open"; + delete questions[0].answer;🤖 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/validate.test.js` around lines 496 - 501, In the reordered callback passed to check, simplify the first question mutation by removing the assignment of undefined to questions[0].answer and retain only the questionType update and subsequent property deletion.apps/docs/docs/mcp-server/lesson-validation.md (1)
88-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winState that
patch_lessonalso filters warnings against the baseline.
checkStandardinapps/mcp/src/tools.jsappliesnewFindingsto both errors and warnings whenbaselineDocis set. The section describes only the error behaviour, so a reader can expect pre-existing warnings to be returned bypatch_lesson.📝 Proposed wording
`patch_lesson` validates the lesson **before** and **after** the edit and holds the caller only to the difference. Without that, a one-line tweak to a lesson written in the web editor — or written before these rules existed — would be blocked by defects the patch never touched and the assistant may have no mandate to change. + +The same filter applies to warnings: `patch_lesson` reports only the warnings its edit +introduced, not the ones the lesson already carried.🤖 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/docs/docs/mcp-server/lesson-validation.md` around lines 88 - 97, Update the “Patching an existing lesson” section to state that patch_lesson compares both errors and warnings against the baseline, so pre-existing warnings are filtered out and only newly introduced findings are reported.apps/mcp/src/validate.js (1)
571-579: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHandle the same-section numeric duplicate wording.
If both
numberquestions in one section resolve to the same value, the message reads "section 1 and section 1". The defect is real, but the location text does not help the model correct it.♻️ Proposed message split
- error( - "E_NUMBER_DUPLICATE", - key, - ctx.number, - `The number ${answer} answers two different questions (section ${first} and section ${ctx.number}). ` + - "Every numeric answer in a lesson should be distinct — rework one of the problems so it lands on a " + - "different figure.", - ); + const place = + first === ctx.number + ? `twice within section ${ctx.number}` + : `in both section ${first} and section ${ctx.number}`; + error( + "E_NUMBER_DUPLICATE", + key, + ctx.number, + `The number ${answer} answers two different questions (${place}). ` + + "Every numeric answer in a lesson should be distinct — rework one of the problems so it lands on a " + + "different figure.", + );🤖 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/validate.js` around lines 571 - 579, The duplicate-number error handling in the numeric validation branch should use distinct wording when the earlier duplicate section matches ctx.number: report that two questions in the same section share the value instead of repeating “section N and section N”; retain the existing cross-section message when the sections differ.
🤖 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/mcp/src/standards.js`:
- Around line 143-147: Add W_NO_QUESTION to the “Flagged but allowed” warning
list in the standards text, alongside the existing non-blocking validation
warnings. Keep the wording and behavior of the other listed warning codes
unchanged.
In `@apps/mcp/src/tools.js`:
- Around line 240-296: Update newFindings and the finding-key construction used
by validateLesson so per-question findings include a stable question or block
identity alongside the code and normalized answer. Preserve the existing
section-renumbering stability, while ensuring identical findings on different
questions are not treated as pre-existing.
---
Nitpick comments:
In `@apps/docs/docs/mcp-server/lesson-validation.md`:
- Around line 88-97: Update the “Patching an existing lesson” section to state
that patch_lesson compares both errors and warnings against the baseline, so
pre-existing warnings are filtered out and only newly introduced findings are
reported.
In `@apps/mcp/src/validate.js`:
- Around line 571-579: The duplicate-number error handling in the numeric
validation branch should use distinct wording when the earlier duplicate section
matches ctx.number: report that two questions in the same section share the
value instead of repeating “section N and section N”; retain the existing
cross-section message when the sections differ.
In `@apps/mcp/test/validate.test.js`:
- Around line 496-501: In the reordered callback passed to check, simplify the
first question mutation by removing the assignment of undefined to
questions[0].answer and retain only the questionType update and subsequent
property deletion.
🪄 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: 33876da1-705b-4c24-9192-8695d9411300
⛔ Files ignored due to path filters (1)
apps/mcp/icon.pngis excluded by!**/*.png
📒 Files selected for processing (18)
apps/docs/docs/mcp-server/configuration.mdapps/docs/docs/mcp-server/development.mdapps/docs/docs/mcp-server/install-bundle.mdapps/docs/docs/mcp-server/lesson-validation.mdapps/docs/docs/mcp-server/packaging.mdapps/docs/docs/mcp-server/remote-mode.mdapps/docs/docs/mcp-server/setup.mdapps/docs/docs/mcp-server/tools.mdapps/docs/rspress.config.tsapps/mcp/package.jsonapps/mcp/scripts/make-icon.mjsapps/mcp/scripts/pack.mjsapps/mcp/src/doc.jsapps/mcp/src/standards.jsapps/mcp/src/tools.jsapps/mcp/src/validate.jsapps/mcp/test/smoke.test.jsapps/mcp/test/validate.test.js
💤 Files with no reviewable changes (2)
- apps/mcp/scripts/make-icon.mjs
- apps/mcp/package.json
…ones CodeRabbit caught a real hole in the baseline filter, and it is the kind that fails silently. Findings were keyed on their code and offending value alone, so a patch that added a genuinely new question carrying the same defect on the same word was written off as pre-existing and let through. Reproduced before fixing: a lesson with an ungrounded background answer "delta", plus a patch adding a second background question also answering "delta" in a different section, and newFindings reported nothing. Keys now carry the block's id — generated once and preserved through move_section, move_block and replace_block, so it identifies the defect without tying it to a position. Per-section findings use the section id for the same reason; they were keyed on the section *number*, which meant moving a section resurrected every warning it carried. Fixing that surfaced a second ordering bug the first fix would otherwise have introduced. A collision names two parties, and which one the walk reaches first depends on section order — so moving a section swapped the pair and rewrote the key of a defect nobody had touched. Pairs are now sorted before they become a key. The renumbering test covers both ends of this and now asserts on warnings as well as errors. Also from the review: - E_NUMBER_DUPLICATE read "section 1 and section 1" when both purple questions in one section landed on the same figure. The defect was real but the location text sent the author looking in the wrong place; same-section duplicates now say "both in section 1". - W_NO_QUESTION was missing from the warning list in the standards text. - The validation docs said only errors are filtered against the baseline; checkStandard filters warnings too, and the identity rules are now written down rather than left for the next reader to infer. - validateInput was only ever exercised through inputBlocksFromSections, so the patch path it also serves went untested. Covered now, including the operation labels a rejection quotes back. - formatFindings' 25-item cap and validationErrorMessage's wording had no direct tests despite being the contract the model reads. Covered. - Dropped a pointless destructuring assignment in a test. Not taken: splitting validate.js into smaller modules, and wrapping rejections in a typed error carrying structured findings. Reasons in the PR thread. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
Thanks both — one of these was a real bug that fails silently, so I reproduced it before touching anything. Fixed@coderabbitai — finding keys need block identity (tools.js comment). Correct, and worse than it looks. Keys were Keys now carry the block's id, which That fix exposed a second ordering bug it would otherwise have introduced: a collision names two parties, and which one the walk reaches first depends on section order, so moving a section swapped the pair and rewrote the key of an untouched defect. Pairs are sorted before becoming a key now. The renumbering test covers both ends and asserts on warnings too. @coderabbitai — @coderabbitai — @coderabbitai — docs didn't mention warnings are baseline-filtered. @sourcery-ai — @sourcery-ai — @coderabbitai — pointless destructuring in a test. Dropped. Not takenSplit A typed error carrying structured findings. 50 tests in |
|
Tip For best results, initiate chat on the files or code changes.
The stable block and section identities fix the silent The corrected 🐇 ✅ ✏️ Learnings added
You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/packaging.md`:
- Line 36: Update the fenced code block at the affected documentation section to
use the text language identifier, changing the opening fence to ```text so
markdownlint rule MD040 passes. Run the documentation formatting and linting
checks afterward.
🪄 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: 9a1488ca-690f-4861-bc72-f77550f2b623
📒 Files selected for processing (6)
apps/docs/docs/mcp-server/lesson-validation.mdapps/docs/docs/mcp-server/packaging.mdapps/mcp/scripts/pack.mjsapps/mcp/src/standards.jsapps/mcp/src/validate.jsapps/mcp/test/validate.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/mcp/src/standards.js
- apps/mcp/src/validate.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The authoring standard has only ever been prose. It reaches the model through the server's MCP
instructions— which the spec makes optional and which claude.ai's connector UI drops — plus a tool description the model may or may not act on. Every rule was one an author had to remember, and the ones broken in practice were the mechanical ones: a green answer that isn't in its own passage, a spelling word hiding inside an answer, the same word answering two questions.What
Splits the standard in two.
standards.jskeeps the half that needs judgement (tone, difficulty, what makes a tight open easy). Newvalidate.jstakes the half a script can decide and enforces it on every write path —create_lesson,create_lesson_file,update_lesson,patch_lesson— so it holds even when the model never read a word of the standard.Errors reject the write (13 codes: grounding, background-in-text, spelling length/duplicate/collision, answer reuse, numeric duplicates, open-with-answer, retired stem). Warnings ride along with a successful one (9 codes: section count, question shape/order, tight-vs-extended split, orange answer shape, spelling count, missing steps).
skipValidation: trueturns the errors off for the case where the user genuinely wants what the standard forbids.Rejections name the section, the offending value and the fix, because the model reads them and resubmits:
Decisions worth review
patch_lessonis judged only on what it changed. It validates before and after and reports the difference. Otherwise a one-line tweak to a lesson written in the web editor gets blocked by defects the patch never touched. Findings are keyed on the defect's identity, not its message, so moving a section doesn't make later findings look new.update_lessongets no exemption — it replaces the document, so it owns it.blockSchemais now.passthrough(). zod strips unknown keys, soexampleAnsweron an open question was dropped before any handler saw it — the model was told nothing and kept sending it. There's a test pinning this through a real MCP round trip, since removing the.passthrough()would otherwise regress it invisibly.E_NUMBER_DUPLICATEis a hard error. The source guide's enumerated error list omitted it, but its ordering section grouped numeric duplicates with the hard errors. Easy to demote to a warning.W_SPELLING_IN_CAPSis a warning, not an error. Acronyms (NASA, DNA) trip the caps extraction legitimately.Also tightens the standard's own text: the orange-question traps (general knowledge belongs in a background question; don't paraphrase what the passage says), and the answer-reuse rule — previously "no 6+ letter word repeats across sections", which let
GAS,ASHandROCKthrough. Now one answer word, one question, anywhere, at any length.Testing
validate.test.jsis built around a six-section lesson written exactly to the standard that must produce no errors and no warnings; every other case mutates that fixture one rule at a time. A false positive blocks an author who did nothing wrong, so the clean-lesson case is the one that matters most. 45 tests inapps/mcp, full workspace suite green, docs site builds.Two commits riding along
Replace the generated MCP bundle icon with a hand-made one— @playforge-coding's work, unrelated to the above. The icon is now a real image rather than one drawn by a script, somake-icon.mjsis deleted. I also removed the now-danglingiconentry inapps/mcp/package.jsonthat still pointed at the deleted file, and reformattedpack.mjs(the shortened error message fits on one line now).Fix the .mcpb build, broken since core became a runtime dependency— noticed while checking the icon change didn't break packaging. It didn't; packaging was already broken, and has been since0c1a336moved the Wikimedia plumbing into@spelling-creator/core. Staging copiespackage.jsonverbatim, so npm met"@spelling-creator/core": "workspace:*"and refused the whole install withEUNSUPPORTEDPROTOCOL. No bundle could be built — themcpb-releaseworkflow would have failed on its next run.npm has never understood
workspace:, andfile:wouldn't help either (npm symlinks those, and symlinks are exactly what doesn't survive the zip). Sopacknow strips workspace deps from the staged manifest before npm sees it, and copies the core modules the server actually imports into the stagednode_modulesas real files.That's only cheap because those modules are dependency-free — core's own list (yjs, docx, isomorphic-git, supabase) serves its browser modules.
packchecks that assumption rather than trusting it: it walks the imports out from each module the server uses and fails the build if any reaches a real package, naming both. I verified the guard fires by temporarily adding adompurifyimport torichText.js:Verified end-to-end by copying the staged bundle outside the workspace, where no symlink can rescue it, and importing the server — it loads, and only
richText.jsandwikimedia.jsare vendored.Not addressed
add_imagestill defaults to end-of-prose while the standard wants images atindex: 0. The tool description now tells the model to pass it explicitly, but changing the default is a behaviour change, not a validation one.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
skipValidationoption for supported writing tools.Bug Fixes
Chores