feat: Add dry-run support and refactor bot-workflows.yml (#1288) #1431
Conversation
|
Hi, this is WorkflowBot.
|
Mounil2005
changed the title
📝 WalkthroughWalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
participant Trigger as Trigger (workflow_run / workflow_dispatch)
participant Actions as GitHub Actions
participant Repo as Repository (checkout)
participant Script as bot-workflows.sh
participant GHCLI as gh CLI
participant GitHub as GitHub API
Trigger->>Actions: start notify-pr job
Actions->>Repo: checkout repository
Actions->>Script: run bot-workflows.sh (env: REPO, FAILED_*, DRY_RUN)
Script->>GHCLI: query failed run details (FAILED_RUN_ID)
GHCLI->>GitHub: fetch workflow run -> returns head branch
Script->>GHCLI: search open PR for branch
GHCLI->>GitHub: fetch PR details
alt PR found and DRY_RUN = 0
Script->>GHCLI: post comment on PR
GHCLI->>GitHub: create comment
else PR found and DRY_RUN = 1
Script->>Actions: log simulated comment (no post)
else PR not found
Script->>Actions: log no PR found
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/bot-workflows.yml (2)
20-22: Concurrency group should handle workflow_dispatch deterministically (avoid empty head_branch).
Right now dispatch likely collapses into the same group, which can cause surprising cancels.Proposed fix
concurrency: - group: "workflow-failure-${{ github.event.workflow_run.head_branch }}" + group: "workflow-failure-${{ github.event_name }}-${{ github.event.workflow_run.head_branch || github.run_id }}" cancel-in-progress: true
17-20: Addactions: readpermission (required forgh run view).The workflow calls
gh run viewat line 67 of.github/scripts/bot-workflows.shto retrieve the head branch of the failed workflow run. With explicit permissions declared,actions: readis required for the GITHUB_TOKEN to access workflow run metadata via the GitHub CLI.Proposed fix
permissions: contents: read + actions: read pull-requests: write
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/scripts/bot-workflows.sh.github/workflows/bot-workflows.ymlCHANGELOG.md
🧰 Additional context used
📓 Path-based instructions (2)
.github/scripts/**/*.sh
⚙️ CodeRabbit configuration file
.github/scripts/**/*.sh: Treat shell scripts as production-grade automation.Scripts should be small, focused, and explicit.
Avoid “do-everything” or overly generic scripts.
- MUST use:
set -euo pipefail- MUST validate all required environment variables
- MUST defensively quote variables
- MUST validate all untrusted input
- MUST have bounded loops and pagination
- MUST support dry-run mode if state is mutated
- MUST log key decisions and early exits
Files:
.github/scripts/bot-workflows.sh
.github/workflows/**/*
⚙️ CodeRabbit configuration file
.github/workflows/**/*: Review workflows as security-sensitive infrastructure.A good workflow is small, focused, and boring.
If a workflow is clever, generic, or overly flexible, it is a risk.
PRIORITY 0 — ABSOLUTE REQUIREMENTS
- All third-party actions MUST be pinned to full commit SHAs, similar to other workflows.
permissions:MUST be explicitly declared and minimally scoped.- Workflows MUST behave safely when executed from forks.
- YAML MUST orchestrate steps, not implement business logic.
- Any workflow that mutates GitHub state MUST support dry-run mode.
- Dry-run behavior must be explicit and visible in logs.
- Workflows MUST NOT modify repository source code outside
.github/.
PRIORITY 1 — SCOPE, FOCUS & RESTRAINT
- The title of each workflow must be relevant, match similar naming schemes, and match its script filename.
- Each workflow MUST have a single, clearly defined objective and SHOULD document this in a top-level comment.
- Flag workflows that:
- Attempt to be generic “frameworks”
- Include speculative or future-facing logic
- Perform actions unrelated to the stated goal
- Over-abstraction and excess flexibility are maintenance risks.
PRIORITY 2 — INPUT HARDENING
- Treat ALL GitHub event data as potentially hostile input, including:
- issue titles, bodies, and comments
- labels, usernames, branch names
- Free-form user input MUST NOT be passed directly into:
- shell commands
- gh CLI arguments
- Node.js exec / spawn calls
- Require strict allowlists or exact string matches.
- Flag any use of:
- eval or bash -c
- backticks or $(...) with user-controlled input
------------------...
Files:
.github/workflows/bot-workflows.yml
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~96-~96: The official name of this software platform is spelled with a capital “H”.
Context: ...t-workflows.ymlto use dedicated script.github/scripts/bot-workflows.sh` for improved ...
(GITHUB)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
CHANGELOG.md (1)
96-96: Changelog entry looks consistent and appropriately scoped.
No follow-ups from me here..github/scripts/bot-workflows.sh (1)
120-143: Dry-run behavior is explicit and exits safely.
Logs clearly show what would have happened, and no mutation occurs in DRY_RUN=1..github/workflows/bot-workflows.yml (1)
38-40: Checkout is pinned to a full commit SHA.
Good alignment with the “pin third-party actions” requirement.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/bot-workflows.yml (2)
23-35: Concurrency group needs dispatch-safe fallback.
github.event.workflow_run.head_branchis empty onworkflow_dispatch, causing all manual runs to collapse into the same concurrency group. Add a fallback:Proposed diff
concurrency: - group: "workflow-failure-${{ github.event.workflow_run.head_branch }}" + group: "workflow-failure-${{ github.event.workflow_run.head_branch || github.ref_name || github.run_id }}" cancel-in-progress: trueThe boolean input handling (
github.event.inputs.dry_run == 'true') is correct as-is; GitHub deliversworkflow_dispatchboolean inputs as strings.
20-25: Permissions must be corrected: addactions: readandchecks: readforgh run view, changepull-requests: writetoissues: writeforgh pr comment(plain comments only).The script calls
gh run view(requiresactions: readand optionallychecks: read) andgh pr comment(requiresissues: writefor plain comments, notpull-requests: write). Thepull-requests: writepermission is for PR reviews and inline comments only; posting plain PR comments requiresissues: write.Proposed diff
permissions: contents: read - pull-requests: write + actions: read + checks: read + issues: write
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/scripts/bot-workflows.sh.github/workflows/bot-workflows.yml
🧰 Additional context used
📓 Path-based instructions (2)
.github/scripts/**/*.sh
⚙️ CodeRabbit configuration file
.github/scripts/**/*.sh: Treat shell scripts as production-grade automation.Scripts should be small, focused, and explicit.
Avoid “do-everything” or overly generic scripts.
- MUST use:
set -euo pipefail- MUST validate all required environment variables
- MUST defensively quote variables
- MUST validate all untrusted input
- MUST have bounded loops and pagination
- MUST support dry-run mode if state is mutated
- MUST log key decisions and early exits
Files:
.github/scripts/bot-workflows.sh
.github/workflows/**/*
⚙️ CodeRabbit configuration file
.github/workflows/**/*: Review workflows as security-sensitive infrastructure.A good workflow is small, focused, and boring.
If a workflow is clever, generic, or overly flexible, it is a risk.
PRIORITY 0 — ABSOLUTE REQUIREMENTS
- All third-party actions MUST be pinned to full commit SHAs, similar to other workflows.
permissions:MUST be explicitly declared and minimally scoped.- Workflows MUST behave safely when executed from forks.
- YAML MUST orchestrate steps, not implement business logic.
- Any workflow that mutates GitHub state MUST support dry-run mode.
- Dry-run behavior must be explicit and visible in logs.
- Workflows MUST NOT modify repository source code outside
.github/.
PRIORITY 1 — SCOPE, FOCUS & RESTRAINT
- The title of each workflow must be relevant, match similar naming schemes, and match its script filename.
- Each workflow MUST have a single, clearly defined objective and SHOULD document this in a top-level comment.
- Flag workflows that:
- Attempt to be generic “frameworks”
- Include speculative or future-facing logic
- Perform actions unrelated to the stated goal
- Over-abstraction and excess flexibility are maintenance risks.
PRIORITY 2 — INPUT HARDENING
- Treat ALL GitHub event data as potentially hostile input, including:
- issue titles, bodies, and comments
- labels, usernames, branch names
- Free-form user input MUST NOT be passed directly into:
- shell commands
- gh CLI arguments
- Node.js exec / spawn calls
- Require strict allowlists or exact string matches.
- Flag any use of:
- eval or bash -c
- backticks or $(...) with user-controlled input
------------------...
Files:
.github/workflows/bot-workflows.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
.github/scripts/bot-workflows.sh (1)
118-141: Dry-run behavior is clear and non-mutating.The “would post” vs “skip due to duplicate” logging is explicit and the mutating call is gated behind
DRY_RUN==0..github/workflows/bot-workflows.yml (1)
36-43: Pinned actions and “YAML orchestrates, script implements” are on the right track.
step-security/harden-runnerandactions/checkoutare pinned to SHAs, and the workflow delegates non-trivial logic to.github/scripts/bot-workflows.sh.
|
Hi, this is MergeConflictBot. Please resolve these conflicts locally and push the changes. To assist you, please read: Thank you for contributing! |
Mounil2005
force-pushed
the
feature/bot-workflows-dry-run
branch
from
43a7610 to
25c85c8
Compare
|
Tests: |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/scripts/bot-workflows.sh.github/workflows/bot-workflows.ymlCHANGELOG.md
🧰 Additional context used
📓 Path-based instructions (2)
.github/scripts/**/*.sh
⚙️ CodeRabbit configuration file
.github/scripts/**/*.sh: Treat shell scripts as production-grade automation.Scripts should be small, focused, and explicit.
Avoid “do-everything” or overly generic scripts.
- MUST use:
set -euo pipefail- MUST validate all required environment variables
- MUST defensively quote variables
- MUST validate all untrusted input
- MUST have bounded loops and pagination
- MUST support dry-run mode if state is mutated
- MUST log key decisions and early exits
Files:
.github/scripts/bot-workflows.sh
.github/workflows/**/*
⚙️ CodeRabbit configuration file
.github/workflows/**/*: Review workflows as security-sensitive infrastructure.A good workflow is small, focused, and boring.
If a workflow is clever, generic, or overly flexible, it is a risk.
PRIORITY 0 — ABSOLUTE REQUIREMENTS
- All third-party actions MUST be pinned to full commit SHAs, similar to other workflows.
permissions:MUST be explicitly declared and minimally scoped.- Workflows MUST behave safely when executed from forks.
- YAML MUST orchestrate steps, not implement business logic.
- Any workflow that mutates GitHub state MUST support dry-run mode.
- Dry-run behavior must be explicit and visible in logs.
- Workflows MUST NOT modify repository source code outside
.github/.
PRIORITY 1 — SCOPE, FOCUS & RESTRAINT
- The title of each workflow must be relevant, match similar naming schemes, and match its script filename.
- Each workflow MUST have a single, clearly defined objective and SHOULD document this in a top-level comment.
- Flag workflows that:
- Attempt to be generic “frameworks”
- Include speculative or future-facing logic
- Perform actions unrelated to the stated goal
- Over-abstraction and excess flexibility are maintenance risks.
PRIORITY 2 — INPUT HARDENING
- Treat ALL GitHub event data as potentially hostile input, including:
- issue titles, bodies, and comments
- labels, usernames, branch names
- Free-form user input MUST NOT be passed directly into:
- shell commands
- gh CLI arguments
- Node.js exec / spawn calls
- Require strict allowlists or exact string matches.
- Flag any use of:
- eval or bash -c
- backticks or $(...) with user-controlled input
------------------...
Files:
.github/workflows/bot-workflows.yml
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~97-~97: The official name of this software platform is spelled with a capital “H”.
Context: ...t-workflows.ymlto use dedicated script.github/scripts/bot-workflows.sh` for improved ...
(GITHUB)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (9)
CHANGELOG.md (1)
97-97: LGTM!The changelog entry accurately documents the dry-run support addition and script refactoring. The static analysis hint about "github" casing is a false positive—it's correctly lowercase as part of a filename path.
.github/scripts/bot-workflows.sh (3)
1-2: LGTM!Correct use of
set -euo pipefailand proper shebang. As per coding guidelines for shell scripts.
9-29: LGTM!Good environment variable initialization with fallbacks and robust DRY_RUN normalization that handles multiple input formats (true/false/1/0) case-insensitively.
43-47: LGTM!Excellent input validation—ensures
FAILED_RUN_IDis numeric to prevent injection risks. This defensive check aligns with coding guidelines for validating untrusted input..github/workflows/bot-workflows.yml (5)
4-11: LGTM!Good workflow_dispatch inputs with dry_run defaulting to true for safe testing, as required by coding guidelines: "Workflows that mutate state MUST expose workflow_dispatch with dry_run default true."
20-25: LGTM!Permissions are explicitly scoped to minimum required (contents: read, pull-requests: write), and concurrency is properly defined with cancel-in-progress. Follows coding guidelines for workflows.
36-39: LGTM!harden-runner action correctly pinned to full commit SHA with version comment for reference. As per coding guidelines PRIORITY 0.
41-43: LGTM!Checkout action correctly pinned to full commit SHA, satisfying the PRIORITY 0 requirement that all third-party actions be SHA-pinned.
44-52: LGTM!Good extraction of inline logic to the dedicated script. All required environment variables are properly passed. The condition at line 45 appropriately prevents execution during manual dispatch without a failed_run_id.
| env: | ||
| # Behaviour: | ||
| # - workflow_run: DRY_RUN = 0 (real actions) | ||
| # - workflow_dispatch: DRY_RUN derived from the "dry_run" input | ||
| DRY_RUN: ${{ github.event_name == 'workflow_dispatch' && (github.event.inputs.dry_run == 'true' && '1' || '0') || '0' }} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider simplifying the DRY_RUN expression for readability.
The nested ternary expression is correct but dense:
DRY_RUN: ${{ github.event_name == 'workflow_dispatch' && (github.event.inputs.dry_run == 'true' && '1' || '0') || '0' }}While functionally sound, a comment explaining the logic would improve maintainability.
📝 Add clarifying comment
env:
- # Behaviour:
- # - workflow_run: DRY_RUN = 0 (real actions)
- # - workflow_dispatch: DRY_RUN derived from the "dry_run" input
+ # DRY_RUN behavior by event type:
+ # - workflow_run: always 0 (real actions - post comments)
+ # - workflow_dispatch with dry_run=true: 1 (simulate only)
+ # - workflow_dispatch with dry_run=false: 0 (real actions)
DRY_RUN: ${{ github.event_name == 'workflow_dispatch' && (github.event.inputs.dry_run == 'true' && '1' || '0') || '0' }}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/scripts/bot-workflows.sh
🧰 Additional context used
📓 Path-based instructions (1)
.github/scripts/**/*.sh
⚙️ CodeRabbit configuration file
.github/scripts/**/*.sh: Treat shell scripts as production-grade automation.Scripts should be small, focused, and explicit.
Avoid “do-everything” or overly generic scripts.
- MUST use:
set -euo pipefail- MUST validate all required environment variables
- MUST defensively quote variables
- MUST validate all untrusted input
- MUST have bounded loops and pagination
- MUST support dry-run mode if state is mutated
- MUST log key decisions and early exits
Files:
.github/scripts/bot-workflows.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
.github/scripts/bot-workflows.sh (7)
1-8: LGTM!The script correctly uses
set -euo pipefailas required by the coding guidelines, and the header documentation clearly explains the dry-run behavior.
18-29: LGTM!Good input normalization with case-insensitive matching and proper cleanup of the
nocasematchshell option.
31-62: LGTM!Good validation logic with appropriate handling for dry-run mode. The numeric validation on
FAILED_RUN_ID(line 44) is a solid security practice to prevent injection.
64-70: LGTM!Clear logging banner that aids debugging and aligns with the coding guideline to "log key decisions."
72-80: LGTM!Good defensive check for
ghCLI availability. The warning-only approach for auth status is reasonable since subsequent commands will provide clearer errors if auth is actually required and missing.
114-132: LGTM!The marker-based approach for duplicate detection is clean, and the heredoc keeps the multi-line comment readable.
163-186: LGTM!Clean separation between dry-run simulation and real posting, with proper error handling on the
gh pr commentcall.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/scripts/bot-workflows.sh
🧰 Additional context used
📓 Path-based instructions (1)
.github/scripts/**/*.sh
⚙️ CodeRabbit configuration file
.github/scripts/**/*.sh: Treat shell scripts as production-grade automation.Scripts should be small, focused, and explicit.
Avoid “do-everything” or overly generic scripts.
- MUST use:
set -euo pipefail- MUST validate all required environment variables
- MUST defensively quote variables
- MUST validate all untrusted input
- MUST have bounded loops and pagination
- MUST support dry-run mode if state is mutated
- MUST log key decisions and early exits
Files:
.github/scripts/bot-workflows.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
.github/scripts/bot-workflows.sh (4)
1-2: LGTM! Proper shell script foundation.The script correctly uses
set -euo pipefailas required by coding guidelines, ensuring it exits on errors, undefined variables, and pipeline failures.
82-112: LGTM! PR lookup logic is well-implemented.The script correctly:
- Uses
|| echo ""to handle command failures gracefully- Provides informative error messages
- Exits gracefully in dry-run mode when data is unavailable
- Properly quotes all variables
- Differentiates between dry-run and real execution appropriately
114-132: LGTM! Comment template is well-structured.The notification message includes:
- A unique marker for duplicate detection
- Clear explanation of the issue
- Helpful documentation links
- Professional tone
163-191: LGTM! Posting logic correctly implements dry-run mode.The script properly:
- Differentiates between dry-run and real execution
- Checks for duplicates before posting
- Provides verbose logging for dry-run mode
- Handles posting errors appropriately
- Logs completion status
This aligns with the coding guideline: "MUST support dry-run mode if state is mutated" and "MUST log key decisions and early exits."
| # Normalise DRY_RUN input ("true"/"false" -> 1/0, case-insensitive) | ||
| shopt -s nocasematch | ||
| case "$DRY_RUN" in | ||
| 1|0) ;; | ||
| "true") DRY_RUN=1 ;; | ||
| "false") DRY_RUN=0 ;; | ||
| *) | ||
| echo "ERROR: DRY_RUN must be one of: true, false, 1, 0 (got: $DRY_RUN)" | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| shopt -u nocasematch |
There was a problem hiding this comment.
Quote the variable in the error message.
Line 25 should quote $DRY_RUN to prevent word splitting and ensure proper error output.
🔧 Proposed fix
- echo "ERROR: DRY_RUN must be one of: true, false, 1, 0 (got: $DRY_RUN)"
+ echo "ERROR: DRY_RUN must be one of: true, false, 1, 0 (got: '$DRY_RUN')"📝 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.
| # Normalise DRY_RUN input ("true"/"false" -> 1/0, case-insensitive) | |
| shopt -s nocasematch | |
| case "$DRY_RUN" in | |
| 1|0) ;; | |
| "true") DRY_RUN=1 ;; | |
| "false") DRY_RUN=0 ;; | |
| *) | |
| echo "ERROR: DRY_RUN must be one of: true, false, 1, 0 (got: $DRY_RUN)" | |
| exit 1 | |
| ;; | |
| esac | |
| shopt -u nocasematch | |
| # Normalise DRY_RUN input ("true"/"false" -> 1/0, case-insensitive) | |
| shopt -s nocasematch | |
| case "$DRY_RUN" in | |
| 1|0) ;; | |
| "true") DRY_RUN=1 ;; | |
| "false") DRY_RUN=0 ;; | |
| *) | |
| echo "ERROR: DRY_RUN must be one of: true, false, 1, 0 (got: '$DRY_RUN')" | |
| exit 1 | |
| ;; | |
| esac | |
| shopt -u nocasematch |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/scripts/bot-workflows.sh
🧰 Additional context used
📓 Path-based instructions (1)
.github/scripts/**/*.sh
⚙️ CodeRabbit configuration file
.github/scripts/**/*.sh: Treat shell scripts as production-grade automation.Scripts should be small, focused, and explicit.
Avoid “do-everything” or overly generic scripts.
- MUST use:
set -euo pipefail- MUST validate all required environment variables
- MUST defensively quote variables
- MUST validate all untrusted input
- MUST have bounded loops and pagination
- MUST support dry-run mode if state is mutated
- MUST log key decisions and early exits
Files:
.github/scripts/bot-workflows.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
.github/scripts/bot-workflows.sh (7)
1-2: LGTM! Proper error handling with set -euo pipefail.Follows the coding guidelines requirement for production-grade shell scripts. This ensures the script exits on any error, undefined variable, or pipe failure.
9-30: LGTM! Robust environment variable setup and normalization.The fallback pattern for
GH_TOKEN, the safe default to dry-run mode, and the flexible normalization ofDRY_RUN(accepting "true"/"false"/1/0) are all well-implemented.
31-62: LGTM! Well-designed conditional validation for dry-run support.The validation logic correctly distinguishes between dry-run and real execution modes:
- In real mode (DRY_RUN=0): Strict validation ensures all required inputs are present
- In dry-run mode (DRY_RUN=1): Demo defaults allow testing without real values
The numeric validation for
FAILED_RUN_ID(line 44) prevents injection attacks. TheREPOvariable is always validated since it's needed for both modes.
64-80: LGTM! Appropriate logging and tool availability checks.The initialization logging provides clear visibility into the script's configuration. The
ghCLI availability check correctly exits on failure, while the auth check provides a helpful warning without being overly strict (subsequent commands will fail if auth is actually broken).
82-110: LGTM! Robust PR lookup with proper error handling.The branch and PR lookup logic is well-implemented:
- Variables are properly quoted throughout
- Fallback handling (
|| echo "") prevents premature script exit- Graceful exits in dry-run mode when data is unavailable
- Clear logging for all decision paths
114-132: LGTM! Well-structured notification message.The heredoc approach for building the multi-line comment is clean and maintainable. The message includes helpful links to documentation and is professional in tone.
163-186: LGTM! Excellent dry-run and posting logic.The implementation properly separates dry-run simulation from real execution:
- Dry-run mode provides full visibility into intended actions
- Real mode has proper error handling and success/failure logging
- All variables in the
gh pr commentcommand are properly quoted- Exits with appropriate error codes
Follows the coding guideline requirement: "MUST support dry-run mode if state is mutated."
Signed-off-by: Mounil <[email protected]>
…#1288) Signed-off-by: Mounil <[email protected]>
…dger#1288) Signed-off-by: Mounil <[email protected]>
…dger#1288) Signed-off-by: Mounil <[email protected]>
…iero-ledger#1288) Signed-off-by: Mounil <[email protected]>
…-run consistency (hiero-ledger#1288) Signed-off-by: Mounil <[email protected]>
Signed-off-by: Mounil <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Mounil Kankhara <[email protected]>
…ate-mutating ops (hiero-ledger#1288) Signed-off-by: Mounil <[email protected]>
Mounil2005
force-pushed
the
feature/bot-workflows-dry-run
branch
from
781908a to
22267cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/bot-workflows.yml (1)
20-25: Permissions and concurrency group need hardening forworkflow_dispatchtrigger and GitHub API operations.
- Concurrency:
github.event.workflow_run.head_branchis undefined when triggered viaworkflow_dispatch, producing an empty concurrency group. Manual workflow runs will collide and cancel each other.- Permissions: Missing
actions: read(required bygh run viewat line 98) andissues: read/issues: write(required by issue comments API at lines 153–155 andgh pr commentat line 192). Current permissions only declarepull-requests: write.Proposed fix
permissions: contents: read - pull-requests: write + actions: read + issues: write + pull-requests: read concurrency: - group: "workflow-failure-${{ github.event.workflow_run.head_branch }}" + group: >- + workflow-failure-${{ github.event_name == 'workflow_run' + && github.event.workflow_run.head_branch + || format('manual-{0}', github.run_id) }} cancel-in-progress: true
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/scripts/bot-workflows.sh.github/workflows/bot-workflows.ymlCHANGELOG.md
🧰 Additional context used
📓 Path-based instructions (2)
.github/scripts/**/*.sh
⚙️ CodeRabbit configuration file
.github/scripts/**/*.sh: Treat shell scripts as production-grade automation.Scripts should be small, focused, and explicit.
Avoid “do-everything” or overly generic scripts.
- MUST use:
set -euo pipefail- MUST validate all required environment variables
- MUST defensively quote variables
- MUST validate all untrusted input
- MUST have bounded loops and pagination
- MUST support dry-run mode if state is mutated
- MUST log key decisions and early exits
Files:
.github/scripts/bot-workflows.sh
.github/workflows/**/*
⚙️ CodeRabbit configuration file
.github/workflows/**/*: Review workflows as security-sensitive infrastructure.A good workflow is small, focused, and boring.
If a workflow is clever, generic, or overly flexible, it is a risk.
PRIORITY 0 — ABSOLUTE REQUIREMENTS
- All third-party actions MUST be pinned to full commit SHAs, similar to other workflows.
permissions:MUST be explicitly declared and minimally scoped.- Workflows MUST behave safely when executed from forks.
- YAML MUST orchestrate steps, not implement business logic.
- Any workflow that mutates GitHub state MUST support dry-run mode.
- Dry-run behavior must be explicit and visible in logs.
- Workflows MUST NOT modify repository source code outside
.github/.
PRIORITY 1 — SCOPE, FOCUS & RESTRAINT
- The title of each workflow must be relevant, match similar naming schemes, and match its script filename.
- Each workflow MUST have a single, clearly defined objective and SHOULD document this in a top-level comment.
- Flag workflows that:
- Attempt to be generic “frameworks”
- Include speculative or future-facing logic
- Perform actions unrelated to the stated goal
- Over-abstraction and excess flexibility are maintenance risks.
PRIORITY 2 — INPUT HARDENING
- Treat ALL GitHub event data as potentially hostile input, including:
- issue titles, bodies, and comments
- labels, usernames, branch names
- Free-form user input MUST NOT be passed directly into:
- shell commands
- gh CLI arguments
- Node.js exec / spawn calls
- Require strict allowlists or exact string matches.
- Flag any use of:
- eval or bash -c
- backticks or $(...) with user-controlled input
------------------...
Files:
.github/workflows/bot-workflows.yml
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~99-~99: The official name of this software platform is spelled with a capital “H”.
Context: ...t-workflows.ymlto use dedicated script.github/scripts/bot-workflows.sh` for improved ...
(GITHUB)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
.github/scripts/bot-workflows.sh (2)
127-145: Confirm desired idempotency semantics: this marker blocks all future notifications for the PR.Because
MARKERis constant, once posted it will suppress any later “workflow failed again” notifications for that PR, regardless of run ID/workflow. If that’s intended (“at most one comment per PR”), great—otherwise include workflow/run info in the marker.
80-94: Thegh auth statuscheck is not fragile in GitHub Actions whenGH_TOKENis properly set (as it is here). The documented behavior and web sources confirm thatgh auth statusrespects theGH_TOKENenvironment variable. The existing code already handles failures defensively: it only warns in dry-run mode and exits in non-dry-run mode, which is appropriate. The proposed fix usinggh api /useris not documented as a recommended practice and would waste API quota.
| env: | ||
| # Behaviour: | ||
| # - workflow_run: DRY_RUN = 0 (real actions) | ||
| # - workflow_dispatch: DRY_RUN derived from the "dry_run" input | ||
| DRY_RUN: ${{ github.event_name == 'workflow_dispatch' && (github.event.inputs.dry_run == 'true' && '1' || '0') || '0' }} | ||
| steps: |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
git ls-files .github/workflows/ | head -20Repository: hiero-ledger/hiero-sdk-python
Length of output: 1026
🏁 Script executed:
fd "bot-workflows" .github/Repository: hiero-ledger/hiero-sdk-python
Length of output: 141
🏁 Script executed:
cat -n .github/workflows/bot-workflows.yml | head -50Repository: hiero-ledger/hiero-sdk-python
Length of output: 2271
🌐 Web query:
GitHub Actions workflow_dispatch boolean input type github.event.inputs
💡 Result:
Short answer: github.event.inputs values are strings (so boolean inputs arrive as 'true'/'false'). Use the inputs context (inputs.) to get preserved Boolean values, or explicitly convert/compare (e.g. github.event.inputs.foo == 'true' or fromJson) when you must read github.event.inputs. [1][2][3][4]
References
- Workflow docs: inputs context preserves booleans; github.event.inputs is stringy. [1][2]
- Changelog (input types added). [3]
- Community/issue discussion and recommended workarounds (compare to 'true' / fromJson). [4][5]
🌐 Web query:
GitHub Actions inputs vs github.event.inputs workflow_dispatch
💡 Result:
Short answer
-
Use inputs (e.g. ${{ inputs.foo }}) — it's the unified, preferred context for values defined in on.workflow_dispatch and on.workflow_call. It preserves boolean/number types for workflow_dispatch and works for reusable workflows. [1][2][3][4]
-
github.event.inputs (e.g. ${{ github.event.inputs.foo }}) is the older event-payload location kept for backward compatibility; values there are (historically) stringified for workflow_dispatch. Prefer inputs when possible. [1][2][3]
Quick example
- workflow file:
on:
workflow_dispatch:
inputs:
flag:
type: boolean - access:
- ${{ inputs.flag }} # boolean
- ${{ github.event.inputs.flag }} # kept for compatibility (may be stringified). [1][2][4]
Sources
[1] GitHub Docs — Triggering a workflow (workflow_dispatch inputs note).
[2] GitHub Docs — Workflow syntax (on.workflow_dispatch, inputs note).
[3] GitHub changelog — Inputs unified across manual and reusable workflows (June 2022).
[4] GitHub Docs — Contexts reference (inputs context).
Use the inputs context instead of github.event.inputs for boolean input access.
For workflow_dispatch with a typed boolean input, github.event.inputs.dry_run is a string ('true'/'false'), not a boolean. The inputs context preserves the actual boolean type and is the preferred way to access workflow inputs. This simplifies the expression while maintaining the 1/0 contract for the script.
Proposed fix
env:
# Behaviour:
# - workflow_run: DRY_RUN = 0 (real actions)
# - workflow_dispatch: DRY_RUN derived from the "dry_run" input
- DRY_RUN: ${{ github.event_name == 'workflow_dispatch' && (github.event.inputs.dry_run == 'true' && '1' || '0') || '0' }}
+ DRY_RUN: ${{ github.event_name == 'workflow_dispatch' && (inputs.dry_run && '1' || '0') || '0' }}| - name: Notify PR of workflow failure | ||
| if: github.event_name != 'workflow_dispatch' || github.event.inputs.failed_run_id != '' | ||
| env: | ||
| FAILED_WORKFLOW_NAME: ${{ github.event.workflow_run.name || 'Manual Test Run' }} | ||
| FAILED_RUN_ID: ${{ github.event.inputs.failed_run_id || github.event.workflow_run.id }} | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: | | ||
| REPO="${{ github.repository }}" | ||
| COMMENT=$(cat <<EOF | ||
| Hi, this is WorkflowBot. | ||
| Your pull request cannot be merged as it is not passing all our workflow checks. | ||
| Please click on each check to review the logs and resolve issues so all checks pass. | ||
| To help you: | ||
| - [DCO signing guide](https://github.com/hiero-ledger/hiero-sdk-python/blob/main/docs/sdk_developers/signing.md) | ||
| - [Changelog guide](https://github.com/hiero-ledger/hiero-sdk-python/blob/main/docs/sdk_developers/changelog_entry.md) | ||
| - [Merge conflicts guide](https://github.com/hiero-ledger/hiero-sdk-python/blob/main/docs/sdk_developers/merge_conflicts.md) | ||
| - [Rebase guide](https://github.com/hiero-ledger/hiero-sdk-python/blob/main/docs/sdk_developers/rebasing.md) | ||
| - [Testing guide](https://github.com/hiero-ledger/hiero-sdk-python/blob/main/docs/sdk_developers/testing.md) | ||
| - [Discord](https://github.com/hiero-ledger/hiero-sdk-python/blob/main/docs/discord.md) | ||
| - [Community Calls](https://zoom-lfx.platform.linuxfoundation.org/meetings/hiero?view=week) | ||
| Thank you for contributing! | ||
| From the Hiero Python SDK Team | ||
| EOF | ||
| ) | ||
| EXISTING_COMMENT=$(gh pr view "$PR_NUMBER" --repo "$REPO" --comments \ | ||
| --json comments --jq ".comments[] | select(.body == \"${COMMENT//\"/\\\"}\") | .id") | ||
| if [ -z "$EXISTING_COMMENT" ]; then | ||
| gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$COMMENT" | ||
| else | ||
| echo "Comment already exists, skipping." | ||
| fi | ||
| REPO: ${{ github.repository }} | ||
| DRY_RUN: ${{ env.DRY_RUN }} | ||
| run: bash .github/scripts/bot-workflows.sh |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
GitHub workflow_run event payload pull_requests array documentation
💡 Result:
Yes — the workflow_run webhook payload includes a pull_requests array (an array of pull request objects). It typically contains PR fields such as number, base.ref, head.ref, and sometimes base.sha; the array represents pull requests whose head_sha/head_branch matches the workflow run (and can be empty). See GitHub's webhook payload docs and related notes/examples. [1][2][3]
Sources
- GitHub Webhook events & payloads — workflow_run (pull_requests array). [1]
- GitHub Community discussion with example fields (number, base.ref, head.ref, base.sha). [2]
- workflow_runs API / docs noting pull_requests description. [3]
🏁 Script executed:
# Find and examine the bot-workflows.sh script to understand current PR lookup
find .github -name "bot-workflows.sh" -type fRepository: hiero-ledger/hiero-sdk-python
Length of output: 105
🏁 Script executed:
cat -n .github/scripts/bot-workflows.shRepository: hiero-ledger/hiero-sdk-python
Length of output: 8310
Pass PR number from the workflow_run payload to reduce API calls and simplify fork behavior.
If github.event.workflow_run.pull_requests[0].number is present, pass it as PR_NUMBER env var to the script. This eliminates two API calls (gh run view and gh pr list) and uses the event payload directly, which is more reliable in fork scenarios.
Update both the workflow env var (as shown in the diff) and the script to accept and prioritize PR_NUMBER if provided, falling back to the current branch-based lookup only when empty.
Workflow change
- name: Notify PR of workflow failure
if: github.event_name != 'workflow_dispatch' || github.event.inputs.failed_run_id != ''
env:
FAILED_WORKFLOW_NAME: ${{ github.event.workflow_run.name || 'Manual Test Run' }}
FAILED_RUN_ID: ${{ github.event.inputs.failed_run_id || github.event.workflow_run.id }}
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
REPO: ${{ github.repository }}
DRY_RUN: ${{ env.DRY_RUN }}
+ PR_NUMBER: ${{ github.event.workflow_run.pull_requests[0].number || '' }}
run: bash .github/scripts/bot-workflows.sh| - Added technical docstrings and hardening (set -euo pipefail) to the pr-check-test-files.sh script (#1336) | ||
| - Added prompt for coderabbit to review `Query` and it's sub-classes. | ||
|
|
||
| - Added dry-run support and refactored `bot-workflows.yml` to use dedicated script `.github/scripts/bot-workflows.sh` for improved maintainability and testability. (#1288) |
There was a problem hiding this comment.
Prefer the full workflow path in the changelog entry for clarity.
Consider referencing .github/workflows/bot-workflows.yml (not just bot-workflows.yml) so readers can locate it unambiguously.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~99-~99: The official name of this software platform is spelled with a capital “H”.
Context: ...t-workflows.ymlto use dedicated script.github/scripts/bot-workflows.sh` for improved ...
(GITHUB)
Signed-off-by: Mounil <[email protected]>
|
HI @Mounil2005 seen this. |
That's alright! |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/scripts/bot-workflows.sh
🧰 Additional context used
📓 Path-based instructions (1)
.github/scripts/**/*.sh
⚙️ CodeRabbit configuration file
.github/scripts/**/*.sh: Treat shell scripts as production-grade automation.Scripts should be small, focused, and explicit.
Avoid “do-everything” or overly generic scripts.
- MUST use:
set -euo pipefail- MUST validate all required environment variables
- MUST defensively quote variables
- MUST validate all untrusted input
- MUST have bounded loops and pagination
- MUST support dry-run mode if state is mutated
- MUST log key decisions and early exits
Files:
.github/scripts/bot-workflows.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (6)
.github/scripts/bot-workflows.sh (6)
1-2: LGTM! Proper strict mode enabled.The script correctly uses
set -euo pipefailas required by the coding guidelines for production-grade automation.
80-98: LGTM! Comprehensive prerequisite checks.The script properly validates that required tools (
gh,jq) are available and handles authentication checks appropriately for both dry-run and live modes.
100-128: LGTM! Robust PR lookup with appropriate error handling.The branch-based PR lookup correctly handles edge cases (missing runs, no associated PRs) and gracefully exits when no notification is needed. The dry-run tolerance is well-implemented.
132-150: LGTM! Well-structured notification message.The comment includes a unique marker for duplicate detection and provides helpful documentation links. The heredoc usage is appropriate for multi-line content.
152-175: LGTM! Excellent pagination with proper bounds.The duplicate detection correctly implements:
- Bounded pagination (MAX_PAGES) as required by coding guidelines
- Per-page marker search avoiding invalid JSON concatenation
- Proper empty-page detection for early exit
- Safe jq usage with existence checks
177-204: LGTM! Proper dry-run support and error handling.The posting logic correctly:
- Respects dry-run mode as required by coding guidelines
- Logs key decisions (duplicate detection, posting actions)
- Handles posting failures with appropriate error codes
- Prevents duplicate notifications
| if [[ -z "$REPO" ]]; then | ||
| echo "ERROR: REPO environment variable not set." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider validating REPO format.
The script assumes REPO follows the owner/repo format required by GitHub CLI commands (lines 103, 118, 158, 197). Without validation, malformed input could cause obscure failures later in the script.
🔍 Proposed format validation
if [[ -z "$REPO" ]]; then
echo "ERROR: REPO environment variable not set."
exit 1
fi
+
+# Validate REPO format (must be owner/repo)
+if ! [[ "$REPO" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]]; then
+ echo "ERROR: REPO must be in 'owner/repo' format (got: '$REPO')"
+ exit 1
+fi📝 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.
| if [[ -z "$REPO" ]]; then | |
| echo "ERROR: REPO environment variable not set." | |
| exit 1 | |
| fi | |
| if [[ -z "$REPO" ]]; then | |
| echo "ERROR: REPO environment variable not set." | |
| exit 1 | |
| fi | |
| # Validate REPO format (must be owner/repo) | |
| if ! [[ "$REPO" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]]; then | |
| echo "ERROR: REPO must be in 'owner/repo' format (got: '$REPO')" | |
| exit 1 | |
| fi |
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
Hi @Mounil2005 some ideas to make this clearer, as there is quite a lot going on now
| Your pull request cannot be merged as it is not passing all our workflow checks. | ||
| Please click on each check to review the logs and resolve issues so all checks pass. | ||
| To help you: | ||
| - [DCO signing guide](https://github.com/hiero-ledger/hiero-sdk-python/blob/main/docs/sdk_developers/signing.md) |
There was a problem hiding this comment.
We should move these links to the top so we can edit them later easily, rather than have them hard coded in the script
|
|
||
| if ! gh auth status >/dev/null 2>&1; then | ||
| if (( DRY_RUN == 0 )); then | ||
| echo "ERROR: gh authentication required for non-dry-run mode." |
There was a problem hiding this comment.
If you are refactoring, i would recommend to use javascript and github action, will simplify a lot
There was a problem hiding this comment.
Do you want me to completely change it into js and github action or only the particular thing and sort of hybrid?
There was a problem hiding this comment.
All as JS is my suggestion - not essential - but if you are refactoring, this will typically lead to better code
| echo "Found PR #$PR_NUMBER" | ||
|
|
||
| # Build notification message with failure details and documentation links | ||
| MARKER="<!-- workflowbot:workflow-failure-notifier -->" |
There was a problem hiding this comment.
this should be at the top so we know we can change it
aceppaluni
left a comment
aceppaluni
left a comment
There was a problem hiding this comment.
@Mounil2005 This is awesome work!!
I concur with @exploreriii suggested changes.
If you need assistance with the bash script, please let us know. We are happy to help :)
@exploreriii What is your opinion on the changelog entry?
3 participants
Description:
Add dry-run support and refactor bot-workflows.yml for improved maintainability and safety.
Related issue(s):
Fixes #1288
Notes for reviewer:
This PR addresses two CodeRabbit bot findings:
The workflow now supports safe testing through dry-run mode and has cleaner, more maintainable code structure following established project patterns from other bot workflows.
Checklist