Skip to content

Clarify auto-approve risk message and policy comment, add CI test workflow - #136

Merged
LinceMathew merged 3 commits into
masterfrom
auto-approve-message-and-ci-tests
Oct 18, 2026
Merged

LinceMathew merged 3 commits into
masterfrom
auto-approve-message-and-ci-tests

Conversation

@LinceMathew

@LinceMathew LinceMathew commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes two confusing parts of the auto-approve output found while testing, adds tests for policy edge cases, and adds a manual CI workflow for the Go test suite.

Risk stage message

  • Before: Risk score 1.00 (P0) is above the threshold of 0.50 (P20). With no score history, the P0 is a placeholder and the 0.50 is a fixed fallback, not the P20 value, so it read as "P0 is above P20".
  • Now: P numbers show only when there's score history to rank against.
    • No history: Risk score 1.00 is above the cutoff of 0.50
    • With history: Risk score 0.90 (P90) is above the cutoff of 0.20 (P20)
  • The message is built in a small riskReason function. Decision logic is unchanged.

Policy PR comment

  • The verdict was printed twice ("LiveReview policy: Not approved…" and "Policy: Not approved · Review #…"). It's now one line with the review link, followed by the stage table.

Tests

  • risk_reason_test.go: exact messages for no, minimal and full history, plus a check that a blocked review's P number is never below the policy's P (and a passing one never at or above it).
  • daily_cap_test.go: no cap means unlimited; a failed count query skips the cap (pins the current fail-open behaviour).
  • 3 new scenarios.json cases: bot passes at min level 0, required LLM confidence 0, security-sensitive with low confidence reports the confidence.

CI

  • New livereview-tests workflow, run manually (workflow_dispatch). Each run gets its own throwaway Postgres 18 container, runs dbmate up on it, then make testall, and reports a livereview-tests status on the commit. It never touches staging, so unmerged migrations, parallel runs and leftover test rows can't affect it.
  • make testall skips 28 tests that already fail on master and 2 packages that don't build (scripts, cmd/onboarding-pdf), listed in TESTALL_SKIP_TESTS / TESTALL_SKIP_PKGS. Remove a name once its test is fixed.
  • 7 internal/api DB tests now use DATABASE_URL (via testdb_test.go) instead of a hardcoded local connection, and the prorated charge test passes cents (6400) as the function expects. These 8 tests now pass and are no longer skipped.
  • TestServeChatCSV and TestCIGateGeneratedScriptE2E use a unique email per run, and the CI gate test cleans up child rows before the user and org, so rows left by an earlier run can't cause duplicate-key failures.
  • The gemini test no longer sets the Temperature field, which was removed from gemini.TestableFields.

Linked Issue

None. Found while testing auto-approve for the demo.

Validation

  • go test ./internal/policyengine/: TestRiskReason*, TestDailyCap* and all TestScenarios cases pass, including the 3 new ones.
  • go test ./internal/api/ -run '…': the 8 fixed api tests pass against a local DB.
  • Full make testall run locally to collect the known failing tests for the skip list.
  • gofmt and go vet clean on changed packages.

Checklist

  • This PR fulfills an agreed issue.
  • I kept the change narrow and scoped.
  • I ran the most specific relevant validation and described it above.
  • If I changed behavior, I called that out clearly in this PR.
  • If I touched UI, I attached a GIF or video walkthrough. This is required.
  • If this change touches security, disclosure flow, credentials, storage, network behavior, or licensing/entitlement enforcement, I reviewed SECURITY.md and updated related documentation if needed.
  • I have read and accept the Contributor License Agreement.

Notes For Reviewers

  • The workflow only appears in the Actions tab after this is merged to master.
  • TestResolvePolicy* needs the default-policy migration (20261017100000); the workflow runs dbmate up first. Locally, run dbmate up before make testall.
  • Tests run on an empty database. Any test that relied on existing data would show up on the first run and can be added to the skip list.
  • license/payment tests print Razorpay keys and create a real plan. They're in the skip list and untouched here.

🔍 LiveReview

⚙️ Customize risk-based approval policy for this repository or your organization.

…kflow

- Risk stage message shows P numbers only when there is score history
- Policy PR comment has a single verdict header with the review link
- Tests for the risk message, daily cap, bot min level 0 and LLM edge cases
- Manual livereview-tests workflow running make testall on staging DB
- Skip known failing tests in testall; api DB tests use DATABASE_URL

LiveReview Pre-Commit Check: vouched (iter:1, coverage:0%)
@LinceMathew
LinceMathew force-pushed the auto-approve-message-and-ci-tests branch from ea1d494 to 1fa76ef Compare October 18, 2026 09:29
@LiveReview-Bot

Copy link
Copy Markdown

Add Staging Test Workflow and Stabilize Test Suites

Overview

Adds a GitHub Actions workflow to run database migrations and test suites against a staging PostgreSQL instance. Skips 28 known failing tests and updates test helpers to unblock pipeline execution. Refactors policy engine risk reasons and review link formatting for missing history edge cases.

Technical Highlights

  • .github/workflows/livereview-tests.yml: Adds a manual workflow running dbmate migrations and make testall against staging PostgreSQL.
  • Makefile: Defines TESTALL_SKIP_TESTS regex and TESTALL_SKIP_PKGS to bypass 28 failing tests and standalone packages.
  • internal/policyengine/engine.go: Adds riskReason helper to format risk verdicts cleanly when past score history is empty.
  • internal/jobqueue/pr_policy_worker.go: Extracts policyReviewLink helper to separate review URLs from the markdown table.
  • internal/api/testdb_test.go: Introduces a unified database resolver reading DATABASE_URL with a local fallback.

Impact

  • Functionality: Enables automated schema migration and test validation against real staging PostgreSQL environments.
  • Risk: Concurrent workflow runs lack isolation and can cause data collisions on the shared staging database.

statuses: write

jobs:
livereview-tests:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity: warning

There is no concurrency group defined for this workflow. Consequently, parallel workflow runs can collide on the shared staging database.

Suggestions:

  1. Add concurrency group with cancel-in-progress: false to serialize runs against DATABASE_URL

- chat files and CI gate tests use a unique email per run
- CI gate test cleans up child rows before the user and org
- gemini test no longer sets the removed Temperature field

LiveReview Pre-Commit Check: vouched (iter:1, coverage:0%)
@LiveReview-Bot

Copy link
Copy Markdown

Add Live CI Test Workflow and Stabilize Integration Tests

Overview

This change adds an on-demand GitHub Actions workflow to run the test suite against a live PostgreSQL database. It fixes test isolation, teardown sequencing, and shared database URL helpers across API integration tests. It also updates policy engine risk string formatting and skips known failing tests in the Makefile.

Technical Highlights

  • .github/workflows/livereview-tests.yml: Introduces a manual GitHub Actions workflow running schema migrations and make testall against PostgreSQL with commit status reporting.
  • Makefile: Skips 28 staging-failing tests and excludes ./scripts and ./cmd/onboarding-pdf to allow CI test suites to pass.
  • internal/api/testdb_test.go: Centralizes database URL resolution via testDatabaseURL() with DATABASE_URL environment variable support and local defaults.
  • internal/api/ci_gate_script_e2e_test.go & chat_files_test.go: Enforces strict parent-child teardown sequencing and generates timestamped unique user emails to eliminate test pollution.
  • internal/policyengine/engine.go: Omits percentile tags (P%d) in risk reason strings when historical score data is absent.
  • internal/jobqueue/pr_policy_worker.go: Adds direct review links via policyReviewLink and simplifies markdown table rendering for stage details.

Impact

  • Functionality: Developers can run integration tests against live PostgreSQL in CI, and PR policy comments now link directly to reviews.
  • Risk: Skipping 28 tests in make testall risks masking real regressions in staging environments.

Comment thread .github/workflows/livereview-tests.yml Outdated

# The DB tests skip when the database is unreachable, so fail here instead of passing silently.
- name: Check staging database is reachable
run: psql "$DATABASE_URL" -c 'select 1' > /dev/null

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity: warning

The psql client is not preinstalled on the ubuntu-24.04 runner, which causes step 24 to fail.

Suggestions:

  1. Install postgresql-client prior to running psql (sudo apt-get install -y postgresql-client).
  2. Alternatively, verify connectivity using a small Go script or dbmate.

Comment thread .github/workflows/livereview-tests.yml Outdated
run: sudo npm install -g dbmate

- name: Run staging migrations
run: dbmate --url "$DATABASE_URL" up

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity: warning

Running dbmate up and executing tests against a shared staging database creates a risk of schema corruption and test run collisions.

Suggestions:

  1. Use an isolated PostgreSQL service container in the GitHub Actions job (services: postgres: ...).
  2. If remote DB is required, create an ephemeral schema or dedicated test database per CI run.

- Postgres 18 service container per run, so migrations and tests never touch staging
- Drop the psql reachability step; dbmate up fails the job if the DB is down
- Skip schema dump in CI to avoid a pg_dump version mismatch

LiveReview Pre-Commit Check: vouched (iter:1, coverage:0%)
@LinceMathew
LinceMathew merged commit 894d955 into master Oct 18, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants