fix: Close remaining RFC 0008 wrap-action gaps in hook scoping, cleanup, and preflight

[mwiebe]

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

The WRAP_ACTIONS extension (RFC 0008) lets a job author write a "wrapper" environment.
Normally the runtime runs a job's commands directly; with a wrapper active,
the runtime instead calls one of the wrapper's three hook scripts
(onWrapEnvEnter, onWrapTaskRun, onWrapEnvExit) and hands it a
description of the command it replaced through WrappedAction.* template
variables (.Command, .Args, .Environment, ...). The classic use case
is running every task inside a container: the hook reads those values and
re-runs the command under docker exec.

The OpenJD conformance suite (small job templates that pin the behaviors the
spec requires) recently started running its WRAP_ACTIONS tests on Windows,
and 5 of them failed against our CLI.

1. A wrapper's own let variables didn't work in its hooks. An
   environment script can define let: variables (small named values
   computed once, like image_tag = 'v2'). The spec says those names are
   available in the environment's actions — and the wrap hooks are actions
   of the wrapper's script. But the runtime resolved hooks against the
   wrapped step's variables, which know nothing about the wrapper's, so a
   hook referencing {{image_tag}} failed with "Undefined variable".

2. The wrapper's and the wrapped step's variables were not kept separate.
   The wrapped command's description (WrappedAction.*) and the
   hook's own script were resolved against the same symbol table. If the
   wrapper and the step both defined a let variable named who, one
   side's value silently won. Each side must only ever see its own scope.

3. A failed environment start skipped all cleanup. OpenJD guarantees
   that every environment a session entered — or tried to enter — gets
   its exit action run before the session ends (so a wrapper that started a
   container always gets to tear it down). The CLI instead aborted the whole
   run on the first enter failure: nothing was exited. Step-scoped
   environments had the same bug through a separate early-return.

4. Two wrappers could sneak in. The spec allows at most ONE wrapping
   environment per session, and requires rejecting a violating session
   before entering any environment. Our validator catches two wrappers
   inside a single template file, but two wrappers supplied as separate
   --environment files were only caught at enter time — after the first
   wrapper's own onEnter had already run (and possibly started a
   container).

5. Two smaller gaps: WrappedAction.Environment (the list of KEY=value
   variables a hook can forward into its container) omitted variables an
   environment declared via its static variables: map, only including
   openjd_env exports — the spec was clarified to include both; and the
   model crate's API doc didn't list a function that is actually exported
   at the crate root.

What was the solution? (How)

Two scopes, kept strictly apart (openjd-sessions). The wrap dispatch
now builds two separate symbol tables:

• the inner scope — everything the wrapped command would have seen had
  it run unwrapped (the step's or inner environment's own let variables
  and embedded files). WrappedAction.Command/Args/Timeout/Cancelation.*
  are resolved against this table, and only this table.
• the hook scope — the wrapper's own world: its frozen parameter values,
  its own let variables (fixing problem 1), plus the WrappedAction.*
  values computed above. The hook script resolves against this table, and
  only this table.

Previously both jobs were done on one shared table (problem 2). A new
regression test defines the same let name in both scopes and asserts each
side sees its own value.

A cleanup path in the CLI (openjd-cli). The run command now
records every environment it enters — job-level, template-supplied, and
step-scoped — in one list. A failed enter no longer aborts the run
structure: it marks the session failed, skips remaining work, and falls
through to cleanup, which exits every recorded environment in reverse
(LIFO) order. Step environments unwind to a per-step baseline. This makes
the CLI honor the guarantee end-to-end: a wrapper whose onWrapEnvEnter
fails still gets its onWrapEnvExit and its own onExit.

Preflight before any enter (openjd-cli). Before entering anything,
the CLI scans every environment stack the run will build (external
--environment templates + the job's environments + each selected step's
environments) and rejects the run if any stack contains more than one
wrap-defining environment. The session-level enter-time check stays as
defense in depth for library callers.

Environment-template parameters in hooks (openjd-cli). --environment
templates are now converted with a symbol table built from the preprocessed
job parameters, so a wrapper template's own parameterDefinitions resolve
inside its hooks ({{Param.Prefix}} in a hook no longer fails).

Spec docs updated in the same PR: specs/cli/run.md (cleanup +
preflight), specs/sessions/public-api.md (new
MultipleWrapEnvironments error), specs/model/public-api.md
(convert_environment_with_symtab at the crate root).

What is the impact of this change?

• Wrapper environments behave per RFC 0008 in the CLI: hooks can use their
  own parameters and let variables, name collisions between wrapper and
  wrapped scopes are impossible, failed enters still clean up, and invalid
  two-wrapper sessions are rejected before any side effects.
• openjd run's environment enter/exit flow was restructured; output now
  includes a Process exited with code: N line for a failed enter action
  (which the conformance runner uses to attribute failures).
• The previously POSIX-only WRAP_ACTIONS conformance fixtures now pass on
  Windows (the fixture-side changes are in the companion
  openjd-specifications PR).

How was this change tested?

• Have you run the unit tests?
  • Yes: cargo test --workspace is green except two pre-existing Windows
    cross-user logon-error-mapping failures on the development workstation.
  • New tests: 3 sessions integration tests (wrapper let in hooks,
    variables: map in WrappedAction.Environment, wrapper/wrapped scope
    separation) and 1 CLI end-to-end test (failed wrap-enter still runs
    wrap-exit and the wrapper's own exit) with 2 new fixture templates.
• Full OpenJD conformance suite on Windows: 1112/1112, including all
  WRAP_ACTIONS fixtures that were previously skipped there.
• cargo clippy --all-features --all-targets --workspace -- -D warnings
  is clean.
• Scope separation additionally verified by hand end-to-end through the
  CLI with same-named let bindings in wrapper and step.

Was this change documented?

• Are relevant docstrings in the code base updated?
  • Yes. seed_wrapped_action_symbols documents the two-scope model,
    build_wrapped_inner_scope documents how the inner scope is built, and
    the CLI's enter/exit macros document the cleanup guarantee. The spec
    documents under specs/ were updated in the same commits (see above).

Is this a breaking change?

No for the published API surface. openjd-sessions gains a new
SessionError::MultipleWrapEnvironments variant (additive; breaking only
for exhaustive matches on this non-trivial error enum, which the
pre-release policy tolerates). Behavior changes are all corrections toward
what RFC 0008 already required.

Does this change impact security?

No new files, directories, or permission changes. The scope-separation fix
is defensive.

---

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]

This final cleanup (and the per-step exit_envs_down_to!(step_env_baseline)) is bypassed when run_task returns an Err rather than a failed ActionState. The three task call sites still use .map_err(...)? (lines 624, 706, 763), so a session-level error from run_task returns early from execute() and skips all environment exits — the same cleanup-bypass class this PR fixes for the enter path.

This matters here specifically because the scope-separation work newly introduces run_task error modes in the wrap path: if a wrap environment’s let bindings or WrappedAction.* resolution fails, seed_wrapped_action_symbols returns SessionError::FormatString, which propagates out of run_task as Err. In that case a wrapper whose onEnter already started a container would never get its onWrapEnvExit/onExit run — the very guarantee the PR is trying to establish end-to-end.

Consider treating a run_task Err the same as a failed enter: log it, set session_failed = true, and fall through to cleanup instead of ?-propagating.

[mwiebe]

I think #276 is covering this already. Let's look at it in that pr.

[jericht]

nit: this execute function is getting a bit long at 800+ lines, might be worth simplifying it later on

[leongdl]

Important fix: the cleanup guarantee is still bypassed when run_task returns Err

This PR fixes the failed-enter path so cleanup always runs, but the same guarantee has a remaining hole on the task path in crates/openjd-cli/src/run/mod.rs:

All three session.run_task(...) call sites use

.map_err(|e| format!("Step '{}': {e}", step.name))?

and the parameter-space setup inside the step loop (StepParameterSpaceIterator::new(ps)?, the validate_containment early returns) does the same. A nonzero task exit returns Ok(Failed) and flows into exit_envs_down_to! — but a dispatch error returns Err, and the ? early-returns past both cleanup macros.

This is exactly the failure class this PR makes more likely to occur: a wrap hook referencing an undefined variable fails in seed_wrapped_action_symbols → SessionError::FormatString via fail_action_setup → run_task returns Err → the wrapper's onWrapEnvExit / onExit never run, and a container the wrapper started leaks.

Note the session state at that point is ReadyEnding (set by fail_action_setup / drive_action), and exit_environment accepts ReadyEnding — so the exits would succeed if the CLI fell through to cleanup instead of returning.

Suggested fix (mirrors the enter-failure handling this PR already added):

let result = match session.run_task(...).await {
    Ok(r) => r,
    Err(e) => {
        eprintln!("ERROR: Step '{}': {e}", step.name);
        session_failed = true;
        break; // falls through to exit_envs_down_to!(step_env_baseline) / (0)
    }
};

with the same treatment for the iterator/containment errors. A companion test would be the existing test_run_failed_wrap_enter_still_runs_wrap_exit_and_own_exit pattern, but with a wrap task hook referencing {{undefined_var}} and asserting WrapEnv Own Exit still appears.

Happy to see this as a fast-follow rather than blocking — the PR is a clear net improvement — but it closes the same guarantee (RFC 0008 "Lifecycle and cleanup guarantees") this PR sets out to establish, so it'd be good to ticket it before merge.

[leongdl]

Complexity metrics for the functions this PR touches

Measured at head adec788 (cyclomatic complexity approximated by counting if/while/for/match arms/&&/||/? — treat as ±10%):

Function (touched by PR)	Length	~CC
openjd-cli run/mod.rs::execute	779 lines	~113
session.rs::enter_environment_with_output	258 lines	~31
session.rs::exit_environment	239 lines	~25
session.rs::run_task	182 lines	~15
session.rs::seed_wrapped_action_symbols	124 lines	~14
session.rs::build_wrapped_inner_scope	34 lines	~7

For comparison, execute() on the base branch was 682 lines / ~CC 103, so this PR adds ~97 lines and ~10 complexity points to a function already ~10x past the common CC threshold of 10–15.

To be fair to this PR: it reduces structural duplication (four divergent copy-pasted enter/exit loops became one entered_envs ledger + two macros, so there is now exactly one enter path and one exit path), and the new sessions-crate helpers are well-factored. The problem is the function it lives in.

At CC ~113, "does every failure path reach cleanup?" is genuinely hard to audit — even with AI assistance, code is really hard to make right at these levels. It's why the four divergent exit loops existed in the first place, and why the remaining run_task-Err cleanup bypass (previous comment) slipped through this PR too. The enter_env!/exit_envs_down_to! macros are also a symptom: they exist because the logic captures a dozen locals from execute's scope.

Suggested follow-up (not blocking): introduce a RunContext struct owning session, entered_envs, session_failed, and the formatting state, turning the macros into methods and letting execute() split into ~100-line phases (preflight / environment lifecycle / step execution / reporting). Fixing the run_task-Err cleanup gap would be a natural moment to start, since it touches exactly the error-propagation paths a decomposition would clarify.

[leongdl]

1. Important fix — run_task Err bypasses cleanup:
   #277 (comment)
   (#277 (comment)) — describes the
   trigger (wrap-hook dispatch error), why cleanup would have succeeded (session is in ReadyEnding), the
   suggested match-and-break fix, and a companion test idea. Framed as fast-follow-acceptable but
   ticket-before-merge.

[leongdl]

Performance: the wrap dispatch now deep-clones the entire Environment on every wrapped task run

crates/openjd-sessions/src/session.rs, run_task wrap dispatch (~line 1649) — this PR changed:

.map(|action| (wrap_env.resolved_symtab.clone(), action))

to

.map(|action| (wrap_env.clone(), action))

which deep-copies the whole Environment — including the wrapper's embedded script bodies in script.embedded_files data — on every wrapped task run. The same pattern is at the onWrapEnvEnter/onWrapEnvExit dispatch sites (~lines 1192 and 1436), though those are per-environment rather than per-task.

seed_wrapped_action_symbols only needs three things from the wrap env: name, resolved_symtab, and script.let_bindings. The clone appears to be forced by the borrow conflict with fail_action_setup(&mut self).

Why it matters: embedding the wrapper's hook script via embeddedFiles is the common RFC 0008 pattern (the container-wrapper use case from the PR description), so this is a full copy of potentially MB-scale script data per task. At CLI scale (sequential tasks, subprocess spawn dominating) it's mostly noise — but openjd-sessions is the library worker agents consume, and those run much hotter.

Suggested fix, either of:

• store Arc<Environment> in self.environments so the dispatch clones a pointer, or
• extract just the needed fields into a small owned struct (name: String, resolved_symtab: Option<SerializedSymbolTable>, let_bindings: Option<Vec<String>>) before the &mut self borrow, and pass that to seed_wrapped_action_symbols.

Not blocking, but cheap to fix now while the dispatch code is fresh.