fix(585): validate context_scope directives against the closed registry#800
Merged
Merged
Conversation
… registry Unknown/typo'd directives must raise (were silently copied through, inert); removed names (seed_path/static_data) raise the SAME generic error naming the valid set, with no rename narrative.
normalize_context_scope silently copied any unrecognized directive into the scope, where no consumer ever reads it — a typo like 'observ:' scoped nothing and raised no error. A hardcoded old->new rename map (REMOVED_DIRECTIVES) loudly caught two specific removed names while every other typo stayed silent. Replace both with one closed-set guard: a directive name not in DIRECTIVE_REGISTRY raises ConfigurationError naming the key and listing the valid set (derived from the registry). Typos, removed names, and unknown keys now fail the same loud, uniform way — no per-name rename narrative. Make DIRECTIVE_REGISTRY the single authority for the valid set: ContextScopeDict carried a dead 'keep' (no runtime consumer, no YAML uses it) and lacked 'drops'; align it to the registry and drop the dead 'keep' read in the docs parser. Test inversion: test_handles_unknown_directives asserted the silent pass-through (bug-as-feature) and TestRemovedDirectives asserted the rename hint; both now assert the loud generic error.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
normalize_context_scopesilently accepted any unrecognizedcontext_scopedirective: it copied the value into the normalized scope, where no downstream consumer ever reads an unknown key (consumers only readobserve/passthrough/drop/drops/seed). So a typo likeobserv:scoped nothing and raised no error — a silent-config-typo class. A hardcodedREMOVED_DIRECTIVES = {"seed_path": "seed", "static_data": "seed"}map loudly caught two specific removed names with a rename-migration message, while every other typo stayed silent.Both are replaced with one closed-set guard: a directive name not in
DIRECTIVE_REGISTRYraisesConfigurationErrornaming the key and listing the valid set (derived from the registry). Typos, removed names, and unknown keys now fail the same loud, uniform way — no per-name rename narrative, nothing project-specific in the framework.Single authority for the valid set:
ContextScopeDictcarried a deadkeep(no runtime consumer, no YAML uses it as a directive — only as a field ref likeaggregate_votes.keep) and lackeddrops. Aligned it to the registry and removed the deadkeepread in the docs parser.Design decisions
seed_path:configs — a backward-compat shim. Every wrong name now gets the same generic "not a valid directive. Valid directives: …" line.DIRECTIVE_REGISTRYis the truth), not registry-only, so the type stops lying.Verification
test_handles_unknown_directivesasserted the silent pass-through;TestRemovedDirectivesasserted the rename hint) + 2 new (observtypo, unknown key). Genuine assertion failures.ruff+mypyclean.keepfield-refs untouched, valid-set can't drift (it issorted(DIRECTIVE_REGISTRY)), andworkflow_static_analyzer.pyalready catches theConfigurationErrorand maps it toStaticTypeError.agac inspect -a ql_domcthrough the actual loader, not a unit dict):scan-projectcanary divergence is pre-existing (baseline.shconfirms it fails on main too; stale project action registry, unrelated to directive validation). Filed as a separate follow-up.Notes
seed_path:(8+) now get the generic error — they already failed on main via the old map. Migrating themseed_path:→seed:is a separate change in the qanalabs repo; the framework carries no per-user migration text.