fix(sessions): Keep task-emitted openjd_env exports in WrappedAction.Environment by leongdl · Pull Request #372 · OpenJobDescription/openjd-rs

leongdl · 2026-09-08T19:11:32Z

Fixes: no GitHub issue; a regression in #362, found by the upstream conformance fixture named below

What was the problem/requirement? (What/Why)

RFC 0008 requires WrappedAction.Environment to carry every openjd_env export from any earlier action in the session, "regardless of whether that action ran normally or via a wrap hook" (rfcs/0008-environment-wrap-actions.md:447-450).

run_task drives messages under a step identifier (session.rs:1864) that never has a created_env_vars entry — only enter_environment creates one (session.rs:1161). #362 repointed the three seed_wrapped_action_symbols call sites from the cumulative self.env_vars to live_session_env_vars(), which replays created_env_vars for still-entered environments only. Correct for its purpose, and it left every export a task makes with nowhere to live: the cumulative map was also the only home for exports no environment owns, and after #362 nothing reads it.

This regressed against the released lineage rather than being a long-standing gap. openjd-cli 0.1.14 / openjd-sessions 0.5.5 pass the fixture below. main does not. It has not shipped yet, so fixing it before the next release keeps it out of a published crate and out of openjd-model-for-python's next wheel embed.

Example template, and where it breaks

Two tasks. Each onRun exports a variable. The wrap hook prints the environment it was handed, then runs the wrapped command so the task's macro is forwarded through its stdout.

steps:
- name: Step1
  parameterSpace:
    taskParameterDefinitions:
    - name: N
      type: INT
      range: [1, 2]
  script:
    actions:
      onRun:
        command: python
        args: ["-c", "print('openjd_env: TASK{{Task.Param.N}}_VAR=set-by-task-{{Task.Param.N}}')"]
environments:
- specificationVersion: environment-2023-09
  extensions: [WRAP_ACTIONS, EXPR]
  environment:
    name: WrapEnv
    script:
      actions:
        onWrapTaskRun:
          command: python
          args:
          - "-c"
          # Dump what we were given, then run the wrapped task.
          - "import subprocess,sys\nfor e in {{repr_py(WrappedAction.Environment)}}:\n print('WAENV='+e)\nsys.exit(subprocess.run([{{repr_py(WrappedAction.Command)}}]+{{repr_py(WrappedAction.Args)}}).returncode)"

Task 2's hook must print WAENV=TASK1_VAR=set-by-task-1. On main it prints nothing:

--------- Running Task
N(INT) = 1
Output:
openjd_env: TASK1_VAR=set-by-task-1     <- SetEnv applied, under a step identifier
Process exited with code: 0                 with no created_env_vars entry
--------- Running Task
N(INT) = 2
Output:
openjd_env: TASK2_VAR=set-by-task-2     <- no WAENV line, so
Process exited with code: 0                 WrappedAction.Environment was empty

The same run against openjd-cli 0.1.14 prints WAENV=TASK1_VAR=set-by-task-1.

Upstream fixture, on branch conformance-wrap-actions-gaps:
conformance-tests/2023-09/WRAP_ACTIONS/jobs/wrap-openjd-env-task-grand-child-visible-next-task.test.yaml.

What was the solution? (How)

Add session_env_vars for exports no environment owns, and route every export through one place, record_env_export. live_session_env_vars returns the entered environments' replay layered over that map.

Restoring the pre-#362 read of the cumulative map would have fixed the fixture and reopened what #362 closed, so it is not that. The split is what makes both true at once.

Walkthrough of the fix

1. record_env_export, the routing rule. If the identifier has a created_env_vars entry, the export belongs to that environment and goes there, so exit_environment still prunes it. Otherwise it goes to session_env_vars for the life of the session. SetEnv, UnsetEnv, RedactedEnv and the declarative variables: map at enter all call it, so there is one rule rather than four copies of an if let.

2. The scoping constraint, which is not a style choice. An environment's own exports must stay out of session_env_vars. The available simplification, having every export write it, makes an environment's variables survive its own exit and re-fails #362's regression test. That test, exited_env_vars_absent_from_later_wrapped_action_environment, is in the mutation set below precisely so this cannot be "simplified" back later.

3. The clearing rule. An environment writing a name also removes it from session_env_vars, so the same name never appears in both stores. Without it a task's earlier value would either shadow the environment's while it is entered (overlay-wins) or reappear when the environment exits (created-wins) — states neither the pre-#362 cumulative map nor the post-#362 replay produces. environment_export_supersedes_an_earlier_task_export pins both halves.

4. live_session_env_vars starts from the overlay rather than an empty map, so the replay layers on top and an environment's value wins while it is entered.

5. Process environments are untouched. evaluate_env_vars still reads created_env_vars alone, so session_env_vars never reaches a child process and a task printing an openjd_env: line behaves the same wrapped and unwrapped. task_emitted_env_var_absent_from_later_subprocess_environment is the negative control, and it is what fails when a mutant routes a task export into created_env_vars.

What is the impact of this change?

WrappedAction.Environment regains variables a task exported, matching openjd-cli 0.1.14. Process environments, evaluate_env_vars and #362's pruning are unchanged. No public API change: both the field and the method are private.

How was this change tested?

cargo test --workspace passes. cargo clippy --all-features --all-targets --workspace -- -D warnings is clean. cargo fmt --all --check is clean.
The upstream fixture fails before this change and passes after. Sweeps against the release binary: EXPR 356/356, WRAP_ACTIONS 96/97. The one failure, wrap-openjd-env-grand-child-under-exit-hook, reproduces with this change stashed and is a separate pre-existing gap in the exit-hook path — worth its own issue, not fixed here.
New integration tests in crates/openjd-sessions/tests/integration/test_wrap_actions.rs are checked against 7 mutants. The mutants remove the overlay read, remove the overlay write, route environment exports into the overlay, drop the clearing rule, ignore a task unset, widen a task unset to clear the whole map, and route a task export into created_env_vars. All 7 are caught, and fix(sessions): prune exited environment variables from WrappedAction.Environment #362's own regression test is in the target set, which is what catches the third.
Not verified: Windows and macOS cross-user paths. The change is platform-independent (HashMap bookkeeping), and CI covers the three platforms.

Was this change documented?

Yes. specs/sessions/session.md gains an "Env var exports a task makes" section under Task Execution, stating the routing rule, the clearing rule, and that session_env_vars never feeds a process environment. The field and record_env_export carry doc comments with the same content, including what the narrow scope prevents.

Is this a breaking change?

No. session_env_vars and record_env_export are private, and no public signature changes. specs/sessions/public-api.md doesn't change.

Does this change impact security?

It narrows rather than widens the case #362 was about, and keeps its regression test green. An environment's exports — including openjd_redacted_env values — stay in created_env_vars and are still pruned from the wrap symbol when the environment exits. Only exports that no environment owns persist, which is what the released crate already did.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…Environment RFC 0008 requires WrappedAction.Environment to carry every openjd_env export from any earlier action in the session, "regardless of whether that action ran normally or via a wrap hook". A task runs under a step identifier that has no created_env_vars entry, so OpenJobDescription#362's live_session_env_vars replay dropped every export a task made and the next task's hook was handed nothing. OpenJobDescription#362 was right to stop reading the cumulative env_vars map: it retained exited environments' variables, including redacted ones. The gap it left is that the cumulative map was also the only home for exports no environment owns. Add session_env_vars for exactly those, and route every export through record_env_export. An environment's own exports still go to created_env_vars[identifier], so exit_environment prunes them from the wrap symbol; anything else goes to session_env_vars, which live_session_env_vars merges beneath the entered environments' replay. An environment writing a name clears that name from session_env_vars, so the two stores never hold the same name and the last writer is in effect. Without that, a task's earlier value would either shadow the environment's while it is entered or reappear when it exits, which is neither the pre-OpenJobDescription#362 nor the post-OpenJobDescription#362 result. session_env_vars never feeds a process environment. That stays evaluate_env_vars, built from created_env_vars alone, so a task printing an openjd_env: line behaves the same wrapped and unwrapped. This restores what openjd-sessions 0.5.5 did via the cumulative map, which is why the released CLI passes the fixture below and main does not. Verified against the upstream conformance fixture 2023-09/WRAP_ACTIONS/jobs/wrap-openjd-env-task-grand-child-visible-next-task on branch conformance-wrap-actions-gaps, which fails before this change and passes after. Sweeps: EXPR 356/356, WRAP_ACTIONS 96/97. The remaining failure, wrap-openjd-env-grand-child-under-exit-hook, reproduces with this change stashed and is a separate pre-existing gap in the exit-hook path. Four integration tests added, checked against 7 mutants, all caught. OpenJobDescription#362's own regression test is in that set: it rejects the simpler design where every export writes session_env_vars, because an environment's exports would then survive its exit. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>

…he overlay on top Closes two defects the automated review found in the first revision, and one pre-existing conformance failure that fell out with them. Ownership by the live stack. exit_environment pops the identifier off environments_entered before running onExit (session.rs:1476) and never removes the created_env_vars entry, so a created_env_vars lookup alone still called an onExit export environment-owned. record_env_export wrote it into an entry no reader consults and cleared the name from session_env_vars, so an onExit export silently deleted a task's export of that name and its own value went nowhere. Ownership is now decided by environments_entered. This also closes wrap-openjd-env-grand-child-under-exit-hook, which the previous revision's message recorded as a separate pre-existing gap: with onExit exports routed to the session-lifetime map, WRAP_ACTIONS goes 96/97 to 97/97. Overlay on top, not underneath. "Last writer wins" only held for environment after task. A task exporting a name an already-entered environment declared left both stores holding it, the replay won, and the task's export resurfaced when the environment exited -- the exact state the doc comment claimed the design prevented. layering session_env_vars over the replay, together with the existing rule that an environment write clears the overlay, makes the later writer win in both orders. Also drops a redundant contains_key conjunct from the guard: get_mut already returns None when the key is absent, so the conjunct could not change behaviour. A mutant removing it survived the suite, which is what a semantically inert branch looks like. run_subprocess exports reach the wrap symbol, which the review asked to be explicit about. Its {session}:subprocess:{uuid} identifier never has a created_env_vars entry, so those exports land in session_env_vars. RFC 0008 does not require it, since run_subprocess is not an OpenJD action, but openjd-sessions 0.5.5 surfaced them through its cumulative env_vars and dropping them would be a second divergence. Documented in the field comment and the spec, and pinned by run_subprocess_exports_reach_wrapped_action_environment. Three tests added, one per finding. Re-checked against 7 mutants covering both merge orders, both halves of the ownership guard, the clearing rule and both unset behaviours; 6 caught and the 7th was the inert conjunct now deleted. Sweeps: WRAP_ACTIONS 97/97, clippy clean with -D warnings, cargo test --workspace green. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>

Closes two more review findings. Both had one root cause: a pair of maps cannot express "the last write still in effect", so the previous revision only approximated it. A plain HashMap<String, String> has no tombstone. A task's openjd_unset_env could only remove from the session map, so when a still-entered environment owned the name, the replay put it straight back and the unset did nothing -- while a task SET of the same name in the same configuration did take effect. Clearing the session map on an environment write is lossy. With an outer environment also declaring the name, a task export was destroyed rather than shadowed, so when an inner environment declaring it exited, the outer environment's older value silently took over instead of the task's later one. Replace both stores' interaction with env_write_log: every openjd_env and openjd_unset_env write, in order, tagged Environment(id) or Session. live_session_env_vars folds it, skipping writes whose environment has exited, so for each name the winner is the last write still counting and an unset applies as a removal. One entry per (owner, key) keeps it bounded by distinct pairs. Environment writes still also go to created_env_vars, which is what evaluate_env_vars builds process environments from; the log never feeds one. environment_export_supersedes_an_earlier_task_export changed, so this is a behaviour change, not a refactor. It asserted that neither value survives the declaring environment's exit, which encoded the lossy clear as if intended. The task's write was only shadowed, so it takes over again -- the same reasoning that makes the two-environment case correct. Also corrects the seed_wrapped_action_symbols contract comment, which still told callers exited environments' variables are excluded. An onExit export is owned by no environment and does persist, which an_exit_scripts_export_does_not_erase_a_task_export asserts, so the comment invited someone to fix the behaviour back out. Two tests added, one per finding, each reproduced before the change. 45 wrap tests pass including OpenJobDescription#362's regression test, WRAP_ACTIONS 97/97, clippy clean with -D warnings, cargo test --workspace green. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>

github-actions · 2026-09-09T03:42:38Z

+                .environments_entered
+                .iter()
+                .any(|entered| entered == id),
+        }


Re-entering an environment under the same identifier resurrects the previous entry exports in WrappedAction.Environment.

enter_environment allows reusing an identifier after the environment has exited (exit_environment does self.environments.remove(identifier), so the DuplicateEnvironment check passes again) — wrap_env_file_eviction_on_exit_gives_fresh_path exercises exactly this with Some("env-1"). On re-entry, session.rs:1199 does created_env_vars.insert(identifier.clone(), HashMap::new()), deliberately wiping the prior variables, so evaluate_env_vars correctly starts the second entry from empty.

env_write_log has no such reset. Writes from the first entry are still in the log tagged Environment("env-1"), and env_write_in_effect only asks whether that id is on environments_entered — which it is again. So every name the first entry exported and the second entry does not re-declare comes back into live_session_env_vars(), at its original log position, with the old value.

That is both a change from pre-#362 behaviour (the symbol was derived from created_env_vars, so it reset with the map) and an internal divergence: after re-entry the process environment and WrappedAction.Environment disagree about the same name. A stale None tombstone from the first entry can also suppress a task export made in between.

The fix belongs next to the created_env_vars reset — drop stale writes for the identifier being entered:

self.created_env_vars.insert(identifier.clone(), HashMap::new()); self.env_write_log .retain(|w| w.owner != EnvWriteOwner::Environment(identifier.clone()));

This also bounds the log across repeated enter/exit cycles, which currently accumulate dead entries for the lifetime of the session.

github-actions · 2026-09-09T03:43:54Z

+    /// name is not held in both stores at once. Combined with
+    /// `live_session_env_vars` layering the session-lifetime map over the replay,
+    /// the later writer is the one in effect in both orders: environment after
+    /// task, and task after environment.


This doc block describes a design the code no longer implements, and names a field that does not exist.

There is no session_env_vars store on Session (the only session_env_vars in the crate is the seed_wrapped_action_symbols parameter), so "goes to the session-lifetime session_env_vars", "An environment writing a name clears it from session_env_vars", and "live_session_env_vars layering the session-lifetime map over the replay" all describe the two-map/overlay approach that env_write_log replaced. The implementation below neither clears anything from a second store nor layers a map over a replay — it appends to one ordered log and folds it.

Since the surrounding tests (task_export_supersedes_an_entered_environments_value and friends) explicitly document the two-map approach as the thing that got this wrong, leaving its vocabulary in the doc of the function that fixed it is actively misleading. Suggest rewriting the last paragraph in terms of the log: one entry per (owner, key) moved to the end on rewrite, and live_session_env_vars folding in write order, so the last in-effect write wins in both orders.

github-actions · 2026-09-09T03:46:58Z

+`evaluate_env_vars` builds process environments from. `env_write_log` never feeds a
+process environment.
+
+`session_env_vars` never feeds a process environment. That is `evaluate_env_vars`,


This paragraph is a leftover from the earlier two-store design and should be dropped.

There is no session_env_vars store on Session — the only session_env_vars in the crate is the seed_wrapped_action_symbols parameter, which is a value passed in, not a store that could "feed a process environment". The paragraph two above it already makes the intended point correctly and in terms of the store that actually exists:

env_write_log never feeds a process environment.

So this is both a near-verbatim duplicate and the one copy that names the wrong thing. Same stale vocabulary appears in the record_env_export doc comment (session.rs:2389-2392), flagged separately.

mwiebe · 2026-09-09T17:43:52Z

+    /// An environment's own export goes to `created_env_vars[identifier]`, so
+    /// `exit_environment` drops it from the wrap symbol. Anything else — a task's
+    /// `onRun`, or an `onWrapTaskRun` hook standing in for one — has no such
+    /// entry, and goes to the session-lifetime `session_env_vars`.


I don't understand this rule. onRun and onWrapTaskRun shouldn't be adding any entries to this, right? It looks like they get to add entries with more permanence than the environments do?

leongdl requested a review from a team as a code owner September 8, 2026 19:11

github-actions Bot added the waiting-on-maintainers label Sep 8, 2026