fix(config): scope.options sees runtime env config before the file flush (#1618)#1726
fix(config): scope.options sees runtime env config before the file flush (#1618)#1726heskew wants to merge 6 commits into
Conversation
…ush (#1618) OptionsWatcher re-parsed the on-disk root config, while componentLoader decides from the resolved in-memory config — which deterministically includes runtime env config (HARPER_SET_CONFIG et al.). At boot 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) could observe pre-env values the loader never saw. Proven via instrumented CI runs on #1616: componentLoader logged enabled:true on every thread of a failing run while the routes 404'd. OptionsWatcher now composes the env layers (composeConfigFromEnv, side-effect free, same precedence as the runtime pipeline) over every root-config read, including the install window where the file does not exist yet. Application config.yaml scopes are untouched, as is behavior when no config env vars are set. flattenObject additionally warns once per path when an env-var config value is an empty object — it flattens to nothing by design (load-bearing for restore-on-removal semantics) but the silent drop has confused users. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
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.
|
Reviewed; no blockers found. |
- ENOENT branch no longer clobbers #rootConfig before the name check — a first read whose scope name is absent from env config falls through to the original handling and emits 'ready' (was: 'remove', which nothing consumes at boot, hanging Scope.ready). - Root-config discrimination is now an exact match against the resolved root-config path (lazy configUtils require), covering the legacy harperdb-config.yaml name and excluding components that ship a root-named file; filename fallback (both names) when configUtils isn't initialized. - composeConfigFromEnv failures in the ENOENT branch route to 'error' instead of rejecting unhandled. - Tests: name-absent ENOENT emits ready (regression guard); legacy filename gets the overlay. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The exact-path check against getConfigFilePath() misclassified every watcher in environments where configUtils resolves a real instance (the unit harness boots one), sending the overlay tests into a ready-hang and timing out unit CI. The heuristic is now filename-only (current + legacy root names) with an explicit isRootConfig constructor argument for callers that know root-ness authoritatively. Known limitation documented: a component shipping its own harper-config.yaml is misclassified as root — harmless unless config env vars are set and keys collide. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…OptionsWatcher OptionsWatcher had absorbed config-domain knowledge — the three env-var names, the root config filenames, and the compose semantics. Move all of it behind two config-layer functions: overlayRootEnvConfig(base) (no-op when no config env vars; composes otherwise; handles the empty/absent base) and isRootConfigFilename(). OptionsWatcher now only expresses 'this is the root config, overlay env onto it', which is the minimal knowledge a watcher needs. Also moots the gemini review note about the inline object guard — that guard now lives inside overlayRootEnvConfig. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Good fix for the #1618 race. Two things worth a look before merge:
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 |
|
Cross-PR dependency note. cc @kriszyp @kylebernhardy. This PR composes the root One seam: 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). |
…(gemini review)
Env config is file-independent, but the ENOENT read path discarded it: when the
scope's name was in the composed env config and #scopedConfig was already set, the
branch fell through to #resetConfig()+emit('remove'), wiping active config on a
transient/unreadable-file read. Now, when the scope IS in the composed env config,
an ENOENT preserves it — first read emits ready, a later read merges — and only
falls through to reset when the scope is absent from env config. Real deletion is
unaffected (chokidar 'unlink' -> #handleUnlink).
Addresses the gemini [high] review note on #1726.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ng vars The warning branch fell through to the process.env assignment, so a component .env still landed the HARPER_*CONFIG trio in the process environment — harmless while config is composed-then-memoized, but a consumer that re-composes from process.env (#1618/#1726) would then honor it, silently re-inverting the top-down relationship. Warn AND skip; test asserts the trio never reaches process.env even with loadEnv override enabled (heskew review). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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:
One open design decision for you (raised on #1580, cross-linked): this PR honors Merging this makes #1616 (#631 |
…ng vars The warning branch fell through to the process.env assignment, so a component .env still landed the HARPER_*CONFIG trio in the process environment — harmless while config is composed-then-memoized, but a consumer that re-composes from process.env (#1618/#1726) would then honor it, silently re-inverting the top-down relationship. Warn AND skip; test asserts the trio never reaches process.env even with loadEnv override enabled (heskew review). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ng vars The warning branch fell through to the process.env assignment, so a component .env still landed the HARPER_*CONFIG trio in the process environment — harmless while config is composed-then-memoized, but a consumer that re-composes from process.env (#1618/#1726) would then honor it, silently re-inverting the top-down relationship. Warn AND skip; test asserts the trio never reaches process.env even with loadEnv override enabled (heskew review). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cher (kris review) The isRootConfig constructor escape hatch had no caller: componentLoader had the authoritative isRoot two frames up and never threaded it. Now the Scope construction passes isRoot through to OptionsWatcher, so real component loads use the loader's signal — an app component that ships a root-named config file no longer gets the env overlay. The filename heuristic remains as fallback for direct constructions only (tests, ad-hoc callers); doc updated accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Both addressed, @kriszyp:
Unit suites green on the wiring (34 passing incl. the env-overlay cases); CI running on |
Summary
scope.options(theOptionsWatcherevery 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-timescope.optionsreads (e.g. anenabledgate inhandleApplication) can observe pre-env values the loader itself never saw.Fix: for the root config file only,
OptionsWatchercomposes the env layers over every file read via the existing side-effect-freecomposeConfigFromEnv()(same documented precedence as the runtime pipeline), including the file-not-yet-written install window. Application scopes watch their ownconfig.yamland are untouched. Additionally, an empty-object env value (componentName: {}) — whichflattenObjectmust 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
/v1gateway branch pinned the divergence precisely: on failing runs, componentLoader sawmodelsGateway config={"enabled":true}on both main/0 and http/1 (the env-merged in-memory view), yet all routes 404'd — becausehandleApplication'sscope.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-onlycomponentsRootregistering cleanly) is consistent: that key is read by the loader from memory, never through anOptionsWatcher.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. NotecomposeConfigFromEnvruns 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;flattenObjectsemantics unchanged (a first attempt that preserved{}as a leaf broke the restore-on-removal contract —http: {}must not wipehttp.port— caught by the existing suite).harperConfigEnvVarsis leaf-safe (fs/path/crypto/lodash/type-only logger); the module's existing lazygetLogger()pattern is reused for the warning.Tests
unitTests/components/OptionsWatcher-envOverlay.test.js(new): env key missing from file → delivered; env overrides file value; applicationconfig.yamlNOT 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).OptionsWatcher,harperConfigEnvVars-*, andconfigUtils*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'sloadEnv-delivered case (component setsprocess.envduring 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:#rootConfigbefore the name check, so a scope whose name wasn't in env config gotremoveinstead ofreadyon first read →Scope.readyhang. Now the composed object becomes state only when the name is present; regression-guard test added.harperdb-config.yaml) missed the overlay, and a component shipping a root-named file got it wrongly. Root-ness is now determined by an explicitisRootConfigconstructor 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 againstgetConfigFilePath()was reverted: in any environment whereconfigUtilsresolves a real instance (including the unit-test harness) it misclassified the overlay tests' temp watchers, hangingreadyand timing out unit CI. Documented residual limitation: a component that ships its ownharper-config.yamlis 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.composeConfigFromEnvthrows in the ENOENT branch now route toerrorinstead 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.