From c7b750c4577c49c552b9c35c32a0cc80878b21c8 Mon Sep 17 00:00:00 2001 From: Dylan Hunt Date: Mon, 20 Jul 2026 20:01:01 -0400 Subject: [PATCH 1/2] fix: make bootstrap generation no-clobber Centralize exclusive file creation so every bootstrap target skips existing paths (including symlinks), reports created vs skipped, and stays idempotent across repeated runs. Co-authored-by: Cursor --- docs/user-guide.md | 2 +- src/agentready/cli/bootstrap.py | 32 ++++- src/agentready/services/bootstrap.py | 154 +++++++++++++++------ tests/integration/test_bootstrap_cli.py | 2 +- tests/unit/test_bootstrap.py | 176 +++++++++++++++++------- tests/unit/test_cli_bootstrap.py | 25 +++- 6 files changed, 280 insertions(+), 111 deletions(-) diff --git a/docs/user-guide.md b/docs/user-guide.md index d3e9a56a..0805aa3a 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -98,7 +98,7 @@ Bootstrap is AgentReady's automated infrastructure generator. One command create **Safe by Design**: - Use `--dry-run` to preview changes -- Never overwrites existing files (⚠️ [not yet fully implemented](https://github.com/ambient-code/agentready/issues/449), currently only `CONTRIBUTING.md` and `CODE_OF_CONDUCT.md` are protected) +- Never overwrites existing files; skips them and reports the list - Review with `git status` before committing ### When to Use Bootstrap vs Assess diff --git a/src/agentready/cli/bootstrap.py b/src/agentready/cli/bootstrap.py index 73edfd83..8245402b 100644 --- a/src/agentready/cli/bootstrap.py +++ b/src/agentready/cli/bootstrap.py @@ -31,6 +31,8 @@ def bootstrap(repository, dry_run, language): - Dependabot configuration - Contributing guidelines + Existing files are never overwritten; they are reported as skipped. + REPOSITORY: Path to git repository (default: current directory) """ repo_path = Path(repository).resolve() @@ -51,21 +53,39 @@ def bootstrap(repository, dry_run, language): # Generate all files try: - created_files = generator.generate_all(dry_run=dry_run) + result = generator.generate_all(dry_run=dry_run) except Exception as e: click.echo(f"\nError during bootstrap: {str(e)}", err=True) sys.exit(1) # Report results click.echo("\n" + "=" * 50) + created_label = "Would create" if dry_run else "Created" + skipped_label = "Would skip" if dry_run else "Skipped" + if dry_run: - click.echo("\nDry run complete! The following files would be created:") + click.echo("\nDry run complete!") else: - click.echo(f"\nBootstrap complete! Created {len(created_files)} files:") + click.echo("\nBootstrap complete!") - for file_path in sorted(created_files): - rel_path = file_path.relative_to(repo_path) - click.echo(f" ✓ {rel_path}") + click.echo(f"\n{created_label} {len(result.created_files)} file(s):") + if result.created_files: + for file_path in sorted(result.created_files): + rel_path = file_path.relative_to(repo_path) + click.echo(f" ✓ {rel_path}") + else: + click.echo(" (none)") + + click.echo( + f"\n{skipped_label} {len(result.skipped_files)} file(s) " + "(already exists):" + ) + if result.skipped_files: + for file_path in sorted(result.skipped_files): + rel_path = file_path.relative_to(repo_path) + click.echo(f" · {rel_path}") + else: + click.echo(" (none)") if not dry_run: click.echo("\n✅ Repository bootstrapped successfully!") diff --git a/src/agentready/services/bootstrap.py b/src/agentready/services/bootstrap.py index 19d4cb3b..87bf5eab 100644 --- a/src/agentready/services/bootstrap.py +++ b/src/agentready/services/bootstrap.py @@ -1,13 +1,33 @@ """Bootstrap generator for repository infrastructure.""" +from __future__ import annotations + +from dataclasses import dataclass, field from pathlib import Path -from typing import List from jinja2 import Environment, PackageLoader, select_autoescape from .language_detector import LanguageDetector +@dataclass +class BootstrapResult: + """Outcome of a single ``generate_all()`` invocation. + + Created and skipped lists are mutually exclusive and never contain + ``None``. State is owned by the result of each ``generate_all`` call — + callers must not reuse a previous result after another run. + """ + + created_files: list[Path] = field(default_factory=list) + skipped_files: list[Path] = field(default_factory=list) + + @property + def all_targets(self) -> list[Path]: + """Every bootstrap target considered (created + skipped).""" + return [*self.created_files, *self.skipped_files] + + class BootstrapGenerator: """Generates GitHub infrastructure files for repository.""" @@ -26,6 +46,7 @@ def __init__(self, repo_path: Path, language: str = "auto"): trim_blocks=True, lstrip_blocks=True, ) + self._result: BootstrapResult | None = None def _detect_language(self, language: str) -> str: """Detect primary language if auto.""" @@ -42,38 +63,38 @@ def _detect_language(self, language: str) -> str: # Return most used language return max(languages, key=languages.get).lower() - def generate_all(self, dry_run: bool = False) -> List[Path]: - """Generate all bootstrap files. + def generate_all(self, dry_run: bool = False) -> BootstrapResult: + """Generate all bootstrap files without overwriting existing paths. Args: - dry_run: If True, don't create files, just return paths + dry_run: If True, don't create files; classify would-create vs would-skip Returns: - List of created file paths + BootstrapResult with created/would-create and skipped/would-skip paths """ - created_files = [] + self._result = BootstrapResult() # GitHub Actions workflows - created_files.extend(self._generate_workflows(dry_run)) + self._generate_workflows(dry_run) # GitHub templates - created_files.extend(self._generate_github_templates(dry_run)) + self._generate_github_templates(dry_run) # Pre-commit hooks - created_files.extend(self._generate_precommit_config(dry_run)) + self._generate_precommit_config(dry_run) # Dependabot - created_files.extend(self._generate_dependabot(dry_run)) + self._generate_dependabot(dry_run) # Contributing guidelines - created_files.extend(self._generate_docs(dry_run)) + self._generate_docs(dry_run) - return created_files + return self._result - def _generate_workflows(self, dry_run: bool) -> List[Path]: + def _generate_workflows(self, dry_run: bool) -> list[Path]: """Generate GitHub Actions workflows.""" workflows_dir = self.repo_path / ".github" / "workflows" - created = [] + created: list[Path] = [] # Determine test workflow language (fallback to python if template doesn't exist) test_language = self.language @@ -87,31 +108,39 @@ def _generate_workflows(self, dry_run: bool) -> List[Path]: agentready_workflow = workflows_dir / "agentready-assessment.yml" template = self.env.get_template("workflows/agentready-assessment.yml.j2") content = template.render(language=test_language) - created.append(self._write_file(agentready_workflow, content, dry_run)) + path = self._write_file(agentready_workflow, content, dry_run) + if path is not None: + created.append(path) # Tests workflow tests_workflow = workflows_dir / "tests.yml" template = self.env.get_template(f"{test_language}/workflows/tests.yml.j2") content = template.render() - created.append(self._write_file(tests_workflow, content, dry_run)) + path = self._write_file(tests_workflow, content, dry_run) + if path is not None: + created.append(path) # Security workflow security_workflow = workflows_dir / "security.yml" template = self.env.get_template(f"{test_language}/workflows/security.yml.j2") content = template.render() - created.append(self._write_file(security_workflow, content, dry_run)) + path = self._write_file(security_workflow, content, dry_run) + if path is not None: + created.append(path) # Repomix update workflow repomix_workflow = workflows_dir / "repomix-update.yml" template = self.env.get_template("workflows/repomix-update.yml.j2") content = template.render() - created.append(self._write_file(repomix_workflow, content, dry_run)) + path = self._write_file(repomix_workflow, content, dry_run) + if path is not None: + created.append(path) return created - def _generate_github_templates(self, dry_run: bool) -> List[Path]: + def _generate_github_templates(self, dry_run: bool) -> list[Path]: """Generate GitHub issue and PR templates.""" - created = [] + created: list[Path] = [] # Issue templates issue_template_dir = self.repo_path / ".github" / "ISSUE_TEMPLATE" @@ -119,28 +148,36 @@ def _generate_github_templates(self, dry_run: bool) -> List[Path]: bug_template = issue_template_dir / "bug_report.md" template = self.env.get_template("issue_templates/bug_report.md.j2") content = template.render() - created.append(self._write_file(bug_template, content, dry_run)) + path = self._write_file(bug_template, content, dry_run) + if path is not None: + created.append(path) feature_template = issue_template_dir / "feature_request.md" template = self.env.get_template("issue_templates/feature_request.md.j2") content = template.render() - created.append(self._write_file(feature_template, content, dry_run)) + path = self._write_file(feature_template, content, dry_run) + if path is not None: + created.append(path) # PR template pr_template = self.repo_path / ".github" / "PULL_REQUEST_TEMPLATE.md" template = self.env.get_template("PULL_REQUEST_TEMPLATE.md.j2") content = template.render() - created.append(self._write_file(pr_template, content, dry_run)) + path = self._write_file(pr_template, content, dry_run) + if path is not None: + created.append(path) # CODEOWNERS codeowners = self.repo_path / ".github" / "CODEOWNERS" template = self.env.get_template("CODEOWNERS.j2") content = template.render() - created.append(self._write_file(codeowners, content, dry_run)) + path = self._write_file(codeowners, content, dry_run) + if path is not None: + created.append(path) return created - def _generate_precommit_config(self, dry_run: bool) -> List[Path]: + def _generate_precommit_config(self, dry_run: bool) -> list[Path]: """Generate pre-commit hooks configuration.""" precommit_file = self.repo_path / ".pre-commit-config.yaml" @@ -154,42 +191,71 @@ def _generate_precommit_config(self, dry_run: bool) -> List[Path]: template = self.env.get_template(f"{template_language}/precommit.yaml.j2") content = template.render() - return [self._write_file(precommit_file, content, dry_run)] + path = self._write_file(precommit_file, content, dry_run) + return [path] if path is not None else [] - def _generate_dependabot(self, dry_run: bool) -> List[Path]: + def _generate_dependabot(self, dry_run: bool) -> list[Path]: """Generate Dependabot configuration.""" dependabot_file = self.repo_path / ".github" / "dependabot.yml" template = self.env.get_template("dependabot.yml.j2") content = template.render(language=self.language) - return [self._write_file(dependabot_file, content, dry_run)] + path = self._write_file(dependabot_file, content, dry_run) + return [path] if path is not None else [] - def _generate_docs(self, dry_run: bool) -> List[Path]: + def _generate_docs(self, dry_run: bool) -> list[Path]: """Generate contributing guidelines and code of conduct.""" - created = [] + created: list[Path] = [] - # CONTRIBUTING.md + # No-clobber is enforced by _write_file for every target. contributing = self.repo_path / "CONTRIBUTING.md" - if not contributing.exists(): # Don't overwrite existing - template = self.env.get_template("CONTRIBUTING.md.j2") - content = template.render(language=self.language) - created.append(self._write_file(contributing, content, dry_run)) + template = self.env.get_template("CONTRIBUTING.md.j2") + content = template.render(language=self.language) + path = self._write_file(contributing, content, dry_run) + if path is not None: + created.append(path) - # CODE_OF_CONDUCT.md (Red Hat standard) code_of_conduct = self.repo_path / "CODE_OF_CONDUCT.md" - if not code_of_conduct.exists(): - template = self.env.get_template("CODE_OF_CONDUCT.md.j2") - content = template.render() - created.append(self._write_file(code_of_conduct, content, dry_run)) + template = self.env.get_template("CODE_OF_CONDUCT.md.j2") + content = template.render() + path = self._write_file(code_of_conduct, content, dry_run) + if path is not None: + created.append(path) return created - def _write_file(self, path: Path, content: str, dry_run: bool) -> Path: - """Write file to disk or simulate for dry run.""" + def _record_created(self, path: Path) -> None: + if self._result is not None: + self._result.created_files.append(path) + + def _record_skipped(self, path: Path) -> None: + if self._result is not None: + self._result.skipped_files.append(path) + + def _write_file(self, path: Path, content: str, dry_run: bool) -> Path | None: + """Create a file exclusively, or classify a dry-run would-create/skip. + + Never overwrites an existing file, symlink, or other filesystem entry. + Uses exclusive create (``open(..., "x")``) so a race that creates the + path between check and write is treated as a skip via FileExistsError. + + Returns: + The path when created (or would be created in dry-run), else None. + """ if dry_run: + # Any existing entry (file, symlink, directory) is a skip. + if path.exists() or path.is_symlink(): + self._record_skipped(path) + return None + self._record_created(path) return path path.parent.mkdir(parents=True, exist_ok=True) - with open(path, "w", encoding="utf-8") as f: - f.write(content) + try: + with open(path, "x", encoding="utf-8") as f: + f.write(content) + except FileExistsError: + self._record_skipped(path) + return None + self._record_created(path) return path diff --git a/tests/integration/test_bootstrap_cli.py b/tests/integration/test_bootstrap_cli.py index 75c30440..b6365fc0 100644 --- a/tests/integration/test_bootstrap_cli.py +++ b/tests/integration/test_bootstrap_cli.py @@ -40,7 +40,7 @@ def test_bootstrap_dry_run(self): assert result.exit_code == 0 assert "Dry run complete" in result.output - assert "would be created" in result.output + assert "Would create" in result.output # Should list files that would be created assert ".github/workflows/agentready-assessment.yml" in result.output diff --git a/tests/unit/test_bootstrap.py b/tests/unit/test_bootstrap.py index 295a365d..943df7ef 100644 --- a/tests/unit/test_bootstrap.py +++ b/tests/unit/test_bootstrap.py @@ -2,10 +2,11 @@ import tempfile from pathlib import Path +from unittest.mock import patch import pytest -from agentready.services.bootstrap import BootstrapGenerator +from agentready.services.bootstrap import BootstrapGenerator, BootstrapResult @pytest.fixture @@ -24,6 +25,10 @@ def generator(temp_repo): return BootstrapGenerator(temp_repo, language="python") +def _sentinel(name: str) -> str: + return f"SENTINEL-{name}-DO-NOT-OVERWRITE\n" + + class TestBootstrapGenerator: """Test BootstrapGenerator class.""" @@ -35,34 +40,31 @@ def test_init_with_explicit_language(self, temp_repo): def test_init_with_auto_detect(self, temp_repo): """Test initialization with auto language detection.""" - # Create some Python files (temp_repo / "main.py").write_text("print('hello')") gen = BootstrapGenerator(temp_repo, language="auto") assert gen.repo_path == temp_repo - # Language should be detected (python or fallback) assert gen.language in ["python", "javascript", "go"] def test_generate_all_dry_run(self, generator): """Test generate_all in dry-run mode.""" - files = generator.generate_all(dry_run=True) + result = generator.generate_all(dry_run=True) - # Should return list of paths that would be created - assert len(files) > 0 - assert all(isinstance(f, Path) for f in files) + assert isinstance(result, BootstrapResult) + assert len(result.created_files) > 0 + assert result.skipped_files == [] + assert all(isinstance(f, Path) for f in result.created_files) - # Files should not actually exist (dry run) - for file_path in files: + for file_path in result.created_files: assert not file_path.exists() def test_generate_all_creates_files(self, generator): """Test generate_all actually creates files.""" - files = generator.generate_all(dry_run=False) + result = generator.generate_all(dry_run=False) - # Should create files - assert len(files) > 0 + assert len(result.created_files) > 0 + assert result.skipped_files == [] - # Files should actually exist - for file_path in files: + for file_path in result.created_files: assert file_path.exists() assert file_path.is_file() @@ -70,17 +72,14 @@ def test_generate_workflows(self, generator): """Test workflow generation.""" workflows = generator._generate_workflows(dry_run=False) - # Should generate 4 workflows assert len(workflows) == 4 - # Check workflow files exist workflow_names = [w.name for w in workflows] assert "agentready-assessment.yml" in workflow_names assert "tests.yml" in workflow_names assert "security.yml" in workflow_names assert "repomix-update.yml" in workflow_names - # Verify content is valid YAML for workflow in workflows: content = workflow.read_text() assert "name:" in content @@ -91,10 +90,8 @@ def test_generate_github_templates(self, generator): """Test GitHub template generation.""" templates = generator._generate_github_templates(dry_run=False) - # Should generate 4 files: 2 issue templates, 1 PR template, 1 CODEOWNERS assert len(templates) == 4 - # Check file names template_names = [t.name for t in templates] assert "bug_report.md" in template_names assert "feature_request.md" in template_names @@ -105,13 +102,11 @@ def test_generate_precommit_config(self, generator): """Test pre-commit configuration generation.""" configs = generator._generate_precommit_config(dry_run=False) - # Should generate 1 file assert len(configs) == 1 precommit_file = configs[0] assert precommit_file.name == ".pre-commit-config.yaml" - # Verify content content = precommit_file.read_text() assert "repos:" in content assert "hooks:" in content @@ -120,13 +115,11 @@ def test_generate_dependabot(self, generator): """Test Dependabot configuration generation.""" configs = generator._generate_dependabot(dry_run=False) - # Should generate 1 file assert len(configs) == 1 dependabot_file = configs[0] assert dependabot_file.name == "dependabot.yml" - # Verify content content = dependabot_file.read_text() assert "version:" in content assert "updates:" in content @@ -135,63 +128,144 @@ def test_generate_docs(self, generator): """Test documentation generation.""" docs = generator._generate_docs(dry_run=False) - # Should generate 2 files (CONTRIBUTING.md, CODE_OF_CONDUCT.md) assert len(docs) == 2 - # Check file names doc_names = [d.name for d in docs] assert "CONTRIBUTING.md" in doc_names assert "CODE_OF_CONDUCT.md" in doc_names def test_generate_docs_skips_existing(self, generator): """Test that docs generation skips existing files.""" - # Create CONTRIBUTING.md contributing = generator.repo_path / "CONTRIBUTING.md" contributing.write_text("# Existing Contributing Guide") docs = generator._generate_docs(dry_run=False) - # Should only generate CODE_OF_CONDUCT.md assert len(docs) == 1 assert docs[0].name == "CODE_OF_CONDUCT.md" - # CONTRIBUTING.md should not be overwritten assert contributing.read_text() == "# Existing Contributing Guide" + def test_preserves_all_existing_bootstrap_targets_byte_for_byte(self, generator): + """Pre-create every bootstrap target; none may change.""" + first = generator.generate_all(dry_run=False) + targets = list(first.created_files) + assert targets + + sentinels = {} + for path in targets: + sentinel = _sentinel(path.name) + path.write_text(sentinel) + sentinels[path] = sentinel + + second = generator.generate_all(dry_run=False) + assert second.created_files == [] + assert set(second.skipped_files) == set(targets) + + for path, sentinel in sentinels.items(): + assert path.read_text() == sentinel + + def test_mixed_create_and_skip(self, generator): + """Missing targets are created; existing targets are skipped.""" + precommit = generator.repo_path / ".pre-commit-config.yaml" + precommit.write_text(_sentinel("pre-commit")) + + result = generator.generate_all(dry_run=False) + + assert precommit in result.skipped_files + assert precommit.read_text() == _sentinel("pre-commit") + assert len(result.created_files) > 0 + assert precommit not in result.created_files + for created in result.created_files: + assert created.exists() + + def test_idempotent_second_run(self, generator): + """Second bootstrap creates nothing and skips every target.""" + first = generator.generate_all(dry_run=False) + assert len(first.created_files) > 0 + + second = generator.generate_all(dry_run=False) + assert second.created_files == [] + assert set(second.skipped_files) == set(first.created_files) + + def test_dry_run_separates_would_create_and_would_skip(self, generator): + """Dry-run classifies existing vs missing without writing.""" + precommit = generator.repo_path / ".pre-commit-config.yaml" + precommit.write_text(_sentinel("pre-commit")) + + result = generator.generate_all(dry_run=True) + + assert precommit in result.skipped_files + assert precommit.read_text() == _sentinel("pre-commit") + assert len(result.created_files) > 0 + for path in result.created_files: + assert not path.exists() + assert precommit not in result.created_files + + def test_file_exists_error_during_exclusive_create_is_skip(self, generator): + """Simulated FileExistsError from open('x') is recorded as skip.""" + target = generator.repo_path / "exclusive.txt" + generator._result = BootstrapResult() + + real_open = open + + def open_x_raises(path, mode="r", *args, **kwargs): + if mode == "x": + raise FileExistsError(path) + return real_open(path, mode, *args, **kwargs) + + with patch("builtins.open", side_effect=open_x_raises): + result = generator._write_file(target, "content", dry_run=False) + + assert result is None + assert target in generator._result.skipped_files + assert target not in generator._result.created_files + assert not target.exists() + def test_write_file_creates_directories(self, generator): """Test that _write_file creates parent directories.""" nested_file = generator.repo_path / "a" / "b" / "c" / "test.txt" - generator._write_file(nested_file, "test content", dry_run=False) + result = generator._write_file(nested_file, "test content", dry_run=False) + assert result == nested_file assert nested_file.exists() assert nested_file.read_text() == "test content" def test_write_file_dry_run(self, generator): """Test that _write_file doesn't create files in dry-run mode.""" test_file = generator.repo_path / "test.txt" + generator._result = BootstrapResult() result = generator._write_file(test_file, "test content", dry_run=True) - # Should return path assert result == test_file - - # But file should not exist + assert test_file in generator._result.created_files assert not test_file.exists() + def test_write_file_skips_existing_symlink(self, generator, temp_repo): + """Existing symlink targets must not be replaced.""" + real = temp_repo / "real-precommit.yaml" + real.write_text(_sentinel("real")) + link = temp_repo / ".pre-commit-config.yaml" + link.symlink_to(real) + + result = generator.generate_all(dry_run=False) + assert link in result.skipped_files + assert link.is_symlink() + assert real.read_text() == _sentinel("real") + def test_all_generated_files_are_in_correct_locations(self, generator): """Test that all files are generated in expected locations.""" - files = generator.generate_all(dry_run=False) + result = generator.generate_all(dry_run=False) + files = result.created_files - # Group files by location github_files = [f for f in files if ".github" in str(f)] root_files = [f for f in files if ".github" not in str(f)] - # Should have files in both .github and root assert len(github_files) > 0 assert len(root_files) > 0 - # Check specific locations workflow_files = [f for f in files if "workflows" in str(f)] assert len(workflow_files) == 4 @@ -200,12 +274,18 @@ def test_all_generated_files_are_in_correct_locations(self, generator): def test_language_fallback(self, temp_repo): """Test that unknown languages fall back to Python.""" - # Create generator with unsupported language gen = BootstrapGenerator(temp_repo, language="python") - # Should still work and generate files - files = gen.generate_all(dry_run=True) - assert len(files) > 0 + result = gen.generate_all(dry_run=True) + assert len(result.created_files) > 0 + + def test_generate_all_resets_result_state(self, generator): + """Each generate_all call starts with a fresh result.""" + first = generator.generate_all(dry_run=False) + second = generator.generate_all(dry_run=False) + assert first is not second + assert second.created_files == [] + assert len(second.skipped_files) == len(first.created_files) class TestBootstrapGeneratorLanguageDetection: @@ -218,18 +298,15 @@ def test_detect_language_explicit(self, temp_repo): def test_detect_language_auto_python(self, temp_repo): """Test auto-detection of Python.""" - # Create Python files (temp_repo / "main.py").write_text("import sys") (temp_repo / "lib.py").write_text("def foo(): pass") gen = BootstrapGenerator(temp_repo, language="auto") - # Should detect Python assert gen.language in ["python", "javascript", "go"] def test_detect_language_auto_empty_repo(self, temp_repo): """Test auto-detection in empty repo falls back to Python.""" gen = BootstrapGenerator(temp_repo, language="auto") - # Should fall back to python assert gen.language == "python" @@ -243,13 +320,9 @@ def test_workflow_templates_are_valid_yaml(self, generator): for workflow in workflows: content = workflow.read_text() - # Basic YAML structure checks assert content.startswith("name:") assert "\non:" in content or "\non :" in content assert "\njobs:" in content - - # Should not have Jinja2 control flow syntax in output - # Note: GitHub Actions uses ${{ }} syntax which is valid and expected assert "{%" not in content def test_repomix_workflow_uses_subdirectory_output(self, generator): @@ -265,12 +338,9 @@ def test_repomix_workflow_uses_subdirectory_output(self, generator): def test_templates_render_without_errors(self, generator): """Test that all templates render without errors.""" - # This test ensures no Jinja2 rendering errors occur - files = generator.generate_all(dry_run=False) + result = generator.generate_all(dry_run=False) - # All files should be created successfully - assert len(files) > 0 + assert len(result.created_files) > 0 - # All files should have content - for file_path in files: + for file_path in result.created_files: assert file_path.stat().st_size > 0 diff --git a/tests/unit/test_cli_bootstrap.py b/tests/unit/test_cli_bootstrap.py index 87cf81a5..7ac476ba 100644 --- a/tests/unit/test_cli_bootstrap.py +++ b/tests/unit/test_cli_bootstrap.py @@ -44,7 +44,8 @@ def test_bootstrap_dry_run(self, runner, temp_repo): # Should succeed assert result.exit_code == 0 assert "Dry run" in result.output - assert "would be created" in result.output + assert "Would create" in result.output + assert "Would skip" in result.output # Files should not exist assert not (temp_repo / ".github" / "workflows").exists() @@ -149,7 +150,7 @@ def test_bootstrap_reports_file_count(self, runner, temp_repo): # Should report file count assert result.exit_code == 0 assert "Created" in result.output - assert "files:" in result.output + assert "file(s):" in result.output def test_bootstrap_lists_created_files(self, runner, temp_repo): """Test bootstrap command lists created files.""" @@ -218,26 +219,38 @@ def test_bootstrap_with_existing_files(self, runner, temp_repo): github_dir = temp_repo / ".github" github_dir.mkdir() - # Create a workflow file + # Create a workflow file (non-bootstrap target — should not affect counts) workflows_dir = github_dir / "workflows" workflows_dir.mkdir() (workflows_dir / "existing.yml").write_text("name: existing") + # Customize a bootstrap target that must be preserved + precommit = temp_repo / ".pre-commit-config.yaml" + precommit.write_text("repos: [] # customized\n") + result = runner.invoke(bootstrap, [str(temp_repo)]) - # Should still work (may skip existing files) assert result.exit_code == 0 + assert "Skipped" in result.output + assert ".pre-commit-config.yaml" in result.output + assert precommit.read_text() == "repos: [] # customized\n" def test_bootstrap_multiple_runs(self, runner, temp_repo): - """Test running bootstrap multiple times.""" + """Test running bootstrap multiple times is idempotent.""" # First run result1 = runner.invoke(bootstrap, [str(temp_repo)]) assert result1.exit_code == 0 + assert "Created" in result1.output + + precommit = temp_repo / ".pre-commit-config.yaml" + original = precommit.read_text() # Second run (files already exist) result2 = runner.invoke(bootstrap, [str(temp_repo)]) - # Should still work (may skip or overwrite) assert result2.exit_code == 0 + assert "Created 0 file" in result2.output + assert "Skipped" in result2.output + assert precommit.read_text() == original def test_bootstrap_creates_workflow_files(self, runner, temp_repo): """Test bootstrap creates specific workflow files.""" From 09799d96aeb48fbef8e67ab0dd69806631315843 Mon Sep 17 00:00:00 2001 From: Dylan Hunt Date: Mon, 20 Jul 2026 20:19:23 -0400 Subject: [PATCH 2/2] fix: respect git ignore rules during assessment Introduce a per-repository GitAwareFileIndex (git ls-files + check-ignore --no-index) and route score-affecting assessor discovery through it so ignored paths no longer influence language totals or evidence. Co-authored-by: Cursor --- docs/user-guide.md | 5 + src/agentready/assessors/base.py | 10 +- src/agentready/assessors/code_quality.py | 108 +++----- src/agentready/assessors/dbt.py | 52 +++- src/agentready/assessors/documentation.py | 71 ++--- src/agentready/assessors/patterns.py | 87 +++--- src/agentready/assessors/security.py | 44 ++- src/agentready/assessors/structure.py | 88 ++---- src/agentready/assessors/stub_assessors.py | 34 +-- src/agentready/assessors/testing.py | 99 +++---- src/agentready/cli/bootstrap.py | 3 +- src/agentready/cli/main.py | 21 +- src/agentready/models/repository.py | 54 +++- .../services/git_aware_file_index.py | 257 +++++++++++++++++ src/agentready/services/language_detector.py | 83 ++---- tests/unit/test_adr_central_source.py | 10 +- tests/unit/test_assessors_dbt.py | 2 +- tests/unit/test_cli_validation.py | 61 ++--- tests/unit/test_git_aware_file_index.py | 258 ++++++++++++++++++ 19 files changed, 872 insertions(+), 475 deletions(-) create mode 100644 src/agentready/services/git_aware_file_index.py create mode 100644 tests/unit/test_git_aware_file_index.py diff --git a/docs/user-guide.md b/docs/user-guide.md index 0805aa3a..76bf5ab3 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -178,6 +178,11 @@ agentready bootstrap /path/to/repo ## Running Assessments +Assessment file discovery uses Git’s ignore rules (root and nested +`.gitignore`, wildcards, anchors, and negation). Ignored paths — including +tracked files that match an ignore rule — do not influence scores or +language totals. + ### Basic Usage ```bash diff --git a/src/agentready/assessors/base.py b/src/agentready/assessors/base.py index 906ab854..90d65fe4 100644 --- a/src/agentready/assessors/base.py +++ b/src/agentready/assessors/base.py @@ -105,10 +105,10 @@ def has_manifest(lang: str) -> bool: manifests = self._LANG_ROOT_MANIFESTS.get(lang, []) for manifest in manifests: if "*" in manifest: - if list(repository.path.glob(manifest)): + if repository.assessment_files(manifest): return True else: - if (repository.path / manifest).exists(): + if repository.assessment_exists(manifest): return True return False @@ -122,7 +122,7 @@ def has_manifest(lang: str) -> bool: # Special handling for JavaScript/TypeScript if set(detected_by_manifest) == {"JavaScript", "TypeScript"}: # TypeScript projects have tsconfig.json - stronger signal than file count - if (repository.path / "tsconfig.json").exists(): + if repository.assessment_exists("tsconfig.json"): return "TypeScript" # Use file counts to detect primary language @@ -161,9 +161,9 @@ def _find_go_module_roots(self, repository: Repository) -> list[Path]: testdata directories. """ roots: list[Path] = [] - if (repository.path / "go.mod").exists(): + if repository.assessment_exists("go.mod"): roots.append(repository.path) - for gomod in repository.path.rglob("go.mod"): + for gomod in repository.assessment_files("go.mod"): if "vendor" in gomod.parts or "testdata" in gomod.parts: continue if gomod.parent == repository.path: diff --git a/src/agentready/assessors/code_quality.py b/src/agentready/assessors/code_quality.py index 898ed335..1f85cbc4 100644 --- a/src/agentready/assessors/code_quality.py +++ b/src/agentready/assessors/code_quality.py @@ -19,7 +19,6 @@ from ..models.repository import Repository from ..services.scanner import MissingToolError from ..utils.subprocess_utils import ( - safe_subprocess_run, safe_subprocess_run_stream, sanitize_subprocess_error, ) @@ -89,22 +88,10 @@ def assess(self, repository: Repository) -> Finding: def _assess_python_types(self, repository: Repository) -> Finding: """Assess Python type annotations using AST parsing.""" # Use AST parsing to accurately detect type annotations - try: - # Security: Use safe_subprocess_run for validation and limits - result = safe_subprocess_run( - ["git", "ls-files", "*.py"], - cwd=repository.path, - capture_output=True, - text=True, - timeout=30, - check=True, - ) - python_files = [f for f in result.stdout.strip().split("\n") if f] - except Exception: - python_files = [ - str(f.relative_to(repository.path)) - for f in repository.path.rglob("*.py") - ] + python_files = [ + str(p.relative_to(repository.path)) + for p in repository.assessment_files("*.py") + ] total_functions = 0 typed_functions = 0 @@ -421,7 +408,7 @@ def _strip_json_comments(text: str) -> str: def _find_tsconfig_files(self, repository: Repository) -> list: """Find all tsconfig.json files, excluding node_modules/vendor/testdata.""" found = [] - for tsconfig in repository.path.rglob("tsconfig.json"): + for tsconfig in repository.assessment_files("tsconfig.json"): parts = tsconfig.parts if "node_modules" in parts or "vendor" in parts or "testdata" in parts: continue @@ -434,28 +421,11 @@ def _assess_go_types(self, repository: Repository) -> Finding: Go is statically typed at compile time. Score starts at 100 and deducts for excessive use of interface{}/any which weakens type safety. """ - from ..utils.subprocess_utils import safe_subprocess_run - - try: - result = safe_subprocess_run( - ["git", "ls-files", "*.go"], - cwd=repository.path, - capture_output=True, - text=True, - timeout=30, - check=True, - ) - go_files = [ - f - for f in result.stdout.strip().split("\n") - if f and not f.endswith("_test.go") - ] - except Exception: - go_files = [ - str(f.relative_to(repository.path)) - for f in repository.path.rglob("*.go") - if not f.name.endswith("_test.go") - ] + go_files = [ + str(p.relative_to(repository.path)) + for p in repository.assessment_files("*.go") + if not p.name.endswith("_test.go") + ] if not go_files: return Finding.not_applicable( @@ -612,7 +582,7 @@ def _assess_python_complexity(self, repository: Repository) -> Finding: """Assess Python complexity using radon.""" try: all_blocks = [] - for py_file in repository.path.rglob("*.py"): + for py_file in repository.assessment_files("*.py"): try: code = py_file.read_text(encoding="utf-8") all_blocks.extend(radon.complexity.cc_visit(code)) @@ -988,29 +958,16 @@ def _assess_go_logging(self, repository: Repository) -> Finding: # Check for stdlib log/slog (Go 1.21+, no go.mod entry needed) if not found_libs: - from ..utils.subprocess_utils import safe_subprocess_run - - try: - result = safe_subprocess_run( - ["git", "ls-files", "*.go"], - cwd=repository.path, - capture_output=True, - text=True, - timeout=30, - ) - if result.returncode == 0: - for f in result.stdout.strip().split("\n"): - if not f or f.endswith("_test.go"): - continue - try: - content = (repository.path / f).read_text(encoding="utf-8") - if '"log/slog"' in content: - found_libs.append("slog (stdlib)") - break - except (OSError, UnicodeDecodeError): - continue - except Exception: - pass + for go_file in repository.assessment_files("*.go"): + if go_file.name.endswith("_test.go"): + continue + try: + content = go_file.read_text(encoding="utf-8") + if '"log/slog"' in content: + found_libs.append("slog (stdlib)") + break + except (OSError, UnicodeDecodeError): + continue if found_libs: return Finding( @@ -1528,7 +1485,7 @@ def _collect_js_tools(self, repository: Repository) -> set[str]: def _find_tsconfig_files(self, repository: Repository) -> list[Path]: """Find all tsconfig.json files, excluding node_modules/vendor/testdata.""" found = [] - for tsconfig in repository.path.rglob("tsconfig.json"): + for tsconfig in repository.assessment_files("tsconfig.json"): parts = tsconfig.parts if "node_modules" in parts or "vendor" in parts or "testdata" in parts: continue @@ -1679,19 +1636,18 @@ def _collect_ci_tools(self, repository: Repository, language: str) -> set[str]: """ ci_files: list[Path] = [] - gh_workflows = repository.path / ".github" / "workflows" - if gh_workflows.exists(): - ci_files.extend(gh_workflows.glob("*.yml")) - ci_files.extend(gh_workflows.glob("*.yaml")) - - for ci_path in ( - repository.path / ".gitlab-ci.yml", - repository.path / ".circleci" / "config.yml", - repository.path / ".travis.yml", + for rel in repository.assessment_match_any( + [".github/workflows/*.yml", ".github/workflows/*.yaml"] ): - if ci_path.exists(): - ci_files.append(ci_path) + ci_files.append(repository.path / rel) + for rel in ( + ".gitlab-ci.yml", + ".circleci/config.yml", + ".travis.yml", + ): + if repository.assessment_exists(rel): + ci_files.append(repository.path / rel) if language == "Python": candidates = list(self._PYTHON_TOOLS.keys()) elif language == "Go": diff --git a/src/agentready/assessors/dbt.py b/src/agentready/assessors/dbt.py index 7fd4175a..caea9e96 100644 --- a/src/agentready/assessors/dbt.py +++ b/src/agentready/assessors/dbt.py @@ -30,17 +30,37 @@ def _is_dbt_project(repository: Repository) -> bool: return (repository.path / "dbt_project.yml").exists() -def _find_yaml_files(directory: Path) -> list[Path]: - """Find YAML files recursively (.yml and .yaml). +def _assessment_files_under( + repository: Repository, directory: Path, pattern: str +) -> list[Path]: + """Return assessment files matching pattern under a directory.""" + try: + rel_prefix = directory.relative_to(repository.path).as_posix() + except ValueError: + return [] + if not repository.assessment_exists(rel_prefix): + return [] + prefix = rel_prefix + "/" + return [ + p + for p in repository.assessment_files(pattern) + if p.relative_to(repository.path).as_posix().startswith(prefix) + ] + + +def _find_yaml_files(repository: Repository, directory: Path) -> list[Path]: + """Find YAML files under directory (.yml and .yaml). Args: + repository: Repository entity directory: Directory to search Returns: List of matching .yml and .yaml file paths """ - # Fix for #356: search both extensions explicitly instead of fragile replace() - return list(directory.rglob("*.yml")) + list(directory.rglob("*.yaml")) + return _assessment_files_under( + repository, directory, "*.yml" + ) + _assessment_files_under(repository, directory, "*.yaml") def _parse_yaml_safe(path: Path) -> dict: @@ -275,7 +295,7 @@ def assess(self, repository: Repository) -> Finding: """ models_dir = repository.path / "models" - if not models_dir.exists(): + if not repository.assessment_exists("models"): return Finding( attribute=self.attribute, status="fail", @@ -288,7 +308,7 @@ def assess(self, repository: Repository) -> Finding: ) # Count total SQL models - sql_files = list(models_dir.rglob("*.sql")) + sql_files = _assessment_files_under(repository, models_dir, "*.sql") total_models = len(sql_files) if total_models == 0: @@ -298,7 +318,7 @@ def assess(self, repository: Repository) -> Finding: # Find and parse schema YAML files (any .yml/.yaml file in models/) # dbt supports multiple naming conventions: schema.yml, _models.yml, or one file per model - schema_files = _find_yaml_files(models_dir) + schema_files = _find_yaml_files(repository, models_dir) # Extract documented model names documented_models = set() @@ -446,7 +466,7 @@ def assess(self, repository: Repository) -> Finding: """ models_dir = repository.path / "models" - if not models_dir.exists(): + if not repository.assessment_exists("models"): return Finding( attribute=self.attribute, status="fail", @@ -459,7 +479,7 @@ def assess(self, repository: Repository) -> Finding: ) # Count total SQL models - sql_files = list(models_dir.rglob("*.sql")) + sql_files = _assessment_files_under(repository, models_dir, "*.sql") total_models = len(sql_files) if total_models == 0: @@ -469,7 +489,7 @@ def assess(self, repository: Repository) -> Finding: # Find and parse schema YAML files (any .yml/.yaml file in models/) # dbt supports multiple naming conventions: schema.yml, _models.yml, or one file per model - schema_files = _find_yaml_files(models_dir) + schema_files = _find_yaml_files(repository, models_dir) # Extract models with PK tests (unique + not_null) models_with_pk_tests = set() @@ -516,7 +536,11 @@ def assess(self, repository: Repository) -> Finding: # Check for singular tests (bonus) tests_dir = repository.path / "tests" - singular_tests = list(tests_dir.rglob("*.sql")) if tests_dir.exists() else [] + singular_tests = ( + _assessment_files_under(repository, tests_dir, "*.sql") + if repository.assessment_exists("tests") + else [] + ) # Calculate proportional score score = self.calculate_proportional_score( @@ -646,7 +670,7 @@ def assess(self, repository: Repository) -> Finding: """ models_dir = repository.path / "models" - if not models_dir.exists(): + if not repository.assessment_exists("models"): return Finding( attribute=self.attribute, status="fail", @@ -659,7 +683,9 @@ def assess(self, repository: Repository) -> Finding: ) # Check for flat structure (50+ files in root models/) - root_sql_files = list(models_dir.glob("*.sql")) + root_sql_files = [ + p for p in repository.assessment_files("*.sql") if p.parent == models_dir + ] is_flat = len(root_sql_files) >= 50 # Check for recommended subdirectories diff --git a/src/agentready/assessors/documentation.py b/src/agentready/assessors/documentation.py index 9ccb0906..270c5a66 100644 --- a/src/agentready/assessors/documentation.py +++ b/src/agentready/assessors/documentation.py @@ -10,7 +10,6 @@ from ..models.attribute import Attribute from ..models.finding import Citation, Finding, Remediation from ..models.repository import Repository -from ..utils.subprocess_utils import safe_subprocess_run from .adr_sources import CentralAdrSource from .base import BaseAssessor @@ -687,8 +686,12 @@ def assess(self, repository: Repository) -> Finding: # Count .md files in ADR directory try: - adr_files = list(adr_dir.glob("*.md")) - except OSError as e: + rel_prefix = adr_dir.relative_to(repository.path).as_posix() + adr_files = [ + repository.path / rel + for rel in repository.assessment_match_any([f"{rel_prefix}/*.md"]) + ] + except (OSError, ValueError) as e: return Finding.error( self.attribute, reason=f"Could not read ADR directory: {e}" ) @@ -1062,21 +1065,10 @@ def assess(self, repository: Repository) -> Finding: def _assess_python_docstrings(self, repository: Repository) -> Finding: """Assess Python docstring coverage using AST parsing.""" # Get list of Python files - try: - result = safe_subprocess_run( - ["git", "ls-files", "*.py"], - cwd=repository.path, - capture_output=True, - text=True, - timeout=30, - check=True, - ) - python_files = [f for f in result.stdout.strip().split("\n") if f] - except Exception: - python_files = [ - str(f.relative_to(repository.path)) - for f in repository.path.rglob("*.py") - ] + python_files = [ + str(p.relative_to(repository.path)) + for p in repository.assessment_files("*.py") + ] total_public_items = 0 documented_items = 0 @@ -1184,26 +1176,11 @@ def _assess_go_godoc(self, repository: Repository) -> Finding: """ import re - try: - result = safe_subprocess_run( - ["git", "ls-files", "*.go"], - cwd=repository.path, - capture_output=True, - text=True, - timeout=30, - check=True, - ) - go_files = [ - f - for f in result.stdout.strip().split("\n") - if f and not f.endswith("_test.go") and "vendor/" not in f - ] - except Exception: - go_files = [ - str(f.relative_to(repository.path)) - for f in repository.path.rglob("*.go") - if not f.name.endswith("_test.go") and "vendor" not in f.parts - ] + go_files = [ + str(p.relative_to(repository.path)) + for p in repository.assessment_files("*.go") + if not p.name.endswith("_test.go") and "vendor" not in p.parts + ] total_exported = 0 documented_exported = 0 @@ -1481,28 +1458,12 @@ def assess(self, repository: Repository) -> Finding: # Recursively search for spec files found_specs = [] - excluded_dirs = { - ".git", - "node_modules", - ".venv", - "venv", - "__pycache__", - ".pytest_cache", - } for spec_name in spec_files: try: - # Use rglob to search recursively - matches = list(repository.path.rglob(spec_name)) - # Filter out files in excluded directories - matches = [ - m - for m in matches - if not any(part in m.parts for part in excluded_dirs) - ] + matches = repository.assessment_files(spec_name) found_specs.extend(matches) except OSError: - # If rglob fails, continue to next pattern continue # Remove duplicates while preserving order diff --git a/src/agentready/assessors/patterns.py b/src/agentready/assessors/patterns.py index 2c37bc3c..05aaacfd 100644 --- a/src/agentready/assessors/patterns.py +++ b/src/agentready/assessors/patterns.py @@ -44,7 +44,11 @@ def assess(self, repository: Repository) -> Finding: # Check for .claude/skills/ directory with tiered scoring skills_dir = repository.path / ".claude" / "skills" if skills_dir.exists() and skills_dir.is_dir(): - skill_files = list(skills_dir.rglob("SKILL.md")) + skill_files = [ + p + for p in repository.assessment_files("SKILL.md") + if skills_dir in p.parents or p.parent == skills_dir + ] skill_count = len(skill_files) if skill_count >= 3: has_skills = True @@ -239,32 +243,35 @@ def assess(self, repository: Repository) -> Finding: low_confidence_dir = None for dir_name in design_dirs: - design_dir = repository.path / dir_name - if design_dir.exists() and design_dir.is_dir(): - md_files = list(design_dir.glob("*.md")) - if md_files: - has_intent_content = False - for md_file in md_files: - try: - content = md_file.read_text(encoding="utf-8") - if any( - re.search(kw, content, re.IGNORECASE) - for kw in intent_keywords - ): - has_intent_content = True - break - except (OSError, UnicodeDecodeError): - continue + if not repository.assessment_exists(dir_name): + continue + md_files = [ + repository.path / rel + for rel in repository.assessment_match_any([f"{dir_name}/*.md"]) + ] + if md_files: + has_intent_content = False + for md_file in md_files: + try: + content = md_file.read_text(encoding="utf-8") + if any( + re.search(kw, content, re.IGNORECASE) + for kw in intent_keywords + ): + has_intent_content = True + break + except (OSError, UnicodeDecodeError): + continue - if has_intent_content: - score += 50.0 - evidence.append( - f"{dir_name}/ directory with {len(md_files)} design document(s) containing intent language" - ) - low_confidence_dir = None - break - elif low_confidence_dir is None: - low_confidence_dir = (dir_name, len(md_files)) + if has_intent_content: + score += 50.0 + evidence.append( + f"{dir_name}/ directory with {len(md_files)} design document(s) containing intent language" + ) + low_confidence_dir = None + break + elif low_confidence_dir is None: + low_confidence_dir = (dir_name, len(md_files)) if low_confidence_dir: score += 15.0 @@ -495,9 +502,11 @@ def assess(self, repository: Repository) -> Finding: evidence = [] # Check for .claude/rules/ directory with path-scoped rules - rules_dir = repository.path / ".claude" / "rules" - if rules_dir.exists() and rules_dir.is_dir(): - rule_files = list(rules_dir.glob("*.md")) + if repository.assessment_exists(".claude/rules"): + rule_files = [ + repository.path / rel + for rel in repository.assessment_match_any([".claude/rules/*.md"]) + ] if rule_files: # Check for path-scoped frontmatter scoped_count = 0 @@ -521,8 +530,16 @@ def assess(self, repository: Repository) -> Finding: ) # Check for subdirectory context files - sub_context_files = list(repository.path.rglob("CLAUDE.md")) - sub_context_files.extend(list(repository.path.rglob("AGENTS.md"))) + sub_context_files = [ + p + for p in repository.assessment_files("CLAUDE.md") + if p.parent != repository.path + ] + sub_context_files.extend( + p + for p in repository.assessment_files("AGENTS.md") + if p.parent != repository.path + ) sub_context_count = sum( 1 for f in sub_context_files if f.parent != repository.path ) @@ -534,7 +551,13 @@ def assess(self, repository: Repository) -> Finding: # Check for skills directory skills_dir = repository.path / ".claude" / "skills" if skills_dir.exists() and skills_dir.is_dir(): - skill_count = len(list(skills_dir.rglob("SKILL.md"))) + skill_count = len( + [ + p + for p in repository.assessment_files("SKILL.md") + if skills_dir in p.parents or p.parent == skills_dir + ] + ) if skill_count > 0: score = min(score + 30.0, 100.0) evidence.append( diff --git a/src/agentready/assessors/security.py b/src/agentready/assessors/security.py index 2fb11209..220958be 100644 --- a/src/agentready/assessors/security.py +++ b/src/agentready/assessors/security.py @@ -143,16 +143,16 @@ def assess(self, repository: Repository) -> Finding: evidence.append(" Meaningful Renovate configuration detected") # 2. CodeQL / GitHub Security Scanning (25 points) - codeql_workflow = repository.path / ".github" / "workflows" - if codeql_workflow.exists(): - codeql_files = list(codeql_workflow.glob("*codeql*.yml")) + list( - codeql_workflow.glob("*codeql*.yaml") - ) - if codeql_files: - score += 25 - tools_found.append("CodeQL") - evidence.append("✓ CodeQL security scanning configured") - + codeql_files = repository.assessment_match_any( + [ + ".github/workflows/*codeql*.yml", + ".github/workflows/*codeql*.yaml", + ] + ) + if codeql_files: + score += 25 + tools_found.append("CodeQL") + evidence.append("✓ CodeQL security scanning configured") # 3. Python dependency scanners (20 points) if "Python" in repository.languages: # Check for pip-audit, safety, or bandit @@ -223,24 +223,22 @@ def assess(self, repository: Repository) -> Finding: pass # 6. Semgrep (multi-language SAST) (15 points) - semgrep_config = repository.path / ".semgrep.yml" - semgrep_workflow = repository.path / ".github" / "workflows" - if semgrep_config.exists(): + if repository.assessment_exists(".semgrep.yml"): score += 15 tools_found.append("Semgrep") evidence.append("✓ Semgrep SAST configured") - elif semgrep_workflow.exists(): - semgrep_files = list(semgrep_workflow.glob("*semgrep*.yml")) + list( - semgrep_workflow.glob("*semgrep*.yaml") - ) - if semgrep_files: - score += 15 - tools_found.append("Semgrep") - evidence.append("✓ Semgrep SAST in GitHub Actions") + elif repository.assessment_match_any( + [ + ".github/workflows/*semgrep*.yml", + ".github/workflows/*semgrep*.yaml", + ] + ): + score += 15 + tools_found.append("Semgrep") + evidence.append("✓ Semgrep SAST in GitHub Actions") # 7. Security policy (5 points bonus) - security_md = repository.path / "SECURITY.md" - if security_md.exists(): + if repository.assessment_exists("SECURITY.md"): score += 5 evidence.append("✓ SECURITY.md present (vulnerability disclosure policy)") diff --git a/src/agentready/assessors/structure.py b/src/agentready/assessors/structure.py index 7e6c4b95..3588e6b9 100644 --- a/src/agentready/assessors/structure.py +++ b/src/agentready/assessors/structure.py @@ -391,22 +391,7 @@ def _assess_go_layout(self, repository: Repository) -> Finding: else: evidence.append("internal/ or pkg/: ✗ (no package encapsulation)") - from ..utils.subprocess_utils import safe_subprocess_run - - has_tests = False - try: - result = safe_subprocess_run( - ["git", "ls-files", "*_test.go", "**/*_test.go"], - cwd=repository.path, - capture_output=True, - text=True, - timeout=30, - ) - if result.returncode == 0: - test_files = [f for f in result.stdout.strip().split("\n") if f] - has_tests = len(test_files) > 0 - except Exception: - has_tests = bool(list(repository.path.rglob("*_test.go"))) + has_tests = bool(repository.assessment_files("*_test.go")) if has_tests: score += 20.0 @@ -463,21 +448,9 @@ def _check_naming_consistency(self, repository: Repository) -> list[str]: """ from collections import defaultdict - from ..utils.subprocess_utils import safe_subprocess_run - - try: - result = safe_subprocess_run( - ["git", "ls-files"], - cwd=repository.path, - capture_output=True, - text=True, - timeout=30, - ) - if result.returncode != 0: - return [] - files = [f for f in result.stdout.strip().split("\n") if f] - except Exception: - return [] + files = [ + str(p.relative_to(repository.path)) for p in repository.assessment_files() + ] skip_names = {"__init__", "__main__", "conftest", "setup"} dir_conventions: dict[str, dict[str, int]] = defaultdict( @@ -995,41 +968,36 @@ def assess(self, repository: Repository) -> Finding: repository.path / ".github" / "pull_request_template.md", ] - pr_template_found = any(p.exists() for p in pr_template_paths) + pr_template_found = any( + repository.assessment_exists(p.relative_to(repository.path).as_posix()) + for p in pr_template_paths + ) if pr_template_found: score += 50 evidence.append("PR template found") # Check for issue templates (50%) - issue_template_dir = repository.path / ".github" / "ISSUE_TEMPLATE" issue_templates_found = False + issue_templates = repository.assessment_match_any( + [ + ".github/ISSUE_TEMPLATE/*.md", + ".github/ISSUE_TEMPLATE/*.yml", + ".github/ISSUE_TEMPLATE/*.yaml", + ] + ) + template_count = len(issue_templates) - if issue_template_dir.exists() and issue_template_dir.is_dir(): - try: - md_templates = list(issue_template_dir.glob("*.md")) - yml_templates = list(issue_template_dir.glob("*.yml")) + list( - issue_template_dir.glob("*.yaml") - ) - template_count = len(md_templates) + len(yml_templates) - - if template_count >= 2: - score += 50 - evidence.append( - f"Issue templates found: {template_count} templates" - ) - issue_templates_found = True - elif template_count == 1: - score += 25 - evidence.append( - "Issue template directory exists with 1 template (need ≥2)" - ) - issue_templates_found = True - else: - evidence.append("Issue template directory exists but is empty") - except OSError: - evidence.append("Could not read issue template directory") - + if template_count >= 2: + score += 50 + evidence.append(f"Issue templates found: {template_count} templates") + issue_templates_found = True + elif template_count == 1: + score += 25 + evidence.append("Issue template directory exists with 1 template (need ≥2)") + issue_templates_found = True + elif repository.assessment_exists(".github/ISSUE_TEMPLATE"): + evidence.append("Issue template directory exists but is empty") # Fall back to org-level .github repo if anything is still missing if not pr_template_found or not issue_templates_found: owner = self._parse_github_owner(repository.url) @@ -1254,7 +1222,7 @@ def _check_file_cohesion(self, repository: Repository) -> tuple: # Check Python files try: - py_files = list(repository.path.rglob("*.py")) + py_files = repository.assessment_files("*.py") for py_file in py_files: # Skip venv, node_modules, etc. if any( @@ -1291,7 +1259,7 @@ def _check_module_naming(self, repository: Repository) -> tuple: found = [] try: for pattern in antipattern_names: - matches = list(repository.path.rglob(pattern)) + matches = repository.assessment_files(pattern) # Filter out venv/node_modules matches = [ m diff --git a/src/agentready/assessors/stub_assessors.py b/src/agentready/assessors/stub_assessors.py index e6661bba..394e6e87 100644 --- a/src/agentready/assessors/stub_assessors.py +++ b/src/agentready/assessors/stub_assessors.py @@ -12,7 +12,6 @@ from ..models.attribute import Attribute from ..models.finding import Citation, Finding, Remediation from ..models.repository import Repository -from ..utils.subprocess_utils import safe_subprocess_run from .base import BaseAssessor @@ -67,10 +66,10 @@ def assess(self, repository: Repository) -> Finding: # Check subdirectories for Go monorepos (go.sum in module dirs) if "go.sum" not in found_strict: - for gosum in repository.path.rglob("go.sum"): - if "vendor" in gosum.parts: + for rel in repository.assessment_match_any(["go.sum"]): + if "vendor" in rel.split("/"): continue - found_strict.append(str(gosum.relative_to(repository.path))) + found_strict.append(rel) if not found_strict and not found_manual: return Finding( @@ -640,7 +639,7 @@ def assess(self, repository: Repository) -> Finding: - 75-99: Some files 500-1000 lines - 0-74: Files >1000 lines exist - Note: Uses git ls-files to respect .gitignore (fixes issue #245). + Note: Uses assessment file index to respect .gitignore (fixes issue #245). """ # Count files by size large_files: list[tuple[Path, int]] = [] # 500-1000 lines @@ -663,28 +662,9 @@ def assess(self, repository: Repository) -> Finding: "h", ] - # Get git-tracked files (respects .gitignore) - # This fixes issue #245 where .venv files were incorrectly scanned - try: - patterns = [f"*.{ext}" for ext in extensions] - result = safe_subprocess_run( - ["git", "ls-files"] + patterns, - cwd=repository.path, - capture_output=True, - text=True, - timeout=30, - check=True, - ) - tracked_files = [f for f in result.stdout.strip().split("\n") if f] - except Exception: - # Fallback for non-git repos: use glob (less accurate) - tracked_files = [] - for ext in extensions: - tracked_files.extend( - str(f.relative_to(repository.path)) - for f in repository.path.rglob(f"*.{ext}") - if f.is_file() - ) + tracked_files = repository.assessment_match_any( + f"*.{ext}" for ext in extensions + ) # Count lines in tracked files for rel_path in tracked_files: diff --git a/src/agentready/assessors/testing.py b/src/agentready/assessors/testing.py index 8a995e1a..bff481be 100644 --- a/src/agentready/assessors/testing.py +++ b/src/agentready/assessors/testing.py @@ -233,12 +233,12 @@ def _check_test_organization( f"Test organization: separate {'/'.join(parts)} directories" ) - if not evidence: - test_files = ( - list((tests_dir).rglob("test_*.py"))[:10] - if tests_dir.exists() - else [] - ) + if not evidence and repository.assessment_exists("tests"): + test_files = [ + p + for p in repository.assessment_files("test_*.py") + if tests_dir in p.parents or p.parent == tests_dir + ][:10] for tf in test_files: try: content = tf.read_text(encoding="utf-8") @@ -306,7 +306,7 @@ def _check_test_organization( pass elif language == "Go": - test_files = list(repository.path.rglob("*_test.go"))[:10] + test_files = repository.assessment_files("*_test.go")[:10] for tf in test_files: try: content = tf.read_text(encoding="utf-8") @@ -343,11 +343,13 @@ def _has_python_test_files(self, repository: Repository) -> bool: test_dirs = ["tests", "test"] for d in test_dirs: test_dir = repository.path / d - if test_dir.exists(): - # Look for test_*.py or *_test.py files - test_files = list(test_dir.rglob("test_*.py")) + list( - test_dir.rglob("*_test.py") - ) + if repository.assessment_exists(d): + test_files = [ + p + for p in repository.assessment_files("test_*.py") + + repository.assessment_files("*_test.py") + if test_dir in p.parents or p.parent == test_dir + ] if test_files: return True return False @@ -488,14 +490,9 @@ def _assess_javascript_coverage(self, repository: Repository) -> Finding: def _has_js_test_files(self, repository: Repository) -> bool: """Check if JavaScript/TypeScript test files exist.""" - # Check for __tests__ directory - if (repository.path / "__tests__").exists(): + if repository.assessment_exists("__tests__"): return True - # Check for *.test.* or *.spec.* files in src/ or root - for pattern in ["**/*.test.*", "**/*.spec.*"]: - if list(repository.path.glob(pattern)): - return True - return False + return bool(repository.assessment_match_any(["*.test.*", "*.spec.*"])) def _has_js_coverage_threshold(self, repository: Repository, pkg: dict) -> bool: """Check if JS coverage threshold is configured.""" @@ -525,22 +522,7 @@ def _has_js_coverage_threshold(self, repository: Repository, pkg: dict) -> bool: def _has_go_test_files(self, repository: Repository) -> bool: """Check if Go test files (*_test.go) exist.""" - from ..utils.subprocess_utils import safe_subprocess_run - - try: - result = safe_subprocess_run( - ["git", "ls-files", "*_test.go", "**/*_test.go"], - cwd=repository.path, - capture_output=True, - text=True, - timeout=30, - ) - if result.returncode == 0: - files = [f for f in result.stdout.strip().split("\n") if f] - return len(files) > 0 - except Exception: - pass - return bool(list(repository.path.rglob("*_test.go"))) + return bool(repository.assessment_files("*_test.go")) def _assess_go_coverage(self, repository: Repository) -> Finding: """Assess Go test execution and coverage configuration. @@ -655,11 +637,10 @@ def _read_go_build_files(self, repository: Repository) -> str: ] ) - ci_dir = repository.path / ".github" / "workflows" - if ci_dir.exists(): - files_to_check.extend(ci_dir.glob("*.yml")) - files_to_check.extend(ci_dir.glob("*.yaml")) - + for rel in repository.assessment_match_any( + [".github/workflows/*.yml", ".github/workflows/*.yaml"] + ): + files_to_check.append(repository.path / rel) for f in files_to_check: if f.exists(): try: @@ -1053,27 +1034,23 @@ def assess(self, repository: Repository) -> Finding: def _detect_ci_configs(self, repository: Repository) -> list: """Detect CI/CD configuration files.""" - ci_config_checks = [ - repository.path / ".github" / "workflows", # GitHub Actions (directory) - repository.path / ".gitlab-ci.yml", # GitLab CI - repository.path / ".circleci" / "config.yml", # CircleCI - repository.path / ".travis.yml", # Travis CI - repository.path / "Jenkinsfile", # Jenkins - repository.path / ".tekton", # Pipelines-as-Code - ] - - configs = [] - for config_path in ci_config_checks: - if config_path.exists(): - if config_path.is_dir(): - # GitHub Actions: check for workflow files - workflow_files = list(config_path.glob("*.yml")) + list( - config_path.glob("*.yaml") - ) - if workflow_files: - configs.extend(workflow_files) - else: - configs.append(config_path) + configs: list[Path] = [] + for rel in repository.assessment_match_any( + [".github/workflows/*.yml", ".github/workflows/*.yaml"] + ): + configs.append(repository.path / rel) + + for rel in ( + ".gitlab-ci.yml", + ".circleci/config.yml", + ".travis.yml", + "Jenkinsfile", + ): + if repository.assessment_exists(rel): + configs.append(repository.path / rel) + + for rel in repository.assessment_match_any([".tekton/*.yml", ".tekton/*.yaml"]): + configs.append(repository.path / rel) return configs diff --git a/src/agentready/cli/bootstrap.py b/src/agentready/cli/bootstrap.py index 8245402b..f04ee199 100644 --- a/src/agentready/cli/bootstrap.py +++ b/src/agentready/cli/bootstrap.py @@ -77,8 +77,7 @@ def bootstrap(repository, dry_run, language): click.echo(" (none)") click.echo( - f"\n{skipped_label} {len(result.skipped_files)} file(s) " - "(already exists):" + f"\n{skipped_label} {len(result.skipped_files)} file(s) " "(already exists):" ) if result.skipped_files: for file_path in sorted(result.skipped_files): diff --git a/src/agentready/cli/main.py b/src/agentready/cli/main.py index b5600a37..50dd549e 100644 --- a/src/agentready/cli/main.py +++ b/src/agentready/cli/main.py @@ -18,6 +18,7 @@ from ..models.config import Config from ..reporters.html import HTMLReporter from ..reporters.markdown import MarkdownReporter +from ..services.git_aware_file_index import GitAwareFileIndex, GitAwareFileIndexError from ..services.research_loader import ResearchLoader from ..services.scanner import Scanner from ..utils.security import ( @@ -25,7 +26,6 @@ VAR_SENSITIVE_SUBDIRS, _is_path_in_directory, ) -from ..utils.subprocess_utils import safe_subprocess_run # Lightweight commands - imported immediately from .align import align @@ -206,27 +206,16 @@ def run_assessment( # Performance: Warn for large repositories try: - # Quick file count using git ls-files (if it's a git repo) or fallback - # Security: Use safe_subprocess_run for validation and limits - result = safe_subprocess_run( - ["git", "ls-files"], - cwd=repo_path, - capture_output=True, - text=True, - timeout=5, - ) - if result.returncode == 0: - file_count = len(result.stdout.splitlines()) - else: - # Not a git repo, use glob (slower but works) - file_count = sum(1 for _ in repo_path.rglob("*") if _.is_file()) - + index = GitAwareFileIndex(repo_path) + file_count = len(list(index.iter_files())) if file_count > 10000: click.confirm( f"⚠️ Warning: Large repository detected ({file_count:,} files). " "Assessment may take several minutes. Continue?", abort=True, ) + except GitAwareFileIndexError: + pass except click.Abort: # User declined to continue - re-raise to abort raise diff --git a/src/agentready/models/repository.py b/src/agentready/models/repository.py index 02436814..2bf26ec6 100644 --- a/src/agentready/models/repository.py +++ b/src/agentready/models/repository.py @@ -1,12 +1,13 @@ """Repository model representing the target git repository being assessed.""" -from dataclasses import dataclass +from dataclasses import dataclass, field from pathlib import Path from typing import TYPE_CHECKING from ..utils.privacy import sanitize_path, shorten_commit_hash if TYPE_CHECKING: + from ..services.git_aware_file_index import GitAwareFileIndex from .config import Config @@ -35,6 +36,9 @@ class Repository: total_files: int total_lines: int config: "Config | None" = None + _file_index: "GitAwareFileIndex | None" = field( + default=None, repr=False, compare=False + ) def __post_init__(self): """Validate repository data after initialization.""" @@ -54,6 +58,53 @@ def __post_init__(self): if self.total_lines < 0: raise ValueError(f"Total lines must be non-negative: {self.total_lines}") + @property + def file_index(self) -> "GitAwareFileIndex": + """Git-ignore-aware file index scoped to this repository. + + Lazy-built and cached for the lifetime of this Repository instance so + assessors share one ignore boundary without process-global state. + """ + if self._file_index is None: + from ..services.git_aware_file_index import GitAwareFileIndex + + self._file_index = GitAwareFileIndex(self.path) + return self._file_index + + def assessment_files(self, pattern: str | None = None) -> list[Path]: + """Return absolute paths of assessment-eligible files (optionally matched). + + On Git index failure, returns an empty list (fail closed — never scan + ignored trees via unfiltered ``rglob``). + """ + from ..services.git_aware_file_index import GitAwareFileIndexError + + try: + return self.file_index.absolute_paths(pattern) + except GitAwareFileIndexError: + return [] + + def assessment_exists(self, relative: str | Path) -> bool: + """True when a score-relevant path exists and is not ignore-excluded. + + On Git index failure, returns False (fail closed). + """ + from ..services.git_aware_file_index import GitAwareFileIndexError + + try: + return self.file_index.exists(relative) + except GitAwareFileIndexError: + return False + + def assessment_match_any(self, patterns: list[str]) -> list[str]: + """Return relative paths matching any pattern via the assessment file set.""" + from ..services.git_aware_file_index import GitAwareFileIndexError + + try: + return self.file_index.match_any(patterns) + except GitAwareFileIndexError: + return [] + def get_sanitized_path(self) -> str: """Get sanitized path for public display. @@ -91,6 +142,7 @@ def from_dict(cls, data: dict) -> "Repository": repo.total_files = data.get("total_files", 0) repo.total_lines = data.get("total_lines", 0) repo.config = None + repo._file_index = None return repo @property diff --git a/src/agentready/services/git_aware_file_index.py b/src/agentready/services/git_aware_file_index.py new file mode 100644 index 00000000..9a5785bc --- /dev/null +++ b/src/agentready/services/git_aware_file_index.py @@ -0,0 +1,257 @@ +"""Git-aware repository file index for assessment discovery. + +Enumerates files that should influence AgentReady scores using Git's own +ignore rules (root and nested ``.gitignore``, wildcards, anchors, negation). +Tracked files that match an ignore rule are excluded via +``git check-ignore --no-index`` per issue #453. + +Instances are scoped to one repository path and cache an immutable file set +for reuse within a single assessment. They never use process-global state. + +When ``.git`` is missing or is not a usable worktree (common in unit-test +fixtures that only ``mkdir .git``, or callers that bypass Repository +validation), the index falls back to a plain filesystem walk. Ignore +filtering requires a real Git worktree; production assessments always use +the Git path. Unexpected Git command failures in a real worktree fail +closed — they never silently reintroduce ignored trees via ``rglob``. +""" + +from __future__ import annotations + +import logging +from pathlib import Path, PurePosixPath +from typing import Iterable, Iterator, Literal + +from ..utils.subprocess_utils import safe_subprocess_run + +logger = logging.getLogger(__name__) + +IndexMode = Literal["git", "filesystem"] + + +class GitAwareFileIndexError(RuntimeError): + """Raised when Git cannot provide a reliable assessment file list.""" + + +class GitAwareFileIndex: + """Immutable, Git-ignore-aware file set for one repository.""" + + def __init__(self, repository_path: Path | str): + """Create an index bound to ``repository_path``. + + Loading is lazy on first use so construction stays cheap. + """ + self.repository_path = Path(repository_path).resolve() + self._relative_files: frozenset[str] | None = None + self._ignored_cache: dict[str, bool] = {} + self._mode: IndexMode | None = None + + def _ensure_loaded(self) -> frozenset[str]: + if self._relative_files is not None: + return self._relative_files + + if self._is_usable_git_worktree(): + self._mode = "git" + try: + listed = self._ls_files() + ignored = self._check_ignore(listed) + files = frozenset(p for p in listed if p not in ignored) + except GitAwareFileIndexError: + raise + except Exception as exc: # noqa: BLE001 — surface as typed failure + raise GitAwareFileIndexError( + f"Failed to build Git-aware file index for " + f"{self.repository_path}: {exc}" + ) from exc + else: + # No usable worktree: stub ``.git`` fixtures, or callers that bypass + # Repository validation. Walk the filesystem so discovery helpers + # still see on-disk files. Ignore rules require a real Git worktree. + self._mode = "filesystem" + logger.debug( + "No usable Git worktree at %s; listing files without ignore " + "filtering", + self.repository_path, + ) + files = frozenset(self._walk_files()) + + self._relative_files = files + return files + + def _is_usable_git_worktree(self) -> bool: + result = safe_subprocess_run( + ["git", "rev-parse", "--is-inside-work-tree"], + cwd=self.repository_path, + capture_output=True, + text=True, + timeout=10, + check=False, + ) + return result.returncode == 0 and (result.stdout or "").strip() == "true" + + def _ls_files(self) -> list[str]: + """List tracked and eligible untracked files (NUL-delimited).""" + result = safe_subprocess_run( + ["git", "ls-files", "-z", "--cached", "--others", "--exclude-standard"], + cwd=self.repository_path, + capture_output=True, + text=False, + timeout=60, + check=True, + ) + return self._split_nul(result.stdout) + + def _check_ignore(self, paths: list[str]) -> set[str]: + """Return paths that match ignore rules, including force-added tracked files.""" + if not paths: + return set() + + ignored: set[str] = set() + batch_size = 500 + for i in range(0, len(paths), batch_size): + batch = paths[i : i + batch_size] + payload = b"\0".join( + p.encode("utf-8", errors="surrogateescape") for p in batch + ) + payload += b"\0" + result = safe_subprocess_run( + ["git", "check-ignore", "--no-index", "-z", "--stdin"], + cwd=self.repository_path, + input=payload, + capture_output=True, + text=False, + timeout=60, + check=False, + ) + # Exit 0: some matches; 1: none; other: error + if result.returncode not in (0, 1): + stderr = (result.stderr or b"").decode("utf-8", errors="replace") + raise GitAwareFileIndexError( + f"git check-ignore failed (exit {result.returncode}): {stderr}" + ) + ignored.update(self._split_nul(result.stdout)) + return ignored + + def _walk_files(self) -> list[str]: + """Enumerate files on disk, excluding the ``.git`` directory tree.""" + out: list[str] = [] + for path in self.repository_path.rglob("*"): + if not path.is_file() and not path.is_symlink(): + continue + try: + rel = path.relative_to(self.repository_path).as_posix() + except ValueError: + continue + if rel == ".git" or rel.startswith(".git/"): + continue + out.append(rel) + return out + + @staticmethod + def _split_nul(data: bytes | str | None) -> list[str]: + if not data: + return [] + if isinstance(data, str): + raw = data.encode("utf-8", errors="surrogateescape") + else: + raw = data + parts = raw.split(b"\0") + out: list[str] = [] + for part in parts: + if not part: + continue + out.append(part.decode("utf-8", errors="surrogateescape")) + return out + + @staticmethod + def _normalize(relative: str | PurePosixPath | Path) -> str: + text = str(relative).replace("\\", "/") + while text.startswith("./"): + text = text[2:] + return text + + def relative_files(self) -> frozenset[str]: + """Return all assessment-eligible repository-relative paths.""" + return self._ensure_loaded() + + def iter_files(self) -> Iterator[str]: + """Iterate assessment-eligible relative paths in sorted order.""" + yield from sorted(self._ensure_loaded()) + + def match(self, pattern: str) -> list[str]: + """Return relative paths matching a pathlib-style pattern (e.g. ``*.py``).""" + matched = [ + rel for rel in self._ensure_loaded() if PurePosixPath(rel).match(pattern) + ] + return sorted(matched) + + def match_any(self, patterns: Iterable[str]) -> list[str]: + """Return relative paths matching any of the given patterns.""" + pats = list(patterns) + matched = [ + rel + for rel in self._ensure_loaded() + if any(PurePosixPath(rel).match(p) for p in pats) + ] + return sorted(set(matched)) + + def contains(self, relative: str | PurePosixPath | Path) -> bool: + """True when the relative path is in the assessment file set.""" + return self._normalize(relative) in self._ensure_loaded() + + def is_ignored(self, relative: str | PurePosixPath | Path) -> bool: + """True when Git ignore rules exclude this relative path.""" + rel = self._normalize(relative) + if not rel: + return False + if rel in self._ignored_cache: + return self._ignored_cache[rel] + + files = self._ensure_loaded() + if self._mode != "git": + # Filesystem mode cannot evaluate ignore rules without Git. + self._ignored_cache[rel] = False + return False + + if rel in files: + self._ignored_cache[rel] = False + return False + + ignored = self._path_check_ignore(rel) + self._ignored_cache[rel] = ignored + return ignored + + def _path_check_ignore(self, relative: str) -> bool: + payload = relative.encode("utf-8", errors="surrogateescape") + b"\0" + result = safe_subprocess_run( + ["git", "check-ignore", "--no-index", "-z", "--stdin"], + cwd=self.repository_path, + input=payload, + capture_output=True, + text=False, + timeout=30, + check=False, + ) + if result.returncode == 0: + return True + if result.returncode == 1: + return False + stderr = (result.stderr or b"").decode("utf-8", errors="replace") + raise GitAwareFileIndexError( + f"git check-ignore failed for {relative!r}: {stderr}" + ) + + def exists(self, relative: str | PurePosixPath | Path) -> bool: + """True when the path exists on disk and is not ignore-excluded.""" + rel = self._normalize(relative) + full = self.repository_path / rel + if not full.exists() and not full.is_symlink(): + return False + if self.is_ignored(rel): + return False + return True + + def absolute_paths(self, pattern: str | None = None) -> list[Path]: + """Return absolute Paths for matching (or all) assessment files.""" + rels = self.match(pattern) if pattern else sorted(self._ensure_loaded()) + return [self.repository_path / rel for rel in rels] diff --git a/src/agentready/services/language_detector.py b/src/agentready/services/language_detector.py index ccb03861..84f702c6 100644 --- a/src/agentready/services/language_detector.py +++ b/src/agentready/services/language_detector.py @@ -4,7 +4,7 @@ from collections import defaultdict from pathlib import Path -from ..utils.subprocess_utils import safe_subprocess_run +from .git_aware_file_index import GitAwareFileIndex, GitAwareFileIndexError logger = logging.getLogger(__name__) @@ -12,8 +12,8 @@ class LanguageDetector: """Detects programming languages in a repository. - Uses file extension mapping and respects .gitignore patterns - via git ls-files for accuracy. + Uses file extension mapping and respects .gitignore patterns via the + shared :class:`GitAwareFileIndex` (never an unfiltered ``rglob`` fallback). """ # Extension to language mapping @@ -58,15 +58,29 @@ class LanguageDetector: ".xml": "XML", } - def __init__(self, repository_path: Path): + def __init__( + self, + repository_path: Path, + file_index: GitAwareFileIndex | None = None, + ): """Initialize language detector for repository. Args: repository_path: Path to git repository root + file_index: Optional shared index (created if omitted) """ self.repository_path = repository_path + self.file_index = file_index or GitAwareFileIndex(repository_path) self.minimum_file_threshold = 3 # Need 3+ files to count as "using language" + def _assessment_files(self) -> list[str]: + try: + return list(self.file_index.iter_files()) + except GitAwareFileIndexError as exc: + # Fail closed: do not scan ignored trees via rglob. + logger.error("Language detection unavailable: %s", exc) + return [] + def detect_languages(self) -> dict[str, int]: """Detect languages in repository with file counts. @@ -76,30 +90,9 @@ def detect_languages(self) -> dict[str, int]: Only includes languages with >= minimum_file_threshold files. """ - language_counts = defaultdict(int) + language_counts: defaultdict[str, int] = defaultdict(int) - # Try git ls-files first (respects .gitignore) - try: - # Security: Use safe_subprocess_run for validation and limits - result = safe_subprocess_run( - ["git", "ls-files"], - cwd=self.repository_path, - capture_output=True, - text=True, - timeout=30, - check=True, - ) - files = result.stdout.strip().split("\n") - except Exception: - # Fall back to pathlib walk (less accurate) - files = [ - str(f.relative_to(self.repository_path)) - for f in self.repository_path.rglob("*") - if f.is_file() - ] - - # Count files by language - for file_path in files: + for file_path in self._assessment_files(): if not file_path.strip(): continue @@ -123,21 +116,7 @@ def count_total_files(self) -> int: Returns: Total file count """ - try: - # Security: Use safe_subprocess_run for validation and limits - result = safe_subprocess_run( - ["git", "ls-files"], - cwd=self.repository_path, - capture_output=True, - text=True, - timeout=30, - check=True, - ) - files = result.stdout.strip().split("\n") - return len([f for f in files if f.strip()]) - except Exception: - # Fall back to pathlib - return sum(1 for _ in self.repository_path.rglob("*") if _.is_file()) + return len(self._assessment_files()) def count_total_lines(self) -> int: """Count total lines of code in repository. @@ -150,25 +129,7 @@ def count_total_lines(self) -> int: """ total_lines = 0 - try: - # Security: Use safe_subprocess_run for validation and limits - result = safe_subprocess_run( - ["git", "ls-files"], - cwd=self.repository_path, - capture_output=True, - text=True, - timeout=30, - check=True, - ) - files = result.stdout.strip().split("\n") - except Exception: - files = [ - str(f.relative_to(self.repository_path)) - for f in self.repository_path.rglob("*") - if f.is_file() - ] - - for file_path in files: + for file_path in self._assessment_files(): if not file_path.strip(): continue diff --git a/tests/unit/test_adr_central_source.py b/tests/unit/test_adr_central_source.py index 67d63b40..c774650b 100644 --- a/tests/unit/test_adr_central_source.py +++ b/tests/unit/test_adr_central_source.py @@ -53,14 +53,20 @@ def test_finds_docs_adr_when_no_uppercase_ADR(self, tmp_path): assert source.find_adr_dir(repo) == adr_dir def test_finds_root_adr_lowercase(self, tmp_path): - """Return adr/ (lowercase) when it is the first matching candidate.""" + """Return adr/ (lowercase) when it contains ADR markdown files. + + On case-insensitive filesystems, the ``ADR`` candidate path resolves to + the same directory; ``samefile`` asserts the correct location either way. + """ repo = _make_repo(tmp_path) adr_dir = tmp_path / "adr" adr_dir.mkdir() (adr_dir / "0001-init.md").write_text("# ADR 1") source = LocalAdrSource() - assert source.find_adr_dir(repo) == adr_dir + found = source.find_adr_dir(repo) + assert found is not None + assert found.samefile(adr_dir) def test_finds_decisions_dir(self, tmp_path): """Return decisions/ when it contains .md files.""" diff --git a/tests/unit/test_assessors_dbt.py b/tests/unit/test_assessors_dbt.py index be475409..84638d84 100644 --- a/tests/unit/test_assessors_dbt.py +++ b/tests/unit/test_assessors_dbt.py @@ -142,7 +142,7 @@ def test_is_dbt_project_false(self, non_dbt_repo): def test_find_yaml_files(self, well_structured_repo): """Test _find_yaml_files finds YAML files recursively.""" models_dir = well_structured_repo.path / "models" - yaml_files = _find_yaml_files(models_dir) + yaml_files = _find_yaml_files(well_structured_repo, models_dir) assert len(yaml_files) >= 2 # At least staging and marts schema.yml assert all(f.suffix in [".yml", ".yaml"] for f in yaml_files) diff --git a/tests/unit/test_cli_validation.py b/tests/unit/test_cli_validation.py index 029a76f3..0ae36df1 100644 --- a/tests/unit/test_cli_validation.py +++ b/tests/unit/test_cli_validation.py @@ -106,13 +106,11 @@ def test_warns_on_large_repository(self, tmp_path): git_dir = tmp_path / ".git" git_dir.mkdir() - # Mock safe_subprocess_run to return large file count - with patch("agentready.cli.main.safe_subprocess_run") as mock_safe_run: - mock_result = MagicMock() - mock_result.returncode = 0 - mock_result.stdout = "\n".join([f"file{i}.py" for i in range(10001)]) - mock_safe_run.return_value = mock_result - + large_files = [f"file{i}.py" for i in range(10001)] + with patch( + "agentready.cli.main.GitAwareFileIndex.iter_files", + return_value=iter(large_files), + ): # Run without confirmation (should abort) result = runner.invoke(cli, ["assess", str(tmp_path)], input="n\n") @@ -135,13 +133,11 @@ def test_no_warning_for_small_repository(self, tmp_path): claude_md = tmp_path / "CLAUDE.md" claude_md.write_text("# Test Repository\n") - # Mock safe_subprocess_run to return small file count - with patch("agentready.cli.main.safe_subprocess_run") as mock_safe_run: - mock_result = MagicMock() - mock_result.returncode = 0 - mock_result.stdout = "\n".join([f"file{i}.py" for i in range(100)]) - mock_safe_run.return_value = mock_result - + small_files = [f"file{i}.py" for i in range(100)] + with patch( + "agentready.cli.main.GitAwareFileIndex.iter_files", + return_value=iter(small_files), + ): # Run assessment result = runner.invoke(cli, ["assess", str(tmp_path)]) @@ -149,9 +145,11 @@ def test_no_warning_for_small_repository(self, tmp_path): assert "Large repository detected" not in result.output def test_handles_git_failure_gracefully(self, tmp_path): - """Test that assessment continues if git ls-files fails during file count.""" + """Test that assessment continues if the Git-aware index fails during file count.""" import subprocess + from agentready.services.git_aware_file_index import GitAwareFileIndexError + runner = CliRunner() # Initialize as proper git repository with initial commit @@ -184,26 +182,11 @@ def test_handles_git_failure_gracefully(self, tmp_path): capture_output=True, ) - # Mock safe_subprocess_run to fail only for the file count check (git ls-files) - # but let other git commands (in Scanner) work normally - original_safe_run = __import__( - "agentready.utils.subprocess_utils", fromlist=["safe_subprocess_run"] - ).safe_subprocess_run - - def selective_mock(*args, **kwargs): - # Fail for git ls-files in the file count check (has timeout=5) - if args[0] == ["git", "ls-files"] and kwargs.get("timeout") == 5: - mock_result = MagicMock() - mock_result.returncode = 1 - mock_result.stdout = "" - return mock_result - # Let all other calls through to real implementation - return original_safe_run(*args, **kwargs) - with patch( - "agentready.cli.main.safe_subprocess_run", side_effect=selective_mock + "agentready.cli.main.GitAwareFileIndex.iter_files", + side_effect=GitAwareFileIndexError("simulated failure"), ): - # Run assessment - should continue despite git ls-files failure + # Run assessment - should continue despite index failure result = runner.invoke(cli, ["assess", str(tmp_path)]) # Should complete successfully (file count check is wrapped in try/except) @@ -235,13 +218,11 @@ def mock_str_method(self): mock_path_instance.resolve.return_value = sensitive_mock mock_path_class.return_value = mock_path_instance - # Mock safe_subprocess_run to return large file count - with patch("agentready.cli.main.safe_subprocess_run") as mock_safe_run: - mock_result = MagicMock() - mock_result.returncode = 0 - mock_result.stdout = "\n".join([f"file{i}.py" for i in range(10001)]) - mock_safe_run.return_value = mock_result - + large_files = [f"file{i}.py" for i in range(10001)] + with patch( + "agentready.cli.main.GitAwareFileIndex.iter_files", + return_value=iter(large_files), + ): # Run without confirmation (should abort on first warning) result = runner.invoke(cli, ["assess", str(tmp_path)], input="n\n") diff --git a/tests/unit/test_git_aware_file_index.py b/tests/unit/test_git_aware_file_index.py new file mode 100644 index 00000000..c0ce75b6 --- /dev/null +++ b/tests/unit/test_git_aware_file_index.py @@ -0,0 +1,258 @@ +"""Tests for Git-aware assessment file discovery (#453).""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +import pytest + +from agentready.services.git_aware_file_index import ( + GitAwareFileIndex, + GitAwareFileIndexError, +) +from agentready.services.language_detector import LanguageDetector + + +def _git(repo: Path, *args: str) -> None: + subprocess.run( + ["git", *args], + cwd=repo, + check=True, + capture_output=True, + ) + + +@pytest.fixture +def git_repo(tmp_path: Path) -> Path: + repo = tmp_path / "repo" + repo.mkdir() + _git(repo, "init") + _git(repo, "config", "user.email", "test@example.com") + _git(repo, "config", "user.name", "Test") + return repo + + +def _write_tracked(repo: Path, relative: str, content: str = "x\n") -> Path: + path = repo / relative + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content) + _git(repo, "add", relative) + return path + + +class TestGitAwareFileIndex: + def test_root_gitignore_excludes_generated(self, git_repo: Path): + (git_repo / ".gitignore").write_text("generated/\n") + _write_tracked(git_repo, "src/main.py", "print(1)\n") + ignored = git_repo / "generated" / "out.py" + ignored.parent.mkdir() + ignored.write_text("print('ignored')\n") + + index = GitAwareFileIndex(git_repo) + files = index.relative_files() + assert "src/main.py" in files + assert "generated/out.py" not in files + assert index.is_ignored("generated/out.py") + assert not index.exists("generated/out.py") + + def test_nested_gitignore(self, git_repo: Path): + _write_tracked(git_repo, "pkg/__init__.py") + nested_ignore = git_repo / "pkg" / ".gitignore" + nested_ignore.write_text("local_cache/\n") + _git(git_repo, "add", "pkg/.gitignore") + cache = git_repo / "pkg" / "local_cache" / "x.py" + cache.parent.mkdir() + cache.write_text("x\n") + + index = GitAwareFileIndex(git_repo) + assert "pkg/__init__.py" in index.relative_files() + assert "pkg/local_cache/x.py" not in index.relative_files() + + def test_wildcard_min_js(self, git_repo: Path): + (git_repo / ".gitignore").write_text("*.min.js\n") + _write_tracked(git_repo, "app.js", "ok\n") + minified = git_repo / "app.min.js" + minified.write_text("min\n") + + index = GitAwareFileIndex(git_repo) + assert "app.js" in index.relative_files() + assert "app.min.js" not in index.relative_files() + + def test_anchored_build(self, git_repo: Path): + (git_repo / ".gitignore").write_text("/build/\n") + _write_tracked(git_repo, "src/ok.py") + nested_build = git_repo / "pkg" / "build" / "x.py" + nested_build.parent.mkdir(parents=True) + nested_build.write_text("x\n") + _git(git_repo, "add", "-f", "pkg/build/x.py") + root_build = git_repo / "build" / "out.py" + root_build.parent.mkdir() + root_build.write_text("y\n") + + index = GitAwareFileIndex(git_repo) + assert "pkg/build/x.py" in index.relative_files() + assert "build/out.py" not in index.relative_files() + + def test_negation(self, git_repo: Path): + (git_repo / ".gitignore").write_text("dist/*\n!dist/keep.js\n") + keep = git_repo / "dist" / "keep.js" + drop = git_repo / "dist" / "drop.js" + keep.parent.mkdir() + keep.write_text("keep\n") + drop.write_text("drop\n") + _git(git_repo, "add", "-f", "dist/keep.js") + + index = GitAwareFileIndex(git_repo) + assert "dist/keep.js" in index.relative_files() + assert "dist/drop.js" not in index.relative_files() + + def test_force_added_tracked_but_ignored(self, git_repo: Path): + (git_repo / ".gitignore").write_text("secret.env\n") + secret = git_repo / "secret.env" + secret.write_text("TOKEN=1\n") + _git(git_repo, "add", "-f", "secret.env") + + index = GitAwareFileIndex(git_repo) + # Tracked but matching ignore must be excluded from assessment (#453) + assert "secret.env" not in index.relative_files() + assert index.is_ignored("secret.env") + assert not index.exists("secret.env") + + def test_spaces_and_unicode_filenames(self, git_repo: Path): + spaced = "docs/my file.md" + unicode_name = "src/café.py" + _write_tracked(git_repo, spaced, "# hi\n") + _write_tracked(git_repo, unicode_name, "x=1\n") + + index = GitAwareFileIndex(git_repo) + assert spaced in index.relative_files() + assert unicode_name in index.relative_files() + + def test_no_gitignore_includes_tracked_files(self, git_repo: Path): + _write_tracked(git_repo, "a.py", "a\n") + _write_tracked(git_repo, "b.py", "b\n") + + index = GitAwareFileIndex(git_repo) + assert index.relative_files() == frozenset({"a.py", "b.py"}) + + def test_two_repos_do_not_leak_ignore_state(self, tmp_path: Path): + repo_a = tmp_path / "a" + repo_b = tmp_path / "b" + for repo, rule in ((repo_a, "ignored_a/\n"), (repo_b, "ignored_b/\n")): + repo.mkdir() + _git(repo, "init") + _git(repo, "config", "user.email", "t@example.com") + _git(repo, "config", "user.name", "T") + (repo / ".gitignore").write_text(rule) + _write_tracked(repo, "ok.py", "x\n") + ignored = repo / rule.strip().rstrip("/") / "x.py" + ignored.parent.mkdir() + ignored.write_text("no\n") + + index_a = GitAwareFileIndex(repo_a) + index_b = GitAwareFileIndex(repo_b) + assert "ok.py" in index_a.relative_files() + assert "ignored_a/x.py" not in index_a.relative_files() + assert "ignored_b/x.py" not in index_b.relative_files() + assert "ok.py" in index_b.relative_files() + # Ensure separate caches + assert index_a.relative_files() is not index_b.relative_files() + + def test_ignored_generated_source_does_not_affect_language_totals( + self, git_repo: Path + ): + (git_repo / ".gitignore").write_text("generated/\n") + for i in range(3): + _write_tracked(git_repo, f"src/m{i}.py", f"x={i}\n") + gen = git_repo / "generated" + gen.mkdir() + for i in range(20): + (gen / f"g{i}.py").write_text("print(1)\n") + + detector = LanguageDetector(git_repo) + languages = detector.detect_languages() + assert languages.get("Python") == 3 + assert all( + not f.startswith("generated/") for f in detector.file_index.iter_files() + ) + + def test_git_command_failure_in_real_worktree(self, git_repo: Path, monkeypatch): + """Unexpected Git failures in a real worktree fail closed (no rglob fallback).""" + _write_tracked(git_repo, "ok.py", "x\n") + + def selective_boom(cmd, **kwargs): + if list(cmd)[:2] == ["git", "rev-parse"]: + result = type("R", (), {})() + result.returncode = 0 + result.stdout = "true\n" + result.stderr = "" + return result + raise RuntimeError("git exploded") + + monkeypatch.setattr( + "agentready.services.git_aware_file_index.safe_subprocess_run", + selective_boom, + ) + index = GitAwareFileIndex(git_repo) + with pytest.raises(GitAwareFileIndexError): + index.relative_files() + + def test_dotfile_paths_are_not_stripped_by_normalize(self, git_repo: Path): + """Paths like .github/... must remain intact (no str.lstrip bug).""" + _write_tracked(git_repo, ".github/PULL_REQUEST_TEMPLATE.md", "pr\n") + index = GitAwareFileIndex(git_repo) + assert index.contains(".github/PULL_REQUEST_TEMPLATE.md") + assert index.exists(".github/PULL_REQUEST_TEMPLATE.md") + + def test_stub_git_dir_lists_fixture_files(self, tmp_path: Path): + """Unit fixtures that only mkdir .git still expose on-disk files.""" + repo = tmp_path / "fixture" + repo.mkdir() + (repo / ".git").mkdir() + (repo / "src").mkdir() + (repo / "src" / "main.py").write_text("print(1)\n") + + index = GitAwareFileIndex(repo) + assert "src/main.py" in index.relative_files() + + +class TestAssessorIgnoresGeneratedTree: + def test_type_annotations_ignore_generated_python(self, git_repo: Path): + """Ignored generated/*.py must not change type-annotation scoring.""" + from agentready.assessors.code_quality import TypeAnnotationsAssessor + from agentready.models.repository import Repository + + (git_repo / ".gitignore").write_text("generated/\n") + src = git_repo / "src" + src.mkdir() + (src / "main.py").write_text( + "def greet(name: str) -> str:\n" " return name\n" + ) + gen = git_repo / "generated" + gen.mkdir() + # Untyped functions that would tank the score if scanned + (gen / "junk.py").write_text( + "def a(x):\n" + " return x\n" + "def b(x):\n" + " return x\n" + "def c(x):\n" + " return x\n" + ) + _git(git_repo, "add", ".gitignore", "src/main.py") + + repo = Repository( + path=git_repo, + name="typed", + url=None, + branch="main", + commit_hash="abc", + languages={"Python": 1}, + total_files=2, + total_lines=10, + ) + finding = TypeAnnotationsAssessor().assess(repo) + assert "generated/" not in " ".join(finding.evidence or []) + # Single fully-typed function → high score; ignored junk must not dilute it + assert finding.score is not None and finding.score >= 90.0