fix: address review comments on suppress_comments guard detection (#1430)#1431
Merged
fix: address review comments on suppress_comments guard detection (#1430)#1431
Conversation
Add suppress_comments boolean input (default: false) to reusable-18-autofix.yml, gating the three PR comment-posting steps: - Upsert consolidated PR comment - Upsert clean-mode file summary comment - Upsert safe sweep file summary comment Update generate_suppression_guard_comment.py to recognise suppress_comments as a valid guard alongside should_post_review, and fix DEFAULT_WORKFLOWS to match actual filenames. Add test coverage for the new suppress_comments guard detection. Closes remaining deferred tasks from issue #1414.
1. Parse suppress_comments guard semantics instead of just checking token presence. A regex now verifies the expression uses a negation An inverted guard like 'suppress_comments == true' is no longer falsely treated as properly guarded. 2. Update build_comment() message to reference the actual workflow filenames (agents-keepalive-loop.yml, reusable-18-autofix.yml) instead of the non-existent keepalive.yml. 3. Add test for inverted guard detection.
stranske
temporarily deployed
to
agent-high-privilege
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the suppression-guard detector to avoid treating inverted suppress_comments conditions as valid guards, and aligns the generated guidance text with the repository’s actual workflow filenames.
Changes:
- Tighten guard detection to only accept negated
suppress_commentspatterns (soinputs.suppress_comments == trueis flagged as unguarded). - Update
build_comment()guidance text to reference the correct keepalive/autofix workflow filenames. - Add a regression test covering the inverted-guard case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
scripts/generate_suppression_guard_comment.py |
Restricts suppress_comments guard detection to negation forms and updates the workflow guidance message. |
tests/scripts/test_generate_suppression_guard_comment.py |
Adds a test ensuring inverted suppress_comments == true guards are flagged as unguarded. |
![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: 9b9c56aac1
ℹ️ 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".
Contributor
Automated Status SummaryHead SHA: 48a1e9b
Coverage Overview
Coverage Trend
Top Coverage Hotspots (lowest coverage)
Updated automatically; will refresh on subsequent CI/Docker completions. Keepalive checklistScopePR #1413 addressed issue #1412, but verification identified remaining gaps (verdict: CONCERNS). This follow-up issue closes those gaps by (1) enforcing suppression at the workflow level so comment/review posting cannot run when suppressed, (2) implementing missing modules required for the core logic and tests, (3) fixing output semantics to avoid duplicate Context for AgentRelated Issues/PRsTasks
Acceptance criteria
|
Contributor
🤖 Keepalive Loop StatusPR #1431 | Agent: Codex | Iteration 0/5 Current State
🔍 Failure Classification| Error type | infrastructure | |
stranske
temporarily deployed
to
agent-standard
GitHub Actions
Inactive
stranske
temporarily deployed
to
agent-standard
GitHub Actions
Inactive
stranske
temporarily deployed
to
agent-high-privilege
GitHub Actions
Inactive
Extend _SUPPRESS_NEGATION_RE to match !(inputs.suppress_comments) in 'needs-human' findings when the guard uses parenthesized negation. Add test for the parenthesized form.
stranske
temporarily deployed
to
agent-standard
GitHub Actions
Inactive
github-actions
bot
added
the
autofix
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated Status Summary
Scope
PR #1413 addressed issue #1412, but verification identified remaining gaps (verdict: CONCERNS). This follow-up issue closes those gaps by (1) enforcing suppression at the workflow level so comment/review posting cannot run when suppressed, (2) implementing missing modules required for the core logic and tests, (3) fixing output semantics to avoid duplicate
$GITHUB_OUTPUTentries, (4) bounding pagination to prevent excessive API calls, and (5) removing remaining TODO/skipped coverage so the full test suite can validate behavior end-to-end.Context for Agent
Related Issues/PRs
Tasks
.github/workflows/keepalive.ymlto add explicitif:guards on every step/job that posts a PR comment or PR review so they cannot run when the suppression output indicates posting is suppressed..github/workflows/autofix.ymlto add explicitif:guards on every step/job that posts a PR comment so they cannot run when the suppression output indicates posting is suppressed.scripts/keepalive_review_guard.jsexporting functions to load the designated review result file and evaluate it, returningfalsewhen the file is missing, JSON parsing fails, or the parsed payload is an all-empty object.scripts/should-post-review.jsto call intoscripts/keepalive_review_guard.jsand ensure the final computed decision output isfalsewhen the guard returnsfalse.scripts/should-post-review.jsto write exactly onekey=valueline per run to the file path inprocess.env.GITHUB_OUTPUT(replace anyappendFileSync-style duplication) while keeping the output key name unchanged.test/keepalive_review_guard.test.jsto cover evaluator edge cases: missing file, invalid JSON, and all-empty object payload returningfalse.test/should-post-review.test.jswith an integration-style test that runsscripts/should-post-review.jsend-to-end using a tempGITHUB_OUTPUTfile and asserts it outputsfalsewhen the guard encounters missing/invalid/all-empty payload.scripts/bot-comment-handler.jsto enforce a hard upper bound on pagination (constant/configurableN) when listing PR comments for deduplication.test/bot-comment-handler.test.jsto assert the mocked PR comment-list API call count is<= Neven when mocked responses keep returning full pages.scripts/bot-comment-dismiss.jsexporting the API expected bytest/bot-comment-dismiss.test.js, and wire deterministic mocks/fixtures as needed so the test runs without network calls.it.skip/describe.skipor TODO placeholders intest/**that bypass assertions for the implemented features so the full test suite executes.Acceptance criteria
.github/workflows/keepalive.yml, every step/job that posts a PR comment or PR review includes anif:guard that evaluates to false when the suppression output indicates posting is suppressed..github/workflows/autofix.yml, every step/job that posts a PR comment includes anif:guard that evaluates to false when the suppression output indicates posting is suppressed.scripts/keepalive_review_guard.jsexists and exports functions to load the designated review result file and evaluate it, and the evaluator returnsfalsewhen: (a) the file does not exist, (b) file contents are not valid JSON, or (c) the parsed JSON payload is an all-empty object.scripts/should-post-review.jscalls intokeepalive_review_guardsuch that when the designated review result file is missing, invalid JSON, or all-empty, the final computed decision output used by workflows isfalse.scripts/should-post-review.jswrites exactly one line for the chosen output key to the file path specified by theGITHUB_OUTPUTenvironment variable (no duplicate keys/lines for the same output per run).scripts/should-post-review.jsmatches the key consumed in.github/workflows/keepalive.ymland.github/workflows/autofix.yml(workflows reference the exact same key name).test/keepalive_review_guard.test.jsincludes explicit test cases asserting the evaluator returnsfalsefor: (1) missing review result file, (2) invalid JSON file contents, and (3) all-empty object payload.test/should-post-review.test.jsincludes at least one integration-style test that executesscripts/should-post-review.jsend-to-end and asserts the produced$GITHUB_OUTPUTvalue isfalsewhen the guard encounters (a) missing file, (b) invalid JSON, or (c) all-empty payload.scripts/bot-comment-handler.jsenforces a hard upper bound on pagination when listing PR comments for deduplication so it stops requesting further pages afterNpages even if the API continues returning full pages.test/bot-comment-handler.test.jsverifies the maximum number of comment-list API calls does not exceed the configured page limit (<= N) via mock call-count assertions.scripts/bot-comment-dismiss.jsexists and can be imported bytest/bot-comment-dismiss.test.jswithout module-not-found errors.test/bot-comment-dismiss.test.jspasses using deterministic mocks/fixtures (no network calls).test/**are skipped (it.skip,describe.skip) and no TODO placeholders remain that bypass assertions for the implemented features.Head SHA: 9cf48fb
Latest Runs: ✅ success — Gate
Required: gate: ✅ success