refactor(audit-r3-soft): implement 27 triaged soft recommendations (B1–B9)#79
Merged
Conversation
…1–B9)
From the R3 full-package review's 68 soft recommendations, triaged (27 KEEP /
1 DECIDE / 37 SKIP-with-reason) and implemented the KEEP set in 9 themed batches,
each built in an isolated worktree and integrated here with ruff + targeted tests
re-verified on the composed tree.
B1 docs: deploy.en.md gains the 5 LaTeX-sandbox env vars + DATALAB_SSE_DISABLE_
RATE_LIMIT + DATALAB_TRUST_PROXY_HEADERS; CLAUDE.md env list expanded;
ARCHITECTURE.md gains a datalab_core service-layer section.
(DATALAB_LATEX_ENGINE deliberately NOT documented as a deploy var — it's
tools-only, not on the web runtime path.)
B2 docstrings: fit_custom_model (all params/errors), precision_guard (clamp
params, constants, mp.dps contract, example).
B3 bilingual: mcmc_fitter run_mcmc's 4 validation ValueErrors + constraints
_parse_expr_safe parse error now use _dual_msg; bilingual-separator tests.
B4 type-safety: drop defensive getattr on required fields (hp_fitter), TypeAlias
on MpfCallable, real typed ModelSpecification fields for implicit metadata
(implicit_model sets them as kwargs, not setattr). mypy --strict clean.
B5 perf: J^T J via mp.matrix multiply; hoisted mp.mpf("0") — behavior-preserving.
B6 tests: new suites for precision (_coerce_int/precision_guard/clamps/validators),
_ast_metrics, _generate_seed_variants, ParameterState.compose — plus a
loud-fail guard in compose() for the real length-mismatch truncation.
B7 expression_engine: ZeroDivisionError from / and % now wrapped in a bilingual
ValueError (honors the module's ValueError-only contract) + tests.
B8 statistics: deduplicate _bool_option/_string_option into datalab_core/
statistics_helpers.py (strict fail-fast default; verified no caller needs
string variants).
B9 maintainability: hoist a per-iteration combo read; consolidate a 3-line
trim_visible; delete the empty WindowFittingParamsMixin placeholder; drop the
single-use _latex_escape_text wrapper.
360+ tests across all touched areas pass; ruff + mypy --strict clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR bundles bilingual (Chinese/English) error messaging across constraint parsing, MCMC validation, and expression-engine division-by-zero handling; deduplicates statistics option-parsing helpers into a new shared module; removes an unused desktop mixin; refactors implicit-model field wiring and hp_fitter internals; updates deployment/architecture docs; and adds extensive new tests. ChangesBilingual error messages and expression engine
Statistics helper deduplication
Fitting internals
Desktop app cleanup
Docs and precision guard
Estimated code review effort: 3 (Moderate) | ~30 minutes 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app_desktop/window.py (1)
1366-1373: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate combo-box query: reuse
methodinstead of re-fetchingtime_series_methodlater.
methodhere andtime_series_method(further down, unchanged, lines 1433-1437) both callself.stats_time_series_method_combo.currentData()and hold the same value within this same function call. Consider consolidating to a single variable now that this call has been hoisted.♻️ Proposed consolidation
method = ( self.stats_time_series_method_combo.currentData() if hasattr(self, "stats_time_series_method_combo") else None ) @@ - time_series_method = ( - self.stats_time_series_method_combo.currentData() - if hasattr(self, "stats_time_series_method_combo") - else None - ) - is_ewma = time_series_method == "ewma" - is_rolling_std = time_series_method == "rolling_std" + is_ewma = method == "ewma" + is_rolling_std = method == "rolling_std"🤖 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_desktop/window.py` around lines 1366 - 1373, The time-series method is being queried twice in the same update path, once into method and again later into time_series_method, which is redundant. In the relevant window update logic in window.py, reuse the already hoisted method value instead of calling self.stats_time_series_method_combo.currentData() again, and make sure the later visibility/branching code references that single variable consistently.fitting/hp_fitter.py (1)
742-746: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent getattr usage for a now-guaranteed field.
implicit_definitionis a properModelSpecificationdataclass field (defaultNone) as of this same PR, yet line 745 still reads it viagetattr(current_model, "implicit_definition", None).current_modelis always aModelSpecificationhere (eithermodelor themodel_factoryreturn value, both typed asModelSpecification), so this can be simplified to direct attribute access — consistent with thedependent_defs/expressioncleanups made elsewhere in this same diff (lines 256, 691), and it lets mypy actually type-check the access instead of returningAny.♻️ Proposed simplification
can_parallelize = ( model_factory is None and _dependent_defs_are_picklable(parameter_state) - and getattr(current_model, "implicit_definition", None) is None + and current_model.implicit_definition is 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 `@fitting/hp_fitter.py` around lines 742 - 746, The parallelization check in the hp_fitter logic still uses getattr for a field that is now guaranteed on ModelSpecification. Update the can_parallelize expression to read implicit_definition directly from current_model, since current_model is always a ModelSpecification here and this should match the dependent_defs/expression direct-access cleanup elsewhere in the file. Keep the change localized around the can_parallelize assignment so mypy can type-check the field access properly.
🤖 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 `@docs/CODE_REVIEW_2026_R3_SOFT_PLAN.md`:
- Around line 11-12: The plan still points to stale paths and env-var names, so
update the affected notes to match the shipped tree. In the CLAUDE.md and
docs/ARCHITECTURE.md references, use the current DATALAB_LATEX_* set exposed by
app_web/latex_security.py, remove or replace the outdated DATALAB_LATEX_ENGINE
mention unless it is truly present, and change any statistics/_helpers.py
reference to datalab_core/statistics_helpers.py. Keep the plan wording aligned
with the current codebase and cross-reference the existing CLAUDE.md and
architecture sections instead of duplicating outdated location details.
---
Nitpick comments:
In `@app_desktop/window.py`:
- Around line 1366-1373: The time-series method is being queried twice in the
same update path, once into method and again later into time_series_method,
which is redundant. In the relevant window update logic in window.py, reuse the
already hoisted method value instead of calling
self.stats_time_series_method_combo.currentData() again, and make sure the later
visibility/branching code references that single variable consistently.
In `@fitting/hp_fitter.py`:
- Around line 742-746: The parallelization check in the hp_fitter logic still
uses getattr for a field that is now guaranteed on ModelSpecification. Update
the can_parallelize expression to read implicit_definition directly from
current_model, since current_model is always a ModelSpecification here and this
should match the dependent_defs/expression direct-access cleanup elsewhere in
the file. Keep the change localized around the can_parallelize assignment so
mypy can type-check the field access properly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d868b04a-9bdf-4661-91cd-45c86934366a
📒 Files selected for processing (28)
CLAUDE.mdapp_desktop/fitting_latex_writer.pyapp_desktop/window.pyapp_desktop/window_fitting_mixin.pyapp_desktop/window_fitting_params_mixin.pydatalab_core/statistics.pydatalab_core/statistics_grouped.pydatalab_core/statistics_helpers.pydatalab_core/statistics_matrix.pydatalab_core/statistics_time_series.pydocs/ARCHITECTURE.mddocs/CODE_REVIEW_2026_R3_SOFT_PLAN.mddocs/web/deploy.en.mdfitting/constraints.pyfitting/hp_fitter.pyfitting/implicit_model.pyfitting/mcmc_fitter.pyfitting/model_parser.pyshared/expression_engine.pyshared/precision.pytests/test_bilingual_errors.pytests/test_constraints_compose.pytests/test_datalab_core_statistics.pytests/test_expression_engine_ast_metrics.pytests/test_expression_engine_zerodiv.pytests/test_file_size_ratchet.pytests/test_hp_fitter_seed_variants.pytests/test_shared_precision.py
💤 Files with no reviewable changes (1)
- app_desktop/window_fitting_params_mixin.py
Comment on lines
+11
to
+12
| - `CLAUDE.md:48` — expand env-var list to include the 8 missing vars (`DATALAB_TRUST_PROXY_HEADERS`, the 5 LaTeX vars, `DATALAB_LATEX_ENGINE`), or add a one-line pointer to `deploy.en.md`. `[_idx 27]` | ||
| - `docs/ARCHITECTURE.md:78` — add a `datalab_core` service-layer section between "Computation modules" and "Cross-cutting conventions"; cross-reference CLAUDE.md rather than duplicating. `[_idx 26]` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep this plan aligned with the shipped paths.
The batch notes still mention DATALAB_LATEX_ENGINE and statistics/_helpers.py, but the current tree exposes only the five DATALAB_LATEX_* vars in app_web/latex_security.py and the helper module now lives at datalab_core/statistics_helpers.py. Please update the plan so it doesn't point readers at stale locations.
Also applies to: 50-52
🤖 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/CODE_REVIEW_2026_R3_SOFT_PLAN.md` around lines 11 - 12, The plan still
points to stale paths and env-var names, so update the affected notes to match
the shipped tree. In the CLAUDE.md and docs/ARCHITECTURE.md references, use the
current DATALAB_LATEX_* set exposed by app_web/latex_security.py, remove or
replace the outdated DATALAB_LATEX_ENGINE mention unless it is truly present,
and change any statistics/_helpers.py reference to
datalab_core/statistics_helpers.py. Keep the plan wording aligned with the
current codebase and cross-reference the existing CLAUDE.md and architecture
sections instead of duplicating outdated location details.
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 — soft-recommendation implementation (27 KEEP items, B1–B9)
Triaged the R3 review's 68 soft recommendations (27 KEEP / 1 DECIDE / 37 SKIP-with-reason) and implemented the KEEP set in 9 themed batches. Each batch was built in an isolated worktree, then integrated and independently re-verified on the composed tree (not trusting per-batch self-reports).
deploy.en.mdgains the 5 LaTeX-sandbox env vars +DATALAB_SSE_DISABLE_RATE_LIMIT+DATALAB_TRUST_PROXY_HEADERS;CLAUDE.mdenv list expanded;ARCHITECTURE.mdgains adatalab_coreservice-layer section. (DATALAB_LATEX_ENGINEdeliberately not documented — tools-only.)fit_custom_model(all params/errors),precision_guard(clamp params, constants, mp.dps contract).mcmc_fitter.run_mcmc's 4 validationValueErrors +constraints._parse_expr_safenow use_dual_msg; separator tests.getattron required fields;TypeAliasonMpfCallable; real typedModelSpecificationimplicit fields (set as kwargs, notsetattr). mypy --strict clean.J^T Jviamp.matrixmultiply; hoistedmp.mpf("0")— behavior-preserving.precision/_ast_metrics/_generate_seed_variants/ParameterState.compose— plus a loud-fail guard incompose()for a real length-mismatch truncation.ZeroDivisionErrorfrom/and%wrapped in a bilingualValueError(honors the ValueError-only contract) + tests._bool_option/_string_optionintodatalab_core/statistics_helpers.py(strict fail-fast; verified no caller needs string variants).trim_visible; delete the emptyWindowFittingParamsMixinplaceholder; drop the single-use_latex_escape_textwrapper.Also: consciously re-baselined
fitting/hp_fitter.pyin the file-size ratchet (819→890, almost all docstrings).Test plan
ruff+mypy --strict(shared/fitting/extrapolation_methods/datalab_latex) clean.Deferred (not in this PR): 1 DECIDE (
_refresh_display_formatrefactor — needs a maintainer call) and Batch 10 (ExtrapolationWindow3196-line decomposition — its own staged track).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation