feat: PR review gate — PreToolUse hook with CodeRabbit#1296
Open
jeremyeder wants to merge 8 commits intoambient-code:mainfrom
Open
feat: PR review gate — PreToolUse hook with CodeRabbit#1296jeremyeder wants to merge 8 commits intoambient-code:mainfrom
jeremyeder wants to merge 8 commits intoambient-code:mainfrom
Conversation
Stateless PreToolUse hook that gates gh pr create behind mechanical code quality checks (gofmt, go vet, eslint, ruff, no panic(), no any, secret detection) and CodeRabbit AI review (security, performance, K8s safety per .coderabbit.yaml). Exits 0 to allow, 2 to block with findings. Skips gracefully when toolchains are unavailable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Hooks into Bash tool calls via .claude/settings.json. The script passes through non-PR commands instantly and only gates gh pr create. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
LLM-layer review instruction complements the mechanical PreToolUse hook and CodeRabbit AI review. Agents self-review against governance docs before opening PRs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When Amber creates a PR from an issue and CI passes, it now applies ambient-code:self-reviewed so Mergify can merge without human approval. Both the issue-label and fix-issue prompts are updated. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use array for error accumulation instead of fragile string concat - Extract duplicate error report block into block_with_errors() - Compute diff once, reuse for file list and secret scan - Batch panic() and any-type checks with xargs instead of per-file loops - Remove unnecessary file existence checks (trust git output) - Remove unused CR_EXIT variable - Trim redundant comments Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Instead of regex-matching "rate.limit" in raw output, parse the errorType field from --agent JSON. Reports actual wait time when rate-limited. Separates rate-limit from auth failure messages. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CodeRabbit CLI emits one JSON object per line (NDJSON) plus non-JSON lines like "[error] stopping cli". Filter to valid JSON lines before jq parsing. Also handle generic error types, not just rate_limit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughIntroduces an automated PR review gate via new Changes
Sequence Diagram(s)sequenceDiagram
actor Agent as AI Agent
participant Tool as PreToolUse Hook
participant Checks as Mechanical Checks
participant CR as CodeRabbit
participant GitHub as gh pr create
Agent->>Tool: Attempt gh pr create
Tool->>Tool: Intercept & compute diff vs main
Tool->>Checks: Run linters, format, secrets scan
alt Mechanical checks fail
Checks-->>Tool: Issues found
Tool->>Agent: Block PR (exit 2)
else Mechanical checks pass
Checks-->>Tool: ✓ Pass
Tool->>CR: Run coderabbit review --agent
alt CodeRabbit finds blocking errors
CR-->>Tool: severity-error findings
Tool->>Agent: Block PR (exit 2)
else CodeRabbit clear or unavailable
CR-->>Tool: No blocking errors
Tool->>GitHub: Allow execution (exit 0)
GitHub->>Agent: PR created
Agent->>Agent: Apply ambient-code:self-reviewed label
end
end
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/amber-issue-handler.yml:
- Around line 107-110: The workflow adds the ambient-code:self-reviewed label
but .mergify.yml still requires human approvals, so update .mergify.yml to
recognize that label as an alternative merge condition: modify the relevant
rule(s) (the approval/required_conditions block in the top rule set) to allow
merge when label:ambient-code:self-reviewed is present (or add a new rule that
permits merging when that label exists), and mirror that change for the other
affected rule(s) referenced in lines 303-306 so the workflow text and mergify
policy are consistent.
In `@scripts/hooks/pr-review-gate.sh`:
- Around line 58-71: The script currently silently skips checks when toolchains
are missing (checks that use "command -v gofmt" and "command -v go"); update the
hook to fail fast and surface missing tool errors by testing for required tools
up front (gofmt, go, ruff, frontend deps) or immediately after each "command -v"
check and appending a clear error to ERRORS (e.g., include the tool name and
context) and/or exiting non-zero; specifically modify the blocks that reference
GO_FILES, REPO_ROOT, mod, UNFORMATTED, VET_OUTPUT, and ERRORS so that when
"command -v <tool>" returns false you add an ERRORS entry like "<tool> not
found: required for <mod or repo check>" (or abort) rather than silently
skipping the validation.
- Around line 140-145: The current elif condition treats any .type=="error" as
an auth/CLI skip because of the `|| echo "$CR_JSON" | jq -e 'select(.type ==
"error")'` check; update the conditional so only auth/CLI errors are skipped
(e.g., rely solely on CR_ERROR_TYPE == "auth" or explicitly test a specific
metadata reason in CR_JSON like `jq -e 'select(.metadata.reason=="auth" or
.metadata.reason=="cli")'`) and remove the broad `.type == "error"` test; keep
the CR_MSG extraction logic (CR_JSON/CR_MSG) and ensure other error types
(internal/config/runtime) fall through to the rate-limit/error handling path
instead of being treated as skipped.
🪄 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: CHILL
Plan: Pro
Run ID: 8a8578d3-113b-47fe-97ad-81e963d74c39
📒 Files selected for processing (5)
.claude/settings.json.github/workflows/amber-issue-handler.ymlBOOKMARKS.mdCLAUDE.mdscripts/hooks/pr-review-gate.sh
Comment on lines
+107
to
+110
| 7. Add the `ambient-code:self-reviewed` label to the PR. | ||
| 8. Ensure CI passes. If it fails, investigate and fix. | ||
| 9. Do not merge. Leave the PR open for human review. | ||
| 10. When you comment on the PR, include this footer at the end: |
Contributor
There was a problem hiding this comment.
The new self-reviewed label step does not unlock the merge path this PR describes.
These prompt updates tell the agent to add ambient-code:self-reviewed, but .mergify.yml:1-44 still only queues PRs with approved reviews and never checks that label. So this change will not let self-reviewed PRs merge without human approval.
Also applies to: 303-306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/amber-issue-handler.yml around lines 107 - 110, The
workflow adds the ambient-code:self-reviewed label but .mergify.yml still
requires human approvals, so update .mergify.yml to recognize that label as an
alternative merge condition: modify the relevant rule(s) (the
approval/required_conditions block in the top rule set) to allow merge when
label:ambient-code:self-reviewed is present (or add a new rule that permits
merging when that label exists), and mirror that change for the other affected
rule(s) referenced in lines 303-306 so the workflow text and mergify policy are
consistent.
Comment on lines
+58
to
+71
| if command -v gofmt &>/dev/null; then | ||
| MOD_GO_FILES=$(echo "$GO_FILES" | grep "^$mod/" || true) | ||
| if [ -n "$MOD_GO_FILES" ]; then | ||
| UNFORMATTED=$(cd "$REPO_ROOT" && echo "$MOD_GO_FILES" | xargs gofmt -l 2>/dev/null || true) | ||
| if [ -n "$UNFORMATTED" ]; then | ||
| ERRORS+=("gofmt: unformatted files:" "$UNFORMATTED") | ||
| fi | ||
| fi | ||
| fi | ||
|
|
||
| if command -v go &>/dev/null; then | ||
| VET_OUTPUT=$(cd "$REPO_ROOT/$mod" && go vet ./... 2>&1) || \ | ||
| ERRORS+=("go vet failed in ${mod}:" "$VET_OUTPUT") | ||
| fi |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'command -v gofmt|command -v go\b|node_modules|command -v ruff' scripts/hooks/pr-review-gate.shRepository: ambient-code/platform
Length of output: 286
🏁 Script executed:
cat scripts/hooks/pr-review-gate.shRepository: ambient-code/platform
Length of output: 6873
Missing toolchains silently skip required checks, degrading the gate.
If go, gofmt, ruff, or frontend dependencies are absent, the hook skips those validations and allows gh pr create anyway (lines 58, 68, 90, 106). In a fresh environment without all toolchains installed, the advertised PR gate degrades to best-effort and code that would fail CI checks can be created locally. The script has no validation or initialization to ensure required tools are available—it silently skips checks if they're missing, unlike CodeRabbit which explicitly logs when unavailable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/hooks/pr-review-gate.sh` around lines 58 - 71, The script currently
silently skips checks when toolchains are missing (checks that use "command -v
gofmt" and "command -v go"); update the hook to fail fast and surface missing
tool errors by testing for required tools up front (gofmt, go, ruff, frontend
deps) or immediately after each "command -v" check and appending a clear error
to ERRORS (e.g., include the tool name and context) and/or exiting non-zero;
specifically modify the blocks that reference GO_FILES, REPO_ROOT, mod,
UNFORMATTED, VET_OUTPUT, and ERRORS so that when "command -v <tool>" returns
false you add an ERRORS entry like "<tool> not found: required for <mod or repo
check>" (or abort) rather than silently skipping the validation.
Comment on lines
+140
to
+145
| if [ "$CR_ERROR_TYPE" = "rate_limit" ]; then | ||
| CR_WAIT=$(echo "$CR_JSON" | jq -r 'select(.type == "error") | .metadata.waitTime // "unknown"' 2>/dev/null || true) | ||
| echo "PR Review Gate: CodeRabbit rate-limited (wait: $CR_WAIT)" >&2 | ||
| elif [ "$CR_ERROR_TYPE" = "auth" ] || echo "$CR_JSON" | jq -e 'select(.type == "error")' &>/dev/null; then | ||
| CR_MSG=$(echo "$CR_JSON" | jq -r 'select(.type == "error") | .message // "unknown error"' 2>/dev/null || true) | ||
| echo "PR Review Gate: CodeRabbit skipped ($CR_MSG)" >&2 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
CR_JSON='{"type":"error","errorType":"internal","message":"boom"}'
CR_ERROR_TYPE=$(printf '%s\n' "$CR_JSON" | jq -r 'select(.type == "error") | .errorType')
if [ "$CR_ERROR_TYPE" = "rate_limit" ]; then
echo rate_limit
elif [ "$CR_ERROR_TYPE" = "auth" ] || printf '%s\n' "$CR_JSON" | jq -e 'select(.type == "error")' >/dev/null; then
echo skipped
else
echo blocked
fiRepository: ambient-code/platform
Length of output: 72
🏁 Script executed:
cat -n scripts/hooks/pr-review-gate.sh | sed -n '130,155p'Repository: ambient-code/platform
Length of output: 1642
🏁 Script executed:
# Search for error handling context and PR objective mentions
grep -n -B5 -A5 "rate_limit\|auth\|error.*type\|degrade\|graceful" scripts/hooks/pr-review-gate.sh | head -80Repository: ambient-code/platform
Length of output: 1489
Non-auth errors are silently treated as skipped instead of blocking.
Line 143's || jq -e 'select(.type == "error")' catches all error types, not just auth. The documented intent (line 128) is to skip gracefully only for CLI/auth issues, but internal/config/runtime errors bypass the gate entirely. The pre-execution test confirms: errorType="internal" results in "skipped".
Suggested fix
- elif [ "$CR_ERROR_TYPE" = "auth" ] || echo "$CR_JSON" | jq -e 'select(.type == "error")' &>/dev/null; then
+ elif [ "$CR_ERROR_TYPE" = "auth" ]; then
CR_MSG=$(echo "$CR_JSON" | jq -r 'select(.type == "error") | .message // "unknown error"' 2>/dev/null || true)
echo "PR Review Gate: CodeRabbit skipped ($CR_MSG)" >&2
+ elif echo "$CR_JSON" | jq -e 'select(.type == "error")' &>/dev/null; then
+ CR_MSG=$(echo "$CR_JSON" | jq -r 'select(.type == "error") | .message // "unknown error"' 2>/dev/null || true)
+ ERRORS+=("CodeRabbit review failed:" "$CR_MSG")
elif [ -n "$CR_JSON" ]; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if [ "$CR_ERROR_TYPE" = "rate_limit" ]; then | |
| CR_WAIT=$(echo "$CR_JSON" | jq -r 'select(.type == "error") | .metadata.waitTime // "unknown"' 2>/dev/null || true) | |
| echo "PR Review Gate: CodeRabbit rate-limited (wait: $CR_WAIT)" >&2 | |
| elif [ "$CR_ERROR_TYPE" = "auth" ] || echo "$CR_JSON" | jq -e 'select(.type == "error")' &>/dev/null; then | |
| CR_MSG=$(echo "$CR_JSON" | jq -r 'select(.type == "error") | .message // "unknown error"' 2>/dev/null || true) | |
| echo "PR Review Gate: CodeRabbit skipped ($CR_MSG)" >&2 | |
| if [ "$CR_ERROR_TYPE" = "rate_limit" ]; then | |
| CR_WAIT=$(echo "$CR_JSON" | jq -r 'select(.type == "error") | .metadata.waitTime // "unknown"' 2>/dev/null || true) | |
| echo "PR Review Gate: CodeRabbit rate-limited (wait: $CR_WAIT)" >&2 | |
| elif [ "$CR_ERROR_TYPE" = "auth" ]; then | |
| CR_MSG=$(echo "$CR_JSON" | jq -r 'select(.type == "error") | .message // "unknown error"' 2>/dev/null || true) | |
| echo "PR Review Gate: CodeRabbit skipped ($CR_MSG)" >&2 | |
| elif echo "$CR_JSON" | jq -e 'select(.type == "error")' &>/dev/null; then | |
| CR_MSG=$(echo "$CR_JSON" | jq -r 'select(.type == "error") | .message // "unknown error"' 2>/dev/null || true) | |
| ERRORS+=("CodeRabbit review failed:" "$CR_MSG") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/hooks/pr-review-gate.sh` around lines 140 - 145, The current elif
condition treats any .type=="error" as an auth/CLI skip because of the `|| echo
"$CR_JSON" | jq -e 'select(.type == "error")'` check; update the conditional so
only auth/CLI errors are skipped (e.g., rely solely on CR_ERROR_TYPE == "auth"
or explicitly test a specific metadata reason in CR_JSON like `jq -e
'select(.metadata.reason=="auth" or .metadata.reason=="cli")'`) and remove the
broad `.type == "error"` test; keep the CR_MSG extraction logic (CR_JSON/CR_MSG)
and ensure other error types (internal/config/runtime) fall through to the
rate-limit/error handling path instead of being treated as skipped.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
scripts/hooks/pr-review-gate.sh) that gatesgh pr createbehind mechanical code quality checks (gofmt, go vet, eslint, ruff, panic(), any types, secret detection) and CodeRabbit AI review.claude/settings.jsonso all sessions in this repo get it automaticallyambient-code:self-reviewedlabel after PR creationHow it works
--agentmode parsed correctlyTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation