feat(bazi): expose immutable Shen Sha catalogs - #15
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 5ff0348d96
ℹ️ 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".
| add_library(taiyin_bazi_python_rules STATIC | ||
| "${_TAIYIN_CORE_SOURCE_DIR}/bazi_astrology/src/bazi.cpp" | ||
| "${_TAIYIN_CORE_SOURCE_DIR}/bazi_astrology/src/bazi_rules.cpp" | ||
| "${_TAIYIN_CORE_SOURCE_DIR}/bazi_astrology/src/shen_sha_catalog.cpp" |
There was a problem hiding this comment.
In builds without the unpublished adjacent development checkout—including the wheel workflows, which resolve v1.0.0-beta.11 from this CMake file—the pinned core does not contain shen_sha_catalog.cpp or its header. Adding this source therefore makes every BaZi wheel build and isolated sdist installation fail before compilation can complete; publish and pin the compatible core revision and checksum before landing this change.
Useful? React with 👍 / 👎.
| py::dict rule = py::cast<py::dict>(item); | ||
| py::function callback = rule["test"].cast<py::function>(); | ||
| values.push_back({rule["id"].cast<std::string>(), rule["name"].cast<std::string>(), | ||
| [callback](const taiyin::bazi::BaziShenShaInput& input) { |
There was a problem hiding this comment.
When a predicate closes over its BaziShenShaContext—or is a bound method on an owner that stores the context—this strong native capture forms context -> native snapshot -> callback -> context. Pybind11 cannot expose the py::function held inside this C++ value to Python's cycle collector, and the new wrapper provides no close() operation, so dropping the context permanently retains the callback and everything it captures; use a cycle-safe ownership strategy or provide an explicit way to release the native callback references.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 513d0aa5b7
ℹ️ 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".
| ) | ||
| # Public immutable post-beta.11 revision that first includes Shen Sha catalogs. | ||
| # Use a commit pin until the next core release tag is available. | ||
| set(TAIYIN_CORE_REVISION "df5cedcc90522cf990ee50f1991f74272a79f27b") |
There was a problem hiding this comment.
Both .github/workflows/build-distributions.yml and .github/workflows/release-pypi.yml run .github/scripts/core_pin.py before checking out the C++ sources, but that resolver requires every package CMake file to define both TAIYIN_CORE_REVISION and TAIYIN_CORE_ARCHIVE_SHA256 and requires their pairs to match. Replacing only the BaZi pair with this lone post-beta commit makes the resolver immediately raise ValueError: packages/taiyin-bazi/CMakeLists.txt: expected exactly one literal TAIYIN_CORE_ARCHIVE_SHA256, so all wheel distribution and release jobs stop before building; update the shared resolver/workflows for per-package pins or advance the shared pin in lockstep.
Useful? React with 👍 / 👎.
|
@codex review Fixed the distribution-resolver P1 in c4f4cef: all three packages now use the same public immutable core commit and independently verified archive SHA-256. Restored BaZi archive fetching (no Git requirement). Kept the shared resolver strict, added missing-checksum regression coverage, and made the isolated-sdist CI invoke the release resolver. Local pin tests: 19 passed. Clean source CI is rerunning on this commit. |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Validation