Nick Vigilante
35b3052ea8
## Summary
Redirects apply in file order and the first match wins, so rules have to
run from most specific to least.
Nothing enforced that, so a wildcard placed above a narrower rule inside
its subtree silently swallowed it.
## Problem
Given these two rules in that order, the second never applies:
```json
[
{ "source": "/docs/retired/:section(.*)", "destination": "/docs/reference/:section" },
{ "source": "/docs/retired/foo", "destination": "/docs/about" }
]
```
A reader asking for `/docs/retired/foo` is sent to `/docs/reference/foo`
instead of `/docs/about`.
If `/docs/reference/foo` happens to exist, every existing check passes:
the destination resolves, there is no duplicate source, no chain, and no
loop.
The rule is simply dead, and the only symptom is readers landing on the
wrong page.
`docs/redirects.json` is empty today, so this adds the guard before the
file gets its first wildcard rather than after.
## Fix
Compare each rule against the ones before it and fail when an earlier
source matches every URL a later source would.
Coverage is decided on path boundaries, not string prefixes, so
`/docs/retired/:path*` does not cover `/docs/retired-too/page`.
Two cases stay legal on purpose:
- An exact rule above a wildcard on the same prefix, which claims the
bare prefix and leaves the subtree to the wildcard.
- A regex wildcard above an exact rule on its bare prefix, since
`/:name(.*)` never matches the prefix itself.
Rules that share a source are still reported as duplicates, not as
unreachable, so one mistake produces one message.
## Changes
- `rules.go`: add `pattern.covers` and `checkOrder`, called from
`checkAcrossRules`.
- `README.md`: document the ordering requirement and the new failure.
- `main_test.go`: 11 cases covering each pattern pairing, both
orderings, the path-boundary case, reporting once against the first
coverer, and the duplicate interaction.
## Testing
- `go test ./scripts/docsredirectscheck/` passes;
`TestCheckRedirectsOrder` runs all 11 subtests.
- `make lint/docs-redirects` passes against the committed
`docs/redirects.json`.
- `gofmt -l` and `go vet` are clean on the package.
- Ran the binary against the example above: it fails with
`unreachable: rule [0] (source "/docs/retired/:section(.*)") comes first
and matches every URL this rule would; order rules from most specific to
least`,
exits 1, and passes with exit 0 once the two rules are swapped.
<details>
<summary>Why this check, and what it is a prerequisite for</summary>
`docs/redirects.json` is validated in CI and triggers a docs rebuild,
but no site reads it yet.
The redirects actually served live in two other hand-maintained files:
`coder.com/redirects.json` (297 rules, 173 under `/docs`) and
`docs-engine`'s `apps/router/redirects.json` (221 rules).
The plan is to make `docs/redirects.json` the single authored source and
have docs-engine compile it per version, so `apps/router/redirects.json`
becomes generated output.
Classifying the router's 221 rules showed all of them are same-version
moves once the version prefix is stripped, and that expanding them into
this file's narrow grammar yields 279 authored rules.
That expansion is what makes this check necessary now.
Collapsing many exact rules into wildcards is an engine optimization,
and the authored file will carry both wildcards and exact rules in the
same subtree, which is exactly the shape that can silently swallow a
rule.
Adding the guard while the file is still empty means the migration
cannot introduce an unreachable rule without CI saying so.
</details>
---
Resolves DOCS-1188.
Opened by Coder Agents on behalf of @nickvigilante.
<details>
<summary>CI note: one unrelated flake</summary>
`test-go-pg (ubuntu-latest)` failed on `coderd/x/chatd
TestChatTurnStopsAtStepLimit/Default`.
This PR changes three files under `scripts/docsredirectscheck` and
nothing in `coderd/`, so the failure is unrelated to the diff.
`lint`, `gen`, `fmt`, `check-build`, `test-go-race-pg`, `test-go-pg-17`,
`test-e2e`, and `test-go-pg` on macOS and Windows all pass.
The first run of this PR also had `changes`, `cla`, and `release-labels`
cancelled rather than failed, all three at exactly 15m03s, which skipped
the Go jobs entirely. A `--failed` rerun produced the results above.
</details>
docsredirectscheck
docsredirectscheck validates docs/redirects.json, the list of redirects the
documentation website applies for pages that have moved or been removed. It
runs as make lint/docs-redirects (part of make lint and make lint-light),
so a bad redirect is caught in the pull request that introduces it.
A missing docs/redirects.json passes.
The rule file
docs/redirects.json is a JSON array of rules:
[
{
"source": "/docs/about/architecture",
"destination": "/docs/about",
"permanent": true
},
{
"source": "/docs/tutorials/ai-agents/:path*",
"destination": "/docs/ai-coder/:path*"
}
]
sourceanddestinationare full site paths that start with/docs/. Write them without a version segment such as/@main; the docs website adds that itself.permanentis optional and defaults totrue.- Rules apply in file order and the first match wins, so they must run from most specific to least. Put an exact rule above any wildcard that would also match it.
- A
sourceis an exact path, a trailing/:name*(matches the prefix and everything beneath it), or a trailing/:name(.*)(matches everything beneath the prefix, not the prefix itself). - A
destinationis an exact path, a trailing/:nameor/:name*that reuses the source's parameter, or anhttp(s)://URL.
What it catches
These fail the check:
- A malformed file. Not valid JSON, not an array, an entry that is not an
object, a missing or empty
sourceordestination, a non-booleanpermanent, or an unknown field. - Unsupported syntax. A source outside
/docs/, a version segment, or any pattern beyond the forms above. - A destination that is not a page. An internal destination must be a route
in
docs/manifest.json. A pattern destination must point into a section that has pages, and may only use a parameter the source defines. - A source that hides a live page. Redirects apply before pages, so a rule whose source matches a page in the manifest makes that page unreachable.
- Duplicates, chains, and loops. Two rules with the same source, a rule whose destination is another rule's source (point it at the final page instead), and cycles.
- A rule that can never apply. An earlier rule whose source matches every URL a later rule would, which makes the later rule unreachable. A wildcard above a rule inside its subtree is the usual cause; reorder so the specific rule comes first. An exact rule above a wildcard on the same prefix is allowed, because it claims the bare prefix and leaves the subtree to the wildcard.
Removed-route warning
On a pull request the check also compares docs/manifest.json with the base
branch. If routes were removed, docs/redirects.json was not changed, and no
existing rule already covers the removed routes, it prints a warning. In GitHub
Actions the warning is an annotation on the manifest. It never fails the run,
because pages are sometimes removed on purpose.
The comparison is by route, not by file, so renaming admin/foo/index.md to
admin/foo.md (same URL) is not a removal.
In GitHub Actions the base is the first parent of the pull request's merge
commit, which is the tip of the branch the change merges into. Elsewhere pass
-base <rev>; with neither, or when the revision is unavailable (a shallow
checkout), the warning is skipped with a note.
How it decides which pages exist
It derives routes from docs/manifest.json exactly as the docs website does:
strip a leading ./, then strip the suffixes README.md, index.md, and
.md one after another, trim slashes, and prefix /docs. That includes the
website's quirk of matching the suffixes as plain text rather than per path
segment, so a file named my-index.md becomes the route /docs/.../my-. Every
manifest entry with a path counts as a page, whether or not it appears in the
sidebar. The landing page /docs always exists, and /docs/about exists when
the first page is titled "About".
Usage
go run ./scripts/docsredirectscheck [-manifest path] [-redirects path] [-base rev]
Exit status is 0 when the file is valid or absent, 1 when it has problems, and 2 when the check itself could not run (for example the manifest is unreadable).
Limitations
- Chains through a pattern destination are not followed, because where such a destination lands depends on the URL being redirected.
- Only the three source forms above are understood. Regular-expression groups, version ranges, and mid-path parameters are rejected rather than guessed at.
- The removed-route warning compares routes only; it does not know whether an external site links to a removed page.