Skip to content

Add Vale prose linting#50

Merged
plx merged 10 commits into
mainfrom
plx/add-vale-article-lint
Jul 8, 2026
Merged

Add Vale prose linting#50
plx merged 10 commits into
mainfrom
plx/add-vale-article-lint

Conversation

@plx

@plx plx commented Jul 1, 2026

Copy link
Copy Markdown
Owner

Adds project-local Vale configuration and a starter Terminology style for article Markdown/MDX, focused on Apple capitalization consistency.

Wires lint:prose through npm scripts, the justfile, local CI parity, and the reusable GitHub Actions validation workflow.

Updates validation docs and fixes an existing Objective-C capitalization issue in content.

Review follow-up:

  • Expanded Vale fixtures to cover the remaining Apple substitution rules: iPad, iPadOS, iPhone, watchOS, tvOS, visionOS, and App Store.
  • Made the Vale fixture test compare exact alert matches, so missing rules and duplicate overlapping alerts fail CI.
  • Scoped the standalone iPad substitution away from iPadOS phrases.
  • Lint image alt text with article prose and documented the non-exhaustive casing tradeoff.

Validation: npm ci, npm run lint:prose, just lint-prose, npm run test:ci, npm run lint:markdown, git diff --check.

Latest local validation: npm run test:ci.

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

PR Validation ❌

Some checks failed. Please review the errors above.

Checks Performed:

Please check the workflow logs for details on which checks failed.

This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged.

@claude

This comment has been minimized.

@plx plx marked this pull request as ready for review July 2, 2026 01:55
@plx

plx commented Jul 2, 2026

Copy link
Copy Markdown
Owner Author

Addressed in c934390.

  • Made the Apple terminology substitutions case-sensitive with explicit incorrect forms, so correctly cased terms no longer rely on ignorecase: true.
  • Preserved historical classic Mac OS / Mac OS X references and documented local Vale exceptions plus MDX JSX prop caveats in .vale/README.md.
  • Added npm run test:vale with pass/fail fixtures for the Apple terminology rules and wired it into local CI, build.yml, and the PR validation comment.
  • Tightened the SPELLCHECK.md Vale wording and documented the Vale install-time binary note.

Verified with npm run test:ci.

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

PR Validation ❌

Some checks failed. Please review the errors above.

Checks Performed:

Please check the workflow logs for details on which checks failed.

This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged.

@claude

claude Bot commented Jul 2, 2026

Copy link
Copy Markdown

Review: Add Vale prose linting

Nice addition overall — the Vale wiring is clean, consistent with the existing spellcheck/lint conventions (npm scripts → justfile → CI reusable workflow → docs), and .vale/README.md clearly documents the rationale for case-sensitive substitutions and the historical-name exceptions (classic Mac OS, Mac OS X are correctly left unflagged since they're not in the enumerated "incorrect forms" lists). A few notes:

Test coverage

  • scripts/check-vale-fixtures.js only exercises 7 of the 13 substitution rules in Apple.yml (ios, xcode, Swift UI, Objective C, Test Flight, MacOS, Apple Silicon). iPad, iPadOS, iPhone, watchOS, tvOS, visionOS, and App Store have no regression coverage — a future edit that accidentally breaks one of those regexes (e.g. a typo in the \b(...)\b alternation) would pass CI silently. Worth extending apple-terminology-invalid.md/-valid.md to cover all swap keys, since that's the whole point of test:vale.

Minor / non-blocking

  • The enumerated casing lists in Apple.yml (e.g. ipad[ -]?os|ipad[ -]?OS|iPad[ -]os|...) aren't exhaustive — some casing permutations (e.g. IpadOs, objectiveC) would slip through unflagged since ignorecase: false requires each variant to be listed explicitly. That's a reasonable, documented trade-off (avoids fighting case-insensitive matching against correctly-cased forms), just a known limitation worth keeping in mind — the maintenance burden grows with each new term added.
  • @vvago/vale has a postinstall script that downloads a platform binary during npm ci. That's inherent to how the package works and is already called out in .vale/README.md, but it's a new supply-chain surface (network fetch + binary execution at install time) worth being aware of if this repo ever tightens install-time network policy.
  • .vale.ini uses IgnoredScopes = code, tt, img — excluding img means alt text won't get terminology checking. Given the project's accessibility focus, alt text is one of the more likely places prose actually ships to users, so it may be worth reconsidering that exclusion (or confirming it was intentional to avoid noise from non-prose alt attributes).

Things that look right

  • CI wiring is correct: pr-validation.yml delegates to build.yml (not just a static checklist), and the new "Run Prose Lint (Vale)" / "Run Vale Fixture Tests" steps run before the build step, so failures are caught early without wasting build time.
  • npm run lint:prose is scoped to src/content while fixtures live under .vale/fixtures with their own .vale.ini section, so fixtures won't pollute the real content lint run — and the deliberately-misspelled fixture content won't trip cspell either, since its globs don't include .vale/.
  • The Objective-C fix in the-three-file-pattern.md is a legitimate correction and doubles as a demonstration that the new rule works.
  • @vvago/vale is correctly placed in devDependencies, and the lockfile is consistent.

Nothing here blocks merging — the test-coverage gap is the one item I'd actually act on before or shortly after merge.

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

PR Validation ✅

All checks passed! Ready for review.

Checks Performed:

✓ Linting
✓ Unit tests
✓ Spell check (source)
✓ Prose lint (Vale)
✓ Vale fixture tests
✓ Type checking & Build
✓ Spell check (HTML)
✓ Internal link validation
✓ Artifact upload

This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged.

@claude

claude Bot commented Jul 2, 2026

Copy link
Copy Markdown

Review: Add Vale prose linting

Nice addition — wiring Vale into npm run test:ci/CI with a small, focused Terminology style is a clean way to keep Apple product-name casing consistent, and the docs (CLAUDE.md, VALIDATION.md, SPELLCHECK.md, justfile) were all kept in sync with the new scripts. A couple of things worth tightening:

1. Fixture tests don't cover half the substitution rules

.vale/styles/Terminology/Apple.yml defines 14 swap entries, but .vale/fixtures/apple-terminology-invalid.md / apple-terminology-valid.md (and the expectedMatches list in scripts/check-vale-fixtures.js) only exercise 7 of them: ios, xcode, Swift UI, Objective C, Test Flight, MacOS, Apple Silicon.

Not exercised by npm run test:vale: iPad, iPadOS, iPhone, watchOS, tvOS, visionOS, App Store. A future edit that breaks one of those regexes (e.g. a bad escape in the watch[ -]?os alternation) would pass CI silently. Worth adding a phrase per remaining term to both fixture files and the expected-matches array.

2. Overlapping rules can double-flag the same text

The ipad rule (line 7) and the ipad[ -]?os / iPadOS rule (line 8) are both \b...\b substitution matches, and both can match inside the same phrase: "ipad os" matches the standalone ipad rule and the ipad os rule, since there's a word boundary right after "ipad" and before the space. That produces two overlapping Vale alerts (suggesting iPad and iPadOS for overlapping spans) for what's really one mistake. None of the other compound terms (watch os, tv os, vision os) have a standalone sibling rule, so this only affects iPad/iPadOS. Worth scoping the bare ipad rule so it doesn't also fire inside "ipad os" — this gap isn't covered by the fixtures either (see point 1), so it'd be easy to miss.

Minor / non-blocking

  • The substitution style enumerates specific wrong casings per term (ignorecase: false) rather than case-insensitive matching plus an allowlist. That's a deliberate, documented tradeoff (.vale/README.md), but it means uncommon casings that weren't explicitly enumerated (e.g. Macos, Ipados, Iphone) will silently pass. Worth a one-line callout in the README so contributors don't assume the rule is exhaustive.
  • Good security hygiene in scripts/run-vale.js / scripts/check-vale-fixtures.js: spawnSync is called with argument arrays rather than a shell string, so there's no shell-injection surface even though file paths flow into the command.
  • scripts/vale.js correctly branches on process.platform === "win32" for the binary name — nice cross-platform touch even though CI only runs Linux today.
  • The Objective-C capitalization fix in src/content/briefs/objective-c/the-three-file-pattern.md is a good real-world validation that the new rule catches a genuine pre-existing issue.

No security or performance concerns — this only touches devDependencies/CI tooling and doesn't affect the built site's runtime.

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

PR Validation ✅

All checks passed! Ready for review.

Checks Performed:

✓ Linting
✓ Unit tests
✓ Spell check (source)
✓ Prose lint (Vale)
✓ Vale fixture tests
✓ Type checking & Build
✓ Spell check (HTML)
✓ Internal link validation
✓ Artifact upload

This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged.

@claude

claude Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review: Add Vale prose linting

Nicely scoped addition — the CI wiring, docs, justfile, and fixture-based regression tests for the Terminology style all fit together cleanly, and the follow-up commits already addressed the overlap/duplicate-alert and iPad/iPadOS-scoping issues from earlier review passes. A few remaining notes:

Correctness (minor)

.vale/styles/Terminology/Apple.yml — the iPad/iPadOS lookaheads are inconsistent in what case of "os" they exclude:

'\b(ipad(?![ -]?(?:os|OS)\b)|IPad(?![ -]?OS\b)|Ipad(?![ -]?OS\b)|IPAD)\b': iPad

The plain ipad branch excludes both os and OS in its lookahead, but the IPad and Ipad branches only exclude uppercase OS. So a mixed-case typo like "IPad os" or "Ipad os" (capitalized IPad/Ipad + lowercase "os", separated by a space/dash) isn't excluded from the plain iPad swap, and also doesn't match any of the iPadOS swap alternatives (they require matching case pairs). Net effect: that phrase gets "corrected" to iPad os instead of being flagged as needing iPadOS. It's a narrow edge case (an unusual capitalization combo), and the README already documents the substitution list as a non-exhaustive guardrail rather than an exhaustive casing detector, so this may be acceptable — but it looks like an oversight rather than an intentional gap, since the plain ipad branch handles both cases correctly. Worth aligning the three lookaheads for consistency if easy to do.

Security (informational, not blocking)

@vvago/vale runs a postinstall script that downloads a Vale binary at npm install/npm ci time (hasInstallScript: true in the lockfile). This is a fairly common pattern (similar to Playwright's browser download), and the npm package itself is integrity-pinned via package-lock.json, but it's worth being aware that the downloaded binary isn't covered by that same integrity hash. Not a blocker, just flagging for awareness given it runs in CI.

Test coverage

  • The fixture-based tests (scripts/check-vale-fixtures.js) give solid coverage of the substitution rules themselves — comparing exact alert sets (vs. just "some alerts fired") is a good call to catch silently-missing or duplicated rules.
  • The thin wrapper scripts (scripts/vale.js, scripts/run-vale.js) have no direct tests (e.g., the "Vale executable not found" error path in requireValeExecutable), but given how small and low-risk they are, this seems like an acceptable tradeoff rather than a real gap.

Everything else

  • CI wiring (build.yml, pr-validation.yml) and docs (VALIDATION.md, CI_CD_DOCUMENTATION.md, WORKFLOW_ARCHITECTURE.md, CLAUDE.md, SPELLCHECK.md) are all kept in sync with the new scripts — good attention to detail there.
  • .vale.ini's IgnoredScopes = code, tt correctly keeps code spans out of prose linting.
  • The Objective-C capitalization fix in the brief content is a nice, correctly-scoped drive-by fix.

Overall this looks ready to merge; the iPad/iPadOS lookahead inconsistency is the only thing I'd consider actually fixing, and even that's low severity.

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

PR Validation ✅

All checks passed! Ready for review.

Checks Performed:

✓ Linting
✓ Unit tests
✓ Spell check (source)
✓ Prose lint (Vale)
✓ Vale fixture tests
✓ Type checking & Build
✓ Spell check (HTML)
✓ Internal link validation
✓ Artifact upload

This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged.

@claude

claude Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review: Add Vale prose linting

Nicely scoped addition — the Vale wiring, CI parity docs, justfile targets, and fixture-based regression test are all consistent with each other, and check-vale-fixtures.js asserting exact alert sets (not just "count > 0") is a good call given the review history on this PR.

Correctness: IPAD case is inconsistent with the other iPad casing variants

.vale/styles/Terminology/Apple.yml:7-8:

'\b(ipad(?![ -]?(?:os|OS)\b)|IPad(?![ -]?(?:os|OS)\b)|Ipad(?![ -]?(?:os|OS)\b)|IPAD)\b': iPad
'\b(ipad[ -]?os|ipad[ -]?OS|iPad[ -]os|iPad[ -]OS|IPad[ -]?os|IPad[ -]?OS|Ipad[ -]?os|Ipad[ -]?OS|IPADOS)\b': iPadOS

The ipad/IPad/Ipad alternatives all carry a negative lookahead so they don't fire when followed by os/OS (that's what "Scoped the standalone iPad substitution away from iPadOS phrases" fixed). But the fourth alternative, IPAD, has no such lookahead. So for all-caps input like IPAD OS or IPAD-OS:

  • The standalone iPad rule does match IPAD (there's a word boundary right after it, before the space/hyphen), incorrectly suggesting "iPad" and completely dropping the OS-related correction.
  • The iPadOS rule can't catch it either — its alternatives only include the solid IPADOS form, not IPAD[ -]?OS.

Net effect: IPAD OS / IPAD-OS get a misleading "use iPad" suggestion instead of "use iPadOS". Given the PR's whole point is nailing these substitutions precisely (and given the two follow-up commits already fixing closely related overlap bugs), this seems worth closing too — add the same (?![ -]?(?:os|OS)\b) lookahead to the IPAD alternative, and add an IPAD[ -]?OS alternative to the iPadOS rule. Worth a fixture case as well, since apple-terminology-invalid.md doesn't currently exercise the all-caps form.

Minor / non-blocking

  • Supply chain: @vvago/vale has hasInstallScript: true and downloads the Vale binary from the network during npm ci. Reasonable given it's dev tooling only (not shipped to dist/), but worth being aware of for CI reliability if GitHub's network to the release asset ever has issues — requireValeExecutable's error message ("Run npm ci before invoking Vale") is a nice touch for the local-dev failure mode at least.
  • The remaining casing gaps (e.g., mac OS, App STORE, objective-C) are already called out in .vale/README.md as an accepted non-exhaustive tradeoff, so no note needed there beyond the IPAD case above, which is a genuine inconsistency rather than a missing permutation.
  • scripts/check-vale-fixtures.js logs stderr from the Vale process but doesn't fail the run on non-empty stderr — probably fine since the exact-match assertion is the real gate, just flagging in case that was meant to be a hard failure too.

Everything else looks solid

  • CI wiring (build.ymllint:prose + test:vale before build) and the reusable-workflow reference from pr-validation.yml are correctly threaded through.
  • Docs (CLAUDE.md, VALIDATION.md, SPELLCHECK.md) updated consistently with the new scripts/just targets.
  • The Objective-C capitalization fix in src/content/briefs/objective-c/the-three-file-pattern.md is a nice concrete proof the linter catches real content issues.
  • No security concerns beyond the supply-chain note above; nothing here touches user-facing runtime code.

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

PR Validation ✅

All checks passed! Ready for review.

Checks Performed:

✓ Linting
✓ Unit tests
✓ Spell check (source)
✓ Prose lint (Vale)
✓ Vale fixture tests
✓ Type checking & Build
✓ Spell check (HTML)
✓ Internal link validation
✓ RSS feed validation
✓ Artifact upload

This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged.

@claude

claude Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review: Add Vale prose linting

Nice addition overall the fixture driven test approach in scripts/check-vale-fixtures.js, asserting exact match sets rather than just pass or fail, is a good pattern; it will catch missing rules and duplicate or overlapping alerts, which the commit history shows it already did during review iteration. Docs (CI_CD_DOCUMENTATION.md, WORKFLOW_ARCHITECTURE.md, VALIDATION.md, .vale/README.md, CLAUDE.md, justfile) are updated consistently, and CI wiring (build.yml, pr-validation.yml) looks correct new steps run after npm ci (so the Vale binary from @vvago/vale postinstall is present) and before the build.

Code quality / maintainability

  • .vale/styles/Terminology/Apple.yml hand-enumerates every case variant per term instead of using ignorecase: true with a targeted exclusion. It works per the fixtures, but two different techniques are used to keep the correct spelling from self-matching: a negative lookahead for ipad/ipados, versus a mandatory-separator-when-the-OS-suffix-is-capitalized trick for watchOS/tvOS/visionOS/TestFlight. Consolidating on one technique would ease future maintenance, since adding a new term correctly currently requires remembering which trick applies.
  • Because of the hand enumeration, a few mixed-case forms slip through silently: "mac OS" (space plus capital OS), "TvOS", "WatchOs", etc. are not in any swap list, so they would pass unflagged. The README already documents this as a deliberate regression-tested guardrail rather than an exhaustive casing detector, so this is more a heads-up for future maintenance than a requested change.

Potential bugs

  • One case is more than a coverage gap: for the ipad rule, the negative lookahead only excludes a lowercase "os" or all-caps "OS" suffix. A mixed-case suffix like "ipad-Os" matches neither the ipad exclusion nor the iPadOS swap list, so it would get partially flagged with "Use iPad instead of ipad" a misleading fix for what is actually an iPadOS typo. A narrow edge case, but worth knowing about since it produces a wrong suggestion rather than just a missed one.
  • Worth keeping in mind: the ios/IOS/Ios swap is case-sensitive but not scope-aware it would also flag Cisco IOS or similar non-Apple "IOS" usage if that ever shows up in content. Not an issue today (grepped current content, no false positives), but worth remembering if networking or infra topics get covered later.

Performance
No concerns Vale only runs against src/content/**/*.{md,mdx} and the fixtures, and build.yml uses actions/setup-node npm caching, so the Vale binary download is not repeated needlessly across cached runs.

Security

  • @vvago/vale has a postinstall script that downloads a platform binary. package-lock.json pins the exact resolved version (3.15.1) even though package.json uses a caret range, so npm ci stays reproducible thats good. Just flagging for awareness that this introduces an external binary download into the install step, same category as other tooling already in the repo (e.g. Playwright), so likely an accepted tradeoff already.
  • No other security concerns no user input is processed; this is dev/CI tooling only.

Test coverage

  • The fixture tests are solid: valid and invalid .md fixtures plus exact-match assertions in check-vale-fixtures.js cover all the substitution rules. Good call tightening it to compare exact Match sets rather than just pass/fail, per the "Tighten Vale terminology fixtures" commit.
  • Minor gap: the fixtures do not include a case for the ipad-Os-style mixed-suffix scenario above, so there is no regression test either way if that gets addressed.

Nothing here blocks merging the core wiring is correct and well-tested; the notes above are mostly about long-term maintainability of the regex rules as more Apple terms get added.

@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

PR Validation ✅

All checks passed! Ready for review.

Checks Performed:

✓ Linting
✓ Unit tests
✓ Spell check (source)
✓ Prose lint (Vale)
✓ Vale fixture tests
✓ Type checking & Build
✓ Spell check (HTML)
✓ Internal link validation
✓ RSS feed validation
✓ Artifact upload

This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged.

@claude

claude Bot commented Jul 4, 2026

Copy link
Copy Markdown

Review: Add Vale prose linting

Went through the diff (config, scripts, styles, fixtures, CI/docs wiring) and traced the Apple.yml substitution regexes by hand against both fixtures. Overall this is solid, well-tested work — a few notes below, nothing blocking.

What's good

  • Exact-match fixture testing in scripts/check-vale-fixtures.js (comparing the full sorted set of alert.Match values, not just "at least one alert") is the right call — it means a rule silently stopping matching, or a rule double-firing, both fail CI instead of passing accidentally. The PR description notes this was added in response to review feedback; good catch.
  • The negative lookahead separating the standalone iPad rule from the iPadOS rule ((?:ipad|IPad|Ipad|IPAD)(?![ -]?[oO][sS]\b)) correctly avoids double-flagging phrases like ipad-Os or IPAD-OS with both rules simultaneously. I traced all 18 expected matches in apple-terminology-invalid.md against the regex by hand and they check out, including the all-caps IPADOS/IPAD-OS cases that needed a dedicated follow-up commit.
  • scripts/run-vale.js / check-vale-fixtures.js pass args to spawnSync as arrays rather than interpolating into a shell string, so there's no command-injection surface despite shelling out to an external binary.
  • Doc/CI wiring is thorough and consistent: justfile, package.json (validate:all, test:ci, test:ci:verbose), VALIDATION.md, the CI docs, and CLAUDE.md were all updated together, and the GitHub Actions steps mirror the new npm scripts.
  • I grepped src/content for the incorrect-case forms the new rules target and found none outstanding, so merging this shouldn't retroactively break the build on existing content.

Minor / non-blocking notes

  • The substitution lists rely on hand-enumerated case variants (e.g. ios|IOS|Ios, Objective C|Objective-c|ObjectiveC|OBJECTIVE[ -]?C) rather than ignorecase: true with an explicit allow-list for the correct form. That's a reasonable tradeoff given Vale's substitution style doesn't support per-key case-insensitivity with exceptions, and it's already called out well in .vale/README.md ("not an exhaustive casing detector") — but it does mean some miscased forms slip through, e.g. objective-C (lowercase word + dash + capital C) isn't caught by any of the four Objective-C alternatives. Given Objective-C capitalization is literally the motivating bug fixed in this PR (the-three-file-pattern.md), it might be worth a one-line comment directly in Apple.yml pointing at the README rationale, since a future contributor editing the YAML in isolation won't necessarily see it.
  • @vvago/vale runs a postinstall script that downloads a platform-specific Vale binary. Not a new category of risk for this repo's tooling, but worth knowing it's there if npm ci ever needs to run somewhere without network egress.
  • Nice touch documenting the <!-- vale Terminology.Apple = NO/YES --> escape hatch for legitimate historical terms (classic Mac OS, Mac OS X) — that'll save someone a confused debugging session later.

No functional bugs found in the regex logic, script error handling, or CI wiring. Test coverage (valid + invalid fixtures, exact-match assertions) is appropriately rigorous for a linting rule that's proven fiddly to get right across the commit history.

@plx plx merged commit f7f69a5 into main Jul 8, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant