fix(modules): gate the module pin conflict on the cohort, not the flag - #3409
fix(modules): gate the module pin conflict on the cohort, not the flag#3409kwakayama wants to merge 3 commits into
Conversation
An armed VERYFRONT_DEPENDENCY_PINNING flag conflicted every client module of every project while the rollout sat at 0%. The guard treated "no pin key" as a conflict whenever the flag was set, but an out-of-cohort document correctly emits no key, so the two disagreed and the browser got 409 Unknown dependency snapshot for every module. Consulting the cohort restores the property the rollout percent is supposed to have: the flag stays inert until ramped.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesDependency-pinning rollout
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/server/module-server.test.ts`:
- Around line 2880-2898: Update the test cleanup in the finally block for the
dependency-pinning test to restore the prior state of both
DEPENDENCY_PINNING_ENV_FLAG and DEPENDENCY_PINNING_ROLLOUT_PERCENT_ENV. If the
suite guarantees the flag was initially unset, clear DEPENDENCY_PINNING_ENV_FLAG
alongside the existing rollout cleanup.
In `@src/modules/server/module-server.ts`:
- Around line 418-424: Update the cohort check in serveModule’s
dependency-pinning condition to pass effectiveProjectId to
isProjectInDependencyPinningCohort instead of options.projectId, preserving the
existing flag and key checks. Add coverage for a request where projectId and
projectUUID differ, verifying cohort gating uses the effective identity and
returns the correct 409 behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f2a467cb-933e-4247-b9ae-437851af167f
📒 Files selected for processing (2)
src/modules/server/module-server.test.tssrc/modules/server/module-server.ts
The suite already clears DEPENDENCY_PINNING_ENV_FLAG globally, but not the rollout percent that decides the cohort. Cleaning only one leaves a later test on a cohort it never chose, so both now reset together.
…rce uses createDependencyPinningSource is handed effectiveProjectId (projectUUID ?? projectId), so bucketing the guard on the raw projectId let the check and the snapshot it guards classify a multi-project request into different cohorts -- the same disagreement this PR set out to remove. Adds coverage for the allowlist arm of the cohort. That test does not prove which identity is read: where the two disagree, other guards answer 409 with the identical body, so the response cannot distinguish them.
|
Superseded by #3417, which consolidates this with the other queued render-path fixes plus the v0.1.1206 bump so one queue pass lands the set. Dequeued from the merge queue to avoid this merging underneath the consolidated branch and leaving it empty or conflicting. Please do not re-queue — merge #3417 instead. Closing once that lands. |
1 participant
Every client module of every hosted project answered
409 Unknown dependency snapshotin production. No page hydrated.The defect
src/modules/server/module-server.ts:417conflicted a module request whenever the pinning flag was set and the request carried no pin key:That is the flag, not the cohort. Production runs
VERYFRONT_DEPENDENCY_PINNING=1withROLLOUT_PERCENT=0and no project allowlist, so no project is in the cohort and every document correctly emits no pin key. The document and the module handler therefore disagreed: one emitted no key, the other refused every request that lacked one.dependency-pinning-cohort.tsstates the property this broke:The armed flag was not inert. At 0% it conflicted 100% of client modules.
Reproduction
Locally, against the real production release, with the environment read off the live deployment rather than guessed:
PINNING=1,ROLLOUT=0(production's config)Unknown dependency snapshotPINNING=0Production returns the same 409 for
/_vf_modules/pages/index.js, so the reproduction is faithful.Fix
Consult the cohort, which is what decides who is actually pinning. A project genuinely in the cohort that omits its key still conflicts — that path is unchanged and its existing tests still pass.
Proof
Regression test written first, red before the change with the exact production symptom:
Green after. End to end with the rebuilt binary, production env, real release:
Zero 409s remain on any module path. Two project-module URLs return 503 in my sandbox from a separate release-asset-manifest branch that also fires with pinning disabled, so it is not this defect and not this fix.
Suite: 268 passed / 3344 steps across
modules,security,handlers, andtransforms/esm.deno fmtclean.Not fixed here
The same pages also violate CSP: the document references
esm.sh,images.veryfront.com, andfonts.googleapis.comwhile the default policy is'self'-only. I confirmed this is independent of this fix — with modules serving correctly, the document still carries 6esm.shreferences and 291 image references.I did try adding the ESM CDN to the default
script-srcand reverted it: an existing test, "default CSP admits no remote hosts or broad network schemes", fails withscript-src must not hardcode a remote host. That strictness is deliberate, so widening it is a design decision rather than a bug fix, and it belongs in its own change with an owner who can make that call.Summary by CodeRabbit