Repository navigation
Feat/auto approve - #133
Conversation
LiveReview Pre-Commit Check: ran (iter:1, coverage:0%)
Implements a layered funnel (eligibility -> static jq gates -> trained risk score -> LLM classification -> decision) for the PR-link auto-approve flow, replacing the previous "no critical findings" placeholder. Includes: - Org/repo-scoped policy config (auto_approve_policies) with an online-learning risk model (internal/riskscore) anchored to a common-sense prior, category-level weight overrides, and daily + on-demand retraining. - Static gate reuse via internal/canonicaldoc (shared with CI/CD rulesets). - Eligibility by per-user review level, with identity mapping for PR authors and a whitelist bypass. - Review detail "Auto Approve" tab, Settings policy editor (Risk Model / Static Rules / Eligibility / General / Production Feedback sub-tabs), and repository-level overrides reusing the same field components. - Force Approve: an audited, level-gated override for a specific blocked review, and manual/webhook production-outcome reporting that feeds the model's online update. Fixes found during review and testing: a malformed claim-UPDATE string that broke Force Approve and landing-delay publish on every attempt, a cold-start 500 on a never-scored org's risk-model endpoint, and a missing org-ownership check on the manual outcome endpoint. Added regression tests for all three plus policy resolution and canonical-doc scoping. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Risk-based auto-approve introduces a lot of unfamiliar settings (risk threshold percentile, LLM confidence, blast radius weighting, landing delay, force approve, etc.), so most users will not guess their meaning from the label alone. Adds an "i" info icon with a hover tooltip next to every control across the Settings policy editor, the repository override section, and the review detail Auto Approve tab's funnel/force-approve/ outcome panels. New shared pieces: InfoHint/FieldLabel (built on the existing Tooltip primitive) and a single helpText.ts holding every hint's copy, written in Simplified Technical English (short sentences, one idea each, concrete examples) so it stays easy to review and keep consistent. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
The funnel ran risk score before LLM classification and skipped the LLM
gate entirely ("skipped (earlier stage blocked)") whenever risk score
already failed. But the LLM gate makes no live call at that point — it's
a threshold check against the diffClassification the review-generation
prompt already wrote to metadata.review_result, available the whole time
the funnel runs. Evaluate risk and LLM independently so both verdicts
always show, and block on either; the final-decision reason now lists
every failing gate instead of only the first one found.
Also makes each policy funnel stage expandable in the Auto Approve tab,
showing the detail the backend already computed but didn't render: the
risk score's full feature-by-feature breakdown, the LLM gate's class and
confidence, static's matched ruleset, etc.
LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
The risk model only let admins tune 4 generic signals (blast radius, security findings, critical findings, diff size) even though every finding is already classified into one of 10 categories (Security, Reliability, Correctness, Performance, Scalability, Compliance & Governance, Maintainability, Architecture, Developer Experience, Cost). Add a feature + override slider for each of those categories, append-only so old trained model versions keep scoring correctly. Security defaults 1.5x, Correctness/Reliability 1.25x, Performance 1.1x, everything else 1x, reflecting how most teams actually prioritize these. Also: - Debounce the category sliders (save 300ms after the user stops dragging, with instant local feedback) instead of firing a request and a full model refetch on every drag tick, which could visibly snap the thumb back mid-drag. - Rename "Auto Approve" to "Auto Approve Policies" in the mega menu and Settings tab list so it's easier to find by search. - Put the General sub-tab first, since it holds the settings people reach for most often. - Format the Production Feedback webhook body as a real code block instead of a single dense inline string. - Make every policy hover-hint readable: wider tooltip padding/line-height, explicit paragraph breaks instead of one run-on sentence, and each category's sub-issues rendered as a bulleted list instead of a long comma-separated clause. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
LiveReview Pre-Commit Check: ran (iter:2, coverage:9%)
…r super admins Every other owner-only link in megaMenuData.ts is gated by `ctx.isSuperAdmin || ctx.orgRole === 'owner'`, but this one was missing the isSuperAdmin fallback. A super admin could already open Settings -> Auto Approve Policies directly (that tab's own gate, canManageCurrentOrg, already accounts for isSuperAdmin), but the menu/search entry used a stricter, inconsistent check and silently disappeared from both the dropdown and Ctrl-K search, which share the same filtered list. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Two sliders (critical findings, diff size) were hidden behind a collapsed "Other risk signals" details element, and blast radius sat at the very bottom of that hidden group even though it's the strongest single risk signal the model has - how far a change's effects reach, independent of which finding category triggered it. Hiding any of these behind a fold made them easy to miss and implied they mattered less than the finding categories above them. - Move blast radius to the top of the slider list and remove the collapsed section entirely, so every weight dial is visible by default. - Give blast radius the highest default multiplier (2.0x), ahead of security's 1.5x, matching how strongly it should influence scoring. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
…table report The risk-based auto-approve policy engine (internal/policyengine, internal/riskscore) was already built and wired up, but there was no way to verify it behaved sensibly on common-sense inputs, no proof the ML feedback loop actually learns anything, and no explanation anyone outside the implementation could follow. Adds: - A 46-case scenario corpus (internal/policyengine/testdata/scenarios.json) run against the real engine.RunWithPolicy dry-run path, covering every funnel stage, all 10 finding-taxonomy categories, mode shortcuts, threshold boundaries, and daily caps/delays. - Formal-ish property/unit tests on the model's math (category-override precedence, prior monotonicity, anchor decay) and DB-backed tests on the learning paths (batch refit's promotion gate, the real-time online nudge, the /simulate handler's no-side-effects guarantee). - A threshold experiment (internal/riskscore/threshold_experiment_test.go) computing real approval rates at P0/P10/P20/P50/P80/P100 against a shared, reproducible reference distribution (internal/riskscore/referencemix), proving the percentile setting and the cutoff it resolves to are two different things. - A learning-cycle test showing a review's score move 0.668 -> 0.975 -> 0.999 as synthetic production outcomes are reported against it, with the full before/after weight vector logged, not just the score. - docs/risk-policy-explainability.md and docs/risk-policy-verification-report.html (an interactive report with a live score calculator, sortable results/threshold tables, and a full signal reference) explaining all of this at CTO/manager/engineer depth. - scripts/regen_risk_report.py (make regen-risk-report): regenerates every number in the HTML report directly from the three tests' structured log output, so the report can't silently drift from the model it describes. Refuses to run if any test fails. Every test cleans up everything it writes (verified by running the full suite twice and checking row counts return to zero), so it's safe to run repeatedly against any database, including a live instance. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
…ration The auto-approve policy's only production visibility was one-review-at-a- time (the explain panel) and one-shot historical replay (Simulate). There was no way to see how the policy is actually behaving on an org's real, live traffic: where scores land relative to the threshold, how many PRs are being approved/blocked/overridden, or how often a reported outcome disagreed with the decision. Adds: - GET /api/v1/auto-approve-insights (internal/api/auto_approve_insights_handlers.go): aggregates review_policy_log, risk_feature_snapshots, review_policy_overrides, and review_outcomes (excluding source='test', same exclusion the training path uses) over a selectable 7/30/90-day window into a score histogram with the resolved cutoff, decision counts/trend/override rate, and an outcome-calibration cross-tab with a false-approve rate. - A Reports -> Auto Approve tab (ui/src/pages/Reports/AutoApproveReport.tsx) rendering all three, plus a mega-menu entry and a link from Settings' existing Production Feedback tab. - A handler test seeding two orgs and asserting both the aggregation math and tenant isolation (no org can see another org's counts/snapshots/ outcomes/overrides). LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
LiveReview Pre-Commit Check: ran (iter:1, coverage:0%)
LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Adds the Auto-Approve pitch (Meta RADAR-inspired risk-based auto-approval, three risk signals, the Track/Learn/Automate/Improve playbook) ahead of the existing blast-radius explanation, mirroring the site's new hero ordering. Adds an Auto-Approve anchor and wires it into the pillar table and the Get Started nav table. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Auto-Approve now comes first as its own section (matches the site's new hero ordering) with a clickable demo video; the existing blast-radius content becomes its own "Within-Diff Prioritization" section below it, since the two are different capabilities. Updates the nav table's link to the relocated blast-radius content accordingly. Note: the Auto-Approve demo is committed to the repo and linked as a clickable thumbnail rather than an inline player, since GitHub only auto-embeds videos uploaded through its web drag-and-drop flow, not ones committed directly to a repo. LiveReview Pre-Commit Check: skipped (iter:2, coverage:0%)
- "Not approved" instead of "Changes requested" in PR comments and the decisions table - Risk threshold presets (Conservative/Balanced/Aggressive) with a P-marked custom slider - Risk reason shows score and threshold with their percentiles - Auto-approve stat next to Risk assessment, pulsing dot on the Auto Approve tab LiveReview Pre-Commit Check: vouched (iter:3, coverage:0%)
Drops pg_dump version, River job table/index and review_events sequence drift; keeps this branch's migration changes. LiveReview Pre-Commit Check: vouched (iter:1, coverage:0%)
Implement Risk-Based Auto-Approval Policy EngineOverviewThis change introduces a new risk-based auto-approval policy engine. It evaluates pull requests using a multi-layered architecture. This system aims to reduce reviewer fatigue and focus human attention on high-risk changes. Technical Highlights
Impact
|
| review_id bigint NOT NULL, | ||
| org_id bigint NOT NULL, | ||
| ts timestamp with time zone DEFAULT now() NOT NULL, | ||
| event_type text NOT NULL, |
There was a problem hiding this comment.
Severity: warning
The event_type field is currently a text type. Consider using an enum for stricter data validation and improved query performance.
Suggestions:
- Use a
CHECKconstraint or PostgreSQLENUMtype forevent_typeto restrict values and improve data integrity. - Document expected
event_typevalues.
| org_id bigint NOT NULL, | ||
| ts timestamp with time zone DEFAULT now() NOT NULL, | ||
| event_type text NOT NULL, | ||
| level text, |
There was a problem hiding this comment.
Severity: warning
The level field is currently a text type. Consider using an enum for stricter data validation and improved query performance.
Suggestions:
- Use a
CHECKconstraint or PostgreSQLENUMtype forlevelto restrict values and improve data integrity. - Document expected
levelvalues.
| ts timestamp with time zone DEFAULT now() NOT NULL, | ||
| event_type text NOT NULL, | ||
| level text, | ||
| batch_id text, |
There was a problem hiding this comment.
Severity: info
The batch_id field is currently a text type. If it represents a UUID, consider using the UUID data type.
Suggestions:
- If
batch_idis always a UUID, useuuidtype for better type safety and storage efficiency.
| review_id bigint NOT NULL, | ||
| org_id bigint NOT NULL, | ||
| reported_by_user_id bigint, | ||
| outcome_type text NOT NULL, |
There was a problem hiding this comment.
Severity: info
The outcome_type field is currently a text type. While it already has a CHECK value, using an enum would make the possible values more explicit.
Suggestions:
- Consider using a PostgreSQL
ENUMtype foroutcome_typefor stronger type enforcement and clarity, instead of aCHECKconstraint.
| org_id bigint NOT NULL, | ||
| reported_by_user_id bigint, | ||
| outcome_type text NOT NULL, | ||
| source text NOT NULL, |
There was a problem hiding this comment.
Severity: info
The source field is currently a text type. While it already has a CHECK value, using an enum would make the possible values more explicit.
Suggestions:
- Consider using a PostgreSQL
ENUMtype forsourcefor stronger type enforcement and clarity, instead of aCHECKconstraint.
| id bigint NOT NULL, | ||
| review_id bigint NOT NULL, | ||
| org_id bigint NOT NULL, | ||
| stage text NOT NULL, |
There was a problem hiding this comment.
Severity: warning
The stage field is currently a text type. Consider using an enum for stricter data validation and improved query performance.
Suggestions:
- Use a
CHECKconstraint or PostgreSQLENUMtype forstageto restrict values and improve data integrity. - Document expected
stagevalues.
| review_id bigint NOT NULL, | ||
| org_id bigint NOT NULL, | ||
| stage text NOT NULL, | ||
| verdict text NOT NULL, |
There was a problem hiding this comment.
Severity: warning
The verdict field is currently a text type. Consider using an enum for stricter data validation and improved query performance.
Suggestions:
- Use a
CHECKconstraint or PostgreSQLENUMtype forverdictto restrict values and improve data integrity. - Document expected
verdictvalues.
| CREATE TABLE public.risk_factor_overrides ( | ||
| id bigint NOT NULL, | ||
| org_id bigint NOT NULL, | ||
| feature_category text NOT NULL, |
There was a problem hiding this comment.
Severity: warning
The feature_category field is currently a text type. Consider using an enum for stricter data validation and improved query performance.
Suggestions:
- Use a
CHECKconstraint or PostgreSQLENUMtype forfeature_categoryto restrict values and improve data integrity. - Document expected
feature_categoryvalues.
|
|
||
| CREATE TABLE public.risk_model_versions ( | ||
| id bigint NOT NULL, | ||
| org_id bigint, |
There was a problem hiding this comment.
Severity: info
The org_id field is nullable. This implies global models. Please clarify the intended behavior and scope.
Suggestions:
- Add a comment explaining when
org_idis NULL (e.g., for global default models). - Ensure application logic correctly handles NULL
org_idfor model selection.
| trained_at timestamp with time zone DEFAULT now() NOT NULL, | ||
| training_sample_count integer DEFAULT 0 NOT NULL, | ||
| holdout_auc double precision, | ||
| update_kind text NOT NULL, |
There was a problem hiding this comment.
Severity: info
The update_kind field is currently a text type. While it already has a CHECK value, using an enum would make the possible values more explicit.
Suggestions:
- Consider using a PostgreSQL
ENUMtype forupdate_kindfor stronger type enforcement and clarity, instead of aCHECKconstraint.
|
|
||
| CREATE TABLE public.upgrade_request_events ( | ||
| id bigint DEFAULT nextval('public.upgrade_request_events_id_seq'::regclass) NOT NULL, | ||
| upgrade_request_id character varying(36) NOT NULL, |
There was a problem hiding this comment.
Severity: warning
The upgrade_request_id field is of type character varying(36). If it represents a UUID, please use the uuid data type.
Suggestions:
- Change
upgrade_request_idtouuidtype for better type safety, validation, and storage efficiency.
| id bigint DEFAULT nextval('public.upgrade_request_events_id_seq'::regclass) NOT NULL, | ||
| upgrade_request_id character varying(36) NOT NULL, | ||
| org_id bigint NOT NULL, | ||
| event_source character varying(64) NOT NULL, |
There was a problem hiding this comment.
Severity: warning
The event_source field is of type character varying(64). Consider using an enum for stricter data validation.
Suggestions:
- Use a
CHECKconstraint or PostgreSQLENUMtype forevent_sourceto restrict values and improve data integrity. - Document expected
event_sourcevalues.
| upgrade_request_id character varying(36) NOT NULL, | ||
| org_id bigint NOT NULL, | ||
| event_source character varying(64) NOT NULL, | ||
| event_type character varying(64) NOT NULL, |
There was a problem hiding this comment.
Severity: warning
The event_type field is of type character varying(64). Consider using an enum for stricter data validation.
Suggestions:
- Use a
CHECKconstraint or PostgreSQLENUMtype forevent_typeto restrict values and improve data integrity. - Document expected
event_typevalues.
| is_active BOOLEAN NOT NULL DEFAULT false | ||
| ); | ||
|
|
||
| CREATE INDEX idx_risk_model_versions_org_active ON risk_model_versions (org_id) WHERE is_active; |
There was a problem hiding this comment.
Severity: info
The index on org_id and is_active for risk_model_versions is critical for the performance of active model lookups.
| feature_category TEXT NOT NULL, | ||
| multiplier DOUBLE PRECISION NOT NULL DEFAULT 1.0, | ||
| updated_at TIMESTAMPTZ NOT NULL DEFAULT now(), | ||
| UNIQUE (org_id, feature_category) |
There was a problem hiding this comment.
Severity: info
The UNIQUE (org_id, feature_category) constraint on risk_factor_overrides ensures that there is only one override per category per organization.
| raw_llm_output text NOT NULL, | ||
| created_at timestamp with time zone DEFAULT now() NOT NULL, | ||
| context text[], | ||
| context jsonb, |
There was a problem hiding this comment.
Severity: warning
The type change for chat_charts.context from text[] to jsonb requires ensuring that all code paths correctly handle jsonb.
Suggestions:
- Verify all application code interacting with
chat_charts.contextis updated forjsonbtype.
| data bytea NOT NULL, | ||
| created_at timestamp with time zone DEFAULT now() NOT NULL, | ||
| context text[] | ||
| context jsonb |
There was a problem hiding this comment.
Severity: warning
The type change for chat_files.context from text[] to jsonb requires ensuring that all code paths correctly handle jsonb.
Suggestions:
- Verify all application code interacting with
chat_files.contextis updated forjsonbtype.
| ALTER SEQUENCE public.review_commits_id_seq OWNED BY public.review_commits.id; | ||
|
|
||
|
|
||
| -- |
There was a problem hiding this comment.
Severity: warning
The review_events table has been removed. Please confirm that this deletion is intentional and that no data loss has occurred.
Suggestions:
- Confirm
review_eventstable removal was intentional and data migration/archiving was handled if necessary.
| event_type text NOT NULL, | ||
| level text, | ||
| batch_id text, | ||
| data jsonb NOT NULL |
There was a problem hiding this comment.
Severity: warning
The jsonb column data is marked as NOT NULL but lacks a default empty object. If empty data is considered valid, please add DEFAULT '{}'::jsonb.
Suggestions:
- Change
data jsonb NOT NULLtodata jsonb DEFAULT '{}'::jsonb NOT NULLif empty JSON is a valid state.
| id bigint NOT NULL, | ||
| review_id bigint NOT NULL, | ||
| org_id bigint NOT NULL, | ||
| reported_by_user_id bigint, |
There was a problem hiding this comment.
Severity: info
The reported_by_user_id column is nullable. Please clarify whether system-generated outcomes should link to a specific system user ID or if NULL is acceptable.
Suggestions:
- Add a comment explaining when
reported_by_user_idcan be NULL. - Consider a default system user ID if all outcomes must be attributable.
| org_id bigint NOT NULL, | ||
| feature_category text NOT NULL, | ||
| multiplier double precision DEFAULT 1.0 NOT NULL, | ||
| updated_at timestamp with time zone DEFAULT now() NOT NULL |
There was a problem hiding this comment.
Severity: warning
The updated_at column has DEFAULT now() but no ON UPDATE trigger. The application must manually update this field.
Suggestions:
- Implement an
ON UPDATEtrigger forupdated_atto automatically update on row modification. - Ensure all application updates to
risk_factor_overridesexplicitly setupdated_at.
| review_id bigint NOT NULL, | ||
| org_id bigint NOT NULL, | ||
| model_version integer NOT NULL, | ||
| features jsonb NOT NULL, |
There was a problem hiding this comment.
Severity: warning
The jsonb column features is marked as NOT NULL but lacks a default empty object. If empty features are considered valid, please add DEFAULT '{}'::jsonb.
Suggestions:
- Change
features jsonb NOT NULLtofeatures jsonb DEFAULT '{}'::jsonb NOT NULLif empty JSON is a valid state.
|
|
||
| CREATE TABLE public.risk_model_versions ( | ||
| id bigint NOT NULL, | ||
| org_id bigint, |
There was a problem hiding this comment.
Severity: warning
Consider adding a unique constraint on (org_id, version) for risk_model_versions to prevent duplicate versions within an organization.
Suggestions:
- Add
ALTER TABLE public.risk_model_versions ADD CONSTRAINT risk_model_versions_org_id_version_key UNIQUE (org_id, version);
| training_sample_count integer DEFAULT 0 NOT NULL, | ||
| holdout_auc double precision, | ||
| update_kind text NOT NULL, | ||
| is_active boolean DEFAULT false NOT NULL, |
There was a problem hiding this comment.
Severity: warning
An is_active flag exists. If only one model can be active per org_id (or globally), please add a partial unique index.
Suggestions:
- Add
CREATE UNIQUE INDEX risk_model_versions_active_idx ON public.risk_model_versions (org_id) WHERE is_active = true;to enforce single active model per org.
|
|
||
| CREATE TABLE public.upgrade_request_events ( | ||
| id bigint DEFAULT nextval('public.upgrade_request_events_id_seq'::regclass) NOT NULL, | ||
| upgrade_request_id character varying(36) NOT NULL, |
There was a problem hiding this comment.
Severity: warning
The upgrade_request_id is currently character varying(36). If this field represents a UUID, using the uuid type would be more efficient for storage and validation.
Suggestions:
- Change
upgrade_request_id character varying(36)toupgrade_request_id uuid.
| value text NOT NULL, | ||
| created_by bigint, | ||
| created_at timestamp with time zone DEFAULT now() NOT NULL, | ||
| updated_at timestamp with time zone DEFAULT now() NOT NULL, |
There was a problem hiding this comment.
Severity: warning
The updated_at column has DEFAULT now() but no ON UPDATE trigger. The application must manually update this field.
Suggestions:
- Implement an
ON UPDATEtrigger forupdated_atto automatically update on row modification. - Ensure all application updates to
user_identitiesexplicitly setupdated_at.
| created_by bigint, | ||
| created_at timestamp with time zone DEFAULT now() NOT NULL, | ||
| updated_at timestamp with time zone DEFAULT now() NOT NULL, | ||
| CONSTRAINT user_identities_kind_check CHECK ((kind = ANY (ARRAY['email'::text, 'username'::text]))) |
There was a problem hiding this comment.
Severity: warning
Add a unique constraint on (org_id, kind, value) to prevent duplicate identities for the same user and organization.
Suggestions:
- Add
ALTER TABLE public.user_identities ADD CONSTRAINT user_identities_org_id_kind_value_key UNIQUE (org_id, kind, value);
|
|
||
| ALTER TABLE ONLY public.review_commits | ||
| ADD CONSTRAINT review_commits_repository_id_fkey FOREIGN KEY (repository_id) REFERENCES public.repositories(id); | ||
| ADD CONSTRAINT review_commits_repository_id_fkey FOREIGN KEY (repository_id) REFERENCES public.repositories(id) ON DELETE SET NULL; |
There was a problem hiding this comment.
Severity: warning
The foreign key ON DELETE SET NULL changes the behavior; therefore, repository_id must be nullable.
Suggestions:
- Confirm
review_commits.repository_idcolumn is nullable to avoid constraint errors on repository deletion.
| -- | ||
|
|
||
| ALTER TABLE ONLY public.upgrade_request_events | ||
| ADD CONSTRAINT upgrade_request_events_org_id_fkey FOREIGN KEY (org_id) REFERENCES public.orgs(id) ON DELETE CASCADE; |
There was a problem hiding this comment.
Severity: warning
The foreign key ON DELETE CASCADE implies that the history in upgrade_request_events is not needed after an organization is deleted.
Suggestions:
- Verify
ON DELETE CASCADEis the intended behavior; considerSET NULLorNO ACTIONif event history must persist for auditing or debugging.
| -- | ||
|
|
||
| ALTER TABLE ONLY public.upgrade_request_events | ||
| ADD CONSTRAINT upgrade_request_events_upgrade_request_id_fkey FOREIGN KEY (upgrade_request_id) REFERENCES public.upgrade_requests(upgrade_request_id) ON DELETE CASCADE; |
There was a problem hiding this comment.
Severity: warning
The foreign key ON DELETE CASCADE implies that the history in upgrade_request_events is not needed after a request is deleted.
Suggestions:
- Verify
ON DELETE CASCADEis the intended behavior; considerSET NULLorNO ACTIONif event history must persist for auditing or debugging.
| | Auth | Both links require a LiveReview login. Logged-out users log in and are sent back to the link (existing `redirectAfterLogin` flow). | | ||
| | Org | The user is switched to the PR's org automatically. Non-members get "not found". | | ||
| | Trigger behavior | Opening the trigger link starts the review immediately (no confirm page, for now) | | ||
| | Repeat clicks | Each click starts a new review, **unless** a review for the PR is `created`/`in_progress` and started less than 10 minutes ago. Then it redirects to that review. | |
There was a problem hiding this comment.
Severity: info
A 10-minute reuse window for repeat clicks is a good mitigation for accidental review starts from link previewers.
|
|
||
| 1. Skip unless `event.Metadata["action"]` is `opened` (GitHub) or `open` (GitLab). | ||
| 2. Claim the post atomically: | ||
| `UPDATE pull_requests SET links_comment_posted_at = now() WHERE id = $1 AND links_comment_posted_at IS NULL`. |
There was a problem hiding this comment.
Severity: info
The atomic update for links_comment_posted_at prevents race conditions that could occur from concurrent webhook deliveries.
| 3. Load the repo token via the existing `repositoryConnectorInfo` lookup. | ||
| 4. Read the public URL via the webhook store's `GetWebhookPublicEndpoint` (inject the store | ||
| into the worker in `jobqueue.go`). | ||
| 5. Read the description, append the section if the marker is missing, and save it. On failure, reset `links_comment_posted_at` to `NULL` and return the |
There was a problem hiding this comment.
Severity: info
Resetting links_comment_posted_at on failure ensures that River retries and prevents a stale state.
| middleware chain can't be used here (approved exception). | ||
| - Handler looks up `pull_requests.org_id` and verifies the user is an active member of that | ||
| org. Returns `{ "org_id": ... }`, or **404** if the PR doesn't exist or the user isn't a | ||
| member (no existence leak). |
There was a problem hiding this comment.
Severity: info
Returning a 404 for a non-existent PR or a non-member is a good security practice as it prevents existence leaks.
| filter is an option on the existing Reviews page. | ||
|
|
||
| ## Known limitations | ||
|
|
There was a problem hiding this comment.
Severity: info
The risk of link previewers initiating reviews has been acknowledged, and it's good to document this and plan for a future confirmation page.
| In plain terms, the model works like this: | ||
|
|
||
| ``` | ||
| z = bias + sum over every signal of (its value x its weight x its category importance) |
There was a problem hiding this comment.
Severity: warning
The formula variables 'value', 'weight', and 'category importance' lack clear definitions. Please clarify how each of these is determined (e.g., raw, normalized, learned, or configured).
Suggestions:
- Add a brief explanation for each term in the formula (e.g., 'its value' is the normalized signal, 'its weight' is learned, 'its category importance' is configured).
| "safe enough" means something different in those two situations. | ||
|
|
||
| Finally, the model watches its own track record. Every outcome reported | ||
| from production — a revert, an incident, a confirmed-safe merge — nudges |
There was a problem hiding this comment.
Severity: warning
The phrase 'small, careful amount' for model nudges is vague. Please specify how this amount is determined (e.g., fixed learning rate, adaptive, or bounded).
Suggestions:
- Elaborate on the mechanism for determining the 'small, careful amount' for real-time nudges, including any safeguards against over-correction.
|
|
||
| ## Risks and What We're Watching | ||
|
|
||
| A brand-new customer org has no score history to calibrate a percentile |
There was a problem hiding this comment.
Severity: warning
The 'conservative fixed cutoff' for new organizations lacks specifics. Please define its value and whether it is configurable or dynamically set.
Suggestions:
- Specify the default value of the 'conservative fixed cutoff' and clarify if it's a global constant, configurable, or derived from a baseline.
| specifically so that no single signal is ever the only thing standing | ||
| between a risky change and production. | ||
|
|
||
| One capability was scoped and deliberately set aside for a later phase: a |
There was a problem hiding this comment.
Severity: info
The observe-only mode is critical for building trust and adoption. Consider prioritizing its implementation earlier for new customers.
Suggestions:
- Re-evaluate the priority of the 'observe-only mode' feature, as it can significantly reduce friction and build confidence for new customer onboarding.
| require_llm_confidence INTEGER NOT NULL DEFAULT 8, -- out of 10, mirrors RADAR's ACR gate | ||
| blocking_ruleset_ids BIGINT[] NOT NULL DEFAULT '{}', -- reuses ci_rulesets.id | ||
| landing_delay_seconds INTEGER NOT NULL DEFAULT 0, -- post-approve human-rejection window | ||
| daily_approve_cap INTEGER, -- NULL = unlimited |
There was a problem hiding this comment.
Severity: warning
For daily_approve_cap, the scope (organization/repository) and time window (rolling 24 hours/calendar day) are missing.
Suggestions:
- Specify if the cap is per-org or per-repo.
- Clarify the time window for the daily cap (e.g., rolling 24 hours, UTC calendar day).
| they're part of that stage's design, not generic schema.) | ||
|
|
||
| **`review_outcomes`** (new table) for the feedback loop: `review_id, org_id, | ||
| reported_by_user_id, outcome_type ('revert'|'incident'|'confirmed_safe'), |
There was a problem hiding this comment.
Severity: warning
For review_outcomes.source set to 'ci', please clarify the security and authentication mechanisms for automated CI reporting.
Suggestions:
- Detail how CI systems will authenticate and authorize to report outcomes, especially if
restrict_outcome_reporting_to_adminsis true.
Summary
Adds risk-based auto-approval for pull requests, plus "Start a review" links in every new PR's description, across GitHub, GitLab, Gitea, Bitbucket and Azure DevOps.
PR review links
Auto-approve policy engine
Review page
Other
review_commits.repository_idis nowON DELETE SET NULL).Database
Three additive migrations, applied automatically by
dbmate upon start:20261012090000:pull_requests.links_comment_posted_at20261012100000:review_commitsforeign key →ON DELETE SET NULL20261013090000: risk policy engine tables (auto_approve_policies,review_policy_log,review_policy_overrides,review_outcomes,risk_*,user_identities),users.review_level(default 1),pull_requests.author_emailNo new environment variables, Docker dependencies or deploy-script changes.
Linked Issue
Validation
npx tsc --noEmitpasses.make security-govulncheck: no vulnerabilities in called code.make security-osv: no vulnerabilities (after the existing ignore list).Checklist
SECURITY.mdand updated related documentation if needed.Notes For Reviewers
livereview_prod_urlset; without it the section is skipped with a warning.write:pullrequestto edit descriptions and post verdicts. Without it, those calls are logged and skipped.docs/risk-policy-engine.md,docs/risk-policy-engine-six-pager.md,docs/pr-review-links-plan.md.