Skip to content

Make sync consume the resolved source graph - #178

Open
KayleeWilliams wants to merge 2 commits into
dx/157-resolve-projectfrom
dx/sync-resolved-graph
Open

Make sync consume the resolved source graph#178
KayleeWilliams wants to merge 2 commits into
dx/157-resolve-projectfrom
dx/sync-resolved-graph

Conversation

@KayleeWilliams

@KayleeWilliams KayleeWilliams commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

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 and generate --json report; resolveRemoteSources (sync/sync.ts) rebuilt a second one from the collections map for leadtype sync to clone from. Same dedup key, different rules — and three confirmed-by-execution bugs live in the gap:

  1. Sparse mismatch silently misrepresented. Normalize merged two collections on one (repository, ref) without comparing sparse, so sparse: ["docs"] beside sparse: ["packages"] normalized into one source claiming ["docs"] serves both — while sync threw. Doctor and generate --json presented a coherent, wrong graph for a config that cannot sync.
  2. Mixed explicit/default cacheDir. Normalize backfilled (existing.cacheDir ??= …) when only one collection set cacheDir, 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".
  3. Source id collision with the implicit local source. A git source the user names local beside a repository-less collection produced two id: "local" entries in resolved.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. resolveRemoteSources is gone. projectRemoteSources filters resolved.sources to git sources and resolves cacheDir against the config dir — nothing else. syncCollections becomes syncSources and takes resolved.sources itself; 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 carries id, refKind, and collectionKeys directly.

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 local with 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 that resolveCollection reads 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 --noEmit clean for the package. bun run check-types at the workspace level still trips the pre-existing parallel-build race on this stack; that fix 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.

`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.
@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: 7c296b21-040f-40e5-812b-01cdf5f1fb50

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

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

ℹ️ 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.

  • resolveRemoteSources deleted, projectRemoteSources added — sync no longer walks the collections map to rebuild its own (repository, ref) graph; it filters resolved.sources to git sources and resolves cacheDir against the config dir, copying id, refKind, sparse, and collectionKeys through unchanged.
  • syncCollectionssyncSources — the option is now sources: readonly ResolvedSource[] instead of a collections map, and SyncSourceResult.source / SyncResult.skipped carry the new SyncSource type (ResolvedRemoteSource & { id, refKind }).
  • Agreement validation moved into resolveSources — mixed explicit/default cacheDir and disagreeing sparse sets now throw at normalize time, and a new assertUniqueSourceIds rejects a git source authored as local beside repository-less collections.
  • cli/sync.ts remapping removed — the repo#ref → resolved-source lookup and its ?? entry.source.repository fallback are gone; the output loop reads id, refKind, and collectionKeys straight off the synced source.
  • Both callers hand over the first-pass graphcli/sync.ts and cli/generate.ts pass resolved.sources from 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 in sync.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-explicit cacheDir check) compares raw authored strings while the sibling check this PR adds at :270-291 compares path.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 of cacheDir agreement, 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.

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

Comment thread packages/leadtype/src/config/normalize.ts Outdated
Comment thread packages/leadtype/src/sync/sync.test.ts
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.

@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 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 changes0e3658e, 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.

  • cacheDir equivalence no longer rebases onto cwdresolveSources computes resolveBase as configDir ?? (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 itnormalize.test.ts:366-404 drives the shared configPath-only normalize() helper with an absolute cacheDir equal to the config-relative default. That case throws under the old resolveBase = "." (vitest's cwd is packages/leadtype, not /repo), and the sibling test at :336-364 could not catch it because its relative default dir resolves identically under any base.
  • Removed the tautological projection assertionsync.test.ts:990-1008 now derives its expectation from resolved.sources.filter(kind === "git") with path.resolve(configDir, cacheDir ?? defaultCacheDir(...)) computed inline, instead of comparing syncSources' output against the same projectRemoteSources call 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.

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

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