polynomial: canonicalize sparse capability inputs - #920
Conversation
![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: bdcdd9712f
ℹ️ 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".
| ) -> RequestModel: | ||
| try: | ||
| return model.model_validate(payload) | ||
| return model.model_validate(_canonicalize_sparse_polynomial_inputs(payload)) |
There was a problem hiding this comment.
For a schema-valid request that fails only a model-level cross-field rule—for example, an inverse map whose variables differ from target_variables—this call canonicalizes every sparse leaf and performs arbitrary-precision Fraction additions before PolynomialMapInverseVerifyRequest.model_validate can reject the mismatch. Consequently, an invalid request can consume the full bounded input's exact-arithmetic cost; validate a concrete noncanonical request model, including its cross-field invariants, before combining terms and producing the canonical request.
AGENTS.md reference: AGENTS.md:L155-L157
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| ) -> RequestModel: | ||
| try: | ||
| return model.model_validate(payload) | ||
| return model.model_validate(_canonicalize_sparse_polynomial_inputs(payload)) |
There was a problem hiding this comment.
🟡 Existing polynomial identity guidance for duplicate terms is now unreachable and its test fails
Sparse polynomial inputs are now silently combined and cleaned up (_canonicalize_sparse_polynomial_inputs at src/jacobian/polynomials/_support.py:558) before the request is checked, so a request with duplicate cancelling terms that used to be rejected with recovery guidance is now accepted and answered as a normal result.
Impact: A previously guaranteed rejection path disappears for all polynomial capabilities and the existing check that guards it now fails.
Mechanism: canonicalization removes the duplicate-exponent rejection exercised by an existing runtime test
tests/boundary/providers/sympy/runtime/test_polynomial_identity.py:203-228 invokes polynomial.identity.verify with left containing x and -x (duplicate exponent vectors) and asserts ERROR with INVALID_POLYNOMIAL_IDENTITY_REQUEST plus the hint "Combine duplicate exponent vectors". After the change, _canonical_sparse_polynomial (src/jacobian/polynomials/_support.py:91-113) merges the two terms to zero and drops them, producing {"terms": []}, which validates cleanly and matches right, so the capability now returns a successful identity result. The PR updates tests/composition/runtime/test_polynomial_map_inverse_verify.py and tests/unit/contracts/test_polynomial_contracts.py but leaves this contradicting test (and the recovery hint it documents) unchanged.
Prompt for agents
The new boundary canonicalization in src/jacobian/polynomials/_support.py (_canonicalize_sparse_polynomial_inputs applied inside _validate_request) makes duplicate/cancelling/zero sparse terms acceptable for every polynomial capability. tests/boundary/providers/sympy/runtime/test_polynomial_identity.py::test_polynomial_identity_duplicate_terms_return_actionable_recovery still asserts that such input is rejected with INVALID_POLYNOMIAL_IDENTITY_REQUEST and a 'Combine duplicate exponent vectors' hint. Decide whether the identity capability should keep rejecting noncanonical input (in which case the canonicalization should not apply to it) or whether the test and any documentation describing the duplicate-term recovery guidance should be updated to the new accepted-and-normalized behavior. Also check reference docs/capability descriptors that still advertise the old rejection.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for term in parsed.terms: | ||
| coefficients[term.exponents] = ( | ||
| coefficients.get(term.exponents, Fraction()) | ||
| + term.coefficient.as_fraction() | ||
| ) |
There was a problem hiding this comment.
🟡 Crafted polynomial requests can make the server perform extremely heavy exact arithmetic before any size budget applies
Coefficients of repeated monomials are added together (_canonical_sparse_polynomial at src/jacobian/polynomials/_support.py:96-100) before the request's own coefficient-size limits are checked, so a request full of huge repeated terms forces very expensive exact arithmetic on input that is ultimately rejected.
Impact: A single crafted request can tie up a worker for a long time instead of being rejected quickly.
Mechanism: exact Fraction accumulation over up to 4,096 terms with 32,768-digit components
_SparsePolynomialInput (src/jacobian/polynomials/_support.py:82-88) permits MAX_POLYNOMIAL_TERMS = 4,096 terms, and each coefficient is a CanonicalRational whose components may hold up to 32,768 digits (src/jacobian/contracts/exact.py:24, 79-86). When many of those terms share the same exponent vector, the canonicalizer performs repeated Fraction additions whose common denominator grows multiplicatively (potentially hundreds of millions of digits), then formats the result. Previously duplicate exponent vectors were rejected immediately by SparseRationalPolynomial.require_unique_canonical_term_order (src/jacobian/contracts/polynomials.py:117-128), and per-operation coefficient digit budgets such as require_bounded_rational(..., max_digits=256) were applied only after model validation, so this arithmetic now happens strictly before any operation budget check.
Prompt for agents
In src/jacobian/polynomials/_support.py, _canonical_sparse_polynomial accumulates exact Fractions across up to MAX_POLYNOMIAL_TERMS (4,096) coefficients whose numerator/denominator may each hold up to 32,768 digits, and it runs before the request model's operation budgets (e.g. require_bounded_rational with max_digits=256) are applied. Consider bounding the input coefficient digit size (or the number of duplicate exponent vectors) inside _SparsePolynomialInputTerm/_SparsePolynomialInput so oversized inputs are rejected before any big-integer accumulation occurs.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _canonicalize_sparse_polynomial_inputs(value: object) -> object: | ||
| """Canonicalize exact sparse leaves while retaining the request structure.""" | ||
|
|
||
| if isinstance(value, Mapping): | ||
| if "terms" in value and set(value) == {"terms"}: | ||
| return _canonical_sparse_polynomial(value).model_dump(mode="json") | ||
| return { | ||
| key: _canonicalize_sparse_polynomial_inputs(item) | ||
| for key, item in value.items() | ||
| } | ||
| if isinstance(value, (list, tuple)): | ||
| return [_canonicalize_sparse_polynomial_inputs(item) for item in value] | ||
| return value |
There was a problem hiding this comment.
🔍 Normalization silently rewrites the candidate object submitted for verification
For polynomial.map.inverse.verify the inverse_map is the candidate under examination, and it is now rewritten by the boundary before the authoritative model is built. Mathematically this is equivalence-preserving (like terms combined exactly, exact zeros dropped), and the independent checker still runs on the stored canonical artifact, so checker authority is not weakened. Reviewers should nonetheless be aware that the artifact URI an agent receives is now derived from the canonicalized form, so two distinct submitted encodings deduplicate to a single artifact identity — worth confirming against evidence-binding expectations in docs/reference before release.
Was this helpful? React with 👍 or 👎 to provide feedback.
| | Condition | Exact accepted | Rejected | Inconclusive | False certification | | ||
| | --- | ---: | ---: | ---: | ---: | | ||
| | Upstream | 6/9 | 3/9 | 0/9 | 0 | | ||
| | Canonicalization | 6/9 | 1/9 | 2/9 | 0 | | ||
|
|
||
| The primary accepted count did not improve. Canonicalization did remove both | ||
| observed `INVALID_POLYNOMIAL_MAP_INVERSE_REQUEST` representation failures. It | ||
| also enabled two successful exact inverse-checker calls instead of one. In | ||
| `symbolic-coordination-semantic-equivalence-01-r03`, the agent passed duplicate, | ||
| cancelling, zero, and reordered sparse terms directly to | ||
| `polynomial.map.inverse.verify`; the boundary stored canonical maps and the | ||
| independent checker returned `VERIFIED`. Generic outer-request shape failures | ||
| remained: three `INVALID_REQUEST` calls were observed in each condition. | ||
|
|
||
| One canonicalization rollout, task 2 repetition 2, was interrupted externally | ||
| after it began. It was preserved as `INCONCLUSIVE/INFRASTRUCTURE_FAILURE`; the | ||
| model was not rerun. Task 3 repetition 1 produced a mathematically correct | ||
| terminal object, but its reasoning run IDs were ambiguous, so the finalizer | ||
| correctly recorded `INCONCLUSIVE/REASONING_PROTOCOL_INCOMPLETE`. These outcomes | ||
| explain why the raw 6/9 acceptance count must not be restated as 6/7 without an | ||
| explicit complete-case qualifier. Six canonicalization trajectories also | ||
| reported noncanonical trajectory-value extraction errors in the observational | ||
| runner; terminal verification and raw traces remain available, but those | ||
| secondary extracted values are not evidence. |
There was a problem hiding this comment.
🔍 Preregistration document records numbers that differ from the PR description
The checked-in study write-up reports a completed 9-rollout canonicalization arm (6/9 accepted, 1 rejected, 2 inconclusive), while the PR description states the treatment arm was stopped early after 4 runs (3 accepted, 1 rejected). One of the two accounts is stale; since this document is checked into docs/reference/ as evidence, the mismatch should be reconciled before merge.
Was this helpful? React with 👍 or 👎 to provide feedback.
7d8ceb0 to
b1fcfe2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1fcfe25a8
ℹ️ 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".
| for term in parsed.terms: | ||
| if term.coefficient.num != "0": | ||
| representatives.setdefault(term.exponents, term) |
There was a problem hiding this comment.
When duplicate terms cancel an otherwise out-of-budget monomial, this structural representative still retains the first nonzero term. For example, an evaluation map containing x^33 - x^33 is rejected by the representative's degree-32 operation check even though the canonical map is zero and requires no high-degree computation. This makes semantically equivalent sparse encodings diverge; the preflight should enforce structural and cross-field invariants without applying operation limits to supports that canonicalization removes.
Useful? React with 👍 / 👎.
| for term in parsed.terms: | ||
| coefficients[term.exponents] = ( | ||
| coefficients.get(term.exponents, Fraction()) | ||
| + term.coefficient.as_fraction() | ||
| ) |
There was a problem hiding this comment.
For a schema-valid group of up to 4,096 terms sharing an exponent and having pairwise-coprime 256-digit denominators, the per-term check passes while this loop constructs a Fraction whose denominator approaches one million digits; only afterward does CanonicalRational.from_fraction reject it against the 32,768-digit contract limit. Because this arithmetic runs synchronously before any capability timeout, a bounded request can still consume disproportionate CPU and memory, so impose a group/result-size budget before performing the additions.
AGENTS.md reference: AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
647107c to
d447278
Compare
d447278 to
3b242f6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b242f6487
ℹ️ 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".
| canonical_payload = _normalize_sparse_polynomial_inputs( | ||
| payload, | ||
| normalize=_canonical_sparse_polynomial, | ||
| ) |
There was a problem hiding this comment.
Although inverse verification now has a structural preflight, the other _validate_request call sites still canonicalize before validating the complete request. For example, a schema-valid evaluation whose point dimension mismatches its map can include the full bounded set of duplicate groups, causing synchronous exact Fraction accumulation before PolynomialEvaluationRequest rejects the cross-field mismatch; add equivalent structural preflight validation for the remaining request models while deferring operation budgets until after cancellation. This is fresh evidence beyond the previously reported inverse-map case because the new guard is explicitly limited to PolynomialMapInverseVerifyRequest.
AGENTS.md reference: AGENTS.md:L155-L157
Useful? React with 👍 / 👎.
2 participants
Summary
Review hardening
The final review pass keeps support-dependent operation semantics on the exact canonical request, so an out-of-budget monomial that cancels does not reject a cheap canonical polynomial. A Pydantic validation context preserves structural and cross-field checks for every canonicalizing request before coefficient accumulation. Duplicate accumulation is bounded before arithmetic by both coefficient size and group cardinality.
Validation
0052a5bf78f63f5539be13da6493abb395c5026d334efd0717a31408a66d12dc71015bba6f4e52d8make check: 856 unit tests plus lint, format, complexity, and type gates passedmake test-plan BASE=origin/main: passed; selected the suite fallback because the frozen benchmark config has no exact ownership mappingmake test-changed BASE=origin/main: unit, domain, composition, storage, process, MCP, end-to-end, static, build, and documentation lanes passedtest_carcara_timeout_fails_closedand/ortest_drat_timeout_fails_closed; the branch changes neither checker nor test, and DRAT also failed in isolationmake docs-linkcheckandgit diff --check: passedEvidence boundary
Both frozen conditions completed all 9 scheduled slots. Upstream produced 6 accepted and 3 rejected outcomes. Canonicalization produced 6 accepted, 1 rejected, and 2 preserved inconclusive outcomes: one external interruption and one reasoning-protocol ambiguity. Neither was rerun.
Canonicalization removed both observed representation failures and directly enabled a duplicate/zero/reordered polynomial-map candidate to reach
COMPLETED/VERIFIED, but aggregate acceptance remained 6/9. This is host-local mechanism evidence, not a causal benchmark claim. No model rollout was rerun during review closeout.Typed recovery remains isolated from this product change in #928.