diff --git a/.github/skill-tests/test_merge.sh b/.github/skill-tests/test_merge.sh index b878722d..ad797f97 100644 --- a/.github/skill-tests/test_merge.sh +++ b/.github/skill-tests/test_merge.sh @@ -274,4 +274,52 @@ test_detect_allowed_methods_requires_repo_slug() { assert_contains "$_CAPTURED" "REPO_SLUG not set" "should report missing REPO_SLUG" } +test_format_merge_commit_with_headline() { + _source_merge + assert_eq "$(format_merge_commit abcdef1234567 "Add thing")" "abcdef1 Add thing" "should shorten oid and append headline" +} + +test_format_merge_commit_null_headline() { + _source_merge + assert_eq "$(format_merge_commit abcdef1234567 null)" "abcdef1" "should drop a null headline" + assert_eq "$(format_merge_commit abcdef1234567 "")" "abcdef1" "should drop an empty headline" +} + +test_wait_for_checks_skipped_with_flag() { + _source_merge + WAIT_CHECKS=false + capture wait_for_checks + assert_eq "$_CAPTURED_EXIT" "0" "should skip the gate when WAIT_CHECKS=false" +} + +test_wait_for_checks_refuses_failing() { + _source_merge + WAIT_CHECKS=true + PR_NUMBER=1 + count_required_checks() { [[ "$1" == "fail" ]] && echo 2 || echo 0; } + capture wait_for_checks + assert_eq "$_CAPTURED_EXIT" "5" "should exit 5 on failing required checks" + assert_contains "$_CAPTURED" "failing required check" "should report failing checks" +} + +test_wait_for_checks_times_out_on_pending() { + _source_merge + WAIT_CHECKS=true + PR_NUMBER=1 + CHECK_TIMEOUT=0 + count_required_checks() { [[ "$1" == "pending" ]] && echo 1 || echo 0; } + capture wait_for_checks + assert_eq "$_CAPTURED_EXIT" "5" "should exit 5 when checks stay pending" + assert_contains "$_CAPTURED" "pending required check" "should report pending checks" +} + +test_wait_for_checks_passes_when_clear() { + _source_merge + WAIT_CHECKS=true + PR_NUMBER=1 + count_required_checks() { echo 0; } + capture wait_for_checks + assert_eq "$_CAPTURED_EXIT" "0" "should pass when no checks fail or pend" +} + run_tests diff --git a/agents/skills/cross-agent/SKILL.md b/agents/skills/cross-agent/SKILL.md index 0188d61a..2ad4b695 100644 --- a/agents/skills/cross-agent/SKILL.md +++ b/agents/skills/cross-agent/SKILL.md @@ -40,7 +40,7 @@ Scan for existing skills in all locations: 1. `.agents/skills/*/SKILL.md` (already canonical) 2. `.claude/commands/*.md` (Claude flat commands) 3. `.claude/skills/*/SKILL.md` (Claude directory skills) -Also check for a lowercase `claude.md` — Claude Code requires `CLAUDE.md` (uppercase) for discovery. Use `ls -1 | grep '^claude\.md$'` to detect the wrong casing (a plain `test -f` is unreliable on case-insensitive filesystems). +Also check for a lowercase `claude.md` — Claude Code requires `CLAUDE.md` (uppercase) for discovery. Use `command ls -1 | grep '^claude\.md$'` to detect the wrong casing (a plain `test -f` is unreliable on case-insensitive filesystems). Classify the repo: @@ -301,4 +301,4 @@ Present a final summary: - **CLAUDE.md has significant content**: Merge into AGENTS.md before symlinking. Show the user what was merged. - **Mixed state**: Some skills in .agents/skills/, some in .claude/commands/. Migrate only the ones not already canonical. - **Skill name collision**: If a skill exists in both .claude/commands/ and .agents/skills/ with different content, warn the user and skip. Do not overwrite canonical skills. -- **Lowercase `claude.md`**: Claude Code requires uppercase `CLAUDE.md`. If `claude.md` exists, rename it (`git mv` if tracked, `mv` otherwise) before proceeding with Step 7. Use `ls -1 | grep '^claude\.md$'` for detection — `test -f` is unreliable on case-insensitive filesystems (macOS default). +- **Lowercase `claude.md`**: Claude Code requires uppercase `CLAUDE.md`. If `claude.md` exists, rename it (`git mv` if tracked, `mv` otherwise) before proceeding with Step 7. Use `command ls -1 | grep '^claude\.md$'` for detection — `test -f` is unreliable on case-insensitive filesystems (macOS default). diff --git a/agents/skills/dream/SKILL.md b/agents/skills/dream/SKILL.md index 0cfbf6f2..8c5bbd7f 100644 --- a/agents/skills/dream/SKILL.md +++ b/agents/skills/dream/SKILL.md @@ -28,7 +28,7 @@ The only escape hatch is `--dry-run` for previewing what dream *would* do. - Argus KB available: !`command -v argus 2>/dev/null | head -1` - Recent changes since last dream run: !`tail -100 ~/.dots/sys/kb-changes/changes.jsonl 2>/dev/null | head -100` -- Last dream run: !`ls -t ~/.dots/sys/dream-runs 2>/dev/null | head -1` +- Last dream run: !`command ls -t ~/.dots/sys/dream-runs 2>/dev/null | head -1` - Today's date: !`date +%Y-%m-%d` ## MCP tool naming diff --git a/agents/skills/equip/SKILL.md b/agents/skills/equip/SKILL.md index 09293473..3c6514eb 100644 --- a/agents/skills/equip/SKILL.md +++ b/agents/skills/equip/SKILL.md @@ -16,8 +16,8 @@ If no arguments are provided, ask the user what to analyze. ## Context - Project root: !`pwd` -- Existing skills: !`ls agents/skills/ 2>/dev/null | head -40` -- Custom agents: !`ls agents/custom/ 2>/dev/null | head -20` +- Existing skills: !`command ls agents/skills/ 2>/dev/null | head -40` +- Custom agents: !`command ls agents/custom/ 2>/dev/null | head -20` - Skill descriptions: !`grep -r "^description:" agents/skills/*/SKILL.md 2>/dev/null | head -40` - Agent descriptions: !`grep "^description:" agents/custom/*.md 2>/dev/null | head -20` - Project type: !`find . -maxdepth 1 \( -name go.mod -o -name Gemfile -o -name package.json -o -name Cargo.toml -o -name pyproject.toml \) 2>/dev/null | head -3` diff --git a/agents/skills/handoff/SKILL.md b/agents/skills/handoff/SKILL.md index 1ad01e22..15aee4b0 100644 --- a/agents/skills/handoff/SKILL.md +++ b/agents/skills/handoff/SKILL.md @@ -18,7 +18,7 @@ Generate a structured prompt capturing the current conversation context so it ca ## Context - Calling task ID: !`echo $ARGUS_TASK_ID 2>/dev/null | head -1` -- Argus projects: !`ls -1 ~/.argus/worktrees/ 2>/dev/null | head -50` +- Argus projects: !`command ls -1 ~/.argus/worktrees/ 2>/dev/null | head -50` - Repo root: !`git rev-parse --show-toplevel 2>/dev/null | head -1` - Branch: !`git branch --show-current` - Base ref: !`git branch -r 2>/dev/null | grep -oE 'origin/(main|master)' | head -1` @@ -28,7 +28,7 @@ Generate a structured prompt capturing the current conversation context so it ca - Changed files vs main: !`git diff --name-only origin/main..HEAD 2>/dev/null | head -30` - Changed files vs master: !`git diff --name-only origin/master..HEAD 2>/dev/null | head -30` - KB usage restriction in this repo: !`grep -liE 'never use argus-kb|argus-kb.*sandbox|sandboxed away from external systems' CLAUDE.md AGENTS.md 2>/dev/null | head -3` -- Repo-local context/ directory present: !`ls -d context 2>/dev/null | head -1` +- Repo-local context/ directory present: !`command ls -d context 2>/dev/null | head -1` ## Instructions diff --git a/agents/skills/improve/SKILL.md b/agents/skills/improve/SKILL.md index a47b5a94..4312be0a 100644 --- a/agents/skills/improve/SKILL.md +++ b/agents/skills/improve/SKILL.md @@ -22,7 +22,7 @@ Run `/improve` at the end of any session where: - Current repo: !`git rev-parse --show-toplevel 2>/dev/null | head -1` - Skills directory: !`find agents/skills -maxdepth 2 -name SKILL.md 2>/dev/null | head -30` -- Context directory exists: !`ls context/knowledge/index.md 2>/dev/null | head -1` +- Context directory exists: !`command ls context/knowledge/index.md 2>/dev/null | head -1` - Knowledge base index: !`cat context/knowledge/index.md 2>/dev/null | head -30` - Context directory structure: !`find context -type f 2>/dev/null | head -20` - Voice profile: !`find . -maxdepth 3 -name 'voice-profile.md' -o -name 'VOICE.md' -o -name 'voice.md' 2>/dev/null | head -5` diff --git a/agents/skills/merge/SKILL.md b/agents/skills/merge/SKILL.md index 0267e643..a235e7c0 100644 --- a/agents/skills/merge/SKILL.md +++ b/agents/skills/merge/SKILL.md @@ -77,9 +77,11 @@ Resolve the script path — use the first that exists: 2. `agents/skills/merge/scripts/merge.sh` (repo-relative, for development/workspaces) ``` -bash [--method ] [--keep-branch] "" "<body>" +bash <script-path> [--method <squash|merge|rebase>] [--keep-branch] [--no-wait-checks] "<title>" "<body>" ``` +The script waits for pending required checks (default 1200s, override with the MERGE_CHECK_TIMEOUT env var in seconds) and refuses to merge when a required check is failing. Pass `--no-wait-checks` only when the user explicitly wants to skip that gate. + #### Choosing a merge method The script defaults to **squash** (collapses all commits into one). Pick a different method when squash would lose intentional structure: @@ -97,7 +99,7 @@ After a successful **squash** merge, the script auto-switches the current worktr - `--method merge` / `--method rebase` — auto-switch is **off** by default. Both methods preserve the original commits, so the feature branch may still be useful for stacked PRs or chained follow-on work. - `--keep-branch` — opt out of auto-switch on squash. The worktree stays on the feature branch and the script prints a divergence warning instead. Use this for stacked-PR workflows that squash but still need to keep the branch checked out (rare). -If auto-switch fails (e.g., uncommitted changes in the worktree, or `master` is checked out in another worktree), the script falls back to a loud warning rather than aborting — the merge itself already succeeded. +If the default branch is checked out in another worktree, the script falls back to `git checkout --detach <remote>/<default>`, leaving this worktree on a detached HEAD at the new tip (create a new branch before committing). If that also fails (e.g., uncommitted changes), it prints a loud warning listing both options — `git checkout --detach <remote>/<default>`, or `git reset --hard <remote>/<default>` to reuse the branch name (discards the branch commits, so only after the merge landed) — rather than aborting, since the merge itself already succeeded. #### Review-required repos: automatic `--admin` escalation @@ -135,4 +137,5 @@ On a repo where you hold admin, `/merge` lands the PR through the review gate it - If the admin error mentions pending/required checks (or the PR still has checks running), tell the user: "PR is blocked by a still-pending required status check, which prevented even the admin merge. Wait for checks to finish, then re-run /merge." Re-running once checks are green typically lands it. - If the admin error actually indicates missing permissions (e.g. "must have admin rights", "not authorized"), tell the user: "PR requires an approving review and I lack admin on this repo to bypass it. Ask a reviewer to approve, then re-run /merge." - Otherwise, surface the literal admin error to the user verbatim and suggest the matching next step. +- **Exit 5 (required checks)** — A required check is failing, or still pending after the wait timeout. For failing checks, report which ones (`gh pr checks <number>`) and fix them first. For a timeout, tell the user the checks are still running and to re-run /merge once they finish (or raise MERGE_CHECK_TIMEOUT). - **Exit 1** — Report the error from stderr. diff --git a/agents/skills/merge/scripts/merge.sh b/agents/skills/merge/scripts/merge.sh index 71ac5723..16455348 100755 --- a/agents/skills/merge/scripts/merge.sh +++ b/agents/skills/merge/scripts/merge.sh @@ -3,7 +3,7 @@ set -euo pipefail # merge.sh — Merge current branch to the default branch via GitHub PR # -# Usage: merge.sh [--skip-rebase] [--method <squash|merge|rebase>] [--squash|--merge|--rebase] [--keep-branch] "<title>" "<body>" +# Usage: merge.sh [--skip-rebase] [--no-wait-checks] [--method <squash|merge|rebase>] [--squash|--merge|--rebase] [--keep-branch] "<title>" "<body>" # # Without an explicit method, merge.sh probes the repo's allowed methods and # tries them in preference order (squash → rebase → merge). Pass --method (or @@ -22,6 +22,10 @@ set -euo pipefail # 2 — rebase conflict (needs manual resolution) # 3 — no commits to merge # 4 — PR blocked (review required / changes requested) and auto-merge unavailable +# 5 — required checks failing, or still pending after the wait timeout +# +# Before merging, required checks are awaited (default 1200s, override with +# MERGE_CHECK_TIMEOUT=<seconds>). Pass --no-wait-checks to skip the gate. COAUTHOR="Co-Authored-By: Claude <noreply@anthropic.com>" @@ -44,6 +48,9 @@ PR_REVIEW_DECISION="" LAST_GH_ERR="" MERGE_METHOD_FLAG="" # input flag (squash|merge|rebase|""); narrows ALLOWED_METHODS when set KEEP_BRANCH=false # when true, restore checkout to feature branch after squash merge +WAIT_CHECKS=true # when false, skip the required-checks gate +CHECK_TIMEOUT="${MERGE_CHECK_TIMEOUT:-1200}" +CHECK_POLL_INTERVAL="${MERGE_CHECK_POLL_INTERVAL:-15}" # --- Helpers --- @@ -182,6 +189,42 @@ check_pr_state() { fi } +# Count required checks in a gh bucket (pending|fail|cancel). Prints 0 when the +# PR has no required checks. A failed query also prints 0 (the gate fails open +# rather than blocking merges on gh outages) but warns so the skip is visible. +count_required_checks() { + local bucket="$1" count + if ! count=$(gh pr checks "$PR_NUMBER" --repo "$REPO_SLUG" --required --json bucket \ + --jq "[.[] | select(.bucket == \"${bucket}\")] | length" 2>/dev/null); then + # gh exits non-zero with no output when no required checks are configured + count=0 + info "Could not read required checks for PR #${PR_NUMBER} (none configured, or gh error); treating as 0 ${bucket}" >&2 + fi + [[ "$count" =~ ^[0-9]+$ ]] || count=0 + echo "$count" +} + +# Refuse on failing required checks; wait (up to CHECK_TIMEOUT) on pending ones. +wait_for_checks() { + [[ "$WAIT_CHECKS" == true ]] || return 0 + + local waited=0 failing pending + while true; do + failing=$(( $(count_required_checks fail) + $(count_required_checks cancel) )) + [[ "$failing" -gt 0 ]] && die 5 "PR #${PR_NUMBER} has ${failing} failing required check(s) — fix them before merging (or pass --no-wait-checks to skip this gate)" + + pending=$(count_required_checks pending) + [[ "$pending" -eq 0 ]] && return 0 + + if [[ "$waited" -ge "$CHECK_TIMEOUT" ]]; then + die 5 "PR #${PR_NUMBER} still has ${pending} pending required check(s) after ${CHECK_TIMEOUT}s — wait for them and re-run /merge" + fi + info "Waiting on ${pending} pending required check(s) (~${waited}s of ${CHECK_TIMEOUT}s; gh call time not counted)..." + sleep "$CHECK_POLL_INTERVAL" + waited=$((waited + CHECK_POLL_INTERVAL)) + done +} + detect_allowed_methods() { [[ -z "$REPO_SLUG" ]] && die 1 "REPO_SLUG not set — detect_default_branch must run first" @@ -334,12 +377,22 @@ fetch_merge_commit() { return 0 # no merge commit yet for auto-merge fi - local commit_line - commit_line=$(gh pr view "$PR_NUMBER" --repo "$REPO_SLUG" --json mergeCommit \ - --jq '.mergeCommit | "\(.oid[0:7]) \(.messageHeadline)"' 2>/dev/null || echo "") - if [[ -n "$commit_line" ]]; then - MERGE_COMMIT="$commit_line" - fi + # gh's mergeCommit JSON carries only the oid for PRs (messageHeadline comes + # back null), so fetch the headline from the commits API. + local oid headline + oid=$(gh pr view "$PR_NUMBER" --repo "$REPO_SLUG" --json mergeCommit \ + --jq '.mergeCommit.oid // empty' 2>/dev/null || echo "") + [[ -z "$oid" ]] && return 0 + headline=$(gh api "repos/${REPO_SLUG}/commits/${oid}" \ + --jq '.commit.message | split("\n")[0]' 2>/dev/null || echo "") + MERGE_COMMIT=$(format_merge_commit "$oid" "$headline") +} + +# Format "<short-oid> <headline>"; omit the headline when it is empty or "null". +format_merge_commit() { + local oid="$1" headline="${2:-}" + [[ "$headline" == "null" ]] && headline="" + echo "${oid:0:7}${headline:+ $headline}" } update_local_master() { @@ -383,13 +436,20 @@ update_local_master() { git -C "$worktree_path" pull "$TARGET" "$DEFAULT_BRANCH" 2>/dev/null || true fi if [[ "$auto_switch" == true ]]; then - info "NOTE: could not auto-switch this worktree to ${DEFAULT_BRANCH} — still on '${BRANCH}' (now divergent)." - if [[ -n "$worktree_path" ]]; then - info " ${DEFAULT_BRANCH} is checked out in worktree '${worktree_path}'." + # Fall back to a detached HEAD at the new remote tip: it leaves the + # divergent feature branch without needing the default branch's name. + if git fetch "$TARGET" "$DEFAULT_BRANCH" 2>/dev/null \ + && git checkout --detach "${TARGET}/${DEFAULT_BRANCH}" 2>/dev/null; then + info "Worktree now detached at ${TARGET}/${DEFAULT_BRANCH}${worktree_path:+ (${DEFAULT_BRANCH} is checked out in worktree '${worktree_path}')}; '${BRANCH}' was squashed and is now divergent." + info " Run 'git checkout -b <new-branch>' before committing new work." else - info " Run 'git fetch ${TARGET} ${DEFAULT_BRANCH}' to create a local ref, then" + info "NOTE: could not auto-switch this worktree to ${DEFAULT_BRANCH} — still on '${BRANCH}' (now divergent)." + if [[ -n "$worktree_path" ]]; then + info " ${DEFAULT_BRANCH} is checked out in worktree '${worktree_path}'." + fi + info " Options: 'git checkout --detach ${TARGET}/${DEFAULT_BRANCH}' (leave the branch, keep it as-is)," + info " or 'git reset --hard ${TARGET}/${DEFAULT_BRANCH}' (discards '${BRANCH}' commits, reuses the branch name; only safe after the merge landed)." fi - info " 'git checkout ${DEFAULT_BRANCH}' or 'git reset --hard ${TARGET}/${DEFAULT_BRANCH}' before continuing." fi fi } @@ -429,12 +489,13 @@ main() { while [[ "${1:-}" == --* ]]; do case "$1" in --skip-rebase) skip_rebase=true; shift ;; + --no-wait-checks) WAIT_CHECKS=false; shift ;; --squash) MERGE_METHOD_FLAG="squash"; shift ;; --merge) MERGE_METHOD_FLAG="merge"; shift ;; --rebase) MERGE_METHOD_FLAG="rebase"; shift ;; --method) MERGE_METHOD_FLAG="${2:?--method requires squash|merge|rebase}"; shift 2 ;; --keep-branch) KEEP_BRANCH=true; shift ;; - *) die 1 "Unknown flag: $1 — usage: merge.sh [--skip-rebase] [--method <squash|merge|rebase>] [--squash|--merge|--rebase] [--keep-branch] \"<title>\" \"<body>\"" ;; + *) die 1 "Unknown flag: $1 — usage: merge.sh [--skip-rebase] [--no-wait-checks] [--method <squash|merge|rebase>] [--squash|--merge|--rebase] [--keep-branch] \"<title>\" \"<body>\"" ;; esac done @@ -443,7 +504,7 @@ main() { *) die 1 "Invalid --method '${MERGE_METHOD_FLAG}' — must be squash, merge, or rebase" ;; esac - local title="${1:?Usage: merge.sh [--skip-rebase] [--method <squash|merge|rebase>] \"<title>\" \"<body>\"}" + local title="${1:?Usage: merge.sh [--skip-rebase] [--no-wait-checks] [--keep-branch] [--method <squash|merge|rebase>] \"<title>\" \"<body>\"}" local body="${2:-}" # Append co-author line @@ -478,6 +539,7 @@ ${COAUTHOR}" info "Method override: using only ${MERGE_METHOD_FLAG} (skipping probe-based fallback)" ALLOWED_METHODS=("$MERGE_METHOD_FLAG") fi + wait_for_checks do_merge "$title" "$body" fetch_merge_commit # must precede update_local_master (uses GitHub API, not local state) update_local_master diff --git a/agents/skills/plan/SKILL.md b/agents/skills/plan/SKILL.md index 822726c5..cec1f94a 100644 --- a/agents/skills/plan/SKILL.md +++ b/agents/skills/plan/SKILL.md @@ -18,7 +18,7 @@ Read a PRD, tech spike, or feature spec and produce a concrete implementation pl - Recent commits: !`git log --oneline -10 2>/dev/null | head -10` - Prior plans: !`find .context/plans -name "*.md" 2>/dev/null | head -10` - Prior spikes: !`find context/spikes -name "*.md" 2>/dev/null | head -10` -- Phase artifacts: !`ls -t .context/phases/*.md 2>/dev/null | head -10` +- Phase artifacts: !`command ls -t .context/phases/*.md 2>/dev/null | head -10` ## Phase Protocol diff --git a/agents/skills/pr/SKILL.md b/agents/skills/pr/SKILL.md index 8b3b49df..fe977102 100644 --- a/agents/skills/pr/SKILL.md +++ b/agents/skills/pr/SKILL.md @@ -77,7 +77,7 @@ The check is cheap (~2s for a typical PR) and catches the most common qlty fmt f #### 2b: Push -Push the current branch to origin. Use `git push -u origin HEAD` if no upstream is set. If there is nothing new to push, that is fine — continue to Step 3. +Push the current branch with `git push -u origin HEAD`. Always use this exact form, never a bare `git push`: with `push.default=matching` a bare push also pushes unrelated stale local branches (for example a behind local `master`), and the resulting non-fast-forward rejection looks like the PR push failed. If there is nothing new to push, that is fine — continue to Step 3. ### Step 3: Open a PR (or use existing one) @@ -172,7 +172,13 @@ gh pr view <number> --repo <owner/repo> --json mergeStateStatus,mergeable Then use `gh pr checks <number> --repo <owner/repo>` to see the current state of all checks. -- If checks are still running, poll `gh pr checks` with exponential backoff: wait 60s, then 120s, then 240s, capping at 300s between polls. Stop after 30 minutes total and report to the user. +- If checks are still running, wait with a background until-loop and continue when its completion notification arrives. Do NOT chain foreground `sleep N; cmd` polls — the harness refuses them. Run this with `run_in_background`: + + ``` + timeout 1800 bash -c 'until [ "$(gh pr checks <number> --repo <owner/repo> --json bucket --jq "[.[]|select(.bucket==\"pending\")]|length")" = "0" ]; do sleep 30; done' + ``` + + Then re-run `gh pr checks` once to read the final state. If the timeout (30 minutes) expires, report to the user. - **Distinguish a genuinely-running slow check from a stuck/queued one** before either continuing to wait or declaring it stuck. A required check on a slow repo (a 10-minute build-test, for example) is easy to mistake for a wedged workflow. Probe the underlying check-run status for the head commit: ``` @@ -198,7 +204,7 @@ Then use `gh pr checks <number> --repo <owner/repo>` to see the current state of - Analyze the failure. Read the relevant source files to understand the issue. - Fix the code. Make the minimal change needed to resolve the failure. - Stage and commit the fix with a clear message describing what was fixed and why. Stage specific files (`git add <path>`), not `git add -A`/`git add .`, so `.context/` scratch never gets swept in. -- Push all fixes with `git push`. +- Push all fixes with `git push -u origin HEAD`. #### 4c: Check for and resolve ALL review threads @@ -243,7 +249,7 @@ Filter to threads where `isResolved` is false. If there are zero unresolved thre For bot comments specifically: bot suggestions are frequently false positives or conflict with project conventions. Do not blindly apply every suggestion. Evaluate each on its merits. Complexity warnings from linter bots (qlty, CodeClimate) are almost always informational — the complexity is inherent to the function. Similar-code detection is usually informational when the code implements a shared interface pattern. -3. **Reply to the thread.** Always reply before resolving, so there is an audit trail: +3. **Reply to the thread — after confirmation.** Posting a reply sends a message on the user's behalf. Unless the user has already authorized replies for this PR, do not post any reply yet: first draft all replies (one per thread, keyed by thread and author), show them to the user in a single batch, and ask for one explicit yes. If the user declines or does not answer, resolve the threads without replying and tell the user which threads were resolved silently and why. Once confirmed, reply before resolving so there is an audit trail: - **(a)** Reply confirming what you changed (e.g., "Fixed — added nil check at line 42"). - **(b)** Reply explaining why no change was made (e.g., "This complexity is inherent to the dispatch logic — extracting sub-functions would hurt readability without reducing actual complexity"). - **(c)** Reply with a brief acknowledgment (e.g., "Acknowledged — these implement the same abstract interface, so the similarity is expected"). @@ -261,6 +267,8 @@ gh api graphql -f query=' }' ``` +**Rate-limited review bots.** A review check reading "Review rate limited" (CodeRabbit and similar) is non-blocking: do not wait on it or treat it as a failure. Still run the thread fetch above and address any threads the bot posted before it was limited — a valid finding can land before the limit hits. + **After addressing all threads:** if any code changes were made, stage, commit, and push them before looping back. **Expect AI re-reviewers to re-review on every push.** Tools like CodeRabbit and CI-based reviewers (a `claude-review`/`robothanx`-style job) re-run on each new commit, and each pushed fix frequently surfaces NEW incremental findings — often 2nd-order effects of the fix you just made (e.g., adding a call inside a signal handler prompts the next review to flag that call's missing timeout). This is normal: expect 2-4 cycles to converge, and do not treat a fresh finding after an otherwise-clean round as the loop being stuck. The "stuck (same failure 3+ times)" guard in Step 4d applies to the SAME finding recurring after you tried to fix it — genuinely-new findings each round are progress, not churn. diff --git a/agents/skills/rereview-loop/SKILL.md b/agents/skills/rereview-loop/SKILL.md index 7ec212b2..e993a0e2 100644 --- a/agents/skills/rereview-loop/SKILL.md +++ b/agents/skills/rereview-loop/SKILL.md @@ -14,6 +14,7 @@ Run `/rereview` repeatedly: review → fix the findings → re-review → repeat - `max=<N>` - cap the number of review→fix iterations (default 5) - `severity=blocking` - only auto-fix BLOCKING findings (default fixes BLOCKING + WARNING) - `severity=all` - auto-fix BLOCKING + WARNING + INFO + - `push` - after each iteration commit, and push the branch with `git push -u origin HEAD` (opt-in; the same applies if the user explicitly asks mid-run to update the PR between loops) - Any combination, space-separated. Example: `/rereview-loop max=3 severity=blocking` ## Context @@ -55,6 +56,8 @@ last_iteration_committed = false (did the previous iteration produce a commit? total_pushbacks = 0 (cumulative judgment-call pushbacks across all iterations) total_verified_false_positives = 0 (cumulative findings disproven by an actually-executed command, not judgment) total_findings_addressed = 0 (cumulative findings touched, for the pushback rate) +push_each_iteration = false (override with push) +accepted_limits = [] (running list of accepted limits and pushed-back findings, with file:line and one-line rationale) stashed = false (set true if Phase 0 stashes unrelated work) ``` @@ -80,7 +83,7 @@ Announce the run: ## Phase 1: Run /rereview -Invoke the `/rereview` skill via the Skill tool, passing `rereview_scope` as its argument (empty string if no scope was given). Wait for it to complete and capture the full final report. +Invoke the `/rereview` skill via the Skill tool, passing `rereview_scope` as its argument (empty string if no scope was given). **Pass the current `accepted_limits` list along** (state it in the invocation so `/rereview` includes it in the reviewer briefing under ACCEPTED LIMITS); without it reviewers re-flag settled items every round. Wait for it to complete and capture the full final report. `/rereview` will: 1. Gather all changed files and dependents @@ -118,6 +121,8 @@ OTHERWISE: → Continue to Phase 3 to address findings. ``` +**Cap semantics.** `iteration` counts completed fix rounds, and Phase 1 reviews before every check. With max=N the loop runs at most N fix rounds and therefore at most N+1 reviews: the cap check fires only after a review, so every fix round is always followed by a review. When the cap fires, that final review's findings are reported but not fixed. State this plainly in Phase 4 (for example: "cap hit after N fix rounds; the final review's M findings were not addressed"). + ### Stuck-state detection After Phase 3 commits the iteration's fixes, the next iteration's Phase 2 must check whether progress was actually made. Use a hard counter, not a paraphrased-text signature: @@ -134,6 +139,13 @@ IF iteration > 0 AND last_iteration_committed == true AND - If this is the second consecutive iteration with no progress, EXIT with status = STUCK after applying best-effort fixes once more. + **Second-order findings are progress.** New findings caused by the + previous fix (a fix surfaces a follow-on issue at a different file:line + or a different aspect of the same code) count as progress even when the + B/W/I counts do not fall. Only treat the loop as stuck when the SAME + findings recur at the same location after a fix attempt. Note + second-order findings in the heartbeat as "new, caused by iter {N} fix". + A finding resolved as a **verified false positive** (Phase 3(b)) counts as genuine progress even though no code changed. Note it explicitly as "resolved via verification" in the heartbeat so it does not read as an @@ -175,6 +187,7 @@ For each finding: - Why the existing code is correct - What evidence convinced you (link to existing test, prior decision, code that shows the invariant) - Note this in the iteration summary so the user can sanity-check + - Add the finding (file:line, one-line rationale) to `accepted_limits` so later reviewers are told it is settled **Distinguish two kinds of pushback:** - **Verified false positive.** The finding made a mechanically-checkable claim (compiles, a test passes or fails, a command produces some output) and you actually ran that command in this worktree and it contradicts the claim. Record the exact command and its real output in the iteration summary. Increment `total_verified_false_positives`, not `total_pushbacks` -- this is empirical, not a judgment call, and is exempt from the pushback ceiling below. @@ -188,7 +201,7 @@ For each finding: Pushing back on a quarter of an independent panel's findings -- in any single iteration or cumulatively -- usually means you're rationalizing rather than reviewing. Verified false positives do not count toward this ceiling because they are proven with a command's real output, not argued. - - **(c) Defer to user.** The finding requires a product decision, scope expansion, or knowledge you don't have (e.g., "the new SLA isn't documented"). Do NOT silently skip. Surface it in the iteration summary and continue. The loop will likely re-flag it next iteration; that is correct behavior. Record the deferral; a re-flag of an already-deferred item is not a new finding and does not add to `total_findings_addressed`. + - **(c) Defer to user.** The finding requires a product decision, scope expansion, or knowledge you don't have (e.g., "the new SLA isn't documented"). Do NOT silently skip. Surface it in the iteration summary and continue. Do not add it to `accepted_limits` (the user has not accepted it), so the loop may re-flag it next iteration; that is correct behavior. Record the deferral; a re-flag of an already-deferred item is not a new finding and does not add to `total_findings_addressed`. 3. **After all findings in this iteration are processed:** @@ -205,7 +218,7 @@ For each finding: {bulleted list of finding summaries with file:line, prefixed by [BLOCKING] / [WARNING] / [INFO]}" ``` - Set `last_iteration_committed = true` once the commit succeeds. If no findings led to code changes in this iteration (everything was pushed back or deferred), do NOT create an empty commit -- leave `last_iteration_committed = false`. + Set `last_iteration_committed = true` once the commit succeeds. If `push_each_iteration` is true, run `git push -u origin HEAD` now (never a bare `git push`). If no findings led to code changes in this iteration (everything was pushed back or deferred), do NOT create an empty commit -- leave `last_iteration_committed = false`. 5. **Heartbeat.** Output the per-iteration summary defined below before looping back. @@ -235,7 +248,7 @@ If there are pushback or deferred items, list them under the heartbeat with file - Do not amend prior commits -- each iteration gets its own commit so the audit trail is clear. - Do not delete or skip tests to make findings go away. If a test is genuinely wrong, replace it; explain in the commit message. - Do not introduce new abstractions, refactors, or "while I'm here" cleanups. The loop fixes findings, nothing else. -- Do not push to remote. Pushing is the user's call. +- Do not push to remote unless `push` was passed or the user explicitly asked for a push. Otherwise pushing is the user's call. --- @@ -265,15 +278,18 @@ When the loop exits, output: ## Outstanding items (deferred or pushed back) {numbered list, or "None."} +## Accepted limits fed to reviewers +{the accepted_limits list, or "None."} + ## Recommendation {One of: - - "Clean — safe to push and merge." + - "Clean — safe to push and merge." (if `push` was passed: "Clean — already pushed; safe to merge.") - "Warnings remain (ignored per `severity=blocking`). Review the warnings list before merging." - "Iteration cap hit with findings still open. Re-run /rereview-loop, raise max=, or address remaining items manually." - "Loop is stuck — reviewers re-flagged the same findings after a fix attempt. Manual investigation needed."} ``` -Do NOT push the branch. Do NOT open or update a PR. The loop's job is to drive findings to zero locally; merging is the user's decision. +Do NOT push the branch unless `push` was passed or the user asked for it (use `git push -u origin HEAD`). Do NOT open a PR. The loop's job is to drive findings to zero; merging is the user's decision. --- @@ -295,4 +311,5 @@ Do NOT push the branch. Do NOT open or update a PR. The loop's job is to drive f - Each iteration commits independently so a future `git log` shows exactly what the loop changed and why. - The B/W/I count comparison in Phase 2 exists to catch the bad-fix → re-flag → bad-fix cycle. A count-based check (instead of paraphrased text matching) is robust against reviewer agents wording the same finding differently across iterations. - The 25% pushback ceiling -- both per-iteration and cumulative -- exists because once you start rejecting a quarter of an independent panel's findings, the failure mode is almost always rationalization, not the reviewers being wrong. The cumulative bound prevents systematic 1-of-4 pushbacks from compounding. Each distinct finding counts once so that reviewers re-raising the same settled judgment call cannot trip the ceiling without a new disagreement. -- This skill does not push or open a PR -- that is the user's call. Pair with `/pr` after a clean exit. +- This skill does not push (unless `push` is passed or the user asks) or open a PR -- that is the user's call. Pair with `/pr` after a clean exit. +- The accepted_limits list exists because reviewers are independent and stateless: without it they re-flag settled items every round and the loop never converges. diff --git a/agents/skills/rereview/SKILL.md b/agents/skills/rereview/SKILL.md index 0587f85f..77a4a6ab 100644 --- a/agents/skills/rereview/SKILL.md +++ b/agents/skills/rereview/SKILL.md @@ -16,7 +16,7 @@ Re-review all branch changes from scratch with independent competing reviewers. - Current branch: !`git branch --show-current` - Base ref: !`git branch -r 2>/dev/null | grep -oE 'origin/(main|master)' | head -1` - Git status: !`git status --short` -- Project type: !`ls go.mod Gemfile package.json Cargo.toml pyproject.toml 2>/dev/null | head -5` +- Project type: !`command ls go.mod Gemfile package.json Cargo.toml pyproject.toml 2>/dev/null | head -5` - Test framework: !`git ls-files 2>/dev/null | grep -E '_test\.|\.test\.|_spec\.' | head -10` - Changes vs main: !`git diff --stat origin/main...HEAD 2>/dev/null | head -50` - Changes vs master: !`git diff --stat origin/master...HEAD 2>/dev/null | head -50` @@ -95,20 +95,30 @@ Record lint result: CLEAN, WARNINGS, or ERRORS. Use `subagent_type="general-purpose"` and `model: "sonnet"` for all reviewers. -Each Task call should use the reviewer briefing template below, with only the reviewer name changed (ALPHA, BRAVO, CHARLIE). +**Write the shared briefing to one file; do not inline it three times.** Fill in the reviewer briefing template below once (branch, commits, changed files list, test baseline, safe commands, skim paths, accepted limits) and save it to a scratch file outside the worktree, for example `$TMPDIR/rereview-brief-{safe-branch}.md` (where {safe-branch} is the branch name with every `/` replaced by `-`). Each Task prompt is then a short stub: "You are REVIEWER {ALPHA / BRAVO / CHARLIE}. Read your full briefing from {path} and follow it exactly." This cuts prompt size roughly 3x with no loss of quality, and guarantees all three reviewers see identical data. The reviewers read changed and dependent files themselves from the worktree, so the brief lists their paths instead of embedding full contents. + +If the caller (for example /rereview-loop) supplied a list of **accepted limits and pushed-back findings**, include it in the briefing under ACCEPTED LIMITS. + +### Safe Commands + +Detect which commands are safe to run in parallel from the repo's AGENTS.md or CLAUDE.md (look for notes on shared test databases, ports, lockfiles, or required test ordering). Put the result in the briefing's SAFE COMMANDS slot. Example: when several suites share one test database, parallel reviewers may run only package-scoped tests, never the full gate. If the docs say nothing, default to: package-scoped or single-file tests only; the lead runs the full suite in Phase 1. + +### Generated Files and Fixtures + +For large diffs, list in the briefing which changed paths are generated code, lockfiles, snapshots, or synthetic fixtures (for example, a directory of fixture JSON that makes up most of the diff lines). Reviewers skim those paths for structure and spot-check a sample rather than reading every line. ### Large Diff Handling If the diff exceeds 30KB, save it to a temp file instead of passing it inline: ```bash -git diff {base}...HEAD > /tmp/review-diff-{branch}.txt +git diff {base}...HEAD > "$TMPDIR/review-diff-{safe-branch}.txt" # safe-branch: replace "/" with "-" ``` -Then replace the `FULL DIFF` section in the briefing with: +Then replace the `FULL DIFF` section in the briefing with the following. Keep this file outside the worktree too: ``` -FULL DIFF: Read the diff from /tmp/review-diff-{branch}.txt +FULL DIFF: Read the diff from {scratch dir}/review-diff-{safe-branch}.txt ``` This prevents context window overflow in reviewer agents. @@ -124,14 +134,25 @@ MANDATE: This review exists to guarantee ZERO REGRESSIONS. Go slow. Analyze ever CALIBRATION -- READ CAREFULLY: - Your DEFAULT verdict is REQUEST CHANGES, not APPROVE. You need overwhelming evidence of correctness to approve. -- First implementations typically have 3-5 real issues. Finding zero issues is a red flag -- look harder. -- False positives are acceptable. False negatives are not. Flag anything uncertain. +- First implementations typically have 3-5 real issues, so look hard before concluding there are none. But if after a genuinely thorough review you find no real defect, APPROVE. Do NOT invent findings to avoid an empty report; manufactured warnings waste the fix loop. +- Prefer flagging genuine uncertainty over staying silent, and mark it as uncertain. Never pad the report with findings you do not believe. - Do NOT downplay risk. "MEDIUM" regression risk is normal and honest. Reserve "LOW" for trivially small changes. - Run verification commands before making claims. Check what actually exists, not what you assume. - Any claim that code does or does not compile, that a test does or does not pass, or any other mechanically-checkable claim MUST be backed by a command you actually executed in this worktree, with the real output shown in your report. Never infer or reason about this abstractly. Never trust an isolated or minimal repro as a substitute for running the real code -- generic-inference and other language subtleties can make a minimal repro fail where the actual codebase compiles cleanly, or the reverse. If you cannot run the command (no working environment, missing dependencies), say so explicitly and mark the claim UNVERIFIED -- do not state it as fact. - When verifying that a type-level change is safe (a prop made optional, a generic signature changed, and similar), also make one deliberately-wrong, unrelated break and confirm the type-checker still flags it in the same run. This rules out type inference silently collapsing to `any` as an explanation for a clean compile. Reviewers run in parallel on ONE shared worktree, so NEVER edit a tracked file for this check -- another reviewer would read or typecheck your break. Put the break in a scratch copy of the code, or in a new untracked file under a scratch directory that the type-checker includes, and delete it afterward. If neither is practical, skip the check and say so in your report; otherwise note that you did it. - Do NOT edit, commit, push, stash, or reset anything tracked in the worktree. You are read-only except for scratch files you create and delete. +WORKTREE HYGIENE: Other reviewers are reading this worktree at the same time. NEVER create probe, scratch, or repro files inside the worktree, and never edit tracked files (see the type-check rule above). Put all scratch files, including copies used for deliberate-break checks, in a temp directory outside the worktree (for example under $TMPDIR). Do not run formatters or auto-fixers that rewrite tracked files. + +SAFE COMMANDS (detected from the repo docs; run nothing broader in parallel): +{safe commands slot} + +SKIM PATHS (generated files, lockfiles, fixtures: spot-check only): +{paths or "None"} + +ACCEPTED LIMITS (settled in earlier rounds; do not re-flag unless you have new evidence): +{list or "None"} + BRANCH: {branch name} COMMITS: {git log output} @@ -139,11 +160,11 @@ COMMITS: FULL DIFF: {full git diff} -CHANGED FILES (full contents): -{for each changed file, include filename and full contents} +CHANGED FILES (read each one in full from the worktree): +{one path per line} -DEPENDENT FILES (files that reference changed code): -{for each dependent file, include filename and full contents} +DEPENDENT FILES (files that reference changed code; read the relevant ones): +{one path per line} TEST BASELINE: - Test suite result: {PASS / FAIL / N/A} diff --git a/agents/skills/ship/SKILL.md b/agents/skills/ship/SKILL.md index 52ede056..f6c9377f 100644 --- a/agents/skills/ship/SKILL.md +++ b/agents/skills/ship/SKILL.md @@ -89,6 +89,8 @@ And skip directly to Phase 3. Work through the review report systematically: +If a fix answers an existing bot or human review thread on the PR, apply the same rule as `/pr` Step 4c: do not post a reply on the user's behalf without confirmation. Draft the replies, show them in one batch, and ask for one explicit yes; if declined, resolve the threads silently and tell the user. + ### 2a: Fix BLOCKING issues For each blocking issue in the review report: @@ -161,7 +163,7 @@ Print: Invoke the `/merge` skill to land the branch into master. -Handle merge script exit codes as documented in the `/merge` skill (conflicts, nothing to merge, review blocked, errors). +Handle merge script exit codes as documented in the `/merge` skill (conflicts, nothing to merge, review blocked, required checks failing or pending, errors). **If CI fails on tests unrelated to your PR**, fix them inline as part of `/ship` rather than rebasing or treating the merge as blocked. Pre-existing flakes (timing-sensitive tests, data races on shared mutable state, environment-dependent assertions) are normal during a `/ship` cycle on a project with sparse CI history. Capture each fix as its own commit with a message that names the flake and the cause (e.g., "fix flaky-test sleep timing on slower CI" or "fix data race on shared buffer under -race") so the rationale stays clear in history. Then re-run `/merge`. @@ -177,7 +179,7 @@ DEFAULT_BRANCH=$(git branch -r 2>/dev/null | grep -oE 'origin/(main|master)' | h git checkout "$DEFAULT_BRANCH" 2>/dev/null && git pull --ff-only origin "$DEFAULT_BRANCH" ``` -If the checkout fails (uncommitted changes, no local default-branch ref, another worktree owns it, etc.), **warn but do not abort** — the pipeline is complete; this is hygiene. Print the current branch and a one-line note that follow-up work in this worktree should `git checkout "$DEFAULT_BRANCH"` (or `git reset --hard origin/"$DEFAULT_BRANCH"`) before committing. +If the checkout fails because another worktree owns the default branch, reuse `/merge`'s fallback: `git fetch origin "$DEFAULT_BRANCH"` then `git checkout --detach origin/"$DEFAULT_BRANCH"`. If it fails for another reason (uncommitted changes, no local default-branch ref), **warn but do not abort** — the pipeline is complete; this is hygiene. Print the current branch and a one-line note that follow-up work in this worktree should `git checkout "$DEFAULT_BRANCH"`, `git checkout --detach origin/"$DEFAULT_BRANCH"`, or (only after the merge landed) `git reset --hard origin/"$DEFAULT_BRANCH"` before committing. --- diff --git a/agents/skills/wrapup/SKILL.md b/agents/skills/wrapup/SKILL.md index 5742a2ed..0ba845fd 100644 --- a/agents/skills/wrapup/SKILL.md +++ b/agents/skills/wrapup/SKILL.md @@ -16,8 +16,8 @@ End-of-session sweep that detects loose ends and routes to the right follow-up a - Unpushed commits vs master: !`git log origin/master..HEAD --oneline 2>/dev/null | head -20` - Open PRs (mine): !`gh pr list --author @me --state open 2>/dev/null | head -10` - Stash list: !`git stash list 2>/dev/null | head -5` -- Knowledge base exists: !`ls context/knowledge/index.md 2>/dev/null | head -1` -- Context directory exists: !`ls context/ 2>/dev/null | head -5` +- Knowledge base exists: !`command ls context/knowledge/index.md 2>/dev/null | head -1` +- Context directory exists: !`command ls context/ 2>/dev/null | head -5` ## Instructions diff --git a/agents/skills/write-skill/SKILL.md b/agents/skills/write-skill/SKILL.md index c93e4aac..d31522c1 100644 --- a/agents/skills/write-skill/SKILL.md +++ b/agents/skills/write-skill/SKILL.md @@ -80,7 +80,7 @@ In the examples below, !{command} represents the actual syntax (exclamation mark - Current branch: !{git branch --show-current} - Git status: !{git status --short} -- Project type: !{ls -1 go.mod package.json Cargo.toml 2>/dev/null | head -3} +- Project type: !{command ls -1 go.mod package.json Cargo.toml 2>/dev/null | head -3} ``` #### CRITICAL: No `$()` Command Substitution