Skip to content

chore(deps): Bump openjd-* Rust crates to 0.7.0 #359

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
leongdl merged 2 commits into from
Sep 10, 2026
Merged

chore(deps): Bump openjd-* Rust crates to 0.7.0 #359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8799642
chore(deps): Bump openjd-* Rust crates to 0.7.0
leongdl Sep 10, 2026
8cd99f1
docs: Sync the validate_expressions docs and note the Zs control
leongdl Sep 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions THIRD-PARTY-LICENSES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2534,9 +2534,9 @@ limitations under the License.
** itoa; version 1.0.18 -- https://crates.io/crates/itoa
** libc; version 0.2.189 -- https://crates.io/crates/libc
** manyhow-macros; version 0.11.4 -- https://crates.io/crates/manyhow-macros
** openjd-expr; version 0.6.0 -- https://crates.io/crates/openjd-expr
** openjd-model; version 0.6.1 -- https://crates.io/crates/openjd-model
** openjd-sessions; version 0.5.6 -- https://crates.io/crates/openjd-sessions
** openjd-expr; version 0.7.0 -- https://crates.io/crates/openjd-expr
** openjd-model; version 0.7.0 -- https://crates.io/crates/openjd-model
** openjd-sessions; version 0.5.7 -- https://crates.io/crates/openjd-sessions
** pin-project-lite; version 0.2.17 -- https://crates.io/crates/pin-project-lite
** portable-atomic; version 1.15.0 -- https://crates.io/crates/portable-atomic
** proc-macro2; version 1.0.107 -- https://crates.io/crates/proc-macro2
Expand Down
6 changes: 3 additions & 3 deletions rust-bindings/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ name = "_openjd_rs"
crate-type = ["cdylib", "rlib"]

[dependencies]
openjd-expr = "0.6.0"
openjd-model = "0.6.1"
openjd-sessions = "0.5.6"
openjd-expr = "0.7.0"

@github-actions github-actions Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bump silently breaks make_list_err_to_py's message rewriting, because openjd-rs#373 changed the wording of the unresolved-element error in addition to the validate_expressions signature the PR description covers.

rust-bindings/src/expr/expr_value.rs:40-48 parses the upstream headline:

if let Some(rest) = headline.strip_prefix("make_list expected ") {
    if let Some((expected, got_part)) = rest.split_once(" element, got ") {
        if got_part == "unresolved" { /* reference-parity message */ }

In 0.6.0 there was no dedicated unresolved check in make_list, so an unresolved element fell through to the per-type element match and produced make_list expected int element, got unresolved — which matches " element, got ", sets got_part == "unresolved", and yields the documented reference message.

0.7.0 adds an up-front check that short-circuits before the per-type match (crates/openjd-expr/src/value.rs):

if elements.iter().any(Self::is_unresolved) {
    return Err(ExpressionError::type_error(
        "make_list expected concrete elements, got unresolved",
    ));
}

strip_prefix still succeeds, leaving rest == "concrete elements, got unresolved". That does not contain " element, got " — the text is elements,, so the s breaks the literal — so split_once returns None and the code falls through to headline.to_string(). ExprValue([42, ExprValue.unresolved(ExprType("int"))]) therefore now raises

TypeError: make_list expected concrete elements, got unresolved

instead of the parity message the doc comment on make_list_err_to_py and TestExprValueListConstructionErrors (test/openjd/expr/test_lists.py:822-847) both state is the contract: Cannot construct a list containing unresolved values. Use ExprValue.unresolved() ....

The two regression tests there only assert match="unresolved", and the raw upstream headline happens to contain that word, so both still pass — which is presumably why this went unnoticed. Worth adding the concrete elements spelling to the prefix handling (or matching on got unresolved rather than the " element, got " infix, so a future upstream rewording degrades to the parity message rather than leaking crate-internal make_list phrasing to Python callers), and tightening the tests to assert the full expected message so the next bump catches this.

openjd-model = "0.7.0"
openjd-sessions = "0.5.7"
tokio = { version = "1", features = ["rt-multi-thread"] }
uuid = { version = "1", features = ["v4"] }
serde_json = "1"
Expand Down
14 changes: 11 additions & 3 deletions rust-bindings/src/expr/format_string.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,15 @@ impl PyFormatString {
/// checking through the expression tree.
///
/// Mirrors the Rust crate's
/// `FormatString::validate_expressions(symtab, lib)`. Returns
/// `None` on success.
/// `FormatString::validate_expressions(symtab, lib, target_type)`.
Comment thread
leongdl marked this conversation as resolved.
/// Returns `None` on success.
///
/// `target_type` is passed as `None` because `resolve` and
/// `resolve_string` above resolve without one; validation has to
/// observe the same values resolution will produce. The crate
/// returns a `StaticResolution` (resolved-length bound and, when
/// fully concrete, the resolved value); this binding is pass/fail
/// only and discards it.
#[pyo3(signature = (symtab, *, profile=None))]
fn validate_expressions(
&self,
Expand All @@ -117,7 +124,8 @@ impl PyFormatString {
let st = extract_symtab(symtab)?;
let lib = profile_for_call(profile);
self.inner
.validate_expressions(&st, &lib)
.validate_expressions(&st, &lib, None)

@github-actions github-actions Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The escaping bug this PR is regression-testing for ExprValue is still live a few lines below, in this same file: PyFormatString::__repr__ (line 136-138) does

format!("FormatString(\"{}\")", self.inner.raw())

which interpolates the raw source with no escaping at all — not even for the " delimiter or the backslash. So any format string whose raw text contains a double quote, a backslash, or a control character produces a repr that either does not parse or parses as a different value:

  • FormatString(hello "world") → FormatString("hello "world"") — unparseable.
  • FormatString(C:\a) → FormatString("C:\a") — parses as BEL, exactly the a failure mode called out in the new test_repr_list_path_with_format comment. Windows-style raw paths in templates hit this routinely.
  • A raw containing a newline terminates the string literal mid-repr.

This matters more than usual here because raw() is attacker-influenced template text, and repr() is what ends up in log lines and exception messages.

Given openjd-rs#374 added the escaping writer for ExprValue, would it make sense to route FormatString.__repr__ through the same escaper (and pick the delimiter per CPython, as the new test_repr_list_selects_delimiter_per_element asserts)? Otherwise the two repr implementations in the bindings now disagree on whether their output round-trips.

@leongdl leongdl Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, and out of scope for this PR. Reproduced through the binding — all three of your cases:

raw repr(FormatString(raw)) parses as Python?
hello "world" FormatString("hello "world"") SyntaxError: invalid syntax
C:\a FormatString("C:\a") parses — as BEL
a\nb literal newline mid-repr SyntaxError: unterminated string literal

And ExprValue.__repr__ handles all three correctly on this branch, so the two implementations do now disagree on whether their output round-trips. Your reading is right.

Not fixing it here, for two reasons.

First, __repr__ is not in this diff. This PR's hunks in this file are the validate_expressions doc comment and its call site; the format! you quote is pre-existing on mainline and fails identically there. A dependency bump should not carry an unrelated behaviour change to a second method.

Second, the fix you suggest is not currently implementable. The escaping writer #374 added is crate-private:

openjd-expr-0.7.0/src/lib.rs:27:      pub(crate) mod py_escape;
openjd-expr-0.7.0/src/py_escape.rs:33: pub(crate) fn write_py_string_literal(...)

So the bindings crate cannot route through it. Closing this properly needs openjd-rs to export the writer (or expose a repr_python-style helper for a bare string) first, then a follow-up here. Routing through ExprValue::String(raw).repr_python() instead would work but embeds the ExprValue(...) wrapper in a FormatString repr, which is worse.

Recorded in the PR description as a known gap with the upstream dependency named, so it does not live only in this thread.

@leongdl leongdl Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correcting my earlier reply on this thread. I said closing this needs openjd-rs to export the escaping writer first. That was wrong about the remedy, though right that py_escape is pub(crate). PyO3 exposes the interpreter's own repr() directly, so no upstream change is needed — and it is a better escaper than the one I was waiting on, since it is correct by construction and is the same oracle these tests assert against. SymbolTable.__repr__ in this crate already delegates to Python's repr, so the pattern was in front of me.

The sessions/ slice is now up as #361, with a py_repr helper the remaining sites can reuse.

Investigating it also showed the problem is wider than this one call site, in a way worth recording here. Two classes:

  • No escaping at all, the shape you flagged. 4 sites; 2 are genuinely broken (FormatString.__repr__, ParsedExpression.__repr__). ExprType and RangeExpr are safe because their constructors reject anything hostile.
  • Rust Debug {:?}, ~23 sites. Debug agrees with Python on the quote, backslash and C0 controls, but renders anything else non-printable as \u{a0}, which CPython rejects outright. 14 of these are reachable from Python. This class is invisible to a reader checking only for a missing escape, which is why the flagged site was the tip of it.

The worst one was not in either of our lists: ActionResult(stdout={:?}) carries captured process output, so a non-ASCII byte in a job's stdout breaks the repr as a matter of course rather than under attack.

One correction to your framing, for the record: this is not reachable via raw() being attacker-influenced in the way FormatString is. Template validation rejects only Cc control characters, so U+00A0 and U+3000 — both Zs — pass into step and job names untouched, which is how they reach these reprs.

Still leaving this thread open: #361 covers sessions/ only, so FormatString.__repr__ itself is unfixed. It is recorded in #359's description as a known gap, and #361's description names the remaining sites.

.map(|_| ())
.map_err(format_string_validation_err_to_py)
}

Expand Down
6 changes: 5 additions & 1 deletion specs/python-expr-interface.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,7 +816,11 @@ except FormatStringValidationError as e:
The error message embeds the ``[start, end]`` byte offsets of the
failing ``{{...}}`` pair so callers can produce structured
diagnostics or syntax-highlight the failing segment. Mirrors the
Rust crate's ``FormatString::validate_expressions(symtab, lib)``.
Rust crate's
``FormatString::validate_expressions(symtab, lib, target_type)``,
which the binding calls with ``target_type=None`` to match ``resolve``
and ``resolve_string``. The crate returns a ``StaticResolution``; the
binding is pass/fail only and discards it.

**Equality and hashability.** `FormatString` implements `__eq__` and
`__hash__` on the raw source string. Two format strings compare equal
Expand Down
11 changes: 9 additions & 2 deletions src/openjd/_openjd_rs.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -915,8 +915,15 @@ class FormatString:
checking through the expression tree.

Mirrors the Rust crate's
`FormatString::validate_expressions(symtab, lib)`. Returns
`None` on success.
`FormatString::validate_expressions(symtab, lib, target_type)`.
Returns `None` on success.

`target_type` is passed as `None` because `resolve` and
`resolve_string` above resolve without one; validation has to
observe the same values resolution will produce. The crate
returns a `StaticResolution` (resolved-length bound and, when
fully concrete, the resolved value); this binding is pass/fail
only and discards it.
"""

def __str__(self) -> builtins.str: ...
Expand Down
134 changes: 120 additions & 14 deletions test/openjd/expr/test_expression_value.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,27 +337,133 @@ def test_repr_empty_list_list_path(self) -> None:

@pytest.mark.parametrize("pf", [PathFormat.POSIX, PathFormat.WINDOWS])
def test_repr_list_path_with_format(self, pf: PathFormat) -> None:
import re

v = ExprValue(["/a", "/b"], type="list[path]", path_format=pf)
r = repr(v)
assert re.match(
r"ExprValue\(\['(/|\\)a', '(/|\\)b'\], type='list\[path\]', "
rf"path_format=PathFormat\.{pf.name}\)",
r,
# Under WINDOWS the leading "/" normalises to "\", which the
# literal must escape -- so the expectation comes from CPython's
# repr of the normalised items rather than a regex that would
# accept either spelling. openjd-rs#374; before it, the WINDOWS
# case emitted '\a', which Python parses as BEL.
assert repr(v) == (
f"ExprValue({v.item()!r}, type='list[path]', path_format=PathFormat.{pf.name})"
)
assert eval(repr(v)) == v

@pytest.mark.parametrize("pf", [PathFormat.POSIX, PathFormat.WINDOWS])
def test_repr_list_list_path_with_format(self, pf: PathFormat) -> None:
import re

v = ExprValue([["/a"], ["/b"]], type="list[list[path]]", path_format=pf)
r = repr(v)
assert re.match(
r"ExprValue\(\[\['(/|\\)a'\], \['(/|\\)b'\]\], type='list\[list\[path\]\]', "
rf"path_format=PathFormat\.{pf.name}\)",
r,
assert repr(v) == (
f"ExprValue({v.item()!r}, type='list[list[path]]', path_format=PathFormat.{pf.name})"
)
assert eval(repr(v)) == v


class TestExprValueReprEscaping:
"""``__repr__`` escapes its embedded Python literal.

``repr_python`` previously escaped nothing, not even the quote or the
backslash, so a value carrying a quote, a backslash or a control
character produced a literal CPython cannot parse (a raw newline
terminates the string; a NUL cannot appear in source at all) or, worse,
one that parses as a different value (``'a\\b'`` is a backspace).
Fixed upstream in openjd-rs#374, reachable here via ``__repr__``.

Expression Language 2.2.6 defines the escaping as following Python's
own ``repr``, so ``repr(str)`` is the oracle rather than a hand-written
expectation.
"""

# Quotes and backslashes (delimiter selection and doubling), the C0
# controls, DEL, and the non-ASCII characters CPython escapes by
# category -- U+0085, U+00A0, U+00AD, U+2028, U+2029, U+3000, U+200B,
# a private-use character and an astral non-printable -- alongside
# printable non-ASCII that must survive verbatim.
#
# "a b" is the negative half of the Zs discrimination: U+0020,
# U+00A0 and U+3000 are all Zs, and CPython escapes only the latter
# two. It is the case that isolates that distinction -- the quote
# cases above also carry a U+0020, but they vary the delimiter at
# the same time.
ESCAPING_CASES = [
"it's",
'say "hi"',
'it\'s a "x"',
"'",
"a\\b",
"a\\",
"\\'",
"hello\nworld",
"a\rb",
"a\r\nb",
"a\tb",
"a\x00b",
"a\x1bb",
"a\x7fb",
"a\x0b\x0cb",
"café",
"a\U0001f600b",
"a\x85b",
"a\xa0b",
"a\xadb",
"a\u2028b",
"a\u2029b",
"a\u3000b",
"a\u200bb",
"a b",
Comment thread
leongdl marked this conversation as resolved.
"a\ue000b",
"a\U00100000b",
"a\U000e0100b",
]

@pytest.mark.parametrize("s", ESCAPING_CASES)
def test_repr_string_matches_cpython(self, s: str) -> None:
assert repr(ExprValue(s)) == f"ExprValue({s!r})"

@pytest.mark.parametrize("s", ESCAPING_CASES)
def test_repr_string_round_trips(self, s: str) -> None:
# The point of escaping: the literal parses back to the value.
assert eval(repr(ExprValue(s))) == ExprValue(s)

@pytest.mark.parametrize("s", ESCAPING_CASES)
def test_repr_list_element_matches_cpython(self, s: str) -> None:
# ``repr_python_list`` renders elements through the same writer; a
# list-only regression would go unnoticed by the scalar cases.
assert repr(ExprValue([s])) == f"ExprValue({[s]!r}, type='list[string]')"

def test_repr_list_selects_delimiter_per_element(self) -> None:
# CPython picks a delimiter per element, so these two differ.
assert repr(ExprValue(["it's", "a\nb"])) == (
"ExprValue([\"it's\", 'a\\nb'], type='list[string]')"
)

def test_repr_nested_list_element_escaped(self) -> None:
assert repr(ExprValue([["a\nb"]])) == r"ExprValue([['a\nb']], type='list[list[string]]')"

def test_repr_path_escaped(self) -> None:
v = ExprValue("/tmp/a\nb.txt", type="path", path_format=PathFormat.POSIX)
assert repr(v) == r"ExprValue('/tmp/a\nb.txt', type='path', path_format=PathFormat.POSIX)"

def test_repr_list_path_escaped(self) -> None:
# ``String`` and ``Path`` share one match arm; splitting them would
# leave ``list[path]`` unescaped.
v = ExprValue(["/a\nb"], type="list[path]", path_format=PathFormat.POSIX)
assert repr(v) == r"ExprValue(['/a\nb'], type='list[path]', path_format=PathFormat.POSIX)"

@pytest.mark.parametrize(
"value,expected",
[
(42, "ExprValue(42)"),
(True, "ExprValue(True)"),
(None, "ExprValue(None)"),
(Decimal("3.500"), "ExprValue('3.500', type='float')"),
],
)
def test_repr_non_string_text_unaltered(self, value: object, expected: str) -> None:
# Negative control: numeric and keyword text needs no escaping, so
# routing it through the shared writer must not change it.
assert repr(ExprValue(value)) == expected

def test_repr_range_expr_unaltered(self) -> None:
assert repr(ExprValue("1-5", type="range_expr")) == "ExprValue('1-5', type='range_expr')"


class TestMemorySize:
Expand Down
Loading