feat(create-malloy-package): scaffold from a warehouse, not only a local file

[mlennie]

What this does

npm create @malloy-publisher/malloy-package can now start from a warehouse, not only from a local file. --connection <type> scaffolds a package with a BigQuery, Snowflake or Postgres connection alongside the existing --data path for CSV, Parquet, JSON, NDJSON and XLSX. The two are mutually exclusive.

This is item 2a of the OSS & Local Development fortnight and closes the gap the open-sourcing plan already described: its setup section says local files need no connection config while "a warehouse adds one entry to publisher.json", and until now nothing wrote that entry. defaultConfig() emitted connections: [], always.

4 commits, 12 files, +1794/-60.

Four design decisions, stated rather than implied

1. Flags, not prompts. The tool is non-interactive today, agents run it, and the documented invocation is a one-liner people paste from a README. --connection <type>, --connection-name, --table, plus per-dialect identity flags.

2. No secret flag exists at all. There is no --password, no --service-account-key, no --private-key. A credential cannot reach the config file or the shell history through this tool. The scaffolder emits ${VAR} references, generates a .env.example with names and no values, adds .env to the generated .gitignore, and prints the export lines. Plaintext remains available by hand-editing and is documented as a thing you choose. That is stronger than making plaintext opt-in, because a credential typed on a command line is in shell history whatever the tool does with it afterwards. BigQuery omits the key entirely and falls through to Application Default Credentials.

3. Structural validation at scaffold time, live validation deferred. Under the recommended ${VAR} path the credentials usually are not in the environment when the scaffolder runs, so a live test would fail on the recommended path and teach users to ignore it. The scaffolder prints the exact post-boot test command instead. --test-connection is a clean follow-up.

4. Never invent a table name. With --table, it writes a starter model over that table. Without one, it writes the connection plus a commented stub and points AGENTS.md at malloy_searchDatabaseSchema (#960), which is the tool that can answer "what is in here".

The trap that changed the code

cred add connection validates names against ^[a-zA-Z_][a-zA-Z0-9_]+$ and skips what it rejects. Deriving a connection name from a user-supplied database or project name routinely produces something like my-warehouse, which works locally and then silently does not arrive after publishing. The scaffolder now emits only the portable intersection and refuses anything outside it rather than quietly rewriting it.

That came from the parallel Publisher-to-Credible chat, which also settled the artifact question: inline connections only, no standalone connections.json at scaffold time, because Publisher can only read the array inline and a second file would be two copies with no drift detection. The shapes are the same by construction, not coincidence: the control plane's Connection is allOf: [$ref dataplane Connection, + extras].

Verification

Six rounds to convergence, the last changing nothing: HEAD, git diff and git status --porcelain byte-identical before and after.

• Static gate: lint 0, typecheck 0, prettier clean but for the six standing git-ignored generated files.
• Tests: scaffolder 418 pass / 0 fail, server unit 2979 pass / 3 skip / 0 fail, integration 313 pass / 0 fail.
• Manual, warehouse path: real Postgres in Docker, five seeded rows in public.orders. Scaffold, export the variable, boot, query, record_count: 5. The no-table stub path booted and served with no load errors.
• Manual, CSV regression: connections: [], no .env.example, malloy-config.json present, data/sales.csv present, ten sample rows returned. Unchanged.
• /api/v0/connections/test exercised both ways to check the generated documentation: {"status":"ok"} with the right password, password authentication failed for user "demo" with a wrong one.

One documented trap, no code change

With PGSSLMODE set against a non-SSL Postgres, a plain scaffolded connection fails with packages=0 load_errors=1. It only bites deployments that set the variable (the Credible worker ships allow), so this is recorded in the skill with the measurements rather than changed in what the scaffolder writes.

Not fixed here, deliberately

Two pre-existing defects were measured while building this and are out of scope for a scaffolder PR, filed for an owner:

• An unset ${VAR} does not fail the boot. The server starts, prints ready, and reports environments=0 packages=0 load_errors=0 with the variable named nowhere, so the one field the docs point users at reads clean in exactly the case it exists to describe.
• GET /api/v0/status returns the substituted warehouse password in plaintext on an unauthenticated endpoint, and logger.warn(..., { connection: conn }) spreads a whole connection config into a log.

The bun.lock is deliberately not committed: it is pre-existing drift on main (the committed lock lags create-malloy-package@0.0.7 and a @malloydata/malloy-filter devDependency), and no dependency was added on this branch.

Reviewer note

All six rounds were self-review; this session could not spawn independent review agents. It found real defects, including several cases of my own prose gone false against my own code, but "the author can no longer find anything" is weaker than "an independent reader found nothing".

[Sha-Bang]

Read the whole diff, then checked the branch out, ran it, and scaffolded a warehouse package end to end. This is the strongest of the six on tests and the clearest on its own design decisions — connection.spec.ts tests the things I would have asked for, including "no secret reaches the entry, for any dialect" and the flag-ownership exhaustiveness case. Three things I would fix before it lands, two of them about where the credential actually ends up.

What I verified rather than took on faith

• 418 pass / 0 fail, as claimed. (My first run showed 5 failures; that was me skipping pretest, which builds @malloy-publisher/skills. Mentioning it only because the failure mode is confusing.)
• Payload field names check out against api-doc.yaml for all three dialects: host/port/databaseName/userName/password, account/username/warehouse/database/schema/role, defaultProjectId/location.
• The BigQuery ADC claim is real — serviceAccountKeyJson is read under an if (service/connection.ts:848), so omitting it is a working config rather than an incomplete one.
• preview() really does escape newlines via printable(), so the .env.example line-injection defence is not decorative.
• Scaffolded --connection postgres --table public.orders for real: connections[] entry with "password": "${MALLOY_POSTGRES_PASSWORD}", no data/, no malloy-config.json, .env in .gitignore, .env.example with names and no values, and a starter model that reads postgres.table('public.orders'). Exactly what the description says.

Is it worth doing?

Yes, and it closes a gap the docs had already promised: defaultConfig() emitted connections: [] unconditionally while the setup docs said a warehouse "adds one entry to publisher.json". Nothing wrote that entry.

Is it the right way?

Yes, on the decisions that matter. No secret flag at all is the right call and the reasoning is correct — a credential on a command line is in shell history whatever the tool does next, so "plaintext by hand only" beats "plaintext opt-in". Flags over prompts is right for a tool agents run. And the portable-name intersection is a genuinely good catch: emitting only what cred add connection will accept, rather than sanitizing silently, is the difference between a name that works locally and one that survives publishing.

Two dents, both about the credential's actual resting place, inline below: the .env file nothing reads, and the unauthenticated endpoint that serves the substituted password.

Does it fix it?

For Postgres, demonstrably — you booted it against real Docker Postgres and I reproduced the generated artifacts. BigQuery and Snowflake are structurally validated only. The field names check out against the spec so the risk is low, but worth stating plainly in the description: nobody has booted either dialect.

Open-source quality?

Strong. Tests are the best of this batch, the dialect table makes adding a warehouse a data change, and the ExtraFlag type exists specifically because leaving a flag out of both lists made --pg-port vanish in silence — that is the kind of comment that earns its place. The "Reviewer note" owning that all six rounds were self-review is good practice and I would keep doing it.

---

Must-fix: the inert .env · say where the password is actually readable · the prose #1049 falsifies
Should-fix: merge after #1057 · the non-array connections overwrite · a dead coverage pragma

🤖 Reviewed with Claude Code

[Sha-Bang]

Second pass, this one on code quality only — nothing here is about correctness, and none of it blocks separately from the review above.

Two things I measured first, because they are the usual suspicions and in this case both come back clean:

• Comment density is not a problem. connection.ts is 159 comment lines in 545 (29%), against 32% in config.ts and 33% in names.ts. The new file is slightly leaner than the package it joins, so it reads as house style rather than padding. Worth stating so nobody "tidies" it later.
• Output growth is proportionate. I ran both paths off the same build: --data prints 53 lines, --connection postgres --table ... prints 68. The extra 15 are a connection block that genuinely has more to say. The one caveat is in the inline comment below.

What is left is two maintainability findings and two nits.

🤖 Reviewed with Claude Code

[mlennie]

Thanks for checking this out and running it rather than reading it. Everything below is in the branch; the tip is 22315115. Answering in the order you raised things, and flagging up front that two of your code-quality items are deferred rather than done, with the reasoning, plus three corrections to claims of mine that turned out to be wrong.

Must-fix 1: the .env file nothing reads

Fixed, and it was my defect rather than a wording slip. .env.example now lists the variable names with export lines and no copy step, which also makes the .gitignore entry honest belt-and-braces instead of cover for a file the tool invited.

I took your option (b) rather than (a), and the reason is concrete rather than preference. Wiring it up would put set -a; . ./.env; set +a && in front of the npx call in the generated start script. That is POSIX shell, and npm runs scripts through cmd on Windows, so it would break the generated workspace there to save a step everywhere else. Recording that so nobody re-proposes (a) later: it is not that sourcing is inelegant, it is that the generated script has to run on Windows.

Must-fix 2: the unauthenticated status leak

The disclosure is now in .env.example and in the generated AGENTS.md, which are the two places the person handling the credential actually looks.

One correction you should have, so you can judge it yourself rather than take my word: it was not stated nowhere. skills/malloy-connections/SKILL.md already carried it under "A note on what ${VAR} does and does not protect". I am not hiding behind that, because your substantive point stands: a caveat in a skill while the reassurance is printed three times to the terminal is a lopsided story, and the fix was to put it where the terminal reader meets it. But you may want to look at that existing paragraph and say whether the wording was strong enough, since I would rather you assessed it than accepted my summary.

Two corrections to my own findings here, both against me.

First, I reported that the credential comes back from four endpoints. That was a sample reported as a census. I probed four paths by hand and found it in four; I never enumerated the API. The number I can defend is the one taken from the route table rather than by probing: fifteen registrations can return a connection, seven under /environments, seven legacy /projects aliases, and /status. Probing understates it, and it understated it twice independently, because the legacy aliases do not occur to you when you are picking paths to curl.

Second, I described the mechanism as the status builder mutating the environment object it spreads. That is wrong. serialize() returned the live connections array rather than a redacted copy, so every route served the credential-bearing array directly. My conclusion held, and so did the prediction that a serialize()-level fix reaches further than the status response, but the explanation was wrong and I had written it into a code comment. Both are corrected in the branch.

The wording in the shipped text needed no correction through any of that, because it says "several unauthenticated REST endpoints" and commits to no count or route. That is deliberate for the reason it turned out to demonstrate: naming one path invites a reader to protect that path and stop, and naming a number invites hardening exactly that many. All of them build from one serializer, so the count is a consequence rather than a boundary, and it moves whenever a route is added. Authenticate the API, not the paths you happen to know about.

Must-fix 3: prose that #1049 falsifies

Fixed at all three sites, using your framing. What a user now sees:

Start the server without it and the environment does not load:
it comes up reporting ready and serving nothing. Older servers report that
as no load errors with nothing naming the variable; newer ones name it in
loadErrors. Either way, export it before you start.

That rewording is also the answer to your separate note on the CLI's success block, where four of the fifteen lines this PR adds were that hand-wrapped paragraph. It is no longer hand-wrapped and no longer describes one server's reading as definitive, so it collapses to roughly what you suggested while staying true whichever of the two PRs lands first. Same edit, two places you raised it.

I also took the suggestion you made on #1059, which was the most valuable thing in the review: a test that asserts the behaviour the prose depends on, so the prose has something to break it. env_var_prose.spec.ts covers the CLI output and the generated AGENTS.md.

Worth telling you how that went, because it makes your point better than the test does. My first version banned the phrase "no load errors" outright and failed against prose that was correct, since the right wording names both readings and one of them is that phrase. It later failed a second time, when it asserted the disclosure contained "status" and I removed the endpoint name for the reason above. Testing wording is genuinely harder than testing behaviour: the naive guard punishes the fix it exists to protect. It now asserts at the level of the claim rather than the sentence.

Should-fixes

Merge after #1057: accepted, then overtaken. Once #1065 landed, scaffold.ts conflicted for real, so holding traded a mechanical cost for a content one. Merged now; if #1057 lands I regenerate the bundle again, which is a command rather than a judgement.

Non-array connections: fixed. It refuses the way assertCanAddPackage does, with a test asserting the user's half-typed value survives untouched and that no package directory or .env.example is left behind. You were right that the two keys behaving differently on the same malformed shape was the wart.

istanbul ignore next: removed, reasoning kept as a comment. Correct that it did nothing; this package tests with bun and there is no istanbul in the repo.

Code quality

The hand mapping in index.ts: fixed, and this one was not cosmetic. You were right that it reintroduced exactly the drift ExtraFlag was invented to kill, one layer up, and the part that makes it dangerous is your observation that the ownership test reads DIALECTS and therefore stays green through the mistake. I had built a guard and then created a gap the guard could not see. Now buildConnection({ ...options, connection: options.connection }), with a test that an optional dialect field actually reaches the written entry.

The six sibling connection* fields: DEFERRED, not done. Filed as issue 18 in our follow-up list with the reasoning. You are right that one optional nested connection?: {...} makes the "there is no connection" invariant structural rather than conventional at four call sites. I am deferring because it is a type-surface refactor across several call sites that changes no behaviour, on a PR that had been open five days with changes requested, and I would rather land the correctness and honesty fixes than grow the diff. Say the word and I will do it here instead.

[payload: string]: unknown on ConnectionEntry: DEFERRED, not done. Filed as issue 19 with the reasoning. Your point is well made and slightly uncomfortable: a typo'd payload key typechecks and produces a config Publisher silently ignores, which is the exact failure class the rest of that file is careful about. Deferred for the same reason as 18. A discriminated union on type, or Partial<Record<PayloadKey, unknown>> with PayloadKey derived from DIALECTS, closes it without losing the per-dialect key.

Trailing blank line in .env.example: fixed.

Off the diff

The pretest failure mode: now a section in CONTRIBUTING.md. Running bun test src instead of bun run test skips the skills build and produces about five failures that look like defects in the scaffolder. You should not have hit that on a first run.

The PR description's reference to malloy-open-sourcing-plan.md: agreed, it should come out, and I have left it to Monty rather than editing the body under your review threads.

One more finding, since it is the pattern you were reviewing for

While the branch was behind, #1065 landed and made explicit that SERVER_VERSION in source is a dev default: the publish workflow substitutes npm's current latest into that line at release time. A comment of mine said "0.0.250, the version this tool pins", which is false for every published user. Corrected to cite only what was measured.

That is the same shape as the load_errors problem you found, and as my "four endpoints" above. Stating it plainly because it is the useful generalisation from your review: every one of these was a claim anchored to something I had genuinely verified, where the thing verified was not the thing the reader gets. The correct measurement is exactly what makes the claim feel safe to write, which is why it kept happening.

Verification

Full gate green: typecheck, lint, prettier (bar the six standing git-ignored generated files), license headers clean except five pre-existing gaps on main, scaffolder 424 pass, server unit 3174 pass / 3 skip / 0 fail, integration 313 pass / 0 fail. Live: scaffolded against a real Postgres, booted, queried the model. CSV path unchanged and writes no .env.example. Every new guard was mutation-tested individually, reverted and watched go red, with the file confirmed byte-identical afterwards.

Two operational notes for anyone running this locally. Integration needs PUBLISHER_PORT and MCP_PORT set from a worktree, because the suite imports src/server, which binds the real listeners at import time and takes 4000; two worktrees collide and the suite dies before a single test reports. And after merging a main that changes a test fixture, clear packages/server/publisher_data and publisher.db, or the server serves the pre-merge copy and the failure names the fixture while looking exactly like a broken merge.

Finally, the caveat from the PR body still stands and I would not want the amount of process above to blur it: every review round on this branch before yours was mine. This session cannot spawn sub-agents, so no independent lens read the diff until you did. Two of the defects I found were my own prose gone false against code I had cited but never read, which is what self-review is worst at, and three of the corrections above came from other people measuring things I had asserted. Your pass was the first real review, not a confirmation of mine.

[Sha-Bang]

re-reviewing at 22315115. Nine of my eleven findings are genuinely fixed, and I checked each one in the code rather than trusting the resolution, because none of the eleven threads carries a reply — two of them are resolved without being fixed, and from the outside that is indistinguishable from done. #1049 is the contrast: you answered each finding there, and it made that pass much faster to verify.

Closed, verified:

• the inert .env. You took the honest option rather than wiring it up, and the .env.example header now says "nothing reads a .env either" outright. The rejected alternative is written down with its reason (set -a; . ./.env is POSIX, npm shells through cmd on Windows), which is the right call.
• the load_errors=0 prose, in all three places — now version-agnostic. Exactly the fix I hoped for, and it is the model for the one new finding below.
• the non-array connections overwrite. assertCanAddConnection refuses it. Verified live against a config with "connections": {}: it errors, explains itself, and leaves the file byte-identical.
• the hand flag mapping — buildConnection({ ...options, connection: options.connection }), with a comment saying why.
• the istanbul ignore next pragma — gone, reasoning kept.
• the .env.example trailing blank — trimEnd() plus one newline; confirmed on a real scaffold, the file ends MALLOY_POSTGRES_PASSWORD=\n.
• the output-volume paragraph — collapsed.

Still open, and now marked resolved: the six sibling connection* fields on ScaffoldResult, and the [payload: string]: unknown index signature. Both were should-fix, both are unchanged. Re-flagged inline — declining either is a fine answer, I would just like it said out loud.

One new must-fix, and it is the fix you already made one clause earlier in the same sentence: the credential-exposure paragraph is falsified by #1071. Inline.

What I ran:

• bun run test → 424 pass / 0 fail. One caveat that is not yours: the script passes no --timeout, so names.spec.ts's two "MALLOY_RESERVED drift" cases blow the 5s default on a slower machine (7.8s and 5.6s here) and the suite goes red on a spec this PR does not touch. Pre-existing, but this PR grows the suite that hits it.
• bun run test:e2e → 14 pass / 0 fail, against a dist/server.mjs built from this branch. That is the acceptance test that matters and it is green.
• A real scaffold, --connection postgres --table public.orders: the config entry carries "password": "${MALLOY_POSTGRES_PASSWORD}" and nothing else, no data/, no malloy-config.json, .env in .gitignore, .env.example names with no values, starter model reading postgres.table('public.orders').
• Four edge cases, all of which behave and all of which explain themselves: a connection named duckdb, a table path containing a quote, --pg-port passed with --connection snowflake, and the non-array connections above.

Housekeeping: the title still carries two leading spaces, and the branch is two commits behind main.

[Sha-Bang]

must-fix, and it is the fix you already made one clause earlier.

This sentence hedges the load_errors claim correctly — "older servers report that with no load errors and nothing naming the variable, newer ones name it in loadErrors" — and then asserts the next one flat: "a running Publisher serves connection config, with these values already substituted, from several unauthenticated REST endpoints."

#1071 makes that false. It is in the same batch, and it redacts at the single serializer this text is about: environment.ts:2963 becomes connections: toPublicConnections(this.listApiConnections()), and its integration sweep asserts /api/v0/status specifically carries no secret. Your own skill is what makes that fix total rather than partial — "Every route that can return a connection builds from one serializer, so the set is a consequence of that serializer rather than a boundary". One serializer, one change, all fifteen registrations.

So whichever of the two lands second, this text tells a user their warehouse password is being served by an API that has stopped serving it. Three places carry it, and this is the worst of them, because it is written into somebody else's AGENTS.md where nobody will re-read it:

• here, the generated AGENTS.md
• connection.ts:577, the .env.example header
• skills/malloy-connections/SKILL.md:218, which ships in the skills package

Confirmed on a real scaffold: the generated file carries both clauses in one sentence, the first hedged and the second not.

Hedge it the same way. The advice survives either merge order, because it never really rested on the leak — keeping the whole API on localhost or behind a gateway is right whether or not connection payloads are redacted. Something like: the indirection keeps the value out of the file and out of shell history, and that is all it does; depending on the server version a running Publisher may serve connection config with these values substituted, so keep the whole API on localhost or behind a gateway rather than protecting particular paths. Then #1071 landing improves a user's situation instead of dating your text.

The comment in connection.ts already knows this, incidentally: it says the serializer "returned the live connections array rather than a redacted copy", past tense. The comment is version-aware; the string it guards is not.

[Sha-Bang]

re-flagging: still open, thread resolved.

[payload: string]: unknown is unchanged, so a typo'd payload key still typechecks and still produces a config Publisher silently ignores — the failure class this file is otherwise careful about. The closed form from last time:

{ name: string; type: WarehouseType } & Partial<
   Record<
      "postgresConnection" | "bigqueryConnection" | "snowflakeConnection",
      Record<string, unknown>
   >
>

Not blocking on its own, and declining is a perfectly good answer. The thing worth changing is the resolve-without-reply: from the outside a resolved thread means fixed, and two of the eleven here were not, so I had to read the code to work out which two.

[Sha-Bang]

re-flagging: still open, thread resolved.

Six sibling connection* fields, and result.connectionName === undefined is still the de facto "there is no connection" test, at :2157 and :2159 among others.

ScaffoldOptions did pick up a nested connection?: BuiltConnection at :130, so the input side is now the shape I was asking for while the result side is not. That makes the asymmetry more visible than it was, not less: one side of this function passes a single optional object around and the other spreads it into six fields and then infers absence from one of them.

Same standing as the other one — fine to decline, worth saying so.

[Sha-Bang]

merge-order, and cheaper than last time: #1057 is approved.

This is still a one-line edit to the single-line bundle — wc -l on it reports 0 newlines. Land #1057 first and regenerate here (cd packages/server && bun run src/mcp/skills/build_skills_bundle.ts ../../skills), and the new skill lands as a handful of readable lines.

The other order means resolving a conflict inside a 470 KB single line, which is precisely what #1057 exists to prevent, and this PR adding a skill is the case it was measured against.

[mlennie]

Thanks for re-reviewing at 22315115, and for reading the code rather than trusting the resolutions. That cost you work it should not have: the threads were resolved without per-thread replies, so from the outside a resolved-and-fixed thread and a resolved-and-not-fixed thread looked identical, and you had to diff to tell which two were which. Answering by name below so this pass does not repeat that.

Tip is now 1e4deb50.

The new must-fix: the leak clause was asserted flat

Fixed, and you are right that it is the fix I had already made one clause earlier in the same sentence. The load_errors half was hedged and the leak half was not, and #1071 makes the unhedged half false at the single serializer every connection-returning route builds from. Whichever of us lands second, that text would have told a user their warehouse password is served by an API that had stopped serving it.

Fixed in all three places: the .env.example header, the generated AGENTS.md, and skills/malloy-connections/SKILL.md. I took your framing, because the advice never really rested on the leak:

# What this buys you: the value is not in the config file and not in
# your shell history. That is the whole of it.
#
# Depending on the server version, a running Publisher may serve
# connection config with these values already substituted. Keep the whole
# API on localhost or behind a gateway that authenticates, rather than
# protecting particular paths.

Your observation that the comment in connection.ts already knew this, describing the serializer in the past tense while the string it guarded did not, is the part I would not have found on my own. A version-aware comment guarding a version-blind string is its own warning, and I have not seen that failure named before.

The guard should have caught this and did not, so it now asserts that leak claims are hedged. I also rewrote its older assertions: three successive versions asserted particular vocabulary, first status and later unauthenticated, and each went red against a correct rewrite. A guard that punishes the fix it exists to protect is worse than no guard, so the assertions are now about properties of the claim rather than its words.

The two re-flagged items: both done, not declined

You said declining either was fine if said out loud. I would rather do them, for the reason you gave: pointing out that ScaffoldOptions had picked up a nested connection?: BuiltConnection while ScaffoldResult still spread into six fields made the asymmetry harder to defend, not easier. One side of the function passed a single optional object and the other inferred absence from one of six.

Six sibling fields: now one optional nested connection?: {...} on ScaffoldResult, so result.connection === undefined is the whole test and "there is no connection" is a property rather than a convention. The compiler enumerated all 28 call sites, which is what made it safe to do at this stage. Generated output is byte-identical, confirmed on a real scaffold.

The index signature: closed, in the form you wrote out:

export type ConnectionEntry = {
   name: string;
   type: WarehouseType;
} & Partial<Record<PayloadKey, Record<string, unknown>>>;

I also typed the dialect table's payloadKey as PayloadKey rather than string, since that is where the key actually comes from and leaving it loose there would have left the closed type unable to catch the typo it exists to catch. Verified by introducing postgressConnection and watching tsc reject it with a did-you-mean.

The threads you closed

Nothing owed on these, listed so the count is checkable: the inert .env and the recorded Windows reason, the load_errors prose in all three places, the non-array connections refusal, the hand flag mapping, the istanbul pragma, the .env.example trailing blank, and the output-volume paragraph. Seven.

Merge order

Done, in the order you asked for, and it was the right call. #1057 merged as 1580c4f, I merged main and regenerated with cd packages/server && bun run src/mcp/skills/build_skills_bundle.ts ../../skills, and this skill lands as 289 readable lines rather than a diff inside a single 470 KB line. That is the whole benefit of the ordering and it would not have existed the other way round.

I verified the resolution in both directions rather than assuming the regeneration was correct, since a regeneration is exactly where a silent revert of someone else's work would hide. Two distinct phrases of my new text checked in the bundle and in the skill source, plus positive probes that other people's edits survived: #1035's data-app wording, #1020's dashboards skill, and two unrelated skills. 57 entries, no conflict markers, and the bundle spec is 12 pass, up from 10 because #1057 added the indent checks.

The test timeout

Filed as a follow-up rather than fixed here, since it touches a shared script and this PR touches none. Worth recording both measurements: you saw 7.8s and 5.6s on the two MALLOY_RESERVED drift cases, and the whole of names.spec.ts runs in 1.51s on mine. It not reproducing everywhere is the problem rather than a reason to dismiss it, because it fails for the reviewer and not the author. The asymmetry is the tell: test:e2e passes --timeout 180000 and test passes nothing, so one script was given a budget and the other was not. You are also right that this PR makes it worse by growing the suite, 418 to 426.

Housekeeping

The two leading spaces in the PR title and the malloy-open-sourcing-plan.md reference in the description are both with Monty; I have left the body alone rather than editing it under your threads.

One thing I got wrong that you have not seen yet

I reported that the credential came back from four endpoints. That was a sample reported as a census: I probed four paths and found it in four, and never enumerated the API. The defensible number comes from the route table rather than from probing, and it is fifteen registrations, including the legacy /projects aliases that probing does not suggest. I also described the mechanism as the status builder mutating the environment object it spreads; that was wrong, and serialize() returning the live connections array is what actually made every route serve it.

Both are corrected in the branch. The user-facing wording needed no change through either correction, because it committed to no count and no route, which is the same property the hedging above gives it. Raising it because it is the pattern you were reviewing for, and it is easier to see in someone else's text than in your own.

Verification

All measured at adcf4b8b, with origin/main at 1580c4f and the branch 0 behind it. Stating the tip because several numbers in this batch, mine included, have turned out to be correct about a different tree.

• Scaffolder bun run test: 426 pass / 0 fail, 426 across 12 files.
• Scaffolder bun run test:e2e: 14 pass / 0 fail, against a dist/server.mjs built from this branch.
• Server unit: 3171 pass / 3 skip / 0 fail, 3174 across 150 files.
• Server integration: 315 pass / 0 fail, 315 across 37 files.
• Bundle spec: 12 pass / 0 fail.
• Live: scaffolded against a real Postgres, booted to environments=1 packages=1 load_errors=0, queried the model. CSV path unchanged and writes no .env.example.
• Every new guard mutation-tested, including restoring the exact flat clause you flagged and watching it go red.

A note on that integration number, because it moved twice and the second move was me getting it wrong.

test:integration runs bun test --timeout 200000 tests, and tests there is a substring path filter, not a directory. So when packages/server/publisher_data is populated, the run also collects publisher_data/examples/storefront/tests/, which is the storefront example's own test suite: exactly 72 tests across 3 files, gitignored, and nothing to do with the server. That is the entire difference between the two figures you may see quoted, 315 across 37 files against 387 across 40.

I reported 313 first, which was right for the tree it was measured on. I was then challenged because 313 sat well below the 387 other people were reporting, re-ran with a populated publisher_data, got 387, and "corrected" myself to the wrong number. The larger figure agreed with what several others were seeing, and I treated that agreement as evidence. It was the same mistake in a new coat: a number that is real, reproducible, and not measuring what the claim says it measures.

The real suite is 315 here (313 before this branch merged current main, which added two). 387 is that plus a bundled example's tests. It is filed with a one-character fix.

I am spelling this out rather than quietly printing 315, because I put 313 in front of you, then told you it was wrong, and it was not.

[Sha-Bang]

Closing this one on scope rather than quality.

To be clear about what's being closed: this was the strongest of the batch on tests, 9 of the 11 review findings were fixed and verified, and the security posture — no --password, no --service-account-key, ${VAR} references only, .env in the generated .gitignore — was the right call and argued for properly rather than assumed. The "a credential typed on a command line is in shell history whatever the tool does with it afterwards" reasoning is worth keeping wherever this comes back.

What it doesn't have any more is an owner or a live roadmap slot. It's a 2,183-line feature justified by a fortnight plan, 7 of its 14 files have moved on main since, and carrying a feature of this size to merge on someone else's behalf isn't a good use of the next block of work when the question "do we still want warehouse scaffolding in the scaffolder?" hasn't been re-asked.

If that answer comes back yes, this branch is the place to start — the design decisions are documented in the description and most of the review is already discharged.