Code review quality depended on which senior was on duty. That was the bug.
Not a missing linter, not a bad process doc - the review you got depended on who was around. So I built a GitHub Action to set a floor. This post is about the boring parts that actually took time: a Rack 3.2 gotcha that ate my webhook, a Sequel JSONB trap, and teaching an LLM to shut up.
The shape of the thing
The flow is simple on paper:
PR opened/updated
→ GitHub sends a webhook
→ API pulls the diff
→ LLM reviews it against a ruleset
→ Action posts inline comments on the PR
The Action itself is thin - it just calls the API and posts results. All the logic lives server-side, which means I can fix a prompt without asking anyone to update a workflow file.
Gotcha 1: Rack 3.2 ate my request body
The API is Grape on Rack 3.2. First version used standard declared params. Worked in tests, died in production: every webhook came through with all fields "missing".
The reason: something upstream consumed the request body before the endpoint read it, and under Rack 3.2 the input stream is not rewindable. By the time Grape validated params, the body was an empty string.
The fix was to stop fighting the framework:
post '/v1/webhooks/github' do
payload = JSON.parse(request.body.read)
# symbolize keys, validate manually, move on
end
Dropped Grape param validation for that endpoint entirely. Manual parse, manual check. Less elegant, but it works and it's obvious.
Lesson: when a body is consumed once and can't be rewound, middleware order is part of your API contract whether you like it or not.
Gotcha 2: Sequel JSONB is a DelegateClass
Second bug was quieter. Review results are stored as JSONB in Postgres via Sequel. At some point the dashboard started showing "0 issues, score null" for reviews that clearly had findings.
The cause: Sequel::Postgres::JSONBHash is a DelegateClass(Hash). Which means value.is_a?(Hash) returns false. My serialization code checked is_a?(Hash) and silently skipped every JSONB field.
I've hit this twice now. The rule I follow: never check is_a?(Hash) on a JSONB column - coerce explicitly first.
The LLM part: teaching it to shut up
The hardest part wasn't the plumbing, it was calibration. An LLM reviewer's default setting is to have opinions about everything - and a reviewer that nitpicks every import gets muted within a week.
The diff alone is useless. My first prompt was "here's the diff, review it". The model reviewed the diff - and invented problems in code it couldn't see. It flagged a renamed method as "possibly unused" because the caller lived in a file outside the diff. The fix: give it the diff plus the surrounding context of touched files, and explicitly tell it which files it can and cannot see. A model that knows the limits of its visibility stops guessing.
Structured output, not prose. I ask for JSON: file, line, severity, comment. Free-form markdown reviews look impressive and are useless to post as inline comments - you end up regex-parsing your own LLM output. With structured output, posting is mechanical, and I can drop findings before they reach the PR.
Filtering before posting. Not every finding deserves a comment. I filter out:
- style nits a linter already covers (rubocop, eslint exist for a reason)
- "consider adding a docstring" on internal helpers
- anything the model marks below a severity threshold
What survives gets posted. The ratio of dropped-to-posted findings is the single most important number in the whole system, and it's tuned by hand, not by a benchmark.
"No comment" is a valid outcome. Early versions felt compelled to say something on every PR. Now the prompt explicitly says: if the change is fine, return an empty list. A reviewer that stays silent on a clean PR builds trust; one that invents a nit to justify its existence destroys it.
Rulesets instead of one giant prompt. Instead of one prompt trying to be an expert in everything, the review runs against a ruleset - a set of focused instructions (security, error handling, naming). This makes behavior explainable: when a user asks "why did it flag this", the answer is a rule, not "the model felt like it". It also means I can improve one area without regressing the others.
What I'd do differently. I'd start with fewer, stricter rules and expand. Starting broad means weeks of tuning noise down; starting narrow means weeks of adding value up. Same effort, opposite morale.
I don't have hard numbers to share - this is a young project - but the direction is clear: the value of an automated reviewer is measured by what it doesn't say.
Fail-closed, not fail-open
One design decision I'd defend: when the AI is unavailable, the Action does nothing graceful like auto-approving. It escalates to human review. A reviewer that makes things up when confused is worse than no reviewer.
What's next
GitLab support, configurable rulesets per repo, a summary report on the PR. It's early - the repo is review-action, the product page is code-critic.com.
If you've built something similar, I'm curious what your false-positive experience looks like - that's the part nobody writes about honestly.
Top comments (2)
Since you asked for honesty on false positives: the number that changed our behavior wasn't findings-per-PR, it was precision measured on real commit history, per rule — synthetic fixtures were a warmup at best.
Our gate is static (rule-based, no LLM in the loop), but the calibration pain rhymes. First representative backtest on 92 commits assembled from real code across a dozen repos: 1,049 findings → 200 after one fix round; findings on clean commits 39 → 6. The single biggest win was your exact instinct — silence beats guessing. Any check whose receiver we couldn't resolve stayed silent instead of flagged, precision-first. Your "a model that knows the limits of its visibility stops guessing" is the LLM version of the same rule.
Two things I'd add from the same trenches:
"No comment" needs a declared scope. GitHub truncates large diffs (300 files / 20k lines / 1MB), so a silent reviewer might be clean, or might never have seen it. We made truncation always emit a "files not checked: N" signal. A reviewer that says nothing on purpose builds trust; one that silently didn't look doesn't.
Freeze the sample before you tune. "Tuned by hand" is fine, but if the sample moves under you, you're tuning to noise. We keep a labeled commit set pinned and re-score it after every ruleset change before the dropped-to-posted ratio is allowed to move.
Curious where your ratio sits now — and whether the "invented problems in code it couldn't see" class ever fully went away, or just got quieter.
the dropped-to-posted ratio as the number to watch makes sense. once no comment counts as a valid outcome, the reviewer stops having to sort the noise from the actual findings.