fix: cast env-var strings before comparing in updateConfigValue#499
Merged
Conversation
updateConfigValue's "skip if values are the same" check used loose
equality, which handles string<->number ('9925' == 9925) but not
string<->boolean ('true' != true) or string<->array. Any deployment
with a boolean or array env var (e.g. LOGGING_STDSTREAMS=true in the
default Harper Dockerfile) hit the write path on every restart even
when nothing actually changed, generating a backup and rewriting the
config file each boot.
Cast the env value through castConfigValue (the same coercion the write
path applies a few lines below) and deep-compare. With this fix the
check correctly skips the update when env-derived values match the
on-disk config regardless of source type.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Reviewed; no blockers found. |
DavidCockerill
approved these changes
May 8, 2026
cb1kenobi
approved these changes
May 8, 2026
github-actions Bot
pushed a commit
that referenced
this pull request
May 8, 2026
…typecast fix: cast env-var strings before comparing in updateConfigValue
2 tasks
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.
Summary
Fix
updateConfigValue's "skip if no changes" check (config/configUtils.js:516) to cast env-var strings to typed values before comparing. Without this, every Harper restart rewrites the config file (and creates a backup) for any deployment that uses a boolean or array env var.Why
Env vars arrive in
process.envas strings — there is no boolean/number env var at the OS level. The check was using loose equality:Loose equality coerces string↔number cleanly (
'9925' == 9925→ true) but not string↔boolean ('true' != truebecause'true'becomesNaN,truebecomes1, andNaN != 1) and not string↔array. Any deployment with a boolean or array config env var trips the check on every boot.This affects the default Harper Dockerfile, which sets
LOGGING_STDSTREAMS=true. Every Harper container is rewriting its config and creating a backup on every restart, even with no actual config change. Same md5sum across N backups — observed in the field.This was also the trigger for the partial-config corruption fixed in #493: every restart was opening the truncate window for
fs.writeFileSyncwhile concurrent readers were starting up. With #493's atomic write, the corruption no longer occurs, but the spurious writes and backups still happen. This PR stops them.What changed
config/configUtils.js:511-526: castparsedArgs[arg]throughcastConfigValue(the same coercion already used by the write path 80 lines below) and deep-compare with_.isEqual.unitTests/config/configUtils.test.js: added two tests — one verifying string env values matching typed config values are detected as equal (the fix), one verifying genuine differences still trigger an update (no regression).Areas to look at
'' vs null,'0' vs null,'3d' vs '3d',null vs undefined) and didn't find a regression — the new comparison agrees withcastConfigValue, which is what the actual write path uses anyway.castConfigValueignores itsparamargument (pre-existing — there's a TS6133 diagnostic on it). Passingargworks because it's never read; if someone later wiresparamup, this comparison call site would need to use the canonicalCONFIG_PARAM_MAP[arg.toLowerCase()]instead.initialize()calls (launch()runs it before forking, daemon child likely runs it again). With this fix, the second call sees no diff and skips cleanly. If duplicate writes still occur after this lands, that's a separate bug.Test plan
npm run test:unit:configpasses locally (130 passing)