Add package boundary check + migrate LocalNet to canton-dev-tools (ENG-1635) - #451
Conversation
ENG-1635: guard that only dist/** ships (no LocalNet CLI, DARs, or libs). Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
📝 WalkthroughWalkthroughThe pull request adds npm package artifact validation to CI and publishing workflows. It also migrates integration tooling to Canton development tools and configures shared-secret authentication for LocalNet tests. ChangesPackage artifact validation
Shared-secret test tooling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@scripts/check-package-artifacts.cjs`:
- Around line 77-89: Update the package path validation around packagePaths and
forbiddenPackagePathReason to enforce an allowlist: accept only paths under
dist/ and the explicit required npm metadata files, rejecting every other
packaged path. Preserve the existing required runtime entry checks, and add a
requirement for at least one file under dist/ocf-schema when that runtime
directory is needed.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: be79d325-5a33-40d0-b386-f4c82a823ab5
📒 Files selected for processing (3)
.github/workflows/ci.ymlpackage.jsonscripts/check-package-artifacts.cjs
There was a problem hiding this comment.
Pull request overview
Adds an npm packaging guard to prevent repository-only artifacts from entering the published SDK.
Changes:
- Validates
npm packcontents, size, and required entry points. - Exposes the validator through an npm script.
- Runs validation after CI builds.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
scripts/check-package-artifacts.cjs |
Implements package validation. |
package.json |
Adds the validation command. |
.github/workflows/ci.yml |
Runs validation in CI. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Switch the publish boundary guard from a denylist to an allowlist of dist/** plus npm metadata, require packaged dist/ocf-schema objects, and add table-driven rejection tests. Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
scripts/check-package-artifacts.cjs:89
- This only proves that one arbitrary file exists below
objects/. A package containing onlydist/ocf-schema/objects/Issuer.schema.json(the positive test fixture) passes, while exported runtime APIs support dozens of paths fromOCF_OBJECT_SCHEMA_PATHSand will throw when any requested schema or referenced schema is absent. Validate the complete runtime-required schema set (including referenced schemas), not just that the directory is non-empty.
const hasOcfSchemaObjects = [...packagePaths].some((packagePath) =>
packagePath.startsWith('dist/ocf-schema/objects/')
);
scripts/check-package-artifacts.cjs:38
npm-packlistalways includes rootCOPYINGfiles, but this exact-name set does not recognize them. Adding standard legal metadata such asCOPYINGwould therefore make this boundary check fail even though npm includes it regardless offiles. Recognize npm's README/license/licence/copying family case-insensitively rather than relying only on the enumerated variants.
This issue also appears on line 87 of the same file.
if (ALLOWED_NPM_METADATA_FILES.has(packagePath)) return true;
Run check:package-artifacts in publish.yml after prepare-release so the release artifact is validated before npm publish. Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
Rewire localnet* scripts to canton-dev-tools, drop the stale Quickstart pin override so Dev Tools defaults apply, and replace local testConfig helpers with @fairmint/canton-dev-tools/testing while keeping OCP-specific transaction helpers and the package-boundary pack guard. Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
HardlyDifficult
changed the title
| "lint": "eslint . --max-warnings=0", | ||
| "lint:fix": "eslint . --fix --max-warnings=0", | ||
| "localnet": "CANTON_LOCALNET_AUTH_MODE=shared-secret CANTON_LOCALNET_QUICKSTART_REF=$npm_package_config_localnet_quickstart_ref canton-localnet", | ||
| "localnet": "CANTON_LOCALNET_AUTH_MODE=shared-secret FAIRMINT_TEST_SHARED_SECRET=unsafe canton-dev-tools", |
There was a problem hiding this comment.
Fixed in afd7983 — test-ocp-quickstart.yml now uses Node 22 to match @fairmint/canton-dev-tools engines.
| "devDependencies": { | ||
| "@apidevtools/json-schema-ref-parser": "15.3.5", | ||
| "@eslint/eslintrc": "3.3.5", | ||
| "@fairmint/canton-dev-tools": "0.1.1", |
There was a problem hiding this comment.
Already addressed on this branch — @fairmint/canton-node-sdk is pinned to 0.0.232 in devDependencies (satisfies the Dev Tools peer). No further change needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/integration/setupSharedSecretEnv.ts`:
- Around line 8-11: Update the shared-secret setup near
FAIRMINT_TEST_SHARED_SECRET to also set OCP_TEST_AUTH_MODE to the shared-secret
mode used by integrationTestHarness.ts, ensuring client construction and
ledger-api-user rights setup use the same authentication source.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 24f30cf4-801f-4eab-b9a9-7778c03b764f
📒 Files selected for processing (11)
.github/workflows/publish.ymljest.integration.config.jspackage.jsonscripts/check-package-artifacts.cjsscripts/quickstart/waitForReady.tstest/integration/quickstart.smoke.test.tstest/integration/setup/integrationTestHarness.tstest/integration/setupSharedSecretEnv.tstest/scripts/checkPackageArtifacts.test.tstest/utils/testConfig.tstsconfig.tests.json
💤 Files with no reviewable changes (1)
- test/utils/testConfig.ts
@fairmint/canton-dev-tools@0.1.1 requires peer @fairmint/canton-node-sdk >=0.0.232; CI npm install failed on the prior 0.0.227 pin. Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
49-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the shared-secret override semantics.
The
localnetcommand overwrites an existingFAIRMINT_TEST_SHARED_SECRETwithunsafe. Use a fallback assignment instead. Thelogsargument is supported bycanton-dev-tools.🤖 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 `@package.json` at line 49, Update the localnet script’s FAIRMINT_TEST_SHARED_SECRET assignment to set unsafe only when the variable is not already defined, preserving caller-provided secrets while keeping canton-dev-tools and its logs argument behavior unchanged.
🤖 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.
Outside diff comments:
In `@package.json`:
- Line 49: Update the localnet script’s FAIRMINT_TEST_SHARED_SECRET assignment
to set unsafe only when the variable is not already defined, preserving
caller-provided secrets while keeping canton-dev-tools and its logs argument
behavior unchanged.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (3)
package.json:81
@fairmint/canton-dev-tools@0.1.1declares Node>=22, but.github/workflows/test-ocp-quickstart.yml:20-23still provisions Node 20 for the workflow that installs this dependency and invokes its CLI/testing entrypoint. The LocalNet verification therefore runs on an unsupported runtime and emits an engine mismatch; update that workflow to Node 22 as part of this cutover.
"@fairmint/canton-dev-tools": "0.1.1",
test/integration/setupSharedSecretEnv.ts:8
??=leaves an explicitly emptyFAIRMINT_TEST_SHARED_SECRETuntouched, while Dev Tools treats an empty value as unset. The helper then sees no shared-secret override and falls back to OAuth2 against this shared-secret LocalNet profile. Use||=so empty variables receive the OCP secret orunsafefallback too.
process.env.FAIRMINT_TEST_SHARED_SECRET ??=
scripts/quickstart/waitForReady.ts:15
??=does not replace an explicitly emptyFAIRMINT_TEST_SHARED_SECRET, but Dev Tools ignores empty values. In that casewaitForLedgerJsonApiReady()constructs an OAuth2 config even though this script targets the shared-secret LocalNet profile. Use||=to preserve non-empty overrides while treating an empty variable as unset.
process.env.FAIRMINT_TEST_SHARED_SECRET ??=
|
@copilot review |
|
@coderabbitai review |
|
@cursor review |
|
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b9394a7. Configure here.
| const hasOcfSchemaObjects = [...packagePaths].some((packagePath) => | ||
| packagePath.startsWith('dist/ocf-schema/objects/') |
There was a problem hiding this comment.
Deferring a full packed-vs-source schema manifest equality check for a follow-up. This PR already requires dist/ocf-schema/** plus at least one objects/ schema; the copy step publishes the complete tree and CI check:package-artifacts is green. Expanding to exact path-set equality is valuable but out of scope for the ENG-1635 LocalNet cutover.
Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
Summary
Hard cutover to published
@fairmint/canton-dev-tools@0.1.1(npm — notgithub:Fairmint/canton-dev-tools#…):check:package-artifacts, CI + publish wiring)@fairmint/canton-dev-tools@0.1.1as adevDependencylocalnet*scripts fromcanton-localnet→canton-dev-toolsconfig.localnet_quickstart_refoverride (5c90cf4d…) so Dev Tools pins applytest/utils/testConfig.ts; import from@fairmint/canton-dev-tools/testing(shared-secret env shim for OCP CI)transactionHelpers/ integration harnessPart of ENG-1635. Companion docs: https://github.com/Fairmint/dev-docs/pull/29. Node-sdk hard cutover (~2k LOC deleted): Fairmint/canton-node-sdk#398.
Test plan
npm install --legacy-peer-depsnpm run buildnpm run typechecknpm test(2811 tests)npm run check:package-artifactsnpm run localnet:verify)Note
Medium Risk
Touches npm publish gates and LocalNet/CI integration auth; runtime SDK API is unchanged, and the pack guard reduces accidental leakage of non-dist files.
Overview
Adds
npm run check:package-artifactsand runs it in CI and publish after build. The script dry-runsnpm pack, enforces that onlydist/**plus standard npm metadata ship, requires core entrypoints anddist/ocf-schema/objects, and caps unpacked size (configurable viaMAX_PACKAGE_UNPACKED_BYTES). Unit tests cover the validator.LocalNet / integration moves from
canton-localnetto@fairmint/canton-dev-tools@0.1.1:localnet*scripts invokecanton-dev-toolswith shared-secret env, dropsconfig.localnet_quickstart_ref, and bumps@fairmint/canton-node-sdk. In-repotest/utils/testConfig.tsis removed; integration tests and quickstart scripts use@fairmint/canton-dev-tools/testing, withsetupSharedSecretEnv.tswired intojest.integration.config.jsso OCP CI keeps shared-secret auth andOCP_TEST_AUTH_MODE.jsonwebtokenis dropped as a dev dependency. The QuickStart workflow uses Node 22.Reviewed by Cursor Bugbot for commit afd7983. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests