Skip to content

fix(web): parse rule-preview versions as strictly as the Rust evaluator (audit M17) - #443

Merged
passcod merged 2 commits into
mainfrom
claude/pr-370-fix-m17-rule-preview-range
Aug 1, 2026
Merged

fix(web): parse rule-preview versions as strictly as the Rust evaluator (audit M17)#443
passcod merged 2 commits into
mainfrom
claude/pr-370-fix-m17-rule-preview-range

Conversation

@passcod

@passcod passcod commented Aug 1, 2026

Copy link
Copy Markdown
Member

Fixes M17 (medium) from the audit in #370.

The bug

healthcheck-rule-eval.ts opens by declaring the Rust IfLadder evaluator the source of truth and itself a mirror. Rust parses the left-hand side of in_range with parse::<node_semver::Version>() — strict. The preview ran it through semver.coerce(), which fabricates a full version from a partial ("2.28"2.28.0) and strips prerelease and build suffixes.

So an admin authoring status.tamanuVersion in_range >=2.28.0 against a server reporting 2.28.0-rc.1 sees "would file at critical" in the editor, while production ingestion evaluates the condition false and files at the base severity. The preview is the only feedback the editor gives, so the rule ships believed-working.

The fix

semver.valid(lhs) instead of semver.valid(semver.coerce(lhs)?.version ?? lhs). A partial is not a version, and a prerelease stays a prerelease — so it satisfies only a range that names one, which is what both semver.satisfies and node_semver's Range::satisfies do.

Test plumbing

Vitest was configured in vite.config.ts but had no tests and no CI step, so a regression test here wouldn't have run anywhere. This adds:

  • private-web/src/lib/healthcheck-rule-eval.test.ts — the first vitest suite;
  • a Frontend unit tests step in the Playwright job (it already does npm ci; the unit tests need no browser or database, so they run before the expensive setup and fail fast);
  • just test-web for local parity.

Five cases: partial versions rejected, prerelease not satisfying a stable range, ordinary versions still matching, prerelease matching a range that names one, non-string LHS false. The first two are confirmed to fail against the unfixed evaluator.

Two sibling findings in the same TS/Rust-mirror-drift pattern (L19 ==/!= reference equality, L20 duration rounding) are untouched here — now that there's somewhere to put them, they'd be easy follow-ups.


Generated by Claude Code

`healthcheck-rule-eval.ts` declares itself a mirror of the Rust `IfLadder`
evaluator, which parses the left-hand side of `in_range` with
`parse::<node_semver::Version>()` — strict, no coercion. The preview ran it
through `semver.coerce()` instead, which fabricates a full version from a
partial ("2.28" → 2.28.0) and strips prerelease and build suffixes.

So an admin authoring `status.tamanuVersion in_range >=2.28.0` against a
server reporting `2.28.0-rc.1` sees "would file at critical" in the editor,
while production ingestion evaluates the condition false and files at the
base severity. The preview is the only feedback the editor gives, so the rule
ships believed-working.

`semver.valid(lhs)` matches the Rust parse: a partial is not a version, and
a prerelease stays a prerelease (so it satisfies only a range that names
one).

Vitest was configured but had no tests and no CI step, so this adds both —
otherwise the regression test wouldn't run anywhere. `just test-web` is the
local equivalent.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SGfH1cdFKPnKpM7ytRThft
passcod pushed a commit that referenced this pull request Aug 1, 2026
Both this branch and #443 add the same `just test-web`, with different
comments — so git saw two different changes and conflicted on a recipe that
is identical in substance. Same text on both sides merges as one change,
which is cheaper than ordering the two PRs against each other.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SGfH1cdFKPnKpM7ytRThft
@passcod
passcod marked this pull request as ready for review August 1, 2026 22:20
The two siblings this PR's description deferred, done here now that there's a
vitest suite to put them in.

**L19 — `==`/`!=` used reference equality.** The comment claimed "strict
structural equality (JSON-level)", but `===` compares arrays and objects by
reference, so `[1,2] == [1,2]` is true in Rust's `json_equal` (which compares
`serde_json::Value`s structurally) and was false in the preview. The preview
reported the opposite of what the rule does. Now a real deep comparison,
key-order-insensitive as `serde_json::Value` equality is.

Also in L19: `toNumber` used `Number()`, which is more permissive than the
`str::parse::<f64>()` Rust uses — it accepts `0x10` as 16, and the TS trimmed
surrounding whitespace Rust rejects. Coercion is now gated on Rust's f64
grammar. `inf`/`nan` spellings are deliberately left out: they can only arrive
as strings, where a match is already structurally equal, and against a real
number the comparison is false either way.

**L20 — `humanSeconds` compounded rounding.** Each unit was rounded from the
previous *rounded* unit, so 1h29m35s rounded to 90m and then displayed "2h",
and 1d11h58m displayed "2d". Each unit now comes from the raw seconds. The
rounded lower unit still picks when to step up, so 3599s reads "1h" rather
than "60m".

Twelve new tests; seven of them fail against the unfixed code, the rest pin
behaviour that must not change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SGfH1cdFKPnKpM7ytRThft

passcod commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

Did the two follow-ups here rather than deferring them, so the whole TS/Rust mirror-drift cluster lands together.

L19 — ==/!= used reference equality. The comment said "strict structural equality (JSON-level)" but === compares arrays and objects by reference, so [1,2] == [1,2] is true in Rust's json_equal (comparing serde_json::Values) and was false in the preview — the preview stating the opposite of what the rule does. Replaced with a real deep comparison, key-order-insensitive to match serde_json::Value.

The second half of L19: toNumber used Number(), which accepts more than the str::parse::<f64>() Rust uses — 0x10 is 16 to JS and an error to Rust — and the TS trimmed whitespace that Rust rejects. Coercion is now gated on Rust's f64 grammar.

One deliberate omission: Rust's parse also accepts inf/nan spellings. Left out because they can only arrive as strings, where an equal string is already structurally equal, and compared against a real number the answer is false either way. Happy to add them if you'd rather the mirror be literal.

L20 — humanSeconds compounded rounding. Each unit was rounded from the previous rounded unit, so 1h29m35s → 90m → "2h", and 1d11h58m → "2d". Each unit now derives from the raw seconds. The rounded lower unit still decides when to step up, so 3599s reads "1h" and not "60m".

Twelve new tests across the two files. Seven fail against the unfixed code (all three == structural cases, !=, the Number()-vs-parse::<f64>() case, and both rounding cases); the rest pin behaviour that must not change. 17 pass total, biome clean.


Generated by Claude Code

passcod commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

Correction: "biome clean" in my previous comment was wrong — biome isn't used in this repo (no biome.json, no dependency, no lint script). I ran npx biome check, got no output, and reported that as a pass. It never ran a configured linter, so the claim was empty.

The rest of that comment stands: 17 vitest tests pass, seven of the new ones fail against the unfixed code, and tsc -b is clean.

I'd also introduced biome-ignore directives in the test file on #450, referencing rules from the same non-existent linter — they appeared nowhere else in the codebase. Removed there.


Generated by Claude Code

@passcod
passcod enabled auto-merge August 1, 2026 22:33
@passcod
passcod added this pull request to the merge queue Aug 1, 2026
Merged via the queue into main with commit 3c26b4b Aug 1, 2026
7 checks passed
@passcod
passcod deleted the claude/pr-370-fix-m17-rule-preview-range branch August 1, 2026 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants