Conversation
|
Quorum review (5 independent agents). 8 fixtures + README. Net: 2 clean GOOD (type-lowercase, and the widening pair is GOOD-WITH-NITS), 6 GOOD-WITH-NITS. The central contested point: the five int64 fixtures document behavior BOTH implementations exhibit (matches the sweep) and are safe data-integrity adoptions — but the base 2023-09 spec text never bounds (int64 lives only in the EXPR doc), so the spec-literalist reviewer voted all five NOT-GOOD as 'spec question misclassified as implementation bug'. Majority keeps them parked here, with the classification updated and a base-spec integer-bounds erratum filed as the promotion gate. Also: 2--type-lowercase duplicates PR #166's 2--type-lowercase-string (same construct, same defect) — dedupe across the two PRs. README quality is the best of the five expected-failures branches (genuine dual-implementation attribution). |
Spec-correct fixtures that FAIL against current reference implementations: the int64 acceptance family (default/minValue/maxValue/allowedValues at 2^63, default at -2^63-1), the base type-name case-sensitivity gate, and 1.2.1 merge-widening. Parked in proposed/ (not discovered by the runner) so merging keeps the suite green; per-fixture output, spec citation, and classification in proposed/README.md. Promote each fixture unchanged when its fix lands. Companion to conformance-base-gaps. Review: quorum-review fixes — restructured to kind-level proposed/ directories (<component>/<kind>/proposed/) so promotion is a mechanical one-directory move; README split per kind and corrected: the int64 family is now classified as a spec question gated on a base-spec integer-bounds erratum (the base doc states no bound; a bignum implementation is arguably conformant), with genuine per-implementation attribution; adopted jobs/proposed/2.3--int-value-above-int64-max.invalid.test.yaml from the base-gaps PR (openjd-rs rejects the supplied 2^63 but openjd-model 0.11.x accepts it — verified both this session), replacing the stale claim that it 'lives in the main suite'; noted the widening fixtures' scheduler-merge blast radius and that the duplicate type-lowercase pin in the param-types PR was dropped in favor of this branch's copy. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
1c1d3d1 to
f5837f2
Compare
|
Quorum-review fixes applied and pushed (rebased onto mainline 3069673):
|
…ong rule The two 1.2.1--constraint-widening-* fixtures parked here asserted that a Job Template widening an Environment Template's constraint must be refused. That is wrong. Issue OpenJobDescription#107 records that §1.2.1 constrains the merged result rather than each definition, so a wider later definition is allowed and has no effect. Both reference implementations already behave that way, and the maintainer review on this PR reached the same conclusion. A fixture that encodes the wrong reading is worse than no fixture, because it would make the divergence permanent. Measuring the correct assertion found a real defect that the wrong one hid. Both implementations apply the merged constraint to a supplied value. Only one applies it to a merged default: merged default below the merged minValue openjd-rs runs it, python refuses merged default below the merged minLength openjd-rs runs it, python refuses In each the default satisfies the Job Template's own bound, so the merged bound is the only thing making it invalid. openjd-rs prints COUNT[5] and VALUE[abc]. Cause, measured in openjd-rs at 951358be: preprocess_job_parameters (crates/openjd-model/src/job/create_job/parameters.rs) calls param.check_constraints in its supplied-value branch and never in its default branch, which coerces the default and inserts it. A default is only ever checked against the bounds of the template that declared it, at decode time. Three cases isolate it, and only the third diverges: default below its own minValue, no environment both refuse at validation default below its own minValue, wider environment both refuse at validation default satisfies its own, below the merged one openjd-rs runs it The third is why the existing live fixture jobs/1.2.1--merged-default-violates-constraint.invalid.test.yaml passes on openjd-rs for the wrong reason: its default violates the Job Template's own bound, so decode-time validation catches it before any merge is consulted. So the two replacements stay parked, but for a stated implementation reason rather than an unresolved spec question. One check_constraints call in the default branch closes both, and no spec decision is needed. The four supplied-value and accept fixtures that pin the intersection itself pass on both implementations and belong in the live directory, not here; they are on a separate branch. The proposed/ README section is rewritten to match: the cause, the three-case isolation, and why the earlier pair was withdrawn. Measured on openjd-rs at 951358be and openjd-cli 0.7.7 with openjd-model 0.11.11.post3+gd8174e3c3. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
…e suite
The eight fixtures on this branch were parked in base/*/proposed/, which the
runner does not scan, so nothing ran them. Move all of them into the live
directories, add four more that close measured gaps, and delete the two
proposed/ README inventories.
Both conformance workflows will go red. That is the point: the failures are
what these fixtures were written to record. Measured against openjd-cli 0.7.7
and openjd-rs main e4efd67 over the full 2023-09/base suite, Rust is 665/9 and
Python is 668/6, and the base suite has no other failures on either, so all 15
failures below belong to these 12 fixtures.
fixture python rust
2--type-lowercase pass pass
2.3--int-default-above-int64-max FAIL FAIL
2.3--int-default-below-int64-min FAIL FAIL
2.3--int-minvalue-above-int64-max FAIL FAIL
2.3--int-maxvalue-above-int64-max FAIL FAIL
2.3--int-allowedvalues-above-int64-max FAIL FAIL
2.3--int-default-above-int64-max-intstring FAIL pass
1.2.1--merged-default-below-merged-minvalue pass FAIL
1.2.1--merged-default-below-merged-minlength pass FAIL
1.2.1--merged-default-not-in-merged-allowedvalues-int pass FAIL
1.2.1--merged-default-below-merged-minvalue-float pass FAIL
2.3--int-value-above-int64-max pass pass
2--type-lowercase now passes both. openjd-rs #364 gated type-name case on the
EXPR extension, and that fix ships in the published openjd-cli 0.1.18 the Rust
lane installs. Its old README row saying openjd-rs accepts it was stale.
2.3--int-value-above-int64-max also passes both, but Python passes it for the
wrong reason. Probed directly: preprocess_job_parameters accepts the value,
create_job accepts it, and only create_job_with_symbol_tables rejects, with an
uncaught ValueError from the expr symbol-table build rather than validation.
The runner cannot tell, because an .invalid. fixture passes on any non-zero
exit.
Four fixtures new here, each probed against both CLIs with a control that both
accept, so each isolates one variable:
merged-default-not-in-merged-allowedvalues-int INT allowedValues is a
separate openjd-rs branch from STRING, which is why the live
1.2.1--default-must-satisfy-merged-constraints passes while this fails.
merged-default-below-merged-minvalue-float FLOAT is a separate
check_constraints arm from INT.
2.3--int-default-above-int64-max-intstring the string form is a separate
parse path. A Python fix that only bounds bare integers would pass the five
bare-integer fixtures and still accept this.
Every header is rewritten to spec, test, expectation in three paragraphs. The
two merged-default headers no longer narrate openjd-rs internals or cite twin
fixtures that live on an unmerged branch, so they stay accurate once a fix
lands.
Reviewer note on the six int64 fixtures. Section 2.3 types these properties as
<integer> and states no bound; the int64 range appears only in the Expression
Language type table. A bignum implementation is arguably conformant as the text
stands, so these assert a rule the base spec does not carry and should not
merge before a section 2.3 integer-bounds erratum. The case for that erratum is
measured: openjd-rs accepts default 9223372036854775808 and then resolves the
default to 9223372036854775807, silently.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Warning
The conformance workflows go red on this PR, deliberately. These fixtures were parked in
base/*/proposed/, which the runner does not scan, so nothing ran them. They are now live so thefailures are visible. Six of the twelve are gated on a spec erratum that does not exist yet and
should not merge before it lands. See "Do not merge as-is" below.
12 fixtures across two families, all
.invalid. Measured over the full2023-09/basesuite againstopenjd-cli 0.7.7 and openjd-rs
maine4efd67: Rust 665 passed / 9 failed, Python 668 passed /6 failed. The base suite has no other failures on either implementation, so every failure belongs to
this PR.
job_templates/2--type-lowercase.invalid.yamljob_templates/2.3--int-default-above-int64-max.invalid.yamljob_templates/2.3--int-default-below-int64-min.invalid.yamljob_templates/2.3--int-minvalue-above-int64-max.invalid.yamljob_templates/2.3--int-maxvalue-above-int64-max.invalid.yamljob_templates/2.3--int-allowedvalues-above-int64-max.invalid.yamljob_templates/2.3--int-default-above-int64-max-intstring.invalid.yamljobs/1.2.1--merged-default-below-merged-minvalue.invalid.test.yamljobs/1.2.1--merged-default-below-merged-minlength.invalid.test.yamljobs/1.2.1--merged-default-not-in-merged-allowedvalues-int.invalid.test.yamljobs/1.2.1--merged-default-below-merged-minvalue-float.invalid.test.yamljobs/2.3--int-value-above-int64-max.invalid.test.yamlA merged default is not checked against the merged constraint (4 fixtures)
Section 1.2.1 requires the merged job parameter definition to be internally consistent and names the
defaultas its example. Each fixture gives the job template, processed last, a default thatsatisfies its own bound but violates the bound merged in from an environment template. Python
refuses all four at job generation. openjd-rs runs them.
Rejection follows under both readings of "each constraint must become more constrained": the literal
one, and the intersection reading proposed in #107. These fixtures do not depend on #107 closing.
Each was probed with a control that both implementations accept, so each isolates one variable:
default: 50inside the merged[10, 100],default: abcdefat the merged minimum 5,default: 3inside the merged
[1, 2, 3],default: 15.0above the merged 10.0. The supplied-value equivalents(
-p Count=5,-p Value=abc) are refused by both, which is what leaves the default uncovered.Three of the four axes are separate code paths in openjd-rs, not one.
check_constraintshasseparate INT, FLOAT and STRING arms, and the default-vs-
allowedValuescheck invalidate_satisfiablecovers STRING and PATH only. That is why the live1.2.1--default-must-satisfy-merged-constraints.invalid.test.yaml(STRINGallowedValues) alreadypasses while the INT
allowedValuesfixture here does not. PATH is deliberately left uncovered:openjd-rs joins a PATH default to the template directory before checking, so the outcome would
depend on the runner's working directory.
INT and the int64 boundary (7 fixtures)
Both implementations accept
default,minValue,maxValueandallowedValuesat 2^63 anddefaultat -2^63-1.What the acceptance costs, measured: openjd-rs accepts
default: 9223372036854775808atopenjd checkand then resolves the default to9223372036854775807at run time. Silentsaturation, not an error. The Python CLI accepts the same template at
checkand dies atrunwithCannot convert '9223372036854775808' to int: number too large to fit in target type, raised from_serialize_symbol_tablein_create_job.py. An uncaughtValueError, not validation.jobs/2.3--int-value-above-int64-max.invalid.test.yamlpasses on both for that reason, and Pythonpasses it by accident. Probed directly:
preprocess_job_parametersaccepts the value,create_jobaccepts it, and only
create_job_with_symbol_tablesrejects. An earlier revision of this branchclaimed the Python CLI accepts it; that is true of
create_joband false of the CLI.The
<intstring>fixture is a falsifiability guard. openjd-rs parses the string form withstr::parse::<i64>and already rejects it. Python coerces both forms to an unboundedint, so afix in
_precheck_is_int_typethat only checksisinstance(value, int)would pass all fivebare-integer fixtures and still accept the string.
Type-name case (1 fixture, passes both)
2--type-lowercase.invalid.yamlwas parked as an openjd-rs bug. It is not one any more: #364 gatedtype-name case on the EXPR extension, and that fix ships in the published
openjd-cli0.1.18 theRust lane installs. Its positive twin is
EXPR/job_templates/2--type-case-insensitive.yaml.Do not merge as-is
The six int64 fixtures assert a rule the base spec does not state. Section 2.3 types these
properties as
<integer>and sets no bound; the int64 range appears only in the Expression Languagetype table, an EXPR document. A bignum implementation is arguably conformant as the text stands.
They need a section 2.3 integer-bounds erratum first, and no issue for it exists yet. The
saturation measurement above is the case for filing one.
The four merged-default fixtures and
2--type-lowercaseare ready. Splitting them out of this PR isthe fastest way to get a green base suite with the merge gap recorded.
Two runner limitations found while measuring
Neither is a defect in these fixtures.
run_jobreturnsresult.returncode == 0for an.invalid.fixture before it reads
expected, so a rejection for the wrong reason, or a crash, counts as apass. That is how the Python traceback above went unnoticed, and it means adding
expectedentriesto an
.invalid.fixture has no effect. Separately, a positive.test.yamlwith noexpectedblockfalls through to
return Truewithout consultingreturncode, so it passes even when the CLI exitsnon-zero.
Changes in this revision
Every header rewritten to spec, test, expectation. The merged-default headers no longer narrate
openjd-rs internals or point at twin fixtures that live on an unmerged branch, so they stay accurate
once a fix lands. Both
proposed/README.mdinventories deleted; their measurement data is in thisdescription. Four fixtures added. Non-ASCII characters removed from every fixture, since the runner
opens them at the locale encoding and a UTF-8 character is mojibake on Windows.