Skip to content

Add artifact wire format and docbank-backed local store - #1242

Merged
wesm merged 10 commits into
mainfrom
artifact/1-format-store
Jul 23, 2026
Merged

Add artifact wire format and docbank-backed local store#1242
wesm merged 10 commits into
mainfrom
artifact/1-format-store

Conversation

@wesm

@wesm wesm commented Jul 23, 2026

Copy link
Copy Markdown
Member

PR1 of the artifact-sync split (supersedes the closed #1239, extracted and pared from its reference branch with @maphew as co-author).

This lands the foundation of internal/artifact: the frozen v1 wire format and the local content-addressed store, with no export/import pipeline, transports, server, db, or CLI integration yet.

What's here

  • Wire format types (checkpoint, manifest, segment message, metadata event, raw source) with key-sorted canonical JSON encoding and golden-hash tests. The goldens are byte-identical to the reference implementation and are the frozen v1 format going forward; the raw_source contract (SHA-256 identity, size cap, application/jsonl media-type allowlist, sanitized relative paths) is frozen and validated here even though capture lands in PR3.
  • A reflection parity test that pins manifestSession to db.Session's JSON-visible fields, so incidental session-struct growth cannot silently re-hash every manifest.
  • Zstd wire codec with pinned decoder window, encoded/decoded byte limits, and hash-verified reads (corruption-grade robustness; truncation and bit-rot detection).
  • ArtifactStore interface and the Docbank-backed implementation (go.kenn.io/docbank, new dependency, plus a kit upgrade), with a contract suite that runs against both SQLite drivers (mattn and modernc).
  • Repository: open-on-demand vault ownership under $AGENTSVIEW_DATA_DIR/artifacts. Nothing creates the vault for users who don't opt in; serve integration comes later.
  • config.ValidateArtifactOriginID with table tests.

Deliberately absent, arriving in later PRs

  • Export ledger/pipeline and import (PR2), raw JSONL capture (PR3), folder transport + sync CLI (PR4), metadata/HLC machinery (PR5), GC/maintenance/quarantine listing and the pack scheduler (PR6). A few reference symbols whose only consumers live in those PRs were deferred with them (contextArtifactReader, decoded-limit consts, quarantine listing surface, compression tests); each returns verbatim with its consumer. ErrArtifactUnsupported is frozen store vocabulary whose returning consumer is PR6 maintenance.
  • canonicalArtifactPath is temporarily duplicated into repository.go; PR2's sync.go extraction must reconcile to a single copy.
  • Quarantine keeps its reason parameter (currently discarded): PR6's quarantine listing may persist it once docbank can store a reason.

Where to look

  • internal/artifact/wire.go + format_test.go — the format freeze and golden hashes.
  • internal/artifact/canonical_json.go — deterministic encoder (byte-identical to the reference; it determines every content hash).
  • internal/artifact/store_docbank.go + store_contract_test.go — store semantics: immutable create, conflict detection, verified reads, quarantine/trash, two-driver matrix.

🤖 Generated with Claude Code

wesm and others added 3 commits July 23, 2026 07:48
Co-authored-by: maphew <maphew@gmail.com>
Extracted from the artifact-sync reference branch as PR1 of the split:
frozen wire format types and golden hashes, canonical JSON encoder,
zstd wire codec, logical store interface, docbank-backed store, and
open-on-demand vault repository. Export/import pipelines, transports,
GC/maintenance, and metadata machinery land in later PRs.

Co-authored-by: maphew <maphew@gmail.com>
Co-authored-by: maphew <maphew@gmail.com>
@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (69dced5)

The artifact package is generally sound, but one medium-severity creation-path issue can permanently wedge new artifacts.

Medium

  • internal/artifact/store_docbank.go:68Create accepts a noncanonical media type for a new reference. Because that metadata is immutable, a later retry with the correct media type conflicts, leaving the artifact unusable. Validate mediaType against canonicalArtifactMediaType(ref.Kind) before creation, and add coverage for mismatched media types on previously absent references.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 8m40s

TestOpenRepositoryRetainsAbsoluteCanonicalRoot built its relative data
dir with filepath.Rel against the working directory, which fails on
Windows CI where the checkout (D:) and temp dir (C:) live on different
drives. Chdir to the temp dir's parent and open by base name so the
relative path never crosses volumes.

Co-authored-by: maphew <maphew@gmail.com>
@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (551edb4)

The change is generally sound, but three medium-severity correctness issues should be addressed.

Medium

  • internal/artifact/store_docbank.go:68Create accepts noncanonical media types for new artifacts. Because refs are immutable, an artifact can be permanently published with incorrect metadata, and later retries with the correct media type will conflict. Validate against canonicalArtifactMediaType(ref.Kind) before initial publication and test mismatched creates.

  • internal/artifact/store_docbank.go:382Close sets s.vault = nil without synchronization while other operations may concurrently read it, causing a data race or nil dereference during shutdown. Synchronize vault access and closure, and have post-close operations return fs.ErrClosed.

  • internal/artifact/wire_codec.go:198 — All non-context decode errors are classified as ErrArtifactCorrupt, including transient source and destination I/O failures. This can cause valid artifacts to be quarantined or discarded instead of retried. Preserve underlying I/O errors and reserve ErrArtifactCorrupt for malformed encoding, verification failures, and size-limit violations.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 11m15s

Restore the reference branch's media-type validation in Create: docbank's
own authority conflict only covers mismatches against existing content, so
a new artifact created with a noncanonical media type was silently
persisted forever under an immutable ref. Noncanonical types on new
artifacts now fail with ErrArtifactInvalid; mismatches against existing
artifacts keep returning ErrArtifactConflict.

Close no longer nil-writes the vault pointer, which raced concurrent store
operations during shutdown. Idempotency and post-close rejection both come
from docbank's lifecycle lock; mapDocbankError now surfaces those as
fs.ErrClosed.

Co-authored-by: maphew <maphew@gmail.com>
@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (4457ae2)

One medium-severity issue should be fixed before approval.

Medium

  • internal/artifact/wire_codec.go:198DecodeWire classifies destination write failures, such as disk-full or permission errors, as ErrArtifactCorrupt, falsely marking valid input as corrupt. Tag destination errors separately and return them without wrapping in ErrArtifactCorrupt; add a failing-writer test.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 9m59s

DecodeWire wrapped every non-context failure as ErrArtifactCorrupt,
including write errors from the caller's destination such as disk-full or
permission failures, falsely branding valid wire input as corrupt. Tag
destination writes with an internal sentinel and return those errors
without the corruption wrap; oversize output and malformed encoding still
classify as corrupt.

Co-authored-by: maphew <maphew@gmail.com>
@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (edafdc2)

Review verdict: One medium-severity canonical JSON validation issue should be fixed before merging.

Medium

  • internal/artifact/canonical_json.go:52 — Canonicalization decodes only the first JSON value without verifying EOF. An input such as {"a":1}{"b":2} silently canonicalizes and hashes identically to {"a":1}, losing data and permitting content-hash collisions. After the initial decode, attempt another decode and require io.EOF, rejecting trailing non-whitespace content.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 8m24s

The RawMessage canonicalization path decoded only the first JSON value,
so raw content like {"a":1}{"b":2} silently canonicalized and hashed
identically to {"a":1}, losing data behind a content-hash collision.
Require io.EOF after the first value; trailing whitespace stays accepted
and canonical bytes for valid input are unchanged, as the format goldens
confirm.

Co-authored-by: maphew <maphew@gmail.com>
@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (358eb7e)

Medium-severity issue found: secret-scan state is incorrectly included in manifest serialization.

Medium

  • internal/artifact/manifest_session.go:48SecretLeakCount violates the manifest contract. It is serialized and restored even though secret-scan state is deliberately excluded. This changes manifest hashes after scans and can import a nonzero count without its corresponding findings or rules version.
    • Fix: Remove SecretLeakCount from the wire DTO and conversion paths, explicitly exempt it from the parity test, and verify imported sessions start unscanned.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 9m10s

Replace the pseudo-version pin with the released tag. The delta on the
consumed API is additive (optional create provenance, a Provenance read
method, and an internal open-locking simplification); no signatures used
by internal/artifact changed.

Co-authored-by: maphew <maphew@gmail.com>
@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (943b0a6)

Medium-severity correctness issues remain in artifact error classification and session manifest serialization.

Medium

  • internal/artifact/wire_codec.go:170 — Source-reader errors are wrapped as ErrArtifactCorrupt, so transient network or disk failures may quarantine valid artifacts. Preserve underlying source-read errors separately, while classifying malformed encoding and limit violations as corruption.

  • internal/artifact/manifest_session.go:120 — Quality-signal serialization depends on the transient Session.QualitySignals pointer, causing equivalent database-loaded and JSON-decoded sessions to produce different content hashes. Derive signals from s.StoredQualitySignals() and restore them with ApplyQualitySignals, or serialize only session_quality_signals.

  • internal/artifact/manifest_session.go:48secret_leak_count is serialized and restored even though secret-scan state is explicitly excluded. This can create imported sessions with a nonzero count but no findings or rules version, and makes hashes depend on scan state. Remove or zero the field and add coverage for sessions with nonzero secret findings.


Reviewers: 2 done | Synthesis: codex, 11s | Total: 10m14s

…rors

Drop quality_signals from the manifest session DTO. db.Session's pointer
is load-path-transient — JSON decodes populate it while database scans
fill only hidden scalar columns — so serializing it hashed the same
logical session differently depending on how it was loaded. The
manifest-level session_quality_signals field is the single canonical
carrier (built from StoredQualitySignals, restored with
ApplyQualitySignals), and reference-produced manifests never populated
the inner field, so canonical bytes are unchanged and the goldens hold.
The db.Session parity test documents the exemption and asserts manifest
bytes are pointer-independent.

Tag wire source read failures with a sentinel, mirroring the destination
tagging: network resets and disk errors during decode now surface with
the underlying error preserved instead of ErrArtifactCorrupt. io.EOF
passes through untagged, and a source that ends cleanly but early still
classifies as corrupt.

Co-authored-by: maphew <maphew@gmail.com>
@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (799318a)

Artifact support is well-structured overall, but two medium-severity correctness and validation issues should be addressed.

Medium

  • internal/artifact/manifest_session.go:47SecretLeakCount is serialized and restored without the corresponding findings or SecretsRulesVersion. This breaks the invariant that the count summarizes persisted findings and may cause imported, unscanned sessions to appear to contain secrets or be excluded from recall. Exclude SecretLeakCount from the manifest DTO, or transport and restore the complete secret-finding state atomically.

  • internal/artifact/wire.go:255 — Raw-source validation accepts Windows absolute paths using forward slashes, such as C:/Users/example/session.jsonl, despite requiring relative paths. Add platform-neutral rejection of drive-qualified and volume-relative paths, with coverage for both C:/... and C:....


Reviewers: 2 done | Synthesis: codex, 10s | Total: 12m1s

Raw-source path validation accepted Windows drive-qualified and
volume-relative paths written with forward slashes (C:/Users/...,
C:...), which satisfy the relative-path checks on non-Windows parsers.
Reject any colon in the path: one platform-neutral rule that also covers
NTFS alternate data streams and subsumes the URI-scheme check.

Co-authored-by: maphew <maphew@gmail.com>
@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (07147eb)

The changes are clean at Medium severity and above; only Low-severity issues were reported and are omitted.


Reviewers: 2 done | Synthesis: codex, 5s | Total: 12m7s

@wesm
wesm merged commit 7d9f1c5 into main Jul 23, 2026
23 checks passed
@wesm
wesm deleted the artifact/1-format-store branch July 23, 2026 21:56
wesm added a commit that referenced this pull request Jul 28, 2026
Second PR in the artifact-sync split (follows #1242, which froze the wire format and added the docbank-backed store). Extracted from the closed #1239 branch with review fixes applied during extraction.

## What this adds

- **SQLite publication ledger** (`internal/db`): `artifact_export_queue`, `artifact_publications`, `artifact_publication_revisions`, `artifact_checkpoint_heads`/`_floors`, plus session triggers that enqueue owned-session changes. Triggers are origin-gated — they fire only once an artifact origin exists in `pg_sync_state` — so archives that never opt in carry no queue writes. Trigger DDL lives in Go and is installed after column migrations (drops run before), so no trigger references session columns while migrations run.
- **Go enqueue hooks** for child-only mutations that don't touch trigger-covered session columns: batch message writes (queue generation sampled around the batch, enqueues exactly once when the triggers didn't fire), standalone usage-event replacement, and token-coverage backfill. Queue bootstrap is an explicit call at origin creation, not a migrate-time backfill.
- **Origin lifecycle** (`internal/artifact`): `EnsureOrigin`/`AdoptOrigin`/`StoredOrigin`. Creating or adopting an origin bootstraps the export queue; a failed bootstrap rolls the origin back (deleting the key when there was none before, since the gates test key existence) so a retry re-runs population. Adopting over a different established origin force-requeues every owned session with a generation bump, because prior acknowledgements belong to the old origin.
- **Checkpointed export pipeline** (`internal/artifact`): claims pending queue rows, publishes content-addressed session manifests and segments into the store, records publication revisions, and advances per-origin checkpoint heads with monotone sequence reservation. Stale claims (a writer advanced the generation mid-export) roll back atomically. Incremental export is bounded by the dirty batch; an unchanged archive costs a catalog identity check only. Full export streams all bodies, then re-checks the queue up to 32 settle rounds — hitting the bound returns the accumulated result with an error meaning "made progress, run again."
- **Resync carriage**: `CopySyncStateFrom` carries queue, publication, revision, and checkpoint state across a full resync and re-dirties every copied queue row — the origin gate keeps triggers silent in the rebuild's temp DB, and a parser bump changes manifest hashes anyway, so the exporter must re-verify each session (unchanged content is cheap to skip, being content-addressed).

## Scope and limitations

- Export-side only. Nothing wires the pipeline to the daemon or CLI yet; that lands with folder transport and import in a later PR. There is no import/read path here.
- The ledger is local SQLite state. PostgreSQL push and shared query shapes are untouched.
- The wire format is unchanged: no golden churn, format stays frozen at v1.

## Where to look

- `internal/db/artifact_publication.go` — queue and ledger SQL, origin-gated enqueue
- `internal/db/db.go` — trigger DDL split around column migrations, bootstrap/requeue
- `internal/artifact/export.go` — publish → manifest → checkpoint ordering and claim lifecycle
- `internal/artifact/origin.go` — origin lifecycle and rollback semantics
- `internal/db/orphaned.go` — resync carriage and the pending-flag decision

Co-authored-by: Wes McKinney <wesm@users.noreply.github.com>
cursor Bot pushed a commit to diazMelgarejo/periscope that referenced this pull request Jul 29, 2026
Second PR in the artifact-sync split (follows kenn-io#1242, which froze the wire format and added the docbank-backed store). Extracted from the closed kenn-io#1239 branch with review fixes applied during extraction.

- **SQLite publication ledger** (`internal/db`): `artifact_export_queue`, `artifact_publications`, `artifact_publication_revisions`, `artifact_checkpoint_heads`/`_floors`, plus session triggers that enqueue owned-session changes. Triggers are origin-gated — they fire only once an artifact origin exists in `pg_sync_state` — so archives that never opt in carry no queue writes. Trigger DDL lives in Go and is installed after column migrations (drops run before), so no trigger references session columns while migrations run.
- **Go enqueue hooks** for child-only mutations that don't touch trigger-covered session columns: batch message writes (queue generation sampled around the batch, enqueues exactly once when the triggers didn't fire), standalone usage-event replacement, and token-coverage backfill. Queue bootstrap is an explicit call at origin creation, not a migrate-time backfill.
- **Origin lifecycle** (`internal/artifact`): `EnsureOrigin`/`AdoptOrigin`/`StoredOrigin`. Creating or adopting an origin bootstraps the export queue; a failed bootstrap rolls the origin back (deleting the key when there was none before, since the gates test key existence) so a retry re-runs population. Adopting over a different established origin force-requeues every owned session with a generation bump, because prior acknowledgements belong to the old origin.
- **Checkpointed export pipeline** (`internal/artifact`): claims pending queue rows, publishes content-addressed session manifests and segments into the store, records publication revisions, and advances per-origin checkpoint heads with monotone sequence reservation. Stale claims (a writer advanced the generation mid-export) roll back atomically. Incremental export is bounded by the dirty batch; an unchanged archive costs a catalog identity check only. Full export streams all bodies, then re-checks the queue up to 32 settle rounds — hitting the bound returns the accumulated result with an error meaning "made progress, run again."
- **Resync carriage**: `CopySyncStateFrom` carries queue, publication, revision, and checkpoint state across a full resync and re-dirties every copied queue row — the origin gate keeps triggers silent in the rebuild's temp DB, and a parser bump changes manifest hashes anyway, so the exporter must re-verify each session (unchanged content is cheap to skip, being content-addressed).

- Export-side only. Nothing wires the pipeline to the daemon or CLI yet; that lands with folder transport and import in a later PR. There is no import/read path here.
- The ledger is local SQLite state. PostgreSQL push and shared query shapes are untouched.
- The wire format is unchanged: no golden churn, format stays frozen at v1.

- `internal/db/artifact_publication.go` — queue and ledger SQL, origin-gated enqueue
- `internal/db/db.go` — trigger DDL split around column migrations, bootstrap/requeue
- `internal/artifact/export.go` — publish → manifest → checkpoint ordering and claim lifecycle
- `internal/artifact/origin.go` — origin lifecycle and rollback semantics
- `internal/db/orphaned.go` — resync carriage and the pending-flag decision

Co-authored-by: Wes McKinney <wesm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant