From 3ab894371a1824a834e2c71e961367c16f743d49 Mon Sep 17 00:00:00 2001 From: Jack Rothrock Date: Fri, 11 Sep 2026 13:22:04 -0600 Subject: [PATCH] feat(pr-review): conductor can trigger a full review via API/CLI/skill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lets the host (global) Claude run a full PR review itself, not just the browser button: - API: POST /api/prs//review starts the review WORKFLOW as a background job (auto-runs enrich + risk if missing, then the pr.review prompt); poll GET /api/prs//analysis (new "review" field), read GET /api/prs//review. POST /api/prs//comment publishes the markdown as a PR comment. All in the OpenAPI spec so the conductor discovers them (drift test passes). - CLI: `corral pr review [--wait]` — start, or wait-and-print. - Skill: a "Full review of a PR" section that distinguishes a full review (host, read-only — reads the PR, writes a review, does NOT run code) from a sandbox verify (runs the branch in a container), and states they're independent and can run AT THE SAME TIME. Maps the phrasings: "do a full review" → review only; "do a full review AND verify in a sandbox" → both, concurrently. - docs/CHEAT_SHEET.md: human-readable, table-driven examples of what to say to the dashboard's Claude (fake repos project-planner/invoice-service), matched to real API/CLI/skill actions; linked from docs/README. Test: TestAPIPRReviewRoutes (stored review null before a run, analysis exposes the review job, start route wired). Dashboard + prreview + automations pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/CHEAT_SHEET.md | 107 ++++++++++++++++++++ docs/README.md | 1 + host/skills/corral-api/SKILL.md | 43 ++++++++ internal/dashboard/api_pr_review_test.go | 57 +++++++++++ internal/dashboard/automations_endpoints.go | 8 ++ internal/dashboard/openapi.json | 107 ++++++++++++++++++++ internal/dashboard/pr_analysis_jobs.go | 58 +++++++++++ internal/dashboard/pr_cli.go | 76 ++++++++++++++ 8 files changed, 457 insertions(+) create mode 100644 docs/CHEAT_SHEET.md create mode 100644 internal/dashboard/api_pr_review_test.go diff --git a/docs/CHEAT_SHEET.md b/docs/CHEAT_SHEET.md new file mode 100644 index 00000000..f7d912b9 --- /dev/null +++ b/docs/CHEAT_SHEET.md @@ -0,0 +1,107 @@ +# Talking to Claude — a cheat sheet + +The global Claude in the dashboard (open it with **⌘K**, or the **Ask Claude** +button) is a *conductor*: you talk to it in plain English and it drives Corral for +you — starting projects, reviewing PRs, building images, running flows. + +This page is a set of **things you can just say**, grouped by what you want to get +done. They're examples, not magic words — say them your own way. The repo names +here (`project-planner`, `invoice-service`, …) are made up; swap in yours. + +> Tip: the more specific you are, the better. "Review PR #212 and post the +> findings as a comment" beats "look at that PR". + +--- + +## Reviewing a pull request + +Corral has two *different* things it can do to a PR. Knowing which you want gets +you the right result: + +- **Full review** — Claude *reads* the PR (the diff, the risk analysis, the hot + spots) and writes up a review. It never runs the code. Fast. +- **Sandbox verify** — Claude spins up a throwaway sandbox on the PR's branch and + actually *runs* it: builds it, runs the tests, tries the app. Slower, but it + proves the change works. + +You can ask for either — or **both at once**. + +| Say this | What happens | +|---|---| +| *"Do a full review of PR #212 in `project-planner`."* | Full review only. Reads + writes a review. | +| *"Review PR #212 and post the findings as a comment."* | Full review, then drafts a PR comment for you to approve. | +| *"Verify PR #212 works in a sandbox."* | Sandbox verify only. Runs the branch in a container. | +| *"Do a full review of PR #212 **and** verify it works in a sandbox."* | Both — the review runs on the host while the sandbox builds and tests. They run at the same time and report back separately. | +| *"What's the risk on PR #212?"* | Runs the risk analysis and summarizes it. | + +--- + +## Starting work in a sandbox + +Sandboxes are isolated containers where Claude does actual code work — editing, +building, running — without touching your real machine. + +| Say this | What happens | +|---|---| +| *"Start a project on `invoice-service` and add PDF export to invoices."* | Creates a sandbox on the repo, hands Claude the task, and starts it. | +| *"Create a project using both `invoice-service` and `project-planner`, and wire the planner to read invoices from the invoice API."* | A single sandbox with **both** repos checked out, working the cross-repo task. | +| *"Spin up a sandbox on the `feature/webhooks` branch of `invoice-service` and get the tests passing."* | Same, but on a specific branch. | +| *"Work GitHub issue #88 in `project-planner`."* | Creates a sandbox seeded from the issue and starts on it. | + +> "Create a project" and "start a project" go together — Claude creates it *and* +> starts it, then hands your task to the sandbox's own Claude and supervises. + +--- + +## Getting an app running to look at + +| Say this | What happens | +|---|---| +| *"Get `project-planner` running so I can see it."* | Boots the app in a sandbox and points the **Live View** tab at the right port. | +| *"The image for `invoice-service` is missing — build it."* | Builds the repo's Docker image inside a sandbox and saves it as the repo's baseline so future projects reuse it. | +| *"Verify the Live View actually renders, don't just assume."* | Drives a real browser at the Live View to confirm it's not a blank page. | + +--- + +## Checking on things + +| Say this | What happens | +|---|---| +| *"What projects are running right now?"* | Lists projects and whether each is working, waiting, or idle. | +| *"Is the `project-planner` sandbox stuck?"* | Reads that sandbox's conversation and tells you what it's doing. | +| *"Show me the open PRs across my repos."* | The cross-repo PR inbox. | +| *"What flows do I have, and when did the triage one last run?"* | Lists automations/flows and recent runs. | +| *"Pull up the recent AI activity."* | Recent entries from the activity log. | + +--- + +## GitHub housekeeping + +| Say this | What happens | +|---|---| +| *"Create an issue in `invoice-service`: 'Refunds double-charge on retry', and start work on it."* | Files the issue, then spins up a sandbox to work it. | +| *"Merge PR #212 in `project-planner` with squash."* | Merges via your configured strategy (waits for CI, no force). | +| *"Leave a note on PR #212: watch the migration in 0012."* | A private, local note — **not** posted to GitHub. | +| *"Comment on PR #212 with the review."* | Posts a comment to GitHub (this one *is* public). | + +--- + +## Doing several at once + +The conductor is happy to fan out. You can stack asks in one sentence: + +- *"Do a full review of PR #212, verify it in a sandbox, and while that runs, get + `project-planner` up in Live View."* +- *"Start projects on `invoice-service` and `project-planner`, and in each, run the + test suite and tell me what fails."* + +It kicks each off, keeps an eye on them, and reports back as results land. + +--- + +### Two quick reminders + +- **The global Claude runs on your machine, not in a sandbox.** It orchestrates; + the *code work* happens inside sandboxes it creates. That's the safety boundary. +- **Nothing goes to GitHub unless you say so.** Reviews, notes, and analysis stay + local until you explicitly ask to comment, merge, or push. diff --git a/docs/README.md b/docs/README.md index 6ac29ffa..f0adc4c7 100644 --- a/docs/README.md +++ b/docs/README.md @@ -6,6 +6,7 @@ you a dashboard to drive them. For quick help while you're in the app, press ## Start here +- [Cheat sheet](CHEAT_SHEET.md) — things you can just *say* to the dashboard's Claude. - [Using Corral](usage.md) — install, the CLI, and the end-to-end workflow. - [Security model](security.md) — the trust boundary, in detail. - [Architecture](architecture.md) — how the pieces fit (proxy, firewall, container). diff --git a/host/skills/corral-api/SKILL.md b/host/skills/corral-api/SKILL.md index 8cf729b5..c6d70747 100644 --- a/host/skills/corral-api/SKILL.md +++ b/host/skills/corral-api/SKILL.md @@ -513,6 +513,49 @@ corral api POST /api/repos//analyze # git-only, synchronous corral api GET /api/repos//analysis-status # analyzed / up-to-date? ``` +### Full review of a PR (the "do a full review" ask) + +A **full review** is a single workflow that ties the above together: it auto-runs +the block/heuristics + risk analysis if they're missing, then feeds those findings +plus the PR description and diff into the editable **`pr.review`** prompt (host +Claude, **read-only**) and stores a markdown review. Also fire-and-return: + +``` +corral api POST /api/prs//review # start the full review (background) +corral api GET /api/prs//analysis # poll → the "review" field {status} +corral api GET /api/prs//review # read the markdown once status=done +# or, from the CLI, do all of that in one call: +corral pr review --wait # starts, waits, prints the review +``` + +To **post the review as a PR comment** (this writes to GitHub), take the markdown +from `/review` and send it: + +``` +corral api POST /api/prs//comment -d '{"body":""}' +``` + +**Full review ≠ sandbox verify — know which the user wants.** These are two +different things and they are INDEPENDENT (you can run one, the other, or both at +once): + +- **Full review** (this endpoint) — host, read-only. It READS the PR (diff + + prior analysis) and WRITES a review. It does NOT run the code, build, or test. + This is what "do a full review of PR #N" means. +- **Sandbox verify** — spins up a sandbox project on the PR's branch and actually + RUNS the change (build, tests, the app) inside a container. This is the + `POST /projects/create` (PR branch + a verify prompt) → `POST /p//start` + flow above. This is what "verify it works" / "run it in a sandbox" means. + +Map the ask to the right action(s): + +- *"Do a full review of PR #14."* → just `POST /api/prs//review`. +- *"Verify PR #14 works in a sandbox."* → just the create+start sandbox verify. +- *"Do a full review AND verify PR #14 in a sandbox."* → do BOTH, and you can + kick them off at the SAME TIME — the review runs on the host while the sandbox + builds/tests. They don't depend on each other; start both, then report each + result as it lands. + ## PR notes (private, local) Notes are **local annotations stored only in Corral** — never posted to GitHub diff --git a/internal/dashboard/api_pr_review_test.go b/internal/dashboard/api_pr_review_test.go new file mode 100644 index 00000000..71b876f7 --- /dev/null +++ b/internal/dashboard/api_pr_review_test.go @@ -0,0 +1,57 @@ +package dashboard + +import ( + "encoding/json" + "net/http" + "strings" + "testing" +) + +// TestAPIPRReviewRoutes exercises the conductor-facing review surface: the stored +// review is null before a run, the analysis status carries a "review" field, and +// the start route is reachable (returns started, or 502 when claude is absent — +// either proves the route is wired, which is what we're testing here). +func TestAPIPRReviewRoutes(t *testing.T) { + srv, _, prID := apiTestServer(t) + defer srv.Close() + base := "/api/prs/" + itoa(prID) + + // GET review before any run → {"review": null}. + code, body := apiReq(t, srv, http.MethodGet, base+"/review", "") + if code != http.StatusOK { + t.Fatalf("GET review = %d, want 200 (%s)", code, body) + } + var got struct { + Review *struct { + Markdown string `json:"markdown"` + } `json:"review"` + } + if err := json.Unmarshal([]byte(body), &got); err != nil { + t.Fatalf("decode review: %v (%s)", err, body) + } + if got.Review != nil { + t.Errorf("expected null review before a run, got %+v", got.Review) + } + + // The analysis status must expose a "review" job alongside enrich + risk, so a + // conductor can poll it. + code, body = apiReq(t, srv, http.MethodGet, base+"/analysis", "") + if code != http.StatusOK { + t.Fatalf("GET analysis = %d, want 200", code) + } + for _, want := range []string{`"review"`, `"enrich"`, `"risk"`} { + if !strings.Contains(body, want) { + t.Errorf("analysis status missing %s: %s", want, body) + } + } + + // POST review starts the job (200 started) or 502 if the claude CLI isn't on + // this machine — both mean the route is wired. A 404 would mean it isn't. + code, body = apiReq(t, srv, http.MethodPost, base+"/review", "{}") + if code != http.StatusOK && code != http.StatusBadGateway { + t.Fatalf("POST review = %d, want 200 or 502 (%s)", code, body) + } + if code == http.StatusOK && !strings.Contains(body, `"review"`) { + t.Errorf("started review response should name the kind: %s", body) + } +} diff --git a/internal/dashboard/automations_endpoints.go b/internal/dashboard/automations_endpoints.go index 1c4a48e9..84c30333 100644 --- a/internal/dashboard/automations_endpoints.go +++ b/internal/dashboard/automations_endpoints.go @@ -183,6 +183,14 @@ func (d *dashboardServer) handleAPIPRItem(w http.ResponseWriter, r *http.Request d.handleAPIPREnrich(w, r, prID) case action == "analyze" && r.Method == http.MethodPost: d.handleAPIPRRiskStart(w, r, prID) + case action == "review" && r.Method == http.MethodPost: + d.handleAPIPRReviewStart(w, r, prID) + case action == "review" && r.Method == http.MethodGet: + d.handlePRReviewGet(w, r, prID) + case action == "comment" && r.Method == http.MethodPost: + // Post a comment on the PR (posts to GitHub via gh). Used to publish a + // full-review result: read GET .../review, then POST it here as {body}. + d.handlePRAction(w, r, prID, "comment") case action == "analysis" && r.Method == http.MethodGet: d.handleAPIPRAnalysisStatus(w, r, prID) case action == "blocks" && r.Method == http.MethodGet: diff --git a/internal/dashboard/openapi.json b/internal/dashboard/openapi.json index ede27511..53a767b7 100644 --- a/internal/dashboard/openapi.json +++ b/internal/dashboard/openapi.json @@ -2431,6 +2431,113 @@ } } }, + "/api/prs/{id}/review": { + "parameters": [ + { + "name": "id", + "in": "path", + "required": true, + "schema": { + "type": "string" + } + } + ], + "post": { + "summary": "Start a FULL REVIEW of a PR — fire and return", + "description": "Runs the full-review WORKFLOW in the BACKGROUND (returns immediately): auto-runs the block/heuristics + risk analysis if missing, then feeds those findings + the PR description + diff into the editable pr.review prompt (host Claude, read-only) and stores the markdown. This is a REVIEW ONLY — it reads the PR and writes a review; it does NOT run the code. To also run the change (build/tests) in an isolated container, ALSO start a sandbox verify (POST /projects/create with the PR branch + a verify prompt, then POST /p/{id}/start); the two are independent and can run at the same time. Poll GET /api/prs/{id}/analysis (the \"review\" field) for completion, then read the result from GET /api/prs/{id}/review.", + "responses": { + "200": { + "description": "Started", + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "ok": { + "type": "boolean" + }, + "kind": { + "type": "string" + }, + "status": { + "type": "string" + } + } + } + } + } + }, + "502": { + "description": "claude CLI unavailable" + } + } + }, + "get": { + "summary": "Read a PR's stored full review", + "description": "The last computed full-review markdown (null if never run). Read after the review job finishes (poll GET /api/prs/{id}/analysis). The markdown can be posted as a PR comment via POST /api/prs/{id}/comment.", + "responses": { + "200": { + "description": "Review", + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "review": { + "type": "object", + "nullable": true + } + } + } + } + } + } + } + } + }, + "/api/prs/{id}/comment": { + "parameters": [ + { + "name": "id", + "in": "path", + "required": true, + "schema": { + "type": "string" + } + } + ], + "post": { + "summary": "Post a comment on the PR (to GitHub)", + "description": "Posts a comment on the PR via gh. Use it to PUBLISH a full-review result: read GET /api/prs/{id}/review, then POST its markdown here as {body}. This writes to GitHub, so it is subject to the API-writes gate.", + "requestBody": { + "required": true, + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "body": { + "type": "string", + "description": "The comment markdown to post." + } + }, + "required": [ + "body" + ] + } + } + } + }, + "responses": { + "200": { + "description": "Posted" + }, + "400": { + "description": "Not a GitHub remote" + } + } + } + }, "/api/prs/{id}/stack": { "parameters": [ { diff --git a/internal/dashboard/pr_analysis_jobs.go b/internal/dashboard/pr_analysis_jobs.go index cffc2af6..44f9a6be 100644 --- a/internal/dashboard/pr_analysis_jobs.go +++ b/internal/dashboard/pr_analysis_jobs.go @@ -151,6 +151,63 @@ func (d *dashboardServer) startRisk(prID, parentConvID int64) error { return nil } +// startReview kicks off the FULL-REVIEW workflow in the background: it auto-runs +// the block/heuristics + risk analysis if missing, then feeds those findings + +// the PR description + diff into the editable pr.review prompt (host, read-only) +// and stores the markdown. Long-running, so it's fire-and-poll like enrich/risk. +func (d *dashboardServer) startReview(prID, parentConvID int64) error { + claudeBin, err := resolveClaudeBin() + if err != nil { + return fmt.Errorf("the `claude` CLI could not be located — install Claude Code and restart the dashboard") + } + if !d.analysisJobs.begin(prID, "review") { + return nil // already running — idempotent start + } + go func() { + s, err := d.getStore() + if err != nil { + d.analysisJobs.finish(prID, "review", err) + return + } + svc := prreview.New(s).WithPromptResolver(d.promptResolver()) + repoID, _ := svc.RepoIDForPR(prID) + num, _, _, _, _ := svc.PRHookContext(prID) + ctx, endSpan := d.applog().StartSpan(context.Background(), applog.Entry{ + Category: applog.CatAI, Event: "ai.review", + Message: applog.Fmt("Full review PR #%d (api)", num), + RepoID: repoID, Meta: map[string]any{"pr": num, "via": "api"}, + }) + runner := func(kind string) aiRunner { + return d.capturingRunner(ctx, convOrigin{ + Kind: "analysis", OriginID: fmt.Sprintf("%s-%d", kind, prID), RepoID: repoID, PRNumber: num, + ParentConversationID: parentConvID, + }, prreview.NewClaudeRunner(claudeBin)) + } + // Auto-run prereqs if missing (one call does the whole workflow); best-effort. + if blocks, _ := svc.Blocks(prID); len(blocks) == 0 { + _, _ = svc.ExtractBlocks(ctx, prID, runner("enrich")) + } + if risk, _ := svc.StoredRisk(prID); risk == nil { + _, _ = svc.AnalyzeRisk(ctx, prID, runner("risk")) + } + _, aerr := svc.RunFullReview(ctx, prID, runner("review")) + endSpan(aerr) + d.analysisJobs.finish(prID, "review", aerr) + }() + return nil +} + +// handleAPIPRReviewStart: POST /api/prs//review — start the full-review +// workflow in the background; poll GET /api/prs//analysis (the "review" +// field), then read the result from GET /api/prs//review. +func (d *dashboardServer) handleAPIPRReviewStart(w http.ResponseWriter, r *http.Request, prID int64) { + if err := d.startReview(prID, parentConvFromRequest(r)); err != nil { + http.Error(w, err.Error(), http.StatusBadGateway) + return + } + writeFilesJSON(w, map[string]any{"ok": true, "kind": "review", "status": d.analysisJobs.state(prID, "review").Status}) +} + // handleAPIPREnrich: POST /api/prs//enrich — start the per-block AI analysis // in the background; returns immediately. Poll GET /api/prs//analysis for // completion, then read GET /api/prs//blocks. @@ -178,5 +235,6 @@ func (d *dashboardServer) handleAPIPRAnalysisStatus(w http.ResponseWriter, r *ht writeFilesJSON(w, map[string]any{ "enrich": d.analysisJobs.state(prID, "enrich"), "risk": d.analysisJobs.state(prID, "risk"), + "review": d.analysisJobs.state(prID, "review"), }) } diff --git a/internal/dashboard/pr_cli.go b/internal/dashboard/pr_cli.go index e3ec2bcb..cdb70756 100644 --- a/internal/dashboard/pr_cli.go +++ b/internal/dashboard/pr_cli.go @@ -6,6 +6,7 @@ import ( "os" "strconv" "strings" + "time" ) // CmdPR implements `corral pr` — inspect and manage a PR's LOCAL Corral links @@ -37,6 +38,8 @@ func CmdPR(args []string) error { return prLinksSuggest(rest) case "stack": return prStack(rest) + case "review": + return prReview(rest) case "link": return prLinkAdd(rest) case "unlink": @@ -54,6 +57,7 @@ func prUsage() error { links list a PR's local links suggest suggest PRs to link (by changed-file overlap) stack detect stacked PRs (git ancestry, from the mirror) + review [--wait] run a FULL AI review (risk+heuristics → review prompt) link [flags] link two PRs unlink remove a link @@ -201,6 +205,78 @@ func prStack(args []string) error { return prPrintResult(status, body) } +// prReview starts a full AI review of a PR (background job). With --wait it polls +// until the review finishes and prints the resulting markdown; otherwise it +// returns immediately after starting (poll GET /api/prs//analysis yourself). +// +// corral pr review [--wait] +// +// This is REVIEW ONLY — it reads the PR and writes a review; it does NOT run the +// code. To also run the change in a sandbox, create+start a project on the PR +// branch separately; the two are independent and can run at the same time. +func prReview(args []string) error { + if len(args) < 1 { + return fmt.Errorf("usage: corral pr review [--wait]") + } + id, err := parsePRID(args[0]) + if err != nil { + return err + } + wait := false + for _, a := range args[1:] { + if a == "--wait" { + wait = true + } + } + + status, body, err := dashboardRequest("POST", fmt.Sprintf("/api/prs/%d/review", id), "{}") + if err != nil { + return err + } + if status < 200 || status >= 300 { + return prPrintResult(status, body) + } + if !wait { + fmt.Printf("Started full review of PR %d. Poll GET /api/prs/%d/analysis (the \"review\" field), then read GET /api/prs/%d/review — or re-run with --wait.\n", id, id, id) + return nil + } + + // Poll the analysis status until the review job leaves "running", then print + // the stored review markdown. Bounded so a stuck job doesn't hang forever. + fmt.Fprintln(os.Stderr, "Reviewing (risk + heuristics → review)… this can take a minute.") + for i := 0; i < 240; i++ { // ~4 min at 1s + time.Sleep(1 * time.Second) + st, sb, serr := dashboardRequest("GET", fmt.Sprintf("/api/prs/%d/analysis", id), "") + if serr != nil || st < 200 || st >= 300 { + continue + } + var an struct { + Review struct { + Status string `json:"status"` + Error string `json:"error"` + } `json:"review"` + } + if json.Unmarshal(sb, &an) != nil { + continue + } + switch an.Review.Status { + case "done": + _, rb, rerr := dashboardRequest("GET", fmt.Sprintf("/api/prs/%d/review", id), "") + if rerr != nil { + return rerr + } + os.Stdout.Write(rb) + if len(rb) > 0 && rb[len(rb)-1] != '\n' { + fmt.Fprintln(os.Stdout) + } + return nil + case "failed": + return fmt.Errorf("review failed: %s", an.Review.Error) + } + } + return fmt.Errorf("timed out waiting for the review; poll GET /api/prs/%d/review", id) +} + func prLinkAdd(args []string) error { // Positional: ; then optional --rel/--note (any order). var pos []string