diff --git a/AGENTS.md b/AGENTS.md index 2dc50f8f6..5d7c3dc8d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -558,6 +558,33 @@ This grants no merge authority: the strict merge policy below still applies. (`gh issue reopen `) per [`revert-merge.md`](shared/workflow/revert-merge.md). +## Only work PRs opened by the user, assigned to the user, explicitly requested by the user, or authored by the Actions app + +Before pushing to, editing, commenting on, reviewing, resolving threads on, +dispatching a paid review of, or merging any PR, resolve the invoking user +and read the PR's author and assignees. +Proceed only when the author or one of the assignees is that user (or an +alias `memories/reviewing-prs.md` lists for that same user), the user +explicitly asked for work on that PR by name (or, through an explicit +`chores` call, on the Dependabot/Renovate population), or the author is the +GitHub Actions app (`github-actions`). +A mention such as "do not touch" followed by a PR number is not a request, a +claim comment confers no scope, and a sweep skill's "every open PR" means +every PR that passes this test. +An explicit exclusion ("do not touch" followed by a PR number) is a veto: it +removes that PR before any positive arm is evaluated, the user's own PRs and +the Actions app's included, and every sweep carries the exclusion list into +each recheck and each delegated scan. +A review-only run that CI or a skill invocation dispatched naming the target +PR (an `@claude review`, a `claude-code-review.yml` run) is that explicit +request, whoever authored the PR; it reviews and stops there. +An out-of-scope PR is reported to the user and left untouched. +When no identity operation is available, fail closed the way `ardia` does: +leave the author and assignee arms unevaluated, act only on PRs the user +explicitly asked for or the Actions app authored, and say so in the report. +`memories/reviewing-prs.md` carries the full rule and its provenance; +`skills/ardia/SKILL.md` step 1 is the reference implementation. + ## Always arm a persistent PR loop This applies in any repo, not only Morrison-Lab ones. diff --git a/CLAUDE.md b/CLAUDE.md index c8e4f7d97..0f5dd4bdd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -617,6 +617,8 @@ One to three sentences is enough. The trailing marker is required, per the section above: this comment paraphrases the user in the user's own voice under the user's own login, which is the shape most easily read as their own writing. Don't quote verbatim — paraphrase so it reads naturally in the PR thread. Skip trivial acknowledgments or conversational exchanges with nothing to act on. +Post it only on a PR that passes `memories/reviewing-prs.md`'s scope test. +Feedback about an out-of-scope PR, such as a request not to touch it, stays in chat and the session notebook rather than on that PR. This makes context visible to future @claude sessions, other reviewers, and contributors who only see the PR thread. @@ -930,7 +932,7 @@ The key points, restated here because a bare pointer is invisible to a consumer Attribution is a second axis, and it runs before the claim: intersect the merge's own deleted and renamed paths (`git diff --name-status -M "$merge^1" "$merge" | grep -E '^(D|R)'`) with each conflict, and report conflicts caused alongside conflicts found. `git show --name-status ` cannot supply that set for a **true** (two-parent) merge --- it prints no file list at all there, and grepping its header for `^[ADMR]` returns three phantom paths. It does diff a squash merge normally, so whether it works depends on how the repo merges rather than on the commit in front of you. - A conflict you caused on a branch you do not own is an explanatory comment, not a push. + A conflict you caused on a PR that fails `memories/reviewing-prs.md`'s scope test is a report to the user, not a comment or a push. - **Independent per-PR checking cannot see pair collisions.** Every PR can be clean against `main` while two of them conflict with each other. Only a pairwise `git merge-tree` between PR heads finds that. @@ -1817,6 +1819,8 @@ recurred immediately in a `jq` filter reading a PR review body.) A peer may have further commits planned, so merging one that just went clean can destroy work it was about to push --- and that is exactly the case where the peer's PR unblocks yours and the temptation is strongest. Start the clock at the clean verdict on the current head, which a push resets, rather than at the PR's `updatedAt`, which any comment bumps. The threshold is an inference, so confirm it: message the owning session directly when `ListAgents` reaches it, and otherwise post a comment saying you intend to merge and wait a further five minutes for a hold-off. + The path applies only to a peer PR that passes `memories/reviewing-prs.md`'s scope test (a peer session under your own login satisfies the author arm). + Another lab member's PR that fails the test gets neither the comment nor the merge. [`mwc`](skills/mwc/SKILL.md)'s "Another session's PR" section carries the derivation and the pattern/anti-pattern pair (ai-config#2460). **One standing exception: PRs targeting `Morrison-Lab/ai-config` carry a standing `mwc` grant**, with no per-session re-issue and no `enable-mwc` step --- `hooks/no-unauthorized-merge.py` reads the merge's target repo off the command. diff --git a/codex-skills/ardia/SKILL.md b/codex-skills/ardia/SKILL.md index eba74ac35..d3a1692a0 100644 --- a/codex-skills/ardia/SKILL.md +++ b/codex-skills/ardia/SKILL.md @@ -1,6 +1,6 @@ --- name: "ardia" -description: "Drive all open PRs to clean." +description: "Drive every in-scope open PR to clean." --- # ardia (Codex wrapper) diff --git a/memories/MEMORY.md b/memories/MEMORY.md index 1d34309ce..23c9f0077 100644 --- a/memories/MEMORY.md +++ b/memories/MEMORY.md @@ -13,7 +13,7 @@ index in that directory. | File | Title | Covers | |------|-------|--------| | [`preferences.md`](preferences.md) | User preferences (cross-workspace) | Standing working rules: never-assume/always-verify, record learnings as you go, cite sources for tool-behavior claims, issue-first, and the ARDI / fully-clean definitions. | -| [`reviewing-prs.md`](reviewing-prs.md) | Reviewing someone else's PR | Leftover-artifact findings follow what the PR is landing, owner scope vs the author's mechanical box, review-only is not ARDI, and post every finding already in hand, split from `preferences.md` at the 1200-line gate (UCD-SERG/shigella#31). | +| [`reviewing-prs.md`](reviewing-prs.md) | Reviewing someone else's PR | Leftover-artifact findings follow what the PR is landing, owner scope vs the author's mechanical box, review-only is not ARDI, post every finding already in hand, and only work PRs opened by the user, assigned to the user, explicitly requested by name, or authored by the Actions app (a sweep's "every open PR" is not a scope grant; UCD-SERG/serodynamics 2026-09-01), split from `preferences.md` at the 1200-line gate (UCD-SERG/shigella#31). | | [`github.md`](github.md) | GitHub PR/issue queue management | Operational write-action checklist, stale remotes, the GII startup sweep, stacked-PR pitfalls, consumer enumeration (`gh search code`), secondary rate limits, and `gh pr edit`. The `gh` CLI itself, GitLab, remote-session bash access, and consumer-CI content now live in their own split files (linked at the top). | | [`gh-cli.md`](gh-cli.md) | GitHub CLI (`gh`) | The `gh` CLI behavior, rate limits, pagination, handling `GH_PAGER`, GraphQL vs REST pools, and fallback strategies. Split out of `github.md`. | | [`github-remote-sessions.md`](github-remote-sessions.md) | GitHub access from bash in remote/web sessions | What a remote or web session can reach on GitHub from bash when `gh`/`glab` are absent: session-scope 403s, the MCP-then-`add_repo`-then-`git ls-remote` ladder, the proxy's push-but-not-delete rule, and GitHub Pages policy denials. Split out of `github.md` (ai-config#694 pattern) at the 1200-line gate. | diff --git a/memories/github.md b/memories/github.md index 3be1a8898..daf1ccbcb 100644 --- a/memories/github.md +++ b/memories/github.md @@ -65,8 +65,16 @@ Moved to [`gh-cli.md`](gh-cli.md). When starting a GII loop, do a cleanup pass before diving into ARDI: -1. **List all open PRs** with `mcp__github__list_pull_requests`. Look for - stale bot-opened PRs that target the same issues as the queue. +1. **List all open PRs** with `mcp__github__list_pull_requests` (fields + `number,title,user,assignees`). + Keep the full list for step 3's issue-coverage detection, which only reads + it, and apply `reviewing-prs.md`'s scope test (opened by or assigned to the + invoking user, explicitly requested, or authored by the GitHub Actions app) + immediately before every mutation in steps 2 through 4 (the close in step + 2, the close and the note in step 3, the merge in step 4): an out-of-scope + PR is reported to the user rather than closed, commented on, or merged + into, and an issue it already covers is left to it rather than grabbed. + Look for stale bot-opened PRs that target the same issues as the queue. 2. **Close empty PRs** — bot-opened branches with no commits (e.g. a `@claude` task run that posted a comment but never pushed code). Check `get_commits` on each PR before closing. diff --git a/memories/preferences.md b/memories/preferences.md index 566f0b8ae..58f572ad9 100644 --- a/memories/preferences.md +++ b/memories/preferences.md @@ -318,7 +318,7 @@ Merge the most-isolated PR (disjoint files) FIRST --- it rides through without a re-resync; sequence foundational/big same-file PRs LAST so lighter PRs rebase onto simpler `main`. An agent watching a PR must POLL its own `mergeable`/`mergeStateStatus` on EVERY watch tick (a newly-appearing conflict from someone else's merge is NOT a CI event, so a CI-completion monitor never fires on it), and on catching one immediately `git fetch origin main && git merge origin/main`, resolve, re-run checks, push --- staying in the watch loop until the PR is merged or closed (clean regresses to CONFLICTING when main moves). The coordinator's nudge is only a backstop for a genuinely-dead agent. (Learned on sparta 2026-07-01 merging the movement cluster.) - Beyond same-file collisions: after ANY merge that advances the base (`main`), proactively re-sync EVERY trailing open PR branch and resolve conflicts --- don't wait for a branch to show DIRTY or for the next review trigger. + Beyond same-file collisions: after ANY merge that advances the base (`main`), proactively re-sync EVERY trailing open PR branch that passes `memories/reviewing-prs.md`'s scope test and resolve conflicts (an out-of-scope branch is reported to the user and left untouched) --- don't wait for a branch to show DIRTY or for the next review trigger. In R packages the recurring conflicts are DESCRIPTION `Version:` (bump above main) and NEWS.md (union-merge, keeping both sides' bullets and one subsection per heading); the `@claude` bot auto-syncs non-conflicting branches but does NOT resolve these real DESCRIPTION/NEWS conflicts. Sequential merges cascade version-check reds and NEWS/DESCRIPTION conflicts down the whole stack of trailing PRs, so keeping them all synced after each merge keeps the queue mergeable; parallelize with worktree-isolated workers, capped at ~3 concurrent to respect shared CI runners. (Learned on ucdavis/bcs.) **The DESCRIPTION half of this cascade is obsolete once a repo adopts `Morrison-Lab/gha`'s new `bump-dev-version`/`version-check` capabilities (gha#390, tracking gha#388)** --- PRs stop touching `Version:` at all, so there's no version-bump conflict left to cascade down the stack. @@ -397,8 +397,10 @@ The existing instruction already covered this; the gap was execution discipline in a fast multi-merge loop, not missing guidance --- re-read this bullet at the top of every "pick the next backlog item" cycle. In a multi-AGENT pipeline, UMS runs at BOTH levels: each subagent runs UMS once ITS PR merges (it stops after reporting CLEAN, so the coordinator resumes it post-merge with a "your PR merged, run UMS" nudge --- or the agent-launch spec bakes in a final UMS step), and the coordinator runs its own UMS for the cross-PR orchestration learnings no single subagent can see (merge-order sequencing, conflict-cascade handling, pipeline mechanics). Each agent writes its OWN memory file plus one MEMORY.md index line to keep the conflict surface small; avoid rewriting shared memory bodies concurrently. (Learned on sparta 2026-07-01.) -- After ANY PR merges to main (under mwc, post-merge, or manual merge), IMMEDIATELY and autonomously sweep all open PRs in the repository for merge conflicts (`gh pr list --state open --json number,title,headRefName,mergeable,mergeStateStatus`). - For any PR reporting `CONFLICTING` or `UNKNOWN`, fetch main, test the merge, resolve the conflict in an isolated worktree, and push the sync commit proactively without waiting for the user to point it out or ask for it. (Learned on ai-config, 2026-08-24: "cai: you should have checked PR conflicts on your own".) +- After ANY PR merges to main (under mwc, post-merge, or manual merge), IMMEDIATELY and autonomously sweep all open PRs in the repository for merge conflicts (`gh pr list --state open --json number,title,headRefName,author,assignees,mergeable,mergeStateStatus`). + Filter that list by `memories/reviewing-prs.md`'s scope test first (opened by or assigned to the invoking user, explicitly requested by name, or authored by the GitHub Actions app); + an out-of-scope conflicting PR is reported to the user and left untouched. + For any in-scope PR reporting `CONFLICTING` or `UNKNOWN`, fetch main, test the merge, resolve the conflict in an isolated worktree, and push the sync commit proactively without waiting for the user to point it out or ask for it. (Learned on ai-config, 2026-08-24: "cai: you should have checked PR conflicts on your own".) - Keep it simple. Don't over-explain or ask permission for straightforward fixes --- just do them. - Don't re-ask a decision that's already settled and built. @@ -531,9 +533,12 @@ Both additions were already on `main` in fuller form, and the diff had also rewritten three *correct* relative links into broken ones --- the `check-links.py` failure being blamed on that session all along.) - **Don't touch anyone else's branch.** - **Do:** only push to or modify branches I created in my own worktree. - **Don't:** push commits, force-push, checkout, or edit branches belonging to another session or user --- even if the content looks worth keeping or the branch looks abandoned. - If a branch needs work that isn't mine, flag it and let the owner handle it. (User directive, 2026-08-19.) + **Do:** only push to or modify branches I created in my own worktree, or a PR branch that passes `memories/reviewing-prs.md`'s scope test (opened by me, assigned to me, explicitly requested, or the Actions app's) and carries no live claim from another session. + **Don't:** push commits, force-push, checkout, or edit branches belonging to another session or user that fail that test --- even if the content looks worth keeping or the branch looks abandoned. + If a branch needs work that isn't mine, flag it and let the owner handle it. + A live claim on an in-scope branch still means waiting for it to expire, per `claim-pr`. + (User directive, 2026-08-19. + The scope-test carve-out follows the 2026-09-01 directives in `reviewing-prs.md`.) - **A delegated subagent runs in the parent session's working tree, so the "Use ONE worktree per branch/PR" rule above governs your own agents, not only other sessions.** The remedy is already written down: [`gip`](../skills/gip/SKILL.md) says to give every subagent `isolation: "worktree"`, and diff --git a/memories/reviewing-prs.md b/memories/reviewing-prs.md index f901b29ed..c18c26a44 100644 --- a/memories/reviewing-prs.md +++ b/memories/reviewing-prs.md @@ -65,6 +65,61 @@ A later request to iterate is a driving request. branch, not when you were asked only to read it. See also [`shared/workflow/ardi.md`](../shared/workflow/ardi.md). +## Only work PRs I opened, am assigned to, or asked for by name, or the Actions app authored + +Work only on pull requests opened by me (`d-morrison` or `dem-extra1`), +assigned to me, +explicitly asked for by name in the request, +or authored by the GitHub Actions app +(app slug `github-actions`, +which the APIs surface as `github-actions[bot]`, bare `github-actions`, or `app/github-actions` +--- a submodule bump, an automated sync PR). +A PR opened by another lab member, or by any other bot, +is not mine to drive unless it is assigned to me or I explicitly asked for it +--- however clean, stale, or easy it looks, +and however a sweep skill words its scope. + +The user stated the **Do** side, in two steps: first the author/assignee test, then (same day) that workflow-opened PRs such as a submodule bump are fine. +The **Don't** side is inferred from the near-miss that prompted it, and is what makes the rule checkable. +The named-in-request arm came from neither directive: it is carried over from `ardia`'s former "unless told to" bullet, which this rule replaced. +The narrowing of "workflow-opened" to the `github-actions[bot]` login is also inferred: the example given was a `bump-submodule.yml` PR, and Dependabot and Copilot PRs post under their own logins and were not named. +That login match covers a workflow that opens its PR with `GITHUB_TOKEN`; +`gha`'s reusable sync workflows (`bump-submodule.yml`, `bump-dev-version.yml`, `sync-shared-fragments.yml`, `sync-upstream.yml`, read at `Morrison-Lab/gha` `82f3c36` on 2026-09-01) hand `open-sync-pr` `${{ secrets.WORKFLOW_TOKEN || github.token }}`, so when the repo sets that secret the PR posts under that token's identity: the PAT holder's own login, or an App's `[bot]`. +The rule therefore narrows the directive's "workflow-opened" on purpose, because provenance is not observable from the API: the arm is an author test on the `github-actions` app, not a provenance test, and such a PR is in scope through the author arm when the PAT is mine, and needs an assignment or naming under an App token or another member's PAT, however it was opened. + +- **Do:** treat `d-morrison` and `dem-extra1` as one person --- this corpus's owner --- when either is the invoking user (`preferences.md`'s `## Git author mapping` entry records the same two-account split, with the owner written as "the repository owner"). + They are written as literals here on purpose: `preferences.md`'s "Never hardcode usernames" rule exempts values that must resolve to a real account, and an `author.login` match is exactly that. + A skill must still resolve the invoking user dynamically and only *add* these aliases, so another lab member running the vendored corpus filters on their own identity, not on these two. +- **Do:** before touching any PR, resolve who you are running as, read the PR's author and assignees (`author.login` / `assignees[].login` from `gh --json`, `user.login` / `assignees[].login` from REST, `user.login` plus bare login strings in `assignees` from the MCP tools --- `mcp__github__list_pull_requests` and `pull_request_read` returned `"assignees": ["d-morrison"]` on 2026-09-01, and the list tool omits the key on an unassigned PR --- and `author.username` / `assignees[].username` from GitLab), and proceed only when the author is you (or one of those aliases) or the GitHub Actions app (`github-actions`, in whichever form the source returns it), you or one of those aliases is among the assignees, or the user explicitly asked for work on that PR. + A mention is not a request: "do not touch [#284](https://github.com/UCD-SERG/serodynamics/pull/284)" names [#284](https://github.com/UCD-SERG/serodynamics/pull/284) and excludes it, and a link given as context authorizes nothing. + An explicit exclusion is the opposite of a request: it is a veto checked before every positive arm, so a PR I authored, am assigned to, or the Actions app opened is still left untouched when the request says not to touch it, and the exclusion list travels with the sweep into every recheck and every delegated scan (`skills/chores/SKILL.md`'s `PR_SCOPE_EXCLUDED` is the executable form). + Match the app slug `github-actions` in whichever form the source returns it (forms measured 2026-09-01 against `cli/cli` trunk and the REST API; + re-check if a tool's output format changes): the REST API and the MCP tools suffix it (`github-actions[bot]`), GraphQL and `scripts/pr-sweep.py` return it bare (`github-actions`), and `gh pr list --json author` prefixes it (`app/github-actions`, with `is_bot: true`). +- **Do:** on a sweep (`ardia`, `gia`, `ardiaei`, `gmd`), filter the PR list by that test first, and say in the report which PRs were excluded and why. +- **Do (inferred):** when no identity operation is available (no `gh`, no `mcp__github__get_me`, no `glab`), fail closed: + leave the author and assignee arms unevaluated, keep only the PRs the user explicitly asked for or the Actions app authored, and say so in the report. + `skills/ardia/SKILL.md` step 1 and `AGENTS.md` state the same fallback. +- **Don't:** push commits to, rewrite the title or body of, comment on, review, dispatch a paid review on, resolve threads on, or merge a PR that fails the test. + A review-only run that CI or a skill invocation dispatched naming the PR (an `@claude review`, a `claude-code-review.yml` run) satisfies the explicit-request arm for that review whoever the author is, and stops at the review per "Review-only is not working the PR" above. +- **Don't:** read a skill's "drive every open PR" as a scope grant that overrides this --- "every" means every PR that is mine. +- **Don't:** treat a PR from a bot other than the GitHub Actions app (a Dependabot PR, a Copilot-agent PR, a PR another App's token opened) as mine by default; + such a PR needs an assignment or an explicit request like any other. + An explicit `chores` invocation names the Dependabot/Renovate population, which is the named-in-request arm. +- **Don't:** stand down from a PR the `github-actions` app authored on the ground that a bot opened it --- that is the over-correction the second directive reversed. + +The near-miss looks like diligence from the inside, which is why the rule needs the explicit Don'ts. +On `UCD-SERG/serodynamics`, 2026-09-01, a `gia` sweep pushed commits to [#284](https://github.com/UCD-SERG/serodynamics/pull/284), [#292](https://github.com/UCD-SERG/serodynamics/pull/292), [#298](https://github.com/UCD-SERG/serodynamics/pull/298) and [#311](https://github.com/UCD-SERG/serodynamics/pull/311) and dispatched a review on [#310](https://github.com/UCD-SERG/serodynamics/pull/310) --- four authored by other lab members, one ([#292](https://github.com/UCD-SERG/serodynamics/pull/292)) by `github-actions[bot]`, none assigned to me --- and drove all of them to a clean verdict before the correction arrived. +The session then stood down from all five, and the user reversed that for the workflow-opened one, [#292](https://github.com/UCD-SERG/serodynamics/pull/292), which was fine to drive. +So the population error was the four human-authored PRs, and the over-correction was the fifth. +Every individual action was a correct ARDI step. +The error was the population, decided by reading "every open PR" in the skill rather than by asking whose PRs they were. +Each of those threads got one disclosure comment naming the commits (or, on [#310](https://github.com/UCD-SERG/serodynamics/pull/310), the dispatched review), so the authors can keep or revert them. + +The issue carve-out is inferred too: nothing in either directive mentioned issues. +An issue on a repo I own is different from a PR on it: +filing, triaging, and commenting on issues is fine, +and an issue someone else's open PR already fixes is left to that PR (not grabbed, and that PR not driven either). + ## Search the issue thread before rebutting "no source exists" A reviewer asked for a permalink to the post a chapter summarized. diff --git a/scripts/pr-overlap.py b/scripts/pr-overlap.py index e8deac8ee..28a266e0f 100644 --- a/scripts/pr-overlap.py +++ b/scripts/pr-overlap.py @@ -84,7 +84,9 @@ Read-only. It reports; it never pushes, comments, closes, or merges. Reporting two PRs as colliding is not authorization to drive either one -- -`shared/workflow/ardi.md` limits that to PRs a session owns or has claimed. +`shared/workflow/ardi.md` limits that to PRs that pass +`memories/reviewing-prs.md`'s scope test (opened by or assigned to the +invoking user, explicitly requested by name, or authored by the GitHub Actions app (`github-actions`)). """ from __future__ import annotations diff --git a/scripts/pr-sweep.py b/scripts/pr-sweep.py index 24d514b94..974bdf84c 100644 --- a/scripts/pr-sweep.py +++ b/scripts/pr-sweep.py @@ -39,7 +39,9 @@ Read-only. It reports; it never pushes, comments, or merges. Reporting a PR as stalled is not authorization to drive it -- `shared/workflow/ardi.md` -limits that to PRs a session owns or has explicitly claimed. +limits that to PRs that pass `memories/reviewing-prs.md`'s scope test +(opened by or assigned to the invoking user, explicitly requested by name, +or authored by the GitHub Actions app). """ from __future__ import annotations diff --git a/scripts/validate-skills.py b/scripts/validate-skills.py index f47dd43d7..d0b6a8e56 100755 --- a/scripts/validate-skills.py +++ b/scripts/validate-skills.py @@ -260,6 +260,11 @@ def check_skills() -> None: "OPENROUTER_API_KEY", "PROJECT_ID", "PR_NUMBER", + # env vars: the chores skill's scope inputs (aliases of the invoking user, + # PR numbers named in the request), per memories/reviewing-prs.md + "PR_SCOPE_ALIASES", + "PR_SCOPE_EXCLUDED", + "PR_SCOPE_REQUESTED", "REBASE_HEAD", "REVERT_HEAD", "R_LIBS_USER", diff --git a/shared/workflow/ardi.md b/shared/workflow/ardi.md index 38f49758b..e5d5fcfb8 100644 --- a/shared/workflow/ardi.md +++ b/shared/workflow/ardi.md @@ -151,9 +151,12 @@ When the timer fires: - If the reviewer failed, was canceled, skipped with no replacement (e.g. quota limit), or produced a stub review with no stated verdict: invoke self-review fallback per [`self-review-fallback.md`](self-review-fallback.md) rather than stalling the loop. - Otherwise, fix any underlying workflow or dispatch issues discovered along the way and schedule another timer to maintain continuous monitoring until a review lands, self-review fallback triggers, or CI completes. This applies transitively to PR-driving -workflows such as `gi`, `gii`, and `ardia`; only monitor PRs the session owns or -has explicitly claimed, so the rule does not authorize changing someone else's -work. +workflows such as `gi`, `gii`, and `ardia`; +only monitor PRs that pass `memories/reviewing-prs.md`'s scope test +(opened by or assigned to the invoking user, explicitly requested by name, +or authored by the GitHub Actions app), +so the rule does not authorize changing someone else's work, +and a claim comment does not bring another member's PR into scope. The loop's terminal action is to **report the PR ready, not to merge it**. Merging is human-gated --- it happens only on an explicit human "merge it" (the diff --git a/shared/workflow/batch-merge-and-resolve.cases.md b/shared/workflow/batch-merge-and-resolve.cases.md index 973a8e046..7d640ea9d 100644 --- a/shared/workflow/batch-merge-and-resolve.cases.md +++ b/shared/workflow/batch-merge-and-resolve.cases.md @@ -32,8 +32,11 @@ PR #555 looked caused, because its conflict was on `19ab811d`, confirmed with `git log --diff-filter=D`. Without attribution the sweep prescribes claiming and resolving all 20. The branch behind #511 was a CRAN release branch this session did not own, so -the response was an explanatory comment naming the rename and where the content -went, not a push. +the response at the time was an explanatory comment naming the rename and where +the content went, not a push. +That comment is superseded: #511 was Copilot-authored and assigned to others, +so under `memories/reviewing-prs.md`'s scope test it gets no comment either, +and the current response is a report to the user. `git show --name-status "$merge"` was the first command reached for and printed no file list at all --- both merges here are two-parent merges, which is the case that behaves this way; re-measured against this corpus's own merge diff --git a/shared/workflow/batch-merge-and-resolve.md b/shared/workflow/batch-merge-and-resolve.md index 3ae6c12e6..958b625d6 100644 --- a/shared/workflow/batch-merge-and-resolve.md +++ b/shared/workflow/batch-merge-and-resolve.md @@ -131,20 +131,22 @@ So its empty answer and its broken answer look alike, per [`fail-fast`](../principles/fail-fast.md). Use the `git diff ^1 ` form above. -Then match the response to standing, not only to cause. -A conflict you caused on a branch you do not own is an explanatory comment -naming the deletion or rename and where the content went, rather than a push. -[`sync-with-main`](sync-with-main.md) does prescribe pushing the re-applied -change to a sibling branch, and that fits a CI workflow in a repo you drive; it -does not fit a release branch carrying an out-of-band process a push can -disrupt. +Then match the response to scope, not only to cause. +A conflict you caused on a PR that fails `memories/reviewing-prs.md`'s scope +test is a report to the user naming the deletion or rename and where the +content went, and the PR is left untouched: no comment, no push. +On a PR that passes the test, [`sync-with-main`](sync-with-main.md) does +prescribe pushing the re-applied change to the sibling branch, and that fits +a CI workflow in a repo you drive. +It does not fit a release branch carrying an out-of-band process a push can +disrupt, which gets the report instead however it scores on the test. - **Do:** derive the merge's own deleted and renamed paths, and intersect them with each conflict before claiming anything. - **Do:** report both counts --- conflicts found, and conflicts caused --- so the gap between them is visible rather than implied. -- **Do:** comment rather than push when a conflict you caused sits on a branch - you do not own. +- **Do:** report to the user rather than push or comment when a conflict you + caused sits on a PR that fails the scope test. - **Don't:** read a post-merge sweep's hit list as your work queue; on an old backlog most of it predates your merge. - **Don't:** derive that path set with `git show` --- it reports nothing for a diff --git a/shared/workflow/derive-dont-enumerate.md b/shared/workflow/derive-dont-enumerate.md index e05eb364b..7b7fada4b 100644 --- a/shared/workflow/derive-dont-enumerate.md +++ b/shared/workflow/derive-dont-enumerate.md @@ -236,8 +236,10 @@ Per [`algorithmatize-checks`](algorithmatize-checks.md), "which PRs are stalled" The script is also an instance of [`deterministic-tools`](../principles/deterministic-tools.md): judging coverage by eye is exactly the recurring judgment task that fragment says should become a tool. It is **read-only reporting, not authorization**. -[`ardi`](ardi.md) limits its monitoring mandate to PRs a session owns or has explicitly claimed, and a PR appearing in this sweep does not transfer ownership. -Surface an unowned stalled PR to the human, or claim it per [`claim-pr`](claim-pr.md) before driving it. +[`ardi`](ardi.md) limits its monitoring mandate to PRs that pass `memories/reviewing-prs.md`'s scope test (opened by or assigned to the invoking user, explicitly requested by name, or authored by the GitHub Actions app (`github-actions`)), +and a PR appearing in this sweep does not bring it into scope. +Surface an out-of-scope stalled PR to the human, who can assign it or name it; +a claim comment per [`claim-pr`](claim-pr.md) does not bring it into scope. [`pr-status-all`](../../skills/pr-status-all/SKILL.md) remains the richer per-PR dashboard. This is the cheap standing sweep that says where to point it. diff --git a/shared/workflow/sync-with-main.md b/shared/workflow/sync-with-main.md index afc35f369..c3d51f9fc 100644 --- a/shared/workflow/sync-with-main.md +++ b/shared/workflow/sync-with-main.md @@ -219,30 +219,39 @@ still edit that same inline block --- your merge just broke their textual diff, even though their intended change is usually trivial to re-apply to the new location.** This is the mirror image of the case above: there, you're the one resyncing after `main` moved a copy of your logic; here, -*you* are the one who moved the logic, so the burden of noticing and fixing -the resulting conflict falls on you, not on the sibling PR's author waiting -to hit it. Don't wait for that PR's own merge/CI to surface the conflict --- +*you* are the one who moved the logic, so the burden of noticing and +surfacing the resulting conflict falls on you, not on the sibling PR's author +waiting to hit it, and of fixing it where `memories/reviewing-prs.md`'s +scope test permits. +Don't wait for that PR's own merge/CI to surface the conflict --- check every open PR touching the same file right after your extraction merges: `git merge-tree "$(git merge-base origin/main origin/)" origin/main origin/` (or `gh pr diff ` against the new `main`) shows whether it still applies -cleanly. Re-apply the -sibling PR's actual semantic change (not a mechanical `--theirs`) to the new -location, verify with a direct diff that the extracted unit now differs from -`main` by exactly that PR's intended change and nothing else, then push to -their branch and flag what you did in a PR comment. +cleanly. +That check reads the sibling branch and edits nothing, so it runs for every +sibling PR. +Then apply `memories/reviewing-prs.md`'s scope test before preparing +anything: for a sibling PR that fails it, report the conflict to the user and +leave the PR untouched, since they can assign or name it first. +For one that passes, re-apply the sibling PR's actual semantic change (not a +mechanical `--theirs`) to the new location, verify with a direct diff that +the extracted unit now differs from `main` by exactly that PR's intended +change and nothing else, then push and flag what you did in a PR comment. See [`sync-with-main.cases.md`](sync-with-main.cases.md), "Check other open PRs after merging an extraction". -**That "push to their branch" is scoped by standing, not only by cause.** +**That "push to their branch" is scoped by the scope test, not only by cause.** gha#201/#202 were CI workflow files in a repo the author drove, where a push saves the sibling's author a round and risks nothing they were relying on. -The same push onto a branch you do not own --- a colleague's active work, and -most sharply a release branch carrying an out-of-band process --- can disrupt -something a comment would not. -There, name the extraction, the deletion, or the rename -and where the content went in a PR comment, -and leave the push to whoever owns the branch. +The same push onto a PR that fails the scope test --- a colleague's active +work you were neither assigned nor asked to drive --- and, whatever the test +says, onto a release branch carrying an out-of-band process, can disrupt +something a report would not. +There, report the extraction, the deletion, or the rename +and where the content went to the user, +who can assign or name the PR if they want it acted on, +and leave the branch untouched. Causing the conflict obliges you to *surface* it. It does not by itself license editing someone else's branch. See [`batch-merge-and-resolve`](batch-merge-and-resolve.md), diff --git a/skills/ardi/SKILL.md b/skills/ardi/SKILL.md index 0c1563bda..624aafd08 100644 --- a/skills/ardi/SKILL.md +++ b/skills/ardi/SKILL.md @@ -174,9 +174,12 @@ sits unread. **Opportunistic conflict sweep.** After pushing (or after any round where all findings were Rebutted/Deferred with no push), scan other open PRs in the same repo for merge conflicts: ```bash - gh pr list --state open --json number,title,headRefName,mergeable,mergeStateStatus,comments # LIST_PRS + gh pr list --state open --json number,title,headRefName,author,assignees,mergeable,mergeStateStatus,comments # LIST_PRS ``` - For each PR where `mergeable == "CONFLICTING"` **or `"UNKNOWN"`** (see `resolve-conflicts`, "Verify before you act" --- `UNKNOWN` can mean GitHub hasn't finished computing yet, not that there's no conflict), verify with `git merge-tree --write-tree origin/main origin/` (git ≥ 2.38) before acting, then check claim status (most recent comment) and fix unclaimed ones --- same cascade procedure as `post-merge` step 1.5 (claim → isolated worktree → fetch main → merge → `resolve-conflicts` skill → push → unclaim). + Filter that list by `memories/reviewing-prs.md`'s scope test first, as `ardia` step 1 does (opened by or assigned to the invoking user, explicitly requested by name, or authored by the GitHub Actions app (`github-actions`)); + an out-of-scope conflicting PR is reported to the user and left untouched (no comment, no push); + they can assign or name it if they want it resolved. + For each in-scope PR where `mergeable == "CONFLICTING"` **or `"UNKNOWN"`** (see `resolve-conflicts`, "Verify before you act" --- `UNKNOWN` can mean GitHub hasn't finished computing yet, not that there's no conflict), verify with `git merge-tree --write-tree origin/main origin/` (git ≥ 2.38) before acting, then check claim status (most recent comment) and fix unclaimed ones --- same cascade procedure as `post-merge` step 1.5 (claim → isolated worktree → fetch main → merge → `resolve-conflicts` skill → push → unclaim). A merge to `main` during your ARDI loop can create new conflicts in sibling PRs; clearing them while waiting for the next verdict is better than letting them pile up. 5. **Post the ARD summary** as a comment on the MR/PR (table format per the @@ -243,10 +246,12 @@ How depends on the repo's review trigger first, and on whether this round pushed Other PRs in this repo can become conflicting at any time (someone merges to `main` while the review runs). Poll every few minutes with `/loop` or a manual re-check: ```bash - gh pr list --state open --json number,title,headRefName,mergeable,mergeStateStatus,comments \ + gh pr list --state open --json number,title,headRefName,author,assignees,mergeable,mergeStateStatus,comments \ --jq '.[] | select(.mergeable == "CONFLICTING" or .mergeable == "UNKNOWN")' # LIST_PRS ``` - Verify each candidate with `git merge-tree --write-tree origin/main + Apply the same scope test as the sweep above before touching a candidate; + an out-of-scope one is reported to the user and left untouched. + Verify each in-scope candidate with `git merge-tree --write-tree origin/main origin/` (git ≥ 2.38; see `resolve-conflicts`, "Verify before you act") before claiming --- `UNKNOWN` isn't proof of a real conflict, and `CONFLICTING` can be stale if a sibling PR merged since GitHub last computed it. Claim and diff --git a/skills/ardia/SKILL.md b/skills/ardia/SKILL.md index db8ddda26..864b9c0c9 100644 --- a/skills/ardia/SKILL.md +++ b/skills/ardia/SKILL.md @@ -1,6 +1,6 @@ --- name: ardia -description: "Drive all open PRs to clean." +description: "Drive every in-scope open PR to clean." user-invocable: true allowed-tools: - Bash @@ -12,8 +12,9 @@ allowed-tools: # ARDIA — ARD + Iterate-All -Apply the ARDI loop (ARD + iterate) to every open PR/MR in the repo, driving -each to a clean review verdict in series. +Apply the ARDI loop (ARD + iterate) to every in-scope open PR/MR in the repo +(step 1's scope test decides which), driving each to a clean review verdict in +series. Triage and local patch preparation may run in parallel first; every action that mutates a PR stays serial. @@ -22,13 +23,36 @@ mutates a PR stays serial. 1. **List the open PRs/MRs and decide which are in scope.** ```bash gh pr list --state open --limit 100 \ - --json number,title,headRefName,baseRefName,isDraft,author,reviewDecision # LIST_PRS + --json number,title,headRefName,baseRefName,isDraft,author,assignees,reviewDecision # LIST_PRS ``` On GitLab, use `glab api "projects/:id/merge_requests?state=opened&per_page=100"` and look for `source_branch` (≡ `headRefName`) and `target_branch` (≡ `baseRefName`) in the JSON — `glab mr list` alone does not expose these fields. State the scope rules when you report, so the user can correct: + - **Only PRs opened by the user, assigned to the user, explicitly requested by name, or authored by the Actions app are in scope.** + Resolve the invoking user first + --- `gh api user --jq .login` locally, `mcp__github__get_me` in a remote session, `glab api user | jq -r .username` on GitLab, where the fields are `author.username` and `assignees[].username` --- + and add any aliases `memories/reviewing-prs.md` lists for that person, + so a lab member running this corpus from a vendored checkout filters on their own identity rather than the corpus owner's. + When no identity operation is available, fail closed: leave the author and assignee arms unevaluated, keep only explicitly requested and Actions-app-authored PRs, and say so in the report. + Normalise the fields first: the author is `author.login` in `gh --json`, `user.login` in REST and `mcp__github__list_pull_requests`, and `author.username` on GitLab; + assignees are `assignees[].login` in `gh --json` and REST, bare login strings in `assignees` from `mcp__github__list_pull_requests`, or `assignees[].username` on GitLab. + The MCP list tool omits the `assignees` key on an unassigned PR (measured 2026-09-01). + Keep a PR only when its author is one of those logins or the repository's own workflow bot (the app slug `github-actions`, in whichever form the source returns it; + `memories/reviewing-prs.md` lists the forms), + one of those logins is among its `assignees`, + or the user explicitly asked for work on that PR (or, via an explicit `chores` call, on the bot population) --- a mention such as "do not touch [#284](https://github.com/UCD-SERG/serodynamics/pull/284)" is not a request; + exclude every other PR from the action queue before doing anything else, and name the excluded ones in the report so the user can assign or name any they want driven. + An explicit exclusion in the request ("ardia, but do not touch" followed by a number) is a veto checked before every positive arm, the way `chores`'s `PR_SCOPE_EXCLUDED` works: + an excluded PR is dropped even when the user authored it, is assigned to it, or the Actions app opened it, and the exclusion list travels with the sweep into every recheck below and into any brief handed to a subagent. + Scope is a live precondition rather than a one-time classification: assignment can change while a sweep runs, so re-read the author and assignees and reapply the test immediately before each push, comment, review, or merge on a PR, and drop and report one that no longer passes. + Keep the full listing for the stack detection in step 3, so an in-scope PR based on an out-of-scope PR's branch is still recognised as stacked. + Such a child is driven only while its base is clean or merged. + An open, unclean out-of-scope base is left to its author, and the child is parked and reported with it, per the stacked-PR rule in step 3. + "Every open PR" anywhere in this skill means every PR that survives this filter. + An out-of-scope PR (one that fails this test: another lab member's or another bot's, such as Dependabot's or a Copilot agent's, that is neither assigned to the user nor explicitly requested) is not driven, reviewed, or edited, however clean it looks (see `memories/reviewing-prs.md`, "Only work PRs I opened, am assigned to, or asked for by name, or the Actions app authored"; + measured on `UCD-SERG/serodynamics` 2026-09-01, where the sweep drove four other authors' PRs before the correction arrived). - **Include drafts** (`isDraft: true`) unless another agent is actively driving one. A draft is the corpus's own in-flight claim signal --- [`pr-on-claim`](../../shared/workflow/pr-on-claim.md) opens one from an empty `start:` scaffold commit before any code exists --- @@ -48,9 +72,6 @@ mutates a PR stays serial. mark it ready for review --- a clean verdict is unreachable while it stays draft. Name each draft's disposition, and the signal that decided it, in the scope report, so the user can veto before the loop touches it. - - **Only iterate PRs the user owns / is responsible for** by default. In a - shared repo, don't start review loops (which push commits) on other - people's PRs unless told to. If unsure who owns what, ask first. - **A green PR with no review check run is parked, not finished.** On a repo whose review workflow is `workflow_dispatch`-only, nothing fires on push, so a PR nobody ever reviewed presents exactly like one @@ -178,6 +199,9 @@ mutates a PR stays serial. review findings), complete ARDI on the base first to drive it to clean and merge it, then start the derived PR. Never run ARDI on a derived PR while its base is still open and unclean — you'd be reviewing against a moving target. + When the base fails the scope test in step 1, do not run ARDI on it either: + park the derived PR, report the pair so the user can assign or name the + base, and move to the next PR. A PR reaching **clean-but-unmerged** is that PR's terminal state, **not** a reason to pause the sweep: merging is human-gated (you don't self-merge), but diff --git a/skills/ardiaei/SKILL.md b/skills/ardiaei/SKILL.md index 53d9f5521..4917414b3 100644 --- a/skills/ardiaei/SKILL.md +++ b/skills/ardiaei/SKILL.md @@ -16,7 +16,7 @@ Clear the open-PR review queue **and** capture what doing so taught you, by composing two existing skills in sequence: 1. **Phase 1 — `ardia`** ([ardia](../ardia/SKILL.md), ARD + - Iterate-All): drive every open PR/MR to a clean review verdict, in series. + Iterate-All): drive every open PR/MR (every one that passes `ardia`'s scope filter) to a clean review verdict, in series. 2. **Phase 2 — `ums`** ([ums](../ums/SKILL.md), Update Memories and Skills): review what the ARDIA loop surfaced — recurring review findings, CI quirks, tool gotchas, workflow gaps — and persist it by editing memory files diff --git a/skills/cascade/SKILL.md b/skills/cascade/SKILL.md index f01283fc4..ab4efb7f9 100644 --- a/skills/cascade/SKILL.md +++ b/skills/cascade/SKILL.md @@ -26,8 +26,16 @@ this skill exists to pin the meaning.) 1. **Map the stack.** List open PRs and their base branches - (`gh pr list --json number,headRefName,baseRefName`, + (`gh pr list --json number,headRefName,baseRefName,author,assignees`, or `mcp__github__list_pull_requests` in remote sessions). + Keep the full list for mapping the stack, then exclude from the action + queue every PR that fails `memories/reviewing-prs.md`'s scope test, as + `ardia` step 1 does (opened by or assigned to the invoking user, + explicitly requested by name, or authored by the GitHub Actions app (`github-actions`)), and + report the PRs excluded: every later step pushes to the branch, which + that memory forbids on any other PR. + A stacked PR whose base PR is out of scope is synced against that base + as it stands; the base itself is left to its author. A PR based on `main` is unstacked; a PR based on another PR's head branch is stacked on it. Order the work from the base outward: diff --git a/skills/check-history/SKILL.md b/skills/check-history/SKILL.md index a783c4a1e..f0944fe25 100644 --- a/skills/check-history/SKILL.md +++ b/skills/check-history/SKILL.md @@ -36,8 +36,8 @@ checks (skipping them wastes a whole issue-pick): issue, so you don't open a second, parallel PR for the same work. ```bash - gh pr list --state open --json number,title,headRefName,body \ - --jq '.[] | select(((.body // "") | test("#\\b")) or (.title | test("#\\b"))) | "#\(.number) \(.title) [\(.headRefName)]"' # LIST_PRS + gh pr list --state open --json number,title,headRefName,body,author,assignees \ + --jq '.[] | select(((.body // "") | test("#\\b")) or (.title | test("#\\b"))) | "#\(.number) \(.title) [\(.headRefName); \(.author.login); assignees: \([.assignees[].login] | join(","))]"' # LIST_PRS ``` - **On a long-lived or foundational issue, the issue text AND any design-doc @@ -52,8 +52,18 @@ checks (skipping them wastes a whole issue-pick): sibling PRs — nearly rebuilt already-completed work. The right move was to audit, correct the stale issue/doc status, and pick the genuine next slice.) -If an open PR already covers it, **drive that PR to clean** instead of -re-implementing (ask before pushing to a branch you didn't create). If `main` +If an open PR already covers it and passes `memories/reviewing-prs.md`'s +scope test (opened by or assigned to the invoking user, explicitly requested, +or authored by the GitHub Actions app), **drive that PR to clean** instead of +re-implementing. +Scope establishes authorization, not that the branch is free: read the PR's +claim state first per [`claim-pr`](../../shared/workflow/claim-pr.md), skip it +while another session's claim is live (a `hold off` comment with a push or +comment in the last 2 hours), and take over an unclaimed or expired one by +posting your own claim comment before the first push. +If it covers the issue but fails that test, leave both the PR and the issue +alone and report that the PR exists, so the user can assign or name it. +If `main` already satisfies the issue, stand it down and report — don't open a no-op PR. When the issue is only *partly* done, don't rebuild the done part: audit it, correct the stale issue/doc status, and scope only the genuine remaining slice. @@ -67,18 +77,24 @@ correct the stale issue/doc status, and scope only the genuine remaining slice. **GitHub:** ```bash - gh pr list --state open --limit 100 --json number,title,body \ - --jq '.[] | select((.body // "") | test("(Closes|Fixes|Resolves) #\\b"; "i")) | "#\(.number) \(.title)"' # LIST_PRS + gh pr list --state open --limit 100 --json number,title,body,author,assignees \ + --jq '.[] | select((.body // "") | test("(Closes|Fixes|Resolves) #\\b"; "i")) | "#\(.number) \(.title) [\(.author.login); assignees: \([.assignees[].login] | join(","))]"' # LIST_PRS ``` **GitLab:** ```bash glab mr list --state opened --per-page=50 --output json 2>/dev/null \ - | jq -r '.[] | select((.description // "") | test("(Closes|Fixes|Resolves) #\\b"; "i")) | "!\(.iid) \(.title)"' + | jq -r '.[] | select((.description // "") | test("(Closes|Fixes|Resolves) #\\b"; "i")) | "!\(.iid) \(.title) [\(.author.username); assignees: \([.assignees[].username] | join(","))]"' ``` - If an open PR already covers the issue, **review or extend it** instead of - opening a competing one. This catches *in-flight* work; the merged/closed + If an open PR already covers the issue, apply the scope test above to the + author and assignee logins the listing prints: + **review or extend it** when it passes and the same claim check clears + (another session's live claim still blocks you, and an expired one is taken + over by posting your own claim comment first), and leave it untouched + (reporting it to the user) when it fails. + Either way, do not open a competing one. + This catches *in-flight* work; the merged/closed history below catches *settled* decisions. 1. **List recent merged MRs** touching the same area: diff --git a/skills/chores/SKILL.md b/skills/chores/SKILL.md index ecb1b2432..c94d76c4f 100644 --- a/skills/chores/SKILL.md +++ b/skills/chores/SKILL.md @@ -29,16 +29,27 @@ for the user's call before merging. ## What counts as a chore PR -A PR is in scope if **any** of these hold: - -- Author is a bot: `app/dependabot`, `dependabot[bot]`, `app/renovate`, - `renovate[bot]`. -- Title is a conventional-commit chore: starts with `chore(` (e.g. - `chore(actions):`, `chore(submodule):`, `chore(deps):`). -- Labels include `dependencies`. - -Human-authored feature PRs are **out of scope** — those go through `ardia` / -`gia` (review-to-clean), not this skill. +A PR is in scope when **either** of these holds: + +- Its author is one of the dependency bots this skill exists for, matched in + the exact login form the source returns: `app/dependabot`, + `dependabot[bot]`, `app/renovate`, `renovate[bot]`. + An explicit `chores` call names that population, which is what admits those + two bots and no other author. +- It looks like a chore --- the title starts with `chore(` (e.g. + `chore(actions):`, `chore(submodule):`, `chore(deps):`), or the labels + include `dependencies` --- **and** it passes `memories/reviewing-prs.md`'s + scope test for the invoking user: authored by the GitHub Actions app + (`github-actions`, which opens `chore(submodule):` bumps) or by the invoking + user or one of their aliases, assigned to one of them, or one the user + explicitly asked this run to work on (a mention such as "do not touch" + followed by a number is not a request). + +Human-authored feature PRs are **out of scope** --- those go through `ardia` / +`gia` (review-to-clean), not this skill --- and so is a chore-titled or +`dependencies`-labelled PR whose author is another lab member or another bot, +unless the invoking user is assigned to it or explicitly asked this run to +work on it. ## Procedure @@ -57,13 +68,57 @@ This skill is GitHub-first (`gh`). For a GitLab repo, the same shape applies via ### 1. List the open chore PRs +Set the three scope inputs first, the way `REPO` is set above. +`PR_SCOPE_ALIASES` is the comma-separated list of other logins +`memories/reviewing-prs.md` names as the same person as the resolved user +(leave it unset when that file lists none for them), and `PR_SCOPE_REQUESTED` +is the comma-separated list of PR numbers the user explicitly asked this run +to work on, never a number merely mentioned or excluded +(leave it unset when there are none). +`PR_SCOPE_EXCLUDED` is the comma-separated list of PR numbers the user told +this run not to touch ("chores, but do not touch" followed by a number); it +is a veto checked before every positive arm, bot authors included, so an +excluded dependency-bot PR is neither listed nor merged. +With all three unset the filter keeps only the resolved login's own PRs, the +assigned ones, and the bots', which is the fail-closed default. + ```bash +set -eo pipefail # a failed command, or a failed gh pr list in the pipeline below, stops here +ME=$(gh api user --jq .login 2>/dev/null) || ME="" # WHO_AM_I +if [ -z "$ME" ]; then + # Fail closed, and say so: with no identity the author and assignee arms + # stay unevaluated (aliases included), so only bot-authored and explicitly + # requested PRs pass. + PR_SCOPE_ALIASES="" + echo "::warning::identity lookup failed; author/assignee arms unevaluated (report this)" >&2 +fi +# e.g. PR_SCOPE_ALIASES=other-login # from memories/reviewing-prs.md +# e.g. PR_SCOPE_REQUESTED=123,456 # PRs the user asked this run to work on +# e.g. PR_SCOPE_EXCLUDED=789 # PRs the user told this run not to touch +IDS=$(jq -cn --arg me "$ME" --arg al "${PR_SCOPE_ALIASES:-}" \ + '[$me] + ($al | split(",") | map(select(length > 0))) | map(select(length > 0)) | unique') +REQ=$(jq -cn --arg r "${PR_SCOPE_REQUESTED:-}" \ + '$r | split(",") | map(select(length > 0) | tonumber)') +EXC=$(jq -cn --arg x "${PR_SCOPE_EXCLUDED:-}" \ + '$x | split(",") | map(select(length > 0) | tonumber)') gh pr list --repo "$REPO" --state open --limit 200 \ - --json number,title,author,labels,mergeable \ - --jq '.[] | select( - (.author.login | test("dependabot|renovate")) - or (.title | startswith("chore(")) - or ([.labels[].name] | index("dependencies")) + --json number,title,author,assignees,labels,mergeable \ + | jq -r --argjson ids "$IDS" --argjson req "$REQ" --argjson exc "$EXC" '.[] | select( + ((.number as $n | $exc | index($n)) == null) + and ( + (.author.login | test("^(app/(dependabot|renovate)|(dependabot|renovate)\\[bot\\])$")) + or ( + ( + (.author.login | test("^(app/github-actions|github-actions\\[bot\\]|github-actions)$")) + or ((.author.login as $a | $ids | index($a)) != null) + or any(.assignees[].login; . as $x | ($ids | index($x)) != null) + or ((.number as $n | $req | index($n)) != null) + ) and ( + (.title | startswith("chore(")) + or (([.labels[].name] | index("dependencies")) != null) + ) + ) + ) ) | "\(.number)\t\(.mergeable)\t\(.title)"' # LIST_PRS ``` @@ -72,6 +127,14 @@ would otherwise be silently truncated. If there are none, say so and stop. +That listing is a snapshot. +Assignment, the title, and the labels can all change while the sweep runs, +so re-fetch every input the predicate reads (author, assignees, title, +labels) and reapply the same predicate, the `PR_SCOPE_EXCLUDED` veto +included, immediately before each write action in steps 2-5 (closing a bump +PR, a `@dependabot` comment, a merge), and drop and report a PR that no longer +passes. + ### 2. Classify each PR by bump size Parse the version pair out of the title (`... from X to Y`) and compare the @@ -222,5 +285,7 @@ bump is sitting unflagged. - ❌ Force-merging a PR with `pending` or `fail` checks. - ❌ Reporting "chores done" while a flagged major bump is still open with no decision recorded. -- ❌ Treating human feature PRs as chores (or vice-versa) — scope by author / - `chore(` title / `dependencies` label. +- ❌ Treating human feature PRs as chores (or vice-versa) --- a dependency + bot's PR is a chore by author; any other PR needs the `chore(` title or + `dependencies` label **and** an in-scope author or assignee, never the + title or label alone. diff --git a/skills/clear-all/SKILL.md b/skills/clear-all/SKILL.md index 351e512ac..f3689ebd7 100644 --- a/skills/clear-all/SKILL.md +++ b/skills/clear-all/SKILL.md @@ -12,8 +12,8 @@ allowed-tools: # clear-all (alias for `gia`) This is an alias for the **gia** (Grab Issues + iterate-All) skill, which clears -the repo's entire work queue end to end: drive every open PR/MR to a clean -review verdict with green CI, and open a PR for every open issue that lacks one +the repo's entire work queue end to end: drive every in-scope open PR/MR (per +`ardia` step 1's scope test) to a clean review verdict with green CI, and open a PR for every open issue that lacks one (each new PR is itself driven to clean). Read and follow the canonical skill: @@ -21,5 +21,5 @@ Read and follow the canonical skill: > `gia` runs PRs-first (ARDIA), then issues (GII), even though "clear-all" > describes the issues half first — the end state is identical (every issue has -> a PR, every PR is clean and green), and clearing existing PRs first can close +> a PR, every in-scope PR is clean and green), and clearing existing PRs first can close > issues a pending PR already resolves. diff --git a/skills/gi/SKILL.md b/skills/gi/SKILL.md index a6afd2664..75b435828 100644 --- a/skills/gi/SKILL.md +++ b/skills/gi/SKILL.md @@ -134,22 +134,29 @@ An expired claim is taken over by posting your own claim comment, never silently ```bash # GitHub — list open PRs and scan for any whose title or branch references this issue: -gh pr list --state open --json number,title,headRefName | cat # LIST_PRS +gh pr list --state open --json number,title,headRefName,author,assignees \ + --jq '.[] | "#\(.number) \(.title) [\(.headRefName); \(.author.login); assignees: \([.assignees[].login] | join(","))]"' # LIST_PRS # Authoritative — the issue's cross-referenced open PRs via the REST timeline API. # (gh issue view --json has no timelineItems field; in the timeline, source.type is # always "issue", so a PR is one whose source.issue.pull_request is non-null. The # state filter keeps only open PRs — merged/closed siblings aren't active competitors. # --paginate walks every page so a cross-reference past the first 30 events isn't missed.) gh api --paginate repos///issues//timeline \ - --jq '.[] | select(.event == "cross-referenced") | .source.issue | select(.pull_request != null) | select(.state == "open") | "#\(.number) \(.title)"' | cat # ISSUE_LINKED_PRS + --jq '.[] | select(.event == "cross-referenced") | .source.issue | select(.pull_request != null) | select(.state == "open") | "#\(.number) \(.title) [\(.user.login); assignees: \([.assignees[].login] | join(","))]"' | cat # ISSUE_LINKED_PRS ``` If an open PR already exists for the issue: - **Don't open a competing PR.** The issue is already being worked. - Skip it and grab the next unblocked issue instead. -- Or, if the existing PR is stalled/abandoned and you're taking it over, +- Or, if the existing PR is stalled/abandoned and it passes + `memories/reviewing-prs.md`'s scope test (opened by or assigned to the + invoking user, explicitly requested, or authored by the GitHub Actions app; + the listing above prints the author and assignee logins), take it over: check it out (use the existing PR branch), claim the PR, and ARDI it rather than starting fresh. +- A stalled PR that fails that test is not yours to take over: skip the + issue, leave the PR untouched, and report it so the user can assign or + name it. ### 5. Check history, peers, and research DRW diff --git a/skills/gia/SKILL.md b/skills/gia/SKILL.md index 94a9eb5f2..1bd2353b0 100644 --- a/skills/gia/SKILL.md +++ b/skills/gia/SKILL.md @@ -51,6 +51,11 @@ several repos (e.g. a web session scoped to multiple repos). If the working dir isn't itself a single repo, or more than one repo is in scope, ask which repo's queue to clear before surveying --- don't assume the first one found. +**Confirm whose PRs are in scope, too.** +Both phases act only on PRs opened by the invoking user, assigned to that user, explicitly requested by name, or authored by the GitHub Actions app (app slug `github-actions`); +`ardia`'s step 1 resolves that user, applies the filter, and reports what it dropped. +An out-of-scope PR (one that fails that filter: another lab member's or another bot's that is neither assigned to the user nor explicitly requested) stays theirs, and an issue such a PR already fixes is left to it rather than grabbed. + ### Phase 1 — ARDIA (existing open PRs/MRs) Run the full [`ardia`](../ardia/SKILL.md) procedure: list every open PR/MR and @@ -162,3 +167,4 @@ and [`delegate-to-codex`](../delegate-to-codex/SKILL.md). - ❌ Running Phase 2 unbounded — keep GII's wave boundary. - ❌ Starting the next wave on your own once the current one is fully finished — stop and ask, with a recommendation, per "Stopping conditions". - ❌ Grabbing an issue a pending Phase-1 PR already closes. +- ❌ Driving, reviewing, or editing a PR that was not opened by the user, not assigned to the user, not explicitly requested by name, and not authored by the Actions app --- "every open PR" means every PR that passes the filter. diff --git a/skills/giardia/SKILL.md b/skills/giardia/SKILL.md index cb8f17081..8ad53faad 100644 --- a/skills/giardia/SKILL.md +++ b/skills/giardia/SKILL.md @@ -12,8 +12,8 @@ allowed-tools: # giardia (alias for `gia`) This is a mnemonic alias for the **gia** (Grab Issues + iterate-All) skill, -which clears the repo's entire work queue end to end: drive every open PR/MR to -a clean review verdict with green CI, and open a PR for every open issue that +which clears the repo's entire work queue end to end: drive every in-scope open +PR/MR (per `ardia` step 1's scope test) to a clean review verdict with green CI, and open a PR for every open issue that lacks one (each new PR is itself driven to clean). Read and follow the canonical skill: diff --git a/skills/handoff/SKILL.md b/skills/handoff/SKILL.md index 80f2cd61f..56dfcdd65 100644 --- a/skills/handoff/SKILL.md +++ b/skills/handoff/SKILL.md @@ -77,6 +77,9 @@ the existing one). If the work has an open PR/MR and you've **claimed** it (see `claim-pr`), post a short note so the `@claude` bot and other sessions don't push conflicting changes — especially when you have unpushed local commits or running jobs. +Post it only when that PR still passes `memories/reviewing-prs.md`'s scope +test: a claim confers no scope, so if the PR has since fallen out of scope, +skip the note and report the PR to the user instead. ```bash gh pr comment --body "⏸️ **Local session paused** () — still claimed. diff --git a/skills/iterate-all/SKILL.md b/skills/iterate-all/SKILL.md index b50c6cf5d..35d5638c7 100644 --- a/skills/iterate-all/SKILL.md +++ b/skills/iterate-all/SKILL.md @@ -7,7 +7,7 @@ user-invocable: true # iterate-all (alias for `ardia`) This is a synonym alias for the **ardia** skill (ARD + Iterate-All) — the same -"run the single-PR loop over every open PR in series, then report a table" +"run the single-PR loop over every in-scope open PR in series, then report a table" orchestrator. Read and follow the canonical skill: → **[ardia](../ardia/SKILL.md)** diff --git a/skills/mma/SKILL.md b/skills/mma/SKILL.md index 04c4579cd..4b02eb32c 100644 --- a/skills/mma/SKILL.md +++ b/skills/mma/SKILL.md @@ -13,7 +13,7 @@ allowed-tools: # mma Repo-wide fan-out of [`sync-pr-branch`](../sync-pr-branch/SKILL.md): instead -of resyncing one branch, resync **every currently-open PR** in the repo +of resyncing one branch, resync **every currently-open PR** in the repo that passes `memories/reviewing-prs.md`'s scope test against `main` (and each PR's own remote), so a burst of merges into `main` doesn't leave the rest of the queue stale and conflict-prone. @@ -35,7 +35,13 @@ fix for free. ## The procedure 1. **List every open PR in the repo** (`mcp__github__list_pull_requests` / - `gh pr list`). Note each PR's number and `headRefName`. + `gh pr list --json number,headRefName,author,assignees`). + Note each PR's number, `headRefName`, `author`, and `assignees`. + Filter the list by `memories/reviewing-prs.md`'s scope test first, as + `ardia` step 1 does (opened by or assigned to the invoking user, + explicitly requested by name, or authored by the GitHub Actions app (`github-actions`)), and report the PRs + dropped: a resync pushes a merge commit, which that memory forbids on any + other PR. 2. **Check whether main is actually ahead** before touching anything: ```bash diff --git a/skills/mwc/SKILL.md b/skills/mwc/SKILL.md index 212a442aa..8b66b9ef6 100644 --- a/skills/mwc/SKILL.md +++ b/skills/mwc/SKILL.md @@ -62,6 +62,10 @@ It is still the wrong default, because a peer may have further commits planned, **A peer's PR may be merged once it has been ready to merge for more than twenty minutes** (maintainer directive, 2026-08-27). +That peer's PR must itself pass `memories/reviewing-prs.md`'s scope test first: opened by or assigned to the invoking user, explicitly requested, or authored by the GitHub Actions app. +A peer session running under your own login satisfies the author arm, which is the case this section was written for. +Another lab member's PR that fails the test does not become yours by going quiet: it gets neither the warning comment nor the merge, and is reported to the user instead. + The interval is what does the work here, and it is doing something specific rather than being a polite pause. A session actively driving a PR pushes again within a few minutes of a clean verdict, so twenty minutes of quiet distinguishes *waiting on a human* from *mid-round* --- which is the only thing you actually need to know, and the one thing you cannot ask the peer for reliably. diff --git a/skills/post-merge/SKILL.md b/skills/post-merge/SKILL.md index 7e90ac20e..7ca92079c 100644 --- a/skills/post-merge/SKILL.md +++ b/skills/post-merge/SKILL.md @@ -227,20 +227,35 @@ finding's disposition.) subagent** rather than running the scan-and-resolve loop in the main thread --- it's exactly the kind of investigation-plus-fix work the coordinator should hand off (see `memories/preferences.md`'s coordinator-mode bullet). -Brief the subagent with the merged PR's number/branch and the steps below; +Brief the subagent with the merged PR's number/branch, the resolved invoking +user and their aliases, the exact list of PR numbers the request explicitly +authorized (possibly empty), the exact list it explicitly excluded (possibly +empty; a veto over every positive arm, the user's own PRs included), and the +steps below; have it report back which PRs it found conflicting, what it did about each, and any it skipped (already claimed, conflict it couldn't understand). Do the scan inline only for a solo (non-orchestrated) session. -**If any OTHER agents already own a claimed branch (an active, resumable -`Agent`-tool session, not a one-shot `Workflow`-internal `agent()` call), -message each one directly right after the merge, instead of relying solely on +**If any OTHER agents of this session already own a claimed branch (an active, +resumable `Agent`-tool session, not a one-shot `Workflow`-internal `agent()` +call), message each one directly right after the merge, instead of relying solely on a separate scan to find and fix their conflict after the fact:** "main just advanced (PR #N merged) --- fetch and merge origin/main into your branch now, resolve any conflict yourself (you have the context on your own change), then continue." This is faster and higher-context than a scanning subagent guessing at the resolution from outside: the branch's own owning agent already knows why its code looks the way it does. +Those branches are usually this session's own work, but re-check each one +against `memories/reviewing-prs.md`'s scope test before sending the +instruction: an assignment can change, and a delegated agent may be driving +an assigned or requested PR that another author opened. +A branch that fails the test, or that is claimed by an agent that is not +yours, is handled like any other open PR: the scope filter decides, and an +out-of-scope one is reported to the user and left untouched. +When the re-check finds one of this session's own agents on a branch that +now fails the test, tell that agent to stop before reporting the PR; +withholding the merge-main message alone leaves it free to keep polling and +pushing. **This depends on the coordinator finding out about a merge in the first place --- so brief every delegated agent, up front, to report back the @@ -264,10 +279,17 @@ Scan right after the merge is confirmed: ```bash gh pr list --state open \ - --json number,title,headRefName,mergeable,mergeStateStatus,comments # LIST_PRS + --json number,title,headRefName,author,assignees,mergeable,mergeStateStatus,comments # LIST_PRS ``` -For each PR where `mergeable == "CONFLICTING"` **or `"UNKNOWN"`** (GitHub can +Filter that list by `memories/reviewing-prs.md`'s scope test first, as +`ardia` step 1 does (opened by or assigned to the invoking user, on the +explicitly authorized list from the brief, or authored by the GitHub Actions +app (`github-actions`)), and report the PRs dropped. +A conflict on an out-of-scope PR is reported to the user and the PR left +untouched (no comment, no push); a claim comment does not bring it into scope. + +For each in-scope PR where `mergeable == "CONFLICTING"` **or `"UNKNOWN"`** (GitHub can take minutes to finish computing mergeability after a push — a genuinely conflicting PR can sit in `UNKNOWN` and get missed if you filter for `CONFLICTING` alone): @@ -332,7 +354,8 @@ conflicting PR can sit in `UNKNOWN` and get missed if you filter for If a live claim stands --- a push or comment within the last 2 hours --- skip the PR. Another session owns it. An expired claim (over 2 idle hours) no longer blocks. - Take over with a fresh claim comment of your own, per [`claim-pr`](../../shared/workflow/claim-pr.md)'s expiration rule. + Take over with a fresh claim comment of your own, per [`claim-pr`](../../shared/workflow/claim-pr.md)'s expiration rule; + that claim guards against session collisions on a PR already in scope, and never reads an out-of-scope PR into scope. 4. **Claim it.** ```bash gh pr comment --body "Working on this — please hold off on pushing to this branch until I'm done. @@ -378,16 +401,17 @@ Resolve PRs one at a time — not because worktrees race each other (each worktr One-at-a-time keeps the blast radius small. Skip any PR whose conflict is in a file you can't understand without more context — comment asking for clarification instead. -**Match the response to standing, not only to cause.** -Step 2 says whether a conflict is yours; it does not say the branch is. -A conflict you genuinely caused, on a branch you do not own ---- a colleague's in-flight work, -and most sharply a release branch carrying an out-of-band process --- -is an explanatory comment naming the deletion or rename -and where the content went, rather than a push to their branch. -`sync-with-main` does prescribe re-applying the change on the sibling branch -and pushing it, and that fits a workflow or CI file in a repo you drive. -It is not the default for someone else's release branch. +**Match the response to scope, not only to cause.** +Step 2 says whether a conflict is yours; the scope filter in the paragraph +above step 1 says whether the PR is. +A conflict you genuinely caused on a PR that fails that test is a report to +the user naming the deletion or rename and where the content went; the PR +gets no comment and no push. +On an in-scope PR, `sync-with-main` does prescribe re-applying the change on +the sibling branch and pushing it, and that fits a workflow or CI file in a +repo you drive. +A release branch carrying an out-of-band process gets the report instead, +whatever the test says. ### 2. Tidy the local branch @@ -976,5 +1000,6 @@ When this post-merge wrap-up completes the session's work **and no PR this sessi - ❌ Treating the whole cascade-scan hit list as work caused by this merge, without intersecting it against the merge's own deleted and renamed paths (step 1.5's own step 2) --- on an old backlog that claims other people's stale PRs for no reason. -- ❌ Pushing a resolution to a branch you don't own when a comment would do --- - sharpest on a release branch, where a push can disrupt an out-of-band process. +- ❌ Pushing to, or commenting on, a PR that fails the scope test --- it gets a + report to the user --- or pushing to a release branch carrying an out-of-band + process even when it passes, where a report is the safe form. diff --git a/skills/wrap-up/SKILL.md b/skills/wrap-up/SKILL.md index f1024fd2a..bc3bae75a 100644 --- a/skills/wrap-up/SKILL.md +++ b/skills/wrap-up/SKILL.md @@ -37,8 +37,8 @@ your model's tool via [`tool-mappings.md`](../../tool-mappings.md) instead of the `gh` command shown if this session doesn't have `gh`: ```bash -gh pr list --state open --json number,title,headRefName,author,mergeable,mergeStateStatus,comments \ - --jq '.[] | "#\(.number) [\(.author.login)] \(.title) [\(.mergeable)]"' # LIST_PRS +gh pr list --state open --json number,title,headRefName,author,assignees,mergeable,mergeStateStatus,comments \ + --jq '.[] | "#\(.number) \(.headRefName) [\(.author.login); assignees: \([.assignees[].login] | join(","))] \(.title) [\(.mergeable)]"' # LIST_PRS gh issue list --state open --json number,title --jq '.[] | "#\(.number) \(.title)"' # LIST_ISSUES git status --short # uncommitted work? git worktree list # leftover worktrees (agent isolation / session-lock)? @@ -51,10 +51,14 @@ git log --oneline -5 origin/main # what actually landed on main by the user, and vice-versa. - If the session touched **other repos** (e.g. an upstream dependency), check those too — `gh pr list --repo / --state open - --json number,title,headRefName,author,mergeable,mergeStateStatus,comments` + --json number,title,headRefName,author,assignees,mergeable,mergeStateStatus,comments` (`LIST_PRS`). - **Merge conflict sweep.** Before closing out, check every open PR's - `mergeable` field. For each PR with `mergeable == "CONFLICTING"` **or + `mergeable` field. + Filter the list by `memories/reviewing-prs.md`'s scope test first, as + `ardia` step 1 does; report an out-of-scope conflicting PR to the user and + leave it untouched (no comment, no push). + For each in-scope PR with `mergeable == "CONFLICTING"` **or `"UNKNOWN"`** (see `resolve-conflicts`, "Verify before you act" — `UNKNOWN` can mean GitHub hasn't finished computing yet), verify with `git merge-tree --write-tree origin/main origin/` (git ≥ 2.38) before acting, diff --git a/tool-mappings.md b/tool-mappings.md index e10286511..e7870384a 100644 --- a/tool-mappings.md +++ b/tool-mappings.md @@ -49,6 +49,7 @@ operation to the equivalent GitHub MCP tool so any model can run a skill. | --- | --- | --- | --- | | `VIEW_PR` | Read a pull request's details and metadata. | `gh pr view ""` | `mcp__github__pull_request_read (method=get)` | | `LIST_PRS` | List pull requests. | `gh pr list` | `mcp__github__list_pull_requests` | +| `WHO_AM_I` | Resolve the invoking user's login, for the PR-scope filter in `memories/reviewing-prs.md`. | `gh api user --jq .login` | `mcp__github__get_me` | | `SEARCH_PRS` | Search pull requests by keyword / query string. | `gh pr list --search ""` | `mcp__github__search_pull_requests` | | `DIFF_PR` | Read a pull request's diff. | `gh pr diff ""` | `mcp__github__pull_request_read (method=get_diff)` | | `PR_CHECKS` | Read a pull request's CI check / status results. | `gh pr checks ""` | `mcp__github__pull_request_read (method=get_check_runs)` | diff --git a/tool-mappings.yml b/tool-mappings.yml index 4ff1dd73c..0d19a40da 100644 --- a/tool-mappings.yml +++ b/tool-mappings.yml @@ -59,6 +59,10 @@ operations: description: List pull requests. cli: gh pr list github_mcp: mcp__github__list_pull_requests + - id: WHO_AM_I + description: Resolve the invoking user's login, for the PR-scope filter in `memories/reviewing-prs.md`. + cli: gh api user --jq .login + github_mcp: mcp__github__get_me - id: SEARCH_PRS description: Search pull requests by keyword / query string. cli: gh pr list --search ""