[cachafla]

Pull Request Description

What and why?

Figures that AI drafting embeds in a document showed "Result not found" after the test ran again. The library keys each figure <result_id>:<4 random chars> (validmind/tests/output.py, TestResult.add_figure), and makes a new key on every run, so the key an embed holds is gone after the next run.

The keys must stay unique per run. The platform stores each image at figures/<key>/<key>.png, so a stable key such as <result_id>:<n> would overwrite the image files of older runs, and the test history would show the newest image. Uploads also run at the same time (asyncio.gather), so the platform cannot read an order from them.

So each figure now sends its position in the test result:

• Figure has a new index field.
• TestResult._pre_serialize_figures (it runs before every upload, in log() and log_async()) sets figure.index to the figure's position in result.figures.
• Figure.serialize() sends it as metadata._index. Keys do not change.

Comparison tests also send metadata._variant: the input and param combination of the sub-result that made the figure (sorted JSON, set in _combine_figures). They put each combination's figures one after another in input order, so a position alone can point at another combination's figure after a re-run with the inputs in another order. The platform matches a position only within the same variant (validmind/backend#3891, validmind/frontend#3072).

The platform uses _index to find the same figure after a re-run (validmind/backend#3882 and validmind/frontend#3066, both merged).

How to test

• uv run python -m unittest tests.test_results tests.test_comparison_figures: 37 pass.
• test_each_figure_sends_the_combination_that_made_it fails without the change (no _variant) and passes with it.
• test_figure_serialize_omits_index_until_logged checks that a figure that was never logged sends no _index.
• The new test test_test_result_log_async_sends_each_figures_position fails without the change (no _index in the metadata) and passes with it.
• Manual: run a test with two figures, for example validmind.model_validation.sklearn.ROCCurve on two datasets, and log it. Each log_figure request has metadata with _index 0 and 1.

What needs special review?

• Async tests that do not run: the async test methods in TestResultClasses (for example test_test_result_log_async) are on a plain unittest.TestCase, so they are never awaited. They pass without running, and they also call MockAsyncResponse with a json= argument that it does not accept. The new test is synchronous and uses asyncio.run, so it really runs. I did not change the existing ones.
• Mismatch between versions: with an older platform, the extra metadata key has no effect.

Dependencies, breaking changes, and deployment notes

• No breaking change. Keys and upload order do not change.
• Works with validmind/backend#3882 and validmind/frontend#3066 (both merged). Each PR works alone: without _index, the platform falls back to the exact key, then to the result's only figure.

Release notes

Figures now record their position in a test result, so documents that embed a figure keep showing it after the test is run again.

Checklist

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

🤖 Generated with Claude Code

[juanmleng]

Review: #570 at 1d5da51 (merge-base cc64d40)

Verdict: Merge with recommendations

Computed by the calibrated-review skill, run by @juanmleng.

Summary

• What changed: Each logged figure now sends its position in the test result as metadata._index, so the platform can find the same figure after a re-run.
• Result: No blocking issues. One small fix to the PR body, one item for later, and one design question.
• Main thing to know: Position works as identity only while the figure list keeps the same shape between runs. When it changes, an embed can show a different figure instead of "Result not found".
• Next step: Edit the PR body (FN1), answer Q1 here, then merge.

Fix now

FN1

• What: The PR body names the Shortcut story twice ("PRs on SC-…").
• Why: This repo is public. The story id belongs in the title only.
• Where: PR description, "What and why?" and "Dependencies" sections.
• Suggested fix: Link backend#3882 and frontend#3066 instead.

---

FN2

• What: Nothing checks that _index is left out for a figure that was never logged (index is None).
• Why: This is the backward-compatible branch of serialize(). A one-line assertion pins it.
• Where: validmind/vm_models/figure.py:149, tests/test_results.py
• Suggested fix: Assert that "_index" isn't in the metadata of Figure(...).serialize().

Fix later

FL1. On the platform side, the embed matcher falls back to "the result's only figure" even when the embed recorded an _index that the new run doesn't have. Say a test logged two figures, an embed points at the second, and the next run logs one. The embed then shows the first figure under the second one's caption, both in the app and in exports. This doesn't block this PR because the matcher lives in the already-merged backend change. The fallback should apply only to embeds without an _index, meaning those from older libraries. Suggested owner: the backend change's author.

Questions for a human

Q1

• Owner: @cachafla
• Question: Is the figure's position a stable enough identity, or should each figure also send something that describes it, so the platform can refuse a mismatch?
• Context: Comparison tests (validmind/tests/comparison.py:222) concatenate figures in input-combination order under the same result id. Re-running ROC Curve with datasets [train, test] and then [test, train] keeps both positions, but an embed of the test-set curve now shows the train-set curve. add_figure/remove_figure shift positions the same way. The third acceptance criterion rules out a wrong figure for older libraries. This is the same outcome for the new one. One option is to send the sorted input ids and params alongside _index, and require them to match on the platform.

Accepted risks

• What I checked: Whether any figure upload bypasses _pre_serialize_figures, which would leave _index unset.
• Why it is acceptable: alog_figure is called only from TestResult.log_async, which runs _pre_serialize_figures first, and log() runs it too.

---

• What I checked: Comparison tests reuse the sub-results' Figure objects, so index gets overwritten.
• Why it is acceptable: Sub-results aren't logged. Only the combined result is, and its index is set right before upload.

---

• What I checked: An older platform receiving _index.
• Why it is acceptable: It's an extra metadata key that older platforms ignore. Keys and upload order don't change.

Verification performed

• uv run python -m unittest tests.test_results at 1d5da51 → 35 passed
• test_test_result_log_async_sends_each_figures_position with validmind/vm_models reverted to main → failed, as expected
• git grep alog_figure → one call site, after _pre_serialize_figures
• Platform find_figure read on backend main → matches key, then _index, then only figure
• PR body claim that manual log_figure requests carry _index 0 and 1 → NOT CHECKED

[cachafla]

@juanmleng thank you for the review. Here is what changed for each item:

FN1: the PR body has no story ids now. It links validmind/backend#3882 and validmind/frontend#3066 instead.

FN2: test_figure_serialize_omits_index_until_logged (f933a2b) checks that a figure that was never logged sends no _index, and that a figure with an index sends it.

FL1: fixed on the platform, because #3882 and #3066 were already merged: validmind/backend#3891 and validmind/frontend#3072. When the embed's figure had an _index, the matcher now takes only the figure at that position. If there is no such figure, it shows "Result not found". The only-figure fallback stays, but only for figures from older libraries, which have no _index.

Q1: position alone is not enough for comparison tests, as you showed. In 2d1ef94, each figure of a comparison result also sends metadata._variant: the inputs and params of the sub-result that made it, as sorted JSON (set in _combine_figures). #3891 and #3072 match a position only when the variant is the same. So [train, test] followed by [test, train] gives "Result not found", not the train-set curve. Normal tests send no variant. For them, a list of figures that changes shape through add_figure or remove_figure can still match by position. I think that is acceptable, because the order there comes from the test code, not from the inputs.

CI: the failed "tests" job at 2d1ef94 is test_oidc_device.test_poll_device_token_success_after_pending. It patches the global time.monotonic with four fixed values, so any other call to it during the test makes the poll time out. It passed at f933a2b and passes locally, so I started a rerun. Its fix (patch time.monotonic only in validmind.oidc_device) is outside this PR.

[juanmleng]

Thanks — all four addressed. Checked 2d1ef94: tests pass, the comparison test fails without the change, and validmind/backend#3891's matcher matches what you described. Approving; worth merging validmind/backend#3891 and validmind/frontend#3072 before or with this one.