feat: parse profiles in the core, and have the Python SDK stop parsing - #185
feat: parse profiles in the core, and have the Python SDK stop parsing#185dzerik wants to merge 1 commit into
Conversation
Closes the second half of multikernel#174. The SDK carried its own TOML parser and its own grammars, and they had already diverged from the core in two places you found: `parse_memory_size` accepted fractions and a `T` suffix that `ByteSize::parse` rejects, and `time_start` went through `int()`, so an RFC 3339 stamp worked in the CLI and raised through the SDK. A third grammar sat unused in the dataclass, `time_start_timestamp`, with naive-means-UTC semantics. `sandlock_profile_parse` takes TOML text and returns canonical JSON with every micro-grammar already resolved: mounts as `{virt, host, ro}` objects, sizes as integer bytes, `time_start` as epoch seconds. The SDK's remaining job is a field-for-field copy into its dataclass, so introspection, `dataclasses.replace` and preset composition keep working. Unknown keys are rejected on both sides, so future drift fails at load time instead of mis-parsing silently. `sandbox_to_json` was not reusable as-is: it re-emits mounts as `V:H:ro` spec strings, which would have put string parsing straight back into the SDK. The canonical form emits structured mounts instead. Its `ro` is the effective setting for the virtual path, not the flag written on one spec: the core keys read-only mounts by virtual path (`Sandbox::fs_mount_ro` is a list of virtual paths), so two specs sharing a virtual path share one verdict, and reporting the written flag would describe a policy no layer applies. Public Python API changes, deliberately and without shims: max_memory: str | int | None -> int | None max_disk: str | None -> int | None time_start: float | str | None -> float | None fs_mount: Mapping[str, str] -> Sequence[Mount] `Mount(virt, host, ro)` is new and mirrors the canonical field names. `fs_mount` becoming a sequence is what lets a read-only mount be expressed at all from Python; it reaches the C ABI through the `fs_mount_ro` setter added in multikernel#180. `tomli` is gone from the dependencies. Two more changes to the same surface, both consequences of the SDK no longer holding an opinion of its own: - `on_error` loaded from a profile now defaults to COMMIT where it defaulted to ABORT. The canonical form always resolves both branch actions, and the SDK copies what it is handed, so a profile that says nothing about the error path gets the core's answer rather than the dataclass's second opinion. Deliberate, since the CLI, a profile and the Go SDK have always meant COMMIT for that policy, but it changes what happens to a COW branch for a profile already in use, and it changes it silently. Only the profile path moves; the dataclass default is untouched here. - `parse_memory_size`, `Sandbox.memory_bytes()` and `Sandbox.time_start_timestamp()` are removed. The first two were the SDK's byte-size grammar and its accessor, the third the unused third grammar named above. Nothing replaces them: the resolved value is the field. Two core changes came out of this rather than the SDK: - Rebuilding a builder from a parsed profile ran `extend_net_allow_for_http` a second time over an allowlist that already held its derived entries, so the helper is now idempotent, with a test. - `ByteSize::parse` multiplies with `checked_mul`. The unchecked multiply wrapped in release builds, so `memory = "17179869184G"` parsed cleanly and installed a ceiling of zero bytes, with nothing reported anywhere and the guest SIGKILLed on its first allocation. It is an out-of-range error now. Verified against the CLI message for message on every grammar: the same profile loads identically, or fails identically, through both paths, with one gap left open and pinned rather than papered over. `sandlock_sandbox_builder_time_start` takes a `uint64` of seconds, so a stamp the core keeps in full loads from a profile and then cannot be handed to a builder: `"2026-01-01T00:00:00.5Z"` and any instant before 1970 are what that costs. The SDK refuses them by name instead of wrapping a negative value through an unsigned setter, and `test_time_start_the_c_abi_cannot_carry_is_refused_loudly` holds it there. Closing the gap means changing that setter's signature, which is a later commit in this series.
|
Not a review, just a note: I am planning to cut the release in a few days, since this PR is fairly large, I'd suggest to defer it to the next release. WDYT? |
|
Agreed, and thanks for saying so before the release rather than after. Deferring all three is the right call: the volume is genuinely large, and a release is the worst moment to take a breaking change, which is most of what this series is. One thing worth flagging, since it makes the three look even heavier than they are. They are a stack, and because they come from a fork they can only target
Tests are 58 to 71 percent of each. That does not make them small, and I am not arguing the point: even the new-only figures are several times the size of anything recently merged here. Just noting it so the review order is clear whenever you get to them: #185, then #186, then #187, each on top of the last. I have opened #190 with one commit lifted out of this PR: No rush on the rest from my side. |
Closes the second half of #174.
The problem
The Python SDK carried its own TOML parser and its own copies of the string
micro-grammars, and they had already drifted from the core in two places:
parse_memory_sizeaccepted fractions and aTsuffix thatByteSize::parserejects, somemory = "1.5T"was valid through the SDKand invalid through the CLI.
time_startwent throughint(), so an RFC 3339 stamp loaded in the CLIand raised through the SDK.
A third grammar sat unused in the dataclass,
time_start_timestamp, withnaive-means-UTC semantics that nothing else in the project shares.
The drift was not an accident of implementation. Two independent parsers
that must agree byte for byte will diverge, because nothing makes them
agree except attention.
What this does
sandlock_profile_parsetakes TOML text and returns canonical JSON withevery micro-grammar already resolved: mounts as
{virt, host, ro}objects,sizes as integer bytes,
time_startas epoch seconds, bind ports asexpanded integer lists. The SDK's remaining job is a field-for-field copy
into its dataclass, so introspection,
dataclasses.replaceand presetcomposition keep working. Unknown keys are rejected on both sides, so a
future schema change fails at load time instead of being mis-parsed
silently.
sandbox_to_jsonwas not reusable as-is: it re-emits mounts asV:H:rospec strings, which would have put string parsing straight back into the
SDK. The canonical form emits structured mounts instead.
The canonical
rois the effective setting for the virtual path, not theflag written on one spec. The core keys read-only mounts by virtual path
(
Sandbox::fs_mount_rois a list of virtual paths, andchroot/dispatch.rs::is_mount_romatches on it), so two specs sharing avirtual path share one verdict. Reporting the written flag would describe a
policy no layer applies. One residual gap is documented rather than hidden:
a mount nested under a read-only one is write-denied at run time while its
canonical
rostays false, and closing that needs(virt, host)keying.Breaking changes
Public Python API, deliberately and without shims:
Mount(virt, host, ro)is new and mirrors the canonical field names.fs_mountbecoming a sequence is what lets a read-only mount be expressedat all from Python; it reaches the C ABI through the
fs_mount_rosetteradded in #180.
tomliis gone from the dependencies.parse_memory_size,Sandbox.memory_bytes()andSandbox.time_start_timestamp()are removed. The first two were the SDK'sbyte-size grammar and its accessor, the third the unused grammar named
above. Nothing replaces them: the resolved value is the field.
on_errorloaded from a profile now defaults to COMMIT where it defaultedto ABORT. The canonical form always resolves both branch actions, and the
SDK copies what it is handed, so a profile silent about the error path gets
the core's answer rather than the dataclass's second opinion. This is
deliberate, since the CLI, a profile and the Go SDK have always meant COMMIT
for that policy, but it changes what happens to a COW branch for a profile
already in use, and it changes it silently. Only the profile path moves; the
dataclass default is untouched here.
Two core fixes that came out of this
Rebuilding a builder from a parsed profile ran
extend_net_allow_for_httpa second time over an allowlist that already held its derived entries, so
the helper is idempotent now, with a test.
ByteSize::parsemultiplies withchecked_mul. The unchecked multiplywrapped in release builds, so
memory = "17179869184G"parsed cleanly andinstalled a ceiling of zero bytes, with nothing reported anywhere and the
guest SIGKILLed on its first allocation. It is an out-of-range error now.
One gap left open and pinned
Verified against the CLI message for message on every grammar: the same
profile loads identically, or fails identically, through both paths, with
one exception.
sandlock_sandbox_builder_time_starttakes auint64of seconds, so astamp the core keeps in full loads from a profile and then cannot be handed
to a builder.
"2026-01-01T00:00:00.5Z"and any instant before 1970 arewhat that costs. The SDK refuses them by name instead of wrapping a negative
value through an unsigned setter, and
test_time_start_the_c_abi_cannot_carry_is_refused_loudlyholds it there.Closing the gap means changing that setter's signature, which is the third
PR in this series.
Position in the series
This is the first of three. The next two are stacked on it:
Each is reviewable on its own and each is green on its own.
Testing
cargo test -p sandlock-core --lib: 730 pass at this commit.cargo test -p sandlock-core --test integration -- --test-threads=1: green.cargo test --workspace --exclude sandlock-core: green.(
profile_canonical_adversarial.rs, 447 lines) drives the canonical formagainst malformed and boundary input.
git diff --exit-codegate.