TrkQual BDT

[AndrewEdmonds11]

This PR goes with Mu2e/ArtAnalysis#7. Once this PR is merged, there will be two trkqual branches:

• trkqual contains the ANN result
• trkqual2 contains the BDT result

UPDATE 2026-07-24:
This PR goes with Mu2e/ArtAnalysis#7. Once this PR is merged, there will be two trkqual branches:

• trkqual contains the ANN result
• trkqual_bdt contains the BDT result

The following features have been added:

• trkqual branches can have arbirary names,
• trkqual metadata (version number, input tag) is stored in a histogram
  • you can check this using the checkEventNtuple script
  • you can require a specific model version in RooUtil analysis script using util.RequireTrkQualMetadata (see rooutil/examples/CompareTrkQualTrainings_UserBranches.C)
• RooUtil can better support run-time defined branches with the new UserBranch (should resolve RooUtil can only handle two trkqual branches and one mcsteps branch #306)

[oksuzian]

PR Review Summary — "TrkQual BDT" (#381)

Reviewed at head 151e5beb (opened 2026-06-30, 13 commits, last updated 2026-07-24). First review of this PR; no prior reviews or comments. Companion to Mu2e/ArtAnalysis#7. 15 files, +526/−214: a schema change from trkQualTags to a richer trkQualLeaves table, a trkqual_metadata provenance histogram, and a new RooUtil UserBranch mechanism for run-time-named branches.

Decision

• 🔴 Request changes. The design is a clear improvement — explicit leaf names instead of positional qual2/qual3, model provenance recorded in the file, and a general UserBranch that retires the hardcoded trkqual_alt/trkqual3 hack (#306). One defect blocks it: with trk.fill : false, the metadata histogram is created with the right number of bins but never labelled, and the new RooUtil reader then throws on every such file. Three fcl configs shipped in this repo produce exactly that. The fix is one condition. Everything else below is minor or coordination.

Scope understood

• src/EventNtupleMaker_module.cc: fhicl::Sequence<std::string> trkQualTags → fhicl::Sequence<fhicl::Table<TrkQualLeafConfig>> trkQualLeaves (leafname, inputTag, modelVersion); output branch name becomes <branchname>qual<leafname> instead of positional numbering; a trkqual_metadata TH1I records one labelled bin per configured algorithm.
• inc/TrkQualMetadata.hh (new): the {output_branch, input_tag, model_version} struct shared by the module and RooUtil.
• rooutil/: new UserBranch.hh (event- and track-scoped run-time branches), RooUtil::SetUserBranches/GetTrkQualMetadata/RequireTrkQualVersion, removal of the hardcoded trkqual_alt, and CompareTrkQualTrainings.C → CompareTrkQualTrainings_UserBranches.C.
• bin/checkEventNtuple: prints the metadata bin labels.
• fcl + docs: prolog.fcl migrated to trkQualLeaves and gains xgbFilename; from_mcs-reflection.fcl and from_mcs-mixed_trkQualCompare.fcl migrated; three READMEs updated.

---

Findings

1. 🔴 [S0] With trk.fill : false, trkqual_metadata is written with unlabelled bins, and RooUtil throws when reading the file.

   • Evidence, producer side — the bin count and the labels are computed under different conditions:

     for (const auto& trkFitConfig : _allTrkFitBranches) {
       if (trkFitConfig.fill()) nTrkQualAlgorithms += trkFitConfig.trkQualLeaves().size();   // per-branch fill only
     }
     if (nTrkQualAlgorithms > 0) { hTrkQualMetadata = tfs->make<TH1I>("trkqual_metadata", ..., nTrkQualAlgorithms, 0, nTrkQualAlgorithms); }
     ...
     if (_conf.trk().fill()) {                       // <-- the labels are written inside the global gate
       for (...) { if (!i_trkFitConfig.fill()) continue; ... SetBinLabel(++iTrkQualAlgorithm, metadataLabel); }
     }

     The count ignores the global trk.fill() switch; the labelling honours it. So trk.fill : false with any filled branch that has leaves produces a histogram with N bins and N empty labels.
   • Evidence, consumer side — rooutil/inc/RooUtil.hh, LoadTrkQualMetadata, called from the constructor for every input file:

     const std::string label = metadata_histogram->GetXaxis()->GetBinLabel(bin);
     const auto input_tag_pos = label.find(input_tag_marker);
     ...
     if (input_tag_pos == std::string::npos || ...) throw std::runtime_error("Invalid TrkQual metadata in " + filename + ": " + label);

     An empty label yields npos and throws. The histogram exists, so the early return for "no metadata" does not save it.
   • Evidence, that this is reached by shipped configs: the default is trk.fits : [ @local::All ] (fcl/prolog.fcl:329), and All has fill : true with two leaves after this PR. fcl/from_dig-calo.fcl, fcl/from_rec-crv-kpp.fcl and fcl/from_rec-crv-kpp-MC.fcl all set physics.analyzers.EventNtuple.trk.fill : false while inheriting that fits list. Ntuples from all three become unopenable in RooUtil — constructor throws, before any analysis code runs.
   • Impact: a file that looks fine to checkEventNtuple (it prints empty entries) and is rejected outright by the repo's own analysis library. It is also a silent trap for any user config that disables the tracker subsystem, which is the documented way to make calo- or CRV-only ntuples.
   • Suggested fix: gate the count the same way as the labels —

     if (_conf.trk().fill()) {
       for (const auto& trkFitConfig : _allTrkFitBranches) {
         if (trkFitConfig.fill()) nTrkQualAlgorithms += trkFitConfig.trkQualLeaves().size();
       }
     }

     Worth also making the reader defensive: skip empty labels rather than throwing, so older or oddly-produced files degrade to "no metadata" instead of being unreadable.

2. 🟠 [S1] This PR and ArtAnalysis#7 are mutually blocking — every merge order breaks jobs, and the window is not addressed anywhere.

   • Evidence: fcl/prolog.fcl adds xgbFilename : "ArtAnalysis/TrkDiag/data/TrkQual_BDT1_v2.0.ubj" to the TrkQual table, and every inputTag gains an :ANN/:BDT instance. Neither exists in ArtAnalysis main: TrackQuality's validated Config has no xgbFilename key (unknown key → fhicl validation error), and it still produces<MVAResultCollection>() on the unnamed instance. Conversely, merging ArtAnalysis#7 first makes xgbFilename required, which EventNtuple main's prolog does not set. Both orders abort at module construction.
   • Impact: two repos, two merge buttons, no atomic operation — there will be a window in which the next Analysis musing build is broken, and nothing in either PR body says how to sequence it. ArtAnalysis#7 also currently has open change requests (reviewed at a1920768), so the dependency is not merge-ready.
   • Suggested fix: state the sequencing plan in both PR bodies. The cleanest de-risking is on the ArtAnalysis side: keep the ANN on the unnamed instance and add only "BDT" as a named one, and give xgbFilename a default. Then #7 merges harmlessly on its own, this PR follows, and there is no broken window. If the instance rename is kept, say explicitly that the two must land within the same musing build.

3. 🟡 [S2] Nothing validates that leafname values are unique, and the old scheme could not produce a collision.

   • Evidence: the module builds output_branch = branchname + "qual" + leafname and calls _ntuple->Branch((outputBranch+".").c_str(), ...) once per leaf, with no check that two leaves in the same fit differ. The code this replaces auto-numbered (qual, qual2, qual3), so distinctness was structural.
   • Impact: a config with two leaves sharing a leafname writes two TTree branches with the same name, and — because LoadTrkQualMetadata keys its map on output_branch — the two bins from the same file then collide and throw "TrkQual metadata for <branch> differs between input files". The message names the wrong cause; the files are one file.
   • Suggested fix: throw a cet::exception at construction when two leaves in a fit produce the same output_branch, naming both input tags. That converts a confusing downstream failure into a config error at job start.

4. 🟡 [S2] modelVersion has three different naming conventions inside this one PR, and RequireTrkQualVersion compares them by exact string.

   • Evidence: fcl/prolog.fcl writes modelVersion : "ANN1_v2" and "BDT1_v2"; fcl/from_mcs-reflection.fcl writes "TrkQual_ANN1_v2" for the same ANN model; fcl/from_mcs-mixed_trkQualCompare.fcl uses "TrkQual_ANN1_v1.0"/"TrkQual_ANN1_v1.1"/"TrkQual_ANN1_v2"; rooutil/README.md's example checks "ANN2_v2". Meanwhile the BDT artefact is TrkQual_BDT1_v2.0.ubj — v2.0, not v2.
   • Impact: RequireTrkQualVersion(branch, expected) is a literal != on these strings. An analyst who copies the README's form and runs on standard ntuples gets a spurious throw; one who copies the prolog form gets a spurious throw on reflection ntuples. The provenance feature only works if the strings are conventional.
   • Suggested fix: pick one form — ideally the model filename stem, so TrkQual_ANN1_v2 and TrkQual_BDT1_v2.0 — use it in every fcl and README in this PR, and say in fcl/README.md that it must match the model file. Settling the v2/v2.0 split with MLTrain (the notebook's training_version = "2.0" is what produces the dotted form) would remove the ambiguity at the source.

5. 🟡 [S2] fcl/from_mcs-mixed_trkQualCompare.fcl points at two ONNX files that do not exist.

   • Evidence: it now sets onnxFilename : "ArtAnalysis/TrkDiag/data/TrkQual_ANN1_v1.onnx" and ".../TrkQual_ANN1_v1.1.onnx". ArtAnalysis/TrkDiag/data/ contains TrkQual_ANN1_v1.dat, TrkQual_ANN1_v1.1.dat, TrkQual_ANN1_v2.dat and TrkQual_ANN1_v2.onnx — the two v1 models exist only in the retired SOFIE .dat format.
   • Impact: the example cannot run as shipped. The PR's own comment says the files "must be installed beside the v2 model before running this example", which is honest, and fcl/README.md repeats it — but neither says where to obtain them, and ONNX exports of v1/v1.1 do not appear to exist anywhere. (For context, the file was already broken on main for a different reason — it set the retired datFilename key — so this is an improvement that stops one step short.)
   • Suggested fix: either commit the two ONNX conversions to ArtAnalysis, or point the example at models that exist (e.g. compare TrkQualAll:ANN against TrkQualAll:BDT, which needs no extra files and demonstrates the same feature).

6. 🟡 [S2] AGENTS.md still documents the removed trkQualTags key.

   • Evidence: AGENTS.md:90 at this head still shows trkQualTags : ["TrkQualDeM"] in its EventNtuple configuration example. Three READMEs were updated in this PR; this one was not.
   • Impact: it is the file an agent or newcomer reads first for the config shape, and after this PR it documents a key that fails validation.
   • Suggested fix: update the snippet to the trkQualLeaves form used in fcl/prolog.fcl.

7. ⚪ [S3] Batch, none gating:

   • The metadata round-trips through a formatted string ("%s: input tag = %s; model version = %s") parsed back with find. Any field containing ": input tag = " or "; model version = " mis-splits, and char metadataLabel[1024] silently truncates. Both are unlikely in practice; a std::string concatenation would at least remove the truncation and the <cstdio> include.
   • The PR body advertises util.RequireTrkQualMetadata; the implemented method is RequireTrkQualVersion (the README has it right).
   • trk.fillTrkQual is declared in the Config (:162) and read nowhere in the module — pre-existing, but it is exactly the switch a user would reach for to work around finding 1, and fcl/from_mcs-reflection.fcl and fcl/from_mcs-extracted.fcl both set it expecting an effect. Either wire it up or remove it.
   • Header-guard styles now differ inside the repo: the new inc/TrkQualMetadata.hh correctly uses EventNtuple_TrkQualMetadata_hh, while the new rooutil/inc/UserBranch.hh uses UserBranch_hh_ — which does match its neighbours (Event_hh_, Track_hh_, RooUtil_hh_), so this is a repo-wide inconsistency to settle separately, not something to fix in this PR.
   • EventUserBranch<T>::BranchOutput no-ops when value_ is still null, i.e. before the first GetEntry. Worth a comment on the call-order requirement, since the failure is a silently missing output branch.

Verified 🟢 (checked, no action needed)

• 🟢 The cross-file provenance check is correct and is the most valuable part of the feature: LoadTrkQualMetadata throws when the same output branch carries a different input tag or model version in different files of a chain, which is exactly the mixed-training accident the metadata exists to prevent.
• 🟢 No null dereference of hTrkQualMetadata. The labelling loop runs only for branches with fill() true inside if (_conf.trk().fill()), and the counting loop is a superset of that, so the pointer is always non-null where it is used and iTrkQualAlgorithm can never exceed the bin count. (The superset relation is precisely what produces finding 1 — the histogram is over-sized, never under-sized.)
• 🟢 The default case is backward compatible: leafname : "" yields trkqual, byte-identical to the old branch + "qual" name, so existing analysis code reading trkqual is unaffected by the schema change.
• 🟢 Retiring trkqual_alt/the hardcoded trkqual3 in favour of UserBranch is a real simplification — it removes a // TODO: un-hardcode those and three special cases from Event.hh (bind, per-track update, and the erase path), and the replacement example ships in the same PR.
• 🟢 UserBranch is defensively written where it matters: Bind checks both branch existence and branch status, TrackElementPtr bounds-checks the index, EraseTrack checks null and range, and RooUtil::SetUserBranches de-duplicates by name.
• 🟢 checkEventNtuple guards on folder.GetListOfKeys().Contains(...) before reading the histogram, so older files without it print nothing rather than failing.
• 🟢 Blast radius of the schema change is contained: no trkQualTags reference exists in Production or mu2e-trig-config; all in-repo fcl uses are migrated in this PR (only AGENTS.md, finding 6, is left).
• 🟢 Storing provenance in TH1 bin labels is not a new hack — it mirrors the existing version histogram idiom that SetVersionNumber already reads.
• 🟢 PR body quality is good: intent, the branch names users will see, and the feature list are all stated, with the superseded plan struck through rather than deleted.

Validation check

• Build/tests run: none — EventNtuple has no CI (no statuses and no check runs at 151e5beb), the PR body carries no build or run evidence, and this review did not compile or run anything. All findings are from reading the code at this head plus the state of ArtAnalysis main.
• Config contract check: fail — finding 1 (metadata histogram inconsistent with its own reader) and finding 2 (keys and product instances that do not exist in ArtAnalysis main).
• Cross-repo consistency: needs follow-up — mutually blocking with ArtAnalysis#7 (finding 2); Production and mu2e-trig-config are unaffected.
• Merge state: the PR reports mergeable: false — it conflicts with main and needs a rebase before any of this can be exercised.

Residual risk

• Finding 1 is the sharp edge: it produces files that pass casual inspection and fail hard at analysis time, from configs nobody edits.
• The BDT output itself is unvalidated here and in ArtAnalysis#7 — two quality branches ship with equal standing and no guidance on which analyses should use. A ROC overlay in either PR body would settle it.
• RequireTrkQualVersion is only as good as the modelVersion strings (finding 4); until they are conventional, the safest use is GetTrkQualMetadata plus an explicit comparison in analysis code.

Author follow-ups

1. Count nTrkQualAlgorithms under the same _conf.trk().fill() gate that writes the labels, and consider making LoadTrkQualMetadata skip empty labels rather than throw (finding 1, blocking).
2. State the merge sequencing with ArtAnalysis#7 in both PR bodies — or de-risk it by keeping the ANN on the unnamed product instance and defaulting xgbFilename on the ArtAnalysis side (finding 2).
3. Reject duplicate leafnames at construction with a message naming both input tags (finding 3).
4. Settle one modelVersion convention and use it in every fcl and README here (finding 4).
5. Point from_mcs-mixed_trkQualCompare.fcl at models that exist, or commit the v1/v1.1 ONNX exports (finding 5).
6. Update the AGENTS.md config snippet (finding 6), and rebase — the PR currently conflicts with main.
7. Optional: the S3 batch in finding 7, especially wiring up or removing trk.fillTrkQual.

[oksuzian]

Notes on the merge conflict

I ran the merge locally (151e5beb + main @ 84e7404e) to see what it would take, and aborted without pushing — a couple of the resolutions are yours to make rather than mine. Posting the analysis in case it saves you the exploration.

main is 45 commits ahead and has landed work that overlaps this PR directly: 12 conflict hunks across 6 files.

Mechanical — no decisions involved

File	Conflict	Resolution
README.md	Both sides add a checkEventNtuple sentence, both citing v6.13.0	Combine the two sentences
rooutil/inc/Event.hh	This PR adds SetUserBranches; main moved CheckForBranch out into the new BranchUtils.hh as a free function	Keep SetUserBranches, drop the now-duplicated member
rooutil/inc/Track.hh	This PR adds SetUserBranch/GetUserBranch + the user_branches map; main adds the trkdtdt pointer in the same block	Keep both

Needs a decision from you

1. Version number. This PR sets minor/patch to 11/2; main is now 12/1. Both branches' READMEs advertise features "after v6.13.0", so 13/0 looks right — but that's a release call, not a merge resolution.

2. LoadTrkQualMetadata vs the new totals mechanism. This is the substantive one. The PR hooks SetNProcessedEvents(file) and LoadTrkQualMetadata(file) into the per-file loop in RooUtil.hh. On main, the SetNProcessedEvents method and the n_proc_events member no longer exist — the subrun work merged as Subrun ntuple #391 (5e844f2) replaced them with a generic totals mechanism (TotalNames(), totals, incomplete_totals, GetTotal("n_proc_events")) that checkEventNtuple already prints, and hVersion picked up a = nullptr initialiser in the same refactor. So after the merge, this PR's bespoke trkqual_metadata histogram plus its own printTrkQualMetadata sits alongside a general per-file provenance mechanism built for the same purpose. Re-adding the deleted SetNProcessedEvents call would be wrong; whether TrkQual provenance should fold into totals (or stay separate, with LoadTrkQualMetadata hooked into whatever loop survives) is a design question I didn't want to answer on your behalf.

One thing worth knowing before resolving

main has already migrated trkPIDTags to the instance-qualified form via #386 — trkPIDTags : ["TrkPID:MergeKKAll"] in the All branch, ["TrkPID:KKDe"] in De — and added trkDtDtTag : "TrkDtDt:MergeKKAll". That is the EventNtuple half of the ArtAnalysis TrackPID change, i.e. the same label:instance pattern this PR introduces for TrkQual. Both conflicting hunks in fcl/prolog.fcl are exactly this: your trkQualLeaves on one side, main's new trkPIDTags/trkDtDtTag on the other. A correct resolution keeps both — it is easy to lose trkDtDtTag here, since it is a pure addition on the main side of the hunk.

The src/EventNtupleMaker_module.cc hunk is the version-number lines plus the trkqual_metadata block; note that resolving it re-commits the nTrkQualAlgorithms counting issue from my review above, so it may be worth fixing that in the same pass rather than merging and then amending.

Happy to push the three mechanical files and the prolog.fcl combination if that is useful, leaving the version number and the metadata/totals question for you — just say the word.

[AndrewEdmonds11]

Replies to initial AI review. I'll now work on the merge conflict

🔴 [S0] With trk.fill : false, trkqual_metadata is written with unlabelled bins, and RooUtil throws when reading the file.

Good catch. Fixed with code as suggested

🟠 [S1] This PR and ArtAnalysis#7 are mutually blocking — every merge order breaks jobs, and the window is not addressed anywhere.

I think the AI is making a mountain out of a molehill here. We just need to have them both merged before we tag and release

🟡 [S2] Nothing validates that leafname values are unique, and the old scheme could not produce a collision.

Added uniqueness check and throw if leafname already found.

🟡 [S2] modelVersion has three different naming conventions inside this one PR, and RequireTrkQualVersion compares them by exact string

Already solved based on work done in response to AI review of Mu2e/ArtAnalysis#7

🟡 [S2] fcl/from_mcs-mixed_trkQualCompare.fcl points at two ONNX files that do not exist

Will update in the future. Added a TODO to the file

🟡 [S2] AGENTS.md still documents the removed trkQualTags key.

I'm not sure what to do here. Should I remove the AGENTS.md file and we have agents rely on the READMEs?

⚪ [S3] Batch, none gating

Fixed a few of these (see commit messages for details)

[oksuzian]

PR Review Summary — re-review of "TrkQual BDT" (#381)

Reviewed at head 913baba2 (2026-08-07). Second pass. Prior review: 🔴 at 151e5beb (review 4879365426, 7 findings), plus my merge-conflict notes (comment 5211695423). The delta is 7 commits, 151e5beb..913baba2: six author commits and the main merge 913baba2. Every prior finding is accounted for below, verified in the code at this head rather than taken from the commit messages. The PR is now mergeable: clean, and I checked the merge resolution hunk-by-hunk against main @ 84e7404e (which is the merge's second parent, so git diff upstream/main 913baba2 is exactly this PR's net contribution).

Decision

• 🔴 Request changes. The prior blocker is properly fixed and verified, and four of the seven findings are closed. What replaces it is entirely merge damage: the merge dropped a closing brace and rooutil/inc/RooUtil.hh no longer compiles, and it rolled the ntuple version number backwards past main. Both are mechanical. The good news is the thing I warned was easiest to lose in that merge — main's trkDtDtTag/trkPIDTags work — came through intact.

Scope understood (delta only)

• b4702c05 fixes the S0 metadata/trk.fill mismatch; 925985bb adds the leafname uniqueness check; 22c26c17 removes the dead fillTrkQual fhicl key; 09492660 follows ArtAnalysis#7's model-file rename; c752ee83 adds a TODO to the compare example; 43aaac09 changes an include guard.
• 913baba2 merges main (45 commits), which had landed the subrun/totals mechanism (#391), trkPIDTags instance qualification and trkDtDtTag (#386), and BranchUtils.hh.

---

Findings

1. 🔴 [S0] The merge lost the closing brace of LoadTrkQualMetadata — rooutil/inc/RooUtil.hh does not compile.

   • Evidence: at 43aaac09 (the pre-merge branch tip) the function closed cleanly — trkqual_metadata[...] = metadata; at :267, } closing the loop at :268, } closing the function at :269. At 913baba2 the second brace is gone: :369 assigns, :370 closes the for, :371 is a whitespace-only line, and :372 is static const std::vector<std::string>& TotalNames() { — main's method, now nested inside LoadTrkQualMetadata's body. A brace-depth trace over the file ends at depth 1 at EOF instead of 0; TotalNames, BuildSubRunIndex and every data member from :392 on sit at function scope instead of class scope.
   • Confirmed with a compiler, not by eye. Stripping #-prefixed lines (so it parses without ROOT) and running g++ -std=c++20 -fsyntax-only:

     error: a function-definition is not allowed here before ‘{’ token   <- TotalNames()
     error: expected ‘}’ at end of input
     

     The stripped-preprocessor trick means the other diagnostics in that run are noise, but these two are pure syntax and independent of the missing headers.
   • Impact: RooUtil.hh is the entry point for every rooutil analysis — all six rooutil/examples/*.C, validation/test_rooutil_examples.sh, and any user macro. Nothing that includes it builds. It is a pure merge artifact: the pre-merge branch and main both compile.
   • Suggested fix: one } after :370, and drop the trailing-whitespace line while you are there.

2. 🟠 [S1] The merge rolled the ntuple version backwards, from main's 6.12.1 to 6.11.2.

   • Evidence: src/EventNtupleMaker_module.cc:638-640 at this head stamps major 6, minor 11, patch 2. main at the merge base stamps 6 / 12 / 1 (:613-615). The net diff against main shows minor 12 → 11 and patch 1 → 2, i.e. the merge took the branch side of a hunk where main had moved on.
   • Impact: ntuples produced after this merges would advertise a version older than files already produced from main, and RooUtil::SetVersionNumber (rooutil/inc/RooUtil.hh:63-74) reads those three bins as the file's identity. This PR's own README.md also promises the trkqual metadata "for versions after v6.13.0" — a claim 6.11.2 can never satisfy.
   • Suggested fix: 6 / 13 / 0. This is the release call I flagged in the merge notes rather than resolve myself; the merge resolved it by default instead of by decision.

3. 🟠 [S1] Track::trkqual_bdt is declared but never bound — a permanently null pointer named exactly like the new branch.

   • Evidence: rooutil/inc/Track.hh:206 declares mu2e::MVAResultInfo* trkqual_bdt = nullptr;. The only other occurrence of that identifier anywhere in the repo is the string literal "trkqual_bdt" used as a default argument in rooutil/examples/CompareTrkQualTrainings_UserBranches.C:21. Nothing assigns the member: Event::Update calls UpdateObject for track.trkqual, track.trkpid, track.trkdtdt and then walks user_branches — trkqual_bdt is not in that list, and Event.hh has no CheckForBranch for it either.
   • Impact: prolog.fcl:277-278 produces a branch literally called trkqual_bdt, and every sibling member of that struct is bound, so track.trkqual_bdt->result() is the obvious thing for a user to write — and it dereferences null. This is the residue of trkqual_alt: it was renamed rather than removed, which also undercuts the PR's own goal of retiring hardcoded quality members in favour of UserBranch.
   • Suggested fix: delete the member. The BDT is reached with GetUserBranch<mu2e::MVAResultInfo>("trkqual_bdt"), which is exactly what the new example demonstrates.
   • This one is mine to own: my previous review credited this PR with removing trkqual_alt cleanly, and it did not — I read the deletions in Event.hh and did not check Track.hh.

4. 🟡 [S2] CheckForBranch now exists twice.

   • Evidence: main moved it out to rooutil/inc/BranchUtils.hh:7 as a free inline in namespace rooutil, where SubRun.hh:10-13 uses it. The merge kept the branch's copy as well, so rooutil/inc/Event.hh:108 re-declares it as an Event member with a byte-identical body — while Event.hh:51 still includes BranchUtils.hh.
   • Impact: not a compile break. The member hides the namespace-scope function for all 40-odd calls inside Event, and since the bodies are identical the behaviour is unchanged. The cost is that two copies of the same four lines now have to stay in sync, and the include reads as if the free function were the one in use.
   • Suggested fix: delete Event.hh:107-112; the include already provides it. (This is the "drop the now-duplicated member" line from my merge notes.)

5. 🟡 [S2] modelVersion still has three conventions — carried over, unaddressed.

   • You replied that this was "already solved based on work done in response to AI review of Mu2e/ArtAnalysis#7". That work settled the model filenames (TrkQual_ANN1_v2.0.onnx, TrkQual_BDT1_v2.0.ubj); the modelVersion strings are separate, and they are what RequireTrkQualVersion compares. At this head:
     • fcl/prolog.fcl:206,277,278,294,477 → ANN1_v2 / BDT1_v2
     • fcl/from_mcs-reflection.fcl:85,92 → TrkQual_ANN1_v2
     • fcl/from_mcs-mixed_trkQualCompare.fcl:19-21 → TrkQual_ANN1_v1.0 / v1.1 / v2
     • fcl/README.md:19-20 → TrkQual_ANN1_v2 / TrkQual_ANN1_v3_rc1
   • Impact: an analyst who copies RequireTrkQualVersion("trkqual", "TrkQual_ANN1_v2") straight out of your own fcl/README.md gets a throw on every standard ntuple, because prolog.fcl stamps ANN1_v2. The provenance feature only pays off if the strings are conventional.
   • Extra: from_mcs-mixed_trkQualCompare.fcl:21 is three spellings of one model on one line — leafname _v2_0, modelVersion : "TrkQual_ANN1_v2", file TrkQual_ANN1_v2.0.onnx.
   • Suggested fix: one form everywhere — the filename stem (TrkQual_ANN1_v2.0, TrkQual_BDT1_v2.0) is the one that cannot drift from the artefact — and a line in fcl/README.md saying it must match the model file.

6. 🟡 [S2] AGENTS.md:90 still documents the removed trkQualTags key — carried over, with an answer to your question.

   • You asked whether to delete AGENTS.md and let agents rely on the READMEs. I would keep it: AGENTS.md is the filename agent tooling looks for by convention, and a README is not a substitute for that. But the drift you just hit is the argument against the content it currently has — a duplicated fhicl example that nobody remembers to update. The version that does not rot is a short pointer file: what the repo is, where the build lives, and "for EventNtuple configuration see fcl/README.md", with no inline config snippet at all.
   • Minimum for this PR is the one line: replace the trkQualTags : ["TrkQualDeM"] snippet with the trkQualLeaves form from prolog.fcl.

7. 🟡 [S2] from_mcs-mixed_trkQualCompare.fcl still points at ONNX files that do not exist — partially addressed.

   • The TODO added in c752ee83 is honest and I would not block on it. Worth noting the zero-cost alternative, though: after this PR TrkQualAll produces both :ANN and :BDT by default, so the example can demonstrate the whole user-branch mechanism by comparing trkqual against trkqual_bdt with no extra model files at all, and stop shipping a config that cannot be run as-is.

8. ⚪ [S3] Batch, none gating:

   • inc/TrkQualMetadata.hh — 43aaac09 ("Match include guards with files already in repo") changed EventNtuple_TrkQualMetadata_hh to TrkQualMetadata_hh. That took my note about rooutil/inc/UserBranch.hh and applied it to the other file: my review said this header's original guard was the correct one under the Mu2e convention (repo prefix outside Offline). The neighbours it was matched to are TrkInfo_HH, MVAResultInfo_HH, SubRunInfo_HH, TrkDtDtInfo_HH — uppercase _HH — so the new form matches neither the convention nor its own directory. Zero functional impact; revert it or leave it, but the repo-wide cleanup is still the real fix and still belongs in its own PR.
   • The PR body advertises util.RequireTrkQualMetadata; the implemented method is RequireTrkQualVersion (the README has it right).
   • The metadata still round-trips through snprintf into char[1024] and is parsed back with find — unchanged, still not worth blocking on, still cheaper as string concatenation.
   • The new uniqueness check (src/EventNtupleMaker_module.cc:571-579) is scoped per fit branch, which is exactly what I asked for. Note that several prolog entries share branchname : "trk" (Ext, Off, All, DeCalib, TTMCBranch), so two different fits both configured with leafname : "" would still produce two trkqual branches and surface as the misleading "differs between input files" throw. Keying the check on the full output-branch name across all fits closes that too.
   • Two unrelated blank lines were deleted in src/EventNtupleMaker_module.cc (both immediately before // Time clusters).

---

Carry-forward accounting (vs review 4879365426 at 151e5beb)

1. 🟢 [was S0] trk.fill : false produced unlabelled metadata bins — FIXED in b4702c05, verified. src/EventNtupleMaker_module.cc:641-647 now counts inside if (_conf.trk().fill()), matching the gate that writes the labels, so the count and the labelling are driven by the same condition. Re-checked the null-dereference argument at the new head: the labelling loop is still a subset of the counting loop, so hTrkQualMetadata is non-null wherever it is used. from_dig-calo.fcl, from_rec-crv-kpp.fcl and from_rec-crv-kpp-MC.fcl now produce no histogram at all rather than a broken one. The reader was not made defensive against empty labels — that half was optional and I am not carrying it.
2. 🟠 [was S1] Mutual blocking with ArtAnalysis#7 — still open, and the coupling grew. Your reply ("we just need to have them both merged before we tag and release") is a fair answer for the release, and I will not argue the size of the mountain — but it does not cover the window in which someone builds the Analysis musing from the two mains, which is the failure I was describing. What is new: 09492660 follows #7's rename to TrkQual_ANN1_v2.0.onnx (fcl/prolog.fcl:17), so this PR is consistent with #7 as it stands today. My re-review of #7 at c0dee801 flagged that rename as its own S0 and recommended reverting it (fixing MLTrain's training_version to "2" instead), because it breaks EventNtuple main and from_mcs-mixed_trkQualCompare.fcl. Pick the direction once, in #7; if it reverts, prolog.fcl:17 here reverts with it.
3. 🟢 [was S2] Duplicate leafname — FIXED in 925985bb, verified. src/EventNtupleMaker_module.cc:571-579 throws a cet::exception at construction naming the offending output branch. See the scope note in finding 8.
4. 🟡 [was S2] modelVersion conventions — UNADDRESSED, now finding 5.
5. 🟡 [was S2] Nonexistent ONNX files in the compare example — PARTIAL (c752ee83 adds a TODO), now finding 7.
6. 🟡 [was S2] AGENTS.md — UNADDRESSED, now finding 6, with an answer to your question.
7. ⚪ [was S3] Batch — one properly done, one done backwards, the rest open. trk.fillTrkQual is genuinely removed (22c26c17): the fhicl atom is gone from Config, and the two configs that set it (from_mcs-extracted.fcl, from_mcs-reflection.fcl) plus the prolog.fcl default were all cleaned up in the same commit — the removal of a validated key would otherwise be an S0, so this was the right way to do it. The include-guard change went the wrong direction (finding 8). The snprintf round-trip, the PR-body method name and the BranchOutput call-order comment are unchanged.

---

Verified 🟢 — no action needed

• 🟢 The merge preserved main's trkDtDtTag / trkPIDTags work, which is what I flagged as easiest to lose in these hunks. prolog.fcl All keeps trkPIDTags : ["TrkPID:MergeKKAll"] and trkDtDtTag : "TrkDtDt:MergeKKAll" alongside the new trkQualLeaves; De keeps ["TrkPID:KKDe"] and its trkDtDtTag; ENDeCalib keeps its own; fillTrkDtDt : true survives in the EventNtupleMaker defaults; and Track::trkdtdt (Track.hh:193) plus all four Event.hh sites (:62, :139, :295, :434) are intact.
• 🟢 Removing the fillTrkQual fhicl key is safe outside this repo. Code search over the Mu2e org returns fillTrkQual only in EventNtuple — the three files this PR fixes, plus the unrelated InfoStructHelper::fillTrkQualInfo. Production and mu2e-trig-config return zero hits for both fillTrkQual and trkQualTags, so no external config breaks on the schema change.
• 🟢 main's subrun/totals mechanism came through unchanged. TotalNames, totals, incomplete_totals, BuildSubRunIndex, SubRun.hh, SelectSubRuns.C and SubRunCounting.C are all present with no edits from this branch — the net diff against main in that region is additions only. They are merely mis-scoped by finding 1's missing brace, and correcting that brace restores them to class scope with no further work.
• 🟢 bin/checkEventNtuple and README.md merged correctly — the totals printing from main and the trkqual-metadata printing from this branch both survive, and the README sentence combines both features rather than replacing one.
• 🟢 The provenance design itself is unchanged and still the most valuable part: cross-file LoadTrkQualMetadata rejects a chain whose files disagree on input tag or model version.
• 🟢 The leafname : "" default still yields trkqual, byte-identical to the old name, so existing analysis code is unaffected by the schema change.

Validation check

• Build/tests run: EventNtuple has no CI (no statuses, no check runs at 913baba2) and the PR body carries no build evidence. I did compile-check one file: grep -v '^\s*#' rooutil/inc/RooUtil.hh > /tmp/x.cc && g++ -std=c++20 -fsyntax-only /tmp/x.cc, which is how finding 1 was confirmed rather than inferred. Everything else is static, against this head, main @ 84e7404e, and ArtAnalysis#7 @ c0dee801.
• Config contract check: partial — the fhicl schema is coherent and the fillTrkQual removal is clean; modelVersion (finding 5) is not.
• Cross-repo consistency: needs follow-up — settle the ArtAnalysis#7 model-filename direction, then align prolog.fcl:17 (carry-forward 2). Production and mu2e-trig-config are unaffected.
• Merge quality: verified hunk-by-hunk against main. Six conflicted files, twelve hunks: ten resolved correctly, one lost a brace (finding 1), one took the stale side of the version bump (finding 2), one carried a duplicate (finding 4).

Residual risk

• Findings 1 and 2 are both merge damage rather than design problems, which is worth saying plainly — the review content of the PR is in better shape than it was last pass. But with no CI, a dropped brace in a header ships to whoever builds next, and a wrong version number is invisible forever after the files are written.
• The BDT is still unvalidated in either PR — two quality branches ship with equal standing and no guidance on which one analyses should use. A ROC overlay in one of the two PR bodies would close it.
• RequireTrkQualVersion remains only as good as the modelVersion strings; until finding 5 is settled, GetTrkQualMetadata plus an explicit comparison is the safer call in analysis code.

Author follow-ups

1. Add the missing } in rooutil/inc/RooUtil.hh after :370 (finding 1, blocking) — and consider a compile-only CI job, since this is precisely the class of break that one would have caught.
2. Set the version to 6/13/0 (finding 2).
3. Delete Track::trkqual_bdt (finding 3) and the duplicated Event::CheckForBranch (finding 4) — both are merge/rename residue.
4. Settle one modelVersion convention and use it in every fcl and README here (finding 5).
5. Fix the AGENTS.md snippet, or reduce the file to a pointer (finding 6).
6. Optional: the compare example against trkqual/trkqual_bdt (finding 7) and the S3 batch (finding 8).
7. Coordinate the model-filename direction with ArtAnalysis#7 before either merges (carry-forward 2).

[AndrewEdmonds11]

🔴 [S0] The merge lost the closing brace of LoadTrkQualMetadata — rooutil/inc/RooUtil.hh does not compile.

Good catch. Fixed

🟠 [S1] The merge rolled the ntuple version backwards, from main's 6.12.1 to 6.11.2

Reverted back to 6.12.1

🟠 [S1] Track::trkqual_bdt is declared but never bound — a permanently null pointer named exactly like the new branch

Removed. As suggested by the AI

🟡 [S2] CheckForBranch now exists twice.

Removed

[oksuzian]

PR Review Summary — third pass on "TrkQual BDT" (#381)

Reviewed at head 8e45eb7e (2026-08-07). Prior reviews: 🔴 at 151e5beb (4879365426) and 🔴 at 913baba2 (4885041129). The delta is 4 commits, 913baba2..8e45eb7e, +4/−11 across four files — all of it fixes to the merge damage from the last pass. Verified in the code at this head.

Decision

• 🟡 Comment. Every blocker is fixed, and I checked each one rather than taking the commit message for it: RooUtil.hh now parses, the version number is right, and the two dangling members are gone. Nothing in this PR's own code is S0 or S1 any more. What is left is the cross-repo sequencing with ArtAnalysis#7 and three 🟡 documentation items carried from the first pass — none of which gate the merge.
• Mechanically: a COMMENTED review does not clear an earlier CHANGES_REQUESTED on GitHub, so my block from 913baba2 still shows until I approve. I am not re-asserting it. Ping me once the modelVersion strings are settled (finding 2) and I will approve; I would rather not approve while ArtAnalysis#7 still carries an open S0 that this PR's prolog.fcl:17 depends on.

---

Findings

1. 🟠 [S1] Sequencing with ArtAnalysis#7 — carried over, and the coupling is now on the model filename too.

   • Status: ArtAnalysis#7 is unchanged at c0dee801 with my CHANGES_REQUESTED open. fcl/prolog.fcl:17 here points at TrkQual_ANN1_v2.0.onnx, which exists only on the #7 branch — ArtAnalysis main still ships TrkQual_ANN1_v2.onnx. My review of #7 called that rename its own S0 and recommended reverting it (fixing MLTrain's training_version to "2" instead of renaming the deployed artefact).
   • Impact: this PR is consistent with #7 as it stands today, so the pair is coherent — but if #7 reverts the rename, prolog.fcl:17 and from_mcs-mixed_trkQualCompare.fcl:14 revert with it. And merging either PR alone still aborts jobs, which your "both merged before we tag and release" plan covers for the release but not for anyone building the Analysis musing from the two mains in between.
   • Suggested fix: settle the filename direction once, in #7. Nothing to do here until then.

2. 🟡 [S2] modelVersion still has four spellings, and both documented examples of the new API throw — carried over, unaddressed.

   • This is the same finding as last pass; restating it sharply because it is now the main thing between this PR and an approve. For the plain trkqual branch of TrkQualAll:ANN, three files disagree about what is stored there:
     • fcl/prolog.fcl:277 — what is actually stamped: ANN1_v2
     • fcl/README.md:19 — documents the same leaf as TrkQual_ANN1_v2
     • rooutil/README.md:143 — util.RequireTrkQualVersion("trkqual", "ANN2_v2");
   • Impact: RequireTrkQualVersion is a literal !=, so both README examples throw on a standard ntuple. The rooutil one is also the only usage example the feature ships with, and its ANN2 looks like a typo for ANN1 on top of the convention mismatch. Elsewhere: from_mcs-reflection.fcl:85,92 use TrkQual_ANN1_v2, and from_mcs-mixed_trkQualCompare.fcl:21 manages three spellings of one model on a single line — leafname _v2_0, modelVersion : "TrkQual_ANN1_v2", file TrkQual_ANN1_v2.0.onnx.
   • Suggested fix: one form everywhere — the model filename stem (TrkQual_ANN1_v2.0, TrkQual_BDT1_v2.0) is the only one that cannot drift from the artefact — plus a line in fcl/README.md saying it must match the model file, and the same string in both README examples.

3. 🟡 [S2] AGENTS.md:90 still documents the removed trkQualTags key — carried over.

   • Answering the question you raised: I would keep the file. AGENTS.md is the name agent tooling looks for by convention and a README is not a substitute for it. But the drift you just hit is the argument against its current contents — a duplicated fhicl example nobody remembers to update. The version that does not rot is a short pointer: what the repo is, where the build lives, and "for EventNtuple configuration see fcl/README.md", with no inline config snippet at all.
   • Minimum for this PR is the one line: replace trkQualTags : ["TrkQualDeM"] with the trkQualLeaves form.

4. 🟡 [S2] from_mcs-mixed_trkQualCompare.fcl still points at ONNX files that do not exist — partially addressed.

   • The TODO from c752ee83 is honest and I would not block on it. Restating the zero-cost alternative: after this PR TrkQualAll produces both :ANN and :BDT by default, so the example can demonstrate the whole user-branch mechanism by comparing trkqual against trkqual_bdt with no extra model files, and stop shipping a config that cannot be run as written.

5. ⚪ [S3] Batch, none gating, all carried:

   • The PR body still advertises util.RequireTrkQualMetadata; the implemented method is RequireTrkQualVersion.
   • inc/TrkQualMetadata.hh still guards on TrkQualMetadata_hh. As noted last pass, 43aaac09 applied my UserBranch.hh remark to the wrong file — and the inc/ neighbours it was matched to are TrkInfo_HH, MVAResultInfo_HH, SubRunInfo_HH, TrkDtDtInfo_HH, uppercase. Zero functional impact; the repo-wide cleanup belongs in its own PR either way.
   • The metadata still round-trips through snprintf into char[1024] and is parsed back with find.
   • The uniqueness check is scoped per fit branch. Several prolog entries share branchname : "trk" (Ext, Off, All, DeCalib, TTMCBranch), so two different fits both configured with leafname : "" would still collide and surface as the misleading "differs between input files" throw. Keying on the full output-branch name across all fits closes that too.

---

Carry-forward accounting (vs review 4885041129 at 913baba2)

1. 🟢 [was S0] Missing closing brace in LoadTrkQualMetadata — FIXED in 7dba084, verified. rooutil/inc/RooUtil.hh:371 now closes the function before TotalNames(). Re-ran the same check that found it — grep -v '^\s*#' rooutil/inc/RooUtil.hh > /tmp/x.cc && g++ -std=c++20 -fsyntax-only /tmp/x.cc — and both structural errors are gone; brace depth returns to 0 at EOF, so TotalNames, BuildSubRunIndex and the data members are back at class scope.
2. 🟢 [was S1] Version number — FIXED in 541dc62, verified, and my suggested value was wrong. src/EventNtupleMaker_module.cc:638-640 is back to 6 / 12 / 1. I had asked for 6/13/0; matching main is the better answer and I should have checked the convention before prescribing one. This repo bumps the version in a dedicated release commit (cda78eb, 2026-07-06, "Update version number"), the stamped value tracks the latest tag (v06_12_01), and main's own README already forward-references v6.13.0 for the totals histograms while stamping 6.12.1. So the README claim in this PR is the same forward reference, and the release commit will cover both features at once. Reverting to main's value was exactly right.
3. 🟢 [was S1] Track::trkqual_bdt — FIXED in 5cf51fd, verified. The member is gone from rooutil/inc/Track.hh; the only remaining occurrence of the identifier in the repo is the string literal in CompareTrkQualTrainings_UserBranches.C:21, which is the branch name passed to GetUserBranch — i.e. the mechanism this PR was built to use.
4. 🟢 [was S2] Duplicated CheckForBranch — FIXED in 8e45eb7, verified. The member is removed from rooutil/inc/Event.hh; Event.hh:51 still includes BranchUtils.hh, so all ~40 call sites now resolve to the single free inline at BranchUtils.hh:7 that SubRun.hh also uses.
5. 🟡 [was S2] modelVersion conventions — UNADDRESSED, now finding 2.
6. 🟡 [was S2] AGENTS.md — UNADDRESSED, now finding 3.
7. 🟡 [was S2] Nonexistent ONNX files in the compare example — PARTIAL, now finding 4.
8. ⚪ [was S3] Batch — unchanged, now finding 5.
9. 🟠 [was S1] ArtAnalysis#7 sequencing — carried, now finding 1.

---

Verified 🟢 — no action needed

• 🟢 No other structural damage survived the merge. Brace-balance check over every header this PR touches — Event.hh, Track.hh, UserBranch.hh, RooUtil.hh, inc/TrkQualMetadata.hh — returns 0 for all five. The lost brace was the only one.
• 🟢 The rest of the merge is still intact after these four commits. The net contribution against main is unchanged in the twelve files this delta did not touch, and main's work that these hunks sit on top of — trkDtDtTag, the instance-qualified trkPIDTags, the subrun/totals mechanism, SubRun.hh, BranchUtils.hh — is all present and unmodified.
• 🟢 The prior S0 fix from pass two still holds at this head: nTrkQualAlgorithms is counted under the same _conf.trk().fill() gate that writes the bin labels, and the labelling loop remains a subset of the counting loop, so hTrkQualMetadata is never dereferenced null.
• 🟢 The leafname uniqueness check, the fillTrkQual schema removal (verified clean across the Mu2e org last pass), and the leafname : "" backward compatibility are all unchanged and still correct.

Validation check

• Build/tests run: EventNtuple has no CI (no statuses, no check runs at 8e45eb7e). I ran the preprocessor-stripped g++ -fsyntax-only parse check on rooutil/inc/RooUtil.hh — that is a syntax check, not a build, and nothing here has been compiled against ROOT or Offline. Everything else is static, against this head, main @ 84e7404e, and ArtAnalysis#7 @ c0dee801.
• Config contract check: partial — the fhicl schema is coherent; modelVersion (finding 2) is not.
• Cross-repo consistency: needs follow-up — the ArtAnalysis#7 model-filename direction (finding 1). Production and mu2e-trig-config are unaffected.
• Merge quality: the three defects found in 913baba2 are all corrected; the remaining resolution was verified hunk-by-hunk against main last pass and is unchanged.

Residual risk

• No CI still means the next dropped brace ships to whoever builds first. This PR is the second time in two passes that a header-only merge artifact was caught by reading rather than by a machine — a compile-only job over rooutil/examples/ would pay for itself.
• The BDT remains unvalidated in either PR: two quality branches ship with equal standing and no guidance on which analyses should use. A ROC overlay in one of the two PR bodies would close it.
• Until finding 2 is settled, GetTrkQualMetadata plus an explicit comparison is the safer call in analysis code than RequireTrkQualVersion.

Author follow-ups

1. Settle one modelVersion convention and use it in prolog.fcl, both READMEs and the two example configs (finding 2) — this is the one I would like before approving.
2. Fix the AGENTS.md snippet, or reduce the file to a pointer (finding 3).
3. Coordinate the model-filename direction with ArtAnalysis#7 (finding 1); nothing to change here until that is decided.
4. Optional: retarget the compare example at trkqual/trkqual_bdt (finding 4), and the S3 batch (finding 5).

[AndrewEdmonds11]

🟠 [S1] Sequencing with ArtAnalysis#7 — carried over, and the coupling is now on the model filename too.

I think this is a non-issue. Both PRs will go in and be validated before we re-run

🟡 [S2] modelVersion still has four spellings, and both documented examples of the new API throw — carried over, unaddressed.

Fixed

🟡 [S2] AGENTS.md:90 still documents the removed trkQualTags key — carried over.

Fixed

🟡 [S2] from_mcs-mixed_trkQualCompare.fcl still points at ONNX files that do not exist — partially addressed.

Not fixing now. Will fix later

⚪ [S3] Batch, none gating, all carried:

Not fixing these

[oksuzian]

PR Review Summary — fourth pass on "TrkQual BDT" (#381)

Reviewed at head 03f9290c (2026-08-07). Prior reviews: 🔴 at 151e5beb (4879365426), 🔴 at 913baba2 (4885041129), 🟡 at 8e45eb7e (4885210239). The delta is 2 commits, 8e45eb7e..03f9290c, +8/−8 across four files — modelVersion strings and the AGENTS.md snippet. No C++ changed since the last pass.

Decision

• 🟢 Approve. Every blocker raised across three passes is fixed and verified in the code, not taken from the commit messages. What remains is one 🟡 that is half-done and two items you have explicitly declined or deferred — none of which gate this. Approving also clears my two standing CHANGES_REQUESTED, which is the right state for the PR now.
• One condition that is yours to hold, not mine: this still has to land in the same musing build as ArtAnalysis#7, because fcl/prolog.fcl:17 names a model file that only exists on that branch. You have said twice that the plan covers it, and I am taking that as settled — I am not raising it again.

Scope understood (delta only)

• 63fd21f moves prolog.fcl to ANN1_v2.0 / BDT1_v2.0 and corrects the rooutil/README.md example from ANN2_v2 to ANN1_v2.0; fcl/README.md goes to TrkQual_ANN1_v2.0.
• 03f9290 replaces the stale trkQualTags snippet in AGENTS.md with the trkQualLeaves form.

---

Findings

1. 🟡 [S2] modelVersion — partially fixed. The v2 → v2.0 half is done; the TrkQual_ prefix half is not, and one of the two new commits is already stale against the other.

   • The important half landed: rooutil/README.md:143 is now RequireTrkQualVersion("trkqual", "ANN1_v2.0"), which is exactly what fcl/prolog.fcl:277 stamps for trkqual. The one documented call of the new API works on a standard ntuple. That was the sharp edge and it is gone.
   • What is left is a prefix split — three of five places still carry TrkQual_:
     • fcl/prolog.fcl:206,277,278,294,477 → ANN1_v2.0 / BDT1_v2.0
     • fcl/README.md:19 → TrkQual_ANN1_v2.0 for the same TrkQualAll:ANN leaf
     • fcl/from_mcs-reflection.fcl:85,92 → TrkQual_ANN1_v2 (untouched)
     • fcl/from_mcs-mixed_trkQualCompare.fcl:21 → TrkQual_ANN1_v2 for a leaf named _v2_0 whose model file is TrkQual_ANN1_v2.0.onnx
     • AGENTS.md:90 → ANN1_v2
   • Impact, concretely: TrkQualReflecte/TrkQualReflectmu are @table::TrkQual (from_mcs-reflection.fcl:25-32), so they load the same TrkQual_ANN1_v2.0.onnx as TrkQualAll:ANN — but record it as TrkQual_ANN1_v2 instead of ANN1_v2.0. One model, two provenance strings, decided by which shipped fcl produced the file. RequireTrkQualVersion("trkqual", "ANN1_v2.0") copied from the newly-corrected README throws on reflection ntuples.
   • Worth noting for its own sake: 03f9290 ("Fix AGENTS.md") writes modelVersion : "ANN1_v2", which 63fd21f — the commit immediately before it — had already superseded with ANN1_v2.0. A duplicated config example went stale inside the same PR that fixed it, roughly forty minutes apart. That is the argument for reducing AGENTS.md to a pointer at fcl/README.md rather than keeping a second copy of the config in it.
   • Suggested fix: four one-line edits — AGENTS.md:90, fcl/README.md:19, from_mcs-reflection.fcl:85,92, from_mcs-mixed_trkQualCompare.fcl:21 — all to whichever form you want as canonical. Not blocking; the reflection one is the only one with a runtime consequence.

2. ⚪ [S3] Accepted as your call, recorded so the thread is complete: the compare-example ONNX files (deferred — "will fix later", TODO in the file), and the S3 batch from the last pass (declined — snprintf round-trip, TrkQualMetadata_hh guard, PR body's RequireTrkQualMetadata vs RequireTrkQualVersion, per-fit scope of the uniqueness check). No action requested.

---

Carry-forward accounting — full history across four passes

From review 4885210239 at 8e45eb7e:

1. 🟡 [was S2] modelVersion — PARTIAL, finding 1 above.
2. 🟢 [was S2] AGENTS.md trkQualTags — FIXED in 03f9290, verified. AGENTS.md:90 now shows the trkQualLeaves form, so the file no longer documents a key that fails fhicl validation. (Its modelVersion value is stale — finding 1 — but that is a string, not a schema error.)
3. 🟡 [was S2] Compare-example ONNX files — deferred by author, no longer carried as open.
4. ⚪ [was S3] Batch — declined by author, no longer carried as open.
5. 🟠 [was S1] ArtAnalysis#7 sequencing — closed as author's decision. You have stated the plan twice; it is your merge to sequence and I am not re-litigating it. Recording the fact rather than the disagreement: prolog.fcl:17 and from_mcs-mixed_trkQualCompare.fcl:14 name TrkQual_ANN1_v2.0.onnx, which exists on the ArtAnalysis#7 branch and not on ArtAnalysis main.

From review 4885041129 at 913baba2 — all fixed and verified last pass, re-checked at this head:
6. 🟢 [was S0] Missing brace in LoadTrkQualMetadata — fixed in 7dba084. Re-ran the parse check at this head: clean.
7. 🟢 [was S1] Version number rolled backwards — fixed in 541dc62; 6/12/1, matching main and the v06_12_01 tag.
8. 🟢 [was S1] Track::trkqual_bdt dangling member — removed in 5cf51fd.
9. 🟢 [was S2] Duplicated CheckForBranch — removed in 8e45eb7.

From review 4879365426 at 151e5beb:
10. 🟢 [was S0] trk.fill : false producing unlabelled metadata bins — fixed in b4702c05, verified, and still correct at this head.
11. 🟢 [was S2] Duplicate leafname values went unvalidated — fixed in 925985bb; construction now throws naming the offending output branch.
12. 🟢 [was S3] Dead trk.fillTrkQual fhicl key — removed in 22c26c17, with all three configs that set it cleaned up in the same commit and no external consumer anywhere in the Mu2e org.

---

Verified 🟢 — no action needed

• 🟢 No C++ changed since the last pass. git diff --name-only 8e45eb7e 03f9290c returns only AGENTS.md, fcl/README.md, fcl/prolog.fcl, rooutil/README.md, so everything verified at 8e45eb7e stands unchanged. Re-ran the RooUtil.hh parse check anyway: clean.
• 🟢 The feature is coherent end to end. trkQualLeaves names the branches, trkqual_metadata records provenance per branch, checkEventNtuple prints it, LoadTrkQualMetadata reads it back and rejects a chain whose files disagree, and RequireTrkQualVersion asserts on it — with the README example now matching what the default config actually stamps.
• 🟢 Backward compatibility holds. leafname : "" still yields trkqual, byte-identical to the old name, so existing analysis code is untouched by the schema change; checkEventNtuple and LoadTrkQualMetadata both no-op on files without the histogram.
• 🟢 The merge with main is clean in substance, verified hunk-by-hunk two passes ago and unchanged since: trkDtDtTag, the instance-qualified trkPIDTags, the subrun/totals mechanism, SubRun.hh and BranchUtils.hh are all present and unmodified by this branch.
• 🟢 Retiring trkqual_alt/trkqual3 for UserBranch is a genuine simplification and now actually complete — three special cases gone from Event.hh, the hardcoded member gone from Track.hh, and a working example shipped in the same PR. This resolves #306.

Validation check

• Build/tests run: EventNtuple has no CI (no statuses, no check runs at 03f9290c). Across the four passes the only machine check I ran was a preprocessor-stripped g++ -fsyntax-only parse of rooutil/inc/RooUtil.hh — a syntax check, not a build. Nothing here has been compiled against ROOT or Offline, and no ntuple has been produced or read. Everything else is static, against this head, main @ 84e7404e, and ArtAnalysis#7 @ c0dee801.
• Config contract check: pass — the fhicl schema is coherent, the fillTrkQual removal is clean org-wide, and the metadata histogram is consistent with its own reader. The residual modelVersion inconsistency is a value, not a contract.
• Cross-repo consistency: by plan — consistent with ArtAnalysis#7 as it stands; merge sequencing is the author's stated plan. Production and mu2e-trig-config are unaffected.

Residual risk

• Reflection ntuples will record a different provenance string for the same model than everything else (finding 1). Cheap to fix now, awkward to fix once files exist.
• The BDT itself is still unvalidated in either PR — two quality branches ship with equal standing and no guidance on which analyses should use. Not a review finding, but it is the open physics question behind the whole pair; a ROC overlay in one PR body would close it.
• No CI means the next header-only breakage ships to whoever builds first. Two of the four passes on this PR found defects a compile-only job over rooutil/examples/ would have caught in seconds. That is a repo-level gap, not this PR's problem, but this PR is decent evidence for it.

Author follow-ups (none blocking)

1. The four modelVersion one-liners, from_mcs-reflection.fcl:85,92 first (finding 1).
2. Consider reducing AGENTS.md to a pointer at fcl/README.md — the stale value it shipped with today is a good argument for not keeping a second copy of the config.
3. Fix the PR body's util.RequireTrkQualMetadata → RequireTrkQualVersion before merge, since the body becomes the squash-commit message.

Four passes in a day is a fast loop, and worth saying: every blocker I raised was real and every fix for one landed correctly. The two places I was wrong — prescribing 6/13/0 for the version, and the include-guard remark that sent you at the wrong file — cost you edits you should not have had to make.