Add optional pre-commit hook for code formatting #5178
2 issues
code-review: Found 2 issues (1 medium, 1 low)
Medium
Hook ignores dotnet format exit code and relies on fragile grep for "formatted" - `.githooks/pre-commit:8-9`
The hook pipes dotnet format --verify-no-changes output through grep -q "formatted" to decide pass/fail, discarding the tool's actual exit code. If dotnet format fails for an unrelated reason (e.g., restore needed, SDK/workload missing, project load error) and its error output does not contain the word "formatted", the hook will silently report success and allow the commit. Conversely, benign output containing the substring "formatted" could cause false failures. The CI workflow relies on --verify-no-changes's non-zero exit code, so the hook diverges from CI behavior despite the comment claiming otherwise.
Low
Glob `./**/*OptionsSetup.cs` is shell-expanded before reaching dotnet format - `.githooks/pre-commit:9`
The exclusion argument ./**/*OptionsSetup.cs is unquoted, so bash will glob-expand it (with globstar if enabled, otherwise only one level) before passing to dotnet format. This can produce inconsistent behavior between developer machines depending on shell options, and may pass an expanded list of files that dotnet format does not interpret the same way as the literal pattern used in CI. The CI workflow runs the same unquoted pattern, but in CI it executes under a known shell/env; local developer shells vary.
Duration: 37.3s · Tokens: 64.6k in / 1.7k out · Cost: $0.49 (+merge: $0.00)
Annotations
Check warning on line 9 in .githooks/pre-commit
sentry-warden / warden: code-review
Hook ignores dotnet format exit code and relies on fragile grep for "formatted"
The hook pipes `dotnet format --verify-no-changes` output through `grep -q "formatted"` to decide pass/fail, discarding the tool's actual exit code. If `dotnet format` fails for an unrelated reason (e.g., restore needed, SDK/workload missing, project load error) and its error output does not contain the word "formatted", the hook will silently report success and allow the commit. Conversely, benign output containing the substring "formatted" could cause false failures. The CI workflow relies on `--verify-no-changes`'s non-zero exit code, so the hook diverges from CI behavior despite the comment claiming otherwise.