Skip to content

Stabilize generated unit conversion output and fix agentic hook#9443

Merged
hl662 merged 12 commits into
masterfrom
nam/deterministic-basic-unit-conversions
Jun 29, 2026
Merged

Stabilize generated unit conversion output and fix agentic hook#9443
hl662 merged 12 commits into
masterfrom
nam/deterministic-basic-unit-conversions

Conversation

@hl662

@hl662 hl662 commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

TL;DR

Generated basic conversion artifacts were drifting across Node and V8 runtimes in the last printed digits, which created noisy generated diffs even when the underlying conversions were effectively the same. This PR rounds generated built-in conversion constants to stable 15-significant-digit text, regenerates the checked-in table, and tightens the parity checks so meaningful conversion drift still fails.

The build also now checks that generated unit artifacts stay committed after generation, so CI catches stale checked-in outputs instead of letting rush build rewrite them silently. The broader hook-script test harness and any generalized generated-file guardrail cleanup are intentionally left for a follow-up PR.

What

Asset Why it exists
core/quantity/scripts/generatedModuleBuilders.ts Raw floating point recomputation was producing runtime-specific trailing digits, so the generator now canonicalizes emitted numbers and keeps the conversion-entry shape explicit in the type system.
core/quantity/src/internal/BasicUnitConversions.generated.ts The checked-in conversion table needs to reflect the canonicalized output so rebuilds stay stable across runtimes.
core/quantity/src/test/GenerateUnitsArtifacts.test.ts Representative literals need to prove the visually suspicious round-number changes are intentional and that canonicalization does not emit invalid overflow values.
core/quantity/src/test/Quantity.test.ts Provider parity coverage needs a named drift budget because canonicalized generated values are expected to differ slightly from live recomputation while still staying well within acceptable precision.
.github/workflows/ci.yaml rush build can rewrite generated unit artifacts before tests observe them, so CI now fails if those generated files are left dirty after the build.
.github/hooks/scripts/itwinjs-core-pre-tool-use.mjs Pi's commit gate was asking Rush to verify against HEAD before the new change file existed in a commit, so the hook now falls back to checking the staged tree for change-file coverage before blocking the commit.

Tests

New and updated test coverage
Test What it covers
GenerateUnitsArtifacts.test.tscanonicalizes generated conversion values that drift across Node runtimes Guards representative constants that previously flipped in the last printed digits across runtimes, including the round-number values that looked most suspicious in review.
GenerateUnitsArtifacts.test.tspreserves finite values when canonicalization would overflow on reparse Ensures finite generator inputs do not turn into Infinity when rounded text is parsed back for emitted output.
GenerateUnitsArtifacts.test.tsrebuilds the checked-in basic conversion artifact exactly from Units.json Keeps the generated conversion module byte-stable against the source schema and generator logic during local test runs.
Quantity.test.tsgenerated basic conversion data matches provider-backed conversions for every bundled same-phenomenon unit pair Verifies the canonicalized built-in table still tracks provider recomputation within the declared drift budget across all bundled same-phenomenon pairs.
CI — generated unit artifact drift check Fails the workflow if rush build rewrites generated unit files that were not committed.
commit gate staged-tree smoke test Verifies the hook now allows the commit when the staged tree includes both a changed published package and a matching staged Rush change file.

Nambot 🤖 (powered by GPT-5.5)

@hl662 hl662 changed the title Stabilize generated unit conversion output and fix commit gate fallback Stabilize generated unit conversion output and fix agentic hook Jun 24, 2026
@hl662 hl662 requested a review from Copilot June 24, 2026 21:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR makes unit-conversion generator output deterministic across Node/V8 runtimes by canonicalizing emitted floating-point literals, regenerates the checked-in conversion table accordingly, and adjusts tests to validate both determinism and acceptable drift versus provider recomputation. It also updates the pre-tool-use commit gate hook to fall back to staged-tree change-file coverage when rush change --verify fails pre-commit.

Changes:

  • Canonicalize generated conversion constants (15 significant digits) and regenerate BasicUnitConversions.generated.ts to eliminate cross-runtime trailing-digit drift.
  • Tighten/extend tests to ensure deterministic artifact rebuilds and allow a defined drift budget vs provider-backed recomputation.
  • Add a staged-tree fallback path in the commit gate hook when rush change --verify fails before commit.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
core/quantity/scripts/generatedModuleBuilders.ts Adds typed tuples for conversion entries and canonical number formatting for deterministic generation.
core/quantity/src/internal/BasicUnitConversions.generated.ts Regenerates the conversion table with canonicalized numeric literals.
core/quantity/src/test/GenerateUnitsArtifacts.test.ts Adds spot-checks for canonicalized emitted literals and continues enforcing byte-stable regeneration.
core/quantity/src/test/Quantity.test.ts Introduces a named drift tolerance when comparing generated conversions vs provider recomputation.
common/changes/@itwin/core-quantity/nam-deterministic-basic-unit-conversions_2026-06-24-21-07.json Adds a Rush change entry documenting the determinism work.
.github/hooks/scripts/itwinjs-core-pre-tool-use.mjs Adds staged-tree fallback logic for change-file coverage when pre-commit verification fails.

Comment thread .github/hooks/scripts/itwinjs-core-pre-tool-use.mjs Outdated
@hl662 hl662 marked this pull request as ready for review June 25, 2026 19:05
@hl662 hl662 requested review from a team, RohitPtnkr1996 and rschili as code owners June 25, 2026 19:05

@aruniverse aruniverse 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.

Adversarial review of PR #9443 across three angles: numeric precision, hook correctness, and type/test quality. All value changes in BasicUnitConversions.generated.ts were verified correct — NG, NMOL_PER_CUB_DM, IN_PER_FT, FAHRENHEIT, and others are fixing genuine sub-ULP floating-point artifacts rather than changing physical values. The type generics improvement (GeneratedEntry<TValue>, BasicConversionValue) provides real compile-time safety and is correctly applied. No blockers.

Fix that can't be inlined (not in diff context):

core/quantity/scripts/generatedModuleBuilders.ts:305buildGeneratedDefaultPersistenceModule still has a leftover as string cast that the type-tightening refactor missed:

const unitName = stripSchemaPrefix(entry.value as string, source.name);
//                                              ^^^^^^^^^ unnecessary

entries is Array<GeneratedEntry<string>> here, so entry.value is already typed string. The PR's own goal was to eliminate these casts — this one slipped through. Safe one-character fix: remove as string.

Deferred / pre-existing (not introduced by this PR):

  • isGitCommitCommand regex false-positive: git commit-graph write matches \bgit\s+commit\b due to the word boundary between t and -. Pre-existing in an earlier commit, out of scope here.
  • Hook scripts have no automated tests. The PR description mentions a 'staged-tree smoke test' that does not exist in the repo. Recommend filing a follow-up to add Jest/Vitest coverage for collectPackageNamesFromChangeFile, collectChangedProjects, and the fallback path.

Comment thread core/quantity/scripts/generatedModuleBuilders.ts Outdated
Comment thread core/quantity/scripts/generatedModuleBuilders.ts
Comment thread .github/hooks/scripts/itwinjs-core-pre-tool-use.mjs Outdated
Comment thread core/quantity/src/test/GenerateUnitsArtifacts.test.ts
Comment thread .github/hooks/scripts/itwinjs-core-pre-tool-use.mjs Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@aruniverse aruniverse 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.

entry.value as string remains at line 309 — likely missed because it was in the review body text rather than an inline comment

hl662 and others added 2 commits June 26, 2026 15:21
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@hl662 hl662 enabled auto-merge (squash) June 29, 2026 14:56
@hl662 hl662 added this to the iTwin.js 5.11 milestone Jun 29, 2026

@rschili rschili left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving so you can get the unit conversion stabilization in for 5.11.

I'm not a fan of the unrelated agentic hook that sneaks in with this PR. But since it already exists and is just being relaxed, I won't be a bother. Do we really need this though?

@hl662 hl662 merged commit 618ec6a into master Jun 29, 2026
21 checks passed
@hl662 hl662 deleted the nam/deterministic-basic-unit-conversions branch June 29, 2026 16:38
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.

4 participants