fix(config): scope.options sees runtime env config before the file flush (#1618)

[heskew]

Summary

scope.options (the OptionsWatcher every component scope boots with) reads its state by re-parsing the config file from disk — while the componentLoader decides what to load from the resolved in-memory config, which deterministically includes runtime env config (HARPER_SET_CONFIG / HARPER_CONFIG / HARPER_DEFAULT_CONFIG). At boot those two views race: the env-applied values reach the file only after the first thread's runtime re-apply flushes them, so a component's boot-time scope.options reads (e.g. an enabled gate in handleApplication) can observe pre-env values the loader itself never saw.

Fix: for the root config file only, OptionsWatcher composes the env layers over every file read via the existing side-effect-free composeConfigFromEnv() (same documented precedence as the runtime pipeline), including the file-not-yet-written install window. Application scopes watch their own config.yaml and are untouched. Additionally, an empty-object env value (componentName: {}) — which flattenObject must drop (empty objects are load-bearing "no overrides here" markers for the restore-on-removal semantics) — now logs a once-per-path warning instead of vanishing silently.

Evidence (from #1616's CI-only 404s)

Instrumented CI runs on the /v1 gateway branch pinned the divergence precisely: on failing runs, componentLoader saw modelsGateway config={"enabled":true} on both main/0 and http/1 (the env-merged in-memory view), yet all routes 404'd — because handleApplication's scope.options.get(['enabled']) read the on-disk file, which only received the env values at [http/1] Config file updated with runtime env var values, after loading. Green/red flipped run-to-run on functionally identical code — a pure race. macOS boots consistently won the race; Linux CI lost it roughly half the time. The QA characterization on the issue (env-only componentsRoot registering cleanly) is consistent: that key is read by the loader from memory, never through an OptionsWatcher.

Where to look

• components/OptionsWatcher.ts — the overlay is gated on #isRootConfig (an explicit constructor arg when the caller knows root-ness, else a filename heuristic matching the current + legacy root config names) plus env vars actually being present, so the no-env and application-scope paths are byte-identical to before. The ENOENT branch now serves env-composed config during the install window rather than resetting to defaults. Note composeConfigFromEnv runs on every root-file read (not just the first): a file-change event during the racy window would otherwise clobber the scoped config via the watcher's merge/remove path.
• config/harperConfigEnvVars.ts — warning only; flattenObject semantics unchanged (a first attempt that preserved {} as a leaf broke the restore-on-removal contract — http: {} must not wipe http.port — caught by the existing suite).
• Import-cycle safety: harperConfigEnvVars is leaf-safe (fs/path/crypto/lodash/type-only logger); the module's existing lazy getLogger() pattern is reused for the warning.

Tests

• unitTests/components/OptionsWatcher-envOverlay.test.js (new): env key missing from file → delivered; env overrides file value; application config.yaml NOT overlaid; install window (file absent) → env config delivered; no env vars → file verbatim.
• unitTests/config/harperConfigEnvVars-compose.test.js: empty-object env value drops without clobbering the base (pins both halves of the documented semantics).
• Existing OptionsWatcher, harperConfigEnvVars-*, and configUtils* suites pass unchanged.

Relationship to #1513

Same family (config composed/memoized before components run). This PR fixes the scope.options-vs-loader divergence for process-env-delivered config; #1513's loadEnv-delivered case (component sets process.env during loading) is upstream of both views and remains open.

Closes #1618

---

Generated by an LLM (Claude Fable 5, with earlier investigation by Claude Sonnet 5 workers). Review focus: the overlay-on-every-read decision and the ENOENT branch in OptionsWatcher.

🤖 Generated with Claude Code

Cross-model review (step 10) outcome

Both outside-model legs failed environmentally (agy timeouts; Codex sandbox-blocked) — Harper-domain adjudication pass only, caveat noted. It confirmed the core approach and surfaced three findings, fixed in c4963f607:

1. ENOENT branch regression (would-be blocker): the composed config clobbered #rootConfig before the name check, so a scope whose name wasn't in env config got remove instead of ready on first read → Scope.ready hang. Now the composed object becomes state only when the name is present; regression-guard test added.
2. Legacy root config (harperdb-config.yaml) missed the overlay, and a component shipping a root-named file got it wrongly. Root-ness is now determined by an explicit isRootConfig constructor arg for callers that know it, falling back to a filename heuristic that matches both the current and legacy root names (de32bb61d). An earlier attempt to discriminate by exact-match against getConfigFilePath() was reverted: in any environment where configUtils resolves a real instance (including the unit-test harness) it misclassified the overlay tests' temp watchers, hanging ready and timing out unit CI. Documented residual limitation: a component that ships its own harper-config.yaml is still classified as root by the heuristic — harmless unless config env vars are set and its keys collide with them; the explicit arg is the escape hatch.
3. Error symmetry: composeConfigFromEnv throws in the ENOENT branch now route to error instead of an unhandled rejection.

Verified clean by the review: no-env and application-scope paths byte-identical; the merge/remove path can't corrupt scoped config on subsequent reads (composition is a superset of file keys); no import cycles; watcher reads are cold-path.

[gemini-code-assist]

Code Review

This pull request ensures that runtime environment configurations are overlaid on top of the root configuration (harper-config.yaml) during file reads and missing file errors, preventing race conditions during boot. It also adds a warning when empty objects are dropped during configuration flattening. The review feedback highlights two key improvements: merging the composed configuration with this.#scopedConfig during ENOENT errors to prevent resetting active configurations, and using the safer #isConfig helper instead of typeof parsed === 'object' to avoid incorrectly identifying arrays as objects.

[claude]

Reviewed; no blockers found.

[kriszyp]

Good fix for the #1618 race. Two things worth a look before merge:

1. The new isRootConfig constructor escape hatch (OptionsWatcher.ts:23) has no caller — Scope.ts:124 is the only place OptionsWatcher is constructed and it doesn't pass it, even though componentLoader.ts already has the authoritative isRoot boolean in scope two frames up and just doesn't thread it through. Since componentLoader.ts:334 checks for harper-config.yaml first for every component (root or not), any app component using that filename gets the env overlay by default today — the escape hatch this PR adds for exactly that case is unreachable.
2. The ENOENT-with-env-config-present branch only fires on a scope's first read (this.#name in composed && !this.#scopedConfig) — if the scope already booted and later hits a runtime ENOENT (file deleted mid-run), it falls through to reset-to-defaults even with env vars still valid. Not a regression (pre-PR behavior was the same), but it does contradict the success-path comment's stated intent of composing env over every read.

Neither is a blocker, but the first one in particular seems like it should either be wired up or the constructor arg dropped until it has a caller.

— KrAIs

[heskew]

Cross-PR dependency note. cc @kriszyp @kylebernhardy.

This PR composes the root scope.options from process.env config-shaping vars (HARPER_DEFAULT_CONFIG/HARPER_CONFIG/HARPER_SET_CONFIG), which is correct given those vars only reach process.env via the sanctioned channels (the real process environment / harper-config.yaml).

One seam: resources/loadEnv.ts currently injects a component .env's config-shaping vars into process.env (it warns but doesn't continue past the assignment — see #1580). Because this PR re-composes from process.env on every root-config read, that injection could get a component-delivered trio var honored here, re-inverting the top-down config relationship #1580 establishes. Today that's masked (config is memoized before loadEnv runs); this PR unmasks it.

Closing that belongs at the injection point in #1580, not here — noted on that PR. So this PR's correctness formally depends on #1580's loadEnv skip, and the two should land with #1580 first (or together), not in an order that opens the window. No logic change needed here otherwise.

Comment generated by an LLM (Claude Opus 4.8, 1M context).

[heskew]

Ready for review — feedback complete, CI green (unit v22/v24/v26 + integration; the one flaky red seen was the blob-suite #1557 family, cleared on rerun — config-independent).

Since the bot pass, two things changed worth a look, @kriszyp:

1. Config-boundary refactor. The env-var names, root-config filenames, and compose semantics were bleeding into components/OptionsWatcher.ts; they now live in the config layer behind overlayRootEnvConfig() / isRootConfigFilename(). OptionsWatcher only expresses "this is the root config → overlay env onto it."
2. ENOENT-preserve fix (gemini [high]). Env config is file-independent, so a transient/install-window read (real deletion is handled separately by #handleUnlink) no longer discards an env-derived scope's config — it preserves/merges when the scope is in the composed env config, and only resets when it isn't.

One open design decision for you (raised on #1580, cross-linked): this PR honors process.env config-shaping vars on root-config reads, which is correct given those vars reach process.env only via the sanctioned channels. #1580 should enforce that at the injection point (loadEnv skips the trio) rather than each consumer defending itself — your call on enforce-at-injection vs defense-in-depth. #1726 lands with-or-after #1580.

Merging this makes #1616 (#631 /v1 gateway) integration test deterministic.

[heskew]

Both addressed, @kriszyp:

1. Wired up in 24f3adc2f. componentLoader now passes its authoritative isRoot through the Scope construction into OptionsWatcher(…, isRootConfig), so real component loads never rely on the filename heuristic — an app component shipping a root-named config file no longer gets the env overlay. The heuristic is demoted to fallback for direct constructions only (tests, ad-hoc callers), with the doc updated to say exactly that.

2. Already fixed in cbc93d4ac — our changes crossed (your comment 15:21Z, that push 18:58Z). The ENOENT branch no longer gates on first-read: the condition is this.#name in composed, and when the scope has already booted it #merges the composed env config instead of falling through to reset-to-defaults. A runtime ENOENT with env vars still valid now preserves the env-derived config, matching the success-path comment's every-read intent. (Real deletion still routes through #handleUnlink.)

Unit suites green on the wiring (34 passing incl. the env-overlay cases); CI running on 24f3adc2f.

[kriszyp]

Re-reviewed at 24f3adc — thanks for threading the loader's authoritative isRoot through Scope → OptionsWatcher. That cleanly resolves my prior note that the isRootConfig param was heuristic-only/effectively dead: real component loads no longer depend on the filename heuristic, so a non-root component shipping a root-named config file can't accidentally take the env-overlay path. Nice, correctly-scoped change. The core env-overlay fix still looks right and I re-verified it holds at head.

A few things I'd still like to see addressed (none lose committed data or block merge, but this is a high-risk config-race path so I'd rather pin them down):

Significant — empty-object scope in the file is stripped once any config env var is set (harperConfigEnvVars.ts:330). overlayRootEnvConfig(parsed) routes the base file through composeConfigFromEnv → flattenObject, which intentionally drops empty objects. That's correct for an override layer (http: {} = "no overrides"), but the base file isn't an override layer — a literal modelsGateway: {} there is user content. With any config env var set + a bare componentName: {} scope in the file, that scope vanishes from the composed #rootConfig, #name in #rootConfig goes false, and the else-branch fires remove (unloading a scope that was present pre-PR). This is exactly the env-layer-semantics-leaking-onto-the-base-layer shape worth avoiding. Could we preserve base empty-objects (or overlay without routing the base through the env-drop path)?

Significant — #handleUnlink bypasses the env overlay (OptionsWatcher.ts:246). A runtime delete of the root config unconditionally #resetConfig()s to DEFAULT_CONFIG and emits remove, discarding any HARPER_SET_CONFIG-defined scope — the exact env-independence the ENOENT branch was just taught to preserve, left open on the sibling path. It's runtime-deletion-only (not the boot race, and not a regression), so lower real-world likelihood — but giving it the same overlayRootEnvConfig(undefined) fallback would close the asymmetry.

Tests — both branches touched to fix a Scope.ready hang are still unpinned: the ENOENT merge branch (OptionsWatcher.ts:203) and an empty-object-base-survives case. Both sit on code changed to fix a hang, so a regression guard each would be cheap insurance. The composeError → emit('error') path is realistically unreachable at boot (env JSON is validated upfront), so that one's just defense-in-depth.

Follow-up — config/RootConfigWatcher.ts reads the root config with no env overlay and is consumed by harper_logger.ts updateLogSettings() on the same lifecycle hooks that race in #1618. Lower impact (a wrong initial log level self-corrects on the next change), but it's the same race this PR is titled to fix, on a second consumer — worth an explicit follow-up ticket rather than leaving it silent.

Minor perf note (not blocking): every root component gets its own Scope/OptionsWatcher pointed at the same physical root config file, so one write now re-runs composeConfigFromEnv (3× JSON.parse + flattenObject + cloneDeep) once per root component. Cold config-edit path, so fine — just flagging in case someone later wants to memoize on file-content-hash.

Overall: solid root-cause fix and the new commit is a real improvement. Happy to approve once the empty-object-base semantics are resolved and the regression tests land (RootConfigWatcher can be a follow-up).

—
🤖 Reviewed by KrAIs (Claude Opus 4.8) on Kris's behalf. (Gemini/agy was unavailable this pass — findings are from an earlier cross-model run + this Claude pass.)

[heskew]

Both significants + the tests addressed in 3aea585, @kriszyp:

Empty-object base scopes — fixed at the compose layer. The base file no longer loses content to the env-layer flatten: composeConfigFromEnv restores base empty-object paths that composition didn't otherwise populate (restoreBaseEmptyObjects). The layering semantics you called out are preserved exactly — an env layer that sets or replaces the path wins (no resurrection as {}), and env-layer {} keeps its load-bearing "no overrides here" drop semantics (pinned by a test). The once-per-path drop warning no longer fires for base-file empties, since they're no longer dropped.

#handleUnlink — same overlay, same error routing. The ENOENT read path and unlink now share one #applyEnvOnlyConfig fallback: env-defined scope present → first application emits ready, already-configured merges (never resets); malformed env JSON routes to error; no env scope → the original reset+remove semantics, with a log line saying the env-defined config remains in effect when it does.

Tests — five compose-level cases (bare {} survives, nested {}, env-replacement wins, env-population wins, env-layer {} drop pinned) and two watcher-level regression guards: a bare componentName: {} root-config scope reaching ready with env vars set (previously hung/removed), and a root-config deletion keeping an env-defined scope alive — the latter exercises the shared merge branch you asked to pin (config already set → merge, not reset). The compose tests were negatively verified (2 fail with the restore disabled).

RootConfigWatcher follow-up — agreed it shouldn't stay silent; I have the ticket drafted and will link it here once filed.

The perf note is noted — memoizing composeConfigFromEnv on file-content-hash is easy to bolt on if the cold path ever matters.

[kriszyp]

Approving. This removes the boot race rather than reordering it: OptionsWatcher recomputes the resolved config from env via the canonical composeConfigFromEnv() on every root-config read, so scope.options matches the loader's view regardless of file-flush timing — enforces the invariant instead of papering over the symptom. No hot-path cost (reads fire on chokidar events + boot; hasConfigEnvVars() short-circuits to a no-op when no config env vars are set). Head commit closed two env-independence leaks since the prior review. CI green across Node 22/24/26, Windows, Bun, uWS. The two-sources-of-truth note is architectural (#1513 boundary), not blocking.

— reviewed with KrAIs (Claude Opus 4.8)