Skip to content

build(mabr): publish ROUTE proxy package - #19

Merged
kkroo merged 3 commits into
mainfrom
codex/publish-mabr-route-proxy
Oct 25, 2026
Merged

kkroo merged 3 commits into
mainfrom
codex/publish-mabr-route-proxy

Conversation

@kkroo

@kkroo kkroo commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

Problem

@blockcast/mabr-route-proxy is 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

  • mark the package public with repository metadata;
  • build/test and pack it in the existing release workflow;
  • publish it through a separate mabr-route-proxy-v<version> tag stream so the existing gateway package tags remain independent.

Validation: npm run build and npm test in packages/mabr-route-proxy (32 passed).

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.tags is still ["v*", "window-multicast-bridge-v*"]. A mabr-route-proxy-v0.1.0 tag matches neither glob (v* is prefix-anchored), so pushing it never starts this workflow and the new publish-mabr job (:481-511) is unreachable — the package is never published, which is the whole point of the PR. The publish-mabr skipped run on this PR cannot reveal this, since its if: is false on every non-tag event anyway.
    • Add "mabr-route-proxy-v*" to the tags: list.

Important Issues (1)

  • [pr-review-toolkit: errors] .github/workflows/publish-npm.yml:110 — "Verify tarball contents" loops over npm-packages/*.tgz npm-packages-bridge/*.tgz only. The new public tarball in npm-packages-mabr/ skips every shipping gate: non-empty package/dist/, no compiled .wasm/.so/.node artifact, and the BLO- / .github/privacy-patterns.txt private-identifier scan. This repo is public and removeComments lets JSDoc prose reach dist/, so an empty or leaking @blockcast/mabr-route-proxy would publish with provenance unchecked. (The package's own test:e2e exercises a ROUTE wasm path, which makes the binary-artifact gate worth having here specifically.)
    • Add npm-packages-mabr/*.tgz to that for list.

Suggestions (1)

  • [pr-review-toolkit: code] .github/workflows/publish-npm.yml:104 — ls -l npm-packages npm-packages-bridge omits npm-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: write for npm OIDC provenance; repository.directory metadata is correct for provenance.

Recommended Action

  1. Fix Critical issues before merge.
  2. Address Important issues this cycle.
  3. Consider Suggestions opportunistically.

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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; a mabr-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/*.tgz still omits npm-packages-mabr/*.tgz.

Critical Issues (1)

  • [gstack/review] .github/workflows/publish-npm.yml:7 — (prior:9100a63 critical 1) on.push.tags does not include mabr-route-proxy-v*. v* is prefix-anchored, so pushing mabr-route-proxy-v0.1.0 starts no run and the publish-mabr job (:481-511) is unreachable — the package this PR exists to publish is never published. The publish-mabr skipped result on PR runs cannot surface this, because its if: is false on every non-tag event.
    • Add "mabr-route-proxy-v*" to the tags: list.

Important Issues (1)

  • [pr-review-toolkit: errors] .github/workflows/publish-npm.yml:112 — (prior:9100a63 important 1) "Verify tarball contents" still loops only over npm-packages/*.tgz npm-packages-bridge/*.tgz. The new public tarball in npm-packages-mabr/ bypasses the non-empty dist/, no-compiled-binary, and BLO- / .github/privacy-patterns.txt private-identifier gates in a public repo.
    • Add npm-packages-mabr/*.tgz to that for list.

Suggestions (1)

  • [pr-review-toolkit: code] .github/workflows/publish-npm.yml:104 — ls -l npm-packages npm-packages-bridge omits npm-packages-mabr; add it so the build log shows what was packed.

Strengths

  • New in this head: discoverSlsEndpoint now 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 exact RequestInit for 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: write on a GitHub-hosted runner, and repository.directory is correct for provenance.

Recommended Action

  1. Fix Critical issues before merge.
  2. Address Important issues this cycle.
  3. Consider Suggestions opportunistically.

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 a mabr-route-proxy-v* tag now triggers the workflow and publish-mabr (:481-483) can run.
  • prior:9100a63 important 1 — fixed — .github/workflows/publish-npm.yml:112 — the "Verify tarball contents" loop now includes npm-packages-mabr/*.tgz. The mabr tarball now goes through the same checks as the other two: non-empty dist/, no binaries, a private-identifier scan, no workspace:/file: deps, and the publishConfig/repository checks.
  • 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-proxy does not exist on the npm registry yet (GET registry.npmjs.org/@blockcast%2fmabr-route-proxy → 404), and this workflow publishes by OIDC only, with no NPM_TOKEN. npm trusted publishers are set up per package, so the first mabr-route-proxy-v0.1.0 tag will probably fail at npm publish unless someone sets up the package on npm first. The error will look like an auth failure or E404 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-mabr correctly leaves out registry-url:, so there is no fallback junk credential.
  • discoverSlsEndpoint sends { credentials: 'omit', mode: 'cors' }, and the test pins it for both LLS requests.
  • The build job builds and tests the package before packing, so a broken dist/ fails before upload.

Recommended Action

  1. No blocking changes requested.
  2. Merge once the remaining required CI checks finish green.

@kkroo
kkroo merged commit df1f1d0 into main Oct 25, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant