carlos / platform Public

PLAN 1: NOT READY TO BUILD.

Reviewed 296807a2a337 against fetched origin/main at 1c43c2ce82f6, spec 7.22, claim 6.30, and the five prior reviews. This was a static source and plan review: no modifications, fetches, host access, or test runs.

I found 4 P1, 4 P2 and 1 P3 findings.

1. P1 — Two tasks consume fields before the task that creates them. Plan 1:1355 reads edgesFilePlacementView.PrevRelease and .MovesFloorMinRelease. Neither exists on main; Task 4 adds only MovesProtocol. Task 13 creates the other fields. Task 4 therefore cannot pass its build gate.

Separately, Plan 1:2611 requires Task 7b’s heldStageAuthority to read hold_op, but Route.HoldOp and its migration arrive in Task 12.

Fix: introduce the console reader fields in Task 4 and move the registry prerequisite before Task 7b. Also resolve two smaller symbol omissions: Task 3 names ErrOpMismatch, which main does not define, and Task 4 uses fleetBox.FloorArmed without including it in the declared fixture shape.

1. P1 — The shared box-identity test helper introduces an import cycle. Plan 1:961 creates boxid/boxidtest, whose API returns boxid.Record, and rewrites the existing boxid_test.go to use it. That file is currently package boxid.

The resulting test dependency is boxid → boxidtest → boxid. Fix: explicitly migrate those tests to external package boxid_test, or retain an internal helper that does not import the child package. Console tests can use the exported helper.

1. P1 — T7 preparation requires state that cannot exist until after readiness. Plan 1:2611 authorizes root staging only when: - the target registry row already carries hold_op == req.Op; - rec.Move.Inputs.Release == req.Version.

But Plan 1:3198 needs that relay during preparing, before T7 can pass and the move become ready. Plan 1 supplies neither prerequisite. Plan 2 writes Move.Inputs at cut admission (Plan 2:185); its target held row appears after the flip (Plan 2:719).

On the hardened production unit, preparation therefore cannot complete. A nil Inputs also needs explicit handling.

Fix: define preparing-stage authority and the producer of its release identity within Plan 1. Add an integration test starting from an actual MoveStart record, with no target registry row, exercising ErrStageBlocked → root relay → T7 pass. The current readiness test stubs ReleaseReady, so it misses this cycle.

1. P1 — The recovery command is incorrect under systemd, and its test bypasses the failure. Plan 1:3507 embeds printf "%s\n%s\n" directly in ExecStart. Systemd expands %s to the service manager user’s shell; literal percent signs require escaping. Shell quotes do not prevent systemd’s expansion.

With an armed floor, this can reject a valid .prev and select the failed current executable. The proposed test at line 3515 extracts the command and executes it through sh, so it passes without exercising systemd’s processing.

There is also a comparison mismatch: sort -V orders v0.30.0 before v0.30.0-rc.1, admitting that prerelease against a final-release floor, unlike semantic-version comparison.

Fix: use correctly escaped unit syntax or a dedicated helper, enforce the same version semantics as ReleaseAtOrAbove, and test the command after unit expansion.

1. P2 — The held-operation downgrade prohibition covers only rollback. Plan 1:3519 adds the held-row refusal to rollback, while install and restoreFiles check only the minimum release.

With floor v0.30.0, current v0.32.0, and a held operation, installing or recovering v0.31.0 still passes. Main’s recoverIfNeeded calls restoreFiles directly, bypassing rollback. This misses spec:3531: any downgrade of a held participant is refused.

Fix: cover each activating downgrade path. Add armed-floor tests where the candidate remains above the floor but below the current release.

1. P2 — Archive verification accepts bytes after the authenticated final chunk. Plan 1:2570 returns from Open immediately on the final chunk, without requiring ciphertext EOF. Verify hashes through a TeeReader consumed by Open; its subsequent drains consume the decoded stream, not the remaining ciphertext.

Consequently, a valid sealed object with an appended suffix can pass against the original manifest’s sealed digest, although the complete object has a different digest.

Fix: reject trailing ciphertext and add an appended-byte/concatenated-stream test. The existing corruption and truncation cases do not establish this property.

1. P2 — T3 reports shadowing at the wrong configuration layer. Plan 1:2609 collects MergedVersions.Shadowed from the remote instance-config overlay. T3 tests that overlay overriding bundle variables.

The required report concerns the carried <host>.env overriding rendered configuration (spec:2537). Main applies that file later, in ExecBackend.instanceEnv. The proposed collection point cannot observe it.

Fix: collect shadow names during final launch-environment composition. Test a carried .env override with no remote instance overlay.

1. P2 — Commit commands still omit required changes and contradict task boundaries. Plan 1:3544 adds lease.MirrorPrefixKeep, but Task 14’s git add at line 3555 excludes internal/lease. Following it leaves callers committed without their implementation.

Task 11’s command at line 3386 also stages host files explicitly deferred to Task 14. Neither command includes the required .amadan/ci.d/30-minio-contract amendment.

Fix: align file lists, gates and staging commands with the actual task boundary. Verify the staged tree, not just the working tree.

1. P3 — The index retains superseded implementation instructions. Index:166, D3, still describes SDK signing immediately after the guard. Task 6 now correctly requires the guard’s captured timestamp to be passed to the signer. D4 still says the S3 mirror implements VersionedCopier, while the detailed plan preserves the ordinary mirror and introduces a guarded staging adapter.

Task 4 also still says it creates the constant-only floor.go, although Task 1 now owns that file. The cache instructions at index line 183 contradict the supplied machine rule to retain Go’s default cache.

Fix: remove these competing instructions and update the affected “Fixed” dispositions.

The task-by-task audit is:

| Task | Build/API assessment | Test assessment from inspection | |---|---|---| | 0 — Switch | Additive configuration; legacy delegation is explicit. | New switch assertions discriminate; unchanged legacy tests are intentionally baseline checks. | | 1 — Shared types/floor | Definitions now precede most consumers; no duplicate Check remains. | ID, floor and comparison cases target new behavior. | | 2 — Record journal | Main’s record/transition APIs support the additions; guarded-cut signature is corrected. | Identity-conflict and additive-wire tests are meaningful. | | 3 — Claim helpers | Required claim primitives exist on main; clarify ErrOpMismatch. | Admission, touch and released-move evidence cases discriminate. | | 4 — Request door | Blocked by findings 1–2. Selector signatures and liveEdges exist as cited. | Fleet fixtures now exercise real identity verification, subject to the helper fixes. | | 5 — Versioned receipts | Existing mirror/receipt interfaces support the extension. | Multipart and missing-version cases cover the prior gap. | | 6 — Signing guard | Existing lease and signer APIs match; explicit signAt addresses the prior timing defect. | Pause and retry cases test the intended timestamp property. | | 7 — Archive | No additional structural build blocker identified. | Missing trailing-ciphertext case: finding 6. | | 7b — Held helpers | Existing render, staging and strict-process APIs fit. Ordering/runtime blockers: findings 1, 3. | Local helper tests miss the preparing-state integration failure. | | 8 — Timers | Registry timer shape supports the proposed canonical snapshot. | Snapshot/digest cases are meaningful. | | 9 — Environment | Existing launch composition supports extraction. | Classification tests are meaningful; T3 shadow reporting remains incomplete. | | 10 — Budget | Referenced model and arithmetic are available. | Arithmetic/boundary cases are specified; not executed here. | | 11 — Readiness | Pure package can precede Task 14; host implementation is explicitly deferred. | Freshness/provenance checks discriminate; stubbed T7 masks finding 3. | | 12 — Holds | Migration and conditional operations are explicit additions. | Wrong-operation release/delete tests are meaningful; task must move earlier. | | 13 — Floor | Existing applier hooks exist. Findings 4–5 prevent approval. | Shell-only and below-floor tests miss the identified production paths. | | 14 — Integration | Legacy worker is retained; guarded transitions and generation filtering are specified. | Blocked by T7 and incomplete commit contents. | | 15 — Copy gate | Existing gate is available; copying the design record into the implementation tree is explicit. | Separate inventory and pending/default-off checks discriminate. | | 16 — Final gate | Cannot certify until preceding blockers are fixed. | No gate was run during this review. |

Claim independence: I found no direct build dependency on unmerged claim Plans 5, 6 or 8. The claim primitives used by Plan 1 are present on main, and the shared MoveInputs definition has an explicit ownership rule. The preparation dependency in finding 3 is on later moves work and must still be removed.

Shippability: D13 now correctly preserves the legacy door and worker and explicitly names the independently armed floor exception. However, the plan cannot keep every intermediate commit shippable because of the build-order/import-cycle failures and incomplete commits. The armed-floor implementation also remains incorrect.

Test confidence: this review establishes concrete gaps in the proposed tests, not an executed red/green result. Existing legacy tests should already pass without the new code; that is intentional. The missing integration and negative cases above are needed before claiming the new tests establish the required behavior.

---

FIRST PROMPT: Focused read-only review of moves Phase 1 Plan 1 and its index before implementation.

WHAT: Check buildability, claim independence, dormant behavior, tests and spec coverage.

STATUS: Review complete: PLAN 1 NOT READY TO BUILD; 4 P1, 4 P2, 1 P3.

NEEDED: Nothing for this review.

RECOMMENDED: Correct the prerequisites and preparing-stage authority first, then repair floor enforcement and its tests.

SESSION: Safe to close. No files changed, tests run, or background processes started.