Make sync consume the resolved source graph - #178
Conversation
`leadtype sync` re-derived its own source graph from the collections map (resolveRemoteSources), disagreeing with the graph resolveSources reports to doctor and `generate --json`. Three bugs stemmed from the duplication: a sparse disagreement normalized into a coherent, wrong graph that sync then rejected; a mixed explicit/default cacheDir was backfilled by normalize but rejected by sync; and a git source named "local" produced two sources with the same id. The agreement checks now live in resolveSources — sparse sets and cache dirs (compared as resolved paths, so an explicit cacheDir spelling out the default stays valid) fail at normalize time, and a duplicate source id is rejected outright. Sync's derivation is gone: projectRemoteSources is a projection of resolved.sources, and syncSources takes the graph itself, so exactly one place decides source identity, sparse sets, and cache dirs. The sync CLI drops its repo#ref remapping — the id, ref kind, and dependents come straight off the synced source. New agreement tests assert, across a matrix of authoring shapes, that the graph sync acts on is the graph normalize reports and that resolveCollection reads from the same checkouts.
|
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: 0e50cdde22
ℹ️ 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.
ℹ️ No correctness issues found in the consolidation — two rough edges and a missing changeset.
Reviewed changes — full diff of the single commit 0e50cdd, plus the surrounding call graph (config/load.ts, config/project.ts, config/inherit.ts, cli/generate.ts, cli/doctor.ts) and a local run of the package test suite.
resolveRemoteSourcesdeleted,projectRemoteSourcesadded — sync no longer walks the collections map to rebuild its own(repository, ref)graph; it filtersresolved.sourcesto git sources and resolvescacheDiragainst the config dir, copyingid,refKind,sparse, andcollectionKeysthrough unchanged.syncCollections→syncSources— the option is nowsources: readonly ResolvedSource[]instead of a collections map, andSyncSourceResult.source/SyncResult.skippedcarry the newSyncSourcetype (ResolvedRemoteSource & { id, refKind }).- Agreement validation moved into
resolveSources— mixed explicit/defaultcacheDirand disagreeingsparsesets now throw at normalize time, and a newassertUniqueSourceIdsrejects a git source authored aslocalbeside repository-less collections. cli/sync.tsremapping removed — therepo#ref→ resolved-source lookup and its?? entry.source.repositoryfallback are gone; the output loop readsid,refKind, andcollectionKeysstraight off the synced source.- Both callers hand over the first-pass graph —
cli/sync.tsandcli/generate.tspassresolved.sourcesfrom the normalization that ran during config load. - Tests — three regression tests plus three parity tests in
normalize.test.ts, and a six-shape cross-subsystem matrix insync.test.ts.
I verified the load-bearing invariant myself: the configDir at both syncSources call sites (path.dirname(loaded.path)) is the same one load.ts:1019 passed to normalizeDocsConfig, expandGitSources flattens sources groups onto collections before resolveSources validates them, and inherit.ts never touches repository/ref/cacheDir/sparse — so keeping the first-pass graph across the re-normalize in generate.ts:1710 is sound. syncSources and projectRemoteSources are not public API, so no caller can supply an unvalidated graph. The 10 cli.test.ts failures I saw locally are environmental (packages/leadtype/dist isn't built); sync.test.ts and normalize.test.ts pass.
⚠️ Three new config-load errors ship without a changeset
This PR turns three previously-accepted config shapes into hard throws at config load, and adds no .changeset entry. Every prior commit on this stack that touched normalize.ts shipped one (canonical-config-api.md, git-source-groups.md, resolve-project.md), and the existing entries don't cover this: git-source-groups.md documents the sparse agreement rule, but nothing describes the explicit-vs-default cacheDir rule or the local id collision — nor that the error surface moved from leadtype sync to config load, so doctor, generate without --sync, and createDocsProject now fail on configs they previously tolerated.
Technical details
# Missing changeset for the validation move
## Affected sites
- `.changeset/` — no entry added by this PR; `git log --name-only` shows every prior
`normalize.ts` commit on this stack paired with one.
- `packages/leadtype/src/config/normalize.ts:270-291` — new explicit-vs-default `cacheDir` rejection.
- `packages/leadtype/src/config/normalize.ts:358-372` — new duplicate-source-id rejection.
- `docs/concepts/config-model.mdx:87-115` — documents the source graph but not the agreement
rules a shared acquisition must satisfy.
## Required outcome
- A changeset entry describing which config shapes now fail, and that they fail at config load
rather than at `leadtype sync`.
- Decide whether `docs/concepts/config-model.mdx` should state the agreement rules alongside the
source-graph shape it already documents.
## Open questions for the human
- Is this stack releasing as one changeset owned by #167, or does each PR carry its own? If the
former, the `resolve-project.md` entry needs amending rather than a new file.ℹ️ The rewritten leadtype sync output has no test covering it
runSyncCommand has no test anywhere — there is no cli/sync.test.ts, and cli.test.ts never drives the sync subcommand. This PR rewrites its output construction (cli/sync.ts:145-155), deleting the resolvedById lookup and both ?? fallbacks, on the strength of a claim in the PR body that the output lines are unchanged. Nothing in the suite would catch it if a named gitSource group stopped reporting its authored id, or if the mutable-ref warning stopped firing.
Technical details
# `runSyncCommand` output is uncovered
## Affected sites
- `packages/leadtype/src/cli/sync.ts:145-163` — output loop and mutable-ref warning, rewritten
in this PR, with no test exercising it.
## Required outcome
- A test that drives `runSyncCommand` with a fake `SyncCliIo` and a config containing a named
`gitSource` group plus an anonymous flat collection, asserting the printed id is the authored
source name for one and `repository#ref` for the other, and that a mutable ref produces the
stderr warning.
## Suggested approach (optional)
- `syncSources` takes an injectable `runner`, but `runSyncCommand` doesn't thread one through, so
the test needs a real temp dir + config file, or a small seam for the runner. The existing
`seedFakeCheckout` helper in `sync.test.ts` covers the checkout side.ℹ️ Nitpicks
normalize.ts:239-247(the pre-existing both-explicitcacheDircheck) compares raw authored strings while the sibling check this PR adds at:270-291comparespath.resolved paths. Not a regression — the string check already ran at load time before this PR — but now that this function is the sole owner ofcacheDiragreement, the two halves disagree on strictness:"./vendor/acme"beside"vendor/acme"is rejected, while"./vendor/acme"beside the default that resolves to the same place is accepted.
Claude Opus | 𝕏
When normalizeDocsConfig got configPath but no configDir, the cacheDir-equivalence check fell back to process.cwd(), so an explicit absolute cacheDir spelling out the config-relative default was rejected or accepted depending on invocation cwd. Derive the fallback base from path.dirname(configPath), keeping cwd as the last resort only when neither is known. Also pin the sync graph agreement test against expectations derived independently from the resolved graph's git entries — it previously compared projectRemoteSources against itself, so it could never fail.
There was a problem hiding this comment.
ℹ️ No new issues in
0e3658e— both prior inline findings are fixed correctly. Holding off on approval only because the changeset gap flagged in the previous review is still open.
Reviewed changes — 0e3658e, the single commit since the prior pullfrog review of 0e50cdd, plus the surrounding callers (config/load.ts, config/project.ts, cli/generate.ts) and a local run of normalize.test.ts + sync.test.ts and tsgo --noEmit.
cacheDirequivalence no longer rebases onto cwd —resolveSourcescomputesresolveBaseasconfigDir ?? (configPath ? path.dirname(configPath) : "."), so the accept/reject decision is derived from the config being normalized rather than the process working directory.- Added a discriminating regression test for it —
normalize.test.ts:366-404drives the sharedconfigPath-onlynormalize()helper with an absolute cacheDir equal to the config-relative default. That case throws under the oldresolveBase = "."(vitest's cwd ispackages/leadtype, not/repo), and the sibling test at:336-364could not catch it because its relative default dir resolves identically under any base. - Removed the tautological projection assertion —
sync.test.ts:990-1008now derives its expectation fromresolved.sources.filter(kind === "git")withpath.resolve(configDir, cacheDir ?? defaultCacheDir(...))computed inline, instead of comparingsyncSources' output against the sameprojectRemoteSourcescall it made internally.
I confirmed the resolveBase change is behaviour-neutral for every in-repo production path: load.ts:1019, project.ts:316-320, project.ts:413-416, and generate.ts:1710-1713 all pass a real configDir, so only external configPath-only callers and the test helper see a different base. normalize.test.ts + sync.test.ts pass (82 tests) and tsgo --noEmit is clean for the package.
Claude Opus | 𝕏

Stacked on #167. Follows the graph #157 made visible to its conclusion: sync now consumes it instead of re-deriving its own.
The evidence
Two functions decided what a source is.
resolveSources(config/normalize.ts) builds the resolved graph doctor andgenerate --jsonreport;resolveRemoteSources(sync/sync.ts) rebuilt a second one from the collections map forleadtype syncto clone from. Same dedup key, different rules — and three confirmed-by-execution bugs live in the gap:(repository, ref)without comparingsparse, sosparse: ["docs"]besidesparse: ["packages"]normalized into one source claiming["docs"]serves both — while sync threw. Doctor andgenerate --jsonpresented a coherent, wrong graph for a config that cannot sync.existing.cacheDir ??= …) when only one collection setcacheDir, resolving to one source at the explicit dir; sync compared resolved paths, gave the unset collection the default cache dir, and threw "different cacheDir values" — contradicting normalize's own comment that cacheDir conflicts are "rejected here rather than at clone time".localbeside a repository-less collection produced twoid: "local"entries inresolved.sources— and ids are the join key for sync output, doctor, and every JSON surface.The design
Validation moves into
resolveSources. A shared acquisition must agree on its sparse set (compared as a set — order is irrelevant to git sparse-checkout) and its cache dir (compared as resolved paths, so an explicit cacheDir that spells out the default location stays valid, as it always synced). Duplicate source ids are rejected outright. Each error names the collections, the disagreement, and the fix, at normalize time — where doctor and JSON read from.Sync's derivation becomes a projection.
resolveRemoteSourcesis gone.projectRemoteSourcesfiltersresolved.sourcesto git sources and resolvescacheDiragainst the config dir — nothing else.syncCollectionsbecomessyncSourcesand takesresolved.sourcesitself; both callers (leadtype sync,generate --sync) hand it the graph from the first normalization pass — the only pass that saw authored source names. Exactly one place now decides source identity, sparse sets, and cache dirs. The sync CLI's repo#ref remapping is deleted: the synced source carriesid,refKind, andcollectionKeysdirectly.External behavior is otherwise unchanged. Configs that synced before sync identically — same clone layout, same manifests, same output lines. Configs sync rejected are now rejected at config load, with messages at least as specific. A git source named
localwith no local collections stays valid, since there is no collision to misread.Tests
Each bug has a regression test in normalize.test.ts, plus the parity cases that must keep working (order-insensitive sparse, explicit-cacheDir-equals-default,
local-named source with no local collections). New in kind: a cross-subsystem agreement suite in sync.test.ts asserts, for a matrix of authoring shapes (flat, gitSource group, mixed, explicit/default cacheDir, sparse variants, local+remote), that the graph sync acts on is the graph normalize reports and thatresolveCollectionreads every collection from its source's checkout — the class of test whose absence let each subsystem pass while disagreeing with the other.Verification
883 tests pass (
bun run test), lint clean,tsgo --noEmitclean for the package.bun run check-typesat the workspace level still trips the pre-existing parallel-build race on this stack; that fix is #166, offmain.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.