From 55497a547160083c7cc75e4ef627b3e0a84871a6 Mon Sep 17 00:00:00 2001 From: Markus Neusinger <2921697+MarkusNeusinger@users.noreply.github.com> Date: Sat, 5 Sep 2026 14:58:08 +0200 Subject: [PATCH 1/3] chore: carry the sibling retro's guardrails into this repo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The session retro that landed in kurrentschrift found this repository asymmetric on four of its lessons. Transferred here, in this repository's own conventions and against its own anchors. The precedence gap. Auto mode hands a delegated agent a reminder that prescribes editing files with sed, heredocs and short scripts. It arrives later in the context than CLAUDE.md, so it reads as the more recent instruction, and three agents in one day followed it — two of them for the single-token substitution that fills a 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 tree sits on. CLAUDE.md now states both as precedence, because an agent cannot infer precedence from the order its messages arrived in. The companion file. `.claude/guardrails.md` is new here: CLAUDE.md is loaded into every session and pays its cost on every turn, so it keeps the one-liners and the narrative moves next door. Three sections, only the ones that apply here — the two lines every delegate brief carries, the `! bash` runbook pattern for a prod pair the harness classifier refuses (with this repository's own origin-gate README as the worked case, rollback as its own script), and why a one-token substitution is not an exemption from the Edit/Write rule. The measurement rules of the sibling do not apply and are not carried. What "ready to merge" means. `review_on_push` has been false since 2026-09-03, and #11216 wrote that into CLAUDE.md and the /pull_request command — but /open-pr still carried the opposite as a gotcha and had no merge conditions at all. It now says what to require on the head SHA: non-Copilot checks completed and green (deduped by name, newest wins), a Copilot review that actually exists on the PR rather than a check run that can complete as cancelled, and zero unresolved threads. mergeable=UNKNOWN is transient; CONFLICTING gets no CI at all, which reads as "checks pending". Merging stays the owner's call. Three smaller ones. /open-pr names commit-message and PR-body files after the branch and consumes them in the step that wrote them, because the scratchpad is shared by every agent of one session. /verify-frontend § 3b generalises the ~13 px legibility floor into the rule it is an instance of: a numeric rule is measured on the page, not on the plan. /write-docs gains the claim-by-claim diff duty for any text a reader relies on, because shortening drops qualifiers before it drops sentences. tests/unit/test_agent_instructions.py grows a fifth pin: every section of the companion file maps to a binding one-liner in CLAUDE.md, and the companion has to say that CLAUDE.md outranks it — so a rule cannot end up living only where no session loads it. Not transferred: the versioned-contract duty for /verify-api. It presumes a tool that pushes shared state at the API while both sides must agree how the data is computed, and answers 409 on a mismatch. This repository's shared-state writes go straight to Postgres from the pipeline, so the mechanism has nothing to attach to here. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01UEScQMZFvxxNNyNJYryfa3 --- .claude/guardrails.md | 112 ++++++++++++++++++++++++ .claude/skills/open-pr/SKILL.md | 100 +++++++++++++++++++-- .claude/skills/verify-frontend/SKILL.md | 21 +++++ .claude/skills/write-docs/SKILL.md | 29 ++++++ CLAUDE.md | 8 +- changelog.d/sibling-retro-guardrails.md | 57 ++++++++++++ tests/unit/test_agent_instructions.py | 67 +++++++++++++- 7 files changed, 383 insertions(+), 11 deletions(-) create mode 100644 .claude/guardrails.md create mode 100644 changelog.d/sibling-retro-guardrails.md diff --git a/.claude/guardrails.md b/.claude/guardrails.md new file mode 100644 index 00000000000..f70d75ffe20 --- /dev/null +++ b/.claude/guardrails.md @@ -0,0 +1,112 @@ +# Guardrails — the long version + +`CLAUDE.md` states each working rule as one line: the rule, its shortest +reason, its date. 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, including the +secret having been disabled during the incident, so it looks nothing up. + +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..a65d62b8361 100644 --- a/.claude/skills/open-pr/SKILL.md +++ b/.claude/skills/open-pr/SKILL.md @@ -116,6 +116,26 @@ 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 message or body goes through a file — and that +file is named after the branch.** `git commit -F` and `gh pr create +--body-file` keep the prose out of shell quoting, but the scratchpad +is shared by every agent of one session, so a generic +`commitmsg.txt` / `prbody.md` gets overwritten by a parallel agent +and a later re-read commits someone else's text (sibling repo, +2026-09-05: both files were clobbered mid-run). Derive the name, or +take a private directory: + +```bash +BRANCH=$(git branch --show-current) +MSG="$SCRATCH/commitmsg-$BRANCH.txt" # or: D=$(mktemp -d) and write inside it +``` + +Write it with the Write tool, then `git commit -F "$MSG"` in the SAME +step that wrote it — never re-read one of these files a turn later to +reuse it, because between the two the file may belong to another +agent. They are scratch input to one command, not a record; the +record is the commit. + ## 3 · After opening: pipeline + review loop (do not skip) Repeat until **both** hold: all checks pass AND zero unresolved @@ -135,15 +155,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 +217,52 @@ 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. + ```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(.started_at)) | .[] | "\(.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 +289,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..11836d17ad8 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, its shortest reason, its date. 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..e656193393b --- /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. + +### 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. +- **`/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. +- **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. +- **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. +- **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. diff --git a/tests/unit/test_agent_instructions.py b/tests/unit/test_agent_instructions.py index ff268167860..02982f61e3f 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,62 @@ 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. +GUARDRAIL_SECTION_ANCHORS = { + "Delegated agents run on Opus by default": "opus by default", + "External-system writes need explicit, named authorization": "explicit, named authorization", + "Modify repo files only with the Edit/Write tools": "heredocs/sed", +} + + +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: + """Each rationale section maps to a rule CLAUDE.md states in its own right.""" + anchor = GUARDRAIL_SECTION_ANCHORS[section] + assert anchor.lower() in _flat(CLAUDE_MD.read_text(encoding="utf-8")), ( + f"guardrails.md § {section!r} has no binding counterpart in CLAUDE.md" + ) From cb37df82b3b37fb5f5f58653cdaaa68a0a72ed0f Mon Sep 17 00:00:00 2001 From: Markus Neusinger <2921697+MarkusNeusinger@users.noreply.github.com> Date: Sat, 5 Sep 2026 14:59:17 +0200 Subject: [PATCH 2/3] chore: reference the PR in the changelog fragment Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01UEScQMZFvxxNNyNJYryfa3 --- changelog.d/sibling-retro-guardrails.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/changelog.d/sibling-retro-guardrails.md b/changelog.d/sibling-retro-guardrails.md index e656193393b..1d2328d2350 100644 --- a/changelog.d/sibling-retro-guardrails.md +++ b/changelog.d/sibling-retro-guardrails.md @@ -11,7 +11,7 @@ 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. + `CLAUDE.md` outranks it. (#11605) ### Changed @@ -23,7 +23,7 @@ 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. + 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 @@ -36,22 +36,22 @@ 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. + 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. + 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. + 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. + cannot be supported is removed rather than softened. (#11605) From 98141bf32cb9cc4acc7c442f8c4ef393bbadc29e Mon Sep 17 00:00:00 2001 From: Markus Neusinger <2921697+MarkusNeusinger@users.noreply.github.com> Date: Sat, 5 Sep 2026 15:07:48 +0200 Subject: [PATCH 3/3] chore: make the transferred guardrails true of this repository MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven review findings, all of them real, all applied. The format claim. CLAUDE.md's rules mostly carry no date, so describing the compact form as "rule, reason, date" promised metadata that is not there — in the new pointer section and in the companion file's opening. Both now say what the format actually is. The rollback claim. The origin-gate rollback block does resolve the serving revision and its image; what it avoids is looking up the SECRET, which is the thing an incident may have disabled. The companion file said "it looks nothing up", which is stronger than the runbook. The scratchpad recipe was three ways wrong. It reached for a `$SCRATCH` this skill never sets, it built a filename out of a branch name that can carry a slash — `release/v1.2.3` names a directory that does not exist — and it promised the fix covered PR bodies, while the mandated `/pull_request` builds its body inline with a quoted heredoc and has no collision to avoid. It now leads with `mktemp -d`, substitutes the separators when a name is derived, and says where the rule does and does not bite. The merge-readiness dedupe picked the wrong run. A check run carries no `created_at`, and `started_at` stays null until the run begins, so `max_by(.started_at)` hands the row to the OLD completed attempt while the retry is still queued — the exact misread the step exists to prevent. It dedupes on `.id`, which grows with creation and is always set. And the pin under-asserted. A companion section that elaborates several obligations now registers all of them: a single "opus by default" anchor stayed green while the worktree line or the Edit/Write precedence was deleted from CLAUDE.md and left living only where no session loads it. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01UEScQMZFvxxNNyNJYryfa3 --- .claude/guardrails.md | 13 +++++--- .claude/skills/open-pr/SKILL.md | 47 ++++++++++++++++++--------- CLAUDE.md | 2 +- tests/unit/test_agent_instructions.py | 26 ++++++++++----- 4 files changed, 58 insertions(+), 30 deletions(-) diff --git a/.claude/guardrails.md b/.claude/guardrails.md index f70d75ffe20..9b7bcbd4524 100644 --- a/.claude/guardrails.md +++ b/.claude/guardrails.md @@ -1,8 +1,8 @@ # Guardrails — the long version -`CLAUDE.md` states each working rule as one line: the rule, its shortest -reason, its date. This file holds what does not fit there — the incident that -produced the rule, the recipe it implies, the numbers that make it credible. +`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 @@ -82,8 +82,11 @@ 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, including the -secret having been disabled during the incident, so it looks nothing up. +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 diff --git a/.claude/skills/open-pr/SKILL.md b/.claude/skills/open-pr/SKILL.md index a65d62b8361..3edd2378e7f 100644 --- a/.claude/skills/open-pr/SKILL.md +++ b/.claude/skills/open-pr/SKILL.md @@ -116,25 +116,35 @@ 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 message or body goes through a file — and that -file is named after the branch.** `git commit -F` and `gh pr create ---body-file` keep the prose out of shell quoting, but the scratchpad -is shared by every agent of one session, so a generic -`commitmsg.txt` / `prbody.md` gets overwritten by a parallel agent -and a later re-read commits someone else's text (sibling repo, -2026-09-05: both files were clobbered mid-run). Derive the name, or -take a private directory: +**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 -BRANCH=$(git branch --show-current) -MSG="$SCRATCH/commitmsg-$BRANCH.txt" # or: D=$(mktemp -d) and write inside it +D=$(mktemp -d) # or, if you name it: BRANCH=$(git branch --show-current) +MSG="$D/commitmsg.txt" # SLUG=${BRANCH//\//-}; MSG="$D/commitmsg-$SLUG.txt" ``` -Write it with the Write tool, then `git commit -F "$MSG"` in the SAME -step that wrote it — never re-read one of these files a turn later to -reuse it, because between the two the file may belong to another -agent. They are scratch input to one command, not a record; the -record is the commit. +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) @@ -230,9 +240,14 @@ push, `gh pr view --json headRefOid`: 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(.started_at)) | .[] | "\(.name): \(.status) \(.conclusion // "")"' + --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 diff --git a/CLAUDE.md b/CLAUDE.md index 11836d17ad8..d500d88d10c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,7 +27,7 @@ A companion guide `.github/copilot-instructions.md` carries the shared rules for ## Guardrails (the long version) -The rules in this file are stated as one line each: the rule, its shortest reason, its date. 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. +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 diff --git a/tests/unit/test_agent_instructions.py b/tests/unit/test_agent_instructions.py index 02982f61e3f..c6fd2608f76 100644 --- a/tests/unit/test_agent_instructions.py +++ b/tests/unit/test_agent_instructions.py @@ -362,10 +362,21 @@ def test_guardrails_split_stays_subordinate() -> None: # 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", - "External-system writes need explicit, named authorization": "explicit, named authorization", - "Modify repo files only with the Edit/Write tools": "heredocs/sed", + "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"], } @@ -387,8 +398,7 @@ def test_every_companion_section_is_registered() -> None: @pytest.mark.parametrize("section", sorted(GUARDRAIL_SECTION_ANCHORS), ids=lambda s: s[:40]) def test_companion_section_has_a_binding_rule(section: str) -> None: - """Each rationale section maps to a rule CLAUDE.md states in its own right.""" - anchor = GUARDRAIL_SECTION_ANCHORS[section] - assert anchor.lower() in _flat(CLAUDE_MD.read_text(encoding="utf-8")), ( - f"guardrails.md § {section!r} has no binding counterpart in CLAUDE.md" - ) + """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}"