Repository navigation
Bind artifact evidence to what was actually published - #29
Conversation
Three defects around the artifact boundary, all of which let a cell settle as a success while the evidence behind it was absent, unbindable, or substituted. A WorkerSuccess could carry null evidence refs. `_publish_available_artifacts` enforces the artifact byte ceilings before anything reaches the store, so a bridge response can verify perfectly against the bytes that crossed the boundary while the bytes backing a required kind never got published -- `verify_artifact_bytes` is handed the raw mapping, where a skipped entry is present either way. The result was a success whose `result_ref`/ `candidate_ref` were null, which downstream cannot distinguish from a trial that produced nothing. `_settle` now fails the cell with `UnpublishedEvidence` instead. Publication order is no longer the bridge's to choose either: the manifest and the entries it declares are published first, in sorted path order, so one large undeclared extra under an early key cannot crowd out the evidence a success is gated on. The `manifest` required kind was unsatisfiable. An entry for `manifest.json` would have to declare the checksum of the bytes carrying that checksum. Every writer that met the requirement met it with a decoy file -- three fixtures and `qualify_local.py`, independently -- so the "manifest evidence" a success was gated on was reliably not the manifest, and `manifest_ref` pointed at the decoy. The kind is gone, `ArtifactEntry` reserves `MANIFEST_PATH` so the fixed point is a typed rejection rather than something each author rediscovers, and `manifest_ref` resolves to the manifest itself. A repository file could replace the sealed instruction. The bridge mounts a package with the `workspace/` prefix stripped, so a repository containing a file named `instruction.md` lands on the sealed instruction's mounted path -- and since the package arrives sorted, the repository file wins. Content from the arm under test could rewrite the task that arm is measured against. `gate_package` and `_mount_paths` now both refuse a package whose entries collide once mounted; the bridge repeats the check because it must not depend on any particular producer having run it. Two smaller corrections in the same seam: the rendered instruction told the model its repository was under `workspace/`, a directory that does not exist in the container (the prefix is a package-internal namespace, stripped at mount time); and `manifest.json` is now bounded by the per-artifact ceiling but excluded from the aggregate one, since charging the binding document to the budget it bounds would evict a declared entry from a maximal honest manifest. Reviewed-for: blocking #2, plus findings B and C from the meta-review of comms/review-epic-pier-support.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XogEG4ML9Hi8SrLgqC1Fym
|
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 |
There was a problem hiding this comment.
🟡 Changes recommended
The bridge transport still counts manifest bytes against the aggregate artifact limit, and the incomplete-evidence error text still names a removed required kind.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR binds successful Pier evidence to artifacts actually published, removes the impossible manifest self-check requirement, and rejects package mount collisions.
Changes:
- Adds publication-priority and missing-reference checks.
- Treats
manifest.jsonas a binding document, not an artifact kind. - Prevents instruction/contract shadowing and corrects workspace documentation.
File summaries
| File | Description |
|---|---|
src/assay/adapters/pier.py |
Prioritized artifact publication and required-reference validation. |
src/assay/pier_protocol.py |
Reserved manifest path and revised required kinds. |
src/assay/pier_packaging.py |
Fixed mount instructions and collision checks. |
integrations/pier/src/assay_pier_bridge/container.py |
Rejects mounted-path collisions. |
integrations/pier/scripts/qualify_local.py |
Removed manifest decoy artifact. |
docs/pier-integration.md |
Documented the updated evidence contract. |
tests/test_pier_evidence_publication.py |
Tests publication ordering and missing references. |
tests/test_pier_packaging.py |
Tests package collision and instruction behavior. |
integrations/pier/tests/test_mount_projection.py |
Tests mount projection collisions. |
tests/test_pier_artifacts.py |
Tests manifest reservation and required kinds. |
tests/test_pier_run.py |
Updates successful-run fixtures. |
tests/test_pier_final_rehearsal.py |
Updates rehearsal fixtures. |
tests/test_pier_accounting.py |
Updates accounting fixtures and documentation. |
tests/test_pier_adapter.py |
Updates required-kind assertions. |
Review details
- Files reviewed: 14/14 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.
| ``manifest.json`` is bounded by the per-artifact ceiling but deliberately | ||
| excluded from the aggregate one. ``MAX_AGGREGATE_ARTIFACT_BYTES`` bounds | ||
| what a manifest may *declare*, and the manifest is never one of its own | ||
| entries; charging it to that same budget would make a manifest declaring | ||
| the maximum permitted aggregate impossible to publish in full, evicting | ||
| a declared entry to make room for the document declaring it. The total | ||
| a hostile bridge can put in the store is still bounded, just by | ||
| ``MAX_AGGREGATE_ARTIFACT_BYTES + MAX_ARTIFACT_BYTES``. |
There was a problem hiding this comment.
Confirmed and fixed in 5ddbd24. Reproduced at the boundary first: a manifest declaring exactly MAX_AGGREGATE_ARTIFACT_BYTES plus the 1,278-byte manifest.json was rejected by TrialResult, and the same artifacts minus the manifest were accepted — so the adapter-side exemption was unreachable through any real bridge, and test_a_maximal_honest_manifest_still_publishes_in_full passed only because it called _publish_available_artifacts directly and never built the wire model that would refuse its input.
bounded_artifacts now exempts MANIFEST_PATH (a local constant, no import edge) and still bounds it per-artifact. The shared boundary is covered the way package_digest already is: tests/fixtures/pier_wire_contract.json gained an artifact_budget block with the two ceilings, the reserved path and five accept/reject cases as byte counts, asserted from both tests/test_pier_wire_contract.py and integrations/pier/tests/test_wire_contract.py, plus three focused tests in test_artifact_channel.py. A ceiling changed on one side alone now fails on the other.
| # The four checked artifacts a success is bound to; "transcript" (ATIF) is an | ||
| # optional derived view and is deliberately excluded from this set. | ||
| REQUIRED_SUCCESS_KINDS: frozenset[str] = frozenset( | ||
| {"raw_trajectory", "candidate", "result", "configuration", "manifest"} | ||
| {"raw_trajectory", "candidate", "result", "configuration"} |
There was a problem hiding this comment.
Confirmed and fixed in 5ddbd24. The message now names only the four kinds.
The underlying reason it went stale is that the IncompleteEvidence path had no test at all — grep found no reference to it anywhere outside the one raise. It has one now, and rather than asserting a literal string it parses the message back into a set and compares that to REQUIRED_SUCCESS_KINDS, so the next kind added or removed fails here.
One deliberate exclusion, pinned by its own test: an undeclared result never reaches this check. With no result bytes there is no exit_status to read, so bridge_reports_failure rejects the cell earlier.
…oundary
Review round on this PR, both findings confirmed against the branch.
The manifest exemption stopped at the adapter. `_publish_available_artifacts`
excludes `manifest.json` from `MAX_AGGREGATE_ARTIFACT_BYTES` so that a
manifest declaring the maximum permitted aggregate can still be published in
full, but `assay_pier_bridge.protocol.TrialResult.bounded_artifacts` was still
summing every returned blob, the manifest included. An honest trial at the
ceiling was therefore rejected at the wire boundary, on the bytes describing
what it had already carried successfully:
core ArtifactManifest accepted a maximal manifest: 32000000
manifest.json bytes: 1278
total crossing the wire: 32001278
BRIDGE TrialResult REJECTED IT: ValidationError
Value error, artifacts exceed the aggregate byte limit
same artifacts minus manifest.json: ACCEPTED
The adapter-side exemption was consequently unreachable through any real
bridge, and the test covering it passed only because it called
`_publish_available_artifacts` directly and never constructed the wire model
that would have refused its input. The bridge now mirrors the exemption, with
its own `MANIFEST_PATH` constant since there is no import edge to share one.
Nothing structural connects those two copies, so the rule is pinned in the
shared cross-project fixture the way `package_digest` and the trial name
already are. `artifact_budget` carries the two ceilings, the reserved path,
and five accept/reject cases expressed as byte counts; each project
synthesizes blobs of those sizes and asserts its own enforcement point agrees
-- `assay` that every path reaches the store, the bridge that `TrialResult`
accepts the result. A ceiling changed on one side alone now fails on the
other, where the other copy lives.
Separately, `IncompleteEvidence` still told callers the manifest was missing
"raw trajectory/candidate/result/configuration/manifest evidence" after
`manifest` stopped being a kind. That path had no test at all, which is how it
went stale; it has one now, and the test parses the message back into a set
and compares it to `REQUIRED_SUCCESS_KINDS` rather than to a literal. An
undeclared `result` is excluded deliberately and pinned by its own test: with
no result bytes there is no `exit_status` to read, so `bridge_reports_failure`
rejects the cell before the evidence check is reached.
Also corrects a comment that undercounted the decoy workarounds as two; there
were four.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XogEG4ML9Hi8SrLgqC1Fym
Phase 2 of the fix plan from the meta-review of
comms/review-epic-pier-support.md.Branched off
0f93011(the merge of #28), fetched immediately before branching.Three defects around the artifact boundary. Each one let a cell settle as a
success while the evidence behind it was absent, unbindable, or substituted.
Blocking #2 — a
WorkerSuccesscould carry null evidence refs_publish_available_artifactsenforces the artifact byte ceilings beforeanything reaches the store.
verify_artifact_bytesis handed the rawmapping that crossed the boundary, where a skipped entry is present either
way — so a manifest could verify perfectly while the bytes backing a required
kind never got published, and
_settlebuilt its output fromrefs.get(path).The review's repro produced this:
Downstream cannot tell that from a trial that genuinely produced nothing.
_settlenow fails the cell withUnpublishedEvidenceif any required kind— or the manifest — resolves to no ref.
entries it declares are published first, both groups in sorted path order,
so one large undeclared extra under an early key cannot crowd out the
evidence a success is gated on.
verify_artifact_byteswould catch a skippeddeclared entry is corrected; it never could.
The review's own repro script now returns a fully-populated success.
Finding C — the
manifestrequired kind was unsatisfiableAn entry for
manifest.jsonwould have to declare the checksum of the bytescarrying that checksum. Every writer that satisfied the requirement satisfied
it with a decoy file, independently:
tests/test_pier_run.pymanifest_marker.jsontests/test_pier_accounting.pymanifest_marker.jsontests/test_pier_final_rehearsal.pymanifest_marker.jsonintegrations/pier/scripts/qualify_local.pymanifest-evidence.jsontest_pier_accounting.py's docstring named the problem exactly — "achecksum-of-itself fixed point with no stable solution" — and worked around it
rather than reporting it. So the "required manifest evidence" a success was
gated on was reliably not the manifest, and
manifest_refpointed at thedecoy.
The kind is removed from
ArtifactKind,REQUIRED_SUCCESS_KINDSandJSON_STRUCTURED_ARTIFACT_KINDS.ArtifactEntrynow reservesMANIFEST_PATHso the fixed point is a typed rejection at the boundary rather than something
each author rediscovers by trial and error.
manifest_refresolves to themanifest itself, whose integrity comes from parsing it and binding it to the
authorized exchange — not from a self-checksum.
Finding B — a repository file could replace the sealed instruction
The bridge mounts a package with the
workspace/prefix stripped, so therepository sits beside the instruction rather than under it. That projection
is not injective: a repository containing a file named
instruction.mdispackaged as
workspace/instruction.mdand lands on the sealed instruction'smounted path. The package arrives sorted, so the repository file wins.
gate_packageaccepted that package. Content from the arm under test couldrewrite the task that arm is being measured against — the one substitution the
sealed boundary exists to prevent. Both sides now refuse a package whose
entries collide once mounted; the bridge repeats the check rather than
depending on any particular producer having run it, per the no-import-edge
rule.
Two smaller corrections in the same seam
workspace/.WORKSPACE_PREFIXis a package-internal namespace that thebridge strips at mount time, so that directory does not exist in the
container — the model was sent somewhere empty. It now describes the mount
it actually gets.
manifest.jsonis bounded by the per-artifact ceiling but excluded from theaggregate one, on both sides of the boundary. A manifest may declare up to
MAX_AGGREGATE_ARTIFACT_BYTES; charging the binding document to the budgetit bounds would evict a declared entry from a maximal honest manifest, which
the new required-ref check would then report as a failure. Total store
exposure stays bounded, by
MAX_AGGREGATE_ARTIFACT_BYTES + MAX_ARTIFACT_BYTES.Not in this PR
A real dispatch still ends in
MissingManifest: no bridge writes amanifest.jsonyet. That manifest has to come from the side that observed thetrial — building one in the adapter from the bytes it was just handed would be
the consumer attesting to itself. The module docstrings record it as the
remaining gap rather than closing it from the wrong side.
Review round (
5ddbd24)Both review comments confirmed against the branch and fixed.
The manifest exemption stopped at the adapter.
TrialResult.bounded_artifacts(
integrations/pier/.../protocol.py) still summed every returned blob, themanifest included, so an honest trial at the ceiling was rejected at the wire
boundary before the adapter's exemption could matter:
That made the adapter-side exemption unreachable through any real bridge, and
test_a_maximal_honest_manifest_still_publishes_in_fullpassed only because itcalled
_publish_available_artifactsdirectly and never constructed the wiremodel that would have refused its input. The bridge now mirrors the exemption
with its own
MANIFEST_PATH, and the rule is pinned in the shared fixture theway
package_digestand the trial name already are:artifact_budgetcarriesthe ceilings, the reserved path and five accept/reject cases as byte counts,
and each project synthesizes blobs of those sizes and asserts its own
enforcement point agrees. A ceiling changed on one side alone now fails on the
other.
IncompleteEvidencestill named a removed kind. It reportedconfiguration/manifest evidenceaftermanifeststopped being one. The pathhad no test at all, which is how it went stale; the new one parses the message
back into a set and compares it to
REQUIRED_SUCCESS_KINDSrather than to aliteral. An undeclared
resultis excluded deliberately and pinned by its owntest -- with no result bytes there is no
exit_status, sobridge_reports_failurerejects the cell before the evidence check is reached.Checks
pytest -qruff check src tests integrations/pier,mypy srcpython -m assay.schema_export --checkpytest -q tests/test_pier_acceptance.pypytest -q,mypy srcMYPYPATH=… mypy integrations/pier/scripts/qualify_local.pyEQUAL? TrueNew tests:
tests/test_pier_evidence_publication.py(13),integrations/pier/tests/test_mount_projection.py(5), plus 4 gate/collisiontests in
test_pier_packaging.py, 3 manifest-kind tests intest_pier_artifacts.py, the sharedartifact_budgetcases asserted from bothtests/test_pier_wire_contract.pyandintegrations/pier/tests/test_wire_contract.py, and 3 budget tests inintegrations/pier/tests/test_artifact_channel.py.🤖 Generated with Claude Code
https://claude.ai/code/session_01XogEG4ML9Hi8SrLgqC1Fym