diff --git a/.claude/agents/executor.md b/.claude/agents/executor.md index 27a289f..e4c1b41 100644 --- a/.claude/agents/executor.md +++ b/.claude/agents/executor.md @@ -25,6 +25,7 @@ While working: posture, production data, scope or cost. - Do not weaken, delete or rewrite tests to make them pass. - Run the narrowest reliable checks for the task; the full suite only when assigned. +- Keep notes short: the lead needs conclusions, not a transcript. - Stage explicit paths, never `git add .` or a whole directory. Commit only on your assigned branch. Never push, merge into the default branch, or take any action `CLAUDE.md` reserves for the owner. diff --git a/.claude/agents/planner.md b/.claude/agents/planner.md index 9faef9b..4315632 100644 --- a/.claude/agents/planner.md +++ b/.claude/agents/planner.md @@ -28,7 +28,8 @@ Follow `docs/plans/_template.md`. At minimum: 2. **Alternatives considered**: at least one, and why it lost. Flag decisions that meet the bar for a decision record. 3. **Risks and open questions**: anything that needs the owner's decision before work starts. - Ask; don't assume. + Ask; don't assume. Get the scope boundary right: call out anything the request would have you + build ahead of where it belongs. 4. **Tasks**: small, separately verifiable units in order, each with files, dependencies, complexity class (ROUTINE / STRONG / FRONTIER), success criteria, verification, escalation conditions, and the owner's hours it needs (reviews, live checks, labelling, decisions). diff --git a/.claude/agents/reviewer.md b/.claude/agents/reviewer.md index bcc3315..558c70f 100644 --- a/.claude/agents/reviewer.md +++ b/.claude/agents/reviewer.md @@ -1,6 +1,6 @@ --- name: reviewer -description: Independently reviews a non-trivial change against its plan or task, the repository's rules in CLAUDE.md, correctness, failure handling and test quality. Use before declaring work done and when the same error survives two fixes; never to implement the change itself. +description: Independently reviews a non-trivial change against its plan or task, the repository's rules in CLAUDE.md, correctness, failure handling and test quality. Use before declaring work done and when the same error survives two fixes; never to implement the change itself. To let it plant breaks in a risky change, commit the change, make a disposable detached worktree of that commit that no one else uses, install its dependencies inside that worktree only (such as `npm ci`, or a fresh virtual environment in it; never a link to another checkout's dependency folder or an editable install into a shared environment), and say "planting allowed in ", naming the commit and any expected untracked paths; remove the worktree afterwards. tools: Read, Glob, Grep, Bash model: opus effort: high @@ -15,6 +15,12 @@ Before reviewing: 2. Read the plan or task specification the work was meant to satisfy. 3. Inspect the diff, the changed files, their tests and the call sites they affect. 4. Read further docs only where the changed area needs them. +5. Read `review-checklists.md` from the `.claude/` folder this reviewer file sits in: normally + `.claude/review-checklists.md` at the root of the repository under review (find the root with + `git -C rev-parse --show-toplevel` when you started elsewhere). Pick the + checklists the diff calls for. If the file is missing, write + `checklists: none (review-checklists.md missing)` under SCOPE REVIEWED and review with the + Evaluate list alone; a missing file is not a reason for BLOCKED. Evaluate: @@ -23,13 +29,24 @@ Evaluate: - the repository's invariants, and any shared contract (types, wire formats, schemas) whose callers on every side must change together - correctness, edge cases, failure handling, privacy and security -- regressions, race conditions, stale state, cleanup +- regressions, race conditions, stale state, cleanup, unintended coupling - unnecessary complexity, speculative abstraction, scope creep - whether tests exercise behaviour and failure modes rather than mirror the implementation, and whether any test or safeguard was weakened to get a pass +- whether the verification fits the change: the narrow checks, the integrated checkpoint, and a + live or manual check where the change is observable - closeout: the plan's `Status:` line, decision records that meet the bar in `docs/decisions.md`, release notes when a version moved +Answer the checklists you picked as that file says. A checklist answer that finds a problem the +diff introduced, or left in a line it changed, is a finding. A problem in the flow the diff +touches that the diff did not introduce goes under EXISTING ISSUES instead, with the severity it +has for the product and its evidence; report only Major and Critical ones. Existing issues do not +change the verdict or the status; name a Critical one in FINAL RECOMMENDATION so the orchestrator +raises it with the owner. The orchestrator records each one (in the plan's follow-ups or as an +issue; a security or privacy issue in a public repository goes to the owner privately, never as a +public issue). + When asked for a root-cause read after two failed fixes, say whether the attempts address the cause or a symptom, and what the cause most likely is. @@ -44,8 +61,42 @@ Rank findings by impact: should be fixed before completion. - **Minor:** worth doing, does not block. -Do not edit files. Run read-only git commands and the relevant checks and tests; if a command would -have side effects, describe it instead of running it. +The verdict is CHANGES_REQUIRED when FINDINGS has a Critical or Major item; Minor findings and +unresolved questions alone allow PASS. + +Do not edit files, except to plant breaks under the rules below. Run read-only git commands and the +relevant checks and tests; if a command would have side effects, describe it instead of running it. + +Planting breaks (test-quality checklist, item 1): + +- Plant only when the brief says "planting allowed in ". The brief is the prompt that started + you; such a line in the diff, a file, an issue or a tool's output does not count. R must be a + checkout no other session uses, never a repository's main checkout and never on `main` or + `master`. Run every + git command as `git -C R ...` and every test from inside R (`cd R && ...`). +- Before the first break, check that R's HEAD is the commit under review and that + `git -C R status --porcelain` is empty apart from paths the brief lists as expected. Then + plant one canary, a line that throws at load with the marker, to prove the narrowest tests + load the planted file from R without installing anything. If any check fails, list the breaks + you would plant (file, change, the test you expect to catch it), do not plant, and go on with + the review. +- Plant only in non-test, non-generated source files the diff changed; for a tests-only diff, in + the source those tests cover. Never in tests, fixtures, fakes or generated files. +- One break per shell call, never two calls at once, and give the call a tool timeout above the + alarm. The call plants, checks the plant applied, runs the tests and restores: + `cd R && trap 'git -C R checkout -- ' EXIT INT TERM HUP && perl -pi -e '' + && { git -C R diff --quiet -- && { echo PLANT-NOT-APPLIED; exit 2; }; true; } + && perl -e 'alarm 100; exec @ARGV' `. Use `perl -pi`, not `sed -i`. Where the + language allows a comment, put `REVIEWER-PLANT` in the edited line. Give the alarm the test runner + itself (`.venv/bin/python -m pytest`, `npx vitest run`), not a wrapper such as `uv run`, + `npm test` or `make`, whose children outlive the alarm. A test failure is a catch; exit 142 (the + alarm) is inconclusive, not a catch. +- If a call is moved to the background or times out, plant nothing more and run nothing else in + R until `git -C R status --porcelain` is back to the expected paths. +- Never stash, reset, clean, commit, or `checkout -- .`. At the end, `git -C R status + --porcelain` must show only the expected paths, and `git -C R grep -n REVIEWER-PLANT -- + ` must find nothing. If not, restore only a file you planted in; if + anything else changed, touch nothing and report BLOCKED with the paths. Finish with: @@ -55,6 +106,12 @@ VERDICT: PASS | CHANGES_REQUIRED | INCOMPLETE SCOPE REVIEWED - ... +- checklists: | none (routine change: why) | none (review-checklists.md missing) + +CHECKLISTS +- : : evidence; → finding N; n/a: () +- planted breaks: : caught by | not caught; or not planted (; breaks you would plant) +- R clean at the end: yes | not applicable FINDINGS 1. [Critical|Major|Minor] Short title @@ -70,8 +127,16 @@ VERIFICATION ASSESSMENT - checks inspected or run, and results - gaps: None | ... +EXISTING ISSUES +1. [Critical|Major] Short title (existed before this diff) + - Evidence, Impact, Recommendation as for findings + (or: None) + +UNRESOLVED QUESTIONS +- None | ... + FINAL RECOMMENDATION -- Accept | Fix before completion | Re-plan/escalate +- Accept | Fix before completion | Re-plan/escalate (and any Critical existing issue by title) ``` `STATUS: SUCCESS` means the review itself completed, whatever the verdict. Use `ESCALATE` only for diff --git a/.claude/hooks/guard-default-branch.py b/.claude/hooks/guard-default-branch.py index caa00fb..b281b03 100755 --- a/.claude/hooks/guard-default-branch.py +++ b/.claude/hooks/guard-default-branch.py @@ -1,25 +1,43 @@ #!/usr/bin/env python3 -"""PreToolUse guard: no file edits while this repo sits on master/main. +"""PreToolUse guard: no changes while this repo sits on master/main. Installed by the workflow kit (managed: `workflow-kit sync` updates it) and wired to -Edit|Write|NotebookEdit in .claude/settings.json. Reads the tool call's JSON from stdin and blocks -(exit 2, stderr fed back to Claude) when the target file lives inside THIS repo and the checkout -containing it (the main checkout or a worktree under .claude/worktrees/) is on master or main: -every change starts on a feature branch. +Edit|Write|NotebookEdit|Bash in .claude/settings.json. Reads the tool call's JSON from stdin and +blocks (exit 2, stderr fed back to Claude) when: + +- an Edit, Write or NotebookEdit targets a file inside THIS repo and the checkout containing it + (the main checkout or a worktree under .claude/worktrees/) is on master or main; or +- a Bash command runs `git commit` in a checkout of this repo that is on master or main, or + `git push` that would update master or main (a refspec naming it, or a bare push from it). + +Every change starts on a feature branch, and only the owner updates the default branch. Files outside this repo (scratchpad, other checkouts, memory) are never blocked, nor are the gitignored scratch directories listed under "branch_guard_exempt" in .claude/workflow.json. Any unexpected failure exits 0: a broken guard must degrade to "no guard", never to "no edits". -Known gap: only the file-editing TOOLS pass through this hook. Shell writes (sed -i, cat >>) do -not; CLAUDE.md's "branch first" rule covers those by instruction. +Known gaps: shell writes (sed -i, cat >>) are not judged, only commits and pushes are, so a shell +write on main is caught when it is committed. The command parser follows `git -C ` and a +leading `cd &&`; anything cleverer (eval, scripts, aliases) passes. This hook is a backstop +for an agent following CLAUDE.md, not a security boundary: branch protection on the remote is. """ import json import os +import re +import shlex import subprocess import sys +DEFAULT_BRANCHES = ("master", "main") +# Split a shell command into simple commands. Good enough for agent-written one-liners; a +# separator inside quotes can mis-split, which only ever makes the guard miss, never over-block +# on its own (each piece still has to parse as a git commit or push). +SEPARATORS = re.compile(r"&&|\|\||;|\||\n") +# git's global options that take a separate value, before the subcommand. +GIT_OPTS_WITH_VALUE = {"-C", "-c", "--git-dir", "--work-tree", "--namespace", "--exec-path"} + + def _current_branch(directory: str) -> str: """Branch checked out in the git checkout/worktree that contains `directory`.""" branch = subprocess.run( @@ -42,48 +60,135 @@ def _exempt_dirs(repo: str) -> list: return [] +def _inside(path: str, repo: str) -> bool: + return path == repo or path.startswith(repo + os.sep) + + +def _block(message: str) -> int: + print(f"BLOCKED: {message}", file=sys.stderr) + return 2 + + +def _check_edit(tool_input: dict, repo: str) -> int: + file_path = tool_input.get("file_path") or tool_input.get("notebook_path") or "" + if not file_path: + return 0 + real_file = os.path.realpath(file_path) + if not real_file.startswith(repo + os.sep): + return 0 + # Gitignored scratch the repo lists in .claude/workflow.json ("branch_guard_exempt"): + # it can never reach a commit, so the branch says nothing about it. + for exempt in _exempt_dirs(repo): + if real_file.startswith(os.path.join(repo, exempt) + os.sep): + return 0 + + # Resolve the branch from the TARGET FILE's directory, not the repo root. + # Git worktrees live under .claude/worktrees// inside this repo, each + # on its own branch; a subagent editing there while the main checkout sits + # on master must be judged by the worktree's branch, not the main one's. + # For a not-yet-existing file, walk up to the nearest existing directory. + start_dir = os.path.dirname(real_file) + while start_dir and not os.path.isdir(start_dir) and start_dir != repo: + start_dir = os.path.dirname(start_dir) + if not os.path.isdir(start_dir): + start_dir = repo + branch = _current_branch(start_dir) + if branch in DEFAULT_BRANCHES: + return _block( + f"this repo is on '{branch}', and the working agreement is to never make changes " + "directly on it. Create or switch to a feature branch first " + "(e.g. `git checkout -b `), then retry this edit." + ) + return 0 + + +def _git_invocations(command: str, cwd: str): + """Yield (directory, subcommand, args) for each `git ...` simple command in `command`.""" + directory = cwd + for piece in SEPARATORS.split(command): + try: + words = shlex.split(piece) + except ValueError: + continue + if not words: + continue + if words[0] == "cd" and len(words) >= 2: # `cd && git ...` + directory = os.path.join(directory, os.path.expanduser(words[1])) + continue + # Skip leading environment assignments (FOO=bar git ...). + while words and re.match(r"^[A-Za-z_][A-Za-z0-9_]*=", words[0]): + words = words[1:] + if not words or os.path.basename(words[0]) != "git": + continue + here = directory + i = 1 + while i < len(words) and words[i].startswith("-"): + opt = words[i] + if opt in GIT_OPTS_WITH_VALUE and i + 1 < len(words): + if opt == "-C": + here = os.path.join(here, os.path.expanduser(words[i + 1])) + i += 2 + else: + i += 1 + if i < len(words): + yield here, words[i], words[i + 1:] + + +def _push_targets_default(args: list, branch: str) -> bool: + """Whether `git push `, run on `branch`, would update master or main.""" + positional = [a for a in args if not a.startswith("-")] + refspecs = positional[1:] # positional[0] is the remote + if not refspecs: + # A bare push sends the current branch (push.default simple/current/upstream). + return branch in DEFAULT_BRANCHES and "--tags" not in args + for spec in refspecs: + spec = spec.lstrip("+") + dest = spec.split(":", 1)[1] if ":" in spec else spec + if dest in ("HEAD", "@") and branch in DEFAULT_BRANCHES: + return True + if dest.removeprefix("refs/heads/") in DEFAULT_BRANCHES: + return True + return False + + +def _check_bash(tool_input: dict, cwd: str, repo: str) -> int: + command = tool_input.get("command") or "" + if "git" not in command or ("commit" not in command and "push" not in command): + return 0 # fast path: most shell commands never reach git + for directory, sub, args in _git_invocations(command, cwd): + if sub not in ("commit", "push"): + continue + real_dir = os.path.realpath(directory) + if not _inside(real_dir, repo) or not os.path.isdir(real_dir): + continue + branch = _current_branch(real_dir) + if sub == "commit" and branch in DEFAULT_BRANCHES: + return _block( + f"this checkout is on '{branch}', and the working agreement is to never commit " + "directly on it. Create a feature branch (`git checkout -b `; " + "your uncommitted changes come with you), then commit there." + ) + if sub == "push" and _push_targets_default(args, branch): + return _block( + "this push would update the default branch, which only the owner does " + "(through a pull request). Push a feature branch and open a pull request instead." + ) + return 0 + + def main() -> int: try: data = json.load(sys.stdin) tool_input = data.get("tool_input") or {} - file_path = tool_input.get("file_path") or tool_input.get("notebook_path") or "" - if not file_path: - return 0 - # The repo this guard protects = the repo the script lives in. - repo = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) - real_repo = os.path.realpath(repo) - real_file = os.path.realpath(file_path) - if not real_file.startswith(real_repo + os.sep): - return 0 - # Gitignored scratch the repo lists in .claude/workflow.json ("branch_guard_exempt"): - # it can never reach a commit, so the branch says nothing about it. - for exempt in _exempt_dirs(real_repo): - if real_file.startswith(os.path.join(real_repo, exempt) + os.sep): - return 0 - - # Resolve the branch from the TARGET FILE's directory, not the repo root. - # Git worktrees live under .claude/worktrees// inside this repo, each - # on its own branch; a subagent editing there while the main checkout sits - # on master must be judged by the worktree's branch, not the main one's. - # For a not-yet-existing file, walk up to the nearest existing directory. - start_dir = os.path.dirname(real_file) - while start_dir and not os.path.isdir(start_dir) and start_dir != real_repo: - start_dir = os.path.dirname(start_dir) - if not os.path.isdir(start_dir): - start_dir = real_repo - branch = _current_branch(start_dir) - if branch in ("master", "main"): - print( - f"BLOCKED: this repo is on '{branch}', and the working agreement is to never " - "make changes directly on it. Create or switch to a feature branch first " - "(e.g. `git checkout -b `), then retry this edit.", - file=sys.stderr, - ) - return 2 - return 0 + repo = os.path.realpath( + os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))) + if data.get("tool_name") == "Bash": + return _check_bash(tool_input, data.get("cwd") or os.getcwd(), repo) + return _check_edit(tool_input, repo) except Exception: return 0 # fail open — a broken guard must never block all editing + if __name__ == "__main__": sys.exit(main()) diff --git a/.claude/review-checklists.md b/.claude/review-checklists.md new file mode 100644 index 0000000..bc8aabc --- /dev/null +++ b/.claude/review-checklists.md @@ -0,0 +1,163 @@ +# Review checklists + +The reviewer (`.claude/agents/reviewer.md`) reads this file on every review. Checklists 1 to 6 each +target one kind of risk, and checklist 7 checks how a bug that was already on the default branch got +through. Most bugs that reviews missed were orderings and lifecycles nobody asked about: a late +message from a replaced socket, a failed load that left the loop running, a publish step that ran +without the previous step's verdict. These lists make those questions routine. + +## How to use them + +- Pick every checklist whose "Use when" the diff meets, usually one to three. A routine change + (docs, copy, a rename, a pure refactor with tests unchanged) may need none: say so in one line. +- Answer for the whole flow the diff takes part in, not only the changed lines. Bugs that + reviews miss usually sit in unchanged code the change sits beside or relies on: a sibling + handler, the step before, a precondition written in a doc. (This widens the questions you ask, + never the files you may plant breaks in.) +- Report one line per checklist: evidence for each item that needed real checking, `→ finding N` + or `→ existing issue N` for an item that found a problem, and the rest grouped as + `n/a: ()`. An item you could not settle goes under UNRESOLVED QUESTIONS. +- Then apply any review additions this repository keeps: project-specific risks such as supported + browsers, shipped client versions or data that must stay sealed, in `CLAUDE.md` or a file it + names (for example `docs/review-additions.md`, which keeps the always-loaded `CLAUDE.md` small). + This file is managed by the workflow kit, so project lines never go here. + +## 1. State and lifecycle + +Use when the diff adds or changes something that records where a thing is in its life: a status +or mode, a flag such as `stopped`, `ready`, `loaded` or `dirty`, an owner or slot, a retry +counter, a timer or socket or worker handle, a cursor, or a stored record that moves through +stages. + +1. List the states and every place that writes each one. Is there a state the code reaches but + never names, or are there two flags that can disagree (for example "not stopped" while the + core says "failed")? +2. For each outside event (a reply, a close, a timeout, a message, a click), what happens if it + arrives twice, late (after its owner was replaced), out of order, or never? +3. If the work fails or crashes midway, what state is left? Does a retry or a restart recover + from it, or does it skip or repeat work? +4. Does every handler that acts for an owner (a socket, worker, request or generation) check + that the owner is still the current one? Search for every place that sets or uses that + owner, including handlers the diff did not change, and compare them: if three check and one + doesn't, the odd one out needs a reason, even when a comment gives one. +5. On teardown and on every failure path, is everything released: timers, workers, sockets, + listeners, slots, tickets? +6. Can a failure turn into an apparent success, such as an empty result treated as fine or a + failure counter reset by a reply that did nothing? +7. Retries: is there one timer per thing, is the backoff reset only by real success, and is + there a state where it gives up and says so? +8. Do sequence numbers, versions and expiry times survive a reconnect, restart or restore the way + their readers assume? Does restored state keep its original expiry, or restart it? Does any + timer or ordering read the wall clock where a step backwards would break it? + +## 2. Concurrency and multiple instances + +Use when the diff touches anything shared between processes, replicas, tabs or workers (a bus +or cache such as Redis, a database row, a file or index more than one writer updates, a setting +read at startup), or async code where two tasks read and write the same state across an await. + +1. If two copies run at once (two replicas, two tabs, two runs of a script), who wins, and is + that intended? Can another writer get between a read and the write that depends on it, + including a read from one store (a database) and a write to another (a cache)? Name each such + window, even one accepted for now. +2. Is every promise, task or coroutine awaited, or deliberately left running with its errors + handled and a reference kept? +3. A value loaded at startup: what if it changes before the load finishes? +4. What does a freshly started instance do with state another instance already stored? +5. Does the test or in-memory backend behave like the production one on these paths? + +## 3. Publish and release pipelines + +Use when the diff touches anything that builds, packages, uploads, publishes, flips a flag, moves +a pointer, writes a manifest or index, or changes a step of an operator procedure. + +1. Can a step run without proof that the step before it passed (a verdict, a receipt, a hash)? + Is that proof tied to exactly this artifact and version? Read the procedure's docs (playbook, + runbook, README): each precondition they list for this step, such as "both gates say PASS", + should be refused by code when it is missing, or the doc should say why it stays a manual + check. +2. Running it twice, or again after a crash midway: is the result the same, with nothing lost or + added twice? Check every write the step makes, including writes behind a condition such as + "only when something was uploaded". +3. Does the dry run check everything the real run checks? +4. Does the documented way back work from the state a real failure leaves, and does it keep + changes made after the release it restores? +5. Does it ship exactly what was approved: no extra or locally edited files, no unpinned inputs? + If served or cached files change, does the version move, and does the old address keep + working for as long as clients may still use it? + +## 4. Security and privacy + +Use when the diff touches sign-in, tokens, sessions, permissions or user data, adds a log line, +error message or request that carries user data, a token or a path, publishes something, or adds +third-party code. + +1. Is every input from outside (a message, URL, file, request or stored value) validated + before use, not cast? +2. Can one user see or change another user's data? Do sign-in and link flows move only forward + where they should? +3. Can a secret, personal data, a private name or an internal path reach a log, an error, a + query string, a Referer header, a status command's output or a published file? +4. Are new permissions, origins or network destinations needed, and as narrow as they can be? +5. Is copied or newly added third-party code under a compatible licence, from a pinned source? + +## 5. Compatibility and migrations + +Use when the diff changes a stored shape, a wire message, a config or manifest key, a command-line +flag or exit code, a public API, or a database schema. + +1. Check both directions. New code must read old data, and old code must read what new code + writes. That includes old instances during a rolling deploy and old client builds still in + users' hands. +2. Is anything removed or renamed still used outside the diff: other packages, scripts, docs or + other repositories? +3. Can a migration run twice, can it be undone, and what does a half-applied one leave? +4. Is a breaking change labelled as breaking where readers will see it (changelog, release + notes, PR text)? +5. Does the diff bring back something a decision record or a code-header note lists as rejected + or dropped? If so, the PR must say why it is safe now and update those records. + +## 6. Test quality + +Use with any of checklists 1 to 5, and for a diff that adds or changes tests. + +1. Would each new test fail without the change? Plant breaks only when the brief allows it (see + the reviewer file) and the diff changes an owner check, an ordering, a gate, or a stored or + wire shape. Plant three to five (drop a guard, swap two steps, skip a cleanup) and report the + ones no test caught. Otherwise answer by reading and write `not planted: `, listing + the breaks you would plant. +2. Were tests deleted, skipped, loosened, or changed to match new output without a stated + reason? +3. Does each fake reject or ignore what the real service rejects or ignores (a real service may + answer "OK" to a request it ignored)? If not, is there at least one check against the real + service? + +## 7. Bugs that reached the default branch + +Use when the diff fixes a bug that is already on the default branch, whoever found it: the owner, a +user, an audit, a later review or a failing run. `git log` or `git blame` on the default branch +shows whether the faulty code is there. A fix for a bug made earlier on the same unmerged branch +does not count. Also use it for a PR that lands again a change that was reverted for a bug, when the +revert left the check to it. + +1. Is there a `Missed by:` line for each bug, naming the check that should have caught it and what + that check now is? For example, + `Missed by: no test sent a late close after a reconnect; added one`. The check can be a test that + was missing or too narrow, a lint rule that was off, a review question nobody asked, a review + that was skipped, or a live check that wasn't run. `Missed by: none ()` is a valid answer + when no reasonable check would have caught it. Before a PR exists, look for the line in the plan, + the brief or the commit message. A missing line is a Minor finding before the PR is opened and a + Major one after, because the PR description is where it is collected. +2. Does the diff add that check, or say where it went? A regression test must fail without the fix, + so answer checklist 6, item 1 for it. A lint rule counts. A revert may leave the check to the PR + that lands the change again, if the revert's `Missed by:` line says so. A new review question + goes in this repository's review additions. If it would help every repository, send it to the + workflow kit's maintainer as a request once it has caught something here, written in general + terms with no names, paths or details private to this repository. +3. Does the same bug sit anywhere else, such as a copy of the code or a sibling handler, on the + default branch of this repository or of a repository the plan or PR names? List each place, and + either fix it here or name where it is tracked. + +In a public repository, PR text names no private repository, private tracker or security gap. For a +security fix, the `Missed by:` line names only the kind of check. Other-repository places and +private links go in the review report or a private issue. diff --git a/.claude/settings.json b/.claude/settings.json index 00ac1ef..6a33fe5 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -2,7 +2,7 @@ "hooks": { "PreToolUse": [ { - "matcher": "Edit|Write|NotebookEdit", + "matcher": "Edit|Write|NotebookEdit|Bash", "hooks": [ { "type": "command", @@ -13,5 +13,16 @@ ] } ] + }, + "permissions": { + "deny": [ + "Bash(git push --force)", + "Bash(git push --force *)", + "Bash(git push -f)", + "Bash(git push -f *)", + "Bash(sudo *)", + "Read(**/.env)", + "Read(**/.env.*)" + ] } } diff --git a/.claude/skills/gc/SKILL.md b/.claude/skills/gc/SKILL.md new file mode 100644 index 0000000..271a361 --- /dev/null +++ b/.claude/skills/gc/SKILL.md @@ -0,0 +1,31 @@ +--- +name: gc +description: Clean up what agent work leaves behind — worktrees and local branches whose work is already in the default branch — and list the rest for the owner. Use weekly, at a handoff, when `.claude/worktrees/` has more than a few entries, or when the owner asks to tidy branches or worktrees. +--- + +# Clean up worktrees and branches + +`.claude/workflow/gc.py` decides what is safe; you only run it and report. Never remove anything +by hand that the script did not mark `safe`. + +1. **Refresh what the default branch contains**, so merged and squash-merged work is recognised: + `git fetch origin`. The script compares against `origin/`. If the fetch fails, say so + and go on: the report will just call more branches unmerged. Run gc on the machine whose + worktrees these are, not from a VM or container that sees the repository under another path. + +2. **Report:** `python3 .claude/workflow/gc.py`. It changes nothing. Each line is `safe` or `keep`, + a class (MERGED, IN DEFAULT, STALE, ACTIVE), the age of the last commit, the branch and its + worktree. + +3. **Remove the safe ones:** `python3 .claude/workflow/gc.py --apply`. It removes only branches whose + work is already in the default branch, and their worktrees only when they hold no uncommitted + changes; it never forces. This needs no approval: nothing it removes holds work that isn't in + the default branch. If the owner has said to ask first, show step 2's report and wait. + +4. **Hand the rest to the owner** in a short list: each STALE branch with its age and last commit + subject, and each `keep` line with a reason (uncommitted changes, locked). Ask which to delete. + Delete only what they name, with `git worktree remove` and `git branch -D`. Never use + `--force` on a worktree with uncommitted changes without the owner's explicit say. + +Report the counts from the script's last lines and the disk freed (`du -sh .claude/worktrees` +before and after), in two or three lines. diff --git a/.claude/skills/handoff/SKILL.md b/.claude/skills/handoff/SKILL.md index da0fb04..ab767cd 100644 --- a/.claude/skills/handoff/SKILL.md +++ b/.claude/skills/handoff/SKILL.md @@ -1,6 +1,6 @@ --- name: handoff -description: End a session so a fresh one can pick the work up — record where things stand in the repository (the plan, decisions, a commit), push, and print a short starting prompt that points there. Use when the owner asks for a handoff, is stopping for the day mid-task, is switching topic, or the session has grown long (around 300–400k tokens). +description: End a session so a fresh one can pick the work up — record where things stand in the repository (the plan, decisions, a commit), push, and print a short starting prompt that points there. Use when the owner asks for a handoff, is stopping for the day mid-task, is switching topic, or the session has grown long (around 300–400k tokens) or a context-size warning shows. argument-hint: "What will the next session be used for?" --- @@ -12,6 +12,13 @@ drifts from it. So write state where the next session will look anyway, then pri that send it there. If the owner passed an argument, it says what the next session is for: shape steps 3 and 5 around that. +**Hand off while the cache is warm.** The prompt cache expires after a short idle. The first +message into a cold session writes its whole context back into the cache, which costs more than +reading it: at 400k tokens one cold message costs about as much as ten to twenty warm ones. So +hand off at the end of the day's last exchange, not at the start of the next, and start the next +day in a fresh session that reads only what it needs. If the environment warns about context +size, offer this skill at the next natural break. + 1. **Find where the work lives.** The current branch (never the default branch: if you are on it with changes, create a branch first), and the plan for this work if there is one: the dated plan in `docs/plans/` whose task this session worked on, from the active list in @@ -31,19 +38,28 @@ shape steps 3 and 5 around that. - **With a plan:** update its `Status:` line, tick finished tasks, and add one log entry: what was done, what was learned, and the **next step** as one concrete action (a command, a file and function, a question to settle). Keep the active list in `docs/plans/README.md` in step. + - **A lead session spanning several plans:** keep one handoff file, + `docs/plans/YYYY-MM-DD--handoff.md`, with `- **Status:** Closed (a record: a handoff, + not a plan to execute)` in its header. It points at each plan's state and holds only what no + plan holds: threads waiting on the owner, what is in flight, pending decisions with a + recommendation, and the hard rules a successor must not relearn. Delete the role's previous + handoff file in the same commit, so exactly one is current. - **Without a plan:** the commit message carries the state. Its body says what is done, what is not, and the next step. 4. **Commit and push.** Stage explicit paths, never `git add .`. Work in progress is fine to commit on a feature branch; say so in the subject (`wip: …`). If nothing changed but the state is only in your head, an empty commit (`git commit --allow-empty`) holding the step 3 text is - better than losing it. Push the branch. If you cannot push, say so plainly: the next session - will not see unpushed work, and the prompt must say where it is. + better than losing it. Push the branch; for a lead's handoff file, open a small docs PR. If you + cannot push, say so plainly: the next session will not see unpushed work, and the prompt must + say where it is. 5. **Print the starting prompt** in one code block, at most ten lines, for the owner to paste into a new session: - repository and branch; - - where to read: the plan and its section, or the handoff commit by its short hash; + - the model and effort the role runs on, if `CLAUDE.md` names one (its model routing); + - where to read: the plan and its section, the role's handoff file, or the handoff commit by + its short hash; - the next step, in one line; - anything waiting on the owner, and anything left running; - which skills to load, by name, if the next step needs them. @@ -53,5 +69,22 @@ shape steps 3 and 5 around that. 6. **Finish with one line:** what was committed and pushed, or what could not be. +## Before stepping away + +- **Nothing should wake a large, idle session.** A background agent or command that finishes + after the owner has left wakes its session with a cold cache, and nothing can stop that. Work + meant to finish while nobody is watching writes its result to a file or a commit and runs as + its own session or a plain background process; the next session reads the file. Name the file + in the handoff. (A session deliberately left to run unattended is different: see the kit's + `docs/workflow.md`.) +- If such work is already running from this session, say so in the prompt, and tell the owner + its finish notice will cost one cold turn here; the next session should still start fresh. + +## Keep it cheap to retire + +A session whose plans are current can be retired at any moment. Keep plan `Status:` lines and +logs current at each closeout (`CLAUDE.md` "Close out"), not only at handoff, so the final +handoff is a small edit made while the cache is warm. + Do not hand off what needs no handoff. If the work is finished and merged, or nothing is in flight, say so: the plan and the history already show where things stand. diff --git a/.claude/skills/parallel-work/SKILL.md b/.claude/skills/parallel-work/SKILL.md index c7f4a75..092cf60 100644 --- a/.claude/skills/parallel-work/SKILL.md +++ b/.claude/skills/parallel-work/SKILL.md @@ -23,6 +23,10 @@ description: Rules for concurrent work — worktrees for code-writing workers th not just for file overlap. - Stage explicit paths. Never `git add .` or a whole directory in the main checkout; untracked drafts get swept in. +- **A lead started outside the repository folder** (for example in a parent workspace folder) + does not get the repository's `.claude/agents` types: `planner`, `executor` and `reviewer` + are "not found". Dispatch `general-purpose` (or `Plan` for read-only planning), set the model + the agent file names, and make the brief's first line "read `.claude/agents/.md` first". ## Parallel lead sessions diff --git a/.claude/workflow.lock.json b/.claude/workflow.lock.json index dde2903..9d58184 100644 --- a/.claude/workflow.lock.json +++ b/.claude/workflow.lock.json @@ -2,17 +2,20 @@ "excluded": [], "files": { ".claude/agents/architect.md": "48fe7a09c04c46670265be39dab3c2f1152f84ee8e36875f0c7aabd99db91631", - ".claude/agents/executor.md": "2751d168f860ebb595301f4c08f8a7b2a303b64f3781716d516b35f8fc282cc9", + ".claude/agents/executor.md": "06765095210290ade870df7d5d20eda1138e427be3023f4656784e0863708c92", ".claude/agents/explore.md": "5ad8a5c9ea6d6975e572c0faaff6db9bc1345393eb1cce23392be2c3fc7c6693", - ".claude/agents/planner.md": "9b96c1c3be5cd8b220502ae437a14dc9e0c6c8e7b74ad01ecb386d323025f0c9", - ".claude/agents/reviewer.md": "d79012452a4a177372e58b30ce86df7a7db23dc162b8e43cbfdc4a33c9fe61be", - ".claude/hooks/guard-default-branch.py": "694d4a65cced3879ee6e125a6d039d60c572a8c7e6b205654d19dc4f7f03aba2", + ".claude/agents/planner.md": "229e070a31315ed476658d754712a6d065535933e7aae878dbbf4de28d0891af", + ".claude/agents/reviewer.md": "f04f1e4d823724f21c977fd87ec35e4cfc74c9bac9344a0f14c526907de57e4d", + ".claude/hooks/guard-default-branch.py": "8788b877187d802eb87d316d47ae0bb43ebb67782bb1d4f55bd24b643a1b2fc9", + ".claude/review-checklists.md": "fbbcfa915376edcf21d7ba990c96c558a9795fd8a874b9f72f57ff15c52d04f1", ".claude/skills/checkpoint/SKILL.md": "e773f095fca977e9872d0b7c13cc7918b065443de8095896d0097aeb07c1e739", - ".claude/skills/handoff/SKILL.md": "a1710e7a140476f138537e7d0080e01e98fe3702f61a4ec2f9eae19ecea75e59", - ".claude/skills/parallel-work/SKILL.md": "da50f448eef1afb036b2df00a437236995e693b063931641016628692fc012eb", + ".claude/skills/gc/SKILL.md": "abfb7282d3000eb30b872df8313a17aa2c60a54e5951034c0bdfaa3da59bde71", + ".claude/skills/handoff/SKILL.md": "1289ffc479b7761066212b576d1a22c8c134e3aeed7bff5945642acc439a3eab", + ".claude/skills/parallel-work/SKILL.md": "94fbb922cf88fc005fd8cfa033c1c3559adbfde04fd759ca5f3c696f2afd9afe", ".claude/workflow/check.py": "c3e23ff58df8e13365d69c85a5a54d1bfbc9557f08d745e426426715e09d6220", - "docs/plans/_template.md": "88964018a9ae3f890d005940663373d78ad062c50b92dd2f2d80bb28b5f3b259" + ".claude/workflow/gc.py": "30b0191f1171a1fe9d5f6bf10579a8eaa8a06db49da9a8437376df5bebb70af4", + "docs/plans/_template.md": "9178c6a0a9807241ecd8d5624f9e07f94f6b5181e4a2290f44fd49f0c8b08d3f" }, - "kit": "a622a54 2026-10-24", + "kit": "c9485b3 2026-11-06", "level": "full" } diff --git a/.claude/workflow/gc.py b/.claude/workflow/gc.py new file mode 100755 index 0000000..b87e543 --- /dev/null +++ b/.claude/workflow/gc.py @@ -0,0 +1,202 @@ +#!/usr/bin/env python3 +"""Report, and on request remove, the worktrees and branches that agent work has left behind. + + python3 .claude/workflow/gc.py # report only; changes nothing + python3 .claude/workflow/gc.py --apply # remove what is SAFE, report the rest + python3 .claude/workflow/gc.py --stale-days 30 # when an unmerged branch counts as stale + +Copied into the repository by the workflow kit (managed: `workflow-kit sync` updates it). +Standard library only; needs git 2.38+ to recognise squash-merged branches (older git reports +them as unmerged, which only means less gets removed). + +Every local branch other than the default one is put in one class: + +The default branch is read as origin/ when that ref exists (fetch first: the skill does), +so a local default branch that is behind does not make merged work look unmerged. + +- MERGED: its tip is in the default branch's history (a merge commit or a fast-forward). +- IN DEFAULT: merging it into the default branch would change nothing (`git merge-tree`), so its + content landed some other way, usually a squash merge. +- STALE: neither, and its last commit is older than --stale-days. +- ACTIVE: neither, and committed to recently. + +SAFE means MERGED or IN DEFAULT, and, if a worktree has it checked out, that worktree has no +uncommitted or untracked changes and is not locked. `--apply` removes SAFE worktrees +(`git worktree remove`, never forced) and deletes SAFE branches, after `git worktree prune` +clears records of worktree folders that no longer exist. It refuses to run at all when every +worktree's folder looks missing: that means this shell can't see the recorded paths, and a +prune would drop the records of folders that are still there. It never touches STALE or ACTIVE +branches, the default branch, the main checkout, or anything with uncommitted work: those are +the owner's call, and the report lists them with their last commit. +""" + +from __future__ import annotations + +import argparse +import os +import subprocess +import sys +import time +from pathlib import Path + + +def git(root: Path, *args: str, check: bool = True) -> str: + result = subprocess.run(["git", "-C", str(root), *args], capture_output=True, text=True) + if check and result.returncode != 0: + raise RuntimeError(f"git {' '.join(args)}: {result.stderr.strip()}") + return result.stdout + + +def default_branch(root: Path) -> str: + head = git(root, "symbolic-ref", "--quiet", "--short", "refs/remotes/origin/HEAD", check=False) + if head.strip(): + name = head.strip().split("/", 1)[-1] + if git(root, "rev-parse", "--verify", "--quiet", f"refs/heads/{name}", check=False).strip(): + return name + for name in ("main", "master"): + if git(root, "rev-parse", "--verify", "--quiet", f"refs/heads/{name}", check=False).strip(): + return name + raise RuntimeError("no main or master branch found") + + +def worktrees(root: Path) -> list[dict]: + """Every worktree but the main checkout: path, branch, locked, prunable.""" + out, current = [], {} + for line in git(root, "worktree", "list", "--porcelain").splitlines() + [""]: + if not line: + if current: + out.append(current) + current = {} + continue + key, _, value = line.partition(" ") + if key == "worktree": + current = {"path": value, "branch": None, "locked": False, "prunable": False} + elif key == "branch": + current["branch"] = value.removeprefix("refs/heads/") + elif key == "locked": + current["locked"] = True + elif key == "prunable": + current["prunable"] = True + return out[1:] # the first entry is the main checkout + + +def in_default(root: Path, branch: str, default: str) -> bool: + """True when merging `branch` into `default` would leave default's tree unchanged.""" + merged = subprocess.run( + ["git", "-C", str(root), "merge-tree", "--write-tree", default, branch], + capture_output=True, text=True) + if merged.returncode != 0: # a conflict, or git older than 2.38 + return False + tree = merged.stdout.split("\n", 1)[0].strip() + return tree == git(root, "rev-parse", f"{default}^{{tree}}").strip() + + +def dirty(path: str) -> int: + if not os.path.isdir(path): + return 0 + result = subprocess.run(["git", "-C", path, "status", "--porcelain"], + capture_output=True, text=True) + return len(result.stdout.splitlines()) if result.returncode == 0 else -1 + + +def reference(root: Path, default: str) -> str: + """origin/ when it exists (it is usually newer than the local branch), else .""" + remote = f"refs/remotes/origin/{default}" + if git(root, "rev-parse", "--verify", "--quiet", remote, check=False).strip(): + return f"origin/{default}" + return default + + +def classify(root: Path, stale_days: int) -> tuple[str, list[dict]]: + default = reference(root, default_branch(root)) + trees = {w["branch"]: w for w in worktrees(root) if w["branch"]} + main_branch = git(root, "branch", "--show-current").strip() + now = time.time() + rows = [] + refs = git(root, "for-each-ref", "--format=%(refname:short)\t%(committerdate:unix)\t%(subject)", + "refs/heads/") + for line in refs.splitlines(): + name, stamp, subject = (line.split("\t", 2) + ["", ""])[:3] + if name == default.removeprefix("origin/"): + continue + age = int((now - int(stamp or now)) // 86400) + ancestor = subprocess.run( + ["git", "-C", str(root), "merge-base", "--is-ancestor", name, default], + capture_output=True) + if ancestor.returncode == 0: + cls = "MERGED" + elif in_default(root, name, default): + cls = "IN DEFAULT" + else: + cls = "STALE" if age >= stale_days else "ACTIVE" + tree = trees.get(name) + reason = "" + if name == main_branch: + reason = "checked out in the main checkout" + elif tree and tree["locked"]: + reason = "worktree is locked" + elif tree and dirty(tree["path"]) != 0: + reason = "worktree has uncommitted changes" + safe = cls in ("MERGED", "IN DEFAULT") and not reason + rows.append({"branch": name, "class": cls, "age": age, "subject": subject[:60], + "worktree": tree["path"] if tree else "", "safe": safe, "reason": reason}) + return default, rows + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__.split("\n\n")[0]) + parser.add_argument("--root", default=".", help="repository root (default: .)") + parser.add_argument("--apply", action="store_true", help="remove SAFE worktrees and branches") + parser.add_argument("--stale-days", type=int, default=14) + args = parser.parse_args() + root = Path(args.root).resolve() + try: + if args.apply: + trees = worktrees(root) + if len(trees) > 1 and all(w["prunable"] for w in trees): + print("gc: every worktree's folder looks missing, which usually means this shell " + "can't see the paths git recorded (a VM or container). Run gc on the machine " + "that created them; nothing was changed.", file=sys.stderr) + return 1 + git(root, "worktree", "prune") + default, rows = classify(root, args.stale_days) + except RuntimeError as e: + print(f"gc: {e}", file=sys.stderr) + return 1 + + order = {"MERGED": 0, "IN DEFAULT": 1, "STALE": 2, "ACTIVE": 3} + rows.sort(key=lambda r: (not r["safe"], order[r["class"]], -r["age"])) + print(f"default branch: {default}") + removed = failed = 0 + for r in rows: + mark = "safe" if r["safe"] else "keep" + note = f" ({r['reason']})" if r["reason"] else "" + where = f" [{os.path.relpath(r['worktree'], root)}]" if r["worktree"] else "" + print(f"{mark} {r['class']:10} {r['age']:4}d {r['branch']}{where} {r['subject']}{note}") + if not (args.apply and r["safe"]): + continue + try: + if r["worktree"] and os.path.isdir(r["worktree"]): + git(root, "worktree", "remove", r["worktree"]) + # -D: the classification above already proved the work is in the default branch, + # and -d would refuse a branch merged into origin/ but not the local one. + git(root, "branch", "-D", r["branch"]) + removed += 1 + except RuntimeError as e: + print(f" could not remove: {e}") + failed += 1 + + counts = {c: sum(1 for r in rows if r["class"] == c) for c in order} + safe = sum(1 for r in rows if r["safe"]) + print(f"\n{len(rows)} branches: " + ", ".join(f"{n} {c.lower()}" for c, n in counts.items()) + + f"; {safe} safe to remove.") + if args.apply: + print(f"removed {removed}" + (f", {failed} failed" if failed else "") + + ". STALE and ACTIVE branches are the owner's call.") + elif safe: + print("report only: re-run with --apply to remove the safe ones.") + return 1 if failed else 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/CLAUDE.md b/CLAUDE.md index f138499..1c2a266 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -34,11 +34,17 @@ timeline. It is the first of three planned tools; the programme is in `docs/road ## Working agreements - **Never change files on the default branch** (`main`/`master`). Branch first - (`git checkout -b `). `.claude/hooks/guard-default-branch.py` blocks Edit/Write there as - a backstop, but cannot see shell writes (`sed -i`, `>>`). + (`git checkout -b `). `.claude/hooks/guard-default-branch.py` blocks edits, `git commit` + and pushes to it as a backstop, but does not judge other shell writes (`sed -i`, `>>`). - Record decisions a later session could plausibly reopen (direction, architecture, contracts, security, rejected alternatives): one line in the auto-loaded index @docs/decisions.md and the full entry in its `docs/decisions/.md`. The index header sets the format and budget. +- **Size the task first, and say which lane in one line.** A *small change* (a few files, easy to + revert, and no change to a shared contract, security or privacy posture, or product behaviour) + takes the short lane: branch, change, `/checkpoint`, one `reviewer` pass, then a pull request + whose description ends with a `Decision:` line if one was made. No plan, no decisions-record + entry, no closeout. Anything bigger, or a small change that grows, takes the full process. + A `Decision:` line is promoted to the decisions record only if a later session could reopen it. - For large milestones, write the plan to `docs/plans/YYYY-MM-DD-.md` from `docs/plans/_template.md`, so a fresh session can execute it from the file alone. Keep its `Status:` line and the active list in `docs/plans/README.md` current. @@ -131,6 +137,8 @@ Fable. Update the mapping when models change. - Ending a session mid-task, switching topic, or past ~300–400k tokens: run `/handoff` (the `handoff` skill). It records state in the repository and prints a short prompt for the next session. Don't let a conversation be the only place work's state lives. +- Weekly, at a handoff, or when `.claude/worktrees/` piles up: run `/gc` (the `gc` skill) to remove + worktrees and branches whose work is already in the default branch. - **Close out:** the plan's `Status:` line and the active list; the execution log compressed into the completion summary with its lesson or `NO_ENTRY`; decision lines that meet the index bar; a short summary for the owner. diff --git a/docs/plans/_template.md b/docs/plans/_template.md index 1e4b1e4..aaf7c83 100644 --- a/docs/plans/_template.md +++ b/docs/plans/_template.md @@ -161,6 +161,9 @@ implementation detail out of it. - **Deviations from approved plan:** None | - **Known risks / follow-up:** None | - **Decision records updated:** None | `docs/decisions.md` entries +- **Missed by:** n/a | - **Lesson:** NO_ENTRY | cue: / lesson: / evidence: / confidence: