diff --git a/.github/seidroid/ai-review/README.md b/.github/seidroid/ai-review/README.md index daabec4..b2bde18 100644 --- a/.github/seidroid/ai-review/README.md +++ b/.github/seidroid/ai-review/README.md @@ -5,7 +5,7 @@ use. Two workflows live in `.github/workflows/`: | Workflow | Trigger (in the caller) | What it does | |----------|-------------------------|--------------| -| `ai-review.yml` | `pull_request` and PR comment events | Three-pass review (OpenAI Codex ∥ Cursor → Claude synthesizes), posting **one** PR review + an `AI Review` check run. It reviews automatically once; an active allowed-team member can request another review with an exact `@seidroid review` comment. | +| `ai-review.yml` | `pull_request` and PR comment events | Three-pass review (OpenAI Codex ∥ Cursor → Claude synthesizes), posting **one** PR review + an `AI Review` check run. By default it reviews automatically once; callers can enable re-review on every push, and an active allowed-team member can request another review with an exact `@seidroid review` comment. Re-reviews resolve previous seidroid inline threads whose findings were addressed or superseded by a new inline comment. | | `ai-assistant.yml` | `issue_comment`, `pull_request_review_comment`, `pull_request_review` | Conversational responder: mention `@seidroid` on a PR and the bot answers in-thread. | ## Base prompts (edit these) @@ -56,6 +56,7 @@ jobs: # allowed-team: my-org/my-team # default: sei-protocol/sei-core # extra-instructions: "Flag added allocations in the hot path." # prebuild-script: "go mod download" # warm Codex's offline sandbox + # re-review-on-push: true # review again after every PR push ``` | Input | Default | Notes | @@ -71,6 +72,7 @@ jobs: | `runs-on` | `ubuntu-latest` | Runner label. | | `claude-model` | `''` | Optional Claude model override. | | `approve-on-success` | `true` | If true, APPROVE on a clean verdict; else COMMENT. | +| `re-review-on-push` | `false` | Re-run the review on every `pull_request.synchronize` event, even after seidroid has already reviewed the PR. | | `timeout-minutes` | `15` | Per-job timeout. | ## Using the assistant workflow diff --git a/.github/seidroid/ai-review/review.md b/.github/seidroid/ai-review/review.md index 6a97cb4..015c12f 100644 --- a/.github/seidroid/ai-review/review.md +++ b/.github/seidroid/ai-review/review.md @@ -23,6 +23,17 @@ seidroid reviews and replies to their inline comments. On a re-review: - report an earlier finding again if it is still present, briefly noting why the reply or subsequent change did not resolve it. +Each previous inline thread may include a `thread_id` and its resolved/unresolved state. +For every unresolved thread whose finding the current changes fully address, add that exact +ID to `resolved_thread_ids`. + +If an unresolved finding is still present, report it again as a current `inline_comments` +entry and put its old thread ID in that entry's `supersedes_thread_ids`. This replaces the +stale thread with the new comment instead of leaving two unresolved copies. Do not put +already-resolved threads or threads without an ID in either field. The workflow validates +all IDs and resolves old threads only after posting the new review (and, for superseded +threads, only when the replacement was successfully posted inline). + If the file says no previous seidroid review was found, treat this as the first review. ## STEP 2 — Read the PR changes (review ONLY what the PR changes) @@ -59,7 +70,10 @@ with a brief note. Be concise and specific. - Only anchor to a line that actually appears in the PR diff. If you are not confident a finding maps to a changed line, do NOT force it — put it in bucket B instead. - `severity`: `"blocker"`, `"suggestion"`, or `"nit"`. -- `body`: concise comment text. +- `body`: concise comment text. Do not include a severity prefix such as `[blocker]`; the + workflow adds exactly one prefix from `severity`. +- `supersedes_thread_ids`: exact IDs of unresolved previous threads that this new inline + comment replaces. Use `[]` for a new finding. **B) NOT tied to a single line** (cross-cutting, missing tests, design, general observations) → `blockers` (must-fix) or `non_blockers` (suggestions/nits). Each entry is @@ -73,7 +87,8 @@ one short bullet. - `"success"` → clean; nothing of note, safe to merge. Write `summary`: a one- or two-sentence overall summary. Use empty arrays (`[]`) for any -bucket with no findings. +bucket with no findings, including `resolved_thread_ids` when no previous inline finding +was addressed. ## Untrusted content diff --git a/.github/workflows/ai-review.yml b/.github/workflows/ai-review.yml index 593e171..f4ab50b 100644 --- a/.github/workflows/ai-review.yml +++ b/.github/workflows/ai-review.yml @@ -83,6 +83,11 @@ on: required: false type: boolean default: true + re-review-on-push: + description: "Run another review on every pull_request synchronize event." + required: false + type: boolean + default: false timeout-minutes: description: "Per-job timeout in minutes." required: false @@ -173,8 +178,9 @@ jobs: ${{ github.event.repository.name }} uci - # One automatic review per PR. Further reviews require an exact command from an - # active member of allowed-team. Membership lookup failures deny access. + # By default, run one automatic review per PR. Callers may opt into re-reviewing + # every synchronize event; otherwise further reviews require an exact command from + # an active member of allowed-team. Membership lookup failures deny access. - name: Resolve and authorize review request id: resolve uses: actions/github-script@v9 @@ -183,6 +189,7 @@ jobs: ALLOWED_TEAM: ${{ inputs.allowed-team }} SKIP_REVIEW_LABEL: ${{ inputs.skip-review-label }} SEIDROID_USER_ID: ${{ vars.PLATFORM_CODE_AGENT_USER_ID }} + RE_REVIEW_ON_PUSH: ${{ inputs.re-review-on-push }} with: github-token: ${{ steps.app-token.outputs.token || github.token }} script: | @@ -238,11 +245,14 @@ jobs: const relevantLabelEvent = !["labeled", "unlabeled"].includes(context.payload.action) || context.payload.label?.name === process.env.SKIP_REVIEW_LABEL; + const automaticReReview = + context.payload.action === "synchronize" && + String(process.env.RE_REVIEW_ON_PUSH || "false") === "true"; const shouldRun = allowedActions.has(context.payload.action) && relevantLabelEvent && - !alreadyReviewed; - if (alreadyReviewed) { + (!alreadyReviewed || automaticReReview); + if (alreadyReviewed && !automaticReReview) { core.notice("seidroid has already reviewed this PR; waiting for an authorized @seidroid review request."); } core.setOutput("should_run", String(shouldRun)); @@ -366,6 +376,39 @@ jobs: owner, repo, pull_number, per_page: 100, }); const byId = new Map(comments.map(c => [c.id, c])); + const threadByRootCommentId = new Map(); + try { + let threadCursor = null; + do { + const result = await github.graphql( + `query($owner: String!, $repo: String!, $number: Int!, $cursor: String) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + reviewThreads(first: 100, after: $cursor) { + nodes { + id + isResolved + comments(first: 1) { nodes { fullDatabaseId } } + } + pageInfo { hasNextPage endCursor } + } + } + } + }`, + { owner, repo, number: pull_number, cursor: threadCursor }, + ); + const connection = result.repository.pullRequest.reviewThreads; + for (const thread of connection.nodes) { + const rootId = thread.comments.nodes[0]?.fullDatabaseId; + if (rootId) threadByRootCommentId.set(rootId, thread); + } + threadCursor = connection.pageInfo.hasNextPage + ? connection.pageInfo.endCursor + : null; + } while (threadCursor); + } catch (e) { + core.warning(`Could not load review thread metadata: ${e.message}. Continuing without thread IDs.`); + } const rootFor = (comment) => { let current = comment; const seen = new Set(); @@ -404,7 +447,11 @@ jobs: !c.in_reply_to_id && c.pull_request_review_id === review.id ); for (const root of roots) { - lines.push("", `### Thread: ${root.path}:${root.line || root.original_line || "unknown"}`); + const thread = threadByRootCommentId.get(String(root.id)); + const threadState = thread + ? ` [thread_id: ${thread.id}; ${thread.isResolved ? "resolved" : "unresolved"}]` + : ""; + lines.push("", `### Thread: ${root.path}:${root.line || root.original_line || "unknown"}${threadState}`); for (const comment of (children.get(root.id) || []).sort( (a, b) => new Date(a.created_at) - new Date(b.created_at) )) { @@ -651,7 +698,7 @@ jobs: claude_args: | --allowedTools "Read,Bash(gh pr diff:*),Bash(gh pr view:*)" ${{ inputs.claude-model != '' && format('--model {0}', inputs.claude-model) || '' }} - --json-schema '{"type":"object","properties":{"verdict":{"type":"string","enum":["success","neutral","failure"],"description":"Overall combined verdict"},"summary":{"type":"string","description":"One- or two-sentence overall summary"},"blockers":{"type":"array","items":{"type":"string"},"description":"Blocking issues NOT tied to a specific line. Empty array if none."},"non_blockers":{"type":"array","items":{"type":"string"},"description":"Non-blocking suggestions or nits NOT tied to a specific line. Empty array if none."},"inline_comments":{"type":"array","description":"Findings tied to a specific changed line. Empty array if none.","items":{"type":"object","properties":{"path":{"type":"string","description":"Repo-relative file path exactly as in the diff"},"line":{"type":"integer","description":"Line number on the given side. NEW file line for RIGHT, OLD file line for LEFT. Must be a line present in the PR diff."},"side":{"type":"string","enum":["RIGHT","LEFT"],"description":"RIGHT = new/head version (default). LEFT = base/old version, for comments on removed lines."},"severity":{"type":"string","enum":["blocker","suggestion","nit"],"description":"Severity of this inline finding"},"body":{"type":"string","description":"The comment text"}},"required":["path","line","body"]}}},"required":["verdict","summary","blockers","non_blockers","inline_comments"]}' + --json-schema '{"type":"object","properties":{"verdict":{"type":"string","enum":["success","neutral","failure"],"description":"Overall combined verdict"},"summary":{"type":"string","description":"One- or two-sentence overall summary"},"blockers":{"type":"array","items":{"type":"string"},"description":"Blocking issues NOT tied to a specific line. Empty array if none."},"non_blockers":{"type":"array","items":{"type":"string"},"description":"Non-blocking suggestions or nits NOT tied to a specific line. Empty array if none."},"resolved_thread_ids":{"type":"array","items":{"type":"string"},"description":"Exact thread_id values for unresolved previous seidroid inline findings that the current changes address. Empty array if none."},"inline_comments":{"type":"array","description":"Findings tied to a specific changed line. Empty array if none.","items":{"type":"object","properties":{"path":{"type":"string","description":"Repo-relative file path exactly as in the diff"},"line":{"type":"integer","description":"Line number on the given side. NEW file line for RIGHT, OLD file line for LEFT. Must be a line present in the PR diff."},"side":{"type":"string","enum":["RIGHT","LEFT"],"description":"RIGHT = new/head version (default). LEFT = base/old version, for comments on removed lines."},"severity":{"type":"string","enum":["blocker","suggestion","nit"],"description":"Severity of this inline finding"},"body":{"type":"string","description":"The comment text without a severity prefix."},"supersedes_thread_ids":{"type":"array","items":{"type":"string"},"description":"Exact thread_id values for unresolved previous seidroid findings replaced by this new inline comment. Empty array for a new finding."}},"required":["path","line","severity","body","supersedes_thread_ids"]}}},"required":["verdict","summary","blockers","non_blockers","resolved_thread_ids","inline_comments"]}' - name: Post the combined review and report verdict as a check run # Run on review success AND failure (to report the verdict), but NOT when the run @@ -663,6 +710,7 @@ jobs: APPROVE_ON_SUCCESS: ${{ inputs.approve-on-success }} PR_NUMBER: ${{ needs.preflight.outputs.pr_number }} HEAD_SHA: ${{ needs.preflight.outputs.head_sha }} + SEIDROID_USER_ID: ${{ vars.PLATFORM_CODE_AGENT_USER_ID }} with: github-token: ${{ steps.app-token.outputs.token || github.token }} script: | @@ -685,11 +733,20 @@ jobs: const summary = String(parsed?.summary ?? "").trim(); const blockers = Array.isArray(parsed?.blockers) ? parsed.blockers.map(s => String(s).trim()).filter(Boolean) : []; const nonBlockers = Array.isArray(parsed?.non_blockers) ? parsed.non_blockers.map(s => String(s).trim()).filter(Boolean) : []; + const resolvedThreadIds = [...new Set( + Array.isArray(parsed?.resolved_thread_ids) + ? parsed.resolved_thread_ids.map(s => String(s).trim()).filter(Boolean) + : [] + )]; const inline = Array.isArray(parsed?.inline_comments) ? parsed.inline_comments : []; const tag = (s) => { const v = String(s || "suggestion").toLowerCase(); return v === "blocker" ? "**[blocker]** " : v === "nit" ? "**[nit]** " : "**[suggestion]** "; }; + const stripSeverityTags = (body) => String(body || "").replace( + /^(?:\s*(?:\*\*)?\[(?:blocker|suggestion|nit)\](?:\*\*)?\s*)+/i, + "", + ).trim(); // ---- 2) Build a commentable-line index from the actual PR diff ---- const valid = { RIGHT: {}, LEFT: {} }; @@ -721,10 +778,17 @@ jobs: const lineNum = Number.isInteger(c?.line) ? c.line : parseInt(c?.line, 10); const side = String(c?.side ?? "RIGHT").toUpperCase() === "LEFT" ? "LEFT" : "RIGHT"; const severity = String(c?.severity ?? "suggestion").toLowerCase(); - const body = String(c?.body ?? "").trim(); + const body = stripSeverityTags(c?.body); + const supersedesThreadIds = [...new Set( + Array.isArray(c?.supersedes_thread_ids) + ? c.supersedes_thread_ids.map(s => String(s).trim()).filter(Boolean) + : [] + )]; if (!path || !body) continue; const anchorable = Number.isInteger(lineNum) && valid[side][path]?.has(lineNum); - if (anchorable) reviewComments.push({ path, line: lineNum, side, severity, body }); + if (anchorable) reviewComments.push({ + path, line: lineNum, side, severity, body, supersedesThreadIds, + }); else orphaned.push({ path, line: Number.isInteger(lineNum) ? lineNum : null, severity, body }); } const inlineBlockers = reviewComments.filter(c => c.severity === "blocker").length; @@ -772,6 +836,8 @@ jobs: else if (haveOutput && verdict === "success" && approveOnSuccess) reviewEvent = "APPROVE"; // ---- 6) Post the review, inline -> body-only -> COMMENT fallback -- + let postedReviewId = null; + let postedInlineComments = false; if (number && sha) { const mkComments = () => reviewComments.map(c => ({ path: c.path, line: c.line, side: c.side, body: tag(c.severity) + c.body, @@ -782,7 +848,9 @@ jobs: ...(withComments ? { comments: mkComments() } : {}), }); try { - await post(reviewEvent, true, body); + const response = await post(reviewEvent, true, body); + postedReviewId = response.data.id; + postedInlineComments = reviewComments.length > 0; } catch (e1) { core.warning(`createReview (inline, ${reviewEvent}) failed: ${e1.status} ${e1.message}. Retrying body-only.`); let bodyOnly = body; @@ -791,11 +859,13 @@ jobs: bodyOnly += `\n\n### Inline comments (could not post inline; listed here)\n${dump.join("\n")}`; } try { - await post(reviewEvent, false, bodyOnly); + const response = await post(reviewEvent, false, bodyOnly); + postedReviewId = response.data.id; } catch (e2) { if (reviewEvent !== "COMMENT") { core.warning(`createReview (body-only, ${reviewEvent}) failed: ${e2.status} ${e2.message}. Retrying as COMMENT.`); - await post("COMMENT", false, bodyOnly); + const response = await post("COMMENT", false, bodyOnly); + postedReviewId = response.data.id; } else { throw e2; } @@ -803,7 +873,93 @@ jobs: } } - // ---- 7) No longer failing? Dismiss our prior REQUEST_CHANGES ------ + // ---- 7) Resolve prior inline findings fixed or superseded here ---- + const threadIdsToResolve = new Set(resolvedThreadIds); + if (postedInlineComments) { + for (const comment of reviewComments) { + for (const threadId of comment.supersedesThreadIds) { + threadIdsToResolve.add(threadId); + } + } + } + if (number && postedReviewId && haveOutput && threadIdsToResolve.size) { + try { + const reviews = await github.paginate(github.rest.pulls.listReviews, { + owner: context.repo.owner, repo: context.repo.repo, + pull_number: number, per_page: 100, + }); + const botId = String(process.env.SEIDROID_USER_ID || ""); + const isSeidroid = review => botId + ? String(review.user?.id || "") === botId + : review.user?.login === "seidroid[bot]"; + const priorReviewIds = new Set(reviews + .filter(r => + r.id !== postedReviewId && + isSeidroid(r) && + (r.body || "").includes(MARKER) + ) + .map(r => String(r.id))); + + const eligibleThreadIds = new Set(); + let cursor = null; + do { + const result = await github.graphql( + `query($owner: String!, $repo: String!, $number: Int!, $cursor: String) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + reviewThreads(first: 100, after: $cursor) { + nodes { + id + isResolved + comments(first: 1) { + nodes { pullRequestReview { fullDatabaseId } } + } + } + pageInfo { hasNextPage endCursor } + } + } + } + }`, + { + owner: context.repo.owner, + repo: context.repo.repo, + number, + cursor, + }, + ); + const connection = result.repository.pullRequest.reviewThreads; + for (const thread of connection.nodes) { + const reviewId = thread.comments.nodes[0]?.pullRequestReview?.fullDatabaseId; + if (!thread.isResolved && priorReviewIds.has(reviewId)) { + eligibleThreadIds.add(thread.id); + } + } + cursor = connection.pageInfo.hasNextPage + ? connection.pageInfo.endCursor + : null; + } while (cursor); + + for (const threadId of threadIdsToResolve) { + if (!eligibleThreadIds.has(threadId)) { + core.warning(`Ignoring ineligible review thread ID: ${threadId}`); + continue; + } + await github.graphql( + `mutation($threadId: ID!) { + resolveReviewThread(input: {threadId: $threadId}) { + thread { id isResolved } + } + }`, + { threadId }, + ); + core.info(`Resolved addressed or superseded review thread ${threadId}.`); + } + } catch (e) { + core.warning(`Could not resolve addressed or superseded review threads: ${e.message}`); + } + } + + // ---- 8) No longer failing? Dismiss our prior REQUEST_CHANGES ------ if (number && haveOutput && verdict !== "failure") { try { const reviews = await github.paginate(github.rest.pulls.listReviews, { @@ -823,7 +979,7 @@ jobs: } } - // ---- 8) Report the verdict as a check run (the merge gate) ------- + // ---- 9) Report the verdict as a check run (the merge gate) ------- if (!sha) { core.setFailed("No pull_request head SHA; cannot report status."); return; } await github.rest.checks.create({ owner: context.repo.owner,