Repository navigation
build(mabr): publish ROUTE proxy package - #19
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (applied directly; nested CLI not launched).
Reviewed head: 9100a63
Critical Issues (1)
- [gstack/review]
.github/workflows/publish-npm.yml:7—on.push.tagsis still["v*", "window-multicast-bridge-v*"]. Amabr-route-proxy-v0.1.0tag matches neither glob (v*is prefix-anchored), so pushing it never starts this workflow and the newpublish-mabrjob (:481-511) is unreachable — the package is never published, which is the whole point of the PR. Thepublish-mabr skippedrun on this PR cannot reveal this, since itsif:is false on every non-tag event anyway.- Add
"mabr-route-proxy-v*"to thetags:list.
- Add
Important Issues (1)
- [pr-review-toolkit: errors]
.github/workflows/publish-npm.yml:110— "Verify tarball contents" loops overnpm-packages/*.tgz npm-packages-bridge/*.tgzonly. The new public tarball innpm-packages-mabr/skips every shipping gate: non-emptypackage/dist/, no compiled.wasm/.so/.nodeartifact, and theBLO-/.github/privacy-patterns.txtprivate-identifier scan. This repo is public andremoveCommentslets JSDoc prose reachdist/, so an empty or leaking@blockcast/mabr-route-proxywould publish with provenance unchecked. (The package's owntest:e2eexercises a ROUTE wasm path, which makes the binary-artifact gate worth having here specifically.)- Add
npm-packages-mabr/*.tgzto thatforlist.
- Add
Suggestions (1)
- [pr-review-toolkit: code]
.github/workflows/publish-npm.yml:104—ls -l npm-packages npm-packages-bridgeomitsnpm-packages-mabr; add it so the build log shows what was packed.
Strengths
- Separate tag stream, separate artifact, and a tag-vs-packed-version assertion read from the tarball rather than the repo tree — consistent with the existing bridge pattern.
- Publish stays on a GitHub-hosted runner with scoped
id-token: writefor npm OIDC provenance;repository.directorymetadata is correct for provenance.
Recommended Action
- Fix Critical issues before merge.
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
Contributor
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (applied directly; nested CLI not launched).
Reviewed head: e01ac0a
Prior Findings Dispositioned (2)
- prior:9100a63 critical 1 — still-present — mirrored here as critical 1 —
.github/workflows/publish-npm.yml:7—tags: ["v*", "window-multicast-bridge-v*"]is unchanged at this head; amabr-route-proxy-v*tag still never triggers the workflow. - prior:9100a63 important 1 — still-present — mirrored here as important 1 —
.github/workflows/publish-npm.yml:112—for tgz in npm-packages/*.tgz npm-packages-bridge/*.tgzstill omitsnpm-packages-mabr/*.tgz.
Critical Issues (1)
- [gstack/review]
.github/workflows/publish-npm.yml:7— (prior:9100a63 critical 1)on.push.tagsdoes not includemabr-route-proxy-v*.v*is prefix-anchored, so pushingmabr-route-proxy-v0.1.0starts no run and thepublish-mabrjob (:481-511) is unreachable — the package this PR exists to publish is never published. Thepublish-mabr skippedresult on PR runs cannot surface this, because itsif:is false on every non-tag event.- Add
"mabr-route-proxy-v*"to thetags:list.
- Add
Important Issues (1)
- [pr-review-toolkit: errors]
.github/workflows/publish-npm.yml:112— (prior:9100a63 important 1) "Verify tarball contents" still loops only overnpm-packages/*.tgz npm-packages-bridge/*.tgz. The new public tarball innpm-packages-mabr/bypasses the non-emptydist/, no-compiled-binary, andBLO-/.github/privacy-patterns.txtprivate-identifier gates in a public repo.- Add
npm-packages-mabr/*.tgzto thatforlist.
- Add
Suggestions (1)
- [pr-review-toolkit: code]
.github/workflows/publish-npm.yml:104—ls -l npm-packages npm-packages-bridgeomitsnpm-packages-mabr; add it so the build log shows what was packed.
Strengths
- New in this head:
discoverSlsEndpointnow fetches DS/LLS with{ credentials: 'omit', mode: 'cors' }(packages/mabr-route-proxy/src/route.ts:237), keeping ambient browser cookies out of public discovery requests, and the test pins the exactRequestInitfor both requests (route.test.ts:459-462). - Separate tag stream and artifact, with the tag-vs-version assertion read from the packed tarball; publish keeps scoped
id-token: writeon a GitHub-hosted runner, andrepository.directoryis correct for provenance.
Recommended Action
- Fix Critical issues before merge.
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
Contributor
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (applied directly; nested CLI not launched).
Reviewed head: d1ae830
Looks good.
Prior Findings Dispositioned (4)
- prior:9100a63 critical 1 — fixed —
.github/workflows/publish-npm.yml:7—tags: ["v*", "window-multicast-bridge-v*", "mabr-route-proxy-v*"], so amabr-route-proxy-v*tag now triggers the workflow andpublish-mabr(:481-483) can run. - prior:9100a63 important 1 — fixed —
.github/workflows/publish-npm.yml:112— the "Verify tarball contents" loop now includesnpm-packages-mabr/*.tgz. The mabr tarball now goes through the same checks as the other two: non-emptydist/, no binaries, a private-identifier scan, noworkspace:/file:deps, and thepublishConfig/repositorychecks. - prior:e01ac0a critical 1 — fixed —
.github/workflows/publish-npm.yml:7— same as prior:9100a63 critical 1, which this mirrored. - prior:e01ac0a important 1 — fixed —
.github/workflows/publish-npm.yml:112— same as prior:9100a63 important 1, which this mirrored.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [gstack/review]
.github/workflows/publish-npm.yml:510—@blockcast/mabr-route-proxydoes not exist on the npm registry yet (GET registry.npmjs.org/@blockcast%2fmabr-route-proxy→ 404), and this workflow publishes by OIDC only, with noNPM_TOKEN. npm trusted publishers are set up per package, so the firstmabr-route-proxy-v0.1.0tag will probably fail atnpm publishunless someone sets up the package on npm first. The error will look like an auth failure orE404 PUT. Before pushing the first tag, set up a trusted publisher for this package/workflow on npmjs (or publish it once by hand).
Strengths
- The release pieces are kept separate: tag stream, artifact, and an
if:on the publish job. The tag is checked against the version in the packed tarball, not the repo tree.publish-mabrcorrectly leaves outregistry-url:, so there is no fallback junk credential. discoverSlsEndpointsends{ credentials: 'omit', mode: 'cors' }, and the test pins it for both LLS requests.- The
buildjob builds and tests the package before packing, so a brokendist/fails before upload.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
@blockcast/mabr-route-proxyis source-complete and tested, but the beacon release workflow does not build or publish it. Player-origin builds therefore cannot consume the canonical DS/slt→ SLS(S,G)→ DRIAD implementation without copying generated vendor files.Change
mabr-route-proxy-v<version>tag stream so the existing gateway package tags remain independent.Validation:
npm run buildandnpm testinpackages/mabr-route-proxy(32 passed).