SEP-1664: Emit the Extra Args field on schema-driven snippet forms

[marcuscruz-percona]

Summary

• build_snippet_schema() now renders the Extra Args field for snippets that opt into allow_extra_args, matching the legacy Jinja form, and the ATW batch form / snippets detail page / alert-troubleshooting accordion all pick it up from that single lowering. Extra Args stays strictly per-snippet in the ATW batch schema and dispatch path (never shared-merged), and the execution model accepts the value under both the new and legacy wire spellings so the Jinja path keeps working until SEP-1500 retires it.
• Follow-up fix from review: reject a frontmatter parameter literally named extra_args at parse time, since it collided with the synthesized Extra Args field's wire name. Before this, the collision either surfaced as an opaque duplicate field name(s) error from build_snippet_schema(), or — worse — silently double-bound a submitted value in the execution model (--configured=VALUE VALUE), independent of whether the form was ever rendered.
• Collapsed the wire name "extra_args", previously defined independently in three places (framework/schema.py, snippet.py x2) with an import-cycle comment explaining why, into one cycle-free app/sep/snippets/models/constants.py module both sides import.
• Clarified ATWBatchExecuteWrite.shared_args / ATWMergedSchemaResponse.shared docstrings to spell out that NON_SHAREABLE_FIELD_NAMES (Extra Args) is excluded regardless of declaration, not just "filtered to declared parameters."
• Dropped a lingering ticket reference from a test docstring.

Tested

• Loaded a snippet with allow_extra_args: true, confirmed the Extra Args field renders on the snippet detail page and in the ATW batch form, and that a submitted value reaches the executed command.
• Attempted to declare a frontmatter parameter named extra_args on a snippet and confirmed it's rejected with a clear "reserved" validation error instead of loading.
• Confirmed a batch of two snippets that both declare Extra Args does not merge it into the shared section.

Checklist

• New/modified functions have type hints and rST docstrings
• New tests added for new features or bug fixes
• All tests pass locally (make test)
• Pre-commit hooks pass (make run-pre-commit)
• Database migrations generated if models changed (make makemigrations) — N/A, no model changes
• User-facing changes documented (README, inline help, UI text)
• Configuration changes documented with examples — N/A
• Changelog fragment added under changelog.d/ if the change is user-facing (make changelog-add)

[Copilot]

Pull request overview

This PR updates SEP’s schema-driven Snippets execution forms to surface the “Extra Args” field when a snippet opts into it (allow_extra_args), keeps ATW batch behavior consistent (never merging Extra Args into shared args), and hardens snippet frontmatter parsing by rejecting a parameter name that would collide with the synthesized execution field.

Changes:

• Emit the schema-driven extra_args field for opted-in snippets and accept both new (extra_args) and legacy (-extra_args-) submission spellings.
• Prevent wire-name collisions by rejecting a frontmatter parameter literally named extra_args.
• Ensure ATW batch schema/dispatch never promotes or applies Extra Args from shared args, and update OpenAPI snapshots/specs + add tests.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/app/sep/snippets/models/test_snippet.py	Adds unit coverage for Extra Args binding (new + legacy), absence when not opted in, and reserved-name rejection.
tests/app/sep/snippets/models/test_meta.py	Adds validation coverage for rejecting reserved parameter names.
tests/app/sep/snapshots/openapi/atw.json	Updates OpenAPI snapshot docstrings describing non-shareable fields behavior.
tests/app/sep/apps/snippets/test_script_source.py	Adds end-to-end tests ensuring Extra Args flows into the built command (new + legacy spellings).
tests/app/sep/apps/snippets/test_schema.py	Adds schema tests for rendering/omitting Extra Args and for dropping reserved-name parameters.
tests/app/sep/apps/atw/test_batch_api.py	Adds batch API tests ensuring Extra Args stays per-snippet and is dropped from shared_args, plus dispatch verification.
frontend/packages/api/specs/sep.json	Updates the frontend API spec descriptions to match the new non-shareable semantics.
changelog.d/SEP-1664.fixed.md	Documents the user-facing schema-driven Extra Args rendering and reserved-name rejection.
app/sep/snippets/models/snippet.py	Adds AliasChoices so execution args accept both legacy and schema-driven extra_args keys; centralizes the field name constant usage.
app/sep/snippets/models/meta.py	Rejects frontmatter parameter name collisions with the synthesized Extra Args execution field.
app/sep/snippets/models/constants.py	Introduces a cycle-free shared home for the extra_args wire-name constant.
app/sep/apps/snippets/schema.py	Adds the Extra Args field to the Execution section when allow_extra_args is enabled.
app/sep/apps/framework/schema.py	Re-exports the shared Extra Args field-name constant and clarifies docs around synthesized execution fields.
app/sep/apps/atw/batch.py	Defines NON_SHAREABLE_FIELD_NAMES (Extra Args) and ensures dispatch filtering excludes it from shared_args.
app/sep/apps/atw/api_routes.py	Ensures Extra Args is never considered for shared-field merging in the merged schema response.

[yyyyyyyan]

@marcuscruz-percona — this lands the field in the right place and proves it end-to-end. Putting the emit in build_snippet_schema() means the detail page and the alert-troubleshooting accordion pick it up from the same lowering, and the label/placeholder/description are a byte-faithful port of the legacy EXTRA_ARGS_INPUT at app/sep/snippets/models/snippet.py:102-107 — nothing dropped, which is the usual failure mode for this kind of port. I also like that dispatch_batch_item closes the shared_args back door rather than stopping at the merged schema: keeping extra_args out of shared would have left a caller free to POST it directly, and test_shared_args_never_apply_extra_args pins that second path with a comment saying why it exists separately. The tests reach the built command string rather than stopping at "the field renders", which is the assertion that actually catches an alias mismatch.

One thing I'd like changed before merge, plus a few small ones — most are inline with suggestions attached.

The # noqa: F401 re-export is dead, and its comment is inaccurate — app/sep/apps/framework/schema.py:86-88. The comment says the name is re-exported "for existing importers", but EXTRA_ARGS_FIELD_NAME doesn't exist on main at all, so there are none. The only two importers are app/sep/apps/atw/batch.py:40 and app/sep/apps/snippets/schema.py:57, both added here, and both can reach app.sep.snippets.models.constants directly — snippets/schema.py already imports from app.sep.snippets.models.meta and ...models.snippet at lines 69/74, and constants.py is a zero-import leaf so batch.py can't cycle on it. Dropping the re-export also lets the constants docstring go back to its three-name wording, which is the second inline comment.

parameter_fields()'s docstring no longer describes what it returns — app/sep/apps/atw/batch.py:265-271. Its diff is empty so nothing points at it, but the enumeration ("an executor-host selector, a sudo toggle, and a script-preview pane") is now missing a fourth item, and :return: Every parameter field the snippet declares is no longer true — the returned list carries a synthesized field the snippet doesn't declare. That's load-bearing rather than cosmetic: dispatch_batch_item:373 calls the derived set declared and has to subtract NON_SHAREABLE_FIELD_NAMES from it precisely because the promise stopped holding. Worth adding Extra Args to the enumeration and qualifying the return.

The rest are inline: validator placement in SnippetMetaParameter, a missing :raises:, a noun-phrase docstring opener, and one sentence on the changelog fragment.

Separately, I left a follow-up comment about the reserved-name guard covering one of the four synthesized field names — that's not a change request for this PR, just something worth tracking.

Happy to re-review once the re-export comes off; everything else is small.

[yyyyyyyan]

Reserve every synthesized execution field name against frontmatter parameter collisions, not just extra_args.

build_snippet_schema() synthesizes four execution-section field names: executor_host and script_preview unconditionally, sudo conditionally, and now extra_args. app/sep/apps/framework/schema.py:1334-1336 raises duplicate field name(s) across form sections when a frontmatter parameter collides with any of them, so a snippet declaring a parameter named executor_host still produces exactly the opaque error this ticket set out to eliminate.

Prioritizing extra_args here was right — it is the only one of the four that also double-binds, since to_validation_field() sets alias=self.name and the execution model accepts extra_args as a validation alias, so a submitted value silently bound twice. The other three are pre-existing and out of scope for this PR. Worth reserving the whole synthesized set from one place rather than one name at a time.

[yyyyyyyan]

Tracked the follow-up from this comment as SEP-1715: Reserve every synthesized execution field name against frontmatter parameter collisions, not just extra_args.

[marcuscruz-percona]

@copilot please fix the merge conflicts in this pull request.

[marcuscruz-percona]

All change requests addressed in 8d16d85, plus the two items from the review body that had no inline thread:

• parameter_fields() docstring (app/sep/apps/atw/batch.py:263-274) — the enumeration now lists Extra Args as a fourth synthesized field and says why it stays per-snippet, and :return: is qualified to admit the synthesized field the snippet does not declare. That is the promise dispatch_batch_item relies on when it subtracts NON_SHAREABLE_FIELD_NAMES.
• Framework re-export — gone, along with its # noqa: F401; the constants docstring is back to its main three-name wording.

