diff --git a/.github/workflows/codex-autofix-review-comments.yml b/.github/workflows/codex-autofix-review-comments.yml index 89602d6f5..74be36b8d 100644 --- a/.github/workflows/codex-autofix-review-comments.yml +++ b/.github/workflows/codex-autofix-review-comments.yml @@ -8,7 +8,7 @@ permissions: contents: read issues: write models: read - pull-requests: read + pull-requests: write jobs: request-codex-autoresolve: @@ -35,6 +35,7 @@ jobs: "chatgpt-codex-connector[bot]", ]); const scopedResolveCommand = "@codex resolve actionable Codex review findings for this pull request and current head"; + const resolvedDispositionMarker = ""; if ( reviewComment.user?.type !== "Bot" || @@ -45,7 +46,80 @@ jobs: } if (reviewComment.in_reply_to_id) { - core.notice("Skipping Codex auto-resolve request because the comment is a review-thread reply."); + const replyBody = (reviewComment.body || "").trimStart(); + if (!replyBody.startsWith(resolvedDispositionMarker)) { + core.notice("Skipping review-thread reply without the trusted resolved disposition marker."); + return; + } + + const owner = context.repo.owner; + const repo = context.repo.repo; + const issue_number = pr.number; + const targetCommentIds = new Set([reviewComment.id, reviewComment.in_reply_to_id]); + const reviewThreadsQuery = ` + query ReviewThreads($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: 100) { + nodes { databaseId } + } + } + pageInfo { + endCursor + hasNextPage + } + } + } + } + } + `; + + let cursor = null; + let matchingThread; + + try { + do { + const result = await github.graphql(reviewThreadsQuery, { + owner, + repo, + number: issue_number, + cursor, + }); + const connection = result.repository?.pullRequest?.reviewThreads; + matchingThread = connection?.nodes?.find((thread) => + thread.comments.nodes.some((comment) => targetCommentIds.has(comment.databaseId)), + ); + cursor = connection?.pageInfo?.hasNextPage ? connection.pageInfo.endCursor : null; + } while (!matchingThread && cursor); + + if (!matchingThread) { + core.setFailed("Codex auto-resolve could not find the review thread for the disposition reply."); + return; + } + + if (matchingThread.isResolved) { + core.notice("The Codex review thread was already resolved."); + return; + } + + await github.graphql( + `mutation ResolveReviewThread($threadId: ID!) { + resolveReviewThread(input: { threadId: $threadId }) { + thread { id isResolved } + } + }`, + { threadId: matchingThread.id }, + ); + core.notice("Resolved the Codex review thread after its explicit disposition reply."); + } catch (error) { + const message = `Codex auto-resolve failed while closing the review thread: ${error.message || error}`; + core.warning(message); + core.setFailed(message); + } return; } @@ -61,8 +135,8 @@ jobs: const owner = context.repo.owner; const repo = context.repo.repo; const issue_number = pr.number; - const marker = ``; - const maxAutoResolveRequests = 3; + const marker = ``; + const maxAutoResolveRequests = 1; let existingComments; try { @@ -94,19 +168,20 @@ jobs: ); if (hasTrustedExistingRequest) { - core.notice("Skipping duplicate Codex auto-resolve request for this pull request head."); + core.notice("Skipping duplicate Codex auto-resolve request for this pull request."); return; } if (trustedExistingRequests.length >= maxAutoResolveRequests) { - core.warning(`Skipping Codex auto-resolve request because this pull request reached the automatic repair limit of ${maxAutoResolveRequests}.`); + core.warning("Skipping Codex auto-resolve request because this pull request already used its single automatic repair pass."); return; } const prompt = [ marker, + ``, ``, - `${scopedResolveCommand} using the repository instructions. Always fix P0 and P1 findings. For P2 and lower findings, decide whether each is worth fixing automatically. Fix clear, scoped, low-risk issues with the best minimal change; otherwise reply explaining why the issue is deferred or not actionable. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit unless explicitly asked. After each fix or decision, resolve the review conversation if supported. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation.`, + `${scopedResolveCommand} using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. After fixing or dispositioning a thread, reply in that thread with ${resolvedDispositionMarker} as the first line, followed by a concise summary; that marker authorizes the workflow to close that exact thread. If human input or new authorization is required, do not use the marker and leave the thread open with the blocker. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation.`, ].join("\n\n"); try { diff --git a/AGENTS.md b/AGENTS.md index 5c8c100f1..300d9c801 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -323,9 +323,11 @@ These instructions apply to Codex GitHub pull request reviews and Codex tasks st - Prioritize high-confidence findings that affect correctness, security, privacy, data loss, auth/permissions, migrations, API contracts, production reliability, clinical behavior, source governance, or user-facing behavior. - Do not comment on formatting, naming, style, minor cleanup, or speculative refactors unless they create a real bug or maintainability risk. - Prefer fewer, stronger findings over exhaustive low-value review comments. +- An automatic review may emit at most three inline findings total. Use inline comments only for P0/P1 issues; put non-blocking P2 context in one summary and omit P3 feedback. - A finding must cite concrete changed code and explain the failure mode. - Do not suggest broad rewrites during review. Recommend the smallest change that resolves the issue. - Do not propose or start fixes unless explicitly asked with an `@codex fix...` or `@codex resolve...` comment, or when the repository's Codex auto-resolve workflow posts that command. +- Treat automatic review as single-pass per pull request. Do not re-review a later head, repeat a prior finding, or create another review during an auto-resolve task unless a human explicitly requests a fresh review. ### Severity calibration @@ -355,7 +357,7 @@ If a high-risk area is touched, review more carefully for regressions, missing t ### Cost and usage control -Avoid broad repeated review passes. Do not request exhaustive review behavior unless the PR touches security, auth, data loss, migrations, billing, production reliability, clinical output, source governance, or private document access. Prefer targeted validation and targeted review comments. +Avoid broad repeated review passes. Do not request exhaustive review behavior unless the PR touches security, auth, data loss, migrations, billing, production reliability, clinical output, source governance, or private document access. Prefer targeted validation and targeted review comments. A new commit from the automatic repair task is not permission for another automatic review. ### Fix behavior @@ -381,8 +383,9 @@ When explicitly asked to fix or resolve review findings: - After fixing a P0 or P1 finding, reply with the fix summary and resolve the review conversation when supported by GitHub permissions/tooling. - After fixing an approved P2 or lower finding, reply with the fix summary and resolve the review conversation when supported. - After deciding not to fix a P2 or lower finding, reply with the reason, note whether it is deferred or not actionable, and resolve the review conversation when supported. -- Do not leave a review conversation open after it has been fixed or explicitly dispositioned. -- If GitHub permissions/tooling do not allow resolving a review conversation, leave a clear reply that says the issue is fixed or dispositioned and that the conversation is ready for a human to resolve. +- For every fixed or fully dispositioned thread, start the thread reply with ``. The workflow uses this trusted marker to close that exact thread. +- Do not use the marker when human input or new authorization is required; explain the blocker and leave that thread open. +- Do not leave a review conversation open after it has been fixed or fully dispositioned. If direct resolution is unavailable, the marker reply is the required fallback and the workflow performs the closure. ### Automatic resolve trigger @@ -392,18 +395,20 @@ Automatic Codex review is review-only by default. This repository includes `.git - Match the trusted Codex connector bot by exact login and bot type; do not use substring login checks. - Keep per-pull-request concurrency on the authorized job, not the whole workflow, so unrelated review comments cannot displace a pending Codex request. - Pin the supported Node 24-based `actions/github-script` release to its reviewed immutable commit SHA. -- The workflow must skip review-thread replies and auto-resolve request comments so Codex fix summaries do not re-trigger the workflow. -- The workflow must ask Codex to resolve only actionable Codex review findings for the triggering pull request and current head using these repository instructions. -- The workflow may request one repair per pull-request head SHA, capped at three automatic repair cycles per pull request. +- The workflow must skip auto-resolve request comments and must never turn review-thread replies into new repair requests. +- The workflow must treat unmarked review-thread replies as inert. A trusted Codex reply beginning with `` may only resolve the exact containing thread. +- The workflow must ask Codex to resolve only existing actionable Codex review findings for the triggering pull request and current head using these repository instructions; the resolve task must not perform a new review or create new findings. +- The workflow may request one automatic repair pass per pull request lifetime. Later heads require an explicit human request. - Only trust a pull-request deduplication marker when it was posted by the GitHub Actions bot. - Permission failures while reading or creating pull-request comments must fail the workflow visibly, not return a successful soft-skip. +- Grant `pull-requests: write` only to the narrow marker-driven thread-resolution workflow; retain read-only repository contents and do not approve reviews or alter code. - The workflow must not run Codex directly with API credentials. - P0 and P1 findings should always be fixed. - P2 and lower findings should be fixed only when clear, scoped, low-risk, and testable; otherwise explain the decision and resolve or mark ready for human resolution. ### Primary PR command -`@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. Always fix P0 and P1 findings. For P2 and lower findings, decide whether each is worth fixing automatically. Fix clear, scoped, low-risk issues with the best minimal change; otherwise reply explaining why the issue is deferred or not actionable. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit unless explicitly asked. After each fix or decision, resolve the review conversation if supported. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation.` +`@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. After fixing or dispositioning a thread, reply in that thread with as the first line, followed by a concise summary; that marker authorizes the workflow to close that exact thread. If human input or new authorization is required, do not use the marker and leave the thread open with the blocker. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation.` ## Cursor Cloud specific instructions diff --git a/docs/codex-review-protocol.md b/docs/codex-review-protocol.md index 41541489c..308956f9a 100644 --- a/docs/codex-review-protocol.md +++ b/docs/codex-review-protocol.md @@ -9,10 +9,12 @@ Use this protocol for every Codex review, audit, bug hunt, PR review, release-re - Review the current diff, named PR, named branch, or explicitly requested area. - Do not expand into stale branches or unrelated modules unless a confirmed defect crosses that boundary. - Before branch or PR review, check `docs/branch-review-ledger.md`: resolve the target with `git rev-parse`, compare the HEAD and scope, and skip unchanged completed reviews unless the user asks for a fresh pass. +- Treat GitHub automatic review as one pass per pull request. A repair commit or later head does not authorize another automatic pass; require an explicit human request before re-reviewing. ## Review Output - Lead with findings, ordered by severity: P0, P1, P2, then P3. +- For automatic PR reviews, emit no more than three inline findings and reserve them for P0/P1 defects. Consolidate non-blocking P2 context into one summary and omit P3 feedback. - Each finding must include file/line evidence, trigger or failure path, expected behavior, actual risk, and the smallest proof or check that would catch it. - If no high-confidence issue is found, say so clearly and name the highest residual risk area. - Include checks run, checks not run, and whether any check was skipped because it could touch an API/provider. @@ -23,6 +25,8 @@ Use this protocol for every Codex review, audit, bug hunt, PR review, release-re - For a pure review request, do not edit files, stage, commit, push, post PR comments, rerun hosted CI, or call provider-backed services. - Exception: append the completed review record to `docs/branch-review-ledger.md` so throttling state persists. - If the user clearly asks to fix confirmed findings, make the smallest safe change and verify with local, static, or mocked checks first. +- During an automatic resolve task, work only existing unresolved Codex threads. Do not start a new review, add standalone findings, or request another review. +- After fixing or fully dispositioning a thread, start the reply with ``; the workflow will close that exact thread. Do not use the marker when human input or new authorization is required, and leave that blocked thread open with a concise reason. - Ask before any OpenAI, Supabase, GitHub/GitLab, hosted CI, or provider-backed workflow. - After any completed branch/PR review, update `docs/branch-review-ledger.md` with date, branch/ref, HEAD, scope, outcome, and checks. This ledger append is allowed even during a pure review. diff --git a/scripts/check-codex-autofix-workflow.mjs b/scripts/check-codex-autofix-workflow.mjs index 4d59357d6..c593b2ec5 100644 --- a/scripts/check-codex-autofix-workflow.mjs +++ b/scripts/check-codex-autofix-workflow.mjs @@ -10,7 +10,8 @@ const reviewProtocol = fs.readFileSync(reviewProtocolPath, "utf8"); const failures = []; const githubScriptPin = "3a2844b7e9c422d3c10d287c895573f7108da1b3"; const scopedResolveCommand = "@codex resolve actionable Codex review findings for this pull request and current head"; -const scopedResolvePrompt = `\${scopedResolveCommand} using the repository instructions. Always fix P0 and P1 findings. For P2 and lower findings, decide whether each is worth fixing automatically. Fix clear, scoped, low-risk issues with the best minimal change; otherwise reply explaining why the issue is deferred or not actionable. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit unless explicitly asked. After each fix or decision, resolve the review conversation if supported. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation.`; +const resolvedDispositionMarker = ""; +const scopedResolvePrompt = `\${scopedResolveCommand} using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. After fixing or dispositioning a thread, reply in that thread with \${resolvedDispositionMarker} as the first line, followed by a concise summary; that marker authorizes the workflow to close that exact thread. If human input or new authorization is required, do not use the marker and leave the thread open with the blocker. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation.`; const forbiddenPatterns = [ { @@ -44,8 +45,8 @@ const forbiddenPatterns = [ "Do not apply Codex auto-resolve concurrency to the whole workflow; unrelated review comments must not displace an authorized pending job.", }, { - pattern: /^\s*(?:contents|pull-requests):\s*write\s*$/m, - message: "Do not grant content or pull-request write permission to the Codex auto-resolve bridge.", + pattern: /^\s*contents:\s*write\s*$/m, + message: "Do not grant content write permission to the Codex auto-resolve bridge.", }, { pattern: /uses:\s*actions\/github-script@v\d+/, @@ -82,13 +83,27 @@ if ( failures.push(`${reviewProtocolPath} must preserve the provider confirmation boundary.`); } +const requiredInstructionChecks = [ + [agentInstructionsPath, agentInstructions, "one automatic repair pass per pull request lifetime"], + [agentInstructionsPath, agentInstructions, resolvedDispositionMarker], + [reviewProtocolPath, reviewProtocol, "Treat GitHub automatic review as one pass per pull request"], + [reviewProtocolPath, reviewProtocol, resolvedDispositionMarker], + [reviewProtocolPath, reviewProtocol, "Do not start a new review"], +]; + +for (const [path, contents, requiredCheck] of requiredInstructionChecks) { + if (!contents.includes(requiredCheck)) { + failures.push(`${path} is missing automatic review lifecycle guidance: ${requiredCheck}`); + } +} + const requiredTriggerAndPermissionChecks = [ " pull_request_review_comment:", " types: [created]", " contents: read", " issues: write", " models: read", - " pull-requests: read", + " pull-requests: write", `uses: actions/github-script@${githubScriptPin} # v9.0.0`, "github.event.pull_request.state == 'open'", ]; @@ -111,8 +126,8 @@ for (const requiredCheck of requiredConcurrencyChecks) { } } -if (!workflow.includes("codex-autoresolve:${pr.head.sha}")) { - failures.push("Codex auto-resolve marker must be scoped to the pull request head SHA."); +if (!workflow.includes("codex-autoresolve-pr:${pr.number}")) { + failures.push("Codex auto-resolve marker must be scoped to the pull request for a single lifetime pass."); } const requiredIdentityChecks = [ @@ -141,10 +156,25 @@ for (const requiredCheck of requiredSelfTriggerChecks) { } } +const requiredThreadResolutionChecks = [ + `const resolvedDispositionMarker = "${resolvedDispositionMarker}"`, + "replyBody.startsWith(resolvedDispositionMarker)", + "reviewThreads(first: 100, after: $cursor)", + "resolveReviewThread(input: { threadId: $threadId })", + "pull-requests: write", + "core.setFailed(message)", +]; + +for (const requiredCheck of requiredThreadResolutionChecks) { + if (!workflow.includes(requiredCheck)) { + failures.push(`Codex auto-resolve workflow is missing direct review-thread resolution: ${requiredCheck}`); + } +} + const requiredDedupeChecks = [ 'comment.user?.type === "Bot"', 'comment.user.login === "github-actions[bot]"', - "const maxAutoResolveRequests = 3", + "const maxAutoResolveRequests = 1", "const trustedExistingRequests = existingComments.filter(", '.trimStart().startsWith("", + body: "", user: { login: "attacker", type: "User" }, }, ], }); expect(result.createdComments).toHaveLength(1); - expect(result.createdComments[0]?.body).toContain(""); + expect(result.createdComments[0]?.body).toContain(""); }); it("honors a deduplication marker posted by GitHub Actions", async () => { const result = await runWorkflowScript({ existingComments: [ { - body: "", + body: "", user: { login: "github-actions[bot]", type: "Bot" }, }, ], @@ -302,30 +326,41 @@ describe("Codex auto-resolve workflow script", () => { expect(result.notices).toContainEqual(expect.stringContaining("Skipping duplicate")); }); - it("creates a follow-up request when Codex reviews a new pull request head", async () => { + it("does not create a follow-up repair pass when Codex reviews a new pull request head", async () => { const result = await runWorkflowScript({ existingComments: [ { - body: "", + body: "\n\n", user: { login: "github-actions[bot]", type: "Bot" }, }, ], }); - expect(result.createdComments).toHaveLength(1); - expect(result.createdComments[0]?.body).toContain(""); + expect(result.createdComments).toHaveLength(0); + expect(result.notices).toContainEqual(expect.stringContaining("Skipping duplicate")); }); - it("stops automatic repair after three trusted head-specific requests", async () => { + it("stops automatic repair after one trusted legacy request", async () => { const result = await runWorkflowScript({ - existingComments: ["head-sha-1", "head-sha-2", "head-sha-3"].map((sha) => ({ - body: ``, - user: { login: "github-actions[bot]", type: "Bot" }, - })), + existingComments: [ + { + body: "", + user: { login: "github-actions[bot]", type: "Bot" }, + }, + ], }); expect(result.createdComments).toHaveLength(0); - expect(result.warnings).toContainEqual(expect.stringContaining("automatic repair limit")); + expect(result.warnings).toContainEqual(expect.stringContaining("single automatic repair pass")); + }); + + it("emits a single-pass prompt with the thread disposition marker", async () => { + const result = await runWorkflowScript(); + + expect(result.createdComments).toHaveLength(1); + expect(result.createdComments[0]?.body).toContain("single automatic repair pass"); + expect(result.createdComments[0]?.body).toContain(""); + expect(result.createdComments[0]?.body).toContain("do not perform a fresh review"); }); it("does not skip a finding that merely quotes the marker and command", async () => { @@ -341,6 +376,85 @@ describe("Codex auto-resolve workflow script", () => { expect(result.createdComments).toHaveLength(1); }); + it("ignores review-thread replies without the resolved disposition marker", async () => { + const result = await runWorkflowScript({ + reviewComment: { body: "Fixed in the latest commit.", in_reply_to_id: 41 }, + }); + + expect(result.graphqlCalls).toHaveLength(0); + expect(result.createdComments).toHaveLength(0); + expect(result.notices).toContainEqual(expect.stringContaining("without the trusted resolved disposition marker")); + }); + + it("resolves the exact review thread after a trusted disposition reply", async () => { + const result = await runWorkflowScript({ + graphqlResults: [ + { + repository: { + pullRequest: { + reviewThreads: { + nodes: [ + { + comments: { nodes: [{ databaseId: 41 }, { databaseId: 99 }] }, + id: "thread-1", + isResolved: false, + }, + ], + pageInfo: { endCursor: null, hasNextPage: false }, + }, + }, + }, + }, + { resolveReviewThread: { thread: { id: "thread-1", isResolved: true } } }, + ], + reviewComment: { + body: "\n\nFixed and verified.", + in_reply_to_id: 41, + }, + }); + + expect(result.graphqlCalls).toHaveLength(2); + expect(result.graphqlCalls[1]?.query).toContain("resolveReviewThread"); + expect(result.graphqlCalls[1]?.variables).toEqual({ threadId: "thread-1" }); + expect(result.notices).toContainEqual(expect.stringContaining("Resolved the Codex review thread")); + }); + + it("fails visibly when a disposition reply cannot be mapped to a review thread", async () => { + const result = await runWorkflowScript({ + graphqlResults: [ + { + repository: { + pullRequest: { + reviewThreads: { + nodes: [], + pageInfo: { endCursor: null, hasNextPage: false }, + }, + }, + }, + }, + ], + reviewComment: { + body: "\n\nDispositioned.", + in_reply_to_id: 41, + }, + }); + + expect(result.failures).toContainEqual(expect.stringContaining("could not find the review thread")); + }); + + it("fails visibly when GitHub rejects direct review-thread resolution", async () => { + const result = await runWorkflowScript({ + graphqlError: new Error("permission denied"), + reviewComment: { + body: "\n\nFixed.", + in_reply_to_id: 41, + }, + }); + + expect(result.failures).toContainEqual(expect.stringContaining("permission denied")); + expect(result.warnings).toContainEqual(expect.stringContaining("permission denied")); + }); + it("fails visibly on a permission failure while listing comments", async () => { const result = await runWorkflowScript({ paginateError: { status: 403 } });