From 74c780549e1309caf9625b5974d5688913de008e Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 10 Jul 2026 22:24:28 +0800 Subject: [PATCH 1/2] fix: harden Codex review auto-resolve flow --- .../codex-autofix-review-comments.yml | 70 +++- AGENTS.md | 7 +- docs/branch-review-ledger.md | 9 +- scripts/check-codex-autofix-workflow.mjs | 124 +++++- scripts/check-github-action-pins.mjs | 8 + tests/codex-autofix-workflow.test.ts | 353 ++++++++++++++++++ 6 files changed, 531 insertions(+), 40 deletions(-) create mode 100644 tests/codex-autofix-workflow.test.ts diff --git a/.github/workflows/codex-autofix-review-comments.yml b/.github/workflows/codex-autofix-review-comments.yml index 8514c28e8..317d2a0d5 100644 --- a/.github/workflows/codex-autofix-review-comments.yml +++ b/.github/workflows/codex-autofix-review-comments.yml @@ -9,37 +9,50 @@ permissions: issues: write pull-requests: read -concurrency: - group: codex-autoresolve-${{ github.event.pull_request.number }} - cancel-in-progress: false - jobs: request-codex-autoresolve: name: Request Codex auto-resolve runs-on: ubuntu-24.04 if: > github.event.pull_request.state == 'open' && - contains(github.event.comment.user.login, 'chatgpt-codex-connector') + github.event.comment.user.type == 'Bot' && + (github.event.comment.user.login == 'chatgpt-codex-connector' || + github.event.comment.user.login == 'chatgpt-codex-connector[bot]') + concurrency: + group: codex-autoresolve-${{ github.event.pull_request.number }} + cancel-in-progress: false steps: - name: Ask Codex to resolve review comments - uses: actions/github-script@v7 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: github-token: ${{ github.token }} script: | const pr = context.payload.pull_request; const reviewComment = context.payload.comment; + const allowedCodexBotLogins = new Set([ + "chatgpt-codex-connector", + "chatgpt-codex-connector[bot]", + ]); + const scopedResolveCommand = "@codex resolve actionable Codex review findings for this pull request and current head"; + + if ( + reviewComment.user?.type !== "Bot" || + !allowedCodexBotLogins.has(reviewComment.user.login) + ) { + core.warning("Skipping Codex auto-resolve request because the review comment author is not the trusted Codex connector bot."); + return; + } if (reviewComment.in_reply_to_id) { core.notice("Skipping Codex auto-resolve request because the comment is a review-thread reply."); return; } - const sourceBody = reviewComment.body || ""; - if ( - sourceBody.includes("codex-autoresolve:") || - sourceBody.includes("codex-autoresolve-pr:") || - sourceBody.includes("@codex resolve actionable Codex review findings for this pull request and current head") - ) { + const sourceBody = (reviewComment.body || "").trimStart(); + const hasAutoResolveMarker = + sourceBody.startsWith("`; + const marker = ``; + const maxAutoResolveRequests = 3; let existingComments; try { @@ -59,23 +73,39 @@ jobs: }); } catch (error) { if (error.status === 403) { - core.warning("Skipping Codex auto-resolve request because GitHub denied permission to list issue comments."); - core.notice("Grant issues: read/write permission or allow the workflow token to read issue comments to enable duplicate request checks."); + const message = "Codex auto-resolve failed because GitHub denied permission to list issue comments."; + core.warning(message); + core.setFailed(message); return; } throw error; } - if (existingComments.some((comment) => (comment.body || "").includes(marker))) { - core.notice("Skipping duplicate Codex auto-resolve request for this pull request."); + const trustedExistingRequests = existingComments.filter( + (comment) => + comment.user?.type === "Bot" && + comment.user.login === "github-actions[bot]" && + (comment.body || "").trimStart().startsWith("`, - "@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.", + `${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.`, ].join("\n\n"); try { @@ -87,7 +117,9 @@ jobs: }); } catch (error) { if (error.status === 403) { - core.warning("Skipping Codex auto-resolve request because this workflow token cannot comment on the pull request."); + const message = "Codex auto-resolve failed because this workflow token cannot comment on the pull request."; + core.warning(message); + core.setFailed(message); return; } diff --git a/AGENTS.md b/AGENTS.md index d08affc1d..121148643 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -389,9 +389,14 @@ When explicitly asked to fix or resolve review findings: Automatic Codex review is review-only by default. This repository includes `.github/workflows/codex-autofix-review-comments.yml`, which requests the resolve task automatically after Codex posts a PR review or inline review comment. - The workflow must only trigger from Codex review bot reviews or comments on open pull requests. +- 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 all review comments using these repository instructions. -- The workflow must avoid duplicate requests for the same pull request, even after follow-up commits change the head SHA. +- The workflow may request one repair per pull-request head SHA, capped at three automatic repair cycles per pull 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. - 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. diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index e4d61b8c5..d65a6c373 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -18,6 +18,9 @@ Use this ledger to prevent repeated branch and PR reviews when the reviewed HEAD ## Review Records -| Date | Branch or ref | Reviewed HEAD | Scope | Outcome | Checks | -| ---------- | -------------- | ------------- | -------------- | ------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------- | -| 2026-07-09 | example/branch | abc1234 | branch-cleanup | Example: already merged into `main`; no unique patch content. | `git log --right-only --cherry-pick main...example/branch`; `git diff --name-status main...example/branch` | +| Date | Branch or ref | Reviewed HEAD | Scope | Outcome | Checks | +| ---------- | ------------------------- | ---------------------------------------- | ---------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| 2026-07-09 | example/branch | abc1234 | branch-cleanup | Example: already merged into `main`; no unique patch content. | `git log --right-only --cherry-pick main...example/branch`; `git diff --name-status main...example/branch` | +| 2026-07-10 | codex/review-autofix-flow | 155c801cd58f797037d8aaa8b885405a1c599249 | codex-autofix-flow | Fixed exact connector authorization, trusted-marker deduplication, and strict self-trigger matching; added regression coverage. | `npm run check:codex-autofix-workflow`; focused Vitest (4 passed); `npm run verify:cheap` pre-test stages passed before tool timeout; `npm test` (1,415 passed, 1 skipped); focused Prettier check; `npm run check:github-actions` | +| 2026-07-10 | codex/review-autofix-flow | 155c801cd58f797037d8aaa8b885405a1c599249 | codex-autofix-flow-followup | Fixed untrusted workflow-level concurrency interference and migrated the bridge from the Node 20 action runtime to `actions/github-script@v9`; added direct embedded-script execution coverage. | Focused Vitest (13 passed); targeted ESLint; `tsc --noEmit`; `npm run check:codex-autofix-workflow`; `npm run check:github-actions`; focused Prettier check; `git diff --check` | +| 2026-07-10 | codex/review-autofix-flow | 155c801cd58f797037d8aaa8b885405a1c599249 | codex-autofix-residual-fixes | Replaced one-shot PR deduplication with a three-cycle head-SHA cap, made comment permission failures fail visibly, and pinned `github-script` v9.0.0 to its verified immutable commit. | TDD red run (7 expected failures); focused Vitest green run (15 passed); `npm run verify:cheap` (152 files passed, 1 skipped; 1,426 tests passed, 1 skipped); focused Prettier check; `git diff --check`; official `git ls-remote` tag verification | diff --git a/scripts/check-codex-autofix-workflow.mjs b/scripts/check-codex-autofix-workflow.mjs index d7e5ffa20..c33dc92da 100644 --- a/scripts/check-codex-autofix-workflow.mjs +++ b/scripts/check-codex-autofix-workflow.mjs @@ -1,21 +1,14 @@ import fs from "node:fs"; -const workflowPath = ".github/workflows/codex-autofix-review-comments.yml"; +const workflowPath = process.argv[2] ?? ".github/workflows/codex-autofix-review-comments.yml"; const workflow = fs.readFileSync(workflowPath, "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 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 forbiddenPatterns = [ - { - pattern: /github\.event\.pull_request\.head\.sha/, - message: "Do not key the Codex auto-resolve workflow on github.event.pull_request.head.sha.", - }, - { - pattern: /pr\.head\.sha/, - message: "Do not key the Codex auto-resolve workflow on pr.head.sha.", - }, { pattern: /^\s*issue_comment:/m, message: "Do not trigger Codex auto-resolve from issue_comment events.", @@ -28,6 +21,32 @@ const forbiddenPatterns = [ pattern: /@codex resolve all review comments/, message: "Do not use the broad Codex resolve-all command; use the scoped actionable-findings command.", }, + { + pattern: /contains\(\s*github\.event\.comment\.user\.login/, + message: + "Do not authorize the Codex connector with a substring login match; require an exact trusted bot identity.", + }, + { + pattern: /sourceBody\.includes\("codex-autoresolve(?:-pr)?:"\)/, + message: "Do not treat an auto-resolve marker mentioned anywhere in a review finding as a self-triggered request.", + }, + { + pattern: /existingComments\.some\(\(comment\) => \(comment\.body \|\| ""\)\.includes\(marker\)\)/, + message: "Do not trust auto-resolve markers from arbitrary pull request commenters.", + }, + { + pattern: /^concurrency:/m, + message: + "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: /uses:\s*actions\/github-script@v\d+/, + message: "Do not use a mutable github-script tag; pin the reviewed action commit.", + }, ]; for (const { pattern, message } of forbiddenPatterns) { @@ -36,19 +55,90 @@ for (const { pattern, message } of forbiddenPatterns) { } } -if (!workflow.includes("group: codex-autoresolve-${{ github.event.pull_request.number }}")) { - failures.push("Codex auto-resolve concurrency must be scoped to the pull request number only."); +const requiredTriggerAndPermissionChecks = [ + " pull_request_review_comment:", + " types: [created]", + " contents: read", + " issues: write", + " pull-requests: read", + `uses: actions/github-script@${githubScriptPin} # v9.0.0`, + "github.event.pull_request.state == 'open'", +]; + +for (const requiredCheck of requiredTriggerAndPermissionChecks) { + if (!workflow.includes(requiredCheck)) { + failures.push(`Codex auto-resolve workflow is missing trigger or permission check: ${requiredCheck}`); + } +} + +const requiredConcurrencyChecks = [ + " concurrency:", + " group: codex-autoresolve-${{ github.event.pull_request.number }}", + " cancel-in-progress: false", +]; + +for (const requiredCheck of requiredConcurrencyChecks) { + if (!workflow.includes(requiredCheck)) { + failures.push(`Codex auto-resolve workflow is missing authorized job concurrency check: ${requiredCheck}`); + } +} + +if (!workflow.includes("codex-autoresolve:${pr.head.sha}")) { + failures.push("Codex auto-resolve marker must be scoped to the pull request head SHA."); +} + +const requiredIdentityChecks = [ + "github.event.comment.user.type == 'Bot'", + "github.event.comment.user.login == 'chatgpt-codex-connector'", + "github.event.comment.user.login == 'chatgpt-codex-connector[bot]'", + 'reviewComment.user?.type !== "Bot"', + "!allowedCodexBotLogins.has(reviewComment.user.login)", +]; + +for (const requiredCheck of requiredIdentityChecks) { + if (!workflow.includes(requiredCheck)) { + failures.push(`Codex auto-resolve workflow is missing trusted connector identity check: ${requiredCheck}`); + } +} + +const requiredSelfTriggerChecks = [ + 'sourceBody.startsWith("", + user: { login: "attacker", type: "User" }, + }, + ], + }); + + expect(result.createdComments).toHaveLength(1); + expect(result.createdComments[0]?.body).toContain(""); + }); + + it("honors a deduplication marker posted by GitHub Actions", async () => { + const result = await runWorkflowScript({ + existingComments: [ + { + body: "", + user: { login: "github-actions[bot]", type: "Bot" }, + }, + ], + }); + + expect(result.createdComments).toHaveLength(0); + expect(result.notices).toContainEqual(expect.stringContaining("Skipping duplicate")); + }); + + it("creates a follow-up request when Codex reviews a new pull request head", async () => { + const result = await runWorkflowScript({ + existingComments: [ + { + body: "", + user: { login: "github-actions[bot]", type: "Bot" }, + }, + ], + }); + + expect(result.createdComments).toHaveLength(1); + expect(result.createdComments[0]?.body).toContain(""); + }); + + it("stops automatic repair after three trusted head-specific requests", 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" }, + })), + }); + + expect(result.createdComments).toHaveLength(0); + expect(result.warnings).toContainEqual(expect.stringContaining("automatic repair limit")); + }); + + it("does not skip a finding that merely quotes the marker and command", async () => { + const result = await runWorkflowScript({ + reviewComment: { + body: [ + "This finding quotes in the middle of a sentence.", + "It also quotes @codex resolve actionable Codex review findings for this pull request and current head.", + ].join(" "), + }, + }); + + expect(result.createdComments).toHaveLength(1); + }); + + it("fails visibly on a permission failure while listing comments", async () => { + const result = await runWorkflowScript({ paginateError: { status: 403 } }); + + expect(result.createdComments).toHaveLength(0); + expect(result.failures).toContainEqual(expect.stringContaining("denied permission to list issue comments")); + expect(result.warnings).toContainEqual(expect.stringContaining("denied permission to list issue comments")); + }); + + it("fails visibly on a permission failure while creating the request", async () => { + const result = await runWorkflowScript({ createError: { status: 403 } }); + + expect(result.createdComments).toHaveLength(1); + expect(result.failures).toContainEqual(expect.stringContaining("cannot comment on the pull request")); + expect(result.warnings).toContainEqual(expect.stringContaining("cannot comment on the pull request")); + }); + + it("rethrows unexpected failures while listing comments", async () => { + await expect(runWorkflowScript({ paginateError: { status: 500 } })).rejects.toEqual({ status: 500 }); + }); +}); From 859624fa801462a10acb7ca28655498e0d75904d Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 10 Jul 2026 22:38:43 +0800 Subject: [PATCH 2/2] docs: hyphenate Node 24-based policy --- AGENTS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 121148643..eec34f2f5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -391,7 +391,7 @@ Automatic Codex review is review-only by default. This repository includes `.git - The workflow must only trigger from Codex review bot reviews or comments on open pull requests. - 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. +- 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 all review comments using these repository instructions. - The workflow may request one repair per pull-request head SHA, capped at three automatic repair cycles per pull request.