fix: reject or warn on top-level param-file keys outside defaults:/grid:/params: (#530) - #537
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…id:/params: (#530) pkg/params.ParamFileFormat has exactly three fields, and both json.Unmarshal and yaml.Unmarshal silently drop any top-level key that is none of them — a ttl:/idle_timeout:/cost_limit: written one indentation level too high used to vanish with no trace at all, not even a PARAM_* env var. Two of spawn's own shipped examples had this bug: examples/simple-params.yaml and examples/schedule-params.yaml, both fixed here. A top-level key that is a recognized spawn setting (reusing #526's recognizedRowKeys/reservedRowKeys registry) is now a hard error before anything is launched or priced. Anything else unrecognized is a warning printed to stderr, so harmless metadata (description:, version:) does not break. Covers spawn launch --param-file (pkg/params + cmd/sweep_keys.go), spawn resume (which reloads the file independently), and spawn schedule create (its own separate parseParamsFile in cmd/schedule.go).
scttfrdmn
force-pushed
the
fix/530-top-level-param-keys-dropped
branch
from
August 19, 2026 21:55
0009820 to
d486bfe
Compare
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.
Closes #530.
The bug
pkg/params.ParamFileFormathas exactly three fields —defaults,grid,params— and both parsers (pkg/params/parser.go's JSON and YAML paths)unmarshal straight into it. Any top-level key that is none of those three is
discarded silently, before validation ever sees it — not passed through as a
PARAM_*env var (which at least leaves a tag/trace), just gone.Two of spawn's own shipped examples were written wrong this way:
examples/simple-params.yamlhadregion:/instance_type:/ami:at thetop level.
examples/schedule-params.yamlhadsweep_name:/region:/max_concurrent:/launch_delay:/instance_type:/ami:/disk_size:atthe top level — and
cmd/schedule.go'sgetSweepName/getMaxConcurrent/getLaunchDelayonly ever read those fromdefaults:.The dangerous case:
ttl:,idle_timeout:,cost_limit:at the top level —an easy mistake, and the same shape one level up from the row-level keys
#526 already fixed — vanish completely, producing an unbounded instance with
no error, no warning, no tag.
The fix
Per the issue's suggested design, a blanket error is not right — most
unknown top-level keys are harmless metadata. So:
ttl,idle_timeout,cost_limit,instance_type, ..., plus the reservedCLI-only/near-miss list —
sweep_name,max_concurrent,launch_delay,ttl_hours,budget, etc). This reuses Unknown param-file keys silently become PARAM_* env vars — a mistyped or invented spend control caps nothing #526'srecognizedRowKeys/reservedRowKeysregistry rather than a second, independently-driftinglist. Message:
"ttl" at the top level is ignored; move it under defaults: to apply it to every row.description:,version:, and other reasonable metadata. Printed to stderr the same wayUnknown param-file keys silently become PARAM_* env vars — a mistyped or invented spend control caps nothing #526's passthrough-params list is, so it has somewhere to be noticed.
examples/simple-params.yamlandexamples/schedule-params.yamlinthis PR, since the new check fires immediately on spawn's own examples
otherwise.
Scope:
pkg/paramsANDcmd/schedule.goThe issue flagged that
spawn scheduleuses its own separateparseParamsFileincmd/schedule.go, notpkg/params— a fix inpkg/paramsalone would not cover it. Both paths turned out tractable to fixwith the same shared classification/error logic
(
classifyTopLevelKey/validateTopLevelParamKeysincmd/sweep_keys.go),so both are fixed here:
pkg/params.ParamFileFormatgets a newUnknownTopLevelKeys []stringfield, populated by a second generic-map decode pass in
parseJSON/parseYAML(decoding twice rather thanDisallowUnknownFields, sinceunknown keys are not automatically an error here).
cmd/schedule.go'sparseParamsFile/parsedParamsgets the sameunknownTopLevelKeysdetection, independently, since it has its own structand its own copy of this bug.
cmd/launch_sweep.go(thespawn launch --param-filesweep path) andcmd/resume.go(which reloads the file independently) both callvalidateTopLevelParamKeysbeforevalidateSweepParamKeys's existing Unknown param-file keys silently become PARAM_* env vars — a mistyped or invented spend control caps nothing #526check, so a top-level mistake is reported first — it means a setting the
user wrote was never even read, the more dangerous of the two failures.
cmd/schedule.go'srunScheduleCreatecalls the same function.Deliberately NOT unified: the launch path treats
defaults:as a per-rowfallback, while the schedule path treats it as sweep-scope (reading
sweep_name/max_concurrent/launch_delayfrom there) — a real, pre-existingdivergence the issue called out as not necessarily in scope to fix here. Kept
as-is;
examples/schedule-params.yaml's comments now document it.Tests
pkg/params/parser_test.go:TestParseYAML_UnknownTopLevelKeys/TestParseJSON_UnknownTopLevelKeys(both formats populate the fieldcorrectly, and the dropped key does not leak into
Defaults),TestParseYAML_NoUnknownTopLevelKeys(clean file → empty), andTestParseYAML_GridIsNotUnknown(grid: itself is not flagged).cmd/sweep_keys_test.go:TestClassifyTopLevelKeyErrorsOnDangerousKeys/...OnReservedKeys/...WarnsOnHarmlessKeys, andTestValidateTopLevelParamKeys{Errors,WarnOnlyPasses,AcceptsCleanFile}.test/e2e/tier0_sweep_top_level_keys_test.go(Substrate, zero spend), 3tests: a top-level
ttl: 2hrejected with nothing launched(
requireNothingLaunched, same style astier0_sweep_param_keys_test.gofrom Unknown param-file keys silently become PARAM_* env vars — a mistyped or invented spend control caps nothing #526), a top-level
cost_limit: 5likewise, and a harmless top-leveldescription:that only warns (named in stderr) while the sweep stilllaunches and gets tagged normally.
The
ttl/cost_limitrejection tests pass a real--ttl 1hon the commandline so the pre-existing
--no-detach"every row needs a bound" guard(#525) would otherwise let the launch through — without it a non-zero exit
would prove only that the row was unbounded, not that the top-level key
itself was caught. All 3 new e2e tests + the 2 new unit test files verified
failing (compile error, pre-fix) and passing (post-fix) via a stash-based
before/after run. Full
test/e2e -tags=e2e_tier0suite andmake checkbothgreen.