From 6df2ccfda6df5a8c9f1f54aa0492374e31fa8333 Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Tue, 15 Sep 2026 18:02:41 -0700 Subject: [PATCH 1/3] Run address-pr-reviews from a detached HEAD An agent harness (PostHog Desktop, cloud runners) checks the PR head out detached, where `git branch --show-current` is empty. git-pr resolves the PR from HEAD's commit there, and repo-context.sh's new resolve_branch_name names the branch for log-step-done.sh and ran-report.sh. log-command.sh keeps its network-free contract and skips the new tier. address-pr-reviews asks its three approval gates through the harness's structured question tool, because a task runner reads a prose question as the end of the turn. Its new --unattended flag takes each gate's default: apply every fix, reply to and resolve bot threads, commit, and push the PR head ref. babysit-prs passes it on dispatch. Generated-By: PostHog Desktop Task-Id: 4e81ba70-85c0-47fa-9cf7-26fce96f35a1 --- .github/workflows/test.yml | 1 + ai/AGENTS.md | 2 +- ai/bin/log-command.sh | 3 + ai/bin/log-step-done.sh | 4 +- ai/helpers/repo-context.sh | 35 +++++- ai/helpers/tests/test-repo-context.sh | 47 +++++++- ai/skills/address-pr-reviews/SKILL.md | 33 ++++-- ai/skills/babysit-prs/SKILL.md | 2 +- ai/skills/ran/scripts/ran-report.sh | 4 +- .../ran/scripts/tests/test-ran-report.sh | 34 +++++- ai/skills/wait-for-pr-reviews/SKILL.md | 2 + ai/skills/wait-for-pr-reviews/scripts/git-pr | 21 ++++ ai/tests/test-log-step-done.sh | 54 ++++++++- bin/git-pr | 21 ++++ bin/lib/test-git-pr.sh | 109 ++++++++++++++++++ 15 files changed, 348 insertions(+), 24 deletions(-) create mode 100755 bin/lib/test-git-pr.sh diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index aa8d2cd..935bf5c 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -23,3 +23,4 @@ jobs: ai/tests/test-log-step-done.sh ai/skills/ran/scripts/tests/test-ran-report.sh ai/helpers/tests/test-repo-context.sh + bin/lib/test-git-pr.sh diff --git a/ai/AGENTS.md b/ai/AGENTS.md index 64d51d0..632c6c2 100644 --- a/ai/AGENTS.md +++ b/ai/AGENTS.md @@ -59,7 +59,7 @@ Write as the user in all public-facing content. Don't refer to yourself as an AI **Always use `gh` CLI** for GitHub operations. Never use GitHub MCP server tools. -**Never post PR review comments without explicit user approval.** See the `github-pr-operations` skill for endpoint reference and thread-resolution commands. +**Never post a PR review comment or a reply to a human reviewer without explicit user approval.** A reply to a bot reviewer's comment (Copilot, ReviewHog, Greptile, Graphite) may also go out unasked under a skill's explicit `--unattended` mode, which is what lets an unattended sweep leave bot threads resolved. See the `github-pr-operations` skill for endpoint reference and thread-resolution commands. ## Project-Specific Workflow diff --git a/ai/bin/log-command.sh b/ai/bin/log-command.sh index 15ca968..892e9be 100755 --- a/ai/bin/log-command.sh +++ b/ai/bin/log-command.sh @@ -63,6 +63,9 @@ step=$(canonical_step "$raw") || exit 0 derive_org_repo || exit 0 repo_context_is_path_safe || exit 0 +# Not resolve_branch_name: this hook fires on every prompt in every repo and +# must never make a network call. A detached checkout loses the started record +# and keeps the completion one log-step-done.sh writes. branch=$(git branch --show-current 2> /dev/null) [ -n "$branch" ] || exit 0 log_file=$(command_log_path "$REPO_ORG" "$REPO_REPO" "$branch") || exit 0 diff --git a/ai/bin/log-step-done.sh b/ai/bin/log-step-done.sh index 1e7c852..d35ca58 100755 --- a/ai/bin/log-step-done.sh +++ b/ai/bin/log-step-done.sh @@ -36,8 +36,8 @@ command_step_declared "$step" || die "'$step' is not a step in COMMAND_STEP_TABL derive_org_repo || die "not a GitHub repository" repo_context_is_path_safe || die "org or repo is not safe as a path component" -branch=$(git branch --show-current 2> /dev/null) -[ -n "$branch" ] || die "detached HEAD: no branch to record against" +branch=$(resolve_branch_name network) || + die "no branch to record against: HEAD is detached, RAN_BRANCH is unset, and no PR has this commit as its head" log_file=$(command_log_path "$REPO_ORG" "$REPO_REPO" "$branch") || die "branch name has no safe log filename" sha=$(git rev-parse --short HEAD 2> /dev/null) || die "no commits on this branch" diff --git a/ai/helpers/repo-context.sh b/ai/helpers/repo-context.sh index a65cd39..6a8010f 100644 --- a/ai/helpers/repo-context.sh +++ b/ai/helpers/repo-context.sh @@ -1,5 +1,5 @@ #!/usr/bin/env bash -# Derive the GitHub org/repo of the current directory's origin remote. +# Name the GitHub org, repo, and branch of the current directory's checkout. # # Usage: source repo-context.sh, then: derive_org_repo || # @@ -25,6 +25,39 @@ derive_org_repo() { return 1 } +# Name the branch this checkout's work belongs to, and print it. +# +# An agent harness (PostHog Desktop, cloud runners) checks the PR head out +# detached, where `git branch --show-current` is empty. RAN_BRANCH lets a caller +# that already resolved the PR skip the last tier. Pass "network" to allow that +# tier, which asks GitHub which PR has HEAD as its head commit; a caller that +# must never touch the network omits the argument and gets the first two tiers. +# +# Usage: resolve_branch_name [network] +# Returns 1 when no tier answers. +resolve_branch_name() { + local branch head_sha + branch=$(git branch --show-current 2> /dev/null) || branch="" + [ -n "$branch" ] || branch="${RAN_BRANCH:-}" + if [ -z "$branch" ] && [ "${1:-}" = "network" ]; then + head_sha=$(git rev-parse HEAD 2> /dev/null) || head_sha="" + if [ -n "$head_sha" ]; then + # The endpoint also lists PRs this commit merged into, so a detached + # main would otherwise resolve to whatever landed last. Only an exact + # head.sha match is this commit's own PR. + branch=$(env GIT_PR_HEAD_SHA="$head_sha" GH_PAGER= \ + gh api "repos/{owner}/{repo}/commits/${head_sha}/pulls" \ + --jq '[.[] | select(.head.sha == $ENV.GIT_PR_HEAD_SHA)] | sort_by(.state != "open") | .[0].head.ref' \ + 2> /dev/null) || branch="" + if [ "$branch" = "null" ]; then + branch="" + fi + fi + fi + [ -n "$branch" ] || return 1 + printf '%s\n' "$branch" +} + # The captures come from a remote URL, which whoever set the remote controls: # git@github.com:../evil.git parses as an org of "..". Any caller that builds a # filesystem path out of REPO_ORG/REPO_REPO must pass this first, or the path diff --git a/ai/helpers/tests/test-repo-context.sh b/ai/helpers/tests/test-repo-context.sh index 6652a31..d849d5a 100755 --- a/ai/helpers/tests/test-repo-context.sh +++ b/ai/helpers/tests/test-repo-context.sh @@ -1,5 +1,6 @@ #!/usr/bin/env bash -# Tests for derive_org_repo across origin URL shapes. +# Tests for derive_org_repo across origin URL shapes, and for resolve_branch_name +# across its three tiers. set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" @@ -40,6 +41,50 @@ check "github subdomain rejected" "$(try_url 'https://foo.github.com/org/repo')" check "ssh:// url accepted" "$(try_url 'ssh://git@github.com/PostHog/posthog.git')" "PostHog/posthog" check "outside a repo" "$( (cd "$(mktemp -d)" && if derive_org_repo; then echo yes; else echo none; fi) )" "none" +# ── resolve_branch_name ───────────────────────────────────────────────────── + +SHIM_DIR=$(mktemp -d) +trap 'rm -rf "$SHIM_DIR"' EXIT +cat > "${SHIM_DIR}/gh" <<'SHIM' +#!/usr/bin/env bash +# SELF stands in for the sha the caller asked about, so a fixture can say "this +# PR's head is the commit under test" without the test knowing the sha. +[ "$1" = api ] || exit 1 +printf '%s' "${GH_API_JSON-[]}" | sed "s/SELF/${GIT_PR_HEAD_SHA}/g" | jq -r "${4-.}" +SHIM +chmod +x "${SHIM_DIR}/gh" + +# Prints resolve_branch_name's answer, or "none", from a throwaway repo. Pass +# "detach" as the first argument to run it with no branch checked out; the rest +# become resolve_branch_name's own arguments. +try_branch() { # [detach] [network] + local d detach="" + [[ "${1-}" == detach ]] && { detach=yes; shift; } + d=$(mktemp -d) + git -C "$d" init -q -b haacked/work + git -C "$d" commit -q --allow-empty -m first + [[ -n "$detach" ]] && git -C "$d" checkout -q --detach HEAD + (cd "$d" && PATH="${SHIM_DIR}:${PATH}" \ + bash -c 'source "$1"; shift; resolve_branch_name "$@" || echo none' _ "${SCRIPT_DIR}/../repo-context.sh" "$@") + rm -rf "$d" +} + +check "attached checkout" "$(try_branch)" "haacked/work" +check "RAN_BRANCH does not override a real branch" "$(RAN_BRANCH=other try_branch)" "haacked/work" +check "detached with no tier left" "$(try_branch detach)" "none" +check "detached falls back to RAN_BRANCH" "$(RAN_BRANCH=haacked/env try_branch detach)" "haacked/env" +check "detached without the network argument never asks GitHub" \ + "$(GH_API_JSON='[{"head":{"sha":"x","ref":"haacked/pr"},"state":"open"}]' try_branch detach)" "none" + +MATCHING='[{"head":{"sha":"SELF","ref":"haacked/pr"},"state":"open"}]' +check "detached resolves the head ref of the PR whose head is this commit" \ + "$(GH_API_JSON="$MATCHING" try_branch detach network)" "haacked/pr" +check "detached rejects a PR this commit only merged into" \ + "$(GH_API_JSON='[{"head":{"sha":"deadbeef","ref":"haacked/merged-into"},"state":"open"}]' try_branch detach network)" "none" +check "detached prefers the open PR" \ + "$(GH_API_JSON='[{"head":{"sha":"SELF","ref":"a"},"state":"closed"},{"head":{"sha":"SELF","ref":"b"},"state":"open"}]' try_branch detach network)" "b" +check "detached with no associated PR" "$(GH_API_JSON='[]' try_branch detach network)" "none" + echo "" echo "Passed: ${passes}, Failed: ${failures}" [[ "${failures}" -eq 0 ]] diff --git a/ai/skills/address-pr-reviews/SKILL.md b/ai/skills/address-pr-reviews/SKILL.md index 7a87376..9367130 100644 --- a/ai/skills/address-pr-reviews/SKILL.md +++ b/ai/skills/address-pr-reviews/SKILL.md @@ -1,7 +1,7 @@ --- name: address-pr-reviews description: Evaluate unresolved PR review comments from any reviewer — bots and humans — fix legitimate issues, and reply to dismissed ones. -argument-hint: "[|] [--no-push]" +argument-hint: "[|] [--no-push] [--unattended]" model: sonnet metadata: execution-tier: balanced @@ -19,26 +19,34 @@ This skill never requests a review from anyone, and never waits for one. It only - PR URL: `https://github.com/owner/repo/pull/123` - PR number: `123` (infers repo from current directory) - `--no-push`: commit fixes as usual but never push — the invoker owns the push (`wait-for-pr-reviews` passes this while reviews are in flight) +- `--unattended`: nobody is watching this run, so treat this skill's approval gates as approved and never ask. `babysit-prs` passes it on every dispatch. Each gate below states the default it takes. Example invocations: - `/address-pr-reviews` -- process review comments for the current branch's PR - `/address-pr-reviews https://github.com/owner/repo/pull/123` -- process a specific PR - `/address-pr-reviews 123` -- process PR #123 in the current repo +- `/address-pr-reviews 123 --unattended` -- process PR #123 with no one watching + +## Asking the user + +Three steps below stop for approval. Ask through the harness's structured question tool whenever the harness has one — `AskUserQuestion` in Claude Code and PostHog Desktop. A task runner reads a question asked in ordinary prose as the end of the turn, so the run finishes at the gate with the work half done, and nothing afterwards tells that apart from a completed pass. A harness without such a tool asks in prose as usual. + +Under `--unattended`, do not ask at all. Take the default each gate names. ## Your Task ### Step 1: Detect PR -If `--no-push` is present, remember it and strip it — the detection script treats any non-flag token as the PR argument. Then run it with what remains, or with no argument at all when nothing remains: +Remember and strip `--no-push` and `--unattended` — the detection script treats any non-flag token as the PR argument. Then run it with what remains, or with no argument at all when nothing remains: ```bash -~/.dotfiles/bin/detect-pr.sh "" +~/.dotfiles/bin/detect-pr.sh --json "" ``` -This outputs tab-separated: `owner\trepo_name\trepo\tpr_number` +This outputs `{"pr_number", "org", "repo", "head_branch", "head_sha", "error"}`. JSON mode always exits 0, so read `.error`: if it is non-null, report it and stop. `org` and `repo` come back lowercased, which every `gh` call below accepts. -Parse these into variables for use in subsequent steps. If the script fails, report the error and stop. +Save `PR_NUMBER`, `ORG`, `REPO` (as `/`), and `HEAD_BRANCH`. Resolve the PR once, here. An agent harness checks the PR head out detached, where the detection script resolves the PR from HEAD's commit; once this skill commits a fix, HEAD moves off the PR head and a second lookup finds nothing. Step 5 needs `HEAD_BRANCH` to push. ### Step 2: Fetch and Filter Unaddressed Comments @@ -64,9 +72,11 @@ If the script fails or exits non-zero, report the error and stop — do not trea If the array is empty, record the finished pass and stop, reporting "No unaddressed review comments to process" plus who is still mid-review if the pre-check found anyone. A PR with nothing to address is a completed run, not an abandoned one, and leaving it unrecorded makes every later `/go` invoke this skill again: ```bash -~/.dotfiles/ai/bin/log-step-done.sh address-pr-reviews +RAN_BRANCH="$HEAD_BRANCH" ~/.dotfiles/ai/bin/log-step-done.sh address-pr-reviews ``` +`RAN_BRANCH` names the branch to record against. A detached checkout has none, and passing it here reuses the answer Step 1 already has instead of paying for the lookup again. + Do not record it when the fetch itself failed above; an error is not an empty comment set. Otherwise, report how many comments were found and proceed. @@ -103,7 +113,7 @@ Present your assessment for each comment with: - Your reasoning (1-2 sentences) - Your proposed action (what you'd fix, or what you'd reply) -After evaluating all comments, present a summary table and ask the user for confirmation before proceeding. +After evaluating all comments, present a summary table, then ask for confirmation as **Asking the user** describes: apply every fix, pick a subset, or stop. Unattended default: apply every fix. Before presenting the assessments and summary table, apply the `plain-writing` skill in technical mode to them. Keep every quoted comment and every verdict exactly as written. Apply the same rules to each reply you draft in Steps 4 and 5, before you show it. @@ -118,16 +128,17 @@ With user confirmation: **For not-legit comments, branch on who authored the comment:** -- **Bots (`is_bot` true — Copilot, ReviewHog, Greptile, Graphite, or any other GitHub App):** Draft a concise, professional reply explaining why the code is correct, show the draft to the user and wait for explicit approval, then post it: `gh api "repos//pulls//comments//replies" --method POST -f body=''`. Resolve the thread: `~/.dotfiles/bin/gh-resolve-threads "https://github.com//pull/" --comment-id `. +- **Bots (`is_bot` true — Copilot, ReviewHog, Greptile, Graphite, or any other GitHub App):** Draft a concise, professional reply explaining why the code is correct, show the draft, and ask for approval as **Asking the user** describes. Unattended default: post it and resolve the thread. Post it with: `gh api "repos//pulls//comments//replies" --method POST -f body=''`. Resolve the thread: `~/.dotfiles/bin/gh-resolve-threads "https://github.com//pull/" --comment-id `. - **Human reviewers (`is_bot` false):** Never post anything. Draft the reply and hold it for the user to review and post themselves (see Step 5). Leave the thread unresolved so the reviewer gets the last word. ### Step 5: Finalize 1. Show a summary: N comments fixed, M comments dismissed 2. **Present drafted replies to human reviewers for the user to post.** For each not-legit comment from a human reviewer, show the file:line, the comment quote, and your drafted reply. Write each reply to a file so it survives quotes and newlines, then give the user the exact command to post it — the same replies endpoint as Step 4, with `-F body=@` in place of `-f body=`. The user reviews each reply and posts the ones they approve. -3. If any files were changed, invoke the `comment-cleanup` skill over those files, so the fixes don't ship the over-commenting they were written with, then `git add` each one again so its edits reach the commit. Naming the files keeps the pass off unrelated work the checkout was already carrying. Report anything it hands back for the user's call with the summary. Then ask the user if they want to commit and push: +3. If any files were changed, invoke the `comment-cleanup` skill over those files, so the fixes don't ship the over-commenting they were written with, then `git add` each one again so its edits reach the commit. Naming the files keeps the pass off unrelated work the checkout was already carrying. Report anything it hands back for the user's call with the summary. Then ask whether to commit and push, as **Asking the user** describes. Unattended default: commit, then push. - Commit message: "Address PR review feedback" - - Push to the current branch + - Push with `git push HEAD:refs/heads/$HEAD_BRANCH`, naming the ref. A worktree checked out at the PR head has no current branch, and a bare `git push` there fails with no upstream. + - Push only when the PR's head repo is `$REPO`. A fork PR's head repo has no local remote, so report that and leave the commit unpushed. - Under `--no-push`, commit but don't push — tell the user the invoker owns the push - After the commit lands, append one `### Held comment:` block per held item to `.notes/review-skipped.md`, in the format `review-fix-cycle` Step 7a uses. An unattended `babysit-prs` sweep has no one watching the summary, and `explain-open` reads that file. 4. Record each dismissed comment in the shared state file so future runs filter it out. Extract the body from the Step 2 file with jq — never retype or paste it yourself; a single altered byte changes the hash and breaks the dedup — and pipe it into the record script: @@ -143,7 +154,7 @@ The script hashes the body, appends it to the state file (creating the file if n 6. Last action of the run, once the steps above are done: record that this step finished, so `/ran` and `/go` can tell a completed pass from one that was interrupted at the prompt. This is the same call Step 2 makes when there is nothing to address, and only one of the two runs in any given pass. ```bash -~/.dotfiles/ai/bin/log-step-done.sh address-pr-reviews +RAN_BRANCH="$HEAD_BRANCH" ~/.dotfiles/ai/bin/log-step-done.sh address-pr-reviews ``` Skip it only if you stopped early without working the comments, which is exactly the case the record is there to exclude. A non-zero exit is worth one line in the summary and nothing more. diff --git a/ai/skills/babysit-prs/SKILL.md b/ai/skills/babysit-prs/SKILL.md index d951d55..4e4cb8b 100644 --- a/ai/skills/babysit-prs/SKILL.md +++ b/ai/skills/babysit-prs/SKILL.md @@ -152,7 +152,7 @@ Handle each active PR, working from its checkout: This allowlist is hand-synced with `ci-monitor`'s in its Step 5, and is deliberately one state shorter: a `landed` PR is reported in Step 2 and never gets here, so admitting it would only ever mean pushing to a closed PR's branch. Widen this list only for a state Step 2 lets through. - **CI failing** → invoke the `ci-monitor` skill with the PR URL and `--unattended`. It classifies flaky vs legit failures, fixes legit ones, and reports flaky ones to @PostHog in #flakey-tests via the `report-flake` agent. This sweep runs unattended (typically under `/loop`), so there is no one to answer `ci-monitor`'s "re-run and report?" prompt: proceed as if approved — re-run the flaky failures and let `report-flake` post in `post` mode. The same applies to its fix handler's approval prompt: proceed as if "Fix all" was chosen. The agent dedups against flakes already reported there, so known flakes produce no duplicate posts even across repeated sweeps. -- **New review comments** → invoke the `address-pr-reviews` skill with the PR URL. It evaluates each comment, fixes legitimate findings, and handles replies per its own rules. +- **New review comments** → invoke the `address-pr-reviews` skill with the PR URL and `--unattended`. It evaluates each comment, fixes legitimate findings, and handles replies per its own rules. This sweep has no one to answer its approval gates, so `--unattended` makes it take their defaults: apply every fix, reply to and resolve bot threads, commit, and push. It still never replies to a human reviewer. - Push resulting commits to the PR branch. Never force-push from this sweep — the one force-push in scope lives inside `ci-monitor`'s conflict-fix path, behind its own gates. Never merge, close, or mark ready-for-review. If a dispatch fails twice for the same PR, record the failure in the summary and move on; don't retry within the sweep. diff --git a/ai/skills/ran/scripts/ran-report.sh b/ai/skills/ran/scripts/ran-report.sh index cee0f5d..aef6c10 100755 --- a/ai/skills/ran/scripts/ran-report.sh +++ b/ai/skills/ran/scripts/ran-report.sh @@ -55,8 +55,8 @@ command -v jq > /dev/null 2>&1 || fail "Required command not found: jq" derive_org_repo || fail "No GitHub origin remote" repo_context_is_path_safe || fail "Unsafe org or repo name in the origin URL" -branch=$(git branch --show-current 2> /dev/null) -[ -n "$branch" ] || fail "Detached HEAD: no branch to report on" +branch=$(resolve_branch_name network) || + fail "No branch to report on: HEAD is detached, RAN_BRANCH is unset, and no PR has this commit as its head" head_sha=$(git rev-parse --short HEAD 2> /dev/null) || fail "No commits on this branch" log_file=$(command_log_path "$REPO_ORG" "$REPO_REPO" "$branch") || fail "Branch name has no safe log filename" diff --git a/ai/skills/ran/scripts/tests/test-ran-report.sh b/ai/skills/ran/scripts/tests/test-ran-report.sh index 7d82b3f..ac1c967 100755 --- a/ai/skills/ran/scripts/tests/test-ran-report.sh +++ b/ai/skills/ran/scripts/tests/test-ran-report.sh @@ -27,7 +27,18 @@ OUT_FILE="${TEST_ROOT}/stdout" ERR_FILE="${TEST_ROOT}/stderr" READER_STATUS=0 -mkdir -p "$FAKE_HOME" "$LOG_DIR" +SHIM_BIN="${TEST_ROOT}/bin" +mkdir -p "$FAKE_HOME" "$LOG_DIR" "$SHIM_BIN" + +# A detached checkout resolves its branch by asking GitHub which PR has HEAD as +# its head commit. This shim answers from GH_API_JSON, where SELF stands in for +# the sha under test, so the suite stays offline. +cat > "${SHIM_BIN}/gh" <<'SHIM' +#!/usr/bin/env bash +[ "$1" = api ] || exit 1 +printf '%s' "${GH_API_JSON-[]}" | sed "s/SELF/${GIT_PR_HEAD_SHA}/g" | jq -r "${4-.}" +SHIM +chmod +x "${SHIM_BIN}/gh" # An inherited state-directory override would point the reader at the real log. unset RAN_STATE_DIR @@ -131,7 +142,8 @@ run_reader() { # repo [args...] : > "$ERR_FILE" ( cd "$repo" || exit 1 - HOME="$FAKE_HOME" TZ=UTC GH_TOKEN="" GITHUB_TOKEN="" "$READER" "$@" + PATH="${SHIM_BIN}:${PATH}" HOME="$FAKE_HOME" TZ=UTC GH_TOKEN="" GITHUB_TOKEN="" \ + "$READER" "$@" ) > "$OUT_FILE" 2> "$ERR_FILE" READER_STATUS=$? } @@ -411,5 +423,23 @@ run_reader "$NOT_A_REPO" --json check_eq "--json outside a repo exits 0" "$READER_STATUS" "0" check "--json outside a repo reports an error field" json_has_error +# ── Detached HEAD ──────────────────────────────────────────────────────────── +# An agent harness checks the PR head out detached, and log-step-done.sh records +# against the PR's head ref from there. The report has to read that same log +# back, or the branch's finished steps all render as never run. + +DETACHED=$(new_repo detached) +git -C "$DETACHED" checkout -q --detach HEAD +happy_path_log +GH_API_JSON='[{"head":{"sha":"SELF","ref":"haacked/breadcrumbs"},"state":"open"}]' \ + run_reader "$DETACHED" + +check_eq "a detached HEAD exits 0" "$READER_STATUS" "0" +check "it reports the branch its PR heads" out_has "Branch haacked/breadcrumbs" +check_eq "it reads the log that branch's records land in" "$(marker simplify)" "✓" + +GH_API_JSON='[]' run_reader "$DETACHED" +check "a detached HEAD with no PR fails" test "$READER_STATUS" -ne 0 + summary [[ "${failures}" -eq 0 ]] diff --git a/ai/skills/wait-for-pr-reviews/SKILL.md b/ai/skills/wait-for-pr-reviews/SKILL.md index a20b0d7..125e670 100644 --- a/ai/skills/wait-for-pr-reviews/SKILL.md +++ b/ai/skills/wait-for-pr-reviews/SKILL.md @@ -19,6 +19,8 @@ Requires Bash 4+, Git, jq, and authenticated `gh` 2.53+. Resolve `scripts/` path Comment processing also requires the installed `address-pr-reviews` skill and its runtime dependencies. Those are separate from the helpers bundled here. +This skill never passes `address-pr-reviews` its `--unattended` flag. `/go` chains this one in front of a person, so its approval gates have someone to answer them. `babysit-prs` and `/loop` are the unattended entry points. + ## Arguments (parsed from user input) - No arguments: detect PR from the current branch diff --git a/ai/skills/wait-for-pr-reviews/scripts/git-pr b/ai/skills/wait-for-pr-reviews/scripts/git-pr index 44fc494..5b23fb3 100755 --- a/ai/skills/wait-for-pr-reviews/scripts/git-pr +++ b/ai/skills/wait-for-pr-reviews/scripts/git-pr @@ -11,6 +11,11 @@ # same-named branch in an unrelated fork (e.g. "main", "patch-1") can # outrank the right one. # The push remote's owner narrows that match back down to the right fork. +# +# An agent harness (PostHog Desktop, cloud runners) checks the PR head out +# detached, so there is no branch name to resolve. That case asks the +# commit-to-PR endpoint for HEAD's SHA instead. Code search is index-lagged and +# would miss a PR pushed moments ago. set -e @@ -22,6 +27,22 @@ if [ -n "$1" ]; then fi current_branch=$(git branch --show-current) + +if [ -z "$current_branch" ]; then + head_sha=$(git rev-parse HEAD 2>/dev/null || true) + if [ -n "$head_sha" ]; then + # The endpoint also lists PRs this commit merged *into*, so a detached + # main would resolve to whatever landed last. Only an exact head.sha + # match is this commit's own PR. The REST state is lowercase; the + # `gh pr list` path below reads it uppercase. + url=$(env GIT_PR_HEAD_SHA="$head_sha" GH_PAGER= gh api "repos/{owner}/{repo}/commits/${head_sha}/pulls" \ + --jq '[.[] | select(.head.sha == $ENV.GIT_PR_HEAD_SHA)] | sort_by(.state != "open") | .[0].html_url' 2>/dev/null || true) + if [ -n "$url" ] && [ "$url" != "null" ]; then + echo "$url" + exit + fi + fi +fi branch="" remote="" if [ -n "$current_branch" ]; then diff --git a/ai/tests/test-log-step-done.sh b/ai/tests/test-log-step-done.sh index b043c05..a7a205c 100755 --- a/ai/tests/test-log-step-done.sh +++ b/ai/tests/test-log-step-done.sh @@ -22,9 +22,23 @@ FAKE_HOME="${TEST_ROOT}/home" STATE_ROOT="${FAKE_HOME}/.local/state/ran" STDOUT_FILE="${TEST_ROOT}/stdout" STDERR_FILE="${TEST_ROOT}/stderr" +SHIM_BIN="${TEST_ROOT}/bin" +GH_CALLS="${TEST_ROOT}/gh-calls" WRITER_STATUS=0 +WRITER_ENV=() -mkdir -p "$FAKE_HOME" +mkdir -p "$FAKE_HOME" "$SHIM_BIN" + +# The detached-HEAD tier of resolve_branch_name asks GitHub which PR has HEAD as +# its head commit. This shim answers from GH_API_JSON and logs the call, so the +# suite stays offline and can assert the attached path never reaches here. +cat > "${SHIM_BIN}/gh" <<'SHIM' +#!/usr/bin/env bash +printf '%s\n' "$1" >> "$CALLS" +[ "$1" = api ] || exit 1 +printf '%s' "${GH_API_JSON-[]}" | jq -r "${4-.}" +SHIM +chmod +x "${SHIM_BIN}/gh" unset RAN_STATE_DIR @@ -81,11 +95,14 @@ run_writer() { # repo [args...] shift : > "$STDOUT_FILE" : > "$STDERR_FILE" + : > "$GH_CALLS" ( cd "$repo" || exit 1 - HOME="$FAKE_HOME" "$WRITER" "$@" + PATH="${SHIM_BIN}:${PATH}" CALLS="$GH_CALLS" HOME="$FAKE_HOME" \ + env "${WRITER_ENV[@]}" "$WRITER" "$@" ) > "$STDOUT_FILE" 2> "$STDERR_FILE" WRITER_STATUS=$? + WRITER_ENV=() } state_files() { @@ -127,6 +144,7 @@ check_eq "sha is HEAD" "$(field "$LOG_PATH" .sha)" "$REPO_SHA" check_eq "branch is the current branch" "$(field "$LOG_PATH" .branch)" "haacked/breadcrumbs" check_eq "there is no command, since no command was typed" \ "$(field "$LOG_PATH" .command)" "null" +check "an attached branch never asks GitHub for one" test ! -s "$GH_CALLS" # Appending matters as much here as in the hook: the completion record has to # join the invocation rather than replace the branch's history. @@ -170,10 +188,40 @@ NON_GITHUB=$(make_repo elsewhere "git@gitlab.com:haacked/thing.git" "haacked/x") run_writer "$NON_GITHUB" review-code check "a non-GitHub origin fails" test "$WRITER_STATUS" -ne 0 +# ── Detached HEAD ──────────────────────────────────────────────────────────── +# An agent harness checks the PR head out detached, so there is no branch name +# to record against. The record has to land on the branch an attended run on the +# same work would write, or /ran and /go never see the completed pass. + DETACHED=$(make_repo detached "git@github.com:haacked/dotfiles.git" "haacked/tmp") git -C "$DETACHED" checkout -q --detach HEAD +DETACHED_SHA=$(git -C "$DETACHED" rev-parse HEAD) +DETACHED_SHORT=$(git -C "$DETACHED" rev-parse --short HEAD) + +WRITER_ENV=(RAN_BRANCH=haacked/from-env) +run_writer "$DETACHED" review-code +ENV_LOG="${STATE_ROOT}/haacked/dotfiles/haacked-from-env.jsonl" +check_eq "a detached HEAD with RAN_BRANCH exits 0" "$WRITER_STATUS" "0" +check_eq "it records against the branch RAN_BRANCH names" \ + "$(field "$ENV_LOG" .branch)" "haacked/from-env" +check_eq "it still records HEAD's sha" "$(field "$ENV_LOG" .sha)" "$DETACHED_SHORT" +check "RAN_BRANCH skips the GitHub lookup" test ! -s "$GH_CALLS" + +PR_JSON=$(jq -n -c --arg sha "$DETACHED_SHA" \ + '[{head: {sha: $sha, ref: "haacked/from-pr"}, state: "open"}]') +WRITER_ENV=(GH_API_JSON="$PR_JSON") +run_writer "$DETACHED" review-code +PR_LOG="${STATE_ROOT}/haacked/dotfiles/haacked-from-pr.jsonl" +check_eq "a detached HEAD resolves the branch from its PR" "$WRITER_STATUS" "0" +check_eq "it records against the PR's head ref" \ + "$(field "$PR_LOG" .branch)" "haacked/from-pr" + +BEFORE_DETACHED=$(state_files | wc -l | tr -d ' ') +WRITER_ENV=(GH_API_JSON='[]') run_writer "$DETACHED" review-code -check "a detached HEAD fails" test "$WRITER_STATUS" -ne 0 +check "a detached HEAD with no PR fails" test "$WRITER_STATUS" -ne 0 +check "it explains itself on stderr" test "$(stderr_bytes)" -gt 0 +check_eq "it writes nothing" "$(state_files | wc -l | tr -d ' ')" "$BEFORE_DETACHED" # ── Containment ────────────────────────────────────────────────────────────── # The org, repo, and branch become path components here exactly as they do in diff --git a/bin/git-pr b/bin/git-pr index 44fc494..5b23fb3 100755 --- a/bin/git-pr +++ b/bin/git-pr @@ -11,6 +11,11 @@ # same-named branch in an unrelated fork (e.g. "main", "patch-1") can # outrank the right one. # The push remote's owner narrows that match back down to the right fork. +# +# An agent harness (PostHog Desktop, cloud runners) checks the PR head out +# detached, so there is no branch name to resolve. That case asks the +# commit-to-PR endpoint for HEAD's SHA instead. Code search is index-lagged and +# would miss a PR pushed moments ago. set -e @@ -22,6 +27,22 @@ if [ -n "$1" ]; then fi current_branch=$(git branch --show-current) + +if [ -z "$current_branch" ]; then + head_sha=$(git rev-parse HEAD 2>/dev/null || true) + if [ -n "$head_sha" ]; then + # The endpoint also lists PRs this commit merged *into*, so a detached + # main would resolve to whatever landed last. Only an exact head.sha + # match is this commit's own PR. The REST state is lowercase; the + # `gh pr list` path below reads it uppercase. + url=$(env GIT_PR_HEAD_SHA="$head_sha" GH_PAGER= gh api "repos/{owner}/{repo}/commits/${head_sha}/pulls" \ + --jq '[.[] | select(.head.sha == $ENV.GIT_PR_HEAD_SHA)] | sort_by(.state != "open") | .[0].html_url' 2>/dev/null || true) + if [ -n "$url" ] && [ "$url" != "null" ]; then + echo "$url" + exit + fi + fi +fi branch="" remote="" if [ -n "$current_branch" ]; then diff --git a/bin/lib/test-git-pr.sh b/bin/lib/test-git-pr.sh new file mode 100755 index 0000000..14a279e --- /dev/null +++ b/bin/lib/test-git-pr.sh @@ -0,0 +1,109 @@ +#!/bin/bash +# Tests for git-pr: resolving a PR URL from a detached HEAD. +# +# Usage: test-git-pr.sh +# +# Builds a throwaway repo with a github.com origin and drives git-pr as a +# subprocess. The gh calls hit a PATH shim whose answers come from env vars +# (GH_API_JSON for the commits/pulls endpoint, GH_VIEW_RC for the bare +# `gh pr view` fallback), so every case is offline; the controlled PATH keeps +# system git visible and the real gh invisible. The shim appends each +# subcommand to $CALLS so a test can assert a path was never taken. +# Cleans up on exit. + +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +# shellcheck source=bin/lib/test-helpers.sh +source "$SCRIPT_DIR/test-helpers.sh" + +BIN="$SCRIPT_DIR/../git-pr" + +# ── Fixture ────────────────────────────────────────────────────────────────── + +TESTTMP="$(cd "$(mktemp -d)" && pwd -P)" +trap 'rm -rf "$TESTTMP"' EXIT + +WORK="$TESTTMP/work" +CALLS="$TESTTMP/calls" + +mkdir -p "$WORK" +git -C "$WORK" init -q +git -C "$WORK" config user.email test@example.com +git -C "$WORK" config user.name "Test" +git -C "$WORK" config commit.gpgsign false +git -C "$WORK" checkout -q -b main +git -C "$WORK" remote add origin git@github.com:haacked/dotfiles.git +echo one > "$WORK/one" +git -C "$WORK" add one +git -C "$WORK" commit -qm one + +HEAD_SHA=$(git -C "$WORK" rev-parse HEAD) + +SHIM_PATH="$TESTTMP/bin:/usr/bin:/bin" +mkdir -p "$TESTTMP/bin" +cat > "$TESTTMP/bin/gh" <<'SHIM' +#!/bin/bash +echo "$1 $2" >> "$CALLS" +if [ "$1" = api ]; then + printf '%s' "${GH_API_JSON-[]}" | jq -r "${4-.}" + exit 0 +fi +exit "${GH_VIEW_RC:-1}" +SHIM +chmod +x "$TESTTMP/bin/gh" + +cd "$WORK" || exit 1 + +# Runs git-pr with the shim on PATH, capturing stdout into $OUT and the exit +# status into $RC. Truncates the call log first so each case asserts its own. +run_git_pr() { # run_git_pr [VAR=value ...] + : > "$CALLS" + RC=0 + OUT=$(env PATH="$SHIM_PATH" CALLS="$CALLS" "$@" bash "$BIN" 2>/dev/null) || RC=$? +} + +# A commits/pulls response holding one PR. +pull_json() { # pull_json + jq -n -c --arg sha "$1" --arg state "$2" --arg url "$3" \ + '[{head: {sha: $sha, ref: "haacked/x"}, state: $state, html_url: $url}]' +} + +# ── Test: detached HEAD with a matching head.sha resolves ──────────────────── + +git checkout -q --detach HEAD + +run_git_pr GH_API_JSON="$(pull_json "$HEAD_SHA" open https://github.com/haacked/dotfiles/pull/7)" +assert "detached HEAD exits 0 on a match" test "$RC" -eq 0 +assert "detached HEAD prints the PR URL" test "$OUT" = https://github.com/haacked/dotfiles/pull/7 +assert_not "detached HEAD never falls back to gh pr view" grep -q 'pr view' "$CALLS" + +# ── Test: a PR the commit merged into is not this commit's PR ──────────────── + +run_git_pr GH_API_JSON="$(pull_json 0000000000000000000000000000000000000000 closed https://github.com/haacked/dotfiles/pull/8)" +assert "a non-matching head.sha exits non-zero" test "$RC" -ne 0 +assert "a non-matching head.sha prints nothing" test -z "$OUT" +assert "a non-matching head.sha falls through to gh pr view" grep -q 'pr view' "$CALLS" + +# ── Test: an open PR outranks a closed one ────────────────────────────────── + +BOTH=$(jq -n -c --arg sha "$HEAD_SHA" \ + '[{head: {sha: $sha, ref: "a"}, state: "closed", html_url: "https://github.com/haacked/dotfiles/pull/1"}, + {head: {sha: $sha, ref: "b"}, state: "open", html_url: "https://github.com/haacked/dotfiles/pull/2"}]') +run_git_pr GH_API_JSON="$BOTH" +assert "an open PR outranks a closed one" test "$OUT" = https://github.com/haacked/dotfiles/pull/2 + +# ── Test: no associated PR keeps the existing failure ─────────────────────── + +run_git_pr GH_API_JSON='[]' +assert "no associated PR exits non-zero" test "$RC" -ne 0 +assert "no associated PR falls through to gh pr view" grep -q 'pr view' "$CALLS" + +# ── Test: an attached branch never queries the commit endpoint ────────────── + +git checkout -q main +run_git_pr GH_API_JSON="$(pull_json "$HEAD_SHA" open https://github.com/haacked/dotfiles/pull/7)" +assert_not "an attached branch never calls gh api" grep -q '^api' "$CALLS" + +print_results From ba955d70b9dd052b809daaf1bc607a19f76ec91d Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Wed, 16 Sep 2026 09:21:24 -0700 Subject: [PATCH 2/3] Give the repo-context test fixture a git identity A CI runner has no global user.name or user.email, so the fixture's commit failed and left HEAD unborn. The detach then read "HEAD" as a path. Generated-By: PostHog Desktop Task-Id: 4e81ba70-85c0-47fa-9cf7-26fce96f35a1 --- ai/helpers/tests/test-repo-context.sh | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ai/helpers/tests/test-repo-context.sh b/ai/helpers/tests/test-repo-context.sh index d849d5a..2b08cff 100755 --- a/ai/helpers/tests/test-repo-context.sh +++ b/ai/helpers/tests/test-repo-context.sh @@ -62,8 +62,11 @@ try_branch() { # [detach] [network] [[ "${1-}" == detach ]] && { detach=yes; shift; } d=$(mktemp -d) git -C "$d" init -q -b haacked/work - git -C "$d" commit -q --allow-empty -m first - [[ -n "$detach" ]] && git -C "$d" checkout -q --detach HEAD + # A CI runner has no global git identity, and an unborn HEAD makes the + # detach below read "HEAD" as a path instead. + git -C "$d" -c user.email=test@example.com -c user.name=Test \ + -c commit.gpgsign=false commit -q --allow-empty -m first + [[ -n "$detach" ]] && git -C "$d" checkout -q --detach (cd "$d" && PATH="${SHIM_DIR}:${PATH}" \ bash -c 'source "$1"; shift; resolve_branch_name "$@" || echo none' _ "${SCRIPT_DIR}/../repo-context.sh" "$@") rm -rf "$d" From bebaae8985272cdc3976c36379265e44c4501b49 Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Wed, 16 Sep 2026 10:10:00 -0700 Subject: [PATCH 3/3] Address PR review feedback Abort address-pr-reviews Step 1 when HEAD has moved off the PR head it resolved, so an invocation from a stale or different checkout can't push unrelated commits onto the PR's branch. Give resolve_branch_name a persisted-file cache tier, scoped to the checkout's private git dir, so a branch resolved via RAN_BRANCH or the network tier survives into a later process in the same checkout. This is what lets /ran read a detached checkout's branch after a commit moves HEAD off the PR head it was resolved from, such as under address-pr-reviews' --no-push path. Stop restating AGENTS.md's posting rule in github-pr-operations/SKILL.md, so the two can't drift out of sync again. Co-Authored-By: Claude Sonnet 5 Generated-By: PostHog Desktop Task-Id: 4e81ba70-85c0-47fa-9cf7-26fce96f35a1 --- ai/helpers/repo-context.sh | 43 +++++++++------ ai/helpers/tests/test-repo-context.sh | 47 +++++++++++++++- ai/skills/address-pr-reviews/SKILL.md | 4 +- ai/skills/github-pr-operations/SKILL.md | 2 +- .../ran/scripts/tests/test-ran-report.sh | 29 +++++++++- ai/tests/test-log-step-done.sh | 54 +++++++++++++++---- 6 files changed, 151 insertions(+), 28 deletions(-) diff --git a/ai/helpers/repo-context.sh b/ai/helpers/repo-context.sh index 6a8010f..0c99323 100644 --- a/ai/helpers/repo-context.sh +++ b/ai/helpers/repo-context.sh @@ -31,28 +31,41 @@ derive_org_repo() { # detached, where `git branch --show-current` is empty. RAN_BRANCH lets a caller # that already resolved the PR skip the last tier. Pass "network" to allow that # tier, which asks GitHub which PR has HEAD as its head commit; a caller that -# must never touch the network omits the argument and gets the first two tiers. +# must never touch the network omits the argument and gets the other tiers. +# +# A branch resolved through RAN_BRANCH or the network tier is cached in a file +# under this checkout's private git dir. That location is worktree-specific +# even for a linked worktree. A later call in the same checkout reads the +# cache instead of asking for RAN_BRANCH again or hitting the network. This is +# what lets `/ran` read a detached checkout's branch after a commit has moved +# HEAD off the PR head it was resolved from. # # Usage: resolve_branch_name [network] # Returns 1 when no tier answers. resolve_branch_name() { - local branch head_sha + local branch head_sha git_dir cache_file branch=$(git branch --show-current 2> /dev/null) || branch="" - [ -n "$branch" ] || branch="${RAN_BRANCH:-}" - if [ -z "$branch" ] && [ "${1:-}" = "network" ]; then - head_sha=$(git rev-parse HEAD 2> /dev/null) || head_sha="" - if [ -n "$head_sha" ]; then - # The endpoint also lists PRs this commit merged into, so a detached - # main would otherwise resolve to whatever landed last. Only an exact - # head.sha match is this commit's own PR. - branch=$(env GIT_PR_HEAD_SHA="$head_sha" GH_PAGER= \ - gh api "repos/{owner}/{repo}/commits/${head_sha}/pulls" \ - --jq '[.[] | select(.head.sha == $ENV.GIT_PR_HEAD_SHA)] | sort_by(.state != "open") | .[0].head.ref' \ - 2> /dev/null) || branch="" - if [ "$branch" = "null" ]; then - branch="" + if [ -z "$branch" ]; then + git_dir=$(git rev-parse --git-dir 2> /dev/null) || git_dir="" + cache_file="${git_dir:+${git_dir}/ran-branch}" + branch="${RAN_BRANCH:-}" + [ -n "$branch" ] || [ -z "$cache_file" ] || branch=$(cat "$cache_file" 2> /dev/null) || branch="" + if [ -z "$branch" ] && [ "${1:-}" = "network" ]; then + head_sha=$(git rev-parse HEAD 2> /dev/null) || head_sha="" + if [ -n "$head_sha" ]; then + # The endpoint also lists PRs this commit merged into, so a detached + # main would otherwise resolve to whatever landed last. Only an exact + # head.sha match is this commit's own PR. + branch=$(env GIT_PR_HEAD_SHA="$head_sha" GH_PAGER= \ + gh api "repos/{owner}/{repo}/commits/${head_sha}/pulls" \ + --jq '[.[] | select(.head.sha == $ENV.GIT_PR_HEAD_SHA)] | sort_by(.state != "open") | .[0].head.ref' \ + 2> /dev/null) || branch="" + if [ "$branch" = "null" ]; then + branch="" + fi fi fi + [ -z "$branch" ] || [ -z "$cache_file" ] || printf '%s\n' "$branch" > "$cache_file" 2> /dev/null || true fi [ -n "$branch" ] || return 1 printf '%s\n' "$branch" diff --git a/ai/helpers/tests/test-repo-context.sh b/ai/helpers/tests/test-repo-context.sh index 2b08cff..f99295c 100755 --- a/ai/helpers/tests/test-repo-context.sh +++ b/ai/helpers/tests/test-repo-context.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash # Tests for derive_org_repo across origin URL shapes, and for resolve_branch_name -# across its three tiers. +# across its tiers. set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" @@ -88,6 +88,51 @@ check "detached prefers the open PR" \ "$(GH_API_JSON='[{"head":{"sha":"SELF","ref":"a"},"state":"closed"},{"head":{"sha":"SELF","ref":"b"},"state":"open"}]' try_branch detach network)" "b" check "detached with no associated PR" "$(GH_API_JSON='[]' try_branch detach network)" "none" +# ── resolve_branch_name cache tier ────────────────────────────────────────── +# A branch resolved via RAN_BRANCH or the network tier persists to a file +# under the checkout's private git dir. A later call in the same checkout, run +# as a separate process, reads that file instead of RAN_BRANCH or the network. +# This is what lets `/ran` read a detached checkout's branch after a commit +# has moved HEAD off the PR head it was resolved from. + +CACHE_REPO=$(mktemp -d) +NETWORK_REPO=$(mktemp -d) +trap 'rm -rf "$SHIM_DIR" "$CACHE_REPO" "$NETWORK_REPO"' EXIT + +git -C "$CACHE_REPO" init -q -b haacked/work +git -C "$CACHE_REPO" -c user.email=test@example.com -c user.name=Test \ + -c commit.gpgsign=false commit -q --allow-empty -m first +git -C "$CACHE_REPO" checkout -q --detach + +resolve_in_cache_repo() { # [args...] -> prints resolve_branch_name's answer or "none" + (cd "$CACHE_REPO" && PATH="${SHIM_DIR}:${PATH}" \ + bash -c 'source "$1"; shift; resolve_branch_name "$@" || echo none' _ "${SCRIPT_DIR}/../repo-context.sh" "$@") +} + +RAN_BRANCH=haacked/cached resolve_in_cache_repo > /dev/null +check "a RAN_BRANCH-resolved branch persists for a later call with no RAN_BRANCH" \ + "$(resolve_in_cache_repo)" "haacked/cached" +check "the cache tier never asks GitHub" \ + "$(GH_API_JSON='[{"head":{"sha":"x","ref":"haacked/other"},"state":"open"}]' resolve_in_cache_repo network)" "haacked/cached" + +git -C "$NETWORK_REPO" init -q -b haacked/work +git -C "$NETWORK_REPO" -c user.email=test@example.com -c user.name=Test \ + -c commit.gpgsign=false commit -q --allow-empty -m first +git -C "$NETWORK_REPO" checkout -q --detach +NETWORK_SHA=$(git -C "$NETWORK_REPO" rev-parse HEAD) + +resolve_in_network_repo() { # [args...] -> prints resolve_branch_name's answer or "none" + (cd "$NETWORK_REPO" && PATH="${SHIM_DIR}:${PATH}" \ + bash -c 'source "$1"; shift; resolve_branch_name "$@" || echo none' _ "${SCRIPT_DIR}/../repo-context.sh" "$@") +} + +MATCH_NETWORK=$(printf '[{"head":{"sha":"%s","ref":"haacked/from-network"},"state":"open"}]' "$NETWORK_SHA") +GH_API_JSON="$MATCH_NETWORK" resolve_in_network_repo network > /dev/null +check "a network-resolved branch persists for a later call with no network argument" \ + "$(resolve_in_network_repo)" "haacked/from-network" +check "the cache tier wins over a subsequent network miss, as a commit moving HEAD off the PR head causes" \ + "$(GH_API_JSON='[]' resolve_in_network_repo network)" "haacked/from-network" + echo "" echo "Passed: ${passes}, Failed: ${failures}" [[ "${failures}" -eq 0 ]] diff --git a/ai/skills/address-pr-reviews/SKILL.md b/ai/skills/address-pr-reviews/SKILL.md index 9367130..120a5ca 100644 --- a/ai/skills/address-pr-reviews/SKILL.md +++ b/ai/skills/address-pr-reviews/SKILL.md @@ -46,7 +46,9 @@ Remember and strip `--no-push` and `--unattended` — the detection script treat This outputs `{"pr_number", "org", "repo", "head_branch", "head_sha", "error"}`. JSON mode always exits 0, so read `.error`: if it is non-null, report it and stop. `org` and `repo` come back lowercased, which every `gh` call below accepts. -Save `PR_NUMBER`, `ORG`, `REPO` (as `/`), and `HEAD_BRANCH`. Resolve the PR once, here. An agent harness checks the PR head out detached, where the detection script resolves the PR from HEAD's commit; once this skill commits a fix, HEAD moves off the PR head and a second lookup finds nothing. Step 5 needs `HEAD_BRANCH` to push. +Save `PR_NUMBER`, `ORG`, `REPO` (as `/`), `HEAD_BRANCH`, and `HEAD_SHA`. Resolve the PR once, here. An agent harness checks the PR head out detached, where the detection script resolves the PR from HEAD's commit; once this skill commits a fix, HEAD moves off the PR head and a second lookup finds nothing. Step 5 needs `HEAD_BRANCH` to push. + +Abort now if `git rev-parse HEAD` differs from `HEAD_SHA`. An invocation with an explicit PR URL or number can run from a checkout that isn't the PR head — a stale worktree, or one left on unrelated commits — and Step 5's push would then carry that unrelated `HEAD` onto the PR's branch. ### Step 2: Fetch and Filter Unaddressed Comments diff --git a/ai/skills/github-pr-operations/SKILL.md b/ai/skills/github-pr-operations/SKILL.md index 4112c4e..74b6c5d 100644 --- a/ai/skills/github-pr-operations/SKILL.md +++ b/ai/skills/github-pr-operations/SKILL.md @@ -5,7 +5,7 @@ description: Reference for GitHub PR review endpoints and resolving review threa # GitHub PR Operations -Endpoint reference for PR review operations. The always-on rules (never post without approval, always use `gh` CLI, no volunteered AI attribution) live in `~/.dotfiles/ai/AGENTS.md` — this skill is just the mechanics. For the end-to-end review-comment workflow (deciding what's a real issue, drafting replies, when to resolve vs. leave open), use the `address-pr-reviews` skill instead — it embeds these same commands in context. +Endpoint reference for PR review operations. The always-on rules for when you may post, always use `gh` CLI, no volunteered AI attribution, live in `~/.dotfiles/ai/AGENTS.md` — this skill is just the mechanics. For the end-to-end review-comment workflow (deciding what's a real issue, drafting replies, when to resolve vs. leave open), use the `address-pr-reviews` skill instead — it embeds these same commands in context. ## Posting review comments diff --git a/ai/skills/ran/scripts/tests/test-ran-report.sh b/ai/skills/ran/scripts/tests/test-ran-report.sh index ac1c967..92ef27f 100755 --- a/ai/skills/ran/scripts/tests/test-ran-report.sh +++ b/ai/skills/ran/scripts/tests/test-ran-report.sh @@ -427,6 +427,11 @@ check "--json outside a repo reports an error field" json_has_error # An agent harness checks the PR head out detached, and log-step-done.sh records # against the PR's head ref from there. The report has to read that same log # back, or the branch's finished steps all render as never run. +# +# Each case gets its own repo. A resolved branch caches under the checkout's +# git dir. Reusing one repo across cases with different GH_API_JSON values +# would let an earlier case's cache answer a later case instead of the tier +# under test. DETACHED=$(new_repo detached) git -C "$DETACHED" checkout -q --detach HEAD @@ -438,8 +443,30 @@ check_eq "a detached HEAD exits 0" "$READER_STATUS" "0" check "it reports the branch its PR heads" out_has "Branch haacked/breadcrumbs" check_eq "it reads the log that branch's records land in" "$(marker simplify)" "✓" -GH_API_JSON='[]' run_reader "$DETACHED" +DETACHED_NO_PR=$(new_repo detached-no-pr) +git -C "$DETACHED_NO_PR" checkout -q --detach HEAD +GH_API_JSON='[]' run_reader "$DETACHED_NO_PR" check "a detached HEAD with no PR fails" test "$READER_STATUS" -ne 0 +# ── Detached HEAD after a commit ──────────────────────────────────────────── +# address-pr-reviews resolves the branch once, via RAN_BRANCH or this same +# network lookup, and hands it to log-step-done.sh. A later commit under +# --no-push, or before a push completes, moves HEAD off the PR head that +# lookup matched. A later /ran here then has no HEAD the network tier can +# match either. The cache resolve_branch_name wrote on the first lookup has to +# answer this one. + +DETACHED_CACHE=$(new_repo detached-cache) +git -C "$DETACHED_CACHE" checkout -q --detach HEAD +happy_path_log +GH_API_JSON='[{"head":{"sha":"SELF","ref":"haacked/breadcrumbs"},"state":"open"}]' \ + run_reader "$DETACHED_CACHE" +check_eq "the first lookup, still at the PR head, exits 0" "$READER_STATUS" "0" + +commit_at "$DETACHED_CACHE" "2026-08-28T09:00:00Z" "review fix" +GH_API_JSON='[]' run_reader "$DETACHED_CACHE" +check_eq "a later run past that commit still exits 0" "$READER_STATUS" "0" +check "it still reports the cached branch" out_has "Branch haacked/breadcrumbs" + summary [[ "${failures}" -eq 0 ]] diff --git a/ai/tests/test-log-step-done.sh b/ai/tests/test-log-step-done.sh index a7a205c..01b3a9a 100755 --- a/ai/tests/test-log-step-done.sh +++ b/ai/tests/test-log-step-done.sh @@ -192,37 +192,73 @@ check "a non-GitHub origin fails" test "$WRITER_STATUS" -ne 0 # An agent harness checks the PR head out detached, so there is no branch name # to record against. The record has to land on the branch an attended run on the # same work would write, or /ran and /go never see the completed pass. +# +# Each case below gets its own repo. resolve_branch_name caches a resolved +# branch under the checkout's git dir. Reusing one repo across cases with +# different RAN_BRANCH/GH_API_JSON values would let an earlier case's cache +# answer a later case instead of the tier under test. -DETACHED=$(make_repo detached "git@github.com:haacked/dotfiles.git" "haacked/tmp") -git -C "$DETACHED" checkout -q --detach HEAD -DETACHED_SHA=$(git -C "$DETACHED" rev-parse HEAD) -DETACHED_SHORT=$(git -C "$DETACHED" rev-parse --short HEAD) +DETACHED_ENV=$(make_repo detached-env "git@github.com:haacked/dotfiles.git" "haacked/tmp") +git -C "$DETACHED_ENV" checkout -q --detach HEAD +DETACHED_ENV_SHORT=$(git -C "$DETACHED_ENV" rev-parse --short HEAD) WRITER_ENV=(RAN_BRANCH=haacked/from-env) -run_writer "$DETACHED" review-code +run_writer "$DETACHED_ENV" review-code ENV_LOG="${STATE_ROOT}/haacked/dotfiles/haacked-from-env.jsonl" check_eq "a detached HEAD with RAN_BRANCH exits 0" "$WRITER_STATUS" "0" check_eq "it records against the branch RAN_BRANCH names" \ "$(field "$ENV_LOG" .branch)" "haacked/from-env" -check_eq "it still records HEAD's sha" "$(field "$ENV_LOG" .sha)" "$DETACHED_SHORT" +check_eq "it still records HEAD's sha" "$(field "$ENV_LOG" .sha)" "$DETACHED_ENV_SHORT" check "RAN_BRANCH skips the GitHub lookup" test ! -s "$GH_CALLS" -PR_JSON=$(jq -n -c --arg sha "$DETACHED_SHA" \ +DETACHED_PR=$(make_repo detached-pr "git@github.com:haacked/dotfiles.git" "haacked/tmp") +git -C "$DETACHED_PR" checkout -q --detach HEAD +DETACHED_PR_SHA=$(git -C "$DETACHED_PR" rev-parse HEAD) +PR_JSON=$(jq -n -c --arg sha "$DETACHED_PR_SHA" \ '[{head: {sha: $sha, ref: "haacked/from-pr"}, state: "open"}]') WRITER_ENV=(GH_API_JSON="$PR_JSON") -run_writer "$DETACHED" review-code +run_writer "$DETACHED_PR" review-code PR_LOG="${STATE_ROOT}/haacked/dotfiles/haacked-from-pr.jsonl" check_eq "a detached HEAD resolves the branch from its PR" "$WRITER_STATUS" "0" check_eq "it records against the PR's head ref" \ "$(field "$PR_LOG" .branch)" "haacked/from-pr" +DETACHED_NONE=$(make_repo detached-none "git@github.com:haacked/dotfiles.git" "haacked/tmp") +git -C "$DETACHED_NONE" checkout -q --detach HEAD BEFORE_DETACHED=$(state_files | wc -l | tr -d ' ') WRITER_ENV=(GH_API_JSON='[]') -run_writer "$DETACHED" review-code +run_writer "$DETACHED_NONE" review-code check "a detached HEAD with no PR fails" test "$WRITER_STATUS" -ne 0 check "it explains itself on stderr" test "$(stderr_bytes)" -gt 0 check_eq "it writes nothing" "$(state_files | wc -l | tr -d ' ')" "$BEFORE_DETACHED" +# ── Detached HEAD after a commit ──────────────────────────────────────────── +# address-pr-reviews resolves RAN_BRANCH once and passes it to this writer. +# A later commit under --no-push, or before a push completes, moves HEAD off +# the PR head RAN_BRANCH was resolved from. A second run then has neither +# RAN_BRANCH nor a HEAD the network tier can match. The cache resolve_branch_name +# wrote on the first run has to answer the second. + +DETACHED_CACHE=$(make_repo detached-cache "git@github.com:haacked/dotfiles.git" "haacked/tmp") +git -C "$DETACHED_CACHE" checkout -q --detach HEAD + +WRITER_ENV=(RAN_BRANCH=haacked/cached) +run_writer "$DETACHED_CACHE" review-code +check_eq "the first run with RAN_BRANCH exits 0" "$WRITER_STATUS" "0" + +git -C "$DETACHED_CACHE" -c user.email=test@example.com -c user.name=Test \ + -c commit.gpgsign=false commit -q --allow-empty -m "review fix" + +WRITER_ENV=(GH_API_JSON='[]') +run_writer "$DETACHED_CACHE" simplify +CACHE_LOG="${STATE_ROOT}/haacked/dotfiles/haacked-cached.jsonl" +check_eq "a later run with no RAN_BRANCH and no PR match still exits 0" "$WRITER_STATUS" "0" +check_eq "both runs record against the cached branch, in one log" \ + "$(jq -s 'length' "$CACHE_LOG" 2> /dev/null)" "2" +check_eq "the later run's step is recorded" \ + "$(jq -s '[.[] | select(.step == "simplify")] | length' "$CACHE_LOG" 2> /dev/null)" "1" +check "the cache tier answers before the network tier is asked" test ! -s "$GH_CALLS" + # ── Containment ────────────────────────────────────────────────────────────── # The org, repo, and branch become path components here exactly as they do in # the hook, so the same traversal guard has to hold.