Home / Projects / Orchestrata Ultimate / Decisions / ADR index / ADR-0073

ADR-0073: ERR-110-002 amended: the default-on review engine aborts a commit on findings the change introduced (delta baseline)

← ADR-0072 · ADR-0074 →

The record

Status: Accepted (ruling, Mohamed, 2026-10-07: "accept adr-0073") · Date: 2026-10-06

Context

SPEC-110 FR-012 puts an InMemoryReviewEngine in front of every CodemodWorkspace.commit() (REQ-110-033/035: review runs on commit) and ERR-110-002 says what an error-severity finding does:

| ERR-110-002 | REQ-110-013 | Review pass produces error-severity finding | ERROR | Abort commit, return CommitReport with findings, write nothing to disk |

The spec names no legacy carve-out, so plan decision D6 (plans/spec-110-completion-plan.md §1) gave the engine baseline='absolute'|'delta' with 'absolute' as the default and wrote: "Flipping the default is a spec amendment and needs its own ADR; none is minted until a packet measures a legacy-file abort in practice."

Packet S3 (commits 24b856ccd, 81c5ea708, 32a56e4d9 on feat/spec-110-completion) built the engine and wired it on by default. The S3 reviewer measured the abort in practice (2026-10-06, probe probe_engine.py: load the default include of src, scripts and tests, pass every file as an identity change through InMemoryReviewEngine(include_phase3=False)):

Most of the 419 are false positives with named causes, all fixable without a ruling: the token-scanner path hands raw lines including string literals to the reused regexes (the mock-pattern tables in refactor/codemod/review.py, verification/review/checks_safety.py, agents/volks/codedirector/review_checks.py and agents/volks/qa/volk.py flag themselves); CO_THIRD_PARTY_DECLARED reads only [project] dependencies (so google from protobuf and pytest are "undeclared"); _is_test_path treats the production module core/threading/test_scheduler.py as a test file; a narrow --include makes CO_LOCAL_MODULE_EXISTS report same-package imports outside the include as missing. The S3 fix round 1 (commits ace6149cb, 32b00b303, 13f063277) removed those four causes (token-blanked lexical scans, every dependency declaration site plus packages_distributions(), the test-path rule, narrow-include scoping) and re-ran the same probe once on the same tree (3,234 files; load 35.7 s, review 88.1 s, 3,385 MB maxrss):

Of the 25 src/+scripts/ errors that remain, the true positives by the rules as written are gateway/config.py:59 (_DEV_JWT_SECRET = "dev-secret") and the two deliberate exec() module loaders (scripts/serialize_codebase_ast.py:6957, scripts/_janus_patched/janus/janus.py:411); the rest are two further false-positive causes left for the next round so the measurement stays single: the sk- alternative of the key regex in verification/review's check_security has no word boundary (it matches task-management-… and ask-openrouter in docstrings), and CO_THIRD_PARTY_DECLARED does not yet treat sibling-script stems under scripts/ as local the way check-deps does (about 11 to 13 errors). The 222 tests/ errors are literal test credentials and flushdb/DELETE calls in shared fixtures: true positives by the rules as shipped, and exactly the legacy debt an 'absolute' default turns into a veto on every codemod that touches those files. The structural question therefore stands at either count: with 'absolute' the engine refuses to apply ANY codemod to a file that already carries one error-severity finding, including the codemod that would remove it.

A separate conformance gap the same review found, phase 2 not reading a CIG built from the mutated forest (REQ-110-035), is not part of this decision; it is carried in the plan's S3 row.

Decision (proposed)

Amend ERR-110-002 for the engine commit() runs by default: an error-severity finding aborts the commit when the change INTRODUCED it, measured as the delta between the review of the pre-mutation forest and the review of the mutated forest, matched on (path, rule, message) with line numbers stripped from the message. InMemoryReviewEngine keeps baseline='absolute'|'delta'; the default becomes 'delta'; 'absolute' stays available for clean-slate gates and scripts/codemod.py exposes it as --baseline absolute. Plan D6 is superseded by this record on acceptance. Pre-existing error findings are still reported (as findings on the CommitReport), they no longer veto the write.

Alternatives rejected

• Keep 'absolute' as the default (D6 as written). Faithful to the letter of ERR-110-002, but a codemod tool that cannot touch 134 files of its own repository, and in the general case cannot touch any file with one legacy finding, blocks exactly the refactors that remove the debt. The spec's intent for the review is "a change must not make the code worse", which the delta baseline states directly.
• Default run_review_engine=False (engine opt-in). Removes the abort by removing the review; contradicts REQ-110-033/035 (review runs on commit) and loses the warnings the engine does produce correctly.
• Downgrade every finding on a pre-existing line to a warning. The same effect as delta with a worse model: it needs the same before/after pairing to know which lines are pre-existing, then throws the severity away.
• Per-rule allow-lists in pyproject.toml. Configuration that grows with the debt and has to be maintained by hand; it does not address a codemod that introduces no new finding being blocked by an old one.

Consequences

• InMemoryReviewEngine(baseline=...) defaults to 'delta'; the engine runs its checks a second time per commit, over the ORIGINAL trees of the changed modules only (review() builds mutated and original from changes.entries; the untouched forest is context, not input). Cost is therefore about twice the review of the changed files, not of the tree; the reviewer's 84 s / 65 MB figure is the worst case of every one of the 3,234 files changed at once.
• The S3 reviewer's finding 2 was a precondition: _delta matched on messages that embed L<n>: / (line n), so a one-line insertion above legacy code reported every legacy finding as introduced. Fix round 1 (ace6149cb) matches on a line-free message and tests the prepend case (delta 0, absolute 1) and the genuinely-introduced case (delta exactly 1).
• ERR-110-002's row in specs/110-codemod-engine/spec.md gains the sentence "for the default engine, 'error-severity finding' means one the change introduced (delta baseline, ADR-0073); baseline='absolute' restores the unconditional abort"; the checklist row for REQ-110-034/035 cites this ADR; TST-110-021 gains a case where a legacy error is present and an unrelated edit still writes.
• docs/codemod-review-engine.md documents the default and the escape.

Provenance

• Plan D6 (plans/spec-110-completion-plan.md, 2026-10-06) set the trigger: "none is minted until a packet measures a legacy-file abort in practice".
• S3 reviewer report, 2026-10-06, finding 1 (BLOCKER) with the numbers in the table above; finding 2 (delta line-shift) as the precondition.
• Recommendation is the orchestrator's; the ruling is Mohamed's.
• Ruled Accepted by Mohamed on 2026-10-07, verbatim: "accept adr-0073" (after the three-line summary and the "what do we lose if I accept" exchange; the cost consequence was corrected in bbc794bec before the ruling). Execution: the S3 packet's next round flips InMemoryReviewEngine's default to 'delta', amends the ERR-110-002 row, adds the TST-110-021 legacy-error case and the CLI --baseline absolute escape, and supersedes plan D6.
• Executed 2026-10-07 in 7db0f25e1 (constructor default 'delta', CLI --baseline {delta,absolute}, ERR-110-002 row amended, plan D6 superseded) and ac8188445 (fix round 6). Two corrections to the execution sentence above: the checklist rows amended were REQ-110-033/034/035 (the TST rows are cited, not amended), and the legacy-error-does-not-abort case lives in TST-110-026 (tests/refactor/codemod/test_review_engine_commit_wiring.py) and tests/scripts/test_codemod_cli.py, not in TST-110-021.
• Refinement (round 6, ac8188445), recorded here because the Accepted body is immutable: the delta key is (path, rule, line-free message, stripped line text) with multiplicity, not path/rule/line-free message. An in-place edit of a line that already carried a content-free finding (placeholder to real secret) is therefore introduced and aborts, and duplicating a legacy line is caught; the one masking shape left is a legacy line removed and the same text re-added elsewhere in the same file (a moved line), and an edit on a continuation line of a multi-line statement stays legacy because the key reads the finding's own line; both are stated in docs/codemod-review-engine.md. The spec's events table records the legacy flag and legacy_count (c6946173a). Legacy findings are reported, not dropped: InMemoryReviewEngine.review_detailed() returns ReviewOutcome(introduced, legacy), CommitReport.legacy_findings carries them, FINDING_EMITTED marks them legacy: True and REVIEW_COMPLETED carries legacy_count; review() and the commit veto set are introduced findings only.
• Measured on the whole tree at 7397f2c35 (round-7 review): absolute 235 errors in 93 files, 12,786 warnings; delta 0 introduced.

Related ADRs

No other ADR is named in this record.

Rulings cited

None.

Specs cited

• SPEC-110