fix(ci): re-run PR gate on issue_comment and gate prepush frontend lint on changes - #1123
Conversation
… changes
The maintainer GUI-waiver comment ("not touching gui") never re-ran the
enforce-target gate because pull_request_target types do not include issue
comments. Add an issue_comment trigger so the waiver takes effect when
posted, resolving the PR number from the issue payload and falling the
checkout back to the default branch.
Also stop running lint:gui unconditionally in the local prepush hook: it now
runs only when the push touches gui/, mirroring doctor:gui:if-changed. CI
already gated GUI lint behind the changes filter; the local hook now matches.
|
✅ Deterministic PR hygiene checks passed. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds conditional GUI linting for pre-push checks. The PR-target workflow reruns for created or edited maintainer comments on pull requests. Shared hygiene helpers preserve hygiene sections in consolidated gate comments. ChangesGUI linting and hook integration
Issue-comment PR enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GitHub
participant PRTargetWorkflow
participant QualityMessages
participant PRHygieneWorkflow
GitHub->>PRTargetWorkflow: issue_comment created or edited
PRTargetWorkflow->>QualityMessages: rebuild gate comment
QualityMessages-->>PRTargetWorkflow: preserve existing hygiene section
PRHygieneWorkflow->>QualityMessages: merge hygiene status
QualityMessages-->>PRHygieneWorkflow: consolidated gate comment body
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ READY
|
…tests The CJS validator test asserted the exact base-SHA checkout ref and did not cover the new issue_comment trigger. Update the ref assertion to the fallback form and add a test pinning that a maintainer GUI-waiver comment re-runs the gate via the issue_comment event.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/enforce-pr-target.yml:
- Around line 16-19: Restrict the workflow job handling issue comments to events
where github.event.issue.pull_request is present, while preserving other event
triggers. Add a regression test covering an ordinary issue comment and obtain
the required explicit security review specified by MAINTAINERS.md.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9a5e67da-4509-4784-80b5-dde363d9497c
📒 Files selected for processing (8)
.github/workflows/enforce-pr-target.ymlCONTRIBUTING.mdpackage.jsonscripts/fixtures/lint-findings-exit.tsscripts/lint-gui-if-changed.tsscripts/setup-hooks.tstests/ci-workflows.test.tstests/helpers/enforce-pr-target-harness.ts
|
Maintainer screenshot waiver: this PR is not touching gui. The changed files do not modify the rendered dashboard, so a UI screenshot is not applicable. This waiver applies only to the screenshot gate. The PR should remain draft for now. The issue_comment job must be restricted to comments whose issue payload contains pull_request; otherwise every ordinary issue comment can start a PR-only workflow with a non-PR issue number. Please add the job-level event guard and a regression test for an ordinary issue comment being skipped. Because this changes a pull_request_target/issue_comment workflow boundary, the explicit workflow security review required by MAINTAINERS.md is also still needed. I will re-check the updated head and full CI after those are resolved. |
Wibias
changed the title
…olidate bot comments The issue_comment trigger fired for any comment on any issue from any user. Guard the enforce-target job so only maintainer (OWNER/COLLABORATOR/MEMBER) comments on actual PRs re-run the write-capable gate; a comment on a plain issue or from a contributor is skipped, with a defensive in-script re-check matching the job-level if. Also consolidate the PR gate and PR hygiene bot messages into the single opencodex-pr-gate comment. The hygiene workflow now writes its status block into the gate comment (preserving the gate section) instead of posting a second standalone message, and the gate rebuild preserves an existing hygiene block so neither workflow clobbers the other.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/pr-quality-messages.cjs:
- Around line 118-120: Update the hygiene-block regular expressions used by the
body match and existence check around HYGIENE_BLOCK_START/HYGIENE_BLOCK_END so
both delimiters match only when they occupy complete lines, including
appropriate line-boundary handling. Apply the same predicate consistently at
both locations, and add a regression test covering delimiter text embedded in a
filename-like hygiene line.
In @.github/workflows/enforce-pr-target.yml:
- Around line 284-293: Use the same per-PR concurrency group for the
comment-update workflows: update the concurrency configuration in
.github/workflows/enforce-pr-target.yml (lines 284-293) and
.github/workflows/pr-hygiene.yml (lines 129-150) to share one key, preserving
any existing PR-specific isolation. Ensure the shared group serializes updates
performed by the gate comment rebuild flow and the hygiene comment-update flow.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bf81c5a2-8a1f-4913-a9d4-4e4ea442bcad
📒 Files selected for processing (8)
.github/scripts/enforce-pr-target.test.cjs.github/scripts/pr-quality-messages.cjs.github/scripts/pr-quality-messages.test.cjs.github/workflows/enforce-pr-target.yml.github/workflows/issue-quality-tests.yml.github/workflows/pr-hygiene.ymltests/ci-workflows.test.tstests/helpers/enforce-pr-target-harness.ts
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
The issue-comment guard is now correct on head 732f296e: it restricts the job to maintainer comments on pull requests, repeats that check inside the trusted script, and covers both ordinary issues and non-maintainer comments. That resolves my previous workflow-boundary blocker.
One new merge blocker remains in the hygiene-comment consolidation. enforce-pr-target.yml and pr-hygiene.yml now both read and replace the same complete bot comment, but they run under different concurrency groups (enforce-pr-target-* and pr-hygiene-*). On the same PR event they can both read body A, independently build bodies B and C, and last-write-wins can restore stale gate/readiness state or drop the newly written hygiene block. The round-trip unit tests cover sequential rebuilds only; they do not make the cross-workflow read-modify-write atomic.
Please keep the hygiene status as its standalone comment in this PR (the smallest and safest fix), or move both mutations behind one serialized owner with a regression that proves an interleaved gate/hygiene update cannot lose either state. Simply sharing a concurrency group also needs care because the current hygiene workflow uses cancel-in-progress: true, which may cancel the gate mid-mutation. The standalone-comment approach avoids expanding this PR further.
After that is resolved and exact-head CI is green, the guarded issue_comment trigger and conditional GUI lint are acceptable from the workflow security perspective.
![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: ac8505d76c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…te comment concurrency Two CodeRabbit findings on the consolidated gate comment: - The hygiene block delimiters matched anywhere in the comment body. A contributor-controlled changed filename could embed delimiter text mid-line and corrupt the block boundary on the next rewrite. Anchor both delimiters to complete lines via a shared regex used by the existence check and the replacement, with a regression test for embedded delimiter text. - The gate and hygiene workflows each had their own per-PR concurrency group while both read-modify-write the same consolidated comment. A concurrent gate rebuild and hygiene update could run from stale snapshots and the last write would drop the other's section. Share one per-PR concurrency group between the two workflows and pin it in the workflow tests.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pr-hygiene.yml (1)
140-152: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove stale standalone hygiene comments after merging into the gate.
In
.github/workflows/pr-hygiene.yml, .github/workflows/pr-hygiene.yml lines 143-152 update the existing gate comment but leave any earliergithub-actions[bot]comment containingHYGIENE_MARKERin place. Add stale cleanup afterupdateCommentand add a regression test for the case where both comments already exist.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pr-hygiene.yml around lines 140 - 152, After updating the gate comment in the existing merge path, remove any other github-actions[bot] comments containing HYGIENE_MARKER, excluding the gateComment itself; preserve the updated gate comment and use the existing GitHub issue-comment deletion mechanism. Add a regression test covering both an existing gate comment and a stale standalone hygiene comment, verifying the stale comment is deleted after merging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/pr-hygiene.yml:
- Around line 140-152: After updating the gate comment in the existing merge
path, remove any other github-actions[bot] comments containing HYGIENE_MARKER,
excluding the gateComment itself; preserve the updated gate comment and use the
existing GitHub issue-comment deletion mechanism. Add a regression test covering
both an existing gate comment and a stale standalone hygiene comment, verifying
the stale comment is deleted after merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b69a00ee-2167-497b-b386-b50145e7bf12
📒 Files selected for processing (5)
.github/scripts/pr-quality-messages.cjs.github/scripts/pr-quality-messages.test.cjs.github/workflows/enforce-pr-target.yml.github/workflows/pr-hygiene.ymltests/ci-workflows.test.ts
…rifiable checklists Codex-bot review findings on the issue_comment trigger: - The checkout fell back to the repository default branch (main) on issue_comment, which can lag the integration branch the gate enforces. Fall back to dev (the gate's only allowed base) so comment-triggered runs evaluate with the gate's own current scripts. - issue_comment events carry no pull_request.head.sha, so eventHeadSha fell back to the live head and a completed checklist with no recorded completion head was accepted as attesting the current head. Pass an empty eventHeadSha on issue_comment so completionIsStale resets the checklist (fail closed) instead of promoting readiness from an unverifiable attestation. Regression tests cover both.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/enforce-pr-target.yml (1)
43-48: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuthorize comment reruns against
MAINTAINERS.md.Lines 43-48 and 164-168 treat
MEMBERandCOLLABORATORas maintainers. These GitHub author associations do not prove that the commenter is in the repository's canonical maintainer list. A non-maintainer with either association can start this write-capable gate and cause PR comment, label, title, and draft-state mutations.Use
parseMaintainerLogins()with the trusted checked-outMAINTAINERS.md, then requirecontext.payload.comment.user.loginto be in that list beforegithub.rest.pulls.get()or any mutation. Add a regression case for aMEMBERorCOLLABORATORabsent fromMAINTAINERS.md.Proposed guard
- const association = context.payload.comment?.author_association; - const isMaintainer = ["OWNER", "COLLABORATOR", "MEMBER"].includes( - association - ); + const commenter = context.payload.comment?.user?.login; + const isMaintainer = + typeof commenter === "string" && + readMaintainerLogins().includes(commenter);As per path instructions,
.github/**is a security boundary and maintainer-only issue-comment reruns must not weaken this control.Also applies to: 161-168
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/enforce-pr-target.yml around lines 43 - 48, Replace the issue-comment authorization checks in the workflow guards around the pull-request lookup and mutation steps with canonical maintainer validation: load the trusted checked-out MAINTAINERS.md through parseMaintainerLogins(), then require context.payload.comment.user.login to be present in the parsed list before github.rest.pulls.get() or any write operation. Do not treat MEMBER or COLLABORATOR as sufficient authorization, and add a regression case covering an associated user absent from MAINTAINERS.md.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/ci-workflows.test.ts`:
- Around line 2699-2708: Merge the duplicate comments properties in this test
fixture into one comments array, preserving both the maintainer “not touching
gui” waiver comment and the readinessComment fixture. Remove the second comments
key so the harness receives both entries and Biome no longer reports a duplicate
key.
---
Outside diff comments:
In @.github/workflows/enforce-pr-target.yml:
- Around line 43-48: Replace the issue-comment authorization checks in the
workflow guards around the pull-request lookup and mutation steps with canonical
maintainer validation: load the trusted checked-out MAINTAINERS.md through
parseMaintainerLogins(), then require context.payload.comment.user.login to be
present in the parsed list before github.rest.pulls.get() or any write
operation. Do not treat MEMBER or COLLABORATOR as sufficient authorization, and
add a regression case covering an associated user absent from MAINTAINERS.md.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3a97e072-1636-443f-9e17-215b549b8967
📒 Files selected for processing (4)
.github/scripts/enforce-pr-target.test.cjs.github/scripts/pr-quality-state.test.cjs.github/workflows/enforce-pr-target.ymltests/ci-workflows.test.ts
…rleaving Maintainer review blocker: both workflows read-modify-write the same consolidated gate comment, but pr-hygiene used cancel-in-progress: true. With the shared per-PR concurrency group, a newer hygiene run could cancel an in-flight gate mutation, losing that read-modify-write. Set cancel-in-progress: false so runs in the shared group queue, matching the enforce-target workflow. Add an interleaving regression test: a gate rebuild followed by a hygiene update (and the reverse order) preserves both the gate status and the hygiene block, with exactly one block each way. Pin the non-cancelling shared group in the workflow structure test.
|
[GD] Addressed the maintainer review blocker on head a0740f7. The hygiene-comment consolidation race is closed two ways:
The |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/pr-quality-messages.test.cjs:
- Around line 400-413: The reverse-order test must verify that the gate rebuild
preserves hygiene content from the preceding update. Update the test around
afterHygieneFirst and afterGateSecond to extract and reuse the hygiene lines
from afterHygieneFirst instead of supplying a new hard-coded hygiene array, or
call the production rebuild path that performs this extraction.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2957acea-43c8-4ac5-97d0-2e65f3723694
📒 Files selected for processing (3)
.github/scripts/pr-quality-messages.test.cjs.github/workflows/pr-hygiene.ymltests/ci-workflows.test.ts
…verse-order test Two CodeRabbit findings on the interleaving and issue_comment tests: - The issue_comment checklist-provenance test passed two `comments` keys; the second overwrote the maintainer waiver comment. Merge both fixtures into one array so the waiver is actually delivered to the harness. - The reverse-order interleaving test built afterHygieneFirst but never consumed it; afterGateSecond used a hard-coded hygiene array, so the test passed even if the gate rebuild discarded the prior hygiene update. Extract the hygiene content from afterHygieneFirst via extractHygieneSection and feed it into the gate rebuild.
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Re-reviewed head 8c4ad03f. The ordinary-issue guard, shared non-cancelling concurrency group, order-independent hygiene merge, and reverse-order regression are now in place. The focused workflow suite passes 121/121 locally.
One workflow-security blocker remains. .github/workflows/enforce-pr-target.yml still treats OWNER, COLLABORATOR, or MEMBER author association as sufficient authorization in both the job guard and the in-script guard. Those associations are broader than this repository's canonical maintainer list, so a non-maintainer collaborator/member can start this write-capable gate and cause PR comment, label, title, and draft-state mutations.
parseMaintainerLogins and the trusted checked-out MAINTAINERS.md are already available in this job. The association check can remain as a cheap job-level prefilter, but before github.rest.pulls.get() or any mutation the script must load the trusted maintainer list and require context.payload.comment.user.login to be a member of it. Please add a regression where the event reports COLLABORATOR or MEMBER, but the commenter is absent from MAINTAINERS.md; the harness should observe no PR lookup and no writes.
CI is also still running on this exact head. Keeping changes requested until the canonical-maintainer authorization and exact-head CI are complete.
Maintainer security blocker: OWNER/COLLABORATOR/MEMBER association is broader than the repository's canonical maintainer list, so a non-maintainer collaborator or member could start the write-capable gate and cause PR comment, label, title, and draft-state mutations. Before pulls.get or any mutation, the issue_comment guard now loads the trusted MAINTAINERS.md list (case-insensitively) and requires the commenter's login to be in it. The association check remains as the cheap job-level prefilter; the canonical list is the authorization. Regression: a COLLABORATOR who is not in MAINTAINERS.md cannot re-run the gate — no PR lookup, no writes, no GraphQL mutation.
|
[GD] Addressed on head 22cd62c: the Before Regression test added: "a COLLABORATOR who is not in MAINTAINERS.md cannot re-run the gate" — the harness observes no |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Approved on head 22cd62cb after the workflow-security re-review.
The comment-trigger path now uses author association only as a cheap job prefilter, then reads the trusted checked-out MAINTAINERS.md and requires the commenter's normalized login to be in that canonical list before pulls.get or any mutation. Missing/unreadable maintainer data fails the run closed. The regression proves that an associated but non-maintainer collaborator performs no PR lookup or write.
The earlier ordinary-issue guard, shared non-cancelling per-PR concurrency group, order-independent hygiene merge, and stale-checklist behavior remain intact. Focused workflow validation passes 122/122 locally.
Required CI is still running on this exact head; this approval does not waive it. Merge only after every required check is green.
2 participants
Summary
issue_commentevents, so a maintainer's frontend-screenshot waiver comment takes effect immediately instead of waiting for a PR edit or push. This fixes the scenario on test(catalog): pin the routed reasoning joint contract (#1100) + bug-grade triage unit #1119 where the waiver was posted but the gate never re-evaluated.issue_commentre-run is restricted to maintainer comments (OWNER / COLLABORATOR / MEMBER) on actual pull requests. A comment on a plain issue, or from a non-maintainer, does not start the write-capable gate — enforced by a job-levelifplus a defensive in-script re-check.prepushhook now runs the frontend eslint check only when the push touches the dashboard frontend directory, matching the existing frontend-doctor-if-changed behavior and the CIgatesjob's change filter. A tests-only or docs-only push no longer pays for an unconditional frontend lint run.opencodex-pr-gatecomment, and the gate preserves an existing hygiene block across rebuilds. No more two separate bot messages on a PR.Validation
bun test tests/ci-workflows.test.ts— 120 pass / 0 fail (covers the issue_comment re-run, the maintainer-only guard, the plain-issue skip, and the hygiene-section round-trip)node --test .github/scripts/enforce-pr-target.test.cjs— 17 pass / 0 failnode --test .github/scripts/*.test.cjs— 434 pass / 0 failbun test tests/docs-bun-source-requirement.test.ts tests/repo-hygiene.test.ts— 12 pass / 0 failbun run typecheck— cleanbun run privacy:scan— passedReview notes
issue_commenttrigger never touches PR head code: the checkout stays on the trusted base/default branch, and the gate script re-reads the live PR via the API. This is the same trusted-base model aspull_request_target, so it does not open the head-controlled-YAML escalation that review events would.pull_numberresolution falls back fromcontext.payload.pull_request?.numbertocontext.payload.issue?.number, and the concurrency group resolves from whichever payload exists.<!-- pr-hygiene-block:start/end -->), so both workflows edit one comment without clobbering each other's section.Summary by CodeRabbit
New Features
Bug Fixes
Documentation