Conversation
Every file the host deploy uploaded carried "max-age=63072000, public, immutable", because config/deploy.js widened ember-cli-deploy-s3's filePattern to '**/*' while leaving its default cacheControl in place. That default is written for the plugin's own default pattern, which covers hashed assets only. The directive is correct for anything under assets/, whose names are content-addressed. It is wrong for everything else: index.html is the only document naming the current bundle, so a browser holding an immutable copy keeps loading the build it first cached, for two years or until the entry is evicted. The Embroider virtual entrypoints (@embroider/virtual/vendor.js, which carries EmberENV, and @embroider/virtual/app.css) and the service workers have the same stable-name-changing-content shape. All of it was observable on both deployed environments. Split the upload into two passes over one dist via ember-cli-deploy's pipeline alias: assets/** keeps the immutable directive, and its complement uploads with no-cache, max-age=0, must-revalidate plus an Expires in the past. The patterns are complements, so every file is uploaded by exactly one pass. CloudFront honours the origin directive and its cache policy floors at a one-second TTL, so the edge follows. The suite drives the real plugin with its S3 client stubbed, so it reads the plugin's own file matching and the exact directive each file would be uploaded with. It also checks the assets/ claim the split rests on against a built dist when one is named. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The directive each deployed file carries is decided entirely by config/deploy.js, and a deploy is the first place a mistake shows up -- by which point a wrong directive is already in browsers that will not ask again. Run the suite as its own job on boxel-touching PRs, where it needs nothing running, and again in the host build workflow against tmp/deploy-dist, which is the artifact the deploy uploads and the one place the assets/ claim can be checked against real output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 185ee2fc4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Preview deploymentsHost Test Results 1 files ±0 1 suites ±0 2h 5m 11s ⏱️ -37s Results for commit e9bc84c. ± Comparison against earlier commit 185ee2f. Realm Server Test Results 1 files 244 suites 1h 33m 51s ⏱️ Results for commit e9bc84c. |
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Went after the two glob patterns under the plugin's own minimatch, the pipeline.alias interaction with the build-only target and the rest of the pipeline, whether the suite can fail, and the CI wiring. Did not look at the CloudFront distribution config, which lives outside this repo.
One blocking issue: the dist gate added to the host build workflow fails on its first run, and its first run is a deploy. The directive split itself holds — the two patterns partition the real 334-file deploy dist 321/13 with no overlap and nothing dropped, and the suite's assertions do discriminate: four of them fail against a config without the split.
- Reconcile
DIST_FILESwith the dist the deploy uploads — two entries are absent from it, two real stable-named files are missing from it. Blocking; see the comment on the array. - Assert the partition over the walked
builtlist rather than only overDIST_FILES— see the comment on the dist walk. support/**is outside the host tsconfiginclude, so nothing typechecks the suite — see the comment oninterface Upload.- The unaliased branch in
uploadsForis unreachable — see the comment on it.
Adjacent, out of scope: the production host deploy ships test-modules/good.js, test-modules/bad.js and test-realm-sw.js. That predates this change; the file list introduced here is just the first place it is written down.
| // Paths a deployed host build serves — every stable-named one it has, plus a | ||
| // sample of the content-addressed ones. `assets/` is the only place the build | ||
| // writes content-addressed filenames; every other path keeps its name across | ||
| // builds, which is what makes an immutable directive on one of them a promise | ||
| // the filename cannot keep. | ||
| const DIST_FILES = [ | ||
| 'index.html', | ||
| 'robots.txt', | ||
| 'auth-service-worker.js', | ||
| 'test-realm-sw.js', | ||
| 'testem.js', | ||
| 'boxel-favicon.png', | ||
| 'boxel-webclip.png', | ||
| 'default-realm-icon.png', | ||
| 'boxel-ui-checksum.txt', | ||
| '@embroider/virtual/vendor.js', | ||
| '@embroider/virtual/app.css', | ||
| 'test-modules/good.js', | ||
| 'tests/index.html', | ||
| 'assets/main-DMcx_nWD.js', | ||
| 'assets/main-BlRjxoZ5.css', | ||
| 'assets/editor.main-CQnjGZh9.js', | ||
| 'assets/ai-assist-icon-animated-C8cXGt-5.webp', | ||
| ]; |
There was a problem hiding this comment.
[Claude Code 🤖] testem.js and tests/index.html are not in the dist the deploy uploads, so the HOST_DIST_DIR: tmp/deploy-dist step in the host build workflow fails. That workflow is reachable only from the deploy workflow, so its first run is a deploy — and release lists build-host in needs, so the failure stops the whole release, not just the host upload.
The dist deploy:boxel-host build-only writes is 334 files. Both preview deploy jobs log every uploaded key at --verbose; that listing is 321 under assets/ plus these 13:
index.html robots.txt
auth-service-worker.js test-realm-sw.js
boxel-favicon.png boxel-webclip.png
default-realm-icon.png boxel-ui-checksum.txt
@embroider/virtual/vendor.js @embroider/virtual/vendor.css
@embroider/virtual/app.css test-modules/good.js
test-modules/bad.js
Materialised that file set and ran the suite against it with HOST_DIST_DIR pointed at it: AssertionError: the build writes testem.js, with tests/index.html behind it. Both belong to the dev dist that ember test --path ./dist serves, which is the other candidate dir falls back to — so a local run with HOST_DIST_DIR unset passes and hides this.
The same listing makes the comment above the array wrong in the other direction: @embroider/virtual/vendor.css and test-modules/bad.js are uploaded by the stable-named pass and are not in the list, so "every stable-named one it has" does not hold.
Drop the two absent entries and add the two missing ones. Regression, blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in e9bc84c: dropped testem.js and tests/index.html, added @embroider/virtual/vendor.css and test-modules/bad.js.
Confirmed the same 13 stable-named paths independently, from the host-dist artifact a main CI run uploads (gh run download … --name host-dist) rather than the preview logs — that build is 312 files, 299 under assets/, and the stable-named set matches yours exactly. Ran the corrected suite against that artifact with HOST_DIST_DIR pointed at it: 9/9, including the new partition assertion over the real list.
Worth recording why the list was wrong, because the trap is reusable: the deploy uploads with allowOverwrite and never prunes, so both paths answer 200 on staging and production as leftovers of builds that did emit them. Checking the deployed URLs agreed with the local packages/host/dist, which is a dev build and does include the test tree — two sources, both wrong in the same direction. The comment above the array now says the buckets are cumulative and that a build's own output is the only authority.
| let built = walk(dir).map((file) => relative(dir, file)); | ||
|
|
||
| for (let expected of DIST_FILES) { | ||
| assert.ok( | ||
| built.includes(expected) || | ||
| // Content-addressed names change every build, so match the shape. | ||
| (expected.startsWith('assets/') && | ||
| built.some((file) => file.startsWith('assets/'))), | ||
| `the build writes ${expected}`, | ||
| ); | ||
| } | ||
|
|
||
| let stableNamedUnderAssets = built.filter( | ||
| (file) => | ||
| file.startsWith('assets/') && !/-[A-Za-z0-9_-]{8,}\.[^.]+$/.test(file), | ||
| ); | ||
| assert.deepEqual( | ||
| stableNamedUnderAssets, | ||
| [], | ||
| 'everything under assets/ is content-addressed', | ||
| ); |
There was a problem hiding this comment.
[Claude Code 🤖] built is the real file list, but it is only checked for containment against DIST_FILES — never run through the two passes. That is the check that would have caught the stale entries above, and it is the only one that can see a file the patterns miss at all.
The partition test asserts over DIST_FILES, so it confirms a hand-maintained list against itself. A path matched by neither pattern cannot appear in it: ember-cli-deploy-build globs distFiles with dot: true, while ember-cli-deploy-s3 filters with dot: dotFolders (defaulting to false) on both assets/** and the fileIgnorePattern, so any dot-prefixed path is dropped by both passes. Checked against the plugin's own minimatch: assets/.hidden.js, .well-known/x and dir/.dotfile land in neither. Nothing in the current dist is dot-prefixed and this predates the split, but it is exactly the failure the partition comment names ("silently missing from the deploy"), and the same unqualified claim sits above IMMUTABLE_UPLOAD in config/deploy.js.
Running built through the same two filters here and asserting it partitions closes both halves, and is indifferent to which build flavour the dist came from — which matters because the two candidate dists have different file sets.
Drift half is this PR's, dot half is pre-existing. Non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Done in e9bc84c. uploadsFor takes a file list, and the dist test runs the real built through both passes: it asserts the upload set equals built exactly, and that a file is immutable if and only if it is under assets/. Renamed to a real dist partitions across the two passes, since that is what it is for.
On the dot half — agreed, and it is the reason the partition assertion could otherwise be quietly conditional. The test asserts the dist contains no dot-prefixed path, with the reason stated inline, so the day a build emits one it fails there rather than shipping a file no pass uploads. Verified it discriminates by dropping .well-known/probe.txt into a copy of the real dist: AssertionError: no dot-prefixed paths in the dist naming the path.
I left dotFolders alone. Turning it on would change what gets uploaded, which is a deploy behaviour change this does not need, and the exclusion is applied identically to both passes so the split did not introduce it. The claim in config/deploy.js is now qualified to say the patterns are complements over what the upload considers, and that dotFolders off means a dot-prefixed path reaches neither.
| interface Upload { | ||
| filePaths: string[]; | ||
| cacheControl: string; | ||
| expires?: Date; | ||
| prefix: string; | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] Nothing typechecks this file. packages/host/tsconfig.json includes app/**/*, tests/**/*, types/**/* and the sibling realm dirs, not support/** — tsc -p tsconfig.json --listFilesOnly emits nothing under packages/host/support/. node --test strips the annotations without checking them, and lint:js does not typecheck, so Upload drifting from the plugin's actual option shape would go unnoticed.
Add support/**/* to include, or drop the annotations so the file doesn't read as covered. Non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Added support/**/* to the host tsconfig.json include in e9bc84c — and doing so immediately earned its keep, because it surfaced two real errors:
support/…(14,31): error TS1470: The 'import.meta' meta-property is not allowed in files which will build into CommonJS output.
packages/host/package.json has no "type", so under module: nodenext a .ts file there is CommonJS to the type-checker and import.meta is illegal. It ran anyway only because Node's ESM detection sees the import statements and treats the file as a module — the two disagreed, and nothing was in a position to say so.
Renamed to .mts, which both agree is ESM. ember-tsc --noEmit now reports zero errors under support/ (the rest of the run is the fresh-worktree @cardstack/boxel-icons TS2307s, which CI does not hit since it builds icons first).
That rename would have traded one blind spot for another: .mts matched no files glob in .eslintrc.js, so linting it directly gave Parsing error: The keyword 'import' is reserved and directory traversal skipped it entirely. Added mts to the **/*.{js,ts} override and a small node-env override for support/**/*.mts; it lints clean.
| // A config that aliases the plugin runs one instance per alias, each reading | ||
| // the ENV key of the same name; an unaliased one runs a single `s3` instance. | ||
| // Both shapes are driven here so the assertions below are what decides. | ||
| let aliases: string[] = config.pipeline.alias?.s3?.as ?? ['s3']; |
There was a problem hiding this comment.
[Claude Code 🤖] "Both shapes are driven here" holds for no target: deployConfig sets pipeline.alias.s3.as unconditionally, so config.pipeline.alias?.s3?.as ?? ['s3'] always takes the aliased branch and ['s3'] is unreachable. Drop the fallback and the sentence, or drive an explicitly unaliased config so the claim is true. Non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Right, it was dead. Dropped both the ?? ['s3'] fallback and the sentence in e9bc84c; uploadsFor reads config.pipeline.alias.s3.as directly.
The fallback existed to let the suite run against an unaliased config, which is how the positive control was taken — reverting config/deploy.js and watching the four defect-describing tests fail. That is a one-off check, not something the committed harness should carry a dead branch for, and a comment asserting coverage the code cannot reach is worse than not having it.
The expected-dist list named testem.js and tests/index.html, which the deploy build does not emit -- the deployed buckets are never pruned, so both answer 200 there as leftovers of builds that did. The dist gate in the build workflow asserts every listed path is present, and that workflow is reachable only from a deploy, with release listing it in needs. It also missed @embroider/virtual/vendor.css and test-modules/bad.js, which the stable-named pass does upload. The list now matches a build's own output, and the dist check earns its place: it runs the real file list through the two passes and asserts it partitions, rather than only checking the list against itself. That is the only assertion that can see a path neither pattern matches. Dot-prefixed paths are exactly that path. ember-cli-deploy-build lists distFiles with dot globbing on, while ember-cli-deploy-s3 filters both passes with dotFolders off, so such a file would be absent from the deploy without failing it. No build emits one; asserting that keeps the partition claim unconditional instead of quietly scoped. Nothing checked the suite itself: support/ sat outside the tsconfig include, and a .ts file under this package's CommonJS default is a CommonJS file to the type-checker, so import.meta was an error there while Node's ESM detection accepted it. The .mts extension makes both agree, and the eslint override covers the extension so directory traversal still reaches it. Dropped the unaliased-config fallback in the harness. The config aliases unconditionally, so the branch was unreachable and the comment claiming both shapes were exercised was false. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What this is about
The host app is a single-page app. When you open it, the browser fetches one small HTML document — the "app shell",
index.html, about 8 KB — and that document is the only thing that names the JavaScript bundle for the build currently deployed (<script src="/assets/main-DMcx_nWD.js">). Everything else the app loads is named by that shell, directly or indirectly.The shell's filename never changes. The bundle's filename does: the build writes every file under
assets/with a hash of its own contents in the name, so a new build producesmain-<newhash>.jsalongside a shell that points at it. That split is the whole reason the scheme works, and it's why the two halves need opposite caching rules.The problem
Every file the host deploy uploaded was stored in S3 with the same response header:
immutableis a promise to the browser that this URL's content will never change, so it should never bother asking again — not on a reload, not in two years. For a hashed filename underassets/that promise is trivially true and the caching is exactly what you want. For the shell it is false, and the consequence is that a browser keeps running whichever build it first loaded.This was observable directly. A tab opened one day was still running
main-CFoh7RJ4.jsthe next, while the origin was servingmain-DMcx_nWD.js, withperformance.getEntriesByType('navigation')[0].transferSize === 0anddeliveryType === "cache". Appending any unused query parameter produced a cache miss and the current bundle immediately — the browser's HTTP cache keys on the full URL, so a URL it hasn't seen is the only way to get a fresh shell. A normal reload does not help; only a hard reload, a different URL, or clearing site data.Practically, that means a fix shipped to the host is invisible to anyone whose browser is holding a shell entry, with no upper bound short of eviction, and a host build from an arbitrary earlier day goes on talking to today's realm-server. Both deployed environments carried the header, and deep links carry it too — they resolve through the distribution's
403 -> /index.htmlcustom error response, which serves the shell object's headers.It isn't only the shell
Three more files have the same shape — content that changes between builds, under a URL that doesn't:
/@embroider/virtual/vendor.jssetswindow.EmberENV(the Ember feature flags and environment config)./@embroider/virtual/app.cssis real application CSS — the z-index scale, error and link styles, the Monaco tooltip styling./@embroider/virtual/vendor.css.Those are referenced by the shell but are not under
assets/, so they were immutable too. A browser could get a fresh shell and still pin those. The service workers (auth-service-worker.js,test-realm-sw.js) and the small static files (robots.txt, the favicons) are the same category.Where it came from
packages/host/config/deploy.jssetsfilePattern: '**/*'on theember-cli-deploy-s3plugin and leavescacheControlalone. The plugin's defaultcacheControlismax-age=63072000, public, immutable— which is a sane default only because the plugin's defaultfilePatterncovers hashed asset extensions and deliberately excludes.html. Widening the pattern to everything without also narrowing the directive is what applied an asset rule to the whole dist.The fix
The rule this makes explicit:
immutableis only ever safe for a content-addressed filename, and the build writes exactly those underassets/.ember-cli-deploylets you run one plugin as several named instances (pipeline.alias), so the dist is now uploaded in two passes:Cache-Controls3-content-addressedassets/**max-age=63072000, public, immutables3-stable-namedno-cache, max-age=0, must-revalidateThe two patterns are complements, so every file the upload considers is uploaded by exactly one pass — a file matched by neither would be silently missing from the deploy, and a file matched by both would be uploaded twice with a race over which directive survived. There is a test for precisely that, run over a real dist.
no-cachedoes not mean "don't cache"; it means "cache it, but ask before reusing it". The browser stores the shell and sendsIf-None-Matchwith the stored ETag on the next visit, and the server answers304 Not Modifiedwith no body when the build hasn't moved. So the steady-state cost is one conditional request for an 8 KB document — and in exchange, a deploy reaches open and returning clients.Expiresis also set in the past, which is ignored anywhereCache-Controlis understood and matters only to an HTTP/1.0-era cache.The
assets/**directive keeps its existing two-year value, so nothing about how the hashed bundles are cached changes.What CloudFront does with it
Both distributions serve S3 through the managed
CachingOptimizedForUncompressedObjectscache policy and a response-headers policy that only sets CORS headers — it defines no custom headers, so it does not rewriteCache-Control. The origin's directive is what reaches the viewer, which is why the wrong value was visible end-to-end in the first place and why fixing it at the origin is sufficient; no distribution change is needed. That cache policy floors CloudFront's own TTL at one second, so the edge revalidates against S3 essentially immediately, and the deploy already issues a/*invalidation on top of that.Previews
The PR preview targets already forced everything to revalidate, for the same reason stated slightly differently: a preview is rebuilt in place under one prefix, so nothing it serves is content-addressed in a way that survives the next push to the branch. They keep that behaviour — both passes revalidate and both take the branch prefix. Both preview deploy jobs on this branch are green, so the aliased config has run end-to-end against real S3.
Limits worth knowing
It does not unpin browsers already pinned. A browser holding a shell entry stored under the old
immutabledirective never contacts the origin for that URL, so neither the new S3 metadata nor a CloudFront invalidation reaches it. Those clients stay on their build until the entry is evicted or someone hard-reloads. The failure is silent rather than broken — the deploy never prunes, so the superseded bundle a pinned client asks for still answers 200. A recovery mechanism for that population is separate work, tracked on its own; fixing the header first is the prerequisite, since otherwise every newly-pinned browser is a fresh instance of the problem.Only objects the current build uploads get re-stamped. The buckets are never pruned, so an object left behind by an older build keeps whatever
Cache-Controlit was written with. The shell is emitted by every build, so it is corrected on the next deploy.Dot-prefixed paths reach neither pass.
ember-cli-deploy-buildlistsdistFileswith dot globbing on, whileember-cli-deploy-s3filters withdotFoldersoff — on both theassets/**pattern and the ignore pattern — so such a file would be absent from the deploy without failing it. This predates the split and applies to both passes identically. No build emits one, and the dist test asserts that, so the day one appears it fails there rather than shipping a gap.Tests
packages/host/support/deploy-cache-control.test.mts, run withnode --test(the same pattern aspackages/matrix's shard-assignment test). It is.mtsrather than.tsbecause this package has no"type"field, so undermodule: nodenexta.tsfile there is CommonJS to the type-checker andimport.metais an error in it — while Node's ESM detection accepts it. The.mtsextension makes both agree.Rather than re-implement the plugin's glob matching and assert against the re-implementation, the suite instantiates the real
ember-cli-deploy-s3plugin from the realconfig/deploy.js, replaces itsuploadClientwith a stub, and runsconfigure+upload. What it reads back is the plugin's own file matching and the exactCache-Controleach file would have been uploaded with.For each of the
stagingandproductiontargets it checks that the two passes partition the dist, that a file is immutable if and only if it is underassets/, and that the shell revalidates. For both preview targets it checks that nothing is immutable and everything is prefixed.The last test is the one that catches what the others can't. It walks a real dist and runs that file list through both passes — so it can see a path neither pattern matches, and it notices the hand-maintained list drifting from what the build emits. It also asserts nothing under
assets/has an unhashed name and that no dot-prefixed path is present. It skips loudly when no dist is on disk, but fails rather than skips when a dist is explicitly named, so the workflow step below can't quietly become a no-op that still reports green.That drift check matters because the obvious way to compile the expected file list is wrong: the buckets are never pruned, so paths from builds that no longer emit them still answer 200 on both deployed environments, and the local
packages/host/distis a dev build that includes the test tree. A build's own output is the only authority, which is what the workflow step below supplies.Verified as a positive control by running the suite against the pre-change config: the four tests that describe the defect fail, reporting
index.html (uploaded by s3 as max-age=63072000, public, immutable), while the partition and preview tests correctly still pass. The dist assertions were checked the same way — an unhashed name underassets/and an injected.well-known/probe.txteach fail with the offending path named.CI
Two places, because they check different things:
tmp/deploy-dist— the artifact the deploy actually uploads, and the only place theassets/claim can be checked against real build output rather than a fixture.support/**/*is added to the hosttsconfig.jsoninclude andmtsto the eslint overrides, solint:typesandlint:jsboth cover the suite; neither did before.Where the changes live
packages/host/config/deploy.js— the two-pass split.packages/host/support/deploy-cache-control.test.mts— the suite.packages/host/package.json,tsconfig.json,.eslintrc.js— the script, and putting the suite under both linters..github/workflows/ci.yaml,.github/workflows/build-host.yml— the two gates.🤖 Generated with Claude Code