|
| 1 | +# Issue #294 impl-stage codex-review — round 0 (2026-06-11) |
| 2 | + |
| 3 | +Codex (via the codex-companion runtime, codex-cli 0.130.0) reviewing |
| 4 | +the diff on `impl/issue-294-checkpoint-bundle` vs `main` at the |
| 5 | +reviewed commit `0a2d1c8` — the planless checkpoint-bundle chunk |
| 6 | +(issue #294). Verdict: 3 blocking + 2 non-blocking + 1 |
| 7 | +deferred-needs-issue. All three blocking findings addressed in the |
| 8 | +same PR (fix commit follows the reviewed commit); the non-blocking |
| 9 | +and deferred items are filed as issues. |
| 10 | + |
| 11 | +## [Blocking 1] Snapshot↔bundle race + late `exported_at` stamp |
| 12 | + |
| 13 | +`eden_storage._checkpoint.export_checkpoint` read git state (the |
| 14 | +provider fetch) after the store transaction closed, and stamped |
| 15 | +`exported_at` after that read — chapter 10 §6 requires one logical |
| 16 | +snapshot instant, and §5 makes `exported_at` the §10 recovery-probe |
| 17 | +anchor for the snapshot. The superset argument (roles publish refs |
| 18 | +before committing rows, §12 permits superset bundles) covers the |
| 19 | +common path, but a ref **deleted or force-moved** between snapshot |
| 20 | +and fetch could still leave the bundle missing something the snapshot |
| 21 | +references. |
| 22 | + |
| 23 | +**Resolution:** (a) `exported_at` is now stamped inside the snapshot |
| 24 | +transaction. (b) Non-empty provider bundles are self-validated |
| 25 | +against the frozen snapshot with the importer's own §12 check — |
| 26 | +`_validate_bundle_covers_snapshot` reuses |
| 27 | +`_validate_bundle_cross_references` over the snapshot's re-validated |
| 28 | +variant/idea rows. Mirroring the importer exactly means the check can |
| 29 | +never reject an archive that would have imported; it only moves the |
| 30 | +failure to export time, while the source still exists to retry |
| 31 | +against. The wire route maps the resulting `CheckpointInvalid` to the |
| 32 | +retryable 503 `eden://reference-error/checkpoint-repo-unavailable`. |
| 33 | +Regression tests: `test_repo_bundle_provider_bundle_must_cover_snapshot` |
| 34 | +(bundle missing a snapshot branch → raise, zero archive bytes) and |
| 35 | +`test_repo_bundle_provider_covering_bundle_passes` in |
| 36 | +`test_checkpoint_storage.py`. |
| 37 | + |
| 38 | +## [Blocking 2] Bundle-creation failure still collapsed to the silent zero-byte placeholder |
| 39 | + |
| 40 | +`_compose_repo_bundle` swallowed `CheckpointInvalid` from |
| 41 | +`create_bundle` into `b""` unconditionally — with a remote of record |
| 42 | +configured, that re-creates the #294 silent-non-resumable-archive |
| 43 | +failure mode (200 with an empty bundle), and the importer skips §12 |
| 44 | +validation on zero-byte bundles, so the rows would import without git |
| 45 | +history. |
| 46 | + |
| 47 | +**Resolution:** the swallow now survives only in the no-remote |
| 48 | +posture (a local test repo with no refs is a legitimate empty |
| 49 | +bundle). When `checkpoint_repo_refresh` is configured, any bundle |
| 50 | +failure after a successful sync raises `CheckpointRepoUnavailable` |
| 51 | +(503) — a healthy seeded remote always carries at least the seed ref. |
| 52 | +Regression tests: `test_export_bundle_failure_with_remote_maps_to_503` |
| 53 | +and `test_export_empty_local_repo_without_remote_keeps_placeholder` |
| 54 | +in `test_checkpoint_wire.py`. |
| 55 | + |
| 56 | +## [Blocking 3] §14.1 `format_version` query param ignored |
| 57 | + |
| 58 | +`spec/v0/07-wire-protocol.md` §14.1 defines the optional |
| 59 | +`format_version` query and mandates 400 for unrecognized values; the |
| 60 | +export handler ignored it entirely (pre-existing wave-4 gap, surfaced |
| 61 | +by this review). |
| 62 | + |
| 63 | +**Resolution:** the route now parses `format_version` and rejects any |
| 64 | +value other than `CHECKPOINT_FORMAT_VERSION` with `BadRequest` |
| 65 | +(`eden://error/bad-request`). Tests: |
| 66 | +`test_export_accepts_current_format_version` / |
| 67 | +`test_export_rejects_unrecognized_format_version`. |
| 68 | + |
| 69 | +## [Non-blocking 1] Smoke round-trip equality is counts/id-sets, not full objects |
| 70 | + |
| 71 | +`smoke-checkpoint.sh` Phase 6 compares counts and sorted id sets; |
| 72 | +chapter 10 §9 promises field-level round-trip modulo the documented |
| 73 | +import stamps. Filed as |
| 74 | +[#312](https://github.com/ealt/eden/issues/312) (smoke-depth |
| 75 | +improvement; production path already exercised). |
| 76 | + |
| 77 | +## [Non-blocking 2] Ordering test didn't prove bundle-satisfies-snapshot |
| 78 | + |
| 79 | +Addressed by the Blocking-1 resolution's new coverage tests; the |
| 80 | +post-snapshot-mutation test now also documents that the §12 |
| 81 | +self-validation runs against the frozen snapshot, not a re-read. |
| 82 | + |
| 83 | +## [Deferred] Streaming export for very large archives |
| 84 | + |
| 85 | +The route docstring deferred a streaming temp-file materialization |
| 86 | +model without a tracking issue. Filed as |
| 87 | +[#313](https://github.com/ealt/eden/issues/313) (chapter 10 §6 |
| 88 | +explicitly permits the buffering model; this is scalability |
| 89 | +hardening, not a conformance gap). |
0 commit comments