fix: fail closed on a type: adr document the adr-smadr gate cannot see - #210
Merged
Merged
Conversation
#209) The ADR carve-out (#203) removes documents from the mif-validate corpus on a content predicate, but the adr-smadr CI job only globs docs/adr/*.md — non-recursive (the structured-madr Action resolves glob(join(path, pattern)) verbatim). A type: adr document in another L3 tree, or nested below docs/adr/, was carved out of one gate and invisible to the other: validated by nothing, CI green. corpus.mjs now partitions the L3 walk once and throws when a type: adr document sits outside the adr-smadr gate's actual coverage, keeping the content-based carve-out while making the module's stated contract true. Closes #209.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR closes a validation gap where type: adr documents could be carved out of the mif-validate corpus yet also be invisible to the adr-smadr CI job due to its non-recursive docs/adr/*.md glob, causing CI to pass while validating nothing.
Changes:
- Adds a fail-closed partition in
scripts/lib/corpus.mjsthat throws if anytype: adrdoc falls outside theadr-smadrjob’s actual coverage. - Adds regression tests that reproduce the “escapes both gates” scenarios and assert the new fail-closed behavior.
- Updates CI and changelog documentation/comments to reflect the non-recursive gate boundary.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/corpus.test.mjs | Adds regression tests covering ungated type: adr placements and the expected fail-closed behavior. |
| scripts/lib/corpus.mjs | Introduces partitionL3Docs() to split ADR vs non-ADR docs and throw on ADRs outside docs/adr/*.md coverage. |
| CHANGELOG.md | Documents the fix and its rationale for gate coverage. |
| .github/workflows/ci.yml | Clarifies in-job comments that pattern: '*.md' is non-recursive and relies on corpus fail-closed enforcement. |
Suppressed comments (1)
scripts/lib/corpus.mjs:98
dirname(f) !== ADR_GATE_DIRrelies on platform-specific path parsing. If any globbed path contains backslashes (e.g., on Windows), this check can misclassify direct children of docs/adr as "ungated" and throw. Preferposix.dirnameon normalized forward-slash paths for repo-relative comparisons.
const adrDocs = files.filter((f) => isAdrDoc(f));
const ungated = adrDocs.filter((f) => dirname(f) !== ADR_GATE_DIR);
if (ungated.length > 0) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…yml, align hook comment - dirname(f) is compared against a forward-slash literal; normalize via path.sep like validate-plugin.mjs's labelFor (Copilot review thread) - pin ADR_GATE_DIR agreement with the adr-smadr job's path/pattern in a PR-gated test so the two literals cannot hand-sync-drift apart - mif-guard.mjs no longer claims its carve-out matches CI 'exactly'; placement is CI's contract, the hook's carve-out is unconditional - nested-ADR test asserts both partition entry points; stale filter wording dropped from listAdrDocs()'s comment
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
scripts/lib/corpus.mjs:90
- partitionL3Docs() is invoked separately by listL3Docs() and listAdrDocs(). Callers like listAllGatedDocs() (and provenance-corpus-check.mjs via listAllGatedDocs) end up walking the L3 trees twice (glob + frontmatter reads), which contradicts the nearby “One walk” intent and adds avoidable IO. Consider memoizing the partition per process.cwd() so a single run doesn’t rescan the filesystem twice while still supporting tests that chdir().
function partitionL3Docs() {
for (const d of L3_DIRS) {
if (!isDirectory(d)) throw new Error(`L3 doc directory missing: ${d}`);
}
const files = L3_DIRS.flatMap((d) => globSync(`${d}/**/*.md`)).sort();
tests/corpus.test.mjs:233
- This regex pins
pattern:to single quotes only. That makes the regression test brittle to harmless YAML formatting changes (e.g.,pattern: "*.md"orpattern: *.md) even though the semantic contract is the same. Consider accepting either quote style (or none).
const projectAdrSteps = [...ci.matchAll(/path:[ \t]*(docs\/[^\s]+)\s*\n\s*pattern:[ \t]*'([^']+)'/g)];
Merged
zircote
added a commit
that referenced
this pull request
Aug 5, 2026
Four real issues found by an independent review pass, all confirmed and fixed: 1. CHANGELOG.md had no entry for this change -- repo precedent (#210) is that every fix PR adds one. Added under the existing empty [Unreleased] section. 2. checkKebabCase's README exemption applied to ANY path segment named "README", not just the file's own basename -- readmeAsIndex:true on a directory literally named README/ (e.g. docs/README/foo.md) silently passed the kebab check, letting a non-kebab route segment reach the route model unflagged. Scoped the exemption to the last segment only, matching what the "index" exemption already gets for free (index is itself kebab-valid, so it never had this exposure). 3. index.md and README.md in the same directory both resolving to that directory's route was silently absorbed into one Set entry by buildRouteSet under readmeAsIndex:true -- exactly the "route model cannot be trusted" condition checkKebabCase already exists to catch, just a different cause. Added checkRouteCollisions(), wired into checkAll with the same fail-closed-by-default / allowNonKebab-audit-mode split, threaded through check-doc-links.mjs's error handling and human report, and audit-deterministic.mjs's finding summary/recommendation. Running the fixed checker against the research-harness-template corpus that motivated #213 surfaced a real, previously-undetected instance: docs/README.md and docs/index.mdx both exist at the docs root and would collide under readmeAsIndex -- flagging separately, out of scope here. 4. Added missing test coverage: an uppercase README.md at the docs root (previously only a subdirectory case and a lowercase root case were tested), and a mdLinksRewritten link carrying a #anchor/?query suffix. 5. Documented (comment only, no live caller affected) that checkFile's fileSet parameter must be supplied for the mdLinksRewritten exemption to take effect -- a direct caller passing mdLinksRewritten: true without fileSet silently gets the pre-#213 behavior rather than an error. 6 new regression tests (31 total in doc-links-params.test.mjs, up from 25). 325/325 tests pass, validate-plugin clean, lint:md clean. Re-verified against research-harness-template/docs: 14 findings now (13 genuine broken links + 1 real, newly-surfaced route collision), 4 files -- up from 13/3 before this fixup commit, which is the collision detector doing its job, not a regression.
zircote
added a commit
that referenced
this pull request
Aug 5, 2026
…en-md-links route model gaps (#214) * fix: hydrate-ontology flat-path mismatch, README-as-index and rewritten-md-links route model gaps Two bugs found while running audit-docs against research-harness-template/docs. hydrate-ontology.mjs (#212): both remote sources and the sibling-checkout fallback assumed a per-ontology subdirectory layout (ontologies/<name>/<name>.ontology.yaml), but the ontologies repo vendors flat (ontologies/<name>.ontology.yaml) -- confirmed against the live repo and mif-spec.dev (both flat paths return 200, both nested paths 404). Corrected all three source paths to match. link-integrity route model (#213): two related gaps in scripts/lib/doc-links.mjs's route model, both opt-in (default false, zero behavior change for every existing caller/test): - readmeAsIndex: a content-collection config that re-slugs a subdirectory README.md to its directory's own route (research-harness-template's custom generateId is a real example) wasn't modeled at all -- README.md was treated as an ordinary same-level page. This is what actually broke research-harness-template#834: check-doc-links --write "fixed" a correct link into a broken one, self-consistently, because the route it computed for README.md itself was already wrong. routeForDocFile/checkKebabCase now accept the option. - mdLinksRewritten: a site wiring a build-time remark/rehype plugin (e.g. astro-rehype-relative-markdown-links) resolves file-relative .md/.mdx links itself, so the md-suffix-links defect class is a false positive for such a site -- the check had no way to know this. checkFile now accepts the option and skips a .md-suffixed link entirely when it's file-relative and resolves to a real file, leaving every other case (absolute paths, wrong depth, nonexistent targets) checked exactly as before. Both flags thread through check-doc-links.mjs, audit-deterministic.mjs (incl. the generated md-suffix-links fix_command), and the audit-docs skill's own Step 3 invocation, opt-in only -- the skill instructs never inferring either from a target site's behavior. Verified against the real research-harness-template/docs corpus that motivated both reports: with --readme-as-index --md-links-rewritten, the 13 genuinely broken links remain flagged and the false-positive md-suffix class and README non-kebab warnings are both gone; without them, this plugin's own docs/ (74 files, neither convention) is unaffected. 9 new regression tests (2 for hydrate-ontology's real remote/sibling paths via manual verification, 5 for readmeAsIndex including an exact PR research-harness-template#834 reproduction, 5 for mdLinksRewritten). 319/319 tests pass, validate-plugin clean, lint:md clean. Fixes #212. Fixes #213. * fix: address review findings on PR #214 Four real issues found by an independent review pass, all confirmed and fixed: 1. CHANGELOG.md had no entry for this change -- repo precedent (#210) is that every fix PR adds one. Added under the existing empty [Unreleased] section. 2. checkKebabCase's README exemption applied to ANY path segment named "README", not just the file's own basename -- readmeAsIndex:true on a directory literally named README/ (e.g. docs/README/foo.md) silently passed the kebab check, letting a non-kebab route segment reach the route model unflagged. Scoped the exemption to the last segment only, matching what the "index" exemption already gets for free (index is itself kebab-valid, so it never had this exposure). 3. index.md and README.md in the same directory both resolving to that directory's route was silently absorbed into one Set entry by buildRouteSet under readmeAsIndex:true -- exactly the "route model cannot be trusted" condition checkKebabCase already exists to catch, just a different cause. Added checkRouteCollisions(), wired into checkAll with the same fail-closed-by-default / allowNonKebab-audit-mode split, threaded through check-doc-links.mjs's error handling and human report, and audit-deterministic.mjs's finding summary/recommendation. Running the fixed checker against the research-harness-template corpus that motivated #213 surfaced a real, previously-undetected instance: docs/README.md and docs/index.mdx both exist at the docs root and would collide under readmeAsIndex -- flagging separately, out of scope here. 4. Added missing test coverage: an uppercase README.md at the docs root (previously only a subdirectory case and a lowercase root case were tested), and a mdLinksRewritten link carrying a #anchor/?query suffix. 5. Documented (comment only, no live caller affected) that checkFile's fileSet parameter must be supplied for the mdLinksRewritten exemption to take effect -- a direct caller passing mdLinksRewritten: true without fileSet silently gets the pre-#213 behavior rather than an error. 6 new regression tests (31 total in doc-links-params.test.mjs, up from 25). 325/325 tests pass, validate-plugin clean, lint:md clean. Re-verified against research-harness-template/docs: 14 findings now (13 genuine broken links + 1 real, newly-surfaced route collision), 4 files -- up from 13/3 before this fixup commit, which is the collision detector doing its job, not a regression.
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.
Closes #209.
Problem
The ADR carve-out (#203) removes documents from the
mif-validatecorpus on a content predicate (type: adr, any L3 tree), but theadr-smadrCI job that is supposed to gate them instead is scoped to a directory — and itspattern: '*.md'is non-recursive: the structured-madr Action resolvesglob(join(path, pattern))verbatim (src/validate.js), so only direct children ofdocs/adr/are ever seen. Atype: adrdocument in another L3 tree, or nested belowdocs/adr/, was carved out of one gate and invisible to the other: validated by nothing, CI green.Resolution (option 2 of the three in #209, extended)
scripts/lib/corpus.mjsnow walks the L3 trees once (partitionL3Docs()) and throws when atype: adrdocument sits outside theadr-smadrgate's actual coverage — outsidedocs/adr/, or nested below it. This keeps the content-based carve-out #208 chose, makes the module's stated contract true, and matches the fail-closed discipline already in the file (missing tree, empty result, non-directory tree).The issue's open question about globstar semantics is resolved empirically: the Action's glob is non-recursive, so nested
docs/adr/**files do escape it — the fail-closed check covers that shape too.Acceptance criteria mapping
type: adrdoc under any L3 tree is validated by exactly one gate: direct children ofdocs/adr/go toadr-smadr; anywhere else now throws in every corpus consumer (mif-validatesteps,engine-parity.mjs,provenance-corpus-check.mjs) — PR-gated, never silently passing.corpus.mjs's stated contract and whatci.ymlgates now agree; both comment blocks updated to say so.corpus.mjs: both fail-closed tests fail, 2/16) and pass after (16/16; fulltest:hook308/308).Gates run locally
validate-pluginOK ·test:hook308/308 ·lint:md0 issues / 317 files ·actionlintclean · provenance-corpus-check byte-idempotent.