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
13 changes: 8 additions & 5 deletions .github/copilot-instructions.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ For every review:
4. Use an inline comment when a changed line can anchor the finding. Use the review body only when no valid inline anchor exists.
5. End the review body with the exact machine-readable marker required by the `fleet-code-review` skill.

The review automation is `scripts/pr_review.py`, run from a hub checkout. Use its `status`, `wait`, `comment`, and `reply --resolve` commands instead of reconstructing GraphQL queries or copying review identifiers by hand. Use `comment` for a suppressed-finding answer in the pull request conversation. Its status gate verifies the current head, diff coverage, output shape, inline threads, body-only findings, and required checks.
The review automation is `scripts/pr_review.py`, run from a hub checkout. Use its `status`, `wait`, `attest`, `comment`, and `reply --resolve` commands instead of reconstructing GraphQL queries or copying review identifiers by hand. Use `comment` for a suppressed-finding answer in the pull request conversation. Its status gate verifies the current head, diff coverage, output shape, inline threads, body-only findings, and required checks.

A formal review with no findings is complete only when it covers the current head and full diff coverage of the change set that head has is stated, or read from a file table as below. The round covering the head states it, or the newest round that states it at all does and the pull request changes the same set of files at both commits, which is the only condition under which a statement carries forward. Only that newest round is consulted, so an older round whose change set does match carries nothing. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. A round reporting partial coverage of the diff blocks the merge, and so does a refusal, a coverage statement that does not reach this head, meaning absent from every round or carried by none because the change set moved or could not be compared, with no file table standing in for it, an unrecognized output shape, an unresolved thread, or a body-only finding. Re-run the loop after every fix push. Never infer review completion from `mergeStateStatus: CLEAN`.
A formal review with no findings is complete only when it covers the current head and full diff coverage of the change set that head has is stated, or read from a file table as below. A round covering the head states it. Where none does, a round covering the head whose own file table names exactly the changed files covers it, which is the reading a round at Balanced review effort gives. Where no round covering the head carries a table of its own, the newest round that does stands in where its table names exactly the changed files. A table stands in only where no Copilot round on the pull request, on any commit, states partial coverage, so a pull request that ever had a partial round goes to the maintainer. A round reporting partial coverage of the diff blocks the merge, and so does a refusal, a head that no coverage statement, file table, or attested local pass covers, an output shape the digest notes and nobody has read by hand, an unresolved thread, or a body-only finding. On a pull request into a branch other than the default, an attested local pass covers the head, `coverage=local` in the digest. A partial round, a truncated review history, or a refusal on the head still blocks that reading. A pull request into the default branch, a promotion among them, always carries a Copilot round on its head. Re-run the loop after every fix push. Never infer review completion from `mergeStateStatus: CLEAN`.

