Serve Go vanity import paths from moq.dev - #126
Conversation
![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: 6371e38656
ℹ️ 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".
| const module = vanity(url); | ||
| if (module) return module; |
There was a problem hiding this comment.
In the live environment, wrangler.jsonc routes both moq.dev and de.moq.dev to this Worker, but vanity handling runs before the existing DEMOQED hostname rewrite and does not inspect the hostname. Consequently, a request such as https://de.moq.dev/moq is redirected to pkg.go.dev, while https://de.moq.dev/moq?go-get=1 returns the moq.dev/moq metadata instead of being rewritten to /de/moq; restrict this call to the primary/staging vanity hosts or perform the de rewrite first.
Useful? React with 👍 / 👎.
`go get` resolves an import path by fetching it over HTTPS with ?go-get=1 and reading the repo out of a <meta name="go-import"> tag, so serving that tag here makes moq.dev own the published import path instead of whichever mirror repo the code lands in. The Go wrapper can then be imported as `moq.dev/moq` rather than `github.com/moq-dev/moq-go/moq`, which also means the identifier is `moq` with no alias, and a future mirror rename costs a line in the table rather than a breaking change for every consumer. Two paths are served, both inert until the matching go.mod switches its module path: moq.dev/moq -> github.com/moq-dev/moq-go moq.dev/moq-ffi -> github.com/moq-dev/moq-go-ffi A module owns every path beneath it, because the go command asks about the full import path (package directory included) and the tag has to answer with the module path regardless. Requests without ?go-get=1 are people, so they redirect to pkg.go.dev. `run_worker_first` needs the /moq* wildcard for the same reason #125 needed /api/*: an asset miss is answered with the 404 page rather than falling through, so the Worker would never run. Verified against `wrangler dev`, which reproduces that routing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
6371e38 to
799a20b
Compare
|
Warning Review limit reached
Next review available in: 53 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe Worker now handles Go vanity import paths for 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review findings from Codex. The vanity lookup ran ahead of the de.moq.dev -> /de rewrite and never looked at the hostname, so DEMOQED lost every URL beginning with /moq: de.moq.dev/moq redirected to pkg.go.dev instead of resolving against /de. Doing the rewrite first fixes it, since that branch returns, and it means the import paths only ever answer for the host they're published under. The tests only reached vanity() directly, which is the half that can't break the site. worker/index.test.ts drives the Worker's fetch handler against a stub ASSETS binding instead, so the ordering above is pinned by a test that fails without it. That still doesn't cover `run_worker_first` -- only Cloudflare routes that -- so index.ts says out loud that the Worker doesn't see a request unless wrangler.jsonc lists it. None of this ran in CI: the workflow calls `bun run check` (biome + tsc), which never runs `bun test`, so these assertions and the existing broadcast tests were all decorative. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Ran an adversarial review pass with Codex, plus the inline finding already on this PR. Two were real and are fixed in e759b43; one is rejected. Fixedde.moq.dev lost every The tests missed the layer that can actually break the site. They only called And none of it ran in CI. The workflow called Rejected"NO SHIP: the target repos don't declare these module paths." True, deliberate, and already the "Not in this PR" section above: the mirrors still declare The one useful detail in that finding is real and already noted: with today's layout the usable path would be "Staging can't supply end-to-end evidence." By design. CI is green. (written by Opus 5) |
1 participant
go getresolves an import path by fetching it over HTTPS with?go-get=1andreading the repo to clone out of a
<meta name="go-import">tag. Serving thattag here makes moq.dev own the published import path instead of whichever mirror
repo the code happens to land in, so the Go wrapper can be imported as
moq.dev/moqrather thangithub.com/moq-dev/moq-go/moq. That also makes theidentifier
moqwith no alias, since the last path element finally matches thepackage name, and it means a future mirror rename costs a line in a table rather
than a breaking change for every consumer.
Two modules are served:
moq.dev/moqmoq.dev/moq-ffiA module owns every path beneath it, because the go command asks about the full
import path (package directory included) while the tag has to answer with the
module path regardless. Requests without
?go-get=1are people, so they redirectto pkg.go.dev. The page also carries
go-source, which is what pkg.go.devfollows to link a symbol back to the line that declares it.
run_worker_first
/moq*is load-bearing for the same reason #125 needed/api/*: an asset missis answered with the 404 page rather than falling through, so the Worker never
runs. Confirmed against
wrangler dev, which reproduces that routing:/moq?go-get=1returned the site's 404 page until the wildcard was added. It's awildcard rather than four exact entries so that adding a module to the table
can't silently 404 by forgetting this file.
Testing
worker/vanity.test.tscovers the tag, a package directory resolving to itsmodule,
/moqand/moq-ffinot bleeding into each other, and the humanredirect. Against
wrangler dev: both go-get paths return the right prefix,/moqand/moq/sub/pkg302 to pkg.go.dev, and/,/blog/,/rss.xml, andan unrelated
/moqadilloall still serve normally.tsc --noEmitandbun testare clean.
Not in this PR
The tags are inert until each
go.modswitches its module path, which is abreaking change and lives in the moq repo.
moq.dev/moqonly avoids an alias ifthe package moves to its module root (today it's in a
moq/subdirectory), andthe release scripts, both workflows, and
doc/lib/go/all name the old path.Old
github.com/moq-dev/moq-goversions keep resolving from the same tags afterthe switch, so a
retractor a README note is worth adding then.(written by Opus 5)