Skip to content

Make baseUrl a config field instead of a repeated knob - #182

Open
KayleeWilliams wants to merge 9 commits into
dx/157-resolve-projectfrom
dx/base-url-config
Open

Make baseUrl a config field instead of a repeated knob#182
KayleeWilliams wants to merge 9 commits into
dx/157-resolve-projectfrom
dx/base-url-config

Conversation

@KayleeWilliams

@KayleeWilliams KayleeWilliams commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #167. Closes the last knob the docs audit found on the common path.

The evidence

baseUrl was the one value every snippet had to repeat in both forms of the common path — generate --base-url https://… for the CLI, createDocsProject({ baseUrl }) for the runtime — because it was not a config field. Every other value the two halves share already flows through the resolved config; this one rode along as a flag and an argument, so a fresh scaffold carried it three times (source config comment aside): lib/source.ts, the docs:generate script, and the post-scaffold generate run. Three copies of one fact is exactly the drift the config model exists to prevent.

baseUrl in the config

A site-owned, top-level field next to product:

export default defineDocsConfig({
  product: { name: "Acme", tagline: "Acme does one useful thing." },
  baseUrl: "https://acme.dev",
});

With it set, the scaffolded path has zero knobs: init writes the value once into docs/docs.config.ts, the docs:generate script is leadtype generate --src . --out public, and the runtime is createDocsProject() with no arguments.

Ownership. The field lives on the shared config shape, but the ownership table assigns it to the site half: where a site publishes is the consuming site's fact, so it is never inherited via inheritConfig, and a source-owned docs.config.* read by several sites should leave it unset — each site's own config, or its explicit override, supplies it. In a single-repo project the one config file is both owners at once, which is why init writes it there. Documented in the config-model page.

Precedence, explicit-wins. --base-url / createDocsProject({ baseUrl }) > config field > the deployment-URL env fallbacks that applied before. No behavior change for existing flag users or for configs without the field.

Validation and normalization. Absolute http(s) URL, optional path prefix, no query or fragment, trailing slashes stripped at load — the same normalization normalizeBaseUrl applies to env fallbacks, so URL joins can never produce //. Invalid values fail with the config path and field named, in the loader's error style.

Provenance and diagnostics. The resolved config always carries a baseUrl provenance entry — explicit with the config path, or default naming the env chain. doctor reports the resolved URL and its origin (baseUrl: https://acme.dev (explicit) in human output, a config.baseUrl value/origin pair in --json), and generate --explain reports the localhost/env fallback when nothing was authored anywhere, with the field to set.

Verification

  • 843 tests pass (56 files), including new coverage for: config parsing + URL validation, trailing-slash normalization, flag-over-config and argument-over-config precedence, feeds satisfied by the config field with env vars cleared, provenance entries, --explain output (present on fallback, quiet when authored), doctor value/origin reporting, and the init scaffold carrying baseUrl exactly once.
  • tsgo --noEmit clean in packages/leadtype; turbo run lint clean.
  • paths.lock.json regenerated for the two pages this PR edits (reference/cli, concepts/config-model) only. Three other entries (integrate-with-fumadocs, collections, use-the-source-primitive) are stale at the parent branch's HEAD — pre-existing on Resolve the project once instead of in every command #167, left untouched here to avoid a cross-PR conflict.
  • Workspace-level bun run check-types still trips the parallel-build race tracked in Fix three pre-existing breakages in the example apps and task graph #166; not chased here.

Deliberately out of scope


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

A docs audit found baseUrl was the one value every snippet still had to
repeat in both forms of the common path: generate --base-url for the CLI
and createDocsProject({ baseUrl }) for the runtime. It is now a
site-owned top-level config field next to product, so the scaffolded
path carries zero knobs: init writes it once into docs/docs.config.ts,
the docs:generate script drops --base-url, and lib/source.ts becomes
createDocsProject() with no arguments.

Precedence is explicit-wins with no behavior change for existing
setups: flag/argument > config field > the deployment-URL env fallbacks
that applied before. The value is validated at load (absolute http(s)
URL, no query or fragment) and normalized (trailing slashes stripped),
never inherited via inheritConfig — where a site publishes is the
consuming site's fact, so a source-owned docs.config.* read by several
sites leaves it unset. Provenance records explicit/default with the env
chain named, doctor reports the resolved URL and its origin, and
generate --explain reports the fallback when nothing was authored.
@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: 95249de6-d174-4b60-af9d-a4b9444c752f

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: c8cbc733bd

ℹ️ 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/cli/init.ts
Comment thread packages/leadtype/src/cli/init-templates.ts
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 — two loose ends worth a look before merge.

Reviewed changes — full read of the single commit c8cbc733 against dx/157-resolve-project, plus the surrounding config-load, inheritance, and base-URL-consumer code the diff doesn't touch.

  • baseUrl on DocsConfig — a top-level optional field (llm/llm.ts), threaded onto ResolvedDocsConfig and SerializableResolvedConfig.
  • Validation and normalization at loadnormalizeConfigBaseUrl (config/normalize.ts) trims, strips trailing slashes, and rejects non-absolute, non-http(s), and query/fragment-bearing values with the config path named; validateDocsConfig type-checks the authored shape.
  • One resolution point per halfargs.baseUrl ?? metadata.baseUrl in cli/generate.ts feeds every site-artifact generator; input.baseUrl ?? config.baseUrl in project/index.ts feeds the runtime.
  • Provenance and diagnostics — a baseUrl provenance entry is written on every normalize call, doctor reports value + origin in both output modes, and generate --explain reports the env fallback when nothing was authored.
  • Scaffold de-duplicationinit writes baseUrl only into docs/docs.config.ts; lib/source.ts, the docs:generate script, and the post-scaffold generate run all drop it, and buildPlan/nextPlan/astroPlan/nuxtPlan/sveltekitPlan lose the parameter.
  • Docsreference/cli.mdx flag rows and feeds prose, concepts/config-model.mdx ownership paragraph, paths.lock.json rehashed for exactly those two pages.

I verified the three claims most likely to be wrong and they hold: baseUrl is genuinely absent from SOURCE_CONFIG_INHERIT_FIELDS and DEFAULT_SOURCE_CONFIG_INHERIT, so "never inherited" is enforced rather than just documented; the only surviving args.baseUrl references are the arg parser and the single resolution point, so no artifact generator was missed; and every path that can produce a DocsConfig — both config filenames via loadDocsConfigFromDir, and an inline config object via resolveProject — runs normalizeDocsConfig, so an unvalidated value cannot reach URL joining.

ℹ️ The new doctor output isn't in the doctor reference

docs/reference/doctor.mdx presents the --json config object as a stable contract "for agents and automation", and its What it checks → Config paragraph enumerates what the section reports. This PR adds a config.baseUrl { value, origin } pair and a new human-report line, and neither appears on that page — so the one page an automation author reads to learn the shape now under-describes it.

Technical details
# Document the `config.baseUrl` field in the doctor reference

## Affected sites
- `docs/reference/doctor.mdx:54` — the "Config" bullet lists path/mode, schema errors, deprecations, and provenance; the resolved base URL and its origin are not mentioned.
- `docs/reference/doctor.mdx:90-102` — the `--json` sample `config` object shows `path`, `mode`, `deprecations`, `provenance` but not `baseUrl`.
- `packages/leadtype/src/cli/doctor.ts:78` — the field being added.
- `packages/leadtype/src/cli/doctor.ts:710-714` — the human line `baseUrl: <url>  (<origin>)`, which the page's human-output sample also predates.

## Required outcome
- A reader of `docs/reference/doctor.mdx` can learn that `doctor` reports the resolved base URL, what `explicit` vs `default` mean for it, and the exact `--json` key that carries it.

ℹ️ Only the config spelling of baseUrl is validated

normalizeConfigBaseUrl rejects relative URLs, non-http(s) schemes, and query/fragment values for the config field, but --base-url and createDocsProject({ baseUrl }) still accept any string and flow straight into normalizeBaseUrl, which only strips trailing slashes. Since the PR positions the config field as the preferred home and the other two as overrides, the strict rule now applies to the path least likely to hold a bad value, and a user who moves a working-but-sloppy value from a flag into the config gets a new hard failure.

Technical details
# Decide whether `--base-url` and `createDocsProject({ baseUrl })` should share the config field's validation

## Affected sites
- `packages/leadtype/src/config/normalize.ts:94-121``normalizeConfigBaseUrl`, applied only to `config.baseUrl`.
- `packages/leadtype/src/cli/generate.ts:1796``args.baseUrl` wins over the validated value and is itself unchecked.
- `packages/leadtype/src/project/index.ts:293` — same for `input.baseUrl`.

## Open questions for the human
- Is the asymmetry deliberate (validating the flag would be a breaking change for existing callers passing e.g. a path-only value), or should all three spellings share one validator?
- If deliberate, the `DocsConfig.baseUrl` JSDoc and the `reference/cli.mdx` `--base-url` row are the right places to say the flag is not validated, so the difference is discoverable rather than surprising.

ℹ️ Nitpicks

  • packages/leadtype/src/config/normalize.ts:442-445 pairs origin: "default" with inferredFrom, but FieldProvenance (packages/leadtype/src/config/types.ts:54) documents inferredFrom as "For inferred values". Either the origin or the field doc is slightly off — the env chain is closer to derived-at-runtime than to a static default.
  • packages/leadtype/src/cli.test.ts:3243expect(sitemap).not.toContain("acme.dev//") passes even without the trailing-slash handling, since normalizeBaseUrl strips trailing slashes at every consumer. The preceding toContain assertion is what actually pins the behavior; the negative one is decoration.

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

Comment thread packages/leadtype/src/cli/doctor.ts Outdated
Review follow-ups on the baseUrl config field:

- init validates and normalizes --base-url at parse time with the same
  validator the config loader applies (normalizeAuthoredBaseUrl, now
  exported), so an invalid value fails as a usage error before any file
  is written instead of scaffolding a project every later command rejects.
- init refuses --base-url when docs/docs.config.ts already exists and
  --force is absent: the config is the flag's only destination, so
  skipping the write would silently drop an explicit flag. The error
  names both fixes (edit the config, or rerun with --force).
- The baseUrl validator also checks the authored string for ? and #
  delimiters — a bare trailing delimiter parses with empty search/hash,
  so the parsed components alone let it through into every joined URL.
- Doctor's report comments no longer claim the resolved baseUrl is what
  artifacts will actually use: it is what the config resolves in
  doctor's own environment, and generate --base-url or the build's env
  can override it at generation time.

@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: ff63487f47

ℹ️ 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/cli/init.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 init guard changes what the documented --dry-run and --json preview modes do, and the extracted validator didn't reach the two spellings that actually feed URL joins.

Reviewed changes — the delta since the prior pullfrog review at c8cbc733: one new commit, ff63487f, plus a re-read of the validator, init, and doctor code it touches.

  • Extracted the base-URL validatornormalizeConfigBaseUrl became an exported normalizeAuthoredBaseUrl(baseUrl, subject) in config/normalize.ts, with the config path now supplied as the error subject rather than baked in.
  • Closed the bare-delimiter hole — a /[?#]/ test over the authored string rejects https://acme.dev? and https://acme.dev#, which parsed.search / parsed.hash both report as empty. Covered by a dedicated test that fails without the check.
  • Validated init --base-url at parse timeparseInitArgs runs the flag through the shared validator, so a bad value is a usage error before any file is written, and a trailing slash is normalized on the way into the scaffolded config.
  • Added a guard for the skipped-config caserunInitCommand exits 2 when --base-url is passed, docs/docs.config.ts already exists, and --force is absent, instead of silently dropping the flag.
  • Narrowed the doctor baseUrl wording — the DoctorReport.config.baseUrl JSDoc and the report comment now describe what the config resolves in doctor's own environment and note that a generate --base-url flag or the build's env can override it. This addresses the prior review's inline note; output and test expectations are unchanged.

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

Comment thread packages/leadtype/src/cli/init.ts Outdated
Comment thread packages/leadtype/src/config/normalize.ts Outdated
Comment thread packages/leadtype/src/config/normalize.ts Outdated
Review follow-up on the baseUrl config field: when docs/docs.config.ts
already exists without a baseUrl field and init reruns without
--base-url, writeFiles skips the config — so the documented framework
default (:4321 Astro, :5173 SvelteKit) lands nowhere and every absolute
URL silently resolves against http://localhost:3000.

- init now inspects the existing config with the same loader every
  other command uses (a regex over the authored source would miss a
  spread or imported value) and refuses with exit 2 — the same shape as
  the --base-url refusal, naming the exact line to add or --force —
  when the config loads without baseUrl and the framework default is
  not the generic dev URL.
- Reruns stay quiet no-ops everywhere nothing is lost: the config sets
  baseUrl, the framework default is http://localhost:3000 anyway
  (next, nuxt), or the config cannot be loaded at all — an unloadable
  config already fails loudly in the post-scaffold generate, so init
  must not refuse on what it cannot inspect.
- The generic dev URL becomes a named constant (GENERIC_DEV_BASE_URL)
  shared by defaultBaseUrl and the refusal.

@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: bf3f3ac3ac

ℹ️ 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/cli/init.ts Outdated
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.

Important

The new rerun guard refuses the bare leadtype init --framework astro|sveltekit that two app READMEs tell users to run, and the remedy it prints hardcodes a localhost URL that overrides the deployment env chain.

Reviewed changes — the delta since the prior pullfrog review at ff63487f: one new commit, bf3f3ac3, plus a re-read of the init command path, writeFiles, and the normalizeBaseUrl fallback chain the guard now reasons about.

  • Named the generic dev URLhttp://localhost:3000 became an exported GENERIC_DEV_BASE_URL in cli/init-templates.ts, with a JSDoc tying it to normalizeBaseUrl's last-resort fallback; defaultBaseUrl's default branch returns it.
  • Added a rerun refusal for the stranded-default caserunInitCommand exits 2 when no --base-url and no --force are passed, the framework default is not the generic dev URL, docs/docs.config.ts exists, and that config declares no baseUrl.
  • Probed the existing config with the real loaderexistingConfigDeclaresBaseUrl runs loadDocsConfigFromDir rather than a regex over the source, returning null (permissive) when the config is absent or fails to load.
  • Added four init rerun tests — refusal on an Astro config without baseUrl, a quiet rerun when it has one, a permitted rerun on Next, and a no-op re-scaffold when the config cannot be loaded, plus a writeExistingConfig helper and a not.toContain("--base-url") assertion on the patched docs:generate script.

⚠️ Two new init refusals that the CLI reference doesn't admit exist

docs/reference/cli.mdx:34 still reads "Exit codes: 0 success, 2 CLI usage error or no framework detected", and the --force row still describes the flag as nothing more than "Overwrite files that already exist". Between ff63487 and this commit, init gained two conditions under which a well-formed invocation exits 2, and both messages point at --force as the escape — which writeFiles applies to every file in the plan, not just the config. Beyond the doc gap there is a shape question only you can settle: a warning would leave the documented preview and rerun paths intact while still surfacing the stranded default.

Technical details
# Document the new `init` refusals, or reconsider refusing at all

## Affected sites
- `docs/reference/cli.mdx:34` — the exit-code sentence covers only usage errors and undetected frameworks; neither new refusal is either.
- `docs/reference/cli.mdx:27` — the `--force` row reads as a per-file overwrite toggle, but both new messages present it as the way to resolve a `baseUrl` conflict.
- `packages/leadtype/src/cli/init.ts:220``writeFiles` applies `force` uniformly to every entry in `allFiles`, so following either message's `--force` advice also rewrites `astro.config.mjs`, `src/lib/source.ts`, route handlers, and any other scaffolded file the user has since edited.
- `packages/leadtype/src/cli/init.ts:449-458`, `:466-478` — the two guards.

## Required outcome
- A reader of `docs/reference/cli.mdx` can learn that `init` exits 2 when `--base-url` has nowhere to land or the existing config would strand a non-generic framework default, and what each remedy actually does.
- The `--force` remedy's blast radius (every file in the plan) is stated wherever it is offered.

## Open questions for the human
- Is a hard exit-2 the right shape, or should the stranded-default case be a warning on stderr with exit 0? A warning preserves the documented `--dry-run` / `--json` contracts and the README-documented rerun, and the failure it prevents (localhost URLs in generated artifacts) is already visible in `doctor` and `generate --explain`.
- If refusal stays, is there a narrower escape than `--force` — e.g. a flag that only rewrites the config?

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

Comment thread packages/leadtype/src/cli/init.ts Outdated
Comment thread packages/leadtype/src/cli/init.ts Outdated
Comment thread packages/leadtype/src/cli/init.test.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: 00be748ba6

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

@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 in the new commits — two documentation loose ends inline.

Reviewed changes — the delta since the prior pullfrog review at bf3f3ac3: two new commits, 479e1c17 and 00be748b, plus a re-read of init's conflict path, the shared validator, and the two call sites the validator newly covers.

  • Carried the two init baseUrl conflicts into the preview modes instead of refusing — the flag conflict and the stranded-default conflict now compute a shared baseUrlConflict message; the exit-2 refusal fires only when neither --json nor --dry-run is set. --json keeps its plan object and gains an additive warnings array; --dry-run keeps its plan and writes the conflict to stderr with a note that a real run exits 2.
  • Routed the remaining two authored spellings through the shared validatorgenerate --base-url validates at parse time (usage error, exit 2) and createDocsProject({ baseUrl }) validates an explicit argument, each with its own error subject. The normalizeBaseUrl env chain is untouched.
  • Stored the parser's serialization rather than the authored textnormalizeAuthoredBaseUrl returns stripTrailingSlashes(parsed.href), so https://acme.dev\api is stored as https://acme.dev/api and a raw space is stored encoded, closing the sibling of the bare-delimiter hole.

I checked two falsifiable questions against the code and both came back clean, so neither is a finding:

  • No realistic input regresses from the new validation. readValue (cli/generate.ts:383-389) already rejected an empty or --prefixed flag value before this PR, and runGenerateCommand (:1512-1522) already caught parse throws as exit 2. Every base URL passed anywhere in this repo, its apps, and its docs is a clean literal absolute URL, so nothing that used to work now fails.
  • The init conflict refactor's control flow holds. Enumerating the argv combinations, no path drops the message, emits it twice, or routes it to the wrong channel, and the "only reachable under --dry-run" reasoning behind the late stderr warning at cli/init.ts:544 is accurate — the --json branch returns above it.

Two threads from the prior review stay open: the localhost remedy text at cli/init.ts:475 is unchanged by these commits, and the init.test.ts "cannot be loaded" test is byte-identical, so its premise still rests on module resolution failing rather than on anything the test asserts.

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

Comment thread .changeset/base-url-config.md Outdated
Comment thread packages/leadtype/src/cli/init.ts Outdated
…ly reads

An absent baseUrl is how a site picks its production URL up from the
deployment env fallbacks at generate time — which init cannot observe —
so the no-flag rerun guard was refusing exactly the config the changeset
promises keeps working, and its remedy would pin production URLs to
localhost. The rerun now proceeds in every mode and carries an
informational note naming both resolutions (fixed baseUrl in the config,
or the env fallback) through the warnings channel; only the explicit
--base-url conflict still refuses, since that value would be dropped.

Both guards now judge the config generate would actually read: a root
leadtype.config.* outranks the docs/docs.config.ts init writes, so
--base-url refuses naming that file (with or without --force), and the
note inspects it instead of the shadowed docs config.

The null-tolerance test now hand-writes an unloadable config that also
omits baseUrl, so the loader returning null is the only way its empty
stderr can hold.

@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 in the new commit — one over-stated message inline, and one capability narrowing that nothing documents.

Reviewed changes — the delta since the prior pullfrog review at 00be748b: one new commit, a9aa7908, plus a re-read of init's conflict and note paths, loadDocsConfig's discovery order, and the seven init.test.ts cases the commit adds or rewrites.

  • Made config discovery, not init's own scaffold target, the thing init judges — a new findRootConfigFilename probes the project root for leadtype.config.{ts,js,mjs,cjs}, and both the --base-url refusal and the missing-baseUrl note now point at whichever config actually wins. existingConfigDeclaresBaseUrl was generalized to take a directory and a filename list so it can load either one. I verified the precedence claim against the source rather than the new JSDoc: config/load.ts:1096-1116 tries cwd + LEADTYPE_CONFIG_FILENAMES first and returns before it ever reaches the docs dirs, and generate.ts:1686 calls loadLeadtypeConfig(srcDir) ahead of loadDocsConfig.
  • Replaced the exit-2 rerun guard with an informational note — a kept config that omits baseUrl is no longer treated as a mistake, which is right: an unset field is exactly how a site picks its production URL up from the deployment env at generate time, and init cannot observe that env. The note is gated on baseUrl !== GENERIC_DEV_BASE_URL, so it fires only where a framework dev default (:4321, :5173) would otherwise be silently discarded.
  • Rewrote the "cannot be loaded" test so its premise is load-bearing — this properly retires the finding from the prior review. The config is now hand-written with import "@leadtype-test/definitely-not-installed"; and no baseUrl, and the case runs under --framework astro, so a successful load would emit the note and break expect(capture.stderr).toBe(""). null is now the only reading under which the assertion holds.

I traced two things that looked like bugs and neither is one, so neither is a finding: the .ts-only hardcode in docsConfigExists is correct through every extension scenario, because a freshly written docs.config.ts is first in DOCS_CONFIG_FILENAMES and so outranks a pre-existing .js sibling; and baseUrlConflict and baseUrlNote are mutually exclusive by construction (args.baseUrl !== undefined versus === undefined), so the baseUrlWarnings array is always length ≤ 1 — harmless, and the array shape is what the --json contract wants anyway.

ℹ️ init --base-url became unusable in root-config projects, and nothing says so

The refusal at cli/init.ts:478-479 fires whenever a root leadtype.config.* exists, and unlike the sibling branch it ignores --force — the test at init.test.ts:640 pins both spellings. On a project that has a root config but no docs scaffold yet, leadtype init --base-url https://acme.dev now exits 2 and writes nothing at all, where before this PR it scaffolded the whole tree.

The behaviour is defensible — the value really would land somewhere generate doesn't read — but the narrowing is larger than the reasoning suggests, because before this PR --base-url did not go only into a config. It was also baked into the docs:generate script and lib/source.ts, both of which are read regardless of config discovery. So this is a capability that used to work in root-config projects and now cannot be expressed through init at all.

Nothing tells a user that. docs/reference/cli.mdx:34 still lists init's exit 2 as CLI usage error or no framework detected, and the changeset describes the config field without mentioning that a previously-working init invocation now refuses. Worth a line in each.

Two smaller edges in the same area, both fine to leave but worth a decision:

  • When the root config wins and the run proceeds without --base-url, init still writes docs/docs.config.ts carrying a baseUrl and the comment Where this site publishes … Update it when you deploy. — an inert file that describes itself as authoritative.
  • The note is gated on baseUrl !== GENERIC_DEV_BASE_URL, so Next and Nuxt projects get no note at all in the same situation. That is deliberate (there is no dev default to strand), but it does mean the "which config wins" information only ever surfaces for two of the four frameworks.

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

Comment thread packages/leadtype/src/cli/init.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: 32486a9142

ℹ️ 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 in the new commit — one check-ordering suggestion inline that undercuts the commit's own no-echo intent.

Reviewed changes — the delta since the prior pullfrog review at a9aa7908: one new commit, 32486a91, plus a re-read of normalizeAuthoredBaseUrl's rejection ladder, init's --json/--dry-run plan shape, and the four cli.mdx rows the commit rewrites.

  • Added a credentials rejection to the shared validatorparsed.username/parsed.password are checked at normalize.ts:125-134 and, unlike every sibling rejection, the message deliberately does not echo the authored value. The reasoning in the comment is sound and the functional coverage is complete: https://u:p@h, https://u@h, and https://:p@h all trip it, and https://@h leaves no userinfo in href at all, so there is no shape that slips through into a joined URL.
  • Made the --json plan's warnings field unconditional — this properly retires the absence-signal critique from the prior review. The plan now always carries warnings (empty when clean) instead of the conditional spread, and the clean-plan test at init.test.ts:125 asserts toEqual([]) rather than not.toHaveProperty. Still purely additive over the released plan shape, since the field is new in this release either way.
  • Caught the docs up to the behaviour — both --base-url rows in cli.mdx now say a malformed value is a usage error (exit 2), the --dry-run row notes the post-plan warning, the --json row describes the warnings array, and the changeset drops "nothing changes for existing setups" in favour of naming the rejected shapes and stating plainly that the flag and the argument are stricter than before.

I checked three falsifiable questions against the code and all came back clean, so none is a finding: init has no path that can carry a credentialed value anywhere, so the new rejection cannot fire from a scaffold; baseUrlConflict and baseUrlNote are still mutually exclusive by construction after the warnings change, so an unconditional array is still length ≤ 1; and the new --dry-run/--json prose matches the code in all three modes — write mode refuses with exit 2 at init.ts:531-534, --json returns above the stderr line, and --dry-run prints the plan then the warning.

ℹ️ Nitpicks

  • packages/leadtype/src/llm/llm.ts:500-502 — the DocsConfig.baseUrl JSDoc still enumerates the rejection set as "not an absolute http(s) URL — or carries a query or fragment". Credentials are now a fourth rejected shape and this is the doc an agent reads when writing a config. One clause.
  • docs/reference/cli.mdx — the --dry-run and --json rows now describe the conflict path, but nothing documents the note path at cli/init.ts:608-610: a kept config without baseUrl prints an informational line on stderr in a normal write-mode run, and that line also lands in the warnings array. A user reading the reference has no way to know that a successful init can write to stderr.

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

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

ℹ️ 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 in the new commit — both prior findings are properly closed, and one claim in the new comment inherits the same over-statement it replaces.

Reviewed changes — the delta since the prior pullfrog review at 32486a91: one new commit, a7b87879, plus a re-read of normalizeAuthoredBaseUrl's rejection ladder and init's conflict/note paths.

  • Reordered the credentials rejection above the protocol rejectionconfig/normalize.ts:120-138. A credentialed value now hits the non-echoing message for every scheme, and a credential-free non-http(s) value keeps the echoing diagnostic that makes it addressable. This is exactly the fix the prior review asked for, and no existing test regressed: the protocol case at normalize.test.ts:695-698 uses a credential-free ftp://acme.dev.
  • Extended the credentials test with the ftp:// casenormalize.test.ts:729-741 asserts /must not embed credentials/ and not.toContain on both userinfo parts. I checked that it is genuinely falsifiable: under the old order the value reaches the protocol message, which fails both assertions.
  • Softened the three absolute "never read" claims in cli/init.ts — the findRootConfigFilename JSDoc (:167), the block comment above the guard (:471-473, which now carries the generate --src docs --docs-dir . counterexample), and the user-facing baseUrlConflict message (:481). I grepped the package: no never read phrasing survives, and baseUrlNote (:522) never made the claim. The stable prefix init.test.ts:673-674 pins is unchanged, so the reword orphaned no test.

Behaviour is unchanged by both edits, which is the right call in each case.

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

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

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

// port (`https://user:pass@host:99999`) throws before the credentials
// check below ever runs — so redact anything userinfo-shaped before
// echoing; the secret must not land in stderr/CI logs.
const redacted = normalized.replace(/\/\/[^/@\s]*@/, "//<redacted>@");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Redact through the final userinfo delimiter

When a malformed URL contains @ inside its password, such as https://user:pa@ss@host:bad, parsing fails but this non-greedy authority pattern redacts only through the first @; the resulting diagnostic still prints ss@host:bad, leaking part of the credential into CLI or CI logs. Fresh evidence after the earlier malformed-URL finding is this multi-@ userinfo path, which the new regex does not cover; redact through the authority's final @ or avoid echoing parser-rejected values.

Useful? React with 👍 / 👎.

@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 found.

Reviewed changes — the delta since the prior pullfrog review at a7b87879: one new commit, 31fa7314, plus a re-read of normalizeAuthoredBaseUrl's full rejection ladder and the credentials test it extends.

  • Redacted userinfo before echoing a value the parser rejectsconfig/normalize.ts:120 runs the authored string through .replace(/\/\/[^/@\s]*@/, "//<redacted>@") before interpolating it into the "is not an absolute URL" message, so a credentialed value that throws in new URL for an unrelated reason no longer prints the secret. This goes further than the prior review asked — the ask was only to soften the comment's safety claim — and going further is the better call: the comment at :116-119 now states the actual reason rather than describing a residual.
  • Extended the credentials test with the parse-failure caseconfig/normalize.test.ts:743-760 asserts the bad-port value yields <redacted>@acme.dev and contains neither userinfo part, and pins that a credential-free malformed value still echoes verbatim. Genuinely falsifiable: without the replace, not.toContain("s3cret") fails.

I re-ran the counterexamples the prior thread enumerated, and the redaction covers every one of them: https://buildbot:s3cret@acme.dev:99999, …:abc, a space in the host, an empty host, and a bare //buildbot:s3cret@acme.dev all throw in new URL and all come back redacted, while not a url and acme.dev pass through unchanged. Two things I checked that are not findings: the inline regex literal is fine here because packages/leadtype/biome.jsonc:9-12 turns performance/useTopLevelRegex off, and the regex can over-redact a path segment on an already-broken value (https://acme.dev:99999/docs//a@b), which costs nothing on a string that is being rejected anyway.

One thing worth recording rather than acting on: the buildbot:s3cret@acme.dev spelling — no //, so WHATWG reads buildbot: as the scheme and the rest as an opaque path — still parses, with empty username/password, so it skips the credentials check and lands on the echoing protocol message at :139-142. That case was called out in the prior thread as independent of the ordering fix, and this commit deliberately scoped to the parse-failure branch, so I'm not re-raising it; the only stale artifact is the "Ordered above every rejection that echoes the authored value" phrasing at :126, which is now very slightly broader than the code guarantees. Not worth a commit on its own.

Pullfrog  | 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