Implementation review: PB-112, PR 753

Field	Value
Backlog item	PB-112 Semantic model best-practice checks in governance policies (features FEAT-GOV-09, FEAT-META-13). It is a documented limitation of PB-108 (finding 1 of `PB-108-643.md`).
Pull request	#753, merged as e9f3cadf (head 5c86ed7c; quality PASS 9.1, `docs/quality/pr-review.md` rows #753). #560 (the Best Practice Analyzer that runs in Fabric) is listed on the item but is still an open draft and was not reviewed here.
Package	none
Reviewer	Engineering quality review thread. It gave #753 its quality verdict but didn't write it.
Date	2026-10-27
Environment	Linux container, main 1b3c37bf, Python 3.13 venv for the engine, Node with Playwright Chromium. No tenant, no Windows.

Outcome#

Recommended: Accepted with Documented Limitations. The product owner decides.

The criterion assumed the semantic model rules would run in Fabric and use capacity. #753 does it differently. It registers SM001 to SM007, the BPA rules that need only a model's definition, as a local model checker. They check the model Update model would write for the project's last layer, worked out on this PC from the metadata project. So nothing is read from Fabric, no job runs and no capacity is used.

A policy can name the rules, and Check now runs them. Check now never says they run in Fabric, because they don't.

What #753 doesn't cover:

The rules that need VertiPaq statistics, such as sizes and cardinality. Those need the deployed model in Fabric, and that is #560, still a draft.
A model that wasn't built by Update model, or one changed in Fabric after deployment.

The limitations table below lists both.

If the product owner accepts:

the criterion should be reworded as "Check now says where each rule runs and whether it uses capacity";
the Fabric half needs its own backlog item. PB-112 can't stay open for it once accepted.

Recommended by: Engineering quality review. Decided by (product owner): … on ….

Acceptance criteria#

#	Criterion (copied from the backlog item)	Result	Evidence
1	A policy can name semantic model rules; Check now says they run in Fabric and use capacity before running them	Met for naming and running. Not met as worded, because they run on this PC instead.	Naming. `best_practice_checkers.py:72` registers kind `model` with `model_practices.rules` (SM001 to SM007). `test_model_practices.py::test_the_rules_are_registered_as_the_model_kind` asserts the seven ids, `runs_in == "local"`, and that a checker running in Fabric can't take the `model` kind over (`register` raises "model rules already run in local"). Running. `central/checks_service.py` `_gc_model_items` builds the model item when a policy names a `model` rule. `test_check_now_runs_model_rules_on_the_last_layer` names SM003, SM005 and SM006 in a policy, then checks three things: Check now reports findings `SM003 SM_Gold: fact_sales` and `SM005 SM_Gold: dim_customer`; SM006 passes; a description saved in the catalog clears its SM005 finding. Where it runs. `model_practices.py` docstring: "Nothing is read from Fabric and no Fabric job runs". `docs/semantic-model-rules.md:7` says "Nothing is read from Fabric, no Fabric job runs, and it uses no capacity". Rules that need VertiPaq statistics aren't registered (`docs/governance.md:185`), so a policy can't name a rule that would need Fabric.

Checks#

Check	Result	Evidence or note
Scope matches the item and package	Partly	The definition-only rules, policy naming and Check now are present. The Fabric BPA half (#560) is not merged (limitation 1).
New tests fail without the change, pass with it	Met	`test_model_practices.py` is new in #753, and its module `model_practices.py` doesn't exist without the change. It was checked at review time (`pr-review.md` row #753 6a5b142c).
Full suites pass (pytest, UI, cargo)	Met	`pytest sidecar/tests` on main 1b3c37bf: 4847 passed, 76 skipped, 0 failed in 21.5 min. `node --test app/tests/ui.test.cjs`: 295/0. cargo: not run (PB-071 not landed).
Reachable where docs say; Preview behavior right; packaged into the engine	Met	Platform > Governance > Best practices (behind `preview("bestPractices")`) lists templates. A policy's Checked by takes `SM003, SM005`. Check now shows the findings. A finding's fix opens the data dictionary or the entity (`governance-checks.js`, `open_model` → `goMeta("model")`). Browser `best-practice-checks.test.cjs`: 8/0. `model_practices.py` is in the engine package with the other checkers.
OneLake data safety	Met	It reads the project files and the catalog's descriptions only. Check now writes only `governance_checks.json` on this PC. Nothing is written to OneLake or Fabric.
Authentication and distribution	Met	No new scope, no network call.
No unproven local-equals-Fabric claim; Preview tags kept	Met, with one point to note	The rules check the model the project would write, and the docs say so (`semantic-model-rules.md:3-7`). They never claim to check the deployed model. The page is marked Preview.
Docs match the code; linked; strict docs build passes	Met	`semantic-model-rules.md` matches `RULES` (ids, severities, categories, fixes). It is linked from README.md, docs/README.md and zensical.toml. `governance.md:185, 215, 217` describe the `model` checker. `zensical build --strict` on main: clean (docs audit 4, #796).
Accessibility of new controls	Met	The only new control is the fix link, a standard `btn` with its label text.
Inventory and backlog updated; `product_data.py all` consistent	Met	#797 moves PB-112 to In review with #753 in `prs`, and FEAT-GOV-09 names it. `product_data.py check` on #797's head: "Product data is consistent."

Findings#

#	Severity	Finding	Where	Status
1	should fix (for Accepted)	The criterion's Fabric half is not delivered: BPA rules that need VertiPaq statistics, run on the deployed model in Fabric with a capacity warning. #560 is an open draft.	`governance_checks.py` (no `runs_in="fabric"` model checker); PR #560	Documented limitation 1; needs a new backlog item if PB-112 is accepted
2	note	The rules check the model Update model would write, not what is deployed, so a model built or changed outside KratosForge isn't checked.	`model_practices.model_item`	Documented limitation 2
3	note	`model_practices` uses `model_dictionary._delta_type`, a private helper, across modules (nit from the #753 quality review).	`model_practices.py`	Open nit
4	note	When #560's Fabric checker lands it needs its own kind: `register` refuses to replace the local `model` checker, so the rules can't silently disappear.	`governance_checks.register`; `test_the_rules_are_registered_as_the_model_kind`	Guarded by a test

Documented limitations (only for Accepted with Documented Limitations)#

Limitation	Where users are told	Backlog item that tracks it
BPA rules that need VertiPaq statistics (Fabric, uses capacity) aren't checked	`docs/semantic-model-rules.md` ("Rules that need the model's VertiPaq statistics ... aren't checked here"); `docs/governance.md:185`	New item to open (owner: Best Practice Analyzer, #560)
Only the model Update model writes is checked, not the deployed model	`docs/semantic-model-rules.md:3-7` ("the one Update model writes ... Nothing is read from Fabric")	The same new item (#560 reads the deployed model)

Inventory and backlog changes#

On acceptance: PB-112 In review → Accepted (with documented limitations); criterion reworded as above; FEAT-GOV-09 verification notes SM001-SM007 with this review; a new backlog item for the Fabric BPA checker (#560) takes limitation 1 and 2.
Until the product owner decides: unchanged (PB-112 In review, from #797).