feat: PR change gating — pre-incident PR risk review (DEV-1252) - #506
Merged
damianloch merged 30 commits intoJun 18, 2026
Conversation
…hat, and orchestrator sub-agents
…dict parsing/rendering)
… handler, tool denylist)
…ew, keep existing, never delete)
…ted, context-line filtered)
…uard, markdown-cell backtick escape, denylist contract docs + tests)
…s collect (github_adapter imports the github_app_token jwt chain)
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds repository-level change-gating: DB flag, API + frontend toggle, Celery investigate_pr workflow, GitHub PR adapter, diff/verdict utilities with hidden markers/fingerprints, tool denylist wiring across agent runtime, auth-mode refactoring, and comprehensive unit tests. ChangesOAuth Auth-Mode Refactoring
Agent Tool Denylist Infrastructure
GitHub PR Review Service Layer
Change Gating Task & Webhook Orchestration
Repository Configuration & Frontend
Configuration & Wiring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![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: 6
🤖 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 `@client/src/components/github-provider-integration.tsx`:
- Around line 1188-1200: The Switch for "Incident Prevention" is missing an
accessible name; update the Switch element in github-provider-integration.tsx to
include an aria-label or aria-labelledby that references the repository name
(use repo.repo_full_name) so each toggle is announced uniquely, e.g. build a
label like "Incident Prevention for {repo.repo_full_name}" and add it as
aria-label (or create a hidden id'd label and use aria-labelledby); keep
existing props (checked, disabled, onCheckedChange -> handleChangeGatingToggle,
data-testid, className) and ensure gatingUpdating.has(repo.repo_full_name) logic
remains unchanged.
In `@server/routes/github/github_repo_selection.py`:
- Around line 296-317: The LEFT JOIN allows connected_repos.r.installation_id to
be non-null while no matching github_installations row exists, so the code must
ensure the joined installation (alias i) actually exists and is not suspended
before treating it as active: either change the query to an INNER JOIN between
connected_repos r and github_installations i (or add "AND i.installation_id IS
NOT NULL" to the WHERE) so i is guaranteed present, or keep the LEFT JOIN but
return 409 when the joined installation columns are NULL; update the downstream
checks around row (currently using row[0] and row[1]) to explicitly verify i
exists (e.g., check the joined installation column is not NULL) before assuming
an active installation.
In `@server/services/change_gating/github_adapter.py`:
- Around line 91-112: The function find_aurora_reviews currently accepts any
review authored by an account with user.type == "Bot" if the body has an Aurora
marker; instead require the reviewer to match Aurora's configured bot identity
(e.g., compare user.get("login") or user.get("id") to the expected Aurora bot
login/id) before accepting the review. Update find_aurora_reviews (and the
related find_latest_aurora_review call sites/tests) to take an
expected_aurora_bot identifier parameter (login or id), check that value against
each review's user field in addition to has_aurora_marker(review.get("body")),
and propagate the new parameter through callers and unit tests so only reviews
from the configured Aurora bot are returned.
In `@server/services/change_gating/verdict.py`:
- Around line 179-187: The description string is interpolating
attacker-controlled PR fields (pr.get('title'), pr.get('body')) and the diff
block without escaping, so update the construction of description to pass title
and body through an escaping helper (call _escape_prompt_data on pr.get('title')
and pr.get('body')) and likewise sanitize the diff content before appending to
diff_block/diff_excerpt; ensure _escape_prompt_data exists and safely
neutralizes delimiter sequences like "</pr_description>" and triple backticks,
then use that helper wherever description, diff_block, or diff_excerpt are built
(e.g., in the description assignment and the diff_block/diff_excerpt population)
so all user-controlled strings are escaped before interpolation.
In `@server/tasks/change_gating.py`:
- Around line 142-160: The RLS session variables are only reset on the happy
path causing pooled connections to leak tenant context; update the three call
sites (_verify_enrollment, _read_final_assistant_message,
_resolve_change_gating_owner) to guarantee cleanup by either (preferred)
changing set_rls_context to use transaction-scoped SET LOCAL so settings
auto-reset at transaction end, or if retaining connection-scoped SET, wrap the
call and subsequent DB work in try/finally and always execute the RESET
statements ("RESET myapp.current_user_id; RESET myapp.current_org_id;") in the
finally block to ensure the pooled connection never returns with stale RLS
context. Ensure references to set_rls_context and the RESET statements are
preserved and applied consistently across all three functions.
In `@server/utils/db/db_utils.py`:
- Around line 1601-1616: The migration that adds the change_gating_enabled
column currently swallows errors in the except block (the try around
cursor.execute("ALTER TABLE connected_repos ADD COLUMN IF NOT EXISTS
change_gating_enabled ...")), which can let the service boot with a broken
schema; modify the except block to fail fast by logging the error (use
logging.error) and then re-raising the exception or exiting the process instead
of just calling conn.rollback(), so any failure to add change_gating_enabled
during startup halts boot and surfaces the DB migration error.
🪄 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
Run ID: 4b73e655-9c8e-4832-896f-875d5683534c
📒 Files selected for processing (26)
.env.example.github/workflows/linters.ymlclient/src/components/github-provider-integration.tsxdocker-compose.airtight.ymldocker-compose.prod-local.ymldocker-compose.yamlserver/celery_config.pyserver/chat/backend/agent/agent.pyserver/chat/backend/agent/orchestrator/dispatcher.pyserver/chat/backend/agent/orchestrator/sub_agent.pyserver/chat/backend/agent/utils/state.pyserver/chat/background/task.pyserver/routes/github/github_repo_selection.pyserver/services/change_gating/__init__.pyserver/services/change_gating/diff_utils.pyserver/services/change_gating/github_adapter.pyserver/services/change_gating/verdict.pyserver/tasks/change_gating.pyserver/tasks/github_webhook_tasks.pyserver/tests/chat/test_tool_denylist.pyserver/tests/services/test_change_gating_adapter.pyserver/tests/services/test_change_gating_diff_utils.pyserver/tests/services/test_change_gating_verdict.pyserver/tests/tasks/__init__.pyserver/tests/tasks/test_change_gating_handler.pyserver/utils/db/db_utils.py
…ity; prompt-injection escaping; orphan-install guard; a11y label; sonar cleanups)
…ict literal, RESET RLS constant, globalThis)
Contributor
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 `@server/tasks/github_webhook_tasks.py`:
- Around line 697-700: The RLS reset (_RESET_RLS_SQL) can be skipped if an
exception occurs after set_rls_context succeeds in the probing loop; to fix,
wrap the loop body that calls set_rls_context and cur.execute in a try/finally
so cur.execute(_RESET_RLS_SQL) always runs in the finally block, or convert the
RLS change to a transaction-local setting (SET LOCAL) so cleanup is automatic;
update the probing loop around set_rls_context and the linked_users cur.execute
calls to ensure the RESET is executed regardless of exceptions.
🪄 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
Run ID: b0e78b7b-24f2-4a7a-85ca-196c9006b243
📒 Files selected for processing (3)
client/src/components/github-provider-integration.tsxserver/tasks/change_gating.pyserver/tasks/github_webhook_tasks.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@server/services/change_gating/diff_utils.py`:
- Around line 135-237: Extract the per-file block rendering logic from the
`build_per_file_diff` function into a separate nested helper function. Create a
new helper function (e.g., `_build_file_block`) that accepts the file dict, the
escape function, and the max_file_chars parameter, and returns the formatted
block string. Move the logic that constructs the header with filename and stats,
handles the missing patch case, applies truncation with the note, and builds the
complete block into this helper. Then replace the inline block construction in
the loop with a call to this new helper, passing the necessary parameters. This
refactoring will reduce the cognitive complexity of the main function by
extracting a self-contained concern into a separate, reusable helper.
In `@server/tests/auth/test_github_auth_mode.py`:
- Around line 13-18: The `_set_mode` function is missing an explicit return type
annotation, which causes Ruff ANN202 to flag it. Add a return type annotation of
`-> None` to the function signature to indicate that the function does not
return a value. This should be added to the function definition line between the
closing parenthesis of the parameters and the colon.
In `@server/tests/tasks/test_github_repo_import.py`:
- Around line 122-127: The variable conn is assigned from the _run function call
but never used in the test assertions that follow. Replace conn with an
underscore (_) in the tuple unpacking at the _run call to indicate the variable
is intentionally unused and satisfy the linter.
- Around line 51-65: The mocked `db_pool` is not being picked up because
`import_installation_repos` performs a deferred import of `from
utils.db.connection_pool import db_pool` inside its function body, which creates
a new local binding after the patch is applied. Instead of patching at the
source `utils.db.connection_pool.db_pool`, patch the attribute where it is
actually used by the task module. Change the patch target from
`patch("utils.db.connection_pool.db_pool", db_pool)` to
`patch("import_installation_repos.db_pool", db_pool)` (or the appropriate module
path where the task imports and uses db_pool), ensuring the mock is injected
into the task's namespace before the function executes and performs its import.
🪄 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
Run ID: 4201a5b7-2a79-4c06-9f28-50f1baa7baf0
📒 Files selected for processing (18)
client/src/components/connectors/ConnectorRegistry.tsclient/src/components/github-provider-integration.tsxserver/chat/backend/agent/tools/github_repos_tool.pyserver/routes/connector_status.pyserver/routes/github/github_app.pyserver/routes/github/github_oauth.pyserver/routes/github/github_repo_metadata.pyserver/routes/github/github_repo_selection.pyserver/routes/github/github_user_repos.pyserver/services/change_gating/diff_utils.pyserver/services/change_gating/verdict.pyserver/tasks/change_gating.pyserver/tests/auth/test_github_auth_mode.pyserver/tests/services/test_change_gating_diff_utils.pyserver/tests/services/test_change_gating_verdict.pyserver/tests/tasks/test_github_repo_import.pyserver/utils/auth/github_auth_mode.pyserver/utils/auth/github_auth_router.py
…ed celery) can run it
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/routes/github/github_repo_metadata.py (1)
260-296:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t derive repo ownership and “new row” side effects from a stale snapshot.
existingis loaded before the UPSERT, then reused to chooseowner_idand enqueue metadata. Concurrent/retried imports for the same org can both see no row; because the conflict target is(user_id, provider, repo_full_name), different users can create duplicate same-org repo rows, and same-user races can still double-dispatch metadata. Serialize this section per org/repo or enforce an org-level uniqueness contract, then derivenewly_addedfrom the database-enforced insert outcome rather than the pre-UPSERT map.🤖 Prompt for 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. In `@server/routes/github/github_repo_metadata.py` around lines 260 - 296, The existing map is loaded as a stale snapshot before the UPSERT operations, causing race conditions where concurrent imports can both think they are the first to insert a repo. Instead of deriving owner_id and newly_added from the pre-UPSERT map, you need to either add row-level locking or check the actual insert outcome from the database. Replace the logic that populates existing before the loop and then uses it to determine owner_id and track newly_added with logic that either locks rows per org/repo during the operation, or queries the database after each UPSERT (via the cursor's rowcount or other insert-result mechanism) to determine what was actually inserted versus updated, then populate newly_added based on those verified insert outcomes rather than the stale pre-UPSERT snapshot.
🤖 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 `@server/routes/github/github_repo_metadata.py`:
- Line 199: The _import_installation_repos method is missing a return type
annotation. Since the method only exits via bare return statements or
fallthrough that implicitly return None, add the `-> None` return type
annotation to the method signature after the closing parenthesis of the
parameters.
In `@server/tests/tasks/test_github_repo_import.py`:
- Around line 28-35: Add a return type annotation to the _task_self function to
satisfy Ruff's ANN202 check. The function currently returns a SimpleNamespace
object without a type hint; add the appropriate return type annotation
indicating that the function returns a SimpleNamespace instance.
---
Outside diff comments:
In `@server/routes/github/github_repo_metadata.py`:
- Around line 260-296: The existing map is loaded as a stale snapshot before the
UPSERT operations, causing race conditions where concurrent imports can both
think they are the first to insert a repo. Instead of deriving owner_id and
newly_added from the pre-UPSERT map, you need to either add row-level locking or
check the actual insert outcome from the database. Replace the logic that
populates existing before the loop and then uses it to determine owner_id and
track newly_added with logic that either locks rows per org/repo during the
operation, or queries the database after each UPSERT (via the cursor's rowcount
or other insert-result mechanism) to determine what was actually inserted versus
updated, then populate newly_added based on those verified insert outcomes
rather than the stale pre-UPSERT snapshot.
🪄 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
Run ID: 3ca6f304-567c-474d-95da-bd50d471840a
📒 Files selected for processing (2)
server/routes/github/github_repo_metadata.pyserver/tests/tasks/test_github_repo_import.py
…nd drop unused test var
damianloch
requested changes
Jun 16, 2026
… into connectors page
…ape prior_findings
…me_limit to 15min
…nd proactive correlation
…move dead denylist infrastructure
…ng CHANGE_GATING_DRY_RUN
…heavy diffs trigger false positives)
damianloch
requested changes
Jun 18, 2026
damianloch
requested changes
Jun 18, 2026
damianloch
approved these changes
Jun 18, 2026
damianloch
deleted the
sms10221/dev-1252-alert-prediction-via-change-gate
branch
June 18, 2026 20:00
|
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
3 participants
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
Aurora now reviews pull requests on enrolled repos' default branches and posts a GitHub App review — APPROVE when the change looks safe, COMMENT with inline findings when it carries incident risk. Like CodeRabbit, but for incident prevention rather than code quality. Opt-in per repo via an "Incident Prevention" toggle; default off.
How it works
opened/reopened/ready_for_review/synchronizeagainst the default branch of an enrolled, App-installed repo. Drafts, non-default-branch, not-enrolled, and suspended-install PRs are skipped. Redis dedupe absorbs redeliveries.run_background_chat,mode=ask) with a read-only tool denylist (no write/exec/IaC tools). The agent investigates with the same read-only tools as RCA (logs, metrics, postmortems, etc.) and returns a SAFE/RISKY verdict with findings.connected_repos.change_gating_enabledboolean column only.Scope
No Aurora UI tab, no Slack notifications, no REQUEST_CHANGES (advisory only — never blocks merge), no rate limiting. GitLab/Bitbucket left as a future adapter behind the same class boundary. A
CHANGE_GATING_DRY_RUNflag logs the would-be review instead of posting (calibration before granting write perms).Test plan
requests), diff-hunk math, verdict parsing/rendering, webhook filter matrix, and the per-run tool denylist.pre-checksgreen.Manual follow-up
Summary by CodeRabbit