diff --git a/.antigravity/skills/code-refinement/SKILL.md b/.antigravity/skills/code-refinement/SKILL.md index c1c3882..bcb5593 100644 --- a/.antigravity/skills/code-refinement/SKILL.md +++ b/.antigravity/skills/code-refinement/SKILL.md @@ -1,6 +1,6 @@ --- name: code-refinement -description: "Review staged files for code quality (KISS, DRY, YAGNI, Clean Code) and fix linting issues. Use to clean up staged work before review or commit — unlike code-review, this one edits the code: it applies refactors, runs the linter, and fills test gaps." +description: "Review staged files for code quality (KISS, DRY, YAGNI, Clean Code) and fix linting issues. Use to clean up staged work before review or commit. Unlike code-review, this one edits the code: it applies refactors, runs the linter, and fills test gaps." --- # Code Refinement diff --git a/.antigravity/skills/code-review/SKILL.md b/.antigravity/skills/code-review/SKILL.md index 2c62780..4c0d5d9 100644 --- a/.antigravity/skills/code-review/SKILL.md +++ b/.antigravity/skills/code-review/SKILL.md @@ -5,30 +5,11 @@ description: "Review staged changes for security, correctness, performance, and # Role -You are a senior code reviewer and security expert. -You only read and analyze the code — you must never modify any source code files in the repository. -The sole exception is writing your review output into a Markdown file. -You never ask the user what to do next and you produce exactly one review report per run. - -## Output Location - -- Always write your complete review to a file named `agent-code-review.md` in the project root. -- Overwrite the file completely on each run — do not append. -- This file is the only file you may create or modify. -- Do not stage, commit, or push this file. - -## Iterative Review Behavior - -- On each run, treat the task as a fresh review of the currently staged changes. -- Continue reviewing until there are no High or Medium severity issues and no Low severity blockers, then clearly state in the Summary that the code is good to go. +You are a senior code reviewer and security expert. Read and analyze only; never modify a source file. `agent-code-review.md` in the project root is the single file you may write, overwritten completely each run. Do not stage, commit, or push it. Never ask the user what to do next, and produce exactly one report per run. ## Scope and Inputs -- Review only files that are currently staged in Git, not the entire repository. -- Focus on changed lines and minimal necessary surrounding context. -- If information is missing, state reasonable assumptions and proceed. - -## How to Collect Context +Each run is a fresh review of the currently staged files, not the whole repository. Focus on changed lines and the minimum surrounding context. If information is missing, state a reasonable assumption and proceed. - `git diff --staged --unified=0 --no-color` is the primary input; pull `-U3` when a finding needs surrounding context. - Cite line numbers from the `+` side of each hunk so they match the post-merge file. @@ -37,16 +18,7 @@ You never ask the user what to do next and you produce exactly one review report ## Review Policy -Prioritize findings that materially improve: - -- Security, reliability, data integrity, privacy. -- Correctness and performance where clearly impactful. -- Clarity and Clean Code. - -Avoid nitpicks: - -- Do not flag purely stylistic issues unless a project style rule is clearly violated. -- Recommend formatting or lint rules only when they prevent bugs or confusion. +Prioritize what materially improves security, reliability, data integrity, and privacy; correctness and performance where clearly impactful; and clarity. Do not flag purely stylistic issues unless a project style rule is clearly violated, and recommend formatting or lint rules only when they prevent bugs or confusion. ## Severity Definitions @@ -54,20 +26,13 @@ Avoid nitpicks: - **Medium:** likely bugs, race conditions, significant performance or maintainability problems. - **Low:** clarity, naming, minor cleanup. A Low finding is a blocker only when it violates an explicit project rule (lint configuration or a documented convention); otherwise it never blocks the verdict. -## Security Checklist - -- Map each security finding to OWASP Top Ten, e.g., A01 Broken Access Control, A02 Cryptographic Failures, A03 Injection, etc. -- For HTTP APIs, also consider OWASP API Top 10. -- Provide actionable mitigations. +Review until no High or Medium issues and no Low blockers remain, then record the verdict in the Summary. -## Clean Code and Clarity Checks +## What to Check -- Prefer small, focused functions, clear names, elimination of duplication, obvious control flow. -- Suggest local refactors near changed lines. -- Provide minimal viable patches as examples when safe. -- Identify dead code (unused variables, functions, imports, classes). -- Check for DRY violations (repeated logic or patterns that could be abstracted). -- Check for YAGNI violations (unnecessary code, abstractions, or parameters that add complexity without current value). +- **Security:** map each finding to OWASP Top Ten (A01 Broken Access Control, A02 Cryptographic Failures, A03 Injection, etc.), plus OWASP API Top 10 for HTTP APIs. Provide actionable mitigations. +- **Clean code:** small focused functions, clear names, obvious control flow. Suggest local refactors near changed lines, with minimal viable patches as examples when safe. +- **Dead code, DRY, YAGNI:** unused variables, functions, imports, or classes; repeated logic worth abstracting; abstractions or parameters that add complexity without current value. ## Output Format @@ -84,7 +49,7 @@ Write the following structure into `agent-code-review.md`. `N` is the review ite - One paragraph on overall risk and clarity. - Finding counts: High X, Medium Y, Low Z. -- If no High or Medium remain and no Low blockers, state: **Verdict: good to go**. +- If no High or Medium remain and no Low blockers, state: **Verdict: good to go**. Automation depends on detecting this exact string. ## Findings diff --git a/.antigravity/skills/commitmsg/SKILL.md b/.antigravity/skills/commitmsg/SKILL.md index e40f81a..b8938db 100644 --- a/.antigravity/skills/commitmsg/SKILL.md +++ b/.antigravity/skills/commitmsg/SKILL.md @@ -1,6 +1,6 @@ --- name: commitmsg -description: "Propose a single git commit message for the currently staged changes. Use when asked to write, draft, or suggest a commit message, or to check that a message matches the repo's conventions. Proposes the message only — it does not commit." +description: "Propose a single git commit message for the currently staged changes. Use when asked to write, draft, or suggest a commit message, or to check that a message matches the repo's conventions. Proposes the message only; it does not commit." --- # Commit Message @@ -9,10 +9,10 @@ description: "Propose a single git commit message for the currently staged chang Run these commands to understand the changes: -- `git diff --staged --stat` — the shape of the change; then `git diff --staged` (or per-file diffs) only for files whose purpose isn't clear from the stat -- `git status -s` — staged file list -- `git log -n 20 --oneline` — recent style and to avoid repetition -- `git branch --show-current` — if it contains a ticket ID (e.g. ABC-123), prefix the subject line +- `git diff --staged --stat`: the shape of the change; then `git diff --staged` (or per-file diffs) only for files whose purpose isn't clear from the stat +- `git status -s`: staged file list +- `git log -n 20 --oneline`: recent style and to avoid repetition +- `git branch --show-current`: if it contains a ticket ID (e.g. ABC-123), prefix the subject line ## Rules diff --git a/.antigravity/skills/dependency-review/SKILL.md b/.antigravity/skills/dependency-review/SKILL.md index 42afe01..3254de4 100644 --- a/.antigravity/skills/dependency-review/SKILL.md +++ b/.antigravity/skills/dependency-review/SKILL.md @@ -1,36 +1,30 @@ --- name: dependency-review -description: "Audit package dependency updates for supply-chain risk: publish-age gate, changelog/diff verification, security advisories, community signals, and breaking changes. Use whenever a branch, PR, or working directory changes a dependency manifest or lockfile — package.json, requirements.txt, pyproject.toml, Gemfile, go.mod, Cargo.toml, pom.xml, build.gradle, composer.json, pubspec.yaml, or the lockfile beside them — including Dependabot/Renovate batches and newly added packages." +description: "Audit package dependency updates for supply-chain risk: publish-age gate, changelog/diff verification, security advisories, community signals, and breaking changes. Use whenever a branch, PR, or working directory changes a dependency manifest or lockfile (package.json, requirements.txt, pyproject.toml, Gemfile, go.mod, Cargo.toml, pom.xml, build.gradle, composer.json, pubspec.yaml, or the lockfile beside them), including Dependabot/Renovate batches and newly added packages." --- # Package Update Supply Chain Review -Review dependency updates to catch supply chain attacks, breaking changes, and risky packages before they land in your codebase. - ## Review Workflow -For each updated or newly added package, work through all five checks below. Prefer CLI and API lookups (`npm view`, `pip index`, `gh api`, `curl` against registry/OSV endpoints) over web browsing — they are faster, cheaper, and available in more environments. Never invent results for a check you could not actually perform: report it as **SKIPPED** with the reason. Present findings in a single summary report at the end, grouped by package. Flag any failing check as a **HOLD** and recommend the team investigate before merging. +For each updated or newly added package, work through all five checks below. Prefer CLI and API lookups (`npm view`, `pip index`, `gh api`, `curl` against registry/OSV endpoints) over web browsing. Never invent a result for a check you could not actually perform: report it as **SKIPPED** with the reason. Present findings in a single summary report at the end, grouped by package, and flag any failing check as a **HOLD** to investigate before merging. ### 1. Publication Age Gate -**Goal:** Confirm the release is at least 7 days old. Compromised or typosquatted releases are usually caught within the first few days, so letting a release "bake" gives the community and automated scanners time to notice. - -**Steps:** +Confirm the release is at least 7 days old. Compromised and typosquatted releases are usually caught within the first few days, so letting one bake gives scanners and the community time to notice. -1. Look up the publish date for the exact version on its registry — e.g. `npm view time --json`, `curl https://pypi.org/pypi///json`, `gem info --remote`, or the registry's web page. -2. If fewer than 7 days have elapsed, flag this as **HOLD - TOO NEW** and include the publish date, the age in days, and a recommendation to wait or pin to the prior version. +1. Look up the publish date for the exact version (`npm view time --json`, `curl https://pypi.org/pypi///json`, or the registry's page). +2. Under 7 days → **HOLD - TOO NEW**, with the publish date, the age in days, and a recommendation to wait or pin to the prior version. ### 2. Changelog and Diff Verification -**Goal:** Confirm the code changes match what the release notes claim. - -**Steps:** +Confirm the code changes match what the release notes claim. -1. Locate the changelog, release notes, or GitHub releases page for the new version (e.g. `gh release view --repo /`). -2. Identify the claimed changes (bug fixes, features, refactors, etc.). -3. Skim the actual source diff between the old and new version (e.g. `gh api repos///compare/v1.2.3...v1.4.0` or the repo's compare view). -4. Look for discrepancies: Are there unexpected new files? New network calls? Obfuscated code? Post-install scripts that were not present before? -5. Pay special attention to install hooks (`preinstall`, `postinstall` in npm; `setup.py` entry points in Python; `build.rs` changes in Rust; etc.) since these execute automatically and are a top vector for supply chain attacks. +1. Locate the changelog or releases page for the new version (`gh release view --repo /`). +2. Identify the claimed changes. +3. Skim the source diff between the old and new version (`gh api repos///compare/v1.2.3...v1.4.0`). +4. Look for discrepancies: unexpected new files, new network calls, obfuscated code, post-install scripts that were not there before. +5. Pay special attention to install hooks (`preinstall`/`postinstall` in npm, `setup.py` entry points in Python, `build.rs` in Rust); they execute automatically and are a top supply chain vector. **Red flags to call out:** @@ -38,53 +32,38 @@ For each updated or newly added package, work through all five checks below. Pre - New outbound HTTP/DNS calls, especially to IP addresses or unusual domains - Environment variable reads for tokens, keys, or credentials - New native/binary dependencies or compiled assets -- Changes to CI config or build scripts that fetch remote resources +- CI config or build script changes that fetch remote resources ### 3. Security Advisory Review -**Goal:** Check whether the package or specific version has known vulnerabilities. - -**Steps:** - -1. Query advisory sources for the package name and version range: `gh api /advisories --method GET -f "affects="`, the OSV API (`curl -s https://api.osv.dev/v1/query -d '{"package":{"name":"","ecosystem":""}}'`), or ecosystem tools (`npm audit`, `pip-audit`, `cargo audit`). -2. Check whether the update itself is a security patch. If so, note the CVE(s) it addresses and confirm the fix is present in the version being adopted. -3. Check whether the new version introduces any new advisories. This can happen when a patch also pulls in a vulnerable transitive dependency. -4. Report findings as: **No known advisories**, **Fixes CVE-XXXX-YYYY (severity)**, or **HOLD - OPEN ADVISORY: CVE-XXXX-YYYY**. +1. Query advisories for the package name and version range: `gh api /advisories --method GET -f "affects="`, the OSV API (`curl -s https://api.osv.dev/v1/query -d '{"package":{"name":"","ecosystem":""}}'`), or ecosystem tools (`npm audit`, `pip-audit`, `cargo audit`). +2. If the update is itself a security patch, note the CVE(s) it addresses and confirm the fix is present in the version being adopted. +3. Check whether the new version introduces new advisories, since a patch can pull in a vulnerable transitive dependency. +4. Report as **No known advisories**, **Fixes CVE-XXXX-YYYY (severity)**, or **HOLD - OPEN ADVISORY: CVE-XXXX-YYYY**. ### 4. Community Signals (best effort) -**Goal:** See if real users are reporting problems, compromises, or regressions with this release. +Look for real users reporting problems, compromises, or regressions with this release. -**Steps:** +1. Check the repo's issues filed after the release date (`gh api "search/issues?q=repo:/+created:>"`). +2. With web access, also search the package name + version on Stack Overflow, Hacker News, and the ecosystem's channels, looking for several people reporting the same crash or suspicious activity. Without web access, limit this check to the CLI/API sources and say so. +3. Compare download counts against the package's historical trend where the registry exposes them; a sudden spike or drop can indicate typosquatting or an abandoned fork. -1. Check the package's GitHub Issues for reports filed after the release date (e.g. `gh api "search/issues?q=repo:/+created:>"` or `gh issue list --repo /`). -2. If you have web access, also search for the package name + version on Stack Overflow, Hacker News, and the ecosystem's community channels; look for patterns such as multiple people reporting the same crash or suspicious activity. -3. Compare download counts against the package's historical trend where the registry exposes them (e.g. `npm view ` plus the npm downloads API). A sudden spike or drop can indicate typosquatting or an abandoned fork. -4. Without web access, limit this check to what the CLI/API sources above can answer and say so. - -**Report as:** A brief summary of community sentiment, "No community issues found for this version" if clean, or **SKIPPED** with the reason if the sources were unreachable. +Report a brief sentiment summary, "No community issues found for this version" if clean, or **SKIPPED** with the reason. ### 5. Breaking Changes and Migration Notes -**Goal:** Identify API or behavioral changes that could break existing code. - -**Steps:** - -1. Check the bump against semver. Breaking changes in a minor or patch release are a red flag on their own — either accidental or a sign of poor maintenance practices. +1. Check the bump against semver. Breaking changes in a minor or patch release are a red flag on their own, either accidental or a sign of poor maintenance. 2. Read the migration guide or upgrade notes if one exists. -3. Look at the diff for: removed or renamed exports, changed function signatures, altered default values, removed configuration options, or dropped support for runtimes/platforms. -4. Search the codebase for usages of any changed or removed APIs. List the files and line numbers that may need updates. -5. Note any changes to the package's peer dependency requirements, minimum runtime versions (Node, Python, Ruby, etc.), or required environment variables. - -**Report as:** +3. Look in the diff for removed or renamed exports, changed function signatures, altered default values, removed configuration options, or dropped runtime/platform support. +4. Search the codebase for usages of anything changed or removed. List the files and line numbers that may need updates. +5. Note changes to peer dependency requirements, minimum runtime versions (Node, Python, Ruby), or required environment variables. -- **No breaking changes** for seamless upgrades. -- **Breaking changes detected** with a list of what changed and which files in the codebase are affected. -- **Potential breaking changes** for behavioral changes that may not cause compile/import errors but could alter runtime behavior (e.g., a default timeout changing from 30s to 5s). +Report as **No breaking changes**; **Breaking changes detected** with what changed and which files are affected; or **Potential breaking changes** for behavioral shifts that compile and import fine but alter runtime behavior (e.g. a default timeout dropping from 30s to 5s). ## Output Format -Present the full review as a structured report. Here is the template: +Present the full review as a structured report: ```text # Package Update Review @@ -115,13 +94,11 @@ Present the full review as a structured report. Here is the template: ## Edge Cases -- **New dependencies** (not just version bumps): Apply the same five checks but also verify the package is the intended one (check for typosquatting by comparing to similarly named popular packages) and review its overall maintenance health (last commit date, number of maintainers, bus factor). -- **Lockfile-only changes** with no manifest change: These can happen from transitive dependency resolution. Still review the transitive packages that changed, though a lighter touch is acceptable for patch-level transitive bumps in well-known packages. -- **Monorepos with many packages:** Group related packages (e.g., `@babel/*` or `@angular/*`) and note that they are part of a coordinated release, which reduces (but does not eliminate) the need for individual diff review. -- **Private/internal packages:** The publication age gate may not apply, but the diff verification and breaking change checks still do. +- **New dependencies** (not just version bumps): same five checks, plus verify it is the intended package (compare against similarly named popular ones for typosquatting) and review maintenance health: last commit date, maintainer count, bus factor. +- **Lockfile-only changes** with no manifest change: still review the transitive packages that changed, though a lighter touch is acceptable for patch-level bumps in well-known packages. +- **Monorepo groups** (`@babel/*`, `@angular/*`): treat as one coordinated release, which reduces but does not eliminate individual diff review. +- **Private/internal packages:** the publication age gate may not apply; diff verification and breaking change checks still do. Never send internal package names or versions to public registry or advisory endpoints. Query the private registry where it exposes an API, otherwise mark those checks **SKIPPED** with the reason. -## Tips for Efficiency +## Priorities -- Start with the publication age gate; it is the fastest check and can immediately flag the riskiest updates. -- For large dependency updates (e.g., Dependabot batches), prioritize direct dependencies over transitive ones, and prioritize packages with install hooks. -- If a package has hundreds of thousands of weekly downloads and is maintained by a well-known org (e.g., Meta, Google, Vercel), the changelog and community checks can be lighter. But never skip the security advisory check. +Start with the publication age gate; it is the fastest check and immediately flags the riskiest updates. In large batches (Dependabot/Renovate), prioritize direct dependencies over transitive ones, and packages with install hooks over those without. For high-download packages from well-known orgs, the changelog and community checks can be lighter; never skip the security advisory check. diff --git a/.antigravity/skills/efficient-orchestration/SKILL.md b/.antigravity/skills/efficient-orchestration/SKILL.md index 4e56a5d..453d903 100644 --- a/.antigravity/skills/efficient-orchestration/SKILL.md +++ b/.antigravity/skills/efficient-orchestration/SKILL.md @@ -1,6 +1,6 @@ --- name: efficient-orchestration -description: "Run this task with your current model orchestrating while cheaper subagents do the token-heavy research, coding, and testing. Use for work that is large, parallelizable, or token-hungry — broad repo scans, long logs, wide test or browser passes, repetitive edits — or when the user asks to conserve usage limits. Skip it for small, sequential, or judgment-dense tasks." +description: "Run this task with your current model orchestrating while cheaper subagents do the token-heavy research, coding, and testing. Use for work that is large, parallelizable, or token-hungry (broad repo scans, long logs, wide test or browser passes, repetitive edits), or when the user asks to conserve usage limits. Skip it for small, sequential, or judgment-dense tasks." --- # Efficient Orchestration @@ -21,15 +21,15 @@ Order the models your harness can run from cheapest to most capable (e.g. on Cla ## Pick each subagent's model -By task difficulty, not task type — and never above your own tier: +By task difficulty, not task type, and never above your own tier: -- **Cheapest tier:** mechanical, high-volume, low-judgment work — search sweeps, inventory, log reduction, simple edits. +- **Cheapest tier:** mechanical, high-volume, low-judgment work: search sweeps, inventory, log reduction, simple edits. - **Mid tier (default):** focused research, routine or narrow patches, test runs, straightforward debugging. -- **Your tier:** complex work delegated for parallelism or context isolation, not savings — intricate refactors, multi-file features, subtle bugs, design exploration. +- **Your tier:** complex work delegated for parallelism or context isolation, not savings: intricate refactors, multi-file features, subtle bugs, design exploration. -Start at the cheapest tier that can plausibly succeed; after two failures at a tier, escalate one tier or take the work back — never a third retry at the same tier. +Start at the cheapest tier that can plausibly succeed; after two failures at a tier, escalate one tier or take the work back; never a third retry at the same tier. -Pin an explicit model on every spawn, and where the harness offers stable aliases (e.g. Claude's `haiku`/`sonnet`/`opus`), prefer them over dated full IDs — aliases survive model rotations; pinned IDs hard-fail. Don't assume built-in subagents are cheap: some harnesses default them to inheriting your main-session model (Claude Code's built-in Explore/Plan/general-purpose do; a per-invocation model overrides it, and a user-level `Explore` agent with `model: haiku` catches the spontaneous searches you don't route). Where the harness supports per-agent reasoning effort, run cheap-tier recon and mechanical work at low effort — current-generation low roughly matches previous-generation highest. +Pin an explicit model on every spawn, preferring your harness's stable aliases (Claude's `haiku`/`sonnet`/`opus`; check your own tool's model list for equivalents) over dated full IDs, since aliases survive model rotations while pinned IDs hard-fail. Don't assume built-in subagents are cheap: some harnesses default them to inheriting your main-session model, so read the agent definitions and set a per-invocation model where they do. On Claude Code, the built-in Explore/Plan/general-purpose agents inherit, and a user-level `Explore` agent with `model: haiku` catches the spontaneous searches you don't route. Where the harness supports per-agent reasoning effort, run cheap-tier recon and mechanical work at low effort. ## Run it @@ -41,18 +41,18 @@ Pin an explicit model on every spawn, and where the harness offers stable aliase ## Usage limits -- Know what delegation buys. On pay-per-token APIs, cheaper tiers cut real dollars. On subscriptions, most cost is context reprocessing, and each subagent rebuilds context whose findings flow back into yours — total tokens can rise while the binding quota falls. The durable wins are bucket arbitrage (e.g. Claude's separate Sonnet-only weekly allowance, while the frontier tier drains the shared bucket fastest), wall-clock parallelism, and a lean main context. +- On pay-per-token APIs, cheaper tiers cut real dollars. On subscriptions, each subagent rebuilds context whose findings flow back into yours, so total tokens can rise while the binding quota falls; the durable wins there are bucket arbitrage (e.g. Claude's separate Sonnet-only weekly allowance, while the frontier tier drains the shared bucket fastest), wall-clock parallelism, and a lean main context. - Delegate in bounded waves (~3 parallel); between waves check the harness's usage surface (on Claude Code, `npx -y ccusage@latest blocks --active --json`; elsewhere a usage/status command if one exists, or treat the first rate-limit error as the cap). Stop launching once any usage window nears ~95%; let in-flight work finish. -- For long unattended runs, pause at the cap and resume when it clears: finish the wave, then use the harness's scheduled-wakeup primitive, chaining wakeups of ≤3600s until the window clears — never busy-wait with `sleep`. If no such primitive exists, write a self-contained resume prompt to a handoff file (remaining plan, the 95% rule, the exact usage command and its last reading, subagent handoffs) and tell the user to relaunch with it. On resume, re-verify with the usage command — a fresh block timestamp, not elapsed wall-clock, proves rollover. Tell the user which window tripped, the observed %, the next check time, and the outstanding work. +- For long unattended runs, pause at the cap and resume when it clears: finish the wave, then use the harness's scheduled-wakeup primitive, chaining wakeups of ≤3600s until the window clears, never busy-waiting with `sleep`. If no such primitive exists, write a self-contained resume prompt to a handoff file (remaining plan, the 95% rule, the exact usage command and its last reading, subagent handoffs) and tell the user to relaunch with it. On resume, re-verify with the usage command: a fresh block timestamp, not elapsed wall-clock, proves rollover. Tell the user which window tripped, the observed %, the next check time, and the outstanding work. ## Vet results -Reports are leads, not facts. Before acting on a high-impact finding, opening a PR, or claiming done: reopen key cited files, confirm line refs and failures, review the final diff, and resolve subagent disagreements yourself. For non-trivial completed work, spawn a fresh-context verifier on your tier that only tries to refute the claim — rerun the tests, drive the affected flow, probe edge cases — and never fixes anything; independent refutation beats self-review. +Reports are leads, not facts. Before acting on a high-impact finding, opening a PR, or claiming done: reopen key cited files, confirm line refs and failures, review the final diff, and resolve subagent disagreements yourself. For non-trivial completed work, spawn a fresh-context verifier on your tier that only tries to refute the claim (rerun the tests, drive the affected flow, probe edge cases) and never fixes anything; independent refutation beats self-review. ## Guardrails - Don't delegate a blocker your next step needs. -- Don't let two subagents edit the same files, and don't implement slices workers own — coordination plus duplicated implementation costs more than either alone. +- Don't let two subagents edit the same files, and don't implement slices workers own; coordination plus duplicated implementation costs more than either alone. - Enforce read-only roles (recon, review, verification) by tool allowlist where the harness supports it, not prompt text. -- Route security-sensitive work (authn/authz, secrets, crypto, hardening) to a capable non-frontier tier — frontier safety classifiers can refuse benign defensive work mid-task. +- Route security-sensitive work (authn/authz, secrets, crypto, hardening) to a capable non-frontier tier; frontier safety classifiers can refuse benign defensive work mid-task. - If the task is tiny, doesn't parallelize, or needs delicate judgment throughout, skip the ceremony and do it yourself. diff --git a/.antigravity/skills/review-pr/SKILL.md b/.antigravity/skills/review-pr/SKILL.md index 27576cf..a7dbad5 100644 --- a/.antigravity/skills/review-pr/SKILL.md +++ b/.antigravity/skills/review-pr/SKILL.md @@ -45,15 +45,15 @@ gh api graphql --paginate --slurp \ **Auto-resolve:** If a thread's first comment body matches any `$IGNORED_FILE` entry (`grep -qxF`), resolve via `resolveReviewThread` mutation without classifying. -If unresolved threads remain → step 3. Do NOT re-request a bot review while threads are still open — process existing feedback first. Only when zero unresolved threads remain → step 5. +If unresolved threads remain → step 3. Do NOT re-request a bot review while threads are still open; process existing feedback first. Only when zero unresolved threads remain → step 5. ### 3. Classify and resolve Read referenced file + context for each remaining thread, then classify: -- **Already addressed / Informational / Inaccurate** — append body to `$IGNORED_FILE`, resolve (reply with brief explanation if inaccurate). -- **Valid fix** — implement minimal change. Must meet ALL: (1) fixes a real bug — wrong behavior, data loss, security, crash, or race condition; (2) net-simpler or complexity-neutral; (3) concrete, not speculative. -- **Nitpick / Low-value** — resolve WITHOUT implementing. Includes: style preferences not enforced by linter, docstring suggestions on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append body to `$IGNORED_FILE`, reply with one-line rationale, resolve. +- **Already addressed / Informational / Inaccurate**: append body to `$IGNORED_FILE`, resolve (reply with brief explanation if inaccurate). +- **Valid fix**: implement minimal change. Must meet ALL: (1) fixes a real bug (wrong behavior, data loss, security, crash, or race condition); (2) net-simpler or complexity-neutral; (3) concrete, not speculative. +- **Nitpick / Low-value**: resolve WITHOUT implementing. Includes: style preferences not enforced by linter, docstring suggestions on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append body to `$IGNORED_FILE`, reply with one-line rationale, resolve. ### 4. Push fixes @@ -61,34 +61,52 @@ Stage, commit (`fix:`/`refactor:`/etc.), push, verify CI green, resolve fixed th ### 5. Ensure bot review covers latest commit -Only reached when zero unresolved threads remain. Get HEAD SHA: `gh pr view {PR_NUMBER} --json commits --jq '.commits[-1].oid'`. Bots = logins ending in `[bot]`. Fetch their latest reviews: +Each bot's latest review, and the commit it covers: ```bash -gh api repos/{owner}/{repo}/pulls/{PR_NUMBER}/reviews \ - --jq '[.[] | select(.user.login | endswith("[bot]"))] | group_by(.user.login) | map(max_by(.submitted_at))' +head_sha=$(gh pr view {PR_NUMBER} --json commits --jq '.commits[-1].oid') + +latest() { gh api --paginate --slurp repos/{owner}/{repo}/pulls/{PR_NUMBER}/reviews \ + | jq -r 'add | [.[] | select(.user.login | endswith("[bot]"))] | group_by(.user.login) + | map(max_by(.submitted_at)) | .[] | "\(.user.login) \(.commit_id)"'; } + +stale=$(latest | grep -v " $head_sha$" | cut -d' ' -f1 | sort -u) ``` -If a bot's latest review already covers HEAD → success. Stop. +`/reviews` alone identifies the review bots; CI and deploy bots never appear there. `--slurp` piped to `jq`, not `--jq`: under `--paginate` a `--jq` filter runs per page, so `max_by` returns a per-page max and lists a long-running PR's bots twice. + +Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise re-trigger each login in `stale`; they do not re-review a push on their own. + +| Bot | Login | Re-trigger with | +| --- | --- | --- | +| Copilot | `copilot-pull-request-reviewer[bot]` | `gh pr edit {PR_NUMBER} --add-reviewer @copilot` | +| CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | +| Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -Otherwise, re-request and poll (first check 8 min, timeout 15 min, poll 60 s). `gh pr edit --add-reviewer` re-requests reviews from existing bot reviewers — do not skip this. +- Pass the literal `@copilot`; its raw `[bot]` login can exit 0 having requested nothing. Confirm Copilot specifically, not just that some reviewer is pending: `gh api repos/{owner}/{repo}/pulls/{PR_NUMBER} --jq '.requested_reviewers[].login' | grep -qiE '^(Copilot|copilot-pull-request-reviewer\[bot\])$'`. A miss means it did not take, and the poll below would burn its full timeout waiting. Match both spellings: `requested_reviewers` returns the login as `Copilot`, while the review it later submits carries `copilot-pull-request-reviewer[bot]`, so checking only the `[bot]` form reports failure on every successful request. +- App-based bots (CodeRabbit, Greptile) cannot be requested as reviewers at all; a mention is their only trigger. `@coderabbitai full review` re-reviews the whole diff rather than just new commits. +- **Bot not in the table, or none found** → ask the user for the exact trigger. Never guess a mention string: a wrong one posts a visible no-op comment. + +Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired, one per line, dropping any you could not trigger: ```bash -bot_logins=$(gh api repos/{owner}/{repo}/pulls/{PR_NUMBER}/reviews \ - --jq '[.[] | select(.user.login | endswith("[bot]")) | .user.login] | unique | .[]') +triggered="$stale" # minus any bot you could not trigger -for bot in $bot_logins; do - gh pr edit {PR_NUMBER} --add-reviewer "$bot" -done +# Never poll on an empty set: comm would report nothing pending and the loop +# would break on the first pass, declaring success without waiting. +[ -n "$triggered" ] || { echo "nothing was triggered"; exit 1; } end=$((SECONDS+900)); sleep 480 while [ $SECONDS -lt $end ]; do - commit_id=$(gh api repos/{owner}/{repo}/pulls/{PR_NUMBER}/reviews \ - --jq '[.[] | select(.user.login=="{bot}")] | max_by(.submitted_at) | .commit_id') - [ "$commit_id" = "$head_sha" ] && break + pending=$(comm -23 <(printf '%s\n' "$triggered" | sort -u) \ + <(latest | grep " $head_sha$" | cut -d' ' -f1 | sort -u)) + [ -z "$pending" ] && break sleep 60 done ``` -Timeout → tell user to re-run this command and stop. Success → go back to step 2. +Run both blocks in one shell: `head_sha` and `latest` do not survive separate tool calls. If your harness blocks foreground `sleep`, run the whole wait as one backgrounded command rather than sleeping between tool calls. + +Timeout → name the bots still pending, tell user to re-run this command, stop. Success → go back to step 2. -Declare success when step 2 finds zero unresolved threads AND step 5 confirms a bot review on HEAD. Stop at iteration 5. Report: threads resolved, fixes made, threads auto-ignored, threads remaining, CI status. +Stop at iteration 5. Report: threads resolved, fixes made, threads auto-ignored, threads remaining, CI status. diff --git a/.claude/agents/Explore.md b/.claude/agents/Explore.md index 80c018f..23255cb 100644 --- a/.claude/agents/Explore.md +++ b/.claude/agents/Explore.md @@ -1,6 +1,6 @@ --- name: Explore -description: Read-only search agent for broad fan-out searches — when answering means sweeping many files, directories, or naming conventions and you only need the conclusion, not the file dumps. It reads excerpts rather than whole files, so it locates code; it doesn't review or audit it. Specify search breadth: "quick" for targeted lookups, "medium" for moderate exploration, "very thorough" for multiple locations and naming conventions. +description: Read-only search agent for broad fan-out searches, for when answering means sweeping many files, directories, or naming conventions and you only need the conclusion, not the file dumps. It reads excerpts rather than whole files, so it locates code; it doesn't review or audit it. Specify search breadth: "quick" for targeted lookups, "medium" for moderate exploration, "very thorough" for multiple locations and naming conventions. model: haiku effort: low tools: Read, Glob, Grep @@ -9,7 +9,7 @@ tools: Read, Glob, Grep # Explore You are a fast, read-only exploration agent. Sweep the codebase at the requested -breadth, locate what was asked for, and report conclusions — not raw file +breadth, locate what was asked for, and report conclusions, not raw file contents. - Search first (Glob/Grep), then Read only the relevant excerpts; never dump diff --git a/.claude/commands/code-refinement.md b/.claude/commands/code-refinement.md index 7c6e0d5..4eb105d 100644 --- a/.claude/commands/code-refinement.md +++ b/.claude/commands/code-refinement.md @@ -1,5 +1,5 @@ --- -description: "Review staged files for code quality (KISS, DRY, YAGNI, Clean Code) and fix linting issues. Use to clean up staged work before review or commit — unlike code-review, this one edits the code: it applies refactors, runs the linter, and fills test gaps." +description: "Review staged files for code quality (KISS, DRY, YAGNI, Clean Code) and fix linting issues. Use to clean up staged work before review or commit. Unlike code-review, this one edits the code: it applies refactors, runs the linter, and fills test gaps." --- # Code Refinement diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index d22f036..57dacdd 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -4,30 +4,11 @@ description: "Review staged changes for security, correctness, performance, and # Role -You are a senior code reviewer and security expert. -You only read and analyze the code — you must never modify any source code files in the repository. -The sole exception is writing your review output into a Markdown file. -You never ask the user what to do next and you produce exactly one review report per run. - -## Output Location - -- Always write your complete review to a file named `agent-code-review.md` in the project root. -- Overwrite the file completely on each run — do not append. -- This file is the only file you may create or modify. -- Do not stage, commit, or push this file. - -## Iterative Review Behavior - -- On each run, treat the task as a fresh review of the currently staged changes. -- Continue reviewing until there are no High or Medium severity issues and no Low severity blockers, then clearly state in the Summary that the code is good to go. +You are a senior code reviewer and security expert. Read and analyze only; never modify a source file. `agent-code-review.md` in the project root is the single file you may write, overwritten completely each run. Do not stage, commit, or push it. Never ask the user what to do next, and produce exactly one report per run. ## Scope and Inputs -- Review only files that are currently staged in Git, not the entire repository. -- Focus on changed lines and minimal necessary surrounding context. -- If information is missing, state reasonable assumptions and proceed. - -## How to Collect Context +Each run is a fresh review of the currently staged files, not the whole repository. Focus on changed lines and the minimum surrounding context. If information is missing, state a reasonable assumption and proceed. - `git diff --staged --unified=0 --no-color` is the primary input; pull `-U3` when a finding needs surrounding context. - Cite line numbers from the `+` side of each hunk so they match the post-merge file. @@ -36,16 +17,7 @@ You never ask the user what to do next and you produce exactly one review report ## Review Policy -Prioritize findings that materially improve: - -- Security, reliability, data integrity, privacy. -- Correctness and performance where clearly impactful. -- Clarity and Clean Code. - -Avoid nitpicks: - -- Do not flag purely stylistic issues unless a project style rule is clearly violated. -- Recommend formatting or lint rules only when they prevent bugs or confusion. +Prioritize what materially improves security, reliability, data integrity, and privacy; correctness and performance where clearly impactful; and clarity. Do not flag purely stylistic issues unless a project style rule is clearly violated, and recommend formatting or lint rules only when they prevent bugs or confusion. ## Severity Definitions @@ -53,20 +25,13 @@ Avoid nitpicks: - **Medium:** likely bugs, race conditions, significant performance or maintainability problems. - **Low:** clarity, naming, minor cleanup. A Low finding is a blocker only when it violates an explicit project rule (lint configuration or a documented convention); otherwise it never blocks the verdict. -## Security Checklist - -- Map each security finding to OWASP Top Ten, e.g., A01 Broken Access Control, A02 Cryptographic Failures, A03 Injection, etc. -- For HTTP APIs, also consider OWASP API Top 10. -- Provide actionable mitigations. +Review until no High or Medium issues and no Low blockers remain, then record the verdict in the Summary. -## Clean Code and Clarity Checks +## What to Check -- Prefer small, focused functions, clear names, elimination of duplication, obvious control flow. -- Suggest local refactors near changed lines. -- Provide minimal viable patches as examples when safe. -- Identify dead code (unused variables, functions, imports, classes). -- Check for DRY violations (repeated logic or patterns that could be abstracted). -- Check for YAGNI violations (unnecessary code, abstractions, or parameters that add complexity without current value). +- **Security:** map each finding to OWASP Top Ten (A01 Broken Access Control, A02 Cryptographic Failures, A03 Injection, etc.), plus OWASP API Top 10 for HTTP APIs. Provide actionable mitigations. +- **Clean code:** small focused functions, clear names, obvious control flow. Suggest local refactors near changed lines, with minimal viable patches as examples when safe. +- **Dead code, DRY, YAGNI:** unused variables, functions, imports, or classes; repeated logic worth abstracting; abstractions or parameters that add complexity without current value. ## Output Format @@ -83,7 +48,7 @@ Write the following structure into `agent-code-review.md`. `N` is the review ite - One paragraph on overall risk and clarity. - Finding counts: High X, Medium Y, Low Z. -- If no High or Medium remain and no Low blockers, state: **Verdict: good to go**. +- If no High or Medium remain and no Low blockers, state: **Verdict: good to go**. Automation depends on detecting this exact string. ## Findings diff --git a/.claude/commands/commitmsg.md b/.claude/commands/commitmsg.md index ef7a6da..3fd4d9a 100644 --- a/.claude/commands/commitmsg.md +++ b/.claude/commands/commitmsg.md @@ -1,5 +1,5 @@ --- -description: "Propose a single git commit message for the currently staged changes. Use when asked to write, draft, or suggest a commit message, or to check that a message matches the repo's conventions. Proposes the message only — it does not commit." +description: "Propose a single git commit message for the currently staged changes. Use when asked to write, draft, or suggest a commit message, or to check that a message matches the repo's conventions. Proposes the message only; it does not commit." allowed-tools: Bash(git diff:*), Bash(git status:*), Bash(git log:*), Bash(git branch:*) --- @@ -9,10 +9,10 @@ allowed-tools: Bash(git diff:*), Bash(git status:*), Bash(git log:*), Bash(git b Run these commands to understand the changes: -- `git diff --staged --stat` — the shape of the change; then `git diff --staged` (or per-file diffs) only for files whose purpose isn't clear from the stat -- `git status -s` — staged file list -- `git log -n 20 --oneline` — recent style and to avoid repetition -- `git branch --show-current` — if it contains a ticket ID (e.g. ABC-123), prefix the subject line +- `git diff --staged --stat`: the shape of the change; then `git diff --staged` (or per-file diffs) only for files whose purpose isn't clear from the stat +- `git status -s`: staged file list +- `git log -n 20 --oneline`: recent style and to avoid repetition +- `git branch --show-current`: if it contains a ticket ID (e.g. ABC-123), prefix the subject line ## Rules diff --git a/.claude/commands/dependency-review.md b/.claude/commands/dependency-review.md index 7d971c5..9516d2f 100644 --- a/.claude/commands/dependency-review.md +++ b/.claude/commands/dependency-review.md @@ -1,35 +1,29 @@ --- -description: "Audit package dependency updates for supply-chain risk: publish-age gate, changelog/diff verification, security advisories, community signals, and breaking changes. Use whenever a branch, PR, or working directory changes a dependency manifest or lockfile — package.json, requirements.txt, pyproject.toml, Gemfile, go.mod, Cargo.toml, pom.xml, build.gradle, composer.json, pubspec.yaml, or the lockfile beside them — including Dependabot/Renovate batches and newly added packages." +description: "Audit package dependency updates for supply-chain risk: publish-age gate, changelog/diff verification, security advisories, community signals, and breaking changes. Use whenever a branch, PR, or working directory changes a dependency manifest or lockfile (package.json, requirements.txt, pyproject.toml, Gemfile, go.mod, Cargo.toml, pom.xml, build.gradle, composer.json, pubspec.yaml, or the lockfile beside them), including Dependabot/Renovate batches and newly added packages." --- # Package Update Supply Chain Review -Review dependency updates to catch supply chain attacks, breaking changes, and risky packages before they land in your codebase. - ## Review Workflow -For each updated or newly added package, work through all five checks below. Prefer CLI and API lookups (`npm view`, `pip index`, `gh api`, `curl` against registry/OSV endpoints) over web browsing — they are faster, cheaper, and available in more environments. Never invent results for a check you could not actually perform: report it as **SKIPPED** with the reason. Present findings in a single summary report at the end, grouped by package. Flag any failing check as a **HOLD** and recommend the team investigate before merging. +For each updated or newly added package, work through all five checks below. Prefer CLI and API lookups (`npm view`, `pip index`, `gh api`, `curl` against registry/OSV endpoints) over web browsing. Never invent a result for a check you could not actually perform: report it as **SKIPPED** with the reason. Present findings in a single summary report at the end, grouped by package, and flag any failing check as a **HOLD** to investigate before merging. ### 1. Publication Age Gate -**Goal:** Confirm the release is at least 7 days old. Compromised or typosquatted releases are usually caught within the first few days, so letting a release "bake" gives the community and automated scanners time to notice. - -**Steps:** +Confirm the release is at least 7 days old. Compromised and typosquatted releases are usually caught within the first few days, so letting one bake gives scanners and the community time to notice. -1. Look up the publish date for the exact version on its registry — e.g. `npm view time --json`, `curl https://pypi.org/pypi///json`, `gem info --remote`, or the registry's web page. -2. If fewer than 7 days have elapsed, flag this as **HOLD - TOO NEW** and include the publish date, the age in days, and a recommendation to wait or pin to the prior version. +1. Look up the publish date for the exact version (`npm view time --json`, `curl https://pypi.org/pypi///json`, or the registry's page). +2. Under 7 days → **HOLD - TOO NEW**, with the publish date, the age in days, and a recommendation to wait or pin to the prior version. ### 2. Changelog and Diff Verification -**Goal:** Confirm the code changes match what the release notes claim. - -**Steps:** +Confirm the code changes match what the release notes claim. -1. Locate the changelog, release notes, or GitHub releases page for the new version (e.g. `gh release view --repo /`). -2. Identify the claimed changes (bug fixes, features, refactors, etc.). -3. Skim the actual source diff between the old and new version (e.g. `gh api repos///compare/v1.2.3...v1.4.0` or the repo's compare view). -4. Look for discrepancies: Are there unexpected new files? New network calls? Obfuscated code? Post-install scripts that were not present before? -5. Pay special attention to install hooks (`preinstall`, `postinstall` in npm; `setup.py` entry points in Python; `build.rs` changes in Rust; etc.) since these execute automatically and are a top vector for supply chain attacks. +1. Locate the changelog or releases page for the new version (`gh release view --repo /`). +2. Identify the claimed changes. +3. Skim the source diff between the old and new version (`gh api repos///compare/v1.2.3...v1.4.0`). +4. Look for discrepancies: unexpected new files, new network calls, obfuscated code, post-install scripts that were not there before. +5. Pay special attention to install hooks (`preinstall`/`postinstall` in npm, `setup.py` entry points in Python, `build.rs` in Rust); they execute automatically and are a top supply chain vector. **Red flags to call out:** @@ -37,53 +31,38 @@ For each updated or newly added package, work through all five checks below. Pre - New outbound HTTP/DNS calls, especially to IP addresses or unusual domains - Environment variable reads for tokens, keys, or credentials - New native/binary dependencies or compiled assets -- Changes to CI config or build scripts that fetch remote resources +- CI config or build script changes that fetch remote resources ### 3. Security Advisory Review -**Goal:** Check whether the package or specific version has known vulnerabilities. - -**Steps:** - -1. Query advisory sources for the package name and version range: `gh api /advisories --method GET -f "affects="`, the OSV API (`curl -s https://api.osv.dev/v1/query -d '{"package":{"name":"","ecosystem":""}}'`), or ecosystem tools (`npm audit`, `pip-audit`, `cargo audit`). -2. Check whether the update itself is a security patch. If so, note the CVE(s) it addresses and confirm the fix is present in the version being adopted. -3. Check whether the new version introduces any new advisories. This can happen when a patch also pulls in a vulnerable transitive dependency. -4. Report findings as: **No known advisories**, **Fixes CVE-XXXX-YYYY (severity)**, or **HOLD - OPEN ADVISORY: CVE-XXXX-YYYY**. +1. Query advisories for the package name and version range: `gh api /advisories --method GET -f "affects="`, the OSV API (`curl -s https://api.osv.dev/v1/query -d '{"package":{"name":"","ecosystem":""}}'`), or ecosystem tools (`npm audit`, `pip-audit`, `cargo audit`). +2. If the update is itself a security patch, note the CVE(s) it addresses and confirm the fix is present in the version being adopted. +3. Check whether the new version introduces new advisories, since a patch can pull in a vulnerable transitive dependency. +4. Report as **No known advisories**, **Fixes CVE-XXXX-YYYY (severity)**, or **HOLD - OPEN ADVISORY: CVE-XXXX-YYYY**. ### 4. Community Signals (best effort) -**Goal:** See if real users are reporting problems, compromises, or regressions with this release. +Look for real users reporting problems, compromises, or regressions with this release. -**Steps:** +1. Check the repo's issues filed after the release date (`gh api "search/issues?q=repo:/+created:>"`). +2. With web access, also search the package name + version on Stack Overflow, Hacker News, and the ecosystem's channels, looking for several people reporting the same crash or suspicious activity. Without web access, limit this check to the CLI/API sources and say so. +3. Compare download counts against the package's historical trend where the registry exposes them; a sudden spike or drop can indicate typosquatting or an abandoned fork. -1. Check the package's GitHub Issues for reports filed after the release date (e.g. `gh api "search/issues?q=repo:/+created:>"` or `gh issue list --repo /`). -2. If you have web access, also search for the package name + version on Stack Overflow, Hacker News, and the ecosystem's community channels; look for patterns such as multiple people reporting the same crash or suspicious activity. -3. Compare download counts against the package's historical trend where the registry exposes them (e.g. `npm view ` plus the npm downloads API). A sudden spike or drop can indicate typosquatting or an abandoned fork. -4. Without web access, limit this check to what the CLI/API sources above can answer and say so. - -**Report as:** A brief summary of community sentiment, "No community issues found for this version" if clean, or **SKIPPED** with the reason if the sources were unreachable. +Report a brief sentiment summary, "No community issues found for this version" if clean, or **SKIPPED** with the reason. ### 5. Breaking Changes and Migration Notes -**Goal:** Identify API or behavioral changes that could break existing code. - -**Steps:** - -1. Check the bump against semver. Breaking changes in a minor or patch release are a red flag on their own — either accidental or a sign of poor maintenance practices. +1. Check the bump against semver. Breaking changes in a minor or patch release are a red flag on their own, either accidental or a sign of poor maintenance. 2. Read the migration guide or upgrade notes if one exists. -3. Look at the diff for: removed or renamed exports, changed function signatures, altered default values, removed configuration options, or dropped support for runtimes/platforms. -4. Search the codebase for usages of any changed or removed APIs. List the files and line numbers that may need updates. -5. Note any changes to the package's peer dependency requirements, minimum runtime versions (Node, Python, Ruby, etc.), or required environment variables. - -**Report as:** +3. Look in the diff for removed or renamed exports, changed function signatures, altered default values, removed configuration options, or dropped runtime/platform support. +4. Search the codebase for usages of anything changed or removed. List the files and line numbers that may need updates. +5. Note changes to peer dependency requirements, minimum runtime versions (Node, Python, Ruby), or required environment variables. -- **No breaking changes** for seamless upgrades. -- **Breaking changes detected** with a list of what changed and which files in the codebase are affected. -- **Potential breaking changes** for behavioral changes that may not cause compile/import errors but could alter runtime behavior (e.g., a default timeout changing from 30s to 5s). +Report as **No breaking changes**; **Breaking changes detected** with what changed and which files are affected; or **Potential breaking changes** for behavioral shifts that compile and import fine but alter runtime behavior (e.g. a default timeout dropping from 30s to 5s). ## Output Format -Present the full review as a structured report. Here is the template: +Present the full review as a structured report: ```text # Package Update Review @@ -114,13 +93,11 @@ Present the full review as a structured report. Here is the template: ## Edge Cases -- **New dependencies** (not just version bumps): Apply the same five checks but also verify the package is the intended one (check for typosquatting by comparing to similarly named popular packages) and review its overall maintenance health (last commit date, number of maintainers, bus factor). -- **Lockfile-only changes** with no manifest change: These can happen from transitive dependency resolution. Still review the transitive packages that changed, though a lighter touch is acceptable for patch-level transitive bumps in well-known packages. -- **Monorepos with many packages:** Group related packages (e.g., `@babel/*` or `@angular/*`) and note that they are part of a coordinated release, which reduces (but does not eliminate) the need for individual diff review. -- **Private/internal packages:** The publication age gate may not apply, but the diff verification and breaking change checks still do. +- **New dependencies** (not just version bumps): same five checks, plus verify it is the intended package (compare against similarly named popular ones for typosquatting) and review maintenance health: last commit date, maintainer count, bus factor. +- **Lockfile-only changes** with no manifest change: still review the transitive packages that changed, though a lighter touch is acceptable for patch-level bumps in well-known packages. +- **Monorepo groups** (`@babel/*`, `@angular/*`): treat as one coordinated release, which reduces but does not eliminate individual diff review. +- **Private/internal packages:** the publication age gate may not apply; diff verification and breaking change checks still do. Never send internal package names or versions to public registry or advisory endpoints. Query the private registry where it exposes an API, otherwise mark those checks **SKIPPED** with the reason. -## Tips for Efficiency +## Priorities -- Start with the publication age gate; it is the fastest check and can immediately flag the riskiest updates. -- For large dependency updates (e.g., Dependabot batches), prioritize direct dependencies over transitive ones, and prioritize packages with install hooks. -- If a package has hundreds of thousands of weekly downloads and is maintained by a well-known org (e.g., Meta, Google, Vercel), the changelog and community checks can be lighter. But never skip the security advisory check. +Start with the publication age gate; it is the fastest check and immediately flags the riskiest updates. In large batches (Dependabot/Renovate), prioritize direct dependencies over transitive ones, and packages with install hooks over those without. For high-download packages from well-known orgs, the changelog and community checks can be lighter; never skip the security advisory check. diff --git a/.claude/commands/efficient-orchestration.md b/.claude/commands/efficient-orchestration.md index 602fa18..06e3546 100644 --- a/.claude/commands/efficient-orchestration.md +++ b/.claude/commands/efficient-orchestration.md @@ -1,5 +1,5 @@ --- -description: "Run this task with your current model orchestrating while cheaper subagents do the token-heavy research, coding, and testing. Use for work that is large, parallelizable, or token-hungry — broad repo scans, long logs, wide test or browser passes, repetitive edits — or when the user asks to conserve usage limits. Skip it for small, sequential, or judgment-dense tasks." +description: "Run this task with your current model orchestrating while cheaper subagents do the token-heavy research, coding, and testing. Use for work that is large, parallelizable, or token-hungry (broad repo scans, long logs, wide test or browser passes, repetitive edits), or when the user asks to conserve usage limits. Skip it for small, sequential, or judgment-dense tasks." --- # Efficient Orchestration @@ -20,15 +20,15 @@ Order the models your harness can run from cheapest to most capable (e.g. on Cla ## Pick each subagent's model -By task difficulty, not task type — and never above your own tier: +By task difficulty, not task type, and never above your own tier: -- **Cheapest tier:** mechanical, high-volume, low-judgment work — search sweeps, inventory, log reduction, simple edits. +- **Cheapest tier:** mechanical, high-volume, low-judgment work: search sweeps, inventory, log reduction, simple edits. - **Mid tier (default):** focused research, routine or narrow patches, test runs, straightforward debugging. -- **Your tier:** complex work delegated for parallelism or context isolation, not savings — intricate refactors, multi-file features, subtle bugs, design exploration. +- **Your tier:** complex work delegated for parallelism or context isolation, not savings: intricate refactors, multi-file features, subtle bugs, design exploration. -Start at the cheapest tier that can plausibly succeed; after two failures at a tier, escalate one tier or take the work back — never a third retry at the same tier. +Start at the cheapest tier that can plausibly succeed; after two failures at a tier, escalate one tier or take the work back; never a third retry at the same tier. -Pin an explicit model on every spawn, and where the harness offers stable aliases (e.g. Claude's `haiku`/`sonnet`/`opus`), prefer them over dated full IDs — aliases survive model rotations; pinned IDs hard-fail. Don't assume built-in subagents are cheap: some harnesses default them to inheriting your main-session model (Claude Code's built-in Explore/Plan/general-purpose do; a per-invocation model overrides it, and a user-level `Explore` agent with `model: haiku` catches the spontaneous searches you don't route). Where the harness supports per-agent reasoning effort, run cheap-tier recon and mechanical work at low effort — current-generation low roughly matches previous-generation highest. +Pin an explicit model on every spawn, preferring your harness's stable aliases (Claude's `haiku`/`sonnet`/`opus`; check your own tool's model list for equivalents) over dated full IDs, since aliases survive model rotations while pinned IDs hard-fail. Don't assume built-in subagents are cheap: some harnesses default them to inheriting your main-session model, so read the agent definitions and set a per-invocation model where they do. On Claude Code, the built-in Explore/Plan/general-purpose agents inherit, and a user-level `Explore` agent with `model: haiku` catches the spontaneous searches you don't route. Where the harness supports per-agent reasoning effort, run cheap-tier recon and mechanical work at low effort. ## Run it @@ -40,18 +40,18 @@ Pin an explicit model on every spawn, and where the harness offers stable aliase ## Usage limits -- Know what delegation buys. On pay-per-token APIs, cheaper tiers cut real dollars. On subscriptions, most cost is context reprocessing, and each subagent rebuilds context whose findings flow back into yours — total tokens can rise while the binding quota falls. The durable wins are bucket arbitrage (e.g. Claude's separate Sonnet-only weekly allowance, while the frontier tier drains the shared bucket fastest), wall-clock parallelism, and a lean main context. +- On pay-per-token APIs, cheaper tiers cut real dollars. On subscriptions, each subagent rebuilds context whose findings flow back into yours, so total tokens can rise while the binding quota falls; the durable wins there are bucket arbitrage (e.g. Claude's separate Sonnet-only weekly allowance, while the frontier tier drains the shared bucket fastest), wall-clock parallelism, and a lean main context. - Delegate in bounded waves (~3 parallel); between waves check the harness's usage surface (on Claude Code, `npx -y ccusage@latest blocks --active --json`; elsewhere a usage/status command if one exists, or treat the first rate-limit error as the cap). Stop launching once any usage window nears ~95%; let in-flight work finish. -- For long unattended runs, pause at the cap and resume when it clears: finish the wave, then use the harness's scheduled-wakeup primitive, chaining wakeups of ≤3600s until the window clears — never busy-wait with `sleep`. If no such primitive exists, write a self-contained resume prompt to a handoff file (remaining plan, the 95% rule, the exact usage command and its last reading, subagent handoffs) and tell the user to relaunch with it. On resume, re-verify with the usage command — a fresh block timestamp, not elapsed wall-clock, proves rollover. Tell the user which window tripped, the observed %, the next check time, and the outstanding work. +- For long unattended runs, pause at the cap and resume when it clears: finish the wave, then use the harness's scheduled-wakeup primitive, chaining wakeups of ≤3600s until the window clears, never busy-waiting with `sleep`. If no such primitive exists, write a self-contained resume prompt to a handoff file (remaining plan, the 95% rule, the exact usage command and its last reading, subagent handoffs) and tell the user to relaunch with it. On resume, re-verify with the usage command: a fresh block timestamp, not elapsed wall-clock, proves rollover. Tell the user which window tripped, the observed %, the next check time, and the outstanding work. ## Vet results -Reports are leads, not facts. Before acting on a high-impact finding, opening a PR, or claiming done: reopen key cited files, confirm line refs and failures, review the final diff, and resolve subagent disagreements yourself. For non-trivial completed work, spawn a fresh-context verifier on your tier that only tries to refute the claim — rerun the tests, drive the affected flow, probe edge cases — and never fixes anything; independent refutation beats self-review. +Reports are leads, not facts. Before acting on a high-impact finding, opening a PR, or claiming done: reopen key cited files, confirm line refs and failures, review the final diff, and resolve subagent disagreements yourself. For non-trivial completed work, spawn a fresh-context verifier on your tier that only tries to refute the claim (rerun the tests, drive the affected flow, probe edge cases) and never fixes anything; independent refutation beats self-review. ## Guardrails - Don't delegate a blocker your next step needs. -- Don't let two subagents edit the same files, and don't implement slices workers own — coordination plus duplicated implementation costs more than either alone. +- Don't let two subagents edit the same files, and don't implement slices workers own; coordination plus duplicated implementation costs more than either alone. - Enforce read-only roles (recon, review, verification) by tool allowlist where the harness supports it, not prompt text. -- Route security-sensitive work (authn/authz, secrets, crypto, hardening) to a capable non-frontier tier — frontier safety classifiers can refuse benign defensive work mid-task. +- Route security-sensitive work (authn/authz, secrets, crypto, hardening) to a capable non-frontier tier; frontier safety classifiers can refuse benign defensive work mid-task. - If the task is tiny, doesn't parallelize, or needs delicate judgment throughout, skip the ceremony and do it yourself. diff --git a/.claude/commands/review-pr.md b/.claude/commands/review-pr.md index 6cd7973..8c2f846 100644 --- a/.claude/commands/review-pr.md +++ b/.claude/commands/review-pr.md @@ -45,15 +45,15 @@ gh api graphql --paginate --slurp \ **Auto-resolve:** If a thread's first comment body matches any `$IGNORED_FILE` entry (`grep -qxF`), resolve via `resolveReviewThread` mutation without classifying. -If unresolved threads remain → step 3. Do NOT re-request a bot review while threads are still open — process existing feedback first. Only when zero unresolved threads remain → step 5. +If unresolved threads remain → step 3. Do NOT re-request a bot review while threads are still open; process existing feedback first. Only when zero unresolved threads remain → step 5. ### 3. Classify and resolve Read referenced file + context for each remaining thread, then classify: -- **Already addressed / Informational / Inaccurate** — append body to `$IGNORED_FILE`, resolve (reply with brief explanation if inaccurate). -- **Valid fix** — implement minimal change. Must meet ALL: (1) fixes a real bug — wrong behavior, data loss, security, crash, or race condition; (2) net-simpler or complexity-neutral; (3) concrete, not speculative. -- **Nitpick / Low-value** — resolve WITHOUT implementing. Includes: style preferences not enforced by linter, docstring suggestions on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append body to `$IGNORED_FILE`, reply with one-line rationale, resolve. +- **Already addressed / Informational / Inaccurate**: append body to `$IGNORED_FILE`, resolve (reply with brief explanation if inaccurate). +- **Valid fix**: implement minimal change. Must meet ALL: (1) fixes a real bug (wrong behavior, data loss, security, crash, or race condition); (2) net-simpler or complexity-neutral; (3) concrete, not speculative. +- **Nitpick / Low-value**: resolve WITHOUT implementing. Includes: style preferences not enforced by linter, docstring suggestions on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append body to `$IGNORED_FILE`, reply with one-line rationale, resolve. ### 4. Push fixes @@ -61,34 +61,52 @@ Stage, commit (`fix:`/`refactor:`/etc.), push, verify CI green, resolve fixed th ### 5. Ensure bot review covers latest commit -Only reached when zero unresolved threads remain. Get HEAD SHA: `gh pr view {PR_NUMBER} --json commits --jq '.commits[-1].oid'`. Bots = logins ending in `[bot]`. Fetch their latest reviews: +Each bot's latest review, and the commit it covers: ```bash -gh api repos/{owner}/{repo}/pulls/{PR_NUMBER}/reviews \ - --jq '[.[] | select(.user.login | endswith("[bot]"))] | group_by(.user.login) | map(max_by(.submitted_at))' +head_sha=$(gh pr view {PR_NUMBER} --json commits --jq '.commits[-1].oid') + +latest() { gh api --paginate --slurp repos/{owner}/{repo}/pulls/{PR_NUMBER}/reviews \ + | jq -r 'add | [.[] | select(.user.login | endswith("[bot]"))] | group_by(.user.login) + | map(max_by(.submitted_at)) | .[] | "\(.user.login) \(.commit_id)"'; } + +stale=$(latest | grep -v " $head_sha$" | cut -d' ' -f1 | sort -u) ``` -If a bot's latest review already covers HEAD → success. Stop. +`/reviews` alone identifies the review bots; CI and deploy bots never appear there. `--slurp` piped to `jq`, not `--jq`: under `--paginate` a `--jq` filter runs per page, so `max_by` returns a per-page max and lists a long-running PR's bots twice. + +Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise re-trigger each login in `stale`; they do not re-review a push on their own. + +| Bot | Login | Re-trigger with | +| --- | --- | --- | +| Copilot | `copilot-pull-request-reviewer[bot]` | `gh pr edit {PR_NUMBER} --add-reviewer @copilot` | +| CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | +| Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -Otherwise, re-request and poll (first check 8 min, timeout 15 min, poll 60 s). `gh pr edit --add-reviewer` re-requests reviews from existing bot reviewers — do not skip this. +- Pass the literal `@copilot`; its raw `[bot]` login can exit 0 having requested nothing. Confirm Copilot specifically, not just that some reviewer is pending: `gh api repos/{owner}/{repo}/pulls/{PR_NUMBER} --jq '.requested_reviewers[].login' | grep -qiE '^(Copilot|copilot-pull-request-reviewer\[bot\])$'`. A miss means it did not take, and the poll below would burn its full timeout waiting. Match both spellings: `requested_reviewers` returns the login as `Copilot`, while the review it later submits carries `copilot-pull-request-reviewer[bot]`, so checking only the `[bot]` form reports failure on every successful request. +- App-based bots (CodeRabbit, Greptile) cannot be requested as reviewers at all; a mention is their only trigger. `@coderabbitai full review` re-reviews the whole diff rather than just new commits. +- **Bot not in the table, or none found** → ask the user for the exact trigger. Never guess a mention string: a wrong one posts a visible no-op comment. + +Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired, one per line, dropping any you could not trigger: ```bash -bot_logins=$(gh api repos/{owner}/{repo}/pulls/{PR_NUMBER}/reviews \ - --jq '[.[] | select(.user.login | endswith("[bot]")) | .user.login] | unique | .[]') +triggered="$stale" # minus any bot you could not trigger -for bot in $bot_logins; do - gh pr edit {PR_NUMBER} --add-reviewer "$bot" -done +# Never poll on an empty set: comm would report nothing pending and the loop +# would break on the first pass, declaring success without waiting. +[ -n "$triggered" ] || { echo "nothing was triggered"; exit 1; } end=$((SECONDS+900)); sleep 480 while [ $SECONDS -lt $end ]; do - commit_id=$(gh api repos/{owner}/{repo}/pulls/{PR_NUMBER}/reviews \ - --jq '[.[] | select(.user.login=="{bot}")] | max_by(.submitted_at) | .commit_id') - [ "$commit_id" = "$head_sha" ] && break + pending=$(comm -23 <(printf '%s\n' "$triggered" | sort -u) \ + <(latest | grep " $head_sha$" | cut -d' ' -f1 | sort -u)) + [ -z "$pending" ] && break sleep 60 done ``` -Timeout → tell user to re-run this command and stop. Success → go back to step 2. +Run both blocks in one shell: `head_sha` and `latest` do not survive separate tool calls. If your harness blocks foreground `sleep`, run the whole wait as one backgrounded command rather than sleeping between tool calls. + +Timeout → name the bots still pending, tell user to re-run this command, stop. Success → go back to step 2. -Declare success when step 2 finds zero unresolved threads AND step 5 confirms a bot review on HEAD. Stop at iteration 5. Report: threads resolved, fixes made, threads auto-ignored, threads remaining, CI status. +Stop at iteration 5. Report: threads resolved, fixes made, threads auto-ignored, threads remaining, CI status. diff --git a/.codex/skills/code-refinement/SKILL.md b/.codex/skills/code-refinement/SKILL.md index c1c3882..bcb5593 100644 --- a/.codex/skills/code-refinement/SKILL.md +++ b/.codex/skills/code-refinement/SKILL.md @@ -1,6 +1,6 @@ --- name: code-refinement -description: "Review staged files for code quality (KISS, DRY, YAGNI, Clean Code) and fix linting issues. Use to clean up staged work before review or commit — unlike code-review, this one edits the code: it applies refactors, runs the linter, and fills test gaps." +description: "Review staged files for code quality (KISS, DRY, YAGNI, Clean Code) and fix linting issues. Use to clean up staged work before review or commit. Unlike code-review, this one edits the code: it applies refactors, runs the linter, and fills test gaps." --- # Code Refinement diff --git a/.codex/skills/code-review/SKILL.md b/.codex/skills/code-review/SKILL.md index 2c62780..4c0d5d9 100644 --- a/.codex/skills/code-review/SKILL.md +++ b/.codex/skills/code-review/SKILL.md @@ -5,30 +5,11 @@ description: "Review staged changes for security, correctness, performance, and # Role -You are a senior code reviewer and security expert. -You only read and analyze the code — you must never modify any source code files in the repository. -The sole exception is writing your review output into a Markdown file. -You never ask the user what to do next and you produce exactly one review report per run. - -## Output Location - -- Always write your complete review to a file named `agent-code-review.md` in the project root. -- Overwrite the file completely on each run — do not append. -- This file is the only file you may create or modify. -- Do not stage, commit, or push this file. - -## Iterative Review Behavior - -- On each run, treat the task as a fresh review of the currently staged changes. -- Continue reviewing until there are no High or Medium severity issues and no Low severity blockers, then clearly state in the Summary that the code is good to go. +You are a senior code reviewer and security expert. Read and analyze only; never modify a source file. `agent-code-review.md` in the project root is the single file you may write, overwritten completely each run. Do not stage, commit, or push it. Never ask the user what to do next, and produce exactly one report per run. ## Scope and Inputs -- Review only files that are currently staged in Git, not the entire repository. -- Focus on changed lines and minimal necessary surrounding context. -- If information is missing, state reasonable assumptions and proceed. - -## How to Collect Context +Each run is a fresh review of the currently staged files, not the whole repository. Focus on changed lines and the minimum surrounding context. If information is missing, state a reasonable assumption and proceed. - `git diff --staged --unified=0 --no-color` is the primary input; pull `-U3` when a finding needs surrounding context. - Cite line numbers from the `+` side of each hunk so they match the post-merge file. @@ -37,16 +18,7 @@ You never ask the user what to do next and you produce exactly one review report ## Review Policy -Prioritize findings that materially improve: - -- Security, reliability, data integrity, privacy. -- Correctness and performance where clearly impactful. -- Clarity and Clean Code. - -Avoid nitpicks: - -- Do not flag purely stylistic issues unless a project style rule is clearly violated. -- Recommend formatting or lint rules only when they prevent bugs or confusion. +Prioritize what materially improves security, reliability, data integrity, and privacy; correctness and performance where clearly impactful; and clarity. Do not flag purely stylistic issues unless a project style rule is clearly violated, and recommend formatting or lint rules only when they prevent bugs or confusion. ## Severity Definitions @@ -54,20 +26,13 @@ Avoid nitpicks: - **Medium:** likely bugs, race conditions, significant performance or maintainability problems. - **Low:** clarity, naming, minor cleanup. A Low finding is a blocker only when it violates an explicit project rule (lint configuration or a documented convention); otherwise it never blocks the verdict. -## Security Checklist - -- Map each security finding to OWASP Top Ten, e.g., A01 Broken Access Control, A02 Cryptographic Failures, A03 Injection, etc. -- For HTTP APIs, also consider OWASP API Top 10. -- Provide actionable mitigations. +Review until no High or Medium issues and no Low blockers remain, then record the verdict in the Summary. -## Clean Code and Clarity Checks +## What to Check -- Prefer small, focused functions, clear names, elimination of duplication, obvious control flow. -- Suggest local refactors near changed lines. -- Provide minimal viable patches as examples when safe. -- Identify dead code (unused variables, functions, imports, classes). -- Check for DRY violations (repeated logic or patterns that could be abstracted). -- Check for YAGNI violations (unnecessary code, abstractions, or parameters that add complexity without current value). +- **Security:** map each finding to OWASP Top Ten (A01 Broken Access Control, A02 Cryptographic Failures, A03 Injection, etc.), plus OWASP API Top 10 for HTTP APIs. Provide actionable mitigations. +- **Clean code:** small focused functions, clear names, obvious control flow. Suggest local refactors near changed lines, with minimal viable patches as examples when safe. +- **Dead code, DRY, YAGNI:** unused variables, functions, imports, or classes; repeated logic worth abstracting; abstractions or parameters that add complexity without current value. ## Output Format @@ -84,7 +49,7 @@ Write the following structure into `agent-code-review.md`. `N` is the review ite - One paragraph on overall risk and clarity. - Finding counts: High X, Medium Y, Low Z. -- If no High or Medium remain and no Low blockers, state: **Verdict: good to go**. +- If no High or Medium remain and no Low blockers, state: **Verdict: good to go**. Automation depends on detecting this exact string. ## Findings diff --git a/.codex/skills/commitmsg/SKILL.md b/.codex/skills/commitmsg/SKILL.md index e40f81a..b8938db 100644 --- a/.codex/skills/commitmsg/SKILL.md +++ b/.codex/skills/commitmsg/SKILL.md @@ -1,6 +1,6 @@ --- name: commitmsg -description: "Propose a single git commit message for the currently staged changes. Use when asked to write, draft, or suggest a commit message, or to check that a message matches the repo's conventions. Proposes the message only — it does not commit." +description: "Propose a single git commit message for the currently staged changes. Use when asked to write, draft, or suggest a commit message, or to check that a message matches the repo's conventions. Proposes the message only; it does not commit." --- # Commit Message @@ -9,10 +9,10 @@ description: "Propose a single git commit message for the currently staged chang Run these commands to understand the changes: -- `git diff --staged --stat` — the shape of the change; then `git diff --staged` (or per-file diffs) only for files whose purpose isn't clear from the stat -- `git status -s` — staged file list -- `git log -n 20 --oneline` — recent style and to avoid repetition -- `git branch --show-current` — if it contains a ticket ID (e.g. ABC-123), prefix the subject line +- `git diff --staged --stat`: the shape of the change; then `git diff --staged` (or per-file diffs) only for files whose purpose isn't clear from the stat +- `git status -s`: staged file list +- `git log -n 20 --oneline`: recent style and to avoid repetition +- `git branch --show-current`: if it contains a ticket ID (e.g. ABC-123), prefix the subject line ## Rules diff --git a/.codex/skills/dependency-review/SKILL.md b/.codex/skills/dependency-review/SKILL.md index 42afe01..3254de4 100644 --- a/.codex/skills/dependency-review/SKILL.md +++ b/.codex/skills/dependency-review/SKILL.md @@ -1,36 +1,30 @@ --- name: dependency-review -description: "Audit package dependency updates for supply-chain risk: publish-age gate, changelog/diff verification, security advisories, community signals, and breaking changes. Use whenever a branch, PR, or working directory changes a dependency manifest or lockfile — package.json, requirements.txt, pyproject.toml, Gemfile, go.mod, Cargo.toml, pom.xml, build.gradle, composer.json, pubspec.yaml, or the lockfile beside them — including Dependabot/Renovate batches and newly added packages." +description: "Audit package dependency updates for supply-chain risk: publish-age gate, changelog/diff verification, security advisories, community signals, and breaking changes. Use whenever a branch, PR, or working directory changes a dependency manifest or lockfile (package.json, requirements.txt, pyproject.toml, Gemfile, go.mod, Cargo.toml, pom.xml, build.gradle, composer.json, pubspec.yaml, or the lockfile beside them), including Dependabot/Renovate batches and newly added packages." --- # Package Update Supply Chain Review -Review dependency updates to catch supply chain attacks, breaking changes, and risky packages before they land in your codebase. - ## Review Workflow -For each updated or newly added package, work through all five checks below. Prefer CLI and API lookups (`npm view`, `pip index`, `gh api`, `curl` against registry/OSV endpoints) over web browsing — they are faster, cheaper, and available in more environments. Never invent results for a check you could not actually perform: report it as **SKIPPED** with the reason. Present findings in a single summary report at the end, grouped by package. Flag any failing check as a **HOLD** and recommend the team investigate before merging. +For each updated or newly added package, work through all five checks below. Prefer CLI and API lookups (`npm view`, `pip index`, `gh api`, `curl` against registry/OSV endpoints) over web browsing. Never invent a result for a check you could not actually perform: report it as **SKIPPED** with the reason. Present findings in a single summary report at the end, grouped by package, and flag any failing check as a **HOLD** to investigate before merging. ### 1. Publication Age Gate -**Goal:** Confirm the release is at least 7 days old. Compromised or typosquatted releases are usually caught within the first few days, so letting a release "bake" gives the community and automated scanners time to notice. - -**Steps:** +Confirm the release is at least 7 days old. Compromised and typosquatted releases are usually caught within the first few days, so letting one bake gives scanners and the community time to notice. -1. Look up the publish date for the exact version on its registry — e.g. `npm view time --json`, `curl https://pypi.org/pypi///json`, `gem info --remote`, or the registry's web page. -2. If fewer than 7 days have elapsed, flag this as **HOLD - TOO NEW** and include the publish date, the age in days, and a recommendation to wait or pin to the prior version. +1. Look up the publish date for the exact version (`npm view time --json`, `curl https://pypi.org/pypi///json`, or the registry's page). +2. Under 7 days → **HOLD - TOO NEW**, with the publish date, the age in days, and a recommendation to wait or pin to the prior version. ### 2. Changelog and Diff Verification -**Goal:** Confirm the code changes match what the release notes claim. - -**Steps:** +Confirm the code changes match what the release notes claim. -1. Locate the changelog, release notes, or GitHub releases page for the new version (e.g. `gh release view --repo /`). -2. Identify the claimed changes (bug fixes, features, refactors, etc.). -3. Skim the actual source diff between the old and new version (e.g. `gh api repos///compare/v1.2.3...v1.4.0` or the repo's compare view). -4. Look for discrepancies: Are there unexpected new files? New network calls? Obfuscated code? Post-install scripts that were not present before? -5. Pay special attention to install hooks (`preinstall`, `postinstall` in npm; `setup.py` entry points in Python; `build.rs` changes in Rust; etc.) since these execute automatically and are a top vector for supply chain attacks. +1. Locate the changelog or releases page for the new version (`gh release view --repo /`). +2. Identify the claimed changes. +3. Skim the source diff between the old and new version (`gh api repos///compare/v1.2.3...v1.4.0`). +4. Look for discrepancies: unexpected new files, new network calls, obfuscated code, post-install scripts that were not there before. +5. Pay special attention to install hooks (`preinstall`/`postinstall` in npm, `setup.py` entry points in Python, `build.rs` in Rust); they execute automatically and are a top supply chain vector. **Red flags to call out:** @@ -38,53 +32,38 @@ For each updated or newly added package, work through all five checks below. Pre - New outbound HTTP/DNS calls, especially to IP addresses or unusual domains - Environment variable reads for tokens, keys, or credentials - New native/binary dependencies or compiled assets -- Changes to CI config or build scripts that fetch remote resources +- CI config or build script changes that fetch remote resources ### 3. Security Advisory Review -**Goal:** Check whether the package or specific version has known vulnerabilities. - -**Steps:** - -1. Query advisory sources for the package name and version range: `gh api /advisories --method GET -f "affects="`, the OSV API (`curl -s https://api.osv.dev/v1/query -d '{"package":{"name":"","ecosystem":""}}'`), or ecosystem tools (`npm audit`, `pip-audit`, `cargo audit`). -2. Check whether the update itself is a security patch. If so, note the CVE(s) it addresses and confirm the fix is present in the version being adopted. -3. Check whether the new version introduces any new advisories. This can happen when a patch also pulls in a vulnerable transitive dependency. -4. Report findings as: **No known advisories**, **Fixes CVE-XXXX-YYYY (severity)**, or **HOLD - OPEN ADVISORY: CVE-XXXX-YYYY**. +1. Query advisories for the package name and version range: `gh api /advisories --method GET -f "affects="`, the OSV API (`curl -s https://api.osv.dev/v1/query -d '{"package":{"name":"","ecosystem":""}}'`), or ecosystem tools (`npm audit`, `pip-audit`, `cargo audit`). +2. If the update is itself a security patch, note the CVE(s) it addresses and confirm the fix is present in the version being adopted. +3. Check whether the new version introduces new advisories, since a patch can pull in a vulnerable transitive dependency. +4. Report as **No known advisories**, **Fixes CVE-XXXX-YYYY (severity)**, or **HOLD - OPEN ADVISORY: CVE-XXXX-YYYY**. ### 4. Community Signals (best effort) -**Goal:** See if real users are reporting problems, compromises, or regressions with this release. +Look for real users reporting problems, compromises, or regressions with this release. -**Steps:** +1. Check the repo's issues filed after the release date (`gh api "search/issues?q=repo:/+created:>"`). +2. With web access, also search the package name + version on Stack Overflow, Hacker News, and the ecosystem's channels, looking for several people reporting the same crash or suspicious activity. Without web access, limit this check to the CLI/API sources and say so. +3. Compare download counts against the package's historical trend where the registry exposes them; a sudden spike or drop can indicate typosquatting or an abandoned fork. -1. Check the package's GitHub Issues for reports filed after the release date (e.g. `gh api "search/issues?q=repo:/+created:>"` or `gh issue list --repo /`). -2. If you have web access, also search for the package name + version on Stack Overflow, Hacker News, and the ecosystem's community channels; look for patterns such as multiple people reporting the same crash or suspicious activity. -3. Compare download counts against the package's historical trend where the registry exposes them (e.g. `npm view ` plus the npm downloads API). A sudden spike or drop can indicate typosquatting or an abandoned fork. -4. Without web access, limit this check to what the CLI/API sources above can answer and say so. - -**Report as:** A brief summary of community sentiment, "No community issues found for this version" if clean, or **SKIPPED** with the reason if the sources were unreachable. +Report a brief sentiment summary, "No community issues found for this version" if clean, or **SKIPPED** with the reason. ### 5. Breaking Changes and Migration Notes -**Goal:** Identify API or behavioral changes that could break existing code. - -**Steps:** - -1. Check the bump against semver. Breaking changes in a minor or patch release are a red flag on their own — either accidental or a sign of poor maintenance practices. +1. Check the bump against semver. Breaking changes in a minor or patch release are a red flag on their own, either accidental or a sign of poor maintenance. 2. Read the migration guide or upgrade notes if one exists. -3. Look at the diff for: removed or renamed exports, changed function signatures, altered default values, removed configuration options, or dropped support for runtimes/platforms. -4. Search the codebase for usages of any changed or removed APIs. List the files and line numbers that may need updates. -5. Note any changes to the package's peer dependency requirements, minimum runtime versions (Node, Python, Ruby, etc.), or required environment variables. - -**Report as:** +3. Look in the diff for removed or renamed exports, changed function signatures, altered default values, removed configuration options, or dropped runtime/platform support. +4. Search the codebase for usages of anything changed or removed. List the files and line numbers that may need updates. +5. Note changes to peer dependency requirements, minimum runtime versions (Node, Python, Ruby), or required environment variables. -- **No breaking changes** for seamless upgrades. -- **Breaking changes detected** with a list of what changed and which files in the codebase are affected. -- **Potential breaking changes** for behavioral changes that may not cause compile/import errors but could alter runtime behavior (e.g., a default timeout changing from 30s to 5s). +Report as **No breaking changes**; **Breaking changes detected** with what changed and which files are affected; or **Potential breaking changes** for behavioral shifts that compile and import fine but alter runtime behavior (e.g. a default timeout dropping from 30s to 5s). ## Output Format -Present the full review as a structured report. Here is the template: +Present the full review as a structured report: ```text # Package Update Review @@ -115,13 +94,11 @@ Present the full review as a structured report. Here is the template: ## Edge Cases -- **New dependencies** (not just version bumps): Apply the same five checks but also verify the package is the intended one (check for typosquatting by comparing to similarly named popular packages) and review its overall maintenance health (last commit date, number of maintainers, bus factor). -- **Lockfile-only changes** with no manifest change: These can happen from transitive dependency resolution. Still review the transitive packages that changed, though a lighter touch is acceptable for patch-level transitive bumps in well-known packages. -- **Monorepos with many packages:** Group related packages (e.g., `@babel/*` or `@angular/*`) and note that they are part of a coordinated release, which reduces (but does not eliminate) the need for individual diff review. -- **Private/internal packages:** The publication age gate may not apply, but the diff verification and breaking change checks still do. +- **New dependencies** (not just version bumps): same five checks, plus verify it is the intended package (compare against similarly named popular ones for typosquatting) and review maintenance health: last commit date, maintainer count, bus factor. +- **Lockfile-only changes** with no manifest change: still review the transitive packages that changed, though a lighter touch is acceptable for patch-level bumps in well-known packages. +- **Monorepo groups** (`@babel/*`, `@angular/*`): treat as one coordinated release, which reduces but does not eliminate individual diff review. +- **Private/internal packages:** the publication age gate may not apply; diff verification and breaking change checks still do. Never send internal package names or versions to public registry or advisory endpoints. Query the private registry where it exposes an API, otherwise mark those checks **SKIPPED** with the reason. -## Tips for Efficiency +## Priorities -- Start with the publication age gate; it is the fastest check and can immediately flag the riskiest updates. -- For large dependency updates (e.g., Dependabot batches), prioritize direct dependencies over transitive ones, and prioritize packages with install hooks. -- If a package has hundreds of thousands of weekly downloads and is maintained by a well-known org (e.g., Meta, Google, Vercel), the changelog and community checks can be lighter. But never skip the security advisory check. +Start with the publication age gate; it is the fastest check and immediately flags the riskiest updates. In large batches (Dependabot/Renovate), prioritize direct dependencies over transitive ones, and packages with install hooks over those without. For high-download packages from well-known orgs, the changelog and community checks can be lighter; never skip the security advisory check. diff --git a/.codex/skills/efficient-orchestration/SKILL.md b/.codex/skills/efficient-orchestration/SKILL.md index 4e56a5d..453d903 100644 --- a/.codex/skills/efficient-orchestration/SKILL.md +++ b/.codex/skills/efficient-orchestration/SKILL.md @@ -1,6 +1,6 @@ --- name: efficient-orchestration -description: "Run this task with your current model orchestrating while cheaper subagents do the token-heavy research, coding, and testing. Use for work that is large, parallelizable, or token-hungry — broad repo scans, long logs, wide test or browser passes, repetitive edits — or when the user asks to conserve usage limits. Skip it for small, sequential, or judgment-dense tasks." +description: "Run this task with your current model orchestrating while cheaper subagents do the token-heavy research, coding, and testing. Use for work that is large, parallelizable, or token-hungry (broad repo scans, long logs, wide test or browser passes, repetitive edits), or when the user asks to conserve usage limits. Skip it for small, sequential, or judgment-dense tasks." --- # Efficient Orchestration @@ -21,15 +21,15 @@ Order the models your harness can run from cheapest to most capable (e.g. on Cla ## Pick each subagent's model -By task difficulty, not task type — and never above your own tier: +By task difficulty, not task type, and never above your own tier: -- **Cheapest tier:** mechanical, high-volume, low-judgment work — search sweeps, inventory, log reduction, simple edits. +- **Cheapest tier:** mechanical, high-volume, low-judgment work: search sweeps, inventory, log reduction, simple edits. - **Mid tier (default):** focused research, routine or narrow patches, test runs, straightforward debugging. -- **Your tier:** complex work delegated for parallelism or context isolation, not savings — intricate refactors, multi-file features, subtle bugs, design exploration. +- **Your tier:** complex work delegated for parallelism or context isolation, not savings: intricate refactors, multi-file features, subtle bugs, design exploration. -Start at the cheapest tier that can plausibly succeed; after two failures at a tier, escalate one tier or take the work back — never a third retry at the same tier. +Start at the cheapest tier that can plausibly succeed; after two failures at a tier, escalate one tier or take the work back; never a third retry at the same tier. -Pin an explicit model on every spawn, and where the harness offers stable aliases (e.g. Claude's `haiku`/`sonnet`/`opus`), prefer them over dated full IDs — aliases survive model rotations; pinned IDs hard-fail. Don't assume built-in subagents are cheap: some harnesses default them to inheriting your main-session model (Claude Code's built-in Explore/Plan/general-purpose do; a per-invocation model overrides it, and a user-level `Explore` agent with `model: haiku` catches the spontaneous searches you don't route). Where the harness supports per-agent reasoning effort, run cheap-tier recon and mechanical work at low effort — current-generation low roughly matches previous-generation highest. +Pin an explicit model on every spawn, preferring your harness's stable aliases (Claude's `haiku`/`sonnet`/`opus`; check your own tool's model list for equivalents) over dated full IDs, since aliases survive model rotations while pinned IDs hard-fail. Don't assume built-in subagents are cheap: some harnesses default them to inheriting your main-session model, so read the agent definitions and set a per-invocation model where they do. On Claude Code, the built-in Explore/Plan/general-purpose agents inherit, and a user-level `Explore` agent with `model: haiku` catches the spontaneous searches you don't route. Where the harness supports per-agent reasoning effort, run cheap-tier recon and mechanical work at low effort. ## Run it @@ -41,18 +41,18 @@ Pin an explicit model on every spawn, and where the harness offers stable aliase ## Usage limits -- Know what delegation buys. On pay-per-token APIs, cheaper tiers cut real dollars. On subscriptions, most cost is context reprocessing, and each subagent rebuilds context whose findings flow back into yours — total tokens can rise while the binding quota falls. The durable wins are bucket arbitrage (e.g. Claude's separate Sonnet-only weekly allowance, while the frontier tier drains the shared bucket fastest), wall-clock parallelism, and a lean main context. +- On pay-per-token APIs, cheaper tiers cut real dollars. On subscriptions, each subagent rebuilds context whose findings flow back into yours, so total tokens can rise while the binding quota falls; the durable wins there are bucket arbitrage (e.g. Claude's separate Sonnet-only weekly allowance, while the frontier tier drains the shared bucket fastest), wall-clock parallelism, and a lean main context. - Delegate in bounded waves (~3 parallel); between waves check the harness's usage surface (on Claude Code, `npx -y ccusage@latest blocks --active --json`; elsewhere a usage/status command if one exists, or treat the first rate-limit error as the cap). Stop launching once any usage window nears ~95%; let in-flight work finish. -- For long unattended runs, pause at the cap and resume when it clears: finish the wave, then use the harness's scheduled-wakeup primitive, chaining wakeups of ≤3600s until the window clears — never busy-wait with `sleep`. If no such primitive exists, write a self-contained resume prompt to a handoff file (remaining plan, the 95% rule, the exact usage command and its last reading, subagent handoffs) and tell the user to relaunch with it. On resume, re-verify with the usage command — a fresh block timestamp, not elapsed wall-clock, proves rollover. Tell the user which window tripped, the observed %, the next check time, and the outstanding work. +- For long unattended runs, pause at the cap and resume when it clears: finish the wave, then use the harness's scheduled-wakeup primitive, chaining wakeups of ≤3600s until the window clears, never busy-waiting with `sleep`. If no such primitive exists, write a self-contained resume prompt to a handoff file (remaining plan, the 95% rule, the exact usage command and its last reading, subagent handoffs) and tell the user to relaunch with it. On resume, re-verify with the usage command: a fresh block timestamp, not elapsed wall-clock, proves rollover. Tell the user which window tripped, the observed %, the next check time, and the outstanding work. ## Vet results -Reports are leads, not facts. Before acting on a high-impact finding, opening a PR, or claiming done: reopen key cited files, confirm line refs and failures, review the final diff, and resolve subagent disagreements yourself. For non-trivial completed work, spawn a fresh-context verifier on your tier that only tries to refute the claim — rerun the tests, drive the affected flow, probe edge cases — and never fixes anything; independent refutation beats self-review. +Reports are leads, not facts. Before acting on a high-impact finding, opening a PR, or claiming done: reopen key cited files, confirm line refs and failures, review the final diff, and resolve subagent disagreements yourself. For non-trivial completed work, spawn a fresh-context verifier on your tier that only tries to refute the claim (rerun the tests, drive the affected flow, probe edge cases) and never fixes anything; independent refutation beats self-review. ## Guardrails - Don't delegate a blocker your next step needs. -- Don't let two subagents edit the same files, and don't implement slices workers own — coordination plus duplicated implementation costs more than either alone. +- Don't let two subagents edit the same files, and don't implement slices workers own; coordination plus duplicated implementation costs more than either alone. - Enforce read-only roles (recon, review, verification) by tool allowlist where the harness supports it, not prompt text. -- Route security-sensitive work (authn/authz, secrets, crypto, hardening) to a capable non-frontier tier — frontier safety classifiers can refuse benign defensive work mid-task. +- Route security-sensitive work (authn/authz, secrets, crypto, hardening) to a capable non-frontier tier; frontier safety classifiers can refuse benign defensive work mid-task. - If the task is tiny, doesn't parallelize, or needs delicate judgment throughout, skip the ceremony and do it yourself. diff --git a/.codex/skills/review-pr/SKILL.md b/.codex/skills/review-pr/SKILL.md index 27576cf..a7dbad5 100644 --- a/.codex/skills/review-pr/SKILL.md +++ b/.codex/skills/review-pr/SKILL.md @@ -45,15 +45,15 @@ gh api graphql --paginate --slurp \ **Auto-resolve:** If a thread's first comment body matches any `$IGNORED_FILE` entry (`grep -qxF`), resolve via `resolveReviewThread` mutation without classifying. -If unresolved threads remain → step 3. Do NOT re-request a bot review while threads are still open — process existing feedback first. Only when zero unresolved threads remain → step 5. +If unresolved threads remain → step 3. Do NOT re-request a bot review while threads are still open; process existing feedback first. Only when zero unresolved threads remain → step 5. ### 3. Classify and resolve Read referenced file + context for each remaining thread, then classify: -- **Already addressed / Informational / Inaccurate** — append body to `$IGNORED_FILE`, resolve (reply with brief explanation if inaccurate). -- **Valid fix** — implement minimal change. Must meet ALL: (1) fixes a real bug — wrong behavior, data loss, security, crash, or race condition; (2) net-simpler or complexity-neutral; (3) concrete, not speculative. -- **Nitpick / Low-value** — resolve WITHOUT implementing. Includes: style preferences not enforced by linter, docstring suggestions on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append body to `$IGNORED_FILE`, reply with one-line rationale, resolve. +- **Already addressed / Informational / Inaccurate**: append body to `$IGNORED_FILE`, resolve (reply with brief explanation if inaccurate). +- **Valid fix**: implement minimal change. Must meet ALL: (1) fixes a real bug (wrong behavior, data loss, security, crash, or race condition); (2) net-simpler or complexity-neutral; (3) concrete, not speculative. +- **Nitpick / Low-value**: resolve WITHOUT implementing. Includes: style preferences not enforced by linter, docstring suggestions on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append body to `$IGNORED_FILE`, reply with one-line rationale, resolve. ### 4. Push fixes @@ -61,34 +61,52 @@ Stage, commit (`fix:`/`refactor:`/etc.), push, verify CI green, resolve fixed th ### 5. Ensure bot review covers latest commit -Only reached when zero unresolved threads remain. Get HEAD SHA: `gh pr view {PR_NUMBER} --json commits --jq '.commits[-1].oid'`. Bots = logins ending in `[bot]`. Fetch their latest reviews: +Each bot's latest review, and the commit it covers: ```bash -gh api repos/{owner}/{repo}/pulls/{PR_NUMBER}/reviews \ - --jq '[.[] | select(.user.login | endswith("[bot]"))] | group_by(.user.login) | map(max_by(.submitted_at))' +head_sha=$(gh pr view {PR_NUMBER} --json commits --jq '.commits[-1].oid') + +latest() { gh api --paginate --slurp repos/{owner}/{repo}/pulls/{PR_NUMBER}/reviews \ + | jq -r 'add | [.[] | select(.user.login | endswith("[bot]"))] | group_by(.user.login) + | map(max_by(.submitted_at)) | .[] | "\(.user.login) \(.commit_id)"'; } + +stale=$(latest | grep -v " $head_sha$" | cut -d' ' -f1 | sort -u) ``` -If a bot's latest review already covers HEAD → success. Stop. +`/reviews` alone identifies the review bots; CI and deploy bots never appear there. `--slurp` piped to `jq`, not `--jq`: under `--paginate` a `--jq` filter runs per page, so `max_by` returns a per-page max and lists a long-running PR's bots twice. + +Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise re-trigger each login in `stale`; they do not re-review a push on their own. + +| Bot | Login | Re-trigger with | +| --- | --- | --- | +| Copilot | `copilot-pull-request-reviewer[bot]` | `gh pr edit {PR_NUMBER} --add-reviewer @copilot` | +| CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | +| Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -Otherwise, re-request and poll (first check 8 min, timeout 15 min, poll 60 s). `gh pr edit --add-reviewer` re-requests reviews from existing bot reviewers — do not skip this. +- Pass the literal `@copilot`; its raw `[bot]` login can exit 0 having requested nothing. Confirm Copilot specifically, not just that some reviewer is pending: `gh api repos/{owner}/{repo}/pulls/{PR_NUMBER} --jq '.requested_reviewers[].login' | grep -qiE '^(Copilot|copilot-pull-request-reviewer\[bot\])$'`. A miss means it did not take, and the poll below would burn its full timeout waiting. Match both spellings: `requested_reviewers` returns the login as `Copilot`, while the review it later submits carries `copilot-pull-request-reviewer[bot]`, so checking only the `[bot]` form reports failure on every successful request. +- App-based bots (CodeRabbit, Greptile) cannot be requested as reviewers at all; a mention is their only trigger. `@coderabbitai full review` re-reviews the whole diff rather than just new commits. +- **Bot not in the table, or none found** → ask the user for the exact trigger. Never guess a mention string: a wrong one posts a visible no-op comment. + +Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired, one per line, dropping any you could not trigger: ```bash -bot_logins=$(gh api repos/{owner}/{repo}/pulls/{PR_NUMBER}/reviews \ - --jq '[.[] | select(.user.login | endswith("[bot]")) | .user.login] | unique | .[]') +triggered="$stale" # minus any bot you could not trigger -for bot in $bot_logins; do - gh pr edit {PR_NUMBER} --add-reviewer "$bot" -done +# Never poll on an empty set: comm would report nothing pending and the loop +# would break on the first pass, declaring success without waiting. +[ -n "$triggered" ] || { echo "nothing was triggered"; exit 1; } end=$((SECONDS+900)); sleep 480 while [ $SECONDS -lt $end ]; do - commit_id=$(gh api repos/{owner}/{repo}/pulls/{PR_NUMBER}/reviews \ - --jq '[.[] | select(.user.login=="{bot}")] | max_by(.submitted_at) | .commit_id') - [ "$commit_id" = "$head_sha" ] && break + pending=$(comm -23 <(printf '%s\n' "$triggered" | sort -u) \ + <(latest | grep " $head_sha$" | cut -d' ' -f1 | sort -u)) + [ -z "$pending" ] && break sleep 60 done ``` -Timeout → tell user to re-run this command and stop. Success → go back to step 2. +Run both blocks in one shell: `head_sha` and `latest` do not survive separate tool calls. If your harness blocks foreground `sleep`, run the whole wait as one backgrounded command rather than sleeping between tool calls. + +Timeout → name the bots still pending, tell user to re-run this command, stop. Success → go back to step 2. -Declare success when step 2 finds zero unresolved threads AND step 5 confirms a bot review on HEAD. Stop at iteration 5. Report: threads resolved, fixes made, threads auto-ignored, threads remaining, CI status. +Stop at iteration 5. Report: threads resolved, fixes made, threads auto-ignored, threads remaining, CI status. diff --git a/.copilot/skills/code-refinement/SKILL.md b/.copilot/skills/code-refinement/SKILL.md index c1c3882..bcb5593 100644 --- a/.copilot/skills/code-refinement/SKILL.md +++ b/.copilot/skills/code-refinement/SKILL.md @@ -1,6 +1,6 @@ --- name: code-refinement -description: "Review staged files for code quality (KISS, DRY, YAGNI, Clean Code) and fix linting issues. Use to clean up staged work before review or commit — unlike code-review, this one edits the code: it applies refactors, runs the linter, and fills test gaps." +description: "Review staged files for code quality (KISS, DRY, YAGNI, Clean Code) and fix linting issues. Use to clean up staged work before review or commit. Unlike code-review, this one edits the code: it applies refactors, runs the linter, and fills test gaps." --- # Code Refinement diff --git a/.copilot/skills/code-review/SKILL.md b/.copilot/skills/code-review/SKILL.md index 2c62780..4c0d5d9 100644 --- a/.copilot/skills/code-review/SKILL.md +++ b/.copilot/skills/code-review/SKILL.md @@ -5,30 +5,11 @@ description: "Review staged changes for security, correctness, performance, and # Role -You are a senior code reviewer and security expert. -You only read and analyze the code — you must never modify any source code files in the repository. -The sole exception is writing your review output into a Markdown file. -You never ask the user what to do next and you produce exactly one review report per run. - -## Output Location - -- Always write your complete review to a file named `agent-code-review.md` in the project root. -- Overwrite the file completely on each run — do not append. -- This file is the only file you may create or modify. -- Do not stage, commit, or push this file. - -## Iterative Review Behavior - -- On each run, treat the task as a fresh review of the currently staged changes. -- Continue reviewing until there are no High or Medium severity issues and no Low severity blockers, then clearly state in the Summary that the code is good to go. +You are a senior code reviewer and security expert. Read and analyze only; never modify a source file. `agent-code-review.md` in the project root is the single file you may write, overwritten completely each run. Do not stage, commit, or push it. Never ask the user what to do next, and produce exactly one report per run. ## Scope and Inputs -- Review only files that are currently staged in Git, not the entire repository. -- Focus on changed lines and minimal necessary surrounding context. -- If information is missing, state reasonable assumptions and proceed. - -## How to Collect Context +Each run is a fresh review of the currently staged files, not the whole repository. Focus on changed lines and the minimum surrounding context. If information is missing, state a reasonable assumption and proceed. - `git diff --staged --unified=0 --no-color` is the primary input; pull `-U3` when a finding needs surrounding context. - Cite line numbers from the `+` side of each hunk so they match the post-merge file. @@ -37,16 +18,7 @@ You never ask the user what to do next and you produce exactly one review report ## Review Policy -Prioritize findings that materially improve: - -- Security, reliability, data integrity, privacy. -- Correctness and performance where clearly impactful. -- Clarity and Clean Code. - -Avoid nitpicks: - -- Do not flag purely stylistic issues unless a project style rule is clearly violated. -- Recommend formatting or lint rules only when they prevent bugs or confusion. +Prioritize what materially improves security, reliability, data integrity, and privacy; correctness and performance where clearly impactful; and clarity. Do not flag purely stylistic issues unless a project style rule is clearly violated, and recommend formatting or lint rules only when they prevent bugs or confusion. ## Severity Definitions @@ -54,20 +26,13 @@ Avoid nitpicks: - **Medium:** likely bugs, race conditions, significant performance or maintainability problems. - **Low:** clarity, naming, minor cleanup. A Low finding is a blocker only when it violates an explicit project rule (lint configuration or a documented convention); otherwise it never blocks the verdict. -## Security Checklist - -- Map each security finding to OWASP Top Ten, e.g., A01 Broken Access Control, A02 Cryptographic Failures, A03 Injection, etc. -- For HTTP APIs, also consider OWASP API Top 10. -- Provide actionable mitigations. +Review until no High or Medium issues and no Low blockers remain, then record the verdict in the Summary. -## Clean Code and Clarity Checks +## What to Check -- Prefer small, focused functions, clear names, elimination of duplication, obvious control flow. -- Suggest local refactors near changed lines. -- Provide minimal viable patches as examples when safe. -- Identify dead code (unused variables, functions, imports, classes). -- Check for DRY violations (repeated logic or patterns that could be abstracted). -- Check for YAGNI violations (unnecessary code, abstractions, or parameters that add complexity without current value). +- **Security:** map each finding to OWASP Top Ten (A01 Broken Access Control, A02 Cryptographic Failures, A03 Injection, etc.), plus OWASP API Top 10 for HTTP APIs. Provide actionable mitigations. +- **Clean code:** small focused functions, clear names, obvious control flow. Suggest local refactors near changed lines, with minimal viable patches as examples when safe. +- **Dead code, DRY, YAGNI:** unused variables, functions, imports, or classes; repeated logic worth abstracting; abstractions or parameters that add complexity without current value. ## Output Format @@ -84,7 +49,7 @@ Write the following structure into `agent-code-review.md`. `N` is the review ite - One paragraph on overall risk and clarity. - Finding counts: High X, Medium Y, Low Z. -- If no High or Medium remain and no Low blockers, state: **Verdict: good to go**. +- If no High or Medium remain and no Low blockers, state: **Verdict: good to go**. Automation depends on detecting this exact string. ## Findings diff --git a/.copilot/skills/commitmsg/SKILL.md b/.copilot/skills/commitmsg/SKILL.md index e40f81a..b8938db 100644 --- a/.copilot/skills/commitmsg/SKILL.md +++ b/.copilot/skills/commitmsg/SKILL.md @@ -1,6 +1,6 @@ --- name: commitmsg -description: "Propose a single git commit message for the currently staged changes. Use when asked to write, draft, or suggest a commit message, or to check that a message matches the repo's conventions. Proposes the message only — it does not commit." +description: "Propose a single git commit message for the currently staged changes. Use when asked to write, draft, or suggest a commit message, or to check that a message matches the repo's conventions. Proposes the message only; it does not commit." --- # Commit Message @@ -9,10 +9,10 @@ description: "Propose a single git commit message for the currently staged chang Run these commands to understand the changes: -- `git diff --staged --stat` — the shape of the change; then `git diff --staged` (or per-file diffs) only for files whose purpose isn't clear from the stat -- `git status -s` — staged file list -- `git log -n 20 --oneline` — recent style and to avoid repetition -- `git branch --show-current` — if it contains a ticket ID (e.g. ABC-123), prefix the subject line +- `git diff --staged --stat`: the shape of the change; then `git diff --staged` (or per-file diffs) only for files whose purpose isn't clear from the stat +- `git status -s`: staged file list +- `git log -n 20 --oneline`: recent style and to avoid repetition +- `git branch --show-current`: if it contains a ticket ID (e.g. ABC-123), prefix the subject line ## Rules diff --git a/.copilot/skills/dependency-review/SKILL.md b/.copilot/skills/dependency-review/SKILL.md index 42afe01..3254de4 100644 --- a/.copilot/skills/dependency-review/SKILL.md +++ b/.copilot/skills/dependency-review/SKILL.md @@ -1,36 +1,30 @@ --- name: dependency-review -description: "Audit package dependency updates for supply-chain risk: publish-age gate, changelog/diff verification, security advisories, community signals, and breaking changes. Use whenever a branch, PR, or working directory changes a dependency manifest or lockfile — package.json, requirements.txt, pyproject.toml, Gemfile, go.mod, Cargo.toml, pom.xml, build.gradle, composer.json, pubspec.yaml, or the lockfile beside them — including Dependabot/Renovate batches and newly added packages." +description: "Audit package dependency updates for supply-chain risk: publish-age gate, changelog/diff verification, security advisories, community signals, and breaking changes. Use whenever a branch, PR, or working directory changes a dependency manifest or lockfile (package.json, requirements.txt, pyproject.toml, Gemfile, go.mod, Cargo.toml, pom.xml, build.gradle, composer.json, pubspec.yaml, or the lockfile beside them), including Dependabot/Renovate batches and newly added packages." --- # Package Update Supply Chain Review -Review dependency updates to catch supply chain attacks, breaking changes, and risky packages before they land in your codebase. - ## Review Workflow -For each updated or newly added package, work through all five checks below. Prefer CLI and API lookups (`npm view`, `pip index`, `gh api`, `curl` against registry/OSV endpoints) over web browsing — they are faster, cheaper, and available in more environments. Never invent results for a check you could not actually perform: report it as **SKIPPED** with the reason. Present findings in a single summary report at the end, grouped by package. Flag any failing check as a **HOLD** and recommend the team investigate before merging. +For each updated or newly added package, work through all five checks below. Prefer CLI and API lookups (`npm view`, `pip index`, `gh api`, `curl` against registry/OSV endpoints) over web browsing. Never invent a result for a check you could not actually perform: report it as **SKIPPED** with the reason. Present findings in a single summary report at the end, grouped by package, and flag any failing check as a **HOLD** to investigate before merging. ### 1. Publication Age Gate -**Goal:** Confirm the release is at least 7 days old. Compromised or typosquatted releases are usually caught within the first few days, so letting a release "bake" gives the community and automated scanners time to notice. - -**Steps:** +Confirm the release is at least 7 days old. Compromised and typosquatted releases are usually caught within the first few days, so letting one bake gives scanners and the community time to notice. -1. Look up the publish date for the exact version on its registry — e.g. `npm view time --json`, `curl https://pypi.org/pypi///json`, `gem info --remote`, or the registry's web page. -2. If fewer than 7 days have elapsed, flag this as **HOLD - TOO NEW** and include the publish date, the age in days, and a recommendation to wait or pin to the prior version. +1. Look up the publish date for the exact version (`npm view time --json`, `curl https://pypi.org/pypi///json`, or the registry's page). +2. Under 7 days → **HOLD - TOO NEW**, with the publish date, the age in days, and a recommendation to wait or pin to the prior version. ### 2. Changelog and Diff Verification -**Goal:** Confirm the code changes match what the release notes claim. - -**Steps:** +Confirm the code changes match what the release notes claim. -1. Locate the changelog, release notes, or GitHub releases page for the new version (e.g. `gh release view --repo /`). -2. Identify the claimed changes (bug fixes, features, refactors, etc.). -3. Skim the actual source diff between the old and new version (e.g. `gh api repos///compare/v1.2.3...v1.4.0` or the repo's compare view). -4. Look for discrepancies: Are there unexpected new files? New network calls? Obfuscated code? Post-install scripts that were not present before? -5. Pay special attention to install hooks (`preinstall`, `postinstall` in npm; `setup.py` entry points in Python; `build.rs` changes in Rust; etc.) since these execute automatically and are a top vector for supply chain attacks. +1. Locate the changelog or releases page for the new version (`gh release view --repo /`). +2. Identify the claimed changes. +3. Skim the source diff between the old and new version (`gh api repos///compare/v1.2.3...v1.4.0`). +4. Look for discrepancies: unexpected new files, new network calls, obfuscated code, post-install scripts that were not there before. +5. Pay special attention to install hooks (`preinstall`/`postinstall` in npm, `setup.py` entry points in Python, `build.rs` in Rust); they execute automatically and are a top supply chain vector. **Red flags to call out:** @@ -38,53 +32,38 @@ For each updated or newly added package, work through all five checks below. Pre - New outbound HTTP/DNS calls, especially to IP addresses or unusual domains - Environment variable reads for tokens, keys, or credentials - New native/binary dependencies or compiled assets -- Changes to CI config or build scripts that fetch remote resources +- CI config or build script changes that fetch remote resources ### 3. Security Advisory Review -**Goal:** Check whether the package or specific version has known vulnerabilities. - -**Steps:** - -1. Query advisory sources for the package name and version range: `gh api /advisories --method GET -f "affects="`, the OSV API (`curl -s https://api.osv.dev/v1/query -d '{"package":{"name":"","ecosystem":""}}'`), or ecosystem tools (`npm audit`, `pip-audit`, `cargo audit`). -2. Check whether the update itself is a security patch. If so, note the CVE(s) it addresses and confirm the fix is present in the version being adopted. -3. Check whether the new version introduces any new advisories. This can happen when a patch also pulls in a vulnerable transitive dependency. -4. Report findings as: **No known advisories**, **Fixes CVE-XXXX-YYYY (severity)**, or **HOLD - OPEN ADVISORY: CVE-XXXX-YYYY**. +1. Query advisories for the package name and version range: `gh api /advisories --method GET -f "affects="`, the OSV API (`curl -s https://api.osv.dev/v1/query -d '{"package":{"name":"","ecosystem":""}}'`), or ecosystem tools (`npm audit`, `pip-audit`, `cargo audit`). +2. If the update is itself a security patch, note the CVE(s) it addresses and confirm the fix is present in the version being adopted. +3. Check whether the new version introduces new advisories, since a patch can pull in a vulnerable transitive dependency. +4. Report as **No known advisories**, **Fixes CVE-XXXX-YYYY (severity)**, or **HOLD - OPEN ADVISORY: CVE-XXXX-YYYY**. ### 4. Community Signals (best effort) -**Goal:** See if real users are reporting problems, compromises, or regressions with this release. +Look for real users reporting problems, compromises, or regressions with this release. -**Steps:** +1. Check the repo's issues filed after the release date (`gh api "search/issues?q=repo:/+created:>"`). +2. With web access, also search the package name + version on Stack Overflow, Hacker News, and the ecosystem's channels, looking for several people reporting the same crash or suspicious activity. Without web access, limit this check to the CLI/API sources and say so. +3. Compare download counts against the package's historical trend where the registry exposes them; a sudden spike or drop can indicate typosquatting or an abandoned fork. -1. Check the package's GitHub Issues for reports filed after the release date (e.g. `gh api "search/issues?q=repo:/+created:>"` or `gh issue list --repo /`). -2. If you have web access, also search for the package name + version on Stack Overflow, Hacker News, and the ecosystem's community channels; look for patterns such as multiple people reporting the same crash or suspicious activity. -3. Compare download counts against the package's historical trend where the registry exposes them (e.g. `npm view ` plus the npm downloads API). A sudden spike or drop can indicate typosquatting or an abandoned fork. -4. Without web access, limit this check to what the CLI/API sources above can answer and say so. - -**Report as:** A brief summary of community sentiment, "No community issues found for this version" if clean, or **SKIPPED** with the reason if the sources were unreachable. +Report a brief sentiment summary, "No community issues found for this version" if clean, or **SKIPPED** with the reason. ### 5. Breaking Changes and Migration Notes -**Goal:** Identify API or behavioral changes that could break existing code. - -**Steps:** - -1. Check the bump against semver. Breaking changes in a minor or patch release are a red flag on their own — either accidental or a sign of poor maintenance practices. +1. Check the bump against semver. Breaking changes in a minor or patch release are a red flag on their own, either accidental or a sign of poor maintenance. 2. Read the migration guide or upgrade notes if one exists. -3. Look at the diff for: removed or renamed exports, changed function signatures, altered default values, removed configuration options, or dropped support for runtimes/platforms. -4. Search the codebase for usages of any changed or removed APIs. List the files and line numbers that may need updates. -5. Note any changes to the package's peer dependency requirements, minimum runtime versions (Node, Python, Ruby, etc.), or required environment variables. - -**Report as:** +3. Look in the diff for removed or renamed exports, changed function signatures, altered default values, removed configuration options, or dropped runtime/platform support. +4. Search the codebase for usages of anything changed or removed. List the files and line numbers that may need updates. +5. Note changes to peer dependency requirements, minimum runtime versions (Node, Python, Ruby), or required environment variables. -- **No breaking changes** for seamless upgrades. -- **Breaking changes detected** with a list of what changed and which files in the codebase are affected. -- **Potential breaking changes** for behavioral changes that may not cause compile/import errors but could alter runtime behavior (e.g., a default timeout changing from 30s to 5s). +Report as **No breaking changes**; **Breaking changes detected** with what changed and which files are affected; or **Potential breaking changes** for behavioral shifts that compile and import fine but alter runtime behavior (e.g. a default timeout dropping from 30s to 5s). ## Output Format -Present the full review as a structured report. Here is the template: +Present the full review as a structured report: ```text # Package Update Review @@ -115,13 +94,11 @@ Present the full review as a structured report. Here is the template: ## Edge Cases -- **New dependencies** (not just version bumps): Apply the same five checks but also verify the package is the intended one (check for typosquatting by comparing to similarly named popular packages) and review its overall maintenance health (last commit date, number of maintainers, bus factor). -- **Lockfile-only changes** with no manifest change: These can happen from transitive dependency resolution. Still review the transitive packages that changed, though a lighter touch is acceptable for patch-level transitive bumps in well-known packages. -- **Monorepos with many packages:** Group related packages (e.g., `@babel/*` or `@angular/*`) and note that they are part of a coordinated release, which reduces (but does not eliminate) the need for individual diff review. -- **Private/internal packages:** The publication age gate may not apply, but the diff verification and breaking change checks still do. +- **New dependencies** (not just version bumps): same five checks, plus verify it is the intended package (compare against similarly named popular ones for typosquatting) and review maintenance health: last commit date, maintainer count, bus factor. +- **Lockfile-only changes** with no manifest change: still review the transitive packages that changed, though a lighter touch is acceptable for patch-level bumps in well-known packages. +- **Monorepo groups** (`@babel/*`, `@angular/*`): treat as one coordinated release, which reduces but does not eliminate individual diff review. +- **Private/internal packages:** the publication age gate may not apply; diff verification and breaking change checks still do. Never send internal package names or versions to public registry or advisory endpoints. Query the private registry where it exposes an API, otherwise mark those checks **SKIPPED** with the reason. -## Tips for Efficiency +## Priorities -- Start with the publication age gate; it is the fastest check and can immediately flag the riskiest updates. -- For large dependency updates (e.g., Dependabot batches), prioritize direct dependencies over transitive ones, and prioritize packages with install hooks. -- If a package has hundreds of thousands of weekly downloads and is maintained by a well-known org (e.g., Meta, Google, Vercel), the changelog and community checks can be lighter. But never skip the security advisory check. +Start with the publication age gate; it is the fastest check and immediately flags the riskiest updates. In large batches (Dependabot/Renovate), prioritize direct dependencies over transitive ones, and packages with install hooks over those without. For high-download packages from well-known orgs, the changelog and community checks can be lighter; never skip the security advisory check. diff --git a/.copilot/skills/efficient-orchestration/SKILL.md b/.copilot/skills/efficient-orchestration/SKILL.md index 4e56a5d..453d903 100644 --- a/.copilot/skills/efficient-orchestration/SKILL.md +++ b/.copilot/skills/efficient-orchestration/SKILL.md @@ -1,6 +1,6 @@ --- name: efficient-orchestration -description: "Run this task with your current model orchestrating while cheaper subagents do the token-heavy research, coding, and testing. Use for work that is large, parallelizable, or token-hungry — broad repo scans, long logs, wide test or browser passes, repetitive edits — or when the user asks to conserve usage limits. Skip it for small, sequential, or judgment-dense tasks." +description: "Run this task with your current model orchestrating while cheaper subagents do the token-heavy research, coding, and testing. Use for work that is large, parallelizable, or token-hungry (broad repo scans, long logs, wide test or browser passes, repetitive edits), or when the user asks to conserve usage limits. Skip it for small, sequential, or judgment-dense tasks." --- # Efficient Orchestration @@ -21,15 +21,15 @@ Order the models your harness can run from cheapest to most capable (e.g. on Cla ## Pick each subagent's model -By task difficulty, not task type — and never above your own tier: +By task difficulty, not task type, and never above your own tier: -- **Cheapest tier:** mechanical, high-volume, low-judgment work — search sweeps, inventory, log reduction, simple edits. +- **Cheapest tier:** mechanical, high-volume, low-judgment work: search sweeps, inventory, log reduction, simple edits. - **Mid tier (default):** focused research, routine or narrow patches, test runs, straightforward debugging. -- **Your tier:** complex work delegated for parallelism or context isolation, not savings — intricate refactors, multi-file features, subtle bugs, design exploration. +- **Your tier:** complex work delegated for parallelism or context isolation, not savings: intricate refactors, multi-file features, subtle bugs, design exploration. -Start at the cheapest tier that can plausibly succeed; after two failures at a tier, escalate one tier or take the work back — never a third retry at the same tier. +Start at the cheapest tier that can plausibly succeed; after two failures at a tier, escalate one tier or take the work back; never a third retry at the same tier. -Pin an explicit model on every spawn, and where the harness offers stable aliases (e.g. Claude's `haiku`/`sonnet`/`opus`), prefer them over dated full IDs — aliases survive model rotations; pinned IDs hard-fail. Don't assume built-in subagents are cheap: some harnesses default them to inheriting your main-session model (Claude Code's built-in Explore/Plan/general-purpose do; a per-invocation model overrides it, and a user-level `Explore` agent with `model: haiku` catches the spontaneous searches you don't route). Where the harness supports per-agent reasoning effort, run cheap-tier recon and mechanical work at low effort — current-generation low roughly matches previous-generation highest. +Pin an explicit model on every spawn, preferring your harness's stable aliases (Claude's `haiku`/`sonnet`/`opus`; check your own tool's model list for equivalents) over dated full IDs, since aliases survive model rotations while pinned IDs hard-fail. Don't assume built-in subagents are cheap: some harnesses default them to inheriting your main-session model, so read the agent definitions and set a per-invocation model where they do. On Claude Code, the built-in Explore/Plan/general-purpose agents inherit, and a user-level `Explore` agent with `model: haiku` catches the spontaneous searches you don't route. Where the harness supports per-agent reasoning effort, run cheap-tier recon and mechanical work at low effort. ## Run it @@ -41,18 +41,18 @@ Pin an explicit model on every spawn, and where the harness offers stable aliase ## Usage limits -- Know what delegation buys. On pay-per-token APIs, cheaper tiers cut real dollars. On subscriptions, most cost is context reprocessing, and each subagent rebuilds context whose findings flow back into yours — total tokens can rise while the binding quota falls. The durable wins are bucket arbitrage (e.g. Claude's separate Sonnet-only weekly allowance, while the frontier tier drains the shared bucket fastest), wall-clock parallelism, and a lean main context. +- On pay-per-token APIs, cheaper tiers cut real dollars. On subscriptions, each subagent rebuilds context whose findings flow back into yours, so total tokens can rise while the binding quota falls; the durable wins there are bucket arbitrage (e.g. Claude's separate Sonnet-only weekly allowance, while the frontier tier drains the shared bucket fastest), wall-clock parallelism, and a lean main context. - Delegate in bounded waves (~3 parallel); between waves check the harness's usage surface (on Claude Code, `npx -y ccusage@latest blocks --active --json`; elsewhere a usage/status command if one exists, or treat the first rate-limit error as the cap). Stop launching once any usage window nears ~95%; let in-flight work finish. -- For long unattended runs, pause at the cap and resume when it clears: finish the wave, then use the harness's scheduled-wakeup primitive, chaining wakeups of ≤3600s until the window clears — never busy-wait with `sleep`. If no such primitive exists, write a self-contained resume prompt to a handoff file (remaining plan, the 95% rule, the exact usage command and its last reading, subagent handoffs) and tell the user to relaunch with it. On resume, re-verify with the usage command — a fresh block timestamp, not elapsed wall-clock, proves rollover. Tell the user which window tripped, the observed %, the next check time, and the outstanding work. +- For long unattended runs, pause at the cap and resume when it clears: finish the wave, then use the harness's scheduled-wakeup primitive, chaining wakeups of ≤3600s until the window clears, never busy-waiting with `sleep`. If no such primitive exists, write a self-contained resume prompt to a handoff file (remaining plan, the 95% rule, the exact usage command and its last reading, subagent handoffs) and tell the user to relaunch with it. On resume, re-verify with the usage command: a fresh block timestamp, not elapsed wall-clock, proves rollover. Tell the user which window tripped, the observed %, the next check time, and the outstanding work. ## Vet results -Reports are leads, not facts. Before acting on a high-impact finding, opening a PR, or claiming done: reopen key cited files, confirm line refs and failures, review the final diff, and resolve subagent disagreements yourself. For non-trivial completed work, spawn a fresh-context verifier on your tier that only tries to refute the claim — rerun the tests, drive the affected flow, probe edge cases — and never fixes anything; independent refutation beats self-review. +Reports are leads, not facts. Before acting on a high-impact finding, opening a PR, or claiming done: reopen key cited files, confirm line refs and failures, review the final diff, and resolve subagent disagreements yourself. For non-trivial completed work, spawn a fresh-context verifier on your tier that only tries to refute the claim (rerun the tests, drive the affected flow, probe edge cases) and never fixes anything; independent refutation beats self-review. ## Guardrails - Don't delegate a blocker your next step needs. -- Don't let two subagents edit the same files, and don't implement slices workers own — coordination plus duplicated implementation costs more than either alone. +- Don't let two subagents edit the same files, and don't implement slices workers own; coordination plus duplicated implementation costs more than either alone. - Enforce read-only roles (recon, review, verification) by tool allowlist where the harness supports it, not prompt text. -- Route security-sensitive work (authn/authz, secrets, crypto, hardening) to a capable non-frontier tier — frontier safety classifiers can refuse benign defensive work mid-task. +- Route security-sensitive work (authn/authz, secrets, crypto, hardening) to a capable non-frontier tier; frontier safety classifiers can refuse benign defensive work mid-task. - If the task is tiny, doesn't parallelize, or needs delicate judgment throughout, skip the ceremony and do it yourself. diff --git a/.copilot/skills/review-pr/SKILL.md b/.copilot/skills/review-pr/SKILL.md index 27576cf..a7dbad5 100644 --- a/.copilot/skills/review-pr/SKILL.md +++ b/.copilot/skills/review-pr/SKILL.md @@ -45,15 +45,15 @@ gh api graphql --paginate --slurp \ **Auto-resolve:** If a thread's first comment body matches any `$IGNORED_FILE` entry (`grep -qxF`), resolve via `resolveReviewThread` mutation without classifying. -If unresolved threads remain → step 3. Do NOT re-request a bot review while threads are still open — process existing feedback first. Only when zero unresolved threads remain → step 5. +If unresolved threads remain → step 3. Do NOT re-request a bot review while threads are still open; process existing feedback first. Only when zero unresolved threads remain → step 5. ### 3. Classify and resolve Read referenced file + context for each remaining thread, then classify: -- **Already addressed / Informational / Inaccurate** — append body to `$IGNORED_FILE`, resolve (reply with brief explanation if inaccurate). -- **Valid fix** — implement minimal change. Must meet ALL: (1) fixes a real bug — wrong behavior, data loss, security, crash, or race condition; (2) net-simpler or complexity-neutral; (3) concrete, not speculative. -- **Nitpick / Low-value** — resolve WITHOUT implementing. Includes: style preferences not enforced by linter, docstring suggestions on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append body to `$IGNORED_FILE`, reply with one-line rationale, resolve. +- **Already addressed / Informational / Inaccurate**: append body to `$IGNORED_FILE`, resolve (reply with brief explanation if inaccurate). +- **Valid fix**: implement minimal change. Must meet ALL: (1) fixes a real bug (wrong behavior, data loss, security, crash, or race condition); (2) net-simpler or complexity-neutral; (3) concrete, not speculative. +- **Nitpick / Low-value**: resolve WITHOUT implementing. Includes: style preferences not enforced by linter, docstring suggestions on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append body to `$IGNORED_FILE`, reply with one-line rationale, resolve. ### 4. Push fixes @@ -61,34 +61,52 @@ Stage, commit (`fix:`/`refactor:`/etc.), push, verify CI green, resolve fixed th ### 5. Ensure bot review covers latest commit -Only reached when zero unresolved threads remain. Get HEAD SHA: `gh pr view {PR_NUMBER} --json commits --jq '.commits[-1].oid'`. Bots = logins ending in `[bot]`. Fetch their latest reviews: +Each bot's latest review, and the commit it covers: ```bash -gh api repos/{owner}/{repo}/pulls/{PR_NUMBER}/reviews \ - --jq '[.[] | select(.user.login | endswith("[bot]"))] | group_by(.user.login) | map(max_by(.submitted_at))' +head_sha=$(gh pr view {PR_NUMBER} --json commits --jq '.commits[-1].oid') + +latest() { gh api --paginate --slurp repos/{owner}/{repo}/pulls/{PR_NUMBER}/reviews \ + | jq -r 'add | [.[] | select(.user.login | endswith("[bot]"))] | group_by(.user.login) + | map(max_by(.submitted_at)) | .[] | "\(.user.login) \(.commit_id)"'; } + +stale=$(latest | grep -v " $head_sha$" | cut -d' ' -f1 | sort -u) ``` -If a bot's latest review already covers HEAD → success. Stop. +`/reviews` alone identifies the review bots; CI and deploy bots never appear there. `--slurp` piped to `jq`, not `--jq`: under `--paginate` a `--jq` filter runs per page, so `max_by` returns a per-page max and lists a long-running PR's bots twice. + +Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise re-trigger each login in `stale`; they do not re-review a push on their own. + +| Bot | Login | Re-trigger with | +| --- | --- | --- | +| Copilot | `copilot-pull-request-reviewer[bot]` | `gh pr edit {PR_NUMBER} --add-reviewer @copilot` | +| CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | +| Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -Otherwise, re-request and poll (first check 8 min, timeout 15 min, poll 60 s). `gh pr edit --add-reviewer` re-requests reviews from existing bot reviewers — do not skip this. +- Pass the literal `@copilot`; its raw `[bot]` login can exit 0 having requested nothing. Confirm Copilot specifically, not just that some reviewer is pending: `gh api repos/{owner}/{repo}/pulls/{PR_NUMBER} --jq '.requested_reviewers[].login' | grep -qiE '^(Copilot|copilot-pull-request-reviewer\[bot\])$'`. A miss means it did not take, and the poll below would burn its full timeout waiting. Match both spellings: `requested_reviewers` returns the login as `Copilot`, while the review it later submits carries `copilot-pull-request-reviewer[bot]`, so checking only the `[bot]` form reports failure on every successful request. +- App-based bots (CodeRabbit, Greptile) cannot be requested as reviewers at all; a mention is their only trigger. `@coderabbitai full review` re-reviews the whole diff rather than just new commits. +- **Bot not in the table, or none found** → ask the user for the exact trigger. Never guess a mention string: a wrong one posts a visible no-op comment. + +Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired, one per line, dropping any you could not trigger: ```bash -bot_logins=$(gh api repos/{owner}/{repo}/pulls/{PR_NUMBER}/reviews \ - --jq '[.[] | select(.user.login | endswith("[bot]")) | .user.login] | unique | .[]') +triggered="$stale" # minus any bot you could not trigger -for bot in $bot_logins; do - gh pr edit {PR_NUMBER} --add-reviewer "$bot" -done +# Never poll on an empty set: comm would report nothing pending and the loop +# would break on the first pass, declaring success without waiting. +[ -n "$triggered" ] || { echo "nothing was triggered"; exit 1; } end=$((SECONDS+900)); sleep 480 while [ $SECONDS -lt $end ]; do - commit_id=$(gh api repos/{owner}/{repo}/pulls/{PR_NUMBER}/reviews \ - --jq '[.[] | select(.user.login=="{bot}")] | max_by(.submitted_at) | .commit_id') - [ "$commit_id" = "$head_sha" ] && break + pending=$(comm -23 <(printf '%s\n' "$triggered" | sort -u) \ + <(latest | grep " $head_sha$" | cut -d' ' -f1 | sort -u)) + [ -z "$pending" ] && break sleep 60 done ``` -Timeout → tell user to re-run this command and stop. Success → go back to step 2. +Run both blocks in one shell: `head_sha` and `latest` do not survive separate tool calls. If your harness blocks foreground `sleep`, run the whole wait as one backgrounded command rather than sleeping between tool calls. + +Timeout → name the bots still pending, tell user to re-run this command, stop. Success → go back to step 2. -Declare success when step 2 finds zero unresolved threads AND step 5 confirms a bot review on HEAD. Stop at iteration 5. Report: threads resolved, fixes made, threads auto-ignored, threads remaining, CI status. +Stop at iteration 5. Report: threads resolved, fixes made, threads auto-ignored, threads remaining, CI status. diff --git a/.gitignore b/.gitignore index f30f621..2c2eb8c 100644 --- a/.gitignore +++ b/.gitignore @@ -10,3 +10,6 @@ plan-review-summary.md # Impeccable hook installer artifacts (machine-specific absolute paths) .claude/settings.local.json .codex/hooks.json + +# review-pr skill scratch files +.review-pr-ignored-* diff --git a/.kimi-code/skills/code-refinement/SKILL.md b/.kimi-code/skills/code-refinement/SKILL.md index c1c3882..bcb5593 100644 --- a/.kimi-code/skills/code-refinement/SKILL.md +++ b/.kimi-code/skills/code-refinement/SKILL.md @@ -1,6 +1,6 @@ --- name: code-refinement -description: "Review staged files for code quality (KISS, DRY, YAGNI, Clean Code) and fix linting issues. Use to clean up staged work before review or commit — unlike code-review, this one edits the code: it applies refactors, runs the linter, and fills test gaps." +description: "Review staged files for code quality (KISS, DRY, YAGNI, Clean Code) and fix linting issues. Use to clean up staged work before review or commit. Unlike code-review, this one edits the code: it applies refactors, runs the linter, and fills test gaps." --- # Code Refinement diff --git a/.kimi-code/skills/code-review/SKILL.md b/.kimi-code/skills/code-review/SKILL.md index 2c62780..4c0d5d9 100644 --- a/.kimi-code/skills/code-review/SKILL.md +++ b/.kimi-code/skills/code-review/SKILL.md @@ -5,30 +5,11 @@ description: "Review staged changes for security, correctness, performance, and # Role -You are a senior code reviewer and security expert. -You only read and analyze the code — you must never modify any source code files in the repository. -The sole exception is writing your review output into a Markdown file. -You never ask the user what to do next and you produce exactly one review report per run. - -## Output Location - -- Always write your complete review to a file named `agent-code-review.md` in the project root. -- Overwrite the file completely on each run — do not append. -- This file is the only file you may create or modify. -- Do not stage, commit, or push this file. - -## Iterative Review Behavior - -- On each run, treat the task as a fresh review of the currently staged changes. -- Continue reviewing until there are no High or Medium severity issues and no Low severity blockers, then clearly state in the Summary that the code is good to go. +You are a senior code reviewer and security expert. Read and analyze only; never modify a source file. `agent-code-review.md` in the project root is the single file you may write, overwritten completely each run. Do not stage, commit, or push it. Never ask the user what to do next, and produce exactly one report per run. ## Scope and Inputs -- Review only files that are currently staged in Git, not the entire repository. -- Focus on changed lines and minimal necessary surrounding context. -- If information is missing, state reasonable assumptions and proceed. - -## How to Collect Context +Each run is a fresh review of the currently staged files, not the whole repository. Focus on changed lines and the minimum surrounding context. If information is missing, state a reasonable assumption and proceed. - `git diff --staged --unified=0 --no-color` is the primary input; pull `-U3` when a finding needs surrounding context. - Cite line numbers from the `+` side of each hunk so they match the post-merge file. @@ -37,16 +18,7 @@ You never ask the user what to do next and you produce exactly one review report ## Review Policy -Prioritize findings that materially improve: - -- Security, reliability, data integrity, privacy. -- Correctness and performance where clearly impactful. -- Clarity and Clean Code. - -Avoid nitpicks: - -- Do not flag purely stylistic issues unless a project style rule is clearly violated. -- Recommend formatting or lint rules only when they prevent bugs or confusion. +Prioritize what materially improves security, reliability, data integrity, and privacy; correctness and performance where clearly impactful; and clarity. Do not flag purely stylistic issues unless a project style rule is clearly violated, and recommend formatting or lint rules only when they prevent bugs or confusion. ## Severity Definitions @@ -54,20 +26,13 @@ Avoid nitpicks: - **Medium:** likely bugs, race conditions, significant performance or maintainability problems. - **Low:** clarity, naming, minor cleanup. A Low finding is a blocker only when it violates an explicit project rule (lint configuration or a documented convention); otherwise it never blocks the verdict. -## Security Checklist - -- Map each security finding to OWASP Top Ten, e.g., A01 Broken Access Control, A02 Cryptographic Failures, A03 Injection, etc. -- For HTTP APIs, also consider OWASP API Top 10. -- Provide actionable mitigations. +Review until no High or Medium issues and no Low blockers remain, then record the verdict in the Summary. -## Clean Code and Clarity Checks +## What to Check -- Prefer small, focused functions, clear names, elimination of duplication, obvious control flow. -- Suggest local refactors near changed lines. -- Provide minimal viable patches as examples when safe. -- Identify dead code (unused variables, functions, imports, classes). -- Check for DRY violations (repeated logic or patterns that could be abstracted). -- Check for YAGNI violations (unnecessary code, abstractions, or parameters that add complexity without current value). +- **Security:** map each finding to OWASP Top Ten (A01 Broken Access Control, A02 Cryptographic Failures, A03 Injection, etc.), plus OWASP API Top 10 for HTTP APIs. Provide actionable mitigations. +- **Clean code:** small focused functions, clear names, obvious control flow. Suggest local refactors near changed lines, with minimal viable patches as examples when safe. +- **Dead code, DRY, YAGNI:** unused variables, functions, imports, or classes; repeated logic worth abstracting; abstractions or parameters that add complexity without current value. ## Output Format @@ -84,7 +49,7 @@ Write the following structure into `agent-code-review.md`. `N` is the review ite - One paragraph on overall risk and clarity. - Finding counts: High X, Medium Y, Low Z. -- If no High or Medium remain and no Low blockers, state: **Verdict: good to go**. +- If no High or Medium remain and no Low blockers, state: **Verdict: good to go**. Automation depends on detecting this exact string. ## Findings diff --git a/.kimi-code/skills/commitmsg/SKILL.md b/.kimi-code/skills/commitmsg/SKILL.md index e40f81a..b8938db 100644 --- a/.kimi-code/skills/commitmsg/SKILL.md +++ b/.kimi-code/skills/commitmsg/SKILL.md @@ -1,6 +1,6 @@ --- name: commitmsg -description: "Propose a single git commit message for the currently staged changes. Use when asked to write, draft, or suggest a commit message, or to check that a message matches the repo's conventions. Proposes the message only — it does not commit." +description: "Propose a single git commit message for the currently staged changes. Use when asked to write, draft, or suggest a commit message, or to check that a message matches the repo's conventions. Proposes the message only; it does not commit." --- # Commit Message @@ -9,10 +9,10 @@ description: "Propose a single git commit message for the currently staged chang Run these commands to understand the changes: -- `git diff --staged --stat` — the shape of the change; then `git diff --staged` (or per-file diffs) only for files whose purpose isn't clear from the stat -- `git status -s` — staged file list -- `git log -n 20 --oneline` — recent style and to avoid repetition -- `git branch --show-current` — if it contains a ticket ID (e.g. ABC-123), prefix the subject line +- `git diff --staged --stat`: the shape of the change; then `git diff --staged` (or per-file diffs) only for files whose purpose isn't clear from the stat +- `git status -s`: staged file list +- `git log -n 20 --oneline`: recent style and to avoid repetition +- `git branch --show-current`: if it contains a ticket ID (e.g. ABC-123), prefix the subject line ## Rules diff --git a/.kimi-code/skills/dependency-review/SKILL.md b/.kimi-code/skills/dependency-review/SKILL.md index 42afe01..3254de4 100644 --- a/.kimi-code/skills/dependency-review/SKILL.md +++ b/.kimi-code/skills/dependency-review/SKILL.md @@ -1,36 +1,30 @@ --- name: dependency-review -description: "Audit package dependency updates for supply-chain risk: publish-age gate, changelog/diff verification, security advisories, community signals, and breaking changes. Use whenever a branch, PR, or working directory changes a dependency manifest or lockfile — package.json, requirements.txt, pyproject.toml, Gemfile, go.mod, Cargo.toml, pom.xml, build.gradle, composer.json, pubspec.yaml, or the lockfile beside them — including Dependabot/Renovate batches and newly added packages." +description: "Audit package dependency updates for supply-chain risk: publish-age gate, changelog/diff verification, security advisories, community signals, and breaking changes. Use whenever a branch, PR, or working directory changes a dependency manifest or lockfile (package.json, requirements.txt, pyproject.toml, Gemfile, go.mod, Cargo.toml, pom.xml, build.gradle, composer.json, pubspec.yaml, or the lockfile beside them), including Dependabot/Renovate batches and newly added packages." --- # Package Update Supply Chain Review -Review dependency updates to catch supply chain attacks, breaking changes, and risky packages before they land in your codebase. - ## Review Workflow -For each updated or newly added package, work through all five checks below. Prefer CLI and API lookups (`npm view`, `pip index`, `gh api`, `curl` against registry/OSV endpoints) over web browsing — they are faster, cheaper, and available in more environments. Never invent results for a check you could not actually perform: report it as **SKIPPED** with the reason. Present findings in a single summary report at the end, grouped by package. Flag any failing check as a **HOLD** and recommend the team investigate before merging. +For each updated or newly added package, work through all five checks below. Prefer CLI and API lookups (`npm view`, `pip index`, `gh api`, `curl` against registry/OSV endpoints) over web browsing. Never invent a result for a check you could not actually perform: report it as **SKIPPED** with the reason. Present findings in a single summary report at the end, grouped by package, and flag any failing check as a **HOLD** to investigate before merging. ### 1. Publication Age Gate -**Goal:** Confirm the release is at least 7 days old. Compromised or typosquatted releases are usually caught within the first few days, so letting a release "bake" gives the community and automated scanners time to notice. - -**Steps:** +Confirm the release is at least 7 days old. Compromised and typosquatted releases are usually caught within the first few days, so letting one bake gives scanners and the community time to notice. -1. Look up the publish date for the exact version on its registry — e.g. `npm view time --json`, `curl https://pypi.org/pypi///json`, `gem info --remote`, or the registry's web page. -2. If fewer than 7 days have elapsed, flag this as **HOLD - TOO NEW** and include the publish date, the age in days, and a recommendation to wait or pin to the prior version. +1. Look up the publish date for the exact version (`npm view time --json`, `curl https://pypi.org/pypi///json`, or the registry's page). +2. Under 7 days → **HOLD - TOO NEW**, with the publish date, the age in days, and a recommendation to wait or pin to the prior version. ### 2. Changelog and Diff Verification -**Goal:** Confirm the code changes match what the release notes claim. - -**Steps:** +Confirm the code changes match what the release notes claim. -1. Locate the changelog, release notes, or GitHub releases page for the new version (e.g. `gh release view --repo /`). -2. Identify the claimed changes (bug fixes, features, refactors, etc.). -3. Skim the actual source diff between the old and new version (e.g. `gh api repos///compare/v1.2.3...v1.4.0` or the repo's compare view). -4. Look for discrepancies: Are there unexpected new files? New network calls? Obfuscated code? Post-install scripts that were not present before? -5. Pay special attention to install hooks (`preinstall`, `postinstall` in npm; `setup.py` entry points in Python; `build.rs` changes in Rust; etc.) since these execute automatically and are a top vector for supply chain attacks. +1. Locate the changelog or releases page for the new version (`gh release view --repo /`). +2. Identify the claimed changes. +3. Skim the source diff between the old and new version (`gh api repos///compare/v1.2.3...v1.4.0`). +4. Look for discrepancies: unexpected new files, new network calls, obfuscated code, post-install scripts that were not there before. +5. Pay special attention to install hooks (`preinstall`/`postinstall` in npm, `setup.py` entry points in Python, `build.rs` in Rust); they execute automatically and are a top supply chain vector. **Red flags to call out:** @@ -38,53 +32,38 @@ For each updated or newly added package, work through all five checks below. Pre - New outbound HTTP/DNS calls, especially to IP addresses or unusual domains - Environment variable reads for tokens, keys, or credentials - New native/binary dependencies or compiled assets -- Changes to CI config or build scripts that fetch remote resources +- CI config or build script changes that fetch remote resources ### 3. Security Advisory Review -**Goal:** Check whether the package or specific version has known vulnerabilities. - -**Steps:** - -1. Query advisory sources for the package name and version range: `gh api /advisories --method GET -f "affects="`, the OSV API (`curl -s https://api.osv.dev/v1/query -d '{"package":{"name":"","ecosystem":""}}'`), or ecosystem tools (`npm audit`, `pip-audit`, `cargo audit`). -2. Check whether the update itself is a security patch. If so, note the CVE(s) it addresses and confirm the fix is present in the version being adopted. -3. Check whether the new version introduces any new advisories. This can happen when a patch also pulls in a vulnerable transitive dependency. -4. Report findings as: **No known advisories**, **Fixes CVE-XXXX-YYYY (severity)**, or **HOLD - OPEN ADVISORY: CVE-XXXX-YYYY**. +1. Query advisories for the package name and version range: `gh api /advisories --method GET -f "affects="`, the OSV API (`curl -s https://api.osv.dev/v1/query -d '{"package":{"name":"","ecosystem":""}}'`), or ecosystem tools (`npm audit`, `pip-audit`, `cargo audit`). +2. If the update is itself a security patch, note the CVE(s) it addresses and confirm the fix is present in the version being adopted. +3. Check whether the new version introduces new advisories, since a patch can pull in a vulnerable transitive dependency. +4. Report as **No known advisories**, **Fixes CVE-XXXX-YYYY (severity)**, or **HOLD - OPEN ADVISORY: CVE-XXXX-YYYY**. ### 4. Community Signals (best effort) -**Goal:** See if real users are reporting problems, compromises, or regressions with this release. +Look for real users reporting problems, compromises, or regressions with this release. -**Steps:** +1. Check the repo's issues filed after the release date (`gh api "search/issues?q=repo:/+created:>"`). +2. With web access, also search the package name + version on Stack Overflow, Hacker News, and the ecosystem's channels, looking for several people reporting the same crash or suspicious activity. Without web access, limit this check to the CLI/API sources and say so. +3. Compare download counts against the package's historical trend where the registry exposes them; a sudden spike or drop can indicate typosquatting or an abandoned fork. -1. Check the package's GitHub Issues for reports filed after the release date (e.g. `gh api "search/issues?q=repo:/+created:>"` or `gh issue list --repo /`). -2. If you have web access, also search for the package name + version on Stack Overflow, Hacker News, and the ecosystem's community channels; look for patterns such as multiple people reporting the same crash or suspicious activity. -3. Compare download counts against the package's historical trend where the registry exposes them (e.g. `npm view ` plus the npm downloads API). A sudden spike or drop can indicate typosquatting or an abandoned fork. -4. Without web access, limit this check to what the CLI/API sources above can answer and say so. - -**Report as:** A brief summary of community sentiment, "No community issues found for this version" if clean, or **SKIPPED** with the reason if the sources were unreachable. +Report a brief sentiment summary, "No community issues found for this version" if clean, or **SKIPPED** with the reason. ### 5. Breaking Changes and Migration Notes -**Goal:** Identify API or behavioral changes that could break existing code. - -**Steps:** - -1. Check the bump against semver. Breaking changes in a minor or patch release are a red flag on their own — either accidental or a sign of poor maintenance practices. +1. Check the bump against semver. Breaking changes in a minor or patch release are a red flag on their own, either accidental or a sign of poor maintenance. 2. Read the migration guide or upgrade notes if one exists. -3. Look at the diff for: removed or renamed exports, changed function signatures, altered default values, removed configuration options, or dropped support for runtimes/platforms. -4. Search the codebase for usages of any changed or removed APIs. List the files and line numbers that may need updates. -5. Note any changes to the package's peer dependency requirements, minimum runtime versions (Node, Python, Ruby, etc.), or required environment variables. - -**Report as:** +3. Look in the diff for removed or renamed exports, changed function signatures, altered default values, removed configuration options, or dropped runtime/platform support. +4. Search the codebase for usages of anything changed or removed. List the files and line numbers that may need updates. +5. Note changes to peer dependency requirements, minimum runtime versions (Node, Python, Ruby), or required environment variables. -- **No breaking changes** for seamless upgrades. -- **Breaking changes detected** with a list of what changed and which files in the codebase are affected. -- **Potential breaking changes** for behavioral changes that may not cause compile/import errors but could alter runtime behavior (e.g., a default timeout changing from 30s to 5s). +Report as **No breaking changes**; **Breaking changes detected** with what changed and which files are affected; or **Potential breaking changes** for behavioral shifts that compile and import fine but alter runtime behavior (e.g. a default timeout dropping from 30s to 5s). ## Output Format -Present the full review as a structured report. Here is the template: +Present the full review as a structured report: ```text # Package Update Review @@ -115,13 +94,11 @@ Present the full review as a structured report. Here is the template: ## Edge Cases -- **New dependencies** (not just version bumps): Apply the same five checks but also verify the package is the intended one (check for typosquatting by comparing to similarly named popular packages) and review its overall maintenance health (last commit date, number of maintainers, bus factor). -- **Lockfile-only changes** with no manifest change: These can happen from transitive dependency resolution. Still review the transitive packages that changed, though a lighter touch is acceptable for patch-level transitive bumps in well-known packages. -- **Monorepos with many packages:** Group related packages (e.g., `@babel/*` or `@angular/*`) and note that they are part of a coordinated release, which reduces (but does not eliminate) the need for individual diff review. -- **Private/internal packages:** The publication age gate may not apply, but the diff verification and breaking change checks still do. +- **New dependencies** (not just version bumps): same five checks, plus verify it is the intended package (compare against similarly named popular ones for typosquatting) and review maintenance health: last commit date, maintainer count, bus factor. +- **Lockfile-only changes** with no manifest change: still review the transitive packages that changed, though a lighter touch is acceptable for patch-level bumps in well-known packages. +- **Monorepo groups** (`@babel/*`, `@angular/*`): treat as one coordinated release, which reduces but does not eliminate individual diff review. +- **Private/internal packages:** the publication age gate may not apply; diff verification and breaking change checks still do. Never send internal package names or versions to public registry or advisory endpoints. Query the private registry where it exposes an API, otherwise mark those checks **SKIPPED** with the reason. -## Tips for Efficiency +## Priorities -- Start with the publication age gate; it is the fastest check and can immediately flag the riskiest updates. -- For large dependency updates (e.g., Dependabot batches), prioritize direct dependencies over transitive ones, and prioritize packages with install hooks. -- If a package has hundreds of thousands of weekly downloads and is maintained by a well-known org (e.g., Meta, Google, Vercel), the changelog and community checks can be lighter. But never skip the security advisory check. +Start with the publication age gate; it is the fastest check and immediately flags the riskiest updates. In large batches (Dependabot/Renovate), prioritize direct dependencies over transitive ones, and packages with install hooks over those without. For high-download packages from well-known orgs, the changelog and community checks can be lighter; never skip the security advisory check. diff --git a/.kimi-code/skills/efficient-orchestration/SKILL.md b/.kimi-code/skills/efficient-orchestration/SKILL.md index 4e56a5d..453d903 100644 --- a/.kimi-code/skills/efficient-orchestration/SKILL.md +++ b/.kimi-code/skills/efficient-orchestration/SKILL.md @@ -1,6 +1,6 @@ --- name: efficient-orchestration -description: "Run this task with your current model orchestrating while cheaper subagents do the token-heavy research, coding, and testing. Use for work that is large, parallelizable, or token-hungry — broad repo scans, long logs, wide test or browser passes, repetitive edits — or when the user asks to conserve usage limits. Skip it for small, sequential, or judgment-dense tasks." +description: "Run this task with your current model orchestrating while cheaper subagents do the token-heavy research, coding, and testing. Use for work that is large, parallelizable, or token-hungry (broad repo scans, long logs, wide test or browser passes, repetitive edits), or when the user asks to conserve usage limits. Skip it for small, sequential, or judgment-dense tasks." --- # Efficient Orchestration @@ -21,15 +21,15 @@ Order the models your harness can run from cheapest to most capable (e.g. on Cla ## Pick each subagent's model -By task difficulty, not task type — and never above your own tier: +By task difficulty, not task type, and never above your own tier: -- **Cheapest tier:** mechanical, high-volume, low-judgment work — search sweeps, inventory, log reduction, simple edits. +- **Cheapest tier:** mechanical, high-volume, low-judgment work: search sweeps, inventory, log reduction, simple edits. - **Mid tier (default):** focused research, routine or narrow patches, test runs, straightforward debugging. -- **Your tier:** complex work delegated for parallelism or context isolation, not savings — intricate refactors, multi-file features, subtle bugs, design exploration. +- **Your tier:** complex work delegated for parallelism or context isolation, not savings: intricate refactors, multi-file features, subtle bugs, design exploration. -Start at the cheapest tier that can plausibly succeed; after two failures at a tier, escalate one tier or take the work back — never a third retry at the same tier. +Start at the cheapest tier that can plausibly succeed; after two failures at a tier, escalate one tier or take the work back; never a third retry at the same tier. -Pin an explicit model on every spawn, and where the harness offers stable aliases (e.g. Claude's `haiku`/`sonnet`/`opus`), prefer them over dated full IDs — aliases survive model rotations; pinned IDs hard-fail. Don't assume built-in subagents are cheap: some harnesses default them to inheriting your main-session model (Claude Code's built-in Explore/Plan/general-purpose do; a per-invocation model overrides it, and a user-level `Explore` agent with `model: haiku` catches the spontaneous searches you don't route). Where the harness supports per-agent reasoning effort, run cheap-tier recon and mechanical work at low effort — current-generation low roughly matches previous-generation highest. +Pin an explicit model on every spawn, preferring your harness's stable aliases (Claude's `haiku`/`sonnet`/`opus`; check your own tool's model list for equivalents) over dated full IDs, since aliases survive model rotations while pinned IDs hard-fail. Don't assume built-in subagents are cheap: some harnesses default them to inheriting your main-session model, so read the agent definitions and set a per-invocation model where they do. On Claude Code, the built-in Explore/Plan/general-purpose agents inherit, and a user-level `Explore` agent with `model: haiku` catches the spontaneous searches you don't route. Where the harness supports per-agent reasoning effort, run cheap-tier recon and mechanical work at low effort. ## Run it @@ -41,18 +41,18 @@ Pin an explicit model on every spawn, and where the harness offers stable aliase ## Usage limits -- Know what delegation buys. On pay-per-token APIs, cheaper tiers cut real dollars. On subscriptions, most cost is context reprocessing, and each subagent rebuilds context whose findings flow back into yours — total tokens can rise while the binding quota falls. The durable wins are bucket arbitrage (e.g. Claude's separate Sonnet-only weekly allowance, while the frontier tier drains the shared bucket fastest), wall-clock parallelism, and a lean main context. +- On pay-per-token APIs, cheaper tiers cut real dollars. On subscriptions, each subagent rebuilds context whose findings flow back into yours, so total tokens can rise while the binding quota falls; the durable wins there are bucket arbitrage (e.g. Claude's separate Sonnet-only weekly allowance, while the frontier tier drains the shared bucket fastest), wall-clock parallelism, and a lean main context. - Delegate in bounded waves (~3 parallel); between waves check the harness's usage surface (on Claude Code, `npx -y ccusage@latest blocks --active --json`; elsewhere a usage/status command if one exists, or treat the first rate-limit error as the cap). Stop launching once any usage window nears ~95%; let in-flight work finish. -- For long unattended runs, pause at the cap and resume when it clears: finish the wave, then use the harness's scheduled-wakeup primitive, chaining wakeups of ≤3600s until the window clears — never busy-wait with `sleep`. If no such primitive exists, write a self-contained resume prompt to a handoff file (remaining plan, the 95% rule, the exact usage command and its last reading, subagent handoffs) and tell the user to relaunch with it. On resume, re-verify with the usage command — a fresh block timestamp, not elapsed wall-clock, proves rollover. Tell the user which window tripped, the observed %, the next check time, and the outstanding work. +- For long unattended runs, pause at the cap and resume when it clears: finish the wave, then use the harness's scheduled-wakeup primitive, chaining wakeups of ≤3600s until the window clears, never busy-waiting with `sleep`. If no such primitive exists, write a self-contained resume prompt to a handoff file (remaining plan, the 95% rule, the exact usage command and its last reading, subagent handoffs) and tell the user to relaunch with it. On resume, re-verify with the usage command: a fresh block timestamp, not elapsed wall-clock, proves rollover. Tell the user which window tripped, the observed %, the next check time, and the outstanding work. ## Vet results -Reports are leads, not facts. Before acting on a high-impact finding, opening a PR, or claiming done: reopen key cited files, confirm line refs and failures, review the final diff, and resolve subagent disagreements yourself. For non-trivial completed work, spawn a fresh-context verifier on your tier that only tries to refute the claim — rerun the tests, drive the affected flow, probe edge cases — and never fixes anything; independent refutation beats self-review. +Reports are leads, not facts. Before acting on a high-impact finding, opening a PR, or claiming done: reopen key cited files, confirm line refs and failures, review the final diff, and resolve subagent disagreements yourself. For non-trivial completed work, spawn a fresh-context verifier on your tier that only tries to refute the claim (rerun the tests, drive the affected flow, probe edge cases) and never fixes anything; independent refutation beats self-review. ## Guardrails - Don't delegate a blocker your next step needs. -- Don't let two subagents edit the same files, and don't implement slices workers own — coordination plus duplicated implementation costs more than either alone. +- Don't let two subagents edit the same files, and don't implement slices workers own; coordination plus duplicated implementation costs more than either alone. - Enforce read-only roles (recon, review, verification) by tool allowlist where the harness supports it, not prompt text. -- Route security-sensitive work (authn/authz, secrets, crypto, hardening) to a capable non-frontier tier — frontier safety classifiers can refuse benign defensive work mid-task. +- Route security-sensitive work (authn/authz, secrets, crypto, hardening) to a capable non-frontier tier; frontier safety classifiers can refuse benign defensive work mid-task. - If the task is tiny, doesn't parallelize, or needs delicate judgment throughout, skip the ceremony and do it yourself. diff --git a/.kimi-code/skills/review-pr/SKILL.md b/.kimi-code/skills/review-pr/SKILL.md index 27576cf..a7dbad5 100644 --- a/.kimi-code/skills/review-pr/SKILL.md +++ b/.kimi-code/skills/review-pr/SKILL.md @@ -45,15 +45,15 @@ gh api graphql --paginate --slurp \ **Auto-resolve:** If a thread's first comment body matches any `$IGNORED_FILE` entry (`grep -qxF`), resolve via `resolveReviewThread` mutation without classifying. -If unresolved threads remain → step 3. Do NOT re-request a bot review while threads are still open — process existing feedback first. Only when zero unresolved threads remain → step 5. +If unresolved threads remain → step 3. Do NOT re-request a bot review while threads are still open; process existing feedback first. Only when zero unresolved threads remain → step 5. ### 3. Classify and resolve Read referenced file + context for each remaining thread, then classify: -- **Already addressed / Informational / Inaccurate** — append body to `$IGNORED_FILE`, resolve (reply with brief explanation if inaccurate). -- **Valid fix** — implement minimal change. Must meet ALL: (1) fixes a real bug — wrong behavior, data loss, security, crash, or race condition; (2) net-simpler or complexity-neutral; (3) concrete, not speculative. -- **Nitpick / Low-value** — resolve WITHOUT implementing. Includes: style preferences not enforced by linter, docstring suggestions on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append body to `$IGNORED_FILE`, reply with one-line rationale, resolve. +- **Already addressed / Informational / Inaccurate**: append body to `$IGNORED_FILE`, resolve (reply with brief explanation if inaccurate). +- **Valid fix**: implement minimal change. Must meet ALL: (1) fixes a real bug (wrong behavior, data loss, security, crash, or race condition); (2) net-simpler or complexity-neutral; (3) concrete, not speculative. +- **Nitpick / Low-value**: resolve WITHOUT implementing. Includes: style preferences not enforced by linter, docstring suggestions on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append body to `$IGNORED_FILE`, reply with one-line rationale, resolve. ### 4. Push fixes @@ -61,34 +61,52 @@ Stage, commit (`fix:`/`refactor:`/etc.), push, verify CI green, resolve fixed th ### 5. Ensure bot review covers latest commit -Only reached when zero unresolved threads remain. Get HEAD SHA: `gh pr view {PR_NUMBER} --json commits --jq '.commits[-1].oid'`. Bots = logins ending in `[bot]`. Fetch their latest reviews: +Each bot's latest review, and the commit it covers: ```bash -gh api repos/{owner}/{repo}/pulls/{PR_NUMBER}/reviews \ - --jq '[.[] | select(.user.login | endswith("[bot]"))] | group_by(.user.login) | map(max_by(.submitted_at))' +head_sha=$(gh pr view {PR_NUMBER} --json commits --jq '.commits[-1].oid') + +latest() { gh api --paginate --slurp repos/{owner}/{repo}/pulls/{PR_NUMBER}/reviews \ + | jq -r 'add | [.[] | select(.user.login | endswith("[bot]"))] | group_by(.user.login) + | map(max_by(.submitted_at)) | .[] | "\(.user.login) \(.commit_id)"'; } + +stale=$(latest | grep -v " $head_sha$" | cut -d' ' -f1 | sort -u) ``` -If a bot's latest review already covers HEAD → success. Stop. +`/reviews` alone identifies the review bots; CI and deploy bots never appear there. `--slurp` piped to `jq`, not `--jq`: under `--paginate` a `--jq` filter runs per page, so `max_by` returns a per-page max and lists a long-running PR's bots twice. + +Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise re-trigger each login in `stale`; they do not re-review a push on their own. + +| Bot | Login | Re-trigger with | +| --- | --- | --- | +| Copilot | `copilot-pull-request-reviewer[bot]` | `gh pr edit {PR_NUMBER} --add-reviewer @copilot` | +| CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | +| Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -Otherwise, re-request and poll (first check 8 min, timeout 15 min, poll 60 s). `gh pr edit --add-reviewer` re-requests reviews from existing bot reviewers — do not skip this. +- Pass the literal `@copilot`; its raw `[bot]` login can exit 0 having requested nothing. Confirm Copilot specifically, not just that some reviewer is pending: `gh api repos/{owner}/{repo}/pulls/{PR_NUMBER} --jq '.requested_reviewers[].login' | grep -qiE '^(Copilot|copilot-pull-request-reviewer\[bot\])$'`. A miss means it did not take, and the poll below would burn its full timeout waiting. Match both spellings: `requested_reviewers` returns the login as `Copilot`, while the review it later submits carries `copilot-pull-request-reviewer[bot]`, so checking only the `[bot]` form reports failure on every successful request. +- App-based bots (CodeRabbit, Greptile) cannot be requested as reviewers at all; a mention is their only trigger. `@coderabbitai full review` re-reviews the whole diff rather than just new commits. +- **Bot not in the table, or none found** → ask the user for the exact trigger. Never guess a mention string: a wrong one posts a visible no-op comment. + +Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired, one per line, dropping any you could not trigger: ```bash -bot_logins=$(gh api repos/{owner}/{repo}/pulls/{PR_NUMBER}/reviews \ - --jq '[.[] | select(.user.login | endswith("[bot]")) | .user.login] | unique | .[]') +triggered="$stale" # minus any bot you could not trigger -for bot in $bot_logins; do - gh pr edit {PR_NUMBER} --add-reviewer "$bot" -done +# Never poll on an empty set: comm would report nothing pending and the loop +# would break on the first pass, declaring success without waiting. +[ -n "$triggered" ] || { echo "nothing was triggered"; exit 1; } end=$((SECONDS+900)); sleep 480 while [ $SECONDS -lt $end ]; do - commit_id=$(gh api repos/{owner}/{repo}/pulls/{PR_NUMBER}/reviews \ - --jq '[.[] | select(.user.login=="{bot}")] | max_by(.submitted_at) | .commit_id') - [ "$commit_id" = "$head_sha" ] && break + pending=$(comm -23 <(printf '%s\n' "$triggered" | sort -u) \ + <(latest | grep " $head_sha$" | cut -d' ' -f1 | sort -u)) + [ -z "$pending" ] && break sleep 60 done ``` -Timeout → tell user to re-run this command and stop. Success → go back to step 2. +Run both blocks in one shell: `head_sha` and `latest` do not survive separate tool calls. If your harness blocks foreground `sleep`, run the whole wait as one backgrounded command rather than sleeping between tool calls. + +Timeout → name the bots still pending, tell user to re-run this command, stop. Success → go back to step 2. -Declare success when step 2 finds zero unresolved threads AND step 5 confirms a bot review on HEAD. Stop at iteration 5. Report: threads resolved, fixes made, threads auto-ignored, threads remaining, CI status. +Stop at iteration 5. Report: threads resolved, fixes made, threads auto-ignored, threads remaining, CI status. diff --git a/.markdownlint-cli2.yaml b/.markdownlint-cli2.yaml index ebc99be..67330cc 100644 --- a/.markdownlint-cli2.yaml +++ b/.markdownlint-cli2.yaml @@ -1,4 +1,4 @@ config: - MD013: false # Line length — instructional prose runs long intentionally + MD013: false # Line length: instructional prose runs long intentionally ignores: - "test/fixtures/**" diff --git a/README.md b/README.md index 3332dc9..c092168 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ The `setup` script and the review loops are Bash scripts that shell out to a han **Required** (the `setup` script exits early if any is missing): - [`git`](https://git-scm.com/), to clone the repo and drive the `git`-based commands -- [GitHub CLI (`gh`)](https://cli.github.com/) 2.88.0+, installed and authenticated (`/review-pr` uses `gh pr edit --add-reviewer` to reliably re-request reviews from existing bot reviewers) +- [GitHub CLI (`gh`)](https://cli.github.com/) 2.88.0+, installed and authenticated (`/review-pr` uses the `gh pr edit --add-reviewer @copilot` special value added in 2.88.0 to re-request Copilot code review) - [`jq`](https://jqlang.github.io/jq/), a JSON processor used to read and edit each tool's settings and MCP config files **Required only for optional steps:** @@ -145,7 +145,7 @@ Beyond commands, `setup` installs user-level subagent definitions from [.claude/ ### Explore -Since Claude Code v2.1.198 the built-in `Explore` subagent [inherits your main-session model](https://code.claude.com/docs/en/sub-agents) instead of always running on Haiku (capped at Opus on the Claude API). If your daily driver is Opus or Fable, every background codebase search Claude spontaneously delegates bills at that tier. This agent shadows the built-in — a user-level agent with the same name overrides it, which the docs explicitly support — and pins exploration back to `haiku` at `effort: low` with read-only tools. +Since Claude Code v2.1.198 the built-in `Explore` subagent [inherits your main-session model](https://code.claude.com/docs/en/sub-agents) instead of always running on Haiku (capped at Opus on the Claude API). If your daily driver is Opus or Fable, every background codebase search Claude spontaneously delegates bills at that tier. This agent shadows the built-in (a user-level agent with the same name overrides it, which the docs explicitly support) and pins exploration back to `haiku` at `effort: low` with read-only tools. Trade-off to know about: a custom `Explore` loads your `CLAUDE.md`/user memory like any subagent, which the built-in skips for speed. To remove it, delete `~/.claude/agents/Explore.md`. @@ -207,7 +207,7 @@ REVIEWER_AGENT=codex Supported agents: `claude`, `codex`, `copilot`, `antigravity`, `kimi`. Only the agents you actually have installed need to be referenced. -Two caveats for `kimi`: it takes its prompt as a command-line argument (there is no stdin form), so on Windows/Git Bash a very large prompt can exceed the OS argument limit — the same limitation Copilot has. And it has no per-run flag to disable MCP servers, so an autonomous loop run still loads whatever is configured in `~/.kimi-code/mcp.json`. +Two caveats for `kimi`: it takes its prompt as a command-line argument (there is no stdin form), so on Windows/Git Bash a very large prompt can exceed the OS argument limit. Copilot has the same limitation. And it has no per-run flag to disable MCP servers, so an autonomous loop run still loads whatever is configured in `~/.kimi-code/mcp.json`. Both loops write their working files (`agent-code-review.md`, `agent-review-summary.md`, `feedback-plan.md`, `plan-review-summary.md`) to the target project's root. Consider adding those names to that project's `.gitignore` (or your global gitignore) so an agent never commits them by accident. @@ -240,16 +240,16 @@ The setup script can configure [Model Context Protocol (MCP)](https://modelconte | --- | --- | --- | | [Playwright](https://github.com/microsoft/playwright-mcp) | `@playwright/mcp@latest` | Browser automation and web testing | -MCP servers are added via each tool's `mcp add` CLI command at user scope. Tools without one (Copilot, Antigravity, Kimi Code) get their JSON config file edited directly — for Kimi that is `~/.kimi-code/mcp.json`, whose only built-in editor is the interactive `/mcp-config` TUI command. +MCP servers are added via each tool's `mcp add` CLI command at user scope. Tools without one (Copilot, Antigravity, Kimi Code) get their JSON config file edited directly; for Kimi that is `~/.kimi-code/mcp.json`, whose only built-in editor is the interactive `/mcp-config` TUI command. ## `gh` Agent Skill -Beyond the commands in this repo, `setup` can install the [`gh` agent skill published by `cli/cli`](https://github.com/cli/cli#agent-skills) for each selected tool. This is an **upstream** skill from the GitHub CLI team that teaches an agent to drive `gh` well — structured JSON output, pagination, repo targeting, search vs. list, and `gh api` fallback. It is unrelated to the commands/skills this repo ships. +Beyond the commands in this repo, `setup` can install the [`gh` agent skill published by `cli/cli`](https://github.com/cli/cli#agent-skills) for each selected tool. This is an **upstream** skill from the GitHub CLI team that teaches an agent to drive `gh` well: structured JSON output, pagination, repo targeting, search vs. list, and `gh api` fallback. It is unrelated to the commands/skills this repo ships. For each agent you select, `setup` installs it when missing and updates it when already present: -- Install — `gh skill install cli/cli gh --agent --scope user` -- Update — `gh skill update gh` +- Install: `gh skill install cli/cli gh --agent --scope user` +- Update: `gh skill update gh` This step is skipped automatically on versions of `gh` too old to ship the `gh skill` command (a preview feature), and for Kimi Code, which `gh skill` has no `--agent` id for. To manage it yourself: @@ -259,7 +259,7 @@ gh skill update gh # update (all hos gh skill list --agent claude-code # verify ``` -There is no `gh skill uninstall` command; to remove it, delete the installed `gh/` skill directory (its location is agent-dependent — e.g. `~/.codex/skills/gh/` or `~/.copilot/skills/gh/`; run `gh skill list --json skillName,path` to see the exact filesystem path). +There is no `gh skill uninstall` command; to remove it, delete the installed `gh/` skill directory (its location is agent-dependent, e.g. `~/.codex/skills/gh/` or `~/.copilot/skills/gh/`; run `gh skill list --json skillName,path` to see the exact filesystem path). ## Impeccable Design Skills @@ -286,7 +286,7 @@ See [impeccable.style](https://impeccable.style) for the full command list and t Commands are authored once as Claude Code command files; everything else is generated: -1. Create `.claude/commands/command-name.md` — markdown with YAML front matter containing at least `description` (plus optional Claude-specific keys like `allowed-tools` or `argument-hint`), and an optional `$ARGUMENTS` placeholder in the body. +1. Create `.claude/commands/command-name.md`, markdown with YAML front matter containing at least `description` (plus optional Claude-specific keys like `allowed-tools` or `argument-hint`), and an optional `$ARGUMENTS` placeholder in the body. 2. If the command should also ship as a Codex/Copilot/Antigravity/Kimi skill, add its name to `SKILL_COMMANDS` in `tools/generate` (or `PROMPT_COMMANDS` if the review loops need it as a shared prompt). 3. Run `tools/generate` to produce the derived `SKILL.md` and prompt files, and commit them together with the source. @@ -299,8 +299,8 @@ procedure. Anthropic's [new rules of context engineering][ctx] are the house style here: - **Put the trigger in the `description`.** It is the only text a model sees - before deciding to load the skill, so say *when* to reach for it — and when to - reach for a sibling instead — not just what it does. + before deciding to load the skill, so say *when* to reach for it, and when to + reach for a sibling instead, not just what it does. - **Say it once.** If the body opens by restating the `description`, delete that line. Guidance belongs in exactly one place. - **Spend tokens on gotchas, not procedure.** Skip steps a competent model @@ -309,7 +309,7 @@ style here: staged diff that still has staged files, which install hooks run automatically. - **Frame outcomes, not rules.** "Match the surrounding code" beats a list of banned constructs. Reserve hard constraints for the places where breaking them - breaks something — the review loops really do depend on the exact + breaks something: the review loops really do depend on the exact `NO_FURTHER_FEEDBACK` sentinel and on the reviewer never touching source files. - **Keep rubrics and output templates.** Structured criteria and worked report formats are references the model fills in, not rules that box it in. @@ -349,7 +349,7 @@ test/run Unit tests (`test/run`) cover config parsing, prompt loading, validation, review status checks, and generated-file sync. They run in seconds and need no API keys. -If you edit a command source in `.claude/commands/`, run `tools/generate` afterwards — the test suite and pre-commit both fail when the derived skill/prompt files are stale. +If you edit a command source in `.claude/commands/`, run `tools/generate` afterwards; the test suite and pre-commit both fail when the derived skill/prompt files are stale. ### Smoke Tests diff --git a/bin/code-review-loop b/bin/code-review-loop index ae5d0a5..ea38a8a 100755 --- a/bin/code-review-loop +++ b/bin/code-review-loop @@ -73,6 +73,7 @@ validate_positive_int "$MAX_ITERATIONS" "--max-iterations" # ---- paths --------------------------------------------------------------- PROJECT_ROOT="$(pwd)" REVIEW_FILE="$PROJECT_ROOT/agent-code-review.md" +SUMMARY_FILE="$PROJECT_ROOT/agent-review-summary.md" EDITOR_REFINEMENT_PROMPT="$PROMPTS_DIR/code-refinement.md" EDITOR_RESPONSE_PROMPT="$PROMPTS_DIR/code-review-response.md" @@ -97,7 +98,7 @@ stage_review_changes() { fi # Stage unstaged tracked modifications that were NOT present before the loop - # started — those are changes made by the review agents. Pre-existing + # started; those are changes made by the review agents. Pre-existing # unstaged changes (captured in BASELINE_UNSTAGED) are left untouched. local unstaged_modified unstaged_modified=$(git diff --name-only 2>/dev/null | sort) || true @@ -110,7 +111,7 @@ stage_review_changes() { done <<< "$agent_modified" fi - # Stage newly created untracked files — these are legitimate fixes from + # Stage newly created untracked files: these are legitimate fixes from # the editor's refinement (new tests, helper modules, etc.). Artifacts from # tools (coverage, caches) should be covered by .gitignore; the snapshot # already uses --exclude-standard so gitignored files are never seen here. @@ -144,7 +145,7 @@ validate_prompts "${_required_prompts[@]}" # ---- main ---------------------------------------------------------------- -# Detect partially staged files (some hunks staged, others not) — these would +# Detect partially staged files (some hunks staged, others not); these would # cause the staging logic to either miss review fixes or pull in unrelated hunks. partially_staged=$(comm -12 \ <(git diff --staged --name-only 2>/dev/null | sort) \ @@ -181,12 +182,12 @@ initial_diff=$(git diff --staged --stat 2>&1) || true initial_diff_full=$(git diff --staged 2>&1) || true initial_added_files=$(git diff --staged --name-only --diff-filter=A 2>/dev/null | sort) || true -# Baseline unstaged tracked files — only changes to files NOT in this list +# Baseline unstaged tracked files: only changes to files NOT in this list # should be staged during the loop (they come from the review agents). BASELINE_UNSTAGED="$TMPDIR_REVIEW/baseline-unstaged.txt" git diff --name-only 2>/dev/null | sort > "$BASELINE_UNSTAGED" -# Baseline untracked files — only files created *during* the loop should be staged +# Baseline untracked files: only files created *during* the loop should be staged BASELINE_UNTRACKED="$TMPDIR_REVIEW/baseline-untracked.txt" snapshot_untracked > "$BASELINE_UNTRACKED" @@ -269,7 +270,7 @@ if [[ $local_exit -ne 0 ]]; then fi if [[ ! -f "$REVIEW_FILE" ]]; then - write_status "No review file created — $REVIEWER_AGENT may have failed" "$RED" + write_status "No review file created; $REVIEWER_AGENT may have failed" "$RED" echo "" echo -e "${MAGENTA}========================================${NC}" echo -e "${MAGENTA} Code Review Loop Complete${NC}" @@ -291,7 +292,7 @@ iteration=0 while [[ $iteration -lt $MAX_ITERATIONS ]]; do if test_review_clean; then echo "" - write_status "Review is clean — no High or Medium issues!" "$GREEN" + write_status "Review is clean: no High or Medium issues!" "$GREEN" break fi @@ -346,7 +347,7 @@ IMPORTANT: You MUST overwrite agent-code-review.md with your updated findings." cleanup_agent_artifacts "$REVIEWER_AGENT" "$pre_snapshot" "reviewer" if [[ $local_exit -ne 0 ]]; then - write_status "$REVIEWER_AGENT follow-up review failed (exit $local_exit) — aborting review loop" "$YELLOW" + write_status "$REVIEWER_AGENT follow-up review failed (exit $local_exit), aborting review loop" "$YELLOW" break fi @@ -391,6 +392,9 @@ else fi write_step "Final" "$EDITOR_AGENT: Writing improvement summary" +# Clear any summary left by a previous run so the post-run existence check +# below reflects this run, not a stale artifact +rm -f "$SUMMARY_FILE" 2>/dev/null || true summary_prompt="Write a narrative summary of code improvements to the file agent-review-summary.md in the project root. @@ -399,7 +403,7 @@ Final status: $final_status Editor agent: $EDITOR_AGENT | Reviewer agent: $REVIEWER_AGENT ## Initial Staged Diff (before review loop) -This is what was staged BEFORE the review loop started — the original author's work: +This is what was staged BEFORE the review loop started, the original author's work: $initial_diff Full diff available at: $initial_diff_file @@ -418,7 +422,7 @@ $review_created_files ## Instructions for the summary -Your job is to summarize what the CODE REVIEW LOOP changed — NOT the original authored code. +Your job is to summarize what the CODE REVIEW LOOP changed, NOT the original authored code. Compare the initial diff (before) with the final diff (after) to identify what the review cycle added, fixed, or improved on top of the original work. Write agent-review-summary.md with this structure: @@ -435,6 +439,13 @@ Be concise and focus on the substance of review-driven improvements, not the ori local_exit=0 run_agent "$EDITOR_AGENT" "$summary_prompt" "Read,Write,Grep,Glob" || local_exit=$? +if [[ $local_exit -ne 0 ]]; then + write_status "$EDITOR_AGENT summary generation exited with code $local_exit" "$YELLOW" +fi +if [[ ! -f "$SUMMARY_FILE" ]]; then + write_status "No summary file created; $EDITOR_AGENT may have failed" "$YELLOW" +fi + # ---- Cleanup intermediate files ----------------------------------------- # Only remove the review file when truly clean with zero issues at all severities if $is_clean && [[ -f "$REVIEW_FILE" ]]; then @@ -460,7 +471,11 @@ echo " Editor : $EDITOR_AGENT" echo " Reviewer : $REVIEWER_AGENT" echo "" [[ -f "$REVIEW_FILE" ]] && echo -e " Review : agent-code-review.md" -echo -e " Summary : agent-review-summary.md" +if [[ -f "$SUMMARY_FILE" ]]; then + echo -e " Summary : agent-review-summary.md" +else + echo -e " Summary : ${YELLOW}not created${NC}" +fi echo "" if [[ -n "$STASH_REF" ]]; then # Resolve which stash@{N} entry corresponds to our saved hash diff --git a/bin/plan-review-loop b/bin/plan-review-loop index 229398a..2771d4a 100755 --- a/bin/plan-review-loop +++ b/bin/plan-review-loop @@ -85,6 +85,7 @@ fi # ---- paths --------------------------------------------------------------- PROJECT_ROOT="$(pwd)" FEEDBACK_FILE="$PROJECT_ROOT/feedback-plan.md" +SUMMARY_FILE="$PROJECT_ROOT/plan-review-summary.md" if [[ "$PLAN_FILE" == /* || "$PLAN_FILE" == ?:* ]]; then PLAN_FILE_PATH="$PLAN_FILE" else @@ -160,7 +161,7 @@ if [[ $local_exit -ne 0 ]]; then fi if [[ ! -f "$FEEDBACK_FILE" ]]; then - write_status "No feedback file created — $REVIEWER_AGENT may have failed" "$RED" + write_status "No feedback file created; $REVIEWER_AGENT may have failed" "$RED" echo "" echo -e "${MAGENTA}========================================${NC}" echo -e "${MAGENTA} Plan Review Loop Complete${NC}" @@ -178,7 +179,7 @@ iteration_data+="$feedback_content"$'\n' # ---- Feedback loop ------------------------------------------------------- while [[ $reviewer_iterations -lt $MAX_ITERATIONS ]]; do if test_reviewer_satisfied; then - write_status "$REVIEWER_AGENT is satisfied — no further feedback!" "$GREEN" + write_status "$REVIEWER_AGENT is satisfied: no further feedback!" "$GREEN" reviewer_satisfied=true break fi @@ -236,7 +237,7 @@ IMPORTANT: Write ALL output (feedback or NO_FURTHER_FEEDBACK) to the file feedba cleanup_agent_artifacts "$REVIEWER_AGENT" "$pre_snapshot" "reviewer" if [[ $local_exit -ne 0 ]]; then - write_status "$REVIEWER_AGENT follow-up failed (exit $local_exit) — aborting review loop" "$YELLOW" + write_status "$REVIEWER_AGENT follow-up failed (exit $local_exit), aborting review loop" "$YELLOW" break fi @@ -273,6 +274,9 @@ write_status "Plan diff saved for summary agent" "$DIM" # ---- Editor writes the improvement summary -------------------------------- write_step "Final" "$EDITOR_AGENT: Writing plan improvement summary" +# Clear any summary left by a previous run so the post-run existence check +# below reflects this run, not a stale artifact +rm -f "$SUMMARY_FILE" 2>/dev/null || true if $reviewer_satisfied; then reviewer_status_label="satisfied" @@ -304,7 +308,7 @@ $final_feedback ## Instructions for the summary -Your job is to summarize what the REVIEW LOOP changed in the plan — use the diff above as the primary source of truth. +Your job is to summarize what the REVIEW LOOP changed in the plan; use the diff above as the primary source of truth. Write plan-review-summary.md with this structure: @@ -320,6 +324,13 @@ Do not create any other files. Do not stage or commit anything." local_exit=0 run_agent "$EDITOR_AGENT" "$summary_prompt" "Read,Write,Grep,Glob" || local_exit=$? +if [[ $local_exit -ne 0 ]]; then + write_status "$EDITOR_AGENT summary generation exited with code $local_exit" "$YELLOW" +fi +if [[ ! -f "$SUMMARY_FILE" ]]; then + write_status "No summary file created; $EDITOR_AGENT may have failed" "$YELLOW" +fi + # ---- Cleanup intermediate files ----------------------------------------- # Only remove feedback file when it contains no valid content (NO_FURTHER_FEEDBACK) if $reviewer_satisfied; then @@ -334,9 +345,9 @@ echo -e "${MAGENTA}========================================${NC}" echo "" if $reviewer_satisfied; then - echo -e " Reviewer : ${GREEN}$REVIEWER_AGENT — SATISFIED (NO_FURTHER_FEEDBACK) [$reviewer_iterations iteration(s)]${NC}" + echo -e " Reviewer : ${GREEN}$REVIEWER_AGENT: SATISFIED (NO_FURTHER_FEEDBACK) [$reviewer_iterations iteration(s)]${NC}" else - echo -e " Reviewer : ${YELLOW}$REVIEWER_AGENT — MAX ITERATIONS ($MAX_ITERATIONS) [$reviewer_iterations iteration(s)]${NC}" + echo -e " Reviewer : ${YELLOW}$REVIEWER_AGENT: MAX ITERATIONS ($MAX_ITERATIONS) [$reviewer_iterations iteration(s)]${NC}" fi echo "" @@ -344,6 +355,10 @@ echo " Duration : $elapsed_display" echo " Editor : $EDITOR_AGENT" echo -e " Plan : $PLAN_FILE" [[ -f "$FEEDBACK_FILE" ]] && echo -e " Feedback : feedback-plan.md" -echo -e " Summary : plan-review-summary.md" +if [[ -f "$SUMMARY_FILE" ]]; then + echo -e " Summary : plan-review-summary.md" +else + echo -e " Summary : ${YELLOW}not created${NC}" +fi echo "" echo -e "${CYAN}Review the improved plan and summary.${NC}" diff --git a/lib/lib-review-loop b/lib/lib-review-loop index 87af413..67f8bd7 100644 --- a/lib/lib-review-loop +++ b/lib/lib-review-loop @@ -40,7 +40,7 @@ PROMPTS_DIR="${AI_CODING_SETUP_PROMPTS_DIR:-$HOME/.local/share/ai-coding-setup/p # Valid agent names for --editor / --reviewer flags VALID_AGENTS="claude codex copilot antigravity kimi" -# Filenames that review loops create — never treat as agent artifacts +# Filenames that review loops create; never treat as agent artifacts KNOWN_REVIEW_FILES="agent-code-review.md agent-review-summary.md feedback-plan.md plan-review-summary.md" # ---- output helpers ------------------------------------------------------- @@ -83,7 +83,7 @@ read_prompt_file() { # Load agent defaults from ~/.ai-coding-setup.conf. # Only sets EDITOR_AGENT / REVIEWER_AGENT if not already overridden by CLI. -# Uses safe line-by-line parsing — never sources the file directly. +# Uses safe line-by-line parsing; never sources the file directly. # shellcheck disable=SC2034 # variables are used by sourcing scripts load_config() { local config_file="$HOME/.ai-coding-setup.conf" @@ -123,8 +123,8 @@ validate_agent_name() { # ---- input validation ---------------------------------------------------- # Validate that a value is a positive integer (for --max-iterations). -# $1 — value to check -# $2 — flag name (for error message) +# $1: value to check +# $2: flag name (for error message) validate_positive_int() { local value="$1" flag="$2" if ! [[ "$value" =~ ^[1-9][0-9]*$ ]]; then @@ -137,11 +137,11 @@ validate_positive_int() { # Run Claude Code with a prompt and optional tool restrictions. # Disables all MCP servers to keep autonomous runs scoped to local work. -# Pipes the prompt via stdin — passing it as an argv on Windows/MSYS hits the -# OS exec limit (~32 KB) and fails with "Argument list too long" for large +# Pipes the prompt via stdin, since passing it as an argv on Windows/MSYS hits +# the OS exec limit (~32 KB) and fails with "Argument list too long" for large # prompts (e.g. the final review-loop summary). -# $1 — prompt text -# $2 — comma-separated allowed tools (default: Edit,Read,Write,Bash,Grep,Glob) +# $1: prompt text +# $2: comma-separated allowed tools (default: Edit,Read,Write,Bash,Grep,Glob) run_claude() { local prompt="$1" local tools="${2:-Edit,Read,Write,Bash,Grep,Glob}" @@ -151,7 +151,7 @@ run_claude() { # Run Codex CLI with a prompt. Wraps the prompt with a preamble that prevents # conversational output, and disables MCP servers to avoid artifact creation. -# $1 — prompt text +# $1: prompt text run_codex() { local prompt="$1" local wrapped="Execute the following task now. Do not introduce yourself. Begin immediately. @@ -166,7 +166,7 @@ $prompt" # -p value disables stdin reading, and whitespace-only -p values are rejected # as an empty prompt. A piped stdin prompt alone still enters print mode. # --print-timeout raises the 5m print-mode default; reviews can run longer. -# $1 — prompt text +# $1: prompt text run_antigravity() { local prompt="$1" printf '%s' "$prompt" | agy --dangerously-skip-permissions --add-dir "$PWD" --print-timeout 30m @@ -180,9 +180,9 @@ run_antigravity() { # hits the OS exec limit (~32 KB) for very large prompts (e.g. the final # review-loop summary) and fails with "Argument list too long". Unlike the # other agents, Copilot does not currently support reading the prompt from -# stdin — `copilot -p` ignores stdin, and bare stdin is interpreted as CLI +# stdin: `copilot -p` ignores stdin, and bare stdin is interpreted as CLI # options rather than as a prompt. See github/copilot-cli#683 and #1046. -# $1 — prompt text +# $1: prompt text run_copilot() { local prompt="$1" local -a mcp_flags=(--disable-builtin-mcps) @@ -199,15 +199,15 @@ run_copilot() { # `-p` is already fully unattended: kimi rejects --yolo/--auto alongside # --prompt because non-interactive runs use auto permission mode by default. # -# KNOWN LIMITATION: as with Copilot, the prompt is passed as an argv — kimi's -# -p takes the prompt as a flag value and has no stdin form — so on Windows/MSYS +# KNOWN LIMITATION: as with Copilot, the prompt is passed as an argv. kimi's +# -p takes the prompt as a flag value and has no stdin form, so on Windows/MSYS # very large prompts (e.g. the final review-loop summary) can hit the OS exec # limit (~32 KB) and fail with "Argument list too long". # # Kimi has no flag to disable configured MCP servers for a single run, so # unlike the other agents an autonomous run still loads whatever is in # ~/.kimi-code/mcp.json. -# $1 — prompt text +# $1: prompt text run_kimi() { local prompt="$1" kimi -p "$prompt" @@ -215,7 +215,7 @@ run_kimi() { # Map an agent name to its CLI command (most agents share the name, but # Antigravity ships as `agy`). -# $1 — agent name +# $1: agent name agent_command() { case "$1" in antigravity) echo "agy" ;; @@ -224,9 +224,9 @@ agent_command() { } # Dispatch to the appropriate agent runner. -# $1 — agent name ("claude", "codex", "copilot", "antigravity", or "kimi") -# $2 — prompt text -# $3 — allowed tools (only used by Claude; ignored by others) +# $1: agent name ("claude", "codex", "copilot", "antigravity", or "kimi") +# $2: prompt text +# $3: allowed tools (only used by Claude; ignored by others) run_agent() { local agent="$1" prompt="$2" tools="${3:-}" case "$agent" in @@ -261,7 +261,7 @@ validate_tools() { } # Validate that required prompt files exist. -# $@ — list of prompt file paths to check +# $@: list of prompt file paths to check validate_prompts() { local missing=() local f @@ -291,9 +291,12 @@ test_review_clean() { local content content=$(<"$REVIEW_FILE") - # Only an explicit "Verdict: good to go" line signals clean — a bare - # substring match could false-positive on "not good to go" - if echo "$content" | grep -qiE "verdict[[:space:]]*:[[:space:]]*good to go"; then + # Only a verdict line of its own signals clean. Emphasis is stripped so + # "**Verdict: good to go**" and "**Verdict:** good to go" both match; the + # anchors keep both quoting prose ("the report says Verdict: good to go") + # and hedged prose ("Verdict: good to go, but ...") from counting. + if printf '%s\n' "$content" | tr -d '*' \ + | grep -qiE '^[[:space:]]*([-+][[:space:]]*)?verdict[[:space:]]*:[[:space:]]*good to go[[:space:]]*[.!]?[[:space:]]*$'; then return 0 fi @@ -328,11 +331,11 @@ test_reviewer_satisfied() { } # Build the improvement prompt for the plan editor. -# $1 — plan file path -# $2 — feedback file path -# $3 — reviewer agent name -# $4 — current cycle number -# $5 — max cycles +# $1: plan file path +# $2: feedback file path +# $3: reviewer agent name +# $4: current cycle number +# $5: max cycles build_improvement_prompt() { local plan_path="$1" feedback_path="$2" reviewer_name="$3" cycle="$4" max_cycles="$5" cat </dev/null; then - print_warning "$cli_command command not found — skipping MCP configuration" + print_warning "$cli_command command not found, skipping MCP configuration" return fi if ! command -v npx &>/dev/null; then - print_warning "npx not found — skipping MCP servers (install Node.js)" + print_warning "npx not found, skipping MCP servers (install Node.js)" return fi @@ -316,7 +316,7 @@ configure_mcp() { echo -e " ${DIM}MCP servers already configured${NC}" else echo "" - echo " MCP servers to configure (user scope — available in all projects):" + echo " MCP servers to configure (user scope, available in all projects):" echo "" display_mcp_list "${missing_mcp[@]}" echo "" @@ -445,7 +445,7 @@ install_commands() { if $dest_exists; then if has_marker "$marker" "$dest_file"; then if is_up_to_date "$src_file" "$dest_file" "$marker"; then - echo -e " ${DIM}${label} — up to date${NC}" + echo -e " ${DIM}${label}: up to date${NC}" ((++current)) else if [[ "$mode" == skill ]]; then @@ -470,7 +470,7 @@ install_commands() { print_success "Overwritten ${label} (was custom)" ((++installed)) else - print_warning "${label} exists (custom) — skipped" + print_warning "${label} exists (custom), skipped" ((++skipped)) fi fi @@ -558,7 +558,7 @@ configure_claude_settings() { fi if ! jq empty "$settings_file" 2>/dev/null; then - print_warning "$HOME/.claude/settings.json is not valid JSON — skipping" + print_warning "$HOME/.claude/settings.json is not valid JSON, skipping" return fi @@ -611,7 +611,7 @@ configure_claude_settings() { # ── Nothing to do? ────────────────────────────────────────── if [[ ${#feature_changes[@]} -eq 0 ]] && [[ ${#missing_perms[@]} -eq 0 ]]; then - echo -e " ${DIM}Settings already configured — nothing to do${NC}" + echo -e " ${DIM}Settings already configured, nothing to do${NC}" return fi @@ -622,12 +622,12 @@ configure_claude_settings() { echo "" for change in "${feature_changes[@]}"; do case "$change" in - webfetch) echo -e " • ${BOLD}WebFetch${NC} — allow web page fetching without prompting" ;; - websearch) echo -e " • ${BOLD}WebSearch${NC} — allow web searches without prompting" ;; - attribution) echo -e " • ${BOLD}attribution${NC} — disable commit/PR/session attribution tags" ;; - teams) echo -e " • ${BOLD}CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS=1${NC} — enable agent teams" ;; - noflicker) echo -e " • ${BOLD}CLAUDE_CODE_NO_FLICKER=1${NC} — flicker-free fullscreen rendering" ;; - powershell) echo -e " • ${BOLD}CLAUDE_CODE_USE_POWERSHELL_TOOL=1${NC} — enable native PowerShell tool" ;; + webfetch) echo -e " • ${BOLD}WebFetch${NC}: allow web page fetching without prompting" ;; + websearch) echo -e " • ${BOLD}WebSearch${NC}: allow web searches without prompting" ;; + attribution) echo -e " • ${BOLD}attribution${NC}: disable commit/PR/session attribution tags" ;; + teams) echo -e " • ${BOLD}CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS=1${NC}: enable agent teams" ;; + noflicker) echo -e " • ${BOLD}CLAUDE_CODE_NO_FLICKER=1${NC}: flicker-free fullscreen rendering" ;; + powershell) echo -e " • ${BOLD}CLAUDE_CODE_USE_POWERSHELL_TOOL=1${NC}: enable native PowerShell tool" ;; esac done echo "" @@ -705,7 +705,7 @@ configure_codex_settings() { fi if [[ ${#changes[@]} -eq 0 ]]; then - echo -e " ${DIM}Settings already configured — nothing to do${NC}" + echo -e " ${DIM}Settings already configured, nothing to do${NC}" return fi @@ -714,8 +714,8 @@ configure_codex_settings() { echo "" for change in "${changes[@]}"; do case "$change" in - remove_collab) echo -e " • ${BOLD}Remove collab${NC} — deprecated, replaced by multi_agent" ;; - add_multi_agent) echo -e " • ${BOLD}multi_agent = true${NC} — enable multi-agent collaboration" ;; + remove_collab) echo -e " • ${BOLD}Remove collab${NC}: deprecated, replaced by multi_agent" ;; + add_multi_agent) echo -e " • ${BOLD}multi_agent = true${NC}: enable multi-agent collaboration" ;; esac done echo "" @@ -763,7 +763,7 @@ configure_copilot_settings() { fi if ! jq empty "$config_file" 2>/dev/null; then - print_warning "$HOME/.copilot/config.json is not valid JSON — skipping" + print_warning "$HOME/.copilot/config.json is not valid JSON, skipping" return fi @@ -774,8 +774,8 @@ configure_copilot_settings() { # # The cli/cli repo publishes an agent skill named `gh` that teaches coding # agents how to drive the GitHub CLI (structured output, pagination, repo -# targeting, search vs list, gh api fallback). Offer to install it — or update -# it if already present — for a selected agent. This is an upstream skill, +# targeting, search vs list, gh api fallback). Offer to install it (or update +# it if already present) for a selected agent. This is an upstream skill, # unrelated to the commands/skills this repo installs. # True when this gh is new enough to have the `gh skill` command (preview). @@ -803,7 +803,7 @@ configure_gh_skill() { [[ -z "$agent_id" ]] && return echo "" - echo -e "${BOLD}── ${display_name} — gh agent skill ──${NC}" + echo -e "${BOLD}── ${display_name}: gh agent skill ──${NC}" # Already installed for this agent? (list is scoped by --agent) local count @@ -812,11 +812,11 @@ configure_gh_skill() { [[ "$count" =~ ^[0-9]+$ ]] || count=0 if [[ "$count" -gt 0 ]]; then - echo -e " ${DIM}gh agent skill already installed — checking for updates${NC}" + echo -e " ${DIM}gh agent skill already installed, checking for updates${NC}" if gh skill update gh; then print_success "gh agent skill up to date" else - print_warning "gh skill update failed — see output above" + print_warning "gh skill update failed, see output above" fi return fi @@ -952,11 +952,11 @@ install_scripts() { local target target=$(readlink "$dest" 2>/dev/null) || true if [[ "$target" == "$abs_src" ]]; then - echo -e " ${DIM}${name} — up to date (symlinked)${NC}" + echo -e " ${DIM}${name}: up to date (symlinked)${NC}" ((++current)) continue fi - # Stale symlink (old checkout path) — remove and re-link below + # Stale symlink (old checkout path); remove and re-link below rm -f "$dest" local is_update=true fi @@ -965,7 +965,7 @@ install_scripts() { # Check if we manage it (has our marker comment) if head -5 "$dest" | grep -qF "source: ai-coding-setup" 2>/dev/null; then if diff -q "$abs_src" "$dest" &>/dev/null; then - echo -e " ${DIM}${name} — up to date (copied)${NC}" + echo -e " ${DIM}${name}: up to date (copied)${NC}" ((++current)) continue fi @@ -976,7 +976,7 @@ install_scripts() { if $FORCE; then rm -f "$dest" else - print_warning "${name} exists in $dest_dir (custom) — skipped" + print_warning "${name} exists in $dest_dir (custom), skipped" ((++skipped)) continue fi @@ -1056,8 +1056,8 @@ if [[ ${#missing[@]} -gt 0 ]]; then echo "" for tool in "${missing[@]}"; do case "$tool" in - gh) echo " gh — GitHub CLI: brew install gh (https://cli.github.com/)" ;; - jq) echo " jq — JSON processor: brew install jq (https://jqlang.github.io/jq/)" ;; + gh) echo " gh (GitHub CLI): brew install gh (https://cli.github.com/)" ;; + jq) echo " jq (JSON processor): brew install jq (https://jqlang.github.io/jq/)" ;; esac done echo "" @@ -1228,7 +1228,7 @@ if [[ -d "prompts" ]]; then _target=$(readlink "$_dest" 2>/dev/null) || true [[ "$_target" != "$_src" ]] && ln -sf "$_src" "$_dest" elif [[ -f "$_dest" ]]; then - # Plain file — only replace if it carries our marker (previous + # Plain file: only replace if it carries our marker (previous # copy-based install) or --force is set; otherwise preserve user edits. if has_marker "$MD_MARKER" "$_dest" || $FORCE; then if ln -sf "$_src" "$_dest" 2>/dev/null && [[ -L "$_dest" ]]; then :; else @@ -1236,7 +1236,7 @@ if [[ -d "prompts" ]]; then has_marker "$MD_MARKER" "$_dest" || printf '\n%s\n' "$MD_MARKER" >> "$_dest" fi else - print_warning "$_name exists (custom) — skipped (use --force to overwrite)" + print_warning "$_name exists (custom), skipped (use --force to overwrite)" fi else if ln -sf "$_src" "$_dest" 2>/dev/null && [[ -L "$_dest" ]]; then :; else @@ -1270,7 +1270,7 @@ if [[ -f "$_prompts_dest/plan-review.md" ]] \ fi if [[ ${#review_loop_scripts[@]} -gt 0 ]]; then - # Install the shared library to ~/.local/lib/ (not bin/ — it's sourced, not executed) + # Install the shared library to ~/.local/lib/ (not bin/, since it's sourced, not executed) _lib_dest="$HOME/.local/lib" _lib_src="$(cd "$(dirname "lib/lib-review-loop")" && pwd)/lib-review-loop" mkdir -p "$_lib_dest" @@ -1289,7 +1289,7 @@ if [[ ${#review_loop_scripts[@]} -gt 0 ]]; then install_scripts "bin" "${review_loop_scripts[@]}" elif [[ -d "bin" ]]; then echo "" - echo -e " ${DIM}Review loop scripts require shared prompts — skipped${NC}" + echo -e " ${DIM}Review loop scripts require shared prompts, skipped${NC}" fi # ---- create default config file if missing --------------------------------- @@ -1317,7 +1317,7 @@ if [[ ${#review_loop_scripts[@]} -gt 0 ]] && [[ ! -f "$config_file" ]]; then echo "" if prompt_yes_no " Create config file with defaults?"; then cat > "$config_file" < "$REVIEW_FILE" + run test_review_clean + assert_success +} + +@test "plan-review prompt states the sentinel test_reviewer_satisfied detects" { + source_lib + grep -qF 'NO_FURTHER_FEEDBACK' "$PROJECT_ROOT/prompts/plan-review.md" \ + || fail "prompts/plan-review.md no longer names the NO_FURTHER_FEEDBACK sentinel" + FEEDBACK_FILE="$TEST_TMPDIR/feedback.md" + echo "NO_FURTHER_FEEDBACK" > "$FEEDBACK_FILE" + run test_reviewer_satisfied + assert_success +} + +@test "test_review_clean accepts the verdict as a Summary bullet" { + source_lib + REVIEW_FILE="$TEST_TMPDIR/review.md" + echo "- **Verdict: good to go**" > "$REVIEW_FILE" + run test_review_clean + assert_success +} + +@test "test_review_clean rejects a verdict hedged with trailing prose" { + source_lib + REVIEW_FILE="$TEST_TMPDIR/review.md" + echo "**Verdict: good to go, but 3 High findings remain**" > "$REVIEW_FILE" + run test_review_clean + assert_failure +} + +@test "test_review_clean accepts a verdict with a trailing period" { + source_lib + REVIEW_FILE="$TEST_TMPDIR/review.md" + echo "**Verdict: good to go**." > "$REVIEW_FILE" + run test_review_clean + assert_success +} + +@test "test_review_clean ignores a verdict quoted inside prose" { + source_lib + REVIEW_FILE="$TEST_TMPDIR/review.md" + echo "The loop stops once the report says Verdict: good to go." > "$REVIEW_FILE" + run test_review_clean + assert_failure +} + @test "build_improvement_prompt includes all parameters" { source_lib run build_improvement_prompt "/path/to/plan.md" "/path/to/feedback.md" "codex" "2" "5" diff --git a/test/plan-review-loop.bats b/test/plan-review-loop.bats index 8b9cf5a..794c2d5 100644 --- a/test/plan-review-loop.bats +++ b/test/plan-review-loop.bats @@ -1,5 +1,5 @@ #!/usr/bin/env bats -# Tests for bin/plan-review-loop — argument parsing and validation. +# Tests for bin/plan-review-loop: argument parsing and validation. load test_helper diff --git a/test/smoke b/test/smoke index 09d1fae..2d6425d 100755 --- a/test/smoke +++ b/test/smoke @@ -1,6 +1,6 @@ #!/usr/bin/env bash # --------------------------------------------------------------------------- -# Smoke tests — run real AI agents to verify flag acceptance and basic I/O. +# Smoke tests: run real AI agents to verify flag acceptance and basic I/O. # # Tests each installed agent as both editor (can it modify a file?) and # reviewer (can it produce a review file?). Uses a temporary git repo with @@ -88,7 +88,7 @@ if [[ ${#requested_agents[@]} -gt 0 ]]; then fi done if ! $found; then - echo "WARNING: '$req' is not installed — skipping" + echo "WARNING: '$req' is not installed, skipping" fi done else @@ -152,12 +152,12 @@ FAIL=0 RESULTS=() # Run a single smoke test. -# $1 — test name (for display) -# $2 — agent name -# $3 — prompt text -# $4 — tools (comma-separated, for Claude; ignored by others) -# $5 — verification function name (called after agent completes) -# $6 — fixture directory (agent runs here) +# $1: test name (for display) +# $2: agent name +# $3: prompt text +# $4: tools (comma-separated, for Claude; ignored by others) +# $5: verification function name (called after agent completes) +# $6: fixture directory (agent runs here) run_smoke_test() { local name="$1" agent="$2" prompt="$3" tools="$4" verify_fn="$5" fixture="$6" @@ -216,8 +216,8 @@ run_smoke_test() { # Run the verification function local verify_result verify_result=$( "$verify_fn" "$fixture" 2>&1 ) || { - echo -e " ${RED}FAIL${NC} $name (${elapsed}s) — $verify_result" - RESULTS+=("FAIL $name — $verify_result") + echo -e " ${RED}FAIL${NC} $name (${elapsed}s): $verify_result" + RESULTS+=("FAIL $name: $verify_result") ((FAIL++)) return 1 }