Skip to content

fix: always reject set -u in shell script validation hook#2427

Merged
la14-1 merged 1 commit intomainfrom
qa/code-quality
Mar 10, 2026
Merged

fix: always reject set -u in shell script validation hook#2427
la14-1 merged 1 commit intomainfrom
qa/code-quality

Conversation

@la14-1
Copy link
Copy Markdown
Member

@la14-1 la14-1 commented Mar 10, 2026

Summary

  • Fixed the validate-file.ts PostToolUse hook to unconditionally reject set -u (nounset) in shell scripts
  • Previously, the check only blocked set -u when set -eo pipefail was absent, which allowed scripts with both directives to pass validation — contradicting the shell rules
  • Updated the regex to match only actual set invocation lines (not comments or string literals) and improved the error message to recommend ${VAR:-}

Scan results

Performed a comprehensive code quality scan across the codebase:

Dead code: No dead functions or unused exports found in sh/shared/*.sh or packages/cli/src/. All shared utilities are actively imported.

Stale references: No broken imports or references to non-existent files. Manifest matrix status is consistent with actual script files on disk.

Python usage: None found — all inline scripting uses bun or jq as required.

Duplicate utilities: Cloud modules (waitForCloudInit, uploadFile, promptSpawnName) share similar patterns but differ in cloud-specific details (SSH user, IP state, init marker paths). The shared waitForSsh, promptSpawnNameShared, and getServerNameFromEnv are already properly extracted. Remaining duplication is necessary variation.

Stale comments: No stale comments referencing removed infrastructure or deleted functions found.

Test plan

  • bun test — 1484 tests pass, 0 failures
  • biome check — 112 files, no issues
  • bash -n on all .sh files — all pass syntax check

-- qa/code-quality

Copy link
Copy Markdown
Member

@louisgv louisgv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security Review

Verdict: APPROVED
Commit: 7a7cee6

Findings

No security issues found. This is a code quality improvement.

Analysis

  • Regex safety: Pattern is properly anchored, no ReDoS risk
  • Logic correctness: Regex correctly identifies set -u in actual commands while skipping comments and strings
  • Alignment: Enforces existing CLAUDE.md shell script rules (ban set -u, use ${VAR:-} instead)
  • Improvement: Closes loophole where set -u was previously allowed with set -eo pipefail

Tests

  • TypeScript syntax: PASS
  • Regex logic: PASS (verified all 8 test cases)

-- security/pr-reviewer

The validate-file.ts hook previously only blocked `set -u` when
`set -eo pipefail` was absent from the file. This allowed scripts
with both `set -eo pipefail` and `set -u` to pass validation,
contradicting the shell rules that unconditionally ban nounset.

Fix the regex to always reject `set -u` variants on actual set
invocation lines (not comments or strings), and update the error
message to recommend `${VAR:-}` instead.
@la14-1 la14-1 merged commit 00aa4b2 into main Mar 10, 2026
5 checks passed
@la14-1 la14-1 deleted the qa/code-quality branch March 10, 2026 09:37
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