chore: add gated docs linting and repoint the e2e Gitea image - #262
Conversation
Adds vale (with a HouseStyle ruleset for em dashes, correctives, filler, headings, product name, and spelling) and markdownlint-cli2, installs both in the devcontainer, and asserts their versions in CI alongside the other linters. docs/style-guide.md records the conventions the ruleset enforces, including the three prescriptive decisions the repo needed a call on: no em dashes in prose, British spelling with American kept where a word names a thing in the code, and a list of words to cut. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThis change adds pinned Markdown and prose linting tools, Vale and markdownlint configurations, scoped Taskfile execution, changed-file discovery, CI validation, and documentation guidance describing the resulting standards and workflow. ChangesDocumentation linting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant Taskfile
participant DocsFiles
participant Markdownlint
participant Vale
CI->>Taskfile: run task lint
Taskfile->>DocsFiles: select changed Markdown files
DocsFiles-->>Taskfile: return sorted file list
Taskfile->>Markdownlint: lint Markdown structure
Taskfile->>Vale: lint prose rules
Markdownlint-->>CI: return structural result
Vale-->>CI: return prose result
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (1)
internal/controller/gittarget_source_cluster_test.go (1)
504-504: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository’s test naming convention.
Rename
TestAuditRouteForto a scenario-qualified name such asTestAuditRouteFor_ProviderRouteResolution. As per coding guidelines, Go tests must use theTestFunctionName_Scenario(t *testing.T)naming convention.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/gittarget_source_cluster_test.go` at line 504, Rename the Go test function TestAuditRouteFor to a scenario-qualified name following the TestFunctionName_Scenario convention, such as TestAuditRouteFor_ProviderRouteResolution. Update only the test declaration and any references needed to keep the test runnable.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 112-118: Update both CI version-validation jobs around the
markdownlint-cli2 and vale checks to compare their installed versions against
MARKDOWNLINT_CLI2_VERSION and VALE_VERSION from the devcontainer configuration,
failing when either differs. Keep the existing version commands, and apply the
same explicit pin assertions in both locations.
In @.vale/styles/HouseStyle/Spelling.yml:
- Around line 1-6: Update the spelling mappings in the HouseStyle spelling rule
to permit British English forms used in prose, including “behaviour” and
“organisation,” while preserving American spellings for code entities. Align the
rule with the documented British-English policy rather than enforcing American
replacements.
- Around line 19-20: Update the spelling rules in Spelling.yml to match
capitalized variants such as Behaviour and Organisation, either by adding their
capitalized entries or by enabling case-insensitive matching with capitalization
support. Preserve the existing error-level spelling validation.
In `@cmd/main.go`:
- Around line 265-266: Update the shared audit-stream wording to use “audit
route” terminology: in cmd/main.go lines 265-266, revise the setupLog.Info
message; in charts/gitops-reverser/values.schema.json lines 278-280, describe
the value as an audit route and explain that ClusterProviders join it through
spec.attribution.auditRoute; in docs/architecture.md lines 610-614, replace
provider-name and provider-partition wording with audit-route terminology.
- Around line 506-513: Trim surrounding whitespace from the audit-route
annotation key before storing it in cfg.auditRouteAnnotationKey via the flag
setup, so validation and handler annotation lookup use the same normalized
value. Preserve the existing empty-value behavior that disables the bare
endpoint.
In `@CONTRIBUTING.md`:
- Around line 44-46: Align the docs-only validation instructions with the actual
checks by either updating task lint-docs to run markdownlint-cli2, Vale, and
reference checking, or explicitly listing all required commands. Update
CONTRIBUTING.md lines 44-46 and docs/style-guide.md line 291 consistently so
both documents describe the same command contract.
In `@docs/bug-report.md`:
- Line 1: Update the new prose in the bug report, including its title, to remove
all em dashes. Replace each em dash with appropriate punctuation such as a
colon, comma, or separate sentence while preserving the original meaning.
- Around line 92-116: The root-cause analysis in the “Root cause” section must
be clearly marked as describing the pre-fix implementation. Pin its code
references to commit b9c2e495, or explicitly label the analysis and referenced
behavior as a pre-fix snapshot, so it does not contradict target_watch
attribution through the provider’s audit route.
In `@docs/design/attribution-fact-identity.md`:
- Line 565: Update the sentence containing “which is exactly what an operator
who renamed their route has not got” to remove the flagged word, using
“precisely” or an equivalent rephrasing while preserving the original meaning.
- Around line 32-34: Update the ingestion description in the attribution fact
identity documentation to state that ingestion resolves and stores facts by the
incoming route without performing a Kubernetes lookup. Remove the contradictory
claim that ingestion does not change, while preserving the existing key
structure and terminology.
- Line 127: Update the “Ingestion stays low level” wording in the
attribution-fact-identity documentation to use the hyphenated form “low-level,”
leaving the surrounding sentence unchanged.
In `@docs/design/docs-linting.md`:
- Around line 172-176: Clarify the two heading-alert figures in
docs/design/docs-linting.md by adding the applicable lint scope or run context
for each count, or align them if they refer to the same run. Update the baseline
table and the “Heading case needs a vocabulary” section without changing the
underlying linting recommendations.
In `@docs/facts/audit-webhook-api-server-connectivity.md`:
- Line 7: The audit webhook documentation incorrectly describes the URL segment
as a cluster-provider name. Update the route description and related
example/instructions to use `/audit-webhook/<audit-route>`, noting that the
route may be shared across providers or supplied by event attribution
annotations; preserve the existing bare-route behavior and configuration
reference.
In `@docs/INDEX.md`:
- Around line 68-73: Correct the “Thirteen other open items” heading in
docs/INDEX.md to match the table’s twelve entries; update the count to twelve
without changing the listed items.
In `@internal/controller/gittarget_controller.go`:
- Line 222: The audit route declaration in
internal/controller/gittarget_controller.go at lines 222-222 must not fall back
to the provider name when route resolution fails; propagate the error or reuse
the validated ClusterProvider and skip declaration until the route is known.
Update internal/controller/gittarget_source_cluster_test.go at lines 545-548 to
expect the safe error/no-declaration behavior instead of unreadable-provider
fallback.
In `@internal/webhook/audit_handler.go`:
- Around line 328-353: Update the rejection and validation messages used by
resolveEventRoute, rejectUnroutableEvent, and validateAuditWebhookPath to use
audit-route terminology instead of ClusterProvider terminology. Describe
rejection as caused by a missing or empty routing annotation, and direct
operators to post to /audit-webhook/<audit-route>; preserve the existing
behavior that undeclared annotation values are accepted.
---
Nitpick comments:
In `@internal/controller/gittarget_source_cluster_test.go`:
- Line 504: Rename the Go test function TestAuditRouteFor to a
scenario-qualified name following the TestFunctionName_Scenario convention, such
as TestAuditRouteFor_ProviderRouteResolution. Update only the test declaration
and any references needed to keep the test runnable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 73ad41fb-8bc9-40cc-a6de-7507e683b67d
📒 Files selected for processing (58)
.devcontainer/Dockerfile.github/workflows/ci.yml.markdownlint-cli2.jsonc.vale.ini.vale/styles/HouseStyle/Correctives.yml.vale/styles/HouseStyle/EmDash.yml.vale/styles/HouseStyle/Filler.yml.vale/styles/HouseStyle/Headings.yml.vale/styles/HouseStyle/ProductName.yml.vale/styles/HouseStyle/Spelling.yml.vale/styles/HouseStyle/WordsToCut.ymlCONTRIBUTING.mdapi/v1alpha3/audit_route_test.goapi/v1alpha3/clusterprovider_types.goapi/v1alpha3/gittarget_types.goapi/v1alpha3/zz_generated.deepcopy.gocharts/gitops-reverser/README.mdcharts/gitops-reverser/templates/deployment.yamlcharts/gitops-reverser/values.schema.jsoncharts/gitops-reverser/values.yamlcmd/main.goconfig/crd/bases/configbutler.ai_clusterproviders.yamldocs/INDEX.mddocs/README.mddocs/architecture.mddocs/attribution-setup-guide.mddocs/bug-report.mddocs/config-flag-conventions.mddocs/configuration.mddocs/design/attribution-fact-identity.mddocs/design/docs-linting.mddocs/design/multi-source-audit-ingress-hardening.mddocs/facts/audit-webhook-api-server-connectivity.mddocs/finished/clusterprovider-fact-purge.mddocs/finished/multi-cluster-author-attribution.mddocs/style-guide.mdinternal/audit/outcome/outcome.gointernal/audit/outcome/outcome_test.gointernal/controller/clusterprovider_controller.gointernal/controller/clusterprovider_controller_unit_test.gointernal/controller/gittarget_controller.gointernal/controller/gittarget_source_cluster.gointernal/controller/gittarget_source_cluster_test.gointernal/queue/attribution_index.gointernal/queue/attribution_index_test.gointernal/queue/key_prefix_test.gointernal/watch/audit_route_test.gointernal/watch/author_resolver.gointernal/watch/author_resolver_test.gointernal/watch/cluster_context.gointernal/watch/manager.gointernal/watch/materialization.gointernal/watch/target_watch.gointernal/webhook/audit_handler.gointernal/webhook/audit_handler_test.gointernal/webhook/audit_metrics_test.gotest/e2e/audit_route_attribution_e2e_test.gotest/e2e/cluster/audit/webhook-config.yaml
💤 Files with no reviewable changes (2)
- docs/finished/clusterprovider-fact-purge.md
- internal/audit/outcome/outcome_test.go
f96a6a9 to
80c1c20
Compare
`task lint-docs` becomes the aggregate of three non-overlapping checks: lint-doc-links (hack/doccheck, unchanged), lint-markdown, and lint-prose. The name is kept because AGENTS.md, the style guide, and the CI cache assertion all cite it, and it is now a superset of what it used to do, so every existing instruction to run it stays true. `task lint-fix` gains the markdown half. hack/docs-files.sh is the single place that builds the markdown file list, so a local run and CI select identically. Exclusions deliberately do not move there: which files exist is a git question, which of them to skip stays in each tool's own config, so there is one list of each rather than two that can disagree. Both new tasks gate the markdown a branch touches rather than the whole tree. Measured with the committed config, 102 of 174 linted files fail markdownlint and 148 of 174 fail Vale, so a whole-tree gate is a wall and a 148-file baseline would be a repo-wide exclusion in disguise. A file is cleaned when someone opens it. The cost is that opening a file adopts its backlog, which CONTRIBUTING.md now says out loud. Three Vale rules changed, each measured against the real corpus. WordsToCut is split. It flagged `actually` in the exact sentence its own comment blessed as legitimate, and a rule that fires on its documented exception teaches people to skim its output. The four words that need the deletion test moved to Filler-Judgment.yml at suggestion level, which can never gate; what is left is mechanical and is the list that can carry an error level once the gate widens. EmDash keeps the default scope, and now records why. `scope: raw` raises per-occurrence recall from 37% to 99.9%, which is tempting, but it forfeits the fenced-specimen exemption the style guide requires (92 of 3,126 dashes across 32 files), disables both in-file suppression mechanisms, and buys no gate correctness: over the 148 files carrying a prose dash, the default scope misses none entirely and falsely flags none, while raw adds a false positive on style-guide.md itself. The rule decides whether a file is clean; `grep` gives the full list while fixing one. ProductName learned two more kube-apiserver spellings (the list had two and caught one of the three stragglers in the tree), and Headings gained the Go identifiers and language names that were false positives. CI: the lint job checks out with fetch-depth 0 and passes GITHUB_BASE_REF, because a shallow checkout leaves no merge base and the file list would fall back to the working tree, which is empty in CI. The gate would have passed everything, silently, so hack/docs-files.sh now fails rather than warns when the base is unreachable and CI is set. The cache assertion exempts the four un-fingerprinted docs tasks by name; adding a fifth without `sources:` and without listing it there fails the step on purpose. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The root README now passes both linters. Structure: the badge block keeps its place at the top of the file and MD041 is disabled for that one line rather than repo-wide, because the rule catches six other files and this is the only one that legitimately opens with something else. The Platforms badge linked to `#`, which went nowhere, so it is a plain image now. The five quick-start steps were bold text used as headings and are real `###` headings. Prose: all 23 em dashes are gone, matched to the job rather than swapped for commas, and three of the replacements are the style guide's own worked examples. `recognising` became `recognizing`, and the `X, not Y` correctives came down to the budget of three. One inconsistency the tooling surfaced: the audit-webhook link said `kube-api server` while the attribution section said `kube-apiserver`, and ProductName carried neither spelling. Both are fixed, here and in the rule. docs/README.md gets the same em dash pass so the gate is green on the files this branch touches. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
These two are the reference models the style guide names for depth and tone, and they predate it, so they carried the habits it lists as things to fix. Both now pass markdownlint and Vale with zero errors. `markdownlint-cli2 --fix` did 139 of the 141 structural findings, all of them `*` bullets becoming `-` in architecture.md. configuration.md needed none. The two it could not fix were worth having. One was a long line. The other was a dead same-document anchor: architecture.md linked to `#recovery-replay-plus-mark-and-sweep` while the heading reads "Recovery: resume, replay, or list plus mark-and-sweep". hack/doccheck passes that file clean, because it resolves cross-document references and same-document fragments are markdownlint's MD051. The three checks are not redundant. By hand: 134 em dashes (Vale reported 21 of them, which is the documented per-occurrence under-reporting at six times scale), four British spellings, and 28 title-case headings. Changing a heading's capitalization does not move its GitHub anchor, so that pass changes no link, and both checkers confirm it. Four headings were left alone and added to the Headings exceptions instead: BranchWorker, RuleStore, TypeRegistry, and WatchedTypeTable are Go identifiers, not title case. No wording was changed to satisfy a linter beyond re-punctuating; the two always-wrong words that came out (`simply`, `powerful`) were replaced with what they were standing in for. The remaining alerts are three warnings and nine suggestions, all judgment calls left for a human. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PR #263 edited docs/architecture.md and docs/configuration.md, which this branch had rewritten against the style guide, so both files conflicted. architecture.md auto-merged. configuration.md had one hunk where #263 renamed `/audit-webhook/<name>` to `/audit-webhook/<audit-route>` on the same line this branch had re-punctuated; the resolution keeps both. Verified after resolving: every line that differs from main in those two files is a bullet, a heading's capitalization, or a punctuation mark, and #263's `audit-route` and `auditRoute` terms are all present. Both files still pass markdownlint and Vale with zero errors. Merged rather than rebased on purpose: PR #262 carries 16 inline review comments that a force-push would orphan, and this repository squash-merges, so the merge commit does not survive into main anyway. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/docs-linting.md`:
- Around line 303-308: Update the paragraph describing hack/docs-files.sh and
shallow checkout behavior to state that CI exits with status 1 when the base ref
is unreachable, while only local runs warn and fall back. Remove the inaccurate
claim that CI continues with an empty file list, preserving the existing
explanation of full history and GITHUB_BASE_REF.
In `@hack/docs-files.sh`:
- Around line 69-72: Update the Markdown file collection commands in the
documentation lint script to include staged changes by adding the cached diff
output alongside the existing working-tree and untracked-file listings. Preserve
the current deleted-file filtering and Markdown path restriction.
In `@README.md`:
- Around line 42-44: Update the ordinary prose in the README’s “What it can't”
section to use the British spelling “behaviour” instead of “behavior,” while
leaving code entities and technical terminology unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a1e7adae-95c2-4f13-88df-cd964aa20696
📒 Files selected for processing (17)
.github/workflows/ci.yml.markdownlint-cli2.jsonc.vale.ini.vale/styles/HouseStyle/Correctives.yml.vale/styles/HouseStyle/EmDash.yml.vale/styles/HouseStyle/Filler-Judgment.yml.vale/styles/HouseStyle/Headings.yml.vale/styles/HouseStyle/ProductName.yml.vale/styles/HouseStyle/WordsToCut.ymlCONTRIBUTING.mdREADME.mdTaskfile-build.ymldocs/README.mddocs/architecture.mddocs/configuration.mddocs/design/docs-linting.mdhack/docs-files.sh
🚧 Files skipped from review as they are similar to previous changes (7)
- .vale/styles/HouseStyle/Headings.yml
- .vale/styles/HouseStyle/EmDash.yml
- .markdownlint-cli2.jsonc
- .vale.ini
- .vale/styles/HouseStyle/Correctives.yml
- .vale/styles/HouseStyle/ProductName.yml
- docs/architecture.md
The markdown and prose gates were derived from `git diff <merge-base>`, so they covered whatever a branch happened to touch. Replaced with .docs-lint-scope, a committed list, starting at README.md, docs/architecture.md, and docs/configuration.md. Everything else is reported by `DOCS_SCOPE=all` and gates nothing until its path is added. The diff-derived version had one failure mode that this removes rather than patches. A merge base needs history, CI checks out shallow, and an unreachable base produced an EMPTY file list, which is a green gate rather than a red one. Keeping it meant fetch-depth 0, threading GITHUB_BASE_REF into the lint container, and a special case in the script for "unreachable base but we are in CI". All three are gone; the lint job checks out normally again. A committed list is also identical on every machine, needs no history, and grows by review. It drops the diff model's worst property for contributors: editing one line of a long unmigrated document no longer adopts that document's entire backlog. What it gives up is files nobody listed, which can be added or edited with em dashes and nothing fails. That is the accepted cost of starting narrow, and CONTRIBUTING.md now asks people to run both tools on what they touch anyway. The guard that survived is the reason the old code had one: hack/docs-files.sh fails when the scope file is missing, lists nothing, or names a path git does not track, because each of those silently shrinks the gate to nothing. lint-doc-links is unchanged and still covers every tracked file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three files still said the docs linters were installed but wired into nothing, which stopped being true when they were folded into `task lint`: `.vale.ini`, `.markdownlint-cli2.jsonc`, and the `docs-linting.md` row in `docs/INDEX.md`. The design doc also described the scope as "the files a branch touches", the diff-based model it explicitly rejects further down in favor of the committed `.docs-lint-scope` list. AGENTS.md is the source of truth for validation and never learned about the new gate, so it still sent a docs-only change off with "a quick sanity check" and listed `task lint` as Go, workflows and Dockerfiles only. It now names `task lint-docs` and what gates which files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`ignorecase: false` on the Spelling rule meant only the lowercase swap keys ever matched, so any British form in sentence-initial position went through an error-level gate untouched. Measured on a specimen: `behaviour` was flagged while `Behaviour`, `Organisation`, `Labelled`, and `Modelled` all passed. `ignorecase: true` plus `capitalize: true` closes it and carries the source token's case into the suggestion, so the message reads "use 'Behavior', not 'Behaviour'". Over every tracked doc it finds 152 spellings where the case-sensitive rule found 143; the three gated files stay at zero. Also reconciles two counts the design docs contradicted themselves on: the open-items heading in docs/INDEX.md said thirteen over a twelve-row table, and docs-linting.md quoted the heading-case rule as both 378 and 503. Reported by CodeRabbit on #262. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds markdown linting to the repo, writes down the conventions it enforces, and gates it on a
short list of files rather than the whole tree.
What runs
task lint-docsis now three checks, andtask lintincludes it:lint-doc-linkshack/docchecklint-markdownlint-proseHouseStyleBoth new tools are installed in the
cistage of the devcontainer, so CI and local runs are thesame check. Node moved from
devtociwith them, since markdownlint-cli2 is an npm package.Why the gate is narrow
Structure and prose gate only the files
.docs-lint-scopelists, todayREADME.md,docs/architecture.md, anddocs/configuration.md. Measured with the committedconfig, 102 of 174 files fail markdownlint and 148 of 174 fail Vale, almost all of the latter on em
dashes. Fixing the tree would be one enormous commit colliding with everything in flight, so the
list starts at three files and grows as documents are cleaned. Those three are brought into
compliance here;
task lint-markdown DOCS_SCOPE=allshows the rest of the backlog.The scope is a committed list, not a git diff. The diff version needs a merge base, CI checks out
shallow, and an unreachable base made the file list empty, which is a green gate that checks
nothing.
hack/docs-files.shis the single place that resolves it and fails if the list is missing,empty, or names an untracked path.
Reference checking is not staged this way:
lint-doc-linksstill covers every tracked file.Also here
docker.gitea.combecame unreachable from CIand the devcontainer, and the pod stuck in
ImagePullBackOffkilled every leg inBeforeSuite.Same image, digest verified. Docker Hub anonymous rate limits are the trade; a pull-through cache
is the follow-up.
main. fix(attribution)!: partition author facts by declared audit route #263 merged adocs/INDEX.mdrow linking tostyle-guide.mdanddesign/docs-linting.md, which only exist here, sodoccheckfails onmaintoday and takes theRelease workflow down with it.
docs/style-guide.mdrecords the conventions, anddocs/design/docs-linting.mdcarries the measurements behind everyconfiguration choice.
AGENTS.mdandCONTRIBUTING.mddescribe the new gate.🤖 Generated with Claude Code