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..21833ac --- /dev/null +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/plan.md @@ -0,0 +1,112 @@ +# 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. +- 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`. + +**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 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. + +--- + +### 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 = {} })` (`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`. + - `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/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:** +- 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, 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: 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`. + +### 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` (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 `observe` (no `DEVTEAM_RUN`); snapshot = sorted `[relpath, sha256]` of `api/docs/devteam/run`. Refusals assert `status !== 0`, `stderr` matches `/DEVTEAM_RUN=.* is not inside this kit's home \(.*\): a session's run folder leaked into another repo's kit/`, and `stderr` includes exactly `DEVTEAM_RUN=/docs/devteam/run is not inside this kit's home (): `. 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 repo `/linked` (origin `https://github.com/me/linked.git`) with a worktree `/linked-wt`, named in home's settings.md as `links.linked` (kind repo, remote `me/linked`, path); 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 phase, gate and both sent lines are 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 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. The same again after appending a line to the copy (so it differs from home's kit, which would otherwise be re-exec'd). Red first, then in devteam.mjs's re-exec block (~34; `git()` isn't initialized there, so `execFileSync`): when the script's folder has a git common dir, `process.argv[2]` isn't `help`/`--help`, and the real path of `/docs/devteam/scripts` (`--show-toplevel` added to that block's `git rev-parse`) isn't the script's folder, write spec §7's text to stderr and exit 1. More cases in the test: a copy at `/vendor/kit`, and the nested copy beside an empty `.git` folder. 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 + +- [ ] **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. +- [ ] **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 + +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/pr-body.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/pr-body.md new file mode 100644 index 0000000..0d57e34 --- /dev/null +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/pr-body.md @@ -0,0 +1,24 @@ +Kit tests run on a devteam box or inside a worker can no longer reach the real fleet (incident p-20261114-214815998-supervisor: a kit test run from kit-46's worktree wrote ~25 lines into home's live log). + +## What changes + +- **One env scrub for the tests** (`test/lib/kit.mjs`): at import, every `DEVTEAM_*` but the harness's own knobs (`HARNESS_ENV`: `DEVTEAM_TEST_*`, `FORK`, `FORK_AUDIT`, `G1_RECORD`, `CGROUP`, `MEMINFO`) and `TMUX`/`TMUX_PANE` are deleted, then `DEVTEAM_BOX=off` is set. The per-file denylists in machine, operator-threads, hub-overview and gateway tests go; gateway.test.mjs imports the helper; hub-overview's kit gets a temp `HOME`. +- **`DEVTEAM_BOX=off`** (devteam.mjs, box detection): the kit ignores the box's files (`/opt/devteam/gateway.mjs`, `/etc/devteam/thread-key.pub`) only while `HOME`, by real path, isn't the account's passwd home; anything unresolvable leaves the files on. Box detection is a marked block, loaded in a vm by `boxFns()` and tested in CI. +- **The kit refuses a foreign run folder**: a `DEVTEAM_RUN` whose real path isn't the default run folder's or inside the resolved home (`mainRoot`) dies naming both paths, for every command but `help` and the machine-wide ones. +- **A kit copy that isn't its checkout's own `docs/devteam`** (by the real path of `git rev-parse --show-toplevel`, asked without the caller's `GIT_DIR`) dies before the re-exec instead of taking that checkout's home; `bareProject` asserts each fixture is a repo of its own. + +## How to verify + +- `docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/verification.md`: a row per done-when, with commands and results. +- The decoy: `env -u DEVTEAM_HOME DEVTEAM_ROLE=worker DEVTEAM_RUN= node --test --test-name-pattern 'status says at a glance' test/machine.test.mjs` from the kit's checkout writes the owner rule into `/log/owner-2026-11.jsonl` at main, and nothing after this change (it is now a test in machine.test.mjs). +- On a devteam box: operator-threads went from 15 of 16 failing at main to all passing; machine and hub-overview pass. + +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). Nor a DEVTEAM_RUN set on purpose to some other folder inside home. + +Note: `test/lib/ports.mjs:22` still says the gateway test claims no port block; it now claims one through `lib/kit.mjs` (ports.mjs is outside this ticket's touches). + +## One-way doors + +None. + +🤖 Generated with [Claude Code](https://claude.com/claude-code) diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/final-1.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/final-1.md new file mode 100644 index 0000000..e90860e --- /dev/null +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/final-1.md @@ -0,0 +1,37 @@ +# Final review, pass 1 + +- Reviewer: final-reviewer (fresh subagent, claude-opus) +- Reviewed: 65fd63c45c6bc55015437adf95932683a38f8ff6 (the whole branch) + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| F1 | Minor | spec acceptance 6 ("Exempt: help, and … `recover --machine`"); quality.md §3 (mutate where a test can't prove it) | devteam.mjs:483; machine.test.mjs "a kit refuses a run folder outside its home" (l.180) | The `recover --machine` exemption from the run-folder guard has no test. Mutation: I deleted `&& !(command === 'recover' && flags.machine)`, and the three run-folder tests still pass. The l.180 loop checks `help`, `machine status` and `operator-thread list`, but never `recover --machine`. The code is right (probe: `recover --machine --dry-run` with a foreign `DEVTEAM_RUN` exits 0). Fix: add `['recover', '--machine', '--dry-run', '--in', ]` to that loop. | wont-fix: Minor; the exemption is correct and was checked by hand twice; a test of recover --machine reaches the account's real projects | +| F2 | Minor | spec acceptance 1 (`HARNESS_ENV` names `DEVTEAM_FORK_AUDIT`, `DEVTEAM_G1_RECORD`) | test/lib/kit.mjs:23; machine.test.mjs "importing the test helper…" | The env-names test asserts that only `DEVTEAM_FORK`, `DEVTEAM_TEST_KEEP`, `DEVTEAM_CGROUP` and `DEVTEAM_MEMINFO` survive the scrub. Mutation: I dropped `FORK_AUDIT` and `G1_RECORD` from `HARNESS_ENV`, and the test still passes. Scrubbing them would quietly turn off `DEVTEAM_G1_RECORD=1` re-recording (components.test.mjs:22) and the fork audit (lib/chain.mjs:4856). Fix: seed and assert every listed knob. | wont-fix: Minor; CI and the fork harness would fail loudly if either knob were dropped | +| F3 | Minor | spec acceptance 7 (nested copy can't take home) | devteam.mjs:44-50 | If a kit copy sits inside home beside an invalid `.git` *file* (junk, or a stale gitfile from a removed worktree), git fails. `top` is then null and the check is skipped, as for a kit in no checkout. Run from home's cwd, homeAt falls back to `cwdCommon`, so the copy writes into home's log. Probe: `observe` from `/fx` with `fx/.git` = "nonsense" landed in `/docs/devteam/run/log`. This matches the spec's "a kit in no git checkout skips the check", which the box image relies on, and the case is contrived. The covered cases (no `.git`, empty `.git` folder, vendored copy) are refused. Noted, not blocking. | wont-fix: Minor; a broken .git file reads as no checkout, the rule the box image's kit relies on (spec §7) | + +## What I ran (review worktree `.worktrees/review-kit-201-65fd63c4-final` at 65fd63c, every test through `scripts/run-isolated.sh --test-reporter=tap`, one file at a time) + +- `test/machine.test.mjs --test-name-pattern='run folder|repo link|copied into|importing the test helper'`: 6 pass, 0 fail, 16 skipped by pattern. That includes the incident's decoy reproduction (unskipped: tmux is present), the refusal, take and repo-link tests, and the nested-copy test. +- `test/operator-threads.test.mjs` whole, on this box (both `/opt/devteam/gateway.mjs` and `/etc/devteam/thread-key.pub` exist): 17 pass, 0 fail, 1 skipped (Chrome). +- `test/hub-overview.test.mjs`: 2 pass, 1 skipped (Chrome). `test/gateway.test.mjs`: 69/69. +- CI at 65fd63c: `test` and `box-image` both completed with success. The PR head has since moved to 1690f6a, which only fills two Disposition cells in reviews/plan-2.md and reviews/spec-2.md, so per §3 this verdict still holds there. +- Touches: the diff touches only the six allowed files plus records under `docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/`. No file was added outside those records. +- Acceptance-1 grep: none of the four test files has a `DEVTEAM_*`/`TMUX` denylist left. speed.test.mjs and components.test.mjs still have theirs, but they are outside the files the spec names. + +Probes with the branch's kit on throwaway fixtures, with the session env removed: +- A foreign `DEVTEAM_RUN` dies with exactly the spec's message, and the other run folder is unchanged. So does a symlink inside home pointing at another home's run, and a not-yet-made path under such a symlink. +- Each of these is taken: a relative `DEVTEAM_RUN`, a trailing slash, a symlinked checkout path (also with `--preserve-symlinks-main`), and `rebase --exec` in home. +- `help`, `--help`, no command, and `recover --machine` are never refused. +- A nested copy with no `.git` is refused even with cwd outside home. +- On this box `$HOME`, `os.homedir()` and the passwd home are all `/data/home` (Dockerfile `usermod -d /data/home`). `HOME` unset falls back to passwd, so `DEVTEAM_BOX=off` is ignored at the real home. + +I also read the dashboard and machine-wide spawns: `projectKit` and `recover --machine` delete `DEVTEAM_RUN` before running another repo's kit. Tmux windows get home's own `runDir`. The box boot runs `/opt/devteam/kit`, which is in no repo, with machine-wide commands only. `check-pr`'s `validate --strict` runs in a detached worktree of home, so it stays inside home. + +Mutations (in the review worktree, reverted after each): +- caught: `boxFilesOff` comparing `HOME` strings instead of real paths (operator-threads box test fails); `lib/kit.mjs` not setting `DEVTEAM_BOX=off` (15 of 18 operator-threads tests fail on this box). +- survived: dropping the `recover --machine` exemption (F1); dropping `FORK_AUDIT` and `G1_RECORD` from `HARNESS_ENV` (F2). +- Not re-run: the guard removed, a prefix check, no real paths, the nested check removed or moved, the `GIT_DIR` scrub removed, and `??` for `||`. reviews/task-3*.md records these; I read the tests that catch each one. + +No Critical or Important finding. No way found for the kit to take a foreign `DEVTEAM_RUN` through normal use, for a git-less nested copy to take home, for any legitimate setup listed to be refused, or for `DEVTEAM_BOX=off` to be honored at the account's real home. P6 and R1: the change stops the accidental paths, stays small, and the PR body says in one line what it doesn't defend against. + +Verdict: APPROVE 65fd63c45c6bc55015437adf95932683a38f8ff6 diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/final-2.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/final-2.md new file mode 100644 index 0000000..6c10cbb --- /dev/null +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/final-2.md @@ -0,0 +1,11 @@ +# Final review, pass 2 + +- Reviewer: final-confirmer (fresh subagent, claude-opus) +- Reviewed: e985ef22de034cddb432c7fd903f07b5234f28e8 (the diff since 65fd63c) + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| + +The diff 65fd63c..e985ef2 touches three files, all under `docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/`: `final-1.md` is new (the pass-1 record, verdict APPROVE 65fd63c), and `plan-2.md` and `spec-2.md` only fill the empty Disposition cells of P2-1, P2-2 and F2-1 to F2-4. In each changed row the Id, Severity, Rule, Where and Finding text are byte-identical, and no verdict or note paragraph changed. No code, test or workflow file changed, so the code and tests at the head are exactly what pass 1 approved. The three wont-fix dispositions in final-1.md are for Minor findings, which per quality.md §3 don't block, and each gives a reason (the exemption was checked by hand; a dropped knob would fail loudly; the case falls under the spec's no-checkout rule). CI: `test` and `box-image` completed with success at 65fd63c. The runs at e985ef2 are still in progress, which doesn't matter because the head differs only in review records. + +Verdict: APPROVE e985ef22de034cddb432c7fd903f07b5234f28e8 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/plan-2.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/plan-2.md new file mode 100644 index 0000000..81c9b84 --- /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_*`. | fixed: the env-names test compares only names outside HARNESS_ENV | +| 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. | fixed: the scrub is one regex loop, so the zero-hit grep finds nothing | + +**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 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/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..74e6f31 --- /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`). | fixed: the decoy test checks pass 1, fail 0 and an unskipped ok line, in TAP (plan Readings) | +| 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. | fixed: plan Step 2b names the expected matches | +| 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. | fixed: superseded by the corrected limit section (plan Readings) | +| 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. | fixed: boxFns supplies previewBox's block and crypto | + +**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 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/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/reviews/spec-5.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-5.md new file mode 100644 index 0000000..284540c --- /dev/null +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-5.md @@ -0,0 +1,35 @@ +# Spec and plan review (scope change), pass 3 + +- Reviewer: rulings-confirmer (fresh subagent, claude-opus) +- Reviewed: e9cae1315d0ae80ed7f6ee7833ccb16a9c57cef9 + +Read: spec-4.md, the diff 3a4cf60..e9cae13 (spec §2, §6, §7, Rulings; plan Task 3), and in devteam.mjs at e9cae13 the re-exec block (34-47), `git()` (133-144), homeAt (405-447), `runDir` (463) and `cwdRepo` (2372). I also checked every `cpSync(kit…)` in test/ and cloud/Dockerfile. I ran no kit commands or tests. + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| S5-1 | Important | m-20261114-224904663-supervisor ("a kit copy nested in a home without its own .git must not resolve that home"); spec §7 | plan Task 3 Step 3b "beside homeAt (before it reads home)"; devteam.mjs 34-47 | The re-exec block runs before homeAt. For a nested copy (no `.git` of its own) it takes home's common dir, so `kitMain` is home's `docs/devteam`, which has `project/`. If the copy's bytes differ from home's main kit, it re-execs into home's main kit. That kit passes Step 3b's check (it is its checkout's own) and runs against home's project and live log. This is the incident class: a branch's changed kit copied into a fixture without `git init`. Step 3b's test can't see the hole, because `home` and the copy both come from `kit`, so they are identical and no re-exec happens. Fix: apply the same "script is its checkout's own kit" test in the re-exec block, so a nested copy is never re-exec'd and dies, or skips the re-exec and then dies at Step 3b. Use `execFileSync` there, not `git()`: `git()` reads `let gitReadCache` (126), which is in its TDZ at line 40. Add a test case where the copy differs from home's kit (append a comment to the copied devteam.mjs). | fixed: the check moved into the re-exec block; tested with a copy that differs | +| S5-2 | Important | quality §3 (a disposition that doesn't fix its finding); S4-3 | plan Task 3 Step 1 "includes `app-api` and `app`" | S4-3 is dispositioned `fixed` ("the test asserts the home as '()'"), but the committed plan still says "includes `app-api` and `app`" and the regex `\(.*\)`. The spec has no exact-path assertion either. If the fix is only in uncommitted test code, the record doesn't carry it. Write the exact assertion into the plan: `DEVTEAM_RUN=${apiRun}` and `(${app})`, or their real paths. | fixed: exact message prefix with both paths, in spec, plan and test | +| S5-3 | Important | m-20261114-224640331-supervisor ("a worker on a repo link"); S4-2 | plan Task 3 Step 1 "a plain repo `/linked` with a worktree"; spec §6 Must not break | S4-2 asked for a repo link of home, named in its settings, and the spec now says "a worker on a repo link". The plan's `linked` is a plain repo that home's settings don't name. So `cwdRepo()` (2372) returns null and `gate`, `phase` and `check-ticket` take the non-link paths (no `repo` field, no link gates, no `linkLocation`/`links.local.json`), not the ones kit-46's worker takes. The plan also asserts only the sent line in home's run, while the spec asks for the phase, send, gate and merge-request lines. Fix: register `linked` as a `kind: repo` link in home's settings (remote = `linked`'s path or origin, location so `repoReadable` holds), run from its worktree, and assert each line in home's run log (the gate line with `repo: `). | fixed: linked is a real repo link in home's settings; phase, gate and both sends checked in home's log | +| S5-4 | Minor | consistency | spec §6 "Not defended (P6): … or a kit copied into home's own tree" | §7 now defends a kit copied into home's tree without a repo of its own, so this clause contradicts it. Narrow it to "with `DEVTEAM_OWN_KIT`", or drop it. | fixed: the nested copy is no longer listed as undefended | +| S5-5 | Minor | spec §7 clarity | spec §7 "When the running kit's script is not `/…`" | Plan Step 3b skips the check when there is no top level ("when it is set"). That is what keeps a kit in no repo working (the box image's `/opt/devteam/kit`, cloud/Dockerfile 122; a machine without git). The spec doesn't say so. Add "a kit in no git checkout is unaffected", and a test for it if it's cheap (a copy under os.tmpdir with no `git init`, `help` or `machine`). | fixed: a kit in no git checkout skips, said in §7 | + +Notes: + +- S4-1: fixed. Both spec and plan seed with `observe`. +- S4-2: partly fixed. Acceptance 6 has the must-not-break and the plan has its own test, but the fixture isn't a repo link (S5-3). +- S4-3: not fixed in the committed docs (S5-2). +- S4-4: fixed. The spec names the fresh `app-ln` fixture. +- Ruling m-20261114-224640331: in Rulings and acceptance 6, with a plan step. Fidelity is S5-3. +- Ruling m-20261114-224904663, decoy: spec §2 reproduces it exactly (cwd the kit's checkout, `DEVTEAM_HOME` unset, worker, `DEVTEAM_RUN=`, the name pattern, an empty decoy). Plan Step 4 matches and has workable timeouts. +- Ruling m-20261114-224904663, nested kit: in acceptance 7 and Step 3b, and the `bareProject` assertion is there. The re-exec hole is S5-1. +- Is Step 3b safe? `git()` is a hoisted function, and `gitReadCache`/`HOME_CWD` are initialized before homeAt, so the call works there and returns null on any git failure, which skips the check. I checked the setups the brief named: + - A home main checkout, and a home worktree's own copy before the re-exec: the top level is that checkout, so the check passes. + - After the re-exec: home's main kit, so it passes. + - The kit's own repo (this worktree, `kit` = `/docs/devteam`): passes. + - review-worktrees under `.worktrees/` (each has its own `.git` file): pass. + - A kit in no repo (the box's `/opt/devteam/kit`; `/opt` and the box's tmp aren't in a repo): skipped. + - Every test fixture that copies the kit (kit.mjs, chain.mjs, machine, gateway, components, dashboard, dev-links, multirepo-home): `git init` or `clone` is run first, and the copy goes to `/docs/devteam`, so they all pass. + - The kit sets up no git hooks and no symlinked kits, so neither a `GIT_DIR`/`GIT_WORK_TREE` from a hook nor a symlinked `docs/devteam` arises. + - A kit vendored as a git submodule, or at a non-top path in a monorepo, would now die, but homeAt already resolves those wrongly. That isn't a new break. + +Verdict: CHANGES e9cae1315d0ae80ed7f6ee7833ccb16a9c57cef9 diff --git a/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-6.md b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-6.md new file mode 100644 index 0000000..471c733 --- /dev/null +++ b/docs/devteam-dev/tickets/kit-201-tests-isolated-on-box/reviews/spec-6.md @@ -0,0 +1,21 @@ +# Spec and plan review (scope change), pass 4 + +- Reviewer: rulings-confirmer-2 (fresh subagent, claude-opus) +- Reviewed: 4aefda145e235e363a73894c0891588bad0ae00a + +Read: spec-5.md, the diff e9cae13..4aefda1 (spec §6 and §7; plan Task 3 Steps 1 and 3b), and the re-exec block in devteam.mjs at 4aefda1 (lines 34-56). I also checked every `cpSync(kit…)` in test/. I ran no kit commands or tests. + +| Id | Severity | Rule | Where | Finding | Disposition | +|---|---|---|---|---|---| +| S6-1 | Important | m-20261114-224904663-supervisor ("a kit copy nested in a home without its own .git must not resolve that home"); spec §7 | spec §7 "`