Reviewing Odoo code before it merges — your own AI-generated patch or someone else's PR. Use after writing or generating an Odoo change and before commit, or when asked to review/audit an Odoo module, diff, or addon. Catches the Odoo-specific defects AI confidently ships that lint and "it ran for me as admin" miss: blanket sudo() / privilege bypass, missing ACL or record rules, N+1 recordset loops, incomplete @api.depends on stored computes, patching core instead of a separate addon, wrong inheritance mode / MRO layer, deprecated v≤16 syntax (attrs/states/<tree>/name_get), guessed field & method names, unwired __init__ / manifest, data-loss-on-upgrade renames, and public routes with the wrong auth. Pairs with odoo-testing (review finds it, tests prove it). Read ground truth from the running instance to confirm a suspicion — don't guess whether a field or rule exists. Targets Odoo 17/18/19.
Reviewing Odoo code before it merges — your own AI-generated patch or someone else's PR. Use after writing or generating an Odoo change and before commit, or when asked to review/audit an Odoo module, diff, or addon. Catches the Odoo-specific defects AI confidently ships that lint and "it ran for me as admin" miss: blanket sudo() / privilege bypass, missing ACL or record rules, N+1 recordset loops, incomplete @api.depends on stored computes, patching core instead of a separate addon, wrong inheritance mode / MRO layer, deprecated v≤16 syntax (attrs/states/<tree>/name_get), guessed field & method names, unwired __init__ / manifest, data-loss-on-upgrade renames, and public routes with the wrong auth. Pairs with odoo-testing (review finds it, tests prove it). Read ground truth from the running instance to confirm a suspicion — don't guess whether a field or rule exists. Targets Odoo 17/18/19.
Odoo code review — the second gate
odoo-testing proves a change does what it claims; this skill catches what it
claims that it shouldn't, and what it silently broke elsewhere. AI-generated
Odoo code is the dangerous case: it's syntactically clean, it "works for admin
on my machine", and it fails later — for a non-admin user, on the second
company, on a list of records, on -u, or on a hot list view. Review against
the Odoo-specific contracts a linter can't see.
The rule: every finding is either confirmed against the running instance
(via odoo-introspect) or written down as an explicit assumption to verify —
review never adds new guesses.
Version floor: Odoo 17/18, through Odoo 19 (current LTS). Deprecation checks below assume 17/18+; for older
targets — and for the v18.1 → 19 renames — see skills/odoo-introspect/references/version-matrix.md.
How to review (in order)
Get the diff and the ground truth. Read the change; run odoo-ai all <model>
for every model it touches (fields, MRO, security, depends). Half of "review"
is just comparing the patch to what the brief says already exists.
Run the checklist below, severity-first (security → data loss → silent
correctness → performance → style).
Confirm, don't assert. "This field may not exist" → check the brief. "This
override may not run" → read the MRO / trace it. Turn each suspicion into a
fact or a flagged assumption.
Hand correctness findings to odoo-testing — a real defect deserves a
failing test, not just a comment.
The review checklist (severity-ordered)
Security (highest stakes — fails open, not loud)
No blanket sudo() — every sudo() has a one-line reason and is the
narrowest sub-call, not a method-wide bypass. A sudo() added to silence an
AccessError is almost always hiding a correct record rule (→ odoo-security).
New model ships ir.model.access.csv — all four perm_* set
deliberately; otherwise non-admins get a silent 403 while admin "works".
Record rules considered — multi-company / "own records" isolation is a
rule, not Python filtering. Confirm rules exist in the brief.
Field-level groups= on sensitive fields, not deletion/Python guards.
Public routes (@http.route) use the right auth=; no csrf=False on
a session POST; no controller-wide sudo() (→ odoo-web).
No SQL/domain injection — user input is parameterized, never f-string'd
into cr.execute or a domain.
Data loss & upgrade safety
Field/model rename has a pre-migration — a bare Python rename drops the
old column and creates an empty one (→ odoo-migration). Silent data loss.
Changed stored-compute logic schedules a recompute — existing rows keep
stale values otherwise.
New required field on a populated model is backfilled in pre- — not
post- (too late for the NOT NULL).
Edited shipped data respects noupdate — UI-editable config is
noupdate="1"; a protected record needs a migration, not a -u.
Silent correctness
Field & method names exist — checked against the brief, not memory
(account.move not account.invoice, real partner_id not invented).
Override lands at the intended MRO layer — __manifest__.depends
includes the addon that owns the method (depend on sale_stock, not just
sale, to extend a stock hook). Confirm against the brief's recommended depends.
super() is called (and in the right place) — a layer that drops
super() cuts the chain; an early return skips it. Check the MRO analysis;
trace big flows.
Right hook, not the public shell — value-building belongs in a
_prepare_* / _action_* / _get_* hook, not an override of
create/write/action_* (→ odoo-dev, odoo-domain-playbooks).
Built-in, not hand-rolled — derived value = computed/related field;
rule = @api.constrains; numbering = ir.sequence. Procedural code in
write() to compute/validate is a smell.
create override is batch-safe — @api.model_create_multi, vals is
a list; tested on a multi-record recordset.
Wired in — every new .py imported in its __init__.py; every new
data/view file listed in __manifest__['data'] in dependency order; new OWL/JS
in the correct assets bundle. Unwired = dead code, no error.
Performance
No query in a loop — no search/browse per record; search once with
in, then filtered/mapped (→ odoo-perf).
@api.depends is exhaustive on every stored compute (dotted paths
included) — audit against model_brief.fields[].depends. A missing dep silently
rots the value.
store=True is justified (search/group/report on it), not "just in
case" — stored computes add write amplification.
Batch write/create, not field-by-field in a loop.
New index only where searched/grouped, not on everything.
Version & frontend currency
No attrs= / states= in v17/18 views — direct invisible=/readonly=
Python expressions; <list> not <tree>; <chatter/> (→ odoo-views).
No name_get() — _compute_display_name; no removed ORM names
(fields_view_get→get_view, etc. — → version-matrix).
No unmarked public method that should be private (v18.2+) — public
model methods are RPC-callable by default; internal helpers want @api.private
(→ odoo-security). A privilege/exposure check, not style.
OWL reads props.record.data[props.name] / writes via .update(...);
correct import paths; template in the bundle (→ odoo-owl). Public JS uses the
right framework (publicWidget vs Interactions — → odoo-web).
Dependencies & hygiene
No hallucinated Python packages — every external_dependencies['python']
/ import is a real, maintained package at a pinned version (slopsquatting guard).
No leftover debug — breakpoint(), print(), import pdb, bare
except: pass swallowing errors.
The "looks fine, fails later" smells (quick scan)
Smell in the diff
Likely real defect
Confirm with
.sudo() with no comment
privilege bypass / hidden record rule
odoo-security dossier
for r in recs: ...search(/browse
N+1
odoo-perf, trace_flow SQL count
store=True + short @api.depends
stale stored value
model_brief depends
new model, no .csv in the diff
non-admin 403
access_rights in brief
Python field renamed, no migrations/
data loss on -u
odoo-migration
override of action_confirm/create
wrong hook / broken super()
MRO + trace_flow
attrs= / <tree> / name_get
hard parse error / no-op on 17/18
odoo-views, version-matrix
auth='public' reading user data
data exposure
odoo-web, record rules
new .py not in __init__
dead override, no error
odoo-module-scaffold
Output of a review
State, concisely: blocking issues (security, data loss, silent correctness)
vs non-blocking (perf/style); each with the file/line, why it's wrong in
Odoo terms, the fix, and — for correctness — the test that should prove it.
Don't rewrite the whole patch; point to the smallest correct change (the
odoo-dev ethos).
Run the checklist as a linter
Much of the checklist above is now executable: odoo-ai validate <path...>
(local, no DB) statically flags the high-signal, mechanical defects — attrs/
states, <tree>, name_get, type='json' on 19+, create() without
@api.model_create_multi, search()/browse() in loops, f-string cr.execute,
uncommented sudo(), self._cr/_uid/_context, fragile xpath, leftover debug.
Run it first to clear the mechanical issues, then spend your judgement on what a
linter can't see (MRO layer, security intent, data-loss, right hook) — confirmed
against the brief. It complements, never replaces, the confirm-against-the-instance
rule. Before sharing any introspection JSON with an external LLM, run
odoo-ai redact <file> / odoo-ai scan-secrets <file>.
References & related skills
This skill's references
references/review-checklist.md — the checklist expanded dimension by dimension
(security/sudo, data-loss/migration, silent-correctness/MRO+super+depends,
performance/N+1, multi-company, version currency), each line with the why in
Odoo terms and the exact odoo-ai command that confirms it.
references/ai-failure-modes.md — the specific confident-but-wrong patterns AI
ships (hallucinated names, memory-era syntax, sudo()-to-silence, batch-unsafe
create, N+1, wrong-hook override, bare rename, slopsquatting, default-company
rule), each with the odoo-ai command that catches it.
Other skills in the loop
odoo-testing — turn each correctness finding into a failing-then-passing
test; the PR checklist there is the merge gate this skill feeds.
odoo-introspect — confirm every suspicion (fields, MRO, security, depends)
against the running instance instead of asserting.
