ci: verify documented npm commands exist (and drop two that never did) - #249
Open
joeVenner wants to merge 2 commits into
Open
ci: verify documented npm commands exist (and drop two that never did)#249joeVenner wants to merge 2 commits into
joeVenner wants to merge 2 commits into
Conversation
fly/README.md documents `npm run smoke:fly` and `npm run smoke:x`, and env vars X_SMOKE_REQUIRE_AUTH / X_SMOKE_REQUIRE_FIREHOSE that go with them. Neither script is defined in any package.json, and `git log -S` shows neither has ever been defined in this repo's history. The two env vars appear nowhere outside this file. There is no corresponding scripts/fly-smoke.ts either, so the paragraph describing what "the smoke test" backs up and restores documents a run nobody can perform. Removed rather than corrected because there is no correct name to point at. The `npm run smoke:git-cli` block below is real and is left alone.
The stale `npm run smoke:fly` / `npm run smoke:x` blocks removed in the previous commit sat in fly/README.md indefinitely because nothing checks that a command shown in the docs is a command you can run. Every other class of rot here has a gate — knip for dead code, oxlint and eslint for source, prettier for formatting — so add the equivalent for documented commands and run it in the lint job alongside them. The check reads every tracked *.md, extracts each `npm run <script>`, and fails if no tracked package.json defines that script. Resolution is deliberately a union across all packages rather than the package nearest the file. Docs legitimately show commands to run from another directory — README.md documents `npm run build` and `npm run serve`, which only plugins/web-ui and cli define — so matching per directory would reject correct docs. The union still catches the case that matters: a command no package defines anywhere. It cannot catch a command documented against the wrong directory. Currently clean at 26 references, 14 distinct, across 7 packages.
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
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.
fly/README.mddocuments two commands that do not exist:FLY_API_TOKEN="$(fly tokens create deploy -a "$FLY_SANDBOX_APP_NAME")" npm run smoke:fly FLY_API_TOKEN=... npm run smoke:xNeither
smoke:flynorsmoke:xis defined in anypackage.json, andgit log -Sshows neither has ever been defined in this repository's history. There is noscripts/fly-smoke.tseither, so the paragraph describing what "the smoke test" backs up and restores documents a run nobody can perform. TheX_SMOKE_REQUIRE_AUTHandX_SMOKE_REQUIRE_FIREHOSEenv vars in the same block appear nowhere outside that file.The interesting part isn't the two stale blocks — it's that nothing would have caught them. Every other class of rot in this repo has a gate:
knipfor dead code,oxlintandeslintfor source,prettierfor formatting. Documented commands had none, so these sat indefinitely.So this PR is mostly the guard, with the two fixes that make it pass.
Commits
docs(fly): drop smoke commands that were never implemented— removes the two command blocks and the prose that exists only to describe them. Removed rather than corrected because there is no correct name to point at; if those scripts are pending, the right move is to re-add the docs with them and I'm happy to drop this commit. Thenpm run smoke:git-cliblock below it is real and is left untouched.ci: verify documented npm commands exist— addsscripts/check-doc-npm-scripts.mjs, alint:docsscript, and one step in thelintjob next tolint:knip/lint:ox. It reads every tracked*.md, extracts eachnpm run <script>, and fails if no trackedpackage.jsondefines that script.The one design decision
Resolution is a union across all packages, not the package nearest the file. Docs legitimately show commands to run from another directory — root
README.mddocumentsnpm run buildandnpm run serve, which onlyplugins/web-uiandclidefine — so matching per directory would reject correct docs.The tradeoff: the union catches the case that matters (a command no package defines anywhere) but cannot catch a command documented against the wrong directory. I went with the version that has no false positives, on the grounds that a lint gate which fires on correct docs gets ignored or removed. Happy to tighten it if you'd rather have the stricter check plus an allowlist.
Verification
On this branch the checker reports:
Against
mainbefore the docs commit, it reports exactly the two real hits and nothing else — no false positives across the whole repo, including.claude/skills/*.md,cli/,fly/,deploy/, and the plugin READMEs:Negative-tested by appending
npm run definitely-not-a-scripttodeployment.md— caught with file:line, exit 1.Full lint job run locally and green:
format:check,lint,lint:knip,lint:ox,lint:docs. Commits are ordered so each is green on its own — the docs fix lands before the gate that would flag it.Nothing outside the lint job is touched, so no runtime, test, or build behavior changes.
Note on process
CONTRIBUTING.mdasks for bugs as issues rather than PRs, and I want to be upfront that I'm aware of that. I went with a PR here because the substance is a CI script rather than a product change, and describing it in prose seemed strictly worse than showing it. If you'd prefer this as an issue, or as anadrs/note, say the word and I'll move it.Separately, I filed #248 for an unrelated bug in
plugins/chassis/src/http.tsfollowing the issue path.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.