From adaecf462c12bbd50a9e826f745d3e58c5be4fd2 Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Fri, 23 Oct 2026 16:12:29 -0700 Subject: [PATCH 1/4] Continue work on write-like-me Claude-Session: https://claude.ai/code/session_014r1UL24kL2ipTAeC3mmxPP --- ai/AGENTS.md | 1 + ai/helpers/portable-skills.sh | 1 + ai/skills/address-pr-reviews/SKILL.md | 2 +- .../references/plain-writing/SKILL.md | 12 ++- .../plain-writing/references/voice.md | 62 +++++++++++++++ .../scripts/plain-writing-lint.py | 21 ++++- ai/skills/create-pr/SKILL.md | 4 +- ai/skills/plain-writing/SKILL.md | 12 ++- ai/skills/plain-writing/references/voice.md | 62 +++++++++++++++ .../scripts/plain-writing-lint.py | 22 +++++- .../scripts/tests/test_plain_writing_lint.py | 76 +++++++++++++++++++ 11 files changed, 262 insertions(+), 13 deletions(-) create mode 100644 ai/skills/address-pr-reviews/references/plain-writing/references/voice.md create mode 100644 ai/skills/plain-writing/references/voice.md diff --git a/ai/AGENTS.md b/ai/AGENTS.md index 5c48283f..6d15154d 100644 --- a/ai/AGENTS.md +++ b/ai/AGENTS.md @@ -121,6 +121,7 @@ These rules govern every response, chat replies included. The `plain-writing` sk - Describe concrete behavior instead of inventing labels, metaphors, or jargon. - Never use an em dash or an en dash. Use a comma, colon, semicolon, parenthesis, or a second sentence. - Start concise. Add detail only when it affects a decision or prevents a mistake. +- Keep text sent as me short: the point, the ask, and at most one reason. Hedge suggestions and ask questions instead of giving orders, because that leaves room for me being wrong. Before writing a PR description, review comment, report, design doc, or a message sent on my behalf, apply `plain-writing`. Skip it for commit messages, code comments, and chat replies, and skip it when the active skill already applies `plain-writing` itself, as `create-pr` does. Code comments skip the full pass but not the naming rule under Style: don't leave a label the reader has to decode. diff --git a/ai/helpers/portable-skills.sh b/ai/helpers/portable-skills.sh index 723c76d8..18247862 100644 --- a/ai/helpers/portable-skills.sh +++ b/ai/helpers/portable-skills.sh @@ -38,6 +38,7 @@ address-pr-reviews:ai/skills/wait-for-pr-reviews/scripts/helpers/pending-reviews address-pr-reviews:ai/skills/comment-cleanup/SKILL.md:references/comment-cleanup/SKILL.md address-pr-reviews:ai/skills/plain-writing/SKILL.md:references/plain-writing/SKILL.md address-pr-reviews:ai/skills/plain-writing/references/strict.md:references/plain-writing/references/strict.md +address-pr-reviews:ai/skills/plain-writing/references/voice.md:references/plain-writing/references/voice.md address-pr-reviews:ai/skills/plain-writing/references/voice-match.md:references/plain-writing/references/voice-match.md address-pr-reviews:ai/skills/plain-writing/scripts/plain-writing-lint.py:references/plain-writing/scripts/plain-writing-lint.py' diff --git a/ai/skills/address-pr-reviews/SKILL.md b/ai/skills/address-pr-reviews/SKILL.md index e815a5f4..25b2f23f 100644 --- a/ai/skills/address-pr-reviews/SKILL.md +++ b/ai/skills/address-pr-reviews/SKILL.md @@ -138,7 +138,7 @@ With user confirmation: **For not-legit comments, branch on who authored the comment:** -- **Bots (`is_bot` true — Copilot, ReviewHog, Greptile, Graphite, or any other GitHub App):** Draft a concise, professional reply explaining why the code is correct, show the draft, and ask for approval as **Asking the user** describes. Unattended default: post it and resolve the thread. Post it with: `gh api "repos//pulls//comments//replies" --method POST -f body=''`. Resolve the thread: `scripts/gh-resolve-threads "https://github.com//pull/" --comment-id `. +- **Bots (`is_bot` true — Copilot, ReviewHog, Greptile, Graphite, or any other GitHub App):** Draft a short reply that gives the reason the code is correct, show the draft, and ask for approval as **Asking the user** describes. Unattended default: post it and resolve the thread. Post it with: `gh api "repos//pulls//comments//replies" --method POST -f body=''`. Resolve the thread: `scripts/gh-resolve-threads "https://github.com//pull/" --comment-id `. - **Human reviewers (`is_bot` false):** Never post anything. Draft the reply and hold it for the user to review and post themselves (see Step 5). Leave the thread unresolved so the reviewer gets the last word. ### Step 5: Finalize diff --git a/ai/skills/address-pr-reviews/references/plain-writing/SKILL.md b/ai/skills/address-pr-reviews/references/plain-writing/SKILL.md index 2b87315e..fa6d6c2e 100644 --- a/ai/skills/address-pr-reviews/references/plain-writing/SKILL.md +++ b/ai/skills/address-pr-reviews/references/plain-writing/SKILL.md @@ -15,7 +15,9 @@ Make the text easy for its intended reader to understand on the first pass. - `voice-match` is for personal writing when the user supplies a writing sample. Read [references/voice-match.md](references/voice-match.md) before writing. - `review` identifies unclear passages without rewriting them. For each one, quote the passage, say what makes it hard to read, and give the smallest useful change. That list of findings is the entire output for `review` mode. -When a mode's reference file conflicts with a rule below, follow the reference file. +When the text is a message the user sends to colleagues, also read [references/voice.md](references/voice.md) before writing. That covers Slack messages, review comments, replies, and summaries, issue comments, and PR descriptions. Reports, plans, and status updates don't use it, even when the user posts them. + +When a reference file conflicts with a rule below, follow the reference file. ## Reading the Arguments @@ -26,11 +28,12 @@ When a mode's reference file conflicts with a rule below, follow the reference f ## Preserve the Source -- Preserve every fact, number, condition, qualifier, citation, link, identifier, error string, and technical term that carries exact meaning. +- When the user wrote the source, preserve every fact, number, condition, qualifier, citation, link, identifier, error string, and technical term that carries exact meaning. Keep the requested format and approximate level of detail. Cut repetition, not substance. +- When you or a calling skill wrote the draft, keep only what the reader needs to act or decide. Leave out what they already know, can see in a linked diff or document, or would ask for if they needed it. That usually means a step-by-step mechanism, how other code does the same thing, citations they don't need to act on, and a second statement of a consequence. Never drop a fact that the ask or decision depends on. +- Keep every fact that stays exact, including its numbers, identifiers, qualifiers, and error strings. - Keep code blocks, inline code, commands, quotations, templates, checkboxes, headings, and required labels unchanged unless the user asks to edit them. - Do not add facts, examples, opinions, certainty, humor, personality, or conclusions that are not in the source. - Keep uncertainty when the source is uncertain. Clear writing must not become a stronger claim. -- Keep the requested format and approximate level of detail. Cut repetition, not substance. ## Write Clearly @@ -43,6 +46,7 @@ When a mode's reference file conflicts with a rule below, follow the reference f - Remove signposting, filler, hype, vague attribution, repeated summaries, generic conclusions, and chatbot closers. - Do not deny a smaller claim to set up the real one. Write what is true instead of `it is not just a cache, it is a full index`. - Do not end a sentence with an `-ing` clause that restates it. Cut `highlighting the need for review` or replace it with the fact it stands for. +- Do not make "Nothing" or "No X" the subject of a sentence. Put the real subject first and negate the verb: write `it doesn't change anything that already exists` instead of `nothing changes that wasn't already existing`. - Do not pad a list to three items for rhythm. Use as many items as the content has. - Do not use an em dash or an en dash, in any mode including voice-match. Restructure the sentence with other punctuation or separate sentences. - Allow natural contractions in technical mode. Do not force all sentences to the same length or rhythm. @@ -55,7 +59,7 @@ Before you return your answer, silently reread the result as its intended reader 1. Is the main point in the first sentence or first useful line? 2. Is the requested action clear? 3. Does each sentence make sense without a second pass? -4. Did every fact and technical token survive? +4. Did every fact you had to keep survive, with its technical tokens exact? 5. Can any sentence lose words without losing meaning or tone? Before you return the text, check it with `python3 scripts/plain-writing-lint.py`, resolving `scripts/` against this skill's own directory. Pipe the text on stdin, or pass a file path when you edited a file. In strict mode, add `--strict` first. Treat the output as warnings, not a quality score. Fix real problems and ignore false positives. diff --git a/ai/skills/address-pr-reviews/references/plain-writing/references/voice.md b/ai/skills/address-pr-reviews/references/plain-writing/references/voice.md new file mode 100644 index 00000000..7cf69cbf --- /dev/null +++ b/ai/skills/address-pr-reviews/references/plain-writing/references/voice.md @@ -0,0 +1,62 @@ +# Writing as the User + +The samples below come from the user's public GitHub comments from before 2023, written without AI help. + +## Length + +The reader knows the code and will ask if they need more. Give them the point or problem in terms of the code, the ask, and at most one reason they can't see from where they sit. + +- A review comment is one or two sentences, about 20 words. An argument about API design can run to a short paragraph or two. +- A review summary is one line, or nothing when the inline comments say it all. +- A Slack message is one to three sentences. Split a longer thought into several short messages, or link to the PR, doc, or video that has the detail. + +## Hedge Suggestions and Ask Questions + +The user hedges on purpose because it leaves room for being wrong. + +- Hedge a suggestion or a judgment call: "I think…", "I'd recommend…", "Maybe this should say…", "Minor nit, but…". +- Ask when the author might know something the user doesn't: "As it's currently written, this can throw. Is that handled elsewhere?" +- Ask for a missing test as a request: "Mind adding a unit test that fails without this fix?" +- State a bug you verified without a hedge: "If a process is holding a lock on that old file, this could fail with an exception." +- When the ask adds scope, give the author a way out: "If you don't anticipate that, then I'm fine with just specifying the named parameter." +- Use one hedge per claim. + +## Sentence Shape + +- Write in the first person: "I think", "I'm a little confused by this logic here", "I generally avoid booleans in this situation". +- Give the reason in one clause from the user's point of view: "The benefit of this is to make it easier to call this method in more scenarios." +- Show instead of describing: a suggestion block for a change, a screenshot for UI, a link to the demo. +- Say what you don't know: "My memory is a bit hazy, but I think there are test helpers for creating a repository." +- Use contractions. + +## Samples + +Review comments: + +- "These should probably be properties. Most likely they should be `readonly` properties. I know that makes creating instances of these not look as nice as using property initializers when constructing an instance, but if these properties should never be mutated after construction, then it guides future consumers to the right behavior." +- "So this is just casting a mutable list to an `IReadOnlyList` which can be cast back to `List`. I know we probably won't do that, but I'm thinking of the next person to come along. The safe thing to do is to wrap `List` with a `ReadOnlyList`." +- "I understand that if an input is `type="hidden"` we might not want to validate it, but why would we not validate a field just because it's scrolled offscreen? Wouldn't that allow someone to validate and submit a form by making their browser window smaller and scroll the invalid field off the screen?" +- "Minor nit, but since this isn't settable, it should be `readonly`." +- "What's this 250ms wait for?" +- "Can these throw exceptions?" +- "We're just going to swallow this error? Not even a log? What would it mean if this fails? What's the user experience here?" +- "This comment is no longer correct." A suggestion block follows it. + +Review summaries: + +- "Just a few comments. I don't feel too strongly about them though." +- "Looks good! Just a couple of questions really." +- "Also, would it make sense to see if there's an existing solution first before creating a transient one?" + +Conversation comments: + +- "Hmm, it's unclear to me why this would not work in VS2013 but work in your unit test context. Have you run a Fiddler trace to make sure the request is going out?" +- "One more thing. We can't remove `cibuild.ps1`. For now, we should just change it to call into the fake script just like build.cmd does. `cibuild.ps1` is a convention for our internal build server." +- "Ugh, nothing is ever easy. I think we should continue down this path. It'll eventually get fixed. Maybe we can keep an old .sln/.csproj file around for a bit if we get complaints." + +PR descriptions: + +- "These methods actually returned mutable collections that happen to implement the readonly interfaces. User could cast them to the actual type and add things. I'd rather avoid even that possibility by making these truly return readonly stuff." +- "Coordinating the timing of event handler callbacks with async code is very tricky. Adding a custom validation event gives consumers an option to listen for those events rather than the form submit event." +- "There are cases where you need to override the accepts header in an observable client. Rather than add an override to every possible method, I'm just making the `GetAndFlattenAllPages` method public. That way, you can drop to the underlying `IConnection` method and pass in an accepts header, but you don't have to recreate all the logic for paging." +- "It's possible for a repository to have a null value for the `pushed_at` field. For example, if it's been created online and never pushed to via git. This fixes that and adds an integration test." diff --git a/ai/skills/address-pr-reviews/references/plain-writing/scripts/plain-writing-lint.py b/ai/skills/address-pr-reviews/references/plain-writing/scripts/plain-writing-lint.py index c57dccfc..d691b164 100755 --- a/ai/skills/address-pr-reviews/references/plain-writing/scripts/plain-writing-lint.py +++ b/ai/skills/address-pr-reviews/references/plain-writing/scripts/plain-writing-lint.py @@ -10,6 +10,10 @@ from pathlib import Path from typing import Iterable, TypedDict +SENTENCE_END = r"[.!?]" +SENTENCE_OPENING = rf"(?:^\s*(?:(?:[-*+]|\d+[.)])\s+)?|{SENTENCE_END}\s+)" +MODAL = r"can|could|will|would|should|must" + RAW_PATTERNS: dict[str, tuple[str, ...]] = { "filler": ( r"\blet(?:'|’)s (?:dive in|dive into|explore|break this down)\b", @@ -44,6 +48,17 @@ r"\b(?:is|are|was|were)n(?:'|\u2019)t (?:just |only |merely |simply )?about " r"[^.!?;\n]{2,80}?, (?:it|this|that)(?:'|\u2019)s about\b", ), + # The "No" verb list leaves out verbs that double as plural nouns, so "No behavioral + # changes in this PR" stays quiet. "No one" reads like "Nobody", which people write. + # "No matter" starts a clause rather than naming a subject. + "negative_subject": ( + rf"{SENTENCE_OPENING}Nothing (?:else |in (?:this|the) \w+ )?" + rf"(?:[a-z]+s|{MODAL})\b", + rf"{SENTENCE_OPENING}No (?!one\b|matter\b)(?:[a-z][\w-]* ){{1,2}}" + rf"(?:is|are|was|were|has|have|does|did|{MODAL}" + r"|(?:use|need|cover|exercise|assert|depend|touch|catch)(?:e?s)?" + r"|(?:verif|rel)(?:y|ies))\b", + ), # A comma inside the clause means a mid-sentence aside ("the report, highlighting # three defects, was filed"), which is ordinary prose rather than the tic. "trailing_participle": ( @@ -60,6 +75,10 @@ "trailing_participle": ( "Trailing -ing clause restates the sentence; cut it or name the fact." ), + "negative_subject": ( + 'Opens with "Nothing" or "No X" as the subject; start with what the sentence ' + "is about and negate the verb." + ), } # Compiled once at import so the per-line loop below only dispatches matches, the same @@ -77,7 +96,7 @@ URL = re.compile(r"https?://[^\s)>]+") WORD = re.compile(r"[A-Za-z0-9][A-Za-z0-9'’/-]*") FENCE = re.compile(r"^ {0,3}(`{3,}|~{3,})") -SENTENCE_SPLIT = re.compile(r"(?<=[.!?])\s+") +SENTENCE_SPLIT = re.compile(rf"(?<={SENTENCE_END})\s+") FRONTMATTER_DELIMITER = "---" diff --git a/ai/skills/create-pr/SKILL.md b/ai/skills/create-pr/SKILL.md index 3ebed3e5..c64fbb37 100644 --- a/ai/skills/create-pr/SKILL.md +++ b/ai/skills/create-pr/SKILL.md @@ -173,9 +173,9 @@ Notes: ### 6. Edit the prose before preview -First cut the problem and solution prose from Step 5. Remove text whose only job is to walk through the diff, such as tours of individual files and the step-by-step mechanics of code the reviewer will read anyway. Keep the problem, the key decisions, and anything a reviewer would miss in the code. Keep a value or identifier when it is the cause or the behavior change. This cut does not touch the test plan (including an embedded saved test plan), checkboxes, the agent-context section, the `Depends on` lines, or the footer. When the template or a repo skill it names sets its own rules for what to cut, follow those instead. `plain-writing` keeps every fact it receives, so it will not make this cut for you. +First cut the problem and solution prose from Step 5. Remove text whose only job is to walk through the diff, such as tours of individual files and the step-by-step mechanics of code the reviewer will read anyway. Keep the problem, the key decisions, and anything a reviewer would miss in the code. Keep a value or identifier when it is the cause or the behavior change. This cut does not touch the test plan (including an embedded saved test plan), checkboxes, the agent-context section, the `Depends on` lines, or the footer. When the template or a repo skill it names sets its own rules for what to cut, follow those instead. -Then apply the `plain-writing` skill in technical mode to the composed body. Keep the template structure, its checkboxes, the `Depends on` lines, and any tool-mandated footer exactly as given, and leave the agent-context section in the first-person agent voice Step 5 requires. Last, enforce the PR-specific rules in the Writing voice section, and ask of each sentence: would a staff engineer send this verbatim in a Slack message to the reviewer? If not, simplify it. The agent-context section and the footer are exempt from that test. Show only the finished body in the preview. +Then apply the `plain-writing` skill in technical mode to the composed body. Keep the template structure, its checkboxes, the test plan, the `Depends on` lines, and any tool-mandated footer exactly as given, and leave the agent-context section in the first-person agent voice Step 5 requires. Last, enforce the PR-specific rules in the Writing voice section, and ask of each sentence: would a staff engineer send this verbatim in a Slack message to the reviewer? If not, simplify it. The agent-context section and the footer are exempt from that test. Show only the finished body in the preview. ### 7. Show Preview and Confirm diff --git a/ai/skills/plain-writing/SKILL.md b/ai/skills/plain-writing/SKILL.md index 2b87315e..fa6d6c2e 100644 --- a/ai/skills/plain-writing/SKILL.md +++ b/ai/skills/plain-writing/SKILL.md @@ -15,7 +15,9 @@ Make the text easy for its intended reader to understand on the first pass. - `voice-match` is for personal writing when the user supplies a writing sample. Read [references/voice-match.md](references/voice-match.md) before writing. - `review` identifies unclear passages without rewriting them. For each one, quote the passage, say what makes it hard to read, and give the smallest useful change. That list of findings is the entire output for `review` mode. -When a mode's reference file conflicts with a rule below, follow the reference file. +When the text is a message the user sends to colleagues, also read [references/voice.md](references/voice.md) before writing. That covers Slack messages, review comments, replies, and summaries, issue comments, and PR descriptions. Reports, plans, and status updates don't use it, even when the user posts them. + +When a reference file conflicts with a rule below, follow the reference file. ## Reading the Arguments @@ -26,11 +28,12 @@ When a mode's reference file conflicts with a rule below, follow the reference f ## Preserve the Source -- Preserve every fact, number, condition, qualifier, citation, link, identifier, error string, and technical term that carries exact meaning. +- When the user wrote the source, preserve every fact, number, condition, qualifier, citation, link, identifier, error string, and technical term that carries exact meaning. Keep the requested format and approximate level of detail. Cut repetition, not substance. +- When you or a calling skill wrote the draft, keep only what the reader needs to act or decide. Leave out what they already know, can see in a linked diff or document, or would ask for if they needed it. That usually means a step-by-step mechanism, how other code does the same thing, citations they don't need to act on, and a second statement of a consequence. Never drop a fact that the ask or decision depends on. +- Keep every fact that stays exact, including its numbers, identifiers, qualifiers, and error strings. - Keep code blocks, inline code, commands, quotations, templates, checkboxes, headings, and required labels unchanged unless the user asks to edit them. - Do not add facts, examples, opinions, certainty, humor, personality, or conclusions that are not in the source. - Keep uncertainty when the source is uncertain. Clear writing must not become a stronger claim. -- Keep the requested format and approximate level of detail. Cut repetition, not substance. ## Write Clearly @@ -43,6 +46,7 @@ When a mode's reference file conflicts with a rule below, follow the reference f - Remove signposting, filler, hype, vague attribution, repeated summaries, generic conclusions, and chatbot closers. - Do not deny a smaller claim to set up the real one. Write what is true instead of `it is not just a cache, it is a full index`. - Do not end a sentence with an `-ing` clause that restates it. Cut `highlighting the need for review` or replace it with the fact it stands for. +- Do not make "Nothing" or "No X" the subject of a sentence. Put the real subject first and negate the verb: write `it doesn't change anything that already exists` instead of `nothing changes that wasn't already existing`. - Do not pad a list to three items for rhythm. Use as many items as the content has. - Do not use an em dash or an en dash, in any mode including voice-match. Restructure the sentence with other punctuation or separate sentences. - Allow natural contractions in technical mode. Do not force all sentences to the same length or rhythm. @@ -55,7 +59,7 @@ Before you return your answer, silently reread the result as its intended reader 1. Is the main point in the first sentence or first useful line? 2. Is the requested action clear? 3. Does each sentence make sense without a second pass? -4. Did every fact and technical token survive? +4. Did every fact you had to keep survive, with its technical tokens exact? 5. Can any sentence lose words without losing meaning or tone? Before you return the text, check it with `python3 scripts/plain-writing-lint.py`, resolving `scripts/` against this skill's own directory. Pipe the text on stdin, or pass a file path when you edited a file. In strict mode, add `--strict` first. Treat the output as warnings, not a quality score. Fix real problems and ignore false positives. diff --git a/ai/skills/plain-writing/references/voice.md b/ai/skills/plain-writing/references/voice.md new file mode 100644 index 00000000..7cf69cbf --- /dev/null +++ b/ai/skills/plain-writing/references/voice.md @@ -0,0 +1,62 @@ +# Writing as the User + +The samples below come from the user's public GitHub comments from before 2023, written without AI help. + +## Length + +The reader knows the code and will ask if they need more. Give them the point or problem in terms of the code, the ask, and at most one reason they can't see from where they sit. + +- A review comment is one or two sentences, about 20 words. An argument about API design can run to a short paragraph or two. +- A review summary is one line, or nothing when the inline comments say it all. +- A Slack message is one to three sentences. Split a longer thought into several short messages, or link to the PR, doc, or video that has the detail. + +## Hedge Suggestions and Ask Questions + +The user hedges on purpose because it leaves room for being wrong. + +- Hedge a suggestion or a judgment call: "I think…", "I'd recommend…", "Maybe this should say…", "Minor nit, but…". +- Ask when the author might know something the user doesn't: "As it's currently written, this can throw. Is that handled elsewhere?" +- Ask for a missing test as a request: "Mind adding a unit test that fails without this fix?" +- State a bug you verified without a hedge: "If a process is holding a lock on that old file, this could fail with an exception." +- When the ask adds scope, give the author a way out: "If you don't anticipate that, then I'm fine with just specifying the named parameter." +- Use one hedge per claim. + +## Sentence Shape + +- Write in the first person: "I think", "I'm a little confused by this logic here", "I generally avoid booleans in this situation". +- Give the reason in one clause from the user's point of view: "The benefit of this is to make it easier to call this method in more scenarios." +- Show instead of describing: a suggestion block for a change, a screenshot for UI, a link to the demo. +- Say what you don't know: "My memory is a bit hazy, but I think there are test helpers for creating a repository." +- Use contractions. + +## Samples + +Review comments: + +- "These should probably be properties. Most likely they should be `readonly` properties. I know that makes creating instances of these not look as nice as using property initializers when constructing an instance, but if these properties should never be mutated after construction, then it guides future consumers to the right behavior." +- "So this is just casting a mutable list to an `IReadOnlyList` which can be cast back to `List`. I know we probably won't do that, but I'm thinking of the next person to come along. The safe thing to do is to wrap `List` with a `ReadOnlyList`." +- "I understand that if an input is `type="hidden"` we might not want to validate it, but why would we not validate a field just because it's scrolled offscreen? Wouldn't that allow someone to validate and submit a form by making their browser window smaller and scroll the invalid field off the screen?" +- "Minor nit, but since this isn't settable, it should be `readonly`." +- "What's this 250ms wait for?" +- "Can these throw exceptions?" +- "We're just going to swallow this error? Not even a log? What would it mean if this fails? What's the user experience here?" +- "This comment is no longer correct." A suggestion block follows it. + +Review summaries: + +- "Just a few comments. I don't feel too strongly about them though." +- "Looks good! Just a couple of questions really." +- "Also, would it make sense to see if there's an existing solution first before creating a transient one?" + +Conversation comments: + +- "Hmm, it's unclear to me why this would not work in VS2013 but work in your unit test context. Have you run a Fiddler trace to make sure the request is going out?" +- "One more thing. We can't remove `cibuild.ps1`. For now, we should just change it to call into the fake script just like build.cmd does. `cibuild.ps1` is a convention for our internal build server." +- "Ugh, nothing is ever easy. I think we should continue down this path. It'll eventually get fixed. Maybe we can keep an old .sln/.csproj file around for a bit if we get complaints." + +PR descriptions: + +- "These methods actually returned mutable collections that happen to implement the readonly interfaces. User could cast them to the actual type and add things. I'd rather avoid even that possibility by making these truly return readonly stuff." +- "Coordinating the timing of event handler callbacks with async code is very tricky. Adding a custom validation event gives consumers an option to listen for those events rather than the form submit event." +- "There are cases where you need to override the accepts header in an observable client. Rather than add an override to every possible method, I'm just making the `GetAndFlattenAllPages` method public. That way, you can drop to the underlying `IConnection` method and pass in an accepts header, but you don't have to recreate all the logic for paging." +- "It's possible for a repository to have a null value for the `pushed_at` field. For example, if it's been created online and never pushed to via git. This fixes that and adds an integration test." diff --git a/ai/skills/plain-writing/scripts/plain-writing-lint.py b/ai/skills/plain-writing/scripts/plain-writing-lint.py index c57dccfc..b0daa71c 100755 --- a/ai/skills/plain-writing/scripts/plain-writing-lint.py +++ b/ai/skills/plain-writing/scripts/plain-writing-lint.py @@ -10,6 +10,10 @@ from pathlib import Path from typing import Iterable, TypedDict +SENTENCE_END = r"[.!?]" +SENTENCE_OPENING = rf"(?:^\s*(?:(?:[-*+]|\d+[.)])\s+)?|{SENTENCE_END}\s+)" +MODAL = r"can|could|will|would|should|must" + RAW_PATTERNS: dict[str, tuple[str, ...]] = { "filler": ( r"\blet(?:'|’)s (?:dive in|dive into|explore|break this down)\b", @@ -44,6 +48,18 @@ r"\b(?:is|are|was|were)n(?:'|\u2019)t (?:just |only |merely |simply )?about " r"[^.!?;\n]{2,80}?, (?:it|this|that)(?:'|\u2019)s about\b", ), + # The "No" verb list leaves out verbs that double as plural nouns. The linter + # therefore does not flag "No behavioral changes in this PR". "No one" reads like + # "Nobody", which people write. + # "No matter" starts a clause rather than naming a subject. + "negative_subject": ( + rf"{SENTENCE_OPENING}Nothing (?:else |in (?:this|the) \w+ )?" + rf"(?:[a-z]+s|{MODAL})\b", + rf"{SENTENCE_OPENING}No (?!one\b|matter\b)(?:[a-z][\w-]* ){{1,2}}" + rf"(?:is|are|was|were|has|have|does|did|{MODAL}" + r"|(?:use|need|cover|exercise|assert|depend|touch|catch)(?:e?s)?" + r"|(?:verif|rel)(?:y|ies))\b", + ), # A comma inside the clause means a mid-sentence aside ("the report, highlighting # three defects, was filed"), which is ordinary prose rather than the tic. "trailing_participle": ( @@ -60,6 +76,10 @@ "trailing_participle": ( "Trailing -ing clause restates the sentence; cut it or name the fact." ), + "negative_subject": ( + 'Opens with "Nothing" or "No X" as the subject; start with what the sentence ' + "is about and negate the verb." + ), } # Compiled once at import so the per-line loop below only dispatches matches, the same @@ -77,7 +97,7 @@ URL = re.compile(r"https?://[^\s)>]+") WORD = re.compile(r"[A-Za-z0-9][A-Za-z0-9'’/-]*") FENCE = re.compile(r"^ {0,3}(`{3,}|~{3,})") -SENTENCE_SPLIT = re.compile(r"(?<=[.!?])\s+") +SENTENCE_SPLIT = re.compile(rf"(?<={SENTENCE_END})\s+") FRONTMATTER_DELIMITER = "---" diff --git a/ai/skills/plain-writing/scripts/tests/test_plain_writing_lint.py b/ai/skills/plain-writing/scripts/tests/test_plain_writing_lint.py index d1372c05..a05916a3 100644 --- a/ai/skills/plain-writing/scripts/tests/test_plain_writing_lint.py +++ b/ai/skills/plain-writing/scripts/tests/test_plain_writing_lint.py @@ -148,6 +148,67 @@ def test_mid_sentence_aside_is_not_a_trailing_participle(self) -> None: with self.subTest(text=text): self.assertEqual(lint(text), []) + def test_flags_negative_subject(self) -> None: + for text in ( + "Nothing changes that wasn't already existing.", + "Nothing else reads it.", + "Nothing in this PR changes the schema.", + "Nothing should happen.", + "No test covers the expired token case.", + "No caller depends on the old name.", + "No changes are needed.", + "No existing test covers the expired token case.", + "No tests exercise this path.", + "No tests use the old name.", + "No other caller uses it.", + ): + with self.subTest(text=text): + self.assertIn("negative_subject", categories(lint(text))) + + def test_flags_negative_subject_at_every_sentence_opening(self) -> None: + for opening in ("- ", "1. ", " - ", "The flag is off. "): + for sentence in ( + "Nothing else reads it.", + "No caller depends on the old name.", + ): + text = opening + sentence + with self.subTest(text=text): + self.assertIn("negative_subject", categories(lint(text))) + + def test_plain_negation_is_not_a_negative_subject(self) -> None: + for text in ( + "It doesn't change anything that already exists.", + "There's nothing else to do.", + "Nobody is working on this.", + "No one has asked.", + "No, that's expected.", + ): + with self.subTest(text=text): + self.assertEqual(lint(text), []) + + def test_fragment_is_not_a_negative_subject(self) -> None: + for text in ( + "Nothing to note in the release notes.", + "Nothing in production.", + "No unit tests yet.", + "No behavioral changes in this PR.", + "No breaking changes.", + "No idea why this happens.", + "No matter what is chosen, the request retries.", + ): + with self.subTest(text=text): + self.assertEqual(lint(text), []) + + def test_negative_word_away_from_a_sentence_opening_is_not_flagged(self) -> None: + for text in ( + "We found that no caller depends on the old name.", + "The flag is off, and nothing else reads it.", + "> Nothing changes here.", + "> No caller depends on the old name.", + ): + with self.subTest(text=text): + self.assertEqual(lint(text), []) + def test_new_categories_carry_their_own_message(self) -> None: generic = lint("This robust process works.")[0]["message"] specific = lint("It's not just a linter, it's a style guide.")[0]["message"] @@ -160,6 +221,21 @@ def test_new_categories_carry_their_own_message(self) -> None: "Denies a smaller claim to set up the real one; state what is true.", ) + def test_negative_subject_carries_its_own_message(self) -> None: + messages = [ + warning["message"] + for warning in lint("No caller depends on the old name.") + if warning["category"] == "negative_subject" + ] + + self.assertEqual( + messages, + [ + 'Opens with "Nothing" or "No X" as the subject; start with what ' + "the sentence is about and negate the verb." + ], + ) + def test_cli_warns_without_failing_unless_requested(self) -> None: command = [sys.executable, str(SCRIPT), "--json"] warning_only = subprocess.run( From 0e789e5475f2db47bf9463873be7c2c43edf1bc4 Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Fri, 23 Oct 2026 16:54:25 -0700 Subject: [PATCH 2/4] Address review findings Claude-Session: https://claude.ai/code/session_014r1UL24kL2ipTAeC3mmxPP --- .../references/plain-writing/SKILL.md | 8 ++++---- .../references/plain-writing/references/voice.md | 2 +- .../plain-writing/scripts/plain-writing-lint.py | 11 +++++++---- ai/skills/plain-writing/SKILL.md | 8 ++++---- ai/skills/plain-writing/references/voice.md | 2 +- .../plain-writing/scripts/plain-writing-lint.py | 12 +++++++----- .../scripts/tests/test_plain_writing_lint.py | 2 ++ 7 files changed, 26 insertions(+), 19 deletions(-) diff --git a/ai/skills/address-pr-reviews/references/plain-writing/SKILL.md b/ai/skills/address-pr-reviews/references/plain-writing/SKILL.md index fa6d6c2e..74b1ef90 100644 --- a/ai/skills/address-pr-reviews/references/plain-writing/SKILL.md +++ b/ai/skills/address-pr-reviews/references/plain-writing/SKILL.md @@ -15,7 +15,7 @@ Make the text easy for its intended reader to understand on the first pass. - `voice-match` is for personal writing when the user supplies a writing sample. Read [references/voice-match.md](references/voice-match.md) before writing. - `review` identifies unclear passages without rewriting them. For each one, quote the passage, say what makes it hard to read, and give the smallest useful change. That list of findings is the entire output for `review` mode. -When the text is a message the user sends to colleagues, also read [references/voice.md](references/voice.md) before writing. That covers Slack messages, review comments, replies, and summaries, issue comments, and PR descriptions. Reports, plans, and status updates don't use it, even when the user posts them. +When the text is a message the user sends to colleagues, also read [references/voice.md](references/voice.md) before writing. That covers Slack messages, review comments, replies, and summaries, issue comments, and PR descriptions. Reports, plans, and status updates don't use it, even when the user posts them. Its length targets apply only to drafts you or a calling skill wrote. When the user wrote the source, Preserve the Source wins over them. When a reference file conflicts with a rule below, follow the reference file. @@ -28,9 +28,9 @@ When a reference file conflicts with a rule below, follow the reference file. ## Preserve the Source -- When the user wrote the source, preserve every fact, number, condition, qualifier, citation, link, identifier, error string, and technical term that carries exact meaning. Keep the requested format and approximate level of detail. Cut repetition, not substance. -- When you or a calling skill wrote the draft, keep only what the reader needs to act or decide. Leave out what they already know, can see in a linked diff or document, or would ask for if they needed it. That usually means a step-by-step mechanism, how other code does the same thing, citations they don't need to act on, and a second statement of a consequence. Never drop a fact that the ask or decision depends on. -- Keep every fact that stays exact, including its numbers, identifiers, qualifiers, and error strings. +- Keep the requested format. +- When the user wrote the source, preserve every fact, number, condition, qualifier, citation, link, identifier, error string, and technical term that carries exact meaning. Keep the approximate level of detail. Cut repetition, not substance. +- When you or a calling skill wrote the draft, keep only what the reader needs to act or decide. Leave out what they already know, can see in a linked diff or document, or would ask for if they needed it. That usually means a step-by-step mechanism, how other code does the same thing, citations they don't need to act on, and a second statement of a consequence. Never drop a fact that the ask or decision depends on. Every fact you keep stays exact, including its numbers, identifiers, qualifiers, links, and error strings. - Keep code blocks, inline code, commands, quotations, templates, checkboxes, headings, and required labels unchanged unless the user asks to edit them. - Do not add facts, examples, opinions, certainty, humor, personality, or conclusions that are not in the source. - Keep uncertainty when the source is uncertain. Clear writing must not become a stronger claim. diff --git a/ai/skills/address-pr-reviews/references/plain-writing/references/voice.md b/ai/skills/address-pr-reviews/references/plain-writing/references/voice.md index 7cf69cbf..38a51e05 100644 --- a/ai/skills/address-pr-reviews/references/plain-writing/references/voice.md +++ b/ai/skills/address-pr-reviews/references/plain-writing/references/voice.md @@ -19,7 +19,7 @@ The user hedges on purpose because it leaves room for being wrong. - Ask for a missing test as a request: "Mind adding a unit test that fails without this fix?" - State a bug you verified without a hedge: "If a process is holding a lock on that old file, this could fail with an exception." - When the ask adds scope, give the author a way out: "If you don't anticipate that, then I'm fine with just specifying the named parameter." -- Use one hedge per claim. +- Use at most one hedge per claim. ## Sentence Shape diff --git a/ai/skills/address-pr-reviews/references/plain-writing/scripts/plain-writing-lint.py b/ai/skills/address-pr-reviews/references/plain-writing/scripts/plain-writing-lint.py index d691b164..e66087f7 100755 --- a/ai/skills/address-pr-reviews/references/plain-writing/scripts/plain-writing-lint.py +++ b/ai/skills/address-pr-reviews/references/plain-writing/scripts/plain-writing-lint.py @@ -48,12 +48,15 @@ r"\b(?:is|are|was|were)n(?:'|\u2019)t (?:just |only |merely |simply )?about " r"[^.!?;\n]{2,80}?, (?:it|this|that)(?:'|\u2019)s about\b", ), - # The "No" verb list leaves out verbs that double as plural nouns, so "No behavioral - # changes in this PR" stays quiet. "No one" reads like "Nobody", which people write. - # "No matter" starts a clause rather than naming a subject. + # The "No" verb list leaves out "change" and "test". The linter therefore does not + # flag "No behavioral changes in this PR" or "No unit tests yet". Some listed verbs + # are also plural nouns. The linter therefore flags "No new uses of the API". + # "No one" reads like "Nobody", which people write. + # "No matter" starts a clause rather than naming a subject. After "Nothing", a word + # that ends in "ous" is an adjective, as in "Nothing serious". "negative_subject": ( rf"{SENTENCE_OPENING}Nothing (?:else |in (?:this|the) \w+ )?" - rf"(?:[a-z]+s|{MODAL})\b", + rf"(?:[a-z]+(? None: for text in ( "Nothing to note in the release notes.", "Nothing in production.", + "Nothing serious, just a nit.", + "Nothing obvious here.", "No unit tests yet.", "No behavioral changes in this PR.", "No breaking changes.", From 6588fb746a12c1906a46a292aee67096e3cfd27a Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Fri, 23 Oct 2026 16:56:53 -0700 Subject: [PATCH 3/4] Simplify review fixes Claude-Session: https://claude.ai/code/session_014r1UL24kL2ipTAeC3mmxPP --- .../address-pr-reviews/references/plain-writing/SKILL.md | 2 +- .../references/plain-writing/references/voice.md | 2 +- .../references/plain-writing/scripts/plain-writing-lint.py | 4 ++-- ai/skills/plain-writing/SKILL.md | 2 +- ai/skills/plain-writing/references/voice.md | 2 +- ai/skills/plain-writing/scripts/plain-writing-lint.py | 4 ++-- .../plain-writing/scripts/tests/test_plain_writing_lint.py | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-) diff --git a/ai/skills/address-pr-reviews/references/plain-writing/SKILL.md b/ai/skills/address-pr-reviews/references/plain-writing/SKILL.md index 74b1ef90..3b8ada59 100644 --- a/ai/skills/address-pr-reviews/references/plain-writing/SKILL.md +++ b/ai/skills/address-pr-reviews/references/plain-writing/SKILL.md @@ -15,7 +15,7 @@ Make the text easy for its intended reader to understand on the first pass. - `voice-match` is for personal writing when the user supplies a writing sample. Read [references/voice-match.md](references/voice-match.md) before writing. - `review` identifies unclear passages without rewriting them. For each one, quote the passage, say what makes it hard to read, and give the smallest useful change. That list of findings is the entire output for `review` mode. -When the text is a message the user sends to colleagues, also read [references/voice.md](references/voice.md) before writing. That covers Slack messages, review comments, replies, and summaries, issue comments, and PR descriptions. Reports, plans, and status updates don't use it, even when the user posts them. Its length targets apply only to drafts you or a calling skill wrote. When the user wrote the source, Preserve the Source wins over them. +When the text is a message the user sends to colleagues, also read [references/voice.md](references/voice.md) before writing. That covers Slack messages, review comments, replies, and summaries, issue comments, and PR descriptions. Reports, plans, and status updates don't use it, even when the user posts them. When a reference file conflicts with a rule below, follow the reference file. diff --git a/ai/skills/address-pr-reviews/references/plain-writing/references/voice.md b/ai/skills/address-pr-reviews/references/plain-writing/references/voice.md index 38a51e05..facdfc4f 100644 --- a/ai/skills/address-pr-reviews/references/plain-writing/references/voice.md +++ b/ai/skills/address-pr-reviews/references/plain-writing/references/voice.md @@ -4,7 +4,7 @@ The samples below come from the user's public GitHub comments from before 2023, ## Length -The reader knows the code and will ask if they need more. Give them the point or problem in terms of the code, the ask, and at most one reason they can't see from where they sit. +The reader knows the code and will ask if they need more. Give them the point or problem in terms of the code, the ask, and at most one reason they can't see from where they sit. These targets apply to drafts you or a calling skill wrote. When the user wrote the text, keep its length. - A review comment is one or two sentences, about 20 words. An argument about API design can run to a short paragraph or two. - A review summary is one line, or nothing when the inline comments say it all. diff --git a/ai/skills/address-pr-reviews/references/plain-writing/scripts/plain-writing-lint.py b/ai/skills/address-pr-reviews/references/plain-writing/scripts/plain-writing-lint.py index e66087f7..eab87c1f 100755 --- a/ai/skills/address-pr-reviews/references/plain-writing/scripts/plain-writing-lint.py +++ b/ai/skills/address-pr-reviews/references/plain-writing/scripts/plain-writing-lint.py @@ -53,10 +53,10 @@ # are also plural nouns. The linter therefore flags "No new uses of the API". # "No one" reads like "Nobody", which people write. # "No matter" starts a clause rather than naming a subject. After "Nothing", a word - # that ends in "ous" is an adjective, as in "Nothing serious". + # that ends in "us" or "ss" is rarely a verb: "Nothing serious", "Nothing less". "negative_subject": ( rf"{SENTENCE_OPENING}Nothing (?:else |in (?:this|the) \w+ )?" - rf"(?:[a-z]+(? None: "Nothing to note in the release notes.", "Nothing in production.", "Nothing serious, just a nit.", - "Nothing obvious here.", + "Nothing less than a rewrite.", "No unit tests yet.", "No behavioral changes in this PR.", "No breaking changes.", From b2ab0cd8b00bf2e795cf393f38ac1d22039d20bb Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Fri, 23 Oct 2026 17:13:50 -0700 Subject: [PATCH 4/4] Skip idioms in negative_subject and keep facts when the author is unknown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ai/skills/plain-writing/scripts/plain-writing-lint.py: "No X" counts only when X is a noun phrase, so "No idea what…", "No need to…", and "No it is not." aren't flagged - ai/skills/plain-writing/SKILL.md: a source of unknown authorship keeps every fact Claude-Session: https://claude.ai/code/session_014r1UL24kL2ipTAeC3mmxPP --- .../references/plain-writing/SKILL.md | 2 +- .../scripts/plain-writing-lint.py | 27 +++++++++++++------ ai/skills/plain-writing/SKILL.md | 2 +- .../scripts/plain-writing-lint.py | 27 +++++++++++++------ .../scripts/tests/test_plain_writing_lint.py | 22 ++++++++++++++- 5 files changed, 61 insertions(+), 19 deletions(-) diff --git a/ai/skills/address-pr-reviews/references/plain-writing/SKILL.md b/ai/skills/address-pr-reviews/references/plain-writing/SKILL.md index 3b8ada59..8daef78d 100644 --- a/ai/skills/address-pr-reviews/references/plain-writing/SKILL.md +++ b/ai/skills/address-pr-reviews/references/plain-writing/SKILL.md @@ -29,7 +29,7 @@ When a reference file conflicts with a rule below, follow the reference file. ## Preserve the Source - Keep the requested format. -- When the user wrote the source, preserve every fact, number, condition, qualifier, citation, link, identifier, error string, and technical term that carries exact meaning. Keep the approximate level of detail. Cut repetition, not substance. +- When the user wrote the source, or you can't tell who did, preserve every fact, number, condition, qualifier, citation, link, identifier, error string, and technical term that carries exact meaning. Keep the approximate level of detail. Cut repetition, not substance. - When you or a calling skill wrote the draft, keep only what the reader needs to act or decide. Leave out what they already know, can see in a linked diff or document, or would ask for if they needed it. That usually means a step-by-step mechanism, how other code does the same thing, citations they don't need to act on, and a second statement of a consequence. Never drop a fact that the ask or decision depends on. Every fact you keep stays exact, including its numbers, identifiers, qualifiers, links, and error strings. - Keep code blocks, inline code, commands, quotations, templates, checkboxes, headings, and required labels unchanged unless the user asks to edit them. - Do not add facts, examples, opinions, certainty, humor, personality, or conclusions that are not in the source. diff --git a/ai/skills/address-pr-reviews/references/plain-writing/scripts/plain-writing-lint.py b/ai/skills/address-pr-reviews/references/plain-writing/scripts/plain-writing-lint.py index eab87c1f..2b64725b 100755 --- a/ai/skills/address-pr-reviews/references/plain-writing/scripts/plain-writing-lint.py +++ b/ai/skills/address-pr-reviews/references/plain-writing/scripts/plain-writing-lint.py @@ -13,6 +13,10 @@ SENTENCE_END = r"[.!?]" SENTENCE_OPENING = rf"(?:^\s*(?:(?:[-*+]|\d+[.)])\s+)?|{SENTENCE_END}\s+)" MODAL = r"can|could|will|would|should|must" +CLAUSE_WORD = ( + r"it|this|these|those|there|they|we|you|i|he|she" + r"|what|why|how|when|where|if|whether" +) RAW_PATTERNS: dict[str, tuple[str, ...]] = { "filler": ( @@ -48,16 +52,23 @@ r"\b(?:is|are|was|were)n(?:'|\u2019)t (?:just |only |merely |simply )?about " r"[^.!?;\n]{2,80}?, (?:it|this|that)(?:'|\u2019)s about\b", ), - # The "No" verb list leaves out "change" and "test". The linter therefore does not - # flag "No behavioral changes in this PR" or "No unit tests yet". Some listed verbs - # are also plural nouns. The linter therefore flags "No new uses of the API". - # "No one" reads like "Nobody", which people write. - # "No matter" starts a clause rather than naming a subject. After "Nothing", a word - # that ends in "us" or "ss" is rarely a verb: "Nothing serious", "Nothing less". + # The words between "No" and the verb must form a noun phrase. A pronoun or a + # question word there means "No" starts a clause, as in "No it is not" or "No idea + # what is going on". "To" there starts an infinitive, as in "No need to use it". + # "That" and "who" may follow the first word because they start relative clauses, + # as in "No caller that uses it remains". "No one", "No matter", "No longer", and + # "No doubt" are idioms. + # The verb list leaves out "change" and "test". The linter therefore does not flag + # "No behavioral changes in this PR" or "No unit tests yet". Some listed verbs are + # also plural nouns. The linter therefore flags "No new uses of the API". + # After "Nothing", a word that ends in "us" or "ss" is rarely a verb: "Nothing + # serious", "Nothing less". "As", "besides", and "this" don't start a verb phrase. "negative_subject": ( rf"{SENTENCE_OPENING}Nothing (?:else |in (?:this|the) \w+ )?" - rf"(?:[a-z]+(? None: "No tests exercise this path.", "No tests use the old name.", "No other caller uses it.", + "No other way is possible.", + "No idea is too small.", + "No caller that uses it remains.", + "No user who has access can see it.", + "Nothing is ever easy.", ): with self.subTest(text=text): self.assertIn("negative_subject", categories(lint(text))) @@ -182,21 +187,36 @@ def test_plain_negation_is_not_a_negative_subject(self) -> None: "Nobody is working on this.", "No one has asked.", "No, that's expected.", + "No it is not.", + "No this is expected.", + "No they are not.", + "No I have not.", ): with self.subTest(text=text): self.assertEqual(lint(text), []) - def test_fragment_is_not_a_negative_subject(self) -> None: + def test_fragment_or_idiom_is_not_a_negative_subject(self) -> None: for text in ( "Nothing to note in the release notes.", "Nothing in production.", "Nothing serious, just a nit.", "Nothing less than a rewrite.", + "Nothing as far as I can tell.", + "Nothing besides the docs.", "No unit tests yet.", "No behavioral changes in this PR.", "No breaking changes.", "No idea why this happens.", + "No idea what is going on.", + "No clue what is going on.", + "No doubt this is faster.", + "No way this is right.", + "No wonder this is slow.", + "No surprise there is a bug.", + "No need to touch this file.", + "- No longer depends on the old helper.", "No matter what is chosen, the request retries.", + "Nothing this week.", ): with self.subTest(text=text): self.assertEqual(lint(text), [])