kit-48-box-pins-command: devteam.mjs box-pins: verify, check and bump the box image's pins, and check what the image installed

docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/plan-1.md

@@ -0,0 +1,34 @@
+# Plan review, pass 1
+
+- Reviewer: plan-reviewer (fresh subagent, claude-opus-5-5)
+- Reviewed: c989c4adbb8233b13ce61bf1227c511c16fc68ce (plan.md, against spec.md as approved in spec-4)
+
+I read the plan, the spec, `ticket show kit-48-box-pins-command` (the top-level done_when set), quality.md §3 and §6, and the code the plan touches at c989c4a: the re-exec block and `parseArgs` (1-110), `homeAt`/`machineWide`/`KIT_VIEWS`/`kitsOwnRepoAt` and the `DEVTEAM_RUN` check (~420-490), `replyNow` (128), `cmdHub` (~13867), HELP/COMMANDS/dispatch (~19077-19230), test/machine.test.mjs 1-125, test/lib/kit.mjs's head, the kit-own fixture in test/multirepo-home.test.mjs (81-91), cloud/Dockerfile 60-136 and 515-525, and test.yml.
+
+What holds:
+- The fixture works. A temp git repo with origin `sanctuarycomputer/devteam` and a copy of `docs/devteam` (no `project/`, `run/`) gets through the re-exec block (its own `docs/devteam/scripts`, no `project/`) and `homeAt` (no settings.md, but `KIT_VIEWS` plus `kitsOwnRepoAt`), as multirepo-home.test.mjs's kit-own fixture already shows for `status` and friends. `positional` is parsed before `homeAt`, so the `machineWide` test can read `positional[0]`. `os.tmpdir()` here (`/data/home/tmp`) and on CI is in no repo, so T4's kit copy is free of the re-exec check.
+- I ran every Dockerfile edit in T1's tests against the real cloud/Dockerfile with a node script. Each replacement matches. The simulated stray scan (the plan's skip set and bounds) gives no hits on the real file, one hit (line 98, gh) for the URL case, none for TTYD=1.0.0, and claude-swap on line 76 for the comment case. No line of the real file matches `^\s*#\s*hold\b`. Exactly one line matches the `DEVTEAM_BOX_TOOLS` pattern.
+- Node 18: global `fetch` (already used by the kit at 824, 13584, 16378), `AbortSignal.timeout`, `arrayBuffer()` and redirect-follow are all present. Same-origin redirects keep `Authorization` in Node's fetch, so a token wrongly sent to `/dl` would still reach `/assets` and be seen.
+- T4's design catches a PATH lookup (wrong shims first, plus the file's own `bin` stubs) and a missing `UV_TOOL_DIR` (the right uv lists claude-swap only with it). T3's tests catch a partial write (gh fails after claude-code and claude-swap were bumped), a digest-only implementation (the zero-digest case would be written, exit 0), a digest-ignoring one, and a non-following download (the 302 body would be hashed).
+- Every file is inside touches. Interfaces mostly line up across tasks (`readPins`'s line indexes feed `bumpPins`; `latestOf` returns the release T3 needs).
+
+Two things block: the tests hard-code the Dockerfile's current pins, so the first real `box-pins bump` breaks CI on the release-prep PR (F1); and the prerelease case can't fail for a missing prerelease filter (F2). F3 is the TL;DR rule.
+
+Values and refusals: checked at the spec stage (spec-1 to spec-4). The plan adds no behaviour beyond the approved spec, so none is reopened. P5 (adversarial for authority): the token and download paths were tried again here (F5).
+
+| Id | Severity | Rule | Where | Finding | Disposition |
+|---|---|---|---|---|---|
+| F1 | Important | done-when 1 ("test/machine.test.mjs passes in CI"); spec Contracts (consumer: the playbook's release-prep bump) | T1 tests (URL case `2.104.0`; the "another tool's own ARG" case, which needs claude-swap at `1.0.0`); T2 (`ttyd: stale 1.7.7 -> 1.7.8`); T3 (`TTYD_VERSION 1.7.7 -> 1.7.8`, "ttyd's fixture latest = 1.7.7") | `pinsCopy` copies the real cloud/Dockerfile, but the assertions hard-code its current pins. The first real `box-pins bump`, run by the release-prep PR this ticket exists for, moves those pins and fails CI on that PR. Take gh to 2.105.0: the URL case inserts `2.104.0`, which is no longer the pin, so verify exits 0 and the test fails. Take claude-swap to 1.0.1: the comment case no longer names it, and the TTYD=1.0.0 case passes without testing anything. Either read the pins from the copied file in the test and build every expected string from them, or have `pinsCopy` first set every pin `ARG` to fixed fixture values (with the real file kept only for the "ok on the kit's own Dockerfile" case). | fixed: pinsCopy rewrites the copy's pins to FIXED values first; every expected string comes from FIXED |
+| F2 | Important | done-when 3 ("ignores GitHub prereleases ... (each in the test)"); spec check (GitHub: "neither `draft` nor `prerelease`") | T2 test `check ignores a GitHub prerelease` (ttyd list `[1.8.0-rc prerelease:true, 1.6.9, 1.7.8]`); the fixture default ("a newer prerelease on tsl0922/ttyd", tag unspecified) | `1.8.0-rc` fails `versionParts` anyway, so an implementation that never reads `prerelease` still skips it, and the test passes. The done-when's property goes untested. Give the prerelease a plain-version tag (`1.8.0`, `prerelease: true`) so only the flag can skip it, and add a `draft: true` plain-version entry for the draft filter. | fixed: plain tags flagged prerelease: true and draft: true |
+| F3 | Important | quality.md §6 (a TL;DR that says something the plan doesn't do); done-when 7; spec Acceptance 7 ("clears ... explicitly") | TL;DR bullet 6; T5 Step 1 (`machine(['help'])`); T4 (env not named) | The TL;DR says every new test passes an env with `DEVTEAM_IMAGE_HUB`, `DEVTEAM_KIT_RELEASE` and `DEVTEAM_BOX_TOOLS` deleted. T5's help test runs through `machine()`, which passes `baseEnv` with no explicit deletion. T4's tests name only what they set ("DEVTEAM_BOX_BIN=…, DEVTEAM_BOX_TOOLS=…, PATH=shimBin:…"), not `pinsEnv`. Have T4 build its env with `pinsEnv({...})` and T5 run help with `pinsEnv()` too (or a `machine` call given it), and say so in each task. | fixed: TL;DR names pinsEnv; T4 and T5 spawn with pinsEnv |
+| F4 | Minor | plan Interfaces (consistency across tasks) | T2 test `check sends GH_TOKEN to the GitHub API only` | This T2 test ends with "then a bump", which is T3. Until T3 lands, the bump arm says `not yet`, so T2's Step 4 ("Run to pass") can't pass as written. Move the download half to T3, where the bump tests are. | fixed: the download half moved to T3 ('bump sends no token with a download') |
+| F5 | Minor | spec Security ("`GH_TOKEN` is sent only to the GitHub API base, never to a download URL"); spec check ("on the API requests only") | T2 `upstream()` interface ("every request counted in `state.hits` by path"); the GH_TOKEN test | A count by path can't show headers, which this test asserts. Record each request's path and `authorization` header. Assert none under `/npm`, `/pypi`, `/dl` or `/assets` carries one; the plan checks only `/assets`. Consider pointing one `browser_download_url` inside the `/gh` base too: the spec says no auth on a download "whatever its URL", and the fixture as planned only tests a prefix check. | fixed: hits record headers; no auth asserted under /npm, /pypi, /dl, /assets |
+| F6 | Minor | plan Interfaces | T1 produces `pins()` (sync, `.status`); T2 says each test "spawns asynchronously (`execFile` promisified)"; `safe` is produced in T2 but T1's messages already use the filter, and T4 lists `safe` among "T1's" | The async runner T2-T4 need is never named. Promisified `execFile` also rejects on a non-zero exit, which most of these tests expect, so the helper should resolve `{ status, stdout, stderr }` either way. Name it (e.g. `pinsAsync`) in T1 or T2's helpers, and move `safe` to T1, where verify first needs it. | fixed: pinsAsync named (resolves on non-zero exit); safe moved to T1 |
+| F7 | Minor | spec bump step 2 ("hashed (sha256, streamed, 120 s timeout each)") | T3 Step 3 (`sha256 of arrayBuffer()`); Global Constraints | The plan buffers each asset whole, where the spec says streamed, and doesn't call this out as a departure. On Node 18 a body can be streamed without async iteration: `res.body.getReader()` and a `read()` loop into `hash.update`. Either stream, or record the departure (assets are tens of MB, so buffering is tolerable). | fixed: the arrayBuffer departure is stated in Global Constraints |
+| F8 | Minor | done-when 1 (CI passes; a hung test holds the job until its 30-minute timeout, test.yml) | T2-T3 `upstream()` | If an assertion fails before `close()`, the in-process server keeps the test file's process alive. The run then hangs until the job's timeout instead of reporting the failure. Close it in `t.after(() => u.close())`, or `unref()` the server. | fixed: upstream(t) unref()s and closes in t.after |
+| F9 | Suggestion | | T1 Step 2 | The expected failure is not `unknown command "box-pins"`. Before `box-pins` joins `KIT_VIEWS`, `homeAt` dies first in the fixture repo with `no docs/devteam/project/settings.md here …`. | fixed: expected failure is the no-settings.md refusal |
+| F10 | Suggestion | | T4 Step 1 | The file's `write` doesn't set a mode. Say that the fake binaries are `chmod 0o755`, as the file's existing stubs are, or every tool reads `did not run (EACCES)`. | fixed: fake binaries chmod 0o755 |
+| F11 | Suggestion | | `pinsEnv` | Also delete `UV_TOOL_DIR`. A developer shell that has it set to `/opt/uv/tools` would let a run that leaves it out pass locally (CI still catches it). | fixed: pinsEnv deletes UV_TOOL_DIR |
+| F12 | Suggestion | spec bump step 4 (original's mode kept), Edge cases (`unchanged`) | T3 tests | Neither the mode kept across the rename nor the `unchanged` skip is tested. Both are cheap: chmod the copy 0640 before a bump and check it after; run a bump with every tool current and `--release` equal to the stamp. | fixed: a T3 test for the kept mode and the 'unchanged' skip |
+
+Verdict: CHANGES c989c4adbb8233b13ce61bf1227c511c16fc68ce

docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/plan-2.md

@@ -0,0 +1,26 @@
+# Plan review, pass 2
+
+- Reviewer: plan-reviewer (fresh subagent, claude-opus-5-5)
+- Reviewed: 1698fde6afa319987814b7db05123d28afddd1ef (plan.md, against spec.md as approved in spec-4)
+
+I read the plan at 1698fde, pass 1's record and its fix diff (c989c4a..1698fde), `ticket show kit-48-box-pins-command` (the top-level done_when set), quality.md §3 and §6, cloud/Dockerfile's pin block (66-136) and stamp (515-525), and the head of test/machine.test.mjs (imports, `baseEnv`, the `bin` stubs).
+
+What holds:
+- F1: `pinsCopy` rewrites every `ARG <P>_VERSION=`, `_SHA256_*=` and `DEVTEAM_RELEASE=` line to `FIXED` before the edit. Every T1 edit still finds its target. `^(ARG TTYD_SHA256_X86_64=)\w+$` and `^ARG GH_SHA256_AARCH64=\w+\n` match a fixed hex sum. The gh URL text (`download/v$GH_VERSION/gh_${GH_VERSION}_linux`, line 98) and `# ttyd, the web terminal` (line 76) are not ARG lines, so the rewrite leaves them alone. `ARG DEVTEAM_RELEASE=` sits at line 519 with no indent. The expected strings agree with `FIXED`: `2.104.0` for gh, claude-swap at `1.0.0` against a TTYD set to `1.0.0`, `stale 1.7.7 -> 1.7.8`, `TTYD_VERSION ${FIXED.TTYD}`, ttyd's latest kept at 1.7.7 for "already current", and `v0.1.0` older than `v0.52.0`. `fixedSum(arg)` is distinct per ARG, so no sum collides with another or with a literal. Only the real-file case reads the real pins, through a regex.
+- F2: the plain tags 1.9.0 (prerelease) and 2.0.0 (draft) can only be excluded by their flags, and 1.6.9 after 1.7.8 still covers the max-not-first case.
+- F3: the TL;DR names `pinsEnv`, and T4 and T5 both spawn with it. It now matches the tasks.
+- F4/F5: the download half is in T3, and `hits` record headers. The download test asserts no authorization under `/dl`, `/assets`, `/npm` or `/pypi`.
+- F6-F12: `pinsAsync` is named. `safe` is in T1. The arrayBuffer departure is stated. `upstream(t)` unrefs and closes. The expected T1 failure is corrected. The fakes are chmodded 0o755. `UV_TOOL_DIR` is deleted. There is a mode and `unchanged` test.
+- Every done-when has a task and a test. DW1 is T1's exit-2 case (pinsRepo has no `cloud/`) plus the fixture server. DW2 is T1's cases. DW3 is T2. DW4 is T3. DW5 is T4 (image path, shims first on PATH, uv gated on `UV_TOOL_DIR`, cwd no repo). DW6 is T5. DW7 is `pinsEnv`.
+
+What doesn't hold: F1's fix makes the tests survive a real bump, but not a real hold (F1 below). A hold is the other edit this command invites on cloud/Dockerfile, and the Dockerfile's own comment (line 71) tells people how to add one.
+
+Values and refusals: checked at the spec stage. The fixes add no behaviour.
+
+| Id | Severity | Rule | Where | Finding | Disposition |
+|---|---|---|---|---|---|
+| F1 | Important | done-when 1 (CI passes); spec Contracts (the hold is a supported edit of cloud/Dockerfile) | T1 `pinsCopy` | `pinsCopy` rewrites the pin values but carries any `# hold:` line over from the real file. When a real hold lands (e.g. `# hold: <reason>` above `ARG UV_VERSION=`), several tests break. T2's "stale for all six → status 1" sees `uv: held`. T3's hold-on-uv test stacks a second `# hold:` line, so the first is "not directly above a _VERSION ARG", verify refuses, and bump exits 1. T1's "hold above MISE_VERSION" cases break if mise is the one held. This is the same failure path as plan-1 F1, through the other edit the feature exists for. Fix: have `pinsCopy` also drop every line matching `/^\s*#\s*hold\b/i` before `edit`, so tests start hold-free and add only their own. | fixed: pinsCopy drops every hold-like line before the test's edit |
+| F2 | Minor | plan Interfaces (plan-1 F6) | T2, the paragraph after Step 1 | It still says "Each test spawns asynchronously (`execFile` promisified)", which contradicts T1's `pinsAsync`. Promisified `execFile` rejects on a non-zero exit, which most of these tests expect. Say `pinsAsync`. | fixed: T2 names pinsAsync |
+| F3 | Suggestion | | T2 Interfaces "Produces" | `safe` is still listed as T2's, as well as T1's (plan-1 F6 moved it). Drop it from T2. | fixed: safe listed under T1 only |
+
+Verdict: CHANGES 1698fde6afa319987814b7db05123d28afddd1ef

docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/plan-3.md

@@ -0,0 +1,20 @@
+# Plan review, pass 3
+
+- Reviewer: plan-reviewer (fresh subagent, claude-opus-5-5)
+- Reviewed: e369ea4594cc6279eee48725ad1207c746e86e23 (plan.md, against spec.md as approved in spec-4)
+
+I read the whole plan at e369ea4, pass 2's record and its fix diff (1698fde..e369ea4), pass 1's record, `ticket show kit-48-box-pins-command` (the top-level done_when set), quality.md §3 and §6, and cloud/Dockerfile's pin ARGs (77-133, 519) and every line naming a hold.
+
+What holds:
+- plan-2 F1: `pinsCopy` now drops every line matching `/^\s*#\s*hold\b/i` before the test's `edit`, so a real hold in cloud/Dockerfile can't stack a second hold in T3, turn T2's "stale for all six" into `held`, or break T1's mise hold cases. Each test adds its own hold after the drop, so T1's hold cases (including the misplaced one above `# ttyd, the web terminal` and `# Hold: x`) and T3's malformed-hold case still see what they put there. The Dockerfile's instruction comment (line 71, `#   To hold a tool back, ...`) doesn't match the pattern, so the drop doesn't touch it, and the real-file `verify` case is unaffected.
+- plan-2 F2: T2 now says each test spawns with `pinsAsync` (T1), which resolves on a non-zero exit.
+- plan-2 F3: `safe` is listed under T1's Produces only. T4 consumes it from T1.
+- The fixes add no behaviour. The plan is still coherent. Every interface a task consumes is produced by an earlier task (`readPins`'s line indexes go to `bumpPins`, `latestOf`'s release goes to T3's assets, `BOX_TOOLS.run/parse` goes to T4). Every file is inside touches (`docs/devteam/scripts/devteam.mjs`, `test/machine.test.mjs`).
+- Each done-when has a test. DW1 is the fixture server plus T1's exit-2 case. DW2 is T1's cases. DW3 is T2. DW4 is T3. DW5 is T4 (image path, wrong shims first on PATH, uv gated on `UV_TOOL_DIR`, cwd no repo). DW6 is T5, and no file is added. DW7 is `pinsEnv` in T1–T5.
+- The TL;DR matches the tasks: five tasks in order, `pinsEnv`, fixed test pins, all-or-nothing write, and the digest cross-check.
+
+Values and refusals: checked at the spec stage (spec-1 to spec-4). The plan adds no behaviour beyond the approved spec.
+
+No findings.
+
+Verdict: APPROVE e369ea4594cc6279eee48725ad1207c746e86e23