Skip to content

Share one navigation drift resolver between doctor and nav - #179

Merged
KayleeWilliams merged 6 commits into
dx/157-resolve-projectfrom
dx/nav-drift-resolver
Aug 15, 2026
Merged

Share one navigation drift resolver between doctor and nav#179
KayleeWilliams merged 6 commits into
dx/157-resolve-projectfrom
dx/nav-drift-resolver

Conversation

@KayleeWilliams

@KayleeWilliams KayleeWilliams commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

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 doctor and nav. 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 generate stages; 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 — with generate, or with each other:

  1. nav failed outright on every i18n project. It omitted i18n from resolveDocsNavigation, so with the default locale under its own directory, navigation: ["index"] exits 1 with Nav page "index" under "root" did not match a documentation page. doctor passed it, with a comment explaining why it must.
  2. doctor crashed with no report when navigation resolution threw. A pin typo escaped inspectNavigation to 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.
  3. nav counted and flagged excluded pages. With exclude: ["drafts/**"], nav reported pageCount 2 and unplaced ["/docs/drafts/wip"] while doctor counted 1 — and doctor's own routedPages still counted the excluded page.
  4. nav reported the root pages of inferred trees as drift. It emptied the placed-at-root set for inferred/groups origins 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.
  5. nav read only its own collection's groups where doctor merged all collections' (as generate does), so a page whose group: a sibling collection declares was a false-positive unknownGroup.
  6. doctor accumulated merged group titles once per collection — the groups branch emits every declared group per manifest, so two collections sharing two groups printed sections: Reference, How-to, Reference, How-to (the open thread on Resolve the project once instead of in every command #167).
  7. nav silently discarded blocking diagnostics. blocking was only read when the content directory was missing, so source.inherit-failed against a readable checkout printed a fallback tree — precisely the wrong one — as { ok: true }, exit 0, while doctor and createDocsProject both failed (the other open thread on Resolve the project once instead of in every command #167).
  8. Mixed navigation origins collapsed to explicit in doctor's report.
  9. A pinned page shadowed at the top level of nav: [...] silently no-opped, because the root-entry loop applied the group loop's first-wins skip without its pin check (the llm.ts nit 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.ts owns 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:

const resolved = await resolveCollectionNavigation(project, collection);
resolved.manifest;      // mounts, merged groups, i18n — or absent, see diagnostics
resolved.pageCount;     // distinct pages, restricted to what generate stages
resolved.drift;         // unplaced, duplicate, unknownGroup
resolved.diagnostics;   // nav.unresolvable, with owner and fix — not a throw

Diagnostics, not exceptions, following resolveProject's contract: a pin typo is now a nav.unresolvable finding with a stable id, the owning config field, and a fix command — valid --json, exit 1. nav fails 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 own navigationOrigin in doctor's report. The root-entry pin loop in llm.ts shares 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.ts root-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.unresolvable with owner and fix as valid JSON, both commands count 1 page under exclude, the inferred project reports no unplaced pages, the sibling-collection group resolves, sections dedupe, the inherit-failure exits 1 from nav, mixed origins report mixed, and the shadowed root pin throws.

One regression test per bug — five in nav.test.ts, four in doctor.test.ts, one in authoring.test.ts. 829 tests pass (819 before), plus this repo's own doctor and lint docs CI gates re-run locally. docs/reference/doctor.mdx documents nav.unresolvable and the mixed origin.

bun run check-types passes at the package level; the repo-level parallel-build race is #166, off main.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 3e61ca41-337d-415c-8398-c68c331d71bc

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread packages/leadtype/src/config/navigation.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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; exports resolveCollectionNavigation, resolveProjectNavigation, and findBlockingDiagnostic.
  • doctor's inspectNavigation drops from ~106 lines of hand-assembly to ~46 lines reading resolveProjectNavigation(project); resolution failures now surface as a nav.unresolvable diagnostic instead of crashing the command.
  • DoctorReport gains a required collections[].navigationOrigin and a navigation.origin that reports "mixed" when collections disagree; documented in docs/reference/doctor.mdx.
  • nav delegates to the same resolver, deletes its local findDuplicates, and moves the blocking-diagnostic check ahead of the contentDir check.
  • llm.ts extracts collectNavEntryPages and routes buildNavigationFromNav's root loop through it, so root-level nav pins now run the pinnedUrlPaths shadowing check they previously skipped (new throw on a shadowed root pin).
  • 10 new tests across nav.test.ts, doctor.test.ts, and navigation/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:

  • nav refuses to print a tree and exits 1.
  • doctor prints groups, a page count, and drift derived from that same wrong fallback tree — plausibly emitting spurious nav.unrepresented-page warnings 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's pageCount now comes from resolved.pageCount (distinct routed urlPaths), but reportInferredTree still computes it with the local countPages(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.ts is 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. A config/navigation.test.ts exercising the helpers directly would make the next change to this file much cheaper to verify.
  • Stacked PR. Base is dx/157-resolve-project rather than main, so #167 needs to land first.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread packages/leadtype/src/config/navigation.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread packages/leadtype/src/config/navigation.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in config/navigation.ts gained dot: true and expandDirectories: false, matching copySourceFiles (cli/generate.ts:988-997) option for option.
  • Three nav regression tests. A dot-directory page counted under an unrelated filter, a bare-directory include staying literal at 0 pages, and a bare-directory exclude pruning its contents.
  • Prior thread retired. The author replied and resolved the filteredPageSet thread, with a correction that the expandDirectories flag is a no-op on the ignore side. I confirmed that empirically — tinyglobby prunes a directory named by a bare ignore entry 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 exclude test passes identically with and without the change. nav.test.ts:294-315 documents tinyglobby's ignore-pruning behavior rather than guarding the fix — I confirmed the expandDirectories flag makes no difference on the ignore side. 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.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread packages/leadtype/src/cli/nav.test.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread packages/leadtype/src/config/navigation.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. filteredPageSet and isExcluded are gone from config/navigation.ts; admittedFileFilter now builds a filterFile predicate and passes it into resolveDocsNavigation, 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. derivationPathFilter is exported from config/project.ts so the resolver and inferNavigationFromContent share the same tinyglobby options instead of a second hand-rolled matcher.
  • New nav.unresolvable error path, with the fix string naming include/exclude when the collection has filters, plus regression tests in nav.test.ts and doctor.test.ts asserting 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, the mixed origin, and the required navigationOrigin field, 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 shows collections[0].pageCount 1 from countCollectionPages next to navigation.routedPages 0 from the resolver, in the same JSON report.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread packages/leadtype/src/cli/doctor.test.ts
Comment thread packages/leadtype/src/cli/nav.test.ts
Comment thread packages/leadtype/src/cli/nav.ts

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[!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:

  • generate staging fix (cli/generate.ts:1286-1296). hasSourceFilters folds source.filters into the fast-path bail-out. I confirmed source.filters is undefined unless include/exclude is non-empty (generate.ts:937-942), so the predicate doesn't over-stage, and the else branch routes through copySourceFiles with the same dot: true, expandDirectories: false globs the resolver uses.
  • Per-locale navigation resolution (config/navigation.ts:252-284, :342), answering the Codex thread. Non-default locales contribute unknown entries and locale-scoped nav.unresolvable diagnostics; 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). collectNavEntryPages extracted so buildNavigationFromNav's root loop enforces the same shadow check as groups.

ℹ️ Nitpicks

  • Still no changeset, and the scope grew again. 041d4454 changes generate's published output for any existing single-default-docs config that declares include/exclude — previously those filters were ignored and the pages shipped. That's a user-visible behavior change on top of nav.unresolvable, the mixed origin, and the required navigationOrigin field.
  • Empty admitted set fails with an internal-sounding error. include: ["guides"] against a bare guides/ directory (no glob) admits nothing; generate exits 1 — correctly, and the comment at nav.test.ts:286-291 is accurate about that — but the message is generateLLMFullContextFiles 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, and llms.txt + .well-known/llms.txt are written before the failure.
  • Carried, unchanged: nav still drops failure.fix (thread left open at cli/nav.ts:309-314); doctor still never calls findBlockingDiagnostic (only nav.ts:20, nav.ts:285 reference it); doctor's collections[].pageCount and navigation.routedPages still disagree in the same JSON report.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

if (
isDefaultSingleSource &&
!(hasFilters || hasSourceFilters || forceStaging)
) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 basePaths entry so includeMarkdown can resolve outside the mirror, or
  • Stage everything and apply source.filters at 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-1296hasSourceFilters routes the single default docs collection through copySourceFiles, which applies source.filters while copying. Excluded files are absent from tempDocsDir.
  • packages/leadtype/src/remark/plugins/include.remark.ts:863includeMarkdown resolves src relative to the including file's own directory. generate.ts:272 registers the plugin with no basePaths, 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 exclude that 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.filters govern 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.

Comment thread packages/leadtype/src/config/navigation.ts
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.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues in this round — 19ff52f is a clean, well-tested fix to my prior thread. The only thing withholding approval is the still-open <import> partials thread on generate.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.

  • nav now prints the diagnostic's fix hint (cli/nav.ts:311). The nav.unresolvable failure branch appends → ${failure.fix} when present, matching the blocking-diagnostic branch 20 lines above it. This retires the thread I opened at cli/nav.ts:309-314: the include/exclude guidance the resolver generates now reaches a nav user instead of only doctor --json. It writes to stderr, so --json output 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 only Nav 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.

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

@KayleeWilliams
KayleeWilliams merged commit 76af4e4 into main Aug 15, 2026
6 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant