SEP-1715: Reserve every synthesized execution field name against frontmatter parameter collisions - #1290
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.
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 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: marcuscruz-percona <272857389+marcuscruz-percona@users.noreply.github.com>
Resolved the merge conflicts and pushed the fix in commit |
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: marcuscruz-percona <272857389+marcuscruz-percona@users.noreply.github.com>
Resolved the merge conflicts and pushed the fix in commit |
github-actions
Bot
added
app:atw
get_dipper_execution_meta was deleted from dipper/deps.py by main's legacy Jinja SSR removal (SEP-1687); the merge into this branch left a stale import and test exercising it, breaking collection for the whole suite. The guard it covered is already exercised by test_reserved_parameter_name_blocks_the_shared_meta_builder via the shared meta builder both execution flows use. Also fixes ruff-flagged unused/unsorted imports in three files.
yyyyyyyan
left a comment
yyyyyyyan
left a comment
There was a problem hiding this comment.
@marcuscruz-percona — the guard ordering here is the part I want to call out: because deps.py:322 raises on a None interpreter first, not script.can_execute at line 324 can only mean the second conjunct of BaseSnippet.can_execute failed, so validated_parameters.errors is provably non-empty and the message can never trail an empty colon. DipperScript extending BaseSnippet rather than Snippet keeps the approval gate out of it too. On the test side, each test_every_synthesized_field_name_is_reserved pairs its <= subset assert with an == dict assert, which is what stops the subset form from passing vacuously on an empty field list, and test_reserves_every_field_name_the_module_declares reads its expectation off vars(field_names) instead of retyping it — that test is the single-definition-site guarantee rather than a restatement of it.
The guard's documented rationale rests on a second Dipper flow that no longer exists — app/sep/apps/dipper/deps.py:299. The docstring says "legacy and JSON Dipper flows" and then justifies the guard's placement with "Both flows assemble their meta here … rather than at either entry point". There is one flow and one entry point: build_dipper_meta_from_args has exactly one caller (build_dipper_execution_meta, deps.py:340), which has exactly one caller (dipper_api_execute, api_routes.py:205), the only POST route on the router — and get_dipper_execution_meta is gone from the tree. This branch's own final commit cbc6fc5 deleted the legacy-flow test for that reason, so the tests were corrected and the prose that asserts the same fact was left standing in a docstring the PR rewrote. tests/app/sep/apps/dipper/test_deps.py:174 repeats it.
One thing with no line in the diff to hang it on: app/sep/apps/shared/disk_script_source.py:263 refuses with a detail naming only the script, though the reason given for appending reasons in Dipper — that the app does not surface validated-parameter errors on its form — holds there identically. Two lines would leave the siblings symmetric.
Eight smaller items are inline.
Approving — the findings above are yours to take or leave; resolve the threads when you have.
|
Two items from this review that are out of scope for SEP-1715 and want their own tickets. The same condition raises different status codes across the three script apps. Dipper's plugin schema carries ATW's display name. |
- Drop the stale "legacy and JSON Dipper flows" rationale: there is one flow and one entry point, so the guard's placement is described by the seam it sits on. Same claim removed from the test docstring. - Replace the dead ``:meth:`` cross-reference to a private validator with the generic statement, matching the sibling leaf modules. - Give the wire-name constants ``#:`` doc-comments. - Pin the union of ATW's two batch-merge sets against the reserved set, so a fifth synthesized name cannot be reserved centrally while staying an ordinary shareable parameter in a batch form. - Scope the uniqueness guarantee in the form-schema helpers to task-style schemas, which is the branch the validator actually runs, and reconcile the two docstrings that disagreed about it. - Narrow the helpers' field-class mappings to ``type[SchemaBaseModel]``. - Keep the bare-assert suppression inline on the three asserts that need it instead of relaxing ``S`` for the whole helper module. - Append the parser's messages to the disk-script refusal detail, so it matches its Dipper sibling. - Fix ``synthesises`` and the ``--`` runs that render as en dashes in rST.
|
Review addressed in The disk-script sibling asymmetry is fixed. The suppression-config change is gone rather than documented.
The two out-of-scope items (403-vs-400 across the three script apps, and Dipper's plugin schema carrying ATW's display name) I'll file as their own tickets. |
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.