From d6d052863a1f59bf49016cb30ff2f2689f299b92 Mon Sep 17 00:00:00 2001 From: Jier Date: Sun, 13 Sep 2026 22:59:31 +0200 Subject: [PATCH 1/3] refactor: honor suppression in the shared role-analysis path Change set: role_analysis.py, role_orchestrator.py, document_collection.py, scan/collection.py, CLAIMS.md, + new tests/suppression/test_analysis_path_parity.py.add RAG and web app Kusto query examples --- CLAIMS.md | 34 +++++++++ docsible/commands/document_collection.py | 6 +- .../orchestrators/role_orchestrator.py | 25 +++---- .../commands/document_role/role_analysis.py | 13 +++- docsible/commands/scan/collection.py | 6 +- .../suppression/test_analysis_path_parity.py | 75 +++++++++++++++++++ 6 files changed, 136 insertions(+), 23 deletions(-) create mode 100644 tests/suppression/test_analysis_path_parity.py diff --git a/CLAIMS.md b/CLAIMS.md index 408b6dc..5bdcda7 100644 --- a/CLAIMS.md +++ b/CLAIMS.md @@ -392,6 +392,40 @@ 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. +- Known caveat found while testing: the suppression store resolves to + `/.docsible/suppress.yml`, but a role's `.docsible` is normally a + metadata *file*, so a per-role store cannot sit at `role/.docsible/` for roles + that have one (only a project-root `.docsible/` directory works). Single-role + usage (where `role_path` is often the project root) masks this; the + collection/scan paths expose it. Unify the store location in the same + collection/CI pass. + ## Remaining Duplication Work The source-only duplication scan is below the original baseline, but remaining diff --git a/docsible/commands/document_collection.py b/docsible/commands/document_collection.py index eb8d25e..961bc61 100644 --- a/docsible/commands/document_collection.py +++ b/docsible/commands/document_collection.py @@ -209,8 +209,10 @@ 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, including suppression). + analysis = analyze_role( + role_info, role_path, min_confidence=0.7, apply_suppressions=True + ) 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..d80aedb 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,7 @@ def analyze_role( include_patterns: bool = False, min_confidence: float = 0.7, cached_complexity_report: ComplexityReport | None = None, + apply_suppressions: bool = False, ) -> RoleAnalysis: """Compute complexity (incl. execution graph) and recommendations. @@ -57,6 +59,9 @@ 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 (via the + suppression store for ``role_path``) so all callers honor + suppression from one place; also returned as ``suppressed`` Returns: RoleAnalysis with the complexity report, recommendations, and the @@ -71,10 +76,16 @@ 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=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..e7404e5 100644 --- a/docsible/commands/scan/collection.py +++ b/docsible/commands/scan/collection.py @@ -80,10 +80,8 @@ 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. + analysis = analyze_role(role_info, role_path, min_confidence=0.7, apply_suppressions=True) complexity_report = analysis.complexity_report complexity = _complexity_label(complexity_report.category.value) recommendations = analysis.recommendations diff --git a/tests/suppression/test_analysis_path_parity.py b/tests/suppression/test_analysis_path_parity.py new file mode 100644 index 0000000..60964cb --- /dev/null +++ b/tests/suppression/test_analysis_path_parity.py @@ -0,0 +1,75 @@ +"""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 + + web_role = collection / "roles" / "web_role" + # The role's `.docsible` is a metadata file; the suppression store needs + # `.docsible/` as a directory, so drop the file in this throwaway copy. + (web_role / ".docsible").unlink(missing_ok=True) + (web_role / ".docsible").mkdir() + (web_role / ".docsible" / "suppress.yml").write_text(RULE) + + assert info_count() < before # scan now honors the suppression From be69d828b96ab72a79c28f4b549f5d301678803a Mon Sep 17 00:00:00 2001 From: Jier Date: Sun, 13 Sep 2026 23:17:38 +0200 Subject: [PATCH 2/3] =?UTF-8?q?-=20Single-role=20keeps=20role=5Fpath=20as?= =?UTF-8?q?=20base=20(correct=20for=20--role=20.),=20and=20I=20recorded=20?= =?UTF-8?q?the=20one=20residual=20honestly:=20absolute=20--role=20/abs/pat?= =?UTF-8?q?h=20single-role=20still=20resolves=20from=20the=20role=20dir,?= =?UTF-8?q?=20not=20cwd=20=E2=80=94=20a=20small=20follow-up,=20not=20a=20r?= =?UTF-8?q?egression.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CLAIMS.md | 18 ++++++---- docsible/commands/document_collection.py | 8 +++-- .../commands/document_role/role_analysis.py | 15 ++++++--- docsible/commands/scan/collection.py | 17 +++++++--- .../suppression/test_analysis_path_parity.py | 33 ++++++++++++++----- 5 files changed, 66 insertions(+), 25 deletions(-) diff --git a/CLAIMS.md b/CLAIMS.md index 5bdcda7..3f57509 100644 --- a/CLAIMS.md +++ b/CLAIMS.md @@ -418,13 +418,17 @@ Tracked sequencing (technical ordering, not a dated roadmap): 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. -- Known caveat found while testing: the suppression store resolves to - `/.docsible/suppress.yml`, but a role's `.docsible` is normally a - metadata *file*, so a per-role store cannot sit at `role/.docsible/` for roles - that have one (only a project-root `.docsible/` directory works). Single-role - usage (where `role_path` is often the project root) masks this; the - collection/scan paths expose it. Unify the store location in the same - collection/CI pass. +- 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 diff --git a/docsible/commands/document_collection.py b/docsible/commands/document_collection.py index 961bc61..451d3c5 100644 --- a/docsible/commands/document_collection.py +++ b/docsible/commands/document_collection.py @@ -209,9 +209,13 @@ def document_collection_roles( # Analyze complexity, execution graph, and recommendations — # identical to standalone `document role` and `scan collection` - # (shared analyze_role, including suppression). + # (shared analyze_role; suppression resolved from the collection root). analysis = analyze_role( - role_info, role_path, min_confidence=0.7, apply_suppressions=True + role_info, + role_path, + min_confidence=0.7, + apply_suppressions=True, + suppress_base_path=collection_root, ) role_readme_path = role_path / output diff --git a/docsible/commands/document_role/role_analysis.py b/docsible/commands/document_role/role_analysis.py index d80aedb..43e8496 100644 --- a/docsible/commands/document_role/role_analysis.py +++ b/docsible/commands/document_role/role_analysis.py @@ -48,6 +48,7 @@ def analyze_role( 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. @@ -59,9 +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 (via the - suppression store for ``role_path``) so all callers honor - suppression from one place; also returned as ``suppressed`` + 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 @@ -80,7 +85,9 @@ def analyze_role( if apply_suppressions: from docsible.suppression.engine import apply_suppressions as _filter - recommendations, suppressed = _filter(recommendations, base_path=role_path) + recommendations, suppressed = _filter( + recommendations, base_path=suppress_base_path or role_path + ) return RoleAnalysis( complexity_report=complexity_report, recommendations=recommendations, diff --git a/docsible/commands/scan/collection.py b/docsible/commands/scan/collection.py index e7404e5..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,8 +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, including suppression. - analysis = analyze_role(role_info, role_path, min_confidence=0.7, apply_suppressions=True) + # `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 @@ -223,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 index 60964cb..d5184a2 100644 --- a/tests/suppression/test_analysis_path_parity.py +++ b/tests/suppression/test_analysis_path_parity.py @@ -65,11 +65,28 @@ def info_count() -> int: before = info_count() assert before >= 1 # the "examples/ directory" info finding exists - web_role = collection / "roles" / "web_role" - # The role's `.docsible` is a metadata file; the suppression store needs - # `.docsible/` as a directory, so drop the file in this throwaway copy. - (web_role / ".docsible").unlink(missing_ok=True) - (web_role / ".docsible").mkdir() - (web_role / ".docsible" / "suppress.yml").write_text(RULE) - - assert info_count() < before # scan now honors the suppression + # 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) From a0b9a66d52b3fa4d32dec59d38e94e94674bb430 Mon Sep 17 00:00:00 2001 From: Jier Date: Sun, 13 Sep 2026 23:45:26 +0200 Subject: [PATCH 3/3] docs: refresh jscpd baseline (17 clones / 1.02%) and record clone grouping --- AGENTS.md | 2 +- CLAIMS.md | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) 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 3f57509..c3c20e6 100644 --- a/CLAIMS.md +++ b/CLAIMS.md @@ -461,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