code-review

# Skill: Quality Review & TMDL Validation ## Purpose Perform a comprehensive cross-check of ALL generated artifacts before the user opens the PBIP project in Power BI Desktop. Catching errors here prevents time-consuming troubleshooting outside VS Code. ## Prerequisites — MANDATORY Before starting the review: 1. **Read** `.github/skills/physical-model-tmdl/references/tmdl-syntax-reference.md` for syntax validation rules. 2. **Read** `.github/references/naming-conventions.md` for naming compliance. 3. If (and ONLY if) RLS is implemented, **read** `.github/references/security-rls-best-practices.md` and validate the model follows least-privilege, explicit allow/deny behavior, and auditable security logic. 4. **Load** all generated TMDL files from `<ProjectName>/PBIP/<ProjectName>.SemanticModel/definition/`. 5. **Load** the CSV mock data files from `<ProjectName>/data/`. 6. **Verify** any uncertain TMDL syntax using `microsoft_docs_search` or `microsoft_docs_fetch` MCP tool. ## Input / Output | | | |---|---| | **Input** | All TMDL files in `<ProjectName>/PBIP/<ProjectName>.SemanticModel/definition/`, all CSV files in `<ProjectName>/data/` | | **Output** | `<ProjectName>/tests/quality_review.md` | ## Review Checklist ### 1. TMDL Syntax Validation - [ ] **Indentation**: ALL TMDL files use TAB characters (not spaces). Verify by checking for `\t`. - [ ] **Root-level objects**: `table`, `relationship`, `expression`, `model`, `database` have NO indentation. - [ ] **Properties**: Indented ONE tab from their parent object. - [ ] **Multi-line expressions**: Indented TWO tabs from the parent object declaration. - [ ] **Blank lines**: Used to separate sibling objects (measures, columns) but NOT between a property and its parent. - [ ] **Colons** (`:`) used for non-expression properties only. - [ ] **Equals** (`=`) used for expressions/default properties only. - [ ] **Object names**: Enclosed in single quotes if containing spaces, dots, equals, colons, or single quotes. - [ ] **No trailing whitespace** on any line. - [ ] **No mixed indentation** (tabs and spaces mixed on same line). ### 2. Structural Integrity - [ ] `model.tmdl` contains `ref table` entries for ALL tables defined in `tables/` folder. - [ ] **CRITICAL**: `model.tmdl` contains `defaultPowerBIDataSourceVersion: powerBI_V3` property. Without it, Power BI Desktop throws *"A data model with version 3 of metadata is required"* and all refresh operations fail. - [ ] `database.tmdl` exists with correct `compatibilityLevel` matching Power BI Desktop version (1600 for December 2025, 1567 for September 2024). - [ ] `relationships.tmdl` contains entries for ALL FK-PK relationships. - [ ] One `.tmdl` file per table exists in `tables/` folder. - [ ] `_Measures.tmdl` table file exists with at least one measure. - [ ] `expressions.tmdl` exists if shared M parameters are used. ### 3. Relationship Validation For EACH relationship in `relationships.tmdl`: - [ ] `fromColumn` references an existing column in an existing Fact table. - [ ] `toColumn` references an existing column in an existing Dimension table (must have `isKey`). - [ ] Relationship is **single-directional** (unless explicitly bi-directional for RLS). - [ ] **CRITICAL**: `securityFilteringBehavior: oneDirection` for ALL relationships (unless specific RLS requirement). Max 1 `bothDirections` per table. - [ ] `crossFilteringBehavior: oneDirection` for standard Star Schema (Dim → Fact). - [ ] No **duplicate relationships** between the same column pairs. - [ ] No **circular dependency** chains (A→B→C→A). - [ ] FK columns in Fact tables are marked `isHidden`. - [ ] **CRITICAL: No ambiguous paths** — For EACH pair of tables, verify there is ONLY ONE active relationship path. Common ambiguities: - [ ] Fact_Sales → Dim_Country (direct) AND Fact_Sales → Dim_Customer → Dim_Country (indirect) - [ ] Fact_Sales → Dim_Area (direct) AND Fact_Sales → Dim_Customer → Dim_Country → Dim_Area (indirect) - [ ] Fact_Sales → Dim_Industry (direct) AND Fact_Sales → Dim_Customer → Dim_Industry (indirect) - **Fix**: Remove redundant direct relationships from fact tables. Keep only the path through the most granular dimension. ### 3.1 Field Parameter Semantic Review - [ ] Dimension parameter tables are disconnected and contain only user-facing fields that are safe to swap within the same visual role. - [ ] Measure parameter tables are disconnected and contain only measures that are semantically comparable within the same visual. - [ ] For each measure parameter, the dimensions used in the consuming visual are reachable through active relationships for **all** selectable measures. - [ ] If a measure parameter mixes measures from different fact tables, the visual context is intentionally restricted to shared conformed dimensions or explicitly documented reconciliation logic. - [ ] Reject any measure-switch design where a selected dimension produces ambiguous, partial, or misleading filtering for one or more selectable measures. ### 4. Data Type Consistency For EACH column across all tables: - [ ] `dataType` matches expected type (int64 for keys, decimal for amounts, string for text, dateTime for dates). - [ ] `sourceColumn` matches the actual column name in the CSV file header. - [ ] `summarizeBy` is set to `none` for keys and non-additive columns. - [ ] `formatString` is appropriate (currency formats for amounts, percentage for ratios, date format for dates). ### 5. DAX Measures Validation For EACH measure in `_Measures.tmdl`: - [ ] Uses **VAR / RETURN** pattern for non-trivial measures. - [ ] **VAR names do NOT use DAX reserved keywords** (`Variance`, `Status`, `Value`, `Date`, `Time`, `Year`, `Month`, `Day`, `Table`, `Column`, `Measure`, `Name`, `Count`, `Sum`, `Average`, `Min`, `Max`, `Result`, `Error`, `Number`, `Text`, `True`, `False`, `Blank`, `If`, `And`, `Or`, `Not`, `In`, `Order`, `Filter`). Use descriptive prefixes instead (e.g., `SalesBudgetVariance`, `BudgetStatusValue`). - [ ] Uses **DIVIDE()** for ALL division operations (no `/` operator). - [ ] ALL referenced columns exist in the model with correct `Table[Column]` syntax. - [ ] ALL referenced measures exist and are not self-referential or circular. - [ ] Time intelligence functions reference `Dim_Date[Date]` (the Date Table column). - [ ] `displayFolder` is assigned to each measure. - [ ] `lineageTag` is present and is a valid GUID. - [ ] `formatString` is appropriate for the measure type. ### 6. Dim_Date Validation - [ ] `Dim_Date` table contains a hidden integer surrogate key column for joins. - [ ] A `Date` column with `dataType: dateTime` exists and is the key column after Date Table marking is persisted in Desktop. - [ ] Fiscal year/month/quarter columns are present if fiscal periods are referenced in specs. - [ ] The generated date range covers the full reporting period. ### 7. CSV Mock Data Validation - [ ] ALL CSV files exist in `<ProjectName>/data/` folder. - [ ] Files are comma-delimited and UTF-8 encoded. - [ ] Column names in CSV headers EXACTLY match `sourceColumn` values in TMDL. - [ ] ALL FK values in Fact table CSVs exist as PK values in corresponding Dimension CSVs (referential integrity). - [ ] No NULL values in PK or FK columns. - [ ] Numeric fields contain valid decimal numbers (no text in numeric columns). - [ ] Date fields use a consistent, parseable format. ### 8. Naming Convention Compliance - [ ] Fact table names: `Fact_<BusinessProcess>` PascalCase. - [ ] Dimension table names: `Dim_<Entity>` PascalCase. - [ ] Measures table: `_Measures`. - [ ] Key columns: `<Entity>Key` PascalCase. - [ ] Attribute columns: PascalCase with descriptive names. - [ ] Measure names: Descriptive, following patterns in naming-conventions.md. - [ ] CSV file names: lowercase with underscores (e.g., `fact_sales.csv`, `dim_date.csv`). - [ ] TMDL file names: PascalCase matching table name (e.g., `Fact_Sales.tmdl`, `Dim_Date.tmdl`). ### 9. Partition Expression Validation For EACH table's partition: - [ ] M expression uses valid Power Query syntax. - [ ] `File.Contents()` path points to correct CSV file location. - [ ] `Columns` count in `Csv.Document` matches actual CSV column count. - [ ] `Encoding` is `65001` (UTF-8). - [ ] `Table.PromoteHeaders` is used to read CSV headers. - [ ] `Table.TransformColumnTypes` maps types correctly (Int64.Type for integers, type number for decimals, type text for strings, type datetime for dates). ### 10. RLS Validation (if applicable) - [ ] RLS role files exist in `roles/` subfolder. - [ ] `tablePermission` filter expressions reference ONLY Dimension tables (never Fact tables directly). - [ ] Filter expressions use valid DAX syntax. - [ ] RLS logic does NOT have overly permissive fallbacks (e.g., `TRUE()` for unknown users). Prefer explicit deny-by-default behavior. - [ ] Bi-directional cross-filtering is enabled on relationships filtered by RLS roles. ### 11. BPA Rules Validation (Detective Quality Assurance) **Purpose**: Validate ALL generated TMDL and DAX code against Tabular Editor Best Practice Analyzer rules. This is the final quality gate before user opens PBIP in Power BI Desktop. **Reference**: `.github/skills/code-review/references/bpa-rules-reference.md` contains all 27+ BPA rules with examples. #### 11.1 DAX Expression Rules (Error Severity) **Critical Rules - ZERO TOLERANCE**: - [ ] **DAX_FULLY_QUALIFIED_COLUMNS**: ALL column references in DAX use `Table[Column]` syntax (no unqualified `[Column]`) - **Validation**: Grep search all measures for `\[[A-Za-z]+\]` pattern without table prefix - **Fix**: Replace `[Column]` with `Table[Column]` - [ ] **DAX_DIVISION_COLUMNS**: ALL division operations use `DIVIDE()` function, NO `/` operator - **Validation**: Grep search all measures for `/` operator in expressions - **Fix**: Replace `[Numerator] / [Denominator]` with `DIVIDE([Numerator], [Denominator], 0)` - [ ] **AVOID_RESERVED_KEYWORDS**: Object names do NOT use DAX/SQL reserved keywords (Date, Table, Value, Column, Year, etc.) - **Validation**: Check table/column names against reserved keyword list - **Fix**: Add prefix/suffix (e.g., `Date` → `Dim_Date`, `Value` → `DateValue`) - [ ] **DAX_VAR_RESERVED_KEYWORDS**: VAR names inside DAX expressions do NOT use reserved keywords (`Variance`, `Status`, `Value`, `Date`, `Time`, `Year`, `Month`, `Day`, `Table`, `Column`, etc.) - **Validation**: For each `VAR <name>` in measure expressions, check `<name>` against forbidden keyword list - **Fix**: Use descriptive prefixed names (e.g., `VAR Variance` → `VAR SalesBudgetVariance`, `VAR Status` → `VAR BudgetStatusValue`) #### 11.2 DAX Expression Rules (Warning Severity) - [ ] **DAX_UNQUALIFIED_MEASURES**: Measure references are unqualified (use `[Measure]` not `Table[Measure]`) - **Validation**: Grep search for `\[_Measures\]\.` or `Table\.` before measure names - **Fix**: Remove table prefix from measure references - [ ] **DAX_TODO_COMMENTS**: No TODO/FIXME/HACK comments in measures - **Validation**: Grep search for `TODO|FIXME|HACK|TEMPORARY` in measure expressions - **Fix**: Complete implementation or remove comment if measure is production-ready #### 11.3 Formatting Rules (Warning Severity) - [ ] **OBJECTS_WITH_NO_FORMAT_STRING_COLUMNS**: Numeric columns have `formatString` property - **Validation**: Check all columns with `dataType: decimal|double|int64` (except hidden FKs) - **Fix**: Add `formatString: "$#,##0.00"` for currency, `"0.00%"` for percentage, `"#,##0"` for integers - [ ] **OBJECTS_WITH_NO_FORMAT_STRING_MEASURES**: ALL measures have `formatString` property - **Validation**: Check all measures in `_Measures.tmdl` - **Fix**: Add appropriate `formatString` based on measure type #### 11.4 Metadata Rules (Error/Warning Severity) - [ ] **AVOID_FLOAT_DATATYPE** (Error): Numeric columns use `dataType: decimal` NOT `double` - **Validation**: Grep search for `dataType: double` in all table TMDL files - **Fix**: Replace `double` with `decimal` - [ ] **SUMMARIZEBY_SHOULD_BE_NONE** (Warning): ALL columns have `summarizeBy: none` - **Validation**: Check all columns (dimension attributes and fact measures) - **Fix**: Add or change to `summarizeBy: none` - [ ] **DISABLE_ATTRIBUTE_HIERARCHIES** (Info): Foreign key columns have `isAvailableInMDX: false` - **Validation**: Check all FK columns in fact tables - **Fix**: Add `isAvailableInMDX: false` #### 11.5 Model Layout Rules (Warning Severity) - [ ] **HIDE_FOREIGN_KEY_COLUMNS** (Warning): ALL FK columns in fact tables have `isHidden: true` - **Validation**: Check all columns with `Key` suffix in fact tables - **Fix**: Add `isHidden: true` - [ ] **ORGANIZE_MEASURES_IN_DISPLAY_FOLDERS** (Info): Measures have `displayFolder` property - **Validation**: Check all measures in `_Measures.tmdl` - **Fix**: Add `displayFolder: "Category\\Subcategory"` for logical grouping - [ ] **PROVIDE_DESCRIPTIONS_FOR_MEASURES** (Info): Complex measures (time intelligence, statistical) have `description` property - **Validation**: Check measures with CALCULATE, DATESYTD, SAMEPERIODLASTYEAR, etc. - **Fix**: Add `description: "Business logic explanation"` - [ ] **ORGANIZE_COLUMNS_IN_DISPLAY_FOLDERS** (Info): Dimension tables with 15+ columns use `displayFolder` for attribute grouping - **Validation**: Count columns per dimension table - **Fix**: Add `displayFolder: "Geography"` for location attributes, `"Demographics"` for personal attributes #### 11.6 Naming Convention Rules