feat: forward per-job OpenJD extensions to template parsing - #1049
feat: forward per-job OpenJD extensions to template parsing#1049seant-aws wants to merge 2 commits into
Conversation
| "name": "WrapEnv", | ||
| "script": { | ||
| "actions": { | ||
| "onWrapEnvEnter": {"command": "/bin/true"}, |
There was a problem hiding this comment.
These two wrap-action scenarios hardcode POSIX-only executables — /bin/true (lines 502-503, 601-602), /bin/sh (line 604) and /usr/bin/env (line 608) — but have no platform guard. hatch run integ-test runs pytest test/integ on windows-latest as well as ubuntu-latest in .github/workflows/code_quality.yml (IntegrationTests matrix), so these will fail on the Windows runners.
Other integ tests in this repo gate on platform explicitly (e.g. test/integ/startup/test_host_configuration.py:291 uses @pytest.mark.skipif(sys.platform != "win32", ...)). Suggest adding @pytest.mark.skipif(sys.platform == "win32", reason="POSIX-only shell utilities") to test_wrap_action_resolves_wrapped_step_name and test_wrap_action_reexecutes_with_repr_sh_quoting, or parameterizing the commands per-platform the way test/e2e/test_wrap_actions.py::_build_wrap_actions_template does.
| _ENVIRONMENT_TEMPLATE_PARAMETER_TYPES = frozenset(("STRING", "PATH", "INT", "FLOAT")) | ||
|
|
||
|
|
||
| def _to_environment_parameter_definitions(values: dict[str, Any]) -> list[dict[str, str]]: |
There was a problem hiding this comment.
_to_environment_parameter_definitions feeds service-supplied parameter names straight into a template that is then validated by the decoder. Previously those names were only used as lookup keys for value substitution and were never schema-validated, so any name the service returns that is not a legal OpenJD parameter identifier (OpenJD constrains parameter names to an identifier-like pattern) will now cause decode_environment_template to reject the whole template — failing enter_environment for every environment in that session, including ones that reference no parameters at all.
parameters_from_api_response (job_entities/job_details.py:40) does no name validation, and _validate_job_parameters (line 434) only validates the value shape, not the key. So a job whose jobDetails.parameters contains e.g. a colon- or dash-bearing key would go from "works" to "all environments fail to decode" on the Rust runtime.
Worth confirming what key set the service can actually return here. If names are not guaranteed to be valid OpenJD identifiers, the filter should also drop (or the call should tolerate) names that do not match the identifier pattern, the same defensive posture _ENVIRONMENT_TEMPLATE_PARAMETER_TYPES takes for types.
| ) | ||
| # The step name should appear somewhere in the session action outputs | ||
| actions_str = str(log_response) | ||
| assert self.STEP_NAME in actions_str, ( |
There was a problem hiding this comment.
This assertion does not test what the docstring claims. It stringifies the get_session_actions API response (action metadata), not session log output — and STEP_NAME ("WrapVerifyStep") is the step name in the submitted template, so if the response carries the step name anywhere in its action definitions the assertion passes whether or not the onWrapTaskRun hook ever ran or resolved {{WrappedStep.Name}}. The test would still pass with the wrap hook removed entirely.
To actually prove the hook resolved, echo a sentinel that appears nowhere else in the template (as test_wrap_action_reexecutes_with_repr_sh_quoting does with REEXEC_SENTINEL) and/or read the CloudWatch session log stream rather than the session-actions API response.
| if self._environment_parameter_definitions: | ||
| template["parameterDefinitions"] = self._environment_parameter_definitions | ||
| if self._supported_extensions: | ||
| template["extensions"] = list(self._supported_extensions) |
There was a problem hiding this comment.
The extension plumbing added here is only applied to the enter_environment decode path. run_task (line 379) still calls deserialize_step(...) with no extensions key in the wire dict and no supported_extensions argument, so a step script that uses extension-gated syntax (e.g. EXPR expressions in command/args) will still fail to deserialize on the Rust runtime for the same reason environments did before this change.
The new integ coverage does not catch this: test_wrap_action_resolves_wrapped_step_name puts the wrap hooks in the environment and passes a plain step script, so the step decode path never sees extension syntax.
Is that gap intentional (step scripts can never carry extension syntax), or should deserialize_step receive the same treatment? If the binding has no supported_extensions parameter, a comment noting the known limitation would help the next reader.
51543bf to
3172c17
Compare
| if self._environment_parameter_definitions: | ||
| template["parameterDefinitions"] = self._environment_parameter_definitions | ||
| if self._supported_extensions: | ||
| template["extensions"] = list(self._supported_extensions) |
There was a problem hiding this comment.
The production path through the new extensions key is never exercised against the real decoder.
Session always builds the config with supported_extensions=INTERIM_SUPPORTED_EXTENSIONS (every ExtensionName), so in production self._supported_extensions is non-empty and every enter_environment call now sends template["extensions"] = [...] plus a supported_extensions=[...] kwarg. But nothing validates that shape:
- The unit tests (
test_enter_environment_when_*) patchdecode_environment_template, so they only assert the payload dict Python built — not that the decoder accepts it. - The new integ test
test_enter_environment_when_job_has_typed_params_resolves_param_referenceomitssupported_extensionsfrom itsSessionRuntimeConfig, so it defaults to()and takes the other branch: noextensionskey,supported_extensions=None. Its own comment says it exists to "exercise the REAL decoder ... unit tests mock it and cannot validate payload shape" — but it validates only the empty-extensions shape.
So if environment-2023-09 does not accept a top-level extensions field, or rejects declaring an extension the environment does not actually use, enter_environment breaks for every environment on the Rust runtime and no test in this PR would fail. Adding supported_extensions=("EXPR",) to that integ config (or a second parametrization of it) would close the gap cheaply.
Relatedly: OpenJD extensions is normally a declaration of the extensions a template uses, not the set the host supports. Declaring all of them unconditionally is a different assertion than what the surrounding comment ("declaring which extensions the template uses") claims.
| # Interim value: allow all known extensions for simplicity. This should be replaced with the | ||
| # list of extensions actually requested for the job once that information is returned by | ||
| # BatchGetJobEntity. | ||
| INTERIM_SUPPORTED_EXTENSIONS: tuple[str, ...] = tuple(v.value for v in ExtensionName) |
There was a problem hiding this comment.
Enabling all extensions at parse_model time moves a failure that used to happen early and loudly into the middle of a session, at least on the Rust runtime.
Before this change, an environment/step template using extension-gated syntax was rejected by from_boto — a clean entity-level failure. Now it parses, and the resulting pydantic model can carry extension constructs (e.g. onWrapEnvEnter/onWrapTaskRun). RustSessionRuntime.__init__ then filters the same extension list through ModelExtension.from_str and drops names the Rust crate does not define — and the comment at rust.py:257 names WRAP_ACTIONS as exactly such a case.
If that filtering does drop an extension the template actually uses, the combination is: the Python layer accepts the template, ModelProfile does not enable the extension, template["extensions"] omits it, but environment.model_dump() still contains the wrap-action keys — so decode_environment_template fails per-environment inside enter_environment instead of at entity fetch. Same for deserialize_step.
Two things worth confirming:
- Is
ModelExtension.from_str("WRAP_ACTIONS")actually non-None with the pinnedopenjd-sessions == 0.10.14? If not, the newtest_from_boto_parses_wrap_actions_environmentcoverage documents a shape the Rust runtime cannot then run. parse_model(..., supported_extensions=...)anddecode_environment_template(..., supported_extensions=...)are new kwarg usages. The floor pin isopenjd-model >= 0.11.1; if the kwarg landed after 0.11.1, that floor needs bumping or the agent breaks at import/call time on a valid resolved version.
| model=Environment_2023_09, obj=environment_details_data["template"] | ||
| model=Environment_2023_09, | ||
| obj=environment_details_data["template"], | ||
| supported_extensions=resolve_supported_extensions(environment_details_data), |
There was a problem hiding this comment.
extensions is read here, but validate_entity_data will reject the response before from_boto ever runs.
validate_object (validation.py:42) is a strict allowlist — any key not in the fields tuple raises ValueError(f"Unexpected fields: ..."). EnvironmentDetails.validate_entity_data (line 86) still lists only template/environmentId/jobId/schemaVersion, and job_entities.py:405 always calls validate_entity_data immediately before from_boto.
So the moment the service actually starts returning extensions in an environmentDetails payload, entity fetch raises Unexpected fields: "extensions" and the environment fails — which is the exact opposite of the additive-optional contract _extensions.py documents ("absent from responses until the service deploys the feature ... workers shipped before the service change continue to behave identically"). Today the field is unreachable; after deployment it is fatal.
Same gap in the other two entities touched by this PR:
step_details.py:113—validate_entity_dataomitsextensions, soresolve_supported_extensions(step_details_data)at lines 72/80 can also never see it.job_details.py:283—JobDetailsDatagained anextensionskey inapi_models.py, but that validator has noextensionsfield either.
Each needs Field(key="extensions", expected_type=list, required=False) added. Note also that validate_object only type-checks the container, not the elements, so a non-str element would flow into tuple(extensions) and on into parse_model — worth an element check like the one step_details.py:123 already does for dependencies.
The existing tests do not catch this because they call from_boto directly with a hand-built dict, bypassing validation entirely.
| # This does not obey the spec. It should be changed at a later date to the list of requested | ||
| # extensions once those are returned by BatchGetJobEntity | ||
| supported_extensions=tuple(v.value for v in ExtensionName), | ||
| supported_extensions=INTERIM_SUPPORTED_EXTENSIONS, |
There was a problem hiding this comment.
This leaves the feature half-wired: the service-supplied list gates parsing but not execution.
api_models.py gained JobDetailsData.extensions, but nothing reads it — JobDetails.from_boto (job_details.py:230) does not call resolve_supported_extensions, JobDetails has no field for it, and this line still hardcodes INTERIM_SUPPORTED_EXTENSIONS (all known extensions).
So once the service starts returning extensions, the two halves disagree:
EnvironmentDetails.from_boto/StepDetails.from_botohonor the service list — a template using an extension the service did not list fails to parse.- The session runtime is still built with all extensions, and
RustSessionRuntimethen unconditionally sendstemplate["extensions"] = <all>plussupported_extensions=<all>on everyenter_environment(rust.py:330-335).
The failure mode that actually matters is the reverse direction: if the service lists an extension that INTERIM_SUPPORTED_EXTENSIONS does not contain (a newer extension than this worker build knows), the entity parses under the service list but the session runtime never enables it, so the template that just passed parsing fails at decode/run time. INTERIM_SUPPORTED_EXTENSIONS is derived from the locally-pinned ExtensionName enum, so it is a build-time constant that can lag the service.
If plumbing the resolved list through JobDetails into SessionRuntimeConfig.supported_extensions is out of scope for this PR, adding the unused extensions key to JobDetailsData invites the reader to think it is already respected — worth either wiring it or noting the TODO where the constant is still used.
| """ | ||
| extensions = entity_data.get("extensions") | ||
| if extensions is None: | ||
| return INTERIM_SUPPORTED_EXTENSIONS |
There was a problem hiding this comment.
tuple(extensions) forwards service-supplied strings verbatim into parse_model(supported_extensions=...), and the docstring makes that explicit ("use verbatim, no filtering"). That is the one place where being unfiltered is risky rather than safe.
INTERIM_SUPPORTED_EXTENSIONS is built from the locally-pinned ExtensionName enum, so this worker only knows the extensions its pinned openjd-model knows. The service, being deployed independently, can return a name that is newer than the pin. If parse_model validates supported_extensions against ExtensionName (rejecting or erroring on unknown members), then a single unrecognized name in the response makes from_boto raise for every environment and step of that job — a hard entity-fetch failure on a job that would have run fine before, and one that older workers cannot be patched out of after the fact.
test_unknown_extension_passes_through (test_extensions.py:44) asserts only that the helper returns ("EXPR", "FUTURE_UNKNOWN_EXT"); it never feeds that tuple to parse_model, so it confirms the pass-through happens without establishing that the pass-through is safe.
Worth confirming what parse_model does with an unknown extension name. If it is not tolerant, the safe posture for a forward-compatibility shim is to intersect with ExtensionName (dropping unknowns with a warning) — the same defensive filtering RustSessionRuntime.__init__ already applies via ModelExtension.from_str at rust.py:266, and the same reasoning as the _ENVIRONMENT_TEMPLATE_PARAMETER_TYPES allowlist comment ("an unrecognized future type can never introduce a new decode failure"). Note that dropping unknowns here is strictly better than failing, since an extension this worker cannot model is one it cannot execute either.
…emplates Templates using the WRAP_ACTIONS extension (onWrapEnvEnter, onWrapTaskRun, onWrapEnvExit) failed to parse. Three call sites parsed OpenJD templates without declaring which extensions the agent accepts: - EnvironmentDetails.from_boto parsed the environment template with no supported extensions, so a wrap environment was rejected outright. - StepDetails.from_boto did the same, which matters independently because a wrap environment also reaches the agent nested in a step template's stepEnvironments. - The Rust session runtime called decode_environment_template without either an "extensions" declaration in the template dict or a supported_extensions accept-list. Both are required; supplying only one still fails. The interim "accept all known extensions" value previously lived inline in Session.__init__. It now has a single home in sessions/_extensions.py so the eventual switch to the extensions actually requested for a job is a one-site change rather than several copies drifting apart. The Rust runtime keeps the extension names as strings alongside the enums it hands to the session profile, since the decode function takes strings. Only names that convert successfully are retained, so both lists describe the same set. Not changed: the all-extensions interim policy itself, deserialize_step, and plumbing configuration into JobEntities. Verified with a local job harness running a wrap-actions template on both the Python and Rust session runtimes with no workarounds applied, plus a non-wrap template on both to check for regressions. Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>
BatchGetJobEntity now includes an `extensions` field on environment and step entity payloads, listing the OpenJD extensions the job was submitted with. This list gates which extensions the template parser allows during validation. When the key is absent (old service, or legacy responses), the parse sites default to empty -- no extensions enabled for validation. This is safe because template validation only prevents *declaring* unsupported extensions; it does not affect runtime behavior. The session-scoped extensions list (RUNTIME_CAPABILITY_EXTENSIONS, passed at session construction) deliberately stays full -- it drives runtime behavior such as REDACTED_ENV_VARS env-var propagation via openjd/sessions/ _action_filter.py, which gates on extension presence in the session's RevisionExtensions. Omitting a supported extension from the session list would silently disable the runtime feature. Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>
f2949e0 to
5000312
Compare
| """ | ||
| extensions = entity_data.get("extensions") | ||
| if extensions is None: | ||
| return () |
There was a problem hiding this comment.
This revision makes the first commit in this same PR inert in production.
Commit 83641ee ("fix: forward supported extensions when parsing environment and step templates") existed because parse_model with no supported_extensions rejects extension-gated syntax, so a WRAP_ACTIONS / EXPR environment or step template failed at from_boto. It fixed that by forwarding the full known-extension list.
Commit 5000312 then changes the source of that list to resolve_supported_extensions(entity_data), whose documented behaviour is to return () when the key is absent — and the docstring two lines up states the key is absent today ("absent from responses until the service deploys the feature"). So the value actually reaching parse_model in production is (), which is the same as not passing it at all. The parse fix does not take effect until the service ships the field.
Two consequences worth confirming are intended:
- If the service is not yet returning
extensions, this PR ships no observable change to template parsing — the wrap-action support it enables is gated on an unshipped service change, with no feature flag or log line indicating why a WRAP_ACTIONS template is still being rejected. - Combined with the
validate_entity_dataallowlist gap (separate thread onenvironment_details.py), the field is currently unreachable rather than merely absent, so even a service that does return it produces()— actually a hardValueError— never the populated tuple.
The new unit tests do not surface this because they hand-build "extensions": ["EXPR", "WRAP_ACTIONS"] directly into the entity dict, which is a payload shape nothing in production can currently produce.
If the intent is "honour the service list when present, otherwise keep working as before," the absent-key default needs to be RUNTIME_CAPABILITY_EXTENSIONS, not () — that is both backwards-compatible and forward-compatible. If the intent really is (), the docstring claim that "the empty default is safe because this value only gates template validation" is the crux: gating template validation to no extensions is precisely what rejects the templates commit 83641ee set out to accept.
Minor, related: test_absent_extensions_falls_back and test_empty_extensions_does_not_fall_back (test_step_details.py:210, :229) are named for a fallback that no longer exists in this revision, which makes the intended contract harder to read.
|
|
||
| result = StepDetails.from_boto(cast(StepDetailsData, step_details_data)) | ||
|
|
||
| assert result.step_template.name == "TestStep" |
There was a problem hiding this comment.
All four tests in TestFromBotoExtensions pass vacuously — none of them can distinguish the extensions value that was forwarded.
Every one of them uses the same plain template (onRun with command/args, no extension-gated syntax), and each asserts only step_template.name. That template parses under supported_extensions=(), ("EXPR",), or anything else, so:
test_service_supplied_extensions_reach_parse_model(line 174) does not establish that["EXPR"]reachedparse_model. It would still pass if thesupported_extensions=argument were deleted fromstep_details.py:72entirely.test_empty_extensions_does_not_fall_back(line 219) andtest_absent_extensions_falls_back(line 197) assert the identical thing about the identical template, so they cannot show that the empty-list and absent-key branches differ — which is the one behavioural distinctionresolve_supported_extensionsexists to draw.test_service_supplied_extensions_reach_parse_model_legacy_shape(line 241) likewise only checks the"Placeholder"name, which comes from the wrapping code, not from extension handling.
The neighbouring test_from_boto_parses_wrap_actions_environment (line 121) is the one test here with real discriminating power, because onWrapEnvEnter/onWrapTaskRun only parse when WRAP_ACTIONS is enabled. Two cheap changes would give this class the same power:
- Positive: use extension-gated syntax (the wrap-action shape, or an EXPR expression) so the parse depends on the forwarded list.
- Negative: assert the mirror case — same wrap-action template with
"extensions": []or the key absent should raise, proving the empty default really does gate validation. Right now nothing in the suite asserts that a template is rejected when its extension is not listed, which is exactly the production behaviour this PR introduces.
Alternatively, mock.patch parse_model and assert on the supported_extensions kwarg — which is what the docstrings claim these tests do.
What was the problem/requirement? (What/Why)
Two problems, both about which OpenJD extensions a job is allowed to use.
The agent never told its template parsers which extensions to accept.
parse_modelgates extension-dependent syntax — wrap-action hooks (onWrapEnvEnter/onWrapTaskRun/onWrapEnvExit) and EXPR format strings — so any template using them was rejected at parse time, before any runtime code ran, on both session runtimes.Separately, the extension set was hardcoded to every name the model library knows, rather than the set the job actually requested.
What was the solution? (How)
Resolve the extension list per job entity from the service and forward it to every parse site.
BatchGetJobEntitycarries an optionalextensionslist onJobDetailsEntity,StepDetailsEntity, andEnvironmentDetailsEntity(a sibling oftemplate, not nested inside it). The agent now reads it and passes it toparse_modelin bothEnvironmentDetails.from_botoandStepDetails.from_boto, and to the Rust adapter'sdecode_environment_template.The field is additive-optional and absent until the service deploys it. Absent resolves to empty — no extension is enabled for template validation unless the service says the job uses it. Absent is distinguished from an explicitly empty list, so a job that requests nothing is not silently upgraded.
The list is passed through verbatim with no filtering against
ExtensionName: the service's contract is a pattern-validated string, not a closed enum, so it may legitimately send a name this agent's openjd version does not know. Unrecognized names insupported_extensionsare inert.One deliberate asymmetry: the session-scoped list (
RUNTIME_CAPABILITY_EXTENSIONS, feeding the PythonRevisionExtensionsand the RustModelProfile) stays at the full known set. That value is a runtime capability declaration, not a validation gate —openjd/sessions/_action_filter.pygatesopenjd_redacted_envenvironment-variable propagation on whetherREDACTED_ENV_VARSappears in the session'sRevisionExtensions, so emptying it would stop that variable reaching subsequent actions. Template validation is gated by the per-entity value; runtime behavior by the session value.What is the impact of this change?
Extension-gated templates parse successfully when the service reports the job uses those extensions. No behavior change today: the service currently rejects at
CreateJobany template declaring these extensions unless the account is enabled for the feature, so no such template can reach a worker yet.Because the field is additive-optional and absent means empty, no coordination between agent and service rollout is required, and nothing needs cleaning up when the service ships.
How was this change tested?
Unit tests cover the resolver (absent, present, explicitly empty, and an unrecognized name passing through), both
from_botoparse sites including the legacyStepScriptstep shape, and the Rust adapter's extension forwarding. Wrap-action templates are asserted to parse through the OpenJD model at both the environment and step level.Build, unit suite (3061 passed / 39 skipped), lint (ruff + format + mypy), and the integration suite all pass. Each commit passes standalone.
Wrap-action differential and end-to-end tests are deliberately held for a follow-up PR — they depend on this change, so they cannot be verified against
mainlineuntil it lands.Was this change documented?
No user-facing documentation change needed — this enables existing OpenJD extension semantics that an implementation gap was blocking.
Is this a breaking change?
No.