Ask for a blocked decision instead of reporting it - #561
Conversation
"Communicating with the User" required a numbered list for a question and said nothing about where the question goes. A summary is the wrong place: it reads as a report of finished work, so the one line still waiting on the user is the easiest in it to skim past, and the work sits owned by neither side while both believe the other has it. The new bullet requires a direct interactive prompt with selectable options, raised where the work stops rather than folded into a closing paragraph. The numbered list stays the fallback for an interface that offers no prompt, so the existing rule is narrowed rather than replaced. "Communicating with the User" is verbatim fleet law, so the fleet carries an older copy of the section until it is re-vendored. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Ready to approve
The change is a narrowly scoped governance clarification that is internally consistent with the surrounding rules and introduces no behavioral or tooling risk.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR updates GOVERNANCE.md to ensure that work blocked on a maintainer-only decision is surfaced as an explicit, interface-level prompt (with selectable options when supported) at the moment progress stops, rather than being buried as prose in a summary where it can be overlooked.
Changes:
- Add a new "Communicating with the User" rule requiring blocked decisions/authorizations to be raised as a direct interactive prompt (with a numbered-list fallback when no prompt UI exists).
File summaries
| File | Description |
|---|---|
| GOVERNANCE.md | Adds a governance bullet clarifying how agents must surface maintainer-blocked decisions (prompt-first, numbered-list fallback). |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The rule depends on develop-versus-default-branch mechanics, which "Branching Model" states and "Release Model" does not. An agent writing a feature PR body reaches for the branch section or the PR-title section, and the rule sat in neither, so the words are unchanged and only the home moves: it follows the promotion-execution traps, beside the other develop-to-main mechanics. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Ready to approve
The changes are narrowly scoped governance clarifications with no apparent formatting or consistency issues in the updated Markdown.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The rule required an interactive prompt and left the options unspecified, which a prompt offering only ways to wait satisfies while clearing nothing: the user is asked to confirm that the block exists rather than to lift it, and the work sits exactly where prose left it. The options are now the actions themselves, with the unblocking one naming what it authorizes, so selecting it is the go-ahead. Where the agent may not perform that action itself, the option says who does. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Ready to approve
The change is limited to governance documentation updates with consistent formatting and no functional code impact.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The clickable-link rule named one syntax, and markdown link syntax is clickable only where markdown renders. On a surface that shows it as literal text, an interactive prompt's question and option text among them, following the rule as written produces a reference the reader has to retype, which is the outcome the rule exists to prevent. The form now follows the surface: the full URL where markdown does not render, and the test is whether it can be clicked where it is read. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Ready to approve
The changes are governance-only and the remaining feedback is minor wording/format consistency.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
GOVERNANCE.md:317
- This rule mixes "markdown" and "Markdown" when referring to the markup language. Since it is a proper name and the sentence already uses "Markdown link syntax", the earlier/later occurrences should be capitalized for consistency.
- **Reference every pull request as a clickable link.** When you mention a PR (in chat, a summary, or a report), render it as a markdown link to the PR (`[#123](https://github.com/OWNER/REPO/pull/123)`), never a bare `#123`. The same applies to issues and commits. **The form follows the surface.** Markdown link syntax is clickable only where markdown renders, so on a surface that shows it as literal text (an interactive prompt's question and option text, a plain terminal line), write the full URL on its own, which the surface itself makes clickable. The test is whether the reader can click it where it is read, not whether it was written in the syntax that works elsewhere, since a reference the reader has to retype is not a link.
GOVERNANCE.md:58
- In this section, branch names are consistently formatted as code (e.g.
develop,main), but this bullet uses bare "develop" in a few places ("feature -> develop PR", "develop PR body"). This makes the formatting inconsistent and slightly harder to skim as branch-vs-word.
This issue also appears on line 317 of the same file.
- **Issue-closing keywords (`Closes #N`, `Fixes #N`) go in the `develop -> main` promotion PR, not the feature -> develop PR.** GitHub auto-closes an issue only when the closing keyword merges into the **default branch** (`main`). A feature/develop PR merges into `develop`, so the keyword never fires there. Reference the issue in the develop PR body if useful, but put the actual closing keyword on the promotion PR. If a develop PR merges with the keyword on it, the keyword does nothing and the issue stays open, so put it on the promotion PR body instead. Close the issue by hand citing the squash SHA only when the promotion has already merged without it.
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Two formatting inconsistencies the review round surfaced, in text this
pull request already touches.
The closing-keyword rule names branches bare ("the feature -> develop
PR", "the develop PR body") while the section around its new home writes
every branch as code. It arrived here byte-identical so the move stayed
readable as a move, and the formatting is its own commit for the same
reason.
The clickable-link rule opened a sentence with "Markdown" while the
bullet it extends writes "a markdown link" three words earlier. Prose in
this file names the markup lowercase and reserves the capital for the
extension and the linter, so the new sentence follows the bullet.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Answering the two suppressed comments from the round on
Accepted, resolved toward lowercase rather than up. The bullet writes "a markdown link" three words earlier, and prose in this file names the markup lowercase throughout ("every markdown file", "stripping markdown links"), reserving the capital for the Markdown All in One extension and for markdownlint's own subject line. The new sentence now opens "A markdown link is clickable only where markdown renders".
Accepted, and worth saying why it was there. That bullet is pre-existing law relocated by this pull request from "Release Model", and it was moved byte-identical so the diff reads as a move rather than as a rewrite. The formatting is its own commit for the same reason: The second finding also reports "This issue also appears on line 317 of the same file". Line 317 is the clickable-link bullet, which names no branch, so nothing was changed there on that account. |
There was a problem hiding this comment.
🟡 Changes recommended
The PR description claims the moved issue-closing rule is byte-identical/unchanged, but the diff shows wording changes that should be reconciled (either restore identical text or update the description).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
GOVERNANCE.md:58
- The PR description says the issue-closing-keyword rule move is "byte-identical" and "unchanged", but the moved bullet’s wording was modified (e.g.,
feature/develop->feature -> \develop``, and branch-name backticks were added/removed). If the intent is a pure move, keep the text exactly identical to the original line; otherwise, update the PR description to reflect that the rule text changed.
- **Issue-closing keywords (`Closes #N`, `Fixes #N`) go in the `develop -> main` promotion PR, not the feature -> `develop` PR.** GitHub auto-closes an issue only when the closing keyword merges into the **default branch** (`main`). A feature -> `develop` PR merges into `develop`, so the keyword never fires there. Reference the issue in the `develop` PR body if useful, but put the actual closing keyword on the promotion PR. If a `develop` PR merges with the keyword on it, the keyword does nothing and the issue stays open, so put it on the promotion PR body instead. Close the issue by hand citing the squash SHA only when the promotion has already merged without it.
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Answering the suppressed comment from the round on
Accepted, and the description was the stale half. The claim was written for The text is deliberately not restored to identical. The formatting inconsistency was reported against this same line one round earlier and accepted, so reverting it to satisfy the description would trade a correct fix for a stale sentence. |
There was a problem hiding this comment.
🟢 Ready to approve
The changes are documentation-only governance refinements and appear internally consistent with existing section conventions.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The previous wording assumed a surface that shows markdown as literal text still links a bare URL. An interactive prompt does neither: the URL renders as plain text the reader has to select and copy, which is the outcome the clickable-link rule exists to prevent, so following the rule as written produced the failure it forbids. The link now goes in the message that accompanies the prompt, where markdown renders, and the prompt names the object plainly. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The updated “clickable PR link” bullet is internally contradictory (“never a bare #123” vs later allowing #123 in prompt text) and should be made explicitly surface-dependent for clarity.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
GOVERNANCE.md:317
- This bullet says to "never" use a bare
#123, but later instructs using#123in interactive prompt text. That creates an internal contradiction; consider making the rule explicitly surface-dependent so the exception is not framed as a violation.
- **Reference every pull request as a clickable link.** When you mention a PR (in chat, a summary, or a report), render it as a markdown link to the PR (`[#123](https://github.com/OWNER/REPO/pull/123)`), never a bare `#123`. The same applies to issues and commits. **The form follows the surface.** A markdown link is clickable only where markdown renders, and some surfaces link neither the markdown nor a bare URL, an interactive prompt's question and option text among them. Pasting a full URL into one of those does not rescue it, since the reader gets a string to copy, which is the outcome this rule exists to prevent. Put the clickable link in the message that accompanies the prompt, where markdown does render, and let the prompt name the object plainly (`#123`). The test is whether the reader can click it where it is read, not whether it was written in the syntax that works elsewhere.
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The bullet banned a bare reference outright and then required one in a prompt, so the rule contradicted itself and an agent following the opening sentence would produce the unclickable URL the addition exists to stop. The ban now applies where markdown renders, which is the surface the original sentence assumed, and the prompt case reads as the other half of one rule rather than as an exception to it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Ready to approve
Documentation-only updates look consistent and coherent, with only a minor capitalization/wording nit called out in a comment.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
GOVERNANCE.md:317
- Capitalization/wording: use "Markdown" (proper noun) and clarify "the markdown" to "Markdown link syntax" for readability.
- **Reference every pull request as a clickable link.** When you mention a PR on a surface that renders markdown (chat, a summary, a report), render it as a markdown link to the PR (`[#123](https://github.com/OWNER/REPO/pull/123)`), never a bare `#123`. The same applies to issues and commits. **The form follows the surface.** Some surfaces link neither the markdown nor a bare URL, an interactive prompt's question and option text among them, and pasting a full URL into one of those does not rescue it, since the reader gets a string to copy, which is the outcome this rule exists to prevent. There the reference is a bare `#123`, and the clickable link goes in the message that accompanies the prompt. The test is whether the reader can click it where it is read, not whether it was written in the syntax that works elsewhere.
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
"Neither the markdown nor a bare URL" leans on the sentence before it to say what "the markdown" is. Naming the markdown link makes the pair read without the back-reference. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Answering the suppressed comment from the round on
Wording accepted, fixed in Capitalization declined, because it reverses a finding from the round on |
There was a problem hiding this comment.
🟢 Ready to approve
The changes are documentation-only, internally consistent with the surrounding guidance, and introduce no behavioral or interface-contract risk.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
) Fixes the false clean reported in #562: `scripts/pr_review.py` printed `suppressed=0` over a review body carrying `### Suppressed comments (2)`, so the gate that decides whether a review round is answered reported a clean round over findings no thread will ever surface. ## Cause The reviewer moved the section from its own `<details>` wrapper to a markdown heading nested inside the `Review details` wrapper, and `suppressed_blocks()` missed it twice over: - The primary path matched the heading against the wrapper's `<summary>`, which now reads `Review details`. - The fallback that exists for exactly this case scanned `DETAILS.sub('', body)`, which deletes every `<details>` block, including the one the heading now sits in. ## Fix Each region (every wrapper's contents, plus what is left outside them all) is scanned line by line for the heading, so the section is found as its own wrapper's `<summary>`, as a markdown heading nested inside another wrapper, or bare in the body. Neither old shape is retargeted away, since both appear across the rounds of a single pull request. A block now starts at its own heading, so `finding_count()` reads the heading's own `(N)` rather than the wrapper's, which would have floored two findings to one. ## Verification `python3 -m unittest discover -s scripts` passes (247 tests), with four cases added for the nested shape: that it reports at all, that the count is the heading's own, that both shapes report together in one run, and that the neighbouring file-summary wrapper is still not read as a finding. Replayed over the reviewer's own bodies on the last seventeen pull requests of this repository, the new parse recovers **6 findings across 5 rounds** that previously read as clean (on #546 and #561), and changes no count that was already right. The runbook section in `.github/copilot-instructions.md` gains the same fact, since its rule to match more than one *phrasing* did not say the section also *moves*, and a filter reading a wrapper's `<summary>` reports zero on the nested shape. Related: #562 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Answers the live half of #565, the six suppressed Copilot findings the digest defect fixed in #562 hid. A suppressed finding reaches no thread, so the digest was the only place it could have surfaced, and it reported `suppressed=0` for five rounds across two pull requests. ## The two prose defects Both are in `TODO.md`, both raised on #546 round 4, both still present on `develop`. | Entry | Was | Now | | --- | --- | --- | | downstream surface | `does not fail loudly, it quietly audits itself` | two sentences | | reading a local clone | `answers instead what that clone last saw` | `answers a different question, which is what that clone last saw` | ## The capitalization convention The reviewer asked for the proper noun on one bullet of #561, and that bullet is now internally consistent in lowercase. So the live question is not the line but the file-wide mix: `GOVERNANCE.md` alone carried **17 lowercase against 7 capitalized**, and the same mix is vendored fleet-wide, which is why settling it per file settles it nowhere. **`CODESTYLE.md` "Markdown and Spelling" gains item 5.** Prose capitalizes the format's name. Lowercase is for the strings a machine reads and nothing else: a tool or package name (`markdownlint`, `yzhang.markdown-all-in-one`), a settings key (`markdown.extension.toc.levels`), a heading anchor (`#markdown-and-spelling`), an identifier in code, and a file extension. What it settles is the mix rather than either spelling, since a file carrying both gives the next author no default and a reviewer a finding to raise on whichever one it wrote last. It lands in `CODESTYLE.md` because every repo carries that file, so the convention arrives with it rather than being re-decided per repo. **The sweep corrected 40 occurrences across 18 files** in documents, code comments, docstrings, and two `spec/audit.py` output strings. No identifier changed: the pattern skips a word adjacent to `.`, `-`, `_`, `[`, `#`, or a paren, so `heading_texts(markdown)` and `markdown.splitlines()` were left alone, and the result was read line by line before applying. ## For the reviewer Four `verbatim` regions changed, so every downstream repo is byte-mismatched until re-vendored: - Three `GOVERNANCE.md` sections by one word each, "Documentation Style Conventions", "Communicating with the User", and "Repository Details". - Two comment lines in `.markdownlint-cli2.jsonc`, which is `verbatim` and `whole`, so a config nothing else changed about now reports as drifted. `CODESTYLE.md` is `intent`, so its new item reaches the fleet as a rule each repo adopts in its own copy rather than as bytes to match. `TODO.md`'s re-vendor entry records all of this and now names **five files rather than three**, so the next sweep does not have to rediscover it from a diff that reads cosmetic. The three findings #565 records as already resolved were re-verified against the tree and need no change here. ## Verification | Gate | Result | | --- | --- | | `prose_lint.py` gating rules (charset, dupword, spelling) | clean tree-wide | | `test_prose_lint.py`, `test_repo_gate.py`, `test_pr_review.py` | pass (157 prose tests) | | `spec/audit.py --selftest` | PASS | | `scripts/repo_gate.py` | eol 0, sha-pin 0 | | `spec/validate.py` | 22 cataloged, 0 backlog, clean | | markdownlint-cli2 | 0 issues in 44 files | | cspell | 0 issues | | editorconfig-checker | clean | Every changed file kept its own line endings, which was checked after a first attempt flattened CRLF to LF and was reverted before anything was committed. `prose_lint.py --diff` reports **five warn-only findings** (four `comment-wrap`, one `dash`) on the three lines this diff touched in `spec/audit.py`, `spec/validate.py`, and `spec/fidelity-model.md`. All five predate this change and belong to the #519 backlog, where those files carry 101 and 8 violations respectively. Correcting the one line of each that this diff touches would leave the file no more conformant while burying a one-word sweep in rewrapped comments. ## Not in this PR - A `prose_lint` rule for the convention. The legitimate lowercase uses are the whole difficulty, so a checker is a design question rather than a follow-through, and stating the rule is what #565 asked for. - The fleet re-vendor, which is `TODO.md`'s entry and needs the maintainer to name the repos. Related: #565 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…574) Promotes the nine commits `develop` has carried since the previous promotion (#555) to `main`. Merge commit only, no squash, and `develop` is not deleted. ## What lands - **#560** Standardize the static-site-deploy repo type and its destination, including the deploy-verification guarantee and a retention rule that records which side owns the prune. - **#561** Ask for a blocked decision instead of reporting it, and scope the clickable-link rule to a surface that renders markdown. - **#564** Read a suppressed section nested inside the review-details wrapper, so `scripts/pr_review.py` stops reporting a clean round over findings that reach no thread. - **#566** Settle Markdown capitalization in `CODESTYLE.md`, and fix the two `TODO.md` prose defects the digest defect had hidden. - **#568** Check a `driftNote` on every run rather than only on an otherwise clean audit, and state what the audit does not evaluate. - **#569** Name who trips the production ref gate, and state that a harness refusal is a different thing from the maintainer's permission and is not lifted by it. - **#571** Re-audit `ptr727/Blog` against the hugo type and bump the conformance matrix. - **#573** Scope the semicolon exemption to the sentence its list lives in, with the 44 newly reported occurrences recorded on #519 rather than swept here. - **#572** Exempt a verbatim section from the coordination-reference scan. ## Issues closed Fixes #562. Closes #565. The closing keywords sit on this promotion rather than on the feature pull requests, because GitHub fires them only on a merge into the default branch. #456, #558, #563, #567, and #570 are already closed against their merged fixes. ## Verification `git merge-tree` reports no conflict between `origin/main` and `origin/develop`, so this promotion needs no throwaway resolution branch. Every constituent pull request merged green with its review loop closed.
GOVERNANCE.md"Communicating with the User" governed the form of a question (a numbered list) and said nothing about where it goes. So a decision only the maintainer can make could satisfy the section while sitting in the last paragraph of a summary, which is where it dies: a summary reads as a report of finished work, the one line still waiting on the user is the easiest in it to skim past, and the item ends up owned by neither side while each believes the other has it.What lands
One bullet in "Communicating with the User". Work blocked on the user is raised as a direct interactive prompt with selectable options, at the point the work stops, never as prose in a summary. The blocked item is the message rather than a closing remark on a message about something else. It narrows rather than replaces the numbered-list bullet above it: the list stays the form wherever the interface offers no prompt mechanism, so an agent on a plain-text surface is still covered.
A second bullet-level clause: the options offered are the actions themselves. The first draft required the prompt and left its options unspecified, which a prompt offering only ways to wait satisfies while clearing nothing. The option that unblocks the work now names the action it authorizes, so selecting it is the go-ahead, and where the agent may not perform that action itself the option says who does.
A third change, to the clickable-link bullet above them: the form follows the surface. That rule named one syntax, and a markdown link is clickable only where markdown renders. An interactive prompt links neither the markdown nor a bare URL, so following the rule as written puts an unclickable string in front of the reader, which is the outcome it exists to prevent. Its ban on a bare
#123is now scoped to a surface that renders markdown, and where none does, the reference is the bare#123and the clickable link goes in the accompanying message. Found by using the new prompt rule and watching the link fail in it.The closing-keyword rule moves from "Release Model" to "Branching Model", unchanged. That rule was queued for restatement in this PR, and it was already law: closing keywords go on the
develop -> mainpromotion PR, because GitHub fires them only on a merge into the default branch, with a hand-close citing the squash SHA as the fallback. Nothing is added for it, since a second statement of a rule is the failure mode rather than the fix. What was wrong was its address. It turns on develop-versus-default-branch mechanics that "Branching Model" states and "Release Model" does not, and an agent writing a feature PR body reaches for the branch section, so it now follows the promotion-execution traps beside the otherdevelop -> mainmechanics. The move commit carries the line byte-identical, so it reads as a move rather than as a rewrite. A later commit then formats its branch names as code, matching the section it now sits in, which is a change to the rule's text and is deliberately kept out of the move commit.For the reviewer
Both sections are
verbatimfidelity inspec/files.json, so every fleet repo carries the older copies until re-vendored, and an audit run against a downstream repo before that reports both sections as divergent.A tree-wide grep for the moved rule's distinctive phrasing (
issue-closing,closing keyword,Fixes #N) finds no other site restating or cross-referencing it, so no anchor or citation goes stale with the move.Verification
prose_lint(both CI invocations),repo_gate.py,spec/validate.py,test_prose_lint.py, markdownlint-cli2, and editorconfig-checker all clean after each commit. The added line carries the file's CRLF endings.