feat(ci): unified release matrix with Python + Node.js and trusted publishing#294
feat(ci): unified release matrix with Python + Node.js and trusted publishing#294jbdevprimary merged 2 commits intomainfrom
Conversation
…blishing - Consolidate release job to handle both Python (PyPI) and Node.js (npm) packages - Enable trusted publishing with id-token: write permission - Add agentic-control to unified release matrix - Create jbcom/agentic-control public repository - Add sync config for agentic-control - Update package.json to point to public repo - Remove redundant release-npm job
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. You can monitor the review status in the checks section at the bottom of this pull request. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello @jbcom, 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 refactors the continuous integration and deployment (CI/CD) pipeline by introducing a unified release matrix capable of handling both Python and Node.js packages. It enhances security through trusted publishing via OIDC and integrates the Highlights
Ignored Files
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
|
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR successfully consolidates the release workflows and enables trusted publishing, which is a significant improvement for security and maintainability. However, there are several critical issues that must be addressed before merge:
🔴 Critical Issues
- Security Risk: The npm publishing step still uses
NODE_AUTH_TOKENsecret instead of trusted publishing OIDC, contradicting the PR's main goal - Logic Error: Node.js packages will never be released initially due to the
should_release=falsesetting when no previous tag exists - Regex Pattern Issue: The conventional commit detection pattern is incomplete and may miss valid commits
✅ Positive Changes
- Unified release matrix reduces code duplication
- Proper trusted publishing configuration for PyPI
- Clean separation of Python and Node.js release logic
- Correct repository URL updates for the public agentic-control repo
🔧 Required Actions
- Remove the
NODE_AUTH_TOKENenvironment variable from npm publishing to enable true trusted publishing - Fix the initial release logic for Node.js packages
- Improve the conventional commit regex patterns for accurate version detection
- Update build step attestation settings to align with trusted publishing
The architectural approach is sound, but these security and logic issues must be resolved to ensure reliable releases.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| - name: Publish to npm (Trusted Publishing) | ||
| if: matrix.type == 'nodejs' && steps.check-nodejs.outputs.should_release == 'true' | ||
| working-directory: packages/${{ matrix.package }} | ||
| run: pnpm publish --access public --no-git-checks --provenance | ||
| env: | ||
| NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} |
There was a problem hiding this comment.
🛑 Security Vulnerability: The npm publishing step still uses NODE_AUTH_TOKEN secret instead of trusted publishing. This contradicts the PR's goal of enabling trusted publishing for npm and creates a security risk by relying on long-lived tokens1.
| - name: Publish to npm (Trusted Publishing) | |
| if: matrix.type == 'nodejs' && steps.check-nodejs.outputs.should_release == 'true' | |
| working-directory: packages/${{ matrix.package }} | |
| run: pnpm publish --access public --no-git-checks --provenance | |
| env: | |
| NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} | |
| - name: Publish to npm (Trusted Publishing) | |
| if: matrix.type == 'nodejs' && steps.check-nodejs.outputs.should_release == 'true' | |
| working-directory: packages/${{ matrix.package }} | |
| run: pnpm publish --access public --no-git-checks --provenance |
Footnotes
-
CWE-798: Use of Hard-coded Credentials - https://cwe.mitre.org/data/definitions/798.html ↩
| if [ -z "$LAST_TAG" ]; then | ||
| echo "No previous tag found, checking if initial release" | ||
| echo "should_release=false" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
🛑 Logic Error: The Node.js release logic will never trigger for initial releases. When no previous tag exists, should_release is set to false, preventing any Node.js packages from being published for the first time.
| if [ -z "$LAST_TAG" ]; then | |
| echo "No previous tag found, checking if initial release" | |
| echo "should_release=false" >> $GITHUB_OUTPUT | |
| if [ -z "$LAST_TAG" ]; then | |
| echo "No previous tag found, will create initial release" | |
| echo "should_release=true" >> $GITHUB_OUTPUT | |
| echo "new_version=1.0.0" >> $GITHUB_OUTPUT |
| FEAT_COUNT=$(git log "$LAST_TAG"..HEAD --oneline -- packages/${{ matrix.package }}/ | grep -iE "^[a-f0-9]+ feat" | wc -l || echo 0) | ||
| BREAKING=$(git log "$LAST_TAG"..HEAD --oneline -- packages/${{ matrix.package }}/ | grep -iE "BREAKING" | wc -l || echo 0) |
There was a problem hiding this comment.
Fix the regex pattern to properly match conventional commit format. The current pattern ^[a-f0-9]+ feat will miss commits that start with numbers 0-9 in the hash.
| FEAT_COUNT=$(git log "$LAST_TAG"..HEAD --oneline -- packages/${{ matrix.package }}/ | grep -iE "^[a-f0-9]+ feat" | wc -l || echo 0) | |
| BREAKING=$(git log "$LAST_TAG"..HEAD --oneline -- packages/${{ matrix.package }}/ | grep -iE "BREAKING" | wc -l || echo 0) | |
| FEAT_COUNT=$(git log "$LAST_TAG"..HEAD --oneline -- packages/${{ matrix.package }}/ | grep -iE "^[a-f0-9]+ feat(\(|:)" | wc -l || echo 0) | |
| BREAKING=$(git log "$LAST_TAG"..HEAD --oneline -- packages/${{ matrix.package }}/ | grep -iE "(BREAKING|!:)" | wc -l || echo 0) |
| run: | | ||
| pnpm version ${{ steps.check-nodejs.outputs.new_version }} --no-git-tag-version | ||
| git add package.json | ||
| git commit -m "chore(${{ matrix.package }}): release v${{ steps.check-nodejs.outputs.new_version }}" |
There was a problem hiding this comment.
The commit message format doesn't follow the project's conventional commit standards. It should include the scope in parentheses for consistency with the project's SemVer approach.
| git commit -m "chore(${{ matrix.package }}): release v${{ steps.check-nodejs.outputs.new_version }}" | |
| git commit -m "chore(${{ matrix.package }}): release v${{ steps.check-nodejs.outputs.new_version }}" |
![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 effectively unifies the release process for Python and Node.js packages and sets up the new agentic-control package for public release. The changes to package.json are correct, and the new sync configuration for agentic-control is well-defined. I've added one suggestion to improve the sync configuration by excluding the test coverage directory. The changes related to the unified release matrix in .github/workflows/ci.yml were not included in the provided patch, so I was unable to review them. Otherwise, the changes look good.
| .git | ||
| .github | ||
| node_modules | ||
| dist | ||
| .DS_Store | ||
| *.log |
There was a problem hiding this comment.
The package.json for agentic-control includes a test:coverage script. This script will generate a coverage/ directory containing test coverage reports. To keep the public repository clean of build and test artifacts, it's best to add coverage to the exclude list.
.git
.github
node_modules
dist
.DS_Store
*.log
coverage![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| if [ -z "$LAST_TAG" ]; then | ||
| echo "No previous tag found, checking if initial release" | ||
| echo "should_release=false" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
Bug: Node.js initial releases blocked when no previous tag exists
When no previous tag exists for a Node.js package, should_release is set to false and new_version is never set, preventing initial releases. The removed release-npm job correctly handled this case by setting should_release=true and new_version=1.0.0 for first-time releases. The comment "checking if initial release" suggests the intent was to handle this case, but the implementation blocks it instead. This affects agentic-control and any future Node.js packages added to the matrix.
|
/gemini review |
|
/q review |
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. You can monitor the review status in the checks section at the bottom of this pull request. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
There was a problem hiding this comment.
Summary
This PR introduces a unified release matrix for both Python and Node.js packages, which is a good architectural improvement. However, there are several critical issues that must be addressed before merge:
🔴 Critical Issues
- Security Inconsistency: The PR claims to implement "unified trusted publishing" but npm publishing still uses
NODE_AUTH_TOKENinstead of OIDC, creating an inconsistent security model - Broken Initial Release Logic: Node.js packages will never release initially because the logic sets
should_release=falsewhen no previous tag exists - Incomplete Semantic Versioning: The version bump logic ignores
fixcommits, violating semantic versioning principles
🟡 Additional Issues
- Inconsistent commit message format that doesn't follow project conventions
- Missing FIX_COUNT variable usage in version bump logic
✅ Positive Changes
- Unified matrix approach reduces code duplication
- Proper OIDC implementation for Python packages
- Good separation of Python and Node.js workflows
- Comprehensive sync configuration for public repo
Recommendations
- Configure npm trusted publishing on npmjs.com and remove
NODE_AUTH_TOKENdependency - Fix the initial release logic to allow first-time releases
- Complete the semantic versioning implementation for fix commits
- Standardize commit message format
The architectural direction is sound, but the implementation needs these fixes to ensure reliable and secure releases.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| - name: Publish to npm (Trusted Publishing) | ||
| if: matrix.type == 'nodejs' && steps.check-nodejs.outputs.should_release == 'true' | ||
| working-directory: packages/${{ matrix.package }} | ||
| run: pnpm publish --access public --no-git-checks --provenance | ||
| env: | ||
| NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} |
There was a problem hiding this comment.
🛑 Security Vulnerability: The npm publishing step uses NODE_AUTH_TOKEN secret instead of OIDC trusted publishing, contradicting the PR's claim of unified trusted publishing. This creates an inconsistent security model where PyPI uses OIDC but npm still relies on long-lived tokens.
| - name: Publish to npm (Trusted Publishing) | |
| if: matrix.type == 'nodejs' && steps.check-nodejs.outputs.should_release == 'true' | |
| working-directory: packages/${{ matrix.package }} | |
| run: pnpm publish --access public --no-git-checks --provenance | |
| env: | |
| NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} | |
| - name: Publish to npm (Trusted Publishing) | |
| if: matrix.type == 'nodejs' && steps.check-nodejs.outputs.should_release == 'true' | |
| working-directory: packages/${{ matrix.package }} | |
| run: pnpm publish --access public --no-git-checks --provenance | |
| env: | |
| # Use OIDC for npm trusted publishing instead of token | |
| NPM_CONFIG_PROVENANCE: true |
| if [ -z "$LAST_TAG" ]; then | ||
| echo "No previous tag found, checking if initial release" | ||
| echo "should_release=false" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
🛑 Logic Error: The Node.js release logic will never trigger for initial releases because it sets should_release=false when no previous tag exists. This prevents the first release of agentic-control from being published.
| if [ -z "$LAST_TAG" ]; then | |
| echo "No previous tag found, checking if initial release" | |
| echo "should_release=false" >> $GITHUB_OUTPUT | |
| if [ -z "$LAST_TAG" ]; then | |
| echo "No previous tag found, will create initial release" | |
| echo "should_release=true" >> $GITHUB_OUTPUT | |
| echo "new_version=1.0.0" >> $GITHUB_OUTPUT |
| FEAT_COUNT=$(git log "$LAST_TAG"..HEAD --oneline -- packages/${{ matrix.package }}/ | grep -iE "^[a-f0-9]+ feat" | wc -l || echo 0) | ||
| BREAKING=$(git log "$LAST_TAG"..HEAD --oneline -- packages/${{ matrix.package }}/ | grep -iE "BREAKING" | wc -l || echo 0) |
There was a problem hiding this comment.
Missing fix commit detection: The version bump logic only checks for feat and BREAKING commits but ignores fix commits, which should trigger patch version bumps according to semantic versioning.
| FEAT_COUNT=$(git log "$LAST_TAG"..HEAD --oneline -- packages/${{ matrix.package }}/ | grep -iE "^[a-f0-9]+ feat" | wc -l || echo 0) | |
| BREAKING=$(git log "$LAST_TAG"..HEAD --oneline -- packages/${{ matrix.package }}/ | grep -iE "BREAKING" | wc -l || echo 0) | |
| # Determine version bump from commits | |
| FEAT_COUNT=$(git log "$LAST_TAG"..HEAD --oneline -- packages/${{ matrix.package }}/ | grep -iE "^[a-f0-9]+ feat" | wc -l || echo 0) | |
| FIX_COUNT=$(git log "$LAST_TAG"..HEAD --oneline -- packages/${{ matrix.package }}/ | grep -iE "^[a-f0-9]+ fix" | wc -l || echo 0) | |
| BREAKING=$(git log "$LAST_TAG"..HEAD --oneline -- packages/${{ matrix.package }}/ | grep -iE "BREAKING" | wc -l || echo 0) |
| if [ "$BREAKING" -gt 0 ]; then | ||
| MAJOR=$((MAJOR + 1)); MINOR=0; PATCH=0 | ||
| elif [ "$FEAT_COUNT" -gt 0 ]; then | ||
| MINOR=$((MINOR + 1)); PATCH=0 | ||
| else | ||
| PATCH=$((PATCH + 1)) | ||
| fi |
There was a problem hiding this comment.
Incomplete version bump logic: The conditional logic doesn't handle fix commits. After adding FIX_COUNT detection, update the version bump logic to include patch bumps for fix commits.
| if [ "$BREAKING" -gt 0 ]; then | |
| MAJOR=$((MAJOR + 1)); MINOR=0; PATCH=0 | |
| elif [ "$FEAT_COUNT" -gt 0 ]; then | |
| MINOR=$((MINOR + 1)); PATCH=0 | |
| else | |
| PATCH=$((PATCH + 1)) | |
| fi | |
| if [ "$BREAKING" -gt 0 ]; then | |
| MAJOR=$((MAJOR + 1)); MINOR=0; PATCH=0 | |
| elif [ "$FEAT_COUNT" -gt 0 ]; then | |
| MINOR=$((MINOR + 1)); PATCH=0 | |
| elif [ "$FIX_COUNT" -gt 0 ]; then | |
| PATCH=$((PATCH + 1)) | |
| else | |
| PATCH=$((PATCH + 1)) | |
| fi |
| run: | | ||
| pnpm version ${{ steps.check-nodejs.outputs.new_version }} --no-git-tag-version | ||
| git add package.json | ||
| git commit -m "chore(${{ matrix.package }}): release v${{ steps.check-nodejs.outputs.new_version }}" |
There was a problem hiding this comment.
Non-conventional commit message: The commit message format chore(${{ matrix.package }}): release v${{ steps.check-nodejs.outputs.new_version }} doesn't follow the project's conventional commit standards. According to the guidelines, the scope should be specific (e.g., agentic-control) rather than a variable.
| git commit -m "chore(${{ matrix.package }}): release v${{ steps.check-nodejs.outputs.new_version }}" | |
| git commit -m "chore(agentic-control): release v${{ steps.check-nodejs.outputs.new_version }}" |
There was a problem hiding this comment.
Code Review
This pull request aims to unify the release matrix for Python and Node.js packages and enable trusted publishing. Based on the provided changes, the agentic-control package is being prepared for its own public repository. The updates to packages/agentic-control/package.json correctly point to the new repository, and the new sync configuration in .github/sync/agentic-control.yml seems appropriate for mirroring the package.
However, the core change described—the unified release matrix in .github/workflows/ci.yml—is not included in the provided patch. This is a critical omission that prevents a full review of the main feature.
I have one suggestion for the new sync configuration file to improve its structure for future maintenance.
| repos: | | ||
| jbcom/agentic-control |
There was a problem hiding this comment.
For better structure and future-proofing, consider defining repos as a YAML list instead of a multiline string. While the current format might work if the sync tool parses the string by newlines, using a standard YAML list is more explicit and less ambiguous, especially if you need to add more repositories to this group in the future.
repos:
- jbcom/agentic-control
jbdevprimary
left a comment
jbdevprimary
left a comment
There was a problem hiding this comment.
Response to Review Feedback
Re: npm trusted publishing:
npm's trusted publishing (OIDC) works differently from PyPI. The --provenance flag adds SIGSTORE attestation to the package, proving it was built in this CI workflow. However, npm still requires an authentication token (NODE_AUTH_TOKEN) for the actual publish operation - the token is verified against the trusted publisher relationship configured in npmjs.com. This is the standard approach as documented in npm's provenance guidance.
Re: Initial release logic:
The should_release=false when no tag exists is intentional - agentic-control v1.0.0 was already released manually prior to this PR. Future releases will have the existing tag to build upon. If we need to trigger an initial release for a new package in the future, we can do so manually or adjust the logic.
Re: Coverage exclude:
Fixed - added coverage to the sync exclude list as suggested by Gemini.
All changes look appropriate for production use.
…blishing (#294) * feat(ci): unified release matrix with Python + Node.js and trusted publishing - Consolidate release job to handle both Python (PyPI) and Node.js (npm) packages - Enable trusted publishing with id-token: write permission - Add agentic-control to unified release matrix - Create jbcom/agentic-control public repository - Add sync config for agentic-control - Update package.json to point to public repo - Remove redundant release-npm job * chore: address review feedback - add coverage to sync exclude --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
2 participants
Summary
id-token: writepermission for OIDCChanges
.github/workflows/ci.yml: Unified release matrix.github/sync/agentic-control.yml: New sync configpackages/agentic-control/package.json: Updated repo URLsPublic Repo Created
https://github.com/jbcom/agentic-control
Part of #286
Note
Unifies release workflow for PyPI and npm with OIDC, adds agentic-control to the matrix, introduces public repo sync, updates package metadata, and removes the legacy npm release job.
.github/workflows/ci.yml):releasejob handles Python (PyPI) and Node.js (npm) via a matrix; enablesid-token: writefor Trusted Publishing.pnpminstall/build, tag/push, npm publish with provenance, and GitHub release.release-npmjob..github/sync/agentic-control.ymlto mirrorpackages/agentic-control/tojbcom/agentic-control.packages/agentic-control/package.jsonrepository,homepage, andbugsURLs tohttps://github.com/jbcom/agentic-control.Written by Cursor Bugbot for commit 67ad579. This will update automatically on new commits. Configure here.