Add docs validation script#2
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7d4909dee
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (launchesDotnetProject) { | ||
| const projectIndex = args.indexOf('--project'); | ||
| const noBuildIndex = args.indexOf('--no-build'); | ||
| if (noBuildIndex === -1 || noBuildIndex > projectIndex) { |
There was a problem hiding this comment.
Allow valid dotnet option ordering
When a host snippet uses the valid dotnet run --project app.csproj --no-build -- mcp serve ordering, this validator reports a failure even though --no-build is still a dotnet run option before the -- application-argument separator. Microsoft Learn's dotnet run docs list both --no-build and --project as run options and state that recognized options are consumed before -- (https://learn.microsoft.com/en-us/dotnet/core/tools/dotnet-run), so this should check that --no-build appears before -- rather than before --project.
Useful? React with 👍 / 👎.
carldebilly
left a comment
There was a problem hiding this comment.
Good idea overall — this encodes the invariants from the recent MCP docs reviews (--no-build, repl-contacts-sample, VS Code servers shape, Repl.Mcp wording) as executable checks. One logic error and a couple of coordination notes below.
Cross-repo note: this validator consecrates --no-build + a prior dotnet build as the canonical host-config form. The yllibed/repl repo (PR #37) currently documents plain dotnet run in the sample README and configs/*.json — those should be aligned to the same form, otherwise the repo and the website will document two different shapes for the same sample.
| if (launchesDotnetProject) { | ||
| const projectIndex = args.indexOf('--project'); | ||
| const noBuildIndex = args.indexOf('--no-build'); | ||
| if (noBuildIndex === -1 || noBuildIndex > projectIndex) { |
There was a problem hiding this comment.
This ordering constraint is stricter than dotnet run actually requires: options can appear in any order before the -- separator, so ["run", "--project", "x.csproj", "--no-build", "--", "mcp", "serve"] is perfectly valid yet fails this check with a confusing message. The real invariant is presence before the -- separator (after --, --no-build would be an app argument, not a dotnet option):
const dashDashIndex = args.indexOf('--');
const noBuildIndex = args.indexOf('--no-build');
if (noBuildIndex === -1 || (dashDashIndex !== -1 && noBuildIndex > dashDashIndex)) {
fail(...);
}| fail(file, 1, 'Describe Repl.Mcp as the component used to build MCP servers, not as the MCP server itself.'); | ||
| } | ||
|
|
||
| if (file.endsWith('cookbook/mcp-server.mdx') && text.includes('## Agent-host setup')) { |
There was a problem hiding this comment.
Sequencing hazard with PR #1: the ## Agent-host setup section this branch validates only exists in PR #1, which as currently written uses "contacts-sample", has no "--no-build", and no dotnet build samples/08-mcp-server/McpServerSample.csproj instruction. Each PR passes in isolation because these checks are conditional, but once both are merged (and CI is wired), docs:validate will fail on main. PR #1 needs to be updated to satisfy these invariants before or together with this one.
|
|
||
| function validateMcpHostDocs(file, text) { | ||
| if (/Repl\.Mcp\s+is\s+(?:an?|the)\s+MCP server/i.test(text)) { | ||
| fail(file, 1, 'Describe Repl.Mcp as the component used to build MCP servers, not as the MCP server itself.'); |
There was a problem hiding this comment.
Minor: this reports the violation at line 1 instead of the offending line, while findLine exists and is used by the other checks — fail(file, findLine(text, 'Repl.Mcp'), ...) would point reviewers at the actual sentence.
| } | ||
|
|
||
| if (inJson) buffer.push(line); | ||
| } |
There was a problem hiding this comment.
Minor: a ```json fence left unclosed at EOF is silently dropped (the buffered block is never pushed), so a malformed doc skips validation instead of failing it. Consider flushing (or failing) when inJson is still true after the loop.
Summary
npm run docs:validatefor Repl/MCP documentation invariants.dotnet runhost launches and VS Codeserversshape requirements.Verification
npm run docs:validate— passed, 37 files checked.git diff --check— passed.ASTRO_TELEMETRY_DISABLED=1 XDG_CONFIG_HOME=/workspace/.config npm run astro -- build— passed, 36 pages built.Follow-up needed for CI wiring
I attempted to include the read-only PR workflow and to remove the daily Pages deploy schedule, but the current
autocarltoken only has thereposcope. GitHub rejected pushes that modify.github/workflows/*without theworkflowscope:So this PR intentionally contains the script only. With a token/account that has the
workflowscope, the CI wiring should add this workflow:Daily deployment note
The daily deployment is caused by
.github/workflows/deploy.yml:If daily deploys are not useful, remove that
scheduleblock. Deploys will still happen onpushtomain,workflow_dispatch, andrepository_dispatchfrom Repl releases.