Repository navigation
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00d546f9e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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 }); |
There was a problem hiding this comment.
When the Codex summary or fallback review is edited or deleted after listComments/listReviews returns—for example, when a clean summary changes to running or findings—this check verifies only that the head SHA is unchanged and then publishes the cached success. GitHub treats the latest state for a commit-status context as authoritative, so the PR can become mergeable until the queued event updates the status. Ensure the triggering mutation is included in the evaluated snapshot and revalidate the mutable evidence immediately before publishing success.
AGENTS.md reference: AGENTS.md:L12-L16
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c343cb2: revoke prior success before evaluation, require the triggering edit/deletion to be visible, reread mutable comments/reviews before publishing, and fail closed on changed evidence. Added edit/deletion and stale event snapshot regressions.
| // 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' || |
There was a problem hiding this comment.
The repository's current GitHub owner/login is hemsoft-dev, so review objects authored by that maintainer expose user.login as hemsoft-dev, not the pre-rename HemSoft. This strict comparison rejects every genuine fallback receipt before the live permission check, leaving the required gate permanently pending whenever Codex is hung or unavailable; use the current canonical login or derive the trusted reviewer identity from repository configuration.
AGENTS.md reference: AGENTS.md:L12-L16
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Live REST evidence disproves the rename premise: gh api user and gh api users/HemSoft both return login HemSoft, id 8227352; PR #249 review 5623312122 has that same author ID/login. hemsoft-dev owns the repository but is a different identity. Hardened in c343cb2 by trusting stable user ID 8227352 and using the actual current login for the live permission lookup. Rename and impostor regressions pass.
| 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 }); |
There was a problem hiding this comment.
In the inspected .github/workflows/ci.yml, the five normal required checks run on pull_request, whose GITHUB_SHA is the test merge commit. GitHub documents that when the test merge commit has a status, that commit must pass the required checks. This call publishes codex-review only on pr.head.sha, so after CI reports on the merge commit, that evaluated commit never receives the required review context and otherwise-ready PRs remain blocked. Validate evidence against the head, but publish and race-check the current merge_commit_sha as well.
AGENTS.md reference: AGENTS.md:L12-L16
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c343cb2: review is evaluated against the head and status is published to both head and corresponding test merge revision. Both SHAs are reread before a final verdict; a changed merge revision fails closed. CI completion refreshes metadata to handle initially uncomputed merge revisions. Added merge race and dual-publication regressions.
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 785b19db5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const match = comment.body?.match(/<!-- codex-review-state:v1 (\{[^\n]*\}) -->/); | ||
| if (!match) continue; | ||
| let review; | ||
| try { review = JSON.parse(match[1]).code_review; } catch { continue; } |
There was a problem hiding this comment.
When the newest bot summary for this head contains the state marker but malformed JSON, this continue falls through to an older clean summary and returns success. That makes the required gate pass even though the latest purported result cannot be validated; once the newest candidate summary is encountered, malformed content should yield pending or failure instead of resurrecting stale evidence.
AGENTS.md reference: AGENTS.md:L12-L16
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7b2cb47. The newest trusted summary marker now fails closed if its JSON or review structure is malformed, instead of reviving older clean evidence. Tests cover broken JSON, missing/null review state and truncated markers. Thirteen policy tests and actionlint pass.
|
|
||
| jobs: | ||
| review: | ||
| if: github.event_name != 'issue_comment' || github.event.issue.pull_request |
There was a problem hiding this comment.
On a public PR, this condition accepts every issue_comment actor, while run() unconditionally publishes a pending status before determining whether Codex evidence changed. Any user can therefore enqueue uncancelled runs by posting or editing ordinary comments; every run temporarily revokes a valid gate, and an API or rate-limit failure after that revocation leaves the PR blocked until another successful refresh. Restrict metadata-triggered runs to comments that can actually change the Codex verdict.
AGENTS.md reference: AGENTS.md:L12-L16
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7b2cb47. Workflow and handler both filter comment triggers to structured Codex-bot summaries. Review triggers are limited to the trusted maintainer ID. Unrelated public comments/reviews return before any API read or status revocation; a regression asserts no statuses are emitted. Thirteen policy tests and actionlint pass.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b2cb47753
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // 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') |
There was a problem hiding this comment.
When a collaborator with write access edits a Codex-authored comment, this filter still sees comment.user as the original bot and accepts the edited JSON as trusted evidence; GitHub explicitly allows write-access users to edit anyone's issue and pull-request comments. Such a collaborator can replace a findings summary with an exact-head clean payload, and the resulting issue_comment run publishes the required GitHub Actions status. Authenticate the mutation actor or use evidence whose contents cannot be rewritten by repository collaborators.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5b539c4. The gate now reads GraphQL IssueComment author, last editor, edit time, body and update time, and requires an authenticated bot mutation. A collaborator-authored edit fails on both its event and later manual/CI refreshes. Regression tests cover forged and genuine bot edits. Nineteen policy tests pass. Awaiting a clean current-head review before resolving this thread.
| 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; |
There was a problem hiding this comment.
When the deleted bot comment is the newest current-head verdict and an older clean summary for the same head remains, this branch accepts the deletion and codexVerdict immediately resurrects that older success. Because GitHub permits anyone with write access to delete comments, deleting a newer findings or running summary can therefore make the required gate pass; treat deletion of review evidence as non-clean rather than reevaluating older evidence.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5b539c4. Deletion now records an Actions-authored, head-bound failure watermark. Later refreshes preserve that latch instead of accepting an older summary. A missing recorded summary also fails if its deletion event was not delivered. Only a newer authenticated bot update recovers. Regression tests cover deletion, refresh and recovery. Nineteen policy tests pass. Awaiting a clean current-head review before resolving this thread.
| 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 |
There was a problem hiding this comment.
Because this concurrency group is applied at workflow level, an ordinary comment or untrusted review enters the same per-PR group before the job-level if skips it. GitHub documents that a group permits only one pending run and that a newer run cancels the existing pending run, even with cancel-in-progress: false; therefore, if a findings edit queues behind an active clean evaluation and a user then posts an ordinary comment, the skipped ordinary run can cancel the only revalidation while the active run publishes cached success. Assign ineligible events a different group or move concurrency below the eligibility check.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5b539c4. Moved concurrency to the eligible job, after its if condition, so unrelated public comments/reviews cannot enter the review group. Also enabled GitHub's documented queue: max mode so eligible pending events are retained. Local actionlint suppresses only its unsupported queue-key diagnostic; all remaining validation passes. Nineteen policy tests pass. Awaiting a clean current-head review before resolving this thread.
| for (const sha of shas) { | ||
| await github.rest.repos.createCommitStatus({ owner, repo, sha, context: CONTEXT, ...verdict }); |
There was a problem hiding this comment.
When the first status write succeeds for head but the second write fails for merge—for example, from a transient API or rate-limit error—the function aborts while the test merge commit retains its previous successful codex-review status. GitHub evaluates the test merge commit whenever it has statuses, as described in its required-check troubleshooting table, so the PR can remain mergeable despite failed revalidation; publish the pending revocation to the merge SHA first or otherwise ensure a partial write cannot leave the evaluated commit successful.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5b539c4. Pending revocation now writes the test merge revision before the head. A regression injects a failure on the subsequent head write and verifies that the evaluated merge revision is already pending. Nineteen policy tests pass. Awaiting a clean current-head review before resolving this thread.
| 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 }); |
There was a problem hiding this comment.
For every completed CI run associated with a PR, spreading context into a plain object drops the prototype getter that provides context.repo, so run() fails at const { owner, repo } = context.repo before refreshing any status. The Actions Toolkit defines repo as a class getter rather than an own context field, which object spread does not copy; pass repo: context.repo explicitly or preserve the original context prototype so the post-CI refresh can execute.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5b539c4. Added workflowRunContext(), which explicitly copies repo: context.repo before replacing the payload. A regression uses a prototype getter like the Actions toolkit and exercises the complete status refresh. Nineteen policy tests pass. Awaiting a clean current-head review before resolving this thread.
|
@codex review |
HemSoft
left a comment
HemSoft
left a comment
There was a problem hiding this comment.
Cross-model review (Claude Opus 5.5 via T3)
Reviewed head: 5b539c47e16e46023edf146a9b39844bbd460b87
Base tip and merge-base: f1b0d230e4e07aba8361c44ceef4152673752020
Reviewer: Claude Opus 5.5 at xhigh effort, standing in for connected Codex. The Codex summary has shown running for 5b539c4 since 00:16 EDT on 2026-10-23.
Result: two P2 findings, posted inline. This review is not clean and carries no fallback receipt.
Rechecked at this head: all ten earlier Codex findings are addressed in code and tests:
- revoke, reread and fingerprint before publishing, with a final head and merge recheck;
- the numeric maintainer ID with a live current-login permission check;
- publication to the head and the test merge;
- a newer malformed summary fails closed;
- eligibility filtering before revocation and concurrency;
- GraphQL author and last-editor authentication;
- the deletion watermark;
- job-level concurrency with
queue: max; - revoking the merge before the head;
- the
workflow_runrepo getter.
The watermark's protection against non-bot writers is covered by the first inline finding.
What I ran or checked:
- The exact head, exported to a temporary directory, with Node 26.12.1:
node --test .github/scripts/codex-review.test.cjspassed 19 of 19. - actionlint 1.7.12 reported one diagnostic, the
queuekey on line 39, and was clean with only that diagnostic ignored.queue: maxis documented for job-level concurrency: up to 100 pending runs, and not allowed withcancel-in-progress: true(syntax). git diff --checkwas clean.- The settings preview made read-only GETs and printed the six contexts with app 15368 and
strict: false. - Live data matches the parser. The real summary on #250 and the completed summaries on #236 and #247–#249 use the marker and field shape the parser expects. GraphQL returns author and editor
Bot/chatgpt-codex-connector, andupdatedAtand the body match REST exactly. - CI 39899324172 on this head is green across all five jobs, including the "Test Codex review policy" step. The diff changes no Rust or C# product source.
- The review-event bootstrap emits a notice and no status. It checks out main, as seen in run 39899393578.
Non-blocking note: the permission API maps maintain to write in permission, so the 'maintain' entry in codex-review.cjs:177 can never match. This has no effect today, because user 8227352 resolves to HemSoft with role_name: admin. Use role_name if a maintain-role maintainer should qualify.
Limits: I made no settings writes and ran no forged-status proof, so both findings rest on run logs, live read-only API responses, docs and inspection. Locally I used Node 26 and CI uses Node 20. Because of the bootstrap, the gate helper has not yet run against the live API. Activation and the disposable blocked-PR test remain the post-merge steps listed in the PR body.
| pull_request_review: | ||
| types: [submitted, edited, dismissed] |
There was a problem hiding this comment.
pull_request_review loads the workflow file from refs/pull/N/merge, not from main (event reference). Same-repository PRs get the requested write token. This already happens on this PR: main f1b0d23 has no codex-review.yml, yet run 39899393578 ran this file for a review on 5b539c4 and its log shows Statuses: write.
Trigger: a same-repository PR edits this file's if: or inline script: to call createCommitStatus with context: 'codex-review', state: 'success' for its head and merge_commit_sha. Anyone then submits a review, and an author can comment-review their own PR. Checking out main doesn't help, because the PR-controlled YAML decides what runs. That status comes from GitHub Actions (app 15368), the source require-merge-checks.py binds codex-review to. After activation, the gate passes without Codex or a fallback receipt.
The root cause is broader. Any same-repository workflow, for example on: push with permissions: statuses: write, can write the same context. It can also write a newer codex-review-evidence success that clears the deletion/tamper latch, because codex-review.cjs:103-104 trusts any github-actions[bot] creator. The repository has a non-admin write collaborator, so this isn't limited to admins, who are bound by enforce_admins.
Impact: a writer can self-certify review. This contradicts MERGE_POLICY.md:16, "executes only the script from trusted main, never PR code".
Fix: run privileged evaluation only on events whose workflow comes from the default branch. For example, use an unprivileged pull_request_review relay with permissions: {} that triggers this evaluator through workflow_run, or drop the trigger and refresh with workflow_dispatch. Then issue codex-review and the watermark with an identity that only default-branch jobs can obtain, such as a GitHub App token from a main-restricted environment, and bind the required check to that App ID. If that isn't wanted, document that write collaborators are trusted. To verify, check that no run of the privileged job uses a refs/pull/* workflow source.
Evidence: the run log, the docs and code inspection. I did not run a forged-status proof against the repository.
| if args.apply: | ||
| # This endpoint changes only required statuses. It never replaces review, | ||
| # conversation, admin-enforcement or bypass settings. | ||
| result = api(path, "PATCH", payload) |
There was a problem hiding this comment.
--apply cannot enable required checks from main's current protection
This always PATCHes .../protection/required_status_checks. Main is protected but has no required status checks yet. At 01:37 EDT today, read-only gh api repos/hemsoft-dev/codexbar/branches/main/protection/required_status_checks returned 404 Required status checks not enabled, and GET .../protection has no required_status_checks key. The update endpoint works on that same sub-resource. A public report of PATCH in this state returned the same Required status checks not enabled error. The accepted answer was to enable checks through the full protection update first (community #24424).
Impact: the documented post-merge activation (MERGE_POLICY.md:64-76) is expected to stop with CalledProcessError and enable none of the six checks. The checked claim that the PR provides an additive helper therefore isn't supported yet. It fails closed and weakens nothing.
Fix: when required_status_checks is absent, enable checks with an update that touches only status-check fields. Options are GraphQL updateBranchProtectionRule with requiresStatusChecks and requiredStatusChecks, or a PUT .../protection that sends every current setting back. Keep this PATCH for the case where checks already exist, then read back the six contexts and app IDs.
Evidence: the live read-only 404 and the docs. I did not send the PATCH, because this review makes no settings writes.
|
Closing at maintainer direction. The review gate is deferred until the planned SFL reviewer goes live; its App will provide the required review status. Issue #237 remains open and blocked. No new App, branch protection or ruleset settings were created or changed. |
Summary
Adds an exact-head
codex-reviewstatus backed by the connected bot's structured verdict. Missing, running, stale, malformed or non-clean results cannot pass. A clean Claude Opus fallback requires a review on the actual head from the named maintainer and a structured receipt. It cannot override Codex findings.The workflow reads GitHub metadata and runs only trusted-main code. It does not request reviews, poll, promote PRs or merge. Adds an additive settings helper and documents the five normal CI checks plus the review status. Package stays conditional to avoid permanently pending path-filtered checks.
Acceptance Criteria
codex-review, sourced to the actual GitHub Actions app.Definition of Done
node --test .github/scripts/codex-review.test.cjs, workflow lint,cargo fmt --all --check, settings preview andgit diff --checkpass.Validation
Current implementation authenticates the summary author and last editor through GraphQL, records an Actions-authored evidence watermark, and preserves a deletion/tamper failure across later refreshes until a newer authenticated bot update. It revokes the test merge status before the head status. Unrelated events are filtered before job concurrency, and queued eligible events are retained. The CI-completion adapter explicitly preserves Actions Context's repo getter.
Nineteen Node tests pass, including collaborator edits, persistent deletion, recovery by a newer bot update, partial status-write failure and workflow-run dispatch. Workflow lint passes with only the installed actionlint 1.7.12 unknown-key diagnostic for GitHub's documented
queue: maxfield suppressed. See GitHub concurrency syntax.git diff --checkpasses. The trusted-main bootstrap issues no status until its helper is present and never loads PR code.The settings preview identified GitHub Actions app ID 15368 and the six intended contexts. The helper patches only required statuses and preserves other protection settings. Review-policy tests run in normal CI's build job.
No Rust or C# product source changed. Rust Clippy and workspace tests passed for the identical product tree in the preceding merged PR; the full repository CI runs again here. The new workflow is bootstrapped from trusted main and therefore begins operating after merge. Settings activation is an explicit post-merge rollout step, not a pre-merge claim.
Risks and deferred work
Live protection must be activated after merge. The mission retains ownership of that step and the disposable blocked-PR test. A failed or missing review remains a blocked merge; no admin bypass is allowed.
Closes #237