From 8813b51f306d1b4d1a27a2c300fbe25256fce3d5 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Sun, 2 Aug 2026 13:07:52 -0700 Subject: [PATCH 1/6] test: Pin declared-extension activation, Job.Name, Step.Name, and enter-failure cleanup Six behaviours the RFC 0007/0008 CLI work introduced were unpinned: reverting the template-declared-extensions fix entirely still passed all 307 tests. Each new test drives a real template through do_run and asserts on observed output. - Extension behaviours activate only when a template DECLARES the extension, never from the CLI's --extensions accept-list. A job template declaring no extensions must not get REDACTED_ENV_VARS even though --extensions defaults to every supported one. Kills: supported_extensions=extensions in _run_command (the pre-fix behaviour). - The declared set is the UNION over the job template and every external environment template. Only the environment template declares REDACTED_ENV_VARS, and redaction is active for the whole session. Kills: dropping the environment-template union loop. - Job.Name is seeded into the session symbol table, so a step-level EXPR `let` binding resolves it. Asserts the printed VALUE equals the job's name, so neither omitting job_name= from Session() nor passing an empty string satisfies it. - The step name reaches Session.run_task and feeds WrappedStep.Name inside an active onWrapTaskRun hook. Asserts the resolved value equals the step's real name. Kills: passing a wrong constant for step_name. - Step.Name is out of scope outside a step: a job environment referencing it is rejected. NOTE this is openjd-model static validation at template-read time, so it does NOT pin the CLI's non-seeding of step_name on job/external enters -- no loadable template can observe that, because the same rejection applies to every environment-template location including RFC 0008 wrap hooks. The docstring records the limit; the CLI's decision stays pinned only by test_localsession_step_env_enter_receives_step_name. - An enter that raises BEFORE the session registers the environment (two external wrap-hook environments hitting RFC 0008's "at most one") must drop it from the CLI's entered list, so the real error surfaces instead of a masking "Cannot exit unknown Environment". Kills: never popping on raise. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- .../templates/env_declares_redacted_env.yaml | 10 ++ .../templates/env_wrap_echoes_step_name.yaml | 17 +++ .../openjd/cli/templates/env_wrap_second.yaml | 17 +++ .../cli/templates/job_env_refs_step_name.yaml | 19 ++++ .../cli/templates/job_name_expr_job.yaml | 21 ++++ .../templates/no_extensions_shows_secret.yaml | 11 ++ .../templates/redacted_env_undeclared.yaml | 18 +++ .../cli/templates/wrapped_step_name_job.yaml | 11 ++ test/openjd/cli/test_redacted_env.py | 58 ++++++++++ test/openjd/cli/test_run_command.py | 106 ++++++++++++++++++ 10 files changed, 288 insertions(+) create mode 100644 test/openjd/cli/templates/env_declares_redacted_env.yaml create mode 100644 test/openjd/cli/templates/env_wrap_echoes_step_name.yaml create mode 100644 test/openjd/cli/templates/env_wrap_second.yaml create mode 100644 test/openjd/cli/templates/job_env_refs_step_name.yaml create mode 100644 test/openjd/cli/templates/job_name_expr_job.yaml create mode 100644 test/openjd/cli/templates/no_extensions_shows_secret.yaml create mode 100644 test/openjd/cli/templates/redacted_env_undeclared.yaml create mode 100644 test/openjd/cli/templates/wrapped_step_name_job.yaml diff --git a/test/openjd/cli/templates/env_declares_redacted_env.yaml b/test/openjd/cli/templates/env_declares_redacted_env.yaml new file mode 100644 index 0000000..e2775f6 --- /dev/null +++ b/test/openjd/cli/templates/env_declares_redacted_env.yaml @@ -0,0 +1,10 @@ +specificationVersion: environment-2023-09 +extensions: + - REDACTED_ENV_VARS +environment: + name: EnvDeclaresRedactedEnv + script: + actions: + onEnter: + command: python + args: ["-c", "print('openjd_redacted_env: SECRET=s3cret')"] diff --git a/test/openjd/cli/templates/env_wrap_echoes_step_name.yaml b/test/openjd/cli/templates/env_wrap_echoes_step_name.yaml new file mode 100644 index 0000000..24e1fd4 --- /dev/null +++ b/test/openjd/cli/templates/env_wrap_echoes_step_name.yaml @@ -0,0 +1,17 @@ +specificationVersion: environment-2023-09 +extensions: + - EXPR + - WRAP_ACTIONS +environment: + name: WrapEchoesStepName + script: + actions: + onWrapEnvEnter: + command: python + args: ["-c", "print('WrapEnvEnter')"] + onWrapTaskRun: + command: python + args: ["-c", "print('WrappedStepName={{ WrappedStep.Name }}')"] + onWrapEnvExit: + command: python + args: ["-c", "print('WrapEnvExit')"] diff --git a/test/openjd/cli/templates/env_wrap_second.yaml b/test/openjd/cli/templates/env_wrap_second.yaml new file mode 100644 index 0000000..be20744 --- /dev/null +++ b/test/openjd/cli/templates/env_wrap_second.yaml @@ -0,0 +1,17 @@ +specificationVersion: environment-2023-09 +extensions: + - EXPR + - WRAP_ACTIONS +environment: + name: WrapSecond + script: + actions: + onWrapEnvEnter: + command: python + args: ["-c", "print('WrapSecondEnvEnter')"] + onWrapTaskRun: + command: python + args: ["-c", "print('WrapSecondTaskRun')"] + onWrapEnvExit: + command: python + args: ["-c", "print('WrapSecondEnvExit')"] diff --git a/test/openjd/cli/templates/job_env_refs_step_name.yaml b/test/openjd/cli/templates/job_env_refs_step_name.yaml new file mode 100644 index 0000000..90a438f --- /dev/null +++ b/test/openjd/cli/templates/job_env_refs_step_name.yaml @@ -0,0 +1,19 @@ +specificationVersion: "jobtemplate-2023-09" +extensions: + - EXPR +name: JobEnvRefsStepName + +jobEnvironments: + - name: JobEnvRefsStep + script: + actions: + onEnter: + command: python + args: ["-c", "print('JobEnvSawStepName={{ Step.Name }}')"] +steps: + - name: TheStep + script: + actions: + onRun: + command: python + args: ["-c", "print('TaskRan')"] diff --git a/test/openjd/cli/templates/job_name_expr_job.yaml b/test/openjd/cli/templates/job_name_expr_job.yaml new file mode 100644 index 0000000..31336a9 --- /dev/null +++ b/test/openjd/cli/templates/job_name_expr_job.yaml @@ -0,0 +1,21 @@ +specificationVersion: "jobtemplate-2023-09" +extensions: + - EXPR +name: JobNameExprJob + +steps: + - name: EchoJobName + let: + - bound_job_name = Job.Name + stepEnvironments: + - name: JobNameEcho + script: + actions: + onEnter: + command: python + args: ["-c", "print('EnvSawJobName={{ bound_job_name }}')"] + script: + actions: + onRun: + command: python + args: ["-c", "print('TaskRan')"] diff --git a/test/openjd/cli/templates/no_extensions_shows_secret.yaml b/test/openjd/cli/templates/no_extensions_shows_secret.yaml new file mode 100644 index 0000000..a8b81b7 --- /dev/null +++ b/test/openjd/cli/templates/no_extensions_shows_secret.yaml @@ -0,0 +1,11 @@ +specificationVersion: "jobtemplate-2023-09" +name: No Extensions Shows Secret +description: Declares no extensions; prints whether SECRET reached the task + +steps: + - name: ShowSecret + script: + actions: + onRun: + command: python + args: ["-c", "import os; print('SECRET_IS=' + str(os.environ.get('SECRET')))"] diff --git a/test/openjd/cli/templates/redacted_env_undeclared.yaml b/test/openjd/cli/templates/redacted_env_undeclared.yaml new file mode 100644 index 0000000..0943511 --- /dev/null +++ b/test/openjd/cli/templates/redacted_env_undeclared.yaml @@ -0,0 +1,18 @@ +specificationVersion: "jobtemplate-2023-09" +name: Redacted Env Undeclared +description: Emits openjd_redacted_env without declaring the REDACTED_ENV_VARS extension + +jobEnvironments: + - name: EmitRedactedEnv + script: + actions: + onEnter: + command: python + args: ["-c", "print('openjd_redacted_env: SECRET=s3cret')"] +steps: + - name: ShowSecret + script: + actions: + onRun: + command: python + args: ["-c", "import os; print('SECRET_IS=' + str(os.environ.get('SECRET')))"] diff --git a/test/openjd/cli/templates/wrapped_step_name_job.yaml b/test/openjd/cli/templates/wrapped_step_name_job.yaml new file mode 100644 index 0000000..ccc7cb9 --- /dev/null +++ b/test/openjd/cli/templates/wrapped_step_name_job.yaml @@ -0,0 +1,11 @@ +specificationVersion: "jobtemplate-2023-09" +name: WrappedStepNameJob +description: A single step whose onRun is replaced by an active onWrapTaskRun hook + +steps: + - name: WrappedTaskStep + script: + actions: + onRun: + command: python + args: ["-c", "print('TaskRanUnwrapped')"] diff --git a/test/openjd/cli/test_redacted_env.py b/test/openjd/cli/test_redacted_env.py index 2affa5c..e42847b 100644 --- a/test/openjd/cli/test_redacted_env.py +++ b/test/openjd/cli/test_redacted_env.py @@ -43,3 +43,61 @@ def test_run_job_with_redacted_env(capsys): assert ( unexpected_message not in outerr.out ), f"Found unexpected line in output:\n{format_capsys_outerr(outerr)}" + + +def test_run_job_redacted_env_not_enabled_by_accept_list(capsys): + """ + Extension behaviors activate only when a template *declares* the extension. + The CLI's --extensions list is what the CLI accepts, not what it enables. + + This job template declares no extensions, so REDACTED_ENV_VARS must stay + off even though --extensions defaults to every supported extension: the + variable does not reach the task, and the session warns that the extension + is not enabled. Building the session's RevisionExtensions from the + accept-list instead of the declared list flips both observations. + """ + outerr = run_openjd_cli_main( + capsys, + args=[ + "run", + str(TEMPLATE_DIR / "redacted_env_undeclared.yaml"), + ], + expected_exit_code=0, + ) + + assert ( + "SECRET_IS=None" in outerr.out + ), f"openjd_redacted_env set SECRET despite the extension not being declared:\n{format_capsys_outerr(outerr)}" + assert ( + "REDACTED_ENV_VARS extension is not enabled" in outerr.out + ), f"Expected the not-enabled warning:\n{format_capsys_outerr(outerr)}" + + +def test_run_job_redacted_env_declared_by_environment_template(capsys): + """ + The enabled extensions are the union of what the job template and every + external environment template declare. Here only the environment template + declares REDACTED_ENV_VARS, so redaction is active for the whole session: + SECRET reaches the task and its value is masked in the logs. Dropping the + environment templates from the union leaves the extension off. + """ + outerr = run_openjd_cli_main( + capsys, + args=[ + "run", + str(TEMPLATE_DIR / "no_extensions_shows_secret.yaml"), + "--environment", + str(TEMPLATE_DIR / "env_declares_redacted_env.yaml"), + ], + expected_exit_code=0, + ) + + assert ( + "SECRET_IS=" + "*" * 8 in outerr.out + ), f"SECRET was not set-and-redacted for the task:\n{format_capsys_outerr(outerr)}" + assert ( + "REDACTED_ENV_VARS extension is not enabled" not in outerr.out + ), f"The extension declared by the environment template was not enabled:\n{format_capsys_outerr(outerr)}" + assert ( + "s3cret" not in outerr.out + ), f"The redacted value leaked into the output:\n{format_capsys_outerr(outerr)}" diff --git a/test/openjd/cli/test_run_command.py b/test/openjd/cli/test_run_command.py index 5550c15..ad28f46 100644 --- a/test/openjd/cli/test_run_command.py +++ b/test/openjd/cli/test_run_command.py @@ -699,6 +699,112 @@ def test_do_run_step_name_in_step_environment(capsys: pytest.CaptureFixture) -> assert "TaskRan" in outerr.out +def test_do_run_job_name_in_step_let_binding(capsys: pytest.CaptureFixture) -> None: + """ + RFC 0007 §7.3.1 (EXPR): Job.Name is seeded into the session symbol table, + so a step-level `let` binding may reference it and the step's environments + resolve the binding. The assertion is on the value, so neither omitting + `job_name` from the Session (Job.Name undefined, the binding fails) nor + passing an empty one satisfies it. + """ + template_dir = Path(__file__).parent / "templates" + args = [ + "run", + str(template_dir / "job_name_expr_job.yaml"), + "--step", + "EchoJobName", + ] + outerr = run_openjd_cli_main(capsys, args=args, expected_exit_code=0) + assert ( + "EnvSawJobName=JobNameExprJob" in outerr.out + ), f"Job.Name did not resolve to the job's name:\n{format_capsys_outerr(outerr)}" + assert "TaskRan" in outerr.out + + +def test_do_run_wrapped_step_name_is_the_running_step(capsys: pytest.CaptureFixture) -> None: + """ + RFC 0008: the step name the CLI passes to Session.run_task feeds + WrappedStep.Name inside an active onWrapTaskRun hook. The hook echoes it, + so the assertion pins the resolved *value* against the step's real name -- + a constant or otherwise wrong step name fails here. + """ + template_dir = Path(__file__).parent / "templates" + args = [ + "run", + str(template_dir / "wrapped_step_name_job.yaml"), + "--environment", + str(template_dir / "env_wrap_echoes_step_name.yaml"), + ] + outerr = run_openjd_cli_main(capsys, args=args, expected_exit_code=0) + assert ( + "WrappedStepName=WrappedTaskStep" in outerr.out + ), f"WrappedStep.Name did not resolve to the running step's name:\n{format_capsys_outerr(outerr)}" + # The hook runs *instead of* the wrapped onRun. + assert "TaskRanUnwrapped" not in outerr.out + + +def test_do_run_step_name_undefined_outside_a_step(capsys: pytest.CaptureFixture) -> None: + """ + Step.Name exists only within a step's own scope: a job environment that + references it is rejected outright, with the model naming Job.Name as the + symbol that *is* in scope there. + + Scope of what this pins: the rejection is openjd-model *static* validation + at template-read time, so it fires before a Session is ever constructed. + That makes it a genuine end-to-end guarantee that Step.Name cannot be used + outside a step -- but it does NOT pin the CLI's own decision to leave + `step_name` off job/external environment enters, because the template never + reaches that code. The same rejection applies to every environment-template + location (onEnter, onExit, and RFC 0008 wrap hooks), so no loadable template + can observe whether the CLI seeded Step.Name there. That decision is pinned + only by test_localsession_step_env_enter_receives_step_name. + """ + template_dir = Path(__file__).parent / "templates" + args = [ + "run", + str(template_dir / "job_env_refs_step_name.yaml"), + ] + outerr = run_openjd_cli_main(capsys, args=args, expected_exit_code=1) + assert ( + "Variable Step.Name does not exist at this location" in outerr.out + ), f"Expected Step.Name to be out of scope for a job environment:\n{format_capsys_outerr(outerr)}" + assert "JobEnvSawStepName=" not in outerr.out + + +def test_do_run_enter_failure_before_registration_surfaces_real_error( + capsys: pytest.CaptureFixture, +) -> None: + """ + Two external environment templates both defining wrap hooks make + Session.enter_environment reject the second one up front, per RFC 0008, + *before* the session registers it. The CLI must drop that environment from + its own entered list so cleanup does not try to exit it -- otherwise the + session's "Cannot exit unknown Environment" RuntimeError escapes cleanup + and replaces the real error in the result. + """ + template_dir = Path(__file__).parent / "templates" + args = [ + "run", + str(template_dir / "wrapped_step_name_job.yaml"), + "--environment", + str(template_dir / "env_wrap_echoes_step_name.yaml"), + "--environment", + str(template_dir / "env_wrap_second.yaml"), + ] + outerr = run_openjd_cli_main(capsys, args=args, expected_exit_code=1) + + assert ( + "at most one Environment defining wrap hooks" in outerr.out + ), f"The RFC 0008 rejection is not the surfaced error:\n{format_capsys_outerr(outerr)}" + assert ( + "Cannot exit unknown Environment" not in outerr.out + ), f"Cleanup tried to exit the unregistered environment:\n{format_capsys_outerr(outerr)}" + # The unregistered environment is never exited, so the result keeps the + # default message rather than one built from a cleanup-time RuntimeError. + assert "Session ended with errors; see Task logs for details" in outerr.out + assert "WrapSecondEnvExit" not in outerr.out + + class TestProcessTaskParams: """Testing that we properly handle the values of the --task-param/-tp command-line argument""" From 58bc66b54a237f0be81d61001c394121302d6299 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Sun, 2 Aug 2026 13:35:17 -0700 Subject: [PATCH 2/6] docs: Correct three claims in the RFC 0007/0008 threading comments The enter-failure comment named two triggers that cannot reach it: at the pinned openjd-sessions floor (0.10.11) enter_environment has no `raise` after it registers the environment -- every failure it detects past that point routes through _fail_action_before_start() and returns normally (verified by AST-walking the function: raises at 780/782/793, append at 812, none after). The post-registration branch is therefore defensive, and now says so rather than implying a reachable path. The dependency-floor comment named run_task as what breaks below 0.10.11, but `job_name` is passed to Session() unconditionally, so construction raises TypeError first; `extra_let_bindings` is also 0.10.11-only. The forwarding comment explained that None is not passed without saying that pthat pthat pthat pteqthat pthat pthat pthat pteqthat pthat pthat pthat pteqthat pthat pthat pthat pteqthat pthat pthat pthat pteqthat pthat pthaion pins it. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- pyproject.toml | 8 ++++--- .../cli/_run/_local_session/_actions.py | 11 +++++++++ .../_run/_local_session/_session_manager.py | 23 ++++++++++++------- 3 files changed, 31 insertions(+), 11 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 1d0ded5..b444ce0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -29,9 +29,11 @@ classifiers = [ "Intended Audience :: End Users/Desktop" ] dependencies = [ - # 0.10.11 is the first release with `step_name` on both Session.run_task and - # Session.enter_environment (RFC 0007/0008). Below it, run_task() raises - # TypeError on every task run, since this CLI passes the keyword unconditionally. + # 0.10.11 is the first release carrying all three keywords this CLI now passes + # (RFC 0007/0008): `job_name` on Session, and `step_name` on both + # Session.run_task and Session.enter_environment (which also gained + # `extra_let_bindings`). Below this floor Session() raises TypeError at + # construction, before any task runs, since `job_name` is passed unconditionally. "openjd-sessions >= 0.10.11,< 0.11", "openjd-model >= 0.9,< 0.12" ] diff --git a/src/openjd/cli/_run/_local_session/_actions.py b/src/openjd/cli/_run/_local_session/_actions.py index 9e46538..be49bec 100644 --- a/src/openjd/cli/_run/_local_session/_actions.py +++ b/src/openjd/cli/_run/_local_session/_actions.py @@ -93,6 +93,17 @@ def run(self): # bindings means "no extra bindings", and job/external environment # enters have no owning step, so `Step.Name` must stay undefined for # them rather than being seeded with None. + # + # Passing None explicitly would be equivalent today, since + # enter_environment itself skips a None `step_name` and a falsy + # `extra_let_bindings`. Omitting them keeps this call site's intent + # ("this enter has no owning step") legible at the boundary rather + # than relying on the callee's None-handling, and it is what + # test_localsession_step_env_enter_receives_step_name asserts — that + # assertion is currently the only check that job and external enters + # do not seed Step.Name, because a template referencing Step.Name + # outside a step is rejected by static validation before any session + # is built, so no loadable template can observe it. optional_kwargs: dict[str, Any] = {} if self._extra_let_bindings: optional_kwargs["extra_let_bindings"] = self._extra_let_bindings diff --git a/src/openjd/cli/_run/_local_session/_session_manager.py b/src/openjd/cli/_run/_local_session/_session_manager.py index 3aeb1d0..602ac95 100644 --- a/src/openjd/cli/_run/_local_session/_session_manager.py +++ b/src/openjd/cli/_run/_local_session/_session_manager.py @@ -281,14 +281,21 @@ def run_environment_enters( except (RuntimeError, ValueError) as exc: # Session.enter_environment raises (rather than reporting # through the action-status callback) when it rejects the - # environment up front — e.g. the RFC 0008 "at most one wrap - # environment" RuntimeError, or a ValueError from the extra - # `let` bindings — but it can also raise *after* registering - # the environment (e.g. an environment `variables` expression - # that fails to evaluate). Only when the session did NOT - # register the environment may we drop it from our entered - # list (cleanup must not try to exit it). If the session did - # register it, it must stay in our list so cleanup exits it — + # environment up front, before registering it — the RFC 0008 + # "at most one Environment defining wrap hooks" RuntimeError is + # the trigger that reaches us in practice. Every failure it + # detects *after* registering goes through + # _fail_action_before_start() and returns normally instead, so + # as of openjd-sessions 0.10.11 nothing raises post-registration + # and the else-branch below is defensive. It is kept because the + # cost of being wrong is asymmetric: if a future release does + # raise after registering, dropping the environment from our + # list would skip its onExit and desynchronize us from the + # session's LIFO exit-ordering check, masking the original error + # with "Must exit Environment X first". So: only when the session + # did NOT register the environment may we drop it from our + # entered list (cleanup must not try to exit it). If the session + # did register it, it must stay in our list so cleanup exits it — # popping it here would skip its onExit and desynchronize us # from the session's LIFO exit ordering check, masking the # original error with "Must exit Environment X first". From eff2abe4743f4a9aa2d1bb112ff8d99b6d0c3f95 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Sun, 2 Aug 2026 13:35:30 -0700 Subject: [PATCH 3/6] test: Widen the timing margins on two timeout fixtures Both tests asserted on a race against a tight action timeout and failed intermittently once this branch added 12 tests to the 12-worker parallel run: job_sleep_exit_normal left under 2s for interpreter startup before the timeout killed the action, so 'SLEEP' was never printed; and feature_bundle_1_timeout gave a bash action that sleeps 1s a 5s budget, which a loaded run exceeded, turning exit 0 into exit 1. Measured before: the full suite failed 2 of 3 runs on this branch while the base commit passed 3 of 3 (301 tests vs 313 -- the added contention, not a behaviour change; both tests pass 5/5 in isolation on both trees). After: 3 of 3 runs green. Neither test's subject changes. TaskTimeout still times out, because the sleep is now an order of magnitude longer than the timesleep is now an order of magnitude longer than the timesleep is now an order of magnitude longer than the timesleep is now an order of magnitude longer thanit asserts moves, 5s to 20s. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- .../cli/templates/feature_bundle_1_timeout.yaml | 6 +++++- .../cli/templates/job_sleep_exit_normal.yaml | 16 ++++++++++++++-- test/openjd/cli/test_feature_bundle_1.py | 2 +- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/test/openjd/cli/templates/feature_bundle_1_timeout.yaml b/test/openjd/cli/templates/feature_bundle_1_timeout.yaml index 3e21a31..6a7d461 100644 --- a/test/openjd/cli/templates/feature_bundle_1_timeout.yaml +++ b/test/openjd/cli/templates/feature_bundle_1_timeout.yaml @@ -5,7 +5,11 @@ extensions: parameterDefinitions: - name: Timeout type: INT - default: 5 + # Generous relative to the ~1s the action needs: the point of the test is + # that the format string resolves into `timeout`, not that the timeout + # fires. A tight value made the action race its own budget under a loaded + # parallel test run and exit non-zero. + default: 20 steps: - name: TimeoutStep script: diff --git a/test/openjd/cli/templates/job_sleep_exit_normal.yaml b/test/openjd/cli/templates/job_sleep_exit_normal.yaml index f0f5e2f..9a96a19 100644 --- a/test/openjd/cli/templates/job_sleep_exit_normal.yaml +++ b/test/openjd/cli/templates/job_sleep_exit_normal.yaml @@ -12,9 +12,21 @@ "args": [ "-c", # Obfuscate "EXIT_NORMAL" so it doesn't appear in the log when Windows prints the command that's run to the log. - "import time,sys; print('SLEEP'); sys.stdout.flush(); time.sleep(5); print(chr(69)+'XIT_NORMAL')", + "import time,sys; print('SLEEP'); sys.stdout.flush(); time.sleep(60); print(chr(69)+'XIT_NORMAL')", ], - "timeout": 2, + # The margins are deliberately wide, in both + # directions, because this test asserts on a + # race: SLEEP must be printed *before* the + # timeout fires, and EXIT_NORMAL must not be. + # A 2s timeout left under 2s for interpreter + # startup, which a loaded parallel test run + # can exceed -- the action was then killed + # before printing SLEEP and the test failed + # intermittently. 5s of startup headroom, and + # a sleep an order of magnitude longer than + # the timeout, make both directions robust + # without changing what is verified. + "timeout": 5, } } }, diff --git a/test/openjd/cli/test_feature_bundle_1.py b/test/openjd/cli/test_feature_bundle_1.py index 09079a6..94f29ec 100644 --- a/test/openjd/cli/test_feature_bundle_1.py +++ b/test/openjd/cli/test_feature_bundle_1.py @@ -45,7 +45,7 @@ def test_format_string_timeout(self, capsys) -> None: """Test that format string timeout is resolved.""" template = TEMPLATES_DIR / "feature_bundle_1_timeout.yaml" outerr = run_openjd_cli_main(capsys, args=["run", str(template)], expected_exit_code=0) - assert "Running with timeout 5s" in outerr.out + assert "Running with timeout 20s" in outerr.out def test_format_string_amount_minmax(self, capsys) -> None: """Test that format string min/max in AmountRequirement is resolved.""" From 83713273dccc3bddda56dff44052b848418f4025 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Sat, 29 Aug 2026 01:00:52 -0700 Subject: [PATCH 4/6] fix: Forward the create-time step symbol table to sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A step's template-scope `let` (RFC 0005 §3.6) is resolved once at job creation. openjd-model no longer merges the resolved bindings into `script.let` and openjd-sessions no longer re-evaluates the source expressions, so the only channel those values have into a session is the `resolved_symtab` argument. The CLI forwarded nothing, so a step-level `let` produced no bindings at all. It also still passed `extra_let_bindings`, which openjd-sessions removed in 0.12.0 in favour of `resolved_symtab`. That raised TypeError for any step with a `let`, which surfaced as "Cannot exit unknown Environment" once the failed enter left the environment unregistered. Take the tables from `create_job_with_symbol_tables` (same job as `create_job`, tables returned instead of discarded), carry them on LocalSession keyed by step name, and forward the right one at all three entry points: `run_task`, `enter_environment` for a step's environments, and `exit_environment` with the same table the enter used. The values are already `SerializedSymbolTable`, which is what the sessions API takes, so nothing is converted. `extra_let_bindings` is removed rather than kept beside the new channel. Raise both floors, since neither range contained a release with the API: openjd-sessions to >= 0.12.0 (first release with `resolved_symtab` on all three entry points; 0.10.11 through 0.11.0 have `extra_let_bindings` and no `resolved_symtab`, so no version satisfies both) and openjd-model to >= 0.11.4 (first release exporting `create_job_with_symbol_tables`). Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- pyproject.toml | 19 +- src/openjd/cli/_common/__init__.py | 11 +- src/openjd/cli/_common/_job_from_template.py | 30 ++- .../cli/_run/_local_session/_actions.py | 85 +++++-- .../_run/_local_session/_session_manager.py | 65 ++++-- src/openjd/cli/_run/_run_command.py | 13 +- src/openjd/cli/_summary/_summary_command.py | 2 +- test/openjd/cli/conftest.py | 20 +- .../cli/templates/step_let_symtab_job.yaml | 39 ++++ test/openjd/cli/test_common.py | 2 +- test/openjd/cli/test_local_session.py | 4 +- test/openjd/cli/test_step_symbol_tables.py | 215 ++++++++++++++++++ 12 files changed, 438 insertions(+), 67 deletions(-) create mode 100644 test/openjd/cli/templates/step_let_symtab_job.yaml create mode 100644 test/openjd/cli/test_step_symbol_tables.py diff --git a/pyproject.toml b/pyproject.toml index b444ce0..bb88b5e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -29,13 +29,18 @@ classifiers = [ "Intended Audience :: End Users/Desktop" ] dependencies = [ - # 0.10.11 is the first release carrying all three keywords this CLI now passes - # (RFC 0007/0008): `job_name` on Session, and `step_name` on both - # Session.run_task and Session.enter_environment (which also gained - # `extra_let_bindings`). Below this floor Session() raises TypeError at - # construction, before any task runs, since `job_name` is passed unconditionally. - "openjd-sessions >= 0.10.11,< 0.11", - "openjd-model >= 0.9,< 0.12" + # 0.12.0 is the first release carrying `resolved_symtab` on all three of + # Session.enter_environment, Session.exit_environment and Session.run_task. + # It is also the release that removed `extra_let_bindings` from + # enter_environment: 0.10.11 through 0.11.0 have that keyword and no + # `resolved_symtab`, so there is no version satisfying both channels and the + # floor has to move rather than being feature-detected. Below 0.12.0 every + # call site here raises TypeError on the unexpected `resolved_symtab`. + "openjd-sessions >= 0.12.0,< 0.13", + # 0.11.4 is the first release exporting `create_job_with_symbol_tables` and + # `JobWithSymbolTables`. 0.11.3 and below have `create_job` only, so the + # step-scope resolved symbol tables cannot be obtained at all. + "openjd-model >= 0.11.4,< 0.12" ] [project.urls] diff --git a/src/openjd/cli/_common/__init__.py b/src/openjd/cli/_common/__init__.py index be487bd..93cdf3c 100644 --- a/src/openjd/cli/_common/__init__.py +++ b/src/openjd/cli/_common/__init__.py @@ -4,7 +4,7 @@ from dataclasses import asdict, dataclass from enum import Enum from pathlib import Path -from typing import Callable, Literal +from typing import TYPE_CHECKING, Callable, Literal import json import yaml import os @@ -23,6 +23,10 @@ ) from openjd.model import DecodeValidationError, Job, JobParameterValues, EnvironmentTemplate +if TYPE_CHECKING: + # Annotations only; see the note in _job_from_template.py. + from openjd.expr import SerializedSymbolTable + __all__ = [ "add_extensions_argument", "get_doc_type", @@ -118,7 +122,10 @@ def generate_job( environments: list[EnvironmentTemplate] = [], *, supported_extensions: list[str], -) -> tuple[Job, JobParameterValues]: +) -> tuple[Job, JobParameterValues, dict[str, "SerializedSymbolTable"]]: + """Returns the job, its parameter values, and the per-step resolved symbol + tables keyed by step name. See :func:`job_from_template` for why the tables + must be carried alongside the job rather than discarded.""" try: # Raises: RuntimeError, DecodeValidationError template = read_job_template(args.path, supported_extensions=supported_extensions) diff --git a/src/openjd/cli/_common/_job_from_template.py b/src/openjd/cli/_common/_job_from_template.py index 11727d4..7c13dbb 100644 --- a/src/openjd/cli/_common/_job_from_template.py +++ b/src/openjd/cli/_common/_job_from_template.py @@ -4,7 +4,7 @@ import json from pathlib import Path import re -from typing import Optional, Union +from typing import TYPE_CHECKING, Optional, Union import yaml from ._validation_utils import get_doc_type @@ -15,10 +15,17 @@ Job, JobParameterValues, JobTemplate, - create_job, + create_job_with_symbol_tables, preprocess_job_parameters, ) +if TYPE_CHECKING: + # Only for annotations: `openjd.expr` is a facade over the native + # extension, and importing the CLI must not load it. The tables the model + # hands back are already instances of this type, so nothing here + # constructs one. + from openjd.expr import SerializedSymbolTable + def get_params_from_file(parameter_string: str) -> Union[dict, list]: """ @@ -111,10 +118,18 @@ def job_from_template( parameter_args: list[str] | None, job_template_dir: Path, current_working_dir: Path, -) -> tuple[Job, JobParameterValues]: +) -> tuple[Job, JobParameterValues, dict[str, "SerializedSymbolTable"]]: """ Given a decoded Job Template and a user-input parameter dictionary, - generates a Job object and the parameter values for running the job. + generates a Job object, the parameter values for running the job, and the + per-step resolved symbol tables, keyed by step name. + + A step's template-scope `let` bindings (RFC 0005 §3.6) are evaluated once + here, at job creation, and their resolved values reach a session only + through those tables -- neither the model nor the session re-derives them + from the source expressions. Sessions for a step must therefore be given + `step_symbol_tables[step.name]`, or the step's `let` produces no bindings + at all. Raises: RuntimeError if parameters are an unsupported type or don't correspond to the template """ @@ -132,11 +147,14 @@ def job_from_template( raise RuntimeError(str(ve)) try: - job = create_job( + # `create_job_with_symbol_tables` returns the same Job that + # `create_job` does; it additionally returns the symbol tables that + # instantiation built instead of discarding them. + created = create_job_with_symbol_tables( job_template=template, job_parameter_values=parameter_values, environment_templates=environments, ) - return (job, parameter_values) + return (created.job, parameter_values, created.step_symbol_tables) except DecodeValidationError as dve: raise RuntimeError(f"Could not generate Job from template and parameters: {str(dve)}") diff --git a/src/openjd/cli/_run/_local_session/_actions.py b/src/openjd/cli/_run/_local_session/_actions.py index be49bec..483038c 100644 --- a/src/openjd/cli/_run/_local_session/_actions.py +++ b/src/openjd/cli/_run/_local_session/_actions.py @@ -1,12 +1,18 @@ # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. from enum import Enum -from typing import Any, Optional +from typing import TYPE_CHECKING, Optional from openjd.model import Step, TaskParameterSet from openjd.model.v2023_09 import Environment from openjd.sessions import Session +if TYPE_CHECKING: + # Annotations only: `openjd.expr` is a facade over the native extension and + # importing the CLI must not load it. These tables are produced by + # `create_job_with_symbol_tables` and forwarded unchanged. + from openjd.expr import SerializedSymbolTable + class EnvironmentType(str, Enum): """ @@ -40,11 +46,24 @@ def run(self): class RunTaskAction(SessionAction): _step: Step _parameters: TaskParameterSet + _resolved_symtab: Optional["SerializedSymbolTable"] - def __init__(self, session: Session, step: Step, parameters: TaskParameterSet): + def __init__( + self, + session: Session, + step: Step, + parameters: TaskParameterSet, + resolved_symtab: Optional["SerializedSymbolTable"] = None, + ): super(RunTaskAction, self).__init__(session) self._step = step self._parameters = parameters + # The step's create-time resolved symbol table + # (`create_job_with_symbol_tables().step_symbol_tables[step.name]`). + # It is the only channel for the step's template-scope `let` values: + # the model does not merge them into `script.let`, and the session does + # not re-evaluate the source expressions. + self._resolved_symtab = resolved_symtab def run(self): self._session.run_task( @@ -53,6 +72,7 @@ def run(self): # RFC 0008: the step name feeds the WrappedStep.Name template # variable inside an active onWrapTaskRun hook. step_name=self._step.name, + resolved_symtab=self._resolved_symtab, ) def __str__(self): @@ -63,7 +83,7 @@ def __str__(self): class EnterEnvironmentAction(SessionAction): _environment: Environment _id: str - _extra_let_bindings: Optional[list[str]] + _resolved_symtab: Optional["SerializedSymbolTable"] _step_name: Optional[str] def __init__( @@ -71,15 +91,18 @@ def __init__( session: Session, environment: Environment, env_id: str, - extra_let_bindings: Optional[list[str]] = None, + resolved_symtab: Optional["SerializedSymbolTable"] = None, step_name: Optional[str] = None, ): super(EnterEnvironmentAction, self).__init__(session) self._environment = environment self._id = env_id - # RFC 0007: a step's environments are entered with the step-level - # `let` bindings so their variables/actions can reference them. - self._extra_let_bindings = extra_let_bindings + # RFC 0005 §3.6: a step's environments are entered with the step's + # create-time resolved symbol table, which carries the step's + # template-scope `let` values so their variables/actions can reference + # them. Only step-environment enters have one; job and external + # environment enters leave it None. + self._resolved_symtab = resolved_symtab # RFC 0007 §7.3.1 (EXPR): the owning step's name seeds Step.Name for # a step environment's `let` bindings, variables, and actions. Only # step-environment enters carry a step name; job/external enters @@ -87,31 +110,26 @@ def __init__( self._step_name = step_name def run(self): - # Both keywords are guaranteed by this package's `openjd-sessions` - # floor (>= 0.10.11), so neither is feature-detected. They are still - # only forwarded when they carry something: a step with no `let` - # bindings means "no extra bindings", and job/external environment - # enters have no owning step, so `Step.Name` must stay undefined for - # them rather than being seeded with None. - # - # Passing None explicitly would be equivalent today, since - # enter_environment itself skips a None `step_name` and a falsy - # `extra_let_bindings`. Omitting them keeps this call site's intent - # ("this enter has no owning step") legible at the boundary rather - # than relying on the callee's None-handling, and it is what + # `step_name` is omitted rather than passed as None when this enter has + # no owning step. Passing None would be equivalent today, since + # enter_environment skips a None `step_name`, but omitting it keeps the + # call site's intent legible at the boundary and it is what # test_localsession_step_env_enter_receives_step_name asserts — that # assertion is currently the only check that job and external enters # do not seed Step.Name, because a template referencing Step.Name # outside a step is rejected by static validation before any session # is built, so no loadable template can observe it. - optional_kwargs: dict[str, Any] = {} - if self._extra_let_bindings: - optional_kwargs["extra_let_bindings"] = self._extra_let_bindings + # + # `resolved_symtab` is passed unconditionally: None is its documented + # "no table" value and there is no analogous assertion keyed on its + # absence. + optional_kwargs: dict[str, str] = {} if self._step_name is not None: optional_kwargs["step_name"] = self._step_name self._session.enter_environment( environment=self._environment, identifier=self._id, + resolved_symtab=self._resolved_symtab, **optional_kwargs, ) @@ -122,15 +140,34 @@ def __str__(self): class ExitEnvironmentAction(SessionAction): _id: str _keep_session_running: bool + _resolved_symtab: Optional["SerializedSymbolTable"] - def __init__(self, session: Session, id: str, keep_session_running: bool): + def __init__( + self, + session: Session, + id: str, + keep_session_running: bool, + resolved_symtab: Optional["SerializedSymbolTable"] = None, + ): super(ExitEnvironmentAction, self).__init__(session) self._id = id self._keep_session_running = keep_session_running + # The same table the environment was entered with, so its onExit + # resolves in the same scope as its onEnter (what + # Session.exit_environment documents for this argument). + # + # Unlike the worker agent, the CLI holds the table as an object rather + # than a JSON string served by the service, so there is no parse step + # here that could fail and no wrapper degrading a parse failure to None + # to keep teardown unconditional. An environment entered without a + # table simply exits without one. + self._resolved_symtab = resolved_symtab def run(self): self._session.exit_environment( - identifier=self._id, keep_session_running=self._keep_session_running + identifier=self._id, + keep_session_running=self._keep_session_running, + resolved_symtab=self._resolved_symtab, ) def __str__(self): diff --git a/src/openjd/cli/_run/_local_session/_session_manager.py b/src/openjd/cli/_run/_local_session/_session_manager.py index 602ac95..48d350e 100644 --- a/src/openjd/cli/_run/_local_session/_session_manager.py +++ b/src/openjd/cli/_run/_local_session/_session_manager.py @@ -3,7 +3,7 @@ from queue import Queue from threading import Event import time -from typing import Any, Iterable, Optional, Type +from typing import TYPE_CHECKING, Any, Iterable, Optional, Type from types import FrameType, TracebackType from signal import signal, SIGINT, SIGTERM, SIG_DFL from itertools import islice @@ -39,6 +39,10 @@ PathMappingRule, ) +if TYPE_CHECKING: + # Annotations only; see the note in _actions.py. + from openjd.expr import SerializedSymbolTable + class LocalSessionFailed(RuntimeError): """ @@ -118,6 +122,8 @@ class LocalSession: _path_mapping_rules: Optional[list[PathMappingRule]] _environments: Optional[list[Any]] _environments_entered: list[tuple[EnvironmentType, str]] + _step_symbol_tables: dict[str, "SerializedSymbolTable"] + _entered_env_symtabs: dict[str, "SerializedSymbolTable"] _log_handler: LocalSessionLogHandler _cleanup_called: bool @@ -127,6 +133,7 @@ def __init__( job: Job, job_parameter_values: JobParameterValues, session_id: str, + step_symbol_tables: Optional[dict[str, "SerializedSymbolTable"]] = None, timestamp_format: LoggingTimestampFormat = LoggingTimestampFormat.RELATIVE, path_mapping_rules: Optional[list[PathMappingRule]] = None, environments: Optional[list[Any]] = None, @@ -142,6 +149,21 @@ def __init__( self._timestamp_format = timestamp_format self._path_mapping_rules = path_mapping_rules self._environments = environments + # The create-time resolved symbol tables from + # `create_job_with_symbol_tables`, keyed by step name. A step's + # template-scope `let` (RFC 0005 §3.6) is evaluated once at job + # creation, and this is the only channel its resolved values have into + # a session: the model does not fold them into `script.let` and the + # session does not re-evaluate the source expressions. A caller that + # omits them gets a session in which every step-level `let` is simply + # undefined. + self._step_symbol_tables = step_symbol_tables or {} + # The table each entered environment was entered with, keyed by + # environment identifier, so its exit can be given the same one -- + # `Session.exit_environment` documents that as the way an onExit + # resolves in the same scope as its onEnter. Environments entered + # without a table are simply absent. + self._entered_env_symtabs = {} # Create an OpenJD Session self._openjd_session = Session( @@ -249,7 +271,7 @@ def run_environment_enters( environments: Optional[list[Any]], type: EnvironmentType, *, - extra_let_bindings: Optional[list[str]] = None, + resolved_symtab: Optional["SerializedSymbolTable"] = None, step_name: Optional[str] = None, ): """Enter one or more environments in the session.""" @@ -268,14 +290,19 @@ def run_environment_enters( session=self._openjd_session, environment=env, env_id=env_id, - # RFC 0007: a step's environments see the step-level `let` - # bindings. - extra_let_bindings=extra_let_bindings, + # RFC 0005 §3.6: a step's environments see the step's resolved + # template-scope `let` values through its symbol table. + resolved_symtab=resolved_symtab, # RFC 0007 §7.3.1 (EXPR): a step's environments see Step.Name. # Only step-environment enters carry a step name. step_name=step_name, ) self._environments_entered.append((type, env_id)) + # Recorded before the enter runs, and deliberately not undone on + # failure: whether the enter succeeds or not, the exit that follows + # must be given the table the enter was attempted with. + if resolved_symtab is not None: + self._entered_env_symtabs[env_id] = resolved_symtab try: self._current_action.run() except (RuntimeError, ValueError) as exc: @@ -332,7 +359,12 @@ def run_environment_exits(self, type: EnvironmentType, *, keep_session_running: prev_action_failed = self.failed self._action_ended.clear() self._current_action = ExitEnvironmentAction( - session=self._openjd_session, id=env_id, keep_session_running=keep_session_running + session=self._openjd_session, + id=env_id, + keep_session_running=keep_session_running, + # The same table the enter used, so onExit resolves in the same + # scope as onEnter. `pop` because an environment is exited once. + resolved_symtab=self._entered_env_symtabs.pop(env_id, None), ) self._current_action.run() self._action_ended.wait() @@ -352,7 +384,11 @@ def run_task(self, step: Step, parameter_set: TaskParameterSet) -> None: self._action_ended.clear() self._current_action = RunTaskAction( - session=self._openjd_session, step=step, parameters=parameter_set + session=self._openjd_session, + step=step, + parameters=parameter_set, + # RFC 0005 §3.6: the step's resolved template-scope `let` values. + resolved_symtab=self._step_symbol_tables.get(step.name), ) self._current_action.run() self._action_ended.wait() @@ -443,17 +479,16 @@ def run_step( if task_parameters is None: task_parameters = StepParameterSpaceIterator(space=step.parameterSpace) - # Enter all the step environments. When the step defines step-level - # `let` bindings (RFC 0007), its environments are entered with them so - # their variables and actions can reference them. - # getattr guard: requires an openjd-model with Step.let on the - # instantiated Job (openjd-model PR #318+); collapse to plain - # `step.let` once the version pin floor guarantees it. - step_let_bindings = getattr(step, "let", None) + # Enter all the step environments with the step's create-time resolved + # symbol table, so a step-level `let` (RFC 0005 §3.6) is visible to + # their variables and actions. The source expressions are not + # re-evaluated anywhere downstream, so a missing table here means the + # step's `let` names are undefined rather than merely stale. + resolved_symtab = self._step_symbol_tables.get(step.name) self.run_environment_enters( step.stepEnvironments, EnvironmentType.STEP, - extra_let_bindings=step_let_bindings or None, + resolved_symtab=resolved_symtab, # RFC 0007 §7.3.1 (EXPR): Step.Name is available to a step's # environments (openjd-rs threads the step's resolved symbol # table into enter_environment; this is the CLI counterpart). diff --git a/src/openjd/cli/_run/_run_command.py b/src/openjd/cli/_run/_run_command.py index 6f1b837..8011b40 100644 --- a/src/openjd/cli/_run/_run_command.py +++ b/src/openjd/cli/_run/_run_command.py @@ -4,7 +4,7 @@ from dataclasses import dataclass from pathlib import Path import json -from typing import Iterable, Optional +from typing import TYPE_CHECKING, Iterable, Optional import re import logging import time @@ -40,6 +40,10 @@ ) from openjd.sessions import PathMappingRule, LOG +if TYPE_CHECKING: + # Annotations only; see the note in _local_session/_actions.py. + from openjd.expr import SerializedSymbolTable + @dataclass class OpenJDRunResult(OpenJDCliResult): @@ -321,6 +325,7 @@ def _run_local_session( *, job: Job, job_parameter_values: JobParameterValues, + step_symbol_tables: Optional[dict[str, "SerializedSymbolTable"]] = None, step_list: list[Step], selected_step: Optional[Step], timestamp_format: LoggingTimestampFormat, @@ -346,6 +351,9 @@ def _run_local_session( with LocalSession( job=job, job_parameter_values=job_parameter_values, + # RFC 0005 §3.6: each step's create-time resolved `let` values. The + # only channel they have into the session. + step_symbol_tables=step_symbol_tables, timestamp_format=timestamp_format, session_id="CLI-session", path_mapping_rules=path_mapping_rules, @@ -464,7 +472,7 @@ def do_run(args: Namespace) -> OpenJDCliResult: try: # Raises: RuntimeError - the_job, job_parameter_values = generate_job( + the_job, job_parameter_values, step_symbol_tables = generate_job( args, environments, supported_extensions=extensions ) @@ -572,6 +580,7 @@ def do_run(args: Namespace) -> OpenJDCliResult: return _run_local_session( job=the_job, job_parameter_values=job_parameter_values, + step_symbol_tables=step_symbol_tables, step_list=step_list, selected_step=selected_step, task_parameter_values=task_parameter_values, diff --git a/src/openjd/cli/_summary/_summary_command.py b/src/openjd/cli/_summary/_summary_command.py index ebcb52a..86226b1 100644 --- a/src/openjd/cli/_summary/_summary_command.py +++ b/src/openjd/cli/_summary/_summary_command.py @@ -35,7 +35,7 @@ def do_summary(args: Namespace) -> OpenJDCliResult: try: # Raises: RuntimeError - sample_job, _ = generate_job(args, supported_extensions=extensions) + sample_job, _, _ = generate_job(args, supported_extensions=extensions) except RuntimeError as rte: return OpenJDCliResult(status="error", message=str(rte)) diff --git a/test/openjd/cli/conftest.py b/test/openjd/cli/conftest.py index 111ca35..7dad9c9 100644 --- a/test/openjd/cli/conftest.py +++ b/test/openjd/cli/conftest.py @@ -28,14 +28,20 @@ def sample_job_and_dirs(request): os.makedirs(current_working_dir) template = decode_job_template(template=MOCK_TEMPLATE) + # `job_from_template` also returns the per-step resolved symbol tables; + # this fixture's consumers construct LocalSessions without them, so it + # keeps yielding the (job, parameters, dirs...) shape. Tests that need + # the tables use the `sample_job_with_symtabs` fixture below. + job, parameters, _ = job_from_template( + template=template, + environments=[], + parameter_args=request.param, + job_template_dir=template_dir, + current_working_dir=current_working_dir, + ) yield ( - *job_from_template( - template=template, - environments=[], - parameter_args=request.param, - job_template_dir=template_dir, - current_working_dir=current_working_dir, - ), + job, + parameters, template_dir, current_working_dir, ) diff --git a/test/openjd/cli/templates/step_let_symtab_job.yaml b/test/openjd/cli/templates/step_let_symtab_job.yaml new file mode 100644 index 0000000..074205b --- /dev/null +++ b/test/openjd/cli/templates/step_let_symtab_job.yaml @@ -0,0 +1,39 @@ +specificationVersion: "jobtemplate-2023-09" +extensions: + - EXPR +name: StepLetSymtabJob +parameterDefinitions: + - name: Region + type: STRING + default: us-west-2 +jobEnvironments: + # Present so the "job environments get no step table" assertion has a call to + # inspect. Without it that assertion is vacuous. + - name: JobEnv + script: + actions: + onEnter: + command: python + args: ["-c", "print('JobEnvEntered')"] +steps: + - name: EchoStepLet + let: + # Template-scope `let`: resolved once at job creation. Its resolved value + # reaches a session only through the step's symbol table, so every + # assertion below fails outright if the CLI forwards nothing. + - bucket = "assets-" + Param.Region + stepEnvironments: + - name: StepLetEnv + script: + actions: + onEnter: + command: python + args: ["-c", "print('EnterSawStepLet={{ bucket }}')"] + onExit: + command: python + args: ["-c", "print('ExitSawStepLet={{ bucket }}')"] + script: + actions: + onRun: + command: python + args: ["-c", "print('TaskSawStepLet={{ bucket }}')"] diff --git a/test/openjd/cli/test_common.py b/test/openjd/cli/test_common.py index dad8dab..af501c5 100644 --- a/test/openjd/cli/test_common.py +++ b/test/openjd/cli/test_common.py @@ -370,7 +370,7 @@ def test_job_from_template_success( template_dir, current_working_dir = template_dir_and_cwd template = decode_job_template(template=template_dict) - result, _ = job_from_template(template, [], mock_params, template_dir, current_working_dir) + result, _, _ = job_from_template(template, [], mock_params, template_dir, current_working_dir) assert result.name == expected_job_name assert [step.model_dump(exclude_none=True) for step in result.steps] == [ step.model_dump(exclude_none=True) for step in template.steps diff --git a/test/openjd/cli/test_local_session.py b/test/openjd/cli/test_local_session.py index 712f37f..d226d63 100644 --- a/test/openjd/cli/test_local_session.py +++ b/test/openjd/cli/test_local_session.py @@ -172,7 +172,7 @@ def test_localsession_run_success( session, sample_job.steps[step_index].stepEnvironments, EnvironmentType.STEP, - extra_let_bindings=None, + resolved_symtab=None, step_name=sample_job.steps[step_index].name, ), ] @@ -337,7 +337,7 @@ def test_localsession_run_failed(sample_job_and_dirs: tuple, capsys: pytest.Capt session, sample_job.steps[SampleSteps.BadCommand].stepEnvironments, EnvironmentType.STEP, - extra_let_bindings=None, + resolved_symtab=None, step_name=sample_job.steps[SampleSteps.BadCommand].name, ), ] diff --git a/test/openjd/cli/test_step_symbol_tables.py b/test/openjd/cli/test_step_symbol_tables.py new file mode 100644 index 0000000..2924bdd --- /dev/null +++ b/test/openjd/cli/test_step_symbol_tables.py @@ -0,0 +1,215 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +"""The create-time step symbol table's path from job creation into a session. + +A step's template-scope ``let`` (RFC 0005 §3.6) is evaluated once, at job +creation. openjd-model does not merge the resolved bindings into ``script.let`` +and openjd-sessions does not re-evaluate the source expressions, so the only +channel those values have into a session is the ``resolved_symtab`` argument on +``Session.enter_environment``, ``Session.run_task`` and +``Session.exit_environment``. A CLI that forwards nothing produces a step whose +``let`` names are simply undefined -- not stale, absent. + +These tests pin the forwarding at each of the three call sites, and the +end-to-end result of all three together. +""" + +from pathlib import Path +from typing import Any +from unittest.mock import patch + +import pytest +import yaml + +from . import format_capsys_outerr, run_openjd_cli_main +from openjd.cli._common import SUPPORTED_EXTENSIONS +from openjd.cli._common._job_from_template import job_from_template +from openjd.cli._run._local_session._session_manager import EnvironmentType, LocalSession +from openjd.model import ( + RevisionExtensions, + StepParameterSpaceIterator, + decode_job_template, +) +from openjd.sessions import Session + +TEMPLATE_PATH = Path(__file__).parent / "templates" / "step_let_symtab_job.yaml" +STEP_NAME = "EchoStepLet" +# The resolved value of the template's `bucket = "assets-" + Param.Region` +# binding, under the Region parameter's default. +EXPECTED_BUCKET = "assets-us-west-2" + + +@pytest.fixture +def step_let_job(tmp_path: Path) -> Any: + """The step-``let`` job, together with its per-step resolved symbol tables. + + Deliberately a real ``job_from_template`` call rather than a stub table: the + point under test is that the values the model resolves at job creation reach + the session, and a hand-built table would not exercise that. + """ + template = decode_job_template( + template=yaml.safe_load(TEMPLATE_PATH.read_text()), + supported_extensions=SUPPORTED_EXTENSIONS, + ) + job, parameters, step_symbol_tables = job_from_template( + template=template, + environments=[], + parameter_args=[], + job_template_dir=tmp_path, + current_working_dir=tmp_path, + ) + return job, parameters, step_symbol_tables + + +def _local_session(job, parameters, step_symbol_tables, session_id: str) -> LocalSession: + """A LocalSession built the way ``do_run`` builds one for this job. + + ``revision_extensions`` must carry the extensions the job declares, or the + session parses the step's EXPR constructs against the default surface. + """ + return LocalSession( + job=job, + job_parameter_values=parameters, + session_id=session_id, + step_symbol_tables=step_symbol_tables, + revision_extensions=RevisionExtensions( + spec_rev=job.revision, supported_extensions=list(job.extensions or []) + ), + ) + + +def test_the_model_resolves_the_step_let_into_the_step_table(step_let_job) -> None: + """Precondition for the three forwarding tests: the table is worth sending. + + Not a forwarding test. It pins that ``create_job_with_symbol_tables`` + returns a table for the step, keyed by step name, carrying the step's + resolved ``let`` -- so a later forwarding test that finds nothing has a + forwarding bug rather than an empty producer. + """ + _, _, step_symbol_tables = step_let_job + + assert STEP_NAME in step_symbol_tables + symbols = step_symbol_tables[STEP_NAME].to_symtab().symbols + assert "bucket" in symbols + + +def test_run_task_forwards_the_steps_resolved_symtab(step_let_job) -> None: + """``Session.run_task`` receives the table for the step being run. + + Spied rather than stubbed, and driven through ``run_step``: the real + ``Session.run_task`` is what eventually fires the action-status callback + that ``LocalSession.run_task`` blocks on, so a no-op stub deadlocks. + """ + job, parameters, step_symbol_tables = step_let_job + step = next(s for s in job.steps if s.name == STEP_NAME) + parameter_set = next(iter(StepParameterSpaceIterator(space=step.parameterSpace))) + + with patch.object( + Session, "run_task", autospec=True, side_effect=Session.run_task + ) as patched_run_task: + with _local_session(job, parameters, step_symbol_tables, "run-task-symtab") as session: + session.run_step(step, task_parameters=[parameter_set]) + + assert not session.failed + patched_run_task.assert_called_once() + forwarded = patched_run_task.call_args.kwargs["resolved_symtab"] + # Identity, not merely truthiness: the step's own table, not another step's + # and not a rebuilt one. + assert forwarded is step_symbol_tables[STEP_NAME] + + +def test_step_env_enter_forwards_the_steps_resolved_symtab(step_let_job) -> None: + """A step environment is entered with its owning step's table. + + The job/external assertion at the end is a negative control: it cannot fail + against a revert of the forwarding (a revert makes *every* enter get no + table, which is what it asserts for these). It guards the other direction -- + forwarding one step's table to job or external environments, which are + outside step scope. + """ + job, parameters, step_symbol_tables = step_let_job + step = next(s for s in job.steps if s.name == STEP_NAME) + + with patch.object( + Session, "enter_environment", autospec=True, side_effect=Session.enter_environment + ) as patched_enter: + with _local_session(job, parameters, step_symbol_tables, "enter-symtab") as session: + session.run_step(step) + + assert not session.failed + + step_prefix = f"{EnvironmentType.STEP.name} - " + step_env_calls = [ + c for c in patched_enter.call_args_list if c.kwargs["identifier"].startswith(step_prefix) + ] + assert step_env_calls + for enter_call in step_env_calls: + assert enter_call.kwargs["resolved_symtab"] is step_symbol_tables[STEP_NAME] + + for enter_call in patched_enter.call_args_list: + if not enter_call.kwargs["identifier"].startswith(step_prefix): + assert enter_call.kwargs.get("resolved_symtab") is None + + +def test_step_env_exit_forwards_the_table_the_enter_used(step_let_job) -> None: + """A step environment's exit receives the same table its enter did. + + ``Session.exit_environment`` documents that as what makes an ``onExit`` + resolve in the same scope as its ``onEnter``. Identity against the enter's + argument rather than against the mapping, so a second lookup that happened + to return an equal-but-distinct table would still be visible. + """ + job, parameters, step_symbol_tables = step_let_job + step = next(s for s in job.steps if s.name == STEP_NAME) + + with ( + patch.object( + Session, "enter_environment", autospec=True, side_effect=Session.enter_environment + ) as patched_enter, + patch.object( + Session, "exit_environment", autospec=True, side_effect=Session.exit_environment + ) as patched_exit, + ): + with _local_session(job, parameters, step_symbol_tables, "exit-symtab") as session: + session.run_step(step) + + assert not session.failed + + step_prefix = f"{EnvironmentType.STEP.name} - " + entered = { + c.kwargs["identifier"]: c.kwargs["resolved_symtab"] + for c in patched_enter.call_args_list + if c.kwargs["identifier"].startswith(step_prefix) + } + assert entered + + exited = { + c.kwargs["identifier"]: c.kwargs.get("resolved_symtab") + for c in patched_exit.call_args_list + if c.kwargs["identifier"].startswith(step_prefix) + } + assert set(exited) == set(entered) + for identifier, table in entered.items(): + assert exited[identifier] is table + + +def test_do_run_step_let_reaches_all_three_action_kinds( + capsys: pytest.CaptureFixture, +) -> None: + """End to end: a step-level ``let`` produces its bindings in the session. + + One run covers all three entry points -- the step environment's ``onEnter`` + and ``onExit`` and the task's ``onRun`` each echo the bound value. The + assertion is on the resolved *value*, so a template-scope binding that + arrived unevaluated, or as a different step's value, fails here rather than + passing on mere presence. + """ + outerr = run_openjd_cli_main( + capsys, + args=["run", str(TEMPLATE_PATH), "--step", STEP_NAME], + expected_exit_code=0, + ) + + for label in ("EnterSawStepLet", "TaskSawStepLet", "ExitSawStepLet"): + assert ( + f"{label}={EXPECTED_BUCKET}" in outerr.out + ), f"{label} did not see the step's resolved `let`:\n{format_capsys_outerr(outerr)}" From 5bd7b98b6802ada23e9bcacf7129a57437a11d8f Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Sat, 29 Aug 2026 08:22:40 -0700 Subject: [PATCH 5/6] docs: Point the conftest note at a real fixture The note in `sample_job_and_dirs` sent readers to a `sample_job_with_symtabs` fixture "below" that exists nowhere in the suite. The fixture that actually carries the per-step resolved symbol tables is `step_let_job` in test_step_symbol_tables.py, which builds them through a real `job_from_template` call. Comment only; no behaviour change. The CodeQL "unused import" report for `SerializedSymbolTable` in _run/_run_command.py is a false positive and is deliberately not acted on: the import is already guarded by `if TYPE_CHECKING`, and the quoted annotation on `_run_local_session`'s `step_symbol_tables` parameter needs it. Removing it makes mypy fail with `Name "SerializedSymbolTable" is not defined`. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- test/openjd/cli/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/openjd/cli/conftest.py b/test/openjd/cli/conftest.py index 7dad9c9..98ab7ec 100644 --- a/test/openjd/cli/conftest.py +++ b/test/openjd/cli/conftest.py @@ -31,7 +31,7 @@ def sample_job_and_dirs(request): # `job_from_template` also returns the per-step resolved symbol tables; # this fixture's consumers construct LocalSessions without them, so it # keeps yielding the (job, parameters, dirs...) shape. Tests that need - # the tables use the `sample_job_with_symtabs` fixture below. + # the tables use the `step_let_job` fixture in test_step_symbol_tables.py. job, parameters, _ = job_from_template( template=template, environments=[], From 65c4ad12eed67d3a8bd1d412eabf643b7eae29a6 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Sat, 29 Aug 2026 09:12:43 -0700 Subject: [PATCH 6/6] docs: Fix a stale version citation and two step-let docstring overclaims Three text corrections, no behaviour change. The enter_environment rationale cited openjd-sessions 0.10.11 for "nothing raises post-registration" while pyproject now pins >= 0.12.0. The claim still holds at 0.12.0 (verified against the released wheel: every raise in enter_environment precedes registration, and the post-registration resolved_symtab failure calls _fail_action_before_start and returns), so only the citation moves. test_do_run_job_name_in_step_let_binding claimed neither omitting job_name from the Session nor passing an empty one would satisfy it. Measured otherwise: deleting job_name=str(job.name) leaves the test passing, because the binding is step-level and openjd-model resolves it at job creation, so the value is already a literal in the resolved table before Session.__init__ runs. Reworded to the create-time forward path it does pin: model to step_symbol_tables to CLI to step-environment enter. test_do_run_step_name_in_step_environment had the same shape, calling itself "the end-to-end proof of the feature". Dropping step_name=step_name on the enter also leaves it passing (measured); that keyword is pinned by test_localsession_step_env_enter_receives_step_name. Scoped accordingly. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- .../_run/_local_session/_session_manager.py | 2 +- test/openjd/cli/test_run_command.py | 41 +++++++++++++------ 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/src/openjd/cli/_run/_local_session/_session_manager.py b/src/openjd/cli/_run/_local_session/_session_manager.py index 48d350e..e4b5273 100644 --- a/src/openjd/cli/_run/_local_session/_session_manager.py +++ b/src/openjd/cli/_run/_local_session/_session_manager.py @@ -313,7 +313,7 @@ def run_environment_enters( # the trigger that reaches us in practice. Every failure it # detects *after* registering goes through # _fail_action_before_start() and returns normally instead, so - # as of openjd-sessions 0.10.11 nothing raises post-registration + # as of openjd-sessions 0.12.0 nothing raises post-registration # and the else-branch below is defensive. It is kept because the # cost of being wrong is asymmetric: if a future release does # raise after registering, dropping the environment from our diff --git a/test/openjd/cli/test_run_command.py b/test/openjd/cli/test_run_command.py index ad28f46..433c130 100644 --- a/test/openjd/cli/test_run_command.py +++ b/test/openjd/cli/test_run_command.py @@ -676,14 +676,21 @@ def test_run_local_session_enter_environment_raises(capsys: pytest.CaptureFixtur def test_do_run_step_name_in_step_environment(capsys: pytest.CaptureFixture) -> None: """ RFC 0007 §7.3.1 (EXPR) parity with openjd-rs: a step-level `let` binding - may reference Step.Name, and the step's environments are entered with the - binding so their actions can echo it. - - This is the end-to-end proof of the feature. It used to be `skipif`-gated on - feature-detecting the `step_name` keyword, which meant it did not run at all - against a sessions build that lacked it -- so the only test that actually - exercised Step.Name in a step environment was silently skipped. The - `openjd-sessions >= 0.10.11` floor guarantees the keyword, so it always runs. + may reference Step.Name, and the resolved value reaches the step's + environments. + + Same shape as test_do_run_job_name_in_step_let_binding: this pins the + create-time forward path -- openjd-model resolves `bound_name = Step.Name` + at job creation, the value travels in `step_symbol_tables`, the CLI hands + that table to the step-environment enter, and the onEnter action echoes it. + + Scope: it does NOT pin the CLI's `step_name` keyword on that enter. The + binding is step-*level*, so it is already a literal in the resolved table; + dropping `step_name=step_name` leaves this test passing (measured). That + keyword is pinned by test_localsession_step_env_enter_receives_step_name. + It also used to be `skipif`-gated on feature-detecting the keyword, so it + did not run at all against a sessions build that lacked it; the declared + openjd-sessions floor guarantees the keyword, so it always runs. """ template_dir = Path(__file__).parent / "templates" args = [ @@ -701,11 +708,19 @@ def test_do_run_step_name_in_step_environment(capsys: pytest.CaptureFixture) -> def test_do_run_job_name_in_step_let_binding(capsys: pytest.CaptureFixture) -> None: """ - RFC 0007 §7.3.1 (EXPR): Job.Name is seeded into the session symbol table, - so a step-level `let` binding may reference it and the step's environments - resolve the binding. The assertion is on the value, so neither omitting - `job_name` from the Session (Job.Name undefined, the binding fails) nor - passing an empty one satisfies it. + RFC 0007 §7.3.1 (EXPR): a step-level `let` binding may reference Job.Name, + and the resolved value reaches the step's environments. + + What this pins is the create-time forward path, end to end: openjd-model + resolves `bound_job_name = Job.Name` at job creation, the value travels in + `step_symbol_tables`, the CLI hands that table to the step-environment + enter, and the onEnter action echoes it. The assertion is on the value, so + any break in that chain fails here. + + Scope: because the binding is step-*level*, it is resolved before a Session + exists and is already a literal in the resolved table. Deleting + `job_name=str(job.name)` from the Session construction therefore leaves this + test passing (measured), so it does NOT pin the Session's Job.Name seeding. """ template_dir = Path(__file__).parent / "templates" args = [