fix: close the resolved-value follow-ups from the #404 and #407 reviews by mwiebe · Pull Request #410 · OpenJobDescription/openjd-rs

mwiebe · 2026-09-25T22:37:07Z

Fixes: n/a — closes follow-up items recorded from the reviews of #404 and #407

What was the problem/requirement? (What/Why)

The reviews of #404 (job-creation resolved-value checks) and #407
(the create_job context contract) left a short list of follow-ups,
none blocking but each a real gap. This PR closes the four that are
small and self-contained:

A budget-accounting leak in list comprehensions. Every
child-evaluator call in eval_listcomp used ?, returning before the
child's counters were absorbed back into the parent. When an
enclosing construct absorbs the error and continues (an
unresolved-test conditional or and/or swallowing a value error —
the idiomatic shape at job creation, where Session.* is unresolved),
the parent resumed with the failed iteration's spend missing.
Measured: [int('A' * 1000000) for x in [1]] if Session.Flag else []
reported peak_memory 512 after a 1 MB allocation.
contains_budget_error's sub-error recursion was untested. No
test drove a compound both-branches-fail error carrying a budget
exceedance through a suppression site; deleting the recursion left
the suite green.
The two check-symtab builders parsed let bindings under different
profiles. Step scripts used the caller's host profile (as pass 8
does); environments went through the public evaluate_let_bindings,
which parses with the latest profile, every extension. An environment
let could therefore accept syntax pass 8 refused, or a crate upgrade
could silently widen what job creation accepts. They also reported
failures in two different message formats.
Four cleanups from an independent review of feat!: require create_job's context to cover the template's extensions #407: a
ModelExtension::ALL iteration that a future variant could escape,
two spec references to a section that doesn't exist, a spec omission
in summary.md, and one test still hand-rolling its create_job
context the pre-feat!: require create_job's context to cover the template's extensions #407 way.

What was the solution? (How)

eval_listcomp now hands the child's counters (and the regex cache
moved into it) back on every exit — the filter-error and non-bool
filter arms, the body error, and the result push's budget pre-check.
The body evaluation returns a Result<Option<_>> that is absorbed
once before ?, so the invariant holds without exemption. The loop
variable's clone is still live at the push, so a push-time exceedance
now reports it: two pinned diagnostics shift by exactly that clone's
64 bytes (16480 → 16544; 134217824 → 134217888), and their comments
explain the composition. Five tests pin the five sites; each was
verified by mutation to fail when its own absorb is removed.
Two tests for the recursion: the compound propagates through a boolop
(deleting the recursion fails it), and the control — a compound of two
value errors — is still suppressed.
Both check-symtab builders call one private
evaluate_check_let_bindings: host profile, POSIX, the caller's
budgets, one script let binding '<name>': <error> diagnostic. The
public evaluate_let_bindings is unchanged as the run-time entry
point for openjd-sessions and openjd-for-js. Two decisions the
unification made implicitly are now documented in job-creation.md:
structurally malformed bindings are skipped at the check path (pass 8
already rejects them at decode), and the check path's caret aligns to
the bare expression where pass 8 and run time align to the full
binding string.
create_job iterates the template profile's own extension set
(exhaustive by construction), sorted for a deterministic message and
pinned by a two-extension test; the dangling spec references point at
preprocess_job_parameters; summary.md records the context and
limits it passes; the sessions test uses
default_validation_context().

Two things worth knowing up front as a reviewer:

The 64-byte shift in the two pinned memory diagnostics is
intentional, not drift.
The parse-profile fix is correctness-by-construction today: no
ExprExtension variants exist yet, so the latest profile and any
current-revision profile accept identical syntax. The test pins what
is observable now — env and step-script let failures produce
byte-identical diagnostics through create_job (it fails on the old
code, where the env path used the other message format).

What is the impact of this change?

Callers who lower the evaluation budgets get exact accounting for
comprehensions whose failure is absorbed by an enclosing construct;
previously such failures were under-metered.
A push-time memory exceedance inside a comprehension reports 64 bytes
more than before (the live loop-variable clone). No caller depends on
the exact figure; the conformance suite passes unchanged.
At job creation, an environment let that fails with the real
parameter values now reports script let binding '<name>': … (the
step-script format) instead of Error evaluating let binding '<name>': …. Structurally malformed environment let bindings that
somehow bypassed decode are skipped at the check path rather than
erroring there; they still fail at run time.
The Compatibility error for several missing extensions now lists
them sorted (EXPR, FEATURE_BUNDLE_1) rather than in
ModelExtension::ALL order.
No public API signatures change. evaluate_check_let_bindings is
private; evaluate_let_bindings is untouched.

How was this change tested?

Have you run the unit tests? Yes. Workspace suite green
(openjd-expr 3,458 integration + 374 unit; openjd-model 1,792 +
372; openjd-cli 154; openjd-sessions 275 — the two remaining
sessions failures on the dev machine are the pre-existing
LogonUserW domain-environment issue, untouched by this change).
cargo clippy --all-features --all-targets --workspace -- -D warnings
clean; cargo fmt --check clean.
Full OpenJD conformance suite: 1139/1139 (Windows).
Every new test was verified in both directions: passes with the fix,
fails against the pre-fix code (via git stash of the changed source
file, or by mutating the specific absorb site it targets).
An independent agent audit of the changeset returned APPROVE with no
correctness defects; its non-blocking findings (four untested
absorption sites, a dangling spec reference, two undocumented
decisions, the absorb-before-push simplification) are all addressed
here.

Was this change documented?

Are relevant docstrings in the code base updated? Yes.
absorb_and_pass and evaluate_check_let_bindings carry rationale
docs; the FsEval struct docs and both pinned-diagnostic tests explain
their numbers.
Specs co-evolved: specs/expr/evaluator.md (ListComp counter
handoff), specs/model/job-creation.md (shared let path,
parse-profile rule, the two documented decisions, for-js as a caller),
specs/model/validation.md and specs/cli/summary.md (reference
fixes / the context summary passes). public-api.md needs no change
— nothing public moved.

Is this a breaking change?

No. No public signatures change. The observable differences are an
error-message format for one job-creation failure path, a sorted
listing in one Compatibility message, and a 64-byte difference in two
memory-exceedance diagnostics — none of which is a contract callers
parse.

Does this change impact security?

Does the change need to be threat modeled? No. The change makes
the evaluation memory/operation budgets more accurate (closing an
under-metering path an absorbed comprehension failure could exploit
to spend beyond the configured budget), and creates or modifies no
files or directories.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

and OpenJobDescription#407 reviews Four follow-up items recorded after the create_job profile-contract change (OpenJobDescription#407), plus the findings of an independent audit of this changeset. Items refer to the design doc's follow-up list. Item 10 - eval_listcomp discarded a failing child's counters. Every child.evaluate/eval_node in the comprehension used `?`, returning before absorb_counters ran, so an iteration that spent memory and operations and then errored left the parent's counters untouched. Moot when the error propagates to the top, but an enclosing absorption site - an unresolved-test conditional or boolop swallowing a value error - resumed with the spend missing: measured, `[int('A' * 1000000) for x in [1]] if Session.Flag else []` reported peak_memory 512 after a 1 MB allocation. The child's counters (and the regex cache moved into it) are now handed back on every exit: the filter-error and non-bool-filter arms, the body error, and - via a Result<Option<_>> that is absorbed once before `?` - the result push's budget pre-check too, so the invariant holds without exemption. The loop variable's clone is still live at the push, so a push-time exceedance now reports it; two pinned diagnostics shift by exactly that clone's 64 bytes (16480 -> 16544; 134217824 -> 134217888) and their comments explain the composition. Five tests pin the five sites, each verified by mutation to fail when its own absorb is removed. Item 13 - contains_budget_error's sub-error recursion was mutation-survivable: no test drove a compound both-branches-fail error carrying a budget exceedance through a suppression site. Two tests: the compound propagates through a boolop (deleting the recursion fails it), and the control - a compound of two value errors - is still suppressed. Item 5 (second half) - the two check-symtab builders parsed script- level let bindings under different profiles: build_task_check_symtab with the caller's host profile (as pass 8 does), build_env_check_symtab via the public evaluate_let_bindings, which parses with the latest profile - so an environment let could accept syntax pass 8 refused, or a crate upgrade could silently widen what job creation accepts. Both now call one private evaluate_check_let_bindings: host profile, POSIX, the caller's budgets, one `script let binding '<name>': <error>` diagnostic. The public evaluate_let_bindings is unchanged as the run-time entry point for openjd-sessions and openjd-for-js. Today no ExprExtension variants exist, so the fix is correctness by construction; the test pins what is observable: env and step-script let failures produce byte-identical diagnostics through create_job (fails pre-refactor - the env path used another message format). Two implicit decisions are now documented in job-creation.md: structurally malformed bindings are skipped at the check path because pass 8 rejects them at decode, and the check path's caret aligns to the bare expression where pass 8 and run time align to the full binding. Item 14 - four cleanups from the independent review of OpenJobDescription#407: the missing-extension check iterates the template profile's own extension set (exhaustive by construction) sorted for a deterministic message, pinned by a two-extension test; FsEval docs and validation.md pointed at a non-existent "Path Parameters" section (now preprocess_job_parameters); summary.md records the create_job context and limits summary passes; the sessions integration test uses default_validation_context() like every other caller. Verification: workspace clippy clean; expr/model/cli suites green; conformance 1139/1139. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>

