Refactor audit categorization

[kaligrafy]

Proposition from Claude:

And maybe remove the second letter in the audit code

[kaligrafy]

Design proposal: a stable, declarative audit check system (By Claude Opus 5)

Context: follow-up to #1733. @greenscientist asked
(a) why M/I/L all mean "error" instead of just E, and (b) whether NEED_REVIEW should be its own
type distinct from warning; and noted that the current system is being patched rather than designed.
This document tries to answer both questions from the same root cause.

---

1. Root cause: the identifier carries mutable classification

errorCode currently does three incompatible jobs at once:

1. Identity — it is part of the primary key of sv_audits
   (interview_id, error_code, object_type, object_uuid), so it is what carries the reviewer's
   ignore flag across re-audits, and what makes 2023/2025/2026 waves comparable.
2. Classification — the middle letter encodes the kind of problem, and (since refactor(audits): use W prefix for warning-level audit error codes #1733) implicitly the
   severity.
3. i18n key — t('audits:' + errorCode).

Job 1 requires the string to be immutable forever. Jobs 2 and 3 make it change every time our
understanding of the check changes. That contradiction is the actual bug, and it is why:

• refactor(audits): use W prefix for warning-level audit error codes #1733 exists at all (two checks emitted level: 'warning' under an _L_/_I_ code — the same fact
  written twice by hand, so it drifted);
• CodeRabbit flagged the same back-compat problem on three consecutive review passes (renaming orphans
  the persisted ignore flags and the locale keys);
• @kaligrafy's objection to E is correct on its own terms — renaming breaks cross-wave comparability —
  but that objection only has force because identity and classification are the same string.

@greenscientist's counter-proposal (a two-letter code [WRI][MIL]) is a genuine improvement in
expressiveness, but it does not fix this: it puts more mutable metadata into the immutable key, so
the next reclassification breaks the primary key again, twice as often.

Proposal: make errorCode a permanent, meaningless-by-design identifier, and move every classification
axis into typed, declared metadata. Once you do that, question (a) dissolves and question (b) has a
clean answer.

---

2. The classification axes are genuinely orthogonal

The current single letter is trying to express at least three independent things. They are independent in
the strong sense: all combinations occur in the existing check set.

Axis	Question it answers	Values
category	What kind of defect is it? (diagnostic)	missing, invalid, inconsistent, implausible, informational
severity	How much does it damage the usability of the record? (data quality)	blocking, degrading, none
disposition	What is a reviewer expected to do? (workflow)	mustCorrect, mustReview, mayReview, noAction

M/I/L are three values of category. W/F are two values of severity. That is exactly why the
list "M, I, L, W, F" felt incoherent — it is two enums concatenated. This is the direct answer to
"why M-L-I for errors and not just E?": E is the severity, M/I/L are the category, and both
are needed because they are used for different purposes (severity drives the reviewer's queue ordering;
category drives which correction tool applies and which quality indicator the finding feeds).

2.1 disposition answers the NEED_REVIEW question

@greenscientist is right that "needs review" is not the same thing as "warning", and the axis table shows
why: severity is a property of the data, disposition is a property of the workflow. They dissociate in
all four quadrants:

	mustCorrect / mustReview	mayReview / noAction
blocking	Home geography missing → record unusable, human must fix.	Home geography malformed GeoJSON → unusable, but auto-repairable; no human needed.
degrading	Trip duration implausible → record still usable, but only a human can adjudicate.	Car count slightly high per licence → logged, investigate in aggregate, never per-interview.

So: yes, add the concept — but as disposition, not as a sixth letter. A letter cannot express two
independent facts.

2.2 A fourth, purely operational axis

cost: 'cheap' | 'expensive' replaces the current standardAuditChecks / extendedAuditChecks map
duplication (all extended maps are currently {}). One registry, one filter at run time, instead of two
parallel object literals per survey object.

---

3. Concrete shape

3.1 New types (evolution-common/src/services/audits/types.ts)

/** What kind of defect the check detects. Diagnostic axis. */
export type AuditCategory =
    | 'missing'        // a datum that should be present is absent
    | 'invalid'        // present, but malformed or outside its domain
    | 'inconsistent'   // two or more data contradict each other
    | 'implausible'    // internally consistent but statistically unlikely
    | 'informational'; // not a defect; a fact about the interview

/** How much the finding degrades usability of the record. Data-quality axis. */
export type AuditSeverity =
    | 'blocking'   // the object (or its subtree) is unusable for analysis until resolved
    | 'degrading'  // usable, but the affected variable is unreliable
    | 'none';

/** What the reviewer is expected to do. Workflow axis. */
export type AuditDisposition =
    | 'mustCorrect'  // a human must edit the data
    | 'mustReview'   // a human must look and decide (correct, or ignore with a reason)
    | 'mayReview'    // queued, no obligation
    | 'noAction';    // never surfaced per-interview; aggregate only

For UI back-compat, keep level on AuditForObject but make it derived by the runner
(blocking → 'error', degrading → 'warning', none → 'info') and mark it @deprecated. No admin-UI
change is required on day 1.

3.2 The check definition replaces the bare function

export type AuditCheckOutcome<TParams extends Record<string, unknown> = Record<string, unknown>> =
    | undefined                 // pass
    | 'notApplicable'           // could not be evaluated (see §3.3)
    | {
          params?: TParams;               // evidence + i18n interpolation values
          severity?: AuditSeverity;       // must be one of `severities`
          disposition?: AuditDisposition; // must be one of `dispositions`
      };

export type AuditCheckDefinition<TContext, TParams extends Record<string, unknown> = Record<string, unknown>> = {
    /** STABLE identifier. Never renamed, never reused, never parsed for meaning. */
    code: string;
    /** Codes this check has previously been published under. Drives ignore-flag and locale fallback. */
    previousCodes?: readonly string[];
    objectType: SurveyObjectName;
    /** Bump when the *semantics* change (thresholds, predicate). Resets reviewers' `ignore`. */
    version: number;

    category: AuditCategory;
    /** First entry is the default; `run` may return any listed value. */
    severities: readonly [AuditSeverity, ...AuditSeverity[]];
    dispositions: readonly [AuditDisposition, ...AuditDisposition[]];
    cost: 'cheap' | 'expensive';
    groups: readonly AuditChecksGroup[];   // reuses the existing AuditChecksGroup type

    /** Config keys the check needs; if absent, the runner emits `notApplicable`, never a failure. */
    requiredConfig?: readonly string[];
    /** Declared interpolation parameters; CI checks them against en/fr locale placeholders. */
    paramNames?: readonly string[];
    /** One line, English. Feeds the generated inventory — no hand-maintained list. */
    description: string;

    run: (context: TContext) => AuditCheckOutcome<TParams> | Promise<AuditCheckOutcome<TParams>>;
};

The key structural point: run can no longer construct errorCode, level, objectType,
objectUuid or version. The runner assembles the AuditForObject from the definition plus the returned
payload. The class of bug that #1733 is fixing becomes unrepresentable, rather than being caught by
convention and code review. This is the single highest-value change in this document and it is also the
cheapest.

3.3 Three-state outcome, not two

Today a check that cannot run (survey area not configured, field not collected by this survey) returns
undefined — indistinguishable from an actual pass. That makes every quality indicator uninterpretable,
because the denominator is unknown: "0 geography errors" may mean the geography is clean or that
territorial validation was never configured.

'notApplicable' fixes the denominator. It should be recorded in the audit run metadata, not as a row in
sv_audits.

3.4 Naming, once the metadata moves out

Recommended: <OBJECT_PREFIX>_<DescriptiveName>, e.g. HH_CarNumberPerLicenceTooHigh,
HM_PreGeographyAndHomeGeographyTooFarApart.

Keep the object prefix — it is cheap, it is stable (a check's object type essentially never changes), and
it makes codes greppable and sortable. Drop the category/severity letter, which is precisely the part that
changes.

Honest trade-off: the letter is genuinely useful when someone reads a flat CSV export with no join.
Neutralise this by emitting category, severity and disposition as columns in the audit export —
which is better practice anyway than parsing semantics out of a string with a regex (see the
[MILFW] vs [MILWN] grep confusion in #1733).

---

4. The registry, and invariants enforced in CI

Replace { [errorCode: string]: AuditCheckFunction } with an array of definitions per object type, and
add one conformance test that iterates the whole registry and asserts:

1. code is unique across the registry, and matches ^[A-Z]{1,2}_[A-Za-z0-9]+$.
2. No code collides with any previousCodes entry anywhere (no aliasing cycles or ambiguity).
3. objectType matches the map the definition is registered in.
4. severities[0] / dispositions[0] are consistent with category per the default table in §5.
5. Locale keys exist in locales/en/audits.json and locales/fr/audits.json, for code and every
   previousCodes entry (this is the current AuditLocales.test.ts, extended).
6. The interpolation placeholders in both locale strings are exactly paramNames — same set, both
   languages. (Directly supports #1741.)
7. A unit test file exists for every code (glob on __tests__/<object>/<code>.test.ts).
8. description is non-empty.

Then generate, don't maintain: yarn audits:docs writes the inventory table (code, object, category,
severity, disposition, groups, description) into the README from the registry. This eliminates the
hand-maintained list in §7 of the guide and the whole class of review comment tahini raised on #1733
("cette liste n'a pas été enlevée dans un autre commit? Il faudrait rebaser probablement").

---

5. Default matrix (so authors are not asked to make three free choices)

Three enums × ~40 checks is a lot of judgment calls, and judgment calls get made inconsistently. Provide
defaults keyed on category; authors only override deliberately, and the override is visible in review.

category	default severity	default disposition	Typical override
missing	blocking	mustCorrect	degrading when the field is optional per fieldIsRequired
invalid	blocking	mustCorrect	mayReview when auto-repairable
inconsistent	blocking	mustReview	—
implausible	degrading	mustReview	mayReview for low-precision heuristics (§7)
informational	none	noAction	—

Note that missing needing a dynamic severity is a real requirement, not a hypothetical: HH_M_Size
should be blocking where the field is required by the project config and merely informational where it is
not. That is why severities is a set and run may return one of them.

---

6. Migration path

The renames are unavoidable if the letter is dropped, so make renaming a first-class, supported
operation rather than a one-off breakage:

1. previousCodes on the definition (not a global alias file) — colocated with the check, so it cannot
   drift, and CI invariant evolution: URLs with // in it are not equivalent to / #2 above catches collisions.
2. Derive auditCodeAliases from the registry at load time.
3. mergeWithExisting resolves any persisted error_code through the alias map before matching, so
   reviewers' ignore flags survive the rename. This is exactly what CodeRabbit asked for three times.
4. Locale lookup falls back through previousCodes, so historical exports still render.
5. One DB migration rewrites error_code in sv_audits through the alias map, plus new columns
   category, severity, disposition, params jsonb. level retained, backfilled, deprecated.
6. Publish a code ↔ previousCodes crosswalk table with each wave's data release. Cross-wave
   comparability then becomes a documented, versioned artefact rather than a property we are afraid to
   disturb — which is what it should have been in the first place.

---

7. Beyond taxonomy: the things that actually make it robust

The naming is the visible problem; these are the ones that will bite during the next survey wave.

7.1 Per-check isolation. Today a single throw kills the entire audit pass for that interview (stated
in the guide, §9). Wrap each run in try/catch in the runner; a throw becomes a synthetic
SYS_CheckFailure audit carrying the failing code in params, plus a structured log. ~20 lines, removes
a whole failure mode, and makes "guard anything that can throw" a runner guarantee instead of a
documentation plea.

7.2 Structured evidence instead of baked messages. run returns params (the actual numbers), never
an English sentence. The row then says why it fired: { carNumber: 4, licences: 1, threshold: 2 }.
Three benefits: real i18n (#1741), self-explanatory audit rows years later, and analysable distributions
of the evidence itself.

7.3 Threshold provenance and determinism. Any implausible check has a threshold. Record it in
params and bump version when it changes. Stronger requirement: a check must be a deterministic
function of (context, projectConfig, statisticsSnapshot). If a threshold is ever derived from the
wave's own distribution, that distribution must be pinned per audit run — otherwise re-running audits
silently changes historical results, and the audit set stops being reproducible. This matters more for us
than for a typical web app, because these outputs end up in published methodology.

7.4 Golden fixture corpus. A small set of anonymised (or synthetic) interview fixtures with a
snapshotted expected audit set. Unit tests per check are necessary but they only test checks in isolation;
the fixture corpus catches interaction effects and silent severity drift across releases. This is what
converts "we have tests" into "the audit suite is validated against known data".

7.5 Empirical calibration — the part I would fight for. An audit check is a binary classifier, and we
currently never measure its precision. Instrument it: for each code and wave, record the reviewer
outcome (ignored vs. data actually edited). Then:

• a mustReview check that reviewers ignore >90% of the time is noise and should be demoted to
  mayReview or retired;
• a mayReview check that is corrected >50% of the time should be promoted;
• severity and disposition become measured, revisable quantities with an audit trail rather than
  opinions defended in PR threads.

This is also the principled answer to the warning vs. NEED_REVIEW question: the boundary is a
threshold on measured precision, chosen once and applied uniformly, not a judgement re-litigated per
check. Publish the table per wave; it is a genuinely publishable methodological contribution.

Caveat: this needs a way to attribute an edit to an audit. ignore is already stored; linking a
correction to the audit that prompted it may need a small addition to the review UI or a diff-based
heuristic. I have not read the review UI code, so I do not know how much work this is.

---

8. What to do with #1733 right now

Two defensible options:

• (A) Merge as-is. Then the two renamed checks get renamed twice (once now, once at the redesign),
  breaking ignore flags twice — unless previousCodes chains, which it can.
• (B) Recommended: split it. Keep the documentation fixes and the level-consistency fix; drop the two
  renames. Do the rename once, inside the redesign, with alias support in place. The renames are only two
  codes, and CodeRabbit has flagged the back-compat gap on three separate passes, which is a reasonable
  signal that shipping the rename without the alias mechanism is premature.

Either way: open the high-priority design issue @greenscientist asked for, with this as the starting text.

Suggested sequencing

Phase	Content	Rough size
0	Design issue + agreement on the three axes and on code immutability	discussion
1	Types, AuditCheckDefinition, runner assembles AuditForObject, per-check try/catch, notApplicable	small–medium, mechanical
2	Migrate existing checks to definitions (no renames yet), registry conformance test, generated inventory	medium, purely mechanical, parallelisable
3	DB migration (new columns + alias rewrite), previousCodes, mergeWithExisting alias resolution, then the renames	medium
4	Structured params + i18n interpolation (#1741), fixture corpus	medium
5	Calibration instrumentation and the first per-wave precision table	research-side

Phases 1–2 are worth doing even if the naming debate never converges: they remove the drift that caused
#1733 regardless of which letters we settle on.

---

9. Confidence and what I have not verified

Stated explicitly so nobody over-trusts this:

• High confidence: the identity/classification conflation is the root cause; the three axes are
  orthogonal; making run unable to emit its own errorCode/level eliminates the refactor(audits): use W prefix for warning-level audit error codes #1733 bug class;
  generated inventory beats a hand-maintained list.
• Medium confidence: the specific enum values. implausible vs. inconsistent will have grey zones,
  and mayReview vs. noAction may collapse into one value in practice. Expect one revision after the
  first wave of real use.
• Medium-low confidence: effort estimates. I read the two READMEs, types.ts, and the refactor(audits): use W prefix for warning-level audit error codes #1733 thread.
  I have not read AuditCheckRunners.ts, SurveyObjectAuditor.ts, AuditService.ts,
  audits.db.queries.ts (in particular mergeWithExisting), or the admin review UI. Anything I say about
  what is "cheap" is an inference from documentation, not from the code.
• Known unquantified: how much the admin UI depends on level specifically, and whether external
  consumers (ARTM deliverables, past-wave analysis scripts) read error_code strings and parse the
  middle letter. If they do, §6.6's crosswalk becomes a hard requirement rather than a nicety.
• Risk I would flag against my own proposal: three enums plus groups plus cost is five metadata
  fields per check. Every additional required field is a field that will be filled in carelessly. The
  default matrix in §5 and the CI conformance test in §4 are what keep this from degenerating; if the
  team is not willing to maintain those, the honest fallback is @greenscientist's two-letter scheme plus a
  strict alias mechanism, which is worse but much smaller.