From 30be090ac05cce6dc08ba4ed947938c089417703 Mon Sep 17 00:00:00 2001 From: Hugh Francis Date: Sat, 14 Nov 2026 21:58:16 +0000 Subject: [PATCH 01/22] kit-201: spec Co-Authored-By: Claude Opus 5.5 --- .../kit-201-tests-isolated-on-box/spec.md | 109 ++++++++++++++++++ 1 file changed, 109 insertions(+) create mode 100644 docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md new file mode 100644 index 0000000..9b0804a --- /dev/null +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md @@ -0,0 +1,109 @@ +# kit-201 spec: kit tests can't reach the real fleet + +## Why + +Incident p-20261114-214815998-supervisor: at 21:46 a kit test run from kit-46's worktree +(`test/machine.test.mjs`, "status says at a glance…") wrote ~25 lines into home's live log. The +worker's session carried `DEVTEAM_RUN`; `machine.test.mjs` and `operator-threads.test.mjs` strip +`DEVTEAM_ROLE|TICKET|FEATURE` and `DEVTEAM_HOME` but keep `DEVTEAM_RUN`, so the kit under test wrote +into the fleet's run folder. Separately, 15 of 16 `operator-threads` tests fail on a devteam box +because the box's own files (`/opt/devteam/gateway.mjs`, `/etc/devteam/thread-key.pub`) make the kit +under test believe it is on that box, whatever the test's environment says. + +## What exists today (cca0976) + +- `test/lib/kit.mjs` at import deletes only `DEVTEAM_HOME`, `TMUX`, `TMUX_PANE`, then sets its own + knobs (`DEVTEAM_CSWAP=off`, `DEVTEAM_SUPERVISOR_UP=off`, `DEVTEAM_CGROUP/MEMINFO ??=`, + `DEVTEAM_HUB_PORT`, `DEVTEAM_PORT_BASE`, `TMUX_TMPDIR`). +- `machine.test.mjs:14,34`, `operator-threads.test.mjs:19,67,626`, `hub-overview.test.mjs:133` each + repeat a partial denylist of their own. +- `gateway.test.mjs` does not import `lib/kit.mjs`; it spawns the kit with `process.env` whole. +- A worker on a box also carries `DEVTEAM_PUBLIC_URL` and `DEVTEAM_PREVIEWS`; `DEVTEAM_PUBLIC_URL` + alone makes `previewBox()`, and so `onBox()`, true (devteam.mjs ~6618, 13004). +- Box detection, devteam.mjs ~13004-13030: `onBox()` = `previewBox()` or the gateway file exists; + `threadsNeedGateway()` = `onBox()` or the gateway file; `gatewayThreadKey()` reads + `BOX_THREAD_KEY` whenever either box file exists, else `DEVTEAM_GATEWAY_THREAD_KEY`. The comment + there is deliberate: the image's file, not only the environment, says box, because an agent can + change the environment of a command it runs. +- Harness knobs that must survive the scrub (they are read by a process that imports `lib/kit.mjs` + before its own top level runs): `DEVTEAM_FORK` (kit.test.mjs → lib/fork.mjs), `DEVTEAM_FORK_AUDIT`, + `DEVTEAM_TEST_*` (KEEP, COLD, GITLEAKS, READ_DELAY_MS), `DEVTEAM_G1_RECORD`, `DEVTEAM_CGROUP`, + `DEVTEAM_MEMINFO` (test.yml sets the last two). + +## Acceptance checks (the done-whens) + +1. **One shared scrub.** `test/lib/kit.mjs`, at import, deletes every `DEVTEAM_*` variable except the + harness knobs above (an explicit list, `HARNESS_ENV`), and `TMUX`, `TMUX_PANE`, before setting its + own. Every test that spawns the kit gets its env from that scrubbed `process.env`: + `machine.test.mjs`, `operator-threads.test.mjs` and `hub-overview.test.mjs` drop their own + `DEVTEAM_*`/`TMUX` denylists (they keep stripping `CLAUDECODE|CLAUDE_*`, which isn't devteam's + and is outside this ticket), and `gateway.test.mjs` imports `lib/kit.mjs` so its spawns inherit the + scrub. Check: `grep -n "DEVTEAM_RUN\|'DEVTEAM_HOME'" test/{machine,operator-threads,hub-overview,gateway}.test.mjs` + finds no denylist; the scrub is the only one. +2. **Proven against a decoy.** A test in `machine.test.mjs` runs the incident's own test ("status says + at a glance…") in a child `node --test` whose environment carries `DEVTEAM_RUN=/run`, + `DEVTEAM_HOME=`, `DEVTEAM_ROLE=worker`, `DEVTEAM_TICKET=`, `DEVTEAM_PUBLIC_URL`, `TMUX` + and `TMUX_PANE`; the child passes, and the decoy folder holds no file afterwards. A second, quick + check: a child that imports `lib/kit.mjs` and prints its `DEVTEAM_*` names shows only the harness + knobs and the kit's own settings. +3. **Box detection can be turned off.** `DEVTEAM_BOX=off` makes the kit ignore the box's files + (`BOX_GATEWAY_FILE`, `BOX_THREAD_KEY`) in `onBox()`, `threadsNeedGateway()` and + `gatewayThreadKey()`: box-ness then comes from the environment only (`DEVTEAM_PUBLIC_URL`), and + the key from `DEVTEAM_GATEWAY_THREAD_KEY`, exactly as on a CI runner. `lib/kit.mjs` sets it. + Check: the three files' tests pass on this box (where both box files exist) with the session env + removed, and still pass in CI. +4. **CI and files.** test.yml passes on every job at the PR head; no file is added (the records + under `docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/` aside). + +## The off switch's limit (needs the supervisor's word) + +The box files exist so an agent can't make a command it runs believe it is off the box (and so take +an owner's thread press unsigned). A bare `DEVTEAM_BOX=off` would undo that. So the switch is honored +only while `$HOME` is not the account's own home (`os.userInfo().homedir`, from the password file, +not from `$HOME`): every owner record box detection guards (the operator's threads, `operatorDir()`) +lives under `os.homedir()`, so a command that turns the box off can only act on a throwaway home, +never the real operator's store. The tests that need it (`operator-threads`) already set `HOME` to a +temp dir; `hub-overview` will too (its hub registry then lives under its own temp dir, which is more +isolated, not less). + +## Approaches considered + +- **A. Bare `DEVTEAM_BOX=off`** (the ticket's example). Smallest; reopens the hole the file check + closed: an agent on a box runs `DEVTEAM_BOX=off devteam.mjs operator-thread …` against the real + operator store unsigned. +- **B. `DEVTEAM_BOX=off`, honored only off the account's own home** (recommended). One more + condition; the real store stays guarded. +- **C. Overridable box file paths** (`DEVTEAM_BOX_ROOT=`). Same weakness as A, and more + surface (two constants become env-driven). + +## Edge cases + +- `DEVTEAM_BOX` set to anything but `off`: ignored (box detection as today). +- A forked child (lib/fork.mjs) imports `lib/kit.mjs` again: `DEVTEAM_FORK` survives; its ports are + reassigned by the child's own import, as today. +- `cli-runner.mjs` replaces its env wholesale per request; it doesn't import `lib/kit.mjs`, so + per-call env (`{ DEVTEAM_ROLE: 'supervisor' }`) still reaches the kit. +- Tests that set `DEVTEAM_*` after import (`DEVTEAM_CLAIM_TOKEN`, `DEVTEAM_PUBLIC_MAX`, + `DEVTEAM_GATE_TICKET`) are unaffected: the scrub runs once, at import. +- The nested `node --test` child must not inherit `NODE_TEST_CONTEXT`, or it reports to the parent's + runner instead of running. + +## Contracts + +None provided or consumed: `DEVTEAM_BOX` is a new, test-facing env knob of the kit, documented in the +comment beside `onBox()`. + +## Out of scope + +- The other test files' own env handling beyond what importing the scrub fixes. +- `CLAUDECODE`/`CLAUDE_*` stripping. +- Any change to what a real box does when `DEVTEAM_BOX` is unset. + +## Rulings + +- m-20261114-215142046-supervisor (to kit-200, carried over by m-20261114-215602684-supervisor): + touches add `test/lib/kit.mjs` and `test/gateway.test.mjs`; the shared helper is a scrub at import + in `test/lib/kit.mjs`; keep it small. +- m-20261114-215602684-supervisor: run every kit test with + `env -u DEVTEAM_RUN -u DEVTEAM_HOME -u DEVTEAM_ROLE -u DEVTEAM_TICKET -u DEVTEAM_FEATURE TMUX_TMPDIR=$(mktemp -d)`; + this ticket lands before kit-46, kit-63 and kit-142. From 945f4765e4e767e215bac406e8fec15b01de4200 Mon Sep 17 00:00:00 2001 From: Hugh Francis Date: Sat, 14 Nov 2026 22:04:02 +0000 Subject: [PATCH 02/22] kit-201: spec after review pass 1, plan, isolated test runner Co-Authored-By: Claude Opus 5.5 --- .../kit-201-tests-isolated-on-box/plan.md | 91 +++++++++++++++++++ .../reviews/spec-1.md | 17 ++++ .../scripts/run-isolated.sh | 10 ++ .../kit-201-tests-isolated-on-box/spec.md | 75 +++++++++++---- 4 files changed, 177 insertions(+), 16 deletions(-) create mode 100644 docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md create mode 100644 docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-1.md create mode 100755 docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/scripts/run-isolated.sh diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md new file mode 100644 index 0000000..d82b07d --- /dev/null +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md @@ -0,0 +1,91 @@ +# kit-201 plan: kit tests can't reach the real fleet + +## TL;DR + +- One scrub in `test/lib/kit.mjs` at import deletes every `DEVTEAM_*` (but the harness's own knobs, `HARNESS_ENV`) and `TMUX`/`TMUX_PANE`, then sets `DEVTEAM_BOX=off`. +- The per-file `DEVTEAM_*`/`TMUX` denylists in machine, operator-threads and hub-overview go; gateway.test.mjs imports `lib/kit.mjs`; hub-overview gives its kit a temp `HOME`. +- devteam.mjs: `DEVTEAM_BOX=off` makes the kit ignore the box's files, honored only while `$HOME` (by real path) isn't the passwd home (ruling d-20261114-215844973-supervisor). +- The box code is marked as a block and loaded in a vm with a stand-in fs/os/env, so the limit is tested in CI. +- A decoy test re-runs the incident's own test with a worker's env pointed at a decoy run folder: nothing lands there. +- Three tasks; no file added; risk normal. + +> **For agentic workers:** executed natively (executing-plans), each task reviewed by a fresh subagent into `reviews/task-.md`. + +**Goal:** kit tests run on a devteam box or inside a worker can't write to the real fleet and pass where CI passes. + +**Architecture:** the shared test helper owns the env; the kit gains one guarded env knob for its box detection; the box code becomes a cut-out block so its rule is unit-tested. + +**Tech Stack:** Node ≥ 20, `node:test`, `node:vm`. + +**Spec:** `docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md` + +## Global Constraints + +- Only files in touches: `docs/devteam/scripts/devteam.mjs`, `test/machine.test.mjs`, `test/operator-threads.test.mjs`, `test/hub-overview.test.mjs`, `test/lib/kit.mjs`, `test/gateway.test.mjs`. No file is added (records under this folder aside). +- Every test run here goes through `docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/scripts/run-isolated.sh` (ruling m-20261114-215602684-supervisor), never with the session's own env. +- `DEVTEAM_BOX` unset, or anything but `off`, leaves box detection exactly as at main. +- Match the files' idiom: one-line arrow helpers, a short comment saying why above each. + +## Review Focus + +1. `HOME` a symlink to the passwd home: must count as the account's own home (Task 1 test). +2. `HOME` not resolvable (`realpathSync` throws), or no passwd entry (`os.userInfo()` throws): the switch is ignored, fail closed (Task 1 test: `HOME` naming a missing folder; `passwd: null` makes the stand-in `userInfo` throw). +3. A process that imports `lib/kit.mjs` and reads a harness knob after (lib/fork.mjs's `DEVTEAM_FORK`): must survive (Task 2 env-names test asserts `DEVTEAM_FORK` and `DEVTEAM_TEST_KEEP` survive). +4. `DEVTEAM_PUBLIC_URL` / `DEVTEAM_PREVIEWS` from a box worker: must be scrubbed, or `onBox()` stays true by env (Task 2 env-names test). +5. The nested `node --test` inheriting `NODE_TEST_CONTEXT` and reporting into the parent (Task 2 decoy test deletes it, and asserts the child's own TAP says `# pass 1`). + +--- + +### Task 1: the guarded off switch, as a tested block + +**Files:** +- Modify: `docs/devteam/scripts/devteam.mjs` (~6618 `previewBox`; ~12998-13030 box detection) +- Modify: `test/lib/kit.mjs` (a `boxFns()` loader beside `gitReadFns`) +- Test: `test/operator-threads.test.mjs` + +**Interfaces:** +- Produces (devteam.mjs): markers `/* preview box start */ … /* preview box end */` around `previewBox()`, `/* box start */ … /* box end */` around `onBox` … `gatewayThreadKey`. New in the block: `const boxFilesOff = () => boolean` (true only when `process.env.DEVTEAM_BOX === 'off'` and `fs.realpathSync(os.homedir())` and `fs.realpathSync(os.userInfo().homedir)` both resolve and differ; any throw → false), `const boxFile = (f) => !boxFilesOff() && fs.existsSync(f)`; `onBox`, `threadsNeedGateway`, `gatewayThreadKey` use `boxFile` in place of `fs.existsSync` on the two box constants. The comment above `BOX_GATEWAY_FILE` says why the switch is limited this way and what it doesn't defend against. +- Produces (kit.mjs): `boxFns({ files = [], home, passwd, env = {} }) => { onBox, threadsNeedGateway, gatewayThreadKey, boxFilesOff, BOX_THREAD_KEY }`, run in a vm whose `fs` answers `existsSync`/`statSync`/`readFileSync` from real disk except the two box paths, which exist iff listed in `files` (their stat/read go to stand-in files the loader writes in a temp dir: `BOX_THREAD_KEY`'s content is a public key the caller passes as `files[path]` — so `files` is `{ [path]: text }`), `realpathSync` real; `os.homedir()` = `home`, `os.userInfo().homedir` = `passwd` (`passwd: null` → `userInfo()` throws, as for a uid with no entry); `process.env` = `{ HOME: home, ...env }`; `crypto`, `path`, `URL` real. + +- [ ] **Step 1: write the failing test** in `operator-threads.test.mjs`: `test('DEVTEAM_BOX=off turns the box's files off only for a throwaway home: at the account's own home a box stays a box', …)`. Fixture: `passwd` = a temp dir, `link` = a symlink to it, `tmpHome` = another temp dir; `files` = both box paths, the key file holding `GW_PUBLIC`'s text; `env.DEVTEAM_GATEWAY_THREAD_KEY` = a file holding `otherKey`'s public half. Assertions: + - `home: passwd, env: { DEVTEAM_BOX: 'off' }` → `onBox() === true`, `threadsNeedGateway() === true`, `gatewayThreadKey()` exports equal to `GW_PUBLIC`'s key, `boxFilesOff() === false`. + - `home: link`, same env → same four results; `home: passwd + '/'` → same. + - `home: tmpHome, passwd: null, env: { DEVTEAM_BOX: 'off' }` → `boxFilesOff() === false`, `onBox() === true`. + - `home: path.join(tmpHome, 'missing')` → `boxFilesOff() === false`, `onBox() === true`. + - `home: tmpHome, env: { DEVTEAM_BOX: 'off' }` → `onBox() === false`, `threadsNeedGateway() === false`, `gatewayThreadKey()` is the env's key; with `DEVTEAM_PUBLIC_URL: 'https://box.example.com'` added, `onBox() === true` (box by env) and the key still the env's. + - `home: tmpHome`, `DEVTEAM_BOX` unset, then `'on'` → `onBox() === true`, key is the box file's. + - no `files`, `home: passwd`, no env → `onBox() === false` (a laptop). +- [ ] **Step 2: run it, see it fail** — `run-isolated.sh --test-name-pattern='DEVTEAM_BOX=off' test/operator-threads.test.mjs` → fails at `boxFns` (no `/* box start */` block). +- [ ] **Step 3: implement** the markers, `boxFilesOff`, `boxFile` in devteam.mjs and `boxFns` in kit.mjs. +- [ ] **Step 4: run it, see it pass**; then `node --check docs/devteam/scripts/devteam.mjs`. +- [ ] **Step 5: commit** `kit-201: DEVTEAM_BOX=off, honored only off the account's own home`; review into `reviews/task-1.md`. + +### Task 2: one scrub, and the decoy proof + +**Files:** +- Modify: `test/lib/kit.mjs:22-41` (the scrub), `test/machine.test.mjs:14,34`, `test/operator-threads.test.mjs:19,67,626`, `test/hub-overview.test.mjs:132-133`, `test/gateway.test.mjs` (imports; loops at 723, 764; its "claims no block" comment at 18) +- Test: `test/machine.test.mjs` + +**Interfaces:** +- Produces (kit.mjs): `export const HARNESS_ENV = /^DEVTEAM_(TEST_.*|FORK|FORK_AUDIT|G1_RECORD|CGROUP|MEMINFO)$/`; at import, before any of its own settings, delete every key matching `/^DEVTEAM_/` not matching `HARNESS_ENV`, and `TMUX`, `TMUX_PANE`; then `process.env.DEVTEAM_BOX = 'off'` (comment: the kit honors it only for a throwaway `HOME`, which every test that needs it sets). +- machine.test.mjs:14 and operator-threads.test.mjs:19 keep only their `CLAUDECODE|CLAUDE_*` part (operator-threads keeps `DEVTEAM_OPERATOR_COMMAND`/`PUBLIC_URL` out by the scrub now); the `for (const k of [...]) delete env[k]` lines at machine:34, operator-threads:67,626, hub-overview:133 go. hub-overview's env gains `HOME: base`; gateway.test.mjs's loops at 723 and 764 go. gateway.test.mjs gains `import './lib/kit.mjs';` first among its local imports (its own `DEVTEAM_HUB_PORT`/`PORT_BASE` settings run after and win). + +- [ ] **Step 1: write the failing tests** in `machine.test.mjs`: + - `test('importing the test helper leaves no session's DEVTEAM_* or tmux in the environment, only the harness's own knobs', …)`: spawn `node --input-type=module -e "await import(); console.log(JSON.stringify(Object.keys(process.env).filter(k => /^(DEVTEAM_|TMUX)/.test(k)).sort()))"` with env carrying `DEVTEAM_RUN`, `DEVTEAM_HOME`, `DEVTEAM_ROLE=worker`, `DEVTEAM_TICKET=x-1`, `DEVTEAM_FEATURE`, `DEVTEAM_SUPERVISOR`, `DEVTEAM_PUBLIC_URL`, `DEVTEAM_PREVIEWS=1`, `TMUX=x`, `TMUX_PANE=%9`, `DEVTEAM_FORK=`, `DEVTEAM_TEST_KEEP=1`; assert the printed list equals exactly `['DEVTEAM_BOX','DEVTEAM_CGROUP','DEVTEAM_CSWAP','DEVTEAM_FORK','DEVTEAM_HUB_PORT','DEVTEAM_MEMINFO','DEVTEAM_PORT_BASE','DEVTEAM_SUPERVISOR_UP','DEVTEAM_TEST_KEEP','TMUX_TMPDIR']`. + - `test('a worker's session env sends nothing into its run folder: the incident's own test, run under it, writes nothing in the decoy', { skip: hasTmux ? false : 'needs tmux', timeout: 300000 }, …)`: `decoy` = temp dir with `run/` inside; spawn `node --test --test-reporter=tap --test-name-pattern='^status says at a glance' test/machine.test.mjs` with `{ ...process.env, DEVTEAM_RUN: decoy/run, DEVTEAM_HOME: decoy, DEVTEAM_ROLE: 'worker', DEVTEAM_TICKET: 'x-1', DEVTEAM_PUBLIC_URL: 'https://box.example.com', TMUX: 'x', TMUX_PANE: '%9' }` minus `NODE_TEST_CONTEXT`; assert status 0, stdout matches `/^# pass 1$/m`, `/^# skip 0$/m` and `/^# fail 0$/m`, and a recursive listing of `decoy` is `['run']` (the empty folder only). +- [ ] **Step 2: run them, see them fail** (`run-isolated.sh --test-name-pattern='importing the test helper|decoy' test/machine.test.mjs`): the env list holds `DEVTEAM_RUN` etc.; the decoy holds log files (the incident reproduced, against the decoy). Paste the failing listing into `reviews/task-2.md`'s notes or `verification.md`. +- [ ] **Step 2b: check done-when 1** with the spec's AC1 grep after Step 3: no match. +- [ ] **Step 3: implement** the scrub and the per-file removals above. +- [ ] **Step 4: run them, see them pass.** +- [ ] **Step 5: commit** `kit-201: one env scrub for every test that spawns the kit`; review into `reviews/task-2.md`. + +### Task 3: the three files on the box, and the suite + +- [ ] **Step 1:** `run-isolated.sh --test-reporter=tap test/operator-threads.test.mjs test/machine.test.mjs test/hub-overview.test.mjs` on this box (both box files present) → `# fail 0` (baseline at cca0976: operator-threads 1 pass / 15 fail). +- [ ] **Step 2:** the same three with the session's own env (only `TMUX_TMPDIR` set to a fresh dir, through a second script `run-as-worker.sh` beside run-isolated.sh, which keeps `DEVTEAM_RUN` pointed at a decoy copy, never home's) → `# fail 0`, decoy empty. +- [ ] **Step 3:** `run-isolated.sh` on `test/gateway.test.mjs` and `test/kit.test.mjs` (fork path) → pass; then the full suite as test.yml runs it, through `devteam.mjs gate test`. +- [ ] **Step 4:** push; test.yml green on every job; `verification.md` with a row per done-when at the commit. + +## Rulings + +See the spec's Rulings (m-20261114-215142046, m-20261114-215602684, d-20261114-215844973). diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-1.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-1.md new file mode 100644 index 0000000..28bdc0f --- /dev/null +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-1.md @@ -0,0 +1,17 @@ +# Spec review, pass 1 + +- Reviewer: spec-reviewer (fresh subagent, claude-opus) +- Reviewed: 30be090ac05cce6dc08ba4ed947938c089417703 + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| F1 | Important | spec "The off switch's limit"; P6; P5 | docs/devteam/scripts/devteam.mjs:14548, 14807-14820, 463, 13052 | The premise the supervisor is asked to rule on is false. The spec says "every owner record box detection guards ... lives under `os.homedir()`". It doesn't. `threadsNeedGateway()` also guards the dashboard's `/api/pane-answer` (14548) and `pane answered` (14807), and both write an owner `pane-answer` act into the project's log (`runDir`, from cwd or `DEVTEAM_RUN`, 463) and type the held lines into the supervisor's real tmux pane. None of that is under `$HOME`, so with option B, `DEVTEAM_BOX=off HOME=/tmp/x` still gets an unsigned owner act on a box. The guard as written (`$HOME` !== `os.userInfo().homedir`) can also be passed with the real store: `HOME=/data/home/`, `/data/home/.` or a symlink to it is a different string, but `operatorDir()` (13052) resolves to the real `~/operator`. P6 is the owner's direction here: agents are trusted not to forge, prefer the simpler design and say in one line what it leaves undefended. Either take A and add that line (the only path to the switch is a deliberate `DEVTEAM_BOX=off`, which nothing outside `lib/kit.mjs` sets), or keep B with a realpath comparison and a true statement of its limit (it doesn't cover pane answers or the project log). As written, the section gives the supervisor a wrong argument to rule on. | fixed: limit restated per P6 (stops the accidental path; pane answers named in the undefended line); realpath compare | +| F2 | Important | done-when 2; spec AC2 | test/machine.test.mjs:255 | The proof can pass without proving anything. The incident's test is `{ skip: !hasTmux }`, and "the child passes" is also true when that test is skipped. Then the decoy stays empty whether or not the scrub works. A name pattern that matches nothing passes the same way. The spec needs the parent to assert that the child ran exactly that test and that it passed (pass 1, skip 0, from the child's TAP or reporter output). It should also say that the test fails at cca0976, with the decoy holding files, so the check is shown to catch the leak it is for (red before green). The decoy should be a run folder the pre-fix kit really writes into (`DEVTEAM_RUN=/run` resolved, not refused). | fixed: the child must report pass 1, skip 0, fail 0; a pre-fix failing run is recorded | +| F3 | Minor | done-when 1 ("one shared helper"); spec AC1 check | test/gateway.test.mjs:723,764; test/machine.test.mjs:14 | The AC1 check, ``grep -n "DEVTEAM_RUN\|'DEVTEAM_HOME'"``, misses the denylists that remain. gateway.test.mjs has two `for (const k of ['DEVTEAM_ROLE', 'DEVTEAM_TICKET', 'DEVTEAM_FEATURE', 'TMUX', 'TMUX_PANE']) delete …` loops, and machine.test.mjs:14's regex holds `DEVTEAM_ROLE|TICKET|FEATURE`. The spec says to drop the other three files' denylists but says nothing about gateway's. State what happens to gateway's loops, and make the check match any `DEVTEAM_` or `TMUX` delete in those files. | fixed: gateway loops 723/764 and machine:14 regex named; AC1 grep widened | +| F4 | Minor | spec Edge cases; "The off switch's limit" | (new code beside devteam.mjs:13004) | `os.userInfo()` throws when the uid has no passwd entry (a container run with an arbitrary uid). The spec doesn't say what happens then. If option B stays, it should fail closed: the switch is ignored and box detection works as today. | fixed: a userInfo or realpath throw ignores the switch (fails closed), tested | +| F5 | Minor | done-when 1, 4 | test/gateway.test.mjs:17-22, 756-764; test/lib/ports.mjs:22 | Importing `lib/kit.mjs` into gateway.test.mjs has side effects the spec doesn't mention. It claims a port block, though both files say the gateway test claims none. It puts the tmux shim first on PATH, so the real-hub test's `TMUX_TMPDIR: noTmux` (not a `dt-tmux-*` dir) now gets "tmux refused" (the operator's state becomes no-tmux rather than missing). And it sets `DEVTEAM_CSWAP` and `DEVTEAM_SUPERVISOR_UP`. Each is probably harmless, but the spec should name them and update the "claims no block" comments. | fixed: the side effects are named in Edge cases | +| F6 | Suggestion | spec AC1 ("an explicit list, HARNESS_ENV") | spec "What exists today", last bullet | `DEVTEAM_TEST_*` is given as a glob, but the spec also calls `HARNESS_ENV` an explicit list. Name the four (`DEVTEAM_TEST_KEEP`, `_COLD`, `_GITLEAKS`, `_READ_DELAY_MS`) or say that a prefix is allowed. I checked the knob list against every `process.env.DEVTEAM_*` read in test/. The rest are set by the kit or by a test after import (`HUB_PORT`, `PORT_BASE`, `CLAIM_TOKEN`, `PUBLIC_MAX`, `GATE_TICKET`, `HUB_RETRY_MS`) or read by fixture scripts the kit spawns (`PARAMS`, `SINCE`), so the list is complete. | fixed: HARNESS_ENV names its members and the DEVTEAM_TEST_ prefix | + +Values and refusals: P1 (harness-agnostic): no change, this is the test harness and an env knob. P2 (nothing project-specific): holds. P3/P4: the off-switch question is about the trust model, but the owner already set the direction in P6, so the supervisor can decide it within that direction and report it rather than escalate. P5 (adversarial review for authority): this review tried to break the HOME guard and did (F1). P6 (keep it simple): the spec picks the more complex option B on a security argument that doesn't hold, and doesn't name in one line what it leaves undefended (F1). R1 (no self-widening): holds under normal use. Only `lib/kit.mjs`, inside test processes, sets `DEVTEAM_BOX`, and kit-spawned workers on those tests' own tmux servers don't reach a real fleet. A confused agent wouldn't hit it by accident. R2 (owner-facing messages): no owner-facing message. + +Verdict: CHANGES 30be090ac05cce6dc08ba4ed947938c089417703 diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/scripts/run-isolated.sh b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/scripts/run-isolated.sh new file mode 100755 index 0000000..657f899 --- /dev/null +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/scripts/run-isolated.sh @@ -0,0 +1,10 @@ +#!/bin/sh +# Runs kit test files with the fleet's session env removed (ruling m-20261114-215602684-supervisor). +# usage: run-isolated.sh ... +cd "$(dirname "$0")/../../../../.." || exit 1 +d=$(mktemp -d) +env -u DEVTEAM_RUN -u DEVTEAM_HOME -u DEVTEAM_ROLE -u DEVTEAM_TICKET -u DEVTEAM_FEATURE -u DEVTEAM_SUPERVISOR \ + -u DEVTEAM_PUBLIC_URL -u DEVTEAM_PREVIEWS -u TMUX -u TMUX_PANE TMUX_TMPDIR="$d" nice node --test "$@" +s=$? +rm -rf "$d" +exit $s diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md index 9b0804a..b026903 100644 --- a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md @@ -34,16 +34,25 @@ under test believe it is on that box, whatever the test's environment says. 1. **One shared scrub.** `test/lib/kit.mjs`, at import, deletes every `DEVTEAM_*` variable except the harness knobs above (an explicit list, `HARNESS_ENV`), and `TMUX`, `TMUX_PANE`, before setting its - own. Every test that spawns the kit gets its env from that scrubbed `process.env`: - `machine.test.mjs`, `operator-threads.test.mjs` and `hub-overview.test.mjs` drop their own - `DEVTEAM_*`/`TMUX` denylists (they keep stripping `CLAUDECODE|CLAUDE_*`, which isn't devteam's - and is outside this ticket), and `gateway.test.mjs` imports `lib/kit.mjs` so its spawns inherit the - scrub. Check: `grep -n "DEVTEAM_RUN\|'DEVTEAM_HOME'" test/{machine,operator-threads,hub-overview,gateway}.test.mjs` + own. `HARNESS_ENV` names `DEVTEAM_FORK`, `DEVTEAM_FORK_AUDIT`, `DEVTEAM_G1_RECORD`, + `DEVTEAM_CGROUP`, `DEVTEAM_MEMINFO` and the prefix `DEVTEAM_TEST_` (the suite's own switches: + KEEP, COLD, GITLEAKS, READ_DELAY_MS today, and any it adds). Every test that spawns the kit gets its env from that scrubbed `process.env`: + `machine.test.mjs` (l.14's regex keeps only `CLAUDECODE|CLAUDE_*`, which isn't devteam's and is + outside this ticket; l.34's loop goes), `operator-threads.test.mjs` (l.19 likewise; l.67 and + l.626 go), `hub-overview.test.mjs` (l.133 goes) and `gateway.test.mjs` (its loops at l.723 and + l.764 go) drop their own `DEVTEAM_*`/`TMUX` denylists, and `gateway.test.mjs` imports + `lib/kit.mjs` so its spawns inherit the scrub. Check: + `grep -nE "DEVTEAM_(ROLE|TICKET|FEATURE|RUN|HOME|SUPERVISOR|PUBLIC_URL)'?[],|)]|delete [a-zA-Z.]*\[k\]" test/{machine,operator-threads,hub-overview,gateway}.test.mjs` finds no denylist; the scrub is the only one. 2. **Proven against a decoy.** A test in `machine.test.mjs` runs the incident's own test ("status says at a glance…") in a child `node --test` whose environment carries `DEVTEAM_RUN=/run`, `DEVTEAM_HOME=`, `DEVTEAM_ROLE=worker`, `DEVTEAM_TICKET=`, `DEVTEAM_PUBLIC_URL`, `TMUX` - and `TMUX_PANE`; the child passes, and the decoy folder holds no file afterwards. A second, quick + and `TMUX_PANE`; the child's own TAP says exactly one test ran and passed (`# pass 1`, + `# skip 0`, `# fail 0`: a skipped test, or a name pattern matching nothing, fails the proof), and + the decoy folder holds nothing but its empty `run/` afterwards. The test skips, as the incident's + does, where there is no tmux. Before the scrub lands, it is run once and fails with files in the + decoy (the pre-fix kit does write into `DEVTEAM_RUN`, as the incident shows); that run is recorded + in `reviews/task-2.md` or `verification.md`. A second, quick check: a child that imports `lib/kit.mjs` and prints its `DEVTEAM_*` names shows only the harness knobs and the kit's own settings. 3. **Box detection can be turned off.** `DEVTEAM_BOX=off` makes the kit ignore the box's files @@ -55,16 +64,35 @@ under test believe it is on that box, whatever the test's environment says. 4. **CI and files.** test.yml passes on every job at the PR head; no file is added (the records under `docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/` aside). -## The off switch's limit (needs the supervisor's word) - -The box files exist so an agent can't make a command it runs believe it is off the box (and so take -an owner's thread press unsigned). A bare `DEVTEAM_BOX=off` would undo that. So the switch is honored -only while `$HOME` is not the account's own home (`os.userInfo().homedir`, from the password file, -not from `$HOME`): every owner record box detection guards (the operator's threads, `operatorDir()`) -lives under `os.homedir()`, so a command that turns the box off can only act on a throwaway home, -never the real operator's store. The tests that need it (`operator-threads`) already set `HOME` to a -temp dir; `hub-overview` will too (its hub registry then lives under its own temp dir, which is more -isolated, not less). +## The off switch's limit (ruled: B, d-20261114-215844973-supervisor) + +The box files exist so that normal use of a command on a box can't take an owner's press unsigned +(an owner thread turn, `operator-thread …`; a pane answer, `/api/pane-answer` and `pane answered`, +devteam.mjs ~14548, ~14807). A bare `DEVTEAM_BOX=off` that leaked from a test's env into a command +run at the account's own home would undo that by accident. So the switch is honored only while +`$HOME` is not the account's own home: both `$HOME` and `os.userInfo().homedir` (from the password +file, not from `$HOME`) are compared by real path (`fs.realpathSync`), so a `HOME` that is the home +with a trailing slash or a symlink to it counts as the home. When either can't be resolved +(`os.userInfo()` throws for a uid with no passwd entry, or `HOME` names nothing), the switch is +ignored: box detection works as today (fails closed). The tests that need it (`operator-threads`, +gateway's box tests) already set `HOME` to a temp dir; `hub-overview` will too (its hub registry +then lives under its own temp dir, more isolated, not less). + +Per P6 (agents are trusted not to forge), this stops the accidental path and no more. What it +doesn't defend against, in one line in the PR body (the ruling asks for it): an agent that sets +`HOME` to a temp folder on purpose (and copies the store there) gets the laptop's unsigned behaviour, +for the operator threads in that copy and for pane answers into the real project's log and pane. + +5. **The limit is tested** (the ruling's added done-when). The box-detection code is marked as a block + in devteam.mjs (`/* box start */ … /* box end */`, beside `previewBox()`'s use), and + `test/lib/kit.mjs` loads it in a vm with a stand-in `fs` (the box files exist or not), `os` + (`homedir()` from the test's `HOME`, `userInfo().homedir` the passwd home) and `process.env`, as + `coverageFns()`/`gitReadFns()` already load theirs. A test in `operator-threads.test.mjs` checks, + on a box-shaped fixture (both box files exist): with `HOME` equal to the passwd home, + `DEVTEAM_BOX=off` is ignored and `onBox()`, `threadsNeedGateway()` stay true and + `gatewayThreadKey()` reads the box's key; with `HOME` a symlink to it, the same; with `HOME` a + temp folder, all three follow the environment; with `DEVTEAM_BOX` unset or `on`, the files win. + It runs in CI, where no box file exists, because the files are the stand-in's. ## Approaches considered @@ -88,6 +116,15 @@ isolated, not less). - The nested `node --test` child must not inherit `NODE_TEST_CONTEXT`, or it reports to the parent's runner instead of running. +- `gateway.test.mjs` importing `lib/kit.mjs` takes its side effects too: it claims one port block + (the gateway's own servers still pick ports outside 20000..32699; the comments in ports.mjs and + gateway.test.mjs that say it claims none are updated, ports.mjs's only if it is put in touches, + else left and noted), it puts the tmux shim first on PATH (the real-hub test's `TMUX_TMPDIR` + names a folder with no server, so its tmux was already refused or serverless; with the shim it is + refused outright, which is what that test wants: nothing typed into), and it sets + `DEVTEAM_CSWAP=off`, `DEVTEAM_SUPERVISOR_UP=off`, `DEVTEAM_BOX=off` (all safer for a test). + Its own `DEVTEAM_HUB_PORT`/`PORT_BASE` settings run after the import and win. + ## Contracts None provided or consumed: `DEVTEAM_BOX` is a new, test-facing env knob of the kit, documented in the @@ -101,6 +138,12 @@ comment beside `onBox()`. ## Rulings +- d-20261114-215844973-supervisor (m-20261114-215845202-supervisor, re m-20261114-215838230): rule B, + the switch honored only off the account's own home. Add a test that on a box-shaped fixture with + HOME equal to the passwd home, DEVTEAM_BOX=off is ignored and onBox() stays true. The PR body says + in one line what it doesn't defend against (an agent that sets HOME to a temp folder and copies the + store there). + - m-20261114-215142046-supervisor (to kit-200, carried over by m-20261114-215602684-supervisor): touches add `test/lib/kit.mjs` and `test/gateway.test.mjs`; the shared helper is a scrub at import in `test/lib/kit.mjs`; keep it small. From bfe8b65ff4f2c606fae8c1cf5ca71c4eea48d53e Mon Sep 17 00:00:00 2001 From: Hugh Francis Date: Sat, 14 Nov 2026 22:10:52 +0000 Subject: [PATCH 03/22] kit-201: plan after review pass 1; spec review pass 2 Co-Authored-By: Claude Opus 5.5 --- .../kit-201-tests-isolated-on-box/plan.md | 19 +++++++------ .../reviews/plan-1.md | 20 ++++++++++++++ .../reviews/spec-2.md | 27 +++++++++++++++++++ 3 files changed, 58 insertions(+), 8 deletions(-) create mode 100644 docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/plan-1.md create mode 100644 docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-2.md diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md index d82b07d..7b95128 100644 --- a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md @@ -32,7 +32,9 @@ 2. `HOME` not resolvable (`realpathSync` throws), or no passwd entry (`os.userInfo()` throws): the switch is ignored, fail closed (Task 1 test: `HOME` naming a missing folder; `passwd: null` makes the stand-in `userInfo` throw). 3. A process that imports `lib/kit.mjs` and reads a harness knob after (lib/fork.mjs's `DEVTEAM_FORK`): must survive (Task 2 env-names test asserts `DEVTEAM_FORK` and `DEVTEAM_TEST_KEEP` survive). 4. `DEVTEAM_PUBLIC_URL` / `DEVTEAM_PREVIEWS` from a box worker: must be scrubbed, or `onBox()` stays true by env (Task 2 env-names test). -5. The nested `node --test` inheriting `NODE_TEST_CONTEXT` and reporting into the parent (Task 2 decoy test deletes it, and asserts the child's own TAP says `# pass 1`). +5. The nested `node --test` inheriting `NODE_TEST_CONTEXT` and reporting into the parent (Task 2 decoy test deletes it, and asserts on the child's own TAP, as Task 2 says). + +Readings of the approved spec (spec-2 F2-1..F2-4, plan-1): the spec's "`# skip 0`" is read as "the incident's test ran and was not skipped", checked per Task 2 (Node's TAP says `# skipped N`, and Node 18/20 count tests filtered out by a name pattern as skipped). The spec's AC1 grep is read with its known non-denylist matches named in Task 2 Step 2b. The spec's "Approaches" wording for B is superseded by its corrected limit section. --- @@ -45,7 +47,7 @@ **Interfaces:** - Produces (devteam.mjs): markers `/* preview box start */ … /* preview box end */` around `previewBox()`, `/* box start */ … /* box end */` around `onBox` … `gatewayThreadKey`. New in the block: `const boxFilesOff = () => boolean` (true only when `process.env.DEVTEAM_BOX === 'off'` and `fs.realpathSync(os.homedir())` and `fs.realpathSync(os.userInfo().homedir)` both resolve and differ; any throw → false), `const boxFile = (f) => !boxFilesOff() && fs.existsSync(f)`; `onBox`, `threadsNeedGateway`, `gatewayThreadKey` use `boxFile` in place of `fs.existsSync` on the two box constants. The comment above `BOX_GATEWAY_FILE` says why the switch is limited this way and what it doesn't defend against. -- Produces (kit.mjs): `boxFns({ files = [], home, passwd, env = {} }) => { onBox, threadsNeedGateway, gatewayThreadKey, boxFilesOff, BOX_THREAD_KEY }`, run in a vm whose `fs` answers `existsSync`/`statSync`/`readFileSync` from real disk except the two box paths, which exist iff listed in `files` (their stat/read go to stand-in files the loader writes in a temp dir: `BOX_THREAD_KEY`'s content is a public key the caller passes as `files[path]` — so `files` is `{ [path]: text }`), `realpathSync` real; `os.homedir()` = `home`, `os.userInfo().homedir` = `passwd` (`passwd: null` → `userInfo()` throws, as for a uid with no entry); `process.env` = `{ HOME: home, ...env }`; `crypto`, `path`, `URL` real. +- Produces (kit.mjs): `boxFns({ files = {}, home, passwd, env = {} })` (`files`: `{ [boxPath]: text }`, e.g. `{ '/opt/devteam/gateway.mjs': '', '/etc/devteam/thread-key.pub': pem }`) => { onBox, threadsNeedGateway, gatewayThreadKey, boxFilesOff, BOX_THREAD_KEY }`, run in a vm whose `fs` answers `existsSync`/`statSync`/`readFileSync` from real disk except the two box paths, which exist iff listed in `files` (their stat/read go to stand-in files the loader writes in a temp dir, holding the text the caller gives), `realpathSync` real; `os.homedir()` = `home`, `os.userInfo().homedir` = `passwd` (`passwd: null` → `userInfo()` throws, as for a uid with no entry); `process.env` = `{ HOME: home, ...env }`; `crypto`, `path`, `URL` real. - [ ] **Step 1: write the failing test** in `operator-threads.test.mjs`: `test('DEVTEAM_BOX=off turns the box's files off only for a throwaway home: at the account's own home a box stays a box', …)`. Fixture: `passwd` = a temp dir, `link` = a symlink to it, `tmpHome` = another temp dir; `files` = both box paths, the key file holding `GW_PUBLIC`'s text; `env.DEVTEAM_GATEWAY_THREAD_KEY` = a file holding `otherKey`'s public half. Assertions: - `home: passwd, env: { DEVTEAM_BOX: 'off' }` → `onBox() === true`, `threadsNeedGateway() === true`, `gatewayThreadKey()` exports equal to `GW_PUBLIC`'s key, `boxFilesOff() === false`. @@ -63,7 +65,7 @@ ### Task 2: one scrub, and the decoy proof **Files:** -- Modify: `test/lib/kit.mjs:22-41` (the scrub), `test/machine.test.mjs:14,34`, `test/operator-threads.test.mjs:19,67,626`, `test/hub-overview.test.mjs:132-133`, `test/gateway.test.mjs` (imports; loops at 723, 764; its "claims no block" comment at 18) +- Modify: `test/lib/kit.mjs:22-41` (the scrub), `test/lib/kit.mjs:95` (fleetFixture's own denylist goes; it keeps setting its own `TMUX_TMPDIR`), `test/machine.test.mjs:14,34`, `test/operator-threads.test.mjs:19,67,626`, `test/hub-overview.test.mjs:132-133`, `test/gateway.test.mjs` (imports; loops at 723, 764; its "claims no block" comment at 18) - Test: `test/machine.test.mjs` **Interfaces:** @@ -71,10 +73,10 @@ - machine.test.mjs:14 and operator-threads.test.mjs:19 keep only their `CLAUDECODE|CLAUDE_*` part (operator-threads keeps `DEVTEAM_OPERATOR_COMMAND`/`PUBLIC_URL` out by the scrub now); the `for (const k of [...]) delete env[k]` lines at machine:34, operator-threads:67,626, hub-overview:133 go. hub-overview's env gains `HOME: base`; gateway.test.mjs's loops at 723 and 764 go. gateway.test.mjs gains `import './lib/kit.mjs';` first among its local imports (its own `DEVTEAM_HUB_PORT`/`PORT_BASE` settings run after and win). - [ ] **Step 1: write the failing tests** in `machine.test.mjs`: - - `test('importing the test helper leaves no session's DEVTEAM_* or tmux in the environment, only the harness's own knobs', …)`: spawn `node --input-type=module -e "await import(); console.log(JSON.stringify(Object.keys(process.env).filter(k => /^(DEVTEAM_|TMUX)/.test(k)).sort()))"` with env carrying `DEVTEAM_RUN`, `DEVTEAM_HOME`, `DEVTEAM_ROLE=worker`, `DEVTEAM_TICKET=x-1`, `DEVTEAM_FEATURE`, `DEVTEAM_SUPERVISOR`, `DEVTEAM_PUBLIC_URL`, `DEVTEAM_PREVIEWS=1`, `TMUX=x`, `TMUX_PANE=%9`, `DEVTEAM_FORK=`, `DEVTEAM_TEST_KEEP=1`; assert the printed list equals exactly `['DEVTEAM_BOX','DEVTEAM_CGROUP','DEVTEAM_CSWAP','DEVTEAM_FORK','DEVTEAM_HUB_PORT','DEVTEAM_MEMINFO','DEVTEAM_PORT_BASE','DEVTEAM_SUPERVISOR_UP','DEVTEAM_TEST_KEEP','TMUX_TMPDIR']`. - - `test('a worker's session env sends nothing into its run folder: the incident's own test, run under it, writes nothing in the decoy', { skip: hasTmux ? false : 'needs tmux', timeout: 300000 }, …)`: `decoy` = temp dir with `run/` inside; spawn `node --test --test-reporter=tap --test-name-pattern='^status says at a glance' test/machine.test.mjs` with `{ ...process.env, DEVTEAM_RUN: decoy/run, DEVTEAM_HOME: decoy, DEVTEAM_ROLE: 'worker', DEVTEAM_TICKET: 'x-1', DEVTEAM_PUBLIC_URL: 'https://box.example.com', TMUX: 'x', TMUX_PANE: '%9' }` minus `NODE_TEST_CONTEXT`; assert status 0, stdout matches `/^# pass 1$/m`, `/^# skip 0$/m` and `/^# fail 0$/m`, and a recursive listing of `decoy` is `['run']` (the empty folder only). + - `test('importing the test helper leaves no session's DEVTEAM_* or tmux in the environment, only the harness's own knobs', …)`: spawn `node --input-type=module -e "await import(); console.log(JSON.stringify(Object.keys(process.env).filter(k => /^(DEVTEAM_|TMUX)/.test(k)).sort()))"` with env carrying `DEVTEAM_RUN`, `DEVTEAM_HOME`, `DEVTEAM_ROLE=worker`, `DEVTEAM_TICKET=x-1`, `DEVTEAM_FEATURE`, `DEVTEAM_SUPERVISOR`, `DEVTEAM_PUBLIC_URL`, `DEVTEAM_PREVIEWS=1`, `TMUX=x`, `TMUX_PANE=%9`, `DEVTEAM_FORK=`, `DEVTEAM_TEST_KEEP=1`; assert the printed list, less any `DEVTEAM_TEST_*` name (CI's rest job sets `DEVTEAM_TEST_GITLEAKS`; a developer may set others), equals exactly `['DEVTEAM_BOX','DEVTEAM_CGROUP','DEVTEAM_CSWAP','DEVTEAM_FORK','DEVTEAM_HUB_PORT','DEVTEAM_MEMINFO','DEVTEAM_PORT_BASE','DEVTEAM_SUPERVISOR_UP','TMUX_TMPDIR']`, and that it includes `DEVTEAM_TEST_KEEP`. + - `test('a worker's session env sends nothing into its run folder: the incident's own test, run under it, writes nothing in the decoy', { skip: hasTmux ? false : 'needs tmux', timeout: 300000 }, …)`: `decoy` = temp dir with `run/` inside; spawn `node --test --test-reporter=tap --test-name-pattern='^status says at a glance' test/machine.test.mjs` with `cwd: REPO` and `{ ...process.env, DEVTEAM_RUN: decoy/run, DEVTEAM_HOME: decoy, DEVTEAM_ROLE: 'worker', DEVTEAM_TICKET: 'x-1', DEVTEAM_PUBLIC_URL: 'https://box.example.com', TMUX: 'x', TMUX_PANE: '%9' }` minus `NODE_TEST_CONTEXT`; assert status 0; stdout matches `/^# pass 1$/m` and `/^# fail 0$/m`; exactly one `^ok \d+ - status says at a glance` line, without `# SKIP`; every other `^ok ` line carries `# SKIP test name does not match pattern` (how Node 18/20 report filtered tests; 22/24 print none); and a recursive listing of `decoy` is `['run']` (the empty folder only). - [ ] **Step 2: run them, see them fail** (`run-isolated.sh --test-name-pattern='importing the test helper|decoy' test/machine.test.mjs`): the env list holds `DEVTEAM_RUN` etc.; the decoy holds log files (the incident reproduced, against the decoy). Paste the failing listing into `reviews/task-2.md`'s notes or `verification.md`. -- [ ] **Step 2b: check done-when 1** with the spec's AC1 grep after Step 3: no match. +- [ ] **Step 2b: check done-when 1** with the spec's AC1 grep after Step 3: the only matches are machine.test.mjs:14 and operator-threads.test.mjs:19 (their `CLAUDECODE|CLAUDE_*` regex, now with no `DEVTEAM_` or `TMUX` in it) and gateway.test.mjs:92 (a header cleanup, `delete h[k]`); plus `grep -nE "'(DEVTEAM_[A-Z_]+|TMUX|TMUX_PANE)'\]\) delete|DEVTEAM_(ROLE|TICKET|FEATURE|RUN)\|" test/lib/kit.mjs test/{machine,operator-threads,hub-overview,gateway}.test.mjs` finds nothing. - [ ] **Step 3: implement** the scrub and the per-file removals above. - [ ] **Step 4: run them, see them pass.** - [ ] **Step 5: commit** `kit-201: one env scrub for every test that spawns the kit`; review into `reviews/task-2.md`. @@ -82,9 +84,10 @@ ### Task 3: the three files on the box, and the suite - [ ] **Step 1:** `run-isolated.sh --test-reporter=tap test/operator-threads.test.mjs test/machine.test.mjs test/hub-overview.test.mjs` on this box (both box files present) → `# fail 0` (baseline at cca0976: operator-threads 1 pass / 15 fail). -- [ ] **Step 2:** the same three with the session's own env (only `TMUX_TMPDIR` set to a fresh dir, through a second script `run-as-worker.sh` beside run-isolated.sh, which keeps `DEVTEAM_RUN` pointed at a decoy copy, never home's) → `# fail 0`, decoy empty. -- [ ] **Step 3:** `run-isolated.sh` on `test/gateway.test.mjs` and `test/kit.test.mjs` (fork path) → pass; then the full suite as test.yml runs it, through `devteam.mjs gate test`. +- [ ] **Step 2:** only after Task 2's decoy proof passes: the same three with a worker-shaped env (`run-as-worker.sh` beside run-isolated.sh: `DEVTEAM_RUN`, `DEVTEAM_HOME` set to a fresh decoy, `DEVTEAM_ROLE=worker`, `DEVTEAM_TICKET`, `DEVTEAM_PUBLIC_URL`, `TMUX`, `TMUX_PANE` set, never home's run) → `# fail 0`, decoy empty. The hub-overview tests on `fleetFixture` keep the real `HOME`, so the off switch is ignored for them; they passed on this box at main and must still. +- [ ] **Step 3:** `run-isolated.sh` on `test/gateway.test.mjs` and `test/kit.test.mjs` (fork path) → pass; then the full suite as test.yml runs it, as `devteam.mjs gate test -- `: the gate itself runs with the session env and records into home's run (that is its job), the suite under it runs scrubbed. - [ ] **Step 4:** push; test.yml green on every job; `verification.md` with a row per done-when at the commit. +- [ ] **Step 5:** the PR body has the ruling's line, verbatim in substance: "Not defended: an agent that sets HOME to a temp folder on purpose (and copies the store there) gets the laptop's unsigned behaviour, for operator threads in that copy and for pane answers into the real project's log and pane (P6)." And a note: `test/lib/ports.mjs:22` still says the gateway test claims no block (outside touches; now it claims one). ## Rulings diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/plan-1.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/plan-1.md new file mode 100644 index 0000000..e26a7fc --- /dev/null +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/plan-1.md @@ -0,0 +1,20 @@ +# Plan review, pass 1 + +- Reviewer: plan-reviewer (fresh subagent, claude-opus) +- Reviewed: 945f4765e4e767e215bac406e8fec15b01de4200 + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| P1 | Important | done-when 2, 4; spec AC2 | plan.md Task 2 Step 1 (decoy test); test.yml matrix `node: [18, 20, 22, 24]` | The decoy test's TAP assertions can't pass as written, on any Node. node's TAP summary line is `# skipped N`, not `# skip N`, so `/^# skip 0$/m` never matches. On Node 18 and 20, which test.yml runs, tests that `--test-name-pattern` filters out still appear as `ok N - … # SKIP test name does not match pattern` and are counted, so `# skipped 0` fails there too (machine.test.mjs has ~17 tests). I checked this with a 3-test file under 18.20.4 and 20.19.6 (`# pass 1`, `# skipped 2`) and under 22.23.4 and 24.22.3 (`# pass 1`, `# skipped 0`). The check still has to fail on a skipped or unmatched incident test (spec AC2, spec-1 F2). One way to do that on every major: assert `# pass 1` and `# fail 0`, assert exactly one `^ok \d+ - status says at a glance` line with no `# SKIP`, and assert every other `ok` line carries `# SKIP test name does not match pattern`. | fixed: assert pass 1, fail 0, one unskipped ok line for the incident test, every other ok line a name-pattern skip | +| P2 | Important | spec AC2 (second check); done-when 4 | plan.md Task 2 Step 1 (env-names test) | The expected list is exact, but the child's env is "carrying" the listed variables, which in practice means `{ ...process.env, … }`. CI's `rest` job sets `DEVTEAM_TEST_GITLEAKS: '1'` (test.yml), and HARNESS_ENV keeps it, so the printed list gains `DEVTEAM_TEST_GITLEAKS` and the test fails in CI. The same happens on a developer's run with `DEVTEAM_TEST_COLD`, `DEVTEAM_FORK_AUDIT` or `DEVTEAM_G1_RECORD` set. The fix is either of two things. Build the child env from `process.env` minus every HARNESS_ENV key, then add the listed ones. Or compare against the expected list plus the harness names the parent already holds. | fixed: DEVTEAM_TEST_* names are left out of the exact comparison; TEST_KEEP asserted present | +| P3 | Important | done-when 1; spec AC1 check | plan.md Task 2 Step 2b; test/machine.test.mjs:14; test/operator-threads.test.mjs:19; test/gateway.test.mjs:92 | Step 2b expects the spec's AC1 grep to find "no match" after Step 3. It will still match three lines. machine:14 and operator-threads:19 are kept for `CLAUDECODE`/`CLAUDE_*` and still end in `delete process.env[k]`, which `delete [a-zA-Z.]*\[k\]` matches. gateway:92 is an unrelated header cleanup, `delete h[k]`. The planned check gives the wrong result, so the done-when 1 evidence would be fudged, or the worker would strip the CLAUDE_* handling, which is out of scope. The plan should name the expected residual matches, or ask the supervisor for a tighter grep, for example one that also requires `DEVTEAM_\|TMUX` on the line. | fixed: Step 2b names the three expected non-denylist matches and adds a zero-hit grep | +| P4 | Minor | done-when 1 ("one shared helper"); spec AC1 ("the scrub is the only one") | test/lib/kit.mjs:95 (`fleetFixture`) | `fleetFixture`'s own denylist (`DEVTEAM_ROLE/TICKET/FEATURE/RUN`, `TMUX`, `TMUX_PANE`) stays. After the scrub it is dead and is a second denylist. kit.mjs is in touches, so remove it in Task 2, or say why it stays. | fixed: fleetFixture's denylist at kit.mjs:95 goes too | +| P5 | Minor | spec AC2 | plan.md Task 2 Step 1 (decoy spawn) | The child is spawned on the relative `test/machine.test.mjs` and no `cwd` is given. Use `path.join(testDir, 'machine.test.mjs')`, or pass `cwd: path.resolve(testDir, '..')`, so the proof doesn't depend on where the parent runs. | fixed: cwd REPO | +| P6 | Minor | spec AC5 | plan.md Task 1 Interfaces (`boxFns`), Step 1 | The `files` shape contradicts itself. The signature defaults it to `[]`, but the text says it is `{ [path]: text }`, and Step 1 says "`files` = both box paths". Settle on the object form, default `{}`, and have Step 1 pass each box path with its content (the key's PEM read from `GW_PUBLIC`, which is a path, not text). | fixed: files is { [boxPath]: text } throughout | +| P7 | Minor | spec Rulings (d-20261114-215844973-supervisor: "The PR body says in one line what it doesn't defend against") | plan.md Task 3 Step 4 | No step writes the PR body line the ruling requires. Add it to Step 4. Name the ports.mjs:22 comment ("The gateway test claims no block") there as well, which the spec's Edge cases say is "left and noted" because ports.mjs isn't in touches. | fixed: Task 3 Step 5 writes the PR line and the ports.mjs note | +| P8 | Minor | plan Global Constraints; ruling m-20261114-215602684-supervisor | plan.md Task 3 Steps 2-3 | Two steps conflict with the Global Constraint that every test run goes through run-isolated.sh. Step 3's `devteam.mjs gate test` runs the whole suite under `gateEnv()` (devteam.mjs:4375), that is `{ ...process.env, DEVTEAM_HOME: mainRoot, … }` with the live `DEVTEAM_RUN`. Step 2 adds `run-as-worker.sh` for "the session's own env" but keeps `DEVTEAM_RUN` at a "decoy copy", which it never defines. Both are reasonable as the end-to-end check once the decoy proof is green, but the plan should state them as deliberate exceptions, run only after Task 2's proof passes. It should also say exactly what `run-as-worker.sh` sets and unsets, so it can't hand a test the live log. | fixed: run-as-worker uses a decoy run only after the proof passes; gate runs run-isolated.sh under it | +| P9 | Suggestion | done-when 3 | test/hub-overview.test.mjs:15, 86; test/lib/kit.mjs:94 | Only the Chrome test gets a temp `HOME`. hub-overview's `fleetFixture` tests run with the real `HOME`, so on the box `DEVTEAM_BOX=off` is ignored there and the box files still win. They look box-insensitive (`hello` pulse), and Task 3 Step 1 would catch it, but the plan could say so. | fixed: noted in Task 3 Step 2 (they keep the real HOME and must still pass) | + +Checked and holding. Task 1's vm block evaluates. The cut code (`previewBox` plus `onBox … gatewayThreadKey`, including `threadKeyMemo` and `OPERATOR_PROOF_MS`) needs only `fs`, `os`, `crypto`, `process` and `URL`, and the plan supplies all five. `BOX_GATEWAY_FILE` is declared after `onBox`, which is safe because it is read only at call time. A fresh context per `boxFns` call keeps `threadKeyMemo` from going stale. The stand-in `existsSync` answers for the two box paths, so the real `/opt` and `/etc` files on this box don't leak. `/* box start */` is not a substring of `/* preview box start */`, and no marker exists yet. Task 2's expected names match what kit.mjs and its imports leave: ports.mjs and cli.mjs set no env, and `cli.mjs`'s `machineless` only builds per-call env. ESM evaluates kit.mjs's imports before its body, but neither one reads `DEVTEAM_*` at top level. `DEVTEAM_FORK` and `DEVTEAM_TEST_KEEP` survive for lib/fork.mjs. Dropping the per-file loops loses nothing the scrub doesn't cover. Deleting `NODE_TEST_CONTEXT` is right. TL;DR matches the tasks (quality §6). + +Verdict: CHANGES 945f4765e4e767e215bac406e8fec15b01de4200 diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-2.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-2.md new file mode 100644 index 0000000..6152d59 --- /dev/null +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-2.md @@ -0,0 +1,27 @@ +# Spec review, pass 2 + +- Reviewer: spec-confirmer (fresh subagent, claude-opus) +- Reviewed: 945f4765e4e767e215bac406e8fec15b01de4200 + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| F2-1 | Minor | spec AC2 | spec AC2 ("`# pass 1`, `# skip 0`, `# fail 0`"); .github/workflows/test.yml:14 | Node's TAP summary line is `# skipped N`, not `# skip N`, so the literal as written never matches. Also, from Node 23 on (CI runs 24) the default reporter is spec even when stdout isn't a TTY, so the child prints `ℹ pass 1` and no TAP at all. Name `--test-reporter=tap` for the child (Node 18.15+ has it) and use the real summary keys (`# pass 1`, `# fail 0`, `# skipped 0`). | | +| F2-2 | Minor | spec AC1 check | test/machine.test.mjs:14; test/operator-threads.test.mjs:19; test/gateway.test.mjs:92 | The widened grep's `delete [a-zA-Z.]*\[k\]` branch still matches lines that stay after the fix: the kept `CLAUDECODE|CLAUDE_*` strips (`delete process.env[k]`, machine:14, operator-threads:19) and gateway:92's unrelated `delete h[k]` (dropping undefined headers). So "finds no denylist" is not a zero-hit check. Either list the three hits expected to remain or tighten the pattern, e.g. require a `DEVTEAM_`/`TMUX` literal on the same line. | | +| F2-3 | Minor | P6; spec "The off switch's limit" | spec "Approaches considered", A and B | The Approaches section wasn't touched and still argues the old premise: B "the real store stays guarded", and A "reopens the hole". The restated limit says B stops only the accidental path and leaves pane answers into the real project log and pane undefended. Align the two bullets with the limit section so the rationale the spec keeps is the true one. | | +| F2-4 | Suggestion | (none) | spec done-when 5; docs/devteam/scripts/devteam.mjs:6618, 13004-13030 | The block to cut for the vm test isn't self-contained. `onBox()` calls `previewBox()` (l.6618, far outside it), `gatewayThreadKey()` uses `crypto`, and unrelated consts (`OPERATOR_PROOF_MS`, the threadKey memo) sit between the box lines. Say that the vm context supplies `previewBox` (or an env-only stand-in) and `crypto`, or that the box lines are first moved together. | | + +**F1 (Important, the false premise and the passable HOME guard): confirmed fixed.** The limit section now says what B does: it stops a leaked `DEVTEAM_BOX=off` at the account's own home and nothing more. It names the pane-answer path (`/api/pane-answer`, `pane answered`) and the project log as left undefended, and puts that one line in the PR body, as P6 and the ruling ask. The guard now compares `fs.realpathSync` of `$HOME` and of `os.userInfo().homedir`, which closes the trailing-slash, `/.` and symlink bypasses, and done-when 5 tests the symlink case. R1 holds: normal use, meaning a leaked test env or a confused agent at its real home, can't reach the unsigned path. The remaining stale wording in Approaches is F2-3 (Minor). + +**F2 (Important, a proof that can pass vacuously): confirmed fixed.** AC2 now requires exactly one test to run and pass with none skipped or failed, so a skipped incident test or a name pattern that matches nothing fails the proof. The decoy is a `DEVTEAM_RUN` folder the pre-fix kit really writes into, it must hold only its empty `run/` afterwards, and a recorded pre-fix run must fail (red before green). A tmux-less host skips the test, but CI installs tmux (test.yml:31), so the proof runs there. The literal summary keys and the reporter flag are F2-1 (Minor). + +**F3 (Minor, denylists the check missed): confirmed fixed in substance.** Gateway's loops at l.723 and l.764, machine:14's regex and operator-threads:19, 67 and 626 are each named with what happens to them, and the grep now catches every listed denylist. It over-matches on lines that remain (F2-2, Minor). + +**F4 (Minor, `os.userInfo()` throwing): confirmed fixed.** A throw from `userInfo()` or from realpath, including an unresolvable `HOME`, ignores the switch and fails closed. Done-when 5's matrix covers the HOME cases. The spec says this case is "tested" but the matrix doesn't list the throw explicitly; the plan should carry it. + +**F5 (Minor, side effects of importing lib/kit.mjs into gateway.test.mjs): confirmed fixed.** The new Edge-cases bullet names the port block and the comments to update, the tmux shim on PATH and why refusal is what the real-hub test wants, and the three `off` knobs. It also notes that gateway's own `DEVTEAM_HUB_PORT`/`PORT_BASE` settings run after the import and win. Gateway's box tests set `HOME` to a temp folder (gateway.test.mjs:722, 763), so `DEVTEAM_BOX=off` is honored there. + +**F6 (Suggestion, `HARNESS_ENV` as a glob vs a list): confirmed fixed.** `HARNESS_ENV` names its five members and the `DEVTEAM_TEST_` prefix, with today's four switches listed. + +Values and refusals for the new text: P6 is satisfied (the simpler guard stops the accidental path and names in one line what it leaves undefended). R1 holds (see F1). Nothing in the diff crosses a refusal. No new Critical or Important problem found. + +Verdict: APPROVE 945f4765e4e767e215bac406e8fec15b01de4200 From 96b3fce07c8761f80a8ce408d4b1e992068fc4bb Mon Sep 17 00:00:00 2001 From: Hugh Francis Date: Sat, 14 Nov 2026 22:12:23 +0000 Subject: [PATCH 04/22] kit-201: plan review pass 2 Co-Authored-By: Claude Opus 5.5 --- .../reviews/plan-2.md | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/plan-2.md diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/plan-2.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/plan-2.md new file mode 100644 index 0000000..05747c8 --- /dev/null +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/plan-2.md @@ -0,0 +1,31 @@ +# Plan review, pass 2 + +- Reviewer: plan-confirmer (fresh subagent, claude-opus) +- Reviewed: bfe8b65ff4f2c606fae8c1cf5ca71c4eea48d53e + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| P2-1 | Minor | done-when 4 | plan.md Task 2 Step 1 (env-names test); Task 2 Interfaces `HARNESS_ENV` | The exact comparison leaves out only `DEVTEAM_TEST_*`. `HARNESS_ENV` also keeps `DEVTEAM_FORK_AUDIT` and `DEVTEAM_G1_RECORD`, which pass-1 P2 named, so a developer who has either set gets a false failure. CI doesn't set them (test.yml:53 sets only `DEVTEAM_TEST_GITLEAKS`), which is why this is Minor. The fix is to leave out every `HARNESS_ENV` name the parent holds and the test doesn't set, not only `DEVTEAM_TEST_*`. | | +| P2-2 | Minor | done-when 1 | plan.md Task 2 Step 2b (zero-hit grep); Task 2 Interfaces (the scrub deletes "`TMUX`, `TMUX_PANE`") | The new grep includes `test/lib/kit.mjs`. Its first branch, `'(…\|TMUX\|TMUX_PANE)'\]\) delete`, matches the scrub's own tmux removal if that is written in the files' idiom, `for (const k of ['TMUX', 'TMUX_PANE']) delete process.env[k]`. "Finds nothing" would then fail against the intended code. Either name that line as an expected hit or say the scrub's tmux removal is written in another form. Today the grep finds all nine denylists (I checked at bfe8b65), so it is not vacuous. | | + +**P1 (Important, decoy TAP assertions): fixed.** The new assertions hold on every Node major: `# pass 1`, `# fail 0`, exactly one unskipped `ok … - status says at a glance` line, and every other top-level `ok` line a name-pattern skip. The incident test is `{ skip: !hasTmux }` (machine.test.mjs:255), so a tmux skip would print `# SKIP` on that line and fail the check, as AC2 wants. `--test-reporter=tap` is named, which covers spec-2 F2-1's Node 23+ default-reporter point. The Edge-cases line and the Readings paragraph say the same thing. + +**P2 (Important, CI's `DEVTEAM_TEST_GITLEAKS`): fixed for CI.** `DEVTEAM_TEST_*` names are left out of the comparison, and `DEVTEAM_TEST_KEEP` is asserted present. A developer with other harness knobs set still gets a false failure; that residue is P2-1 (Minor). + +**P3 (Important, AC1 grep not zero-hit): fixed.** Step 2b names the three residual matches (machine:14, operator-threads:19, gateway:92), which I confirmed with the grep. It adds a tighter grep that today catches every denylist, including kit.mjs:95. One self-match risk on the new scrub remains; that is P2-2 (Minor). + +**P4 (Minor, fleetFixture denylist): fixed.** kit.mjs:95 is in Task 2's Modify list. It keeps its own `TMUX_TMPDIR`. + +**P5 (Minor, cwd): fixed.** The spawn now has `cwd: REPO`. + +**P6 (Minor, `files` shape): fixed.** `files = {}` is `{ [boxPath]: text }` throughout, with an example. The stand-ins hold the caller's text, and Step 1 passes `GW_PUBLIC`'s text. + +**P7 (Minor, PR body line): fixed.** Task 3 Step 5 gives the ruling's line and the ports.mjs:22 note. + +**P8 (Minor, runs outside run-isolated.sh): fixed.** run-as-worker.sh runs only after Task 2's proof passes, and Step 2 says exactly what it sets: a fresh decoy `DEVTEAM_RUN`/`DEVTEAM_HOME`, never home's run. The gate now wraps run-isolated.sh: `gate test -- ` exists (devteam.mjs `cmdGate`, execFileSync with inherited env), so only the gate record goes to home's run, and the plan says that is deliberate. The script lives under the ticket folder, which the "no file added" constraint exempts. + +**P9 (Suggestion, hub-overview real HOME): addressed** in Task 3 Step 2. + +Spec-2 minor notes, carried as Readings: F2-1 is in Task 2 (reporter and keys), F2-2 in Step 2b, F2-3 is read as superseded by the spec's limit section, and pass-1 confirmed that Task 1's vm block covers F2-4. I found no new Critical or Important problem in the diff. + +Verdict: APPROVE bfe8b65ff4f2c606fae8c1cf5ca71c4eea48d53e From f0e6cdc8e7c0f702c5ccccbc27f3d1cc1f926398 Mon Sep 17 00:00:00 2001 From: Hugh Francis Date: Sat, 14 Nov 2026 22:13:40 +0000 Subject: [PATCH 05/22] kit-201: DEVTEAM_BOX=off, honored only off the account's own home The box detection (onBox, threadsNeedGateway, gatewayThreadKey) is marked as a block and loaded by test/lib/kit.mjs's boxFns on a machine of the test's making, so the limit is tested in CI where no box file exists. Co-Authored-By: Claude Opus 5.5 --- docs/devteam/scripts/devteam.mjs | 21 +++++++++++++--- test/lib/kit.mjs | 16 ++++++++++++ test/operator-threads.test.mjs | 43 +++++++++++++++++++++++++++++++- 3 files changed, 76 insertions(+), 4 deletions(-) diff --git a/docs/devteam/scripts/devteam.mjs b/docs/devteam/scripts/devteam.mjs index 5c2070d..90ae815 100644 --- a/docs/devteam/scripts/devteam.mjs +++ b/docs/devteam/scripts/devteam.mjs @@ -6615,9 +6615,11 @@ async function previewClaim(session, label) { const probePort = async (port) => { try { return (await answers(port, '127.0.0.1')) || (await answers(port, '::1')); } catch { return false; } }; // The box's address when this machine has one, and whether its gateway serves previews: only the // release that does sets DEVTEAM_PREVIEWS, so the kit never prints a link that would answer 421. +/* preview box start */ function previewBox() { try { return process.env.DEVTEAM_PUBLIC_URL ? new URL(process.env.DEVTEAM_PUBLIC_URL) : null; } catch { return null; } } +/* preview box end */ const previewServed = () => !!previewBox() && process.env.DEVTEAM_PREVIEWS === '1'; const previewLink = (r) => { const b = previewBox(); return previewServed() ? `${b.protocol}//${r.name}.${b.host}` : `http://127.0.0.1:${r.port}`; }; function previewSay(r, up) { @@ -12999,9 +13001,10 @@ const OPERATOR_WINDOW = 'operator'; const OPERATOR_THREAD_ID = /^ot-\d{8}-\d{9}-[0-9a-f]{6}$/; const OPERATOR_SUBJECT_MAX = 1200; // bytes, as encoded in the line const OPERATOR_STARTING_MS = 60000; +/* box start */ // The image's own file says box too: an agent can unset DEVTEAM_PUBLIC_URL for a dashboard it starts, // and that dashboard's hub would then keep threads elsewhere and start an operator of its own. -const onBox = () => !!previewBox() || fs.existsSync(BOX_GATEWAY_FILE); +const onBox = () => !!previewBox() || boxFile(BOX_GATEWAY_FILE); // On a box the owner is never on the machine: they reach the hub only through the gateway, behind // their passkey. And every agent runs as the hub's own Unix user (dev), so nothing a request says // about where it came from, Origin and Sec-Fetch-Site included, tells the owner's press from an @@ -13013,13 +13016,24 @@ const onBox = () => !!previewBox() || fs.existsSync(BOX_GATEWAY_FILE); // which an agent can change for a command it runs itself. On a laptop nothing is signed. const BOX_GATEWAY_FILE = '/opt/devteam/gateway.mjs'; const BOX_THREAD_KEY = '/etc/devteam/thread-key.pub'; +// DEVTEAM_BOX=off: a test run on a box ignores the box's files, so the kit under test sees only the +// box its environment describes, as on a CI runner. It holds only while HOME (by real path) isn't +// the account's own home, from the password file: every test that needs it runs on a throwaway +// HOME, and a switch leaked into a command run at the real home changes nothing. Anything that +// can't be resolved leaves the files on. Not defended (the agents are trusted not to forge): a +// command run with HOME set elsewhere on purpose gets a laptop's unsigned threads and pane answers. +const boxFilesOff = () => { + if (process.env.DEVTEAM_BOX !== 'off') return false; + try { return fs.realpathSync(os.homedir()) !== fs.realpathSync(os.userInfo().homedir); } catch { return false; } +}; +const boxFile = (f) => !boxFilesOff() && fs.existsSync(f); const OPERATOR_PROOF_MS = 120000; -const threadsNeedGateway = () => onBox() || fs.existsSync(BOX_GATEWAY_FILE); +const threadsNeedGateway = () => onBox() || boxFile(BOX_GATEWAY_FILE); // The public half: on a box only the root-owned file; elsewhere (a test of a box) the file the // environment names. null when there is none, and then nothing on a box is signed. let threadKeyMemo = { at: null, key: null }; function gatewayThreadKey() { - const f = fs.existsSync(BOX_GATEWAY_FILE) || fs.existsSync(BOX_THREAD_KEY) ? BOX_THREAD_KEY : process.env.DEVTEAM_GATEWAY_THREAD_KEY; + const f = boxFile(BOX_GATEWAY_FILE) || boxFile(BOX_THREAD_KEY) ? BOX_THREAD_KEY : process.env.DEVTEAM_GATEWAY_THREAD_KEY; if (!f) return null; let at; try { const st = fs.statSync(f); at = `${f}:${st.ino}:${st.mtimeMs}:${st.size}`; } catch { return null; } @@ -13029,6 +13043,7 @@ function gatewayThreadKey() { threadKeyMemo = { at, key }; return key; } +/* box end */ // What the gateway signs (gateway.mjs, hubSignature): the route, when, a nonce, and the body's hash. const gatewaySigned = (p, ts, nonce, body) => Buffer.from(`devteam-operator-post-v1\n${p}\n${ts}\n${nonce}\n${sha256(body)}`); // Why a press's proof isn't the gateway's (null when it is): { path, ts, nonce, sig, body (base64) }, diff --git a/test/lib/kit.mjs b/test/lib/kit.mjs index de7c0dc..d7e5e18 100644 --- a/test/lib/kit.mjs +++ b/test/lib/kit.mjs @@ -308,6 +308,22 @@ export function gitFactFns(cwd, runDir) { const fns = vm.runInContext(`${cut('/* git reads start */', '/* git reads end */')}\n${cut('/* git facts start */', '/* git facts end */')}\nHOME_CWD = ${JSON.stringify(cwd)};\n;({ commitDateOf, ancestorOf, releasedInAt, factGet })`, ctx); return { ...fns, exit: () => { for (const f of exits.splice(0)) f(); } }; } +// The script's box detection, loaded by itself on a machine of the test's making: the box's files +// exist only as files names them ({ path: text }), HOME is home, the password file's home is passwd +// (null: the uid has none, and userInfo throws), and the environment is { HOME, ...env }. +export function boxFns({ files = {}, home, passwd, env = {} }) { + const src = fs.readFileSync(path.join(kit, 'scripts', 'devteam.mjs'), 'utf8'); + const cut = (a, b) => { const i = src.indexOf(a), j = src.indexOf(b); assert.ok(i >= 0 && j > i, `${a} … ${b} marks a block in devteam.mjs`); return src.slice(i, j); }; + const BOX = ['/opt/devteam/gateway.mjs', '/etc/devteam/thread-key.pub']; + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'devteam-boxfns-')); + process.on('exit', () => fs.rmSync(dir, { recursive: true, force: true })); + const stand = Object.fromEntries(BOX.filter((f) => f in files).map((f, i) => { const p = path.join(dir, `box-${i}`); fs.writeFileSync(p, files[f]); return [f, p]; })); + const at = (f) => (BOX.includes(f) ? stand[f] ?? path.join(dir, 'none') : f); + const sandboxFs = { existsSync: (f) => fs.existsSync(at(f)), statSync: (f) => fs.statSync(at(f)), readFileSync: (f, e) => fs.readFileSync(at(f), e), realpathSync: (f) => fs.realpathSync(f) }; + const sandboxOs = { homedir: () => home, userInfo: () => { if (passwd === null) throw new Error('no passwd entry'); return { homedir: passwd }; } }; + const ctx = vm.createContext({ fs: sandboxFs, os: sandboxOs, crypto, URL, process: { env: { HOME: home, ...env } } }); + return vm.runInContext(`${cut('/* preview box start */', '/* preview box end */')}\n${cut('/* box start */', '/* box end */')}\n;({ onBox, threadsNeedGateway, gatewayThreadKey, boxFilesOff, BOX_GATEWAY_FILE, BOX_THREAD_KEY })`, ctx); +} // The script's ranked keyword search, loaded by itself. export function searchFns() { const src = fs.readFileSync(path.join(kit, 'scripts', 'devteam.mjs'), 'utf8'); diff --git a/test/operator-threads.test.mjs b/test/operator-threads.test.mjs index f326469..2d74f0f 100644 --- a/test/operator-threads.test.mjs +++ b/test/operator-threads.test.mjs @@ -10,7 +10,7 @@ import fs from 'node:fs'; import http from 'node:http'; import os from 'node:os'; import path from 'node:path'; -import { PORTS, bareProject, hasTmux, kit, startDashboard, until, write } from './lib/kit.mjs'; +import { PORTS, bareProject, boxFns, hasTmux, kit, startDashboard, until, write } from './lib/kit.mjs'; import { canDrive, openChrome } from './lib/chrome.mjs'; import { hubSignature } from '../cloud/gateway.mjs'; @@ -109,6 +109,47 @@ function seat(M, screens = [box(prompt)]) { const shown = (pane, text) => until(() => tmuxSh(['capture-pane', '-p', '-t', pane]).stdout.includes(text), `pane ${pane} drawn`); const SUBJECT = (root, selector = 'inbox', label = 'Inbox 3') => ({ kind: 'overview', root, name: path.basename(root), anchor: { selector, label } }); +// A test on a box turns the box's files off (DEVTEAM_BOX=off, lib/kit.mjs), and the kit takes that only +// for a throwaway HOME: at the account's own home, however it is spelled, a box stays a box. +test('DEVTEAM_BOX=off turns the box\'s files off only for a throwaway home: at the account\'s own home a box stays a box', () => { + const passwd = path.join(tmp, 'box-home'); + const link = path.join(tmp, 'box-home-link'); + const tmpHome = path.join(tmp, 'box-tmp-home'); + fs.mkdirSync(passwd); + fs.mkdirSync(tmpHome); + fs.symlinkSync(passwd, link); + const pem = (f) => crypto.createPublicKey(fs.readFileSync(f, 'utf8')).export({ type: 'spki', format: 'pem' }); + const envKey = path.join(tmp, 'box-env-key.pub'); + fs.writeFileSync(envKey, crypto.createPublicKey(otherKey).export({ type: 'spki', format: 'pem' })); + const files = { '/opt/devteam/gateway.mjs': '', '/etc/devteam/thread-key.pub': fs.readFileSync(GW_PUBLIC, 'utf8') }; + const keyOf = (B) => B.gatewayThreadKey()?.export({ type: 'spki', format: 'pem' }) ?? null; + const off = { DEVTEAM_BOX: 'off', DEVTEAM_GATEWAY_THREAD_KEY: envKey }; + const stays = (B, why) => { + assert.equal(B.boxFilesOff(), false, why); + assert.equal(B.onBox(), true, why); + assert.equal(B.threadsNeedGateway(), true, why); + assert.equal(keyOf(B), pem(GW_PUBLIC), `${why}: the box's own key`); + }; + stays(boxFns({ files, home: passwd, passwd, env: off }), 'HOME is the passwd home'); + stays(boxFns({ files, home: link, passwd, env: off }), 'HOME is a symlink to it'); + stays(boxFns({ files, home: `${passwd}/`, passwd, env: off }), 'HOME is it, with a trailing slash'); + stays(boxFns({ files, home: path.join(tmpHome, 'missing'), passwd, env: off }), 'HOME names nothing'); + stays(boxFns({ files, home: tmpHome, passwd: null, env: off }), 'no passwd entry'); + for (const v of [undefined, 'on', 'OFF', '']) stays(boxFns({ files, home: tmpHome, passwd, env: { DEVTEAM_GATEWAY_THREAD_KEY: envKey, ...(v === undefined ? {} : { DEVTEAM_BOX: v }) } }), `DEVTEAM_BOX=${v}`); + const T = boxFns({ files, home: tmpHome, passwd, env: off }); + assert.equal(T.boxFilesOff(), true, 'a throwaway HOME'); + assert.equal(T.onBox(), false, 'no box but the environment\'s'); + assert.equal(T.threadsNeedGateway(), false); + assert.equal(keyOf(T), pem(envKey), 'the environment\'s key, not the box\'s'); + const U = boxFns({ files, home: tmpHome, passwd, env: { ...off, DEVTEAM_PUBLIC_URL: 'https://box.example.com' } }); + assert.equal(U.onBox(), true, 'a box by its environment'); + assert.equal(keyOf(U), pem(envKey)); + const L = boxFns({ home: passwd, passwd }); + assert.equal(L.onBox(), false, 'a laptop'); + assert.equal(L.threadsNeedGateway(), false); + assert.equal(keyOf(L), null); +}); + test('the store: a thread, its turns, closed and opened again, kept beside the operator in no repo', () => { const M = machine({ starts: false }); const said = M.ok(['new', '--title=alpha: Inbox 3', `--subject=${JSON.stringify(SUBJECT('/code/alpha'))}`, '--', 'Why three?']).split('\n'); From 775d5e914ef47bc3522b965e8a40c0fee317cd39 Mon Sep 17 00:00:00 2001 From: Hugh Francis Date: Sat, 14 Nov 2026 22:16:07 +0000 Subject: [PATCH 06/22] kit-201: task 1 review Co-Authored-By: Claude Opus 5.5 --- .../reviews/task-1.md | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/task-1.md diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/task-1.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/task-1.md new file mode 100644 index 0000000..d000f01 --- /dev/null +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/task-1.md @@ -0,0 +1,31 @@ +# Task 1 review, pass 1 + +- Reviewer: task-reviewer (fresh subagent, claude-opus) +- Reviewed: f0e6cdc8e7c0f702c5ccccbc27f3d1cc1f926398 + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| T1-1 | Suggestion | none | test/lib/kit.mjs boxFns | Every `boxFns()` call adds its own `process.on('exit')` listener and its own temp dir. The DEVTEAM_BOX=off test calls it 12 times, so each run prints `MaxListenersExceededWarning: 11 exit listeners` (seen in the TAP of the base run). One shared dir, or one listener the module registers once, would stop the noise. Nothing fails because of it. | fixed in Task 2: one temp dir and one exit listener per process | +| T1-2 | Suggestion | spec "The off switch's limit" (P6) | devteam.mjs:13025 boxFilesOff | A realpath comparison can't see a second path to the same directory, such as a bind mount or another mount of the volume, so a HOME spelled that way would count as a throwaway home. The spec asks for realpath and only for the accidental path, and nothing on the box mounts /data/home twice, so this isn't a defect. Comparing `statSync().dev`/`.ino` would close that gap too, if it ever matters. | wont-fix: the spec pins a real-path compare; nothing on the box mounts the home twice | + +No Critical or Important findings. Checked: + +- **Unset or other values.** When `DEVTEAM_BOX` is anything but exactly `off`, `boxFile(f)` is `fs.existsSync(f)`, so `onBox`, `threadsNeedGateway` and `gatewayThreadKey` work exactly as on main. The test covers unset, `on`, `OFF` and empty. +- **Fails closed.** If either realpath throws, or `userInfo()` throws, `boxFilesOff()` is false (L-SEC-2). +- **Real HOME and passwd home.** On this box both are `/data/home`, and the Dockerfile's `as_dev` sets `HOME=/data/home`, so the switch is never honored at the real home. Node's `os.homedir()` reads `$HOME`, and `os.userInfo().homedir` reads the passwd file. When HOME is unset, `os.homedir()` falls back to the passwd home, which compares equal. A relative HOME resolves to its real path. +- **Nothing reads or sets the switch outside the test.** `git grep DEVTEAM_BOX` over docs/, cloud/, test/ and .github/ finds only devteam.mjs's `boxFilesOff` and its comment, plus the new test. The only other hit is cloud/Dockerfile's `DEVTEAM_BOX_HOLD_SECONDS`, which is a different variable. Nothing else in devteam.mjs reads the two box paths. +- **No real box files reach the test.** The stand-in `fs` sends both box paths to temp files the loader writes, or to a missing path when a file isn't listed. The block reads no other path except the key file the test's environment names. So the real /opt and /etc files on this box don't leak in: the laptop case `L` passes here, where both real files exist. +- **Spec acceptance check 5.** The markers, the vm loader with stand-in fs, os and env, and the test cases (passwd home, symlink, trailing slash, missing HOME, no passwd entry, temp HOME with the environment's key, DEVTEAM_PUBLIC_URL, unset and `on`) all match the spec and plan Task 1. `node --check` passes. + +Mutations: each one ran in a `git archive` copy of f0e6cdc under the scratchpad, with run-isolated.sh copied in, using `--test-reporter=tap --test-name-pattern='DEVTEAM_BOX=off' test/operator-threads.test.mjs`. The unmutated copy passes (`# pass 1`, `# fail 0`). Every mutation fails the test. + +| Mutation | Failed at | +|---|---| +| Drop `realpathSync` and compare the raw strings | 'HOME is a symlink to it' | +| Make the catch return `true` (fail open) | 'HOME names nothing' | +| `boxFile` ignores the switch | the throwaway-HOME assertions | +| Drop the home check (`return DEVTEAM_BOX === 'off'`) | 'HOME is the passwd home' | +| `gatewayThreadKey` uses `fs.existsSync(BOX_GATEWAY_FILE)` directly | the env-key assertion | +| Accept the value case-insensitively | 'DEVTEAM_BOX=OFF' | + +Verdict: APPROVE f0e6cdc8e7c0f702c5ccccbc27f3d1cc1f926398 From d13898bb5997082025927a064a46189cb39d6674 Mon Sep 17 00:00:00 2001 From: Hugh Francis Date: Sat, 14 Nov 2026 22:20:56 +0000 Subject: [PATCH 07/22] kit-201: one env scrub for every test that spawns the kit test/lib/kit.mjs deletes every DEVTEAM_* but the harness's own knobs (HARNESS_ENV), and TMUX/TMUX_PANE, at import, and sets DEVTEAM_BOX=off; the per-file denylists go, gateway.test.mjs imports the helper, hub-overview's kit gets a temp HOME. A decoy test runs the incident's own test under a worker's env and finds nothing written. Co-Authored-By: Claude Opus 5.5 --- test/gateway.test.mjs | 6 +++--- test/hub-overview.test.mjs | 3 +-- test/lib/kit.mjs | 22 ++++++++++++-------- test/machine.test.mjs | 38 +++++++++++++++++++++++++++++++--- test/operator-threads.test.mjs | 4 +--- 5 files changed, 53 insertions(+), 20 deletions(-) diff --git a/test/gateway.test.mjs b/test/gateway.test.mjs index 01e9765..c97b988 100644 --- a/test/gateway.test.mjs +++ b/test/gateway.test.mjs @@ -12,10 +12,12 @@ import os from 'node:os'; import path from 'node:path'; import vm from 'node:vm'; import { fileURLToPath } from 'node:url'; +// the suite's shared env scrub, tmux shim and stand-ins (it also claims a port block, unused here) +import './lib/kit.mjs'; const here = path.dirname(fileURLToPath(import.meta.url)); const kit = path.resolve(here, '..', 'docs', 'devteam'); -// a block of its own, apart from kit.test.mjs's (20000 and up) +// ports of its own, apart from the blocks lib/kit.mjs hands out (20000 and up); set after its import, so these win const PORTS = 10000 + (process.pid % 90) * 100; const HUB = PORTS; const P = PORTS + 1; @@ -720,7 +722,6 @@ test('the owner\'s threads with the operator reach the hub through the box\'s na const x = seen.at(-1); const proof = { path: x.url, ts: x.headers['x-devteam-gateway-ts'], nonce: x.headers['x-devteam-gateway-nonce'], sig: x.headers['x-devteam-gateway-sig'], body: Buffer.from(x.body).toString('base64') }; const kitEnv = { ...process.env, HOME: box, DEVTEAM_PUBLIC_URL: 'https://box.example.com', DEVTEAM_GATEWAY_THREAD_KEY: path.join(box, 'thread-key.pub'), DEVTEAM_OPERATOR_COMMAND: '', DEVTEAM_THREAD_PROOF: 'stdin' }; - for (const k of ['DEVTEAM_ROLE', 'DEVTEAM_TICKET', 'DEVTEAM_FEATURE', 'TMUX', 'TMUX_PANE']) delete kitEnv[k]; const kitRun = (input) => spawnSync(process.execPath, [path.join(kit, 'scripts', 'devteam.mjs'), 'operator-thread', 'new', '--', 'hello from the gateway'], { cwd: box, encoding: 'utf8', env: kitEnv, input }); const took = kitRun(JSON.stringify(proof)); assert.equal(took.status, 0, took.stderr); @@ -761,7 +762,6 @@ test('the gateway to a real hub on a box: a signed press is kept as the owner\'s // a box as its hub sees one: the public URL, the gateway's public key, and a tmux with no server // (so nothing on this machine is ever typed into) const env = { ...process.env, HOME: home, TMUX_TMPDIR: noTmux, DEVTEAM_PUBLIC_URL: 'https://box.example.com', DEVTEAM_GATEWAY_THREAD_KEY: keyFile }; - for (const k of ['DEVTEAM_ROLE', 'DEVTEAM_TICKET', 'DEVTEAM_FEATURE', 'TMUX', 'TMUX_PANE']) delete env[k]; const d = spawn(process.execPath, [path.join(A, 'docs/devteam/scripts/devteam.mjs'), 'dashboard'], { cwd: A, env, stdio: ['ignore', 'pipe', 'pipe'] }); let out = ''; d.stdout.on('data', (b) => { out += b; }); diff --git a/test/hub-overview.test.mjs b/test/hub-overview.test.mjs index 8dbcf53..d46e8c7 100644 --- a/test/hub-overview.test.mjs +++ b/test/hub-overview.test.mjs @@ -129,8 +129,7 @@ test('the Overview in Chrome: Park by keyboard, Park all and Resume all through t.after(() => fs.rmSync(base, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 })); const HUB = PORTS + 60; const P = PORTS + 61; - const env = { ...process.env, DEVTEAM_HUB_PORT: String(HUB), DEVTEAM_PORT_BASE: String(P), DEVTEAM_HUB_RETRY_MS: '500', CLAUDE_CONFIG_DIR: path.join(base, 'claude'), CODEX_HOME: path.join(base, 'codex') }; - for (const k of ['DEVTEAM_ROLE', 'DEVTEAM_TICKET', 'DEVTEAM_FEATURE', 'DEVTEAM_RUN', 'DEVTEAM_SUPERVISOR', 'TMUX', 'TMUX_PANE']) delete env[k]; + const env = { ...process.env, HOME: base, DEVTEAM_HUB_PORT: String(HUB), DEVTEAM_PORT_BASE: String(P), DEVTEAM_HUB_RETRY_MS: '500', CLAUDE_CONFIG_DIR: path.join(base, 'claude'), CODEX_HOME: path.join(base, 'codex') }; const A = bareProject(base, 'alpha'); const B = bareProject(base, 'beta'); for (const r of [A, B]) execFileSync('sh', ['-c', 'git add -A && git -c user.email=t@t -c user.name=t commit -qm init'], { cwd: r }); diff --git a/test/lib/kit.mjs b/test/lib/kit.mjs index d7e5e18..04c9de0 100644 --- a/test/lib/kit.mjs +++ b/test/lib/kit.mjs @@ -19,6 +19,14 @@ export const kit = path.resolve(testDir, '..', 'docs', 'devteam'); // The tmux session a project's workers live in: devteam--. export function sessionOf(root, name) { return `devteam-${name}-${crypto.createHash('sha1').update(fs.realpathSync(root)).digest('hex').slice(0, 6)}`; } export const line = (o) => `${JSON.stringify({ v: 1, ...o })}\n`; +// The suite's own switches, the only DEVTEAM_* a test process keeps from the shell that ran it. +export const HARNESS_ENV = /^DEVTEAM_(TEST_.*|FORK|FORK_AUDIT|G1_RECORD|CGROUP|MEMINFO)$/; +// Nothing of the session that runs the tests reaches the kits under test: a worker's DEVTEAM_RUN once +// sent a test's writes into its fleet's live log, and a box's DEVTEAM_PUBLIC_URL makes every kit a box. +for (const k of Object.keys(process.env)) if ((/^DEVTEAM_/.test(k) && !HARNESS_ENV.test(k)) || /^TMUX(_PANE)?$/.test(k)) delete process.env[k]; +// On a box, its files say box whatever a test's environment says; the kit drops them for a test's +// throwaway HOME (devteam.mjs, boxFilesOff), so a test sees the box its environment describes. +process.env.DEVTEAM_BOX = 'off'; // A test never rotates the machine's real Claude accounts: a test that wants cswap sets a stand-in. process.env.DEVTEAM_CSWAP = 'off'; // A fixture's stand-in supervisor runs under whatever agent runs the tests; it isn't that agent. @@ -34,10 +42,6 @@ process.env.DEVTEAM_MEMINFO ??= path.join(os.tmpdir(), 'dt-no-such-machine'); // is removed only as the process exits, after its server. export const tmuxRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'dt-tmux-')); process.env.TMUX_TMPDIR = tmuxRoot; -delete process.env.TMUX; -// a devteam session's own home never reaches the kits under test -delete process.env.DEVTEAM_HOME; -delete process.env.TMUX_PANE; // The real tmux is the first one on PATH that isn't a shim: a fork or a warm CLI process inherits // its parent's PATH, shim first. const isShim = (f) => { try { return fs.readFileSync(f, 'utf8').includes('tmux refused: TMUX_TMPDIR'); } catch { return false; } }; @@ -91,9 +95,7 @@ export function fleetFixture(name) { const home = path.join(base, 'home'); const tmuxDir = fs.mkdtempSync(path.join(os.tmpdir(), 'dt-tmux-')); const env = (role) => { - const e = { ...process.env, CLAUDE_CONFIG_DIR: path.join(home, 'claude'), CODEX_HOME: path.join(home, 'codex'), TMUX_TMPDIR: tmuxDir }; - for (const k of ['DEVTEAM_ROLE', 'DEVTEAM_TICKET', 'DEVTEAM_FEATURE', 'DEVTEAM_RUN', 'TMUX', 'TMUX_PANE']) delete e[k]; - return { ...e, ...role }; + return { ...process.env, CLAUDE_CONFIG_DIR: path.join(home, 'claude'), CODEX_HOME: path.join(home, 'codex'), TMUX_TMPDIR: tmuxDir, ...role }; }; const run = (cwd, args, role = {}, input) => { const r = runNode(path.join(cwd, 'docs/devteam/scripts/devteam.mjs'), args, { cwd, env: env(role), input }); return { status: r.status, out: r.stdout.trim(), err: r.stderr.trim() }; }; const ok = (cwd, args, role, input) => { const r = run(cwd, args, role, input); assert.equal(r.status, 0, `${args.join(' ')}: ${r.err || r.out}`); return r.out; }; @@ -308,6 +310,9 @@ export function gitFactFns(cwd, runDir) { const fns = vm.runInContext(`${cut('/* git reads start */', '/* git reads end */')}\n${cut('/* git facts start */', '/* git facts end */')}\nHOME_CWD = ${JSON.stringify(cwd)};\n;({ commitDateOf, ancestorOf, releasedInAt, factGet })`, ctx); return { ...fns, exit: () => { for (const f of exits.splice(0)) f(); } }; } +// One folder for every boxFns machine in this process, removed as it exits. +let boxFnsRoot = null; +const boxFnsDir = () => { if (!boxFnsRoot) { boxFnsRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'devteam-boxfns-')); process.on('exit', () => fs.rmSync(boxFnsRoot, { recursive: true, force: true })); } return boxFnsRoot; }; // The script's box detection, loaded by itself on a machine of the test's making: the box's files // exist only as files names them ({ path: text }), HOME is home, the password file's home is passwd // (null: the uid has none, and userInfo throws), and the environment is { HOME, ...env }. @@ -315,8 +320,7 @@ export function boxFns({ files = {}, home, passwd, env = {} }) { const src = fs.readFileSync(path.join(kit, 'scripts', 'devteam.mjs'), 'utf8'); const cut = (a, b) => { const i = src.indexOf(a), j = src.indexOf(b); assert.ok(i >= 0 && j > i, `${a} … ${b} marks a block in devteam.mjs`); return src.slice(i, j); }; const BOX = ['/opt/devteam/gateway.mjs', '/etc/devteam/thread-key.pub']; - const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'devteam-boxfns-')); - process.on('exit', () => fs.rmSync(dir, { recursive: true, force: true })); + const dir = fs.mkdtempSync(path.join(boxFnsDir(), 'm-')); const stand = Object.fromEntries(BOX.filter((f) => f in files).map((f, i) => { const p = path.join(dir, `box-${i}`); fs.writeFileSync(p, files[f]); return [f, p]; })); const at = (f) => (BOX.includes(f) ? stand[f] ?? path.join(dir, 'none') : f); const sandboxFs = { existsSync: (f) => fs.existsSync(at(f)), statSync: (f) => fs.statSync(at(f)), readFileSync: (f, e) => fs.readFileSync(at(f), e), realpathSync: (f) => fs.realpathSync(f) }; diff --git a/test/machine.test.mjs b/test/machine.test.mjs index 714aa0c..38000eb 100644 --- a/test/machine.test.mjs +++ b/test/machine.test.mjs @@ -7,11 +7,11 @@ import { execFile, execFileSync, spawn, spawnSync } from 'node:child_process'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; -import { hasTmux, kit, testDir, until, write } from './lib/kit.mjs'; +import { HARNESS_ENV, hasTmux, kit, testDir, until, write } from './lib/kit.mjs'; // tmux reads UTF-8 from the locale's name: the prompt's ❯ and its non-breaking space must survive process.env.LANG = process.platform === 'darwin' ? 'en_US.UTF-8' : 'C.UTF-8'; -for (const k of Object.keys(process.env)) if (/^(CLAUDECODE|CLAUDE_CODE_.*|CLAUDE_PID|CLAUDE_CONFIG_DIR|DEVTEAM_ROLE|DEVTEAM_TICKET|DEVTEAM_FEATURE)$/.test(k)) delete process.env[k]; +for (const k of Object.keys(process.env)) if (/^(CLAUDECODE|CLAUDE_CODE_.*|CLAUDE_PID|CLAUDE_CONFIG_DIR)$/.test(k)) delete process.env[k]; const tmp = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'devteam-machine-'))); process.on('exit', () => fs.rmSync(tmp, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 })); const home = path.join(tmp, 'home'); @@ -31,7 +31,6 @@ const stubShell = path.join(bin, 'stubshell'); write(stubShell, `#!/bin/sh\nPATH=${bin}:/usr/bin:/bin\nexport PATH\nENV=/dev/null\nexport ENV\ncase "$1" in -c) exec /bin/sh "$@" ;; *) exec /bin/sh -i ;; esac\n`); for (const f of ['gh', 'mise', 'claude', 'stubshell']) fs.chmodSync(path.join(bin, f), 0o755); const baseEnv = { ...process.env, HOME: home, PATH: `${bin}${path.delimiter}${process.env.PATH}`, DEVTEAM_TELL_SETTLE_SECONDS: '0.3', DEVTEAM_MACHINE_EPOCH: 'boot2' }; -for (const k of ['DEVTEAM_HOME', 'TMUX', 'TMUX_PANE']) delete baseEnv[k]; const tmuxSh = (args) => spawnSync('tmux', args, { encoding: 'utf8', env: baseEnv }); if (hasTmux) { execFileSync('tmux', ['new-session', '-d', '-s', 'keep', '-x', '200', '-y', '50', 'sleep 86400'], { env: baseEnv }); @@ -114,6 +113,39 @@ setInterval(() => {}, 1e6);\n`); const shown = (pane, text) => until(() => tmuxSh(['capture-pane', '-p', '-t', pane]).stdout.includes(text), `pane ${pane} drawn`); const paneState = (pane) => machine(['machine', 'pane-state', pane]).stdout.trim(); +// A worker's session env, as a box gives it: its run folder, its home, its role and ticket, the box's +// address, its tmux pane. Pointed at a decoy, it shows whether a test can still write to the fleet. +const workerEnv = (decoy) => ({ DEVTEAM_RUN: path.join(decoy, 'run'), DEVTEAM_HOME: decoy, DEVTEAM_ROLE: 'worker', DEVTEAM_TICKET: 'x-1', DEVTEAM_FEATURE: 'x', DEVTEAM_SUPERVISOR: '%1', DEVTEAM_PUBLIC_URL: 'https://box.example.com', DEVTEAM_PREVIEWS: '1', TMUX: 'x', TMUX_PANE: '%9' }); +const decoyOf = (name) => { const d = fs.mkdtempSync(path.join(tmp, `${name}-`)); fs.mkdirSync(path.join(d, 'run')); return d; }; +const filesIn = (d) => fs.readdirSync(d, { recursive: true }).map(String).sort(); + +test('importing the test helper leaves no session\'s DEVTEAM_* or tmux in the environment, only the harness\'s own knobs', () => { + const decoy = decoyOf('env-decoy'); + const env = { ...process.env, ...workerEnv(decoy), DEVTEAM_FORK: path.join(decoy, 'fork'), DEVTEAM_TEST_KEEP: '1' }; + const r = spawnSync(process.execPath, ['--input-type=module', '-e', `await import(${JSON.stringify(new URL('./lib/kit.mjs', import.meta.url).href)}); console.log(JSON.stringify(Object.keys(process.env).filter((k) => /^(DEVTEAM_|TMUX)/.test(k)).sort()))`], { cwd: tmp, env, encoding: 'utf8' }); + assert.equal(r.status, 0, r.stderr); + const names = JSON.parse(r.stdout.trim().split('\n').at(-1)); + assert.deepEqual(names.filter((k) => !HARNESS_ENV.test(k)), ['DEVTEAM_BOX', 'DEVTEAM_CSWAP', 'DEVTEAM_HUB_PORT', 'DEVTEAM_PORT_BASE', 'DEVTEAM_SUPERVISOR_UP', 'TMUX_TMPDIR']); + for (const k of ['DEVTEAM_FORK', 'DEVTEAM_TEST_KEEP', 'DEVTEAM_CGROUP', 'DEVTEAM_MEMINFO']) assert.ok(names.includes(k), `${k} survives: the harness reads it`); + assert.deepEqual(filesIn(decoy), ['run']); +}); + +test('a worker\'s session env sends nothing into its run folder: the incident\'s own test, run under it, writes nothing in the decoy', { skip: !hasTmux, timeout: 300000 }, () => { + const decoy = decoyOf('run-decoy'); + const env = { ...process.env, ...workerEnv(decoy) }; + delete env.NODE_TEST_CONTEXT; + const r = spawnSync(process.execPath, ['--test', '--test-reporter=tap', '--test-name-pattern=^status says at a glance', path.join(testDir, 'machine.test.mjs')], { cwd: REPO, env, encoding: 'utf8', timeout: 280000 }); + assert.equal(r.status, 0, r.stdout + r.stderr); + assert.match(r.stdout, /^# pass 1$/m); + assert.match(r.stdout, /^# fail 0$/m); + const oks = r.stdout.split('\n').filter((l) => /^ok \d+ - /.test(l)); + const ran = oks.filter((l) => / - status says at a glance/.test(l)); + assert.equal(ran.length, 1, 'the incident\'s test ran'); + assert.doesNotMatch(ran[0], /# SKIP/, 'and was not skipped'); + for (const l of oks.filter((x) => !ran.includes(x))) assert.match(l, /# SKIP test name does not match pattern$/, l); + assert.deepEqual(filesIn(decoy), ['run'], 'nothing written in the worker\'s run folder or home'); +}); + test('what a pane is at, from what is on it', { skip: !hasTmux }, async () => { const at = async (screen, want, why) => { const a = agent({ screens: [screen] }); await shown(a.pane, screen.replace(/\x1b\[[0-9;]*m/g, '').split('\n').find((l) => l.trim()).replace(/ /g, ' ').trim().slice(0, 12)); assert.equal(paneState(a.pane), want, why); a.kill(); }; await at(box(`\x1b[38;5;246m${prompt}\x1b[39m`), 'ready', 'an empty input, with the real non-breaking space'); diff --git a/test/operator-threads.test.mjs b/test/operator-threads.test.mjs index 2d74f0f..819b4b0 100644 --- a/test/operator-threads.test.mjs +++ b/test/operator-threads.test.mjs @@ -16,7 +16,7 @@ import { hubSignature } from '../cloud/gateway.mjs'; // tmux reads UTF-8 from the locale's name: the prompt's ❯ and its non-breaking space must survive process.env.LANG = process.platform === 'darwin' ? 'en_US.UTF-8' : 'C.UTF-8'; -for (const k of Object.keys(process.env)) if (/^(CLAUDECODE|CLAUDE_CODE_.*|CLAUDE_PID|CLAUDE_CONFIG_DIR|DEVTEAM_ROLE|DEVTEAM_TICKET|DEVTEAM_FEATURE|DEVTEAM_PUBLIC_URL|DEVTEAM_OPERATOR_COMMAND)$/.test(k)) delete process.env[k]; +for (const k of Object.keys(process.env)) if (/^(CLAUDECODE|CLAUDE_CODE_.*|CLAUDE_PID|CLAUDE_CONFIG_DIR)$/.test(k)) delete process.env[k]; const tmp = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'devteam-operator-'))); process.on('exit', () => fs.rmSync(tmp, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 })); const KIT = path.join(kit, 'scripts', 'devteam.mjs'); @@ -64,7 +64,6 @@ function machine({ box = false, starts = true } = {}) { fs.mkdirSync(home); const hub = PORTS + 80 + n; const env = { ...process.env, HOME: home, PATH: `${bin}${path.delimiter}${process.env.PATH}`, DEVTEAM_HUB_PORT: String(hub), DEVTEAM_TELL_SETTLE_SECONDS: '0.3', ...(box ? { DEVTEAM_PUBLIC_URL: 'https://box.example.com', DEVTEAM_GATEWAY_THREAD_KEY: GW_PUBLIC } : {}), ...(starts ? {} : { DEVTEAM_OPERATOR_COMMAND: '' }) }; - for (const k of ['DEVTEAM_HOME', 'TMUX', 'TMUX_PANE']) delete env[k]; const dir = box ? path.join(home, 'operator') : path.join(home, '.devteam', `operator-${hub}`); // input: a press's proof on stdin, as the hub hands it over (and says it has) const run = (args, extra = {}, input = undefined) => spawnSync(process.execPath, [KIT, 'operator-thread', ...args], { cwd: nowhere, encoding: 'utf8', env: { ...env, ...(input === undefined ? {} : { DEVTEAM_THREAD_PROOF: 'stdin' }), ...extra }, input }); @@ -664,7 +663,6 @@ async function hubFixture(t, name, { box: boxed = false } = {}) { const P = PORTS + 31; const a = agentProgram([box(prompt)]); const env = { ...process.env, HOME: home, DEVTEAM_HUB_PORT: String(HUB), DEVTEAM_PORT_BASE: String(P), DEVTEAM_HUB_RETRY_MS: '500', DEVTEAM_TELL_SETTLE_SECONDS: '0.3', CLAUDE_CONFIG_DIR: path.join(base, 'claude'), CODEX_HOME: path.join(base, 'codex'), ...(boxed ? { DEVTEAM_PUBLIC_URL: 'https://box.example.com', DEVTEAM_GATEWAY_THREAD_KEY: GW_PUBLIC } : {}) }; - for (const k of ['DEVTEAM_ROLE', 'DEVTEAM_TICKET', 'DEVTEAM_FEATURE', 'DEVTEAM_RUN', 'DEVTEAM_SUPERVISOR', 'TMUX', 'TMUX_PANE']) delete env[k]; const A = bareProject(base, 'alpha'); execFileSync('sh', ['-c', 'git add -A && git -c user.email=t@t -c user.name=t commit -qm init'], { cwd: A }); const dash = startDashboard(A, env); From 23b6774c6f2c30a0b5e521ae44e5850dd665234b Mon Sep 17 00:00:00 2001 From: Hugh Francis Date: Sat, 14 Nov 2026 22:33:48 +0000 Subject: [PATCH 08/22] kit-201: task 2 review Co-Authored-By: Claude Opus 5.5 --- .../reviews/task-2.md | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/task-2.md diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/task-2.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/task-2.md new file mode 100644 index 0000000..205831d --- /dev/null +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/task-2.md @@ -0,0 +1,63 @@ +# Task 2 review, pass 1 + +- Reviewer: task-reviewer (fresh subagent, claude-opus) +- Reviewed: d13898bb5997082025927a064a46189cb39d6674 + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| T2-1 | Minor | spec AC2 ("fails with files in the decoy") | test/machine.test.mjs:137-145 (decoy test) | The test asserts the child's exit status before it lists the decoy. When the env leaks, the child's own test fails first: with DEVTEAM_RUN kept, the kit writes into the decoy instead of the project's run folder, and the incident test throws `ENOENT … docs/devteam/run/accounts.json`. So the outer failure shows the child's TAP, not the files in the decoy. The decoy check only runs when the child passes. The test still fails under the leak, so the proof holds. But its failure message doesn't say what the spec says it proves. Either list the decoy first, or add `filesIn(decoy)` to the status assertion's message. | fixed in Task 3: the decoy listing is asserted before the child's status | +| T2-2 | Minor | spec AC2 (the failing run "is recorded in reviews/task-2.md or verification.md"); plan Task 2 Step 2 | ticket records | No pre-fix failing run was recorded: there was no task-2.md and no verification.md at this head. This review records one (see Mutations below), which meets the spec's "or reviews/task-2.md". | fixed: the reviewer's mutation run is the pre-fix record | +| T2-3 | Suggestion | none | test/gateway.test.mjs:320, 1256, 1309, 1484 | `delete env.DEVTEAM_ROLE` is now redundant, because the scrub already removed it from `process.env`. The spec only names the loops at 723 and 764, so leaving these is in scope. They are harmless. | wont-fix: harmless, outside the spec's scope | + +No Critical or Important findings. Checked: + +- **The scrub's reach.** + - Every test file that spawns the kit imports lib/kit.mjs, now including gateway.test.mjs. gitleaks.test.mjs doesn't import it, but it spawns only gitleaks, never the kit. + - ES import order: operator-threads imports `../cloud/gateway.mjs`, which reads `DEVTEAM_HUB_PORT` and `DEVTEAM_PORT_BASE` at its top level. It is imported after lib/kit.mjs, so it sees the scrubbed, re-set values. gateway.test.mjs loads gateway.mjs by dynamic import, after its own settings. + - cli.mjs is evaluated before kit.mjs's body, but its warm runners start lazily inside `runNode`, after the scrub. cli-runner.mjs replaces its env wholesale with the request's, which is built from the scrubbed `process.env`. + - kit.test.mjs's forks get `{ ...process.env, DEVTEAM_FORK }`. fork.mjs → chain.mjs → kit.mjs re-scrubs, and `DEVTEAM_FORK` survives because it matches `HARNESS_ENV`. + - chain.mjs builds its env from `process.env` plus role overlays, so it inherits the scrub. + - No test sets `DEVTEAM_ROLE`, `TICKET`, `FEATURE`, `RUN`, `HOME`, `TMUX` or `TMUX_PANE` on `process.env` after import. No file imports kit.mjs dynamically after setting env, apart from the new env-names test, which does so on purpose. +- **Harness knobs.** Every `process.env.DEVTEAM_*` read in test/ is one of these: + - a `HARNESS_ENV` member (`TEST_KEEP`, `TEST_COLD`, `TEST_GITLEAKS`, `TEST_READ_DELAY_MS`, `FORK`, `FORK_AUDIT`, `G1_RECORD`, `CGROUP`, `MEMINFO`); + - a value the kit or the test sets itself (`HUB_PORT`, `PORT_BASE`, `CSWAP`, `SUPERVISOR_UP`, `BOX`, `HUB_RETRY_MS`, `CLAIM_TOKEN`, `PUBLIC_MAX`, `GATE_TICKET`); + - a fixture script's input from the kit (`PARAMS`, `SINCE`). + + test.yml sets only `DEVTEAM_CGROUP`, `DEVTEAM_MEMINFO` and `DEVTEAM_TEST_GITLEAKS`, and all three survive the scrub. Nothing was deleted wrongly. +- **What removing the per-file lists means.** For every list that went, the scrub deletes a superset: + - operator-threads' `DEVTEAM_OPERATOR_COMMAND` and `DEVTEAM_PUBLIC_URL`; + - machine's `ROLE`, `TICKET`, `FEATURE`, `HOME` and `TMUX*`; + - hub-overview's and operator-threads' hubFixture lists; + - gateway's two loops; + - fleetFixture's list. + + Every value a test sets afterwards, such as `DEVTEAM_OPERATOR_COMMAND: ''`, `DEVTEAM_PUBLIC_URL` or the box key, is added after the spread, so it still wins. +- **hub-overview's `HOME: base`.** On Node 18 the Chrome test skips ("needs … WebSocket"), so I ran it on Node 22.23.4. It passed (`# pass 1`). bareProject's git commits pass `-c user.email/name` and run with the test's own env. Chrome's profile is its own. +- **gateway.test.mjs and lib/kit.mjs.** Its own `DEVTEAM_HUB_PORT` and `PORT_BASE` are set after the import and win. The real-hub test's `TMUX_TMPDIR` (`…/real-hub-*/tmux`) is not `dt-tmux-*`, so the shim refuses tmux outright, which is what that test wants. All 69 tests pass. The comment at its l.20 is updated. test/lib/ports.mjs:22 still says the gateway claims no block; it is outside touches, and plan Task 3 Step 5 notes it for the PR body. +- **The env-names test.** It asserts the exact non-harness set, and asserts that `DEVTEAM_FORK`, `TEST_KEEP`, `CGROUP` and `MEMINFO` survive. That covers Review Focus 3 and 4. +- **The decoy test.** It deletes `NODE_TEST_CONTEXT`. It asserts `# pass 1` and `# fail 0`, exactly one `ok … status says at a glance` line with no SKIP, and every other line filtered by name (the plan's reading of `# skip 0`). The child's pattern excludes the decoy test itself, so it doesn't recurse. +- **T1-1.** `boxFns` now makes one root and one exit listener per process. +- **AC1 grep.** The only matches are machine:14 and operator-threads:19 (now only `CLAUDECODE|CLAUDE_*`) and gateway:94 (the header cleanup). The second grep in plan Step 2b finds nothing. + +Ran, each via run-isolated.sh, one file at a time: + +| Run | Result | +|---|---| +| this worktree, `--test-name-pattern='importing the test helper\|decoy' test/machine.test.mjs` | `# pass 2`, `# fail 0` | +| this worktree, `test/gateway.test.mjs` whole | 69/69 pass | +| this worktree, `test/hub-overview.test.mjs` (Node 18) | 2 pass, 1 skip (Chrome needs WebSocket) | +| the same file on Node 22.23.4, `--test-name-pattern='Overview in Chrome'` | `# pass 1`, `# fail 0` | + +Mutations: each ran in a `git archive` copy of d13898b under my scratchpad (run-isolated.sh at the same relative place), with the two new machine tests. + +| Copy | Change | Env-names test | Decoy test | +|---|---|---|---| +| base | none | pass | pass | +| norun | the scrub keeps `DEVTEAM_RUN` | FAIL (`+ 'DEVTEAM_RUN'`) | FAIL | +| noscrub | the pre-fix scrub: delete only `DEVTEAM_HOME\|ROLE\|TICKET\|FEATURE` and `TMUX*` | FAIL (`+ DEVTEAM_PREVIEWS, PUBLIC_URL, RUN, SUPERVISOR`) | FAIL | + +In both failing copies, the decoy test failed at the child's status: its incident test failed with `ENOENT …/statused/docs/devteam/run/accounts.json`, because the kit wrote into the decoy's run folder instead. + +I added a print before the status assertion in the norun copy. It showed the decoy holding `["run","run/log","run/log/owner-2026-11.jsonl","run/log/supervisor-2026-11.jsonl"]`. This is the incident, reproduced against the decoy. It is the record of the failing run that spec AC2 asks for. + +Verdict: APPROVE d13898bb5997082025927a064a46189cb39d6674 From 8ffa2238a3ca00f14767001b6114c6823eca3f3e Mon Sep 17 00:00:00 2001 From: Hugh Francis Date: Sat, 14 Nov 2026 22:35:11 +0000 Subject: [PATCH 09/22] kit-201: spec and plan take the scope change (the kit refuses a foreign DEVTEAM_RUN) Co-Authored-By: Claude Opus 5.5 --- .../kit-201-tests-isolated-on-box/plan.md | 20 +++++++++++++++-- .../kit-201-tests-isolated-on-box/spec.md | 22 +++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md index 7b95128..944a749 100644 --- a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md @@ -7,7 +7,8 @@ - devteam.mjs: `DEVTEAM_BOX=off` makes the kit ignore the box's files, honored only while `$HOME` (by real path) isn't the passwd home (ruling d-20261114-215844973-supervisor). - The box code is marked as a block and loaded in a vm with a stand-in fs/os/env, so the limit is tested in CI. - A decoy test re-runs the incident's own test with a worker's env pointed at a decoy run folder: nothing lands there. -- Three tasks; no file added; risk normal. +- The kit refuses a `DEVTEAM_RUN` outside its resolved home (scope change m-20261114-222834445): every command but help dies naming both paths. +- Four tasks; no file added; risk normal. > **For agentic workers:** executed natively (executing-plans), each task reviewed by a fresh subagent into `reviews/task-.md`. @@ -81,7 +82,22 @@ Readings of the approved spec (spec-2 F2-1..F2-4, plan-1): the spec's "`# skip 0 - [ ] **Step 4: run them, see them pass.** - [ ] **Step 5: commit** `kit-201: one env scrub for every test that spawns the kit`; review into `reviews/task-2.md`. -### Task 3: the three files on the box, and the suite +### Task 3: the kit refuses a foreign run folder + +**Files:** +- Modify: `docs/devteam/scripts/devteam.mjs:463` (beside `runDir`) +- Test: `test/machine.test.mjs` + +**Interfaces:** +- Produces: right after `runDir`, a check (inline: `insideDir` is defined later and is a `const`) that, when `process.env.DEVTEAM_RUN` is set and `command` isn't `help`/`--help`/missing, compares `realPath(mainRoot)` with the real path of `runDir`'s nearest existing ancestor (re-joined with the rest) and `die()`s with spec §6's text unless they are equal or the run is under the root. No other change. + +- [ ] **Step 1: write the failing tests** in `machine.test.mjs`: `test('a kit refuses a run folder outside its home: a worker\'s DEVTEAM_RUN leaked into a fixture\'s kit writes nothing there, and names both', …)` and `test('a kit keeps its own run folder, by any spelling, as it always did', …)`. Fixtures: `A = bareProject(base, 'fixture')`, `H = bareProject(base, 'home')` (the stand-in home; its `docs/devteam/run` exists). Snapshot `H`'s run folder (relative path → sha of each file). Run `A`'s kit from `A` with `{ DEVTEAM_RUN: H/docs/devteam/run }` for `['phase', 'spec']` (as `DEVTEAM_ROLE: 'worker', DEVTEAM_TICKET: 'x-1'`), `['observe', '--subject', 'x', '--kind', 'other', '--detail', 'x']` and `['status']`: each status ≠ 0, stderr contains both `H/docs/devteam/run` (or its real path) and `A`, and the snapshot is unchanged. Second test: `DEVTEAM_RUN` = `A/docs/devteam/run`, = a symlink to it, and unset: `status` exits 0 each time, and `observe …` exits 0 with its line in `A`'s own run log. +- [ ] **Step 2: run them, see them fail** (the refusal test: the commands succeed and `H`'s run folder changes). +- [ ] **Step 3: implement** the check. +- [ ] **Step 4: run them, see them pass**; also the decoy test (it must still pass: its child's kit commands carry the fixture's own run, or none). Move the decoy test's `filesIn(decoy)` assertion before its status assertion (task-2 T2-1). +- [ ] **Step 5: commit** `kit-201: the kit refuses a DEVTEAM_RUN outside its home`; review into `reviews/task-3.md`. + +### Task 4: the three files on the box, and the suite - [ ] **Step 1:** `run-isolated.sh --test-reporter=tap test/operator-threads.test.mjs test/machine.test.mjs test/hub-overview.test.mjs` on this box (both box files present) → `# fail 0` (baseline at cca0976: operator-threads 1 pass / 15 fail). - [ ] **Step 2:** only after Task 2's decoy proof passes: the same three with a worker-shaped env (`run-as-worker.sh` beside run-isolated.sh: `DEVTEAM_RUN`, `DEVTEAM_HOME` set to a fresh decoy, `DEVTEAM_ROLE=worker`, `DEVTEAM_TICKET`, `DEVTEAM_PUBLIC_URL`, `TMUX`, `TMUX_PANE` set, never home's run) → `# fail 0`, decoy empty. The hub-overview tests on `fleetFixture` keep the real `HOME`, so the off switch is ignored for them; they passed on this box at main and must still. diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md index b026903..52e8881 100644 --- a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md @@ -64,6 +64,24 @@ under test believe it is on that box, whatever the test's environment says. 4. **CI and files.** test.yml passes on every job at the PR head; no file is added (the records under `docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/` aside). +6. **The kit refuses a foreign run folder** (scope change, m-20261114-222834445-supervisor; the + ticket's new done-when 3). `runDir` (devteam.mjs ~463) takes `DEVTEAM_RUN` from any cwd; that is + how a fixture repo's kit, with a worker's `DEVTEAM_RUN`, wrote owner-credited lines into home's + log. Now, when `DEVTEAM_RUN` is set and its real path (the nearest existing ancestor's, when the + folder isn't made yet) is not `mainRoot`'s real path or inside it, every command but `help` dies + before anything is read or written, naming both: `DEVTEAM_RUN= is not inside this kit's home + (): a session's run folder leaked into another repo's kit; unset DEVTEAM_RUN, or run + home's kit from home`. Every command, not only the writing ones: one rule that a reader can't + get wrong, and a read of another fleet's log is as misleading as a write (my call; the ticket says + log-writing commands, which this covers). Within home (a worker or planner in home's worktree, a + worker in a linked repo's worktree with `DEVTEAM_HOME`, the dashboard, home's own spawns, which + carry `DEVTEAM_RUN=/docs/devteam/run`) nothing changes; unset `DEVTEAM_RUN` is unchanged. + Tests (machine.test.mjs): from a `bareProject` fixture with `DEVTEAM_RUN` pointing at a second + fixture's run folder (standing in for home's), `log`-writing `phase`/`observe` and a read + (`status`) die with both paths in stderr and the second fixture's run folder is byte-for-byte + unchanged; with `DEVTEAM_RUN` pointing at the fixture's own run folder, a symlink to it, or + unset, the same commands work. + ## The off switch's limit (ruled: B, d-20261114-215844973-supervisor) The box files exist so that normal use of a command on a box can't take an owner's press unsigned @@ -138,6 +156,10 @@ comment beside `onBox()`. ## Rulings +- m-20261114-222834445-supervisor: scope change, new done-when 3: the kit refuses a DEVTEAM_RUN not + inside the resolved home (mainRoot, realpath) for log-writing commands, dying with both paths; + inside home unchanged; test both; update spec and plan with a short review pass, then build. + - d-20261114-215844973-supervisor (m-20261114-215845202-supervisor, re m-20261114-215838230): rule B, the switch honored only off the account's own home. Add a test that on a box-shaped fixture with HOME equal to the passwd home, DEVTEAM_BOX=off is ignored and onBox() stays true. The PR body says From 3a4cf600e62d0c9ab743976090af4bfea37dc3c2 Mon Sep 17 00:00:00 2001 From: Hugh Francis Date: Sat, 14 Nov 2026 22:46:24 +0000 Subject: [PATCH 10/22] kit-201: scope change after review Co-Authored-By: Claude Opus 5.5 --- .../kit-201-tests-isolated-on-box/plan.md | 8 ++-- .../reviews/spec-3.md | 24 +++++++++++ .../kit-201-tests-isolated-on-box/spec.md | 41 ++++++++++++------- 3 files changed, 55 insertions(+), 18 deletions(-) create mode 100644 docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-3.md diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md index 944a749..0d494a4 100644 --- a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md @@ -89,12 +89,12 @@ Readings of the approved spec (spec-2 F2-1..F2-4, plan-1): the spec's "`# skip 0 - Test: `test/machine.test.mjs` **Interfaces:** -- Produces: right after `runDir`, a check (inline: `insideDir` is defined later and is a `const`) that, when `process.env.DEVTEAM_RUN` is set and `command` isn't `help`/`--help`/missing, compares `realPath(mainRoot)` with the real path of `runDir`'s nearest existing ancestor (re-joined with the rest) and `die()`s with spec §6's text unless they are equal or the run is under the root. No other change. +- Produces: right after `runDir` (the `insideDir` helper is a later `const`, so inline): `const realPathMade = (p) => `; when `process.env.DEVTEAM_RUN` is set and `command` is not `help`/`--help`/missing/`machine`/`operator-thread`, nor `recover` with `flags.machine`: `const r = realPathMade(runDir), root = realPath(mainRoot)`; taken iff `r === realPathMade(path.join(mainRoot, 'docs', 'devteam', 'run'))` or `r === root` or `r.startsWith(root + path.sep)`; else `die()`. -- [ ] **Step 1: write the failing tests** in `machine.test.mjs`: `test('a kit refuses a run folder outside its home: a worker\'s DEVTEAM_RUN leaked into a fixture\'s kit writes nothing there, and names both', …)` and `test('a kit keeps its own run folder, by any spelling, as it always did', …)`. Fixtures: `A = bareProject(base, 'fixture')`, `H = bareProject(base, 'home')` (the stand-in home; its `docs/devteam/run` exists). Snapshot `H`'s run folder (relative path → sha of each file). Run `A`'s kit from `A` with `{ DEVTEAM_RUN: H/docs/devteam/run }` for `['phase', 'spec']` (as `DEVTEAM_ROLE: 'worker', DEVTEAM_TICKET: 'x-1'`), `['observe', '--subject', 'x', '--kind', 'other', '--detail', 'x']` and `['status']`: each status ≠ 0, stderr contains both `H/docs/devteam/run` (or its real path) and `A`, and the snapshot is unchanged. Second test: `DEVTEAM_RUN` = `A/docs/devteam/run`, = a symlink to it, and unset: `status` exits 0 each time, and `observe …` exits 0 with its line in `A`'s own run log. -- [ ] **Step 2: run them, see them fail** (the refusal test: the commands succeed and `H`'s run folder changes). +- [ ] **Step 1: write the failing tests** in `machine.test.mjs` (spec §6's list, exactly): `test('a kit refuses a run folder outside its home: a worker\'s DEVTEAM_RUN leaked into a fixture\'s kit writes nothing there, and says why', …)` and `test('a kit takes a run folder inside its home, however it is reached: its own, a symlink, one not made yet, a worktree\'s, a linked repo\'s, a run folder that is itself a symlink', …)`. Fixtures under one temp `base`: `app = bareProject(base, 'app')`, `api = bareProject(base, 'app-api')`; seed each run with `status` (no `DEVTEAM_RUN`); snapshot = sorted `[relpath, sha256]` of `api/docs/devteam/run`. Refusals assert `status !== 0` and `stderr` matches `/DEVTEAM_RUN=.* is not inside this kit's home \(.*\): a session's run folder leaked into another repo's kit/` and includes `app-api` and `app`. The worktree case: `git worktree add -q /app-wt` from `app` after a commit; run `app`'s worktree kit from it. The linked-repo case: a plain `git init` repo `/other`, `DEVTEAM_HOME=app`, `app`'s kit by absolute path, cwd `other`. The symlinked-run case: a fresh `bareProject(base, 'app-ln')` whose `docs/devteam/run` is moved to `/elsewhere-run` and symlinked back, `DEVTEAM_RUN=/docs/devteam/run`. +- [ ] **Step 2: run them, see them fail** (the refusal test: the commands succeed and `app-api`'s run folder changes). - [ ] **Step 3: implement** the check. -- [ ] **Step 4: run them, see them pass**; also the decoy test (it must still pass: its child's kit commands carry the fixture's own run, or none). Move the decoy test's `filesIn(decoy)` assertion before its status assertion (task-2 T2-1). +- [ ] **Step 4: run them, see them pass**; then the decoy test, and `operator-threads`/`gateway` (the exempt commands), via run-isolated.sh. Move the decoy test's `filesIn(decoy)` assertion before its status assertion (task-2 T2-1). - [ ] **Step 5: commit** `kit-201: the kit refuses a DEVTEAM_RUN outside its home`; review into `reviews/task-3.md`. ### Task 4: the three files on the box, and the suite diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-3.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-3.md new file mode 100644 index 0000000..9854472 --- /dev/null +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-3.md @@ -0,0 +1,24 @@ +# Spec and plan review (scope change), pass 1 + +- Reviewer: scope-reviewer (fresh subagent, claude-opus) +- Reviewed: 8ffa2238a3ca00f14767001b6114c6823eca3f3e + +Read: the diff (spec §6, plan Task 3), devteam.mjs 1-60 (re-exec), 380-480 (homeAt, mainRoot, runDir), every `DEVTEAM_RUN` in the kit (463, 12734, 12803, 15340, 18109), worker.md, cloud/Dockerfile's box commands, every test that sets or scrubs `DEVTEAM_RUN`, `bareProject`, `cmdPhase`, `cmdObserve`, `cmdMachine`, `cmdRecover`. One probe, run through run-isolated.sh and deleted afterwards: with two `bareProject` fixtures and the kit at d13898b, `phase spec`, `observe` and `status` run from A with `DEVTEAM_RUN=H/docs/devteam/run` all exit 0. The first two create `H/docs/devteam/run/log/{x-1,supervisor-2026-11}.jsonl`, so the refusal test does fail without the guard. Right after `bareProject`, though, `H/docs/devteam/run` does not exist. + +Setups checked that the guard as specified leaves alone: a worker or planner in a home worktree (`mainRoot` is dirname of home's common dir, and the worker's `DEVTEAM_RUN` comes from line 15340, which is home's run); a worker in a linked repo's worktree with `DEVTEAM_HOME` (this ticket's own case; done-when 3 runs home's kit by absolute path, so `mainRoot` is home); the re-exec to the main checkout's kit, which carries the same guard; the dashboard and the kits home spawns (same home; `projectKit` and `recover --machine` already strip `DEVTEAM_RUN`); box boot, which sets no `DEVTEAM_RUN`; and the tests (only machine.test.mjs's `workerEnv` sets it, and that env reaches only `node --test` and the helper import, both scrubbed by test/lib/kit.mjs). A symlinked home also passes, because both sides are compared by real path. + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| S3-1 | Important | spec §6 "Within home … nothing changes"; done-when 3 | spec §6; plan Task 3 Interfaces | The check compares real paths, so a home whose `docs/devteam/run` is a symlink to somewhere outside the checkout (run kept on another disk or volume, or out of a synced folder) refuses every worker and planner. They get `DEVTEAM_RUN=/docs/devteam/run` from line 15340, and its real path is outside `realPath(mainRoot)`. Unset `DEVTEAM_RUN` resolves to that same folder and keeps working, so the same log is allowed one way and refused the other. Fix: also accept when the run's real path equals the real path of the default run folder (`path.join(mainRoot,'docs','devteam','run')`), and add a test where H's run folder is a symlink to a folder outside H. | fixed: a run whose real path is the default run folder's is taken; tested with a symlinked run | +| S3-2 | Important | spec §6 "Tests … with `DEVTEAM_RUN` pointing at the fixture's own run folder … work"; quality §3 (missing test, plausible failure path) | plan Task 3 Step 1, second test | The "keeps its own run folder" test runs only from A's main checkout. The setups most likely to be refused by mistake are a worker whose cwd is a home worktree (`mainRoot` comes from the common dir, not the cwd) and a worker in a linked repo's checkout with `DEVTEAM_HOME`, the shape of done-when 3. Neither is tested, and no other test runs the kit with a worker's `DEVTEAM_RUN` (test/lib/kit.mjs strips it everywhere). Add one: commit once in H, `git worktree add`, then run `status` and `observe` from the worktree with `DEVTEAM_RUN=H/docs/devteam/run`, and once from a second plain repo with `DEVTEAM_HOME=H`, both exit 0. | fixed: worktree and linked-repo (DEVTEAM_HOME) cases tested | +| S3-3 | Important | spec §6 "is not `mainRoot`'s real path or inside it"; quality §3 | plan Task 3 Interfaces "or the run is under the root" | The plan doesn't say how "under the root" is decided. A plain `startsWith` accepts a sibling whose name starts with home's (`/code/app` and `/code/app-api`, a common home and linked-repo pair), and the fixtures `fixture` and `home` can't catch that. Specify a separator-bounded test (`path.relative`, not `..` and not absolute) and add a case named to catch it, e.g. A = `bareProject(base, 'home-api')` refusing `home`'s run, or the other way round. | fixed: separator-bounded; fixtures app and app-api | +| S3-4 | Minor | plan Task 3 Step 1 "its `docs/devteam/run` exists" | plan Task 3 Step 1 | Wrong as written: `bareProject`'s `init` doesn't create `run/` (probe above). Snapshotting it with `readdirSync` throws. Create it and put a log line in it first, so "byte-for-byte unchanged" covers an existing folder with content. Keep a second case with a run folder that doesn't exist yet, which is the nearest-existing-ancestor branch spec §6 names and the plan doesn't test. In the second test, the symlink to `A/docs/devteam/run` also needs that folder to exist first. | fixed: runs seeded first; a not-yet-made folder inside home tested | +| S3-5 | Minor | spec §6 "every command but `help`"; devteam.mjs homeAt `machineWide` | spec §6; plan Task 3 Interfaces | `machine`, `operator-thread` and `recover --machine` are meant to run "from anywhere, a folder that is no project included" (homeAt exempts them), and they don't use `runDir`: they write `~/.box`, `~/.devteam` and other repos' kits with the env scrubbed. On a box they run from `/opt/devteam/kit`, which is in no checkout, so `mainRoot` is the cwd's repo or the cwd itself. Refusing them whenever a `DEVTEAM_RUN` is inherited, say a session in a worker's pane running `box-alert` from `~`, breaks a command that writes nothing there and protects nothing. Exempt the same `machineWide` set, and say so in §6. | fixed: machine, operator-thread, recover --machine exempt | +| S3-6 | Minor | ruling m-20261114-222834445 ("for log-writing commands"); constitution §6 precedence; P3 | spec §6 "(my call …)" | Widening the refusal to reads goes past the supervisor's explicit instruction. It is a superset and P3 lets it be decided, but it should be reported: say it in the PR body, as a line in the handoff, or in a message to the supervisor, not only inside the spec. The reasoning that a read of another fleet's log misleads is sound. | fixed: reported to the supervisor | +| S3-7 | Minor | spec §6 (the exact message) | plan Task 3 Step 1 | The assertion "stderr contains both H's run and A" is weak, since A's path shows up in many of the kit's errors. Match the spec's text: `/is not inside this kit's home/` together with both paths. | fixed: the test matches the message text | +| S3-8 | Minor | P6 ("say in one line what it doesn't defend against") | spec §6 | No such line. Suggested: it doesn't stop a kit given a foreign home through `DEVTEAM_HOME`/`--project` (those have their own checks), nor a run folder pointed anywhere inside home. | fixed: P6 line added | +| S3-9 | Suggestion | spec §6 "before anything is read or written" | spec §6 | This overclaims: the re-exec and homeAt have already run git and read files by line 463. "Before the command runs, or anything is written" is accurate. | fixed: says after home is resolved, before the run folder is read or written | + +Values and refusals (strategy.md): R1 is not crossed. The change takes away a path by which a misplaced kit wrote owner- or supervisor-credited lines into home's log, which is exactly the accidental leak P6 asks reviews to stop. P5: the review tried to break the guard through symlinked runs, worktree cwds, linked repos, machine-wide commands, a prefix sibling and a run folder that isn't there yet (S3-1 to S3-5). P2 holds: nothing in it is specific to one project. P1 holds: it is harness-agnostic. R2 doesn't apply: the message is for agents, not the owner. P6 is missing its one line (S3-8). + +Verdict: CHANGES 8ffa2238a3ca00f14767001b6114c6823eca3f3e diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md index 52e8881..dbe1e13 100644 --- a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md @@ -67,20 +67,33 @@ under test believe it is on that box, whatever the test's environment says. 6. **The kit refuses a foreign run folder** (scope change, m-20261114-222834445-supervisor; the ticket's new done-when 3). `runDir` (devteam.mjs ~463) takes `DEVTEAM_RUN` from any cwd; that is how a fixture repo's kit, with a worker's `DEVTEAM_RUN`, wrote owner-credited lines into home's - log. Now, when `DEVTEAM_RUN` is set and its real path (the nearest existing ancestor's, when the - folder isn't made yet) is not `mainRoot`'s real path or inside it, every command but `help` dies - before anything is read or written, naming both: `DEVTEAM_RUN= is not inside this kit's home - (): a session's run folder leaked into another repo's kit; unset DEVTEAM_RUN, or run - home's kit from home`. Every command, not only the writing ones: one rule that a reader can't - get wrong, and a read of another fleet's log is as misleading as a write (my call; the ticket says - log-writing commands, which this covers). Within home (a worker or planner in home's worktree, a - worker in a linked repo's worktree with `DEVTEAM_HOME`, the dashboard, home's own spawns, which - carry `DEVTEAM_RUN=/docs/devteam/run`) nothing changes; unset `DEVTEAM_RUN` is unchanged. - Tests (machine.test.mjs): from a `bareProject` fixture with `DEVTEAM_RUN` pointing at a second - fixture's run folder (standing in for home's), `log`-writing `phase`/`observe` and a read - (`status`) die with both paths in stderr and the second fixture's run folder is byte-for-byte - unchanged; with `DEVTEAM_RUN` pointing at the fixture's own run folder, a symlink to it, or - unset, the same commands work. + log. Now, when `DEVTEAM_RUN` is set, it is taken only if its real path (the nearest existing + ancestor's real path, re-joined with the rest, when the folder isn't made yet) is either the real + path of the default run folder (`/docs/devteam/run`, which may itself be a symlink out + of the checkout) or `realPath(mainRoot)` or under it, separator-bounded (`-api` is not under + ``). Otherwise the command dies, after home is resolved and before the run folder is + read or written, with: `DEVTEAM_RUN= is not inside this kit's home (): a session's + run folder leaked into another repo's kit; unset DEVTEAM_RUN, or run home's kit from home`. + Exempt: `help`, and the machine-wide commands homeAt already exempts (`machine`, + `operator-thread`, `recover --machine`), which run from anywhere and keep their records outside + any run folder. Every other command is checked, not only the writing ones: one rule a reader + can't get wrong, and a read of another fleet's log misleads as much as a write does (my call, + reported to the supervisor; it covers the ticket's "log-writing"). Within home nothing changes: + a worker or planner in home's worktree, a worker in a linked repo's checkout with `DEVTEAM_HOME`, + the dashboard, home's own spawns (they carry `DEVTEAM_RUN=/docs/devteam/run`), and an + unset `DEVTEAM_RUN`. + Tests (machine.test.mjs), on `bareProject` fixtures `app` (the kit run) and `app-api` (standing + in for home; a prefix check would wrongly pass it), each run folder seeded by one command run + with no `DEVTEAM_RUN`: refused, from `app` with `DEVTEAM_RUN=/docs/devteam/run`: + `phase spec` (as a worker), `observe …` and `status` each exit non-zero with the message above + (both paths in it), and `app-api`'s run folder is byte-for-byte unchanged. Taken: `DEVTEAM_RUN` + as `app`'s own run folder, a symlink to it, a not-yet-made folder inside `app`, and unset, from + `app`; from a `git worktree` of `app` with `app`'s run folder (a worker in home's worktree); from + a third repo's checkout with `DEVTEAM_HOME=` and `app`'s kit by absolute path (a worker in a + linked repo); and with `app`'s `docs/devteam/run` a symlink to a folder outside it: `observe …` + exits 0 and its line is in that run folder's log, `status` exits 0. + Not defended (P6): a `DEVTEAM_RUN` that names, on purpose, a folder inside the kit's home other + than its run folder, or a kit copied into home's own tree. ## The off switch's limit (ruled: B, d-20261114-215844973-supervisor) From e9cae1315d0ae80ed7f6ee7833ccb16a9c57cef9 Mon Sep 17 00:00:00 2001 From: Hugh Francis Date: Sat, 14 Nov 2026 22:51:44 +0000 Subject: [PATCH 11/22] kit-201: spec and plan take the supervisor's link-worker, decoy and nested-kit rulings Co-Authored-By: Claude Opus 5.5 --- .../kit-201-tests-isolated-on-box/plan.md | 6 ++- .../reviews/spec-4.md | 27 ++++++++++ .../kit-201-tests-isolated-on-box/spec.md | 50 ++++++++++++++----- 3 files changed, 68 insertions(+), 15 deletions(-) create mode 100644 docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-4.md diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md index 0d494a4..f8c1b91 100644 --- a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md @@ -8,6 +8,7 @@ - The box code is marked as a block and loaded in a vm with a stand-in fs/os/env, so the limit is tested in CI. - A decoy test re-runs the incident's own test with a worker's env pointed at a decoy run folder: nothing lands there. - The kit refuses a `DEVTEAM_RUN` outside its resolved home (scope change m-20261114-222834445): every command but help dies naming both paths. +- A kit copy nested in a home without its own repo dies instead of taking that home; `bareProject` asserts its fixtures are repos of their own. - Four tasks; no file added; risk normal. > **For agentic workers:** executed natively (executing-plans), each task reviewed by a fresh subagent into `reviews/task-.md`. @@ -91,10 +92,11 @@ Readings of the approved spec (spec-2 F2-1..F2-4, plan-1): the spec's "`# skip 0 **Interfaces:** - Produces: right after `runDir` (the `insideDir` helper is a later `const`, so inline): `const realPathMade = (p) => `; when `process.env.DEVTEAM_RUN` is set and `command` is not `help`/`--help`/missing/`machine`/`operator-thread`, nor `recover` with `flags.machine`: `const r = realPathMade(runDir), root = realPath(mainRoot)`; taken iff `r === realPathMade(path.join(mainRoot, 'docs', 'devteam', 'run'))` or `r === root` or `r.startsWith(root + path.sep)`; else `die()`. -- [ ] **Step 1: write the failing tests** in `machine.test.mjs` (spec §6's list, exactly): `test('a kit refuses a run folder outside its home: a worker\'s DEVTEAM_RUN leaked into a fixture\'s kit writes nothing there, and says why', …)` and `test('a kit takes a run folder inside its home, however it is reached: its own, a symlink, one not made yet, a worktree\'s, a linked repo\'s, a run folder that is itself a symlink', …)`. Fixtures under one temp `base`: `app = bareProject(base, 'app')`, `api = bareProject(base, 'app-api')`; seed each run with `status` (no `DEVTEAM_RUN`); snapshot = sorted `[relpath, sha256]` of `api/docs/devteam/run`. Refusals assert `status !== 0` and `stderr` matches `/DEVTEAM_RUN=.* is not inside this kit's home \(.*\): a session's run folder leaked into another repo's kit/` and includes `app-api` and `app`. The worktree case: `git worktree add -q /app-wt` from `app` after a commit; run `app`'s worktree kit from it. The linked-repo case: a plain `git init` repo `/other`, `DEVTEAM_HOME=app`, `app`'s kit by absolute path, cwd `other`. The symlinked-run case: a fresh `bareProject(base, 'app-ln')` whose `docs/devteam/run` is moved to `/elsewhere-run` and symlinked back, `DEVTEAM_RUN=/docs/devteam/run`. +- [ ] **Step 1: write the failing tests** in `machine.test.mjs` (spec §6's list, exactly): `test('a kit refuses a run folder outside its home: a worker\'s DEVTEAM_RUN leaked into a fixture\'s kit writes nothing there, and says why', …)` and `test('a kit takes a run folder inside its home, however it is reached: its own, a symlink, one not made yet, a worktree\'s, a linked repo\'s, a run folder that is itself a symlink', …)`. Fixtures under one temp `base`: `app = bareProject(base, 'app')`, `api = bareProject(base, 'app-api')`; seed each run with `observe` (no `DEVTEAM_RUN`); snapshot = sorted `[relpath, sha256]` of `api/docs/devteam/run`. Refusals assert `status !== 0` and `stderr` matches `/DEVTEAM_RUN=.* is not inside this kit's home \(.*\): a session's run folder leaked into another repo's kit/` and includes `app-api` and `app`. The worktree case: `git worktree add -q /app-wt` from `app` after a commit; run `app`'s worktree kit from it. The linked-repo case is its own test, `test('a worker on a repo link keeps every command it uses: …')`: home `bareProject(base, 'home')`; a plain repo `/linked` with a worktree `/linked-wt`; from `linked-wt`, home's kit by absolute path with `DEVTEAM_HOME=home`, `DEVTEAM_RUN=/docs/devteam/run`, `DEVTEAM_ROLE=worker`, `DEVTEAM_TICKET=x-1`: `phase build --task T1`, `send supervisor … --kind info`, `gate test -- node -e 0`, `send supervisor 'x-1 ready to merge' --kind merge-request`, `whoami` exit 0; `check-ticket x-1`'s stderr doesn't match the refusal; the sent line is in home's run log. The symlinked-run case: a fresh `bareProject(base, 'app-ln')` whose `docs/devteam/run` is moved to `/elsewhere-run` and symlinked back, `DEVTEAM_RUN=/docs/devteam/run`. - [ ] **Step 2: run them, see them fail** (the refusal test: the commands succeed and `app-api`'s run folder changes). - [ ] **Step 3: implement** the check. -- [ ] **Step 4: run them, see them pass**; then the decoy test, and `operator-threads`/`gateway` (the exempt commands), via run-isolated.sh. Move the decoy test's `filesIn(decoy)` assertion before its status assertion (task-2 T2-1). +- [ ] **Step 3b: the nested kit** (spec §7): a test `test('a kit copied into a home\'s tree without a repo of its own refuses to run against that home', …)`: `home = bareProject(base, 'home')`, seeded with `observe`; `fs.cpSync(kit, /fixtures/plain/docs/devteam)` (no project/run, as bareProject copies); snapshot home's run; `status` and `observe` from `/fixtures/plain` with the copy's kit and no `DEVTEAM_RUN` exit non-zero with stderr matching `/is not its checkout's own .*: a copy inside another repo can't run against that repo's home/`; snapshot unchanged. Red first, then in devteam.mjs, beside homeAt (before it reads home): `const scriptTop = git(['rev-parse', '--show-toplevel'], here)`; when it is set, `command` isn't `help`/`--help`, and `realPath(path.join(scriptTop, 'docs', 'devteam', 'scripts')) !== realPath(here)`, die with spec §7's text. In `bareProject`, after `git init`: `assert.equal(fs.realpathSync(execFileSync('git', ['rev-parse', '--show-toplevel'], { cwd: r, encoding: 'utf8' }).trim()), fs.realpathSync(r), 'a fixture is a repo of its own')`. +- [ ] **Step 4: run them, see them pass**; then the decoy test, rewritten to spec §2's exact reproduction (`DEVTEAM_RUN=`, `DEVTEAM_ROLE=worker`, no `DEVTEAM_HOME`, cwd the kit's checkout; the decoy must be empty; child timeout 900 s, test timeout 960 s: on a loaded box the incident test alone takes ~270 s), and `operator-threads`/`gateway` (the exempt commands), via run-isolated.sh. Move the decoy test's `filesIn(decoy)` assertion before its status assertion (task-2 T2-1). - [ ] **Step 5: commit** `kit-201: the kit refuses a DEVTEAM_RUN outside its home`; review into `reviews/task-3.md`. ### Task 4: the three files on the box, and the suite diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-4.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-4.md new file mode 100644 index 0000000..e1a6272 --- /dev/null +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-4.md @@ -0,0 +1,27 @@ +# Spec and plan review (scope change), pass 2 + +- Reviewer: scope-confirmer (fresh subagent, claude-opus) +- Reviewed: 3a4cf600e62d0c9ab743976090af4bfea37dc3c2 + +Read: spec-3.md, the diff 8ffa223..3a4cf60 (spec §6, plan Task 3), devteam.mjs 55-135 and 380-480, test/lib/kit.mjs `bareProject`, the log's messages to kit-201. Everything the plan's check uses is defined before line 463: `command` (59), `flags` (84), `die` (99, and its `queuedNudges` at 114), `realPath` (393), `mainRoot` (459). One probe, in the scratchpad with `env -i` (no inherited `DEVTEAM_*`), then deleted: a fixture built the way `bareProject` builds one, then `status` run with no `DEVTEAM_RUN`. It exits 0, and `docs/devteam/run` still doesn't exist afterwards. + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| S4-1 | Important | quality §3 (a disposition that doesn't fix its finding); S3-4 | plan Task 3 Step 1 "seed each run with `status` (no `DEVTEAM_RUN`)" | `status` writes nothing into the run folder (probe above), so seeding with it leaves `docs/devteam/run` missing. Three things break. The `api` snapshot's `readdirSync` throws, or, if it is written to tolerate a missing folder, "byte-for-byte unchanged" covers an empty folder only, which is what S3-4 asked to avoid. The symlinked-run case can't "move" an `app-ln/docs/devteam/run` that doesn't exist. The symlink to `app`'s own run points at nothing. Fix: seed with a command that writes a log line (`observe --subject x --kind other --detail seed`, which the pass-1 probe showed creates `run/log/…`), and say so in both spec and plan. | fixed: seeded with observe | +| S4-2 | Important | owner/supervisor instruction first (constitution §6); m-20261114-224640331-supervisor | spec §6 "Taken: … from a third repo's checkout with `DEVTEAM_HOME`"; plan Task 3 Step 1 "linked-repo case" | The supervisor answered S3-6's report at 22:46:40, 16 s after this commit. The answer adds a must-not-break, with a test, and asks for it in the spec's acceptance: a worker on a repo link (cwd in the link's worktree, `DEVTEAM_HOME`=home, `DEVTEAM_RUN`=home's run) keeps working for every command it uses (`phase`, `send`, `check-ticket`, `gate`, `merge-request`). The spec and plan cover a plain `git init` repo, which is neither a link in `app`'s settings nor a worktree, and they test only `observe` and `status`. Fix: in spec §6 and plan Task 3, make the case a repo link of `app` (named in its settings) with the worker's cwd in a worktree of that link, and run those five commands, or as many as can run in a fixture, naming any left out and why. Pass means none of them is refused by the guard. | fixed: the link worker's must-not-break is in acceptance 6 and its own test | +| S4-3 | Minor | quality §3 (weak assertion); S3-7 | plan Task 3 Step 1 "includes `app-api` and `app`" | `app` is a substring of `app-api`, so "includes `app`" always passes, and `\(.*\)` doesn't check which root the message names. Assert the exact text instead: `DEVTEAM_RUN=${apiRun}` and `(${app})`, or their real paths. | fixed: the test asserts the home as '()', the exact path in parentheses | +| S4-4 | Suggestion | consistency | spec §6 "with `app`'s `docs/devteam/run` a symlink"; plan "a fresh `bareProject(base, 'app-ln')`" | The spec and plan name different fixtures for the symlinked-run case. The plan's choice, a fresh fixture, is the better one; make the spec say the same. | fixed: the spec names the plan's fresh app-ln fixture | + +Notes per pass-1 finding: + +- S3-1, fixed. A run whose real path equals `realPathMade(/docs/devteam/run)` is taken, and the symlinked-run case tests it. That test depends on S4-1. +- S3-2, partly fixed. The home-worktree case is there. The linked-repo case is a plain repo, not a link's worktree, and the supervisor now asks for more than that (S4-2). +- S3-3, fixed. `r === root || r.startsWith(root + path.sep)` is separator-bounded, and `app`/`app-api` catch a bare prefix test. +- S3-4, not fixed as written (S4-1). The not-yet-made-folder case is added and is fine. +- S3-5, fixed. The spec and plan exempt `machine`, `operator-thread` and `recover --machine` the same way homeAt does. Plan Step 4 runs the operator-threads and gateway tests as regression cover. +- S3-6, fixed. Reported to the supervisor as m-20261114-224628733, and the answer (m-20261114-224640331) approves the widening, but it brings S4-2. +- S3-7, mostly fixed. The test matches the message text, but the path assertions are weak (S4-3). +- S3-8, fixed. The P6 line is present. +- S3-9, fixed. The spec now says "after home is resolved and before the run folder is read or written", which is accurate. + +Verdict: CHANGES 3a4cf600e62d0c9ab743976090af4bfea37dc3c2 diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md index dbe1e13..e73e7f8 100644 --- a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/spec.md @@ -44,15 +44,16 @@ under test believe it is on that box, whatever the test's environment says. `lib/kit.mjs` so its spawns inherit the scrub. Check: `grep -nE "DEVTEAM_(ROLE|TICKET|FEATURE|RUN|HOME|SUPERVISOR|PUBLIC_URL)'?[],|)]|delete [a-zA-Z.]*\[k\]" test/{machine,operator-threads,hub-overview,gateway}.test.mjs` finds no denylist; the scrub is the only one. -2. **Proven against a decoy.** A test in `machine.test.mjs` runs the incident's own test ("status says - at a glance…") in a child `node --test` whose environment carries `DEVTEAM_RUN=/run`, - `DEVTEAM_HOME=`, `DEVTEAM_ROLE=worker`, `DEVTEAM_TICKET=`, `DEVTEAM_PUBLIC_URL`, `TMUX` - and `TMUX_PANE`; the child's own TAP says exactly one test ran and passed (`# pass 1`, - `# skip 0`, `# fail 0`: a skipped test, or a name pattern matching nothing, fails the proof), and - the decoy folder holds nothing but its empty `run/` afterwards. The test skips, as the incident's - does, where there is no tmux. Before the scrub lands, it is run once and fails with files in the - decoy (the pre-fix kit does write into `DEVTEAM_RUN`, as the incident shows); that run is recorded - in `reviews/task-2.md` or `verification.md`. A second, quick +2. **Proven against a decoy.** A test in `machine.test.mjs` runs exactly the supervisor's + reproduction (m-20261114-224904663): from the kit's checkout, `DEVTEAM_HOME` unset, + `DEVTEAM_ROLE=worker`, `DEVTEAM_RUN=`, `node --test --test-name-pattern 'status says at a + glance' test/machine.test.mjs` (with `--test-reporter=tap`, so its summary can be read). At main + that writes the owner rule into `/log/owner-2026-11.jsonl`; after this change the decoy + is empty. The child's own TAP says the incident's test ran, unskipped, and passed (`# pass 1`, + `# fail 0`; a skipped test, or a name pattern matching nothing, fails the proof). The test skips, + as the incident's does, where there is no tmux. The pre-fix failing run is recorded in + `reviews/task-2.md`. The other session variables (`DEVTEAM_HOME`, `DEVTEAM_TICKET`, + `DEVTEAM_PUBLIC_URL`, `TMUX`, `TMUX_PANE`, …) are covered by the env-names test. A second, quick check: a child that imports `lib/kit.mjs` and prints its `DEVTEAM_*` names shows only the harness knobs and the kit's own settings. 3. **Box detection can be turned off.** `DEVTEAM_BOX=off` makes the kit ignore the box's files @@ -83,18 +84,35 @@ under test believe it is on that box, whatever the test's environment says. the dashboard, home's own spawns (they carry `DEVTEAM_RUN=/docs/devteam/run`), and an unset `DEVTEAM_RUN`. Tests (machine.test.mjs), on `bareProject` fixtures `app` (the kit run) and `app-api` (standing - in for home; a prefix check would wrongly pass it), each run folder seeded by one command run - with no `DEVTEAM_RUN`: refused, from `app` with `DEVTEAM_RUN=/docs/devteam/run`: + in for home; a prefix check would wrongly pass it), each run folder seeded by one `observe` run + with no `DEVTEAM_RUN` (`status` writes nothing): refused, from `app` with `DEVTEAM_RUN=/docs/devteam/run`: `phase spec` (as a worker), `observe …` and `status` each exit non-zero with the message above (both paths in it), and `app-api`'s run folder is byte-for-byte unchanged. Taken: `DEVTEAM_RUN` as `app`'s own run folder, a symlink to it, a not-yet-made folder inside `app`, and unset, from `app`; from a `git worktree` of `app` with `app`'s run folder (a worker in home's worktree); from a third repo's checkout with `DEVTEAM_HOME=` and `app`'s kit by absolute path (a worker in a - linked repo); and with `app`'s `docs/devteam/run` a symlink to a folder outside it: `observe …` - exits 0 and its line is in that run folder's log, `status` exits 0. + linked repo); and a fresh fixture `app-ln` whose `docs/devteam/run` is a symlink to a folder + outside it: `observe …` exits 0 and its line is in that run folder's log, `status` exits 0. + **Must not break** (m-20261114-224640331-supervisor): a worker on a repo link, cwd in the link + repo's worktree, `DEVTEAM_HOME=`, `DEVTEAM_RUN=/docs/devteam/run` (as kit-46 and this + ticket's worker run today) keeps every command it uses: `phase`, `send`, `gate`, `send … --kind + merge-request` exit 0 with their lines in home's run, and `check-ticket` is never refused by the + guard (it may fail on the ticket's own merits). Not defended (P6): a `DEVTEAM_RUN` that names, on purpose, a folder inside the kit's home other than its run folder, or a kit copied into home's own tree. +7. **A kit nested in a home without a repo of its own can't take that home** (m-20261114-224904663). + A kit copied into a folder inside home's checkout that has no `.git` of its own (a fixture made + without `git init`) resolves home's git, so it would run against home's project and live log. + When the running kit's script is not `/docs/devteam/scripts/` (by real + path; `git rev-parse --show-toplevel` from the script's folder), every command but `help` dies: + `this kit (