Discourse PR #9 — Live Benchmark — FriendlyReviewer

Golden Comments (3/3 found)

#	Comment	Severity	Status	FriendlyReviewer Detail
#1	`destroy_all` in `add_automatically` can permanently delete legitimate group members during an unrelated `save`	HIGH	✓ Found	FriendlyReviewer: "`add_automatically` calls `destroy_all` on existing members. This will PERMANENTLY delete legitimate group members if triggered during an unrelated `save` call. Use `transaction` + safe guard." (group.rb L180)
#2	Infinite recursion: `add_automatically` calls `save` which calls `add_automatically` when automatic group memberships are configured	HIGH	✓ Found	FriendlyReviewer: "INFINITE LOOP: `add_automatically` → `save` → `add_automatically` when automatic group memberships are configured. The `before_update` guard does not prevent re-entrance." (group.rb L180)
#3	`JSON.parse(nil)` crashes with `TypeError` when `grant_trust_level` is nil	HIGH	✓ Found	FriendlyReviewer: "`JSON.parse(group.grant_trust_level)` — if `grant_trust_level` is nil, `JSON.parse(nil)` raises `TypeError: no implicit conversion of nil into String`. Add `grant_trust_level.present?` guard." (group.rb L195)

Supplementary Findings (1 finding)

Finding	File	Severity	Legitimate?
`destroy_all` triggers callbacks: `after_destroy` may send notifications to all deleted members	group.rb:200	LOW	✓ Minor but valid — use `delete_all` instead

Honest Analysis

FriendlyReviewer detected all 3 golden bugs in this PR — a perfect 100% recall. This is the strongest result across the entire Discourse benchmark. Every golden finding is a High severity bug (data loss via destroy_all, infinite re-entrance loop, JSON.parse(nil) crash), and FriendlyReviewer identified each one with precise diagnostics and concrete fix suggestions.

The single supplementary finding is a Low severity observation about callback overhead from destroy_all. It is legitimate but minor — FriendlyReviewer recommends delete_all to avoid sending spurious after_destroy notifications. There are no medium-severity supplementary findings, which is consistent with this being a small refactor on a single model. No false positives, no severity inflation, no noise.

This PR is FriendlyReviewer's best performance in the benchmark: perfect golden recall, no missed critical bugs, and zero hallucinated findings. The review is concise and impactful — every line of feedback addresses a real production risk. The only supplementary finding is Low severity and is excluded from the coverage tally per the benchmark methodology.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👏

🌥️ Tech
3 high, 0 medium, 1 low

The MR addresses group membership ownership but introduces critical data-loss risks. Key issues include: (1) potential data loss via destroy_all in add_automatically — this deletes ALL members before re-adding, which could permanently remove legitimate group members during an unrelated save call; (2) infinite recursion between save, add_automatically, and save when automatic group memberships are configured; (3) JSON.parse(nil) will crash when grant_trust_level is nil; (4) destroy_all trigger firing callbacks like after_destroy notifications for every deleted user. The performance dashboard query (group_requests_timeframe) introduces an unnecessary SQL call on every request, and the test should validate the number of group memberships. Good: using before_update instead of before_save correctly scopes the guard, and the test coverage for the ownership migration is decent.

These might need a close look

🔴 app/models/group.rb (L180)
add_automatically calls destroy_all on existing members. This will PERMANENTLY delete legitimate group members if triggered during an unrelated save call. Use transaction + safe guard.
🔴 app/models/group.rb (L180)
INFINITE LOOP: add_automatically → save → add_automatically when automatic group memberships are configured. The MR adds a before_update guard but it does not prevent re-entrance if save is called from within automatic group membership logic.
🔴 app/models/group.rb (L195)
JSON.parse(group.grant_trust_level) — if grant_trust_level is nil, JSON.parse(nil) raises TypeError: no implicit conversion of nil into String. Add grant_trust_level.present? guard.

Worth checking

None — the FR didn't find any medium-severity supplementary issues for this PR.

Small things (take or leave)

🔵 app/models/group.rb (L200)
destroy_all triggers callbacks: after_destroy may send notifications to all deleted members. Use delete_all instead to avoid callback overhead.

FIX: proper handling of group memberships