From f9f6ab4bf01f355aa9d512de65f779a918f89728 Mon Sep 17 00:00:00 2001 From: claude Date: Fri, 21 Aug 2026 23:32:49 +0000 Subject: [PATCH] docs(plans): 2026-08-21 pragent update design - Repo opt-in (.pr-review.json:enabled) replaces AI-REVIEW/AI-USAGE labels. - Token humanization (fmt_tokens), multi-provider equivalent cost (Claude, GPT, Gemini, Grok), richer review summary (walkthrough + risk verdict + test coverage), trivial/info severity levels, per-PR merge confidence 1-5. - Defers reachability severity demotion, rules-mining from feedback, and heavy infra (sequence diagrams, T-rex, cross-repo) to future work. --- .../plans/2026-08-21-pragent-update-design.md | 411 ++++++++++++++++++ 1 file changed, 411 insertions(+) create mode 100644 docs/plans/2026-08-21-pragent-update-design.md diff --git a/docs/plans/2026-08-21-pragent-update-design.md b/docs/plans/2026-08-21-pragent-update-design.md new file mode 100644 index 0000000..13214f0 --- /dev/null +++ b/docs/plans/2026-08-21-pragent-update-design.md @@ -0,0 +1,411 @@ +# pragent — Update Design + +**Date:** 2026-08-21 +**Status:** Approved (brainstorm, 2026-08-21) +**Replaces:** none — additive + behavioral. Existing `docs/plans/2026-08-04-pragent-design.md` stays authoritative on architecture. + +## Problem + +The pilot has been live long enough to surface pain that the original design didn't +cover: + +1. **Two labels to remember.** `AI-REVIEW` (gate) + `AI-USAGE` (opt-in for the + cost block) are per-PR. Every new contributor reads the README wrong at least + once. Reviews that the team *wanted* are skipped because nobody labeled; reviews + we *don't* want still run because the label is sticky. +2. **Token numbers are unreadable.** `Total Tokens: 2071025 in / 17303 out` + requires a mental carry. The pilot already measures the tokens; the rendering + just doesn't help. +3. **Cost is anchored on one provider.** The pilot runs free (headroom/glm-5.2) + but the only equivalent-cost line is Claude Sonnet. We can't answer "what would + this have cost on GPT / Gemini / Grok?" without running the CLI on a different + model. +4. **The PR summary is operational, not useful.** A lens-fanout run posts + `Multi-lens review of repo#index (sha X). Lenses: security,perf. Findings: + critical=0 high=1 medium=2 low=1.` That tells a reviewer *how the bot worked*, + not *what they should look at*. Real products post a risk verdict, a + file-by-file walkthrough, and a test-coverage note. +5. **Triage noise is the dominant failure mode** in every competitor (CodeRabbit, + Qodo, Greptile, DoorDash). We already address most of it (severity_floor, + per-file cap, cross-lens agreement, tone-strip), but two cheap wins are left on + the table: a per-PR *merge confidence* badge, and a richer severity scale that + includes `trivial` / `info` (CodeRabbit's pattern). + +This update also distills lessons from a 30-article survey of AI code review +products (CodeRabbit, Qodo/Merge + PR-Agent, Greptile, GitHub Copilot code +review, Gemini Code Assist, qodo-ai/pr-agent, anc95/ChatGPT-CodeReview, Sourcery, +Danger, plus the security literature around the April 2026 prompt-injection +disclosures). Where we already match the state of the art, this update notes it +and moves on; where a competitor's pattern is genuinely better, it lands here. + +## Decisions + +| Question | Decision | Why | +|---|---|---| +| Trigger | `.pr-review.json:enabled` on the PR's base branch | Repo opt-in replaces labels. No per-PR manual step. Trust stays on base. | +| Default when `.pr-review.json` is absent | Disabled | Explicit opt-in. Mirrors "labels fully removed." | +| Cost model | Always render when usage data is present | Drop the `report_usage` parameter + `AI-USAGE` label + `PRAGENT_USAGE_ALWAYS` env. | +| Token rendering | `1,234,567 (1.2M)` | Python `f"{n:,}"` + short suffix only when `n ≥ 1000`. | +| Multi-provider cost | Markdown table in the collapsible usage block | Replaces the single Sonnet line. Default compare set: Sonnet, GPT-5, Gemini 2.5 Pro, Grok 4.5. | +| Summary depth | Add `walkthrough` / `risk_verdict` / `test_coverage` to the agent JSON; Python fallback for lens synthesis | Agent produces the rich text; Python derives the same three when the lens fan-out is engaged. | +| Severity scale | Extend from 4 → 6 levels: add `trivial` + `info` | Matches CodeRabbit. Backward compat (unknown → medium). | +| Merge confidence | 1–5 integer in the review header. Python-computed. | Stole the badge idea from Greptile. | +| Reachability demotion | Defer | Needs the security graph. Note in §7. | +| Rules mining from feedback | Defer | `feedback_harvest` / `feedback_analyze` exist; distillation is a separate effort. | +| Sequence diagrams / T-rex / cross-repo | Skip | Too heavy for the pilot. | + +## 1. Label removal + repo opt-in + +### `.pr-review.json` schema delta + +```diff + { ++ "enabled": true, + "focus": [...], + "exclude_paths": [...], + ... + } +``` + +`enabled` is a top-level boolean, default `false`, read from the **base branch** +(unchanged trust rule — `fetch_repo_config(ref=base_ref)` already handles this). + +### Webhook behavior (`pilot/webhook_server.py`) + +- Remove constants `AI_REVIEW_LABEL`, `AI_USAGE_LABEL`. Remove + `_labels_have_ai_review`. Remove the `report_usage` plumbing from + `_handle_pull_request` and `_run_review`. +- New helper `is_repo_enabled(api, repo, ref, token) -> bool` in `webhook_server.py` + (or reused via `fetch_repo_config` — see below). `False` on any failure + (404, parse error, missing key, malformed value). Logs the reason to stderr. +- `_handle_pull_request` order of operations: + 1. `action in SKIP_ACTIONS` → `200 ignore` + 2. base_ref present + fetch config + 3. `if not config.get("enabled")` → `200 "skip (repo not opted in)"` + 4. claim in-flight slot + 5. thread off `_run_review` +- Pre-claim gate keeps opted-out repos from consuming concurrency slots on + bursts. One extra `GET contents/.pr-review.json` per PR event (404 for + unconfigured repos) — negligible. + +### `pilot/ai_review.py` cleanup + +- Delete `AI_REVIEW_LABEL`, `AI_USAGE_LABEL` constants. +- Delete `pr_has_label()` helper (its only call sites were the AI-USAGE + re-reads at render time). +- Drop the `report_usage: bool` parameter from `review_pr()`. Always render + the collapsible usage block when `usage` is not None. +- Remove the two `PRAGENT_USAGE_ALWAYS` references (env reads). +- Extend `parse_repo_config()` to extract `enabled` (validate is bool, + default False). +- Extend `effective_config()` to preserve `enabled` through the style-defaults + merge. + +### Docs + +- `README.md`: rewrite "Label a PR `AI-REVIEW`" + "add the AI-REVIEW label" to + "commit `.pr-review.json: {"enabled": true}` to the default branch." Drop the + AI-USAGE paragraph. Update the flow diagram. +- `pilot/README-webhook.md`: replace onboarding steps. Drop the per-PR label + ceremony. +- `pilot/README.md` (CI-step path): if it still references labels, remove. + +## 2. Token humanization + +New helper in `pilot/ai_review.py`: + +``` +def fmt_tokens(n: int | None) -> str: + """1234567 -> '1,234,567 (1.2M)'; 0 -> '0'; <1000 -> comma-form; None -> '?'.""" +``` + +Rules: +- `None` → `"?"`. +- `n < 1000` → `f"{n:,}"` (no short suffix — most findings have ~tens of tokens). +- `1000 ≤ n < 1_000_000` → `f"{n:,} ({n/1000:.1f}K)"`, drop trailing `.0`. +- `1_000_000 ≤ n < 1_000_000_000` → `f"{n:,} ({n/1_000_000:.1f}M)"`. +- else `...B`. +- Negative inputs → `"?"` (defensive — never expected from usage dicts). + +Apply in: +- `pilot/ai_review._render_collapsible_usage` — input, output, reasoning, + cache_read, cache_write, total. +- `pilot/ai_review.inline_comment_body` — the `🪙 ~N tok (...)` per-finding + line. + +Tests: `test_fmt_tokens` golden vectors — `0`, `42`, `999`, `1000`, `1234`, +`1_234_567`, `1_234_567_890`, `None`, `-1`. + +## 3. Multi-provider cost in usage section + +### `pilot/cost_model.PRICES` — extend with real published rates + +Source: Anthropic platform docs, OpenAI pricing, Gemini API pricing, xAI docs. +Fetched 2026-08-21. Numbers in USD per million tokens. + +| key | input | output | cache_write | cache_read | +|---|---:|---:|---:|---:| +| `claude-opus-5` | 5.00 | 25.00 | 6.25 | 0.50 | +| `claude-sonnet-5` | 2.00 | 10.00 | 2.50 | 0.20 | +| `claude-haiku-4-5` | 1.00 | 5.00 | 1.25 | 0.10 | +| `gpt-5` | 1.25 | 10.00 | 1.25 | 0.125 | +| `gpt-5-mini` | 0.25 | 2.00 | 0.25 | 0.025 | +| `gemini-2.5-pro` | 1.875 | 12.50 | 1.875 | 0.1875 | +| `gemini-2.5-flash` | 0.30 | 2.50 | 0.30 | 0.03 | +| `grok-4.5` | 2.00 | 6.00 | 2.00 | 0.30 | +| `grok-4.3` | 1.25 | 2.50 | 1.25 | 0.20 | + +Notes on derivation: +- Gemini 2.5 Pro publishes a tiered range (`$1.25–$2.50` in, `$10–$15` out, + `$0.125–$0.25` cached). Midpoints are taken for a single line; the + `compare_against` field lets a repo override per-key if precision matters. +- Providers without a separate cache_write charge (OpenAI, Gemini, Grok) set + `cache_write = input` so the existing `cost()` formula continues to work + without a branch on provider. +- `cost_target` (the highlighted single line) and `compare_against` (the table) + are independent fields — see §3.2. + +### 3.1 Render + +Replace the single `**Est. cost on {provider}**: $X.XX` line in +`_render_collapsible_usage` with a compact markdown table: + +``` +**Equivalent cost on paid providers** (this run's measured tokens): + +| Provider | Cost | +|---|---:| +| Claude Sonnet 5 | $4.32 | +| GPT-5 | $2.71 | +| Gemini 2.5 Pro | $4.04 | +| Grok 4.5 | $4.32 | +``` + +Sort cheapest-first. Skip rows whose cost is `$0.00`. Bold the row matching +`cost_target` (the user-selected highlight). + +### 3.2 Config + +`.pr-review.json`: +```json +{ + "enabled": true, + "cost_target": "claude-sonnet-5", + "compare_against": ["claude-sonnet-5", "gpt-5", "gemini-2.5-pro", "grok-4.5"] +} +``` + +`parse_repo_config()`: +- Validate each key exists in `PRICES`. Drop unknowns to stderr (keeps + `cost_model._resolve_price_target`'s typo-reporting consistent). +- Cap the list at `CONFIG_MAX_LIST_ITEMS` (12). +- Default when absent: `["claude-sonnet-5", "gpt-5", "gemini-2.5-pro", + "grok-4.5"]`. + +### 3.3 Tests + +`tests/pilot/test_cost_model.py`: +- Add equivalent-cost golden vectors against the new price keys. +- Update `test_observed_report_prices_every_model` and + `test_report_renders_every_requested_model` to cover the new keys. +- Add `test_compare_against_parsing` (valid / unknown / over-cap / missing). + +## 4. Richer review summary + +### 4.1 Schema additions (agent prompts + `SYSTEM_PROMPT`) + +```json +{ + "walkthrough": [ + "file X: does Y", + "file Z: refactors W" + ], + "risk_verdict": "Medium risk: changes auth middleware without adding tests.", + "test_coverage": "No tests for behavioral change in pilot/foo.py." +} +``` + +Rules (added to `.opencode/agents/pragent.md`, each lens agent `.md`, and the +ollama `SYSTEM_PROMPT`): +- `walkthrough`: 2–6 bullets, file- or change-grouped, plain prose (no + severity emoji). Skip if the diff is one obvious line. +- `risk_verdict`: exactly one line. Lead with `Low|Medium|High|Critical risk:` + followed by a concrete reason grounded in the diff. +- `test_coverage`: short string. One of + `Tests added` / `Tests changed` / `No tests for behavioral change` / + `No test files in repo` / a repo-specific free-text override from + `instructions`. + +### 4.2 Parsing + +Extend `parse_review_output(text)` and the lens fan-out's synthetic-text +builder (`pilot/opencode_review.run_lenses_review`) to emit these three +fields in the final JSON block. Empty defaults preserve backward compat with +agents that haven't been re-deployed yet. + +### 4.3 Python fallback (when fields are empty) + +The multi-lens fan-out already synthesizes the findings JSON in Python today; +add a `_synthesize_summary_fields(findings, diff) -> dict` helper that +computes: +- `walkthrough`: group `merged` findings by `path`, one bullet per path + containing the peak severity emoji and the first-problem truncated to ~80 + chars. If `merged` is empty, list `changed_files(diff)` with the size of + the diff as the body ("`pilot/foo.py` — +12 lines"). +- `risk_verdict`: from `sev_counts` and `_multi_lens` flags: + - any critical → `Critical risk: critical finding(s).` + - any high → `High risk: high finding(s).` + - any medium → `Medium risk: medium finding(s) ( lens).` + - else `Low risk: clean or minor nits only.` +- `test_coverage`: scan `changed_files(diff)` with `is_test_path()`. Three + buckets: + - any test path changed alongside non-test paths → `Tests added` (or + `Tests changed`). + - non-test paths present, no test path → `No tests for behavioral change in + .` + - no test paths at all and non-test paths present → `No tests for + behavioral change in .` (same as above; the + distinction "no test files in repo" needs a tree scan — keep it simple + for v1). + +### 4.4 Render + +Extend `format_review_body()` to render three new sections between +`### Summary of Changes` and `### Key Risks & Concerns`: + +``` +### Risk Verdict +🟡 Medium risk: changes auth middleware without adding tests. + +### Walkthrough +- `pilot/foo.py` — adds retry logic for transient Gitea API errors +- `pilot/bar.py` — extracts shared header parser + +### Test Coverage +No tests for behavioral change in pilot/foo.py. +``` + +Each section renders an `_No
provided._` placeholder when empty +(matches the existing `Summary of Changes` / `Key Risks & Concerns` collapse +behavior). + +### 4.5 Tests + +`tests/pilot/test_ai_review.py`: +- Golden vectors for each new section (provided + Python-fallback paths). +- Combined body test: summary + walkthrough + risk + tests + table + + collapsible usage all render in the right order with no orphan markers. + +## 6. Stolen ideas + +### 6.1 Merge confidence 1–5 (Greptile) + +New function `merge_confidence(findings: list[dict]) -> int` in +`pilot/ai_review.py`: + +``` +start at 5 +-1 if any critical finding +-1 if any high finding +-1 if any medium finding +-1 if any _multi_lens: True finding (cross-lens agreement = harder to dismiss) +clamp to [1, 5] +``` + +Render in `REVIEW_HEADER`: + +``` +🤖 **AI Review** · pragent pilot · glm-5.2:cloud · `abc12345` · Merge confidence: 3/5 🟡 +``` + +Badge map: 5/4 = 🟢, 3 = 🟡, 2 = 🟠, 1 = 🔴. + +Tests: golden vectors for all 5 score branches. + +### 6.2 Add `trivial` + `info` severity levels (CodeRabbit) + +Extend `SEVERITIES` and `SEVERITY_RANK`: + +``` +SEVERITIES = ("critical", "high", "medium", "low", "trivial", "info") +SEVERITY_RANK = {"info": -1, "trivial": 0, "low": 1, "medium": 2, "high": 3, "critical": 4} +``` + +Update `_severity_badge` emoji map (`trivial`/`info` = ⚪). Update +`apply_repo_config` threshold semantics so `medium+` still means what it +meant (only `low` ranks below `medium` is unchanged). Update agent prompts +to permit emitting `trivial` / `info`. Backward compat: `_normalize_finding` +already coerces unknown severities to `medium`. + +Tests: existing `test_apply_repo_config` cases keep passing; add +`test_severity_threshold_respects_new_levels` and +`test_unknown_severity_normalizes_to_medium`. + +### 6.3 Reachability-aware severity demotion — DEFER + +CodeRabbit Security demotes severity by one level when a vulnerability is +unreachable / only theoretically exploitable. We can't compute reachability +without the security graph. Document in §7 and revisit when a Code-Rabbit- +style graph index lands. + +### 6.4 Rules mining from feedback — DEFER + +`pilot/feedback_harvest.py` + `pilot/feedback_analyze.py` exist. A future +`pilot/learn_rules.py` cron job will distill FP-vote signals into +`.pr-review.learned.json` and merge into `instructions`. Document in §7. + +### 6.5 Sequence diagrams / T-rex / cross-repo — SKIP + +Too heavy for the pilot's footprint. Document in §7. + +## 7. Deferred (not in this update) + +- **Reachability-aware severity demotion.** Requires a Code-Rabbit-style + reachability graph over the repo. +- **Rules mining from feedback.** A `learn_rules.py` job that consumes the + feedback DB and writes `.pr-review.learned.json`. `feedback_harvest` / + `feedback_analyze` are the substrate. +- **Sequence diagrams / T-rex sandbox / cross-repo review.** Three features + Greptile / Qodo highlight. All require either a code graph index (heavy + precompute) or sandbox runtime execution (separate infra). Skip. +- **Per-finding confidence scores.** Greptile publishes a 0–5 score on every + comment. We deliberately stay on severity — confidence on findings + requires the agent to self-estimate, which is unreliable without a + cross-lens consensus check. The merge-confidence badge (§6.1) is the + higher-signal version of the same idea. +- **`Fix with Cursor` handoff.** Greptile ships a one-click "send all findings + to Cursor/Codex/Claude Code." Our users *are* the bot's host, not an + external coding IDE. Skip. +- **Cost-model batch column.** The cost model already prices batch at 50%; + the PR-review path will never use it (stateful agent loops aren't + batchable). Keep the column for completeness, no new work. + +## 8. Risk register + +| Risk | Mitigation | +|---|---| +| Webhook floods the API with `.pr-review.json` fetches on a large owner | One `GET` per PR event, mostly 404. Documented acceptable. The dedicated `/health` already reports inflight count. | +| `.pr-review.json:enabled` set on a high-traffic repo creates surprise review load | The README will document the opt-in explicitly. The webhook's `PRAGENT_MAX_CONCURRENT_REVIEWS` already bounds the spawn rate. | +| New severity levels (`trivial` / `info`) break repos that filter on `medium+` | `apply_repo_config` threshold semantics preserve the rank of `low` and `medium`. `trivial` ranks below `low`, `info` below `trivial`. New filters naturally include them. | +| Token humanization loses precision a maintainer relies on | `fmt_tokens` always keeps the full comma-separated number; the short suffix is a parenthetical. | +| Multi-provider cost table is misleading when a provider has tiered pricing | `compare_against` is a per-repo override. The README documents the midpoints for Gemini 2.5 Pro. | +| Agent prompt change for `walkthrough` / `risk_verdict` / `test_coverage` causes regressions on deployed agents | Python fallback (§4.3) synthesizes the same fields when the agent omits them. Backward compat preserved by empty defaults. | +| Removing `report_usage` breaks the `review_pr` tests that pass it | Test updates are part of this update. | +| Removing labels breaks users who still apply them | No Gitea API change is needed; the labels just stop being read. A one-paragraph README note acknowledges the change. | + +## 9. Prioritized implementation list + +| # | Item | Section | Effort | +|---|---|---|---| +| P0 | Label removal + repo opt-in (`enabled` in `.pr-review.json`) | §1 | M | +| P1 | `fmt_tokens()` helper + apply in usage + inline | §2 | S | +| P1 | Multi-provider cost table (extend `PRICES`, render table, `compare_against`) | §3 | M | +| P2 | Richer summary (`walkthrough` / `risk_verdict` / `test_coverage`) schema + Python fallback | §4 | L | +| P2 | `trivial` + `info` severity levels | §6.2 | S | +| P3 | Merge confidence 1–5 in review header | §6.1 | S | +| P3 | README + `pilot/README-webhook.md` rewrite | §1, §10 | S | +| P3 | Test updates across all sections | (each) | M | + +P0 first because it changes webhook behavior (must land with the repo-opt-in +docs so onboarding isn't broken mid-rollout). P1 items are independent and +small — ship together. P2 ships the user-visible summary improvement. \ No newline at end of file