diff --git a/.github/workflows/code_style.yml b/.github/workflows/code_style.yml index bd964729..b5055717 100644 --- a/.github/workflows/code_style.yml +++ b/.github/workflows/code_style.yml @@ -78,15 +78,55 @@ jobs: - name: Check lints run: cargo clippy --all --benches --tests --examples ${{ matrix.flags }} -- -D warnings + no-panics: + name: No panics in production code + runs-on: ubuntu-latest + steps: + - name: Checkout repository + uses: actions/checkout@v6 + with: + fetch-depth: 0 + - name: Check for .unwrap(), .expect(), assert!() in production code + run: | + BASE="${{ github.event.pull_request.base.sha }}" + # Get added lines in .rs files (production only, exclude tests/) + ADDED=$(git diff "$BASE"...HEAD -- 'src/**/*.rs' 'crates/**/*.rs' \ + | grep -E '^\+[^+]' || true) + + if [ -z "$ADDED" ]; then + echo "No production Rust changes detected." + exit 0 + fi + + # Match panic-inducing patterns, excluding test code and safety suppressions + VIOLATIONS=$(echo "$ADDED" \ + | grep -E '\.(unwrap|expect)\(|[^_]assert(_eq|_ne)?!' \ + | grep -Ev 'debug_assert|// safety:|#\[cfg\(test\)\]|#\[test\]|mod tests' \ + || true) + + if [ -n "$VIOLATIONS" ]; then + echo "::error::Found .unwrap(), .expect(), or assert!() in production code." + echo "Production code must use proper error handling instead of panicking." + echo "Suppress false positives with an inline '// safety: ' comment." + echo "" + echo "$VIOLATIONS" | head -20 + echo "" + COUNT=$(echo "$VIOLATIONS" | wc -l | tr -d ' ') + echo "Total: $COUNT violation(s)" + exit 1 + fi + + echo "OK: No panic-inducing calls in changed production code." + # Roll-up job for branch protection code-style: name: Code Style (fmt + clippy + deny) runs-on: ubuntu-latest if: always() - needs: [format, clippy, clippy-windows, deny-check] + needs: [format, clippy, clippy-windows, deny-check, no-panics] steps: - run: | - if [[ "${{ needs.format.result }}" != "success" || "${{ needs.clippy.result }}" != "success" || "${{ needs.deny-check.result }}" != "success" ]]; then + if [[ "${{ needs.format.result }}" != "success" || "${{ needs.clippy.result }}" != "success" || "${{ needs.deny-check.result }}" != "success" || "${{ needs.no-panics.result }}" != "success" ]]; then echo "One or more jobs failed" exit 1 fi diff --git a/scripts/check-boundaries.sh b/scripts/check-boundaries.sh index 56c85979..0d21fcf2 100755 --- a/scripts/check-boundaries.sh +++ b/scripts/check-boundaries.sh @@ -70,19 +70,21 @@ echo # This is a WARNING, not a hard violation. # -------------------------------------------------------------------------- -echo "--- Check 2: .unwrap() / .expect() in production code ---" +echo "--- Check 2: .unwrap() / .expect() / assert!() in production code ---" -# Collect raw matches excluding obvious test-only files and lines -raw_results=$(grep -rn '\.unwrap()\|\.expect(' src/ \ +# Collect raw matches excluding obvious test-only files and lines. +# Also catches assert!(), assert_eq!(), assert_ne!() but NOT debug_assert variants. +raw_results=$(grep -rnE '\.(unwrap|expect)\(|[^_]assert(_eq|_ne)?!' src/ \ --include='*.rs' \ | grep -v 'src/main.rs' \ | grep -v 'src/testing.rs' \ | grep -v 'src/setup/' \ + | grep -Ev 'debug_assert|// safety:' \ || true) if [ -n "$raw_results" ]; then total=$(echo "$raw_results" | wc -l | tr -d ' ') - echo "WARNING: ~$total .unwrap()/.expect() calls found in src/ (excluding main/testing/setup)." + echo "WARNING: ~$total .unwrap()/.expect()/assert!() calls found in src/ (excluding main/testing/setup)." echo "Many are in test modules; a per-file breakdown helps triage:" echo # Show per-file counts, sorted by count descending, top 15 diff --git a/scripts/pre-commit-safety.sh b/scripts/pre-commit-safety.sh index 3fddc3b8..a4ec3286 100755 --- a/scripts/pre-commit-safety.sh +++ b/scripts/pre-commit-safety.sh @@ -10,6 +10,7 @@ # 3. Hardcoded /tmp paths in tests (flaky in parallel runs) # 4. Tool parameters logged without redaction (secret leaks) # 5. Multi-step DB operations without transaction wrapping +# 6. .unwrap(), .expect(), assert!() in production code (panics) # # Suppress individual lines with an inline "// safety: " comment. @@ -128,6 +129,24 @@ if [ -n "$DIFF_W_OUTPUT" ]; then fi fi +# 6. .unwrap(), .expect(), assert!() in production code +# Matches added lines containing panic-inducing calls. +# Excludes test files, test modules, and debug_assert (compiled out in release). +# Suppress with "// safety: ". +PROD_DIFF="$DIFF_OUTPUT" +# Strip hunks from test-only files (tests/ directory, *_test.rs, test_*.rs) +PROD_DIFF=$(echo "$PROD_DIFF" | grep -v '^+++ b/tests/' || true) +if echo "$PROD_DIFF" | grep -nE '^\+' \ + | grep -E '\.(unwrap|expect)\(|[^_]assert(_eq|_ne)?!' \ + | grep -vE 'debug_assert|// safety:|#\[cfg\(test\)\]|#\[test\]|mod tests' \ + | head -5 | grep -q .; then + warn "PANIC" "Production code must not use .unwrap(), .expect(), or assert!(). Use proper error handling." + echo "$PROD_DIFF" | grep -nE '^\+' \ + | grep -E '\.(unwrap|expect)\(|[^_]assert(_eq|_ne)?!' \ + | grep -vE 'debug_assert|// safety:|#\[cfg\(test\)\]|#\[test\]|mod tests' \ + | head -5 | sed 's/^/ /' +fi + if [ "$WARNINGS" -gt 0 ]; then echo "" echo "Found $WARNINGS potential issue(s). Fix them or add '// safety: ' to suppress."