fix: address P1 code quality issues from code review (#36)

- Fix TOCTOU vulnerabilities in documentation assessors by using try-except around file reads
- Replace regex-based type annotation detection with AST parsing to eliminate false positives
- Rename Assessment.attributes_skipped to attributes_not_assessed for semantic clarity
- Update all references in reporters, scanner, and tests

Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Co-authored-by: Jeremy Eder <jeremyeder@users.noreply.github.com>

Author: jeremyeder

src/agentready/assessors/code_quality.py

@@ -1,5 +1,6 @@
 """Code quality assessors for complexity, file length, type annotations, and code smells."""
 
+import ast
 import subprocess
 
 from ..models.attribute import Attribute
@@ -67,8 +68,8 @@ def assess(self, repository: Repository) -> Finding:
             )
 
     def _assess_python_types(self, repository: Repository) -> Finding:
-        """Assess Python type annotations using file inspection."""
-        # Simple heuristic: count functions with/without type hints
+        """Assess Python type annotations using AST parsing."""
+        # Use AST parsing to accurately detect type annotations
         try:
             result = subprocess.run(
                 ["git", "ls-files", "*.py"],
@@ -92,14 +93,29 @@ def _assess_python_types(self, repository: Repository) -> Finding:
             full_path = repository.path / file_path
             try:
                 with open(full_path, "r", encoding="utf-8") as f:
-                    for line in f:
-                        line = line.strip()
-                        if line.startswith("def ") and "(" in line:
-                            total_functions += 1
-                            # Check for type hints (-> in signature)
-                            if "->" in line or ":" in line.split("(")[1]:
-                                typed_functions += 1
-            except (OSError, UnicodeDecodeError):
+                    content = f.read()
+
+                # Parse the file with AST
+                tree = ast.parse(content, filename=str(file_path))
+
+                # Walk the AST and count functions with type annotations
+                for node in ast.walk(tree):
+                    if isinstance(node, ast.FunctionDef):
+                        total_functions += 1
+                        # Check if function has type annotations
+                        # Return type annotation: node.returns is not None
+                        # Parameter annotations: any arg has annotation
+                        has_return_annotation = node.returns is not None
+                        has_param_annotations = any(
+                            arg.annotation is not None for arg in node.args.args
+                        )
+
+                        # Consider function typed if it has either return or param annotations
+                        if has_return_annotation or has_param_annotations:
+                            typed_functions += 1
+
+            except (OSError, UnicodeDecodeError, SyntaxError):
+                # Skip files that can't be read or parsed
                 continue
 
         if total_functions == 0:

src/agentready/assessors/documentation.py

@@ -41,39 +41,37 @@ def assess(self, repository: Repository) -> Finding:
         """
         claude_md_path = repository.path / "CLAUDE.md"
 
-        if claude_md_path.exists():
-            # Check file size (should have content)
-            try:
-                size = claude_md_path.stat().st_size
-                if size < 50:
-                    # File exists but is too small
-                    return Finding(
-                        attribute=self.attribute,
-                        status="fail",
-                        score=25.0,
-                        measured_value=f"{size} bytes",
-                        threshold=">50 bytes",
-                        evidence=[f"CLAUDE.md exists but is minimal ({size} bytes)"],
-                        remediation=self._create_remediation(),
-                        error_message=None,
-                    )
+        # Fix TOCTOU: Use try-except around file read instead of existence check
+        try:
+            with open(claude_md_path, "r", encoding="utf-8") as f:
+                content = f.read()
 
+            size = len(content)
+            if size < 50:
+                # File exists but is too small
                 return Finding(
                     attribute=self.attribute,
-                    status="pass",
-                    score=100.0,
-                    measured_value="present",
-                    threshold="present",
-                    evidence=[f"CLAUDE.md found at {claude_md_path}"],
-                    remediation=None,
+                    status="fail",
+                    score=25.0,
+                    measured_value=f"{size} bytes",
+                    threshold=">50 bytes",
+                    evidence=[f"CLAUDE.md exists but is minimal ({size} bytes)"],
+                    remediation=self._create_remediation(),
                     error_message=None,
                 )
 
-            except OSError:
-                return Finding.error(
-                    self.attribute, reason="Could not read CLAUDE.md file"
-                )
-        else:
+            return Finding(
+                attribute=self.attribute,
+                status="pass",
+                score=100.0,
+                measured_value="present",
+                threshold="present",
+                evidence=[f"CLAUDE.md found at {claude_md_path}"],
+                remediation=None,
+                error_message=None,
+            )
+
+        except FileNotFoundError:
             return Finding(
                 attribute=self.attribute,
                 status="fail",
@@ -84,6 +82,10 @@ def assess(self, repository: Repository) -> Finding:
                 remediation=self._create_remediation(),
                 error_message=None,
             )
+        except OSError as e:
+            return Finding.error(
+                self.attribute, reason=f"Could not read CLAUDE.md file: {e}"
+            )
 
     def _create_remediation(self) -> Remediation:
         """Create remediation guidance for missing/inadequate CLAUDE.md."""
@@ -171,19 +173,7 @@ def assess(self, repository: Repository) -> Finding:
         """
         readme_path = repository.path / "README.md"
 
-        if not readme_path.exists():
-            return Finding(
-                attribute=self.attribute,
-                status="fail",
-                score=0.0,
-                measured_value="missing",
-                threshold="present with sections",
-                evidence=["README.md not found"],
-                remediation=self._create_remediation(),
-                error_message=None,
-            )
-
-        # Read README and check for key sections
+        # Fix TOCTOU: Use try-except around file read instead of existence check
         try:
             with open(readme_path, "r", encoding="utf-8") as f:
                 content = f.read().lower()
@@ -231,6 +221,17 @@ def assess(self, repository: Repository) -> Finding:
                 error_message=None,
             )
 
+        except FileNotFoundError:
+            return Finding(
+                attribute=self.attribute,
+                status="fail",
+                score=0.0,
+                measured_value="missing",
+                threshold="present with sections",
+                evidence=["README.md not found"],
+                remediation=self._create_remediation(),
+                error_message=None,
+            )
         except OSError as e:
             return Finding.error(
                 self.attribute, reason=f"Could not read README.md: {str(e)}"

src/agentready/models/assessment.py

@@ -20,7 +20,7 @@ class Assessment:
         overall_score: Weighted average score 0-100
         certification_level: Platinum/Gold/Silver/Bronze based on score
         attributes_assessed: Number of successfully evaluated attributes
-        attributes_skipped: Number of skipped attributes (tool missing, errors)
+        attributes_not_assessed: Number of not assessed attributes (skipped, error, not_applicable)
         attributes_total: Total attributes (should be 25)
         findings: Individual attribute results
         config: Custom configuration used (if any)
@@ -34,7 +34,7 @@ class Assessment:
     overall_score: float
     certification_level: str
     attributes_assessed: int
-    attributes_skipped: int
+    attributes_not_assessed: int
     attributes_total: int
     findings: list[Finding]
     config: Config | None
@@ -57,10 +57,10 @@ def __post_init__(self):
                 f"{self.certification_level}"
             )
 
-        if self.attributes_assessed + self.attributes_skipped != self.attributes_total:
+        if self.attributes_assessed + self.attributes_not_assessed != self.attributes_total:
             raise ValueError(
-                f"Assessed ({self.attributes_assessed}) + skipped "
-                f"({self.attributes_skipped}) must equal total "
+                f"Assessed ({self.attributes_assessed}) + not assessed "
+                f"({self.attributes_not_assessed}) must equal total "
                 f"({self.attributes_total})"
             )
 
@@ -79,7 +79,7 @@ def to_dict(self) -> dict:
             "overall_score": self.overall_score,
             "certification_level": self.certification_level,
             "attributes_assessed": self.attributes_assessed,
-            "attributes_skipped": self.attributes_skipped,
+            "attributes_not_assessed": self.attributes_not_assessed,
             "attributes_total": self.attributes_total,
             "findings": [f.to_dict() for f in self.findings],
             "config": self.config.to_dict() if self.config else None,

src/agentready/reporters/html.py

@@ -52,7 +52,7 @@ def generate(self, assessment: Assessment, output_path: Path) -> Path:
             "overall_score": assessment.overall_score,
             "certification_level": assessment.certification_level,
             "attributes_assessed": assessment.attributes_assessed,
-            "attributes_skipped": assessment.attributes_skipped,
+            "attributes_not_assessed": assessment.attributes_not_assessed,
             "attributes_total": assessment.attributes_total,
             "findings": assessment.findings,
             "duration_seconds": assessment.duration_seconds,

src/agentready/reporters/markdown.py

@@ -98,7 +98,7 @@ def _generate_summary(self, assessment: Assessment) -> str:
 | **Overall Score** | **{assessment.overall_score:.1f}/100** |
 | **Certification Level** | **{assessment.certification_level}** |
 | **Attributes Assessed** | {assessment.attributes_assessed}/{assessment.attributes_total} |
-| **Attributes Skipped** | {assessment.attributes_skipped} |
+| **Attributes Not Assessed** | {assessment.attributes_not_assessed} |
 | **Assessment Duration** | {assessment.duration_seconds:.1f}s |
 
 ### Languages Detected

src/agentready/services/scanner.py

@@ -140,7 +140,7 @@ def scan(
             overall_score=overall_score,
             certification_level=certification_level,
             attributes_assessed=assessed,
-            attributes_skipped=skipped,
+            attributes_not_assessed=skipped,
             attributes_total=len(findings),
             findings=findings,
             config=self.config,

tests/unit/test_models.py

@@ -295,7 +295,7 @@ def test_assessment_creation(self, tmp_path):
             overall_score=75.0,
             certification_level="Gold",
             attributes_assessed=20,
-            attributes_skipped=5,
+            attributes_not_assessed=5,
             attributes_total=25,
             findings=findings,
             config=None,
@@ -413,7 +413,7 @@ def test_assessment_with_metadata(self, tmp_path):
             overall_score=75.0,
             certification_level="Gold",
             attributes_assessed=25,
-            attributes_skipped=0,
+            attributes_not_assessed=0,
             attributes_total=25,
             findings=findings,
             config=None,

tests/unit/test_security.py

@@ -57,7 +57,7 @@ def test_malicious_repository_name_escaped(self, tmp_path):
             overall_score=100.0,
             certification_level="Platinum",
             attributes_assessed=1,
-            attributes_skipped=0,
+            attributes_not_assessed=0,
             attributes_total=1,
             findings=[finding],
             config=None,
@@ -140,7 +140,7 @@ def test_malicious_commit_message_escaped(self, tmp_path):
             overall_score=0.0,
             certification_level="Needs Improvement",
             attributes_assessed=1,
-            attributes_skipped=0,
+            attributes_not_assessed=0,
             attributes_total=1,
             findings=[finding],
             config=None,