CalPatRec code review: 26 issues identified across the package by Copilot · Pull Request #1788 · Mu2e/Offline

Copilot · 2026-04-08T01:11:27Z

Review feedback addressed:

Restore "update time" comment in DeltaCandidate::removeSeed()
Revert ReplaceFirstHit fSumT and coordinate accumulator changes in DeltaSeed.cc
Restore fNSeeds > 1 guard in MergeDeltaCandidate (alongside denom != 0 check)

Not fixed (by design):

Fix Implement configuration validation for CollectionSizeAnalyzer. #8, Configuration validation #11, fixed bug in calHelixFinder that was creating a bias in the reconstructed Lambda. + added sequences to implement the fast-track based trigger lines, which essentially uses a skimmed collection of ComboHits; the skimming process uses the calorimeter clusters above 50 MeV to set a time window of about 100 ns (this is necessary to allow the delta-ray removal algorithm to have an optimal identification efficiency). #18, fixed extMonSD code to fix bug. moved extMonSD ownership and registr… #20, fix output file typo #22-24, Trkdiag cleanup #26 — see original PR description

…e package Agent-Logs-Url: https://github.com/Mu2e/Offline/sessions/23aa289b-bce0-4124-9da6-e052cee9af45 Co-authored-by: oksuzian <48927306+oksuzian@users.noreply.github.com>

FNALbuild · 2026-04-08T01:11:58Z

Hi @Copilot,
You have proposed changes to files in these packages:

CalPatRec

which require these tests: build.

@Mu2e/fnalbuild-users, @Mu2e/write have access to CI actions on main.

📝 The author of this pull request is not a member of the Mu2e github organisation.

About FNALbuild. Code review on Mu2e/Offline.

Agent-Logs-Url: https://github.com/Mu2e/Offline/sessions/23aa289b-bce0-4124-9da6-e052cee9af45 Co-authored-by: oksuzian <48927306+oksuzian@users.noreply.github.com>

…stead Agent-Logs-Url: https://github.com/Mu2e/Offline/sessions/4376776e-355e-4158-8204-38fdf6998859 Co-authored-by: oksuzian <48927306+oksuzian@users.noreply.github.com>

- Fix #1: Buffer overflow in DeltaCandidate::removeSeed() - add bounds checks - Fix #2: Stale facez pointer in CalHelixFinderAlg - set to ibest.face - Fix #3: Division-by-zero guards in DeltaSeed/DeltaCandidate/ProtonCandidate accessors - Fix #4: Guard zero determinant in DeltaSeed::CalculateCogAndChi2 and DeltaCandidate - Fix #5: Guard rho==0 in DeltaCandidate direction calculation - Fix #6: Null pointer dereference in PrefetchData_module - check _shpcol - Fix #7: Division by zero in PhiClusterFinder::initCluster() - guard weight - Fix #9: Memory leak - use unique_ptr for DeltaFinderAlg in DeltaFinder_module - Fix #10: Out-of-bounds in DeltaFinderAlg - skip invalid indices instead of continuing - Fix #11: Update coordinate accumulators in DeltaSeed::ReplaceFirstHit() - Fix #12: Guard division by zero in time-vs-z fit (MergeDeltaCandidate, ProtonCandidate) - Fix #13: Guard weight==0 in CalHelixFinderAlg phi weight calculations - Fix #14: Guard _dfdz==0 in CalHelixFinderAlg helix calculations - Fix #15: Early return on radius==0 or _dfdz==0 in defineHelixParams - Fix #16: Guard timeVar()==0 in TZClusterFinder - Fix #17: Clean up TCanvas in TZClusterFinder destructor, init _c1 to nullptr - Fix #19: Add indexArray bounds check in ComboHitFilter - Fix #21: Use nullptr instead of 0 in CalHelixFinder_module error handling - Fix #25: Replace NULL with nullptr throughout CalPatRec Agent-Logs-Url: https://github.com/Mu2e/Offline/sessions/f71e1120-4e72-45c3-9734-76ea0561201d Co-authored-by: oksuzian <48927306+oksuzian@users.noreply.github.com>

oksuzian · 2026-04-08T04:11:30Z

@FNALbuild run build test

FNALbuild · 2026-04-08T04:11:59Z

⌛ The following tests have been triggered for 6e09f2c: build (Build queue - API unavailable)

FNALbuild · 2026-04-08T04:31:45Z

☀️ The build tests passed at 6e09f2c.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged `6e09f2c` at `5ec09f9`
build (prof)	✅	Log file. Build time: 08 min 24 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	❌	Log file. Return Code 2.
check_cmake	✅	Log file.
FIXME, TODO	➡️	TODO (2) FIXME (45) in 15 files
clang-tidy	➡️	19 errors 1215 warnings
whitespace check	➡️	found whitespace errors

