security(marketing): migrate static site to Astro 7.1+ - #1293
Conversation
backnotprop
left a comment
backnotprop
left a comment
There was a problem hiding this comment.
TLDR
Approve after dropping the four sidebar.order edits. The migration itself is sound and I verified it against a real build rather than by reading: Astro 7.1.6 produces the same 40 routes, byte-identical RSS/sitemaps, zero broken asset references, byte-identical text for all 183 code blocks, and byte-identical hydrated island DOM for both React islands. The security claim checks out exactly against the live alert list. The one substantive problem is that the four docs frontmatter edits are unnecessary, and they are the sole cause of a sidebar reorder across 28 docs pages, which contradicts the PR body's parity claim.
The output: "static" CI failure I started this review on was fixed mid-review by 2c4f986. Confirmed resolved: CI=true bun test security-build.test.ts now passes on the latest head, and all checks are green.
Build evidence below comes from two clean git worktree builds (origin/main ef49c701 vs PR head 2c4f986f), bun install --frozen-lockfile, bun run build:marketing, Bun 1.3.14.
VERIFIED-OK
Build parity. Both builds emit 40 HTML routes; the route set diffs clean.
main: 155 files, 40 html PR: 154 files, 40 html
HTML route set: IDENTICAL
asset refs: main 197 refs / 0 missing PR 197 refs / 0 missing
The one-file delta is exactly what the body claims: main emits GitHubStars.COO7oFrR.js + GitHubStars.DYNNI5Yh.js, PR emits a single GitHubStars.B0mHpvtJ.js. js count goes 6 to 5, every other extension count is unchanged.
The auto-generated keyboard-shortcuts page survives. This was the page I most expected to break, since apps/marketing/src/pages/docs/[...slug].astro special-cases it to render from the shortcut registry instead of the markdown body. It renders identically: 29 <table>, 159 <tr>, 4 <h2> in both builds.
Content parity. Normalized text across all 40 pages shows zero content differences outside <pre> blocks. Inside them, all 183 code blocks are byte-identical after entity decoding. The only serialization changes are cosmetically equivalent entity choices, for example <file.md> becoming <file.md> in blog/plannotator-meets-pi, and & becoming & in og/twitter titles. Both decode to the same characters.
RSS and sitemaps are byte-identical. rss.xml, sitemap-index.xml, sitemap-0.xml all pass cmp.
Both islands actually hydrate. I served each dist/ and dumped the post-JS DOM with headless Chromium across home, blog index, a blog post, the shortcuts page, code-review and workspaces. Island counts, <svg> counts and <button> counts match on every page, and the hydrated island inner DOM is character-for-character identical:
ModeToggleIsland inner DOM: 621 chars, identical main vs PR
GitHubStars inner DOM: 263 chars, identical main vs PR
So the theme toggle, the only client JS on the site, renders and hydrates unchanged.
compressHTML: true at astro.config.mjs:12 is a real fix, not defensive noise. I checked the installed schema defaults rather than trusting the comment:
astro 5 node_modules/astro/dist/core/config/schemas/base.js:33 compressHTML: true
astro 7 node_modules/astro/dist/core/config/schemas/defaults.js:29 compressHTML: "jsx"
The default genuinely flipped to "jsx", so pinning it is correct and is likely why the text parity above came out as clean as it did.
Security claim is precise. I pulled the live Dependabot alerts. There are exactly 8 open astro alerts on apps/marketing/package.json, and all 8 GHSA ids match the PR body one-for-one. The highest patched floor across them is GHSA-4g3v-8h47-v7g6 at 7.1.0, which confirms the body's "7.1.0 is the common safe floor" reasoning. 7.1.6 clears all 8. The remaining open alerts are 4 on @earendil-works/pi-coding-agent, correctly left to #1291.
Deploy safety. deploy.yml is untouched. Output dir stays apps/marketing/dist, the /_astro/ prefix is unchanged, top-level dist/ entries are identical between builds, and the file-extension histogram is unchanged apart from the js dedup, so the aws s3 sync --delete layout and content-type surface are unaffected. Worth noting the workflow's verify job gates deploy on a green Test run for the same SHA, so a red main could not have silently shipped this.
MDX removal is clean. Zero .mdx files under src/, and no source file imports z from astro:content other than content.config.ts:3, which correctly moves to astro/zod. Full repo bun run typecheck passes (exit 0).
SHOULD FIX: drop the four sidebar.order edits
The PR body says the reassignment keeps "generated nav and pagination order byte-for-byte equivalent". I tested that directly and it does not hold, in both directions.
Files:
apps/marketing/src/content/docs/getting-started/ui-settings.md:5order 6 to 8apps/marketing/src/content/docs/guides/claude-code.md:5order 4 to 5apps/marketing/src/content/docs/guides/opencode.md:5order 5 to 6apps/marketing/src/content/docs/guides/opencode-migration-0-19-1.md:5order 6 to 7
I built the Astro 7 migration three ways and compared every /docs/ href on all 40 pages:
main (Astro 5) vs PR as submitted -> 28 pages differ
main (Astro 5) vs PR minus the 4 edits -> 4 pages differ
Two conclusions:
-
The edits were not needed. With the four files reverted to their main values and everything else in the PR intact, Astro 7 reproduces main's sidebar order exactly:
Installation, Quickstart, Configuration, Claude Code, OpenCode, UI Settings, OpenCode Migration (0.19.1), .... Astro 7's loader resolved the order-6 tie the same way Astro 5 did. The premise that Astro 7 "exposed ambiguous ties" needing repair is not borne out by the build. -
The edits are what break parity. As submitted, the sidebar on all 28 docs pages moves
OpenCode Migration (0.19.1)ahead ofUI Settings, because ui-settings was pushed to 8 while the migration doc went to 7, resolving the old tie in the opposite direction from the rendered Astro 5 order.
They also do not buy the pagination stability they look aimed at. Prev/next still differs on exactly 4 pages either way, just a different 4:
PR as submitted -> 4 pagination diffs (plan-review, ui-settings, opencode, opencode-migration)
PR minus the 4 edits -> 4 pagination diffs (configuration, claude-code, opencode, verifying-your-install)
Reverting the four files is strictly better: 28 pages of sidebar drift go away and the pagination situation is no worse. It also shrinks a security migration back out of content files it has no reason to touch.
Since docs are the deprecated section, none of this is urgent, and if you would rather not respin, letting the sidebar drift is a defensible call. But the PR body should not claim byte-for-byte nav parity while changing 28 pages.
CONSIDER
The residual 4-page pagination drift is a latent bug in apps/marketing/src/layouts/Docs.astro:19-20, not something this PR caused.
const allDocs = (await getCollection('docs')).sort(
(a, b) => a.data.sidebar.order - b.data.sidebar.order
);This sorts every doc globally by sidebar.order alone, but order values restart per section, so the global list is mostly ties. Array.prototype.sort is stable, so tie order is whatever getCollection traversal order happens to be, which is exactly the thing an Astro major can change. That is why prev/next moves on 4 pages no matter which frontmatter values you pick. A real fix is sorting by (section, order, id), but on a deprecated docs section that is probably not worth spending on. Mentioning it so the drift is understood as pre-existing fragility rather than migration damage.
security-build.test.ts:34 builds into the repo working tree.
temporaryRoot = await mkdtemp(join(marketingRoot, '.astro-security-'));afterAll cleans it up on a normal run, but an interrupted or killed run leaves an apps/marketing/.astro-security-XXXXXX/ directory containing a full dist/. I confirmed it is not gitignored (git check-ignore returns nothing), so it would surface as untracked noise. Either add .astro-security-* to the marketing .gitignore or use os.tmpdir() if Astro's root resolution allows it.
The new test runs a full Astro build inside bun test. It cost about 1.2 to 1.7s locally and roughly 8s on the CI runner, against a 30s beforeAll timeout. Fine today, but it is now a per-suite Astro build on a cold cache, so the headroom is thinner than it looks.
Held Dependabot PRs
- #1285 (
@astrojs/mdx4.3.14 to 7.0.5): fully subsumed, closeable. This PR deletes the dependency and themdx()integration outright, and I confirmed zero.mdxfiles exist underapps/marketing/src. #1285 would reintroduce a package with no consumer. - #1281, #1282, #1283, #1284: not subsumed. #1284 (wrangler) touches paste and waitlist services, #1283 (parse5) touches pi-extension and packages/shared, #1282 (@types/node) touches hook/portal/review. #1281's group does include
@astrojs/rss,tailwindcssand@tailwindcss/vite, which are marketing deps, but this PR leaves all three untouched, so it is not superseded. Expect abun.lockconflict between #1281 and this PR whichever lands second.
Scope and CI
Scope is appropriately tight for a startup security fix: config, package.json, lockfile, one content-config move, one test. No unrelated churn, no over-engineering, no adapter or architecture drift. The only scope I would trim is the four content files above.
On the deprecated docs nuance: the section still builds and renders correctly, all 28 pages present with working links, so nothing is broken there. The effort that did land on it is the four frontmatter edits, which is the wasted-and-mildly-harmful category rather than wasted-but-harmless, hence the SHOULD FIX.
CI is green on 2c4f986f, including build, both test jobs, npm-package, gitleaks and zizmor. I independently reproduced the pre-fix failure (CI=true makes picocolors emit ANSI, so the raw toContain('output: "static"') could never match) and confirmed the stripAnsi fix resolves it.
AI-assisted review (Claude, Opus) under maintainer direction.
backnotprop
left a comment
backnotprop
left a comment
There was a problem hiding this comment.
Delta review: 9cfdfb2
TLDR: RESOLVED, and with a correction I owe you on my own earlier finding. The four sidebar.order edits are reverted (docs frontmatter is now byte-identical to main), the migration is intact, and CI is green including the security-build test. Ship it. But I need to walk back the parity framing in my first review: reverting does not produce byte-for-byte sidebar/pagination parity with main, and it was never going to, because that ordering is non-deterministic in main too. Details below so the record is honest.
Confirmed on 9cfdfb2
- Revert landed.
git diff origin/main...HEAD -- apps/marketing/src/content/docs/is empty. The full marketing diff is now just astro.config.mjs, package.json, the content-config move, and the new test. Clean. - Migration intact, dist deployable. Fresh worktree build: 40 routes, route set matches main, 0 missing asset refs across 197 references, both React islands present (
GitHubStars,ModeToggleIsland), andrss.xml/sitemap-index.xml/sitemap-0.xmlare byte-identical to main. Page content (nav stripped) is unchanged apart from the already-cleared Shiki<pre>whitespace serialization. - CI green on 9cfdfb2: both
testjobs,build,npm-package. I re-ran the security-build test underCI=truelocally (the ANSI condition that flaked): 2 pass, 0 fail. - #1285 still closeable. mdx is gone from package.json, astro.config.mjs, and there are zero
.mdxsource files. Unchanged by the revert.
Correction to my earlier SHOULD FIX
My first review said the four edits "cause a 28-page reorder" and that reverting restores parity. That was half right. The reorder is real, but the revert does not remove it, and the edits were not really its cause. Evidence from this pass:
- I rebuilt main from
origin/maina second time in a fresh worktree. Comparing that main to the no-edits build still shows the same 28-page delta (24 pages sidebar reorder, 4 pages prev/next shift). - Two separate checkouts of the identical
origin/maincommit resolve the ordering differently from each other, while rebuilds inside one checkout are stable. So the order depends on filesystem enumeration order of the checkout, not on the source. - Root cause:
apps/marketing/src/layouts/Docs.astro:19sorts all docs globally bysidebar.orderalone, and there are two order-value ties (ui-settingsandopencode-migration-0-19-1both at 6;claude-codeandverifying-your-installboth at 4). Stable sort then leaves tie order at the mercy ofgetCollectiontraversal order, which varies per checkout, in both Astro 5 and Astro 7.
So my earlier "4 diffs vs 28 diffs" was an artifact of which checkout I happened to compare against. The honest statement is: the rendered sidebar/pagination order between any two builds of this site is not stable today, independent of this PR. Reverting the edits was still the correct call (a security migration should not carry content-file changes, and it keeps the ordering no worse than main), and your corrected wording is accurate. But nobody should promise byte-for-byte nav parity until Docs.astro gets a stable tie-breaker (sort by [section, order, id]). That is a pre-existing latent bug on the deprecated docs section, out of scope here, and low stakes: it only ever swaps two adjacent already-tied docs items.
Verdict
Merge-ready. The dist is sound and deployable, the security remediation is unchanged and precise (all 8 astro advisories cleared at 7.1.6), and #1285 is safe to close as subsumed. The only open item is the pre-existing docs-sort instability, which is not this PR's to fix.
AI-assisted review (Claude, Opus) under maintainer direction.
1 participant
Summary
Migrates the static
apps/marketingsite from Astro 5.18.2 to Astro 7.1.6 and@astrojs/react6.0.2, removes the unused MDX integration, adopts Astro 7's content-config location/API, and adds a focused build regression test for thedefine:varsbreakout class and static-only artifact shape.The production architecture does not change: Astro still builds with
output: "static", no adapter or request-time Astro server is introduced, and deployment remains adist/upload to S3 followed by CloudFront invalidation. This PR does not deploy itself.Security rationale
This resolves the eight current Astro Dependabot alerts:
define:varsscript breakoutrenderHTMLElement/spread fixCVE-2026-73422 affects Astro through 7.0.9, so Astro 7.1.0 is the common safe floor. This PR selects 7.1.6: the latest eligible release under the repository's seven-day dependency-age policy when audited. Astro 7.2.1 was one day old and was deliberately excluded.
Existing production reachability was low: this site is prerendered and serves only static files; there are no server islands, adapters, transition directives, dynamic slot names, dynamic spread attribute names, or custom
renderHTMLElementpath. The onedefine:varssite uses build-time defaults/operator-controlled environment values rather than visitor input. That reduces present exploitability, but does not justify retaining a vulnerable compiler/runtime toolchain, especially when future content can change those assumptions.Migration
astro: 5.18.2 -> 7.1.6@astrojs/react: 4.4.2 -> 6.0.2@astrojs/mdxandmdx(); inventory found 36.mdfiles, zero.mdxfiles, and no MDX imports/usages beyond the integrationsrc/content/config.tstosrc/content.config.tsand imports Zod fromastro/zod, per the Astro 7 migration guide@tailwindcss/viteunchanged; their resolved versions are Vite 8 compatibleoutput: "static"and no adaptercompressHTML: trueexplicitly to retain Astro 5's HTML-aware whitespace handling instead of Astro 7's new JSX whitespace defaultsidebar.orderfrontmatter unchanged; the equal-order tie remains a pre-existing content issue rather than part of this migrationbun.locksecurity-build.test.ts, which builds with a mixed-case/encoded script terminator and asserts escaped output plus the absence of server artifacts/endpoints/manifestsSupply-chain audit
The dependency update skill's maintainer, age, provenance/signature, tarball-diff, runtime-dependency, lifecycle-script, suspicious-pattern, and upstream migration/release checks were completed before implementation.
withastro/astro's release workflow; no consumer install script. This is a substantive major (945 package files / 66,368 tarball diff lines), replacing the compiler and Vite major and adding eight direct runtime dependencies, so it was treated as review-required rather than a routine bump.@astrojs/react6.0.2 (14 days): maintainer set unchanged; signed with matching SLSA provenance; no lifecycle scripts or suspicious executable/network patterns. Tarball grows 27 -> 29 files; 13 files changed/added for Astro 7/Vite 8 integration, option serialization, filtering, and React output handling. New runtime dependencies aredevalueand@astrojs/internal-helpers.@astrojs/mdxremoval: safe by repository inventory. Astro's built-in Markdown/content loader covers every current source; removing the integration reduces the graph.Astro 7 requires Node >=22.12.0 and Vite 8. The migration was built with repository-pinned Bun 1.3.14 and separately with supported Node 24.15.0. Official Astro 6 and Astro 7 migration guidance and upstream release notes were reviewed.
bun auditwas also reviewed. Its remaining monorepo findings are pre-existing/out-of-scope dependency lines (including unchanged RSS/SVGO/sharp reports), not additions to silently fold into this security migration. This PR intentionally does not absorb unrelated Dependabot/Renovate work.Validation
Reproducibility and build
bun install --frozen-lockfilewith Bun 1.3.14: pass (1,977 platform-applicable packages installed)bun run buildinapps/marketing: pass, 40 static pagesnode node_modules/astro/bin/astro.mjs buildwith Node 24.15.0: pass, 40 static pagesbun run typecheck: passbun test: 3,374 pass, 496 skip, 0 fail; 9,983 assertions across 3,870 tests / 322 filesStatic output and content
The one-file reduction is intentional: React 6 deduplicates two equivalent GitHub-star island chunks into one shared chunk. Hashed JS/CSS names and the shared React runtime naturally change.
.mjs/.cjs, server directory, adapter artifact, server-island endpoint, request-time manifest, or server bundle indist/Astro 7's Satteri Markdown processor intentionally changes some serialized article markup: smart typography is normalized and highlighted-code DOM differs. The content, headings, metadata, links, and rendered presentation remain equivalent.
Security and hydration
The hostile Workspaces build value included both an encoded termination and literal mixed-case termination:
Astro 5 emitted the literal
</ScRiPt ><img...breakout sequence. Astro 7 emits\u003c/ScRiPt >\u003cimg...; the regression test also asserts the literal terminator/tag are absent.Browser checks on the built static site confirmed both React islands hydrate, the GitHub-star island loads live data, the theme island switches Light/Dark and restores correctly, and the console remains error-free.
Visual comparison
Captured and inspected Astro 5 vs Astro 7 at 1440x1000 for home, docs installation, blog list, blog article, code review, and Workspaces, plus 390x844 mobile captures for home, docs, code review, and Workspaces. No genuine layout, typography, content, responsive, or interaction regression was found. Deterministic-page SSIM was 0.998-1.000; the home-page pixel deltas were the live GitHub star count and animation capture timing, not a migration difference.
Risk, rollout, and rollback
Main migration risks are the new Node floor, Vite/compiler major, and Satteri serialization differences. The static artifact comparison, two-runtime builds, malicious-input test, full suite, browser hydration, and desktop/mobile review materially reduce those risks.
Recommended rollout after review/green CI:
dist/output;If production verification finds a regression, restore/redeploy the retained previous static artifact and invalidate CloudFront, then fix forward on Astro 7. Do not treat returning to vulnerable Astro 5 as a long-term remediation.
PR #1285's standalone
@astrojs/mdxbump is conceptually superseded by this coherent migration. This PR does not mutate, close, or merge #1285.CI
The first CI attempt exposed one test-harness portability issue: Astro's CI log colored the quoted
"static"value, so a plain substring assertion failed even though the build and malicious-input assertion passed. Commit2c4f986fmakes that assertion ANSI-color agnostic and reproduced locally underFORCE_COLOR=1.All checks on
9cfdfb27are green after restoring the four originalsidebar.ordervalues:Release eligibility, publish, attestation, and release jobs skipped as expected for this pull request; no package was published and nothing was deployed.
Review follow-up
9cfdfb27drops the four proposed docs frontmatter edits;apps/marketing/src/content/docs/now has zero diff frommain