Keep configured API paths out of PWA runtime caches - #2416
Conversation
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. |
|
Independent review at
This PR is in its one bounded fix round. The repair will version the policy acknowledgement so #2350 cannot satisfy it, invalidate the affected static runtime cache fail closed, and extend deterministic plus installed-worker A-to-B proof. The head is not mergeable until the fix, current-base reproof, fresh exact-head review and CI complete. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fad3a06aa
ℹ️ 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".
|
Current-base reconciliation pushed at The new main delta ( Current-head proof:
The real installed-worker-to-new-worker browser transition remains NOT verified because the isolated Playwright run stalled before spawning a browser. This PR remains unmergeable pending that proof. |
|
Final review-fix head: The late Codex finding was confirmed: the legacy worker replies with Focused proof:
This does not close the existing verification blocker. The real old-worker-to-v2-controller browser execution remains NOT verified. |
|
Independent exact-head review receipt Head: No realistic CRITICAL/HIGH findings. The review confirmed the v2 marker in source and generated worker, full deletion of Residual review note: every worker activation flushes legitimate static runtime entries until fetched again. This is the intentional fail-closed security tradeoff for this repair; the app shell remains precached. Hosted CI at this checkpoint: 28 of 34 checks completed successfully or intentionally skipped, 6 still running, 0 failed. The real installed old-worker-to-v2-controller browser transition is still NOT verified, so this PR is parked and must not merge. |
|
Parked-state amendment for exact head The failure is the tracked source-launcher substrate defect, not a PWA regression. #2159 and #2161 plus the open |
|
Coordination note on the parked state. The conflict here is smaller than The PWA work does not conflictProbed against current
Everything else auto-merges, including So the restart is a bookkeeping reconciliation on two shared append-only files, not a re-derivation of the reviewed What actually still blocks itThe Ordering that follows from the above: Nothing here re-opens the reviewed acceptance or asks for new work. |
…se-cache-boundary # Conflicts: # docs/agentic/FAILURE_LEDGER.md # docs/agentic/failure_ledger.jsonl
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b980111b0
ℹ️ 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".
…se-cache-boundary
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
[Claude lane claim v2 update] The runner blocker is gone; the browser proof now runsThe failure-ledger record
Bypassing the default Browser proof: the worker transition is demonstratedA strict probe (controller must be non-null and answer v2, so it cannot pass
The same proof DISPROVES summary claim 5 — do not mergeClaim 5 ("Invalidate Reproduced on three consecutive runs. Instrumenting a snapshot at the exact One caution for whoever picks this up: an emptied This is the same HIGH the 2026-09-03T04:46Z review raised — stale configured-base StatusPushed the current-base merge commit only. No ledger record was closed and no status: PARKED — worker transition demonstrated, but the static-cache requalification run this session: |
…ivate The generated worker loads api-cache-cleanup.js with importScripts() from inside vite-plugin-pwa's asynchronous AMD define() factory, so the activate listener is attached after the event has already been dispatched and never fires. Measured in Chromium: a seeded taskdeck-static-assets entry survived the whole old-worker-to-v2 migration while the same file's message listener answered the policy handshake. Run the sweep at evaluation time, deduplicated with the activate hook, and record completion in a taskdeck-pwa-cache-policy-v2 marker cache so it stays a one-time migration rather than a purge on every worker restart.
…vate event The src/tests/pwa mocks dispatched activate themselves, so they passed while the shipped worker never ran the sweep at all. Evaluate the emitted dist/api-cache-cleanup.js against a fake CacheStorage keyed on the cache names parsed out of the generated dist/sw.js, dispatching no activate event: reverting the evaluation-time sweep turns this red. Update the mock-based specs to the surfaced original error and the marker cache.
… regression playwright.pwa-proof.config.ts has no webServer: the operator starts the backend and a vite preview of a production build, which is what the test/playwright-pwa-preview-bootstrap ledger record asked for. The strict spec gates on TASKDECK_E2E_PWA_PREVIEW like the existing preview regression and pins the two assertions pwa-api-cache.spec.ts left loose: a non-null v2 controller, an emptied taskdeck-static-assets, and a preserved share-target queue.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
[Claude lane claim v2 update] root cause The confirmed HIGH was real, and the previous PR body's claim was false. Instrumenting the shipped worker in Chromium settled it: I wrote a breadcrumb into a dedicated cache at script-evaluation time and at each step inside the fix
browser proof (strict probe, 3/3 consecutive, webServer-free config against a real backend on :5000 and
requalification (all at the pushed head unless noted)
NOT verified: the full backend suite and the broad frontend suite (untouched by this change); real-device/other-browser behaviour — all evidence is Chromium via Playwright. adopted into the repo: docs: status: fresh repair cycle round 1 complete; ready for exact-head CI, one fresh-context review, aging window, and coordinator merge |
The evaluation-time sweep runs during install, while the old vulnerable worker is
still the controller and can still store an identity-bound response in
taskdeck-static-assets. Activation previously awaited the memoised sweep, which
had already resolved and whose marker short-circuited every later pass, so an
entry cached in that install-to-activation window survived the migration.
Activation now calls retireCaches({ force: true }), ignoring both the memo and the
marker, and still fails activation on error. The evaluation-time call keeps its
one-time marker semantics for builds where activate never reaches the listener.
Covered by two new cases that dispatch activate after a completed evaluation-time
sweep; reverting the forced sweep turns both red.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
[Claude lane claim v2 update — review round 1 dispositions] HIGH — install-time sweep + memo + marker lets a poisoned entry survive: CONFIRMED and FIXED. The finding was correct. I reproduced it in a real Chromium before touching the code: seeding HIGH — proof: the e2e half is NOT delivered; the unit half is. Being straight about this rather than shipping something that looks like proof. I made three genuinely different attempts at the browser-level race case and could not place the seed deterministically inside the install-to-activation window from page JavaScript:
So I removed the e2e race case rather than commit a test whose red/green does not track the defect. The HIGH is instead pinned deterministically in MEDIUM — marker name uncoupled from the policy constant: FIXED (~10 lines). New test LOW — runner scope: FIXED. LOW — docs sentence: FIXED. requalification at this head
Two Operator note worth having: NOT verified: full backend suite and broad frontend suite (untouched); non-Chromium browsers; a browser-level regression for the install-to-activation window specifically. status: review round 1 addressed; ready for exact-head CI, the aging window, and coordinator merge. Not merged, CI not polled. |
|
Fresh repair-cycle review receipt (round 1 at a1d2d53, fix round verified at 1c5e9b3). Verdict after the fix round: no CRITICAL or HIGH finding. Merge-blocking: none. Round count in this repair cycle: one review round, one fix round. Earlier cycles: the original two rounds under the Codex lane, then the park. Repair-cycle findings and disposition:
Fix-diff verification (by trace against the emitted Tracked, not fixed here: a browser-level Playwright regression for the install-to-activation window could not be made reliable in three attempts and is tracked in #2475. LOW, declined: a stale return-type annotation and an The original ledger record for the runner blocker was resolved in this PR: the "idle Playwright" symptom was output buffering from a Remaining gate: exact-head hosted ci-required green and the three-minute aging floor, then a merge commit. Closes #2411 on merge. |
1 participant
Closes #2411
Summary
VITE_API_BASE_URLat build time and exclude both the default/apipath and the configured API base from locale and static runtime-cache matchers./assets/apibuild.registration.update().taskdeck-static-assetsduring the security-policy activation, preserve the share-target queue, and reject activation when CacheStorage cleanup fails.This keeps identity-bound API responses out of the shared runtime cache while preserving build-owned locale, image, icon and font caching after a safe worker activates.
Review fix
The first independent review found two HIGH blockers at
99a0acc6554f7a4b2460ac97f9be7179c7800684: the #2350 policy marker was still accepted, and configured-base entries could survive activation. The bounded fix round replaces that marker, clears the whole affected runtime cache, fails activation on cleanup error, and adds old-worker, worker-restart and stale-entry regressions.Verification
npm run test:pwa-generated-worker: 3/3 passed after a nested/assets/apibuild.npm test -- --maxWorkers=2: 364 files passed; 5,411 tests passed; 3 skipped.node --test --test-concurrency=1 --test-timeout=30000 scripts/ci/dev-up.test.mjs: 85/85 passed on Windows, including the case that timed out in the prior hosted run.npm run typecheck: passed.npm run lint: passed with 0 errors and 8 existing warnings.git diff --checkpassed.Not verified
docs/agentic/failure_ledger.jsonl; this PR remains unmergeable until that transition is demonstrated.Docs
docs/platform/PWA_OFFLINE_BEHAVIOR.mdafter PR Map the PWA cache boundary and fix a false-green in the seam map #2409 released its lease.autodoc/AGENT_INDEX.mdbecause the shared build-time factories replace its stale inline-copy warning.