Review effort is user-controlled. The automation observes `Lite`, `Balanced`, or `Max`, including an inherited `Default (<level>)`, and never selects or changes the setting. Effort does not determine coverage or completion. A request can complete without a `copilot_work_started` event, so absence of that event is not a stalled-review verdict. When `wait` returns `PENDING` with `requested=yes`, report the state and rerun `wait` for another bounded interval by default, reading that field as acceptance of the request rather than as delivery of a round. Do not clear the request on that first timeout, because it may still be active. Where a second bounded wait times out as well, read the pending set, clear it only where no human or team reviewer is requested alongside the bot, and rerun `wait`, which then has nothing outstanding to defer to and requests afresh, or polls and says so on its own auto-request line where it finds no reviewer node id to request with. The clear replaces that set rather than adding to it and nothing restores a request it drops, so a stall on a pull request that has a human or team reviewer requested goes to the maintainer, and so does one still pending after the wait that follows a clear. That clear is a recovery step the script does not implement, and `docs/pr-reviewer-reference.md`, in the hub checkout the script is run from, carries it. This recovery replaces only the review request and never changes the effort setting. A `wait` ending `REQUEST_NOT_RECORDED`, exit 48, is a different state and takes none of this recovery: the request returned success and left neither a pending reviewer nor a review-request event, which is how an exhausted Copilot allowance has shown itself, so clearing and requesting again does not clear it and it goes to the maintainer.
Review effort is user-controlled. The automation observes `Lite`, `Balanced`, or `Max`, including an inherited `Default (<level>)`, and never selects or changes the setting. Effort does not determine coverage or completion. A request can complete without a `copilot_work_started` event, so absence of that event is not a stalled-review verdict. When `wait` returns `PENDING` with `requested=yes`, report the state and rerun `wait` for another bounded interval by default, reading that field as acceptance of the request rather than as delivery of a round. Do not clear the request on that first timeout, because it may still be active. Where a second bounded wait times out as well, read the pending set, clear it only where no human or team reviewer is requested alongside the bot, and rerun `wait`, which then has nothing outstanding to defer to and requests afresh, or polls and says so on its own auto-request line where it finds no reviewer node id to request with. The clear replaces that set rather than adding to it and nothing restores a request it drops, so a stall on a pull request that has a human or team reviewer requested goes to the maintainer, and so does one still pending after the wait that follows a clear. That clear is a recovery step the script does not implement, and `docs/pr-reviewer-reference.md`, in the hub checkout the script is run from, carries it. This recovery replaces only the review request and never changes the effort setting. A `wait` ending `REQUEST_NOT_RECORDED`, exit 40, is a different state and takes none of this recovery: the request returned success and left neither a pending reviewer nor a review-request event, which is how an exhausted Copilot allowance has shown itself, so clearing and requesting again does not clear it and it goes to the maintainer.

### Disproved Claims

Expand All @@ -40,9 +40,12 @@ Review effort is user-controlled. The automation observes `Lite`, `Balanced`, or

**The record answers a repeated claim and never dismisses a new one.** An entry is cited only where the revision it names is still what the tree carries, and the reply carries the proof re-read rather than a pointer to the entry, since a reviewer that cannot open this file learns nothing from being pointed at it. Judge a finding on its merits first and match it against this record second, because reading it the other way round is how a real finding gets closed by a stale proof.

**The entries are this repository's own.** Each names a file and a revision, so a repository holding a copy of this file carries the shape and the rules above rather than these findings, deletes an entry whose subject it does not carry, and records what it has proved itself.
**The entries are this repository's own.** Each names a file and a revision. A repository holding a copy of this file therefore carries the shape and the rules above rather than these findings. That repository deletes an entry whose subject it does not carry and records what it has proved itself. An entry is never about hub-hosted machinery, the tooling a repository reaches rather than carries per [GOVERNANCE.md "Hub-Hosted Tooling"](../GOVERNANCE.md#hub-hosted-tooling). A repository without that tooling has nothing to re-check such a claim against, so a constraint on that tooling is recorded in that tooling's code.

No entries yet. Add one here only after disproving a claim against this repository's own tree at a named revision.
- **Ruff refuses a test's import that follows `sys.path.insert`, as E402, so the Python gate fails until each such import carries a suppression.** Raised by Copilot as a P1 on two renamed test files, and the pattern recurs in every test that imports the script it tests.
- **Disproved by** - running the gate CI runs, `uvx ruff@latest check .` from inside `checks/` and from inside `scripts/`. Both print `All checks passed!`. Each `pyproject.toml` adds only `extend-select = ["I"]` to ruff's default rule set. The rule list `ruff check --show-settings` prints holds no E402. The same run with `--select E402` reports the imports, so the pattern is present and the rule is off.
- **Proved against** - ruff 0.17.2, and `checks/pyproject.toml`, `scripts/pyproject.toml`, and the tests under `checks/tests/` and `scripts/tests/` on `develop` at `b18f7cc`.
- **Delete when** - either `pyproject.toml` selects E402 or a rule set holding it, or a ruff release adds E402 to its default rule set. Tests that stop importing after `sys.path.insert` end it too.

## When in Doubt

Expand Down
Loading
Loading