fix(sessions): resolve Env.File.* inside wrap action hooks - #315
fix(sessions): resolve Env.File.* inside wrap action hooks#315seant-aws wants to merge 1 commit into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| // Register wrap env file paths BEFORE seed (so lets can reference Env.File.*). | ||
| if let Some((ref wrap_id, ref files)) = wrap_env_id_and_files { | ||
| self.ensure_wrap_env_files(wrap_id, files.as_deref(), &mut action_symtab) |
There was a problem hiding this comment.
The wrap env's Env.File.* symbols are registered into action_symtab before build_wrapped_inner_scope is called (line 1738 here, and lines 1214/1224 + 1480/1487 on the enter/exit paths). build_wrapped_inner_scope takes &action_symtab as its base and clones it, so the wrapper's Env.File.* entries end up inside the inner scope too — the scope in which the wrapped action's command/args/timeout and the inner entity's let bindings are resolved.
That inverts the isolation invariant seed_wrapped_action_symbols documents ("a wrapper-defined name can never leak into the wrapped action's resolved values"):
- For a step (
onWrapTaskRun),Env.File.*is not part of a step script's scope at all, yet the wrappedonRuncan now resolve{{Env.File.<wrapper-file>}}. - For
onWrapEnvEnter/onWrapEnvExit, the inner env sees any wrapper file name it does not itself declare (its own names correctly shadow, since it allocates over the same keys afterwards).
Model-level validation rejects most such references, so the blast radius is templates that bypass it (externally supplied / worker-injected environments) — but the resolution result differs from the unwrapped run, which is the property the wrap path is supposed to preserve. Building the inner scope from a snapshot of action_symtab taken before the wrap-env registration would keep both scopes correct while preserving the register→seed→write ordering.
| // exited. The on-disk files are intentionally NOT deleted — they live | ||
| // in the session files directory and are cleaned up with it. | ||
| if env_has_any_wrap_hook(&env) { | ||
| self.wrap_env_file_records.remove(identifier); |
There was a problem hiding this comment.
Eviction is unreachable when the wrap environment's own onExit fails: the EnvironmentScriptFailed return a few lines above (and the result? before it) bypasses this line, while self.environments.remove(identifier) / environments_entered.pop() have already run at the top of the function.
The leftover entry is more than a leak. Because identifier is no longer in self.environments, the DuplicateEnvironment check in enter_environment_with_output no longer rejects it, so a caller that retries with the same identifier (worker-agent-supplied ids are stable) will hit ensure_wrap_env_files → cache hit and get the previous environment's FileRecords: symbols named after the old embedded files, pointing at the old paths, while the new environment's files are never allocated or written. A hook referencing one of the new file names then fails with Undefined variable.
Removing the entry right after the environments.remove/pop (i.e. alongside the other tracking teardown, before the exit script runs) makes eviction unconditional and keeps it paired with the state it mirrors.
| &self.session_id, | ||
| ) | ||
| .with_user(self.cross_user.user.clone()); | ||
| ef.allocate_file_paths(files, symtab)?; |
There was a problem hiding this comment.
This cache is a second, independent allocation of the same wrap environment's embedded files. When the wrap env was entered, EnvironmentScriptRunner::enter built its own EmbeddedFiles and allocated paths for the onEnter script (runner/env_script.rs:236) — that instance is dropped, and the first wrap-hook invocation allocates again here.
For named files both allocations land on <files_dir>/<filename>, so they agree. For unnamed files each allocation calls random_hex_filename(), so {{Env.File.foo}} inside the wrap env's own onEnter and {{Env.File.foo}} inside its onWrapTaskRun resolve to different files. Anything the onEnter script writes into that path is invisible to the hooks — which is a natural way to author a wrap env (stage state in an unnamed embedded file at enter, consume it per task) and would silently read the wrong file. It also leaves an extra unnamed file in the session files dir per wrap env.
Populating the cache from the enter-time allocation (or having enter consult the same cache) would make the symbol mean one file for the whole environment. If the divergence is intentional, it is worth stating explicitly in specs/sessions/embedded-files.md, since the new section reads as though there is a single allocation per wrap environment.
A wrap environment's embedded files were never registered as `Env.File.*`
symbols in the resolution scope of its wrap hooks, so a hook referencing
`{{Env.File.<name>}}` failed with `Undefined variable`. This affected all
three hooks -- onWrapEnvEnter, onWrapTaskRun and onWrapEnvExit -- since
none of them consulted the wrap environment's `embedded_files`. Only
inline wrap scripts worked, which is why the gap went unnoticed: every
existing wrap test declared `embedded_files: None`.
`EmbeddedFiles::allocate_file_paths` both allocates on-disk paths and
defines the symbols in one pass, so there was no way to reuse an
allocation across invocations. This adds `register_file_paths`, which
defines the symbols for already-allocated records without allocating
paths or creating files, and a per-wrap-environment cache on `Session`
keyed by the environment identifier. Paths are allocated on the first
hook invocation and reused thereafter, so `Env.File.*` paths stay stable
across tasks and unnamed embedded files do not accumulate one temporary
file per task. Contents are rewritten per invocation so each invocation
starts from the authored content. The cache is dropped when the wrap
environment itself exits; the files live in the session files directory
and are cleaned up with it.
The registration is driven from the three hook dispatch sites rather
than from `seed_wrapped_action_symbols`, which would otherwise be the
single shared place to put it. That function evaluates the wrap
environment's `let` bindings and replaces the symbol table with the
result, so the required order is: register file paths, then seed, then
write contents. Registering first is what allows a wrap environment's
`let` binding to reference `{{Env.File.*}}`; writing last is what allows
embedded file `data` to resolve against the post-`let` symbol table.
This matches the ordering already used by `build_wrapped_inner_scope`.
Driving it from inside the seed function would also require threading
the session's files directory, session id and user through a function
that has no `&self`, and whose `WrapEnvironmentScope` argument does not
carry `embedded_files`.
Tests cover `Env.File` in a wrap-only environment (no `onEnter`), in an
environment that also has `onEnter`, in the enter and exit hooks, in a
`let` binding, with multiple embedded files, and when the hook does not
reference the files at all.
Two path-stability tests exist deliberately. The named-file case is
insufficient on its own: a named embedded file always resolves to
`<files directory>/<filename>`, so its path is identical whether or not
the allocation is cached. Mutation testing confirmed this -- forcing the
cache-miss branch left the named-file test passing while the unnamed-file
test failed with two different generated filenames. Only an unnamed file,
whose name is generated at allocation time, can detect a regression in
the caching behaviour.
Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>
dc81917 to
ae83e51
Compare
1 participant
What was the problem/requirement? (What/Why)
A wrap environment's embedded files were never registered as
Env.File.*symbols in the resolution scope of its wrap hooks (onWrapEnvEnter,onWrapTaskRun,onWrapEnvExit). Any hook referencing{{Env.File.<name>}}failed withUndefined variable. The Python v0 implementation already handled this; the Rust engine had deferred it with an explicit code comment.What was the solution? (How)
Added
EmbeddedFiles::register_file_paths— defines symbols for already-allocated file records without re-allocating paths or creating files. The session caches per-wrap-environment allocations keyed by environment identifier, so paths are allocated once (first hook invocation) and symbols are re-registered on subsequent invocations.What is the impact of this change?
Env.File.*now resolves correctly in all three wrap hooks. No change to non-wrap session paths.How was this change tested?
cargo test -p openjd-sessions— 452 passed, 0 failed (unit 165, integration 281, doc 6)cargo clippy -p openjd-sessions— cleantest_wrap_actions.rs:test_wrap_env_enter_resolves_env_file,test_wrap_task_run_resolves_env_file,test_wrap_env_exit_resolves_env_file, plus a caching test that verifies the same path is reused across invocations.Was this change documented?
Updated
specs/sessions/embedded-files.mdandspecs/sessions/session.md.Is this a breaking change?
No.
Does this change impact security?
No.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.