diff --git a/.claude/skills/issue-to-pr/SKILL.md b/.claude/skills/issue-to-pr/SKILL.md index 9d342168..c43c5624 100644 --- a/.claude/skills/issue-to-pr/SKILL.md +++ b/.claude/skills/issue-to-pr/SKILL.md @@ -1,17 +1,16 @@ --- name: issue-to-pr +disable-model-invocation: true description: >- - End-to-end implement a GitHub issue and open a ready pull request. Use this - whenever the user wants you to actually FIX or BUILD something from an issue — - phrasings like "fix issue 123", "implement issue #45", "work on issue 200", - "take issue 88 to a PR", "start a worktree and resolve this issue", or pastes - an issue URL and says "do this". It creates an isolated git worktree off the - latest main, finds the root cause (bug) or scopes the feature, implements the - smallest correct change following repo conventions, verifies it (build/lint/ - tests), runs a Codex adversarial review and addresses the findings, then - commits and opens a ready PR. Do NOT use this for pure triage (use - triage-issue), pure code review of an existing PR (use review-pr-or-mr / - code-review), or when the user only wants analysis without code changes. + End-to-end implement a GitHub issue and open a ready pull request. Invoke + explicitly as `/issue-to-pr ` (this is a side-effecting + workflow — it creates branches and opens PRs — so it does not auto-trigger). + It creates an isolated git worktree off the latest main, reproduces the bug + (or scopes the feature), finds the root cause, implements the smallest correct + change following repo conventions, verifies it (build/lint/tests), runs a Codex + adversarial review and addresses the material findings, then commits and opens + a ready PR. Not for pure triage (use triage-issue), pure review of an existing + PR (use review-pr-or-mr / code-review), or analysis without code changes. --- # Issue → PR @@ -20,31 +19,38 @@ Turn a single GitHub issue into a verified, reviewed, ready-to-merge pull request — autonomously — while staying honest about uncertainty and stopping when a human decision is genuinely required. -The goal is not "produce a diff." It is: **understand the real problem, make the -smallest change that correctly solves it, prove it works, have an adversary try -to break it, and only then ship a PR.** Speed comes from doing these in order, -not from skipping them. +The goal is not "produce a diff." It is: **confirm the real problem exists, +make the smallest change that correctly solves it, prove it works by execution, +have an adversary try to break it, and only then ship a PR.** Speed comes from +doing these in order, not from skipping them. ## Inputs -- An issue number (`123`), a `#123` reference, or a full issue URL. -- The repo is the current git repository; PRs target `origin` (`Silo-Server/silo-server`) and base `main`. - -If no issue is given, ask for one. Do not guess. +- The issue reference is passed as `$ARGUMENTS` — a number (`123`), a `#123` + reference, or a full issue URL. If none is given, ask; do not guess. +- The repo is the current git repository; PRs target `origin` + (`Silo-Server/silo-server`) and base `main`. ## Operating principles -- **Root cause over symptom.** For bugs, reproduce first and find *why* it - happens before touching code. A fix you can't explain is a fix you can't trust. -- **Smallest correct change.** Match surrounding code's idioms, comment density, - and naming. Put code in the package that owns the behavior (see CLAUDE.md); - don't create catch-all helpers or duplicate logic — extract shared logic when - you find yourself copying. -- **Verify before you believe.** Build, lint, and run the relevant tests. Report - failures honestly; never claim something passes that you didn't run. -- **Let the adversary win sometimes.** The Codex adversarial review exists to - break your confidence. Take its material findings seriously and fix them; loop - until it's clean or you can defend why a finding doesn't apply. +- **Default to "maybe nothing needs fixing."** Coding agents "fix" already-correct + or stale code in a large fraction of cases because they don't stop to confirm the + problem is real. Confirm the issue reproduces on current `main` before changing + anything — it may already be fixed. +- **Root cause over symptom.** A fix you can't explain is a fix you can't trust. +- **Verify by execution, not by reading.** Build/lint/tests are the primary + correctness signal. The adversarial review is a *second* layer on top — never a + substitute for green checks. If you can't verify it, don't ship it. +- **Smallest correct change.** Match surrounding code's idioms, naming, and comment + density. Put code in the package that owns the behavior (CLAUDE.md); don't create + catch-all helpers or duplicate logic — search for an existing helper before + writing a new one, and extract shared logic instead of copying. +- **Don't trust recall for APIs.** Verify every symbol, function, and signature you + call actually exists by grepping the codebase — agents hallucinate plausible-looking + APIs. +- **Don't gold-plate.** A reviewer told to find gaps will always find some. Address + what affects correctness or the issue's stated requirements; treat the rest as + optional. Extra abstraction "just in case" is a defect, not diligence. - **Know when to stop.** See [Stop and ask](#stop-and-ask). Autonomy is for execution, not for inventing product decisions or shipping unverifiable guesses. @@ -61,106 +67,119 @@ Read everything before writing anything. gh issue view --json number,title,body,labels,state,author,url,comments ``` -Determine: -- **Bug vs. feature** — from labels (`bug`, `enhancement`, `feature`…) and the - body. This shapes branch prefix (`fix/` vs `feat/`) and commit type. -- **Acceptance criteria** — what does "done" mean? If the issue is vague, - underspecified, or needs a product call, stop and ask (see [Stop and ask](#stop-and-ask)). -- **Linked scope** — does the issue reference an epic / sub-issue (e.g. "Part of - #NNN")? You'll link it in the PR. Per CLAUDE.md, PRs should link the capability - epic or sub-issue they serve. -- **Client surface** — does this touch API contracts, auth, playback, session, - library, or metadata behavior consumed by `silo-android` / `silo-apple`? If so, - note that coordinated client follow-up may be needed (call it out in the PR). +Determine: **bug vs. feature** (labels + body → branch prefix `fix/` vs `feat/`); +**acceptance criteria** (what does "done" mean?); **linked scope** (does it serve +an epic / sub-issue to reference as `Part of #NNN`?); and **client surface** (does +it touch API/auth/playback/session/library/metadata behavior consumed by +`silo-android` / `silo-apple`? — if so, coordinated client follow-up may be needed). + +If the issue is vague, underspecified, or needs a product call, stop and ask now — +before spending a worktree on the wrong problem. ### 2. Create a worktree off the latest main -Work in isolation so the user's current checkout is untouched. Create the -worktree at the **main repository root**, not inside the current worktree. +Work in isolation so the user's checkout is untouched. Create it at the **main +repository root**, not inside the current worktree. ```bash # Resolve the primary repo root (works from inside any worktree) MAIN_ROOT="$(cd "$(dirname "$(git rev-parse --git-common-dir)")" && pwd)" +git -C "$MAIN_ROOT" fetch origin # always branch off fresh main -# Always branch off the freshly fetched main -git -C "$MAIN_ROOT" fetch origin - -SLUG= # e.g. preserve-metadata-on-toggle -BRANCH=fix/issue--$SLUG # use feat/ for features +SLUG= +BRANCH=fix/issue--$SLUG # use feat/ for features WT="$MAIN_ROOT/.claude/worktrees/issue-" git -C "$MAIN_ROOT" worktree add -b "$BRANCH" "$WT" origin/main cd "$WT" ``` -If the branch or worktree already exists from a previous run, reuse it (cd in) -rather than failing — but make sure it's based on current `origin/main`; if it's -stale, tell the user before continuing. +If the branch/worktree already exists from a prior run, reuse it (cd in) rather +than failing — but confirm it's based on current `origin/main`; if it's stale, tell +the user before continuing. From here, **all work happens in `$WT`** (the Bash tool +keeps the working directory between calls); use absolute paths when in doubt. -From here on, **all work happens in `$WT`** (the Bash tool keeps the working -directory between calls). Use absolute paths when in doubt. +### 3. Reproduce / confirm — a hard gate -### 3. Investigate and plan +This is the highest-leverage step. Do not skip it. -- **Bug:** reproduce it (a failing test, a script, or a precise trace through the - code). Pin down the exact faulty code path and the invariant it violates. - Prefer adding/adjusting a test that fails before the fix and passes after. -- **Feature:** map where it fits — the owning package, existing patterns to - follow, the data flow, and the smallest set of files to change. For anything - architectural or multi-file, write a brief plan and confirm direction before a - large build-out. +- **Bug:** reproduce it concretely — ideally a **test that fails on current `main`** + (red), or a precise script/trace through the faulty path. Pin down the exact code + path and the invariant it violates. + - **If you cannot reproduce it, STOP and report** — do not patch. The issue may + already be fixed, be environment-specific, or be a misunderstanding. Shipping a + speculative fix for a non-reproducing bug is the most common way agents make + things worse. +- **Feature:** confirm the capability is actually missing, then map where it fits — + the owning package, patterns to follow, the data flow, the smallest set of files. + +**Plan proportionally.** If you could describe the diff in one sentence (a typo, a +log line, a one-line guard), skip formal planning and make the change. Plan first +when the work is multi-file, unfamiliar, or architecturally uncertain — name the +files/interfaces involved, state what's out of scope, and end the plan with the +verification step. For anything large, confirm direction with the user before a big +build-out. ### 4. Implement -Make the change. Keep it scoped to this one concern (one concern per PR). Respect -the hard rules: +Make the change, scoped to this one concern (one concern per PR). Respect the hard +rules: - **API:** additive-only within `/api/v1` — never rename/remove a response field, change a field's type, or repurpose a status code. New behavior = new fields/endpoints; expose capability endpoints for feature detection. -- **Migrations:** new DB changes are Goose SQL migrations created with +- **Migrations:** new DB changes are Goose SQL migrations via `make migrate-create NAME=...` (timestamped). Never `goose fix`, never paired - `.up.sql`/`.down.sql`. See CLAUDE.md. -- **Maintainability:** extract shared logic instead of duplicating; change - existing code rather than bolting on local workarounds. + `.up.sql`/`.down.sql`. +- **Maintainability:** extract shared logic instead of duplicating; change existing + code rather than bolting on local workarounds. -### 5. Verify +For a bug, the failing test from step 3 should now pass. -Run the relevant checks and make them pass. The exact commands (and the worktree -build quirks — `GOWORK=off`, stubbing `web/dist`, the v2 golangci invocation, the -flaky GPU tests) are in **[references/verification.md](references/verification.md)** — read it before running anything, -because plain `go build ./...` / `make lint` fail inside `.claude/worktrees`. +### 5. Verify — the primary correctness gate -At minimum, before moving on: -- Go: build + the relevant package tests pass. -- Frontend (if `web/` changed): `pnpm run lint`, `pnpm run format:check`, build. -- `make verify-local-paths` is clean. +Run the checks and make them genuinely pass. The exact commands and the worktree +quirks (`GOWORK=off`, stubbing `web/dist`, the v2 golangci invocation, the flaky +GPU tests) are in **[references/verification.md](references/verification.md)** — +read it first, because plain `go build ./...` / `make lint` fail inside +`.claude/worktrees`. -If you can't make checks pass, stop and report — don't paper over it. +At minimum: Go build + the relevant package tests pass; if `web/` changed, +`pnpm run lint` + `pnpm run format:check` + build pass; `make verify-local-paths` is +clean. Run the **full relevant suite**, not just your new test — catch regressions +in code you didn't touch. Show the command output as evidence; don't assert success. + +**Never reach green by weakening the checks.** Deleting/skipping/`xfail`-ing tests, +loosening assertions, lowering coverage thresholds, or relaxing lint/CI to make a +red check pass is a blocker, full stop — if a test genuinely must change, that needs +explicit human sign-off, not a quiet edit. If you can't make the checks pass on the +merits, stop and report. ### 6. Adversarial review (Codex) -Commit your work first (so the review sees exactly what will ship), then run a -Codex adversarial review of the branch against `main`. Full invocation, -output handling, looping, and fallback are in +Commit your work first (so the review sees exactly what will ship), then run a Codex +adversarial review of the branch against `main`. Full invocation, output handling, +the iteration cap, and the fallback are in **[references/codex-review.md](references/codex-review.md)** — follow it exactly. -The loop in short: +The loop, in short: 1. Commit the implementation on the branch. 2. Run the adversarial review (`--base main`, foreground/`--wait`, extended timeout). -3. For each **material** finding, either fix it (then re-run the review) or write - down a defensible reason it doesn't apply. -4. Repeat until the review returns `approve` / no material findings, or you've - resolved/dispositioned everything and can defend the result. +3. For each finding: **first confirm it's real and grounded in the actual code** + (reviewers over-flag — don't fix phantom nits). Then **fix** material findings + that affect correctness or the stated requirements (and re-run the review), or + **rebut** them with a defensible, written reason. Note non-material findings as + optional. +4. **Cap the loop at 2 iterations (3 absolute max).** Debugging effectiveness decays + sharply after ~2–3 attempts, so thrashing won't help — if material findings still + stand after the cap, stop and escalate to the user instead of looping. Do not open the PR while a material, un-rebutted adversarial finding stands. ### 7. Commit, push, open the PR -Commits use Conventional Commit subjects scoped to the domain, e.g. -`fix(playback): guard against nil session on reconnect` or -`feat(catalog): add per-library scan toggle`. End commit messages with the -trailer: +Conventional Commit subjects scoped to the domain, e.g. +`fix(playback): guard against nil session on reconnect`. End commit messages with: ``` Co-Authored-By: Claude Opus 4.8 @@ -172,19 +191,19 @@ Push and open a **ready** (non-draft) PR against `main`: git push -u origin "$BRANCH" gh pr create --base main --head "$BRANCH" --title "" --body "$(cat <<'EOF' ## Problem - + ## Approach ## Verification - + ## Adversarial review - + ## Risks & follow-up - Closes # @@ -203,34 +222,43 @@ End the PR body with: ``` For **UI changes**, attach before/after screenshots or a recording (the repo -requires this) — capture them or, if you can't, explicitly flag in the PR that -they're still needed. +requires this); if you can't capture them, explicitly flag in the PR that they're +still needed. ### 8. Report back -Give the user the PR URL, a 2–3 line summary of the change, the verification -result, the adversarial-review outcome, and any flagged risks or required -client-side follow-up. Mention the worktree path so they can inspect it. +Give the user the PR URL, a 2–3 line summary, the verification result, the +adversarial-review outcome, any flagged risks or required client follow-up, and the +worktree path so they can inspect it. + +## Long or multi-iteration runs + +Performance degrades on long sessions, and context compaction can silently drop +constraints. So: keep a short progress note and a "failed approaches" list in the +scratchpad so you don't repeat dead ends; and after any compaction, re-read CLAUDE.md +and re-assert the hard guardrails (additive-only API, migration rules, no weakening +of tests) before continuing. ## Stop and ask Pause and ask the user (don't push a PR) when: -- The issue is ambiguous, underspecified, or requires a product/UX decision. -- The "fix" would require an API contract change that can't stay additive, or a - risky/irreversible migration. +- The issue is ambiguous, underspecified, or needs a product/UX decision. +- **The bug doesn't reproduce** on current `main`. +- The fix would require a non-additive API change, or a risky/irreversible migration. - The change balloons into something large or architectural — propose a plan first. -- Checks can't be made to pass, or the adversarial review keeps surfacing material - findings you can't resolve or rebut. -- The right fix clearly belongs in a sibling repo (`silo-android`, `silo-apple`, - a plugin/SDK repo) rather than here — see CLAUDE.md's multi-repo guidance. +- Checks can't be made to pass on the merits, or material adversarial findings remain + after the iteration cap. +- A test/CI change would be needed to "pass" — get sign-off, don't do it silently. +- The right fix clearly belongs in a sibling repo (`silo-android`, `silo-apple`, a + plugin/SDK repo) — see CLAUDE.md's multi-repo guidance. In these cases, summarize what you found and what you'd do, and let the user decide. ## Notes -- This skill is authorized to open ready PRs autonomously once checks pass and the - adversarial review is clean — that's the durable instruction from setup. It is - not authorized to merge, force-push shared branches, or close issues by hand. -- Never invent a fix you can't verify or explain. An honest "here's what's - uncertain" beats a confident wrong PR. +- Authorized to open ready PRs autonomously once checks pass and the adversarial + review is clean — that's the durable instruction from setup. Not authorized to + merge, force-push shared branches, or close issues by hand. +- Never invent a fix you can't verify or explain. An honest "here's what's uncertain" + beats a confident wrong PR. diff --git a/.claude/skills/issue-to-pr/references/codex-review.md b/.claude/skills/issue-to-pr/references/codex-review.md index 017a0e6d..9eeda831 100644 --- a/.claude/skills/issue-to-pr/references/codex-review.md +++ b/.claude/skills/issue-to-pr/references/codex-review.md @@ -54,21 +54,32 @@ The review returns a verdict plus findings: - **`needs-attention`** → there is at least one material risk. Each finding names a file, a line range, an impact, and a concrete recommendation. -For every material finding, do one of two things — never ignore it: +LLM reviewers systematically **over-flag** — they report gaps even when the work is +sound. So for every finding, in order: -1. **Fix it**, then re-run the review (go back to step 2 after committing the fix), or -2. **Rebut it** with a defensible, written reason it doesn't apply to this change - (and capture that reasoning in the PR's "Adversarial review" section). +1. **Confirm it's real.** Open the cited file/lines and verify the finding is + grounded in the actual code and actually reachable. Drop findings that reference + code that doesn't exist, lines outside the diff, or behavior the code doesn't have + — don't fix phantom nits. +2. For a confirmed finding, do one of two things — never silently ignore it: + - **Fix it** if it affects correctness or the issue's stated requirements, then + re-run the review (go back to step 2 after committing the fix); or + - **Rebut it** with a defensible, written reason it doesn't apply, and capture + that reasoning in the PR's "Adversarial review" section. +3. Treat style/naming/speculative/low-value findings as **optional** — don't + gold-plate. The adversarial prompt is told not to emit these, but if any slip + through, note and move on; don't add abstraction or defensive code for cases that + can't happen. -Style/naming/low-value nits are out of scope for this gate; the adversarial -prompt is instructed not to emit them, but if any slip through, don't block on them. - -## 4. Loop until clean +## 4. Loop — but cap it Repeat implement → verify → commit → review until the review is `approve` or every -material finding is fixed or defensibly rebutted. Only then open the PR. If the -review keeps surfacing material findings you can't resolve, stop and bring it to -the user rather than shipping over a standing objection. +material finding is fixed or defensibly rebutted. + +**Cap the loop at 2 iterations (3 absolute max).** Debugging effectiveness decays +sharply after ~2–3 attempts — if material findings still stand after the cap, stop +and escalate to the user with a summary rather than thrashing or shipping over a +standing objection. Only open the PR once the review is clean within the cap. ## 5. Record it in the PR