chore: git-guardrails hook + SDK pin (paired with ui-mobile#422) - #301
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #301 — chore: git-guardrails hook + SDK pin
Scope: .claude/hooks/git-guardrails.mjs (new), .claude/settings.json (new), global.json (new) — no src/ product code touched.
Recommendation: APPROVE (no Critical/High findings; two Medium defense-in-depth gaps noted for a fast follow-up)
Summary:
This PR adds a PreToolUse(Bash) hook that deterministically blocks --no-verify, --no-gpg-sign, and direct/force pushes to main/master, plus a global.json pinning the .NET 10 SDK feature band. It is pure tooling — no product code, no DTOs, no endpoints — so most rubric dimensions (SOLID, comment policy, type safety, DESIGN.md, parity, contract drift, backend hard rules) are N/A: the diff never touches their surface. The hook script itself was security-reviewed manually since it is the PR whole purpose (a bypass-prevention gate); the security-reviewer subagent gate (any src/ code changed) is not met here, and contract-aligner is N/A (no DTO/Controller/packages/shared change).
Findings:
Medium: Bare git push to main bypasses the no-direct-push rule
- location: .claude/hooks/git-guardrails.mjs:33-36 (rule 3 regex)
- issue: Rule 3 only fires when the literal string main/master appears in the command text. git push, git push -f, or git push --force with no explicit remote/branch (which pushes the current branch via the configured upstream) never matches — if the current local branch happens to be main, this is silently allowed through.
- risk: The PR body claims the hook blocks direct or force pushes to the protected branch, but for the most common invocation of push (no args) it does not. GitHub server-side branch protection remains the real backstop, so this is a gap in the fast local pre-check, not an unguarded path to data loss.
- fix: when a push command has no explicit ref, shell out to git rev-parse --abbrev-ref HEAD and block if the current branch is main/master; or at minimum treat bare git push / git push -f / git push --force* (no positional ref args) as suspect.
Medium: git commit -n bypasses the --no-verify guard
- location: .claude/hooks/git-guardrails.mjs:23-26 (rule 1 regex)
- issue: -n is git documented short alias for --no-verify on git commit, but rule 1 only matches the literal string --no-verify.
- risk: git commit -n -m ... silently skips pre-commit hooks, defeating exactly the protection this hook and CLAUDE.md never-skip-hooks rule are meant to enforce.
- fix: add a git-commit-scoped check for a standalone -n flag (scope it to the commit subcommand specifically — -n means --dry-run on git push and limit-count on git log, so a blanket -n match would false-positive).
Both are fail-open by design (any hook error leads to exit 0), which is a reasonable, documented tradeoff for a Bash-gating hook and not itself a finding.
Validation:
- Build/Unit Tests/SonarCloud: run as separate required CI checks per workflow instructions — not re-run here
- SDK pin compatibility: global.json version 10.0.100 + rollForward latestFeature matches CI dotnet-version 10.0.x (actions/setup-dotnet@v5) — no build-break risk identified
What is good:
- Replaces prose-only enforcement (CLAUDE.md says do not force-push main) with a deterministic gate — matches the repo no-workarounds, root-cause philosophy.
- Fail-open error handling is explicit and documented, not a silent gap.
- global.json correctly scopes to feature-band roll-forward rather than pinning an exact patch, matching CI.
- Small, single-purpose, well-scoped diff with no product-code blast radius.
Recommendation:
Approve as-is; the two Medium findings are good candidates for a fast follow-up PR (both are narrow regex extensions to the same file) rather than blockers, since GitHub server-side branch protection and the actual git hooks remain the authoritative enforcement layer this local guard is front-running.
Paired with orbit-ui-mobile#422 (the AI environment overhaul). Adds the deterministic guards that must hold even when orbit-api is opened directly (standalone or opencode), not launched from the mobile session: - .claude/hooks/git-guardrails.mjs + settings.json - a PreToolUse Bash gate that blocks commit-signing bypass, hook-skipping, and direct or force pushes to the protected branch. Mirrors the mobile guard. - global.json - pins the .NET 10 SDK feature band so agents and CI build on the same SDK. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
c6a08a9 to
04b290e
Compare
|
There was a problem hiding this comment.
Review — PR #301 (chore/agentic-guardrails)
Scope: .claude/hooks/git-guardrails.mjs (new), .claude/settings.json (new), global.json (new), .github/workflows/sonarcloud.yml (1-line exclusion tweak). No src/ product code touched.
Recommendation: APPROVE
Summary
This PR has a single commit (04b290e3). The diff is unchanged since a prior review already approved it — no new commits have been pushed since. Re-verifying that review's findings against the current code:
- Bare
git pushto main — mitigated. A fallback block ingit-guardrails.mjsisolates the push segment, counts positional args afterpush, and blocks when ≤1 explicit ref is given and the current branch (viagit rev-parse --abbrev-ref HEAD) ismain/master. git commit -nbypass — mostly mitigated, one narrow remaining gap. Rule 4 catches-nwhen it appears first or standalone (e.g.git commit -n -m "msg"), but the regex can't skip over a flag's argument text, sogit commit -m "msg" -n(a plausible ordering) does not match. This is a real but narrow Medium-severity defense-in-depth gap — not a blocker, since branch-protection/pre-commit hooks remain the authoritative backstop and this hook fails open by design. Worth a fast-follow: extend the rule to look for a standalone-ntoken anywhere in thecommit-scoped segment rather than only in a leading flag run.
Other context (informational, not gated by this pass)
- SonarCloud's Quality Gate failure (0% coverage on new code) is expected for a diff that's entirely a Node.js hook script + JSON config with no dotnet-testable surface; Build/Unit Tests/SonarCloud are CI-owned checks out of scope here.
- Cross-repo dimensions (contract-aligner,
packages/sharedbackward-compat) are not verifiable in this job (siblingorbit-ui-mobilerepo isn't checked out), and this diff touches no DTO/endpoint/Zod-schema surface anyway, so they're N/A.
No Critical or High findings. Approving.
…ency (#355) * perf(api): parallelize/batch background fan-out to cut sequential latency (#243) Three background/command paths awaited slow per-item work sequentially. Behavior is preserved; only the fan-out is now concurrent or batched, with per-item error isolation intact. - CreateChallengeCommand: invite pushes now fan out via a throttled Task.WhenAll (SemaphoreSlim cap 5), each send on its own DI child scope so PushNotificationService gets an isolated DbContext (EF DbContext is not thread-safe; matches the #301 Astra-tools isolation pattern). - ReminderSchedulerService: record-keeping stays sequential on the single request-scoped DbContext (its in-memory dedup + ordering are unchanged), and the network sends are deferred into a bounded worker pool, each worker draining a shared queue on its own isolated scope. Push failures are now logged per recipient instead of aborting the tick. - StreakFreezeAutoActivationService: eligible users are staged and saved in one batched SaveChanges; on a unique/concurrency conflict it falls back to the original per-user saves, preserving the failure-isolation contract the existing tests lock in. Tests: multi-recipient fan-out + per-item push failure isolation for the reminder scheduler; batched single-SaveChanges for streak-freeze; throttled fan-out delivering each invitee exactly once for challenges. Refs thomasluizon/orbit-ui-mobile#243 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(api): isolate per-user push failures in streak-freeze notify loop (#243) Review caught a real regression from batching: NotifyActivatedAsync sent pushes after the single batched SaveChanges committed all freezes, but a throw mid-loop aborted the remaining sends. Because the freeze rows are already persisted, StageFreeze's freeze-exists guard skips those users on every future tick, so their "streak protected" notification was lost permanently. Wrap each SendToUserAsync in a shared NotifyFreezeActivatedAsync helper (log Warning + continue), used by both the batch and per-user fallback paths, mirroring DrainPushQueueAsync. Adds a regression test asserting a mid-loop push throw still notifies the other staged users and persists all freezes. Refs thomasluizon/orbit-ui-mobile#243 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Paired with orbit-ui-mobile#422 (the AI environment overhaul).
Adds the deterministic guards that must hold even when orbit-api is opened directly (standalone or opencode), not launched from the mobile session:
.claude/hooks/git-guardrails.mjs+settings.json— a PreToolUse Bash gate that blocks commit-signing bypass, hook-skipping, and direct or force pushes to the protected branch. Mirrors the mobile guard.global.json— pins the .NET 10 SDK feature band so agents and CI build on the same SDK.No product code touched.
Generated with Claude Code.