fix: Render a PATH parameter in the host's path format - #364
Merged
leongdl merged 2 commits intoSep 4, 2026
Conversation
jericht
approved these changes
Sep 3, 2026
AlexTranAmz
approved these changes
Sep 3, 2026
leongdl
enabled auto-merge (squash)
September 3, 2026 23:41
A session is host scope, so a `path` there takes the host operating system's semantics -- separators included (Expression Language 1.2.1: "In host contexts (SESSION and TASK scopes), the semantics match the host's operating system"). This package applied the host's separators only *inside* a matching path mapping rule, in `PathMappingRule.apply`, so that was the only thing that ever chose one. A PATH parameter that no rule matched reached the task in whatever form the submitter wrote it. Measured on a Windows host with no rules, `Task.Param.InputFile` for `/path/a.exr` was `/path/a.exr`, where conformance fixture 2023-09/base/jobs/3.4--path-parameter.test.yaml requires `\path\a.exr`; with a rule present but not matching, the same. That fixture has been failing on the Python conformance job's windows-latest leg continuously since 2026-05-22. openjd-rs applies the format unconditionally, wrapping both the mapped and the unmapped case in `ExprValue::new_path(mapped, PathFormat::host())` for `Param.<PATH>`, `Task.Param.<PATH>` and each element of the LIST[PATH] forms, while leaving `RawParam.*` raw. `processed_parameter_value` now mirrors that: `to_host_path_separators` wraps `apply_mapping` on both PATH branches, which is idempotent for a value a rule did match and so keeps the two paths agreeing. `to_host_path_separators` duplicates Rust's `normalize_path_separators` rather than calling it through `openjd.expr`. The fixture that pins this is a base-spec template, so the fix sits on the non-EXPR path that test/openjd/ test_import_purity.py protects, and the native extension must not become a load-time requirement there. The URI arm reuses this module's existing `_URI_SOURCE_RE`, so only Rust's three-way branch is restated. It lives in `_path_mapping` and reads that module's `os_name` deliberately: `apply` reads the same name, and deriving the host twice would let the two disagree. Separators and nothing else. Rendering through PureWindowsPath would satisfy the headline assertion and still be wrong -- measured, it turns `s3://bucket/key` into `s3:\bucket\key`, collapses `/a//b` to `\a\b`, drops the trailing separator that `apply` deliberately preserves, and renders `""` as `.`. 17 tests, written first and failing 7/17 against the unmodified source. Full suite unchanged apart from those 7 (15 failed/996 passed -> 8 failed/1003 passed, the 8 pre-existing and environmental). All 7 mutants caught, including one that formats `RawParam.*` too and one that swaps in PureWindowsPath. Windows is exercised through a patched seam rather than on Windows, so the end-to-end confirmation is 3.4--path-parameter's next windows-latest result. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Four findings from the automated review. Three were correct, one was not. **Correct.** The comment claiming this is "idempotent for a value a rule did match" overclaimed. It holds for a POSIX- or WINDOWS-format rule, whose output `apply` rebuilds through `PureWindowsPath`, but not for a URI-format rule: `_apply_uri` copies `destination_path` verbatim and uses the host separator only for the appended child parts. Measured, a URI rule with a POSIX-spelled destination on a Windows host gives `/tmp/openjd\scene\out` from `apply` alone, which this then completes to `\tmp\openjd\scene\out`. That is a real behaviour change on a matched rule and no test covered it. It is also exactly what openjd-rs produces for the same mapped string, measured through `new_path(mapped, host())`, so completing it is the intent rather than a side effect. The comment now states both cases and `test_a_matching_uri_rule_is_completed_not_left_mixed` pins them, asserting the `apply`-only value first so a future change to `_apply_uri` says which half moved. **Correct.** Two maintainability points: `to_host_path_separators` was defined above the `_URI_SOURCE_RE` it reads, which worked only because the global is resolved at call time; it now follows the regex. And the test module's docstring named a `host_path_format` that never shipped -- an earlier name for this function -- so a reader following it found nothing. **Not an error.** The review flagged `_URI_SOURCE_RE`'s one-character scheme as misclassifying a Windows drive letter, since `C://Users/foo` is treated as a URI and returned unchanged while `C:/Users/foo` is converted, and asked whether openjd-rs guards against a one-character scheme. It does not. Measured through the engine, openjd-rs renders `C://Users/foo`, `C:/Users/foo`, `x://y/z` and four other cases identically to this function -- 7/7 agreement. RFC 3986 §3.1 admits a single-character scheme and the Expression Language states the pattern as `^[a-zA-Z][a-zA-Z0-9+.-]*://`, so this is the specified behaviour, and tightening the regex would make this package diverge from the implementation it is being aligned with. Pinned deliberately by `test_a_one_character_scheme_matches_the_engine` so it is not later "fixed" into a divergence; if the behaviour is wrong it is wrong in the specification. Also, per review feedback, the three branches now carry the openjd-expr source they were transcribed from -- `normalize_path_separators` in `crates/openjd-expr/src/value.rs` -- quoted inline so the copy can be diffed against the original without leaving the file. `normalize_path_separators` and the `uri_path::parse` behind its `is_uri` are byte identical in openjd-expr 0.5.0 and 0.6.0, checked against both crates.io sources rather than assumed. No production logic changed: the diff is one moved function, comments, and tests. 21 tests now (was 17), all 7 mutants still caught, and the full suite is unchanged at the same 8 pre-existing environmental failures with 1007 passing (was 1003). Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
leongdl
force-pushed
the
fix/path-param-host-format
branch
from
September 3, 2026 23:41
e9b5315 to
c7bbc22
Compare
seant-aws
approved these changes
Sep 3, 2026
Merged
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.
Fixes: no GitHub issue; found from the Python conformance job's
windows-latestleg, which has been red onopenjd-specificationsmainline continuously since 2026-05-22.What was the problem/requirement? (What/Why)
A session is host scope, so a
paththere takes the host operating system's semantics — separators included. Expression Language §1.2.1: "In host contexts (SESSION and TASK scopes), the semantics match the host's operating system."This package applied the host's separators only inside a matching path mapping rule, in
PathMappingRule.apply, so that was the only thing that ever chose one. APATHparameter that no rule matched reached the task in whatever form the submitter wrote it.Measured on
mainline(98d3e95) withopenjd-model0.11.6, building a session symbol table for aPATHparameter of/path/a.exr:Task.Param.InputFile/path/a.exr/path/a.exr— wrong/pathC:\path\a.exr/path/a.exr— wrongConformance fixture
2023-09/base/jobs/3.4--path-parameter.test.yamlrequiresTASK:InputFile=\path\a.exron Windows and/path/a.exron POSIX. It carries bothoutput_posixandoutput_windows, so it states both readings; we satisfied one.The fixture declares no
extensions, so this is reachable from a base-spec template with no expression evaluation involved.Worth noting:
record_expr_types' docstring already asserted the behaviour the code did not have — "Param.*carries the declared type (a PATH is a host-format path value with path mapping applied)".What was the solution? (How)
processed_parameter_valuenow wrapsapply_mappingon bothPATHbranches with a newto_host_path_separators. Applying it afterapply_mappingis idempotent for a value a rule did match (\has no/left to replace), which is what keeps the mapped and unmapped paths agreeing rather than making them two rules again.This mirrors openjd-rs, which applies the format unconditionally on all four surfaces:
crates/openjd-sessions/src/session.rs)Param.<PATH>,Task.Param.<PATH>ExprValue::new_path(mapped, PathFormat::host())LIST[PATH]formsnew_path(m, host())per elementRawParam.*,Task.RawParam.*param.value.clone()Two deliberate choices a reviewer should weigh:
It duplicates Rust's
normalize_path_separatorsrather than calling it.str(ExprValue(v, "path", PathFormat.WINDOWS))reaches the same Rust function through the bindings and cannot drift — measured, it is exactly right. Buttest/openjd/test_import_purity.pyrequires that the native extension not become a load-time requirement ofopenjd.sessions, at import time or while resolving a non-EXPR template, and per above the fixture that pins this is a non-EXPR template. So the fix sits squarely on the path that test protects. The URI arm reuses this module's existing_URI_SOURCE_RE, so only Rust's three-way branch is restated. This is the same tradeopenjd.model._format_strings._parseralready makes, which that test's own docstring cites as precedent.It lives in
_path_mappingand reads that module'sos_name, not_session'sos.name. Those are two spellings of "which host is this", andPathMappingRule.applyreads the first. Deriving the host twice would let a caller — or a test — patch one and not the other, producing a self-inconsistent host where mapped values render Windows and unmapped ones render POSIX.Not
PureWindowsPath, which is already imported in the module and would satisfy the headline assertion while being a different transformation. Measured:str(PureWindowsPath(..))s3://bucket/keys3://bucket/keys3:\bucket\key— corrupts the URI/a//b\a\\b\a\b— collapses/a/\a\\a— drops the trailing separator"""".— invents a pathThe trailing-separator row is the sharpest:
applygoes out of its way to preserve a trailing separator (_has_trailing_slash), so aPurePathnormalization applied after it would undo a behaviour this package deliberately implements.What is the impact of this change?
On a Windows host, a
PATHorLIST[PATH]parameter that no path mapping rule matched now reaches the task with\separators instead of/. That is the conforming behaviour and the point of the change, but it is a behaviour change on Windows fleets and consumers should know: a caller that was relying on the POSIX spelling surviving to the task will see the change. POSIX hosts are unaffected — the POSIX arm is a no-op, because a backslash is a legal character in a POSIX filename and rewriting one would corrupt the path.RawParam.*andTask.RawParam.*are unchanged, per the oracle table above. Non-PATHtypes are unchanged, including aSTRINGwhose value happens to look like a path.How was this change tested?
Yes, unit tests have been run.
New:
test/openjd/sessions_v0/test_path_parameter_host_format.py, 17 cases, written before the fix and run against unmodified source first — 7 failed, 10 passed. The 10 were the negative controls, which the pre-fix code already satisfied; a file where everything failed would not distinguish the defect from a broken harness. After the fix: 17 passed.A Windows host has to be simulated, because a POSIX host renders both readings identically and a value comparison here would pass whatever the code did. The tests patch one seam,
_path_mapping.os_name, for the reason given above.Full suite, both sides, same machine and interpreter (macOS, Python 3.13.7), with the test file present in both runs so the comparison isolates the source change:
src/unmodifiedSet difference on failing test IDs: the 7 pinning tests moved failed → passed, nothing newly broken, same 8 failures both sides. Those 8 are pre-existing and environmental, confirmed by running them against the unmodified tree in isolation — 6 ×
test_subprocess.py::test_run_gracetime_when_process_ends_but_grandchild_uses_stdout, and 2 ×test_session_scenarios.py::test_scenariowhich fail with'python': No such file or directorybecause this machine has no barepythononPATH.test_session.py::test_params_are_path_mapped, the existing test over the same function, passes unmodified.Mutation testing, after the tests passed: baseline green at 17, then all 7 mutants caught, 0 survived.
PATHcall site drops the wrapperLIST_PATHcall site drops the wrapperRawParam.*formatted tooPureWindowsPathinstead of separator replacementThe counts are the useful part. They are all different, and three are 1, so M3, M5 and M6 are each caught by one specific assertion rather than by everything collapsing at once. M6 catches over-correction and M7 the plausible wrong fix; I expected M7 to survive and it did not.
ruff checkandmypy src testare clean. Noteruff formatalso wants to reformat one unrelated line in_session.py—mainlineitself is unclean under the pinnedruff == 0.15.*(0.15.22), so that drift is pre-existing and I have deliberately left it out of this PR.What is not tested: Windows, on Windows. Everything above is measured, but the Windows half is measured through a patched seam. The end-to-end confirmation is
3.4--path-parametergoing green on the nextwindows-latestconformance run.Was this change documented?
Yes.
to_host_path_separatorscarries a docstring giving the three arms, the openjd-rs function it duplicates, why it is a duplicate rather than a call, and whyPureWindowsPathis not used. The call site records why the format is applied to the unmapped case too. The test module's docstring explains the Windows simulation and why one seam is the right number.Is this a breaking change?
Not to a public contract, but it is a behaviour change on Windows — see What is the impact above. No signature, export or type changes;
to_host_path_separatorsis new in a private module. A consumer on a Windows fleet that depended on the previous (non-conforming) POSIX spelling reaching the task will observe the difference, and should treat this as the correction it is.Does this change impact security?
No new files or directories are created, and no permissions are changed. The change is a string transformation on a parameter value already flowing through this code path.
One thing worth a reviewer's eye rather than a threat model: the transformation is applied after path mapping, so it cannot affect which rule matches or defeat a path mapping rule — matching happens first, in
apply, and is untouched. It also deliberately does not normalize..or collapse separators, so it cannot change what a path resolves to.Cross-port to openjd-rs
PathFormat::host()unconditionally incrates/openjd-sessions/src/session.rsand passes3.4--path-parameteronwindows-latesttoday. This PR brings Python to Rust's behaviour, so there is nothing to port back.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.