Make baseUrl a config field instead of a repeated knob - #182
Make baseUrl a config field instead of a repeated knob#182KayleeWilliams wants to merge 9 commits into
Conversation
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.
|
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: 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".
There was a problem hiding this comment.
ℹ️ 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.
baseUrlonDocsConfig— a top-level optional field (llm/llm.ts), threaded ontoResolvedDocsConfigandSerializableResolvedConfig.- Validation and normalization at load —
normalizeConfigBaseUrl(config/normalize.ts) trims, strips trailing slashes, and rejects non-absolute, non-http(s), and query/fragment-bearing values with the config path named;validateDocsConfigtype-checks the authored shape. - One resolution point per half —
args.baseUrl ?? metadata.baseUrlincli/generate.tsfeeds every site-artifact generator;input.baseUrl ?? config.baseUrlinproject/index.tsfeeds the runtime. - Provenance and diagnostics — a
baseUrlprovenance entry is written on every normalize call,doctorreports value + origin in both output modes, andgenerate --explainreports the env fallback when nothing was authored. - Scaffold de-duplication —
initwritesbaseUrlonly intodocs/docs.config.ts;lib/source.ts, thedocs:generatescript, and the post-scaffold generate run all drop it, andbuildPlan/nextPlan/astroPlan/nuxtPlan/sveltekitPlanlose the parameter. - Docs —
reference/cli.mdxflag rows and feeds prose,concepts/config-model.mdxownership paragraph,paths.lock.jsonrehashed 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-445pairsorigin: "default"withinferredFrom, butFieldProvenance(packages/leadtype/src/config/types.ts:54) documentsinferredFromas "Forinferredvalues". 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:3243—expect(sitemap).not.toContain("acme.dev//")passes even without the trailing-slash handling, sincenormalizeBaseUrlstrips trailing slashes at every consumer. The precedingtoContainassertion is what actually pins the behavior; the negative one is decoration.
Claude Opus | 𝕏
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.
There was a problem hiding this comment.
💡 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".
There was a problem hiding this comment.
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 validator —
normalizeConfigBaseUrlbecame an exportednormalizeAuthoredBaseUrl(baseUrl, subject)inconfig/normalize.ts, with the config path now supplied as the errorsubjectrather than baked in. - Closed the bare-delimiter hole — a
/[?#]/test over the authored string rejectshttps://acme.dev?andhttps://acme.dev#, whichparsed.search/parsed.hashboth report as empty. Covered by a dedicated test that fails without the check. - Validated
init --base-urlat parse time —parseInitArgsruns 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 case —
runInitCommandexits 2 when--base-urlis passed,docs/docs.config.tsalready exists, and--forceis absent, instead of silently dropping the flag. - Narrowed the doctor
baseUrlwording — theDoctorReport.config.baseUrlJSDoc and the report comment now describe what the config resolves in doctor's own environment and note that agenerate --base-urlflag or the build's env can override it. This addresses the prior review's inline note; output and test expectations are unchanged.
Claude Opus | 𝕏
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.
There was a problem hiding this comment.
💡 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".
There was a problem hiding this comment.
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 URL —
http://localhost:3000became an exportedGENERIC_DEV_BASE_URLincli/init-templates.ts, with a JSDoc tying it tonormalizeBaseUrl's last-resort fallback;defaultBaseUrl'sdefaultbranch returns it. - Added a rerun refusal for the stranded-default case —
runInitCommandexits 2 when no--base-urland no--forceare passed, the framework default is not the generic dev URL,docs/docs.config.tsexists, and that config declares nobaseUrl. - Probed the existing config with the real loader —
existingConfigDeclaresBaseUrlrunsloadDocsConfigFromDirrather than a regex over the source, returningnull(permissive) when the config is absent or fails to load. - Added four
initrerun tests — refusal on an Astro config withoutbaseUrl, 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 awriteExistingConfighelper and anot.toContain("--base-url")assertion on the patcheddocs:generatescript.
⚠️ 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?Claude Opus | 𝕏
There was a problem hiding this comment.
💡 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".
There was a problem hiding this comment.
ℹ️ 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
initbaseUrl conflicts into the preview modes instead of refusing — the flag conflict and the stranded-default conflict now compute a sharedbaseUrlConflictmessage; the exit-2 refusal fires only when neither--jsonnor--dry-runis set.--jsonkeeps its plan object and gains an additivewarningsarray;--dry-runkeeps 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 validator —
generate --base-urlvalidates at parse time (usage error, exit 2) andcreateDocsProject({ baseUrl })validates an explicit argument, each with its own errorsubject. ThenormalizeBaseUrlenv chain is untouched. - Stored the parser's serialization rather than the authored text —
normalizeAuthoredBaseUrlreturnsstripTrailingSlashes(parsed.href), sohttps://acme.dev\apiis stored ashttps://acme.dev/apiand 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, andrunGenerateCommand(: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
initconflict 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 atcli/init.ts:544is accurate — the--jsonbranch 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.
Claude Opus | 𝕏
…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.
There was a problem hiding this comment.
ℹ️ 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
initjudges — a newfindRootConfigFilenameprobes the project root forleadtype.config.{ts,js,mjs,cjs}, and both the--base-urlrefusal and the missing-baseUrlnote now point at whichever config actually wins.existingConfigDeclaresBaseUrlwas 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-1116triescwd+LEADTYPE_CONFIG_FILENAMESfirst and returns before it ever reaches the docs dirs, andgenerate.ts:1686callsloadLeadtypeConfig(srcDir)ahead ofloadDocsConfig. - Replaced the exit-2 rerun guard with an informational note — a kept config that omits
baseUrlis 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, andinitcannot observe that env. The note is gated onbaseUrl !== 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 nobaseUrl, and the case runs under--framework astro, so a successful load would emit the note and breakexpect(capture.stderr).toBe("").nullis 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,initstill writesdocs/docs.config.tscarrying abaseUrland the commentWhere 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.
Claude Opus | 𝕏
dbd2581 to
32486a9
Compare
There was a problem hiding this comment.
💡 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".
There was a problem hiding this comment.
ℹ️ 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 validator —
parsed.username/parsed.passwordare checked atnormalize.ts:125-134and, 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, andhttps://:p@hall trip it, andhttps://@hleaves no userinfo inhrefat all, so there is no shape that slips through into a joined URL. - Made the
--jsonplan'swarningsfield unconditional — this properly retires the absence-signal critique from the prior review. The plan now always carrieswarnings(empty when clean) instead of the conditional spread, and the clean-plan test atinit.test.ts:125assertstoEqual([])rather thannot.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-urlrows incli.mdxnow say a malformed value is a usage error (exit 2), the--dry-runrow notes the post-plan warning, the--jsonrow describes thewarningsarray, 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— theDocsConfig.baseUrlJSDoc 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-runand--jsonrows now describe the conflict path, but nothing documents the note path atcli/init.ts:608-610: a kept config withoutbaseUrlprints an informational line on stderr in a normal write-mode run, and that line also lands in thewarningsarray. A user reading the reference has no way to know that a successfulinitcan write to stderr.
Claude Opus | 𝕏
There was a problem hiding this comment.
💡 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".
There was a problem hiding this comment.
ℹ️ 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 rejection —
config/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 atnormalize.test.ts:695-698uses a credential-freeftp://acme.dev. - Extended the credentials test with the
ftp://case —normalize.test.ts:729-741asserts/must not embed credentials/andnot.toContainon 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— thefindRootConfigFilenameJSDoc (:167), the block comment above the guard (:471-473, which now carries thegenerate --src docs --docs-dir .counterexample), and the user-facingbaseUrlConflictmessage (:481). I grepped the package: nonever readphrasing survives, andbaseUrlNote(:522) never made the claim. The stable prefixinit.test.ts:673-674pins is unchanged, so the reword orphaned no test.
Behaviour is unchanged by both edits, which is the right call in each case.
Claude Opus | 𝕏
There was a problem hiding this comment.
💡 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>@"); |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
✅ 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 rejects —
config/normalize.ts:120runs the authored string through.replace(/\/\/[^/@\s]*@/, "//<redacted>@")before interpolating it into the "is not an absolute URL" message, so a credentialed value that throws innew URLfor 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-119now states the actual reason rather than describing a residual. - Extended the credentials test with the parse-failure case —
config/normalize.test.ts:743-760asserts the bad-port value yields<redacted>@acme.devand contains neither userinfo part, and pins that a credential-free malformed value still echoes verbatim. Genuinely falsifiable: without thereplace,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.
Claude Opus | 𝕏

Stacked on #167. Closes the last knob the docs audit found on the common path.
The evidence
baseUrlwas 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, thedocs:generatescript, and the post-scaffold generate run. Three copies of one fact is exactly the drift the config model exists to prevent.baseUrlin the configA site-owned, top-level field next to
product:With it set, the scaffolded path has zero knobs:
initwrites the value once intodocs/docs.config.ts, thedocs:generatescript isleadtype generate --src . --out public, and the runtime iscreateDocsProject()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-owneddocs.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 whyinitwrites 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
normalizeBaseUrlapplies 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
baseUrlprovenance entry —explicitwith the config path, ordefaultnaming the env chain.doctorreports the resolved URL and its origin (baseUrl: https://acme.dev (explicit)in human output, aconfig.baseUrlvalue/origin pair in--json), andgenerate --explainreports the localhost/env fallback when nothing was authored anywhere, with the field to set.Verification
--explainoutput (present on fallback, quiet when authored), doctor value/origin reporting, and the init scaffold carryingbaseUrlexactly once.tsgo --noEmitclean inpackages/leadtype;turbo run lintclean.paths.lock.jsonregenerated 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.bun run check-typesstill 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
doctorintegration is the minimal value/origin hook described above — no structural changes tocli/doctor.tsthat would collide with Share one navigation drift resolver between doctor and nav #179's drift-resolver refactor. If Share one navigation drift resolver between doctor and nav #179 reshapes the report, theconfig.baseUrlfield carries over as-is.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.