From 1d56c2317f8a25d2b289b5f3e2604c5b46780b99 Mon Sep 17 00:00:00 2001 From: Amit Ray <51674969+amitray007@users.noreply.github.com> Date: Tue, 2 Jun 2026 15:18:59 +0530 Subject: [PATCH] chore: add externalized, self-verifying audit suppression mechanism [skip ci] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Recurring audit false positives (deliberate deprecated-alias methods, intentionally partial holiday enums, the backward-compat State.REMOVED value) previously reappeared on every run. They are now declared in a data file, specs/audit-ignore.json — nothing to ignore is hard-coded in the audit script. scripts/audit_sdk.py now: - builds structured findings with stable (type, key) fingerprints - loads specs/audit-ignore.json (override: --ignore-file; absent = none) - suppresses a finding ONLY while it still occurs; for enums only the listed values are hidden, so newly added values still surface - reports hidden findings under "Suppressed (Verified)" with reasons and surfaces entries that match nothing under "Stale Ignores" - excludes suppressed findings from the active counts (clean report) Also fixes scan_string_concat_issues to match its documented intent: flag implicit string concatenation only inside [ ] list displays (a real missing-comma bug, e.g. in nullable/mandatory lists), not in parenthesised assignments or call arguments. This removes 8 long-standing false positives at the source without needing ignore entries, while the check now actually catches missing commas in list literals. The maintain-audit skill (Phase 2) now re-verifies suppressions on every run: it re-confirms each ignore's reason against the current code/spec, recommends removing stale or no-longer-valid entries (resurfacing the real finding), and flags newly surfaced enum values not covered by an ignore — so the agent, not just the script, keeps the ignore list honest each audit. Report section headers and Coverage Summary keys are preserved so format_pr_comment.py keeps working; counts now reflect active findings. Adds tests/test_audit_ignore.py (28 tests) covering ignore loading, suppression, enum value-subset verification, stale detection, enum finding computation, and the list-context detector fix. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/skills/maintain-audit/SKILL.md | 45 ++++ CLAUDE.md | 14 +- scripts/audit_sdk.py | 340 +++++++++++++++++++++---- specs/audit-ignore.json | 53 ++++ tests/test_audit_ignore.py | 331 ++++++++++++++++++++++++ 5 files changed, 728 insertions(+), 55 deletions(-) create mode 100644 specs/audit-ignore.json create mode 100644 tests/test_audit_ignore.py diff --git a/.claude/skills/maintain-audit/SKILL.md b/.claude/skills/maintain-audit/SKILL.md index 7509972..e8bbce0 100644 --- a/.claude/skills/maintain-audit/SKILL.md +++ b/.claude/skills/maintain-audit/SKILL.md @@ -66,6 +66,8 @@ session. - Enum Staleness - Deprecation Notices - Code Issues + - Suppressed (Verified) + - Stale Ignores If any section is missing, the script may have errored — check stderr output. @@ -121,6 +123,42 @@ mentioned in release notes or the diff report are most likely to have issues. 16. **Verify extra SDK methods** — For methods the script flagged as having no OAS match, check if they map to deprecated, removed, or renamed endpoints in the spec. +### Re-verify suppressions (`specs/audit-ignore.json`) + +The audit script suppresses reviewed findings listed in `specs/audit-ignore.json` and re-checks +them **mechanically** every run: a suppression only hides a finding while that finding still +occurs, and any entry matching nothing is reported under **Stale Ignores**. The script cannot +judge whether a suppression's *reason* is still true — that is this phase's job. Do this every +audit; never assume a suppressed finding is still safe just because it is in the file. + +16a. **Re-confirm each active suppression.** Read `specs/audit-ignore.json` and the report's + **Suppressed (Verified)** section. For EVERY entry, re-read the referenced code/spec and + confirm the stated `reason` still holds — do not take it on trust: + - `extra_method`: the method still exists, still delegates to the canonical method, and still + emits a `DeprecationWarning`. If the spec has since ADDED a matching operation (so it is no + longer "extra"), or the method's behaviour changed, the ignore is no longer valid. + - `enum_staleness`: the documented rationale still applies (e.g. the country-split holiday + design; `State.REMOVED` kept for backward compatibility). If the SDK enum or the spec + changed so the rationale no longer fits, the ignore is no longer valid. + - any other type: the flagged construct is still intentional. + + For any entry whose reason no longer holds → **remove it from `specs/audit-ignore.json`** so + the finding resurfaces, and handle that finding as a normal Phase 3 item (apply the fix, do + not keep ignoring it). + +16b. **Act on Stale Ignores.** Every entry the script lists under **Stale Ignores** matched no + current finding — its condition is gone. Recommend **removing** it (a stale entry hides + nothing but rots the list). Keep one only if you can justify an imminent re-occurrence. + +16c. **Check for newly surfaced enum values.** For `enum_staleness` entries with an explicit + `values` list, look for the SAME enum key still appearing under the active **Enum Staleness** + section — the script surfaces values not covered by the ignore. A newly surfaced value is a + real finding to address; it is NOT covered by the existing suppression and must not be added + to the ignore file without its own review. + +Fold the outcome of 16a–16c into the Phase 3 change list (entries to remove and why, findings to +apply instead of ignore, and suppressions re-confirmed as still valid). + ## Phase 3: Prepare Change List 17. Consolidate ALL findings (script-detected + review-detected + release-note-informed) into a @@ -146,6 +184,13 @@ mentioned in release notes or the diff report are most likely to have issues. - Naming inconsistencies between spec and SDK - Behavioral changes from release notes that don't require code changes + **Suppression re-verification (from Phase 2 steps 16a–16c):** + - Ignore entries to REMOVE from `specs/audit-ignore.json` — stale, or whose reason no longer + holds. If removing an entry resurfaces a real finding, also list that finding under Must Fix + or Should Fix as appropriate. + - Newly surfaced enum values not covered by an existing suppression (treat as Should Fix). + - Suppressions re-confirmed as still valid (note briefly; no action needed). + Each item MUST include: - Category (Must Fix / Should Fix / Informational) - Description of the issue diff --git a/CLAUDE.md b/CLAUDE.md index 45134f2..2ab6483 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -190,6 +190,18 @@ Uses `VERSION_BUMP_TOKEN` (fine-grained PAT with Contents:Read/Write) to push ve | `scripts/generate_release_notes.py` | Generates changelog from git commits (used by CI) | | `scripts/fetch_spec.py` | Downloads latest Etsy OAS spec to `specs/latest.json` | | `scripts/diff_spec.py` | Diffs `specs/baseline.json` vs `specs/latest.json`, outputs `specs/diff-report.md` | -| `scripts/audit_sdk.py` | Audits SDK coverage against OAS spec, outputs `specs/audit-report.md` | +| `scripts/audit_sdk.py` | Audits SDK coverage against OAS spec, outputs `specs/audit-report.md`. Loads `specs/audit-ignore.json` to suppress reviewed findings (override with `--ignore-file`) | | `scripts/check_releases.py` | Checks Etsy GitHub releases for new changes, outputs `specs/release-notes.md` | | `scripts/format_pr_comment.py` | Formats audit report as a PR comment (used by CI) | + +### Audit Suppressions (`specs/audit-ignore.json`) + +Reviewed, accepted audit findings (deliberate deprecated aliases, intentionally +partial enums, etc.) live in `specs/audit-ignore.json` — never hard-coded in +`audit_sdk.py`. Each run re-derives findings and **only suppresses an entry while +its finding still occurs**; for enums, only the listed `values` are hidden, so a +newly added value still surfaces. Entries matching nothing are reported under a +**Stale Ignores** section so the list stays honest, and suppressed findings are +listed (with reasons) under **Suppressed (Verified)**. To accept a finding, add an +entry (`type` + `key`, plus `direction`/`values` for `enum_staleness`); to stop +accepting it, delete the entry. A missing file means "no suppressions". diff --git a/scripts/audit_sdk.py b/scripts/audit_sdk.py index b7a83b3..f54aaba 100644 --- a/scripts/audit_sdk.py +++ b/scripts/audit_sdk.py @@ -310,10 +310,15 @@ def scan_init_exports(init_path: Path) -> Set[str]: def scan_string_concat_issues(directory: Path) -> List[dict]: - """Detect implicit string concatenation in list literals inside .py files. - - Two adjacent STRING tokens without a comma between them indicate - accidental concatenation like: "foo" "bar" -> "foobar". + """Detect implicit string concatenation inside list literals in .py files. + + Two adjacent STRING tokens with no comma between them silently concatenate + ("foo" "bar" -> "foobar"). Inside a list display this almost always means a + missing comma between intended elements (e.g. a ``nullable``/``mandatory`` + list) — a real bug. Elsewhere (parenthesised assignments, function-call + arguments) the same construct is the idiomatic way to split one long string + across lines, so only occurrences whose innermost enclosing bracket is a + square bracket are flagged. """ issues = [] for py_file in sorted(directory.glob("*.py")): @@ -325,10 +330,23 @@ def scan_string_concat_issues(directory: Path) -> List[dict]: except (tokenize.TokenError, SyntaxError): continue + bracket_stack: List[str] = [] prev_tok = None for tok in tokens: - if tok.type == tokenize.STRING: - if prev_tok is not None and prev_tok.type == tokenize.STRING: + if tok.type == tokenize.OP and tok.string in "([{": + bracket_stack.append(tok.string) + prev_tok = None + elif tok.type == tokenize.OP and tok.string in ")]}": + if bracket_stack: + bracket_stack.pop() + prev_tok = None + elif tok.type == tokenize.STRING: + in_list = bool(bracket_stack) and bracket_stack[-1] == "[" + if ( + in_list + and prev_tok is not None + and prev_tok.type == tokenize.STRING + ): issues.append( { "file": py_file.name, @@ -439,13 +457,157 @@ def build_method_index( } +def load_ignores(path: Optional[Path]) -> List[dict]: + """Load reviewed audit suppressions from an external JSON file. + + Returns an empty list when the file is absent or unreadable, so the audit + behaves exactly as if no suppressions were configured. Nothing to ignore is + hard-coded in this module; the accepted exceptions live entirely in the + data file (default: ``specs/audit-ignore.json``). + """ + if path is None or not path.exists(): + return [] + try: + data = json.loads(path.read_text(encoding="utf-8")) + except (json.JSONDecodeError, OSError): + return [] + ignores = data.get("ignores", []) if isinstance(data, dict) else [] + return [ig for ig in ignores if isinstance(ig, dict)] + + +def _norm_values(values: Any) -> Set[str]: + """Lower-case and stringify a collection of enum values for comparison.""" + return {str(v).lower() for v in values} + + +def compute_enum_findings( + spec: dict, sdk_enums: Dict[str, List[str]] +) -> List[dict]: + """Compare OAS enum definitions against SDK enum classes. + + Yields one finding per (spec enum field, matched SDK enum, direction) whose + value sets differ. ``direction`` is ``"missing"`` (present in the spec, + absent from the SDK) or ``"extra"`` (present in the SDK, absent from the + spec). ``values`` is a set of lower-cased strings. + """ + findings: List[dict] = [] + spec_enums = get_spec_enums(spec) + for spec_key, spec_values in sorted(spec_enums.items()): + prop_name = spec_key.split(".")[-1] if "." in spec_key else spec_key + expected_enum_name = snake_to_pascal(prop_name) + + best_match = None + if expected_enum_name in sdk_enums: + best_match = expected_enum_name + else: + best_overlap = 0 + for sdk_name, sdk_values in sdk_enums.items(): + sdk_value_set = {str(v).lower() for v in sdk_values} + spec_value_set = {str(v).lower() for v in spec_values} + overlap = len(sdk_value_set & spec_value_set) + smaller_set = min(len(sdk_value_set), len(spec_value_set)) + min_required = max(2, smaller_set // 2) + if overlap >= min_required and overlap > best_overlap: + best_overlap = overlap + best_match = sdk_name + + if not best_match: + continue + + sdk_value_set = {str(v).lower() for v in sdk_enums[best_match]} + spec_value_set = {str(v).lower() for v in spec_values} + missing = spec_value_set - sdk_value_set + extra = sdk_value_set - spec_value_set + key = f"{spec_key} -> {best_match}" + for direction, values in (("missing", missing), ("extra", extra)): + if values: + findings.append( + { + "type": "enum_staleness", + "key": key, + "direction": direction, + "values": values, + "sdk_enum": best_match, + "spec_key": spec_key, + } + ) + return findings + + +def partition_findings( + findings: List[dict], ignores: List[dict] +) -> Tuple[List[dict], List[dict], List[dict]]: + """Split findings into active vs suppressed, and surface stale ignores. + + A finding matches an ignore when ``type`` and ``key`` are equal (plus + ``direction`` for enum findings). For enum findings the ignore's ``values`` + is re-verified against the finding's *current* values: ``"*"`` suppresses + all; a list suppresses only those values while any remaining (newly + appeared) values stay active — so a suppression can never silently hide a + value it was not reviewed for. An ignore that suppresses nothing on this run + is returned as stale (its condition no longer exists). + + Returns ``(active, suppressed, stale_ignores)``. Each suppressed entry is + the finding dict with an added ``"ignore"`` key holding the matched entry. + """ + used = [False] * len(ignores) + active: List[dict] = [] + suppressed: List[dict] = [] + + for finding in findings: + match_idx = None + for i, ig in enumerate(ignores): + if ig.get("type") != finding.get("type"): + continue + if ig.get("key") != finding.get("key"): + continue + if finding.get("type") == "enum_staleness" and ig.get( + "direction" + ) != finding.get("direction"): + continue + match_idx = i + break + + if match_idx is None: + active.append(finding) + continue + + ig = ignores[match_idx] + + if finding.get("type") == "enum_staleness" and isinstance( + finding.get("values"), set + ): + ig_values = ig.get("values", "*") + if ig_values == "*": + used[match_idx] = True + suppressed.append({**finding, "ignore": ig}) + continue + ig_set = _norm_values(ig_values) + covered = finding["values"] & ig_set + remaining = finding["values"] - ig_set + if covered: + used[match_idx] = True + suppressed.append({**finding, "values": covered, "ignore": ig}) + if remaining: + active.append({**finding, "values": remaining}) + continue + + used[match_idx] = True + suppressed.append({**finding, "ignore": ig}) + + stale = [ig for i, ig in enumerate(ignores) if not used[i]] + return active, suppressed, stale + + def generate_report( spec: dict, resources_dir: Path, enums_dir: Path, models_dir: Path, + ignores: Optional[List[dict]] = None, ) -> str: """Generate the audit report.""" + ignores = ignores or [] lines = ["# Etsy SDK Audit Report\n"] operations = get_operations(spec) @@ -508,13 +670,43 @@ def generate_report( # Deprecation notices deprecations = detect_description_deprecations(spec) + # --- Build structured findings and apply external suppressions --- + # Each finding carries a stable (type, key) so the ignore engine can match + # and re-verify it every run. Nothing to ignore is hard-coded here; accepted + # exceptions live in specs/audit-ignore.json (see load_ignores). + findings: List[dict] = [] + for method_name, info in sorted(extra_methods): + findings.append( + { + "type": "extra_method", + "key": f"{info['class']}.{method_name}", + "method": method_name, + "info": info, + } + ) + findings.extend(compute_enum_findings(spec, sdk_enums)) + for issue in concat_issues: + findings.append( + { + "type": "code_issue", + "key": f"{issue['file']}::{issue['strings']}", + "issue": issue, + } + ) + + active, suppressed, stale_ignores = partition_findings(findings, ignores) + active_extra = [f for f in active if f["type"] == "extra_method"] + active_enum = [f for f in active if f["type"] == "enum_staleness"] + active_code = [f for f in active if f["type"] == "code_issue"] + lines.append("## Coverage Summary\n") lines.append(f"- Total OAS operations: {total_ops}") lines.append(f"- Mapped to SDK methods: {mapped_count} ({impl_count} implemented, {not_impl_count} stubs)") lines.append(f"- Missing from SDK: {len(unmapped)}") - lines.append(f"- Extra SDK methods (no OAS match): {len(extra_methods)}") - lines.append(f"- Code issues found: {len(concat_issues)}") + lines.append(f"- Extra SDK methods (no OAS match): {len(active_extra)}") + lines.append(f"- Code issues found: {len(active_code)}") lines.append(f"- Missing exports: {len(missing_exports)}") + lines.append(f"- Suppressed (verified): {len(suppressed)}") pct = (impl_count / total_ops * 100) if total_ops > 0 else 0 lines.append(f"- Effective coverage: {pct:.1f}%\n") @@ -562,10 +754,11 @@ def generate_report( # --- Extra SDK Methods --- lines.append("\n## Extra SDK Methods\n") lines.append("SDK methods with no matching OAS operation (possibly removed or renamed).\n") - if extra_methods: - for method_name, info in sorted(extra_methods): + if active_extra: + for f in sorted(active_extra, key=lambda x: x["key"]): + info = f["info"] lines.append( - f"- **{method_name}** in `{info['file']}:{info['line']}` ({info['class']})" + f"- **{f['method']}** in `{info['file']}:{info['line']}` ({info['class']})" ) else: lines.append("No extra methods found.\n") @@ -699,46 +892,24 @@ def generate_report( # --- Enum Staleness --- lines.append("\n## Enum Staleness\n") lines.append("OAS enum values not reflected in SDK enum classes.\n") - spec_enums = get_spec_enums(spec) - any_enum_issues = False - - for spec_key, spec_values in sorted(spec_enums.items()): - # Extract property name from spec key (Schema.property_name) - prop_name = spec_key.split(".")[-1] if "." in spec_key else spec_key - expected_enum_name = snake_to_pascal(prop_name) - - # Try name-based match first - best_match = None - if expected_enum_name in sdk_enums: - best_match = expected_enum_name - else: - # Fall back to overlap matching with threshold - best_overlap = 0 - for sdk_name, sdk_values in sdk_enums.items(): - sdk_value_set = {str(v).lower() for v in sdk_values} - spec_value_set = {str(v).lower() for v in spec_values} - overlap = len(sdk_value_set & spec_value_set) - smaller_set = min(len(sdk_value_set), len(spec_value_set)) - min_required = max(2, smaller_set // 2) - if overlap >= min_required and overlap > best_overlap: - best_overlap = overlap - best_match = sdk_name - - if best_match: - sdk_value_set = {str(v).lower() for v in sdk_enums[best_match]} - spec_value_set = {str(v).lower() for v in spec_values} - missing = spec_value_set - sdk_value_set - extra = sdk_value_set - spec_value_set - if missing or extra: - any_enum_issues = True - lines.append(f"### {spec_key} -> SDK `{best_match}`\n") - if missing: - lines.append(f"- Missing from SDK: {', '.join(sorted(missing))}") - if extra: - lines.append(f"- Extra in SDK: {', '.join(sorted(extra))}") - lines.append("") - - if not any_enum_issues: + if active_enum: + enum_by_key: Dict[str, Dict[str, dict]] = {} + for f in active_enum: + enum_by_key.setdefault(f["key"], {})[f["direction"]] = f + for key in sorted(enum_by_key): + dirs = enum_by_key[key] + sample = next(iter(dirs.values())) + lines.append(f"### {sample['spec_key']} -> SDK `{sample['sdk_enum']}`\n") + if "missing" in dirs: + lines.append( + f"- Missing from SDK: {', '.join(sorted(dirs['missing']['values']))}" + ) + if "extra" in dirs: + lines.append( + f"- Extra in SDK: {', '.join(sorted(dirs['extra']['values']))}" + ) + lines.append("") + else: lines.append("All enum values are in sync.\n") # --- Deprecation Notices --- @@ -762,16 +933,68 @@ def generate_report( # --- Code Issues --- lines.append("\n## Code Issues\n") lines.append("Potential bugs detected by static analysis.\n") - if concat_issues: + if active_code: lines.append("### Implicit String Concatenation\n") lines.append( "Adjacent string literals without a comma — these silently concatenate into a single string.\n" ) - for issue in concat_issues: + for f in active_code: + issue = f["issue"] lines.append(f"- `{issue['file']}:{issue['line']}`: {issue['strings']}") else: lines.append("No code issues found.\n") + # --- Suppressed (Verified) --- + lines.append("\n## Suppressed (Verified)\n") + lines.append( + "Findings matched by an entry in `specs/audit-ignore.json`. Each was reviewed " + "and accepted; the audit re-verifies on every run that the finding still occurs " + "before hiding it, so this list cannot drift away from reality.\n" + ) + if suppressed: + type_titles = { + "extra_method": "Extra SDK Methods", + "enum_staleness": "Enum Staleness", + "code_issue": "Code Issues", + } + suppressed_by_type: Dict[str, List[dict]] = {} + for f in suppressed: + suppressed_by_type.setdefault(f["type"], []).append(f) + for ftype in sorted(suppressed_by_type): + lines.append(f"### {type_titles.get(ftype, ftype)}\n") + for f in suppressed_by_type[ftype]: + reason = f.get("ignore", {}).get("reason", "(no reason given)") + if f["type"] == "enum_staleness": + vals = ", ".join(sorted(f["values"])) + lines.append(f"- `{f['key']}` [{f['direction']}: {vals}] — {reason}") + elif f["type"] == "extra_method": + info = f["info"] + lines.append(f"- `{f['method']}` ({info['class']}) — {reason}") + elif f["type"] == "code_issue": + issue = f["issue"] + lines.append(f"- `{issue['file']}`: {issue['strings']} — {reason}") + else: + lines.append(f"- `{f['key']}` — {reason}") + lines.append("") + else: + lines.append("No findings suppressed.\n") + + # --- Stale Ignores --- + lines.append("\n## Stale Ignores\n") + lines.append( + "Entries in `specs/audit-ignore.json` that matched no current finding — the " + "condition each was created for no longer exists, so the entry can be removed.\n" + ) + if stale_ignores: + for ig in stale_ignores: + direction = f" [{ig['direction']}]" if ig.get("direction") else "" + reason = ig.get("reason", "") + lines.append( + f"- **{ig.get('type', '?')}** `{ig.get('key', '(no key)')}`{direction} — {reason}" + ) + else: + lines.append("No stale ignores.\n") + return "\n".join(lines) @@ -783,6 +1006,12 @@ def main() -> int: default=None, help="Path to OAS spec (default: specs/baseline.json)", ) + parser.add_argument( + "--ignore-file", + type=Path, + default=None, + help="Path to audit suppressions (default: specs/audit-ignore.json)", + ) args = parser.parse_args() project_root = Path(__file__).parent.parent @@ -802,7 +1031,10 @@ def main() -> int: print(f"Error: Failed to parse {spec_path}: {e}") return 1 - report = generate_report(spec, resources_dir, enums_dir, models_dir) + ignore_path = args.ignore_file or project_root / "specs" / "audit-ignore.json" + ignores = load_ignores(ignore_path) + + report = generate_report(spec, resources_dir, enums_dir, models_dir, ignores) print(report) report_path = project_root / "specs" / "audit-report.md" diff --git a/specs/audit-ignore.json b/specs/audit-ignore.json new file mode 100644 index 0000000..3fc60fd --- /dev/null +++ b/specs/audit-ignore.json @@ -0,0 +1,53 @@ +{ + "_README": "Reviewed, accepted audit findings that should not count as noise. scripts/audit_sdk.py loads this file and, on EVERY run, re-derives findings and only suppresses an entry while its finding still occurs (for enums, only the listed values are suppressed; newly appeared values stay active). Entries that match nothing are reported under 'Stale Ignores' so this list stays honest. Nothing here is hard-coded in the script — to accept a finding, add an entry; to stop accepting it, delete the entry. Match fields: 'type' + 'key' (+ 'direction' for enum_staleness). 'values' (enum only): \"*\" = all, or a list of specific values. 'reason'/'added' are documentation.", + "ignores": [ + { + "type": "extra_method", + "key": "ListingResource.get_listings_by_listings_ids", + "reason": "Intentional deprecated alias for get_listings_by_listing_ids; delegates and emits DeprecationWarning.", + "added": "2026-06-02" + }, + { + "type": "extra_method", + "key": "PaymentResource.get_shop_payment_account_ledger_entry_payments", + "reason": "Intentional deprecated alias for get_payment_account_ledger_entry_payments; delegates and emits DeprecationWarning.", + "added": "2026-06-02" + }, + { + "type": "extra_method", + "key": "ReceiptTransactionsResource.get_shop_receipt_transaction_by_shop", + "reason": "Intentional deprecated alias for get_shop_receipt_transactions_by_shop; delegates and emits DeprecationWarning.", + "added": "2026-06-02" + }, + { + "type": "extra_method", + "key": "ShippingProfileResource.get_shop_shipping_profile_destination_by_shipping_profile", + "reason": "Intentional deprecated alias for get_shop_shipping_profile_destinations_by_shipping_profile; delegates and emits DeprecationWarning.", + "added": "2026-06-02" + }, + { + "type": "enum_staleness", + "key": "ShopHolidayPreference.holiday_id -> CA_HOLIDAYS", + "direction": "missing", + "values": "*", + "reason": "By design: the SDK names US_HOLIDAYS (1-11) and CA_HOLIDAYS (12-23) and documents passing integer IDs directly for other regions (24-105), so the full spec enum is intentionally not enumerated. See enums/HolidayPreferences.py.", + "added": "2026-06-02" + }, + { + "type": "enum_staleness", + "key": "ShopListing.state -> State", + "direction": "extra", + "values": ["removed"], + "reason": "State.REMOVED is kept for backward compatibility and is not in the OAS response schema. Documented inline in enums/Listing.py; may be removed in the next major version.", + "added": "2026-06-02" + }, + { + "type": "enum_staleness", + "key": "ShopListingWithAssociations.state -> State", + "direction": "extra", + "values": ["removed"], + "reason": "State.REMOVED is kept for backward compatibility and is not in the OAS response schema. Documented inline in enums/Listing.py; may be removed in the next major version.", + "added": "2026-06-02" + } + ] +} diff --git a/tests/test_audit_ignore.py b/tests/test_audit_ignore.py new file mode 100644 index 0000000..8b09aeb --- /dev/null +++ b/tests/test_audit_ignore.py @@ -0,0 +1,331 @@ +"""Tests for the externalized, self-verifying audit suppression mechanism and +the list-context fix to the implicit-string-concatenation detector in +``scripts/audit_sdk.py``. +""" + +import json +import sys +from pathlib import Path + +import pytest + +# scripts/ is not a package; put it on the path so we can import the audit tool. +SCRIPTS_DIR = Path(__file__).resolve().parent.parent / "scripts" +if str(SCRIPTS_DIR) not in sys.path: + sys.path.insert(0, str(SCRIPTS_DIR)) + +import audit_sdk # noqa: E402 + + +# --------------------------------------------------------------------------- # +# load_ignores +# --------------------------------------------------------------------------- # +class TestLoadIgnores: + def test_missing_file_returns_empty(self, tmp_path): + assert audit_sdk.load_ignores(tmp_path / "nope.json") == [] + + def test_none_path_returns_empty(self): + assert audit_sdk.load_ignores(None) == [] + + def test_malformed_json_returns_empty(self, tmp_path): + p = tmp_path / "bad.json" + p.write_text("{not valid json", encoding="utf-8") + assert audit_sdk.load_ignores(p) == [] + + def test_valid_file_parsed(self, tmp_path): + p = tmp_path / "ignore.json" + p.write_text( + json.dumps( + {"ignores": [{"type": "extra_method", "key": "A.b", "reason": "x"}]} + ), + encoding="utf-8", + ) + ignores = audit_sdk.load_ignores(p) + assert len(ignores) == 1 + assert ignores[0]["key"] == "A.b" + + def test_non_dict_entries_filtered_out(self, tmp_path): + p = tmp_path / "ignore.json" + p.write_text( + json.dumps({"ignores": [{"type": "extra_method", "key": "A.b"}, "garbage"]}), + encoding="utf-8", + ) + assert audit_sdk.load_ignores(p) == [{"type": "extra_method", "key": "A.b"}] + + def test_missing_ignores_key_returns_empty(self, tmp_path): + p = tmp_path / "ignore.json" + p.write_text(json.dumps({"something_else": []}), encoding="utf-8") + assert audit_sdk.load_ignores(p) == [] + + +# --------------------------------------------------------------------------- # +# partition_findings — atomic findings +# --------------------------------------------------------------------------- # +class TestPartitionAtomic: + def test_unmatched_finding_stays_active(self): + findings = [{"type": "extra_method", "key": "A.b"}] + active, suppressed, stale = audit_sdk.partition_findings(findings, []) + assert active == findings + assert suppressed == [] + assert stale == [] + + def test_matched_finding_is_suppressed(self): + findings = [{"type": "extra_method", "key": "A.b"}] + ignores = [{"type": "extra_method", "key": "A.b", "reason": "alias"}] + active, suppressed, stale = audit_sdk.partition_findings(findings, ignores) + assert active == [] + assert len(suppressed) == 1 + assert suppressed[0]["ignore"]["reason"] == "alias" + assert stale == [] + + def test_type_mismatch_does_not_match(self): + findings = [{"type": "extra_method", "key": "A.b"}] + ignores = [{"type": "code_issue", "key": "A.b"}] + active, suppressed, stale = audit_sdk.partition_findings(findings, ignores) + assert active == findings + assert suppressed == [] + assert len(stale) == 1 # the ignore matched nothing + + def test_key_mismatch_does_not_match(self): + findings = [{"type": "extra_method", "key": "A.b"}] + ignores = [{"type": "extra_method", "key": "A.c"}] + active, _, stale = audit_sdk.partition_findings(findings, ignores) + assert active == findings + assert len(stale) == 1 + + +# --------------------------------------------------------------------------- # +# partition_findings — enum value verification +# --------------------------------------------------------------------------- # +class TestPartitionEnum: + def _enum(self, direction, values): + return { + "type": "enum_staleness", + "key": "Foo.state -> State", + "direction": direction, + "values": set(values), + "sdk_enum": "State", + "spec_key": "Foo.state", + } + + def test_wildcard_suppresses_all(self): + findings = [self._enum("missing", {"1", "2", "3"})] + ignores = [ + { + "type": "enum_staleness", + "key": "Foo.state -> State", + "direction": "missing", + "values": "*", + } + ] + active, suppressed, stale = audit_sdk.partition_findings(findings, ignores) + assert active == [] + assert len(suppressed) == 1 + assert stale == [] + + def test_listed_values_suppressed_exactly(self): + findings = [self._enum("extra", {"removed"})] + ignores = [ + { + "type": "enum_staleness", + "key": "Foo.state -> State", + "direction": "extra", + "values": ["removed"], + } + ] + active, suppressed, stale = audit_sdk.partition_findings(findings, ignores) + assert active == [] + assert len(suppressed) == 1 + assert suppressed[0]["values"] == {"removed"} + assert stale == [] + + def test_new_value_stays_active_while_known_value_suppressed(self): + # Ignore covers "removed"; a newly-appeared "archived" must NOT be hidden. + findings = [self._enum("extra", {"removed", "archived"})] + ignores = [ + { + "type": "enum_staleness", + "key": "Foo.state -> State", + "direction": "extra", + "values": ["removed"], + } + ] + active, suppressed, stale = audit_sdk.partition_findings(findings, ignores) + assert len(active) == 1 + assert active[0]["values"] == {"archived"} + assert len(suppressed) == 1 + assert suppressed[0]["values"] == {"removed"} + assert stale == [] # the ignore did suppress "removed", so it is not stale + + def test_direction_mismatch_is_not_suppressed(self): + findings = [self._enum("extra", {"removed"})] + ignores = [ + { + "type": "enum_staleness", + "key": "Foo.state -> State", + "direction": "missing", + "values": "*", + } + ] + active, suppressed, stale = audit_sdk.partition_findings(findings, ignores) + assert len(active) == 1 + assert suppressed == [] + assert len(stale) == 1 + + def test_value_case_insensitive(self): + findings = [self._enum("extra", {"removed"})] + ignores = [ + { + "type": "enum_staleness", + "key": "Foo.state -> State", + "direction": "extra", + "values": ["REMOVED"], + } + ] + active, suppressed, _ = audit_sdk.partition_findings(findings, ignores) + assert active == [] + assert len(suppressed) == 1 + + +# --------------------------------------------------------------------------- # +# partition_findings — stale tracking +# --------------------------------------------------------------------------- # +class TestStaleTracking: + def test_used_entry_not_stale_unused_entry_stale(self): + findings = [{"type": "extra_method", "key": "A.b"}] + ignores = [ + {"type": "extra_method", "key": "A.b", "reason": "used"}, + {"type": "extra_method", "key": "X.y", "reason": "stale"}, + ] + active, suppressed, stale = audit_sdk.partition_findings(findings, ignores) + assert active == [] + assert len(suppressed) == 1 + assert len(stale) == 1 + assert stale[0]["reason"] == "stale" + + def test_partial_enum_ignore_is_not_stale(self): + finding = { + "type": "enum_staleness", + "key": "Foo.state -> State", + "direction": "extra", + "values": {"removed", "new"}, + "sdk_enum": "State", + "spec_key": "Foo.state", + } + ignores = [ + { + "type": "enum_staleness", + "key": "Foo.state -> State", + "direction": "extra", + "values": ["removed"], + } + ] + _, _, stale = audit_sdk.partition_findings([finding], ignores) + assert stale == [] + + +# --------------------------------------------------------------------------- # +# compute_enum_findings +# --------------------------------------------------------------------------- # +class TestComputeEnumFindings: + def test_missing_value_detected(self): + spec = { + "components": { + "schemas": {"Foo": {"properties": {"color": {"enum": ["red", "green", "blue"]}}}} + } + } + findings = audit_sdk.compute_enum_findings(spec, {"Color": ["red", "green"]}) + assert len(findings) == 1 + f = findings[0] + assert f["direction"] == "missing" + assert f["values"] == {"blue"} + assert f["key"] == "Foo.color -> Color" + + def test_extra_value_detected(self): + spec = { + "components": { + "schemas": {"Foo": {"properties": {"color": {"enum": ["red", "green"]}}}} + } + } + findings = audit_sdk.compute_enum_findings( + spec, {"Color": ["red", "green", "purple"]} + ) + assert len(findings) == 1 + assert findings[0]["direction"] == "extra" + assert findings[0]["values"] == {"purple"} + + def test_in_sync_yields_no_findings(self): + spec = { + "components": { + "schemas": {"Foo": {"properties": {"color": {"enum": ["red", "green"]}}}} + } + } + assert audit_sdk.compute_enum_findings(spec, {"Color": ["red", "green"]}) == [] + + +# --------------------------------------------------------------------------- # +# scan_string_concat_issues — list-context detection +# --------------------------------------------------------------------------- # +class TestStringConcatDetector: + def _write(self, tmp_path, name, code): + (tmp_path / name).write_text(code, encoding="utf-8") + + def test_missing_comma_in_list_is_flagged(self, tmp_path): + self._write(tmp_path, "buggy.py", 'nullable = ["a", "b" "c"]\n') + issues = audit_sdk.scan_string_concat_issues(tmp_path) + assert len(issues) == 1 + assert issues[0]["file"] == "buggy.py" + + def test_multiline_list_missing_comma_is_flagged(self, tmp_path): + self._write( + tmp_path, + "buggy2.py", + 'mandatory = [\n "first"\n "second",\n "third",\n]\n', + ) + issues = audit_sdk.scan_string_concat_issues(tmp_path) + assert len(issues) == 1 + + def test_parenthesised_assignment_not_flagged(self, tmp_path): + self._write( + tmp_path, + "intentional.py", + 'MSG = (\n "long part one "\n "long part two"\n)\n', + ) + assert audit_sdk.scan_string_concat_issues(tmp_path) == [] + + def test_function_call_args_not_flagged(self, tmp_path): + self._write( + tmp_path, + "warn.py", + 'import warnings\n' + 'def f():\n' + ' warnings.warn("alpha " "beta", DeprecationWarning)\n', + ) + assert audit_sdk.scan_string_concat_issues(tmp_path) == [] + + def test_proper_list_with_commas_not_flagged(self, tmp_path): + self._write(tmp_path, "clean.py", 'items = ["a", "b", "c"]\n') + assert audit_sdk.scan_string_concat_issues(tmp_path) == [] + + def test_concat_in_nested_list_inside_call_is_flagged(self, tmp_path): + self._write(tmp_path, "nested.py", 'foo(["x" "y"])\n') + issues = audit_sdk.scan_string_concat_issues(tmp_path) + assert len(issues) == 1 + + def test_dunder_files_skipped(self, tmp_path): + self._write(tmp_path, "__init__.py", 'x = ["a" "b"]\n') + assert audit_sdk.scan_string_concat_issues(tmp_path) == [] + + +# --------------------------------------------------------------------------- # +# Integration: the shipped ignore file silences exactly the known findings +# --------------------------------------------------------------------------- # +class TestShippedIgnoreFile: + def test_shipped_ignore_file_is_valid_and_nonempty(self): + path = SCRIPTS_DIR.parent / "specs" / "audit-ignore.json" + ignores = audit_sdk.load_ignores(path) + assert len(ignores) >= 1 + for ig in ignores: + assert "type" in ig and "key" in ig and "reason" in ig + if ig["type"] == "enum_staleness": + assert "direction" in ig and "values" in ig