[WIP] clean up the c1z interfaces by kans · Pull Request #986 · ConductorOne/baton-sdk

kans · 2026-06-25T17:23:52Z

No description provided.

github-actions · 2026-06-25T17:29:14Z

General PR Review: [WIP] clean up the c1z interfaces

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 23465ff7a59e.
Review mode: full
View review run

Review Summary

Scanned the full PR diff for security and correctness. This PR introduces explicit OpenStore/CreateStore lifecycle constructors (deprecating the open-or-create NewStore/NewC1ZFile/C1File), adds EngineLifecycleDriver with SQLite/Pebble implementations, a new engine-neutral GenerateSyncDiffFromFiles and a SanitizeResumableCheckpointWriter capability, and migrates all non-test call sites. New behavior defaults to old behavior, exported additions are additive, and the changes are covered by new tests (open_create_test.go, diff_files_test.go, sanitize_checkpoint_test.go, engine_registry_test.go). The prior review's wrapOpenExistingStoreError finding is now addressed: isStoreCorruptionError uses a corruption allow-list (ErrInvalidFile, EOF, v3 magic/truncation, zstd decode errors) and returns transient I/O errors unwrapped, so they no longer satisfy errors.Is(err, ErrStoreCorrupt).

Security Issues

None found.

Correctness Issues

None found.

Suggestions

pkg/sync/syncer.go:2547-2553 — OpenStore ignores WithEngine and opens by on-disk format, so a resume into an existing SQLite file with storageEngine == EnginePebble no longer auto-converts to Pebble the way NewStore did. Likely intended (safer for resume), but a silent default change worth confirming. (low confidence)

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/sync/syncer.go`:
- Around line 2547-2553: The migration from dotc1z.NewStore to OpenStore/CreateStore
  changes engine-selection behavior for the resume path. Previously NewStore ->
  selectStoreDriver would auto-convert an existing writable V1/SQLite file to Pebble
  in place when WithEngine(EnginePebble) was set. OpenStore deliberately ignores
  WithEngine and opens by the file's on-disk format, so resuming into an existing
  SQLite file while storageEngine == EnginePebble now keeps it SQLite instead of
  converting. Confirm this is intended; if downstream relied on auto-convert-on-open,
  either document the change as a deliberate compatibility break or restore an explicit
  conversion step for this path.

github-actions

No blocking issues found.

github-actions

No blocking issues found.

github-actions

Blocking issues found — see review comments.

github-actions

Blocking issues found — see review comments.

github-actions · 2026-06-29T15:40:24Z

-	if inferred == "" {
+
+	// Auto-select: any Pebble input → Pebble output.
+	if hasPebble {


🟡 Suggestion: Mixed SQLite/Pebble inputs now resolve to Pebble output, and the doc above claims "SQLite inputs in a Pebble run are converted ... automatically; callers are not required to pre-convert." That holds only for the overlay path (compactPebble, which calls convertSQLiteInputToPebble). The fold path (compactPebbleFold) opens each input and uses enginepkg.AsEngine, returning "input is not a pebble c1z" for SQLite inputs — it has no conversion step. Because resolvePebbleMode selects fold purely on size, a mixed-input run that meets the fold size-gate will fail. Consider converting SQLite inputs in the fold path too, or gating fold off when any input is SQLite. (medium confidence)

github-actions

Blocking issues found — see review comments.

github-actions

Blocking issues found — see review comments.

github-actions · 2026-06-30T18:24:24Z

+		errors.Is(err, context.DeadlineExceeded) ||
+		errors.Is(err, syscall.ENOSPC) ||
+		os.IsPermission(err) {
+		return err


🟡 Suggestion: The OpenStore doc comment (line 202) promises transient I/O failures are "returned as-is and do not satisfy errors.Is(err, ErrStoreCorrupt)", but this allow-list only covers context cancellation, ENOSPC, and permission errors. Other genuinely transient failures (e.g. EMFILE/too-many-open-files, EIO, EROFS, temp-dir extraction failures) get wrapped as StoreCorruptError. A downstream caller that quarantines/deletes files on errors.Is(err, ErrStoreCorrupt) could destroy a valid c1z after a transient failure. Consider classifying as corrupt only for known decode errors (e.g. ErrInvalidFile, format/manifest read failures) rather than treating everything off the allow-list as corruption. (medium confidence)

github-actions

Blocking issues found — see review comments.

github-actions

No blocking issues found.

github-actions · 2026-06-30T19:09:59Z

+	// OpenStore is strict about existing files and requires a valid c1z.
+	// CreateStore accepts missing paths and zero-byte placeholders so callers
+	// can touch a target path before the first sync.
+	if stat, statErr := os.Stat(s.c1zPath); statErr == nil && stat.Size() > 0 {
+		store, err = dotc1z.OpenStore(ctx, s.c1zPath, storeOpts...)
+	} else {
+		store, err = dotc1z.CreateStore(ctx, s.c1zPath, storeOpts...)


🟡 Suggestion (low confidence): Behavior change worth confirming. The old NewStore/selectStoreDriver path would auto-convert an existing V1/SQLite file to Pebble in place when WithEngine(EnginePebble) was set and the file was writable. OpenStore deliberately ignores WithEngine and opens by the on-disk format, so a resume into an existing SQLite file while storageEngine == EnginePebble now stays SQLite instead of converting. This is arguably the safer behavior for resume, but it is a silent default change for this code path — confirm it's intended and that no downstream relied on the auto-convert-on-open.

github-actions

No blocking issues found.

kans requested a review from a team June 25, 2026 17:23