Come usare
- Copia il comando e controlla il pacchetto prima di installarlo nel tuo ambiente agente.
- Controlla strumenti, credenziali e ambienti richiesti nel documento originale.
- Descrivi input e risultato desiderato; confronta l’output con le fonti.
npx skills add https://github.com/bagisto/agent-skills --skill bagisto-code-reviewPrima di iniziare
- L’installazione non include abbonamenti, quote API o accesso ai file.
- Stelle e installazioni indicano attività, non un test indipendente di qualità o sicurezza.
Contenuto dal repository con titoli, esempi, codice, tabelle, link e immagini preservati.
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.phpbut notconfig/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 totranslateOrNew($locale)where$localecan be the literalall— the category create form postslocale=all. On MySQL that silently creates a row with an emptynameandslug; 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.
