feat: RFC 0008 environment wrap actions - #333
Merged
Merged
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
leongdl
force-pushed
the
rfc008
branch
8 times, most recently
from
July 17, 2026 22:37
d757cd7 to
06e30ee
Compare
AlexTranAmz
reviewed
Jul 20, 2026
leongdl
force-pushed
the
rfc008
branch
2 times, most recently
from
July 22, 2026 21:15
935fefb to
d1ec0d2
Compare
5 tasks
…ement the WRAP_ACTIONS extension (RFC 0008) in the v0 session, with EXPR (RFC 0007) runtime parity with openjd-rs: - Wrap-hook dispatch: an environment's onWrapEnvEnter / onWrapTaskRun / onWrapEnvExit runs in place of an inner environment's onEnter/onExit or a step's onRun, with WrappedAction.Command/Args/Environment/ Timeout/Cancelation.* and WrappedEnv.Name/WrappedStep.Name injected. At most one wrap-defining environment may be active (enforced at enter time). - Two strictly separated scopes (openjd-rs OpenJobDescription#277 parity): the wrapped action's values resolve against the INNER entity's own scope (its script-level let bindings and embedded files, materialized in runner order: paths, lets, contents); the hook script resolves against the wrap environment's own scope (its lets, evaluated by the runner) plus the WrappedAction.* overlay. Same-named lets on the two sides each resolve to their own value. - WrappedAction.Environment carries every session-defined variable: openjd_env definitions and entered environments' declarative variables: maps; host-inherited variables are excluded. - EXPR runtime: runner-evaluated script-level let bindings ordered around embedded-file materialization (paths before lets, contents after), typed symbol tables, enter_environment(extra_let_bindings=...) so a step's environments see the step-level lets. - Cancelation: WrappedAction.Cancelation.Mode/NotifyPeriodInSeconds resolved through the enforcement path, with Template Schemas 5.3.2 defaults (120s task onRun / 30s otherwise). Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Review fixes on top of the RFC 0008 wrap-actions commit: - enter_environment/exit_environment no longer raise a raw ExpressionError out of the public API when an extra `let` binding fails to evaluate: the action fails through _fail_action_before_start with the callback, leaving the environment entered-but-failed exactly like a failed onEnter subprocess. - enter_environment(step_name=...) seeds Step.Name (RFC 0007 §7.3.1) before the extra bindings evaluate, remembered per environment and re-seeded at exit — openjd-rs parity (the Rust runtime threads the per-step resolved symtab into both enter and exit). - let-binding RHS parsing memoized and single-sourced via openjd.model.evaluate_let_bindings (was re-parsed through the Rust engine per env-enter/exit/task). - Wrap environment embedded-file paths allocated once per environment and reused across wrap-hook invocations (stable Env.File.*, no O(task-count) temp-file growth); contents still re-resolved per task. - One resolve_optional_int_field(ge=, le=) replaces three disagreeing optional-int resolvers — this also adds the previously missing non-positive check to Session._resolve_action_timeout (openjd-rs parity). - Complexity/duplication reductions: _fail_action, shared runner cancel() via _cancel_with_effective_cancelation, _make_env_script_runner, _try_inject_wrapped_symbols, WRAP_HOOK_ACTION_NAMES single-sourced, SimplifiedEnvironmentVariableChanges.effective_items() (no more private poking from _collect_session_env_list), whole-field detection via FormatString.whole_field_expression(). - pyproject: note that the openjd-model pin floor must be the first release containing model PR OpenJobDescription#318's surface. Tests: 647 passed (12 new regression tests: failure paths, Step.Name enter+exit, parse memoization, int-field bounds, wrap-file reuse); pre-existing environment failures unchanged. mypy/black/ruff clean. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
leongdl
force-pushed
the
rfc008
branch
2 times, most recently
from
July 25, 2026 00:04
cdf95fa to
3f786cb
Compare
Two Rust-parity fixes in the v0 session runtime: 1. WrappedAction.Args now uses RFC 0005 1.3.2 typed argument semantics. The enforcement path's typed arg loop (null skip, list flattening, display coercion) is extracted into a shared module-level helper, resolve_action_arg_values, in _runner_base.py; both _inject_wrapped_task_symbols and _inject_wrapped_env_symbols now use it, so a wrap hook sees exactly the argv the wrapped action would have run with unwrapped -- mirroring openjd-rs, whose seed_wrapped_action_symbols resolves through the same resolve_action_args as the runner. The helper also gains openjd-rs's plain-string-resolution fallback when typed resolution fails. 2. An empty string is no longer conflated with null. resolve_optional_int_field and resolve_effective_cancelation's deferred-mode branch now resolve typed (resolve_value) and treat only a typed null result as "field omitted" / "no cancelation declared". A genuine empty string now reaches the "must be a positive integer, got ''" / "must resolve to ... got ''" errors, matching openjd-rs's resolve_action_timeout, resolve_notify_period_seconds, and resolve_effective_cancelation, which only special-case ExprValue::Null. The whole_field_expression() pre-check is removed: resolve_value only yields a typed null for whole-field expressions, so null semantics remain whole-field-only by construction. Test updates: the deferred-cancelation unit-test helper now parses its format strings with the EXPR extension (deferred forwarding is an RFC 0008 construct and WRAP_ACTIONS requires EXPR), since the previous legacy parse pinned the non-Rust "" == null behavior. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Address findings F1-F8 and Review22-F3/F4 from round 3 concurrency review: - F1: Always route EnvironmentScriptRunner.cancel() through pending-cancel path - F2+F4: Atomic terminal arbitration - claim pending cancel in _on_process_exit, move liveness check inside lock in _cancel - F3: Monotonic merge for duplicate pending cancels (min time_limit, OR failed) - F5: Snapshot action_status before publishing READY state - F6: Wrap cancel_info.json write in try/except, fallback to immediate terminate - F7: Detect self-join in shutdown(), use wait=False if called from worker thread - F8: Move callback outside lock, wrap in try/except to prevent child discard - Review22-F3: Snapshot _runner in cancel_action to avoid bare AssertionError - Review22-F4: Bind _process once in notify/terminate to avoid TOCTOU race Add test_concurrency_fixes.py with 8 unit tests covering the defensive behaviors. None of these issues reproduce in openjd-rs due to CancellationToken, tokio async, and Rust's Result<> error handling model. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
…-G7, R4-G8)
R4-1: Bound let RHS length before parse (MAX_LET_BINDING_LENGTH=4096)
Prevents SIGBUS crash from parser stack overflow on malicious input.
R4-5: Hoist _fail_action() call outside the runner lock
Prevents deadlock when callback calls cancel().
R4-6: Isolate consumer-callback exceptions at terminal delivery
Try/except around callback invocations in _fail_action() and
_fail_action_before_start() so exceptions don't escape public API.
R4-G7: Make failure attribution monotonic for live duplicate cancels
Use 'or' merge for _notify_canceled_action_as_failed so earlier
mark_action_failed=True isn't erased by subsequent cancels.
R4-G8: Pass bound process snapshot through platform helpers
notify()/terminate() pass proc to _posix_signal_subprocess() and
_windows_notify_subprocess() to eliminate reload race.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Implement all nine round-5 findings, plus the sibling occurrences of each found by sweeping the codebase for the same defect class. Log redaction and the action filter (_action_filter.py): - R5-1: clear record.args unconditionally. The redaction logic only inspects record.msg, but a handler calls getMessage(), which re-runs `msg % args` -- so a record leaving the filter with args populated re-interpolated an unscanned secret into the emitted line. On the format-failure path the args are now folded into msg textually rather than dropped, because a record whose own formatting is broken would otherwise raise inside the handler and logging would dump the raw args to stderr, outside this filter's reach. - R5-2: catch Exception, not just ValueError. `handler` invokes the consumer-supplied callback, and any other exception type escaped filter() into the stdout pump thread -- killing the pump, dropping the rest of the subprocess's output, and leaving the child unreaped. Adds _invoke_callback() for the one callback not routed through `handler`, and makes the redaction step itself fail closed. - Anchor the env var NAME regexes with \Z instead of $. `$` also matches before a trailing newline, so "FOO\n" validated as a name. Values stay permissive on purpose: multi-line values via the JSON form are supported, tested behaviour. Temporary directories (_tempdir.py, _session.py): - R5-3: <tempdir>/OpenJD is a fixed, predictable path whose parent is world-writable on typical POSIX hosts, and makedirs(exist_ok=True) accepted whatever was already there -- another local user's directory, or a symlink. Validate ownership with lstat and refuse anything that is not a real directory owned by this euid or root. makedirs()' mode is umask-masked, so the mode is now also set outright; without it, a hostile umask leaves the root untraversable by the job user. Sibling: Session._openjd_session_root_dir() was a second creator of the same path with the mode constant duplicated. Deleted -- custom_gettempdir() now owns create, validate and chmod, which also covers TempDir(dir=None). - R5-7: keep the exception in TempDir.cleanup(). It was accepted and discarded, so failures listed bare paths with no cause on the one path where "permission denied" versus "still held open" changes what to do next. Uses onexc on 3.12+, onerror below that. - R5-8: document the trust precondition behind the deliberate 0o770 widening. The grant is to a group, so every member of it can modify the session tree. Narrowing it would remove cross-user impersonation rather than harden it. Generated shell script (_runner_base.py): - R5-4: shlex.quote the `cd` path. A single quote in the path closed the hand-written quoted region and the remainder was interpreted by /bin/sh. - R5-5: validate env var names before emitting export/unset. The value was quoted but the name was not, so a name containing `;` or `$(` injected commands. Names that are not POSIX shell identifiers are skipped with a warning rather than rejected: rejecting would break Windows embedders (ProgramFiles(x86) is a real name), escaping is impossible, and the variable still reaches the subprocess through Popen's env=, which uses no shell. Previously such a name produced an outright /bin/sh syntax error that failed the whole action. Invariants under `python -O` (six files): - R5-6: convert the invariant-bearing asserts to explicit raises, leaving pure type-narrowing ones alone. Triaged on whether the check is in a public API, runs on a background thread where an AssertionError reaches only threading.excepthook, or *is* an earlier fix. Notably ScriptRunnerBase.state asserted on a reachable inconsistency, making the runner permanently unreadable after a failed submit, and the two asserts added by the R4-5 fix silently reverted that fix under -O. Also binds LoggingSubprocess.exit_code once; the double-load raised AttributeError out of a public property when read cross-thread while run()'s finally cleared _process. - Records the assert-versus-raise policy in DEVELOPMENT.md. Process groups (_subprocess.py, _linux/_sudo.py): - R5-9: record an unknown process group as None rather than substituting the dead child's pid. None is already this field's "unknown" value, which _posix_signal_subprocess() and find_sudo_child_process_group_id() both use; _subprocess.py was the outlier, and in the sudo branch the pid is not even the right kind of identifier. The behaviour the fallback existed to protect (an immediately-exiting child must not fail the action) is preserved. Sibling: guard the first getpgid in find_sudo_child_process_group_id, which let ESRCH escape to callers only looking for a signal target. Adds test/openjd/sessions_v0/test_round5_fixes.py: 60 tests, each mutation-checked by reverting its fix and confirming the test fails. Verified: 784 passed / 39 skipped / 16 xfailed (only the 2 known xdist timing flakes fail, and pass in isolation); ruff, mypy and black clean; conformance 2023-09 at 1162 passed / 0 failed. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
A five-agent sweep of 4c57f40 found three defects in that commit itself. The first is a regression: it turned a latent AssertionError into silent state corruption. REG-1 (_runner_base.py) -- R5-6 replaced `assert self._run_future is not None` in the `state` property with `return READY`. Two consequences, both reproduced: a) `_run`'s single-use guard is `if self.state != READY: raise`, so reporting READY for a launch that failed after `_process` was assigned silently opened the guard. A second subprocess launched over the first, replacing `_process` and orphaning the original child. b) `_on_process_exit` built `ActionState(self.state.value)`. `ActionState` has no `ready` member, so this raised ValueError, which the F8 try/except around the callback caught and logged -- delivering no terminal callback at all. A consumer would wait forever on an action that had finished. Fixed three ways, smallest first: - The single-use guard is now latched on a new `_launched` flag rather than derived from `state`. Tying the rule to the state classification is what let it break when the classification changed; a "has a launch been attempted" latch cannot drift that way. `state` is still consulted for the `_state_override` that `_fail_action` sets. - `_process` and `_run_future` are published together after `_pool.submit` rather than `_process` first. The inconsistent pair was observable on the ordinary success path too, since `state` is read without the lock; removing the window removes the question the R5-6 change had to answer. - New `_terminal_action_state()` maps an unclassifiable state to FAILED. Publishing a wrong-but-terminal state beats publishing nothing. REG-2 (_action_filter.py) -- the R5-2 containment interpolated the consumer's exception into an f-string at three sites, so an exception whose `__str__` raises escaped `filter()` anyway, from inside the handler meant to contain it. New `_describe_exception()` falls back through str, repr, and the type name. REG-3 (_tempdir.py) -- the R5-3 ownership check was check-then-use: it validated with `os.lstat(path)` and then called `os.stat(path)`/`os.chmod(path)`, both of which re-resolve the name and follow symlinks. Swapping the entry for a symlink in between defeated the check and widened the link's target to 0o755. `_prepare_temp_dir_root` now opens the directory once with O_NOFOLLOW and O_DIRECTORY and uses fstat/fchmod, so every decision and every modification applies to the inode that was validated. Also from the sweep, in the same files: - Removed the `_ENVVAR_NAME_FORBIDDEN` post-decode check added in 4c57f40. It is unreachable -- the raw-message regex rejects every input that would decode to a bad name -- and its test's assertion loop never executed, so it passed against a deliberately broken implementation. The test is rewritten to assert the rejection directly and now fails when the regex is loosened. - Corrected two comments that overstated what the code does: the `\Z` anchor change is defence in depth, not a fix for a reachable defect (the outer filter regex already strips a trailing newline), and `_on_process_exit` now genuinely reads the future it was called with. - Corrected the R5-5 warning text. It promised the variable still reaches the subprocess via Popen's env=, which is false for a cross-user action: `sudo -i` starts a login shell that resets the environment, so the generated script's export lines are the only channel there. Adds test/openjd/sessions_v0/test_round5_regressions.py (14 tests). Every test was mutation-checked against 9 mutants covering each behaviour above, with __pycache__ cleared between mutants -- restoring a file with `mv` puts back the original mtime and can leave Python serving bytecode compiled from the mutant, which produced one false failure while writing these. Verified: 801 passed / 39 skipped / 16 xfailed (only the known xdist timing flake fails, and passes in isolation); ruff, mypy and black clean. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
RFC 0008 specifies two strictly separated resolution scopes: the wrapped action
resolves against the INNER entity's own scope, and the hook resolves against the
WRAP environment's own scope plus the WrappedAction.* overlay.
Only one direction was actually isolated. `_build_wrapped_inner_scope` resolves
the wrapped action against a copy, so wrap -> inner was closed. But the hook
resolved against the inner entity's own symbol table, so everything the Session
writes into that table directly was readable from a hook:
- a wrapped task's Task.Param.* / Task.RawParam.*
- the running step's Step.Name
- the extra_let_bindings the *inner* environment was entered with
Reproduced: a hook argument of `{{Task.Param.Frame}}` resolved to the wrapped
task's frame number, and `{{Step.Name}}` to the running step's name.
`_seed_wrap_env_scope` runs after injection, so it only re-seeded the wrap env's
own context -- it never removed the inner entity's.
This matters because a wrap environment and the job it wraps need not have the
same author: the wrapping environment is the mechanism by which an operator
interposes a container runtime or a license gate, and it should not thereby gain
read access to the work it is wrapping. RFC 0008 supplies WrappedStep.Name
precisely because Step.Name is not meant to be reachable from a hook, and
openjd-model rejects none of these references in an environment script, so this
runtime was the only gate.
An inner script's *script-level* `let` bindings never leaked -- those live only in
the copy -- and that part was already covered by
test_wrap_actions.py::test_inner_env_let_does_not_leak_into_hook_scope.
Fix: a new `_build_wrap_hook_scope()` builds the hook a fresh session-scope table
rather than reusing the inner entity's, and all three hook call sites
(enter_environment, exit_environment, run_task) inject into and run against it.
The path-mapping symbols are copied over rather than re-materialized so both
scopes name the same rules file and no second file is written.
A hook still gets everything it legitimately needs: session scope, Job.Name,
job parameters, the path-mapping symbols and the engine's host rules, its own
environment's enter-time step name and extra_let_bindings, the WrappedAction.*
overlay, and its own script-level lets and embedded files.
Adds test/openjd/sessions_v0/test_wrap_scope_isolation.py (13 tests), which
asserts on the symbol table the Session actually hands to the hook's runner
rather than on a table the test built itself.
Three subagents audited the tests independently. Their findings changed the
result rather than confirming it:
- A gap they found is now covered: a mutant that keeps the hook's EXPR host
context but empties its path-mapping rules made apply_path_mapping() inside
a hook silently become the identity function, with the whole suite green. No
test anywhere combined a wrap environment with path_mapping_rules. Now
test_path_mapping_reaches_the_hook_intact does.
- Also now covered: EXPR *types* (leaking the inner table's expr_types, and
stripping the hook's own) survived every mutation.
- Two audit-driven test corrections: the capture helper indexed the most
recent runner, so a regression that stopped invoking one hook would have let
a test assert against the other hook's table; and `_run_until_ready`
returned silently on timeout, so twelve assertions on a table built before
the subprocess starts would have passed against a hung action.
- Switched from `true`/`echo` to the suite's `python_exe` fixture: neither is a
native Windows executable, and every test here now requires its action to
complete.
- Removed a dead self-justifying helper and a factually wrong claim in the
module docstring about what was previously covered.
Mutation-checked with 12 mutants, including each of the three call sites
independently and the plausible-but-wrong fix of copying the inner table. The
harness now verifies each restore by checksum and requires a green baseline: an
earlier run silently left one mutation in place, which made every later verdict
meaningless.
Verified: 813 passed / 39 skipped / 16 xfailed (only the 2 known xdist timing
flakes fail; they pass in isolation); ruff, mypy and black clean.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
`LoggingSubprocess.run()` is the only holder of the Popen handle, and clearing `self._process` in its `finally` is the point after which nothing can reach the child again -- `notify()` and `terminate()` both become permanent no-ops. Yet `_log_subproc_stdout()`, `wait()` and the returncode capture all sat inside one `try`, so any exception out of the stdout pump skipped the wait and the capture while still clearing the handle. Reproduced with a `logging.Filter` that raises on the child's output -- filters run inside `Logger.handle`, so they propagate into the pump, and installing one is a supported use of this library (Session installs ActionMonitoringFilter). Result: a child still running after `run()` returned, `exit_code` None, `is_running` False, `failed_to_start` False, and `terminate()` unable to reach it. For a cross-user action that is a process running as the job user with no owner, holding the session directory open. An earlier change hardened ActionMonitoringFilter so it no longer raises. That closed the one in-tree trigger; it did not close this, which is structural. Fix: a new `_reap()` runs from the `finally` on every path. If the child is still alive it is terminated and waited for, and its exit status is recorded either way. `terminate()`'s signal delivery is extracted into `_terminate_process(proc)` so the `finally` can reach it after `self._process` is cleared -- the same argument-passing reason the platform helpers already take the Popen. The wait is bounded (ABANDONED_PROCESS_REAP_TIMEOUT_SECONDS): this runs on the runner's pool worker, so a child that somehow outlives SIGKILL must not hang the future as well as leaking the process. Failures inside the reap are caught and logged, because it runs in a `finally` and anything it raised would replace the exception already propagating and hide the real cause. Deliberately removed rather than added: an `if self._returncode is None:` guard around the status capture. A mutation test showed it could never change the outcome -- `run` has already assigned the same value on the normal path, and this is the only assignment on the abandoned one. An unfalsifiable check manufactures confidence, so the assignment is unconditional and the comment says why. Adds test/openjd/sessions_v0/test_subprocess_reaping.py (11 tests). Notable: the "a reap failure must not replace the original exception" test goes through `run()` rather than calling `_reap()` directly. The first version called `_reap` itself, never entered the `finally` being pinned, and passed happily when the reap's `except Exception` was narrowed to `except OSError` -- the mutation run caught that and the test was rewritten. Mutation-checked: 8 mutants, 8 caught, including the original defect, leaking the child, leaving a zombie, losing the exit code, terminating a healthy child on the normal path, an unbounded wait, and not releasing the handle. Verified: 824 passed / 39 skipped / 16 xfailed (only the 2 known xdist timing flakes fail; they pass in isolation); ruff, mypy and black clean. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Two small findings from the five-agent sweep, plus three follow-ups a test audit
raised on the first attempt at them.
shutil.chown raises LookupError for an unresolvable group name, and LookupError is
neither OSError nor ValueError. Every handler in this package around file
materialization or session setup catches some combination of
OSError/ValueError/RuntimeError, so an unknown group escaped all of them and
surfaced as a bare LookupError out of the public Session API. PosixSessionUser
does not validate its group, so a caller only has to mis-type one.
New chown_group() translates it at the two call sites -- write_file_for_user and
TempDir.__init__ -- rather than widening six handlers. A group that cannot be
resolved is a failure to change ownership, callers already treat that as OSError,
and this way a handler added later is covered without anyone remembering to.
find_child_process_id_pgrep raised FindSignalTargetError for any non-zero pgrep
exit. Exit 1 means "no processes matched", which is the expected answer for most
of the caller's retry window: sudo has forked but the kernel has not finished
creating the workload. Measured on macOS, the first scan at ~25ms returned no
match for a child that appeared at ~300ms. Exit 1 now returns None, which is what
the procfs implementation of the same lookup already does; other non-zero exits
still raise, now naming the exit code and pgrep's own output (stderr is merged
into stdout, so the message was the only place the reason could survive).
Three follow-ups from the test audit of the above, each a real gap rather than a
polish item:
- find_sudo_child_process_group_id had its `except FindSignalTargetError`
OUTSIDE the retry `while`, so fixing the pgrep exit code only moved the
problem: one bad scan still ended the whole search. Everything the loop races
is transient -- the procfs scan sees more than one child while sudo's fork
settles, and a pgrep invocation can fail without recurring. The scan is now
guarded inside the loop and the error remembered, so a timeout can say what
kept going wrong.
- TempDir.__init__ leaked its mkdtemp directory when the ownership step failed.
Construction had raised, so the caller had no handle to clean up with, and the
directory sits under a shared root nothing prunes. The chown fix makes that
path much easier to reach -- a typo'd group, not just a permissions problem --
so the ownership block is now wrapped with a best-effort rmtree that re-raises
the original error rather than replacing it.
- Two lines of my own were inert and are gone: a `sudo_child_pid = None` in the
new except (the scan only runs when it is already falsy) and an
`or ''` guard on pgrep's stdout (run(stdout=PIPE, text=True) never yields
None). Both survived their mutants, which was the correct signal.
Tests: new test/openjd/sessions_v0/test_linux_sudo.py, plus additions to
test_embedded_files.py, test_tempdir.py and conftest.py. Notable coverage the
audit added beyond the obvious: both the procfs and pgrep branches of the retry
fix, the pre-existing process-group races that had to survive it (a child that
exits mid-scan, a child still sharing sudo's group), that TempDir's cleanup does
not mask the error it cleans up after, and the Windows half of the TempDir leak,
which had nothing.
Mutation-checked: 15 mutants, 15 caught, restores checksum-verified. Two further
mutants were run separately and are expected to survive -- they remove the inert
lines described above -- and are kept out of the gated spec so its "0 survived"
stays meaningful.
Verified: ruff, mypy and black clean; the touched suites 58 passed / 13 skipped /
6 xfailed.
Not fixed here, and now written up in
SuperDaveDocs/pr-reviews/sessions-333/40-deferred-hard-findings.md: the
regression test for a954917 is itself flaky (fails ~1 run in 4 in isolation, for a
reason in the test rather than in production). It gets its own commit.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
The suite had two persistent false reds. Both were test defects, and neither was
mysterious once actually investigated rather than re-run.
test_run_action_default_timeout asserted a one-second window on a child that
prints one line per second:
assert f"Log from test {T - 1}" in messages
assert f"Log from test {T + 1}" not in messages
The runtime-limit Timer is armed in `_run` BEFORE the child is submitted to the
pool, so the Timer's clock starts before the child interpreter even launches. For
the 2s case the child had to reach its second line within 2s of arming, which made
the assertion a measure of interpreter startup and scheduler latency. Measured:
injecting a 1.5s startup delay -- well under the 2s timeout -- fails the old
assertion with the runner correctly in TIMEOUT. Under `-n auto` a loaded host
supplies that delay for free.
It is split into the two things it was conflating:
- test_run_action_effective_timeout asserts the time limit `_run_action` hands
to `_run`. No subprocess, no wall clock. Five cases, including two the old
end-to-end form could not afford: an action timeout SMALLER than the default,
and an action timeout with no default at all.
- test_run_action_timeout_is_enforced keeps the end-to-end check, deliberately
loose: the terminal state, and whether the child reached its last line. It
says nothing about which second it got to, because that is the part that was
never this test's business.
Mutation-checked, 3 mutants, 3 caught: ignoring the action's own timeout, ignoring
the caller's default, and never passing the limit to `_run`. The replacement pins
strictly more than the flaky version did -- the old test could not distinguish
"the default was ignored" from "the host was slow".
test_cancel_during_launch_is_not_dropped failed about 1 run in 4 in isolation, and
this one is my own doing. Commit ec29eeb changed `_run` to publish `_process` and
`_run_future` together AFTER `_pool.submit`, which was right for the `state`
property but widened the window in which `_process` is still None. The test's
`assert runner._process is not None` precondition then fired on the canceller
thread, killing it, so `canceller_done` was never set, `_start_after_cancel` waited
out its full 30s, and the assertion reported SUCCESS -- a failure a long way from
its cause.
The precondition is wrong now rather than merely unlucky: both windows (before
_process is published, and after it exists but before it has started) must record
the cancel rather than drop it, and the assertions after the run already check
exactly that. Precondition removed, and the canceller wrapped in try/finally so a
future failure there cannot masquerade as a product failure.
The product behaviour is unchanged by this commit.
Verified: three consecutive full-suite runs, 852 passed / 0 failed each. First
green run of the suite in this campaign. ruff, mypy and black clean.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
A loaded 108-second suite run failed 14 tests at once -- every cancel/terminate
test in the suite -- while the product was behaving correctly. All 14 pass
serially. One log line makes the mechanism plain:
Canceling subprocess 69379 via termination method
Log from test 0 ... Log from test 19 # child ran the full 20s, exit 0
These tests start a real child, cancel or time it out, and assert on the outcome.
The assertions are right, but they assume the child and the runtime get scheduled
promptly. Under `-n auto` with twelve workers each also sleeping on a child, that
assumption fails. The failure is indistinguishable from a real cancel regression,
which is the expensive part: it teaches you to re-run rather than to read.
Adds a `serial_process` mark (a `pytest.mark.xdist_group`) in conftest, applied to
the tests that race the wall clock. Every such test lands on one xdist worker, so
they run serially with respect to each other while the other ~800 tests still run
in parallel. Requires `--dist=loadgroup`, added to addopts; under the default
`--dist=load` the marker is accepted, ignored, and reported nowhere.
Adds an autouse `_quiesce_after_process_test` fixture, keyed on that mark, which
cancels stray `threading.Timer`s and waits briefly for the thread count to settle.
Serialising only helps if the tests also stop overlapping in the background: a
`ScriptRunnerBase` leaves a timer running for a whole unexpired timeout or grace
period, plus a pool worker, and a test that finishes early by cancelling its child
hands both to whatever runs next.
Scoping, after measuring rather than assuming. Marking the two big classes
wholesale put ~60 process tests on one worker and made it the critical path: the
suite went 40s -> 94s. The mark is therefore on the 13 specific tests that
actually flaked, not on `TestScriptRunnerBase` or
`TestLoggingSubprocessSameUser` entire. The quiesce budget is 1s, not 5s: some
tests legitimately leave a daemon stdout-reader thread that never exits, and a
generous budget was being spent in full on every one of them -- measured at 5s of
teardown for a single test.
Also removes the suite's slowest test. `test_run_action_default_timeout`'s
no-timeout case ran a 20-second child to completion, at 21.3s the slowest test by
a factor of three and the entire critical path. Split into
test_run_action_timeout_terminates_the_action (unchanged intent) and
test_run_action_without_timeout_runs_to_completion, which uses a child that exits
after half a second -- what is being asserted is that no timer cut the action
short, and a child that exits on its own shows that just as well.
Adds test_conftest_serial_process.py so this mechanism cannot regress silently.
Note its first version asserted `getoption("dist") == "loadgroup"` and failed:
inside an xdist worker that option is `"no"`, because a worker runs its share
serially and only the controller distributes. It now reads `addopts`.
No product code changes.
Verified: three consecutive full runs, 855 passed / 0 failed, in 44.2s / 41.5s /
40.7s -- the pre-change baseline was 40s with intermittent 14-failure runs. ruff,
mypy and black clean.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
test_round5_fixes.py and test_round5_regressions.py were named after the review round that produced them, which tells a later reader nothing, and each mixed tests for five different modules. Split by module under test: test_action_filter_hardening.py redaction + consumer-callback containment test_tempdir_hardening.py shared temp root validation + cleanup test_generated_shell_script.py POSIX action script quoting test_optimized_mode_invariants.py invariants that must survive python -O test_subprocess_process_group.py process-group recording test_runner_launch_state.py runner state after a failed launch Class names lose their finding IDs for the behaviour they pin, e.g. TestR51RedactionArgsBypass -> TestRedactionDoesNotLeakViaRecordArgs. Test bodies are unchanged: 76 tests before, 76 after, all passing. Full suite still 855 passed / 39 skipped / 16 xfailed. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
The `Run Linting` step failed on every Windows matrix job, which fail-fast then cancelled the rest of the matrix. mypy on win32 does not see `os.geteuid`, `os.fchmod`, `os.getpgid` or `signal.SIGKILL`, all of which this PR added inside POSIX-gated code paths. Adds `# type: ignore` to the 11 sites, matching the convention already in _tempdir.py:257. No behaviour change; `warn_unused_ignores` is false, so the comments are inert on POSIX. Verified `mypy --platform win32 src test` and native mypy both clean, plus ruff, black, and 855 passed / 39 skipped / 16 xfailed. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
The R5-3 temp-root hardening validated `<tempdir>/OpenJD` through a single descriptor, and its docstring claimed the `getattr(os, "O_NOFOLLOW", 0)` fallbacks degraded that to "a plain open" on Windows. That claim was wrong and untested: Windows returns EACCES for `os.open()` on a directory whatever the flags are, so `custom_gettempdir()` raised on every call and every Windows test that builds a Session failed with RuntimeError: Refusing to use temporary directory C:\ProgramData\Amazon\ OpenJD: it could not be opened as a real directory ([Errno 13] Permission denied) Splits the validator by platform. POSIX keeps the descriptor, which is what closes the symlink-swap window, and now uses the flags unconditionally instead of pretending to be portable. Windows validates with `os.lstat`, which does not traverse a symlink or junction, and sets no mode -- the mode is a POSIX mode and Windows access is governed by the ACLs inherited from %PROGRAMDATA%. The docstring says plainly that the Windows check is the weaker of the two. Tests: six new tests, all runnable on POSIX so a POSIX CI host catches a Windows-only regression. `test_a_non_posix_platform_never_opens_a_descriptor` pins the dispatch by patching `is_posix`, with `test_a_posix_platform_still_uses_a_descriptor` as the negative control so a mutation routing everything to the weaker validator also fails. Also fixes two tests that patched only `gettempdir` and so silently operated on the real %PROGRAMDATA%\Amazon\OpenJD when run on Windows. 4/4 mutants caught. 861 passed / 39 skipped / 16 xfailed; ruff, black, and mypy clean on both native and --platform win32. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
test_a_posix_platform_still_uses_a_descriptor drives the POSIX validator directly by patching is_posix, and that branch names os.O_NOFOLLOW and os.O_DIRECTORY. Those are absent on some Windows builds -- the Windows 3.13 job failed with AttributeError while 3.14 passed -- so the test is now POSIX-only. It is a control for a mutation that is only ever evaluated on a POSIX host, so nothing is lost. All five temp-root mutants still caught. Also deletes the `# ==== / # R5-x -- ... / # ====` section banners the split carried into the wrong files: every one of them announced content that is not in the file it now sits in, e.g. test_runner_launch_state.py opened with "R5-1 -- redaction must not leave record.args populated". 861 passed / 39 skipped / 16 xfailed; ruff, black, and mypy clean on native and --platform win32. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Five small findings, no behaviour change.
_session.py: the `if step_name is None: raise` inside run_task's wrap branch
is unreachable, as CodeQL reported. run_task already rejects that combination
at method entry under the same `wrap_env is not None` condition, so the second
check only narrowed the type -- and its comment claimed the opposite ("a
cross-field invariant, not type-checker narrowing"). Replaced with a single
`cast(str, step_name)` bound where the requirement has been proven. This is
the third unfalsifiable guard of mine to come out of this branch.
_subprocess.py: dropped the `del proc` in run()'s finally, which CodeQL
flagged as an unnecessary delete. `proc` is a function local and dies with the
frame. It is pre-existing on mainline, but 44ff172 wrapped it in a try/finally
that existed only to hold it, so the whole construct is gone and the comment
now describes what actually matters -- clearing `self._process`.
test_tempdir_hardening.py: `lowest_free_fd()` opened a descriptor outside a
try/finally. A `with` block cannot be used because the descriptor number *is*
the measurement, so it has to be closed before being returned; try/finally
does that on every path. Closes three "file is not always closed" alerts.
test_linux_sudo.py, test_subprocess_reaping.py: the two bare `except: pass`
teardown handlers now say why swallowing is correct, which is what CodeQL's
"empty except" rule asks for.
861 passed / 39 skipped / 16 xfailed; ruff, black and mypy clean on native and
--platform win32.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
412 lines of unreferenced copy. Nothing in src/ or test/ imports it: the only `._linux._*` imports in the package are in _subprocess.py:16-17, and because that module sits at openjd/sessions/, they resolve to the live _linux/. No module under _v1/ imports `._linux` at all, so the relative-import route that would have reached this copy does not exist either. It is not merely dead, it is dangerously stale. The dead _sudo.py still carries the unguarded `os.getpgid(sudo_process.pid)` that d29aafe fixed in the live copy -- 82 differing lines between the two. A reader grepping for find_sudo_child_process_group_id got two hits and no signal about which one runs. Pre-existing (arrived on mainline via OpenJobDescription#316) and untouched by this branch, but this branch widened the gap by fixing only the live copy, which is what makes it worth removing here. No test changes: 861 passed / 39 skipped / 16 xfailed, unchanged. mypy now covers 88 source files instead of 91. ruff, black and mypy clean on native and --platform win32. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Two contained simplifications in the cancel/filter paths. No behaviour change: the whole suite passes unmodified. R13 -- _runner_base.ScriptRunnerBase._signal_process(process, method). The "send a signal, warn on OSError" block was written four times. Three were identical modulo the verb (_cancel's terminate arm, _cancel's notify arm, and _on_notify_period_end's terminate); the fourth, the cancel_info.json write fallback, carries a different message and is left alone. `process` is a parameter rather than a read of self._process so _on_notify_period_end keeps passing the lock-free local snapshot it already holds instead of re-reading an attribute another thread may have cleared. The verb is a Literal["terminate", "notify"] with an explicit branch rather than a getattr, so the dispatch stays type-checked. _cancel: 133 -> 114 lines. R5 -- _action_filter.py: removed the unreachable `except json.JSONDecodeError`. The only json.loads call sits inside an inner handler that re-raises as ValueError, so the outer arm could never fire; the whole outer try/except goes with it. Also binds envvar_set_matcher_str.match(message) once instead of evaluating it at both :513 and :521 -- the same match now decides validity and selects the parse branch. Verified: 861 passed / 39 skipped / 16 xfailed, unchanged. ruff, black and mypy clean on native and --platform win32. R13's dispatch is mutation-checked: 3 mutants (dispatch inverted, notify never sent, terminate never sent), 3 caught. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
mwiebe
approved these changes
Jul 27, 2026
mwiebe
left a comment
mwiebe
left a comment
Contributor
There was a problem hiding this comment.
Generally came through clean, found an issue worth fixing in a follow-up
| extra=LogExtraInfo(openjd_log_content=LogContent.PROCESS_CONTROL), | ||
| ) | ||
|
|
||
| self._logger.info( |
Contributor
There was a problem hiding this comment.
Testing found this correctness error:
Child processes leak when startup logging raises. openjd-sessions-for-python/src/openjd/sessions/_subprocess.py:227 logs twice before entering the new reaping try/finally at line 236. A logging filter raising on "Command started as pid" left the child attached and running. Move the ownership try/finally to immediately after successful process creation.
leongdl
enabled auto-merge (squash)
July 28, 2026 20:19
wyongzhi
approved these changes
Jul 28, 2026
Merged
leongdl
added a commit
to leongdl/openjd-sessions-for-python
that referenced
this pull request
Jul 29, 2026
`LoggingSubprocess.run()` logged "Command started as pid" and "Output:", and did the POSIX process-group lookup, *before* entering the `try` whose `finally` reaps the child. Anything raising in that window escaped `run()` having never waited on the child: a live process, no exit code recorded, and the runner publishing a terminal state for it anyway. The reachable trigger is a `logging.Filter` that raises. Filters propagate out of `Logger.handle`, unlike handlers, whose exceptions are routed to `handleError` -- and installing a filter is a supported use of this library; `Session` installs its own. Reproduced before the fix: the child stayed alive with `exit_code is None`, while a filter raising from inside the pump (already covered by the `finally`) reaped correctly. The ownership `try` now begins immediately after successful process creation, so every exit path from that point goes through `_reap`. The body binds `proc = self._process` once instead of re-reading the attribute, matching the bind-once convention used by `notify()`/`terminate()`. `_reap`'s own logging is now best-effort. It runs inside `run()`'s `finally`, so anything escaping it replaces the exception already propagating and hides the real cause. That containment was in place for the terminate and the wait but not for the three `logger.error` calls between them -- and a raising logging stack is exactly what routes execution into `_reap`, which makes those calls the likeliest thing to fail there, not the least. Reported by mwiebe on OpenJobDescription#333: OpenJobDescription#333 (comment) Two honest notes on the scope of the fix: - Reordering `_reap` to terminate before logging was tried and dropped. With best-effort logging in place the swallow already lets the terminate run, so the ordering changed nothing an `Exception` could observe; it only guarded a `BaseException` from the logging stack, which cannot reach the pool worker thread `run()` executes on. Mutation testing proved it inert, so it is not shipped rather than shipped behind a contrived test. - When the process-group lookup is *itself* what fails, the boundary buys the reap attempt and the handle release but not the kill: with no group recorded, `_posix_signal_subprocess` declines to signal rather than target a possibly recycled pid (the R5-9 convention). That is a pre-existing limitation of the signalling path, not something this change introduces, and the test for that path asserts only what actually happens. Recorded as a follow-up. Tests: 6 added. Every one mutation-checked -- 5 mutants, 0 survivors: the startup logging moved back outside the boundary, the process-group lookup moved back outside it, and each of the three reap log sites bypassing best-effort. The pre-existing 13 tests in the file pass unmodified. Full suite 884 passed; ruff, black, and mypy clean on native and --platform win32. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
leongdl
added a commit
that referenced
this pull request
Jul 30, 2026
`LoggingSubprocess.run()` logged "Command started as pid" and "Output:", and did the POSIX process-group lookup, *before* entering the `try` whose `finally` reaps the child. Anything raising in that window escaped `run()` having never waited on the child: a live process, no exit code recorded, and the runner publishing a terminal state for it anyway. The reachable trigger is a `logging.Filter` that raises. Filters propagate out of `Logger.handle`, unlike handlers, whose exceptions are routed to `handleError` -- and installing a filter is a supported use of this library; `Session` installs its own. Reproduced before the fix: the child stayed alive with `exit_code is None`, while a filter raising from inside the pump (already covered by the `finally`) reaped correctly. The ownership `try` now begins immediately after successful process creation, so every exit path from that point goes through `_reap`. The body binds `proc = self._process` once instead of re-reading the attribute, matching the bind-once convention used by `notify()`/`terminate()`. `_reap`'s own logging is now best-effort. It runs inside `run()`'s `finally`, so anything escaping it replaces the exception already propagating and hides the real cause. That containment was in place for the terminate and the wait but not for the three `logger.error` calls between them -- and a raising logging stack is exactly what routes execution into `_reap`, which makes those calls the likeliest thing to fail there, not the least. Reported by mwiebe on #333: #333 (comment) Two honest notes on the scope of the fix: - Reordering `_reap` to terminate before logging was tried and dropped. With best-effort logging in place the swallow already lets the terminate run, so the ordering changed nothing an `Exception` could observe; it only guarded a `BaseException` from the logging stack, which cannot reach the pool worker thread `run()` executes on. Mutation testing proved it inert, so it is not shipped rather than shipped behind a contrived test. - When the process-group lookup is *itself* what fails, the boundary buys the reap attempt and the handle release but not the kill: with no group recorded, `_posix_signal_subprocess` declines to signal rather than target a possibly recycled pid (the R5-9 convention). That is a pre-existing limitation of the signalling path, not something this change introduces, and the test for that path asserts only what actually happens. Recorded as a follow-up. Tests: 6 added. Every one mutation-checked -- 5 mutants, 0 survivors: the startup logging moved back outside the boundary, the process-group lookup moved back outside it, and each of the three reap log sites bypassing best-effort. The pre-existing 13 tests in the file pass unmodified. Full suite 884 passed; ruff, black, and mypy clean on native and --platform win32. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> (cherry picked from commit acee655)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
This PR teaches the Python v0 session runtime the RFC 0008 "wrap actions" extension: an environment that defines onWrapEnvEnter / onWrapTaskRun / onWrapEnvExit intercepts inner environments' enter/exit actions and tasks' run actions, running its own hook instead — with the wrapped action's fully-resolved command, args, environment variables, timeout, and cancelation config injected as WrappedAction.* template variables. To do that faithfully it also lands the EXPR (RFC 0007) runtime machinery the feature rides on: script-level let bindings evaluated in the same order as openjd-rs, typed symbol tables, and cancel-time resolution of format-string cancelation modes. The openjd-rs crate is the reference throughout, and the Python code mirrors it closely — including the two-scope separation that keeps a wrapper's names from ever leaking into the wrapped action's resolved values. What deserves the most scrutiny: the three wrap-interception branches in _session.py, and the two small Rust-parity gaps found in this pass.
What was the problem/requirement? (What/Why)
RFC 0008 (
WRAP_ACTIONS) lets a job author write a "wrapper" environment whose hook scripts (onWrapEnvEnter,onWrapTaskRun,onWrapEnvExit) run in place of an inner environment'sonEnter/onExitor a step'sonRun— the classic use case is running every task inside a container. The v0 Python session runtime had no support for wrap hooks, and needed EXPR (RFC 0007) runtime parity with openjd-rs for the pieces the hooks depend on (typed symbol tables, script-levelletbindings, embedded-file ordering).What was the solution? (How)
One squashed commit implementing:
WrappedAction.Command/Args/Environment/Timeout/Cancelation.*andWrappedEnv.Name/WrappedStep.Nameinjected into the hook's symbol table. At most one wrap-defining environment may be active (enforced at enter time).letbindings and embedded files, materialized in runner order (paths → lets → contents) — while the hook script resolves against the wrap environment's own scope plus theWrappedAction.*overlay. Aletname bound by both sides resolves to each side's own value (regression-tested). Wrapped actions referencingTask.File.*/Env.File.*resolve to real on-disk paths.WrappedAction.Environmentcarries every session-defined variable:openjd_envdefinitions and entered environments' declarativevariables:maps (host-inherited variables excluded), matching openjd-rs chore(deps): update ruff requirement from ==0.13.* to ==0.14.* #277 and the cross-platform fixtures in openjd-specifications.letbindings ordered around embedded-file materialization, typed symbol tables, andenter_environment(extra_let_bindings=...)(optional, defaultNone— fully backwards compatible) so a step's environments see the step-level lets.WrappedAction.Cancelation.Mode/NotifyPeriodInSecondsresolved through the same enforcement path the runtime uses, with the Template Schemas 5.3.2 defaults (120 s for a task'sonRun, 30 s otherwise).What is the impact of this change?
Wrapper environments behave per RFC 0008 in the v0 Python session runtime, with scope isolation guarantees identical to openjd-rs. The public API is backwards compatible: the only signature change is the new optional
extra_let_bindingskeyword onenter_environment, defaulting toNone.How was this change tested?
hatch run teston macOS; the only intermittent failures are two pre-existing xdist timing flakes intest_run_action_default_timeout/test_cancel_notifythat pass in isolation and also fail identically on mainline).mypy(hatch run typing) andblack/ruff(hatch run style): clean.letname on both sides; innerletinvisible to the hook), inner embedded-file resolution throughWrappedAction.Command,variables:-map inclusion inWrappedAction.Environment, and cancelation-mode/notify-period injection.python-rsbranch installed): WRAP_ACTIONS 68/72 — the 4 remaining failures are the wrap-cancelation fixtures that require openjd-model-for-python chore(release): 0.10.9 #313 to land in the model first.Depends on: openjd-model-for-python
python-rsbranch (unreleased model APIs). Merge after the model change.Review fixes (second commit)
fix: Address review findings — let-binding crash paths, Step.Name, perf, from a multi-aspect review (crashes / performance / maintainability) with a post-fix clean sweep:enter_environment/exit_environmentno longer raise a rawExpressionErrorout of the public API when an extraletbinding fails — the action fails through the normal callback path, leaving the environment entered-but-failed like a failed onEnter subprocess.enter_environment(step_name=...)seedsStep.Name(RFC 0007 §7.3.1) before the extra bindings evaluate, remembered per environment and re-seeded at exit — the v0 counterpart of the per-step resolved symtab the Rust runtime threads into enter/exit.openjd.model.evaluate_let_bindings; wrap-environment embedded-file paths allocated once per environment and reused across wrap-hook invocations (stableEnv.File.*, no O(task-count) temp-file growth) while contents are still re-resolved per task.resolve_optional_int_field(ge=, le=)replaces three disagreeing optional-int resolvers — this also adds the previously missing non-positive check toSession._resolve_action_timeout(openjd-rs parity)._fail_action, shared runnercancel(),_make_env_script_runner,_try_inject_wrapped_symbols,WRAP_HOOK_ACTION_NAMESsingle-sourced,effective_items()accessor, whole-field detection via the model's parsedwhole_field_expression().Tests after fixes: 647 passed (12 new regression tests); pre-existing environment failures unchanged; mypy/black/ruff clean. Note for release: the
openjd-modelpin floor must be the first model release containing openjd-model-for-python#318's surface (comment added above the pin).