From 00d546f9e040e4ed9ab9d0c1b3b0ab3659183f09 Mon Sep 17 00:00:00 2001 From: HemSoft Date: Thu, 22 Oct 2026 23:13:52 -0400 Subject: [PATCH 1/5] fix: enforce current-head CI and Codex review before merge (#237) --- .github/scripts/codex-review.cjs | 67 ++++++++++++++++++++++++ .github/scripts/codex-review.test.cjs | 68 +++++++++++++++++++++++++ .github/scripts/require-merge-checks.py | 49 ++++++++++++++++++ .github/workflows/ci.yml | 3 ++ .github/workflows/codex-review.yml | 43 ++++++++++++++++ docs/MERGE_POLICY.md | 66 ++++++++++++++++++++++++ 6 files changed, 296 insertions(+) create mode 100644 .github/scripts/codex-review.cjs create mode 100644 .github/scripts/codex-review.test.cjs create mode 100644 .github/scripts/require-merge-checks.py create mode 100644 .github/workflows/codex-review.yml create mode 100644 docs/MERGE_POLICY.md diff --git a/.github/scripts/codex-review.cjs b/.github/scripts/codex-review.cjs new file mode 100644 index 00000000..260d7717 --- /dev/null +++ b/.github/scripts/codex-review.cjs @@ -0,0 +1,67 @@ +'use strict'; + +const CONTEXT = 'codex-review'; +const CODEX = 'chatgpt-codex-connector[bot]'; + +// Only a structured result from the connected app can satisfy the ordinary gate. +function codexVerdict(comments, head) { + const summaries = comments.filter(comment => comment.user?.login === CODEX && comment.user?.type === 'Bot') + .sort((a, b) => Date.parse(b.updated_at) - Date.parse(a.updated_at)); + for (const comment of summaries) { + const match = comment.body?.match(//); + if (!match) continue; + let review; + try { review = JSON.parse(match[1]).code_review; } catch { continue; } + if (review?.commit_sha !== head) continue; + if (review.status !== 'completed') return { state: 'pending', description: 'Current-head Codex review is running' }; + if (review.has_any_findings === false && review.has_unresolved_findings === false) { + return { state: 'success', description: 'Codex reviewed this exact head with no findings', target_url: comment.html_url }; + } + return { state: 'failure', description: 'Codex findings or an incomplete verdict need attention', target_url: comment.html_url }; + } + return { state: 'pending', description: 'A clean Codex review of this exact head is required' }; +} + +// A maintainer-published cross-model review is the documented alternative when +// connected Codex is unavailable. It is review evidence, never a bypass flag. +function fallbackReceipt(review, head) { + if (review.user?.login !== 'HemSoft' || review.user?.type !== 'User' || + review.commit_id !== head || review.state !== 'COMMENTED') return false; + const match = review.body?.match(/^/); + if (!match) return false; + try { + const receipt = JSON.parse(match[1]); + return receipt.head === head && receipt.clean === true && receipt.provider === 'claude' && + receipt.model === 'claude-opus-5-5' && ['hung', 'unavailable'].includes(receipt.codex); + } catch { return false; } +} + +async function run({ github, context, core }) { + const { owner, repo } = context.repo; + const number = context.payload.pull_request?.number ?? context.payload.issue?.number ?? Number(context.payload.inputs?.number); + if (!Number.isSafeInteger(number) || number <= 0) throw new Error('A pull request number is required'); + const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: number }); + if (pr.state !== 'open' || pr.base.ref !== 'main') return; + const head = pr.head.sha; + const comments = await github.paginate(github.rest.issues.listComments, { owner, repo, issue_number: number, per_page: 100 }); + let verdict = codexVerdict(comments, head); + const reviews = await github.paginate(github.rest.pulls.listReviews, { owner, repo, pull_number: number, per_page: 100 }); + const latest = reviews.filter(review => review.user?.login === 'HemSoft' && review.commit_id === head).at(-1); + const fallback = latest && fallbackReceipt(latest, head) ? latest : null; + if (fallback && verdict.state === 'pending') { + const { data: permission } = await github.rest.repos.getCollaboratorPermissionLevel({ owner, repo, username: 'HemSoft' }); + if (['admin', 'maintain'].includes(permission.permission)) { + verdict = { state: 'success', description: 'Clean current-head Claude Opus fallback review', target_url: fallback.html_url }; + } + } + // Never attach a decision assembled across a concurrent head change. + const { data: live } = await github.rest.pulls.get({ owner, repo, pull_number: number }); + if (live.state !== 'open' || live.head.sha !== head) { + core.info('Head changed while reading reviews; the synchronize event owns the new head'); + return; + } + await github.rest.repos.createCommitStatus({ owner, repo, sha: head, context: CONTEXT, ...verdict }); + core.info(`${CONTEXT} ${head}: ${verdict.state}`); +} + +module.exports = { codexVerdict, fallbackReceipt, run }; diff --git a/.github/scripts/codex-review.test.cjs b/.github/scripts/codex-review.test.cjs new file mode 100644 index 00000000..413a0ed7 --- /dev/null +++ b/.github/scripts/codex-review.test.cjs @@ -0,0 +1,68 @@ +'use strict'; +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const { codexVerdict, fallbackReceipt, run } = require('./codex-review.cjs'); +const head = 'a'.repeat(40); +const clean = { commit_sha: head, status: 'completed', has_any_findings: false, has_unresolved_findings: false }; +const summary = (result, user = { login: 'chatgpt-codex-connector[bot]', type: 'Bot' }) => ({ + user, body: ``, updated_at: '2026-10-22', html_url: 'https://github.com/review' +}); + +test('only a completed clean bot verdict for the exact head passes', () => { + assert.equal(codexVerdict([summary(clean)], head).state, 'success'); + for (const result of [ { ...clean, commit_sha: 'b'.repeat(40) }, { ...clean, status: 'running' }, + { ...clean, has_any_findings: true }, { ...clean, has_unresolved_findings: true }, + { ...clean, has_any_findings: null }, { ...clean, has_unresolved_findings: undefined } ]) { + assert.notEqual(codexVerdict([summary(result)], head).state, 'success'); + } + assert.equal(codexVerdict([], head).state, 'pending'); + assert.equal(codexVerdict([summary(clean, { login: 'someone', type: 'User' })], head).state, 'pending'); + assert.equal(codexVerdict([summary(clean, { login: 'chatgpt-codex-connector[bot]', type: 'User' })], head).state, 'pending'); + assert.equal(codexVerdict([{ ...summary(clean), body: '' }], head).state, 'pending'); +}); + +test('a later running or failed result replaces an earlier clean result', () => { + const later = { ...summary({ ...clean, status: 'running' }), updated_at: '2026-10-23' }; + assert.equal(codexVerdict([summary(clean), later], head).state, 'pending'); +}); + +const receipt = { head, clean: true, provider: 'claude', model: 'claude-opus-5-5', codex: 'hung' }; +const fallback = { user: { login: 'HemSoft', type: 'User' }, commit_id: head, state: 'COMMENTED', + body: ``, html_url: 'https://github.com/fallback' }; +test('fallback requires the named maintainer, exact head, model and explicit clean receipt', () => { + assert.equal(fallbackReceipt(fallback, head), true); + for (const change of [{ commit_id: 'b'.repeat(40) }, { state: 'CHANGES_REQUESTED' }, + { user: { login: 'someone', type: 'User' } }, { body: 'clean' }, + { body: `` }]) { + assert.equal(fallbackReceipt({ ...fallback, ...change }, head), false); + } +}); + +function client({ moved = false, reviews = [], comments = [], permission = 'admin' } = {}) { + const statuses = []; let reads = 0; + const github = { paginate: async method => method(), rest: { + pulls: { get: async () => ({ data: { state: 'open', base: { ref: 'main' }, head: { sha: moved && reads++ ? 'b'.repeat(40) : head } } }), listReviews: async () => reviews }, + issues: { listComments: async () => comments }, + repos: { createCommitStatus: async value => statuses.push(value), getCollaboratorPermissionLevel: async () => ({ data: { permission } }) } + } }; + return { github, statuses, context: { repo: { owner: 'hemsoft-dev', repo: 'codexbar' }, payload: { pull_request: { number: 1 } } }, core: { info() {} } }; +} + +test('a concurrent head change cannot publish a stale decision', async () => { + const fixture = client({ moved: true }); await run(fixture); assert.equal(fixture.statuses.length, 0); +}); +test('missing evidence publishes pending on the exact head', async () => { + const fixture = client(); await run(fixture); + assert.equal(fixture.statuses[0].sha, head); assert.equal(fixture.statuses[0].state, 'pending'); +}); +test('fallback also verifies live maintainer permission', async () => { + const allowed = client({ reviews: [fallback] }); await run(allowed); assert.equal(allowed.statuses[0].state, 'success'); + const refused = client({ reviews: [fallback], permission: 'write' }); await run(refused); assert.equal(refused.statuses[0].state, 'pending'); +}); + +test('fallback cannot hide Codex findings or a later non-clean maintainer review', async () => { + const findings = client({ reviews: [fallback], comments: [summary({ ...clean, has_any_findings: true })] }); + await run(findings); assert.equal(findings.statuses[0].state, 'failure'); + const later = client({ reviews: [fallback, { ...fallback, body: 'P2: fix this' }] }); + await run(later); assert.equal(later.statuses[0].state, 'pending'); +}); diff --git a/.github/scripts/require-merge-checks.py b/.github/scripts/require-merge-checks.py new file mode 100644 index 00000000..0c8785f2 --- /dev/null +++ b/.github/scripts/require-merge-checks.py @@ -0,0 +1,49 @@ +#!/usr/bin/env python3 +"""Add required checks without changing any other main protection setting.""" +import argparse +import json +import subprocess + + +def api(path, method="GET", payload=None): + command = ["gh", "api", path, "--method", method] + if payload is not None: + command += ["--input", "-"] + result = subprocess.run(command, input=None if payload is None else json.dumps(payload), + text=True, capture_output=True, check=True) + return json.loads(result.stdout) + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--apply", action="store_true", help="Apply the additive status requirement") + args = parser.parse_args() + repo = "hemsoft-dev/codexbar" + path = f"repos/{repo}/branches/main/protection/required_status_checks" + # The checks source is discovered from a real CI result, not a user's status. + checks = api(f"repos/{repo}/commits/main/check-runs")["check_runs"] + app_ids = {check["app"]["id"] for check in checks if check["name"] == "security-scan"} + if len(app_ids) != 1: + raise RuntimeError("Cannot identify the CI GitHub App from main's security-scan") + app_id = app_ids.pop() + protection = api(f"repos/{repo}/branches/main/protection") + existing = protection.get("required_status_checks", {}) + required = {check["context"]: check for check in existing.get("checks", [])} + for name in existing.get("contexts", []): + required.setdefault(name, {"context": name, "app_id": -1}) + for name in ["build", "security-scan", "rust (windows-latest)", "rust (macos-latest)", + "rust (ubuntu-latest)", "codex-review"]: + required[name] = {"context": name, "app_id": app_id} + payload = {"strict": existing.get("strict", False), "checks": list(required.values())} + print(json.dumps(payload, indent=2)) + if args.apply: + # This endpoint changes only required statuses. It never replaces review, + # conversation, admin-enforcement or bypass settings. + result = api(path, "PATCH", payload) + assert {(c["context"], c["app_id"]) for c in payload["checks"]}.issubset( + {(c["context"], c["app_id"]) for c in result["checks"]}) + print("Required checks verified") + + +if __name__ == "__main__": + main() diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 497d1072..8755cba9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -97,6 +97,9 @@ jobs: node-version: '20' cache: 'npm' + - name: Test Codex review policy + run: node --test .github/scripts/codex-review.test.cjs + - name: Install markdownlint-cli run: npm ci diff --git a/.github/workflows/codex-review.yml b/.github/workflows/codex-review.yml new file mode 100644 index 00000000..fea7f478 --- /dev/null +++ b/.github/workflows/codex-review.yml @@ -0,0 +1,43 @@ +name: Codex review gate + +# Metadata only. Never check out or execute a pull request's code with this token. +on: + pull_request_target: + branches: [main] + types: [opened, reopened, synchronize, ready_for_review] + issue_comment: + types: [created, edited, deleted] + pull_request_review: + types: [submitted, edited, dismissed] + workflow_dispatch: + inputs: + number: + description: Pull request number whose review evidence should be refreshed + required: true + type: string + +permissions: + contents: read + pull-requests: read + issues: read + statuses: write + +concurrency: + group: codex-review-${{ github.event.pull_request.number || github.event.issue.number || inputs.number }} + cancel-in-progress: false + +jobs: + review: + if: github.event_name != 'issue_comment' || github.event.issue.pull_request + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + # Use trusted main, including for review/comment events on a fork. + ref: ${{ github.event.repository.default_branch }} + persist-credentials: false + - uses: actions/github-script@v8 + with: + script: | + const { run } = require('./.github/scripts/codex-review.cjs'); + await run({ github, context, core }); diff --git a/docs/MERGE_POLICY.md b/docs/MERGE_POLICY.md new file mode 100644 index 00000000..c469b93e --- /dev/null +++ b/docs/MERGE_POLICY.md @@ -0,0 +1,66 @@ +# Merge policy + +Main requires these checks from GitHub Actions: `build`, `security-scan`, +`rust (windows-latest)`, `rust (macos-latest)`, `rust (ubuntu-latest)` and +`codex-review`. Conversation resolution and enforcement for administrators +remain enabled. There is no admin bypass and no additional human approval +requirement. A clean comment-only Codex review is sufficient review evidence. + +The review gate reads the connected Codex bot's structured summary, not a +comment that merely says `@codex review`, an eyes reaction, or another user's +copy of a result. The summary must name the complete current head SHA, be +completed, and report no findings. Running, malformed, missing, or old-head +results cannot pass. Each new head needs its own review. The workflow reads +GitHub metadata and executes only the script from trusted main, never PR code. + +Request `@codex review` once per head. If it remains hung for an hour, or the +integration explicitly refuses the review, use the maintainer-authorized +Claude Opus 5.5 fallback. Fix findings and repeat review until clean. Publish +its clean result as a COMMENT review on the exact commit, as `HemSoft`, with +this first-line receipt, followed by the review's scope and evidence: + +```text + +``` + +Use `unavailable` instead of `hung` for an explicit integration refusal. +The gate verifies the review's actual commit and live maintainer permission. +The latest HemSoft review on that head must carry the clean receipt. It cannot +override Codex's completed findings. This is a reviewed alternative to a hung +integration, not a switch that skips review. Unresolved threads still block +merge through native conversation protection. + +The gate refreshes on PR head changes, bot comment updates and reviews. A +maintainer can refresh existing evidence with +`gh workflow run codex-review.yml -f number=PR_NUMBER`. This does not request a +review or change the evidence requirements. There is no polling or schedule. + +## Required check setup + +After the review workflow has merged to main, preview the additive settings: + +```sh +python3 .github/scripts/require-merge-checks.py +``` + +Then apply them with `--apply`. The helper discovers GitHub Actions' app ID +from main's security job and changes only required statuses. It preserves +existing requirements and strictness, conversation resolution, administrator +enforcement, review settings and bypass policy. Read back +`gh api repos/hemsoft-dev/codexbar/branches/main/protection` and verify the six +contexts and source app IDs. Repository settings are live configuration; +merging this document alone does not activate them. + +The path-filtered Package workflow is deliberately not globally required. +GitHub can leave a required check pending when its workflow is skipped by a +path filter. Packaging changes still run Package, and its applicable results +must pass before merge. Normal CI and the review status run for every PR. + +Validate the live protection with a disposable PR: missing review must leave +`codex-review` pending, and an old-head result must not pass its successor. +Confirm `mergeStateStatus` is blocked with missing/failed requirements. Close +the fixture without merging. Never attempt an admin merge to test protection. + +Policy references: [required status checks](https://docs.github.com/en/pull-requests/how-tos/merge-and-close-pull-requests/troubleshooting-required-status-checks), +[protected branches](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches), +and [safe metadata workflows](https://docs.github.com/en/actions/reference/security/securely-using-pull_request_target). From c343cb2c64c1a0fc398caf1e815d6974106b7779 Mon Sep 17 00:00:00 2001 From: HemSoft Date: Thu, 22 Oct 2026 23:24:30 -0400 Subject: [PATCH 2/5] fix: revalidate review evidence and gate test merge revisions (#237) --- .github/scripts/codex-review.cjs | 76 ++++++++++++++++++++++----- .github/scripts/codex-review.test.cjs | 70 ++++++++++++++++++------ .github/workflows/codex-review.yml | 13 ++++- docs/MERGE_POLICY.md | 13 +++-- 4 files changed, 137 insertions(+), 35 deletions(-) diff --git a/.github/scripts/codex-review.cjs b/.github/scripts/codex-review.cjs index 260d7717..3fa7bc46 100644 --- a/.github/scripts/codex-review.cjs +++ b/.github/scripts/codex-review.cjs @@ -1,6 +1,7 @@ 'use strict'; const CONTEXT = 'codex-review'; +const MAINTAINER_ID = 8227352; const CODEX = 'chatgpt-codex-connector[bot]'; // Only a structured result from the connected app can satisfy the ordinary gate. @@ -25,7 +26,7 @@ function codexVerdict(comments, head) { // A maintainer-published cross-model review is the documented alternative when // connected Codex is unavailable. It is review evidence, never a bypass flag. function fallbackReceipt(review, head) { - if (review.user?.login !== 'HemSoft' || review.user?.type !== 'User' || + if (review.user?.id !== MAINTAINER_ID || review.user?.type !== 'User' || review.commit_id !== head || review.state !== 'COMMENTED') return false; const match = review.body?.match(/^/); if (!match) return false; @@ -36,6 +37,41 @@ function fallbackReceipt(review, head) { } catch { return false; } } +function triggerIncluded(context, comments, reviews) { + const event = context.payload; + const comment = event.comment; + if (comment?.user?.login === CODEX && comment.user.type === 'Bot') { + const actual = comments.find(item => item.id === comment.id); + if (event.action === 'deleted') return !actual; + if (!actual) return false; + if (Date.parse(actual.updated_at) > Date.parse(comment.updated_at)) return true; + return actual.updated_at === comment.updated_at && actual.body === comment.body; + } + const review = event.review; + if (review?.user?.id === MAINTAINER_ID) { + const actual = reviews.find(item => item.id === review.id); + return actual?.body === review.body && actual?.state.toUpperCase() === review.state.toUpperCase(); + } + return true; +} + +async function snapshot(github, owner, repo, number) { + const comments = await github.paginate(github.rest.issues.listComments, { owner, repo, issue_number: number, per_page: 100 }); + const reviews = await github.paginate(github.rest.pulls.listReviews, { owner, repo, pull_number: number, per_page: 100 }); + return { comments, reviews }; +} + +// Compare all mutable fields that can affect the decision, including deletion +// and later maintainer reviews. Ordinary conversation cannot invalidate it. +function fingerprint({ comments, reviews }) { + return JSON.stringify({ + comments: comments.filter(item => item.user?.login === CODEX).map(item => + [item.id, item.user, item.updated_at, item.body]), + reviews: reviews.filter(item => item.user?.id === MAINTAINER_ID).map(item => + [item.id, item.user, item.commit_id, item.state, item.body]) + }); +} + async function run({ github, context, core }) { const { owner, repo } = context.repo; const number = context.payload.pull_request?.number ?? context.payload.issue?.number ?? Number(context.payload.inputs?.number); @@ -43,25 +79,39 @@ async function run({ github, context, core }) { const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: number }); if (pr.state !== 'open' || pr.base.ref !== 'main') return; const head = pr.head.sha; - const comments = await github.paginate(github.rest.issues.listComments, { owner, repo, issue_number: number, per_page: 100 }); - let verdict = codexVerdict(comments, head); - const reviews = await github.paginate(github.rest.pulls.listReviews, { owner, repo, pull_number: number, per_page: 100 }); - const latest = reviews.filter(review => review.user?.login === 'HemSoft' && review.commit_id === head).at(-1); + const merge = pr.merge_commit_sha; + const shas = [...new Set([head, merge].filter(Boolean))]; + const publish = async verdict => { + for (const sha of shas) { + await github.rest.repos.createCommitStatus({ owner, repo, sha, context: CONTEXT, ...verdict }); + } + }; + // Revoke previous success before reading mutable evidence. An API failure + // therefore leaves the gate pending rather than retaining cached consent. + await publish({ state: 'pending', description: 'Revalidating current-head review evidence' }); + const first = await snapshot(github, owner, repo, number); + let verdict = codexVerdict(first.comments, head); + const latest = first.reviews.filter(review => review.user?.id === MAINTAINER_ID && review.commit_id === head).at(-1); const fallback = latest && fallbackReceipt(latest, head) ? latest : null; - if (fallback && verdict.state === 'pending') { - const { data: permission } = await github.rest.repos.getCollaboratorPermissionLevel({ owner, repo, username: 'HemSoft' }); + const fresh = await snapshot(github, owner, repo, number); + const unchanged = triggerIncluded(context, fresh.comments, fresh.reviews) && fingerprint(first) === fingerprint(fresh); + if (!unchanged) { + verdict = { state: 'pending', description: 'Review evidence changed; a fresh event must revalidate it' }; + } else if (fallback && verdict.state === 'pending') { + const { data: permission } = await github.rest.repos.getCollaboratorPermissionLevel({ owner, repo, username: fallback.user.login }); if (['admin', 'maintain'].includes(permission.permission)) { verdict = { state: 'success', description: 'Clean current-head Claude Opus fallback review', target_url: fallback.html_url }; } } - // Never attach a decision assembled across a concurrent head change. + if (!merge) verdict = { state: 'pending', description: 'Waiting for GitHub to compute the test merge commit' }; + // Head evidence stays bound to the corresponding test merge revision. const { data: live } = await github.rest.pulls.get({ owner, repo, pull_number: number }); - if (live.state !== 'open' || live.head.sha !== head) { - core.info('Head changed while reading reviews; the synchronize event owns the new head'); + if (live.state !== 'open' || live.head.sha !== head || live.merge_commit_sha !== merge) { + core.info('Head or test merge changed; no final verdict was published'); return; } - await github.rest.repos.createCommitStatus({ owner, repo, sha: head, context: CONTEXT, ...verdict }); - core.info(`${CONTEXT} ${head}: ${verdict.state}`); + await publish(verdict); + core.info(`${CONTEXT} ${head} / ${merge}: ${verdict.state}`); } -module.exports = { codexVerdict, fallbackReceipt, run }; +module.exports = { codexVerdict, fallbackReceipt, triggerIncluded, run }; diff --git a/.github/scripts/codex-review.test.cjs b/.github/scripts/codex-review.test.cjs index 413a0ed7..d333ff19 100644 --- a/.github/scripts/codex-review.test.cjs +++ b/.github/scripts/codex-review.test.cjs @@ -1,7 +1,7 @@ 'use strict'; const { test } = require('node:test'); const assert = require('node:assert/strict'); -const { codexVerdict, fallbackReceipt, run } = require('./codex-review.cjs'); +const { codexVerdict, fallbackReceipt, triggerIncluded, run } = require('./codex-review.cjs'); const head = 'a'.repeat(40); const clean = { commit_sha: head, status: 'completed', has_any_findings: false, has_unresolved_findings: false }; const summary = (result, user = { login: 'chatgpt-codex-connector[bot]', type: 'Bot' }) => ({ @@ -27,7 +27,7 @@ test('a later running or failed result replaces an earlier clean result', () => }); const receipt = { head, clean: true, provider: 'claude', model: 'claude-opus-5-5', codex: 'hung' }; -const fallback = { user: { login: 'HemSoft', type: 'User' }, commit_id: head, state: 'COMMENTED', +const fallback = { user: { id: 8227352, login: 'HemSoft', type: 'User' }, commit_id: head, state: 'COMMENTED', body: ``, html_url: 'https://github.com/fallback' }; test('fallback requires the named maintainer, exact head, model and explicit clean receipt', () => { assert.equal(fallbackReceipt(fallback, head), true); @@ -38,31 +38,69 @@ test('fallback requires the named maintainer, exact head, model and explicit cle } }); -function client({ moved = false, reviews = [], comments = [], permission = 'admin' } = {}) { - const statuses = []; let reads = 0; +const merge = 'c'.repeat(40); +function client({ moved = false, mergeMoved = false, reviews = [], comments = [], + freshComments = comments, freshReviews = reviews, permission = 'admin', payload = { pull_request: { number: 1 } } } = {}) { + const statuses = []; let reads = 0; let commentReads = 0; let reviewReads = 0; const github = { paginate: async method => method(), rest: { - pulls: { get: async () => ({ data: { state: 'open', base: { ref: 'main' }, head: { sha: moved && reads++ ? 'b'.repeat(40) : head } } }), listReviews: async () => reviews }, - issues: { listComments: async () => comments }, + pulls: { get: async () => { + const later = reads++ > 0; + return { data: { state: 'open', base: { ref: 'main' }, head: { sha: moved && later ? 'b'.repeat(40) : head }, + merge_commit_sha: mergeMoved && later ? 'd'.repeat(40) : merge } }; + }, listReviews: async () => reviewReads++ ? freshReviews : reviews }, + issues: { listComments: async () => commentReads++ ? freshComments : comments }, repos: { createCommitStatus: async value => statuses.push(value), getCollaboratorPermissionLevel: async () => ({ data: { permission } }) } } }; - return { github, statuses, context: { repo: { owner: 'hemsoft-dev', repo: 'codexbar' }, payload: { pull_request: { number: 1 } } }, core: { info() {} } }; + return { github, statuses, context: { repo: { owner: 'hemsoft-dev', repo: 'codexbar' }, payload }, core: { info() {} } }; } +const result = fixture => fixture.statuses.at(-1).state; -test('a concurrent head change cannot publish a stale decision', async () => { - const fixture = client({ moved: true }); await run(fixture); assert.equal(fixture.statuses.length, 0); +test('a concurrent head or merge change cannot publish success', async () => { + for (const change of [{ moved: true }, { mergeMoved: true }]) { + const fixture = client({ ...change, comments: [summary(clean)] }); await run(fixture); + assert.equal(fixture.statuses.length, 2); + assert.ok(fixture.statuses.every(status => status.state === 'pending')); + } }); -test('missing evidence publishes pending on the exact head', async () => { +test('missing evidence publishes pending on both head and test merge', async () => { const fixture = client(); await run(fixture); - assert.equal(fixture.statuses[0].sha, head); assert.equal(fixture.statuses[0].state, 'pending'); + assert.deepEqual(fixture.statuses.slice(-2).map(status => status.sha), [head, merge]); + assert.equal(result(fixture), 'pending'); +}); +test('clean review publishes success on head and test merge', async () => { + const fixture = client({ comments: [summary(clean)] }); await run(fixture); + assert.deepEqual(fixture.statuses.slice(-2).map(status => [status.sha, status.state]), [[head, 'success'], [merge, 'success']]); }); test('fallback also verifies live maintainer permission', async () => { - const allowed = client({ reviews: [fallback] }); await run(allowed); assert.equal(allowed.statuses[0].state, 'success'); - const refused = client({ reviews: [fallback], permission: 'write' }); await run(refused); assert.equal(refused.statuses[0].state, 'pending'); + const allowed = client({ reviews: [fallback] }); await run(allowed); assert.equal(result(allowed), 'success'); + const refused = client({ reviews: [fallback], permission: 'write' }); await run(refused); assert.equal(result(refused), 'pending'); }); - test('fallback cannot hide Codex findings or a later non-clean maintainer review', async () => { const findings = client({ reviews: [fallback], comments: [summary({ ...clean, has_any_findings: true })] }); - await run(findings); assert.equal(findings.statuses[0].state, 'failure'); + await run(findings); assert.equal(result(findings), 'failure'); const later = client({ reviews: [fallback, { ...fallback, body: 'P2: fix this' }] }); - await run(later); assert.equal(later.statuses[0].state, 'pending'); + await run(later); assert.equal(result(later), 'pending'); +}); +test('the maintainer may rename without trusting an unrelated account with the old login', () => { + assert.equal(fallbackReceipt({ ...fallback, user: { ...fallback.user, login: 'renamed' } }, head), true); + assert.equal(fallbackReceipt({ ...fallback, user: { ...fallback.user, id: 1 } }, head), false); +}); +test('summary or fallback mutation or deletion before publication leaves pending', async () => { + for (const freshComments of [[], [summary({ ...clean, status: 'running' })], [summary({ ...clean, has_any_findings: true })]]) { + const fixture = client({ comments: [summary(clean)], freshComments }); await run(fixture); + assert.equal(result(fixture), 'pending'); + } + for (const freshReviews of [[], [{ ...fallback, body: 'findings' }]]) { + const fixture = client({ reviews: [fallback], freshReviews }); await run(fixture); + assert.equal(result(fixture), 'pending'); + } +}); +test('a stale API snapshot cannot ignore the triggering edit or deletion', async () => { + const old = { ...summary(clean), id: 42 }; + const edited = { ...old, body: 'running', updated_at: '2026-10-23' }; + for (const [action, comment] of [['edited', edited], ['deleted', old]]) { + const fixture = client({ comments: [old], payload: { issue: { number: 1 }, action, comment } }); + await run(fixture); assert.equal(result(fixture), 'pending'); + } + assert.equal(triggerIncluded({ payload: { action: 'deleted', comment: old } }, [], []), true); }); diff --git a/.github/workflows/codex-review.yml b/.github/workflows/codex-review.yml index fea7f478..162b654c 100644 --- a/.github/workflows/codex-review.yml +++ b/.github/workflows/codex-review.yml @@ -9,6 +9,9 @@ on: types: [created, edited, deleted] pull_request_review: types: [submitted, edited, dismissed] + workflow_run: + workflows: [CI] + types: [completed] workflow_dispatch: inputs: number: @@ -23,7 +26,7 @@ permissions: statuses: write concurrency: - group: codex-review-${{ github.event.pull_request.number || github.event.issue.number || inputs.number }} + group: codex-review-${{ github.event.pull_request.number || github.event.issue.number || inputs.number || github.event.workflow_run.pull_requests[0].number || github.run_id }} cancel-in-progress: false jobs: @@ -40,4 +43,10 @@ jobs: with: script: | const { run } = require('./.github/scripts/codex-review.cjs'); - await run({ github, context, core }); + if (context.eventName === 'workflow_run') { + for (const pr of context.payload.workflow_run.pull_requests) { + await run({ github, context: { ...context, payload: { pull_request: { number: pr.number } } }, core }); + } + } else { + await run({ github, context, core }); + } diff --git a/docs/MERGE_POLICY.md b/docs/MERGE_POLICY.md index c469b93e..4bf5a70c 100644 --- a/docs/MERGE_POLICY.md +++ b/docs/MERGE_POLICY.md @@ -16,7 +16,7 @@ GitHub metadata and executes only the script from trusted main, never PR code. Request `@codex review` once per head. If it remains hung for an hour, or the integration explicitly refuses the review, use the maintainer-authorized Claude Opus 5.5 fallback. Fix findings and repeat review until clean. Publish -its clean result as a COMMENT review on the exact commit, as `HemSoft`, with +its clean result as a COMMENT review on the exact commit, as the trusted maintainer, GitHub user ID `8227352`, with this first-line receipt, followed by the review's scope and evidence: ```text @@ -24,13 +24,18 @@ this first-line receipt, followed by the review's scope and evidence: ``` Use `unavailable` instead of `hung` for an explicit integration refusal. -The gate verifies the review's actual commit and live maintainer permission. -The latest HemSoft review on that head must carry the clean receipt. It cannot +The gate verifies the review's actual commit, stable author ID and live +maintainer permission using the author's current login. +The latest review from that user on that head must carry the clean receipt. It cannot override Codex's completed findings. This is a reviewed alternative to a hung integration, not a switch that skips review. Unresolved threads still block merge through native conversation protection. -The gate refreshes on PR head changes, bot comment updates and reviews. A +The gate refreshes on PR head changes, bot comment updates, reviews and CI +completion. It publishes to both the reviewed head and its current test merge +revision, and rejects a concurrent change to either. It revokes prior success +before evaluation, checks that the triggering edit or deletion is visible, +and reads mutable evidence again immediately before publishing a verdict. A maintainer can refresh existing evidence with `gh workflow run codex-review.yml -f number=PR_NUMBER`. This does not request a review or change the evidence requirements. There is no polling or schedule. From 785b19db5c244086ba6cb80d41719fda9abc72f7 Mon Sep 17 00:00:00 2001 From: HemSoft Date: Thu, 22 Oct 2026 23:25:59 -0400 Subject: [PATCH 3/5] fix: defer review status during trusted-main bootstrap (#237) --- .github/workflows/codex-review.yml | 7 ++++++- docs/MERGE_POLICY.md | 5 +++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/.github/workflows/codex-review.yml b/.github/workflows/codex-review.yml index 162b654c..911b17a2 100644 --- a/.github/workflows/codex-review.yml +++ b/.github/workflows/codex-review.yml @@ -42,7 +42,12 @@ jobs: - uses: actions/github-script@v8 with: script: | - const { run } = require('./.github/scripts/codex-review.cjs'); + const helper = './.github/scripts/codex-review.cjs'; + if (!require('node:fs').existsSync(helper)) { + core.notice('Bootstrap: trusted main does not contain the review helper yet; no review status issued'); + return; + } + const { run } = require(helper); if (context.eventName === 'workflow_run') { for (const pr of context.payload.workflow_run.pull_requests) { await run({ github, context: { ...context, payload: { pull_request: { number: pr.number } } }, core }); diff --git a/docs/MERGE_POLICY.md b/docs/MERGE_POLICY.md index 4bf5a70c..d607594e 100644 --- a/docs/MERGE_POLICY.md +++ b/docs/MERGE_POLICY.md @@ -42,6 +42,11 @@ review or change the evidence requirements. There is no polling or schedule. ## Required check setup +During the initial PR, review events can run its workflow before the helper +exists on trusted main. That bootstrap case emits a notice and issues no +review status. It cannot satisfy a required `codex-review` check. Never load +the helper from a PR branch to work around the bootstrap. + After the review workflow has merged to main, preview the additive settings: ```sh From 7b2cb4775369e83b9a55bcf634288a419172d634 Mon Sep 17 00:00:00 2001 From: HemSoft Date: Fri, 23 Oct 2026 00:03:21 -0400 Subject: [PATCH 4/5] fix: reject malformed review summaries and unrelated triggers (#237) --- .github/scripts/codex-review.cjs | 24 +++++++++++++++++++----- .github/scripts/codex-review.test.cjs | 21 +++++++++++++++++++-- .github/workflows/codex-review.yml | 8 +++++++- 3 files changed, 45 insertions(+), 8 deletions(-) diff --git a/.github/scripts/codex-review.cjs b/.github/scripts/codex-review.cjs index 3fa7bc46..50783f4d 100644 --- a/.github/scripts/codex-review.cjs +++ b/.github/scripts/codex-review.cjs @@ -9,11 +9,14 @@ function codexVerdict(comments, head) { const summaries = comments.filter(comment => comment.user?.login === CODEX && comment.user?.type === 'Bot') .sort((a, b) => Date.parse(b.updated_at) - Date.parse(a.updated_at)); for (const comment of summaries) { - const match = comment.body?.match(//); - if (!match) continue; + if (!comment.body?.includes('/); + if (!match) return malformed; let review; - try { review = JSON.parse(match[1]).code_review; } catch { continue; } - if (review?.commit_sha !== head) continue; + try { review = JSON.parse(match[1]).code_review; } catch { return malformed; } + if (!review || typeof review.commit_sha !== 'string') return malformed; + if (review.commit_sha !== head) continue; if (review.status !== 'completed') return { state: 'pending', description: 'Current-head Codex review is running' }; if (review.has_any_findings === false && review.has_unresolved_findings === false) { return { state: 'success', description: 'Codex reviewed this exact head with no findings', target_url: comment.html_url }; @@ -37,6 +40,16 @@ function fallbackReceipt(review, head) { } catch { return false; } } +function eligibleEvent(context) { + if (context.eventName === 'issue_comment') { + const comment = context.payload.comment; + return !!context.payload.issue?.pull_request && comment?.user?.login === CODEX && + comment.user.type === 'Bot' && !!comment.body?.includes('' }], head).state, 'pending'); + assert.equal(codexVerdict([{ ...summary(clean), body: '' }], head).state, 'failure'); }); test('a later running or failed result replaces an earlier clean result', () => { @@ -104,3 +104,20 @@ test('a stale API snapshot cannot ignore the triggering edit or deletion', async } assert.equal(triggerIncluded({ payload: { action: 'deleted', comment: old } }, [], []), true); }); + +test('a newer malformed summary cannot resurrect an older clean result', () => { + for (const body of ['', '', + '', '`, updated_at: '2026-10-22', html_url: 'https://github.com/review' + id: 42, node_id: 'comment42', user, body: ``, updated_at: '2026-10-22', html_url: 'https://github.com/review' }); test('only a completed clean bot verdict for the exact head passes', () => { @@ -40,16 +40,24 @@ test('fallback requires the named maintainer, exact head, model and explicit cle const merge = 'c'.repeat(40); function client({ moved = false, mergeMoved = false, reviews = [], comments = [], - freshComments = comments, freshReviews = reviews, permission = 'admin', payload = { pull_request: { number: 1 } } } = {}) { + freshComments = comments, freshReviews = reviews, permission = 'admin', priorStatuses = [], statusError, payload = { pull_request: { number: 1 } } } = {}) { const statuses = []; let reads = 0; let commentReads = 0; let reviewReads = 0; - const github = { paginate: async method => method(), rest: { + const github = { graphql: async (_query, { id }) => { + const comment = [...freshComments, ...comments].find(item => item.node_id === id); + return { node: comment ? { body: comment.body, updatedAt: comment.updated_at, + author: { login: 'chatgpt-codex-connector', __typename: 'Bot' }, + lastEditedAt: comment.editedAt ?? null, editor: comment.editor ?? null } : null }; + }, paginate: async method => method(), rest: { pulls: { get: async () => { const later = reads++ > 0; return { data: { state: 'open', base: { ref: 'main' }, head: { sha: moved && later ? 'b'.repeat(40) : head }, merge_commit_sha: mergeMoved && later ? 'd'.repeat(40) : merge } }; }, listReviews: async () => reviewReads++ ? freshReviews : reviews }, issues: { listComments: async () => commentReads++ ? freshComments : comments }, - repos: { createCommitStatus: async value => statuses.push(value), getCollaboratorPermissionLevel: async () => ({ data: { permission } }) } + repos: { listCommitStatusesForRef: async () => priorStatuses, createCommitStatus: async value => { + if (statusError?.(value)) throw new Error('status write failed'); + statuses.push(value); + }, getCollaboratorPermissionLevel: async () => ({ data: { permission } }) } } }; return { github, statuses, context: { repo: { owner: 'hemsoft-dev', repo: 'codexbar' }, payload }, core: { info() {} } }; } @@ -58,18 +66,19 @@ const result = fixture => fixture.statuses.at(-1).state; test('a concurrent head or merge change cannot publish success', async () => { for (const change of [{ moved: true }, { mergeMoved: true }]) { const fixture = client({ ...change, comments: [summary(clean)] }); await run(fixture); - assert.equal(fixture.statuses.length, 2); - assert.ok(fixture.statuses.every(status => status.state === 'pending')); + const gates = fixture.statuses.filter(item => item.context === 'codex-review'); + assert.equal(gates.length, 2); + assert.ok(gates.every(status => status.state === 'pending')); } }); test('missing evidence publishes pending on both head and test merge', async () => { const fixture = client(); await run(fixture); - assert.deepEqual(fixture.statuses.slice(-2).map(status => status.sha), [head, merge]); + assert.deepEqual(fixture.statuses.slice(-2).map(status => status.sha), [merge, head]); assert.equal(result(fixture), 'pending'); }); test('clean review publishes success on head and test merge', async () => { const fixture = client({ comments: [summary(clean)] }); await run(fixture); - assert.deepEqual(fixture.statuses.slice(-2).map(status => [status.sha, status.state]), [[head, 'success'], [merge, 'success']]); + assert.deepEqual(fixture.statuses.slice(-2).map(status => [status.sha, status.state]), [[merge, 'success'], [head, 'success']]); }); test('fallback also verifies live maintainer permission', async () => { const allowed = client({ reviews: [fallback] }); await run(allowed); assert.equal(result(allowed), 'success'); @@ -121,3 +130,46 @@ test('unrelated comments and reviews cannot revoke a clean gate', async () => { } assert.equal(eligibleEvent({ eventName: 'issue_comment', payload: { issue: { pull_request: {} }, comment: summary(clean) } }), true); }); + +const watermark = (id, at, state = 'success') => ({ context: 'codex-review-evidence', state, + creator: { login: 'github-actions[bot]', type: 'Bot' }, description: JSON.stringify({ id, at }) }); +test('a collaborator edit is not trusted on either an event or manual refresh', async () => { + for (const eventName of ['issue_comment', 'workflow_dispatch']) { + const forged = { ...summary(clean), editedAt: '2026-10-22', editor: { login: 'writer', __typename: 'User' } }; + const fixture = client({ comments: [forged], payload: { issue: { number: 1, pull_request: {} }, comment: forged, action: 'edited' } }); + fixture.context.eventName = eventName; + await run(fixture); assert.equal(result(fixture), 'failure'); + } + const restored = { ...summary(clean), editedAt: '2026-10-22', editor: { login: 'chatgpt-codex-connector', __typename: 'Bot' } }; + const fixture = client({ comments: [restored] }); await run(fixture); assert.equal(result(fixture), 'success'); +}); +test('deletion latches failure instead of reviving an older clean summary on refresh', async () => { + const deleted = { ...summary({ ...clean, has_any_findings: true }), id: 43, updated_at: '2026-10-23' }; + const fixture = client({ comments: [summary(clean)], payload: { issue: { number: 1, pull_request: {} }, comment: deleted, action: 'deleted' } }); + fixture.context.eventName = 'issue_comment'; + await run(fixture); assert.equal(result(fixture), 'failure'); + const latch = fixture.statuses.find(item => item.context === 'codex-review-evidence'); + const refresh = client({ comments: [summary(clean)], priorStatuses: [{ ...latch, creator: { login: 'github-actions[bot]', type: 'Bot' } }] }); + await run(refresh); assert.equal(result(refresh), 'failure'); +}); +test('a missing recorded summary fails even if its deletion event was not delivered', async () => { + const fixture = client({ comments: [summary(clean)], priorStatuses: [watermark(43, '2026-10-23')] }); + await run(fixture); assert.equal(result(fixture), 'failure'); +}); +test('a newer authenticated bot update recovers a deletion latch', async () => { + const fixture = client({ comments: [{ ...summary(clean), id: 44, updated_at: '2026-10-24' }], + priorStatuses: [watermark(43, '2026-10-23', 'failure')] }); + await run(fixture); assert.equal(result(fixture), 'success'); +}); +test('partial pending publication cannot retain evaluated merge success', async () => { + const fixture = client({ comments: [summary(clean)], statusError: value => value.sha === head }); + await assert.rejects(run(fixture), /status write failed/); + assert.deepEqual(fixture.statuses.map(item => [item.sha, item.state]), [[merge, 'pending']]); +}); +test('workflow-run dispatch preserves an inherited Actions repo getter', async () => { + const fixture = client({ comments: [summary(clean)] }); + const original = Object.assign(Object.create({ get repo() { return { owner: 'hemsoft-dev', repo: 'codexbar' }; } }), + { eventName: 'workflow_run', payload: {} }); + fixture.context = workflowRunContext(original, 1); + await run(fixture); assert.equal(result(fixture), 'success'); +}); diff --git a/.github/workflows/codex-review.yml b/.github/workflows/codex-review.yml index 747bccb5..46ef189b 100644 --- a/.github/workflows/codex-review.yml +++ b/.github/workflows/codex-review.yml @@ -25,19 +25,18 @@ permissions: issues: read statuses: write -concurrency: - group: codex-review-${{ github.event.pull_request.number || github.event.issue.number || inputs.number || github.event.workflow_run.pull_requests[0].number || github.run_id }} - cancel-in-progress: false - jobs: review: if: >- (github.event_name != 'issue_comment' || (github.event.issue.pull_request && github.event.comment.user.login == 'chatgpt-codex-connector[bot]' && - github.event.comment.user.type == 'Bot' && - contains(github.event.comment.body, '