fix(config): sanitize argv before merging into resolved config - #2002
fix(config): sanitize argv before merging into resolved config#2002gfargo-horizon-agent[bot] wants to merge 3 commits into
Conversation
Strip undefined-valued argv keys and yargs bookkeeping (_, $0), and deep-merge an argv-supplied `service` instead of replacing it wholesale, mirroring the sanitization already done by the env/git config loaders. Previously an unset argv flag could silently clobber a resolved config value with `undefined`, and a partial `service` override dropped the rest of the resolved service config.
![gfargo-horizon-agent[bot]](https://avatars.githubusercontent.com/in/4083186?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 Agent review (sonnet) — LGTM
REVIEW: LGTM
RESOLVES: full
The PR implements exactly the plan: strips yargs' _/$0, runs removeUndefined on argv before merging, and deep-merges service one level mirroring services/env.ts. All three regression tests pass, lint and typecheck are clean, the change is scoped to the two files the plan specified, and I verified downstream callers (commandExecutor.ts) get raw argv separately from the sanitized options, so stripping _/$0 from the merged config has no live-behavior impact.
There was a problem hiding this comment.
🔎 Agent review (sonnet) — LGTM
REVIEW: LGTM
RESOLVES: full
The diff replaces the shallow { ...config, ...argv } spread with a sanitized merge that strips yargs' _/$0, removes explicitly-undefined argv keys via the existing removeUndefined util, and deep-merges an argv-supplied service — exactly mirroring the pattern already used in services/env.ts. Three new targeted tests cover the undefined-clobber, bookkeeping-key-strip, and service-deep-merge cases, and all 9 tests in the file pass.
There was a problem hiding this comment.
🔎 Agent review (sonnet) — LGTM
REVIEW: LGTM
RESOLVES: full
The diff sanitizes argv before merging into resolved config exactly as the ticket specifies: strips yargs _/$0, filters undefined values via the existing removeUndefined helper, and deep-merges service one level, mirroring the identical precedent already established in services/env.ts. Scope is tight (only loadConfig.ts + its test file), and three new tests directly cover the three gaps called out in the work item.
There was a problem hiding this comment.
🔎 Agent re-review (sonnet, delta) — LGTM
REVIEW: LGTM
RESOLVES: full
The revise adds a shallow one-level deep-merge for the argv-supplied service object plus three targeted tests, matching the existing precedent in services/env.ts. No new bugs, scope creep, or missing coverage were introduced.
…1922-lib-2-fix-config-the-argv-spre # Conflicts: # src/lib/config/utils/loadConfig.ts
There was a problem hiding this comment.
🔎 Agent re-review (sonnet, delta) — CONCERNS
REVIEW: CONCERNS
RESOLVES: partial
The added service deep-merge closes the shallow-replace gap (b), but it merges argvService without stripping nested undefined leaves, so a synthetic argv like { service: { provider: 'anthropic', model: undefined } } (the exact pattern the issue calls out for defaultRouter.ts) still clobbers a resolved service.model with undefined — reopening a narrower copy of bug (a) one level down. No test exercises this nested case.
1 concern — 1 inline on the diff
| if (argvService && config.service) { | ||
| merged.service = { | ||
| ...(config.service as object), | ||
| ...(argvService as object), |
There was a problem hiding this comment.
cleanedArgv = removeUndefined(restArgv) only strips top-level undefined keys; it does not recurse into service. So if the argv object's service field itself contains an explicitly-undefined property (e.g. { provider: 'anthropic', model: undefined }, plausible from a hand-built synthetic argv like defaultRouter.ts produces), the deep-merge spreads that undefined over config.service, silently dropping a resolved model/authentication/endpoint value — the same clobbering bug the PR is fixing, just scoped to the nested object instead of the top level.
| ...(argvService as object), | |
| merged.service = { ...(config.service as object), ...removeUndefined(argvService as Record<string, unknown>) } as Config['service'] |
…erge The service deep-merge added in #2002 spread argv.service as-is, so a synthetic argv like { service: { model: undefined } } (the shape defaultRouter.ts hand-builds) still clobbered a resolved service.model with undefined — reopening the original bug one level down.
What
Hardens the final argv merge in
loadConfigso it can't silently clobber resolved config or drop parts of a mergedserviceobject.Why
Closes #1922
Plane: OSS-1637
The env and git-config loaders both sanitize themselves (
removeUndefined), but the trailing{ ...config, ...argv }spread inloadConfigdid not — so anyundefined-valued argv key overwrote a resolved value withundefined, the spread shallow-replaced an argv-suppliedserviceobject entirely, and yargs'_/$0bookkeeping keys leaked into the returnedConfig. Currently latent (today's callers never emitundefinedargv keys), but the fix removes the landmine for any future caller (e.g. hand-built synthetic argv) that does.How
_,$0) out of argv before merging.removeUndefinedon the remaining argv so unset flags never clobber a resolved value.serviceone level, mirroring the existing merge inservices/env.ts, instead of letting the top-level spread replace it wholesale.Testing
loadConfig.test.ts, all 9 tests in the suite pass)🤖 Generated by the harbor agent loop. Reviewed by a human before merge.
Relates to #1922