Add specs, runbook, and env template for GitHub PR reviews

index eb1c6d1..d843148 100644 --- a/.env.example +++ b/.env.example @@ -56,6 +56,29 @@ STRIPE_SECRET_KEY=sk_test_your-stripe-secret-key STRIPE_WEBHOOK_SECRET=whsec_your-webhook-signing-secret +# ----------------------------------------------------------------------------- +# GitHub App — siGit Code automatic PR reviews (the hosted bot). Register the +# App and find these values per docs/github-app-reviews.md. In production, +# prefer Rails credentials (github_app.*) — the private key is multi-line. +# POST /github/webhooks uses these. +# ----------------------------------------------------------------------------- + +GITHUB_APP_ID=123456 + +# Webhook signing secret set on the GitHub App. +GITHUB_APP_WEBHOOK_SECRET=your-github-webhook-secret + +# The App's private key PEM with literal \n escapes in place of newlines. +GITHUB_APP_PRIVATE_KEY="-----BEGIN RSA PRIVATE KEY-----\n...\n-----END RSA PRIVATE KEY-----" + +# Kill switch: set to "false" to stop automatic reviews (new webhooks AND the +# queued backlog). Reviews are on by default when the App is configured. +# SIGIT_GITHUB_REVIEWS_ENABLED=true + +# Onde tier used for reviews; defaults to onde-large. +# SIGIT_GITHUB_REVIEWS_MODEL=onde-large + + # ----------------------------------------------------------------------------- # Database (PostgreSQL) # The defaults below work with a local Postgres.app installation.

new file mode 100644 index 0000000..94f86e6 --- /dev/null +++ b/docs/github-app-reviews.md @@ -0,0 +1,108 @@ +# siGit Code GitHub App — automatic PR reviews + +The hosted GitHub App behind siGit Code's pull-request reviews. Users install +the App on their github.com repos; every reviewable pull-request event hits +`POST /github/webhooks`, and `GithubPrReviewJob` posts a walkthrough summary +plus line-level comments on the PR, powered by Onde Cloud inference. + +## How it fits together + +``` +github.com ──webhook──▶ GithubWebhooksController ──enqueue──▶ GithubPrReviewJob (Solid Queue) + │ verifies X-Hub-Signature-256 │ fetch PR + diff (GithubAppService) + │ syncs github_app_installations │ budget/annotate (GithubReviewPrompt) + └ <10s, never does slow work │ inference (OndeCloudService) + │ validate lines (GithubPatchIndex) + └ POST one review back to GitHub +``` + +State lives in two tables: `github_app_installations` (mirror of App +installations, plus the cached 1-hour installation token) and +`github_pr_reviews` (one row per repo + PR + head SHA — the idempotency +guard, and the audit trail: status, skip reason, error, comment count). + +## Registering the GitHub App (one-time) + +1. GitHub → Settings → Developer settings → **GitHub Apps** → New GitHub App + (register under the org that owns the product). +2. Basics: + - Name: **siGit Code** + - Homepage URL: `https://sigit.si/code` + - Webhook URL: `https://sigit.si/github/webhooks` + - Webhook secret: generate one (`bin/rails secret | head -c 48`) and save it + for step 4. +3. Permissions and events: + - Repository permissions: **Pull requests: Read and write**, + **Contents: Read-only**, **Metadata: Read-only** (mandatory). + - Subscribe to events: **Pull request**. (`installation` and + `installation_repositories` events are always delivered to App webhooks; + there is no checkbox for them.) + - Where can this App be installed: **Any account**. +4. After creating: note the **App ID**, then **Generate a private key** + (downloads a `.pem`). + +## Configuration + +Preferred in production: Rails credentials (`bin/rails credentials:edit`), +because the private key is multi-line and prod env vars live in the `git` +user's `~/.profile`, which is single-line only: + +```yaml +github_app: + app_id: "123456" + webhook_secret: "..." + private_key: | + -----BEGIN RSA PRIVATE KEY----- + ... + -----END RSA PRIVATE KEY----- +``` + +ENV fallback (dev, or single-line values): `GITHUB_APP_ID`, +`GITHUB_APP_WEBHOOK_SECRET`, and `GITHUB_APP_PRIVATE_KEY` with literal `\n` +escapes in place of newlines. See `.env.example`. + +Switches: + +- `SIGIT_GITHUB_REVIEWS_ENABLED=false` — the kill switch. Stops new enqueues + *and* drains already-queued jobs as no-ops (the job re-checks). Reviews are + on by default whenever the App credentials are configured. +- `SIGIT_GITHUB_REVIEWS_MODEL` — Onde tier for reviews (default `onde-large`). + +## Production rollout + +The deploy is the usual `git push` to the bare-repo hook (see +`.agents/skills/deployment/SKILL.md`), but this feature is the first user of +Solid Queue, so on the first deploy: + +1. Verify the queue schema loaded: the hook's `db:prepare` should load + `db/queue_schema.rb` into `sigitsi_production_queue` (that DB exists but + was empty — a known issue in the deployment skill). Check with + `psql sigitsi_production_queue -c '\dt'` — expect `solid_queue_*` tables. +2. **Amend the post-receive hook before relying on `bin/jobs start`**: the + hook starts a jobs supervisor on every deploy but never stops the previous + one, so supervisors stack. Add something like + `pkill -f 'solid_queue' || true` before the `bin/jobs start` line. +3. Add the App credentials (step above), restart Puma. +4. Install the App on a test repo, open a PR, and watch: + - the jobs log (`output-jobs.log`) for the review run, + - the `github_pr_reviews` row (`status`, `skip_reason`, `error_message`), + - the PR itself for the summary + line comments. + +## Operational notes + +- **GitHub never retries webhook deliveries** (10-second timeout, fire-once). + A missed delivery is recovered by the next push (`synchronize`) or manually: + App settings → Advanced → Recent Deliveries → Redeliver (kept 30 days). +- **Skipped, not failed**: drafts, closed/superseded PRs, PRs over the size + budget (> `GithubReviewPrompt::MAX_FILES` files or nothing reviewable) are + recorded as `skipped` with a `skip_reason`; over-budget PRs get a short + explanatory comment instead of a review. +- **422 from GitHub on review creation** means a comment targeted a line + outside the diff. The job degrades automatically: full review → + summary-only review → plain issue comment. If these show up in the logs, + look at `GithubPatchIndex` first. +- **One review per head SHA**: redelivered webhooks and rapid pushes collapse + via the unique `(repo_full_name, pr_number, head_sha)` index; a newer push + supersedes queued reviews of older SHAs. +- Reviews are **ungated** in v1 — any installation gets them. Billing or + account linking is future work.

new file mode 100644 index 0000000..017dfa3 --- /dev/null +++ b/spec/jobs/github_pr_review_job_spec.rb @@ -0,0 +1,156 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe GithubPrReviewJob do + let(:installation) do + GithubAppInstallation.create!(installation_id: 555, account_login: "octocat") + end + + let(:pull_request) do + { "number" => 7, "state" => "open", "draft" => false, + "title" => "Fix login", "body" => "Corrects the token check.", + "base" => { "ref" => "main" }, "head" => { "ref" => "fix-login", "sha" => "headsha1" } } + end + + let(:files) do + [ { "filename" => "app/a.rb", "status" => "modified", "additions" => 1, "deletions" => 0, + "patch" => "@@ -1,2 +1,3 @@\n line one\n+line two\n line three" } ] + end + + let(:model_reply) do + { "summary" => "Tightens the token check.", + "findings" => [ + { "path" => "app/a.rb", "line" => 2, "severity" => "warning", "comment" => "Handle nil token." }, + { "path" => "app/a.rb", "line" => 90, "severity" => "nit", "comment" => "Off the diff." } + ] }.to_json + end + + def onde_response(content) = { "choices" => [ { "message" => { "content" => content } } ] } + + def perform + described_class.perform_now( + installation_id: installation.id, repo_full_name: "octocat/hello", + pr_number: 7, head_sha: "headsha1" + ) + end + + before do + allow(GithubReviewConfig).to receive_messages(enabled?: true, model: "onde-large") + allow(GithubAppService).to receive_messages( + pull_request: pull_request, pull_request_files: files, + create_review: {}, create_issue_comment: {} + ) + allow(OndeCloudService).to receive(:create).and_return(onde_response(model_reply)) + end + + it "posts one review with the summary and only diff-valid comments, then completes" do + perform + + expect(GithubAppService).to have_received(:create_review) do |_inst, repo, number, commit_id:, body:, comments:| + expect([ repo, number, commit_id ]).to eq([ "octocat/hello", 7, "headsha1" ]) + expect(body).to include("Tightens the token check.").and include("siGit Code") + expect(comments).to eq([ { "path" => "app/a.rb", "line" => 2, "side" => "RIGHT", + "body" => "**warning** — Handle nil token." } ]) + end + + review = GithubPrReview.find_by!(repo_full_name: "octocat/hello", pr_number: 7, head_sha: "headsha1") + expect(review).to have_attributes(status: "completed", comment_count: 1, model: "onde-large") + end + + it "is idempotent: a second run for the same head SHA does nothing" do + perform + perform + + expect(GithubAppService).to have_received(:create_review).once + end + + it "no-ops when the kill switch is off, leaving the event reviewable later" do + allow(GithubReviewConfig).to receive(:enabled?).and_return(false) + perform + + expect(OndeCloudService).not_to have_received(:create) + expect(GithubPrReview.count).to eq(0) + end + + it "skips when a newer push superseded this SHA" do + pull_request["head"]["sha"] = "newersha" + perform + + expect(OndeCloudService).not_to have_received(:create) + expect(GithubPrReview.last).to have_attributes(status: "skipped", skip_reason: "superseded") + end + + it "skips a PR that turned draft after enqueue" do + pull_request["draft"] = true + perform + + expect(GithubPrReview.last).to have_attributes(status: "skipped", skip_reason: "draft") + expect(OndeCloudService).not_to have_received(:create) + end + + it "skips a PR that vanished (404)" do + allow(GithubAppService).to receive(:pull_request) + .and_raise(GithubAppService::ApiError.new("gone", status: 404)) + perform + + expect(GithubPrReview.last).to have_attributes(status: "skipped", skip_reason: "pr_gone") + end + + it "posts a note instead of a review when the PR is over budget" do + too_many = (0..GithubReviewPrompt::MAX_FILES).map do |i| + { "filename" => "f#{i}.rb", "patch" => "@@ -1,1 +1,2 @@\n a\n+b" } + end + allow(GithubAppService).to receive(:pull_request_files).and_return(too_many) + perform + + expect(GithubAppService).to have_received(:create_issue_comment) + expect(GithubAppService).not_to have_received(:create_review) + expect(OndeCloudService).not_to have_received(:create) + expect(GithubPrReview.last).to have_attributes(status: "skipped", skip_reason: "too_large") + end + + it "degrades to summary-only, then an issue comment, when GitHub 422s the review" do + allow(GithubAppService).to receive(:create_review) + .and_raise(GithubAppService::ApiError.new("invalid", status: 422, body: "{}")) + perform + + expect(GithubAppService).to have_received(:create_review).twice # full, then summary-only + expect(GithubAppService).to have_received(:create_issue_comment) + expect(GithubPrReview.last).to have_attributes(status: "completed") + end + + it "retries once with a fresh token on 401" do + calls = 0 + allow(GithubAppService).to receive(:pull_request) do + calls += 1 + raise GithubAppService::ApiError.new("expired", status: 401) if calls == 1 + + pull_request + end + installation.update!(access_token: "ghs_stale", access_token_expires_at: 1.hour.from_now) + + perform + + expect(installation.reload.access_token).to be_nil + expect(GithubPrReview.last).to have_attributes(status: "completed") + end + + it "marks the review failed and posts nothing when the model reply is unparseable" do + allow(OndeCloudService).to receive(:create).and_return(onde_response("not json at all")) + perform + + expect(GithubAppService).not_to have_received(:create_review) + expect(GithubAppService).not_to have_received(:create_issue_comment) + expect(GithubPrReview.last.status).to eq("failed") + end + + it "releases the claim and schedules a retry on a transient inference failure" do + ActiveJob::Base.queue_adapter = :test + allow(OndeCloudService).to receive(:create) + .and_raise(OndeCloudService::UpstreamError.new("bad gateway", status: 502)) + + expect { perform }.to have_enqueued_job(described_class) # retry_on re-enqueues + expect(GithubPrReview.last.status).to eq("pending") # claim released for the retry + end +end

