Feature/licensing architecture#30
Open
magmacomputing wants to merge 7 commits into
Open
Conversation
…ased plugin licensing and revocation
📝 WalkthroughWalkthroughThis PR introduces a licensing system for Tempo plugins with JWT-based key verification and cascading discovery, publishes the shared library as a public npm package, and refactors Tempo's internal architecture for type safety. It spans version 2.9.3 → 2.10.0 across the monorepo and includes comprehensive documentation, tests, and build pipeline integration. ChangesLicensing Infrastructure & Core Integration
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tempo/src/tempo.class.ts (1)
1338-1354:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winLicense enforcement is bypassed on eager term discovery.
The premium-term guard exists in delegator callback path, but
#discover()eagerly installs getters for all terms without that check. In non-lazy mode, premium terms can still resolve even when status isRevoked/Expired/Invalid(Line 1338 onward).Suggested fix
} else { Tempo.#terms.forEach(term => { const define = (keyOnly: boolean, anchor?: any) => { + const rt = getRuntime(); + const licensedOut = [LICENSE.Revoked, LICENSE.Expired, LICENSE.Invalid].includes(rt.license.status as any); + if (licensedOut && hasOwn(rt.license.scopes, term.key)) return undefined; try { const res = term.resolve ? term.resolve.call(this, anchor) : term.define.call(this, keyOnly, anchor); const out = (getTermRange(this, (Array.isArray(res) ? (res as any) : [res]), keyOnly, anchor) as any); return isObject(out) ? secure(out) : out;🤖 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 `@packages/tempo/src/tempo.class.ts` around lines 1338 - 1354, Tempo.#terms are being eagerly exposed by the define closure and this.#setLazy without enforcing the premium-term/license check used elsewhere; update the define installation so it first checks the same premium-term guard used in the delegator callback path before invoking term.resolve or term.define, returning a license-denied placeholder or throwing as the delegator does when status is Revoked/Expired/Invalid; ensure the check is applied for both the primary key (where this.#setLazy is called with true) and the optional term.scope (false), and keep references to term.resolve, term.define and this.toDateTime() intact so only the pre-check is added rather than changing resolution logic.
🧹 Nitpick comments (2)
packages/tempo/rollup.config.js (1)
11-11: ⚡ Quick winBuild order dependency on external package.
The
licensePathresolves to../../../tempo-plugin/packages/_core/dist/index.js, which requires the_corepackage to be built before the Tempo bundle step runs. Ensure your build pipeline buildstempo-plugin/_corefirst, or this will fail with a module resolution error.🤖 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 `@packages/tempo/rollup.config.js` at line 11, The rollup config's licensePath constant (licensePath in packages/tempo/rollup.config.js) points to ../../../tempo-plugin/packages/_core/dist/index.js which introduces a build-order dependency; ensure the tempo-plugin/_core package is built before packaging Tempo by updating the build pipeline (CI task order or npm/yarn workspace scripts) to run the _core build prior to the Tempo bundle step, or change the reference to a source-level import pattern that doesn't require the compiled dist (e.g., point to the package entry or use the workspace package name) so the module resolution succeeds when running rollup.packages/tempo/test/plugins/licensing.full.test.ts (1)
10-10: ⚡ Quick winAvoid type cast; use proper reset method.
Bypassing TypeScript safety with
(rt as any).statehides potential issues. If runtime state needs to be reset for testing, expose a properresetState()test utility or make the property legally deletable.♻️ Proposed safer alternative
Add a test-only reset helper to the runtime module:
// In support.runtime.ts (test exports section) export function resetRuntimeForTesting() { const rt = getRuntime(); delete (rt as any).state; // Encapsulated here }Then in the test:
+import { getRuntime, resetRuntimeForTesting } from '#tempo/support/support.runtime.js'; + beforeEach(() => { - const rt = getRuntime(); - delete (rt as any).state; + resetRuntimeForTesting(); vi.clearAllMocks(); });🤖 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 `@packages/tempo/test/plugins/licensing.full.test.ts` at line 10, The test currently bypasses TypeScript safety by using delete (rt as any).state; instead add a proper test-reset helper in the runtime module (e.g., implement and export resetRuntimeForTesting() in support.runtime.ts) that obtains the runtime via getRuntime() and performs the controlled state deletion/reset internally, then update the test in licensing.full.test.ts to call resetRuntimeForTesting() rather than deleting (rt as any).state; this keeps the delete encapsulated, preserves type safety at call sites, and exposes a clear API for test resets (reference symbols: getRuntime, resetRuntimeForTesting, and the place where the test currently deletes rt.state).
🤖 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 @.gitignore:
- Around line 56-57: The .gitignore entries are malformed: replace the lines
that currently read ".npmrc (global/local if they contain tokens)" and "! .npmrc
(the one we created is safe as it uses variables)" with a proper ignore rule and
negation using the correct gitignore syntax — e.g. add a rule to ignore
tokenized/local npmrc files (such as ".npmrc" or ".npmrc.local") and use a
proper negation entry "!.npmrc" (no leading space) to keep the safe project
.npmrc; update the entries referenced in the diff so the ignore rule and the
negation are valid gitignore patterns.
In `@packages/tempo/plan/licensing_architecture.md`:
- Line 28: The global discovery key is inconsistent: some places use
globalThis[TEMPO_DISCOVERY].license while others use window.__TEMPO_DISCOVERY__;
pick and apply one canonical name everywhere (e.g., attach __TEMPO_DISCOVERY__
to the global object) and update references accordingly; replace uses of
globalThis[TEMPO_DISCOVERY] and window.__TEMPO_DISCOVERY__ with a single
canonical access pattern (for example globalThis.__TEMPO_DISCOVERY__ or
globalThis[TEMPO_DISCOVERY] where TEMPO_DISCOVERY is a single exported constant)
and ensure any property access (like .license) uses that unified symbol across
the codebase and docs.
- Line 47: Update the "Cross-Session Persistence" section to explicitly mark
persisting the license JWT to localStorage as opt-in and unsafe by default:
state the XSS token-theft risk, recommend memory or sessionStorage as the
default, require a documented opt-in flag (e.g.,
"enableCrossSessionPersistence") before using localStorage, and add a short
mitigation checklist (use HttpOnly cookies where possible, CSP, input
sanitization, short TTL, and rotate/revoke tokens) so the architecture does not
normalize insecure storage of the license JWT.
In `@packages/tempo/plan/licensing_strategy.md`:
- Around line 44-48: The licensing discovery order in licensing_strategy.md
(items referencing Tempo.init, process.env.TEMPO_LICENSE, .temporc /
tempo.config.json, and globalThis.TEMPO_LICENSE) diverges from the architecture
spec; reconcile and update the document to the authoritative cascade (matching
the architecture plan’s config file locations and precedence), and ensure the
documented precedence aligns with the implementation points (e.g., Tempo.init(),
environment variable lookup, config-file lookup, and globalThis variable) so
core and plugins use the exact same source order.
- Line 1: The opening sentence "Now that we have the 'Plugin' architecture, we
need to determine how this will merge into a Tempo v2 strategy.." has an extra
period and slightly awkward phrasing; edit that first line to end with a single
period and consider clearer wording such as "Now that we have the 'Plugin'
architecture, we need to determine how this will merge into the Tempo v2
strategy" or "how this will merge with the Tempo v2 strategy" to improve
readability.
- Line 61: Update the Markdown heading "###Scenario 1" to include a space after
the hash marks so it reads "### Scenario 1" to comply with ATX heading syntax
and markdown linting; locate and replace the token "###Scenario 1" in the
licensing_strategy.md content (the heading at the start of the Scenario 1
section) with "### Scenario 1".
In `@packages/tempo/plan/v3-architecture-discoverability.md`:
- Line 15: The doc currently contradicts itself by saying the Registry should
publish `@magmacomputing/library` to public npmjs while later referring to
“private registry authentication”; update the document to pick one registry
model and make CI auth steps explicit: either (A) state that
`@magmacomputing/library` is published to public npmjs and remove or replace any
“private registry authentication” text with npmjs publish instructions and GH
Actions/NPM_TOKEN setup, or (B) state that packages are hosted on a private
registry and replace the “npmjs” mention with the private registry name and add
CI auth instructions (npmrc, registry URL, credentials/secrets) for the release
pipeline; ensure you edit the “Registry” line and the line containing “private
registry authentication” so both reflect the chosen model consistently.
In `@packages/tempo/src/plugin/term/term.type.ts`:
- Around line 31-33: The PremiumPlugin interface currently types status as
string; update it to the LICENSE enum type used by
Internal.LicenseState/Internal.ValidationResult (i.e. replace status?: string
with status?: enums.LICENSE or the LICENSE type alias in scope) so merging
license metadata into PremiumPlugin is type-safe; ensure any necessary
import/namespace (enums or LICENSE) is added and update usages of PremiumPlugin
that set status to use the enum values.
In `@packages/tempo/src/support/support.init.ts`:
- Around line 148-149: The current guard (runtime.license.key !== key) prevents
re-verification when the same license token is re-applied; remove or change that
guard so decodeJWT(key) and the verification flow always run when extend({
license }) is called (or add an explicit force flag to trigger re-check). Update
the conditional around runtime.license.key and decodeJWT(key) in support.init
(and the identical check in the extend({ license }) path) to allow re-running
the JWT decode/verification even if the incoming key equals runtime.license.key,
then continue the existing verification/update logic as before.
- Around line 166-179: validator.verify() is unhandled for rejections and its
async result can overwrite newer license state; capture a snapshot of the
current license identity before calling validator.verify() (e.g., const
initialJti = runtime.license.jti) and in the Promise resolution only apply res
to runtime.license if runtime.license.jti === initialJti (ignore stale results),
and add a .catch handler to handle Promise rejection by setting
runtime.license.error/status appropriately and calling logWarn; reference
validator.verify(), runtime.license, res.jti and LICENSE to locate where to
apply these changes.
In `@packages/tempo/test/plugins/licensing.full.test.ts`:
- Line 139: Replace the fragile "await new Promise(r => setTimeout(r, 0))"
polling with deterministic waiting: either use Vitest's vi.waitFor(() => /*
condition */) to poll the expected state change (e.g., vi.waitFor(() =>
someSpyOrStateHasBeenCalled())) or, if the module under test exposes the
init/reckoning promise, await that promise directly (e.g., await initReckoning()
or await returnedPromise) instead of setTimeout; update both occurrences of "new
Promise(r => setTimeout(r, 0))" so tests wait deterministically.
- Line 27: Replace the Node-only Buffer usage with a portable base64 helper: add
a small function base64Encode(input) that returns
Buffer.from(input).toString('base64') when Buffer exists, otherwise uses
TextEncoder + btoa (e.g. btoa(String.fromCharCode(...new
TextEncoder().encode(input)))) to produce base64 in browser/Deno; then replace
the Buffer.from(JSON.stringify(payload)).toString('base64') call used to build
mockToken (and the five other occurrences) with
base64Encode(JSON.stringify(payload)).
- Around line 115-127: Move the vi.mock('#tempo/license', ...) call out of the
test function into module scope so Vitest can hoist it; replace the inline class
behavior with a default mock and then in individual tests override behavior
using vi.mocked(...) or Validator.mockImplementation(...) to return the desired
verify() shape (referencing the Validator symbol and the existing verify method)
so each test can customize status/scopes without relying on an in-test vi.mock
call.
- Around line 152-156: The Revoked-status test relies on a mock created inside
another test; move the module mock for '#tempo/license' out of the test body to
module scope so Vitest hoists it (replace the in-test vi.mock(...) with a
top-level vi.mock('#tempo/license', ...)), then update the per-test behavior by
calling vi.mocked((await
import('#tempo/license'))).Validator.prototype.verify.mockImplementation(...) or
use vi.spyOn((await import('#tempo/license')),
'Validator').prototype.verify.mockImplementation(...) inside each test to set
the specific return for that scenario; this ensures vi.clearAllMocks() in
beforeEach won't remove the base mock and each test (e.g., the "transitions from
Pending to Active" and "handles Revoked status" tests) sets its own verify
behavior via Validator.prototype.verify.
In `@packages/tempo/vitest.config.ts`:
- Line 52: The vitest alias entries mapping /^#tempo\/license$/ ->
resolve(__dirname, '../../../tempo-plugin/packages/_core/dist/index.js') (and
the duplicate mapping later) hardcode an external repo path that doesn't exist;
remove those alias objects from the aliases array in vitest.config.ts so
Node/npm can resolve "#tempo/license" via the package
"@magmacomputing/tempo-plugin-_core" declared in package.json, or alternatively
replace the alias replacement with the package name
"@magmacomputing/tempo-plugin-_core" instead of the file-system resolve call to
let standard npm resolution handle it (update the alias objects referenced by
the find /^#tempo\/license$/ pattern and any duplicate entries).
---
Outside diff comments:
In `@packages/tempo/src/tempo.class.ts`:
- Around line 1338-1354: Tempo.#terms are being eagerly exposed by the define
closure and this.#setLazy without enforcing the premium-term/license check used
elsewhere; update the define installation so it first checks the same
premium-term guard used in the delegator callback path before invoking
term.resolve or term.define, returning a license-denied placeholder or throwing
as the delegator does when status is Revoked/Expired/Invalid; ensure the check
is applied for both the primary key (where this.#setLazy is called with true)
and the optional term.scope (false), and keep references to term.resolve,
term.define and this.toDateTime() intact so only the pre-check is added rather
than changing resolution logic.
---
Nitpick comments:
In `@packages/tempo/rollup.config.js`:
- Line 11: The rollup config's licensePath constant (licensePath in
packages/tempo/rollup.config.js) points to
../../../tempo-plugin/packages/_core/dist/index.js which introduces a
build-order dependency; ensure the tempo-plugin/_core package is built before
packaging Tempo by updating the build pipeline (CI task order or npm/yarn
workspace scripts) to run the _core build prior to the Tempo bundle step, or
change the reference to a source-level import pattern that doesn't require the
compiled dist (e.g., point to the package entry or use the workspace package
name) so the module resolution succeeds when running rollup.
In `@packages/tempo/test/plugins/licensing.full.test.ts`:
- Line 10: The test currently bypasses TypeScript safety by using delete (rt as
any).state; instead add a proper test-reset helper in the runtime module (e.g.,
implement and export resetRuntimeForTesting() in support.runtime.ts) that
obtains the runtime via getRuntime() and performs the controlled state
deletion/reset internally, then update the test in licensing.full.test.ts to
call resetRuntimeForTesting() rather than deleting (rt as any).state; this keeps
the delete encapsulated, preserves type safety at call sites, and exposes a
clear API for test resets (reference symbols: getRuntime,
resetRuntimeForTesting, and the place where the test currently deletes
rt.state).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0865a451-25a1-4ebc-8fb6-4e551f8a3542
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
.gitignore.npmrcCHANGELOG.mdpackage.jsonpackages/library/LICENSEpackages/library/package.jsonpackages/library/src/common/storage.library.tspackages/library/src/common/utility.library.tspackages/library/src/tsconfig.jsonpackages/library/test/tsconfig.jsonpackages/library/tsconfig.jsonpackages/tempo/CHANGELOG.mdpackages/tempo/package.jsonpackages/tempo/plan/licensing_architecture.mdpackages/tempo/plan/licensing_strategy.mdpackages/tempo/plan/v3-architecture-discoverability.mdpackages/tempo/rollup.config.jspackages/tempo/src/library.index.tspackages/tempo/src/module/module.parse.tspackages/tempo/src/plugin/plugin.util.tspackages/tempo/src/plugin/term/term.index.tspackages/tempo/src/plugin/term/term.type.tspackages/tempo/src/plugin/term/term.util.tspackages/tempo/src/support/support.default.tspackages/tempo/src/support/support.enum.tspackages/tempo/src/support/support.index.tspackages/tempo/src/support/support.init.tspackages/tempo/src/support/support.runtime.tspackages/tempo/src/support/support.symbol.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/src/tsconfig.jsonpackages/tempo/test/plugins/licensing.full.test.tspackages/tempo/test/tsconfig.jsonpackages/tempo/vitest.config.tstsconfig.base.json
💤 Files with no reviewable changes (2)
- packages/library/tsconfig.json
- packages/library/test/tsconfig.json
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/tempo/src/support/support.init.ts (1)
160-167:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrengthen stale-result guard for licenses without
jti.The current guard can pass stale results when tokens omit
jti(both sides remainundefined). CaptureactiveKey(or a monotonic version) and require both identity checks before mutatingruntime.license.Also applies to: 180-182
🤖 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 `@packages/tempo/src/support/support.init.ts` around lines 160 - 167, The stale-result guard using only initialJti can fail when jti is undefined; capture an additional identity (e.g., current runtime.license.key or a monotonic counter stored as activeKey) before calling Validator.verify inside Pledge.onResolve, and require BOTH runtime.license.jti === initialJti AND runtime.license.key === activeKey (or the monotonic counter unchanged) before mutating runtime.license; apply the same dual-check pattern to the other Pledge.onResolve instance(s) that perform similar license mutation (the blocks that create new Pledge<Internal.LicensingModule> and call Validator.verify).
🤖 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 `@packages/library/src/common/utility.library.ts`:
- Line 13: The JWT payload decoding uses atob or Buffer.from on the raw
base64url segment (variable part when computing payload), which will fail for
'-'/'_' and missing padding; normalize the base64url string before decoding by
replacing '-' with '+' and '_' with '/', then pad the string with '=' to a
length multiple of 4, and then decode (keeping the existing branch that uses
atob or Buffer.from). Update the payload construction logic (the line that sets
payload from atob/Buffer.from) to perform this normalization on part first.
- Around line 85-89: The runtime detection currently checks Node first using
isDefined(global.process?.versions?.node) which misidentifies Deno (it may
expose process.versions.node); change the order to detect Deno first by testing
isDefined(global.Deno) and returning { global, type: CONTEXT.Deno } before the
NodeJS check, keeping the isDefined helper and the existing return shapes (and
leaving the subsequent NodeJS branch that returns { global, type: CONTEXT.NodeJS
} unchanged).
In `@packages/tempo/package.json`:
- Line 116: The package mapping for "`#tempo/license`" points to
`@magmacomputing/tempo-plugin-_core` only in test overrides, but runtime code in
tempo.class.ts (dynamic import at/around line 789) requires the real package at
runtime; add "`@magmacomputing/tempo-plugin-_core`" to package.json "dependencies"
so consumers can resolve it, and keep the test stub mapping
(test/support/license-stub-pkg) only for tests (e.g., in devDependencies or test
override), ensuring the production mapping still resolves to the real package.
In `@packages/tempo/rollup.config.js`:
- Around line 113-129: The entryFileNames routing currently returns only the
basename (name) which causes collisions when preserveModules: true; update the
entryFileNames logic (the function using id, name and rel/path.relative) to
include path context or a short hash so outputs are unique: instead of returning
`lib/${name}.js` or `lic/${name}.js` use the relative path segments (e.g.,
derive a modulePath from rel or path.dirname(rel) and emit
`lib/${modulePath}/${name}.js` or `lic/${modulePath}/${name}.js`), or append a
deterministic short hash of id when path context is not applicable; keep the
same id/name/rel variables and ensure node path separators are normalized for
output.
In `@packages/tempo/src/support/support.runtime.ts`:
- Around line 172-185: resetRuntimeForTesting() currently clears license fields
but doesn't prevent pending async verify() callbacks from repopulating runtime.
Modify resetRuntimeForTesting() (in support.runtime.ts) to atomically bump a
verification token/version on the runtime (e.g., set rt.license.jti to a fresh
unique value or increment rt._verifyVersion) so any in-flight verify completions
can be detected as stale; then update the verify completion path (the
function/method that applies verification results to getRuntime()/rt.license) to
check that the token/version from when the verification started matches the
current runtime token/version and refuse to apply results if they differ. Ensure
the token is set before clearing other license fields and is used consistently
by verify() and its callbacks.
---
Duplicate comments:
In `@packages/tempo/src/support/support.init.ts`:
- Around line 160-167: The stale-result guard using only initialJti can fail
when jti is undefined; capture an additional identity (e.g., current
runtime.license.key or a monotonic counter stored as activeKey) before calling
Validator.verify inside Pledge.onResolve, and require BOTH runtime.license.jti
=== initialJti AND runtime.license.key === activeKey (or the monotonic counter
unchanged) before mutating runtime.license; apply the same dual-check pattern to
the other Pledge.onResolve instance(s) that perform similar license mutation
(the blocks that create new Pledge<Internal.LicensingModule> and call
Validator.verify).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87d582cd-9a6c-4565-b010-150bddf40496
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
.gitignorepackages/library/src/common/utility.library.tspackages/tempo/CHANGELOG.mdpackages/tempo/doc/releases/v2.x.mdpackages/tempo/package.jsonpackages/tempo/plan/licensing_architecture.mdpackages/tempo/plan/licensing_strategy.mdpackages/tempo/plan/v3-architecture-discoverability.mdpackages/tempo/rollup.config.jspackages/tempo/src/plugin/term/term.type.tspackages/tempo/src/support/support.init.tspackages/tempo/src/support/support.runtime.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tsconfig.jsonpackages/tempo/test/plugins/licensing.full.test.tspackages/tempo/test/support/license-stub-pkg/index.tspackages/tempo/test/support/license-stub-pkg/package.jsonpackages/tempo/test/tsconfig.jsonpackages/tempo/vitest.config.ts
✅ Files skipped from review due to trivial changes (3)
- packages/tempo/test/support/license-stub-pkg/package.json
- packages/tempo/doc/releases/v2.x.md
- packages/tempo/plan/v3-architecture-discoverability.md
🚧 Files skipped from review as they are similar to previous changes (6)
- .gitignore
- packages/tempo/test/tsconfig.json
- packages/tempo/src/tsconfig.json
- packages/tempo/plan/licensing_architecture.md
- packages/tempo/test/plugins/licensing.full.test.ts
- packages/tempo/src/tempo.class.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/tempo/src/support/support.license.ts (1)
9-14: ⚡ Quick winReplace
Promise<any>with a concrete result type.Line 9 weakens type guarantees on a core licensing contract. A typed return keeps call-sites safe and consistent with the new type-focused architecture.
Proposed fix
+type CommunityLicenseVerification = { + status: 'active'; + scopes: { community: true }; + error: null; +}; + export class Validator { constructor(public key: string) { } - async verify(): Promise<any> { + async verify(): Promise<CommunityLicenseVerification> { return { status: 'active', // Default to active for community use scopes: { community: true }, error: null }; } }🤖 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 `@packages/tempo/src/support/support.license.ts` around lines 9 - 14, The verify method currently returns Promise<any>; define a concrete result type (e.g., LicenseVerificationResult or LicenseStatus) that models the returned object shape (status: string, scopes: { community: boolean }, error: string | null) and update the method signature from verify(): Promise<any> to verify(): Promise<LicenseVerificationResult>; implement or export that interface/type in the same module (or a shared types file) and update any call-sites or imports to use the new type.
🤖 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 `@packages/tempo/bin/resolve-types.ts`:
- Around line 16-17: The code currently hardcodes LIC_SRC_DIR and writes to
LIC_DEST_DIR causing broken downstream types when
'../../../tempo-plugin/packages/@core/dist' is missing; update resolve-types.ts
to first check for existence of LIC_SRC_DIR (using fs.existsSync or fs.stat)
and: if it exists, proceed with the current copy/rewrite logic that targets
LIC_DEST_DIR; if it does not, skip the copy/rewrite and either create
LIC_DEST_DIR with a small fallback index.d.ts that re-exports or declares
minimal types or emit a clear console.warn + exit code depending on desired
behavior; ensure references to LIC_SRC_DIR, LIC_DEST_DIR and DIST_DIR are used
consistently and do not assume presence of external repo.
In `@packages/tempo/package.json`:
- Line 119: The package.json's import map entry "`#tempo/license`" points to a
source-only path "./src/support/support.license.ts" which won't be published;
change the import map to use a conditional mapping that prefers built artifacts
and falls back to source only for dev – e.g., update the "`#tempo/license`" entry
to reference the built file under dist (./dist/support/support.license.js or .ts
as appropriate) as the default and add a development-only conditional mapping to
./src/support/support.license.ts so consumers resolve to the published dist file
while local dev still works; locate the "`#tempo/license`" key in package.json and
mirror this approach for any other src-based mappings to match the package
"files" field.
---
Nitpick comments:
In `@packages/tempo/src/support/support.license.ts`:
- Around line 9-14: The verify method currently returns Promise<any>; define a
concrete result type (e.g., LicenseVerificationResult or LicenseStatus) that
models the returned object shape (status: string, scopes: { community: boolean
}, error: string | null) and update the method signature from verify():
Promise<any> to verify(): Promise<LicenseVerificationResult>; implement or
export that interface/type in the same module (or a shared types file) and
update any call-sites or imports to use the new type.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2c3329bb-6eac-4037-a4d2-a4a88a01d625
📒 Files selected for processing (9)
packages/library/src/common/utility.library.tspackages/tempo/CHANGELOG.mdpackages/tempo/bin/resolve-types.tspackages/tempo/package.jsonpackages/tempo/rollup.config.jspackages/tempo/src/support/support.init.tspackages/tempo/src/support/support.license.tspackages/tempo/src/support/support.runtime.tspackages/tempo/vitest.config.ts
✅ Files skipped from review due to trivial changes (1)
- packages/tempo/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/tempo/src/support/support.runtime.ts
- packages/library/src/common/utility.library.ts
- packages/tempo/src/support/support.init.ts
Comment on lines
+16
to
+17
| const LIC_SRC_DIR = path.resolve('../../../tempo-plugin/packages/@core/dist'); | ||
| const LIC_DEST_DIR = path.resolve(DIST_DIR, 'lic'); |
There was a problem hiding this comment.
License type generation is coupled to a hardcoded external repo path.
The new #tempo/license d.ts rewrite depends on LIC_SRC_DIR existing at ../../../tempo-plugin/packages/@core/dist. If that path is missing, dist/lic types may not be produced while declarations still rewrite to lic/index.js, causing broken type resolution downstream.
Proposed fix
-const LIC_SRC_DIR = path.resolve('../../../tempo-plugin/packages/@core/dist');
const LIC_DEST_DIR = path.resolve(DIST_DIR, 'lic');
+const COMMUNITY_LIC_DTS = path.resolve(DIST_DIR, 'support/support.license.d.ts');
+const PREMIUM_LICENSE_PATH = process.env.TEMPO_LICENSE_PATH
+ ? path.resolve(process.env.TEMPO_LICENSE_PATH)
+ : null;
+const LIC_SRC_DIR = PREMIUM_LICENSE_PATH ? path.dirname(PREMIUM_LICENSE_PATH) : null;
@@
-if (fs.existsSync(LIC_SRC_DIR)) {
+if (LIC_SRC_DIR && fs.existsSync(LIC_SRC_DIR)) {
if (!fs.existsSync(LIC_DEST_DIR)) fs.mkdirSync(LIC_DEST_DIR, { recursive: true });
const licFiles = fs.readdirSync(LIC_SRC_DIR).filter(f => f.endsWith('.d.ts'));
licFiles.forEach(file => {
fs.copyFileSync(path.join(LIC_SRC_DIR, file), path.join(LIC_DEST_DIR, file));
});
}
+
+// Always guarantee a lic/index.d.ts fallback for community builds
+if (!fs.existsSync(LIC_DEST_DIR)) fs.mkdirSync(LIC_DEST_DIR, { recursive: true });
+if (!fs.existsSync(path.join(LIC_DEST_DIR, 'index.d.ts')) && fs.existsSync(COMMUNITY_LIC_DTS)) {
+ fs.copyFileSync(COMMUNITY_LIC_DTS, path.join(LIC_DEST_DIR, 'index.d.ts'));
+}Also applies to: 41-47, 79-99
🤖 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 `@packages/tempo/bin/resolve-types.ts` around lines 16 - 17, The code currently
hardcodes LIC_SRC_DIR and writes to LIC_DEST_DIR causing broken downstream types
when '../../../tempo-plugin/packages/@core/dist' is missing; update
resolve-types.ts to first check for existence of LIC_SRC_DIR (using
fs.existsSync or fs.stat) and: if it exists, proceed with the current
copy/rewrite logic that targets LIC_DEST_DIR; if it does not, skip the
copy/rewrite and either create LIC_DEST_DIR with a small fallback index.d.ts
that re-exports or declares minimal types or emit a clear console.warn + exit
code depending on desired behavior; ensure references to LIC_SRC_DIR,
LIC_DEST_DIR and DIST_DIR are used consistently and do not assume presence of
external repo.
| "development": "./src/module/module.index.ts", | ||
| "default": "./dist/module/module.index.js" | ||
| }, | ||
| "#tempo/license": "./src/support/support.license.ts", |
There was a problem hiding this comment.
#tempo/license points to a non-shipped source path.
Line 119 maps #tempo/license to ./src/support/support.license.ts, but src/ is not published (see files), so consumer resolution can break after publish. Use a conditional import map with a dist default.
Proposed fix
- "`#tempo/license`": "./src/support/support.license.ts",
+ "`#tempo/license`": {
+ "development": "./src/support/support.license.ts",
+ "default": "./dist/lic/index.js"
+ },🤖 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 `@packages/tempo/package.json` at line 119, The package.json's import map entry
"`#tempo/license`" points to a source-only path "./src/support/support.license.ts"
which won't be published; change the import map to use a conditional mapping
that prefers built artifacts and falls back to source only for dev – e.g.,
update the "`#tempo/license`" entry to reference the built file under dist
(./dist/support/support.license.js or .ts as appropriate) as the default and add
a development-only conditional mapping to ./src/support/support.license.ts so
consumers resolve to the published dist file while local dev still works; locate
the "`#tempo/license`" key in package.json and mirror this approach for any other
src-based mappings to match the package "files" field.
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
1 participant
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.
First draft of the Licensing Strategy to protect Premium Term Plugins
Summary by CodeRabbit
New Features
decodeJWTandbase64Encodeutility functionsTempo.licenseAPI for license status inspectionBug Fixes
Documentation