From 474bb2175cd577fd9e4105831d9419d315af2438 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 20:13:26 +0000 Subject: [PATCH] test: give the cross-stack contracts a named tier (RL-11) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Client/tests/unit/admin-static-channel-perms.test.ts` reads and executes `Server/admin/static/index.html`. Filed under `tests/unit`, nothing about its location or name said it locks a server-owned artifact, so a Go developer editing the admin SPA got a red check called "Client Unit Tests" with no clue why. The register describes this as one file. It is not, and the measured set does not match the description in either direction: - Client -> Server: exactly ONE test crosses by filesystem read, not two. `main-page.test.ts` was named in the plan but only carries a prose comment citing `Server/admin/update_handlers.go:181` at line 1046 — no read, no import, nothing to move. - Server -> Client: the four tests the plan named do not cross. `waf_test.go`/`waf_crs_test.go` set a `User-Agent: OwnCordClient/1.0` literal that appears nowhere under `Client/`; `ws_integration_test.go:289` and `sanitize_content_fuzz_test.go:46` are comments. The real crossing is one the register never named: `Server/updater/updater_test.go:630` does `os.ReadFile` on `Client/src-tauri/tauri.conf.json`. The obvious fixes are both wrong. Moving the invariant "to the owning server test" cannot work: `Server/go.mod` carries no JavaScript engine (no goja, otto, v8go, quickjs, rogchap, duktape), so a Go port could only assert at the text level like `admin/perm_grid_test.go` does — and that is not a substitute. Flipping the guard at `admin/static/index.html:1182` to `targetIsTouchedRole=false` reintroduces OC-0154 in full while leaving every greppable identifier intact, so a text-level test passes on a broken file. Relocating it to the e2e admin journey is worse: that job is `continue-on-error: true` and deliberately unpinned ("requiring it is theatre" — `docs/plans/b0-dev-branch-protection.sh`), so it would convert a blocking, pinned gate into one that is green regardless. And the journey does not cover the invariant today: `grep -Eic "perm|access|role|override|matrix"` over its 142 lines returns 0, so the "if e2e already covers it, delete" branch never fires. Done — one tier, applied to the whole set, defined by artifact coupling and placed by runtime capability: - New `Client/tests/contract/`, holding `server-admin-static-channel-perms.test.ts`. Same directory depth, so `../../../Server/...` still resolves; the body is byte-identical apart from a header naming the owner and the runner. - `Server/updater/tauri_key_contract_test.go` splits the one cross-component Go test out of `updater_test.go` verbatim, same `package updater`. It stays in Go — placement follows capability, and Go parses JSON fine — so only the file name has to declare the crossing. Without this the item would have been "moved one file and declared the class closed". - `npm run test:contract`, and the tier, the membership rule and a blocking/non-blocking table in `docs/contributing.md#testing`, which previously described no tiers at all. - `Client/CLAUDE.md`'s tier list was missing `tests/e2e/admin` and `tests/e2e/native` before this; it now lists all seven and states the rule. `Server/CLAUDE.md` records why the SPA's execution-level invariant is locked from the client tree, so nobody "fixes" it into a regex. - Ledger `OC-0154.fix.test` re-pointed and `FINDINGS.md` re-rendered; `.claude/workflows/bughunt.js` — the workflow that produced OC-0154 — no longer describes the TS test surface as `tests/unit/*.test.ts` only. - Three stale cross-stack pointers of exactly the class this item is about: `tests/e2e/helpers.ts:348,351` and `tests/unit/types.test.ts:13` named `docs/brain/06-Specs/PROTOCOL.md`, which does not exist (`docs/brain/` is a gitignored path); all now name `docs/protocol.md`. 15 files, 125 insertions, 33 deletions. No CI job, workflow, vitest, tsconfig, eslint, knip or stryker change, and no new pinned check — `ci.yml`'s `npx vitest run --coverage` has no path filter and `vitest.config.ts` includes `tests/**/*.test.ts`, so enforcement after the move is bit-identical to enforcement before it. That is deliberate: `dev` pins 11 contexts and a 12th is a branch-protection API write, not something a PR can do, so any new job would be advisory until someone separately changed repository settings — strictly less protection than today. Verified: both directions, and the assertion was not weakened. Flipping `admin/static/index.html:1182` to `const targetIsTouchedRole=false;` makes the moved test fail (`AssertionError: expected 'DELETE' not to be 'DELETE'`); `git checkout` of that file makes it pass again — so the invariant survived the move intact rather than becoming a test that passes anywhere. The split Go test's cross-boundary read is live too: with `Client/src-tauri/tauri.conf.json` moved away, `go test ./updater/` fails with `ReadFile(../../Client/src-tauri/tauri.conf.json): no such file or directory` from `tauri_key_contract_test.go:20`, and passes once restored. The full client suite is 192 files / 5257 tests passing, identical to the count before the move; `npm run typecheck` passes, which proves `tests/contract/` is inside the tsconfig graph and that `tests/types/jsdom.d.ts` still resolves the moved test's `import { JSDOM }`. `npm run lint`, `npx prettier --check .`, `go vet ./...` and `go test ./updater/` all pass. `git grep "tests/unit/admin-static-channel-perms"` finds no survivor outside the B1 plan itself. Not included: nothing was deleted, because no e2e sibling covers OC-0154. `Client/tests/types/jsdom.d.ts` was neither moved nor deleted — it is still the only type source for the moved test's `jsdom` import. `capabilities-scope.test.ts` and `tauri-conf-webview2-args.test.ts` read `src-tauri/` and stay in `tests/unit`: `src-tauri` is inside the `Client` component, so they are not contract tests, and the rule earns that rather than hand-waving it — moving them would have forced repoints of ledger entry OC-0089 and `docs/security.md:64` for no gain. Each gained a one-line header saying why. `Server/admin/perm_grid_test.go` and `emoji_section_test.go` read their own package's embedded asset and are unchanged; they are the text-level complement to the execution-level test, not duplicates. No JS engine was added to `go.mod`, no npm root was created under `Server/`, and no root-level `tests/` tier was created — there is no runner for one and no way to make it blocking from a PR. Separately noticed and NOT fixed here: `docs/contributing.md:221` still says "All ten required checks" while `docs/plans/b0-dev-branch-protection.sh` pins eleven since B1-3 added `Repository Hygiene`, and `docs/plans/hp-0-scorecard-2026-08-25.md:109` is stale the same way — that is the branch-protection item's to fix, not this one's, and one register item per commit. Refs RL-11, L-11 --- .claude/workflows/bughunt.js | 4 +- .superpowers/FINDINGS.md | 2 +- .superpowers/findings-ledger.json | 2 +- Client/CLAUDE.md | 7 ++- Client/package.json | 1 + ...server-admin-static-channel-perms.test.ts} | 8 +++ Client/tests/e2e/helpers.ts | 4 +- Client/tests/types/jsdom.d.ts | 6 ++- Client/tests/unit/capabilities-scope.test.ts | 3 ++ .../unit/tauri-conf-webview2-args.test.ts | 2 + Client/tests/unit/types.test.ts | 2 +- Server/CLAUDE.md | 5 ++ Server/updater/tauri_key_contract_test.go | 37 +++++++++++++ Server/updater/updater_test.go | 23 -------- docs/contributing.md | 52 +++++++++++++++++++ 15 files changed, 125 insertions(+), 33 deletions(-) rename Client/tests/{unit/admin-static-channel-perms.test.ts => contract/server-admin-static-channel-perms.test.ts} (91%) create mode 100644 Server/updater/tauri_key_contract_test.go diff --git a/.claude/workflows/bughunt.js b/.claude/workflows/bughunt.js index 1f84fb77..c90df999 100644 --- a/.claude/workflows/bughunt.js +++ b/.claude/workflows/bughunt.js @@ -120,7 +120,7 @@ Method: structural reference is evidence of coupling, not of a bug; open the cited file and confirm. 2. For every candidate, grep for ALL callers before judging - a guard may already live upstream. 3. Check whether an existing test already locks the behavior you think is wrong. If a test asserts it, - it is intended behavior, not a bug. Test files are *_test.go and tests/unit/*.test.ts. + it is intended behavior, not a bug. Test files are *_test.go, tests/unit/*.test.ts and tests/contract/*.test.ts. 4. Report EVERY finding you can prove - there is no cap. The quality bar stays: zero findings is a valid, respectable answer, and each finding needs file, line, and a concrete repro. @@ -200,7 +200,7 @@ const SURFACE_LENSES = [ `an entity when events arrive out of order; read-state that can mark unread messages read, or lose an unread ` + `count, across a reconnect; an async handler whose await lets stale state be written after a newer update ` + `(last-write-wins race); a route guard bypassable by a rapid navigation sequence.\n` + - `Check tests/unit/ before reporting - much of this behavior is already test-locked.`, + `Check tests/unit/ and tests/contract/ before reporting - much of this behavior is already test-locked.`, }, ]; diff --git a/.superpowers/FINDINGS.md b/.superpowers/FINDINGS.md index c72d917f..b0c99e22 100644 --- a/.superpowers/FINDINGS.md +++ b/.superpowers/FINDINGS.md @@ -4742,7 +4742,7 @@ saveChannelPerms writes the quick "Can access" toggles first (PUT allow=0/deny=0 **Suggested fix:** In saveChannelPerms, record the role IDs the quick-toggle loop actually wrote and skip the matrix step when the selected target is one of them — one guard in the one function: collect `const touched=new Set()` in the loop (add role.role_id on each PUT/DELETE), then wrap the matrix block in `if(path && !(permTargetPath().indexOf('/permissions/')>-1 && touched.has(tid)))`. Cleanest variant: give the quick checkbox an onchange that patches the in-memory role.allow/role.deny in state.permChannel and calls renderPermMatrix(), so the matrix always reflects the pending toggle instead of the stale snapshot. -**Fixed:** `69258a51` · test `Client/tests/unit/admin-static-channel-perms.test.ts` · revert-proof pass +**Fixed:** `69258a51` · test `Client/tests/contract/server-admin-static-channel-perms.test.ts` · revert-proof pass ### OC-0155 — medium — Room-key offer pacing budget is per-call, so two back-to-back rotations blow the server's per-second offer cap and strand peers on a dead key diff --git a/.superpowers/findings-ledger.json b/.superpowers/findings-ledger.json index 3d2fa671..a50fb0c0 100644 --- a/.superpowers/findings-ledger.json +++ b/.superpowers/findings-ledger.json @@ -3775,7 +3775,7 @@ "confidence": "high", "fix": { "commit": "69258a51", - "test": "Client/tests/unit/admin-static-channel-perms.test.ts", + "test": "Client/tests/contract/server-admin-static-channel-perms.test.ts", "revertProof": "pass" }, "suggestedFix": "In saveChannelPerms, record the role IDs the quick-toggle loop actually wrote and skip the matrix step when the selected target is one of them — one guard in the one function: collect `const touched=new Set()` in the loop (add role.role_id on each PUT/DELETE), then wrap the matrix block in `if(path && !(permTargetPath().indexOf('/permissions/')>-1 && touched.has(tid)))`. Cleanest variant: give the quick checkbox an onchange that patches the in-memory role.allow/role.deny in state.permChannel and calls renderPermMatrix(), so the matrix always reflects the pending toggle instead of the stale snapshot.", diff --git a/Client/CLAUDE.md b/Client/CLAUDE.md index 21b48b2e..83e151cf 100644 --- a/Client/CLAUDE.md +++ b/Client/CLAUDE.md @@ -9,8 +9,13 @@ Rust backend in `src-tauri/` for native APIs only. LiveKit handles voice/video. `src/pages/`, `src/components/` UI - `src/lib/protocolTypes.ts` and `src/generated/` are generated — see the root CLAUDE.md -- `tests/unit`, `tests/integration` (vitest, jsdom) · `tests/e2e` (Playwright) · +- `tests/unit`, `tests/integration`, `tests/contract` (vitest, jsdom) · + `tests/e2e`, `tests/e2e/admin`, `tests/e2e/native` (Playwright) · `tests/browser` (vitest browser mode) +- A test whose assertions read, import or execute a **`Server/`-owned** + artifact belongs in `tests/contract`, not `tests/unit` — `src-tauri/` is + part of this component, so reading it is an ordinary unit test. The rule + is in [docs/contributing.md](../docs/contributing.md#testing) ## Gotchas diff --git a/Client/package.json b/Client/package.json index 2aa7d88d..18ecad58 100644 --- a/Client/package.json +++ b/Client/package.json @@ -15,6 +15,7 @@ "test": "vitest run", "test:unit": "vitest run tests/unit", "test:integration": "vitest run tests/integration", + "test:contract": "vitest run tests/contract", "test:e2e": "playwright test", "test:e2e:prod": "npm run build && playwright test --config playwright.config.prod.ts", "test:e2e:native": "playwright test --config playwright.config.native.ts", diff --git a/Client/tests/unit/admin-static-channel-perms.test.ts b/Client/tests/contract/server-admin-static-channel-perms.test.ts similarity index 91% rename from Client/tests/unit/admin-static-channel-perms.test.ts rename to Client/tests/contract/server-admin-static-channel-perms.test.ts index 455804fa..d15fa0a8 100644 --- a/Client/tests/unit/admin-static-channel-perms.test.ts +++ b/Client/tests/contract/server-admin-static-channel-perms.test.ts @@ -1,3 +1,8 @@ +// CONTRACT TEST. The artifact under test is owned by Server/admin; the runner +// lives here because placement follows capability, not ownership — the Go +// module carries no JavaScript engine, so nothing under Server/ can execute +// this SPA. See docs/contributing.md#testing for the membership rule. +// // Loads the real Server/admin/static/index.html (the Go admin panel's // single-file SPA) into a scripted jsdom window and drives its inline // channel-permissions logic directly, the same way a browser would. @@ -5,6 +10,9 @@ // There is no bundler or module system for this file — it is one inline //