amadan
Instance key provision: Astra round 1, unedited
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit fd01cdd03a66e4c1f422b5d64508464804d23ffa)
1 file changed,
+56
−0
docs/superpowers/plans/CODEX-PLAN-INSTANCE-KEY-R1.md+56 −0
diff --git a/docs/superpowers/plans/CODEX-PLAN-INSTANCE-KEY-R1.md b/docs/superpowers/plans/CODEX-PLAN-INSTANCE-KEY-R1.md| new file mode 100644 |
| index 000000000..905a4459e |
| --- /dev/null |
| +++ b/docs/superpowers/plans/CODEX-PLAN-INSTANCE-KEY-R1.md |
| @@ -0,0 +1,56 @@ |
| +Reviewed HEAD `3b99911e087cbe6f417aa27a3dc9d6d6e32a5979`. Five correctness findings and one rollout correction: |
| + |
| +1. **P1 — O2 accepts a key the app may never have used.** [Plan:139](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-key-provision/docs/superpowers/plans/2026-10-10-instance-key-provision.md:139) |
| + The platform mints its key even when the app uses its own `instance.key`, which takes precedence. If that app-owned key never reaches the backup, the proposed `instance.key OR CARLOS_INSTANCE_KEY` check still accepts the unrelated platform key. Relocation can then restore sealed data without its actual key, bypassing the existing refusal. Require evidence of which key source protects the database; mere presence of a platform key is insufficient. Test a backed-up platform key alongside a missing app-owned key. |
| + |
| +2. **P1 — A failed durability step becomes success on retry.** [Plan:585](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-key-provision/docs/superpowers/plans/2026-10-10-instance-key-provision.md:585) |
| + After the append succeeds, `Chmod`, file `Sync`, or directory `Sync` can fail. The next ensure sees the nonempty key and returns immediately, allowing a launch without completing the failed durability step. The claim early return at line 1005 bypasses it too. Preserve the bytes, but complete durability before reporting success. Add injected file-sync and directory-sync failures followed by retries, asserting that no child starts before successful synchronization. |
| + |
| +3. **P2 — Every nested backfill that needs to append fails with `EINVAL`.** [Plan:605](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-key-provision/docs/superpowers/plans/2026-10-10-instance-key-provision.md:605) |
| + Backfill uses `create=false`, so the flags exclude `O_CREATE`, but `OpenBeneath` still receives mode `0600`. That helper passes the mode directly to `openat2`, which rejects a nonzero mode without creation flags. I confirmed this kernel behavior with a read-only syscall check. Pass mode zero for an existing-file open, retaining the later `fchmod`. Add a successful nested backfill test; the current successful backfill test exercises only flat layout. |
| + |
| +4. **P2 — An empty instance override defeats the configured-key exception.** [Plan:591](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-key-provision/docs/superpowers/plans/2026-10-10-instance-key-provision.md:591) |
| + With `CARLOS_INSTANCE_KEY=` in `<host>.env` and a nonempty key in `<host>.config.env`, the mint returns `InstanceKeyFromConfig`. However, `mergeEnv` preserves the empty instance override, so the child receives an empty key and an adopting app refuses to start. Resolve this using the effective merged environment and define how the empty override is handled. Test both files together through launch; the two separate empty-value tests miss this combination. |
| + |
| +5. **P2 — O2 rejects the configuration-backed keys that ruling 5 explicitly supports.** [Plan:139](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-key-provision/docs/superpowers/plans/2026-10-10-instance-key-provision.md:139) |
| + An app using `CARLOS_INSTANCE_KEY` from configuration has neither `instance.key` nor a key in the backed-up `<host>.env`, because this plan deliberately mints nothing. With a nonempty database, O2 therefore refuses relocation despite the key being durable in configuration. Define verifiable recovery evidence for this supported source, including delivery of the same effective key on the target, and add a relocation test. |
| + |
| +6. **P3 — The rollout uses an unsupported merge option.** [Plan:1421](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-key-provision/docs/superpowers/plans/2026-10-10-instance-key-provision.md:1421) |
| + `amadan branch merge -squash` contradicts the repository’s explicit workflow: squash locally, push, describe the branch, check CI on the final tip, then merge with `-expect <tip>`. |
| + |
| +The remaining review points: |
| + |
| +- **Concurrency:** sharing `adminTokenMu` serializes the proposed token/key appends within the agent. I found no new lock recursion in the listed callers. |
| +- **Durability:** legacy durability remains disk plus move archives; this does not provide off-box recovery after box loss. O1 is necessary for claim launches, but the capturing fake proves only ordering—not committed publication or launch refusal on publication failure. Those need provider integration tests. |
| +- **Tests:** the snippets appear compatible with the inspected interfaces, but I did not independently compile or run them in this read-only workspace. Coverage also lacks a first flat seed, nested `RoleSeed`, actual hibernation restore, and the failure/retry cases above. A second `Start` does not exercise restore. |
| +- **Secrecy:** I found no proposed production log/error that prints the key, or new console/CLI exposure. Move reports retain HMAC-only values. |
| + |
| +For the five open questions: |
| + |
| +- **Replicas:** may remain deferred only with the stated exclusion from platform-key adoption. |
| +- **Configuration:** blocks the current design until findings 4 and 5 are resolved; reserving the name is not required. |
| +- **`report.token`:** does not block this key-only scope. |
| +- **Workers/sidecars:** does not block the explicit exec-instance scope. |
| +- **Claim-session restarts:** the alleged bypass is not established at this HEAD. Claim retries use captured launch inputs and call `Materialize` before each attempt; production claim swaps refuse. Correct the premise and pin that behavior with a test. Any future path that mints before an asynchronous publish would require a synchronous publication gate. |
| + |
| +--- |
| + |
| +**FIRST PROMPT:** |
| +Review the instance-key plan at HEAD using the supplied brief. |
| + |
| +**WHAT:** |
| +Check whether the plan is ready to implement. |
| + |
| +**STATUS:** |
| +Review complete; six findings. No files changed. |
| + |
| +**NEEDED:** |
| +Nothing for this review. |
| + |
| +**RECOMMENDED:** |
| +Resolve key-source evidence, durability retries, and the two backfill/configuration defects before implementation. |
| + |
| +**SESSION:** |
| +Safe to close; no processes or scratch files were created. |
| + |
| +**Verdict: not ready to build.** |
| \ No newline at end of file |