Open
Conversation
…n web report Simplify the logic for marking rules as Skipped Clean up error repsonses Add show violation support to the web report Added more violation support to generators Signed-off-by: Mike Fuller <mike@finops.org>
Signed-off-by: Mike Fuller <mike@finops.org>
Signed-off-by: Mike Fuller <mike@finops.org>
Normalise responses to errors with NULL, NaN etc Force error messages to take correct hierachy Introduce more error messages and validation samples Better data loading from CSV Signed-off-by: Mike Fuller <mike@finops.org>
Signed-off-by: Mike Fuller <mike@finops.org>
Signed-off-by: Mike Fuller <mike@finops.org>
Signed-off-by: Mike Fuller <mike@finops.org>
Signed-off-by: Mike Fuller <mike@finops.org>
Signed-off-by: Mike Fuller <mike@finops.org>
Matt-Cowsert
requested changes
Jan 27, 2026
Collaborator
Matt-Cowsert
left a comment
There was a problem hiding this comment.
Hey Mike, I think it's worth considering adding these recommended changes. For transparency, I ran the PR through Claude Code to help surface any inconsistencies, but they made sense to me to consider.
-
Type mismatch (
focus_to_duckdb_converter.py:198) -_dependenciesis typed asSet[str]but used asList[DependencyRef] -
XSS vulnerability (
outputter_web.py:1702-1703, 1790-1791) - Sample violation values should be HTML-escaped before rendering
Signed-off-by: Mike Fuller <mike@finops.org>
Correctly escape output to prevent any issues with unsafe values in data being processed by web output Fix up filtering issue in rule view. Signed-off-by: Mike Fuller <mike@finops.org>
Signed-off-by: Mike Fuller <mike@finops.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Material Changes (Excluding Formatting & Tests)
New Result Override Architecture (focus_to_duckdb_converter.py)
Fixes Entities skipped due to Applicability are not being marked as skipped #128
Added apply_result_overrides() method - centralized post-processing system that handles:
Non-applicable rules: Rules that don't meet applicability criteria are now properly skipped
Composite aggregation: Composite rule results are updated based on actual child results
Dependency skips: Rules with failed/skipped dependencies are automatically skipped
New helper methods:
_apply_non_applicable_skips() - Marks non-applicable rules and all descendants
_apply_nested_child_non_applicable_marking() - Handles nested children after composite aggregation
_apply_composite_aggregation() - Updates composite results from child results
_apply_dependency_skips() - Skips rules with failed dependencies
_collect_all_descendants() - Recursively collects all rule descendants
New Check Class: SkippedOptionalCheck
Explicitly handles MAY/OPTIONAL rules that should be skipped
Clear error message: "Rule skipped - marked as MAY/OPTIONAL and not enforced"
Validation Keyword Support
Fixes: #127
Added _get_validation_keyword() method to extract MUST/SHOULD/MAY/RECOMMENDED keywords from rules
Applied throughout all check generators for accurate error messages
2. Improved Error Message Generation
Fixed Double-Negative Grammar Issues
CheckNotValueGenerator: Now detects when keyword already contains "NOT" (e.g., "MUST NOT")
Before: "Column MUST NOT NOT be NULL"
After: "Column MUST NOT be NULL"
CheckNotSameValueGenerator: Same fix for comparing columns
Before: "Columns MUST NOT NOT have same value"
After: "Columns MUST NOT have same value"
Enhanced Sample Violations
Added get_sample_sql() and sample_sql methods to multiple generators
Enables showing actual violating data rows in web reports
3. CSV Data Loading Improvements (csv_data_loader.py)
All-NULL Column Detection
New _peek_for_all_null_columns() method:
Peeks at first ~5000 rows to identify columns that are entirely NULL
Prevents false-positive type errors on NULL-only columns
Modified _try_load_with_types():
Only forces types for all-NULL columns
Lets Polars infer types for columns with data
Enables better type mismatch detection while avoiding false positives
Improved logging for forced type conversions
4. Web Report Enhancements (outputter_web.py)
Skipped Composite Children Handling
Identifies children of skipped composite rules (excluding Dataset entity types)
Automatically marks these children as skipped in the display
Preserves existing skip reasons (e.g., dynamic rules) when present
Default message: "Rule skipped - not applicable to current dataset or configuration"
Enhanced Entity Status Display
Better handling of skipped requirements in entity status calculation
Clear differentiation between non-applicable and dependency-skipped rules
Improved filtering logic for status-based display
Sample Violations Display
Added support for displaying sample violating rows
Shows actual data that failed validation
Formatted display with column=value pairs
5. Rule Model Updates (rule.py)
New Method: is_optional()
Checks if rule has OPTIONAL or MAY keyword
Used to determine if rule should be enforced
6. Integration (spec_rules.py)
Post-Processing Hook
Added call to converter.apply_result_overrides() after all checks run
Ensures all result overrides are applied in a single, maintainable location
Runs before finalization to ensure consistent results
7. Type Safety (focus_to_duckdb_converter.py)
Added Missing Attributes to DuckDBColumnCheck:
_dependencies: Optional[Set[str]] - Tracks rule dependencies
_child_rule_ids: Optional[List[str]] - Tracks composite children
_non_applicable: bool - Non-applicability flag
_non_applicable_reason: Optional[str] - Explanation for non-applicability