Skip to content

reviewing-prs, ardia, gia: only work PRs the user opened, is assigned to, or asked for, or the Actions app authored - #2913

Merged
d-morrison merged 59 commits into
mainfrom
claude/pr-authorship-scope
Sep 2, 2026
Merged

reviewing-prs, ardia, gia: only work PRs the user opened, is assigned to, or asked for, or the Actions app authored#2913
d-morrison merged 59 commits into
mainfrom
claude/pr-authorship-scope

Conversation

@d-morrison

@d-morrison d-morrison commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Directives

Two, both delivered on 2026-09-01 during a gia sweep of UCD-SERG/serodynamics:

UCD-SERG/serodynamics#284 wasn't yours to work on

you should only work on PRs that are opened by me (d-morrison or dem-extra1) or assigned to me

and, after the session had stood down from every PR failing that test:

correction: workflow-opened PRs like UCD-SERG/serodynamics#292 are ok

What the sweep had done

Pushed commits to #284, #292, #298 and #311 and dispatched a paid review on #310 — four PRs by other lab members, one (#292) by github-actions[bot], none assigned to the user (verified from the API). Every individual action was a correct ARDI step. The error was the population: it was decided by reading ardia's "drive every open PR" as a grant, rather than by asking whose PRs they were. The session then stood down from all five, which over-corrected on #292; the second directive reversed that one.

The rule, in one place

A PR is in scope when the invoking user opened it, is assigned to it, explicitly asked for work on it by name (a mention such as "do not touch #N" is not a request; an explicit chores call names the Dependabot/Renovate population), or the GitHub Actions app (github-actions) authored it. An explicit exclusion ("do not touch #N") is a veto checked before every positive arm, the user's own PRs included, and travels with the sweep into every recheck and delegated scan. The last positive arm is an author test, not a provenance test: the directive said "workflow-opened", and the rule narrows that on purpose because provenance is not observable from the API, so a workflow PR opened under WORKFLOW_TOKEN posts under that token's identity and needs an assignment or an explicit request. A claim comment never adds an arm, and scope is a live precondition re-read before every write. An out-of-scope PR is reported to the user and left untouched: no push, no comment, no review, no merge. When no identity operation is available, the filter fails closed to the explicitly-requested and Actions-app-authored arms. Scope authorizes a PR; whether it is free to work is the separate claim check in claim-pr.md.

Where it lives

AGENTS.md — a compact universal section, since the directive is unscoped and that file is the unconditional cross-agent contract.

