Skip to content

fix(ci): fail closed on incomplete review evaluation - #147

Open
lamemustafa wants to merge 8 commits into
masterfrom
tapish-codex/review-gate
Open

fix(ci): fail closed on incomplete review evaluation#147
lamemustafa wants to merge 8 commits into
masterfrom
tapish-codex/review-gate

Conversation

@lamemustafa

@lamemustafa lamemustafa commented Aug 17, 2026

Copy link
Copy Markdown
Owner

Summary

  • Preserves Round 1 fail-closed evaluation, transient GitHub retry, distinct blocking/evaluation exits, and PR-level Codex finding detection.
  • Preserves Round 2's exact minimizedReason === "resolved" predicate.
  • Removes the issue_comment trigger, its PR-head evaluator coupling, and the comment-specific publisher job.
  • Adds 15-minute trusted scheduled reconciliation for eligible open PRs and publishes the exact Review gate context to each captured PR head.
  • Shares one dependency-free GitHub CLI retry helper between evaluator reads and check publication.

Root Cause / Decision Record

Round 2 let an issue_comment evaluation execute PR-authored scripts before a privileged job published that result as trusted. It also lacked publish retry and could not observe Hide → Resolved. The event mechanism is removed rather than extended.

The scheduled job checks out github.event.repository.default_branch, never checks out or installs a PR tree, and executes only the default branch's API-only evaluator and publisher. Exit 0 maps to success, exit 1 to failure, and exit 2 to action_required.

Schedule: */15 * * * *. Fifteen minutes keeps late-finding exposure in minutes, not hours, while avoiding continuous Actions/API churn. GitHub may delay scheduled starts, so this is nominal rather than an instant-clear promise. Hide → Resolved is picked up by the next scheduled run.

Work is capped at 25 eligible PRs per run; cap hits are logged. Only open, non-draft, same-repository PRs are evaluated.

Permission and trigger boundary:

  • Workflow-level permissions remain contents: read and pull-requests: read.
  • Only scheduled-review-gate receives checks: write.
  • Existing pull_request, pull_request_review, and pull_request_review_comment trigger definitions are unchanged.
  • No pull_request_target or issue_comment path exists.

Fork limitation:

  • Scheduled reconciliation logs and skips fork PRs; it does not write required checks to external heads.
  • A late PR-level finding on a fork can leave an earlier head check unchanged until another supported head-associated run or maintainer action.
  • This PR does not imply fork coverage.

Deleted-finding disposition:

  • A deleted Codex PR comment is absent from current GitHub API state. Keeping it blocking requires durable repository-owned history.
  • Deletion is an overt, logged, destructive action by an actor who already has write access, unlike the removed silent privilege escalation.
  • Deferred with scope and acceptance criteria to Track deleted Codex PR findings with durable review-gate state #148.

Scope

  • Runtime extension behavior: none.
  • Workflow: trusted 15-minute reconciliation; removed comment-event machinery.
  • Scripts: shared retry helper, bounded PR selection, exact-head check publication, corrected scheduled-clear remedy.
  • Tests: minimization, Round 1 retry/exit/finding coverage, scheduled selection, mapping/context name, and publisher retry success/exhaustion.
  • Out of scope: extension behavior, manifest permissions, portal flows, dependencies, release claims, and durable deleted-comment state.

Net removal:

  • Captured Round 2 baseline: 6 files, +779/−39 (818 changed lines).
  • Round 3 delta: +424/−426, net −2 lines.
  • Final PR diff: 7 files, +775/−37 (812 changed lines), six changed lines smaller than the captured baseline.

Pack Workflow Preflight

  • pnpm workflow:preflight passed from clean head 01d6b2464d682123ca702cf14caa6ae9fa010bc4.
  • This PR was opened from a Pack branch, not master.
  • Latest master Pack AGENTS guidance was checked.
  • Required Pack privacy/review/verification checklist remains visible.
  • origin/master at 7428f51 was merged as 5e409c3; no rebase was used.

Sanchika Adoption Gate

  • This PR does not consume @sanchika/* packages or copied Sanchika guidance.
  • This PR does not require sanchika/docs/adoption-pack.md or ComplyEaze and Axal completion evidence.
  • This PR does not import parent or Sanchika source paths.

Privacy And Data-Flow Impact

  • No new browser or host permissions.
  • No extension network calls.
  • The only write is a repository-internal GitHub check run from trusted default-branch code.
  • No analytics, telemetry, credentials, session material, GST files, taxpayer data, or public product copy.
  • Fork PRs are skipped and logged.

Sensitive Surface Review

  • Portal target binding, downloads, and service-worker behavior are unchanged.
  • No taxpayer identifiers, portal HTML, raw portal URLs, downloaded files, or workstation paths are present in the diff.
  • No PR-authored code runs in the job with checks: write.

Chrome Web Store Impact

  • No listing, permission, privacy declaration, or release-posture change.
  • Build/ZIP evidence is local verification only, not store-submission or release-readiness sign-off.
  • PR title uses Conventional Commits.

Verification

All final commands exited 0:

  • pnpm install --frozen-lockfile
  • node scripts/run-dependency-audit.mjs — no known vulnerabilities
  • pnpm exec wxt prepare
  • pnpm exec vitest run — 105 files / 1705 tests
  • pnpm exec tsc --noEmit
  • pnpm exec eslint . --max-warnings 0
  • pnpm exec prettier --check .
  • pnpm exec actionlint
  • pnpm workflow:preflight
  • git diff --check
  • pnpm exec wxt build
  • node scripts/verify-extension-package.mjs .output/chrome-mv3
  • pnpm exec wxt zip
  • node scripts/verify-extension-zip.mjs

Vitest aggregate footer:

 Test Files  105 passed (105)
      Tests  1705 passed (1705)
   Start at  05:07:37
   Duration  153.93s (transform 2.28s, setup 0ms, import 14.06s, tests 124.95s, environment 7ms)

The first exact ZIP verification attempt correctly failed because ignored .output held both the prior 0.5.0 and new 0.5.1 ZIPs. The stale generated 0.5.0 ZIP was removed; the rerun and headed-browser check passed.

PR #144 A/B with the exact requested command and expected head a89d806224ced0aee9d5f6cd3f36139baf289fb7:

  • Protected base 7428f51: exit 0, PR review gate passed for lamemustafa/pack#144.
  • Updated head: exit 1, names comment 5318064584, and prints the next-scheduled-run remedy.

Scheduled live boundary:

  • Selection, exact context/mapping, retry, cap logging, fork skip, permissions, and trusted checkout are locally covered.
  • GitHub loads scheduled definitions from the default branch, so the end-to-end scheduled run cannot execute until this workflow version merges.

Artifact Evidence

  • Clean pushed head: 01d6b2464d682123ca702cf14caa6ae9fa010bc4.
  • Local ignored ZIP: .output/complyeazepack-0.5.1-chrome.zip.
  • ZIP SHA-256: 0ced11a01372bee6d6aeff7eadfdfc2ec9071a30ce7c01109e407002ad74ffd6.
  • GitHub Actions for the latest head: pending/currently re-running.

PR Review Follow-Up

Thread/comment Disposition Commit or evidence
P1 evaluate comment-triggered gates from trusted code fixed ebd14c5; event removed; scheduled job checks out default branch
P1 retry PR-head check publication fixed ebd14c5; shared retry helper; transient-success/exhaustion tests
P2 keep deleted findings blocking rejected / linked follow-up #148; durable state required for a logged destructive action by a writer
P2 trigger recheck after resolving a finding fixed ebd14c5; 15-minute schedule and corrected remedy
CodeQL cache poisoning on schedule/workflow_dispatch linked follow-up; fresh thread left open #149; schedule route is unreachable, manual dispatch requires write access but may select external PR content
Round 1 attach verdict to PR head superseded by simpler fix scheduled publisher creates exact Review gate check on captured eligible head
Round 1 require Resolved reason fixed 44ce29b; Resolved clears; Off-topic and Outdated block

Screenshots

Not applicable; no UI changes.

DCO

  • All new commits include Signed-off-by: trailers.

Signed-off-by: Tapish Khandelwal <tapishkhandelwal13@gmail.com>
Signed-off-by: Tapish Khandelwal <tapishkhandelwal13@gmail.com>
@lamemustafa
lamemustafa marked this pull request as ready for review August 17, 2026 22:13

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4b9899dbb9

ℹ️ 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".

Comment thread .github/workflows/review-gate.yml Outdated
Comment thread scripts/check-pr-review-gate.mjs Outdated
Signed-off-by: Tapish Khandelwal <tapishkhandelwal13@gmail.com>
Signed-off-by: Tapish Khandelwal <tapishkhandelwal13@gmail.com>
Signed-off-by: Tapish Khandelwal <tapishkhandelwal13@gmail.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1806e9478b

ℹ️ 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".

Comment thread .github/workflows/review-gate.yml Outdated
Comment thread scripts/publish-review-gate-check.mjs Outdated
Comment thread .github/workflows/review-gate.yml Outdated
Comment on lines +15 to +16
issue_comment:
types: [created, edited, deleted]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep deleted findings blocking the gate

When a Codex PR-level finding is deleted, this deleted event runs only after the comment has disappeared from pullRequest.comments, so the evaluator sees no blocking comment and publishes a successful PR-head check even though no fixed, stale, rejected, or follow-up disposition was recorded. A maintainer can therefore clear the gate by deleting the finding; inspect the deleted event payload and keep matching automated findings blocking unless a durable disposition exists.

AGENTS.md reference: AGENTS.md:L92-L93

Useful? React with 👍 / 👎.

Comment thread .github/workflows/review-gate.yml Outdated
Signed-off-by: Tapish Khandelwal <tapishkhandelwal13@gmail.com>
Signed-off-by: Tapish Khandelwal <tapishkhandelwal13@gmail.com>
Signed-off-by: Tapish Khandelwal <tapishkhandelwal13@gmail.com>
Comment on lines +108 to +117
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1
with:
fetch-depth: 1
ref: ${{ github.event.repository.default_branch }}
persist-credentials: false

- name: Set up Node.js
uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020
with:
node-version: 22.13.0

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linked follow-up: #149. The reported schedule route does not execute this checkout because review-gate is gated by github.event_name != 'schedule'; the separate scheduled-review-gate job checks out default-branch code and never installs PR content. The workflow_dispatch route can execute a selected PR head with pnpm caching, but only after an actor with repository write access dispatches it, and that job has no write permissions. Per the Round 3 stop rule, this pre-existing manual-dispatch/cache boundary is recorded for isolated treatment rather than adding machinery or making a second push here.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 01d6b2464d

ℹ️ 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".

Comment on lines +84 to +88
"--strict-head-review",
"--required-review-author",
"chatgpt-codex-connector",
"--allow-missing-head-review",
"--expected-head-oid",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Include workflow preflight in scheduled gate verdicts

In .github/workflows/review-gate.yml, the event-backed required Review gate includes dependency installation and pnpm workflow:preflight, but scheduled reconciliation reaches this invocation and evaluates only check-pr-review-gate.mjs before publishing the same required check name. If the event run fails because the PR deletes required template content, breaks its lockfile, or otherwise fails preflight, the next schedule can publish success for that same head and supersede the failure for branch protection. Make the scheduled verdict include a trusted equivalent of every condition in the required job, or publish it under a separate required context.

AGENTS.md reference: AGENTS.md:L21-L22

Useful? React with 👍 / 👎.

Comment on lines +84 to +88
"--strict-head-review",
"--required-review-author",
"chatgpt-codex-connector",
"--allow-missing-head-review",
"--expected-head-oid",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve the head-review wait before publishing success

When a PR is opened or synchronized immediately before the 15-minute schedule and Codex has not reviewed the new head yet, this scheduled path passes --allow-missing-head-review without the event workflow's --wait-head-review-ms 180000; the evaluator therefore uses its zero-wait default and publishes a successful required check immediately. That removes the documented grace period and can make the new head mergeable before the automated review arrives. Preserve the wait or publish a non-success verdict while the required head review is still absent.

AGENTS.md reference: AGENTS.md:L56-L59

Useful? React with 👍 / 👎.

Comment on lines +60 to +64
const selected = eligible.slice(0, maxPrs);
if (eligible.length > maxPrs) {
console.warn(
`Review gate schedule cap hit: processing ${maxPrs} of ${eligible.length} eligible pull requests.`,
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Rotate capped reconciliation across all open PRs

When more than 25 eligible same-repository, non-draft PRs are open, every scheduled run rebuilds the same list and slices the same prefix, with no cursor or rotation. PRs beyond that prefix are therefore never reconciled while the first 25 remain open, so a late finding can leave an older successful check in place—or a correctly resolved finding can leave a stale failure indefinitely. Rotate the starting point, persist a cursor, or process all eligible PRs across runs.

AGENTS.md reference: AGENTS.md:L92-L93

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants