Skip to content

fix: GeoSvc requires compact files in standalone mode - #38

Merged
wdconinc merged 1 commit into
mainfrom
claude/fix-geosvc-init
Oct 29, 2026
Merged

wdconinc merged 1 commit into
mainfrom
claude/fix-geosvc-init

Conversation

@wdconinc

Copy link
Copy Markdown
Contributor

Stacked on #37.

Summary

  • Silent empty geometry. GeoSvc::init() without an external detector and with an empty detectors property created an empty detector without a valid world volume (the TODO handle error branch). It now raises a ServiceError, so ServiceSvc::init() reports the service as not initialized. Initializing with an external detector, as EICrecon does, is unaffected.
  • Log message. GeoSvc::init() logged the literal Loading compact file: name; it now logs the file name.

Tests

Both [!shouldfail] GeoSvc tests from #34 are now regular tests: the empty detectors property raises a ServiceError, and the loaded compact file is logged by name.

Test plan

  • clang++ and g++ Debug with CI warning flags in eic_xl:nightly: 84/84 tests pass
  • CI

🤖 Generated with Claude Code

@wdconinc wdconinc mentioned this pull request Oct 28, 2026
1 of 2 tasks
@wdconinc
wdconinc requested a balanced review from Copilot October 28, 2026 23:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The reviewer verified the full diff and surrounding context—confirming the ServiceError raise resolves and is caught correctly, the error and log messages match the tests exactly, and the detector creation reorder is sound—without finding any objective issues to flag.

No open issues 🎉

What changed in this PR

This PR fixes two latent GeoSvc bugs surfaced by the [!shouldfail] tests added in #34. When self-initializing (standalone mode, no external detector) with an empty detectors property, GeoSvc::init() previously hit a // TODO handle error branch and silently built an empty detector with no valid world volume; it now raises a ServiceError so ServiceSvc::init() correctly reports the service as uninitialized. It also fixes a logging bug where the literal string "name" was logged instead of the actual compact file name. The two corresponding tests are promoted from [!shouldfail] to regular tests.

Changes:

  • Raise ServiceError instead of silently proceeding when standalone mode has no compact files, and create the detector only after that check.
  • Log the actual compact file name instead of the literal "name".
  • Convert the two [!shouldfail] GeoSvc tests into regular passing tests asserting the thrown error and the log output.
File Description
core/​src/​geo.cpp Raises ServiceError on empty detectors in standalone mode and logs the real compact file name; reorders detector creation after the guard.
core/​tests/​geo.cpp Promotes the two previously-failing tests to regular tests verifying the ServiceError type/message and the per-file log line; adds the Catch2 string matchers include.

Review effort: Balanced

@wdconinc
wdconinc force-pushed the claude/fix-property-helpers branch from cf6d92f to 8f91293 Compare October 28, 2026 23:39
@wdconinc
wdconinc changed the base branch from claude/fix-property-helpers to main October 28, 2026 23:59
GeoSvc::init without an external detector and with an empty 'detectors'
property silently created an empty detector without a valid world volume
(the "TODO handle error" branch). It now raises a ServiceError, so
ServiceSvc::init reports the service as not initialized.

Also log the name of each compact file that is loaded, instead of the
literal string "name".

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@wdconinc
wdconinc force-pushed the claude/fix-geosvc-init branch from d781b8b to 35fe9e7 Compare October 28, 2026 23:59
@wdconinc
wdconinc merged commit 39fd675 into main Oct 29, 2026
6 checks passed
@wdconinc
wdconinc deleted the claude/fix-geosvc-init branch October 29, 2026 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants