fix(release): make tag workflow the single npm publisher - #20
Conversation
📝 WalkthroughWalkthroughThe release configuration disables local npm publishing and removes the hardcoded Discord webhook fallback. Unit tests verify publishing ownership and confirm that webhook credentials are not embedded in the configuration. ChangesRelease configuration
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (2)
packages/cli/tests/unit/scripts/release-config.test.ts (2)
25-32: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTest the runtime
DISCORD_WEBHOOK_URLcontract.Line 31 only checks that one URL prefix is absent from source. It does not verify that
releaseConfig.notifications.discord.webhookUrlresolves fromprocess.env.DISCORD_WEBHOOK_URL. A regression toundefinedwould still pass and the notifier would silently skip. Set a fake environment value before importingrelease.config.js, assert the resolved property, and restore the environment.🤖 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/cli/tests/unit/scripts/release-config.test.ts` around lines 25 - 32, Update the release-config test around the existing source scan to set a fake process.env.DISCORD_WEBHOOK_URL before loading release.config.js, assert that releaseConfig.notifications.discord.webhookUrl equals that value, and restore the environment afterward, ensuring module import occurs only after the variable is configured.
19-22: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftVerify exclusive npm ownership.
Lines 19-22 verify that local npm publishing is disabled and that one workflow contains
npm publish. They do not verify that no other release script or workflow publishes npm. A second publisher could be added without failing this test. Add an exclusivity check or an integration test for the tag-triggered release path.As per coding guidelines, tests should cover unit tests, integration tests, and end-to-end user workflows.
🤖 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/cli/tests/unit/scripts/release-config.test.ts` around lines 19 - 22, Extend the release configuration tests around releaseConfig and publishWorkflow to verify exclusive npm ownership: confirm the local npm publish setting remains disabled, the tag-triggered workflow performs npm publishing, and no other release script or workflow contains an npm publish command. Cover the tag-triggered release path with an integration or end-to-end assertion if the unit fixtures cannot establish exclusivity.Source: Coding guidelines
🤖 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/cli/tests/unit/scripts/release-config.test.ts`:
- Line 21: Update the publishWorkflow assertion in release-config.test.ts to use
a single-quoted TypeScript string, escaping the embedded YAML quotes while
preserving the expected "- 'v*.*.*'" content; keep the statement
semicolon-terminated and within the configured line width.
---
Nitpick comments:
In `@packages/cli/tests/unit/scripts/release-config.test.ts`:
- Around line 25-32: Update the release-config test around the existing source
scan to set a fake process.env.DISCORD_WEBHOOK_URL before loading
release.config.js, assert that releaseConfig.notifications.discord.webhookUrl
equals that value, and restore the environment afterward, ensuring module import
occurs only after the variable is configured.
- Around line 19-22: Extend the release configuration tests around releaseConfig
and publishWorkflow to verify exclusive npm ownership: confirm the local npm
publish setting remains disabled, the tag-triggered workflow performs npm
publishing, and no other release script or workflow contains an npm publish
command. Cover the tag-triggered release path with an integration or end-to-end
assertion if the unit fixtures cannot establish exclusivity.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 642adb0f-e5fe-43a5-9898-6c6e9fac0433
📒 Files selected for processing (2)
packages/cli/release.config.jspackages/cli/tests/unit/scripts/release-config.test.ts
|
Addressed the valid review suggestions in |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/tests/unit/scripts/release-config.test.ts`:
- Around line 37-48: Update the `loads notification credentials only from the
environment` test to first remove `DISCORD_WEBHOOK_URL`, load the configuration
with a distinct cache key, and assert
`releaseConfig.notifications.discord.webhookUrl` is undefined. Then restore the
environment value, load the configured case, and retain the existing assertion
that it is selected.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3fb90862-1812-45c8-9336-6fdaa7e471af
📒 Files selected for processing (1)
packages/cli/tests/unit/scripts/release-config.test.ts
| it('loads notification credentials only from the environment', async () => { | ||
| const previousWebhook = process.env.DISCORD_WEBHOOK_URL; | ||
| process.env.DISCORD_WEBHOOK_URL = 'https://example.invalid/test-webhook'; | ||
|
|
||
| try { | ||
| const releaseConfig = await loadReleaseConfig('notification-env'); | ||
| const configSource = fs.readFileSync(configPath, 'utf8'); | ||
|
|
||
| expect(releaseConfig.notifications.discord.webhookUrl).toBe( | ||
| 'https://example.invalid/test-webhook' | ||
| ); | ||
| expect(/https:\/\/discord\.com\/api\/webhooks\//.test(configSource)).toBe(false); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Assert behavior when DISCORD_WEBHOOK_URL is absent.
The test verifies that an environment value is selected. It does not verify that no fallback credential exists. A hardcoded or constructed fallback can pass the current test.
Delete the variable, load the configuration with a distinct cache key, and assert that webhookUrl is undefined before testing the configured environment value.
Proposed test update
try {
+ delete process.env.DISCORD_WEBHOOK_URL;
+ const missingCredentialConfig = await loadReleaseConfig(
+ 'notification-no-env'
+ );
+ expect(
+ missingCredentialConfig.notifications.discord.webhookUrl
+ ).toBeUndefined();
+
const releaseConfig = await loadReleaseConfig('notification-env');🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 42-42: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(configPath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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/cli/tests/unit/scripts/release-config.test.ts` around lines 37 - 48,
Update the `loads notification credentials only from the environment` test to
first remove `DISCORD_WEBHOOK_URL`, load the configuration with a distinct cache
key, and assert `releaseConfig.notifications.discord.webhookUrl` is undefined.
Then restore the environment value, load the configured case, and retain the
existing assertion that it is selected.
1 participant
Summary
DISCORD_WEBHOOK_URLinstead of shipping a source defaultVerification
bun x vitest run --config vitest.config.ts --project unit tests/unit/scripts/release-config.test.tsbun x biome check release.config.js tests/unit/scripts/release-config.test.tsbun x tsc --noEmit --pretty falsebun run release:minor --dry-run(resolved0.7.0)Summary by CodeRabbit
Bug Fixes
Tests