release: 3.8.0 - #296
Conversation
Version in Cargo.toml, Cargo.lock and pyproject.toml; release-pinned wheel URLs
and install examples in README.md and the packaged skill; CHANGELOG's Unreleased
section closed as 3.8.0.
Note what is deliberately *not* bumped: docs/mdbook/src/rl.md states the Hub
package depends on `alkahest>=3.7.0`, which is a minimum-version constraint —
alkahest.rl shipped in 3.7.0 and the Hub still works against it, so bumping it
would narrow the requirement for no reason. Historical references in the
CHANGELOG body ("shipped in 3.7") are likewise left alone: they say which
release carried a defect, and rewriting them would destroy exactly the
information the recheck table exists to convey.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found by running the CUDA suite on real dual-3090 hardware — the only configuration where any of these can be observed, since the `cuda` feature is off in every build CI and the shipped wheel produce. The suite itself passes: 17 CUDA-gated tests green, memcheck and racecheck both clean. The nightly sanitizer steps were checking nothing. `compute-sanitizer --tool memcheck cargo test ...` instruments `cargo`, a process that makes no CUDA calls, because the default is `--target-processes application-only`. It emitted a banner, no ERROR SUMMARY, and a green tick. `--target-processes all` makes it follow into the test binaries, which is what produced the clean results above. This is the fourth gate found this cycle that passed while inspecting nothing, after `check_error_codes.py` (unreferenced by any workflow), the valgrind loop globbing a package name that matches zero binaries, and an ASan job that never runs Python. `capabilities()` said `llvm_jit: false` on a build that demonstrably emits NVPTX. `alkahest-core`'s `cuda = ["jit", ...]`, so `--features cuda` links the LLVM backend even though *alkahest-py*'s own `jit` feature is off, and the bit read `cfg!(feature = "jit")` on the wrong crate. A capability has to describe what is linked, not which flag the caller named. `capabilities()["features"]["cuda"]` said `true` while `ak.compile_cuda` raised AttributeError: the native module defines `compile_cuda`, `CudaCompiledFn` and `CudaError` under that feature and `__init__.py` re-exported none of them, so the only way in was the private `alkahest.alkahest`. They are now appended to `__all__` at runtime when present — not listed in the literal, because the names genuinely do not exist in a default build and every name in `__all__` must resolve. Two tests pin the invariants. A third assertion had to be relaxed: it required `not llvm_jit` whenever cranelift was on, encoding "cranelift implies no LLVM", which is false on a cuda build where both are linked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nted nothing Verified on dual RTX 3090 hardware against release/3.8.0 @ d139a46, the configuration none of this is observable in otherwise: CI builds the `cuda` feature nowhere and the shipped wheel does not carry it. The racecheck step still checked nothing. d139a46 added `--target-processes all`, which fixed memcheck: it now emits a real `ERROR SUMMARY: 0 errors`, and a control run with the GPU tests filtered out proves the line is evidence of instrumentation rather than decoration -- with no kernel launched the sanitizer says "Target application terminated before first instrumented API call" and prints no summary at all. Racecheck did not benefit, because wrapping the whole `cargo test` pulls rustdoc's doc-test runner under the sanitizer, where it segfaults (exit 139) after the CUDA suites pass but before the summary prints. `continue-on-error: true` then rendered that as a green tick with no RACECHECK SUMMARY anywhere in the log -- the same failure mode d139a46 set out to remove, one step further down the file. Scoping both steps to the two integration targets that launch kernels yields `RACECHECK SUMMARY: 0 hazards displayed` and costs no coverage: the in-`src` unit tests only generate PTX, and `compute_groebner_basis_gpu(.., None)` takes the `reduce_cpu` path, so neither issues a CUDA API call. `CudaCompiledFn` could only ever run on device 0 from Python. The core has had `call_batch_on(ordinal, ..)` all along -- `nvptx_gpu::nvptx_multi_device_both_3090s` drives both cards through it -- but the binding exposed only `call_batch`, so on a multi-GPU host every device but the first was unreachable, which is the same shape as `cuda` advertising an entry point the public namespace could not reach. `call_batch_on` is now bound, `call_batch` delegates to it with ordinal 0, and the two share one input-validation body. Both cards return bit-identical results across 6028 points on six expressions; an out-of-range ordinal raises a structured `CudaError [E-CUDA-003]` rather than aborting. GPU/CPU numerical agreement was checked and is not a problem. Against numpy's ufuncs -- an oracle sharing no code with the codegen, unlike `numpy_eval`, which delegates to the same Cranelift `CompiledFn` -- every lowered primitive agrees to 0-2 ulp over 5011 points. The largest discrepancy anywhere, 6.17e-15 on `x**3 - 2*x`, is exactly one ulp of `x**3` divided by a difference that cancels 40.8x; the 1.13e-13 on `tan(x)*exp(x**2)` is `exp`'s condition number of |u|=585 amplifying a last-place difference in its argument. Both are what floating point does, not what the backend got wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Second sweep over everything the hunts had written up but not fixed. The silent-error corpus goes 166 -> 225 scored cases, still at 0 silent errors. solve ----- Two independent defects. It picked one Lex generator per variable and never re-checked the finished tuple against the rest of the ideal, so `[x^2-xy, xy-y]` reported `(-1,1)`, whose residual is 2. And generator selection ignored whether the leading coefficient vanishes under the partial assignment: for `<x^2+3y, 2xy+3x, 2y^2+3y>` that coefficient is `2y+3`, exactly zero on the branch `y = -3/2`, so the quadratic formula divided by zero and *both* real solutions disappeared. Step selection is now per-branch and adaptive, and every parameter-free tuple is substituted back into the input equations under complex ball arithmetic — one-sided, so a genuine solution can never be filtered out. A repeated root also reported twice, because the pool folds no arithmetic on literals and `0^2 - 4*1*0` never looked like zero. Two 700-system differential fuzzes, ground truth from resultant elimination plus 60-digit root-finding with every point re-substituted: missing solutions 11 -> 0 and 8 -> 0, spurious 19 -> 0, duplicates 46 -> 0, non-number coordinates 7 -> 0. Zero previously-answered systems now refuse, and one corpus *gains* 7 answers because elimination order is no longer pinned to the monomial order. `method="homotopy"` returned `[]` for a whole branch: the polyhedral cell iterator keeps exactly the edge pairs its binomial solver cannot use, so it never produced a start point and zero paths were reported as zero solutions. It now falls back to the Bezout tracker, and reports E-HOMOTOPY-004 rather than `[]` when no path completes — `[]` cannot mean both "no solutions" and "the tracker failed everywhere". sums, products, telescoping --------------------------- `product_definite` dropped one rational scale factor per index, so answers were off by c^(hi-lo+1); it cancelled only when numerator and denominator scales were equal, which is why integer coefficients always looked right. `sum_definite` checked only the telescoped difference, which never mentions the interior indices, and so summed straight through a pole. `rsolve` mis-shifted the RHS and sold `C0*2^n + C1*2^n` as a general solution for a repeated root. `euler_maclaurin`'s gate could not fail: the constant was fitted at n = 512, the largest check point the gate scored, so the residual there was zero by construction and no fabricated value could be rejected. The fit moved outside the gate and the constant must now reproduce at a second point; genuine constants drift <= 3.2e-3 against >= 0.93 for fabricated ones. `zeilberger` emitted a certificate without the boundary hypothesis WZ requires. Stated as a side condition rather than verified, deliberately: no summation range is supplied, and the hypothesis is a statement about a range, so "verifying" it would mean inventing one. elimination, special functions, panics -------------------------------------- `subresultant_prs` disagreed with `resultant` (4 against 8 on a case whose Sylvester determinant is 8). The reported cause — a discarded pseudo-division multiplier — was real but not sufficient: defective sequences and equal-degree sign errors survived it, 30 of 300 random pairs. Rewritten as Ducos' chain and checked against the determinantal definition implemented from scratch, twice, sharing no code with the module. Not against SymPy, whose `resultant` the hunt had already found wrong for odd-by-odd degrees. Gamma used the reflection formula, and `sin(pi * -2.0)` is 2.45e-16 rather than zero, so `Gamma(-2)` returned 6.4e15; `product_definite(k-5, k, 1, 3)` inherited it as -96 where the truth is -24. Now refuses with the same code as `0^-1`, returning infinity rather than NaN so that `1/Gamma` stays usable at its zeros. `limit(sqrt(x), x, 0, dir="-")` returned 0 for a limit that does not exist over the reals; it now refuses on positive evidence only, and a 1890-limit sweep changed 45 verdicts, all of them correct. Three more panics crossing the FFI boundary as BaseException, plus one that was a flint_abort — a SIGABRT no Python handler can catch. contracts --------- `except alkahest.AlkahestError` missed every Python-layer error: the wrappers subclassed a pure-Python base that was not the native one, so the two hierarchies were disjoint and the documented catch-all silently skipped AnsatzError, CrossCheckError and SmtError — all three modules added this cycle for the loops this release is for. CUDA had no Python-level coverage at all (`pytest -k cuda` selected nothing) and no CI job had ever built the extension with the feature, which is how `ak.compile_cuda` came to raise AttributeError on a build whose own capabilities() advertised `cuda: true`. Now 17 tests in three tiers, and the nightly builds and exercises the Python surface. `compute-sanitizer` was instrumenting `cargo` — a process that makes no CUDA calls — and so passed while checking nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lease/3.8.0 # Conflicts: # .github/workflows/cuda_nightly.yml # tests/test_agent_contract.py
Checked the dispatched nightly rather than trusting the green ticks, and three of the eight shards were inspecting nothing. valgrind built with `cargo build`, which does not produce test binaries, so `target/.../deps/alkahest_cas-*` matched only .rlib/.d artifacts, `[ -x ]` was false for every one, and the loop body never executed. The whole shard emitted no HEAP SUMMARY, no ERROR SUMMARY, no Memcheck banner — and reported success. It now builds with `cargo test --no-run`, skips non-executables explicitly, and **fails when it matches nothing**, which is what would have caught this. fuzz-expr and fuzz-simplifier never fuzzed. AFL refuses to start while the kernel core_pattern is a pipe; it printed its own remedy, exited non-zero, and `|| true` rendered that as a pass. Both shards ran zero executions for however long they have been "green". They now run `cargo afl system-config` first, distinguish `timeout`'s 124 (the 2 h cap doing its job) from a real failure instead of swallowing every status, and assert `execs_done > 0` from fuzzer_stats afterwards. That makes eight gates found this cycle that passed while checking nothing: scripts/check_error_codes.py referenced by no workflow (53 accumulated errors), TESTING.md's valgrind loop naming a package that does not exist, an ASan job scoped so that pytest never runs under it, compute-sanitizer instrumenting `cargo` rather than the test binaries, the same again for racecheck one step further down that file, euler_maclaurin's convergence gate fitted at the very point it scored, and now these three. Tier 1b is genuine: 2 slow tests selected and passed. It completes in under a second because the sparse_interp roadmap case it exists to cover was fixed in #292, not because it skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…llbacks `system-config` runs sudo internally — AFL's own message says so — and cargo lives in ~/.cargo/bin, which sudo's secure_path does not include, so the sudo prefix would have failed with "cargo: command not found" and wasted a two-hour shard proving nothing. Also sets AFL_I_DONT_CARE_ABOUT_MISSING_CRASHES and AFL_SKIP_CPUFREQ, so the fuzzer proceeds even where core_pattern cannot be changed or no CPU governor is exposed, rather than refusing to start. The `execs_done > 0` assertion is what actually proves the shard did work, so the fallbacks are safe: if they are not enough, the shard now fails instead of passing quietly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two gaps, both found by checking rather than by anything failing. `release/**` now gets push CI. `ci.yml` fired on push only for `main`, and the `pull_request` runs did not fire for a PR based on `release/3.8.0` either — PR #295 got no checks at all beyond a CodeRabbit note that reviews are disabled for that base. So every merge into the release branch since the version bump, including a merge commit resolving conflicts across three files, reached the branch with no automated verification whatsoever; local gate runs were the only thing standing behind them. A branch that a release tag is cut from should not be less covered than main. Extended to the semver, cross-compile, CodSpeed and docs workflows for the same reason. CUDA Nightly is no longer scheduled. It targets `[self-hosted, gpu-3090]` and no such runner is registered, and an unrunnable scheduled job does not fail — it queues. Every night for at least five days it sat pending for 24 hours and was auto-cancelled, producing no signal in either direction and one permanently pending job on main. `workflow_dispatch` is retained so it runs the moment a runner exists, and the file is kept because it is the specification an agent on a GPU box follows step for step. The header now records that the CUDA surface is verified by hand instead, and what that verification covered for 3.8.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The guards added in 4225654 did their job — all three shards went from green to red, which is the correct direction for gates that were inspecting nothing. This fixes the underlying causes they exposed. valgrind now takes the executable paths from `cargo test --no-run --message-format=json` rather than globbing `target/<triple>/debug/deps/`. The build succeeded and the glob still matched nothing, because `-Z build-std` does not put the binaries where that path assumed. Asking cargo which executables it produced removes the assumption entirely, and the failure message now dumps every executable cargo did report, so a future mismatch is diagnosable from the log instead of from a bare exit code. Both fuzz shards got past `core_pattern` — `cargo afl system-config` works, once it is not wrongly prefixed with sudo — and then died on `SYSTEM ERROR : Unable to create 'fuzz/out/expr_builder'`. `fuzz/out/` is gitignored, so it does not exist in a fresh checkout, and AFL will not create a nested path under a missing parent. `mkdir -p fuzz/out` first. The seed corpora are tracked and present, but an explicit check makes an empty one say so rather than surfacing as a cryptic zero-execution failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Third layer in the same shard. `fuzz/` is excluded from the root workspace, so `cargo afl build --manifest-path fuzz/Cargo.toml` writes to `fuzz/target/`, while the fuzz invocation pointed at `target/debug/<bin>` — AFL got all the way past core_pattern and the output directory, then aborted with "Program not found or not executable". Same class as the valgrind glob: a hardcoded path standing in for something the build system already knows. The binary is now located with `find` across both candidate target directories, and if it is missing the step prints the executables that *were* produced rather than failing with a bare status. Each of these was invisible while `|| true` swallowed the exit code. Removing it surfaced three stacked breakages one at a time, in dependency order. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Tier 1a was ~16.6 minutes of step time and ASan was 467s of it — nearly half the wait on every pull request, for a check whose value is depth rather than immediacy. Measured per-step rather than guessed: ASan 467s, pytest 173s, cargo test 86s, cargo test (egraph) 79s, maturin 45s, the four clippy feature runs 104s combined, everything else under 15s. Moving ASan (and the nightly toolchain install it alone needed) takes Tier 1a to about 520s, or 8.7 minutes. Nothing else is worth moving: the clippy feature combinations are cheap and catch feature-gated build breaks that are exactly a PR's business, and the egraph test run is a correctness gate. The nightly shard gets a better version than the one it replaces. Tier 1a ran `-p alkahest-cas` because full-workspace ASan under -Z build-std risks the runner cap; nightly has the budget for `--workspace`. Note the nightly job installs the nightly toolchain only for the sanitizer shards, and that condition did not list `asan` — the new shard would have run `cargo +nightly` without it. Fixed here rather than discovered by a red run. The fuzz and valgrind shards discussed alongside this were already nightly-only and never contributed to PR latency. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
First genuine execution of this shard since it was written. It ran the alkahest_cas test binary for 2h49m and reported: definitely lost: 0 bytes in 0 blocks indirectly lost: 0 bytes in 0 blocks ERROR SUMMARY: 4 errors from 4 contexts (suppressed: 315 from 315) No memory-safety errors and no real leaks. The four failures were all "possibly lost", 10,280 bytes across 20 blocks: a thread-local std::thread Thread handle, MPFR's const_log2 cache reached through mpfr_cache, and two hashbrown table allocations. Those are interior pointers into intentional caches and process-lifetime globals — the category valgrind reports and does not consider a defect. So the gate was failing on the wrong thing. `--errors-for-leak-kinds=definite, indirect` keeps an invalid read or write, and any definitely- or indirectly-lost block, as a failure, while a possible leak is reported but does not fail the build. Preferred over four more suppressions matched on stack shape, which would rot the first time an inlining decision changes and which would also hide a genuine leak arriving through the same frames. The existing valgrind.supp is doing its job — 315 records suppressed — and is unchanged. Worth noting for later: 2h49m of a 6h budget, because it valgrinds every unit test in a -Z build-std debug build. The step is named for the Rust/C FFI boundary, and scoping it there would cost little coverage for most of the time. Left alone for now because it completes and is nightly-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
With a 14-minute budget the earlier trade is unnecessary. CI wall-clock is the slowest *job*, not the sum of steps, so ASan does not need to sit inside Tier 1a where its 467s was half the wait. As its own job it runs concurrently: Tier 1a finishes in roughly six minutes without it, the ASan job takes about nine and a half including setup, and the critical path is the larger of the two — about ten minutes, inside the budget, with the PR-time coverage restored. Scoped to `-p alkahest-cas` here, matching what Tier 1a ran. The nightly `asan` shard keeps `--workspace`, so the tiering is deliberate: fast targeted coverage on every PR, the full workspace overnight. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Tier 1b no longer waits for nightly. It was nightly-only because the sparse_interp roadmap case took long enough to matter; #292 replaced the recursive formulation with Zippel's iterative algorithm and the job is now about a minute, of which the tests are half a second. Running in parallel it costs nothing on the critical path, and it guards the oracle call-count property that a regression would otherwise silently undo for a day. valgrind now runs the tests that actually cross into GMP/MPFR/FLINT rather than all 1984 unit tests: 121 selected, 1863 filtered out. Its first real run took 2h49m of a 6h budget to re-check pure-Rust logic the other shards already cover. ALKAHEST_VALGRIND_FILTER overrides it, empty for a full sweep. Two things about that scoping were nearly a new vacuous gate, which is worth recording given the eight found this cycle. libtest takes filters as separate positional arguments and matches them as substrings — a single "a|b" argument matches nothing, runs zero tests, and lets valgrind report a clean bill. And even correctly passed, a filter that stops matching after a rename would do the same silently. So the step now asserts the binary actually executed tests and fails when it did not. Verified locally: the filter list selects 121 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three defect clusters, all of the same family: a routine that could not answer returned something anyway, with nothing behind it. ideal/: `radical` implemented only the zero-dimensional Seidenberg trick over univariate generators appearing verbatim in the basis, and returned the input unchanged otherwise — asserting √I = I. `primary_decomposition` then overwrote every component's `associated_prime` with that, so a field whose name is a guarantee could name a non-prime. Both now compute where provable (monomial, principal, zero-dimensional, and certified leaves) and refuse with E-IDEAL-005/006 otherwise. `triangularize`'s main-variable pick was inverted — it returned the largest index while the bottom-univariate split assumes index 0 is lex-greatest — so generators collided in one slot and the tie-break discarded the rest; it now also verifies <chain> ⊇ I and refuses with E-SOLVE-004 rather than return an under-determined chain. calculus/series: an unreachable order ran away instead of stopping. It now refuses (E-SERIES-003) rather than assemble a truncated prefix, whose O(h^n) term would be a false statement about the remainder. simplify: the flat MAX_EXPAND_POW_EXP=4 cap was incoherent (it allowed 160k products while refusing (x+y)^5) and declined silently. Replaced by a 4096-product budget, with declines recorded as an `expand_pow_limit_reached` step in the derivation log. solver: `solve([a*x-b],[x])` returned b/a without stating a ≠ 0; hypotheses are now recorded and readable via `solve_side_conditions()`. capabilities(): `groebner_cuda` and `numpy` advertised features that named nothing a Python caller could reach — no observation distinguished True from False. Both removed (contract v3), along with the dead numpy dependency; `compute_groebner_basis_gpu`'s silent CPU fallback is now reported through `GpuBackendReport`. A new contract test walks every advertised bit and requires a reachable entry point, so this cannot recur. All refusals travel out of band (take_ideal_refusal / take_triangularize_- refusal / take_series_refusal), following matrix::take_zero_test_refusal: the enums are public and exhaustive, so a correctness fix cannot spend a major version on a new variant. The bindings consult the takers, and a regression test pins that a genuine non-polynomial still reports E-SOLVE-001 rather than being re-attributed to the refusal code. Also fixes groebner_cuda tests panicking instead of skipping when libcuda is absent, and residue() raising a bare AttributeError on a non-numeric point (now E-RESIDUE-005). Gates: fmt, clippy -D warnings, 2026 lib + 26 doctests, cargo doc -D warnings, semver-checks (no update required), 2689 pytest, silent-error corpus 0/238, ruff, check_error_codes, check_api_freeze. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 36 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughVersion 3.8.0 adds correctness checks, structured refusal APIs, solver verification, CUDA reporting, Python binding updates, expanded regression coverage, release-branch CI, and updated documentation. ChangesRelease and CI
Core computation
Public interfaces
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Merging this PR will degrade performance by 32.18%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
CodSpeed: 3 regressions, all measured and accounted forCodSpeed flags 3 regressed benchmarks against
On the
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
alkahest-core/src/simplify/engine.rs (1)
376-405: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse an invocation-scoped expansion-limit collector
alkahest-core/src/simplify/redex.rs#L146-L167:ExpandPowrecords events in worker-local storage, butexpand_limit_log()drains only the caller thread. Merge worker events into the pass log.alkahest-core/src/simplify/engine.rs#L376-L405: prevent nested calls on the same thread from clearing or draining the outer invocation’s events.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@alkahest-core/src/simplify/engine.rs` around lines 376 - 405, Use an invocation-scoped expansion-limit collector: update alkahest-core/src/simplify/engine.rs lines 376-405 to isolate each top-level simplify invocation while preserving outer events across nested calls, and update alkahest-core/src/simplify/redex.rs lines 110-129 so ExpandPow worker events are transferred into the current pass log rather than relying only on caller-thread expand_limit_log() draining. Ensure outer invocation events are not cleared or drained by nested simplify calls.
🟡 Minor comments (17)
python/alkahest/exceptions.py-60-63 (1)
60-63: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
ValueErrorfor the pure-Python fallback base.The native
AlkahestErrorsubclassesValueError. The binding relies on that:alkahest-py/src/lib.rs(lines 3583-3630) states thatAlkahestErrorsubclassesValueErrorsoexcept ValueErrorkeeps working, andtests/test_residue.pyline 63 assertsisinstance(excinfo.value, ValueError). When the extension is missing,_NativeAlkahestError = Exceptiondrops that guarantee, soexcept ValueErrorno longer catches Python-raised wrappers. Align the fallback with the native hierarchy.🛠️ Proposed fallback change
try: # The compiled extension imports no Python modules, so this cannot cycle. from .alkahest import AlkahestError as _NativeAlkahestError except ImportError: # pragma: no cover - pure-Python fallback (no extension) - _NativeAlkahestError = Exception + _NativeAlkahestError = ValueError🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/alkahest/exceptions.py` around lines 60 - 63, Update the pure-Python fallback assignment for _NativeAlkahestError in exceptions.py to use ValueError instead of Exception, preserving the native AlkahestError inheritance contract so Python-raised wrappers remain catchable by except ValueError.tests/silent_errors/corpus.py-631-639 (1)
631-639: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep
KeyboardInterruptandSystemExituncaught.The second clause catches every
BaseException, soKeyboardInterruptandSystemExitalso becomeRuntimeError. A user who interrupts a corpus run then sees a scoredno_answercase instead of a stop, and the run continues. Re-raise those two before the conversion.🛠️ Proposed fix
def op() -> Any: try: return fn() except Exception: raise + except (KeyboardInterrupt, SystemExit): + raise except BaseException as exc: # PanicException is a BaseException — the point raise RuntimeError( f"escaping Rust panic: {type(exc).__module__}.{type(exc).__name__}: {exc}" ) from exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/silent_errors/corpus.py` around lines 631 - 639, Update the exception handling in op so KeyboardInterrupt and SystemExit are re-raised before the broad BaseException conversion; preserve converting other BaseException subclasses, including PanicException, into RuntimeError.tests/silent_errors/corpus.py-812-824 (1)
812-824: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMatch the reported condition more precisely.
Line 816 tests
str(hypothesis_about) in str(c).str(_A)is the single character"a", so any side condition whose text contains the letterascores0.0. A condition such as"b ≠ 0"would not match, but a condition mentioning any other identifier or word containingawould. That lets an unrelated disclosure pass the gate the case exists to enforce. Use a word-boundary match.🛠️ Proposed fix
- stated = any(str(hypothesis_about) in str(c) for c in ak.solve_side_conditions()) + pattern = re.compile(rf"(?<!\w){re.escape(str(hypothesis_about))}(?!\w)") + stated = any(pattern.search(str(c)) for c in ak.solve_side_conditions())Add
import reto the module imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/silent_errors/corpus.py` around lines 812 - 824, Update the side-condition check in op to use a regular-expression word-boundary match for hypothesis_about rather than substring containment, and add the required re import. Preserve the stated path only when a complete identifier/word match is present.docs/sphinx/api/errors.rst-170-182 (2)
170-182: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the new refusal codes to the refusal index.
Lines 50-51 list the refusal codes, but they omit
E-SOLVE-004,E-IDEAL-005, andE-IDEAL-006. This section classifies all three as refusals. Add them to the index so users do not miss the new cases.Proposed refusal-list update
-Refusals: ``E-CAD-001``, ``E-LINALG-010``, ``E-MAT-004``, ``E-SOS-002``, -``E-ANSATZ-003``, ``E-SMT-003``, ``E-INT-001``, ``E-BUDGET-001..003``. +Refusals: ``E-CAD-001``, ``E-LINALG-010``, ``E-MAT-004``, ``E-SOS-002``, +``E-ANSATZ-003``, ``E-SMT-003``, ``E-INT-001``, ``E-BUDGET-001..003``, +``E-SOLVE-004``, ``E-IDEAL-005``, ``E-IDEAL-006``.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/sphinx/api/errors.rst` around lines 170 - 182, Update the refusal-code index at the section around the existing lines 50–51 to include E-SOLVE-004, E-IDEAL-005, and E-IDEAL-006. Keep the existing index structure and descriptions consistent with the refusal entries documented near triangularize, radical, and primary_decomposition.
170-182: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSynchronize refusal documentation with the binding behavior.
The bindings consume the refusal records and tests assert the specific codes. Update the Sphinx note and the stale mdBook paragraph, and add
E-IDEAL-005,E-IDEAL-006, andE-SOLVE-004to the Sphinx refusal list.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/sphinx/api/errors.rst` around lines 170 - 182, Synchronize the refusal documentation with binding behavior: add E-IDEAL-005, E-IDEAL-006, and E-SOLVE-004 to the Sphinx refusal list, update the note describing radical and primary_decomposition to match the binding-exposed refusal records, and revise the corresponding stale mdBook paragraph. Preserve the documented code meanings and reference the existing take_ideal_refusal() and take_triangularize_refusal() symbols.Source: Coding guidelines
docs/mdbook/src/telescoping.md-62-70 (1)
62-70: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winComplete the boundary-check example.
The prose asks the reader to check
G(n, k_hi+1) - G(n, k_lo), but the example computes onlyg_at_lo. Add thek_hi + 1evaluation and compare both values before concluding that the boundary term is zero.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/mdbook/src/telescoping.md` around lines 62 - 70, Complete the boundary-check example after the existing g_at_lo calculation by evaluating cert.boundary_term at k_hi + 1, then compare the upper and lower boundary values to verify their difference is zero. Keep the example aligned with the documented G(n, k_hi+1) - G(n, k_lo) check and use the existing symbols cert.boundary_term, k_hi, and k_lo.docs/mdbook/src/gpu.md-100-115 (1)
100-115: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not treat every
E-CUDA-003as an invalid device ordinal.Line 112 maps any
CudaErrorfromcall_batch_onto “no such device”. Line 148 definesE-CUDA-003to include context creation, module loading, and memory-copy failures. The helper can report a false device count when a device exists but another CUDA operation fails.Restrict the probe to an ordinal-specific error, or describe it as a best-effort probe that does not establish the device count.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/mdbook/src/gpu.md` around lines 100 - 115, Update the device_count helper to avoid interpreting every ak.CudaError from call_batch_on as an invalid ordinal; restrict termination to an ordinal-specific error if available, or revise the documentation and helper semantics to clearly identify the result as a best-effort probe rather than an authoritative device count.docs/mdbook/src/codegen.md-164-176 (1)
164-176: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBind
alkahestbefore callingalkahest.capabilities().The example imports only
compile_cuda, so the capability check raisesNameError. Addimport alkahestor import and callcapabilitiesdirectly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/mdbook/src/codegen.md` around lines 164 - 176, Update the CUDA example around compile_cuda to bind the alkahest module before calling alkahest.capabilities(), either by importing alkahest or importing capabilities directly. Preserve the existing feature-guard behavior and CUDA compilation flow.docs/mdbook/src/gpu.md-53-58 (1)
53-58: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not promise PTX emission from the
cudacapability bit.
cuda == Trueguarantees linked CUDA entry points, but PTX compilation also requireslibdevice.10.bc; its absence raisesE-CUDA-005. Limit this statement to the linked entry points.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/mdbook/src/gpu.md` around lines 53 - 58, Update the CUDA capability description near the `cuda == True` statement to remove the promise that PTX can be emitted on the host. Keep the guarantee limited to the presence of `ak.compile_cuda` and `ak.CudaCompiledFn`, and preserve the existing distinction between capability linkage and runtime GPU availability.alkahest-py/src/lib.rs-10614-10622 (1)
10614-10622: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
solve_side_conditions()can report conditions from the abandoned polynomial attempt.
capture_solve_side_conditionsruns unconditionally aftersolve_polynomial_system. Whennumeric=trueand the result isSolverError::HighDegree, the code falls through to homotopy at Line 10626 and returns numeric points. The captured conditions then describe the failed symbolic attempt, not the returned answer. Clear the captured state on that fallback path.Proposed fix in the numeric fallback branch
if numeric { if let Err(alkahest_core::SolverError::HighDegree(_)) = &result { + // The returned points come from homotopy, so the hypotheses the + // abandoned symbolic attempt assumed do not describe them. + reset_solve_side_conditions(); let opts = HomotopyOpts::default();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@alkahest-py/src/lib.rs` around lines 10614 - 10622, Clear the captured solve-side-condition state when the numeric fallback handles SolverError::HighDegree and proceeds to homotopy, before returning the numeric points. Update the fallback logic surrounding solve_polynomial_system and preserve captured conditions for successful symbolic results and non-fallback errors.tests/test_agent_contract.py-174-177 (1)
174-177: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the CUDA entry-point set with
tests/test_cuda.py.This tuple includes
CudaError.tests/test_cuda.pyLine 43 excludes it and documents at Lines 45-48 that the native module registers it unconditionally, so it resolves on every build. The two files now define the CUDA entry-point set differently. Becauseall(...)requires every name, a regression that droppedCudaErrorfrom a non-CUDA build would still satisfy this assertion whiletest_cuda.pyfails. DropCudaErrorhere and keep the exception coverage in the dedicated test.Proposed alignment
features = alkahest.capabilities()["features"] - reachable = all( - hasattr(alkahest, name) for name in ("compile_cuda", "CudaCompiledFn", "CudaError") - ) + # `CudaError` is deliberately excluded: the native module registers it + # unconditionally, so it resolves on every build. See tests/test_cuda.py. + reachable = all(hasattr(alkahest, name) for name in ("compile_cuda", "CudaCompiledFn")) assert features["cuda"] == reachable, (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_agent_contract.py` around lines 174 - 177, Update the CUDA reachability check in the features assertion to consider only compile_cuda and CudaCompiledFn, matching the entry-point set used by tests/test_cuda.py. Remove CudaError from this tuple and leave its validation to the dedicated CUDA test.alkahest-py/src/lib.rs-3611-3629 (1)
3611-3629: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd
E-RESIDUE-005todocs/sphinx/api/errors.rst.
docs/mdbook/src/errors.mdalready documents the code. The Sphinx error documentation does not reference it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@alkahest-py/src/lib.rs` around lines 3611 - 3629, Update the Sphinx error reference in errors.rst to document E-RESIDUE-005, matching the existing description and guidance for residue point validation already present in the other error documentation. Keep the entry aligned with the surrounding error-code entries.Source: Coding guidelines
alkahest-core/tests/groebner_cuda.rs-75-84 (1)
75-84: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the
fell_back_to_cpu()assertionA CPU-only run can perform zero reductions, so
fell_back_to_cpu()can be false. The existingrequested_device == Noneandreductions_on_gpu == 0checks already enforce the CPU-only path. Do not replace the assertion with a condition that is always true.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@alkahest-core/tests/groebner_cuda.rs` around lines 75 - 84, Remove the assert!(backend.fell_back_to_cpu(), ...) check from assert_no_gpu. Keep the requested_device, reductions_on_gpu, first_gpu_error, and ran_on_gpu assertions unchanged; do not replace the removed assertion with another unconditional condition.python/alkahest/__init__.py-384-388 (1)
384-388: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRebind
CudaErrorinalkahest.exceptionsto the native class. The overlay updates only package globals, soalkahest.exceptions.CudaErrorremains distinct and does not catch native CUDA raises.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/alkahest/__init__.py` around lines 384 - 388, Update the native overlay logic that handles the unconditionally registered CudaError to also rebind alkahest.exceptions.CudaError to the native exception class, keeping the package-level CudaError binding synchronized across CPU-only and CUDA builds.alkahest-core/src/sum/mod.rs-172-184 (1)
172-184: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the comment with the range the guard actually checks.
The comment says a pole "strictly between the bounds" and names the function
interior_undefined_index.interior_undefined_indexscanslo_i..=hi_iinclusive (line 327) andinteger_roots_inacceptsroot >= lo && root <= hi(line 280). Endpoint poles are therefore refused too. Including the endpoints is the correct behavior, becauseΣ_{k=0}^{5} 1/khas no value either. Update the wording so a later reader does not narrow the range to match the word "interior".📝 Proposed comment change
- // A pole of the *summand* strictly between the bounds is invisible in the - // telescoped difference — `G(hi+1) − G(lo)` never mentions the interior - // indices — so it has to be looked for in the summand itself, the same way + // A pole of the *summand* at any index in `[lo, hi]` — endpoints included — + // is invisible in the telescoped difference, which never mentions the + // individual indices, so it has to be looked for in the summand itself, the same way🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@alkahest-core/src/sum/mod.rs` around lines 172 - 184, Update the explanatory comment above interior_undefined_index to describe poles at any index within the inclusive summation bounds, including lo and hi, rather than only poles strictly between them. Keep the guard and error behavior unchanged, and retain the distinction that the summand itself must be checked because telescoping can hide undefined terms.alkahest-core/src/solver/verify.rs-117-124 (1)
117-124: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the exponent negation against
i64::MIN.Line 289 converts the exponent with
to_i64(), soncan bei64::MIN.-nat Line 122 then overflows: a debug build panics, and a release build wraps toi64::MINand recurses until the stack is exhausted. Usechecked_negand report the exponent as unrepresentable.🛡️ Proposed fix
fn powi(&self, n: i64) -> Option<CBall> { if n == 0 { return Some(CBall::one()); } if n < 0 { - let pos = self.powi(-n)?; + let pos = self.powi(n.checked_neg()?)?; return pos.recip(); }
checked_negreturnsNonefori64::MIN, whicheval_uncachedalready maps toVerifyGap::Undefined. IfUnsupportedis the better report for an unrepresentable exponent, reject the exponent at Line 289 instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@alkahest-core/src/solver/verify.rs` around lines 117 - 124, Update powi to use checked_neg when handling negative exponents, returning None when negation is unavailable for i64::MIN so eval_uncached can report VerifyGap::Undefined instead of recursing. Preserve the existing reciprocal flow for representable negative exponents.alkahest-core/src/solver/mod.rs-72-75 (1)
72-75: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winQualify the
Finitepost-condition for parametric tuples.Line 73 states that every tuple survived substitution into the input equations.
refine_solutionskeeps tuples that fail to evaluate withVerifyGap::Unsupportedwithout any check (Line 849), andsolve_polynomial_system's own documentation says a parametric tuple "is returned unverified" (Line 919). Parametric results are returned insideFinite— see the tests at Lines 1295-1340. Restrict the claim to parameter-free tuples so callers do not rely on a guarantee that the parametric path does not provide.📝 Proposed documentation fix
/// Finitely many solutions (each is a `Vec<ExprId>` parallel to `vars`). /// - /// Every tuple has survived substitution back into the input equations, so - /// a returned solution is never one the solver can itself refute. + /// Every **parameter-free** tuple has survived substitution back into the + /// input equations, so it is never one the solver can itself refute. A + /// tuple that mentions a free parameter cannot be substituted numerically + /// and is returned unverified, under the hypotheses reported by + /// [`take_solve_side_conditions`].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@alkahest-core/src/solver/mod.rs` around lines 72 - 75, Update the documentation for the Finite variant to qualify its substitution and refutation guarantee as applying only to parameter-free tuples. Make clear that tuples containing parameters may be returned unverified, while preserving the existing guarantee for fully concrete solutions.
🧹 Nitpick comments (15)
tests/test_series_v215.py (1)
124-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a structural assertion over the printed form.
Line 132 matches
"x^23"instr(s.expr). That couples the test to the printer output. This file already walks the node tree in_has_big_o, so the highest power can be checked structurally and stays stable if the printer changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_series_v215.py` around lines 124 - 132, Replace the string-based `"x^23"` assertion in test_ordinary_high_order_series_still_expands with a structural node-tree assertion, reusing the existing traversal approach from _has_big_o to verify that the expansion contains the highest odd power x^23 without depending on printer formatting.tests/test_parametric_solve.py (1)
112-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider marking the side-condition tests as non-parallel-safe.
ak.solve_side_conditions()reads state that the lastsolvecall wrote. These three tests assert that state across separate calls. If the suite ever runs with thread-level parallelism inside one process, an interleavedsolvefrom another test overwrites the channel and makes these assertions flaky. A marker or a short comment that records the requirement keeps that constraint explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_parametric_solve.py` around lines 112 - 121, The side-condition tests rely on process-global state written by solve, so mark the affected tests as non-parallel-safe or add a concise comment documenting that requirement. Apply this to test_hypotheses_do_not_leak_from_an_earlier_solve and the other tests asserting ak.solve_side_conditions() across separate solve calls.tests/silent_errors/corpus.py (1)
1744-1747: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the shared
_Zsymbol.
_Zis defined at line 703 for the third variable. This case createsPOOL.symbol("z")twice inline. Reusing_Zkeeps one name for one symbol.♻️ Proposed change
- op=solution_count( - [X ** _int(2), Y ** _int(2), POOL.symbol("z") ** _int(2)], - [X, Y, POOL.symbol("z")], - ), + op=solution_count( + [X ** _int(2), Y ** _int(2), _Z ** _int(2)], + [X, Y, _Z], + ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/silent_errors/corpus.py` around lines 1744 - 1747, Update the solution_count call to reuse the shared _Z symbol for the third variable and its squared expression, replacing both inline POOL.symbol("z") constructions while leaving the existing X and Y arguments unchanged.tests/test_residue.py (1)
96-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse one call form, and consider a slow marker.
Lines 105 and 110 call the same function through two names: the module-level
residueimport andak.residue. Use one form for consistency. The test also builds a 300-level nested expression and then runs a successful residue over it. If that path takes noticeable time, mark the test with@pytest.mark.slow, which the defaultpytest tests/run excludes.♻️ Proposed consistency change
- assert ak.residue(deep, z, 0) is not None + assert residue(deep, z, 0) is not NoneAs per coding guidelines: "Run
pytest tests/for Python test suite (default excludes@pytest.mark.slow)".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_residue.py` around lines 96 - 110, Use a single consistent call form in test_deeply_nested_polynomial_point_still_refused_cleanly, replacing either the imported residue call or ak.residue call so both assertions invoke the same symbol. If the successful residue evaluation over the 300-level expression is noticeably slow, add the existing pytest slow marker to this test so the default test suite excludes it.Source: Coding guidelines
alkahest-py/src/lib.rs (1)
9589-9623: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider releasing the GIL around the CUDA launch.
eval_on_devicecallscall_batch_on, which loads a module and launches a kernel while holding the GIL. A large batch therefore blocks every other Python thread.py_seriesat Line 3745 already usespy.allow_threadsfor the same reason. The input columns are already copied into ownedVec<f64>before the call, so the launch section can move outside the GIL.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@alkahest-py/src/lib.rs` around lines 9589 - 9623, Update PyCudaCompiledFn::eval_on_device to wrap the CUDA module-loading and kernel-launch call to self.inner.call_batch_on in Python::allow_threads, keeping the owned input and output buffers outside the closure as needed and preserving existing PyCudaError conversion after the call.tests/test_cuda.py (1)
79-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe broad
except Exceptioncan hide a binding regression as "no device".Any exception from
compile_cudaorcall_batchmarks the device tier as unavailable. On a CUDA build withoutALKAHEST_GPU_TESTS=1, a defect in the binding then silently skips all of tier 3 instead of failing. Narrow the probe so only driver and device errors count as unavailable, and let other exception types propagate.Proposed narrowing of the availability probe
try: fn = ak.compile_cuda(x + pool.integer(1), [x]) fn.call_batch([[2.0]]) - except Exception as exc: # any failure at all means "no usable device" + except ak.CudaError as exc: + # Only a CUDA-layer failure means "no usable device". A TypeError or a + # ValueError from the binding is a defect for the tests to report. return f"no usable CUDA device: {type(exc).__name__}: {exc}" return None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cuda.py` around lines 79 - 84, Update the CUDA availability probe around ak.compile_cuda and fn.call_batch to catch only the established driver/device-related exception types and return the unavailable-device message for those cases. Allow all other exceptions, including binding regressions, to propagate instead of classifying them as an unavailable CUDA device.alkahest-core/src/sum/mod.rs (1)
326-338: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMake the brute-force candidate scan honour the budget.
When any negative-power base fails to parse as a polynomial in
k,unparsedbecomestrueand the candidate list grows to the whole range, up to 2048 indices. Each candidate then runssubsplus a fullsimp. A summand such as1/Γ(k)reaches this path, so up to 2048 simplifications run beforesum_definitereturns.MAX_POLE_SCANbounds the work but does not make it cheap, and this crate already has a cooperative budget (crate::budget::check, used inalkahest-core/src/calculus/series.rs). Add a budget check inside the loop so a caller with an active budget can stop the scan.♻️ Proposed refactor
for j in candidates { + if crate::budget::check().is_err() { + // No opinion rather than a slow answer; the caller's budget owns the + // decision, and `None` never claims "no pole". + return None; + } let mut m = HashMap::new(); m.insert(k, pool.integer(j));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@alkahest-core/src/sum/mod.rs` around lines 326 - 338, Add a cooperative budget check inside the candidate loop over `candidates`, using the existing `crate::budget::check` mechanism as used by the series code, so each expensive `subs`/`simp` iteration can stop when the caller’s budget is exhausted. Preserve candidate ordering, deduplication, and the existing `Some(j)` result behavior.alkahest-core/src/calculus/series.rs (1)
381-393: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the
E-SERIES-003remediation to the error reference. The entry describes the refusal but does not state the user action returned bySeriesRefusal::remediation(): ask for a lower order, raise the budget, or rewrite the expression.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@alkahest-core/src/calculus/series.rs` around lines 381 - 393, Add an E-SERIES-003 entry to the error reference documenting the remediation returned by SeriesRefusal::remediation(): ask for a lower order, raise the budget, or rewrite the expression so repeated derivatives close. Keep the existing refusal description and align the wording with the remediation string.Source: Coding guidelines
alkahest-core/src/simplify/rules.rs (1)
1420-1423: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve expansion-limit details in the derivation step.
take_expand_limitsrecords(node, exponent, summands), butexpand_limit_logdiscards the latter two values. Extend the derivation-step representation and pass both values through so.stepsidentifies the bound that stopped expansion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@alkahest-core/src/simplify/rules.rs` around lines 1420 - 1423, Update the derivation-step representation and the expand-limit handling around take_expand_limits and expand_limit_log to retain and propagate both exponent and summands instead of discarding them. Ensure generated .steps entries identify the expansion bound that prevented expansion.alkahest-core/src/solver/verify.rs (2)
219-230: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAssert the non-negativity precondition of
clamp_nonneg.If
b.hi()is negative, this function returns anArbBallwith a negativerad. Every current call site passes a quantity that is non-negative by construction, so the case does not occur. A negative radius would silently break every later separation test, so enforce the stated precondition in debug builds.♻️ Proposed refactor
fn clamp_nonneg(b: ArbBall) -> ArbBall { if b.lo() >= 0 { return b; } let hi = b.hi(); + debug_assert!(hi >= 0, "clamp_nonneg requires a ball whose upper end is non-negative"); let mid = rug::Float::with_val(VERIFY_PREC, &hi / 2u32);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@alkahest-core/src/solver/verify.rs` around lines 219 - 230, Add a debug-build assertion at the start of clamp_nonneg validating that b.hi() is non-negative, while preserving the existing return and clamping behavior for valid inputs.
155-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompute
im_is_exact_zeroonce.Lines 155 and 174 evaluate the same predicate on the same fields. Hoist it above the fast path.
♻️ Proposed refactor
fn sqrt(&self) -> Option<CBall> { + // `im` is exactly zero for every discriminant built from rationals, + // which is the case that has to stay sharp. + let im_is_exact_zero = self.im.is_exact() && self.im.mid_f64() == 0.0; // A real argument keeps the result on one axis *exactly*, which is what // preserves the distinction between the two roots further up. - let im_is_exact_zero = self.im.is_exact() && self.im.mid_f64() == 0.0; if im_is_exact_zero { @@ let v = clamp_nonneg((modulus - self.re.clone()) * half).sqrt()?; - // `im` is exactly zero for every discriminant built from rationals, - // which is the case that has to stay sharp. - let im_is_exact_zero = self.im.is_exact() && self.im.mid_f64() == 0.0; let im = if im_is_exact_zero || self.im.lo() > 0 {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@alkahest-core/src/solver/verify.rs` around lines 155 - 181, Compute the im_is_exact_zero predicate once before the fast-path condition in the square-root logic, then reuse that binding for the later imaginary-part selection. Remove the duplicate declaration while preserving the existing branches and behavior.alkahest-core/src/solver/polyhedral.rs (1)
395-403: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrack the dead mixed-cell enumeration explicitly.
The documentation now states that
polyhedral_cell_iterreturns an empty list for every input. The whole polyhedral branch insolve_numericalis therefore unreachable work: it computes hulls and edges on every call and always falls back. The refusal-and-fallback behavior is correct, so this is not a defect. Recording it as a tracked limitation keeps the dead branch from being read as working code.Do you want me to open an issue for the mixed-cell criterion so the fallback can be removed later?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@alkahest-core/src/solver/polyhedral.rs` around lines 395 - 403, Track the known limitation in the polyhedral solver by adding an explicit TODO or issue reference near polyhedral_cell_iter and its use in solve_numerical. Document that mixed-cell enumeration currently yields no starts, so solve_numerical intentionally retains the Bézout fallback; do not alter the refusal-and-fallback behavior.alkahest-core/src/poly/resultant.rs (2)
411-429: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEnforce the documented degree precondition.
Line 426 subtracts
qc.len()frompc.len(). The preconditiondeg(p) >= deg(q)holds today becausesubresultant_prsswaps first, so this is not a current defect. If a later caller skips the swap, the subtraction underflows: a debug build panics, and a release build wraps to a hugeu32that makesrug_powallocate without bound. Make the precondition executable.🛡️ Proposed guard
// `deg q < 0` (q = 0) or `deg q = 0`: the chain `S_j`, `0 ≤ j < deg q`, is // empty, so there is nothing to append. if qc.len() <= 1 || pc.is_empty() { return Some(sequence); } + // Documented precondition, made executable: a caller that skips the + // canonical swap would otherwise underflow the exponent below. + let deg_gap = pc.len().checked_sub(qc.len())?; // s = lc(q)^(deg p − deg q); A = q; B = prem(p, −q). - let mut s = rug_pow(&qc[qc.len() - 1], (pc.len() - qc.len()) as u32); + let mut s = rug_pow(&qc[qc.len() - 1], deg_gap as u32);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@alkahest-core/src/poly/resultant.rs` around lines 411 - 429, Enforce the deg(p) >= deg(q) precondition at the start of sprs_inner before computing pc.len() - qc.len(). Return None for inputs where p has lower degree than q, while preserving the existing zero/constant-q handling and valid resultant sequence behavior.
321-331: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
rug::Integer::powfor integer exponentiation.Import
rug::ops::Powand replace the loop withbase.clone().pow(exp). This usesrug 1.30.0’sPow<u32>implementation and avoidsexp - 1sequential multiplications.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@alkahest-core/src/poly/resultant.rs` around lines 321 - 331, Update rug_pow to import rug::ops::Pow and replace the manual exponentiation loop with base.clone().pow(exp), preserving the existing non-negative u32 exponent behavior and zero-exponent result.alkahest-core/src/ideal/primary.rs (1)
776-797: 🚀 Performance & Scalability | 🔵 TrivialMeasure the Gröbner basis cost of the certified radical path.
radical_zero_dimensionalcomputes one GRevLex basis at Line 787 and up tonfurther Lex bases througheliminant_in_varat Line 810.decompose_recursivecallsradical_directat Line 564 on every leaf that reaches the maximal-radical attempt, and that recursion runs toMAX_SPLIT_DEPTH = 48through the saturation split, which computes several bases per variable itself.The cost is inherent to the algorithm, and the refusal contract is the right trade. Two operational suggestions:
- Record the basis-computation count or wall time for the certified paths, so a regression in the CI gate is attributable.
- Consider threading the GRevLex basis and the per-variable eliminants through the recursion, so a repeated leaf does not recompute them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@alkahest-core/src/ideal/primary.rs` around lines 776 - 797, Instrument the certified path in radical_zero_dimensional to record Gröbner-basis computation count or wall time, including the initial GRevLex computation and each eliminant_in_var computation, using the project’s existing measurement mechanism. Ensure measurements cover calls reached through radical_direct and decompose_recursive so CI can attribute regressions to repeated leaf work. Reuse already computed bases or eliminants across recursion only if the existing interfaces support it without changing the refusal contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 169-176: Restrict workflow permissions by adding a top-level
permissions block granting only contents: read, and update the checkout step in
the asan job to disable persisted credentials via persist-credentials: false.
Apply the checkout setting to any other checkout steps that do not require
authenticated Git commands.
- Around line 532-535: Update the Valgrind pipeline in the CI workflow to
preserve the analyzer’s exit status instead of allowing tee to mask failures.
Enable pipefail before the valgrind/tee command, or capture PIPESTATUS[0]
immediately afterward and exit with that status; keep the existing output
capture and Valgrind arguments unchanged.
In @.github/workflows/cuda_nightly.yml:
- Around line 98-104: Ensure the “Run compute-sanitizer racecheck” workflow step
explicitly sets continue-on-error to false so racecheck and sanitizer startup
failures fail the workflow.
In `@alkahest-core/src/simplify/rules.rs`:
- Around line 1473-1475: Update the expansion guard around expansion_products
and MAX_EXPAND_POW_EXP so the product budget is enforced independently of the
compatibility exponent floor. Preserve acceptance for exponents at or below
MAX_EXPAND_POW_EXP only when expansion_products(summands.len(), n_u32) remains
within MAX_EXPAND_POW_PRODUCTS, preventing wide bases from triggering unbounded
distribute_once work.
In `@docs/mdbook/src/errors.md`:
- Around line 94-101: Update the errors documentation section to identify the
Python-facing exception as AlkahestError rather than the Rust-side
PrimaryDecompositionError or IdealRefusal, and document codes E-IDEAL-005 and
E-IDEAL-006 with their .code, .remediation, and .span fields. Remove any claim
that these calls raise an uncoded ValueError, while noting that AlkahestError
remains catchable as ValueError.
In `@docs/mdbook/src/telescoping.md`:
- Around line 25-32: In the telescoping derivation, define the summation
endpoints k_lo and k_hi before introducing the recurrence, including any
dependence on n. Update the shifted-sum argument to account for n-dependent
endpoints via zero-extension or endpoint corrections, and evaluate both boundary
terms G(n, k_hi+1) and G(n, k_lo) before concluding that their difference
vanishes.
---
Outside diff comments:
In `@alkahest-core/src/simplify/engine.rs`:
- Around line 376-405: Use an invocation-scoped expansion-limit collector:
update alkahest-core/src/simplify/engine.rs lines 376-405 to isolate each
top-level simplify invocation while preserving outer events across nested calls,
and update alkahest-core/src/simplify/redex.rs lines 110-129 so ExpandPow worker
events are transferred into the current pass log rather than relying only on
caller-thread expand_limit_log() draining. Ensure outer invocation events are
not cleared or drained by nested simplify calls.
---
Minor comments:
In `@alkahest-core/src/solver/mod.rs`:
- Around line 72-75: Update the documentation for the Finite variant to qualify
its substitution and refutation guarantee as applying only to parameter-free
tuples. Make clear that tuples containing parameters may be returned unverified,
while preserving the existing guarantee for fully concrete solutions.
In `@alkahest-core/src/solver/verify.rs`:
- Around line 117-124: Update powi to use checked_neg when handling negative
exponents, returning None when negation is unavailable for i64::MIN so
eval_uncached can report VerifyGap::Undefined instead of recursing. Preserve the
existing reciprocal flow for representable negative exponents.
In `@alkahest-core/src/sum/mod.rs`:
- Around line 172-184: Update the explanatory comment above
interior_undefined_index to describe poles at any index within the inclusive
summation bounds, including lo and hi, rather than only poles strictly between
them. Keep the guard and error behavior unchanged, and retain the distinction
that the summand itself must be checked because telescoping can hide undefined
terms.
In `@alkahest-core/tests/groebner_cuda.rs`:
- Around line 75-84: Remove the assert!(backend.fell_back_to_cpu(), ...) check
from assert_no_gpu. Keep the requested_device, reductions_on_gpu,
first_gpu_error, and ran_on_gpu assertions unchanged; do not replace the removed
assertion with another unconditional condition.
In `@alkahest-py/src/lib.rs`:
- Around line 10614-10622: Clear the captured solve-side-condition state when
the numeric fallback handles SolverError::HighDegree and proceeds to homotopy,
before returning the numeric points. Update the fallback logic surrounding
solve_polynomial_system and preserve captured conditions for successful symbolic
results and non-fallback errors.
- Around line 3611-3629: Update the Sphinx error reference in errors.rst to
document E-RESIDUE-005, matching the existing description and guidance for
residue point validation already present in the other error documentation. Keep
the entry aligned with the surrounding error-code entries.
In `@docs/mdbook/src/codegen.md`:
- Around line 164-176: Update the CUDA example around compile_cuda to bind the
alkahest module before calling alkahest.capabilities(), either by importing
alkahest or importing capabilities directly. Preserve the existing feature-guard
behavior and CUDA compilation flow.
In `@docs/mdbook/src/gpu.md`:
- Around line 100-115: Update the device_count helper to avoid interpreting
every ak.CudaError from call_batch_on as an invalid ordinal; restrict
termination to an ordinal-specific error if available, or revise the
documentation and helper semantics to clearly identify the result as a
best-effort probe rather than an authoritative device count.
- Around line 53-58: Update the CUDA capability description near the `cuda ==
True` statement to remove the promise that PTX can be emitted on the host. Keep
the guarantee limited to the presence of `ak.compile_cuda` and
`ak.CudaCompiledFn`, and preserve the existing distinction between capability
linkage and runtime GPU availability.
In `@docs/mdbook/src/telescoping.md`:
- Around line 62-70: Complete the boundary-check example after the existing
g_at_lo calculation by evaluating cert.boundary_term at k_hi + 1, then compare
the upper and lower boundary values to verify their difference is zero. Keep the
example aligned with the documented G(n, k_hi+1) - G(n, k_lo) check and use the
existing symbols cert.boundary_term, k_hi, and k_lo.
In `@docs/sphinx/api/errors.rst`:
- Around line 170-182: Update the refusal-code index at the section around the
existing lines 50–51 to include E-SOLVE-004, E-IDEAL-005, and E-IDEAL-006. Keep
the existing index structure and descriptions consistent with the refusal
entries documented near triangularize, radical, and primary_decomposition.
- Around line 170-182: Synchronize the refusal documentation with binding
behavior: add E-IDEAL-005, E-IDEAL-006, and E-SOLVE-004 to the Sphinx refusal
list, update the note describing radical and primary_decomposition to match the
binding-exposed refusal records, and revise the corresponding stale mdBook
paragraph. Preserve the documented code meanings and reference the existing
take_ideal_refusal() and take_triangularize_refusal() symbols.
In `@python/alkahest/__init__.py`:
- Around line 384-388: Update the native overlay logic that handles the
unconditionally registered CudaError to also rebind
alkahest.exceptions.CudaError to the native exception class, keeping the
package-level CudaError binding synchronized across CPU-only and CUDA builds.
In `@python/alkahest/exceptions.py`:
- Around line 60-63: Update the pure-Python fallback assignment for
_NativeAlkahestError in exceptions.py to use ValueError instead of Exception,
preserving the native AlkahestError inheritance contract so Python-raised
wrappers remain catchable by except ValueError.
In `@tests/silent_errors/corpus.py`:
- Around line 631-639: Update the exception handling in op so KeyboardInterrupt
and SystemExit are re-raised before the broad BaseException conversion; preserve
converting other BaseException subclasses, including PanicException, into
RuntimeError.
- Around line 812-824: Update the side-condition check in op to use a
regular-expression word-boundary match for hypothesis_about rather than
substring containment, and add the required re import. Preserve the stated path
only when a complete identifier/word match is present.
In `@tests/test_agent_contract.py`:
- Around line 174-177: Update the CUDA reachability check in the features
assertion to consider only compile_cuda and CudaCompiledFn, matching the
entry-point set used by tests/test_cuda.py. Remove CudaError from this tuple and
leave its validation to the dedicated CUDA test.
---
Nitpick comments:
In `@alkahest-core/src/calculus/series.rs`:
- Around line 381-393: Add an E-SERIES-003 entry to the error reference
documenting the remediation returned by SeriesRefusal::remediation(): ask for a
lower order, raise the budget, or rewrite the expression so repeated derivatives
close. Keep the existing refusal description and align the wording with the
remediation string.
In `@alkahest-core/src/ideal/primary.rs`:
- Around line 776-797: Instrument the certified path in radical_zero_dimensional
to record Gröbner-basis computation count or wall time, including the initial
GRevLex computation and each eliminant_in_var computation, using the project’s
existing measurement mechanism. Ensure measurements cover calls reached through
radical_direct and decompose_recursive so CI can attribute regressions to
repeated leaf work. Reuse already computed bases or eliminants across recursion
only if the existing interfaces support it without changing the refusal
contract.
In `@alkahest-core/src/poly/resultant.rs`:
- Around line 411-429: Enforce the deg(p) >= deg(q) precondition at the start of
sprs_inner before computing pc.len() - qc.len(). Return None for inputs where p
has lower degree than q, while preserving the existing zero/constant-q handling
and valid resultant sequence behavior.
- Around line 321-331: Update rug_pow to import rug::ops::Pow and replace the
manual exponentiation loop with base.clone().pow(exp), preserving the existing
non-negative u32 exponent behavior and zero-exponent result.
In `@alkahest-core/src/simplify/rules.rs`:
- Around line 1420-1423: Update the derivation-step representation and the
expand-limit handling around take_expand_limits and expand_limit_log to retain
and propagate both exponent and summands instead of discarding them. Ensure
generated .steps entries identify the expansion bound that prevented expansion.
In `@alkahest-core/src/solver/polyhedral.rs`:
- Around line 395-403: Track the known limitation in the polyhedral solver by
adding an explicit TODO or issue reference near polyhedral_cell_iter and its use
in solve_numerical. Document that mixed-cell enumeration currently yields no
starts, so solve_numerical intentionally retains the Bézout fallback; do not
alter the refusal-and-fallback behavior.
In `@alkahest-core/src/solver/verify.rs`:
- Around line 219-230: Add a debug-build assertion at the start of clamp_nonneg
validating that b.hi() is non-negative, while preserving the existing return and
clamping behavior for valid inputs.
- Around line 155-181: Compute the im_is_exact_zero predicate once before the
fast-path condition in the square-root logic, then reuse that binding for the
later imaginary-part selection. Remove the duplicate declaration while
preserving the existing branches and behavior.
In `@alkahest-core/src/sum/mod.rs`:
- Around line 326-338: Add a cooperative budget check inside the candidate loop
over `candidates`, using the existing `crate::budget::check` mechanism as used
by the series code, so each expensive `subs`/`simp` iteration can stop when the
caller’s budget is exhausted. Preserve candidate ordering, deduplication, and
the existing `Some(j)` result behavior.
In `@alkahest-py/src/lib.rs`:
- Around line 9589-9623: Update PyCudaCompiledFn::eval_on_device to wrap the
CUDA module-loading and kernel-launch call to self.inner.call_batch_on in
Python::allow_threads, keeping the owned input and output buffers outside the
closure as needed and preserving existing PyCudaError conversion after the call.
In `@tests/silent_errors/corpus.py`:
- Around line 1744-1747: Update the solution_count call to reuse the shared _Z
symbol for the third variable and its squared expression, replacing both inline
POOL.symbol("z") constructions while leaving the existing X and Y arguments
unchanged.
In `@tests/test_cuda.py`:
- Around line 79-84: Update the CUDA availability probe around ak.compile_cuda
and fn.call_batch to catch only the established driver/device-related exception
types and return the unavailable-device message for those cases. Allow all other
exceptions, including binding regressions, to propagate instead of classifying
them as an unavailable CUDA device.
In `@tests/test_parametric_solve.py`:
- Around line 112-121: The side-condition tests rely on process-global state
written by solve, so mark the affected tests as non-parallel-safe or add a
concise comment documenting that requirement. Apply this to
test_hypotheses_do_not_leak_from_an_earlier_solve and the other tests asserting
ak.solve_side_conditions() across separate solve calls.
In `@tests/test_residue.py`:
- Around line 96-110: Use a single consistent call form in
test_deeply_nested_polynomial_point_still_refused_cleanly, replacing either the
imported residue call or ak.residue call so both assertions invoke the same
symbol. If the successful residue evaluation over the 300-level expression is
noticeably slow, add the existing pytest slow marker to this test so the default
test suite excludes it.
In `@tests/test_series_v215.py`:
- Around line 124-132: Replace the string-based `"x^23"` assertion in
test_ordinary_high_order_series_still_expands with a structural node-tree
assertion, reusing the existing traversal approach from _has_big_o to verify
that the expansion contains the highest odd power x^23 without depending on
printer formatting.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c4c1926b-8ab2-46b9-a2e0-32befda8238c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (66)
.github/workflows/alkahest-semver-check.yml.github/workflows/ci-cross.yml.github/workflows/ci.yml.github/workflows/codspeed.yml.github/workflows/cuda_nightly.yml.github/workflows/docs.yml.github/workflows/release-build.ymlCHANGELOG.mdCargo.tomlREADME.mdalkahest-core/src/calculus/euler_maclaurin.rsalkahest-core/src/calculus/limits.rsalkahest-core/src/calculus/mod.rsalkahest-core/src/calculus/series.rsalkahest-core/src/errors/codes.rsalkahest-core/src/holonomic/mod.rsalkahest-core/src/holonomic/zeilberger.rsalkahest-core/src/ideal/mod.rsalkahest-core/src/ideal/primary.rsalkahest-core/src/integrate/risch/tower_integrate.rsalkahest-core/src/lattice/lll.rsalkahest-core/src/lib.rsalkahest-core/src/poly/groebner/cuda.rsalkahest-core/src/poly/groebner/mod.rsalkahest-core/src/poly/resultant.rsalkahest-core/src/primitive/mod.rsalkahest-core/src/simplify/engine.rsalkahest-core/src/simplify/parallel.rsalkahest-core/src/simplify/redex.rsalkahest-core/src/simplify/rules.rsalkahest-core/src/solver/homotopy.rsalkahest-core/src/solver/mod.rsalkahest-core/src/solver/polyhedral.rsalkahest-core/src/solver/regular_chains.rsalkahest-core/src/solver/verify.rsalkahest-core/src/sum/mod.rsalkahest-core/src/sum/product.rsalkahest-core/src/sum/recurrence.rsalkahest-core/src/sum/rsolve.rsalkahest-core/tests/groebner_cuda.rsalkahest-py/Cargo.tomlalkahest-py/src/lib.rsalkahest-skill/alkahest.mddocs/features.mddocs/mdbook/src/SUMMARY.mddocs/mdbook/src/codegen.mddocs/mdbook/src/errors.mddocs/mdbook/src/getting-started.mddocs/mdbook/src/gpu.mddocs/mdbook/src/interop.mddocs/mdbook/src/solving.mddocs/mdbook/src/telescoping.mddocs/sphinx/api/errors.rstpyproject.tomlpython/alkahest/__init__.pypython/alkahest/exceptions.pytests/silent_errors/corpus.pytests/test_agent_contract.pytests/test_batch_workload.pytests/test_cuda.pytests/test_parametric_solve.pytests/test_primary_decomposition_v212.pytests/test_regular_chains_v211.pytests/test_residue.pytests/test_series_v215.pytests/textbook_gate/test_tg_solve.py
| asan: | ||
| name: AddressSanitizer — alkahest-cas | ||
| runs-on: ubuntu-latest | ||
| if: github.event_name != 'schedule' | ||
| timeout-minutes: 60 | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v6 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Restrict token access in the ASan job.
This job executes PR-controlled Cargo build scripts after checkout. The workflow has no explicit permissions block, and actions/checkout persists its token by default. An internal PR can read and use that credential.
Add workflow-level permissions: contents: read. Set persist-credentials: false for checkout steps that do not need authenticated Git commands.
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 176-176: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 169-219: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 169 - 176, Restrict workflow
permissions by adding a top-level permissions block granting only contents:
read, and update the checkout step in the asan job to disable persisted
credentials via persist-credentials: false. Apply the checkout setting to any
other checkout steps that do not require authenticated Git commands.
Source: Linters/SAST tools
| valgrind --leak-check=full --error-exitcode=1 \ | ||
| --suppressions=valgrind.supp "$bin" 2>&1 | ||
| --errors-for-leak-kinds=definite,indirect \ | ||
| --show-leak-kinds=definite,indirect,possible \ | ||
| --suppressions=valgrind.supp "$bin" "${filters[@]}" 2>&1 | tee /tmp/vg-out.txt |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- shell declarations ---'
rg -n '(^|[[:space:]])shell:|pipefail|valgrind|vg-out' .github/workflows/ci.yml
printf '%s\n' '--- workflow lines 480-550 ---'
sed -n '480,550p' .github/workflows/ci.yml
printf '%s\n' '--- workflow/job headers ---'
sed -n '1,230p' .github/workflows/ci.ymlRepository: alkahest-cas/alkahest
Length of output: 14680
🌐 Web query:
GitHub Actions default shell run step Ubuntu bash pipefail official documentation
💡 Result:
In GitHub Actions, the shell behavior for run steps on Ubuntu (and other non-Windows runners) depends on whether you explicitly specify the shell [1][2]. When the shell is not explicitly specified (unspecified), GitHub Actions uses a default shell that is functionally equivalent to bash -e {0} [1][2][3]. This command includes the -e flag (errexit), which ensures the step fails if any command returns a non-zero exit code [3], but it does not include pipefail by default [3][4]. When you explicitly specify shell: bash, GitHub Actions executes the step using the command bash --noprofile --norc -eo pipefail {0} [1][2]. This explicitly includes the pipefail option, which ensures that the exit status of a pipeline is the value of the last (rightmost) command to exit with a non-zero status, or zero if all commands in the pipeline exit successfully [5]. Summary of behavior for Ubuntu/Linux runners: 1. Unspecified shell: Uses bash -e {0}. It has fail-fast behavior (-e), but does not have pipefail enabled [1][3][4]. 2. Explicit shell: bash: Uses bash --noprofile --norc -eo pipefail {0}. This includes both -e and pipefail [1][2][5]. If you require pipefail behavior in your workflow, you should explicitly set shell: bash in your step or job defaults [6][4][5].
Citations:
- 1: https://docs.github.com/actions/using-workflows/workflow-syntax-for-github-actions
- 2: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax?use_case=
- 3: [GitHub Actions] Fail fast from the default shells (bash/sh) in Linux/macOS github/docs#23853
- 4: Default
bashshell doesn't seem to run with-o pipefailactions/runner-images#4459 - 5: Document that
pipefailis already on undershell: bashmlflow/mlflow#23155 - 6: https://docs.github.com/actions/writing-workflows/choosing-what-your-workflow-does/setting-a-default-shell-and-working-directory
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- bash pipeline status without pipefail ---'
bash -c 'set -e; printf "valgrind failure\n" >&2; false | tee /tmp/coderabbit-vg-out.txt; printf "step continued\n"' 2>&1
printf 'exit=%s\n' "$?"
printf '%s\n' '--- bash pipeline status with pipefail ---'
bash -c 'set -e -o pipefail; printf "valgrind failure\n" >&2; false | tee /tmp/coderabbit-vg-out.txt; printf "step continued\n"' 2>&1
printf 'exit=%s\n' "$?"
printf '%s\n' '--- explicit Bash PIPESTATUS handling ---'
bash -c 'set -e; false | tee /tmp/coderabbit-vg-out.txt; status=${PIPESTATUS[0]}; printf "valgrind_status=%s\n" "$status"; exit "$status"' 2>&1
printf 'exit=%s\n' "$?"Repository: alkahest-cas/alkahest
Length of output: 462
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- available sink ---'
command -v cat || true
printf '%s\n' '--- bash -e, no pipefail ---'
bash -c 'set -e; printf "test result: ok. 1 passed\n"; false | cat; printf "step continued\n"'
printf 'exit=%s\n' "$?"
printf '%s\n' '--- bash -e -o pipefail ---'
bash -c 'set -e -o pipefail; printf "test result: ok. 1 passed\n"; false | cat; printf "step continued\n"'
printf 'exit=%s\n' "$?"
printf '%s\n' '--- PIPESTATUS capture ---'
bash -c 'set -e; false | cat; status=${PIPESTATUS[0]}; printf "valgrind_status=%s\n" "$status"; exit "$status"'
printf 'exit=%s\n' "$?"Repository: alkahest-cas/alkahest
Length of output: 385
Preserve the Valgrind failure status.
The default Ubuntu shell uses bash -e without pipefail, so tee can mask Valgrind’s non-zero exit status. Enable set -o pipefail before the pipeline, or capture ${PIPESTATUS[0]} immediately after it and exit with that status.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 532 - 535, Update the Valgrind
pipeline in the CI workflow to preserve the analyzer’s exit status instead of
allowing tee to mask failures. Enable pipefail before the valgrind/tee command,
or capture PIPESTATUS[0] immediately afterward and exit with that status; keep
the existing output capture and Valgrind arguments unchanged.
| - name: Run compute-sanitizer racecheck | ||
| env: | ||
| ALKAHEST_GPU_TESTS: "1" | ||
| run: | | ||
| compute-sanitizer --tool racecheck \ | ||
| cargo test --features cuda,groebner-cuda | ||
| compute-sanitizer --target-processes all --tool racecheck \ | ||
| cargo test --features cuda,groebner-cuda \ | ||
| --test nvptx_gpu --test groebner_cuda |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file=".github/workflows/cuda_nightly.yml"
printf '%s\n' '--- workflow context ---'
sed -n '1,125p' "$file"
printf '%s\n' '--- all continue-on-error entries ---'
rg -n -C 3 'continue-on-error|racecheck|compute-sanitizer|ALKAHEST_GPU_TESTS' "$file"
printf '%s\n' '--- workflow metadata ---'
git diff --stat
git status --shortRepository: alkahest-cas/alkahest
Length of output: 8942
Make racecheck failures fail the workflow.
Set continue-on-error: false. Otherwise, racecheck failures and sanitizer startup failures can pass silently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/cuda_nightly.yml around lines 98 - 104, Ensure the “Run
compute-sanitizer racecheck” workflow step explicitly sets continue-on-error to
false so racecheck and sanitizer startup failures fail the workflow.
| if n_u32 > MAX_EXPAND_POW_EXP | ||
| && expansion_products(summands.len(), n_u32) > MAX_EXPAND_POW_PRODUCTS | ||
| { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The exponent floor bypasses the product budget, so a wide base can still blow up.
The decline requires both conditions. When n_u32 <= MAX_EXPAND_POW_EXP, expansion_products is never consulted, so the product budget does not apply at all. The doc comment at lines 1369-1373 states this openly: (a₁+…+a₂₀)⁴ expands to 160 000 products. The same path accepts far worse. (a₁+…+a₁₀₀₀)⁴ is 10¹² products, and distribute_once pre-allocates acc.len() * summands.len() and interns every product into the pool, so the process exhausts memory before it returns. The exponent is bounded by 4 here, but the base width is not bounded at all.
The old bound had the same hole, so this is not a regression. It is worth closing on the line that introduces a product budget, because the stated purpose at lines 1360-1361 is that "the work is capped so a stray large literal exponent cannot trigger combinatorial blow-up". Keep the compatibility floor, and add a hard product ceiling above it so the floor cannot be used to request unbounded work.
🛡️ Proposed fix
+/// Product count above which [`ExpandPow`] declines whatever the exponent is.
+///
+/// The `MAX_EXPAND_POW_EXP` floor exists for backward compatibility, not because
+/// exponent ≤ 4 is cheap: `(a₁+…+a₁₀₀₀)⁴` is 10¹² products, and
+/// `distribute_once` pre-allocates and interns every one of them. This ceiling
+/// is set far above every shape the floor was kept for (a twenty-term sum to the
+/// fourth power is 160 000) so nothing that expanded before stops expanding.
+const HARD_EXPAND_POW_PRODUCTS: u64 = 1 << 21;
+
@@
- if n_u32 > MAX_EXPAND_POW_EXP
- && expansion_products(summands.len(), n_u32) > MAX_EXPAND_POW_PRODUCTS
- {
+ let products = expansion_products(summands.len(), n_u32);
+ if (n_u32 > MAX_EXPAND_POW_EXP && products > MAX_EXPAND_POW_PRODUCTS)
+ || products > HARD_EXPAND_POW_PRODUCTS
+ {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if n_u32 > MAX_EXPAND_POW_EXP | |
| && expansion_products(summands.len(), n_u32) > MAX_EXPAND_POW_PRODUCTS | |
| { | |
| /// Product count above which [`ExpandPow`] declines whatever the exponent is. | |
| /// | |
| /// The `MAX_EXPAND_POW_EXP` floor exists for backward compatibility, not because | |
| /// exponent ≤ 4 is cheap: `(a₁+…+a₁₀₀₀)⁴` is 10¹² products, and | |
| /// `distribute_once` pre-allocates and interns every one of them. This ceiling | |
| /// is set far above every shape the floor was kept for (a twenty-term sum to the | |
| /// fourth power is 160 000) so nothing that expanded before stops expanding. | |
| const HARD_EXPAND_POW_PRODUCTS: u64 = 1 << 21; | |
| let products = expansion_products(summands.len(), n_u32); | |
| if (n_u32 > MAX_EXPAND_POW_EXP && products > MAX_EXPAND_POW_PRODUCTS) | |
| || products > HARD_EXPAND_POW_PRODUCTS | |
| { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@alkahest-core/src/simplify/rules.rs` around lines 1473 - 1475, Update the
expansion guard around expansion_products and MAX_EXPAND_POW_EXP so the product
budget is enforced independently of the compatibility exponent floor. Preserve
acceptance for exponents at or below MAX_EXPAND_POW_EXP only when
expansion_products(summands.len(), n_u32) remains within
MAX_EXPAND_POW_PRODUCTS, preventing wide bases from triggering unbounded
distribute_once work.
| The result says: with `S(n) = Σ_{k=k_lo}^{k_hi} F(n,k)`, | ||
|
|
||
| ```text | ||
| Σ_i a_i(n)·S(n+i) = 0 | ||
| Σ_i a_i(n)·S(n+i) = G(n, k_hi+1) − G(n, k_lo) | ||
| ``` | ||
|
|
||
| Here that reads `S(n+1) − 2·S(n) = 0`, which together with `S(0) = 1` gives | ||
| `Σ_k C(n,k) = 2^n`. | ||
| and that boundary difference vanishes here, so the recurrence reads | ||
| `S(n+1) − 2·S(n) = 0`, which together with `S(0) = 1` gives `Σ_k C(n,k) = 2^n`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="docs/mdbook/src/telescoping.md"
wc -l "$file"
ast-grep outline "$file" --lang markdown || true
sed -n '1,90p' "$file"
printf '\n--- related references ---\n'
rg -n -C 3 'k_lo|k_hi|boundary|zero|extension|correction|binomial|S\(n' docs/mdbook/src "$file"Repository: alkahest-cas/alkahest
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- telescoping document, focused range ---'
sed -n '1,75p' docs/mdbook/src/telescoping.md
printf '%s\n' '--- zeilberger definitions and call sites ---'
rg -n -C 5 'def zeilberger|fn zeilberger|zeilberger\(' --glob '!docs/**' --glob '!target/**' .Repository: alkahest-cas/alkahest
Length of output: 23707
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- tracked files mentioning the API ---'
git ls-files | rg '(^|/)(zeilberger|telescop|hyper|binomial|combin)' | head -80
printf '%s\n' '--- all relevant symbols, excluding documentation ---'
rg -n -C 4 'side_conditions|boundary_term|class .*Result|zeilberger' --glob '!docs/**' --glob '!target/**' --glob '!*.lock' . | head -300Repository: alkahest-cas/alkahest
Length of output: 21777
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- core endpoint and boundary semantics ---'
sed -n '1,35p' alkahest-core/src/holonomic/zeilberger.rs
sed -n '118,145p' alkahest-core/src/holonomic/zeilberger.rs
sed -n '4550,4635p' alkahest-py/src/lib.rs
printf '%s\n' '--- exact boundary example test ---'
sed -n '510,575p' alkahest-core/src/holonomic/zeilberger.rsRepository: alkahest-cas/alkahest
Length of output: 9670
Define the summation endpoints before deriving the recurrence. If an endpoint depends on n, the shifted sums require zero-extension or endpoint corrections. The boundary example must evaluate both G(n, k_hi+1) and G(n, k_lo), not only the lower endpoint.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/mdbook/src/telescoping.md` around lines 25 - 32, In the telescoping
derivation, define the summation endpoints k_lo and k_hi before introducing the
recurrence, including any dependence on n. Update the shifted-sum argument to
account for n-dependent endpoints via zero-extension or endpoint corrections,
and evaluate both boundary terms G(n, k_hi+1) and G(n, k_lo) before concluding
that their difference vanishes.
`simplify_par_with_config` never drained the bounded-expansion record, so a declined `ExpandPow` left no step in the log: the power came back unchanged with nothing saying why. That is precisely the silent no-op the product budget was added to prevent, reintroduced on the parallel path. The record is thread-local and `apply_rules` runs on a rayon worker, so the sequential trick of draining once per pass in the caller collects nothing — the drain has to happen on whichever thread ran the rules. Reachable from the public `simplify_par_with_config`; not from Python, whose `simplify_par` uses the default config with `expand: false`. The new test fails without the drain (verified by reverting it) and passes with it, so it is not another gate that cannot fail. Also: point the errors.md refusal section at what the bindings now do — it still said `radical` / `primary_decomposition` raise an uncoded `ValueError`, which stopped being true when the takers were wired up — and drop the CI token to `contents: read`, since no job there touches repository state while every one of them runs build scripts from the head of a PR. Found by CodeRabbit on #296. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Release branch for 3.8.0. 15 commits, 0 behind
main.Theme
Most of this release is one defect family: a routine that could not answer returned something anyway. The fixes either compute where the answer is provable, or refuse with a stable code — never assert a result with nothing behind it.
Correctness
radical/primary_decomposition—radicalreturned its input unchanged whenever the zero-dimensional Seidenberg trick didn't apply, asserting√I = I;primary_decompositionthen stamped that onto every component'sassociated_prime, so a field whose name is a guarantee could name a non-prime. Now certified for monomial, principal, zero-dimensional and shape-position cases;E-IDEAL-005/006otherwise.triangularize— the main-variable pick was inverted (largest index, where the bottom-univariate split assumes index 0 is lex-greatest), so generators collided in one slot and the tie-break discarded the rest. Also verifies<chain> ⊇ Iand refuses (E-SOLVE-004) rather than return an under-determined chain.series— an unreachable order ran away; now refuses (E-SERIES-003) rather than assemble a truncated prefix whoseO(h^n)term would be a false statement about the remainder.solve—solve([a*x-b],[x])returnedb/awithout statinga ≠ 0; hypotheses are readable viasolve_side_conditions().residue— bareAttributeErroron a non-numeric point →E-RESIDUE-005.Honesty of the capability contract
capabilities()advertisedgroebner_cudaandnumpy, neither of which named anything a Python caller could reach — no observation distinguishedTruefromFalse. Both removed (contract v3), with the deadnumpydependency.compute_groebner_basis_gpu's silent CPU fallback now reports throughGpuBackendReport. A new contract test walks every advertised bit and requires a reachable entry point.Performance
Matrix::detO(n!) → n^3.13 (1408ms → 0.09ms) ·poly_gcd272,882ms → 0.40ms · discarded-RootSum guard 869s → 154ms ·real_roots1.568× → 0.9975×.expandgains a 4096-product budget replacing an incoherent flat exponent cap, and recordsexpand_pow_limit_reachedwhen it declines.CI that was inspecting nothing
Eight gates were found passing while checking nothing, and fixed: the error-code checker referenced by no workflow (53 accumulated errors), a valgrind shard building no test binaries, both AFL fuzz shards (5 stacked faults, never ran once), an ASan job never running Python,
compute-sanitizerinstrumentingcargo, and others. Fuzzing now demonstrably runs (66.7M + 43.2M execs); valgrind is scoped to the FFI boundary (121 tests, was 2h49m) with a guard that fails if zero tests execute.Design note
Refusals travel out of band (
take_ideal_refusal/take_triangularize_refusal/take_series_refusal), followingmatrix::take_zero_test_refusal: the error enums are public and exhaustive, so a correctness fix in a minor release cannot spend a major version on a new variant. A regression test pins that a genuine non-polynomial still reportsE-SOLVE-001rather than being re-attributed to the refusal code.Gates
All five workflows green on the branch: CI (12m37s), CodSpeed, semver-check (no semver update required), ci-cross, docs. Locally: 2026 lib + 26 doctests, 2689 pytest, silent-error corpus 0 / 238,
cargo doc -D warnings, ruff,check_error_codes,check_api_freeze.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation