Skip to content

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

Open
heskew wants to merge 6 commits into
mainfrom
fix/1618-env-config-before-components
Open

fix(config): scope.options sees runtime env config before the file flush (#1618)#1726
heskew wants to merge 6 commits into
mainfrom
fix/1618-env-config-before-components

Conversation

@heskew

@heskew heskew commented Jul 8, 2026

Copy link
Copy Markdown
Member

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.

…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>
@heskew heskew requested a review from kriszyp July 8, 2026 23:56

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread components/OptionsWatcher.ts Outdated
Comment thread components/OptionsWatcher.ts Outdated
Comment thread components/OptionsWatcher.ts Outdated
@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

heskew and others added 2 commits July 8, 2026 17:51
- 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>
@heskew heskew marked this pull request as ready for review July 9, 2026 04:44
@heskew heskew marked this pull request as draft July 9, 2026 14:19
…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>
@kriszyp

kriszyp commented Jul 9, 2026

Copy link
Copy Markdown
Member

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

heskew commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

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).

…(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>
kylebernhardy pushed a commit that referenced this pull request Jul 9, 2026
…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>
@heskew heskew marked this pull request as ready for review July 10, 2026 01:17
@heskew

heskew commented Jul 10, 2026

Copy link
Copy Markdown
Member Author

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.

github-actions Bot pushed a commit that referenced this pull request Jul 10, 2026
…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>
github-actions Bot pushed a commit that referenced this pull request Jul 10, 2026
…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>
@heskew

heskew commented Jul 11, 2026

Copy link
Copy Markdown
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants