diff --git a/AGENTS.md b/AGENTS.md index f9bcb79..ba7e066 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,8 +2,8 @@ GLaDOS reviews open PRs with a pending review request for the authenticated user, and replies when humans respond on its review-comment threads. -1. **Full review (Phase B):** GitHub Search finds `review-requested:@me` PRs → clone → Cursor SDK local agent → post summary + inline comments (`APPROVE` or `REQUEST_CHANGES`). -2. **Thread replies (Phase A):** On every PR notification wake-up (and before each full review) → inspect GLaDOS review threads → agree (reply + resolve) or disagree (reply, leave open). Settled agreements are never re-raised on later reviews of that PR. +1. **Full review:** GitHub Search finds `review-requested:@me` PRs → clone → Cursor SDK local agent → post summary + inline comments (`APPROVE` or `REQUEST_CHANGES`). +2. **Thread replies (optional feature):** On every PR notification wake-up (and before each full review) → inspect GLaDOS review threads → agree (reply + resolve) or disagree (reply, leave open). Settled agreements are never re-raised on later reviews of that PR. TypeScript, ESM (`"type": "module"`), Node ≥ 22.13. Run scripts with `tsx`. @@ -13,7 +13,7 @@ TypeScript, ESM (`"type": "module"`), Node ≥ 22.13. Run scripts with `tsx`. export GLADOS_TOKEN='ghp_...' # GitHub PAT: search + notifications + repo + pull_requests export CURSOR_API_KEY='cursor_...' -npm run notifications # Phase A + Phase B for pending reviews; Phase A for PR notifications; clear inbox +npm run notifications # review-requested PRs + PR notification wake-ups; clear inbox npm run notifications -- --all # include read notifications when listing inbox npm run smoke # local Cursor SDK smoke test (cwd = this repo) npm test # unit tests (node:test via tsx) @@ -31,7 +31,7 @@ Only `src/cli/` contains runnable entrypoints. Everything else is library code. ``` src/ cli/ - notifications.ts # entrypoint: review-requested PRs, then PR notification wake-ups, inbox cleanup + notifications.ts # entrypoint: wires features; inbox cleanup smoke.ts # entrypoint: one-shot local Agent.prompt smoke test types.ts # NotificationThread, PullRequestRef @@ -44,18 +44,38 @@ src/ pr.ts # search review-requested PRs, pending-reviewer check, notification → PR ref diff.ts # getCommentableLines() — RIGHT-side lines that accept comments reviews.ts # postGithubReview() — validate vs diff, then createReview - threads.ts # GraphQL: list/get review threads, reply, resolve - - review/ - process.ts # processPrReview() — Phase A then Phase B wiring only - thread-process.ts # processPrThreadReplies() / processReviewThreads() — Phase A orchestration - agent.ts # runAgentReview() / runThreadReplies() — Cursor SDK + sandbox/env isolation - payload.ts # review prompt, parse, GitHub formatting, personality, strip control markers - threads.ts # pure: awaiting/settled classification, Phase A prompt/parse, suppress settled findings + threads.ts # GraphQL: list/get review threads, reply, resolve (thread-replies I/O) + + review/ # core PR review feature — no thread-reply knowledge + process.ts # processPrReview() — orchestration only + agent.ts # runAgentReview() / promptLocalAgent() — Cursor SDK + sandbox/env + payload.ts # review prompt, parse, GitHub formatting, personality + + thread-replies/ # optional feature — delete this folder to rip out + index.ts # public API only (import from here outside the folder) + process.ts # Phase A + compose with full review + agent.ts # runThreadReplies() + logic.ts # classification, markers, suppress, Phase A prompt/parse ``` Co-located `*.test.ts` files use Node’s built-in test runner (`npm test`). +### Feature isolation / rip-out + +| Feature | Folder | CLI wiring | +|---------|--------|------------| +| Core PR review | `src/review/` | `processPrReview` | +| Thread replies | `src/thread-replies/` | `processPrReviewWithThreadReplies`, `processPrThreadReplies` | + +Today the CLI imports only from `thread-replies/` for reviews (that composer calls core review pieces). To disable thread replies: + +1. In `cli/notifications.ts`, call `processPrReview` instead of `processPrReviewWithThreadReplies`, and remove the PR-notification → `processPrThreadReplies` branch (mark those notifications done like others). +2. Delete `src/thread-replies/`. +3. Delete `src/github/threads.ts` if unused. +4. Trim this guide’s Phase A sections. + +New optional features get their own top-level folder under `src/` with an `index.ts` public surface; CLI wires them. Do not push feature logic into `review/`. + ## Flow ``` @@ -64,16 +84,14 @@ cli/notifications.ts github/pr.listReviewRequestedPullRequests() # is:open is:pr review-requested:@me github/notifications.listNotifications() → per review-requested PR: - review/process.processPrReview() - → withPrLock - → isReviewRequestedForUser() # skip if not on pending list - → withPrWorkspace → /tmp/glados-*/repository - → thread-process.processReviewThreads() # Phase A - → agent.runAgentReview(+ settled context) # Phase B - → suppressSettledFindings() - → payload.buildGithubReview() → reviews.postGithubReview() + thread-replies.processPrReviewWithThreadReplies() + → withPrLock + withPrWorkspace + → Phase A (processReviewThreads) + → review/agent.runAgentReview(+ settled context) + → suppressSettledFindings + sanitize markers + → payload.buildGithubReview → reviews.postGithubReview → per PullRequest notification (wake-up only; GitHub coalesces PR activity): - thread-process.processPrThreadReplies() # Phase A only; clone only if a reply is needed + thread-replies.processPrThreadReplies() # Phase A only; clone only if a reply is needed markNotificationDone() only if Phase A complete → other notifications: markNotificationDone() ``` @@ -84,7 +102,7 @@ cli/notifications.ts **Per-PR lock:** `withPrLock()` serializes overlapping local CLI runs so two processes cannot reply/review the same PR at once. -## Phase A — thread replies +## Phase A — thread replies (`src/thread-replies/`) **Awaiting:** root comment by GLaDOS, unresolved, not settled, and there is an unacknowledged human reply (anyone) after GLaDOS’s last acknowledgment. @@ -93,15 +111,15 @@ cli/notifications.ts **Actions:** - Agree → controlled reply (`` + ``) → `resolveReviewThread` when `viewerCanResolve`. - Disagree → controlled reply with acknowledgment marker only; leave open. Back-and-forth continues indefinitely whenever someone replies after GLaDOS. -- Agreed but unresolved → retry resolve only (no new agent reply). If `viewerCanResolve` is false, leave Phase A incomplete so the notification is retained for later retry; still treat the issue as settled for Phase B. +- Agreed but unresolved → retry resolve only (no new agent reply). If `viewerCanResolve` is false, leave Phase A incomplete so the notification is retained for later retry; still treat the issue as settled for the following full review. **Race safety:** re-fetch the target thread immediately before each write; compare full comment id/body/`updatedAt` snapshots. After posting, reconcile — if a human raced in, the thread stays awaiting. -**Markers:** appended only by `formatReplyBody()` / controlled code. Strip from all model-authored review text via `stripGladosControlMarkers()`. Root findings never count as agreements. +**Markers:** appended only by `formatReplyBody()` / controlled code in `thread-replies/logic.ts`. Stripped before posting reviews via `sanitizeReviewForPost()`. Root findings never count as agreements. -## Phase B — full review +## Full review (`src/review/`) -Inject settled findings (original body + agreement reason) into `buildReviewPrompt()`. After parse, `suppressSettledFindings()` removes exact text repeats and regenerates the summary if anything was dropped. Open/disagreed threads do **not** suppress new findings. +When composed with thread replies, settled findings (original body + agreement reason) are injected as `extraContext` into `buildReviewPrompt()`. After parse, `suppressSettledFindings()` removes exact text repeats. Open/disagreed threads do **not** suppress new findings. **Approve vs request changes:** `critical` or `high` → `REQUEST_CHANGES`; else `APPROVE`. @@ -131,9 +149,9 @@ Inject settled findings (original body + agreement reason) into `buildReviewProm | Reviewer instructions / JSON schema | `review/payload.ts` → `buildReviewPrompt()` | | Severity levels | `review/payload.ts` → `SEVERITIES` + `Severity` | | GLaDOS voice | `review/payload.ts` → `applyPersonality()` (post-time rewrite) | -| Thread reply prompt / classification / suppress | `review/threads.ts` | -| Phase A orchestration | `review/thread-process.ts` | -| Phase B wiring | `review/process.ts` (no business logic) | +| Thread reply prompt / classification / suppress | `thread-replies/logic.ts` | +| Phase A + compose with review | `thread-replies/process.ts` | +| Core review wiring | `review/process.ts` (no business logic) | | Clone / lock / cleanup | `git/workspace.ts` | | GitHub I/O | `github/` | @@ -145,8 +163,9 @@ Design detail: `docs/superpowers/specs/2026-08-05-review-thread-replies-design.m - **ESM imports** use `.js` extensions in TypeScript source. - **New runnable scripts** go in `src/cli/` only; wire in `package.json`. -- **New library code** goes in `github/`, `git/`, or `review/` — not a generic `utils/`. -- **Keep modules small:** `agent.ts` = SDK + isolation; `payload.ts` / `threads.ts` = pure data; `process.ts` / `thread-process.ts` = wiring. +- **New library code** goes in a domain folder (`github/`, `git/`, `review/`) or a feature folder (`thread-replies/`, …) — not a generic `utils/`. +- **Feature folders** expose a small `index.ts` public API; outside code imports only from that. +- **Keep modules small:** `agent.ts` = SDK + isolation; `payload.ts` / `logic.ts` = pure data; `process.ts` = wiring. - **Tests** live next to the code they cover (`*.test.ts`); add cases when changing classification, markers, locks, or suppression. - **Minimize scope** — match existing style, no over-abstraction. @@ -157,4 +176,4 @@ Design detail: `docs/superpowers/specs/2026-08-05-review-thread-replies-design.m | `GLADOS_TOKEN` | GitHub API (search, notifications, clone auth, reviews, thread reply/resolve) | | `CURSOR_API_KEY` | Cursor SDK local agent runs | -`GLADOS_TOKEN` needs access to arbitrary repos that send review requests (`repo` scope or equivalent). Resolving conversations additionally requires being the PR author **or** having write access on the repo — when that fails, agreement still settles for Phase B but Phase A stays incomplete for retry. +`GLADOS_TOKEN` needs access to arbitrary repos that send review requests (`repo` scope or equivalent). Resolving conversations additionally requires being the PR author **or** having write access on the repo — when that fails, agreement still settles for the following full review but Phase A stays incomplete for retry. diff --git a/docs/superpowers/specs/2026-08-05-review-thread-replies-design.md b/docs/superpowers/specs/2026-08-05-review-thread-replies-design.md index 31703aa..9b9ce76 100644 --- a/docs/superpowers/specs/2026-08-05-review-thread-replies-design.md +++ b/docs/superpowers/specs/2026-08-05-review-thread-replies-design.md @@ -18,14 +18,14 @@ GLaDOS must respond when humans reply on its review comment threads: agree (shor ``` cli/notifications.ts - ├─ review-requested PRs → processPrReview() - │ 1. preparePrWorkspace() - │ 2. processReviewThreads() # Phase A - │ 3. runAgentReview(+ settled) # Phase B - │ 4. postGithubReview() + ├─ review-requested PRs → processPrReviewWithThreadReplies() + │ 1. withPrWorkspace() + │ 2. processReviewThreads() # Phase A (thread-replies/) + │ 3. runAgentReview(+ settled) # core review/ + │ 4. suppress + sanitize + post │ - └─ inbox: reply on a GLaDOS review thread - → processReviewThreads() only + └─ inbox: PullRequest notification wake-up + → processPrThreadReplies() only → markNotificationDone() only after successful processing ``` @@ -71,14 +71,15 @@ Open / disagreed threads are **not** suppressors; new findings on the same topic | Module | Responsibility | |--------|----------------| | `github/threads.ts` | List PR review threads (GraphQL), create thread reply, resolve thread; identify authenticated user | -| `review/threads.ts` | Pure: awaiting vs settled classification; prompt/parse for Phase A; format settled context for Phase B | -| `review/agent.ts` | `runThreadReplies()` for Phase A | -| `review/thread-process.ts` | Orchestrate Phase A thread reads, replies, resolution and retries | -| `review/process.ts` | Run Phase A, pass settled context into Phase B, post full review | -| `cli/notifications.ts` | Treat every PR notification as a fresh Phase A wake-up | - -Keep `process.ts` as wiring only; GitHub I/O in `github/`; prompt/parse and -classification in `review/threads.ts`. +| `thread-replies/` | Isolated feature folder (public API via `index.ts`) | +| `thread-replies/logic.ts` | Pure: awaiting vs settled classification; prompt/parse for Phase A; format settled context; suppress | +| `thread-replies/agent.ts` | `runThreadReplies()` for Phase A | +| `thread-replies/process.ts` | Phase A orchestration + `processPrReviewWithThreadReplies` composer | +| `review/` | Core PR review only — no thread-reply imports | +| `cli/notifications.ts` | Wires features; every PR notification is a Phase A wake-up | + +Keep feature logic inside `thread-replies/`; GitHub I/O in `github/`; core +review stays replaceable without knowing about threads. ## GitHub API notes diff --git a/package.json b/package.json index 5e82fda..c50b39a 100644 --- a/package.json +++ b/package.json @@ -5,7 +5,7 @@ "scripts": { "smoke": "tsx src/cli/smoke.ts", "notifications": "tsx src/cli/notifications.ts", - "test": "tsx --test src/git/workspace.test.ts src/github/pr.test.ts src/review/agent.test.ts src/review/payload.test.ts src/review/threads.test.ts", + "test": "tsx --test 'src/**/*.test.ts'", "typecheck": "tsc --noEmit" }, "dependencies": { diff --git a/src/cli/notifications.ts b/src/cli/notifications.ts index b9b24fc..b184e2e 100644 --- a/src/cli/notifications.ts +++ b/src/cli/notifications.ts @@ -8,8 +8,10 @@ import { pullRequestRefFromNotification, subjectUrlToWebUrl, } from "../github/pr.js"; -import { processPrReview } from "../review/process.js"; -import { processPrThreadReplies } from "../review/thread-process.js"; +import { + processPrReviewWithThreadReplies, + processPrThreadReplies, +} from "../thread-replies/index.js"; const token = process.env.GLADOS_TOKEN; if (!token) { @@ -36,7 +38,8 @@ try { for (const pr of reviewRequestedPrs) { console.log(` ${pr.owner}/${pr.repo} #${pr.prNumber}`); console.log(` ${pr.prUrl}`); - await processPrReview(pr, { + // Feature compose: Phase A + Phase B. Swap for processPrReview to rip out. + await processPrReviewWithThreadReplies(pr, { githubToken: token, cursorApiKey, }); diff --git a/src/review/agent.ts b/src/review/agent.ts index de0e04a..51611b5 100644 --- a/src/review/agent.ts +++ b/src/review/agent.ts @@ -1,27 +1,20 @@ import { Agent } from "@cursor/sdk"; import { mkdir } from "node:fs/promises"; import { basename, dirname, join } from "node:path"; -import type { ReviewThread } from "../github/threads.js"; import { buildReviewPrompt, parseReviewResult, type ReviewPayload, } from "./payload.js"; -import { - buildThreadReplyPrompt, - parseThreadReplyResult, - validateThreadReplyDecisions, - type ThreadReplyDecision, -} from "./threads.js"; export async function runAgentReview( repoDir: string, prUrl: string, cursorApiKey: string, - settledContext = "", + extraContext = "", ): Promise { - const result = await promptAgent( - buildReviewPrompt(prUrl, settledContext), + const result = await promptLocalAgent( + buildReviewPrompt(prUrl, extraContext), repoDir, cursorApiKey, ); @@ -44,44 +37,11 @@ export async function runAgentReview( } } -export async function runThreadReplies( - repoDir: string, - prUrl: string, - threads: ReviewThread[], - cursorApiKey: string, -): Promise { - if (threads.length === 0) return []; - - const result = await promptAgent( - buildThreadReplyPrompt(prUrl, threads), - repoDir, - cursorApiKey, - ); - - if (result.status !== "finished") { - throw new Error(`Thread reply agent ${result.status}: ${result.id}`); - } - - const raw = result.result?.trim(); - if (!raw) { - throw new Error("Agent returned empty thread replies"); - } - - try { - const decisions = parseThreadReplyResult(raw); - validateThreadReplyDecisions( - decisions, - threads.map((thread) => thread.id), - ); - return decisions; - } catch (err) { - console.error("Could not parse thread reply JSON:"); - console.log(raw); - throw err; - } -} - -async function promptAgent( +/** + * Shared local Cursor SDK prompt entry. Features outside this module may call + * this; they own their own prompts and result parsing. + */ +export async function promptLocalAgent( prompt: string, repoDir: string, cursorApiKey: string, diff --git a/src/review/payload.test.ts b/src/review/payload.test.ts deleted file mode 100644 index 67ed832..0000000 --- a/src/review/payload.test.ts +++ /dev/null @@ -1,20 +0,0 @@ -import assert from "node:assert/strict"; -import test from "node:test"; -import { buildGithubReview } from "./payload.js"; - -test("initial review text cannot forge thread control markers", () => { - const review = buildGithubReview({ - summary: "Summary ", - findings: [ - { - severity: "high", - path: "src/example.ts", - line: 12, - body: "Finding ", - }, - ], - }); - - assert.equal(review.body.includes("glados:"), false); - assert.equal(review.comments[0]?.body.includes("glados:"), false); -}); diff --git a/src/review/payload.ts b/src/review/payload.ts index 42d34df..ab0dca8 100644 --- a/src/review/payload.ts +++ b/src/review/payload.ts @@ -28,22 +28,13 @@ export function applyPersonality(text: string): string { return text; } -/** Remove markers reserved for controlled review-thread state. */ -export function stripGladosControlMarkers(text: string): string { - return text - .replace(//gi, "") - .trim(); -} - -function applySafePersonality(text: string): string { - return stripGladosControlMarkers( - applyPersonality(stripGladosControlMarkers(text)), - ); -} - +/** + * Build the full-review agent prompt. + * `extraContext` is an optional appendix from other features (no semantics here). + */ export function buildReviewPrompt( prUrl: string, - settledContext = "", + extraContext = "", ): string { return [ `Review pull request ${prUrl}.`, @@ -59,8 +50,8 @@ export function buildReviewPrompt( "If the change looks good, return an empty findings array.", "If the overall change is very low quality, end the summary with a short GLaDOS-style insult. Otherwise do not.", "", - ...(settledContext - ? [settledContext, ""] + ...(extraContext + ? [extraContext, ""] : []), "Vibe:", "110% over-the-top roleplay: always sound like GlaDOS from Portal conducting tests and doing sarcastic remarks, Absolute immersion into the world of video game Portal.", @@ -138,13 +129,13 @@ export function buildGithubReview(payload: ReviewPayload): { ? "REQUEST_CHANGES" : "APPROVE"; - let body = applySafePersonality(payload.summary); + let body = applyPersonality(payload.summary); if (unanchored.length > 0) { body += "\n\n### Additional findings\n"; for (const finding of unanchored) { const prefix = finding.path ? `\`${finding.path}\`: ` : ""; - body += `\n- **[${finding.severity.toUpperCase()}]** ${prefix}${applySafePersonality(finding.body)}`; + body += `\n- **[${finding.severity.toUpperCase()}]** ${prefix}${applyPersonality(finding.body)}`; } } @@ -152,7 +143,7 @@ export function buildGithubReview(payload: ReviewPayload): { path: finding.path, line: finding.line!, side: "RIGHT" as const, - body: applySafePersonality( + body: applyPersonality( `**[${finding.severity.toUpperCase()}]** ${finding.body}`, ), })); diff --git a/src/review/process.ts b/src/review/process.ts index a5847d9..4445e92 100644 --- a/src/review/process.ts +++ b/src/review/process.ts @@ -5,13 +5,12 @@ import { postGithubReview } from "../github/reviews.js"; import type { PullRequestRef } from "../types.js"; import { runAgentReview } from "./agent.js"; import { buildGithubReview } from "./payload.js"; -import { processReviewThreads } from "./thread-process.js"; -import { - formatSettledContext, - suppressSettledFindings, -} from "./threads.js"; -/** Run Phase A thread handling, then the requested full PR review. */ +/** + * Core full PR review. No thread-reply knowledge. + * CLI uses `processPrReviewWithThreadReplies` from `thread-replies/` when that + * feature is enabled; call this directly to run reviews without it. + */ export async function processPrReview( pr: PullRequestRef, options: { githubToken: string; cursorApiKey: string }, @@ -31,29 +30,13 @@ export async function processPrReview( pr.prNumber, options.githubToken, async (repoDir) => { - const threadResult = await processReviewThreads(pr, { - ...options, - repoDir, - }); - console.log(` Reviewing ${pr.prUrl} ...`); const payload = await runAgentReview( repoDir, pr.prUrl, options.cursorApiKey, - formatSettledContext(threadResult.settled), ); - const filtered = suppressSettledFindings( - payload, - threadResult.settled, - ); - const suppressed = - payload.findings.length - filtered.findings.length; - if (suppressed > 0) { - console.log(` Suppressed ${suppressed} settled finding(s)`); - } - - const review = buildGithubReview(filtered); + const review = buildGithubReview(payload); console.log(` Verdict: ${review.event}`); console.log(` ${review.comments.length} inline comment(s)`); await postGithubReview(options.githubToken, pr, review); @@ -67,7 +50,7 @@ export async function processPrReview( } } -function logReviewError(err: unknown): void { +export function logReviewError(err: unknown): void { if (err instanceof CursorAgentError) { console.error(` Review startup failed: ${err.message}`); } else if (err instanceof Error) { diff --git a/src/thread-replies/agent.ts b/src/thread-replies/agent.ts new file mode 100644 index 0000000..e7ffba6 --- /dev/null +++ b/src/thread-replies/agent.ts @@ -0,0 +1,45 @@ +import type { ReviewThread } from "../github/threads.js"; +import { promptLocalAgent } from "../review/agent.js"; +import { + buildThreadReplyPrompt, + parseThreadReplyResult, + validateThreadReplyDecisions, + type ThreadReplyDecision, +} from "./logic.js"; + +export async function runThreadReplies( + repoDir: string, + prUrl: string, + threads: ReviewThread[], + cursorApiKey: string, +): Promise { + if (threads.length === 0) return []; + + const result = await promptLocalAgent( + buildThreadReplyPrompt(prUrl, threads), + repoDir, + cursorApiKey, + ); + + if (result.status !== "finished") { + throw new Error(`Thread reply agent ${result.status}: ${result.id}`); + } + + const raw = result.result?.trim(); + if (!raw) { + throw new Error("Agent returned empty thread replies"); + } + + try { + const decisions = parseThreadReplyResult(raw); + validateThreadReplyDecisions( + decisions, + threads.map((thread) => thread.id), + ); + return decisions; + } catch (err) { + console.error("Could not parse thread reply JSON:"); + console.log(raw); + throw err; + } +} diff --git a/src/thread-replies/index.ts b/src/thread-replies/index.ts new file mode 100644 index 0000000..198a2af --- /dev/null +++ b/src/thread-replies/index.ts @@ -0,0 +1,11 @@ +/** + * Review-thread replies feature (Phase A + compose with full review). + * + * Public surface — import only from here outside this folder. + * To disable or replace: swap CLI calls and delete `src/thread-replies/` + * (plus `src/github/threads.ts` if nothing else uses it). + */ +export { + processPrReviewWithThreadReplies, + processPrThreadReplies, +} from "./process.js"; diff --git a/src/review/threads.test.ts b/src/thread-replies/logic.test.ts similarity index 93% rename from src/review/threads.test.ts rename to src/thread-replies/logic.test.ts index 61de2c2..7342e86 100644 --- a/src/review/threads.test.ts +++ b/src/thread-replies/logic.test.ts @@ -5,7 +5,7 @@ import type { ReviewThreadComment, } from "../github/threads.js"; import { sortReviewThreadComments } from "../github/threads.js"; -import type { ReviewPayload } from "./payload.js"; +import { buildGithubReview, type ReviewPayload } from "../review/payload.js"; import { AGREE_MARKER, assertCurrentThreadSnapshots, @@ -17,10 +17,11 @@ import { isSettledThread, needsResolveRetry, parseThreadReplyResult, + sanitizeReviewForPost, suppressSettledFindings, validateThreadReplyDecisions, type SettledFinding, -} from "./threads.js"; +} from "./logic.js"; const glados = "glados"; @@ -344,3 +345,22 @@ test("thread comments are encoded as untrusted prompt data", () => { assert.equal(prompt.includes(injection), false); assert.match(prompt, /\\u003c\/thread_data\\u003e/); }); + +test("composed review sanitizes forged thread control markers before post", () => { + const review = sanitizeReviewForPost( + buildGithubReview({ + summary: "Summary ", + findings: [ + { + severity: "high", + path: "src/example.ts", + line: 12, + body: "Finding ", + }, + ], + }), + ); + + assert.equal(review.body.includes("glados:"), false); + assert.equal(review.comments[0]?.body.includes("glados:"), false); +}); diff --git a/src/review/threads.ts b/src/thread-replies/logic.ts similarity index 89% rename from src/review/threads.ts rename to src/thread-replies/logic.ts index 764c058..e71d126 100644 --- a/src/review/threads.ts +++ b/src/thread-replies/logic.ts @@ -1,12 +1,12 @@ import type { ReviewThread } from "../github/threads.js"; import { applyPersonality, - stripGladosControlMarkers, type ReviewPayload, -} from "./payload.js"; +} from "../review/payload.js"; export const AGREE_MARKER = ""; const REPLY_TO_MARKER = //; +const CONTROL_MARKER_RE = //gi; export interface SettledFinding { threadId: string; @@ -22,6 +22,32 @@ export interface ThreadReplyDecision { body: string; } +/** Remove markers reserved for controlled review-thread state. */ +export function stripGladosControlMarkers(text: string): string { + return text.replace(CONTROL_MARKER_RE, "").trim(); +} + +/** + * Strip forged control markers from a review before posting. + * Call this from the thread-replies composer only — core review does not + * know about these markers. + */ +export function sanitizeReviewForPost(review: { + event: "APPROVE" | "REQUEST_CHANGES"; + body: string; + comments: Array<{ path: string; line: number; side: "RIGHT"; body: string }>; + unanchored: ReviewPayload["findings"]; +}): typeof review { + return { + ...review, + body: stripGladosControlMarkers(review.body), + comments: review.comments.map((comment) => ({ + ...comment, + body: stripGladosControlMarkers(comment.body), + })), + }; +} + export function hasAgreeMarker(body: string): boolean { return body.includes(AGREE_MARKER); } @@ -102,7 +128,7 @@ export function listSettledFindings( path: thread.path, line: thread.line, originalBody: root.body, - agreementBody: stripControlMarkers(agreement.body), + agreementBody: stripGladosControlMarkers(agreement.body), }); } return settled; @@ -121,8 +147,8 @@ export function formatSettledContext(settled: SettledFinding[]): string { lines.push( [ `- ${item.threadId} at \`${loc}\``, - ` - Original finding: ${stripControlMarkers(item.originalBody)}`, - ` - Why it was settled: ${stripControlMarkers(item.agreementBody)}`, + ` - Original finding: ${stripGladosControlMarkers(item.originalBody)}`, + ` - Why it was settled: ${stripGladosControlMarkers(item.agreementBody)}`, ].join("\n"), ); } @@ -141,7 +167,7 @@ export function buildThreadReplyPrompt( comments: thread.comments.map((comment) => ({ author: comment.authorLogin, createdAt: comment.createdAt, - body: stripControlMarkers(comment.body), + body: stripGladosControlMarkers(comment.body), })), })); const encodedThreadData = encodeUntrustedPromptData( @@ -296,8 +322,8 @@ export function formatReplyBody( body: string, replyToCommentId: string, ): string { - const text = stripControlMarkers( - applyPersonality(stripControlMarkers(body)), + const text = stripGladosControlMarkers( + applyPersonality(stripGladosControlMarkers(body)), ); const replyMarker = ``; if (decision === "agree") { @@ -306,10 +332,6 @@ export function formatReplyBody( return `${text}\n\n${replyMarker}`; } -function stripControlMarkers(body: string): string { - return stripGladosControlMarkers(body); -} - function findAgreementComment( thread: ReviewThread, gladosLogin: string, @@ -349,7 +371,7 @@ function decodeReplyId(value: string): string | undefined { } function normalizeFindingBody(body: string): string { - return stripControlMarkers(body) + return stripGladosControlMarkers(body) .replace(/^\s*\*\*\[[A-Z]+\]\*\*\s*/i, "") .replace(/\s+/g, " ") .trim() diff --git a/src/review/thread-process.ts b/src/thread-replies/process.ts similarity index 76% rename from src/review/thread-process.ts rename to src/thread-replies/process.ts index 0bee702..94ff5f2 100644 --- a/src/review/thread-process.ts +++ b/src/thread-replies/process.ts @@ -8,17 +8,25 @@ import { resolveReviewThread, type ReviewThread, } from "../github/threads.js"; +import { isReviewRequestedForUser } from "../github/pr.js"; +import { postGithubReview } from "../github/reviews.js"; +import { runAgentReview } from "../review/agent.js"; +import { buildGithubReview } from "../review/payload.js"; +import { logReviewError } from "../review/process.js"; import type { PullRequestRef } from "../types.js"; import { runThreadReplies } from "./agent.js"; import { assertCurrentThreadSnapshots, formatReplyBody, + formatSettledContext, isAwaitingThread, isResolutionPending, listSettledFindings, needsResolveRetry, + sanitizeReviewForPost, + suppressSettledFindings, type SettledFinding, -} from "./threads.js"; +} from "./logic.js"; export interface ThreadProcessResult { settled: SettledFinding[]; @@ -36,6 +44,66 @@ interface ThreadOptions { repoDir?: string; } +/** + * Full review with Phase A first: thread replies, then core review with + * settled-finding suppression. This is the feature entrypoint for + * review-requested PRs — swap for `processPrReview` to disable the feature. + */ +export async function processPrReviewWithThreadReplies( + pr: PullRequestRef, + options: { githubToken: string; cursorApiKey: string }, +): Promise { + try { + return await withPrLock(pr.owner, pr.repo, pr.prNumber, async () => { + const requested = await isReviewRequestedForUser(options.githubToken, pr); + if (!requested) { + console.log(" Skipping: review not requested for this user"); + return true; + } + + console.log(` Cloning ${pr.owner}/${pr.repo}...`); + return withPrWorkspace( + pr.owner, + pr.repo, + pr.prNumber, + options.githubToken, + async (repoDir) => { + const threadResult = await processReviewThreads(pr, { + ...options, + repoDir, + }); + + console.log(` Reviewing ${pr.prUrl} ...`); + const payload = await runAgentReview( + repoDir, + pr.prUrl, + options.cursorApiKey, + formatSettledContext(threadResult.settled), + ); + const filtered = suppressSettledFindings( + payload, + threadResult.settled, + ); + const suppressed = + payload.findings.length - filtered.findings.length; + if (suppressed > 0) { + console.log(` Suppressed ${suppressed} settled finding(s)`); + } + + const review = sanitizeReviewForPost(buildGithubReview(filtered)); + console.log(` Verdict: ${review.event}`); + console.log(` ${review.comments.length} inline comment(s)`); + await postGithubReview(options.githubToken, pr, review); + return true; + }, + ); + }); + } catch (err) { + logReviewError(err); + return false; + } +} + /** Phase A against an existing checkout. */ export async function processReviewThreads( pr: PullRequestRef,