Skip to content

fix: resolve open CodeQL alerts (float widening, workflow permissions) - #22

Merged
mstrathman merged 2 commits into
mainfrom
fix/codeql-alerts
Aug 3, 2026
Merged

fix: resolve open CodeQL alerts (float widening, workflow permissions)#22
mstrathman merged 2 commits into
mainfrom
fix/codeql-alerts

Conversation

@mstrathman

@mstrathman mstrathman commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Resolves the four open CodeQL alerts on the repo.

High: keep the forest product in double before it overflows (predict-student.c)

predict0_forest_predict_row accumulated f->lr * forest_tree_value(...) with
both operands float, so the product was computed in float and only then
widened to the double accumulator. That can overflow to infinity, and under
x87 excess precision (FLT_EVAL_METHOD) even differ by platform, before the
widening. Both spots now cast to double before the multiply, matching the
trainer (predict-train.c already does this).

Model weights and content_hash are unchanged (training was already in double);
only served predictions can shift in their least-significant digits, which the
CHANGELOG notes. Resolves the two cpp/integer-multiplication-cast-to-long
alerts.

Medium: explicit workflow permissions (wheels.yml)

wheels.yml declared no permissions block, so its build jobs ran with the
default token scopes. Added a top-level permissions: contents: read; the
publish job keeps its own id-token: write override for PyPI OIDC, so
publishing is unaffected. Resolves the two actions/missing-workflow-permissions
alerts.

Verification

  • make test: 209 passed, 35 skipped.
  • Local CodeRabbit review against main: no findings.

Merging re-runs CodeQL on main, which auto-resolves the four alerts.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Forest predictions now use consistent double-precision calculations for learning-rate accumulation.
    • Improved numerical reliability by preventing premature floating-point overflow and platform-dependent precision differences.
    • Least-significant prediction digits may differ in some cases.
  • Documentation

    • Added an Unreleased changelog entry describing the prediction precision improvements.

Two high alerts (cpp/integer-multiplication-cast-to-long): forest serving
multiplied f->lr (float) by a float tree value before widening to the double
accumulator, so the product could overflow to infinity, and under x87 excess
precision vary by platform, before the widening. Promote to double before the
multiply in both spots, matching the trainer (predict-train.c already does).

Two medium alerts (actions/missing-workflow-permissions): wheels.yml had no
permissions block, so the build jobs got the default token scopes. Default the
workflow token to contents:read; the publish job keeps its id-token:write
override for PyPI OIDC.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: ea5e1687-0260-42bd-b280-4550be26de29

📥 Commits

Reviewing files that changed from the base of the PR and between 8c8f6ea and 55d6e2e.

📒 Files selected for processing (1)
  • CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md

📝 Walkthrough

Walkthrough

Forest prediction now performs learning-rate multiplication in f64 for regression and classification. The wheels workflow uses read-only default token permissions. The changelog records the precision fix and possible least-significant-digit changes.

Changes

Forest prediction precision

Layer / File(s) Summary
Double-precision forest accumulation
predict-student.c, CHANGELOG.md
Regression and classification paths multiply learning rates in f64 before accumulation. The changelog records the fix and possible prediction digit changes.

Workflow token permissions

Layer / File(s) Summary
Read-only workflow permissions
.github/workflows/wheels.yml
The workflow defaults GITHUB_TOKEN to contents: read. The publish job retains id-token: write for PyPI OIDC publishing.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title uses the required fix: prefix and identifies the CodeQL alert fixes for float widening and workflow permissions.
Description check ✅ Passed The description explains the changes, rationale, affected files, prediction impact, and test results, but omits the template checklist.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Changelog Entry For User-Visible Changes ✅ Passed The prediction behavior change is user-visible, and CHANGELOG.md documents it under [Unreleased] > Fixed at lines 7–17.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/codeql-alerts

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@CHANGELOG.md`:
- Around line 14-15: The CHANGELOG precision note overstates that full
predictions are computed consistently in double precision. Update the entry to
specifically state that the learning-rate/tree-value product and forest
accumulation use f64, while acknowledging that predict0_reg_tree_value() and
forest_tree_value() still produce f32 values; keep the documentation standalone.
🪄 Autofix (Beta)

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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4d41cbd0-b0fd-4226-95fc-34da08e6d306

📥 Commits

Reviewing files that changed from the base of the PR and between d99caf8 and 8c8f6ea.

📒 Files selected for processing (3)
  • .github/workflows/wheels.yml
  • CHANGELOG.md
  • predict-student.c

Comment thread CHANGELOG.md Outdated
Address review: the fix moved the learning-rate/tree-value product and the
forest accumulation to double, but individual tree values are still f32. Say
that precisely instead of claiming the whole prediction is computed in double.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@mstrathman
mstrathman merged commit dd6f6be into main Aug 3, 2026
17 checks passed
@mstrathman
mstrathman deleted the fix/codeql-alerts branch August 3, 2026 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant