Skip to content

fix(ci): turn shellcheck on in workflow-lint and clear the 44 findings (#1130) #1148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
frankbria merged 1 commit into from
Aug 11, 2026
Merged

fix(ci): turn shellcheck on in workflow-lint and clear the 44 findings (#1130) #1148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/claude-code-review.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ jobs:
additions=${{ github.event.pull_request.additions }}
deletions=${{ github.event.pull_request.deletions }}
total=$((additions + deletions))
echo "total=$total" >> $GITHUB_OUTPUT
echo "total=$total" >> "$GITHUB_OUTPUT"

- name: Checkout repository
# Only review substantial changes (5+ files OR 20+ lines changed)
Expand Down
78 changes: 54 additions & 24 deletions .github/workflows/deploy.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,10 @@ jobs:
# Base64 encode to prevent any shell interpretation during transfer
ENV_BASE64=$(echo "$ENV_CONTENT" | base64 -w 0)

# Transfer and decode safely on remote, verify creation
# Transfer and decode safely on remote, verify creation.
# shellcheck disable=SC2029 # client-side expansion is the point:
# ENV_BASE64 and REMOTE_PATH exist only on the runner, and the remote
# side must receive their values, not their names.
ssh "${REMOTE_USER}@${REMOTE_HOST}" "
set -e
echo '${ENV_BASE64}' | base64 -d > '${REMOTE_PATH}/.env.staging.tmp'
Expand All @@ -204,6 +207,11 @@ jobs:
NEXT_PUBLIC_API_URL: ${{ secrets.API_URL }}
NEXT_PUBLIC_WS_URL: ${{ secrets.WS_URL }}
run: |
# shellcheck disable=SC2087 # client-side expansion is deliberate:
# ${NEXT_PUBLIC_*} and ${BACKEND_NAME}/${FRONTEND_NAME} are runner env
# and must be baked in here. The forms that must resolve on the server
# are escaped as \${...} below. Keep this heredoc backtick-free — an
# unquoted delimiter substitutes even '#' lines (#1130).
ssh ${{ secrets.USER }}@${{ secrets.HOST }} "bash -s" << ENDSSH
set -e
echo "🚀 Starting deployment to staging..."
Expand Down Expand Up @@ -261,11 +269,14 @@ jobs:
echo "Debug: FRONTEND_NAME='\${FRONTEND_NAME}'"

# Cold-start from the ecosystem file so its dotenv.config() re-reads
# .env.staging on every deploy. A plain `pm2 restart <name> --update-env`
# .env.staging on every deploy. A plain "pm2 restart NAME --update-env"
# refreshes env from the deploy shell (which never sources .env.staging),
# so newly-added vars like WORKSPACE_ROOT (#896) never reach the process
# and the backend crash-loops. Delete by name (NOT `pm2 delete all`,
# and the backend crash-loops. Delete by name (NOT "pm2 delete all",
# which would kill unrelated apps on this shared box), then start fresh.
# No backticks in here: this heredoc's delimiter is unquoted, so the
# runner's shell command-substitutes even inside a line starting with
# '#' — it is heredoc data, not a shell comment (#1130).
echo "🔁 (Re)starting PM2 services from config..."
pm2 delete "\${BACKEND_NAME}" "\${FRONTEND_NAME}" 2>/dev/null || true
pm2 start ecosystem.staging.config.js --update-env
Expand All @@ -282,15 +293,15 @@ jobs:
MAX_ATTEMPTS=12
SLEEP_SECONDS=5

for i in $(seq 1 $MAX_ATTEMPTS); do
for i in $(seq 1 "$MAX_ATTEMPTS"); do
echo "Health check attempt $i/$MAX_ATTEMPTS..."
if ssh ${{ secrets.USER }}@${{ secrets.HOST }} "curl -sf http://localhost:${{ secrets.API_PORT }}/health"; then
echo "✅ Health check passed on attempt $i"
exit 0
fi
if [ $i -lt $MAX_ATTEMPTS ]; then
if [ "$i" -lt "$MAX_ATTEMPTS" ]; then
echo "⏳ Waiting ${SLEEP_SECONDS}s before retry..."
sleep $SLEEP_SECONDS
sleep "$SLEEP_SECONDS"
fi
done

Expand All @@ -311,12 +322,14 @@ jobs:
# Branch names carry the same metacharacter risk as tags (#933).
BRANCH_NAME: ${{ github.ref_name }}
run: |
echo "## Staging Deployment Summary" >> $GITHUB_STEP_SUMMARY
echo "" >> $GITHUB_STEP_SUMMARY
echo "- **Branch**: $BRANCH_NAME" >> $GITHUB_STEP_SUMMARY
echo "- **Commit**: \`${{ github.sha }}\`" >> $GITHUB_STEP_SUMMARY
echo "- **Deployed by**: ${{ github.actor }}" >> $GITHUB_STEP_SUMMARY
echo "- **Time**: $(date -u '+%Y-%m-%d %H:%M:%S UTC')" >> $GITHUB_STEP_SUMMARY
{
echo "## Staging Deployment Summary"
echo ""
echo "- **Branch**: $BRANCH_NAME"
echo "- **Commit**: \`${{ github.sha }}\`"
echo "- **Deployed by**: ${{ github.actor }}"
echo "- **Time**: $(date -u '+%Y-%m-%d %H:%M:%S UTC')"
} >> "$GITHUB_STEP_SUMMARY"

# ============================================
# Deploy to Production
Expand Down Expand Up @@ -458,7 +471,10 @@ jobs:
# Base64 encode to prevent any shell interpretation during transfer
ENV_BASE64=$(echo "$ENV_CONTENT" | base64 -w 0)

# Transfer and decode safely on remote, verify creation
# Transfer and decode safely on remote, verify creation.
# shellcheck disable=SC2029 # client-side expansion is the point:
# ENV_BASE64 and REMOTE_PATH exist only on the runner, and the remote
# side must receive their values, not their names.
ssh "${REMOTE_USER}@${REMOTE_HOST}" "
set -e
echo '${ENV_BASE64}' | base64 -d > '${REMOTE_PATH}/.env.production.tmp'
Expand All @@ -475,6 +491,11 @@ jobs:

- name: Create pre-deployment backup
run: |
# shellcheck disable=SC2087 # client-side expansion is deliberate:
# ${NEXT_PUBLIC_*} and ${BACKEND_NAME}/${FRONTEND_NAME} are runner env
# and must be baked in here. The forms that must resolve on the server
# are escaped as \${...} below. Keep this heredoc backtick-free — an
# unquoted delimiter substitutes even '#' lines (#1130).
ssh ${{ secrets.USER }}@${{ secrets.HOST }} "bash -s" << ENDSSH
set -e
echo "💾 Creating pre-deployment backup..."
Expand Down Expand Up @@ -592,6 +613,11 @@ jobs:
# which this workflow does not control.
RELEASE_TAG_B64="$(printf '%s' "$RELEASE_TAG" | base64 | tr -d '\n')"

# shellcheck disable=SC2087 # client-side expansion is deliberate:
# ${RELEASE_TAG_B64} and the NEXT_PUBLIC_* vars are runner env and must
# be baked in here; what has to resolve server-side is escaped \${...}.
# Keep this heredoc backtick-free — an unquoted delimiter substitutes
# even '#' lines (#1130).
ssh ${{ secrets.USER }}@${{ secrets.HOST }} "bash -s" << ENDSSH
set -e
echo "🚀 Starting deployment to production..."
Expand Down Expand Up @@ -664,8 +690,10 @@ jobs:

# Cold-start from the ecosystem file so its dotenv.config() re-reads
# .env.production on every deploy — see the staging job for why a plain
# `pm2 restart <name>` silently drops newly-added .env vars (#896).
# Delete by name (NOT `pm2 delete all`) to spare unrelated apps.
# "pm2 restart NAME" silently drops newly-added .env vars (#896).
# Delete by name (NOT "pm2 delete all") to spare unrelated apps.
# No backticks: unquoted heredoc, so '#' lines are still substituted
# on the runner (#1130).
echo "🔁 (Re)starting PM2 services from config..."
pm2 delete "\${BACKEND_NAME}" "\${FRONTEND_NAME}" 2>/dev/null || true
pm2 start ecosystem.production.config.js --update-env
Expand All @@ -682,15 +710,15 @@ jobs:
MAX_ATTEMPTS=12
SLEEP_SECONDS=5

for i in $(seq 1 $MAX_ATTEMPTS); do
for i in $(seq 1 "$MAX_ATTEMPTS"); do
echo "Health check attempt $i/$MAX_ATTEMPTS..."
if ssh ${{ secrets.USER }}@${{ secrets.HOST }} "curl -sf http://localhost:${{ secrets.API_PORT }}/health"; then
echo "✅ Health check passed on attempt $i"
exit 0
fi
if [ $i -lt $MAX_ATTEMPTS ]; then
if [ "$i" -lt "$MAX_ATTEMPTS" ]; then
echo "⏳ Waiting ${SLEEP_SECONDS}s before retry..."
sleep $SLEEP_SECONDS
sleep "$SLEEP_SECONDS"
fi
done

Expand All @@ -713,9 +741,11 @@ jobs:
# in the issue, but the identical defect one step later.
RELEASE_VERSION: ${{ github.event.release.tag_name || github.ref_name }}
run: |
echo "## Production Deployment Summary" >> $GITHUB_STEP_SUMMARY
echo "" >> $GITHUB_STEP_SUMMARY
echo "- **Version**: $RELEASE_VERSION" >> $GITHUB_STEP_SUMMARY
echo "- **Commit**: \`${{ github.sha }}\`" >> $GITHUB_STEP_SUMMARY
echo "- **Deployed by**: ${{ github.actor }}" >> $GITHUB_STEP_SUMMARY
echo "- **Time**: $(date -u '+%Y-%m-%d %H:%M:%S UTC')" >> $GITHUB_STEP_SUMMARY
{
echo "## Production Deployment Summary"
echo ""
echo "- **Version**: $RELEASE_VERSION"
echo "- **Commit**: \`${{ github.sha }}\`"
echo "- **Deployed by**: ${{ github.actor }}"
echo "- **Time**: $(date -u '+%Y-%m-%d %H:%M:%S UTC')"
} >> "$GITHUB_STEP_SUMMARY"
72 changes: 44 additions & 28 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,14 +95,21 @@ jobs:
"https://github.com/rhysd/actionlint/releases/download/v${ACTIONLINT_VERSION}/actionlint_${ACTIONLINT_VERSION}_linux_amd64.tar.gz"
echo "${ACTIONLINT_SHA256} actionlint.tar.gz" | sha256sum -c -
tar -xzf actionlint.tar.gz actionlint
# -shellcheck= disables actionlint's shellcheck integration. This gate
# is about workflows GitHub cannot COMPILE (#1122) — the failure mode
# that produces a run with zero jobs and no logs. shellcheck reports 44
# pre-existing style/info findings in existing `run:` blocks, almost
# all in deploy.yml; turning that on here would either block this fix
# or force blind quoting changes to the deploy path that cannot be
# tested from a PR. Tracked separately in #1130.
./actionlint -color -shellcheck=
# The shell linting is ON (#1130 cleared the 44 pre-existing findings).
# It is not decoration: an unquoted heredoc delimiter made the runner
# command-substitute lines inside deploy.yml's ssh block that only
# LOOKED like comments, so "pm2 restart NAME" written in prose was
# executed on the runner. Only this linter sees that class.
# actionlint silently skips it when the binary is absent from PATH, so
# assert it is installed before trusting a clean run.
# (A comment here may not begin with the tool's own name — that is
# read as a malformed directive, and no backticks either.)
command -v shellcheck >/dev/null || {
echo "::error::shellcheck is not on PATH — actionlint would skip"
echo "every shell finding and this gate would pass vacuously."
exit 1
}
./actionlint -color

# ============================================
# Static Analysis - Check for Hardcoded URLs
Expand Down Expand Up @@ -204,10 +211,11 @@ jobs:
# where the env var is for something unrelated to the localhost URL.
ISSUES=""

# Check common API-related files for hardcoded URLs
for file in $(find web-ui/src -type f \( -name "*.ts" -o -name "*.tsx" \) \
-not -path "*/.next/*" \
-not -path "*/node_modules/*"); do
# Check common API-related files for hardcoded URLs. Read from find
# rather than word-splitting its output: a path containing a space
# would otherwise split into two non-existent files and every grep
# below would silently miss.
while IFS= read -r file; do

# Skip test files
if [[ $file == *".test."* ]] || [[ $file == *".spec."* ]]; then
Expand All @@ -221,7 +229,9 @@ jobs:
ISSUES="${ISSUES}\n - ${file}: Contains localhost URL without env var reference"
fi
fi
done
done < <(find web-ui/src -type f \( -name "*.ts" -o -name "*.tsx" \) \
-not -path "*/.next/*" \
-not -path "*/node_modules/*")

if [ -n "$ISSUES" ]; then
echo "⚠️ API files with potential issues (heuristic check):"
Expand Down Expand Up @@ -335,7 +345,7 @@ jobs:
- name: Check coverage threshold (65%)
working-directory: web-ui
run: |
COVERAGE=$(cat coverage/coverage-summary.json | jq '.total.statements.pct')
COVERAGE=$(jq '.total.statements.pct' coverage/coverage-summary.json)
echo "Coverage: ${COVERAGE}%"
if (( $(echo "$COVERAGE < 65" | bc -l) )); then
echo "❌ Coverage ${COVERAGE}% is below 65% threshold"
Expand Down Expand Up @@ -592,16 +602,18 @@ jobs:
steps:
- name: Report test results
run: |
echo "## Test Suite Results" >> $GITHUB_STEP_SUMMARY
echo "" >> $GITHUB_STEP_SUMMARY
echo "| Check | Status |" >> $GITHUB_STEP_SUMMARY
echo "|-------|--------|" >> $GITHUB_STEP_SUMMARY
echo "| Code Quality | ${{ needs.code-quality.result }} |" >> $GITHUB_STEP_SUMMARY
echo "| Workflow Lint | ${{ needs.workflow-lint.result }} |" >> $GITHUB_STEP_SUMMARY
echo "| Hardcoded URLs | ${{ needs.check-hardcoded-urls.result }} |" >> $GITHUB_STEP_SUMMARY
echo "| Backend Tests | ${{ needs.backend-tests.result }} |" >> $GITHUB_STEP_SUMMARY
echo "| Frontend Tests | ${{ needs.frontend-tests.result }} |" >> $GITHUB_STEP_SUMMARY
echo "| E2E Browser Smoke | ${{ needs.e2e-browser-smoke.result }} |" >> $GITHUB_STEP_SUMMARY
{
echo "## Test Suite Results"
echo ""
echo "| Check | Status |"
echo "|-------|--------|"
echo "| Code Quality | ${{ needs.code-quality.result }} |"
echo "| Workflow Lint | ${{ needs.workflow-lint.result }} |"
echo "| Hardcoded URLs | ${{ needs.check-hardcoded-urls.result }} |"
echo "| Backend Tests | ${{ needs.backend-tests.result }} |"
echo "| Frontend Tests | ${{ needs.frontend-tests.result }} |"
echo "| E2E Browser Smoke | ${{ needs.e2e-browser-smoke.result }} |"
} >> "$GITHUB_STEP_SUMMARY"

# The smoke job is a merge gate: treat any non-success terminal state
# (failure / cancelled / timed_out) as a gate failure, not just
Expand All @@ -614,10 +626,14 @@ jobs:
[ "${{ needs.e2e-browser-smoke.result }}" == "cancelled" ] || \
[ "${{ needs.e2e-browser-smoke.result }}" == "timed_out" ] || \
[ "${{ needs.frontend-tests.result }}" == "failure" ]; then
echo "" >> $GITHUB_STEP_SUMMARY
echo "❌ Some checks failed. Please review the logs above." >> $GITHUB_STEP_SUMMARY
{
echo ""
echo "❌ Some checks failed. Please review the logs above."
} >> "$GITHUB_STEP_SUMMARY"
exit 1
else
echo "" >> $GITHUB_STEP_SUMMARY
echo "✅ All checks passed!" >> $GITHUB_STEP_SUMMARY
{
echo ""
echo "✅ All checks passed!"
} >> "$GITHUB_STEP_SUMMARY"
fi
31 changes: 19 additions & 12 deletions tests/test_workflow_lint_wiring_1122.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,17 +74,24 @@ def test_every_workflow_file_still_parses():
assert "jobs" in data, f"{path.name} declares no jobs"


def test_the_shellcheck_suppression_is_documented_and_tracked():
"""`-shellcheck=` is a deliberate scope limit, not a silent one (#1130).
def test_the_shellcheck_suppression_is_gone():
"""#1130 cleared the 44 pre-existing findings, so the opt-out came out.

actionlint runs shellcheck whenever it is on PATH — which it is on GitHub
runners but often not locally, so this class of check disappears without
warning depending on where you run it. If the suppression is ever removed,
this test should be deleted along with it; if it stays, it stays explained.
It was never cosmetic: with it in place, an unquoted heredoc delimiter in
deploy.yml let the runner command-substitute lines inside the ssh block
that only looked like comments. Nothing but shellcheck catches that.
"""
assert "-shellcheck=" not in TEST_WORKFLOW.read_text()


def test_the_job_fails_when_shellcheck_is_missing_rather_than_passing_vacuously():
"""actionlint SKIPS every shell finding when the binary is absent from
PATH — it does not warn. Without this guard the gate would go quietly
hollow the day the runner image drops shellcheck."""
raw = TEST_WORKFLOW.read_text()
assert "-shellcheck=" in raw
assert "#1130" in raw, "the suppression must point at its follow-up issue"

assert "command -v shellcheck" in raw
assert "::error::" in raw


def test_workflow_compilability_is_still_checked():
Expand All @@ -93,11 +100,11 @@ def test_workflow_compilability_is_still_checked():
line for line in TEST_WORKFLOW.read_text().splitlines()
if "./actionlint" in line
)
# -shellcheck= narrows the checks; it must not be paired with anything that
# would also drop the expression/syntax pass. (Scoped to the invocation
# line — `paths-ignore` appears elsewhere in this file.)
# Nothing on the invocation line may drop the expression/syntax pass, which
# is what the job was created for. (Scoped to the invocation line —
# `paths-ignore` appears elsewhere in this file.)
assert "-ignore" not in invocation
assert "-shellcheck=" in invocation
assert "-shellcheck=" not in invocation


class TestEnvironmentSecretsAreReachable:
Expand Down
Loading