chore(deps): update ruff requirement from ==0.13.* to ==0.14.* - #277
Merged
Conversation
Updates the requirements on [ruff](https://github.com/astral-sh/ruff) to permit the latest version. - [Release notes](https://github.com/astral-sh/ruff/releases) - [Changelog](https://github.com/astral-sh/ruff/blob/main/CHANGELOG.md) - [Commits](astral-sh/ruff@0.13.0...0.14.0) --- updated-dependencies: - dependency-name: ruff dependency-version: 0.14.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
|
epmog
approved these changes
Oct 14, 2025
leongdl
added a commit
to leongdl/openjd-sessions-for-python
that referenced
this pull request
Jul 22, 2026
…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>
leongdl
added a commit
to leongdl/openjd-sessions-for-python
that referenced
this pull request
Jul 24, 2026
…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>
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>
Merged
1 task
mwiebe
pushed a commit
that referenced
this pull request
Aug 4, 2026
LoggingSubprocess created its Popen with encoding="utf-8" but no errors= parameter, so decoding defaulted to strict. A single byte of subprocess output that is not valid UTF-8 (for example a Windows DCC application writing cp1252, such as Unreal Engine emitting the 0x97 em dash) raised UnicodeDecodeError inside the _enqueue_stdout reader thread. The thread died silently, and all subsequent stdout from the subprocess was lost, masking any later errors from the job. Decode with errors="backslashreplace" so undecodable bytes are escaped (e.g. b"\x97" -> "\\x97") instead of raising. This keeps the reader thread alive while preserving the original byte values in the logs, which helps identify the codepage the subprocess is emitting. This ports the fix that openjd-adaptor-runtime-for-python applied to its own LoggingSubprocess in PRs #275 and #277. Regression tests cover: the reader thread surviving undecodable bytes, escape formatting for single/consecutive/truncated invalid sequences, valid multi-byte UTF-8 passing through unescaped, the behavior under a non-default encoding, and the errors= kwarg on the Popen construction shared by the sudo and Windows-as-user paths. Every test was mutation-checked: removing the errors= parameter, substituting replace/ignore handlers, and forcing an ASCII encoding each fail at least one test. 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
1 participant
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.
Updates the requirements on ruff to permit the latest version.
Release notes
Sourced from ruff's releases.
... (truncated)
Changelog
Sourced from ruff's changelog.
... (truncated)
Commits
beea8cdBump 0.14.0 (#20751)416e956[ty] Infer better specializations of unions withNone(etc) (#20749)88c0ce3Update default and latest Python versions for 3.14 (#20725)8fb29ea[ruff] improve handling of intermixed comments inside from-imports (#20561)23ebfe7[ty] Fix tiny mistake in protocol tests (#20743)f90d646[ty] Makeinfer_method_informationless confusing (#20740)15af4c0Move --show-settings snapshots to separate files (#20741)76f8e5bRefactor Rust lint test structure to use RuffTestFixture (#20689)b66a3e7[refurb] Add fixes forFURB101,FURB103(#20520)70f51e9[ty] Printdisplayof types when a property test fails (#20720)Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting
@dependabot rebase.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebasewill rebase this PR@dependabot recreatewill recreate this PR, overwriting any edits that have been made to it@dependabot mergewill merge this PR after your CI passes on it@dependabot squash and mergewill squash and merge this PR after your CI passes on it@dependabot cancel mergewill cancel a previously requested merge and block automerging@dependabot reopenwill reopen this PR if it is closed@dependabot closewill close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot show <dependency name> ignore conditionswill show all of the ignore conditions of the specified dependency@dependabot ignore this major versionwill close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor versionwill close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependencywill close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)