fix: publish npm releases and clarify install identifiers#1446
Conversation
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
📝 WalkthroughWalkthroughThis pull request standardizes the plugin identifier from short form to fully-qualified ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
7771589 to
3438032
Compare
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
There was a problem hiding this comment.
1 issue found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/reusable-release.yml">
<violation number="1" location=".github/workflows/reusable-release.yml:56">
P1: Validate that `inputs.tag` matches `package.json` version before checking/publishing to npm; otherwise this workflow can skip or publish the wrong npm version for the release tag.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| run: | | ||
| TAG_VERSION="${INPUT_TAG#v}" | ||
| PACKAGE_NAME=$(node -p "require('./package.json').name") | ||
| PACKAGE_VERSION=$(node -p "require('./package.json').version") |
There was a problem hiding this comment.
P1: Validate that inputs.tag matches package.json version before checking/publishing to npm; otherwise this workflow can skip or publish the wrong npm version for the release tag.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/reusable-release.yml, line 56:
<comment>Validate that `inputs.tag` matches `package.json` version before checking/publishing to npm; otherwise this workflow can skip or publish the wrong npm version for the release tag.</comment>
<file context>
@@ -47,6 +49,21 @@ jobs:
+ id: npm_publish_state
+ run: |
+ PACKAGE_NAME=$(node -p "require('./package.json').name")
+ PACKAGE_VERSION=$(node -p "require('./package.json').version")
+ if npm view "${PACKAGE_NAME}@${PACKAGE_VERSION}" version >/dev/null 2>&1; then
+ echo "already_published=true" >> "$GITHUB_OUTPUT"
</file context>
| PACKAGE_VERSION=$(node -p "require('./package.json').version") | |
| PACKAGE_VERSION=$(node -p "require('./package.json').version") | |
| TAG_VERSION="${{ inputs.tag }}" | |
| TAG_VERSION="${TAG_VERSION#v}" | |
| if [ "$PACKAGE_VERSION" != "$TAG_VERSION" ]; then | |
| echo "::error::Tag version ($TAG_VERSION) does not match package.json version ($PACKAGE_VERSION)" | |
| exit 1 | |
| fi |
Greptile SummaryThis PR wires npm publishing into both release workflows (
Confidence Score: 4/5Safe to merge after fixing the missing NPM_TOKEN guard in reusable-release.yml. One P1 defect in reusable-release.yml: without a token guard, callers that don't supply NPM_TOKEN on a new version will have the entire release job fail, silently skipping GitHub Release creation. The fix is a small in-run null-check. All other findings are P2. .github/workflows/reusable-release.yml — the Publish npm package step needs a guard for when NPM_TOKEN is not provided. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Tag push / workflow_call] --> B[Checkout & Setup Node]
B --> C[Install deps & validate tag]
C --> D[Check npm publish state\nnpm view pkg@version]
D -->|already_published=true| E[Skip publish]
D -->|already_published=false| F{NPM_TOKEN set?}
F -->|Yes| G[npm publish --access public --provenance]
F -->|No — reusable-release only\nno guard today| H[❌ npm publish fails with 401]
G --> I[Generate release highlights]
E --> I
H --> X[❌ Job fails — GitHub Release never created]
I --> J[Create GitHub Release]
J --> K[✅ Done]
Reviews (1): Last reviewed commit: "fix: wire npm auth into release publish" | Re-trigger Greptile |
| - name: Publish npm package | ||
| if: steps.npm_publish_state.outputs.already_published != 'true' | ||
| env: | ||
| NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} | ||
| run: npm publish --access public --provenance |
There was a problem hiding this comment.
Because NPM_TOKEN is declared required: false, callers may invoke this reusable workflow without providing it. When that happens and the version hasn't been published yet, the already_published check passes (no auth needed for npm view on a public package), but the subsequent npm publish fails with a 401 — and because Create GitHub Release is a later step, it never runs. The caller ends up with neither an npm publish nor a GitHub Release.
Add an explicit skip when no token is available:
| - name: Publish npm package | |
| if: steps.npm_publish_state.outputs.already_published != 'true' | |
| env: | |
| NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} | |
| run: npm publish --access public --provenance | |
| - name: Publish npm package | |
| if: steps.npm_publish_state.outputs.already_published != 'true' | |
| env: | |
| NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} | |
| run: | | |
| if [ -z "$NODE_AUTH_TOKEN" ]; then | |
| echo "NPM_TOKEN not provided — skipping npm publish" | |
| exit 0 | |
| fi | |
| npm publish --access public --provenance |
| for (const relativePath of publicInstallDocs) { | ||
| const content = fs.readFileSync(path.join(repoRoot, relativePath), 'utf8'); |
There was a problem hiding this comment.
readFileSync outside test wrapper crashes on missing file
fs.readFileSync is called outside the test() try/catch wrapper. If any file in publicInstallDocs is renamed or deleted, the call throws an unhandled exception that crashes the process before failed is incremented or a clean error message is printed — making failures hard to diagnose in CI.
| for (const relativePath of publicInstallDocs) { | |
| const content = fs.readFileSync(path.join(repoRoot, relativePath), 'utf8'); | |
| let content; | |
| test(`${relativePath} is readable`, () => { | |
| content = fs.readFileSync(path.join(repoRoot, relativePath), 'utf8'); | |
| }); | |
| if (content === undefined) continue; |
| echo "already_published=false" >> "$GITHUB_OUTPUT" | ||
| fi | ||
|
|
||
| - name: Publish npm package | ||
| if: steps.npm_publish_state.outputs.already_published != 'true' |
There was a problem hiding this comment.
If npm publish succeeds but the subsequent Create GitHub Release step fails (transient API error, bad body_path, etc.), the package lands on npm without a corresponding GitHub release. Swapping the two steps — creating the GitHub release first and publishing to npm after — provides a better fallback: a failed npm publish leaves an orphaned GitHub release (easily re-run) rather than an orphaned npm package (harder to unlink).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/scripts/release-publish.test.js (1)
40-41: Relax regex strictness to avoid format-only test failures.Current regexes are sensitive to quoting/flag order, so harmless YAML formatting changes can fail the test.
♻️ More robust matching patterns
test(`${workflow} configures the npm registry`, () => { - assert.match(content, /registry-url:\s*['"]https:\/\/registry\.npmjs\.org['"]/); + assert.match(content, /registry-url:\s*['"]?https:\/\/registry\.npmjs\.org['"]?/); }); test(`${workflow} checks whether the tagged npm version already exists`, () => { assert.match(content, /Check npm publish state/); - assert.match(content, /npm view "\$\{PACKAGE_NAME\}@\$\{PACKAGE_VERSION\}" version/); + assert.match(content, /npm view\s+['"]?\$\{PACKAGE_NAME\}@\$\{PACKAGE_VERSION\}['"]?\s+version/); }); test(`${workflow} publishes new tag versions to npm`, () => { - assert.match(content, /npm publish --access public --provenance/); + assert.match(content, /npm publish\b(?=[^\n]*--access\s+public)(?=[^\n]*--provenance)/); assert.match(content, /NODE_AUTH_TOKEN:\s*\$\{\{\s*secrets\.NPM_TOKEN\s*\}\}/); });Also applies to: 46-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/scripts/release-publish.test.js` around lines 40 - 41, The test's regex is too strict about quoting/flag order in the workflow match (the test block using variables workflow and content); relax the pattern to accept optional quotes and flexible spacing—e.g. replace /registry-url:\s*['"]https:\/\/registry\.npmjs\.org['"]/ with a pattern that allows optional single or double quotes and any whitespace (such as /registry-url:\s*["']?https:\/\/registry\.npmjs\.org["']?/), and apply the same relaxed approach to the other regexes referenced around lines 46-51 so they no longer fail on harmless YAML formatting/quote-order changes.
🤖 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/reusable-release.yml:
- Around line 15-17: The publish step runs even when NPM_TOKEN is not provided;
add a guard so it only runs when the secret is present by adding an if condition
on the publish job/step (e.g., the "publish" step) such as if: ${{
secrets.NPM_TOKEN != '' }} or if: ${{ secrets.NPM_TOKEN }}; this leaves
NPM_TOKEN optional in the secrets map but prevents the publish step from
executing and failing when the token is omitted.
In `@README.md`:
- Around line 192-197: Remove the duplicated recovery warning paragraph in the
README callout so the warning appears only once; locate the repeated block that
starts "If your local Claude setup was wiped or reset..." in the callout and
delete the second instance, leaving a single copy of the paragraph and
preserving surrounding formatting and emphasis.
In `@tests/scripts/release-publish.test.js`:
- Around line 44-52: Add an assertion in the `${workflow} publishes new tag
versions to npm` test to ensure the publish step is conditionally gated by the
lookup step result: verify the workflow content contains an if expression that
references the "Check npm publish state" step output (e.g., checks for an if
like steps.<check-step-id>.outputs.published == 'false' or
steps.<check-step-id>.outputs.published != 'true'). Update the test that
inspects `content` to assert the presence of that if-condition string so the
publish command (`npm publish --access public --provenance`) is only executed
when the lookup indicates the package is not already published.
---
Nitpick comments:
In `@tests/scripts/release-publish.test.js`:
- Around line 40-41: The test's regex is too strict about quoting/flag order in
the workflow match (the test block using variables workflow and content); relax
the pattern to accept optional quotes and flexible spacing—e.g. replace
/registry-url:\s*['"]https:\/\/registry\.npmjs\.org['"]/ with a pattern that
allows optional single or double quotes and any whitespace (such as
/registry-url:\s*["']?https:\/\/registry\.npmjs\.org["']?/), and apply the same
relaxed approach to the other regexes referenced around lines 46-51 so they no
longer fail on harmless YAML formatting/quote-order changes.
🪄 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
Run ID: 2855c5ff-a79d-425c-9372-4607eff9c114
📒 Files selected for processing (10)
.github/workflows/release.yml.github/workflows/reusable-release.ymlREADME.mdREADME.zh-CN.mddocs/ja-JP/skills/configure-ecc/SKILL.mddocs/pt-BR/README.mddocs/zh-CN/skills/configure-ecc/SKILL.mdpackage.jsontests/docs/install-identifiers.test.jstests/scripts/release-publish.test.js
| secrets: | ||
| NPM_TOKEN: | ||
| required: false |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspecting reusable release workflow conditions around NPM_TOKEN..."
rg -n -C2 'NPM_TOKEN|required: false|Publish npm package|already_published|NODE_AUTH_TOKEN|if:' .github/workflows/reusable-release.ymlRepository: affaan-m/everything-claude-code
Length of output: 965
Optional NPM_TOKEN conflicts with the publish step condition.
Line 17 marks NPM_TOKEN as optional, but the publish step at Line 82 executes whenever the package is unpublished, without verifying token presence. This causes auth failures when callers intentionally omit npm credentials.
🔧 Suggested fix (guard publish on token presence)
- name: Publish npm package
- if: steps.npm_publish_state.outputs.already_published != 'true'
+ if: steps.npm_publish_state.outputs.already_published != 'true' && secrets.NPM_TOKEN != ''
env:
NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}
run: npm publish --access public --provenance
+
+ - name: Skip npm publish (NPM_TOKEN not provided)
+ if: steps.npm_publish_state.outputs.already_published != 'true' && secrets.NPM_TOKEN == ''
+ run: echo "NPM_TOKEN not provided; skipping npm publish."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/reusable-release.yml around lines 15 - 17, The publish
step runs even when NPM_TOKEN is not provided; add a guard so it only runs when
the secret is present by adding an if condition on the publish job/step (e.g.,
the "publish" step) such as if: ${{ secrets.NPM_TOKEN != '' }} or if: ${{
secrets.NPM_TOKEN }}; this leaves NPM_TOKEN optional in the secrets map but
prevents the publish step from executing and failing when the token is omitted.
| > WARNING: **Important:** Claude Code plugins cannot distribute `rules` automatically. Install them manually: | ||
| > | ||
| > If your local Claude setup was wiped or reset, that does not mean you need to repurchase ECC. Start with `ecc list-installed`, then run `ecc doctor` and `ecc repair` before reinstalling anything. That usually restores ECC-managed files without rebuilding your setup. If the problem is account or marketplace access for ECC Tools, handle billing/account recovery separately. | ||
|
|
||
| > If your local Claude setup was wiped or reset, that does not mean you need to repurchase ECC. Start with `ecc list-installed`, then run `ecc doctor` and `ecc repair` before reinstalling anything. That usually restores ECC-managed files without rebuilding your setup. If the problem is account or marketplace access for ECC Tools, handle billing/account recovery separately. | ||
|
|
There was a problem hiding this comment.
Remove duplicated recovery warning paragraph.
The same warning text appears twice in the same callout, which adds noise.
✂️ Suggested cleanup
> WARNING: **Important:** Claude Code plugins cannot distribute `rules` automatically. Install them manually:
>
> If your local Claude setup was wiped or reset, that does not mean you need to repurchase ECC. Start with `ecc list-installed`, then run `ecc doctor` and `ecc repair` before reinstalling anything. That usually restores ECC-managed files without rebuilding your setup. If the problem is account or marketplace access for ECC Tools, handle billing/account recovery separately.
-
-> If your local Claude setup was wiped or reset, that does not mean you need to repurchase ECC. Start with `ecc list-installed`, then run `ecc doctor` and `ecc repair` before reinstalling anything. That usually restores ECC-managed files without rebuilding your setup. If the problem is account or marketplace access for ECC Tools, handle billing/account recovery separately.📝 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.
| > WARNING: **Important:** Claude Code plugins cannot distribute `rules` automatically. Install them manually: | |
| > | |
| > If your local Claude setup was wiped or reset, that does not mean you need to repurchase ECC. Start with `ecc list-installed`, then run `ecc doctor` and `ecc repair` before reinstalling anything. That usually restores ECC-managed files without rebuilding your setup. If the problem is account or marketplace access for ECC Tools, handle billing/account recovery separately. | |
| > If your local Claude setup was wiped or reset, that does not mean you need to repurchase ECC. Start with `ecc list-installed`, then run `ecc doctor` and `ecc repair` before reinstalling anything. That usually restores ECC-managed files without rebuilding your setup. If the problem is account or marketplace access for ECC Tools, handle billing/account recovery separately. | |
| > WARNING: **Important:** Claude Code plugins cannot distribute `rules` automatically. Install them manually: | |
| > | |
| > If your local Claude setup was wiped or reset, that does not mean you need to repurchase ECC. Start with `ecc list-installed`, then run `ecc doctor` and `ecc repair` before reinstalling anything. That usually restores ECC-managed files without rebuilding your setup. If the problem is account or marketplace access for ECC Tools, handle billing/account recovery separately. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 192 - 197, Remove the duplicated recovery warning
paragraph in the README callout so the warning appears only once; locate the
repeated block that starts "If your local Claude setup was wiped or reset..." in
the callout and delete the second instance, leaving a single copy of the
paragraph and preserving surrounding formatting and emphasis.
| test(`${workflow} checks whether the tagged npm version already exists`, () => { | ||
| assert.match(content, /Check npm publish state/); | ||
| assert.match(content, /npm view "\$\{PACKAGE_NAME\}@\$\{PACKAGE_VERSION\}" version/); | ||
| }); | ||
|
|
||
| test(`${workflow} publishes new tag versions to npm`, () => { | ||
| assert.match(content, /npm publish --access public --provenance/); | ||
| assert.match(content, /NODE_AUTH_TOKEN:\s*\$\{\{\s*secrets\.NPM_TOKEN\s*\}\}/); | ||
| }); |
There was a problem hiding this comment.
Add an assertion for the actual “skip publish if already published” gate.
These checks verify version lookup and publish command presence, but they don’t assert that the publish step is conditionally gated by the lookup result. That leaves a regression gap for the core skip behavior.
🔧 Proposed test hardening
test(`${workflow} checks whether the tagged npm version already exists`, () => {
assert.match(content, /Check npm publish state/);
assert.match(content, /npm view "\$\{PACKAGE_NAME\}@\$\{PACKAGE_VERSION\}" version/);
});
test(`${workflow} publishes new tag versions to npm`, () => {
+ // Ensure publish step is conditionally gated by "already published" state
+ assert.match(content, /if:\s*\$\{\{\s*[^}]*already[_-]?published[^}]*\}\}/);
assert.match(content, /npm publish --access public --provenance/);
assert.match(content, /NODE_AUTH_TOKEN:\s*\$\{\{\s*secrets\.NPM_TOKEN\s*\}\}/);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/scripts/release-publish.test.js` around lines 44 - 52, Add an assertion
in the `${workflow} publishes new tag versions to npm` test to ensure the
publish step is conditionally gated by the lookup step result: verify the
workflow content contains an if expression that references the "Check npm
publish state" step output (e.g., checks for an if like
steps.<check-step-id>.outputs.published == 'false' or
steps.<check-step-id>.outputs.published != 'true'). Update the test that
inspects `content` to assert the presence of that if-condition string so the
publish command (`npm publish --access public --provenance`) is only executed
when the lookup indicates the package is not already published.
Summary
Why
The current public state is inconsistent:
v1.10.0releaseecc-universalis still at1.9.0everything-claude-codenpm package is unpublishedecc@eccThis PR fixes the actual release workflow gap and makes the naming/migration story explicit:
affaan-m/everything-claude-codeeverything-claude-code@everything-claude-codeecc-universalThe marketplace/plugin identifier changed to align with Anthropic's marketplace/plugin model. The npm package name remains separate, so users need both surfaces documented clearly instead of assuming they are the same thing.
Validation
node tests/scripts/release-publish.test.jsnode tests/docs/install-identifiers.test.jsnpm testSummary by CodeRabbit
Documentation
everything-claude-code@everything-claude-code.Chores
Summary by cubic
Publishes tagged releases to npm and standardizes the canonical plugin identifier in docs to stop GitHub↔npm version drift and avoid install confusion.
Bug Fixes
release.ymlandreusable-release.ymlwith registry config,id-tokenprovenance,secrets.NPM_TOKENwired toNODE_AUTH_TOKEN, and skip-if-already-published checks.publishConfig.access: public; release notes now listecc-universalandeverything-claude-code@everything-claude-code.Migration
ecc@eccwitheverything-claude-code@everything-claude-codefor plugin installs.ecc-universal; plugin and package names are intentionally different.ecc list-installed,ecc doctor, thenecc repairbefore reinstalling.Written for commit b5c4d2b. Summary will update on new commits.