feat(combinatorics): verify submitted P-recursive tables - #1285
Merged
Conversation
morluto
force-pushed
the
agent/port-p-recursive-table-residuals-v2
branch
from
August 12, 2026 18:57
54c3bdf to
d8e4786
Compare
Refresh #1285 while preserving its finite-table semantic scope.
Contributor
Coverage ReportTotal coverage: 78% (threshold: 50%) |
Owner
Author
AttributionThe exact caller-supplied P-recursive table verification originated in #1182, authored by @yuelgrace1810-ops. This PR ports that semantic work onto the current typed ownership model and preserves that authorship. |
morluto
marked this pull request as ready for review
August 13, 2026 18:18
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![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: 402c28a944
ℹ️ 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
+54
to
+55
| if polynomial[-1].as_fraction() == 0: | ||
| raise ValueError("coefficient polynomial must omit trailing zero terms") |
There was a problem hiding this comment.
Allow a singleton zero polynomial ([{"num":"0","den":"1"}]) and reject a trailing zero only when the vector has multiple coefficients. Zero lag polynomials are valid in sum_j p_j(n)a_(n-j)=0—for example, a_n-a_(n-2)=0 requires p_1=0—but this validator rejects every such table even though the existing P-recursive contract and independent checker both accept [0], silently narrowing the advertised recurrence domain.
AGENTS.md reference: AGENTS.md:L55-L58
Useful? React with 👍 / 👎.
Comment on lines
+64
to
+68
| value, | ||
| max_digits=MAX_COMBINATORICS_INPUT_RATIONAL_DIGITS, | ||
| label="submitted recurrence table value", | ||
| ) | ||
| return self |
There was a problem hiding this comment.
Add an aggregate result-size check before accepting the request, or publish this result durably. A maximum-order, maximum-degree table with many pairwise-coprime bounded denominators can produce hundreds of individually valid residuals whose combined encoding exceeds the 10 MiB InlinePublication limit; this validator checks only each input rational, so the producer performs the expensive complete replay and then fails during publication instead of rejecting the request up front.
AGENTS.md reference: AGENTS.md:L150-L153
Useful? React with 👍 / 👎.
Comment on lines
+114
to
+118
| polynomials = tuple( | ||
| tuple(value.as_fraction() for value in polynomial) | ||
| for polynomial in request.coefficient_polynomials | ||
| ) | ||
| values = tuple(value.as_fraction() for value in request.values) |
There was a problem hiding this comment.
Keep CanonicalRational conversion in the capability adapter and make the public jacobian.math.combinatorics kernel accept and return computational values such as Fraction-based domain values. As exported here, native callers must construct Pydantic wire requests containing decimal strings, and the function immediately converts those strings to Fraction before converting results back to wire models, making the supported native API itself a wire boundary rather than a computational API.
AGENTS.md reference: AGENTS.md:L92-L96
Useful? React with 👍 / 👎.
Comment on lines
+10
to
+14
| __all__ = [ | ||
| "IndexedRecurrenceResidual", | ||
| "PolynomialCoefficientRecurrenceTableRequest", | ||
| "PolynomialCoefficientRecurrenceTableResult", | ||
| "recurrence_table_residuals", |
There was a problem hiding this comment.
Add combinatorics to jacobian.math.__all__ and to the public namespace/import-isolation manifests. This package declares a supported public surface, but import jacobian.math does not expose it like the other domain modules, and the unchanged PUBLIC_API table means its symbols and isolation behavior receive none of the required API checks.
AGENTS.md reference: AGENTS.md:L65-L68
Useful? React with 👍 / 👎.
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
Supersedes #1182 with a post-#1276 typed semantic port.
jacobian.math.combinatoricsfor complete caller-supplied rational tables;sum_j p_j(n) a_(n-j)=0without generating or repairing terms;fractions.Fraction.Ownership
The math layer owns the finite table and residual semantics. The combinatorics domain publishes a thin
OperationSpecand owns the checker declaration. The existing clean-process recurrence checker independently rebuilds the residual ledger.The typed value contains no backend, assurance, workflow, or infinite-recurrence claim. Finite-table verification establishes only the exact supplied table and relation.
Validation
The branch is based directly on
195c4c1ddcb836206fcc759d602d3cf77eb2d7cf. Its port workflow compiled all changed source/test files and removed itself before committing. Tests cover a valid factorial table, a corrupted final term, and a forged success result.Credit
This post-#1276 port carries forward the original P-recursive table-verification work in #1182 by @yuelgrace1810-ops. Their investigation and implementation work are the basis for this replacement.