C++ review#5
Closed
TheDaniel166 wants to merge 39 commits into
Closed
Conversation
…asteroid oracle, and planetary ephemeris utilities
…ze astronomical calculations
…tive backend and update documentation index
asteriod sovereign implementation
Surmado Code Review — ErrorDiff fetch failed: Sorry, the diff exceeded the maximum number of lines (20000): {"resource":"PullRequest","field":"diff","code":"too_large"} - https://docs.github.com/rest/pulls/pulls#get-a-pull-request Review could not run. Surmado Code Review (v1.2-mt) |
|
Too many files changed for review. ( |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
moira | adb5b2b | May 17 2026, 10:17 AM |
✅ Deploy Preview for heroic-custard-7ea345 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Review Summary by QodoNative C++ backend integration, NumPy elimination, and context-aware reader architecture refactoring
WalkthroughsDescription**Major architectural refactoring to eliminate NumPy dependency and implement native C++ backend integration:** • **Native SPK reader infrastructure**: Replaced jplephem dependency with native C++ Chebyshev polynomial evaluation and DAF catalog reading via moira_native module, with jplephem fallback for unsupported segment types • **Context-aware reader management**: Refactored module-level singleton pattern to ContextVar-based reader management with get_active_reader(), swap_reader(), and add_to_global_pool() for flexible kernel lifecycle • **NumPy elimination across core modules**: Removed NumPy dependencies from planets.py, lunar_limb.py, daf_writer.py, astrocartography.py, and corrections.py using native substrate and pure-Python implementations • **Caching and performance optimization**: Implemented LRU caches for apparent-path computations, STAC tile caching for LOLA data, and fast-path optimization for default geocentric ecliptic mode • **Small-body kernel migration**: Migrated _spk_body_kernel.py to native C++ backend with sovereign shard manifest support and Type-13 segment handling • **Asteroid and comet module refactoring**: Updated to use context-aware reader pattern with optional reader parameter instead of mandatory kernel paths • **Lunar limb enhancements**: Added COPC region queries, graceful optional dependency handling, and native vector operations for point-cloud processing • **New transit methods**: Added aspect_transits() and declination_transits() to predictive facade for aspect and declination-based transit finding • **Extended test coverage**: Added comprehensive test suites for native SPK evaluation, property-based LOLA point-cloud tests, Sothic cycle validation, and stress tests for small-body kernels • **Diagnostic tooling**: New profiling and benchmarking scripts for planetary flow bottleneck analysis and native backend validation • **Removed legacy modules**: Deleted solar_cartography.py and lunar_cartography.py with associated tests • **Version bump**: Updated to version 3.1.0 Diagramflowchart LR
A["Legacy jplephem<br/>+ NumPy"] -->|"Native SPK reader<br/>+ ContextVar"| B["Native C++ backend<br/>moira_native"]
A -->|"Pure Python<br/>array.array"| C["Simplified<br/>serialization"]
B -->|"Chebyshev eval<br/>DAF catalog"| D["High-performance<br/>kernel access"]
D -->|"LRU caching<br/>context mgmt"| E["Optimized<br/>planetary flow"]
F["Optional deps<br/>requests/laspy"] -->|"Graceful fallback<br/>_require_lunar_extra"| G["LOLA/COPC<br/>integration"]
G -->|"Native vectors<br/>no NumPy"| H["Lunar limb<br/>computation"]
I["Singleton pattern<br/>get_reader"] -->|"ContextVar override<br/>get_active_reader"| J["Flexible reader<br/>lifecycle"]
J -->|"add_to_global_pool<br/>swap_reader"| K["Multi-kernel<br/>support"]
File Changes1. moira/planets.py
|
Code Review by Qodo
1. Hardcoded Windows CMake paths
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a7b593c4f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
100
to
102
| planets = facade.all_planets_at( | ||
| jd, | ||
| jd_tt, | ||
| bodies=bodies, |
There was a problem hiding this comment.
all_planets_at() expects a UT/UT1 Julian date and already performs its own ut_to_tt() conversion internally, but this call now passes jd_tt. That applies the UTC→TT offset twice, shifting every planet calculation in chart() by roughly another ΔT (tens of seconds), which is large enough to skew fast-moving bodies and downstream chart outputs.
Useful? React with 👍 / 👎.
Foyer PR Review
Verdict: Needs changes before merging — 2 issues that need attention Intent source: PR description (medium confidence)
⚪ Did it do what was asked ❌ Code quality — 2 issues that need attention
✅ Security check What to address first: Repair packets (for Claude Code / Codex / Cursor / Windsurf)Code review: DiagnosisThe PR’s de‑singleton refactor replaced Fix plan
Acceptance checkRun the exact command that produced the failure signal (the original code‑review check): Alternatively, execute the specific test scenarios that validate the reported bugs:
Context
Coverage: 3 of 9 signals (reduced for Python)Foyer's tests, build, dead-code, dependency-hygiene, hallucinated-imports, template-cruft, plan-fidelity, deploy-build, and browser-smoke signals require JavaScript/TypeScript tooling. Native support for Python is on our roadmap. For a fully verified PR on Python, watch this space or reach out at hello@foyer.dev. |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+7
to
+34
| # Explicitly include Python and pybind11 directories to bypass config failures | ||
| include_directories("C:/Python314/Include") | ||
| include_directories("C:/Users/nilad/AppData/Roaming/Python/Python314/site-packages/pybind11/include") | ||
| include_directories(src/native/include) | ||
|
|
||
| # Define the extension module manually under a private backend name. | ||
| add_library(_moira_native MODULE | ||
| src/native/bindings/moira_native.cpp | ||
| src/native/src/lola.cpp | ||
| ) | ||
|
|
||
| # Link against Python library | ||
| target_link_libraries(_moira_native PRIVATE "C:/Python314/libs/python314.lib") | ||
|
|
||
| # Set target properties to match Python's extension requirements | ||
| set_target_properties(_moira_native PROPERTIES | ||
| PREFIX "" | ||
| SUFFIX ".pyd" | ||
| OUTPUT_NAME "_moira_native" | ||
| LIBRARY_OUTPUT_DIRECTORY "${CMAKE_SOURCE_DIR}/moira" | ||
| RUNTIME_OUTPUT_DIRECTORY "${CMAKE_SOURCE_DIR}/moira" | ||
| LIBRARY_OUTPUT_DIRECTORY_RELEASE "${CMAKE_SOURCE_DIR}/moira" | ||
| RUNTIME_OUTPUT_DIRECTORY_RELEASE "${CMAKE_SOURCE_DIR}/moira" | ||
| LIBRARY_OUTPUT_DIRECTORY_DEBUG "${CMAKE_SOURCE_DIR}/moira" | ||
| RUNTIME_OUTPUT_DIRECTORY_DEBUG "${CMAKE_SOURCE_DIR}/moira" | ||
| CXX_VISIBILITY_PRESET hidden | ||
| VISIBILITY_INLINES_HIDDEN ON | ||
| ) |
There was a problem hiding this comment.
1. Hardcoded windows cmake paths 🐞 Bug ☼ Reliability
CMakeLists.txt hardcodes absolute Windows include/library paths and forces a ".pyd" suffix, which will break builds on other machines/OSes and in CI. It also bakes a specific local user path into the build configuration.
Agent Prompt
### Issue description
`CMakeLists.txt` hardcodes developer-specific Windows paths for Python/pybind11 and forces the extension suffix to `.pyd`, making the build non-portable and likely to fail in CI or on non-Windows platforms.
### Issue Context
The native extension should be built using CMake’s Python discovery and pybind11 tooling (or equivalent), with platform-correct extension suffixes and no absolute user paths.
### Fix Focus Areas
- CMakeLists.txt[7-34]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+960
to
+1036
| void solar_cartography_grid_sweep_py( | ||
| const IEvaluator& sun, | ||
| const IEvaluator& moon, | ||
| py::array_t<double> jds, | ||
| py::array_t<double> gasts_deg, | ||
| py::array_t<double> lats_deg, | ||
| py::array_t<double> lons_deg, | ||
| double sun_radius_km, | ||
| double moon_radius_km, | ||
| py::array_t<double> overlap_max, | ||
| py::array_t<double> central_max, | ||
| py::array_t<double> magnitude_max | ||
| ) { | ||
| auto jd_ptr = jds.data(); | ||
| auto gast_ptr = gasts_deg.data(); | ||
| auto lat_ptr = lats_deg.data(); | ||
| auto lon_ptr = lons_deg.data(); | ||
| auto overlap_ptr = overlap_max.mutable_data(); | ||
| auto central_ptr = central_max.mutable_data(); | ||
| auto magnitude_ptr = magnitude_max.mutable_data(); | ||
|
|
||
| solar_cartography_grid_sweep( | ||
| sun, moon, jd_ptr, gast_ptr, jds.size(), | ||
| lat_ptr, lon_ptr, lats_deg.size(), | ||
| sun_radius_km, moon_radius_km, | ||
| overlap_ptr, central_ptr, magnitude_ptr | ||
| ); | ||
| } | ||
|
|
||
| void lunar_cartography_grid_sweep_py( | ||
| const IEvaluator& sun, | ||
| const IEvaluator& moon, | ||
| py::array_t<double> jds, | ||
| py::array_t<double> gasts_deg, | ||
| py::array_t<double> magnitudes_base, | ||
| py::array_t<double> lats_deg, | ||
| py::array_t<double> lons_deg, | ||
| py::array_t<double> penumbral_max, | ||
| py::array_t<double> partial_max, | ||
| py::array_t<double> total_max, | ||
| py::array_t<double> magnitude_max, | ||
| py::object u1_u4_obj, | ||
| py::object u2_u3_obj | ||
| ) { | ||
| auto jd_ptr = jds.data(); | ||
| auto gast_ptr = gasts_deg.data(); | ||
| auto mag_base_ptr = magnitudes_base.data(); | ||
| auto lat_ptr = lats_deg.data(); | ||
| auto lon_ptr = lons_deg.data(); | ||
| auto pen_ptr = penumbral_max.mutable_data(); | ||
| auto par_ptr = partial_max.mutable_data(); | ||
| auto tot_ptr = total_max.mutable_data(); | ||
| auto mag_ptr = magnitude_max.mutable_data(); | ||
|
|
||
| double u1_u4[2], u2_u3[2]; | ||
| double* p_u1_u4 = nullptr; | ||
| double* p_u2_u3 = nullptr; | ||
|
|
||
| if (!u1_u4_obj.is_none()) { | ||
| auto arr = u1_u4_obj.cast<py::array_t<double>>(); | ||
| u1_u4[0] = arr.at(0); | ||
| u1_u4[1] = arr.at(1); | ||
| p_u1_u4 = u1_u4; | ||
| } | ||
| if (!u2_u3_obj.is_none()) { | ||
| auto arr = u2_u3_obj.cast<py::array_t<double>>(); | ||
| u2_u3[0] = arr.at(0); | ||
| u2_u3[1] = arr.at(1); | ||
| p_u2_u3 = u2_u3; | ||
| } | ||
|
|
||
| lunar_cartography_grid_sweep( | ||
| sun, moon, jd_ptr, gast_ptr, mag_base_ptr, jds.size(), | ||
| lat_ptr, lon_ptr, lats_deg.size(), | ||
| pen_ptr, par_ptr, tot_ptr, mag_ptr, | ||
| p_u1_u4, p_u2_u3 | ||
| ); |
There was a problem hiding this comment.
2. Cartography binding oob risk 🐞 Bug ⛨ Security
solar_cartography_grid_sweep_py and lunar_cartography_grid_sweep_py pass raw pointers to native routines using lats_deg.size()/jds.size() without validating that lons_deg and output arrays have matching sizes, enabling out-of-bounds reads/writes and crashes on mismatched inputs. The vectors variant in the same file does validate lengths, indicating the scalar variant is missing required checks.
Agent Prompt
### Issue description
The pybind entrypoints `solar_cartography_grid_sweep_py` and `lunar_cartography_grid_sweep_py` assume input/output NumPy arrays are compatible in length and then use raw pointers. If a caller passes arrays of different lengths (e.g., `lats_deg` and `lons_deg` differ), native code may read/write past buffer bounds.
### Issue Context
The `*_vectors_py` variants already check dimensionality and length consistency; the non-vector variants should enforce similar validation (ndim==1, equal lengths for paired arrays, and output buffers sized to the expected point count).
### Fix Focus Areas
- src/native/bindings/moira_native.cpp[960-1036]
- src/native/bindings/moira_native.cpp[1119-1124]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+62
to
+63
| if __name__ == "__main__": | ||
| app.run(port=5000, debug=True) |
There was a problem hiding this comment.
3. Flask debug mode enabled 🐞 Bug ⛨ Security
app.py runs Flask with debug=True, which enables the interactive debugger and can allow remote code execution if the service is reachable from an untrusted network. This should not be enabled by default in a checked-in entrypoint.
Agent Prompt
### Issue description
The Flask app is launched with `debug=True`, which is unsafe if the server is ever exposed beyond localhost.
### Issue Context
Debug mode should be controlled via environment/config and default to off.
### Fix Focus Areas
- app.py[62-63]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
transits_aspects and transits_equatorial used a hard top-level import of moira_native, unlike every other module which wraps the import in try/except ImportError. This caused the entire moira import chain to fail in CI where the C++ extension is absent, breaking tests that don't use native functionality at all. Also replaced mn.IEvaluator | None return annotations (evaluated eagerly at import time) with object | None. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
…ce into a deferred annotation:
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
1 participant
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.
Summary
Describe the problem this pull request solves and the concrete change it introduces.
Why this approach
Explain why this implementation was chosen over obvious alternatives.
Change surface
Affected area
List the modules, subsystems, or public surfaces touched by this PR.
Validation
Sources or references
List the standards, papers, datasets, doctrine sources, or prior issues that justify the change.
Review notes
Call out anything that deserves careful review, including edge cases, risks, or intentional non-goals.
Checklist