diff --git a/CHANGELOG.md b/CHANGELOG.md index bf11a672..32268a8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Removed +- **BREAKING:** `audit_openssf_baseline` no longer accepts `auto_init_config`, + and `get_pending_data` no longer accepts `level` (#528). Both were ignored: + the audit never created a project file, and pending questions were never + filtered by maturity level. Create an empty `.project/darnit.yaml` with + `init_project_config`. `get_pending_context` no longer accepts `level`; + that ignored parameter was removed in #552. - `.baseline.toml` reading and the code that existed only for it: `load_user_config` (including its `trusted` path), `load_user_config_with_report`, `validate_user_config`, `deep_merge`, and diff --git a/packages/darnit-baseline/src/darnit_baseline/tools.py b/packages/darnit-baseline/src/darnit_baseline/tools.py index 4f68a031..cf57536c 100644 --- a/packages/darnit-baseline/src/darnit_baseline/tools.py +++ b/packages/darnit-baseline/src/darnit_baseline/tools.py @@ -93,7 +93,6 @@ def audit_openssf_baseline( level: int = 3, tags: str | list[str] | None = None, output_format: str = "markdown", - auto_init_config: bool = True, attest: bool = False, sign_attestation: bool = True, staging: bool = False, @@ -121,7 +120,6 @@ def audit_openssf_baseline( Default: "markdown". Use "summary" for compact JSON (id/status/level/details only, no evidence). Use "badge" to generate an OpenSSF Best Practices Badge automation-proposal URL. - auto_init_config: Create .project.yaml if missing. Default: True attest: Generate in-toto attestation after audit. Default: False sign_attestation: Sign attestation with Sigstore. Default: True staging: Use Sigstore staging environment. Default: False @@ -764,7 +762,6 @@ def _context_key_order() -> list[str]: def get_pending_data( local_path: str = ".", control_ids: list[str] | None = None, - level: int = 3, owner: str | None = None, repo: str | None = None, limit: int = 4, @@ -787,7 +784,6 @@ def get_pending_data( Parameters: - `local_path`: Path to repository (default: ".") - `control_ids`: Optional list of control IDs to check - - `level`: Maximum maturity level (1, 2, or 3) - `owner`: GitHub owner (auto-detected if not provided) - `repo`: GitHub repo name (auto-detected if not provided) - `limit`: Max questions to return per batch (default: 4). Use 0 for all. diff --git a/packages/darnit/src/darnit/skills/darnit-data/SKILL.md b/packages/darnit/src/darnit/skills/darnit-data/SKILL.md index fca07e2f..7a6734e9 100644 --- a/packages/darnit/src/darnit/skills/darnit-data/SKILL.md +++ b/packages/darnit/src/darnit/skills/darnit-data/SKILL.md @@ -13,7 +13,7 @@ Guide the user through providing missing project data that darnit needs for accu ## Workflow -1. Call `get_pending_data` MCP tool with `local_path` set to the repository root and `level` set to `3`. +1. Call `get_pending_data` MCP tool with `local_path` set to the repository root. - If the user mentioned a profile, pass it as the `profile` parameter to scope questions to that profile's controls. - This call writes nothing. diff --git a/tests/darnit/parity/tier1/conftest.py b/tests/darnit/parity/tier1/conftest.py index 04078dc0..2b9c05ef 100644 --- a/tests/darnit/parity/tier1/conftest.py +++ b/tests/darnit/parity/tier1/conftest.py @@ -138,7 +138,6 @@ def mcp_tool_result(prepared_fixture: Path) -> AuditResult: local_path=str(prepared_fixture), level=3, output_format="json", - auto_init_config=False, attest=False, prefer_upstream=False, ) diff --git a/tests/darnit/parity/tier1/test_no_product_changes.py b/tests/darnit/parity/tier1/test_no_product_changes.py index f820f9ba..723f5da0 100644 --- a/tests/darnit/parity/tier1/test_no_product_changes.py +++ b/tests/darnit/parity/tier1/test_no_product_changes.py @@ -13,6 +13,7 @@ from __future__ import annotations import os +import re import subprocess import pytest @@ -75,6 +76,30 @@ def _base_ref() -> str | None: return _find_reachable_base() +# Issue #528 dropped the ignored ``auto_init_config`` parameter. The parity +# callers had to stop passing it. Those mechanical edits are not a parity +# feature. A harness file qualifies only when every changed line is one of +# the lines below, and the same change removes the parameter from the +# product signature. Any other added or removed line keeps the file in scope. +_AUTO_INIT_CONFIG_REMOVED_LINES = frozenset( + { + "auto_init_config=False,", + 'args["auto_init_config"] = False', + "# Force safe defaults.", + } +) +_AUTO_INIT_CONFIG_ADDED_LINES = frozenset( + { + 'args.pop("auto_init_config", None)', + "# Force safe defaults. Drop the removed no-op if a model still sends it.", + } +) +_AUTO_INIT_CONFIG_SIGNATURE = re.compile(r"^auto_init_config\s*:\s*bool\b") +_SELF = "tests/darnit/parity/tier1/test_no_product_changes.py" +_FIXTURES = "tests/darnit/parity/fixtures/" +_PRODUCT_PREFIXES = ("packages/darnit/src/", "packages/darnit-baseline/src/") + + def _is_ci() -> bool: """True on GitHub Actions and most other CI runners. @@ -85,6 +110,79 @@ def _is_ci() -> bool: return bool(os.environ.get("CI")) +def _diff_line_bodies(diff_text: str) -> tuple[list[str], list[str]]: + """Added and removed line bodies from a unified diff, headers excluded.""" + removed: list[str] = [] + added: list[str] = [] + for raw in diff_text.splitlines(): + if raw.startswith(("diff ", "index ", "---", "+++", "@@", "\\")): + continue + if raw.startswith("-"): + removed.append(raw[1:].strip()) + elif raw.startswith("+"): + added.append(raw[1:].strip()) + return removed, added + + +def _is_auto_init_config_callsite_diff(diff_text: str) -> bool: + """True when the diff only drops or pops the obsolete ``auto_init_config`` argument.""" + removed, added = _diff_line_bodies(diff_text) + if any(line not in _AUTO_INIT_CONFIG_REMOVED_LINES for line in removed): + return False + if any(line not in _AUTO_INIT_CONFIG_ADDED_LINES for line in added): + return False + removed_code = [line for line in removed if not line.startswith("#")] + added_code = [line for line in added if not line.startswith("#")] + if not removed_code: + return False + assignments = [line for line in removed_code if line == 'args["auto_init_config"] = False'] + kwargs = [line for line in removed_code if line == "auto_init_config=False,"] + pops = [line for line in added_code if line == 'args.pop("auto_init_config", None)'] + return len(assignments) + len(kwargs) == len(removed_code) and pops == added_code and len(pops) == len(assignments) + + +def _product_diff_removes_auto_init_config_parameter(product_diff: str) -> bool: + """True when a removed line is the ``auto_init_config`` signature parameter.""" + for raw in product_diff.splitlines(): + if raw.startswith("-") and not raw.startswith("---") and _AUTO_INIT_CONFIG_SIGNATURE.match(raw[1:].strip()): + return True + return False + + +def _harness_candidates(changed: list[str]) -> list[str]: + return [ + path + for path in changed + if path.startswith("tests/darnit/parity/") and not path.startswith(_FIXTURES) and path != _SELF + ] + + +def parity_harness_feature_paths( + changed: list[str], + harness_diffs: dict[str, str], + product_diff: str, +) -> list[str]: + """Parity paths that make this a feature-028 PR. + + Call-site updates for ``auto_init_config`` drop out only when the product + diff removes that parameter. A missing harness diff is not compatibility. + """ + candidates = _harness_candidates(changed) + if not candidates or not _product_diff_removes_auto_init_config_parameter(product_diff): + return candidates + return [path for path in candidates if not _is_auto_init_config_callsite_diff(harness_diffs.get(path, ""))] + + +def _git_diff(base: str, *paths: str) -> str: + rc = subprocess.run( + ["git", "diff", "--unified=0", f"{base}...HEAD", "--", *paths], + capture_output=True, + text=True, + check=True, + ) + return rc.stdout + + def test_no_product_source_changes() -> None: """FR-014: parity-tests PR MUST NOT modify product source. Test/config files are exempt so a build-config touch or a test refactor stays in @@ -130,12 +228,23 @@ def test_no_product_source_changes() -> None: # Exclude this file itself from the heuristic: a PR that only touches # the guard (to tune scope, adjust base-ref detection, etc.) is a # meta-change to the guard, not a parity-tests-feature PR. - _SELF = "tests/darnit/parity/tier1/test_no_product_changes.py" - _FIXTURES = "tests/darnit/parity/fixtures/" - touches_parity_harness = any( - f.startswith("tests/darnit/parity/") and not f.startswith(_FIXTURES) and f != _SELF for f in changed - ) - if not touches_parity_harness: + # + # Issue #528 also updates existing audit call sites so they no longer + # pass ``auto_init_config``. That is a compatibility edit, not a parity + # feature, when the product signature removes the parameter in this + # same change. The whole file stays in scope if any other line changes. + candidates = _harness_candidates(changed) + product_paths = [path for path in changed if path.startswith(_PRODUCT_PREFIXES)] + product_diff = _git_diff(base, *product_paths) if product_paths else "" + harness_diffs = {path: _git_diff(base, path) for path in candidates} + feature_paths = parity_harness_feature_paths(changed, harness_diffs, product_diff) + if not feature_paths: + if candidates: + pytest.skip( + "FR-014 check skipped: parity-tree edits only drop the removed " + "auto_init_config call sites, and the product signature drops " + "that parameter in this change.", + ) pytest.skip( "FR-014 check skipped: PR does not modify the parity harness " "(tests/darnit/parity/ outside fixtures/, other than this guard), " @@ -143,9 +252,7 @@ def test_no_product_source_changes() -> None: "guardrail does not apply.", ) - forbidden = [ - f for f in changed if (f.startswith("packages/darnit/src/") or f.startswith("packages/darnit-baseline/src/")) - ] + forbidden = [path for path in changed if path.startswith(_PRODUCT_PREFIXES)] assert not forbidden, ( "Feature 028 (parity tests) MUST NOT modify product source code " "(FR-014). Offending files:\n" @@ -153,3 +260,78 @@ def test_no_product_source_changes() -> None: + "\n\nIf a change under packages/*/src/ is genuinely necessary, " "split it into a separate PR that is not scoped to feature 028." ) + + +_PRODUCT = "packages/darnit-baseline/src/darnit_baseline/tools.py" +_SKILL = "packages/darnit/src/darnit/skills/darnit-data/SKILL.md" +_FIXTURE = "tests/darnit/parity/fixtures/all_pass_repo/parity.toml" +_CONFTEST = "tests/darnit/parity/tier1/conftest.py" +_RUN = "tests/darnit/parity/tier2/run.py" +_OPENAI = "tests/darnit/parity/tier2/backends/openai_backend.py" + +_SIGNATURE_DIFF = """\ +--- a/packages/darnit-baseline/src/darnit_baseline/tools.py ++++ b/packages/darnit-baseline/src/darnit_baseline/tools.py +@@ -96,1 +95,0 @@ def audit_openssf_baseline( +- auto_init_config: bool = True, +""" + +_KWARG_DIFF = """\ +--- a/tests/darnit/parity/tier1/conftest.py ++++ b/tests/darnit/parity/tier1/conftest.py +@@ -141,1 +140,0 @@ def mcp_tool_result( +- auto_init_config=False, +""" + +_POP_DIFF = """\ +--- a/tests/darnit/parity/tier2/backends/openai_backend.py ++++ b/tests/darnit/parity/tier2/backends/openai_backend.py +@@ -83,2 +83,2 @@ def _dispatch_tool_call( +- # Force safe defaults. +- args["auto_init_config"] = False ++ # Force safe defaults. Drop the removed no-op if a model still sends it. ++ args.pop("auto_init_config", None) +""" + + +def test_528_compatibility_edits_are_not_a_parity_feature() -> None: + """The three #528 call-site updates do not make this a parity-tests PR.""" + changed = [_PRODUCT, _SKILL, _CONFTEST, _RUN, _OPENAI] + diffs = {_CONFTEST: _KWARG_DIFF, _RUN: _KWARG_DIFF, _OPENAI: _POP_DIFF} + + assert parity_harness_feature_paths(changed, diffs, _SIGNATURE_DIFF) == [] + + +def test_harness_logic_change_with_product_edits_stays_in_scope() -> None: + """An extra assertion beside the call-site edit is still a parity-feature change.""" + tainted = _KWARG_DIFF + "+ assert report.is_green\n" + changed = [_PRODUCT, _CONFTEST] + + assert parity_harness_feature_paths(changed, {_CONFTEST: tainted}, _SIGNATURE_DIFF) == [_CONFTEST] + + +def test_product_only_change_is_not_a_parity_feature() -> None: + assert parity_harness_feature_paths([_PRODUCT, _SKILL], {}, _SIGNATURE_DIFF) == [] + + +def test_fixture_only_change_is_not_a_parity_feature() -> None: + changed = [_PRODUCT, _FIXTURE] + + assert parity_harness_feature_paths(changed, {}, _SIGNATURE_DIFF) == [] + + +def test_compatibility_without_signature_removal_stays_in_scope() -> None: + """A docstring mention is not removal of the signature parameter.""" + docstring_only = "- auto_init_config: Create .project.yaml if missing. Default: True\n" + changed = [_PRODUCT, _CONFTEST] + + assert parity_harness_feature_paths(changed, {_CONFTEST: _KWARG_DIFF}, docstring_only) == [_CONFTEST] + + +def test_unrelated_harness_lines_are_not_compatibility() -> None: + imported = _KWARG_DIFF + "+import os\n" + branched = _POP_DIFF + "+ if True:\n" + changed = [_PRODUCT, _CONFTEST, _OPENAI] + diffs = {_CONFTEST: imported, _OPENAI: branched} + + assert parity_harness_feature_paths(changed, diffs, _SIGNATURE_DIFF) == [_CONFTEST, _OPENAI] diff --git a/tests/darnit/parity/tier2/backends/openai_backend.py b/tests/darnit/parity/tier2/backends/openai_backend.py index e7ec09ac..ee0c5080 100644 --- a/tests/darnit/parity/tier2/backends/openai_backend.py +++ b/tests/darnit/parity/tier2/backends/openai_backend.py @@ -80,8 +80,8 @@ def _dispatch_tool_call(call: Any, fixture_dir: Path) -> str: args["local_path"] = str(fixture_dir) args["output_format"] = "json" args["level"] = 3 - # Force safe defaults. - args["auto_init_config"] = False + # Force safe defaults. Drop the removed no-op if a model still sends it. + args.pop("auto_init_config", None) args["attest"] = False args["prefer_upstream"] = False diff --git a/tests/darnit/parity/tier2/run.py b/tests/darnit/parity/tier2/run.py index 79b961c6..b21f7381 100644 --- a/tests/darnit/parity/tier2/run.py +++ b/tests/darnit/parity/tier2/run.py @@ -74,7 +74,6 @@ def _run_mcp_tool(fixture_dir: Path) -> tuple[str, AuditResult]: local_path=str(fixture_dir), level=3, output_format="json", - auto_init_config=False, attest=False, prefer_upstream=False, ) diff --git a/tests/darnit/tools/test_framework_metadata.py b/tests/darnit/tools/test_framework_metadata.py index 53a2bf18..03629621 100644 --- a/tests/darnit/tools/test_framework_metadata.py +++ b/tests/darnit/tools/test_framework_metadata.py @@ -110,7 +110,6 @@ def test_json_output_has_top_level_metadata_block(self, tmp_path, monkeypatch): raw = audit_openssf_baseline( local_path=str(tmp_path), output_format="json", - auto_init_config=False, attest=False, prefer_upstream=False, ) diff --git a/tests/darnit_baseline/test_tools_signatures.py b/tests/darnit_baseline/test_tools_signatures.py index 361ec105..ba140b8f 100644 --- a/tests/darnit_baseline/test_tools_signatures.py +++ b/tests/darnit_baseline/test_tools_signatures.py @@ -93,3 +93,39 @@ def test_audit_handles_missing_path(self) -> None: result = audit_openssf_baseline(local_path="/nonexistent/path/12345") assert "Error" in result or "not found" in result.lower() + + +class TestRemovedMcpParameters: + """Issue #528: ignored MCP parameters stay off the signatures and schemas.""" + + def test_signatures_omit_ignored_parameters(self) -> None: + import inspect + + from darnit_baseline.tools import audit_openssf_baseline, get_pending_data + + audit_params = inspect.signature(audit_openssf_baseline).parameters + pending_params = inspect.signature(get_pending_data).parameters + + assert "auto_init_config" not in audit_params + assert "level" in audit_params + assert "level" not in pending_params + assert "profile" in pending_params + + def test_mcp_schemas_omit_ignored_parameters(self) -> None: + import asyncio + from importlib.resources import files + from pathlib import Path + + from darnit.server.factory import create_server + + toml = Path(str(files("darnit_baseline") / "openssf-baseline.toml")) + server = create_server(toml) + tools = {tool.name: tool for tool in asyncio.run(server.list_tools())} + + audit_props = tools["audit_openssf_baseline"].inputSchema["properties"] + pending_props = tools["get_pending_data"].inputSchema["properties"] + + assert "auto_init_config" not in audit_props + assert "level" in audit_props + assert "level" not in pending_props + assert "profile" in pending_props diff --git a/vulture_allowlist.py b/vulture_allowlist.py index 29fd9a87..473dbe05 100644 --- a/vulture_allowlist.py +++ b/vulture_allowlist.py @@ -3,8 +3,6 @@ # These entries suppress vulture warnings for code that appears unused but is # intentionally kept. Vulture allowlists use attribute access to mark names used. -# MCP tool parameters: part of the public MCP tool schema, exposed to AI -# assistants even though the function body doesn't use them yet. -# Removing them would change the tool's API contract. -_ = auto_init_config # noqa: F821 # audit_openssf_baseline parameter +# MCP tool parameter kept on the public schema. Vulture allowlists use +# attribute access to mark a name used. _ = attest # noqa: F821 # audit_openssf_baseline parameter