Implementation review: PB-108, PRs 643, 648, 649, 651, 664, 671

Field	Value
Backlog item	PB-108 Best-practice checks and governance templates (features FEAT-GOV-05, FEAT-GOV-09). FEAT-GOV-09 was added to the inventory by the 2026-10-25 daily product check, after this review began.
Pull requests	All merged: #648 (templates, Check now, product checklist), #651 (RE2 naming checks), #643 (notebook rules NB001-NB012), #649 (SQL rules SQ001-SQ010), #664 (NB and SQ rules in Checked by), #671 (a turned-off policy shows not checked). #560 (semantic model BPA engine) 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 each PR its quality verdict but wrote none of them.
Date	2026-10-25
Environment	Linux container, main 5ae85e52, Python 3.13 venv for the engine, Node with Playwright Chromium. No tenant, no Windows.

Outcome#

Accepted with Documented Limitations.

Criterion 1 is met. Criterion 2 is met for notebooks only. Check now has no SQL scripts to give the SQL rules, and no semantic model checker is registered, because #560 isn't merged. The product owner agreed these can ship later, each with its own backlog item (PB-111, PB-112), and each is documented for users once #713 is merged (finding 3).

Recommended by: Engineering quality review (first Changes Required, with this outcome offered). Decided by (product owner): Chris, "Go on all" on 2026-10-26 (project message cmsg_018T4o2pQdpbLBNhHpgVNNGpVYtS9wxoAC4YeAMMvSdTtB), which took this outcome.

Acceptance criteria#

#	Criterion (copied from the backlog item)	Result	Evidence
1	Each rule set registers through governance_checks.register and is off unless a policy names it	Met	`best_practice_checkers.py:63-64` registers `notebook` and `sql`. `governance_checks.py:430-433` registers naming, ownership, classification and contract. A probe against main 5ae85e52 shows registered checkers `classification, contract, naming, notebook, ownership, sql`, all `local`, with 32 rules (12 NB, 10 SQ). A checker given no settings returns `None` for both notebook and SQL items. Naming only NB003 reports only NB003. `enabled: false` returns `None`. A SQL item passed to the notebook checker returns `None`. `test_best_practice_checkers.py::test_a_policy_reports_only_the_rules_it_names` covers this.
2	Check now runs offline for notebooks and SQL; model checks say they run in Fabric and use capacity	Not met (partly)	Notebooks: met. `checks_service.py` `_gc_notebook_items` reads this PC's notebooks only, the checkers are pure, and `test_check_now_runs_notebook_rules_and_says_when_nothing_is_there_to_check` covers it. SQL: not met. `governance_check_now` builds entity, product and notebook items but never SQL items, so a policy naming SQ rules shows "Check now found nothing for its sql rules to check". `docs/governance.md:217` says this honestly. Model checks: not met. No checker registers with `runs_in="fabric"`. Nothing calls `store_fabric_findings`. `model_bpa` isn't on main, because #560 is an open draft.

Checks#

Check	Result	Evidence or note
Scope matches the item and package	Partly	The engine, templates, Check now, the checklist and the NB/SQ registrations are in scope and present. The model BPA half (#560) is not merged.
New tests fail without the change, pass with it	Met	This was checked per PR at review time (see `docs/quality/pr-review.md`, rows #643, #648, #649, #651, #664, #671).
Full suites pass (pytest, UI, cargo)	Met (2 environment failures)	`pytest sidecar/tests` on main 5ae85e52: 4349 passed, 76 skipped, 2 failed in 25 min. Both failures are in `test_warehouse_driver.py` with `ModuleNotFoundError: No module named 'mssql_python'`: the review venv lacks the warehouse driver, which has nothing to do with PB-108. `node --test app/tests/ui.test.cjs`: 285/0. cargo: not run (PB-071 not landed).
Reachable where docs say; Preview behavior right; packaged into the engine	Met for templates and Check now; NB/SQ not reachable on their own	Governance > Best practices is behind `preview("bestPractices")` (`governance-checks.js:19`). Command bar entries are marked `preview: true`. Browser test `best-practice-checks.test.cjs` passed 8/0, run twice. The standalone `notebook_practices` and `sql_practices` RPCs have no UI caller (`test_rpc_surface.py:40-54`: "engine only, the panel waits on Chris's mockup review"), and both docs pages say so.
OneLake data safety	Met	Check now writes only `governance_checks.json` in the app folder (`checks_service.py` docstring, `write_json(self._gc_path(), state)`). It reads the catalog and run logs. The checkers write nothing.
Authentication and distribution	Met	No new scope. Platform calls go through `call_or_repair`, and an older UDF_Platform gets "Finish or repair once".
No unproven local-equals-Fabric claim; Preview tags kept	Met	Everything is tagged Preview. `docs/governance.md:229` says "Not yet tried against SQL_Platform in a live Fabric tenant".
Docs match the code; linked; strict docs build passes	One mismatch	`docs/governance.md:185` says the BPA semantic model checker "plugs in the same way", but none is registered on main. NB and SQL pages are linked from README.md, docs/README.md and zensical.toml. `zensical build` on main 5ae85e52: "No issues found".
Accessibility of new controls	Met	The view chips have `aria-pressed` in a labelled group. The template and policy inputs and the authors select have `aria-label`. Status marks carry `aria-label` and `title` (`governance-checks.js:13-90`).
Inventory and backlog updated; `product_data.py all` consistent	Met (after the outcome)	At review time PB-108's `prs` listed #560 (not merged) but not #664 or #671. FEAT-GOV-09, added by the daily product check, now carries the Best practices surface, so FEAT-GOV-05's reach is right for what it covers. Updated with the outcome (below); `product_data.py all`: "Product data is consistent."

Findings#

#	Severity	Finding	Where	Status
1	blocking (for Accepted)	No semantic model checker is registered, so policies can't name model BPA rules and Check now can't show "runs in Fabric, uses capacity" for them. #560 is an open draft.	`governance_checks.py` (no `runs_in="fabric"` registration); PR #560	Documented limitation: PB-112
2	should fix	Check now never collects SQL items, so SQ rules in a policy always show not checked. This is documented, but it doesn't meet criterion 2.	`central/checks_service.py` `governance_check_now`	Documented limitation: PB-111
3	should fix	The docs say the BPA semantic model checker plugs into the registry, but it isn't there.	`docs/governance.md:185`	Fixed by #713 (quality PASS 9.3)
4	should fix	The inventory and backlog are stale. FEAT-GOV-05 reach says "None". PB-108 `prs` lacks #664 and #671 and lists the unmerged #560. The coordinator's FEAT-GOV-09 doesn't exist.	`docs/product/FEATURE_INVENTORY.json`, `PRODUCT_BACKLOG.json`	Fixed with the outcome (below)
5	note	Notebook items are always `scheduled: False`, and `NB004`/`NB008` are scheduled-only, so no path runs those two rules today. This is documented.	`checks_service.py` `_gc_notebook_items`; `governance_checks.py:133`	Documented limitation: PB-111

Documented limitations (only for Accepted with Documented Limitations)#

Limitation	Where users are told	Backlog item that tracks it
Check now doesn't check SQL scripts yet	`docs/governance.md:217`	PB-111
Semantic model best practices aren't in Check now	`docs/governance.md:185` (#713)	PB-112 (#560)
NB004 and NB008 run only in scheduled runs, which Check now doesn't check yet	`docs/governance.md:217`	PB-111

Inventory and backlog changes#

Made with the outcome, in the same docs PR:

PB-108: state Accepted; prs 643, 648, 649, 651, 664, 671, 713 (#560 moves to PB-112); features FEAT-GOV-05, FEAT-GOV-09; the satisfied external dependencies removed (the BPA cell error moves to PB-112); this record added to evidence.
PB-111 (new, Proposed): Check now covers SQL scripts and scheduled notebooks (findings 2 and 5).
PB-112 (new, Proposed): semantic model best-practice checks in governance policies, through #560 (finding 1).
FEAT-GOV-09: the three limitations above, with their backlog items; next_action points at PB-111 and PB-112; this record added to evidence.
product_data.py all: "Product data is consistent."