feat(sessions): Rust-backed openjd.sessions._v1 via PyO3 - #316
Merged
Conversation
| status_message=status.status_message, | ||
| ) | ||
| self._callback(self._session_id, running_status) | ||
| reported_running = True |
| # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
|
|
||
| import sys | ||
| import ctypes |
| # Note: We must ensure that 'handles_list' must persist until the | ||
| # attribute list is destroyed using DeleteProcThreadAttributeList. We do this by holding on | ||
| # to a reference to it until after the finally block of this try. | ||
| handles_list = inherit_handles( # noqa: F841 # ignore: assigned but not used |
Contributor
Author
Architecture summary (for reviewers)
Bird's eyeThis branch introduces 86 files changed, +4122 / −0 (purely additive — no v0 deletions). Commits
Architecturev0 → v1 API delta
Open questions / things to flag
Tests
Reviewer prerequisitesTo run the test suite locally, the |
seant-aws
force-pushed
the
bindings-rs
branch
2 times, most recently
from
June 23, 2026 21:21
1e92e69 to
201aeac
Compare
| command: Path, os_env_vars: Optional[dict[str, Optional[str]]], working_dir: str | ||
| ) -> str: | ||
| path_var: Optional[str] = _get_path_var_for_shutil_which(os_env_vars, working_dir) | ||
| exe = str(shutil.which(str(command), path=path_var)) |
There was a problem hiding this comment.
shutil.which() returns None when the command is not found, so str(shutil.which(...)) produces the literal string "None". That string is truthy, so the if not exe: guard on the next line never triggers for the not-found case — instead this function returns "None" as the resolved executable path, and the failure surfaces later as a confusing "cannot find/execute 'None'" error rather than the intended RuntimeError("Could not find executable file: ...").
Suggested fix: check the result before stringifying.
exe = shutil.which(str(command), path=path_var)
if not exe:
raise RuntimeError("Could not find executable file: %s" % command)
return exe| yield False | ||
| return | ||
|
|
||
| caps = libcap.cap_get_proc() |
There was a problem hiding this comment.
cap_get_proc() returns a heap-allocated cap_t that the caller owns and must release with cap_free() (see the cap_get_proc(3) man page). This code never calls cap_free(caps) on any of its exit paths, so every invocation of try_use_cap_kill() leaks the capability state object. Since this context manager is entered on each action cancellation, the leak accumulates over the lifetime of a long-running session host.
Consider binding libcap.cap_free and freeing caps in a finally (and note that cap_set_flag/cap_set_proc were registered without an errcheck, so failures there are silently ignored — unlike the get-path functions).
| text=True, | ||
| ) | ||
| if pgrep_result.returncode != 0: | ||
| raise FindSignalTargetError("Unable to query child processes of sudo process") |
There was a problem hiding this comment.
pgrep exits with status 1 specifically to mean "no processes matched" (only status >1 indicates an actual error). On the non-Linux/macOS path this is the normal case while sudo has not yet forked its child — exactly the situation find_sudo_child_process_group_id's retry loop is designed to poll through.
But because this raises FindSignalTargetError on any non-zero return, and the caller wraps the entire while loop in a single try/except FindSignalTargetError, the first poll iteration where the child does not yet exist raises, the exception propagates out of the loop, and the retry never happens. The result is that on macOS the signal target frequently can't be found even though a short wait would have succeeded.
Suggest distinguishing the two cases:
if pgrep_result.returncode == 1:
return None # no child yet — let the caller retry
if pgrep_result.returncode != 0:
raise FindSignalTargetError("Unable to query child processes of sudo process")| return [] | ||
|
|
||
| def __del__(self): | ||
| self.cleanup() |
There was a problem hiding this comment.
If the _RustSession(...) constructor in __init__ raises (e.g. bad credentials, invalid session_root_directory), the Session object exists but self._rust_session was never assigned. When that half-constructed object is garbage-collected, __del__ → cleanup() → self._rust_session.cleanup() will raise AttributeError, masking the original construction error with a noisy error from __del__.
Consider guarding cleanup, e.g. getattr(self, "_rust_session", None) before calling .cleanup(), or setting self._rust_session = None at the top of __init__.
leongdl
reviewed
Jun 25, 2026
| @@ -0,0 +1,258 @@ | |||
| # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
|
|
|||
| """This module contains code for interacting with Linux capabilities. The module uses the ctypes | |||
Contributor
There was a problem hiding this comment.
We should rebase, and get this PR merged soon since the OpenJD side is merged.
Contributor
There was a problem hiding this comment.
Pending the release of OpenJD-model once it is released.
jericht
force-pushed
the
bindings-rs
branch
4 times, most recently
from
June 26, 2026 22:32
198556a to
aa7f038
Compare
| stdin=DEVNULL, | ||
| text=True, | ||
| ) | ||
| if pgrep_result.returncode != 0: |
There was a problem hiding this comment.
pgrep -P <pid> exits with code 1 when no child processes match (not just on error). Here any non-zero return code raises FindSignalTargetError, which propagates out of the retry loop in find_sudo_child_process_group_id (caught by the outer except, logged as a warning, and the function returns None).
On non-Linux POSIX hosts (macOS) the retry loop exists precisely because sudo's child may not exist yet on the first scan. But the very first pgrep before the child is spawned returns exit 1 → raises → the loop is abandoned on the first iteration and the signal target is never found. To preserve the intended retry-until-timeout behavior, treat "no match" (returncode 1 with empty stdout) as return None and only raise on genuine errors (returncode >= 2).
| yield False | ||
| return | ||
|
|
||
| caps = libcap.cap_get_proc() |
There was a problem hiding this comment.
cap_get_proc() allocates a cap_t on the heap that must be released with cap_free() (per the libcap man page: the caller should free releasable memory with cap_free()). Here caps is never freed in any of the three branches that follow, so every call to try_use_cap_kill() leaks one capability-state object. Since this context manager runs on each action cancel/signal, the leak accumulates over the lifetime of a long-running session.
Consider declaring libcap.cap_free (argtype cap_t, restype c_int) in _get_libcap() and calling it on caps from a finally that wraps the body below.
| [tool.coverage.report] | ||
| show_missing = true | ||
| fail_under = 79 | ||
| fail_under = 50 |
There was a problem hiding this comment.
The coverage gate is being lowered from 79% to 50%. This is a substantial regression in the enforced quality bar for the package as a whole. Given that most of the new _v1 code (e.g. the Win32 ctypes layer, _capabilities.py) is thin OS-binding glue that is hard to unit test, this may be intentional — but it also means the new pure-Python logic (_session.py polling/callback state machine, _sudo.py child-discovery) can land largely uncovered. Worth confirming this drop is a deliberate, temporary concession rather than masking untested new logic.
Add a new `openjd.sessions._v1` package that wraps the Rust
`openjd_sessions` crate (in the sibling `openjd-rs` workspace),
alongside the existing pure-Python v0 implementation under
`openjd.sessions`. The v1 namespace is a thin Python shell over
the Rust runtime: format-string evaluation, action dispatch,
process control, environment lifecycle, path mapping, and logging
all happen inside Rust; the Python layer wires up callbacks,
holds the SessionUser objects, and provides the platform-specific
helpers that need to call into native APIs.
Layout
======
The package coexists with the legacy v0 implementation (which
remains functional and untouched):
src/openjd/sessions/
├── _v1/ # Rust-backed v1 (new)
│ ├── __init__.py # public surface re-exports
│ ├── _session.py # Session class — thin wrapper over Rust
│ ├── _session_user.py # PosixSessionUser / WindowsSessionUser
│ ├── _types.py # type aliases for the model._v1.job types
│ │ # used here as Action, EmbeddedFile,
│ │ # Environment, EnvironmentScript, StepScript
│ ├── _logging.py # Python ↔ Rust logging bridge
│ ├── _os_checker.py # cross-platform helpers
│ ├── _path_mapping.py # PathMappingRule re-export
│ ├── _linux/ # Linux-specific helpers (sudo, capabilities)
│ │ ├── _capabilities.py # POSIX capabilities query
│ │ └── _sudo.py # sudo subprocess support
│ ├── _win32/ # Windows-specific helpers
│ │ ├── _api.py # Win32 API bindings
│ │ ├── _helpers.py # convenience wrappers
│ │ ├── _locate_executable.py
│ │ └── _popen_as_user.py # CreateProcessAsUser / CreateProcessWithLogonW
│ └── _scripts/
│ ├── _posix/_signal_subprocess.sh
│ └── _windows/_signal_win_subprocess.py
├── (existing v0 modules — _session.py, _types.py, etc. — are unchanged)
└── ...
Public surface
==============
from openjd.sessions._v1 import (
Session, # Rust pyclass
SessionState, # enum: Ready, Running, Canceling, …
ActionState, ActionStatus, ActionResult,
ScriptRunnerState,
PosixSessionUser, WindowsSessionUser,
SessionError, BadCredentialsException,
PathMappingRule, PathFormat,
FormatString, # re-exported from openjd.expr
)
s = Session(
session_id="...",
job_parameter_values={...},
path_mapping_rules=[...],
session_user=PosixSessionUser("runner", "renderers"),
on_state_change=callback,
on_action_change=callback,
)
s.enter_environment(environment_id, environment, ...)
s.run_task(step, task_parameter_values, ...)
s.cancel_action()
s.exit_environment(environment_id)
s.cleanup()
Implementation summary
======================
1. `Session` is a thin wrapper around the Rust
`openjd._openjd_rs.Session` pyclass. The Python class
manages: callback registration, the in-flight action threading
model, post-completion cleanup, and `__enter__` /
`__exit__` / `cleanup()` semantics. State changes and action
status updates are forwarded synchronously to user callbacks
on the worker thread that observes them — including the
initial RUNNING transition (fixed during this work).
2. The session dispatches actions through
`openjd._openjd_rs.Session` methods. Format-string evaluation,
parameter binding, working-directory setup, embedded-file
materialization, script execution, signal forwarding, timeouts,
and child-process tracking all happen inside the Rust crate;
the Python layer is purely glue.
3. Logging: the v1 layer installs a Python ↔ Rust logging bridge
so messages emitted from Rust (`log::info!`, etc.) appear
under the standard Python `openjd.sessions` logger
hierarchy. Logger name is `openjd.sessions` (not
`openjd.sessions.v1`) so v0 and v1 share the same log
namespace.
4. Session users are split per platform:
- `PosixSessionUser(user, group)` is a thin wrapper around the
Rust pyclass. Replaces the v0 Python `PosixSessionUser` for
v1 callers.
- `WindowsSessionUser` is exposed for v1 callers but the
full Win32 implementation remains in Python (the Rust crate
covers the cross-user invocation primitives but some
Win32-specific surface — like locating executables on PATH
under a different user, or capturing redirected stdout — is
still handled here).
5. Linux helpers under `_v1/_linux/`: `_capabilities.py` queries
POSIX capabilities so callers can decide whether sudo is
necessary; `_sudo.py` supplies the subprocess-spawning helper
the Rust runtime hands out to the Python layer when
PosixSessionUser is configured.
6. Windows helpers under `_v1/_win32/`: `_api.py` and
`_helpers.py` expose the Win32 API surface needed for
user-impersonating subprocess creation; `_locate_executable.py`
resolves PATH lookups under a non-default user;
`_popen_as_user.py` mirrors Python's `subprocess.Popen` API
for child processes spawned via CreateProcessAsUser /
CreateProcessWithLogonW.
7. The `openjd.sessions._v1` package consumes
`openjd.model._v1` types directly. The v1 commit in
`openjd-model-for-python` split that namespace into
submodules (`template`/`job`/`types`/`errors`); the
structural types we need (`Action`, `EmbeddedFile`,
`Environment`, `EnvironmentScript`, `StepScript`) come from
`openjd.model._v1.job`. Type aliases for them are exposed
under their `_2023_09` suffix as `Action_2023_09`,
`EmbeddedFileText_2023_09`, etc., for callers that need to be
explicit about the schema revision.
Tests
=====
`test/openjd/sessions-v1/` is a new package with:
- `test_session_scenarios.py` — full session lifecycle scenarios
driven from YAML scenario+template pairs under
`scenarios/`. Covers EXPR-extension features (let bindings,
host context references), all task parameter types,
environment-file let bindings, path mapping rules, and the
step/env let-binding scope precedence rules.
- `test_pickle.py` — pickle round-trip coverage for v1 value
types (Session is not pickleable, but state enums, status
values, and supporting pyclasses are).
- `test_os_checker.py` — basic platform-check sanity.
- `scenarios/` — six end-to-end scenarios with template +
scenario YAML files exercising the full Python ↔ Rust path.
The test_session_scenarios coverage is comprehensive enough to
substitute for v0's per-feature unit tests for the surface that
v1 exposes. Pre-existing test failures (`utils.windows_acl_helper`
imports, etc.) are unrelated to this work and are documented as
pre-existing in their respective files.
Side-by-side dependency
=======================
This package consumes `openjd.model._v1` from the
`openjd-model-for-python` companion repo (which builds the
`openjd._openjd_rs` extension module). The two repos must move
together: the corresponding openjd-model-for-python PR
("feat: Rust-backed openjd.model._v1, openjd.expr,
openjd.sessions._v1 via PyO3") must land before this PR.
Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>
chore: rename test/openjd/sessions-v{0,1}/ to sessions_v{0,1}/ for pytest+mypy
Mirrors the same rename in the sibling repo openjd-model-for-python
(commit `9f7f55d chore: rename test/openjd/model-v{0,1}/ to
model_v{0,1}/ for mypy`). The pre-existing test directories
`test/openjd/sessions-v0/` and `test/openjd/sessions-v1/` carried
hyphens in their names. Each contained an `__init__.py` (so they
look like Python packages to tools that walk the filesystem). This
broke pytest's import-mode discovery — tests that referenced
sibling modules with relative imports failed with
`ImportError: attempted relative import with no known parent
package`, which cascaded into 16 collection errors and 13 ERROR
test runs. mypy fails the same way for the same reason
(`sessions-v0 is not a valid Python package name`).
This commit:
1. `git mv test/openjd/sessions-v0 test/openjd/sessions_v0` (and
the v1 variant) — 68 file renames in total, no content changes.
2. No textual references in `pyproject.toml`, `hatch.toml`,
docstrings, or source needed updating: unlike
openjd-model-for-python's parallel rename, sessions-for-python
never embedded the path in config or comments.
Verification:
- `hatch run test` collection no longer fails. Test count goes
from 101 passed / 16 ERROR / 13 FAILED on the old layout to
559 passed / 18 FAILED / 33 skipped / 16 xfailed.
- The remaining 18 failures are pre-existing issues unrelated to
this rename:
- 2 sudo-impersonation tests in `test_subprocess.py` need
`OPENJD_TEST_SUDO_*` env vars to actually run (xfail-on-
bare-environment).
- 3 OS-checker tests in `test_os_checker.py` are platform-
gated (`is_windows`/`is_posix` assertions on a Linux runner).
- 13 v1 session-scenario tests fail with
`AttributeError: 'JobParameterType' object has no
attribute 'value'` — a port-from-v0 bug introduced when
`test_session_scenarios.py` was added in
`98656ec feat(sessions): Rust-backed openjd.sessions._v1
via PyO3`. v0's `JobParameterType` is a `str`-Enum (so
`.value` returns the string); v1's pyclass exposes
`.as_str()` instead, but the test wasn't updated.
- mypy will now actually run instead of bailing on the invalid
package name. (Existing `hatch run lint` reports 20 ruff
errors and unrelated lint debt that mypy may reveal further;
separate concern.)
Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>
feat(sessions-v1): add Session context-manager protocol; port scenario tests to v1 enum API
Two follow-ons to the directory rename in `d6f7f69` that
unblock 7 previously-failing v1 session-scenario tests.
**1. Session context manager.** `openjd.sessions._v1.Session` had
`cleanup()` and `__del__` but no `__enter__` / `__exit__`
methods, so `with Session(...) as session:` raised
`TypeError: 'Session' object does not support the context
manager protocol`. The v0 reference `openjd.sessions._session.Session`
implements both as the trivial pattern (`__enter__` returns
`self`, `__exit__` calls `self.cleanup()`); v1 now matches.
**2. Scenario test port-from-v0 cleanup.**
`test/openjd/sessions_v1/test_session_scenarios.py` was added in
`98656ec feat(sessions): Rust-backed openjd.sessions._v1 via PyO3`
by copy-paste from a v0 scaffold without adjusting two
v0-isms that v1's pyclass enums don't support:
* `param_def.type.value` and `session.state.value`. v0's
`JobParameterType` and `SessionState` are `str`-Enums (where
`.value` returns the underlying string). The v1 binding
exposes them as PyO3 pyclass enums without `.value`. The
`build_parameter_values()` helper now passes the
`JobParameterType` variant directly (the upstream constructor
accepts it); the polling loops compare `session.state` against
a module-level `_QUIESCENT_STATES` set of `SessionState`
variants.
* `ParameterValueType(name_str)`. v1's pyclass enum is not
constructible from a string; the helper switched to passing
the variant straight through, dropping the string round-trip.
Test results: `hatch run test` goes from 559 passed / 18 failed
to 566 passed / 11 failed (+7 newly-passing scenario tests, no
regressions). The 11 remaining failures are all real bugs that
need their own fixes:
- 3 platform-gated OS-checker tests (Windows assertions on
Linux runner).
- 2 sudo-impersonation tests needing
`OPENJD_TEST_SUDO_*` env vars.
- 6 scenario tests with output-string assertion mismatches
(real session behaviour or scenario-yaml issues, not
mechanical fixes).
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>
Signed-off-by: Jericho Tolentino <68654047+jericht@users.noreply.github.com>
| from typing import Optional, Sequence | ||
|
|
||
| from .._session_user import SessionUser | ||
| from .._subprocess import LoggingSubprocess |
There was a problem hiding this comment.
This imports from .._subprocess import LoggingSubprocess, but the _v1 package has no _subprocess.py (only the legacy openjd.sessions._subprocess exists). Any attempt to import this module — e.g. from openjd.sessions._v1._win32._locate_executable import locate_windows_executable — will fail with ModuleNotFoundError: No module named 'openjd.sessions._v1._subprocess'.
Nothing in _v1 currently references locate_windows_executable, so this is a latent/dead module today, but the import is broken as written. Either point it at the real module (from openjd.sessions._subprocess import LoggingSubprocess) or drop the file if the Rust runner now handles executable location.
| command: Path, os_env_vars: Optional[dict[str, Optional[str]]], working_dir: str | ||
| ) -> str: | ||
| path_var: Optional[str] = _get_path_var_for_shutil_which(os_env_vars, working_dir) | ||
| exe = str(shutil.which(str(command), path=path_var)) |
There was a problem hiding this comment.
str(shutil.which(...)) stringifies the result before the falsy check. When shutil.which fails to find the executable it returns None, but str(None) is the string "None", which is truthy — so if not exe is never taken, and the function returns the literal string "None" as the executable path instead of raising RuntimeError. The caller then tries to launch "None" as the command.
Check the raw result before stringifying:
exe = shutil.which(str(command), path=path_var)
if not exe:
raise RuntimeError("Could not find executable file: %s" % command)
return exe
seant-aws
marked this pull request as ready for review
June 29, 2026 22:53
leongdl
approved these changes
Jun 29, 2026
jericht
approved these changes
Jun 30, 2026
This was referenced Jul 2, 2026
Closed
Merged
leongdl
added a commit
to leongdl/openjd-sessions-for-python
that referenced
this pull request
Jul 27, 2026
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>
leongdl
added a commit
that referenced
this pull request
Jul 28, 2026
* feat: RFC 0008 environment wrap actions with EXPR runtime parity Implement 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 #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>
* fix: Address review findings — let-binding crash paths, Step.Name, perf
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 #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>
* fix: RFC 0005/0008 typed-arg and null-vs-empty parity with openjd-rs
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>
* fix: Close review findings on failure paths, timeout bounds and typing
Addresses the findings from two independent reviews of this PR, after
validating each against openjd-rs upstream/main and the specification.
Fixes that change behavior:
- An oversized but schema-legal `timeout` no longer raises OverflowError out
of the public Session API. Two limits sit above what we can enforce:
timedelta tops out at 999999999 days, and threading.Timer's deadline
arithmetic overflows just above 2**63 nanoseconds -- so a clamp to
timedelta.max is not enough. A value beyond MAX_SCHEDULABLE_TIMEOUT_SECONDS
now runs the action with no time limit and a warning, matching openjd-rs,
whose Duration-based timer effectively never fires at that magnitude. A
value above u64::MAX fails the action, mirroring openjd-rs's
str::parse::<u64>() -- applied to literal and resolved values alike so that
a value forwarded as {{WrappedAction.Timeout}} behaves as it would have
unwrapped. Reachable from run_task, enter/exit_environment and
run_subprocess; previously left the Session stuck in RUNNING with no
terminal ActionStatus.
- A runtime EXPR failure in an environment's `variables:` map no longer
escapes enter_environment(). It now fails through the callback path with the
identifier returned, so the caller can exit the environment it entered.
Seeding the empty change record is required: the log filter indexes
_created_env_vars without a membership test.
- run_task() now raises ValueError when a wrap environment is active and no
step_name was given, instead of rendering WrappedStep.Name as "". RFC 0008
defines it as the wrapped step's name and <StepName> has a minimum length of
one, so an empty container or label name was a silent wrong result. Raising
keeps the session usable, unlike failing the action.
- URI path-mapping folds the scheme+authority over ASCII only, matching
openjd-rs's eq_ignore_ascii_case. str.lower() folded U+212A KELVIN SIGN to
ASCII 'k', so `s3://bucketK` matched `s3://bucketk`.
Hardening and cleanup:
- Replace getattr duck-typing on the EXPR typed-value API with concrete
ExprValue/TypeCode checks, so model skew fails loudly instead of silently
treating every optional integer field as omitted. Extended to the two sites
that substituted a plausible default: the cancelation notify period, and the
wrap-hook lookup that would otherwise report SUCCESS without running a hook.
- Replace an unreachable TypeError in PathMappingRule.apply with an assert; it
is called from run_task, where nothing would catch it.
- Embedded-file phase logging now says what each phase does; two methods
claimed a write neither performed while the method that writes logged
nothing.
- Correct the wrap-env file-content rewrite rationale: validation rejects
WrappedAction.* in an environment script's data and let, so the content is
invariant. The rewrite is kept for per-invocation determinism.
- Collapse a dead branch in the wrap-hook dispatch, drop the _WRAP_HOOK_NAMES
re-export, document enter_environment's Raises, and correct
exit_environment's (it documented ValueError where the code raises
RuntimeError).
- Document that openjd_env from a task-replacing hook deliberately does not
define a session variable, that this diverges from openjd-rs, and that the
spec does not settle it; log the discard at debug level.
- Note that _run_task_without_session_env bypasses wrap dispatch.
Tests: 18 added (oversized/over-u64 timeouts, the environment.variables
lifecycle, step_name required-and-optional, the first tests _apply_uri has
had, and an end-to-end cancel-delivery test proving the launch-resolved notify
method is applied to a SIGTERM-trapping subprocess). Eleven existing wrap
tests updated to pass step_name.
Conformance 2023-09 with the Python CLI: 1162 passed, 0 failed (unchanged).
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
* fix: Narrow Optional script before setattr in wrap cancelation test
CI lints with `mypy src test`, which flagged env.script as
`EnvironmentScript | None` at the object.__setattr__ call. The invariant
holds (_wrap_env always populates script), so assert it to narrow.
This blocked the whole test matrix via fail-fast, so no OS ran its tests.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
* fix: Close wrap-hook scope gap, cancel races, and path-mapping parity
Addresses a fourth independent review of this PR plus five parallel reviews,
each finding validated against openjd-rs upstream/main and the specification.
Spec-parity fixes:
- A wrap hook could not see the step-level `let` bindings its environment was
entered with, so a template that runs on openjd-rs failed here with
"Undefined variable". A hook resolves in its environment's own scope, and in
openjd-rs that scope is the environment's frozen enter-time symbol table --
so a step environment carries the owning step's bindings into every hook
invocation. New _seed_wrap_env_scope re-seeds the remembered Step.Name and
bindings, called after the wrapped action's own scope is built so they reach
only the hook. Verified byte-identical to openjd-rs, with scope isolation
intact (same binding name on both sides still resolves per side).
- Param.<name> and apply_path_mapping() disagreed for WINDOWS rules, though
RFC 0006 2.3.2 says they are the same transformation. PureWindowsPath
compares via str.lower(), which folds non-ASCII characters -- the same
U+212A KELVIN SIGN hazard the URI fold fix just closed, one branch over.
WINDOWS rules now compare components with the same ASCII-only fold, and a
trailing slash is recognized with either separator.
- A timeout in [2**63, 2**64-1] ran unwrapped but failed when forwarded as
{{WrappedAction.Timeout}}, because the EXPR engine's integers are i64. The
bound is now i64::MAX, which also matches openjd-rs's model.
Failure-path and race fixes:
- enter_environment and exit_environment set the session RUNNING before the
RFC 0008 branch materialized embedded files and allocated the hook's file
records, leaving a measured 77-159ms window in which cancel_action() -- a
cross-thread API guarded only by state == RUNNING -- hit an assert on the
runner and the cancel was silently lost. RUNNING is now set immediately
before the runner is asked to start, as run_task already did.
- A cancel racing action launch was dropped, or raised a bare AssertionError
from _cancel's assert on the subprocess. It is now recorded in
_pending_cancel and applied as soon as the subprocess exists; openjd-rs
holds the equivalent state in a sticky CancellationToken.
- A task parameter whose name starts with openjd_env made run_task raise
RuntimeError with no callback: the log filter's unanchored macro regex
matched the session's own parameter-logging line and tried to cancel with
nothing running. The filter's internal cancels now no-op unless an action is
running. Pre-existing; anchoring the regex is filed separately.
- A malformed URI path-mapping rule (a single-slash typo) was accepted by
from_dict and then crashed Session.__init__ with an error naming neither the
rule nor the value. The scheme grammar is now validated in the constructor.
Line-level corrections:
- The assert in PathMappingRule.apply claimed to stop an exception escaping
the public API; it does not, and under -O it degrades further. Rationale
corrected. Same for three stale rationales left by the previous round.
- _inject_wrapped_cancelation_symbols kept a getattr default that would have
silently told every wrap script the wrapped action declared no cancelation.
- An over-range integer field no longer reports "must be a positive integer"
for a value that plainly is one; the message names the maximum.
- run_task's step_name check is hoisted above _reset_action_state and the task
banner, so rejecting the call no longer discards the previous action status.
- source_path_component_count is private; it is only used internally.
Tests: 11 added, each verified to kill a mutant of the behavior it pins. Five
close gaps found by mutation testing, where breaking the shipped behavior left
the whole suite green -- notably the RFC 0005 1.3.2 typed-argument semantics on
the enforcement path, which every task and environment action goes through and
which only had direct-helper tests. The eight existing test_cancel cases were
updated to model a launched subprocess; they patched _run out and so had been
asserting the cancel-dropped-on-the-floor path.
pytest: 692 passed, 2 failed (the known xdist timing flakes, pass in isolation).
Conformance 2023-09 with the Python CLI: 1162 passed, 0 failed (unchanged).
mypy now run over src AND test, as CI does.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
* fix: Make wrap and URI tests host-independent on Windows
Windows CI ran this PR's tests for the first time (earlier runs were cancelled
by fail-fast before the test step) and found 8 deterministic failures, all in
test code:
- test_wrap_actions.py interpolates the trace-log path bare into `sh -c`.
On Windows a native path's backslashes are consumed as escapes by the sh
these tests invoke, so the redirect landed on a mangled path and the file
never appeared. The hooks were firing correctly. Now quoted and
slash-separated, matching the one interpolation that already was.
- The new URI path-mapping tests built their rule with from_dict, whose
destination_path is a host-flavoured PurePath -- so "/local" renders as
\\local on Windows. They now construct the rule with an explicit
PurePosixPath destination, which keeps the expectations host-independent
without weakening what they assert.
No production code changed.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
* test: Pin the fixes from the parallel-review round
Four behaviors were fixed in the previous commits with no test to hold them.
Each test here was verified to fail against a mutant that reverts its fix.
- WINDOWS path rules fold case over ASCII only: a U+212A homoglyph must not
match an ASCII 'k', while ASCII case-insensitivity and both trailing-slash
separators keep working.
- A URI rule's source_path must carry a real scheme, rejected at construction
where the offending value can be named.
- The session never claims RUNNING while it has no runner, observed at the last
point before the runner is built in all three RFC 0008 wrap paths. That
window is one in which a cross-thread cancel_action() is lost.
- A task parameter named like an env macro (openjd_env, openjd_unset_env,
openjd_redacted_env, openjd_envX) does not make run_task raise: the session's
own parameter-logging line reaches the action-message filter while nothing is
running.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
* chore: Require openjd-model >= 0.11.1
openjd-model-for-python #318 is merged and released, so the version-skew note
can go and the floor can name a real release.
0.11.1 is the floor because it is the first release carrying the whole surface
this package uses. Verified against the published wheels: 0.11.0 provides only
StepTemplate.let, StepScript.let and SymbolTable.expr_types, and is missing
FormatString.resolve_value, FormatString.whole_field_expression,
evaluate_let_bindings, CancelationMethodDeferred and
SymbolTable.expr_host_rules -- this package fails at import against it. 0.11.1
provides all nine.
CI already resolved 0.11.1 under the old floor and ran mypy clean over src and
test on Linux, macOS and Windows, so the merge-order constraint on the model is
satisfied.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
* fix: Do not fail an action whose subprocess exits immediately
CI surfaced this on macOS 3.12 as a flaky failure in an unrelated wrap test,
and it is the same root cause as the Windows-only flake that has been hitting
one of six Windows jobs per run:
posix: os.getpgid(pid) raises ProcessLookupError (ESRCH)
windows: psutil raises NoSuchProcess ('process PID not found (pid=...)')
Both are reached when a trivial command finishes before the runner has finished
recording it. Both were raised on the run future, so a subprocess that ran to
completion was reported as a failed action, and the session went to
READY_ENDING -- which is why a wrap test asserting READY between tasks failed
intermittently on the second of three back-to-back echo actions.
- _subprocess.run: an already-reaped child has no process group to look up.
Fall back to its own pid, which is the group we would have found since the
child is the group leader.
- _windows_process_killer._suspend_process_tree: a process that exits between
being discovered and being walked has no children to suspend. Mirrors the
NoSuchProcess handling already present elsewhere in that module.
Tests: the posix guard is pinned by patching os.getpgid to raise (verified
against a mutant that stops catching it); the Windows walk is pinned with a
psutil mock, gated to Windows.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
* fix: Serialize the pending-cancel handoff against action launch
Addresses the review comment on the previous head: the _pending_cancel handoff
was unsynchronized, so the lost-cancel window it was meant to close was still
open. Two defects, both real:
- The reader in _run and the writer in _cancel_with_resolved_method both
touched _pending_cancel outside self._lock, so a cancel could be recorded
after _run had already consumed (and found nothing).
- The writer keyed on whether the LoggingSubprocess object existed, not on
whether it had started. In the window where the object exists but the pool
thread is still inside _start_subprocess, the canceller handed off to
_cancel, which returns early because the process is not running -- while _run
had already passed its consume point. Dropped by both sides.
Now the decide-and-record step happens under the lock and keys on
has_started, and _run consumes under the lock; _cancel is called outside it,
since it takes the lock itself.
The regression test reproduces the exact interleaving by blocking inside
_start_subprocess so the window is held open deterministically. Verified
against a mutant keyed on object existence: it ends SUCCESS after the full 30
second sleep instead of CANCELED, i.e. the cancel is silently dropped.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
* fix(concurrency): Close race windows in cancel and completion paths
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>
* fix(concurrency): Close round-4 review findings (R4-1, R4-5, R4-6, R4-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>
* fix: Close round-5 review findings R5-1 through R5-9
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>
* fix: Close defects introduced by the round-5 fix commit
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>
* fix(wrap-actions): Isolate a wrap hook's scope from the wrapped entity's
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>
* fix(subprocess): Own the child on every exit path from run()
`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>
* fix: Report chown and pgrep failures through the paths that handle them
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>
* test: Replace two flaky timing tests with deterministic ones
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>
* test: Run the subprocess-racing tests serially, and quiesce between them
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: Name test files for what they test, not review rounds
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>
* fix: Silence Windows mypy on POSIX-only os and signal attributes
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>
* fix: Do not open a directory descriptor on Windows
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: Skip the POSIX-branch control on Windows, drop stale banners
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>
* fix: Address the live CodeQL alerts on this PR
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>
* refactor: Delete the dead _v1/_linux/ duplicate (R1)
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 #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>
* refactor: Single-source the cancel-path signal helper (R5, R13)
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>
---------
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
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.
Planned Merge Date: TBD
Note: force minor bump
Fixes: N/A — this is a multi-commit feature branch, not a bugfix.
What was the problem/requirement? (What/Why)
openjd-sessions-for-pythonis being migrated to Rust (seeopenjd-rs/crates/openjd-sessions). This branch adds the Python consumer side of that migration: a newopenjd.sessions._v1package that delegates the core session machinery (subprocess management, template evaluation, path mapping) to the compiled Rust extensionopenjd._openjd_rs, while preserving the existing v0 API surface untouched.What was the solution? (How)
src/openjd/sessions/_v1/that re-exports symbols fromopenjd._openjd_rsand adds a thin Python orchestration layer (_session.py).Sessionclass wraps the Rust_RustSessionand adds:SessionCallbackTypecallback contract that the worker agent depends on.RUNNINGcallback to avoid a race with the worker-agent scheduler.__enter__/__exit__) that guaranteescleanup()is called.pyo3-logbridge so Rustlog::info!()etc. routes into Python'sloggingmodule.openjd.sessions) is untouched. v1's public API is a strict superset of v0's (same 18 symbols + 3 new:ActionResult,ScriptRunnerState,SessionRuntimeError).See the comment below for the full architecture diagram, file-by-file breakdown, and open questions.
What is the impact of this change?
openjd.sessionstoopenjd.sessions._v1.openjd.sessions._v1requires the compiledopenjd._openjd_rsextension to be installed._linux/and_win32/under_v1/are still pure-Python ctypes (capabilities + Win32 impersonation) — only the session core is Rust-backed in this PR.How was this change tested?
test/openjd/sessions/was renamed totest/openjd/sessions_v0/to coexist cleanly with the newsessions_v1test tree (no v0 test content changes).New
test/openjd/sessions_v1/:test_session_scenarios.py— single parametrized test driving 18 YAML scenarios covering let-bindings and parameter types (incl. path mapping). Platform-gated viarun_on: posix|windows|all.test_pickle.py— 11 tests verifying the Rust-backed types (ActionState,SessionState,ActionStatus,ActionResult,PosixSessionUser) round-trip throughpickle. The worker agent crosses process boundaries with these types, so this matters.test_importable.py— smoke import.Unit tests run locally — blocked on getting
openjd._openjd_rsinstalled in the reviewer's environment; will update once verified.Was this change documented?
__all__in_v1/__init__.py.DEVELOPMENT.mdyet — that should follow once the v0 → v1 migration story is settled.Is this a breaking change?
No. The change is purely additive. v0's public contract (
openjd.sessions) is unchanged. v1 is a strict superset accessed via a new import path.Does this change impact security?
Possibly — flagging for review:
_v1/_win32/retains the Win32 user-impersonation logic (LogonUser, token handling). It's pure-Python ctypes, ported from v0, and there are open TODOs around domain handling that may warrant a threat-model pass before this lands.Session(inopenjd-rs/crates/openjd-sessions) handles subprocess execution and working-directory creation. Its threat model lives upstream inopenjd-rs; this PR only consumes the compiled artifact.If reviewers want to label this with "security", happy to engage.
Cross-port to openjd-rs
openjd-rs/crates/openjd-sessionscrate as the backend foropenjd.sessions._v1. The Rust crate is the source of truth; this PR introduces the Python binding layer.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.