Share one navigation drift resolver between doctor and nav - #179
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7491203b9a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Important
The shared resolver is the right shape and the drift computation is a genuine improvement over both versions it replaces. One issue holds it back: filteredPageSet omits expandDirectories: false, so it disagrees with the staging path it is meant to mirror — the exact divergence class this PR exists to eliminate. Also missing a changeset, and doctor doesn't use the new findBlockingDiagnostic, leaving a residual doctor/nav split.
Reviewed changes
- New
packages/leadtype/src/config/navigation.ts(364 lines) owns manifest resolution, include/exclude filtering, i18n merging, and drift computation for both CLIs; exportsresolveCollectionNavigation,resolveProjectNavigation, andfindBlockingDiagnostic. doctor'sinspectNavigationdrops from ~106 lines of hand-assembly to ~46 lines readingresolveProjectNavigation(project); resolution failures now surface as anav.unresolvablediagnostic instead of crashing the command.DoctorReportgains a requiredcollections[].navigationOriginand anavigation.originthat reports"mixed"when collections disagree; documented indocs/reference/doctor.mdx.navdelegates to the same resolver, deletes its localfindDuplicates, and moves the blocking-diagnostic check ahead of thecontentDircheck.llm.tsextractscollectNavEntryPagesand routesbuildNavigationFromNav's root loop through it, so root-levelnavpins now run thepinnedUrlPathsshadowing check they previously skipped (new throw on a shadowed root pin).- 10 new tests across
nav.test.ts,doctor.test.ts, andnavigation/authoring.test.ts. I traced each against the pre-change code — all 10 genuinely fail without the fix.
⚠️ No changeset for a release with new user-visible behavior
This PR adds a new doctor finding id (nav.unresolvable), a new mixed navigation origin, a new required navigationOrigin field on every collection in DoctorReport, and a new throw for shadowed root-level nav pins. None of the unreleased changesets (resolve-project.md, leadtype-doctor.md, navigation-authoring.md) mention any of it, and there's no changeset in this diff.
The repo's convention is a detailed prose changeset per user-visible change — the existing ones are several paragraphs each. As it stands the release notes will not describe any of the above, and the root-pin throw in particular is a behavior change someone can hit on an existing config without warning.
⚠️ doctor never calls findBlockingDiagnostic, so the two commands still disagree
nav gates on findBlockingDiagnostic(project, collection.key) and exits 1 (cli/nav.ts:285-291), with a comment explaining precisely why: source.inherit-failed fires against a readable checkout, so whatever origin resolution fell back to is "the wrong tree, presented as fine."
But resolveProjectNavigation resolves every readable collection unconditionally, and doctor doesn't consult findBlockingDiagnostic at all. So on a project with a blocking source.inherit-failed:
navrefuses to print a tree and exits 1.doctorprints groups, a page count, and drift derived from that same wrong fallback tree — plausibly emitting spuriousnav.unrepresented-pagewarnings that point the author at pages that aren't actually missing.
That's the same class of divergence the PR is closing, left in place at the seam. Either resolveCollectionNavigation should short-circuit on a blocking diagnostic (and surface it as an error diagnostic both callers see), or doctor should call the helper the way nav does. Worth deciding explicitly rather than by omission.
⚠️ i18n × include/exclude is the one combination with no test
isExcluded (config/navigation.ts:142-151) builds three candidate paths, and the third — `${page.sourceLocale}/${page.logicalPath}` — exists specifically so that a localized page in a filtered collection matches the on-disk path the globs were written against. I verified the logic is correct for locale-directory, flat, and fallback layouts.
But the new tests cover exclude or i18n, never both: nav.test.ts has one exclude test and one i18n test as separate cases, and doctor.test.ts's excluded-page-count test is monolingual. The sourceLocale branch is unexercised, and every localized page in a filtered collection depends on it. A single test with i18n configured plus an exclude entry would cover the branch and pin the path semantics against future changes to outputRelativePathForLocale.
ℹ️ Nitpicks
- Two page-count semantics in one command.
nav'spageCountnow comes fromresolved.pageCount(distinct routed urlPaths), butreportInferredTreestill computes it with the localcountPages(manifest)(pages summed across the group tree). Those differ whenever a page is reachable from two groups. Same field, same JSON output, two meanings depending on whether the project has a config. - No dedicated test file for the new module.
config/navigation.tsis 364 lines with a fair amount of subtle path logic (filteredPageSet,isExcluded,rootIsLiteral, group dedupe by slug), and every bit of its coverage is indirect through the two CLIs. Aconfig/navigation.test.tsexercising the helpers directly would make the next change to this file much cheaper to verify. - Stacked PR. Base is
dx/157-resolve-projectrather thanmain, so #167 needs to land first.
Claude Opus | 𝕏
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bc94d2610
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Important
The glob-option fix is correct and the author's correction about the exclude side is accurate — I verified both against tinyglobby directly. But chasing it surfaced something larger: I ran the real runGenerateCommand against the same fixture shapes these new tests use, and generate does not apply collection-level include/exclude at all for a single default docs collection. The staging mirror the resolver is written to mirror is skipped entirely on that path, so the filtering makes doctor/nav disagree with the build rather than agree with it.
Reviewed changes — the delta since the prior pullfrog review at 7491203:
- Glob parity for the admitted page set.
filteredPageSet's tinyglobby call inconfig/navigation.tsgaineddot: trueandexpandDirectories: false, matchingcopySourceFiles(cli/generate.ts:988-997) option for option. - Three
navregression tests. A dot-directory page counted under an unrelated filter, a bare-directoryincludestaying literal at 0 pages, and a bare-directoryexcludepruning its contents. - Prior thread retired. The author replied and resolved the
filteredPageSetthread, with a correction that theexpandDirectoriesflag is a no-op on theignoreside. I confirmed that empirically — tinyglobby prunes a directory named by a bareignoreentry either way, so only the include side ever diverged.
⚠️ generate never applies collection include/exclude on the default single-collection path, so the resolver's filtering creates the divergence instead of closing it
createSourceMirror (cli/generate.ts:1280-1307) returns the raw, unstaged docs directory when isDefaultSingleSource && !hasFilters. hasFilters is derived from the CLI --include/--exclude flags only — a collection's own filters live on source.filters and are consumed exclusively inside copySourceFiles, which that early return skips. For collections: { docs: { dir: "docs", … } } — the layout leadtype init scaffolds — staging never runs, and resolveDocsNavigation reads the unfiltered directory.
I ran this rather than inferred it. With exclude: ["drafts/**"] and docs/drafts/wip.mdx present, generate reports Converted 2 docs and writes public/docs/drafts/wip.md, while doctor/nav report pageCount 1 and unplaced: [] — the page is live at its URL and the CI-gating tool says there is no drift. With include: ["guides"], generate publishes public/docs/guides/intro.md while the new test asserts pageCount is 0. Add a second collection and the same config behaves the opposite way: staging runs, and drafts/wip.md is correctly absent from the output.
So the pre-PR behavior the description lists as bug #3 (doctor's routedPages counting the excluded page) was the correct answer for this layout, and nav's excluded-page count was correct too. The interesting question this raises is which side is the actual bug — I'd guess generate's fast path silently dropping source.filters is, and the resolver is describing the behavior everyone expects rather than the behavior that ships. But that makes it a generate fix that has to land with this one, not a doctor/nav fix.
Technical details
# Collection filters are dropped by `generate`'s unstaged fast path
## Affected sites
- `packages/leadtype/src/cli/generate.ts:1289-1307` — `hasFilters` is built from
`args.include` / `args.exclude` (CLI flags) only. `source.filters`, populated
from the collection's `include`/`exclude` at `generate.ts:949-954`, is never
consulted, so `isDefaultSingleSource && !hasFilters` takes the early return
and hands `resolveDocsNavigation` the raw content directory.
- `packages/leadtype/src/config/navigation.ts:115-140` — `filteredPageSet` is
documented as "the mirror `generate` stages". For the fast path there is no
mirror, so the admitted set is narrower than what ships.
- `packages/leadtype/src/cli/nav.test.ts:242-244`, `:286-289` and
`packages/leadtype/src/cli/doctor.test.ts:430-432` — the explanatory comments
assert filtered staging happens for exactly the fixture shape where it does
not.
## Reproduction (verified, not inferred)
// leadtype.config.ts — single default collection
collections: { docs: { dir: "docs", routePrefix: "/docs",
exclude: ["drafts/**"], navigation: ["index"] } }
docs/index.mdx
docs/drafts/wip.mdx
runGenerateCommand(["--src", dir, "--out", out])
→ "Converted 2 docs"; public/docs/drafts/wip.md exists
runDoctorCommand(["--src", dir, "--json"])
→ collections[0].pageCount 1, navigation.routedPages 1,
unrepresentedPages []
Second shape, same result class:
collections: { docs: { dir: "docs", routePrefix: "/docs",
include: ["guides"] } }
docs/guides/intro.mdx
generate → public/docs/guides/intro.md published
nav → pageCount 0 (asserted by the new test)
Control — two collections, so staging runs and the filter is honoured:
collections: { docs: { dir: "content/docs", exclude: ["drafts/**"] },
guides: { dir: "content/guides" } }
generate → drafts/wip.md correctly absent from public/
## Required outcome
- `doctor`, `nav`, and `generate` agree about which pages a filtered collection
publishes, for the single-default-collection layout as well as the
multi-source one.
- Whichever side changes, the new tests' expectations and their explanatory
comments describe what `generate` actually does for the fixture they use.
## Suggested approach (optional)
- Fold `source.filters` into the `hasFilters` check so a collection that
declares `include`/`exclude` always forces staging (`forceStaging` already
exists for the OpenAPI case and does exactly this).
- Add a test that runs `runGenerateCommand` on the single-default-collection
fixture and asserts the excluded page is absent from the output tree — the
resolver-level assertions cannot catch this class on their own.
## Open questions for the human
- Is the fast path's filter drop a known trade-off (skip staging for speed on
the common layout) or an oversight? That decides whether this PR waits for a
`generate` fix or ships alongside one.⚠️ doctor now reports two disagreeing page counts for the same collection
countCollectionPages (cli/doctor.ts:257-287, unchanged by this PR) runs the same include/exclude patterns through tinyglobby with the old option set — no dot, no expandDirectories: false — and its result becomes collections[].pageCount. navigation.routedPages now comes from the new resolver. Before this commit the two option sets were identical; now they are not, and the same JSON report can contradict itself.
Verified against the real report: with include: ["guides"] doctor emits collections[0].pageCount: 1 alongside navigation.routedPages: 0, and no collection.no-matches warning. With a .well-known/security.mdx page under an unrelated exclude, it emits pageCount: 1 alongside routedPages: 2. Whatever the resolution of the finding above, these two globs should read the same config the same way.
Technical details
# Two glob semantics for one collection's filters inside `doctor`
## Affected sites
- `packages/leadtype/src/cli/doctor.ts:266-274` — the `fg(include, …)` call in
`countCollectionPages` omits `dot` and `expandDirectories`, so tinyglobby's
defaults apply: dotfiles are skipped and a bare `guides` fans out to
`guides/**`.
- `packages/leadtype/src/config/navigation.ts:128-138` — the same patterns, now
with `dot: true, expandDirectories: false`.
## Observed
collections: { docs: { dir: "docs", include: ["guides"] } }
docs/guides/intro.mdx
collections[0].pageCount 1 ← countCollectionPages
navigation.routedPages 0 ← resolveProjectNavigation
issues [] ← collection.no-matches never fires
collections: { docs: { dir: "docs", exclude: ["drafts/**"] } }
docs/index.mdx + docs/.well-known/security.mdx
collections[0].pageCount 1
navigation.routedPages 2
## Required outcome
- One glob-option set for collection `include`/`exclude` across the whole
codebase, so `pageCount`, `routedPages`, and `collection.no-matches` cannot
disagree about the same collection.
## Suggested approach (optional)
- Have `countCollectionPages` reuse the resolver's admitted set rather than
re-globbing, or at minimum lift the option object into one shared constant.
- `packages/leadtype/src/source/index.ts:516` omits the flags too; still
pre-existing and out of scope here, but it is the third copy.ℹ️ Nitpicks
- The bare-directory
excludetest passes identically with and without the change.nav.test.ts:294-315documents tinyglobby's ignore-pruning behavior rather than guarding the fix — I confirmed theexpandDirectoriesflag makes no difference on theignoreside. That is a reasonable characterization test given how subtle the behavior is; worth saying in the comment that it pins third-party behavior rather than this commit's.
Claude Opus | 𝕏
5bc94d2 to
1703cf0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1703cf0b26
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Important
The new commit answers the Codex thread by resolving navigation against the admitted file set — the right shape, and it retires my earlier filteredPageSet concern entirely. But the premise both the Codex comment and the new test comments rest on ("generate stages the filtered mirror before resolving") is false for the single default docs collection, and I re-verified that against this commit. The divergence I flagged last round has not closed; it inverted and escalated from a silent under-count into a hard doctor/nav failure on a project whose generate exits 0.
Reviewed changes — the delta since the prior pullfrog review at 5bc94d2:
- Resolution over the admitted set, not post-hoc filtering.
filteredPageSetandisExcludedare gone fromconfig/navigation.ts;admittedFileFilternow builds afilterFilepredicate and passes it intoresolveDocsNavigation, so a curated entry naming a filtered-out page fails resolution rather than being quietly dropped from counts afterwards. - One glob semantics for derivation and resolution.
derivationPathFilteris exported fromconfig/project.tsso the resolver andinferNavigationFromContentshare the same tinyglobby options instead of a second hand-rolled matcher. - New
nav.unresolvableerror path, with thefixstring naminginclude/excludewhen the collection has filters, plus regression tests innav.test.tsanddoctor.test.tsasserting exit 1.
⚠️ doctor and nav now hard-fail a build that generate completes successfully
createSourceMirror (cli/generate.ts:1268-1296) returns the raw, unstaged docs directory when isDefaultSingleSource && !hasFilters && !forceStaging. hasFilters is derived from the CLI --include/--exclude flags only; a collection's own filters live on source.filters and are consumed exclusively inside copySourceFiles, which that early return skips. So for collections: { docs: { dir: "docs", … } } — the layout leadtype init scaffolds — no mirror is ever staged, and generate publishes the "excluded" page.
I ran this against 1703cf0b on the new tests' own fixture rather than inferring it:
collections: { docs: { dir: "docs", routePrefix: "/docs",
exclude: ["drafts/**"], navigation: ["index", "drafts/wip"] } }
docs/index.mdx + docs/drafts/wip.mdx
generate → exit 0, "Converted 2 docs", public/docs/drafts/wip.md written
doctor → exit 1, ok: false, navigation.routedPages 0,
nav.unresolvable: Nav page "drafts/wip" … did not match a documentation page
Last round this cost a wrong count. Now it fails the command CI gates on, for a project that builds and ships fine.
To be clear about which side I think is wrong: generate's fast path is. The framework runtime already agrees with this PR — createDocsSource().getNavigation() (source/index.ts:705-724) threads selectedFileFilter() as filterFile and throws on this exact fixture. So the resolver is describing the behavior everyone expects, and generate is the outlier. That makes this a generate fix that has to land with this one, not a reason to revert the resolver.
Technical details
# `generate`'s unstaged fast path drops `source.filters`
## Affected sites
- `packages/leadtype/src/cli/generate.ts:1268-1296` — `hasFilters` is built from
`args.include` / `args.exclude` only. `source.filters`, populated from the
collection's `include`/`exclude` at `generate.ts:949-954`, is never consulted,
so `isDefaultSingleSource && !hasFilters` takes the early return.
- `packages/leadtype/src/config/navigation.ts:194-227` — `admittedFileFilter`'s
predicate is passed to `resolveDocsNavigation` as the set `generate` stages.
On the fast path `generate` stages nothing, so resolution is stricter than the
build.
- `packages/leadtype/src/cli/doctor.test.ts:428-431`,
`packages/leadtype/src/cli/nav.test.ts:390-392`,
`packages/leadtype/src/cli/nav.test.ts:286-289` — the explanatory comments
assert filtered staging happens for exactly the fixture shape where it does not.
## Reproduction (verified against 1703cf0b, not inferred)
// leadtype.config.ts — single default collection
collections: { docs: { dir: "docs", routePrefix: "/docs",
exclude: ["drafts/**"],
navigation: ["index", "drafts/wip"] } }
docs/index.mdx
docs/drafts/wip.mdx
runGenerateCommand(["--src", dir, "--out", out])
→ exit 0, "Converted 2 docs in 7 ms"
→ public/docs/index.md AND public/docs/drafts/wip.md
runDoctorCommand(["--src", dir, "--json"])
→ exit 1, ok: false
→ collections[0].pageCount 1, navigation.routedPages 0
→ nav.unresolvable (error): collection "docs" navigation did not resolve:
Nav page "drafts/wip" under "root" did not match a documentation page.
Control — the framework runtime path agrees with this PR:
createDocsSource().getNavigation() // source/index.ts:705-724
→ threads selectedFileFilter() as filterFile → throws on this fixture
## Required outcome
- `generate`, `doctor`, and `nav` agree about which pages a filtered collection
publishes, for the single-default-collection layout as well as the multi-source
one.
- The new tests' expectations and their explanatory comments describe what
`generate` actually does for the fixture they use.
## Suggested approach (optional)
- Fold `source.filters` into the `hasFilters` check so a collection that declares
`include`/`exclude` always forces staging — `forceStaging` already exists for
the OpenAPI case and does exactly this.
- Add a test that runs `runGenerateCommand` on the single-default-collection
fixture and asserts the excluded page is absent from the output tree. The
resolver-level assertions cannot catch this class on their own.
## Open questions for the human
- Is the fast path's filter drop a known trade-off (skip staging for speed on the
common layout) or an oversight? That decides whether this PR waits for a
`generate` fix or ships alongside one.ℹ️ Nitpicks
- Still no changeset, and its scope grew. Alongside
nav.unresolvable, themixedorigin, and the requirednavigationOriginfield, this commit adds a new hard failure for a curated entry naming a filtered page — a behavior change a user can hit without touching their config. doctor's two page counts still disagree (carried from the prior review, unchanged here): my run above showscollections[0].pageCount 1fromcountCollectionPagesnext tonavigation.routedPages 0from the resolver, in the same JSON report.
Claude Opus | 𝕏
There was a problem hiding this comment.
[!IMPORTANT]
041d4454 closes my prior load-bearing finding at the root cause rather than at the assertion: hasSourceFilters now forces a staged mirror when a collection declares its own include/exclude, so generate, doctor, and nav finally agree about which pages a filtered collection publishes. I re-ran the fixture from last round and it exits 1 on all three. Both of my open threads on that premise are resolved, and the new cli.test.ts build-side test is the right place to pin it.
But making that staging path reachable for the default single-docs layout — the layout leadtype init scaffolds — exposed a second-order break I verified by running it: excluding a _partials/** directory now silently drops the partial's content from published pages, exit 0. And the new per-locale loop reports one finding per configured locale for a single shared source file, where generate reports one.
Reviewed changes — the delta since 1703cf0b:
generatestaging fix (cli/generate.ts:1286-1296).hasSourceFiltersfoldssource.filtersinto the fast-path bail-out. I confirmedsource.filtersisundefinedunless include/exclude is non-empty (generate.ts:937-942), so the predicate doesn't over-stage, and theelsebranch routes throughcopySourceFileswith the samedot: true, expandDirectories: falseglobs the resolver uses.- Per-locale navigation resolution (
config/navigation.ts:252-284,:342), answering the Codex thread. Non-default locales contributeunknownentries and locale-scopednav.unresolvablediagnostics; manifest/counts/placement drift stay default-locale. Locale-independent classes (duplicate logical path, ambiguous layout) correctly aren't duplicated — the default resolve throws and early-returns before the loop. - Root-pin shadowing (
llm/llm.ts:3645-3743).collectNavEntryPagesextracted sobuildNavigationFromNav's root loop enforces the same shadow check as groups.
ℹ️ Nitpicks
- Still no changeset, and the scope grew again.
041d4454changesgenerate's published output for any existing single-default-docsconfig that declaresinclude/exclude— previously those filters were ignored and the pages shipped. That's a user-visible behavior change on top ofnav.unresolvable, themixedorigin, and the requirednavigationOriginfield. - Empty admitted set fails with an internal-sounding error.
include: ["guides"]against a bareguides/directory (no glob) admits nothing;generateexits 1 — correctly, and the comment atnav.test.ts:286-291is accurate about that — but the message isgenerateLLMFullContextFiles found no markdown under "<out>/docs". Run convertAllMdx first, or check that config.outDir matches.The CLI-flag path has a clear guard for exactly this (generate.ts:1041-1045,"No MDX files matched the provided include/exclude filters"); the collection-filter path has no equivalent, andllms.txt+.well-known/llms.txtare written before the failure. - Carried, unchanged:
navstill dropsfailure.fix(thread left open atcli/nav.ts:309-314);doctorstill never callsfindBlockingDiagnostic(onlynav.ts:20,nav.ts:285reference it);doctor'scollections[].pageCountandnavigation.routedPagesstill disagree in the same JSON report.
Claude Opus | 𝕏
| if ( | ||
| isDefaultSingleSource && | ||
| !(hasFilters || hasSourceFilters || forceStaging) | ||
| ) { |
There was a problem hiding this comment.
Forcing a staged mirror for collection filters is the right call, but it makes <import> partials break silently for the layout this newly applies to. exclude: ["_partials/**"] is the canonical reason to write an exclude — you don't want partials published as pages — and now excluding them removes them from the mirror the includer reads.
Verified repro: docs/quickstart.mdx with <import src="./_partials/shared.mdx#snippet" /> and exclude: ["_partials/**"] → generate exits 0, prints Warning: failed to include ./_partials/shared.mdx from /tmp/leadtype-generate-KbdSQi/docs/quickstart.mdx: ENOENT: no such file or directory, and publishes docs/quickstart.md with the partial body missing. Drop the exclude and the body is present. Before this commit the fast path served the real directory in place, so the include resolved.
A warning is the wrong severity for silently shipping a page with a hole in it, but the warning is the symptom — the fix is a design decision I don't want to prescribe:
- Thread the real source directory as a
basePathsentry soincludeMarkdowncan resolve outside the mirror, or - Stage everything and apply
source.filtersat page-selection time instead of at copy time, so filters govern what's published rather than what's readable.
Technical details
Collection exclude silently empties <import> partials
Affected sites
packages/leadtype/src/cli/generate.ts:1286-1296—hasSourceFiltersroutes the single defaultdocscollection throughcopySourceFiles, which appliessource.filterswhile copying. Excluded files are absent fromtempDocsDir.packages/leadtype/src/remark/plugins/include.remark.ts:863—includeMarkdownresolvessrcrelative to the including file's own directory.generate.ts:272registers the plugin with nobasePaths, so there is no fallback root.
Reproduction (verified against 041d445, not inferred)
// leadtype.config.ts
collections: { docs: { dir: "docs", routePrefix: "/docs",
exclude: ["_partials/**"] } }
docs/quickstart.mdx → <import src="./_partials/shared.mdx#snippet" />
docs/_partials/shared.mdx
runGenerateCommand(["--src", dir, "--out", out])
→ exit 0
→ stderr: Warning: failed to include ./_partials/shared.mdx from
/tmp/leadtype-generate-XXXX/docs/quickstart.mdx:
ENOENT: no such file or directory
→ public/docs/quickstart.md written, partial body ABSENT
Control — identical fixture with the exclude removed:
→ exit 0, public/docs/quickstart.md contains the partial body
Required outcome
- A collection
excludethat removes a partials directory keeps<import>working, or fails loudly rather than publishing a page with the include silently dropped.
Open questions for the human
- Should
source.filtersgovern publication or readability? The distinction didn't matter before this commit for the default layout, and it decides which of the two fixes above is right.
The root-entry loop of buildNavigationFromNav applied the same first-wins skip as the group loop but never consulted pinnedUrlPaths, so a pin an earlier top-level entry shadowed silently no-opped while the identical mistake inside a titled section threw. Both loops now share one collector, so the check cannot drift apart again.
The layer above resolveProject — resolve each collection's navigation manifest, compare it against the content on disk — was still assembled per command, and diverged the same way project resolution once did: nav dropped i18n, read only its own collection's groups, counted excluded pages, and reported inferred root pages as drift; doctor let a resolution error escape as a crash and accumulated merged group titles once per collection. config/navigation.ts now owns that layer. Per collection it returns the manifest, its origin, the filtered page counts, the drift (unplaced, duplicated, unknown-group pages), and resolution failures as diagnostics with a stable id (nav.unresolvable); per project it aggregates origins (mixed when collections disagree) and dedupes group titles by slug. Both commands read the result, nav additionally fails on any blocking collection diagnostic instead of only when the content directory was missing, and doctor reports each collection's own origin alongside the top-level one.
The shared resolver's include/exclude glob ran on tinyglobby defaults, diverging from the copySourceFiles pass it mirrors: a page under a dot-directory vanished from any filtered collection's count and drift even though generate stages it, and a bare-directory include fanned out to dir/** and counted pages generate never ships. Pass dot: true and expandDirectories: false, matching the staging glob exactly. A bare-directory exclude is unchanged either way — tinyglobby prunes an ignored directory regardless of expansion, and staging skips the same pages — but a test now pins that parity too.
generate stages the filtered mirror before resolving navigation, so a curated entry naming an include/exclude-filtered page fails the build as missing. The shared resolver read the raw content directory and only removed excluded pages from counts afterward, so doctor said ok and nav exited 0 for a project whose build exits 1. Pass the admitted-set filterFile into resolveDocsNavigation — reusing derivationPathFilter's staging glob semantics — so the same reference comes back as a nav.unresolvable finding, and note the filter in the fix when one is configured.
A single default docs collection skipped staging entirely, so its include/exclude were silently ignored: generate shipped the excluded pages that every other shape (multiple collections, a non-default dir, or an openapi config's forced staging) filters out. Stage the mirror whenever the collection declares filters, so filters mean the same thing everywhere and the shared resolver's admitted-set semantics match the build in this shape too. The shared resolver also resolved navigation only for the default locale, while generate resolves the tree once per configured locale — a zh page declaring an unknown group failed the build with no doctor finding and no nav drift. Resolve each configured locale: non-default locales contribute unknown-group findings and locale-scoped resolution diagnostics, while the manifest, counts, and placement drift stay the default locale's.
007fb56 to
19ff52f
Compare
There was a problem hiding this comment.
ℹ️ No new issues in this round —
19ff52fis a clean, well-tested fix to my prior thread. The only thing withholding approval is the still-open<import>partials thread ongenerate.ts:1296, which nothing in this commit touches.
Reviewed changes — the delta since the prior pullfrog review at 041d445. The branch was rebased since then (33f0807 is that commit's rewritten form, same content), so the net new work is a single commit. 007fb567 was unreachable in this shallow checkout, so I read the full diff end to end rather than a range-diff.
navnow prints the diagnostic'sfixhint (cli/nav.ts:311). Thenav.unresolvablefailure branch appends→ ${failure.fix}when present, matching the blocking-diagnostic branch 20 lines above it. This retires the thread I opened atcli/nav.ts:309-314: the include/exclude guidance the resolver generates now reaches anavuser instead of onlydoctor --json. It writes to stderr, so--jsonoutput on stdout stays valid.- Two assertions added to the excluded-page nav test (
cli/nav.test.ts:396-399). I traced them against the pre-change code — without the append, stderr carries onlyNav page "drafts/wip" … did not match a documentation page, so both the→and the`include`/`exclude`assertions fail. Genuine regression coverage, not theatre.
Two notes on things I deliberately did not raise. The fix string ends with "then re-run leadtype doctor", which now surfaces from a leadtype nav invocation — that phrasing is the existing house convention (cli/doctor.ts:393) and changing it here would be churn. And the per-locale finding duplication from my last round is resolved as deferred to #185 rather than fixed here, so config/navigation.ts:261-284 still emits one nav.unknown-group per configured locale on this branch; that's your call to make, but it does mean merge order now matters in both directions — #167 below, #185 above.
Claude Opus | 𝕏

Stacked on #167, which ended with a warning it then demonstrated: centralizing project resolution fixed the layer below, and the layer above it — navigation resolution plus drift computation — was still assembled by hand in both
doctorandnav. Review of that PR found it had already diverged in the same pattern.The evidence
Rendering "what tree does this project actually resolve to, and what drifted?" takes the same steps everywhere: resolve the manifest with the right mounts, groups, and i18n; restrict it to the pages
generatestages; compare the curated entries against what fell through to the root. Both commands assembled those steps themselves, and running the built CLI against small fixtures showed nine ways they disagree — withgenerate, or with each other:navfailed outright on every i18n project. It omittedi18nfromresolveDocsNavigation, so with the default locale under its own directory,navigation: ["index"]exits 1 withNav page "index" under "root" did not match a documentation page.doctorpassed it, with a comment explaining why it must.doctorcrashed with no report when navigation resolution threw. A pin typo escapedinspectNavigationto the raw CLI handler: non-JSON output under--json, exit 1, no finding. CI gates on doctor, so the one input class most likely to be a typo produced the one failure mode with no stable id.navcounted and flagged excluded pages. Withexclude: ["drafts/**"],navreported pageCount 2 and unplaced["/docs/drafts/wip"]whiledoctorcounted 1 — and doctor's ownroutedPagesstill counted the excluded page.navreported the root pages of inferred trees as drift. It emptied the placed-at-root set forinferred/groupsorigins but still computed unplaced, so every inferred project named its own index page as unplaced — contradicting the adjacent comment, doctor, and nav's own no-config path.navread only its own collection's groups wheredoctormerged all collections' (asgeneratedoes), so a page whosegroup:a sibling collection declares was a false-positiveunknownGroup.doctoraccumulated merged group titles once per collection — the groups branch emits every declared group per manifest, so two collections sharing two groups printedsections: Reference, How-to, Reference, How-to(the open thread on Resolve the project once instead of in every command #167).navsilently discarded blocking diagnostics.blockingwas only read when the content directory was missing, sosource.inherit-failedagainst a readable checkout printed a fallback tree — precisely the wrong one — as{ ok: true }, exit 0, whiledoctorandcreateDocsProjectboth failed (the other open thread on Resolve the project once instead of in every command #167).explicitin doctor's report.nav: [...]silently no-opped, because the root-entry loop applied the group loop's first-wins skip without its pin check (thellm.tsnit on Resolve the project once instead of in every command #167).Two commands, same layer, nine divergences. Same design problem as before, one level up.
One resolver
config/navigation.tsowns the layer. Per collection it returns the manifest, its origin, filtered page counts, and the drift; per project it aggregates origins and dedupes groups. Both commands read the result:Diagnostics, not exceptions, following
resolveProject's contract: a pin typo is now anav.unresolvablefinding with a stable id, the owning config field, and a fix command — valid--json, exit 1.navfails on the same diagnostic with the same message.Drift got sharper, not just shared. The curatable trade-off flagged on #167 — disabling drift for filtered collections trades false positives for false negatives — dissolves once the resolver holds the filtered page list: unplaced is intersected against the pages the globs admit, so an excluded page is never drift and an admitted-but-unrouted page still is.
Blocking diagnostics are checked before the tree is shown, via the shared
findBlockingDiagnostic, independent of whether the content directory resolved.Origins report as they are:
"mixed"at the top level when collections disagree, and each collection now carries its ownnavigationOriginin doctor's report. The root-entry pin loop inllm.tsshares the group loop's collector, so a shadowed pin fails identically at both levels.This resolves the two outstanding review threads on #167 (nav.ts blocking diagnostics, doctor group accumulation) and the
llm.tsroot-pin nit.Verification
All nine were reproduced by running the CLI against fixtures before fixing, and re-run after: nav resolves the i18n project (pageCount 1, no drift), doctor reports
nav.unresolvablewith owner and fix as valid JSON, both commands count 1 page underexclude, the inferred project reports no unplaced pages, the sibling-collection group resolves, sections dedupe, the inherit-failure exits 1 fromnav, mixed origins reportmixed, and the shadowed root pin throws.One regression test per bug — five in
nav.test.ts, four indoctor.test.ts, one inauthoring.test.ts. 829 tests pass (819 before), plus this repo's owndoctorandlint docsCI gates re-run locally.docs/reference/doctor.mdxdocumentsnav.unresolvableand themixedorigin.bun run check-typespasses at the package level; the repo-level parallel-build race is #166, offmain.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.