…'s spend Review finding on this PR, verified: absorb_counters copies all three counters, but only peak_memory and operation_count are cumulative spend - current_memory is the live footprint. The new error arms absorbed all three and returned, so after an enclosing unresolved-test conditional or boolop swallowed the error and kept evaluating on the same evaluator, current_memory still carried the failed iteration's transients: the loop-variable clone and the intermediates of the failed sub-expression, none of which are live. Measured with a binop failure (dispatch releases its inputs only on success, so the 1 MB string stays tracked when `'A' * 1000000 - 1` fails): len(['A' * 1000000 - 1 for x in [1]] if Session.Flag else []) + len('B' * 600000) # limit 1.5 MB passed on main (peak 600576) and failed on this branch with "memory usage (1600776 bytes) exceeded limit" - charging the later 600 KB allocation for a megabyte that was gone. The original under-metering bug and this over-metering one are the two halves of the same mistake: treating spend and footprint as one quantity. The error exits now use absorb_spend_and_reset, which absorbs peak/ops and resets current_memory to the pre-iteration baseline - exactly what the unresolved-filter break arm already did. eval_listcomp_unresolved captures its own baseline and resets on every exit, including success: nothing it tracks (a placeholder loop variable, filter/body values evaluated only for their types) survives the call, so the unconditional carry-forward there was a smaller pre-existing version of the same leak. The success path of the concrete loop is unchanged - the footprint is absorbed before the push (the clone is live at that moment) and reset afterwards. Four tests pin it, one per absorption site: each fails on the PR head with the 1.6 MB exceedance and passes now, while still asserting the failed iteration's 1 MB reaches peak_memory - the two properties do not trade off. The spec's ListComp section states the spend/footprint distinction. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>

…on pre-check Review finding on this PR, verified and correcting an earlier commit's claim. 63d8c37 moved the child's counter absorb ahead of result.push, attributing the resulting 64-byte shift in two pinned diagnostics to "the loop variable's clone, still live at the push". That attribution was wrong: the loop variable's slot in the temp symbol table is never tracked (tmp.set does not go through track). What the child tracks is the clone eval_name returns when the body reads `x` - i.e. the element itself, the very value about to be pushed. Absorbing that footprint before the push made BudgetedVec's pre-check count the element twice: once in the parent's current_memory, once in its own value_bytes. For an Int that is 64 bytes. For a body producing large values it is a full element per push: measured, `['A' * 100000 for x in range(5)]` needs a ~501 KB limit on main and ~601 KB on the PR head - the effective limit shrank by one element. Over-metering, the opposite of what this PR set out to fix. Every exit from an iteration now uses absorb_spend_and_reset: peak and op count come back from the child, current_memory resets to the pre-iteration baseline, and the push pre-check sees baseline + value_bytes + slack - each element charged exactly once, by BudgetedVec. The trailing baseline restore is gone (the reset already happened), absorb_counters is deleted as unused, and the two pinned diagnostics return to their original values with comments that say what the number actually is. A new test pins the scaling: five 100 KB elements fit under 560 KB (fails on the PR head at 601152). The spec paragraph is corrected to match; it no longer names a value that was not being charged. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>

…exceedance Requested by review alongside the double-charge fix (0fceb54): the existing expectations only covered 64-byte elements, where one element counted twice is invisible. Two 600 KB elements fit under 1.5 MB; three exceed it at exactly 1800768 bytes - two held, one being pushed, plus the Vec's projected slack. On the pre-fix commit the two-element case already failed with that same figure, one push early. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>

Review finding on this PR, verified. The error exits reset current_memory to the per-iteration baseline, but that baseline is captured inside the loop, after the iterable was tracked - so an abandoned comprehension left its iterable charged on the evaluator an enclosing construct keeps evaluating on, even though nothing references it. The success path already handles this ("the iterable is consumed by the comprehension") with an explicit release after the loop; the error exits did not. Pre-existing (main's plain `?` exits skipped the release too), but the same class of residue this PR removes. Two more leaking exits than the review listed: the "Cannot iterate over" type error - which an enclosing construct can absorb, and whose iterable may be the large value itself - and the unresolved-iterable early return. Measured with a 600 KB list from the symbol table and a cheap body failure, then a 600 KB allocation under a 1 MB limit: 1200352 bytes reported before, peak 600192 after. The footprint is now captured *before* the iterable is evaluated, and every exit that abandons the comprehension resets to it: the unresolved-iterable return, the non-iterable type error, the three in-loop error arms (which already absorbed the child's spend), and the push's budget exceedance. By construction rather than by subtracting the iterable's size. Four tests pin the four absorbable exits; each fails on the previous commit and passes now. Two notes for the review thread. The review's literal example (`'A' * 1000000 - 1` over a 600 KB iterable, 1.5 MB limit) does not reach the absorption: 600 KB + 1 MB trips the limit inside the body as a budget exceedance, which nothing absorbs. And a large list *literal* as the iterable trips the limit while being built, before the comprehension runs: eval_list charges both the element and the list constructed from it. That is a separate, pre-existing eval_list accounting matter, noted in the test helper and out of scope here. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>

…ew rounds Four spend-vs-footprint cases identified and measured while fixing items 10 and 13 under review, left out of that PR as pre-existing and out of scope: eval_list charging a list literal's elements twice (1,200,128 bytes for one 600 KB element); eval_ifexp's absorb arms not releasing the healthy branch's value (1,600,264 with 600 KB live); dispatch error paths leaving failed-call operands charged (1,600,392 through boolop suppression); and gate-2 let-binding failures carrying no field path. Item 10's record is amended with the three review-round refinements to the footprint handling, and the quick-follow-ups placeholders now cite PR OpenJobDescription#410. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>

…int resets Design-doc items 17, 16 and 12, the root of the class of over-metering that three review rounds on OpenJobDescription#410 kept finding in the comprehension exits: a value error swallowed by an enclosing construct left the evaluator carrying charges for values that no longer existed. Item 17 - dispatch_with_node / dispatch_with_span released a call's operands only on success. A failed binop's operands stayed in current_memory, and every absorbing construct resumed on an evaluator still charged for them. Both helpers now share dispatch_released, which releases the inputs on either path before the error is positioned; eval_call already did this. Measured through a boolop: `[Session.Flag or 'A' * 1000000 - 1 == 'x', len('B' * 600000) > 0]` under 1.5 MB reported 1600392 bytes; it now fits. Item 16 - eval_ifexp's absorb arms tracked an Unresolved result but never released the healthy branch's value (nor, when both branches succeeded, either value). The result carries only a type, so every branch value is released. Measured: `len(int('x') if Session.Flag else 'A' * 1000000) + len('B' * 600000)` under 1.5 MB reported 1600264 bytes; it now fits. Item 12 - the two rules every absorption site needs now live in one helper, eval_speculative: a budget exceedance (directly or in a compound's sub-errors) propagates; an absorbed value error resets the live footprint to the pre-attempt baseline while the spend stands. Both eval_ifexp branches and eval_boolop's post-unresolved operands go through it, so no site can get one rule without the other. Along the way eval_boolop releases every operand its result does not carry - the unresolved placeholder, a superseded concrete operand, and a speculative one that did not decide - which it never did before. One deliberate behaviour change: a budget exceedance in the if-branch now propagates before the else-branch is evaluated. Before, both branches always ran and a budget failure paired with a value failure surfaced as a "Both branches fail" compound; now the budget error surfaces as itself, at once, without spending the else-branch's cost on an already-blown budget. Two tests are updated for that - one byte-count (the released unresolved operand no longer appears in the reported usage) and one whose premise was the compound; it now pins the fail-fast. contains_budget_error's recursion into sub-errors is kept as defence in depth: the evaluator can no longer build a compound around a budget error, but the type permits sub-errors from any source. Seven tests pin the releases (each ifexp arm, both-succeed, a failed binop absorbed by boolop and by ifexp, non-deciding boolop operands); each fails on the branch base and passes now. Spec: a new "Speculative evaluation" section in evaluator.md carries the two rules; BoolOp, IfExp and Dispatch Flow are rewritten against it. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>

Design-doc item 15. eval_list tracked each element as it was evaluated and then tracked the list built from them without ever releasing the elements, so a literal's live footprint was the list plus every element again - and make_list_checked's construction pre-check added the list's heap size on top of the still-charged elements. For 64-byte scalars it was invisible; for large elements it doubled the charge for as long as the literal was held. Measured: `len(['C' * 600000]) + len('B' * 300000)` under 1 MB failed at 1,200,128 bytes with the caret on the literal - during its construction, before anything else ran. On the success path, so it also skewed the numbers for every downstream consumer of a large-element literal. Found while writing OpenJobDescription#410's iterable-abandonment tests, whose first draft used a list literal as the iterable and tripped the limit before the comprehension ran. The elements are released once they are about to be consumed into the list - before the pre-check, on all three construction paths (unresolved placeholder, target-coerced, homogeneity-checked) - mirroring dispatch's release of consumed inputs. The three error returns before that point leave the elements charged like any failed sub-expression's operands; eval_speculative resets the footprint if an enclosing construct absorbs the error. Two pinned diagnostics move by exactly the released element trackers (three Ints, 192 bytes each case): `[1, 2, 3] * 10000000` 1920000576 -> 1920000384, and the large-element comprehension over `[1, 2, 3]` 1800768 -> 1800576. Their comments derive the new figures. A new test pins the fix: the single 600 KB literal plus 300 KB fits under 1 MB (fails pre-fix at 1,200,128), and two such literals held at once by a concatenation exceed at exactly 1200224 - two list charges, not four. Spec: the List section documents the accounting. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>

…ew rounds Four spend-vs-footprint cases identified and measured while fixing items 10 and 13 under review, left out of that PR as pre-existing and out of scope: eval_list charging a list literal's elements twice (1,200,128 bytes for one 600 KB element); eval_ifexp's absorb arms not releasing the healthy branch's value (1,600,264 with 600 KB live); dispatch error paths leaving failed-call operands charged (1,600,392 through boolop suppression); and gate-2 let-binding failures carrying no field path. Item 10's record is amended with the three review-round refinements to the footprint handling, and the quick-follow-ups placeholders now cite PR OpenJobDescription#410. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>

Cross-cutting design for openjd-expr, openjd-model, and openjd-sessions covering the resolved-value constraint gates (template validation, job creation, run time), the lower-bound rule for partially-static format strings, the CallerLimits / SessionLimits configuration surface, and the follow-up log through PRs OpenJobDescription#373, OpenJobDescription#383, OpenJobDescription#397, OpenJobDescription#399, OpenJobDescription#404, OpenJobDescription#407, OpenJobDescription#409, OpenJobDescription#410, and OpenJobDescription#417.

mwiebe requested a review from a team as a code owner September 25, 2026 22:37

github-actions Bot added the waiting-on-maintainers label Sep 25, 2026

github-actions Bot reviewed Sep 25, 2026

View reviewed changes

Comment thread crates/openjd-expr/src/eval/evaluator.rs Outdated

github-actions Bot reviewed Sep 25, 2026

View reviewed changes

Comment thread crates/openjd-expr/src/eval/evaluator.rs Outdated

github-actions Bot reviewed Sep 28, 2026

View reviewed changes

Comment thread crates/openjd-expr/src/eval/evaluator.rs Outdated

github-actions Bot reviewed Sep 28, 2026

View reviewed changes

Comment thread crates/openjd-expr/src/eval/evaluator.rs Outdated

mwiebe added 5 commits September 28, 2026 11:45

mwiebe force-pushed the fix/resolved-value-quick-followups branch from 9682441 to c905bee Compare September 28, 2026 18:56

leongdl approved these changes Sep 28, 2026

View reviewed changes

mwiebe merged commit 4a08fb8 into OpenJobDescription:main Sep 28, 2026
22 checks passed

mwiebe deleted the fix/resolved-value-quick-followups branch September 28, 2026 22:16

github-actions Bot mentioned this pull request Sep 28, 2026

chore: release #416

Merged

Navigation Menu

fix: close the resolved-value follow-ups from the #404 and #407 reviews - #410

fix: close the resolved-value follow-ups from the #404 and #407 reviews#410
mwiebe merged 5 commits into
OpenJobDescription:mainfrom
mwiebe:fix/resolved-value-quick-followups

mwiebe commented Sep 25, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mwiebe commented Sep 25, 2026

What was the problem/requirement? (What/Why)

What was the solution? (How)

What is the impact of this change?

How was this change tested?

Was this change documented?

Is this a breaking change?

Does this change impact security?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants