Wire review suppression guard into keepalive workflow (issue #1414)

.github/scripts/bot-comment-handler.js

@@ -0,0 +1,69 @@
+'use strict';
+
+const DEFAULT_PER_PAGE = 100;
+const MAX_COMMENT_PAGES = 10;
+
+/**
+ * List PR/issue comments with a hard pagination upper bound.
+ *
+ * Callers should pass a `listFn` obtained via createTokenAwareRetry
+ * (from github-api-with-retry.js) so that every page request gets
+ * automatic token rotation and rate-limit back-off.
+ *
+ * @param {object} options
+ * @param {string} options.owner - Repository owner.
+ * @param {string} options.repo  - Repository name.
+ * @param {number} options.issueNumber - PR or issue number.
+ * @param {function} options.listFn - Paginated list function (required).
+ * @param {number} [options.perPage=100]  - Items per page.
+ * @param {number} [options.maxPages=10]  - Hard upper bound on pages fetched.
+ * @returns {Promise<object[]>} Collected comments.
+ */
+async function listCommentsWithLimit(options = {}) {
+  const owner = options.owner;
+  const repo = options.repo;
+  const issueNumber = options.issueNumber;
+  const perPage =
+    typeof options.perPage === 'number' && Number.isFinite(options.perPage)
+      ? options.perPage
+      : DEFAULT_PER_PAGE;
+  const maxPages =
+    typeof options.maxPages === 'number' && Number.isFinite(options.maxPages)
+      ? options.maxPages
+      : MAX_COMMENT_PAGES;
+  const listFn = options.listFn;
+
+  if (!listFn) {
+    throw new Error('listFn is required (use createTokenAwareRetry to wrap the API client)');
+  }
+  if (!owner || !repo) {
+    throw new Error('owner and repo are required');
+  }
+  if (!issueNumber) {
+    throw new Error('issueNumber is required');
+  }
+
+  const comments = [];
+  for (let page = 1; page <= maxPages; page += 1) {
+    const response = await listFn({
+      owner,
+      repo,
+      issue_number: issueNumber,
+      per_page: perPage,
+      page,
+    });
+    const pageData = Array.isArray(response?.data) ? response.data : response || [];
+    comments.push(...pageData);
+    if (pageData.length < perPage) {
+      break;
+    }
+  }
+
+  return comments;
+}
+
+module.exports = {
+  DEFAULT_PER_PAGE,
+  MAX_COMMENT_PAGES,
+  listCommentsWithLimit,
+};

.github/scripts/should-post-review.js

@@ -19,10 +19,10 @@ function writeOutput(shouldPost) {
   const line = `should_post_review=${shouldPost ? 'true' : 'false'}`;
 
   if (outputPath) {
-    fs.appendFileSync(outputPath, `${line}\n`, 'utf8');
-  } else {
-    process.stdout.write(`${line}\n`);
+    fs.writeFileSync(outputPath, `${line}\n`, 'utf8');
+    return;
   }
+  process.stdout.write(`${line}\n`);
 }
 
 function main() {

agents/codex-1417.md

@@ -0,0 +1,11 @@
+<!--
+needs-human:
+Label: needs-human
+Blocked by workflow protection: update .github/workflows/keepalive.yml and .github/workflows/autofix.yml to add explicit `if:` guards on every step/job that posts a PR comment or PR review so they cannot run when suppression is active. Use the suppression output key `should_post_review` (from .github/scripts/should-post-review.js) to gate the posting steps.
+
+Workflow: .github/workflows/keepalive.yml
+- Workflow file not found in repository.
+
+Workflow: .github/workflows/autofix.yml
+- No unguarded PR comment/review posting steps detected (or posting steps are already guarded).
+-->

scripts/check_api_wrapper_guard.py

@@ -132,7 +132,7 @@ def _is_target_file(path: Path) -> bool:
     if "node_modules" in path.parts:
         return False
     # Skip test directories — tests use mock github objects, not real API clients
-    if "__tests__" in path.parts:
+    if "__tests__" in path.parts or "tests" in path.parts:
         return False
     # Skip agent ledger files (task tracking, not API usage)
     if path.parent.name == ".agents" and path.stem.endswith("-ledger"):

scripts/generate_suppression_guard_comment.py

@@ -0,0 +1,183 @@
+#!/usr/bin/env python3
+"""Generate a needs-human comment for suppression guard workflow updates."""
+
+from __future__ import annotations
+
+import argparse
+import pathlib
+import re
+from collections.abc import Iterable, Sequence
+from typing import Any
+
+import yaml
+
+DEFAULT_WORKFLOWS = (
+    pathlib.Path(".github/workflows/keepalive.yml"),
+    pathlib.Path(".github/workflows/autofix.yml"),
+)
+
+SCRIPT_PATTERNS: tuple[tuple[re.Pattern[str], str], ...] = (
+    (
+        re.compile(r"(?:github|octokit)(?:\.rest)?\.issues\.createComment\b"),
+        "issues.createComment",
+    ),
+    (
+        re.compile(r"(?:github|octokit)(?:\.rest)?\.pulls\.createReview\b"),
+        "pulls.createReview",
+    ),
+    (
+        re.compile(r"(?:github|octokit)(?:\.rest)?\.pulls\.createReviewComment\b"),
+        "pulls.createReviewComment",
+    ),
+    (re.compile(r"\\bgh\\s+pr\\s+comment\\b"), "gh pr comment"),
+    (re.compile(r"\\bgh\\s+pr\\s+review\\b"), "gh pr review"),
+)
+
+ACTION_HINTS: tuple[tuple[str, str], ...] = (
+    ("peter-evans/create-or-update-comment", "create-or-update-comment action"),
+    ("peter-evans/create-pull-request", "create-pull-request action"),
+    ("marocchino/sticky-pull-request-comment", "sticky-pull-request-comment action"),
+)
+
+
+def _normalise_keys(node: Any) -> Any:
+    if isinstance(node, dict):
+        normalised: dict[str, Any] = {}
+        for key, value in node.items():
+            key_str = ("on" if key else str(key).lower()) if isinstance(key, bool) else str(key)
+            normalised[key_str] = _normalise_keys(value)
+        return normalised
+    if isinstance(node, list):
+        return [_normalise_keys(item) for item in node]
+    return node
+
+
+def _load_workflow(path: pathlib.Path) -> dict[str, Any]:
+    raw = path.read_text(encoding="utf-8")
+    data = yaml.safe_load(raw) or {}
+    if not isinstance(data, dict):
+        raise SystemExit(f"Workflow {path} should load into a mapping structure")
+    return _normalise_keys(data)
+
+
+def _step_script(step: dict[str, Any]) -> str:
+    run = step.get("run")
+    if isinstance(run, str):
+        return run
+    with_block = step.get("with") or {}
+    script = with_block.get("script")
+    if isinstance(script, str):
+        return script
+    return ""
+
+
+def _step_action_hint(step: dict[str, Any]) -> str | None:
+    uses = step.get("uses")
+    if not isinstance(uses, str):
+        return None
+    lower = uses.lower()
+    for needle, label in ACTION_HINTS:
+        if needle in lower:
+            return label
+    return None
+
+
+def _match_patterns(script: str) -> list[str]:
+    matches: list[str] = []
+    for pattern, label in SCRIPT_PATTERNS:
+        if pattern.search(script):
+            matches.append(label)
+    return matches
+
+
+def _iter_posting_steps(workflow: dict[str, Any]) -> list[tuple[str, str, list[str]]]:
+    findings: list[tuple[str, str, list[str]]] = []
+    jobs = workflow.get("jobs") or {}
+    for job_id, job in jobs.items():
+        job_if = job.get("if")
+        job_if_str = job_if if isinstance(job_if, str) else ""
+        steps = job.get("steps") or []
+        for index, step in enumerate(steps, start=1):
+            if not isinstance(step, dict):
+                continue
+            name = step.get("name") or step.get("id") or f"step-{index}"
+            script = _step_script(step)
+            hints = _match_patterns(script)
+            action_hint = _step_action_hint(step)
+            if action_hint:
+                hints.append(action_hint)
+            step_if = step.get("if")
+            step_if_str = step_if if isinstance(step_if, str) else ""
+            guarded = "should_post_review" in job_if_str or "should_post_review" in step_if_str
+            if hints and not guarded:
+                findings.append((str(job_id), str(name), hints))
+    return findings
+
+
+def _format_findings(
+    workflow_path: pathlib.Path, findings: Sequence[tuple[str, str, list[str]]]
+) -> list[str]:
+    lines: list[str] = []
+    lines.append(f"Workflow: {workflow_path}")
+    if not findings:
+        lines.append(
+            "- No unguarded PR comment/review posting steps detected "
+            "(or posting steps are already guarded)."
+        )
+        return lines
+    for job_id, step_name, hints in findings:
+        hint_str = ", ".join(sorted(set(hints)))
+        lines.append(f"- {job_id} / {step_name} ({hint_str})")
+    return lines
+
+
+def build_comment(workflows: Iterable[pathlib.Path], include_label: bool = False) -> str:
+    lines: list[str] = []
+    if include_label:
+        lines.append("Label: needs-human")
+    lines.append(
+        "Blocked by workflow protection: update .github/workflows/keepalive.yml and "
+        ".github/workflows/autofix.yml to add explicit `if:` guards on every step/job "
+        "that posts a PR comment or PR review so they cannot run when suppression is active. "
+        "Use the suppression output key `should_post_review` (from "
+        ".github/scripts/should-post-review.js) to gate the posting steps."
+    )
+    lines.append("")
+
+    for workflow_path in workflows:
+        if not workflow_path.exists():
+            lines.append(f"Workflow: {workflow_path}")
+            lines.append("- Workflow file not found in repository.")
+            lines.append("")
+            continue
+        workflow = _load_workflow(workflow_path)
+        findings = _iter_posting_steps(workflow)
+        lines.extend(_format_findings(workflow_path, findings))
+        lines.append("")
+
+    if lines and not lines[-1]:
+        lines.pop()
+    return "\n".join(lines)
+
+
+def main() -> None:
+    parser = argparse.ArgumentParser()
+    parser.add_argument(
+        "--workflow",
+        action="append",
+        type=pathlib.Path,
+        dest="workflows",
+        help="Path to a workflow YAML file (repeatable). Defaults to keepalive/autofix.",
+    )
+    parser.add_argument(
+        "--include-label",
+        action="store_true",
+        help="Include needs-human label line in the output.",
+    )
+    args = parser.parse_args()
+    workflows = tuple(args.workflows) if args.workflows else DEFAULT_WORKFLOWS
+    print(build_comment(workflows, include_label=args.include_label))
+
+
+if __name__ == "__main__":
+    main()

templates/consumer-repo/.github/scripts/should-post-review.js

@@ -19,10 +19,10 @@ function writeOutput(shouldPost) {
   const line = `should_post_review=${shouldPost ? 'true' : 'false'}`;
 
   if (outputPath) {
-    fs.appendFileSync(outputPath, `${line}\n`, 'utf8');
-  } else {
-    process.stdout.write(`${line}\n`);
+    fs.writeFileSync(outputPath, `${line}\n`, 'utf8');
+    return;
   }
+  process.stdout.write(`${line}\n`);
 }
 
 function main() {

tests/bot-comment-handler.test.js

@@ -0,0 +1,29 @@
+'use strict';
+
+const test = require('node:test');
+const assert = require('node:assert/strict');
+
+const {
+  listCommentsWithLimit,
+  MAX_COMMENT_PAGES,
+} = require('../.github/scripts/bot-comment-handler');
+
+test('bot-comment-handler enforces pagination upper bound', async () => {
+  let calls = 0;
+  const listFn = async () => {
+    calls += 1;
+    return { data: [{ id: calls }] };
+  };
+
+  await listCommentsWithLimit({
+    owner: 'octo',
+    repo: 'repo',
+    issueNumber: 123,
+    perPage: 1,
+    maxPages: MAX_COMMENT_PAGES,
+    listFn,
+  });
+
+  assert.ok(calls <= MAX_COMMENT_PAGES);
+  assert.equal(calls, MAX_COMMENT_PAGES);
+});

tests/fixtures/review_result/review-invalid.json

@@ -0,0 +1 @@
+{ invalid-json

tests/keepalive_review_guard.test.js

@@ -0,0 +1,36 @@
+'use strict';
+
+const test = require('node:test');
+const assert = require('node:assert/strict');
+const fs = require('node:fs');
+const os = require('node:os');
+const path = require('node:path');
+
+const {
+  evaluateReviewResult,
+  loadReviewResult,
+} = require('../.github/scripts/keepalive_review_guard');
+
+function computeShouldPost(filePath) {
+  const { payload, readError } = loadReviewResult(filePath);
+  return readError ? false : evaluateReviewResult(payload).shouldPost;
+}
+
+test('keepalive_review_guard returns false when review file is missing', () => {
+  const missingPath = path.join(os.tmpdir(), 'missing-review-result.json');
+  const shouldPost = computeShouldPost(missingPath);
+  assert.equal(shouldPost, false);
+});
+
+test('keepalive_review_guard returns false when review JSON is invalid', () => {
+  const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'review-guard-'));
+  const filePath = path.join(dir, 'invalid.json');
+  fs.writeFileSync(filePath, '{not-json', 'utf8');
+  const shouldPost = computeShouldPost(filePath);
+  assert.equal(shouldPost, false);
+});
+
+test('keepalive_review_guard returns false when review payload is all-empty object', () => {
+  const shouldPost = evaluateReviewResult({}).shouldPost;
+  assert.equal(shouldPost, false);
+});