From 9ba6c56013bb795c3aab1a6b1a31baf82fb104b9 Mon Sep 17 00:00:00 2001 From: Jesus Armando Anaya <1445792+JArmandoAnaya@users.noreply.github.com> Date: Wed, 19 Aug 2026 01:22:18 -0700 Subject: [PATCH 1/3] chore(agents): local checks are targeted; CI is the exhaustive gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The policy still carried a mandatory comprehensive step: check.sh ran once before every pull request, every commit was to leave the full check table green, and two change categories escalated straight to the whole gate. Each of those reads as an instruction to run the entire repository locally, which is CI's job. The policy now says one thing in one place: choose the smallest set of checks that gives real feedback on the change, read what CI answered after pushing, and never claim a check passed without running it. High-risk changes escalate to the *relevant* suites — the real-server cycle scenario for state, gating and progress, the drift checks and stub-driven browser specs for a published wire shape — not automatically to everything. check.sh keeps every group and stays 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. --- .agents/skills/backend/python-setup/SKILL.md | 2 +- .agents/skills/frontend/nodejs-setup/SKILL.md | 4 +- .../skills/process/refactor-protocol/SKILL.md | 13 +++-- AGENTS.md | 44 +++++++++------- CONTRIBUTING.md | 51 ++++++++++--------- scripts/check.sh | 8 ++- 6 files changed, 69 insertions(+), 53 deletions(-) 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=() From 1961dad56aad856044f0e696b4a897975edc8dad Mon Sep 17 00:00:00 2001 From: Jesus Armando Anaya <1445792+JArmandoAnaya@users.noreply.github.com> Date: Wed, 19 Aug 2026 01:23:14 -0700 Subject: [PATCH 2/3] fix(agents): setup never touches an existing CLAUDE.md setup_agents.sh deleted a regular 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 has no business converting it by force. The symlink is now created only where nothing exists. The expected symlink is recognized and left alone; a regular file, a directory, or a symlink to any other target is left exactly as found, with a notice naming what was found and the manual command that adopts the canonical text. A regression test runs the real script in a throwaway repository layout and holds all five cases: creation, idempotence, the preserved regular file, the preserved foreign symlink, and the pruning of a deleted skill's link. --- scripts/setup_agents.sh | 31 ++++--- tests/scripts/setup_agents.test.mjs | 138 ++++++++++++++++++++++++++++ 2 files changed, 158 insertions(+), 11 deletions(-) create mode 100644 tests/scripts/setup_agents.test.mjs diff --git a/scripts/setup_agents.sh b/scripts/setup_agents.sh index bbca3dc6..6309ec6b 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,8 +10,9 @@ # 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: an existing CLAUDE.md, +# whatever it is, is left exactly as found. On Windows, run it from Git Bash or WSL. set -euo pipefail @@ -83,16 +84,24 @@ 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. +# 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, and a setup script has no business deleting 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 + target="$(readlink "$claude_md")" + if [ "$target" = "AGENTS.md" ]; then + echo " CLAUDE.md -> AGENTS.md already in place" + else + echo " NOTE: CLAUDE.md is a symlink to '$target', not AGENTS.md — left untouched." >&2 + echo " To adopt the canonical text: rm CLAUDE.md && ln -s AGENTS.md CLAUDE.md" >&2 + fi +elif [ -e "$claude_md" ]; then + echo " NOTE: CLAUDE.md already exists — left untouched." >&2 + echo " To adopt the canonical text: move yours aside, then ln -s AGENTS.md CLAUDE.md" >&2 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..26a665d4 --- /dev/null +++ b/tests/scripts/setup_agents.test.mjs @@ -0,0 +1,138 @@ +/** + * 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. 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 leave everything else + * exactly as found. + */ +import assert from "node:assert/strict"; +import { + copyFileSync, + 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 { + run(root); + 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 says so", () => { + 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.equal(result.status, 0, result.stderr); + assert.ok(lstatSync(claude).isFile(), "still a regular file, not a symlink"); + assert.equal(readFileSync(claude, "utf8"), theirs); + assert.match(result.stderr, /left untouched/); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +test("a symlink to some other target is preserved, and the notice names it", () => { + 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.equal(result.status, 0, result.stderr); + assert.equal(readlinkSync(path.join(root, "CLAUDE.md")), "NOTES.md"); + assert.match(result.stderr, /NOTES\.md/); + assert.match(result.stderr, /left untouched/); + } 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 }); + } +}); From 4a5317462990cd319e38b0ac392ff0fd0f25046b Mon Sep 17 00:00:00 2001 From: Jesus Armando Anaya <1445792+JArmandoAnaya@users.noreply.github.com> Date: Wed, 19 Aug 2026 01:33:48 -0700 Subject: [PATCH 3/3] fix(agents): fail on conflicting Claude config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Preserving an unexpected CLAUDE.md while exiting 0 was the quieter half of the failure the last commit removed: a caller reads "setup succeeded" while Claude is still reading a divergent file. A conflicting CLAUDE.md — a regular file, a directory, or a symlink to any other target — now aborts the run with an ERROR on stderr and a non-zero exit, and the check happens before any skill symlink is created or pruned, so a conflict leaves the tree exactly as it found it and never prints the success message. The missing-file and expected-symlink paths are unchanged: create, or recognize and succeed. The regression suite flips the file and foreign-symlink cases to expect failure, asserts the conflict performed no setup at all, and adds the directory case. --- scripts/setup_agents.sh | 47 +++++++++++++++++---------- tests/scripts/setup_agents.test.mjs | 49 +++++++++++++++++++++++------ 2 files changed, 69 insertions(+), 27 deletions(-) diff --git a/scripts/setup_agents.sh b/scripts/setup_agents.sh index 6309ec6b..c4afc728 100755 --- a/scripts/setup_agents.sh +++ b/scripts/setup_agents.sh @@ -11,14 +11,41 @@ # bash scripts/setup_agents.sh # # 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: an existing CLAUDE.md, -# whatever it is, is left exactly as found. On Windows, run it from Git Bash or WSL. +# 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" @@ -83,22 +110,8 @@ 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 — but only a missing -# CLAUDE.md gets one. Anything already there is a developer's own state, possibly -# carrying local configuration, and a setup script has no business deleting it. -claude_md="$REPO_ROOT/CLAUDE.md" if [ -L "$claude_md" ]; then - target="$(readlink "$claude_md")" - if [ "$target" = "AGENTS.md" ]; then - echo " CLAUDE.md -> AGENTS.md already in place" - else - echo " NOTE: CLAUDE.md is a symlink to '$target', not AGENTS.md — left untouched." >&2 - echo " To adopt the canonical text: rm CLAUDE.md && ln -s AGENTS.md CLAUDE.md" >&2 - fi -elif [ -e "$claude_md" ]; then - echo " NOTE: CLAUDE.md already exists — left untouched." >&2 - echo " To adopt the canonical text: move yours aside, then ln -s AGENTS.md CLAUDE.md" >&2 + echo " CLAUDE.md -> AGENTS.md already in place" else ln -s "AGENTS.md" "$claude_md" echo " linked: CLAUDE.md -> AGENTS.md" diff --git a/tests/scripts/setup_agents.test.mjs b/tests/scripts/setup_agents.test.mjs index 26a665d4..14dc7762 100644 --- a/tests/scripts/setup_agents.test.mjs +++ b/tests/scripts/setup_agents.test.mjs @@ -5,14 +5,18 @@ * 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. 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 leave everything else - * exactly as found. + * 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, @@ -77,7 +81,8 @@ test("a missing CLAUDE.md becomes the AGENTS.md symlink, beside the skill links" test("a second run over its own output is a quiet success", () => { const root = makeRepo(); try { - run(root); + 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"); @@ -87,32 +92,56 @@ test("a second run over its own output is a quiet success", () => { } }); -test("an existing regular CLAUDE.md survives byte-for-byte, and the run says so", () => { +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.equal(result.status, 0, result.stderr); + 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 symlink to some other target is preserved, and the notice names it", () => { +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.equal(result.status, 0, result.stderr); + 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, /left untouched/); + assert.match(result.stderr, /AGENTS\.md/); + assert.doesNotMatch(result.stdout, /Done\./, "no success message after a conflict"); } finally { rmSync(root, { recursive: true, force: true }); }