N.B. These results were obtained from a build of this Pull Request at 6e09f2c after being merged into the base branch at 5ec09f9.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

michaelmackenzie

Mostly this looks okay to me, a large part is either NULL to nullptr (transparent) or checking for cases that should be impossible or caught earlier (combo hits with no hits, 0 radius helices, etc.). There are a few logic changes that should be better understood and confirmed by original authors if possible.

michaelmackenzie · 2026-04-08T13:31:42Z

-    double rho = sqrt(xc*xc+yc*yc);
-    fNx        = xc/rho;
-    fNy        = yc/rho;
-//-----------------------------------------------------------------------------


Please restore this comment

@copilot please address this comment

Restored in 307a15d.

michaelmackenzie · 2026-04-08T13:34:10Z

+      fT0    = tm-fDtDz*zm;
+      fSigT0 = sqrt((t2m-tm*tm)/(fNSeeds-0.9999));
+    }
+    else {


Does this change behavior for fNSeeds == 1? Is fNSeeds == 1 not allowed?

You're right — the original code didn't have an fNSeeds > 1 guard here (unlike removeSeed and AddSeed). MergeDeltaCandidate should always produce fNSeeds >= 2 after merging, so the extra guard was unnecessary and changed behavior. Reverted to only guard denom != 0 in 307a15d.

michaelmackenzie · 2026-04-08T13:35:53Z

+          if ((of < 0) || (of >= kNFaces        )) printf(" >>> ERROR: wrong face    number: %i\n",of);
+          if ((op < 0) || (op >= kNPanelsPerFace)) printf(" >>> ERROR: wrong panel   number: %i\n",op);
+        }
+        continue;


Is the behavior fine when continuing from here, which wasn't done before?

The continue skips adding this single hit to the face data structures when the station/face/panel indices are out of range. Previously the code would print the error but then proceed to use the invalid indices for array access (_data->fFaceData[os][of]), which is undefined behavior. The continue prevents the out-of-bounds access and just drops the problematic hit, which should be safe since these errors indicate corrupted input data that shouldn't be processed anyway.

michaelmackenzie · 2026-04-08T13:36:51Z


      fSumEDep            = Hd->fHit->energyDep()*Hd->fHit->nStrawHits();
-      fSumT               = Hd->fCorrTime*Hd->fHit->nStrawHits();
+      fSumT               = Hd->fCorrTime;


Why remove the N(straws) weight to the summed time? How are these fields used?

Reverted in 307a15d. The nStrawHits() weighting is consistent with how fSumEDep is calculated on the line above, and TMean() divides by fNHits (combo hits, not straw hits), so the original weighting may be intentional. This needs confirmation from the original author.

michaelmackenzie · 2026-04-08T13:40:08Z

+//-----------------------------------------------------------------------------
+// also update the coordinate-sum accumulators
+//-----------------------------------------------------------------------------
+      fSnx2               = Hd->fNx2;


Were these missing in previous evaluations or are they not currently used from these values?

Reverted in 307a15d. These accumulators were not previously set in ReplaceFirstHit, and ReplaceFirstHit is only called when replacing the first hit in the same face of a seed that was just initialized via Init(). The coordinate sums from Init() would already include the original hit's contribution. Whether the stale values cause issues in subsequent CalculateCogAndChi2 calls needs confirmation from the original author.

…laceFirstHit and MergeDeltaCandidate - Restore "update time" section comment in DeltaCandidate::removeSeed() - Revert fNSeeds>1 guard in MergeDeltaCandidate (keep only denom!=0 guard) - Revert fSumT change in ReplaceFirstHit (restore nStrawHits weighting) - Revert coordinate accumulator additions in ReplaceFirstHit Agent-Logs-Url: https://github.com/Mu2e/Offline/sessions/17a94fec-a18a-41e3-9e62-a7b8916ccccd Co-authored-by: michaelmackenzie <30634463+michaelmackenzie@users.noreply.github.com>

Copilot · 2026-04-08T13:54:07Z

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

buildmaster.fnal.gov
- Triggering command: /home/REDACTED/work/_temp/ghcca-node/node/bin/node /home/REDACTED/work/_temp/ghcca-node/node/bin/node --enable-source-maps /home/REDACTED/work/_temp/copilot-developer-action-main/dist/index.js (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Configure Actions setup steps to set up my environment, which run before the firewall is enabled
Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

michaelmackenzie · 2026-04-08T13:56:46Z

@FNALbuild run build test

FNALbuild · 2026-04-08T13:57:10Z

⌛ The following tests have been triggered for 307a15d: build (Build queue - API unavailable)

FNALbuild · 2026-04-08T14:09:16Z

☀️ The build tests passed at 307a15d.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged `307a15d` at `fa82e62`
build (prof)	✅	Log file. Build time: 04 min 10 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	❓	Log file. Return Code 1.
check_cmake	✅	Log file.
FIXME, TODO	➡️	TODO (2) FIXME (45) in 15 files
clang-tidy	➡️	19 errors 1216 warnings
whitespace check	➡️	found whitespace errors

N.B. These results were obtained from a build of this Pull Request at 307a15d after being merged into the base branch at fa82e62.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

michaelmackenzie

I think this looks okay at this point.

… check Agent-Logs-Url: https://github.com/Mu2e/Offline/sessions/8bc3b260-e547-44b2-a226-1f6a86376acc Co-authored-by: oksuzian <48927306+oksuzian@users.noreply.github.com>

Add CalPatRec code review document covering 26 issues found across th…

0bf973a

…e package Agent-Logs-Url: https://github.com/Mu2e/Offline/sessions/23aa289b-bce0-4124-9da6-e052cee9af45 Co-authored-by: oksuzian <48927306+oksuzian@users.noreply.github.com>

Copilot AI assigned Copilot and oksuzian Apr 8, 2026

FNALbuild added the build pending label Apr 8, 2026

Replace non-ASCII arrow symbol with ASCII equivalent in code review doc

0decbd1

Agent-Logs-Url: https://github.com/Mu2e/Offline/sessions/23aa289b-bce0-4124-9da6-e052cee9af45 Co-authored-by: oksuzian <48927306+oksuzian@users.noreply.github.com>

Copilot created this pull request from a session on behalf of oksuzian April 8, 2026 01:12 View session

Copilot AI requested a review from oksuzian April 8, 2026 01:12

Copilot finished work on behalf of oksuzian April 8, 2026 01:12

Copilot started work on behalf of oksuzian April 8, 2026 01:18 View session

Remove code_review.md from repo - review will be in PR description in…

7549a72

…stead Agent-Logs-Url: https://github.com/Mu2e/Offline/sessions/4376776e-355e-4158-8204-38fdf6998859 Co-authored-by: oksuzian <48927306+oksuzian@users.noreply.github.com>

Copilot finished work on behalf of oksuzian April 8, 2026 01:20

Copilot started work on behalf of oksuzian April 8, 2026 01:22 View session

oksuzian requested a review from michaelmackenzie April 8, 2026 01:30

Copilot finished work on behalf of oksuzian April 8, 2026 01:35

oksuzian removed their request for review April 8, 2026 04:11

FNALbuild removed the build pending label Apr 8, 2026

FNALbuild added build pending build running and removed build pending labels Apr 8, 2026

FNALbuild added build finished and removed build running labels Apr 8, 2026

michaelmackenzie reviewed Apr 8, 2026

View reviewed changes

Copilot started work on behalf of michaelmackenzie April 8, 2026 13:46 View session

Copilot AI requested a review from michaelmackenzie April 8, 2026 13:54

Copilot finished work on behalf of michaelmackenzie April 8, 2026 13:54

FNALbuild removed the build finished label Apr 8, 2026

FNALbuild added build pending build running and removed build pending labels Apr 8, 2026

FNALbuild added build finished and removed build running labels Apr 8, 2026

oksuzian marked this pull request as ready for review April 8, 2026 15:08

michaelmackenzie approved these changes Apr 11, 2026

View reviewed changes

Copilot started work on behalf of oksuzian April 12, 2026 02:15 View session

Restore fNSeeds > 1 guard in MergeDeltaCandidate alongside denom != 0…

8d6735d

… check Agent-Logs-Url: https://github.com/Mu2e/Offline/sessions/8bc3b260-e547-44b2-a226-1f6a86376acc Co-authored-by: oksuzian <48927306+oksuzian@users.noreply.github.com>

Copilot AI requested a review from oksuzian April 12, 2026 02:19

Copilot finished work on behalf of oksuzian April 12, 2026 02:19

oksuzian merged commit 43949a2 into main Apr 12, 2026

michaelmackenzie deleted the copilot/review-calpatrec-find-issues branch April 13, 2026 17:23

Conversation

Copilot AI commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FNALbuild commented Apr 8, 2026

Uh oh!

oksuzian commented Apr 8, 2026

Uh oh!

FNALbuild commented Apr 8, 2026

Uh oh!

FNALbuild commented Apr 8, 2026

Uh oh!

michaelmackenzie left a comment

Choose a reason for hiding this comment

Uh oh!

michaelmackenzie Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

michaelmackenzie Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

michaelmackenzie Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

michaelmackenzie Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

michaelmackenzie Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

michaelmackenzie Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Apr 8, 2026

I tried to connect to the following addresses, but was blocked by firewall rules:

Uh oh!

michaelmackenzie commented Apr 8, 2026

Uh oh!

FNALbuild commented Apr 8, 2026

Uh oh!

FNALbuild commented Apr 8, 2026

Uh oh!

michaelmackenzie left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Copilot AI commented Apr 8, 2026 •

edited

Loading