Batch: fix triage:valid issues #457, #438, #428 - #461
Merged
Conversation
…them ci.yml has pinned clang 22 since 10ecd06 (2026-07-19), but seven statements across the tree went on asserting 20. The stale fact was load-bearing rather than cosmetic: it was the single largest stated obstacle to moving the mutation campaign into CI (morph#408), and docs/spec/testing_charter.md is a spec, so a reader resolving the question against the authoritative document was told the wrong thing. Corrected: - docs/spec/testing_charter.md's "Mutation testing is local-only" bullet. The claim stands, but on cost and tooling -- Mull is on no runner image, its IR frontend needs hand version-matching, a campaign is 46+ minutes -- rather than on a compiler mismatch that does not exist. The 64.06% figure was in fact measured on CI's own clang major. - scripts/mutation.sh's version-constraint comment, whose instruction to "install the -20 package and re-measure" was work that does not need doing. - both `compiler` rows in scripts/mutation_survivors.json, and the not_yet_done entry that led with confirming the Mull defect on clang 20 (an abandoned attempt that was chasing a compiler CI does not use). The one still-live fact inside it -- that 0.34.0 is Mull's latest release -- moves up into verification_status. - three more the acceptance criteria sweep in, same defect, same date: codecov.yml's provisional-target note, cmake/compiler_options.cmake's -fcoverage-mcdc note ("unverified on clang 20" -- it is available on the pin), and scripts/check_branch_coverage.py's margin rationale. Nothing reported any of this for over a month, and nothing would have: a sentence about a compiler version compiles, tests and lints exactly as well after the version changes as before. So the correction ships with a gate, scripts/check_ci_clang_pin.sh, following the scripts/check_*.sh + scripts/test_check_*.sh + drift-guard.yml convention already in the tree. It reads CLANG_VERSION out of ci.yml and enforces two rules. Rule A: every `CI pins clang <N>` must name that version. Rule B: every *other* line naming a clang major beside a CI reference is rejected as an unrecognised phrasing, whatever version it names -- rule A alone would be defeated by rewording, and rewording is exactly how six sites came to say the same wrong thing six different ways. Finding zero assertions is a failure, not a pass. scripts/test_check_ci_clang_pin.sh drives ten directions against throwaway copies of the tree, one mutation at a time, each asserted to fail for its stated reason: the wrong version stated in prose, the pin bumped to 23 under prose that stays put (the direction that will recur), a stale claim in a script comment, both rewording shapes, every assertion deleted, ci.yml with no CLANG_VERSION, and three false positives the gate must not manufacture. Verified as a negative control by blinding rule A's mismatch branch, which turns three of those ten red. Fixes #457 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C6uB9zxdSG8qp3VNAAyFvF
The pinned Lightweight resolves unixODBC with pkg_check_modules and then
propagates the result as a PUBLIC *compile option*:
target_compile_options(Lightweight PUBLIC ${ODBC_CFLAGS})
so the include path reaches every consumer as a plain `-I` rather than through
INTERFACE_INCLUDE_DIRECTORIES. A plain `-I` header is coverage-instrumented; an
`-isystem` one is not. On macOS with Homebrew's unixodbc, odbc.pc's includedir
is the versioned Cellar path, which is not on Xcode clang's implicit system
search list, so pkg-config emits a real `-I` -- and sql.h, sqlext.h and
sqlucode.h land in the unfiltered llvm-cov mapping of every coverage-
instrumented target that links Lightweight::Lightweight PUBLIC. scripts/
check_coverage_roots.sh then fails, correctly: three files resolve outside the
checkout. The gate is right; the build is wrong.
morph's three existing SYSTEM demotions all read INTERFACE_INCLUDE_DIRECTORIES
and cannot reach a compile option, and ladder_<rung>_lib -- the target that
compiles the instrumented model code -- has no demotion at all. So this fixes
it once, on the dependency, rather than four more times on the consumers:
cmake/morph_demote_interface_includes.cmake moves every `-I<dir>` out of a
target's INTERFACE_COMPILE_OPTIONS and re-adds it via
target_include_directories(SYSTEM INTERFACE). Joined `-I/path` and separated
`-I;/path` are both handled, every other option survives in order, and the
function is idempotent -- both FetchContent_MakeAvailable(Lightweight) sites
(examples/common and examples/bank) call it, because whichever configures first
is the one that defines the target.
The gate this could not be is the obvious one. On Linux `pkg-config --cflags
odbc` is empty -- measured on this host: modversion 2.3.14, includedir
/usr/include, cflags empty -- so Lightweight's INTERFACE_COMPILE_OPTIONS holds
no `-I` and "assert no -I remains" would pass identically with the function
body deleted. tests/compile_checks/demote_interface_includes_selftest.cmake
hands the helper a synthetic option list instead, covering the joined form, the
separated form, a relative directory, a plain option that must survive in
order, and `-include forced.hpp` (which merely looks like `-I` to a
case-insensitive or prefix-only match, and whose argument must not be eaten),
then asserts both resulting properties exactly. It also pins idempotence, the
no-`-I` case, and the all-`-I` case (INTERFACE_COMPILE_OPTIONS must end up
unset, not holding one empty string). Configure-time FATAL_ERROR, so it runs
wherever tests are configured rather than only where the symptom is.
Verified as a negative control by emptying the function body, which turns the
configure red on the residual-options assertion; restored, the configure
completes. scripts/test_check_coverage_roots.sh still passes all seven
directions unchanged -- the gate itself is untouched.
Not verified: the macOS symptom itself. There is no Mac here, so acceptance
items 5-7 of the issue (an unfiltered export naming zero /opt/homebrew paths, a
green coverage.sh on the affected machine, and the before/after of a real
`-I` -> `-isystem` on a ladder_<rung>_lib command line) remain for whoever has
the hardware. This change proves the transform; those prove it is handed the
real thing.
Fixes #438
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C6uB9zxdSG8qp3VNAAyFvF
`UndoTransaction { ledgerId, journalId }` reverses one journal entry, and
`JournalId` appeared in exactly one wire DTO field in the whole rung -- that
action's own *input*. No result type carried one: `StoreTransaction` and
`UndoTransaction` both answer `GetLedgerResult`, `GetLedger` the same,
`ImportLedgerChunk` counts, and there was no `GetJournal` and no listing. So
the rung shipped a reversal a client could only drive with a guessed number,
and shipped a desktop Undo button (gui/qml/LedgerView.qml) whose sole input
was a journal id no screen ever displayed. The in-process tests reached around
it through a DataMapper, which is exactly what a socket client and the QML
bridge do not have.
Closed the way morph#361/#384 closed the structurally identical hole one level
up -- by adding the action, not by writing the gap down. Recording undo as
"administrative" was not available: `RunReportJob` is genuinely the report
runner's because the code enforces it, while `UndoTransaction` has no
principal check beyond the ordinary owner gate, so declaring it administrative
in prose would be half a scheme the rung cannot keep -- and it would mean
deleting shipped GUI surface.
`ListTransactions { ledgerId, month }` -> `{ entries: [{ id, description,
date, legs }] }`, month-bounded in the exact "YYYY-MM" shape
`GetBudgetReport` uses so the answer cannot grow without bound, oldest first
by the journal's own row id (a client-supplied date is not required to be
monotonic). Registered `Loggable::No` and keyed on `ledgerId` like `GetLedger`,
and gated through db::requireOwnedBook -- a listing of a book's entries is
precisely the read morph#382's gate exists for.
`monthRangeMs` and `detail::isValidYearMonth` move to
ledger/core/time_util.hpp, so `GetBudgetReport` and `ListTransactions` parse
the month through one definition rather than two.
GUI: the presenter gains `listTransactions`/`transactionsListed`, the QML
bridge an `entries` property and a `listTransactions` invokable, and
LedgerView.qml's hand-typed "Journal id to undo" TextField becomes a month
field, a list of that month's entries, and a per-row Undo button that hands
back the id it is showing. A successful undo re-lists the month, since the
reversal makes the list stale.
Retired the three records of the old contract:
undo-needs-a-journal-id-nothing-hands-out.scenario (whose whole premise was
`@body !~ "journalId"`) is replaced by store-list-and-undo-an-entry.scenario;
coverage_allowlist.json's `ledger/UndoTransaction` exemption is deleted, its
stated grounds being false now; and
open-account-transact-report-close.scenario's explanatory comment no longer
contradicts the surface. Both pins move deliberately:
test_morph_scenario.py's ledger name set gains `ListTransactions` and the
corpus total goes 72 -> 73. The workflow floor stays at 16 -- the new file
replaces the retired one rather than being added beside it.
Verified, with the negative control each claim needs:
* 157 ledger cases / 766 assertions pass, and `ctest` over the whole
configured tree is 1716/1716. The new suite is 7 cases / 32 assertions,
compiled under -Weverything -Werror like every other test TU.
* Deleting the `WHERE ledger` clause turns the cross-book case red
("REQUIRE( listed.entries.size() == 1 ) with expansion: 2 == 1"); stubbing
the action to `return ListTransactionsResult{}` turns 6 of the 7 cases
red. Neither is a test that would pass against a stub.
* The scenario passes twice against one database and one real
ladder_ledger_server -- 22 steps, 66 assertions, no failures in either
pass -- and mutate_scenario.py reports 59 mutants, 59 caught, 0 survived.
Against the stubbed model it goes red at exactly the step morph#428 names:
"FAIL step 11: expect ok capture journal=$.entries[0].id / cannot capture
journal: $.entries[0].id is absent / reply: ok body={"entries":[]}".
* scenario_coverage.py exits 0 with ledger at 19/19 actions, 16/16
workflows, 73/73 total and `ledger/UndoTransaction` gone from the
allowlist; test_morph_scenario.py runs 132 tests OK.
Not verified, and stated rather than glossed: the rest of the ledger scenario
corpus does not run today. Every one of its fifteen sibling files sends enums
as integers (`kind=0`, `currency=1`), and morph#444 gave every ledger enum a
`glz::meta`/`glz::enumerate` on 2026-09-04, so a real server now refuses those
payloads with a glaze `expected_quote` parse error. That breakage predates
this change and is unrelated to it -- filed as morph#460 -- so
`run_scenarios.py --rung ledger --twice` cannot pass as a whole, and the new
file was exercised on its own against a server the same way the driver starts
one. It is written against the wire as it actually is (`kind="Asset"`,
`currency="EUR"`), which is why it passes where its siblings do not.
Fixes #428
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C6uB9zxdSG8qp3VNAAyFvF
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
This was referenced Sep 6, 2026
`clang-format` in CI checks every tracked .hpp/.cpp, not just the ones a
pull request changes, and the new `ListTransactions` registration was the
only violation in the tree:
examples/ledger/include/ledger/models/ledger_model.hpp:488:90:
error: code should be clang-formatted [-Wclang-format-violations]
The call fits the column limit on one line, so the wrap before
`Loggable::No` was the deviation -- every other `BRIDGE_REGISTER_ACTION`
in this file is already single-line. Re-ran CI's own form of the check
(`git ls-files -z '*.hpp' '*.cpp' | xargs -0 clang-format --dry-run
-Werror`, clang-format 22.1.8, the major ci.yml pins) over all 779
tracked files: clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C6uB9zxdSG8qp3VNAAyFvF
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Batch of
triage: validissue fixes, one branch, one commit per issue.1. #457 — three (in fact seven) places assert CI pins clang 20;
ci.ymlhas pinned clang 22 since 2026-07-19The defect.
.github/workflows/ci.yml:59setsCLANG_VERSION: "22", set by10ecd064(2026-07-19). Seven statements across the tree went on asserting 20. Not cosmetic: it was the single largest stated obstacle to moving the mutation campaign into CI (#408), anddocs/spec/testing_charter.mdis a spec, so a reader resolving the question against the authoritative document was told the wrong thing. The 64.06% mutation figure was in fact measured on CI's own compiler.The fix. Corrected all seven — the charter's "Mutation testing is local-only" bullet (the claim stands, but on cost and tooling: Mull is on no runner image, its IR frontend needs hand version-matching, a campaign is 46+ minutes),
scripts/mutation.sh's "install the -20 package and re-measure" instruction (work that does not need doing), bothcompilerrows and thenot_yet_doneentry inscripts/mutation_survivors.json, plus the three the acceptance criteria sweep in and the issue body did not list:codecov.yml:109,cmake/compiler_options.cmake:677-679andscripts/check_branch_coverage.py:46,53.git grep "clang 20"now returns nothing.The gate. Correcting a sentence is not a fix — nothing reported this for over a month and nothing would have, because a sentence about a compiler version compiles, tests and lints exactly as well after the version changes.
scripts/check_ci_clang_pin.shreadsCLANG_VERSIONout ofci.ymland enforces two rules:CI pins clang <N>must name that version — the drift catcher;Zero assertions found is a failure, not a pass. Wired into
drift-guard.yml'sprose-lintjob besidecheck_spec_citations.sh, following the existingcheck_*.sh+test_check_*.sh+ workflow convention exactly.Evidence.
Green on the tree:
Red on the fixture the issue asks for — one line appended to
docs/spec/testing_charter.mdasserting the wrong version:…and green again with the line removed (
EXIT=0).The self-test drives ten directions against throwaway git copies of the tree, one mutation at a time, each asserted to fail for its stated reason:
And the self-test's own negative control — rule A's mismatch branch replaced with a
note, i.e. the gate blinded:bash scripts/check_spec_citations.shandbash scripts/test_check_spec_citations.shboth still pass unchanged.Not done, and why: whether the mutation campaign should run in CI at all (#408, blocked on a cost decision); re-running the campaign; bumping the pin. All three are the issue's own out-of-scope list.
2. #438 — Lightweight propagates unixODBC's
-Ias a PUBLIC compile optionThe defect. The pinned Lightweight does
target_compile_options(Lightweight PUBLIC ${ODBC_CFLAGS}), so the ODBC include path reaches every consumer as a plain-Irather than throughINTERFACE_INCLUDE_DIRECTORIES. A plain-Iheader is coverage-instrumented; an-isystemone is not. On macOS with Homebrew'sunixodbc,odbc.pc'sincludediris the versioned Cellar path, which is not on Xcode clang's implicit system search list, sopkg-configemits a real-I— andsql.h/sqlext.h/sqlucode.hland in the unfiltered llvm-cov mapping of every coverage-instrumented target that linksLightweight::LightweightPUBLIC.check_coverage_roots.shthen fails, correctly. The gate is right; the build is wrong.morph's three existing
SYSTEMdemotions all readINTERFACE_INCLUDE_DIRECTORIESand cannot reach a compile option, andladder_<rung>_lib— the target that compiles the instrumented model code — has no demotion at all.The fix.
cmake/morph_demote_interface_includes.cmakemoves every-I<dir>out of a target'sINTERFACE_COMPILE_OPTIONSand re-adds it throughtarget_include_directories(SYSTEM INTERFACE)— the property the three existing demotions already read. Joined-I/pathand separated-I;/pathare both handled, every other option survives in order, the write goes to the real target rather than the alias, and it is idempotent. Called from bothFetchContent_MakeAvailable(Lightweight)sites (examples/common/CMakeLists.txt,examples/bank/CMakeLists.txt), because whichever configures first is the one that defines the target.The gate this could not be. On Linux
pkg-config --cflags odbcis empty — measured on this host:so
Lightweight'sINTERFACE_COMPILE_OPTIONSholds no-Iand "assert no-Iremains in the interface" would pass identically with the function body deleted.tests/compile_checks/demote_interface_includes_selftest.cmakehands the helper a synthetic option list instead — the joined form, the separated form, a relative directory, a plain option that must survive in order, and-include forced.hpp(which merely looks like-Ito a case-insensitive or prefix-only match, and whose argument must not be eaten) — and asserts both resulting properties exactly. It also pins idempotence, the no--Icase, and the all--Icase (INTERFACE_COMPILE_OPTIONSmust end up unset, not holding one empty string element). Configure-timeFATAL_ERROR, so it runs wherever tests are configured rather than only where the symptom is.Evidence — the control failing. With the helper's body removed:
…and with it restored,
-- Configuring done/-- Generating done.bash scripts/test_check_coverage_roots.shstill passes all seven directions unchanged — the gate itself is untouched:Evidence — the coverage leg still runs end to end on Linux. Configured exactly as
ci.yml'slinux-coveragejob does (--preset clang-coverage -DMORPH_BUILD_NET=ON -DMORPH_BUILD_OFFLINE_SQLITE=ON -DMORPH_BUILD_QT=ON -DMORPH_BUILD_LADDER=ON -DMORPH_LADDER_RUNGS=all):check_coverage_roots.sh— the gate the issue is about — passes with every one of the 612 mapped files under the checkout.On "the reported figure is unchanged" (acceptance item 4): I did not take a pre-change baseline on this host, so that is inferred, not measured as a delta. The inference is direct, though:
pkg-config --cflags odbcis empty here, so the helper'sif(NOT _morph_opts) return()fires and it changes nothing at all on Linux — and the change is confined toexamples//cmake//tests/CMakeLists.txt, none of which can move aninclude/morphfigure.Not done, and why: I have no Mac, so acceptance items 5–7 of the issue are not verified — an unfiltered
llvm-cov export -summary-onlynaming zero/opt/homebrewpaths, a greencoverage.shon the affected machine, and the before/after of a real-I→-isystemon aladder_<rung>_libcommand line. I did not reproduce the macOS symptom. The self-test proves the helper transforms what it is given; those three prove it is given the real thing, and remain for whoever has the hardware. Also not done, per the issue's out-of-scope list: any change at all tocheck_coverage_roots.sh(specifically not aSOURCEScross-check, which would have excused 111 of the 246 foreign records in #426's own repro); an upstream Lightweight fix plus pin bump (the correct long-term home, but it does not unblock this repository until a bump lands, and this helper is a no-op once it does); an audit of other third-party dependencies for the same shape.3. #428 — nothing hands out a
journalId, soUndoTransactionand the GUI's Undo control cannot be drivenThe defect.
UndoTransaction { ledgerId, journalId }reverses one journal entry, andJournalIdappeared in exactly one wire DTO field in the whole rung — that action's own input. No result type carried one. So the rung shipped a reversal a client could only drive with a guessed number, and shipped a desktop Undo button (gui/qml/LedgerView.qml:96-111) whose sole input was a journal id no screen ever displayed. The in-process tests reached around it through aDataMapper— exactly what a socket client and the QML bridge do not have.The fix. Closed the way #361/#384 (
b219ce51) closed the structurally identical hole one level up: by adding the action. Recording undo as "administrative" was not available —RunReportJobis genuinely the report runner's because the code enforces it, whileUndoTransactionhas no principal check beyond the ordinary owner gate, so declaring it administrative in prose would be half a scheme the rung cannot keep, and it would mean deleting shipped GUI surface.ListTransactions { ledgerId, month }→{ entries: [{ id, description, date, legs }] }: month-bounded in the exact"YYYY-MM"shapeGetBudgetReportuses so the answer cannot grow without bound; oldest first by the journal's own row id (a client-supplieddateis not required to be monotonic); registeredLoggable::Noand keyed onledgerIdlikeGetLedger; gated throughdb::requireOwnedBook— a listing of a book's entries is precisely the read #382's gate exists for.monthRangeMsanddetail::isValidYearMonthmove toledger/core/time_util.hppso both actions parse the month through one definition.GUI: the presenter gains
listTransactions/transactionsListed, the QML bridge anentriesproperty and alistTransactionsinvokable, andLedgerView.qml's hand-typed "Journal id to undo"TextFieldbecomes a month field, a list of that month's entries, and a per-row Undo button that hands back the id it is showing. A successful undo re-lists the month.The three records of the old contract are retired as the issue specifies:
undo-needs-a-journal-id-nothing-hands-out.scenario(whose whole premise was@body !~ "journalId") replaced bystore-list-and-undo-an-entry.scenario;coverage_allowlist.json'sledger/UndoTransactionentry deleted;open-account-transact-report-close.scenario's comment corrected. Both pins move deliberately:test_morph_scenario.py:993gainsListTransactions,:996goes 72 → 73. The workflow floor stays at 16 — the new file replaces the retired one rather than being added beside it, which is the case the issue scoped as "unless it is a new file".Evidence — would this pass if the feature did nothing? No.
Model cases (7 cases, 32 assertions), green:
Deleting the
WHERE ledgerclause — the assertion the issue names:Stubbing the action to
return ListTransactionsResult{}:The scenario, twice against one database and one real
ladder_ledger_server:Against the stubbed model it goes red at exactly the step the issue names:
Assertion mutation on the new file:
Drift gates:
Whole tree:
Everything, including the new test TU, compiles under
-Weverything -Werror(MORPH_ENABLE_STRICT_COMPILATION=ON); the QML-surface audit intest_ledger_qml_surface.cpppasses, so the new bridge members are really bound by the view in both directions.Not done, and why:
capture journal=$.entries[0].idresolves, shown red then green above. What is not met is the surrounding instruction to run the whole corpus:run_scenarios.py --rung ledger --twicedoes not pass as a whole, and does not pass onmastereither. 15 of the 16 ledger scenario files send enums as integers (kind=0,currency=1), and Batch: fix triage:valid issues #435, #392, #393, and part of #396 #444 (0dc871a9, 2026-09-04) gave every ledger enum aglz::meta/glz::enumerate, so a real server refuses those payloads with a glazeexpected_quoteparse error. Measured on this branch: the only file that passes in the whole ledger corpus is the new one. That breakage predates and is independent of this change, so perAGENTS.mdit is filed separately as scenario: all 16 ledger scenarios are refused by a real server since morph#444 gave the enums glz::meta #460 rather than folded in here — including the larger point, that nothing in CI runs the corpus at all, which is why 16/16 → 1/16 went unnoticed for two days with every gate green. The new scenario is written against the wire as it actually is (kind="Asset",currency="EUR") and was exercised on its own against a server started the same way the driver starts one.RunReportJoband its allowlist entry; pagination/cursors (the month bound is the whole bounding mechanism);StoreTransaction's result type; any QML redesign beyond replacing theundoJournalIdfield; undo semantics — the compensating entry, theAlreadyReversedguard and the causal-parent linkage are exactly as they were.DbFixture, the shapetest_ledger_book_ownership.cppestablished and the shape the issue cites; they are not run over theLocal/LocalSingleThread/Socketmatrix, because that matrix belongs totestkit/journey.hppand no ledger test uses it (onlyexamples/kanban/tests/journeys/does). The out-of-process path is covered by the scenario against a real server instead.Filed while working
glz::meta, and no gate runs the corpus.🤖 Generated with Claude Code