fix(ci): rewrite PyPI publish pipeline to use workflow_dispatch#1725
Draft
fix(ci): rewrite PyPI publish pipeline to use workflow_dispatch#1725
Conversation
Contributor
Greptile SummaryThis PR replaces the event-driven ( Key observations:
|
| Filename | Overview |
|---|---|
| .github/workflows/create-tag.yml | Adds actions: write permission and a dispatch step to trigger publish-pypi-approval.yml after a new version tag is pushed; logic is straightforward and correct. |
| .github/workflows/publish-pypi-approval.yml | Rewrites the publish pipeline to use workflow_dispatch; introduces a three-job build→test→publish chain, but has a shell injection risk from unguarded inputs.version interpolation, unnecessary poetry install in the test job, and a server cleanup gap on test failure. |
| .github/workflows/publish-wheel.yml | File deleted; its manual-dispatch functionality is superseded by the new publish-pypi-approval.yml pipeline. |
Sequence Diagram
sequenceDiagram
participant PR as Merged PR<br/>(chore/release-*)
participant CT as create-tag.yml
participant PA as publish-pypi-approval.yml<br/>(workflow_dispatch)
participant BW as build-wheel job
participant TW as test-wheel job<br/>(Python 3.10–3.13)
participant PP as publish-pypi job<br/>(pypi-production env)
participant PyPI as PyPI
PR->>CT: PR merged into develop
CT->>CT: Extract version from pyproject.toml
CT->>CT: Create & push git tag vX.Y.Z
CT->>PA: gh workflow run --ref develop -f version=X.Y.Z
PA->>BW: Trigger build-wheel
BW->>BW: Checkout tag, validate version
BW->>BW: Build wheel with poetry
BW->>BW: Upload artifact (publish-build)
BW->>TW: needs: build-wheel
TW->>TW: Download artifact
TW->>TW: pip install wheel[server]
TW->>TW: Start server, health-check HTTP 200
TW->>PP: needs: test-wheel
PP->>PP: Awaits pypi-production environment approval
PP->>PP: Download artifact, attest provenance
PP->>PyPI: pypa/gh-action-pypi-publish
PP->>PP: gh release create (draft)
Prompt To Fix All With AI
This is a comment left during a code review.
Path: .github/workflows/publish-pypi-approval.yml
Line: 25-30
Comment:
**Shell injection risk from `inputs.version`**
`${{ inputs.version }}` is interpolated directly into the shell script at the expression evaluation layer, before the shell ever runs. A malicious (or accidentally malformed) value such as `1.0.0" ]; then echo pwned; exit 0; #` would break out of the double-quoted comparison and execute arbitrary shell code. The same pattern repeats at line 171 (inside the `awk` command).
GitHub's own hardening guide recommends passing workflow inputs through environment variables so the shell sees them as data, never as syntax:
```suggestion
- name: Validate version in pyproject.toml
env:
INPUT_VERSION: ${{ inputs.version }}
run: |
VERSION_IN_FILE=$(grep '^version = ' pyproject.toml | sed 's/version = "\(.*\)"/\1/')
if [ "$VERSION_IN_FILE" != "$INPUT_VERSION" ]; then
echo "Version mismatch: pyproject.toml=$VERSION_IN_FILE, expected=$INPUT_VERSION"
exit 1
fi
```
Apply the same pattern to the `awk -v version=` call further down (line 171):
```
awk -v version="$INPUT_VERSION" '...' CHANGELOG.md
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: .github/workflows/publish-pypi-approval.yml
Line: 90-95
Comment:
**Unnecessary `poetry` install in test job**
`pip install poetry==1.8.2` is executed here but `poetry` is never invoked in the `test-wheel` job — only `pip` is used to install the wheel and run the server tests. Removing this reduces install time across all four matrix Python versions.
```suggestion
- name: Install wheel
run: |
pip install --upgrade pip
WHEEL=$(ls nemoguardrails-*.whl)
pip install "${WHEEL}[server]"
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: .github/workflows/publish-pypi-approval.yml
Line: 123-125
Comment:
**Server process not cleaned up on test failure**
`if: ${{ success() }}` means the `kill $SERVER_PID` step is skipped whenever an earlier step fails (e.g., `Check server status` exits non-zero). On a self-hosted runner or a long-lived environment this could leave a dangling `nemoguardrails server` process. Using `always()` ensures cleanup regardless of outcome.
```suggestion
- name: Stop server
if: always()
run: kill $SERVER_PID
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: .github/workflows/publish-pypi-approval.yml
Line: 102-113
Comment:
**Wait loop exits silently if server never starts**
If the server fails to respond within all 30 iterations, the loop ends with exit code `0` and the step is marked as passed. The failure is only detected in the subsequent `Check server status` step, which can produce confusing output (it will report an HTTP error code rather than a timeout). Consider adding an explicit failure after the loop:
```suggestion
- name: Wait for server to be up
run: |
echo "Waiting for server to start..."
for i in {1..30}; do
if curl --output /dev/null --silent --head --fail http://localhost:8000; then
echo "Server is up!"
exit 0
else
echo "Waiting... ($i/30)"
sleep 1
fi
done
echo "Server did not start within 30 seconds." >&2
exit 1
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 4b3abfa
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
25
to
+30
| run: | | ||
| HEAD_BRANCH="${{ github.event.workflow_run.head_branch }}" | ||
| VERSION_IN_FILE=$(grep '^version = ' pyproject.toml | sed 's/version = "\(.*\)"/\1/') | ||
| if [ "$VERSION_IN_FILE" != "${{ inputs.version }}" ]; then | ||
| echo "Version mismatch: pyproject.toml=$VERSION_IN_FILE, expected=${{ inputs.version }}" | ||
| exit 1 | ||
| fi |
Contributor
There was a problem hiding this comment.
Shell injection risk from inputs.version
${{ inputs.version }} is interpolated directly into the shell script at the expression evaluation layer, before the shell ever runs. A malicious (or accidentally malformed) value such as 1.0.0" ]; then echo pwned; exit 0; # would break out of the double-quoted comparison and execute arbitrary shell code. The same pattern repeats at line 171 (inside the awk command).
GitHub's own hardening guide recommends passing workflow inputs through environment variables so the shell sees them as data, never as syntax:
Suggested change
| run: | | |
| HEAD_BRANCH="${{ github.event.workflow_run.head_branch }}" | |
| VERSION_IN_FILE=$(grep '^version = ' pyproject.toml | sed 's/version = "\(.*\)"/\1/') | |
| if [ "$VERSION_IN_FILE" != "${{ inputs.version }}" ]; then | |
| echo "Version mismatch: pyproject.toml=$VERSION_IN_FILE, expected=${{ inputs.version }}" | |
| exit 1 | |
| fi | |
| - name: Validate version in pyproject.toml | |
| env: | |
| INPUT_VERSION: ${{ inputs.version }} | |
| run: | | |
| VERSION_IN_FILE=$(grep '^version = ' pyproject.toml | sed 's/version = "\(.*\)"/\1/') | |
| if [ "$VERSION_IN_FILE" != "$INPUT_VERSION" ]; then | |
| echo "Version mismatch: pyproject.toml=$VERSION_IN_FILE, expected=$INPUT_VERSION" | |
| exit 1 | |
| fi |
Apply the same pattern to the awk -v version= call further down (line 171):
awk -v version="$INPUT_VERSION" '...' CHANGELOG.md
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/publish-pypi-approval.yml
Line: 25-30
Comment:
**Shell injection risk from `inputs.version`**
`${{ inputs.version }}` is interpolated directly into the shell script at the expression evaluation layer, before the shell ever runs. A malicious (or accidentally malformed) value such as `1.0.0" ]; then echo pwned; exit 0; #` would break out of the double-quoted comparison and execute arbitrary shell code. The same pattern repeats at line 171 (inside the `awk` command).
GitHub's own hardening guide recommends passing workflow inputs through environment variables so the shell sees them as data, never as syntax:
```suggestion
- name: Validate version in pyproject.toml
env:
INPUT_VERSION: ${{ inputs.version }}
run: |
VERSION_IN_FILE=$(grep '^version = ' pyproject.toml | sed 's/version = "\(.*\)"/\1/')
if [ "$VERSION_IN_FILE" != "$INPUT_VERSION" ]; then
echo "Version mismatch: pyproject.toml=$VERSION_IN_FILE, expected=$INPUT_VERSION"
exit 1
fi
```
Apply the same pattern to the `awk -v version=` call further down (line 171):
```
awk -v version="$INPUT_VERSION" '...' CHANGELOG.md
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+90
to
+95
| - name: Install wheel | ||
| run: | | ||
| VERSION_IN_FILE=$(grep '^version = ' pyproject.toml | sed 's/version = "\(.*\)"/\1/') | ||
| TAG_VERSION="${{ steps.version.outputs.version }}" | ||
| if [ "$VERSION_IN_FILE" != "$TAG_VERSION" ]; then | ||
| echo "❌ Version mismatch: pyproject.toml=$VERSION_IN_FILE, tag=$TAG_VERSION" | ||
| pip install --upgrade pip | ||
| pip install poetry==1.8.2 | ||
| WHEEL=$(ls nemoguardrails-*.whl) | ||
| pip install "${WHEEL}[server]" |
Contributor
There was a problem hiding this comment.
Unnecessary poetry install in test job
pip install poetry==1.8.2 is executed here but poetry is never invoked in the test-wheel job — only pip is used to install the wheel and run the server tests. Removing this reduces install time across all four matrix Python versions.
Suggested change
| - name: Install wheel | |
| run: | | |
| VERSION_IN_FILE=$(grep '^version = ' pyproject.toml | sed 's/version = "\(.*\)"/\1/') | |
| TAG_VERSION="${{ steps.version.outputs.version }}" | |
| if [ "$VERSION_IN_FILE" != "$TAG_VERSION" ]; then | |
| echo "❌ Version mismatch: pyproject.toml=$VERSION_IN_FILE, tag=$TAG_VERSION" | |
| pip install --upgrade pip | |
| pip install poetry==1.8.2 | |
| WHEEL=$(ls nemoguardrails-*.whl) | |
| pip install "${WHEEL}[server]" | |
| - name: Install wheel | |
| run: | | |
| pip install --upgrade pip | |
| WHEEL=$(ls nemoguardrails-*.whl) | |
| pip install "${WHEEL}[server]" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/publish-pypi-approval.yml
Line: 90-95
Comment:
**Unnecessary `poetry` install in test job**
`pip install poetry==1.8.2` is executed here but `poetry` is never invoked in the `test-wheel` job — only `pip` is used to install the wheel and run the server tests. Removing this reduces install time across all four matrix Python versions.
```suggestion
- name: Install wheel
run: |
pip install --upgrade pip
WHEEL=$(ls nemoguardrails-*.whl)
pip install "${WHEEL}[server]"
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+123
to
+125
| - name: Stop server | ||
| if: ${{ success() }} | ||
| run: kill $SERVER_PID |
Contributor
There was a problem hiding this comment.
Server process not cleaned up on test failure
if: ${{ success() }} means the kill $SERVER_PID step is skipped whenever an earlier step fails (e.g., Check server status exits non-zero). On a self-hosted runner or a long-lived environment this could leave a dangling nemoguardrails server process. Using always() ensures cleanup regardless of outcome.
Suggested change
| - name: Stop server | |
| if: ${{ success() }} | |
| run: kill $SERVER_PID | |
| - name: Stop server | |
| if: always() | |
| run: kill $SERVER_PID |
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/publish-pypi-approval.yml
Line: 123-125
Comment:
**Server process not cleaned up on test failure**
`if: ${{ success() }}` means the `kill $SERVER_PID` step is skipped whenever an earlier step fails (e.g., `Check server status` exits non-zero). On a self-hosted runner or a long-lived environment this could leave a dangling `nemoguardrails server` process. Using `always()` ensures cleanup regardless of outcome.
```suggestion
- name: Stop server
if: always()
run: kill $SERVER_PID
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+102
to
+113
| - name: Wait for server to be up | ||
| run: | | ||
| echo "Waiting for server to start..." | ||
| for i in {1..30}; do | ||
| if curl --output /dev/null --silent --head --fail http://localhost:8000; then | ||
| echo "Server is up!" | ||
| break | ||
| else | ||
| echo "Waiting..." | ||
| sleep 1 | ||
| fi | ||
| done |
Contributor
There was a problem hiding this comment.
Wait loop exits silently if server never starts
If the server fails to respond within all 30 iterations, the loop ends with exit code 0 and the step is marked as passed. The failure is only detected in the subsequent Check server status step, which can produce confusing output (it will report an HTTP error code rather than a timeout). Consider adding an explicit failure after the loop:
Suggested change
| - name: Wait for server to be up | |
| run: | | |
| echo "Waiting for server to start..." | |
| for i in {1..30}; do | |
| if curl --output /dev/null --silent --head --fail http://localhost:8000; then | |
| echo "Server is up!" | |
| break | |
| else | |
| echo "Waiting..." | |
| sleep 1 | |
| fi | |
| done | |
| - name: Wait for server to be up | |
| run: | | |
| echo "Waiting for server to start..." | |
| for i in {1..30}; do | |
| if curl --output /dev/null --silent --head --fail http://localhost:8000; then | |
| echo "Server is up!" | |
| exit 0 | |
| else | |
| echo "Waiting... ($i/30)" | |
| sleep 1 | |
| fi | |
| done | |
| echo "Server did not start within 30 seconds." >&2 | |
| exit 1 |
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/publish-pypi-approval.yml
Line: 102-113
Comment:
**Wait loop exits silently if server never starts**
If the server fails to respond within all 30 iterations, the loop ends with exit code `0` and the step is marked as passed. The failure is only detected in the subsequent `Check server status` step, which can produce confusing output (it will report an HTTP error code rather than a timeout). Consider adding an explicit failure after the loop:
```suggestion
- name: Wait for server to be up
run: |
echo "Waiting for server to start..."
for i in {1..30}; do
if curl --output /dev/null --silent --head --fail http://localhost:8000; then
echo "Server is up!"
exit 0
else
echo "Waiting... ($i/30)"
sleep 1
fi
done
echo "Server did not start within 30 seconds." >&2
exit 1
```
How can I resolve this? If you propose a fix, please make it concise.
📝 WalkthroughWalkthroughThis PR restructures the GitHub Actions publishing workflow by adding a dispatch trigger in create-tag.yml to invoke the new publish pipeline, splitting the monolithic publish flow into three separate jobs (build, test, publish) with enhanced validation and caching in publish-pypi-approval.yml, and removing the legacy publish-wheel.yml workflow. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/publish-pypi-approval.yml (1)
158-196:⚠️ Potential issue | 🔴 CriticalAdd
skip-existing: trueto the PyPI publish action, or reorder the release creation first.PyPI publishing is not idempotent by default—reuploading the same distribution filename fails with "filename already exists" error. If the PyPI upload succeeds but the GitHub release creation fails (line 165), a rerun will fail again at the publish step and cannot recover. The GitHub release step already handles idempotency via existence check (line 185), but the publish step does not. Add
skip-existing: trueto the action, move the GitHub release creation before the PyPI publish, or add a conditional guard to skip publishing if the release already exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish-pypi-approval.yml around lines 158 - 196, The PyPI publish step using action pypa/gh-action-pypi-publish is not idempotent and can fail on reruns if the same artifact already exists; fix by either adding the skip-existing input (add "skip-existing: true" under the Publish to PyPI step for pypa/gh-action-pypi-publish) or by moving the GitHub release creation block to run before the Publish to PyPI step (or add a guard that checks for the GitHub release via gh release view and skips publishing if the release already exists); update the Publish to PyPI step or reorder the two steps accordingly so reruns do not fail with "filename already exists".
🤖 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/create-tag.yml:
- Around line 49-56: The dispatch step "Dispatch publish workflow" is currently
gated by if: steps.check-tag.outputs.exists == 'false' which prevents reruns
from re-dispatching after a transient failure; update that conditional to allow
reruns to proceed (for example change it to if: steps.check-tag.outputs.exists
== 'false' || github.run_attempt > 1) so a retried workflow can still run the gh
workflow run command when the original run failed; keep the gh workflow run
block and env GH_TOKEN unchanged.
---
Outside diff comments:
In @.github/workflows/publish-pypi-approval.yml:
- Around line 158-196: The PyPI publish step using action
pypa/gh-action-pypi-publish is not idempotent and can fail on reruns if the same
artifact already exists; fix by either adding the skip-existing input (add
"skip-existing: true" under the Publish to PyPI step for
pypa/gh-action-pypi-publish) or by moving the GitHub release creation block to
run before the Publish to PyPI step (or add a guard that checks for the GitHub
release via gh release view and skips publishing if the release already exists);
update the Publish to PyPI step or reorder the two steps accordingly so reruns
do not fail with "filename already exists".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0624fe20-4186-41eb-a5c1-aeac34b04df1
📒 Files selected for processing (3)
.github/workflows/create-tag.yml.github/workflows/publish-pypi-approval.yml.github/workflows/publish-wheel.yml
💤 Files with no reviewable changes (1)
- .github/workflows/publish-wheel.yml
Comment on lines
+49
to
+56
| - name: Dispatch publish workflow | ||
| if: steps.check-tag.outputs.exists == 'false' | ||
| env: | ||
| GH_TOKEN: ${{ github.token }} | ||
| run: | | ||
| gh workflow run publish-pypi-approval.yml \ | ||
| --ref develop \ | ||
| -f version=${{ steps.version.outputs.version }} |
There was a problem hiding this comment.
Let reruns re-dispatch the publish workflow.
Line 50 keys this step off the tag being absent before the job ran. If Line 47 pushes the tag and Line 54 fails, a rerun will see exists=true and skip the dispatch entirely, so this workflow cannot recover from a transient handoff failure without a separate manual trigger.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/create-tag.yml around lines 49 - 56, The dispatch step
"Dispatch publish workflow" is currently gated by if:
steps.check-tag.outputs.exists == 'false' which prevents reruns from
re-dispatching after a transient failure; update that conditional to allow
reruns to proceed (for example change it to if: steps.check-tag.outputs.exists
== 'false' || github.run_attempt > 1) so a retried workflow can still run the gh
workflow run command when the original run failed; keep the gh workflow run
block and env GH_TOKEN unchanged.
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
1 participant
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.
Description
Related Issue(s)
Checklist
Summary by CodeRabbit