feat(multipublish): add jsr auth token support and token environment key#224
Conversation
🦋 Changeset detectedLatest commit: c35993e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @stephansama, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly extends the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for publishing to JSR with authentication tokens, enhancing the multipublish tool's capabilities. It includes updates to the configuration schema and the publishing logic to handle JSR-specific options and token retrieval. While the feature implementation is generally sound, I've identified a high-severity security concern related to how the authentication token is passed to the jsr publish command, which could lead to command injection. Additionally, a change in error handling from throwing an error to logging and returning might mask critical failures, which is a medium-severity issue.
| isDryRun && "--dry-run", | ||
| config.allowSlowTypes && "--allow-slow-types", | ||
| isDryRun && "--dry-run", | ||
| authToken && `--token ${authToken}`, |
There was a problem hiding this comment.
Directly interpolating authToken into the command string can lead to a command injection vulnerability if the token contains shell metacharacters. It is generally safer to pass sensitive information like API tokens via environment variables to the child process. The jsr publish command supports reading the token from the JSR_API_TOKEN environment variable. Consider passing the token via the env option of cp.execSync instead of as a command-line argument.
| return console.error( | ||
| "scope does not start with @ symbol. aborting.", | ||
| ); |
There was a problem hiding this comment.
Changing from throw new Error to return console.error alters the error handling behavior. Throwing an error provides a clearer indication of a critical failure and propagates the error up the call stack, which can be important for robust error management and debugging. Returning after logging an error might lead to silent failures that are harder to detect and diagnose.
throw new Error("scope must start with `@` symbol");
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds JSR platform support and token handling: new Changes
Sequence Diagram(s)sequenceDiagram
participant GH as "GitHub Actions (release workflow)"
participant ENV as "Secrets / Environment"
participant MP as "multipublish (publish.ts)"
participant JSR as "JSR Registry/CLI"
GH->>ENV: read JSR_AUTH_TOKEN secret
GH->>MP: run publish job (env includes token)
MP->>MP: detect platform == "jsr" and read tokenEnvironmentKey
MP->>MP: build publish command args (include --token <token> if present)
MP->>JSR: invoke JSR publish command with args
JSR-->>MP: respond success/failure
MP-->>GH: return job exit status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Line 13: Remove the top-level exposure of JSR_AUTH_TOKEN: since the workflow
already sets id-token: write for OIDC, drop JSR_AUTH_TOKEN from the global env
or move it into the specific release job's env; update the env declaration to
remove JSR_AUTH_TOKEN at the workflow level and add it only to the release job
(where publishing occurs) so test and other jobs no longer receive the secret,
keeping behavior for non-OIDC/local runs intact by scoping the secret to the
release job.
In `@core/multipublish/src/publish.ts`:
- Line 89: The npm branch in core/multipublish/src/publish.ts uses an untyped
filter `.filter((x) => x)` which is inconsistent with the JSR path's type guard
`.filter((x): x is string => !!x)`; change the npm-side filter to a proper type
guard (e.g., `.filter((x): x is string => !!x)`) so downstream code that expects
strings has correct typings—locate the npm path where `.filter((x) => x)` is
used and replace it with the typed predicate.
- Around line 126-129: The current change replaces a thrown error with `return
console.error(...)`, which causes the surrounding async function to resolve
instead of reject and silently swallow failures; restore proper error
propagation by replacing `return console.error("scope does not start with @
symbol. aborting.")` with `throw new Error("scope does not start with @ symbol.
aborting.")` (or, if you intentionally want a non-fatal best-effort publish,
explicitly log the message and return a documented no-op result or an explicit
rejected Promise so callers can distinguish behavior); update the code path in
the publish routine where the scope check occurs to either rethrow the Error or
make the silent continuation explicit and documented.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.config/.multipublishrc.json.github/workflows/release.ymlcore/multipublish/src/publish.tscore/multipublish/src/schema.ts
| ZX_VERBOSE: true | ||
| DO_NOT_TRACK: 1 | ||
| NODE_AUTH_TOKEN: ${{secrets.NODE_AUTH_TOKEN}} | ||
| JSR_AUTH_TOKEN: ${{secrets.JSR_AUTH_TOKEN}} |
There was a problem hiding this comment.
JSR_AUTH_TOKEN is exposed to all jobs and is redundant when OIDC is configured.
Two observations:
-
id-token: writeis already set at the workflow level, which is the permission JSR uses for OIDC-based authentication in GitHub Actions. This means the--tokenargument will never be needed for this specific workflow's CI runs — JSR authenticates automatically via OIDC, makingJSR_AUTH_TOKENunused in practice here (the env var will be undefined unless the secret is explicitly set, and the code handles that gracefully). The secret is useful for non-OIDC environments (e.g., local dev), but that context doesn't apply to this workflow file. -
Setting
JSR_AUTH_TOKENat the top-levelenvblock exposes it to thetestjob, which has no need for it. Scoping it to thereleasejob (matching where publishing actually occurs) would be more aligned with least-privilege, though this is consistent with the existing pattern forNODE_AUTH_TOKENand other tokens in this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml at line 13, Remove the top-level exposure of
JSR_AUTH_TOKEN: since the workflow already sets id-token: write for OIDC, drop
JSR_AUTH_TOKEN from the global env or move it into the specific release job's
env; update the env declaration to remove JSR_AUTH_TOKEN at the workflow level
and add it only to the release job (where publishing occurs) so test and other
jobs no longer receive the secret, keeping behavior for non-OIDC/local runs
intact by scoping the secret to the release job.
| authToken && `--token ${authToken}`, | ||
| ] | ||
| .filter((x) => x) | ||
| .filter((x): x is string => !!x) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Filter type guard inconsistency with the npm path.
The JSR path uses a proper type guard .filter((x): x is string => !!x), while the npm path at Line 160 still uses the untyped .filter((x) => x). Consider applying the same type-safe filter there for consistency.
♻️ Proposed fix
// Line 160 (npm path)
- .filter((x) => x)
+ .filter((x): x is string => !!x)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/multipublish/src/publish.ts` at line 89, The npm branch in
core/multipublish/src/publish.ts uses an untyped filter `.filter((x) => x)`
which is inconsistent with the JSR path's type guard `.filter((x): x is string
=> !!x)`; change the npm-side filter to a proper type guard (e.g., `.filter((x):
x is string => !!x)`) so downstream code that expects strings has correct
typings—locate the npm path where `.filter((x) => x)` is used and replace it
with the typed predicate.
| return console.error( | ||
| "scope does not start with @ symbol. aborting.", | ||
| ); | ||
| } |
There was a problem hiding this comment.
return console.error(...) silently swallows the failure — callers receive a resolved Promise<void> instead of a rejection.
console.error returns undefined. So return console.error(...) is equivalent to calling console.error(...) and then return;, which resolves the enclosing async function's promise successfully. The pre-existing throw new Error(...) would have rejected the promise and surfaced the failure to callers. With this change, if a package name has no @ scope, the publish is silently skipped with no observable error propagation — callers have no way to distinguish this from a successful publish.
🐛 Proposed fix
- return console.error(
- "scope does not start with @ symbol. aborting.",
- );
+ console.error("scope does not start with @ symbol. aborting.");
+ throw new Error("scope does not start with @ symbol. aborting.");Or, if silent continuation is intentional (best-effort multi-package publish), document this explicitly and ensure the caller accounts for no-op returns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/multipublish/src/publish.ts` around lines 126 - 129, The current change
replaces a thrown error with `return console.error(...)`, which causes the
surrounding async function to resolve instead of reject and silently swallow
failures; restore proper error propagation by replacing `return
console.error("scope does not start with @ symbol. aborting.")` with `throw new
Error("scope does not start with @ symbol. aborting.")` (or, if you
intentionally want a non-fatal best-effort publish, explicitly log the message
and return a documented no-op result or an explicit rejected Promise so callers
can distinguish behavior); update the code path in the publish routine where the
scope check occurs to either rethrow the Error or make the silent continuation
explicit and documented.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/multipublish/src/schema.test.ts (1)
13-39: 🧹 Nitpick | 🔵 TrivialConsider adding a test for the custom
tokenEnvironmentKeyoverride path.The existing test only exercises the default value path. Adding one case that explicitly provides
tokenEnvironmentKeyfor JSR would confirm the field is overridable and guard against accidental schema constraints being introduced in the future.✅ Suggested additional test case
it("should accept platform tuples with options", () => { // ... existing test ... }); + it("should accept a custom tokenEnvironmentKey for jsr", () => { + const input = [["jsr", { tokenEnvironmentKey: "MY_JSR_TOKEN" }]]; + const result = platformsSchema.parse(input); + expect(result).toEqual([ + [ + "jsr", + { + allowSlowTypes: false, + experimentalGenerateJSR: false, + experimentalUpdateCatalogs: false, + tokenEnvironmentKey: "MY_JSR_TOKEN", + }, + ], + ]); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/multipublish/src/schema.test.ts` around lines 13 - 39, Add a test case to the "should accept platform tuples with options" spec that includes an explicit tokenEnvironmentKey override (e.g., for "jsr") to ensure platformsSchema accepts user-provided tokenEnvironmentKey values; update the input tuple to include a third option object for "jsr" with tokenEnvironmentKey set to a custom string and assert the parsed result contains that custom tokenEnvironmentKey rather than the default in the expected array, referencing platformsSchema.parse and the existing test block to place the new case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/clever-rings-cheer.md:
- Line 5: Reword the changeset summary to clearly state that the package publish
step for unscoped packages on the NPM publish path logs an error but returns
without throwing (causing CI/workflows to continue as successful); reference
that this silent-failure occurs where the NPM publish handler uses
console.error() and returns (publish.ts NPM publish handler), and explicitly
call out the affected platform as "npm publishing" (or "npm registry publish")
and that an auth token was added to jsr.
---
Outside diff comments:
In `@core/multipublish/src/schema.test.ts`:
- Around line 13-39: Add a test case to the "should accept platform tuples with
options" spec that includes an explicit tokenEnvironmentKey override (e.g., for
"jsr") to ensure platformsSchema accepts user-provided tokenEnvironmentKey
values; update the input tuple to include a third option object for "jsr" with
tokenEnvironmentKey set to a custom string and assert the parsed result contains
that custom tokenEnvironmentKey rather than the default in the expected array,
referencing platformsSchema.parse and the existing test block to place the new
case.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/clever-rings-cheer.mdcore/multipublish/src/schema.test.ts
| "@stephansama/multipublish": patch | ||
| --- | ||
|
|
||
| Updated jsr to have auth token. fail without error for unscoped packages |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for JSR publish-related code and unscoped package handling
rg -n "unscoped|jsr.*publish|publish.*jsr|tokenEnvironmentKey|JSR_AUTH_TOKEN" --type ts --type js -C 3Repository: stephansama/packages
Length of output: 4830
🏁 Script executed:
#!/bin/bash
# Read the publish.ts file to see the full JSR publish logic
head -120 core/multipublish/src/publish.ts | tail -60Repository: stephansama/packages
Length of output: 1711
🏁 Script executed:
#!/bin/bash
# Read the JSR-specific logic to see if there's validation for unscoped packages
sed -n '50,80p' core/multipublish/src/publish.tsRepository: stephansama/packages
Length of output: 1019
🏁 Script executed:
#!/bin/bash
# Look for JSR module and check for scope/unscoped package handling
find . -name "*jsr*" -type f | head -20Repository: stephansama/packages
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Read the JSR module to check for unscoped package handling and warnings
cat -n core/multipublish/src/jsr.tsRepository: stephansama/packages
Length of output: 3811
🏁 Script executed:
#!/bin/bash
# Check the JSR publish test to understand expected behavior
cat -n core/multipublish/src/publish.test.ts | sed -n '153,200p'Repository: stephansama/packages
Length of output: 1029
🏁 Script executed:
#!/bin/bash
# Check if there are any tests or logic for unscoped packages
rg -n "scope" core/multipublish/src/jsr.ts core/multipublish/src/publish.ts -C 2Repository: stephansama/packages
Length of output: 867
🏁 Script executed:
#!/bin/bash
# Get more context around the npm scope validation and check if JSR has similar logic
sed -n '40,90p' core/multipublish/src/publish.tsRepository: stephansama/packages
Length of output: 1613
🏁 Script executed:
#!/bin/bash
# Check if there's any validation for unscoped packages in JSR publish, and compare with NPM
sed -n '44,95p' core/multipublish/src/publish.tsRepository: stephansama/packages
Length of output: 1521
🏁 Script executed:
#!/bin/bash
# Get the complete NPM scope validation and error handling
sed -n '110,135p' core/multipublish/src/publish.tsRepository: stephansama/packages
Length of output: 726
Clarify the changelog description; silent-failure behavior for unscoped packages is confirmed in the NPM publish path.
The phrase "fail without error for unscoped packages" is grammatically awkward and ambiguous. More importantly, this silent-failure behavior is confirmed in the codebase at the NPM publish handler (lines 125–129 of publish.ts): when a package is unscoped, console.error() logs a diagnostic but the function returns without throwing, allowing the CI/workflow to continue appearing successful.
Reword for clarity and specify which platform(s) this applies to:
-Updated jsr to have auth token. fail without error for unscoped packages
+Updated NPM and JSR publish to support auth tokens via configurable environment keys. Note: NPM publishing fails silently for unscoped packages (logs error but does not throw).📝 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.
| Updated jsr to have auth token. fail without error for unscoped packages | |
| --- | |
| `@stephansama/multipublish`: patch | |
| --- | |
| Updated NPM and JSR publish to support auth tokens via configurable environment keys. Note: NPM publishing fails silently for unscoped packages (logs error but does not throw). |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.changeset/clever-rings-cheer.md at line 5, Reword the changeset summary to
clearly state that the package publish step for unscoped packages on the NPM
publish path logs an error but returns without throwing (causing CI/workflows to
continue as successful); reference that this silent-failure occurs where the NPM
publish handler uses console.error() and returns (publish.ts NPM publish
handler), and explicitly call out the affected platform as "npm publishing" (or
"npm registry publish") and that an auth token was added to jsr.
More templates
@stephansama/ai-commit-msg
@stephansama/alfred-kaomoji
@stephansama/astro-iconify-svgmap
@stephansama/auto-readme
@stephansama/catppuccin-opml
@stephansama/catppuccin-rss
@stephansama/catppuccin-typedoc
@stephansama/catppuccin-xsl
create-stephansama-example
@stephansama/find-makefile-targets
@stephansama/github-env
@stephansama/multipublish
@stephansama/prettier-plugin-handlebars
@stephansama/remark-asciinema
@stephansama/svelte-social-share-links
@stephansama/typed-env
@stephansama/typed-events
@stephansama/typed-nocodb-api
@stephansama/typed-templates
@stephansama/types-lhci
commit: |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/multipublish/src/jsr.test.ts (1)
70-77: 🧹 Nitpick | 🔵 TrivialTrailing comma missing on line 76.
Line 53 uses a trailing comma after
tokenEnvironmentKey: "test",; line 76 does not. Minor inconsistency worth aligning.🔧 Suggested fix
- tokenEnvironmentKey: "test" + tokenEnvironmentKey: "test",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/multipublish/src/jsr.test.ts` around lines 70 - 77, Update the object literal assigned to appConfig (type JsrPlatformOptionsSchema) so that the final property tokenEnvironmentKey: "test" includes a trailing comma to match the rest of the file's style; modify the appConfig declaration in jsr.test.ts (the object with allowSlowTypes, defaultExclude, defaultInclude, experimentalGenerateJSR, experimentalUpdateCatalogs, tokenEnvironmentKey) to add the trailing comma after tokenEnvironmentKey.core/multipublish/src/publish.test.ts (1)
153-186: 🧹 Nitpick | 🔵 TrivialExtract shared JSR test setup to eliminate duplication.
Both test cases share identical
loadConfigmock andpkgdefinitions (lines 155–171 vs. 192–210). Extract them into a shared setup to reduce noise.♻️ Suggested refactor
+ describe("publishPlatform jsr", () => { + const pkg = { + dir: "/path/to/pkg", + packageJson: { name: "@scope/pkg", version: "1.0.0" }, + relativeDir: "./pkg", + }; + + beforeEach(() => { + vi.mocked(mocks.loadConfig).mockResolvedValue({ + config: { exports: "index.ts", name: "@scope/pkg", version: "1.0.0" }, + filename: "/path/to/pkg/jsr.json", + }); + }); - it("should publish to jsr without token", async () => { - vi.stubEnv("JSR_AUTH_TOKEN", ""); - vi.mocked(mocks.loadConfig).mockResolvedValue({ ... }); - const pkg = { ... }; - const platform = "jsr"; - await publishPlatform(pkg, platform); + it("should publish to jsr without token", async () => { + vi.stubEnv("JSR_AUTH_TOKEN", ""); + await publishPlatform(pkg, "jsr"); expect(mocks.writeFile).toHaveBeenCalledWith("/path/to/pkg/jsr.json", expect.any(String)); expect(mocks.execSync).toHaveBeenCalledWith( "pnpm dlx jsr publish --allow-dirty --allow-slow-types", { stdio: "inherit" }, ); }); - it("should publish to jsr with token", async () => { - const token = "test-token"; - vi.stubEnv("JSR_AUTH_TOKEN", token); - vi.mocked(mocks.loadConfig).mockResolvedValue({ ... }); - const pkg = { ... }; - const platform = "jsr"; - await publishPlatform(pkg, platform); + it("should publish to jsr with token", async () => { + const token = "test-token"; + vi.stubEnv("JSR_AUTH_TOKEN", token); + await publishPlatform(pkg, "jsr"); expect(mocks.writeFile).toHaveBeenCalledWith("/path/to/pkg/jsr.json", expect.any(String)); expect(mocks.execSync).toHaveBeenCalledWith( `pnpm dlx jsr publish --allow-dirty --allow-slow-types --token ${token}`, { stdio: "inherit" }, ); }); + });Also applies to: 188-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/multipublish/src/publish.test.ts` around lines 153 - 186, Extract the duplicated test setup for the JSR tests by moving the repeated mocks.loadConfig.mockResolvedValue payload and the pkg object into a shared helper or a beforeEach so both "should publish to jsr without token" and the other JSR test reuse it; keep per-test vi.stubEnv calls local, and update each test to reference the shared pkg and not redeclare loadConfig or pkg; ensure publishPlatform calls and assertions against mocks.writeFile and mocks.execSync remain unchanged and still reference the same expected values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/multipublish/src/publish.test.ts`:
- Around line 188-223: The test shows publish.ts interpolates JSR_AUTH_TOKEN
directly into an execSync shell string (in publishPlatform), which risks shell
injection; change publishPlatform to call child_process.execFileSync("pnpm",
args, { stdio: "inherit" }) instead of execSync by building an argv array like
["dlx","jsr","publish","--allow-dirty","--allow-slow-types", "--token", token]
only when token exists, and update the test to expect mocks.execFileSync with
the same args/stdio call; ensure the unique symbol publishPlatform (in
publish.ts) is updated and any mocks use execFileSync instead of execSync.
In `@vitest.config.ts`:
- Line 10: The junit report path was changed in the vitest config (see
outputFile: { junit: "./test-report.junit.xml" }) but you must also 1) add the
new root-level filename test-report.junit.xml to .gitignore so it isn't
committed, and 2) update the codecov/test-results-action invocation in
.github/workflows/release.yml to point to "./test-report.junit.xml" instead of
"./coverage/test-report.junit.xml"; make both edits so the produced report is
ignored locally and CI uploads the correct file.
---
Outside diff comments:
In `@core/multipublish/src/jsr.test.ts`:
- Around line 70-77: Update the object literal assigned to appConfig (type
JsrPlatformOptionsSchema) so that the final property tokenEnvironmentKey: "test"
includes a trailing comma to match the rest of the file's style; modify the
appConfig declaration in jsr.test.ts (the object with allowSlowTypes,
defaultExclude, defaultInclude, experimentalGenerateJSR,
experimentalUpdateCatalogs, tokenEnvironmentKey) to add the trailing comma after
tokenEnvironmentKey.
In `@core/multipublish/src/publish.test.ts`:
- Around line 153-186: Extract the duplicated test setup for the JSR tests by
moving the repeated mocks.loadConfig.mockResolvedValue payload and the pkg
object into a shared helper or a beforeEach so both "should publish to jsr
without token" and the other JSR test reuse it; keep per-test vi.stubEnv calls
local, and update each test to reference the shared pkg and not redeclare
loadConfig or pkg; ensure publishPlatform calls and assertions against
mocks.writeFile and mocks.execSync remain unchanged and still reference the same
expected values.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
core/multipublish/src/jsr.test.tscore/multipublish/src/publish.test.tspackage.jsonpnpm-workspace.yamlvitest.config.ts
| it("should publish to jsr with token", async () => { | ||
| const token = "test-token"; | ||
| vi.stubEnv("JSR_AUTH_TOKEN", token); | ||
|
|
||
| vi.mocked(mocks.loadConfig).mockResolvedValue({ | ||
| config: { | ||
| exports: "index.ts", | ||
| name: "@scope/pkg", | ||
| version: "1.0.0", | ||
| }, | ||
| filename: "/path/to/pkg/jsr.json", | ||
| }); | ||
|
|
||
| const pkg = { | ||
| dir: "/path/to/pkg", | ||
| packageJson: { | ||
| name: "@scope/pkg", | ||
| version: "1.0.0", | ||
| }, | ||
| relativeDir: "./pkg", | ||
| }; | ||
|
|
||
| const platform = "jsr"; | ||
|
|
||
| await publishPlatform(pkg, platform); | ||
|
|
||
| expect(mocks.writeFile).toHaveBeenCalledWith( | ||
| "/path/to/pkg/jsr.json", | ||
| expect.any(String), | ||
| ); | ||
|
|
||
| expect(mocks.execSync).toHaveBeenCalledWith( | ||
| `pnpm dlx jsr publish --allow-dirty --allow-slow-types --token ${token}`, | ||
| { stdio: "inherit" }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Token interpolated directly into shell command in publish.ts — potential shell injection.
The test at line 220 confirms publish.ts builds the execSync string by raw interpolation:
pnpm dlx jsr publish --allow-dirty --allow-slow-types --token ${token}
execSync spawns a subshell; if JSR_AUTH_TOKEN ever contains shell metacharacters (spaces, ;, &&, $(...), etc.), it will misbehave or inject commands. While CI-provisioned tokens are typically alphanumeric, the implementation in publish.ts provides no sanitisation or quoting.
Consider switching to execFileSync with an explicit argv array in publish.ts, which bypasses the shell entirely:
// In publish.ts (not in the test file)
const args = ["dlx", "jsr", "publish", "--allow-dirty", "--allow-slow-types"];
if (token) args.push("--token", token);
cp.execFileSync("pnpm", args, { stdio: "inherit" });The test assertion would then change to match execFileSync instead of execSync.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/multipublish/src/publish.test.ts` around lines 188 - 223, The test shows
publish.ts interpolates JSR_AUTH_TOKEN directly into an execSync shell string
(in publishPlatform), which risks shell injection; change publishPlatform to
call child_process.execFileSync("pnpm", args, { stdio: "inherit" }) instead of
execSync by building an argv array like
["dlx","jsr","publish","--allow-dirty","--allow-slow-types", "--token", token]
only when token exists, and update the test to expect mocks.execFileSync with
the same args/stdio call; ensure the unique symbol publishPlatform (in
publish.ts) is updated and any mocks use execFileSync instead of execSync.
| reporter: ["html", "json", "text"], | ||
| }, | ||
| outputFile: { junit: "./coverage/test-report.junit.xml" }, | ||
| outputFile: { junit: "./test-report.junit.xml" }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Check if test-report.junit.xml at root is covered by .gitignore
echo "=== Checking .gitignore for test-report patterns ==="
if [ -f .gitignore ]; then
rg -n "test-report" .gitignore || echo "No test-report entry found in .gitignore"
else
echo ".gitignore not found"
fi
echo ""
echo "=== Checking for broad patterns that might cover root-level test files ==="
rg -n "^\*\.\(xml\|junit\)" .gitignore || echo "No broad XML/junit patterns found"
echo ""
echo "=== Full .gitignore content for context ==="
cat .gitignore
echo ""
echo "=== Checking CI workflow files for junit path references ==="
if [ -d .github/workflows ]; then
rg -n "test-report\|junit\|coverage" .github/workflows/ || echo "No junit/coverage references found in CI workflows"
else
echo ".github/workflows directory not found"
fiRepository: stephansama/packages
Length of output: 850
🏁 Script executed:
#!/bin/bash
# Check for other CI system config files
echo "=== Searching for CI configuration files ==="
fd -type f -name ".gitlab-ci.yml" -o -name "*.circleci*" -o -name "bitbucket-pipelines.yml" -o -name ".travis.yml" 2>/dev/null || echo "No other CI configs found via fd"
echo ""
echo "=== Searching for any 'test' or 'junit' references in .github/ recursively ==="
if [ -d .github ]; then
find .github -type f | head -20
echo ""
rg -i "test|junit|artifact" .github/ || echo "No artifact/test references in .github"
else
echo ".github directory not found"
fi
echo ""
echo "=== Checking if .github/workflows exists and listing files ==="
if [ -d .github/workflows ]; then
ls -la .github/workflows/
else
echo ".github/workflows not found"
fi
echo ""
echo "=== Broader search for 'test-report' in entire repo (excluding node_modules) ==="
rg -i "test-report\|junit\.xml" --glob='!node_modules' . 2>/dev/null | head -20Repository: stephansama/packages
Length of output: 1309
Update .gitignore and CI workflow for new junit report location.
The junit report path change from ./coverage/test-report.junit.xml to ./test-report.junit.xml requires two fixes:
- Add
test-report.junit.xmlto.gitignore— the new root-level file is not currently ignored (only**/coverage/is covered). - Update
.github/workflows/release.yml— the codecov/test-results-action step still references the old path ("./coverage/test-report.junit.xml") and must be changed to"./test-report.junit.xml"to match the new output location.
Without these changes, the junit report will be committed to the repo and the CI test upload will fail.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vitest.config.ts` at line 10, The junit report path was changed in the vitest
config (see outputFile: { junit: "./test-report.junit.xml" }) but you must also
1) add the new root-level filename test-report.junit.xml to .gitignore so it
isn't committed, and 2) update the codecov/test-results-action invocation in
.github/workflows/release.yml to point to "./test-report.junit.xml" instead of
"./coverage/test-report.junit.xml"; make both edits so the produced report is
ignored locally and CI uploads the correct file.
1 participant
Checklist
mainhave been mergedmain