diff --git a/.claude/gh-budget.md b/.claude/gh-budget.md new file mode 100644 index 000000000..8b03d1f49 --- /dev/null +++ b/.claude/gh-budget.md @@ -0,0 +1,174 @@ +# The GitHub API budget: which `gh` command to reach for + +**This is the single source for the rule. Skills and hooks LINK here; they do not +restate it.** An earlier copy of this doctrine lived inside one skill only, and +that skill then violated its own rule a hundred lines above where the rule was +written. Restating is how that happened, so do not restate it. + +## The one fact that drives everything + +GitHub scores two independent budgets, and `gh` routes to them in a way that is +not obvious from the command name. + +| Budget | Size | Scored in | Who spends it | +|---|---|---|---| +| GraphQL | 5000 / hour | **points**, roughly `ceil(nodes/100)` per query | every `gh` porcelain command, and all of Projects V2 | +| REST core | 5000 / hour | requests | `gh api ` | +| REST search | 30 / minute | requests | `gh api search/...` | + +Points are the trap. One query that walks a large connection can cost hundreds +of points, so a handful of careless calls exhausts the hour while the request +COUNT still looks tiny. + +**Every `gh` porcelain command below issues `POST /graphql` and nothing else**, +each verified with `GH_DEBUG=api`: + +Reads: `gh issue view` `gh issue list` `gh pr view` `gh pr list` `gh pr diff` +`gh pr checks` `gh pr status` `gh search issues` `gh label list` + +Writes: `gh issue create` `gh issue edit` `gh issue close` `gh issue comment` + +The reads are essentially the whole read surface an agent uses, so left alone a +session spends its entire GitHub budget on the small expensive pool and never +touches the large cheap one. The writes are lower volume, but a skill whose whole +deliverable IS a write (filing an issue, writing a plan into an issue body, +recording a research note) is blocked outright once the budget is gone, which is +worse than being slow. + +**Do not extend this list by assuming a family routes one way.** `gh label +create` looks like it belongs next to `gh label list` and does not: it issues a +plain `POST /repos/{owner}/{repo}/labels`. Measure with `GH_DEBUG=api` before +adding a command, or you will forbid a working REST call for no benefit. + +## The rule + +**Reserve GraphQL for the two things only GraphQL can do. Everything else is +`gh api` over REST.** + +Only these genuinely require GraphQL: + +1. **Projects V2**, which has no REST API at all (the board, its fields, its + item status). +2. **`resolveReviewThread`**, since REST cannot resolve a review thread. + +## Substitutions + +| Instead of | Use | +|---|---| +| `gh issue view N --json ...` | `gh api repos/{owner}/{repo}/issues/N` | +| `gh issue view N --comments` | `gh api "repos/{owner}/{repo}/issues/N/comments?per_page=100"` | +| `gh issue list --state open` | `gh api "repos/{owner}/{repo}/issues?state=open&per_page=100"` | +| `gh issue list --label X` | `gh api "repos/{owner}/{repo}/issues?labels=X&state=all&per_page=100"` | +| `gh pr view N --json ...` | `gh api repos/{owner}/{repo}/pulls/N` | +| `gh pr list --state merged --head BR` | `gh api "repos/{owner}/{repo}/pulls?state=closed&head={owner}:BR"` then filter `merged_at != null` | +| `gh pr diff N` | `gh api repos/{owner}/{repo}/pulls/N -H "Accept: application/vnd.github.diff"` | +| `gh search issues ...` | `gh api "search/issues?q=..."` | +| `gh label list` | `gh api "repos/{owner}/{repo}/labels?per_page=100"` | +| `gh issue create` | `gh api -X POST repos/{owner}/{repo}/issues --input body.json` | +| `gh issue edit N --body-file F` | `jq -n --rawfile b F '{body:$b}' > p.json` then `gh api -X PATCH repos/{owner}/{repo}/issues/N --input p.json` | +| `gh issue edit N --add-label X` | `gh api -X POST repos/{owner}/{repo}/issues/N/labels -f 'labels[]=X'` | +| `gh issue edit N --add-assignee U` | `gh api -X POST repos/{owner}/{repo}/issues/N/assignees -f 'assignees[]=U'` | +| `gh issue comment N --body-file F` | `jq -n --rawfile b F '{body:$b}' > c.json` then `gh api -X POST repos/{owner}/{repo}/issues/N/comments --input c.json` | +| `gh issue close N --reason completed` | `gh api -X PATCH repos/{owner}/{repo}/issues/N -f state=closed -f state_reason=completed` | + +Build every body payload with `jq -n --rawfile` and send it with `--input`. An +issue body is full of backticks and `$`, and interpolating it into a shell string +runs them as command substitution, which has already silently eaten every code +reference out of a posted review in this repo. + +`{owner}` and `{repo}` expand from the current repository, in the path AND in a +query string, and resolve to nothing when there is no remote, so a call in a +test harness fails closed rather than erroring. + +## Commands that deliberately stay on the porcelain + +`gh pr merge`, `gh pr create`, and `gh pr checks` all route to GraphQL and stay +that way. They run ONCE per PR, so their cost is noise, and each has a specific +reason that outweighs it. Do not "finish the job" by converting them. + +(`gh label create` is on the porcelain too, but for a different reason: it is +already REST underneath, so there is nothing to convert. See the measurement +warning above.) + +**`gh pr merge`.** Two PostToolUse hooks detect a merge by matching the literal +string `gh pr merge` in the command. Rewriting it to `gh api -X PUT .../merge` +silently stops both firing, so merged worktrees leak and the global CLI is never +updated. If it ever has to change, the regexes in +`.claude/hooks/cleanup-merged-worktree.sh` and +`.claude/hooks/release-global-update.sh` change in the same commit. + +**`gh pr create`.** The porcelain resolves the base, head, and repo from local +git state. The REST equivalent needs all three passed explicitly and is easy to +get subtly wrong. + +**`gh pr checks`.** This one is the merge gate, and it merges check-runs AND +legacy commit statuses into a single verdict. A hand-rolled replacement has to +read `commits//check-runs` and `commits//status` and combine them, and +would have to keep doing so correctly forever. The combined-status endpoint also +reports `state: "pending"` when a commit has NO statuses at all, which on this +repo (10 check-runs, 0 statuses, measured) makes the obvious one-call version +report a fully green PR as pending. A cheap call on a path where a miss lets a +red build onto `main` is not worth optimizing. + +## Four traps in the REST replacements + +1. **`GET /repos/{o}/{r}/issues` returns pull requests too.** On this repo that + is 4 PRs mixed into 24 open "issues". Filter with + `select(has("pull_request") | not)` or you will report PRs as issues. +2. **REST paginates at 30 by default.** Pass `per_page=100`, and `--paginate` + where completeness matters. This is the REST-side echo of the + `--limit 20000` trap and truncates just as silently. +3. **A `gh` on PATH may print a banner to STDOUT.** A wrapper that announces + itself before exec'ing the real binary (a mise shim does this) puts that text + inside every `$(gh ...)` capture. In a script, ask for ONE scalar with `--jq` + and take the last line, rather than capturing JSON and parsing it. JSON with + a line in front of it does not parse at all, so this fails loudly in tests + and silently in a hook. + + **The banner is emitted INCONSISTENTLY**, only when the wrapper re-resolves + its tool, so it is present on one call and absent on the next within a single + session. Never strip it by POSITION. `tail -n +2` on multi-line output + silently deletes the first real line whenever the banner did not appear, and + on an issue-body round trip that means the body comes back a line short with + no error anywhere. Strip it by PATTERN (`sed '1{/^mise /d}'`) or, better, + avoid the problem: send bodies with `--input ` and read them back to a + file, so no multi-line payload ever rides a shell capture. +4. **`gh api rate_limit` is free.** It does not consume budget, so a step about + to do heavy board work can check first and degrade or report honestly rather + than failing mid-run. + +## Projects V2, the part that must stay on GraphQL + +The board is the one legitimate GraphQL consumer, so spend its points well. + +**Never dump the whole board to answer a question about ONE issue.** Measured on +2026-08-08, against this board: + +| Call | Cost | +|---|---| +| `gh project item-list 1 --owner webjsdev --limit 20000` | **631 points** | +| the single-issue node lookup below | **1 point** | + +That is 13% of the hourly budget for one dump, and the skills used to call it +two to four times per invocation. Reach the item id from the ISSUE node instead. +It aliases, so a whole batch still costs one request, and the result is stable +enough to cache for the session: + +```sh +gh api graphql -f query='query($n:Int!){repository(owner:"webjsdev",name:"webjs"){ + issue(number:$n){projectItems(first:5){nodes{id project{number} + fieldValueByName(name:"Status"){...on ProjectV2ItemFieldSingleSelectValue{name}}}}}}}' -F n= +``` + +**Never call the full-board dump in a loop.** One whole-board read is legitimate, +in `webjs-list-todos`, which renders every card. Even there, select only the +fields being rendered rather than `fieldValues(first:100)`. + +**Do not re-resolve the static ids.** The project id, the Status field id, and +its option ids never change. They live in `.claude/gh-ids.env`; source that file +instead of spending three GraphQL round trips per run rediscovering constants. +Refresh it with the command written at the top of that file if the board schema +ever changes. + +**Keep polling off GraphQL entirely.** Waiting on background work means repeated +reads, and those belong on the REST issues endpoint. diff --git a/.claude/gh-ids.env b/.claude/gh-ids.env new file mode 100644 index 000000000..e44912a18 --- /dev/null +++ b/.claude/gh-ids.env @@ -0,0 +1,31 @@ +# Static GitHub Projects V2 ids for https://github.com/orgs/webjsdev/projects/1 +# +# These never change. Resolving them costs three GraphQL round trips, and the +# skills used to spend them on every single run rediscovering constants, so they +# are cached here instead. Source this file rather than re-resolving: +# +# source .claude/gh-ids.env +# +# Projects V2 is GraphQL-only and the budget is scored in POINTS, so this is one +# of the few places those points are genuinely unavoidable. Spend them once. +# See .claude/gh-budget.md for the full rule. +# +# REFRESH (only if the board schema changes, which it effectively never does): +# +# gh api graphql -f query='query{organization(login:"webjsdev"){ +# projectV2(number:1){id field(name:"Status"){ +# ...on ProjectV2SingleSelectField{id options{id name}}}}}}' +# +# Verified 2026-08-08. + +PROJECT_ID=PVT_kwDOERfAXc4BZDhV +STATUS_FIELD_ID=PVTSSF_lADOERfAXc4BZDhVzhUE7nE + +# Status options. Only the two the skills actually set are kept here, because an +# id nothing reads is either dead weight or a sign that the skill meant to use it +# is still carrying its own hard-coded copy, which is what this file exists to +# stop. Todo is the default a new card lands in and Done is set by the `Closes +# #N` automation, so no skill sets either. The refresh command above lists every +# option if one is ever needed. +STATUS_READY=ad471dd5 +STATUS_IN_PROGRESS=47fc9ee4 diff --git a/.claude/hooks/cleanup-merged-worktree.sh b/.claude/hooks/cleanup-merged-worktree.sh index ef4b97992..a57c0d167 100755 --- a/.claude/hooks/cleanup-merged-worktree.sh +++ b/.claude/hooks/cleanup-merged-worktree.sh @@ -61,9 +61,25 @@ is_merged() { if git merge-base --is-ancestor "refs/heads/$br" "$base" 2>/dev/null; then return 0; fi # A merged GitHub PR for this head branch (squash merges, which are NOT an # ancestor of base). Network; skipped when gh is absent or unauthenticated. + # + # REST, not `gh pr list`. Every `gh pr *` porcelain command goes through the + # GraphQL API, whose budget is scored in POINTS and which agent sessions here + # routinely exhaust; when it is spent this lookup returns nothing, the branch + # reads as unmerged, and the worktree leaks, which is the exact failure this + # hook exists to prevent. The REST pulls endpoint is a separate budget. + # `{owner}`/`{repo}` expand from the current repo, and resolve to nothing when + # there is no remote (the test harness), so the call fails closed to the + # ancestor check above rather than erroring. + # + # Read the number through `grep -E '^[0-9]+$'` rather than trusting the whole + # capture: a `gh` earlier on PATH may be a wrapper that prints a banner to + # STDOUT before exec'ing the real binary, which would otherwise land inside + # this variable. if command -v gh >/dev/null 2>&1; then local n - n=$(gh pr list --state merged --head "$br" --json number --jq '.[0].number' 2>/dev/null | grep -E '^[0-9]+$' || true) + n=$(gh api "repos/{owner}/{repo}/pulls?state=closed&head={owner}:$br&per_page=100" \ + --jq '[.[] | select(.merged_at != null)] | .[0].number // empty' 2>/dev/null \ + | grep -E '^[0-9]+$' || true) [ -n "$n" ] && return 0 fi return 1 diff --git a/.claude/hooks/release-global-update.sh b/.claude/hooks/release-global-update.sh index 55aded359..0c379cc7e 100755 --- a/.claude/hooks/release-global-update.sh +++ b/.claude/hooks/release-global-update.sh @@ -39,16 +39,23 @@ command -v gh >/dev/null 2>&1 || exit 0 num=$(printf '%s' "$cmd" | grep -oE 'gh pr merge[[:space:]]+#?[0-9]+' | grep -oE '[0-9]+' | head -1) if [ -z "$num" ]; then exit 0; fi -info=$(gh pr view "$num" --json headRefName,title 2>/dev/null || true) -if [ -z "$info" ]; then exit 0; fi -head=$(printf '%s' "$info" | jq -r '.headRefName // ""' 2>/dev/null || true) -title=$(printf '%s' "$info" | jq -r '.title // ""' 2>/dev/null || true) +# REST, not `gh pr view`. Every `gh pr *` porcelain command goes through the +# GraphQL API, whose budget is scored in POINTS and which agent sessions here +# routinely exhaust; when it is spent this fetch returns nothing and the reminder +# silently never fires. The REST pulls endpoint is a separate budget. +# +# Ask for the ONE field this hook reads and take the LAST line, rather than +# capturing JSON and parsing it here. A `gh` earlier on PATH may be a wrapper +# that prints a banner to STDOUT before exec'ing the real binary; prepended to +# JSON that breaks the parse outright, while a scalar survives `tail -n1`. +title=$(gh api "repos/{owner}/{repo}/pulls/$num" --jq '.title // empty' 2>/dev/null | tail -n1) +if [ -z "$title" ]; then exit 0; fi # A real release PR carries the canonical "chore: release " title (the # release process always titles it exactly that). Match the TITLE, not the # branch prefix: a `chore/release-*` branch that is NOT a package release (a hook # tweak, a doc change) would otherwise fire a false reminder with nothing to -# publish. `head` is unused now but kept in the fetch for future signals. +# publish. if ! printf '%s' "$title" | grep -qiE '^chore: release '; then exit 0; fi read -r -d '' MSG <<'EOF' || true diff --git a/.claude/skills/webjs-file-issue/SKILL.md b/.claude/skills/webjs-file-issue/SKILL.md index 18f9bfd54..b20061586 100644 --- a/.claude/skills/webjs-file-issue/SKILL.md +++ b/.claude/skills/webjs-file-issue/SKILL.md @@ -47,23 +47,34 @@ If the user's description is very thin (e.g. "track adding dark mode as a todo") 2. **Create the issue AND assign it to vivek7405.** Every WebJs issue is assigned to the owner (vivek7405) at creation so the project board shows ownership at a glance. - Write the grounded body to a scratch file first, then pass it with - `--body-file`. Do NOT pass it as `--body "..."`: an issue body is full of - backticks and `$` characters, and an unquoted shell string runs them as - command substitution, silently eating whatever they contained. This has - already bitten this repo through a sibling flag: a review posted with - `gh api -f body=` lost every code reference in it and had to be deleted - and reposted. Same mechanism, different flag. + Write the grounded body to a scratch file first, then read it into the + request with `jq -n --rawfile` so it never passes through the shell at all. + Do NOT interpolate it into a shell string: an issue body is full of backticks + and `$` characters, and an unquoted string runs them as command substitution, + silently eating whatever they contained. This has already bitten this repo: + a review posted with `gh api -f body=` lost every code reference in it and + had to be deleted and reposted. ```sh - # write the body to a scratch path first (any disposable location) - gh issue create --repo webjsdev/webjs \ - --title 'dogfood: the router drops the second click' \ - --label bug \ - --assignee vivek7405 \ - --body-file /tmp/issue-body.md + # write the body to a scratch path first (any disposable location), then + # build the request with jq so nothing is interpolated into a shell string + jq -n --rawfile body /tmp/issue-body.md \ + '{title:"dogfood: the router drops the second click", body:$body, + labels:["bug"], assignees:["vivek7405"]}' > /tmp/issue.json + gh api -X POST repos/webjsdev/webjs/issues --input /tmp/issue.json \ + --jq '"\(.number) \(.html_url)"' ``` + REST rather than `gh issue create`, which goes through GraphQL. This is not + theoretical: filing #1339 failed on `gh issue create` with + `API rate limit already exceeded` and went through over REST unchanged. The + skill that files the work must not be the thing a spent budget blocks. See + `.claude/gh-budget.md`. + + Building the payload with `jq -n --rawfile` also settles the quoting problem + the paragraph above describes, since the body never passes through the shell + at all. + A file path is used here rather than a heredoc on purpose. A heredoc whose `EOF` terminator is indented (which it will be, pasted from any nested context like this one) does not terminate: bash swallows the rest of the @@ -130,6 +141,11 @@ For a thin placeholder (user just wants the line item tracked, explicitly deferr ## Failure handling -- If `gh issue create` fails (auth, label missing, network): surface the error and offer to retry with adjusted args. -- If `gh project item-add` fails after the issue was created: report the partial state ("issue #N created but not on board yet") and offer to add it manually. -- If the user's description seems to duplicate an existing open issue: search the board first with `gh project item-list 1 --owner webjsdev --format json --limit 20000` and ask whether to file anyway or use the existing one. +- If the create call fails (auth, label missing, network): surface the error and offer to retry with adjusted args. +- If `gh project item-add` fails after the issue was created: report the partial state ("issue #N created but not on board yet") and offer to add it manually. Projects V2 is GraphQL-only, so this is the one step here a spent GraphQL budget can genuinely block; check `gh api rate_limit` (free) and report the reset time rather than leaving the issue silently off the board. +- If the user's description seems to duplicate an existing open issue: search over REST and ask whether to file anyway or use the existing one. Search the ISSUES, not the board: it matches bodies as well as titles, so it is the better dedupe, and it costs nothing from the GraphQL budget. + + ```sh + gh api "search/issues?q=repo:webjsdev/webjs+is:issue+&per_page=10" \ + --jq '.items[] | "#\(.number) [\(.state)] \(.title)"' + ``` diff --git a/.claude/skills/webjs-list-todos/SKILL.md b/.claude/skills/webjs-list-todos/SKILL.md index 6ab05005b..2b8a63a27 100644 --- a/.claude/skills/webjs-list-todos/SKILL.md +++ b/.claude/skills/webjs-list-todos/SKILL.md @@ -25,6 +25,23 @@ The webjsdev/webjs project tracks work on the GitHub Project at https://github.c gh project item-list 1 --owner webjsdev --format json --limit 20000 ``` + **This is the ONE place a whole-board dump is legitimate**, because this skill + renders every card. Everywhere else, reach a single card from its issue node + instead; see `.claude/gh-budget.md`. + + The dump is expensive even so. It paginates the entire board (past 500 items) + at 100 per page, and the GraphQL budget is scored in points rather than + requests, so it is worth several hundred points of the hourly 5000. Two rules + follow, and neither is optional: + + - **Call it ONCE per invocation and reuse the result.** Never in a loop, and + never a second time to answer a follow-up about one card. + - **Do not re-run it to check one issue.** If the user asks about a specific + number after this ran, use the single-node lookup in `.claude/gh-budget.md`. + + `--limit 20000` is load-bearing, not defensive: the default page is 30, so + without it the board silently truncates and buckets come back wrong. + 2. **Bucket by Status** (Todo, In progress, Done) and pretty-print each bucket with issue numbers and titles. If the user explicitly asked for only one bucket (e.g. "what's in progress"), filter accordingly. 3. **Default presentation.** Show Todo and In progress always. Show Done only if the user asks for it, or if both Todo and In progress are empty. diff --git a/.claude/skills/webjs-ready-for-dev/SKILL.md b/.claude/skills/webjs-ready-for-dev/SKILL.md index bd03de3b2..920b7abc3 100644 --- a/.claude/skills/webjs-ready-for-dev/SKILL.md +++ b/.claude/skills/webjs-ready-for-dev/SKILL.md @@ -44,10 +44,17 @@ Nothing here writes code. The deliverable is the issue body. ### 1. Read the board and the issue bodies ```sh -gh project item-list 1 --owner webjsdev --format json --limit 20000 -gh issue view --repo webjsdev/webjs --comments +# The issue body and its comments, over REST. +gh api "repos/webjsdev/webjs/issues/" --jq '.title, .body' +gh api "repos/webjsdev/webjs/issues//comments?per_page=100" --jq '.[].body' ``` +Read the ISSUES, not the board. This skill is handed the issue numbers it is +readying, so it needs their bodies, and a whole-board dump answers a question +nobody asked at several hundred points of the GraphQL budget. When a card's +Status or item id is genuinely needed, take it from the issue node (step 5), not +from a dump. See `.claude/gh-budget.md` for the rule and the substitution table. + Most WebJs issues already carry a partial `## Implementation plan` written when they were filed. That is a starting point, not a finished plan. The agent's job is to VERIFY it against the current code, complete every decision it left open, @@ -99,8 +106,10 @@ WebJs framework monorepo at , and writing it into the issue body. You are FULLY AUTONOMOUS: never ask a question, settle every open call yourself using industry standard practice and prior art, and state what settled each decision. -ISSUE: # "" (webjsdev/webjs). Start with -`gh issue view <N> --repo webjsdev/webjs --comments`. +ISSUE: #<N> "<title>" (webjsdev/webjs). Start by reading the issue and its +comments over REST, which does not spend the GraphQL budget this repo runs out of: +`gh api repos/webjsdev/webjs/issues/<N> --jq '.title, .body'` then +`gh api "repos/webjsdev/webjs/issues/<N>/comments?per_page=100" --jq '.[].body'`. SPECIFIC GROUND TO COVER <the per-issue block: real paths and line anchors to verify, the decisions that @@ -118,15 +127,19 @@ HARD CONSTRAINTS - Scratch files and repro scripts go ONLY in <scratchpad>. Running a read-only node script or an existing test command to verify a repro is encouraged. - Do NOT open a PR, branch, comment, or new issue, and do not change labels, - assignees, or status. Your ONLY mutation is `gh issue edit`. + assignees, or status. Your ONLY mutation is the issue-body PATCH below. DELIVERABLE Rewrite the ENTIRE issue body so a cold AI agent with zero access to this conversation can implement it end to end with no discovery phase. Write it to -<scratchpad>/issue-<N>-body.md and apply it with: - gh issue edit <N> --repo webjsdev/webjs --body-file <scratchpad>/issue-<N>-body.md -Never pass `--body "..."`, because backticks and `$` get shell-expanded and -silently eat whatever they contained. +<scratchpad>/issue-<N>-body.md and apply it over REST: + jq -n --rawfile b <scratchpad>/issue-<N>-body.md '{body:$b}' > <scratchpad>/patch-<N>.json + gh api -X PATCH repos/webjsdev/webjs/issues/<N> --input <scratchpad>/patch-<N>.json --jq .number +Use exactly that REST call. The porcelain equivalent goes through GraphQL, whose +budget is scored in points and is routinely spent here, and writing the plan into +the issue is this whole job, so it must not be the thing a spent budget blocks. +Build the payload with `jq --rawfile` and never pass the body as a shell string, +because backticks and `$` get expanded and silently eat whatever they contained. BODY SHAPE (exact section order) ## Problem (keep the existing statement's substance; verify every claim and line @@ -167,8 +180,8 @@ PROJECT RULES YOU MUST OBEY IN YOUR OWN PROSE AND ENCODE IN THE PLAN anchors are dated. When finished, report: the decisions you settled, any measurement you took, -anything stale you found in the existing body, and confirmation the -`gh issue edit` applied. +anything stale you found in the existing body, and confirmation the body PATCH +applied. ```` ### 5. Verify each plan and move the card to Ready @@ -188,58 +201,75 @@ Seven means the contract is met. Anything less means the agent has not written yet, or wrote a partial body, and the card stays where it is. Then move the card. The board carries a **Ready** column between Todo and In -progress. These ids are stable, so hard-code them rather than looking them up: - -| Thing | Id | -|---|---| -| Project | `PVT_kwDOERfAXc4BZDhV` | -| Status field | `PVTSSF_lADOERfAXc4BZDhVzhUE7nE` | -| Ready option | `ad471dd5` | +progress. The board ids are stable and live in ONE place, `.claude/gh-ids.env`, +so source them rather than copying them here. A second copy of a constant drifts +exactly the way a second copy of a rule does, which this skill has already +demonstrated once (see the GraphQL budget section below). -The one thing you must fetch is each issue's project-item id. Fetch them ONCE for -the whole batch, in a single aliased query, and cache the result in the scratchpad: +The one id you must fetch is each issue's project-item id. Step 1 below runs +ONCE for the whole batch and caches the result; step 2 runs per card, reading +that cache. **Step 2 must be one shell invocation**, per the note under it. Do +not re-run step 1 per card, which would spend a whole query to re-fetch ids you +already have. ```sh +# 1. Cache the item ids for the batch (one aliased query, one request). gh api graphql -f query=' query { r: repository(owner: "webjsdev", name: "webjs") { i1253: issue(number: 1253) { projectItems(first: 5) { nodes { id project { number } } } } i1264: issue(number: 1264) { projectItems(first: 5) { nodes { id project { number } } } } } -}' | jq -r '.data.r | to_entries[] - | "\(.key|ltrimstr("i"))=\(.value.projectItems.nodes[] | select(.project.number == 1) | .id)"' -``` - -Then each move is one small mutation against the cached id: - -```sh +}' --jq '.data.r | to_entries[] + | "\(.key|ltrimstr("i"))=\(.value.projectItems.nodes[] | select(.project.number == 1) | .id)"' \ + | grep -E '^[0-9]+=PVTI_' > <scratchpad>/item-ids.env + +# 2. Move one card. ITEM comes from the cache above, the rest from gh-ids.env. +source .claude/gh-ids.env # PROJECT_ID, STATUS_FIELD_ID, STATUS_READY +ITEM=$(grep -E "^<N>=" <scratchpad>/item-ids.env | cut -d= -f2) +[ -n "$ITEM" ] || { echo "no item id cached for <N>"; exit 1; } gh project item-edit --id "$ITEM" \ - --project-id PVT_kwDOERfAXc4BZDhV \ - --field-id PVTSSF_lADOERfAXc4BZDhVzhUE7nE \ - --single-select-option-id ad471dd5 + --project-id "$PROJECT_ID" \ + --field-id "$STATUS_FIELD_ID" \ + --single-select-option-id "$STATUS_READY" ``` +**Keep step 2's `source`, `ITEM` lookup, and `item-edit` in the SAME shell +invocation.** Step 1 is separate and runs once for the batch. +Environment variables do not survive between separate tool calls, so a `source` +run as its own step leaves `$PROJECT_ID`, `$STATUS_FIELD_ID`, and `$STATUS_READY` +empty in the next one, and `item-edit` is then called with three empty flags. It +fails rather than corrupting anything, but it fails for a reason nothing in the +output names. The `[ -n "$ITEM" ]` guard catches the same class for the id that +is fetched rather than sourced. `webjs-start-work` step 5 keeps its equivalent in +one block for exactly this reason. + +The query uses `--jq` and a shape filter rather than piping into a separate `jq`. +A `gh` wrapper on PATH may print a banner to stdout, and a downstream `jq` then +fails to parse the whole response and returns NOTHING, silently, so every item +id comes back empty. `--jq` runs inside `gh` on the response itself, and the +`grep` drops any wrapper line that survives. + Move the card ONLY after the body edit is confirmed. A card in Ready is a promise that the plan in the body is implementable, so a card moved on a failed edit is worse than one left in Todo. ### GraphQL budget -The GitHub Projects V2 API is **GraphQL only**, and it rate-limits on a point -budget rather than a request count, so a few careless calls exhaust it for the -session. Two rules keep this skill inside it. +**The rule and the full substitution table live in `.claude/gh-budget.md`. Read +it; do not restate it here.** + +It used to be restated here, and this skill then broke its own rule a hundred +lines above where the rule was written, opening step 1 with the whole-board dump +this section forbids. That is what a second copy does, so there is now one copy +and this is a pointer to it. -**Never call `gh project item-list 1 --owner webjsdev --limit 20000` in a loop.** -That query paginates every item on the board (well past 500 today) to find one -id. Reach the item id from the ISSUE node instead, as above: it is a single node -lookup, it aliases so a whole batch costs one request, and the result is stable -enough to cache for the session. +The two consequences that bind this skill in particular: -**Keep polling off GraphQL entirely.** Waiting on N background agents means -repeated reads, and those belong on the issues REST endpoint -(`gh api repos/webjsdev/webjs/issues/<N>`), which has its own separate budget. -Reserve GraphQL for the two things only it can do: the one batched item-id fetch, -and the per-card status mutation. +- Reach a card's item id from the ISSUE node, never from a board dump. This + skill is handed issue numbers, so it never needs the whole board. +- Keep polling off GraphQL. Waiting on N background agents means repeated reads, + and those go to `gh api repos/webjsdev/webjs/issues/<N>`. ### 6. Report @@ -267,8 +297,8 @@ Concrete failure signs, any one of which means the plan is not ready: - **An agent reports it could not settle a call.** That is a prompt failure, not a user question. Re-run that one agent with the constraint that decides it named explicitly (the invariant, the prior art, or the measurement to take). -- **`gh issue edit` fails.** Usually the body file path or an auth scope. Surface - the error, keep the drafted body in the scratchpad, and retry. +- **The issue-body PATCH fails.** Usually the body file path or an auth scope. + Surface the error, keep the drafted body in the scratchpad, and retry. - **An agent's plan contradicts the issue's existing decision.** Keep the new one only if the agent produced EVIDENCE (code that moved, a measurement, prior art). A preference is not evidence, and re-litigating a settled call wastes the next diff --git a/.claude/skills/webjs-research-record/SKILL.md b/.claude/skills/webjs-research-record/SKILL.md index d7a784703..abb0934d4 100644 --- a/.claude/skills/webjs-research-record/SKILL.md +++ b/.claude/skills/webjs-research-record/SKILL.md @@ -55,31 +55,48 @@ Extract from the user's request: ## Steps +Every `gh issue *` call below is REST (`gh api`), because that porcelain goes +through GraphQL, whose point budget is routinely spent here, and writing the +record IS this skill's whole deliverable, so it must not be the thing a spent +budget blocks. The one porcelain call kept is `gh label create` in step 1, which +is already REST underneath (see the note there). See `.claude/gh-budget.md`. +Build every body payload with `jq -n --rawfile` so backticks and `$` in the prose +never reach the shell. + 1. **Ensure the `research` label exists** (one-time): ```sh - gh label list --repo webjsdev/webjs --search research - # if absent: + gh api "repos/webjsdev/webjs/labels?per_page=100" --jq '[.[].name] | index("research")' + # if that prints null: gh label create research --repo webjsdev/webjs --color 5319e7 \ --description "Research/design/decision record (no code); filter these to read design history" ``` + + The read is REST because `gh label list` goes through GraphQL. The create + stays on the porcelain because `gh label create` does NOT: it issues a plain + `POST /repos/{owner}/{repo}/labels`, so there is nothing to save by rewriting + it and the shorter form is easier to read. 2. **Find or create the issue.** If a backlog `research` issue already exists for this question, use it. Otherwise create one: ```sh - gh issue create --repo webjsdev/webjs --label research \ - --title "research: <question or decision>" --body-file /tmp/research-record.md + jq -n --rawfile b /tmp/research-record.md \ + '{title:"research: <question or decision>", body:$b, labels:["research"]}' > /tmp/rec.json + gh api -X POST repos/webjsdev/webjs/issues --input /tmp/rec.json --jq '"\(.number) \(.html_url)"' ``` When appending to an existing backlog issue, also curate the final conclusion into its body so the answer is readable without the whole thread: ```sh - gh issue edit <n> --repo webjsdev/webjs --body-file /tmp/research-record.md - # confirm the label is present: - gh issue edit <n> --repo webjsdev/webjs --add-label research + jq -n --rawfile b /tmp/research-record.md '{body:$b}' > /tmp/rec-body.json + gh api -X PATCH repos/webjsdev/webjs/issues/<n> --input /tmp/rec-body.json --jq .number + # confirm the label is present (adding one already on the issue is a no-op): + gh api -X POST repos/webjsdev/webjs/issues/<n>/labels -f 'labels[]=research' --jq '[.[].name]|join(",")' ``` 3. **Add deep-dive comments** for the threaded detail: ```sh - gh issue comment <n> --repo webjsdev/webjs --body-file /tmp/deep-dive.md + jq -n --rawfile b /tmp/deep-dive.md '{body:$b}' > /tmp/rec-comment.json + gh api -X POST repos/webjsdev/webjs/issues/<n>/comments --input /tmp/rec-comment.json --jq .html_url ``` 4. **Close the issue as completed** (it is a record, not open work): ```sh - gh issue close <n> --repo webjsdev/webjs --reason completed + gh api -X PATCH repos/webjsdev/webjs/issues/<n> -f state=closed -f state_reason=completed \ + --jq '"\(.state)/\(.state_reason)"' ``` 5. **Report** the issue number and confirm it carries the `research` label. @@ -91,5 +108,11 @@ The actual implementation that the research points to is **separate tracked work ```sh # All research records, filterable by the label: -gh issue list --repo webjsdev/webjs --label research --state all +gh api "repos/webjsdev/webjs/issues?labels=research&state=all&per_page=100" \ + --jq '.[] | select(has("pull_request") | not) | "#\(.number) \(.title)"' ``` + +REST rather than `gh issue list`, which goes through GraphQL. See +`.claude/gh-budget.md` for the rule and the full substitution table. The +`pull_request` filter is not optional: the REST issues endpoint returns PRs +alongside issues. diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index 24fe0d6bd..0e86d9ccb 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -28,10 +28,14 @@ This skill picks up from an EXISTING issue. Before running any step below, confi If you are unsure whether an issue already exists, search before filing: ```sh -gh issue list --repo webjsdev/webjs --search "<keywords>" --state all -gh project item-list 1 --owner webjsdev --format json --limit 20000 +gh api "search/issues?q=repo:webjsdev/webjs+is:issue+<keywords>&per_page=20" \ + --jq '.items[] | "#\(.number) [\(.state)] \(.title)"' ``` +Search the ISSUES over REST rather than dumping the board. It matches bodies as +well as titles, so it is the better duplicate check, and it costs nothing from +the GraphQL budget. See `.claude/gh-budget.md`. + When in doubt, file it. A duplicate is cheap to close; untracked work is the expensive failure. Only once an issue number exists do you continue to Inputs below. ## Inputs @@ -39,29 +43,46 @@ When in doubt, file it. A duplicate is cheap to close; untracked work is the exp The user's request typically names an issue by number (e.g. `#112`) or by description (e.g. "the dist issue"). Resolve the number first: - If the user said `#N` explicitly, use N. -- If they described the issue by topic, run `gh project item-list 1 --owner webjsdev --format json --limit 20000` and match against item titles. If multiple match, ask the user to disambiguate. +- If they described the issue by topic, search the issues over REST and match against titles. If multiple match, ask the user to disambiguate. + + ```sh + gh api "search/issues?q=repo:webjsdev/webjs+is:issue+is:open+<topic>&per_page=20" \ + --jq '.items[] | "#\(.number) \(.title)"' + ``` + + Do not dump the board for this. Searching issues costs nothing from the GraphQL budget and matches bodies too, so it resolves a vague description better than a title scan would. ## Steps 1. **Verify the issue exists and is open. Assign it to vivek7405 if not already.** ```sh - gh issue view <N> --repo webjsdev/webjs --json title,number,state,labels,assignees + gh api repos/webjsdev/webjs/issues/<N> \ + --jq '{number,state,title,labels:[.labels[].name],assignees:[.assignees[].login]}' ``` - If `state` is CLOSED, ask the user whether to reopen it or pick a different one. Otherwise note the title for the branch slug. If `assignees` is empty (an issue filed by drive-by contributor), assign to vivek7405: + REST rather than `gh issue view`, which goes through GraphQL. See `.claude/gh-budget.md`. + + If `state` is CLOSED, ask the user whether to reopen it or pick a different one. Otherwise note the title for the branch slug, and the labels for the branch prefix. If `assignees` is empty (an issue filed by drive-by contributor), assign to vivek7405: ```sh - gh issue edit <N> --repo webjsdev/webjs --add-assignee vivek7405 + gh api -X POST repos/webjsdev/webjs/issues/<N>/assignees -f 'assignees[]=vivek7405' ``` -2. **Confirm the issue is on the project board.** +2. **Confirm the issue is on the project board, and get its item id.** + + Ask the ISSUE, not the board. This is a single node lookup that also returns the id step 5 needs and the card's current Status, so one call replaces both the membership check here and the id hunt later: ```sh - gh project item-list 1 --owner webjsdev --format json --limit 20000 --jq ".items[] | select(.content.number == <N>)" + gh api graphql -f query='query($n:Int!){repository(owner:"webjsdev",name:"webjs"){ + issue(number:$n){projectItems(first:5){nodes{id project{number} + fieldValueByName(name:"Status"){...on ProjectV2ItemFieldSingleSelectValue{name}}}}}}}' \ + -F n=<N> --jq '.data.repository.issue.projectItems.nodes[] | select(.project.number == 1)' ``` - If not present, add it: `gh project item-add 1 --owner webjsdev --url https://github.com/webjsdev/webjs/issues/<N>`. + An empty result means the issue is not on the board. Add it with `gh project item-add 1 --owner webjsdev --url https://github.com/webjsdev/webjs/issues/<N>`, then re-run the lookup to get the new item id. + + **Do NOT dump the whole board to answer this.** That query paginates every item (past 500 today) with nested field values, costing several hundred points of the 5000-point hourly GraphQL budget, to find one id this call returns directly. Projects V2 is GraphQL-only, so these points are the ones genuinely worth protecting. 3. **Fetch, and leave the primary checkout alone.** `git fetch origin`. The task's worktree cuts from `origin/main`, so a dirty or mid-something primary checkout neither blocks starting nor gets "fixed"; it is never edited at all (enforced by `.claude/hooks/require-worktree-for-edits.sh`, which blocks tracked-file edits in a primary checkout). @@ -74,18 +95,18 @@ The user's request typically names an issue by number (e.g. `#112`) or by descri ALL work for the task happens inside that worktree, by absolute path when the session's cwd resets. A fresh worktree has NO `node_modules`; see AGENTS.md for the symlink remedy (#954). Cleanup after merge is automatic (`cleanup-merged-worktree.sh`). After this step, ALSO push after every subsequent commit (`git push` is cheap and is the safety net against losing work). Do not batch multiple commits before pushing. -5. **Move the project card from Todo to In progress.** Resolve the four IDs and call `item-edit`: +5. **Move the project card from Todo to In progress.** The item id came from step 2; the other three ids are constants, so source them instead of rediscovering them: ```sh - N=<issue-number> - PROJECT_ID=$(gh project view 1 --owner webjsdev --format json --jq '.id') - ITEM_ID=$(gh project item-list 1 --owner webjsdev --format json --limit 20000 --jq ".items[] | select(.content.number == $N) | .id") - STATUS_FIELD_ID=$(gh project field-list 1 --owner webjsdev --format json --jq '.fields[] | select(.name == "Status") | .id') - IN_PROGRESS_OPT_ID=$(gh project field-list 1 --owner webjsdev --format json --jq '.fields[] | select(.name == "Status") | .options[] | select(.name == "In progress") | .id') - gh project item-edit --project-id "$PROJECT_ID" --id "$ITEM_ID" --field-id "$STATUS_FIELD_ID" --single-select-option-id "$IN_PROGRESS_OPT_ID" + source .claude/gh-ids.env # PROJECT_ID, STATUS_FIELD_ID, STATUS_IN_PROGRESS + ITEM_ID=<the id step 2 returned> + gh project item-edit --project-id "$PROJECT_ID" --id "$ITEM_ID" \ + --field-id "$STATUS_FIELD_ID" --single-select-option-id "$STATUS_IN_PROGRESS" ``` - The `--limit 20000` on `item-list` is load-bearing, not defensive. The board is well past 200 items and the default page is 30, so without it the `select(.content.number == $N)` filter matches nothing for almost every issue, `ITEM_ID` comes back empty, and `item-edit` fails on an empty `--id`. The same truncation makes step 2 report a card as missing when it is already on the board. + The project id, the Status field id, and its option ids never change, so re-resolving them cost three GraphQL round trips per run for constants. `.claude/gh-ids.env` carries them, with the refresh command in its header for the rare case the board schema moves. + + If `ITEM_ID` is empty, step 2 did not find the card. Go back and add it rather than passing an empty `--id`, which fails. 6. **Open a DRAFT PR immediately, BEFORE writing any code.** This is the single most important ordering rule and it is NOT optional: the PR is opened at the START of the work, not the end. The whole point of the PR is to be the durable, append-only record of the change AS IT HAPPENS: every per-logical-unit commit lands on it, every design-rationale / decision / follow-up context comment is posted to it the moment that discussion happens, and every review round is posted to it. NONE of that is possible if the PR does not exist yet, which is exactly the failure a late `gh pr create` causes. So open it now, empty branch and all (the branch was already pushed in step 4). @@ -409,17 +430,19 @@ The class is real and not small, and what it costs is the same work done later r ### Subagent prompt template +**The template fetches the diff and metadata over REST on purpose.** The porcelain equivalents go through GraphQL, and this template is pasted into EVERY reviewer in every round, so it was the single largest consumer of that budget in this skill. REST is a separate budget and returns the same bytes. Both are reads, so the read-only git constraint in the template is unaffected. See `.claude/gh-budget.md`. + One template serves every reviewer in the cycle: round 1, each delta round, the final whole-diff review, the final review's fix-check, and a refuter. Only the question in its numbered step 5 changes. ``` Review PR #<N> (branch `<branch>`) at https://github.com/webjsdev/webjs/pull/<N> for anything genuinely wrong with it, judged against the project's AGENTS.md and CONVENTIONS.md (root + per-package). -HARD CONSTRAINT, read first: you are running against a repository the main session is actively using, and every worktree of it shares ONE `.git` directory, so a git write here reaches the main session's checkout even from an isolated worktree. You are a READ-ONLY reviewer. Do NOT run any command that changes git branch, HEAD, the index, or the working tree: no `git checkout`, `git switch`, `git reset`, `git restore`, `git stash`, `git pull`, `git fetch` that moves refs, `git merge`, `git rebase`, `git clean`, `git branch -f`, or `git worktree`. Any of these silently corrupts the main session's checkout (it moved HEAD off the branch and looked like lost work, and a stray worktree op once flipped the shared repo's `core.bare` to `true`). You do NOT need to switch branches to review. Use `gh pr diff <N>` and `gh pr view <N>` for the diff and metadata, and read any file at its PR-branch state with `gh api repos/<owner>/<repo>/contents/<path>?ref=<branch> --jq .content | base64 -d`. All of those read from GitHub, so they work whether or not the branch exists locally, which matters because a PR you were asked to review may not be checked out here at all. If the branch does happen to be the one checked out, reading files in place is fine too. The only git you may run is read-only inspection (`git log`, `git show`, `git diff` WITHOUT changing state, `git status`, `git blame`). If you think you need to change git state to do the review, you are wrong; report what you found instead. +HARD CONSTRAINT, read first: you are running against a repository the main session is actively using, and every worktree of it shares ONE `.git` directory, so a git write here reaches the main session's checkout even from an isolated worktree. You are a READ-ONLY reviewer. Do NOT run any command that changes git branch, HEAD, the index, or the working tree: no `git checkout`, `git switch`, `git reset`, `git restore`, `git stash`, `git pull`, `git fetch` that moves refs, `git merge`, `git rebase`, `git clean`, `git branch -f`, or `git worktree`. Any of these silently corrupts the main session's checkout (it moved HEAD off the branch and looked like lost work, and a stray worktree op once flipped the shared repo's `core.bare` to `true`). You do NOT need to switch branches to review. Use `gh api repos/<owner>/<repo>/pulls/<N> -H "Accept: application/vnd.github.diff"` for the diff and `gh api repos/<owner>/<repo>/pulls/<N>` for metadata, and read any file at its PR-branch state with `gh api repos/<owner>/<repo>/contents/<path>?ref=<branch> --jq .content | base64 -d`. All of those read from GitHub, so they work whether or not the branch exists locally, which matters because a PR you were asked to review may not be checked out here at all. If the branch does happen to be the one checked out, reading files in place is fine too. The only git you may run is read-only inspection (`git log`, `git show`, `git diff` WITHOUT changing state, `git status`, `git blame`). If you think you need to change git state to do the review, you are wrong; report what you found instead. You start with no prior context on this PR. Steps: -1. Run `gh pr diff <N> --repo webjsdev/webjs` to see the full diff. -2. Run `gh pr view <N> --repo webjsdev/webjs --json title,body` to see what the author claims it does. +1. Run `gh api repos/webjsdev/webjs/pulls/<N> -H "Accept: application/vnd.github.diff"` to see the full diff. +2. Run `gh api repos/webjsdev/webjs/pulls/<N> --jq '.title, .body'` to see what the author claims it does. 3. Read every file the diff touches in its current state (not just the diff hunks) so you see edits in context. 4. Read root AGENTS.md, the per-package AGENTS.md for each touched package, and CONVENTIONS.md if a scaffolded template was touched. 5. The question for this round is a SCOPE, not a checklist: <Round 1 and the final review: the whole diff. A delta round or a fix-check: the fix commits' diff, and trace their blast radius, grepping every symbol, rule, or concept the fix touches across the whole PR surface and comparing each other occurrence, since a small fix can break something far from its own hunk. A refuter: DISPROVE this claim, <the finding>.> Review it as a whole and report whatever is actually wrong. diff --git a/.gitignore b/.gitignore index c3c1bbe3f..4c7e50fee 100644 --- a/.gitignore +++ b/.gitignore @@ -69,6 +69,8 @@ Thumbs.db # every contributor / agent gets the same PreToolUse rules. .claude/* !.claude/settings.json +!.claude/gh-budget.md +!.claude/gh-ids.env !.claude/hooks/ !.claude/hooks/** !.claude/skills/ diff --git a/AGENTS.md b/AGENTS.md index a96cbe1f4..bde7a9cf0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -75,6 +75,20 @@ Git enforces one-branch-per-worktree, so separate worktrees make the collision i A Skill is model-invoked, so it fires only when the model judges a match. The `.claude/hooks/route-skills.sh` `UserPromptSubmit` hook makes routing deterministic: it keyword-matches each prompt against every skill's documented triggers and injects a directive to invoke the matched skill before other work. Check the available skills and invoke a matching one before starting. The skills themselves are committed under `.claude/skills/` (alongside the hooks), so a fresh clone has both the router and the skills it routes to (no machine-local dependency). Tests in `test/hooks/route-skills.test.mjs`, which also asserts every skill the hook references is committed in-repo. +### Talking to GitHub: `gh api` over REST, never the porcelain + +GitHub scores two independent budgets, and `gh` routes to them in a way the command names hide. **Most `gh` porcelain commands issue `POST /graphql`**, whose 5000/hour budget is scored in **points** rather than requests, so one query walking a large connection can cost hundreds. Sessions here exhaust it routinely, and when it is gone the project board becomes unreachable and two PostToolUse hooks fail silently. The REST budget (`gh api <path>`, 5000 requests/hour) sits idle meanwhile. + +Convert these, which is the whole high-volume surface. Reads: `gh issue view` / `gh issue list` / `gh pr view` / `gh pr list` / `gh pr diff` / `gh pr status` / `gh search issues` / `gh label list`. Writes: `gh issue create` / `gh issue edit` / `gh issue close` / `gh issue comment`. The writes are lower volume, but a task whose whole deliverable is a write is blocked outright once the budget is gone. + +So: **go through `gh api` over REST, and reserve GraphQL for the two things only it can do**, Projects V2 (which has no REST API) and `resolveReviewThread`. + +**Leave these on the porcelain**, even though they do route to GraphQL. `gh pr merge`, because two PostToolUse hooks detect a merge by matching that literal string. `gh pr create`, which resolves base and head from local git state. And `gh pr checks`, which is the merge gate: it folds check-runs and legacy commit statuses into one verdict, and the obvious REST replacement reports a green PR as pending, so a miss there lets a red build onto `main`. All three run once per PR, so converting them buys nothing and risks a lot. + +Do NOT extend either list by assuming a command family routes one way. `gh label create` looks like it belongs beside `gh label list` and does not, since it issues a plain `POST /repos/{owner}/{repo}/labels`, so it needs no conversion. Measure with `GH_DEBUG=api` first. + +The full substitution table, the traps in each replacement, and the reasoning behind every exception live in **`.claude/gh-budget.md`**. That file is the single source; skills and hooks link to it rather than restating it, because an earlier copy inside one skill drifted from the rule it stated. + ### Autonomous mode (sandbox / bypass permissions) When interactive approval is disabled, never block on questions. Auto-decide: cut the task's worktree from `origin/main` (auto-create `<prefix>/<task-slug>` per the label scheme); auto-rebase if the parent moved; auto-merge when ready; **delete** feature/fix branches after merge but **keep** long-lived ones (dev, staging, release/*); auto-generate meaningful commit messages; fix failing tests / convention violations rather than asking. Autonomous mode is MORE disciplined, not less, with the same quality bar. diff --git a/test/hooks/cleanup-merged-worktree.test.mjs b/test/hooks/cleanup-merged-worktree.test.mjs index 2c7949152..439358c0a 100644 --- a/test/hooks/cleanup-merged-worktree.test.mjs +++ b/test/hooks/cleanup-merged-worktree.test.mjs @@ -13,9 +13,9 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; import { execFileSync, spawnSync } from 'node:child_process'; -import { mkdtempSync, writeFileSync, existsSync } from 'node:fs'; +import { mkdtempSync, writeFileSync, existsSync, chmodSync } from 'node:fs'; import { tmpdir } from 'node:os'; -import { join, dirname, resolve } from 'node:path'; +import { join, dirname, resolve, delimiter } from 'node:path'; import { fileURLToPath } from 'node:url'; const HOOK = resolve( @@ -46,14 +46,50 @@ function addWorktree({ git, dir, main }, name, { merged, dirty } = {}) { return path; } +/** + * A fake `gh` on PATH emulating the REST call the hook makes for squash merges, + * `gh api "repos/{owner}/{repo}/pulls?...&head={owner}:<branch>&..." --jq ...`. + * It prints a PR number when `<branch>` is in `mergedBranches`, and nothing + * otherwise. `bannerLine`, when set, is printed to STDOUT first, reproducing a + * PATH wrapper (a mise shim does this locally) that would otherwise land inside + * the hook's `$(gh ...)` capture. + */ +function fakeGhDir(mergedBranches, bannerLine = '') { + const dir = mkdtempSync(join(tmpdir(), 'webjs-wtgh-')); + const gh = join(dir, 'gh'); + writeFileSync( + gh, + [ + '#!/usr/bin/env bash', + bannerLine ? `echo ${JSON.stringify(bannerLine)}` : '', + 'for a in "$@"; do', + ' case "$a" in', + ' *head=*)', + ' br="${a##*:}"; br="${br%%&*}"', + ` for m in ${mergedBranches.map((b) => `'${b}'`).join(' ')}; do`, + ' if [ "$br" = "$m" ]; then echo 4242; exit 0; fi', + ' done ;;', + ' esac', + 'done', + '', + ].join('\n'), + ); + chmodSync(gh, 0o755); + return dir; +} + /** Run the hook with a given command, from a given cwd. Returns {code, out}. */ -function runHook(command, cwd) { +function runHook(command, cwd, { mergedBranches = null, bannerLine = '' } = {}) { + // Default: no stub, so the no-remote temp repo makes gh a harmless no-op + // regardless of host auth, and only the ancestor-of-base signal fires. + const ghDir = mergedBranches ? fakeGhDir(mergedBranches, bannerLine) : null; + const env = { ...process.env, GH_NO_UPDATE_NOTIFIER: '1' }; + if (ghDir) env.PATH = `${ghDir}${delimiter}${process.env.PATH}`; const r = spawnSync('bash', [HOOK], { cwd, input: JSON.stringify({ tool_input: { command } }), encoding: 'utf8', - // Force the no-remote temp repo to make gh a harmless no-op regardless of host auth. - env: { ...process.env, GH_NO_UPDATE_NOTIFIER: '1' }, + env, }); return { code: r.status, out: (r.stdout || '') + (r.stderr || '') }; } @@ -73,6 +109,52 @@ test('removes a merged + clean worktree, keeps dirty and unmerged ones', () => { assert.ok(existsSync(repo.main), 'primary checkout is never removed'); }); +// A squash merge leaves the branch NOT an ancestor of base, so the git signal +// cannot see it and the REST lookup is the only thing that can. This is the path +// that silently stopped working while it went through GraphQL: an exhausted +// point budget returned nothing, every squash-merged branch read as unmerged, +// and its worktree leaked, which is the failure the hook exists to prevent. +test('removes a squash-merged worktree that git alone cannot see as merged', () => { + const repo = makeRepo(); + const squashed = addWorktree(repo, 'feat-squashed', {}); + const unmerged = addWorktree(repo, 'feat-really-unmerged', {}); + + // Neither branch is an ancestor of main; only `feat-squashed` has a merged PR. + const { code } = runHook('gh pr merge 1 --squash', repo.main, { + mergedBranches: ['feat-squashed'], + }); + + assert.equal(code, 0); + assert.ok(!existsSync(squashed), 'a squash-merged branch is detected over REST and removed'); + assert.ok(existsSync(unmerged), 'a branch with no merged PR is still kept'); +}); + +test('squash-merge detection survives a `gh` wrapper that banners to stdout', () => { + const repo = makeRepo(); + const squashed = addWorktree(repo, 'feat-squashed', {}); + + const { code } = runHook('gh pr merge 1 --squash', repo.main, { + mergedBranches: ['feat-squashed'], + bannerLine: 'mise ~/.config/mise/config.toml tools: gh@2.97.0', + }); + + assert.equal(code, 0); + assert.ok(!existsSync(squashed), 'a stdout banner must not hide the PR number'); +}); + +test('a banner with no PR number does not make an unmerged branch look merged', () => { + const repo = makeRepo(); + const unmerged = addWorktree(repo, 'feat-unmerged', {}); + + const { code } = runHook('gh pr merge 1 --squash', repo.main, { + mergedBranches: [], + bannerLine: 'mise ~/.config/mise/config.toml tools: gh@2.97.0', + }); + + assert.equal(code, 0); + assert.ok(existsSync(unmerged), 'banner text must never be read as a PR number'); +}); + test('does nothing on a command that is not `gh pr merge`', () => { const repo = makeRepo(); const clean = addWorktree(repo, 'feat-merged-clean', { merged: true }); diff --git a/test/hooks/gh-budget.test.mjs b/test/hooks/gh-budget.test.mjs new file mode 100644 index 000000000..74974341f --- /dev/null +++ b/test/hooks/gh-budget.test.mjs @@ -0,0 +1,259 @@ +// Guards the GitHub API budget doctrine in `.claude/gh-budget.md`. +// +// GitHub scores GraphQL in POINTS (5000/hour) and REST in requests (5000/hour), +// and every `gh` porcelain READ goes to GraphQL. Sessions here exhausted that +// budget routinely, which left the project board unreachable and made two +// PostToolUse hooks fail silently. The rule is that reads go through +// `gh api` over REST, with GraphQL reserved for Projects V2 and +// `resolveReviewThread`, which is all that genuinely needs it. +// +// Prose is free to NAME a banned command while explaining why not to use it, so +// this scans only fenced code blocks, which is where a command is actually +// being prescribed. + +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { execFileSync } from 'node:child_process'; +import { readFileSync, readdirSync, existsSync } from 'node:fs'; +import { join, dirname, resolve, relative } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '../..'); +const DOCTRINE = join(ROOT, '.claude/gh-budget.md'); +const IDS = join(ROOT, '.claude/gh-ids.env'); + +// Every entry below was checked with `GH_DEBUG=api` and observed to issue +// POST /graphql. Do not add a command here on the assumption that a whole family +// routes one way: `gh label create` looks like it belongs and does NOT, since it +// issues a plain POST /repos/{o}/{r}/labels, so banning it would forbid a +// working REST command for no benefit. + +/** Porcelain reads that spend the GraphQL point budget. Each has a REST form. */ +const BANNED_READS = [ + 'gh issue view', + 'gh issue list', + 'gh pr view', + 'gh pr list', + 'gh pr diff', + 'gh pr status', + 'gh search', + 'gh label list', +]; + +// Mutations that also route to GraphQL. Lower volume than the reads, but a skill +// whose whole deliverable is a write (filing an issue, writing a plan into an +// issue body, recording a research note) is BLOCKED outright when the budget is +// spent, which is the failure this doctrine exists to prevent. `gh project *` is +// absent on purpose: Projects V2 has no REST API at all. +const BANNED_WRITES = [ + 'gh issue create', + 'gh issue edit', + 'gh issue close', + 'gh issue comment', +]; + +const BANNED = [...BANNED_READS, ...BANNED_WRITES]; + +// These route to GraphQL and stay there anyway, for reasons in the doctrine. +// They run once per PR, so the cost is noise against what converting them risks. +const PORCELAIN_EXCEPTIONS = ['gh pr merge', 'gh pr create', 'gh pr checks']; + +/** Board ids belong in .claude/gh-ids.env, never copied into a skill. */ +const ID_LITERALS = [/PVT_[A-Za-z0-9_]+/, /PVTSSF_[A-Za-z0-9_]+/]; + +// `gh pr checks` is a DELIBERATE exception, documented in the doctrine: it is +// the merge gate, it merges check-runs and legacy commit statuses into one +// verdict, and the combined-status endpoint reports "pending" for a commit with +// no statuses, so the obvious one-call replacement calls a green PR pending. +// `gh pr merge` and `gh pr create` are exceptions too, for reasons in the file. + +/** Every fenced code block in a markdown source. */ +function codeBlocks(src) { + return [...src.matchAll(/```[^\n]*\n([\s\S]*?)```/g)].map((m) => m[1]); +} + +function skillFiles() { + const dir = join(ROOT, '.claude/skills'); + return readdirSync(dir, { withFileTypes: true }) + .filter((e) => e.isDirectory() && e.name.startsWith('webjs-')) + .map((e) => join(dir, e.name, 'SKILL.md')) + .filter((p) => existsSync(p)); +} + +function hookFiles() { + const dir = join(ROOT, '.claude/hooks'); + return readdirSync(dir).filter((f) => f.endsWith('.sh')).map((f) => join(dir, f)); +} + +test('the doctrine file exists and is TRACKED by git', () => { + assert.ok(existsSync(DOCTRINE), '.claude/gh-budget.md must exist'); + // `.gitignore` carries a broad `.claude/*` rule, so a new file there is + // ignored unless explicitly negated. Every skill links to this one, so an + // untracked copy means a fresh clone gets dangling references and the rule + // silently reverts to whatever each skill remembers. + const tracked = execFileSync('git', ['ls-files', '--', '.claude/gh-budget.md'], { + cwd: ROOT, + encoding: 'utf8', + }).trim(); + assert.equal(tracked, '.claude/gh-budget.md', 'gh-budget.md must be committed, not gitignored'); +}); + +test('the cached project ids are tracked and define what the skills source', () => { + assert.ok(existsSync(IDS), '.claude/gh-ids.env must exist'); + const tracked = execFileSync('git', ['ls-files', '--', '.claude/gh-ids.env'], { + cwd: ROOT, + encoding: 'utf8', + }).trim(); + assert.equal(tracked, '.claude/gh-ids.env', 'gh-ids.env must be committed, not gitignored'); + + const src = readFileSync(IDS, 'utf8'); + for (const key of ['PROJECT_ID', 'STATUS_FIELD_ID', 'STATUS_IN_PROGRESS']) { + assert.match(src, new RegExp(`^${key}=`, 'm'), `${key} must be defined`); + } +}); + +test('no skill or hook PRESCRIBES a GraphQL porcelain call', () => { + const offenders = []; + + for (const file of skillFiles()) { + const rel = relative(ROOT, file); + for (const block of codeBlocks(readFileSync(file, 'utf8'))) { + for (const banned of BANNED) { + if (block.includes(banned)) offenders.push(`${rel}: ${banned}`); + } + } + } + + for (const file of hookFiles()) { + const rel = relative(ROOT, file); + const src = readFileSync(file, 'utf8'); + for (const line of src.split('\n')) { + if (line.trimStart().startsWith('#')) continue; // a comment may name one + for (const banned of BANNED) { + if (line.includes(banned)) offenders.push(`${rel}: ${banned}`); + } + } + } + + assert.deepEqual( + offenders, + [], + `these spend the GraphQL point budget on calls REST answers for free:\n ${offenders.join('\n ')}`, + ); +}); + +test('no skill hard-codes a board id that .claude/gh-ids.env owns', () => { + // A second copy of a constant drifts the same way a second copy of a rule + // does. This file had both problems: it restated the budget rule and then + // broke it, and it carried its own copy of the project ids. + const offenders = []; + for (const file of skillFiles()) { + const src = readFileSync(file, 'utf8'); + for (const re of ID_LITERALS) { + const hit = src.match(re); + if (hit) offenders.push(`${relative(ROOT, file)}: ${hit[0]}`); + } + } + assert.deepEqual( + offenders, + [], + `source .claude/gh-ids.env instead of copying ids:\n ${offenders.join('\n ')}`, + ); +}); + +test('every id in gh-ids.env has a consumer, and every consumer a definition', () => { + const ids = readFileSync(IDS, 'utf8'); + const defined = [...ids.matchAll(/^([A-Z_]+)=/gm)].map((m) => m[1]); + const skills = skillFiles().map((f) => readFileSync(f, 'utf8')).join('\n'); + + // An id nothing reads is either dead weight or, worse, a signal that the + // skill meant to use it is still carrying its own hard-coded copy. + const unused = defined.filter((k) => !skills.includes(`$${k}`) && !skills.includes(`{${k}}`)); + assert.deepEqual(unused, [], `defined in gh-ids.env but read by no skill: ${unused.join(', ')}`); + + // And nothing may source a name the file does not define. + const used = [...skills.matchAll(/\$(?:\{)?(PROJECT_ID|STATUS_[A-Z_]+)\}?/g)].map((m) => m[1]); + const missing = [...new Set(used)].filter((k) => !defined.includes(k)); + assert.deepEqual(missing, [], `used by a skill but not defined: ${missing.join(', ')}`); +}); + +test('every reference to the doctrine file resolves', () => { + const referrers = [...skillFiles(), ...hookFiles(), join(ROOT, 'AGENTS.md')]; + let found = 0; + for (const file of referrers) { + if (!existsSync(file)) continue; + if (readFileSync(file, 'utf8').includes('.claude/gh-budget.md')) found += 1; + } + assert.ok(found >= 3, `expected several files to link the doctrine, found ${found}`); + assert.ok(existsSync(DOCTRINE), 'and the file they link must exist'); +}); + +test('the doctrine documents everything the guard enforces', () => { + // The guard forbidding more than the doctrine explains is the same drift this + // whole file exists to stop, one level up: an agent blocked on a command would + // be sent to a rule that never mentions it. AGENTS.md matters most here, since + // it is the cross-agent surface and non-Claude tools never read `.claude/`. + const doctrine = readFileSync(DOCTRINE, 'utf8'); + const agents = readFileSync(join(ROOT, 'AGENTS.md'), 'utf8'); + + const undocumented = BANNED.filter((c) => !doctrine.includes(c)); + assert.deepEqual( + undocumented, + [], + `banned by the guard but absent from .claude/gh-budget.md: ${undocumented.join(', ')}`, + ); + + const unannounced = BANNED.filter((c) => !agents.includes(c)); + assert.deepEqual( + unannounced, + [], + `banned by the guard but absent from AGENTS.md: ${unannounced.join(', ')}`, + ); +}); + +test('a shell block that sources gh-ids.env also USES it in the same block', () => { + // Environment variables do not survive between tool calls, so a `source` in + // its own fenced block leaves every id empty in the next one and the command + // runs with blank flags. Keep source and consumer in one block. + const offenders = []; + for (const file of skillFiles()) { + for (const block of codeBlocks(readFileSync(file, 'utf8'))) { + if (!block.includes('source .claude/gh-ids.env')) continue; + if (!/\$\{?(PROJECT_ID|STATUS_[A-Z_]+)/.test(block)) { + offenders.push(relative(ROOT, file)); + } + } + } + assert.deepEqual( + offenders, + [], + `these source gh-ids.env in a block that never reads it:\n ${offenders.join('\n ')}`, + ); +}); + +test('the doctrine names its own exceptions, so they are not "fixed" later', () => { + const src = readFileSync(DOCTRINE, 'utf8'); + for (const cmd of PORCELAIN_EXCEPTIONS) { + assert.ok(src.includes(cmd), `the doctrine must explain why ${cmd} stays on the porcelain`); + } +}); + +test('AGENTS.md never tells an agent to convert a porcelain exception', () => { + // The converse of the assertion above, and the direction that actually broke: + // `gh pr checks` was listed among the commands to convert, on the one surface + // non-Claude agents read, while the doctrine warned that converting it lets a + // red build onto `main`. Both directions have to hold or the two disagree. + const agents = readFileSync(join(ROOT, 'AGENTS.md'), 'utf8'); + const section = agents.slice(agents.indexOf('Talking to GitHub')); + const convert = section.slice(0, section.indexOf('Leave these on the porcelain')); + const wrong = PORCELAIN_EXCEPTIONS.filter((c) => convert.includes(c)); + assert.deepEqual( + wrong, + [], + `AGENTS.md lists these as convert-to-REST, but they are documented exceptions: ${wrong.join(', ')}`, + ); + // And each exception must actually be named as one on that surface. + const kept = agents.slice(agents.indexOf('Leave these on the porcelain')); + const unexplained = PORCELAIN_EXCEPTIONS.filter((c) => !kept.includes(c)); + assert.deepEqual(unexplained, [], `not named as exceptions in AGENTS.md: ${unexplained.join(', ')}`); +}); diff --git a/test/hooks/release-global-update.test.mjs b/test/hooks/release-global-update.test.mjs index 76694efd4..36841fc70 100644 --- a/test/hooks/release-global-update.test.mjs +++ b/test/hooks/release-global-update.test.mjs @@ -5,8 +5,16 @@ // publish lands. A normal PR merge, a non-merge command, and the escape hatch // produce no reminder. It never blocks the tool (always exits 0). // -// `gh pr view` is stubbed with a fake `gh` on PATH so the test is offline and -// deterministic. +// The hook reads the PR title over the REST pulls endpoint (`gh api`), NOT +// `gh pr view`, because every `gh pr *` porcelain command spends the GraphQL +// point budget that agent sessions here exhaust. A fake `gh` on PATH stubs that +// call so the test is offline and deterministic. +// +// The fake also covers a trap the real environment has: a `gh` earlier on PATH +// may be a WRAPPER that prints a banner to stdout before exec'ing the real +// binary (a mise shim does exactly this locally). That banner lands inside any +// `$(gh ...)` capture, so the hook asks for a single scalar and takes the last +// line instead of capturing JSON and parsing it. `bannerLine` exercises that. import { test } from 'node:test'; import assert from 'node:assert/strict'; @@ -21,17 +29,40 @@ const HOOK = resolve( '../../.claude/hooks/release-global-update.sh', ); -/** A fake `gh` on PATH whose `pr view` prints the given headRefName + title. */ -function fakeGhDir(headRefName, title) { +/** + * A fake `gh` on PATH emulating `gh api <endpoint> --jq <expr>`: it reads the + * `--jq` expression and prints the matching scalar, the way the real command + * does. `bannerLine`, when set, is printed to STDOUT first, reproducing a PATH + * wrapper that announces itself before running. + */ +function fakeGhDir(headRefName, title, bannerLine = '') { const dir = mkdtempSync(join(tmpdir(), 'webjs-relhook-')); const gh = join(dir, 'gh'); - writeFileSync(gh, `#!/usr/bin/env bash\necho '${JSON.stringify({ headRefName, title })}'\n`); + writeFileSync( + gh, + [ + '#!/usr/bin/env bash', + bannerLine ? `echo ${JSON.stringify(bannerLine)}` : '', + 'expr=""', + 'prev=""', + 'for a in "$@"; do', + ' if [ "$prev" = "--jq" ]; then expr="$a"; fi', + ' prev="$a"', + 'done', + 'case "$expr" in', + ` *.title*) echo ${JSON.stringify(title)} ;;`, + ` *head.ref*) echo ${JSON.stringify(headRefName)} ;;`, + ' *) ;;', + 'esac', + '', + ].join('\n'), + ); chmodSync(gh, 0o755); return dir; } -function runHook(command, { headRefName = '', title = '', env = {} } = {}) { - const ghDir = fakeGhDir(headRefName, title); +function runHook(command, { headRefName = '', title = '', bannerLine = '', env = {} } = {}) { + const ghDir = fakeGhDir(headRefName, title, bannerLine); try { const r = spawnSync('bash', [HOOK], { input: JSON.stringify({ tool_input: { command } }), @@ -81,6 +112,26 @@ test('does NOTHING for a chore/release-* branch that is not a package release (t assert.doesNotMatch(out, /webjsdev/, 'a chore/release-* branch with a non-release title must not fire'); }); +test('survives a `gh` wrapper that prints a banner to stdout before the payload', () => { + const { code, out } = runHook('gh pr merge 839 --squash', { + headRefName: 'chore/release-2026-07-08b', + title: 'chore: release server 0.8.43', + bannerLine: 'mise ~/.config/mise/config.toml tools: gh@2.97.0', + }); + assert.equal(code, 0); + assert.match(out, /npm update -g webjsdev/, 'a stdout banner must not swallow the title'); +}); + +test('a banner alone, with no title, does not fire the reminder', () => { + const { code, out } = runHook('gh pr merge 839 --squash', { + headRefName: '', + title: '', + bannerLine: 'mise ~/.config/mise/config.toml tools: gh@2.97.0', + }); + assert.equal(code, 0); + assert.doesNotMatch(out, /webjsdev/, 'the banner must never be mistaken for a release title'); +}); + test('does NOTHING for a command that is not `gh pr merge`', () => { const { out } = runHook('git status', { headRefName: 'chore/release-x', title: 'chore: release x' }); assert.doesNotMatch(out, /webjsdev/);