Dual-agent review — request_changes

• Verdict: request_changes
• Risk class: medium
• Auto-merge advisory: 👎 thumbs down — verdict is request_changes; needs human-gated follow-up

Advisory only: no merge was attempted.

Risk capstone

Cross-reference: reviewer findings vs high-risk surfaces

Reports: .git/dual-review/pr-91

Review fixes pushed — 4290b09

Addressed all four actionable findings from the dual-agent review (the lockfile finding was already marked a false positive — package-lock.json is gitignored here):

Reverse partition-drift (Codex, major) — commitBatch now always inspects the existing table when appending: if the dataset derives no partitioning but the table on disk is partitioned, it throws iceberg_partition_spec_drift instead of silently skipping the guard and reporting unpartitioned on the span. New test added beside the forward-drift case (commitBatch rejects reverse drift: null partitioning onto a partitioned table). LLP 0022#drift-rejection updated to record the guard as bidirectional.

Sort asserted only as metadata (Claude, major) — both the commitBatch integration test and the partitioned smoke now read a day-partition parquet file back with hyparquet and assert the conversation_id rows come back in sorted order (cA,cB from cB,cA input). This fails on an icebird that records the sort order but writes unsorted (e.g. 0.8.5). Verified locally against installed icebird 0.8.9: npm test 810 pass / 0 fail, npm run smoke -- iceberg_export_partitioned_local_fs passes.

hyp sink maintain CLI text (Claude, minor) — docstring, per-dataset action label (compaction_unsupported → compaction_out_of_band), and summary line reframed to "compaction not run by this sink — out-of-band only, see LLP 0022", matching maintenance.js and the smoke.

Stray </content> in LLP 0022 (Claude, minor) — removed.

🤖 Generated with Claude Code

🧭 Decision map — where to spend your attention

Companion to the dual-review verdict. This casts no verdict — it points at the 6 forks where the author made a real choice, so you can skim the rest.

Scanned: 33 hunks across 21 files. Most is mechanical: the verbatim promotion of partitionSpecForDeclaration/validatePartitionSpecStability from src/core/cache/iceberg/ to src/core/iceberg/ with re-export shims, ~300 lines of LLP 0022/0003/0014 documentation, new test/smoke fixtures, and signature propagation of the partitioning input. The decisions worth your eyes, in order:

1. Existing tables hard-fail on upgrade — drift is rejected, not migrated · unhappy-path policy

hypaware-core/plugins-workspace/format-iceberg/src/commit.js:108

throw newError(
  'iceberg_partition_spec_drift',
  `iceberg-format: partition spec drift at '${input.tableUrl}': ${message}`
)

• Decision: an append whose derived day-grain spec doesn't match the table's current spec throws a permanent iceberg_partition_spec_drift; partition evolution is unsupported, and the documented operator path is "export to a new prefix" (LLP 0022#drift-rejection).
• Alternative not taken: detect "existing unpartitioned table + newly derived partitioning" and continue unpartitioned with a logged warning (keeping a working pre-upgrade export alive), or auto-roll to a new table prefix.
• Check: the first post-upgrade append to every pre-existing V1 export table whose dataset declares primaryTimestampColumn (ai-gateway, gascity, otel all do) lands exactly here — the PR's own test (iceberg-partitioning.test.js:181) proves this shape fires — and the daemon then re-fails every tick with no in-product migration. Confirm that is the intended upgrade story.

2. icebird 0.8.5 → 0.8.9 — one line that swaps the shared write engine · new dependency

package.json:41

"icebird": "0.8.9",

• Decision: bump the engine that both the export plugin and the intrinsic cache ride, to pick up sort-on-append, scan pruning, and icebergRewrite (icebird [codex] Record AI gateway message context #20/[codex] Add Codex metadata projection #21/[codex] Support positional attach clients #22).
• Alternative not taken: hold the pin until the published 0.8.9 is validated end-to-end here (LLP 0022's own landing requirement). There is no export-only bump — the cache shares the pin, so its write behavior changes with zero cache code in this diff.
• Check: the worktree still resolves 0.8.5 locally, so the suite and smokes validated the old engine. After a fresh install, re-run npm test and the three iceberg smokes — and note sort-on-append, which entry 5 depends on, is asserted nowhere at the row level.

3. Partition axis = writer-owned day(primaryTimestampColumn) · contract / format

hypaware-core/plugins-workspace/format-iceberg/src/partitioning.js:40

iceberg: { fields: [{ column: tsColumn, transform: 'day', required: true }] },

• Decision: the export synthesizes its own day-grain declaration from primaryTimestampColumn, ignoring cachePartitioning for the partition axis.
• Alternatives not taken: inherit cachePartitioning (conversation-identity ⇒ a hard floor of ~1 file per conversation, ~4.5K today and unbounded, that compaction cannot beat), or identity(date) (only works where a day string is precomputed; couples the export to a cache-ism).
• Check: this layout is permanent once written (entry 1 makes drift fatal). Partition values are UTC day ordinals (…_day=20608), not date strings — confirm downstream consumers are fine with that, and that ~25–39K rows/day is the right grain.

4. The drift guard only runs in one direction · unhappy-path policy

hypaware-core/plugins-workspace/format-iceberg/src/commit.js:99

} else if (input.partitioning && priorState.metadata) {

• Decision: spec-stability validation is gated on input.partitioning being truthy — an append with null partitioning is never checked.
• Alternative not taken: a symmetric guard that also rejects (or explicitly preserves-and-reports) when partitioning derives to null but the existing table's spec is non-empty.
• Check: a dataset that loses its primaryTimestampColumn, or whose timestamp column drops from the schema (partitioning.js:34 returns null), silently appends to a partitioned table while the commit span reports hyp_partition_spec: 'unpartitioned'. Reverse drift is undetected and untested.

5. Sort key borrowed from the cache's identity columns, in declared order · contract

hypaware-core/plugins-workspace/format-iceberg/src/partitioning.js:71

const declared = reg.cachePartitioning?.iceberg?.fields ?? []
// ...
if (f.transform !== 'identity') continue

• Decision: the within-partition sort reuses cachePartitioning's identity columns in declared order (asc, nulls-last) — the one place the export reads the cache declaration. Non-identity and schema-missing columns are silently skipped; none ⇒ unsorted.
• Alternative not taken: a writer-owned sort key (matching the partition-axis philosophy) — e.g. conversation_id alone, or with the timestamp as a tiebreaker so rows within a conversation are time-ordered.
• Check: clustering quality hangs on declaration order — conversation_id leads for ai-gateway only because it is declared first — and the sort only materializes if 0.8.9's sort-on-append works (entry 2); tests pin it as metadata, never as row order.

6. Compaction is now possible but deliberately never run by the sink · concurrency / lifecycle

hypaware-core/plugins-workspace/format-iceberg/src/maintenance.js:120

 * @ref LLP 0022#compaction — icebird now exposes `icebergRewrite`
 * (read-rewrite compaction), but the export deliberately does not run it:

• Decision: keep compactionSupported: false, redefining it from "impossible (icebird limitation)" to "not run by this sink — out-of-band only," because day partitions already yield large files and an in-daemon read-rewrite risks the parquet-sink OOM failure mode.
• Alternative not taken: wire icebergRewrite into maintainExportTables now that the engine exposes it, tightening the local-vs-global sort.
• Check: the field's meaning changed while its name and value did not — hyp sink maintain (src/core/cli/core_commands.js:2336-2339) still prints "compaction not supported by icebird," now factually false; and no out-of-band compaction affordance exists yet.

Honorable mentions (real but lower-stakes): partitioning.js:34 — missing/undeclared timestamp ⇒ silently unpartitioned rather than an error; commit.js:262 — currentPartitionSpec quietly falls back to the last spec, then an empty one, before the drift comparison; iceberg-partitioning.test.js:168 — the sort contract is pinned via default-sort-order-id metadata only, so an unsorted write passes; src/core/iceberg/types.d.ts:13 — the promoted core type keeps its Cache… name and cache-iceberg: error prefixes (rename explicitly deferred).

_{Generated by /decision-map. Advisory — directs attention, casts no verdict.}