Merge conflicts are resolved: main (d401dd020) is merged in as c16b6fc and the branch merges clean.

Verification: ruff check / ruff format --check clean on all touched files; tests/app/sep/apps/atw + tests/app/sep/snippets → 701 passed, 11 skipped; OpenAPI snapshot and schema tests → 633 passed, 2 skipped. No snapshot churn, so the docstring edits stayed out of the wire contract.

The SEP-1715 follow-up (reserving all four synthesized names) is left untouched here as agreed.

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
app/sep/apps/atw
batch.py
app/sep/snippets
schema.py
app/sep/snippets/models
constants.py
meta.py
snippet.py
app/sep/sync/syncers
pmm.py
Project Total

This report was generated by python-coverage-comment-action

[yyyyyyyan]

@marcuscruz-percona — all seven threads check out at a235da0c. The re-export is gone and both consumers reach app.sep.snippets.models.constants directly, the framework constant-block docstring is byte-identical to main again, and the reserved-name validator now sits above normalize_visibility_condition so the field list runs uninterrupted from name through sensitive. Folding the exclusion into shared_field_names is behaviour-preserving as expected: both downstream reads of declarations in atw_execution_schema still filter on shared_names (lines 320-322 and 328), so the merged schema and the per-snippet split come out identical, dispatch_batch_item:381 still closes the shared_args path, and test_batch_api.py:603 covers it end-to-end. parameter_fields's docstring picked up the fourth item and the qualified :return: too.

Two docstring artifacts the refactor left behind, neither blocking:

Stale cycle rationale in constants.py — app/sep/snippets/models/constants.py:16,21-25. With the re-export gone, app/sep/apps/framework/schema.py has no reference to the constant at all, but the module docstring still describes the constants as "shared across snippet and framework schema modules" and the constant's own docstring opens with app.sep.apps.framework.schema and app.sep.snippets.models.snippet "both need it". The cycle argument itself still holds — framework imports snippet via script_helpers.py:55, so the models can't import back — but the consumer list is now app.sep.snippets.models.meta / .snippet, app.sep.snippets.schema, and app.sep.apps.atw.batch. Summary bullet 3 in the PR description has the same "both sides import" phrasing if you want to catch it in the same pass.

shared_field_names's :return: omits the new exclusion — app/sep/apps/atw/batch.py:339. The prose at 335-336 states it, but the :return: still reads "The names whose declarations are unanimous across two or more snippets", which is exactly the case the new short-circuit rules out for extra_args. Same shape as the parameter_fields :return: from last round.

Approving — both are one-line docstring edits, fine to fold into any later push.

[marcuscruz-percona]

@marcuscruz-percona — all seven threads check out at a235da0c. The re-export is gone and both consumers reach app.sep.snippets.models.constants directly, the framework constant-block docstring is byte-identical to main again, and the reserved-name validator now sits above normalize_visibility_condition so the field list runs uninterrupted from name through sensitive. Folding the exclusion into shared_field_names is behaviour-preserving as expected: both downstream reads of declarations in atw_execution_schema still filter on shared_names (lines 320-322 and 328), so the merged schema and the per-snippet split come out identical, dispatch_batch_item:381 still closes the shared_args path, and test_batch_api.py:603 covers it end-to-end. parameter_fields's docstring picked up the fourth item and the qualified :return: too.

Two docstring artifacts the refactor left behind, neither blocking:

Stale cycle rationale in constants.py — app/sep/snippets/models/constants.py:16,21-25. With the re-export gone, app/sep/apps/framework/schema.py has no reference to the constant at all, but the module docstring still describes the constants as "shared across snippet and framework schema modules" and the constant's own docstring opens with app.sep.apps.framework.schema and app.sep.snippets.models.snippet "both need it". The cycle argument itself still holds — framework imports snippet via script_helpers.py:55, so the models can't import back — but the consumer list is now app.sep.snippets.models.meta / .snippet, app.sep.snippets.schema, and app.sep.apps.atw.batch. Summary bullet 3 in the PR description has the same "both sides import" phrasing if you want to catch it in the same pass.

shared_field_names's :return: omits the new exclusion — app/sep/apps/atw/batch.py:339. The prose at 335-336 states it, but the :return: still reads "The names whose declarations are unanimous across two or more snippets", which is exactly the case the new short-circuit rules out for extra_args. Same shape as the parameter_fields :return: from last round.

Approving — both are one-line docstring edits, fine to fold into any later push.

@copilot fix the points raised by the review