fix(audit-r3): 8 verified defects from full-package review (D1–D8)#78
Merged
Conversation
…help_specs error
D1/D2 — latex_tables_extrapolation.py embedded user caption (:309) and column
headers (:410) raw into \caption{} / \multicolumn{}, unescaped — a LaTeX
injection / broken-compile gap (R2's F14 fixed only the error-propagation
table). Escape both via shared.latex_escaping.latex_escape, like the sibling
tables.
D4 — statistics_compute.py:380 guarded effective_n with
`elif not mp.almosteq(W2, 0)`, which is True for tiny-but-valid positive W2
(e.g. very large sigmas), silently dropping effective_n even though W2 is
already known > 0 and finite. Replace the elif with else.
D5 — /api/help_specs (public GET) echoed str(exc) on failure, leaking
filesystem paths/internals. Return only type(exc).__name__ and log full detail
server-side.
TDD throughout (RED verified for each). ruff clean; latex + statistics + api
smoke suites green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
D6 — app_web/logic/fitting.py: the fitting-overview plot render caught all
exceptions and returned None with no log. Keep the graceful fallback but
_logger.exception(...) so the cause reaches server logs.
D7 — fitting/runner.py: the observed-linear fast-path `except ValueError: pass`
discarded the reason for falling back to the general solver. Record it into
observed_linear_skipped (already appended to fallback_history), so the fallback
is traceable.
D8 — formula_help.py: _load_help_specs returned {} on any OSError/JSONDecodeError
with no diagnostic. Log each failed candidate path and a final warning listing
all tried paths before the empty fallback (verified: warning emitted, {} returned).
ruff clean; fitting/implicit/api-smoke suites green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…strings (D3) The SSE fit result serialized params/param_errors_stat via float(v), rounding mp.mpf values computed at precision=80+ down to ~17 digits and destroying the product's high-precision result on the web fit-stream path. Serialize with mp.nstr(v, precision) instead (mirrors the desktop worker), and update the OpenAPI FitResult schema (app_web/openapi.py) to declare these as strings, not numbers (Codex refinement). Note: the initial attempt wrapped the value in mp.mpf(v) before mp.nstr — that re-quantizes an existing mpf at the ambient (low) mp.dps back to float precision. The added >17-significant-digit regression test caught it; the fix formats the mpf directly. ruff clean; 93 SSE/web-precision/api tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Records the R3 swarm review (16 dimensions, adversarially verified internally + externally via Codex/gpt-5.5 and Gemini/Antigravity): 8 confirmed defects (D1–D8, all fixed in this branch), 1 rejected (security-shim), and 68 soft recommendations for follow-up (maintainability/perf/testing/type-safety/docs/ bilingual). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR sanitizes exception details returned by the help_specs API, converts SSE/OpenAPI fit parameter serialization from floats to precision-preserving strings, escapes LaTeX caption/header text against injection, removes an incorrect almosteq gate in effective_n computation, adds logging for several previously silent failures, and adds a code review audit document with accompanying tests. ChangesSecurity, precision, and error-handling fixes
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant api_help_specs
participant Logger
Client->>api_help_specs: GET /api/help_specs
api_help_specs->>api_help_specs: exception raised while loading specs
api_help_specs->>Logger: warning(exc_info=True)
api_help_specs-->>Client: 500 {error: exception class name only}
sequenceDiagram
participant FitEngine
participant single_fit_events
participant mpmath
participant Client
FitEngine->>single_fit_events: fit_result.params, param_errors_stat
single_fit_events->>mpmath: nstr(value, precision)
mpmath-->>single_fit_events: precision-preserving string
single_fit_events-->>Client: SSE result event with string-serialized params
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app_web/openapi.py (1)
66-83: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
aic/bic/r2should bestringhere too. The fit serializer writes these as decimal strings, so leaving them asnumbermakes the response schema inconsistent.🤖 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 `@app_web/openapi.py` around lines 66 - 83, The fit response schema is inconsistent because `aic`, `bic`, and `r2` are declared as JSON numbers even though the serializer emits them as decimal strings. Update the schema in `openapi.py` where the fit result properties are defined so `aic`, `bic`, and `r2` use the same string type as `params` and `param_errors_stat`, keeping the OpenAPI contract aligned with the serializer.
🧹 Nitpick comments (2)
app_web/blueprints/api.py (1)
267-279: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider
logger.error/exceptioninstead ofwarningfor a 500 failure.This branch represents a genuine endpoint failure (HTTP 500), not a recoverable fallback. Per common logging conventions, WARNING is meant for degraded-but-functioning paths while ERROR/CRITICAL signal actual failures that alerting/monitoring typically watches for. Using
warninghere risks this being filtered out of error-level alerts.Suggested change
- current_app.logger.warning("Failed to load help specs", exc_info=True) + current_app.logger.error("Failed to load help specs", exc_info=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 `@app_web/blueprints/api.py` around lines 267 - 279, The exception handler in api.py for the help-specs load failure is logging a real HTTP 500 as a warning, which can hide it from error monitoring. Update the logging in the `except Exception as exc` branch to use `current_app.logger.error` or `current_app.logger.exception` with `exc_info=True` so the failure is recorded at error level while keeping the public response unchanged. Keep the full server-side detail logging in this branch and preserve the redacted client-facing message returned from the help-specs endpoint.tests/test_latex_tables_unit.py (1)
136-173: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a backslash-injection payload to the escaping test.
The new test covers
& _ $ # % ~, but not a raw backslash payload (e.g.\input{...}or\write18{...}), which is the more severe injection vector this fix targets. Consider adding an assertion that a literal\in caption/header text is neutralized (e.g. rendered as\textbackslash{}) rather than surviving as a live LaTeX command prefix.🤖 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_latex_tables_unit.py` around lines 136 - 173, The LaTeX escaping test currently covers special characters but misses the backslash injection case, which is the key attack vector. Update the test around generate_latex_table to include a caption or header value containing a literal backslash sequence like a command prefix, then assert that the rendered output from generate_latex_table escapes it safely (for example via a neutralized backslash representation) and that no raw backslash survives in the caption/header text. Use the existing caption and header checks in test_generate_latex_table_escapes_caption_and_headers as the place to add this assertion.
🤖 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.
Outside diff comments:
In `@app_web/openapi.py`:
- Around line 66-83: The fit response schema is inconsistent because `aic`,
`bic`, and `r2` are declared as JSON numbers even though the serializer emits
them as decimal strings. Update the schema in `openapi.py` where the fit result
properties are defined so `aic`, `bic`, and `r2` use the same string type as
`params` and `param_errors_stat`, keeping the OpenAPI contract aligned with the
serializer.
---
Nitpick comments:
In `@app_web/blueprints/api.py`:
- Around line 267-279: The exception handler in api.py for the help-specs load
failure is logging a real HTTP 500 as a warning, which can hide it from error
monitoring. Update the logging in the `except Exception as exc` branch to use
`current_app.logger.error` or `current_app.logger.exception` with
`exc_info=True` so the failure is recorded at error level while keeping the
public response unchanged. Keep the full server-side detail logging in this
branch and preserve the redacted client-facing message returned from the
help-specs endpoint.
In `@tests/test_latex_tables_unit.py`:
- Around line 136-173: The LaTeX escaping test currently covers special
characters but misses the backslash injection case, which is the key attack
vector. Update the test around generate_latex_table to include a caption or
header value containing a literal backslash sequence like a command prefix, then
assert that the rendered output from generate_latex_table escapes it safely (for
example via a neutralized backslash representation) and that no raw backslash
survives in the caption/header text. Use the existing caption and header checks
in test_generate_latex_table_escapes_caption_and_headers as the place to add
this assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f9050e4-45db-4775-84de-0154ec3af78a
📒 Files selected for processing (13)
app_web/blueprints/api.pyapp_web/blueprints/sse.pyapp_web/logic/fitting.pyapp_web/openapi.pydatalab_core/statistics_compute.pydatalab_latex/latex_tables_extrapolation.pydocs/CODE_REVIEW_2026_R3.mdfitting/runner.pyformula_help.pytests/test_app_web_precision_concurrency.pytests/test_latex_tables_unit.pytests/test_statistics_weighted.pytests/test_web_api_smoke.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audit R3 — verified defect fixes (D1–D8)
Fixes the 8 defects that survived the R3 full-package swarm review
(16 dimensions), internal 2-skeptic verification, and external adversarial
re-review by Codex (gpt-5.5, xhigh) + Gemini (via Antigravity). Full
recommendation doc:
docs/CODE_REVIEW_2026_R3.md. TDD throughout (RED verified).datalab_latex/latex_tables_extrapolation.py:309datalab_latex/latex_tables_extrapolation.py:410mp.nstr(strings), notfloat()— preserves precision=80+; OpenAPI schema updated to stringsapp_web/blueprints/sse.py:461,app_web/openapi.pyelif not almosteq(W2,0)→else(tiny-but-valid positive W2 was silently dropped)datalab_core/statistics_compute.py:380/api/help_specsno longer echoesstr(exc)(path/internal leak) — returns class name, logs server-sideapp_web/blueprints/api.py:269app_web/logic/fitting.py:970ValueErrorrecords its fallback reason instead ofpassfitting/runner.py:213formula_help.py:78Rejected by external review (not fixed, documented): the security-shim
"dev-mode disables LaTeX sandboxing" finding — production hard-raises on
security-import failure and the dev fallback still forces
-no-shell-escape.Test plan
re-quantize bug in the first fix attempt (
mp.mpf(v)at ambient dps).ruff checkclean on all changed files.+ the
mypy --strictcore-layer gate.🤖 Generated with Claude Code
Summary by CodeRabbit