SEP-1715: Reserve every synthesized execution field name against frontmatter parameter collisions - #1290
Open
marcuscruz-percona wants to merge 11 commits into
Open
SEP-1715: Reserve every synthesized execution field name against frontmatter parameter collisions#1290marcuscruz-percona wants to merge 11 commits into
marcuscruz-percona wants to merge 11 commits into
Conversation
A frontmatter parameter named executor_host, sudo or script_preview made the execution form fail to build: the synthesized schema carried two fields with that wire name and AppSchema validation aborted with "duplicate field name(s) across form sections", naming neither the parameter nor the frontmatter that produced it. Only the fourth such name, extra_args, was guarded. The four names had no shared home. Three lived in framework/schema.py and one in snippets/models/constants.py, and neither module could import the other because framework.script_helpers imports snippets.models. snippet. Introduce app/sep/apps/field_names.py as an import-free leaf alongside labels.py and nav_icons.py, owning all four constants plus a RESERVED_EXECUTION_FIELD_NAMES frozenset derived from them, and widen the SnippetMetaParameter.name validator from scalar equality to set membership. The builders and the validator now read the same constants, so reserving a fifth synthesized name is a one-site edit. Reservation is unconditional: sudo is rejected on a sudo: never snippet and script_preview in the disk-script app that never synthesizes it, so an author can apply the rule to the frontmatter in front of them without knowing which app will render it. Rejection stays graceful - the parameter is dropped, an error naming it lands on the validated parameters, and execution is blocked unless invalid parameters are configured to be ignored. Also repoint the bare "sudo" literal backing BaseSnippetArgs.sudo_field, a fifth copy of a reserved name that existed only because the constant was unreachable across the import cycle. Coverage generalises from extra_args to all four names, and the two builders outside the snippets app each gain a case proving a reserved-name parameter is dropped rather than raising, plus a guard that every field they synthesize is a member of the reserved set.
Reserving the synthesized execution field names left one acceptance criterion unmet: a script whose frontmatter declares a reserved name has the parameter dropped and can_execute set to False, but Dipper never checked can_execute, so it still dispatched the script without the argument its author declared. The snippets and disk-script apps already enforce this at their execution-meta builders. Guard build_dipper_meta_from_args, which both the JSON and the legacy form flows funnel through, so one check covers both by construction. Also strengthen the reservation coverage: - Restore the synthesized field-type assertions the generalized tests dropped. Each schema suite now asserts a name-to-widget mapping, so a synthesized field silently retyped fails as loudly as one dropped. - Derive the reserved-set expectation in test_field_names from the module's own constants. A fifth constant added without being added to the frozenset now fails; the retyped expectation stayed green. - Promote the form-field traversal, duplicated across three suites, into tests/app/sep/form_schema_utils.py. - Fix the BaseSnippetArgs docstring to open imperatively and use double-backtick literals, and correct the field_names module docstring, which claimed every app synthesizes every field.
marcuscruz-percona
requested review from
a team,
maxbube,
peter-o-addo and
yyyyyyyan
as code owners
August 5, 2026 20:58
There was a problem hiding this comment.
Pull request overview
This PR prevents frontmatter parameter names from colliding with synthesized execution-form field names by centralizing the wire-name vocabulary (executor_host, sudo, script_preview, extra_args) and rejecting those names during parameter validation. It also tightens Dipper’s execution path so scripts with invalid frontmatter parameters cannot be dispatched unless the operator explicitly opts into ignoring invalid parameters.
Changes:
- Introduce
app/sep/apps/field_names.pyas the single, import-free source of truth for synthesized execution field wire names and the reserved-name set. - Update snippet/disk-script/Dipper builders and models to use the centralized constants and to reject reserved frontmatter parameter names at parse time.
- Add/expand test suites across snippets, disk-script source, and Dipper to assert reserved-name behavior and the “block execution on invalid frontmatter” guard; add a changelog fragment.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/app/sep/snippets/test_schema.py | Expands schema synthesis tests to cover all reserved names and “ignore invalid parameters” behavior. |
| tests/app/sep/snippets/models/test_snippet.py | Parameterizes reserved-name validation and adds execution-blocking assertions. |
| tests/app/sep/snippets/models/test_meta.py | Extends SnippetMetaParameter validation tests to cover all reserved names and near-misses. |
| tests/app/sep/form_schema_utils.py | Adds shared helpers for asserting emitted form field names/types across schema builders. |
| tests/app/sep/apps/test_field_names.py | Adds tests pinning wire spellings, reserved set completeness, and “leaf module has no imports”. |
| tests/app/sep/apps/shared/test_disk_script_source.py | Updates disk-script schema tests to use shared form helpers and cover reserved-name dropping. |
| tests/app/sep/apps/dipper/test_schema.py | Adds Dipper schema tests ensuring reserved-name parameters are dropped and synthesized fields are reserved. |
| tests/app/sep/apps/dipper/test_deps.py | Adds tests asserting invalid frontmatter blocks Dipper execution meta building unless opted out. |
| changelog.d/SEP-1715.fixed.md | Documents user-visible behavior change and the new Dipper dispatch guard. |
| app/sep/snippets/schema.py | Switches to importing synthesized field names from app.sep.apps.field_names. |
| app/sep/snippets/models/snippet.py | Uses centralized constants (and updates args-model docstring) for execution arg field names. |
| app/sep/snippets/models/meta.py | Generalizes reserved-name rejection to the full reserved set with an improved error message. |
| app/sep/snippets/models/constants.py | Removes the old leaf constant module (replaced by app.sep.apps.field_names). |
| app/sep/apps/shared/disk_script_source.py | Switches synthesized field-name imports to app.sep.apps.field_names. |
| app/sep/apps/framework/schema.py | Removes embedded synthesized field-name constants (now owned by field_names.py). |
| app/sep/apps/field_names.py | New import-free vocabulary module defining synthesized wire names and reserved-name set. |
| app/sep/apps/dipper/schema.py | Switches synthesized field-name imports to app.sep.apps.field_names. |
| app/sep/apps/dipper/deps.py | Adds guard blocking execution meta building when the script has invalid frontmatter parameters. |
| app/sep/apps/atw/batch.py | Switches synthesized field-name imports (and extra-args field name) to app.sep.apps.field_names. |
marcuscruz-percona
added
the
qa in progress
The snippets, Dipper and disk-script reserved-name tests each carried the same three assertions plus a near-verbatim paragraph explaining why the whole field set is asserted rather than just the reserved name's absence. Move both into assert_only_synthesized_fields() alongside the traversal helpers they already share, so the rationale has one home and a fourth builder's suite gets it for free. Bare asserts in that module need the same per-file S ignore contract_suite.py already carries. Annotate and document what the reserved-name work added: the four new helpers get :param:/:return: entries, and the new test methods get signature annotations. Pre-existing unannotated methods in these files are left alone. Drop raising=False from the two IGNORE_INVALID_PARAMETERS patches, which would have let the tests pass against a renamed setting, make the per-module synthesized-field maps consistently private, and settle field_names.py on one spelling of "synthesize".
The dispatch guard fires on can_execute, which is false for any parameter validation error -- a malformed constraint or a stale visible_when reference, not only a reserved name. Dipper does not surface validated_parameters.errors on its form, so the operator saw a refusal naming the script and nothing else: no screen in that app told them which parameter was at fault. Enumerate the parser's own messages in the detail so the refusal is self-explaining wherever it is read. The changelog fragment credited the guard to Collect Diagnostic Data, which shares a display name with Dipper's plugin schema but is a different, unchanged app. Name the app whose behaviour changed.
Review asked the helper to fail loudly on a repeated wire name rather than let the dict keep whichever field came last. It cannot reach that state: AppSchema validates uniqueness across form sections in a mode="after" validator, so a builder emitting one name twice raises before any schema object exists to traverse -- the repeat surfaces out of the builder call, not out of an assertion here. Note that where the reader will look for it, so the next reviewer does not re-derive it.
marcuscruz-percona
added
qa passed
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Contributor
Author
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: marcuscruz-percona <272857389+marcuscruz-percona@users.noreply.github.com>
Contributor
Resolved the merge conflicts and pushed the fix in commit |
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
executor_host,sudoorscript_previewmade a snippet or script fail to render its execution form at all: the synthesizedAppSchemacarried two fields with that wire name and validation aborted withduplicate field name(s) across form sections, naming neither the parameter nor the frontmatter. SEP-1664 fixed this for the fourth such name,extra_args, and left the other three unguarded. All four now share a single definition site in the new import-free leafapp/sep/apps/field_names.py, and the frontmatter parameter validator rejects any name inRESERVED_EXECUTION_FIELD_NAMES— so reserving a fifth synthesized name is a one-site edit that cannot be forgotten at the validator.sudois rejected on asudo: neversnippet, andscript_previewin the disk-script app that never synthesizes that field.validated_parametersto build its form but never checkedcan_execute, so a script with a dropped parameter still ran — without the argument its author declared. The guard sits inbuild_dipper_meta_from_args, which both the JSON and legacy form flows funnel through, matching the snippets and disk-script apps. The refusal enumerates the parser's own messages, because Dipper does not surface them on the form: a detail naming only the script would leave the cause invisible everywhere in that app's UI.Tested
- name: sudo, load the snippets detail page, and confirm it renders with a flashed error namingsudoas reserved and the execute button disabled — rather than a 500 from the schema build.executor_hostandscript_preview; confirm the same error and no duplicate-field crash.SNIPPETS__META__IGNORE_INVALID_PARAMETERS=true, reload the same snippet, and confirm the form renders with exactly oneexecutor_hostfield and the execute button enabled.- name: sudointo the Dipper script directory, dispatch it, and confirm the 400 names both the script and the reserved-name validation message.Checklist
make test) — 9316 passed, 421 skippedmake run-pre-commit)make makemigrations) — N/A, no model changeschangelog.d/Notes for reviewers
Behaviour change. No frontmatter in the repository declares any of the four names, so nothing in-tree breaks. A customer artifact that does moves from opaque render crash to parameter dropped, not executable, error explains why — strictly better, but visible, hence the changelog fragment. Separately, the Dipper guard fires on any invalid frontmatter parameter, not just reserved names; all six shipped collectors report
can_execute=True, errors=[], so no in-tree Dipper flow is newly blocked.Not a privilege-escalation fix. Author parameters get opaque generated field names via
create_model, so only the fixed-name synthesized fields could collide. The failure was form-schema construction, not argument double-binding.Reservation is exact-match and case-sensitive.
_validate_unique_field_names_in_formscompares with==into aset[str], soSudoprovably cannot collide, and thenamepattern permits uppercase today — case-insensitive reservation would smuggle an unrelated breaking change into a bugfix.Known residual, narrowed but not closed (out of scope on the ticket): Dipper and the disk-script app still do not surface
validated_parameters.errorson their forms, so a reserved-name parameter is dropped there with nothing on the page to explain it. Dipper's dispatch refusal now names the offending parameter and why, so the operator who tries to execute learns the cause; the disk-script app's refusal still names only the script. Pre-existing forextra_args. Only the snippets routes flash these messages at render time.Minor asymmetry left alone:
EXECUTION_HOST_LABELstill re-exports viaframework.schemawhile the wire names now come from the leaf, so Dipper reaches two leaf modules by two routes. Repointing it has no acceptance-criteria value and costs three extra import lines.Error message scope: the rejection message enumerates the full reserved set, slightly beyond AC-5's "names the offending parameter". Kept deliberately — it saves the author a round trip when picking a replacement name.
Changelog app name corrected. The fragment credited the Dipper guard to Collect Diagnostic Data. That is ATW, which is unchanged here — Dipper's plugin schema happens to share that display name, while its app display name is Dipper Data Collection. The fragment now names Dipper.
Test-support cleanup in the last two commits. The three reserved-name suites each repeated the same three assertions plus a near-verbatim paragraph explaining why the whole field set is asserted rather than only the reserved name's absence; both now live in
assert_only_synthesized_fields()beside the traversal helpers they already shared. Also::param:/:return:on the new helpers, signature annotations on the new test methods (pre-existing unannotated methods in those files left alone),raising=Falsedropped from the twoIGNORE_INVALID_PARAMETERSpatches so a renamed setting cannot silently pass the test, and one spelling of "synthesize" infield_names.py.