test(reviewer): guard the comment-selection filter that has broken twice

[doublegate]

What

scripts/agy-review.sh's comment-selection filter decides which PR comments the bot deletes. It has been wrong twice, and neither time was observable from the outside — the review posted fine on both occasions:

1. The just-posted comment was not reliably excluded. new_comment_id came from re-querying the comment list, which races GitHub's read replication; on a miss the exclusion degenerated to select(.id != null) — true for every id — and the run deleted the review it had just published.
2. jq's --arg/--argjson were passed to gh api, which has no such flags. It exited non-zero, 2>/dev/null hid the message, and set -o pipefail + set -e killed the script after posting. Stale comments silently accumulated (this repo's chore(release): cut v1.25.0 "Workbench" #272 reached four) and the job went red with nothing in the log to say why.

Both lived in the same ~10 lines. The second was mine, introduced in #270 and shipped for four PRs before the symptom was traced.

Changes

• The filter moves into a named SELECT_STALE_JQ, so a test can exercise the real thing rather than a copy that drifts.
• scripts/agy-review-selftest.sh lifts MARKER and that filter out of the script by pattern and runs them against fixtures — offline: no gh, no network, no self-hosted runner. Lifting by pattern is deliberate; rename or reshape either declaration and the test fails loudly instead of quietly checking something stale.
• Wired into the lint job, so it runs on every PR rather than only where agy happens to be installed.

The six checks

check	guards
excludes the just-posted comment	regression 1 — self-deletion
ignores other users and other bots	the author filter, which is a security control: without it anyone could paste the marker into a comment and have the bot delete comments on the next run
ignores bot comments without the marker	somebody else's CI summary is not ours to delete
an id of 0 excludes nothing real	the degenerate-exclusion shape
excludes whichever id it is given	the exclusion is by value, not incidental
--arg/--argjson not passed to gh api	regression 2

Verification

Each historical regression was injected and the suite confirmed to fail at the matching check and exit 1:

• dropping | select(.id != $new_id) → fails "excludes whichever id it is given" (want [222 999], got [111 222 999]), 4 checks fail, exit 1
• moving --arg back onto gh api → fails the flag check, exit 1
• healthy tree → all six pass, exit 0

bash -n clean on both scripts; shellcheck -S warning clean on the new one; ci.yml parses as YAML.

Note

This PR is also the first one since the gh api --arg fix reached main. The reviewer workflow checks out the default branch by design (so a PR can never supply the code that reviews it), which is why the fix could not be validated on the release PR — the review job there ran main's still-broken copy. Its behaviour here is the first real end-to-end proof.

🤖 Generated with Claude Code

The change claims that stale review-comment cleanup now preserves the newly posted review and that the regression suite catches both historical failures: incorrect comment selection and invalid gh api jq-flag placement. The claim would be false if the filter selected the new comment, mishandled IDs/authors/markers, accepted either historical regression, or if the self-test were not run successfully in CI.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 28 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: cfb2870f-76ab-4228-bcd7-70a2079ab099

📥 Commits

Reviewing files that changed from the base of the PR and between 4c23da1 and ce99537.

📒 Files selected for processing (1)

• scripts/agy-review-selftest.sh

Walkthrough

The review script now centralizes stale bot-comment selection in a readonly jq filter. An offline self-test validates filtering and gh api argument usage, and CI runs it as part of linting.

Changes

Review comment selection

Layer / File(s)	Summary
Shared stale-comment filter scripts/agy-review.sh	Defines SELECT_STALE_JQ for filtering marked comments by bot author while excluding the newly posted comment, then uses it in the deletion pipeline.
Offline selection validation scripts/agy-review-selftest.sh, .github/workflows/ci.yml	Adds fixture-based assertions for comment selection, unknown IDs, supplied IDs, and invalid gh api jq flags, then runs the self-test in the lint job.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• doublegate/RustySNES#199: Earlier changes to scripts/agy-review.sh that this PR extends with filter refactoring and self-tests.
• doublegate/RustySNES#270: Introduced related review-comment deletion and filtering logic that this PR extracts and tests.

🚥 Pre-merge checks | ✅ 10

✅ Passed checks (10 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title follows Conventional Commits format and accurately describes regression coverage for the reviewer comment-selection filter.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Changelog Entry	✅ Passed	Full origin/main...HEAD diff changes only CI wiring and agy reviewer scripts; it has no CHANGELOG.md hunk and introduces no emulator, frontend, CLI, API, or cartridge behavior.
Docs-As-Spec	✅ Passed	Against origin/main, the PR changes only CI and reviewer scripts; no crates/rustysnes-/ behavior or files changed, so the matching docs requirement is not applicable.
Accuracysnes Bookkeeping	✅ Passed	The base-to-PR diff changes only CI and agy-review scripts; no test or scene under tests/roms/AccuracySNES/gen/src/ was added or removed, so the bookkeeping check is not applicable.
No Panic On Untrusted Input	✅ Passed	The commit changes only Bash and workflow YAML; added-line scans found no new .unwrap(), .expect(), or panic!() calls, so no untrusted-input panic boundary was introduced.
Safety Comment On New Unsafe	✅ Passed	The PR changes only YAML and Bash; targeted diff and added-line searches found no new unsafe block or unsafe fn, so no SAFETY comment is required.

---

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

[coderabbitai]

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/workflows/ci.yml:
- Line 105: Add a job-level permissions block to the lint job containing only
the read access required by checkout, specifically contents: read, while leaving
the existing agy-review-selftest.sh step unchanged.

In `@scripts/agy-review-selftest.sh`:
- Around line 96-101: Update the self-test around the gh api flag check to
detect --arg and --argjson anywhere in the same logical shell command, including
continuation lines after gh api. Alternatively, validate that these flags are
attached to the jq invocation rather than gh api, while preserving the existing
failure reporting and counter behavior.

🪄 Autofix (Beta)

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: 624d2514-3a33-4260-be36-061b73d3c102

📥 Commits

Reviewing files that changed from the base of the PR and between 749444e and 4c23da1.

📒 Files selected for processing (3)

• .github/workflows/ci.yml
• scripts/agy-review-selftest.sh
• scripts/agy-review.sh

📜 Review details

⏰ Context from checks skipped due to timeout. (3)

• GitHub Check: lint
• GitHub Check: accuracysnes
• GitHub Check: test-light

🧰 Additional context used

📓 Path-based instructions (2)

.github/workflows/**

⚙️ CodeRabbit configuration file

.github/workflows/**: Flag third-party actions pinned to a branch rather than a tag or commit SHA, jobs without
a least-privilege permissions: block, and secrets that could reach the log. The
ci-success job is the required check and must depend on every gate it claims to
aggregate — a new job that is not in its needs: list is a gate nothing enforces.

Files:

• .github/workflows/ci.yml

**/*

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*: Do not commit or vendor the generated snesdev_wiki/ mirror; it is gitignored and intended only as a local reference.
Keep commits focused and use Conventional Commits: <type>(<scope>): <subject>, with an imperative subject of at most 72 characters.
Do not use emojis in code, comments, or commit messages.
Before opening a PR, ensure formatting, Clippy, workspace tests, the core embedded build, rustdoc with warnings denied, documentation coverage, and changelog requirements pass.
Ticket completion must be reflected in the relevant to-dos/ sprint file.

**/*: Preserve the one-directional crate graph: chip crates must not depend on one another; rustysnes-core ties them together.
Never commit commercial ROMs; only commit derived screenshots and hashes.
Keep docs/STATUS.md as the authoritative per-subsystem status and update project documentation in the same PR as code changes.
Do not treat RustyNES v2.0 or engine-lineage anchors as project releases.

Files:

• scripts/agy-review.sh
• scripts/agy-review-selftest.sh

🪛 Shellcheck (0.11.0)

scripts/agy-review.sh

[info] 112-116: Expressions don't expand in single quotes, use double quotes for that.

(SC2016)

🔇 Additional comments (2)

scripts/agy-review.sh (1)

103-118: LGTM!

Also applies to: 603-603

scripts/agy-review-selftest.sh (1)

1-95: LGTM!

Also applies to: 103-108

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Security Misconfiguration (CWE-732): Incorrect Permission Assignment for Critical Resource

Declare least-privilege permissions for lint.

Line 105 runs PR-controlled code, but lint has no job-level permissions: block. Its token privileges therefore inherit repository defaults; declare only the read access checkout needs, such as contents: read.

As per path instructions, jobs must have a least-privilege permissions: block.

🤖 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/ci.yml at line 105, Add a job-level permissions block to
the lint job containing only the read access required by checkout, specifically
contents: read, while leaving the existing agy-review-selftest.sh step
unchanged.

Source: Path instructions

[doublegate]

Review response — all three taken, in ce99537

No blocking issues. Every finding describes a silent failure mode in a test whose entire premise is that silent failure is what went wrong twice, so all three are taken rather than argued with.

Suggestion 1 — fragile sed extraction

Right, and the dangerous half is the word silently. The range ends at the first line closing with a quote, so a filter body that ever did so would be truncated — and a truncated jq program can still compile and still return ids, so the suite would keep printing ok while checking something that is not the shipped filter. That is the same shape as the two bugs this file exists to catch.

Two independent guards now, because either alone has a gap:

1. It must compile. A truncation is usually a syntax error, though not always.
2. It must end with the | .id projection, which is what makes it a complete pipeline rather than a prefix of one. Dropping the final select(...) alone would still compile and still emit objects — this catches that.

One thing worth recording: the compile check has to pass --arg marker / --argjson new_id itself. The filter references both, and jq rejects an undefined variable at compile time, so the first version of the check failed a perfectly good program. Caught by running it, which is the only reason it is right now.

Suggestion 2 — line-based gh api ... --arg scan

Real, and a false pass on precisely the mistake the check exists to catch is the worst possible outcome for it. Continuations are folded before matching now:

sed -e ':a' -e '/\\$/{N;s/\\\n//;ba' -e '}' agy-review.sh | grep -qE 'gh api[^|]*--(arg|argjson)'

Nitpick 3 — trailing space from tr '\n' ' '

Taken. paste -sd' ' instead, and the expectations now read as what they are ("111 222") rather than carrying padding an assertion had to mirror.

Verification

Both new guards were confirmed by injecting the exact failure each describes:

injection	result
drop the final | .id from the filter	FAIL: extracted SELECT_STALE_JQ does not end in '| .id'; extraction truncated — exit 1
move --arg onto a continuation line	FAIL --arg/--argjson passed to \gh api`` — exit 1
healthy tree	all six checks pass, exit 0

shellcheck -S warning clean.

---

Aside: this PR is the end-to-end proof of the #270 fix

Worth stating since it was the open question when this branch was opened. On this PR the review job passed (54s) — its first green since #270 broke it — and it left exactly one bot comment.

Both halves matter, and they fail independently: the job passing does not by itself prove the delete step runs, since the failure mode was "post fine, then die". One comment on a PR whose review has run is the positive evidence the gh api | jq pipeline now completes. For contrast, #272 accumulated four during the broken window.

[github-actions]

Antigravity review (Gemini via Ultra)

Adds an offline self-test script (scripts/agy-review-selftest.sh) to CI that validates agy-review.sh's comment-selection jq filter and command formatting against regression fixtures.

Blocking issues

None found.

Suggestions

• scripts/agy-review-selftest.sh:L121-L123: The check for --arg/--argjson flags scans all lines matching gh api with grep. Any explanatory comment in agy-review.sh containing gh api --arg will cause a false-positive selftest failure. Strip code comments (e.g., filtering out ^\s*#) prior to running the grep pattern check.
• scripts/agy-review-selftest.sh:L33-L36: extract_filter() relies on brittle line-range matching (/^SELECT_STALE_JQ='/,/'\$/p). If agy-review.sh is refactored to use double quotes or an inline comment on the closing quote line, extraction will fail or truncate. Consider letting agy-review.sh support a --dump-filter CLI flag to print SELECT_STALE_JQ rather than parsing bash source via sed.

Nitpicks

• scripts/agy-review-selftest.sh:L121: The multiline line-folding sed script (s/\\\n//) relies on GNU sed handling of \n in replacement patterns, which behaves differently in BSD/macOS sed if developers attempt to run the selftest locally outside of Linux.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}