chore: carry the sibling retro's guardrails into this repo - #11605
Merged
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UEScQMZFvxxNNyNJYryfa3
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UEScQMZFvxxNNyNJYryfa3
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The scratch-file workflow and check-run deduplication can fail or permit merging while a newer check remains queued.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR transfers agent-workflow guardrails from the sibling repository and adds regression checks for their documentation structure.
Changes:
- Adds detailed guardrail rationale and updates core agent instructions.
- Revises PR, frontend verification, and documentation skills.
- Adds structural tests and a changelog fragment.
File summaries
| File | Description |
|---|---|
CLAUDE.md |
Adds guardrail precedence and delegation rules. |
.claude/guardrails.md |
Documents incidents and operational recipes. |
.claude/skills/open-pr/SKILL.md |
Revises PR review and merge procedures. |
.claude/skills/verify-frontend/SKILL.md |
Requires browser-measured numeric UI verification. |
.claude/skills/write-docs/SKILL.md |
Adds claim-preservation guidance. |
tests/unit/test_agent_instructions.py |
Tests the guardrail companion structure. |
changelog.d/sibling-retro-guardrails.md |
Records the workflow changes. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 7
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UEScQMZFvxxNNyNJYryfa3
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CLAUDE.mdnow says what the Edit/Write rule outranks (an auto-mode reminder that arrives later in the context and prescribessed/heredoc editing), and the delegation rule adds the second line a brief has to carry: allgitstays inside the agent's own worktree..claude/guardrails.md, the long version of the working rules — three sections only, the ones that apply here: the two lines every delegate brief carries, the! bashrunbook pattern for a prod pair the harness classifier refuses (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.tests/unit/test_agent_instructions.pypins the split so a rule cannot end up living only where no session loads it./open-pr: that PR wrote thereview_on_push: falsecadence intoCLAUDE.mdandagentic/commands/pull_request.md, but the skill still carried "Copilot reviews every push round" as a gotcha and had no merge conditions at all. Plus three smaller transfers — branch-named scratchpad files,/verify-frontend§ 3b (measure the result, not the plan), and/write-docson shortening a text that carries claims.What landed where
CLAUDE.md§ Development Workflowgitstays in its own worktreeCLAUDE.md§ Important Rules (delegation bullet).claude/guardrails.md(new) + pointer section inCLAUDE.md/open-pr§ 3emergeablevs.mergeStateStatus,DIRTYgets no CI/open-pr§ 3e/open-pr§ 3b, and the contradicting gotcha corrected/open-pr§ 2/verify-frontend§ 3b (new)/write-docs§ "Shortening a text that carries claims"Not transferred, with the reason:
/verify-apiversioned-contract duty. It presumes a tool that pushes shared state at the API while both sides must agree how the data is computed, with the SERVER answering409on a mismatch. This repository's shared-state writes go straight to Postgres from the pipeline (sync_to_postgres), and its admin surface is/debugreads plus a cache invalidate — the mechanism has nothing to attach to here..github/copilot-instructions.mdwas left untouched. The Edit/Write rule and the delegation rule do not exist in it at all — that file deliberately carries a subset aimed at review, and the sync duty is "a rule that exists in both". Adding either would be introducing a rule, not mirroring one. Flagging it here rather than deciding it unilaterally.Two judgement calls I made myself:
/write-docsdrops the sibling's "regenerateapp/prerender/in the same push" clause, because this site serves crawlers through the live@seo_proxyrather than a committed prerender directory; and.claude/guardrails.mdattributes the incidents to the sibling repository where that is where they happened, rather than restating them as local history.Plan
N/A — sibling transfer of an already-merged retro.
Test plan
uv run --extra test pytest tests/unit/test_agent_instructions.py— 30 passed, including the three new guardrails pinsuv run --extra test pytest tests/unit— 1929 passeduv run --extra dev ruff check .andruff format --check .— cleanuv run python -m tools.changelog check --base origin/main— fragment well-formed, no[Unreleased]bullet added.claude/guardrails.mdend to end: every claim aboutinfra/cloudflare/README.md(§ "Arming, in full", § "Rolling back") matches what that file actually says/open-pr§ 3e reads correctly against this PR's own head SHA — the check-runs dedupe query returns one row per check name