Conversation
openjd-rs released on 2026-09-18: openjd-expr 0.9.0, openjd-model 0.9.0, openjd-sessions 0.7.0 (OpenJobDescription/openjd-rs#401). The release is one feature plumbed through all three crates -- opt-in resolved-value caps and evaluation budgets (#399) -- plus a job-creation re-check on carried-forward fields (#404) and an MSRV/dependency sweep (#403). Four breaking signature changes reach this package's bindings: FormatString::validate_expressions now takes a FormatStringOptions, decode_environment_template takes CallerLimits, evaluate_let_bindings takes the two budgets, and CallerLimits / SessionConfig gained fields. CallerLimits grows from six fields to ten, and the four new ones are threaded to every entry point upstream threads them to: the environment template decoders, evaluate_let_bindings, and Session, which is the run-time enforcement boundary for the resolved-value caps. Separately, max_template_size was inert in this package. It is checked only inside document_string_to_object, and the parse_string helper passed a default CallerLimits, so a caller asking for a byte ceiling on decode_job_template_str got none. parse_string now takes the caller's own limits. Found while documenting the field. Cargo.lock moved the three crates plus the #403 sweep; THIRD-PARTY-LICENSES regenerated to match. Verified: 6139 passed / 24 skipped / 3 xfailed, coverage 94.16%; ruff, black, mypy, cargo fmt and clippy clean. Six mutants covering each piece of new plumbing were each caught by the new tests. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
| """ | ||
|
|
||
| @staticmethod | ||
| def _template(action: dict[str, Any], embedded: list[dict[str, Any]] | None = None) -> dict: |
There was a problem hiding this comment.
list[dict[str, Any]] | None in a function signature is evaluated eagerly at def time, and types.GenericAlias.__or__ only exists from Python 3.10. This module has no from __future__ import annotations (it imports Union from typing at line 5, which is the pattern the rest of the file follows), so on Python 3.9 this raises TypeError: unsupported operand type(s) for |: 'types.GenericAlias' and 'NoneType' while the class body executes — a collection error for the whole file, not just this test class. pyproject.toml sets requires-python = ">=3.9" and code_quality.yml runs the matrix on 3.9.
Suggest Optional[list[dict[str, Any]]] (adding Optional to the existing typing import) to match the file's existing style.
| return deserialize_step( | ||
| { | ||
| "name": "S", | ||
| "script": {"actions": {"onRun": {"command": "echo", "args": [arg]}}}, |
There was a problem hiding this comment.
These are the first tests in the repo that actually run a session action (test_repr.py only constructs sessions), and code_quality.yml runs the Python matrix on windows-latest and macos-latest as well as Linux. On Windows there is no echo.exe — echo is a cmd.exe builtin — so unless openjd-sessions resolves the command through a shell, the two negative controls that expect the action to actually succeed (test_an_argument_under_the_cap_runs, test_omitting_caller_limits_enforces_nothing, and test_an_unrelated_limit_does_not_affect_the_action) will get FAILED from a spawn error rather than SUCCESS, and fail for a reason unrelated to caps.
Note the positive case is unaffected — it is rejected before spawn — so a Windows breakage here would look like "the negative controls are broken," which is the more confusing failure mode.
Worth either picking a command that exists on all three platforms (e.g. sys.executable with -c pass, parameterizing on os.name, or the pattern test_strings.py uses of gating on os.name), or confirming the crate shells out on Windows.
The helper's annotation is evaluated at class-body definition time, so `list[dict[str, Any]] | None` raised TypeError on Python 3.9 and the whole module failed collection. Every local interpreter is >= 3.10, where the operator is valid, so only the 3.9 CI leg caught it — and its fail-fast cancelled the 12 macOS and Windows jobs. `Optional` never uses `|`, so it cannot reach that path. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
| ) -> PyResult<()> { | ||
| let st = extract_symtab(symtab)?; | ||
| let lib = profile_for_call(profile); | ||
| let opts = FormatStringOptions::new().with_library(&lib); |
There was a problem hiding this comment.
Consistency gap with the rest of this PR. The rationale given for adding caller_limits to evaluate_let_bindings — "a caller enforcing max_eval_memory_bytes / max_eval_operations elsewhere has to pass the same limits here for the budgets to bound every evaluation uniformly" — applies verbatim to the three FormatStringOptions sites here (lines 42, 56, 149) and to the with_library(&lib) builders in expr/evaluate.rs:63 and expr/parsed_expression.rs:76,120,153. Those all pin the spec-recommended 100 MB / 10M defaults with no way for a caller to lower them.
So after this PR a service that sets max_eval_memory_bytes=1MB gets it enforced at decode_*, create_job, evaluate_let_bindings, and in a Session, but any direct use of FormatString.resolve, FormatString.validate_expressions, or evaluate_expression still evaluates on the 100 MB default. The docstring change here documents that as intended ("its default evaluation budgets"), which may well be the plan — but it is worth stating whether the expr-layer entry points are a deliberate follow-up rather than an oversight, since the budgets are a DoS control and this is the layer with no ceiling.
| ** siphasher; version 1.0.3 -- https://crates.io/crates/siphasher | ||
| ** syn; version 2.0.119 -- https://crates.io/crates/syn | ||
| ** syn; version 3.0.4 -- https://crates.io/crates/syn | ||
| ** thin-vec; version 0.2.20 -- https://crates.io/crates/thin-vec |
There was a problem hiding this comment.
Two of the crates this Cargo.lock bump newly vendors appear to be missing entries here. The lock adds ar_archive_writer 0.5.3 and object 0.39.1 (both pulled in via psm ← stacker ← rustpython-ruff_python_parser 0.16.5), and neither name appears anywhere in this file, while the sibling additions from the same subtree — psm, stacker, arrayvec, drop_bomb, itertools 0.15.0, thin-vec, char_str, zmij — all do.
This matters because the file is the distributed attribution notice for an Apache-2.0 AWS package: object is Apache-2.0/MIT and ar_archive_writer is Apache-2.0-with-LLVM-exception, so both carry notice obligations. Worth re-running whatever generator produced the rest of the diff and confirming they are omitted deliberately (e.g. build-dependency-only and out of scope) rather than by accident.
| caller_limits=CallerLimits(max_resolved_arg_len=100), | ||
| ) | ||
| assert decode_environment_template_str( | ||
| json.dumps(self._TEMPLATE), DocumentType.JSON, supported_extensions=[] |
There was a problem hiding this comment.
The docstring says "Negative control on both entry points," but the second assertion passes no caller_limits at all, so it is a control for no cap rather than for a cap that fits. The _str entry point is therefore never exercised with a cap the document satisfies.
That matters specifically because of the parse_string fix in this same PR: decode_environment_template_str now threads the caller's own CallerLimits into document_string_to_object instead of CallerLimits::default(). test_str_entry_point_applies_the_cap pins that a cap which is exceeded rejects, but nothing pins that a cap which is satisfied still accepts on this path — so a future regression that made the _str path over-reject (e.g. measuring the wrong thing, or an off-by-one in the comparison) would be caught only on the dict path.
Suggest adding caller_limits=CallerLimits(max_resolved_arg_len=100) to the decode_environment_template_str call so it mirrors the dict assertion above it.
| ) | ||
| try: | ||
| session.run_task(step_script=_step(arg).script) | ||
| deadline = time.monotonic() + 30 |
There was a problem hiding this comment.
This try/finally can leak a working directory. session.cleanup() (session.rs:698-704) takes the self.session guard and does nothing at all if the slot is None — and the slot is None for exactly as long as a background action thread owns the Session (run_task does guard.take() at session.rs:583). There is no error and no retry; the cleanup is silently skipped.
Two ways in:
- An
assertinside thetryfires while the action is still in flight — most plausibly the"action did not finish within 30s"assert, whose whole purpose is to fire while the action has not finished. - An unexpected exception anywhere in the poll loop.
In either case finally calls cleanup(), the slot is empty, and the session's session_root_directory (a real temp dir, since retain_working_dir defaults false but cleanup is what acts on it) is never removed. The test then fails for the right reason but leaves state behind — and because the session id is time_ns()-derived, repeated failures accumulate distinct directories rather than reusing one.
The narrow fix is to move the terminal-state assertions out of the try (compute state/message, cleanup(), then assert), or to poll-to-terminal inside a wrapper that guarantees the action has ended before cleanup runs. Worth handling since these are the repo's first tests that actually run an action, so this becomes the pattern later session tests copy.
| /// spec-defined limits. The caps that only a job template has | ||
| /// (step and task counts) do not apply here, and | ||
| /// ``max_template_size`` has no document string to measure; | ||
| /// the resolved-value caps and evaluation budgets do apply. |
There was a problem hiding this comment.
Two asymmetries in the new caller_limits docstrings that look unintentional rather than deliberate.
The step/task-count claim is stated only for environment templates. Lines 169 and 215 both say the caps that only a job template has "do not apply here." True, but the reason is that an environment template has no steps, not that the binding filters anything — the same CallerLimits value is passed straight through to decode_environment_template. Since the class-level docs on PyCallerLimits now enumerate all ten fields with no per-entry-point notes, a reader of decode_environment_template reasonably concludes the binding does some filtering. Worth phrasing as "have no counterpart in an environment template" rather than "do not apply here."
max_environment_size and max_env_count are unaccounted for. Those two do have environment-template counterparts, yet neither new environment-template docstring mentions them — the text jumps from "step and task counts do not apply" to "the resolved-value caps and evaluation budgets do apply," leaving the two environment-shaped document caps in neither bucket. The specs/python-model-interface.md table added in this PR lists both as "Enforced: decode," which a reader would take to include decode_environment_template. If they are enforced on this path, say so; if they are not (e.g. max_env_count counts job+step environments and a standalone environment template has neither), that is the more surprising fact and the one worth writing down.
Same text appears in src/openjd/_openjd_rs.pyi:3574,3611 and specs/python-model-interface.md:216-220.
| source = "registry+https://github.com/rust-lang/crates.io-index" | ||
| checksum = "8b4baf93f58d4425749ca49a51c50ebab072c5df6994d08fed93541c331481dc" | ||
| dependencies = [ | ||
| "either", |
There was a problem hiding this comment.
This lock keeps two itertools majors, but THIRD-PARTY-LICENSES.txt was edited as if the old one went away. The diff there replaces the single entry:
-** itertools; version 0.14.0
+** itertools; version 0.15.0
Yet itertools 0.14.0 is still in this lock and still depended on — pyo3-stub-gen references "itertools 0.14.0" (Cargo.lock:1000) while the new rustpython-ruff_python_trivia 0.16.5 references "itertools 0.15.0" (Cargo.lock:1251). The 0.15.0 entry is an addition, not a version bump, so after this PR the notice file attributes a version the build does not use and omits one it does.
Contrast with how the same generator handled hashbrown in this diff: base had entries for both 0.16.1 and 0.17.1, the lock dropped 0.16.1, and the notice correctly dropped just that one — leaving 0.17.1. That is the multi-version behaviour, which makes the itertools single-line swap look like a replace-in-place rather than generator output.
Worth re-running the notice generator and confirming itertools 0.14.0 reappears alongside 0.15.0. Since this file is the distributed attribution notice for an Apache-2.0 AWS package and itertools is Apache-2.0/MIT, dropping a version still in the dependency graph drops a live notice obligation.
| st = SymbolTable({"Param.X": 10}) | ||
| with pytest.raises(ExpressionError) as excinfo: | ||
| evaluate_let_bindings( | ||
| ["a = 'z' * 10000"], st, caller_limits=CallerLimits(max_eval_memory_bytes=16) |
There was a problem hiding this comment.
These assertions pin the exact internal counters an upstream evaluator reports, not the behaviour under test:
"operation count (2) exceeded limit (1)"— line 177"memory usage (72 bytes) exceeded limit (16 bytes)"— line 185- and in
test_parse.py,"memory usage (100136 bytes)"and"operation count (392)"
The behaviour being verified is that the budget is applied, which the limit half of the message and the exception type already establish. The actual half (2, 72, 100136, 392) is an implementation detail of openjd-expr's accounting: a change to how a string's allocation is measured, or to how many ops a * lowers to, shifts those numbers without changing any contract. Because openjd-expr is pinned as "0.9.0" (a caret requirement), a 0.9.x patch release can be picked up without any change in this repo and break these tests — and the failure would read as "the budget stopped working" rather than "the counter got more precise."
72 for 'z' * 10000 is the most fragile of the set: it is neither the input nor the output size, so it is measuring something like peak intermediate accounting at the point the limit tripped — exactly the kind of number that moves.
Suggest asserting on the limit and the shape only, e.g. "exceeded limit (16 bytes)" in message, and dropping the actual counts. The test_let_bindings.py cases additionally assert the "Error evaluating let binding 'a':" prefix, which is this repo's own contract and worth keeping.
Fixes: n/a (dependency bump for OpenJobDescription/openjd-rs#401)
What was the problem/requirement? (What/Why)
openjd-rs released on 2026-09-18: openjd-expr 0.9.0, openjd-model 0.9.0, openjd-sessions 0.7.0. This package pinned 0.8.0 / 0.8.0 / 0.6.0.
The release is one feature plumbed through all three crates — opt-in resolved-value caps and evaluation budgets (openjd-rs#399) — plus a job-creation re-check on carried-forward fields (#404) and an MSRV/dependency sweep (#403). Four public signatures changed, so the bindings do not compile against 0.9.0 without edits:
FormatString::validate_expressionsFormatStringOptionsinstead of(lib, target_type)decode_environment_template&CallerLimitsevaluate_let_bindingsmemory_limit/operation_limitCallerLimits,SessionConfigWhat was the solution? (How)
Bump the three pins, adapt the four call sites, and thread the new policy fields to every entry point upstream threads them to.
CallerLimitsgrows from six fields to ten.max_resolved_arg_lenandmax_resolved_data_lencap a resolvedcommand/ argv entry (Template Schemas §5.1, §5.2) and a resolved embedded-filedatavalue (§6.1.2) — both in characters, for limits the spec defers to the OS.max_eval_memory_bytesandmax_eval_operationsare the Expression Language spec's memory-bounded-evaluation budgets (§1.3.9, §1.3.10), which have recommended defaults rather than maxima. All four are exposed as constructor kwargs and getters, and are covered by__repr__,__reduce__and__eq__.Because the fields are inert unless they reach the stage that enforces them, three entry points gained a
caller_limitsargument:decode_environment_template/decode_environment_template_str— upstream now takes caller limits here, and this package's docstrings previously said environment templates do not accept them.evaluate_let_bindings— a binding evaluates a parsed expression rather than resolving a format string, so without the budgets a caller who lowered them elsewhere would leave let bindings on the defaults.Session— the run-time enforcement boundary, since a worker can run a job that never passed through the validating process. The binding fillsSessionConfig.limitsvia the upstreamFrom<&CallerLimits>.Separately:
max_template_sizewas inert in this package. It is checked in exactly one place,document_string_to_object, against the document's byte length before parsing — and the bindings'parse_stringhelper passed&CallerLimits::default(). A caller askingdecode_job_template_strfor a 10-byte ceiling got no ceiling, silently.parse_stringnow takes the caller's own limits. The dict entry points are handed an already-parsed mapping and have no document string to measure, so the fourcaller_limitsdocstrings now state that asymmetry rather than describing one behaviour for all four. Found while writing the field table forspecs/python-model-interface.md, not by the bump itself.Behaviour changes, each measured through this package's API. Every row was accepted on 0.8.0, because no caller could express the limit:
max_resolved_arg_lendecode_job_templatecommand/args[0]under cap 5:is 20 characters, exceeding the maximum of 5max_resolved_arg_lencreate_job{{Param.P}}passes decode (lower bound 0), thenresolves to at least 30 characters, exceeding the maximum of 10max_resolved_arg_len(#404)create_jobjobEnvironments[0]scriptmax_resolved_arg_lenSession.run_taskFailed to resolve args[0]: resolved value is 40 characters, exceeding the maximum of 5max_resolved_data_lendecode_job_templateembeddedFiles[0] -> datarejected on the same termsmax_eval_memory_bytesdecode_job_template{{ 'a' * 100000 }}under 1024 bytes:memory usage (100136 bytes) exceeded limit (1024 bytes)max_eval_operationsdecode_job_templateoperation count (392) exceeded limit (5)max_eval_operationsevaluate_let_bindingsa = Param.X + 1under 1 operation:operation count (2) exceeded limit (1)max_template_sizedecode_job_template_strTemplate document size (N bytes) exceeds caller limit of 10); previously inertI reconciled the changelog against a source diff of the published crates. Every differing file maps to #399, #404 or #403; there are no unclaimed behaviour changes.
openjd-modelgained an internalEvalBudgetshelper that carries the budgets into every resolution job creation performs, which is whyranges.rsandinstantiate.rsare in the diff.What is the impact of this change?
No existing test changed, and no public contract of this package is removed or narrowed. Three signatures gain an optional keyword argument, and
CallerLimitsgains four optional keyword arguments.One behaviour changes for existing callers: a caller already passing
max_template_sizetodecode_job_template_strordecode_environment_template_strwas getting no enforcement and now gets the ceiling it asked for. A caller whose documents exceed the ceiling they configured will start seeingModelValidationError— which is the behaviour the argument has always advertised.Cargo.lockmoved the three crates plus the #403 sweep (ruff 0.16, sorustpython-ruff_*0.15.8 → 0.16.5,compact_str,get-size2,itertools, and several new transitive crates).THIRD-PARTY-LICENSES.txtwas regenerated withscripts/check_third_party_licenses.sh --updateand verifies clean in CI mode.How was this change tested?
hatch run test: 6139 passed, 24 skipped, 3 xfailed, coverage 94.16%. The three xfails are the pre-existingopenjd.exprknown gaps (symbol-table__setitem__,ExpressionErrorstructured location); none flipped, so nothing in this release closed them.hatch run lint(ruff, black, mypy) clean.cargo fmt --checkandcargo clippy -p openjd-python --all-targets -- -D warningsclean.New tests, each with a negative control and a docstring stating what 0.8.0 did with the same input:
test_parse.py:TestResolvedValueCapsAtTemplateValidation,TestEvaluationBudgetsAtTemplateValidation,TestEnvironmentTemplateCallerLimits,TestMaxTemplateSizeReachesTheParsertest_create_job.py:TestResolvedValueCapsAtJobCreation,TestJobEnvironmentResolvedValueCapsAtJobCreationtest_let_bindings.py:TestEvaluateLetBindingsCallerLimitstest/openjd/sessions/test_caller_limits.py(new file):TestSessionResolvedArgLengthCap, which runs realechosubprocesses — that directory previously held onlytest_repr.py, so the session bindings had no behavioural coverage heretest_pickle.py: bothcaller_limitsround-trips extended to the ten fields, with anloaded == limitsassertion so a field dropped from__reduce__failsMutation check — six mutants, one per piece of new plumbing, each rebuilt and each caught:
caller_limitsignored (both entry points)None, NoneCallerLimits.max_resolved_arg_lennever stored__reduce__dropsmax_eval_operationsSessioncaller_limitsignoredparse_stringreverted toCallerLimits::default()Each mutant was restored byte-for-byte (checksum-verified) with the bytecode cache cleared between runs.
One correction worth recording: two first-draft assertions expected
create_jobto substitute an action's arguments. It does not — they stayFormatString("{{Param.P}}")on the created job, because task parameters resolve in the session. The cap reads the resolved length without rewriting the field, and the tests now assert that.Was this change documented?
Yes. Docstrings on every changed binding, the
_openjd_rs.pyistub (hand-edited;scripts/generate_stubs.shdoes not run on macOS), and three spec files:specs/python-model-interface.md— aCallerLimitsfield table giving each field's bound and the stage that enforces it, thecaller_limitsargument on both environment-template entry points, and theevaluate_let_bindingsbudgets.specs/python-sessions-interface.md—caller_limitson theSessionsketch, with the enforcement-boundary rationale.specs/python-expr-interface.md— thevalidate_expressionsmirror text, which named the old three-argument crate signature.Is this a breaking change?
No. See the impact section for the one behaviour change:
max_template_sizenow does what it says on the*_strentry points.Does this change impact security?
Indirectly, in the direction of enforcement rather than away from it.
max_template_sizewas a policy field a caller could set and receive nothing from, and the new resolved-value caps let a submitting service bound values that previously reached process spawning unbounded. A session that is given no limits still enforces nothing beyond the spec, which is the documented default.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.