memories/reviewing-prs.md — the full rule with both sides per CLAUDE.md's "Record both the pattern and the anti-pattern", stating which parts the user said (author/assignee; workflow PRs are fine), which were carried over (the explicit-request arm, from ardia's former "unless told to" bullet), and which are inferred (the Don'ts, the issue carve-out, the narrowing to the Actions app). It records the three forms of the app's login (github-actions[bot] from REST/MCP, bare github-actions from GraphQL and scripts/pr-sweep.py, app/github-actions from gh --json, measured 2026-09-01), the field names per source (author.login vs user.login vs author.username; the MCP tools return assignees as bare login strings and omit the key when empty), and the WORKFLOW_TOKEN cases against gha at 82f3c36. It lands here rather than in preferences.md because that file is at its size cap and this file's header says review-scope lessons go here.

skills/ardia/SKILL.md — the reference implementation, in step 1: resolve the invoking user (gh api user, mcp__github__get_me, glab api user), add that user's aliases, normalise the author/assignee fields, apply the exclusion veto, keep the full listing for stack detection, exclude out-of-scope PRs from the action queue, and name them in the report. LIST_PRS fetches assignees.

skills/chores/SKILL.md — the executable predicate: PR_SCOPE_ALIASES, PR_SCOPE_REQUESTED, and PR_SCOPE_EXCLUDED inputs, a fail-closed identity block, a jq filter with the veto checked first, and a pre-write refresh of every predicate input (author, assignees, title, labels) before each close, comment, or merge.

skills/ardi, gia, ardiaei, mma, cascade, wrap-up, post-merge, handoff, check-history, gi, mwc, shared/workflow/ardi.md, derive-dont-enumerate.md, sync-with-main.md, batch-merge-and-resolve.md, CLAUDE.md, memories/preferences.md, memories/github.md, memories/MEMORY.md, scripts/pr-sweep.py, scripts/pr-overlap.py — every other place that pushes to, comments on, resolves conflicts on, merges, or describes the population of PRs now applies the same test or defers to ardia's filter, fetching author and assignees where it lists PRs. A whole-corpus grep for the retired "owns or has explicitly claimed" wording is empty.

tool-mappings.yml — a WHO_AM_I operation mapping gh api user --jq .login to mcp__github__get_me (the markdown reference is generated from it).

On the literal logins

preferences.md says never to hardcode usernames. The skills carry no personal usernames — they resolve whoever is running them. Only the memory names d-morrison and dem-extra1, as this owner's two aliases for one person (the split preferences.md's ## Git author mapping already records), under the rule's own exception for values that must match a real account.

Review history

Verification

After each commit, gated so a failure aborts the push: check-memory-file-size.py, new-line-breaks (re-run after the commit, since it reads the commit graph), markdownlint-cli2@0.23.0 with the repo config, validate-skills.py, check-links.py, check-ascii-punctuation.py --diff --base origin/main; both scripts compile; the chores jq filter was extracted from the file and run on sample data for every arm and veto case.

🤖 Generated with Claude Code

https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F

User directive, 2026-09-01, delivered mid-turn on a gia sweep of
UCD-SERG/serodynamics: "you should only work on PRs that are opened by me
(d-morrison or dem-extra1) or assigned to me", after the sweep pushed
commits to #284, #292, #298 and #311 and dispatched a review on #310 --
four other authors' PRs and one bot PR, none assigned to the user.

Record the rule in memories/preferences.md with both sides: the Do the user
stated, and the Don'ts inferred from the near-miss, chiefly that a sweep
skill's "drive every open PR" is not a scope grant.

Put the mechanism where the population is decided. ardia's step 1 already
lists scope rules for drafts; add the authorship filter as the first rule,
so the enumeration never reaches another author's PR. gia's step 0 gets the
same statement beside its existing which-repo check, and its anti-pattern
list gets the corresponding entry.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F
…home

Two CI failures on the first push, both mine.

validate: memories/preferences.md went to 1277 lines against the 1250 cap.
main was already at 1238. reviewing-prs.md's own header says new
review-scope lessons land there rather than as appends to preferences.md,
for exactly this reason, and I had not read it. Move the whole section,
repoint ardia's citation, and extend the MEMORY.md row.

lint-markdown: MD018 at the line that began "#284, #292 ...". Semantic line
wrapping put a PR number at a line start, which markdownlint reads as a
space-less ATX heading. Rewrapped so no line begins with "#".

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@d-morrison d-morrison changed the title preferences, ardia, gia: only work PRs the user opened or is assigned to reviewing-prs, ardia, gia: only work PRs the user opened or is assigned to Sep 1, 2026
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

…; fold the old bullet

Round-1 review findings on #2913, all three accepted.

The LIST_PRS command never fetched assignees, so the "or assigned to me"
half of the new filter could not be evaluated from the data the step tells
you to pull. Add the field.

The new bullet duplicated an older, softer one thirty lines down ("unless
told to ... ask first"). Fold them: the old bullet's one real override
survives as "or the user named the PR in the request"; "ask first" goes,
since author and assignee are two API fields and there is nothing left to
ask.

The literal logins sat in tension with preferences.md's "Never hardcode
usernames" rule, and the reviewer's underlying point was real: a lab member
running ardia from the vendored corpus would have filtered on the owner's
identity and dropped their own PRs. The skill now resolves the invoking user
first (gh api user, or get_me remotely) and only adds the aliases the
memory lists. The memory keeps d-morrison and dem-extra1 as this owner's
two aliases and says why they are literal: the rule's own exception for
values that must match a real account. gia loses its third hardcode site.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F

Copy link
Copy Markdown
Collaborator Author

Round-1 findings, all three addressed in 3342a7f.

1. LIST_PRS never fetched assignees — Addressed. Correct, and it made half the filter unevaluable from the data the step prescribes. assignees is now in the --json list.

2. Duplicate of the older bullet — Addressed. Folded. The old bullet's one genuine override ("unless told to") survives as "or the user named the PR in the request". Its "if unsure, ask first" is gone on purpose: author and assignee are two API fields, so nothing is left to ask, and that softness is exactly what let the incident through.

3. Hardcoded logins vs. the no-hardcoding rule — Addressed, structurally. The underlying point was stronger than a style tension: a lab member running ardia from the vendored corpus would have filtered on the owner's identity and dropped their own PRs. So the skills now carry no usernames — they resolve the invoking user (gh api user / get_me) and only add aliases the memory lists. The memory keeps d-morrison and dem-extra1 as this owner's two aliases for one person, which a gh api user lookup cannot unify, and cites the rule's own exception for values that must match a real account. gia's third hardcode site is removed too; grep confirms neither skill contains either login.

Verified after committing: size checker clean, new-line-breaks and markdownlint-cli2@0.23.0 both pass with negative controls, check-links.py clean.

Not re-dispatching by mention — this repo auto-reviews on push and a mention would race it.


Generated by Claude Code

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

User correction, same day as the authorship rule: a PR opened by one of
the repository's own workflows (github-actions[bot], e.g. a submodule
bump) may be driven without an assignment. Dependabot and Copilot PRs
still need one. Record both gh and API spellings of the workflow login.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F

Copy link
Copy Markdown
Collaborator Author

Pushed 86037f1 on top of 3342a7f, carrying a second directive from the user rather than a review finding: workflow-opened PRs (github-actions[bot], e.g. a submodule bump such as UCD-SERG/serodynamics#292) are in scope without an assignment. Dependabot and Copilot PRs still need one.

Changes: the memory's opening rule and Do/Don't pairs now carry the carve-out and say which parts the user stated and which are inferred (the narrowing to the github-actions[bot] login); the case narrative records the over-correction on #292; ardia's filter and gia's step 0 admit the workflow bot, with both spellings of its login (github-actions[bot] via the API, app/github-actions in gh pr list --json author).

The three round-1 findings stay addressed as described in the previous comment. Same local checks re-run after committing: size cap, new-line-breaks, markdownlint-cli2@0.23.0 (one MD018 caught and fixed before push), check-links.py. The push supersedes the review that was running on 3342a7f; not re-dispatching by mention.


Generated by Claude Code

@d-morrison d-morrison changed the title reviewing-prs, ardia, gia: only work PRs the user opened or is assigned to reviewing-prs, ardia, gia: only work PRs the user opened, is assigned to, or a workflow opened Sep 1, 2026
@github-actions

This comment has been minimized.

- State the workflow bot's login once as the app slug and list all
  three forms (REST/MCP suffix, GraphQL bare, gh --json prefix); the
  earlier two-form list would have dropped workflow PRs on the GraphQL
  path scripts/pr-sweep.py uses.
- Point ardi.md's and pr-sweep.py's scope statements at the memory's
  test; a claim comment no longer reads a PR into scope.
- Rename the memory heading to carry the workflow case; blank line
  before it; fix the MEMORY.md index wording; semantic line breaks on
  the new prose.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F
@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Collaborator Author

The automated review of 86037f1 was quota-skipped, so per shared/workflow/self-review-fallback.md an adversarial-reviewer subagent reviewed the full diff at the bot's standard. It returned six findings; all six addressed in c38a918:

  1. Workflow-login spellings incomplete (factual). Accepted. GraphQL returns the bare slug github-actions, which is what scripts/pr-sweep.py and gh api graphql emit, so "match either" would have dropped workflow PRs on exactly that path. Both sites now state the invariant once: match the app slug github-actions, which REST/MCP suffix (github-actions[bot]), GraphQL/pr-sweep.py return bare, and gh pr list --json author prefixes (app/github-actions, is_bot: true). The reviewer verified the app/ prefix from gh's api/queries_pr.go MarshalJSON and the [bot] form from the MCP output for serodynamics#292.
  2. ardi.md and pr-sweep.py still said "owns or has explicitly claimed" (consistency). Accepted; a claim comment must not read another member's PR into scope. Both now point at the memory's test.
  3. Heading omitted the workflow case (nit). Renamed, and the quoted title in ardia updated.
  4. No blank line before the heading (nit). Added.
  5. Column-wrapped prose (nit). Ran scripts/semantic-line-breaks.py --write over the changed paragraphs; one pre-existing ardia paragraph it also joined was restored to its clause-per-line form.
  6. MEMORY.md row changed subject mid-list (nit). Reworded.

A second adversarial pass on c38a918 is running now; I will post its verdict here before acting on the PR. Local checks after committing: size cap, new-line-breaks, markdownlint-cli2@0.23.0, check-links.py, pr-sweep.py parses.


Generated by Claude Code

… arm everywhere

Second self-review round: the memory's opening sentence, gia's
anti-pattern, the MEMORY.md row, and the section heading stated three
arms while every other site stated four. derive-dont-enumerate.md still
told the reader to claim an unowned PR before driving it. ardia now
points at the memory's login-form table instead of restating it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F

Copy link
Copy Markdown
Collaborator Author

Second adversarial pass (on c38a918; the automated review of that head was also quota-skipped) returned five findings, all addressed in 70a493f and 5b35a5a:

  1. Memory's opening sentence stated three arms; every other site states four. Accepted. "named by me in the request" added, and the "unless it is assigned to me" clause now reads "unless it is assigned to me or I named it".
  2. gia's anti-pattern omitted the named-in-request arm. Accepted; fixed.
  3. shared/workflow/derive-dont-enumerate.md still described ardi's mandate as "owns or has explicitly claimed" and told the reader to claim an unowned PR before driving it. Accepted; it now cites the memory's scope test and says a claim comment does not confer scope. A whole-corpus grep for the retired phrase is empty.
  4. Heading and MEMORY.md row omitted the arm (nit). Both now read "opened, am assigned to, or named, or a workflow opened"; ardia's quotation of the heading updated.
  5. ardia restated the three-spelling login table (nit). It now names the app slug and points at the memory for the forms, so there is one table.

5b35a5a only rebreaks two long lines the new-line-breaks check flagged after 70a493f. Third adversarial pass running on 5b35a5a; CI re-running. Local: size cap, new-line-breaks (exit 0), markdownlint-cli2@0.23.0, check-links.py all clean.


Generated by Claude Code

@github-actions

This comment has been minimized.

Point pr-overlap.py's docstring at the memory's scope test; record where
the named-in-request arm came from and the WORKFLOW_TOKEN edge case for
workflow-opened PRs; make ardia's bold lead name all four arms; rebreak
three prose lines; note that #310 got a dispatched review, not commits.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F

Copy link
Copy Markdown
Collaborator Author

Third adversarial pass (on 5b35a5a) returned seven findings, all addressed in 722be6c and 27dcc5f:

  1. scripts/pr-overlap.py docstring still attributed the retired "owns or has claimed" scope to ardi.md. Accepted; it now cites the memory's test like pr-sweep.py. A corpus grep for the retired phrase is empty.
  2. Provenance of the named-in-request arm unstated. Accepted; the memory now says it was carried over from ardia's former "unless told to" bullet, not from either directive.
  3. WORKFLOW_TOKEN edge case (nit). Accepted and worth recording: gha's open-sync-pr prefers WORKFLOW_TOKEN, so a workflow-opened PR can post under the PAT holder's login and miss the github-actions match; the memory now says it is then in scope only through the author arm when that PAT is the user's.
  4. "naming the commits" was imprecise for Add a skill for pushing general-purpose memories to ai-config from another repo #310 (nit). Fixed: "(or, on Add a skill for pushing general-purpose memories to ai-config from another repo #310, the dispatched review)".
  5. Three hard-wrapped lines (nit). Rebroken per sentence.
  6. ardia's bold lead named only one arm (nit). Now "Only PRs the user opened, is assigned to, or named, or a repository workflow opened, are in scope."
  7. One 272-character line in derive-dont-enumerate.md (nit). Broken before the second clause.

27dcc5f rebreaks one line the new-line-breaks check flagged on 722be6c. Fourth adversarial pass running on 27dcc5f; CI re-running.


Generated by Claude Code

Attribute the WORKFLOW_TOKEN preference to gha's reusable sync workflows
rather than the open-sync-pr composite; state that the issue carve-out
is inferred; make the every-open-PR qualifier reach ardia's intro.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F

Copy link
Copy Markdown
Collaborator Author

Fourth adversarial pass (on 27dcc5f) returned four findings; three addressed in 2f0bcf2, one deferred to a filed issue:

  1. WORKFLOW_TOKEN preference mis-attributed to the open-sync-pr composite. Accepted: the composite's token input is required with no default; the ${{ secrets.WORKFLOW_TOKEN || github.token }} choice lives in gha's four reusable sync workflows (bump-submodule.yml, bump-dev-version.yml, sync-shared-fragments.yml, sync-upstream.yml). The memory now says so.
  2. ardia's "Every open PR" qualifier said "below" but the skill's intro sits above it (nit). Now "anywhere in this skill".
  3. The issue carve-out paragraph carried no provenance (nit). Now marked as inferred: neither directive mentioned issues.
  4. hooks/monitor-open-prs.py polls --author @me only, so assigned and workflow-opened PRs are never reconciled (pre-existing). Deferred; the reviewer suggested a follow-up rather than widening this PR. Filed as a separate issue in this repo referencing reviewing-prs, ardia, gia: only work PRs the user opened, is assigned to, or asked for, or the Actions app authored #2913.

Fifth adversarial pass running on 2f0bcf2; CI re-running. Local checks gated: new-line-breaks, markdownlint-cli2@0.23.0, check-links.py, size cap all pass.


Generated by Claude Code

@github-actions

This comment has been minimized.

…ot once at listing time

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F

Copy link
Copy Markdown
Collaborator Author

ARD for Copilot round thirty-one (on 97cfd16), pushed as 447fae7:

  1. Addressed (skills/ardia/SKILL.md:46) -- step 1 now states that scope is a live precondition rather than a one-time classification: the author and assignees are re-read and the test reapplied immediately before each push, comment, review, or merge, and a PR that no longer passes is dropped and reported.
  2. Addressed (skills/chores/SKILL.md:99) -- the listing is named as a snapshot, and the author and assignees are refreshed and the same predicate reapplied immediately before each write action in steps 3-5 (a @dependabot comment, a merge).

Local checks on 447fae7: new-line-breaks, markdownlint (0 errors), validate-skills, check-ascii-punctuation --diff all pass.

Posted by Claude Code (AI agent) --- not written by a human.


Generated by Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The chores predicate can still select and merge a dependency-bot PR that the user explicitly excluded.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 33/33 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread skills/chores/SKILL.md
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

…edicate

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F

Copy link
Copy Markdown
Collaborator Author

ARD for Copilot round thirty-two (on 447fae7), pushed as 28c20e5:

  1. Addressed (skills/chores/SKILL.md:100) -- a third scope input, PR_SCOPE_EXCLUDED (PR numbers the user told this run not to touch), feeds an $exc array that is checked before every positive arm, bot authors included, and the pre-write refresh reapplies that veto. Proven on sample data: an excluded Dependabot PR and an excluded assigned PR are dropped while the others pass. The validator allowlist carries the new name.

Local checks on 28c20e5: new-line-breaks, markdownlint (0 errors), validate-skills, check-ascii-punctuation --diff all pass.

Posted by Claude Code (AI agent) --- not written by a human.


Generated by Claude Code

@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Collaborator Author

Self-review fallback for 447fae7..28c20e5

The automated @claude review of 28c20e5 was quota-skipped (require-review skipped, no verdict posted), so this is the fallback per shared/workflow/self-review-fallback.md, held to the bot's own bar. Done inline rather than by a dispatched reviewer, since subagent launches are paused for this session.

Scope of the delta (2 files, +17/-6): the PR_SCOPE_EXCLUDED veto in skills/chores/SKILL.md (Copilot round thirty-two's finding), plus one allowlist line in scripts/validate-skills.py so the backticked input name validates.

What was checked

  • Predicate correctness. The jq filter was extracted verbatim from the file (not retyped) and run on a seven-PR sample covering a labelled Dependabot PR, an unlabelled Dependabot PR, an unassigned other-member chore, an assigned other-member chore, an Actions-app chore, the user's own non-chore PR, and an explicitly requested other-member chore. With EXC=[2,4] it lists 1, 5, 7; with EXC=[] it lists 1, 2, 4, 5, 7; with EXC=[7] (requested and vetoed) it lists 1, 2, 4, 5. So the veto outranks every positive arm, the bot-author arm included, and removing it reproduces round thirty-two's behaviour exactly.
  • Parsing. PR_SCOPE_EXCLUDED="2, 4" parses to [2,4] (jq's tonumber tolerates the space). A non-numeric token (#789) makes jq exit non-zero under set -eo pipefail, which is the same loud failure PR_SCOPE_REQUESTED already has and the fail-fast direction we want, not a silent empty veto.
  • Prose agreement. "Set the three scope inputs", the fail-closed sentence ("with all three unset"), the example line, and the snapshot paragraph ("the PR_SCOPE_EXCLUDED veto included") all name the same three inputs; the snapshot paragraph is what makes "neither listed nor merged" true at the merge step, not only at the listing.
  • Validator. validate-skills.py, new-line-breaks, markdownlint-cli2@0.23.0, check-links.py, and check-ascii-punctuation.py --diff --base origin/main all pass on 28c20e5 locally; CI's new-line-breaks, both lints, and build are green on the head, validate still running at the time of writing.

Findings: none.

Verdict

Ready for merge, pending the in-flight Copilot review of 28c20e5 and the remaining validate job.

Posted by Claude Code (AI agent) --- not written by a human.


Generated by Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The chores workflow omits the live scope recheck before its step 2 PR-closing action.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 33/33 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread skills/chores/SKILL.md Outdated
Copilot round thirty-three: the live recheck named steps 3-5, but step 2
closes a bump PR when the repo has migrated to a native plugin, which is
a write action too. Name steps 2-5 and list the close.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F
@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Collaborator Author

Self-review fallback for 28c20e5..988b545

The automated @claude review of 988b545 was quota-skipped (warning above, require-review skipped), so this is the fallback per shared/workflow/self-review-fallback.md, done inline since subagent launches are paused for this session.

Scope of the delta: one file, skills/chores/SKILL.md, one sentence. The live-recheck paragraph now says "before each write action in steps 2-5 (closing a bump PR, a @dependabot comment, a merge)" instead of "steps 3-5 (a @dependabot comment, a merge)". This is Copilot round thirty-three's finding.

What was checked

  • Completeness of the enumeration. grep -nEi 'gh pr (close|comment|merge|review|edit)|close the|merge ' over the skill finds the write actions at step 2 (close the bump PR, line 150), step 3 (·@·d·ependabot r·ebase comment, line 175), and step 4 (gh pr merge at lines 186 and 196, ·@·d·ependabot s·quash a·nd m·erge comment at line 204). Step 5 is the report. No write action sits outside steps 2-5, and none is missing from the parenthetical's three kinds.
  • Semantic line breaks and lint. The rewrapped sentence spans three lines at clause boundaries; new-line-breaks was re-run after the commit (the check reads the commit graph), and markdownlint-cli2@0.23.0, validate-skills.py, check-links.py, and check-ascii-punctuation.py --diff --base origin/main pass.
  • No behavioural change to the predicate. The jq filter is untouched, so the sample-data proofs in the previous self-review still apply.

Findings: none.

Verdict

Ready for merge, pending Copilot's review of 988b545 and CI on that head.

Posted by Claude Code (AI agent) --- not written by a human.


Generated by Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Explicitly excluded PRs can still pass another positive scope arm, including in delegated scans.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 33/33 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread AGENTS.md
Comment thread skills/ardia/SKILL.md
Comment thread skills/post-merge/SKILL.md Outdated
Copilot round thirty-four: the veto existed only in chores's
PR_SCOPE_EXCLUDED, so "do not touch" plus a number still admitted a PR
through the author, assignee, or Actions-app arm in AGENTS.md, the
memory, ardia's reference filter, and post-merge's scanner brief. State
the veto in all four and carry the exclusion list into rechecks and
delegated scans.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F
@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Collaborator Author

Self-review fallback for 988b545..3b32086

The automated @claude review of 3b32086 was quota-skipped (warning above, require-review skipped), so this is the fallback per shared/workflow/self-review-fallback.md, done inline since subagent launches are paused for this session.

Scope of the delta: four files, prose only (+10/-1): the explicit-exclusion veto stated in AGENTS.md's scope section, memories/reviewing-prs.md's Do bullet, skills/ardia/SKILL.md step 1, and skills/post-merge/SKILL.md's scanner brief. Copilot round thirty-four's three findings.

What was checked

  • Consistency with the chores predicate. Each statement describes the veto the same way chores's PR_SCOPE_EXCLUDED implements it: checked before every positive arm, the user's own and Actions-app-authored PRs included, carried into rechecks and delegated scans. The memory names that variable as the executable form, so the prose and the code point at one definition rather than two.
  • Consistency within ardia step 1. The new sentence says an excluded PR is "dropped"; the sentence before it scopes exclusion to the action queue, and the later sentence keeps the full listing for stack detection, so "dropped" reads as dropped from the action queue, which is the intended meaning. No contradiction with the stack-detection rule.
  • The review-only carve-out. A CI- or skill-dispatched review-only run still satisfies the explicit-request arm; the veto applies to a sweep's action queue and does not turn a dispatched review into an out-of-scope act. The two rules govern different inputs and do not collide.
  • Coverage of restatements. A corpus grep for "do not touch", "not a request", and PR_SCOPE_EXCLUDED finds the arm restated only in these four files plus chores; every other site defers to the memory's scope test and so inherits the veto.
  • Checks. markdownlint-cli2@0.23.0, validate-skills.py, check-memory-file-size.py, check-links.py, new-line-breaks (re-run after the commit), and check-ascii-punctuation.py --diff --base origin/main all pass locally on 3b32086.

Findings: none.

Verdict

Ready for merge, pending Copilot's review of 3b32086 and CI on that head.

Posted by Claude Code (AI agent) --- not written by a human.


Generated by Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Two scope-check gaps can still mutate a PR using an omitted or stale classification.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

memories/github.md:75

  • Step 3 is also mutating: it closes superseded drafts and posts a note. Limiting the scope check to steps 2 and 4 therefore still permits closing or commenting on an out-of-scope PR. Apply the filter before every mutation in steps 2 through 4.
    skills/chores/SKILL.md:135
  • The chore predicate also depends on the title and labels, but this live recheck explicitly refreshes only author and assignees. If a human-authored PR loses its chore( title or dependencies label after the initial listing, the stale classification can still lead to a close, comment, or merge. Refresh every predicate input before each write.
  • Files reviewed: 33/33 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot round thirty-five, two suppressed findings. The GII cleanup
sweep gated only steps 2 and 4, but step 3 closes superseded drafts and
posts a note; gate every mutation in steps 2 through 4. The chores
recheck refreshed only author and assignees, but the predicate also
reads the title and labels; re-fetch every input before each write.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F

Copy link
Copy Markdown
Collaborator Author

ARD for Copilot round thirty-five (3b32086)

Both findings were suppressed comments with no thread, so they are answered here. Both accepted and Addressed in ab89045.

# Finding Disposition
1 memories/github.md: the GII cleanup sweep gated the scope test on steps 2 and 4, but step 3 closes superseded drafts and posts a note. Addressed. The sweep now applies the test immediately before every mutation in steps 2 through 4, naming each one (the close in step 2, the close and the note in step 3, the merge in step 4).
2 skills/chores/SKILL.md: the live recheck refreshed only author and assignees, while the predicate also reads the title and labels. Addressed. The recheck now re-fetches every input the predicate reads (author, assignees, title, labels) before each write action in steps 2-5. ardia's recheck is unchanged on purpose: its predicate reads author, assignees, and the request's static request and exclusion lists, so author and assignees are the only mutable inputs there.

Local checks pass on the committed content. Copilot re-requested on ab89045.

Posted by Claude Code (AI agent) --- not written by a human.


Generated by Claude Code

@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Collaborator Author

Self-review fallback for 3b32086..ab89045

The automated @claude review of ab89045 was quota-skipped (warning above, require-review skipped), so this is the fallback per shared/workflow/self-review-fallback.md, done inline since subagent launches are paused for this session.

Scope of the delta: two prose edits, no executable change. memories/github.md's GII cleanup sweep now gates the scope test on every mutation in steps 2 through 4 and names each (close, close plus note, merge). skills/chores/SKILL.md's recheck paragraph now re-fetches author, assignees, title, and labels before each write in steps 2-5.

What was checked

  • Enumeration, both axes. Write actions in the GII sweep were read off the four numbered steps: step 1 lists, step 2 closes, step 3 closes and comments, step 4 merges, so "steps 2 through 4" is the complete set. Predicate inputs in the chores filter were read off the jq: .author.login, .assignees[].login, .title, .labels[].name, .number; the number cannot change, the other four are named in the recheck.
  • Consistency with sibling restatements. grep -rn "re-read the author\|refresh the author\|reapply the" finds only ardia step 1 as another recheck sentence. Its predicate reads author, assignees, and the request's static request and exclusion lists, so "author and assignees" is complete there and it was left unchanged.
  • Checks. markdownlint-cli2@0.23.0, validate-skills.py, check-memory-file-size.py, check-links.py, new-line-breaks (re-run after the commit), and check-ascii-punctuation.py --diff --base origin/main all pass locally on ab89045.

Findings: none.

Verdict

Ready for merge, pending Copilot's review of ab89045 and CI on that head.

Posted by Claude Code (AI agent) --- not written by a human.


Generated by Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The existing-PR paths in check-history can still race another session holding a live claim.

Review details

Suppressed comments (2)

skills/check-history/SKILL.md:58

  • Passing the scope test establishes authorization, not that the branch is free. This replacement now directs the caller to drive the PR even when another session has a live claim; shared/workflow/claim-pr.md:78-82 says that claim still blocks a second session. Check claim status first and only take over an unclaimed or expired PR.
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.

skills/check-history/SKILL.md:88

  • This duplicated path has the same authorization/ownership gap: an in-scope PR may still have a live claim from another session, so immediately reviewing or extending it can race that session. Apply the claim check here too and leave live-claimed work alone until the claim expires or is released.
   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 leave it untouched (reporting
   it to the user) when it fails.
  • Files reviewed: 33/33 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

…s free

Copilot round thirty-six, two suppressed findings. Both existing-PR paths
told the caller to drive, review, or extend an in-scope PR without
reading its claim state, which races another session holding a live
claim (claim-pr.md's second-session rule). Read the claim first, skip a
live one, and take over an expired one by posting a claim comment.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F

Copy link
Copy Markdown
Collaborator Author

ARD for Copilot round thirty-six (on ab89045, two suppressed findings, no threads)

Both Addressed in 2ce74f9.

  • skills/check-history/SKILL.md:58 (drive an in-scope PR without checking its claim). The paragraph now says scope establishes authorization, not that the branch is free: read the PR's claim state first per 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.
  • skills/check-history/SKILL.md:88 (the duplicated review-or-extend path). Now gated on the same claim check clearing, with the live-claim-blocks and expired-claim-takeover halves stated inline.

skills/gi/SKILL.md step 4 already carried both halves (live claim skips, expired claim is taken over by a fresh claim comment, stalled in-scope PR is claimed before ARDI), so this brings check-history into line with it rather than introducing a new rule.

Posted by Claude Code (AI agent) --- not written by a human.


Generated by Claude Code

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.

2026-09-02 04:04 UTC

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Claude finished reviewView run


Confirmed all three prior findings are fixed:

  1. LIST_PRS now fetches assignees (skills/ardia/SKILL.md:26).
  2. The redundant "Only iterate PRs the user owns" bullet was removed, consolidated into the new scope-test section.
  3. The hardcoded-username tension now has a deliberate, dynamic-resolution-first design: ardia/SKILL.md:34 resolves the invoking user dynamically and only adds d-morrison/dem-extra1 as aliases, with an explicit rationale note in memories/reviewing-prs.md.

PR Summary

Self-authored memory/skill correction recorded after a gia sweep on UCD-SERG/serodynamics mistakenly drove PRs opened by other lab members and a bot. The PR adds a new "Only work PRs I opened, am assigned to, or asked for by name, or the Actions app authored" scope rule (memories/reviewing-prs.md), wires it into AGENTS.md, CLAUDE.md, and every skill/script that lists, pushes to, comments on, or merges PRs (ardia, ardi, gia, giigi, chores, cascade, mma, post-merge, wrap-up, handoff, check-history, mwc, pr-overlap.py, pr-sweep.py), and adds a WHO_AM_I tool-mapping entry. Since the last automated-review round (447fae7b), the diff has gone through several more Copilot/self-review-fallback rounds (visible in the PR thread) addressing further nits (fail-closed identity handling, exclusion-veto ordering, wording of "in-scope" examples), all landing in the current head.

Verification performed this round

  • python3 scripts/check-links.py: 3186 relative links across 664 files, none broken.
  • python3 scripts/check-memory-file-size.py: no memory file exceeds 1250 lines (preferences.md is 1244).
  • python3 scripts/validate-skills.py: all 201 skills and Codex wrappers valid (only pre-existing, unrelated capacity-headroom warnings).
  • python3 scripts/check-ascii-punctuation.py --diff --base 81cf4c1: 417 added lines checked, no non-ASCII punctuation.
  • python3 -m py_compile on the three modified Python scripts: compiles clean.
  • Hand-built and ran the new chores predicate's jq filter against synthetic PR data (dependabot author, github-actions author, assigned-to-user author, unassigned non-chore author, feature PR, excluded PR, and the identity-lookup-failed fail-closed path) — all six/seven cases classified correctly, including the fail-closed case correctly excluding an assignee-matched PR when identity resolution fails.
  • Confirmed via gh pr view that CI (validate, lint-markdown, lint-qmd, new-line-breaks) passes or is only in-flight (in-flight checks don't factor into the verdict per this run's instructions), and mergeable: MERGEABLE (no conflicts).

One thing I want to flag as not a finding on this PR: my working-tree checkout shows CLAUDE.md locally modified (git status reports M CLAUDE.md), and that uncommitted local copy reverts several of this PR's CLAUDE.md hunks (e.g. it still reads "A conflict you caused on a branch you do not own..." instead of the PR's new scope-test wording). I verified with git show HEAD:CLAUDE.md that the actual committed HEAD content matches the PR diff correctly — the discrepancy is an artifact of this review sandbox's uncommitted local state, not a defect in the PR's commits. Noting it so it isn't mistaken for a rebase/merge regression in a future round.

I found no new blocking issues, and all previously-flagged findings are resolved in the current diff.

Verdict

Ready for merge

Structured Review Data (JSON)
{
  "schema_version": "1.0",
  "reviewer": "claude",
  "commit_sha": "7b92cbf77322b82d48d0885fc5ddcd671e390ce1",
  "verdict": "CLEAN",
  "findings": []
}

Reviewed commit: 2ce74f9

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

💰 Cost: $1.6011 (review) — run

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The authorization rule is consistently propagated with fail-closed handling and no unresolved defects were found.

Review details
  • Files reviewed: 33/33 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants