Skip to content

Harden variables.sh, remove eval, rename auto-merge workflow (High batch)#198

Merged
ydesgagn merged 1 commit into
masterfrom
fix/high-batch-bug004-qual010-ci002
May 17, 2026
Merged

Harden variables.sh, remove eval, rename auto-merge workflow (High batch)#198
ydesgagn merged 1 commit into
masterfrom
fix/high-batch-bug004-qual010-ci002

Conversation

@ydesgagn
Copy link
Copy Markdown
Contributor

Summary

Remediates three High-tier findings from docs/code-review.md. variables.sh ran with no error handling, five action sites executed input strings via eval/bash -c, and the auto-merge workflow was misnamed (it only approves).

Key changes:

  • BUG-004 (variables/variables.sh): add set -euo pipefail scoped to main() (so sourcing for the test suite is unaffected); initialise TAG, add :- defaults for runner env vars for nounset safety, replace git … | head -n 1 with capture-then-strip so a git failure is no longer masked under pipefail, and make the linter-detection helpers return 0 so an absent config file is a no-op rather than a set -e abort.
  • QUAL-010 (aws/action.yml, linters/phpcs/action.yml, linters/phpstan/action.yml): replace the five eval "${VAR}" / bash -c "${VAR}" sites with bash -c -- "${VAR}".
  • CI-002 (.github/workflows/auto-merge.yml.github/workflows/auto-approve.yml): regenerated from the patched github-build generator (companion PR Rename auto-merge workflow to auto-approve and remove legacy file (CI-002) github-build#387). The legacy auto-merge.yml is deleted and replaced by auto-approve.yml; the CI-001 fork guard, concurrency group, and AUTO-GENERATED header are carried over.

Types of changes

  • Bugfix (fixes an issue)
  • New feature (adds functionality)
  • Refactoring (improves code without changing functionality)
  • Breaking change (incompatible changes)
  • Build or security update (updates dependencies, libraries, or security patches)
  • Code style or documentation update (formatting, renaming, or documentation changes)
  • Other (please describe):

Checklist

  • Unit tests added to validate my fix/feature
  • I have manually tested my change
  • I did not add automation test. Why ?: composite-action test harness does not exist yet (tracked as TEST-002); BUG-004 is covered by the existing bats suite (65/65 against the real script) and an end-to-end run of variables.sh
  • Database changes requiring migration with downtime or reprocessing of existing data
  • The SOUP file lists the risk Level, requirements and verification reasoning associated with each library
  • `readme.md` includes sections on introduction, installation, usage, and contributing
  • `docs/architecture.md` includes sections on the architecture diagram, software units, software of unknown provenance, critical algorithms and risk controls related to PII and security
  • Impact on PII, privacy regulations (CCPA/GDPR/PIPEDA), CIS benchmarks or security (availability/confidentiality/integrity); management must be notified

Further comments (if required)

Two consumer-visible changes. (1) BUG-004: variables.sh now fails fast — a shallow clone, missing tag, or network blip that previously produced empty build vars and silently skipped a deploy will now fail the step. This is the intended correctness fix. (2) CI-002: the workflow file is renamed; auto-merge.yml is removed and auto-approve.yml added. Behaviour is unchanged (it still only approves code-owner PRs). Merge the companion `Cloud-Officer/github-build#387` so future regenerations stay in sync.

@ydesgagn ydesgagn requested a review from a team as a code owner May 17, 2026 12:06
@ydesgagn ydesgagn merged commit e28691b into master May 17, 2026
15 checks passed
@ydesgagn ydesgagn deleted the fix/high-batch-bug004-qual010-ci002 branch May 17, 2026 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants