Conversation
…d of COMP_NAME The workflow was using COMP_NAME to build configmap/secret template paths (e.g. templates/plugin-br-pix-indirect-btg-worker-inbound/configmap.yaml) but the actual directory structure uses VALUES_KEY names (e.g. templates/inbound/configmap.yaml). This caused the if [ -f ] check to silently fail, resulting in detected env vars never being injected into configmap/secret templates. Changes: - Use VALUES_KEY for CONFIGMAP_FILE and SECRET_FILE paths - Update create_secret_template to take VALUES_KEY as single arg - Add ::warning:: annotations when template files are not found Closes #167
lerian-studio
requested changes
Mar 25, 2026
lerian-studio
left a comment
lerian-studio
left a comment
There was a problem hiding this comment.
Pull requests to main can only come from:
develophotfix/*
Your source branch: fix/helm-update-chart-path-bug
Please change the base branch or create a PR from an allowed branch.
github-actions
bot
added
the
workflow
🔍 Lint Analysis
❌ Failures (3)Action Lint
Pinned Actions
Shell Check
|
🛡️ CodeQL Analysis ResultsLanguages analyzed: Found 1 issue(s): 1 Medium
🔍 View full scan logs | 🛡️ Security tab |
|
Caution Review failedPull request was closed or merged during review WalkthroughUpdated the reusable workflow to resolve template paths using Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
bedatty
approved these changes
Mar 25, 2026
Contributor
There was a problem hiding this comment.
LGTM — surgical and correct fix.
Review summary:
- Template paths fixed from
COMP_NAME→VALUES_KEY(root cause of the bug) create_secret_template()simplified from 2 params to 1 (removes fallback that masked the issue)::warning::added inelsebranches to prevent silent failures going forward
No side effects — VALUES_KEY was already used correctly for tag bumps and .Values.* references.
…eferences Resolves SC2086 (double quote to prevent globbing) and SC2129 (group redirects) shellcheck warnings flagged by the PR lint analysis.
lerian-studio
added
size/S
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Pin crazy-max/ghaction-import-gpg and mikefarah/yq to commit SHAs - Move inputs.base_branch to env var to prevent code injection in step summary - Add inline comment dismissing untrusted-checkout false positive
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/helm-update-chart.yml:
- Line 405: The workflow is emitting a dead output named commit_msg (echo
"commit_msg=${COMMIT_MSG}" >> "$GITHUB_OUTPUT") while the subsequent step only
assigns it to COMMIT_MSG and never uses it, so either remove the output emission
and the unused assignment to COMMIT_MSG to eliminate the dead-variable (SC2034)
or keep the output and actually consume COMMIT_MSG in a later step; locate the
echo line that writes commit_msg to GITHUB_OUTPUT and the step that assigns
COMMIT_MSG and either delete both or change downstream steps to reference
COMMIT_MSG so the output is used.
- Around line 244-247: The function create_secret_template builds paths using
the caller-supplied values_key and can be exploited with "../" or absolute
paths; validate values_key against a strict single path-segment slug regex (e.g.
allow only ASCII letters, digits, dot, underscore, hyphen: ^[A-Za-z0-9._-]+$),
and abort with a non-zero exit/explicit error if it fails validation before
constructing secret_file or any other path using TEMPLATES_BASE and values_key;
apply the same validation check wherever values_key is used to build files (the
other create_* template calls that reference values_key) to ensure no
multi-segment, parent-traversal, or absolute paths are allowed.
- Around line 139-147: The exported workflow outputs (CHART, HAS_NEW_ENV_VARS,
SOURCE_REF, SOURCE_REPO, SOURCE_ACTOR, SOURCE_SHA, BRANCH_NAME) are written
using plain key=value which is vulnerable to newline injection; change the
export to GitHub's multiline output format (e.g., write "VAR<<EOF", then the
variable value, then "EOF" for each variable) when appending to GITHUB_OUTPUT to
safely preserve newlines. Also validate and sanitize VALUES_KEY (reject or fail
on values containing ../, /, or other path-traversal characters and any
characters unsafe for yq expressions) before using it in filesystem paths or yq
commands so callers cannot traverse directories or inject expressions.
- Around line 479-483: The workflow currently injects the reusable input `${{
inputs.base_branch }}` directly into the bash script (used in the echo lines
that write to GITHUB_STEP_SUMMARY), which allows command-substitution vectors
like `$(id)` from untrusted callers; fix it by moving `inputs.base_branch` into
the job/step `env:` (e.g. set `env: BASE_BRANCH: ${{ inputs.base_branch }}`) and
then reference the environment variable (`"$BASE_BRANCH"`) in the echo that
writes the Base line (use `"$BASE_BRANCH"` instead of ```${{ inputs.base_branch
}}```); keep the other variables (CHART, BRANCH_NAME) usage as-is and ensure all
echo arguments are quoted to avoid further word-splitting or substitution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 87e4f603-27a6-4f54-88ca-23826eb10e85
📒 Files selected for processing (1)
.github/workflows/helm-update-chart.yml
lerian-studio
added
size/S
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/helm-update-chart.yml (1)
17-27:⚠️ Potential issue | 🟠 MajorMissing
workflow_dispatchanddry_runinput.Per coding guidelines, reusable workflows must:
- Support
workflow_dispatchfor manual testing- Include a
dry_runboolean input (default: false) for workflows that apply state changesThis workflow commits changes, pushes branches, and creates PRs—all state-changing operations that would benefit from dry-run capability.
Add required triggers and input
on: workflow_call: inputs: + dry_run: + description: 'Run in dry-run mode without creating commits/PRs (default: false)' + type: boolean + default: false payload: description: 'JSON payload with chart, components, and metadata' # ... rest of inputs + workflow_dispatch: + inputs: + dry_run: + description: 'Run in dry-run mode without creating commits/PRs' + type: boolean + default: true + payload: + description: 'JSON payload with chart, components, and metadata' + type: string + required: true + # ... mirror other inputsAs per coding guidelines: "Always include a
dry_runinput in reusable workflows" and "Every reusable workflow must supportworkflow_callfor external callers andworkflow_dispatchfor manual testing."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/helm-update-chart.yml around lines 17 - 27, Add a manual trigger and a dry-run input: keep the existing workflow_call block (inputs.payload and inputs.base_branch) and add a top-level workflow_dispatch trigger to allow manual testing, and add an inputs.dry_run boolean (default: false) to the workflow_call inputs so callers can opt into non-mutating runs; ensure the workflow logic references inputs.dry_run where commits/branch pushes/PR creation occur to skip state-changing operations when true.
♻️ Duplicate comments (1)
.github/workflows/helm-update-chart.yml (1)
408-408:⚠️ Potential issue | 🟡 MinorDead output:
commit_msgis assigned but never used.Line 421 captures
COMMIT_MSGfrom this output, but that variable is never referenced afterwards. The pipeline failure (SC2034) is valid.Remove the dead output and assignment
- echo "commit_msg=${COMMIT_MSG}" >> "$GITHUB_OUTPUT" echo "Committing with message: ${COMMIT_MSG}" git commit -m "${COMMIT_MSG}"And at line 421:
- COMMIT_MSG="${{ steps.commit.outputs.commit_msg }}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/helm-update-chart.yml at line 408, Remove the dead output assignment echo "commit_msg=${COMMIT_MSG}" >> "$GITHUB_OUTPUT" and the corresponding capture of that output (the commit_msg/COMMIT_MSG variable) elsewhere in the workflow; specifically delete the echo that writes commit_msg to GITHUB_OUTPUT and any usage or assignment that reads commit_msg/COMMIT_MSG (the unused COMMIT_MSG capture) so the unused variable warning (SC2034) is resolved.
🤖 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/helm-update-chart.yml:
- Around line 484-503: The multiple echo lines writing to GITHUB_STEP_SUMMARY
(using variables CHART, BRANCH_NAME, BASE_BRANCH, HAS_CHANGES and the COMPONENTS
jq output) should be combined into a single redirected block to avoid repeated
redirection (fix SC2129); replace the series of individual echo >>
"$GITHUB_STEP_SUMMARY" calls with one grouped write (e.g., a here-doc or a
single cat <<'EOF' >> "$GITHUB_STEP_SUMMARY" ... EOF) that includes the header,
PR status conditional output (expand the HAS_CHANGES check into the block or
write the conditional result into a variable and include it), and the Components
table header, then append the jq-produced component rows in one grouped
redirection to GITHUB_STEP_SUMMARY so all lines are written with a single >>
operation.
---
Outside diff comments:
In @.github/workflows/helm-update-chart.yml:
- Around line 17-27: Add a manual trigger and a dry-run input: keep the existing
workflow_call block (inputs.payload and inputs.base_branch) and add a top-level
workflow_dispatch trigger to allow manual testing, and add an inputs.dry_run
boolean (default: false) to the workflow_call inputs so callers can opt into
non-mutating runs; ensure the workflow logic references inputs.dry_run where
commits/branch pushes/PR creation occur to skip state-changing operations when
true.
---
Duplicate comments:
In @.github/workflows/helm-update-chart.yml:
- Line 408: Remove the dead output assignment echo "commit_msg=${COMMIT_MSG}" >>
"$GITHUB_OUTPUT" and the corresponding capture of that output (the
commit_msg/COMMIT_MSG variable) elsewhere in the workflow; specifically delete
the echo that writes commit_msg to GITHUB_OUTPUT and any usage or assignment
that reads commit_msg/COMMIT_MSG (the unused COMMIT_MSG capture) so the unused
variable warning (SC2034) is resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5409ceee-8674-405e-9689-fbc6f0ef9d00
📒 Files selected for processing (1)
.github/workflows/helm-update-chart.yml
bedatty
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
All comments addressed. 1 valid (dead commit_msg output — will fix), 1 already fixed (base_branch injection), 2 dismissed (low-risk defense-in-depth for internal workflow_call).
Update all rules and commands (Claude, Cursor, AGENTS.md) to require third-party actions to be pinned by commit SHA instead of mutable tags. LerianStudio org actions remain pinned by release tag.
lerian-studio
added
size/S
github-actions
bot
added
documentation
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
3 participants
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.
Problem
The
helm-update-chartworkflow usesCOMP_NAMEto build template file paths:But the actual directory structure uses
VALUES_KEYnames. For example, the componentplugin-br-pix-indirect-btg-worker-inboundhasvalues_key: inbound, so the template lives attemplates/inbound/configmap.yaml, nottemplates/plugin-br-pix-indirect-btg-worker-inbound/configmap.yaml.The
if [ -f ]check returns false silently, so detected env vars are never injected.Found in PR #1143 (plugin-br-pix-indirect-btg v1.5.0) — 32 env vars detected but none inserted.
Fix
VALUES_KEYinstead ofCOMP_NAMEforCONFIGMAP_FILEandSECRET_FILEpathscreate_secret_templatefunction to takeVALUES_KEYas the single argument (used for both directory path and.Valuesreferences)::warning::annotations when template files are not found (no more silent failures)Closes #167
Summary by CodeRabbit
Bug Fixes
Refactor
Chores
Documentation