Skip to content

feat: typed verification codes + shared hash helpers#288

Open
Nelson Spence (Fieldnote-Echo) wants to merge 3 commits into
feat/deterministic-manifestfrom
feat/typed-verification-codes
Open

feat: typed verification codes + shared hash helpers#288
Nelson Spence (Fieldnote-Echo) wants to merge 3 commits into
feat/deterministic-manifestfrom
feat/typed-verification-codes

Conversation

@Fieldnote-Echo

@Fieldnote-Echo Fieldnote-Echo commented Jul 6, 2026

Copy link
Copy Markdown
Member

Wave U2 of the OrdinalDB post-mortem hardening (post-mortem F2 upstream half + F14). Stacked on #287 (feat/deterministic-manifest) — merge that first; GitHub will retarget this to main.

  • Every verification issue code is now a named pub const in a codes module — zero bare literals at emit sites; internal string-compares switched to consts.
  • #[non_exhaustive] pub enum VerificationCode + ReportIssue::classification() so downstream security code (ordinaldb IntegrityError mapping) branches on typed values, never strings. Serialized report JSON is byte-stable (regression-tested).
  • Structured mismatch detail (expected/actual sha256 + sizes) exposed for lossless downstream error construction.
  • sha256_bytes + bounded sha256_reader sharing sha256_file_bounded's EINTR/bounded core; private sqlite duplicate deduped onto the pub fn (F14 — brief-06 reimplemented this by hand, the drift risk this closes).

Adversarial 3-lens review: clean (low-only findings). Tests: 95-108 passed across feature combos; clippy/fmt clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BQ1JzAocstWiqtCF5J2V7K

Extract the EINTR-safe bounded hashing core from sha256_file_bounded and
share it with a new public sha256_reader; add public sha256_bytes and
dedupe the private sqlite.rs copy onto it.
Add ordvec_manifest::codes with one pub const per issue code (including
the previously format!-built path, metric-spec, and row-id families,
now selected via per-context const structs) and reference the consts at
every emit site, internal compare, and bounded-read call. A source-scan
test rejects new bare literals at emit sites; a value-lock test pins the
security-relevant code strings so silent renames break tests.
Add non_exhaustive VerificationCode with ReportIssue::classification()
over the named code consts so downstream security code branches on
typed values. ReportIssue gains optional artifact_name and
expected/actual sha256+size fields (skip_serializing_if None keeps
existing report JSON byte-stable), populated at the artifact, auxiliary,
and row-identity mismatch sites for downstream IntegrityError
reconstruction.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Add typed verification codes and shared SHA-256 helpers

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Centralize all verification issue codes as codes::* constants to prevent silent drift.
• Add typed VerificationCode classification and structured mismatch details for downstream
 security handling.
• Introduce shared SHA-256 helpers (sha256_bytes, bounded sha256_reader) and regression tests
 for stability.
Diagram

graph TD
  E[("SQLite cache")] --> A["Manifest verifier"] --> C["ReportIssue JSON"]
  A --> B["Issue codes"]
  A --> D["Hash helpers"]
  T["Regression tests"] --> A --> C
  E --> D
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Macro-generated code registry
  • ➕ Single source of truth could generate codes::*, the enum mapping, and tests
  • ➕ Reduces risk of missing new codes in classification or value-lock tests
  • ➖ More metaprogramming complexity and less straightforward code search/grep
  • ➖ Harder to review/maintain without careful macro hygiene
2. Derive-based string↔enum mapping (e.g., `strum`)
  • ➕ Less hand-written match logic for classification()
  • ➕ Can support future bidirectional conversions and display formatting
  • ➖ Adds dependency surface area for a security-adjacent mapping
  • ➖ Still requires a strategy for stable external string values
3. Keep string codes, add typed layer only downstream
  • ➕ No API additions in ordvec-manifest
  • ➕ Avoids introducing optional structured fields in report issues
  • ➖ Downstream remains exposed to typos/drift in raw strings
  • ➖ Doesn’t address emit-site literal reintroduction risk

Recommendation: The PR’s approach is the right trade: centralizing all codes as pub const values plus adding a small, explicitly-scoped VerificationCode classification improves downstream safety without changing the wire format (validated via byte-stability tests). If drift between consts and classification becomes a recurring maintenance issue, consider a macro-generated registry later, but the current explicit mapping is easiest to audit in a hardening context.

Files changed (5) +1281 / -301

Enhancement (1) +803 / -273
lib.rsCentralize issue codes, add typed classification, structured mismatch detail, and hash helpers +803/-273

Centralize issue codes, add typed classification, structured mismatch detail, and hash helpers

• Replaces all emitted issue-code string literals with 'codes::*' constants, including context/field-specific code bundles for path, metric, and row-id validation. Adds '#[non_exhaustive] VerificationCode' plus 'ReportIssue::classification()' for typed downstream branching, and extends 'ReportIssue' with optional artifact/expected/actual SHA-256 and size details while preserving legacy JSON output via 'skip_serializing_if'. Introduces shared hashing helpers ('sha256_bytes', bounded 'sha256_reader') by extracting the bounded/EINTR-safe core from 'sha256_file_bounded'.

ordvec-manifest/src/lib.rs

Refactor (1) +20 / -26
sqlite.rsDeduplicate hashing and adopt shared 'codes::*' constants in sqlite cache flow +20/-26

Deduplicate hashing and adopt shared 'codes::*' constants in sqlite cache flow

• Switches sqlite activation/caching issue codes to the shared 'codes::*' constants and updates bounded-hash calls accordingly. Removes the local 'sha256_bytes' implementation in favor of the public helper and adjusts cache-key hashing to use the '.sha256' string from 'FileHash'.

ordvec-manifest/src/sqlite.rs

Tests (3) +458 / -2
hash_helpers.rsAdd regression tests for 'sha256_bytes' and bounded 'sha256_reader' +53/-0

Add regression tests for 'sha256_bytes' and bounded 'sha256_reader'

• Adds tests asserting consistent digests across file/bytes/reader helpers and validating bounded-reader edge cases (exact bound, over bound, empty input). Ensures new shared hashing core behaves identically across entry points.

ordvec-manifest/tests/hash_helpers.rs

manifest.rsAssert typed classification and SHA-256 mismatch detail in load failure path +17/-2

Assert typed classification and SHA-256 mismatch detail in load failure path

• Extends an existing corrupted-artifact test to validate 'ReportIssue::classification()' returns the expected 'VerificationCode' variant. Verifies expected/actual SHA-256 fields are populated for mismatch issues.

ordvec-manifest/tests/manifest.rs

verification_codes.rsLock code-string values, reject new emit-site literals, and test JSON stability +388/-0

Lock code-string values, reject new emit-site literals, and test JSON stability

• Adds tests that pin security-relevant 'codes::*' values, scan 'src/lib.rs' to prevent reintroducing bare string literals at emit sites, and validate classification mapping coverage. Also verifies verification-report JSON remains byte-stable when structured fields are absent, and that structured fields serialize/round-trip correctly while legacy JSON still parses.

ordvec-manifest/tests/verification_codes.rs

@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Artifact missing misclassified 🐞 Bug ≡ Correctness
Description
ReportIssue::classification maps codes::ARTIFACT_PATH_UNAVAILABLE to
VerificationCode::ArtifactMissing, but resolve_existing_path emits path_unavailable for any
fs::canonicalize error (permission denied, I/O error, etc.), not only NotFound. Downstream consumers
that branch on VerificationCode::ArtifactMissing will incorrectly treat permission or I/O failures
as absent artifacts.
Code

ordvec-manifest/src/lib.rs[3926]

+            codes::ARTIFACT_PATH_UNAVAILABLE => VerificationCode::ArtifactMissing,
Relevance

⭐⭐⭐ High

Team previously accepted fixing misclassified verifier issue codes (row-count limit vs mismatch) in
PR #157.

PR-#157

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The classification arm maps ARTIFACT_PATH_UNAVAILABLE to ArtifactMissing. However,
resolve_existing_path emits path_unavailable for any canonicalize error, not just NotFound. The code
at lib.rs:2547-2555 shows: `Err(err) => { errors.push(ReportIssue::new(issue_codes.path_unavailable,
format!("failed to canonicalize {}: {err}", resolved_path.display()))); return None; }` — this
catches all IO errors. The same pattern applies to all PathIssueCodes instances
(ARTIFACT_PATH_ISSUES, ROW_IDENTITY_PATH_ISSUES, etc.).

ordvec-manifest/src/lib.rs[2547-2555]
ordvec-manifest/src/lib.rs[3922-3950]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description

`ReportIssue::classification()` maps `codes::ARTIFACT_PATH_UNAVAILABLE` to `VerificationCode::ArtifactMissing`. However, `resolve_existing_path` emits `path_unavailable` for **any** `fs::canonicalize` error (permission denied, I/O error, etc.), not only `NotFound`. Downstream consumers that branch on `VerificationCode::ArtifactMissing` will incorrectly treat permission or I/O failures as absent artifacts.

## Issue Context

`resolve_existing_path` in `lib.rs` emits `issue_codes.path_unavailable` for any `Err(err)` from `fs::canonicalize`, regardless of `err.kind()`. The classification arm at line 3926 then maps this code to `ArtifactMissing`, conflating two distinct failure modes.

## Fix Focus Areas

- `ordvec-manifest/src/lib.rs[3843-3864]` — add a new `ArtifactUnavailable` (or similar) variant to `VerificationCode`
- `ordvec-manifest/src/lib.rs[3922-3950]` — map `codes::ARTIFACT_PATH_UNAVAILABLE` to the new variant instead of `ArtifactMissing`
- `ordvec-manifest/tests/verification_codes.rs[199-287]` — update the classification round-trip test to cover the new variant

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

match self.code.as_str() {
codes::ARTIFACT_SHA256_MISMATCH => VerificationCode::ArtifactSha256Mismatch,
codes::ARTIFACT_FILE_SIZE_MISMATCH => VerificationCode::ArtifactFileSizeMismatch,
codes::ARTIFACT_PATH_UNAVAILABLE => VerificationCode::ArtifactMissing,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

1. Artifact missing misclassified 🐞 Bug ≡ Correctness

ReportIssue::classification maps codes::ARTIFACT_PATH_UNAVAILABLE to
VerificationCode::ArtifactMissing, but resolve_existing_path emits path_unavailable for any
fs::canonicalize error (permission denied, I/O error, etc.), not only NotFound. Downstream consumers
that branch on VerificationCode::ArtifactMissing will incorrectly treat permission or I/O failures
as absent artifacts.
Agent Prompt
## Issue description

`ReportIssue::classification()` maps `codes::ARTIFACT_PATH_UNAVAILABLE` to `VerificationCode::ArtifactMissing`. However, `resolve_existing_path` emits `path_unavailable` for **any** `fs::canonicalize` error (permission denied, I/O error, etc.), not only `NotFound`. Downstream consumers that branch on `VerificationCode::ArtifactMissing` will incorrectly treat permission or I/O failures as absent artifacts.

## Issue Context

`resolve_existing_path` in `lib.rs` emits `issue_codes.path_unavailable` for any `Err(err)` from `fs::canonicalize`, regardless of `err.kind()`. The classification arm at line 3926 then maps this code to `ArtifactMissing`, conflating two distinct failure modes.

## Fix Focus Areas

- `ordvec-manifest/src/lib.rs[3843-3864]` — add a new `ArtifactUnavailable` (or similar) variant to `VerificationCode`
- `ordvec-manifest/src/lib.rs[3922-3950]` — map `codes::ARTIFACT_PATH_UNAVAILABLE` to the new variant instead of `ArtifactMissing`
- `ordvec-manifest/tests/verification_codes.rs[199-287]` — update the classification round-trip test to cover the new variant

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

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.

2 participants