Repository navigation
feat(cli)!: reject duplicate keys in the eval config on every entry point - #257
Merged
Merged
Conversation
…oint A key repeated within one mapping of the evaluation config was a warning under --dry-run and silently last-wins under run, eval and EvalSession, so a second api_base or capabilities: decided the run unseen. All of them now read the config through sieval.cli._config_yaml.load_eval_config, which refuses a repeat with the key and both lines. run and run --dry-run refuse before any launch planning. YAML merge keys may still be overridden. Scope: the eval config only. Recipe YAML, sieval infer target files and files sieval wrote itself keep yaml.safe_load; a test pins every yaml load under sieval/cli. Unknown top-level keys stay a warning. BREAKING CHANGE: a config that repeats a key is now refused; load_yaml_with_duplicate_check is removed. Closes #255 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ethan-scitix
force-pushed
the
feat/reject-duplicate-eval-config-keys
branch
from
October 27, 2026 17:44
aa20765 to
1ac10be
Compare
…nfig The duplicate check ran in construct_mapping, but flatten_mapping rewrites a merge source's node in place. An anchor built after a mapping merged from it already held its inherited keys, so an override read as a repeat. Check each node once, in flatten_mapping, on its keys as written. Also trims the loader docstring, test docstrings and config guide note. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 27, 2026
Merged
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.
Type
Summary
sieval run,sieval eval, both--dry-runs andEvalSession(...)read the config through one loader,sieval/cli/_config_yaml.py::load_eval_config. It raisesDuplicateKeyError(aValueError) listing every repeat:<path>: duplicate key 'api_base' at line 5 (first at line 4).--dry-runonly warned.run,evalandEvalSessionused bareyaml.safe_loadand silently kept the last value, so a secondcapabilities:was lost before [RFC]: A capability-based Model IR — decouple model access from the OpenAI protocol #25's schema validator saw it.runandrun --dry-runrefuse before any launch planning (checked in_run_all/_run_dry_run, not left toEvalSession, which builds after engines start). Under--dry-runit is a failedyaml_syntaxcheck (exit 1).safe_loadsemantics hold. Keys are compared after construction (1and0x1collide). Keys a YAML merge (<<: *anchor) brings in may be overridden, including through a chain of anchors. Each mapping is checked once, inflatten_mapping, on its keys as written.sieval infertarget files and files sieval wrote itself (read_persisted_config, the effective_config diff) keepyaml.safe_load.test_no_other_yaml_load_in_the_clipins everyyaml.*loadundersieval/cli.alignment:). That belongs with the RFC [RFC]: A capability-based Model IR — decouple model access from the OpenAI protocol #25 capability-name errors.sieval.cli.validation.load_yaml_with_duplicate_checkis removed (its only caller was the dry-run). In-o jsondry-run output,yaml_syntax.warningsis now always empty; a repeat is the check'sdetail. Not in any identity; resume unaffected; noRESULT_SCHEMA_VERSIONbump. The CHANGELOG entry is staged for the release cut.Related Issues
Closes #255
Refs #25
Test Plan
Automated
ruff check,ruff format --check, pre-commit hooks: cleanpdm run ty check(tests/included): All checks passedpytest tests/unit tests/integration -m "not stress": 10055 passed, 1 skippedNew
tests/unit/cli/test_config_yaml.py:safe_load; top-level, nested and multiple duplicates; merge overrides allowed (flat, and a chain through a nested anchor); a duplicate after a merge is still refused; keys equal after construction; independent loads.run,run --dry-run,eval,eval --dry-run(-o json, exit 1),EvalSession(...), and_run_all/_run_dry_runwith_prepare_launch_batchnot called.yaml.safe_loadfails at least one test. The previousconstruct_mapping-based check fails the nested-anchor merge test.tests/unit/cli/test_validation.py: the last-wins helper tests go with the helper;test_duplicate_key_warningsbecomestest_duplicate_key_is_an_error.Checklist
Required (all PRs)
type(scope): description)AI-Generated Code - <model> (<provider>)in module docstringcore/(core/untouched)load_yaml_with_duplicate_checkhad one caller)If: Breaking Change
🤖 Generated with Claude Code