feat(mongodb-query-index-check): tune review prompt from four months of PR feedback

[mtrunkat]

Analysis of every finding the check posted on apify/apify-core and apify/apify-web PRs since it shipped in mid-May: 111 unique findings on 83 PRs (plus 14 duplicate threads and 19 findings on the two DO-NOT-MERGE stress-test PRs). Outcomes: 74 fixed (67%), 13 consciously declined with reasoning, 12 ignored, 3 disproven with explain plans, 9 on open/abandoned PRs. This PR folds the recurring failure modes back into prompts/review.md.

What the data showed → what changed

Duplicates are the check's most common failure mode (14 duplicate threads): the same finding posted three times in five minutes (apify-core#27675), verbatim re-posts two days later (apify-core#28250), a re-post with severity escalated medium→high (apify-core#27941), and re-posts after migration-file renames (apify-core#30151).
→ Dedup step rewritten: match by substance (same query, same problem class) instead of (path, line); outdated and resolved threads now count; never re-post with changed severity; never follow a landed fix with a downgraded "residual advice" version of the same finding.

One-off migrations carried 27 findings — a quarter of the total, including 11 of the 15 criticals — and engineers accept the scan ("It is migration, so it is fine") as often as they fix it. mtrunkat suggested a migrations policy on apify-core#28597.
→ New "One-off migrations" section: operational advice (hint, batching, replica reads, timing window, explicitly accepted scan) instead of new indexes; a reasoned author accept settles the finding; content-based matching across timestamp renames. An unbatched scan of a multi-million-doc collection stays critical.

