Skip to content

feat(stelae): give a publisher digest, verify and inspect - #1188

Open
scarmuega wants to merge 2 commits into
mainfrom
feat/stelae-publisher-commands
Open

feat(stelae): give a publisher digest, verify and inspect#1188
scarmuega wants to merge 2 commits into
mainfrom
feat/stelae-publisher-commands

Conversation

@scarmuega

@scarmuega scarmuega commented Aug 10, 2026

Copy link
Copy Markdown
Member

Phase 4, publisher productization: the three read/verify commands and incremental detection. Implements the Trellis plan plans/dolos-stelae-publisher-commands.md ("Stelae — a publisher can ask what it has, and a verifier can reproduce it") of the Brain/txpipe domain, contexts solution/stelae + solution/dolos.

What changed

crates/stelae — a Discarding SteleWriter: every layer framed, hashed and compressed exactly as a publish would — zstd runs in full, because the bug class this exists to catch only appears when the same bytes go through the same pipeline twice — and the bytes go to std::io::sink. What comes back is the identity.

crates/snapshot

  • history_for and same_network moved from registry.rs (feature-gated) into export.rs: the contiguity rule is one function shared between the registry publisher and a verifier that has no registry.
  • export::Following reads a predecessor's canonical inscription off disk and extends it by the publisher's own rule (digest --chain-from); a re-encoded copy is refused, since a stele is chained to by the digest of its own bytes.
  • export::Attested carries a published history verbatim; export::verify_reproduction rebuilds every layer through the discarding writer and compares descriptors kind by kind and scope by scope before comparing the inscription digest, with a cheap sequence refusal before the walk.
  • export::Standing reads where a node stands against a repository (empty / up to date / next / ahead-with-distance) out of the two numbers a publish already has.
  • registry::verify streams every blob end to end — blob digest and compressed size against the manifest, diffId, uncompressed size and record count against the inscription — and names the offending layer's kind and scope on failure (Error::LayerVerification). registry::inspect reads the manifest and the config blob and no layer.

src/bin/dolos/snapshot/ — three new modules:

  • digest — canonical inscription + sha256 from local stores and no registry; --chain-from FILE, --output FILE; stdout is the byte-exact document (no trailing newline), the report goes to stderr.
  • verify — transport checks by default; --reproduce opt-in, and the help says it costs what a publish costs. Output states plainly: digests only; signatures and chain provenance are phase 5.
  • inspect — sequence, position, profile, compression, history depth with first/last entries, one line per layer with the compressed size the manifest carries, totals; --json emits the canonical inscription verbatim. No signers column until there are signatures to list; the help says which.
  • publish — a repository already at the node's sequence reports "nothing to publish" and exits zero; --require-new makes that case an error; a node further ahead is still refused, now with the distance alongside both sequences. EpochRange and the plan report are hoisted to snapshot/mod.rs so publish, digest and verify share one epoch parser and one report.

Scope decisions honored: no sign / signature verification, no CI workflow, no gap policy (the refusal stands, its message now names the distance), no [snapshot] source, no tag pruning, no progress reporting, no --scratch-dir.

Done criteria

  1. digest and publish --output-dir produce byte-identical canonical inscriptions — a_discarding_export_reproduces_what_a_publish_stores (records-bearing harness store, compared on the canonical bytes) and a_discarding_writer_reproduces_what_a_directory_stores (toy profile: every descriptor field, blob digest, compressed size, seal).
  2. digest --chain-from reproduces a stele published with reuse ona_stele_published_with_reuse_is_reproduced_from_the_stores (3 layers inherited, digest reproduced from stores alone; chained onto nothing is a different digest).
  3. verify --repo passes against a fresh stele, and refuses with the offence named: a blob that is not the layer (streamed check, layer kind + scope named), a manifest diffId annotation disagreeing (refused at the pull, layer position + both identities named), a history that skips a sequence (refused at parse, gap named). Tampered artifacts are planted through the raw distribution API — the transport refuses to write any of them.
  4. verify --reproduce passes against the store the stele was published from and fails against a store standing at a different epoch — the failure costs a comparison of two sequences, not hours.
  5. inspect lists every layer with the manifest's compressed size (inherited layers included), sizes sum to the manifest total, and its canonical JSON round-trips through Following::read — the exact digest --chain-from path — chaining to the successor's published digest.
  6. publish --repo at the node's sequence: nothing to publish, exit zero; --require-new non-zero; three ahead refused with both sequences and "3 sequences ahead" in the message — a_repository_is_read_as_empty_current_next_or_ahead, a_gap_names_the_distance_alongside_both_sequences, a_publisher_can_ask_where_it_stands.
  7. ✅ Registry end-to-end tests #[ignore]d and run — output quoted below.
  8. ✅ Full gate — results below.

Verification

  • cargo test — full workspace: 15 suites, 0 failures.
  • cargo clippy --all-targets --all-features -- -D warnings — clean.
  • cargo +nightly fmt --all -- --check — clean.
  • cargo deny check advisoriesadvisories ok.
  • cargo tree -p stelae -e normal --all-features — matches nothing ^dolos(-|$).

Ignored registry suites, run against a spawned registry:2

cargo test -p dolos-snapshot --features oci --test snapshot_verify -- --ignored --nocapture:

diffId disagreement: stelae error: manifest disagrees with the inscription: layer 0 is annotated sha256:94455e3ed9f716bea425ef99b51fae47128769a1a0cd04244221e4e14631ab83 and the inscription describes sha256:0329ec9ba1660678676c18209478eed13ef607e234d46e5de275674563dd8ede there
corrupted blob: the blocks layer at {"endSlot":86399,"epoch":0,"startSlot":0} failed verification: stelae error: malformed layer header record: unexpected type string at position 0: expected array
skipped sequence: stelae error: history invariant violated: gap between sequence 0 and sequence 2
reproduced sha256:ef955801a19cee27e7a6069f7b653cda2b2402fc167746b2e8059f8a41b1881f == published sha256:ef955801a19cee27e7a6069f7b653cda2b2402fc167746b2e8059f8a41b1881f; a store at another epoch: the reproduction does not match the published stele — sequence: the published stele is sequence 2 and these stores stand at sequence 1; a reproduction runs over stores at the epoch the stele was published from
inspected 22 layers, 7830 compressed bytes; epoch-1's document chained to sha256:ef955801a19cee27e7a6069f7b653cda2b2402fc167746b2e8059f8a41b1881f
verified latest = sha256:ef955801a19cee27e7a6069f7b653cda2b2402fc167746b2e8059f8a41b1881f (22 layers, 7830 compressed bytes) and epoch-1 = sha256:f9523cbfed6dc7fad48f0d1452948dfe8988218b63bb8d1e2c60cfadc859321a

test result: ok. 6 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.68s

cargo test -p dolos-snapshot --features oci --test publish -- --ignored --nocapture:

publish 1: built 19, reused 0, uploaded 19 (7666 bytes)
publish 2: built 19, reused 3 (3390 bytes not moved), uploaded 19 (4440 bytes)
published (3 layers inherited) sha256:ef955801a19cee27e7a6069f7b653cda2b2402fc167746b2e8059f8a41b1881f == reproduced from stores sha256:ef955801a19cee27e7a6069f7b653cda2b2402fc167746b2e8059f8a41b1881f
inherited sha256:ef955801a19cee27e7a6069f7b653cda2b2402fc167746b2e8059f8a41b1881f == rebuilt sha256:ef955801a19cee27e7a6069f7b653cda2b2402fc167746b2e8059f8a41b1881f

test result: ok. 7 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 3.33s

cargo test -p dolos-snapshot --features oci --test restore_registry -- --ignored --nocapture:

test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.27s

cargo test -p stelae --features oci --test oci -- --ignored: 10 passed; 0 failed (13.94s).

Notes for review

  • digest/inspect --json stdout carries no trailing newline. The document is bytes: digest > stele.json has to hash to the reported identity, and --chain-from refuses anything that is not the canonical encoding itself. The plan report moved to stderr for the same reason; tests/snapshot_publish.rs was updated to read it there.
  • verify --reproduce has not been run against a production-published stele — it needs a node whose stores stand at the published epoch, which this environment does not have. Per the plan's risk section, a failing --reproduce against our own published stele is a finding for org/founder, not a comparison to adjust; the determinism job of dolos-stelae-publisher-pipeline is where that run lives.
  • ADR-004's stale "checked in CI" claims about the cargo tree boundary were trimmed to match reality (the CI guard plan is back in draft); the check ran by hand as part of the gate here.

Trellis trail

  • Plan: plans/dolos-stelae-publisher-commands.md (Brain/txpipe) — done criterion met; plan stays active, retirement is the owner's verdict.
  • Escalations: none.
  • Follow-up: plans/dolos-stelae-sign.md (draft) — Phase 5 signing, the one deferral in this plan with no plan to point at; this PR now promises "phase 5" in verify's output, inspect's help and the module docs.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added snapshot digest to reproduce snapshot identities without writing repository data.
    • Added snapshot inspect for metadata and canonical JSON output without downloading layers.
    • Added snapshot verify, including optional local reproduction checks.
    • Added predecessor chaining, attested histories, and epoch-range filtering.
    • Added repository status reporting and --require-new publishing protection.
  • Bug Fixes

    • Improved detection and reporting of broken histories, mismatched layers, networks, and corrupted snapshot data.
  • Documentation

    • Updated snapshot architecture documentation.

The publisher-productization slice minus sign: an independent party
reproduces a published stele's inscription from its own stores, and an
operator sees what a repository holds without pulling it.

- stelae: a Discarding SteleWriter — every layer framed, hashed and
  compressed exactly as a publish would, into std::io::sink. What comes
  back is the identity, which is the whole of a reproduction.
- snapshot: history_for and same_network move from registry.rs into
  export.rs so a verifier reaches the contiguity rule without a registry
  and without the oci feature; Following reads a predecessor's canonical
  inscription off disk and extends it by the publisher's own rule;
  Attested carries a published history verbatim; verify_reproduction
  rebuilds every layer and compares descriptor by descriptor before
  digest against digest; Standing reads where a node stands against a
  repository before anything is built; registry::verify streams every
  blob against both of its digests; registry::inspect reads the two
  documents and no layer.
- dolos snapshot digest: the canonical inscription and its sha256 from
  local stores and no registry, --chain-from for a chained digest,
  --output for a determinism job. stdout is the document, byte-exact.
- dolos snapshot verify: transport checks by default, --reproduce
  opt-in (it costs what a publish costs). Digests only; signatures and
  chain provenance are phase 5, and the help says so.
- dolos snapshot inspect: sequence, position, history, one line per
  layer with the compressed size the manifest carries; --json emits the
  canonical inscription verbatim, which is what --chain-from takes.
- dolos snapshot publish: a repository already at the node's sequence
  reports nothing to publish and exits zero (--require-new makes it an
  error); the gap refusal now names the distance alongside both
  sequences.

The registry end-to-end tests (publish, snapshot_verify,
restore_registry) are #[ignore]d and were run against a spawned
distribution registry; output in the PR.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds deterministic snapshot reproduction, predecessor-history validation, OCI registry verification and inspection, repository standing checks, and digest, verify, and inspect commands. It also adds storage-free transport support and updates ADR 004 wording.

Changes

Snapshot lifecycle

Layer / File(s) Summary
Storage-free export and history contracts
crates/snapshot/src/export.rs, crates/snapshot/src/lib.rs, crates/stelae/src/...
Adds chained and attested histories, standing classification, reproduction comparison, and the Discarding transport.
Registry standing, verification, and inspection
crates/snapshot/src/registry.rs
Adds repository standing checks, streamed layer verification, and metadata inspection APIs.
Snapshot command flows
src/bin/dolos/snapshot/*
Adds epoch-range handling and the digest, verify, and inspect commands. Publication now checks repository standing and supports --require-new.
Reproduction and registry validation coverage
crates/snapshot/tests/*, crates/stelae/tests/*, tests/snapshot_publish.rs
Adds unit, integration, OCI, tampering, reproduction, inspection, and CLI output tests.
ADR implementation wording
adrs/004_stelae_snapshots.md
Removes CI boundary-check details from ADR 004.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Operator
  participant SnapshotCLI
  participant Registry
  participant LocalStores
  participant Discarding
  Operator->>SnapshotCLI: run verify or digest
  SnapshotCLI->>Registry: inspect or verify stele
  Registry-->>SnapshotCLI: metadata, layer verification, identity
  SnapshotCLI->>LocalStores: load stores and build plan
  LocalStores-->>SnapshotCLI: export inputs
  SnapshotCLI->>Discarding: reproduce layers without persistence
  Discarding-->>SnapshotCLI: canonical inscription and digest
  SnapshotCLI-->>Operator: verification or reproduction result
Loading

Possibly related PRs

  • txpipe/dolos#1168: Extends earlier snapshot export, publishing, and CLI functionality.
  • txpipe/dolos#1173: Builds on predecessor history, registry publishing, and layer reuse APIs.
  • txpipe/dolos#1167: Provides the streaming writer infrastructure used by the new reproduction flow.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main Stelae publisher additions: digest, verify, and inspect workflows.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/stelae-publisher-commands

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@scarmuega
scarmuega marked this pull request as ready for review August 10, 2026 14:47

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (5)
crates/snapshot/src/export.rs (1)

651-673: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Check the network before the walk, as the sequence is checked.

verify_reproduction refuses a sequence mismatch before it touches a store. It does not refuse a network mismatch. If the local stores hold another chain, the walk runs to completion and the divergence surfaces at the final digest comparison as subject: "the inscription" with the reason "the divergence is in a generic field (position, parameters, compression, history) or in layer order".

Two consequences. The operator pays the full compression cost to learn it. The report names a generic field instead of the network.

same_network is in this module and takes exactly these two arguments.

♻️ Proposed check before the export
     if plan.sequence != published.sequence {
         return Err(Error::ReproductionMismatch {
             subject: "sequence".to_owned(),
             reason: format!(
                 "the published stele is sequence {} and these stores stand at sequence {}; a \
                  reproduction runs over stores at the epoch the stele was published from",
                 published.sequence, plan.sequence,
             ),
         });
     }
 
+    // The same refusal a publish makes, for the same reason the sequence is
+    // checked here: a store on another chain cannot reproduce this stele, and
+    // finding that out should not cost a full walk.
+    same_network(published, plan)?;
+
     let reproduced = export(
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/snapshot/src/export.rs` around lines 651 - 673, Update
verify_reproduction to call same_network with published and the local
archive/state context before beginning the store walk, alongside the existing
sequence validation. Return the appropriate network-specific
ReproductionMismatch error immediately when the networks differ, preserving the
current sequence check and reproduction flow for matching networks.
crates/stelae/src/transport.rs (1)

342-364: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider extracting the shared layer_sink prologue.

This is the third copy of the same prologue. SteleDir::layer_sink in crates/stelae/src/dir.rs (lines 300-341) and Registry::layer_sink in crates/stelae/src/oci.rs (lines 861-878) perform the same four steps in the same order: validate the media type, build the LayerHeader, wrap the writer in SeqWriter::with_max_record(LayerWriter::new(...), profile.max_record()), then write the header as the first record.

The doc comment states that the header and the media type are inside the layer's identity. A drift between the three copies would therefore change a diffId. A small shared helper that returns the configured SeqWriter plus the encoded header record would make that drift impossible.

This is optional: the current code is correct and the test a_discarding_writer_reproduces_what_a_directory_stores compares the directory's descriptors against this writer's.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/stelae/src/transport.rs` around lines 342 - 364, Optionally extract
the repeated layer-sink setup shared by SteleDir::layer_sink,
Registry::layer_sink, and this layer_sink method into a helper that validates
the media type, builds LayerHeader, creates the configured SeqWriter, and
returns the encoded header record. Update all three callers to use the helper
while preserving the existing header-first write order and layer identity
behavior.
crates/snapshot/tests/export.rs (1)

630-637: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use stelae::Digest::ALGORITHM for the blob directory name.

crates/stelae/tests/toy_profile.rs builds the same path from stelae::Digest::ALGORITHM. A literal "sha256" here goes stale if the algorithm changes.

♻️ Proposed change
-    let blobs = std::fs::read_dir(temp.path().join("blobs").join("sha256"))
+    let blobs = std::fs::read_dir(
+        temp.path()
+            .join("blobs")
+            .join(stelae::Digest::ALGORITHM),
+    )
         .unwrap()
         .count();
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/snapshot/tests/export.rs` around lines 630 - 637, Update the blob
directory path in the export test to use stelae::Digest::ALGORITHM instead of
the literal "sha256", matching the path construction in the toy profile test
while preserving the existing blob count assertion.
src/bin/dolos/snapshot/mod.rs (1)

15-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Update the heading to match the number of commands that share EpochRange.

publish, digest, and verify all take --epochs and call restrict. The heading says "Two commands, one epoch selection", so it undercounts the sharing it documents.

📝 Proposed doc change
-//! ## Two commands, one epoch selection
+//! ## One epoch selection, shared by every command that takes one
 //!
 //! [`EpochRange`] lives here rather than in either command, because a publisher
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/bin/dolos/snapshot/mod.rs` around lines 15 - 21, Update the module
documentation heading above EpochRange to state that three commands share the
epoch selection. Keep the surrounding explanation and the restrict reference
unchanged.
crates/snapshot/tests/snapshot_verify.rs (1)

392-424: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Bound the socket operations in request.

connect and read_to_end have no timeout. If the registry accepts the connection and then sends nothing, the test blocks instead of failing with a message. Fixture::wait_until_ready in crates/snapshot/tests/registry_fixture/mod.rs documents this exact hazard and bounds every socket operation for it.

🛠️ Proposed fix
-        let mut socket = std::net::TcpStream::connect(&self.address).unwrap();
+        let patience = std::time::Duration::from_secs(30);
+        let endpoint: std::net::SocketAddr = self.address.parse().unwrap();
+
+        let mut socket =
+            std::net::TcpStream::connect_timeout(&endpoint, patience).unwrap();
+
+        socket.set_write_timeout(Some(patience)).unwrap();
+        socket.set_read_timeout(Some(patience)).unwrap();
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/snapshot/tests/snapshot_verify.rs` around lines 392 - 424, Update
Fixture::request to apply the same operation timeout used by
Fixture::wait_until_ready: bound the TcpStream connect, writes, and read_to_end,
and preserve the existing failure behavior while ensuring stalled registry
connections fail promptly with a useful timeout message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/snapshot/src/export.rs`:
- Around line 266-333: Add a unit test in the existing chain tests for
Following::read that supplies valid inscription bytes whose parsed form
re-encodes differently, then assert it returns Error::MalformedInscription. Keep
the test focused on canonical-form rejection and use the existing test helpers
and assertion conventions.

In `@crates/snapshot/src/registry.rs`:
- Around line 435-443: Update the compressed-size verification in the
surrounding verify logic to reject a None result from stele.compressed_size
instead of skipping validation. Preserve the existing mismatch error for
readable claims, and return an appropriate verification error when the
manifest’s compressed size claim is unreadable, so verify cannot succeed for
negative or otherwise invalid claims.

In `@src/bin/dolos/snapshot/digest.rs`:
- Around line 142-153: The canonical stdout writers must explicitly flush and
propagate flush failures. In src/bin/dolos/snapshot/digest.rs lines 142-153,
retain the stdout handle, write the canonical bytes, then call flush with
into_diagnostic().context(...); apply the same change in
src/bin/dolos/snapshot/inspect.rs lines 64-78 within the --json branch before
return Ok(()).

---

Nitpick comments:
In `@crates/snapshot/src/export.rs`:
- Around line 651-673: Update verify_reproduction to call same_network with
published and the local archive/state context before beginning the store walk,
alongside the existing sequence validation. Return the appropriate
network-specific ReproductionMismatch error immediately when the networks
differ, preserving the current sequence check and reproduction flow for matching
networks.

In `@crates/snapshot/tests/export.rs`:
- Around line 630-637: Update the blob directory path in the export test to use
stelae::Digest::ALGORITHM instead of the literal "sha256", matching the path
construction in the toy profile test while preserving the existing blob count
assertion.

In `@crates/snapshot/tests/snapshot_verify.rs`:
- Around line 392-424: Update Fixture::request to apply the same operation
timeout used by Fixture::wait_until_ready: bound the TcpStream connect, writes,
and read_to_end, and preserve the existing failure behavior while ensuring
stalled registry connections fail promptly with a useful timeout message.

In `@crates/stelae/src/transport.rs`:
- Around line 342-364: Optionally extract the repeated layer-sink setup shared
by SteleDir::layer_sink, Registry::layer_sink, and this layer_sink method into a
helper that validates the media type, builds LayerHeader, creates the configured
SeqWriter, and returns the encoded header record. Update all three callers to
use the helper while preserving the existing header-first write order and layer
identity behavior.

In `@src/bin/dolos/snapshot/mod.rs`:
- Around line 15-21: Update the module documentation heading above EpochRange to
state that three commands share the epoch selection. Keep the surrounding
explanation and the restrict reference unchanged.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: bb97b556-442b-4925-9dea-e279171f47f0

📥 Commits

Reviewing files that changed from the base of the PR and between c41df25 and f0c1626.

📒 Files selected for processing (18)
  • adrs/004_stelae_snapshots.md
  • crates/snapshot/src/export.rs
  • crates/snapshot/src/lib.rs
  • crates/snapshot/src/registry.rs
  • crates/snapshot/tests/export.rs
  • crates/snapshot/tests/node/mod.rs
  • crates/snapshot/tests/publish.rs
  • crates/snapshot/tests/registry_fixture/mod.rs
  • crates/snapshot/tests/snapshot_verify.rs
  • crates/stelae/src/lib.rs
  • crates/stelae/src/transport.rs
  • crates/stelae/tests/toy_profile.rs
  • src/bin/dolos/snapshot/digest.rs
  • src/bin/dolos/snapshot/inspect.rs
  • src/bin/dolos/snapshot/mod.rs
  • src/bin/dolos/snapshot/publish.rs
  • src/bin/dolos/snapshot/verify.rs
  • tests/snapshot_publish.rs

Comment thread crates/snapshot/src/export.rs
Comment on lines +435 to +443
if let Some(claimed) = stele.compressed_size(blobs, descriptor)? {
if digests.compressed_size != claimed {
return Err(mismatch(
"compressed size",
claimed.to_string(),
digests.compressed_size.to_string(),
));
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

A manifest with an unreadable compressed size passes verify unchecked.

compressed_size returns None where the manifest claims a negative size. See Stele::compressed_size in crates/stelae/src/oci.rs (lines 1092-1100), which documents that reading. Here None skips the comparison, so verify returns Ok for a manifest whose size claim is impossible.

The doc comment on line 406 states that the layer's byte count "is the compressed size it claims". That does not hold when the claim cannot be read.

inspect prints ? for the same case, which is right for a report. verify is the command that decides an exit code, so it should refuse instead.

🛡️ Proposed refusal for an unreadable size claim
-    if let Some(claimed) = stele.compressed_size(blobs, descriptor)? {
-        if digests.compressed_size != claimed {
-            return Err(mismatch(
-                "compressed size",
-                claimed.to_string(),
-                digests.compressed_size.to_string(),
-            ));
-        }
-    }
+    match stele.compressed_size(blobs, descriptor)? {
+        Some(claimed) if claimed == digests.compressed_size => {}
+        Some(claimed) => {
+            return Err(mismatch(
+                "compressed size",
+                claimed.to_string(),
+                digests.compressed_size.to_string(),
+            ))
+        }
+        // A size a `u64` cannot hold is a manifest this stele cannot be
+        // verified against, rather than one check fewer.
+        None => {
+            return Err(mismatch(
+                "compressed size",
+                "not a readable size".to_owned(),
+                digests.compressed_size.to_string(),
+            ))
+        }
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if let Some(claimed) = stele.compressed_size(blobs, descriptor)? {
if digests.compressed_size != claimed {
return Err(mismatch(
"compressed size",
claimed.to_string(),
digests.compressed_size.to_string(),
));
}
}
match stele.compressed_size(blobs, descriptor)? {
Some(claimed) if claimed == digests.compressed_size => {}
Some(claimed) => {
return Err(mismatch(
"compressed size",
claimed.to_string(),
digests.compressed_size.to_string(),
))
}
// A size a `u64` cannot hold is a manifest this stele cannot be
// verified against, rather than one check fewer.
None => {
return Err(mismatch(
"compressed size",
"not a readable size".to_owned(),
digests.compressed_size.to_string(),
))
}
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/snapshot/src/registry.rs` around lines 435 - 443, Update the
compressed-size verification in the surrounding verify logic to reject a None
result from stele.compressed_size instead of skipping validation. Preserve the
existing mismatch error for readable claims, and return an appropriate
verification error when the manifest’s compressed size claim is unreadable, so
verify cannot succeed for negative or otherwise invalid claims.

Comment thread src/bin/dolos/snapshot/digest.rs
Five findings from the review of #1188, none of which change what the
commands do:

- `digest` and `inspect --json` flush stdout explicitly. The canonical
  inscription carries no trailing newline, so a line-buffered stdout still
  holds its tail when `run` returns and the flush at process exit discards
  its error: `digest > stele.json` onto a full disk or a closed pipe wrote
  a truncated document and exited zero. The `--output` arm already
  propagated through `fs::write`; the two paths now agree.

- `verify_reproduction` checks the network before it walks a store. It
  refused a sequence mismatch up front for exactly this reason, and the
  network is not implied by it — epoch numbers collide across chains, and
  `Attested::of` takes the published history verbatim, so unlike
  `Following::new` nothing else on that path checks it.

- A unit test for `Following::read`'s canonical-form refusal, which had
  none: every caller in the tree hands it canonical bytes, and the branch
  it guards is the one that would chain onto a re-encoded copy and report
  a digest correct for a stele nobody published. Reordered keys and a
  trailing newline, both parseable, both refused.

- `snapshot/mod.rs`'s heading no longer counts the commands that share
  `EpochRange`; `restrict` has three callers now.

- The `Distribution` test client bounds its socket operations, as the
  registry fixture's `wait_until_ready` already does: a wedged registry
  turned an `#[ignore]`d end-to-end test into a hung CI job rather than a
  failing one.

Two further review comments are declined, on the code as it stands:
`registry.rs`'s "unreadable compressed size passes verify unchecked" is
unreachable — `stream_layer` resolves the same layer first and pulls the
blob under a ceiling clamped from that size, so an absent or negative one
fails before the line in question — and the `Digest::ALGORITHM` swap in
`tests/export.rs` is declined because a test pinning on-disk layout should
trip rather than follow an algorithm change silently.
@scarmuega

scarmuega commented Aug 10, 2026

Copy link
Copy Markdown
Member Author

Review pass applied in 5c12704.

Applied (5)

  • Explicit stdout flush in snapshot digest and snapshot inspect --json. The canonical inscription has no trailing newline, so a line-buffered stdout held its tail and the implicit flush at exit discarded the error — dolos snapshot digest > stele.json on a full disk or a closed pipe exited zero with a truncated document. The --output path already propagated via fs::write.
  • verify_reproduction now calls same_network right after the sequence check, before the walk. Epoch numbers collide across chains, and Attested::of (unlike Following::new) checks nothing, so same-sequence/different-chain was reachable and cost a full reproduction to discover.
  • Unit test for Following::read's canonical-form refusal — reordered keys and a trailing newline, both parseable, both MalformedInscription. Every call site in the tree passes canonical bytes, so the branch had no coverage.
  • snapshot/mod.rs heading no longer names a command count; restrict has three callers.
  • Distribution::request in snapshot_verify bounds connect/read/write, matching wait_until_ready in the registry fixture. A wedged registry was a CI hang rather than a failure.

Declined (2)

  • registry.rs:443 unreadable compressed size — unreachable at that line: stream_layer resolves the layer through the same layer_of (returning LayerNotFound if absent) and pulls the blob under a ceiling clamped from descriptor.size, so an absent or negative size fails before the check would run.
  • Digest::ALGORITHM in tests/export.rs — a test pinning on-disk layout should trip on an algorithm change rather than follow it; the "sha256" literal is also the local majority in that file (4 sites vs 2).

The shared layer_sink prologue across dir.rs/oci.rs/transport.rs is deferred: two of the three copies predate this PR, so extracting it is out of this branch's scope.

Gate re-run green: cargo test (workspace, 0 failures), cargo clippy --all-targets --all-features -- -D warnings, cargo +nightly fmt --all -- --check, cargo deny check advisories (ok; the three pre-existing yanked-crate warnings unchanged), cargo tree -p stelae -e normal --all-features still matches nothing ^dolos(-|$). The #[ignore]d registry suites were re-run against a spawned registry:2: snapshot_verify 6/6, publish 7/7, restore_registry 5/5, with the same published identity as before (sha256:ef955801…1b1881f).

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/bin/dolos/snapshot/inspect.rs (2)

140-145: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Keep the aggregate compressed size unknown when any layer size is unavailable.

The per-layer branch correctly prints ? for None. The totals line always prints inspected.total_compressed as a number. The registry implementation clamps negative manifest sizes to zero for that aggregate, so the command can print ? for a layer and a misleading numeric total. (raw.githubusercontent.com)

Compute the total from inspected.compressed, or make the total optional. Print ? when any component is unavailable.

Suggested total handling
+    let total_compressed = if inspected.compressed.iter().any(|size| size.is_none()) {
+        "?".to_owned()
+    } else {
+        inspected.total_compressed.to_string()
+    };
+
     println!(
         "totals:   {} records, {} uncompressed bytes, {} compressed bytes",
         records,
         inscription.uncompressed_size(),
-        inspected.total_compressed,
+        total_compressed,
     );

Also applies to: 157-162

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/bin/dolos/snapshot/inspect.rs` around lines 140 - 145, Update the totals
formatting logic near the compressed display and the corresponding totals line
to derive the aggregate compressed size from inspected.compressed rather than
inspected.total_compressed, preserving an unknown result when any component is
unavailable. Ensure the totals output prints “?” for that case while retaining
numeric output only when all layer sizes are known.

Source: MCP tools


135-138: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Check registerable totals before printing the inspect report.

Lines 138, 158, and 160 print a combined records, uncompressed size, and compressed size without rejecting overflow. Individual fields can be valid while the aggregate exceeds u64::MAX; debug builds can panic, and release builds can show a wrapped total. Use checked accumulation for the record total, and treat Inscription::uncompressed_size() and total_compressed the same way: return a diagnostic instead of printing the wrapped aggregate.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/bin/dolos/snapshot/inspect.rs` around lines 135 - 138, Use checked
accumulation for the `records` total in the inspect-report loop, and apply the
same overflow handling to `Inscription::uncompressed_size()` and
`total_compressed`. If any aggregate exceeds `u64::MAX`, return a diagnostic
before printing the report; otherwise preserve the existing output with
validated totals.

Source: MCP tools

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/bin/dolos/snapshot/inspect.rs`:
- Around line 140-145: Update the totals formatting logic near the compressed
display and the corresponding totals line to derive the aggregate compressed
size from inspected.compressed rather than inspected.total_compressed,
preserving an unknown result when any component is unavailable. Ensure the
totals output prints “?” for that case while retaining numeric output only when
all layer sizes are known.
- Around line 135-138: Use checked accumulation for the `records` total in the
inspect-report loop, and apply the same overflow handling to
`Inscription::uncompressed_size()` and `total_compressed`. If any aggregate
exceeds `u64::MAX`, return a diagnostic before printing the report; otherwise
preserve the existing output with validated totals.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8a506bf9-4e7e-4449-b096-8e70410faeda

📥 Commits

Reviewing files that changed from the base of the PR and between f0c1626 and 5c12704.

📒 Files selected for processing (5)
  • crates/snapshot/src/export.rs
  • crates/snapshot/tests/snapshot_verify.rs
  • src/bin/dolos/snapshot/digest.rs
  • src/bin/dolos/snapshot/inspect.rs
  • src/bin/dolos/snapshot/mod.rs
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/bin/dolos/snapshot/digest.rs
  • crates/snapshot/tests/snapshot_verify.rs
  • crates/snapshot/src/export.rs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant