fix(core): type absent records as Error::NotFound (was InvalidData)#153
Conversation
dirvine
left a comment
There was a problem hiding this comment.
Hermes Agent — Panel Review
Head 38f10df (1 commit on top of main). Verdict: 1 functional gap, otherwise clean.
Local checks (against 38f10df)
cargo check -p ant-core --all-targets— cleancargo clippy --all-targets --all-features -- -D warnings— cleancargo fmt --all -- --check— cleancargo test -p ant-core --lib error::tests— 19/19 pass (incl.test_display_not_found)cargo test -p ant-core --lib classify_error— 1/1 pass
Issue 1 — wrapper-chunk site missed (moderate)
ant-core/src/data/client/data.rs:570-576 — the shrunk-DataMap wrapper-chunk fetch path inside resolve_root_data_map also converts Ok(None) from chunk_get_observed into an error, and currently produces:
Error::InvalidData(format!(
"Missing wrapper chunk {} required to resolve root DataMap",
hex::encode(address)
))That is functionally identical to the three sites this PR retypes: a well-formed address with nothing stored at it, retrieved over a working link. It should also be Error::NotFound(...). The inline comment on lines 558–559 explicitly claims "matching the content-chunk path" — but that path now returns NotFound, so the comment is now stale and the error taxonomy is inconsistent within the same file.
Suggestion: re-type this site the same way as the other three, and refresh the comment to point at Error::NotFound as the source of truth. Small enough to fold into this branch.
Issue 2 — CLI summary mislabels (minor, optional)
ant-cli/src/commands/data/chunk.rs:118-146 — PeerGetSummary::record() has no arm for DataError::NotFound. The fall-through Err(e) arm will print "error message=not found: DataMap chunk not found at abcd" and count it in summary.error instead of not_found. Functional, but chunk get --all-peers accounting and labels will be slightly off for absent records. Optional follow-up: add a DataError::NotFound(_) arm that increments not_found and prints a clean label.
Non-blocking caveats
- Security Audit fail (RUSTSEC-2026-0204, crossbeam-epoch 0.9.18). Confirmed lockfile-inherited —
origin/main's lockfile shows the same advisory. Not introduced by this PR (noCargo.lockdelta). Separate dep-bump workstream. - All other
Error::InvalidDatasites audited and correctly kept:chunk.rs:967/975— mismatched address / hash onSuccessbody (content retrieved, integrity failed). Right.merkle.rs:246/252/261/267— local in-memorychunks_by_addresslookup failures during batch processing. Local state, not network absence. Right.merkle.rs:979— proof-receipt chunk returned with no body. Could argue either way; not claimed in the PR. Fine.
- No backwards-compat risk for
ant-ui(REST API surface unchanged). ant-ffi follow-up is the consumer side per the PR body. - The
classify_errorOk(None) -> Timeoutload-shedding signal viachunk_get_outcomeis preserved pre-conversion, so the adaptive limiter still sees the close-group-exhaustion signal. Correct.
Recommendation
Address Issue 1 (re-type the wrapper-chunk site + refresh the comment) on this branch. Optionally address Issue 2 in the same branch. Then approve.
Security Audit is out of scope here.
A well-formed address with nothing stored at it previously surfaced as
Error::InvalidData ('DataMap chunk not found at …' / 'Missing chunk …
required for data reconstruction'), which reads as a caller bug.
Downstream SDKs inherit that: ant-ffi maps InvalidData to InvalidInput,
so mobile apps show 'invalid input' for a mistyped or absent address
and cannot offer 'not found — check the address' UX.
The not-found signal already existed as chunk_get's Ok(None); this
re-types the three definitively-absent conversion sites (data_map_fetch,
data_map_fetch_from_closest_peers, and the data_download reconstruction
miss) to a new Error::NotFound variant. Integrity failures (mismatched
address / content hash) correctly remain InvalidData.
classify_error treats NotFound as ApplicationError: the peers answered
over a working link, so it must not push the adaptive limiter down.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… tally - resolve_root_data_map: absent wrapper chunk now surfaces as Error::NotFound (was InvalidData), matching the retyped content-chunk path; stale comment refreshed to point at NotFound - ant-cli chunk get --all-peers: PeerGetSummary::record gains a DataError::NotFound arm so absent records tally under not_found with a clean label instead of the generic error bucket; tally test extended Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
38f10df to
e5541cd
Compare
|
Both review items addressed in e5541cd, and the branch is rebased onto main (post-#154) so Security Audit should now run green.
Gates re-run on e5541cd: |
dirvine
left a comment
There was a problem hiding this comment.
Hermes Agent — Follow-up Review
New head e5541cd (1 commit on top of 38f10df). Both prior findings addressed. Approving.
Incremental diff (e5541cd — 2 files, +11/-3)
ant-core/src/data/client/data.rs:572— wrapper-chunk conversion site retyped toError::NotFound(...). Stale comment at lines 558–559 refreshed to point atNotFoundrather thanInvalidData.ant-cli/src/commands/data/chunk.rs:133-136— newErr(DataError::NotFound(_))arm inPeerGetSummary::recordthat incrementsnot_foundand returns"not_found message={e}". Tally test at lines 366–368 + 384 updated (not_foundcount goes from 1 to 2).
Local checks (against e5541cd)
cargo check -p ant-core --all-targets— cleancargo clippy --all-targets --all-features -- -D warnings— cleancargo fmt --all -- --check— cleancargo test -p ant-core --lib data::error::tests— 19/19 passcargo test -p ant-core --lib classify_error— 1/1 pass (compile-time exhaustiveness guard holds with the new variant still covered)cargo test -p ant-cli chunk::tests— 6/6 pass (incl.peer_get_summary_records_tallies)
CI status (latest run on e5541cd)
- Format, Unit Tests (macos + ubuntu), Build (macos + ubuntu), Clippy, Documentation, Security Audit — all PASS
- Security Audit is now green: PR #154 (
chore/sec-audit-crossbeam-0.9.20) landed on main with the crossbeam-epoch bump, so the lockfile no longer triggers RUSTSEC-2026-0204. - E2E Tests (macos + ubuntu), Merkle E2E (macos + ubuntu) — PENDING (still running, ~30–60min). Recheck before final merge but nothing in the diff would change their shape.
Verdict
Both follow-ups land exactly where they should:
- The
NotFound/InvalidDatasplit is now consistent across all four "absent over a working link" conversion sites indata.rs, with the stale comment refreshed. The exhaustiveclassify_error_covers_all_variantstest still passes, confirming the new variant is wired intoclassify_errorcorrectly. - The CLI summary arm matches the
Timeout/Networkarms in shape — same field order, same label prefix — so the JSON consumers won't see a structural surprise; only the counter buckets and labels differ.
No new findings from the follow-up diff.
Recommendation
Approve. Merge once the E2E/Merkle-E2E jobs clear.
Problem
data::Errorhas no not-found variant, so a well-formed address with nothing stored at it surfaces asError::InvalidData— an error class that means the caller sent something malformed. The absent-record signal already exists (chunk_getreturnsOk(None)); it's the conversion sites that stringify it away:data_map_fetch/data_map_fetch_from_closest_peers→InvalidData("DataMap chunk not found at …")data_download's reconstruction miss →InvalidData("Missing chunk … required for data reconstruction")Downstream this misleads real users: ant-ffi maps
InvalidData→InvalidInput, so the mobile SDKs show "invalid input" for a mistyped or absent address and can't offer "not found — check the address" UX. (Mobile tracker: V2-650; sibling of #152 — same lossy-stringification family.)Change
Error::NotFound(String)variant, documented againstInvalidData(absent record vs retrieved-but-malformed/integrity-failing content).chunk.rs) correctly remainInvalidData.classify_error:NotFound→Outcome::ApplicationError— the peers answered over a working link, so absence must not push the adaptive limiter down. (TheOk(None)→ Timeout load-shedding signal insidechunk_get_observedis fed pre-conversion viachunk_get_outcomeand is unaffected.)# Errorssections.Tests
classify_error_covers_all_variants+ the compile-time exhaustiveness guard extended.cargo fmt --check✓ ·cargo clippy --all-targets --all-features -- -D warnings✓ ·cargo test -p ant-core --lib411 passed ·cargo test -p ant-cli18 passed.Consumer
Once this ships in an ant-core release, ant-ffi adds a one-arm From mapping to its existing
ClientError::NotFoundand mobile apps can finally distinguish not-found from caller error.🤖 Generated with Claude Code