Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .editorconfig
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ indent_size = 2
[.github/workflows/*.{yml,yaml}]
end_of_line = lf

# Composite action metadata is Actions-owned like the workflows above, and Dependabot rewrites the
# `uses:` pins inside it with LF exactly as it does theirs, so declaring LF keeps it consistent.
[.github/actions/**/*.{yml,yaml}]
end_of_line = lf

# Catalog snippet workflows mirror the real workflow files pinned LF above.
# Keep the snippets LF, so a copied snippet lands compliant instead of needing conversion.
[catalog/snippets/workflows/*.{yml,yaml}]
Expand Down
97 changes: 97 additions & 0 deletions .github/actions/prose-gate/action.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
# Fleet prose gate, consumed by downstream repos so the rules live here rather than in 20 copies.
# Pin this action to a commit SHA per GOVERNANCE.md "Action pinning".
# The pin is one literal, identical on both branches.
# `uses:` takes no expressions, so a per-branch ref would diverge at every promotion.
# Branch-dependent behavior therefore lives here, where expressions are legal.
name: Fleet prose gate
description: Check the lines a change touches against the fleet prose rules.

inputs:
base:
description: Ref to diff against, so only lines the change touches are reported.
required: true
paths:
description: Paths to scan.
required: false
default: .
rules-ref:
description: >-
Hub ref supplying the rules.
Empty selects develop on every branch except main, which takes the pinned action version.
required: false
default: ''

runs:
using: composite
steps:
# A develop-targeted run reads the rules from hub develop.
# That exercises an unpromoted rule change fleet-wide before it reaches main.
# Every other run uses the copy bundled at this action's pinned SHA.
# A released repo's gate is then reproducible, and a hub commit cannot fail a re-run.
- name: Resolve the rules source step
id: rules
shell: bash
env:
REQUESTED: ${{ inputs.rules-ref }}
# `base_ref` is set only on a pull_request event, and a fleet repo gates on push instead.
# The branch name is the reliable signal, so it is read whenever `base_ref` is empty.
TARGET: ${{ github.base_ref || github.ref_name }}
run: |
set -Eeuo pipefail
ref="${REQUESTED:-}"
# Only main is pinned, since it is the released tier and a re-run of it must not change.
# Every other branch merges into develop, so it tracks develop and exercises rules early.
if [ -z "$ref" ] && [ "$TARGET" != "main" ]; then ref="develop"; fi
if [ -z "$ref" ]; then
# Three levels up from the action directory is the repository root of this pinned checkout.
bundled="$GITHUB_ACTION_PATH/../../../scripts/prose_lint.py"
if [ ! -f "$bundled" ]; then
echo "::error::Bundled prose_lint.py not found at $bundled" >&2
exit 1
fi
echo "script=$bundled" >>"$GITHUB_OUTPUT"
echo "Rules source: bundled at this action's pinned version"
else
dst="$RUNNER_TEMP/prose_lint.py"
url="https://raw.githubusercontent.com/ptr727/ProjectTemplate/$ref/scripts/prose_lint.py"
# Retry a transient network failure, since it would otherwise fail a correct change.
# A non-200 still fails the step, because -f keeps the trust model unchanged.
# Fail loudly rather than skipping the gate, since a silent skip reports a clean pass.
curl -fsSL --retry 3 --retry-delay 2 --retry-all-errors "$url" -o "$dst"
echo "script=$dst" >>"$GITHUB_OUTPUT"
echo "Rules source: hub $ref"
fi

# The caller checks out with fetch-depth 0, because a diff against the base needs its history.
- name: Check prose step
shell: bash
env:
SCRIPT: ${{ steps.rules.outputs.script }}
BASE: ${{ inputs.base }}
PATHS: ${{ inputs.paths }}
run: |
set -Eeuo pipefail
# Check the base resolves before scanning, so an empty or absent ref fails naming itself.
# Unresolvable, the run would report the repository's whole backlog against this change.
# A shallow checkout is the usual cause, so the caller fetches full history.
if ! git rev-parse --verify --quiet "$BASE^{commit}" >/dev/null; then
echo "::error::Diff base '$BASE' does not resolve in this checkout." >&2
echo "::error::Check the ref name and that the job checks out with fetch-depth 0." >&2
exit 1
fi
# A caller may write paths as a multi-line YAML block, and `read -ra` stops at a newline.
# Fold newlines into spaces first, so every path is read.
# Left unfolded it scans only the first path and reports the rest clean.
# That is a silent under-scan, the one failure a gate must never have.
# Splitting on whitespace keeps several paths as several arguments.
# Passing the array quoted keeps each element literal, where an unquoted one would glob.
read -ra scan <<<"$(printf '%s' "$PATHS" | tr '\n' ' ')"
if [ "${#scan[@]}" -eq 0 ]; then
echo "::error::No paths to scan after parsing the paths input." >&2
exit 1
fi
echo "Scanning ${#scan[@]} path(s): ${scan[*]}"
# Options first, then `--`, so every remaining token is read as a literal path.
# A path-shaped token beginning with a dash would otherwise parse as an option, and
# `--list-files` in that position turns the gate into a file listing that exits 0.
python3 "$SCRIPT" --diff "$BASE" -- "${scan[@]}"
56 changes: 52 additions & 4 deletions .github/copilot-instructions.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,22 @@ Auto-review on push is configured (via the branch ruleset's `copilot_code_review
gh api repos/<owner>/<repo>/pulls/<N>/reviews --jq \
'.[] | select(.body | test("Suppressed comments|low confidence")) | .body'

# Scope it to the current head, so an answered finding from an earlier round does not re-open.
# Read every round, not only the head. A suppressed finding has no resolved state, so a push
# does not retire it: it simply stops appearing in a head-scoped query while still unanswered.
# Head-scoping this query is how four rounds went unanswered across three pull requests in a day.
gh api repos/<owner>/<repo>/pulls/<N>/reviews --jq \
'[.[] | select(.body | test("Suppressed comments|low confidence"))] | length'

# Mark which round each came from, since a finding on an older round may since be moot.
PR_HEAD=$(gh pr view <N> --json headRefOid --jq '.headRefOid')
gh api repos/<owner>/<repo>/pulls/<N>/reviews --jq \
"[.[] | select(.commit_id==\"$PR_HEAD\") | select(.body | test(\"Suppressed comments|low confidence\"))] | length"
"[.[] | select(.body | test(\"Suppressed comments|low confidence\"))
| {round: (if .commit_id == \"$PR_HEAD\" then \"head\" else \"earlier\" end), id}]"
```

**Round 1 is normally auto-seeded, so poll for it before trying to self-trigger.** Auto-review-on-open supplies the first review with no `botIds` call needed, but it can lag one to three minutes. After opening a PR (or the first push), **poll** for a Copilot review on the head SHA (see [Verify Review Covered Current Head](#verify-review-covered-current-head)) before concluding none ran. The `requestReviews` mutation below is for **re-requesting on later pushes** (a new head SHA). By then a prior review exists, so its bot node id is readable. A missing bot node id on round 1 therefore means "the auto-review has not landed yet - wait and poll," **not** "ask the maintainer to kick it off."

> **The reviewer login differs by API.** In **GraphQL** (`gh api graphql` and `gh pr view --json reviews`, which is GraphQL-backed) the `Bot.login` is `copilot-pull-request-reviewer`, with **no `[bot]` suffix**. In the **REST** API (`gh api repos/.../issues|pulls/...`) the same account's `user.login` is `copilot-pull-request-reviewer[bot]`, **with** the suffix. Each query below uses the correct form for its API, so match the API, not a single spelling, when adapting them.
> **The reviewer login differs by API, in three forms rather than two.** In **GraphQL** (`gh api graphql` and `gh pr view --json reviews`, which is GraphQL-backed) the `Bot.login` is `copilot-pull-request-reviewer`, with **no `[bot]` suffix**. In the **REST** API (`gh api repos/.../issues|pulls/...`) the same account's `user.login` is `copilot-pull-request-reviewer[bot]`, **with** the suffix. In a REST **timeline** `review_requested` event the `requested_reviewer` is a third spelling again, login `Copilot` with `type` `Bot`, so a filter written against either of the other two selects nothing there and reports a pull request with requests as having none. Match on the type plus a loose login test rather than on any one spelling, and each query below uses the correct form for its API.

```sh
# 1. PR node id + the Copilot reviewer's bot node id (read from any existing
Expand Down Expand Up @@ -109,7 +116,8 @@ Known non-working request paths (don't rely on them, and use the `requestReviews
- `copilot-pull-request-reviewer` as a requested reviewer slug returns 422.
- `requestReviews` with the reviewer's bot node id in **`userIds`** fails with `Could not resolve to User node`, because the Copilot reviewer is a **Bot**, so its node id goes in **`botIds`** (as in the mutation above), never `userIds`.
- `suggestedActors(capabilities: [CAN_BE_ASSIGNED])` lists `copilot-swe-agent` (the coding agent), not `copilot-pull-request-reviewer`, so do not source the reviewer's bot node id there. Read it from an existing review per step 1 above.
- There is no `removePullRequestFromReviewRequest` mutation, and removing the reviewer to force a fresh pass is unnecessary anyway, since `requestReviews` with `union: true` re-fires the review on the current head.
- There is no `removePullRequestFromReviewRequest` mutation, but removal is not therefore impossible: `requestReviews` **replaces** the reviewer set when `union` is false (the schema describes `union` as "add users to the set rather than replace"), so an empty `botIds` with `union: false` removes the pending request. Reach for it only in the stuck case below, since `union: true` re-fires a review on the current head without it.
- `gh pr view --json reviewRequests` **omits a Bot reviewer entirely**, reporting an empty set while Copilot sits in it. Read the pending set through GraphQL `reviewRequests`, which returns the `Bot` node, because the REST-backed projection makes a pending request read as no request at all.

### Verify Review Covered Current Head

Expand Down Expand Up @@ -141,6 +149,46 @@ This path is only for a **genuinely missing** review, meaning no Copilot review

**A slow review is pending, not missing, so poll with backoff and never escalate on a timeout alone.** Copilot can lag far beyond the usual one-to-three minutes when it has been re-requested many times in quick succession, because it throttles under load, and a re-review landing tens of minutes after the request is normal. A poll that times out is therefore evidence only that the review has not landed *yet*, not that Copilot is done or unresponsive. Report the status as "review still pending" and keep polling on a widening interval (for example 20s steps, then a few minutes) rather than stopping. Enter the escalation step below only when the `requestReviews` mutation itself no-ops or errors, or after a genuinely long wait with the request confirmed accepted, never merely because one fixed poll window elapsed.

**Bound each wait, and read what Copilot actually posted before opening another one.** A poll that widens forever is indistinguishable from a poll that has stopped, and "still pending" is the honest report for exactly as long as evidence supports it. Two readings decide whether waiting again is warranted. Compare the request's timestamp against the newest Copilot activity of **any** kind on the pull request, since a reviewer that has already answered on a later head, or that posted an issue comment instead of a formal review, is not a reviewer running late, and a wait that keeps reporting "pending" against a landed review is a broken wait rather than a slow reviewer. Then read that newest response, because a Copilot answer naming a quota or a rate limit is a **terminal** outcome rather than a pending one: no formal review will land, so path (1) never matches the head and path (2) is correctly never confirmed, both paths behave exactly as specified, and the agent waits for something that is not coming. The fix is account-side and re-requesting does not change it, so report it to the maintainer and stop waiting. Where the newest response is neither a review nor a refusal you recognize, that too goes to the maintainer with its text, rather than being waited through.

**A pending request nothing picked up is a third state, and it is the one that looks most like patience.** Copilot raises a `copilot_work_started` timeline event within about half a minute of accepting a request, and submits its review a few minutes later. A request that never draws one is not a slow review, it is a request nothing is acting on, and it stays that way indefinitely: one sat for thirteen and a half hours while the pull request read as waiting on the reviewer. Elapsed time cannot tell the two apart, since a genuinely slow round also shows no review, so read the event rather than the clock. `copilot_work_started` appears in the REST timeline only, and no GraphQL timeline item carries it:

```sh
# The pending set (GraphQL, since the `gh pr view` projection cannot see a Bot reviewer).
gh api graphql -f query='
{ repository(owner:"<owner>",name:"<repo>"){ pullRequest(number:<N>){
reviewRequests(first:10){ totalCount
nodes{ requestedReviewer{ __typename ... on Bot{login} ... on User{login} } } } } } }'

# The request and pickup events, newest last. A `review_requested` with no later
# `copilot_work_started` is the stuck state. Requests are filtered to the reviewer's own,
# since a human requested afterwards is a different request and reading it as this one
# reports a picked-up review as never picked up. `per_page` is the pagination cost.
gh api --paginate 'repos/<owner>/<repo>/issues/<N>/timeline?per_page=100' \
--jq '.[] | select(.event == "copilot_work_started" or (.event == "review_requested"
and .requested_reviewer.type == "Bot"
and ((.requested_reviewer.login // "") | ascii_downcase | test("copilot"))))
| "\(.event) \(.created_at)"'
```

**Recover it by clearing the request and requesting again**, because the pull request UI offers no re-request control while a request is pending, and `requestReviews` with `union: true` adds a reviewer already in the set, which changes nothing. Read the pending set first, since `union: false` replaces the whole set and would drop a human reviewer requested alongside the bot. Where the clear-and-request does not draw a `copilot_work_started` within a minute or so, push a commit instead, since a new head raises a fresh request rather than poking a stale one.

```sh
PR_NODE=$(gh pr view <N> --json id --jq '.id')
# 1. Clear. `union: false` replaces the set, so an empty botIds removes the pending request.
gh api graphql -f query='
mutation($pr: ID!) {
requestReviews(input: { pullRequestId: $pr, botIds: [], union: false }) {
pullRequest { reviewRequests(first: 10) { totalCount } } }
}' -F pr="$PR_NODE"
# 2. Request again, against a now-empty set, with $BOT_ID read as in "Triggering and Polling".
gh api graphql -f query='
mutation($pr: ID!, $bot: ID!) {
requestReviews(input: { pullRequestId: $pr, botIds: [$bot], union: true }) {
pullRequest { reviewRequests(first: 10) { totalCount } } }
}' -F pr="$PR_NODE" -F bot="$BOT_ID"
```

If a review did not run on the current head, retry:

1. Wait briefly and check head-SHA coverage (see above).
Expand Down
3 changes: 2 additions & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ An agent session is billed on the context it carries, not the work it does. Ever
### Session Scope

- **One deliverable, one session.** A session covers one branch and one deliverable, and ends when that work merges. A multi-step task is one deliverable and stays in one session. Two unrelated tasks are two sessions even when they run back to back.
- **End a session at any of these, without being asked:** the branch changes, the pull request merges, the next task is unrelated to the last, or a third review round opens on the same pull request.
- **End a session at any of these, without being asked:** the branch changes, the pull request merges, or the next task is unrelated to the last. A review round is none of them. A loop still producing findings is the deliverable in progress, and a round count is not a reason to leave one open.
- **Hand off in a file, never in context.** Close a session by writing at most 2 KB to a scratch file: branch, pull request link, what is done, the next command. A summary held in context is re-billed until the session ends, and a summary on disk is read once by whoever needs it.
- **Re-derive state, do not carry it.** "This session already has the context" is the signal to split, not to continue. Context that has gone stale is worse than absent, because a file read hundreds of requests ago no longer describes the file.
- **Compaction is a fallback, not the strategy.** It restarts context from a floor and climbs again, where a fresh session starts from zero.
Expand Down Expand Up @@ -45,6 +45,7 @@ If a rule you were given does not cover what you find, stop and report it. Do no
```

- **Wait in a background process, not in a poll loop.** A review or CI wait is a sequence of near-identical requests, each billed for whatever context it happens to carry. Run the wait as one backgrounded command that returns when the condition is met.
- **A wait separates three outcomes, and says which one it reached.** The condition was met, it has not been met yet, and the wait cannot reach it at all are three different results, and a backgrounded wait that emits nothing renders all three identically. Run the command once in the foreground and read its output before backgrounding it, because a wait is only as good as the command inside it, and an unsupported flag on the installed tool version exits non-zero with an empty stdout that every naive test reads as "nothing yet". Never let a fallback stand in for a failed command, since `|| echo '[]'`, `|| true`, and `2>/dev/null` convert an error into that same reading, which is the suppression the write-safety rules already forbid on a mutation. Make the wait emit on failure as loudly as on success, so silence means "still running" and nothing else, and bound it, so a condition that is never coming ends in a report rather than in another wait.

## Where the Rules Live

Expand Down
Loading
Loading