[odoo-code-reviewer] TDD conformance is scored as correctness - a defect specified in the design passes review, and odoo-test-writer inherits the same blind spot

[SonCrits]

Summary

The review chain has no lens that can catch a defect introduced by the design. odoo-code-reviewer is instructed to hold the TDD as authoritative, so faithful transcription of a wrong predicate scores as a pass - and the reviewer reports the match itself as its primary confidence signal. odoo-test-writer derives its cases from the same TDD, so the suite is green before and after, and nothing in the pipeline can see the gap.

Observed on odoo-ai-agents v4.25.0. A human caught it during PR review; four agents had already signed off.

Where the spec mandates it

agents/odoo-code-reviewer.md:34

Treat the main-agent instructions and any Technical Design Document (TDD) as authoritative for intent and acceptance criteria

agents/odoo-code-reviewer.md:73

Read the design doc (child TDD) and hold its §1 Intent / Purpose / Expected outcomes / Business value and §9 Acceptance Criteria (solution + per-module) as the contract the code must satisfy

skills/odoo-code-review/SKILL.md:99

when non-null, pass as DESIGN_DOC: to each per-module reviewer - MANDATORY TDD verify (§1 Intent + §9 ACs + ### TDD Conformance; skipping is a review defect)

Nothing anywhere tells the reviewer the TDD may itself be wrong, or that its authority stops at §1/§9.

What happened

odoo-solution-architect wrote a TDD whose §3 Data model contained a block titled "Enrichment predicate (the whole of the new method's logic, specified precisely)", spelling out a guard over a list of create-vals dicts. Shape, genericised:

for vals in vals_list:
    if vals.get('kind') != 'target' or vals.get('mode') != 'expected':
        continue
    enrich(vals)

Both keys are Selection fields whose model defaults are exactly the values being matched. So a vals dict that omits a key is created by Odoo as precisely the record the predicate wants to match (create() -> _add_missing_default_values, odoo/models.py), yet .get() returns None and the entry is skipped. The predicate does not model what create() will produce.

Then, in order:

1. odoo-coder transcribed §3 faithfully. Correct behaviour for a coder given an authoritative spec.

2. odoo-test-writer derived its cases from the same TDD. No case exercises the key-absent branch, so the branch has no coverage at all.

3. odoo-code-reviewer returned APPROVE, score 98, 0 CRITICAL/HIGH. Its finding fix: pass GH_TOKEN as Authorization header in validate workflow #1, verbatim:

   Guard conditions and order. All five guards match the design's predicate exactly

   Conformance was reported as the reason for confidence.

The suite was green before and after the human's fix, because no test could reach the branch.

Why this is structural, not a bad run

• §3 is an implementation sketch, not an acceptance criterion. The spec scopes TDD authority to §1 Intent and §9 ACs, but says nothing about §3-level detail - and in practice the reviewer validated against §3 and treated the match as evidence of correctness. A reviewer that reads the design cannot un-read it.
• The failure is silent by construction. Design-derived tests cannot cover a branch the design forgot exists. Green tests therefore confirm the flawed design.
• Every downstream agent inherits one upstream mistake. Coder, test-writer and reviewer all took the same document as ground truth. Three sign-offs, one independent opinion between them - zero.

Suggested direction

1. A blind lens. At least one reviewer pass that never receives DESIGN_DOC and must derive expected behaviour from the code plus the model definitions alone. Disagreement between the blind pass and the conformance pass is the signal worth having.
2. Conformance is neutral, never positive. Instruct odoo-code-reviewer that "matches the design" is not evidence of correctness and must not appear as a justification; each guard needs an independent derivation.
3. Say where TDD authority stops. Make explicit that §1/§9 are contract while §3-level implementation detail is a hypothesis the reviewer is expected to challenge.
4. Truth table for vals.get() predicates. Any guard over create-vals should require an explicit table across key-absent / '' / False / None / other, checked against the field's model default. This class of bug is mechanical to catch once someone is told to look.
5. odoo-test-writer: one case per guard boundary, key-absent included, rather than one case per TDD row.

Point 1 is the one that matters. Without an opinion formed independently of the design, the chain can only ever verify that everyone read the same document.