| name | bagisto-code-review |
| description | Use when reviewing Bagisto code changes or a pull request for correctness, convention compliance or quality, or when asked whether a change is ready to merge. Trigger phrases include "review", "code review", "PR review", "is this correct", "conventions", "violations", "code quality", "ready to merge". |
| requires | bagisto-coding-standards |
| license | MIT |
Code Review
What to look for in a Bagisto change, ordered so the findings that matter arrive
first. Pint and the test suites already decide the mechanical questions — spend
the review on what they cannot see.
What the tools already cover
Do not spend review effort on these; run them instead.
| Checked by | Covers |
|---|
vendor/bin/pint --test | Formatting, spacing, import order, trailing commas |
vendor/bin/pest | Behaviour the suite asserts |
php artisan bagisto:translations:check | A key missing from any of the 22 locales |
| Playwright | The browser layer |
If a review comment could have been an exit code, the fix is to run the tool,
not to write the comment.
Blocking
A change should not merge with any of these outstanding.
Correctness
- A query inside a loop, or an N+1 from a missing eager load. The cost is
invisible on seeded data and appears on a real catalogue.
- An unbounded
->get() on a table that grows — products, orders, customers.
Paginate or chunk.
- A repository method that touches seller- or customer-owned rows without
scoping to the owner.
- A raw fragment (
whereRaw, selectRaw, DB::raw, orderByRaw) built by
interpolating a request value.
Security
Owned by the bagisto-coding-standards skill — load it when the diff touches
authorization, rendered output, input, uploads, raw SQL, secrets or payments,
and work its checklist rather than a remembered list. The two that account for
most real findings:
- A storefront query selecting by an id from the request without scoping to the
authenticated customer.
- A DataGrid closure interpolating a value into an HTML attribute without
e() — tags are stripped for you, quotes are not.
Architecture
DB:: or model queries outside a repository. The one sanctioned exception is
a DataGrid's prepareQueryBuilder().- A new model without its Contract, Model, Proxy and Repository.
- A package registered in
bootstrap/providers.php but not config/concord.php,
or the reverse.
- Core files edited to serve an extension. Marketplace and B2B Suite both forbid
core edits outright — the change belongs in a bound subclass, a
view_render_event listener, or a Concord model swap.
- A user-facing string not passed through
trans().
Worth raising, not blocking
- Duplication on the third occurrence. Twice is a coincidence; three times
is a helper.
- A method whose body needs a comment to follow. The codebase forbids
comments inside method bodies, so this is a signal to extract a named method,
not to add prose.
- A docblock or member order violation in a file the change touches. A
pre-existing violation in a touched file is the author's to fix.
- A test that asserts a count or a list position. Both drift as the shared
database grows; assert on the named record instead.
- An event fired on the single-record path but not the mass-action path, or
the reverse.
How to review
- Read the tests first. They state what the author believes the change
does. A change with no test for the bug it fixes is the first question.
- Ask what breaks it. For each claim, look for the input that falsifies it —
an empty collection, a second locale, a guest, a channel that is not the
default.
- Check the reverse. A regression test that passes with the fix reverted
guards nothing. Where the fix is subtle, ask the author to show it failing.
- Follow one path end to end — request, form request, controller,
repository, model, view — rather than reading the diff hunk by hunk. Most
real defects sit in the seam between two files that each look fine.
- Confirm the gates ran, and which were skipped.
Writing the finding
State the defect, then the input that triggers it, then the fix. A finding
without a concrete failure is a preference, and should be marked as one.
CategoryRepository::saveMediaAltText() writes to
translateOrNew($locale) where $locale can be the literal all — the
category create form posts locale=all. On MySQL that silently creates a row
with an empty name and slug; on PostgreSQL it violates NOT NULL. Expand
the sentinel to every locale before writing.
Separate what you verified from what you suspect. "This is an N+1" and "this
might be an N+1, I did not check the relation" are different claims, and
conflating them costs the author more time than saying so.
REQUIRED SUB-SKILL: Use bagisto-change-verification before calling any change done.
