From 94ee93721364e87a80a99a3db4fe1881cc21ade0 Mon Sep 17 00:00:00 2001 From: Tobias Mogdans Date: Fri, 28 Aug 2026 07:29:44 +0200 Subject: [PATCH 1/2] fix(unskip): scope the test<->code seam to DEVLOOP-MANAGED tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit verify-unskip ran on every PR (only `devloop/spec/*` was exempt) and its TEST_FILE regex claimed EVERY test file in the target repo. Since a new file is allowed only if it has no active test, nobody could add an ordinary, active test file outside a spec branch. Live: bsk PR #157 contributes a plain dev tool (tools/dead-members) with its own node:test suite and collected 5 x "new test file contains an active (non-.skip) test" — a pure false alarm, while the approval half reported {"ok":true,"tier":"T1"}. The seam (§11, anti-collusion test<->code) is meant to protect the tests spec-to-tests derived from a reviewed spec, not every test in the repo. - core/managed-tests: parse `.devloop/managed-tests.json` (bare array or {globs:[...]}) and decide per path whether the seam binds it. - cli/verify-unskip: enforce the seam only on managed paths; report `managed`, `managedSource`, `checked` and `unmanaged` so a green run is never silently a disarmed one. - The list is read from the BASE ref first — a PR cannot widen its own exemption; only when the file is absent on base does the checkout count (adoption PR, which necessarily touches the protected set). - Config file, NOT branch name: an agent picks its own branch name, so a branch-coupled exemption is self-serve. `.devloop/**` is in the protected set, and verify-review fails a protected-set touch unconditionally and BEFORE the unskip step runs. - Fail-closed & backwards compatible: absent, empty, malformed or wrong-shaped config => every test file is managed = pre-v0.8.1 behaviour. No adopter gets looser by upgrading. An EXPLICIT `[]` is honoured. - init scaffolds the skeleton at its fail-closed default (`globs: ["**"]`) and notes loudly that it must be narrowed. - git stderr silenced: `git show :` legitimately fails for every added file; the "fatal: path ... does not exist" noise is not a finding. 188 tests green (was 174), tsc clean, dist rebuilt in sync. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BFcJSagot15i7RfhN9hkwp --- USAGE.md | 13 +- dist/cli/verify-unskip.js | 50 ++++++- dist/core/init.js | 15 ++ dist/core/managed-tests.js | 55 +++++++ ...d-verify-unskip-fehlalarm-managed-tests.md | 56 ++++++++ src/cli/verify-unskip.ts | 61 +++++++- src/core/init.ts | 23 +++ src/core/managed-tests.ts | 58 ++++++++ test/cli/verify-unskip.test.ts | 136 +++++++++++++++++- test/core/init.test.ts | 8 ++ test/core/managed-tests.test.ts | 52 +++++++ 11 files changed, 517 insertions(+), 10 deletions(-) create mode 100644 dist/core/managed-tests.js create mode 100644 docs/2026-08-28-befund-verify-unskip-fehlalarm-managed-tests.md create mode 100644 src/core/managed-tests.ts create mode 100644 test/core/managed-tests.test.ts diff --git a/USAGE.md b/USAGE.md index 9d98f03..31040fd 100644 --- a/USAGE.md +++ b/USAGE.md @@ -23,7 +23,7 @@ CI + Branch-Protection. ``` Das legt an: den CI-Workflow `devloop-precondition-check.yml` und das Config-Skeleton -`.devloop/{tier-map,protected-globs,bot-logins}.json`. +`.devloop/{tier-map,protected-globs,bot-logins,managed-tests}.json`. Danach **von Hand** (das ist der Anker, der Selbst-Freigabe verhindert): @@ -43,6 +43,11 @@ Danach **von Hand** (das ist der Anker, der Selbst-Freigabe verhindert): Admin-Override statt eines normalen Reviews. - `.devloop/bot-logins.json` — die GitHub-Login(s) **deines Agenten** (damit seine „Approvals" nie als Mensch zählen). + - `.devloop/managed-tests.json` — **welche Testpfade devloop verwaltet** (Glob-Liste). Nur für + die gilt die Unskip-Naht (§11). Das Skeleton ist `["**"]` = *jede* Testdatei; **eng ziehen** + auf die Pfade, die `spec-to-tests` wirklich autort (deine Käfig-Services / Twin-Areas), + sonst blockiert `verify-unskip` jeden gewöhnlichen PR, der eine normale aktive Testdatei + hinzufügt. Fehlt die Datei (oder ist sie kaputt), gilt fail-closed „alles verwaltet". 5. **CODEOWNERS = das §9-Merge-Tor (wichtig).** CODEOWNERS muss decken: - das **Spec-Verzeichnis** (z.B. `/.specify/specs/`) → erzwingt den Spec-Review-Stopp, tier-unabhängig; - **alle T2/T3-Pfade aus deiner `tier-map`** (auth, migrations, contracts, `src/**`, …). @@ -67,6 +72,12 @@ Danach **von Hand** (das ist der Anker, der Selbst-Freigabe verhindert): > nicht Folklore (`describe.skip` um aktive `it` trifft **beide** Wächter und ist tabu). 8. **`verify-unskip`** als Required Check auf dem Implementierungs-PR wiren (im CI-Template enthalten) — erzwingt, dass `implement` an Tests nur `.skip` entfernt. + > **Umfang setzen (sonst Fehlalarm):** Die Naht gilt für die Pfade in + > `.devloop/managed-tests.json`. Steht dort das Skeleton `["**"]`, gilt sie für **jede** + > Testdatei im Repo — dann kann auch ein PR, der mit devloop nichts zu tun hat (ein + > Dev-Werkzeug mit eigenen Tests), keine aktive Testdatei mehr hinzufügen. Zieh die Liste auf + > deine Käfig-Services eng. Maßgeblich ist die Liste auf dem **Basis-Branch**, nicht die im + > PR — ein PR kann sich die Ausnahme nicht selbst ausstellen. > Prüfen, ob alle Wächter stehen: `devloop check-guardians ` (exit 0 = bereit). diff --git a/dist/cli/verify-unskip.js b/dist/cli/verify-unskip.js index f6fe43e..4dc054b 100644 --- a/dist/cli/verify-unskip.js +++ b/dist/cli/verify-unskip.js @@ -3,9 +3,18 @@ // - an EXISTING test file may change ONLY by removing `.skip` (implement PR). // implement can thus neither author active tests nor edit existing ones. Fail-closed (exit 1). // -// Usage: verify-unskip (baseRef e.g. origin/main) +// SCOPE: the seam applies to the tests devloop MANAGES (those `spec-to-tests` derives from a +// reviewed spec), not to every test file in the repo. The target repo declares them in +// `.devloop/managed-tests.json` (glob list) — a file inside the protected set, so an agent +// cannot widen it. No such file => every test file is managed (today's behaviour, fail-closed). +// Deliberately NOT branch-name-driven: an agent chooses its own branch name. +// +// Usage: verify-unskip [headBranch] (baseRef e.g. origin/main) import { execFileSync } from "node:child_process"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; import { isAllowedTestEdit, isSpecBranch } from "../core/unskip.js"; +import { MANAGED_TESTS_PATH, isManagedTestPath, parseManagedTestGlobs, } from "../core/managed-tests.js"; const repo = process.argv[2] ?? "."; const base = process.argv[3] ?? "origin/main"; const headBranch = process.argv[4] ?? ""; @@ -17,16 +26,40 @@ if (isSpecBranch(headBranch)) { } const gitSafe = (args) => { try { - return execFileSync("git", ["-C", repo, ...args], { encoding: "utf8" }); + // stderr ignored: `git show :` legitimately fails for every file the PR adds + // (and for an absent config) — the "fatal: path ... does not exist" noise is not a finding. + return execFileSync("git", ["-C", repo, ...args], { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + }); } catch { return ""; } }; const TEST_FILE = /\.(test|spec)\.[jt]sx?$/; -const changed = gitSafe(["diff", "--name-only", `${base}...HEAD`]) +// The glob list is read from the BASE ref first: the scope that is already on the protected +// branch decides, so a PR cannot loosen the seam for itself. Only if the file does not exist on +// base do we fall back to the checkout (a repo adopting the file — that PR necessarily touches +// `.devloop/**` = the protected set and is already flagged by verify-review). +const readWorktree = (rel) => { + try { + return readFileSync(join(repo, rel), "utf8"); + } + catch { + return ""; + } +}; +const fromBase = gitSafe(["show", `${base}:${MANAGED_TESTS_PATH}`]); +const fromWorktree = fromBase === "" ? readWorktree(MANAGED_TESTS_PATH) : ""; +const managedRaw = fromBase !== "" ? fromBase : fromWorktree; +const managedSource = fromBase !== "" ? "base" : fromWorktree !== "" ? "worktree" : "default"; +const managedGlobs = parseManagedTestGlobs(managedRaw); +const testFiles = gitSafe(["diff", "--name-only", `${base}...HEAD`]) .split("\n") .filter((f) => TEST_FILE.test(f)); +const changed = testFiles.filter((f) => isManagedTestPath(f, managedGlobs)); +const unmanaged = testFiles.filter((f) => !isManagedTestPath(f, managedGlobs)); const violations = []; for (const file of changed) { const oldContent = gitSafe(["show", `${base}:${file}`]); // "" if the file is new @@ -41,5 +74,14 @@ for (const file of changed) { } } const ok = violations.length === 0; -process.stdout.write(JSON.stringify({ ok, base, checked: changed, violations }, null, 2) + "\n"); +process.stdout.write(JSON.stringify({ + ok, + base, + // Which scope was in force — printed so a green run is never silently a disarmed one. + managed: managedGlobs === null ? "all-test-files (no .devloop/managed-tests.json)" : managedGlobs, + managedSource, + checked: changed, + unmanaged, + violations, +}, null, 2) + "\n"); process.exit(ok ? 0 : 1); diff --git a/dist/core/init.js b/dist/core/init.js index 4febd8a..ad942b2 100644 --- a/dist/core/init.js +++ b/dist/core/init.js @@ -18,6 +18,17 @@ function tierGlobToCodeownersPath(glob) { } // Project-agnostic skeletons — the adopter tunes them in their constitution/CI. const PROTECTED_GLOBS_SKELETON = JSON.stringify(["**/stryker.conf.*", "**/.semgrep/**", "**/constitution.md", "**/CODEOWNERS", "**/*.eslintrc*"], null, 2); +// Which test paths the unskip seam (§11) binds. The skeleton is the FAIL-CLOSED default — +// `**` = every test file, i.e. exactly the pre-v0.8.1 behaviour. Narrow it to the paths +// spec-to-tests actually authors (the caged services / twin areas), otherwise a plain PR that +// adds an ordinary active test file cannot pass the guard. Lives in `.devloop/` = the protected +// set, so an agent cannot widen it to smuggle its own tests past the seam. +const MANAGED_TESTS_SKELETON = JSON.stringify({ + _comment: "Globs of the DEVLOOP-MANAGED test files (written by spec-to-tests, only un-skippable by implement). " + + "Narrow these to your caged areas; ordinary tests outside them are not bound by the unskip seam. " + + "Delete this file to fall back to 'every test file is managed'.", + globs: ["**"], +}, null, 2); // Anchor (b): identities that are NOT human (the agent's bot/app account). A review by any // of these never counts as a human approval. The adopter fills in their agent's login(s). const BOT_LOGINS_SKELETON = JSON.stringify(["devloop-agent[bot]"], null, 2); @@ -55,6 +66,10 @@ export function initRepo(targetRepo, ciTemplate, opts = {}) { } writeIfAbsent(".devloop/protected-globs.json", PROTECTED_GLOBS_SKELETON + "\n"); writeIfAbsent(".devloop/bot-logins.json", BOT_LOGINS_SKELETON + "\n"); + writeIfAbsent(".devloop/managed-tests.json", MANAGED_TESTS_SKELETON + "\n"); + result.notes.push(".devloop/managed-tests.json scopes the unskip seam (§11). The skeleton (`**`) binds EVERY " + + "test file — the fail-closed default. Narrow it to the paths spec-to-tests authors, else a " + + "normal PR adding an ordinary active test file is rejected by verify-unskip."); // Anchor (b) is the default: CI is authoritative. Recorded explicitly so the local merge // hook defers to CI instead of demanding the (anchor-a) local token. // anchor: CI is authoritative (b). twin: the optional spec-to-twin station, disabled by default diff --git a/dist/core/managed-tests.js b/dist/core/managed-tests.js new file mode 100644 index 0000000..05a4c8e --- /dev/null +++ b/dist/core/managed-tests.js @@ -0,0 +1,55 @@ +// WHICH test files does devloop actually manage? (the §11 test<->code seam, scoped) +// +// The unskip seam exists to protect the tests that `spec-to-tests` derived from a reviewed +// spec — the ones `implement` may neither author nor edit. It was never meant to claim EVERY +// test file in the target repo: a repo also holds ordinary, hand-written tests (a dev tool, a +// script, a legacy suite) that have nothing to do with a devloop run. Enforcing the seam on +// those makes it impossible to ADD a normal, active test outside a `devloop/spec/*` branch. +// +// The distinction comes from a CONFIG FILE IN THE TARGET REPO — `.devloop/managed-tests.json`, +// a glob list — deliberately NOT from the branch name: an agent picks its own branch name, so a +// branch-coupled exemption is self-serve. `.devloop/**` is part of the protected set +// (protected-globs.json), so an agent cannot widen the exemption list to smuggle its own tests +// past the seam — `verify-review` fails a diff that touches the protected set, unconditionally +// and independent of tier, BEFORE the unskip step ever runs. +// +// Fail-closed & backwards compatible: NO config file (or an unreadable/malformed one) means +// EVERY test file is managed — exactly today's behaviour. No existing adopter gets looser by +// upgrading; a repo opts into the narrower scope explicitly, in a file only a human can land. +import { matchesAnyGlob } from "./glob.js"; +/** Repo-relative path of the (optional) managed-test glob list. */ +export const MANAGED_TESTS_PATH = ".devloop/managed-tests.json"; +/** + * Parse `.devloop/managed-tests.json`. Accepts a bare array (like protected-globs.json / + * bot-logins.json) or `{ "globs": [...] }`. Anything else — absent, empty, invalid JSON, wrong + * shape, non-string entries — yields `null` (= manage everything), never a partial list: a + * half-understood config must not silently disarm the seam. + * + * An EXPLICIT empty array is honoured (`[]` = "no devloop-managed tests here"); it is a + * deliberate statement in a protected file, not an accident. + */ +export function parseManagedTestGlobs(raw) { + if (raw === null || raw === undefined || raw.trim() === "") + return null; + let parsed; + try { + parsed = JSON.parse(raw); + } + catch { + return null; // malformed -> fail closed + } + const globs = Array.isArray(parsed) + ? parsed + : typeof parsed === "object" && parsed !== null && Array.isArray(parsed.globs) + ? (parsed.globs) + : null; + if (globs === null) + return null; // wrong shape -> fail closed + if (!globs.every((g) => typeof g === "string")) + return null; // non-string entry -> fail closed + return globs; +} +/** Is this repo-relative test path devloop-managed (i.e. does the unskip seam apply to it)? */ +export function isManagedTestPath(path, globs) { + return globs === null ? true : matchesAnyGlob(path, globs); +} diff --git a/docs/2026-08-28-befund-verify-unskip-fehlalarm-managed-tests.md b/docs/2026-08-28-befund-verify-unskip-fehlalarm-managed-tests.md new file mode 100644 index 0000000..124fcee --- /dev/null +++ b/docs/2026-08-28-befund-verify-unskip-fehlalarm-managed-tests.md @@ -0,0 +1,56 @@ +# Befund: `verify-unskip` blockiert jede gewöhnliche neue Testdatei (v0.8.0) — Fix in v0.8.1 + +## Der Fehlalarm, live aufgelaufen + +`src/cli/verify-unskip.ts` lief auf **jedem** PR des Ziel-Repos; die einzige Ausnahme war +`isSpecBranch(headBranch)` (`devloop/spec/*` → No-op). `TEST_FILE = /\.(test|spec)\.[jt]sx?$/` +erfasste **jede** Testdatei im Repo, und für neue Dateien gilt +`isAllowedTestEdit("", neu) === !hasActiveTest(neu)`. + +Folge: **außerhalb eines `devloop/spec/*`-Branches konnte niemand eine gewöhnliche, aktive +Testdatei hinzufügen.** Aufgelaufen am bsk-PR #157, der ein reines Entwickler-Werkzeug +(`tools/dead-members`) mit 32 eigenen `node:test`-Tests beisteuert — mit devloop hat das nichts +zu tun. Verdikt: 5 × `"new test file contains an active (non-.skip) test"`, während der +Freigabe-Teil sauber `{"ok":true,"tier":"T1"}` meldete. + +Die Ursache ist eine **Annahme**, keine Regel: der Wächter unterstellte, dass *alle* Tests im +Repo devloop-verwaltet sind. Die Naht (§11, Anti-Kollusion Test↔Code) soll aber nur die aus der +Spec abgeleiteten Tests schützen — die, die `spec-to-tests` geschrieben hat und `implement` +weder autoren noch ändern darf. + +## Der Fix: verwaltete vs. gewöhnliche Testpfade + +Neu: `.devloop/managed-tests.json` im Ziel-Repo — eine **Glob-Liste** der devloop-verwalteten +Testpfade. Nur für Treffer gilt die Naht; alles andere ist eine gewöhnliche Testdatei und wird +durchgelassen. + +```jsonc +// .devloop/managed-tests.json — bare array oder { "globs": [...] } +["services/*/src/**/*.test.ts", "services/*/twin/**/*.test.ts"] +``` + +Drei Eigenschaften, die den Fix tragen: + +1. **Konfigurationsdatei, nicht Branch-Name.** Ein Agent wählt seinen Branch-Namen selbst; eine + branchnamen-gekoppelte Ausnahme wäre Selbstbedienung (dieselbe Schwäche hat die + Semgrep-Fluchttür des Pilot-Repos an `devloop/spec/*`). `.devloop/**` steht dagegen im + **geschützten Satz** (`protected-globs.json`) — `verify-review` failt bei jeder Berührung, + **tier-unabhängig und vor** dem Unskip-Schritt. +2. **Basis-Ref entscheidet.** Die Liste wird zuerst aus `:.devloop/managed-tests.json` + gelesen: der auf dem geschützten Branch bereits gelandete Umfang gilt, ein PR kann die Naht + also nicht für sich selbst lockern. Nur wenn die Datei auf `base` **nicht existiert**, zählt + der Checkout (Adoptions-PR — und der berührt zwangsläufig den geschützten Satz). +3. **Fail-closed & rückwärtskompatibel.** Datei fehlt, ist leer, kein gültiges JSON, falsch + geformt oder enthält Nicht-Strings → **jede** Testdatei ist verwaltet, also exakt das + Verhalten vor v0.8.1. Kein bestehendes Ziel-Repo wird durch den Bump still lockerer. Eine + **explizit leere** Liste (`[]`) wird respektiert — das ist eine bewusste Aussage in einer + geschützten Datei. + +`init` legt das Skeleton mit `globs: ["**"]` an (= heutiges Verhalten) und **notiert laut**, +dass es einzuengen ist. Der Ausgabe-JSON von `verify-unskip` nennt jetzt `managed` und +`managedSource` — ein grüner Lauf ist damit nie still ein entwaffneter. + +## Was NICHT geändert wurde + +Das bestehende `isSpecBranch`-No-op bleibt. Es ist dieselbe Schwäche (branchnamen-gekoppelt), +aber ein eigener Befund — hier zu ändern hieße, zwei Dinge in einem Diff zu vermischen. diff --git a/src/cli/verify-unskip.ts b/src/cli/verify-unskip.ts index 2d38b2c..39bff14 100644 --- a/src/cli/verify-unskip.ts +++ b/src/cli/verify-unskip.ts @@ -3,10 +3,23 @@ // - an EXISTING test file may change ONLY by removing `.skip` (implement PR). // implement can thus neither author active tests nor edit existing ones. Fail-closed (exit 1). // -// Usage: verify-unskip (baseRef e.g. origin/main) +// SCOPE: the seam applies to the tests devloop MANAGES (those `spec-to-tests` derives from a +// reviewed spec), not to every test file in the repo. The target repo declares them in +// `.devloop/managed-tests.json` (glob list) — a file inside the protected set, so an agent +// cannot widen it. No such file => every test file is managed (today's behaviour, fail-closed). +// Deliberately NOT branch-name-driven: an agent chooses its own branch name. +// +// Usage: verify-unskip [headBranch] (baseRef e.g. origin/main) import { execFileSync } from "node:child_process"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; import { isAllowedTestEdit, isSpecBranch } from "../core/unskip.js"; +import { + MANAGED_TESTS_PATH, + isManagedTestPath, + parseManagedTestGlobs, +} from "../core/managed-tests.js"; const repo = process.argv[2] ?? "."; const base = process.argv[3] ?? "origin/main"; @@ -21,7 +34,12 @@ if (isSpecBranch(headBranch)) { const gitSafe = (args: string[]): string => { try { - return execFileSync("git", ["-C", repo, ...args], { encoding: "utf8" }); + // stderr ignored: `git show :` legitimately fails for every file the PR adds + // (and for an absent config) — the "fatal: path ... does not exist" noise is not a finding. + return execFileSync("git", ["-C", repo, ...args], { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + }); } catch { return ""; } @@ -29,10 +47,30 @@ const gitSafe = (args: string[]): string => { const TEST_FILE = /\.(test|spec)\.[jt]sx?$/; -const changed = gitSafe(["diff", "--name-only", `${base}...HEAD`]) +// The glob list is read from the BASE ref first: the scope that is already on the protected +// branch decides, so a PR cannot loosen the seam for itself. Only if the file does not exist on +// base do we fall back to the checkout (a repo adopting the file — that PR necessarily touches +// `.devloop/**` = the protected set and is already flagged by verify-review). +const readWorktree = (rel: string): string => { + try { + return readFileSync(join(repo, rel), "utf8"); + } catch { + return ""; + } +}; +const fromBase = gitSafe(["show", `${base}:${MANAGED_TESTS_PATH}`]); +const fromWorktree = fromBase === "" ? readWorktree(MANAGED_TESTS_PATH) : ""; +const managedRaw = fromBase !== "" ? fromBase : fromWorktree; +const managedSource = fromBase !== "" ? "base" : fromWorktree !== "" ? "worktree" : "default"; +const managedGlobs = parseManagedTestGlobs(managedRaw); + +const testFiles = gitSafe(["diff", "--name-only", `${base}...HEAD`]) .split("\n") .filter((f) => TEST_FILE.test(f)); +const changed = testFiles.filter((f) => isManagedTestPath(f, managedGlobs)); +const unmanaged = testFiles.filter((f) => !isManagedTestPath(f, managedGlobs)); + const violations: { file: string; reason: string }[] = []; for (const file of changed) { const oldContent = gitSafe(["show", `${base}:${file}`]); // "" if the file is new @@ -49,5 +87,20 @@ for (const file of changed) { } const ok = violations.length === 0; -process.stdout.write(JSON.stringify({ ok, base, checked: changed, violations }, null, 2) + "\n"); +process.stdout.write( + JSON.stringify( + { + ok, + base, + // Which scope was in force — printed so a green run is never silently a disarmed one. + managed: managedGlobs === null ? "all-test-files (no .devloop/managed-tests.json)" : managedGlobs, + managedSource, + checked: changed, + unmanaged, + violations, + }, + null, + 2, + ) + "\n", +); process.exit(ok ? 0 : 1); diff --git a/src/core/init.ts b/src/core/init.ts index d95f8a8..3730084 100644 --- a/src/core/init.ts +++ b/src/core/init.ts @@ -37,6 +37,23 @@ const PROTECTED_GLOBS_SKELETON = JSON.stringify( 2, ); +// Which test paths the unskip seam (§11) binds. The skeleton is the FAIL-CLOSED default — +// `**` = every test file, i.e. exactly the pre-v0.8.1 behaviour. Narrow it to the paths +// spec-to-tests actually authors (the caged services / twin areas), otherwise a plain PR that +// adds an ordinary active test file cannot pass the guard. Lives in `.devloop/` = the protected +// set, so an agent cannot widen it to smuggle its own tests past the seam. +const MANAGED_TESTS_SKELETON = JSON.stringify( + { + _comment: + "Globs of the DEVLOOP-MANAGED test files (written by spec-to-tests, only un-skippable by implement). " + + "Narrow these to your caged areas; ordinary tests outside them are not bound by the unskip seam. " + + "Delete this file to fall back to 'every test file is managed'.", + globs: ["**"], + }, + null, + 2, +); + // Anchor (b): identities that are NOT human (the agent's bot/app account). A review by any // of these never counts as a human approval. The adopter fills in their agent's login(s). const BOT_LOGINS_SKELETON = JSON.stringify(["devloop-agent[bot]"], null, 2); @@ -86,6 +103,12 @@ export function initRepo( writeIfAbsent(".devloop/protected-globs.json", PROTECTED_GLOBS_SKELETON + "\n"); writeIfAbsent(".devloop/bot-logins.json", BOT_LOGINS_SKELETON + "\n"); + writeIfAbsent(".devloop/managed-tests.json", MANAGED_TESTS_SKELETON + "\n"); + result.notes.push( + ".devloop/managed-tests.json scopes the unskip seam (§11). The skeleton (`**`) binds EVERY " + + "test file — the fail-closed default. Narrow it to the paths spec-to-tests authors, else a " + + "normal PR adding an ordinary active test file is rejected by verify-unskip.", + ); // Anchor (b) is the default: CI is authoritative. Recorded explicitly so the local merge // hook defers to CI instead of demanding the (anchor-a) local token. // anchor: CI is authoritative (b). twin: the optional spec-to-twin station, disabled by default diff --git a/src/core/managed-tests.ts b/src/core/managed-tests.ts new file mode 100644 index 0000000..b3b00cf --- /dev/null +++ b/src/core/managed-tests.ts @@ -0,0 +1,58 @@ +// WHICH test files does devloop actually manage? (the §11 test<->code seam, scoped) +// +// The unskip seam exists to protect the tests that `spec-to-tests` derived from a reviewed +// spec — the ones `implement` may neither author nor edit. It was never meant to claim EVERY +// test file in the target repo: a repo also holds ordinary, hand-written tests (a dev tool, a +// script, a legacy suite) that have nothing to do with a devloop run. Enforcing the seam on +// those makes it impossible to ADD a normal, active test outside a `devloop/spec/*` branch. +// +// The distinction comes from a CONFIG FILE IN THE TARGET REPO — `.devloop/managed-tests.json`, +// a glob list — deliberately NOT from the branch name: an agent picks its own branch name, so a +// branch-coupled exemption is self-serve. `.devloop/**` is part of the protected set +// (protected-globs.json), so an agent cannot widen the exemption list to smuggle its own tests +// past the seam — `verify-review` fails a diff that touches the protected set, unconditionally +// and independent of tier, BEFORE the unskip step ever runs. +// +// Fail-closed & backwards compatible: NO config file (or an unreadable/malformed one) means +// EVERY test file is managed — exactly today's behaviour. No existing adopter gets looser by +// upgrading; a repo opts into the narrower scope explicitly, in a file only a human can land. + +import { matchesAnyGlob } from "./glob.js"; + +/** Repo-relative path of the (optional) managed-test glob list. */ +export const MANAGED_TESTS_PATH = ".devloop/managed-tests.json"; + +/** `null` = no usable configuration -> every test file is managed (fail-closed default). */ +export type ManagedTestGlobs = string[] | null; + +/** + * Parse `.devloop/managed-tests.json`. Accepts a bare array (like protected-globs.json / + * bot-logins.json) or `{ "globs": [...] }`. Anything else — absent, empty, invalid JSON, wrong + * shape, non-string entries — yields `null` (= manage everything), never a partial list: a + * half-understood config must not silently disarm the seam. + * + * An EXPLICIT empty array is honoured (`[]` = "no devloop-managed tests here"); it is a + * deliberate statement in a protected file, not an accident. + */ +export function parseManagedTestGlobs(raw: string | null | undefined): ManagedTestGlobs { + if (raw === null || raw === undefined || raw.trim() === "") return null; + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return null; // malformed -> fail closed + } + const globs = Array.isArray(parsed) + ? parsed + : typeof parsed === "object" && parsed !== null && Array.isArray((parsed as { globs?: unknown }).globs) + ? ((parsed as { globs: unknown[] }).globs) + : null; + if (globs === null) return null; // wrong shape -> fail closed + if (!globs.every((g) => typeof g === "string")) return null; // non-string entry -> fail closed + return globs as string[]; +} + +/** Is this repo-relative test path devloop-managed (i.e. does the unskip seam apply to it)? */ +export function isManagedTestPath(path: string, globs: ManagedTestGlobs): boolean { + return globs === null ? true : matchesAnyGlob(path, globs); +} diff --git a/test/cli/verify-unskip.test.ts b/test/cli/verify-unskip.test.ts index 89c20b5..65a243f 100644 --- a/test/cli/verify-unskip.test.ts +++ b/test/cli/verify-unskip.test.ts @@ -1,6 +1,6 @@ import { test, expect, beforeEach, afterEach } from "vitest"; import { execFileSync, spawnSync } from "node:child_process"; -import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { fileURLToPath } from "node:url"; import { dirname, join, resolve } from "node:path"; @@ -80,3 +80,137 @@ test("spec PR: a NEW file with an active test is blocked (exit 1)", () => { expect(r.status).toBe(1); expect(JSON.parse(r.stdout).violations[0].file).toBe("b.test.ts"); }); + +// --- scope: only DEVLOOP-MANAGED test paths are bound by the seam (.devloop/managed-tests.json) --- +// The seam protects the tests spec-to-tests derived from a reviewed spec — not every test file +// in the repo. Without the config file the old, all-encompassing behaviour stands (fail-closed). + +const MANAGED = ".devloop/managed-tests.json"; +const writeManaged = (globs: unknown) => { + mkdirSync(join(repo, ".devloop"), { recursive: true }); + writeFileSync(join(repo, MANAGED), JSON.stringify(globs, null, 2) + "\n"); +}; +const ACTIVE = (name: string) => + `import { test, expect } from "vitest";\ntest("${name} works", () => { expect(1).toBe(1); });\n`; + +test("an ORDINARY new test file (outside the managed globs) with ACTIVE tests passes — the fehlalarm", () => { + // the scope lands on main first (that is where the authoritative list is read from) + writeManaged(["services/**/*.test.ts"]); + git("add", "-A"); + commit("scope the devloop seam to the caged services"); + + git("checkout", "-q", "-b", "chore/tools-dead-members"); + mkdirSync(join(repo, "tools/dead-members"), { recursive: true }); + writeFileSync(join(repo, "tools/dead-members/scan.test.ts"), ACTIVE("scan")); + git("add", "-A"); + commit("chore: a plain dev tool with its own active tests"); + + const r = run(); + expect(r.status).toBe(0); + const out = JSON.parse(r.stdout); + expect(out.ok).toBe(true); + expect(out.violations).toEqual([]); + expect(out.unmanaged).toContain("tools/dead-members/scan.test.ts"); + expect(out.managedSource).toBe("base"); +}); + +test("a NEW file UNDER a managed glob with an active test still fails (the seam stays sharp)", () => { + writeManaged(["services/**/*.test.ts"]); + git("add", "-A"); + commit("scope the devloop seam to the caged services"); + + git("checkout", "-q", "-b", "devloop/smuggle"); + mkdirSync(join(repo, "services/forum-service"), { recursive: true }); + writeFileSync(join(repo, "services/forum-service/b.test.ts"), ACTIVE("REQ-B-1")); + git("add", "-A"); + commit("implement: smuggle an active test into the managed area"); + + const r = run(); + expect(r.status).toBe(1); + const out = JSON.parse(r.stdout); + expect(out.violations.map((v: { file: string }) => v.file)).toEqual(["services/forum-service/b.test.ts"]); +}); + +test("an EXISTING managed test file changed beyond removing .skip still fails", () => { + mkdirSync(join(repo, "services/forum-service"), { recursive: true }); + writeFileSync(join(repo, "services/forum-service/a.test.ts"), SKIPPED); + writeManaged(["services/**/*.test.ts"]); + git("add", "-A"); + commit("spec PR: managed skipped test + scope"); + + git("checkout", "-q", "-b", "devloop/impl"); + writeFileSync( + join(repo, "services/forum-service/a.test.ts"), + SKIPPED.replace("test.skip(", "test(").replace("toBe(42)", "toBe(0)"), + ); + git("add", "-A"); + commit("implement: tamper with a managed test"); + + const r = run(); + expect(r.status).toBe(1); + expect(JSON.parse(r.stdout).violations[0].file).toBe("services/forum-service/a.test.ts"); +}); + +test("WITHOUT the config file everything behaves exactly as before (fail-closed default)", () => { + git("checkout", "-q", "-b", "chore/tools-dead-members"); + mkdirSync(join(repo, "tools/dead-members"), { recursive: true }); + writeFileSync(join(repo, "tools/dead-members/scan.test.ts"), ACTIVE("scan")); + git("add", "-A"); + commit("chore: a plain dev tool with its own active tests"); + + const r = run(); + expect(r.status).toBe(1); // the pre-existing (over-broad) behaviour, unchanged + const out = JSON.parse(r.stdout); + expect(out.violations[0].file).toBe("tools/dead-members/scan.test.ts"); + expect(out.managedSource).toBe("default"); + expect(out.managed).toMatch(/all-test-files/); +}); + +test("the BASE list decides: a PR cannot widen its own exemption list", () => { + // base has a list; the PR tries to replace it with an all-exempting one -> base wins. + writeManaged(["**/*.test.ts"]); + git("add", "-A"); + commit("scope: manage every test file"); + + git("checkout", "-q", "-b", "devloop/widen"); + writeManaged(["nothing/matches/**"]); + writeFileSync(join(repo, "b.test.ts"), ACTIVE("REQ-B-1")); + git("add", "-A"); + commit("implement: widen the exemption AND smuggle an active test"); + + const r = run(); + expect(r.status).toBe(1); // the base list decided, not the PR's own + const out = JSON.parse(r.stdout); + expect(out.managedSource).toBe("base"); + expect(out.violations[0].file).toBe("b.test.ts"); +}); + +test("a malformed managed-tests.json falls back to managing everything (fail-closed)", () => { + mkdirSync(join(repo, ".devloop"), { recursive: true }); + writeFileSync(join(repo, MANAGED), '["services/**"\n'); // truncated JSON + git("add", "-A"); + commit("scope: broken config"); + + git("checkout", "-q", "-b", "chore/tools"); + mkdirSync(join(repo, "tools"), { recursive: true }); + writeFileSync(join(repo, "tools/scan.test.ts"), ACTIVE("scan")); + git("add", "-A"); + commit("chore: active test"); + + const r = run(); + expect(r.status).toBe(1); + expect(JSON.parse(r.stdout).managed).toMatch(/all-test-files/); +}); + +test("when base has no list, the PR checkout is the fallback source (adoption PR)", () => { + git("checkout", "-q", "-b", "chore/adopt-managed-tests"); + writeManaged(["services/**/*.test.ts"]); // introduced by THIS PR (touches the protected set) + mkdirSync(join(repo, "tools/dead-members"), { recursive: true }); + writeFileSync(join(repo, "tools/dead-members/scan.test.ts"), ACTIVE("scan")); + git("add", "-A"); + commit("chore: adopt the managed-test scope"); + + const r = run(); + expect(r.status).toBe(0); + expect(JSON.parse(r.stdout).managedSource).toBe("worktree"); +}); diff --git a/test/core/init.test.ts b/test/core/init.test.ts index 245e8fd..5db39ed 100644 --- a/test/core/init.test.ts +++ b/test/core/init.test.ts @@ -26,6 +26,14 @@ test("init writes the config skeleton (protected-globs + tier-map + bot-logins f expect(existsSync(join(repo, ".devloop/bot-logins.json"))).toBe(true); }); +test("init scaffolds the unskip-seam scope at its FAIL-CLOSED default (`**` = today's behaviour)", () => { + const r = initRepo(repo, TEMPLATE); + const raw = readFileSync(join(repo, ".devloop/managed-tests.json"), "utf8"); + expect(JSON.parse(raw).globs).toEqual(["**"]); // no adopter gets looser by upgrading + // the knob is only useful if the human is told to narrow it — say so, do not scaffold silently + expect(r.notes.join(" ")).toMatch(/managed-tests\.json/); +}); + test("init records the anchor explicitly as b (config.json), so the local hook defers to CI", () => { initRepo(repo, TEMPLATE); const cfgPath = join(repo, ".devloop/config.json"); diff --git a/test/core/managed-tests.test.ts b/test/core/managed-tests.test.ts new file mode 100644 index 0000000..4d921b9 --- /dev/null +++ b/test/core/managed-tests.test.ts @@ -0,0 +1,52 @@ +import { test, expect } from "vitest"; +import { + MANAGED_TESTS_PATH, + isManagedTestPath, + parseManagedTestGlobs, +} from "../../src/core/managed-tests.js"; + +// The seam (§11 test<->code separation) must bind the tests devloop MANAGES — the ones +// spec-to-tests derived from a reviewed spec — and nothing else. The scope comes from a config +// file inside the target repo's PROTECTED set, never from the branch name (self-serve). + +test("the config lives in the protected .devloop/ directory (an agent cannot widen it)", () => { + expect(MANAGED_TESTS_PATH.startsWith(".devloop/")).toBe(true); +}); + +// --- fail-closed: no usable config => everything is managed (today's behaviour) ------------- +test("absent config => every test file is managed (backwards compatible, fail-closed)", () => { + for (const raw of [null, undefined, "", " \n"]) { + expect(parseManagedTestGlobs(raw)).toBe(null); + expect(isManagedTestPath("tools/dead-members/x.test.ts", parseManagedTestGlobs(raw))).toBe(true); + } +}); + +test("malformed / wrong-shaped config => fail closed to 'manage everything', never a partial list", () => { + for (const raw of ['["services/**"', "not json", "42", '"a string"', "null", '{"other": ["x"]}', '["ok", 7]']) { + expect(parseManagedTestGlobs(raw)).toBe(null); + expect(isManagedTestPath("anything.test.ts", parseManagedTestGlobs(raw))).toBe(true); + } +}); + +// --- the two accepted shapes --------------------------------------------------------------- +test("accepts a bare array (like protected-globs.json) and the {globs:[…]} object form", () => { + expect(parseManagedTestGlobs('["services/**/*.test.ts"]')).toEqual(["services/**/*.test.ts"]); + expect(parseManagedTestGlobs('{"globs": ["services/**/*.test.ts"]}')).toEqual([ + "services/**/*.test.ts", + ]); +}); + +test("an EXPLICIT empty list is honoured (a deliberate statement in a protected file)", () => { + expect(parseManagedTestGlobs("[]")).toEqual([]); + expect(isManagedTestPath("a.test.ts", [])).toBe(false); +}); + +// --- scoping ------------------------------------------------------------------------------ +test("only paths matching a configured glob are managed", () => { + const globs = parseManagedTestGlobs('["services/*/src/**/*.test.ts", "services/*/twin/**"]'); + expect(isManagedTestPath("services/forum-service/src/a/b.test.ts", globs)).toBe(true); + expect(isManagedTestPath("services/forum-service/twin/model.test.ts", globs)).toBe(true); + // an ordinary dev tool's own tests are NOT devloop-managed -> the seam must not bind them + expect(isManagedTestPath("tools/dead-members/scan.test.ts", globs)).toBe(false); + expect(isManagedTestPath("scripts/x.spec.js", globs)).toBe(false); +}); From 009a298ad754aba8f9911bf38c7473b142fc2b4d Mon Sep 17 00:00:00 2001 From: Tobias Mogdans Date: Fri, 28 Aug 2026 07:29:44 +0200 Subject: [PATCH 2/2] =?UTF-8?q?chore(release):=20v0.8.1=20=E2=80=94=20unsk?= =?UTF-8?q?ip=20seam=20scoped=20to=20devloop-managed=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also ships 4a55f73 (per-service stryker config detection in monorepos), which landed on main after the v0.8.0 tag. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BFcJSagot15i7RfhN9hkwp --- .claude-plugin/plugin.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 046ccc4..46971b6 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "devloop", - "version": "0.8.0", + "version": "0.8.1", "description": "Generische agentische Dev-Loop-Kette: /devloop:specify|spec-to-tests|implement|critic|loop + Bindungs-Anker (Wächter-Vorbedingung, content-gebundene Stopps, CI-Required-Check). Projektübergreifend.", "author": { "name": "Tobias Mogdans" } } diff --git a/package.json b/package.json index 00259dc..8af5096 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "devloop", - "version": "0.8.0", + "version": "0.8.1", "private": true, "type": "module", "license": "MIT",