fix: give Doctor the host configuration the runtime receives
CI / verify (pull_request) Successful in 42s
CI / infrastructure-smoke (pull_request) Successful in 14s
CI / migration-smoke (pull_request) Successful in 1m24s

`pnpm doctor` and `startApi` each read their own subset of the same
environment variables, and the subsets had drifted. Doctor never read
`S3_*` or module secrets at all, so a correctly configured installation
was told its object storage was "not configured" and every module
requiring a bucket or a secret was reported blocked -- while the runtime,
from the identical environment, bound those buckets and ran the modules.

Both executables now map the environment through one
`loadHostConfiguration(env)` in Core, which takes the environment as an
argument and never reads `process.env` itself. Validation stays with the
existing per-domain validators, so each caller keeps its own failure
behaviour: startup refuses a rejected value, Doctor reports it as a
sanitized code.

Doctor's storage option becomes raw input like its mail option, so a
half-configured provider is reported as a blocking configuration failure
instead of throwing before a report exists. The outbound-email check --
computed, counted toward the result, but absent from every rendered
section -- is now printed, and the rendered set is asserted against a
real report so a future check cannot go missing the same way.

Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
ekklesia
2026-08-24 19:15:49 -05:00
co-authored by Claude Opus 5
parent a835215eb8
commit 2b4f0afb72
23 changed files with 1411 additions and 209 deletions
@@ -0,0 +1,153 @@
import { readdir, readFile } from "node:fs/promises";
import path from "node:path";
import { fileURLToPath } from "node:url";
import { describe, expect, it } from "vitest";
/**
* Structural guarantees about where deployment configuration is read, which no runtime assertion can
* make.
*
* The defect this pass fixed was structural: two executables each read their own subset of the same
* environment variables, and the subsets drifted until `pnpm doctor` reported a correctly configured
* installation as having no object storage. Restoring parity once is not enough — the arrangement
* that allowed the drift has to stay gone.
*/
const repositoryRoot = path.join(path.dirname(fileURLToPath(import.meta.url)), "..", "..");
async function readTypeScriptFiles(
directory: string,
): Promise<ReadonlyArray<{ readonly path: string; readonly text: string }>> {
const results: Array<{ path: string; text: string }> = [];
const walk = async (current: string): Promise<void> => {
for (const entry of await readdir(current, { withFileTypes: true })) {
const full = path.join(current, entry.name);
if (entry.isDirectory()) {
if (entry.name === "node_modules" || entry.name === "dist") {
continue;
}
await walk(full);
continue;
}
if (entry.name.endsWith(".ts") || entry.name.endsWith(".tsx")) {
results.push({ path: full, text: await readFile(full, "utf8") });
}
}
};
await walk(path.join(repositoryRoot, directory));
return results;
}
/**
* Strips block comments and whole-line `//` comments.
*
* These assertions are about what the *code* does, and prose that explains a rule inevitably quotes
* the very token the rule forbids. Trailing comments are deliberately left alone so a URL inside a
* string literal is never truncated.
*/
function withoutComments(text: string): string {
return text
.replaceAll(/\/\*[\s\S]*?\*\//gu, "")
.split("\n")
.filter((line) => !line.trimStart().startsWith("//"))
.join("\n");
}
/**
* Every variable the shared loader *interprets* — parses, defaults, splits, or classifies.
*
* `DATABASE_URL` is deliberately absent: it is passed through verbatim, and the small migration and
* owner-bootstrap commands legitimately read it directly. Interpretation is what drifts.
*/
const INTERPRETED_VARIABLES = Object.freeze([
"EKKLESIA_CLIENT_ADDRESS_HEADER",
"EKKLESIA_CLIENT_ADDRESS_MODE",
"EKKLESIA_CLIENT_ADDRESS_TRUSTED_PROXIES",
"EKKLESIA_DATABASE_CA",
"EKKLESIA_DATABASE_TLS_MODE",
"EKKLESIA_IDENTITY_BASE_URL",
"EKKLESIA_IDENTITY_SECRET",
"EKKLESIA_IDENTITY_TRUSTED_ORIGINS",
"EKKLESIA_PUBLIC_SIGNUP_ENABLED",
"EKKLESIA_SMTP_FROM_ADDRESS",
"EKKLESIA_SMTP_FROM_NAME",
"EKKLESIA_SMTP_HOST",
"EKKLESIA_SMTP_PASSWORD",
"EKKLESIA_SMTP_PORT",
"EKKLESIA_SMTP_SECURITY",
"EKKLESIA_SMTP_USERNAME",
"NODE_ENV",
"S3_ACCESS_KEY",
"S3_BUCKET_PREFIX",
"S3_DELIVERY_ENDPOINT",
"S3_ENDPOINT",
"S3_FORCE_PATH_STYLE",
"S3_REGION",
"S3_SECRET_KEY",
]);
describe("host configuration is read in exactly one place", () => {
it("never reads the process environment from the shared loader itself", async () => {
for (const file of await readTypeScriptFiles(path.join("core", "src", "host"))) {
// The loader is reusable Core code: it takes the environment as an argument, exactly as
// `createEnvironmentModuleSecretSource` does, so both executables can hand it theirs.
expect(withoutComments(file.text), file.path).not.toContain("process.env");
}
});
it("interprets each host infrastructure variable in exactly one file", async () => {
const sources = [
...(await readTypeScriptFiles(path.join("core", "src"))),
...(await readTypeScriptFiles(path.join("apps", "api", "src"))),
];
for (const file of sources) {
const code = withoutComments(file.text);
for (const variable of INTERPRETED_VARIABLES) {
// Reading one of these anywhere else means a second, independently drifting interpretation
// of the same deployment — which is exactly the defect this pass fixed. Operator-facing
// message text that *names* a variable is untouched: only an actual read is forbidden.
expect(code, `${file.path} reads ${variable}`).not.toContain(`process.env.${variable}`);
expect(code, `${file.path} reads ${variable}`).not.toContain(`process.env["${variable}"]`);
}
}
});
it("leaves the two executables reading nothing but their own listener address", async () => {
const doctorCli = withoutComments(
await readFile(path.join(repositoryRoot, "core", "src", "cli", "doctor.ts"), "utf8"),
);
const server = withoutComments(
await readFile(path.join(repositoryRoot, "apps", "api", "src", "server.ts"), "utf8"),
);
for (const source of [doctorCli, server]) {
expect(source).toContain("loadHostConfiguration");
expect(source).not.toContain("DATABASE_URL");
}
// `EKKLESIA_API_HOST`/`EKKLESIA_API_PORT` describe this listener rather than the host's
// infrastructure, so Doctor has no use for them and they stay with the process that binds.
expect(server).toContain("EKKLESIA_API_HOST");
expect(doctorCli).not.toContain("EKKLESIA_API_");
});
it("reads the environment only from executables, never from reusable Core", async () => {
for (const file of await readTypeScriptFiles(path.join("core", "src"))) {
if (file.path.includes(`${path.sep}cli${path.sep}`)) {
continue;
}
expect(withoutComments(file.text), file.path).not.toContain("process.env");
}
});
it("gives Doctor the same object-storage and secret inputs the runtime receives", async () => {
const doctorCli = await readFile(
path.join(repositoryRoot, "core", "src", "cli", "doctor.ts"),
"utf8",
);
// The two options whose absence was the bug. A future edit that drops either fails here rather
// than in a support conversation about storage that Doctor says is missing.
expect(doctorCli).toContain("storageConfiguration: host.storage");
expect(doctorCli).toContain("moduleSecrets: host.moduleSecrets");
});
});