index ef75d46..0903023 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -10,6 +10,10 @@ abort("The Rails environment is running in production mode!") if Rails.env.produ require 'rspec/rails' # Add additional requires below this line. Rails is not loaded until this point! +# No spec may hit the network; GitHub REST calls are stubbed with webmock. +require 'webmock/rspec' +WebMock.disable_net_connect!(allow_localhost: true) + # Requires supporting ruby files with custom matchers and macros, etc, in # spec/support/ and its subdirectories. Files matching `spec/**/*_spec.rb` are # run as spec files by default. This means that files in spec/support that end

new file mode 100644 index 0000000..3cfd868 --- /dev/null +++ b/spec/requests/github_webhooks_spec.rb @@ -0,0 +1,205 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe "GitHub webhooks", type: :request do + before do + ActiveJob::Base.queue_adapter = :test + allow(GithubAppService).to receive_messages(configured?: true, webhook_secret: "hook-secret") + end + + def deliver(event, payload, secret: "hook-secret") + body = payload.to_json + post "/github/webhooks", params: body, headers: { + "CONTENT_TYPE" => "application/json", + "X-GitHub-Event" => event, + "X-Hub-Signature-256" => "sha256=#{OpenSSL::HMAC.hexdigest("SHA256", secret, body)}" + } + end + + def installation_payload(action, id: 555, selection: "selected") + { action: action, + installation: { id: id, account: { login: "octocat", type: "User", id: 9 }, + repository_selection: selection } } + end + + def pull_request_payload(action, draft: false, number: 7, sha: "headsha1") + installation_payload(action).merge( + repository: { full_name: "octocat/hello" }, + pull_request: { number: number, draft: draft, head: { sha: sha } } + ) + end + + describe "authentication" do + it "rejects a tampered body" do + body = installation_payload("created").to_json + post "/github/webhooks", params: body + " ", headers: { + "CONTENT_TYPE" => "application/json", + "X-GitHub-Event" => "installation", + "X-Hub-Signature-256" => "sha256=#{OpenSSL::HMAC.hexdigest("SHA256", "hook-secret", body)}" + } + + expect(response).to have_http_status(:unauthorized) + expect(GithubAppInstallation.count).to eq(0) + end + + it "rejects a signature under the wrong secret" do + deliver("installation", installation_payload("created"), secret: "wrong") + expect(response).to have_http_status(:unauthorized) + end + + it "returns 503 when the app is not configured" do + allow(GithubAppService).to receive(:configured?).and_return(false) + deliver("installation", installation_payload("created")) + expect(response).to have_http_status(:service_unavailable) + end + end + + describe "installation events" do + it "upserts the installation on created" do + deliver("installation", installation_payload("created")) + + expect(response).to have_http_status(:ok) + installation = GithubAppInstallation.find_by!(installation_id: 555) + expect(installation).to have_attributes( + account_login: "octocat", account_type: "User", account_id: 9, + repository_selection: "selected", deleted_at: nil + ) + end + + it "soft-deletes and clears the cached token on deleted" do + installation = GithubAppInstallation.create!( + installation_id: 555, account_login: "octocat", + access_token: "ghs_x", access_token_expires_at: 1.hour.from_now + ) + + deliver("installation", installation_payload("deleted")) + + expect(installation.reload.deleted_at).to be_present + expect(installation.access_token).to be_nil + end + + it "suspends and unsuspends" do + deliver("installation", installation_payload("suspend")) + installation = GithubAppInstallation.find_by!(installation_id: 555) + expect(installation.suspended_at).to be_present + + deliver("installation", installation_payload("unsuspend")) + expect(installation.reload.suspended_at).to be_nil + end + + it "refreshes repository_selection on installation_repositories" do + GithubAppInstallation.create!(installation_id: 555, account_login: "octocat", + repository_selection: "selected") + deliver("installation_repositories", installation_payload("added", selection: "all")) + + expect(GithubAppInstallation.find_by!(installation_id: 555).repository_selection).to eq("all") + end + end + + describe "pull_request events" do + it "enqueues a review for an opened PR (creating the installation if unseen)" do + expect do + deliver("pull_request", pull_request_payload("opened")) + end.to have_enqueued_job(GithubPrReviewJob).with( + installation_id: GithubAppInstallation.last&.id || kind_of(Integer), + repo_full_name: "octocat/hello", pr_number: 7, head_sha: "headsha1" + ).on_queue("default") + + expect(response).to have_http_status(:ok) + expect(GithubAppInstallation.find_by(installation_id: 555)).to be_present + end + + it "enqueues for synchronize, reopened, and ready_for_review" do + %w[synchronize reopened ready_for_review].each do |action| + expect { deliver("pull_request", pull_request_payload(action)) } + .to have_enqueued_job(GithubPrReviewJob) + end + end + + it "ignores other PR actions" do + expect { deliver("pull_request", pull_request_payload("labeled")) } + .not_to have_enqueued_job + expect(response).to have_http_status(:ok) + end + + it "skips draft PRs" do + expect { deliver("pull_request", pull_request_payload("opened", draft: true)) } + .not_to have_enqueued_job + end + + it "skips when the kill switch is off" do + allow(GithubReviewConfig).to receive(:enabled?).and_return(false) + expect { deliver("pull_request", pull_request_payload("opened")) } + .not_to have_enqueued_job + end + + it "skips suspended installations" do + GithubAppInstallation.create!(installation_id: 555, account_login: "octocat", + suspended_at: Time.current) + expect { deliver("pull_request", pull_request_payload("opened")) } + .not_to have_enqueued_job + end + end + + describe "end to end: signed webhook through job to posted review" do + include ActiveJob::TestHelper + + it "reviews the PR and completes the review row" do + allow(GithubAppService).to receive_messages( + pull_request: { + "number" => 7, "state" => "open", "draft" => false, "title" => "Fix login", + "body" => "", "base" => { "ref" => "main" }, + "head" => { "ref" => "fix", "sha" => "headsha1" } + }, + pull_request_files: [ + { "filename" => "app/a.rb", "status" => "modified", "additions" => 1, + "deletions" => 0, "patch" => "@@ -1,2 +1,3 @@\n one\n+two\n three" } + ], + create_review: {} + ) + allow(GithubReviewConfig).to receive_messages(enabled?: true, model: "onde-large") + allow(OndeCloudService).to receive(:create).and_return( + "choices" => [ { "message" => { "content" => { + "summary" => "Adds a line.", + "findings" => [ { "path" => "app/a.rb", "line" => 2, "severity" => "nit", "comment" => "Name it." } ] + }.to_json } } ] + ) + + perform_enqueued_jobs do + deliver("pull_request", pull_request_payload("opened")) + end + + expect(GithubAppService).to have_received(:create_review) do |_inst, repo, number, commit_id:, comments:, **| + expect([ repo, number, commit_id ]).to eq([ "octocat/hello", 7, "headsha1" ]) + expect(comments.first).to include("path" => "app/a.rb", "line" => 2, "side" => "RIGHT") + end + expect(GithubPrReview.last).to have_attributes( + status: "completed", repo_full_name: "octocat/hello", head_sha: "headsha1", comment_count: 1 + ) + end + end + + describe "resilience" do + it "acks unknown events" do + deliver("star", { action: "created" }) + expect(response).to have_http_status(:ok) + end + + it "returns 400 for unparseable JSON" do + body = "{not json" + post "/github/webhooks", params: body, headers: { + "CONTENT_TYPE" => "application/json", + "X-GitHub-Event" => "installation", + "X-Hub-Signature-256" => "sha256=#{OpenSSL::HMAC.hexdigest("SHA256", "hook-secret", body)}" + } + expect(response).to have_http_status(:bad_request) + end + + it "still acks when handling raises, so GitHub doesn't mark the endpoint dead" do + allow(GithubAppInstallation).to receive(:find_or_initialize_by).and_raise("boom") + deliver("installation", installation_payload("created")) + expect(response).to have_http_status(:ok) + end + end +end

