fix: Record task-emitted openjd_env macros for WrappedAction.Environment

[leongdl]

Fixes: no GitHub issue; 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).

A task's onRun, and an onWrapTaskRun hook that replaces it, run with _running_environment_identifier set to None, because run_task calls _reset_action_state(). The ENV and UNSET_ENV branches of _action_callback discarded the macro in that case, so the next task's hook received an empty WrappedAction.Environment.

This was deliberate in 0.12.1: the comment at _session.py:2409-2424 cited the base wiki rule that openjd_env "can only be emitted by the Action for entering an Environment", called the behaviour a known divergence from openjd-rs, and noted that no conformance fixture covered it. That fixture now exists.

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. Before this change it printed nothing:

Running command /tmp/OpenJD/.../tmpb10zmjil.sh
Output:
openjd_env: TASK1_VAR=set-by-task-1     <- macro arrives on the scanned pipe
Process pid 55573 exited with code: 0
...
--------- Running Task
N(INT) = 2
Output:
openjd_env: TASK2_VAR=set-by-task-2     <- and still no WAENV line, so
Process pid 55577 exited with code: 0      WrappedAction.Environment was empty

The macro is visibly parsed and forwarded. It is the recording that was skipped, so this is not a plumbing or buffering problem.

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)

Record such a macro in _session_env_vars, and remove it there on openjd_unset_env. Leave _created_env_vars untouched, so the environment of every child process is unchanged and a task behaves the same wrapped or unwrapped.

That reproduces the released openjd-rs behaviour rather than inventing one. In openjd-sessions 0.5.5 (shipped in openjd-cli 0.1.14, which passes this fixture), apply_message writes the cumulative env_vars for every SetEnv regardless of which action is running, and that map is the session_env_vars argument to seed_wrapped_action_symbols (session.rs:1262, :1534, :1784). Process environments come from evaluate_env_vars, which reads created_env_vars alone:

	rs 0.5.5 (passes)	this change	attribute to the wrap env instead
Task macro in WrappedAction.Environment	yes	yes	yes
Task macro in a child process environment	no	no	yes

The rejected column is why _created_env_vars is left alone: it has no counterpart in any openjd-rs version and would let one task's stdout mutate sibling tasks' environments.

Walkthrough of the fix

1. _action_callback, ENV branch. The None guard now records instead of discarding. Malformed payloads keep the old path: cancel_action_mark_failed is checked first, because the filter pairs that flag with a parse-error str rather than the name/value dict, and because outside an environment there is no environment action to fail — failing the task instead would be a behaviour change for existing jobs.

2. UNSET_ENV branch. The mirror: pop the name. An unset is the one remover from this map, matching openjd-rs, where UnsetEnv erases the cumulative map whether or not an environment declared the name.

3. _collect_session_env_list, a concurrency defect this change widens. The reader iterated _session_env_vars live while _action_callback writes it on the LoggingSubprocess stdout thread. That raises RuntimeError: dictionary keys changed during iteration — reproduced 3 of 3 probe runs before, 0 of 4 after. The hazard pre-existed for environment-emitted macros, but this change adds writes on the task path, which is exactly when this list is built for onWrapTaskRun. The reader now iterates a dict.copy(). Not a lock: the writer runs on the thread forwarding a live child's output and must not be able to block on the reader. An inline comment asserting there was "no new hazard" was wrong and is gone.

4. _log_discarded_env_macro. Now reached only for malformed macros, so its docstring and message say that. Its isinstance(value, dict) branch became unreachable — both call sites are gated on cancel_action_mark_failed, where the payload is always a str — so it is deleted rather than left as dead code with a passing test.

5. Docstrings. _session_env_vars gains the routing rule and a pointer to read openjd-rs at its released tag, not main: openjd-rs #362 repointed wrap-hook seeding at live_session_env_vars(), so main currently fails this same fixture. That is tracked as an openjd-rs regression, with the fix in openjd-rs#372.

What is the impact of this change?

WrappedAction.Environment gains variables a task exported. Real process environments are unchanged, so unwrapped jobs and every existing subprocess see exactly what they saw before.

One consequence to review deliberately: openjd_redacted_env reaches _action_callback as an ENV message with a name/value dict (_action_filter.py:675), so a task's redacted export is now recorded like a plain one and reaches the hook's argv. That matches what an environment's redacted export has always done through this same map, and matches released openjd-rs, where RedactedEnv also writes the cumulative map. Log redaction is unaffected — the filter holds the value in _redacted_values either way. A test pins the behaviour, so excluding redacted values is a one-line inversion if you prefer it.

How was this change tested?

• Have you run the unit tests? Yes. 957 passed, coverage 75.01% against the 50% gate. test_subprocess.py has 6 failures in test_run_gracetime_when_process_ends_but_grandchild_uses_stdout; they reproduce with the source change reverted, so they are a pre-existing macOS environment problem.
• The upstream fixture fails before this change and passes after, verified locally against a venv of openjd-cli 0.7.7 + openjd-model 0.11.9 + this branch. Full sweeps: WRAP_ACTIONS 97/97, EXPR 356/356.
• 8 new tests in test/openjd/sessions_v0/test_wrap_actions.py, checked against 10 mutants: both record paths, the unset path, pop replaced by clear(), the malformed path in both directions, three ways of leaking into _created_env_vars, keying on the wrong field, and the live-dict read. All 10 are caught.
• The class is marked @serial_process and uses variable names unique to it. Three of the 10 mutants survived a first draft of these tests, including pop → clear(), which would let one task's unset wipe every session variable.
• Not verified: Windows. The tests use the same bare sh -c and as_posix() conventions as the rest of this file, which Windows CI runs, but they were run on macOS only.

Was this change documented?

Yes. The _session_env_vars attribute docstring states the routing rule and the openjd-rs comparison. _collect_session_env_list documents why it snapshots, and _log_discarded_env_macro documents its narrowed role. The stale comment claiming the discard was intentional is replaced by the RFC citation that reverses it.

Is this a breaking change?

No. No public interface changes. The behaviour change is confined to the WrappedAction.Environment symbol, which gains entries it was required to have.

Does this change impact security?

Worth a look, and flagged above rather than buried: a task's openjd_redacted_env export now reaches WrappedAction.Environment, and a wrap hook interpolating that symbol puts the value in its argv, readable by other processes on the host. This is consistent with the pre-existing behaviour for an environment's redacted export and with released openjd-rs, and log redaction is unaffected. If maintainers want redacted values kept out of the symbol, that is a deliberate divergence from openjd-rs and the pinning test is where to invert it.

---

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

[github-actions]

A task-emitted openjd_unset_env now removes the name from _session_env_vars even when an entered environment still owns it in _created_env_vars. That makes the two views disagree in the direction that breaks wrapped/unwrapped equivalence — the opposite direction from the SET branch above.

Concretely (this is exactly what test_task_emitted_unset_removes_only_the_named_variable pins): env Setter exports TASKEMIT_DOOMED, a task prints openjd_unset_env: TASKEMIT_DOOMED, and afterwards:

• an unwrapped task still gets TASKEMIT_DOOMED=doomed-value (the test asserts SAW=[doomed-value]), because _created_env_vars was untouched;
• a wrapped task no longer sees it, because WrappedAction.Environment is where the hook gets the variable list from, and the name is gone.

So a task can silently strip a session variable from every subsequent wrapped task while leaving it present for every unwrapped one. The SET branch is careful to avoid precisely this class of divergence ("a task behaves the same wrapped or unwrapped"); the UNSET branch introduces it.

The parity argument in the comment also does not obviously carry over: openjd-rs erasing its cumulative map is consistent there only because that map is not the sole source of the wrapped environment in the same way. If the intent is "a task cannot mutate the effective environment", then the safer reading is that an out-of-environment unset should only be able to remove a name that was itself recorded out-of-environment — i.e. not one an entered environment still holds. Otherwise this needs to be called out as a deliberate, spec-visible asymmetry.

[leongdl]

Correct on the facts, deliberately unfixed. Leaving this thread open, because the asymmetry is real and worth a maintainer decision rather than my judgement.

Everything you describe is what happens, and my own test asserts the pair: the unwrapped reader still gets SAW=[doomed-value] while the wrapped hook no longer sees the name.

Where I would push back is on it being this PR's divergence. I checked the released Rust implementation, which is the parity target the whole change is measured against: in openjd-sessions 0.5.5, UnsetEnv does self.env_vars.remove(&key) unconditionally, before and independently of the per-environment write, and self.env_vars is what seeds WrappedAction.Environment. So a task-emitted unset removes the name from the wrap symbol while created_env_vars keeps it in the process environment — the same split, in the same direction. Your suggested alternative, restricting an out-of-environment unset to names recorded out-of-environment, would be a deliberate divergence from that.

So the two candidate readings are "match the reference implementation" and "preserve wrapped/unwrapped equivalence for unset", and they conflict. I have taken the first because parity is what this PR is for, and because the SET branch's equivalence claim is about process environments, which no unset here touches. But you are right that it needs to be visible rather than implied: the UNSET branch comment names the parity, and I would rather the maintainers rule on whether the spec should say what an out-of-environment unset may remove. Happy to invert it in this PR if the answer is the second reading.

[github-actions]

Security note on the widened SET branch: this path also receives openjd_redacted_env, because _handle_redacted_env re-dispatches as ActionMessageKind.ENV with a plain name/value dict (_action_filter.py:676). So a task printing openjd_redacted_env: SECRET=hunter2 now lands its cleartext value in _session_env_vars, and from there into WrappedAction.Environment, which a wrap hook interpolates into its own command/argv.

Before this change a task-emitted redacted export was dropped here, so this is a new exposure surface, not just a new symbol value. Two concrete leak channels worth checking before merging:

1. argv is world-readable. A hook rendering {{WrappedAction.Environment}} puts the cleartext into the child process command line, visible via /proc/<pid>/cmdline to any process of the same uid (and to ps for the session user). The _redacted_values scrubber only covers the log stream, not process argv.
2. _redacted_values only helps if the exact substring survives. apply_message_redaction does a literal find, so the value is scrubbed when a hook echoes it verbatim — but the "Running command ..." line in _subprocess.py:663 goes through redact_openjd_redacted_env_requests, which only redacts when the literal token openjd_redacted_env: appears in the command line. A hook command that embeds the value (not the token) gets no protection from that helper and relies entirely on the filter having already seen the macro. Since the value is recorded on the stdout thread and the hook is launched from the caller thread, that ordering holds for a later task but is worth confirming, especially for the newly-reachable task-emitted case.

test_task_emitted_redacted_env_is_listed_like_any_other_export already flags this as a deliberate choice and invites inversion — this comment is to make sure the argv-exposure half of the tradeoff is on the record, since the test comment only argues about log redaction ("log redaction is unaffected"). Excluding redacted names from _session_env_vars (or recording a placeholder) would keep the RFC 0008 MUST satisfied for ordinary exports without pushing secrets through argv.

[leongdl]

Correct, and the argv half is a fair addition to the record. Leaving this open as a maintainer decision; I have not changed the behaviour, for one reason worth checking before you rule on it.

The argv exposure is not new with this PR. An environment's openjd_redacted_env export has always been recorded into _session_env_vars by the in-environment branch at line 2446, which pre-dates this change, so it already reaches WrappedAction.Environment and already lands in a hook's argv. What this PR adds is the task-emitted path to the same destination. Excluding redacted values for tasks only would leave two macros with the same name behaving differently depending on which action printed them, which I think is worse than the exposure; excluding them for both paths is defensible but is a behaviour change to existing sessions and a divergence from released openjd-rs, where RedactedEnv also writes the cumulative map that seeds the symbol.

Your second point is the sharper one and I had not verified the ordering, so thank you: redact_openjd_redacted_env_requests only fires on the literal openjd_redacted_env: token, so a hook embedding the value gets nothing from it and depends entirely on the filter having already registered the value. For the newly-reachable case that ordering does hold — _redacted_values.add happens in _handle_redacted_env on the stdout thread strictly before the callback that records the variable, so by the time any later hook can render it the value is registered — but that is an implicit dependency, not a guarantee anything asserts.

Your placeholder suggestion is the option I would pick if maintainers want the exposure closed: it satisfies the RFC MUST for ordinary exports without putting cleartext in argv. test_task_emitted_redacted_env_is_listed_like_any_other_export is where to invert it, and it is one line.

[github-actions]

_session_env_vars is now growable from arbitrary task stdout, and it has no bound. Before this PR only an environment-entry action could add to it — a template-controlled, small set. Now any task that prints openjd_env: lines in a loop appends session-lifetime entries that are never reclaimed (environment exit deliberately does not remove them, per the docstring above), and _collect_session_env_list() renders all of them into WrappedAction.Environment.

Two consequences:

• Memory: a long-lived session with a chatty task grows this dict without limit. dict.copy() on every wrap-hook launch also copies it in full each time.
• WrappedAction.Environment becomes a launch-failure vector: a hook that renders {{WrappedAction.Environment}} (as the RFC examples and the tests in this PR do) puts every entry into the child argv. Once the accumulated list crosses ARG_MAX (~128 KiB per arg / 2 MiB total on Linux), Popen fails with E2BIG and every subsequent wrapped task in the session fails to start — not just the task that emitted the macros. A single misbehaving task can now poison the rest of the session for wrapped execution.

The PR's own concurrency test writes 20 000 entries into this map, so the scale is not hypothetical. Worth considering a cap on the number of task-emitted entries (or on total serialized size) with a warning when it is hit, since unlike the environment-entry path there is no template-side limit on how many a task can emit.

[leongdl]

Correct in mechanism, and I am not fixing it here. Leaving the thread open, since the concern outlives this PR.

The unbounded growth and the E2BIG path are both real, and the consequence you name — one task poisoning wrapped execution for the rest of the session — is the part worth taking seriously.

Two corrections to the scope, though. First, the vector is not new in kind: an environment's onEnter script can already emit openjd_env: in a loop, and those exports go into this same map through the pre-existing in-environment branch, so an unbounded session-lifetime map is reachable on mainline today. What this PR changes is which action can trigger it. Second, released openjd-rs has the same unbounded cumulative map feeding the same symbol, so capping here alone would diverge.

One nit on the evidence: the concurrency test's 20 000 entries were written directly to the dict by the test, not produced through the macro path, so they show the reader's cost rather than a production growth rate. That test has since been replaced with a deterministic one for unrelated reasons, so the number is gone either way.

A cap is the right idea and I would rather it be designed once, for both paths and both implementations, with a defined behaviour when it is hit — silently dropping exports and failing a hook at ARG_MAX are both bad, and choosing between "reject the macro" and "truncate the symbol" is a spec question. That is a follow-up issue rather than something to bolt onto a regression fix. Happy to file it with this analysis if you want it tracked.

[github-actions]

The comment here claims "a task behaves the same wrapped or unwrapped," but the SET branch breaks that in the overwrite direction — and the PR's own test_task_export_does_not_displace_an_environments_value_in_a_child pins the divergence:

• an unwrapped task gets TASKEMIT_BOTH=from-env (the test asserts SAW=[from-env]), because _created_env_vars is untouched;
• a wrapped task is handed TASKEMIT_BOTH=from-task in WrappedAction.Environment, because _session_env_vars[name] = value at line 2452 overwrites unconditionally and _collect_session_env_list() renders the effective value.

So this is the same class of wrapped/unwrapped divergence as the UNSET branch below, not the careful counterexample to it. Worth correcting the comment at minimum.

The part that concerns me more than the docstring is the reachability change. Before this PR a task could not write _session_env_vars at all, so WrappedAction.Environment was entirely template-controlled: declarative variables: plus environment-entry openjd_env. Now any task's stdout can define or redefine any name in it, and the RFC 0008 examples (and the hook in test_wrap_hook_export_reaches_the_next_tasks_wrapped_environment) render that symbol straight into the hook's command. A hook shaped like env {{WrappedAction.Environment}} -- {{WrappedAction.Command}} therefore lets task A set LD_PRELOAD, PATH, PYTHONPATH, or NODE_OPTIONS for every subsequent wrapped task in the session, including tasks from other steps — a cross-task influence channel that does not exist for unwrapped execution and did not exist before this change.

Note this is not the same as the redacted-value/argv point raised separately: that one is about a secret's cleartext transiting argv, this one is about a task choosing the names and values another task's wrapped launch is configured with. Even a hook that does not itself interpolate the symbol into argv is affected if it forwards the list into the child's real environment, which is the documented purpose of the symbol.

RFC 0008's MUST is about inclusion of earlier openjd_env variables; it does not obviously require that a task-emitted name be allowed to shadow a name an entered environment owns in _created_env_vars. Restricting the outside-environment write to names not currently owned by any entered environment (or recording it under a separate task-emitted layer that loses to _created_env_vars when _collect_session_env_list() merges) would satisfy the MUST while keeping the two views from contradicting each other. If shadowing is intended, it deserves to be called out as a spec-visible, security-relevant asymmetry rather than asserted as equivalence in a comment.