diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index 61e03d57..867ea3a8 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -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) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 004e1eef..2f2a57f0 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -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' @@ -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..." @@ -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 --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 @@ -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 @@ -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 @@ -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' @@ -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..." @@ -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..." @@ -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 ` 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 @@ -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 @@ -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" diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 902c0d2d..f2ffa90d 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -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 @@ -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 @@ -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):" @@ -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" @@ -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 @@ -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 diff --git a/tests/test_workflow_lint_wiring_1122.py b/tests/test_workflow_lint_wiring_1122.py index f8c7e72d..47ae4487 100644 --- a/tests/test_workflow_lint_wiring_1122.py +++ b/tests/test_workflow_lint_wiring_1122.py @@ -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(): @@ -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: