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
6 changes: 6 additions & 0 deletions .github/copilot-instructions.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ Auto-review on push is configured (via the branch ruleset's `copilot_code_review

**A review with no inline comments is still a completed review - not a failure, and not a reason to ask the maintainer to re-trigger.** Copilot very often posts a single formal review (GraphQL `state: COMMENTED`) whose body ends with "...reviewed N of N changed files ... and generated no comments" and adds **zero** inline threads. That review carries the head `commit.oid` and fully satisfies the loop - it is the clean-pass success case. Never read "no inline comments" as "the review didn't run," and never re-request or escalate to the maintainer because comments are absent.

**Read the low-confidence findings, which are not inline threads.** A review body can carry a collapsed `<details>` block headed "Comments suppressed due to low confidence", and those findings appear nowhere in `reviewThreads`, so a loop that polls threads alone never sees them and reports a clean pass. They have been right repeatedly, including a rule stated more broadly than its check enforced and a check that skipped fenced blocks in every rule but one. Read the body of every review, investigate each suppressed finding on the same footing as an inline one, and answer it in the PR conversation, since a suppressed finding has no thread to reply on or resolve.

```sh
gh api repos/<owner>/<repo>/pulls/<N>/reviews --jq '.[] | select(.body | contains("low confidence")) | .body'
```

**Round 1 is normally auto-seeded - 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` - **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; match the API, not a single spelling, when adapting them.
Expand Down
18 changes: 13 additions & 5 deletions .github/workflows/validate-task.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@ jobs:
with:
globs: '**/*.md'

# cspell gate = README + HISTORY only; all-*.md would mean endlessly padding cspell.json for technical terms
# (broad live spell-check is the editor extension's job). See CODESTYLE.md "Markdown and Spelling".
# The cspell gate covers README + HISTORY only.
# Gating all *.md would mean endlessly padding cspell.json for technical terms.
# Broad live spell-check is the editor extension's job.
# See CODESTYLE.md "Markdown and Spelling".
- name: Spell check step
uses: streetsidesoftware/cspell-action@de2a73e963e7443969755b648a1008f77033c5b2 # v8.4.0
with:
Expand Down Expand Up @@ -60,7 +62,13 @@ jobs:
- name: Check repo gates step
run: python3 scripts/repo_gate.py

# The charset and duplicate-word rules are clean tree-wide, so they gate. The semicolon
# rule stays warn-only: going green needs the sweep the prose rule itself forbids.
# The charset and duplicate-word rules are clean tree-wide, so they gate.
# Every other prose rule reports in the step below without gating.
- name: Check prose step
run: python3 scripts/prose_lint.py . --check ascii --check dupword
run: python3 scripts/prose_lint.py . --check charset --check dupword

# Warn-only, and visible rather than absent: an unrun check is one nobody acts on.
# The backlog is corrected as each file is next edited, never swept.
- name: Report prose backlog step
continue-on-error: true
run: python3 scripts/prose_lint.py . --check charset-unknown --check semicolon --check dash --check comment-wrap --check comment-case --summary
9 changes: 6 additions & 3 deletions .markdownlint-cli2.jsonc
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
{
"config": {
// Prose paragraphs and data-heavy tables/URLs are intentionally long.
// Reflowing at 80 cols hurts readability and churns diffs.
// Prose paragraphs and data-heavy tables or URLs are intentionally long.
// Reflowing at 80 columns hurts readability and churns diffs.
"MD013": false,
// MD033 (inline HTML) stays enabled so native markdown wins - HTML comments (reference-link dividers) pass it, and details/summary are allowed for GitHub collapsibles, which have no markdown equivalent. Every other element still flags.
// MD033 (inline HTML) stays enabled so native markdown wins.
// HTML comments, used as reference-link dividers, pass it.
// The details and summary elements are allowed for GitHub collapsibles, which have no markdown equivalent.
// Every other element still flags.
"MD033": { "allowed_elements": ["details", "summary"] },
// Require fenced code blocks over the legacy 4-space-indented style.
"MD046": { "style": "fenced" },
Expand Down
30 changes: 19 additions & 11 deletions GOVERNANCE.md
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ Applies to code and workflow (`#`) comments alike.
- Write for the human reading *this* project's code now: state only the non-obvious *why*. No cross-project references (do not name other repos), no historic or design narrative, no rule citations - governance lives in this file, not echoed inline.
- **Keep it short.** One line is the default. A comment earns a second line only by carrying a constraint the code cannot. Most comments are one sentence, and never restate *what* the code does - a well-named symbol already says it.
- **Structured, not prose: one sentence per line, and never wrap a sentence across lines.** No block paragraphs and no multi-sentence run-ons. A comment that genuinely needs several sentences is several lines, each a single sentence. A sentence too long for one sensible line is too long - split the thought.
- **A comment line that opens prose starts with a capital.** A line opening in lowercase reads as the continuation of the one above it, so a sentence that genuinely starts there is capitalized. A trailing fragment that annotates the code on its line is a label rather than a sentence, and the version pin the action-pinning rule requires is one, so neither takes a capital. Where the first word is a tool or identifier whose own casing is lowercase, restructure so the sentence does not open on it rather than capitalizing the name against its official spelling.
- **A multi-line comment shows whether it is a continuation or a list.** A continuation of the same topic stays unindented, one sentence per line. Mark a sub-topic with a `-` after the comment marker (`# -`, `// -`), and only for genuine sub-topics - parallel items hanging off a lead line, never a continuation of one thought.
- **No class-, type-, or file-header summary comment blocks.** A type or file gets a comment only for a specific non-obvious point, kept terse - never a block summarizing what the file contains or what the class is for. A summary restates the declaration below it, goes stale as the file grows, and is the file-scope form of the design narrative and verbosity creep this section already bans. A license or provenance header a tool or policy requires is not a summary and is unaffected.
- **Do not grow a comment across edits.** When you touch code near an existing comment, the comment must come out **same length or shorter** - never append "one more clause" of rationale. If a block comment has crept to multiple sentences of prose, cut it back to its single load-bearing point as part of your change. Verbosity creep is the specific regression to prevent: every iteration that adds a clause is a regression, not an improvement.
Expand All @@ -171,15 +172,20 @@ Sub-topics take a `-` after the comment marker, each elaborating a distinct item

### Character Set

- **Write ASCII in all agent-authored text** - documentation, code, comments, commit messages, and PR descriptions. The agent does not introduce non-ASCII characters. Replace typographic Unicode with its ASCII equivalent on sight:
- em dash (U+2014) and en dash (U+2013) -> hyphen `-` (use a spaced ` - ` for an em-dash-style clause break)
- right arrow (U+2192) -> `->`; double arrow (U+21D2) -> `=>`
- less-than-or-equal (U+2264) -> `<=`; greater-than-or-equal (U+2265) -> `>=`
- curly quotes (U+2018/U+2019/U+201C/U+201D) -> straight `'` and `"`; ellipsis (U+2026) -> `...`
- **No semicolon joining two independent clauses in agent-authored prose** - documentation, comments, commit messages, and PR descriptions. Recast as a comma or as two sentences: "the check runs on push; it gates the merge" becomes "the check runs on push and gates the merge", or two sentences. A semicolon separating items in a list that already contains commas keeps its standard use, and a statement terminator in **code** is untouched by this rule. This bans the semicolon splice only - a colon introducing an explanation, elaboration, or list keeps its standard use and is not a splice. Existing prose is corrected as each file is next edited, not swept.
- **Allowed non-ASCII (two narrow exceptions):**
- **Scientific or technical symbols with no clean ASCII equivalent** - e.g. ohm, micro, degree, pi. Keep the symbol; do not approximate it away.
- **Unicode the developer deliberately typed** - emoji used for emphasis or as callout markers (for example the warning/info markers a maintainer placed in `README.md`). Preserve it; never strip the developer's own characters. This carve-out is for developer-authored text, not a license for the agent to add emoji.
Agent-authored text is ASCII by default: documentation, code, comments, commit messages, and PR descriptions. A non-ASCII character is read against three tiers, because whether one is typography or meaning depends on where it sits. A character in no tier is a finding rather than a silent pass.

- **Tier 1, never legitimate.** Typography carrying no meaning its ASCII form loses. Remove on sight:
- em dash (U+2014) and en dash (U+2013) -> **restructure the sentence**. Two sentences, or a comma. Do not substitute a spaced hyphen.
- right arrow (U+2192) -> `->`, double arrow (U+21D2) -> `=>`
- curly quotes (U+2018/U+2019/U+201C/U+201D) -> straight `'` and `"`
- ellipsis (U+2026) -> `...`, bullet (U+2022) -> `-`
- no-break space (U+00A0) -> a space, non-breaking hyphen (U+2011) -> `-`
- **Tier 2, legitimate only next to a number.** Relational and arithmetic operators: U+2264, U+2265, U+2260, U+00B1, U+2212, U+00D7, U+00F7, U+00B7. Keep one when an adjacent non-space token is a number, a tier-3 symbol, or another tier-2 operator, so a threshold table or a measured range reads as the range it is. In flowing prose write the ASCII form: `<=`, `>=`, `!=`, `+/-`, `-`, `x`, `/`. A U+2264 directly before `35` in a table of sensor thresholds is the range it describes and stays. The same character between two words, as in a sentence about one check running before another, is prose and takes `<=`.
- **Tier 3, always legitimate.** Scientific and unit symbols whose ASCII form would be a lie: micro (U+00B5), degree (U+00B0), ohm (U+2126), pi (U+03C0), superscript two and three (U+00B2, U+00B3), section (U+00A7). Keep the symbol. Do not approximate it away, and do not spell it out.
- **Unicode the developer deliberately typed** stays regardless of tier, such as emoji used for emphasis or as callout markers, for example the warning markers a maintainer placed in `README.md`. Never strip the developer's own characters. The carve-out governs what an agent may rewrite rather than what the gate reports, so an un-tiered character of this kind is still a `charset-unknown` finding until someone classifies it. It covers developer-authored text, and is not a license for the agent to add emoji.
- **An unrecognized non-ASCII character is reported, not allowed.** Classify it into a tier above before using it. A gate that passes whatever it does not recognize stops gating as the character set grows, which is the silent-narrowing failure named under "Verification Discipline".
- **No spaced hyphen joining or interrupting a sentence.** The em-dash-style clause break ` - `, and the paired aside ` - x - `, are both recast: a comma where the clauses are short and closely linked, two sentences where they are not, or parentheses for a genuine aside. This is the same construction the tier-1 em dash is restructured into, so allowing its ASCII spelling would keep the shape and only change the character. A hyphen inside a compound word, a leading list marker, a range, and the `- **Label** - explanation` separator that opens a governed bullet keep their standard use: the last is structurally a colon, and flagging it would restructure the document format rather than the prose. Existing prose is corrected as each file is next edited, not swept.
- **No semicolon in agent-authored prose.** A mid-sentence semicolon joining clauses is recast as a comma or as two sentences. A semicolon separating items in a list that already contains commas keeps its standard use, and a statement terminator in **code** is untouched. A colon introducing an explanation, elaboration, or list is not a semicolon and is unaffected. Existing prose is corrected as each file is next edited, not swept.

### Line Endings

Expand All @@ -203,10 +209,12 @@ Sub-topics take a `-` after the comment marker, each elaborating a distinct item

The checks that separate work actually done from work that merely reports success. Their unifying property: **every failure below is green.** A skipped job and a passing job are indistinguishable in the aggregated required check, a pattern that matches less still exits zero, and a gate that stops gating still reports success. No linter, status check, or review layer catches any of them.

- **A test must assert the mechanism it names.** Label each case by the behavior it proves, and satisfy yourself it would fail if that mechanism broke. A case that passes for an incidental reason - the right answer reached by the wrong path - is worse than no case, because it is later cited as evidence.
- **A test must assert the mechanism it names, and a gate has to be watched failing.** Label each case by the behavior it proves, then write the case that reintroduces the fault and confirm the gate objects to it. A case that passes for an incidental reason, the right answer reached by the wrong path, is worse than no case, because it is later cited as evidence. A proof that restates the gated data instead of reading it proves only that the function works, so drive the real table or the real config. And a gate that finds nothing is indistinguishable from a gate with nothing to find, so assert a floor on what a healthy run covers.
- **Gates, filters, and gate-like watchers fail loud, never narrow quietly.** A pattern that silently matches less, an allowlist that silently stops matching, or a gate that silently stops gating all report success while doing nothing. When a construct exists to notice something, make the not-noticing case produce an error or an annotation. An identity allowlist used as a gate, for one, must raise an error when its list stops matching, not silently pass everything through.
- **Run the repo's whole lint gate before every push, not the parts that look relevant.** CI runs all of them, so a partial local run only defers the failure - and the tool most likely to catch a given change is often the one it seems least about (an edit that manipulates line endings is exactly when `editorconfig-checker` matters). The repo documents each linter's known-working invocation - this rule is that **all** of them run.
- **Editing CRLF files programmatically: `.` matches `\r` in a regex**, so a captured line keeps its carriage return and rejoining with `\r\n` yields `CRCRLF`. A text-mode rewrite has the mirror failure, silently flattening CRLF to LF. Prefer line-based edits (`splitlines(keepends=True)`) or literal replacement over regex reassembly. This is the mechanism behind the Line Endings warning above, and it is worth naming because the corruption is invisible in a rendered diff.
- **Scope a check by what the project declares, not by the file that prompted it.** A check written while editing one file tends to cover that file's language and stop, and then reports success on every other surface the rule governs. Read the declared types, or the config that enumerates them, and cover each one, then assert a floor per surface so a table that narrows fails loudly instead of passing quietly. A rule about comments means every comment syntax the project ships, and a format that carries comments in practice counts even where its specification says otherwise.
- **Never edit source through a shell heredoc when the text carries backslash escapes.** The shell consumes the escape and writes an invisible control character in its place, so a `\b` inside a regex becomes a backspace and the pattern silently matches nothing while every test still passes. Use a file-editing tool for such text. When a check inspects text for control characters, use `str.isprintable()` rather than a codepoint floor, since DEL and the Unicode format characters sit above 32 and are equally invisible in a diff.
- **Never edit an active `.code-workspace` file.** A workspace file rewritten on disk can make VS Code reload the window, and a reload destroys the running agent session's context - the work in flight is lost with nothing to catch it, and the trigger is not fully characterized (an agent's edit has caused the reload where a human's identical edit did not). Surface the needed change for the maintainer to apply by hand.
- **A green check is not evidence the work happened.** A skipped job and a passing job are indistinguishable in the aggregated required check. When a job exists to exercise something, confirm from its log that it ran and produced the output it promises.
- **A workflow change is only fully exercised by CI.** Extracting a `run:` block and executing it locally validates the script and nothing else - `secrets: inherit`, `permissions:`, `needs:` wiring, and reusable-workflow inputs resolve only in a real run.
Expand All @@ -224,7 +232,7 @@ The repo runs a review loop on every PR: local agent iteration plus remote autom

1. Required status checks are green (`mergeStateStatus: CLEAN`), **and**
2. A Copilot review is confirmed on the **current head SHA** by matching the review's commit SHA to the head, not an earlier push - a push makes required checks go green **before** the re-review lands, so a green merge-state can precede the current-head review and never signals readiness on its own, **and**
3. **Every** Copilot finding on that head SHA is closed out - all review threads resolved, **and** any issue-level Copilot comments (which have no resolve action) triaged and replied to - so zero outstanding findings remain, **and**
3. **Every** Copilot finding on that head SHA is closed out - all review threads resolved, **and** any issue-level Copilot comments (which have no resolve action) triaged and replied to, **and** the low-confidence findings collapsed in the review body investigated and answered, since those appear in no thread and a loop that polls threads alone reports a clean pass while they stand - so zero outstanding findings remain, **and**
4. The maintainer has given **explicit** permission to merge.

`mergeStateStatus: CLEAN` reflects **only** required statuses - it never reflects open bot review comments, so `CLEAN` alone is **never** sufficient to merge. A green/`CLEAN` PR with an unresolved Copilot finding fails this gate; treat it as "not mergeable" no matter what the merge-state field says. The agent never merges on its own (consistent with "default to staging"; merging is maintainer-authorized).
Expand Down
Loading
Loading