amadan
plans: the Plan 1 review findings (round 3), committed unedited
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 file changed,
+96
−0
docs/superpowers/plans/CODEX-PLAN1-REVIEW-R3.md+96 −0
CI passed — run details
diff --git a/docs/superpowers/plans/CODEX-PLAN1-REVIEW-R3.md b/docs/superpowers/plans/CODEX-PLAN1-REVIEW-R3.md| new file mode 100644 |
| index 000000000..0026c7d16 |
| --- /dev/null |
| +++ b/docs/superpowers/plans/CODEX-PLAN1-REVIEW-R3.md |
| @@ -0,0 +1,96 @@ |
| +**PLAN 1: NOT READY TO BUILD.** |
| + |
| +Reviewed `d5716bf4` against cached `origin/main` **`1c43c2ce82f621c6ac84a623db54f0ef63075603`**, spec rev 7.19, claim rev 6.27 at `989ee349`, and both prior reviews. Fetch failed on the read-only filesystem; a read-only remote-tip lookup then failed DNS resolution. Consequently, remote freshness is unverified. No files changed, hosts contacted, or tests run. |
| + |
| +1. **P1 — The switch does not protect the intermediate commits.** |
| + [Plan 1:585](/home/paulca/amadan.net/carlos/platform-moves-design/docs/superpowers/plans/2026-10-06-moves-phase1-1-copy-and-staging.md:585) removes the existing cut implementation in Task 2; the worker switch only arrives in Task 14. Task 4 references `s.cfg.MovesEnabled`, which neither main nor an earlier task defines, and only explicitly checks it in the request handler—not cut, abort, or resolve. `moves.DefaultEnabled` likewise has no producer before Task 15 uses it. Moreover, [index D13:155](/home/paulca/amadan.net/carlos/platform-moves-design/docs/superpowers/plans/2026-10-06-moves-phase1-0-index.md:155) disables main’s existing move behavior when unset, so it does not meet the literal “off = no behavior change” requirement. Introduce and test the complete compatibility/switch boundary before Task 2 changes behavior. |
| + |
| +2. **P1 — Tasks 2 and 3 cannot pass their commit gates.** |
| + [Plan 1:585](/home/paulca/amadan.net/carlos/platform-moves-design/docs/superpowers/plans/2026-10-06-moves-phase1-1-copy-and-staging.md:585) deletes `MoveRequest` and `MoveComplete` but omits `internal/provision/provision_test.go`: main still calls them at lines 916, 922, 928, 946, 954 and 1005. |
| + [Plan 1:626](/home/paulca/amadan.net/carlos/platform-moves-design/docs/superpowers/plans/2026-10-06-moves-phase1-1-copy-and-staging.md:626) adds `servingClaim() *Claim`; main already defines `servingClaim(t, ctx, store) Observed` in `internal/hostclaim/delete_test.go:19`. Go cannot overload it. Update all removed-function consumers and rename the new fixture. |
| + |
| +3. **P1 — The specified copy presigner does not exist in the pinned SDK.** |
| + [Plan 1:1334](/home/paulca/amadan.net/carlos/platform-moves-design/docs/superpowers/plans/2026-10-06-moves-phase1-1-copy-and-staging.md:1334) calls `s3.PresignClient.PresignCopyObject`. Main pins S3 **v1.105.0**, whose locally installed source has no such method. Task 6 needs a concrete supported signing implementation, including copy-source conditional headers, response/error decoding, and the captured signing time. This is not an implementation-ready interface. |
| + |
| +4. **P1 — Task 5 breaks existing non-versioned mirroring.** |
| + [Plan 1:1298](/home/paulca/amadan.net/carlos/platform-moves-design/docs/superpowers/plans/2026-10-06-moves-phase1-1-copy-and-staging.md:1298) replaces equal-ETag skipping with a requirement for a receipt containing `DstVersionID`. A plain `MirrorStore` cannot supply that field, so unchanged objects are recopied every round. Main’s `TestMirrorPrefix` already requires the second identical round to copy nothing. Apply the stronger receipt requirement only when version evidence is required; preserve the existing ordinary-mirror path. |
| + |
| +5. **P1 — The source copy is not bound to the claim’s location and generation.** |
| + [Plan 1:3025](/home/paulca/amadan.net/carlos/platform-moves-design/docs/superpowers/plans/2026-10-06-moves-phase1-1-copy-and-staging.md:3025) changes staging destinations but leaves “own replica prefix” unspecified. Main constructs that prefix with `activator.HibernatePrefix(cfg.OwnPrefix, host)` at `movespass.go:366`. That scans the host namespace, not the claim-selected generation. With incarnation directories or `g-<op>/` generations, it copies unrelated generations and retains their directory nesting under `replica/`. Define selection from `holder.location` and `holder.generation`, including legacy-root handling, using the already-merged claim/genselect primitives. Test multiple incarnations and generations. |
| + |
| +6. **P1 — Some staging writes bypass the promised signing guard.** |
| + [Plan 1:3025](/home/paulca/amadan.net/carlos/platform-moves-design/docs/superpowers/plans/2026-10-06-moves-phase1-1-copy-and-staging.md:3025) sends `move.json` through ordinary `VersionedStore.PutIfAbsent`; only the mirror’s copy/put/delete methods explicitly use `Guarded.Before`. Main’s versioned implementation calls the normal SDK directly. Task 11’s measurement writes under the staging `probe/` prefix also lack this contract. Additionally, the round validates the claim **before** acquiring the staging lease, whereas §7.3 requires revalidation afterward. Specify one guarded mutation path for every staging writer and test each request class across expiry/retry. |
| + |
| +7. **P1 — Plan 1’s worker transitions still lack operation-identity guards.** |
| + [Plan 1:3025](/home/paulca/amadan.net/carlos/platform-moves-design/docs/superpowers/plans/2026-10-06-moves-phase1-1-copy-and-staging.md:3025) calls `MovePromoteReady` and `MoveMarkCutting`; Task 2 only adds legacy/state checks. Neither compares the expected move ID and incarnation. `SetRecordMove` rereads and retries, so a delayed round for A can promote or cut a replacement move B in the same state. `MoveGuard` arrives only in Plan 2. Bring the guards forward for every Plan 1 worker transition, including abort completion, and test replacement between observation and CAS. |
| + |
| +8. **P2 — Several proposed tests remain red after correct implementation.** |
| + These are fixture/expectation failures, not useful regression failures: |
| + - [Plan 1:275](/home/paulca/amadan.net/carlos/platform-moves-design/docs/superpowers/plans/2026-10-06-moves-phase1-1-copy-and-staging.md:275): `UpdateRecord(..., "")` is explicitly rejected by main; the legacy fixture never reaches its assertions. |
| + - [Plan 1:1209](/home/paulca/amadan.net/carlos/platform-moves-design/docs/superpowers/plans/2026-10-06-moves-phase1-1-copy-and-staging.md:1209): `fakeMirror.put` creates zero-size objects, so the expected successful recopy still has `BytesCopied == 0`. |
| + - [Plan 1:1382](/home/paulca/amadan.net/carlos/platform-moves-design/docs/superpowers/plans/2026-10-06-moves-phase1-1-copy-and-staging.md:1382): `lease.TestEndpoint` does not exist; the fixture never calls `AddBucket`; `leasetest.NewS3Server` supports create-only PUTs and version-specific deletes, not the full lease/update behavior these tests need. |
| + - [Plan 1:890](/home/paulca/amadan.net/carlos/platform-moves-design/docs/superpowers/plans/2026-10-06-moves-phase1-1-copy-and-staging.md:890): the existing API test needs more than operator/claim setup. It clears the record without releasing the claim, then expects another request to succeed; it also expects abort from newly requested `cutting` to fail, contrary to Task 2. |
| + |
| +9. **P2 — Launch-environment composition does not match main’s spawn contract.** |
| + [Plan 1:2528](/home/paulca/amadan.net/carlos/platform-moves-design/docs/superpowers/plans/2026-10-06-moves-phase1-1-copy-and-staging.md:2528) proposes `mergeEnv(fileEnv, agentOwnedEnv(rt))`. Main already has `agentOwnedVars(rt, fileEnv, haveControl)`, which **removes** tenant-provided agent-owned names when unavailable and depends on the actual control-listener outcome. Merging an agent-only map cannot express those removals. The proposed launch test would pass even if the entire agent-owned layer were omitted. Reuse the existing composition contract and test removals, region, cache presence, and control availability. Separately, `CompareEnv` at line 2485 accepts a missing target `CARLOS_REGION`, and never checks layout names absent from both reports. |
| + |
| +10. **P2 — T5 does not verify the archive requirements it claims to cover.** |
| + [Plan 1:2636](/home/paulca/amadan.net/carlos/platform-moves-design/docs/superpowers/plans/2026-10-06-moves-phase1-1-copy-and-staging.md:2636) specifies `archive.Open` to `io.Discard` and “compare plain.” `Open` returns only an error and emits compressed tar bytes; this establishes decryption, not the plaintext tar digest, manifest entries, allowed entry types, or uncompressed cap. Add a streaming verifier and negative tests for correctly encrypted archives with wrong manifests, special entries, and excessive content. |
| + |
| +11. **P2 — Archive extraction does not preserve modes.** |
| + [Plan 1:1954](/home/paulca/amadan.net/carlos/platform-moves-design/docs/superpowers/plans/2026-10-06-moves-phase1-1-copy-and-staging.md:1954) permanently ORs directory modes with `0700`; file creation is also subject to umask. A valid `0555` directory becomes `0755`, changing the reconstructed plaintext digest and violating §8.1. Use temporary writable directory modes, then restore exact modes after extraction; explicitly restore file modes too. The existing round-trip fixture does not exercise this. |
| + |
| +12. **P2 — Task 12 has a forward dependency and reintroduces the rejected completeness check.** |
| + [Plan 1:2825](/home/paulca/amadan.net/carlos/platform-moves-design/docs/superpowers/plans/2026-10-06-moves-phase1-1-copy-and-staging.md:2825) requires registry `hold_op` inspection and held-row tests before Task 13 creates the field and schema. Move that prerequisite earlier. |
| + [Plan 1:2851](/home/paulca/amadan.net/carlos/platform-moves-design/docs/superpowers/plans/2026-10-06-moves-phase1-1-copy-and-staging.md:2851) again requires `readEdgesSnapshot(...).complete()`. That reader never sets `BoxesComplete`; this contradicts Task 4 and the R2-1 disposition and would refuse every request. |
| + |
| +13. **P2 — Target selection still omits approved rev 7.19 behavior.** |
| + [Plan 1:1072](/home/paulca/amadan.net/carlos/platform-moves-design/docs/superpowers/plans/2026-10-06-moves-phase1-1-copy-and-staging.md:1072) defers eligible-box selection and uses lexical order. Main now contains same-package `eligibleBoxes` and `chooseBox` in `internal/console/boxchoice.go`; neither needs exporting for this caller. Main’s helper still needs reconciliation with the approved SHA-256 tie-break, but the plan must specify that work now. Also bind target reporting to `op.intent.target`; “every box whose placement equals `move.to`” is insufficient for the approved named-target contract. |
| + |
| +14. **P2 — Task 14 contains mutually incompatible acceptance criteria.** |
| + [Plan 1:3028](/home/paulca/amadan.net/carlos/platform-moves-design/docs/superpowers/plans/2026-10-06-moves-phase1-1-copy-and-staging.md:3028) requires an existing `move.json` with no ledger to stop copying, while the implementation and lost-ledger test require accepting `ErrExists` without reading the target object. These source-visible states are indistinguishable. Keep the corrected target-validation rule and replace the obsolete source-refusal test. The self-review at line 3067 also still claims source-side staging deletion, contradicting Task 14’s target-owned cleanup disposition. |
| + |
| +The task-by-task result is: |
| + |
| +| Task | Build and test assessment | |
| +|---|---| |
| +| 1 — IDs | New package/functions; tests have meaningful positive and negative assertions. No blocker found. | |
| +| 2 — Journal | Existing production interfaces exist; deleted consumers, invalid fixture, switch ordering and missing identity guards block it. | |
| +| 3 — Claim transitions | Consumed claim APIs exist on main. Duplicate helper blocks tests; `ReleaseMove` needs its stated implementation/test. | |
| +| 4 — Door | Most APIs exist; console switch producer is missing and existing tests need transition-aware migration. | |
| +| 5 — Copy versions | Existing mirror types match; plain-store regression and zero-size fixture prevent green tests. | |
| +| 6 — Lease/signing | `acquireTTL` exists and can be exported. Copy presigner and test endpoint do not; fixtures are incompatible. | |
| +| 7 — Archive | Crypto signatures match main. New tests cover basic round-trip/tampering, but miss mode preservation. | |
| +| 8 — Timers | `registry.Timer` fields match exactly. No import cycle or substantive blocker found. | |
| +| 9 — Environment | Existing spawn composition differs from the proposed extraction; tests do not protect that contract. | |
| +| 10 — Budget | Referenced model exists at `9c417aaa`; arithmetic and expected values align. No blocker found. | |
| +| 11 — Readiness | Pure dependencies exist by this task. Host work is explicitly deferred to Task 14; T5 verification is incomplete. | |
| +| 12 — Floor | Main applier/edges interfaces exist. Depends on Task 13 and contradicts the corrected completeness rule. | |
| +| 13 — Holds | Registry interfaces and fixture exist. No claim dependency; add a negative test for `DeleteIfHold`, which currently has none. | |
| +| 14 — Integration | Existing harness exists; source selection, guarded writes, identity CAS and contradictory tests block completion. | |
| +| 15 — Copy gate | Requires the missing `DefaultEnabled` producer and explicit inclusion of the copy record when building from main. | |
| +| 16 — Gate | Cannot pass with the preceding failures. No gate was executed during this review. | |
| + |
| +**Claim dependency verdict:** I found no direct Plan 1 call into unmerged `hostops`, `hostbox`, `hostwrite`, or lease-controller packages. The identified build fixes can be made against merged primitives. However, the plan explicitly substitutes manifest existence for **T7 materialization** and cgroup delegation for **S2 strict-next-spawn**. Those are scaffolding, not completed readiness requirements. The direct-socket S1 probe also needs an explicit wake path for an already-hibernating source. |
| + |
| +No new package import cycle was found in Plan 1’s stated dependency graph. The failure is concrete build/test mismatches, incomplete integration contracts, and ineffective intermediate gating. |
| + |
| +--- |
| + |
| +**FIRST PROMPT:** |
| +Focused read-only review of moves Phase 1 Plan 1 and its index before implementation. |
| + |
| +**WHAT:** |
| +Check buildability on main, claim independence, switch-off safety, tests and spec coverage. |
| + |
| +**STATUS:** |
| +Review complete. PLAN 1: NOT READY TO BUILD. Remote freshness unverified. |
| + |
| +**NEEDED:** |
| +Nothing for this review. |
| + |
| +**RECOMMENDED:** |
| +Fix the build blockers and establish the switch boundary first; then revise staging and readiness contracts before re-review. |
| + |
| +**SESSION:** |
| +Safe to close. Repository unchanged; nothing running; no handoff file written. |
| \ No newline at end of file |