Conversation
mostlydev
left a comment
There was a problem hiding this comment.
Review of PR #131 (docs: ADR-024 + plan for issue #128)
TL;DR
Docs-only PR (744 / 0). The committed text is internally coherent and the trust argument is the strongest part. However, the working tree on issue-128-runner-refresh-command has already substantially rewritten both the ADR and the plan in the opposite direction (and added ~1300 lines of implementation), so the committed PR is no longer the design that's actually being built. That divergence — not any single line in the doc — is the biggest thing to resolve before this leaves draft.
Design divergence between PR text and worktree
| PR (committed) | Worktree (uncommitted) | |
|---|---|---|
| Refresh verb | claw pull (extended with mode matrix) |
New claw runners update subtree |
| ADR-022 amendment target | §3 (pull skips build:) | §2 (no new top-level verbs) |
| Single-Clawfile entry | positional path, reusing claw build <path> resolution |
explicit --clawfile <path> flag |
claw build first-use auto-build of missing runner base |
implicit (still runs once) | hard-fails closed, requires runners update first |
| Argument the other approach is rejected for | "breaks ADR-022 §2's no-new-verbs promise" | "overloading claw pull makes mixed-pod workflows harder" |
Each of those flips is individually defensible; but the committed ADR-024 explicitly rejects the design that the worktree now implements (and vice-versa). Picking one and letting the PR reflect it is a prerequisite to merge. My read: the worktree direction (claw runners update) is the better one — it preserves claw pull semantics for operators who only want pinned-infra freshness, gives explicit per-driver targeting (-f pod openclaw is genuinely useful for mixed pods), and the "no new verbs" promise in ADR-022 §2 was always going to bend the moment a maintenance surface was needed. But that means the PR body, ADR title, the §3-narrow-amendment framing, and large parts of the alternatives section all need to be rewritten before the PR matches reality.
Substantive notes on the committed text (in case the §3-amendment direction wins after all)
- ADR title vs. title-slug drift. Filename is
024-runner-base-refresh-from-upstream.md; worktree retitles it toRunner Base Refresh Command and Provenance. If you switch to the verb design, rename the file too — slug-rot makes ADR cross-refs harder later. - The "without exception" line in ADR-022 §3 is real. ADR-024 narrows it ("the anti-collision rationale is preserved intact because there's no registry collision surface for locally built bases"). That logic holds — but you're now asking ADR-022 to mean "without exception for registry-collision reasons" rather than the literal text. Worth amending ADR-022 inline rather than just claiming amendment in ADR-024.
- Probe table row
nanoclaw. Driver isnanoclawbut alias isnanoclaw-orchestrator(verified atinternal/driver/nanoclaw/baseimage.go:3,42). The table uses driver name, which is fine, but document the mapping once so probe failures don't get diagnosed against the wrong tag. - Picoclaw fallback tag. Plan §2 correctly opts picoclaw out of the prober. But picoclaw's recipe pulls
docker.io/sipeed/picoclaw:latestand adds nothing;--no-cachewon't change anything unless upstream re-tagged latest. Worth a sentence acknowledging that picoclaw refresh is essentially "re-pull the upstream tag" rather than "rebuild from source." - Same-day fallback collision regression test. Plan §10 calls this out specifically as the codex finding being defended. Good — that test is the single most important new test in the plan; don't let it slip.
- Drift hint epistemic boundary. ADR §4 and plan §9 both nail this:
claw uponly compares local image IDs and never claims upstream knowledge. This is the right call; preserve it through implementation. - Network failure during refresh (plan open question 5) —
curl install.sh | bashfailures will surface as rawdocker builderrors. For a "this took a few minutes and then died" UX, even a 1-line wrapping error ("upstream installer failed; check network connectivity to openclaw.ai") is worth the cost. Not a blocker.
Plan-vs-implementation drift (worktree)
Quick spot-check against internal/build/build.go and the new cmd/claw/runners.go:
RefreshResult(build.go:65–73) carries the seven fields plan §4 promises (Alias,Version/VersionTag,ImageID,RecipeSHA, etc.) — but noPreviousVer. The "was vX.Y.Z" message in the plan has nothing to read from yet; either capture-before-rebuild or accept that the upgrade message is best-effort and document it.runners.goaccepts driver-name positional args (openclaw,nanoclaw) but nothing validates that names matchingRunnerBaseProviderregistration align with the strings users see inCLAW_TYPE. Worth a unit test asserting the registry name → alias mapping for all six.ResolveLocalRunnerProvenancereturnsRunnerRefreshRequiredErrorwhen<alias>:latestexists but lacks a versioned sibling tag. That's the "I built this manually withdocker build" path — make sure the contributor escape hatch in ADR-024 §6 still works, or document that contributor manual builds need to also tag a:v*sibling.
Process
- PR body uses
Refs #128, notCloses #128. PerCLAUDE.mdthe project board automation requires a closing keyword to move the card. If this PR is meant to close #128 on merge, switch toCloses #128. If it's docs-only and #128 stays open until the implementation lands, "Refs" is fine — but then a separate implementation PR will need the closing keyword. - Project board. Issue #128 is currently "In Progress." Worth a comment on the issue noting that a docs-first PR is open and the implementation is in flight in the worktree.
- The "competing ADR/plan drafts were intentionally not carried forward" line in the PR body is now misleading: the worktree is the competing draft, just unstaged. Either drop the line or rewrite it with current state.
Recommendation
Don't merge as-is. Two reasonable paths forward:
- Land docs first, but rebase/rewrite the committed ADR + plan to match the
claw runners updatedirection the worktree is actually implementing. Keep the trust argument (it's the strongest part). Then a follow-up PR carries the implementation. - Land docs + implementation together — turn this PR into a regular feature PR by committing the worktree changes here, and rewrite the body. Given the implementation is ~1300 LOC across 9+ files and tests, this is a real review session, not a quick-merge.
Either way, the committed PR text and the in-flight code need to converge before this should leave draft.
Summary
claw pullclaw build <path>Notes
Refs #128