Repository navigation
fix: serve the Socket badges from badge.socket.dev - #729
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: allxsmith/bestax/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughSix README files now use Socket’s latest package badge endpoint. The existing Socket package overview links remain unchanged. ChangesSocket badge endpoint updates
Priority: ➖ Normal Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The README badges now use Socket's latest package endpoints while preserving their overview links. This documentation-only change introduces no actionable merge-blocking risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
End-to-end verification on this branchRendered each README at this branch's head through the GitHub API, pulled the camo URL GitHub generated for the Socket badge, and fetched it. All six now serve an actual image:
For contrast, the same probe against |
Preview DeploymentPreview URL: https://e84b700a.bestax.pages.dev |
The old `socket.dev/api/badge/...` path sits behind a Cloudflare bot challenge, so GitHub's camo proxy gets a 403 interstitial back instead of an SVG and the badge renders broken. `badge.socket.dev` is the host Socket serves badges from, and camo is allowlisted for it. Keep the `/latest` segment: it is the spelling every real-world usage carries, and the one verified to return 200 image/svg+xml through camo for @allxsmith/bestax-bulma. Patch release so the README npm serves picks the fix up.
The old `socket.dev/api/badge/...` path sits behind a Cloudflare bot challenge, so GitHub's camo proxy gets a 403 interstitial back instead of an SVG and the badge renders broken. `badge.socket.dev` is the host Socket serves badges from, and camo is allowlisted for it. Keep the `/latest` segment: it is the spelling every real-world usage carries, and the one verified to return 200 image/svg+xml through camo for create-bestax. Patch release so the README npm serves picks the fix up.
The old `socket.dev/api/badge/...` path sits behind a Cloudflare bot challenge, so GitHub's camo proxy gets a 403 interstitial back instead of an SVG and the badge renders broken. `badge.socket.dev` is the host Socket serves badges from, and camo is allowlisted for it. Keep the `/latest` segment: it is the spelling every real-world usage carries, and the one verified to return 200 image/svg+xml through camo for bestax-migrate. Patch release so the README npm serves picks the fix up.
The old `socket.dev/api/badge/...` path sits behind a Cloudflare bot challenge, so GitHub's camo proxy gets a 403 interstitial back instead of an SVG and the badge renders broken. `badge.socket.dev` is the host Socket serves badges from, and camo is allowlisted for it. Keep the `/latest` segment: it is the spelling every real-world usage carries, and the one verified to return 200 image/svg+xml through camo for bestax-mcp. Patch release so the README npm serves picks the fix up.
The old `socket.dev/api/badge/...` path sits behind a Cloudflare bot challenge, so GitHub's camo proxy gets a 403 interstitial back instead of an SVG and the badge renders broken. `badge.socket.dev` is the host Socket serves badges from, and camo is allowlisted for it. Keep the `/latest` segment: it is the spelling every real-world usage carries, and the one verified to return 200 image/svg+xml through camo for @allxsmith/eslint-plugin-bestax. Patch release so the README npm serves picks the fix up.
Same host move as the package READMEs. The root README is not published to npm (the root manifest is private), so this carries no release.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review — 0 blocking · 2 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Robustness | Nothing in CI validates README badge URLs — check:urls reads only bulma-ui/llms.txt and bulma-ui/AGENTS.md, so a future Socket host or path change breaks the badge silently again |
scripts/check-pointer-urls.mjs:43 |
| 2 | 🔵 Advisory | Robustness | npm-side READMEs keep the broken badge until each package cuts its next release; acknowledged in the PR body and in #726, recorded here | bulma-ui/README.md:10 |
Overall: The change is sound and narrowly scoped — six image URLs, link targets untouched, no executable path anywhere in the diff. Everything checkable offline holds: the six badge package names match the real manifests (@allxsmith/bestax-bulma, create-bestax, bestax-migrate, bestax-mcp, @allxsmith/eslint-plugin-bestax), a repo-wide grep finds no remaining socket.dev/api/badge reference outside these six lines, prettier --check on the six files is green, and the release claim in the PR body is accurate — all five publishable packages carry a { type: docs, release: false } rule in their release.config.js, so docs: genuinely ships no bump. The riskiest part is the one thing that cannot be settled from inside the repo: whether camo really gets 200 image/svg+xml back from badge.socket.dev/npm/package/<scoped>/latest. Confirming the rendered badge on main after merge is the only real acceptance test here.
Residual risk:
- The 200-through-camo claim is unverified by me. This review environment has no network egress — both
curlandWebFetchagainst the camo URL were blocked — so the central evidence in the PR could not be reproduced. What was confirmed instead: rendering the PR branch through GitHub itself (gh api -H "Accept: application/vnd.github.html" repos/allxsmith/bestax/readme?ref=docs/726-socket-badge-host) emits a camo URL whose hex payload decodes to exactlyhttps://badge.socket.dev/npm/package/@allxsmith/bestax-bulma/latest, so the URL a reader browser will request is the intended one. Whether it answers 200 stays open until someone looks at the rendered README onmain. - Recurrence stays undetected. Refuted as an introduced risk, open as a standing one:
check:urlsruns atci.yml:118, but itsFILESlist is onlybulma-ui/llms.txtandbulma-ui/AGENTS.md, and neither contains a Socket URL — so this diff cannot red that gate, and equally no gate would catch the badge breaking again. Pre-existing, and not worth adding a flaky third-party fetch to a required check, so it is noted rather than requested. - Stragglers. Refuted: a repo-wide grep for
socket.dev, excludingnode_modulesand.git, returns exactly the six changed lines. Nocreate-bestaxtemplate, skill, docs page, or workflow embeds a Socket badge. - Gate coverage for a markdown-only diff. Refuted as a real gap: no test or script consumes README badge content (the README rules in
check-conformancecover the skills roster and install lines, not badges), andcheck:conformancewas re-run here — every rule passes exceptversion-regression, which fails only because this checkout is shallow with no reachable tags. That is an artifact of the review environment, not of the PR. - Scoped-name encoding. Open but low:
@allxsmith/...is passed unencoded in the path, matching the@borfast/arrispwgenprecedent cited in the issue. If the Socket router treated the scope slash differently the badge would 404 — which the same post-merge look atmaincatches.
🏄 Six mellow URL swaps, zero blast radius, and the homework was already done before this one hit the water — clean drop-in, no wipeout in sight. Just peek at the rendered README once it lands on
mainto be sure the wave actually broke.
39f6cc7 to
18aec08
Compare
allxsmith
changed the title
|
Thanks — both advisories read fairly. Answering each, plus one thing the review correctly flagged as unverifiable. #2 (npm-side READMEs stay broken until each package releases) — now fixed, not just acknowledged. That was true of the Because a package's analyzer carries #1 (nothing in CI validates badge URLs) — agreed, and agreed it should stay unrequested. On the residual risk the review could not settle. The review was right to hold the 200-through-camo claim open given it had no network egress. It has since been checked directly, and not by fetching the origin — by rendering each README at this branch's head through the GitHub API, extracting the camo URL GitHub itself generated, and fetching that. All six return That is as close to the post-merge acceptance test as can be run pre-merge, since camo is serving the real branch content. Still worth a glance at the rendered README on |
|
deep-review: fresh The file diff is byte-identical to the version last reviewed, but the commit structure is new and is the part worth the attention: five Please focus on whether the release wiring actually does what the PR body claims:
|
Preview DeploymentPreview URL: https://921b7436.bestax.pages.dev |
There was a problem hiding this comment.
Deep review — 0 blocking · 5 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Correctness | PR body: the squash warning is wrong in both mechanism and consequence — a default squash would use the unscoped PR title, which patch-releases all five packages, not one | PR body, "Commit structure" |
| 2 | 🔵 Advisory | Correctness | PR body: "the root manifest is private" is not what makes the docs: commit release-free — { type: 'docs', release: false } in all five configs is; the private root manifest is why the root README needs no release |
PR body, commit table |
| 3 | 🔵 Advisory | Robustness | All five CHANGELOGs and GitHub releases will carry all five fix(…) lines — release-notes-generator sees the whole commit range, not the scoped subset |
eslint-plugin/release.config.js:70 |
| 4 | 🔵 Advisory | Robustness | #726 states "no release-triggering commit here"; this PR deliberately reverses that, so Closes #726 closes an issue whose text contradicts what shipped |
README.md:11 |
| 5 | 🔵 Advisory | Robustness | The 200 image/svg+xml through camo claim is still not reproducible from this environment — carried forward from the previous review |
bulma-ui/README.md:10 |
Overall: The release wiring does what the PR claims on questions 1–4, and I verified it against the installed @semantic-release/commit-analyzer@13.0.1 source rather than the configs' comments. Every commit touches exactly one file inside the package its scope names, commitlint is green on all six messages, and each publishable package cuts exactly one patch — none twice, none missed. The one thing to look at first is row 1: the "please merge, do not squash" instruction is defensible on changelog grounds but its stated reason is false, and it has no enforcement — the ruleset permits squash and commitlint runs only in the local commit-msg hook.
Question-by-question, with the evidence
1. Does each fix(<scope>) cut exactly one patch? — Yes.
Two facts from @semantic-release/commit-analyzer@13.0.1 decide it, and both hold:
lib/analyze-commit.jsmatches withisMatchWith(commit, rule, (object, src) => isString(src) && isString(object) ? micromatch.isMatch(object, src) : undefined). An unscoped commit parses toscope: null, which is not a string, so the customizer returnsundefinedand lodash falls back to strict equality against the glob string.{ scope: '!(<pkg>)' }therefore cannot match an unscoped commit — exactly what each config's comment asserts.index.jsfalls back toDEFAULT_RELEASE_RULESonly underisUndefined(commitReleaseType). A custom rule returningfalseis notundefined, sorelease: falsegenuinely suppresses rather than leaking into the angular defaults.
So for package P, across the six commits: fix(P) matches { type: 'fix', scope: P, release: 'patch' }; the other four fix(X) match { scope: '!(P)', release: false }; docs: matches { type: 'docs', release: false }. analyzeCommits reduces to one aggregate release type per run, so a package cannot release twice from one push regardless of how many commits say patch.
Ordering is right too, and it matters here because all five fire at once: ci.yml runs bulma-ui first and eslint-plugin last, so every workspace:^ on @allxsmith/bestax-bulma is rewritten at pack time against the new bulma-ui version.
2. Is the root README.md release-free? — Yes, for a stronger reason than the body gives.
package.json:4 is "private": true, there is no root release.config.js, and ci.yml's publish job has a semantic-release step for the five packages and none for the root. But none of that is what makes the commit release-free: { type: 'docs', release: false } matches in all five configs regardless of scope, and that is the operative rule. The private root manifest explains why the root README needs no release (it is never published to npm), not why the commit triggers none. Conclusion correct, reason conflated — row 2.
3. Does any commit touch a file outside its own package? — No.
18aec086 docs: README.md
15d4d9b4 fix(eslint-plugin): eslint-plugin/README.md
2fe0a456 fix(bestax-mcp): bestax-mcp/README.md
1cd6dabe fix(bestax-migrate): bestax-migrate/README.md
d473c07c fix(create-bestax): create-bestax/README.md
fa03c7e4 fix(bulma-ui): bulma-ui/README.md
File attribution is clean. Changelog attribution is not, and that is row 3: release-notes-generator is handed the whole commit range, not the scoped subset — the mechanism eslint-plugin/release.config.js documents and bestax-migrate/CHANGELOG.md demonstrates, where the top ### Bug Fixes block is a run of **bulma-ui:** entries. So each of the five changelogs gets all five fix(…) lines. Pre-existing and accepted; this structure is simply the maximal case of it.
4. Commitlint on all six? — Green.
pnpm exec commitlint --from e38c1ca --to 18aec086 reports found 0 problems, 0 warnings for each. The five fix scopes are all in RELEASE_SCOPES; docs is not in RELEASE_TYPES, so release-scope-required returns [true] before the scope gate is reached. Headers are well inside 100 chars and bodies wrap at ~72.
5. What breaks under a squash? — Not what the body says.
gh api repos/allxsmith/bestax returns squash_merge_commit_title: COMMIT_OR_PR_TITLE. With six commits that resolves to the PR title, not a commit title — so the squash header would be fix: serve the Socket badges from badge.socket.dev (#729), unscoped. Per the mechanism in question 1, an unscoped fix matches no custom rule in any of the five configs, falls through to the angular defaults, and yields patch for all five packages. That is the fall-through commitlint.config.js already documents in its header comment.
So a default squash ships five patches, same as the merge commit. What it costs is the scoped changelog lines, replaced by one unscoped line in all five. Prefer the merge commit for that reason — but the body's "only one scope would survive" is false, and so is the implied risk of a package missing its release.
Worth knowing alongside it: nothing enforces the request. The main ruleset lists allowed_merge_methods: ["merge","squash","rebase"], and commitlint lives only in the husky commit-msg hook — no CI job lints a merge or squash header.
Residual risk:
- A package releasing twice, or not at all. Refuted.
analyzeCommitsreduces the range to a single release type per run, so duplication is not reachable; and thepublishjob's only gate isgithub.event_name == 'push' && github.ref == 'refs/heads/main'withneeds: build-and-test, which both a merge commit and a squash satisfy.@allxsmith/eslint-plugin-bestax@1.0.0and@1.0.1tags exist, so its first-release unbounded-range case (the onerelease.config.jsguards withsuccessCommentCondition: false) is not in play here either. - Stale badge URLs elsewhere. Refuted: a repo-wide
grep -rn socket.devexcludingnode_modules/.gitreturns exactly the six changed lines — no template, skill, docs page, workflow, or generator embeds one. No generator writes a README. - The published MCP index stamped a patch behind. Refuted as harmful:
bestax-mcppublishes at step 4 while theRegenerate the MCP indexrestamp runs last, so this run's tarball carriesgeneratedFrom.versionone bulma-ui patch behind. The server's drift warning fires on minor or major only — patch drift is silent by design. - Markdown-only gates.
prettier --checkon the six files is green here.pnpm check:conformanceI could not execute in this environment; nothing incheck-conformance.mjsmatchesbadge,shields.io, orsocket, so the rules that could bite this diff do not exist. - The camo 200. Open, unchanged from the last review. Rendering the branch through GitHub (
gh api -H "Accept: application/vnd.github.html" .../readme?ref=docs/726-socket-badge-host) yields a camo URL whose hex payload decodes to exactlyhttps://badge.socket.dev/npm/package/@allxsmith/bestax-bulma/latest, and the old camo digest onmainis617b6c75…— the same one #726 quotes, so the issue's evidence is genuine. Fetching either digest was blocked here. Looking at the rendered README onmainafter merge is still the only acceptance test.
🏄 Same six mellow URL swaps, but the commit structure got a proper tune-up — five clean scoped patches, one per board, and every one of them lands. Only thing off is the squash warning in the body: squashing wouldn't wipe you out, it'd just hand all five the same unscoped tag line. Merge it the usual way and paddle out.
|
Good review. Rows 1, 2 and 4 were real errors on my side and are fixed; 3 and 5 are answered. Nothing left open that I can see. Row 1 — the squash warning was wrong. Corrected. I checked the mechanism against the installed The PR body now says that, states the real cost (five identical unscoped changelog lines instead of five scoped ones), and notes that nothing enforces the choice anyway since the ruleset permits all three methods and commitlint only runs in the local Row 2 — reason conflated. Corrected. The table now attributes the Row 4 — issue text contradicted what ships. Fixed. #726 was written before the patch-release decision and still said "no release-triggering commit here". Its npm section now carries the reversal explicitly, so Row 3 — accepted, no action. All five CHANGELOGs carrying all five Row 5 — verified, just not from your sandbox. The camo Useful confirmation in your review that the CI is green on |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
🎉 This PR is included in version 5.16.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.10 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.3.8 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.2.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Closes #726.
Points all six Socket badges at
badge.socket.dev, keeps the/latestsegment, and ships a patch release per package so the README npm serves picks the fix up immediately.Why the badges broke
socket.dev/api/badge/npm/package/<pkg>is behind a Cloudflare bot challenge. GitHub proxies README images through camo, so camo is what's being blocked, and it returns403 Forbiddenwhere a working badge returns200 image/svg+xml. Same README, two badges:A local
curlcan't diagnose this, because a scripted client from a residential IP getscf-mitigated: challengefrom both the old path and the new host. camo is the client that tells them apart.Why
/latestand not the bare formEvery real-world usage carries a version segment (
/latest,/canary, or a pin) and none uses the bare unversioned form. Verified through camo against repos already onbadge.socket.dev, including a scoped package, which is the case that matters for@allxsmith/bestax-bulmaand@allxsmith/eslint-plugin-bestax:badge.socket.dev/npm/package/dompurify/latestimage/svg+xmlbadge.socket.dev/npm/package/@borfast/arrispwgen/latestimage/svg+xmlAnd against this branch's own head, all six:
README.md@allxsmith/bestax-bulma/latestimage/svg+xmlbulma-ui/README.md@allxsmith/bestax-bulma/latestimage/svg+xmlcreate-bestax/README.mdcreate-bestax/latestimage/svg+xmlbestax-migrate/README.mdbestax-migrate/latestimage/svg+xmlbestax-mcp/README.mdbestax-mcp/latestimage/svg+xmleslint-plugin/README.md@allxsmith/eslint-plugin-bestax/latestimage/svg+xmlCommit structure
npm only re-reads a README when the package publishes, so each package needs its own patch. A package's
commit-analyzercarries{ scope: '!(<pkg>)', release: false }, so one scoped commit can only release one package — which is why this is six commits rather than one:fix(bulma-ui):@allxsmith/bestax-bulmapatchfix(create-bestax):create-bestaxpatchfix(bestax-migrate):bestax-migratepatchfix(bestax-mcp):bestax-mcppatchfix(eslint-plugin):@allxsmith/eslint-plugin-bestaxpatchdocs:(rootREADME.md){ type: 'docs', release: false }matches in all five configsEach commit touches exactly one file, inside the package its scope names. The root README additionally needs no release of its own because the root manifest is
private, so it is never published to npm.On merging
Prefer the repo's usual merge commit, but a squash is not dangerous. An earlier version of this section claimed a squash would leave "only one scope" and cost four of the five releases. That was wrong, and the mechanism is worth stating correctly:
squash_merge_commit_titleisCOMMIT_OR_PR_TITLE, and with six commits that resolves to the PR title, which is unscoped. In@semantic-release/commit-analyzer@13.0.1,lib/analyze-commit.jsmatches rules withisMatchWith(commit, rule, (object, src) => isString(src) && isString(object) ? micromatch.isMatch(object, src) : undefined). An unscoped commit parses toscope: null, which is not a string, so the customizer returnsundefinedand lodash falls back to strict equality — meaning{ scope: '!(<pkg>)' }cannot match it. With no custom rule matching,index.js:64falls through toDEFAULT_RELEASE_RULES, where angular'sfixis a patch. So a squash ships five patches too.What a squash actually costs is changelog quality: five identical unscoped lines instead of five scoped ones. Nothing enforces the choice either — the
mainruleset permitsmerge,squashandrebase, and commitlint runs only in the localcommit-msghook.Changelog attribution
Worth knowing: all five CHANGELOGs will carry all five
fix(…)lines, becauserelease-notes-generatoris handed the whole commit range rather than the scoped subset. That is pre-existing behaviour (bestax-migrate/CHANGELOG.mdalready shows a run of**bulma-ui:**entries); this PR is just the maximal case of it.Scope
Six image URLs. The link targets (
socket.dev/npm/package/<pkg>/overview) are unchanged, since those are pages a human opens in a real browser and the challenge passes there.Gate
The diff is markdown only, so I ran the gates that can see it rather than the full
pnpm all:prettier --checkon the six files andpnpm check:conformance, both green.Summary by CodeRabbit