fix(model): Accept Task.File property access in format strings - #292
Open
leongdl wants to merge 1 commit into
Open
fix(model): Accept Task.File property access in format strings#292leongdl wants to merge 1 commit into
leongdl wants to merge 1 commit into
Conversation
RFC 0005 declares `Task.File.<name>` as `path` typed and shows property
access on one directly inside a format string:
echo "Script: {{Task.File.Run.name}}"
The step-script validator rejected that form. `expression_names()` returns
the whole bare dotted lookup (`Task.File.Run.name`), and the validator
stripped only the `Task.File.` prefix, then looked the entire remaining
tail up as an embedded-file key. `.name` was folded into the filename, so
a template using the form the RFC documents failed validation with
"references undefined embedded file 'Run.name'" -- even though the
runtime resolves the expression correctly and the environment-template
path accepts the equivalent `Env.File.<name>.name`.
Only the first dotted segment is the embedded-file key; the rest is
property access on the resulting path value. Embedded file names are
identifiers and cannot contain a dot, so the split is unambiguous.
Undefined embedded files are still rejected, and the error now names the
file key ('Missing') rather than the property tail ('Missing.name').
Found while reviewing RFC 0008 wrap-action support across the two
implementations; covered by a new conformance fixture in
openjd-specifications#156.
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
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.
Problem
RFC 0005 declares
Task.File.<name>aspathtyped and shows property access on one directly inside a format string:The step-script validator rejected that form.
FormatString::expression_names()returns the whole bare dotted lookup (Task.File.Run.name), and the validator stripped only theTask.File.prefix and then looked the entire remaining tail up as an embedded-file key. The property access got folded into the filename, so a template using the form the RFC documents failed validation:This was inconsistent in two directions: our own runtime resolves the expression correctly, and our environment-template path accepts the equivalent
Env.File.<name>.name.Reproducer
Save as
repro.yamland runopenjd check repro.yaml:before: ERROR ... references undefined embedded file 'Run.name'.(exit 1)after: Template at 'repro.yaml' passes validation checks.(exit 0)Running it produces
NAME:run.txtandSUFFIX:.txt.Fix
Only the first dotted segment after
Task.File.is the embedded-file key; anything further is property access on the resultingpathvalue:Embedded file names are identifiers and cannot contain a dot (enforced a few lines below in the same function), so the split is unambiguous.
split_onceis used rather thansplit('.').next()because the latter needs a fallback arm that is unreachable — dead code that no test can ever pin.Undefined embedded files are still rejected, and the error now names the file key (
'Missing') rather than the property tail ('Missing.name').Conformance
This closes divergence 1 of the two tracked in the draft spec PR:
Both fixtures from that draft now pass against this branch:
Against pre-fix
mainthe first one fails at validation, never reaching the runtime:Full conformance suites on this branch, with #156's fixtures applied:
2023-09/base/*2023-09/EXPR/*Fixture 2 (integer literal above 2^63-1) already passed here — that one is owned by
openjd-model-for-python. #156 can be un-drafted once that side lands.Tests
10 tests in
crates/openjd-model/tests/integration/test_embedded.rs, written before the fix and confirmed failing against unfixed code.Accepted: bare
{{Task.File.Run}},.name,.suffix,.parent, chained.parent.name, and property access inonRun.command(not justargs).Rejected (negative controls): unknown file with and without a property tail, unknown file referenced from
command, and a dotted embedded-file name — the last pins the invariant the first-segment split depends on.Every test was mutation-checked; all 7 mutants are caught:
task_file_nested_property_chain_acceptedonRun.commandtask_file_unknown_name_in_command_rejectedThe last two mutants initially survived, which is why the chained-property and
command-position tests exist. Nothing in the previous 1510-test suite pinned thatonRun.commandwas scanned at all.Three of the ten tests pass against unfixed code by design — they are controls that pin invariants rather than this regression.
Verified: full workspace suite green,
clippy --all-targets -- -D warningsclean,cargo fmt --checkclean.Follow-ups, not in this PR
Base-mode property gating. Base (no
EXPR) should reject path property access, and openjd-rs accepts it — for every path-typed symbol, not justTask.File. Verified against the Python implementation:EXPRextension{{Session.WorkingDirectory.name}}{{Param.SomePathParam.name}}{{Task.File.Run.name}}The first two never touch this code path, so this gap is pre-existing and general; it belongs in the expression/type-check layer, not in the embedded-file existence check. Gating it here would fix one symbol and leave the others divergent, while preserving the misleading "undefined embedded file" wording. No
base/conformance fixture currently exercises path property access, so nothing in the suite regresses either way.A third divergence, found while verifying this one. Under
EXPR, Python accepts{{Task.File.Run.bogusproperty}}at validation time; openjd-rs rejects it with "Cannot access attribute 'bogusproperty' on path". openjd-rs is correct here — the spec defines a fixed property set — and this is currently uncovered by the conformance suite. Worth a fixture in #156 or a follow-up.