Execute P0/P1/P2 roadmap from CODE_REVIEW_2026 (correctness, perf, security, structure)#70
Merged
Merged
Conversation
- .gitignore: exclude per-session scratch diffs, ad-hoc debug scripts (test_ast.py/test_dos.py), root LaTeX compile byproducts, uv.lock, and CLAUDE_HANDOFF.md (per-session handoff note, like task_plan.md). - CLAUDE.md: repo-root architecture/onboarding guide for future sessions.
- datalab_core/: UI-neutral SessionService boundary plus recipes, history/compare, report_bundle, uncertainty_budget, and the extended statistics families (bootstrap, grouped, hypothesis, matrix, time_series), fitting_comparison, recipe_provenance. - shared/: new unit-annotation, expression-registry, formula-export, distribution-summary, error-contribution, archive-validation helpers and precision/parsing/units updates. - fitting/: model_comparison, diagnostics, comparison/diagnostic formatting. - root_solving / statistics_utils: aligned with the new core boundary.
- Split monolithic table generation into per-family modules: budget, fitting, root, grouped/matrix statistics, plus report_bundle. - Updated common/error-propagation table helpers and formula render service.
- New panels: budget, history, history_compare, recipe_preview, report_bundle export/preview, standalone formula_renderer. - Workbench formula/variable panels, model bindings, specs; fitting mixins (models/residuals/formatters) and views aligned with the core service layer. - Remove obsolete formula_tex_render_worker (rendering moved to formula_renderer / formula_render_service).
- logic/ (extrapolation, fitting, error_propagation, statistics, plots, common) routed through datalab_core service factory. - templates (error/fit/stats) and i18n strings updated to match.
- docs/: desktop & web guides, P3/P4 specs/plans under docs/superpowers/. - examples/: bundled recipe JSON and generated workspace templates. - tools/: formula renderer value gate, latex option matrix, release import hygiene, plus screen-capture/schema-scan updates.
- New regressions for recipes, report bundle, history compare, uncertainty budget, extended statistics, units, example workspaces, desktop panels. - Remove root-level ad-hoc test scripts now covered under tests/.
- H1 (high, GUI): history_panel.restore_selected wrapped restore_history_entry_result with no try/except, so a .datalab entry whose family/kind is outside the supported renderer set (legacy/foreign/future/hand-edited) raised HistoryValidationError into the Qt event loop. Mirror the export_selected pattern: catch and surface via message_label, return False. Adds regression test. - H2 (high, precision): run_fitting_comparison serialized mpmath results at the ambient process-global mp.dps instead of the requested precision, silently truncating chi2/aic/bic/rmse/params to ~15 sig figs on the desktop QThread path (web path was already guarded). Wrap compute+serialize in precision_guard(precision). Adds regression test reproducing the 1/3-slope truncation.
Codex-1 (medium, GUI): when standard statistics ran from the main Run button with units enabled, units were only attached to a preflight core_request; the stats CalcJob had no units_config and the worker rebuilt its own per-column requests via build_multi_column_statistics_requests without units, so normal single-column runs silently dropped all unit metadata. Set units_config on the stats CalcJob and pass units=job.units_config into the worker request builder. Also fix two pre-existing stale test doubles in test_app_desktop_workers_core whose fake_render_root_batch_result lacked the root_units_by_name kwarg the real signature now accepts (they failed independently of this change).
Codex-2 (medium, GUI): _run_statistics_matrix_mode and _run_statistics_time_series_mode parsed the source table with _statistics_raw_table, which whitespace-splits — so CSV/TSV input or rows with blank (missing) cells failed (column mismatch / column-not-found) before the matrix missing policies could treat blanks as missing. The grouped workflow already uses _statistics_raw_table_preserving_cells; route matrix and time-series through the same preserving parser. Adds regression test.
M2 (medium, web parity regression): csv_data is now populated for every non-comparison fit/stats result, so the 'serverX || tableToCsv(...)' short-circuit always used the server CSV and the scientific-notation / decimal-places toggles became dead for CSV export (regression vs main). Rebuild the CSV from the visible table via tableToCsv (toggle-aware) for normal results; keep the server CSV only for the fit comparison table, which tableToCsv cannot reconstruct.
M1 (medium, data loss): model_from_manifest forward-wired model.history but not model.provenance, so any datalab.workspace.v2 manifest carrying recipe-provenance metadata lost its audit trail (recipe_id/version/source) on load, while WorkbenchModel treats provenance as a first-class round-trip field. Wire provenance through like history. Adds round-trip regression test.
G1 (medium, data loss): _taylor_sensitivity_metadata keyed sensitivities by display label and overwrote on collision, so two distinct canonical variables sharing a display label (duplicate column headers) lost one variable's sensitivity. The sibling contributions_map already aggregates duplicate labels with '+'. Aggregate the absolute sensitivity by summing; sum relatives only when every contributor supplied one, otherwise drop the relative and record the omission reason. Adds regression test.
- L3 (low, defense-in-depth): the desktop pdflatex/xelatex argv lacked -no-shell-escape, so opening and compiling an untrusted .tex could run shell commands (\write18) on a shell-escape-enabled TeX distro. Add -no-shell-escape for parity with the hardened web path. Adds argv regression test. - G3 (low, cosmetic): generate_statistics_latex_batches fell back to len(lines) (the accumulating LaTeX document line count) as the batch index when a batch omitted "index", producing nonsensical captions like "Batch 24". Fall back to a sequential enumerate counter. Adds regression test.
- L1 (low, i18n parity): the new Parameter Correlations / Standardized Residuals diagnostic tables and the chi2 p-value / max-standardized-residual metrics used hardcoded English, rendering English under the Chinese UI. Add data-i18n on every new label/header and define the keys in both languages. Adds a guard test that every data-i18n key in fit.html is defined in zh and en. - L2 (low, error UX): _error_key_for_exception collapsed comparison-candidates JSON errors and units-unsupported-on-web errors to the generic 'Computation failed.'. Classify 'valid json'/'candidates' as formula_parse_failed and add a dedicated errors.units_unsupported_on_web key (both languages). Adds classifier tests. - docs: TEST_MATRIX.md no longer describes latex_option_matrix as deferred/untracked (those files are now tracked release-gate evidence).
…rlap) V1-V3 (high, visual): the canvas QGroupBox QSS rule (workbench_region_style) set a border but no margin-top + ::title block, so Qt dropped the native title-band reservation and every titled box reparented into the canvas rendered its title overlapping the first control — the user-reported '单位标注' overlapping its '启用单位标注' checkbox, plus all parameter boxes (power/levin/richardson/implicit). Mirror the working config_card pattern: reserve GROUPBOX_TITLE_CLEARANCE (18px) and add a paired QGroupBox::title rule. Also add a unified spacing-token scale (SPACE_XS/SM/MD/LG, INNER_BOX_MARGIN, CARD_MARGIN_*, GROUPBOX_TITLE_CLEARANCE) with legacy names kept as aliases. Adds QSS contract tests for both themes.
- V6 (medium): views/error.py set error_layout.setSpacing(4) on the SHARED card layout, making error rows visibly tighter than every other mode. Remove the override so all five mode cards share 8px. - V5 (low): the shared card builder and make_units_box hardcoded divergent literals (12,10,12,12 / 8,8,8,8 / spacing 8/6). Drive them from the spacing tokens (CARD_MARGIN_*, INNER_BOX_MARGIN, SPACE_MD/SM). Adds a test asserting all five mode section cards share uniform spacing and margins.
…l control) G1 (high, shell button): the fit_mcmc_refine checkbox was enabled whenever emcee is installed, but its value never reached the fit job — _prepare_fit_job/FitJob had no mcmc field and _attach_mcmc_refinement had zero call sites, so ticking it silently gave a plain fit with no refinement and no warning. The MCMC code is complete and bundled, so wire it up: - add FitJob.refine_with_mcmc, read self.fit_mcmc_refine.isChecked() in _prepare_fit_job - split _attach_mcmc_refinement into a FitResult-level _attach_mcmc_refinement_to_fit (the single-explicit-model desktop path returns a FitResult, not a comparison summary); the function self-skips when emcee/evaluator/params are unavailable - call it at every _execute_fit_job_payload return point when the flag is set Strengthen the wiring test to assert the flag propagates to the job AND triggers refinement in the worker (the old test only checked a FitJob was returned).
… wiring - D1 (low): SectionPanel's '?' help button was shown when help_text was set but its clicked signal was never connected — a no-op shell button. Wire it to pop the help text via QToolTip. Adds a test asserting the click shows the help text. - D2 (low, footgun): make_formula_preview_button only self-connects when edit_widget is given; the four None-callers wire it externally. No button is dead today, but a future None-caller that forgets the external connect ships a shell button. Add a guard test that every *_preview_button on the window opens a preview when clicked.
…zes them Pre-existing failure: the per-mode units annotation editors (error/fit/root/stats *_units_inputs_editor / *_units_constants_editor / fit_units_parameters_editor) are real ConstantsEditor state owners but were mounted with no objectName, so the GUI schema scanner flagged all 8 as 'unexpected editable state owner' and test_gui_schema_scan failed. Mirror the input_constants_editor pattern: set each editor's objectName to its owner attribute, and add the units-editor names to the scanner's expected-objects allow-list (they are display-only, so they carry no datalab_state_role). test_gui_schema_scan now passes.
…lake test_root_solving_worker_emits_cancelled_when_subprocess_interrupts and test_fit_batch_worker_emits_cancelled_when_self_consistent_subprocess_interrupts waited on the worker cancelled signal with a 3s budget. The signal is emitted from the worker QThread and delivered to the main-thread event loop; the connection is established before start() so it can only be delayed, not missed. Under the full parallel suite the event loop is contended and 3s was occasionally too tight, making the tests flaky (pass in isolation, fail late in a 285-file run). Raise both the waitSignal and worker.wait budgets to 10s.
Whole-codebase review across 12 dimensions (GUI design, GUI/compute separation, backend performance, code quality, maintainability, GPU feasibility, feature support, modernization, LaTeX output, formula rendering, bugs/concurrency, security), synthesized from a Claude 12-auditor swarm cross-checked by Codex (gpt-5.5) and Antigravity/Gemini (Gemini 3.1 Pro). Findings are file:line-anchored with confidence marks (single/two/three-model agreement); severities recalibrated by the adversarial + cross-check passes; one model disagreement (mypy gate status) resolved by direct run. Includes an honest GPU-feasibility section and a phased P0/P1/P2 roadmap with effort estimates and dependencies.
Web fitting called _parse_fit_data + _column_series (which mp.mpf() the string cells) BEFORE opening the precision guard, so on a worker left at the ambient mp.dps (default 15) high-precision input columns were silently truncated to ~15 sig figs before the fit ran — same input gave different results web vs desktop. Wrap the parse + column/sigma/weights extraction in with _precision_guard(mp_precision). Adds a regression test proving a 1e-24 input digit survives (parsed at ambient dps=15 it rounds to 1.0).
…rer (P0-2) The formula render service parsed compute formulas with parse_expr but had no AST pre-check, so attribute-access gadgets (sqrt(1).__class__) and subscripting reached SymPy and were evaluated to the underlying type object — the first rung of a sandbox escape. Add _reject_unsafe_formula_ast() mirroring the security checks in shared.symbolic_math._validate_symbolic_ast, minus its capitalized- function whitelist so lowercase function names (sin/cos/...) keep rendering. Translate ^ to ** for the AST check only; the real parse still applies convert_xor to the raw string.
datalab_latex/expression_engine.py carried a byte-for-byte copy of shared/expression_engine.py — same safe_eval, same whitelist, same AST limits — differing only in format_latex_formula. Duplicated allowlists silently drift when a math function is added to one and not the other. Make it a thin shim that re-exports the full public + private surface from shared.expression_engine (same objects, not a copy) and overrides only format_latex_formula to route through the render service. An identity test in test_expression_registry.py pins that the two modules share the same objects so they can never drift again. The two safe-eval monkeypatch tests now target the shared implementation module (where safe_eval actually executes) instead of the shim.
mypy --strict on shared/fitting/extrapolation_methods/datalab_latex reported 4 errors that failed test_mypy_strict_zero_errors_on_full_core_layer: - unit_expression_validation._parse_factor built a factor dict with self._consume() (typed str | None) as the key, but from_factors wants dict[str, Fraction]. The token was already narrowed to a non-None str by _is_unit_name; use it as the key and call _consume() only to advance. - latex_tables_common and latex_tables_statistics_grouped imported three formatting helpers through the legacy data_extrapolation_latex_latest shim, whose dynamic re-export mypy cannot resolve (attr-defined). Import them directly from datalab_latex.latex_formatting where they are defined — the same pattern latex_tables_root already uses. mypy --strict now reports zero errors across all 92 core-layer source files.
ensure_tectonic_installed downloaded a ~30 MB binary from GitHub releases and extracted/executed it with no integrity check — a compromised mirror or MITM could deliver a swapped binary that then runs on the user's machine. Pin the SHA-256 of all five release archives this module can fetch (tectonic@0.15.0, computed from the immutable canonical GitHub release, which ships no upstream SHA256SUMS file to fetch at runtime) and verify the download against the pin between streaming and extraction. A mismatch or an unpinned asset raises the new TectonicChecksumError; the finally block still wipes the staging tree so nothing tampered survives. Tests: a mismatch aborts with no binary published and no staging leak; a structural guard asserts every platform the URL builder produces has a pinned digest so a version bump can't silently drop verification. The offline happy-path tests now register their fixture's real digest so the gate stays live rather than bypassed.
The web app set X-Content-Type-Options / X-Frame-Options / X-XSS-Protection but had no Content-Security-Policy, and loaded KaTeX from the jsdelivr CDN with no Subresource Integrity — a compromised CDN could inject arbitrary JS/CSS. - Add a CSP: default-src 'self'; script/style-src add 'unsafe-inline' (the templates carry inline <script>/<style> for theme+i18n bootstrapping) and the jsdelivr origin; img data: for base64 plots; connect 'self' for SSE/api; object-src 'none'; frame-ancestors 'none'; base-uri 'self'. - Pin the KaTeX 0.16.11 script and stylesheet with sha384 SRI hashes (computed from the canonical CDN bytes, verified stable across two fetches). Tests: assert the CSP header is present with the key directives, and that the KaTeX CDN tags carry SRI integrity. 53 security + web tests pass; the CSP is verified non-breaking (pages still render 200 with inline scripts/styles).
…2-5 stage A) The grouped and matrix LaTeX renderers re-ran validate_statistics_*_payload, which forced a datalab_latex -> datalab_core layering inversion. But the payload is already validated in datalab_core at generation time (statistics_grouped.py:235, statistics_matrix.py:77) before it reaches the renderer, and no caller feeds the renderers an unvalidated payload (desktop routes through the core generator; tests build payloads via run_statistics; the pytest.raises tests target the snapshot validators, not the renderers). Drop the redundant re-validation and its import. Removes 2 of the 3 datalab_latex -> datalab_core inversions with zero behavioral change; validators stay in datalab_core (the correct trust-boundary home). 71 grouped/matrix/latex-consistency tests pass.
…e E) Add a static AST enforcement test asserting datalab_latex does not import datalab_core, so the inversion Stage A removed can't silently return. The two renderers freed in Stage A (grouped/matrix) are pinned core-free directly; the one remaining edge — latex_tables_common importing five statistics-display helpers — is listed as a single tracked exception (its fix, relocating those helpers to shared/statistics_display.py, is P2-5 Stage B, a larger core-schema move deferred as directional-only with no functional benefit). Document the layering guard and the statistics_utils frontend-glue classification in ARCHITECTURE.md, and record the staged P2-5 status in the review doc.
_execute_calc_job was a ~800-line, ~178-cyclomatic function that inlined the extrapolation / error / statistics logic in one if/elif/elif under the precision guard. Extract each mode block into a named per-mode handler (_run_extrapolation_mode / _run_error_mode / _run_statistics_mode), mirroring how datalab_core exposes run_extrapolation / run_uncertainty / run_statistics. The handlers are nested functions so they keep sharing the setup closures (_check_cancelled, _v, the segment helpers) and the payload/headers/latex_path accumulators via nonlocal — no state-threading, no behavior change. The top-level dispatch is now a compact 3-way branch over job.mode. Verified behavior-identical: the full 199-test baseline (workers_core + the extrapolation/error/statistics UI suites) passes unchanged, plus root-solving / CLI-batch / example-workspace regressions. A structure test pins the decomposition so it can't collapse back into a monolith. ruff clean.
…tting) shared/plotting.py built every figure via the pyplot state machine (plt.subplots / plt.figure / plt.close), which keeps a non-thread-safe global figure registry — a hazard because the desktop renders plots on worker threads. And render helpers swallowed exceptions with a bare 'except Exception: return None', so a broken plot vanished with no trace. - Add _new_figure_ax() building figures via matplotlib.figure.Figure + FigureCanvasAgg (no pyplot global state). Migrate all 13 plt.subplots and the 1 plt.figure site to it; drop the 14 now-unneeded plt.close(fig) calls (OO figures aren't in the global registry) for fig.clear(). - plt stays imported/re-exported for external callers (MCMC corner plot, workers_core) — only the internal render helpers moved off the state machine. - Every 'except Exception: return None' render swallow now logs a warning with exc_info so failures are diagnosable while still degrading gracefully. Tests: an AST guard asserts no function calls plt.<attr>() internally, the OO factory is used, and no silent swallows remain. 121 plotting/web-plot/mcmc tests pass; mypy/ruff clean.
…rtial) Splitting the ~30 god-files (window.py 3181, workers_core.py 2793, statistics.py 2768, …) is an XL effort done file-by-file. Install the ratchet now so the problem stops worsening while the splits happen incrementally: - No NEW source file may exceed 800 lines (hard fail — split it, or consciously baseline it). - No existing god-file may grow past a frozen per-file baseline (+40 headroom for trivial edits), so the god-files can only shrink. - A staleness check forces dropping a file from the baseline once it's been split under 800, tightening the ratchet automatically. This is the '800-line CI warning' from P2-4. The remaining P2-4 work — actually splitting the god-files and finishing the views/ migration with a typed facade Protocol — is a large dedicated effort per file, left for follow-up.
|
Important Review skippedToo many files! This PR contains 354 files, which is 204 over the limit of 150. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (354)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
Correctness/robustness fixes from the CodeRabbit + Claude-swarm review of the roadmap PR (none were merge blockers; all are real): - resources.py: import Qt from PySide6.QtCore (not QtGui) for colorScheme() — QtGui works on some PySide6 builds but not all, and a failed import here is swallowed, which would silently disable the P1-5 cross-platform detection. - datalab_core/fitting.py: only forward weights/sigma to MCMC when the base fit was weighted — a weighted=False request carrying sigma data would run an unweighted LSQ fit then silently reweight the refinement. - mcmc_refine.py: reject non-positive/non-finite explicit likelihood weights (a negative weight raises log-prob for worse residuals, corrupting the posterior); skip refinement on observations/targets/weights length mismatch; try every evaluator arg-shape (propagate the last TypeError) instead of bailing on the first bound-but-rejected candidate. - gunicorn.conf.py: keep the floor of 2 workers even on an explicit WEB_CONCURRENCY override (=1 would reintroduce the single-worker blocking bug); parse DATALAB_WORKER_TIMEOUT/MAX_REQUESTS via a safe _int_env so a typo can't crash config load. - expression_engine.py shim: re-export compile_expression (completeness). - app_web/logic/root_solving.py: reject an empty guess in the 'name = guess' branch with the clearer format message (was deferred downstream). - style.css: scope the light-theme accent override to a:not(.download) so the P1-10 contrast fix stops recoloring download buttons to cyan. - shared/plotting.py: drop the erroneous WARNING+stack-trace log from the three per-value _*_finite_float coercion helpers (returning None for one bad cell is normal control flow, not a render failure); logging stays on the top-level renderers. mypy --strict clean (93 files); ruff clean; 99 tests across the fixed areas pass.
The review flagged several roadmap tests that assert the letter of a change but can't fail if it's reverted. Tighten them so they actually guard the fix: - test_desktop_theme_mode: add a test that monkeypatches styleHints().colorScheme() to Light/Dark and asserts True/False — this would have caught the QtGui-vs-QtCore import bug (which made the branch unreachable and returned None). - test_desktop_example_workspace_menu: stop putting QKeySequence in a set (it is not reliably hashable in PySide6 — segfaults); compare via a list. - test_mcmc_packaging_declarations: assert the [mcmc] extra via the parsed -e .[...] pointer, not a raw substring that also matches a comment; the scipy tests now assert the file IS a pointer instead of degenerating to duplicate pyproject checks. - test_gunicorn_config: add floor-of-2-on-override and _int_env-invalid-fallback tests for the two gunicorn fixes. - test_expression_compile_cache: assert the fit model callable compiles the expression exactly once at build time, not per evaluation (P1-1's actual win). - test_workers_calc_job_decomposed: assert each mode branch body is a single handler call (real extraction), not just that the handler names appear. - test_security / test_web_theme_toggle: bind the SRI assertion to the actual KaTeX CDN tags; make the contrast-override regex tolerate :not() scoping.
Codex flagged that the P2-3 nested handlers _run_extrapolation/error/statistics_mode shadowed the identically-named window mixin methods (window_statistics_mixin.py:287 window._run_statistics_mode). They live in different namespaces (nested closure vs class method) so there was no functional bug, but the name reuse is confusing. Rename the nested handlers to _execute_*_mode for clarity. No behavior change; 136 workers tests pass.
Owner
Author
Review round 1 — addressedRan three independent reviews on the roadmap changes (Claude adversarial swarm, CodeRabbit, Codex). Verdict: mergeable, no blockers — every confirmed finding was cosmetic, a robustness gap, or a self-passing test. All real findings are now fixed (commits Correctness / robustness
Test hardening — tightened 7 tests the review flagged as self-passing (MCMC substring-in-comment, theme tri-state, compile-once wiring, workers real-extraction, SRI bound to KaTeX tags, gunicorn floor/ Gates: |
The CI I added in P0-4 failed on the first real run: - Tests: PySide6 import failed with 'libEGL.so.1: cannot open shared object file' — the bare ubuntu runner lacks the system Qt libraries pytest-qt needs at configure time. Add an apt-get step installing libegl1/libgl1/ libxkbcommon0 + the xcb libs before the pip install. - Lint: 'mypy --strict shared fitting extrapolation_methods datalab_latex' reported datalab_core errors. Newer mypy applies a CLI --strict to datalab_core too (reached transitively from the roots), surfacing that non-strict layer's known issues on a clean cache. Drop the CLI --strict and rely on the [[tool.mypy.overrides]] strict=true block, which scopes strict to exactly the four roots. Apply the same fix to test_mypy_strict_zero_errors_on_full_core_layer so the test matches CI. Verified locally on a cleared .mypy_cache: 'mypy <roots>' → Success (93 files), and both mypy tests pass.
…e (P2-4) First concrete god-file split, chosen via a dependency-analysis workflow as the lowest-risk seam: history_compare.py's uncertainty-budget comparison family is self-contained (one internal call site, no other comparison family touches its helpers) and covered by test_history_compare.py. Move _compare_budget_rows + 7 _budget_/_missing_budget_ helpers into datalab_core/history_compare_budget.py (~142 lines); drop the dead _budget_row_token (never called). The shared formatting helpers (_parse_number, _diagnostic, _cell, _append_delta_from_values, _safe_key_token, _encoded_key_component) stay in history_compare and are imported by the new module at its top level. To keep this cycle-free, history_compare imports _compare_budget_rows *lazily* inside _build_history_comparison, so there is no import-time back-edge. history_compare.py shrinks 1879 -> 1765 lines; the P2-4 ratchet baseline is lowered accordingly. Verified: both modules import with no cycle; 30 history-compare tests pass unchanged; the extraction is behavior-identical. mypy/ruff clean. (The 4 pre-existing test_desktop_budget_panel failures are unrelated — they fail identically at HEAD without this change.)
CI installs a newer mypy (2.1.0) than local (1.20.2), and they disagree: - 2.1.0 flags cast(Sequence[Any], value) as redundant after the isinstance narrowing; 1.20.2 needs the cast because it still sees Any. Replace the cast with a typed intermediate 'result: Sequence[Any] = value' in root_solving_engine._required_sequence and fitting_engine._required_sequence — both versions accept it. (Removed the now-unused typing.cast import.) - shared/units.py: 2.1.0 flagged '_pint = None' after 'import pint as _pint' as an incompatible Module/None assignment. Import 'pint' and bind '_pint: Any = pint' in the try, keeping the None fallback. Verified clean under BOTH mypy 1.20.2 and 2.1.0 (93 files each); ruff clean; the shared-engine + units regression passes (the one remaining test_root_solving_normalization failure is pre-existing at 4c642eb).
CI (mypy 2.1.0) failed the lint gate with 8 datalab_core errors — redundant casts and non-Literal 'severity' args in history.py / workbench_model.py / statistics_*.py. These are reached transitively when the strict roots import datalab_core, and the base config's warn_redundant_casts surfaces them even though datalab_core is deliberately NOT in the strict perimeter. (They didn't show locally because this machine's editable datalab install points at a different worktree, so local mypy analyzed stale datalab_core sources — a local-env artifact, not a real pass.) Add a datalab_core.* mypy override (warn_redundant_casts=false, disable arg-type) so the gate reports only on the four strict roots. Verified: 'mypy shared fitting extrapolation_methods datalab_latex' → Success (93 files) under BOTH mypy 1.20.2 and 2.1.0; the mypy tests pass. Tightening datalab_core to strict is its own future task.
warn_redundant_casts is a global-only mypy setting — a per-module override is silently ignored, so the previous datalab_core.* override didn't silence the redundant-cast errors that mypy 2.1.0 raised through follow-imports. Fix the 5 genuinely-redundant casts directly instead: - history.py: sha256_bytes() already returns str and canonical_json() returns bytes, so the cast(str, ...)/cast(bytes, ...) wrappers are no-ops (4 removed; dropped the now-unused typing.cast import). - workbench_model.py: compute_workspace_hash() returns str (1 removed). The datalab_core.* override now only relaxes the strict severity arg-type check that leaks through follow-imports (which per-module override DOES support). Verified: mypy CI command → Success (93 files) under both 1.20.2 and 2.1.0; ruff clean; 58 history/workbench/workspace tests pass (cast removal is a no-op).
Fix the real bugs behind the pre-existing test failures so the suite is green: - root_solving/normalization: disabled constants (constants_enabled=False) still entered the problem and created uncertain inputs, because compute_dict() returns rows regardless of the enabled flag (its shared contract). Gate on constants_enabled in normalize_root_problem so disabled constants are ignored. - budget display leaked the encoded 'analysis_row:v1:<base64>' source key into user-facing output: the desktop budget dashboard CSV/pareto labels and the history-compare budget-delta keys showed (or re-encoded) the opaque token instead of the readable base key (e.g. contribution_percent.x). Add public budget_source_key_base() and decode at the display/label sites (budget_panel csv rows + pareto labels + _contribution_suffix; history_compare_budget._budget_source_token). - bilingual: the P1-7 run_button tooltip and the error-mode units tooltips/label were set once via _tr instead of registered for retranslation, so they stayed Chinese after a language switch (the run_button one was a regression from P1-7). Register them with _register_text(..., 'setToolTip') / for the label. Verified: 131 tests across the previously-failing suites pass; mypy/ruff clean.
- test_app_web_baseline_contracts: the comparison-fit test asserted a server- side literal fitCsvFilename = '...'; but the template resolves the filename client-side via a fitCsvIsComparison ternary. Assert the (correct) ternary + fitCsvIsComparison = true instead. - docs/TEST_MATRIX.md: add the 'pre-existing untracked' clause the P0.1 baseline release-gate test (test_release_test_matrix) requires — the release gate treats pre-existing untracked test files as known prior work and requires every referenced test path to be tracked.
test_error_constants_file_hiding_does_not_leak_to_other_constant_modes toggled win.use_constants_file_checkbox, but the high-fidelity workbench redesign (45ec3f2) removed that error-mode 'use constants file' widget — it is created nowhere (only dead hasattr-guarded branches in window.py still reference it), so the test raised AttributeError. Rewrite it as test_constants_editor_numeric_mode_tracks_the_active_mode covering the surviving contract: switching error -> root_solving -> custom fitting keeps the shared input_constants_editor visible and updates its numeric mode. 35 tests pass.
…cision guard test_datalab_core_has_no_float_constructor_conversions is a blanket AST scan that forbids float() in datalab_core (it silently drops mpmath precision). Two core modules added in this branch (mcmc_refine.py P1-3, statistics_hypothesis.py extended stats) legitimately must cross into float-only third-party samplers — emcee cannot consume mp.mpf, and SciPy's stats.t/stats.chi2 are float-only. Both already re-wrap results as mp.mpf(str(value)); the float() is only at the library boundary. Give the guard a line-level opt-out: a trailing "# float-bridge" comment on the same line documents the boundary and exempts that one call. Annotated all 16 legitimate sites (12 emcee, 4 SciPy). Every other float() in the core layer still fails the guard, so accidental precision loss is still caught. This test was green on main (neither module exists there) and went red when these modules landed on the roadmap branch — restoring CI green.
…workbench redesign Four regressions, all green on main, introduced on this branch: 1. Implicit fitter (compute layer, test_implicit_model ×3): the P1-6 parallel seed-solve worker (_solve_seed_variant_task) rebuilds the model from a text recipe via build_model_specification(), which produces a GENERIC evaluator with no per-point closure for the implicit variable — so evaluating the output expression raised "Unknown variable: u/delta". Exclude implicit specs from the parallel path (hp_fitter.py:675) via the existing implicit_definition marker; the serial branch keeps the real implicit closure. One-line guard. 2. Declared constants leaking into implicit param detection (test_implicit_detect_bypasses_invalid_constant_values): the redesign rewrote _raw_constant_names_from_editor() to go through constants_dict(), which drops empty-valued rows — so a blank-valued declared constant (CR) was mis-detected as a fit parameter. Restore main's value-independent name extraction. 3. mpmath.workdps precision violation (test_phase0_precision_guardrails): datalab_core/statistics.py used mp.workdps() (banned; mp.dps is process global). Switch to precision_guard(). Tests updated to the intentional redesign (constants consolidated into one shared input-section editor; no enable checkbox → active iff non-empty): - workbench panel title in fitting reads "Parameters" (constants left the variable panel). - state-ownership guard skips units editors (ConstantsEditor reused for symbol→unit maps, marked by *.units.* schema key, never a state role). - implicit schema-binding assert enters fitting/custom first (shared editor's schema key is bound per active mode). - constraints-checkbox/constants layout test asserts the new structure. - workspace round-trip asserts non-empty constants stay active.
…esults guards Cluster B — statistics routing guards (test_phase0_desktop_guardrails ×2): - compute now flows through the core session service, not a module-level compute_statistics(); rewrite test_statistics_direct_mode_attaches_source_row_ids to run the real path and spy on the display seam (source_row_ids still attach). - _render_statistics_plots gained a value_unit kwarg (unit-aware axes); update the guard's fake stub to accept it. Cluster C — layout order, result round-trip, workbench_results hang: - Workspace restore of a semantic result now routes through _set_result_text so it renders with the same markdown display as the live run. It previously setPlainText()'d the raw markdown, so toPlainText() returned pipes and the root-solving round-trip diverged (live rendered vs restored raw). (app_desktop/workspace_controller.py) - Shell section order test updated to the intentional new order (mode selector above the input section). - test_desktop_workbench_results: fit payload mocks gained units=None to match the real payload shape; without it, payload.units raised AttributeError inside the batch handler, which popped a modal QMessageBox.critical that blocked the headless run forever (the file-level "hang"). All regressions were green on main and introduced by the workbench redesign.
…red toPlainText Follow-up to routing workspace result-restore through _set_result_text (so the restored result renders markdown consistently with the live run). result_edit now renders markdown, so toPlainText() returns rendered text without pipes. The eight semantic-snapshot restore tests asserted raw markdown (pipes, "##") in toPlainText(); point them at the cached _last_result_text, which holds the authoritative markdown source. Same intent, correct field.
…-deterministic
Root-cause fix (real bug): QPushButton.setText() clears an explicitly-set
shortcut in PySide6, so every retranslation / run-stop text swap silently
dropped the run button's Ctrl/⌘+Return. It only survived when the new text
equalled the old — i.e. under a Chinese default (build text is Chinese), so it
passed locally but broke under English (CI). Re-apply the shortcut after any
run-button text change (_reapply_run_button_shortcut, called from the run/stop
setters and after _apply_language). Users switching to English keep the shortcut.
CI determinism (these passed locally, failed on CI's English/x86 runners):
- Pin the language in the desktop test fixtures that assert Chinese strings
(extrapolation/result-schema/statistics/formula-panel/global-options/
gui-workflows) via _apply_language("zh"); tests needing English override it.
A full forced-English suite scan confirmed these were the only locale-fragile
desktop tests.
- test_statistics_latex_summary_rows_include_confidence_interval_metrics:
str(mpf) truncates to mp.dps digits (process-global); pin mp.workdps(15) so a
prior test leaking higher precision can't change the rendered digit count.
- The quantum-defect-implicit example fit is genuinely heavy (~1-2 min locally,
slower on shared CI runners); give implicit examples a 300s run budget.
…ompilable
Swarm-review findings A1/A2 (both HIGH). Header cells (column titles) were
written directly into siunitx S columns without \multicolumn{1}{c}{...} wrapping,
so siunitx tried to parse the title as a number and aborted the LaTeX build. The
data-cell paths already wrap correctly; only the header rows bypassed it.
- latex_tables_fitting.py: wrap the six metric titles ($\chi^2$, Reduced
$\chi^2$, AIC, BIC, RMSE, $R^2$) in the selected-fit comparison header.
- latex_tables_statistics_matrix.py: wrap the covariance/correlation column
names in the matrix header.
Both are reachable whenever the user selects the siunitx path (dcolumn checkbox
unchecked) from desktop or web. Adds tests/test_latex_siunitx_header_wrapping.py
covering both headers under siunitx and dcolumn.
…ion) Swarm-review finding B1 (MEDIUM). FittingComparisonWorker.request_stop() only set a flag that run_fitting_comparison never checked, so Stop could not interrupt a running multi-candidate comparison — the worker fit every candidate to the end and only emitted `cancelled` afterwards. - datalab_core/fitting_comparison.py: check_cancelled() before each candidate in the compare loop (the expensive unit), using the existing core cancellation convention. - datalab_core/session.py: add external_cancellation_scope(checker) — a public context manager that makes check_cancelled() honor an external stop callable for compute entry points (like run_fitting_comparison) that run outside SessionService. ContextVar-scoped, restored on exit, so concurrent jobs stay isolated; None is a no-op. - workers: _execute_fitting_comparison_job_payload wraps the call in that scope and FittingComparisonWorker threads its _stop_requested flag through, mirroring FitWorker/RootSolvingWorker. Adds tests/test_fitting_comparison_cancellation.py (pre-cancel, cancel-between- candidates, None no-op, and normal completion).
…guage switch CodeRabbit review finding (minor). The prior shortcut fix re-applied only the shortcut after retranslation, but _apply_language replays setText on the run button and relabels it to the default "Run" text even mid-run — so switching language during a running job left the button showing "Run" while datalab_run_state was still "stop" (and the shortcut would stop the job): the visible label lied about what it does. Re-run the state-specific setter (_set_button_to_stop_mode / _set_button_to_run_mode) after retranslation so a running Stop button keeps its Stop label + state in the new language; both setters already re-apply the shortcut. Adds a regression test asserting label/state consistency across zh↔en in both run and stop states.
yilibinbin
pushed a commit
that referenced
this pull request
Jul 2, 2026
…10 vs #70) The first F10 pass cleared the previous mode's result inside _on_mode_change, but that defeated release-gate contract #70 (test_run_validation_error_preserves_previous_valid_result_overview): switching mode then hitting a validation error left "No results" because the result was already wiped at switch time, so there was nothing to preserve. Reconcile by moving the clear off the switch: a successful run in the new mode replaces the result (so stale wrong-mode output is never shown/exported once you actually compute — F10's real intent, satisfied via the _on_calc_finished -> _show_*_results path), while a failed or validation-errored run returns early and keeps the last good result (#70). Rewrite F10's own test to assert the new timing (switch preserves; a successful run replaces). Both previously-conflicting tests pass; 221 neighborhood tests pass; ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Summary
This branch executes the full P0/P1/P2 roadmap from
docs/CODE_REVIEW_2026.md(a tri-model whole-codebase review) on top of the prior analysis/workbench handoff work. Every roadmap item was done with per-item TDD and a separate commit; the branch is green (ruffclean,mypy --strict93 core files clean).P0 — Correctness & Security (all 6)
fix(web): parse fitting data inside the precision guard — high-precision input columns were silently truncated.fix(latex): reject attribute/subscript gadgets (sqrt(1).__class__) in the sympy formula renderer (sandbox-escape hardening).refactor(latex): collapse the two duplicate safe-eval engines into one shim (identity-tested).fix(types): clear the mypy--stricterrors in the core layer (now 0).fix(latex): verify the Tectonic download SHA-256 before install (supply-chain).ci: add test + lint CI across Python 3.11/3.12/3.13 (+ lockfile-sync job).P1 — High-Leverage Quality / Perf / UX (all 10)
perf(fitting): cache the parsed formula AST; compile the model once (~2.4x on the eval path).fix(web): deployment-layer concurrency root-fix —gunicorn.conf.pywith a hard floor of 2 workers (mpmathmp.dpsis process-global).feat(fitting): reach MCMC refinement throughdatalab_core(web/CLI parity, was desktop-only).fix(i18n): localize 442 English-only raises in the extended-stats modules + a structural guard.feat(desktop): cross-platform theme detection (QtcolorScheme()) + Auto/Light/Dark menu.perf(fitting): parallelize seed-variant solves via a picklable task (parallel==serial determinism test).feat(a11y): keyboard shortcuts + baseline focus rings (desktop + web).feat(web): add a/rootsroute for web/desktop parity.refactor(desktop): delete dead synchronous fit methods.fix(web): double-submit guard + light-theme AA contrast fix.P2 — Modernization / Structural (6 landed; 2 partial/deferred by design)
build: commituv.lockas the reproducibility pin.build: unify the four requirements files behind pyproject extras.refactor(desktop): split the ~800-line_execute_calc_jobinto per-mode handlers.datalab_latex→datalab_coreinversions removed) + an AST layering-guard test. Stage B (relocating 5 display helpers toshared/) is documented + deferred (directional-only, no functional benefit).fix(latex): one canonicallatex_escape, fixing a web double-escape output-corruption bug.fix/refactor: web CSP header + KaTeX SRI; plotting moved to the OO matplotlib API (thread-safe) with render failures logged instead of swallowed.Test plan
ruff check .cleanmypy --strict shared fitting extrapolation_methods datalab_latex→ 0 errors (93 files)Follow-up 1 — Drive CI to truly green (regression triage)
A robust per-file suite scan surfaced 36 failures that were green on
main— all regressions from this branch's own work (mostly the45ec3f2workbench redesign), plus environment-only failures that never appear locally. Each was root-caused and fixed at the correct layer (not by weakening tests):Real defects (not just test fixes):
Unknown variable: u/delta. Guarded implicit specs out of the parallel path.constants_dict(), which drops empties).QPushButton.setText()clears an explicit shortcut in PySide6, so switching to English silently dropped the run button'sCtrl/⌘+Return— a real UX bug hidden locally by a Chinese default. Now re-applied after every text change.mp.workdpsprecision violation indatalab_core/statistics.py→precision_guard._set_result_text.CI-environment determinism (passed locally, failed on CI's English/x86 runners): pinned language in the locale-asserting desktop fixtures (a full forced-English suite scan confirmed the complete set), pinned
mp.dpsin a digit-formatting test against precision leakage, and gave the heavy implicit example a larger run budget.Follow-up 2 — Adversarial swarm code review
Ran a multi-agent swarm review over the full changed surface (134 source files, 6 dimensions: compute-correctness, precision discipline, UI/core sync, concurrency, web-security, LaTeX). Each finding was adversarially verified (independent skeptics trying to refute it); 4 raw findings → 1 refuted, 3 confirmed and fixed with TDD:
Scolumns without\multicolumn{1}{c}{...}wrapping, so LaTeX compilation aborted whenever the user selected the siunitx path (dcolumn unchecked) for the selected-fit comparison table or the covariance/correlation matrix. Both headers now wrap; regression tests added.FittingComparisonWorker's Stop only set a flag the candidate loop never checked. Addedexternal_cancellation_scope+check_cancelled()per candidate so Stop interrupts a running comparison.Follow-up 3 — CodeRabbit review
Also ran a CodeRabbit review over this session's changes. It surfaced 1 minor finding (no critical/high/medium): the run-button shortcut fix re-applied only the shortcut after retranslation, so switching language mid-run left the button showing "Run" while
datalab_run_statewas still"stop". Fixed by re-running the state-specific setter after retranslation (Stop stays Stop in the new language); regression test added.All fixes verified: ruff clean, mypy clean, layering/precision guards green, full local suite green, and CI green on all three Python versions.
🤖 Generated with Claude Code