Repository navigation
Add tests that HARPER_DEFAULT_CONFIG values survive the built-in config backfill - #3270
Merged
Merged
Conversation
…backfill Boot applies the env config layers in initConfig() before ensureBuiltInComponentConfigKeys() runs, and nothing asserted that the backfill leaves what the env layer supplied. Cover waf.enabled/region/nodeTags on an upgraded config with no waf key, across a restart, and under an empty waf block an earlier boot backfilled, with a control that the backfill still acts when no env layer supplies the key. Dispatch-Task: harper-default-config-backfill-order-test Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dx2k2ATBYemxrCvitR8saa
…eading title and comment The backfill's mirror into the memoized config only runs when it writes a key, which none of the env-layer cases reach. Add a case where the file has lost the key while memory holds the env leaves, so an unconditional mirror now fails. Dispatch-Task: harper-default-config-backfill-order-test Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dx2k2ATBYemxrCvitR8saa
Contributor
There was a problem hiding this comment.
Code Review
This pull request adds a new test file unitTests/config/configUtils-builtInBackfillEnvLeaves.test.js to verify the behavior of built-in component backfills when environment-supplied configuration leaves (specifically HARPER_DEFAULT_CONFIG with waf configurations) are present. The tests cover scenarios such as upgrading configurations without a waf key, maintaining configurations across restarts, filling leaves under empty blocks, preserving memoized leaves, and default backfilling. I have no feedback to provide as there are no review comments and the implementation aligns with the repository's style guidelines.
Picks up 3261bd1 (rocksdb-js 2.12 transaction-log retention), which fixes the QA-822 purge-invariant failure in Integration Tests 1/6 that this PR inherited from its base. Dispatch-Task: pr-maint-4d7a94aed5a7f16e2e17a4f1cfc97d30 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
⊙ Problem
Hosts managed through
HARPER_DEFAULT_CONFIGgetwaf.enabled,waf.regionandwaf.nodeTagsfrom the env config layer. On every bootensureBuiltInComponentConfigKeys()then writeswaf: {}if the key is missing. No test checked that the backfill leaves those values alone, so a change to either step could drop them without a failing test.🤔 Does call order matter?
The task assumed backfilling first would drop the values. I tried it: they survive, because the env layer fills values under an empty
waf: {}. These tests check "the backfill never overwrites" and "the env layer fills under an empty block", not call order. A real-boot WAF activation check would go inqa577-upgrade-builtins.test.ts.💡 Solution
Adds one test file,
configUtils-builtInBackfillEnvLeaves.test.js. On an upgraded config with nowafkey, it runs the env layer and then the backfill, and checks that the three values stay intact in the config file, ingetConfigObj()(what the component loader reads) and in the flat config.The five cases
wafkey: the env layer supplies the values before the backfill runs, and the backfill adds nothing.waf: {}: the env layer fills the values in.wafwhile memory holds the values: the backfill leaves the in-memory values alone.waf: {}, so the cases above cannot pass with the backfill disabled.⚖️ Alternatives
bin/run.tscall order directly — order has no visible effect on the values, so the test would lock in an order nothing depends on.ensureBuiltInComponentConfigKeystests — those write YAML by hand and never run the env layer.env.initSync(true)— it exits the process on a config error instead of failing the test.initConfig()— a production change with no defect to fix, and it would blur the rule that an explicitwaf: falsestays disabled.✅ Verification
npx mochaonconfigUtils-builtInBackfillEnvLeaves.test.js: 5 passing.dist, restored afterwards: removing the backfill's presence check fails 3 cases. Making the env layer skip values under an existing scope fails 3. Making the in-memory mirror unconditional fails the in-memory case. Calling the backfill from insideinitConfig()before the env merge fails none of the value cases.npm run test:unit:main: 7163 passing, 1 failing. The failure,deployCandidateBuild.test.js"closes the git credential socket…", also fails when run alone on this checkout and loads none of this PR's code.test:unit:resourcesandtest:integration:all. This PR adds one file underunitTests/config, which neither suite loads.Integration Tests 1/6failed on this PR's old base inqa822-purge-log-invariants.test.ts, the same as on main at that commit. Main fixed it in Adopt rocksdb-js 2.12's transaction-log retention (#3265). After merging main, that test passes locally (9 of 9).🤖 Generated by Anthropic Claude (Sonnet 5.5); posted via @kriszyp.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Dx2k2ATBYemxrCvitR8saa
Release target: main — test-only addition, no release-line code changes
Related PRs: none found (partial scan: 2 PRs with more than 100 files or 10 linked issues could not be fully matched)
Complexity: easy
Deferred findings
36774f7d87c181dcnit test-coverage —unitTests/config/configUtils-builtInBackfillEnvLeaves.test.js— boot() calls initConfig(true) directly, skipping env.initSync override replay and the non-forced first read that bin/run.ts uses — NOT FILEDOrigin — the dispatch brief this PR was written from
Add a regression test pinning that
HARPER_DEFAULT_CONFIG-supplied component leaf values (for example waf.enabled/region/nodeTags) are not replaced byensureBuiltInComponentConfigKeys()'s empty-object backfill on boot.Acceptance
A unit test that fails if the backfill runs before, or clobbers, env-var-sourced leaf values (demonstrate by temporarily reordering). No production code change needed unless the test finds one.
Dispatch: task
harper-default-config-backfill-order-test· queued by finding-triage/wave-20 · ran by claude/sonnet/xhigh · worker kzyp-xps-1Review-Coverage: authored=claude; ran=gemini,cursor-composer,codex; adjudicated=domain; declined=cursor-grok,cursor-kimi,cursor-muse,muse; rounds=3; full=1 @ 9f14701
Review-Attention: read ~3m (decisions: unit-pin-vs-integration, direct-initconfig-vs-initsync; raised: degraded review) @ 9f14701