Repository navigation
feat(inspect): add Code Contract lint check and sync improvements - #24
Conversation
Add a lightweight static code analysis module that detects code defects in AI artifacts without requiring any LLM API calls. - QC-0 Baseline: syntax parsability, language declaration, structural completeness - QC-1 Syntax: undefined name references, type mismatches - QC-2 Resource: unclosed file handles, unreleased network connections, missing exception handling - QC-3 Pattern: unused imports, missing imports, hardcoded demo values Supports both Markdown code blocks and complete Python source files.
Run the Code Contract lint check before LLM-based analysis to catch syntax errors and code pattern issues early, without API calls. - Add --skip-qc-lint option to disable the check - Render QC results in the terminal before artifact analysis - Include QC section in the markdown report with summary, per-artifact defects, and rule reference - Save a standalone QC report when --skip-defect-check is set
When a module inspection fails (e.g. rate limiting) and returns no defects, the report previously showed a misleading 'No defects found'. Render a CAUTION callout with the error code and message instead. - Propagate the error field into the per-module loop - Aggregate CROSS sub-result statuses so a failed sub-result marks the merged section as failed - Fall back to 'No defects found' when a failed module has no error info
Unify the tool schema upload format between push (paths) and inspect
(findings): both now produce the provider-neutral flat structure
{name, description, parameters}. The OpenAI {type, function} wrapper is
stripped during packing because the CLI cannot know which provider the
project will use; the backend converts when a model endpoint is set.
The tool conversion emits both inputSchema and parameters keys, so the SDK can report the same defect twice under two locations, inflating the defect counts and rendering duplicates in output. - Add _normalize_defect_location to align $.inputSchema with $.parameters - Add _dedup_defects keyed on (id, normalized location); apply it in both the terminal renderer and the markdown report - Strip the redundant inputSchema key from tool payloads when the source schema carries both aliases
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Claude Code Review
PR #24 Code Review
Critical Issues
1. Python 3.11+ Compatibility Issue
File: src/sanityops_cli/code_contract_checker/rules/qc2_resource.py
Line: 295-301
The visit_TryStar method is only available in Python 3.11+. This will raise an AttributeError on older Python versions since ast.TryStar doesn't exist before 3.11.
def visit_TryStar(self, node: ast.TryStar) -> None: # Python 3.11+Fix: Add a version guard or use getattr with a fallback:
def visit_TryStar(self, node: ast.AST) -> None:
# Only available in Python 3.11+
for stmt in node.body:
...Or guard the method registration with if sys.version_info >= (3, 11):.
Important Issues
2. Unused Parameter (Dead Code)
File: src/sanityops_cli/code_contract_checker/checker.py
Line: 172-173
The artifact_result parameter is declared but never used in _read_artifact. Either it should be removed or used for error tracking.
def _read_artifact(self, path: str, artifact_result: QCArtifactResult) -> str | None:3. Dead Code - Unreachable Condition
File: src/sanityops_cli/code_contract_checker/rules/qc1_syntax.py
Lines: 182-186
The dunder name check is unreachable because the earlier if name.startswith("_") already returns for all names starting with underscore:
if name.startswith("_"):
return # This catches __dunder__ names too
if name.startswith("__") and name.endswith("__"): # DEAD CODE
returnFix: Remove the dead code or reorder the conditions.
4. Code Duplication
Files:
src/sanityops_cli/code_contract_checker/rules/qc0_baseline.py(lines 123-210)src/sanityops_cli/code_contract_checker/rules/qc3_pattern.py(lines 248-273)
PYTHON_STDLIB in qc0_baseline and stdlib_modules in qc3_pattern are nearly identical sets duplicated across files.
Fix: Import PYTHON_STDLIB from qc0_baseline in qc3_pattern, similar to how PYTHON_BUILTINS and PYTHON_KEYWORDS are already imported in qc1_syntax.py.
5. Location Prefix Not Propagated
File: src/sanityops_cli/code_contract_checker/checker.py
Lines: 397-402
_check_code_blocks ignores the caller's location context and always uses $.code_blocks[{idx}]:
def _check_code_blocks(
...
) -> None:
for idx, (code, language, line_number) in enumerate(code_blocks):
location = f"$.code_blocks[{idx}]" # Ignores contextCompare to _check_prompt which correctly passes location_prefix to extract_code_blocks. This causes inconsistent defect locations.
Minor Issues
6. Missing Docstring Update
File: src/sanityops_cli/defect_checker/markdown_reporter.py
Line: 151
The _build_report function docstring doesn't document the new qc_result parameter:
def _build_report(
...
qc_result: QCInspectionResult | None = None,
) -> str:
"""Render the full markdown report body."""
# Docstring doesn't mention qc_result7. Silent Tokenize Error Handling
File: src/sanityops_cli/code_contract_checker/rules/qc0_baseline.py
Lines: 271-274
Tokenize errors are silently swallowed, potentially missing bracket issues in malformed code:
except (tokenize.TokenError, IndentationError, SyntaxError):
tokens = [] # Silent failureConsider logging at debug level or adding a comment explaining why this is acceptable.
8. Redundant Tree Walks
File: src/sanityops_cli/code_contract_checker/rules/qc1_syntax.py
Lines: 43-57 vs 90-269
The first ast.walk(tree) loop collects imports, then ScopeCollector visits the tree again. This could be combined into a single pass for efficiency.
Positive Observations
- Good test coverage for the new functionality
- Proper use of
TYPE_CHECKINGfor avoiding circular imports - Clean separation of concerns between rules modules
- The deduplication logic for
inputSchema/parametersis well-implemented - Error handling for YAML/JSON parsing is robust
Overall Assessment
Ready to merge: With fixes
The PR introduces a valuable feature (AST-based code contract checking) with good structure and test coverage. However, the Python 3.11 compatibility issue is critical and must be addressed. The other issues are code quality improvements that should be fixed but don't block merging.
- Add version guard for ast.TryStar visitor method (Python 3.11+) - Remove unused artifact_result parameter from _read_artifact method
There was a problem hiding this comment.
🤖 Claude Code Review
PR #24 Code Review
Critical Issues
1. Missing Function _esc_md_cell in markdown_reporter.py
File: src/sanityops_cli/defect_checker/markdown_reporter.py
Lines: ~175, 176 (in _build_qc_section)
The function _esc_md_cell is called but never defined or imported:
desc = _esc_md_cell(d.description)
fix = _esc_md_cell(d.fix_suggestion)This will cause a NameError at runtime when QC results with defects are rendered. Either define this function locally or import it.
2. Incorrect Copyright Year
Files: All new files in src/sanityops_cli/code_contract_checker/
Lines: Line 1 of each file
All files contain Copyright 2026 zipsonken. This is likely a typo—should be 2024 or 2025.
Important Issues
3. Missing Unit Tests for New Module
Files: src/sanityops_cli/code_contract_checker/
The PR adds ~1,800 lines of new code for the code contract checker but includes no unit tests for:
CodeContractCheckerclass- Individual rule implementations (
check_qc_*functions) QCRendererclassextract_code_blocksfunction
Tests should cover at minimum: syntax detection, undefined name detection, resource leak detection, and edge cases.
4. Inconsistent Standard Library Module Lists
Files:
src/sanityops_cli/code_contract_checker/rules/qc0_baseline.py(lines 102-163,PYTHON_STDLIB)src/sanityops_cli/code_contract_checker/rules/qc3_pattern.py(lines 283-310,stdlib_modules)
Two different sets of stdlib modules exist:
PYTHON_STDLIBin qc0_baseline.py has ~50 modulesstdlib_modulesin qc3_pattern.py has ~25 modules
This inconsistency could cause false positives. Consolidate into a single shared constant.
5. Dead Code in qc1_syntax.py
File: src/sanityops_cli/code_contract_checker/rules/qc1_syntax.py
Lines: 70-75
# Top-level assignments (module-level variables)
elif isinstance(node, ast.Assign):
# Only track module-level assignments (those not inside functions)
# We need to check the parent, but ast.walk doesn't give us that
# So we'll use a different approach with a visitor
passThis entire block and the first ast.walk loop for assignments do nothing. The ScopeCollector visitor handles this correctly. Remove dead code.
Minor Issues
6. Missing Return Type Hint
File: src/sanityops_cli/code_contract_checker/checker.py
Line: 381
def _check_code_blocks(
self,
code_blocks: list[tuple[str, str, int]],
artifact_result: QCArtifactResult,
) -> None:The method should include -> None return type annotation.
7. Hardcoded Max Depth
File: src/sanityops_cli/code_contract_checker/checker.py
Line: 233
max_depth=10 is hardcoded. Consider making this configurable via the constructor for handling deeply nested schemas.
8. Missing yaml Dependency Verification
File: src/sanityops_cli/code_contract_checker/checker.py
Line: 31
The code imports yaml. Ensure PyYAML is listed in project dependencies.
Assessment
Ready to merge: No
The critical issue with the undefined _esc_md_cell function will cause runtime errors. The missing test coverage for a significant new feature is also a concern. Fix the critical and important issues before merging.
- Remove unreachable dunder-name check in QC-1.1 (subsumed by the leading-underscore check) - Reuse PYTHON_STDLIB from qc0_baseline in qc3_pattern instead of duplicating the module list - Propagate caller location prefixes through _check_code_blocks so defect locations stay consistent with their extraction context
There was a problem hiding this comment.
🤖 Claude Code Review
Code Review: PR #24 - Code Contract Checker
Critical Issues
1. Copyright Year Error
File: All new files in src/sanityops_cli/code_contract_checker/
Lines: Line 1 in each file
All new files have copyright year 2026, which is in the future. This should be corrected to the actual year (likely 2024 or 2025).
# Copyright 2026 zipsonken # Should be 2024 or 2025Important Issues
2. YAML Listed as Standard Library Module
File: src/sanityops_cli/code_contract_checker/rules/qc0_baseline.py
Line: 238
yaml (PyYAML) is not part of the Python standard library, but it's listed in PYTHON_STDLIB. This causes false negatives in missing import detection.
PYTHON_STDLIB = frozenset(
{
...
"yaml", # <-- PyYAML is NOT a stdlib module
}
)Fix: Remove "yaml" from PYTHON_STDLIB or add a separate set for third-party packages that are commonly pre-installed.
3. No Unit Tests for New Module
File: N/A
The entire code_contract_checker/ module (445 lines in checker.py, 478 lines in qc0_baseline.py, 394 lines in qc1_syntax.py, etc.) has no unit tests. Given this is a code analysis tool, comprehensive tests are essential to ensure correctness of:
- AST parsing and error handling
- Code block extraction regex
- Rule implementations
- Edge cases (truncated code, mixed languages, etc.)
4. Orphaned Code in Undefined Name Checker
File: src/sanityops_cli/code_contract_checker/rules/qc1_syntax.py
Lines: 87-95
Dead code with a pass statement and misleading comment:
# Top-level assignments (module-level variables)
elif isinstance(node, ast.Assign):
# Only track module-level assignments (those not inside functions)
# We need to check the parent, but ast.walk doesn't give us that
# So we'll use a different approach with a visitor
passThis branch does nothing and should be removed since the logic is handled by ScopeCollector.
5. Scope Tracking Does Not Track Scopes Properly
File: src/sanityops_cli/code_contract_checker/rules/qc1_syntax.py
Lines: 106-218
The ScopeCollector collects all definitions into a single flat set, ignoring actual Python scoping rules. This means a variable defined in function_a will be considered "defined" in function_b, causing false negatives.
class ScopeCollector(ast.NodeVisitor):
def __init__(self):
self.defined: set[str] = set() # Single flat set, no scope nestingFor example:
def foo():
x = 1
def bar():
print(x) # Should be flagged as undefined, but won't beRecommendation: Either implement proper scope tracking with a stack, or document the limitation and accept this as "global name checking only."
Minor Issues
6. Missing ast.get_source_segment Fallback Quality
File: src/sanityops_cli/code_contract_checker/rules/qc2_resource.py
Lines: 154, 235, 305
The fallback node.__class__.__name__ when ast.get_source_segment returns None is not informative:
evidence=ast.get_source_segment(code, node) or node.__class__.__name__,Fix: Return the line of code from the source using the line number, or return a more descriptive fallback.
7. Hardcoded Severity Colors
File: src/sanityops_cli/code_contract_checker/renderer.py
Lines: 144-148
Severity colors are hardcoded inside _format_defect. Consider making them class constants for maintainability:
severity_color = {
"P0": "red bold",
"P1": "yellow bold",
"P2": "dim",
}8. datetime.now() Called Multiple Times
File: src/sanityops_cli/code_contract_checker/checker.py
Lines: 109, 123, 181, etc.
datetime.now().isoformat() is called separately for each artifact. While not a performance issue, it could cause inconsistent timestamps if artifacts are checked around a second boundary. Consider passing a single timestamp.
9. Inconsistent JSON/YAML Error Handling in _check_tool
File: src/sanityops_cli/code_contract_checker/checker.py
Lines: 196-203
The code silently falls back to raw code block extraction on parse errors without logging:
except (json.JSONDecodeError, yaml.YAMLError):
# Not valid JSON/YAML - try to extract code blocks from raw content
code_blocks = extract_code_blocks(content, artifact_result, location_prefix="$.content")Consider adding a log message when falling back, especially in verbose mode.
10. Type Annotation for _build_parent_map Return
File: src/sanityops_cli/code_contract_checker/rules/qc2_resource.py
Line: 76
The return type dict[int, ast.AST] uses int as the key type, which represents id() values. A docstring explaining this is an object-identity map would improve clarity.
Overall Assessment
Ready to merge: No
The PR introduces a substantial new feature (Code Contract Checker) with good structure and integration. However:
- Must fix: Copyright year 2026 is clearly incorrect
- Must fix:
yamlincorrectly listed as stdlib - Should fix: Missing unit tests for new module
- Should address: Dead code in
qc1_syntax.py
The scope tracking limitation in check_qc_1_1_undefined_name is a design choice but should be documented. The implementation is otherwise well-structured with good separation of concerns.
- Document all _build_report parameters including qc_result - Explain why silent tokenize failure is acceptable (QC-0.1 already reports the syntax error as a P0 defect)
|
Addressed all findings from the automated review in 3d886d2, 967758f and 935fd29:
All 294 tests pass; lint and format checks are clean. 🤖 Addressed by Claude Code |
There was a problem hiding this comment.
🤖 Claude Code Review
Code Review: PR #24 - Code Contract Checker
Critical Issues
1. Undefined Function _esc_md_cell
File: src/sanityops_cli/defect_checker/markdown_reporter.py (lines 252-253 in new _build_qc_section)
desc = _esc_md_cell(d.description)
fix = _esc_md_cell(d.fix_suggestion)The function _esc_md_cell is called but not defined anywhere in this diff. If it doesn't exist in the original file, this will cause a NameError at runtime when generating QC reports.
Fix: Define the function or use inline escaping:
def _esc_md_cell(text: str | None) -> str:
if not text:
return "-"
return text.replace("|", "\\|").replace("\n", " ")Important Issues
2. Incorrect Copyright Year
Files: All new files in code_contract_checker/ (6 files)
# Copyright 2026 zipsonkenThe year 2026 is clearly incorrect. Should be 2024 or 2025.
Fix: Update to current year across all files.
3. Missing Unit Tests for Code Contract Checker
Path: tests/unit/code_contract_checker/ - Does not exist
The new code_contract_checker/ module (~1600+ lines) has no unit tests. While there are tests for the renderer and reporter changes, the core QC logic is untested:
checker.py- Main inspection logicrules/qc0_baseline.py- Syntax/structure checksrules/qc1_syntax.py- Name resolutionrules/qc2_resource.py- Resource leak detectionrules/qc3_pattern.py- Pattern detection
Fix: Add unit tests covering:
- Syntax error detection (QC-0.1)
- Code block extraction edge cases
- Undefined name detection
- File handle leak detection
- Import analysis
4. StrEnum String Comparison Pattern
File: src/sanityops_cli/code_contract_checker/renderer.py (lines 91, 107)
gate_style = "red bold" if result.gate_result == "FAIL" else "green bold"
# ...
if result.gate_result == "FAIL":While this works with StrEnum, comparing against string literals is inconsistent with enum usage elsewhere. The GateResult enum is properly defined.
Fix: Use enum values consistently:
from .models import GateResult
# ...
if result.gate_result == GateResult.FAIL:Minor Issues
5. Imports Inside Function Body
File: src/sanityops_cli/code_contract_checker/rules/qc0_baseline.py (lines 304-305)
import io
import tokenizeThese imports are inside check_qc_0_3_structural_completeness() function. While this works, it's unconventional for standard library imports.
Fix: Move to module level imports.
6. Dead Code Comment
File: src/sanityops_cli/code_contract_checker/rules/qc1_syntax.py (lines 87-91)
elif isinstance(node, ast.Assign):
# Only track module-level assignments (those not inside functions)
# We need to check the parent, but ast.walk doesn't give us that
# So we'll use a different approach with a visitor
passThis is dead code with a TODO-like comment. The actual implementation is handled by ScopeCollector class below.
Fix: Remove the dead elif branch and the pass statement.
7. Type Ignore Comment Indicates Weak Typing
File: src/sanityops_cli/code_contract_checker/rules/qc1_syntax.py (line 149)
def visit_AsyncFunctionDef(self, node: ast.AsyncFunctionDef) -> None:
# Same as FunctionDef
self.visit_FunctionDef(node) # type: ignoreThe # type: ignore suppresses a legitimate type mismatch.
Fix: Use a shared helper method instead of reusing visit_FunctionDef:
def _visit_function(self, node: ast.FunctionDef | ast.AsyncFunctionDef) -> None:
# Shared implementationOverall Assessment
Ready to merge: No
The PR introduces a valuable Code Contract Checker feature but has one critical issue (_esc_md_cell undefined) that will cause runtime failures. The missing test coverage for the new module is a significant gap for a ~1600 line addition.
Required fixes before merge:
- Define
_esc_md_cellfunction or remove the call - Correct copyright year from 2026 to current year
- Add unit tests for the code_contract_checker module
Add 48 tests covering the Code Contract checker module: - All check_qc_* rule implementations (syntax, undefined names, type mismatch, resource leaks, patterns) - extract_code_blocks parsing and language aliasing - CodeContractChecker over prompts/tools/skills including .py files, malformed JSON fallback, and clean-artifact pass cases - QCRenderer terminal output for pass/fail gates - save_qc_report markdown generation and unwritable-path handling Also: - Remove dead ast.Assign branch in QC-1.1 (handled by ScopeCollector) - Drop yaml from PYTHON_STDLIB (PyYAML is a third-party package) - Document the flat-scope limitation of check_qc_1_1_undefined_name
|
Addressed the two automated reviews in 0e15e34 (and earlier commits): False claims (no change needed):
Fixed in 0e15e34:
Full suite: 342 passed, lint and format clean. 🤖 Addressed by Claude Code |
There was a problem hiding this comment.
🤖 Claude Code Review
Code Review: PR #24 - Code Contract Checker
Critical Issues
1. Missing _esc_md_cell function
File: src/sanityops_cli/defect_checker/markdown_reporter.py
Lines: 241-242
The function _esc_md_cell is called but never defined or imported:
desc = _esc_md_cell(d.description)
fix = _esc_md_cell(d.fix_suggestion)This will cause a NameError at runtime when generating QC reports with defects.
Important Issues
1. Copyright Year Typo
Files: All new files in code_contract_checker/
All files have copyright year 2026. This appears to be a typo — should likely be the current year.
2. Imports Inside Functions (PEP 8 Violation)
File: src/sanityops_cli/code_contract_checker/rules/qc0_baseline.py
Lines: 377-378
import io
import tokenizeThese imports are inside check_qc_0_3_structural_completeness. Move to top of file per PEP 8.
3. Late Import Inside Function
File: src/sanityops_cli/defect_checker/markdown_reporter.py
Lines: 267-268
from sanityops_cli.code_contract_checker.models import RULE_DEFINITIONS, QCRuleIDImport is inside _build_qc_section. This will execute on every report generation. Move to module imports.
4. Type Ignore Comment Indicates Structural Issue
File: src/sanityops_cli/code_contract_checker/rules/qc1_syntax.py
Line: 142
def visit_AsyncFunctionDef(self, node: ast.AsyncFunctionDef) -> None:
self.visit_FunctionDef(node) # type: ignoreThe # type: ignore masks a type error. Consider refactoring with a shared helper method or using ast.FunctionDef | ast.AsyncFunctionDef union.
5. Recursive Call Missing max_depth Parameter
File: src/sanityops_cli/code_contract_checker/checker.py
Lines: 282, 287
The recursive calls to _check_tool_parameters pass depth=depth + 1 but not max_depth. While it works due to the default, explicit passing would be clearer:
self._check_tool_parameters(
prop_def, artifact_result, prop_location, depth=depth + 1, max_depth=max_depth
)Minor Issues
1. QC Gate Result Not Enforced
File: src/sanityops_cli/commands/inspect.py
Lines: 555-585
The QC lint runs and reports gate_result, but if the gate is FAIL, the command still proceeds to LLM analysis. Consider adding a --fail-on-qc flag or at least documenting the intended behavior.
2. Magic Number for Recursion Depth
File: src/sanityops_cli/code_contract_checker/checker.py
Line: 242
max_depth: int = 10,Consider extracting to a module constant MAX_PARAMETER_DEPTH = 10 for configurability.
3. Missing Test Coverage
File: tests/unit/code_contract_checker/test_checker.py
No explicit tests for:
_check_skillwith YAML frontmatter parsing_parse_markdown_sectionsedge cases_check_tool_parametersrecursion depth limit
4. Code Block Regex Limitation
File: src/sanityops_cli/code_contract_checker/rules/qc0_baseline.py
Line: 35
CODE_BLOCK_PATTERN = re.compile(r"```(\w*)\s*\n(.*?)```(?:\n|$)", re.DOTALL)This regex cannot handle code blocks containing ``` inside them (nested backticks). Consider documenting this known limitation.
Positive Observations
- Clean architecture with proper separation of concerns
- Comprehensive rule set covering syntax, resources, and patterns
- Good test coverage for individual rules
- Proper error handling for file I/O operations
- No security vulnerabilities identified (no eval/exec, safe AST parsing)
- Proper use of dataclasses for models
Overall Assessment
Ready to merge: No
Required fixes:
- Add the missing
_esc_md_cellfunction tomarkdown_reporter.py(Critical — runtime error) - Fix copyright year
- Move late imports to module level
The code is well-structured and adds valuable static analysis capabilities. Once the missing function is added, this should be ready to merge.
- Move io/tokenize imports from check_qc_0_3_structural_completeness to qc0_baseline module scope (PEP 8 E402 style consistency) - Move RULE_DEFINITIONS/QCRuleID imports to module scope in both markdown reporters
|
Addressed the two automated reviews in 554fcd4 (valid findings only): Repeated false claims (already debunked in the previous comment):
Fixed in 554fcd4:
Full suite: 342 passed, lint and format clean. 🤖 Addressed by Claude Code |
There was a problem hiding this comment.
🤖 Claude Code Review
Code Review: PR #24 - Code Contract Checker
Critical Issues
1. Missing _esc_md_cell Function
File: src/sanityops_cli/code_contract_checker/markdown_reporter.py
Line: ~199
The function _esc_md_cell is called but never defined:
desc = _esc_md_cell(d.description)
fix = _esc_md_cell(d.fix_suggestion)This will cause a NameError at runtime when generating QC reports with defects. Either import from an existing module or define locally:
def _esc_md_cell(text: str | None) -> str:
if not text:
return "-"
return text.replace("|", "\\|").replace("\n", " ")2. Copyright Year Typo
Files: All new files in code_contract_checker/
All copyright headers say "2026" which appears to be a typo (future year). Should be the actual year of creation (likely 2024 or 2025).
Important Issues
3. Missing GateResult Export
File: src/sanityops_cli/code_contract_checker/__init__.py
GateResult is imported and exported, but it's not imported in checker.py where QCInspectionResult.gate_result is used. While it works due to string comparison, it would be cleaner to import the enum where used.
4. Inconsistent Logging Style
File: src/sanityops_cli/commands/inspect.py
Lines: ~577-579
Mix of f-string and %-style logging:
logger.info(
f"QC lint completed in {qc_elapsed:.2f}s: {qc_result.total_defects} defects "
...
)vs:
logger.warning("Could not read %s: %s", path, e) # in checker.pyPrefer %-style for logging (lazy evaluation) consistently.
5. Code Block Regex Doesn't Handle Edge Cases
File: src/sanityops_cli/code_contract_checker/rules/qc0_baseline.py
Line: ~34
The regex CODE_BLOCK_PATTERN doesn't handle:
- Nested code blocks (
inside) - Code blocks at EOF without trailing newline (mitigated by
(?:\n|$)) - Tilde-delimited code blocks (
~~~)
Consider documenting these limitations.
6. Test Missing Empty String Case
File: tests/unit/code_contract_checker/test_rules.py
Class: TestQc02LanguageDeclaration
Only tests None and valid language. Should also test empty string "":
def test_empty_language_reports_p1(self):
result = _result()
check_qc_0_2_language_declaration("", result, "$.c", 7)
assert len(result.defects) == 1Minor Issues
7. Hardcoded Recursion Limit
File: src/sanityops_cli/code_contract_checker/checker.py
Line: ~251
max_depth=10 is hardcoded. Consider making it a class constant or parameter for configurability.
8. Potential False Positive in QC-3.3
File: src/sanityops_cli/code_contract_checker/rules/qc3_pattern.py
Lines: ~27-36
Pattern "password" could flag legitimate examples in documentation. Consider checking if the value is in a comment vs actual assignment.
9. Missing Docstring for _call_name
File: src/sanityops_cli/code_contract_checker/rules/qc2_resource.py
Line: ~44
The helper function _call_name lacks a docstring explaining its purpose.
Overall Assessment
Ready to merge: No (Critical fix required)
The code is well-structured with good separation of concerns and comprehensive test coverage. The QC rules are sensible and the integration with the existing inspect command is clean. However, the missing _esc_md_cell function is a runtime error that must be fixed before merge. The copyright year should also be corrected.
…o values - Compare against GateResult.FAIL instead of a string literal in the QC renderer for type clarity - Extract MAX_TOOL_SCHEMA_DEPTH as a module constant - Document CODE_BLOCK_PATTERN limitations (nested/tilde/long fences) - Skip comment-only lines in QC-3.3 so documentation examples like '# password = "password"' are not flagged as hardcoded demo values - Add tests for empty/whitespace language declarations and comment example skipping
There was a problem hiding this comment.
🤖 Claude Code Review
Code Review: PR #24 - Code Contract Checker
Critical Issues
1. Missing Function Definition - Runtime Error
- File:
src/sanityops_cli/defect_checker/markdown_reporter.py - Lines: ~170-175 (in
_build_qc_section) - Issue: The function
_esc_md_cellis called but not defined anywhere in the visible diff. It's not imported fromrenderer.py(only_MODULE_LABELSand_dedup_defectsare imported). If this function doesn't exist in the original file, this will cause aNameErrorat runtime. - Recommendation: Verify
_esc_md_cellexists in the codebase, or add the definition:def _esc_md_cell(text: str | None) -> str: if not text: return "-" return text.replace("|", "\\|").replace("\n", " ")
2. Incorrect Copyright Year
- Files: All new files in
code_contract_checker/module - Issue: Copyright year is listed as "2026" which appears to be a typo. Should likely be 2024 or 2025.
- Recommendation: Correct the copyright year to the actual year of creation.
Important Issues
3. Code Block Regex Edge Cases
- File:
src/sanityops_cli/code_contract_checker/rules/qc0_baseline.py - Lines: 36-40
- Issue: The
CODE_BLOCK_PATTERNregex won't match:- Nested code fences (
```inside a code block) - Tilde-delimited fences (
~~~) - Code blocks at end of file without trailing content after closing fence in some cases
- Nested code fences (
- Recommendation: Document known limitations in the docstring (already partially done) or consider using a proper markdown parser for production use.
4. Inefficient Property Calculation
- File:
src/sanityops_cli/code_contract_checker/models.py - Lines: 140-144 (
gate_resultproperty) - Issue:
gate_resultiterates through all artifacts counting P0 defects each time it's accessed. Called multiple times in rendering. - Recommendation: Cache the result or compute once during inspection:
@functools.cached_property def gate_result(self) -> GateResult: return GateResult.FAIL if self.p0_count > 0 else GateResult.PASS
5. Tool Format Normalization Breaking Change
- File:
src/sanityops_cli/utils/artifact_packer.py - Lines: 343-356
- Issue: Tool format changed from OpenAI wrapper format (
{"type": "function", "function": {...}}) to flat format ({"name": ..., "parameters": ...}). This could break consumers expecting OpenAI format. - Recommendation: Verify backend compatibility or add format conversion documentation.
Minor Issues
6. Type Ignore Comment
- File:
src/sanityops_cli/code_contract_checker/rules/qc1_syntax.py - Line: 174
- Issue:
# type: ignorecomment onvisit_AsyncFunctionDefsuggests type mismatch. The method signature expectsast.AsyncFunctionDefbut callsvisit_FunctionDefwhich expectsast.FunctionDef. - Recommendation: This is acceptable since both have the same interface for the needed fields, but consider adding a comment explaining why it's safe.
7. Missing Test Coverage
- Files:
tests/unit/code_contract_checker/test_rules.py - Issue: Missing tests for:
- F-strings containing brackets
- Multi-line strings with unbalanced brackets
- Binary file handling (would trigger
UnicodeDecodeErrorpath) - Empty artifact files
- Recommendation: Add edge case tests for robustness.
8. Inconsistent Import Style
- File:
src/sanityops_cli/code_contract_checker/checker.py - Lines: 25-32
- Issue: Imports
QCArtifactResult, QCInspectionResultfrom.models, but__init__.pyexports more symbols. Consider importing from__init__for consistency.
Positive Observations
- Well-structured code with clear separation of concerns
- Comprehensive test coverage for main functionality
- Good use of AST for Python code analysis
- Proper handling of Python version compatibility (e.g.,
ast.TryStarcheck) - Clear documentation of limitations in comments
- Defensive programming in
_read_artifactwith proper exception handling
Overall Assessment: Ready to merge: With fixes
The PR is well-designed and mostly complete. Must fix the _esc_md_cell function issue before merging (verify existence or add definition). The copyright year correction and property caching are recommended but not blocking.
|
Addressed the automated review in 3877c84 (valid findings only): False claims (4th consecutive repetition, no change):
Fixed in 3877c84:
Not applied:
Full suite: 345 passed, lint and format clean. 🤖 Addressed by Claude Code |
- Cover f-string braces and triple-quoted string brackets in QC-0.3 - Cover binary (UTF-8 decode failure) and empty artifact files in the checker - Explain why reusing visit_FunctionDef for AsyncFunctionDef is safe and narrow the type: ignore to arg-type
|
Addressed the automated review in 8086c09 (valid findings only): False / already-handled claims (5th repetition, no change):
Fixed in 8086c09:
Not applied (deliberate):
Full suite: 349 passed, lint and format clean. 🤖 Addressed by Claude Code |
There was a problem hiding this comment.
🤖 Claude Code Review
Code Review: PR #24 - Code Contract Checker
Critical Issues
1. Missing Function _esc_md_cell
File: src/sanityops_cli/defect_checker/markdown_reporter.py
Lines: 204-205
The function _esc_md_cell is used but never defined or imported:
desc = _esc_md_cell(d.description)
fix = _esc_md_cell(d.fix_suggestion)This will cause a NameError at runtime when QC defects are present in the report. Either define the function locally (escaping | and \ for markdown tables) or import it if it exists elsewhere.
2. Python Version Compatibility - StrEnum
File: src/sanityops_cli/code_contract_checker/models.py
Lines: 25, 28, 33
StrEnum was added in Python 3.11. If this project supports Python 3.10 or earlier, this will cause an ImportError:
from enum import StrEnum
class QCRuleID(StrEnum):
class Severity(StrEnum):
class GateResult(StrEnum):Either document Python 3.11+ as a requirement, or implement a compatibility fallback:
try:
from enum import StrEnum
except ImportError:
from enum import Enum
class StrEnum(str, Enum):
passImportant Issues
3. Incorrect Copyright Year
Files: All new files
Lines: First line of each file header
All new files use Copyright 2026 which appears to be a typo. Should be the actual year of creation (likely 2024 or 2025).
4. Untested Depth Limiting
File: src/sanityops_cli/code_contract_checker/checker.py
Lines: 22, 235-237
The MAX_TOOL_SCHEMA_DEPTH = 10 guard for preventing stack overflow on deeply-nested schemas is not covered by tests. Add a test case with a schema nested beyond 10 levels to verify it returns safely without crashing.
5. Overly Generic Pattern in High-Risk Calls
File: src/sanityops_cli/code_contract_checker/rules/qc2_resource.py
Lines: 30-37
HIGH_RISK_CALLS includes "connect" which is too generic and will cause false positives on common code like mqtt.connect(), db.connect(), etc. Consider using more qualified names like "socket.connect" or removing this entry.
Minor Issues
6. Silent Failure in Non-Verbose Mode
File: src/sanityops_cli/code_contract_checker/checker.py
Lines: 149-152
File read errors are only logged when verbose=True. This makes debugging difficult when files silently fail to process. Consider always logging at DEBUG level:
logger.debug("Could not read %s: %s", path, e)
if self.verbose:
logger.warning("Could not read %s: %s", path, e)7. Inconsistent Import Style
File: src/sanityops_cli/code_contract_checker/checker.py
Lines: 41-52
Some imports use parentheses for line continuation while others don't. Consider using consistent formatting throughout.
8. Type Ignore Comment Justification
File: src/sanityops_cli/code_contract_checker/rules/qc1_syntax.py
Lines: 115-117
The # type: ignore[arg-type] on visit_AsyncFunctionDef could be avoided by extracting common logic into a helper method that accepts the union type.
Overall Assessment
Ready to merge: No
The critical issue with the missing _esc_md_cell function will cause runtime errors. The Python version compatibility issue with StrEnum needs clarification on minimum supported Python version. These must be fixed before merge.
Once the critical issues are addressed, the code is well-structured with good test coverage for the core functionality. The architecture cleanly separates concerns (checker, rules, rendering, reporting) and the integration into the existing inspect command is well-done.
- Log file read failures at DEBUG in addition to the verbose warning so silent skips stay debuggable in non-verbose runs - Add a regression test proving deeply-nested tool schemas (25 levels) terminate safely at MAX_TOOL_SCHEMA_DEPTH
|
Addressed the automated review in 9b8a648 (valid findings only): False / already-handled claims (6th repetition, no change):
Non-issues (verified, no change):
Fixed in 9b8a648:
Not applied:
Full suite: 350 passed, lint and format clean. 🤖 Addressed by Claude Code |
There was a problem hiding this comment.
🤖 Claude Code Review
PR #24 Code Review: Code Contract Checker
Critical Issues
1. Undefined function _esc_md_cell
File: src/sanityops_cli/defect_checker/markdown_reporter.py (lines ~176-177)
The function _build_qc_section calls _esc_md_cell() to escape markdown table cell content:
desc = _esc_md_cell(d.description)
fix = _esc_md_cell(d.fix_suggestion)However, _esc_md_cell is neither defined in this file nor imported. This will cause a NameError at runtime when the QC section is rendered for any artifact with defects.
Fix: Define the helper function or import it:
def _esc_md_cell(text: str | None) -> str:
"""Escape pipe and newline characters for markdown table cells."""
if not text:
return "-"
return text.replace("|", "\\|").replace("\n", " ")Important Issues
1. Copyright Year Appears Incorrect
Files: All new files in code_contract_checker/
The copyright header uses year 2026, which is a future date. This appears to be a typo.
# Copyright 2026 zipsonkenFix: Update to the correct year.
Minor Issues
1. Code Block Pattern Limitations
File: src/sanityops_cli/code_contract_checker/rules/qc0_baseline.py (lines 36-42)
The CODE_BLOCK_PATTERN regex has documented limitations:
- Tilde-delimited fences (
~~~) are not matched - Opening fences with more than 3 backticks are not matched
- Nested code blocks are not supported
While documented, these limitations could cause false negatives for valid code blocks using alternative fence syntaxes. Consider expanding support or documenting this in user-facing documentation.
2. Missing QC-0.2 in _QC_PYTHON_CHECKS
File: src/sanityops_cli/code_contract_checker/checker.py (lines 42-53)
The _QC_PYTHON_CHECKS tuple does not include check_qc_0_2_language_declaration because it's handled separately during code block extraction. This is intentional but could be clarified with a comment to prevent future confusion.
3. Inconsistent Return Type for _read_artifact
File: src/sanityops_cli/code_contract_checker/checker.py (lines 112-122)
The method _read_artifact returns str | None, but callers immediately check if content is None. Consider raising a custom exception or logging at WARNING level by default (currently only logs WARNING in verbose mode).
Overall Assessment
Ready to merge: No (requires fixes)
The critical issue with the undefined _esc_md_cell function will cause runtime errors when the QC report is generated for artifacts with defects. This must be fixed before merging. The copyright year issue should also be corrected. Otherwise, the implementation is well-structured with good test coverage and clean integration into the existing inspect command.
The language-declaration rule inspects the fence marker, not the code body, so it runs inside extract_code_blocks during block discovery rather than in the per-snippet _QC_PYTHON_CHECKS tuple.
|
Addressed the automated review in 699824c. 7th consecutive repetition of the same false claims (no change):
Fixed in 699824c:
Not applied:
Full suite: 350 passed, lint and format clean. 🤖 Addressed by Claude Code |
There was a problem hiding this comment.
🤖 Claude Code Review
Code Review for PR #24: Code Contract Checker
Critical Issues
-
src/sanityops_cli/code_contract_checker/checker.py(line 159)line_numberis extracted from code blocks but not passed to_run_qc_checks. Hardcodedline_offset=0causes incorrect line numbers in defect reports for prompt files.for idx, (code, language, line_number) in enumerate(code_blocks): location = f"$.content.code_blocks[{idx}]" self._run_qc_checks(code, language, artifact_result, location, line_offset=0) # BUG: line_number ignored
Fix: Either pass
line_numberor use_check_code_blocks()like_check_skill()does:self._check_code_blocks(code_blocks, artifact_result, location_prefix="$.content")
Important Issues
-
src/sanityops_cli/code_contract_checker/checker.py(line 131-136)_read_artifact()catches specific exceptions but may fail on other path-related errors (e.g.,IsADirectoryError). Consider broadening the exception handling:except (FileNotFoundError, PermissionError, UnicodeDecodeError, IsADirectoryError, OSError) as e:
-
src/sanityops_cli/code_contract_checker/rules/qc0_baseline.py(line 35)Regex
CODE_BLOCK_PATTERNuses\w*for language identifiers, which won't match languages with non-word characters likec++orc#. Consider:CODE_BLOCK_PATTERN = re.compile(r"```([^\n]*)\n(.*?)```(?:\n|$)", re.DOTALL)
Minor Issues
-
Multiple new files (copyright headers)
Copyright year is 2026 in all new files. This appears to be a typo (likely intended 2024 or 2025).
-
src/sanityops_cli/code_contract_checker/rules/qc1_syntax.py(lines 52-167)Nested
ScopeCollectorandNameReferenceVisitorclasses are defined insidecheck_qc_1_1_undefined_name(), creating new class objects on every invocation. For hot paths, consider defining them at module level. -
src/sanityops_cli/code_contract_checker/models.py(lines 153-170)Properties
p0_count,p1_count,p2_countiterate over all artifacts and defects multiple times. Consider caching or computing counts once in a single pass if performance becomes a concern.
Test Coverage
Tests are comprehensive and cover:
- All QC rules individually
- Edge cases (binary files, empty files, deeply nested schemas)
- Terminal and markdown rendering
- Defect deduplication logic
Overall Assessment
Ready to merge: No
The critical issue with incorrect line number reporting in _check_prompt() must be fixed before merge. The other issues can be addressed in follow-up PRs.
- CODE_BLOCK_PATTERN now captures the full fence info string and takes its first token as the language, so languages with non-word characters (c++, c#) and titled fences are recognized - _read_artifact catches OSError plus UnicodeDecodeError, covering IsADirectoryError and other path failures without crashing
|
Addressed the automated review in 8514e4c. Claimed Critical issue is false:
Fixed in 8514e4c:
Not applied (deliberate):
Full suite: 352 passed, lint and format clean. 🤖 Addressed by Claude Code |
There was a problem hiding this comment.
🤖 Claude Code Review
Code Review: PR #24 - Code Contract Checker
Critical Issues
1. Missing Function Definition _esc_md_cell
File: src/sanityops_cli/defect_checker/markdown_reporter.py
Lines: 201-202
The function _esc_md_cell() is used in _build_qc_section() but is not defined anywhere in the visible diff. This will cause a NameError at runtime when QC defects are rendered:
desc = _esc_md_cell(d.description)
fix = _esc_md_cell(d.fix_suggestion)Fix: Add the missing helper function:
def _esc_md_cell(text: str | None) -> str:
"""Escape markdown table cell content."""
if text is None:
return ""
return str(text).replace("|", "\\|").replace("\n", " ")Important Issues
2. Incorrect Copyright Year
Files: All new files
Lines: Top of each file
All new files use # Copyright 2026 zipsonken. The year 2026 appears to be a typo and should likely be 2024 or 2025.
3. Breaking Change in Tool Schema Format
File: src/sanityops_cli/utils/artifact_packer.py
Lines: 171-186, 320-362
The tool packing has been changed from OpenAI function calling format ({"type": "function", "function": {...}}) to a flat format ({"name": ..., "parameters": ...}). While the docstring mentions this is "provider-neutral," this could break existing consumers expecting the OpenAI format.
Recommendation: Ensure this change is documented in release notes/changelog, and verify backend compatibility.
4. Type Ignore Workarounds
File: src/sanityops_cli/code_contract_checker/rules/qc1_syntax.py
Lines: 101-102
File: src/sanityops_cli/code_contract_checker/rules/qc3_pattern.py
Line: 301
The # type: ignore[arg-type] comments indicate a design issue. Consider refactoring to extract common visitor logic into a shared method:
def _visit_function_def(self, node: ast.FunctionDef | ast.AsyncFunctionDef) -> None:
# Shared implementationMinor Issues
5. Missing Return Type Annotation
File: src/sanityops_cli/code_contract_checker/checker.py
Line: 447
_check_code_blocks method should have explicit return type:
def _check_code_blocks(
self,
code_blocks: list[tuple[str, str, int]],
artifact_result: QCArtifactResult,
location_prefix: str = "$",
) -> None: # Add this6. Unreachable Code Pattern
File: src/sanityops_cli/code_contract_checker/rules/qc0_baseline.py
Lines: 366-368
The if normalized_lang: check will always be true when lang was empty, because check_qc_0_2_language_declaration is called first and will flag the issue, but the block is still not added. This is correct behavior, but the logic flow could be clearer.
Positive Observations
- Good test coverage with 328 lines of rule tests
- Proper handling of Python 3.11+
TryStarnode with version check - Defensive coding with depth limits for nested schemas
- Clear documentation of limitations (scope handling, fence parsing)
- Proper separation of concerns (checker, renderer, reporter, models)
Overall Assessment
Ready to merge: No (Critical fix required)
The PR introduces valuable functionality but contains a critical bug where _esc_md_cell is called but not defined. This will cause runtime errors when generating QC reports. Once this function is added and the copyright year is corrected, the PR should be ready for merge.
…ease notes - Replace type: ignore workarounds in QC-1.1 and QC-3.2 visitors with union-typed helpers shared by the sync/async function visitors - Record the QC lint feature, the provider-neutral tool upload format (breaking), and recent report fixes in the changelog
There was a problem hiding this comment.
🤖 Claude Code Review
Code Review: PR #24 - Code Contract (QC) Lint Check
Summary
This PR adds a comprehensive AST-based Code Contract lint system that runs before LLM analysis. The implementation is well-structured with good documentation, error handling, and test coverage. Below are the issues identified.
Important
1. max_depth parameter not propagated in recursive calls
File: src/sanityops_cli/code_contract_checker/checker.py (lines 308-318)
The _check_tool_parameters method accepts an optional max_depth parameter, but recursive calls don't pass it, causing the default MAX_TOOL_SCHEMA_DEPTH to be used instead of any custom value:
self._check_tool_parameters(
prop_def, artifact_result, prop_location, depth=depth + 1
)
# max_depth missing - resets to default on each callFix: Propagate max_depth=max_depth in recursive calls if customization is intended to be supported.
Minor
1. Inaccurate pattern description
File: src/sanityops_cli/code_contract_checker/rules/qc3_pattern.py (line 37)
The regex \d{4,6} matches 4-6 digit numbers, not specifically "sequential digits" as the description states:
(re.compile(r'password\s*=\s*["\'](\d{4,6})["\']'), "sequential digits"),Fix: Change description to "weak numeric password" or similar.
2. Type annotation for visit_TryStar
File: src/sanityops_cli/code_contract_checker/rules/qc2_resource.py (line 237)
The method uses ast.AST annotation instead of the more specific ast.TryStar:
def visit_TryStar(self, node: ast.AST) -> None:Fix: Use conditional type hint or add comment explaining the runtime safety via hasattr guard.
3. Missing _esc_md_cell function definition
File: src/sanityops_cli/defect_checker/markdown_reporter.py (line 229)
The function _esc_md_cell is called but not defined in the visible diff. If this is an existing function, this is fine; otherwise, it's a missing import/definition.
Design Consideration (Not a Bug)
QC Gate doesn't block LLM analysis on P0 defects
The gate_result is computed and rendered, but P0 (blocking) defects don't prevent subsequent LLM analysis. This may be intentional (allowing users to see all issues together), but the severity name "blocking" and gate status "FAIL" could mislead users into thinking processing stops. Consider:
- Adding a
--fail-on-qcflag for CI/CD use cases - Clarifying in documentation that QC is advisory by default
Positive Observations
- ✅ Comprehensive test coverage (checker, renderer, rules, reporter)
- ✅ Good error handling (file read errors, UTF-8 decoding, deeply nested schemas)
- ✅ Well-documented limitations in code block parsing
- ✅ Breaking change properly documented in CHANGELOG
- ✅ Defect deduplication correctly handles
inputSchema/parametersaliasing
Overall Assessment
Ready to merge: With fixes
The core functionality is solid. Fix the max_depth propagation issue (Important) and consider the minor description/documentation improvements before merging.
|
Addressed the automated review in 30fd352. 9th repetition of false claims (no change):
Fixed in 30fd352:
Not applied (deliberate):
Full suite: 352 passed, lint and format clean. 🤖 Addressed by Claude Code |
… accurately - Recursive _check_tool_parameters calls now forward the caller's max_depth instead of silently resetting to the default - Rename 'sequential digits' pattern labels to match what the regex actually matches (short numeric password, repeated digits) - Explain why visit_TryStar is annotated ast.AST rather than ast.TryStar
|
Addressed the automated review in d58576c. Fixed:
Not applied (deliberate):
Full suite: 352 passed, lint and format clean. 🤖 Addressed by Claude Code |
There was a problem hiding this comment.
🤖 Claude Code Review
Code Review: PR #24 - Code Contract Lint Check
Summary
This PR adds a Code Contract (QC) lint check feature with AST-based static analysis, changes tool schema format to a provider-neutral flat structure, and fixes duplicate defect reporting. The implementation is well-structured with good test coverage, but has one critical bug and one potential issue requiring attention.
Issues
Critical
1. Undefined function _esc_md_cell in markdown_reporter.py
- File:
src/sanityops_cli/defect_checker/markdown_reporter.py - Lines: Used in
_build_qc_section()around line 260 - Issue: The function
_esc_md_cellis called to escape markdown cell content:But this function is not defined anywhere in the diff or visible file context. This will cause adesc = _esc_md_cell(d.description) fix = _esc_md_cell(d.fix_suggestion)
NameErrorat runtime when generating QC reports with defects. - Fix: Define the missing function, e.g.:
def _esc_md_cell(text: str | None) -> str: if not text: return "-" return text.replace("|", "\\|").replace("\n", " ")
Important
2. Potential import-time error with TYPE_CHECKING guard
- File:
src/sanityops_cli/defect_checker/markdown_reporter.py - Lines: 30-31 (TYPE_CHECKING import) and function signatures using
QCInspectionResult - Issue:
QCInspectionResultis imported only underTYPE_CHECKING, but used in runtime type annotations for_build_qc_section()andsave_markdown_report(). Withoutfrom __future__ import annotationsat the top of the file, this will cause aNameErrorat import time. The diff context doesn't show whether this future import exists. - Fix: Verify
from __future__ import annotationsis present, or move the import outside TYPE_CHECKING.
3. Missing test for QC report when --skip-defect-check is used
- File:
src/sanityops_cli/commands/inspect.py - Lines: 656-663
- Issue: New code path saves QC report when defect check is skipped, but no integration test covers this branch.
- Fix: Add a test for
inspect --skip-defect-checkthat verifies the QC report is saved.
Minor
4. Inconsistent parameter naming
- Files:
checker.pylines 282, 225, 464 - Issue: The method
_run_qc_checksparameter is namedline_offset, but callers pass values namedline_number. While functionally correct, this naming inconsistency reduces code readability. - Fix: Rename to
line_numberconsistently, or document the semantic equivalence.
5. Missing _esc_md_cell test coverage
- File:
tests/unit/defect_checker/test_markdown_reporter.py - Issue: No test verifies that markdown special characters in QC defect descriptions/fixes are properly escaped.
- Fix: Add test case with pipes (
|) and newlines in description text.
6. Hardcoded credential pattern may miss common variants
- File:
src/sanityops_cli/code_contract_checker/rules/qc3_pattern.py - Lines: 42-49
- Issue: The
WEAK_CREDENTIAL_PATTERNSregex forapi_keyusesxxxbut common placeholders likesk-...,test,demo,placeholderare not caught. - Fix: Consider expanding patterns or documenting intentional scope limitation.
Positive Observations
- Clean module structure with clear separation of concerns (models, checker, renderer, reporters, rules)
- Comprehensive test coverage for new QC rules (328 lines of rule tests)
- Good documentation in docstrings and inline comments
- Proper handling of edge cases: binary files, deeply nested schemas, invalid JSON
- Correct use of AST-based analysis without LLM dependency
- Breaking change is properly documented in CHANGELOG.md
Overall Assessment
Ready to merge: No
The critical issue with _esc_md_cell being undefined will cause runtime errors. This must be fixed before merge. The Important issues should be verified or addressed.
- Add an integration test asserting --skip-defect-check still runs the QC lint and writes a standalone qc-inspect-*.md report containing the detected QC-1.1 defect - Clarify the line_offset parameter semantics in _run_qc_checks
|
Addressed the automated review in 5ab9e7f. Verified non-issues:
Fixed in 5ab9e7f:
Not applied (deliberate):
Full suite: 353 passed, lint and format clean. 🤖 Addressed by Claude Code |
There was a problem hiding this comment.
🤖 Claude Code Review
PR #24 Code Review
Critical Issues
1. Missing _esc_md_cell function definition
File: src/sanityops_cli/defect_checker/markdown_reporter.py (lines 233-234 in diff)
The function _esc_md_cell is called in _build_qc_section():
desc = _esc_md_cell(d.description)
fix = _esc_md_cell(d.fix_suggestion)However, this function is not defined anywhere in the visible diff. If it doesn't exist in the original file, this will cause a NameError at runtime. Verify this function exists or add its definition.
Important Issues
2. Type annotation workaround could cause static analysis warnings
File: src/sanityops_cli/code_contract_checker/rules/qc2_resource.py (lines 287-299)
if hasattr(ast, "TryStar"):
def visit_TryStar(self, node: ast.AST) -> None:The parameter is typed as ast.AST but the method body accesses node.body, which is specific to ast.TryStar. While the comment explains the Python < 3.11 compatibility rationale, this will trigger type checker warnings. Consider using # type: ignore[union-attr] on the node.body access or using Union types with runtime checks.
3. Missing error logging in tool parsing
File: src/sanityops_cli/code_contract_checker/checker.py (line 198)
except (json.JSONDecodeError, yaml.YAMLError):
# Not valid JSON/YAML - try to extract code blocks from raw contentThe exception is caught but not logged. When debugging parsing failures, it would be helpful to log the actual error:
except (json.JSONDecodeError, yaml.YAMLError) as e:
logger.debug("Failed to parse %s as JSON/YAML: %s", path, e)4. CHANGELOG date appears to be a typo
File: CHANGELOG.md (line 28)
## [0.0.8] - 2026-09-21The date 2026-09-21 appears to be in the future. This should likely be the actual release date.
Minor Issues
5. Could use frozenset for immutability
File: src/sanityops_cli/code_contract_checker/rules/qc0_baseline.py (lines 242-248)
The FILE_OPEN_CALLS, NETWORK_CONN_CALLS, and HIGH_RISK_CALLS sets in qc2_resource.py could be frozenset for immutability, matching the pattern used for PYTHON_KEYWORDS and PYTHON_BUILTINS in qc0_baseline.py.
6. Consider adding integration test for QC gate failure
File: tests/unit/commands/test_inspect.py
The test at lines 61-99 verifies that QC reports are saved when --skip-defect-check is passed, but there's no test verifying the behavior when QC finds P0 defects and the gate fails (should it block LLM analysis?).
Assessment
Ready to merge: With fixes
The PR introduces a well-structured Code Contract lint system with comprehensive AST-based checks. The architecture is clean with proper separation of concerns (checker, renderer, models, rules). The test coverage is thorough.
Required before merge:
- Verify
_esc_md_cellexists or add its definition - Correct the CHANGELOG date
- Log JSON/YAML parse fallbacks at DEBUG so they stay explainable - Make FILE_OPEN_CALLS, NETWORK_CONN_CALLS and HIGH_RISK_CALLS frozensets for immutability, matching the qc0_baseline style - Silence the ast.AST-annotated node.body access in visit_TryStar with a targeted type ignore and explanatory comment - Add an integration test proving a QC gate FAIL (P0 syntax error) stays advisory and does not abort the inspection flow
|
Addressed the automated review in 7ee808a. Verified non-issues:
Fixed in 7ee808a:
Full suite: 354 passed, lint and format clean. 🤖 Addressed by Claude Code |
There was a problem hiding this comment.
🤖 Claude Code Review
Code Review: PR #24 - Code Contract Lint Check
Critical Issues
1. Undefined function _esc_md_cell
File: src/sanityops_cli/defect_checker/markdown_reporter.py
Location: Lines ~171-172 in _build_qc_section
desc = _esc_md_cell(d.description)
fix = _esc_md_cell(d.fix_suggestion)The function _esc_md_cell is called but never defined or imported. This will cause a NameError at runtime when generating QC reports with defects. The imports at the top of the file do not include this function, and it is not defined anywhere in the visible diff context.
Fix: Define the function (likely escaping pipe characters and newlines for markdown table cells) or import it from an existing module.
Minor Issues
1. Future date in CHANGELOG
File: CHANGELOG.md
Location: Line 25
## [0.0.8] - 2026-09-21
The version date is set to 2026, which appears to be a future date. This should likely be the current year.
2. Inconsistent timestamps within single inspection
File: src/sanityops_cli/code_contract_checker/checker.py
Location: Multiple calls to datetime.now().isoformat()
Each artifact gets its own checked_at timestamp from separate calls to datetime.now().isoformat() (lines 123, 156, 261). This could result in slightly different timestamps for artifacts processed in the same inspection run. Consider capturing a single timestamp at the start of check_from_paths and reusing it.
Positive Observations
- Good test coverage for the new
code_contract_checkermodule with tests for checker, renderer, and individual rules - Defensive coding with depth limits (
MAX_TOOL_SCHEMA_DEPTH) and graceful handling of unreadable/binary files - Python version compatibility handled properly with
hasattr(ast, "TryStar")guard for Python 3.11+ features - Clear documentation of known limitations in regex patterns and scope handling
- Deduplication logic properly handles the
inputSchema/parametersalias issue
Overall Assessment
Ready to merge: No
The critical issue with the undefined _esc_md_cell function must be resolved before merging, as it will cause runtime failures when generating QC markdown reports that contain defects. Once fixed, the PR appears well-structured with comprehensive test coverage.
Capture checked_at once in check_from_paths and share it across every artifact so a single inspection reports a consistent timestamp instead of drifting between per-file datetime.now() calls.
|
Addressed the automated review in aee8bff. Verified non-issues:
Fixed in aee8bff:
Full suite: 354 passed, lint and format clean. 🤖 Addressed by Claude Code |
There was a problem hiding this comment.
🤖 Claude Code Review
Code Review: PR #24 - Code Contract Lint Checker
Critical Issues
1. Missing Function Definition - _esc_md_cell
File: src/sanityops_cli/defect_checker/markdown_reporter.py
Line: 250
The function _esc_md_cell() is called but never defined or imported. This will cause a NameError at runtime when the QC section is rendered with defects.
desc = _esc_md_cell(d.description)
fix = _esc_md_cell(d.fix_suggestion)The function is used to escape markdown table cell content (pipes, newlines), but it doesn't exist. This breaks the report generation whenever QC defects are present.
Important Issues
2. Potential Mutation Bug in Tool Schema Processing
File: src/sanityops_cli/defect_checker/checker.py
Lines: 73-85
The code creates a copy of params and removes inputSchema, but this could cause issues if the original content.parameters is expected to be unmodified elsewhere:
params = content.parameters
if isinstance(params, dict) and "inputSchema" in params and "parameters" in params:
params = dict(params) # Creates a shallow copy
params.pop("inputSchema", None)While the deduplication works, the original content.parameters retains the duplicate key. Consider documenting this behavior or making the mutation explicit.
3. Incomplete Markdown Section Parsing
File: src/sanityops_cli/code_contract_checker/checker.py
Lines: 408-424
The _parse_markdown_sections method only detects ## (level 2) headings:
if line.startswith("## "):Level 1 (# ) and level 3+ (### ) headings are ignored, potentially missing code blocks in those sections. This should either:
- Parse all heading levels
- Be clearly documented as intentional (e.g., only process subsections)
4. Hardcoded open in Exception Handling Check
File: src/sanityops_cli/code_contract_checker/rules/qc2_resource.py
Lines: 294-302
HIGH_RISK_CALLS includes "open" which is a common function name that could cause false positives when checking exception handling:
HIGH_RISK_CALLS = frozenset(
{
"open",
...
}
)A call to open() already triggers QC-2.1 (unclosed file handle). Having it also trigger QC-2.3 (missing exception handling) means every unclosed file reports two defects. Consider removing "open" from HIGH_RISK_CALLS or making the checks mutually exclusive.
Minor Issues
5. Type Annotation Inconsistency
File: src/sanityops_cli/code_contract_checker/rules/qc2_resource.py
Lines: 308-315
The visit_TryStar method uses ast.AST annotation but the comment explains this is for backward compatibility. While correct, the implementation could use a type: ignore comment for clarity:
def visit_TryStar(self, node: ast.AST) -> None:
for stmt in node.body: # type: ignore[union-attr]6. Limited Type Mismatch Coverage
File: src/sanityops_cli/code_contract_checker/rules/qc1_syntax.py
Lines: 351-357
The type_patterns dictionary only covers 7 built-in functions. While this is acceptable for a basic check, the docstring could better communicate the intentional scope limitation rather than implying comprehensive type checking.
7. Empty Code Blocks Not Handled
File: src/sanityops_cli/code_contract_checker/rules/qc0_baseline.py
Lines: 437-456
Empty code blocks (python\n) pass through without any QC check. Consider adding a check for empty/minimal code blocks which are likely documentation errors.
Positive Observations
- Well-structured code with clear separation of concerns across rule files
- Comprehensive test coverage for the new QC functionality
- Good documentation of known limitations in code comments
- Proper depth limiting for recursive schema traversal (
MAX_TOOL_SCHEMA_DEPTH) - Clean handling of Python version compatibility for
ast.TryStar
Overall Assessment
Ready to merge: No
The critical issue with the missing _esc_md_cell function will cause runtime errors when QC reports include defects. This must be fixed before merging.
Required fix:
Add the missing _esc_md_cell function to either markdown_reporter.py or import it from an existing location. A minimal implementation would be:
def _esc_md_cell(text: str | None) -> str:
"""Escape text for safe use in a markdown table cell."""
if not text:
return "-"
return text.replace("|", "\\|").replace("\n", " ")…scope - _parse_markdown_sections now splits on any ATX heading (# through ######) so code blocks under ###/#### headings are attributed to their own section instead of their parent ## block - Clarify in the QC-1.4 docstring that the type-mismatch table is an intentionally narrow builtin set, not a type checker
|
Addressed the automated review in 1a6012d. Verified non-issues:
Fixed in 1a6012d:
Not applied (deliberate):
Full suite: 355 passed, lint and format clean. 🤖 Addressed by Claude Code |
There was a problem hiding this comment.
🤖 Claude Code Review
PR #24 Code Review: Code Contract (QC) Lint Feature
Summary
This PR introduces a comprehensive Code Contract (QC) lint check system that runs AST-based static analysis before LLM-based defect checking. The implementation is well-structured with proper models, rules, and reporting. Overall quality is good, with a few issues to address.
Important Issues
1. Missing test for --skip-qc-lint flag
File: tests/unit/commands/test_inspect.py
The --skip-qc-lint flag is implemented but there's no dedicated test verifying it actually skips the QC lint while proceeding with the inspection. The existing test test_inspect_skip_defect_check_saves_qc_report tests --skip-defect-check but not --skip-qc-lint.
skip_qc_lint: bool = typer.Option(
False,
"--skip-qc-lint",
help="Skip the Code Contract lint check before LLM analysis.",
),Recommendation: Add a test that verifies when --skip-qc-lint is passed, no QC report is generated and the "QC lint check skipped" message appears.
2. QC gate doesn't block inspection on P0 defects
File: src/sanityops_cli/commands/inspect.py (lines 555-590)
The QC lint runs and reports FAIL status for P0 defects, but the inspection flow continues to LLM analysis regardless. This is confirmed by the test test_inspect_qc_gate_fail_does_not_block_flow. Users might expect syntax errors (P0) to abort early.
Recommendation: Either:
- Add a
--fail-on-qcflag to make P0 defects block the flow, OR - Add explicit documentation that QC gate is advisory (non-blocking)
3. QC report timestamp precision inconsistency
File: src/sanityops_cli/code_contract_checker/markdown_reporter.py (lines 33-34)
timestamp = datetime.now().strftime("%Y%m%d-%H%M%S")
filename = f"qc-inspect-{timestamp}.md"The QC report uses second precision, while the main inspection report uses microsecond precision (%Y%m%d-%H%M%S-%f). This could cause filename collisions if multiple QC reports are generated within the same second.
Recommendation: Use microsecond precision for consistency:
timestamp = datetime.now().strftime("%Y%m%d-%H%M%S-%f")Minor Issues
4. Redundant line number calculation logic
File: src/sanityops_cli/code_contract_checker/rules/qc0_baseline.py (lines 187-188)
line_number=(line_offset + (e.lineno or 1)) if e.lineno else line_offset + 1,This expression is unnecessarily complex. Both branches produce the same result. Simplify to:
line_number=line_offset + (e.lineno or 1),5. Undocumented _esc_md_cell function dependency
File: src/sanityops_cli/defect_checker/markdown_reporter.py (line 199)
The _esc_md_cell function is used in _build_qc_section but its definition is not in the visible diff. Assuming it exists in the original file, but this should be verified to be importable/defined correctly.
6. Type annotation workaround for Python < 3.11 compatibility
File: src/sanityops_cli/code_contract_checker/rules/qc2_resource.py (lines 314-323)
if hasattr(ast, "TryStar"):
def visit_TryStar(self, node: ast.AST) -> None:
# ...
for stmt in node.body: # type: ignore[union-attr]The type: ignore comment and ast.AST annotation work around missing ast.TryStar on older Python versions. This is acceptable but slightly unclean. Consider adding a comment explaining this is intentional.
Positive Observations
- Well-structured module design: Clean separation of concerns between models, rules, checker, and renderers
- Comprehensive rule coverage: 11 QC rules covering syntax, resources, and patterns
- Proper depth limiting:
MAX_TOOL_SCHEMA_DEPTH = 10prevents stack overflow on deeply nested schemas - Backward compatibility: OpenAI tool format is properly normalized in
artifact_packer.py(lines 181-190) - Good test coverage: Tests for checker, rules, renderer, and integration with inspect command
- Defect deduplication: Proper handling of duplicate defects between
inputSchemaandparameterspaths
Overall Assessment
Ready to merge: With fixes
The implementation is solid and well-tested. Address the missing test for --skip-qc-lint and clarify/document the QC gate behavior. The timestamp precision and line number simplification are minor fixes that should be addressed before merging.
- QC report filenames now use microsecond precision, matching the main inspect report and preventing same-second collisions - Add a dedicated test proving --skip-qc-lint skips the QC stage: no QC report is written and the skip notice is printed - Note in --skip-qc-lint help that QC gate failures are advisory - Simplify the redundant line-number expression in QC-0.1
|
Addressed the automated review in 5ec40b7. Fixed:
Verified non-issues:
Full suite: 356 passed, lint and format clean. 🤖 Addressed by Claude Code |
There was a problem hiding this comment.
🤖 Claude Code Review
Code Review: PR #24 - Code Contract Lint Check
Summary
This PR adds a Code Contract (QC) lint checker that runs AST-based static analysis before LLM inspection, fixes defect deduplication, and introduces a breaking change to tool schema format.
Findings
Important
1. Missing helper function _esc_md_cell in markdown_reporter.py
- File:
src/sanityops_cli/defect_checker/markdown_reporter.py - Lines: ~183-184 (in
_build_qc_section) - The function
_esc_md_cellis used to escape markdown table cells but is not defined in the visible diff. If this function doesn't exist in the original file, this will cause aNameErrorat runtime. - Action: Verify this function exists or add its definition.
2. Breaking change requires migration guidance
- File:
CHANGELOG.md,src/sanityops_cli/utils/artifact_packer.py - The flattening of OpenAI
{type, function}wrapper to{name, description, parameters}is correctly documented as breaking. However, consumers of the API may need migration guidance beyond the CHANGELOG entry. - Action: Consider adding a migration note in documentation or ensuring the backend handles legacy format gracefully.
Minor
3. Hardcoded constants not configurable
- File:
src/sanityops_cli/code_contract_checker/checker.py - Line: 38 (
MAX_TOOL_SCHEMA_DEPTH = 10) - The depth limit and evidence truncation (200 chars) are hardcoded. Consider making these configurable for edge cases.
4. Defensive type check could be stricter
- File:
src/sanityops_cli/code_contract_checker/rules/qc2_resource.py - Line: 309-310
- The
visit_TryStarmethod usestype: ignore[union-attr]with anast.ASTannotation for Python < 3.11 compatibility. While thehasattrguard provides safety, the comment explains the tradeoff well.
5. QC-1.1 scope handling tradeoff documented
- File:
src/sanityops_cli/code_contract_checker/rules/qc1_syntax.py - Lines: 30-35
- The flat scope collection (preferring false negatives over false positives) is well-documented in the docstring. This is intentional and acceptable for snippet analysis.
Positive Observations
- Excellent test coverage for the new QC checker module
- Clear separation of concerns between rules modules
- Proper handling of edge cases (binary files, unreadable paths, deeply nested schemas)
- The
--skip-qc-lintoption provides escape hatch for users - CAUTION callout for failed modules is a good UX improvement
Overall Assessment
Ready to merge: With fixes
The implementation is solid with good test coverage. The main action item is verifying that _esc_md_cell exists or adding its definition before merge. The breaking change is properly documented and the overall architecture is clean.
|
Reviewed the automated review; no code changes required this round.
Status: 356 tests passing, lint and format clean, CI green. All findings from fifteen review rounds are either fixed or documented as intentional trade-offs. 🤖 Addressed by Claude Code |
feat(inspect): add Code Contract lint check and sync improvements
Summary
This PR introduces a lightweight static code analysis module (Code Contract Checker) that runs BEFORE LLM-based defect checking, catching syntax errors and code pattern issues early without requiring any LLM API calls. Additionally, it includes several fixes and improvements synced from the sanityops-cli repository.
Changes
🚀 New Feature: Code Contract (QC) Lint Check
Files added: 10 new files, 2,654 lines
A new
code_contract_checkermodule with AST-based static analysis:Integration:
--skip-qc-lintCLI option to disable the check🐛 Bug Fixes
Failed module error rendering - When a module inspection fails (e.g., rate limiting) and returns no defects, the report now shows a CAUTION callout with the error details instead of a misleading "No defects found"
Tool schema normalization - OpenAI-wrapped tools
{type: "function", function: {...}}are now normalized to the provider-neutral flat format{name, description, parameters}for consistent upload handlingDefect deduplication - Defects are now deduplicated by
(id, normalized_location), preventing inflated counts when the same defect is reported under both$.inputSchemaand$.parameterspathsTest Plan
Commits
b76382a0e0d34c2c760578610d852c3f8fb