feat: build publishable Relay npm package - #47
Conversation
|
Warning Review limit reached
Next review available in: 6 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe PR makes Relay publishable as Changesnpm distribution and installed runtime
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
krishna916
left a comment
krishna916
left a comment
There was a problem hiding this comment.
Review verdict: Changes required before merge
The implementation is broadly well structured and CI is green, but one packaging-safety requirement is not met.
High — Tarball validation is not a positive allowlist
inspectTarball() only checks that required entries exist and that entries do not match a small forbidden-pattern list. Any unexpected file that does not match those patterns is accepted. For example, an accidentally committed file under integrations/, skills/, or dist/ would be published without failing package verification.
Issue #40 explicitly requires package-content allowlisting and inspection, and the implementation plan calls for rejecting every unapproved tarball path. Please define the complete expected inventory (allowing only intentional hashed web filenames through a narrowly bounded pattern) and fail when entries contains anything outside that allowlist.
The check should report three categories independently:
- missing required entries
- unexpected/unapproved entries
- explicitly forbidden sensitive entries
Add a regression test that injects an otherwise harmless unexpected path such as package/integrations/internal-notes.txt or package/dist/debug.txt and proves inspection fails.
Medium — Avoid hard-coded application version in installed smoke
scripts/package/smoke-installed-package.ts compares MCP and HTTP health against the literal 0.1.0. This will make the package smoke fail on the first version bump even when all shipped components correctly use the new package version. Read the expected version from the source/tarball package metadata once and compare every runtime against that value. This also better verifies the issue requirement that one application version covers CLI, MCP, UI, and assets.
Medium — Do not accept RELAY_DB_PATH=:memory: in installed operation
resolveRuntimePaths() exempts :memory: regardless of whether the value came from explicit in-process injection or RELAY_DB_PATH. The approved #39 contract permits test/internal injection but requires installed environment overrides to be absolute. Preserve :memory: only for explicitDatabasePath; reject it when supplied through RELAY_DB_PATH.
After these changes, rerun pnpm verify, tarball inspection, installed-prefix smoke, and the MCPB staging regression checks.
| const forbidden = entries.filter((path) => | ||
| FORBIDDEN_PACKAGE_PATTERNS.some((pattern) => pattern.test(path)), | ||
| ); | ||
| const webAssets = entries.filter((path) => |
There was a problem hiding this comment.
This is a denylist plus required-file check, not the exact positive allowlist required by #40. Compute unexpected = entries.filter(entry => !isApproved(entry)) and fail for every unapproved path. Keep the web hashed-asset allowance narrow, and add a regression test using an unexpected file under an otherwise published directory.
| env: { ...process.env, RELAY_DB_PATH: databasePath }, | ||
| stderr: 'pipe', | ||
| }); | ||
| const client = new Client({ name: 'relay-installed-package-smoke', version: '1.0.0' }); |
There was a problem hiding this comment.
The expected application version should come from package metadata, not the literal 0.1.0. Otherwise the smoke test becomes stale on every release and does not prove cross-component version consistency against the package being tested.
| const candidate = input.explicitDatabasePath ?? env.RELAY_DB_PATH; | ||
| if (candidate !== undefined) { | ||
| const normalized = candidate.trim(); | ||
| if (!normalized) |
There was a problem hiding this comment.
This exception also permits RELAY_DB_PATH=:memory:. The #39 contract allows internal/test injection precedence but requires the installed environment override to be absolute. Apply the :memory: exception only when explicitDatabasePath supplied the candidate.
Luna review-fix implementation planI added an execution-ready Superpowers plan directly to this PR branch:
Commit: Luna should execute it task-by-task with The plan is intentionally limited to the three review findings:
It includes exact file changes, test cases, implementation sketches, focused verification commands, commit boundaries, review-thread reply text, and a final human verification gate. Do not broaden the PR into setup, doctor, release, or packaging architecture redesign. |
krishna916
left a comment
krishna916
left a comment
There was a problem hiding this comment.
Re-review complete.
The three requested changes are correctly addressed:
- tarball inventory now fails closed through
isApprovedPackagePath, with narrow generated Vite-asset patterns and regression tests for unexpected files; - installed MCP/UI version checks derive the expected version from package metadata rather than hard-coding
0.1.0; :memory:is accepted only for explicit internal/test injection, whileRELAY_DB_PATH=:memory:is rejected as non-absolute.
I found no remaining issue in those fixes.
One merge blocker remains: CI run 30708321527 is failing in tests/integration/mcpb-stage.test.ts because it asserts that staging must emit a chunk-*.js file. The same run proves the staged MCPB server starts successfully with native SQLite, so this appears to be a brittle build-shape assertion rather than a runtime failure. Update the test to verify required staged runtime behavior/files without requiring a chunk filename, then rerun the full CI gate. After CI is green, this is ready from my review perspective.
MCPB CI assertion fixRoot cause: the current Files changed:
The test now verifies behavior and required artifacts: Verification:
|
krishna916
left a comment
krishna916
left a comment
There was a problem hiding this comment.
Re-review complete. The brittle chunk-*.js assertion has been removed and replaced with behavior-relevant checks for the staged MCP entry point and loadable native better-sqlite3 dependency. The existing tests continue to verify dependency materialization, unrelated-CWD startup, protocol-clean stdout, and stderr startup failures. The latest CI run is green. I found no remaining correctness blockers from this review cycle.
Before merge, complete the issue #40 human gate: inspect the final tarball inventory and manually run the installed command outside the checkout. The PR is also still marked draft, and its verification summary/test counts should be refreshed before marking it ready.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
scripts/validate-mcpb-assets.ts (1)
100-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared pnpm constants instead of hardcoding them again.
REQUIRED_PNPM_OVERRIDESandREQUIRED_ONLY_BUILT_DEPENDENCIESare already exported fromscripts/package/verify-package-metadata.tsand imported byscripts/mcpb/model.ts. This file re-hardcodes the same values ({ tmp: '0.2.7' }and['better-sqlite3', 'esbuild']) instead of importing the constants. If the required override or build-approval list changes, one call site can be updated while this one silently drifts out of sync, weakening the check.♻️ Proposed fix to import shared constants
+import { + REQUIRED_ONLY_BUILT_DEPENDENCIES, + REQUIRED_PNPM_OVERRIDES, +} from '../package/verify-package-metadata.js'; + const rootPnpm = ( root as { pnpm?: { overrides?: Record<string, string>; onlyBuiltDependencies?: string[] } } ).pnpm; - if (JSON.stringify(rootPnpm?.overrides) !== JSON.stringify({ tmp: '0.2.7' })) + if (JSON.stringify(rootPnpm?.overrides) !== JSON.stringify(REQUIRED_PNPM_OVERRIDES)) fail('Root pnpm metadata must preserve the tmp 0.2.7 override.'); if ( JSON.stringify(rootPnpm?.onlyBuiltDependencies) !== - JSON.stringify(['better-sqlite3', 'esbuild']) + JSON.stringify(REQUIRED_ONLY_BUILT_DEPENDENCIES) ) fail('Root pnpm metadata must approve better-sqlite3 and esbuild builds.');🤖 Prompt for 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. In `@scripts/validate-mcpb-assets.ts` around lines 100 - 109, Update the pnpm metadata checks in the validation flow to import and reuse REQUIRED_PNPM_OVERRIDES and REQUIRED_ONLY_BUILT_DEPENDENCIES from scripts/package/verify-package-metadata.ts instead of hardcoding the object and array literals. Keep the existing JSON comparisons and failure messages while referencing the shared constants.scripts/package/verify-package-metadata.ts (1)
21-49: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd dedicated unit tests for the duplicate-key scanner.
countTopLevelKeyis a hand-rolled JSON scanner that gates a supply-chain-relevant check: it detects a duplicate top-level"pnpm"key thatJSON.parsewould otherwise silently resolve to the last occurrence. This guard is the kind of logic that most needs direct test coverage, since a subtle bug in it (for example, around escaped quotes or nested arrays) would silently weaken the anti-tampering check it exists to provide.No dedicated unit test file for this module appears in the reviewed files. Add unit tests that cover: single vs. duplicate top-level
pnpmkeys, a nested (non-top-level) key namedpnpm, and escaped quotes inside string values near the key.🤖 Prompt for 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. In `@scripts/package/verify-package-metadata.ts` around lines 21 - 49, Add a dedicated unit test suite for countTopLevelKey covering one versus multiple top-level "pnpm" keys, nested "pnpm" keys that must not count, and escaped quotes in nearby string values. Keep the tests focused on the scanner’s returned count and ensure duplicate detection remains reliable for valid JSON-like input.scripts/package/inspect-tarball.ts (1)
11-25: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winHarden
readTarEntriesagainst extended tar headers.
readTarEntriesparses only USTARname,prefix, andsizewithout reading the header typeflag or handling GNU long-name ('L') / PAX extended-header ('x'or'g') entries. A future tarball with paths above the plain header limits would be parsed as the wrong file instead of failing early, which weakens this fail-closed inventory check. Add an unsupported typeflag guard or switch to an existing tar parser.🤖 Prompt for 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. In `@scripts/package/inspect-tarball.ts` around lines 11 - 25, Harden readTarEntries by inspecting each tar header’s typeflag and rejecting unsupported GNU long-name ('L') and PAX extended-header ('x' or 'g') entries instead of treating them as regular files. Preserve normal USTAR name, prefix, size parsing, and fail closed with a clear error when these extended headers are encountered.tests/unit/distribution/package-metadata.test.ts (1)
1-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest file location does not mirror the source it tests.
This file lives at
tests/unit/distribution/package-metadata.test.ts, but it testsscripts/package/verify-package-metadata.ts. Move it totests/unit/scripts/package/verify-package-metadata.test.tsto mirror the source area.As per coding guidelines,
tests/unit/**/*.test.ts: "Place backend unit tests undertests/unit/, mirroring their source area."🤖 Prompt for 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. In `@tests/unit/distribution/package-metadata.test.ts` around lines 1 - 32, Move the package metadata test containing describe and verifyPackageMetadata to tests/unit/scripts/package/verify-package-metadata.test.ts so its location mirrors scripts/package/verify-package-metadata.ts. Preserve the existing test cases and imports unchanged.Source: Coding guidelines
tests/integration/packaged-assets.test.ts (1)
7-25: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winStage assets into an isolated directory instead of the live repo checkout.
This test calls
stagePackageAssets()without arootDir, so it writesassets/migrationsdirectly into the project checkout, perstagePackageAssets'sresolve(options.rootDir ?? process.cwd())behavior. Other packaging tests in this PR use isolated fixtures (for exampletests/fixtures/package-root/package.json) for the same kind of validation.Running this test concurrently with other integration tests that touch
assets/risks file collisions, and a process crash beforeafterEachruns would leave staged files in the repository checkout. Pass an isolatedrootDir(with the required prerequisite files staged) instead of relying onprocess.cwd().🤖 Prompt for 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. In `@tests/integration/packaged-assets.test.ts` around lines 7 - 25, Update the test around stagePackageAssets to create and use an isolated fixture rootDir containing the required package prerequisites, then pass that rootDir explicitly when staging assets. Update the staged migration and relay database assertions to resolve paths under the isolated root, and retain cleanup for the temporary fixture.
🤖 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 `@package.json`:
- Around line 2-22: Add a publishConfig entry in package.json for the scoped
package `@krishna916/relay`, setting its npm access to public so the standard npm
publish command succeeds with the intended visibility.
In `@src/interfaces/http/create-http-server.ts`:
- Around line 43-46: Change createHttpServer so the default web root from
resolvePackageAssets() is resolved once during server initialization, then pass
that captured directory into getStaticAsset or resolveStaticAsset for every
request. Remove the per-request default evaluation in resolveStaticAsset and
preserve the supplied options.assets override behavior.
In `@src/interfaces/mcp/main.ts`:
- Around line 6-9: Update runMcpServer to preserve the boolean result from
runMcpServerWithDependencies and return the corresponding numeric exit code,
yielding success for a true result and failure for a false startup result.
Ensure this value propagates through runRelay without allowing the MCP wrapper
to default failures to zero.
---
Nitpick comments:
In `@scripts/package/inspect-tarball.ts`:
- Around line 11-25: Harden readTarEntries by inspecting each tar header’s
typeflag and rejecting unsupported GNU long-name ('L') and PAX extended-header
('x' or 'g') entries instead of treating them as regular files. Preserve normal
USTAR name, prefix, size parsing, and fail closed with a clear error when these
extended headers are encountered.
In `@scripts/package/verify-package-metadata.ts`:
- Around line 21-49: Add a dedicated unit test suite for countTopLevelKey
covering one versus multiple top-level "pnpm" keys, nested "pnpm" keys that must
not count, and escaped quotes in nearby string values. Keep the tests focused on
the scanner’s returned count and ensure duplicate detection remains reliable for
valid JSON-like input.
In `@scripts/validate-mcpb-assets.ts`:
- Around line 100-109: Update the pnpm metadata checks in the validation flow to
import and reuse REQUIRED_PNPM_OVERRIDES and REQUIRED_ONLY_BUILT_DEPENDENCIES
from scripts/package/verify-package-metadata.ts instead of hardcoding the object
and array literals. Keep the existing JSON comparisons and failure messages
while referencing the shared constants.
In `@tests/integration/packaged-assets.test.ts`:
- Around line 7-25: Update the test around stagePackageAssets to create and use
an isolated fixture rootDir containing the required package prerequisites, then
pass that rootDir explicitly when staging assets. Update the staged migration
and relay database assertions to resolve paths under the isolated root, and
retain cleanup for the temporary fixture.
In `@tests/unit/distribution/package-metadata.test.ts`:
- Around line 1-32: Move the package metadata test containing describe and
verifyPackageMetadata to
tests/unit/scripts/package/verify-package-metadata.test.ts so its location
mirrors scripts/package/verify-package-metadata.ts. Preserve the existing test
cases and imports unchanged.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: def6d850-aa5d-44b4-9af5-5d7edba7653b
📒 Files selected for processing (45)
.gitignore.prettierignoreLICENSEREADME.mdTHIRD_PARTY_NOTICES.mddocs/distribution/npm-package.mddocs/superpowers/plans/2026-08-01-pr-47-review-fixes.mdpackage.jsonscripts/mcpb/model.tsscripts/mcpb/stage-linux-mcpb.tsscripts/package/inspect-tarball.tsscripts/package/package-files.tsscripts/package/smoke-installed-package.tsscripts/package/stage-package-assets.tsscripts/package/verify-package-metadata.tsscripts/validate-mcpb-assets.tsscripts/validate-repository-assets.tssrc/database/database-config.tssrc/database/migrate.tssrc/distribution/package-assets.tssrc/distribution/package-version.tssrc/distribution/platform-paths.tssrc/distribution/resolve-runtime-paths.tssrc/interfaces/cli/main.tssrc/interfaces/cli/run-relay.tssrc/interfaces/http/create-http-server.tssrc/interfaces/http/http-router.tssrc/interfaces/http/main.tssrc/interfaces/mcp/main.tssrc/interfaces/production-dependencies.tssrc/shared/package-metadata.tstests/fixtures/package-root/package.jsontests/fixtures/package-smoke/README.mdtests/integration/http-health.test.tstests/integration/installed-package.test.tstests/integration/mcpb-stage.test.tstests/integration/package-tarball.test.tstests/integration/packaged-assets.test.tstests/unit/database/connection.test.tstests/unit/distribution/package-assets.test.tstests/unit/distribution/package-metadata.test.tstests/unit/distribution/runtime-paths.test.tstests/unit/interfaces/cli/run-relay.test.tstests/unit/scripts/mcpb/stage-linux-mcpb.test.tstsup.config.ts
|
Addressed the remaining review findings in commit d5dd521. Root cause: the package metadata and packaging helpers had a few independent correctness/maintainability gaps. HTTP asset resolution evaluated the default package root per request, the MCP wrapper discarded the startup boolean so runRelay could convert failure to exit code 0, tar inspection accepted unsupported GNU/PAX extended headers as ordinary entries, and the package metadata/staging tests were not fully aligned with the scripts they exercise. Files changed:
The assertions are behavior-oriented: packaged-assets tests stage into an isolated fixture root; tar tests verify unsupported headers fail closed; the metadata scanner tests verify top-level counting while ignoring nested keys and escaped strings; HTTP tests use an explicit captured root; and runRelay tests verify non-zero MCP exit propagation. No artificial bundler chunks were added. Verification:
|
1 participant
Closes #40
Summary
@krishna916/relaywith one stablerelayexecutableVerification
pnpm verifypassed through formatting, lint, typecheck, coverage, build, metadata, and asset validation; final audit completed separately with registry accesspnpm verify:packagepassed from an isolated npm prefix and unrelated cwdpnpm pack:tarball+pnpm verify:package:contentspassed; normalized inventory contains 36 files and all four migrationspnpm test:mcpb:stageis skipped on Windows; Linux-only MCPB build/runtime evidence remains bounded to the supported Linux environmentSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests