amadan
zdd-4: Astra's final review (REJECT) and scoped re-review (REJECT) of the implementation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 files changed,
+69
−0
docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-4-R2.md+23 −0docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-4.md+46 −0
CI passed — run details
diff --git a/docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-4-R2.md b/docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-4-R2.md| new file mode 100644 |
| index 000000000..9cdeab6aa |
| --- /dev/null |
| +++ b/docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-4-R2.md |
| @@ -0,0 +1,23 @@ |
| +REJECT |
| + |
| +Reviewed `acc6de7a..7b239870` only. |
| + |
| +1. **F1 — NOT ADDRESSED (R12).** Clearing `workerServing` at [activator.go:1972](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-4-release-side/internal/activator/activator.go:1972) ends the retry loop but does not protect captured builds. Worker A can still lose its staged copy after promotions to B and C remove its live-link and registry pins. An absent generations file still supplies no pins at [pins.go:194](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-4-release-side/internal/release/pins.go:194). Sweep/restage can therefore destroy A’s recovery artifact. R12 does not satisfy §4.2’s requirement that referenced copies remain unchanged; it also introduces defect N1 below. |
| + |
| +2. **F2 — ADDRESSED, with a new pacing defect (R13).** Same-build respawns preserve `staleSince` at [activator.go:1993](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-4-release-side/internal/activator/activator.go:1993), and crash/max-run recycling evaluates drift at [workerbuild.go:64](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-4-release-side/internal/activator/workerbuild.go:64). Those changes remove the reported starvation. R13’s unconditional convergence is nevertheless incompatible with §7.9’s admission requirement; see N2. |
| + |
| +3. **F3 — ADDRESSED for the reported chained-link case (R14).** [pins.go:60](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-4-release-side/internal/release/pins.go:60) pins every returned hop, and [staged.go:101](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-4-release-side/internal/release/staged.go:101) captures the terminal path. The added restage-refusal test covers the original `live → operator-current → staged executable` case. However, R14’s exclusion of parent-directory resolution introduces N3. |
| + |
| +4. **R11 note — ADDRESSED (R15).** [adopt.go:1739](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-4-release-side/internal/release/adopt.go:1739) no longer promises that the next full pass will restage a held version. Recovery is conditional on a pass actually restaging that version, or shipping a new name. The conservative refusal remains consistent with §4.2. |
| + |
| +**New defects introduced by the fix diff:** |
| + |
| +- **N1 — P1: A digest refusal becomes acceptance on the next retry.** [activator.go:1972](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-4-release-side/internal/activator/activator.go:1972). Suppose live still names A, but A’s executable has changed from its retained digest. The first recycle correctly refuses it, then discards the retained identity. The next retry passes `onto=nil`; `StartWorkerStaged` calls `ResolveLive`, hashes the damaged bytes as a fresh identity, and executes them. No promotion is required. §4.2’s refusal therefore lasts only one retry. **Smallest fix:** retain the captured identity after refusal; permit a different build only through explicit convergence/adoption admission. Address F1 through complete pins or conservative suppression of destructive staging and eviction. |
| + |
| +- **N2 — P2: Recycles bypass convergence pacing and a negative coalescing decision.** [workerbuild.go:64](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-4-release-side/internal/activator/workerbuild.go:64). Multiple workers with overdue drift and synchronized `max_run` deadlines all execute the installed build in one sweep. None consumes `cycled`, so the existing one-per-sweep limit is ineffective for this path. Moreover, when `CoalesceRecycle` returns false because another canary is running, this branch still changes builds after the grace period—contrary to §7.9. Replacing one spawn with one spawn does not preserve rollout pacing. **Smallest fix:** share the sweep’s convergence budget with these paths; when scheduler admission is present, require it before changing builds. Otherwise recycle the captured build while preserving drift age. |
| + |
| +- **N3 — P2: Relative chained links can resolve to the wrong executable.** [staged.go:64](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-4-release-side/internal/release/staged.go:64). Consider `live/h → /opt/carlos/alias/current`, where `alias → /opt/carlos/versions/v1/links` and `current → ../app`. Filesystem resolution reaches `/opt/carlos/versions/v1/app`. The new lexical join/clean instead produces `/opt/carlos/app`. Previously, hashing and executing the intermediate path followed the filesystem correctly; now startup fails or executes the wrong file, and the real staged copy remains unpinned. **Smallest fix:** resolve each symlink’s containing directory before interpreting relative targets, retain intermediate pins, and fail closed when resolution is uncertain. |
| + |
| +Out of scope: unchanged deferred findings and later-plan scheduler/generation integration were not re-reviewed. |
| + |
| +Static review only; no files modified or tests run. `git diff --check acc6de7a..HEAD` passed. |
| \ No newline at end of file |
diff --git a/docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-4.md b/docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-4.md| new file mode 100644 |
| index 000000000..ed732a7cb |
| --- /dev/null |
| +++ b/docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-4.md |
| @@ -0,0 +1,46 @@ |
| +REJECT |
| + |
| +1. **P1 — Captured worker builds are unprotected before plan 5.** [internal/release/adopt.go:405](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-4-release-side/internal/release/adopt.go:405), [internal/activator/workerbuild.go:52](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-4-release-side/internal/activator/workerbuild.go:52). |
| + |
| + A worker runs A, whose staged files are over six hours old. Two promotions install B then C before the worker converges. Its live link and registry now pin C and B; nothing exported by this branch pins A. The next hourly sweep can delete A. Alternatively, adoption of a re-shipped A can overwrite it because `BoxPins` reports it unreferenced. |
| + |
| + A subsequent crash or `max_run` recycle refuses A through `VerifyStaged`. The exited-worker branch keeps retrying that identity and never reaches convergence, leaving the worker down indefinitely. This happens without any new flag. |
| + |
| + **Smallest fix:** treat worker references as unknown until they are exported, suppressing both destructive restaging and eviction in that state. Alternatively, bring the worker-pin writer and both consumers into this branch. Add a regression covering two repoints followed by sweep/restage and recycle. |
| + |
| +2. **P2 — Same-build recycles can prevent convergence indefinitely.** [internal/activator/activator.go:1974](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-4-release-side/internal/activator/activator.go:1974), [internal/activator/activator.go:2548](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-4-release-side/internal/activator/activator.go:2548). |
| + |
| + An always-on worker runs A with `max_run=30s`; root installs B. Every recycle correctly respawns A, but also clears `staleSince`. The drift never survives the 90-second `ConvergeGrace`, so B never runs. Frequent crashes produce the same starvation; exited workers skip the drift check entirely. Before this change, their respawn followed the updated live link. |
| + |
| + **Smallest fix:** preserve drift age across recycles of the same build, evaluate drift even when a recycle is due, and give an eligible convergence priority while retaining the convergence budget. Test several recycle intervals after a promotion, not only the first recycle. |
| + |
| +3. **P2 — Chained live symlinks hide referenced copies from `BoxPins`.** [internal/release/pins.go:59](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-4-release-side/internal/release/pins.go:59), [internal/release/staged.go:53](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-4-release-side/internal/release/staged.go:53). |
| + |
| + With `live/h → /opt/operator-current → versions/v1/app`, and no registry version pin for v1, `BoxPins` records only `/opt/operator-current`. Another host adopting a re-shipped v1 therefore overwrites a copy the live link still references. `ResolveLive` likewise retains the intermediate link, rather than the final executable path. A later digest refusal detects the damage but does not prevent the overwrite. |
| + |
| + **Smallest fix:** resolve the captured target’s remaining symlinks for executable identity and pinning; preserve necessary intermediate-path pins and fail closed when resolution is uncertain. Add a chained-link restage-refusal test. The ledger’s Task 3 symlink minor needs promotion to a pre-merge fix. |
| + |
| +### Ledger rulings and deferred items |
| + |
| +- **R1, R7, R9:** workflow choices; no implementation objection. |
| +- **R2–R5:** consistent with the binding sections and current code. Preserving cgroup cleanup and the config-admission gate is necessary. |
| +- **R6:** canonical comparison and outage fallback are consistent with the stated contract. This does not establish that the reference set is complete. |
| +- **R8:** reasonable against this base; the later staging integration still needs review. |
| +- **R10:** acceptable. The fixtures still reach the binding refusal and retain their assertions. |
| +- **R11:** the conservative refusal is correct. Its claimed recovery is not guaranteed: a held row is not necessarily adopted by a full pass, and that pass may never stage its requested version. Correct that expectation; do not restore the incomplete-pin overwrite. |
| + |
| +Deferred-minor triage: |
| + |
| +| Ledger area | Disposition | |
| +|---|---| |
| +| Task 1 | Test-comment wording and missing uppercase rejection test: P3. | |
| +| Task 2 | Lossy-sibling comment is already corrected. Older-sibling reuse test: P3. | |
| +| Task 3 | Chained symlink: finding 3. Relative-link assertion, wording and error wrapping: P3. | |
| +| Task 4 | Empty SHA is already rejected by `validateManifest`, so `CanonicalDemand` returns hold. EqualFold/digest assertion gaps: P3. | |
| +| Task 5 | Parent-directory fsync, reader consolidation, missing branch tests, parse-error wrapping and empty-host-list strictness: P3. | |
| +| Task 6 | Missing-platform diagnostics/test, repeated hashing/logging and held-stage explanation: P3. | |
| +| Task 7 | Ordering comment, generic failure reason and coverage gaps: P3. Host-claim selection and unit-less proof-policy behavior are not new defects. | |
| +| Task 8 | Regular-file and unreadable-target refusals fail closed. Missing failure-path test: P3. | |
| +| Task 9 | Captured-copy loss: finding 1, **must fix**. Coalescing backoff/logging and remaining coverage gaps: P3; add refused-recycle coverage with finding 1. | |
| + |
| +Static review only; no files modified and no tests run. `git diff --check 4f26f15c..HEAD` passed. |
| \ No newline at end of file |