new file mode 100644 index 0000000..db3bbce --- /dev/null +++ b/spec/services/github_app_service_spec.rb @@ -0,0 +1,137 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe GithubAppService do + let(:rsa_key) { OpenSSL::PKey::RSA.new(2048) } + + before do + allow(described_class).to receive_messages( + app_id: "12345", + webhook_secret: "hook-secret", + private_key_pem: rsa_key.to_pem + ) + end + + let(:installation) do + GithubAppInstallation.create!(installation_id: 555, account_login: "octocat") + end + + def token_response(token: "ghs_fresh", expires_in: 1.hour) + { status: 201, + body: { token: token, expires_at: expires_in.from_now.utc.iso8601 }.to_json, + headers: { "Content-Type" => "application/json" } } + end + + describe ".app_jwt" do + it "mints an RS256 JWT with the app id and a <10 minute lifetime" do + payload, header = JWT.decode(described_class.send(:app_jwt), rsa_key.public_key, true, + algorithm: "RS256") + + expect(header["alg"]).to eq("RS256") + expect(payload["iss"]).to eq("12345") + expect(payload["iat"]).to be < Time.current.to_i # back-dated for clock skew + expect(payload["exp"] - Time.current.to_i).to be_between(1, 600) + end + end + + describe ".installation_token" do + it "mints and caches a token on the installation row" do + mint = stub_request(:post, "https://api.github.com/app/installations/555/access_tokens") + .to_return(token_response) + + expect(described_class.installation_token(installation)).to eq("ghs_fresh") + expect(installation.reload.access_token).to eq("ghs_fresh") + expect(installation.access_token_expires_at).to be_within(2.minutes).of(1.hour.from_now) + + expect(described_class.installation_token(installation)).to eq("ghs_fresh") + expect(mint).to have_been_requested.once # second call served from the row + end + + it "re-mints when the cached token is near expiry" do + installation.update!(access_token: "ghs_stale", access_token_expires_at: 2.minutes.from_now) + stub_request(:post, "https://api.github.com/app/installations/555/access_tokens") + .to_return(token_response(token: "ghs_new")) + + expect(described_class.installation_token(installation)).to eq("ghs_new") + end + end + + describe ".verify_webhook_signature?" do + let(:payload) { '{"zen":"Design for failure."}' } + let(:signature) { "sha256=#{OpenSSL::HMAC.hexdigest("SHA256", "hook-secret", payload)}" } + + it "accepts the correct HMAC" do + expect(described_class.verify_webhook_signature?(payload, signature)).to be true + end + + it "rejects a tampered payload, a missing header, and a missing secret" do + expect(described_class.verify_webhook_signature?(payload + " ", signature)).to be false + expect(described_class.verify_webhook_signature?(payload, nil)).to be false + + allow(described_class).to receive(:webhook_secret).and_return(nil) + expect(described_class.verify_webhook_signature?(payload, signature)).to be false + end + end + + describe "REST calls" do + before do + installation.update!(access_token: "ghs_live", access_token_expires_at: 1.hour.from_now) + end + + it "fetches a pull request with App headers" do + stub_request(:get, "https://api.github.com/repos/octocat/hello/pulls/7") + .with(headers: { "Authorization" => "Bearer ghs_live", + "X-GitHub-Api-Version" => "2022-11-28" }) + .to_return(status: 200, body: { "number" => 7 }.to_json) + + expect(described_class.pull_request(installation, "octocat/hello", 7)).to eq("number" => 7) + end + + it "follows Link pagination when listing files" do + page_two = "https://api.github.com/repos/octocat/hello/pulls/7/files?per_page=100&page=2" + stub_request(:get, "https://api.github.com/repos/octocat/hello/pulls/7/files?per_page=100") + .to_return(status: 200, body: [ { "filename" => "a.rb" } ].to_json, + headers: { "Link" => "<#{page_two}>; rel=\"next\"" }) + stub_request(:get, page_two) + .to_return(status: 200, body: [ { "filename" => "b.rb" } ].to_json) + + files = described_class.pull_request_files(installation, "octocat/hello", 7) + expect(files.map { |f| f["filename"] }).to eq(%w[a.rb b.rb]) + end + + it "creates a review with the summary body and line comments" do + create = stub_request(:post, "https://api.github.com/repos/octocat/hello/pulls/7/reviews") + .with(body: hash_including( + "commit_id" => "abc123", "event" => "COMMENT", "body" => "Walkthrough", + "comments" => [ { "path" => "a.rb", "line" => 2, "side" => "RIGHT", "body" => "hm" } ] + )) + .to_return(status: 200, body: "{}") + + described_class.create_review(installation, "octocat/hello", 7, + commit_id: "abc123", body: "Walkthrough", + comments: [ { "path" => "a.rb", "line" => 2, "side" => "RIGHT", "body" => "hm" } ]) + expect(create).to have_been_requested + end + + it "raises ApiError carrying status and body on 422" do + stub_request(:post, "https://api.github.com/repos/octocat/hello/pulls/7/reviews") + .to_return(status: 422, body: '{"message":"Unprocessable"}') + + expect do + described_class.create_review(installation, "octocat/hello", 7, + commit_id: "abc123", body: "x") + end.to raise_error(described_class::ApiError) { |error| + expect(error.status).to eq(422) + expect(error.body).to include("Unprocessable") + } + end + + it "wraps connection failures as a 502 ApiError" do + stub_request(:get, "https://api.github.com/repos/octocat/hello/pulls/7").to_timeout + + expect { described_class.pull_request(installation, "octocat/hello", 7) } + .to raise_error(described_class::ApiError) { |error| expect(error.status).to eq(502) } + end + end +end

new file mode 100644 index 0000000..ba8a6ba --- /dev/null +++ b/spec/services/github_patch_index_spec.rb @@ -0,0 +1,96 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe GithubPatchIndex do + # a.rb: new-file lines 1 (context), 2 (added), 3 (context); line 4 removed only. + let(:single_hunk_patch) { "@@ -1,3 +1,3 @@\n line one\n+line two\n line three\n-line four" } + + let(:multi_hunk_patch) do + "@@ -1,2 +1,3 @@\n a\n+b\n c\n@@ -10,2 +11,3 @@\n x\n+y\n z" + end + + def index_for(patch, filename: "a.rb") + described_class.new([ { "filename" => filename, "patch" => patch } ]) + end + + describe "#valid_line?" do + it "accepts added and context lines by new-file number" do + index = index_for(single_hunk_patch) + expect(index.valid_line?("a.rb", 1)).to be true + expect(index.valid_line?("a.rb", 2)).to be true + expect(index.valid_line?("a.rb", 3)).to be true + end + + it "rejects lines outside the hunks" do + index = index_for(single_hunk_patch) + expect(index.valid_line?("a.rb", 4)).to be false + expect(index.valid_line?("a.rb", 100)).to be false + end + + it "rejects unknown files" do + expect(index_for(single_hunk_patch).valid_line?("other.rb", 1)).to be false + end + + it "indexes every hunk of a multi-hunk patch" do + index = index_for(multi_hunk_patch) + expect(index.valid_line?("a.rb", 2)).to be true # + in hunk 1 + expect(index.valid_line?("a.rb", 12)).to be true # + in hunk 2 + expect(index.valid_line?("a.rb", 7)).to be false # between hunks + end + + it "treats a file without a patch (binary) as having no commentable lines" do + index = described_class.new([ { "filename" => "logo.png", "patch" => nil } ]) + expect(index.valid_line?("logo.png", 1)).to be false + end + + it "handles a deleted file (only removed lines) with no commentable lines" do + index = index_for("@@ -1,2 +0,0 @@\n-gone\n-also gone", filename: "dead.rb") + expect(index.valid_line?("dead.rb", 1)).to be false + end + end + + describe "#clamp" do + let(:index) { index_for(single_hunk_patch) } + + it "returns a valid line unchanged" do + expect(index.clamp("a.rb", 2)).to eq(2) + end + + it "snaps a near miss to the nearest commentable line" do + expect(index.clamp("a.rb", 5)).to eq(3) + end + + it "drops a far miss" do + expect(index.clamp("a.rb", 50)).to be_nil + end + + it "drops findings on unknown files or non-integer lines" do + expect(index.clamp("other.rb", 2)).to be_nil + expect(index.clamp("a.rb", nil)).to be_nil + end + end + + describe ".annotate_patch" do + it "prefixes added and context lines with new-file numbers, leaves removed lines bare" do + annotated = described_class.annotate_patch(single_hunk_patch) + lines = annotated.lines.map(&:chomp) + + expect(lines[0]).to eq("@@ -1,3 +1,3 @@") + expect(lines[1]).to match(/\A\s+1 line one\z/) + expect(lines[2]).to match(/\A\s+2 \+line two\z/) + expect(lines[3]).to match(/\A\s+3 line three\z/) + expect(lines[4]).to eq("-line four") + end + + it "restarts numbering at each hunk header" do + annotated = described_class.annotate_patch(multi_hunk_patch) + expect(annotated).to include(" 11 x") + expect(annotated).to include(" 12 +y") + end + + it "returns an empty string for a missing patch" do + expect(described_class.annotate_patch(nil)).to eq("") + end + end +end

new file mode 100644 index 0000000..6d77ae6 --- /dev/null +++ b/spec/services/github_review_prompt_spec.rb @@ -0,0 +1,161 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe GithubReviewPrompt do + def file(name, patch: "@@ -1,1 +1,2 @@\n a\n+b", **extra) + { "filename" => name, "status" => "modified", "additions" => 1, "deletions" => 0, + "patch" => patch }.merge(extra) + end + + describe ".select_files" do + it "keeps normal source files and reports nothing skipped" do + selected, skipped = described_class.select_files([ file("app/a.rb") ]) + expect(selected.map { |f| f["filename"] }).to eq([ "app/a.rb" ]) + expect(skipped).to be_empty + end + + it "excludes lockfiles, vendored paths, minified assets, and db/schema.rb" do + files = [ + file("Gemfile.lock"), file("sub/dir/Cargo.lock"), file("vendor/lib.rb"), + file("node_modules/x/index.js"), file("app.min.js"), file("db/schema.rb"), + file("app/real.rb") + ] + selected, skipped = described_class.select_files(files) + + expect(selected.map { |f| f["filename"] }).to eq([ "app/real.rb" ]) + expect(skipped.map { |f, reason| [ f["filename"], reason ] }) + .to all(satisfy { |_name, reason| reason == "generated or vendored" }) + expect(skipped.size).to eq(6) + end + + it "skips binary files (no patch)" do + _, skipped = described_class.select_files([ file("logo.png", patch: nil) ]) + expect(skipped).to eq([ [ file("logo.png", patch: nil), "no text diff" ] ]) + end + + it "skips a single file over the per-file budget" do + big = file("big.rb", patch: "@@ -1,1 +1,2 @@\n a\n+#{"x" * described_class::MAX_FILE_PATCH_BYTES}") + _, skipped = described_class.select_files([ big ]) + expect(skipped.first.last).to eq("diff too large") + end + + it "drops the largest files first when the total budget is exceeded" do + chunk = "@@ -1,1 +1,2 @@\n a\n+#{"y" * 11_000}" + files = (1..9).map { |i| file("f#{i}.rb", patch: chunk) } + selected, skipped = described_class.select_files(files) + + expect(selected.sum { |f| f["patch"].bytesize }).to be <= described_class::MAX_TOTAL_PATCH_BYTES + expect(selected.size + skipped.size).to eq(9) + expect(skipped.map(&:last)).to all(eq("over the total diff budget")) + end + + it "preserves GitHub's file ordering in the selection" do + files = [ file("z.rb"), file("a.rb") ] + selected, = described_class.select_files(files) + expect(selected.map { |f| f["filename"] }).to eq([ "z.rb", "a.rb" ]) + end + end + + describe ".too_large?" do + it "is true over the file-count cap" do + files = (0..described_class::MAX_FILES).map { |i| file("f#{i}.rb") } + selected, = described_class.select_files(files) + expect(described_class.too_large?(files, selected)).to be true + end + + it "is true when nothing survives selection" do + files = [ file("Gemfile.lock") ] + expect(described_class.too_large?(files, [])).to be true + end + + it "is false for a reviewable PR" do + files = [ file("a.rb") ] + expect(described_class.too_large?(files, files)).to be false + end + end + + describe ".payload" do + let(:pull_request) do + { "title" => "Fix login", "body" => "Corrects the token check.", + "base" => { "ref" => "main" }, "head" => { "ref" => "fix-login", "sha" => "abc1234" } } + end + + it "builds a non-streaming chat payload with annotated diffs" do + payload = described_class.payload( + pull_request: pull_request, files: [ file("app/a.rb") ], + skipped: [ [ file("Gemfile.lock"), "generated or vendored" ] ], model: "onde-large" + ) + + expect(payload["model"]).to eq("onde-large") + expect(payload["stream"]).to be false + system_message, user_message = payload["messages"] + expect(system_message["content"]).to include("siGit Code") + expect(user_message["content"]).to include("Fix login") + .and include("main <- fix-login") + .and include("Gemfile.lock (generated or vendored)") + .and include(" 2 +b") # annotated new-file line number + end + end + + describe ".parse_response" do + let(:valid_json) do + '{"summary": "Adds a thing.", "findings": [{"path": "a.rb", "line": 2, "severity": "warning", "comment": "Check nil."}]}' + end + + it "parses a clean JSON reply" do + parsed = described_class.parse_response(valid_json) + expect(parsed[:summary]).to eq("Adds a thing.") + expect(parsed[:findings]).to eq([ { path: "a.rb", line: 2, severity: "warning", comment: "Check nil." } ]) + end + + it "parses a fenced reply" do + parsed = described_class.parse_response("```json\n#{valid_json}\n```") + expect(parsed[:summary]).to eq("Adds a thing.") + end + + it "parses a prose-wrapped reply" do + parsed = described_class.parse_response("Here is my review:\n#{valid_json}\nHope that helps!") + expect(parsed[:summary]).to eq("Adds a thing.") + end + + it "raises ParseError on garbage" do + expect { described_class.parse_response("no json here") } + .to raise_error(described_class::ParseError) + end + + it "raises ParseError when the summary is missing" do + expect { described_class.parse_response('{"findings": []}') } + .to raise_error(described_class::ParseError, /summary/) + end + + it "drops malformed findings, coerces string lines, defaults bad severities" do + reply = { + "summary" => "ok", + "findings" => [ + { "path" => "a.rb", "line" => "3", "severity" => "silly", "comment" => "x" }, + { "path" => "", "line" => 1, "comment" => "no path" }, + { "path" => "a.rb", "line" => 0, "comment" => "bad line" }, + { "path" => "a.rb", "line" => 1 }, + "not even a hash" + ] + }.to_json + + findings = described_class.parse_response(reply)[:findings] + expect(findings).to eq([ { path: "a.rb", line: 3, severity: "warning", comment: "x" } ]) + end + + it "caps findings at MAX_FINDINGS" do + many = (1..20).map { |i| { "path" => "a.rb", "line" => i, "comment" => "c#{i}" } } + findings = described_class.parse_response({ "summary" => "ok", "findings" => many }.to_json)[:findings] + expect(findings.size).to eq(described_class::MAX_FINDINGS) + end + end + + describe ".summary_footer" do + it "brands the review and cites the short SHA" do + footer = described_class.summary_footer("abcdef0123456789") + expect(footer).to include("siGit Code").and include("abcdef0") + end + end +end