diff --git a/.agents/skills/backend/python-setup/SKILL.md b/.agents/skills/backend/python-setup/SKILL.md index fb93cc31..b2eb4d9f 100644 --- a/.agents/skills/backend/python-setup/SKILL.md +++ b/.agents/skills/backend/python-setup/SKILL.md @@ -58,7 +58,7 @@ hatches are in CONTRIBUTING.md. ## Checks -The check policy — pertinent tests while iterating, the gate once before the PR — is in +The check policy — targeted checks while developing, CI as the exhaustive gate — is in AGENTS.md `## Checks`, commands included. What is specific to Python work: - If you touched FastAPI routes or response models, re-export `openapi.json` diff --git a/.agents/skills/frontend/nodejs-setup/SKILL.md b/.agents/skills/frontend/nodejs-setup/SKILL.md index d8856033..dec7c6c0 100644 --- a/.agents/skills/frontend/nodejs-setup/SKILL.md +++ b/.agents/skills/frontend/nodejs-setup/SKILL.md @@ -90,8 +90,8 @@ Never hand-edit a `version` field. The repo-root `VERSION` file is the source of ## Before you say it works While iterating, run the package you touched — `pnpm --filter @visionset/ test`, and `lint` -on the same filter. The exhaustive run belongs to the gate — AGENTS.md `## Checks`. Two blind -spots worth naming: +on the same filter. The exhaustive run belongs to CI — AGENTS.md `## Checks`. Two blind spots +worth naming, because they say when to escalate to a browser suite: - **None of the workspace commands runs a browser.** Both Playwright suites sit outside them, so a change only chromium can observe passes everything above. diff --git a/.agents/skills/process/refactor-protocol/SKILL.md b/.agents/skills/process/refactor-protocol/SKILL.md index efdfc33d..36b1b6e7 100644 --- a/.agents/skills/process/refactor-protocol/SKILL.md +++ b/.agents/skills/process/refactor-protocol/SKILL.md @@ -40,10 +40,9 @@ state, before adopting, rebasing, or pruning anything. ## Testing requirements -The test-execution policy — pertinent tests while iterating, `bash scripts/check.sh` once before -the PR, CI as the exhaustive gate, reading what CI answered as part of pushing, and the two -change categories that always earn the full gate — lives in **AGENTS.md, `## Checks`**. On top of -it: +The test-execution policy — targeted checks while developing, CI as the exhaustive gate, +reading what CI answered as part of pushing, and how high-risk changes escalate to broader +*relevant* checks — lives in **AGENTS.md, `## Checks`**. On top of it: - **Layout, virtualization, and observer behavior are asserted in real chromium (Playwright), never jsdom** — a never-attached ResizeObserver passes green in jsdom forever. @@ -87,11 +86,11 @@ it: ## PR & CI **Merging is never part of the task.** Every pull request is merged by a human, after review, -with every required check green. The flow: implementation → pre-PR gate → completion report → a -pull request per the tier below → human review → manual merge. +with every required check green. The flow: implementation → targeted checks → completion report +→ a pull request per the tier below → CI read → human review → manual merge. **Tier A — no UI-affecting surface**: open the pull request at completion. **Tier B — -UI-affecting**: stop after the gate; report, open nothing, and wait for explicit instruction — +UI-affecting**: stop at completion; report, open nothing, and wait for explicit instruction — the branch stays on its worktree for visual evaluation first. A change is UI-affecting if it touches anything under `frontend/`, `src/visionset/_static/` or the bundling path, changes wire shapes or `allowed_actions` or server behavior the UI renders, or changes user-visible behavior diff --git a/AGENTS.md b/AGENTS.md index d50d07b7..bafa1667 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -14,7 +14,8 @@ creates **flat per-skill symlinks** so each skill is directly discoverable by ev ``` The flat layout is required because agents discover skills one level deep. The script also links -`CLAUDE.md → AGENTS.md`, so this file is the only instruction source to maintain. +`CLAUDE.md → AGENTS.md` when no `CLAUDE.md` exists, so this file is the only instruction source +to maintain; an existing `CLAUDE.md` — a developer's own file — is left untouched. **Setup:** `bash scripts/setup_agents.sh` once after cloning (Git Bash/WSL on Windows). Safe to re-run; re-run it after adding or removing a skill. @@ -91,34 +92,42 @@ While iterating, run only what the change touches — the file's own suite, the a named test. Running the whole corpus every few edits is what makes the loop slow, and it is not what catches defects. -Every command below is a **subset** of the gate. The third column says what each one does not see, -because a row that stays silent about its own blind spot reads as the check. +Every command below is a **subset** of what CI runs. The third column says what each one does +not see, because a row that stays silent about its own blind spot reads as the check. | Check | Command | Does not cover | | --- | --- | --- | | Python tests | `uv run pytest tests/` for the area you touched | Everything outside the paths you name | | Import contracts | `uv run lint-imports` | — | -| Kernel type-safety | `uv run mypy src/visionset/kernel` | The kernel only. The gate runs `mypy src/visionset` — well over twice as many files; server, CLI, MCP and formats are outside this command | +| Kernel type-safety | `uv run mypy src/visionset/kernel` | The kernel only. CI runs `mypy src/visionset` — well over twice as many files; server, CLI, MCP and formats are outside this command | | Lint / format | `uv run ruff check .` / `uv run ruff format .` | — | | Frontend | `pnpm -r build && pnpm -r test && pnpm -r lint` | **No browser at all.** Both Playwright suites sit outside it, so anything only chromium can see passes here | | OpenAPI contract | `uv run python scripts/export_openapi.py` (commit the diff) | — | -### The gate +### CI is the exhaustive gate -**`bash scripts/check.sh` is the gate, and CI runs the same checks on every pull request.** -Locally it runs **once** — immediately before opening a pull request, so a CI failure does not -burn a three-strike round-trip — or when you are explicitly asked for one. Not every few changes. +**Local checks are targeted; GitHub Actions runs the exhaustive matrix on every pull request.** +Choose the smallest set of checks that gives real feedback on the change, and do not run the +whole repository's suites merely because a commit, push, or pull request is about to happen — +that is CI's job. -**After a push, read what CI answered.** A narrow local run plus an unread CI result is not a -checked change, and a check you have not read is a check that failed. +**After a push, read what CI answered.** A targeted local run plus an unread CI result is not a +checked change, and a check you have not read is a check that failed. Investigate failures, fix +the ones the branch caused, and never report completion while a required check is red or unread. -Two kinds of change earn the full gate however small the diff: anything touching **state, gating or -progress**, where the real-server cycle run has repeatedly been the only detector, and any change to -**the shape of a published wire model**, where a new required field turns every hand-built browser -stub into a runtime failure that only chromium observes. Both are invisible to every command in the -table above. +**High-risk changes escalate to broader *relevant* checks, not to everything.** A change touching +**state, gating or progress** includes the real-server cycle scenario, which has repeatedly been +the only detector for that surface; a change to **the shape of a published wire model** includes +the generation/contract drift checks and the browser specs whose hand-built stubs the new shape +breaks. Pick the checks the risk actually points at. -Report failures verbatim. Never claim a check passed without running it. +`bash scripts/check.sh` remains the comprehensive local run for the moments a developer +deliberately chooses its cost — reproducing a CI failure, debugging an integration problem, +validating a high-risk change locally. It is never a routine step before a commit, push, or +pull request. + +Report failures verbatim. Never claim a check passed without running it. A completion report +names the targeted checks that ran and leaves the exhaustive verdict to the pull request's CI. ## Rules @@ -154,7 +163,8 @@ Report failures verbatim. Never claim a check passed without running it. the responsible developer signs, because authorship is accountability. The sole exception is a service bot acting autonomously by design (Dependabot, a CI bot), which signs as itself. - **NEVER** create commits on your own — only when explicitly asked. -- Every commit leaves the checks above green. +- A commit is accompanied by the targeted checks relevant to it; CI proves the rest on the + pull request, and must be green before a human merges. ### Files diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9ac4ca38..6f326007 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -135,29 +135,31 @@ design, and nothing here re-imposes one on them. ## Checks that must stay green -**`bash scripts/check.sh` is the gate** (or `pnpm check` — the same script). It is the -canonical invocation for humans and agents alike: it collects *every* failure rather than -stopping at the first, carries `set -euo pipefail`, and prints a per-step timing table, and -CI runs it on every pull request. - -**Locally it runs once, not continuously.** While iterating, run only the tests pertinent to -the change — the file's own suite, the module's suite, or a named test — and take a group -with `bash scripts/check.sh python`, `frontend`, `generated` or `browser`. The full pass -belongs immediately before you open a pull request, so that a CI failure does not cost a -round-trip, or to a moment somebody asks for one. Then read what CI answered: a check nobody -read is a check that failed, and a pull request has sat red on the annotator chromium suite -across several pushes for exactly that reason while every narrow signal was green. - -**It runs the browser suites, and that is the default.** Until #314 it ran no browser at -all while calling itself canonical — and during the 2026-08 remediation run the -real-server cycle suite was three separate times the *only* one to catch a regression -(#306, #308, #309), one of which shipped on a green run of this script and went red in CI. -It has since been four: the auto-labeling walk added for #609 found, on its first run, a -runtime gate in the download route that no unit test could reach, because the gate is in -the route rather than in anything a service test drives. -`bash scripts/check.sh --fast` skips them for the inner loop; it says so in a banner rather -than quietly, because "All checks passed" has always meant "all the checks this invocation -ran". +**Local checks are targeted; CI is the exhaustive gate.** While developing, run only the +checks pertinent to the change — the file's own suite, the module's suite, a named test, +the touched package's lint — and let GitHub Actions run the complete matrix on the pull +request. Then read what CI answered: a check nobody read is a check that failed, and a +pull request has sat red on the annotator chromium suite across several pushes for exactly +that reason while every narrow signal was green. + +**`bash scripts/check.sh`** (or `pnpm check` — the same script) is the comprehensive local +run for the moments you deliberately want the whole thing on your own machine: reproducing +a CI failure, debugging an integration problem, validating a high-risk change before +pushing it. It collects *every* failure rather than stopping at the first, carries +`set -euo pipefail`, and prints a per-step timing table. Take a group with +`bash scripts/check.sh python`, `frontend`, `generated` or `browser`; it is not a required +step before a commit, push, or pull request. + +**A high-risk change escalates to the relevant suites, not to everything.** The real-server +cycle suite has repeatedly been the only check to catch a state/gating/progress regression — +a stale capability declaration, a label flip standing in for feedback, a progress counter +running backwards, a runtime gate living in a route no service test drives — so a change on +that surface includes `bash scripts/check.sh browser` (or the specific spec) locally rather +than waiting for CI to say it. A change to a published wire model's shape includes the +generation/drift checks and the browser specs whose hand-built stubs it breaks. +`bash scripts/check.sh --fast` runs everything but the browser suites and says so in a +banner rather than quietly, because "All checks passed" has always meant "all the checks +this invocation ran". **The last line on stdout says what the run covered**, so "all the checks this invocation ran" is something a reader can check rather than infer: @@ -434,7 +436,8 @@ the vehicle the product is designed around, and a pre-release is invisible to a Use [Conventional Commits](https://www.conventionalcommits.org/) (`feat:`, `fix:`, `chore:`, `docs:`, `test:` … with optional scope, e.g. `feat(kernel): …`). Keep commits as logical -increments; every commit should leave the checks above green. +increments; run the checks pertinent to each commit, and let the pull request's CI prove +the rest. **Commits are authored by the contributor.** Coding agents used during development are not credited as authors or co-authors: no `Co-Authored-By` trailer naming one, no "generated with" diff --git a/scripts/check.sh b/scripts/check.sh index e1d63013..6d31b1cd 100755 --- a/scripts/check.sh +++ b/scripts/check.sh @@ -1,5 +1,9 @@ #!/usr/bin/env bash -# The canonical way to run VisionSet's checks. Humans and agents use this one. +# Comprehensive local validation, on demand. CI runs the exhaustive matrix on +# every pull request; this script exists for the moments a developer deliberately +# wants the whole thing locally — reproducing a CI failure, debugging an +# integration problem, validating a high-risk change. It is not a routine step +# before a commit, push, or pull request. # # It exists because `uv run pytest -q | tail -20` reports exit 0 while the suite # fails — a pipeline's status is the last command's — and that once hid a real @@ -51,7 +55,7 @@ cd "$root" # the pipefail above still applies inside whatever the step runs. declare -a failed=() # One ` ` line per step, printed as a table at the end. The cost -# of the full gate is now minutes rather than seconds, so it is worth seeing +# of the full run is minutes rather than seconds, so it is worth seeing # where they go — both to choose `--fast` knowingly and to notice the day a step # quietly doubles. declare -a timings=() diff --git a/scripts/setup_agents.sh b/scripts/setup_agents.sh index bbca3dc6..c4afc728 100755 --- a/scripts/setup_agents.sh +++ b/scripts/setup_agents.sh @@ -1,8 +1,8 @@ #!/usr/bin/env bash # Creates per-skill symlinks so each skill resolves at the flat depth coding agents # expect: .claude/skills/{name}/SKILL.md and .cursor/skills/{name}/SKILL.md — plus a -# CLAUDE.md -> AGENTS.md symlink so Claude Code reads the same instructions as every -# other tool, with nothing to keep in sync by hand. +# CLAUDE.md -> AGENTS.md symlink, created only when no CLAUDE.md exists, so Claude +# Code reads the same instructions as every other tool with nothing to keep in sync. # # The canonical, committed source is .agents/skills/{category}/{name}/ — the category # layer is for human organisation only; the generated symlink trees are git-ignored. @@ -10,14 +10,42 @@ # Run once after cloning, and again after adding or removing a skill: # bash scripts/setup_agents.sh # -# Safe to re-run — existing symlinks are replaced, dangling ones are pruned, and real -# files or directories are never touched. On Windows, run it from Git Bash or WSL. +# Safe to re-run — skill symlinks are replaced, dangling ones are pruned, and anything +# that is not a symlink this script created is never touched: a CLAUDE.md that is not +# the managed symlink is left exactly as found and the script exits non-zero before +# creating anything, so a conflict can never read as a completed setup. On Windows, +# run it from Git Bash or WSL. set -euo pipefail REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" AGENTS_DIR="$REPO_ROOT/.agents/skills" +# CLAUDE.md is Claude Code's entry point; AGENTS.md is the canonical text. A symlink +# means there is exactly one instruction file to maintain — but only a missing +# CLAUDE.md gets one. Anything already there is a developer's own state, possibly +# carrying local configuration, so a conflict aborts the whole run *before* any +# skill symlink is created or pruned: a caller must never read a preserved conflict +# as a completed setup. +claude_md="$REPO_ROOT/CLAUDE.md" +if [ -L "$claude_md" ]; then + target="$(readlink "$claude_md")" + if [ "$target" != "AGENTS.md" ]; then + echo "ERROR: CLAUDE.md is a symlink to '$target', not the managed AGENTS.md symlink." >&2 + echo " It was left untouched, and no setup was performed." >&2 + echo " To adopt the canonical text: rm CLAUDE.md && ln -s AGENTS.md CLAUDE.md," >&2 + echo " then rerun scripts/setup_agents.sh." >&2 + exit 1 + fi +elif [ -e "$claude_md" ]; then + kind="file" + [ -d "$claude_md" ] && kind="directory" + echo "ERROR: CLAUDE.md already exists as a $kind and is not the managed AGENTS.md symlink." >&2 + echo " It was left untouched, and no setup was performed." >&2 + echo " Move or remove it manually, then rerun scripts/setup_agents.sh." >&2 + exit 1 +fi + # Ensure destination is a plain directory (remove an older single-symlink layout if present). ensure_dir() { local dir="$1" @@ -82,17 +110,11 @@ done prune_dangling "$REPO_ROOT/.claude/skills" prune_dangling "$REPO_ROOT/.cursor/skills" -# CLAUDE.md is Claude Code's entry point; AGENTS.md is the canonical text. A symlink -# means there is exactly one instruction file to maintain. A real CLAUDE.md file is an -# old hand-copied twin that has already drifted at least once — replace it. -claude_md="$REPO_ROOT/CLAUDE.md" -if [ -L "$claude_md" ] || [ ! -e "$claude_md" ]; then - ln -sfn "AGENTS.md" "$claude_md" - echo " linked: CLAUDE.md -> AGENTS.md" +if [ -L "$claude_md" ]; then + echo " CLAUDE.md -> AGENTS.md already in place" else - rm "$claude_md" ln -s "AGENTS.md" "$claude_md" - echo " replaced hand-copied CLAUDE.md with symlink -> AGENTS.md" + echo " linked: CLAUDE.md -> AGENTS.md" fi echo "Done." diff --git a/tests/scripts/setup_agents.test.mjs b/tests/scripts/setup_agents.test.mjs new file mode 100644 index 00000000..14dc7762 --- /dev/null +++ b/tests/scripts/setup_agents.test.mjs @@ -0,0 +1,167 @@ +/** + * scripts/setup_agents.sh must never destroy developer state. + * + * The script once deleted a real CLAUDE.md and replaced it with the + * AGENTS.md symlink. A CLAUDE.md that is not the generated symlink is a + * developer's own file — possibly carrying local configuration — and a + * setup script that converts it by force turns "run this once after + * cloning" into data loss. Preserving it while exiting 0 is the quieter + * half of the same failure: a caller reads "setup succeeded" while Claude + * is still reading a divergent file. These tests run the real script in a + * throwaway repository layout and hold it to: create the symlink only + * where nothing exists, be idempotent over its own output, and on any + * conflict leave the object exactly as found, perform no setup at all, + * and exit non-zero. + */ +import assert from "node:assert/strict"; +import { + copyFileSync, + existsSync, + lstatSync, + mkdirSync, + mkdtempSync, + readFileSync, + readlinkSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { spawnSync } from "node:child_process"; +import { fileURLToPath } from "node:url"; +import { test } from "node:test"; + +const REPO = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..", ".."); +const SCRIPT = path.join(REPO, "scripts", "setup_agents.sh"); + +/** + * A minimal repository the script can run against: its own copy under + * scripts/, one committed skill, and an AGENTS.md for the link target. + * The script derives the repo root from BASH_SOURCE, so the copy anchors + * everything to the temp tree. + */ +function makeRepo() { + const root = mkdtempSync(path.join(os.tmpdir(), "visionset-setup-agents-")); + mkdirSync(path.join(root, "scripts")); + copyFileSync(SCRIPT, path.join(root, "scripts", "setup_agents.sh")); + mkdirSync(path.join(root, ".agents", "skills", "backend", "python-setup"), { recursive: true }); + writeFileSync( + path.join(root, ".agents", "skills", "backend", "python-setup", "SKILL.md"), + "# a skill\n", + ); + writeFileSync(path.join(root, "AGENTS.md"), "# canonical instructions\n"); + return root; +} + +function run(root) { + return spawnSync("bash", [path.join(root, "scripts", "setup_agents.sh")], { + encoding: "utf8", + env: { ...process.env, PATH: `${path.dirname(process.execPath)}:/usr/bin:/bin` }, + }); +} + +test("a missing CLAUDE.md becomes the AGENTS.md symlink, beside the skill links", () => { + const root = makeRepo(); + try { + const result = run(root); + assert.equal(result.status, 0, result.stderr); + const claude = path.join(root, "CLAUDE.md"); + assert.ok(lstatSync(claude).isSymbolicLink(), "CLAUDE.md is a symlink"); + assert.equal(readlinkSync(claude), "AGENTS.md"); + for (const tool of [".claude", ".cursor"]) { + const link = path.join(root, tool, "skills", "python-setup"); + assert.ok(lstatSync(link).isSymbolicLink(), `${tool}/skills/python-setup is a symlink`); + } + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +test("a second run over its own output is a quiet success", () => { + const root = makeRepo(); + try { + const first = run(root); + assert.equal(first.status, 0, first.stderr); + const again = run(root); + assert.equal(again.status, 0, again.stderr); + assert.equal(readlinkSync(path.join(root, "CLAUDE.md")), "AGENTS.md"); + assert.match(again.stdout, /already in place/); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +test("an existing regular CLAUDE.md survives byte-for-byte, and the run fails", () => { + const root = makeRepo(); + try { + const claude = path.join(root, "CLAUDE.md"); + const theirs = "# my local instructions\ndo not lose this\n"; + writeFileSync(claude, theirs); + const result = run(root); + assert.notEqual(result.status, 0, "a preserved conflict must not read as success"); + assert.ok(lstatSync(claude).isFile(), "still a regular file, not a symlink"); + assert.equal(readFileSync(claude, "utf8"), theirs); + assert.match(result.stderr, /ERROR/); + assert.match(result.stderr, /left untouched/); + assert.doesNotMatch(result.stdout, /Done\./, "no success message after a conflict"); + assert.ok(!existsSync(path.join(root, ".claude")), "conflict aborts before any setup"); + assert.ok(!existsSync(path.join(root, ".cursor")), "conflict aborts before any setup"); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +test("a directory named CLAUDE.md is preserved whole, and the run fails", () => { + const root = makeRepo(); + try { + const claude = path.join(root, "CLAUDE.md"); + mkdirSync(claude); + writeFileSync(path.join(claude, "inner.md"), "contents\n"); + const result = run(root); + assert.notEqual(result.status, 0, "a preserved conflict must not read as success"); + assert.ok(lstatSync(claude).isDirectory(), "still a directory"); + assert.equal(readFileSync(path.join(claude, "inner.md"), "utf8"), "contents\n"); + assert.match(result.stderr, /ERROR/); + assert.match(result.stderr, /directory/); + assert.doesNotMatch(result.stdout, /Done\./, "no success message after a conflict"); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +test("a symlink to some other target is preserved, and the error names both targets", () => { + const root = makeRepo(); + try { + writeFileSync(path.join(root, "NOTES.md"), "something else\n"); + symlinkSync("NOTES.md", path.join(root, "CLAUDE.md")); + const result = run(root); + assert.notEqual(result.status, 0, "a preserved conflict must not read as success"); + assert.equal(readlinkSync(path.join(root, "CLAUDE.md")), "NOTES.md"); + assert.match(result.stderr, /ERROR/); + assert.match(result.stderr, /NOTES\.md/); + assert.match(result.stderr, /AGENTS\.md/); + assert.doesNotMatch(result.stdout, /Done\./, "no success message after a conflict"); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +test("a symlink whose skill was deleted is pruned on the next run", () => { + const root = makeRepo(); + try { + run(root); + rmSync(path.join(root, ".agents", "skills", "backend", "python-setup"), { + recursive: true, + }); + const again = run(root); + assert.equal(again.status, 0, again.stderr); + assert.throws( + () => lstatSync(path.join(root, ".claude", "skills", "python-setup")), + /ENOENT/, + "the dangling link is gone", + ); + } finally { + rmSync(root, { recursive: true, force: true }); + } +});