diff --git a/AGENTS.md b/AGENTS.md index 48eba18..b92253c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -32,7 +32,7 @@ Run the source-only duplication scan when it is useful: npx --yes jscpd docsible --pattern "**/*.py" ``` -`jscpd` is informational: its current baseline is 21 clones and 1.30% duplicated lines, not a zero-threshold gate. Update the baseline only after reviewing intentional duplication. +`jscpd` is informational, not a zero-threshold gate. Current baseline (2026-09-13): 17 clones and 1.02% duplicated lines (down from 21 / 1.30%). Update this baseline only after reviewing intentional duplication — the current grouping and the one known dedup candidate (`role_orchestrator._render_documentation` ↔ `role_analysis.render_analyzed_role`) are recorded in `CLAIMS.md`. ## Verified Baseline (2026-09-13) diff --git a/CLAIMS.md b/CLAIMS.md index 408b6dc..c3c20e6 100644 --- a/CLAIMS.md +++ b/CLAIMS.md @@ -392,6 +392,44 @@ smaller/synthetic test collection did not surface: `dev-sec/ansible-collection-hardening`, which has 4 real roles + 2 empty submodule dirs under `roles/`.) +## Analysis-path status & merge sequencing + +Single-role `document role`, `document role --collection`, and `scan collection` +now share `analyze_role()` for complexity, execution graph, and recommendations. +As of this change they also share **suppression**: `analyze_role()` filters in +one place, so a suppressed finding is consistently excluded from the terminal +view, CI gates, collection role counts, and scan counts (previously only the +single-role orchestrator applied suppression). + +Tracked sequencing (technical ordering, not a dated roadmap): +- Suppression is now shared, but `document role --collection` still lacks the + `fail_on` exit gate that single-role `document role` has. Add it when the + collection document loop is restructured (collection branch), so CI behaviour + is consistent across paths. +- Removing the deprecated `docsible role` command and retiring `RoleInfoBuilder` + is the only *breaking* change and belongs in the 1.0.0 cut. The + `docsible guide` guides (`getting-started`, `smart-defaults`, + `troubleshooting`) teach `docsible role` exclusively and their tests + (`test_guide_command`, `test_brief_help`, `test_cli_integration`) reference it, + so the guide rewrite must land in the same change that removes the command. +- The hybrid template (`hybrid_modular.jinja2`) does not render the Execution + Graph Summary / Execution Routes sections that the standard template does; + reconcile it before freezing the public graph contract, so "the graph is the + single source" holds for every output path. +- Freeze the JSON graph contract only *after* the collection cross-role work, so + it locks the final node/edge shape. +- Suppression store resolution (fixed in step 3a). Previously `analyze_role` + always read the store from `role_path`, while `docsible suppress add` writes + to the **working-directory** `.docsible/suppress.yml`. `scan collection` and + `document role --collection` now pass `suppress_base_path = `, + so they read the one project/collection-root store and scope per role via a + rule's `--file` — matching the documented model (and the metadata-`.docsible` + file vs store-directory collision can no longer occur for collection roles). + Residual: single-role `analyze_role` still defaults its base to `role_path`, + which equals the project root for the common `--role .` invocation but not for + an absolute `--role /abs/path`; unifying that (e.g. walking up to the nearest + `.docsible/`) is a small follow-up, not a regression. + ## Remaining Duplication Work The source-only duplication scan is below the original baseline, but remaining @@ -423,6 +461,35 @@ duplication is prioritized by ownership and behavior rather than percentage. 6. Remove obsolete duplicate tests and generated fixture backups only after confirming they are not test contracts. +### jscpd (source-only, `docsible/**/*.py`) — 2026-09-13 + +17 clones / 258 duplicated lines (1.02%) / 1414 tokens (1.22%), down from the +prior baseline of 21 clones / 1.30%. Grouped by owner, mapped to the items +above; the ones to actually act on are called out: + +- **`role_orchestrator._render_documentation` ↔ `role_analysis.render_analyzed_role`** + (the `ReadmeRenderer(...).render_role(...)` assembly) — **introduced by the + step‑3 consolidation**: two render assemblies where there should be one. + Action: have the orchestrator delegate to `render_analyzed_role` so a single + path renders. Ties to item 1. +- **`commands/analyze/role.py` ↔ `commands/validate/role.py`** — the two thin + intent-command wrappers duplicate the same option-stack decoration. Low-risk + extraction candidate (a shared decorator), cosmetic. +- **`commands/document/role.py` ↔ `commands/legacy/role.py`** — legacy + duplication; expected to vanish when step‑3b removes `docsible role` + (item 3). +- **`renderers/models/diagram_data.py` ↔ `renderers/models/render_context.py`** + and `readme_renderer.py` internal (`render_role` ↔ `render_collection`) — + renderer-model / renderer-method overlap → item 5. +- Remaining intra-file repeats in `diagrams/mermaid/core.py`, + `diagrams/sequence/role.py`, `diagrams/types/formatters.py`, + `repositories/role_repository.py`, `utils/cache.py` — pre-existing, no owner + overlap with the graph work; leave unless a change touches them. + +Only the first item (`_render_documentation` / `render_analyzed_role`) is a +regression *from* our consolidation and worth folding into the dedup pass; the +rest are either legacy-to-be-removed or pre-existing. + ## Scope of This Document This file records observable project state, commands verified for this diff --git a/docsible/commands/document_collection.py b/docsible/commands/document_collection.py index eb8d25e..451d3c5 100644 --- a/docsible/commands/document_collection.py +++ b/docsible/commands/document_collection.py @@ -209,8 +209,14 @@ def document_collection_roles( # Analyze complexity, execution graph, and recommendations — # identical to standalone `document role` and `scan collection` - # (previously this was skipped entirely for collection roles). - analysis = analyze_role(role_info, role_path, min_confidence=0.7) + # (shared analyze_role; suppression resolved from the collection root). + analysis = analyze_role( + role_info, + role_path, + min_confidence=0.7, + apply_suppressions=True, + suppress_base_path=collection_root, + ) role_readme_path = role_path / output template_type = "hybrid" if hybrid else "standard_modular" diff --git a/docsible/commands/document_role/orchestrators/role_orchestrator.py b/docsible/commands/document_role/orchestrators/role_orchestrator.py index f37f541..36ae135 100644 --- a/docsible/commands/document_role/orchestrators/role_orchestrator.py +++ b/docsible/commands/document_role/orchestrators/role_orchestrator.py @@ -89,24 +89,16 @@ def execute(self) -> None: ): self._validate_documentation(role_info, analysis_report, diagrams, dependency_data) - # Step 7.5: Recommendations were already computed alongside complexity - # in step 4 (shared analyze_role()), using the validated role_path. + # Step 7.5: Recommendations were computed and suppression applied inside + # the shared analyze_role() (step 4), so single-role, --collection, and + # scan all honor suppression from one place. recommendations = analysis.recommendations - - if self.context.analysis.apply_suppressions: - from docsible.suppression.engine import apply_suppressions - - recommendations, suppressed = apply_suppressions( - recommendations, - base_path=role_path, + suppressed = analysis.suppressed + if suppressed and self.context.analysis.output_format != "json": + click.echo( + f" ({len(suppressed)} recommendation(s) suppressed" + f" — see 'docsible suppress list')" ) - if suppressed and self.context.analysis.output_format != "json": - click.echo( - f" ({len(suppressed)} recommendation(s) suppressed" - f" — see 'docsible suppress list')" - ) - else: - suppressed = [] if recommendations or self.context.analysis.output_format == "json": self._display_recommendations(recommendations, analysis_report) @@ -244,6 +236,7 @@ def _analyze_role(self, role_info: dict, role_path: Path): include_patterns=self.context.analysis.simplification_report, min_confidence=0.7, cached_complexity_report=self.context.analysis.cached_complexity_report, + apply_suppressions=self.context.analysis.apply_suppressions, ) def _display_analysis_and_exit(self, analysis_report, role_info: dict) -> None: diff --git a/docsible/commands/document_role/role_analysis.py b/docsible/commands/document_role/role_analysis.py index 43b1602..43e8496 100644 --- a/docsible/commands/document_role/role_analysis.py +++ b/docsible/commands/document_role/role_analysis.py @@ -19,7 +19,7 @@ from __future__ import annotations -from dataclasses import dataclass +from dataclasses import dataclass, field from pathlib import Path from typing import Any @@ -37,6 +37,7 @@ class RoleAnalysis: complexity_report: ComplexityReport recommendations: list[Recommendation] execution_graph: Any = None + suppressed: list[Recommendation] = field(default_factory=list) def analyze_role( @@ -46,6 +47,8 @@ def analyze_role( include_patterns: bool = False, min_confidence: float = 0.7, cached_complexity_report: ComplexityReport | None = None, + apply_suppressions: bool = False, + suppress_base_path: Path | None = None, ) -> RoleAnalysis: """Compute complexity (incl. execution graph) and recommendations. @@ -57,6 +60,13 @@ def analyze_role( min_confidence: Minimum confidence for pattern detection cached_complexity_report: Reuse an already-computed report (e.g. from smart defaults) instead of analyzing again + apply_suppressions: Filter suppressed recommendations here so all + callers (single-role, --collection, scan) honor suppression the + same way, from one place + suppress_base_path: Project/collection root that owns the + `.docsible/suppress.yml` store (rules scope per-role via `--file`). + Defaults to ``role_path`` when omitted. Pass the shared root from + scan/collection so they read one project store, not a per-role one. Returns: RoleAnalysis with the complexity report, recommendations, and the @@ -71,10 +81,18 @@ def analyze_role( execution_graph=execution_graph, ) recommendations = generate_all_recommendations(role_path, complexity_report) + suppressed: list[Recommendation] = [] + if apply_suppressions: + from docsible.suppression.engine import apply_suppressions as _filter + + recommendations, suppressed = _filter( + recommendations, base_path=suppress_base_path or role_path + ) return RoleAnalysis( complexity_report=complexity_report, recommendations=recommendations, execution_graph=execution_graph, + suppressed=suppressed, ) diff --git a/docsible/commands/scan/collection.py b/docsible/commands/scan/collection.py index afbaf46..e1372cc 100644 --- a/docsible/commands/scan/collection.py +++ b/docsible/commands/scan/collection.py @@ -38,12 +38,14 @@ def _complexity_label(category_value: str) -> str: return _COMPLEXITY_MAP.get(category_value.lower(), "unknown") -def _analyse_role(role_path: Path, git_info: dict) -> RoleResult: +def _analyse_role(role_path: Path, git_info: dict, collection_root: Path) -> RoleResult: """Run analysis on a single role and return a RoleResult. Args: role_path: Absolute path to the role directory. git_info: Pre-fetched git repository info (cached at collection level). + collection_root: Project root owning the shared `.docsible/suppress.yml` + store (per-role scoping happens through a rule's `--file`). Returns: RoleResult with metrics and findings. @@ -80,10 +82,15 @@ def _analyse_role(role_path: Path, git_info: dict) -> RoleResult: variable_count = defaults_count + vars_count # Complexity + recommendations — shared with `document role` and - # `document role --collection` so all three agree (this previously called - # generate_all_recommendations() without the complexity report, missing - # the graph-aware findings the other two paths already had). - analysis = analyze_role(role_info, role_path, min_confidence=0.7) + # `document role --collection` so all three agree, including suppression + # resolved from the one collection/project-root store. + analysis = analyze_role( + role_info, + role_path, + min_confidence=0.7, + apply_suppressions=True, + suppress_base_path=collection_root, + ) complexity_report = analysis.complexity_report complexity = _complexity_label(complexity_report.category.value) recommendations = analysis.recommendations @@ -225,7 +232,7 @@ def scan_collection_cmd( role_results: list[RoleResult] = [] for role_path in sorted(role_paths): try: - result = _analyse_role(role_path, git_info) + result = _analyse_role(role_path, git_info, collection_path) role_results.append(result) logger.debug(f"Scanned role: {role_path.name}") except Exception as exc: diff --git a/tests/suppression/test_analysis_path_parity.py b/tests/suppression/test_analysis_path_parity.py new file mode 100644 index 0000000..d5184a2 --- /dev/null +++ b/tests/suppression/test_analysis_path_parity.py @@ -0,0 +1,92 @@ +"""Suppression must apply uniformly across the three analysis entry points. + +Previously only the single-role orchestrator filtered suppressed findings; +`scan collection` and `document role --collection` recomputed recommendations +via analyze_role without suppressing, so a suppressed finding still showed up. +Step 3a moves suppression into the shared analyze_role. +""" + +from __future__ import annotations + +import json +import shutil +from pathlib import Path + +from click.testing import CliRunner + +from docsible.cli import cli +from docsible.commands.document_role.role_analysis import analyze_role +from docsible.commands.role_info_loader import RoleInfoLoader + +FIXTURES = Path(__file__).parent.parent / "fixtures" +MINIMAL_COLLECTION = FIXTURES / "minimal_collection" + +RULE = ( + "rules:\n" + " - id: test1\n" + ' pattern: "examples"\n' + " reason: suppressed in test\n" +) + + +def _write_role(root: Path) -> Path: + (root / "tasks").mkdir(parents=True) + (root / "defaults").mkdir() + (root / "handlers").mkdir() + (root / "tasks" / "main.yml").write_text("---\n- name: t\n debug:\n msg: x\n") + (root / "defaults" / "main.yml").write_text("---\nk: v\n") + (root / "handlers" / "main.yml").write_text("---\n") + return root + + +def test_analyze_role_applies_suppression_when_enabled(tmp_path): + role = _write_role(tmp_path / "r") + (role / ".docsible").mkdir() + (role / ".docsible" / "suppress.yml").write_text(RULE) + + role_info = RoleInfoLoader().load(role) + unfiltered = analyze_role(role_info, role) + filtered = analyze_role(role_info, role, apply_suppressions=True) + + assert any("examples" in r.message for r in unfiltered.recommendations) + assert not any("examples" in r.message for r in filtered.recommendations) + assert any("examples" in r.message for r in filtered.suppressed) + + +def test_scan_collection_honours_suppression(tmp_path): + collection = tmp_path / "collection" + shutil.copytree(MINIMAL_COLLECTION, collection) + + def info_count() -> int: + result = CliRunner().invoke(cli, ["scan", "collection", str(collection), "--output-format", "json"]) + roles = {r["name"]: r for r in json.loads(result.output)["roles"]} + return roles["web_role"]["info_count"] + + before = info_count() + assert before >= 1 # the "examples/ directory" info finding exists + + # One project/collection-root store; rules scope per role via `--file`. + (collection / ".docsible").mkdir() + (collection / ".docsible" / "suppress.yml").write_text(RULE) + + assert info_count() < before # scan now reads the collection-root store + + +def test_suppress_base_path_controls_which_store_is_read(tmp_path): + role = _write_role(tmp_path / "proj" / "roles" / "r") # role has no store + root = tmp_path / "proj" + (root / ".docsible").mkdir(parents=True) + (root / ".docsible" / "suppress.yml").write_text(RULE) # store at project root + + role_info = RoleInfoLoader().load(role) + + # Default base is the role dir -> the project-root store is not read. + by_role = analyze_role(role_info, role, apply_suppressions=True) + assert any("examples" in r.message for r in by_role.recommendations) + + # Explicit project root -> suppression applies. + by_root = analyze_role( + role_info, role, apply_suppressions=True, suppress_base_path=root + ) + assert not any("examples" in r.message for r in by_root.recommendations) + assert any("examples" in r.message for r in by_root.suppressed)