Tier Vale findings in pre-merge reviews: blockers in 🚨, quieter nags, FP fixes - #20663
Open
CamSoper wants to merge 17 commits into
Open
Tier Vale findings in pre-merge reviews: blockers in 🚨, quieter nags, FP fixes#20663CamSoper wants to merge 17 commits into
CamSoper wants to merge 17 commits into
Conversation
… FP fixes Three changes to how Vale prose-lint findings surface in the pinned PR review, all deterministic (no new model calls): 1. Blocker tier. A new `blocker:` allowlist in vale-deterministic-fixes.yaml (Pulumi.Substitutions, Pulumi.Nomenclature, Pulumi.PoliciesSingular, Google.Spelling — near-zero-FP correctness rules) is stamped by vale-findings-filter.py (cap-exempt) and rendered by compose-review.py as [style-blocker] bullets under 🚨 Outstanding with standard **[L<n>]** anchors, so they count toward the review:outstanding-issues label and the auto-refresh gate matches fix-pushes. validate-pinned.py exempts them from trail-matching (Vale findings have no verification-trail record). The triage lane prefixes them [blocker] in the TRIAGE_PROSE comment. 2. Advisory findings quieted. Non-blocker style findings now always render collapsed (per-file <details>) and are excluded from the⚠️ count-table cell — they're kept for the style-rule tuning loop, not reviewer burden. Blocker bullets are outcome-classified by scrape-review-outcomes.py like any 🚨 finding; advisory nags stay counts-only. 3. False positives fixed at the source. HeadingSentenceCase gains multi-word proper-noun exceptions (HashiCorp Vault, AWS Secrets Manager, Azure Key Vault, Google Cloud Secret Manager, 1Password Secrets Automation, CyberArk Conjur, Doppler, Infisical) — verified that generic words like "Secrets"/ "Manager" still flag outside product names. Google.OxfordComma is disabled: its regex can't distinguish serial lists from introductory clauses. Contract copy updated in output-format.md, ci.md, SKILL.md, both workflow prompts, and STYLE-GUIDE.md §Automated checks. Verified: vale 3.14.1 fixture run (FP headings pass, real violations flag); filter/composer/validator dry-runs incl. non-empty-trail case; auto-refresh gate fires on a blocker fix-push; existing scripts test suites pass (65). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RNTs4JzMiG36mQ2ChAyLF4
"Pulumi Service" is always wrong in current content — the correct replacement (Cloud/ESC/Deployments) is context-dependent, which keeps the rule out of deterministic_fix, but shipping the dead name should block like any other product-name error. Also adds the rule to RULE_CATEGORIES (it was unmapped and fell back to the generic "style" category) so its blocker bullets render as "deprecated product name", and syncs the four prose enumerations of the blocker set (output-format.md, both workflow prompts, STYLE-GUIDE.md). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RNTs4JzMiG36mQ2ChAyLF4
Collaborator
|
Your site preview for commit 334307b is ready! 🎉 http://www-testing-pulumi-docs-origin-pr-20663-334307b9.s3-website.us-west-2.amazonaws.com |
The advisory (non-blocker) style findings gain an actionable surface: during the editorial pass, the reviewer triages the collapsed nags and stages the ones whose rewrite is a clear, meaning-preserving improvement into .style-suggestions.json (cap 10; load-bearing hedges like "usually completes in five minutes" and false positives are explicitly excluded). A new deterministic step, post-style-suggestions.py, validates every entry — anchored to a PR-added line (reusing added_lines_per_file from vale-findings-filter.py) and the `original` text present verbatim on that line in the checkout — then posts one event=COMMENT review with a ```suggestion block per finding, so the author can apply all of them with one click via "Add suggestion to batch". Prior suggestion comments are deleted first (delete-and-repost, same as TRIAGE_PROSE), the script always exits 0, and .style-suggestions.json joins the debug artifacts. The collapsed #### Style findings block remains the complete record; a suggested finding keeps its [style] bullet. Blocker findings never render as suggestions. Contract documented in ci.md §3 step 10 and output-format.md; 9 new tests (105 total pass). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RNTs4JzMiG36mQ2ChAyLF4
The 2026-08-03 fork smoke test caught this: post-style-suggestions.py
defaulted --repo to "pulumi/docs" and the workflow invoked it with only
--pr, so on CamSoper/pulumi.docs it POSTed the review to pulumi/docs with
the fork's GITHUB_TOKEN and got 403 "Resource not accessible by
integration". The model had staged a correct sidecar (1 valid suggestion,
0 dropped) that never reached the author. A fork CI probe confirmed the
reviews endpoint itself works fine with GITHUB_TOKEN + pull-requests:
write, isolating the cause to the repo argument.
- Default --repo to $GITHUB_REPOSITORY, falling back to pulumi/docs.
- Pass --repo ${{ github.repository }} explicitly from the workflow.
- Pass --repo through to `gh pr diff` too (it had been resolving from the
local remote by luck).
- Set include-hidden-files on the debug artifact upload: .style-suggestions.json
is a dotfile and was silently dropped from the bundle.
- Regression test pinning the env-derived repo default.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RNTs4JzMiG36mQ2ChAyLF4
Two follow-ups from reviewing the fork smoke-test output. Rename. With correctness errors escalated to the blocker tier and the known false-positive rules disabled, what's left in the advisory block is optional polish, and "findings" oversold it. compose-review.py now emits `#### Style suggestions` (new STYLE_HEADING constant) with a caption to match. validate-pinned.py accepts BOTH spellings (STYLE_HEADINGS) so a re-entrant review merging a pre-rename body still validates, and scrape-review-outcomes.py is unaffected because it keys on the bullet form rather than the heading -- its self-test deliberately keeps the old spelling, since it parses historical merged PRs. No FP narration. The control run in the fork test showed Opus deleting six false-positive heading flags and then writing a paragraph into the pinned comment enumerating them and explaining why -- which re-adds the noise the deletion removed, and already violated DO-NOT item 1. Made it explicit in both workflow prompts, ci.md step 10, and the DO-NOT item: delete false positives silently, with no tally, no rule name, and no explanation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RNTs4JzMiG36mQ2ChAyLF4
The 2026-08-03 re-smoke (fork PR CamSoper#227) authored its own `[style-blocker] _misspelling_` bullet for a "seperate" typo that Vale never reported -- `misspelling` isn't even a category the filter can emit, and Google.Spelling doesn't catch that word. The finding was real and useful, but the provenance was not: `[style-blocker]` is what exempts a 🚨 bullet from trail-matching, so an authored one routes an unverified finding into 🚨 behind a check. Any finding could ride in that way. - New `style-blocker-provenance` validator rule (schema v20): every `[style-blocker]` bullet must match a `blocker: true` entry in `.vale-findings.json` by file and line. Skips when the artifact is absent -- a missing pre-step is not evidence of forgery. A non-blocker finding on the same line does not launder the marker. - Plumb `.vale-findings.json` into Context (defaults to the workspace root, which is where the pre-step already writes it) plus a `--vale-findings` flag. - Both workflow prompts and output-format.md now say plainly: never author the marker; reviewer-found issues go in 🚨 as ordinary `**[L…]**` bullets with a trail record. - 5 regression tests reproducing the exact PR#227 case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RNTs4JzMiG36mQ2ChAyLF4
A suggestion is a whole-line replacement, so one landing on a line that also carries a blocker re-commits the blocking text if the author clicks it. Observed on fork PR #227: the 'Simply' suggestion's replacement line still read "Pulumi Service" and "click", both of which the same review had just flagged in 🚨. post-style-suggestions.py now reads .vale-findings.json and drops any entry whose (file, line) carries a blocker-tier finding, with the reason logged. Nothing is lost — the blocker stays flagged in 🚨 and the author fixes that line there. Non-blocker findings on the line do not trigger the drop, and a missing/unreadable artifact degrades to no filtering. The review prompt now says the same thing so the model doesn't waste an entry, but the deterministic filter is the guarantee. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RNTs4JzMiG36mQ2ChAyLF4
Two review-output tweaks. Drop the batch wording. "Add suggestion to batch" isn't offered in every context the suggestion review renders in, so the body no longer promises it — suggestions are described one at a time. Render the block expanded. Collapsing made sense while style findings still counted toward⚠️ ; now that the count excludes them they cost no review burden, so the disclosure only added a click and hid the entries worth acting on. Bullets group under an `##### <path>` H5 per file. The H5 is load-bearing, not cosmetic: validate-pinned.py's extract_bucket_bullets counts ANY column-0 `**bold**` line as a bucket finding, so a `**<path>**` heading inflated the⚠️ count and tripped the L-prefix rule (caught by the composer's own self-check while building this). ✏️ marks bullets with an applyable suggestion. The mark is appended by post-style-suggestions.py --annotate-draft from the set the GitHub API actually accepted, so it can't advertise a button that isn't there. That required moving the posting step ahead of the pinned upsert (D3, was after E); the ordering is documented at both ends. 118 tests pass, including 4 new annotator cases (same-line-across-files disambiguation, idempotency, empty/missing draft). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RNTs4JzMiG36mQ2ChAyLF4
A published review carried 19 '…' markers, several cutting mid-word: 'claim asserts you reuse the "same IAM policies you have al…'. That reads as a rendering bug and drops the clause that would have made the sentence land. trunc() now backs up to the last space in the final quarter of the budget, hard-cutting only when there isn't one (long URLs), and trims a trailing separator before the ellipsis. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RNTs4JzMiG36mQ2ChAyLF4
Measuring a published review (fork PR #228, 16.1 KB body) put the evidence/source/intuition tail on 🚨 and⚠️ bullets at 1,596 chars — 10% of the whole comment — and every character of it was already rendered verbatim on that claim's 🔍 trail line a few lines above. Worse, it sat between the quoted claim and the fix prose, so the actionable half of each finding started below the fold. The stub now carries the claim, the verdict, and the `framing:` note (which the editorial pass is told to mirror for the ⚔️ mismatch anti-hedge) — nothing else. A representative bullet drops from ~880 to 361 chars. Nothing is weakened: every evidence-checking rule (pass-3-unverifiable-evidence, pass-3-evidence-faithful, verified-claims-trail-faithful) reads the trail, not the bucket bullet. Verified before making the cut. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RNTs4JzMiG36mQ2ChAyLF4
Fork PR #229 shipped four correct ✏️ marks that the workflow did not write — the log read `marked 0 style bullet(s)`. The editorial pass had added them itself, mid-line, and the annotator's idempotency guard then saw a mark already present and skipped every bullet. They matched what posted, but only by luck: the model staged four and all four survived validation. A mark it writes for an entry later dropped by the blocker-line filter, an anchor mismatch, or the cap would promise a one-click button that isn't there — the exact failure the mark exists to prevent, and the same shape as the forgeable [style-blocker] marker. annotate_draft now strips every existing mark from style bullets first and re-applies only for entries the GitHub API accepted, so a model-authored mark is corrected rather than trusted. Its return value is now "marks present" (final state) rather than "newly added"; the idempotency test asserts the body is byte-identical on a second pass. The CI review prompt and ci.md now say plainly not to write the mark, and why the model can't know which entries survive. The deterministic overwrite is the guarantee; the prompt just stops the wasted effort. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RNTs4JzMiG36mQ2ChAyLF4
Batching IS offered on the suggestion comments — "Add suggestion to batch" plus "Commit suggestions" stages several into one commit — so the earlier removal of that wording was wrong. Both the pinned caption and the suggestion review body now say so. The caption also deep-links to the PR's Files-changed tab, since that is where the ```suggestion blocks actually render; a ✏️ in the pinned comment otherwise leaves the reader to work out where to look. The URL is built from --repo/--pr and falls back to unlinked prose when either is absent (local /docs-review runs have neither). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RNTs4JzMiG36mQ2ChAyLF4
Probed the API directly on the fork: POST /pulls/{n}/reviews with a
comments[] array is atomic. One unresolvable anchor returns `422 Line
could not be resolved` and creates NOTHING — there is no partial
acceptance. So a single bad entry silently cost every suggestion in the
run, and the script just logged and exited 0.
On a batch failure it now resolves the head SHA and retries one comment
at a time via POST /pulls/{n}/comments, so a bad anchor costs only itself.
The ✏️ marks are driven by the set that actually landed, which is now a
genuinely partial set rather than all-or-nothing.
This also corrects the rationale recorded earlier: local validation isn't
just tidiness, it is what keeps one malformed entry from sinking the
batch.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RNTs4JzMiG36mQ2ChAyLF4
claude-update.yml told the reviewer both that a mark "keeps it only if the suggestion is still posted" AND that it must "not add or remove it yourself" — contradictory, and the second half was wrong for that lane. post-style-suggestions.py is wired only into claude-code-review.yml, so #update-review never re-posts suggestions and never runs the annotator. Under the old wording the marks silently vanished on every refresh even though the suggestion comments were still live on the PR — the author refreshed the review and lost the pointers to their own buttons. Because that lane leaves the comments untouched, carrying marks across is the correct behavior: a finding that persists still has its posted suggestion, and a finding that disappeared took the (now outdated) comment with it. The rule now says exactly that, and forbids minting a mark for a finding that never carried one. Documented the lane split in output-format.md so the asymmetry is discoverable rather than folklore. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RNTs4JzMiG36mQ2ChAyLF4
The ✏️ marks are the only evidence a suggestion exists, and they live in the last section of a comment that routinely runs 16 KB — below every finding the author actually has to fix. An author who clears 🚨 and stops reading never learns the buttons are there. post-style-suggestions.py --annotate-draft now also writes a one-line banner directly under the count row, deep-linked to Files changed. Like the marks, it's rewritten from the set the API accepted on every run and stripped when nothing posts, so it can't outlive its buttons: a refresh that converts nothing must not leave "4 suggestions are posted inline" standing over zero comments. Two supporting changes make that true on the re-entrant lane, where the draft is the previously published body rather than a fresh compose: - annotate now runs on every path, including zero-posted. It used to return early, which would have left last run's marks and banner in a body whose suggestion comments had just been deleted. - a missing or unreadable sidecar is treated as "no suggestions this run" instead of "skip the run", for the same reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#update-review re-ran Vale but never re-posted the suggestions, so the marks in the refreshed body were hand-carried by the model against comments from an arbitrarily old run. A finding the author fixed kept its ✏️ (its button now outdated), a newly-surfaced one could never gain one, and a mark the model minted pointed at nothing. Both lanes now post every run with the same delete-and-repost semantics. The re-entrant lane can't use --annotate-draft, though: it has no draft to intercept, since the model renders the body and upserts it inside its own step. So annotation is factored into annotate_text() and a new --annotate-pinned fetches the published <!-- CLAUDE_REVIEW N/M --> comments and PATCHes each in place. In place, rather than fetch → concatenate → re-upsert, because re-upserting re-runs the splitter over a body that already carries the splitter's own artifacts — the synthetic </details> and continuation <details> pairs it inserts at page boundaries — which compounds on every refresh. Editing each part touches only the lines the marks and banner live on, and skips the PATCH entirely when a part is already correct so a refresh doesn't re-notify subscribers. The prompt change is the other half: the model is now told to drop every ✏️ it finds and stage the full qualifying set each run, rather than carrying marks across. Same lesson as the two before it — the mark is a promise that a button exists, and only the step that posted can keep it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Observed on the two-lane fork smoke (CamSoper#230): line 768 carried two weasel-word findings, one suggestion posted for it, and both bullets got a mark — 4 marks against a banner reading 3. That is correct behavior, since a suggestion replaces the whole line, but the contract never said so. Says so now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CamSoper
marked this pull request as ready for review
August 5, 2026 00:53
github-actions
Bot
added
review:triaging
Contributor
Pre-merge Review — Last updated 2026-08-05T00:58:20ZTip Summary: This is an infrastructure PR on the review pipeline itself — no reader-facing content changes. It splits Vale findings into a blocker tier that renders in 🚨 Outstanding (a new Review confidence:
Investigation log
🔍 Verification trail41 claims extracted · 2 verified · 0 unverifiable · 0 contradicted
🚨 Outstanding in this PRThese must be resolved or refuted before merging.
|
github-actions
Bot
added
review:outstanding-issues
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Restructures how Vale prose-lint findings surface in the pinned pre-merge review. The tiering/FP changes are deterministic config/Python with no new model calls; the inline-suggestions feature adds a small amount of work to the existing Opus editorial pass (≤10 one-line rewrites) and one API call.
1. Blocker tier — high-confidence Vale findings now block. A new
blocker:allowlist invale-deterministic-fixes.yaml(Pulumi.Substitutions,Pulumi.Nomenclature,Pulumi.PoliciesSingular,Google.Spelling,Pulumi.DeprecatedProductNames— wrong or deprecated product names, banned terms, misspellings, agreement errors; near-zero false-positive rate) is stamped onto findings byvale-findings-filter.py(exempt from the per-file/total caps) and rendered bycompose-review.pyas[style-blocker]bullets under 🚨 Outstanding with standard**[L<n>]**anchors. They count in the 🚨 cell, drive thereview:outstanding-issueslabel, participate in the auto-refresh gate on fix-pushes, and are outcome-classified by the review telemetry. The model may not delete or demote them. The triage lane surfaces them with a[blocker]prefix in theTRIAGE_PROSEcomment.Blocker membership is independent of
deterministic_fix:PoliciesSingularandDeprecatedProductNamesblock even though their fixes need authoring ("Pulumi Service" is always wrong; the right replacement — Cloud/ESC/Deployments — is the author's call).DeprecatedProductNameswas also missing fromRULE_CATEGORIESand rendered as generic "style"; it now renders as "deprecated product name".2. Advisory findings quieted, but actionable. Non-blocker style findings now always render collapsed (per-file⚠️ count-table cell — kept for the style-rule tuning loop, not reviewer burden. Additionally, the editorial pass triages the advisory findings and stages the ones whose rewrite is a clear, meaning-preserving improvement into
<details>) and are excluded from the.style-suggestions.json(cap 10; false positives and load-bearing hedges — "usually completes in five minutes" — are explicitly excluded). A new deterministic step (post-style-suggestions.py) validates every entry against the PR diff and the checked-out file (anti-hallucination: the anchor must be a PR-added line and theoriginaltext must be present verbatim), then posts oneevent: COMMENTreview with asuggestionblock per finding — one-click applyable, batchable into a single commit, and deleted-and-reposted per run so no stale comments accumulate. The script never fails the workflow.3. False positives fixed at the source.
HeadingSentenceCasegains multi-word proper-noun exceptions (HashiCorp Vault,AWS Secrets Manager,Azure Key Vault,Google Cloud Secret Manager,1Password Secrets Automation,CyberArk Conjur,Doppler,Infisical) — phrase entries so generic words like "Secrets"/"Manager"/"Key" still flag when capitalized outside a product name (verified against fixtures).Google.OxfordCommais disabled with a rationale comment: its regex can't distinguish serial lists from introductory clauses ("…the deciding factors, Doppler and…"), joining the nine other Google rules already disabled for FP rate.Contract copy is updated everywhere it's duplicated:
output-format.md,ci.md(including new editorial-pass step 10 for the suggestion sidecar),SKILL.md, the prompts inclaude-code-review.ymlandclaude-update.yml, the validator rules (count-table-matches-bullets,style-render-mode, trail-match exemption),scrape-review-outcomes.py, andSTYLE-GUIDE.md§Automated checks.Verification done:
DeprecatedProductNamesstampsblocker: true,deterministic_fix: false.[style-blocker]bullets that have no trail records (the exemption path), and still enforces counts.auto-refresh-gate.pyfires on a push fixing a blocker's line.post-style-suggestions.py: 9 new tests covering validation (missing text, non-added line, duplicates, cap, payload shape) and the dry-run end-to-end path.Unreleased product version (optional)
N/A
Related issues (optional)
N/A