Skip to content

[SC-15877] Support pandas Styler outputs in test summary tables - #497

Closed
cachafla wants to merge 7 commits into
mainfrom
cachafla/sc-15877/style-summary-table-cells
Closed

cachafla wants to merge 7 commits into
mainfrom
cachafla/sc-15877/style-summary-table-cells

Conversation

@cachafla

@cachafla cachafla commented Apr 27, 2026 •

Copy link
Copy Markdown
Contributor

What and why?

Lets test authors style cells in their test summary tables — color-coded status pills, severity shading, bold thresholds — by returning a pandas.Styler from a test. Styled cells are serialized into a portable, JSON-safe schema that the documentation UI, HTML reports, and DOCX exports all render consistently.

This PR is the library piece. The backend (HTML/DOCX rendering) and frontend (UI rendering) pieces are in companion PRs.

Schema (shared across the three repos): cells may be a scalar or a dict of the form

{ value, bgcolor, color, fontWeight, textAlign }

What changed

  • validmind/tests/load.py — pandas.io.formats.style.Styler is now an accepted TABLE_TYPES output from a test.
  • validmind/tests/output.py — TableOutputHandler accepts Styler and converts it via a new _convert_styler helper. Each cell that has CSS styles applied (background-color / background, color, font-weight, text-align) becomes a {value, bgcolor, color, fontWeight, textAlign} dict; the formatted display value (per the Styler's formatters) is preserved under value. Unstyled cells stay as plain scalars.
  • validmind/vm_models/html_renderer.py — StatefulHTMLRenderer renders structured cells back to HTML for in-notebook display, so notebook output matches portal rendering instead of showing raw {'value': ...} JSON. CSS values are HTML-escaped.
  • notebooks/how_to/tests/custom_tests/style_custom_test_tables.ipynb — a how-to demonstrating styled-cell output from a custom test.
  • tests/test_results.py — covers the Styler conversion path (formatter preservation, all four CSS properties, HTML rendering).
  • validmind/tests/decorator.py — the @vm.test / @vm.scorer docstrings listed only list of dictionaries or a pandas DataFrame as valid table outputs; they now list Styler too and point at ResultTable for the cell schema. These docstrings are the source for the generated Quarto API reference on docs.validmind.ai.
  • validmind/vm_models/result/result.py — ResultTable docstring documents the structured cell schema (shape, partial keys, mixed scalar/dict columns, formatter behavior, verbatim CSS values) so the wire format is written down in the repo and not only in PR descriptions.
  • scripts/run_e2e_notebooks.py — the new how-to is added to NOTEBOOKS_TO_RUN so CI executes and logs it end to end.

How to test

  1. Open the new how-to: notebooks/how_to/tests/custom_tests/style_custom_test_tables.ipynb.
  2. Run a custom test that returns a pandas.Styler (e.g., df.style.map(lambda v: "background-color: #EAF4FF" if ...)).
  3. Inspect the resulting TestResult — styled cells should be dicts of the shape above; unstyled cells remain scalars.
  4. Render the result in a notebook — the styled cells should display with the matching colors / weights, not as {'value': ...} JSON.
  5. Run the test suite: pytest tests/test_results.py -k styler.
  6. Check the updated docstrings: python -c "import validmind as vm; help(vm.test)" should list pandas Styler as a table output, and help(vm.vm_models.result.ResultTable) should describe the styled-cell schema.

What needs special review?

  • The _convert_styler mapping from CSS properties to schema keys (style_key_map) — this defines what styling is portable. We currently support background[-color], color, font-weight, text-align. Anything else is dropped.
  • The documented cell schema in the ResultTable docstring must stay in sync with what the backend and frontend actually render — it is now the canonical written description of the contract.
  • Formatter handling: the display value is run through the Styler's _display_funcs before being placed in the cell dict, so user-applied number formatting is preserved.

Dependencies, breaking changes, and deployment notes

Release notes

Tests can now return a pandas.Styler to apply cell-level styling (background color, text color, bold, alignment) to summary tables — the styling is preserved consistently in the documentation UI, HTML reports, and DOCX exports. See the new style_custom_test_tables how-to notebook.

Checklist

  • What and why
  • Screenshots or videos (Frontend) — N/A
  • How to test
  • What needs special review
  • Dependencies, breaking changes, and deployment notes
  • Labels applied
  • PR linked to Shortcut
  • Unit tests added
  • Tested locally
  • Documentation updated (how-to notebook added)
  • Environment variable additions/changes documented (if required)

Accept pandas Styler as a valid table output type. TableOutputHandler
now converts a Styler into a DataFrame whose styled cells are
serialized as {value, bgcolor, color, fontWeight, textAlign} dicts —
a portable, JSON-safe schema consumed by the documentation UI and
report renderers.

StatefulHTMLRenderer renders structured cells back to HTML for
in-notebook display so notebook output matches portal rendering
without leaking raw {'value': ...} JSON.

Adds a how-to notebook demonstrating styled-cell output from a
custom test, plus tests covering the Styler conversion path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cachafla cachafla added enhancement New feature or request python Pull requests that update Python code labels Apr 27, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR Summary

This PR introduces significant improvements to the handling and rendering of custom test tables within the ValidMind framework. The key changes include:

  1. A new Jupyter notebook (style_custom_test_tables.ipynb) has been added that demonstrates how to use native pandas Styler to apply rich, portable cell-level styling to test result tables. Two custom tests are showcased: one returning a pandas Styler and the other returning structured cell dictionaries. This helps users choose the appropriate method based on their needs.

  2. The unit tests in tests/test_results.py have been updated to verify that the table output handler correctly converts pandas Styler objects by preserving CSS properties (e.g., background-color, color, font-weight, text-align) into a structured format. Additional tests ensure that interactive figure toggling behaves correctly under various environment variable values.

  3. Updates in the ValidMind core include modifications in the table conversion logic to support pandas Styler alongside DataFrame and ResultTable objects. In particular, the TableOutputHandler has been extended with a helper function that converts a Styler instance into a DataFrame with portable cell styles.

  4. The HTML renderer has been enhanced to support rendering of structured cell dictionaries. A new helper function ensures that cell values are safely rendered as HTML after applying necessary style mappings and escaping.

These changes aim to offer improved flexibility and consistency in how styled test tables are processed, displayed, and logged, leading to a better user experience when working with visual test outputs.

Test Suggestions

  • Verify that the conversion of a pandas Styler object to a structured DataFrame preserves all intended CSS properties and formats.
  • Test the HTML rendering of structured cell dictionaries, checking that all style attributes are correctly applied and escaped.
  • Run the interactive figure tests with different environment settings to verify that plotly data toggling behaves as expected.
  • Simulate edge cases by providing empty or malformed table data to ensure robust error handling in the TableOutputHandler.

@cachafla

Copy link
Copy Markdown
Contributor Author

@copilot resolve the merge conflicts in this pull request

…le-summary-table-cells

# Conflicts:
#	tests/test_results.py

Co-authored-by: cachafla <21595+cachafla@users.noreply.github.com>

Copilot AI commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Resolved the merge conflicts and merged main into this branch in 83732b5.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ cachafla
❌ Copilot
You have signed the CLA already but the status is still pending? Let us recheck it.

- decorator.py: @vm.test / @vm.scorer docstrings list `pandas Styler` as a
  valid table return type (these feed the generated Quarto API reference)
- ResultTable: document the structured cell schema so the wire format shared
  with the platform is written down in the repo
- run_e2e_notebooks.py: run style_custom_test_tables.ipynb in CI

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

@panchicore panchicore left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Review: validmind/validmind-library PR #497 at cea6188 (merge-base 7e0238d)

Verdict: Request changes (pending human confirmation)

Computed by the calibrated-review skill, run by Luis (panchicore).

Summary

  • What changed: Tests can now return a pandas.Styler instead of a DataFrame; styled cells are converted into portable {value, bgcolor, color, fontWeight, textAlign} dicts that the docs UI, HTML reports, and DOCX exports (companion PRs backend#3040, frontend#2470) will render.
  • Result: One blocking correctness bug: any test that returns a Styler silently loses the existing 4-decimal rounding for every numeric cell in the result table, not just the styled ones.
  • Main thing to know: _convert_styler calls .astype(object) on the whole DataFrame before placing any styled dict, so ResultTable.__post_init__'s self.data.round(4) becomes a no-op for the entire table the moment a test returns a Styler — even for columns and cells that were never styled.
  • Next step: Round numeric values before/independently of the astype(object) conversion (e.g. round the source DataFrame first, or round each extracted scalar when building the cell dict), then add a regression test with an unformatted numeric column that has no styling at all.

Blocking findings

Styler conversion silently defeats table-wide value rounding

  • Type: correctness
  • Direction: risky
  • What happens: A test returns df.style.map(...) where the CSS rule only touches one column (or even zero cells, if the condition never matches). _convert_styler does df = styler.data.copy(deep=True).astype(object) unconditionally, which converts every column — styled or not — to object dtype. ResultTable.__post_init__ then calls self.data.round(4), but pandas' .round() silently skips non-numeric (object) dtype columns instead of raising. The result: full, unrounded floating-point precision reaches ResultTable.serialize() for every cell in the table, including cells and columns that were never styled at all.
  • Why it matters: This contradicts the PR's own claim ("no breaking changes — existing tests... are unaffected") in spirit: any customer who adopts the new Styler feature on one column of a table gets a silent precision regression across the whole table, with no error and no visual cue in the notebook. It also inflates the JSON payload sent to the platform (the exact "serialization size" concern flagged for this review) since every float now serializes with full precision instead of 4 decimals.
  • Diagram:
flowchart TD
    A["Test returns df.style.map(...)\n(styles only column 'styled_col')"] --> B["_convert_styler:\ndf.astype(object) on ENTIRE frame"]
    B --> C["only cells matching the CSS rule\nbecome {value, bgcolor, ...} dicts"]
    C --> D["ResultTable.__post_init__:\nself.data.round(4)"]
    D --> E["pandas .round() skips\nobject-dtype columns silently"]
    E --> F["untouched_col AND unstyled cells\nin styled_col keep full float precision"]
Loading
  • Evidence:
    • validmind/tests/output.py:112 — df = styler.data.copy(deep=True).astype(object) runs before any styling is known to apply, converting every column.
    • validmind/vm_models/result/result.py:144 — self.data = self.data.round(4) is the existing rounding contract that silently no-ops on object dtype.
    • Ran the actual TableOutputHandler._convert_styler + ResultTable from the repo at the PR head (script below) against a two-column DataFrame where only styled_col has a styling rule and untouched_col has none:
      df = pd.DataFrame({
          "styled_col": [1.23456789, 2.3456789, 3.0],
          "untouched_col": [10.111111, 20.222222, 30.333333],
      })
      styler = df.style.map(lambda v: "background-color: red" if v > 2 else "", subset=["styled_col"])
      rt = ResultTable(data=TableOutputHandler()._convert_styler(styler))
      → rt.data.at[0, "untouched_col"] is 10.111111 (expected 10.1111 if rounding still applied); rt.serialize()["data"][0] is {'styled_col': 1.23456789, 'untouched_col': 10.111111} — full precision, unrounded, on a column that has zero styled cells.
    • tests/test_results.py:138-172 (test_table_output_handler_converts_pandas_styler) is the only Styler test added by this PR; it uses a Styler formatter (.format({"Observed": "{:.1%}"})) that turns the value into a string before it ever reaches .round(), so it never exercises an unformatted numeric column and does not catch this.
    • uv run python -m unittest tests.test_results -v → 32/32 pass, confirming the existing suite does not detect this regression.
  • Suggested fix: Round the source data before or independently of the object-dtype conversion — e.g. styler.data.copy(deep=True).round(4).astype(object) (round while still numeric, then widen dtype), or round each scalar right before building the cell dict / plain value in _convert_styler. Either way, add a test with a numeric column that has zero styled cells alongside a styled column, asserting it still comes out rounded.

Questions for a human

  • Owner: cachafla

  • Question: _convert_styler relies on three underscore-prefixed pandas internals — Styler._compute(), Styler.ctx, and Styler._display_funcs (validmind/tests/output.py:110-136). These aren't part of pandas' public API contract, so a future pandas point release inside the repo's own pandas (>=2.0.3,<3.0.0) constraint (pyproject.toml) could rename or restructure them without a deprecation warning, breaking every customer notebook that returns a styled table with an AttributeError deep inside test execution. Is the team OK accepting that maintenance risk, or should this pin a narrower pandas range / add a version guard with a clear error message?

  • Context: I could not install pandas 2.0.3 in this environment to test the low end of the declared range (build fails under Python 3.14 due to missing pkg_resources), so I can't show it actually breaks anywhere in the supported range — verified only that it works on the currently pinned 2.3.3. Per the skill's reachability rule, "I could not construct the failing scenario" downgrades this from a blocking finding to a question rather than a defect.

  • Owner: cachafla (and the backend/frontend reviewers on the sibling PRs)

  • Question: The ResultTable docstring now states plainly that "CSS values are passed through verbatim from the test author" (validmind/vm_models/result/result.py:135-136) — i.e., this library does zero sanitization of bgcolor/color/etc. before they're serialized and handed to the platform. That's a reasonable design (the library is the producer, not the renderer), but it means backend#3040 and frontend#2470 are the only places that can stop a malicious or malformed CSS string from reaching an HTML/DOCX document. Has that expectation been confirmed with those two PRs' reviewers, since this PR doesn't itself require it?

  • Context: Within this repo, the one place styled cells are rendered to HTML (validmind/vm_models/html_renderer.py:24-51, in-notebook display) does escape correctly — both the CSS value and the display value go through html.escape() before being placed in a style="..." attribute — so the library's own rendering is safe. The verbatim pass-through only becomes a risk in the two repos this review is explicitly not covering.

Accepted risks

  • What I checked: Whether the new Styler/DataFrame dual acceptance in TableOutputHandler.can_handle/process (validmind/tests/output.py:105-106,172-177) breaks the "no breaking changes" claim for existing DataFrame, dict, list, and scalar test outputs.

  • Why it is acceptable: Confirmed the isinstance(table_data, pd.DataFrame) branch and _convert_simple_type are untouched; ran the full tests/test_results.py suite (uv run python -m unittest tests.test_results -v → 32/32 pass) with no regressions outside the Styler path. The claim holds for pre-existing table types.

  • What I checked: Whether escape=False in StatefulHTMLRenderer.render_table's to_html() call (validmind/vm_models/html_renderer.py:194) lets a raw scalar test value inject HTML/script into a notebook.

  • Why it is acceptable: That escape=False setting predates this PR (present unchanged in validmind/vm_models/html_renderer.py at the merge-base) and applies to plain scalar cells exactly as it did before — this PR doesn't touch that exposure, it only adds a formatter (_render_table_cell) that, for the new dict-shaped styled cells, actively escapes both the CSS values and the display value before insertion. So this PR makes the escaping story strictly better for the cells it touches and changes nothing for the cells it doesn't. Out of scope to fix here.

  • What I checked: Whether a plain DataFrame could reach a code path that specifically expects a Styler (or vice versa), given the task's "what if the wrong type is passed" framing.

  • Why it is acceptable: No such path exists — TableOutputHandler and TABLE_TYPES (validmind/tests/load.py:24) accept both types interchangeably and dispatch by isinstance inside the same function, so there's no "Styler expected but got DataFrame" branch to break.

Verification performed

  • git merge-base cea6188f0939920f74954adddf82f2db4f7a3b85 origin/main → 7e0238d6c9bef513f010ce6eb613dad6e079454e (matches baseRefOid reported by gh pr view).
  • gh pr view 497 --repo validmind/validmind-library --json ... → confirmed head cea6188f09..., no existing reviews, author cachafla ≠ current user panchicore (not a self-review).
  • git worktree add <tmpdir> cea6188f0939920f74954adddf82f2db4f7a3b85 — reviewed at a pinned, isolated checkout rather than switching branches in place.
  • uv sync in the worktree → resolved env with pandas 2.3.3 (within the repo's pandas (>=2.0.3,<3.0.0) constraint), jinja2, etc.
  • uv run python -m unittest tests.test_results -v → 32/32 passed (includes the PR's new test_table_output_handler_converts_pandas_styler).
  • uv run python -m unittest tests.test_results -k styler -v → 1/1 passed (isolated).
  • Ran a standalone probe script importing the actual TableOutputHandler and ResultTable from the PR head to reproduce the rounding regression (see Blocking findings evidence above).
  • Verified notebooks/how_to/tests/custom_tests/style_custom_test_tables.ipynb is valid JSON (json.load succeeds); did not execute it end-to-end (no ValidMind API credentials in this environment) — PR claim that CI runs it via scripts/run_e2e_notebooks.py is NOT CHECKED beyond confirming the notebook was added to NOTEBOOKS_TO_RUN.
  • Checked repo root for AGENTS.md/CLAUDE.md/CONTRIBUTING.md conventions files — none exist in validmind-library, so no repo-specific convention violations to check against beyond what's inferred from the code itself.
  • Confirmed escape=False in html_renderer.py's render_table predates this PR by diffing against the merge-base — not a regression introduced here.
  • PR claim "No breaking changes — existing tests that return pd.DataFrame, dict, or scalar tables are unaffected" — CHECKED, holds (see Accepted risks).
  • Attempted to install pandas 2.0.3 (the low end of the declared support range) to test private-API stability at that version — failed to build under Python 3.14 (ModuleNotFoundError: No module named 'pkg_resources'); NOT CHECKED at that version, only at the currently pinned 2.3.3.

_convert_styler widened the whole frame to object dtype before
ResultTable.__post_init__ ran .round(4), which silently skips object
columns. Round while the data is still numeric; formatters keep
seeing the original value.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@cachafla

Copy link
Copy Markdown
Contributor Author

Thanks @panchicore. Addressed in 6095c6f.

Rounding regression (blocking): _convert_styler now rounds while the frame is still numeric (source.round(4).astype(object)), so ResultTable.__post_init__'s .round(4) no longer silently no-ops on the whole table. Formatters still receive the original, unrounded value. Added test_table_output_handler_styler_keeps_rounding, which uses a numeric column with zero styled cells alongside a styled column with no formatter and asserts both come out at 4 decimals. Your probe (untouched_col → 10.1111) now passes.

Private pandas API (_compute, ctx, _display_funcs): verified only on pandas 2.3.3, same as you. I am accepting the maintenance risk for now rather than narrowing the pandas range; if a point release breaks these, the failure is an AttributeError at test-run time and easy to spot in the e2e notebook run.

CSS verbatim pass-through: confirmed with the sibling PRs. The library only produces; backend#3040's allowlist (utils/table_cell_styles.py) and frontend#2470's color2k parse-and-re-emit are the sanitizing layers. I cross-checked 16 boundary cases (3/4/5/6/8-digit hex, rgb/rgba, hsl/hsla, space-separated hsl, deg, negative hue, named colors, javascript:, red;x) and both sides agree on every one.

@cachafla
cachafla requested a review from panchicore September 14, 2026 16:50
…sion it caught

A regression test for unstyled tables found that render_table's new
per-column formatter returned raw numbers, which pandas rejects
(TypeError: object of type 'numpy.float64' has no len()). Attach the
formatter only to columns that hold a styled cell, and return strings
from it, so plain columns render exactly as before.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@cachafla

Copy link
Copy Markdown
Contributor Author

Follow-up in b71a600: added a regression test for the untouched path, and it caught a real bug.

  • New test_table_output_handler_plain_dataframe_is_unchanged runs a plain DataFrame (string + float columns) through TableOutputHandler and asserts both serialize() and to_html() match the pre-PR output.
  • It failed on the PR head: render_table attached the new _render_table_cell formatter to every column, and for non-dict cells it returned the raw value. pandas requires formatters to return strings, so any plain table with a numeric column crashed on to_html() with TypeError: object of type 'numpy.float64' has no len(). That would have broken in-notebook display for every existing test that returns a DataFrame.
  • Fix: the formatter is only attached to columns that actually contain a styled cell, and it returns str(value) for the plain cells in those columns. Plain columns keep pandas' default rendering byte-for-byte. test_table_output_handler_styler_keeps_rounding now also renders to HTML to cover the mixed styled/plain column case.

34/34 passing.

@panchicore panchicore left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🌮

Review: validmind/validmind-library PR #497 at b71a600 (merge-base 7e0238d)

Verdict: Merge with recommendations

Computed by the calibrated-review skill, run by Luis (panchicore). This is a delta re-review: the prior CHANGES_REQUESTED review (2026-09-14T15:26:33Z, at head cea6188f0) requested one blocking fix; this re-review checks commits 6095c6fb and b71a600f3 against that finding and reviews them for new regressions.

Summary

  • What changed since the last review: 6095c6fb fixes the rounding regression from the prior review. b71a600f3 adds a regression test for plain (non-Styler) DataFrame tables, which caught and fixed a second, pre-existing bug in the same feature: in-notebook HTML rendering crashed for any table with a numeric column, Styler or not.
  • Result: The one blocking finding from the prior review is resolved and verified against the actual code, not just the author's claim. No new blocking findings.
  • Main thing to know: The fix for the crash bug (b71a600f3) introduces one narrow, cosmetic formatting inconsistency in the in-notebook HTML view — non-blocking, included below as a fix-now item.
  • Next step: Optional: apply the fix-now formatting nit. Otherwise this is ready to merge; the two risk-appetite questions from the prior review remain open for a human to sign off on (author has responded to both, see below).

Previous blocking findings — status

  • Styler conversion silently defeats table-wide value rounding — RESOLVED. Verified at the code: _convert_styler now rounds while the frame is still numeric (validmind/tests/output.py:116, source.round(4).astype(object)) before the object-dtype cast that previously made ResultTable.__post_init__'s .round(4) a silent no-op. Re-ran the original review's reproduction probe against the fixed code (two-column frame, only one column styled): untouched_col now comes out 10.1111 (was 10.111111), and the styled cell's value also rounds to 2.3457. The author's new test test_table_output_handler_styler_keeps_rounding (tests/test_results.py:196-219) covers the same scenario and passes.

Fix now

  • What: In a numeric column that mixes styled and unstyled cells, unstyled cells now render via Python's str() in the in-notebook HTML view instead of pandas' default column-consistent decimal padding, so they can show different precision than their neighbors (e.g. 1.0 next to 100.25 instead of 1.00).
  • Why: It's a real, verified formatting inconsistency — not a data problem (the underlying value and the JSON serialize() payload are correct either way), and it's confined to this repo's own in-notebook display; the docs UI/HTML/DOCX exports render from the {value, bgcolor, ...} schema independently in the companion backend/frontend PRs, unaffected by this. Low cost to clean up while the file is already open in this PR.
  • Where: validmind/vm_models/html_renderer.py:24-28 (_render_table_cell's str(value) fallback) and validmind/vm_models/html_renderer.py:189-193 (formatter now applied per-column instead of per-cell).
  • Suggested fix: Format the fallback value with the same precision pandas would use for the column (e.g. via the column's own default formatter) instead of a bare str(), or accept it as intentional and note it in a code comment.

Questions for a human

  • Owner: cachafla

  • Question: (carried over, unresolved) _convert_styler still relies on three underscore-prefixed pandas internals (Styler._compute(), .ctx, ._display_funcs, unchanged by this delta — validmind/tests/output.py:110,117,127,138). Is the team OK accepting the maintenance risk of a future pandas point release renaming these without a deprecation warning?

  • Context: In his 16:50:31Z comment, cachafla confirmed he's accepting this risk rather than narrowing the pandas (>=2.0.3,<3.0.0) range, reasoning that a break would surface as an AttributeError at test-run time and be easy to spot in the e2e notebook run. I did not independently re-verify this (it wasn't part of what changed in this delta); flagging so a human explicitly signs off rather than this being implicitly accepted by silence.

  • Owner: cachafla (and the backend/frontend reviewers on the sibling PRs)

  • Question: (carried over, unresolved by this repo) Has the CSS verbatim pass-through — this library does zero sanitization of bgcolor/color/etc. before serializing them — been confirmed safe with the backend (backend#3040) and frontend (frontend#2470) reviewers, since those are the only places that can stop a malicious/malformed CSS string from reaching an HTML/DOCX document?

  • Context: In the same comment, cachafla says he cross-checked 16 boundary cases (hex colors of various lengths, rgb/rgba, hsl/hsla, deg, negative hue, named colors, javascript:, red;x) against backend's allowlist and frontend's color2k parser, and both agree on every one. This claim is about code in two other repos outside this review's scope — I did NOT check it; it's presented here as the author's claim, not verified evidence.

Accepted risks

  • What I checked: Whether the rounding fix (source.round(4).astype(object), dropping the previous explicit .copy(deep=True)) introduces an aliasing bug that mutates the caller's original DataFrame/Styler.data.

  • Why it is acceptable: .round() returns a new DataFrame rather than a view, so the subsequent .astype(object) and df.iat[...] = ... mutations never touch styler.data. Verified with a probe: after calling _convert_styler, both df["styled_col"] (the original source frame) and styler.data["styled_col"] remain [1.23456789, 5.0], unmutated.

  • What I checked: Whether the plain-DataFrame crash fix (b71a600f3) changes behavior for tables that were already working (Styler-styled tables, and plain tables with no numeric columns).

  • Why it is acceptable: Ran the full tests/test_results.py suite at the PR head — 34/34 pass, including the pre-existing Styler HTML test and the two new regression tests. The formatter is now attached only to columns containing at least one dict (styled) cell, so plain tables get no formatter at all and fall back to pandas' untouched default rendering, which is what the new test_table_output_handler_plain_dataframe_is_unchanged asserts byte-for-byte for the cells it checks.

Verification performed

  • gh api repos/validmind/validmind-library/pulls/497 → confirmed head b71a600f3be7e7974f220f2d3158b2ecfafcea0a, base 7e0238d6c9bef513f010ce6eb613dad6e079454e (same merge-base as the prior review — base branch has not moved).
  • gh api repos/validmind/validmind-library/pulls/497/reviews → read panchicore's CHANGES_REQUESTED review body in full (one blocking finding, two non-blocking questions).
  • gh api repos/validmind/validmind-library/issues/497/comments → read both of cachafla's response comments (16:50:31Z, 17:10:25Z) verbatim.
  • git worktree add --detach <path> b71a600f3... in an isolated clone (never checked out in a shared/main working copy) → reviewed at the pinned head.
  • git diff cea6188f09... b71a600f3... → confirmed only validmind/tests/output.py, validmind/vm_models/html_renderer.py, tests/test_results.py changed since the prior review.
  • uv sync in the worktree → resolved environment (pandas 2.3.3, same as the prior review).
  • uv run python -m unittest tests.test_results -v → 34/34 passed.
  • Reproduced the prior review's rounding probe against the fixed code → untouched_col now 10.1111 (rounded), styled cell value now 2.3457 (rounded) — matches the expected fix.
  • Wrote and ran a probe confirming the pre-existing plain-DataFrame HTML crash was real: checked out the prior review's head (cea6188f0, before either new commit) in a second isolated worktree and ran TableOutputHandler().process(df, result); result.to_html() on a plain two-column numeric DataFrame → TypeError: object of type 'numpy.float64' has no len(). Confirms this bug predates the delta (was already in the code reviewed on 2026-09-14, not introduced by the new commits) and that b71a600f3's fix is addressing a real, previously-unflagged defect, not a regression it created.
  • Wrote and ran a probe on a mixed styled/unstyled numeric column → confirmed the fix-now formatting inconsistency (1.0 via str() vs pandas' default 1.00 padding) is real and reproducible.
  • Wrote and ran a probe confirming no aliasing/mutation bug from dropping the explicit .copy(deep=True) in _convert_styler.
  • PR-body claim "34/34 passing" (17:10:25Z comment) — CHECKED, matches.
  • Author's claim of cross-checking 16 CSS boundary cases against the backend/frontend sibling PRs — NOT CHECKED (out of scope: requires reading code in two other repos).
  • Author's claim of accepting the private-pandas-API maintenance risk — NOT CHECKED beyond confirming the code is unchanged from the prior review; this is a risk-appetite decision for a human, not a code claim to verify.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@cachafla

Copy link
Copy Markdown
Contributor Author

Thanks for the re-review. Took the "accept it as intentional and note it in a code comment" option in e69208c. A column that contains a styled cell is object dtype, so there is no column-wide float padding for the plain cells to match; the comment in _render_table_cell now says so. Fully unstyled columns still get pandas' default rendering untouched.

The pandas private-API risk stays accepted as stated in my earlier comment.

@cachafla

Copy link
Copy Markdown
Contributor Author

Closing manually: this PR was already merged into main via GitHub-created merge commit 11ee7a2 (parents 7e0238d + ce844c3), but the PR record never transitioned to merged and the mergeability check stayed stuck on a stale trial merge. No further merge is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request python Pull requests that update Python code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants