Feature: iceberg by philcunliffe · Pull Request #8 · hyparam/hypaware

philcunliffe · 2026-05-21T20:02:04Z

Auto-generated by /feature-launch to host the integration branch for feature iceberg.

Work beads file individual sub-PRs into this branch via the refinery feature-flow loop. When all work + review + ship beads complete, the ship formula flips this PR to ready-for-review.

See /feature-flow for the flow architecture.

…ter kernel support (hy-cxl1) (#12) * feat(kernel): formalize blob-store API + table-format writer support (hy-cxl1) Promote hypaware.blob-store from a metadata-only marker into a real object API (BlobStore: put/get/list/delete) and add hypaware.table-format as a new capability for directory-layout + manifest writers (Iceberg). Public types (collectivus-plugin-kernel-types.d.ts) - Add BlobStore, PutObjectInput/Result, GetObjectInput/Result, ListObjectsInput/ListObjectResult, DeleteObjectInput. - Add TableFormatProvider, TableFormatCreateContext. - Document the BlobStore capability VALUE change (object, not marker). - Add optional `encoder` field to SinkInstanceConfig for table-format writer inner-encoder pinning. Kernel - src/core/config/validate.js: add CAP_TABLE_FORMAT constant; allow blob sink writers to provide either hypaware.encoder (legacy) or hypaware.table-format (new); add `sink_writer_invalid`, `sink_destination_invalid`, `sink_encoder_invalid` error_kinds; legacy `sink_pair_incompatible` retained for encoder-writer flow. - src/core/registry/sinks.js: new `kind: 'table-format'` variant on `instantiate(...)` that resolves TableFormatProvider + BlobStore + inner encoder and invokes provider.createSink. Emits `sink.resolved` / `sink.register` logs with table-format-specific attributes; supports tags intersect with the inner encoder. Plugins - @hypaware/local-fs: provide a full BlobStore implementation (put/get/list/delete) backed by <HYP_HOME>/exports (or pin via plugin config `exports_dir`). Sink contribution unchanged. putObject honours ifNoneMatch='*' via O_EXCL; rejects keys that escape the root. - @hypaware/s3: provide a full BlobStore over the AWS SDK (Put/Get/List/Delete). Honour ifNoneMatch via S3's IfNoneMatch header (maps PreconditionFailed/HTTP 412 to blob_precondition_failed). Activation reads plugin-level config; when bucket is unset, provide a sentinel BlobStore that throws s3_blob_store_unconfigured on use. Tests + smokes - test/core/blob-store.test.js: local-fs round-trip + in-memory fixture exercising the full BlobStore contract. - test/core/sinks-dispatch.test.js: kernel instantiate dispatch for table-format sinks; new error_kind coverage on validateConfig. - test/plugins/s3-blob-store.test.js: s3 BlobStore against a fake S3 client (put/get/list/delete/ifNoneMatch/escape-rejection). - hypaware-core/smoke/flows/blobstore_api_local_fs.js: hermetic smoke that activates @hypaware/local-fs, resolves the capability, and exercises every BlobStore method directly (no sink tick). 99/99 tests pass; 30/30 smokes pass; lint + typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(local-fs): hold a FileHandle in getObject so unlink-after-get is safe (hy-cxl1) The pre-fix getObject called createReadStream(path), which opens the underlying file asynchronously inside the ReadStream's _construct. A consumer that called getObject and then deleted (or discarded) the key before consuming the body raced the lazy open: the unlink would land before the open syscall, the stream would emit an unhandled ENOENT error event, and Node would surface it as an uncaughtException. Linux CI exposed this on the existing 'deleteObject removes the key and is idempotent' test (test/core/blob-store.test.js:114); macOS scheduling happened to win the race in local runs. Fix: open the file via fs.open() before returning, then back the result stream with the held FileHandle (handle.createReadStream). The open is now settled at await time, so any ENOENT surfaces from getObject itself (and we already map ENOENT → null). After the handle is held, a subsequent unlink is benign on POSIX — the fd stays valid until the stream closes. Tests - Drain the body in the existing deleteObject test so the held FileHandle is released before the next iteration. - New 'getObject body survives a concurrent unlink' regression that uses a sync unlink between getObject and body-consume to force the race window deterministically on every platform. 100/100 tests pass; lint + typecheck clean; blobstore_api_local_fs smoke ok. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…fk) (#16) Add the @hypaware/format-iceberg plugin providing `hypaware.table-format@1.0.0`. The TableFormatProvider builds an Iceberg writer over any BlobStore destination (local-fs in V1) and an inner parquet encoder, leveraging icebird's `fileCatalog` + `icebergAppend` with a thin BlobStore-backed Resolver/Lister adapter so metadata + data files round-trip via the destination's put/get/list/delete contract. - Plugin tree under `hypaware-core/plugins-workspace/format-iceberg/`: manifest, index, table-format provider, blob-io adapter, schema, commit, state markers. - BlobStore IO wraps the destination's BlobStore behind icebird's Resolver/Lister API. Writer reuses hyparquet-writer's ByteWriter and flushes via `putObject({ ifNoneMatch })`, surfacing 412 conflicts as `iceberg_commit_conflict`. - Schema mapper assigns stable field ids on first commit and reconciles ids on append with `mergeFieldIdsFromTable`; incompatible type changes, new required columns, column removals, and nullable→ required tightenings raise `iceberg_schema_incompatible`. - State markers under `state/exported-batches/<sink>/<dataset>/<batch>.json` let retries detect already-committed batches. - Telemetry spans: `iceberg.export_batch`, `iceberg.table.load`, `iceberg.table.create`, `iceberg.snapshot.commit`. Log row `iceberg.activate` fires once per boot. - Hermetic smoke `iceberg_export_local_fs` activates real format-iceberg + format-parquet + local-fs, lands 5 rows in the cache, drives one `exportBatch` through `kernel.sinks.instantiate`, asserts that metadata + data files land on disk, that `icebird` reads them back, and that the marker + telemetry are recorded. - Added to `V1_BUNDLED_PLUGIN_ALLOWLIST` so it appears in `hyp plugin list`. The `cli_bundled_plugins_activated` smoke is updated to reflect the new 9-plugin bundled count (`plugins_skipped=3`, format-iceberg in the skipped set). - Traditional tests cover schema mapping, blob-io path/key rendering, marker roundtrip, manifest validity, allowlist membership, provider shape, and commit-conflict normalisation through a real local-fs BlobStore. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…17) Wire @hypaware/format-iceberg to commit through @hypaware/s3 atomically: - s3 BlobStore surfaces bucket/prefix as advisory properties for downstream telemetry, and tags every thrown AWS SDK error with a stable errorKind via classifyAwsError so the iceberg adapter no longer re-classifies SDK shapes. - format-iceberg blob-io adapter maps the new error kinds: blob_precondition_failed -> iceberg_commit_conflict (412) s3_access_denied / s3_bucket_missing / s3_credentials_missing / s3_region_mismatch / s3_config_invalid / s3_blob_store_unconfigured -> iceberg_blob_store_missing everything else -> iceberg_data_write_failed (writes) iceberg_metadata_read_failed (reads) An optional onWrite observer hands every successful metadata write (key + etag + ifNoneMatch) back to the sink so the commit span can surface S3's ETag without coupling the icebird writer surface to OTEL. - iceberg.export_batch / table.load / snapshot.commit / table.create spans now carry hyp_blob_store_kind, bucket, prefix, and (S3 only) etag on successful commits. - New traditional tests: full S3-error -> Iceberg-error mapping table, concurrent-commit retry past a transient PreconditionFailed, initial create race surfaces iceberg_commit_conflict (no retry), reader recovers latest snapshot when version-hint.text is stale or missing, onWrite observer contract. - New hermetic smoke iceberg_export_s3_fixture: real plugin activation over a fake S3 client, forces a v?+ ifNoneMatch collision and asserts the retry path lands a snapshot, end-to-end icebird readback, and bucket/prefix/etag attributes on the commit span. - New acceptance smoke iceberg_export_s3_roundtrip (env-gated by HYP_SMOKE_REAL_S3=1) for real S3 / MinIO round-trip with per-run prefix isolation and best-effort cleanup. Drive-by: fix racy await in test/plugins/s3-export-batch.test.js so the sink registration completes before the helper asserts on it.

philcunliffe · 2026-05-22T16:35:23Z

Dual-agent review — `request_changes`

Verdict: request_changes
Risk class: high
Blast-radius bead: hy-hal9

Risk capstone

Cross-reference: reviewer findings that intersect high-risk surfaces

Source	Finding (severity, evidence)	Intersects
codex.md	Behavioral Correctness — `probeTable()` treats any `iceberg_metadata_read_failed` as "table missing" but reader emits the same kind on transient failures (major, high). Evidence: `format-iceberg/src/commit.js:199`, `format-iceberg/src/blob-io.js:152`	Targets (format-iceberg/blob-io.js, format-iceberg/commit.js)
codex.md	Concurrency, Ordering & State Safety — Marker skip claims "equal or superseded snapshot" but `markerSubsumedBySnapshot` returns hardcoded `false`; retries of already-committed batches risk duplicates (major, high). Evidence: `format-iceberg/src/state.js:124`, `format-iceberg/src/state.js:146`, `format-iceberg/src/table-format.js:274`	Targets (format-iceberg/state.js, table-format.js); Concurrency surface ("Cross-tick concurrency on the same sink handle is not prevented by the driver … orphan data-file writes from a losing commit accumulate"); Risks (cross-tick concurrency bullet)
codex.md	Security Surface — `listObjects({prefix:''})` uses `Prefix=<normalized>` without trailing slash; sibling-namespace keys (e.g. `hyp/exports2/...`) can be listed and surfaced by `relativeFromFullKey`, allowing cleanup/delete loops to touch out-of-scope objects (major, high). Evidence: `s3/src/blob-store.js:76`, `s3/src/blob-store.js:161`, `smoke/flows/iceberg_export_s3_roundtrip.js:243`	Targets (s3 plugin files listed under "Packages / directories"); Direct callers (`iceberg_export_s3_roundtrip.js:164`); Cross-package usage (validateS3SinkConfig reuse)
claude.md	(no findings — skipped per `code-review-quiet` step 1 eligibility check; PR #8 is a draft integration branch)	(none)

Blast radius

Kernel sink registry's instantiate has no production caller — the
new table-format dispatch path
(src/core/registry/sinks.js:193-194) is exercised only by smoke
flows. Production config→instantiate wiring does not exist for any
sink kind today; this PR widens that gap with a third dispatch
shape, but does not yet need to close it (V1 boundary).
src/core/registry/sinks.d.ts is not updated. Consumers importing
types from the local barrel (from '../registry/sinks') cannot
access BlobStore, TableFormatProvider, or
TableFormatCreateContext. They must import directly from
collectivus-plugin-kernel-types. Easy miss for the next plugin
author.
format-iceberg/src/table-format.js:3 reaches into kernel
internals via ../../../../src/core/observability/index.js,
bypassing the capability contract. If the observability module
moves, the iceberg plugin breaks. local-fs and s3 do not have
the same reach — they use hypaware/core/sinks package-export.
Default inner-encoder fallback (@hypaware/format-parquet when
config.encoder is omitted) is documented in
src/core/config/validate.js:170-183 and
collectivus-plugin-kernel-types.d.ts:381-389 but never
implemented. Every smoke flow hard-codes the pin. Whoever lands the
production config→instantiate driver must implement the default
resolution, otherwise an unpinned table-format sink will receive
encoder: undefined and instantiateTableFormat will throw
(registry line 286).
The sink_pair_incompatible error_kind is partially superseded:
encoder-writer flow still emits it; table-format flow emits the new
kinds (sink_writer_invalid, sink_destination_invalid,
sink_encoder_invalid). External consumers — error-classifier
dashboards, status renderers, support scripts — that grep for the
old kind alone will miss table-format failures. test/core/config.test.js
was updated to reflect the new kind for one case, but the broader
consumer surface is not enumerated.
Iceberg exportDataset drains all rows for a dataset into one
in-memory array before commitBatch. For datasets larger than
available memory this is a hard ceiling, not a soft slowdown. No
streaming path exists yet; the comment in local-fs/src/index.js
acknowledges the parallel issue on the blob-write side.
Cross-tick concurrency on the same sink handle is not prevented by
the driver. Iceberg's ifNoneMatch on metadata.json makes data
corruption impossible, but orphan data-file writes from a losing
commit accumulate in the BlobStore until external compaction.
S3 client destroy() may race with in-flight PUTs during
shutdown. Errors are swallowed; nothing in the test suite exercises
this lifecycle gap.
storage.flushTable is consumed via duck-typed casts in three
files (local-fs/src/index.js, s3/src/index.js,
format-iceberg/src/table-format.js) and a fourth on master
(src/core/cli/core_commands.js:725). The kernel storage interface
doesn't yet declare flushTable, so a future change to its
signature has no compile-time signal at the consumer sites.
validateS3SinkConfig is reused for both sink-instance and
plugin-level s3 config validation (s3/src/index.js:resolveBlobStore
vs the sink create path). The function name suggests
sink-instance scope only — a future change targeting sink-instance
behavior will silently change plugin-level activation too.
Test injection keys __blobStoreClientFactory and __clientFactory
are convention-only (double-underscored to avoid JSON-shape
collision). Safe by current parsers but not enforced. A config
file constructed by a misbehaving generator could still trigger the
test client path.

Codex review

Fix Validations

local-fs `getObject` stream/open race (unlink-after-open)

Status: correct
Evidence: blob-store.js#L166, blob-store.test.js#L137
Assessment: The implementation now opens a FileHandle first and streams from that handle, which removes the lazy-open race the new regression test describes.

Findings

Behavioral Correctness

Severity: major
Confidence: high
Evidence: commit.js#L199, blob-io.js#L152
Why it matters: probeTable() treats any iceberg_metadata_read_failed as “table missing”, but createBlobStoreIO.reader() emits that same kind for transient read failures; this can drive incorrect create/append behavior instead of surfacing the real metadata-read error.
Suggested fix: In isProbeMissError, only treat explicit not-found signals as miss (code==='ENOENT', known “no metadata files” messages), and remove/guard the blanket hypErrorKind==='iceberg_metadata_read_failed' branch.

Concurrency, Ordering & State Safety

Severity: major
Confidence: high
Evidence: state.js#L124, state.js#L146, table-format.js#L274
Why it matters: Marker skip logic claims “equal or superseded snapshot”, but supersedence is hardcoded false; retries of an already-committed batch after later snapshots will re-stage/re-append and risk duplicates.
Suggested fix: Implement real ancestry/supersedence check (snapshot-log/parent walk) or persist enough lineage in marker metadata to prove descendant snapshots safely.

Security Surface

Severity: major
Confidence: high
Evidence: s3/blob-store.js#L76, s3/blob-store.js#L161, iceberg_export_s3_roundtrip.js#L243
Why it matters: listObjects({prefix:''}) uses Prefix=<normalized> (no trailing slash), so keys from sibling namespaces (e.g. hyp/exports2/...) can be listed; relativeFromFullKey can then surface them and cleanup/delete loops can touch out-of-scope objects.
Suggested fix: For non-empty configured prefix, query with Prefix: "${normalized}/" and skip any returned key not under that head before yielding/deleting.

No Finding

Contract & Interface Fidelity
Change Impact / Blast Radius
Error Handling & Resilience
Resource Lifecycle & Cleanup
Release Safety
Test Evidence Quality
Architectural Consistency
Debuggability & Operability

Evidence Bundle

Changed hot paths: format-iceberg/blob-io.js, format-iceberg/commit.js, format-iceberg/state.js, format-iceberg/table-format.js, s3/blob-store.js
Impacted callers: table-format.js#L250, table-format.js#L274, src/core/registry/sinks.js#L275, s3/src/index.js#L368
Impacted tests: iceberg-commit.test.js#L44, iceberg-state.test.js#L108, s3-blob-store.test.js#L106, iceberg-s3-errors.test.js#L85
Unresolved uncertainty: Local worktree contents do not fully match the pasted PR diff; review treated the pasted diff as authoritative and used local files only for caller tracing.

Claude review

Code review

Skipped. PR #8 is a draft auto-generated by /feature-launch to host the
integration/iceberg integration branch — work is filed via sub-PRs and
reviewed there. Per the code-review-quiet skill's step 1 eligibility
check, this PR matches both (b) draft and (c) does not need a code review.

No issues recorded.

Generated with Claude Code

Reports: /Users/phil/testcity/.gc/pr-pipeline/reviews/pr-8 · Bead: hy-40nf · Blast-radius: hy-hal9

* fix(iceberg): scope probeTable misses to ENOENT (codex 1/3) The blob-io reader raises `iceberg_metadata_read_failed` for two structurally different conditions: a true miss (sets `code='ENOENT'`) and a transient read failure (no `code`). The old `isProbeMissError` matched the kind alone, so a flaky read drove the sink into a fresh `create` path against an existing table instead of surfacing the real error. Restrict the miss classification to explicit not-found signals (`code === 'ENOENT'`, "no metadata files" messages) and let any other error propagate so the sink driver can retry the batch (hy-cerp). * fix(iceberg): walk parent snapshots for marker supersedence (codex 2/3) `markerSubsumedBySnapshot` previously returned true only when the marker's snapshot equalled the current snapshot — the documented "superseded" branch was a stub that always returned false. That made the marker-skip logic incorrect for the very scenario it was meant to cover: a retry of an already-committed batch after another commit landed re-staged and re-appended the same rows. Walk `metadata.snapshots` via `parent-snapshot-id` from the current snapshot, looking for the marker's snapshot id. Treat the marker as subsumed iff it sits on the ancestor chain. Bounded by the snapshot array length so a malformed cyclic chain cannot hang the walk. The public API now also accepts the full probe state (`{ metadata, currentSnapshotId }`) so ancestry has the data it needs; a bare snapshot string is still accepted for callers that don't have metadata in hand (equality-only behavior preserved) (hy-cerp). * fix(s3): scope listObjects to '<prefix>/' to stop sibling-namespace leak (codex 3/3) S3's ListObjectsV2 treats `Prefix` as a bare string-prefix match. The BlobStore was passing the configured `normalized` value without a trailing slash, so `Prefix: 'hyp/exports'` would also enumerate keys under `hyp/exports2/...`. `relativeFromFullKey` then surfaced those siblings as if they belonged to this BlobStore, and cleanup / delete loops could touch out-of-scope objects. Force a trailing slash on the S3 Prefix whenever a non-empty configured prefix is in play, and add a defense-in-depth scope guard that refuses to yield any returned key that does not start with `${normalized}/`. The no-prefix case (lists the entire bucket) is preserved (hy-cerp).

philcunliffe force-pushed the integration/iceberg branch from 16d40c5 to 9b46718 Compare May 21, 2026 20:21

feature-launch and others added 3 commits May 21, 2026 17:58

chore: seed integration/iceberg for feature flow

5e82def

philcunliffe force-pushed the integration/iceberg branch from 63d8dab to 20ca78c Compare May 22, 2026 00:58

philcunliffe changed the base branch from integration/s3-sink to master May 22, 2026 00:59

philcunliffe mentioned this pull request May 22, 2026

Fix dual-review findings on PR #8 (iceberg) (hy-cerp) #19

Merged

philcunliffe marked this pull request as ready for review May 22, 2026 17:01

philcunliffe merged commit ee22023 into master May 22, 2026
6 checks passed

platypii deleted the integration/iceberg branch May 28, 2026 06:37

philcunliffe mentioned this pull request Jul 23, 2026

Fix #380: derive clientsPicked from pre-filter selection so fleet-managed machines install skills/agents #382

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Feature: iceberg#8

Feature: iceberg#8
philcunliffe merged 5 commits into
masterfrom
integration/iceberg

philcunliffe commented May 21, 2026

Uh oh!

philcunliffe commented May 22, 2026

Fix Validations

local-fs `getObject` stream/open race (unlink-after-open)

Findings

Behavioral Correctness

Concurrency, Ordering & State Safety

Security Surface

No Finding

Evidence Bundle

Code review

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

philcunliffe commented May 21, 2026

Uh oh!

philcunliffe commented May 22, 2026

Dual-agent review — request_changes

Risk capstone

Cross-reference: reviewer findings that intersect high-risk surfaces

Blast radius

Fix Validations

local-fs getObject stream/open race (unlink-after-open)

Findings

Behavioral Correctness

Concurrency, Ordering & State Safety

Security Surface

No Finding

Evidence Bundle

Code review

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Dual-agent review — `request_changes`

local-fs `getObject` stream/open race (unlink-after-open)