All 3 false positives were confident planner claims an explain() refuted: partial-index subsumption of $ne: null (apify-core#28260, "Clanker is not clanking"), $expr $lookup using the index (apify-core#29506), and the planner picking the right index without a hint (apify-core#28395).
→ Planner behavior must be phrased as a testable hypothesis with an invitation to explain(); the $expr equality-join rule no longer claims a collscan and drops to 🟡 medium; hint only when the plan choice is genuinely ambiguous; explain-plan evidence in a reply settles the finding.

Severity triage correction from the check's owner (apify-core#28597): "the severity shall be critical in a case of collection scan with millions of items".
→ Codified in the 🔴 critical rubric — no downgrading because the code path is a daemon or infrequent.

Reasoned declines used to keep the check red forever, which trains people to ignore it (12 findings were merged over silently).
→ Findings settled by a substantive human reply (explain plan, field-presence check, one-off-migration reasoning, API contract) stop counting toward the check result. Behavioral change: a re-run after a reasoned decline now goes green.

The bot's concrete recommendations were occasionally wrong: a suggested index on four fields that don't exist on the type (apify-core#29248), a partialFilterExpression Mongo wouldn't select (apify-core#27675), a keyset-pagination suggestion against an offset/limit API contract (apify-core#30289), and a regex finding "fixed" by dropping the i flag while staying unanchored (apify-core#30484).
→ New guardrails: verify recommended fields exist on the document type; propose partial filters only when provably selectable; check API contracts before suggesting keyset pagination; call out $ne→equality missing-field semantics; make suggested actions complete and atomic (name the load-bearing part).

README updated to reflect the settled-findings and migrations behavior.

Not changed

• Cross-PR duplicates (the same finding on sibling PRs apify-core#29900/apify-core#29901) — the prompt only sees one PR; would need action-level state.
• Findings posted minutes before (or seconds after) merge get ignored — that's a timing property of when the workflow runs, not of the prompt.

Full per-PR data and the weekly/per-engineer breakdown are in the HTML scorecard report generated alongside this PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QGiLipa4LSVzsdXUWKA4RW

---

Generated by Claude Code

[Copilot]

🟡 Changes recommended

Unresolved critical, moderate, and nit findings remain.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR tunes the MongoDB query-index review prompt using historical feedback.

Changes:

• Adds migration-specific guidance and severity rules.
• Refines planner and recommendation guardrails.
• Updates deduplication and settled-finding handling.

File summaries

File	Summary
mongodb-query-index-check/README.md	Updates behavior and migration documentation; two nit findings remain.
mongodb-query-index-check/prompts/review.md	Adds revised review and deduplication rules; two critical and three moderate findings remain.

Review details

Suppressed comments (7)

mongodb-query-index-check/README.md:105

• This migration summary advertises “replica reads” without limiting them to read-only migrations, although the prompt also covers updateMany scans. A secondary cannot execute the write or offload its scan; qualify this advice for read-only operations and document primary-side batching/timing for writes.

One-off migration scripts get operational advice (a `hint`, batching, replica reads, a timing window, or an explicitly accepted scan) instead of new-index recommendations; an unbatched scan of a multi-million-document collection is still critical even there.

mongodb-query-index-check/README.md:103

• The README's top-level behavior summary still says the action fails whenever a finding is reported, while this new exception means settled findings no longer fail the result. Update that earlier summary as well so users are not given contradictory behavior guarantees.

Any finding turns the check red unless `request-changes` is set to `false` — except findings a human already settled with a reasoned reply on an earlier run's thread (an explain plan, "one-off migration, bounded set", an API constraint), which stop counting toward the result.

mongodb-query-index-check/prompts/review.md:64

• Because this migration section also covers updateMany and other writes, “reading from a replica / secondary” is not a valid mitigation for those cases: MongoDB writes must target the primary, so readPreference cannot move the scan off the primary. Restrict replica-read advice to read-only migrations and use batching, hints, or a traffic window for writes.

- **Never recommend adding an index for a one-off migration.** Suggest, in order: a `hint` on an existing index, batching with a bounded batch size, reading from a replica / secondary, running in a low-traffic window, or explicitly accepting the scan with a short comment in the code.

mongodb-query-index-check/prompts/review.md:201

• The first sentence correctly says plan choice is a hypothesis, but the final sentence tells the model that one obvious index "will" be chosen and to skip a hint. A single candidate can still lose to a collection scan or another cost-based plan, so this can suppress real plan-choice issues; avoid prescribing a hint solely from candidate count, but still require an explain() when plan selection is material.

- **State planner behavior as a testable hypothesis, not a certainty.** Every disputed finding in this check's history was a confident planner claim that an `explain()` disproved (partial-filter subsumption of `$ne: null`, `$expr` join index use, plan selection without a `hint`). When your finding hinges on what the planner will choose — rather than on an index that plainly doesn't exist — say so and ask for an `explain()` to confirm. Suggest a `hint` only when the collection genuinely has many overlapping candidate indexes for this filter shape; when one index is the obvious single choice, the planner will pick it — skip the hint.

mongodb-query-index-check/prompts/review.md:213

• This matching rule is still too coarse for deduplication: two different queries on the same collection can have the same problem class (for example, two Users.find calls both missing a shard-key predicate), so the first thread would suppress the second finding. Require query identity—filter/sort/projection or equivalent query content—in addition to collection and problem class; migration renames can still use that content key.

3. **Deduplicate by substance, not by `(path, line)`.** A finding is a duplicate when an existing thread describes the same underlying issue on the same query — same collection, same problem class (e.g. "sort not covered by index", "filter without shard key") — even if the line shifted, the thread is outdated, or the file was renamed (migration files get renamed on rebase; match them by content). Post a finding on a previously-flagged query only when the new commits introduce a genuinely different problem. Never re-post an existing finding with a changed severity, and never follow a fix up with a downgraded "residual advice" version of the same finding — if the fix landed, the thread is done.

mongodb-query-index-check/prompts/review.md:212

• GitHub's default workflow bot login is github-actions[bot] (and a custom github-token may use yet another identity), not the literal github-actions value. This predicate can therefore collect none of the prior bot threads, so reruns will repost duplicates and cannot honor settled replies.

2. Collect ALL threads — **outdated and resolved ones included** — whose first comment's `author` is `github-actions` and whose `body` starts with the template prefix defined in step 4 (any of the four severity emojis followed by ` **MongoDB index check`). Read the human replies in each.

mongodb-query-index-check/prompts/review.md:211

• get_review_comments is cursor-paginated, but this step only describes one call. On a PR with more than one page of review threads, older/outdated/settled comments will be omitted and the check can still repost them; iterate until hasNextPage is false and pass the next page's cursor.

1. Call `mcp__github__pull_request_read` with `method: get_review_comments` and `pullNumber: $PR_NUMBER`. The response is a list of review threads; each has `is_outdated` and a `comments` array (every comment has `author`, `body`, `path`, `line`).

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.