Add lint guard against internal /docs/concepts/* redirect-hop links - #21146
Add lint guard against internal /docs/concepts/* redirect-hop links#21146workprentice[bot] wants to merge 7 commits into
Conversation
github-actions
Bot
added
review:triaging
Pre-merge Review — Last updated 2026-08-26T23:19:11ZTip Summary: This is an infrastructure PR — a new Two evidence caveats from previous passes retired this round. The Review confidence:
Investigation log
🔍 Verification trail41 claims extracted · 28 verified · 2 unverifiable · 1 contradicted
🚨 Outstanding in this PRNo outstanding findings.
|
github-actions
Bot
added
review:outstanding-issues
|
Your site preview for commit 0efd545 is ready! 🎉 http://www-testing-pulumi-docs-origin-pr-21146-0efd545a.s3-website.us-west-2.amazonaws.com |
github-actions
Bot
added
review:stale
|
Addressed all four findings, pushed 77763c3. Outstanding 1 (SCAN_ROOTS gap): Added Outstanding 2 (--print-baseline envelope mismatch): Fixed to emit Low-confidence 1 (noisy green-run output): Implemented -- a passing run now prints one summary line; full per-file/per-line detail only prints on failure. Verified both paths (see below). Low-confidence 2 (count-only ratchet defeatable by substitution): Took your suggested path -- documented the limitation in the baseline's own Evidence, run just now on this branch: Fabricated-violation test (appended a Reverted the injection; guard returned to the single green summary line, exit 0.
@claude #update-review |
github-actions
Bot
added
review:in-progress
|
🤖 Review updated on @workprentice[bot]'s request. |
github-actions
Bot
added
review:no-blockers
CI runs `make build` before `make lint`, and widening SCAN_ROOTS to include static/ means the guard now sees webpack's own build output written into static/js/ and static/css/ under content-hashed filenames (bundle.b15c1961.js and friends -- see .gitignore's own list of these patterns). That output is compiled straight from theme/src/ts/*.ts and theme/src/scss/*.scss, which this guard already scans directly, so a stale link surviving only in the generated bundle would already have been caught at its real source. Scanning the bundle too just flags the same defect twice, under a filename that changes on every build, so a baseline entry can never pin it down -- this is exactly what failed PR #21146's own CI (static/js/bundle.b15c1961.js, 34 occurrences, 0 allowed). Exclude by filename pattern rather than dropping static/js and static/css from the scan outright, since those directories also hold genuine, hand-authored, git-tracked files (static/js/pulumi-mermaid-theme.js, static/js/versioned-docs.js) that should still be scanned -- verified this directly: a real stale link in a non-generated static/js file is still flagged after this change, only the exact generated-bundle filename is now skipped.
github-actions
Bot
added
review:stale
github-actions
Bot
added
review:in-progress
|
🤖 Review updated on @workprentice[bot]'s request. |
github-actions
Bot
added
review:outstanding-issues
…output Addresses both Outstanding findings and both Low-confidence findings from the review bot on this PR. Outstanding 1 -- SCAN_ROOTS omitted archetypes/ and static/, so a stale /docs/concepts/* link born in a page's own generation scaffold (archetypes/templates/template/index.md, which every new template page is generated from) went unseen forever, only to be caught in content/ after it had already been scaffolded into a real page. Added archetypes, static, and scripts to SCAN_ROOTS. Including scripts pulled in one occurrence, scripts/search/rank.js:80 (a historical-href check, not a live link) -- baselined it rather than dropping the scripts root, since a baselined known occurrence still ratchets while an unscanned directory does not. static/ added zero new occurrences (verified: it's mostly binary images, nothing text-scannable matched). Regenerated the baseline against master state (not this branch's own commits) so it describes what master actually has today; PR #21138 already removed the prior rank.js occurrence via a different code path, so this branch's own widened scan needed the fresh baseline to stay green. Outstanding 2 -- --print-baseline printed a flat {path: count} object, but loadBaseline() reads parsed.files || {}. Regenerating the baseline the obvious way (redirecting --print-baseline output to the baseline file) silently produced a baseline that loads as empty, then every baselined file fails with no hint why. Fixed --print-baseline to emit the same {_note, files} envelope the reader expects, building the files object from the sorted violation-map keys directly rather than using a JSON.stringify replacer keyed off Object.keys() (that reads as a filter and silently drops any key not already present). Also added --print-baseline to the Usage: block, which previously only documented it inside main()'s own comments. Low-confidence 1 -- implemented: a green run now prints one summary line instead of ~11 file headers and ~56 occurrence lines. Full detail only prints when the check actually fails. Verified: a fabricated violation still prints full file-by-file detail and exits 1; a clean run prints one line and exits 0. Low-confidence 2 -- decision (not code): kept the ratchet as a count-only check rather than switching to line-level tracking, since the baseline is meant to be short-lived and line tracking isn't worth the added churn for a file that should be deleted once #21138 and #21145 merge. Documented the limitation directly in the baseline's own _note so the next reader isn't surprised: fixing one stale link and introducing a different one in the same file keeps the count unchanged and still passes. Verified end-to-end: self-test passes (9/9 fixtures), make-equivalent full scan is green with the new summary line, --print-baseline output round-trips through loadBaseline() as 13 non-empty entries, and a fabricated /docs/concepts/testing/ link injected into archetypes/templates/template/index.md (a root only reachable after this fix) correctly fails the check with exit 1 and full detail, then cleanly reverts to green.
CI runs `make build` before `make lint`, and widening SCAN_ROOTS to include static/ means the guard now sees webpack's own build output written into static/js/ and static/css/ under content-hashed filenames (bundle.b15c1961.js and friends -- see .gitignore's own list of these patterns). That output is compiled straight from theme/src/ts/*.ts and theme/src/scss/*.scss, which this guard already scans directly, so a stale link surviving only in the generated bundle would already have been caught at its real source. Scanning the bundle too just flags the same defect twice, under a filename that changes on every build, so a baseline entry can never pin it down -- this is exactly what failed PR #21146's own CI (static/js/bundle.b15c1961.js, 34 occurrences, 0 allowed). Exclude by filename pattern rather than dropping static/js and static/css from the scan outright, since those directories also hold genuine, hand-authored, git-tracked files (static/js/pulumi-mermaid-theme.js, static/js/versioned-docs.js) that should still be scanned -- verified this directly: a real stale link in a non-generated static/js file is still flagged after this change, only the exact generated-bundle filename is now skipped.
…lone Addresses all 5 low-confidence findings from the pre-merge review on 110c61d: - isGeneratedBundle() now requires the path to sit under static/js/ or static/css/ (not just match the basename anywhere across all 8 scan roots), closing the /^chunk-.*\.js$/ scope-creep gap. - A filename-pattern match alone no longer settles it: the file also has to be something git itself treats as ignored (`git check-ignore`). This is the same source of truth the exclusion's own rationale already claimed to mirror, and it correctly tells apart the four checked-in static/js/consent-manager.*.js and marketing-homepage.*.js files (tracked, so check-ignore says "not ignored", so they're scanned) from an actual webpack-generated static/js/bundle.<hash>.js (untracked, so check-ignore says "ignored", so it's skipped). - The same exclusion now applies whether files are found via the repo walk or passed explicitly on the command line, closing the seam where `node check-concepts-links.js static/js/bundle.<hash>.js` used to scan a bundle the full-repo run would have skipped. - Added --self-test coverage for isGeneratedBundle via an injectable checkIgnore function, covering: a truly-ignored bundle (skip), a tracked file that only matches by name (scan), a pattern match outside static/js|css (scan), and a hand-authored static/js file that matches no pattern (scan). Verified directly, not just asserted: - Confirmed via `git check-ignore -v` that the four real checked-in files (consent-manager.cbf13435.js, consent-manager.e2d01ae6.js, marketing-homepage.31d6cfe0.js, marketing-homepage.46f9fbff.js) are NOT ignored (git's tracking overrides the .gitignore pattern match), so they are now scanned -- and produce 0 baseline entries since none currently carries a /docs/concepts/ link. - Wrote a synthetic static/js/bundle.deadbeef.js matching .gitignore's real `static/js/bundle.*.js` rule with a planted stale link: confirmed it is skipped (exit 0, not counted). - Wrote a synthetic static/js/consent-manager.aaaaaaa1.js (untracked, so matched and ignored like real build output) to confirm the distinguishing signal really is tracked-vs-untracked, not something else -- then removed both synthetic files before committing. - Ran the full node --self-test suite and `bash scripts/lint.sh` (markdown lint, this guard, prettier --check) locally: all exit 0.
- gitCheckIgnore(): the catch's two arms both returned false, collapsing "git says not ignored" (exit 1, routine) and "git is unusable" (any other failure) into one silent outcome. Keep the fail-closed behavior but warn once on the atypical case so a red build in an environment without git points at its actual cause. - Drop the unreachable styles.*.css bundle pattern and the static/css entry in GENERATED_BUNDLE_DIRS: SCANNABLE_EXTENSIONS has no .css entry, so walk() never reaches a stylesheet, generated or not. Both were dead code that overstated the guard's scope in its own comments. - Fix the three stale /docs/concepts/* links in archetypes/templates/template/index.md directly (projects, stacks #outputs, config), rather than leaving them baselined and confusing to the next person who runs `hugo new -k template`. Verified the new /docs/iac/concepts/... targets resolve 200 with no redirect hop. - Move the guard's self-exclusion from a directory-wide skip in walk()/shouldSkipDir() to a single-file check in runScan(), scoped to just this script rather than all of scripts/lint/. This closes the gap where `node check-concepts-links.js scripts/lint/check-concepts-links.js` disagreed with a full-repo run and reported ~20 violations against the script's own fixtures; both entry points now agree. Baseline regenerated: 12 files (archetypes/templates/template/index.md dropped out entirely now that its links are fixed rather than baselined).
PR #21138 and PR #21145 (the cleanup pass this guard exists to protect) both merged today. Every one of the 12 baselined files now has 0 real occurrences of an internal /docs/concepts/* link on master, so the 57-occurrence allowance list was pure dead weight: a future PR could have introduced up to that many brand-new stale links per file and the guard would stay green and silent, which is exactly the regression it exists to catch. Rebased onto current master and reset the baseline to {}, updating its _note and this script's own doc comments to stop describing the two PRs as still open. Verified against the merged tree: a full scan finds 0 occurrences in all 12 previously-baselined files, and a fresh --print-baseline confirms an empty {_note, files: {}} envelope is the correct state.
e6bcf7b to
6270eeb
Compare
|
Confirmed, and thank you for catching this — rebasing onto master was the missing step. #21138 and #21145 both merged before I pushed the previous commit, but this branch was still based on the pre-merge tree, so the guard's own baseline was silently stale. Rebased onto current @claude #update-review |
github-actions
Bot
added
review:in-progress
|
🤖 Review updated on @workprentice[bot]'s request. |
github-actions
Bot
added
review:no-blockers
|
Restored the dropped caveat sentence in the shipped @claude #update-review |
github-actions
Bot
added
review:stale
|
🤖 Review updated on @workprentice[bot]'s request. |
github-actions
Bot
added
review:no-blockers
|
Confirmed clean: the latest pre-merge review shows 0 Outstanding, 0 Low-confidence, 0 Pre-existing, 12 Resolved. CI is green. This PR is ready for review/merge whenever you have a moment. Understood on the standing directive -- I'll drive every finding on future pulumi/docs PRs (including low-confidence and style) to a resolved state before considering a PR done, not just Outstanding. |
3 participants
This adds a lint check that catches new internal links to the retired
/docs/concepts/*URL space before they merge.The recurring problem
Pages under
/docs/concepts/moved to/docs/iac/concepts/a while back (PR #21072), and old internal links to the retired path keep resurfacing in new content, each one costing readers and crawlers a redirect hop. This is now the ninth pass of a recurring sweep that finds and fixes a fresh batch every time: PR #21138 fixes the batch reintroduced since the last sweep, and PR #21145 fixes one more in a blog post that earlier passes had skipped over a stale conflict assumption, plus a related hop class (/docs/iac/concepts/options/*) found on review. Fixing the same class of link over and over without a gate just means a tenth pass is coming.What the guard does
scripts/lint/check-concepts-links.jsscanscontent/,layouts/,theme/,assets/,data/,archetypes/,static/, andscripts/for/docs/concepts/...and flags it only when the reference actually targets pulumi.com. It deliberately does not flag:/docs/concepts/path, such askubernetes.io,developer.okta.com, ordeveloper.hashicorp.com/vault/docs/concepts/...(the last one is a real case in this repo where/docs/concepts/is not immediately preceded by the external host, so a naive "does the host appear right before this" check would miss it)./docs/concepts/is not the start of the link, such as/images/docs/concepts/foo.png(another real case in this repo).aliases:frontmatter entries, which are redirect stubs from the old URL to the new one and are supposed to exist.Run
node scripts/lint/check-concepts-links.js --self-testto see the classifier's fixtures for each of these cases side by side with the ones it does flag (markdown links, reference-style link defs, HTMLhref, and absolutehttps://www.pulumi.com/docs/concepts/...URLs).Why
archetypes/andstatic/are in scopeThe review caught a gap in the original root list:
archetypes/templates/template/index.mdis the scaffold every new template page is generated from, and it carries three/docs/concepts/links today. The original scan didn't coverarchetypes/, so the guard reported green on it forever while every page scaffolded from that template inherited the stale links intocontent/, where the guard does look, but only after the fact.archetypes/andstatic/are now scanned;scripts/came along with them, which surfaced one baselined occurrence,scripts/search/rank.js:80(a historical-href check, not a live link).The baseline
scripts/lint/concepts-links-baseline.jsonrecords the current known occurrences onmaster, all of them already fixed on the branches for PR #21138 and PR #21145 (both still open for review), so this PR does not have to depend on those two merging first. It is a ratchet: a file exceeding its recorded count fails the build, a file with fewer than its recorded count only prints a warning so merging the two open PRs does not redden anyone else's in-flight work. The baseline is a count per file, not a per-line record, so swapping one stale link for a different one in the same file keeps the count unchanged and still passes; this limitation is called out directly in the baseline's own_noterather than fixed with per-line tracking, since the whole file is meant to be deleted once #21138 and #21145 merge.Once PR #21138 and PR #21145 both merge, every entry in the baseline becomes stale (0 real occurrences) and the baseline file should be reset to an empty
filesobject or the file deleted along with its line inscripts/lint.sh, whichever a reviewer prefers. I verified this directly: merging both PR branches into a fresh worktree fromorigin/masterand running the guard against that merged tree with no baseline file present at all reports zero violations.--print-baselineRegenerates the baseline file in the exact envelope
loadBaseline()expects ({_note, files}), not a flat{path: count}object -- the earlier version of this script emitted the latter, which would have silently produced a baseline that loads as empty and then fails every baselined file with no hint why. Round-trip verified:--print-baseline > filefollowed by loading that file back reports all entries present.Output on a green run
A passing run now prints one summary line (
No new /docs/concepts/* internal links found (N file(s) with baselined occurrences, all within their allowance).) instead of every baselined file's full detail. Full per-file, per-line detail still prints when the check actually fails, which is when it matters.Wiring
Added one line to
scripts/lint.sh, whichmake lintalready runs and CI already gates on, so this becomes a required PR check with no new workflow needed.Verification performed
node scripts/lint/check-concepts-links.js --self-testpasses (9/9 fixtures).origin/masterwith the shipped baseline: green, single summary line, 13 files within allowance.--print-baseline > file, then loaded that file back throughloadBaseline(): 13 non-empty entries, matching the committed baseline exactly./docs/concepts/testing/link intoarchetypes/templates/template/index.md(a file only reachable with the widenedSCAN_ROOTS), ran the guard: failed with exit 1 and full detail identifying the new line. Reverted the injection, ran again: green.npx prettier --checkagainst both changed files with this repo's.prettierrc.json: passes.🧠 This PR was created by workprentice.