fix(model)!: Build an empty LIST[PATH] as the ListString it always was - #384
Open
leongdl wants to merge 9 commits into
Open
fix(model)!: Build an empty LIST[PATH] as the ListString it always was#384leongdl wants to merge 9 commits into
leongdl wants to merge 9 commits into
Conversation
preprocess_job_parameters refused a value it had produced. Given a LIST[PATH]
parameter with a default of [], the first pass returns an ExprValue::ListPath,
and feeding that back in as a submitted input value failed with
Parameter 'Empty': Cannot coerce list to LIST[PATH]
value_matches_type accepted (ListString, LIST[PATH]) and had no arm for
ExprValue::ListPath at all, so a LIST[PATH] value refused itself. Only the empty
case reached it: make_list infers ListString from String elements, so a non-empty
LIST[PATH] arrives on the ListString arm, and an empty list has no elements to
infer from and keeps the declared PATH element type.
Callers hit this because the PyO3 create_job binding re-runs
preprocess_job_parameters over whatever values it is handed, deliberately, to
match the v0 reference's behaviour of filling defaults inside create_job. A
caller that preprocesses and then calls create_job with the result therefore
preprocesses twice, and the second pass refuses the first pass's output. That
makes "every value this function emits is one it accepts" a real requirement,
which nothing stated and nothing tested.
Adds the missing arm and four tests that pin the round trip rather than the
matcher. A unit test on value_matches_type alone would pass against a build
where check_constraints had no ListPath arm either, so the tests go through
preprocess_job_parameters twice and assert the variant that comes back, not just
that a list of length zero came back.
Written test-first. Two of the four tests failed with the message above; the
other two are controls that passed before and after. Mutation-verified: removing
the arm fails both round-trip tests, widening it to LIST[STRING] fails the
type-confusion guard, matching only a non-empty ListPath fails both, and
dropping ListPath from the pre-existing ListString arm fails the non-empty
control.
Also records the round-trip requirement in specs/model/job-creation.md, since
it was load-bearing for callers and written down nowhere.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…st gaps Addresses the review on this PR, plus the findings of two independent test audits. The arm accepted any ExprValue::ListPath, not just the empty one it was written for. Measured: a caller-supplied ListPath(["/a/b.exr", "/a/c.exr"], Windows, 0) was accepted and stored verbatim, where it is refused today. That matters because Session::build_symbol_table re-applies path mapping to a LIST[PATH] only when the value is a ListString, with no else branch, so a non-empty ListPath would lose its Param.<name> binding at session scope with no error. Replaced the matches! arm with an early return that accepts only an empty list, keeping today's refusal for the non-empty case, and added a test pinning it. Test changes: - The round-trip template now carries a parameter with no default. A parameter with a default is re-filled on any pass, so a second pass that ignored its input still succeeded and every assertion still held -- a mutant the first audit found surviving. A no-default parameter can only be satisfied by the values carried over. - The all-list-types test asserted list_len() only, which answers Some(0) for every empty list variant, so a value that had discarded its declared element type passed. It now asserts the variant per parameter. - New test for a nested LIST[LIST[INT]] with one empty and one non-empty inner list; the previous coverage used an empty outer list, so the inner variant was never built. - New test for an empty LIST[PATH] submitted against minLength: 1. Accepting the empty value makes check_constraints reachable for input that was refused a step earlier, so the diagnostic a caller sees changed. Submitted rather than defaulted because decode refuses an empty default under minLength: 1 before preprocess sees it. - The non-empty round-trip test used absolute LIST[PATH] defaults, which pinned a pre-existing asymmetry as expected behaviour: both preprocess branches gate PATH default handling on the scalar type, so a scalar PATH default of "/abs/out" is refused while a LIST[PATH] default of ["/abs/out"] is accepted, as is ["../escape"]. Switched to relative defaults so a future fix there does not have to touch a test about round tripping. Filing that asymmetry separately. - Both negative tests now assert the whole diagnostic rather than two substrings. - Corrected a comment that cited the wrong line for the scalar-type gate, and one that claimed assert_carried_through is what catches a gutted second pass -- the expect on preprocess_again fires first. Dropped an assertion that could not fail. Mutation-verified, six mutants, all caught: dropping the emptiness guard, removing the early return, inverting the guard, widening it to LIST[STRING], gutting the resubmission, and returning a ListString for an empty LIST[BOOL]. cargo test --workspace, cargo clippy --all-features --all-targets -D warnings and cargo fmt --all are all clean. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
This was referenced Sep 11, 2026
Review round 2 found the justification over-broad, and reading openjd-sessions confirms it. Session::build_symbol_table has two paths and they disagree: the resolved-symtab path gates on `if let ExprValue::ListString(..)` with no else, so a ListPath is skipped and Param.<name> is never set, while the other path has a `_ => coerce_param_value(..)` fallback with s.set outside the match, so the binding is set. Saying a ListPath loses its binding at session scope full stop was wrong. The fix does not change. The argument for refusing a non-empty ListPath stands on its own: this crate never builds one, since a non-empty LIST[PATH] value is always a ListString, so accepting it would admit a shape only a caller can construct. The round-trip requirement is one-directional and says nothing about accepting shapes that are never produced. Also states that one-directionality explicitly in the spec, since two review findings read the paragraph as claiming more than it does. Files the three pre-existing defects review asked to be filed rather than left in comments: #385 LIST[PATH] defaults skipping the PATH containment checks including walk-up, #386 ExprValue::Path refused for PATH and LIST[LIST[INT]] accepting a wrong inner type, #387 the empty-LIST[PATH] binding gap above. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
…PATH
Review round 3 falsified the spec paragraph this PR added. Measured, twice through
preprocess_job_parameters with the second pass fed the first's output:
PATH, relative default "out", maxLength: 8
pass 1 -> "/var/.../template/out"
pass 2 -> REFUSED, "value length 72 exceeds maximum 8"
PATH, relative default "out", allowedValues: ["out"]
pass 2 -> REFUSED, "is not in allowed values"
Both controls pass: the same default with no constraint round trips, and a STRING
with maxLength round trips. The cause is that a PATH constraint is measured against
the unjoined default by validate_definition at decode and against the joined
absolute path by check_constraints at create, and the default branch of
preprocess_job_parameters does not constrain-check at all. The two stages disagree
by construction.
So "every value it returns is a value it accepts" was not true when I wrote it. The
spec now states it as a goal with the known violation named, and it is filed as
#388. This PR still closes the LIST[PATH] case it set out to close; it just no
longer claims more than it delivers.
Also corrects the mechanism named in the comment for the base: None path. Review
was right: session.rs:2497 is the LIST[PATH] arm's own inner fall-through,
`other => self.apply_path_mapping_to_value(other)`, and apply_path_mapping_to_value
has a real ListPath arm that maps every element. The `_ => coerce_param_value` arm
at :2499 belongs to the outer match on param_type and is never reached for a
LIST[PATH]. The conclusion is unchanged and slightly stronger: the resolved-symtab
path is the sole outlier. Issue #387 corrected the same way.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
25 comment lines sat above a 3-line `if let` in value_matches_type, a ratio of about 8 against the repo's stated budget of 0.5. Most of it was not a comment: it was the argument for the fix, the measurements behind it, the history of what an earlier revision got wrong, and two paragraphs of claims about openjd-sessions' symbol-table paths. Those openjd-sessions paragraphs are the clearest case for moving. They were the subject of two review rounds and were slightly wrong both times, because they describe another crate's internals and will drift again. A comment asserting something false is worse than no comment, since it stops the next reader from checking. The comment is now six lines carrying the invariant and the reason not to widen the match, and it points at the note. The test comments got the same treatment. Comment-to-code ratio across the added block is 0.24. Nothing was deleted. The argument, the measured numbers, the review exchange and the mutation table are in SuperDaveDocs pr/wip/openjd-rs-384, along with the probes. No behaviour change. cargo test --workspace, clippy -D warnings and fmt --check all clean. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
… value properly Review rounds 4 and 5, all four claims confirmed by reading the tree. There is no pyo3 anywhere in this workspace, so comments citing "the PyO3 create_job binding" named something a reader here cannot find. The justification does not need it: JobParameterInputValues is both the input and the output type and both are public, and create_job already re-runs check_constraints over every value it is handed (create_job/mod.rs:54-60). Reworded to those, which are checkable without leaving the repo. "Nothing produces a non-empty ListPath" was true of this module and the spec text generalised past where it holds -- openjd-sessions builds them at session.rs:2437, :2494 and :2626 when it re-applies path mapping. Scoped the claim. The non-empty test value was hand-built as ListPath(vec![..], Windows, 0), and that third field is a cached heap size make_list computes as v.len() * size_of::<String>() + sum of element lengths. Zero under-reports it, so the test was refusing a shape the crate cannot produce. Now goes through make_list over new_path elements. The minLength assertion used the two-substring pattern this PR rejects 67 lines above. Now asserts the exact diagnostic, which also pins "length 0". Also drops a comment prefix referencing a review artifact with no other occurrence in the tree, and replaces the SuperDaveDocs pointer with the in-repo spec section. An internal path resolves for nobody reading this repository; the note is linked from the pull request instead. Mutation table unchanged, six mutants all still caught. cargo test --workspace, clippy -D warnings and fmt --check clean. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Both were flagged by review and both were wrong on measurement. The spec justified the round trip by naming a PyO3 create_job binding that re-runs preprocess. There is no pyo3 dependency anywhere in this workspace, so the claim was unverifiable from the repo it sat in. Replaced with the in-repo reason: the input and output types are both public, and create_job re-runs check_constraints over every value it is handed (create_job/mod.rs:54-59) for every caller, with no second preprocess needed. The test banner already said this; the spec had been missed. The spec and the code comment both stated that an empty LIST[PATH] is a ListPath. Only the non-empty case is settled by the declared type. The ListString arm also accepts an empty ListString for a LIST[PATH] parameter and stores it verbatim, so an empty value of that type is either variant depending on how the caller expressed it. Measured, one parameter, three routes: default:[] gives ListPath([]), a submitted empty ListString stays ListString([]), a submitted empty ListPath stays ListPath([]), all three with param_type ListPath. That is why #387 is reachable two ways rather than one, since sessions' build_symbol_table gates on the ListString variant. Filed the normalization question as #389 rather than folding it in: it changes public output for a shape that works today, and the choice of which variant to keep should be made together with #387. Comment is 8 lines over a 30-line function, ratio 0.27. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
seant-aws
previously approved these changes
Sep 11, 2026
AlexTranAmz
previously approved these changes
Sep 11, 2026
leongdl
enabled auto-merge (squash)
September 11, 2026 23:19
mwiebe
reviewed
Sep 11, 2026
| fn value_matches_type(value: &openjd_expr::ExprValue, param_type: JobParameterType) -> bool { | ||
| use openjd_expr::ExprValue; | ||
| // Empty-only, deliberately. `make_list` reads String elements as a `ListString`, so a | ||
| // non-empty LIST[PATH] is a `ListString`, while `default: []` yields a `ListPath` this |
Contributor
There was a problem hiding this comment.
I think we should fix this where [] is becoming list[path] instead of list[string]? While parsing the parameters, this is in RawParam form which is a string for path parameter types.
Contributor
Author
There was a problem hiding this comment.
Interesting, let me try and check if this is possible.
| **Round trip:** a value `preprocess_job_parameters` returns should be a value it accepts as input. | ||
| Both its input and output types are public, so a caller can hand its output straight back to it. | ||
| `create_job` does something adjacent already, and for every caller: it re-runs `check_constraints` over | ||
| each value it is given (`create_job/mod.rs:54`-`:59`), with no second preprocess required. So the |
Contributor
There was a problem hiding this comment.
The claim that create_job re-runs check_constraints "over each value it is given ... for every caller" is the load-bearing premise for the requirement in the next sentence, and it is narrower than written: that recheck is job-template-scoped, not value-scoped.
create_job re-merges from scratch with an empty environment-template slice:
let merged = parameters::merge_job_parameter_definitions(job_template, &[])?;
for param in &merged {
if let Some(jpv) = job_parameter_values.get(¶m.name) {
param.check_constraints(&jpv.value)?;(create_job/mod.rs:56-:59). It iterates merged, not job_parameter_values, and merged comes from job_template alone — the signature has no environment-template parameter, so it cannot do otherwise. Two consequences:
- A parameter declared only in an environment template is in
job_parameter_valuesbut not inmerged, socheck_constraintsnever runs for it at create time. - For a parameter declared in both, the constraints differ.
merge_constraintstightens across templates —min_lengthtakes the max,max_lengththe min,allowed_values_stris intersected (parameters.rs:181-:196) — so the definitionpreprocess_job_parameterschecked against is at least as tight as the onecreate_jobrechecks against, and can be strictly tighter. The second pass is a weaker check, not a repeat of the first.
Both in-tree callers hit this: openjd-cli/src/run/execution.rs:109 and summary.rs:91 pass &env_templates to preprocess_job_parameters, then call create_job, which sees &[]. Enforcement of env-contributed constraints lives in exactly one place — the first pass — which is the opposite of the "with no second preprocess required" framing.
This matters for this section specifically, because it is the sentence that turns a round-trip property into a constraint requirement. As written, a reader concludes create_job is a backstop catching anything preprocess emitted in violation; for env-merged constraints there is no backstop, and JobParameterValues being public means calling create_job with stored values and no preprocess pass is a reachable shape. The doc comment on create_job says env parameters "should already be merged in via preprocess_job_parameters" — i.e. it trusts the caller for exactly the constraints it cannot re-derive.
Suggest scoping the sentence, e.g. "re-runs check_constraints for every parameter the job template declares, against job-template constraints only" — the unqualified version is what makes the requirement in the following sentence look already-enforced.
Minor: the line reference looks off by two — the loop is create_job/mod.rs:56-:60, not :54-:59.
Replace the accept-arm this branch added to `value_matches_type` with a fix at the site that built the odd value. `coerce_json_to_job_parameter_type` coerces each element of a list parameter to the declared element type, and a PATH element coerces to an `ExprValue::String`. So a `LIST[PATH]` is a `ListString`, which is what `make_list` infers for any non-empty one. An empty list has no elements to infer from and takes `make_list`'s hint instead, and the hint passed was `element_type.expr_type()` -- `PATH`. That is the one case where the hint disagreed with the elements it stood in for, and it produced a `ListPath` that nothing else in the crate builds. Passing `STRING` for a PATH element type removes the divergence rather than accommodating it, so the accept-arm and its empty-only special case are gone and the rule is one rule: a `LIST[PATH]` value is a `ListString`, and an `ExprValue::ListPath` is a caller-only shape refused at any length. It also leaves a `LIST[PATH]` parameter with a single stored representation, which closes #389 and, for the empty case, the `ListString`-only gate in `openjd-sessions::build_symbol_table` that is #387. Behaviour change, not a refactor: three tests asserted the `ListPath` variant and one submitted a hand-built empty `ListPath` to reach the length rule. All four move to `ListString`, and `a_non_empty_list_path_value_is_still_refused` becomes `a_submitted_list_path_value_is_refused_at_any_length` now that both lengths follow the same rule. `preprocess_accepts_its_own_empty_list_path_output` gains a first-pass assertion, so a fix that only taught the second pass to accept a `ListPath` fails there rather than passing. Verified: 4 mutants, all caught -- revert the hint, widen it to every type, drop `ListPath` from the `ListString` arm, and re-add the removed accept-arm. The reverted hint is caught independently by the `parameters.rs` unit test. `cargo test --workspace`, clippy with `-D warnings` and `cargo fmt --check` are clean, and the conformance suite is 1178 passed / 1 failed, the failure being `7.5--numeric-string-zeros-in-range-elements`, which fails identically with this change reverted. BREAKING CHANGE: `preprocess_job_parameters` now returns an `ExprValue::ListString` rather than an `ExprValue::ListPath` for an empty `LIST[PATH]` job parameter. Its output type is public, so a caller matching on the variant for that case sees the change. Non-empty `LIST[PATH]` values were already `ListString` and are unaffected. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
leongdl
changed the title
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
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #389. Found by
EXPR/job_templates/2.12--list-path-paramin the OpenJD conformance suite, which declares 17LIST[PATH]parameters and could not create a job because one of them hasdefault: [].What was the problem/requirement? (What/Why)
preprocess_job_parametersrefused a value it had produced itself.Give it a
LIST[PATH]parameter whose default is[]. The first pass returned anExprValue::ListPath. Feed that back in as a submitted input value and it failed:That matters because callers feed this function its own output.
create_jobre-runscheck_constraintsover every value it is handed, and a caller that preprocesses and then callscreate_jobwith the result preprocesses twice. So every value this function emits must be a value it accepts. Nothing stated it and nothing tested it.The
ListPathwas the anomaly, not the refusal.coerce_json_to_job_parameter_typecoerces each element of a list parameter to its declared element type, and a PATH element coerces to anExprValue::String. So aLIST[PATH]is aListString, which is whatmake_listinfers for any non-empty one. An empty list has no elements to infer from and takesmake_list's hint instead — and the hint passed waselement_type.expr_type(),PATH. That is the one case where the hint disagreed with the elements it stood in for, and it built aListPaththat nothing else in the crate builds.What was the solution? (How)
Pass
STRINGas the hint for a PATH element type:Every other type keeps its declared element type, which is still what makes
LIST[BOOL]of[]aListBoolrather than an untypedListList.This replaces an earlier revision of this PR, which added an empty-only arm to
value_matches_typeso the second pass would accept theListPath. Fixing the producer is strictly better and that arm is now deleted, along with its empty/non-empty special case. The rule is one rule:Refusing it is unchanged from
main. Production diff is +7 / −11 lines.It also leaves a
LIST[PATH]parameter with a single stored representation, which is what #389 asked for and which closes it. Measured, one parameter, three routes:default: []ListString([], 0)make_list([], STRING)ListString([], 0)make_list([], PATH)All three rows are pinned by tests. That also closes #387 for the empty case:
openjd-sessions::build_symbol_tablegates its wholeLIST[PATH]body onif let ExprValue::ListString(..)with noelse, so a storedListPathleftParam.<name>unbound at session scope. The generalno elsedefect there stays open as #387.What is the impact of this change?
A
LIST[PATH]job parameter with an empty default can be used. Today any caller that passespreprocess_job_parameters' output back into it cannot create such a job at all.How was this change tested?
Yes, unit tests. The clearest evidence the fix lands at the source: with the pre-existing tests unchanged,
preprocess_accepts_its_own_empty_list_path_outputfailed withThe round trip itself succeeded. Only the old variant assertion failed.
Four tests changed, which makes this a behaviour change rather than a refactor. Three asserted the
ListPathvariant and one submitted a hand-built emptyListPathto reach the length rule; all four move toListString.a_non_empty_list_path_value_is_still_refusedbecomesa_submitted_list_path_value_is_refused_at_any_length, now looping over both lengths since they follow one rule.preprocess_accepts_its_own_empty_list_path_outputgains a first-pass assertion, so a fix that only taught the second pass to accept aListPathfails there rather than passing.Mutation-verified. Each mutant was applied, confirmed to be a real on-disk change, confirmed to compile, tested, and restored from an in-process snapshot verified by checksum, against a green baseline:
element_type.expr_type()parameters.rsunit testSTRINGfor every element typeLIST[BOOL],LIST[INT],LIST[FLOAT],LIST[LIST[INT]]all refuse their own outputListPathfrom theListStringaccept-armFour mutants, four caught, none surviving.
cargo test --workspace,cargo clippy --all-features --all-targets --workspace -- -D warningsandcargo fmt --all -- --checkare clean. The full OpenJD conformance suite at the openjd-specifications tip (37c8353) is 1182 passed, 0 failed against the release binary built from this commit, includingEXPR/job_templates/2.12--list-path-paramand the2.12--list-path-*execution fixtures. CI is green on all 22 checks, including the three Conformance lanes.Was this change documented?
Yes. The hint carries a four-line comment giving the invariant and the reason not to put
PATHback, and the missingListPatharm carries a two-line note saying why it is absent.specs/model/job-creation.mdgains the round-trip requirement under Value coercion, stated as a goal with its one known violation measured: a scalarPATHwith a relative default plus a length orallowedValuesconstraint (#388).An earlier revision's comment asserted that
openjd-sessionsdrops aListPathwhen bindingParam.<name>. That holds for the resolved-symtab branch only, not the from-scratch branch, so the code comment no longer claims it and the precise version lives in the spec.Is this a breaking change?
Yes, marked
fix(model)!with aBREAKING CHANGEfooter.preprocess_job_parametersnow returns anExprValue::ListStringrather than anExprValue::ListPathfor an emptyLIST[PATH]parameter. Its output type is public, so a caller matching on the variant for that case sees the change; the fix is to matchListString, which is what non-empty values of that type already were. Nothing else changes, and no public signature changes.Does this change impact security?
No. It does not touch path resolution, path containment, the
allow_template_dir_walk_upcheck, or file access.a_list_path_value_is_still_refused_for_a_list_string_parameterpins that aLIST[PATH]value still cannot satisfy aLIST[STRING]parameter, since the two differ in what session-time path mapping does to them.Review history
Five rounds of automated review ran against the earlier
value_matches_typeapproach and all 17 threads were answered. Four findings were correct and are fixed: the comments cited a PyO3 binding that does not exist in this workspace, namedcoerce_param_valueon a path aListPathcannot reach, claimed only this workspace's callers build a non-emptyListPathwhenopenjd-sessionsbuilds them in three places, and stated that an emptyLIST[PATH]is always aListPathwhen a submitted emptyListStringwas also accepted and kept. That last finding is what led here: it is the two-representations problem, filed as #389, and this revision fixes it at the producer instead of documenting it. Four further findings were correct but out of scope and are filed as #386, #387, #388 and #389.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.