diff --git a/.claude/guardrails.md b/.claude/guardrails.md new file mode 100644 index 00000000000..9b7bcbd4524 --- /dev/null +++ b/.claude/guardrails.md @@ -0,0 +1,115 @@ +# Guardrails — the long version + +`CLAUDE.md` states each working rule as one line: the rule and its shortest +reason. This file holds what does not fit there — the incident that produced +the rule, the recipe it implies, the numbers that make it credible. + +The split exists because `CLAUDE.md` is loaded into **every** session, so its +cost is paid on every turn, while a retro narrative is needed only when +someone actually hits the situation. Nothing here is a new rule. If this file +and `CLAUDE.md` ever disagree, `CLAUDE.md` is the rule and this is the +commentary that fell behind. + +Sections follow the order in which `CLAUDE.md` states the rules. + +--- + +## Delegated agents run on Opus by default + +Owner directive, 2026-08-11, refined 2026-08-16. + +Pass `model: opus` when spawning subagents or workflows — the tier the +`agentic/README.md` model table already uses for large tasks. Escalate to +Fable for genuinely hard reasoning, drop to Sonnet or Haiku for mechanical +grinding, but Opus is the default answer. + +Escalation is for judgment calls, not for every detail. Within its briefed +scope a delegate decides routine matters itself and documents them; otherwise +delegation gains nothing. What comes back to the main loop, which keeps the +overview, is anything that changes scope, contradicts the brief or the docs, +or would be expensive to redo. Say that split explicitly in the prompt: +decide-and-document versus return-as-finding. + +**Two lines every brief carries** (2026-09-05). A delegate reads `CLAUDE.md` +like anyone else, but it also receives harness reminders that arrive LATER in +its context and therefore read as the more recent instruction. In auto mode +one of them prescribes editing files through `sed`, heredocs and short +scripts. In the sibling repository three agents in one day followed it against +the Edit/Write rule, two of them for the single-token `sed -i` that fills the +PR number into a changelog fragment — the same fragment format this repository +uses. A fourth ran `git checkout -b` in the SHARED checkout, which moves a ref +the owner's own working tree is sitting on. So spell both out in the brief, in +the brief's own words: + +1. Repo files are modified ONLY with Edit/Write — `CLAUDE.md` wins over the + auto-mode reminder. +2. All `git` happens in the agent's own worktree; the shared checkout is left + on the branch it was found on. + +Neither is new policy. Both are precedence a brief has to make explicit, +because an agent cannot infer precedence from the order messages arrived in. + +## External-system writes need explicit, named authorization + +The rule is the asking. This section is about what to hand over once the +answer is yes and the agent still cannot run the command itself. + +Some prod runbooks are refused by the harness permission classifier rather +than by the owner — the `gcloud run services update` plus `update-traffic` +pair that arms the site's origin gate is one, observed 2026-09-04. Reaching +for a different phrasing of the same command is the wrong move: the classifier +is not an obstacle to route around. Stopping half-way is not the danger +either, when the runbook is built for it — the pair in +`infra/cloudflare/README.md` § "Arming, in full" stages the new revision and +promotes it by name, so a config that fails to render leaves the revision +never ready and traffic on the old one, which the README calls a safe failure +and the reason its step 4 is not optional. The real danger is reading the +STAGED revision as the finished one and reporting an arm that never took +traffic. + +The pattern that works: write the runbook as ONE script in the scratchpad — +never into the repo — and hand the owner a single line to paste: + +``` +! bash /tmp/…/scratchpad/arm-origin-gate.sh +``` + +The script does the whole pair under `set -euo pipefail`, echoes what it is +about to change, and ends by reading the state back — for that gate, the +`x-origin-gate` response header AND `status.traffic`, because only the second +says which revision answered. Rules for it: one action per script, and no +secret values in its output. + +**The reverse direction is its own script, not the same one with a flag.** +`infra/cloudflare/README.md` § "Rolling back" is explicit about why: a +rollback has to run in the worst state the service can be in, which includes +the secret having been disabled during the incident — so it never looks the +SECRET up. It still resolves the serving revision and its image, because it +has to; what it must not do is depend on anything the incident may have taken +away. + +Afterwards, VERIFY from the session with a read the agent is allowed to make, +instead of trusting a "done" in chat. A runbook that shipped this way belongs +in the owning README, so the next round starts from a reviewed text rather +than a fresh improvisation. + +## Modify repo files only with the Edit/Write tools + +Appending with `>>` counts — appending at the end of a file is exactly the +forbidden path, however little it feels like editing. + +When a Bash command legitimately mutates a tracked file (a formatter, codegen, +`git checkout`), read the file again before the next edit; stale-state errors +cascade otherwise. + +The moment this rule gets broken is when an edit ANCHOR fails — "string not +found", "file modified since read". The answer is a fresh targeted read plus a +longer anchor, never a regex rewrite from a heredoc. The other moment is a +change small enough to feel exempt: a one-token substitution reads like a +`sed` job, and `sed -i` on a changelog fragment is how three delegated agents +in one day broke the rule in the sibling repository (2026-09-05). Size is not +the criterion; the tool is. + +The exemptions are narrow and stated in `CLAUDE.md`: GitHub Actions workflows +and codegen scripts write files by design. An interactive session is never one +of those. diff --git a/.claude/skills/open-pr/SKILL.md b/.claude/skills/open-pr/SKILL.md index 44bb82e0007..3edd2378e7f 100644 --- a/.claude/skills/open-pr/SKILL.md +++ b/.claude/skills/open-pr/SKILL.md @@ -116,6 +116,36 @@ format (Summary / Plan / Test plan), the changelog gate, the push, and the PR-ref follow-up. English throughout, no "Generated with..." lines in the body. +**A multi-paragraph commit message goes through a file — and that +file belongs to this branch alone.** `git commit -F` keeps the prose +out of shell quoting, but the scratchpad is shared by every agent of +one session, so a generic `commitmsg.txt` gets overwritten by a +parallel agent and a later re-read commits someone else's text +(sibling repo, 2026-09-05: both a message and a body file were +clobbered mid-run). Take a private directory, which needs no +sanitising at all: + +```bash +D=$(mktemp -d) # or, if you name it: BRANCH=$(git branch --show-current) +MSG="$D/commitmsg.txt" # SLUG=${BRANCH//\//-}; MSG="$D/commitmsg-$SLUG.txt" +``` + +A branch name is not a filename — `release/v1.2.3` turns the slash +into a directory that does not exist — so substitute the separators +if you derive the name, and never write into a `$SCRATCH` you have +not set yourself. + +Write the file with the Write tool, then `git commit -F "$MSG"` in +the SAME step that wrote it — never re-read one a turn later to reuse +it, because between the two it may belong to another agent. These are +scratch input to one command, not a record; the record is the commit. + +The same holds for a PR body you pass as `--body-file`. The mandated +`/pull_request` command does not take that path — it builds the body +inline with a quoted heredoc (`agentic/commands/pull_request.md` +step 6), which has no collision to avoid — so this applies when you +write a body file yourself. + ## 3 · After opening: pipeline + review loop (do not skip) Repeat until **both** hold: all checks pass AND zero unresolved @@ -135,15 +165,32 @@ On failure: `gh run view --log-failed`, fix, push to the same branch, keep watching. **b. Wait for the Copilot review.** Bot login: -`copilot-pull-request-reviewer[bot]`. It usually lands within ~2 min; -if `gh pr view --json reviewRequests,reviews` shows neither a -request nor a review after the checks pass, request it explicitly -(verified working): +`copilot-pull-request-reviewer[bot]`. It arrives a few minutes after +the PR is OPENED — not after each push; see "One review per PR" +below. If `gh pr view --json reviewRequests,reviews` shows +neither a request nor a review after the checks pass, request it +explicitly (verified working): ```bash gh api -X POST repos/{owner}/{repo}/pulls//requested_reviewers -f "reviewers[]=copilot-pull-request-reviewer[bot]" ``` +**One review per PR is the normal case now.** The ruleset "Automated +Copilot Code Review" (anyplot 10370785, kurrentschrift 18516317) +carries `review_on_push: false` since 2026-09-03 — the owner asked +for the churn to stop, and the setting, not any skill, was what +re-reviewed. Two consequences for this loop. A FIX push starts no new +Copilot run, so a `copilot-*` check on the new head SHA is +legitimately ABSENT; waiting for one that will never come is the +failure mode to avoid — see §3e for what to require instead. And a +fresh review is requested only after a SUBSTANTIVE rework (new +behaviour, a reworked mechanism), never after every push: each +request re-reads the whole diff and surfaces "previously missed" +findings in files the push never touched, which draws another push +(kurrentschrift#406 collected ~15 requests in a day over a one-line +docstring fix). Stop once a round yields no new inline comments but +only carried-over items. + Fetch all three comment surfaces — they carry different content: ```bash @@ -180,6 +227,57 @@ gh api graphql -f query='mutation($id:ID!){resolveReviewThread(input:{threadId:$ report the PR URL and final state. **Do not merge unless explicitly authorized.** +**Merging on request: wait for the review, not just for green.** +When the owner does ask for the merge in this session, four +conditions, all read on the CURRENT head SHA — re-read it after every +push, `gh pr view --json headRefOid`: + +1. A draft is not reviewable — `gh pr ready ` first. Copilot + does not review a draft, so a draft merged "green" was never + reviewed, and `gh pr merge` on a draft fails anyway. Check + `isDraft`. +2. Every non-Copilot check on the head SHA is `completed` and green. + Dedupe the check runs **by name, newest wins**: a superseded run + (a label re-trigger, a cancelled first attempt) stays beside the + current one and reads as a red check that is not there any more. + Dedupe on `.id`, which grows with creation and is always set — a + check run carries no `created_at`, and `started_at` stays null + until the run begins, so a `max_by(.started_at)` would hand the + row to the OLD completed attempt while the new one is still + queued, which is the failure this step exists to prevent. + ```bash + gh api repos/{owner}/{repo}/commits/$(gh pr view --json headRefOid --jq .headRefOid)/check-runs \ + --jq '[.check_runs[]] | group_by(.name) | map(max_by(.id)) | .[] | "\(.name): \(.status) \(.conclusion // "")"' + ``` +3. **A Copilot review actually exists on the PR** — `gh pr view + --json reviews`, author `copilot-pull-request-reviewer`. The + head-SHA check run does not prove one: a run reaches `completed` + with conclusion `cancelled` and delivers nothing. So read the + check run only to learn whether a round is still RUNNING + (`queued`/`in_progress` means wait) and read the review list to + learn whether one was ever delivered. Since `review_on_push` is + off (§3b), the normal state after a fix push is no run on the head + at all with the first round's review standing — that is reviewed, + not unreviewed. If no review exists and the run was cancelled, one + re-request is the whole budget; after that report + green-and-unreviewed and let the owner decide, never loop. +4. Zero unresolved review threads (step c), outdated ones included. + +**Merge state is two different fields; read each by its own name.** +`mergeable` (`gh pr view --json mergeable`) is `MERGEABLE`, +`CONFLICTING` or `UNKNOWN` — `UNKNOWN` right after another merge is +GitHub still computing, so keep polling. `mergeStateStatus` is the +richer enum, where the conflicting case is `DIRTY`. A conflict is not +transient and has a symptom worth knowing: GitHub starts no CI at +all, so the PR shows no red check, just none (#11212 and +kurrentschrift#524, 2026-09-04, both read as "checks pending" for a +while). Report it and merge `origin/main` into the branch instead of +waiting it out. + +Poll all of this from ONE script rather than by hand, and kill a +stale wait loop with the bracket trick (`pkill -f "x[.]y"`), or +`pkill` matches its own calling shell. + ## 4 · After merge (when it happens): watch the deploy Merges to `main` touching `api/**`, `core/**`, or `pyproject.toml` @@ -206,9 +304,12 @@ a 20-minute poll on the global list never saw the builds). Match the - **`isOutdated` ≠ `isResolved`.** A fix-push can outdate a Copilot thread while it stays unresolved; outdated threads still count against review-clean — resolve them explicitly. -- **Copilot reviews every push round.** New threads on changed lines - are the loop working, not noise — but don't chase cosmetic nits - past a couple of rounds; surface stalemates to the user. +- **A fix push no longer starts a review round.** `review_on_push` is + `false` since 2026-09-03 (§3b), so only an explicit — and + substantive — re-request opens another one. When a round does run, + new threads on the changed lines are the loop working, not noise — + but don't chase cosmetic nits past a couple of rounds; surface + stalemates to the user. - **Stacked PRs die when their base squash-merges.** Don't stack; if work depends on an unmerged PR, wait for its merge (or do the work and rebase before opening). diff --git a/.claude/skills/verify-frontend/SKILL.md b/.claude/skills/verify-frontend/SKILL.md index 4925d948fd2..f31e4dbce1d 100644 --- a/.claude/skills/verify-frontend/SKILL.md +++ b/.claude/skills/verify-frontend/SKILL.md @@ -156,6 +156,27 @@ than restating values here. Method: Style questions are **findings to report**, not things to silently fix — palette and typography decisions are settled in the style guide. +## 3b · Numeric rules: measure the result, not the plan + +**A numeric UI rule is verified against the MEASURED result in the +browser, never against the planned one.** A floor, a minimum size, a +cap, a hit target, the ~13 px legibility floor above — the +verification names the rule and the number it measured, on every +surface the rule reaches, at both viewports. + +The case for it is a sibling-repo PR that shipped a 14 px x-height +floor for rendered lines whose planner sized them from the average +advance per character. The plan met the floor and the widest real +line did not, because the frame's own padding scales with the content +and was never in the budget: the page came out at **13.9 px** — a +rule broken by the code that enforces it, and only the measurement on +the page could say so. + +So read the number off the element (`getBoundingClientRect()`, +`getComputedStyle`), not off the code that computed it, and quote the +measurement in the PR. When the fix is to re-plan from the measured +value, say plainly which cases still fall outside the rule. + ## 4 · Performance Only when the change can plausibly move performance (data loading, diff --git a/.claude/skills/write-docs/SKILL.md b/.claude/skills/write-docs/SKILL.md index 763ce73baf8..b166a48665c 100644 --- a/.claude/skills/write-docs/SKILL.md +++ b/.claude/skills/write-docs/SKILL.md @@ -88,6 +88,35 @@ the house guide, repository docs follow Google style. Existing docs migrate **on touch** (the "fix formatting" rule below), no bulk rewrites required. +## Shortening a text that carries claims + +Applies wherever the text lives — a `docs/` reference, `README.md`, +the site's legal and about copy (`app/src/pages/LegalPage.tsx`, +`app/src/pages/AboutPage.tsx`) — whenever the edit makes an existing +text SHORTER: a legal section, a licensing paragraph, a factsheet, +any prose a reader may rely on. + +**Diff claim by claim against the previous version, not paragraph by +paragraph.** Shortening drops qualifiers before it drops sentences, +and a qualifier is what makes a claim true: "only", "unless", "up +to", a condition on a right, the last item of a list. In the sibling +repository on 2026-09-03 a privacy section lost exactly three that +way — an overstated retention period for the rate-limit counters, the +condition on the right to object, and part of a list of rights — +inside an edit that was otherwise a genuine improvement. The check +that catches it is mechanical: list the claims of the old text, then +tick each one off against the new text as kept, deliberately dropped, +or narrowed. + +Two rules on top: + +- **The owner's own sentences stay verbatim.** Where he supplied the + wording, it is quoted, not paraphrased and not tightened. Say in + the PR body that it is his sentence. +- **A claim you cannot support is removed, never softened** into a + vaguer version of itself: an unproven statement made fuzzy is still + an unproven statement. + ## Formatting - Actually FIX formatting issues while editing (headings, lists, code diff --git a/CLAUDE.md b/CLAUDE.md index a2fd5d4bf02..d500d88d10c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -13,7 +13,7 @@ A companion guide `.github/copilot-instructions.md` carries the shared rules for - **On a feature branch**: Claude MAY run `git commit`, `git push`, and `gh pr create` when the work warrants it. Still respect the project's automated pipelines (see "CRITICAL: Mandatory Workflow" below) — e.g. don't manually merge spec/impl PRs. - Confirm before destructive or hard-to-reverse operations (force-push, reset --hard, branch deletion) regardless of branch. - **GitHub Actions workflows ARE allowed to commit/push** - When running as part of `spec-*.yml` or `impl-*.yml` workflows, creating branches, commits, and PRs is expected and required. -- **Delegated agents run on Opus by default** - Pass `model: opus` when spawning subagents/workflows — the same tier the `agentic/README.md` model table (small→haiku, medium→sonnet, large→opus) and the audit/update/agentic commands already use. Escalate to Fable only for genuinely hard tasks that need deep reasoning; drop to Sonnet/Haiku for simple mechanical grinding. Escalation is for genuine judgment calls, not every detail: within its brief a delegate decides routine matters itself and documents them; what comes back to the main loop is anything that changes scope, contradicts the brief or the docs, or would be expensive to redo. State this split explicitly in delegate prompts (decide-and-document vs. return-as-finding). +- **Delegated agents run on Opus by default** - Pass `model: opus` when spawning subagents/workflows — the same tier the `agentic/README.md` model table (small→haiku, medium→sonnet, large→opus) and the audit/update/agentic commands already use. Escalate to Fable only for genuinely hard tasks that need deep reasoning; drop to Sonnet/Haiku for simple mechanical grinding. Escalation is for genuine judgment calls, not every detail: within its brief a delegate decides routine matters itself and documents them; what comes back to the main loop is anything that changes scope, contradicts the brief or the docs, or would be expensive to redo. State this split explicitly in delegate prompts (decide-and-document vs. return-as-finding). Every brief also spells out two things a delegate cannot infer from message order: repo files are modified only with Edit/Write, and all `git` stays inside the agent's own worktree — the shared checkout is left on the branch it was found on. - **Always write in English** - All output text (code comments, commit messages, PR descriptions, issue comments, documentation) must be in English, even if the user writes in another language. - **Repository prose follows the Google developer documentation style guide** - `docs/`, `README.md`, `agentic/docs/`, changelog entries, and PR/issue text use [Google style](https://developers.google.com/style) (sentence-case headings, second person, numbered procedures); the concrete rules and the house-style exception (`docs/reference/style-guide.md` governs website/brand surfaces) live in the `write-docs` skill. Existing docs migrate on touch, not via bulk rewrites. - **Update documentation when making changes** - When adding new features, events, or modifying behavior, always check if related documentation needs updating (e.g., `docs/reference/plausible.md` for analytics events, `docs/workflows/` for workflow changes, `docs/contributing.md` for user-facing changes). @@ -25,6 +25,10 @@ A companion guide `.github/copilot-instructions.md` carries the shared rules for - **Manual user tasks go to Todoist** - Whenever a session identifies a step only the user can or should do (adding `approved` labels, merge authorization, console/billing/DNS actions, secret rotations), create a task in the user's Todoist project **Anyplot** (via the Todoist MCP tools) naming the concrete action and a context link (PR/issue/run URL) — instead of leaving it buried in a chat reply. Interactive sessions only; GitHub Actions workflows have no MCP access. - **Fix small build/test blockers directly, even when out of scope** - If a typecheck error, failing test, lint failure, or other small pipeline blocker shows up while working on something unrelated (incl. things the current PR was not meant to touch), fix it in the same PR or a tiny follow-up — never leave it parked under "out of scope". A latent `tsc` error that doesn't surface locally will silently break the next Cloud Build, which deploys nothing new and leaves production stale even though every PR check looks green (this is exactly how PR #6961's frontend fixes never reached anyplot.ai — the unrelated `prism/r` TS7016 from #6944 was deferred, then blocked the next `yarn build`). The bar: if the fix is < ~20 lines and obviously correct, just do it; if it would expand scope meaningfully, ask first rather than deferring silently. +## Guardrails (the long version) + +The rules in this file are stated as one line each: the rule and its shortest reason. The incident behind a rule, the recipe it implies, and the numbers that make it credible live in [`.claude/guardrails.md`](.claude/guardrails.md) — read that file when you actually hit the situation, not on every turn. This file is loaded into every session and pays its cost on every turn; a retro narrative is needed once. Nothing in the companion file is a new rule: if the two ever disagree, CLAUDE.md is the rule and the companion is commentary that fell behind. + ## Changelog + releases - **Every PR adds `changelog.d/.md`, and NEVER a new bullet in `CHANGELOG.md`** — the fragment is a slice of the changelog in the changelog's own format (`### Category` over bold-titled English bullets with PR refs; the rules and an example are in `changelog.d/README.md`). That shared `[Unreleased]` spot is where sibling PRs used to conflict each other — three times in one night on 2026-09-02/03 — and the CI job "Changelog (fragment)" refuses both a missing fragment and a bullet ADDED to `[Unreleased]` directly — added, not merely different: a bullet is identified by its bold title, so correcting the wording of one already there passes. It also refuses a bare `(#NNNNN)` placeholder: leave the reference out and let `/pull_request` append the real number. Same check locally: `uv run python -m tools.changelog check --base origin/main`; `… preview` prints the pending section. **Exempt:** catalogue-only PRs (everything under `plots/`), the automated plot pipeline (`github-actions[bot]`: spec-create, impl-generate/review/repair/merge, spec auto-polish, daily-regen) and Dependabot — those are summarized in aggregate at release time (see `agentic/commands/release.md`) — plus any PR labelled `skip-changelog`. This rule is duplicated in `.github/copilot-instructions.md` and `agentic/commands/pull_request.md`; keep all three in sync when changing it. @@ -77,7 +81,7 @@ Known gaps with NO verification loop yet (reason through carefully and say so in - **Keep plans simple** - Do not over-scope by adding extra modes, elaborate multi-step processes, or spawning teams when a direct approach is requested. Ask for clarification before expanding scope. Only do exactly what was asked. - **Structural fix over symptomatic fix** - When a cheap symptomatic fix and a correct structural fix compete, take the structural one: fix the cause, never mute the alarm. Never modify working code to make a broken test pass — fix the test or flag it (this applies to the impl-review/repair loop too). - **Proper lint fixes only** - Always apply proper fixes for lint/code quality issues. Never use disable comments (`eslint-disable`, `noqa`, etc.) unless explicitly approved by the user. -- **Modify repo files only with the Edit/Write tools, never via Bash heredocs/sed** (interactive sessions; workflows and codegen scripts are exempt). When a Bash command legitimately mutates a tracked file (formatter, codegen, `git checkout`), Read the file again before the next Edit on it. When an Edit anchor fails ("string not found", "file modified since read"), the answer is a fresh targeted Read plus a longer anchor — never a heredoc or regex rewrite. +- **Modify repo files only with the Edit/Write tools, never via Bash heredocs/sed** (interactive sessions; workflows and codegen scripts are exempt). When a Bash command legitimately mutates a tracked file (formatter, codegen, `git checkout`), Read the file again before the next Edit on it. When an Edit anchor fails ("string not found", "file modified since read"), the answer is a fresh targeted Read plus a longer anchor — never a heredoc or regex rewrite. This rule OUTRANKS any harness or agent-mode reminder that offers shell editing as the faster path, and a delegate's brief says so (2026-09-05). - **Fix formatting when editing docs** - When formatting or improving markdown files, actually fix formatting issues (headings, lists, code blocks, structure) — don't just analyze the content. ## Package Management diff --git a/changelog.d/sibling-retro-guardrails.md b/changelog.d/sibling-retro-guardrails.md new file mode 100644 index 00000000000..1d2328d2350 --- /dev/null +++ b/changelog.d/sibling-retro-guardrails.md @@ -0,0 +1,57 @@ +### Added + +- **`.claude/guardrails.md`, the long version of the working rules.** `CLAUDE.md` + is loaded into every session and pays its cost on every turn, so it states each + rule as one line; the incident that produced a rule, the recipe it implies and + the numbers that make it credible now live in a companion file that is read + only when someone hits the situation. Three sections to start with: the two + lines every delegate brief has to spell out, the pattern for a prod runbook the + harness classifier refuses (one script in the scratchpad, one `! bash` line for + the owner, rollback as its own script, a read-back from the session afterwards), + and why a one-token substitution is not an exemption from the Edit/Write rule. + `tests/unit/test_agent_instructions.py` pins the split: every section maps to a + binding one-liner in `CLAUDE.md`, and the companion file has to say that + `CLAUDE.md` outranks it. (#11605) + +### Changed + +- **The Edit/Write rule now says what it outranks.** Auto mode hands a delegated + agent a reminder that prescribes editing files with `sed`, heredocs and short + scripts, and it arrives later in the context than `CLAUDE.md`, so it reads as + the more recent instruction — three agents in the sibling repository followed + it in one day, two of them for the single-token substitution that fills a PR + number into a changelog fragment. `CLAUDE.md` now states the precedence + explicitly, and the delegation rule adds the second line a brief has to carry: + all `git` stays inside the agent's own worktree, because a `git checkout -b` in + the shared checkout moves a ref the owner's working tree is sitting on. (#11605) +- **`/open-pr` knows what "ready to merge" means since the review ruleset + changed.** `review_on_push` is `false` in the "Automated Copilot Code Review" + ruleset since 2026-09-03, so a fix push starts no Copilot run and the absence + of a `copilot-*` check on the new head SHA is normal, not a reason to keep + waiting — the skill's gotcha still claimed the opposite. The merge-on-request + conditions are now written down: non-Copilot checks completed and green + (deduped by name, newest wins, so a superseded run is not read as red), a + Copilot review that actually exists on the PR — the head check run only says + whether a round is still running, since a run reaches `completed` with + conclusion `cancelled` and delivers nothing — and zero unresolved threads. + `mergeable=UNKNOWN` is GitHub still computing; a conflict (`CONFLICTING`, + `mergeStateStatus` `DIRTY`) gets no CI at all and is reported, not waited out. + Merging stays the owner's call. (#11605) +- **Commit-message and PR-body files are named after the branch.** The scratchpad + is shared by every agent of one session, so a generic `commitmsg.txt` or + `prbody.md` gets overwritten by a parallel agent and a later re-read commits + someone else's text. `/open-pr` now derives the name from the branch and says + the part that makes it safe: write and consume the file in the same step — it + is scratch input to one command, not a record. (#11605) +- **A numeric UI rule is verified against the measured result.** + `/verify-frontend` gains § 3b: a floor, a minimum size or a cap is read off the + element in the browser, never off the code that computed it. The case is a + 14 px x-height floor whose planner sized lines from an average advance — the + plan met the floor and the rendered page came out at 13.9 px, because the + frame's own padding was never in the budget. (#11605) +- **Shortening a text that carries claims is now a checklist.** `/write-docs` + gains the claim-by-claim diff duty: shortening drops qualifiers before it drops + sentences, which is how a privacy section lost an overstated retention period, + a condition on the right to object and part of a list of rights inside an + otherwise good edit. The owner's own sentences stay verbatim, and a claim that + cannot be supported is removed rather than softened. (#11605) diff --git a/tests/unit/test_agent_instructions.py b/tests/unit/test_agent_instructions.py index ff268167860..c6fd2608f76 100644 --- a/tests/unit/test_agent_instructions.py +++ b/tests/unit/test_agent_instructions.py @@ -8,14 +8,16 @@ the guide keeps reading as authoritative while it sends the next agent somewhere that no longer exists. -Four cheap pins, none of which needs the database, the network or a checkout of +Five cheap pins, none of which needs the database, the network or a checkout of anything but this repository: 1. every backtick-quoted repo path in the agent-facing files resolves; 2. every relative Markdown link resolves, and every same-page anchor points at a heading that is there; 3. every skill the routing table names exists as `.claude/skills//SKILL.md`; -4. the rules that are supposed to be mirrored are present on BOTH sides. +4. the rules that are supposed to be mirrored are present on BOTH sides; +5. `.claude/guardrails.md` stays a companion — every section it carries maps to + a binding one-liner in CLAUDE.md, and the file says so about itself. (4) is deliberately a keyword pin, not a text diff: the two files address different audiences and paraphrase each other, so requiring byte equality would @@ -38,10 +40,12 @@ CLAUDE_MD = REPO_ROOT / "CLAUDE.md" COPILOT_MD = REPO_ROOT / ".github" / "copilot-instructions.md" +GUARDRAILS_MD = REPO_ROOT / ".claude" / "guardrails.md" AGENT_FILES = [ CLAUDE_MD, COPILOT_MD, + GUARDRAILS_MD, REPO_ROOT / "agentic" / "docs" / "project-guide.md", REPO_ROOT / "agentic" / "commands" / "prime.md", ] @@ -329,3 +333,72 @@ def test_each_guide_names_the_other_as_its_companion() -> None: assert "companion guide `claude.md`" in copilot for name, text in (("CLAUDE.md", claude), ("copilot-instructions.md", copilot)): assert "both files must stay in sync" in text, f"{name} dropped the sync claim" + + +def test_guardrails_split_stays_subordinate() -> None: + """The rationale file must stay a companion, never become the rule. + + Two ways this split rots: CLAUDE.md loses the pointer, so nobody finds the + rationale; or `.claude/guardrails.md` starts reading like the authority, so + a rule ends up living only there — where no session loads it. + """ + claude = _flat(CLAUDE_MD.read_text(encoding="utf-8")) + guardrails = _flat(GUARDRAILS_MD.read_text(encoding="utf-8")) + + assert ".claude/guardrails.md" in claude, "CLAUDE.md no longer points at the rationale file" + assert "is the rule and this is the commentary" in guardrails, ( + "`.claude/guardrails.md` must state that CLAUDE.md wins — without it the " + "companion file starts reading like the authority" + ) + + +# Every `##` section of `.claude/guardrails.md` → a phrase that must appear in +# CLAUDE.md, where the binding one-liner lives. The map is explicit on purpose: +# adding a section to the companion file fails the test until its rule is +# registered here, and registering it forces you to name the CLAUDE.md line it +# belongs to. A disclaimer sentence alone would not have caught that. +# +# The phrases are searched in the whole of CLAUDE.md rather than in one +# section, because this repository states its rules across "Important Rules" +# and "Development Workflow"; each anchor therefore carries the rule's +# OBLIGATION and not just its subject, so it cannot match some other mention. +# +# A section that carries several obligations registers all of them, not just +# the one its title is about (Copilot review): the delegation section adds the +# worktree line and the Edit/Write precedence to the model-tier rule, and a +# single "opus by default" anchor would stay green while either of those was +# deleted from CLAUDE.md and left living only in the companion file — exactly +# the drift these tests exist to catch. +GUARDRAIL_SECTION_ANCHORS = { + "Delegated agents run on Opus by default": [ + "opus by default", + "all `git` stays inside the agent's own worktree", + "repo files are modified only with edit/write", + ], + "External-system writes need explicit, named authorization": ["explicit, named authorization"], + "Modify repo files only with the Edit/Write tools": ["heredocs/sed", "outranks any harness or agent-mode reminder"], +} + + +def _guardrail_sections() -> list[str]: + return re.findall(r"^## (.+)$", GUARDRAILS_MD.read_text(encoding="utf-8"), re.M) + + +def test_every_companion_section_is_registered() -> None: + """A new section in the companion file must be registered, both ways. + + Unregistered section → a rule could live only where no session loads it. + Registered but absent section → the map grants cover to nothing and rots. + """ + sections = set(_guardrail_sections()) + registered = set(GUARDRAIL_SECTION_ANCHORS) + assert not sections - registered, f"unregistered sections in guardrails.md: {sorted(sections - registered)}" + assert not registered - sections, f"registered but missing from guardrails.md: {sorted(registered - sections)}" + + +@pytest.mark.parametrize("section", sorted(GUARDRAIL_SECTION_ANCHORS), ids=lambda s: s[:40]) +def test_companion_section_has_a_binding_rule(section: str) -> None: + """Every obligation a rationale section elaborates is stated in CLAUDE.md.""" + claude = _flat(CLAUDE_MD.read_text(encoding="utf-8")) + missing = [anchor for anchor in GUARDRAIL_SECTION_ANCHORS[section] if anchor.lower() not in claude] + assert not missing, f"guardrails.md § {section!r} elaborates rules CLAUDE.md no longer states: {missing}"