Skip to content

feat: implement issue #892 — dev-lead fix-review won't act on advisory-bot nitpick threads (green CI + CHANGES_REQUESTED, empty machine findings → status=no-changes → PR stalls on human) - #893

Merged
don-petry merged 8 commits into
mainfrom
dev-lead/issue-892-20260723-1820
Jul 24, 2026

Conversation

@don-petry

@don-petry don-petry commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

User description

Closes #892

Implemented by dev-lead agent. Please review.


CodeAnt-AI Description

Keep PR dispatch moving when only advisory bot comments remain

What Changed

  • PRs no longer wait on unresolved threads that contain only advisory bot comments
  • Human comments still block dispatch, and mixed bot-and-human threads remain blocking
  • Threads with missing comment data still block by default, so uncertain cases are not skipped
  • Added coverage for bot-only, human, mixed, empty, and multi-thread review scenarios

Impact

✅ Fewer PRs stalled by bot nitpicks
✅ Faster dispatch after green CI
✅ Fewer manual review cleanups before merge

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

…y-bot nitpick threads (green CI + CHANGES_REQUESTED, empty machine findings → status=no-changes → PR stalls on human)
@don-petry
don-petry requested a review from a team as a code owner July 23, 2026 18:37
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@codeant-ai

codeant-ai Bot commented Jul 23, 2026

Copy link
Copy Markdown

🤖 CodeAnt AI — Review Status

Status Commit Started (UTC) Finished (UTC)
✅ Reviewed your PR 574bde5 Jul 24, 2026 · 14:22 14:25
✅ Incremental review completed 0ef90a0 Jul 24, 2026 · 09:37 09:38
✅ Reviewed your PR 6a8987c Jul 23, 2026 · 19:13 19:16
✅ Reviewed your PR 4610c88 Jul 23, 2026 · 18:54 18:58
✅ Reviewed your PR 173bfcf Jul 23, 2026 · 18:37 18:41

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@don-petry, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 39 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 53a47ad3-458c-430f-9523-d01fd4e113b4

📥 Commits

Reviewing files that changed from the base of the PR and between b58e090 and 574bde5.

⛔ Files ignored due to path filters (2)
  • node_modules/.package-lock.json is excluded by !**/node_modules/**
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (5)
  • .github/scripts/pr-auto-review/README.md
  • .github/scripts/pr-auto-review/lib/ready-check.sh
  • .github/scripts/pr-auto-review/sweep-dispatch.sh
  • .github/workflows/pr-auto-review-reusable.yml
  • test/workflows/pr-auto-review/blocking-threads.bats
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dev-lead/issue-892-20260723-1820

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 23, 2026
@codeant-ai codeant-ai Bot added the size:XXL This PR changes 1000+ lines, ignoring generated files label Jul 23, 2026
@don-petry

Copy link
Copy Markdown
Contributor Author

Dev-Lead — review-changes (no-changes)

No changes were needed for this PR.

@don-petry
don-petry enabled auto-merge (squash) July 23, 2026 18:38
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Vendor Bats assertion/support libs into node_modules for shell test helpers

🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Add vendored bats-assert and bats-support helper libraries under node_modules.
• Update package-lock to include the new Bats helper dependencies.
• Enable richer Bats test assertions/output formatting via the new helper packages.
Diagram

graph TD
CI["CI job"] --> NPM["npm install"] --> LOCK["package-lock"]
NPM --> NM["node_modules"] --> ASSERT["bats-assert"] --> SUPPORT["bats-support"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Declare as devDependencies (don’t commit node_modules)
  • ➕ Avoids committing large third-party code blobs
  • ➕ Reduces PR noise and merge conflicts
  • ➕ Keeps dependency management standard (npm ci from lockfile)
  • ➖ Requires CI and dev environments to run npm install
  • ➖ Not suitable if repo policy mandates fully vendored dependencies
2. Use git submodules/vendor tarballs for Bats helpers
  • ➕ Explicit upstream pinning and provenance
  • ➕ Smaller diff surface than full node_modules trees (depending on approach)
  • ➖ More operational overhead than npm
  • ➖ Submodules can be cumbersome for contributors

Recommendation: If the repo does not intentionally vendor node_modules, prefer adding bats-assert/bats-support as devDependencies and relying on package-lock.json + npm ci in CI. If vendoring is intentional, this PR’s approach is acceptable, but it would be worth confirming policy and ensuring update automation (e.g., Renovate) covers these dependencies.

Files changed (25) +3580 / -0

Other (25) +3580 / -0
.package-lock.jsonLockfile update to include bats-assert and bats-support +19/-0

Lockfile update to include bats-assert and bats-support

• Adds resolved entries for 'bats-assert@2.2.4' and 'bats-support@0.3.0' (including peer dependency constraints) to the lockfile.

node_modules/.package-lock.json

LICENSEAdd bats-assert license (CC0-1.0) +116/-0

Add bats-assert license (CC0-1.0)

• Introduces the upstream license file for the vendored 'bats-assert' package.

node_modules/bats-assert/LICENSE

README.mdAdd bats-assert upstream documentation +1038/-0

Add bats-assert upstream documentation

• Adds the upstream README describing the assertion API and usage patterns for Bats tests.

node_modules/bats-assert/README.md

load.bashAdd bats-assert loader entrypoint +33/-0

Add bats-assert loader entrypoint

• Adds 'load.bash' that sources the individual assertion/refute implementations for use in Bats test suites.

node_modules/bats-assert/load.bash

package.jsonAdd bats-assert package metadata +49/-0

Add bats-assert package metadata

• Adds upstream package metadata including version, scripts, and peer dependency requirements on 'bats' and 'bats-support'.

node_modules/bats-assert/package.json

assert.bashVendor bats-assert assert() implementation +42/-0

Vendor bats-assert assert() implementation

• Adds the upstream 'assert()' helper used to fail a test when a command/expression evaluates false.

node_modules/bats-assert/src/assert.bash

assert_equal.bashVendor bats-assert assert_equal() implementation +42/-0

Vendor bats-assert assert_equal() implementation

• Adds the upstream 'assert_equal()' helper to compare actual vs expected values and print formatted failure output.

node_modules/bats-assert/src/assert_equal.bash

assert_failure.bashVendor bats-assert assert_failure() implementation +86/-0

Vendor bats-assert assert_failure() implementation

• Adds the upstream 'assert_failure()' helper for asserting non-zero exit status (optionally matching a specific status code).

node_modules/bats-assert/src/assert_failure.bash

assert_line.bashVendor bats-assert assert_line/assert_stderr_line helpers +298/-0

Vendor bats-assert assert_line/assert_stderr_line helpers

• Adds upstream line-based assertion helpers for stdout and stderr, including index/partial/regexp matching and error formatting.

node_modules/bats-assert/src/assert_line.bash

assert_not_equal.bashVendor bats-assert assert_not_equal() implementation +42/-0

Vendor bats-assert assert_not_equal() implementation

• Adds upstream 'assert_not_equal()' helper to ensure two values differ with formatted failure details.

node_modules/bats-assert/src/assert_not_equal.bash

assert_output.bashVendor bats-assert assert_output/assert_stderr stream matchers +247/-0

Vendor bats-assert assert_output/assert_stderr stream matchers

• Adds upstream stream assertion helpers for stdout/stderr supporting literal/partial/regexp and stdin-provided expected values.

node_modules/bats-assert/src/assert_output.bash

assert_regex.bashVendor bats-assert assert_regex() implementation +54/-0

Vendor bats-assert assert_regex() implementation

• Adds upstream regex assertion helper with validation and formatted output on mismatch.

node_modules/bats-assert/src/assert_regex.bash

assert_success.bashVendor bats-assert assert_success() implementation +47/-0

Vendor bats-assert assert_success() implementation

• Adds upstream 'assert_success()' helper to assert '$status == 0' and print '$output'/'$stderr' on failure.

node_modules/bats-assert/src/assert_success.bash

refute.bashVendor bats-assert refute() implementation +42/-0

Vendor bats-assert refute() implementation

• Adds upstream 'refute()' helper used to fail a test when a command/expression evaluates true.

node_modules/bats-assert/src/refute.bash

refute_line.bashVendor bats-assert refute_line/refute_stderr_line helpers +341/-0

Vendor bats-assert refute_line/refute_stderr_line helpers

• Adds upstream line-based refutation helpers (stdout/stderr) including regex validation and highlighted failure rendering.

node_modules/bats-assert/src/refute_line.bash

refute_output.bashVendor bats-assert refute_output/refute_stderr helpers +243/-0

Vendor bats-assert refute_output/refute_stderr helpers

• Adds upstream stream refutation helpers for stdout/stderr with literal/partial/regexp and stdin-provided unexpected values.

node_modules/bats-assert/src/refute_output.bash

refute_regex.bashVendor bats-assert refute_regex() implementation +64/-0

Vendor bats-assert refute_regex() implementation

• Adds upstream regex refutation helper with validation and formatted failure output on match.

node_modules/bats-assert/src/refute_regex.bash

CHANGELOG.mdAdd bats-support changelog +46/-0

Add bats-support changelog

• Introduces the upstream changelog for the vendored 'bats-support' package.

node_modules/bats-support/CHANGELOG.md

LICENSEAdd bats-support license (CC0-1.0) +116/-0

Add bats-support license (CC0-1.0)

• Introduces the upstream license file for the vendored 'bats-support' package.

node_modules/bats-support/LICENSE

README.mdAdd bats-support upstream documentation +189/-0

Add bats-support upstream documentation

• Adds upstream documentation for formatting/error reporting/lang helpers used by Bats helper libraries.

node_modules/bats-support/README.md

load.bashAdd bats-support loader entrypoint +3/-0

Add bats-support loader entrypoint

• Adds 'load.bash' that sources output/error/lang support modules for Bats helpers.

node_modules/bats-support/load.bash

package.jsonAdd bats-support package metadata +30/-0

Add bats-support package metadata

• Adds upstream package metadata including version and peer dependency constraints on 'bats'.

node_modules/bats-support/package.json

error.bashVendor bats-support fail() helper +41/-0

Vendor bats-support fail() helper

• Adds upstream error reporting primitive 'fail()' used by 'bats-assert' to emit failures consistently.

node_modules/bats-support/src/error.bash

lang.bashVendor bats-support caller/stack utilities +73/-0

Vendor bats-support caller/stack utilities

• Adds upstream 'batslib_is_caller()' helper to restrict invocation sites based on call stack inspection.

node_modules/bats-support/src/lang.bash

output.bashVendor bats-support output formatting utilities +279/-0

Vendor bats-support output formatting utilities

• Adds upstream formatting utilities ('batslib_print_kv_*', decorate/prefix/mark, etc.) used by assertions for readable failure output.

node_modules/bats-support/src/output.bash

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request adds the bats-assert and bats-support test helper libraries directly under node_modules/, but completely lacks the actual feature implementation mentioned in the PR title. Feedback highlights that committing node_modules/ directly to version control is a bad practice. Additionally, multiple critical issues were identified in the added Bash scripts, including potential fatal syntax errors across several assertion files when regular expression patterns or expected values are empty, as well as a missing function definition for __check_is_valid_regex when assert_regex.bash is sourced independently.

Comment thread node_modules/.package-lock.json Outdated
Comment thread node_modules/bats-assert/src/refute_line.bash Outdated
Comment thread node_modules/bats-assert/src/assert_line.bash Outdated
Comment thread node_modules/bats-assert/src/assert_line.bash Outdated
Comment thread node_modules/bats-assert/src/assert_output.bash Outdated
Comment thread node_modules/bats-assert/src/refute_line.bash Outdated
Comment thread node_modules/bats-assert/src/refute_line.bash Outdated
Comment thread node_modules/bats-assert/src/refute_output.bash Outdated
Comment thread node_modules/bats-assert/src/refute_regex.bash Outdated
Comment thread node_modules/bats-assert/src/assert_regex.bash Outdated
@don-petry
don-petry disabled auto-merge July 23, 2026 18:41
Comment thread node_modules/bats-assert/src/assert_failure.bash Outdated
Comment thread node_modules/bats-assert/src/assert_line.bash Outdated
Comment thread node_modules/bats-support/src/output.bash Outdated
Comment thread node_modules/bats-support/src/output.bash Outdated
Comment thread node_modules/bats-assert/src/assert_output.bash Outdated
Comment thread node_modules/bats-assert/src/refute_line.bash Outdated
Comment thread node_modules/bats-assert/src/refute_line.bash Outdated
Comment thread node_modules/bats-assert/src/assert_regex.bash Outdated
Comment thread node_modules/bats-assert/src/refute_output.bash Outdated
Comment thread node_modules/bats-assert/src/refute_output.bash Outdated
@qodo-code-review

qodo-code-review Bot commented Jul 23, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 87 rules

Grey Divider


Remediation recommended

1. Untracked vendored npm packages ✗ Dismissed 🐞 Bug ⛨ Security
Description
This PR adds bats-assert and bats-support under node_modules/ (and records them in
node_modules/.package-lock.json) but they are absent from the repo’s root
package.json/package-lock.json, so npm ci/npm prune can remove them and the
dependency-audit/npm-audit path won’t include them in vulnerability reporting.
Code

node_modules/.package-lock.json[R15-32]

+    "node_modules/bats-assert": {
+      "version": "2.2.4",
+      "resolved": "https://registry.npmjs.org/bats-assert/-/bats-assert-2.2.4.tgz",
+      "integrity": "sha512-EcaY4Z+Tbz1c7pnC1SrVSq0epr7tLwFpz6qt7KUW9K8uSw8V12DTfH9d2HxZWvBEATaCuMsZ7KoZMFiSQPRoXw==",
+      "license": "CC0-1.0",
+      "peerDependencies": {
+        "bats": "0.4 || ^1",
+        "bats-support": "^0.3"
+      }
+    },
+    "node_modules/bats-support": {
+      "version": "0.3.0",
+      "resolved": "https://registry.npmjs.org/bats-support/-/bats-support-0.3.0.tgz",
+      "integrity": "sha512-z+2WzXbI4OZgLnynydqH8GpI3+DcOtepO66PlK47SfEzTkiuV9hxn9eIQX+uLVFbt2Oqoc7Ky3TJ/N83lqD+cg==",
+      "license": "CC0-1.0",
+      "peerDependencies": {
+        "bats": "0.4 || ^1"
+      }
Relevance

⭐⭐⭐ High

Repo emphasizes audited, reproducible deps; node_modules artifacts are treated as drift and usually
ignored/removed.

PR-#9
PR-#802

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR diff shows new entries for bats-assert and bats-support in
node_modules/.package-lock.json, but the repo’s root npm manifests still only declare/lock bats,
and the standards docs state npm auditing is based on package-lock.json (root). CI bats tests
install bats from apt, indicating these additions are not exercised in CI and are likely
drift/accidental unless explicitly managed.

node_modules/.package-lock.json[1-35]
package.json[1-4]
package-lock.json[1-21]
.github/workflows/pr-auto-review-tests.yml[42-68]
standards/dependabot-policy.md[290-305]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`node_modules/bats-assert` and `node_modules/bats-support` were added (and captured in `node_modules/.package-lock.json`) but the repo-level npm dependency graph still only includes `bats`. This creates drift: npm workflows based on root manifests (`npm ci`, `npm prune`, and org-standard `npm audit` driven by `package-lock.json`) won’t model/audit these packages and may delete them.

## Issue Context
- CI bats tests install `bats` via apt and do not run `npm install`, so these vendored packages are currently unused by CI.
- Org guidance indicates npm auditing is driven by the root `package-lock.json`.

## Fix Focus Areas
- package.json[1-4]
- package-lock.json[1-21]
- node_modules/.package-lock.json[1-35]
- standards/dependabot-policy.md[290-305]

## Suggested fix
Choose ONE path:
1) **Proper npm-managed devDependencies**: add `bats-assert` and `bats-support` to root `package.json` (likely `devDependencies`), regenerate root `package-lock.json`, and (optionally) stop committing these packages directly under `node_modules/`.
2) **Intentional vendoring**: remove these packages and revert the `node_modules/.package-lock.json` changes, OR document/enforce the vendoring approach (and ensure security/audit tooling accounts for vendored dependencies if they’re meant to be executed).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread node_modules/.package-lock.json Outdated
@donpetry-bot

Copy link
Copy Markdown
Contributor

CI checks on this PR are still running. Once they complete, re-mention @donpetry-bot to trigger a fresh review.

Posted by the donpetry-bot PR-review cascade.

@don-petry

Copy link
Copy Markdown
Contributor Author

Dev-Lead — review-changes (applied)

Changes committed and pushed.

@don-petry
don-petry enabled auto-merge (squash) July 23, 2026 18:54
@don-petry
don-petry disabled auto-merge July 23, 2026 18:54
@codeant-ai codeant-ai Bot added size:XXL This PR changes 1000+ lines, ignoring generated files and removed size:XXL This PR changes 1000+ lines, ignoring generated files labels Jul 23, 2026
Comment thread test/workflows/pr-auto-review/blocking-threads.bats
Comment thread .github/scripts/pr-auto-review/lib/ready-check.sh
Comment thread .github/scripts/pr-auto-review/lib/ready-check.sh
@codeant-ai codeant-ai Bot added the size:XXL This PR changes 1000+ lines, ignoring generated files label Jul 24, 2026
@don-petry

Copy link
Copy Markdown
Contributor Author

Dev-Lead — review-changes (applied)

Changes committed and pushed.

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 24, 2026
@don-petry
don-petry disabled auto-merge July 24, 2026 09:42
@don-petry

Copy link
Copy Markdown
Contributor Author

Note

@don-petry I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically.
Next attempt after: 2026-07-24T10:17:32Z

@don-petry
don-petry enabled auto-merge (squash) July 24, 2026 09:47
@don-petry
don-petry disabled auto-merge July 24, 2026 10:28
@don-petry

Copy link
Copy Markdown
Contributor Author

Dev-Lead — waiting on PR blockers (intent: review-changes)

PR: #893
No changes were committed, but the PR still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews). The retry cron will re-attempt automatically. Next attempt after: 2026-07-24T11:05:58Z

@don-petry
don-petry enabled auto-merge (squash) July 24, 2026 10:36
@donpetry-bot

donpetry-bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor
Superseded by automated re-review at 574bde588a9f9ee93b3e05a85a80c4df668c73ae — click to expand prior review.

Review — fix requested (cycle 2/3)

The automated review identified the following issues. Please address each one:

Findings to fix

Automated review — NEEDS HUMAN REVIEW

Risk: MEDIUM
Reviewed commit: 1e280c86443b0c53b5162ec8c17c37c0e5340d9a
Cascade: triage → deep (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5)

Summary

The core feature for #892 (skip advisory-bot-only review threads when gating auto-dispatch) is correctly implemented in ready-check.sh, sweep-dispatch.sh and pr-auto-review-reusable.yml, with excellent bats coverage (bot-only=0, human/mixed/empty/absent fail-safes=block, and the exact 5-bot #892 scenario) and all CI green. The blocking problem is a repo-hygiene gate failure: the PR accidentally commits an entire node_modules/ tree (bats-assert 2.2.4 + bats-support 0.3.0, ~3672 lines) that is unused (the added test loads only helpers/setup and uses plain bash assertions), not gitignored, and not declared in any package.json — the repo installs bats via CI (apt-get / bats-core clone), not vendored deps. Escalating for remediation (remove node_modules) rather than to security audit, since the authored diff has no security surface; downstream impact on 7 consumers of the reusable workflow is informational only (additive, backward-compatible GraphQL query expansion).

Downstream impact

This change is consumed by 7 downstream repo(s) that pin the affected reusable workflow / lib / prompt. Impacted consumers:

Impacted shared surfaces:
  - .github/workflows/pr-auto-review-reusable.yml

Impacted consumers (7, fetching up to 10):
  - petry-projects/.github (pins .github/workflows/pr-auto-review-reusable.yml)
      .github/workflows/pr-auto-review.yml
  - petry-projects/ContentTwin (pins .github/workflows/pr-auto-review-reusable.yml)
      .github/workflows/pr-auto-review.yml
  - petry-projects/TalkTerm (pins .github/workflows/pr-auto-review-reusable.yml)
      .github/workflows/pr-auto-review.yml
  - petry-projects/bmad-bgreat-suite (pins .github/workflows/pr-auto-review-reusable.yml)
      .github/workflows/pr-auto-review.yml
  - petry-projects/broodly (pins .github/workflows/pr-auto-review-reusable.yml)
      .github/workflows/pr-auto-review.yml
  - petry-projects/google-app-scripts (pins .github/workflows/pr-auto-review-reusable.yml)
      .github/workflows/pr-auto-review.yml
  - petry-projects/markets (pins .github/workflows/pr-auto-review-reusable.yml)
      .github/workflows/pr-auto-review.yml

Findings

  • MAJOR [maintainability] Accidental commit of node_modules/ (bats-assert 2.2.4, bats-support 0.3.0, plus node_modules/.package-lock.json) — ~3672 of the PR's 3690 additions. This vendored code is unused (the added test/workflows/pr-auto-review/blocking-threads.bats loads only 'helpers/setup' and uses plain '[ ]' assertions, never assert_/refute_), not gitignored, and not declared in any package.json. Repo convention installs bats via CI (lint.yml: apt-get install bats; test-dev-lead.yml: clone bats-core). Remove the node_modules/ tree; if bats-assert/support are genuinely wanted, declare them and add node_modules/ to .gitignore. Must be resolved before merge.
    File: node_modules/.package-lock.json
  • INFO [review-signal] Advisory-bot false negative: Gemini (critical) and CodeAnt claim 'the actual feature implementation is completely missing.' This is inaccurate — the feature is present and correct in .github/scripts/pr-auto-review/lib/ready-check.sh (is_advisory_bot_thread jq helper) and the two GraphQL query call sites. The bots were misled by the node_modules diff bloat. Do not act on the 'feature missing' claim.
  • INFO [review-signal] Gemini's 8 'high-priority' regex/empty-pattern syntax findings are all in upstream vendored bats-assert source (node_modules/bats-assert/src/*.bash), not in code authored by this PR. They are real upstream robustness notes but irrelevant to the authored change and will disappear once node_modules/ is removed.
    File: node_modules/bats-assert/src/assert_regex.bash line 39
  • MINOR [correctness] Authored feature logic is sound and well-tested. is_advisory_bot_thread requires comments.nodes non-empty AND every author.__typename == 'Bot'; blocking = unresolved AND not outdated AND not bot-only. Fail-safes verified by tests: absent/empty comments still block, mixed bot+human blocks, bot-open+human-reply blocks (pins comments(first:100) vs first:1). GraphQL expansion is consistent across sweep-dispatch.sh and the reusable workflow with owner/repo/number passed as GraphQL variables (no injection).
    File: .github/scripts/pr-auto-review/lib/ready-check.sh line 111
  • INFO [downstream-impact] pr-auto-review-reusable.yml is pinned by 7 consumers (ContentTwin, TalkTerm, bmad-bgreat-suite, broodly, google-app-scripts, markets, and .github itself). The change is an additive, backward-compatible GraphQL query-string expansion — no new inputs/outputs, no interface break — so this is informational, not an escalation trigger on its own.
    File: .github/workflows/pr-auto-review-reusable.yml
  • INFO [secret] run_secret_scanning MCP tool not available in this environment; did not run it. The gitleaks 'Secret scan' CI check is green, and no credentials/tokens appear in the authored diff.

Reviewed by the PR-review cascade (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5). Reply if you need a human review.

Additional tasks

  1. Resolve all unresolved review thread comments from other reviewers
  2. Ensure all CI checks pass after your changes
  3. Rebase on the target branch if behind
  4. Do NOT modify files unrelated to the findings above

The review cascade will automatically re-review after new commits are pushed.

@don-petry
don-petry disabled auto-merge July 24, 2026 14:14
@don-petry

Copy link
Copy Markdown
Contributor Author

Dev-Lead — review-changes (applied)

Changes committed and pushed.

@don-petry
don-petry enabled auto-merge (squash) July 24, 2026 14:22
@codeant-ai codeant-ai Bot added size:L This PR changes 100-499 lines, ignoring generated files and removed size:XXL This PR changes 1000+ lines, ignoring generated files labels Jul 24, 2026
@don-petry
don-petry disabled auto-merge July 24, 2026 14:23
@sonarqubecloud

Copy link
Copy Markdown

@don-petry

Copy link
Copy Markdown
Contributor Author

Dev-Lead — waiting on PR blockers (intent: fix-reviews)

PR: #893
No changes were committed, but the PR still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews). The retry cron will re-attempt automatically. Next attempt after: 2026-07-24T14:55:44Z

@don-petry
don-petry enabled auto-merge (squash) July 24, 2026 14:25

@donpetry-bot donpetry-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Automated review — APPROVED ✓

Risk: MEDIUM
Reviewed commit: 574bde588a9f9ee93b3e05a85a80c4df668c73ae
Review mode: triage-approved (single reviewer)

Summary

Re-review after the cycle-2 fix request. The sole blocking finding — an accidental ~3,672-line vendored node_modules/ tree (bats-assert 2.2.4 + bats-support 0.3.0) — is fully resolved in 574bde5: all vendored files are deleted and the PR is now a focused 112-addition / 10-deletion change across 7 files. The #892 feature (advisory-bot-only review threads no longer block auto-dispatch) is correctly implemented in ready-check.sh, sweep-dispatch.sh, and pr-auto-review-reusable.yml, with strong bats coverage of every fail-safe. All required CI checks are green and there are zero unresolved review threads.

Linked issue analysis

Closes #892 (dev-lead fix-review stalls on advisory-bot nitpick threads with green CI). The PR addresses the stall mechanism at the dispatch gate: pr_auto_review_blocking_thread_count now excludes threads whose comments are exclusively from Bot-typed authors, so the review cascade can re-dispatch and clear the stale CHANGES_REQUESTED state instead of stalling on skip-unresolved-threads. Conservative fail-safes preserved: absent/empty comments, mixed bot+human threads, and null authors all still block; the CHANGES_REQUESTED gate (criterion 3) is untouched. The prior deep-cascade review validated this approach as correctly implementing #892.

Findings

  • RESOLVED (prior MAJOR, maintainability): vendored node_modules/ tree removed in 574bde5 (bats-assert, bats-support — 26 files, ~3,672 deletions). The remaining node_modules/.package-lock.json is pre-existing on main; this PR only touches its name field.
  • MINOR (hygiene, non-blocking): package-lock.json and node_modules/.package-lock.json name field churns pr-840 → pr-893 — cosmetic artifact of the dev-lead checkout dir naming; consider a follow-up to stabilize the lockfile name.
  • VERIFIED (correctness): is_advisory_bot_thread requires comments.nodes non-empty AND every author.__typename == "Bot"; blocking = unresolved AND not outdated AND not bot-only. 7 new bats tests pin bot-only=0, human=1, absent/empty comments=1, mixed=1, bot-open+human-reply=1 (pins comments(first:100) vs first:1), and the 5-thread #892 scenario=0. bats CI green.
  • VERIFIED (security): GraphQL query strings are static with owner/repo/number passed as GraphQL variables via -f/-F — no injection surface. No secrets in the diff; gitleaks green. run_secret_scanning MCP tool not available in this environment (noted, non-blocking).
  • INFO (downstream): pr-auto-review-reusable.yml is pinned by 7 consumer repos; the change is an additive, backward-compatible GraphQL query expansion with no interface change.

CI status

All required checks green: SonarCloud, CodeQL, agent-shield/AgentShield, Detect ecosystems, Lint, ShellCheck, Agent Security Scan. Also green: bats, Lint and bats, npm audit, gitleaks, Analyze (actions), pr-auto-review, Graphite, CodeRabbit. The 4 non-green entries (dev-lead / dispatch, dev-lead / ci-relay ×2 each) are cancelled/rate-limited dev-lead agent orchestration relays — not required, not code checks.


Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.

@don-petry
don-petry merged commit a8819c0 into main Jul 24, 2026
34 of 48 checks passed
@don-petry
don-petry deleted the dev-lead/issue-892-20260723-1820 branch July 24, 2026 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files

Projects

None yet

2 participants