Open
Conversation
Introduced new commands for managing releases and setting up the development environment. The `release` command allows for preparing, verifying, and publishing packages, while the `setup-dev` command seeds the local API environment with manifests. Additional utility functions and logging capabilities have been implemented to support these features.
Contributor
📝 WalkthroughWalkthroughThe pull request migrates the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
🌏 Preview Deployments
Built from commit: 🤖 This comment will be updated automatically when you push new commits to this PR. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates the internal @ucdjs/ucdjs-scripts tooling into a first-class scripts/ workspace package (now running directly from source), adds new CLI commands for release/setup workflows, and includes a small pipeline-server graph utility refactor for layer sorting.
Changes:
- Move
@ucdjs/ucdjs-scriptsfrompackages/ucdjs-scripts(built TS) toscripts/(JS + JSDoc typing) and update workspace/config to include it. - Add
release,refresh-manifests, andsetup-devCLI command implementations and wire them into workflows/package scripts. - Refactor pipeline-server graph layer sorting to materialize entries before sorting.
Reviewed changes
Copilot reviewed 28 out of 31 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/turbo.json | Removes build outputs task for scripts package (now source-run). |
| scripts/src/types.js | Adds JSDoc typedefs replacing TS interfaces. |
| scripts/src/lib/utils.js | Converts TS annotations to JSDoc; keeps client/root helpers. |
| scripts/src/lib/upload.js | Converts to JS + JSDoc; adjusts upload body handling and typing. |
| scripts/src/lib/manifest.js | New JS manifest generation/tar/etag utilities. |
| scripts/src/lib/logger.js | New lightweight logger with log-level support. |
| scripts/src/lib/config.js | Converts config resolution to JS + JSDoc typing. |
| scripts/src/lib/command.js | New CLI parsing/help utilities (node:util parseArgs). |
| scripts/src/index.js | New CLI dispatcher for setup-dev/refresh-manifests/release. |
| scripts/src/commands/setup-dev.js | Updates setup-dev command to new CLI framework + options parsing. |
| scripts/src/commands/release.js | Adds release command via @ucdjs/release-scripts. |
| scripts/src/commands/refresh-manifests.js | Adds refresh-manifests implementation (etag compare + upload). |
| scripts/release.ts | Removes legacy release entrypoint. |
| scripts/package.json | Updates exports/bin/imports to source-run JS; adds deps for new commands. |
| scripts/jsconfig.json | Adds JS typecheck config (extends shared base). |
| scripts/eslint.config.js | Adds eslint config for scripts workspace. |
| scripts/bin/ucdjs-scripts.js | Adds bin entrypoint that registers moonbeam then runs CLI. |
| pnpm-workspace.yaml | Adds scripts to workspace packages list. |
| pnpm-lock.yaml | Updates lockfile for workspace move + dependency adjustments. |
| packages/ucdjs-scripts/tsdown.config.ts | Removes obsolete build config (package moved). |
| packages/ucdjs-scripts/tsconfig.json | Removes obsolete TS config. |
| packages/ucdjs-scripts/tsconfig.build.json | Removes obsolete TS build config. |
| packages/ucdjs-scripts/src/types.ts | Removes TS types (replaced by JSDoc in scripts/). |
| packages/ucdjs-scripts/src/lib/manifest.ts | Removes old TS manifest lib (replaced by scripts/src/lib/manifest.js). |
| packages/ucdjs-scripts/src/lib/logger.ts | Removes old TS logger (replaced by scripts/src/lib/logger.js). |
| packages/ucdjs-scripts/src/index.ts | Removes old cac-based TS CLI entrypoint. |
| packages/ucdjs-scripts/src/commands/refresh-manifests.ts | Removes old TS refresh-manifests command. |
| packages/pipelines/pipeline-server/src/client/components/graph/graph-utils.ts | Fixes layerGroups sorting by converting iterator to array before toSorted. |
| package.json | Adds scripts workspace and routes release scripts through ucdjs-scripts CLI. |
| .github/workflows/release.yml | Adds CI checks for the new scripts workspace. |
| .github/workflows/refresh-manifest.yaml | Removes build/symlink steps; runs scripts directly via CLI. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
scripts/src/lib/upload.js:35
uploadManifestcurrently copies the tar data (Uint8Array.from(tar)) before uploading. This adds an extra allocation/copy and is unnecessary because Node'sfetchacceptsUint8Array/ArrayBufferViewas a request body. Consider passingtardirectly (or, if you specifically need anArrayBuffer, use a slice that respectsbyteOffset/byteLengthwithout copying twice).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "maxNodeModuleJsDepth": 0 | ||
| }, | ||
| "include": [ | ||
| "./src/**/*.js" |
Comment on lines
+29
to
+38
| async function createCommands() { | ||
| return createReleaseScripts({ | ||
| repo: "ucdjs/ucd", | ||
| workspaceRoot: getMonorepoRoot(), | ||
| packages: { | ||
| excludePrivate: true, | ||
| exclude: ["vscode-ucd"], | ||
| }, | ||
| githubToken: process.env.UCDJS_RELEASE_TOKEN ?? "", | ||
| types: { |
Comment on lines
+66
to
+107
| const parsed = parseCommand(args, { | ||
| usage: `$ ${CLI_NAME} release <prepare|verify|publish>`, | ||
| description: "Run the release workflow against the current workspace.", | ||
| commands: [ | ||
| { | ||
| name: "prepare", | ||
| description: "Create or update the release PR", | ||
| }, | ||
| { | ||
| name: "verify", | ||
| description: "Verify the release branch state", | ||
| }, | ||
| { | ||
| name: "publish", | ||
| description: "Publish releasable packages to npm", | ||
| }, | ||
| ], | ||
| }, { allowPositionals: true }); | ||
| if (!parsed) { | ||
| return; | ||
| } | ||
|
|
||
| if (parsed.positionals.length !== 1) { | ||
| console.error("Missing release command."); | ||
| printCommandHelp({ | ||
| usage: `$ ${CLI_NAME} release <prepare|verify|publish>`, | ||
| description: "Run the release workflow against the current workspace.", | ||
| commands: [ | ||
| { | ||
| name: "prepare", | ||
| description: "Create or update the release PR", | ||
| }, | ||
| { | ||
| name: "verify", | ||
| description: "Verify the release branch state", | ||
| }, | ||
| { | ||
| name: "publish", | ||
| description: "Publish releasable packages to npm", | ||
| }, | ||
| ], | ||
| }); |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/src/lib/utils.js (1)
39-55:⚠️ Potential issue | 🟠 MajorThe single-slot async cache can return the wrong client under concurrent calls.
cachedBaseUrlis updated beforecreateUCDClient()resolves. If two callers request different base URLs concurrently, the later one can leavecachedBaseUrlpointing at one URL whilecachedClientends up holding the other client instance, so a subsequentgetClient(baseUrl)may reuse the wrong client.🧵 Promise-cache fix
-/** `@type` {string | undefined} */ -let cachedBaseUrl; -/** `@type` {UCDClient | null} */ -let cachedClient = null; +/** `@type` {Map<string, Promise<UCDClient>>} */ +const clientCache = new Map(); /** * `@param` {string} baseUrl * `@returns` {Promise<UCDClient>} */ export async function getClient(baseUrl) { - if (cachedClient && cachedBaseUrl === baseUrl) { - return cachedClient; - } - - cachedBaseUrl = baseUrl; - cachedClient = await createUCDClient(baseUrl); - return cachedClient; + let clientPromise = clientCache.get(baseUrl); + if (!clientPromise) { + clientPromise = createUCDClient(baseUrl); + clientCache.set(baseUrl, clientPromise); + } + return clientPromise; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/src/lib/utils.js` around lines 39 - 55, getClient's two-slot cache (cachedBaseUrl + cachedClient) is racy: cachedBaseUrl is set before awaiting createUCDClient, so concurrent calls for different baseUrls can corrupt the cache. Fix by creating the client first (await createUCDClient(baseUrl) into a local variable), then atomically assign cachedClient and cachedBaseUrl only after the client resolves; alternatively replace the single-slot cache with a Map keyed by baseUrl or use an in-flight promise map to deduplicate concurrent requests—update getClient, cachedClient, cachedBaseUrl (or introduce a clientsByBaseUrl / inFlight map) accordingly so the cache assignment happens after the awaited creation.scripts/src/commands/setup-dev.js (1)
81-121:⚠️ Potential issue | 🟠 MajorFail the command when any upload is recorded as an error.
Lines 108-113 collect per-manifest failures, but Lines 117-120 still print a success summary and the command returns normally. That leaves a partially seeded dev environment looking healthy to callers.
Proposed fix
- logger.info("Upload complete!"); - logger.info(`Uploaded ${result.uploaded} manifests:`); + if (!result.success) { + for (const error of result.errors) { + logger.error(`Failed to upload ${error.version}: ${error.reason}`); + } + throw new Error(`Failed to seed ${result.errors.length} manifest(s)`); + } + + logger.info("Upload complete!"); + logger.info(`Uploaded ${result.uploaded} manifests:`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/src/commands/setup-dev.js` around lines 81 - 121, The command currently collects per-manifest failures into result.errors but still prints a success summary and exits normally; update the end of the setup flow (after the uploads loop and before returning) to fail the process when any upload errors were recorded: check result.errors.length (or result.success) and either throw a descriptive Error or call process.exit(1) so callers see a non-zero exit on partial failures; keep the logger usage (logger.info/ logger.error) to print the errors and reference the existing symbols result, UploadResult, uploadToWorker, waitForUploadOnWorker, and logger when implementing the change.
🧹 Nitpick comments (1)
scripts/src/commands/release.js (1)
66-132: Reuse one release command definition.The usage/description/commands object is rebuilt in
parseCommand()and both error paths. Extracting it once will keep the help text from drifting the next time a subcommand changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/src/commands/release.js` around lines 66 - 132, Create a single reusable release command definition object (e.g., const RELEASE_COMMAND = { usage: ` $ ${CLI_NAME} release <prepare|verify|publish>`, description: "...", commands: [...] }) and use it in parseCommand(...) and in both error branches that call printCommandHelp(...) instead of duplicating the literal; update references around parseCommand, parsed.positionals checks, and the Unknown release command branch (which uses VALID_RELEASE_COMMANDS and releaseCommandName) to pass RELEASE_COMMAND to printCommandHelp and to build the parseCommand call so the help text is consistent and maintained in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/bin/ucdjs-scripts.js`:
- Around line 3-4: The static import order is wrong: import
"@ucdjs/moonbeam/register" must run before loading ../src/index.js because
../src/index.js (which imports `#lib/command`) needs the register to be in effect;
change the module to first import or require "@ucdjs/moonbeam/register"
synchronously, then dynamically import("../src/index.js") and destructure the
run export (run) from that dynamic import and invoke it; ensure you remove the
static `import { run } from "../src/index.js"` so the module resolution happens
after registration.
In `@scripts/jsconfig.json`:
- Around line 4-10: The jsconfig include only matches ./src/**/*.js so the new
CLI entrypoint (the string "bin/ucdjs-scripts.js") is not being included for
checkJs; update the "include" array in jsconfig.json to add that entry (or a bin
pattern like "./bin/**/*.js") so the CLI file is covered by semantic checking,
ensuring the include array contains both the existing "./src/**/*.js" and the
new bin path/pattern.
In `@scripts/src/commands/refresh-manifests.js`:
- Around line 103-117: The code calls resolveConfig() unconditionally which
forces --env/--base-url even for --dry-run; change refreshManifests so it checks
dryRun first and only calls resolveConfig (and logs Target: config.baseUrl) when
dryRun is false. Leave dry-run behavior intact (keep the "Dry run mode enabled"
log) and rely on generateManifests' internal API defaults when config is not
resolved; reference the refreshManifests function and the resolveConfig and
generateManifests symbols when making the change.
- Around line 128-150: refreshManifests currently prints the result but doesn't
fail the process when uploads have errors (pushUploadError flips
result.success=false); after calling printResult(result, dryRun) in
refreshManifests (and any other similar command that prints a result), check if
result.success is false and throw an Error (or otherwise exit non‑zero) with a
clear message like "Manifest upload failed" so CI sees the failure; ensure this
change covers the analogous code paths that call
queueUploads/waitForQueuedUploads and printResult so partial failures don't
return success.
- Around line 164-195: The loop calls getRemoteManifestEtag(manifest.version, {
baseUrl, taskKey }) outside the try block so a network rejection will abort
processing all remaining manifests; move the remote ETag lookup into a
per-manifest try/catch (or add a dedicated try around just the
getRemoteManifestEtag call) and on error call pushUploadError(result,
manifest.version, error) (and logger.warn/info as needed) then continue to the
next manifest so only that manifest is recorded as failed while the rest still
get processed; reference getRemoteManifestEtag, createManifestEtag,
normalizeEtag, pushUploadError, and the surrounding per-manifest
loop/queuedUploads logic when making the change.
In `@scripts/src/commands/setup-dev.js`:
- Around line 68-71: The code assumes a fixed origin instead of using the actual
started worker URL returned by unstable_startWorker(); retrieve the runtime URL
by awaiting worker.url (the worker variable returned from unstable_startWorker)
and pass its origin or full URL into the helper calls instead of the hardcoded
"http://127.0.0.1:8787". Update every place that constructs requests to the
local worker (including the other block around the helpers later in the file) to
use await worker.url (or (await worker.url).origin) so the dynamic port/host is
used.
In `@scripts/src/lib/command.js`:
- Around line 83-93: The parsePositiveInteger function accepts partial parses
like "10foo"; change it to validate the full string (e.g., const s =
String(value).trim()) against a digits-only pattern before converting: verify s
matches /^\d+$/ (so non-digit characters cause rejection), then parseInt(s, 10)
and ensure the resulting integer is > 0; keep the existing behavior for
undefined and throw the same Error(`${flagName} must be a positive integer.`)
when validation fails.
In `@scripts/src/lib/logger.js`:
- Around line 29-36: The module currently calls normalizeLevel at import time to
set currentLevel and throws on invalid values, which can crash the CLI before
parsing args; change the initialization so invalid env values are ignored and
default to "info" instead of throwing: either update normalizeLevel to accept a
fallback (or return "info" on unknown levels) or wrap the call that sets
currentLevel in a safe check/try-catch that validates against LOG_LEVELS (using
process.env.UCDJS_LOG_LEVEL ?? process.env.LOG_LEVEL) and assigns "info" when
the value is missing or invalid; keep the normalizeLevel(name) behavior for
valid inputs but ensure module import never throws due to bad env.
In `@scripts/src/lib/manifest.js`:
- Around line 84-110: The exported generateManifests function currently accepts
a batchSize that can be 0 or negative which makes the for loop (index +=
batchSize) never advance; add a guard early in generateManifests to validate
that batchSize is a positive integer (e.g., Number.isInteger(batchSize) &&
batchSize > 0) and either throw a clear Error or coerce to a sane default (e.g.,
1) if invalid; ensure the check happens before the for (let index = 0; index <
versionsToProcess.length; index += batchSize) loop so the function cannot hang
when called programmatically.
---
Outside diff comments:
In `@scripts/src/commands/setup-dev.js`:
- Around line 81-121: The command currently collects per-manifest failures into
result.errors but still prints a success summary and exits normally; update the
end of the setup flow (after the uploads loop and before returning) to fail the
process when any upload errors were recorded: check result.errors.length (or
result.success) and either throw a descriptive Error or call process.exit(1) so
callers see a non-zero exit on partial failures; keep the logger usage
(logger.info/ logger.error) to print the errors and reference the existing
symbols result, UploadResult, uploadToWorker, waitForUploadOnWorker, and logger
when implementing the change.
In `@scripts/src/lib/utils.js`:
- Around line 39-55: getClient's two-slot cache (cachedBaseUrl + cachedClient)
is racy: cachedBaseUrl is set before awaiting createUCDClient, so concurrent
calls for different baseUrls can corrupt the cache. Fix by creating the client
first (await createUCDClient(baseUrl) into a local variable), then atomically
assign cachedClient and cachedBaseUrl only after the client resolves;
alternatively replace the single-slot cache with a Map keyed by baseUrl or use
an in-flight promise map to deduplicate concurrent requests—update getClient,
cachedClient, cachedBaseUrl (or introduce a clientsByBaseUrl / inFlight map)
accordingly so the cache assignment happens after the awaited creation.
---
Nitpick comments:
In `@scripts/src/commands/release.js`:
- Around line 66-132: Create a single reusable release command definition object
(e.g., const RELEASE_COMMAND = { usage: ` $ ${CLI_NAME} release
<prepare|verify|publish>`, description: "...", commands: [...] }) and use it in
parseCommand(...) and in both error branches that call printCommandHelp(...)
instead of duplicating the literal; update references around parseCommand,
parsed.positionals checks, and the Unknown release command branch (which uses
VALID_RELEASE_COMMANDS and releaseCommandName) to pass RELEASE_COMMAND to
printCommandHelp and to build the parseCommand call so the help text is
consistent and maintained in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39f593ed-2220-490c-a19f-e7e96111f6f6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
.github/workflows/refresh-manifest.yaml.github/workflows/release.ymlpackage.jsonpackages/pipelines/pipeline-server/src/client/components/graph/graph-utils.tspackages/ucdjs-scripts/src/commands/refresh-manifests.tspackages/ucdjs-scripts/src/index.tspackages/ucdjs-scripts/src/lib/logger.tspackages/ucdjs-scripts/src/lib/manifest.tspackages/ucdjs-scripts/src/types.tspackages/ucdjs-scripts/tsconfig.build.jsonpackages/ucdjs-scripts/tsconfig.jsonpackages/ucdjs-scripts/tsdown.config.tspnpm-workspace.yamlscripts/bin/ucdjs-scripts.jsscripts/eslint.config.jsscripts/jsconfig.jsonscripts/package.jsonscripts/release.tsscripts/src/commands/refresh-manifests.jsscripts/src/commands/release.jsscripts/src/commands/setup-dev.jsscripts/src/index.jsscripts/src/lib/command.jsscripts/src/lib/config.jsscripts/src/lib/logger.jsscripts/src/lib/manifest.jsscripts/src/lib/upload.jsscripts/src/lib/utils.jsscripts/src/types.jsscripts/turbo.json
💤 Files with no reviewable changes (11)
- .github/workflows/refresh-manifest.yaml
- scripts/turbo.json
- packages/ucdjs-scripts/tsdown.config.ts
- packages/ucdjs-scripts/src/index.ts
- scripts/release.ts
- packages/ucdjs-scripts/src/commands/refresh-manifests.ts
- packages/ucdjs-scripts/src/lib/manifest.ts
- packages/ucdjs-scripts/src/types.ts
- packages/ucdjs-scripts/src/lib/logger.ts
- packages/ucdjs-scripts/tsconfig.build.json
- packages/ucdjs-scripts/tsconfig.json
Comment on lines
+3
to
+4
| import "@ucdjs/moonbeam/register"; | ||
| import { run } from "../src/index.js"; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js ESM, if a module statically imports @ucdjs/moonbeam/registerand../src/index.js, does node:module register()apply before the second static import is resolved, or do you need toawait import() after registering the loader?
💡 Result:
node:module’s register() (and registerHooks()) only affects modules imported after the registration happens. In ESM, all static import statements are resolved/loaded before any code in the module runs, regardless of where the import appears, so registering a loader in the same module will not apply to a sibling static import like ../src/index.js. [1]
So if you need the loader to affect ../src/index.js, you must register first, then await import('../src/index.js') (or ensure the registration runs before app code via node --import …). [1][2]
Register Moonbeam before loading ../src/index.js.
Node.js resolves all static import statements before executing any code in the module. Since @ucdjs/moonbeam/register only affects modules imported after registration runs, the static import of ../src/index.js (which imports #lib/command) will fail. Use a dynamic import after the register step.
Proposed fix
import process from "node:process";
import "@ucdjs/moonbeam/register";
-import { run } from "../src/index.js";
+const { run } = await import("../src/index.js");
run(process.argv.slice(2)).catch((error) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import "@ucdjs/moonbeam/register"; | |
| import { run } from "../src/index.js"; | |
| import "@ucdjs/moonbeam/register"; | |
| const { run } = await import("../src/index.js"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/bin/ucdjs-scripts.js` around lines 3 - 4, The static import order is
wrong: import "@ucdjs/moonbeam/register" must run before loading ../src/index.js
because ../src/index.js (which imports `#lib/command`) needs the register to be in
effect; change the module to first import or require "@ucdjs/moonbeam/register"
synchronously, then dynamically import("../src/index.js") and destructure the
run export (run) from that dynamic import and invoke it; ensure you remove the
static `import { run } from "../src/index.js"` so the module resolution happens
after registration.
Comment on lines
+4
to
+10
| "compilerOptions": { | ||
| "rootDir": ".", | ||
| "maxNodeModuleJsDepth": 0 | ||
| }, | ||
| "include": [ | ||
| "./src/**/*.js" | ||
| ], |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- scripts/jsconfig.json"
sed -n '1,160p' scripts/jsconfig.json
echo
echo "--- base.build candidates"
fd 'base\.build' -t f -HI -x sh -c 'echo ">>> $1"; sed -n "1,200p" "$1"' sh {}Repository: ucdjs/ucd
Length of output: 1932
Include the new CLI entrypoint in the JS project.
bin/ucdjs-scripts.js is the executable used by the new release commands, but it is not covered by the include pattern ./src/**/*.js. checkJs is already enabled in the inherited base config, so semantic checking will work once the include pattern is expanded.
Config change
"include": [
- "./src/**/*.js"
+ "./src/**/*.js",
+ "./bin/**/*.js"
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "compilerOptions": { | |
| "rootDir": ".", | |
| "maxNodeModuleJsDepth": 0 | |
| }, | |
| "include": [ | |
| "./src/**/*.js" | |
| ], | |
| "compilerOptions": { | |
| "rootDir": ".", | |
| "maxNodeModuleJsDepth": 0 | |
| }, | |
| "include": [ | |
| "./src/**/*.js", | |
| "./bin/**/*.js" | |
| ], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/jsconfig.json` around lines 4 - 10, The jsconfig include only matches
./src/**/*.js so the new CLI entrypoint (the string "bin/ucdjs-scripts.js") is
not being included for checkJs; update the "include" array in jsconfig.json to
add that entry (or a bin pattern like "./bin/**/*.js") so the CLI file is
covered by semantic checking, ensuring the include array contains both the
existing "./src/**/*.js" and the new bin path/pattern.
Comment on lines
+103
to
+117
| export async function refreshManifests(options) { | ||
| const versions = parseVersions(options.versions); | ||
| const batchSize = options.batchSize ?? 5; | ||
| const dryRun = options.dryRun ?? false; | ||
|
|
||
| const config = resolveConfig({ | ||
| env: options.env, | ||
| baseUrl: options.baseUrl, | ||
| taskKey: options.taskKey, | ||
| }); | ||
|
|
||
| logger.info(`Target: ${config.baseUrl}`); | ||
| if (dryRun) { | ||
| logger.info("Dry run mode enabled - no changes will be made"); | ||
| } |
Contributor
There was a problem hiding this comment.
Don't require --env or --base-url for dry runs.
resolveConfig() runs before the dry-run branch, so refresh-manifests --dry-run still throws unless an upload target is provided. generateManifests() already has its own API default in scripts/src/lib/manifest.js, so the upload target should only be resolved on the non-dry-run path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/src/commands/refresh-manifests.js` around lines 103 - 117, The code
calls resolveConfig() unconditionally which forces --env/--base-url even for
--dry-run; change refreshManifests so it checks dryRun first and only calls
resolveConfig (and logs Target: config.baseUrl) when dryRun is false. Leave
dry-run behavior intact (keep the "Dry run mode enabled" log) and rely on
generateManifests' internal API defaults when config is not resolved; reference
the refreshManifests function and the resolveConfig and generateManifests
symbols when making the change.
Comment on lines
+128
to
+150
| /** @type {UploadResult} */ | ||
| const result = { | ||
| success: true, | ||
| uploaded: 0, | ||
| skipped: 0, | ||
| errors: [], | ||
| versions: [], | ||
| }; | ||
|
|
||
| if (dryRun) { | ||
| logger.info("Dry run mode: generated manifests only. Skipping upload to tasks endpoint."); | ||
| result.skipped = manifests.length; | ||
| result.versions = manifests.map((manifest) => ({ | ||
| version: manifest.version, | ||
| fileCount: manifest.fileCount, | ||
| })); | ||
| } else { | ||
| const queuedUploads = await queueUploads(manifests, config.baseUrl, config.taskKey, result); | ||
| await waitForQueuedUploads(queuedUploads, config.baseUrl, config.taskKey, result); | ||
| } | ||
|
|
||
| printResult(result, dryRun); | ||
| } |
Contributor
There was a problem hiding this comment.
Fail the command when any manifest upload fails.
pushUploadError() marks result.success = false, but refreshManifests() ignores that flag and returns normally. That lets CI treat partial upload failures as success, and with --log-level warn|error the info-level summary may be hidden as well. Throw after printResult() when !result.success.
🐛 Proposed fix
printResult(result, dryRun);
+ if (!result.success) {
+ throw new Error("One or more manifest uploads failed");
+ }
}Also applies to: 238-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/src/commands/refresh-manifests.js` around lines 128 - 150,
refreshManifests currently prints the result but doesn't fail the process when
uploads have errors (pushUploadError flips result.success=false); after calling
printResult(result, dryRun) in refreshManifests (and any other similar command
that prints a result), check if result.success is false and throw an Error (or
otherwise exit non‑zero) with a clear message like "Manifest upload failed" so
CI sees the failure; ensure this change covers the analogous code paths that
call queueUploads/waitForQueuedUploads and printResult so partial failures don't
return success.
Comment on lines
+164
to
+195
| for (const manifest of manifests) { | ||
| const localEtag = createManifestEtag(manifest.manifest); | ||
| const remoteEtag = await getRemoteManifestEtag(manifest.version, { | ||
| baseUrl, | ||
| taskKey, | ||
| }); | ||
|
|
||
| if (remoteEtag && normalizeEtag(remoteEtag) === normalizeEtag(localEtag)) { | ||
| logger.info(`Skipping ${manifest.version}: no manifest changes detected (${localEtag})`); | ||
| result.skipped += 1; | ||
| continue; | ||
| } | ||
|
|
||
| logger.info(`Preparing manifest tar for ${manifest.version}...`); | ||
| const tar = createManifestTar(manifest); | ||
| logger.info(`Tar archive size for ${manifest.version}: ${tar.byteLength} bytes`); | ||
|
|
||
| try { | ||
| const queued = await uploadManifest(tar, manifest.version, { | ||
| baseUrl, | ||
| taskKey, | ||
| }); | ||
|
|
||
| logger.info(`Queued workflow ${queued.workflowId} for ${manifest.version}`); | ||
|
|
||
| queuedUploads.push({ | ||
| version: manifest.version, | ||
| fileCount: manifest.fileCount, | ||
| workflowId: queued.workflowId, | ||
| }); | ||
| } catch (error) { | ||
| pushUploadError(result, manifest.version, error); |
Contributor
There was a problem hiding this comment.
Catch ETag lookup failures per manifest.
getRemoteManifestEtag() sits outside the current try block, and its fetch() calls in scripts/src/lib/upload.js Lines 117-130 can reject. One transient network error will abort queueUploads() and skip every remaining version, while upload failures are already recorded per-manifest.
🐛 Proposed fix
logger.info("Queueing manifest upload workflows...");
for (const manifest of manifests) {
- const localEtag = createManifestEtag(manifest.manifest);
- const remoteEtag = await getRemoteManifestEtag(manifest.version, {
- baseUrl,
- taskKey,
- });
-
- if (remoteEtag && normalizeEtag(remoteEtag) === normalizeEtag(localEtag)) {
- logger.info(`Skipping ${manifest.version}: no manifest changes detected (${localEtag})`);
- result.skipped += 1;
- continue;
- }
-
- logger.info(`Preparing manifest tar for ${manifest.version}...`);
- const tar = createManifestTar(manifest);
- logger.info(`Tar archive size for ${manifest.version}: ${tar.byteLength} bytes`);
-
try {
+ const localEtag = createManifestEtag(manifest.manifest);
+ const remoteEtag = await getRemoteManifestEtag(manifest.version, {
+ baseUrl,
+ taskKey,
+ });
+
+ if (remoteEtag && normalizeEtag(remoteEtag) === normalizeEtag(localEtag)) {
+ logger.info(`Skipping ${manifest.version}: no manifest changes detected (${localEtag})`);
+ result.skipped += 1;
+ continue;
+ }
+
+ logger.info(`Preparing manifest tar for ${manifest.version}...`);
+ const tar = createManifestTar(manifest);
+ logger.info(`Tar archive size for ${manifest.version}: ${tar.byteLength} bytes`);
+
const queued = await uploadManifest(tar, manifest.version, {
baseUrl,
taskKey,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const manifest of manifests) { | |
| const localEtag = createManifestEtag(manifest.manifest); | |
| const remoteEtag = await getRemoteManifestEtag(manifest.version, { | |
| baseUrl, | |
| taskKey, | |
| }); | |
| if (remoteEtag && normalizeEtag(remoteEtag) === normalizeEtag(localEtag)) { | |
| logger.info(`Skipping ${manifest.version}: no manifest changes detected (${localEtag})`); | |
| result.skipped += 1; | |
| continue; | |
| } | |
| logger.info(`Preparing manifest tar for ${manifest.version}...`); | |
| const tar = createManifestTar(manifest); | |
| logger.info(`Tar archive size for ${manifest.version}: ${tar.byteLength} bytes`); | |
| try { | |
| const queued = await uploadManifest(tar, manifest.version, { | |
| baseUrl, | |
| taskKey, | |
| }); | |
| logger.info(`Queued workflow ${queued.workflowId} for ${manifest.version}`); | |
| queuedUploads.push({ | |
| version: manifest.version, | |
| fileCount: manifest.fileCount, | |
| workflowId: queued.workflowId, | |
| }); | |
| } catch (error) { | |
| pushUploadError(result, manifest.version, error); | |
| for (const manifest of manifests) { | |
| try { | |
| const localEtag = createManifestEtag(manifest.manifest); | |
| const remoteEtag = await getRemoteManifestEtag(manifest.version, { | |
| baseUrl, | |
| taskKey, | |
| }); | |
| if (remoteEtag && normalizeEtag(remoteEtag) === normalizeEtag(localEtag)) { | |
| logger.info(`Skipping ${manifest.version}: no manifest changes detected (${localEtag})`); | |
| result.skipped += 1; | |
| continue; | |
| } | |
| logger.info(`Preparing manifest tar for ${manifest.version}...`); | |
| const tar = createManifestTar(manifest); | |
| logger.info(`Tar archive size for ${manifest.version}: ${tar.byteLength} bytes`); | |
| const queued = await uploadManifest(tar, manifest.version, { | |
| baseUrl, | |
| taskKey, | |
| }); | |
| logger.info(`Queued workflow ${queued.workflowId} for ${manifest.version}`); | |
| queuedUploads.push({ | |
| version: manifest.version, | |
| fileCount: manifest.fileCount, | |
| workflowId: queued.workflowId, | |
| }); | |
| } catch (error) { | |
| pushUploadError(result, manifest.version, error); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/src/commands/refresh-manifests.js` around lines 164 - 195, The loop
calls getRemoteManifestEtag(manifest.version, { baseUrl, taskKey }) outside the
try block so a network rejection will abort processing all remaining manifests;
move the remote ETag lookup into a per-manifest try/catch (or add a dedicated
try around just the getRemoteManifestEtag call) and on error call
pushUploadError(result, manifest.version, error) (and logger.warn/info as
needed) then continue to the next manifest so only that manifest is recorded as
failed while the rest still get processed; reference getRemoteManifestEtag,
createManifestEtag, normalizeEtag, pushUploadError, and the surrounding
per-manifest loop/queuedUploads logic when making the change.
Comment on lines
68
to
71
| const worker = await unstable_startWorker({ | ||
| config: path.join(apiRoot, "./wrangler.jsonc"), | ||
| entrypoint: path.join(apiRoot, "./src/index.ts"), | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Wrangler's current unstable_startWorker API, what fields are returned on the worker instance? Does it expose the started worker URL/port, and can the worker bind to a port other than 8787 when configured or when 8787 is unavailable?
💡 Result:
What does unstable_startWorker() return (worker instance fields)?
In current Wrangler types, the returned worker instance (Worker) exposes: ready, url, inspectorUrl, config, setConfig, patchConfig, fetch, scheduled, queue, dispose(), and raw (the underlying dev environment). [1]
Does it expose the started worker URL/port?
Yes: it exposes url: Promise<URL>, so you can read the full started URL (including hostname + chosen port) from await worker.url. [1]
Can it bind to a port other than 8787 (configured or if 8787 is unavailable)?
- Configured: Yes. You can set
dev.server.port(andhostname/secure, etc.) in the options passed tounstable_startWorker/startWorker, which controls what local address/port the dev server binds to. [1] - If 8787 is unavailable: Wrangler’s types/docs don’t explicitly guarantee an automatic fallback when the default port is taken; the reliable way is to set
dev.server.portyourself and then useawait worker.urlto discover where it actually started. [1][2]
Sources:
[1] Wrangler cli.d.ts (Worker interface + dev.server.port) https://app.unpkg.com/wrangler@4.49.0/files/wrangler-dist/cli.d.ts
[2] Cloudflare docs: “Wrangler’s unstable_startWorker()” (API overview/usage) https://developers.cloudflare.com/workers/testing/unstable_startworker/
Thread the worker's actual URL through the helpers instead of hardcoding a fixed origin.
The worker instance returned from unstable_startWorker() exposes url: Promise<URL>, which provides the started worker's actual URL (including dynamically chosen port if 8787 is unavailable or configured differently). Use await worker.url to fetch this value and pass it to the helpers instead of assuming http://127.0.0.1:8787.
Also applies to: 133-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/src/commands/setup-dev.js` around lines 68 - 71, The code assumes a
fixed origin instead of using the actual started worker URL returned by
unstable_startWorker(); retrieve the runtime URL by awaiting worker.url (the
worker variable returned from unstable_startWorker) and pass its origin or full
URL into the helper calls instead of the hardcoded "http://127.0.0.1:8787".
Update every place that constructs requests to the local worker (including the
other block around the helpers later in the file) to use await worker.url (or
(await worker.url).origin) so the dynamic port/host is used.
Comment on lines
+83
to
+93
| export function parsePositiveInteger(value, flagName) { | ||
| if (value === undefined) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const parsed = Number.parseInt(String(value), 10); | ||
| if (!Number.isInteger(parsed) || parsed <= 0) { | ||
| throw new Error(`${flagName} must be a positive integer.`); | ||
| } | ||
|
|
||
| return parsed; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Number.parseInt behavior partial parse 10foo JavaScript
💡 Result:
Number.parseInt("10foo", 10) returns 10.
Reason: per the spec, parseInt parses from the start of the (trimmed) string and, if it encounters a character that is not a valid digit in the given radix, it stops there and converts only the leading digit substring (here "10"). If the string doesn’t start with any valid digit, it returns NaN. [1]
MDN describes the same “stop at the first invalid character” behavior for Number.parseInt / parseInt. [2]
For comparison, Number("10foo") is NaN because Number(...) requires the entire string to be a valid numeric literal. [2]
Sources:
[1] ECMAScript spec (parseInt algorithm) (tc39.es)
[2] MDN: Number.parseInt() (developer.mozilla.org)
Citations:
- 1: https://tc39.es/ecma262/2025/multipage/global-object.html?utm_source=openai
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/parseInt.?utm_source=openai
🏁 Script executed:
# Search for existing tests or usages of parsePositiveInteger
fd -type f -name "*.js" -o -name "*.json" | xargs rg "parsePositiveInteger" --context=2Repository: ucdjs/ucd
Length of output: 2008
🏁 Script executed:
# Check the file location and surrounding code
cat -n scripts/src/lib/command.js | head -100Repository: ucdjs/ucd
Length of output: 3236
Reject partial parses for numeric flags.
Number.parseInt() accepts leading digit sequences and stops at the first invalid character, so "10foo" parses to 10 instead of throwing an error. Use a full-string digit check before converting.
🛠️ Safer parse
export function parsePositiveInteger(value, flagName) {
if (value === undefined) {
return undefined;
}
- const parsed = Number.parseInt(String(value), 10);
- if (!Number.isInteger(parsed) || parsed <= 0) {
+ const raw = String(value).trim();
+ if (!/^\d+$/.test(raw)) {
+ throw new Error(`${flagName} must be a positive integer.`);
+ }
+ const parsed = Number(raw);
+ if (!Number.isSafeInteger(parsed) || parsed <= 0) {
throw new Error(`${flagName} must be a positive integer.`);
}
return parsed;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/src/lib/command.js` around lines 83 - 93, The parsePositiveInteger
function accepts partial parses like "10foo"; change it to validate the full
string (e.g., const s = String(value).trim()) against a digits-only pattern
before converting: verify s matches /^\d+$/ (so non-digit characters cause
rejection), then parseInt(s, 10) and ensure the resulting integer is > 0; keep
the existing behavior for undefined and throw the same Error(`${flagName} must
be a positive integer.`) when validation fails.
Comment on lines
+29
to
+36
| function normalizeLevel(level) { | ||
| const normalized = /** @type {LogLevel} */ ((level ?? "info").toLowerCase()); | ||
| if (normalized in LOG_LEVELS) return normalized; | ||
| throw new Error("Invalid log level. Use debug|info|warn|error."); | ||
| } | ||
|
|
||
| /** @type {LogLevel} */ | ||
| let currentLevel = normalizeLevel(process.env.UCDJS_LOG_LEVEL ?? process.env.LOG_LEVEL ?? "info"); |
Contributor
There was a problem hiding this comment.
Don't let an invalid LOG_LEVEL crash the CLI at import time.
currentLevel is initialized during module load, so any unsupported process.env.LOG_LEVEL value throws before argument parsing, even for --help. If you want the generic env fallback, ignore invalid values here and fall back to "info" instead of aborting the whole script.
🐛 Proposed fix
function normalizeLevel(level) {
const normalized = /** `@type` {LogLevel} */ ((level ?? "info").toLowerCase());
if (normalized in LOG_LEVELS) return normalized;
throw new Error("Invalid log level. Use debug|info|warn|error.");
}
+function resolveInitialLevel() {
+ const configured = process.env.UCDJS_LOG_LEVEL ?? process.env.LOG_LEVEL;
+ try {
+ return normalizeLevel(configured);
+ } catch {
+ return "info";
+ }
+}
+
/** `@type` {LogLevel} */
-let currentLevel = normalizeLevel(process.env.UCDJS_LOG_LEVEL ?? process.env.LOG_LEVEL ?? "info");
+let currentLevel = resolveInitialLevel();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/src/lib/logger.js` around lines 29 - 36, The module currently calls
normalizeLevel at import time to set currentLevel and throws on invalid values,
which can crash the CLI before parsing args; change the initialization so
invalid env values are ignored and default to "info" instead of throwing: either
update normalizeLevel to accept a fallback (or return "info" on unknown levels)
or wrap the call that sets currentLevel in a safe check/try-catch that validates
against LOG_LEVELS (using process.env.UCDJS_LOG_LEVEL ?? process.env.LOG_LEVEL)
and assigns "info" when the value is missing or invalid; keep the
normalizeLevel(name) behavior for valid inputs but ensure module import never
throws due to bad env.
Comment on lines
+84
to
+110
| export async function generateManifests(options = {}) { | ||
| const { | ||
| versions: inputVersions, | ||
| apiBaseUrl = "https://api.ucdjs.dev", | ||
| batchSize = 5, | ||
| } = options; | ||
|
|
||
| const client = await getClient(apiBaseUrl); | ||
|
|
||
| /** @type {Array<{ version: string }>} */ | ||
| let versionsToProcess; | ||
|
|
||
| if (inputVersions && inputVersions.length > 0) { | ||
| versionsToProcess = inputVersions.map((version) => ({ version })); | ||
| } else { | ||
| logger.info(`Fetching versions from ${apiBaseUrl}...`); | ||
| const allVersions = unwrap(await client.versions.list()); | ||
| versionsToProcess = allVersions.map((version) => ({ version: version.version })); | ||
| logger.info(`Found ${versionsToProcess.length} versions to process`); | ||
| } | ||
|
|
||
| /** @type {GeneratedManifest[]} */ | ||
| const results = []; | ||
| /** @type {Map<string, ExpectedFile[]>} */ | ||
| const fileCache = new Map(); | ||
|
|
||
| for (let index = 0; index < versionsToProcess.length; index += batchSize) { |
Contributor
There was a problem hiding this comment.
Validate batchSize before entering the loop.
index += batchSize on Line 110 never advances for 0 or negative values, so this new public API can hang the process even though the CLI path validates --batch-size. Guard the exported function as well.
🐛 Proposed fix
export async function generateManifests(options = {}) {
const {
versions: inputVersions,
apiBaseUrl = "https://api.ucdjs.dev",
batchSize = 5,
} = options;
+
+ if (!Number.isInteger(batchSize) || batchSize < 1) {
+ throw new Error("batchSize must be a positive integer");
+ }
const client = await getClient(apiBaseUrl);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/src/lib/manifest.js` around lines 84 - 110, The exported
generateManifests function currently accepts a batchSize that can be 0 or
negative which makes the for loop (index += batchSize) never advance; add a
guard early in generateManifests to validate that batchSize is a positive
integer (e.g., Number.isInteger(batchSize) && batchSize > 0) and either throw a
clear Error or coerce to a sane default (e.g., 1) if invalid; ensure the check
happens before the for (let index = 0; index < versionsToProcess.length; index
+= batchSize) loop so the function cannot hang when called programmatically.
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
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.
Summary by CodeRabbit
New Features
Refactor
Chores