chore(dev-loop): gate architecturally-significant PRs on an architecture-reviewer pass#160
Conversation
…ure-reviewer pass Encodes the lesson from the ASB→RabbitMQ swap (PR #159): the "stay present / review significant changes" principle existed in CLAUDE.md but had no trigger at the ship moment, so it got skipped — the review (run only after a prompt) then caught a dead OTel trace source. Principle alone proved skippable, so add the mechanical catch. - CLAUDE.md: explicit rule — architecturally-significant changes get an architecture-reviewer pass before the PR opens; findings addressed or deferred in the PR body before merge. Defines "significant" concretely (transport/dep change, 3+ services, cross-cutting pattern, Domain aggregate, or /feature-spec significance). - .claude/scripts/remind-architecture-review.sh: PreToolUse(Bash) hook that surfaces the reminder when the command is `gh pr create`. Non-blocking (significance is a judgment call; a command-pattern deny could never let a PR through). Smoke-tested both branches. - .claude/settings.json: register the hook. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
WalkthroughAdds a ChangesArchitecture review gate
Article audit record
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
No action — article already encoded more rigorously in the repo’s AI workflow rules.
|
Re-validated against current main after the merge train landed:
The rationale also got stronger since this was opened: the gate is exactly what caught 1 Must-fix + 3 Should-consider on #173 (including a factually wrong teaching comment claiming NotificationService handlers were idempotent) before the PR was opened — and the #159 review it cites went on to surface 25 verified findings, two of them real durability bugs (#168/#169, fixed in #173). |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
…view-gate # Conflicts: # .claude/audits/INDEX.md
…meraldleaf/NextAurora into chore/architecture-review-gate
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/settings.json:
- Around line 53-61: Update the permissions.allow array in the settings
configuration to include the Bash(gh pr create *) permission, alongside the
existing GitHub CLI permissions, so the architecture-review hook can
successfully execute gh pr create.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dad8295b-9d4c-4936-b592-3345a3b766a5
📒 Files selected for processing (4)
.claude/audits/INDEX.md.claude/scripts/remind-architecture-review.sh.claude/settings.jsonCLAUDE.md
What
Makes the "review architecturally-significant changes before merge" rule un-skippable by giving it a trigger at the ship moment.
architecture-reviewerpass before the PR opens; findings addressed or deferred in the PR body before merge. Defines "significant" concretely (transport/dependency change, 3+ services, cross-cutting pattern, Domain aggregate, or/feature-specsignificance)..claude/scripts/remind-architecture-review.sh— aPreToolUse(Bash) hook that surfaces the reminder when the command opens a PR. Non-blocking (significance is a judgment call; a command-pattern deny could never let a PR through). Guards against thegit commitfalse positive (commit messages mention the command in prose)..claude/settings.json— registers the hook.Why
The lesson from PR #159 (ASB→RabbitMQ): the "stay present / review significant changes" principle already existed in CLAUDE.md and I'd read it — but it had no trigger at the ship moment, so it got skipped. The review, run only after a prompt, then caught a real bug (a dead OpenTelemetry trace source that would have silently dropped saga spans). Principle-as-words proved skippable; this adds the mechanical catch, per the project's own convention → hook → CI escalation philosophy.
Applying the gate to this PR
The new hook fired on
gh pr create(working as intended). Assessing significance: this change is dev-loop tooling — a CLAUDE.md process rule + a bash hook + a settings entry, with no application.cscode. The architecture-reviewer agent evaluates application architecture (SOLID/DDD/VSA/perf/security against its Pattern checklist), none of which this PR touches. So per the gate's own criteria it's not significant in the agent's sense — proceeding without the agent (this is exactly the "pattern-conforming / tooling changes may proceed" carve-out).Test plan
jqvalidates.claude/settings.json(2 PreToolUse entries).gh pr create, silent ongit status, and silent on agit commitwhose message mentions "gh pr create" (the false positive this guard fixes).🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Chores