Discourse PR #10 — Live Benchmark — FriendlyReviewer

Golden Comments (2/4 found)

#	Comment	Severity	Status	FriendlyReviewer Detail
#1	NoMethodError `before_validation` in `EmbeddableHost` — if `host` is nil, `self.host.sub!(...)` crashes (NoMethodError). `sub!` also causes frozen string errors.	CRITICAL	✓ Found	FriendlyReviewer found it exactly: "No nil-guard in `before_validation`. If `host` is nil, `self.host.sub!(...)` raises NoMethodError. Additionally, `sub!` mutates the receiver and causes frozen string errors." (embeddable_host.rb:8)
#2	The `update` and `destroy` methods in `Admin::EmbeddableHostsController` don't validate the record exists → nil → NoMethodError	MEDIUM	✓ Found	FriendlyReviewer found it exactly: "If `params[:id]` does not match any EmbeddableHost, `host` will be nil and `save_host(nil)` raises NoMethodError." (embeddable_hosts_controller.rb:10)
#3	`record_for_host` compares `lower(host) = ?` but doesn't normalize the parameter case → mixed-case referer may fail	MEDIUM	✗ Missed	FriendlyReviewer did not specifically identify this case normalization issue in `record_for_host`.
#4	The migration uses raw SQL → existing values with `http://` or paths bypass normalization → lookup may fail	HIGH	✗ Missed	FriendlyReviewer found a different issue in the migration (SQL injection), but missed the migrated data normalization problem.

Supplementary Findings (13 findings)

Finding	File	Severity	Legitimate?
Fabricator file corruption — `category_fabricator.rb` overwritten with the embeddable_host fabricator, destroying all existing category fabricators	category_fabricator.rb:1	HIGH	✓ Crash in tests
SQL injection — the `host` value from the site setting is interpolated directly into SQL	create_embeddable_hosts.rb:25	HIGH	✓ Security
`Admin::EmbeddingController#update` no-op — renders the serialized data but never saves any settings	embedding_controller.rb:8	HIGH	✓ Critical functional bug
`embed: :ids` mismatch — the frontend expects full host objects but the serializer uses `embed: :ids`	embedding_serializer.rb:3	HIGH	✓ Integration bug
Hostname regex too restrictive (TLD capped at {2,5}) + missing uniqueness/presence validations	embeddable_host.rb:3	MEDIUM	✓ Legitimate
`resources :embeddable_hosts` without `only:` → routes for unimplemented actions	routes.rb:154	MEDIUM	✓ Legitimate
Insufficient coverage — missing CRUD, auth, validation in embeddable_hosts_controller_spec	embeddable_hosts_controller_spec.rb:1	MEDIUM	✓ Legitimate
Insufficient coverage — show and update not tested in embedding_controller_spec	embedding_controller_spec.rb:1	MEDIUM	✓ Legitimate
`record_for_host` returns `false` instead of `nil` → subtle issues for callers	embeddable_host.rb:13	LOW	✓ Anti-pattern
Inconsistent capitalization: `edit: "edit"` lowercase vs other labels using title case	client.en.yml:2497	LOW	✓ Cosmetic
Stale translation keys: `embeddable_hosts` / `embed_category` removed from server.en.yml but persist in other locale files	server.en.yml:1167	LOW	✓ Legitimate
Misleading test description: 'is false if embeddable_host is blank' but the implementation no longer checks that	topic_spec.rb:1403	LOW	✓ Legitimate
Typo: 'emeddable' → 'embeddable' in a describe block name	topic_spec.rb:1406	LOW	✓ Typo

Honest Analysis

FriendlyReviewer found golden #1 (NoMethodError before_validation) and #2 (nil ref update/destroy). Both were perfectly identified — FriendlyReviewer even added the frozen string sub-issue for #1, which the benchmark didn't mention.

Golden #3 (case normalization in record_for_host) and #4 (migration data normalization) were not found. These are subtle business logic bugs — FriendlyReviewer did find the SQL injection in the same migration (#4), but missed the "migrated data normalization" angle.

The supplementary findings are high quality. The fabricator file corruption (category_fabricator.rb overwritten) is a test disaster — all specs using the Category fabricator silently break. The no-op update in EmbeddingController is a critical functional bug (the save never persisted). The SQL injection in the migration is a real security issue. The serialization mismatch (embed: :ids vs full objects) will break the frontend UI.

On severity, FriendlyReviewer's HIGH classification is appropriate for critical bugs (fabricator, SQL injection, no-op, nil ref, frozen string), though the test coverage and route findings could have been MEDIUM rather than HIGH. Still, the classification is consistent with other PRs in the Discourse benchmark.

Overall, 2/4 golden found (50%), with 13 supplementary findings including 6 high-severity ones. The quality of the findings compensates for the modest recall rate — the bugs discovered outside the benchmark (fabricator corruption, no-op update, SQL injection) are potentially more severe than the missed golden comments.

FriendlyReviewer's full review

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

friendlyreviewer-staging bot commented now

Hi there 👏

🌥️ Tech
6 high, 4 medium, 5 low

The MR migrates embeddable hosts from SiteSettings to a proper database model with per-host category associations. Key findings include: (1) Fabricator file corruption in category_fabricator.rb which was overwritten with the embeddable_host fabricator, destroying all existing category fabricators; (2) SQL injection in the migration where the host value from the site setting is interpolated directly into SQL; (3) Admin::EmbeddingController#update is a no-op that renders but never saves any settings; (4) Nil-reference crashes when params[:id] doesn't match any record; (5) before_validation crashes on nil host or frozen strings. The frontend expects full host objects but the serializer uses embed: :ids. Test coverage is insufficient for the new controller and missing validations.

These might need a close look

🔴 app/models/embeddable_host.rb (L8)
No nil-guard in before_validation. If host is nil, self.host.sub!(...) raises NoMethodError. Additionally, sub! mutates the receiver and causes frozen string errors.
🔴 app/controllers/admin/embeddable_hosts_controller.rb (L10)
If params[:id] doesn't match any EmbeddableHost, host will be nil and save_host(nil) raises NoMethodError.
🔴 db/migrate/20150818190757_create_embeddable_hosts.rb (L25)
SQL injection: the host value from the site setting is interpolated directly into SQL. Use parameterized queries.
🔴 app/controllers/admin/embedding_controller.rb (L8)
Admin::EmbeddingController#update renders the serialized embedding but never saves any settings. The show and update actions are identical.
🔴 spec/fabricators/category_fabricator.rb (L1)
Fabricator file corruption: category_fabricator.rb was overwritten with the embeddable_host fabricator, destroying all existing category fabricators.
🔴 app/serializers/embedding_serializer.rb (L3)
Serialization mismatch: embedding serializer uses embed: :ids but the frontend expects full host objects. Should use embed: :objects.
🟡 app/models/embeddable_host.rb (L3)
Hostname regex too restrictive: TLD length limited to {2,5} but modern gTLDs exceed that. Missing uniqueness and presence validations.
🟡 config/routes.rb (L154)
Unnecessary routes: resources :embeddable_hosts generates routes for unimplemented actions. Add only: [:create, :update, :destroy].
🟡 spec/controllers/admin/embeddable_hosts_controller_spec.rb (L1)
Insufficient controller test coverage: both embeddable_hosts_controller_spec and embedding_controller_spec only test class hierarchy, missing CRUD, auth, validation, and edge cases.
🟡 spec/controllers/admin/embedding_controller_spec.rb (L1)
Insufficient controller test coverage: show and update actions not tested.

Small things (take or leave)

🔵 app/models/embeddable_host.rb (L13)
Anti-pattern: record_for_host returns false on failure instead of nil. Callers expecting nil may behave unexpectedly.
🔵 config/locales/client.en.yml (L2497)
Inconsistent capitalization: edit: "edit" uses lowercase while other action labels use title case.
🔵 config/locales/server.en.yml (L1167)
Stale translation keys: embeddable_hosts and embed_category were removed from server.en.yml but persist in other locale files.
🔵 spec/models/topic_spec.rb (L1403)
Misleading test description: 'is false if embeddable_host is blank' but the implementation no longer checks that.
🔵 spec/models/topic_spec.rb (L1406)
Typo: 'emeddable' should be 'embeddable' in a describe block name.