Feature/licensing architecture#30
Merged
Merged
Conversation
…ased plugin licensing and revocation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements a comprehensive licensing system for Tempo plugins (JWT-based activation keys, discovery cascade, revocation checking) while publishing ChangesLicensing Infrastructure & Core Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
packages/tempo/doc/tempo.license.md (1)
19-36: ⚡ Quick winConsider adding setup context to the preview example.
The code example shows
t.term.astronomyusage without any prior import or plugin registration. While this previews the API nicely, readers might be confused about how to actually enable the plugin.Consider either:
- Adding a comment like
// After installing and registering the plugin:before line 19, or- Adding a forward reference like
// See "Applying Your License Key" below for full setupThis would bridge the gap between the preview and the complete setup shown in lines 59-79.
🤖 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/doc/tempo.license.md` around lines 19 - 36, The example creates a Tempo instance and accesses t.term.astronomy without any setup context; add a brief clarifying comment above the example (e.g., referencing plugin registration or license setup) so readers know this preview assumes the plugin/ license is already installed/registered — mention Tempo and t.term.astronomy specifically and optionally add a forward reference like “See ‘Applying Your License Key’ below for full setup” or “After installing and registering the plugin:” to make the sample self-contained.
🤖 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/astro-repl.ts`:
- Around line 5-8: The code includes a hard-coded JWT literal assigned to
mockToken and passed into Tempo.init, which must be removed from source; replace
the literal with a call that reads the token from configuration/environment
(e.g., process.env.TEMPO_MOCK_TOKEN or a test-only config loader) or generate a
non-secret dummy value at runtime, and update the Tempo.init invocation to use
that variable (referencing mockToken and Tempo.init) so no secret string is
committed into the repository or history.
- Line 3: The import in astro-repl.ts currently uses a machine-local absolute
path for AstroTerm which will break elsewhere; replace that absolute path with a
project-relative or package import (e.g., a relative path into the local package
dist or the package name used in package.json) so AstroTerm is imported
portably; update the import line that references AstroTerm to use the correct
module specifier (relative path or published package name) and verify the
build/test import resolution works.
In `@packages/tempo/doc/tempo-vs-temporal.md`:
- Around line 97-106: Update the examples and docs to accurately reflect the
API: state that t.since(...) always returns a string (not a Duration) and
document its two output modes—ISO duration string when called without a unit
(e.g., "-P1DT9H...") and human-readable relative strings when a unit is provided
(e.g., "1d ago")—and edit the example uses of t.until(...) to use documented
aliases (replace the undocumented '3pm' with the Period alias 'afternoon') or
add a brief note that literal time strings require registering an alias;
reference the t.since and t.until methods in the change so the examples and
explanatory text match the actual module.duration.ts signatures and parsing
behavior.
In `@packages/tempo/doc/tempo.license.md`:
- Around line 59-79: The example uses a non-existent premium plugin and
incorrect term access; replace the fake import/export and usage with a real
implemented term plugin (e.g., import and extend Tempo with the implemented
SeasonTerm) and update usage to access term metadata via the term name (e.g.,
use t.term.season rather than t.term.astronomy or t.term.astro), or
alternatively change the example text to state that "astro" is a planned premium
feature not yet available; update references to Tempo.init, Tempo.extend,
SeasonTerm, and t.term.season accordingly.
In `@packages/tempo/doc/tempo.term.md`:
- Around line 64-69: The example output for the summer term is wrong: update the
sample metadata produced by new Tempo('01-Jul-2025') so t.term.szn and
t.term.season show Summer with a summer-appropriate symbol instead of
"Snowflake" (e.g., change symbol to 'Sun' or 'Sunflower'), keeping key: 'Summer'
and the existing day/month/sphere fields; locate the example references to
Tempo, t.term.szn and t.term.season and replace only the symbol value in the
sample output.
In `@packages/tempo/doc/tempo.ticker.md`:
- Line 21: The in-page anchor fragments referencing the "Reporting & Registry"
heading are broken; update the links that point to the Reporting & Registry
section (the places that mention Ticker.active / the Ticker namespace) to match
the generated heading ID or add an explicit ID to the heading. Locate the
"Reporting & Registry" heading in the document and either change the links'
fragments to the exact slug your markdown renderer generates (e.g., the
slugified form) or modify the heading to include an explicit anchor (e.g., add
{`#reporting-registry`}) so references to "`#reporting-registry`" (used when linking
to Ticker.active) resolve correctly. Ensure both occurrences (the link near the
Ticker import line and the other reference) are updated consistently.
---
Nitpick comments:
In `@packages/tempo/doc/tempo.license.md`:
- Around line 19-36: The example creates a Tempo instance and accesses
t.term.astronomy without any setup context; add a brief clarifying comment above
the example (e.g., referencing plugin registration or license setup) so readers
know this preview assumes the plugin/ license is already installed/registered —
mention Tempo and t.term.astronomy specifically and optionally add a forward
reference like “See ‘Applying Your License Key’ below for full setup” or “After
installing and registering the plugin:” to make the sample self-contained.
🪄 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: a369bd4a-4020-4adf-b309-0a828b86478c
📒 Files selected for processing (33)
packages/tempo/.vitepress/config.tspackages/tempo/README.mdpackages/tempo/bin/astro-repl.tspackages/tempo/bin/resolve-types.tspackages/tempo/doc/architecture.mdpackages/tempo/doc/commercial.mdpackages/tempo/doc/comparison.mdpackages/tempo/doc/lazy-evaluation-pattern.mdpackages/tempo/doc/migration-guide.mdpackages/tempo/doc/releases/v1.x.mdpackages/tempo/doc/releases/v2.x.mdpackages/tempo/doc/releases/v3.x.mdpackages/tempo/doc/tempo-vs-temporal.mdpackages/tempo/doc/tempo.benchmarks.mdpackages/tempo/doc/tempo.config.mdpackages/tempo/doc/tempo.cookbook.mdpackages/tempo/doc/tempo.debugging.mdpackages/tempo/doc/tempo.license.mdpackages/tempo/doc/tempo.modularity.mdpackages/tempo/doc/tempo.parse.mdpackages/tempo/doc/tempo.pledge.mdpackages/tempo/doc/tempo.plugin.mdpackages/tempo/doc/tempo.shorthand.mdpackages/tempo/doc/tempo.term.mdpackages/tempo/doc/tempo.ticker.mdpackages/tempo/package.jsonpackages/tempo/rollup.config.jspackages/tempo/src/module/module.mutate.tspackages/tempo/src/plugin/plugin.util.tspackages/tempo/src/plugin/term/term.index.tspackages/tempo/src/plugin/term/term.util.tspackages/tempo/src/support/support.license.tspackages/tempo/vitest.config.ts
✅ Files skipped from review due to trivial changes (19)
- packages/tempo/README.md
- packages/tempo/doc/tempo.modularity.md
- packages/tempo/doc/lazy-evaluation-pattern.md
- packages/tempo/doc/tempo.parse.md
- packages/tempo/doc/commercial.md
- packages/tempo/doc/releases/v3.x.md
- packages/tempo/doc/tempo.cookbook.md
- packages/tempo/doc/migration-guide.md
- packages/tempo/src/support/support.license.ts
- packages/tempo/doc/releases/v1.x.md
- packages/tempo/doc/tempo.plugin.md
- packages/tempo/doc/tempo.debugging.md
- packages/tempo/doc/tempo.benchmarks.md
- packages/tempo/doc/tempo.pledge.md
- packages/tempo/doc/tempo.config.md
- packages/tempo/doc/tempo.shorthand.md
- packages/tempo/doc/architecture.md
- packages/tempo/doc/releases/v2.x.md
- packages/tempo/doc/comparison.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/tempo/src/plugin/term/term.index.ts
- packages/tempo/rollup.config.js
- packages/tempo/bin/resolve-types.ts
- packages/tempo/package.json
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/tempo/doc/tempo.ticker.md (1)
162-162:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAnchor link is still brittle; add an explicit heading ID for Zombie Tickers.
[Disposer Pattern](#zombie-tickers-warning)can fail depending on slug generation. Add an explicit ID on the target heading to make this stable.Proposed fix
-## 🧟 Zombie Tickers (Warning) +## 🧟 Zombie Tickers (Warning) {`#zombie-tickers-warning`}Also applies to: 275-275
🤖 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/doc/tempo.ticker.md` at line 162, The anchor for "Zombie Tickers (Warning)" is brittle; add an explicit HTML ID to the heading so links like [Disposer Pattern](`#zombie-tickers-warning`) are stable — update the heading "Zombie Tickers (Warning)" to include an explicit id attribute (e.g., <h2 id="zombie-tickers-warning"> or the Markdown-compatible syntax `## Zombie Tickers (Warning) {`#zombie-tickers-warning`}`) and do the same for the other occurrence referenced (the matching heading at the other location) so both targets have explicit IDs.packages/tempo/doc/tempo.license.md (1)
43-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnify the Astro plugin package name across install/import instructions.
The guide uses two different package names (
@magmacomputing/term-plugin-astrovs@magmacomputing/tempo-plugin-astro), so the setup flow is inconsistent and likely fails for readers copying commands.Suggested doc fix
-1. Run `npm install `@magmacomputing/term-plugin-astro`` in your project. +1. Run `npm install `@magmacomputing/tempo-plugin-astro`` in your project.(Or change the import path instead—just keep both lines identical.)
#!/bin/bash # Verify which package name is canonically used in docs and code. rg -n -C2 '`@magmacomputing/`(term-plugin-astro|tempo-plugin-astro)' packages/tempoAlso applies to: 65-65
🤖 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/doc/tempo.license.md` at line 43, The docs use two different npm package names—@magmacomputing/term-plugin-astro and `@magmacomputing/tempo-plugin-astro`—so update packages/tempo/doc/tempo.license.md to use a single canonical name everywhere (pick the name actually published/used in imports), and make the install command (the line containing "Run `npm install ...`" at the current install instruction) match the import/usage lines (also check the other occurrence around the later install line). Ensure both install commands and any import paths reference the same package identifier consistently.
🤖 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/rollup.config.js`:
- Around line 162-165: The path-sanitization currently only removes one leading
"../" from normalizedRel, so modulePath (used to build the
`lib/${dir}${name}.js` output) can still contain remaining "../" segments for
inputs like "../../node_modules/..."; update the logic that computes
`modulePath` (derived from `normalizedRel`) to strip all leading parent segments
before calling path.dirname — e.g., remove all leading "../" occurrences (or
repeatedly resolve/normalize until no leading "..") so `modulePath` never starts
with "../", then compute `dir` and return the stable `lib/...${name}.js` path as
before.
---
Duplicate comments:
In `@packages/tempo/doc/tempo.license.md`:
- Line 43: The docs use two different npm package
names—@magmacomputing/term-plugin-astro and
`@magmacomputing/tempo-plugin-astro`—so update packages/tempo/doc/tempo.license.md
to use a single canonical name everywhere (pick the name actually published/used
in imports), and make the install command (the line containing "Run `npm install
...`" at the current install instruction) match the import/usage lines (also
check the other occurrence around the later install line). Ensure both install
commands and any import paths reference the same package identifier
consistently.
In `@packages/tempo/doc/tempo.ticker.md`:
- Line 162: The anchor for "Zombie Tickers (Warning)" is brittle; add an
explicit HTML ID to the heading so links like [Disposer
Pattern](`#zombie-tickers-warning`) are stable — update the heading "Zombie
Tickers (Warning)" to include an explicit id attribute (e.g., <h2
id="zombie-tickers-warning"> or the Markdown-compatible syntax `## Zombie
Tickers (Warning) {`#zombie-tickers-warning`}`) and do the same for the other
occurrence referenced (the matching heading at the other location) so both
targets have explicit IDs.
🪄 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: c6a5b6c1-9e9b-4849-9d9c-05f8deed0cdf
📒 Files selected for processing (10)
packages/tempo/doc/tempo-vs-temporal.mdpackages/tempo/doc/tempo.license.mdpackages/tempo/doc/tempo.term.mdpackages/tempo/doc/tempo.ticker.mdpackages/tempo/package.jsonpackages/tempo/rollup.config.jspackages/tempo/src/support/support.license.tspackages/tempo/src/tsconfig.jsonpackages/tempo/test/plugins/licensing.full.test.tspackages/tempo/test/tsconfig.json
✅ Files skipped from review due to trivial changes (1)
- packages/tempo/doc/tempo.term.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/tempo/test/tsconfig.json
- packages/tempo/test/plugins/licensing.full.test.ts
- packages/tempo/package.json
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
Release Notes: v2.10.0
New Features
Tempo.licenseAPI to manage license state and discovery.definePremiumTermhelper.Bug Fixes
Documentation