Repository navigation
Make the core/bridge Pier wire contract agree - #28
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Essentials Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
The core and the bridge were never run against each other. Three
consequences, fixed together because they are one problem:
Package digest. assay.pier_packaging hashed [path, sha256(content)] pairs
through canonical_json; the bridge hashed raw contents through plain
json.dumps, and its docstring called the independence deliberate. Since
DockerTrialHandle rejects any request whose package_digest differs from its
own computation, an independent algorithm rejects every real dispatch. The
bridge now reimplements assay's algorithm locally (the no-import-edge rule
stands), and assay's sorts explicitly instead of inheriting the caller's
mapping order -- canonical_json preserves array order, and the bridge
receives a materialized package with no memory of how it was built.
Trial name. trial_name_for doubles hyphens to stay injective, but it was
only ever compared against itself inside PierExchange; the name Pier
actually received came from sanitized_trial_name, the plain
replace(":", "-") the core docstring explains is broken. ("a-b", "c") and
("a", "b-c") collided on one trial directory. The bridge now mirrors the
doubling rule.
Artifact channel. TrialResult had no artifacts field at all, so even with
matching digests no evidence bytes could cross -- the submission mount is
destroyed by teardown. Both handles now read it back and carry the bytes
out, bounded per-artifact and in aggregate, skipping symlinks and
non-regular files rather than following what a model may have planted
there.
Neither reimplementation can drift silently: tests/fixtures/
pier_wire_contract.json pins the expected digests and trial names, and both
test suites assert against it.
Still open, and now stated where it belongs rather than only in a review:
nothing on the bridge side produces an artifact manifest, so a real
dispatch ends in MissingManifest. A manifest built on the consumer side
from the bytes it just received would only be attesting to itself, which is
what verify_artifact_bytes exists to prevent.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XogEG4ML9Hi8SrLgqC1Fym
There was a problem hiding this comment.
🟡 Changes recommended
Fix aggregate artifact limits before reading files and prevent post-validation mutation of artifact data.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Aligns core and bridge Pier wire contracts for package digests, trial names, and bounded artifact transport.
Changes:
- Standardizes digesting and injective trial naming.
- Adds artifact collection, validation, and transport.
- Adds shared contract fixtures and tests.
- Documents the remaining manifest gap.
The artifact collector can exceed the aggregate memory limit before validation, and nested artifact data remains mutable after validation.
File summaries
| File | Summary |
|---|---|
tests/test_pier_wire_contract.py |
Core wire-contract tests |
tests/test_pier_packaging.py |
Digest test updates |
tests/fixtures/pier_wire_contract.json |
Shared contract vectors |
src/assay/pier_packaging.py |
Deterministic package digesting |
src/assay/adapters/pier.py |
Artifact contract documentation |
integrations/pier/tests/test_wire_contract.py |
Bridge wire-contract tests |
integrations/pier/tests/test_artifact_channel.py |
Artifact safety and limit tests |
integrations/pier/src/assay_pier_bridge/protocol.py |
Artifact-bearing results and bounds |
integrations/pier/src/assay_pier_bridge/pier_adapter.py |
Injective trial naming |
integrations/pier/src/assay_pier_bridge/paths.py |
Safe artifact collection |
integrations/pier/src/assay_pier_bridge/host_driver.py |
Host artifact propagation |
integrations/pier/src/assay_pier_bridge/container.py |
Container artifact propagation and digest alignment |
Review details
Suppressed comments (1)
integrations/pier/src/assay_pier_bridge/protocol.py:150
frozen=Truedoes not freeze the nesteddict: after validation, callers can still assignresult.artifacts["../escaped"] = huge_bytes(or replace an existing value), bypassing bothsafe_relative_pathand the per/aggregate limits before the result is consumed. Since this field is the untrusted evidence boundary, make the stored collection immutable (or revalidate/copy it at every consumption boundary) rather than relying on the model's shallow freeze.
artifacts: dict[str, bytes] = Field(default_factory=dict)
@model_validator(mode="after")
def bounded_artifacts(self) -> TrialResult:
total = 0
for path, data in self.artifacts.items():
safe_relative_path(path)
if len(data) > MAX_ARTIFACT_BYTES:
raise ValueError(f"artifact exceeds the per-artifact byte limit: {path}")
total += len(data)
if total > MAX_AGGREGATE_ARTIFACT_BYTES:
raise ValueError("artifacts exceed the aggregate byte limit")
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| submission_source = submission_file.read_text(encoding="utf-8") | ||
| # Read the submission mount before teardown removes it: these bytes | ||
| # exist nowhere else, and the caller cannot go back for them. | ||
| artifacts = collect_artifacts(self._submission, max_bytes=MAX_ARTIFACT_BYTES) |
| artifacts: dict[str, bytes] = {} | ||
| for path in sorted(root.rglob("*")): | ||
| if path.is_symlink() or not path.is_file(): | ||
| continue | ||
| relative = path.relative_to(root).as_posix() | ||
| if path.lstat().st_size > max_bytes: | ||
| raise ValueError(f"trial artifact exceeds the byte limit: {relative}") | ||
| safe_relative_path(relative) | ||
| artifacts[relative] = path.read_bytes() |
EffectiveEnforcement gained a required storage_limit_mb in #27 (the size-capped /scratch tmpfs); this PR's new test helper predated it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XogEG4ML9Hi8SrLgqC1Fym
492db5e to
a82820e
Compare
First of the fix PRs coming out of the
epic/pier-supportreview(
comms/review-epic-pier-support.md). It closes review blockers #1 and#4, plus the structural gap behind #8 that made #1 unfixable on its
own.
Why these three together
They are one problem: the core and the bridge have never been run against
each other.
Package digest (blocker #1).
assay.pier_packaginghashed[path, sha256(content)]pairs throughcanonical_json; the bridge hashedraw contents through plain
json.dumps, and its docstring called theindependence deliberate and said the two sides were "expected to agree out
of band". They structurally cannot.
DockerTrialHandle.__init__rejects anyrequest whose
package_digestdiffers from its own computation, so everyreal dispatch failed with "materialized package does not match the
request's authorized package_digest".
Fixed by having the bridge reimplement assay's algorithm locally — the
no-import-edge rule stands — and by making assay's own sort explicitly
rather than inheriting the caller's mapping order.
canonical_jsonpreserves array order, and the bridge receives a materialized package with
no memory of how it was built. Both call sites already passed sorted
mappings, so no existing digest value changes.
Trial name (blocker #4).
trial_name_fordoubles hyphens to stayinjective, but it was only ever compared against itself inside
PierExchange. The name Pier actually received came fromsanitized_trial_name—cell_id.replace(":", "-"), the exactnon-injective mapping the core docstring explains is broken.
("a-b", "c")and
("a", "b-c")shared one trial directory on disk. The bridge nowmirrors the doubling rule and rejects a cell id that is not three
components.
Artifact channel (the part of #8 that blocks #1). The bridge's
TrialResulthad noartifactsfield at all — only a baresubmission_refdigest — so even with matching digests no evidence bytescould cross the boundary, and
_settlewould still have returnedMissingManifeston every dispatch. Fixing the digest alone boughtnothing. Both handles now read their submission mount back before
teardowndestroys it and carry the bytes out on the result.That read is the least trustworthy thing this project does, so
collect_artifactsreads only regular files, skips symlinks rather thanfollowing them out of the mount, and checks size via
lstatbefore pullinganything into memory.
TrialResultbounds the result per-artifact and inaggregate against the same ceilings
assay.pier_protocoluses.Keeping the two copies honest
tests/fixtures/pier_wire_contract.jsonpins the expected package digestsand trial names. Both test suites assert against it, so neither
reimplementation can drift without failing on at least one side. The
fixture includes the
a-b:c:w0/a:b-c:w0pair that collided, and thebridge test fails loudly if the fixture ever moves rather than silently
skipping every case.
Verification
The mismatch the review reproduced, before and after:
End to end, a package built by
assay.pier_packaging.build_packageis nowaccepted by
DockerTrialHandleand materialized correctly (no daemonneeded — the digest check runs before any container starts):
ruff check,mypy src,schema_export --checkpytest -qpytest -q,mypy srcRebased onto
epic/pier-supportat1f83fa8(PR #27), which landed afterthis branch was cut. The
pier-qualificationjob it added runs green here,including
ruff check integrations/pier, thequalify_local.pymypy pass,tests/test_pier_acceptance.py(23 passed, 1 skipped), and the bridge's ownsynced suite. Review item #7 ("CI never runs the bridge project") is closed
by #27, not by this PR.
Deliberately not in this PR
A real dispatch still cannot succeed. Nothing on the bridge side
produces an artifact manifest, and
_settlewill not settle a cell as asuccess without one binding the bytes to the authorized exchange. A
manifest built on the consumer side, from the bytes it just received, would
only be the consumer attesting to itself — which is exactly what
verify_artifact_bytesexists to prevent. The gap is now stated inTrialResultandBridgeTrialResultrather than only in a reviewdocument. Closing it is the next PR, alongside blocker #2 (a
WorkerSuccesscan carry null evidence refs), the unbindablemanifestrequired kind, and the
_mount_pathscollision where a repository filenamed
instruction.mdsilently replaces the sealed instruction.Failure-path artifacts are also out of scope here:
BridgeRuntime._failurehas no artifact accessor on the
TrialHandleprotocol, and that protocolchange belongs with the cancellation work (review #5).
🤖 Generated with Claude Code
https://claude.ai/code/session_01XogEG4ML9Hi8SrLgqC1Fym