From b75d93cfea11a6ecb4b0579c60ebd18cce73dcf6 Mon Sep 17 00:00:00 2001 From: Ako Date: Fri, 25 Sep 2026 12:49:07 +0000 Subject: [PATCH] docs(review): record the unbumped catalog-schema-version pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the review of mendixlabs/mxcli#1181. A column added to createTables without bumping CatalogSchemaVersion: the version guard only drops tables when the version CHANGES, and CREATE TABLE IF NOT EXISTS never adds a column, so every user with a cached catalog keeps a table the new SELECT cannot read. Measured: activities_for yielded 302 activities on main and 0 on the branch against the same cache file, with `no such column: UseRequestTimeout` and `mxcli lint` exiting 1. `mxcli report` runs the same rules through the same LintContext and never calls QueryErrors(), so there the same stale cache scores the project silently. The canonical fix carries the reproduction, because the obvious attempt does not reproduce it: the cache has to actually be REUSED. Building it with one `-p` spelling and running the new binary with another invalidates on "MPR path changed" and rebuilds, and a default lint run builds a fast catalog with zero activities — either way the run is green and the bug invisible. Confirm the output says "Loading cached catalog … (from cache)" first. Co-Authored-By: Claude Opus 5 (1M context) --- .claude/commands/mxcli-dev/review.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.claude/commands/mxcli-dev/review.md b/.claude/commands/mxcli-dev/review.md index 1c647739c..0d51e8944 100644 --- a/.claude/commands/mxcli-dev/review.md +++ b/.claude/commands/mxcli-dev/review.md @@ -61,6 +61,7 @@ proactively. Add a row after every review that surfaces something new. | 30 | Two commands compute the same thing from two copies of the setup (`report` re-implementing `lint`'s rule list and skipping its config), so they disagree about a project — and a SCORE carries no provenance, so neither number looks wrong | Code correctness | Extract the shared setup and route both through it. A value test cannot guard this when the copies live inside cobra `RunE` bodies: use a structural check on the source, with a positive control asserted FIRST so it cannot pass vacuously | | 31 | A test helper that needs a heavyweight object only to satisfy a signature (`NewLintContext(nil, nil)`, which panics) invites a nil-guard added purely to make the test compile — behaviour nothing in production needs, defended forever | Test coverage | Narrow the signature instead: if the helper does not use the parameter, drop it and let the caller apply the part it owns. A test that cannot construct an argument is usually telling you the argument does not belong | | 32 | A fix adds a diagnostic for a capability the model lacks while leaving in place the code that asserts the capability EXISTS — MDL042 telling the author a loop's `@caption` is dropped, while `cmd_microflows_builder_annotations.go` still ran `case *microflows.LoopedActivity: activity.Caption = ann.Caption` under the comment "LOOP / WHILE activities can carry a caption just like splits", and the describer still emitted one. Nothing read either back. The next reader trusts the code over the warning, deletes the check, and reopens the bug from the other side. The reason it survives is that it usually has TESTS — three here asserted the caption was carried, all of them against the semantic object and none against storage, so they passed throughout and failed only on the correct fix | Code correctness | `generated/metamodel` is the arbiter: a field on the semantic type it does not declare cannot survive a write, so an assignment to it is dead by construction. Grep the writer, the describer and the semantic struct and delete (or re-comment) whatever sets it. MEASURE before deleting — `exec` then `describe` on a real project, with the UNMODIFIED build, so the deletion rests on the stored document rather than on reading the codec. Invert the tests that defended it rather than deleting them, keeping any half still true (escaping coverage belongs on a type that can carry a caption), and check the inverted test fails when the assignment is put back | +| 33 | A column added to `createTables` without bumping `CatalogSchemaVersion` — the version guard only drops tables when it CHANGES, and `CREATE TABLE IF NOT EXISTS` never adds a column, so every user with a cached catalog keeps a table the new SELECT cannot read. Measured on #1181: `activities_for` yielded 302 on `main` and **0** on the branch against the same cache, `no such column: UseRequestTimeout`, `mxcli lint` exit 1. `mxcli report` runs the same rules and never checks `QueryErrors()`, so there it would score silently | Code correctness | Bump the constant in the same commit — its doc comment says so and `62913741` is the precedent (four columns + 11→12 + a builder test). To REPRODUCE, the cache must actually be reused: build it with the old binary and run the new one with the **same spelling of `-p`**, because a relative-vs-absolute path invalidates on "MPR path changed" and hides the bug; confirm the run says "Loading cached catalog … (from cache)" before believing a green result | ---