Add first-class git source groups for multi-repo docs configs - #161
Add first-class git source groups for multi-repo docs configs#161KayleeWilliams wants to merge 3 commits into
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.
Important
The gitSource design is the right one and the implementation is clean — one expansion point, ids kept flat and global, acquisition cascade explicit about undefined vs falsy. Two behavioural gaps are worth closing before merge, and two prose claims in the new docs/JSDoc are not true of the code as written.
Reviewed changes — commit 4053ba5, all 11 files. Verified locally: bun run check-types clean; normalize.test.ts 32/32; both new cli.test.ts tests pass; biome check clean. I also ran leadtype sync against a real git fixture with a gitSource config and probed the id/validation edge cases below, so each inline finding has executed evidence rather than a read of the code.
Explicitly checked and not a problem: no consumer reads config.collections without going through normalizeDocsConfig. LoadedDocsConfig is constructed in exactly one place (generate.ts loadDocsConfigFromDir), which normalizes, and lintConfigLinks never touches collections/sources. A sources-only config cannot silently lose its collections.
The sync reporting loop this PR rewrote has no test coverage
runSyncCommand's output loop (cli/sync.ts:137-170) is entirely new — the repository#ref join into loaded.resolved.sources, the id column, the collections: continuation line, and the mutable-ref warning. The three existing sync tests (cli.test.ts ~2920-2962) all hit early exits (no config / no collections / local-only) and never reach it.
That matters specifically because of const id = resolved?.id ?? entry.source.repository (cli/sync.ts:154). If the join key ever drifts from how resolveSources builds ref — a defaulted vs. explicit main, say — the fallback quietly prints a repository URL where an authored source id belongs, and no test fails. One test asserting the full two-line stdout block plus the stderr warning for a gitSource config would lock in the format the new docs promise. I confirmed a real fixture produces exactly the documented shape, so the assertion is cheap to write.
docs/pipeline/collections.mdx:54 is now stale (outside the diff)
The field table still lists inheritConfig as `true | object`, but this PR widens SourceConfigInheritance to boolean and the new "Source groups" section 65 lines below documents inheritConfig: false as the way a child opts out. The row should read `boolean | object` so the table and the prose on the same page agree.
ℹ️ Nitpicks
docs/pipeline/collections.mdx:153-159— the samplesyncblock indentscollections:by 9 spaces; the code emits 10 (cli/sync.ts:157). The sample also shows a relative.leadtype/c15t, butsyncprints the absolute path it resolved against the config dir. And thecloned/collections:lines go to stdout while the warning goes to stderr, which the contiguous block hides.cli/generate.ts:1345-1350—validateSourceConfigInheritanceis now called with a source id, but its message template sayscollection "<id>". A user getscollection "c15t"for something they authored as a source.docs/pipeline/agent-setup-prompts.mdx:74still instructs agents to author the repetitive flat form for the "multiple content areas, one repo" case ("share onecacheDirclone across collections of the same repo") — exactly whatgitSourcereplaces. Follow-up, not a blocker for this PR.
Claude Opus | 𝕏
| A failed acquisition names every collection that depended on it, so one clone | ||
| error does not read as one collection's problem. |
There was a problem hiding this comment.
This isn't true for an actual acquisition failure. gitError (sync.ts:266-278) builds only:
`git ${action} failed for ${source.repository}@${source.ref}: ${detail}`
collectionKeys is included by exactly two messages — the "source not synced" pre-check and the --offline cache-miss pre-check — and both fire before any clone is attempted.
Verified by pointing a gitSource with docs + changelog children at a nonexistent repository and running leadtype sync:
git clone --branch main failed for /tmp/…/does-not-exist@main: fatal: repository '/tmp/…/does-not-exist' does not exist
No collection names. So a bad ref, a network error, or an auth failure names only the repository — precisely the "one clone error reads as one collection's problem" case this sentence promises is handled.
Worth noting the new test at cli.test.ts:2689-2722 ("names the dependent collections when a source cannot be acquired") runs generate without --sync, so it only exercises the not-synced pre-check and never reaches gitError — its name and its inline comment ("One failed clone must name everything that depended on it") assert something it doesn't cover.
Either thread collectionKeys into gitError and keep the sentence, or narrow it:
| A failed acquisition names every collection that depended on it, so one clone | |
| error does not read as one collection's problem. | |
| When a source has not been synced, the error names every collection that | |
| depended on it, so one missing clone does not read as one collection's problem. |
There was a problem hiding this comment.
Prose narrowed correctly in f394e04; leaving open for the second half — cli.test.ts:2689 still names itself "when a source cannot be acquired" with a "One failed clone must name everything" comment, while running generate without --sync only reaches the not-synced pre-check.
Claude Opus | 𝕏
There was a problem hiding this comment.
Reviewed changes — incremental re-review of f662396, which adds the sparse checkout option and migrates apps/c15t-example onto it (deleting the hand-rolled git + patch setup).
sparsecascades consistently through every layer I traced:gitSource→expandGitSources(child spread carriessparsealongsiderepository/ref/cacheDir, gated onundefinednot truthiness) →resolveSources→resolveCollection→cloneRemote.sparse: []collapses to "full repository" identically at all five sites, so there's no state where the recorded path set disagrees with the checked-out one.- Cache invalidation is right and backward compatible.
sameSparsecompares as sets (order-insensitive, which matches git),manifestMatchesgates reuse on it, and a manifest written before this PR — nosparsekey — still matches a non-sparse source, so existing caches don't spuriously re-clone. - The four new
sync.test.tscases are falsifiable rather than decorative: the drift test assertsstatus === "fresh", which would read"cached"if thesameSparsegate were removed, and the sparse-checkout assertion pins the exact argv including the--end-of-options guard and thecwd. sparseis build/CLI-only — the runtimecreateDocsSourcepath never clones — so there is no build/runtime config parity gap to mirror here.- I checked the example app's new build-order assumption and it holds:
apps/c15t-example/leadtype.config.tsimportsgitSourcefrom the bare"leadtype"specifier, whoseexportsmap points atdist/, butapps/c15t-example/tsconfig.jsonmapsleadtype→packages/leadtype/src/index.tsand bun honours it. Confirmed by importing the config with nodist/present —setup:realstill works without a prior build. - Verified locally on
f662396:vitest run src/sync/sync.test.ts src/config/normalize.test.ts→ 62 passed;bun --filter leadtype check-typesclean;ultracite checkclean across every touched directory. The changeset already documentssparseaccurately.
The five threads from the 4053ba5 review are all still unaddressed in this commit, so they stay open and I have not re-raised them here — that's also why this isn't an approval.
ℹ️ Everything I found in this commit is documentation drift. No implementation concerns.
📄 apps/c15t-example/README.md wasn't updated with the migration
The file isn't in the diff, so there's no line to anchor to, but three of its claims are now false:
- L43 — "By default the fixture uses the c15t docs-navigation PR branch". The new config pins
2b89e689458497bc985862db21f6b4b03a918e46(C15T_REFstill overrides it, so only the default is wrong). - L44-47 — "Setup applies
patches/c15t-docs-config-framework-navigation.patchafter every refresh so the example dogfoods Leadtype's framework navigation helper against real c15t content". This commit deletes that patch file andsetup-real-content.tsno longer patches anything. A reader following the README will go looking for a file that isn't there. - L47-49 — "
docs:generateandgenerate:realrun the source-config drivenleadtype generatepath againstdocs/docs.config.ts". Generate now runs againstleadtype.config.tsat the app root;docs/docs.config.tsis reached throughinheritConfig: true, which is a different (and more interesting) story worth stating correctly.
While updating it: dropping the patch also drops the only place defineFrameworkNavigation was exercised against real c15t content. llm.test.ts still covers it in unit form, so nothing regresses functionally — but if the dogfooding was the point of the patch, that intent is now lost silently. Worth a line in the README or the changeset either way.
ℹ️ Nitpicks
docs/reference/cli.mdx:319-331documents the.leadtype-sync.jsonshape with a full example object, and this commit adds asparsekey to that manifest. The example still shows onlyversion/repository/ref/commit/syncedAt. Sincecollections.mdx:171now tells readers "the path set is recorded in the sync manifest", the reference page is the natural place to look for it.
Claude Opus | 𝕏
f662396 to
a5c1db1
Compare
a5c1db1 to
79d0ad7
Compare
Remote acquisition is first-class in leadtype, but the config is collection-first: every collection carries `repository`, `ref`, and `cacheDir` even when several come from the same repository. So a config states the acquisition three times for one clone, a shared spread hides the relationship rather than expressing it, and a reader has to know that matching `(repository, ref)` pairs are deduped internally. `gitSource()` declares the acquisition once and nests the content beneath it. The split follows ownership: the source owns `repository`/`ref`/`cacheDir`, one clone lifecycle for all its children, and the default inheritance policy; each collection owns its `dir`, include/exclude, `routePrefix`, mounts, navigation, schema, and its own inheritance exception — including `inheritConfig: false` to opt out of a source-level default. Both forms normalize to the same source graph, so nothing downstream can tell them apart, and `sources` may be used alongside a flat `collections` map. Collection ids stay global rather than scoped to their source: they name staging mounts, error messages, and JSON output, so a silently namespaced id would surface in all three. Two sources claiming one id is an error naming both. So is declaring one `(repository, ref)` under two source names — that is one acquisition written twice, and merging is what was meant. Making pinning visible: a named source keeps its authored id through the resolved graph, so `leadtype sync` reports each source with its dependent collections and warns when one tracks a mutable ref rather than a pinned commit. `generate --json` reports the same graph with the same ids, and a failed acquisition names every collection that depended on it. `SourceConfigInheritance` widens from `true` to `boolean` so a collection can say no. The repo's own snippet typechecking caught that gap in the docs before the type did.
The c15t example never used leadtype's own acquisition. It hand-rolled a sparse clone in a shell script and ran `generate --src <clone>`, so the app that exists to dogfood the recommended pinned-source shape was the one app not using it. Migrating it found the reason: `leadtype sync` had no way to clone part of a repository, and c15t is a monorepo whose docs additionally read `packages/` through `<AutoTypeTable path="./packages/…">`. `sparse` closes that gap. Leadtype clones blobless with `--sparse` and then selects the paths, so git fetches only the blobs behind them — 17 MB and about three seconds for c15t instead of the whole repository. Two rules keep it honest: collections sharing one acquisition must agree on the path set, since one checkout has one set and silently taking the first would leave the other collection reading a directory that isn't there; and the set is recorded in the sync manifest, so adding a path re-clones rather than reusing a cache that looks complete but isn't. The example now declares a `leadtype.config.ts` with `gitSource`, and its setup script is a `leadtype sync` call. That also puts the site/source split where the docs say it belongs: c15t owns navigation and frontmatter schema (inherited via `inheritConfig`), this app owns identity, agent surfaces, and the `llms.sections` starting points — which matter here, because the derived fallback picked the first twelve pages in navigation order and for a 250-page site that is arbitrary. Two things dropped out along the way. The local dogfood patch is gone: it existed to test `defineFrameworkNavigation` against c15t's real config, and that has since landed upstream. And the pinned ref is now a commit SHA rather than a branch — the branch it named no longer exists, so `setup:real` was already broken, and a SHA is what this repo's own docs and `doctor` tell users to use. Verified end to end: sync + generate produces 250 pages from real c15t content with every type table resolved.
A named source could silently lose its id. Expansion spreads the flat collections map in first, so when a flat remote collection and a `gitSource` share a `(repository, ref)`, the flat one reached source resolution first and created the source under `repository#ref` — and that id is what sync output, doctor, and `generate --json` print. The named source now claims it when it arrives. Child collections could set acquisition fields. `GitSourceCollection` omits `repository`/`ref`/`cacheDir`/`sparse` at the type level, but this validator exists precisely for `.js`/`.mjs`/`.cjs` configs that have no type checking — and it treated them as ordinary collection fields, cascading the source's over them. Now rejected, naming the field and the source that owns it. Three overstatements corrected. "Nothing downstream can tell them apart" is wrong by design: the resolved id differs by authoring form, and that difference is deliberate and visible in three places — now documented rather than denied. "A failed acquisition names every dependent collection" holds for the pre-clone cache checks, not for a clone that fails partway: git reports against `repository@ref` and knows nothing about collections. And `sparse` was missing from the ownership table, the page's own answer to which side owns what, despite being the one field a child collection cannot set.
79d0ad7 to
f394e04
Compare
There was a problem hiding this comment.
Reviewed changes — incremental re-review of the third commit, "Address review on the git source groups branch". This is the first push since f662396 that changes PR-authored content (the two before it were rebase-only). The branch was force-pushed again, so of the 16 files in the raw 79d0ad7..f394e04 delta only 8 are PR-authored; the generate.ts re-normalization/i18n-gate/inferLlmsBlocks hunks, the normalize.test.ts /changelog→/releases change, and the collections.mdx:356/cli.mdx:124 edits all come from the base branch and I've excluded them.
All four of the remediations I could verify hold up:
- The authored-name hand-off in
resolveSourcesis sound. I went looking for a stale-read bug and couldn't find one:existingis the same object reference already insources, so the mutation is visible to every later consumer;sourceIdByCollectionis patched both for the already-registeredcollectionKeysand for the current one;ResolvedGitSource.idisn'treadonly; no consumer snapshots.idbefore the loop finishes; andcli/sync.tskeys its join map onrepository#refrather thanid, so the rename can't desync the reporting loop. Flat-first ordering is structurally guaranteed byexpandGitSourcesspreading{...existing}before merging source children, so the resulting id is deterministic rather than authoring-order dependent — and the duplicate-named-sourcethrowstill fires in every ordering. The new test atnormalize.test.ts:430-458is falsifiable: without the fix it readshttps://github.com/acme/acme.git#main. - The narrowed failure prose in
collections.mdx:195-198is now accurate.syncOne'smissingandofflinepre-checks (sync/sync.ts:527-543) both gate on!(hasCheckout && manifestMatches)and both interpolatesource.collectionKeys, so "a missing or stale cache names every collection that depends on it" is true — including the stale case, which is the part the earlier wording got wrong. AndgitErrorreally does only formatrepository@ref. validateGitSourcesnow rejects children that setrepository/ref/cacheDir/sparse, which is what closes the gap betweenGitSourceCollection'sOmitand the untyped.js/.mjs/.cjspath. One blind spot and the missing test are noted inline.- The
llm.tsJSDoc overstatements are gone and the ownership table now carries thesparserow.
Verified locally at f394e04: bun --filter leadtype check-types clean; vitest run src/config/normalize.test.ts src/sync/sync.test.ts → 63 passed (normalize 33, up from 32 by exactly the one new test).
I've replied to and resolved five of the six threads from the previous reviews. One stays open — see below — so this isn't an approval.
ℹ️ Everything new in this commit is a documentation-accuracy issue plus one test gap. No correctness concerns in the implementation.
🧵 collections.mdx:164-165 is still only half-addressed
The prose half of that thread is fixed. The second concern isn't: cli.test.ts:2689 is still named "names the dependent collections when a source cannot be acquired" with the comment "One failed clone must name everything that depended on it", while the test runs generate without --sync. It therefore only ever reaches the not-synced pre-check and never reaches gitError — so it doesn't exercise acquisition failure at all. Either rename it to match what it tests ("names the dependent collections when a source is not synced") or add --sync against an unreachable repository. I've left that thread open rather than resolving it.
📄 Is doctor landing in a sibling PR?
The inline comment covers the four sites. The decision I can't make for you: the PR body makes the same claim, so either leadtype doctor is a real command arriving elsewhere in the #157 stack and these docs are deliberately ahead of the code, or every reference should say sync. If it's the former, a note in the PR body would keep the next reviewer from filing this again.
ℹ️ Unchanged since the last review
Not re-raising these — the previous review bodies cover them and none were touched by this commit: the leadtype sync reporting loop still has zero test coverage (cli/sync.ts is untouched since 4053ba5, and all three sync tests in cli.test.ts hit early exits); collections.mdx:55 still types inheritConfig as `true | object` though it's now boolean | object; apps/c15t-example/README.md's three stale claims (including the silent loss of defineFrameworkNavigation dogfooding against real c15t content, now that the patch file is deleted); the .leadtype-sync.json example at cli.mdx:319-327 still lacking sparse; and docs/pipeline/agent-setup-prompts.mdx:74 still steering agents to the flat form for the one-repo-many-areas case.
Claude Opus | 𝕏
| visible difference is the source's **id**: a named `gitSource` keeps its | ||
| authored name (`c15t`), while the flat form is identified by what it is | ||
| (`https://github.com/c15t/c15t.git#main`). That id is what `leadtype sync` | ||
| prints, what `doctor` reports, and what `generate --json` emits — which is the |
There was a problem hiding this comment.
doctor isn't a command. bun src/cli.ts doctor → unknown command: doctor; the real set is init/generate/sync/lint/mcp/score/help, and there's no docs/reference/doctor.mdx. A repo-wide grep for "doctor" hits only this PR's own new prose, ultracite doctor in AGENTS.md/CLAUDE.md, and one stale line in generated apps/sveltekit-example/static/sitemap.md.
Worth flagging because this commit is the one that removed the other overstatements from the same paragraph, so it's easy to read as already-verified. Four sites need the same decision:
- here (
collections.mdx:140) — "whatdoctorreports" packages/leadtype/src/llm/llm.ts:811— the newgitSourceJSDoc, same sentence structurepackages/leadtype/src/config/normalize.ts:246— code comment on the authored-name hand-off ("sync output, doctor, andgenerate --json")apps/c15t-example/leadtype.config.ts:50— "leadtype doctorflags a mutable ref"; predates this commit but is wrong in the same way, and it's the one a reader is most likely to try
If the intent was sync, note that it's accurate for the mutable-ref claim specifically — cli/sync.ts:159-169 does warn on refKind === "mutable".
| for (const [key, child] of Object.entries(entry.collections)) { | ||
| if (!isPlainRecord(child)) { | ||
| continue; | ||
| } | ||
| // Indexed through a record view: the type omits these fields, which is | ||
| // exactly why an untyped config can still carry them. | ||
| const childRecord = child as Record<string, unknown>; | ||
| for (const owned of ["repository", "ref", "cacheDir", "sparse"]) { | ||
| if (childRecord[owned] !== undefined) { | ||
| throw new Error( | ||
| `docs config at "${configPath}": collection "${key}" sets "${owned}", which its source "${sourceId}" owns. Move it onto the gitSource, or declare the collection in the flat "collections" map instead.` | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This rejection loop has no test. That's a notable asymmetry: the sibling normalize.ts fix in this same commit did get one (normalize.test.ts:430-458), and this guard is less testable by other means — it's the only thing making the untyped-config path agree with GitSourceCollection's Omit, so check-types passing tells you nothing about it. The error message is specific enough (collection "<key>" sets "<field>", which its source "<id>" owns) that a single expect(...).toThrow per owned field would pin it cheaply.
While you're in here: the isPlainRecord guard on L1379-1381 silently continues, which leaves a hole. isPlainRecord (L584-586) is typeof value === "object" && value !== null && !Array.isArray(value), so a child whose typeof is "function" — or an array with named own properties — skips the owned-field check entirely. The cascaded shadow-validation below doesn't catch it either: {...child, repository, ...sparse} copies the child's own enumerable props including a smuggled ref, forces only repository/sparse, and validateCollections has no notion of source-ownership so a well-typed ref string passes. Then expandGitSources (normalize.ts:359-362) spreads the raw child again and only overwrites ref when the source set one — which it usually hasn't, since ref defaults to "main".
I confirmed this end to end through loadLeadtypeConfig (the entry point both generate and sync use): a .mjs config whose docs collection is a function object carrying .dir/.routePrefix/.ref = "evil-ref-from-child", under a gitSource that declares no ref, loads with no error and resolves to sources[0].ref === "evil-ref-from-child". The identical fixture with a plain-object child throws correctly. Narrow — nobody writes a function collection on purpose — but a misauthored untyped config is precisely the threat model the comment above cites, and the failure mode is silent rather than loud. Rejecting non-plain-record children outright would be both stricter and shorter than skipping them.

Closes #153. Stacked on #160, part of #157.
Remote acquisition is first-class, but the config is collection-first: every collection carries
repository,ref, andcacheDireven when several come from the same repository. So a config states the acquisition three times for one clone, a shared spread hides the relationship rather than expressing it, and a reader has to know that matching(repository, ref)pairs are deduped internally.The source owns acquisition and the default inheritance policy; each collection owns its directory, route prefix, navigation, and any inheritance exception. Both forms normalize to the same source graph, and
sourcesmay be used alongside a flatcollectionsmap.Design decisions worth reviewing
Collection ids stay global rather than scoped to their source. They name staging mounts, error messages, and JSON output, so a silently namespaced id would surface in all three. Two sources claiming one id is an error naming both. So is declaring one
(repository, ref)under two source names — that is one acquisition written twice, and merging is what was meant.Pinning is made visible. A named source keeps its authored id through the resolved graph, so
leadtype syncreports each source with its dependent collections and warns when one tracks a mutable ref rather than a pinned commit.generate --jsonreports the same graph with the same ids, and a failed acquisition names every collection that depended on it.sparse, and migrating the c15t exampleThe c15t example never used leadtype's own acquisition — it hand-rolled a sparse clone in a shell script and ran
generate --src <clone>. The app that exists to dogfood the pinned-source shape was the one app not using it.Migrating it found the reason:
leadtype synchad no way to clone part of a repository, and c15t is a monorepo whose docs additionally readpackages/through<AutoTypeTable path="./packages/…">. Sosparseis here too — leadtype clones blobless with--sparseand then selects the paths, so git fetches only the blobs behind them. 17 MB and about three seconds for c15t instead of the whole repository.Two rules keep it honest:
The example now declares a
leadtype.config.tswithgitSource, and its setup script is aleadtype synccall. That also puts the site/source split where the docs say it belongs: c15t owns navigation and frontmatter schema (inherited viainheritConfig), this app owns identity, agent surfaces, and thellms.sectionsstarting points — which matter here, because the derived fallback picked the first twelve pages in navigation order, and for a 250-page site that is arbitrary.Two things dropped out along the way:
defineFrameworkNavigationagainst c15t's real config, and that has since landed upstream.setup:realwas already broken onmain; a SHA is what this repo's own docs anddoctortell users to use.Verified end to end: sync + generate produces 250 pages from real c15t content with every type table resolved.