feat: wire Vale prose linter into docs CI#25467
Conversation
Lands the Vale prose linter as a non-blocking docs CI step. Builds on the scaffold from DOCS-180 (#25466): - .vale.ini at the repo root configures Google + curated write-good + cherry-picked alex rules. Disables Google.Spacing (false positives on codersdk type names in the auto-generated API reference), Google.EmDash (conflicts with our em-dash ban), Google.Latin (i.e. and e.g. are fine), write-good.Passive and write-good.E-Prime (judgment-heavy). - mise.toml pins Vale 3.7.1 via aqua. - Makefile adds build/vale-$VERSION install, docs/.style/.vale-synced sentinel that runs 'vale sync' once per .vale.ini change, and a lint/prose target wrapped in '|| true' for v1 non-blocking severity. - .github/workflows/docs-ci.yaml adds a 'prose' step that lints only the changed Markdown files under docs/, with continue-on-error: true and a cache for the synced styles and binary. - .gitignore excludes the synced upstream styles and the sentinel. - .markdownlint-cli2.jsonc ignores the synced styles so local markdownlint runs do not lint upstream READMEs. - docs/.style/README.md and style-guide.md document how to run Vale locally and what the active rule set is. Severity policy (v1): every rule lands at 'warning'. CI is non-blocking through continue-on-error: true. A rule promotes to 'error' only when (a) it is objectively correct and (b) the existing-content violation count reaches zero. Judgment rules stay at 'suggestion'. Local run on the full docs corpus produces 0 blocking failures, 391 errors, 5202 warnings, 7682 suggestions across 458 files in ~20s. Refs DOCS-40.
Docs preview📖 View docs preview for |
|
/coder-agents-review This PR adds a new docs-ci.yaml step, a new Makefile target, and a new mise.toml pin, so it is squarely "CI changes in a meaningful way." Focus areas:
Base branch is Filed via Coder Agents on Nick's behalf. |
… filter The 'docs/**.md' glob in tj-actions/changed-files skips dot-prefixed directories by default, so the changed-md-docs filter silently dropped docs/.style/README.md and docs/.style/style-guide.md. The Vale prose step never fired on PR #25467 as a result. Drop the second changed-files step and post-filter the changed-md output in shell. grep '^docs/' keeps only docs paths; grep -v '^docs/.style/styles/' excludes the synced upstream packages. The early exit handles PRs that only touched non-docs markdown. Refs DOCS-40.
Vale's release archive places 'vale' at the archive root with no leading ./ (unlike typos), so 'tar -xzf - ./vale' matched nothing and produced 'tar: ./vale: Not found in archive' on CI. Switch to 'vale'. The local Makefile invocation worked before because build/vale-3.7.1 was already present from the spike artifacts. CI hit the cold path and exposed the bug.
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-structured linter integration. The inline comments in .vale.ini justifying each enable/disable decision set a high bar for configuration documentation. The Makefile patterns mirror lint/typos cleanly, the CI caching is thoughtful, and the decision log in the PR description is unusually thorough.
As Hisoka put it: "Two config points that look independent, secretly attached through Vale's exit-code semantics. Whoever follows the documented path gets a surprise."
Severity breakdown: 4 P2, 5 P3, 1 P4, 2 Nit.
The P2 findings cluster around two themes: (1) Vale's exit-code behavior is different from what the PR documents (only errors cause non-zero exit, not warnings/suggestions), which inverts the described promotion path; and (2) operational blind spots where continue-on-error swallows infrastructure failures alongside lint warnings.
The P3 findings are mostly documentation accuracy issues and a maintenance friction point (package names hardcoded in 4 files that could use wildcards).
🤖 This review was automatically generated with Coder Agents.
|
|
||
| ```shell | ||
| make docs/.style/.vale-synced | ||
| ./build/vale-*/vale docs/ # or pass specific files |
There was a problem hiding this comment.
P2 [DEREM-1] The command ./build/vale-*/vale docs/ treats the binary as a directory. The Makefile installs a flat binary at build/vale-3.7.1 (via mv build/vale "$@"), not a directory containing a vale file. The glob ./build/vale-*/vale expands to ./build/vale-3.7.1/vale, which doesn't exist. The CI step gets this right with find build/ -maxdepth 1 -name 'vale-*' -type f. (Netero P3, Mafu-san P2)
"The CI implementation uses
findto locate the binary rather than the fragile glob the docs recommend. The implementation was written with more care than the documentation." -- Mafu-san
Fix: ./build/vale-* docs/ (treat the glob match as the binary itself).
🤖
There was a problem hiding this comment.
Fixed in e49b5cc. ./build/vale-*/vale → ./build/vale-* in docs/.style/README.md. The glob now expands to the binary itself, matching the Makefile install layout (mv build/vale "$@"). Verified ./build/vale-* docs/ runs locally.
Filed via Coder Agents on Nick's behalf.
|
|
||
| The canonical entry point is `make lint/prose`. The first run downloads | ||
| the pinned Vale binary and the configured style packages; subsequent | ||
| runs reuse them. The target wraps Vale in `|| true` so warnings do not |
There was a problem hiding this comment.
P3 [DEREM-2] "The target wraps Vale in || true so warnings do not break make lint" is false. lint/prose is not a prerequisite of lint: (lint: lint/shellcheck lint/go lint/ts lint/examples lint/helm lint/site-icons lint/markdown lint/check-scopes lint/migrations lint/bootstrap lint/architecture lint/emdash lint/agents $(LINT_ACTIONS_TARGETS)). The || true wrapper keeps make lint/prose from exiting non-zero when invoked standalone. make lint is unaffected regardless. (Netero P3, Mafu-san P2, Leorio P3)
"Two places state the relationship correctly [the Makefile comment and decision log]. Two other places state it incorrectly. The correct information existed in the implementation; the docs asserted a dependency relationship without checking the prerequisite list." -- Mafu-san
Fix: "The target wraps Vale in || true so the target exits cleanly despite warnings."
🤖
There was a problem hiding this comment.
Fixed in e49b5cc. The inaccurate make lint claim is gone. docs/.style/style-guide.md's "Running Vale locally" section now cross-references README.md#running-vale-locally (single source of truth) instead of restating the rationale and getting the prereq relationship wrong. This also resolves DEREM-8.
Filed via Coder Agents on Nick's behalf.
| # stay visible in the step log; promote rules to `error` (and drop | ||
| # continue-on-error) once their existing-content violation count | ||
| # reaches zero. See DOCS-40. | ||
| - name: prose | ||
| if: steps.changed-md-docs.outputs.any_changed == 'true' | ||
| continue-on-error: true | ||
| run: | | ||
| make docs/.style/.vale-synced | ||
| vale=$(find build/ -maxdepth 1 -name 'vale-*' -type f -print -quit) | ||
| printf '%s\n' "$ALL_CHANGED_FILES" | tr ',' '\n' | xargs -d '\n' "$vale" | ||
| env: | ||
| ALL_CHANGED_FILES: ${{ steps.changed-md-docs.outputs.all_changed_files }} |
There was a problem hiding this comment.
P2 [DEREM-3] continue-on-error: true blankets infrastructure setup and lint in one step. The prose step runs three shell commands: make docs/.style/.vale-synced (download binary + vale sync), find (resolve binary path), and xargs vale (lint). GitHub Actions runs bash with set -eo pipefail. If command 1 fails (curl 404, corrupt tarball, network error), bash aborts, continue-on-error turns the step green, and Vale never executes. Nobody knows the linter didn't run. (Hisoka P2)
"The intent is to absorb lint warnings. The implementation also absorbs: binary download failure, style-sync failure,
findresolving to empty, Vale crashing. All look identical at the job level." -- Hisoka
Fix: split into two steps. Step 1 (no continue-on-error) runs make docs/.style/.vale-synced. Step 2 (continue-on-error: true) resolves the binary and runs Vale. Infra failures break the build; lint warnings don't.
🤖
There was a problem hiding this comment.
Fixed in e49b5cc. The single prose step is split into two:
- Prepare Vale styles runs
make docs/.style/.vale-syncedwith nocontinue-on-error. Curl 404, corrupt tarball, network error, sync failure, all surface as a red job. - prose runs
vale --no-exit --output=line, also with nocontinue-on-error. Vale's--no-exitsuppresses the alert-triggered non-zero exit, while real failures (missing binary, bad config) still propagate as a red job.
This closes Hisoka's failure mode: anything between "binary download" and "linter actually runs" now breaks CI loudly.
Filed via Coder Agents on Nick's behalf.
| # Severity policy (v1): every rule lands at `warning`. CI is non-blocking | ||
| # (Makefile wraps Vale in `|| true`). A rule promotes to `error` only after | ||
| # (a) it is objectively correct and (b) the existing-content violation count | ||
| # reaches zero. Judgment rules (Wordiness, Weasel, ThereIs) stay at | ||
| # `suggestion` and never promote to `error`. |
There was a problem hiding this comment.
P2 [DEREM-4] The severity policy and promotion path are built on a wrong model of Vale's exit-code behavior. Per Vale's docs: "error-level alerts will result in a non-zero exit code, while warning- and suggestion-level alerts will not." MinAlertLevel controls what is displayed, not the exit code threshold. Only error-level alerts cause non-zero exit, regardless of MinAlertLevel. (Pariston P2, Leorio P3, Razor P3, Gon P3, Mafuuu Nit)
This means:
- The 391 errors in the test output come from un-overridden Google rules at their package-default error level, not from warnings.
|| trueis needed because of those 391 errors, not because warnings trigger non-zero exit.- The documented promotion path ("promote rules to error, then remove
|| true") is backwards. The actual path is: fix or override the existing error-level violations, then|| truebecomes unnecessary. - "Every rule lands at
warning" is also false: three rules land atsuggestion(Google.Parens,write-good.TooWordy,write-good.ThereIs), and the header itself contradicts this four lines later.
"The practical consequence: a future maintainer following the documented promotion path would try to promote a warning rule to error, fix its violations, and expect to be closer to removing
|| true. They would discover that|| trueis still needed because of the 391 pre-existing errors from other Google rules." -- Pariston
PS. Vale supports --no-exit, which suppresses only the alert-triggered exit code. Using vale --no-exit instead of || true would preserve failures from missing config, missing binary, or other non-alert errors.
🤖
There was a problem hiding this comment.
Confirmed correct, fixed in e49b5cc. The previous severity-policy comment was wrong about Vale's exit-code model, and Pariston's read of the data was right: I verified vale 3.14.1 locally and MinAlertLevel only changes display, not exit code.
Changes:
- Makefile:
|| true→--no-exit. Same non-blocking effect for alert-level exits, but missing binary / bad config / missing files still propagate. .vale.inianddocs/.style/style-guide.md: severity-policy comments rewritten. Both now state the correct model: Vale exits non-zero only on error-level alerts regardless ofMinAlertLevel; the 391 baseline errors come from un-overridden Google rules that ship aterror; promotion path is "drive existing error-rule violations to zero, then drop--no-exit," not "promote a warning to error."- Severity policy reframed around the framework Nick uses: severity = f(false-positive rate, gravity).
errorrequires low FPs and high gravity; lower gravity or judgment calls drop towarningorsuggestion. The doc no longer claims "every rule lands at warning" (it never did, three rules ship atsuggestion).
Filed via Coder Agents on Nick's behalf.
| Packages = Google, alex, write-good | ||
|
|
||
| [*.md] | ||
| BasedOnStyles = Google, write-good |
There was a problem hiding this comment.
P2 [DEREM-5] BasedOnStyles = Google, write-good omits Coder. The parent PR's docs/.style/styles/Coder/README.md tells contributors: "Each rule is a YAML file that Vale loads through the BasedOnStyles = Coder setting in the repo-root .vale.ini." That setting does not exist. When a contributor creates docs/.style/styles/Coder/ProductVoice.yml following that guide, Vale silently ignores it. No error, no warning. (Mafuuu P2, Leorio P3)
"Adding
Coderto the list now is a no-op (no.ymlfiles exist in the directory), and it makes the future contributor workflow work without a hidden.vale.iniedit." -- Mafuuu
This PR's style-guide.md (line 122) also lists "Coder (custom): empty in v1" under "Active rule set" without noting that the directory is not loaded, reinforcing the false impression.
Fix: BasedOnStyles = Google, write-good, Coder
🤖
There was a problem hiding this comment.
Fixed in e49b5cc. .vale.ini now reads BasedOnStyles = Google, write-good, Coder. Vale 3.14.1 accepts the empty docs/.style/styles/Coder/ directory without complaint (no "missing rule files" error), so contributors who land their first rule per docs/.style/styles/Coder/README.md will see it work immediately, no hidden .vale.ini edit required. Local vale --no-exit exit code and alert counts unchanged after the addition.
Follow-up: Mafuuu also flagged that docs/about/contributing/documentation.md should redirect to docs/.style/style-guide.md. Tracked in DOCS-186; out of scope for this PR.
Filed via Coder Agents on Nick's behalf.
| ### Running Vale locally | ||
|
|
||
| The canonical entry point is `make lint/prose`. The first run downloads | ||
| the pinned Vale binary and the configured style packages; subsequent | ||
| runs reuse them. The target wraps Vale in `|| true` so warnings do not | ||
| break `make lint`, matching the v1 non-blocking policy. |
There was a problem hiding this comment.
P3 [DEREM-8] "Running Vale locally" is documented in both README.md:70-89 (same directory) and here. The two copies have already diverged within this PR: README includes the broken ./build/vale-*/vale glob (DEREM-1); this version omits it. Two copies of one procedure in the same directory. (Robin Nit, Razor Nit, Zoro P3)
Fix: replace this section with a cross-reference to README.md#running-vale-locally.
🤖
There was a problem hiding this comment.
Fixed in e49b5cc. The duplicate "Running Vale locally" block in style-guide.md is gone; the section is now a one-paragraph cross-reference to README.md#running-vale-locally. Single source for the procedure.
Filed via Coder Agents on Nick's behalf.
| continue-on-error: true | ||
| run: | | ||
| make docs/.style/.vale-synced | ||
| vale=$(find build/ -maxdepth 1 -name 'vale-*' -type f -print -quit) | ||
| printf '%s\n' "$ALL_CHANGED_FILES" | tr ',' '\n' | xargs -d '\n' "$vale" | ||
| env: | ||
| ALL_CHANGED_FILES: ${{ steps.changed-md-docs.outputs.all_changed_files }} |
There was a problem hiding this comment.
P3 [DEREM-9] Vale's default CLI output is a human-readable table. GitHub Actions does not auto-annotate it. Without --output=line and a problem matcher, warnings sit in the step log behind a click-through on a yellow continue-on-error badge. Four friction points between the linter and the developer: notice the yellow icon, click into it, scroll the log, care about non-required warnings. (Luffy P3)
"Adding
--output=lineplus a problem matcher would surface warnings as inline PR annotations for free. Without that, the CI integration is ceremony." -- Luffy
🤖
There was a problem hiding this comment.
Fixed in e49b5cc. Added .github/vale-problem-matcher.json (owner vale, severity warning, regex ^(.+):(\d+):(\d+):([^:]+):(.+)$) and wired the prose step:
echo "::add-matcher::.github/vale-problem-matcher.json"
printf '%s\n' "$files" | xargs -d '\n' "$vale" --no-exit --output=line
echo "::remove-matcher owner=vale::"--output=line emits path:line:col:rule:message, which the regex parses. ::remove-matcher:: after the command keeps the matcher from leaking into later steps. Verified the format against local vale --no-exit --output=line output. Future PRs that touch docs/ will surface alerts as inline PR annotations, no log scrolling required.
Filed via Coder Agents on Nick's behalf.
|
|
||
| ### Active rule set | ||
|
|
||
| The curated set documented in `.vale.ini`: |
There was a problem hiding this comment.
P4 [DEREM-10] This section restates every rule enable, disable, and justification from .vale.ini's inline comments. Same rules, same rationale, same exceptions. Line 109 acknowledges this ("The curated set documented in .vale.ini:") and then duplicates it. When a rule is toggled in .vale.ini, the inline comment updates with the change; this section silently goes stale. (Robin P3, Zoro P4)
Consider replacing the inventory with a policy summary and a pointer to .vale.ini: "The curated set is documented in .vale.ini's inline comments; run make lint/prose to see it in action."
🤖
There was a problem hiding this comment.
Fixed in e49b5cc. The Active rule set section is now a policy summary plus a pointer to .vale.ini:
The curated set lives in
.vale.ini's inline comments. Runmake lint/proseto see it in action.
Then four bullets that describe the high-level shape (Google base, write-good for wordiness, alex a la carte, Coder for custom rules) without restating individual enables, disables, or rationale. .vale.ini's inline comments stay the single source. Robin and Zoro are correct: the previous inventory was an obvious source of drift.
Filed via Coder Agents on Nick's behalf.
| # `vale sync` pulls the packages listed in .vale.ini's Packages directive | ||
| # into StylesPath (docs/.style/styles/). The .vale-synced sentinel makes | ||
| # sync idempotent across `make lint/prose` calls and lets warm checkouts | ||
| # skip the re-sync entirely. Touch any time .vale.ini changes to force a |
There was a problem hiding this comment.
Nit [DEREM-11] "Touch any time .vale.ini changes to force a re-sync" reads as a manual instruction, but Make already rebuilds the sentinel when .vale.ini changes (line 877: docs/.style/.vale-synced: .vale.ini build/vale-$(VALE_VERSION)). If someone reads "Touch" as touch docs/.style/.vale-synced, that makes the sentinel newer and skips the re-sync. (Gon Nit, Leorio P3)
Fix: "Make rebuilds this target when .vale.ini changes."
🤖
There was a problem hiding this comment.
Fixed in e49b5cc. The misleading "Touch any time .vale.ini changes to force a re-sync" is gone; the comment now reads:
Make rebuilds this target when
.vale.inichanges.
No verb that could be mistaken for touch docs/.style/.vale-synced (which would skip the re-sync, the opposite of the intent).
Filed via Coder Agents on Nick's behalf.
|
|
||
| # Google.Spacing flags ASCII codepoint sequences like `k.R` or | ||
| # `codersdk.SomeType` as "should have one space." It produces ~4,500 | ||
| # errors against `docs/reference/api/schemas.md` alone (Vale spike, |
There was a problem hiding this comment.
Nit [DEREM-12] "(Vale spike, 2026-05-18)" uses unexplained jargon. "Spike" is Agile shorthand for a timeboxed investigation. "(measured 2026-05-18)" is unambiguous to anyone reading the config. (Gon)
🤖
There was a problem hiding this comment.
Fixed in e49b5cc. (Vale spike, 2026-05-18) → (measured 2026-05-18). No Agile jargon, same factual claim.
Filed via Coder Agents on Nick's behalf.
Resolves the 12 findings from review id 4314529409 on PR #25467. DEREM-1 (P2): fix `./build/vale-*/vale` glob in docs/.style/README.md; the binary is the file itself, not a directory. DEREM-2 (P3): drop the inaccurate `make lint` claim in style-guide.md; point readers at README's Running Vale section instead. DEREM-3 (P2): split the prose step into `Prepare Vale styles` (no continue-on-error) and `prose` (no continue-on-error; --no-exit), so the job fails on sync failures but lints non-blocking. DEREM-4 (P2): replace `|| true` with Vale's native --no-exit in the Makefile and rewrite the severity-policy comment to match the measured exit-code semantics (Vale exits non-zero only on error alerts, regardless of MinAlertLevel). DEREM-5 (P2): add `Coder` to BasedOnStyles so the empty starter style is loaded and ready to receive rule files. DEREM-6 (P3): replace hardcoded per-package paths in .gitignore and the workflow cache step with `styles/*` plus a `!styles/Coder` negation, reducing the places future packages need to be listed. DEREM-7 (P3): already addressed by c3e9e2f (prose filter matches docs/.style paths) and 6054d9e (tar layout). DEREM-8 (P3): remove the duplicate Running Vale block in style-guide.md and cross-reference README.md as the single source. DEREM-9 (P3): add .github/vale-problem-matcher.json and wire the prose step with `::add-matcher::`/`--output=line`/`::remove-matcher::` so alerts surface as inline PR annotations. DEREM-10 (P4): trim the Active rule set section in style-guide.md to a policy summary plus a pointer to .vale.ini. DEREM-11 (Nit): rephrase the Makefile sync-sentinel comment to describe Make's behavior instead of suggesting `touch`. DEREM-12 (Nit): swap "(Vale spike, 2026-05-18)" for the more neutral "(measured 2026-05-18)" in the Google.Spacing rationale. Two-layer defense on the cache step protects the hand-authored Coder rules: the negation excludes styles/Coder from the cached paths, and the cache key hashes styles/Coder/** so any rule change invalidates the cache even if the negation behavior ever regresses (actions/toolkit#713, actions/cache#494). Verification: - make lint/actions: clean (zizmor, actionlint). - make fmt/markdown: no changes. - make lint/markdown: 0 errors across 463 files. - make lint/prose: exit 0; baseline 391 errors / 5201 warnings / 7673 suggestions across 458 files (consistent with the pre-review run).
Review iteration: addressed all 12 findingsCommit
Verification
Follow-ups (out of scope for this PR)
Filed via Coder Agents on Nick's behalf. |
|
/coder-agents-review Filed via Coder Agents on Nick's behalf. |
There was a problem hiding this comment.
All 12 R1 findings addressed in e49b5cc. The fixes are thorough: the exit-code model is correct in .vale.ini, Makefile, and style-guide.md; the CI step split cleanly separates infra failures from lint warnings; the problem matcher surfaces annotations inline; the wildcard gitignore and cross-referenced docs eliminate maintenance coupling.
Two locations still carry the pre-correction --no-exit explanation (DEREM-14, DEREM-15), and the CI shell filter has a semantic gap (DEREM-13). Severity: 2 P3, 1 Nit.
As Kite noted: "The block comment explaining Vale's CI behavior is unusually thorough. It documents the split-step rationale, the --no-exit semantics, the problem matcher lifecycle, and the cache negation defense-in-depth strategy with specific issue citations."
Process note: the PR description still describes the removed || true and continue-on-error: true mechanisms. The code comments are all correct, but the description will persist as the merge commit summary. Consider updating the description to reflect the current --no-exit + step-split architecture before merge.
🤖 This review was automatically generated with Coder Agents.
| files=$(printf '%s\n' "$ALL_CHANGED_FILES" \ | ||
| | tr ',' '\n' \ | ||
| | grep -E '^docs/' \ | ||
| | grep -v '^docs/\.style/styles/' || true) |
There was a problem hiding this comment.
P3 [DEREM-13] grep -v '^docs/\.style/styles/' is redundant for synced packages (they're gitignored, so tj-actions/changed-files will never list them) and silently excludes tracked files under docs/.style/styles/Coder/. The .gitignore, cache config, and cache key all carefully preserve the Coder directory, but this filter undoes that for Vale linting. (Netero P3)
Fix: remove the grep -v line entirely (gitignore already excludes synced packages), or narrow it to grep -v '^docs/\.style/styles/\(Google\|alex\|write-good\)/'.
🤖
There was a problem hiding this comment.
Fixed in 0048444. The grep -v '^docs/\.style/styles/' filter is gone. Synced upstream packages are gitignored, so tj-actions/changed-files never lists them; the only thing the filter ever rejected was tracked content under docs/.style/styles/Coder/ (e.g. our own README, plus any future Coder/**.md). Net effect: Vale now lints every tracked Markdown file under docs/ that a PR touches, including Coder/README.md.
I considered narrowing to grep -v '^docs/\.style/styles/\(Google\|alex\|write-good\)/' but that would re-introduce exactly the hardcoded package list DEREM-6 set out to remove. The inline comment now documents why no second filter exists.
Verified with make lint/prose: baseline unchanged (391 errors / 5201 warnings / 7673 suggestions). CI on commit 0048444e99 will exercise the new filter end-to-end.
Filed via Coder Agents on Nick's behalf.
| make lint/prose | ||
| ``` | ||
|
|
||
| The target uses Vale's `--no-exit` flag so warning- and suggestion-level |
There was a problem hiding this comment.
P3 [DEREM-14] The --no-exit explanation says "warning- and suggestion-level alerts do not produce a non-zero exit." Warnings and suggestions never produce a non-zero exit, with or without --no-exit. Vale exits non-zero only on error-level alerts. --no-exit suppresses the exit from the baseline error count (un-overridden Google rules). (Zoro P3, Hisoka Nit)
"The sibling file
style-guide.md:108gets this right: 'Vale exits non-zero only on error-level alerts, regardless ofMinAlertLevel.' A reader going fromstyle-guide.mdtoREADME.mdencounters contradictory explanations of the same mechanism." -- Zoro
This is a DEREM-4 fix-chain gap: the correction propagated to .vale.ini, Makefile, and style-guide.md but missed this file.
Fix: "The target uses Vale's --no-exit flag so the baseline error count from un-overridden Google rules does not produce a non-zero exit; real failures (missing binary, bad config) still propagate."
🤖
There was a problem hiding this comment.
Fixed in 0048444. Confirmed gap in the DEREM-4 fix chain. docs/.style/README.md now matches the corrected model used in .vale.ini, the Makefile, and style-guide.md:
The target uses Vale's
--no-exitflag so the baseline error count from un-overridden Google rules does not produce a non-zero exit; real failures (missing binary, bad config) still propagate.
Zoro's catch on the cross-file inconsistency was the key signal. The four DEREM-4 fix-targets now agree.
Filed via Coder Agents on Nick's behalf.
| # handles PRs that only touched non-docs markdown like AGENTS.md | ||
| # or README.md. | ||
| # | ||
| # The step uses `vale --no-exit` so warning- and suggestion-level |
There was a problem hiding this comment.
Nit [DEREM-15] Same incorrect --no-exit model as DEREM-14: "vale --no-exit so warning- and suggestion-level alerts do not fail CI." --no-exit suppresses error-level exits, not warning/suggestion exits. The Makefile comment gets this right. (Zoro P3, Hisoka Nit)
Fix: "vale --no-exit so the baseline error count from un-overridden Google error-level rules does not fail CI, while real failures (missing binary, bad config) still propagate."
🤖
There was a problem hiding this comment.
Fixed in 0048444. Same exit-code model now in the docs-ci.yaml prose-step comment:
The step uses
vale --no-exitso the baseline error count from un-overridden Google error-level rules does not fail CI, while real failures (missing binary, bad config) still propagate.
All five locations that touch this model (.vale.ini, Makefile, docs/.style/style-guide.md, docs/.style/README.md, docs-ci.yaml workflow comment) now agree.
Filed via Coder Agents on Nick's behalf.
Three follow-ups from review id 4320840969. DEREM-13 (P3): drop the redundant `grep -v '^docs/\.style/styles/'` filter from the prose step. Synced upstream packages are gitignored, so tj-actions/changed-files never lists them; the filter only ever rejects tracked Coder/**.md files, which is the opposite of what we want. The inline comment now documents why no second filter exists. DEREM-14 (P3): fix the README's --no-exit explanation. The DEREM-4 fix chain reached .vale.ini, the Makefile, and style-guide.md but missed docs/.style/README.md. The README now matches the rest: --no-exit suppresses the exit from the baseline error count produced by un-overridden Google error-level rules, not from warnings/suggestions (which never trigger non-zero exit regardless of --no-exit). DEREM-15 (Nit): same model fix in the docs-ci.yaml prose-step comment. Verification: make lint/actions, make fmt/markdown, make lint/markdown, make lint/prose all clean. Vale baseline unchanged (391 errors / 5201 warnings / ~7673 suggestions across 458 files).
R2 follow-ups addressedCommit
Process note responseThe PR description was already rewritten earlier this session to remove the stale Verification
Filed via Coder Agents on Nick's behalf. |
|
/coder-agents-review Filed via Coder Agents on Nick's behalf. |
There was a problem hiding this comment.
All 15 findings from R1 and R2 addressed and verified by Netero + panel. The fixes are thorough and consistent: the --no-exit model is now correct across all five locations, the CI step split properly isolates infra failures, the problem matcher surfaces annotations inline, and the PR description matches the implementation.
One new Nit (DEREM-16) on the README escape-hatch command, otherwise clean.
As Hisoka put it: "Three rounds. Fifteen fixes. I pulled every thread I could find across the diff... Every thread either held or was guarded by a layer the author already placed. The code earned the silence."
🤖 This review was automatically generated with Coder Agents.
|
|
||
| ```shell | ||
| make docs/.style/.vale-synced | ||
| ./build/vale-* docs/ # or pass specific files |
There was a problem hiding this comment.
Nit [DEREM-16] ./build/vale-* expands to multiple arguments when more than one Vale version exists in build/ (e.g., developer bumps version in mise.toml without make clean). The canonical entry point make lint/prose uses the exact versioned variable and is always correct. CI uses find -print -quit. Only this escape-hatch path has the ambiguity. (Mafuuu P3)
Low risk since three conditions must align (version bump + no clean + manual invocation), and make lint/prose is the documented primary path. Could note "ensure only one vale-* binary exists, or run make clean first" if desired.
🤖
There was a problem hiding this comment.
Fixed in d394b14. The escape-hatch section in docs/.style/README.md now notes the gotcha:
The
./build/vale-*glob expands to whatever Vale binaries exist underbuild/. If you bump the version inmise.tomlwithout runningmake clean, multiplevale-*binaries can coexist and the glob expands to multiple arguments, breaking the invocation. The canonicalmake lint/prosepath uses the exact versioned binary and is always correct; runmake cleanbefore using the direct-invocation form if you've ever changed the pinned version locally.
Mafuuu's three-condition analysis was right: the path is low-risk in practice, but the failure mode is silent enough (cryptic error from Vale on unexpected positional args) that a developer hitting it would lose minutes diagnosing it. One sentence is cheap insurance.
Filed via Coder Agents on Nick's behalf.
There was a problem hiding this comment.
Addressed in d394b14 (current head). Added an 8-line paragraph to docs/.style/README.md immediately after the direct-invocation snippet, naming the failure mode (version bump + no make clean + manual invocation), pointing readers back to make lint/prose as the always-correct canonical path, and explicitly recommending make clean before using the direct form if the pinned version has ever changed locally.
Landed the same day the Nit was raised; missed the followup acknowledgment then.
Generated by Coder Agents on behalf of @nickvigilante.
DEREM-16 (Nit) from review id 4321386259. `./build/vale-* docs/` is fine when only one vale-X.Y.Z binary exists in build/, but if a developer bumps the version in mise.toml without running `make clean`, multiple binaries coexist and the glob expands to multiple positional args, breaking the command. Low-risk path because three conditions have to align (version bump + no clean + manual invocation), but worth a sentence so a developer who hits it knows to `make clean`.
1 participant
Wires Vale into docs CI as a non-blocking prose-lint step. Builds on the
docs/.style/scaffold from #25466.Base branch is
vigilante/docs-180-...(PR 1), notmain. Will rebase ontomainonce PR 1 merges.What changes
.vale.iniat the repo root: Google base + Coder (custom, empty in v1) + curated write-good.alexrules are pulled in a la carte. Inline comments justify every enable/disable.mise.toml: pin Vale3.7.1via aqua.Makefile: install Vale atbuild/vale-$VERSION(mirrors thelint/typospattern),docs/.style/.vale-syncedsentinel that gatesvale sync, and alint/prosetarget that invokesvale --no-exit..github/workflows/docs-ci.yaml: a newPrepare Vale stylesstep and aprosestep that lints only the changed Markdown files underdocs/, with a problem matcher for inline PR annotations and a cache for both the synced styles and the binary..github/vale-problem-matcher.json: parsesvale --output=lineso alerts surface as PR annotations on the Files Changed tab..gitignoreand the workflow cachepath:: usedocs/.style/styles/*plus a!docs/.style/styles/Codernegation so adding a package does not require parallel edits..markdownlint-cli2.jsonc: ignore the synced styles somake lint/markdowndoes not lint upstream READMEs.docs/.style/README.mdandstyle-guide.md: document how to run Vale locally and what the active rule set is.Severity policy (v1)
Rule severity reflects two things together: the rule's false-positive rate against real Coder docs and the gravity of the rule. Low FPs plus high gravity argues for
error; lower gravity or more judgment calls argue forwarningorsuggestion. v1 lands most rules atwarningand the wordiness rules atsuggestion.A rule promotes to
erroronly when (a) its false-positive rate against real content is effectively zero and (b) the existing-content violation count for that rule is also zero. Vale exits non-zero only on error-level alerts regardless ofMinAlertLevel; the Makefile and CI invoke Vale with--no-exitso the baseline error count from un-overridden Google rules does not fail the build while real failures (missing binary, bad config) still propagate.CI integration
The Vale step ships in two parts:
Prepare Vale stylesrunsvale syncto materialize the configured packages underStylesPath. Nocontinue-on-error. A download or sync failure breaks the job.proserunsvale --no-exitagainst the changed Markdown set. Nocontinue-on-error. The--no-exitflag suppresses Vale's non-zero exit on alerts, so the 391 baseline error-level violations from un-overridden Google rules do not fail CI while the cleanup PRs land; real failures (binary missing, config invalid, file missing) still propagate as a red job.This shape is intentional: any failure between "download the binary" and "linter actually runs" surfaces as a red CI step instead of disappearing into a misleading yellow
continue-on-errorbadge.The
prosestep usesvale --output=lineplus.github/vale-problem-matcher.jsonso each alert renders as an inline annotation on the Files Changed tab.::remove-matcher owner=vale::after the command keeps the matcher from leaking into later steps.Verification
make pre-commitclean (full pre-commit, 23s).make lint/proseon the fulldocs/corpus: 391 errors, 5201 warnings, 7673 suggestions across 458 files in ~22s. Exit 0 because of--no-exit.e49b5ccedf:vale synccached, problem matcher emitted##[warning]path:line:col:rule:messageannotations, job conclusion success.actionlintclean on the modified workflow (validated locally and viamake lint/actions/actionlint).Implementation plan and decision log
Why this rule set
The Vale evaluation against the full docs corpus (measured 2026-05-18) produced ~43,940 raw violations across six candidate base styles. The selection here drops Microsoft and RedHat (overlap with Google, and RedHat's Spacing rule hammers technical IDs), and proselint (Annotations rule treats
> [!NOTE]admonitions as TODO markers).Within the kept styles:
EmDash(conflicts withmake lint/emdash),Latin(i.e./e.g. are fine for our audience),Spacing(4,500 errors oncodersdk.SomeTypepatterns in the auto-generated API reference). Softened:Parenstosuggestion,WordListtowarning.PassiveandE-Primeoff.TooWordyandThereIsare suggestions;Weaselis a warning.BasedOnStyles):Ablist,Condescending,LGBTQ,ProfanityLikely,Race,Suicideat warning. TheProfanityMaybe/ProfanityUnlikelyrules trip onexecute,kill,failed, andattack, which read as technical vocabulary in our context.BasedOnStylesbut the directory is empty in v1. Rules land through the per-rule tickets in the Docs style guide project.Why
vale syncinstead of vendoringThe three style packages weigh ~272 KB combined, so vendoring is cheap. But Vale's ecosystem treats
Packages =+vale syncas canonical, the upstream LICENSE files are not in the package tarballs (would need to be added manually), and the CI cache makes the sync nearly free after the first run. Sticking with the canonical pattern keeps the repo lean and the upgrade path obvious.Why
lint/proseis not inlint:orlint-light:Vale on the full docs corpus takes ~20s and downloads ~50 MB on cold caches. Forcing every pre-commit through that would be aggressive for a feature that ships as warnings.
make lint/typosfollows the same pattern (it is inlint-lightbut notlint; CI invokes it directly). v1 keeps Vale opt-in locally and CI-only by default; promote tolint:once the rule set stabilizes.Rejected alternatives for exit-code handling
--no-exitwas selected over two earlier candidates. Neither appears in the diff; this section is decision-log only.|| truein the Makefile: swallows the non-zero exit, but also swallows missing-binary, bad-config, and missing-files failures.continue-on-error: trueon the workflow step: same blast radius, plus shows up as a misleading yellow icon in the PR UI.vale --no-exitis Vale's own flag for suppressing the non-zero exit on alerts. Real failures (binary missing, config invalid, file missing) still propagate as a red job, which is the behavior the split CI step depends on.Pre-mortem
docs/reference/is dominated by auto-generated content (clidocgen, apidocgen, auditdocgen, metricsdocgen). The architectural decision is to fix the generators, not exclude paths in Vale. Google.Spacing is the only rule we silenced specifically to defer the generator fix; everything else surfaces as warnings.vale sync+ binary download adds ~10s to a cold CI run. The Actions cache keyed offhashFiles('.vale.ini', 'mise.toml', 'docs/.style/styles/Coder/**')makes subsequent runs near-instant; the Coder/** hash is defense-in-depth against actions/toolkit#713 so a future cache release that regresses the path-negation behavior cannot serve a stale Coder/ from cache.Linux_64-bit,Linux_arm64,macOS_64-bit,macOS_arm64. The Makefile handlesuname -mvaluesx86_64,arm64, andaarch64.make lint/proselints the full tree. This mirrorsmake lint/markdown(full tree) vs the changed-files CI step. Acceptable for v1..github/workflows/test-*reference the docs-ci workflow's diff parsing; my change todocs-ci.yamlis purely additive.Depends on #25466 (DOCS-180).
Closes DOCS-40.
Filed via Coder Agents on Nick's behalf.